Revert OAuth2 Client Registration Grant Type Hierarchy

[rwinch]

The changes in f5deebf#diff-b716836901d5fea39e485e35997f2f62 make it so that the grant type is in the hierarchy. I'm not sure if this is a good long term solution since there will likely come a time (soon) where we support multiple grant types in a registration (with one being "preferred"). When a user requests a token they would then specify which grant type if they wanted something different than the default.

With these changes in mind it is going to make configuring the code challenging because the user would need to duplicate the configuration. For example:

spring:
  security:
    oauth2:
      client:
        provider:
          idp:
            issuer-uri: https://example.com/
        registration:
          authorizationcode:
            idp-ac:
              provider: idp
              client-id: client-id
              client-secret: client-secret
          clientcredentials:
            idp-cc:
              provider: idp
              client-id: client-id
              client-secret: client-secret

One option would be to allow additional grant types property on the registration. However, this becomes ambiguous how the user should configure it. Should they place it under authorizationcode or clientcredentials? If they place it under a grant type that is missing a property for the additional grant type then, the would get an error too. For example:

spring:
  security:
    oauth2:
      client:
        provider:
          idp:
            issuer-uri: https://example.com/
        registration:
          clientcredentials:
            idp-cc:
              client-id: client-id
              client-secret: client-secret
              additional_grant_types: authorization_code
              redirect-url: /foo/bar # redirect-url is not needed for client-credentials

I'd like to consider going back to the original way. If the authorization_grant_type is provided, the default redirect-url is then mapped accordingly.

spring:
  security:
    oauth2:
      client:
        provider:
          idp:
            issuer-uri: https://example.com/
        registration:
          idp:
            client-id: client-id
            client-secret: client-secret
            authorization_grant_type: authorization_code

Related: #14554

[mbhave]

@rwinch If there is going to be support for a "preferred" grant type, I don't see why additional_grant_types would be misleading. The default one would be the that's under the key and the additional ones would be supported if the user specifies that in the request.

If they place it under a grant type that is missing a property for the additional grant type then, the would get an error too

We don't have this sort of validation and when/if we do add validation we can take such factors into account.

[rwinch]

We don't have this sort of validation and when/if we do add validation we can take such factors into account.

It was my understanding that part of the benefit of the new approach was that not all grant types need the same properties. This seemed to imply that client credentials wouldn't have a redirect uri. If it does have the redirect uri property, then this is not really an advantage of the new approach. Perhaps I am missing something?

[snicoll]

This seemed to imply that client credentials wouldn't have a redirect uri.

Grant types can have totally different properties with fine-tuned names and/or description. If a property is not valid for a particular grand type, it will simply not be present (and the IDE will not offer auto-completion for it and complain if you try to set it). Besides, the binder won't be able to bind it simply because it isn't there.

[philwebb]

One of the reasons we decided to split the registration types was that although both login and authorization-code have a property named redirect-uri, they actually mean different things depending on the context. By creating two distinct classes we're able to provide different descriptions for them:

For login we have:

Redirect URI. May be left blank when using a pre-defined provider.

For authorization-code we have:

Redirect URI for the registration.

Whilst it's very tempting to create just a single data type, I think keeping them distinct is important.

there will likely come a time (soon) where we support multiple grant types in a registration

We'll have to consider carefully how this will impact users. This might be an example of something that's more complex and best left to the user to manage themselves with bean registration. I'm much keener on making the 80% use-case super solid, rather than adding complexity to the properties. It might also be the case that property duplication is the least bad option. Especially if it turns out that certain registration types can't be mixed.

For example, if you create a single property that represents both login and authorization-code, then what does redirect-uri mean? Is the same URL used for both? Or perhaps that's just a combination that doesn't make sense?

[rwinch]

One of the reasons we decided to split the registration types was that although both login and authorization-code have a property named redirect-uri, they actually mean different things depending on the context

Can you expand on this? In both cases the redirect uri is where the authorization code is being sent to.

Whilst it's very tempting to create just a single data type, I think keeping them distinct is important.

I don't think this makes sense. This is not how OAuth specification or any discussions around OAuth are conveyed. By creating our own type hierarchy, we are now having to use our own terminology, definitions, and explanations.

My fear is that this hierarchy adds complexity to the users. I've already received feedback from two users stating that it was not very intuitive. If we go back to the previous way, all of the mappings become a one to one with existing OAuth terminology which simplifies conversations and understanding for users.

[mbhave]

In both cases the redirect uri is where the authorization code is being sent to.

My understanding was that for the login client registrations the redirect uri needs to match the filterProcessesUrl for the OAuth2LoginAuthenticationFilter and the default for that is /login/oauth2/code/*. So even though the redirect uri for both login and authorization code is where the authorization code is sent, they have different requirements from an end user perspective.

It's probably best to discuss this in person next week.

[mraible]

Hey all,

I'm creating an app that has OAuth 2.0 login and a resource server in the same app. I've found that I have to duplicate values for two keys to make it work.

oidc.issuer-uri: https://dev-737523.oktapreview.com/oauth2/default

spring:
  security:
    oauth2:
      client:
        provider:
          okta:
            issuer-uri: ${oidc.issuer-uri}
        registration:
          login:
            okta:
              client-id: XXX
              client-secret: YYY
              scope: openid email profile
      resourceserver:
        jwt:
          issuer-uri: ${oidc.issuer-uri}

As someone that knows a bit about OAuth 2.0, this makes sense. Yes, there is duplication, but I'm enabling two separate features.

In previous versions, I believe @EnableOAuth2Sso and @EnableResourceServer activated the different features. Will that still be a possibility in Spring Boot 2.1?

Unfortunately, I don't know that this will make sense to the average user. Is it possible to hide the complexity and have "oauth2" or "oidc" be top-level keys? That way, users could even memorize the key names! ;)

For example:

oidc:
  issuer-uri:
  client-id:
  client-secret:

[jgrandja]

I'm chiming in here a little late into the conversation but quite honestly I wasn't aware of this change until I started working on my demo the week before SpringOne. I noticed the property deprecations in M3 so decided to go with M2 for my demo instead as I didn't really want to entertain any questions around the deprecation.

I do remember @mbhave mentioning doing some re-factoring on the properties but it was in passing as we were talking about an unrelated issue.

Anyway, when I first discovered the new property hierarchy, I will admit it did not make sense to me - grouping client registrations by grant type. The OAuth 2.0 Authorization Framework does not discuss any kind of grouping of Client Registrations by grant_type. The one grouping it does mention is Client Types - public or confidential. Nor do any of the extension specifications mention any kind of grouping by grant_type.

If we look at some of the grant_type's defined in the various specifications:

OAuth 2.0 Authorization Framework
Authorization Code
Implicit
Resource Owner Password Credentials
Client Credentials

OpenID Connect Core 1.0
Authorization Code
Implicit
Hybrid ** not actually a grant_type - see below

With these 7 grant types, we have a potential of 7 different types in the property hierarchy.
However, the Authorization Code is defined both in OAuth 2.0 and OIDC with some variations. How would this be handled with the new hierarchy? To complicate things further, the Hybrid flow defined in OIDC uses the authorization_code grant when requesting to the Token Endpoint.

Let's also consider these 2 grants:
Refresh Token
Token Exchange grant_type=urn:ietf:params:oauth:grant-type:token-exchange

We've implemented Refresh Token in 5.1 and Token Exchange will come in 5.2. However, a user would never configure a Client Registration with either of these grants. These grants can only be performed after a Client has been authorized by one of the previous grant_type mentioned, for example, authorization_code.

To take this one step further, the OAuth 2.0 Authorization Framework allows for Extension Grants. So a Provider is free to implement a custom grant_type that is not even a standard but is leveraged by clients for that specific provider implementation. Token Exchange is an example of an Extension Grant that is currently on the standards track.

There are other grant_type that I haven't even mentioned here. So you can imagine how many different grant types we will have in the property hierarchy if we follow this new model. It will get complicated.

As far as I see it, the grant_type is an attribute of a Client Registration that gives it the capability to perform a specific authorization grant flow to obtain an access token in the end. On the Provider itself, the Client may be registered to support multiple grant_types, for example, authorization_code, refresh_token, and client_credentials. We don't allow multiple authorization-grant-type to be configured in Spring Security's ClientRegistration because that would be really confusing to the user. Is this ClientRegistration configured for authorization_code or client_credentials. And if it's configured for client_credentials than which scope are used for that flow compared to the authorization_code flow? So we restrict a ClientRegistration to be configured with only one authorization-grant-type to simplify things.

Anyway, I think I provided enough of my points for now. I agree with @rwinch that we should consider going back to the original hierarchy. I just think the new hierarchy will confuse the user and complicate things in the long run.

[vpavic]

I think the most important point here is in @philwebb's comment:

This might be an example of something that's more complex and best left to the user to manage themselves with bean registration. I'm much keener on making the 80% use-case super solid, rather than adding complexity to the properties.

Spring Boot has always been focused on supporting the typical/simple use case, and making sure that whatever is exposed via properties is well supported in IDEs (property description, auto-completion). From that standpoint I understand the motivation for the new approach and wouldn't want to see support for every use case tried being stuffed in. The whole OAuth 2.0 topic is too complex to be able to do that elegantly.

Also let's not forget that one of the bigger changes made in Spring Boot 2.0 were the simplifications of security configuration with users now being guided to use Spring Security's configuration facilities directly. The reason behind that was the same - complexity. Talking about supporting every possible use case feels like going back in the opposite direction.

To be honest, while it's simple and appealing, for production systems that (should) care about security I personally wouldn't recommend managing client registrations in a static manner at all. The primary reason is that it should be possible to rotate the client secret, which is something that any serious authorization servers provides, without requiring downtime on the client.

[philwebb]

This Spring Security issue will align reactive and servlet support and remove the need for the specific "/login/oauth2/code/*" pattern that is currently needed for the OAuth2LoginAuthenticationFilter to be used. This should help as it means the redirect URL is no longer the driving force behind which filter is used.

[mbhave]

After discussing further with @rwinch and @jgrandja, we've decided to go back to the old way of configuring client registrations. The reactive OAuth2 client support allows the same client registration to be used for multiple flows. It checks whether the user is logged in and performs login if not. If the user is logged in, it will be plain authorization code flow.

The issue that @philwebb linked to in the previous comment will mean that this is true for the servlet support as well and remove the need to distinguish between login and authorization code flow clients.