Skip to content
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

Merged
merged 12 commits into from Oct 1, 2017

Conversation

Projects
None yet
5 participants
@wiliamsouza
Copy link
Member

commented Sep 6, 2017

This pull request removes the need of using grant_type=openid in token endpoint and when defining a application credential the authorization_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 to RequestValidator along side news dispatches for implicit and token grant.

Example old way to send a token request:

curl -X POST \
    -H "Cache-Control: no-cache" \
    -H "Content-Type: application/x-www-form-urlencoded" \
    "http://127.0.0.1:8000/o/token/" \
    -d "client_id=qLSq5KfSvYRho..." \
    -d "client_secret=bhOWtOxm1..." \
    -d "code=B8J3PQBpDeHFohgrVnvs26KrTAMNhI" \
    -d "redirect_uri=http://localhost/callback" \
    -d "grant_type=openid"

Example fixed way to send a token request:

curl -X POST \
    -H "Cache-Control: no-cache" \
    -H "Content-Type: application/x-www-form-urlencoded" \
    "http://127.0.0.1:8000/o/token/" \
    -d "client_id=qLSq5KfSvYRho..." \
    -d "client_secret=bhOWtOxm1..." \
    -d "code=B8J3PQBpDeHFohgrVnvs26KrTAMNhI" \
    -d "redirect_uri=http://localhost/callback" \
    -d "grant_type=authorization_code"

I successfully tested using django-oauth-toolkit the following flows:

  • Implicit Flow
    • Returning only access_token
    • Returning only id_token
    • Return access_token and id_token
  • Authorization Flow
    • Returning only access_token
    • Returning id_token
  • Hybrid flow
    • Returning code and id_token
    • Returning code and access_token
    • Returning code, id_token and access_token
  • Client credentials
  • Resource owner password based

Adding support to OpenID Connect is a working in progress based on this pull request.

@skion

This comment has been minimized.

Copy link
Member

commented Sep 12, 2017

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.

@thedrow

This comment has been minimized.

Copy link
Collaborator

commented Sep 17, 2017

@wiliamsouza Can you please rebase and fix the conflicts?

wiliamsouza added some commits Sep 4, 2017

Add new ImplicitTokenGrantDispatcher
Changes AuthorizationEndpoint response_type `'token'`, `'id_token'` and
`'id_token token'` to work with OpenID Connect and OAuth2 implicit flow
in a transparent way
Add new AuthTokenGrantDispatcher
Change AuthorizationEndpoint grant_types `'authorization_code'` to work with
OpenID Connect and OAuth2 authorization flow in a transparent way
Remove AuthorizationEndpoint grant_types `'openid'`
Now OpenID Connect and OAuth2 authorization flow can use `authorization_code`
in a transparent way

@wiliamsouza wiliamsouza force-pushed the wiliamsouza:openid-connect branch to 9fb35bb Sep 17, 2017

@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2017

@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():

This comment has been minimized.

Copy link
@thedrow

thedrow Sep 18, 2017

Collaborator

Kinda expensive to split the string into a list just to check if token is in it.

This comment has been minimized.

Copy link
@wiliamsouza

wiliamsouza Sep 24, 2017

Author Member

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.

@skion
Copy link
Member

left a comment

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))

This comment has been minimized.

Copy link
@skion

skion Sep 19, 2017

Member

Is it possible to assert here (and below) how the mocked get_authorization_code_scopes() method was called?

This comment has been minimized.

Copy link
@wiliamsouza

wiliamsouza Sep 21, 2017

Author Member

Done in 34aa9b0

oauthlib/oauth2/rfc6749/endpoints/pre_configured.py Outdated
'code id_token': openid_connect_auth,
'code token id_token': openid_connect_auth,
'token': implicit_grant_choice,
'id_token': implicit_grant_choice,

This comment has been minimized.

Copy link
@skion

skion Sep 19, 2017

Member

Should id_token and id_token token not point to the openid_connect_implicit grant directly?

oauthlib/oauth2/rfc6749/grant_types/openid_connect.py Outdated
handler = self.default_token_grant
scopes = ()
parameters = dict(request.decoded_body)
client_id = parameters.get('client_id', '')

This comment has been minimized.

Copy link
@skion

skion Sep 19, 2017

Member

Why do client_id and redirect_uri default to an empty string instead of None?

This comment has been minimized.

Copy link
@wiliamsouza

wiliamsouza Sep 19, 2017

Author Member

Nothing special, changed to None, thanks.

oauthlib/oauth2/rfc6749/grant_types/openid_connect.py Outdated
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.

This comment has been minimized.

Copy link
@skion

skion Sep 19, 2017

Member

Typo: rase

@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Sep 19, 2017

@thedrow @skion suggestions done. Thanks for review.

@thedrow

This comment has been minimized.

Copy link
Collaborator

commented Sep 21, 2017

Is this ready for merge?

@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2017

@skion can answer the question!

oauthlib/oauth2/rfc6749/grant_types/openid_connect.py Outdated
# 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)

This comment has been minimized.

Copy link
@skion

skion Sep 21, 2017

Member

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.

This comment has been minimized.

Copy link
@wiliamsouza

wiliamsouza Sep 21, 2017

Author Member

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.

This comment has been minimized.

Copy link
@skion

skion Sep 25, 2017

Member

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.

This comment has been minimized.

Copy link
@wiliamsouza

wiliamsouza Sep 28, 2017

Author Member

@skion done

@wiliamsouza wiliamsouza force-pushed the wiliamsouza:openid-connect branch to 76c08b7 Sep 24, 2017

@skion

This comment has been minimized.

Copy link
Member

commented Sep 28, 2017

LGTM!

@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2017

@thedrow ready for merge!

@thedrow thedrow merged commit e575cca into oauthlib:master Oct 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@thedrow

This comment has been minimized.

Copy link
Collaborator

commented Oct 1, 2017

Thanks. Is this a breaking change? Just so I'll know how to tag the release.

@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2017

@thedrow Yes not backward compatible.

@skion

This comment has been minimized.

Copy link
Member

commented Oct 18, 2017

Turns out this leads to problems with existing code that doesn't have request_validator.get_authorization_code_scopes defined, as this PR expects it to exist and return something sensible.

To fix we could catch the NotImplementedError in the dispatcher, or alternatively just return an empty list from the base request validator. But neither is particularly aesthetically pleasing.

@thedrow

This comment has been minimized.

Copy link
Collaborator

commented Oct 19, 2017

Oh this should have been released as 3.x :(

@lepture

This comment has been minimized.

Copy link
Collaborator

commented Oct 19, 2017

@thedrow we need to add a breaking changes tag.

@duaneking

This comment has been minimized.

Copy link
Member

commented Oct 19, 2017

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.

@skion

This comment has been minimized.

Copy link
Member

commented Oct 19, 2017

@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.

@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2017

Is there no way to remove 2.0.5 from the history?

@lepture

This comment has been minimized.

Copy link
Collaborator

commented Oct 20, 2017

@wiliamsouza no. But there is a 2.0.6 in pypi now. I cherry picked every commits except this one.

@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2017

@lepture Great job!

@thedrow

This comment has been minimized.

Copy link
Collaborator

commented Oct 23, 2017

Can we reopen this PR or release 3.0 with the relevant commit?

@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2017

@thedrow After #488 we can release 3.0

This was referenced Feb 2, 2018

@skion

This comment has been minimized.

Copy link
Member

commented Feb 8, 2018

@wiliamsouza Would this be backward compatible if we just add this line 78 back in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.