Skip to content
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.

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

Upgrade client secret when appropriately available #1105

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Upgrade client secret when appropriately available
  • Loading branch information
shanman190 committed Feb 27, 2023
commit 867f918a8832afb82596c85618be697b386068d5
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,13 @@ public Authentication authenticate(Authentication authentication) throws Authent
throwInvalidClient("client_secret_expires_at");
}

if (this.passwordEncoder.upgradeEncoding(registeredClient.getClientSecret())) {
registeredClient = RegisteredClient.from(registeredClient)
.clientSecret(this.passwordEncoder.encode(clientSecret))
.build();
registeredClientRepository.save(registeredClient);
}

if (this.logger.isTraceEnabled()) {
this.logger.trace("Validated client authentication parameters");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,14 @@ public InMemoryRegisteredClientRepository(List<RegisteredClient> registrations)
@Override
public void save(RegisteredClient registeredClient) {
Assert.notNull(registeredClient, "registeredClient cannot be null");
assertUniqueIdentifiers(registeredClient, this.idRegistrationMap);
this.idRegistrationMap.put(registeredClient.getId(), registeredClient);
this.clientIdRegistrationMap.put(registeredClient.getClientId(), registeredClient);
if (this.idRegistrationMap.containsKey(registeredClient.getId())) {
this.idRegistrationMap.put(registeredClient.getId(), registeredClient);
this.clientIdRegistrationMap.put(registeredClient.getClientId(), registeredClient);
} else {
assertUniqueIdentifiers(registeredClient, this.idRegistrationMap);
this.idRegistrationMap.put(registeredClient.getId(), registeredClient);
this.clientIdRegistrationMap.put(registeredClient.getClientId(), registeredClient);
}
}

@Nullable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,9 @@ public class JdbcRegisteredClientRepository implements RegisteredClientRepositor

// @formatter:off
private static final String UPDATE_REGISTERED_CLIENT_SQL = "UPDATE " + TABLE_NAME
+ " SET client_name = ?, client_authentication_methods = ?, authorization_grant_types = ?,"
+ " redirect_uris = ?, post_logout_redirect_uris = ?, scopes = ?, client_settings = ?, token_settings = ?"
+ " SET client_secret = ?, client_secret_expires_at = ?, client_name = ?, client_authentication_methods = ?,"
+ " authorization_grant_types = ?, redirect_uris = ?, post_logout_redirect_uris = ?, scopes = ?,"
+ " client_settings = ?, token_settings = ?"
+ " WHERE " + PK_FILTER;
// @formatter:on

Expand Down Expand Up @@ -136,8 +137,6 @@ private void updateRegisteredClient(RegisteredClient registeredClient) {
SqlParameterValue id = parameters.remove(0);
parameters.remove(0); // remove client_id
parameters.remove(0); // remove client_id_issued_at
parameters.remove(0); // remove client_secret
parameters.remove(0); // remove client_secret_expires_at
parameters.add(id);
PreparedStatementSetter pss = new ArgumentPreparedStatementSetter(parameters.toArray());
this.jdbcOperations.update(UPDATE_REGISTERED_CLIENT_SQL, pss);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,11 @@ public String encode(CharSequence rawPassword) {
public boolean matches(CharSequence rawPassword, String encodedPassword) {
return NoOpPasswordEncoder.getInstance().matches(rawPassword, encodedPassword);
}

@Override
public boolean upgradeEncoding(String encodedPassword) {
return true;
}
});
this.authenticationProvider.setPasswordEncoder(this.passwordEncoder);
}
Expand Down Expand Up @@ -222,6 +227,28 @@ public void authenticateWhenValidCredentialsThenAuthenticated() {
assertThat(authenticationResult.getRegisteredClient()).isEqualTo(registeredClient);
}

@Test
public void authenticateWhenValidCredentialsAndNonExpiredThenPasswordUpgraded() {
RegisteredClient registeredClient = TestRegisteredClients.registeredClient().build();
when(this.registeredClientRepository.findByClientId(eq(registeredClient.getClientId())))
.thenReturn(registeredClient);

OAuth2ClientAuthenticationToken authentication = new OAuth2ClientAuthenticationToken(
registeredClient.getClientId(), ClientAuthenticationMethod.CLIENT_SECRET_BASIC, registeredClient.getClientSecret(), null);
OAuth2ClientAuthenticationToken authenticationResult =
(OAuth2ClientAuthenticationToken) this.authenticationProvider.authenticate(authentication);

verify(this.passwordEncoder).matches(any(), any());
verify(this.passwordEncoder).upgradeEncoding(any());
verify(this.passwordEncoder).encode(any());
verify(this.registeredClientRepository).save(any());
assertThat(authenticationResult.isAuthenticated()).isTrue();
assertThat(registeredClient).isNotSameAs(authenticationResult.getPrincipal());
assertThat(authenticationResult.getPrincipal().toString()).isEqualTo(registeredClient.getClientId());
assertThat(authenticationResult.getCredentials().toString()).isEqualTo(registeredClient.getClientSecret());
assertThat(authenticationResult.getRegisteredClient()).isEqualTo(registeredClient);
}

@Test
public void authenticateWhenAuthorizationCodeGrantAndValidCredentialsThenAuthenticated() {
RegisteredClient registeredClient = TestRegisteredClients.registeredClient().build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,12 +153,12 @@ public void saveWhenNullThenThrowIllegalArgumentException() {
}

@Test
public void saveWhenExistingIdThenThrowIllegalArgumentException() {
public void saveWhenExistingIdThenUpdate() {
RegisteredClient registeredClient = createRegisteredClient(
this.registration.getId(), "client-id-2", "client-secret-2");
assertThatIllegalArgumentException()
.isThrownBy(() -> this.clients.save(registeredClient))
.withMessage("Registered client must be unique. Found duplicate identifier: " + registeredClient.getId());
this.registration.getId(), "client-id", "client-secret-2");
this.clients.save(registeredClient);
RegisteredClient savedClient = this.clients.findByClientId(registeredClient.getClientId());
assertThat(savedClient.getClientSecret()).isEqualTo("client-secret-2");
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.security.crypto.factory.PasswordEncoderFactories;
import org.springframework.security.crypto.password.NoOpPasswordEncoder;
import org.springframework.security.crypto.password.PasswordEncoder;
import org.springframework.security.oauth2.core.AuthorizationGrantType;
Expand Down Expand Up @@ -231,6 +232,27 @@ public void requestWhenTokenRequestPostsClientCredentialsThenTokenResponse() thr
verify(jwtCustomizer).customize(any());
}

@Test
public void requestWhenTokenRequestPostsClientCredentialsThenTokenResponseAndSecretUpgraded() throws Exception {
this.spring.register(AuthorizationServerConfigurationCustomPasswordEncoder.class).autowire();

String clientSecret = "secret-2";
RegisteredClient registeredClient = TestRegisteredClients.registeredClient2().clientSecret("{noop}" + clientSecret).build();
this.registeredClientRepository.save(registeredClient);

this.mvc.perform(post(DEFAULT_TOKEN_ENDPOINT_URI)
.param(OAuth2ParameterNames.GRANT_TYPE, AuthorizationGrantType.CLIENT_CREDENTIALS.getValue())
.param(OAuth2ParameterNames.SCOPE, "scope1 scope2")
.param(OAuth2ParameterNames.CLIENT_ID, registeredClient.getClientId())
.param(OAuth2ParameterNames.CLIENT_SECRET, clientSecret))
.andExpect(status().isOk())
.andExpect(jsonPath("$.access_token").isNotEmpty())
.andExpect(jsonPath("$.scope").value("scope1 scope2"));

verify(jwtCustomizer).customize(any());
assertThat(this.registeredClientRepository.findByClientId(registeredClient.getClientId()).getClientSecret()).startsWith("{bcrypt}");
}

@Test
public void requestWhenTokenEndpointCustomizedThenUsed() throws Exception {
this.spring.register(AuthorizationServerConfigurationCustomTokenEndpoint.class).autowire();
Expand Down Expand Up @@ -429,6 +451,15 @@ public SecurityFilterChain authorizationServerSecurityFilterChain(HttpSecurity h
// @formatter:on
}

@EnableWebSecurity
@Configuration(proxyBeanMethods = false)
static class AuthorizationServerConfigurationCustomPasswordEncoder extends AuthorizationServerConfiguration {
@Override
PasswordEncoder passwordEncoder() {
return PasswordEncoderFactories.createDelegatingPasswordEncoder();
}
}

@EnableWebSecurity
@Configuration(proxyBeanMethods = false)
static class AuthorizationServerConfigurationCustomClientAuthentication extends AuthorizationServerConfiguration {
Expand Down