Skip to content

OpenID Connect split #525

Merged
thedrow merged 14 commits intooauthlib:masterfrom
wiliamsouza:openid-connect-split
Jun 5, 2018
Merged

OpenID Connect split #525
thedrow merged 14 commits intooauthlib:masterfrom
wiliamsouza:openid-connect-split

Conversation

@wiliamsouza
Copy link
Copy Markdown
Member

@wiliamsouza wiliamsouza commented Mar 17, 2018

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:

oauthlib/openid/
├── connect
│   ├── core
│   │   ├── endpoints
│   │   │   └── pre_configured.py
│   │   ├── exceptions.py
│   │   ├── grant_types
│   │   │   ├── authorization_code.py
│   │   │   ├── base.py
│   │   │   ├── dispatchers.py
│   │   │   ├── exceptions.py
│   │   │   ├── hybrid.py
│   │   │   ├── implicit.py
│   │   │   └── __init__.py
│   │   ├── __init__.py
│   │   ├── request_validator.py
│   │   └── tokens.py
│   │   └── __init__.py
│   └── __init__.py
└── __init__.py

@wiliamsouza wiliamsouza requested review from skion and thedrow March 24, 2018 01:40
@wiliamsouza wiliamsouza force-pushed the openid-connect-split branch from fd64d07 to 255f0c0 Compare March 24, 2018 01:49
Copy link
Copy Markdown
Member

@skion skion left a 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?

Comment thread oauthlib/openid/connect/core/tokens.py Outdated
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor

@ViktorHaag ViktorHaag Mar 27, 2018

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.

Copy link
Copy Markdown
Collaborator

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.

Copy link
Copy Markdown
Member Author

@wiliamsouza wiliamsouza Mar 27, 2018

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

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Collaborator

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.

Copy link
Copy Markdown
Collaborator

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.

Copy link
Copy Markdown
Member Author

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.

@wiliamsouza
Copy link
Copy Markdown
Member Author

@skion Only moving things no new features added.

@JonathanHuot
Copy link
Copy Markdown
Member

Will be great to increase the test coverage for dispatchers.py and exceptions.py which have a very low pourcentage at the moment.

Copy link
Copy Markdown
Member

@skion skion left a 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?

@skion
Copy link
Copy Markdown
Member

skion commented Apr 13, 2018

Opened an issue for the JWT pull: #537

@skion skion mentioned this pull request May 8, 2018
@skion skion mentioned this pull request May 21, 2018
@skion
Copy link
Copy Markdown
Member

skion commented May 26, 2018

@wiliamsouza Could you rebase this and merge this one?

@skion skion added this to the 3.0.0 milestone May 26, 2018
@wiliamsouza wiliamsouza force-pushed the openid-connect-split branch from 967497a to 5f857f8 Compare May 29, 2018 14:52
@thedrow thedrow merged commit d5a4d5e into oauthlib:master Jun 5, 2018
@skion skion mentioned this pull request Jul 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants