scopes_supported metadata should not be used as default in ClientRegistrations

[martin-v]

Describe the bug
The oauth2-client requests all available scopes on the authorization server by default. That includes also scopes for other clients.

ClientRegistration.getScopes() copies all scopes_supported from authorization server metadata into scope config for this client. So the oauth2-client requested on each authorization all of these scopes.

As Keycloak has recently fixed a bug now throwing error on invalid scope requests, this bug will occur for more people.

To Reproduce

• Install current keycloak >= 10.0.0
• Create new client
• Remove some default keycloak or a new created client scopes from Assigned Default Client Scopes and Assigned Optional Client Scopes of this client
• Try to start a session with this client and oauth2-client.
• Don't configure spring.security.oauth2.client.registration.${name}.scope (This is a workaround to fix the problem)

Expected behavior

As scopes_supported lists all scopes for all clients on this authorization server it's not a good default.

scopes_supported
RECOMMENDED. JSON array containing a list of the OAuth 2.0
[RFC6749] "scope" values that this authorization server supports.
Servers MAY choose not to advertise some supported scope values
even when this parameter is used.
Source: https://tools.ietf.org/html/rfc8414#section-2

Also, considering that the scope parameter is optional.

scope
OPTIONAL. The scope of the access request as described by
Section 3.3.
Source https://tools.ietf.org/html/rfc6749#section-4.1.1

The best default should be just leave it empty.

[jgrandja]

Thanks for the report @martin-v.

The best default should be just leave it empty.

I don't think this makes sense as the client won't request any scopes if the scope property is not configured.

Don't configure spring.security.oauth2.client.registration.${name}.scope (This is a workaround to fix the problem)

This is not a workaround. We typically provide defaults, in order to reduce configuration, but also provide the option to override the default.

I'm going to close this as works as designed.

[shelgen]

@jgrandja I believe there's a misunderstanding here, but it could very well be mine.

When autoconfiguring client registrations in Spring Boot, OAuth2ClientPropertiesRegistrationAdapter#getClientRegistration in turn calls ClientRegistrations#fromIssuerLocation, which gets the supported scopes from the authorization server.
ClientRegistrations#withProviderConfiguration then maps this to a ClientRegistration.Builder with "scopes" set to the full set of supported scopes of the whole authorization server.

Back in OAuth2ClientPropertiesRegistrationAdapter#getClientRegistration, the Builder based on the provider is merged with the Spring Boot properties, which makes the scope property overwrite the scope in the Builder. However, this is only done if the scope property is not null, otherwise the existing value in the Builder is kept and used for further authorization.

So, if scope is not set in the Spring Boot properties, the full list of all existing scopes on the authorization server will be requested for all clients, even though each client most likely only supports a subset of the scopes.
As @martin-v explains, this problem now becomes very visible for people due to the fix in Keycloak 10 of actively rejecting invalid scopes requested for a client. That is also why I came here, after quite a bit of debugging.

@martin-v suggests that rather than having the default requested scopes be all scopes available on the whole authorization server, it should be no scopes, meaning the parameter should not be sent at all. As the parameter is optional, this should be OK. Another point to this is that there is currently no way through properties to choose not to include the scopes parameter in the request.

Am I the one misunderstanding this? If so, is there a reason for having that default value that I'm not seeing?

[jgrandja]

@shelgen Thanks for providing your detailed feedback. After reading over the comments again I realized that my initial analysis was incorrect.

As @martin-v suggests:

The best default should be just leave it empty.

This is actually correct.

Currently, all entries from scopes_supported parameter in discovery response is defaulted for ClientRegistration.scopes, which is not correct.

In most cases, the ClientRegistration.scopes should be explicitly configured since only the application knows which scopes the client needs to request depending on the flow being used.

Apologies for my misunderstanding @martin-v.
As you suggested, we should leave the scopes empty for this fix.
Would you be interested in submitting a PR for this fix?

[martin-v]

@jgrandja I can implement it but I discovered another problem. I'm no longer sure that leave it empty is the best option. Because I discovered that the OpenID Connect Spec requires a scope.

OpenID Connect uses the following OAuth 2.0 request parameters with the Authorization Code Flow:

• scope
  REQUIRED. OpenID Connect requests MUST contain the openid scope value. If the openid scope value is not present, the behavior is entirely unspecified.

Source https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest

In my optinion the question is do more user use OAuth 2.0 without OpenID Connect or with?

@shelgen yes, if we decide to leave it empty we should not be sent the parameter at all

[jgrandja]

@martin-v Good catch. Yes, we should default ClientRegistration.scopes to contain openid only, so it doesn't break existing oauth2Login() clients using OpenID Connect.

Thanks for taking this on!

[martin-v]

In #8790 (comment) is a discussion ongoing what is the best solution to fix this issue. Maybe someone here can give us some additional aspects.