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

[martin-v]

fixes #8514

[pivotal-issuemaster]

@martin-v Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@martin-v Thank you for signing the Contributor License Agreement!

[jgrandja]

Thanks for the PR @martin-v. Please see review comment.

[jgrandja]

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.

[martin-v]

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

[martin-v]

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

[jgrandja]

@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?

[martin-v]

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)

[martin-v]

@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.

[martin-v]

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

[jgrandja]

@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.

[jgrandja]

@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?

[martin-v]

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

[jgrandja]

Thanks for the updates @martin-v ! This is now in master.