From 298ebc7c01ddcdde6069c686e7306c77338e9274 Mon Sep 17 00:00:00 2001 From: Ovidiu Popa Date: Thu, 23 Sep 2021 14:59:16 +0300 Subject: [PATCH] Avoid client secret double encoding when updating a registered client This might have to be revisited at a later point, but to check if a value is encoded or not is quite tricky. The decision was to remove client_secret and client_secret_expires_at from the update statement Closes gh-389 --- .../JdbcRegisteredClientRepository.java | 5 ++-- .../JdbcRegisteredClientRepositoryTests.java | 25 +++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/client/JdbcRegisteredClientRepository.java b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/client/JdbcRegisteredClientRepository.java index 1a9fdf28..02100428 100644 --- a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/client/JdbcRegisteredClientRepository.java +++ b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/client/JdbcRegisteredClientRepository.java @@ -95,8 +95,7 @@ public class JdbcRegisteredClientRepository implements RegisteredClientRepositor // @formatter:off private static final String UPDATE_REGISTERED_CLIENT_SQL = "UPDATE " + TABLE_NAME - + " SET client_secret = ?, client_secret_expires_at = ?," - + " client_name = ?, client_authentication_methods = ?, authorization_grant_types = ?," + + " SET client_name = ?, client_authentication_methods = ?, authorization_grant_types = ?," + " redirect_uris = ?, scopes = ?, client_settings = ?, token_settings = ?" + " WHERE " + PK_FILTER; // @formatter:on @@ -134,6 +133,8 @@ public class JdbcRegisteredClientRepository implements RegisteredClientRepositor 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); diff --git a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/client/JdbcRegisteredClientRepositoryTests.java b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/client/JdbcRegisteredClientRepositoryTests.java index 597667ac..38893a7f 100644 --- a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/client/JdbcRegisteredClientRepositoryTests.java +++ b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/client/JdbcRegisteredClientRepositoryTests.java @@ -40,6 +40,7 @@ import org.springframework.jdbc.core.SqlParameterValue; import org.springframework.jdbc.datasource.embedded.EmbeddedDatabase; import org.springframework.jdbc.datasource.embedded.EmbeddedDatabaseBuilder; import org.springframework.jdbc.datasource.embedded.EmbeddedDatabaseType; +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.jackson2.SecurityJackson2Modules; @@ -57,6 +58,7 @@ import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; @@ -196,6 +198,29 @@ public class JdbcRegisteredClientRepositoryTests { verify(this.passwordEncoder).encode(anyString()); } + // gh-389 + @Test + public void saveWhenClientSecretAlreadyEncodedThenNotUpdated() { + PasswordEncoder passwordEncoder = spy(PasswordEncoderFactories.createDelegatingPasswordEncoder()); + RegisteredClientParametersMapper registeredClientParametersMapper = new RegisteredClientParametersMapper(); + registeredClientParametersMapper.setPasswordEncoder(passwordEncoder); + this.registeredClientRepository.setRegisteredClientParametersMapper(registeredClientParametersMapper); + + RegisteredClient originalRegisteredClient = TestRegisteredClients.registeredClient().build(); + this.registeredClientRepository.save(originalRegisteredClient); + verify(passwordEncoder).encode(eq(originalRegisteredClient.getClientSecret())); + + RegisteredClient registeredClient = this.registeredClientRepository.findById(originalRegisteredClient.getId()); + assertThat(registeredClient).isNotNull(); + assertThat(passwordEncoder.matches(originalRegisteredClient.getClientSecret(), registeredClient.getClientSecret())).isTrue(); + + RegisteredClient updatedRegisteredClient = RegisteredClient.from(registeredClient).clientSecret("updated-client-secret").build(); + this.registeredClientRepository.save(updatedRegisteredClient); + updatedRegisteredClient = this.registeredClientRepository.findById(originalRegisteredClient.getId()); + assertThat(updatedRegisteredClient).isNotNull(); + assertThat(passwordEncoder.matches(originalRegisteredClient.getClientSecret(), updatedRegisteredClient.getClientSecret())).isTrue(); + } + @Test public void findByIdWhenIdNullThenThrowIllegalArgumentException() { // @formatter:off