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
OpenID Connect split #525
OpenID Connect split #525
Conversation
fd64d07
to
255f0c0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea and left an initial comment.
Does this PR contain any functional changes, or merely moving existing code around?
from oauthlib.oauth2.rfc6749.tokens import TokenBase, random_token_generator | ||
|
||
|
||
class JWTToken(TokenBase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't regard JWT tokens to be part of OIDC, but rather a special type of Bearer token. Notably there is RFC7523, which specifies how to use JWTs for token exchange and client authentication within plain OAuth2, but I imagine there could be other use cases even.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As such an example use case that I know of: in the educational tech standards world, the IMSGlobal standards organization is proposing the use of JWTs as a way to carry signed messages from system to system within the Learning Tools Interoperability (LTI) standard, apart from using them as assertions of identity within an OAuth2/OIDC style workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Let's move this out of the openid package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, my suggestion is to start using a tree like:
│ ├── tokens
│ │ ├── bearer
│ │ │ ├── __init__.py
│ │ │ └── rfc6750
│ │ │ └── __init__.py
│ │ ├── __init__.py
│ │ ├── jwe
│ │ │ ├── __init__.py
│ │ │ └── rfc7516
│ │ │ └── __init__.py
│ │ ├── jws
│ │ │ ├── __init__.py
│ │ │ └── rfc7515
│ │ │ └── __init__.py
│ │ └── jwt
│ │ ├── __init__.py
│ │ ├── rfc7519
│ │ │ └── __init__.py
│ │ └── rfc7523
│ │ └── __init__.py
And this can be keep as sibling of oauth2
and openid
folders.
ls oauthlib/
common.py __init__.py oauth1 oauth2 openid signals.py tokens uri_validate.py webfinger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you reckon we need jwe
and jws
in our codebase, or can we just depend on a JWT library for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use a external lib but common interface would be nice cause we need change that lib for any reason we don't break things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this can be handled in a next PR would like let this PR as is and discuss how and where to move JWT in a proceeding one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could split our tokens implementation into a different package later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JWE seems not used in OpenID Connect. JWT requires JWS, JWA and maybe JWK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use #537 to talk about it. This PR will not include no changes like this.
@skion Only moving things no new features added. |
Will be great to increase the test coverage for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be OK to merge since it contains no functional changes. Any objections?
Opened an issue for the JWT pull: #537 |
@wiliamsouza Could you rebase this and merge this one? |
… a backward compatible
967497a
to
5f857f8
Compare
This PR reduces the not backward compatible changes related to OpenID Connect Core improvements wich will only affect those who use OpenID connect that is very small considered with OAuth2 users.
All related OpenID Connect Core code now live in it's own tree like: