New session handling #104
New session handling #104
Conversation
There might not be any client information at this time. This is an effect of automatic client registration as defined in the OIDC federation specification.
…classes that are built on this.
…endpoint into new_session_handling
Now, for documentation.
A token must have a value.
Added types.
| sub=user, | ||
| sid=sid, | ||
| sid=session_id, | ||
| state=request["state"], |
There was a problem hiding this comment.
request.get("state")state is optional
There was a problem hiding this comment.
state is recommendend in oidc ...
There was a problem hiding this comment.
But not required. Hence if it's there in the request we add it to the cookie info otherwise it will just be ignored.
| authn_endpoint = endpoint.get("authorization") | ||
| if authn_endpoint is None: | ||
| LOGGER.warning( | ||
| "No authorization endpoint found, skipping PKCE configuration" | ||
| ) | ||
| return | ||
|
|
||
| token_endpoint = endpoint.get("token") | ||
| if token_endpoint is None: | ||
| LOGGER.warning( | ||
| "No token endpoint found, skipping PKCE configuration" | ||
| ) | ||
| return | ||
|
|
||
| authn_endpoint.post_parse_request.append(post_authn_parse) |
There was a problem hiding this comment.
Why was this removed?
There was a problem hiding this comment.
probably a better path for this module would be src/oidcendpoint/oauth2/add_on/pkce.py as it belongs to OAuth2 and can be used even in oidc
| if authn_time: | ||
| args["authn_time"] = authn_time | ||
| else: | ||
| _ts = kwargs.get("timestamp") |
There was a problem hiding this comment.
what would be a default value on _ts?
| _conf = self.conf.get("userinfo") | ||
| if _conf: | ||
| if self.sdb: | ||
| if self.session_manager: |
There was a problem hiding this comment.
yes "session_manager" is more readable than sdb! 👌
| code) | ||
| _response["refresh_token"] = refresh_token.value | ||
|
|
||
| code.register_usage() |
There was a problem hiding this comment.
Mentioning @rohe because this conversation has been lost after some pushes.
| grant_type_options in (None, True) | ||
| and grant_type in HELPER_BY_GRANT_TYPE | ||
| ): | ||
| self.helper[grant_type] = HELPER_BY_GRANT_TYPE[grant_type] |
There was a problem hiding this comment.
The class will need to be initialized ->
self.helper[grant_type] = HELPER_BY_GRANT_TYPE[grant_type](self)
| }, | ||
| }, | ||
| { | ||
| "authorization_code": True, # Both True and None end up using the defaults |
There was a problem hiding this comment.
These tests were removed and would catch the bug I reviewd above relating to the initialization of the helper endpoint classes.
| return user_info | ||
| else: | ||
| if client_id not in user_info['subordinate']: | ||
| raise ValueError('No session from that client for that user') |
There was a problem hiding this comment.
raise NoSuchGrant (or even NoSuchClientSession)
| :return: | ||
| """ | ||
| if isinstance(request, AuthorizationErrorResponse): | ||
| return request |
There was a problem hiding this comment.
This is probably not strictly related to this PR but could be fixed here anyway. A check should be added here for the grant type because we don't want to check for code in a refresh token grant or a token exchange one.
Or we could check for the request instance like this (maybe strictly checking for the grant type to be authorization_code is cleaner tho):
if isinstance(
request,
(AuthorizationErrorResponse, RefreshAccessTokenRequest, TokenExchangeRequest),
):
|
This PR has been open for long enough. We should close it but there are still some open issues If @rohe is ok with it I can push the fixes required and you can review them so that we can finally close this PR. |
|
Let's !! I've been side tracked by the persistent storage handling. |
The access_token handler was used to mint auth codes
New session handling
Fix bug, default token_handler to None
We already have the grant in most cases so we can fetch the token from inside it instead of querying the DB again.
WIP: Make more fixes to the new_session_handling before it gets merged
@rohe mentioned that the new session management system is ready and that we should start discussing whether it should be merged and if any changes are needed.
So I am taking the liberty to create this PR to discuss any changes we see necessary.