Skip to content

Don't use oidc scopes_supported for scope as default in ClientRegistrations #8790

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

Closed
wants to merge 2 commits into from
Closed

Don't use oidc scopes_supported for scope as default in ClientRegistrations #8790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
81b4d82
Don't use oidc scopes_supported for scope in ClientRegistrations
martin-v Jul 2, 2020
3938ea4
Don't set any scope as discussed in PR 8790
martin-v Aug 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...framework/security/config/oauth2/client/ClientRegistrationsBeanDefinitionParserTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ public void parseWhenIssuerUriConfiguredThenRequestConfigFromIssuer() throws Exc
assertThat(googleRegistration.getClientAuthenticationMethod()).isEqualTo(ClientAuthenticationMethod.BASIC);
assertThat(googleRegistration.getAuthorizationGrantType()).isEqualTo(AuthorizationGrantType.AUTHORIZATION_CODE);
assertThat(googleRegistration.getRedirectUri()).isEqualTo("{baseUrl}/{action}/oauth2/code/{registrationId}");
assertThat(googleRegistration.getScopes()).isEqualTo(StringUtils.commaDelimitedListToSet("openid,profile,email"));
assertThat(googleRegistration.getScopes()).isNull();
assertThat(googleRegistration.getClientName()).isEqualTo(serverUrl);

ProviderDetails googleProviderDetails = googleRegistration.getProviderDetails();
Expand Down
14 changes: 0 additions & 14 deletions ...ain/java/org/springframework/security/oauth2/client/registration/ClientRegistrations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@

import com.nimbusds.oauth2.sdk.GrantType;
import com.nimbusds.oauth2.sdk.ParseException;
import com.nimbusds.oauth2.sdk.Scope;
import com.nimbusds.oauth2.sdk.as.AuthorizationServerMetadata;
import com.nimbusds.openid.connect.sdk.op.OIDCProviderMetadata;
import net.minidev.json.JSONObject;
Expand All @@ -35,7 +34,6 @@
import org.springframework.security.oauth2.core.AuthorizationGrantType;
import org.springframework.security.oauth2.core.ClientAuthenticationMethod;
import org.springframework.security.oauth2.core.oidc.IdTokenClaimNames;
import org.springframework.security.oauth2.core.oidc.OidcScopes;
import org.springframework.util.Assert;
import org.springframework.web.client.HttpClientErrorException;
import org.springframework.web.client.RestTemplate;
Expand Down Expand Up @@ -236,12 +234,10 @@ private static ClientRegistration.Builder withProviderConfiguration(Authorizatio
throw new IllegalArgumentException("Only AuthorizationGrantType.AUTHORIZATION_CODE is supported. The issuer \"" + issuer +
"\" returned a configuration of " + grantTypes);
}
List<String> scopes = getScopes(metadata);
Map<String, Object> configurationMetadata = new LinkedHashMap<>(metadata.toJSONObject());

return ClientRegistration.withRegistrationId(name)
.userNameAttributeName(IdTokenClaimNames.SUB)
.scope(scopes)
.authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE)
.clientAuthenticationMethod(method)
.redirectUri("{baseUrl}/{action}/oauth2/code/{registrationId}")
Expand All @@ -268,16 +264,6 @@ private static ClientAuthenticationMethod getClientAuthenticationMethod(String i
+ "ClientAuthenticationMethod.NONE are supported. The issuer \"" + issuer + "\" returned a configuration of " + metadataAuthMethods);
}

private static List<String> getScopes(AuthorizationServerMetadata metadata) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of removing this method, let's apply this logic:

if metadata.getScopes() contains openid then
default to openid, profile, email
else
default to openid

This will ensure backwards compatibility when CommonOAuth2Provider.GOOGLE and CommonOAuth2Provider.OKTA is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But for CommonOAuth2Provider the scopes are defined, and this is also tested in CommonOAuth2ProviderTests so these stick with openid, profile, email. You are probaply mislead by the bad test-data naming in ClientRegistrationsBeanDefinitionParserTests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jgrandja Did you get a chance to look at that remark?

Copy link
Contributor

Choose a reason for hiding this comment

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

@martin-v Apologies, my last comment was not that clear.

Let's assume the user has configured this:

spring:
  security:
    oauth2:
      client:
        registration:
          google:
            client-id: id
            client-secret: secret
        provider:
          google:
            issuer-uri: https://accounts.google.com

Google discovery endpoint returns openid, profile, email in the scopes_supported parameter, which initializes the ClientRegistration with the 3 scopes.

The changes in this PR would initialize the ClientRegistration with just the openid scope, which changes the runtime behaviour as the UserInfo endpoint would not be called by OidcUserService as it doesn't contain at least one of the accessible scopes.

The logic proposed in previous comment would preserve this behaviour. Furthermore, if the openid scope is returned in scopes_supported then we know the provider supports OpenID Connect and the UserInfo endpoint so it makes sense to also default to profile, email to enable the UserInfo endpoint call.

Makes sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

profile, email are optional scopes in openid-connect, see https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims

One option is if the profile, email is listed in scopes_supported then we add these, but this still can cause the problem from #8514 (If it is listed in 'scopes_supported', this does not mean that it is supported for that particular client)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jgrandja sry, didn't see it.

If we leave it empty we still have the problem mentioned in #8514 (comment) but we can't fix both issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I can do the change, if you still prefer the empty solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

@martin-v Regarding...

If we leave it empty we still have the problem mentioned in #8514

I realize that openid is a required scope for OIDC. However, as I mentioned in my previous comment

We cannot assume that all ClientRegistation's are configured for oauth2Login(), therefore we should not default to openid or any of the additional OIDC scopes. There may be ClientRegistation's that are configured for OAuth 2.0 authorization_code or client_credentials and defaulting to openid would be invalid.

Please go ahead with the change on NOT assigning any defaults to ClientRegistration.scope(). This change will NOT be backported and will only go into the 5.4 release.

Copy link
Contributor

Choose a reason for hiding this comment

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

@martin-v 5.4.0 is being released Sep 2 and I need to get this change in. Are you able to update, as per my last comment, this week?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, didn't notice your response. I will update the PR in a few minutes.

Scope scope = metadata.getScopes();
if (scope == null) {
// If null, default to "openid" which must be supported
return Collections.singletonList(OidcScopes.OPENID);
} else {
return scope.toStringList();
}
}

private ClientRegistrations() {}

}
37 changes: 1 addition & 36 deletions ...java/org/springframework/security/oauth2/client/registration/ClientRegistrationsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ private void assertIssuerMetadata(ClientRegistration registration,
assertThat(registration.getAuthorizationGrantType()).isEqualTo(AuthorizationGrantType.AUTHORIZATION_CODE);
assertThat(registration.getRegistrationId()).isEqualTo(this.server.getHostName());
assertThat(registration.getClientName()).isEqualTo(this.issuer);
assertThat(registration.getScopes()).containsOnly("openid", "email", "profile");
assertThat(registration.getScopes()).isNull();
assertThat(provider.getAuthorizationUri()).isEqualTo("https://example.com/o/oauth2/v2/auth");
assertThat(provider.getTokenUri()).isEqualTo("https://example.com/oauth2/v4/token");
assertThat(provider.getJwkSetUri()).isEqualTo("https://example.com/oauth2/v3/certs");
Expand Down Expand Up @@ -222,41 +222,6 @@ public void issuerWhenOAuth2ContainsTrailingSlashThenSuccess() throws Exception
assertThat(this.issuer).endsWith("/");
}

/**
* https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata
*
* RECOMMENDED. JSON array containing a list of the OAuth 2.0 [RFC6749] scope values that this server supports. The
* server MUST support the openid scope value.
* @throws Exception
*/
@Test
public void issuerWhenScopesNullThenScopesDefaulted() throws Exception {
this.response.remove("scopes_supported");

ClientRegistration registration = registration("").build();

assertThat(registration.getScopes()).containsOnly("openid");
}

@Test
public void issuerWhenOidcFallbackScopesNullThenScopesDefaulted() throws Exception {
this.response.remove("scopes_supported");

ClientRegistration registration = registrationOidcFallback("", null).build();

assertThat(registration.getScopes()).containsOnly("openid");
}

@Test
public void issuerWhenOAuth2ScopesNullThenScopesDefaulted() throws Exception {
this.response.remove("scopes_supported");

ClientRegistration registration = registrationOAuth2("", null).build();

assertThat(registration.getScopes()).containsOnly("openid");
}


@Test
public void issuerWhenGrantTypesSupportedNullThenDefaulted() throws Exception {
this.response.remove("grant_types_supported");
Expand Down