We should @Deprecated CommonOAuth2Provider.OKTA as it doesn't provide much value in defaults.

The original intent of CommonOAuth2Provider is to provide sensible defaults for oauth2Login() when using a common provider, e.g. Google, Okta, Github and Facebook.

The following lists all the possible attributes that MAY be defaulted for ClientRegistration:

  • clientAuthenticationMethod()
  • authorizationGrantType()
  • redirectUri()
  • scope()
  • authorizationUri()
  • tokenUri()
  • jwkSetUri()
  • issuerUri()
  • userInfoUri()
  • userNameAttributeName()
  • clientName()

For CommonOAuth2Provider.OKTA, only the following attributes are defaulted:

  • clientAuthenticationMethod()
  • authorizationGrantType()
  • redirectUri()
  • scope()
  • userNameAttributeName()
  • clientName()

However, ClientRegistration.Builder.build() provides defaults for clientAuthenticationMethod() and clientName(). As well, DefaultOidcUser provides a default for userNameAttributeName().

Therefore, the only attributes that are truly defaulted for CommonOAuth2Provider.OKTA are:

  • authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE)
  • redirectUri("{baseUrl}/{action}/oauth2/code/{registrationId}")
  • scope("openid", "profile", "email")

Furthermore, these defaults are only applicable to the oauth2Login() feature which implements OpenID Connect. For all other ClientRegistration's that use Okta as the provider, but perform other flows other than OpenID Connect (e.g. client_credentials), these defaults would not be valid and the application would need to override them anyway.

It's also very likely that redirectUri() and scope() would be overridden to provide a custom setting.

Based on this detailed analysis, we should @Deprecated CommonOAuth2Provider.OKTA as it doesn't provide much value in defaults.

Comment From: jgrandja

@rwinch I'm curious on your thoughts on @Deprecated CommonOAuth2Provider altogether?

As mentioned in previous comment:

The original intent of CommonOAuth2Provider is to provide sensible defaults for oauth2Login() when using a common provider, e.g. Google, Okta, Github and Facebook.

Most of the defaults provided by CommonOAuth2Provider are only applicable to ClientRegistration's using the oauth2Login(), e.g. OpenID Connect. For all other authorization_grant_type, the defaults would not be applicable or valid. The only defaults that are common across all authorization_grant_type are the endpoint URI's, e.g. authorizationUri(), tokenUri(). However, even these can change in time as Google and Facebook have versioned API's so this would need to be maintained (updated) on our end.

I realize CommonOAuth2Provider allows users to provide minimal configuration for Google, GitHub and Facebook for oauth2Login(), but how many applications are leveraging these providers in a production environment? I suspect these common providers are heavily used in sample applications but in production applications I would guess other providers are being used.

Comment From: jgrandja

Related spring-boot#25549

Comment From: rwinch

However, even these can change in time as Google and Facebook have versioned API's so this would need to be maintained (updated) on our end.

I think Facebook has updated URLs, but the old URL still works. I don't think Google has changed their URLs since this was introduced 4 years ago. I'm not sure this is a good reason to stop support simplifying configuration for users.

Most of the defaults provided by CommonOAuth2Provider are only applicable to ClientRegistration's using the oauth2Login(), e.g. OpenID Connect. For all other authorization_grant_type, the defaults would not be applicable or valid.

I think that there is a lot of value in ensuring oauth log in is easy with these common providers. Perhaps the CommonOAuth2Provider is not the best name and we need to have something that distinguishes that it is for OAuth2 Log In rather than just removing it. However, I don't reach the same conclusion that because this is only valuable for OAuth2 Log In we should deprecate this (unless we replace it with a more meaningful name and preserve the same behavior for users).

Personally, I feel like the approach that @mbhave proposed in https://github.com/spring-projects/spring-boot/issues/25549#issuecomment-867226828 was very pragmatic. Just because we aren't solving for every case, doesn't mean we need to get rid of it so long as it can easily be customized.

Comment From: mbhave

I think CommonOAuth2Provider provides value for providers such as Facebook, Google and Github (if the URLs still work). For Okta, I agree with @Kehrlann and @jgrandja that there isn't much value add. For Facebook, Google and, Github, you can skip the provider configuration entirely if you don't want to customize anything. That's not the case with Okta and you need to either specifiy the authorization and token endpoints or the issuer uri. In fact, the one thing that it is defaulting, which is the scopes, is something we would rather have the user be explicit about. And once you have the provider configuration, adding scopes is just one more line in the properties. Given this, deprecating CommonOAuth2Provider.OKTA seems reasonable to me.

I like @rwinch's suggestion about renaming CommonOAuth2Provider to make it more obvious that it's oauth2Login specific, or at the very least updating the javadoc to say so.

Comment From: jgrandja

@rwinch

I think that there is a lot of value in ensuring oauth log in is easy with these common providers. Perhaps the CommonOAuth2Provider is not the best name and we need to have something that distinguishes that it is for OAuth2 Log In rather than just removing it.

Good point. I agree. I obviously made a quick decision about deprecating :wink:

I think renaming it is a good idea to be more clear to users. Possible name OAuth2LoginClientRegistrationDefaults. I logged this new issue gh-10321.

Personally, I feel like the approach that @mbhave proposed in https://github.com/spring-projects/spring-boot/issues/25549#issuecomment-867226828 was very pragmatic.

Agreed. cc/ @mbhave

I still think we should @Deprecated CommonOAuth2Provider.OKTA as I don't believe it provides much value.

Therefore, the only attributes that are truly defaulted for CommonOAuth2Provider.OKTA are:

  • authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE)
  • redirectUri("{baseUrl}/{action}/oauth2/code/{registrationId}")
  • scope("openid", "profile", "email")

@rwinch What are your thoughts on this?

Comment From: jgrandja

@rwinch and I had an offline discussion and after further analysis, I'm reverting my original proposal on:

I still think we should @Deprecated CommonOAuth2Provider.OKTA as I don't believe it provides much value.

Therefore, the only attributes that are truly defaulted for CommonOAuth2Provider.OKTA are:

  • authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE)
  • redirectUri("{baseUrl}/{action}/oauth2/code/{registrationId}")
  • scope("openid", "profile", "email")

I went through the oauth2Login() sample for Okta to re-familiarize myself with the "Getting Started" experience and how that would change in Spring Security 6.0 with the removal of CommonOAuth2Provider.OKTA.

As it stands today, the spring.security.oauth2.client.registration properties required for Okta are:

okta:
   provider: okta
   client-id: id
   client-secret: secret

If CommonOAuth2Provider.OKTA was removed in Spring Security 6.0, the required properties would then be:

okta:
   provider: okta
   client-id: id
   client-secret: secret
   authorization-grant-type: authorization_code
   redirect-uri: "{baseUrl}/login/oauth2/code/{registrationId}"
   scope: "openid"

This change would degrade the "Getting Started" experience for Okta, since the application now has to configure authorization-grant-type, redirect-uri and scope properties.

Based on this, we're going to leave CommonOAuth2Provider.OKTA as-is.