From 78f44c73274ee61f6074b132597f90888a90b586 Mon Sep 17 00:00:00 2001 From: Joe Grandja Date: Wed, 29 Apr 2020 16:51:16 -0400 Subject: [PATCH] Polish gh-70 --- .../oauth2/server/authorization/Version.java | 13 ++- .../InMemoryRegisteredClientRepository.java | 5 +- .../client/RegisteredClient.java | 85 ++++++++++--------- .../client/RegisteredClientRepository.java | 1 + ...MemoryRegisteredClientRepositoryTests.java | 14 +-- .../client/RegisteredClientTests.java | 70 ++++----------- 6 files changed, 84 insertions(+), 104 deletions(-) diff --git a/core/src/main/java/org/springframework/security/oauth2/server/authorization/Version.java b/core/src/main/java/org/springframework/security/oauth2/server/authorization/Version.java index defa5b4..19d55ab 100644 --- a/core/src/main/java/org/springframework/security/oauth2/server/authorization/Version.java +++ b/core/src/main/java/org/springframework/security/oauth2/server/authorization/Version.java @@ -19,10 +19,19 @@ package org.springframework.security.oauth2.server.authorization; * Internal class used for serialization across Spring Security Authorization Server classes. * * @author Anoop Garlapati + * @since 0.0.1 */ -public class Version { +public final class Version { + private static final int MAJOR = 0; + private static final int MINOR = 0; + private static final int PATCH = 1; + /** * Global Serialization value for Spring Security Authorization Server classes. */ - public static final long SERIAL_VERSION_UID = "0.0.1".hashCode(); + public static final long SERIAL_VERSION_UID = getVersion().hashCode(); + + public static String getVersion() { + return MAJOR + "." + MINOR + "." + PATCH; + } } diff --git a/core/src/main/java/org/springframework/security/oauth2/server/authorization/client/InMemoryRegisteredClientRepository.java b/core/src/main/java/org/springframework/security/oauth2/server/authorization/client/InMemoryRegisteredClientRepository.java index 577816e..c633164 100644 --- a/core/src/main/java/org/springframework/security/oauth2/server/authorization/client/InMemoryRegisteredClientRepository.java +++ b/core/src/main/java/org/springframework/security/oauth2/server/authorization/client/InMemoryRegisteredClientRepository.java @@ -28,6 +28,7 @@ import java.util.concurrent.ConcurrentHashMap; * @author Anoop Garlapati * @see RegisteredClientRepository * @see RegisteredClient + * @since 0.0.1 */ public final class InMemoryRegisteredClientRepository implements RegisteredClientRepository { private final Map idRegistrationMap; @@ -66,8 +67,8 @@ public final class InMemoryRegisteredClientRepository implements RegisteredClien idRegistrationMapResult.put(id, registration); clientIdRegistrationMapResult.put(clientId, registration); } - idRegistrationMap = idRegistrationMapResult; - clientIdRegistrationMap = clientIdRegistrationMapResult; + this.idRegistrationMap = idRegistrationMapResult; + this.clientIdRegistrationMap = clientIdRegistrationMapResult; } @Override diff --git a/core/src/main/java/org/springframework/security/oauth2/server/authorization/client/RegisteredClient.java b/core/src/main/java/org/springframework/security/oauth2/server/authorization/client/RegisteredClient.java index 4f24b3c..2e289a5 100644 --- a/core/src/main/java/org/springframework/security/oauth2/server/authorization/client/RegisteredClient.java +++ b/core/src/main/java/org/springframework/security/oauth2/server/authorization/client/RegisteredClient.java @@ -25,7 +25,6 @@ import java.io.Serializable; import java.net.URI; import java.net.URISyntaxException; import java.util.Collections; -import java.util.HashSet; import java.util.LinkedHashSet; import java.util.Set; import java.util.function.Consumer; @@ -36,17 +35,17 @@ import java.util.function.Consumer; * @author Joe Grandja * @author Anoop Garlapati * @see Section 2 Client Registration + * @since 0.0.1 */ public class RegisteredClient implements Serializable { private static final long serialVersionUID = Version.SERIAL_VERSION_UID; private String id; private String clientId; private String clientSecret; - private Set clientAuthenticationMethods = - Collections.singleton(ClientAuthenticationMethod.BASIC); - private Set authorizationGrantTypes = Collections.emptySet(); - private Set redirectUris = Collections.emptySet(); - private Set scopes = Collections.emptySet(); + private Set clientAuthenticationMethods; + private Set authorizationGrantTypes; + private Set redirectUris; + private Set scopes; protected RegisteredClient() { } @@ -157,8 +156,7 @@ public class RegisteredClient implements Serializable { private String id; private String clientId; private String clientSecret; - private Set clientAuthenticationMethods = - new LinkedHashSet<>(Collections.singletonList(ClientAuthenticationMethod.BASIC)); + private Set clientAuthenticationMethods = new LinkedHashSet<>(); private Set authorizationGrantTypes = new LinkedHashSet<>(); private Set redirectUris = new LinkedHashSet<>(); private Set scopes = new LinkedHashSet<>(); @@ -171,13 +169,18 @@ public class RegisteredClient implements Serializable { this.id = registeredClient.id; this.clientId = registeredClient.clientId; this.clientSecret = registeredClient.clientSecret; - this.clientAuthenticationMethods = registeredClient.clientAuthenticationMethods == null ? null : - new HashSet<>(registeredClient.clientAuthenticationMethods); - this.authorizationGrantTypes = registeredClient.authorizationGrantTypes == null ? null : - new HashSet<>(registeredClient.authorizationGrantTypes); - this.redirectUris = registeredClient.redirectUris == null ? null : - new HashSet<>(registeredClient.redirectUris); - this.scopes = registeredClient.scopes == null ? null : new HashSet<>(registeredClient.scopes); + if (!CollectionUtils.isEmpty(registeredClient.clientAuthenticationMethods)) { + this.clientAuthenticationMethods.addAll(registeredClient.clientAuthenticationMethods); + } + if (!CollectionUtils.isEmpty(registeredClient.authorizationGrantTypes)) { + this.authorizationGrantTypes.addAll(registeredClient.authorizationGrantTypes); + } + if (!CollectionUtils.isEmpty(registeredClient.redirectUris)) { + this.redirectUris.addAll(registeredClient.redirectUris); + } + if (!CollectionUtils.isEmpty(registeredClient.scopes)) { + this.scopes.addAll(registeredClient.scopes); + } } /** @@ -214,8 +217,8 @@ public class RegisteredClient implements Serializable { } /** - * Adds the {@link ClientAuthenticationMethod authentication method} to the set of - * client authentication methods used when authenticating the client with the authorization server. + * Adds an {@link ClientAuthenticationMethod authentication method} + * the client may use when authenticating with the authorization server. * * @param clientAuthenticationMethod the authentication method * @return the {@link Builder} @@ -226,10 +229,10 @@ public class RegisteredClient implements Serializable { } /** - * Sets the {@link ClientAuthenticationMethod authentication method(s)} used - * when authenticating the client with the authorization server. + * A {@code Consumer} of the {@link ClientAuthenticationMethod authentication method(s)} + * allowing the ability to add, replace, or remove. * - * @param clientAuthenticationMethodsConsumer the authentication method(s) {@link Consumer} + * @param clientAuthenticationMethodsConsumer a {@code Consumer} of the authentication method(s) * @return the {@link Builder} */ public Builder clientAuthenticationMethods( @@ -239,8 +242,7 @@ public class RegisteredClient implements Serializable { } /** - * Adds the {@link AuthorizationGrantType authorization grant type} to - * the set of authorization grant types that the client may use. + * Adds an {@link AuthorizationGrantType authorization grant type} the client may use. * * @param authorizationGrantType the authorization grant type * @return the {@link Builder} @@ -251,9 +253,10 @@ public class RegisteredClient implements Serializable { } /** - * Sets the {@link AuthorizationGrantType authorization grant type(s)} that the client may use. + * A {@code Consumer} of the {@link AuthorizationGrantType authorization grant type(s)} + * allowing the ability to add, replace, or remove. * - * @param authorizationGrantTypesConsumer the authorization grant type(s) {@link Consumer} + * @param authorizationGrantTypesConsumer a {@code Consumer} of the authorization grant type(s) * @return the {@link Builder} */ public Builder authorizationGrantTypes(Consumer> authorizationGrantTypesConsumer) { @@ -262,9 +265,9 @@ public class RegisteredClient implements Serializable { } /** - * Adds the redirect URI to the set of redirect URIs that the client may use in redirect-based flows. + * Adds a redirect URI the client may use in a redirect-based flow. * - * @param redirectUri the redirect URI to add + * @param redirectUri the redirect URI * @return the {@link Builder} */ public Builder redirectUri(String redirectUri) { @@ -273,9 +276,10 @@ public class RegisteredClient implements Serializable { } /** - * Sets the redirect URI(s) that the client may use in redirect-based flows. + * A {@code Consumer} of the redirect URI(s) + * allowing the ability to add, replace, or remove. * - * @param redirectUrisConsumer the redirect URI(s) {@link Consumer} + * @param redirectUrisConsumer a {@link Consumer} of the redirect URI(s) * @return the {@link Builder} */ public Builder redirectUris(Consumer> redirectUrisConsumer) { @@ -284,9 +288,9 @@ public class RegisteredClient implements Serializable { } /** - * Adds the scope to the set of scopes used by the client. + * Adds a scope the client may use. * - * @param scope the scope to add + * @param scope the scope * @return the {@link Builder} */ public Builder scope(String scope) { @@ -295,9 +299,10 @@ public class RegisteredClient implements Serializable { } /** - * Sets the scope(s) used by the client. + * A {@code Consumer} of the scope(s) + * allowing the ability to add, replace, or remove. * - * @param scopesConsumer the scope(s) {@link Consumer} + * @param scopesConsumer a {@link Consumer} of the scope(s) * @return the {@link Builder} */ public Builder scopes(Consumer> scopesConsumer) { @@ -311,17 +316,18 @@ public class RegisteredClient implements Serializable { * @return a {@link RegisteredClient} */ public RegisteredClient build() { - Assert.notEmpty(this.clientAuthenticationMethods, "clientAuthenticationMethods cannot be empty"); + Assert.hasText(this.clientId, "clientId cannot be empty"); Assert.notEmpty(this.authorizationGrantTypes, "authorizationGrantTypes cannot be empty"); - if (authorizationGrantTypes.contains(AuthorizationGrantType.AUTHORIZATION_CODE)) { - Assert.hasText(this.id, "id cannot be empty"); - Assert.hasText(this.clientId, "clientId cannot be empty"); + if (this.authorizationGrantTypes.contains(AuthorizationGrantType.AUTHORIZATION_CODE)) { Assert.hasText(this.clientSecret, "clientSecret cannot be empty"); Assert.notEmpty(this.redirectUris, "redirectUris cannot be empty"); } - this.validateScopes(); - this.validateRedirectUris(); - return this.create(); + if (CollectionUtils.isEmpty(this.clientAuthenticationMethods)) { + this.clientAuthenticationMethods.add(ClientAuthenticationMethod.BASIC); + } + validateScopes(); + validateRedirectUris(); + return create(); } private RegisteredClient create() { @@ -380,5 +386,4 @@ public class RegisteredClient implements Serializable { } } } - } diff --git a/core/src/main/java/org/springframework/security/oauth2/server/authorization/client/RegisteredClientRepository.java b/core/src/main/java/org/springframework/security/oauth2/server/authorization/client/RegisteredClientRepository.java index 4da1374..add30f9 100644 --- a/core/src/main/java/org/springframework/security/oauth2/server/authorization/client/RegisteredClientRepository.java +++ b/core/src/main/java/org/springframework/security/oauth2/server/authorization/client/RegisteredClientRepository.java @@ -21,6 +21,7 @@ package org.springframework.security.oauth2.server.authorization.client; * @author Joe Grandja * @author Anoop Garlapati * @see RegisteredClient + * @since 0.0.1 */ public interface RegisteredClientRepository { diff --git a/core/src/test/java/org/springframework/security/oauth2/server/authorization/client/InMemoryRegisteredClientRepositoryTests.java b/core/src/test/java/org/springframework/security/oauth2/server/authorization/client/InMemoryRegisteredClientRepositoryTests.java index 54cc8e6..aae2cfd 100644 --- a/core/src/test/java/org/springframework/security/oauth2/server/authorization/client/InMemoryRegisteredClientRepositoryTests.java +++ b/core/src/test/java/org/springframework/security/oauth2/server/authorization/client/InMemoryRegisteredClientRepositoryTests.java @@ -51,7 +51,7 @@ public class InMemoryRegisteredClientRepositoryTests { } @Test - public void constructorListClientRegistrationWhenEmptyThenThrowIllegalArgumentException() { + public void constructorListRegisteredClientWhenEmptyThenThrowIllegalArgumentException() { assertThatThrownBy(() -> { List registrations = Collections.emptyList(); new InMemoryRegisteredClientRepository(registrations); @@ -82,34 +82,34 @@ public class InMemoryRegisteredClientRepositoryTests { @Test public void findByIdWhenFoundThenFound() { String id = this.registration.getId(); - assertThat(clients.findById(id)).isEqualTo(this.registration); + assertThat(this.clients.findById(id)).isEqualTo(this.registration); } @Test public void findByIdWhenNotFoundThenNull() { String missingId = this.registration.getId() + "MISSING"; - assertThat(clients.findById(missingId)).isNull(); + assertThat(this.clients.findById(missingId)).isNull(); } @Test public void findByIdWhenNullThenThrowIllegalArgumentException() { - assertThatThrownBy(() -> clients.findById(null)).isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> this.clients.findById(null)).isInstanceOf(IllegalArgumentException.class); } @Test public void findByClientIdWhenFoundThenFound() { String clientId = this.registration.getClientId(); - assertThat(clients.findByClientId(clientId)).isEqualTo(this.registration); + assertThat(this.clients.findByClientId(clientId)).isEqualTo(this.registration); } @Test public void findByClientIdWhenNotFoundThenNull() { String missingClientId = this.registration.getClientId() + "MISSING"; - assertThat(clients.findByClientId(missingClientId)).isNull(); + assertThat(this.clients.findByClientId(missingClientId)).isNull(); } @Test public void findByClientIdWhenNullThenThrowIllegalArgumentException() { - assertThatThrownBy(() -> clients.findByClientId(null)).isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> this.clients.findByClientId(null)).isInstanceOf(IllegalArgumentException.class); } } diff --git a/core/src/test/java/org/springframework/security/oauth2/server/authorization/client/RegisteredClientTests.java b/core/src/test/java/org/springframework/security/oauth2/server/authorization/client/RegisteredClientTests.java index 9875e86..85a0267 100644 --- a/core/src/test/java/org/springframework/security/oauth2/server/authorization/client/RegisteredClientTests.java +++ b/core/src/test/java/org/springframework/security/oauth2/server/authorization/client/RegisteredClientTests.java @@ -43,7 +43,7 @@ public class RegisteredClientTests { Collections.singleton(ClientAuthenticationMethod.BASIC); @Test - public void buildWhenAuthorizationGrantTypesIsNotSetThenThrowIllegalArgumentException() { + public void buildWhenAuthorizationGrantTypesNotSetThenThrowIllegalArgumentException() { assertThatThrownBy(() -> RegisteredClient.withId(ID) .clientId(CLIENT_ID) @@ -56,7 +56,7 @@ public class RegisteredClientTests { } @Test - public void buildWhenAuthorizationCodeGrantAllAttributesProvidedThenAllAttributesAreSet() { + public void buildWhenAllAttributesProvidedThenAllAttributesAreSet() { RegisteredClient registration = RegisteredClient.withId(ID) .clientId(CLIENT_ID) .clientSecret(CLIENT_SECRET) @@ -77,21 +77,13 @@ public class RegisteredClientTests { } @Test - public void buildWhenAuthorizationCodeGrantIdIsNullThenThrowIllegalArgumentException() { - assertThatThrownBy(() -> - RegisteredClient.withId(null) - .clientId(CLIENT_ID) - .clientSecret(CLIENT_SECRET) - .authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE) - .clientAuthenticationMethod(ClientAuthenticationMethod.BASIC) - .redirectUris(redirectUris -> redirectUris.addAll(REDIRECT_URIS)) - .scopes(scopes -> scopes.addAll(SCOPES)) - .build() - ).isInstanceOf(IllegalArgumentException.class); + public void buildWhenIdIsNullThenThrowIllegalArgumentException() { + assertThatThrownBy(() -> RegisteredClient.withId(null)) + .isInstanceOf(IllegalArgumentException.class); } @Test - public void buildWhenAuthorizationCodeGrantClientIdIsNullThenThrowIllegalArgumentException() { + public void buildWhenClientIdIsNullThenThrowIllegalArgumentException() { assertThatThrownBy(() -> RegisteredClient.withId(ID) .clientId(null) @@ -105,21 +97,7 @@ public class RegisteredClientTests { } @Test - public void buildWhenAuthorizationCodeGrantClientSecretIsNullThenThrowIllegalArgumentException() { - assertThatThrownBy(() -> - RegisteredClient.withId(ID) - .clientId(null) - .clientSecret(CLIENT_SECRET) - .authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE) - .clientAuthenticationMethod(ClientAuthenticationMethod.BASIC) - .redirectUris(redirectUris -> redirectUris.addAll(REDIRECT_URIS)) - .scopes(scopes -> scopes.addAll(SCOPES)) - .build() - ).isInstanceOf(IllegalArgumentException.class); - } - - @Test - public void buildWhenAuthorizationCodeGrantRedirectUrisNotProvidedThenThrowIllegalArgumentException() { + public void buildWhenRedirectUrisNotProvidedThenThrowIllegalArgumentException() { assertThatThrownBy(() -> RegisteredClient.withId(ID) .clientId(CLIENT_ID) @@ -132,7 +110,7 @@ public class RegisteredClientTests { } @Test - public void buildWhenAuthorizationCodeGrantRedirectUrisConsumerClearsSetThenThrowIllegalArgumentException() { + public void buildWhenRedirectUrisConsumerClearsSetThenThrowIllegalArgumentException() { assertThatThrownBy(() -> RegisteredClient.withId(ID) .clientId(CLIENT_ID) @@ -147,7 +125,7 @@ public class RegisteredClientTests { } @Test - public void buildWhenAuthorizationCodeGrantClientAuthenticationMethodNotProvidedThenDefaultToBasic() { + public void buildWhenClientAuthenticationMethodNotProvidedThenDefaultToBasic() { RegisteredClient registration = RegisteredClient.withId(ID) .clientId(CLIENT_ID) .clientSecret(CLIENT_SECRET) @@ -161,7 +139,7 @@ public class RegisteredClientTests { } @Test - public void buildWhenAuthorizationCodeGrantScopeIsEmptyThenScopeNotRequired() { + public void buildWhenScopeIsEmptyThenScopeNotRequired() { RegisteredClient.withId(ID) .clientId(CLIENT_ID) .clientSecret(CLIENT_SECRET) @@ -172,7 +150,7 @@ public class RegisteredClientTests { } @Test - public void buildWhenAuthorizationCodeGrantScopeConsumerIsProvidedThenConsumerAccepted() { + public void buildWhenScopeConsumerIsProvidedThenConsumerAccepted() { RegisteredClient registration = RegisteredClient.withId(ID) .clientId(CLIENT_ID) .clientSecret(CLIENT_SECRET) @@ -186,11 +164,10 @@ public class RegisteredClientTests { } @Test - public void buildWhenScopeContainsASpaceThenThrowIllegalArgumentException() { + public void buildWhenScopeContainsSpaceThenThrowIllegalArgumentException() { assertThatThrownBy(() -> RegisteredClient.withId(ID) .clientId(CLIENT_ID) - .clientSecret(null) .authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE) .clientAuthenticationMethod(ClientAuthenticationMethod.BASIC) .redirectUris(redirectUris -> redirectUris.addAll(REDIRECT_URIS)) @@ -200,7 +177,7 @@ public class RegisteredClientTests { } @Test - public void buildWhenScopesContainsAnInvalidCharacterThenThrowIllegalArgumentException() { + public void buildWhenScopeContainsInvalidCharacterThenThrowIllegalArgumentException() { assertThatThrownBy(() -> RegisteredClient.withId(ID) .clientId(CLIENT_ID) @@ -214,7 +191,7 @@ public class RegisteredClientTests { } @Test - public void buildWhenRedirectUrisContainInvalidUriThenThrowIllegalArgumentException() { + public void buildWhenRedirectUriInvalidThenThrowIllegalArgumentException() { assertThatThrownBy(() -> RegisteredClient.withId(ID) .clientId(CLIENT_ID) @@ -228,7 +205,7 @@ public class RegisteredClientTests { } @Test - public void buildWhenRedirectUrisContainUriWithFragmentThenThrowIllegalArgumentException() { + public void buildWhenRedirectUriContainsFragmentThenThrowIllegalArgumentException() { assertThatThrownBy(() -> RegisteredClient.withId(ID) .clientId(CLIENT_ID) @@ -281,6 +258,7 @@ public class RegisteredClientTests { RegisteredClient.withId(ID) .clientId(CLIENT_ID) .clientSecret(CLIENT_SECRET) + .authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE) .authorizationGrantTypes(Set::clear) .clientAuthenticationMethod(ClientAuthenticationMethod.BASIC) .redirectUris(redirectUris -> redirectUris.addAll(REDIRECT_URIS)) @@ -323,20 +301,6 @@ public class RegisteredClientTests { .containsExactly(ClientAuthenticationMethod.BASIC, ClientAuthenticationMethod.POST); } - @Test - public void buildWhenClientAuthenticationMethodsConsumerClearsSetThenThrowIllegalArgumentException() { - assertThatThrownBy(() -> { - RegisteredClient.withId(ID) - .clientId(CLIENT_ID) - .clientSecret(CLIENT_SECRET) - .authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE) - .clientAuthenticationMethods(Set::clear) - .redirectUris(redirectUris -> redirectUris.addAll(REDIRECT_URIS)) - .scopes(scopes -> scopes.addAll(SCOPES)) - .build(); - }).isInstanceOf(IllegalArgumentException.class); - } - @Test public void buildWhenOverrideIdThenOverridden() { String overriddenId = "override"; @@ -383,7 +347,7 @@ public class RegisteredClientTests { } @Test - public void buildWhenClientRegistrationValuesOverriddenThenPropagated() { + public void buildWhenRegisteredClientValuesOverriddenThenPropagated() { RegisteredClient registration = TestRegisteredClients.registeredClient().build(); String newSecret = "new-secret"; String newScope = "new-scope";