Skip to content

Make jwks_uri optional for RFC 8414 and required for OpenID Connect #7573

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
Merged
merged 1 commit into from
Nov 11, 2019
Merged

Make jwks_uri optional for RFC 8414 and required for OpenID Connect #7573

merged 1 commit into from
Nov 11, 2019

Conversation

rhamedy
Copy link
Contributor

@rhamedy rhamedy commented Oct 27, 2019

Relating to #7512

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 27, 2019
@rhamedy rhamedy marked this pull request as ready for review October 30, 2019 04:09
@rhamedy
Copy link
Contributor Author

rhamedy commented Oct 30, 2019

Hi @jzheaux, I would appreciate the feedback.

I also noticed that the OIDCProviderMetadata.class constructor has the following validation

if (jwkSetURI == null)
	throw new IllegalArgumentException("The public JWK set URI must not be null");

As per our conversation on the issue, I have pushed my changes and kept the error message the same as above unless you want it to be different.

@rwinch rwinch requested a review from jzheaux October 30, 2019 15:01
@jzheaux jzheaux added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 30, 2019
@jzheaux jzheaux self-assigned this Oct 30, 2019
@jzheaux jzheaux added this to the 5.2.1 milestone Oct 30, 2019
jzheaux
jzheaux requested changes Oct 30, 2019
View reviewed changes
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks, @rhamedy! I've left some feedback inline.

@rhamedy
Copy link
Contributor Author

rhamedy commented Oct 31, 2019

@jzheaux pushed my changes relating to your feedback. I will squash my commits once there are no other changes needed to make. 👍

@rhamedy
Copy link
Contributor Author

rhamedy commented Oct 31, 2019

I find it difficult to come up with good method names. If you think the new method names could be improved, then please feel free to suggest.

@jzheaux jzheaux modified the milestones: 5.2.1, 5.2.x Nov 4, 2019
jzheaux
jzheaux requested changes Nov 4, 2019
View reviewed changes
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

I think the method names look great, @rhamedy, thanks for being so open to feedback.

I've left a few inline comments - I think we are just about there.

@rhamedy
Copy link
Contributor Author

rhamedy commented Nov 4, 2019

Hi @jzheaux, replied to you comments and pushed the recent changes up 👍

@jzheaux jzheaux modified the milestones: 5.2.x, 5.2.2 Nov 5, 2019
@rhamedy
Make jwks_uri optional for RFC 8414 and Required for OpenID Connect
9ba6309 
OpenID Connect Discovery 1.0 expects the OpenId Provider Metadata 
response is expected to return a valid jwks_uri, however, this field is 
optional in the Authorization Server Metadata response as per RFC 8414
specification.

Fixes spring-projectsgh-7512
@rhamedy
Copy link
Contributor Author

rhamedy commented Nov 5, 2019

Thank you for approving the changes. I have squashed my commits 👍

@jzheaux jzheaux added the status: duplicate A duplicate of another issue label Nov 11, 2019
@jzheaux jzheaux merged commit 58ca81d into spring-projects:master Nov 11, 2019
@jzheaux
Copy link
Contributor

jzheaux commented Nov 11, 2019

Thanks, @rhamedy! This is now merged into master.

@rhamedy rhamedy deleted the gh-7512 branch November 12, 2019 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jzheaux jzheaux jzheaux 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.2
Development

Successfully merging this pull request may close these issues.
3 participants
@rhamedy @jzheaux @spring-projects-issues