OpenID connect improvements by wiliamsouza · Pull Request #484 · oauthlib/oauthlib

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

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

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

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

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

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

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

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 django-oauth/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

Uh oh!

Conversation

wiliamsouza commented Sep 6, 2017

Uh oh!

skion commented Sep 12, 2017

Uh oh!

thedrow commented Sep 17, 2017

Uh oh!

wiliamsouza commented Sep 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wiliamsouza Sep 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wiliamsouza commented Sep 19, 2017

Uh oh!

thedrow commented Sep 21, 2017

Uh oh!

wiliamsouza commented Sep 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skion commented Sep 28, 2017

Uh oh!

wiliamsouza commented Sep 28, 2017

Uh oh!

thedrow commented Oct 1, 2017

Uh oh!

wiliamsouza commented Oct 1, 2017

Uh oh!

skion commented Oct 18, 2017

Uh oh!

thedrow commented Oct 19, 2017

Uh oh!

lepture commented Oct 19, 2017

Uh oh!

duaneking commented Oct 19, 2017

Uh oh!

skion commented Oct 19, 2017

Uh oh!

wiliamsouza commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lepture commented Oct 20, 2017

Uh oh!

wiliamsouza commented Oct 20, 2017

Uh oh!

thedrow commented Oct 23, 2017

Uh oh!

wiliamsouza commented Oct 23, 2017

Uh oh!

skion commented Feb 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

wiliamsouza Sep 24, 2017 •

edited

Loading

wiliamsouza commented Oct 20, 2017 •

edited

Loading