Add auto-configuration for spring-security-oauth2-client

[jgrandja]

This PR provides auto-configuration for Spring Security 5.0 OAuth 2.0 / OpenID Connect 1.0 client support.

The default auto-configuration will register a ClientRegistrationRepository with the context and
will enable httpSecurity.oauth2Login() based on a number of conditions.

[jgrandja]

Related #10236

[wilkinsona]

Thanks for the PR, @jgrandja.

Do you have an opinion on what should happen to the existing OAuth-related auto-configuration in Boot? Given that the separate OAuth project is, as I undestand it, kinda dormant now, it would seem desirable for it to be dropped in Boot 2.0 so that we're not tied into maintaining it for longer than we'd like even if that means losing some auto-configured functionality.

[wilkinsona]

I think this should be replaced with one or more @ConfigurationProperties classes with fields initialised with the desired default values.

[jgrandja]

I did look at potentially using @ConfigurationProperties initially but it did not suit my requirements because it supports a static base property path whereas the base property path for client properties is dynamic.

Given the following format for client properties:

[client-property-prefix].[client-key].[property-name]=[property-value]

for example, security.oauth2.client.google.client-id=google-client-id

the [client-key] (google) is dynamic and may be any value assigned by the user.

Here's a use case. The user wants to configure the following 2 google clients.

security.oauth2.client.google-login.client-id=google-login-client-id
and
security.oauth2.client.google-calendar.client-id=google-calendar-client-id

The google-login client will be used for oauth2Login() whereas the google-calendar client will be used for accessing the Calendar API.

The user is not fixed to using google for the client-key. They may choose whatever name for client-key that makes sense for their configuration.

NOTE: The client-key is only used as a logical separator for client configurations. Nothing more than that. It is not used when auto-configuring ClientRegistration instances. Another thing to note as I indirectly mentioned is that the user is able to configure more than one client as they wish - each configuration separated by the client-key. This is a new capability in Spring Security 5 compared to spring-security-oauth where you are limited to configuring only 1 client.

We're also providing a set of common defaults for well-known providers via spring-security-oauth2-client-defaults.properties in order to minimize the configuration for the user. So if the user wants to configure 1 google client only than they can leverage the defaults and would only have to configure the following:

security.oauth2.client.google.client-id
security.oauth2.client.google.client-secret

However, if they decided to name their client-key to google-login instead, than they can still leverage the default property values in the Environment but their configuration would need to look like this:

security.oauth2.client.google-login.client-id=google-client-id
security.oauth2.client.google-login.client-secret=google-client-secret
security.oauth2.client.google-login.authorization-uri=${security.oauth2.client.google.authorization-uri}
security.oauth2.client.google-login.token-uri=${security.oauth2.client.google.token-uri}
...

...leveraging property placeholders.

Hopefully that explains things. And if you have another suggestion for implementation than please let me know.

[wilkinsona]

Thanks for the explanation.

You could largely do this with @ConfigurationProperties with a Map<String, Foo> property where Foo would be a pojo with client-id, client-secret etc properties on it. This is preferable to using the environment directly as it's strongly typed and will provide some auto-completion and documentation in a developer's IDE. I say some, as you won't get any metadata (and therefore IDE support) for anything nested in the map. That situation will improve when we implement #9945 but it's out of scope for 2.0.

/cc @snicoll

[jgrandja]

@wilkinsona I'm not sure I follow you? Let's go by example. Given that @ConfigurationProperties is allowed on Type and Method only, these are my 2 options:

Option 1

	@ConfigurationProperties(value = "security.oauth2.client.google")
	public ClientRegistrationProperties googleClientConfig() {
		return new ClientRegistrationProperties();
	}

Option 2

@ConfigurationProperties(value = "security.oauth2.client.google")
public class GoogleClientConfig extends ClientRegistrationProperties {
}

Option 2 is redundant as I'm simply extending the existing ClientRegistrationProperties class and provide no extra value here. So this option is not really an option.

Option 1 is an extra configuration step that can be avoided. So for each client I configure in my application.yml or defaults.yml I need to also use @ConfigurationProperties. Why? It's really an extra step that IMO is not needed.

Not to mention, as part of the introspection of client properties in the Environment, I'm checking for the required client-id property and if it's not provided than I don't configure the client. How can I do this validation by-pass with @ConfigurationProperties?

Moreover, we do not want to expose ClientRegistration instance as beans in the context. The only bean that should be in the context is a ClientRegistrationRepository bean that is composed of one or more ClientRegistration's

[snicoll]

That's not your only two options for what you need to bind. Here is a simple example:

@ConfigurationProperties(value = "security.oauth2")
public class OAuth2Properties {

    private final Map<String,ClientRegistrationProperties> client = new HashMap<>();

    public Map<String,ClientRegistrationProperties> getClient() { return this.client; }

}

With this setup, you can bind any client via the map, something like security.oauth2.client.google.xyz where xyz is a property of ClientRegistrationProperties.

Having said that, I'd be in favour of restructing that a bit, something like security.oauth2.client.providers or something. The code above is a bit weird, this is much better IMO:

@ConfigurationProperties(value = "security.oauth2.client")
public class OAuth2ClientProperties {

    private final Map<String,ClientRegistrationProperties> providers = new HashMap<>();

    public Map<String,ClientRegistrationProperties> getProviders() { return this.providers; }

}

[jgrandja]

@snicoll @wilkinsona I tried your suggestion for binding the clients as follows:

@ConfigurationProperties(prefix = "security.oauth2.client")
public class OAuth2ClientsProperties {

	private Map<String, ClientRegistrationProperties> clients = new HashMap<>();

	public Map<String, ClientRegistrationProperties> getClients() {
		return clients;
	}

	public void setClients(Map<String, ClientRegistrationProperties> clients) {
		this.clients = clients;
	}
}

But this did not work since the base property path used for binding would be security.oauth2.client.clients based on this structure but the clients are configured in the Environment on this base path security.oauth2.client.<client-key>, for example, security.oauth2.client.google, security.oauth2.client.github, etc.

To get it to work, the structure is this:

@ConfigurationProperties(prefix = OAuth2ClientsProperties.CLIENT_PROPERTY_PREFIX)
public class OAuth2ClientsProperties extends HashMap<String, ClientRegistrationProperties> {

	public static final String CLIENT_PROPERTY_PREFIX = "security.oauth2.client";

}

I will admit that I don't like the fact that I have to extend HashMap even though the client properties is a Map. As you are reviewing things, maybe you can suggest an alternative option?

[snicoll]

@jgrandja I gave you a code sample for that, see OAuth2Properties (there is no need for a setter if the map is initialized).

As far as I know we decide what the path will be. If we decide that the path is security.oauth2.client.providers so bet it. In that case the second example I gave would work.

Sorry, but I don't understand what the problem is. I think we'd like this to move to spring.security.oauth2.* anyway.

[jgrandja]

@snicoll Sorry for the confusion but honestly I'm confused as well. In your initial comment, you did suggest OAuth2Properties but shortly after that you said...

Having said that, I'd be in favour of restructing that a bit, something like security.oauth2.client.providers or something. The code above is a bit weird...

In your last comment, you said

I think we'd like this to move to spring.security.oauth2.* anyway.

Can you please confirm if you want spring at the start of the prefix? This is new and threw me for a loop.

Again, I apologize for the confusion but I want to make sure I get things right so we're not going back and forth here. I don't want to waste your time or mine.

I just pushed an update that includes this:

@ConfigurationProperties(prefix = "security.oauth2")
public class OAuth2Properties {

	private Map<String, ClientRegistrationProperties> clients = new LinkedHashMap<>();

	public Map<String, ClientRegistrationProperties> getClients() {
		return this.clients;
	}

	public void setClients(Map<String, ClientRegistrationProperties> clients) {
		this.clients = clients;
	}
}

If this is not what you expect than please let me where to correct and I'll get it done quick.
Thanks.

[snicoll]

I think the back and forth is because the only thing I am doing here is answering you on the technicality. I gave you options that work and answers the technical concern that you've raised.

Can you please confirm if you want spring at the start of the prefix? This is new and threw me for a loop.

I don't know and it doesn't matter. If/Once the PR is in a good shape to be merged, we can rename the prefix ourselves. It's really not a big deal.

[jgrandja]

@snicoll I'm really hoping we can get this into M4 this Thursday. Please let me know what I need to do on my end to make this happen. Thanks.

[wilkinsona]

It's unusual for an auto-configuration class to use the Environment directly. Ideally, this should be replaced with @ConfigurationProperties classes that I proposed above.

[jgrandja]

See comment

[wilkinsona]

Why is this ordered? It looks like it's to ensure that it is processed before OAuth2LoginConfiguration. I am not sure that it will be guaranteed to have that effect when they're inner-classes rather than classes registered directly with the context. Our preferred approach for this is to use @Import where the order of the referenced classes is guaranteed to be honoured.

[jgrandja]

I've split-up OAuth2LoginConfiguration into ClientRegistrationRepositoryAutoConfiguration and OAuth2LoginAutoConfiguration and leveraged @AutoConfigureBefore to ensure the correct ordering. Will push those changes shortly.

[wilkinsona]

I don't think this should be annotated with @EnableWebSecurity. IIRC, we've never needed it in multiple places in other Spring Boot security configuration classes. I think we just need to make sure that the auto-configuration class that enables web security is doing its job at the right time relative to these configuration classes.

[jgrandja]

Agreed. Will replace @EnableWebSecurity with @Configuration

[wilkinsona]

.forEach() rather than .stream().forEach() would work here wouldn't it?

[jgrandja]

Yes it would. Will update.

[wilkinsona]

If it makes sense to do so, it'd be nice to split this test up a bit

[jgrandja]

The tests will be much smaller now given that I split up the auto-config's into 2 classes.

[wilkinsona]

These tests would benefit from using our new WebApplicationContextRunner. We could take care of that as part of merging things. It may solve the @EnableWebSecurity-related problem as, among other things, it takes care of ordering auto-configuration classes correctly.

[jgrandja]

I changed the tests to ensure AnnotationConfigWebApplicationContext.register() is called in the correct order for the auto-config classes. All tests are passing now. Let me see if I can integrate WebApplicationContextRunner instead.

[jgrandja]

I tried integrating WebApplicationContextRunner, however, I couldn't get it working because ClientPropertyDefaultsEnvironmentPostProcessor did not postProcess and it needs to.

Looking at the API, it wasn't clear on how to register ClientPropertyDefaultsEnvironmentPostProcessor with the runner.

[wilkinsona]

Sounds like another reason not to use the environment post processor approach to me. Another is that you can't switch it off, i.e. you'll always get the property source in the environment, whether or not you're using Spring Security.

I don't think we'll be comfortable merging this until the use of an EPP has been replaced with something else.

[jgrandja]

The property source will only be added if there are client properties available in the Environment if configured by the user. Otherwise, it won't be added.

[wilkinsona]

Thanks for the correction. I'd missed that. Unfortunately, it doesn't change my opinion about using an EnvironmentPostProcessor here. It would be the only auto-configuration in Boot that uses one. We're consistent in using @ConfigurationProperties for this pretty much everywhere else in Boot and I'd like that to continue. Can you please explore the Map<String, Foo> approach that I described earlier?

[jgrandja]

See comment

[jgrandja]

@wilkinsona I pushed a couple commits based on your feedback.

[jgrandja]

@wilkinsona Good question ...regarding dropping the current OAuth-related auto-configuration in Boot 2.0.

FYI, our plan is to bring Resource Server functionality into 5.1 which I'm thinking will be released around March 2018. At this point we'll have full client and resource server support. We will tackle the Authorization Server next after that but this will be a much larger undertaking that may even take up to 1 year.

I would ask this question... Would it be sufficient to have auto-configuration for client and resource server functionality only and not authorization server? I personally would answer yes.

Auto-configuration for Authorization Server IMO is risky and probably should be avoided.

If we were to go with this reasoning than maybe we can drop the current OAuth auto-config classes. By the time we release 5.1 we'll have auto-config for resource server as well.

And given that Boot follows Semantic Versioning, you wouldn't be able to drop it until 3.0 which will be much later than when Spring Security 5.1 is released.

@rwinch What are your thoughts on this?

[rwinch]

@wilkinsona @jgrandja I think it would be ideal if we could drop the auto configuration of the old OAuth related code. However, I think that may be problematic in regards to our timeline on providing a replacement and the needs of UAA users. Perhaps a better option would be to move the old OAuth auto configuration from Boot to another location (i.e. spring-security-oauth)

[wilkinsona]

Thanks, both. That's an interesting suggest, @rwinch. I've opened #10255 to have a discussion focussed on this. Could you comment over there too please?

[jgrandja]

@wilkinsona The EnvironmentPostProcessor has been removed. I believe I have addressed all outstanding items. Good to go for another full review.

[jgrandja]

@philwebb I added a new commit that avoids adding the client defaults to the Environment. This gives us another option at least.

[snicoll]

I can see @philwebb assigned himself but I had that review pending that I left off to finalize M4.

Here are some ideas. My biggest complain is about the configuration structure (easy to fix) and the default properties file.

[snicoll]

We're moving the security configuration to spring.security.* in 2.0. I don't know the scope of the new OAuth2 support but if we have a client support and a resource server support it would be worth splitting things up a bit more.

This was discussed in prior reviews but spring.security.oauth2.client.providers sounds a better name to me. Right now we have no way to define any other client-related property.

[jgrandja]

I'll change the prefix to spring.security.oauth2.

The scope for 5.0 is strictly Client support and soon after around Mar-Apr 2018 we will bring Resource Server support in 5.1. The idea was to initially house the client properties in OAuth2Properties and then leverage the same for Resource Server properties. Hence the placement of it in the package org.springframework.boot.autoconfigure.security.oauth2 and the prefix of spring.security.oauth2.

I don't feel spring.security.oauth2.client.providers makes sense given that the defined Roles, as per spec: Resource Server, Client, Authorization Server and Resource Owner. The providers define the Resource Server and Authorization Server whereas the properties we are defining here represent the Client role and the attributes of a Client Registration. Therefore, providers may confuse the user since these are client-specific properties.

[snicoll]

providers is just a proposal, that's not important. What is important is that Map is a catch all and we may want to define other client-specific properties in the near future so we can't "steal" the client namespace with the map.

In other words, it should be spring.security.oauth2.client.xyz so that we can reuse the spring.security.oauth2.client namespace for other client-specific settings.

[jgrandja]

I initially had the prefix and therefore the namespace as spring.security.oauth2.client but then it didn't quite read right because given the Map<String, ClientRegistrationProperties> clients the binding path would be spring.security.oauth2.client.clients. The namespace spring.security.oauth2.client represents a singular form whereas Spring Security allows for multiple client configurations.

As far as defining other client properties that are Boot specific, then it might make sense to have Boot's own representation of ClientRegistrationProperties that essentially duplicates the one in Spring Security but at least provides the flexibility to create new properties per client that are Boot specific. What do you think?

[jgrandja]

Was just chatting with @rwinch and yes it does make sense to split up OAuth2Properties to OAuth2ClientProperties and OAuth2ResourceServerProperties (future). This will prevent class loading issues as both the client and resource-server modules are optional.

I'll rename OAuth2Properties to OAuth2ClientProperties for now.

[snicoll]

There is no need for a setter for a Map if the field is initialized

[jgrandja]

Ok will remove the setter.

[snicoll]

IMO, those defaults do not belong in Spring Boot

[jgrandja]

I can easily move spring-security-oauth2-client-defaults.properties into Spring Security. Please confirm if this is what you would prefer.

[snicoll]

I am not sure I like the idea that we bind a 3rd party class directly to the environment. We've done that in the past and regretted it. It's just a raw observation though, maybe that's fine.

[jgrandja]

Let me know if you would like a new class similar to ClientRegistrationProperties in Boot. Either way, we will keep ClientRegistrationProperties in Spring Security as it serves as a convenience class in certain use cases.

[snicoll]

I think the defaults and how they are applied should largely derive from that. I can certainly appreciate you have an infrastructure in the project that we would reuse. I just don't think binding it directly to the environment is a good idea.

[jgrandja]

Can you elaborate on a specific issue you had in the past where you used a 3rd party class to bind in the Environment? I would like to understand this more.

[snicoll]

IMO this is too low-level.

What if we had a Map<String,Foo> (name TBD) and one attribute of this object would be a the ID of an oauth2 client type (google, github, facebook, etc). What the auto-configuration can then do is apply default values based on the client-type. If the client is fully configured, that's fine and we'll use that but all the properties that can be inferred by the client type are then set.

In Spring Boot we could put something in place so that new "id" can be registered if necessary.

[jgrandja]

I'm not sure if I completely follow? Maybe an example to elaborate would help.

[philwebb]

I think what @snicoll is suggesting is an attribute that specifies the "template" for common properties.

We could have an enum (probably in Spring Security) that defines well known templates:

public enum OAuth2ClientTemplate {

    GOOGLE, FACEBOOK, GITHUB

}

In the client property we then have a "template" field:

public class OAuth2ClientProperties {
    private OAuth2ClientTemplate template;
    private String redirectUrl;
    private String scope;
    // etc
}

The user can then specify the template in the properties file. Some examples:

# Standard GitHub with some keys
spring.security.oauth2.client.mygithub.template=GITHUB
spring.security.oauth2.client.mygithub.token=ZYX

# GitHub but override the scope with some keys
spring.security.oauth2.client.foogithub.template=GITHUB
spring.security.oauth2.client.foogithub.token=ZYX
spring.security.oauth2.client.foogithub.scope=email

# Something completely custom
spring.security.oauth2.client.bar.redirect-uri=...
spring.security.oauth2.client.bar.token=ZYX
spring.security.oauth2.client.barscope=baz

[jgrandja]

I see. Let me give this one some thought

[snicoll]

That's what I meant (with a few things we may need to adapt but we shouldn't worry about that now). The main idea is that this mechanism of providing default values should be in Spring Security and therefore the API that allows you to configure your client should be there as well.

That's also a reason why we shouldn't bind Spring Security's API directly on the environment. We need to be in control on what options we expose to the user.

[philwebb]

I've marked this one as on-hold because we really need to consider #10255 at the same time.

[mbhave]

@jgrandja shouldn't this be @AutoConfigureAfter(ClientRegistrationRepositoryAutoConfiguration.class) because that's the one that provides the ClientRegistrationRepository bean? Also, I think for the @ConditionalOnMissingBean(WebSecurityConfigurerAdapter.class) to match, it would need to be auto-configured before SecurityAutoConfiguration so that the default WebSecurityConfigurerAdapter from SpringBootWebSecurityConfiguration does not kick in.

[jgrandja]

Thanks for the heads up @mbhave.
Yeah I noticed that today as I was re-working the PR.

I changed it to:

@AutoConfigureBefore(SecurityAutoConfiguration.class)
@AutoConfigureAfter(ClientRegistrationRepositoryAutoConfiguration.class)

I'm going to push this sometime tomorrow after I finish up a couple more things.
Thanks!

[jgrandja]

@snicoll @philwebb I've pushed the updates based on all your feedback.

• Renamed OAuth2Properties -> OAuth2ClientProperties and set prefix to spring.security.oauth2.client
• Added OAuth2ClientProperties.ClientRegistration - no longer binding to Spring Security's ClientRegistrationProperties
• Great idea re: OAuth2ClientProperties.ClientRegistration.clientType - this worked out quite nicely
• Removed defaults properties resource and it now lives in Spring Security, see OAuth2ClientPropertiesUtil

[mbhave]

We simplified this a little more as part of #10497. Closing this PR in favor of that.