Skip to content

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
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
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
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
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
Development

Successfully merging this pull request may close these issues.

3 participants