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.

Sign up for GitHub

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

Jump to bottom

Pass scope, token and request to OIDC_IDTOKEN_PROCESSING_HOOK #194

Merged
merged 5 commits into from
May 24, 2018
Merged

Pass scope, token and request to OIDC_IDTOKEN_PROCESSING_HOOK #194

merged 5 commits into from
May 24, 2018

Conversation

suutari-ai
Copy link
Contributor

@suutari-ai suutari-ai commented Jul 7, 2017 •
edited
Loading

The ID token processing hook might want to add claims to the ID token conditionally based on the scope, token and/or request parameters. Therefore it would be very useful to provide the scope parameter to the processing hook.

This is a backward incompatible change, but IMHO it's worth it. Updating the client code requires only adding the scope, token and request parameters or **kwargs to the hook function . Without this change it requires a nasty hack to get the scope, e.g. something like this:

https://github.com/suutari-ai/tunnistamo/blob/bb5bfde43a77373ccc636f458ca696817733f1ae/oidc_apis/id_token.py

@suutari-ai
Fix scope handling of token endpoint
6199a9a 
The token endpoint handled the scope parameter incorrectly for all of
the three handled grant types:

 1. For "authorization_code" grant type the scope parameter in the token
    request should not be respected but the scope should be taken from
    the authorization code.  It was not totally ignored, but rather the
    scope parameter of the token request was used for the generated ID
    token.  This had two consequences:

      * Spec conforming implementations of authorization code flow
        didn't get correct ID tokens, since they usually don't pass
        scope parameter with the token request.

      * It's possible to get a broader scope for the ID token than what
        is authorized by the user in the original authorization code
        request.

 2. For "refresh_token" grant type the scope parameter in the token
    request should only allow narrowing down the scope.  It wasn't
    narrowed, but rather the original auth code scope was used for the
    access token and the passed in scope parameter was used for the ID
    token (again allowing unauthorized scopes in the ID token).

 3. For "password" grant type the scope parameter in the token request
    should be respected.  The problem with this was that it wasn't
    properly splitted when passed to ID token creation.

Fixes juanifioren#186
@suutari-ai
Pass scope to OIDC_IDTOKEN_PROCESSING_HOOK
eb682f2 
The ID token processing hook might want to add claims to the ID token
conditionally based on the scope parameter.  Therefore it would be very
useful to provide the scope parameter to the processing hook.
@suutari-ai suutari-ai changed the base branch from v0.4.x to develop July 7, 2017 20:05
@wojtek-fliposports
Copy link
Contributor

I was thinking about this too, to be honest.
Maybe we change documentation in way that this hook function should have signature similar to this:

def token_processing_hook(id_token, user, token, request, **kwargs):

kwargs for future compatibility.

For right now I'm thinking about passing request object and whole models.Token (if is created for every type of flows - can't remember right now)

@Flor1an-dev
Copy link

Having the token as well as the request would be beneficial for me. Right now I'm getting both values through stack inspection :/

@suutari-ai suutari-ai mentioned this pull request May 23, 2018
Closed
@suutari-ai
Merge branch 'develop' of github.com:juanifioren/django-oidc-provider
9342046 
* 'develop' of github.com:juanifioren/django-oidc-provider:
  Update changelog.rst
  include request in password grant authenticate call
  Update setup.py
  Update changelog.rst
  Update changelog.rst
  Adjust import order and method order in introspection tests
  Replace resource with client in docs.
  Update settings docs to add extra introspection setting
  Update README.md
  Update README.md
  Remove the Resource model
  Skip csrf protection on introspection endpoint
  Add token introspection endpoint to satisfy https://tools.ietf.org/html/rfc7662
  Test docs with tox.
  Remove Django 1.7 for travis.
  Drop support for Django 1.7.
  Move extract_client_auth to oauth2 utils.
  Remove duplicate link in docs.
  Bump version v0.6.0.
  Fix BaseCodeTokenModel and user attr.
  Update README.md
  Edit README and contribute doc.
  Edit changelog.
  Update changelog.rst
  Add protected_resource_view test using client_credentials.
  Fix docs.
  Improve docs.
  Client credentials implementation.
  Move changelog into docs.
  Update README.md
  Update CHANGELOG.md
  Fixed infinite callback loop in check-session iframe
  Fix PEP8. New migration.
  Update example project.
  Fix PEP8.
  Fix PEP8.
  PEP8 errors and urls.
  PEP8 models.
  Fix contribute docs.
  Fix tox for checking PEP8 all files.
  Update README.md
  Update README.md
  Simplify test suit.
  Update CHANGELOG.md
  Bump version 0.5.3.
  Update installation.rst
  Update CHANGELOG.md
  Fixed wrong Object in Template
  Update project to support Django 2.0
  Now passing along the token to create_id_token function.
  Made token and token_refresh endpoint return requested claims.
  Sphinx documentation fixes (juanifioren#219)
  Use request.user.is_authenticated as a bool with recent Django (juanifioren#216)
  Fixed client id retrieval when aud is a list of str. (juanifioren#210)
  Add owner field to Client (juanifioren#211)
  Update CHANGELOG
  removed tab char
  Add pep8 compliance and checker
  Bump version
  Update CHANGELOG.md
  Preparing v0.5.2 (juanifioren#201)
  Fix Django 2.0 deprecation warnings (juanifioren#185)
  Fix infinite login loop if "prompt=login" (juanifioren#198)
  fixed typos
  Bump version
  Fix scope handling of token endpoint (juanifioren#193)
  Fixes juanifioren#192
  Use stored user consent for public clients too (juanifioren#189)
  Redirect URIs must match exactly. (juanifioren#191)
  Bug juanifioren#187 prompt handling (juanifioren#188)
  Don't pin exact versions in install_requires.
@suutari-ai suutari-ai force-pushed the scope-for-idtoken-processor branch from 6583ee8 to d804d8a Compare May 23, 2018 22:10
@suutari-ai suutari-ai changed the title Pass scope to OIDC_IDTOKEN_PROCESSING_HOOK Pass scope, token and request to OIDC_IDTOKEN_PROCESSING_HOOK May 23, 2018
@suutari-ai suutari-ai force-pushed the scope-for-idtoken-processor branch 2 times, most recently from 14fd133 to e5b7af2 Compare May 23, 2018 22:17
@suutari-ai
Copy link
Contributor Author

@wojtek-fliposports I updated this, as you suggested. Now also the token and the request are passed to the hook function. Also updated the documentation.

This should be ready for review now. @juanifioren What do you think?

@suutari-ai
Pass token and request to OIDC_ID_TOKEN_PROCESSING_HOOK
b0a82aa 
The ID token processing hook might need the token or request too, so
make them available.
@suutari-ai
Document the new ID token processing hook parameters
7eb3157
@suutari-ai suutari-ai force-pushed the scope-for-idtoken-processor branch from e5b7af2 to 7eb3157 Compare May 24, 2018 06:31
@juanifioren juanifioren merged commit c1c84d4 into juanifioren:develop May 24, 2018
@juanifioren
Copy link
Owner

Great contribution, thanks @suutari-ai. Btw, why the need of passing scope? having token.scope. Greetings!

@suutari-ai
Copy link
Contributor Author

Btw, why the need of passing scope? having token.scope.

Hmm.. yes. I thought that it's possible for the requested scope to be different than the token scope for some cases, but now that I recheck the code, it seems that I was wrong there. I checked all create_id_token calls and they all send in a scope value that is equal to token.scope of the token sent in the same call.

Should I create a PR to remove the scope param from the arguments?

@suutari-ai suutari-ai deleted the scope-for-idtoken-processor branch May 31, 2018 06:30
@suutari-ai
Copy link
Contributor Author

@juanifioren I created a PR to remove the scope parameter: #245

@wojtek-fliposports
Copy link
Contributor

wojtek-fliposports commented May 31, 2018 via email

@suutari-ai
Copy link
Contributor Author

suutari-ai commented May 31, 2018 •
edited
Loading

Removing this will prdoce reverse incompatibility

Incompatibility compared to what? Any version of this lib with the scope param was never released AFAIK, so it should be safe to remove it.

@wojtek-fliposports
Copy link
Contributor

wojtek-fliposports commented May 31, 2018 via email

@juanifioren
Copy link
Owner

and what about using **kwargs instead of positional arguments? @suutari-ai @wojtek-fliposports

@suutari-ai
Copy link
Contributor Author

@juanifioren The code is already using kwargs when calling the processing hook, so the order of those arguments in the processing hook definition shouldn't matter: https://github.com/juanifioren/django-oidc-provider/blob/develop/oidc_provider/lib/utils/token.py#L65-L67

@wojtek-fliposports
Copy link
Contributor

wojtek-fliposports commented Jun 1, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@suutari-ai @wojtek-fliposports @Flor1an-dev @juanifioren