From 811cd3a04ae49f5d7173f3e2f6129187e38dc2c2 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Tue, 19 Dec 2017 18:02:39 -0200 Subject: [PATCH] KEYCLOAK-6011 --- .../AbstractUsernameFormAuthenticator.java | 31 ++++++++++++------- .../admin/AttackDetectionResourceTest.java | 2 +- .../testsuite/forms/BruteForceTest.java | 17 ++++++++-- 3 files changed, 35 insertions(+), 15 deletions(-) diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/browser/AbstractUsernameFormAuthenticator.java b/services/src/main/java/org/keycloak/authentication/authenticators/browser/AbstractUsernameFormAuthenticator.java index 6b6da564f9..3448826a29 100755 --- a/services/src/main/java/org/keycloak/authentication/authenticators/browser/AbstractUsernameFormAuthenticator.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/browser/AbstractUsernameFormAuthenticator.java @@ -22,7 +22,6 @@ import org.keycloak.authentication.AbstractFormAuthenticator; import org.keycloak.authentication.AuthenticationFlowContext; import org.keycloak.authentication.AuthenticationFlowError; import org.keycloak.credential.CredentialInput; -import org.keycloak.credential.CredentialModel; import org.keycloak.credential.hash.PasswordHashProvider; import org.keycloak.events.Details; import org.keycloak.events.Errors; @@ -130,17 +129,7 @@ public abstract class AbstractUsernameFormAuthenticator extends AbstractFormAuth context.forceChallenge(challengeResponse); return false; } - if (context.getRealm().isBruteForceProtected()) { - if (context.getProtector().isTemporarilyDisabled(context.getSession(), context.getRealm(), user)) { - context.getEvent().user(user); - context.getEvent().error(Errors.USER_TEMPORARILY_DISABLED); - Response challengeResponse = temporarilyDisabledUser(context); - // this is not a failure so don't call failureChallenge. - //context.failureChallenge(AuthenticationFlowError.USER_TEMPORARILY_DISABLED, challengeResponse); - context.forceChallenge(challengeResponse); - return false; - } - } + if (isTemporarilyDisabledByBruteForce(context, user)) return false; return true; } @@ -203,6 +192,9 @@ public abstract class AbstractUsernameFormAuthenticator extends AbstractFormAuth List credentials = new LinkedList<>(); String password = inputData.getFirst(CredentialRepresentation.PASSWORD); credentials.add(UserCredentialModel.password(password)); + + if (isTemporarilyDisabledByBruteForce(context, user)) return false; + if (password != null && !password.isEmpty() && context.getSession().userCredentialManager().isValid(context.getRealm(), user, credentials)) { return true; } else { @@ -215,4 +207,19 @@ public abstract class AbstractUsernameFormAuthenticator extends AbstractFormAuth } } + private boolean isTemporarilyDisabledByBruteForce(AuthenticationFlowContext context, UserModel user) { + if (context.getRealm().isBruteForceProtected()) { + if (context.getProtector().isTemporarilyDisabled(context.getSession(), context.getRealm(), user)) { + context.getEvent().user(user); + context.getEvent().error(Errors.USER_TEMPORARILY_DISABLED); + Response challengeResponse = temporarilyDisabledUser(context); + // this is not a failure so don't call failureChallenge. + //context.failureChallenge(AuthenticationFlowError.USER_TEMPORARILY_DISABLED, challengeResponse); + context.forceChallenge(challengeResponse); + return true; + } + } + return false; + } + } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/AttackDetectionResourceTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/AttackDetectionResourceTest.java index 531b55696c..e65fdc53bc 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/AttackDetectionResourceTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/AttackDetectionResourceTest.java @@ -63,7 +63,7 @@ public class AttackDetectionResourceTest extends AbstractAdminTest { oauthClient.doLogin("test-user2", "invalid"); oauthClient.doLogin("nosuchuser", "invalid"); - assertBruteForce(detection.bruteForceUserStatus(findUser("test-user@localhost").getId()), 3, true, true); + assertBruteForce(detection.bruteForceUserStatus(findUser("test-user@localhost").getId()), 2, true, true); assertBruteForce(detection.bruteForceUserStatus(findUser("test-user2").getId()), 2, true, true); assertBruteForce(detection.bruteForceUserStatus("nosuchuser"), 0, false, false); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BruteForceTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BruteForceTest.java index 7b65a0c528..d589f2a228 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BruteForceTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BruteForceTest.java @@ -311,11 +311,13 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { loginInvalidPassword(); loginInvalidPassword(); expectTemporarilyDisabled(); + expectTemporarilyDisabled("test-user@localhost", null, "invalid"); clearUserFailures(); loginSuccess(); loginInvalidPassword(); loginInvalidPassword(); expectTemporarilyDisabled(); + expectTemporarilyDisabled("test-user@localhost", null, "invalid"); clearAllUserFailures(); loginSuccess(); } @@ -326,6 +328,8 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { loginInvalidPassword(); loginInvalidPassword(); expectTemporarilyDisabled(); + expectTemporarilyDisabled("test-user@localhost", null, "invalid"); + // KEYCLOAK-5420 // Test to make sure that temporarily disabled doesn't increment failure count testingClient.testing().setTimeOffset(Collections.singletonMap("offset", String.valueOf(6))); @@ -343,6 +347,7 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { loginInvalidPassword("test-User@localhost"); loginInvalidPassword("Test-user@localhost"); expectTemporarilyDisabled(); + expectTemporarilyDisabled("test-user@localhost", null, "invalid"); clearAllUserFailures(); } @@ -363,6 +368,7 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { loginMissingPassword(); loginMissingPassword(); expectTemporarilyDisabled(); + expectTemporarilyDisabled("test-user@localhost", null, "invalid"); clearUserFailures(); loginSuccess(); } @@ -373,6 +379,7 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { loginWithTotpFailure(); loginWithTotpFailure(); expectTemporarilyDisabled(); + expectTemporarilyDisabled("test-user@localhost", null, "invalid"); clearUserFailures(); loginSuccess(); } @@ -383,6 +390,7 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { loginWithMissingTotp(); loginWithMissingTotp(); expectTemporarilyDisabled(); + expectTemporarilyDisabled("test-user@localhost", null, "invalid"); clearUserFailures(); loginSuccess(); } @@ -454,6 +462,7 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { loginInvalidPassword(); loginInvalidPassword(); expectTemporarilyDisabled(); + expectTemporarilyDisabled("test-user@localhost", null, "invalid"); loginPage.resetPassword(); resetPasswordPage.assertCurrent(); @@ -468,12 +477,16 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { } public void expectTemporarilyDisabled() throws Exception { - expectTemporarilyDisabled("test-user@localhost", null); + expectTemporarilyDisabled("test-user@localhost", null, "password"); } public void expectTemporarilyDisabled(String username, String userId) throws Exception { + expectTemporarilyDisabled(username, userId, "password"); + } + + public void expectTemporarilyDisabled(String username, String userId, String password) throws Exception { loginPage.open(); - loginPage.login(username, "password"); + loginPage.login(username, password); loginPage.assertCurrent(); String src = driver.getPageSource();