Add authorization_code and password based OAuth login handlers #5547
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5547 +/- ##
==========================================
- Coverage 83.44% 82.33% -1.12%
==========================================
Files 275 275
Lines 39395 39486 +91
==========================================
- Hits 32873 32510 -363
- Misses 6522 6976 +454
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 15 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
philippjfr
force-pushed
the
password_grant_oauth
branch
from
4459cc6 to
bfa0b4f
Compare
philippjfr
changed the title
|
If its easy to make it mandatory for specific pages or optional the it would be really valuable. |
|
@cdeil Would love it if you guys could try this one out on your end. |
| @@ -12,6 +12,9 @@ The first step in configuring a OAuth is to specify a specific OAuth provider. P | |||
| * `gitlab`: GitLab | |||
| * `google`: Google | |||
| * `okta`: Okta | |||
| * `generic`: Generic OAuth Provider with configurable endpoints | |||
| * `password`: Generic password grant based OAuth Provider with configurable endpoints | |||
| * `code`: Generic code challenge grant based OAuth Provider with configurable endpoints | |||
There was a problem hiding this comment.
code might be somewhat ambiguous name, since there is also a device code flow. What about auth_code? Right, @armaaar ?
There was a problem hiding this comment.
I agree. There is also an OAuth flow called Device Code flow: https://oauth.net/2/grant-types/device-code/
This name might cause confusion
There was a problem hiding this comment.
Makes sense to me, thanks!
There was a problem hiding this comment.
Maybe link from the docs here or elsewhere to the API docs to make it easy to explore the details where reading this?
With search it's possible to find e.g. panel.auth.GenericLoginHandler and other relevant classes, but at least for me having a link to click when reading here would be nice to have.
| - `TOKEN_URL`: The token endpoint of the authentication server, may also be provided using the `PANEL_OAUTH_TOKEN_URL` environment variable. | ||
| - `USER_URL`: The user information endpoint of the authentication server, may also be provided using the `PANEL_OAUTH_USER_URL` environment variable. | ||
|
|
||
| The difference between these three providers is the authentication flow they perform. The `generic` provider uses the standard authentication flow, which requests authorization using the client secret, while the `password` based workflow lets the user log in via a form served on the server (only recommended for testing and development), and the `code` uses a code challenge based auth flow. |
There was a problem hiding this comment.
There is nothing call "standard authentication flow", I guess you mean client credentials flow: https://oauth.net/2/grant-types/client-credentials/
There was a problem hiding this comment.
Probably going to split this guide up into separate pages for the different grant types to clarify this.
There was a problem hiding this comment.
We get this error
Uncaught exception GET /auth (::1)
HTTPServerRequest(protocol='http', host='localhost:5005', method='GET', uri='/auth', version='HTTP/1.1', remote_ip='::1')
Traceback (most recent call last):
File "***/lib/python3.10/site-packages/tornado/web.py", line 1784, in _execute
result = method(*self.path_args, **self.path_kwargs)
File "***/lib/python3.10/site-packages/tornado/web.py", line 3276, in wrapper
if not self.current_user:
File "***/lib/python3.10/site-packages/tornado/web.py", line 1420, in current_user
self._current_user = self.get_current_user()
File "***/lib/python3.10/site-packages/bokeh/server/views/auth_request_handler.py", line 71, in get_current_user
return self.application.auth_provider.get_user(self)
File "***/panel/auth.py", line 1038, in get_user
return request_handler.get_secure_cookie("user", max_age_days=config.oauth_expiry)
File "***/lib/python3.10/site-packages/tornado/web.py", line 836, in get_signed_cookie
self.require_setting("cookie_secret", "secure cookies")
File "***/lib/python3.10/site-packages/tornado/web.py", line 1669, in require_setting
raise Exception(
Exception: You must define the 'cookie_secret' setting in your application to use secure cookies
500 GET /auth (::1) 22.92ms
I thought you create this by default. Do we need to always supply it? if so, shouldn't it be documented that this is required?
|
The solution doesn't work for us. It results in many redirects: 
Here is the final URL: |
|
We have a huge problem with not being able to logout.
Now if we assume that there is a logout button, have a look into this scenario:
Just clearing cookies, and not allowing viewing any page without being authenticated, leads to always being logged in after the first login attempt. without the ability to ever logout. This is unacceptable for us. It also prevents people with multiple accounts to view anything as they are locked to the first account they logged in with |
| user_headers = dict(self._API_BASE_HEADERS) | ||
| if self._access_token_header: | ||
| user_url = self._OAUTH_USER_URL | ||
| user_headers['Authorization'] = self._access_token_header.format( | ||
| body['access_token'] | ||
| ) | ||
| else: | ||
| user_url = '{}{}'.format(self._OAUTH_USER_URL, body['access_token']) | ||
|
|
||
| user_response = await http.fetch(user_url, headers=user_headers) | ||
| user = decode_response_body(user_response) |
There was a problem hiding this comment.
In our use case, The user url is a custom URL which uses one of the decoded properties of the access token as one of its path parameters. We can't supply a fixed url.
We actually don't care about the user info so much, either Make this optional, or provide a way (like a hook) to set the user info ourselves
There was a problem hiding this comment.
I'll have to retest this but when I tried it with your standard endpoint I got back a valid response containing my user information.
There was a problem hiding this comment.
Multiple users from our end return a 403 forbidden response
There was a problem hiding this comment.
Did you set openid as one of the scopes?
There was a problem hiding this comment.
I couldn't reach that point due to the infinite redirects bug
There was a problem hiding this comment.
I still can't reproduce that, running https://github.com/heidelbergcement/hm-panel-training-2023/issues/13 works fine for me.
|
Appreciate the review comments, all reasonable requests. |
|
Going to merge this PR and then continue improving logout and access_token refresh handling as well as the docs in another PR. |
So far our OAuth login handlers all used authorization flows that required a
client_secret. Our new workflow works by using the code challenge auth flow, which redirects the user to a log in page along with a SHA encoded code challenge that is then verified against the verification code that is stored in a cookie.We also add a handler for the password based grant types, which is simple to set up for development but should generally not be used.