OpenID connect improvements #484

wiliamsouza · 2017-09-06T17:59:29Z

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 · 2017-09-12T22:37:32Z

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 · 2017-09-17T12:31:55Z

@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

wiliamsouza · 2017-09-17T20:22:33Z

@thedrow Done!

thedrow · 2017-09-18T08:00:57Z

oauthlib/oauth2/rfc6749/grant_types/implicit.py

+
+        # 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():


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

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

I like it and it doesn't break my implementation. I've added some questions inline.

skion · 2017-09-19T20:30:12Z

tests/oauth2/rfc6749/grant_types/test_openid_connect.py

+
+    def test_create_token_response_openid_without_code(self):
+        handler = self.dispatcher._handler_for_request(self.request)
+        self.assertTrue(isinstance(handler, AuthorizationCodeGrant))


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

Done in 34aa9b0

skion · 2017-09-19T20:33:19Z

oauthlib/oauth2/rfc6749/endpoints/pre_configured.py

-                                           'code id_token': openid_connect_auth,
-                                           'code token id_token': openid_connect_auth,
+                                           'token': implicit_grant_choice,
+                                           'id_token': implicit_grant_choice,


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

skion · 2017-09-19T20:39:51Z

oauthlib/oauth2/rfc6749/grant_types/openid_connect.py

+        handler = self.default_token_grant
+        scopes = ()
+        parameters = dict(request.decoded_body)
+        client_id = parameters.get('client_id', '')


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

Nothing special, changed to None, thanks.

skion · 2017-09-19T20:43:24Z

oauthlib/oauth2/rfc6749/grant_types/openid_connect.py

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


Typo: rase

wiliamsouza · 2017-09-19T22:59:50Z

@thedrow @skion suggestions done. Thanks for review.

thedrow · 2017-09-21T07:40:01Z

Is this ready for merge?

wiliamsouza · 2017-09-21T13:31:15Z

@skion can answer the question!

skion · 2017-09-21T19:50:37Z

oauthlib/oauth2/rfc6749/grant_types/openid_connect.py

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


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

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.

@skion done

…o use openid_connect_implicit direct

skion · 2017-09-28T19:48:49Z

LGTM!

wiliamsouza · 2017-09-28T20:01:45Z

@thedrow ready for merge!

thedrow · 2017-10-01T06:07:51Z

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

wiliamsouza · 2017-10-01T22:11:58Z

@thedrow Yes not backward compatible.

skion · 2017-10-18T20:59:54Z

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 · 2017-10-19T10:07:51Z

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

lepture · 2017-10-19T15:09:03Z

@thedrow we need to add a breaking changes tag.

duaneking · 2017-10-19T16:56:29Z

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 · 2017-10-19T21:02:54Z

@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 · 2017-10-20T15:16:54Z

Is there no way to remove 2.0.5 from the history?

lepture · 2017-10-20T17:19:34Z

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

wiliamsouza · 2017-10-20T18:30:44Z

@lepture Great job!

thedrow · 2017-10-23T11:52:09Z

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

wiliamsouza · 2017-10-23T12:26:15Z

@thedrow After #488 we can release 3.0

skion · 2018-02-08T20:50:41Z

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

This was referenced Sep 6, 2017

OpenID Connect jazzband/django-oauth-toolkit#110

Closed

Needs more maintainers #485

Closed

wiliamsouza added 8 commits September 17, 2017 15:57

Change create_token_response to only save access_token when it's pres…

d42cf77

…ent in request.response_type

Remove unused import, fix indentation and improve comment

685d85c

Fix AuthorizationEndpoint response_type for OpenID Connect hybrid flow

cacade5

Add new ImplicitTokenGrantDispatcher

a112e15

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

a4579b5

Change AuthorizationEndpoint grant_types `'authorization_code'` to work with OpenID Connect and OAuth2 authorization flow in a transparent way

Change tests to include required client_id and redirect_uri

6193972

Remove AuthorizationEndpoint grant_types 'openid'

14aee43

Now OpenID Connect and OAuth2 authorization flow can use `authorization_code` in a transparent way

Add sone blank lines and fix indentation

9fb35bb

wiliamsouza force-pushed the openid-connect branch from a8fe881 to 9fb35bb Compare September 17, 2017 19:20

thedrow requested changes Sep 18, 2017

View reviewed changes

skion reviewed Sep 19, 2017

View reviewed changes

skion reviewed Sep 21, 2017

View reviewed changes

wiliamsouza added 3 commits September 24, 2017 19:02

Change AuthorizationEndpoint grant type id_token and id_token token t…

421dee3

…o use openid_connect_implicit direct

Change default empty value to None and fix a typo

3a27359

Add assert called to AuthTokenGrantDispatcher tests

76c08b7

wiliamsouza force-pushed the openid-connect branch from 34aa9b0 to 76c08b7 Compare September 24, 2017 22:04

Add request to get_authorization_code_scopes

5c4b2a3

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

lepture mentioned this pull request Oct 19, 2017

2.0.5 caused "NotImplementedError: Subclasses must implement this method." error #490

Closed

This was referenced Feb 2, 2018

OAuthLib upcoming release 3.0.0 #512

Closed

Release 3.0.0 #513

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenID connect improvements #484

OpenID connect improvements #484

wiliamsouza commented Sep 6, 2017

skion commented Sep 12, 2017

thedrow commented Sep 17, 2017

wiliamsouza commented Sep 17, 2017

thedrow Sep 18, 2017

wiliamsouza Sep 24, 2017 •

edited

skion left a comment

skion Sep 19, 2017

wiliamsouza Sep 21, 2017

skion Sep 19, 2017

skion Sep 19, 2017

wiliamsouza Sep 19, 2017

skion Sep 19, 2017

wiliamsouza commented Sep 19, 2017

thedrow commented Sep 21, 2017

wiliamsouza commented Sep 21, 2017

skion Sep 21, 2017

wiliamsouza Sep 21, 2017

skion Sep 25, 2017

wiliamsouza Sep 28, 2017

skion commented Sep 28, 2017

wiliamsouza commented Sep 28, 2017

thedrow commented Oct 1, 2017

wiliamsouza commented Oct 1, 2017

skion commented Oct 18, 2017

thedrow commented Oct 19, 2017

lepture commented Oct 19, 2017

duaneking commented Oct 19, 2017

skion commented Oct 19, 2017

wiliamsouza commented Oct 20, 2017 •

edited

lepture commented Oct 20, 2017

wiliamsouza commented Oct 20, 2017

thedrow commented Oct 23, 2017

wiliamsouza commented Oct 23, 2017

skion commented Feb 8, 2018

OpenID connect improvements #484

OpenID connect improvements #484

Conversation

wiliamsouza commented Sep 6, 2017

skion commented Sep 12, 2017

thedrow commented Sep 17, 2017

wiliamsouza commented Sep 17, 2017

Choose a reason for hiding this comment

wiliamsouza Sep 24, 2017 • edited

Choose a reason for hiding this comment

skion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wiliamsouza commented Sep 19, 2017

thedrow commented Sep 21, 2017

wiliamsouza commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skion commented Sep 28, 2017

wiliamsouza commented Sep 28, 2017

thedrow commented Oct 1, 2017

wiliamsouza commented Oct 1, 2017

skion commented Oct 18, 2017

thedrow commented Oct 19, 2017

lepture commented Oct 19, 2017

duaneking commented Oct 19, 2017

skion commented Oct 19, 2017

wiliamsouza commented Oct 20, 2017 • edited

lepture commented Oct 20, 2017

wiliamsouza commented Oct 20, 2017

thedrow commented Oct 23, 2017

wiliamsouza commented Oct 23, 2017

skion commented Feb 8, 2018

wiliamsouza Sep 24, 2017 •

edited

wiliamsouza commented Oct 20, 2017 •

edited