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

[Work in progress] Openid Connect Core support #545

Open
wants to merge 37 commits into
base: master
from

Conversation

Projects
None yet
@wiliamsouza
Copy link
Contributor

commented Jan 18, 2018

This PR add OpenID Connect Core http://openid.net/specs/openid-connect-core-1_0.html support.

To run the tests do:

Django OAuth Toolkit cloned repository

git remote add git@github.com:wiliamsouza/django-oauth-toolkit.git wiliamsouza
git checkout -b openid-connect wiliamsouza/openid-connect
tox

Usage examples:

Authorization-code

http://openid.net/specs/openid-connect-core-1_0.html#CodeFlowAuth

token and id_token

http://127.0.0.1:8000/o/authorize?response_type=code&client_id=HCSTniKnHNj6qP6Dkms39q6IT4pahzfXws2uwTkS&redirect_uri=http://localhost/callback&scope=openid%20read&state=af0ifjsldkj&nonce=n-0S6_WzA2Mj

Only token

http://127.0.0.1:8000/o/authorize?response_type=code&client_id=HCSTniKnHNj6qP6Dkms39q6IT4pahzfXws2uwTkS&redirect_uri=http://localhost/callback&scope=read&state=af0ifjsldkj&nonce=n-0S6_WzA2Mj
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=HCSTniKnHNj6qP6Dkms39q6IT4pahzfXws2uwTkS" \
    -d "client_secret=Ay1rH78PChsOG4mshfdp2oJnomdpu5Vgtdz6jCmDkEM8mKHzcaKo5GEYNGK42KTN8XqEWbbpn1vdHJKYcBgawONx4S1xXY7GtEP9mvsMw593DeXH0aRpWlgySuxeDfe2" \
    -d "code=89BRsh4kQqrwlNMLj4coVZbrpDm0Mh" \
    -d "redirect_uri=http://localhost/callback" \
    -d "grant_type=authorization_code"

implicit

http://openid.net/specs/openid-connect-core-1_0.html#ImplicitFlowAuth

token

http://127.0.0.1:8000/o/authorize?response_type=token&client_id=Ktn0Sh4hO2gA8PKC2aqsauY4ZCxNyIdF1wNfFfJ3&redirect_uri=http://localhost/callback&scope=openid%20read&state=af0ifjsldkj&nonce=n-0S6_WzA2Mj

id_token

http://127.0.0.1:8000/o/authorize?response_type=id_token&client_id=Ktn0Sh4hO2gA8PKC2aqsauY4ZCxNyIdF1wNfFfJ3&redirect_uri=http://localhost/callback&scope=openid%20read&state=af0ifjsldkj&nonce=n-0S6_WzA2Mj

token and id_token

http://127.0.0.1:8000/o/authorize?response_type=id_token%20token&client_id=Ktn0Sh4hO2gA8PKC2aqsauY4ZCxNyIdF1wNfFfJ3&redirect_uri=http://localhost/callback&scope=openid%20read&state=af0ifjsldkj&nonce=n-0S6_WzA2Mj

openid-hybrid

http://openid.net/specs/openid-connect-core-1_0.html#HybridFlowAuth

code, id_token and token

http://127.0.0.1:8000/o/authorize?response_type=code%20id_token%20token&client_id=fZjUvm0YABo6mX1UTjo9VVYay9zj1HpaCZaoa4Fj&redirect_uri=http://localhost/callback&scope=openid%20read&state=af0ifjsldkj&nonce=n-0S6_WzA2Mj

code and id_token

http://127.0.0.1:8000/o/authorize?response_type=code%20id_token&client_id=fZjUvm0YABo6mX1UTjo9VVYay9zj1HpaCZaoa4Fj&redirect_uri=http://localhost/callback&scope=openid%20read&state=af0ifjsldkj&nonce=n-0S6_WzA2Mj

code and token

http://127.0.0.1:8000/o/authorize?response_type=code%20token&client_id=fZjUvm0YABo6mX1UTjo9VVYay9zj1HpaCZaoa4Fj&redirect_uri=http://localhost/callback&scope=openid%20read&state=af0ifjsldkj&nonce=n-0S6_WzA2Mj

code

http://127.0.0.1:8000/o/authorize?response_type=code&client_id=fZjUvm0YABo6mX1UTjo9VVYay9zj1HpaCZaoa4Fj&redirect_uri=http://localhost/callback&scope=openid%20read&state=af0ifjsldkj&nonce=n-0S6_WzA2Mj
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=fZjUvm0YABo6mX1UTjo9VVYay9zj1HpaCZaoa4Fj" \
    -d "client_secret=eiBw6IIQ4zzWFP9gSszEHZwexjCDhJjMtRxoOYQexCJMjQ6gdyN2ME9aUzbGkVopx3NSZRPUb4SV9yKpbVwwW9NKdEpkoyGmcTni7G4KTPqtJrsI9HPubFnDDzsvAf89" \
    -d "code=VQGv7tzYGO13jrRnv6oYT7jHbDSGJZ" \
    -d "redirect_uri=http://localhost/callback" \
    -d "grant_type=authorization_code"

TODO:

@coveralls

This comment has been minimized.

Copy link

commented Jan 18, 2018

Coverage Status

Coverage decreased (-4.7%) to 88.696% when pulling b2847e2 on wiliamsouza:openid-connect into 231efff on evonove:master.

@wiliamsouza wiliamsouza referenced this pull request Jan 18, 2018

Open

OpenID Connect #110

@wiliamsouza

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2018

The CI will be broke until OAuthLib version containing this PR oauthlib/oauthlib#488

@jleclanche

This comment has been minimized.

Copy link
Collaborator

commented Jan 24, 2018

Hi @wiliamsouza

Thanks, this is a high quality PR. I'm not merging it until the upstream work is fully released, but once it is, feel free to ping here again and I'll do a full review pass.

@wiliamsouza wiliamsouza referenced this pull request Jan 29, 2018

Merged

Openid connect jwt #488

@wiliamsouza

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2018

OAuthLib upcoming release 3.0.0 oauthlib/oauthlib#512

@rhenter

This comment has been minimized.

Copy link

commented Mar 21, 2018

Great PR. It will be very good when It wore merged

@afabiani

This comment has been minimized.

Copy link

commented May 31, 2018

Hi all,
how far we are on merging this?

@JonathanHuot

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2018

@jleclanche, oauthlib's PR oauthlib/oauthlib#488 has been merged into oauthlib's master branch. Would be great to validate the changes here before releasing the oauthlib@3.0.0

@wiliamsouza

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

@afabiani As soon as oauthlib@3.0.0 get released

@jleclanche

This comment has been minimized.

Copy link
Collaborator

commented Oct 4, 2018

@JonathanHuot Winter is pretty busy here, I'm not sure I'll soon be in a situation where i can do an updated review. And either way, I wouldn't want to land this without testing it (and I'm not in a position to test it).

So if people want to get this PR moving ahead of oauthlib 3.0 release, bring it into your installations and try it out, report back with any issues.

@wiliamsouza

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

@jleclanche @JonathanHuot I can manage this

@afabiani

This comment has been minimized.

Copy link

commented Oct 5, 2018

@wiliamsouza currently I had to fork this one [1] since it required few more changes in order to fully work with OIDC 1.0 (tested mainly using Keycloak). Will look forward to the new release anyway. Thanks.

[1] - https://github.com/GeoNode/geonode-oauth-toolkit

@jejenone

This comment has been minimized.

Copy link

commented Dec 3, 2018

I would love to see OIDC support in DOT. Looking forward to seeing this merged.

@bpereto

This comment has been minimized.

Copy link

commented Jan 9, 2019

@wiliamsouza oauthlib@3.0.0 is released 🎉

@tribals

This comment has been minimized.

Copy link

commented Jan 11, 2019

@wiliamsouza

I created dummy Django app with this library integrated, seems like the patch is currently doesn't work.

Here is the app: https://dummy-idp-dot.herokuapp.com. I created an OAuth app, you can view it's credentials here: https://dummy-idp-dot.herokuapp.com/oauth/applications/1. I tested it with OpenID Connect debugger: https://oidcdebugger.com. Currently there is an unsupported_response_type error. Here is the request that Debugger was generated:

https://dummy-idp-dot.herokuapp.com/oauth/authorize?client_id=P25mgGSjKjx77Dw4zaiUhhJMWqYgtIVQiogSpATz&redirect_uri=https%3A%2F%2Foidcdebugger.com%2Fdebug&scope=openid%20read&response_type=token%20id_token&response_mode=form_post&nonce=lokuqbmajz
@wiliamsouza

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

Working on it

@wiliamsouza wiliamsouza force-pushed the wiliamsouza:openid-connect branch from 266ff15 to 0d3904a Jan 16, 2019

@coveralls

This comment has been minimized.

Copy link

commented Jan 16, 2019

Pull Request Test Coverage Report for Build 1092

  • 166 of 191 (86.91%) changed or added relevant lines in 11 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.06%) to 93.493%

Changes Missing Coverage Covered Lines Changed/Added Lines %
oauth2_provider/views/oidc.py 25 28 89.29%
oauth2_provider/oauth2_validators.py 80 85 94.12%
oauth2_provider/models.py 31 48 64.58%
Totals Coverage Status
Change from base Build 1085: -1.06%
Covered Lines: 1365
Relevant Lines: 1460

💛 - Coveralls
@wiliamsouza

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

@afabiani Great! Can you explain the changes and how do you test it with Keycloak?

@wiliamsouza

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

@jejenone You can help it get done reviewing and testing this branch.

@wiliamsouza

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

@tribals Can you update the code and redo the test?

@tribals

This comment has been minimized.

Copy link

commented Jan 17, 2019

@wiliamsouza Finally I'm testing in local environment. It seems to mostly work. I tried implicit flow with response_type parameter set to both token and id_token token. Server responds correctly. There are still some misunderstandings about how to hook into this library but they disappear as the more I work with library.

@auvipy
Copy link

left a comment

please re-base to master while updating.

@auvipy
Copy link

left a comment

it's better to pull updates from master and change models based on latest master, and then regenerate migrations based on the migrations in masters branch.

@@ -0,0 +1,17 @@
# Generated by Django 1.11.4 on 2017-09-03 16:32

This comment has been minimized.

Copy link
@auvipy

auvipy Feb 21, 2019

it's better to remove this migration file

@@ -0,0 +1,17 @@
# Generated by Django 1.11.4 on 2017-09-16 18:55

This comment has been minimized.

Copy link
@auvipy

auvipy Feb 21, 2019

remove this migration too

@@ -0,0 +1,35 @@
# Generated by Django 1.11.4 on 2017-10-01 19:13

This comment has been minimized.

Copy link
@auvipy

auvipy Feb 21, 2019

remove this too

@@ -0,0 +1,21 @@
# Generated by Django 2.1.5 on 2019-01-28 18:46

This comment has been minimized.

Copy link
@auvipy

auvipy Feb 21, 2019

remove this too.

@wiliamsouza

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

@auvipy Your merge broke the build. Why did you do that before any discussion?

wiliamsouza and others added some commits Oct 29, 2017

Change errors access_denied/unauthorized_client/consent_required/logi…
…n_required to be 400 as changed in oauthlib/pull/623

@wiliamsouza wiliamsouza force-pushed the wiliamsouza:openid-connect branch from b443376 to b545468 Apr 16, 2019

@@ -0,0 +1,33 @@
# Generated by Django 2.2 on 2019-04-16 14:36

This comment has been minimized.

Copy link
@auvipy

auvipy Apr 17, 2019

is this migration auto-generated?

This comment has been minimized.

Copy link
@wiliamsouza

wiliamsouza Apr 17, 2019

Author Contributor

Yes, it is.

This comment has been minimized.

Copy link
@tribals

tribals Apr 17, 2019

Isn't that they all are autogenarated, is it? Didn't hear about how to write Django migration by hand.

This comment has been minimized.

Copy link
@wiliamsouza

wiliamsouza Apr 18, 2019

Author Contributor

You can. You can pass -n for makemigrations to give the migration a name too.

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.