-
-
Notifications
You must be signed in to change notification settings - Fork 480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OpenID connect improvements #484
Conversation
Nice, I also ran into some missing bits in this area but worked around them in a different way. I'll need to dig up what I did exactly, but do believe we need something that looks like this PR. |
@wiliamsouza Can you please rebase and fix the conflicts? |
…ent in request.response_type
Changes AuthorizationEndpoint response_type `'token'`, `'id_token'` and `'id_token token'` to work with OpenID Connect and OAuth2 implicit flow in a transparent way
Change AuthorizationEndpoint grant_types `'authorization_code'` to work with OpenID Connect and OAuth2 authorization flow in a transparent way
Now OpenID Connect and OAuth2 authorization flow can use `authorization_code` in a transparent way
a8fe881
to
9fb35bb
Compare
@thedrow Done! |
|
||
# In OIDC implicit flow it is possible to have a request_type that does | ||
# not include the access_token! In this case there is no need to save a token. | ||
if "token" in request.response_type.split(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinda expensive to split the string into a list just to check if token is in it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
split()
can not be removed because:
ipdb> 'token' in 'id_token'
True
ipdb> 'token' in ['id_token']
False
When using implicit flow:
http://127.0.0.1:8000/o/authorize?response_type=id_token&client_id=Ktn0Sh4hO2gA8PKC2aqsauY4ZCxNyIdF1wNfFfJ3&redirect_uri=http://localhost/callback&scope=openid&state=af0ifjsldkj&nonce=n-0S6_WzA2Mj
without split
it return access_token
instead of only returning id_token
as requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it and it doesn't break my implementation. I've added some questions inline.
|
||
def test_create_token_response_openid_without_code(self): | ||
handler = self.dispatcher._handler_for_request(self.request) | ||
self.assertTrue(isinstance(handler, AuthorizationCodeGrant)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to assert here (and below) how the mocked get_authorization_code_scopes()
method was called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 34aa9b0
'code id_token': openid_connect_auth, | ||
'code token id_token': openid_connect_auth, | ||
'token': implicit_grant_choice, | ||
'id_token': implicit_grant_choice, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should id_token
and id_token token
not point to the openid_connect_implicit
grant directly?
handler = self.default_token_grant | ||
scopes = () | ||
parameters = dict(request.decoded_body) | ||
client_id = parameters.get('client_id', '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do client_id
and redirect_uri
default to an empty string instead of None
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing special, changed to None
, thanks.
redirect_uri = parameters.get('redirect_uri', '') | ||
|
||
# If code is not pressent fallback to `default_token_grant` wich will | ||
# rase an error for the missing `code` in `create_token_response` step. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: rase
Is this ready for merge? |
@skion can answer the question! |
# If code is not pressent fallback to `default_token_grant` wich will | ||
# raise an error for the missing `code` in `create_token_response` step. | ||
if code: | ||
scopes = self.request_validator.get_authorization_code_scopes(client_id, code, redirect_uri) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I don't particularly like, but also not really sure how to do differently, is that this forces to lookup the grant twice; once in get_authorization_code_scopes()
and once in validate_code()
. Not ideal for performance.
I wonder if we should pass in the request
object here, as with many of the other validator methods. Then at least one can choose to attach the result of the first lookup to the request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a gap between stateless endpoints:
- Authorization Endpoint
- Token Endpoint
request_validator.get_authorization_code_scopes()
is called before request_validator.validate_code()
which is used inside AuthorizationCodeGrant.validate_token_request()
we can call request_validator.validate_code()
here and pass in request.code_validate
a True
value indicating the code was already validated and in AuthorizationCodeGrant.validate_token_request()
line 410 add a check before call request_validator.validate_code()
again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess what I meant was: Could we use the following signature instead?
def get_authorization_code_scopes(self, client_id, code, redirect_uri, request):
This seems to be a common pattern already, and it would allow us to cache database lookups on the request
object easily across validation calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@skion done
34aa9b0
to
76c08b7
Compare
LGTM! |
@thedrow ready for merge! |
Thanks. Is this a breaking change? Just so I'll know how to tag the release. |
@thedrow Yes not backward compatible. |
Turns out this leads to problems with existing code that doesn't have To fix we could catch the |
Oh this should have been released as 3.x :( |
@thedrow we need to add a |
Can we please back this out? It's doing more harm than good right now. I love the idea of OpenIDConnect support, but I also think it should be optional and not shoved into the library without consideration of servers/providers/clients that don't want it or don't need it. My opinion is to let the OAuth2 library do Oauth2, add the extras openid connect wants on top with a library of its own that uses the openid2 library, and keep a good separation of concerns so the code stays as SOLID and DRY as possible. |
@duaneking Not so easy to back out OIDC entirely, since its support was added long before this PR already, and mostly modular/separate from the existing code. The exception being the preconfigured server code, which I agree would have been better to leave alone and add a dedicated OIDC server. But undoing this now would also be a breaking change. IMO this PR should not have been a breaking change at all, and my suggestion would be: Find a fix to make this non-breaking and release 2.0.6, and plan for more structural changes in 3.0. |
Is there no way to remove 2.0.5 from the history? |
@wiliamsouza no. But there is a 2.0.6 in pypi now. I cherry picked every commits except this one. |
@lepture Great job! |
Can we reopen this PR or release 3.0 with the relevant commit? |
@wiliamsouza Would this be backward compatible if we just add this line 78 back in? |
This pull request removes the need of using
grant_type=openid
in token endpoint and when defining a application credential theauthorization_grant_type
can be same already used (authorization-code
) both for OpenID Connect and OAuth2.OpenID Connect and OAuth2 specification links:
To achieve this a new method
get_authorization_code_scopes
have to be added toRequestValidator
along side news dispatches for implicit and token grant.Example old way to send a token request:
Example fixed way to send a token request:
I successfully tested using django-oauth-toolkit the following flows:
Adding support to OpenID Connect is a working in progress based on this pull request.