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

Add Support for Opaque OAuth2 Tokens to Resource Server #6352

Merged
merged 3 commits into from
Feb 7, 2019
Merged

Add Support for Opaque OAuth2 Tokens to Resource Server #6352

merged 3 commits into from
Feb 7, 2019

Conversation

jzheaux
Copy link
Contributor

@jzheaux jzheaux commented Jan 4, 2019

No description provided.

@jzheaux jzheaux force-pushed the opaque branch 3 times, most recently from 3830e0b to 02faaa8 Compare January 8, 2019 15:33
@rwinch rwinch requested a review from jgrandja January 9, 2019 17:44
@rwinch rwinch changed the title Opaque Add Support for Opaque OAuth2 Tokens to Resource Server Jan 9, 2019
@jzheaux jzheaux force-pushed the opaque branch 2 times, most recently from 0f1fc4d to 5faa47b Compare January 10, 2019 02:06
@bdemers bdemers mentioned this pull request Jan 11, 2019
Closed
jgrandja
jgrandja requested changes Jan 11, 2019
View reviewed changes
Copy link
Contributor

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jzheaux. Please see my comments.

...mework/security/oauth2/server/resource/authentication/OpaqueTokenAuthenticationProvider.java Outdated Show resolved Hide resolved
...mework/security/oauth2/server/resource/authentication/OpaqueTokenAuthenticationProvider.java Outdated Show resolved Hide resolved
...mework/security/oauth2/server/resource/authentication/OpaqueTokenAuthenticationProvider.java Outdated Show resolved Hide resolved
...mework/security/oauth2/server/resource/authentication/OpaqueTokenAuthenticationProvider.java Outdated Show resolved Hide resolved
...mework/security/oauth2/server/resource/authentication/OpaqueTokenAuthenticationProvider.java Outdated Show resolved Hide resolved
...mework/security/oauth2/server/resource/authentication/OpaqueTokenAuthenticationProvider.java Outdated Show resolved Hide resolved
...auth2-core/src/main/java/org/springframework/security/oauth2/core/OAuth2TokenWithClaims.java Outdated Show resolved Hide resolved
...curity/oauth2/server/resource/authentication/IntrospectedOAuth2TokenAuthenticationToken.java Outdated Show resolved Hide resolved
This was referenced Jan 14, 2019
Open
Closed
@jzheaux
Copy link
Contributor Author

jzheaux commented Jan 14, 2019

@rwinch I've updated to use RestTemplate. Please feel free to comment there or anywhere else you see room for improvement.

@jzheaux jzheaux force-pushed the opaque branch 6 times, most recently from 978482f to ebadab9 Compare January 17, 2019 23:25
rwinch
rwinch requested changes Jan 23, 2019
View reviewed changes
...config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurer.java Outdated Show resolved Hide resolved
...config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurer.java Outdated Show resolved Hide resolved
.../oauth2-core/src/main/java/org/springframework/security/oauth2/core/OAuth2TokenAccessor.java Outdated Show resolved Hide resolved
...th2resourceserver-opaque/src/main/java/sample/OAuth2ResourceServerSecurityConfiguration.java Outdated Show resolved Hide resolved
samples/boot/oauth2resourceserver-opaque/src/main/resources/application.yml
oauth2:
resourceserver:
opaque:
introspection-uri: ${mockwebserver.url}/introspect
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to ensure we think about if/how this aligns with OAuth2 Log In. Do we support discovery? If an application is supporting oauth2 log in and resource server do we want them to have to configure the client id/secret twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think discovery makes sense through the issuer-uri once it supports the /.well-known/oauth-authorization-server endpoint, which defines introspection_endpoint as an attribute.

As for the client, ideally, the user wouldn't need to specify the same client twice as I imagine that the 95% case is that they are the same. If there is only one client in client registration, it seems reasonable to infer that client is the same for the introspection endpoint.

.../springframework/security/oauth2/server/resource/authentication/IntrospectionClaimNames.java Outdated Show resolved Hide resolved
@jzheaux jzheaux mentioned this pull request Jan 29, 2019
Closed
@jzheaux jzheaux force-pushed the opaque branch 3 times, most recently from fe6a7bc to a0a08d9 Compare February 1, 2019 13:39
@jzheaux jzheaux mentioned this pull request Feb 1, 2019
Closed
3 tasks
jgrandja
jgrandja requested changes Feb 5, 2019
View reviewed changes
Copy link
Contributor

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jzheaux I resolved my previous comments and added a couple new minor ones. This should be it on my end.

...k/security/oauth2/server/resource/authentication/OAuth2IntrospectionAuthenticationToken.java Outdated Show resolved Hide resolved
...ecurity/oauth2/server/resource/authentication/OAuth2IntrospectionAuthenticationProvider.java Outdated Show resolved Hide resolved
...ecurity/oauth2/server/resource/authentication/OAuth2IntrospectionAuthenticationProvider.java Outdated Show resolved Hide resolved
...ecurity/oauth2/server/resource/authentication/OAuth2IntrospectionAuthenticationProvider.java Show resolved Hide resolved
...config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurer.java Outdated Show resolved Hide resolved
@jzheaux jzheaux force-pushed the opaque branch 2 times, most recently from 89693f0 to eb80815 Compare February 5, 2019 22:48
@jgrandja
Copy link
Contributor

jgrandja commented Feb 6, 2019

Thanks @jzheaux. Good to go on my end.

As an FYI, it's best to not force push while in the review process as it's difficult to keep track of changes between review steps. I would just keep adding commits as you update based on feedback.

@rwinch rwinch added status: duplicate A duplicate of another issue New Feature in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Feb 7, 2019
@rwinch rwinch added this to the 5.2.0.M2 milestone Feb 7, 2019
rwinch
rwinch approved these changes Feb 7, 2019
View reviewed changes
Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now. Please feel free to squash, merge, and polish all metadata (labels, milestones, etc).

NOTE: Reactive support is being tracked at #6513

@jzheaux jzheaux merged commit 0428906 into spring-projects:master Feb 7, 2019
@jzheaux jzheaux deleted the opaque branch February 7, 2019 19:40
@jzheaux
Copy link
Contributor Author

jzheaux commented Feb 7, 2019

This is now merged into master.

@rwinch rwinch added the type: enhancement A general enhancement label May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgrandja jgrandja jgrandja approved these changes

@rwinch rwinch rwinch approved these changes

Assignees

@jzheaux jzheaux

Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Milestone
5.2.0.M2
Development

Successfully merging this pull request may close these issues.
3 participants
@jzheaux @jgrandja @rwinch