From 231d6fa9961edf3c2d8ffd1f3dfef7be1ab0ce9d Mon Sep 17 00:00:00 2001 From: d036670 Date: Tue, 24 Oct 2023 17:42:13 +0200 Subject: [PATCH 1/6] fix client admin rest call with empty secret - alternative PR This PR simply uses PasswordValidatorUtil but set the password minLength to 1 which was the default because it was in code The minLength for clients should be 0 and this was the case before Same Tests as before, means, with Default Policy the empty secret is allowed, with the strictPolicy not, see testEmptyClientSecret --- .../identity/uaa/util/PasswordValidatorUtil.java | 4 ++-- ...oneAwareClientSecretPolicyValidatorTests.java | 2 ++ .../webapp/WEB-INF/spring/scim-endpoints.xml | 2 +- .../ClientAdminEndpointsIntegrationTests.java | 16 ++++++++++++++++ 4 files changed, 21 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/util/PasswordValidatorUtil.java b/server/src/main/java/org/cloudfoundry/identity/uaa/util/PasswordValidatorUtil.java index b10d045fd1b..3d1e83ba2fd 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/util/PasswordValidatorUtil.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/util/PasswordValidatorUtil.java @@ -51,8 +51,8 @@ public static PasswordValidator validator(GenericPasswordPolicy policy, MessageResolver messageResolver) { List rules = new ArrayList<>(); - //length is always a rule. We do not allow blank password - int minLength = Math.max(1, policy.getMinLength()); + //length is always a rule + int minLength = Math.max(0, policy.getMinLength()); int maxLength = policy.getMaxLength()>0 ? policy.getMaxLength() : Integer.MAX_VALUE; rules.add(new LengthRule(minLength, maxLength)); diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java index 19fc9c9db49..e3dc5939eb0 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java @@ -39,6 +39,8 @@ void setUp() { @Test void testEmptyClientSecret() { zone.getConfig().setClientSecretPolicy(defaultPolicy); + validator.validate(TEST_SECRET_1); + zone.getConfig().setClientSecretPolicy(strictPolicy); assertThrows(InvalidClientSecretException.class, () -> validator.validate(TEST_SECRET_1)); } diff --git a/uaa/src/main/webapp/WEB-INF/spring/scim-endpoints.xml b/uaa/src/main/webapp/WEB-INF/spring/scim-endpoints.xml index 7014e090cea..2144c21f004 100644 --- a/uaa/src/main/webapp/WEB-INF/spring/scim-endpoints.xml +++ b/uaa/src/main/webapp/WEB-INF/spring/scim-endpoints.xml @@ -35,7 +35,7 @@ - + diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/ClientAdminEndpointsIntegrationTests.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/ClientAdminEndpointsIntegrationTests.java index 744025d25e4..795bc02290d 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/ClientAdminEndpointsIntegrationTests.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/ClientAdminEndpointsIntegrationTests.java @@ -25,6 +25,7 @@ import org.cloudfoundry.identity.uaa.resources.SearchResults; import org.cloudfoundry.identity.uaa.test.TestAccountSetup; import org.cloudfoundry.identity.uaa.test.UaaTestAccounts; +import org.cloudfoundry.identity.uaa.util.UaaStringUtils; import org.cloudfoundry.identity.uaa.zone.ClientSecretPolicy; import org.cloudfoundry.identity.uaa.zone.IdentityZone; import org.cloudfoundry.identity.uaa.zone.IdentityZoneConfiguration; @@ -170,6 +171,21 @@ public void createClientWithSecondarySecret() { assertEquals(HttpStatus.CREATED, result.getStatusCode()); } + @Test + public void createClientWithEmptySecret() { + OAuth2AccessToken token = getClientCredentialsAccessToken("clients.admin"); + HttpHeaders headers = getAuthenticatedHeaders(token); + var client = new ClientDetailsCreation(); + client.setClientId(new RandomValueStringGenerator().generate()); + client.setClientSecret(UaaStringUtils.EMPTY_STRING); + client.setAuthorizedGrantTypes(List.of("password")); + + ResponseEntity result = serverRunning.getRestTemplate() + .exchange(serverRunning.getUrl("/oauth/clients"), HttpMethod.POST, + new HttpEntity<>(client, headers), Void.class); + assertEquals(HttpStatus.CREATED, result.getStatusCode()); + } + @Test public void testCreateClients() throws Exception { doCreateClients(); From cd7a6892300c268b2f2c6536607940d00fcae9b5 Mon Sep 17 00:00:00 2001 From: d036670 Date: Tue, 24 Oct 2023 17:44:54 +0200 Subject: [PATCH 2/6] split the tests --- .../uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java index e3dc5939eb0..a6fc0ca8f65 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java @@ -37,9 +37,13 @@ void setUp() { } @Test - void testEmptyClientSecret() { + void testEmptyClientSecretAllowed() { zone.getConfig().setClientSecretPolicy(defaultPolicy); validator.validate(TEST_SECRET_1); + } + + @Test + void testEmptyClientSecretForbidden() { zone.getConfig().setClientSecretPolicy(strictPolicy); assertThrows(InvalidClientSecretException.class, () -> validator.validate(TEST_SECRET_1)); } From a0d753bae74b9491608a4ccc5dbbbe86763946e3 Mon Sep 17 00:00:00 2001 From: d036670 Date: Tue, 24 Oct 2023 17:53:11 +0200 Subject: [PATCH 3/6] rename TEST_SECRET_1 to TEST_EMPTY_SECRET --- .../uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java index a6fc0ca8f65..cf4c49e6166 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java @@ -17,7 +17,7 @@ class ZoneAwareClientSecretPolicyValidatorTests { private ClientSecretPolicy defaultPolicy = new ClientSecretPolicy(0,255,0,0,0,0,6); private ClientSecretPolicy strictPolicy = new ClientSecretPolicy(6,10,1,1,1,1,6); - private static final String TEST_SECRET_1 = ""; + private static final String TEST_EMPTY_SECRET = ""; private static final String TEST_SECRET_2 = "testsecret"; private static final String TEST_SECRET_3 = "VFNTTDEgMB4GA1UEAxMXZnNzLnN0YWdlLmdlY29tcGFueIb3DQEBAQUADDwDG6wkBY" + "sJSqbSYpw0c76bUB1x5e46eiroRZdU2BEWiQJ9yxV95gGNsdLH1105iubzc9dbxavGIYM9s/+qJRf6WfwDU7VQsURCqIN8eUtnPU808" + @@ -39,13 +39,13 @@ void setUp() { @Test void testEmptyClientSecretAllowed() { zone.getConfig().setClientSecretPolicy(defaultPolicy); - validator.validate(TEST_SECRET_1); + validator.validate(TEST_EMPTY_SECRET); } @Test void testEmptyClientSecretForbidden() { zone.getConfig().setClientSecretPolicy(strictPolicy); - assertThrows(InvalidClientSecretException.class, () -> validator.validate(TEST_SECRET_1)); + assertThrows(InvalidClientSecretException.class, () -> validator.validate(TEST_EMPTY_SECRET)); } @Test From d99061a9ea6ae9f7fd9953e6592ae6caea960e85 Mon Sep 17 00:00:00 2001 From: Markus Strehle <11627201+strehle@users.noreply.github.com> Date: Tue, 24 Oct 2023 18:42:36 +0200 Subject: [PATCH 4/6] Revert the setting Documentation allows minLength with 0 even for passwords --- uaa/src/main/webapp/WEB-INF/spring/scim-endpoints.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/uaa/src/main/webapp/WEB-INF/spring/scim-endpoints.xml b/uaa/src/main/webapp/WEB-INF/spring/scim-endpoints.xml index 2144c21f004..7014e090cea 100644 --- a/uaa/src/main/webapp/WEB-INF/spring/scim-endpoints.xml +++ b/uaa/src/main/webapp/WEB-INF/spring/scim-endpoints.xml @@ -35,7 +35,7 @@ - + From 6b5b6fad32bc71819528c3b47d39719b7208944f Mon Sep 17 00:00:00 2001 From: d036670 Date: Tue, 24 Oct 2023 23:15:13 +0200 Subject: [PATCH 5/6] review --- .../cloudfoundry/identity/uaa/util/PasswordValidatorUtil.java | 3 +-- .../uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/util/PasswordValidatorUtil.java b/server/src/main/java/org/cloudfoundry/identity/uaa/util/PasswordValidatorUtil.java index 3d1e83ba2fd..2275b8c8eb0 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/util/PasswordValidatorUtil.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/util/PasswordValidatorUtil.java @@ -51,8 +51,7 @@ public static PasswordValidator validator(GenericPasswordPolicy policy, MessageResolver messageResolver) { List rules = new ArrayList<>(); - //length is always a rule - int minLength = Math.max(0, policy.getMinLength()); + int minLength = policy.getMinLength()>0 ? policy.getMinLength() : 0; int maxLength = policy.getMaxLength()>0 ? policy.getMaxLength() : Integer.MAX_VALUE; rules.add(new LengthRule(minLength, maxLength)); diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java index cf4c49e6166..77f96b2fbf3 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/zone/ZoneAwareClientSecretPolicyValidatorTests.java @@ -37,13 +37,13 @@ void setUp() { } @Test - void testEmptyClientSecretAllowed() { + void defaultPolicyAcceptsEmptySecret() { zone.getConfig().setClientSecretPolicy(defaultPolicy); validator.validate(TEST_EMPTY_SECRET); } @Test - void testEmptyClientSecretForbidden() { + void strictPolicyRejectsEmptySecret() { zone.getConfig().setClientSecretPolicy(strictPolicy); assertThrows(InvalidClientSecretException.class, () -> validator.validate(TEST_EMPTY_SECRET)); } From b30c52f7e1e7f0e2802c9871ed3c50a8567fedc7 Mon Sep 17 00:00:00 2001 From: d036670 Date: Tue, 24 Oct 2023 23:16:01 +0200 Subject: [PATCH 6/6] removed one test, and refactored another one --- .../validate/UaaPasswordPolicyValidatorTests.java | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/validate/UaaPasswordPolicyValidatorTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/validate/UaaPasswordPolicyValidatorTests.java index 0ae4bd052a4..2a97cf6f15c 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/validate/UaaPasswordPolicyValidatorTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/validate/UaaPasswordPolicyValidatorTests.java @@ -47,20 +47,11 @@ void setUp() { } @Test - void min_password_length_is_always_1_if_set_to_0() { - policy.setMinLength(0); + void min_password_length_is_1() { + policy.setMinLength(1); validatePassword("", "Password must be at least 1 characters in length."); - validatePassword(null, "Password must be at least 1 characters in length."); } - @Test - void min_password_length_is_always_1_if_not_set() { - policy.setMinLength(-1); - validatePassword("", "Password must be at least 1 characters in length."); - validatePassword(null, "Password must be at least 1 characters in length."); - } - - @Test void testValidateSuccess() { validatePassword("Password2&");