OpenID Connect support #915
Conversation
* Add OpenID connect hybrid grant type * Add OpenID connect algorithm type to Application model * Add OpenID connect id token model * Add nonce Authorization as required by OpenID connect Implicit Flow * Add body to create_authorization_response to pass nonce and future OpenID parameters to oauthlib.common.Request * Add OpenID connect ID token creation and validation methods and scopes * Add OpenID connect response types * Add OpenID connect authorization code flow test * Add OpenID connect implicit flow tests * Add validate_user_match method to OAuth2Validator * Add RSA_PRIVATE_KEY setting with blank value * Update tox * Add get_jwt_bearer_token to OAuth2Validator * Add validate_jwt_bearer_token to OAuth2Validator * Change OAuth2Validator.validate_id_token default return value to False to avoid validation security breach * Change to use .encode to avoid py2.7 tox test error * Add OpenID connect hybrid flow tests * Change to use .encode to avoid py2.7 tox test error * Add RSA_PRIVATE_KEY to the list of settings that cannot be empt * Add support for oidc connect discovery * Use double quotes for strings * Rename migrations to avoid name and order conflict * Remove commando to install OAuthLib from master and removed jwcrypto duplication * Remove python 2 compatible code * Change errors access_denied/unauthorized_client/consent_required/login_required to be 400 as changed in oauthlib/pull/623 * Change iss claim value to come from settings * Change to use openid connect code server class * Change test to include missing state * Add id_token relation to AbstractAccessToken * Add claims property to AbstractIDToken * Change OAuth2Validator._create_access_token to save id_token to access_token * Add userinfo endpoint * Update migrations and remove oauthlib duplication * Remove old generated migrations * Add new migrations * Fix tests * Add nonce to hybrid tests * Add missing new attributes to test migration * Rebase fixing conflicts and tests * Remove auto generate message * Fix flake8 issues * Fix test doc deps * Add project settings to be ignored in coverage * Tweak migrations to support non-overidden models * OIDC_USERINFO_ENDPOINT is not mandatory * refresh_token grant should be support for OpenID hybrid * Fix the user info view, and remove hard dependency on DRF * Use proper URL generation for OIDC endpoints * Support rich ID tokens and userinfo claims Extend the validator and override get_additional_claims based on your own user model. * Bug fix for at_hash generation See https://openid.net/specs/openid-connect-core-1_0.html#id_token-tokenExample to prove algorithm * OIDC_ISS_ENDPOINT is an optional setting * Support OIDC urls from issuer url if provided * Test for generated OIDC urls * Flake * Rebase on master and migrate url function to re_path * Handle invalid token format exceptions as invalid tokens * Merge migrations and sort imports isort for flake8 lint check Co-authored-by: Dave Burkholder <dave@thinkwelldesigns.com> Co-authored-by: Wiliam Souza <wiliamsouza83@gmail.com> Co-authored-by: Allisson Azevedo <allisson@gmail.com> Co-authored-by: fvlima <frederico.vieira@gmail.com> Co-authored-by: Shaun Stanworth <shaun.stanworth@googlemail.com>
Codecov Report
@@ Coverage Diff @@
## master #915 +/- ##
==========================================
+ Coverage 95.07% 96.59% +1.52%
==========================================
Files 30 31 +1
Lines 1420 1704 +284
==========================================
+ Hits 1350 1646 +296
+ Misses 70 58 -12
Continue to review full report at Codecov.
|
Make OIDC support optional by not requiring OIDC_RSA_PRIVATE_KEY to be set in the settings, and using the standard oauthlib.oauth2.Server class when an OIDC private key is not configured. Add a test fixture wrapping oauth2_settings. This allows individual tests / test suites to override oauth2 settings and have them reset at the end of the test. This avoids configuration leaking from one test to another, and allows us to test multiple different configurations in one test run. When using the oauth2_settings fixture, allow configuration for the test case to be loaded from a pytest marker called oauth2_settings. Split out OIDC specific tests requiring specific OIDC configuration into separate TestCase. Adjust the OAuthLibMixin to fallback to using the server, validator and core classes specified in oauth2_settings when not hardcoded in to the class. These classes can still be specified as hard-coded attributes in sub-classes, but it's no longer required if you just want what is configured in oauth2_settings, so remove all attributes that are just pointing at the configuration anyway. Add a setting ALWAYS_RELOAD_OAUTHLIB_CORE, which causes OAuthLibMixin to reload the OAuthLibCore object on each request. This is only intended to be used during testing, to allow the views to recognise changes in configuration. Show missing coverage lines in the coverage report. Fixes: django-oauth#873
|
There's still more to come :) I'm working this week on adding more tests + documentation, and then on the TODO items on the checklist |
Add a mixin for OIDC only views that checks that OIDC is correctly configured before allowing the request to continue. If OIDC is not enabled, raise an ImproperlyConfigured exception if the site is in DEBUG mode, otherwise log a warning and return a 404 response.
* Add nonce and claims fields to Grant model. * Complete support for nonce in the OIDC auth code flow, and work around an oauthlib bug in the OIDC hybrid flow that caused a nonce presented in the authentication endpoint to be omitted from the ID token. * Switch to using `RequestValidator.finalize_id_token` instead of `RequestValidator.get_id_token`. This allows us to use the oauthlib stock implementation of `get_id_token`, which creates `at_hash` and `c_hash` when appropriate. * Support `claims` in the authentication endpoint to specify what claims are desired in the ID token, as per section 5.5. Interpreting the claims parameter is up to implementers. * Add documentation on using scopes and claims as authentication endpoint parameters to influence what claims to add to the tokens. * Add tests for `nonce` and `claims` handling.
Not entirely sure how my previous changes caused that, but convert them to sets and compare the sets of scopes.
`code` is sufficient for retrieving an auth code's scopes, and we don't need to do two DB queries where one will suffice.
JWKs are commonly served from `.well-known/jwks.json`. This isn't a standard, but it is in common usage so it makes sense to conform.
Split out how to generate the OIDC issuer (when not specified in settings) out to a util function. Add tests Dont use reverse_lazy when the next thing we do with the url is to format it in to a string - no time for it to be lazy!
* Add full support for HS256 signed OIDC keys. * Add a new default signing algorithm, NO_ALGORITHM. * Add docs on why you shouldn't choose HS256, and how to do it anyway. * Add docs on how to enable an app for OIDC. * Remove OIDC_ID_TOKEN_SIGNING_ALG_VALUES_SUPPORTED and generate it depending on whether you have added an RSA key. * Add validation for your application signing algorithm so its not possible to accidentally setup something insecure. * Add `Application.jwk_key` property to load the correct key for an application. * Peek at a supplied id_token to determine the application it belongs to so that we can load the correct key to verify the signature. * Add an OIDC_ENABLED setting, as we can now enable OIDC without adding OIDC_RSA_PRIVATE_KEY. * Update and add new tests.
Add a jti parameter to each ID token generated. After verifying a received ID token JWT, extract the jti claim to verify that the ID token exists in our database and is not expired. We don't require the contents of the JWT to verify that an ID token hasn't expired or been revoked, just the jti claim, so don't bother to store or index the ID token contents. Because we only would look at this when presented with an ID token JWT, all the claim contents are available in this JWT. Add missing scope check when verifying an ID token, add tests to verify this. Add functions _load_id_token and _load_access_token to OAuth2Validator, analagous to _save_id_token and _create_access_token. These can be overridden in sub-classes to customise loading behaviour, if these models have been swapped.
|
This feature would be much appreciated. We are trying to get OpenID Connect provider support in AlekSIS and are currently struggling to integrate django-oidc-provider with django-oauth-toolkit. Having django-oauth-toolkit be able to do the whole job in time for our release would simplify things a lot! |
|
We got it working in staging, with one issue: In We worked around that by setting Django's |
* Don't swallow ValueError in ClientProtectedResourceMixin * Use OAuthLibCore._get_escaped_full_path to encode URI before passing to oauthlib. * You cannot create an ID Token using client credentials flow, so remove dead code handling this eventuality from OAuth2Validator._save_id_token * Remove TODO comment about saving IDTokens, it isn't necessary.
Ah, that's a bit of a pain. You can also fix the issuer by setting it explicitly in settings, but it would be preferable to avoid adding a setting just to get SSL working correctly. We have some control over how |
OAuthlib will use OAuth2Validator.get_oidc_issuer_endpoint to generate the issuer endpoint; we create a phony django request to use django's mechanisms for generating the fully qualified URL. However, when SSL is enableld, not enough information is passed through to determine that the protocol is HTTPS. Adjust OAuthLibCore.extract_headers() to inject a custom header when the django request is secure. Use this extra header when generating the issuer URL to determine whether to set the protocol to "https", and use a custom django.http.HttpRequest subclass to allow us to set this. Adjust OAuthLibCore.create_authorization_response() to pass through headers to oauthlib to allow this header to be received.
Add kid to id_token header, conditional on the algorithm used
|
@tevansuk Are you still making changes to this PR? I had begun reviewing it but will wait if you aren't ready. |
@n2ygk I'm not planning any more changes at this point. The last two changes were from bug reports I've received:
I'll triage new bug fixes into a new PR to avoid changing this PR any more 👍 |
@tevansuk Sounds like a plan. I assume once we release this as a 1.5.0 release -- hopefully in the next day or two -- there may be a flurry of bug reports, etc. which we can quickly push out as small patch releases. This many commits in a single release is difficult to review. Thanks. |
|
Great work, this is a really good addition! Making a release very soon would be of much help to the AlekSIS project. |
* rebased * use bleeding edge from django-oauth/django-oauth-toolkit#915 * update readme, example of extra data provided to keycloack with mulysaoauthvalidator * Update dependencies * use the released version, make flake8 happy * blacken settings.py while we are here * and black urls.py * and more black * pipenv lock * add mxid_full and mxid_local_part to returned data * Revert "add mxid_full and mxid_local_part to returned data" This reverts commit 4a656ff.
|
@tevansuk hey I'm a new DOT maintainer. I noticed pytest got added to the project in this PR. I'm trying to get some historic context regarding the mix of test fixture libraries in the project. Do you remember why you chose to add pytest over just working with django.test and decorators like |
Fixes #110
Description of the Change
DRAFT
Adds support for OIDC authorization code flow and hybrid flow. This is mostly not my work; I've just rebased #859 onto the latest master and added support for swappable
ModelAdminsfor the new models. It's quite a large change, so I've put it up for review prior to working on finishing off the actual PR (see checklist).Thanks to Dave Burkholder, William Souza, Allisson Azevedo, fvlima and Shaun Stanworth for their initial work on rounds 1 and 2 of OIDC support 👍
Checklist
Significant changes that still need to be made:
CHANGELOG.mdupdated (only for user relevant changes)AUTHORSat_hashandc_hashare requiredid_token_hint(RequestValidator.validate_user_hint)RequestValidator.get_authorization_code_nonce)