From f0b19f30d1cf0ddaf12a0414c7a845c1b55eeb0e Mon Sep 17 00:00:00 2001 From: Joe Grandja Date: Tue, 16 Nov 2021 03:52:54 -0500 Subject: [PATCH] Deprecate PasswordEncoder in JdbcRegisteredClientRepository Closes gh-496 --- .../JdbcRegisteredClientRepository.java | 18 ++----- .../client/RegisteredClientRepository.java | 3 ++ .../JdbcRegisteredClientRepositoryTests.java | 52 ------------------- .../config/AuthorizationServerConfig.java | 2 +- 4 files changed, 9 insertions(+), 66 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 02100428..7a475189 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 @@ -36,7 +36,6 @@ import org.springframework.jdbc.core.JdbcOperations; import org.springframework.jdbc.core.PreparedStatementSetter; import org.springframework.jdbc.core.RowMapper; import org.springframework.jdbc.core.SqlParameterValue; -import org.springframework.security.crypto.factory.PasswordEncoderFactories; import org.springframework.security.crypto.password.PasswordEncoder; import org.springframework.security.jackson2.SecurityJackson2Modules; import org.springframework.security.oauth2.core.AuthorizationGrantType; @@ -294,7 +293,6 @@ public class JdbcRegisteredClientRepository implements RegisteredClientRepositor */ public static class RegisteredClientParametersMapper implements Function> { private ObjectMapper objectMapper = new ObjectMapper(); - private PasswordEncoder passwordEncoder = PasswordEncoderFactories.createDelegatingPasswordEncoder(); public RegisteredClientParametersMapper() { ClassLoader classLoader = JdbcRegisteredClientRepository.class.getClassLoader(); @@ -323,7 +321,7 @@ public class JdbcRegisteredClientRepository implements RegisteredClientRepositor new SqlParameterValue(Types.VARCHAR, registeredClient.getId()), new SqlParameterValue(Types.VARCHAR, registeredClient.getClientId()), new SqlParameterValue(Types.TIMESTAMP, clientIdIssuedAt), - new SqlParameterValue(Types.VARCHAR, encode(registeredClient.getClientSecret())), + new SqlParameterValue(Types.VARCHAR, registeredClient.getClientSecret()), new SqlParameterValue(Types.TIMESTAMP, clientSecretExpiresAt), new SqlParameterValue(Types.VARCHAR, registeredClient.getClientName()), new SqlParameterValue(Types.VARCHAR, StringUtils.collectionToCommaDelimitedString(clientAuthenticationMethods)), @@ -339,10 +337,11 @@ public class JdbcRegisteredClientRepository implements RegisteredClientRepositor this.objectMapper = objectMapper; } - + /** + * @deprecated See javadoc {@link RegisteredClientRepository#save(RegisteredClient)} + */ + @Deprecated public final void setPasswordEncoder(PasswordEncoder passwordEncoder) { - Assert.notNull(passwordEncoder, "passwordEncoder cannot be null"); - this.passwordEncoder = passwordEncoder; } protected final ObjectMapper getObjectMapper() { @@ -357,13 +356,6 @@ public class JdbcRegisteredClientRepository implements RegisteredClientRepositor } } - private String encode(String value) { - if (value != null) { - return this.passwordEncoder.encode(value); - } - return null; - } - } } diff --git a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/client/RegisteredClientRepository.java b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/client/RegisteredClientRepository.java index f1905445..00a00a5f 100644 --- a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/client/RegisteredClientRepository.java +++ b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/client/RegisteredClientRepository.java @@ -31,6 +31,9 @@ public interface RegisteredClientRepository { /** * Saves the registered client. * + *

+ * IMPORTANT: Sensitive information should be encoded externally from the implementation, e.g. {@link RegisteredClient#getClientSecret()} + * * @param registeredClient the {@link RegisteredClient} */ void save(RegisteredClient registeredClient); 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 38893a7f..a635c2db 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,9 +40,6 @@ 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; import org.springframework.security.oauth2.core.AuthorizationGrantType; import org.springframework.security.oauth2.core.ClientAuthenticationMethod; @@ -57,11 +54,8 @@ import static org.assertj.core.api.Assertions.assertThat; 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; /** * Tests for {@link JdbcRegisteredClientRepository}. @@ -77,27 +71,12 @@ public class JdbcRegisteredClientRepositoryTests { private EmbeddedDatabase db; private JdbcOperations jdbcOperations; private JdbcRegisteredClientRepository registeredClientRepository; - private PasswordEncoder passwordEncoder; @Before public void setUp() { this.db = createDb(OAUTH2_REGISTERED_CLIENT_SCHEMA_SQL_RESOURCE); this.jdbcOperations = new JdbcTemplate(this.db); this.registeredClientRepository = new JdbcRegisteredClientRepository(this.jdbcOperations); - 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); - } - }); - RegisteredClientParametersMapper registeredClientParametersMapper = new RegisteredClientParametersMapper(); - registeredClientParametersMapper.setPasswordEncoder(this.passwordEncoder); - this.registeredClientRepository.setRegisteredClientParametersMapper(registeredClientParametersMapper); } @After @@ -167,7 +146,6 @@ public class JdbcRegisteredClientRepositoryTests { this.registeredClientRepository.save(expectedRegisteredClient); RegisteredClient registeredClient = this.registeredClientRepository.findById(expectedRegisteredClient.getId()); assertThat(registeredClient).isEqualTo(expectedRegisteredClient); - verify(this.passwordEncoder).encode(anyString()); } @Test @@ -177,7 +155,6 @@ public class JdbcRegisteredClientRepositoryTests { this.registeredClientRepository.save(expectedRegisteredClient); RegisteredClient registeredClient = this.registeredClientRepository.findById(expectedRegisteredClient.getId()); assertThat(registeredClient).isEqualTo(expectedRegisteredClient); - verifyNoInteractions(this.passwordEncoder); } @Test @@ -185,7 +162,6 @@ public class JdbcRegisteredClientRepositoryTests { RowMapper registeredClientRowMapper = spy(new RegisteredClientRowMapper()); this.registeredClientRepository.setRegisteredClientRowMapper(registeredClientRowMapper); RegisteredClientParametersMapper clientParametersMapper = new RegisteredClientParametersMapper(); - clientParametersMapper.setPasswordEncoder(this.passwordEncoder); Function> registeredClientParametersMapper = spy(clientParametersMapper); this.registeredClientRepository.setRegisteredClientParametersMapper(registeredClientParametersMapper); @@ -195,30 +171,6 @@ public class JdbcRegisteredClientRepositoryTests { assertThat(result).isEqualTo(registeredClient); verify(registeredClientRowMapper).mapRow(any(), anyInt()); verify(registeredClientParametersMapper).apply(any()); - 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 @@ -270,17 +222,13 @@ public class JdbcRegisteredClientRepositoryTests { @Test public void tableDefinitionWhenCustomThenAbleToOverride() { EmbeddedDatabase db = createDb(OAUTH2_CUSTOM_REGISTERED_CLIENT_SCHEMA_SQL_RESOURCE); - RegisteredClientParametersMapper registeredClientParametersMapper = new RegisteredClientParametersMapper(); - registeredClientParametersMapper.setPasswordEncoder(this.passwordEncoder); CustomJdbcRegisteredClientRepository registeredClientRepository = new CustomJdbcRegisteredClientRepository(new JdbcTemplate(db)); - registeredClientRepository.setRegisteredClientParametersMapper(registeredClientParametersMapper); RegisteredClient registeredClient = TestRegisteredClients.registeredClient().build(); registeredClientRepository.save(registeredClient); RegisteredClient foundRegisteredClient1 = registeredClientRepository.findById(registeredClient.getId()); assertThat(foundRegisteredClient1).isEqualTo(registeredClient); RegisteredClient foundRegisteredClient2 = registeredClientRepository.findByClientId(registeredClient.getClientId()); assertThat(foundRegisteredClient2).isEqualTo(registeredClient); - verify(this.passwordEncoder).encode(anyString()); db.shutdown(); } diff --git a/samples/default-authorizationserver/src/main/java/sample/config/AuthorizationServerConfig.java b/samples/default-authorizationserver/src/main/java/sample/config/AuthorizationServerConfig.java index ee796c54..1badedeb 100644 --- a/samples/default-authorizationserver/src/main/java/sample/config/AuthorizationServerConfig.java +++ b/samples/default-authorizationserver/src/main/java/sample/config/AuthorizationServerConfig.java @@ -66,7 +66,7 @@ public class AuthorizationServerConfig { public RegisteredClientRepository registeredClientRepository(JdbcTemplate jdbcTemplate) { RegisteredClient registeredClient = RegisteredClient.withId(UUID.randomUUID().toString()) .clientId("messaging-client") - .clientSecret("secret") + .clientSecret("{noop}secret") .clientAuthenticationMethod(ClientAuthenticationMethod.CLIENT_SECRET_BASIC) .authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE) .authorizationGrantType(AuthorizationGrantType.REFRESH_TOKEN)