search

IdentityPython / oidc-op

An implementation of an OIDC Provider (OP)
Apache License 2.0
64 stars 26 forks source link

Allow requesting for scopes in refresh #85

Closed nsklikas closed 3 years ago

nsklikas commented 3 years ago

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.

rohe commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

rohe commented 3 years ago

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

nsklikas commented 3 years ago

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

Should be fine now

rohe commented 3 years ago

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

nsklikas commented 3 years ago

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

peppelinux commented 3 years ago

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