diff --git a/server-spi-private/src/main/java/org/keycloak/policy/HashAlgorithmPasswordPolicyProviderFactory.java b/server-spi-private/src/main/java/org/keycloak/policy/HashAlgorithmPasswordPolicyProviderFactory.java index c1c621897a..194059b0c6 100644 --- a/server-spi-private/src/main/java/org/keycloak/policy/HashAlgorithmPasswordPolicyProviderFactory.java +++ b/server-spi-private/src/main/java/org/keycloak/policy/HashAlgorithmPasswordPolicyProviderFactory.java @@ -91,7 +91,7 @@ public class HashAlgorithmPasswordPolicyProviderFactory implements PasswordPolic String providerId = value != null && value.length() > 0 ? value : PasswordPolicy.HASH_ALGORITHM_DEFAULT; PasswordHashProvider provider = session.getProvider(PasswordHashProvider.class, providerId); if (provider == null) { - throw new ModelException("Password hashing provider not found"); + throw new PasswordPolicyConfigException("Password hashing provider not found"); } return providerId; } diff --git a/server-spi-private/src/main/java/org/keycloak/policy/HistoryPasswordPolicyProvider.java b/server-spi-private/src/main/java/org/keycloak/policy/HistoryPasswordPolicyProvider.java index 004d54028e..fe7c9df9c3 100644 --- a/server-spi-private/src/main/java/org/keycloak/policy/HistoryPasswordPolicyProvider.java +++ b/server-spi-private/src/main/java/org/keycloak/policy/HistoryPasswordPolicyProvider.java @@ -73,7 +73,7 @@ public class HistoryPasswordPolicyProvider implements PasswordPolicyProvider { @Override public Object parseConfig(String value) { - return value != null ? Integer.parseInt(value) : HistoryPasswordPolicyProviderFactory.DEFAULT_VALUE; + return parseInteger(value, HistoryPasswordPolicyProviderFactory.DEFAULT_VALUE); } @Override diff --git a/server-spi-private/src/main/java/org/keycloak/policy/LengthPasswordPolicyProvider.java b/server-spi-private/src/main/java/org/keycloak/policy/LengthPasswordPolicyProvider.java index 5ba71fee45..82a70294df 100644 --- a/server-spi-private/src/main/java/org/keycloak/policy/LengthPasswordPolicyProvider.java +++ b/server-spi-private/src/main/java/org/keycloak/policy/LengthPasswordPolicyProvider.java @@ -47,7 +47,7 @@ public class LengthPasswordPolicyProvider implements PasswordPolicyProvider { @Override public Object parseConfig(String value) { - return value != null ? Integer.parseInt(value) : 8; + return parseInteger(value, 8); } @Override diff --git a/server-spi-private/src/main/java/org/keycloak/policy/LowerCasePasswordPolicyProvider.java b/server-spi-private/src/main/java/org/keycloak/policy/LowerCasePasswordPolicyProvider.java index f080d00594..b1fb3f46a2 100644 --- a/server-spi-private/src/main/java/org/keycloak/policy/LowerCasePasswordPolicyProvider.java +++ b/server-spi-private/src/main/java/org/keycloak/policy/LowerCasePasswordPolicyProvider.java @@ -53,7 +53,7 @@ public class LowerCasePasswordPolicyProvider implements PasswordPolicyProvider { @Override public Object parseConfig(String value) { - return value != null ? Integer.parseInt(value) : 1; + return parseInteger(value, 1); } @Override diff --git a/server-spi-private/src/main/java/org/keycloak/policy/RegexPatternsPasswordPolicyProvider.java b/server-spi-private/src/main/java/org/keycloak/policy/RegexPatternsPasswordPolicyProvider.java index 52c83b86e8..cfcfbc538c 100644 --- a/server-spi-private/src/main/java/org/keycloak/policy/RegexPatternsPasswordPolicyProvider.java +++ b/server-spi-private/src/main/java/org/keycloak/policy/RegexPatternsPasswordPolicyProvider.java @@ -23,6 +23,7 @@ import org.keycloak.models.UserModel; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.regex.PatternSyntaxException; /** * @author Stian Thorgersen @@ -55,9 +56,13 @@ public class RegexPatternsPasswordPolicyProvider implements PasswordPolicyProvid @Override public Object parseConfig(String value) { if (value == null) { - throw new IllegalArgumentException("Config required"); + throw new PasswordPolicyConfigException("Config required"); + } + try { + return Pattern.compile(value); + } catch (PatternSyntaxException e) { + throw new PasswordPolicyConfigException("Not a valid regular expression"); } - return Pattern.compile(value); } @Override diff --git a/server-spi-private/src/main/java/org/keycloak/policy/SpecialCharsPasswordPolicyProvider.java b/server-spi-private/src/main/java/org/keycloak/policy/SpecialCharsPasswordPolicyProvider.java index fa851372a9..7d2cfe82e0 100644 --- a/server-spi-private/src/main/java/org/keycloak/policy/SpecialCharsPasswordPolicyProvider.java +++ b/server-spi-private/src/main/java/org/keycloak/policy/SpecialCharsPasswordPolicyProvider.java @@ -53,7 +53,7 @@ public class SpecialCharsPasswordPolicyProvider implements PasswordPolicyProvide @Override public Object parseConfig(String value) { - return value != null ? Integer.parseInt(value) : 1; + return parseInteger(value, 1); } @Override diff --git a/server-spi-private/src/main/java/org/keycloak/policy/UpperCasePasswordPolicyProvider.java b/server-spi-private/src/main/java/org/keycloak/policy/UpperCasePasswordPolicyProvider.java index 16ac1eff58..f34f92f71a 100644 --- a/server-spi-private/src/main/java/org/keycloak/policy/UpperCasePasswordPolicyProvider.java +++ b/server-spi-private/src/main/java/org/keycloak/policy/UpperCasePasswordPolicyProvider.java @@ -53,7 +53,7 @@ public class UpperCasePasswordPolicyProvider implements PasswordPolicyProvider { @Override public Object parseConfig(String value) { - return value != null ? Integer.parseInt(value) : 1; + return parseInteger(value, 1); } @Override diff --git a/server-spi-private/src/main/java/org/keycloak/models/ModelDuplicateException.java b/server-spi/src/main/java/org/keycloak/models/ModelDuplicateException.java similarity index 100% rename from server-spi-private/src/main/java/org/keycloak/models/ModelDuplicateException.java rename to server-spi/src/main/java/org/keycloak/models/ModelDuplicateException.java diff --git a/server-spi-private/src/main/java/org/keycloak/models/ModelException.java b/server-spi/src/main/java/org/keycloak/models/ModelException.java similarity index 100% rename from server-spi-private/src/main/java/org/keycloak/models/ModelException.java rename to server-spi/src/main/java/org/keycloak/models/ModelException.java diff --git a/server-spi/src/main/java/org/keycloak/models/PasswordPolicy.java b/server-spi/src/main/java/org/keycloak/models/PasswordPolicy.java index 8833c8a33c..f3367af365 100755 --- a/server-spi/src/main/java/org/keycloak/models/PasswordPolicy.java +++ b/server-spi/src/main/java/org/keycloak/models/PasswordPolicy.java @@ -17,6 +17,7 @@ package org.keycloak.models; +import org.keycloak.policy.PasswordPolicyConfigException; import org.keycloak.policy.PasswordPolicyProvider; import java.io.Serializable; @@ -68,10 +69,17 @@ public class PasswordPolicy implements Serializable { PasswordPolicyProvider provider = session.getProvider(PasswordPolicyProvider.class, key); if (provider == null) { - throw new IllegalArgumentException("Unsupported policy"); + throw new PasswordPolicyConfigException("Password policy not found"); } - policyConfig.put(key, provider.parseConfig(config)); + Object o; + try { + o = provider.parseConfig(config); + } catch (PasswordPolicyConfigException e) { + throw new ModelException("Invalid config for " + key + ": " + e.getMessage()); + } + + policyConfig.put(key, o); } } diff --git a/server-spi/src/main/java/org/keycloak/policy/PasswordPolicyConfigException.java b/server-spi/src/main/java/org/keycloak/policy/PasswordPolicyConfigException.java new file mode 100644 index 0000000000..8d9a879e6f --- /dev/null +++ b/server-spi/src/main/java/org/keycloak/policy/PasswordPolicyConfigException.java @@ -0,0 +1,14 @@ +package org.keycloak.policy; + +import org.keycloak.models.ModelException; + +/** + * Created by st on 23/05/17. + */ +public class PasswordPolicyConfigException extends ModelException { + + public PasswordPolicyConfigException(String message) { + super(message); + } + +} diff --git a/server-spi/src/main/java/org/keycloak/policy/PasswordPolicyProvider.java b/server-spi/src/main/java/org/keycloak/policy/PasswordPolicyProvider.java index 3f7c3ea869..5d54251dfd 100644 --- a/server-spi/src/main/java/org/keycloak/policy/PasswordPolicyProvider.java +++ b/server-spi/src/main/java/org/keycloak/policy/PasswordPolicyProvider.java @@ -33,4 +33,12 @@ public interface PasswordPolicyProvider extends Provider { PolicyError validate(String user, String password); Object parseConfig(String value); + default Integer parseInteger(String value, Integer defaultValue) { + try { + return value != null ? Integer.parseInt(value) : defaultValue; + } catch (NumberFormatException e) { + throw new PasswordPolicyConfigException("Not a valid number"); + } + } + } diff --git a/services/src/main/java/org/keycloak/services/resources/admin/RealmAdminResource.java b/services/src/main/java/org/keycloak/services/resources/admin/RealmAdminResource.java index 55587dae76..f1469d0b5c 100644 --- a/services/src/main/java/org/keycloak/services/resources/admin/RealmAdminResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/RealmAdminResource.java @@ -328,8 +328,6 @@ public class RealmAdminResource { } return Response.noContent().build(); - } catch (PatternSyntaxException e) { - return ErrorResponse.error("Specified regex pattern(s) is invalid.", Response.Status.BAD_REQUEST); } catch (ModelDuplicateException e) { return ErrorResponse.exists("Realm with same name exists"); } catch (ModelException e) { diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/PasswordHashingTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/PasswordHashingTest.java index caaadb9cdb..0784f01b45 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/PasswordHashingTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/PasswordHashingTest.java @@ -97,7 +97,7 @@ public class PasswordHashingTest extends AbstractTestRealmKeycloakTest { fail("Expected error"); } catch (BadRequestException e) { ErrorRepresentation error = e.getResponse().readEntity(ErrorRepresentation.class); - assertEquals("Password hashing provider not found", error.getErrorMessage()); + assertEquals("Invalid config for hashAlgorithm: Password hashing provider not found", error.getErrorMessage()); } } diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/model/PasswordPolicyTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/model/PasswordPolicyTest.java index e21fb0b0fa..93ee3683f1 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/model/PasswordPolicyTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/model/PasswordPolicyTest.java @@ -20,12 +20,14 @@ package org.keycloak.testsuite.model; import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import org.keycloak.models.ModelException; import org.keycloak.models.PasswordPolicy; import org.keycloak.models.RealmModel; import org.keycloak.policy.PasswordPolicyManagerProvider; import java.util.regex.PatternSyntaxException; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; /** @@ -123,7 +125,8 @@ public class PasswordPolicyTest extends AbstractModelTest { try { realmModel.setPasswordPolicy(PasswordPolicy.parse(session, "noSuchPolicy")); Assert.fail("Expected exception"); - } catch (IllegalArgumentException e) { + } catch (ModelException e) { + assertEquals("Password policy not found", e.getMessage()); } } @@ -133,22 +136,22 @@ public class PasswordPolicyTest extends AbstractModelTest { try { realmModel.setPasswordPolicy(PasswordPolicy.parse(session, "regexPattern")); fail("Expected NullPointerException: Regex Pattern cannot be null."); - } catch (IllegalArgumentException e) { - // Expected NPE as regex pattern is null. + } catch (ModelException e) { + assertEquals("Invalid config for regexPattern: Config required", e.getMessage()); } try { realmModel.setPasswordPolicy(PasswordPolicy.parse(session, "regexPattern(*)")); fail("Expected PatternSyntaxException: Regex Pattern cannot be null."); - } catch (PatternSyntaxException e) { - // Expected PSE as regex pattern(or any of its token) is not quantifiable. + } catch (ModelException e) { + assertEquals("Invalid config for regexPattern: Not a valid regular expression", e.getMessage()); } try { realmModel.setPasswordPolicy(PasswordPolicy.parse(session, "regexPattern(*,**)")); fail("Expected PatternSyntaxException: Regex Pattern cannot be null."); - } catch (PatternSyntaxException e) { - // Expected PSE as regex pattern(or any of its token) is not quantifiable. + } catch (ModelException e) { + assertEquals("Invalid config for regexPattern: Not a valid regular expression", e.getMessage()); } //Fails to match one of the regex pattern