Relax validation on ClientRegistration

Fixes gh-5667
spring-projects · Aug 14, 2018 · cbdc7ee · cbdc7ee
1 parent 010d99a
commit cbdc7ee
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 78 deletions.
diff --git a/...main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java b/...main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java
@@ -18,7 +18,6 @@
 import org.springframework.security.oauth2.core.AuthenticationMethod;
 import org.springframework.security.oauth2.core.AuthorizationGrantType;
 import org.springframework.security.oauth2.core.ClientAuthenticationMethod;
-import org.springframework.security.oauth2.core.oidc.OidcScopes;
 import org.springframework.util.Assert;
 import org.springframework.util.StringUtils;
 
@@ -479,7 +478,8 @@ private ClientRegistration create() {
 			providerDetails.jwkSetUri = this.jwkSetUri;
 			clientRegistration.providerDetails = providerDetails;
 
-			clientRegistration.clientName = this.clientName;
+			clientRegistration.clientName = StringUtils.hasText(this.clientName) ?
+					this.clientName : this.registrationId;
 
 			return clientRegistration;
 		}
@@ -489,15 +489,9 @@ private void validateAuthorizationCodeGrantType() {
 					() -> "authorizationGrantType must be " + AuthorizationGrantType.AUTHORIZATION_CODE.getValue());
 			Assert.hasText(this.registrationId, "registrationId cannot be empty");
 			Assert.hasText(this.clientId, "clientId cannot be empty");
-			Assert.notNull(this.clientAuthenticationMethod, "clientAuthenticationMethod cannot be null");
 			Assert.hasText(this.redirectUriTemplate, "redirectUriTemplate cannot be empty");
 			Assert.hasText(this.authorizationUri, "authorizationUri cannot be empty");
 			Assert.hasText(this.tokenUri, "tokenUri cannot be empty");
-			if (this.scopes != null && this.scopes.contains(OidcScopes.OPENID)) {
-				// OIDC Clients need to verify/validate the ID Token
-				Assert.hasText(this.jwkSetUri, "jwkSetUri cannot be empty");
-			}
-			Assert.hasText(this.clientName, "clientName cannot be empty");
 		}
 
 		private void validateImplicitGrantType() {
@@ -507,15 +501,13 @@ private void validateImplicitGrantType() {
 			Assert.hasText(this.clientId, "clientId cannot be empty");
 			Assert.hasText(this.redirectUriTemplate, "redirectUriTemplate cannot be empty");
 			Assert.hasText(this.authorizationUri, "authorizationUri cannot be empty");
-			Assert.hasText(this.clientName, "clientName cannot be empty");
 		}
 
 		private void validateClientCredentialsGrantType() {
 			Assert.isTrue(AuthorizationGrantType.CLIENT_CREDENTIALS.equals(this.authorizationGrantType),
 					() -> "authorizationGrantType must be " + AuthorizationGrantType.CLIENT_CREDENTIALS.getValue());
 			Assert.hasText(this.registrationId, "registrationId cannot be empty");
 			Assert.hasText(this.clientId, "clientId cannot be empty");
-			Assert.notNull(this.clientAuthenticationMethod, "clientAuthenticationMethod cannot be null");
 			Assert.hasText(this.tokenUri, "tokenUri cannot be empty");
 		}
 	}

diff --git a/...java/org/springframework/security/oauth2/client/registration/ClientRegistrationTests.java b/...java/org/springframework/security/oauth2/client/registration/ClientRegistrationTests.java
@@ -142,21 +142,21 @@ public void buildWhenAuthorizationCodeGrantClientSecretIsNullThenDefaultToEmpty(
 		assertThat(clientRegistration.getClientSecret()).isEqualTo("");
 	}
 
-	@Test(expected = IllegalArgumentException.class)
-	public void buildWhenAuthorizationCodeGrantClientAuthenticationMethodIsNullThenThrowIllegalArgumentException() {
-		ClientRegistration.withRegistrationId(REGISTRATION_ID)
-			.clientId(CLIENT_ID)
-			.clientSecret(CLIENT_SECRET)
-			.clientAuthenticationMethod(null)
-			.authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE)
-			.redirectUriTemplate(REDIRECT_URI)
-			.scope(SCOPES.toArray(new String[0]))
-			.authorizationUri(AUTHORIZATION_URI)
-			.tokenUri(TOKEN_URI)
-			.userInfoAuthenticationMethod(AuthenticationMethod.FORM)
-			.jwkSetUri(JWK_SET_URI)
-			.clientName(CLIENT_NAME)
-			.build();
+	@Test
+	public void buildWhenAuthorizationCodeGrantClientAuthenticationMethodNotProvidedThenDefaultToBasic() {
+		ClientRegistration clientRegistration = ClientRegistration.withRegistrationId(REGISTRATION_ID)
+				.clientId(CLIENT_ID)
+				.clientSecret(CLIENT_SECRET)
+				.authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE)
+				.redirectUriTemplate(REDIRECT_URI)
+				.scope(SCOPES.toArray(new String[0]))
+				.authorizationUri(AUTHORIZATION_URI)
+				.tokenUri(TOKEN_URI)
+				.userInfoAuthenticationMethod(AuthenticationMethod.FORM)
+				.jwkSetUri(JWK_SET_URI)
+				.clientName(CLIENT_NAME)
+				.build();
+		assertThat(clientRegistration.getClientAuthenticationMethod()).isEqualTo(ClientAuthenticationMethod.BASIC);
 	}
 
 	@Test(expected = IllegalArgumentException.class)
@@ -228,38 +228,21 @@ public void buildWhenAuthorizationCodeGrantTokenUriIsNullThenThrowIllegalArgumen
 			.build();
 	}
 
-	@Test(expected = IllegalArgumentException.class)
-	public void buildWhenAuthorizationCodeGrantJwkSetUriIsNullThenThrowIllegalArgumentException() {
-		ClientRegistration.withRegistrationId(REGISTRATION_ID)
-			.clientId(CLIENT_ID)
-			.clientSecret(CLIENT_SECRET)
-			.clientAuthenticationMethod(ClientAuthenticationMethod.BASIC)
-			.authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE)
-			.redirectUriTemplate(REDIRECT_URI)
-			.scope(SCOPES.toArray(new String[0]))
-			.authorizationUri(AUTHORIZATION_URI)
-			.tokenUri(TOKEN_URI)
-			.userInfoAuthenticationMethod(AuthenticationMethod.FORM)
-			.jwkSetUri(null)
-			.clientName(CLIENT_NAME)
-			.build();
-	}
-
-	@Test(expected = IllegalArgumentException.class)
-	public void buildWhenAuthorizationCodeGrantClientNameIsNullThenThrowIllegalArgumentException() {
-		ClientRegistration.withRegistrationId(REGISTRATION_ID)
-			.clientId(CLIENT_ID)
-			.clientSecret(CLIENT_SECRET)
-			.clientAuthenticationMethod(ClientAuthenticationMethod.BASIC)
-			.authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE)
-			.redirectUriTemplate(REDIRECT_URI)
-			.scope(SCOPES.toArray(new String[0]))
-			.authorizationUri(AUTHORIZATION_URI)
-			.tokenUri(TOKEN_URI)
-			.userInfoAuthenticationMethod(AuthenticationMethod.FORM)
-			.jwkSetUri(JWK_SET_URI)
-			.clientName(null)
-			.build();
+	@Test
+	public void buildWhenAuthorizationCodeGrantClientNameNotProvidedThenDefaultToRegistrationId() {
+		ClientRegistration clientRegistration = ClientRegistration.withRegistrationId(REGISTRATION_ID)
+				.clientId(CLIENT_ID)
+				.clientSecret(CLIENT_SECRET)
+				.clientAuthenticationMethod(ClientAuthenticationMethod.BASIC)
+				.authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE)
+				.redirectUriTemplate(REDIRECT_URI)
+				.scope(SCOPES.toArray(new String[0]))
+				.authorizationUri(AUTHORIZATION_URI)
+				.tokenUri(TOKEN_URI)
+				.userInfoAuthenticationMethod(AuthenticationMethod.FORM)
+				.jwkSetUri(JWK_SET_URI)
+				.build();
+		assertThat(clientRegistration.getClientName()).isEqualTo(clientRegistration.getRegistrationId());
 	}
 
 	@Test
@@ -381,17 +364,17 @@ public void buildWhenImplicitGrantAuthorizationUriIsNullThenThrowIllegalArgument
 			.build();
 	}
 
-	@Test(expected = IllegalArgumentException.class)
-	public void buildWhenImplicitGrantClientNameIsNullThenThrowIllegalArgumentException() {
-		ClientRegistration.withRegistrationId(REGISTRATION_ID)
-			.clientId(CLIENT_ID)
-			.authorizationGrantType(AuthorizationGrantType.IMPLICIT)
-			.redirectUriTemplate(REDIRECT_URI)
-			.scope(SCOPES.toArray(new String[0]))
-			.authorizationUri(AUTHORIZATION_URI)
-			.userInfoAuthenticationMethod(AuthenticationMethod.FORM)
-			.clientName(null)
-			.build();
+	@Test
+	public void buildWhenImplicitGrantClientNameNotProvidedThenDefaultToRegistrationId() {
+		ClientRegistration clientRegistration = ClientRegistration.withRegistrationId(REGISTRATION_ID)
+				.clientId(CLIENT_ID)
+				.authorizationGrantType(AuthorizationGrantType.IMPLICIT)
+				.redirectUriTemplate(REDIRECT_URI)
+				.scope(SCOPES.toArray(new String[0]))
+				.authorizationUri(AUTHORIZATION_URI)
+				.userInfoAuthenticationMethod(AuthenticationMethod.FORM)
+				.build();
+		assertThat(clientRegistration.getClientName()).isEqualTo(clientRegistration.getRegistrationId());
 	}
 
 	@Test
@@ -475,16 +458,14 @@ public void buildWhenClientCredentialsGrantClientSecretIsNullThenDefaultToEmpty(
 	}
 
 	@Test
-	public void buildWhenClientCredentialsGrantClientAuthenticationMethodIsNullThenThrowIllegalArgumentException() {
-		assertThatThrownBy(() ->
-				ClientRegistration.withRegistrationId(REGISTRATION_ID)
-						.clientId(CLIENT_ID)
-						.clientSecret(CLIENT_SECRET)
-						.clientAuthenticationMethod(null)
-						.authorizationGrantType(AuthorizationGrantType.CLIENT_CREDENTIALS)
-						.tokenUri(TOKEN_URI)
-						.build()
-		).isInstanceOf(IllegalArgumentException.class);
+	public void buildWhenClientCredentialsGrantClientAuthenticationMethodNotProvidedThenDefaultToBasic() {
+		ClientRegistration clientRegistration = ClientRegistration.withRegistrationId(REGISTRATION_ID)
+				.clientId(CLIENT_ID)
+				.clientSecret(CLIENT_SECRET)
+				.authorizationGrantType(AuthorizationGrantType.CLIENT_CREDENTIALS)
+				.tokenUri(TOKEN_URI)
+				.build();
+		assertThat(clientRegistration.getClientAuthenticationMethod()).isEqualTo(ClientAuthenticationMethod.BASIC);
 	}
 
 	@Test