From ad108f519a24b66a1bb5cbc599e722ed5f8568eb Mon Sep 17 00:00:00 2001 From: Joe Grandja Date: Thu, 8 Jul 2021 08:51:32 -0400 Subject: [PATCH] Polish JdbcOAuth2AuthorizationConsentService Issue gh-314 --- ...JdbcOAuth2AuthorizationConsentService.java | 14 ++-- ...Auth2AuthorizationConsentServiceTests.java | 64 +++++++++++-------- 2 files changed, 43 insertions(+), 35 deletions(-) diff --git a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/JdbcOAuth2AuthorizationConsentService.java b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/JdbcOAuth2AuthorizationConsentService.java index ef6a5af1..e489fc0a 100644 --- a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/JdbcOAuth2AuthorizationConsentService.java +++ b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/JdbcOAuth2AuthorizationConsentService.java @@ -40,7 +40,6 @@ import org.springframework.util.StringUtils; /** * A JDBC implementation of an {@link OAuth2AuthorizationConsentService} that uses a - *

* {@link JdbcOperations} for {@link OAuth2AuthorizationConsent} persistence. * *

@@ -50,11 +49,11 @@ import org.springframework.util.StringUtils; * therefore MUST be defined in the database schema. * * @author Ovidiu Popa + * @since 0.1.2 * @see OAuth2AuthorizationConsentService * @see OAuth2AuthorizationConsent * @see JdbcOperations * @see RowMapper - * @since 0.1.2 */ public class JdbcOAuth2AuthorizationConsentService implements OAuth2AuthorizationConsentService { @@ -109,10 +108,8 @@ public class JdbcOAuth2AuthorizationConsentService implements OAuth2Authorizatio @Override public void save(OAuth2AuthorizationConsent authorizationConsent) { Assert.notNull(authorizationConsent, "authorizationConsent cannot be null"); - - OAuth2AuthorizationConsent existingAuthorizationConsent = - findById(authorizationConsent.getRegisteredClientId(), authorizationConsent.getPrincipalName()); - + OAuth2AuthorizationConsent existingAuthorizationConsent = findById( + authorizationConsent.getRegisteredClientId(), authorizationConsent.getPrincipalName()); if (existingAuthorizationConsent == null) { insertAuthorizationConsent(authorizationConsent); } else { @@ -205,7 +202,6 @@ public class JdbcOAuth2AuthorizationConsentService implements OAuth2Authorizatio * {@code ResultSet} to {@link OAuth2AuthorizationConsent}. */ public static class OAuth2AuthorizationConsentRowMapper implements RowMapper { - private final RegisteredClientRepository registeredClientRepository; public OAuth2AuthorizationConsentRowMapper(RegisteredClientRepository registeredClientRepository) { @@ -216,9 +212,7 @@ public class JdbcOAuth2AuthorizationConsentService implements OAuth2Authorizatio @Override public OAuth2AuthorizationConsent mapRow(ResultSet rs, int rowNum) throws SQLException { String registeredClientId = rs.getString("registered_client_id"); - - RegisteredClient registeredClient = this.registeredClientRepository - .findById(registeredClientId); + RegisteredClient registeredClient = this.registeredClientRepository.findById(registeredClientId); if (registeredClient == null) { throw new DataRetrievalFailureException( "The RegisteredClient with id '" + registeredClientId + "' was not found in the RegisteredClientRepository."); diff --git a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/JdbcOAuth2AuthorizationConsentServiceTests.java b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/JdbcOAuth2AuthorizationConsentServiceTests.java index 8e994581..d278f417 100644 --- a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/JdbcOAuth2AuthorizationConsentServiceTests.java +++ b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/JdbcOAuth2AuthorizationConsentServiceTests.java @@ -56,7 +56,6 @@ import static org.mockito.Mockito.when; * @author Ovidiu Popa */ public class JdbcOAuth2AuthorizationConsentServiceTests { - private static final String OAUTH2_AUTHORIZATION_CONSENT_SCHEMA_SQL_RESOURCE = "org/springframework/security/oauth2/server/authorization/oauth2-authorization-consent-schema.sql"; private static final String CUSTOM_OAUTH2_AUTHORIZATION_CONSENT_SCHEMA_SQL_RESOURCE = "org/springframework/security/oauth2/server/authorization/custom-oauth2-authorization-consent-schema.sql"; private static final String PRINCIPAL_NAME = "principal-name"; @@ -64,7 +63,11 @@ public class JdbcOAuth2AuthorizationConsentServiceTests { private static final OAuth2AuthorizationConsent AUTHORIZATION_CONSENT = OAuth2AuthorizationConsent.withId(REGISTERED_CLIENT.getId(), PRINCIPAL_NAME) - .authority(new SimpleGrantedAuthority("some.authority")) + .authority(new SimpleGrantedAuthority("SCOPE_scope1")) + .authority(new SimpleGrantedAuthority("SCOPE_scope2")) + .authority(new SimpleGrantedAuthority("SCOPE_scope3")) + .authority(new SimpleGrantedAuthority("authority-a")) + .authority(new SimpleGrantedAuthority("authority-b")) .build(); private EmbeddedDatabase db; @@ -72,6 +75,19 @@ public class JdbcOAuth2AuthorizationConsentServiceTests { private RegisteredClientRepository registeredClientRepository; private JdbcOAuth2AuthorizationConsentService authorizationConsentService; + @Before + public void setUp() { + this.db = createDb(); + this.jdbcOperations = new JdbcTemplate(this.db); + this.registeredClientRepository = mock(RegisteredClientRepository.class); + this.authorizationConsentService = new JdbcOAuth2AuthorizationConsentService(this.jdbcOperations, this.registeredClientRepository); + } + + @After + public void tearDown() { + this.db.shutdown(); + } + @Test public void constructorWhenJdbcOperationsIsNullThenThrowIllegalArgumentException() { // @formatter:off @@ -127,7 +143,7 @@ public class JdbcOAuth2AuthorizationConsentServiceTests { RegisteredClient newRegisteredClient = TestRegisteredClients.registeredClient() .id("new-client").build(); - when(registeredClientRepository.findById(eq(newRegisteredClient.getId()))) + when(this.registeredClientRepository.findById(eq(newRegisteredClient.getId()))) .thenReturn(newRegisteredClient); this.authorizationConsentService.save(expectedAuthorizationConsent); @@ -143,7 +159,7 @@ public class JdbcOAuth2AuthorizationConsentServiceTests { OAuth2AuthorizationConsent.from(AUTHORIZATION_CONSENT) .authority(new SimpleGrantedAuthority("new.authority")) .build(); - when(registeredClientRepository.findById(eq(REGISTERED_CLIENT.getId()))) + when(this.registeredClientRepository.findById(eq(REGISTERED_CLIENT.getId()))) .thenReturn(REGISTERED_CLIENT); this.authorizationConsentService.save(expectedAuthorizationConsent); @@ -157,7 +173,7 @@ public class JdbcOAuth2AuthorizationConsentServiceTests { @Test public void saveLoadAuthorizationConsentWhenCustomStrategiesSetThenCalled() throws Exception { - when(registeredClientRepository.findById(eq(REGISTERED_CLIENT.getId()))) + when(this.registeredClientRepository.findById(eq(REGISTERED_CLIENT.getId()))) .thenReturn(REGISTERED_CLIENT); JdbcOAuth2AuthorizationConsentService.OAuth2AuthorizationConsentRowMapper authorizationConsentRowMapper = spy( @@ -205,6 +221,17 @@ public class JdbcOAuth2AuthorizationConsentServiceTests { .withMessage("principalName cannot be empty"); } + @Test + public void findByIdWhenAuthorizationConsentExistsThenFound() { + when(this.registeredClientRepository.findById(eq(REGISTERED_CLIENT.getId()))) + .thenReturn(REGISTERED_CLIENT); + + this.authorizationConsentService.save(AUTHORIZATION_CONSENT); + OAuth2AuthorizationConsent authorizationConsent = this.authorizationConsentService.findById( + AUTHORIZATION_CONSENT.getRegisteredClientId(), AUTHORIZATION_CONSENT.getPrincipalName()); + assertThat(authorizationConsent).isNotNull(); + } + @Test public void findByIdWhenAuthorizationConsentDoesNotExistThenNull() { this.authorizationConsentService.save(AUTHORIZATION_CONSENT); @@ -221,27 +248,16 @@ public class JdbcOAuth2AuthorizationConsentServiceTests { OAuth2AuthorizationConsentService authorizationConsentService = new CustomJdbcOAuth2AuthorizationConsentService(new JdbcTemplate(db), this.registeredClientRepository); authorizationConsentService.save(AUTHORIZATION_CONSENT); - OAuth2AuthorizationConsent foundAuthorizationConsent1 = authorizationConsentService.findById(AUTHORIZATION_CONSENT.getRegisteredClientId(), AUTHORIZATION_CONSENT.getPrincipalName()); + OAuth2AuthorizationConsent foundAuthorizationConsent1 = authorizationConsentService.findById( + AUTHORIZATION_CONSENT.getRegisteredClientId(), AUTHORIZATION_CONSENT.getPrincipalName()); assertThat(foundAuthorizationConsent1).isEqualTo(AUTHORIZATION_CONSENT); authorizationConsentService.remove(AUTHORIZATION_CONSENT); - OAuth2AuthorizationConsent foundAuthorizationConsent2 = authorizationConsentService.findById(REGISTERED_CLIENT.getClientId(), AUTHORIZATION_CONSENT.getPrincipalName()); + OAuth2AuthorizationConsent foundAuthorizationConsent2 = authorizationConsentService.findById( + AUTHORIZATION_CONSENT.getRegisteredClientId(), AUTHORIZATION_CONSENT.getPrincipalName()); assertThat(foundAuthorizationConsent2).isNull(); db.shutdown(); } - @Before - public void setUp() { - this.db = createDb(); - this.registeredClientRepository = mock(RegisteredClientRepository.class); - this.jdbcOperations = new JdbcTemplate(this.db); - this.authorizationConsentService = new JdbcOAuth2AuthorizationConsentService(this.jdbcOperations, this.registeredClientRepository); - } - - @After - public void tearDown() { - this.db.shutdown(); - } - private static EmbeddedDatabase createDb() { return createDb(OAUTH2_AUTHORIZATION_CONSENT_SCHEMA_SQL_RESOURCE); } @@ -282,7 +298,7 @@ public class JdbcOAuth2AuthorizationConsentServiceTests { private static final String REMOVE_AUTHORIZATION_CONSENT_SQL = "DELETE FROM " + TABLE_NAME + " WHERE " + PK_FILTER; - CustomJdbcOAuth2AuthorizationConsentService(JdbcOperations jdbcOperations, RegisteredClientRepository registeredClientRepository) { + private CustomJdbcOAuth2AuthorizationConsentService(JdbcOperations jdbcOperations, RegisteredClientRepository registeredClientRepository) { super(jdbcOperations, registeredClientRepository); setAuthorizationConsentRowMapper(new CustomOAuth2AuthorizationConsentRowMapper(registeredClientRepository)); } @@ -317,16 +333,14 @@ public class JdbcOAuth2AuthorizationConsentServiceTests { private static final class CustomOAuth2AuthorizationConsentRowMapper extends JdbcOAuth2AuthorizationConsentService.OAuth2AuthorizationConsentRowMapper { - CustomOAuth2AuthorizationConsentRowMapper(RegisteredClientRepository registeredClientRepository) { + private CustomOAuth2AuthorizationConsentRowMapper(RegisteredClientRepository registeredClientRepository) { super(registeredClientRepository); } @Override public OAuth2AuthorizationConsent mapRow(ResultSet rs, int rowNum) throws SQLException { String registeredClientId = rs.getString("registeredClientId"); - - RegisteredClient registeredClient = getRegisteredClientRepository() - .findById(registeredClientId); + RegisteredClient registeredClient = getRegisteredClientRepository().findById(registeredClientId); if (registeredClient == null) { throw new DataRetrievalFailureException( "The RegisteredClient with id '" + registeredClientId + "' was not found in the RegisteredClientRepository.");