Conversation
fd64d07 to
255f0c0
Compare
skion
left a comment
There was a problem hiding this comment.
I like the idea and left an initial comment.
Does this PR contain any functional changes, or merely moving existing code around?
There was a problem hiding this comment.
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.
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.
I agree. Let's move this out of the openid package.
There was a problem hiding this comment.
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.
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.
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.
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.
We could split our tokens implementation into a different package later.
There was a problem hiding this comment.
JWE seems not used in OpenID Connect. JWT requires JWS, JWA and maybe JWK.
There was a problem hiding this comment.
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 |
skion
left a comment
There was a problem hiding this comment.
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: