Skip to content
This repository was archived by the owner on Jun 23, 2023. It is now read-only.

Conversation

nsklikas
Copy link
Contributor

@nsklikas nsklikas commented Jun 2, 2021

Implement the scope parameter for refresh tokens as described in https://datatracker.ietf.org/doc/html/rfc6749#section-6

Also added the offline_access scope logic to oauth2 token endpoint.

If a client removes the openid scope from a refresh token request then a new ID token will not be issued.
If a client removes the offline_access scope from a refresh token request then a new refresh token will not be issued.

@nsklikas nsklikas requested review from peppelinux and rohe June 2, 2021 15:32
@rohe
Copy link
Collaborator

rohe commented Jun 3, 2021

I like the notion that scope can influence what tokens are minted.
Not so sure about having OIDC concepts moving into OAuth2. What's the reasoning behind this ?

@peppelinux
Copy link
Member

@rohe I think that covering all the OAuth2 aspects in the domain od access_token/refresh_token will gain a finest posture to oidc-op in what are the capabilities and the compliances to standard. After all OAuth2 is the basis of OIDC and too often OIDC implementations seem to forget about it, I think this aspect is really useful in making oidc-op an authentic killer-application!

The changes @nsklikas made seems very reasonably to me and doesn't show any breakages to OIDC profile as well.
we may share any critical issues here if we agree

@nsklikas
Copy link
Contributor Author

nsklikas commented Jun 3, 2021

I agree that OIDC concepts should not be moved into OAuth2. I think that this was implemented properly.
The openid logic was only implemented in oidc/token.py, but the offline_access was implemented in both oidc/token.py and oauth2/token.py

Am I missing something?

@peppelinux
Copy link
Member

I thought on this, overall LGTM but I think that a refresh token MUST only refresh an accass_token and another refresh and not an IDToken.

If we should get again user attributes then we'll request userinfo endpoint with the new access token.

Why should we refresh an IDToken?

@rohe
Copy link
Collaborator

rohe commented Jun 7, 2021

According to the standard doing a refresh using a refresh token may result in minting a new ID Token as well as new access and refresh tokens. So, we must provide that functionality.
Whether we think it's a good idea or could better be handled in an another maner is immaterial.

@peppelinux
Copy link
Member

Ok, so this PR looks good to me as It is

@rohe
Copy link
Collaborator

rohe commented Jun 7, 2021

@nsklikas No, you're right (if I interpret you correctly) we should remove references to offline_access from oauth2/token.py

@nsklikas
Copy link
Contributor Author

nsklikas commented Jun 7, 2021

You are right @rohe, I didn't notice offline_access is not mentioned in the OAuth2 spec.

Should be fine now

@rohe
Copy link
Collaborator

rohe commented Jun 7, 2021

Tests are failing. Have not tried to find out why.

@nsklikas nsklikas force-pushed the feature-refresh-scopes branch from b6fc557 to b6b8658 Compare June 7, 2021 15:56
@nsklikas
Copy link
Contributor Author

nsklikas commented Jun 7, 2021

Forgot to remove offline_access logic from oauth2 tests. Now they run fine.

@peppelinux
Copy link
Member

thank goodness you are here, I had totally lost the thread of the narrative

@peppelinux peppelinux merged commit 155ef33 into IdentityPython:develop Jun 8, 2021
@peppelinux peppelinux mentioned this pull request Jun 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants