Skip to content

Fix to save after encoding the secret when registering the client #1056

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fix to save after encoding the secret when registering the client #1056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion ...orization/config/annotation/web/configurers/OidcClientRegistrationEndpointConfigurer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2020-2022 the original author or authors.
* Copyright 2020-2023 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -26,6 +26,7 @@
import org.springframework.security.authentication.AuthenticationProvider;
import org.springframework.security.config.annotation.ObjectPostProcessor;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update copyright year to 2023

import org.springframework.security.config.annotation.web.builders.HttpSecurity;
import org.springframework.security.crypto.password.PasswordEncoder;
import org.springframework.security.oauth2.core.OAuth2AuthenticationException;
import org.springframework.security.oauth2.core.OAuth2Error;
import org.springframework.security.oauth2.server.authorization.oidc.OidcClientRegistration;
Expand Down Expand Up @@ -221,6 +222,10 @@ private static List<AuthenticationProvider> createDefaultAuthenticationProviders
OAuth2ConfigurerUtils.getRegisteredClientRepository(httpSecurity),
OAuth2ConfigurerUtils.getAuthorizationService(httpSecurity),
OAuth2ConfigurerUtils.getTokenGenerator(httpSecurity));
PasswordEncoder passwordEncoder = OAuth2ConfigurerUtils.getOptionalBean(httpSecurity, PasswordEncoder.class);
if (passwordEncoder != null) {
oidcClientRegistrationAuthenticationProvider.setPasswordEncoder(passwordEncoder);
}
authenticationProviders.add(oidcClientRegistrationAuthenticationProvider);

OidcClientConfigurationAuthenticationProvider oidcClientConfigurationAuthenticationProvider =
Expand Down
38 changes: 36 additions & 2 deletions ...erver/authorization/oidc/authentication/OidcClientRegistrationAuthenticationProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2020-2022 the original author or authors.
* Copyright 2020-2023 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -34,8 +34,10 @@
import org.springframework.security.authentication.AuthenticationProvider;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.AuthenticationException;
import org.springframework.security.crypto.factory.PasswordEncoderFactories;
import org.springframework.security.crypto.keygen.Base64StringKeyGenerator;
import org.springframework.security.crypto.keygen.StringKeyGenerator;
import org.springframework.security.crypto.password.PasswordEncoder;
import org.springframework.security.oauth2.core.AuthorizationGrantType;
import org.springframework.security.oauth2.core.ClaimAccessor;
import org.springframework.security.oauth2.core.ClientAuthenticationMethod;
Expand Down Expand Up @@ -79,6 +81,7 @@
* @see OAuth2TokenGenerator
* @see OidcClientRegistrationAuthenticationToken
* @see OidcClientConfigurationAuthenticationProvider
* @see PasswordEncoder
* @see <a href="https://openid.net/specs/openid-connect-registration-1_0.html#ClientRegistration">3. Client Registration Endpoint</a>
*/
public final class OidcClientRegistrationAuthenticationProvider implements AuthenticationProvider {
Expand All @@ -91,6 +94,8 @@ public final class OidcClientRegistrationAuthenticationProvider implements Authe
private final Converter<RegisteredClient, OidcClientRegistration> clientRegistrationConverter;
private Converter<OidcClientRegistration, RegisteredClient> registeredClientConverter;

private PasswordEncoder passwordEncoder;

/**
* Constructs an {@code OidcClientRegistrationAuthenticationProvider} using the provided parameters.
*
Expand All @@ -109,6 +114,21 @@ public OidcClientRegistrationAuthenticationProvider(RegisteredClientRepository r
this.tokenGenerator = tokenGenerator;
this.clientRegistrationConverter = new RegisteredClientOidcClientRegistrationConverter();
this.registeredClientConverter = new OidcClientRegistrationRegisteredClientConverter();
this.passwordEncoder = PasswordEncoderFactories.createDelegatingPasswordEncoder();
}

/**
* Sets the {@link PasswordEncoder} used to encode the clientSecret
* the {@link RegisteredClient#getClientSecret() client secret}.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the javadoc as the passwordencoder is not used to validate the clientSecret but instead used to encode the clientSecret.
Also, please add @since 1.1.0

* If not set, the client secret will be encoded using
* {@link PasswordEncoderFactories#createDelegatingPasswordEncoder()}.
*
* @param passwordEncoder the {@link PasswordEncoder} used to encode the clientSecret
* @since 1.1.0
*/
public void setPasswordEncoder(PasswordEncoder passwordEncoder) {
Assert.notNull(passwordEncoder, "passwordEncoder cannot be null");
this.passwordEncoder = passwordEncoder;
}

@Override
Expand Down Expand Up @@ -183,7 +203,21 @@ private OidcClientRegistrationAuthenticationToken registerClient(OidcClientRegis
}

RegisteredClient registeredClient = this.registeredClientConverter.convert(clientRegistrationAuthentication.getClientRegistration());
this.registeredClientRepository.save(registeredClient);

// When secret exists, copy RegisteredClient and encode only secret
String rawClientSecret = registeredClient.getClientSecret();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extension point for customizing client metadata before it's saved to RegisteredClientRepository is OidcClientRegistrationAuthenticationProvider.setRegisteredClientConverter(). It's not currently documented in the reference manual but there is an open issue gh-1044 that provides sample code on how to configure.

String clientSecret = null;
RegisteredClient saveRegisteredClient = null;
if (rawClientSecret != null) {
clientSecret = passwordEncoder.encode(rawClientSecret);
saveRegisteredClient = RegisteredClient.from(registeredClient)
.clientSecret(clientSecret)
.build();
} else {
saveRegisteredClient = registeredClient;
}

this.registeredClientRepository.save(saveRegisteredClient);

if (this.logger.isTraceEnabled()) {
this.logger.trace("Saved registered client");
Expand Down
37 changes: 32 additions & 5 deletions ...2/server/authorization/config/annotation/web/configurers/OidcClientRegistrationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2020-2022 the original author or authors.
* Copyright 2020-2023 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -57,7 +57,7 @@
import org.springframework.security.config.annotation.web.builders.HttpSecurity;
import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity;
import org.springframework.security.config.annotation.web.configurers.oauth2.server.resource.OAuth2ResourceServerConfigurer;
import org.springframework.security.crypto.password.NoOpPasswordEncoder;
import org.springframework.security.crypto.factory.PasswordEncoderFactories;
import org.springframework.security.crypto.password.PasswordEncoder;
import org.springframework.security.oauth2.core.AuthorizationGrantType;
import org.springframework.security.oauth2.core.ClientAuthenticationMethod;
Expand Down Expand Up @@ -112,6 +112,7 @@
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.when;
import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.httpBasic;
import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.jwt;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
Expand Down Expand Up @@ -290,7 +291,6 @@ public void requestWhenClientConfigurationRequestAuthorizedThenClientRegistratio

assertThat(clientConfigurationResponse.getClientId()).isEqualTo(clientRegistrationResponse.getClientId());
assertThat(clientConfigurationResponse.getClientIdIssuedAt()).isEqualTo(clientRegistrationResponse.getClientIdIssuedAt());
assertThat(clientConfigurationResponse.getClientSecret()).isEqualTo(clientRegistrationResponse.getClientSecret());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ClientConfigurationResponse.getClientSecret() returns plain Secret, but ClientRegitionrationRespone. getClientSecret() returns the encoded secret, so do not assert.

assertThat(clientConfigurationResponse.getClientSecretExpiresAt()).isEqualTo(clientRegistrationResponse.getClientSecretExpiresAt());
assertThat(clientConfigurationResponse.getClientName()).isEqualTo(clientRegistrationResponse.getClientName());
assertThat(clientConfigurationResponse.getRedirectUris())
Expand Down Expand Up @@ -358,6 +358,34 @@ public void requestWhenClientRegistrationEndpointCustomizedThenUsed() throws Exc
verifyNoInteractions(authenticationFailureHandler);
}

// gh-1056
@Test
public void requestWhenClientRegistersWithSecretThenClientAuthenticationSuccess() throws Exception {
this.spring.register(AuthorizationServerConfiguration.class).autowire();

// @formatter:off
OidcClientRegistration clientRegistration = OidcClientRegistration.builder()
.clientName("client-name")
.redirectUri("https://client.example.com")
.grantType(AuthorizationGrantType.AUTHORIZATION_CODE.getValue())
.grantType(AuthorizationGrantType.CLIENT_CREDENTIALS.getValue())
.scope("scope1")
.scope("scope2")
.build();
// @formatter:on

OidcClientRegistration clientRegistrationResponse = registerClient(clientRegistration);

MvcResult mvcResult = this.mvc.perform(post(DEFAULT_TOKEN_ENDPOINT_URI)
.param(OAuth2ParameterNames.GRANT_TYPE, AuthorizationGrantType.CLIENT_CREDENTIALS.getValue())
.param(OAuth2ParameterNames.SCOPE, "scope1")
.with(httpBasic(clientRegistrationResponse.getClientId(), clientRegistrationResponse.getClientSecret())))
.andExpect(status().isOk())
.andExpect(jsonPath("$.access_token").isNotEmpty())
.andExpect(jsonPath("$.scope").value("scope1"))
.andReturn();
}

@Test
public void requestWhenClientRegistrationEndpointCustomizedWithAuthenticationFailureHandlerThenUsed() throws Exception {
this.spring.register(CustomClientRegistrationConfiguration.class).autowire();
Expand Down Expand Up @@ -566,9 +594,8 @@ AuthorizationServerSettings authorizationServerSettings() {

@Bean
PasswordEncoder passwordEncoder() {
return NoOpPasswordEncoder.getInstance();
return PasswordEncoderFactories.createDelegatingPasswordEncoder();
}

}

}
26 changes: 25 additions & 1 deletion .../authorization/oidc/authentication/OidcClientRegistrationAuthenticationProviderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2020-2022 the original author or authors.
* Copyright 2020-2023 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -30,6 +30,8 @@
import org.springframework.security.authentication.TestingAuthenticationToken;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.authority.AuthorityUtils;
import org.springframework.security.crypto.password.NoOpPasswordEncoder;
import org.springframework.security.crypto.password.PasswordEncoder;
import org.springframework.security.oauth2.core.AuthorizationGrantType;
import org.springframework.security.oauth2.core.ClientAuthenticationMethod;
import org.springframework.security.oauth2.core.OAuth2AccessToken;
Expand Down Expand Up @@ -90,6 +92,8 @@ public class OidcClientRegistrationAuthenticationProviderTests {
private AuthorizationServerSettings authorizationServerSettings;
private OidcClientRegistrationAuthenticationProvider authenticationProvider;

private PasswordEncoder passwordEncoder;

@BeforeEach
public void setUp() {
this.registeredClientRepository = mock(RegisteredClientRepository.class);
Expand All @@ -106,6 +110,18 @@ public Jwt generate(OAuth2TokenContext context) {
AuthorizationServerContextHolder.setContext(new TestAuthorizationServerContext(this.authorizationServerSettings, null));
this.authenticationProvider = new OidcClientRegistrationAuthenticationProvider(
this.registeredClientRepository, this.authorizationService, this.tokenGenerator);
this.passwordEncoder = spy(new PasswordEncoder() {
@Override
public String encode(CharSequence rawPassword) {
return NoOpPasswordEncoder.getInstance().encode(rawPassword);
}

@Override
public boolean matches(CharSequence rawPassword, String encodedPassword) {
return NoOpPasswordEncoder.getInstance().matches(rawPassword, encodedPassword);
}
});
this.authenticationProvider.setPasswordEncoder(this.passwordEncoder);
}

@AfterEach
Expand Down Expand Up @@ -141,6 +157,13 @@ public void setRegisteredClientConverterWhenNullThenThrowIllegalArgumentExceptio
.withMessage("registeredClientConverter cannot be null");
}

@Test
public void setPasswordEncoderWhenNullThenThrowIllegalArgumentException() {
assertThatThrownBy(() -> this.authenticationProvider.setPasswordEncoder(null))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("passwordEncoder cannot be null");
}

@Test
public void supportsWhenTypeOidcClientRegistrationAuthenticationTokenThenReturnTrue() {
assertThat(this.authenticationProvider.supports(OidcClientRegistrationAuthenticationToken.class)).isTrue();
Expand Down Expand Up @@ -472,6 +495,7 @@ public void authenticateWhenTokenEndpointAuthenticationSigningAlgorithmNotProvid
assertThat(authenticationResult.getClientRegistration().getTokenEndpointAuthenticationSigningAlgorithm())
.isEqualTo(MacAlgorithm.HS256.getName());
assertThat(authenticationResult.getClientRegistration().getClientSecret()).isNotNull();
verify(this.passwordEncoder).encode(any());

// @formatter:off
builder
Expand Down