Skip to content

ClientRegistration contains Provider Configuration Metadata #5729

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
Closed
wants to merge 6 commits into from
Closed

ClientRegistration contains Provider Configuration Metadata #5729

wants to merge 6 commits into from

Conversation

jgrandja
Copy link
Contributor

Fixes gh-5540

@jgrandja jgrandja requested a review from rwinch August 23, 2018 20:36
@rwinch rwinch self-assigned this Aug 23, 2018
rwinch
rwinch requested changes Aug 23, 2018
View reviewed changes
...st/java/org/springframework/security/oauth2/client/registration/ClientRegistrationTests.java Outdated
@@ -42,6 +44,13 @@
private static final String TOKEN_URI = "https://provider.com/oauth2/token";
private static final String JWK_SET_URI = "https://provider.com/oauth2/keys";
private static final String CLIENT_NAME = "Client 1";
private static final Map<String, Object> PROVIDER_CONFIGURATION_METADATA = new LinkedHashMap<>();

static {
Copy link
Member

Choose a reason for hiding this comment

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

Please extract initialization into a method.

Copy link
Member

Choose a reason for hiding this comment

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

This isn't truly final because Map is mutable. Please change the Map to be immutable or (preferably) remove the static modifier.

@jgrandja
Copy link
Contributor Author

@rwinch I applied the polish to the test.

rwinch
rwinch requested changes Aug 27, 2018
View reviewed changes
...rc/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java Outdated
@@ -153,6 +155,7 @@ public String toString() {
private String tokenUri;
private UserInfoEndpoint userInfoEndpoint = new UserInfoEndpoint();
private String jwkSetUri;
private Map<String, Object> configurationMetadata;
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to make this by default empty so user's don't need to worry about NPE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NPE would never happen with configurationMetadata - see where it gets init

...rc/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java Outdated
@@ -262,6 +275,7 @@ public static Builder withRegistrationId(String registrationId) {
private AuthenticationMethod userInfoAuthenticationMethod = AuthenticationMethod.HEADER;
private String userNameAttributeName;
private String jwkSetUri;
private Map<String, Object> configurationMetadata;
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to make this by default empty so user's don't need to worry about NPE

...rc/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java Outdated
* @return the {@link Builder}
*/
public Builder providerConfigurationMetadata(Map<String, Object> configurationMetadata) {
this.configurationMetadata = configurationMetadata;
Copy link
Member

Choose a reason for hiding this comment

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

In combination with making configurationMetadata empty, we should also prevent this from be null

@jgrandja
Copy link
Contributor Author

jgrandja commented Sep 4, 2018

@rwinch I applied the requested updates to ClientRegistration.

rwinch
rwinch requested changes Sep 5, 2018
View reviewed changes
...rc/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java Outdated
* @return the {@link Builder}
*/
public Builder providerConfigurationMetadata(Map<String, Object> configurationMetadata) {
if (!CollectionUtils.isEmpty(configurationMetadata)) {
Copy link
Member

Choose a reason for hiding this comment

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

This is not actually setting the value (as the Javadoc states) if it is empty. We should avoid setting the value to null, but we should not avoid setting it to an empty collection if the input is empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I applied the update @rwinch

rwinch
rwinch requested changes Sep 5, 2018
View reviewed changes
...rc/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java Outdated
@@ -453,7 +453,7 @@ public Builder jwkSetUri(String jwkSetUri) {
* @return the {@link Builder}
*/
public Builder providerConfigurationMetadata(Map<String, Object> configurationMetadata) {
if (!CollectionUtils.isEmpty(configurationMetadata)) {
if (configurationMetadata != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a test since this was a problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwinch I added a couple of tests

@rwinch
Copy link
Member

rwinch commented Sep 5, 2018

@jgrandja The changes look good. Once the Travis build passes, please go ahead and squash and merge.

@jgrandja jgrandja added status: duplicate A duplicate of another issue type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) OIDC labels Sep 5, 2018
@jgrandja jgrandja added this to the 5.1.0.RC2 milestone Sep 5, 2018
@jgrandja
Copy link
Contributor Author

jgrandja commented Sep 5, 2018

Merged via 057587e

@jgrandja jgrandja closed this Sep 5, 2018
@jgrandja jgrandja deleted the gh-5540-provider-config branch September 7, 2018 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwinch rwinch rwinch approved these changes

Assignees

@rwinch rwinch

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.1.0.RC2
Development

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