From c9e1e00b95d9f9f7a6f11c7e0c341781ed234c7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nemanja=20Hir=C5=A1l?= Date: Thu, 11 Nov 2021 15:29:16 +0100 Subject: [PATCH] KEYCLOAK-19773 BFD and Direct Grant - inconsistent number of failures Do not "failure" on temporary or permanently locked users, but "forceChallenge" Failure increments number of failures, and forceChallenge doesn't Test cases cover: 1. Already disabled users 2. Temporarily disabled users by BFD 3. Permanently disabled users by BFD --- .../directgrant/ValidateUsername.java | 4 +- .../testsuite/forms/BruteForceTest.java | 100 +++++++++++++++++- 2 files changed, 100 insertions(+), 4 deletions(-) diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/directgrant/ValidateUsername.java b/services/src/main/java/org/keycloak/authentication/authenticators/directgrant/ValidateUsername.java index 34dc1360da..b14ffe0437 100755 --- a/services/src/main/java/org/keycloak/authentication/authenticators/directgrant/ValidateUsername.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/directgrant/ValidateUsername.java @@ -82,7 +82,7 @@ public class ValidateUsername extends AbstractDirectGrantAuthenticator { context.getEvent().user(user); context.getEvent().error(bruteForceError); Response challengeResponse = errorResponse(Response.Status.UNAUTHORIZED.getStatusCode(), "invalid_grant", "Invalid user credentials"); - context.failure(AuthenticationFlowError.INVALID_USER, challengeResponse); + context.forceChallenge(challengeResponse); return; } @@ -90,7 +90,7 @@ public class ValidateUsername extends AbstractDirectGrantAuthenticator { context.getEvent().user(user); context.getEvent().error(Errors.USER_DISABLED); Response challengeResponse = errorResponse(Response.Status.BAD_REQUEST.getStatusCode(), "invalid_grant", "Account disabled"); - context.failure(AuthenticationFlowError.INVALID_USER, challengeResponse); + context.forceChallenge(challengeResponse); return; } context.setUser(user); 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 274d15d62b..510501b9b9 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 @@ -16,6 +16,7 @@ */ package org.keycloak.testsuite.forms; +import org.hamcrest.MatcherAssert; import org.jboss.arquillian.graphene.page.Page; import org.junit.After; import org.junit.Assert; @@ -98,13 +99,15 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { private int lifespan; + private static final Integer failureFactor= 2; + @Override public void configureTestRealm(RealmRepresentation testRealm) { UserRepresentation user = RealmRepUtil.findUser(testRealm, "test-user@localhost"); UserBuilder.edit(user).totpSecret("totpSecret"); testRealm.setBruteForceProtected(true); - testRealm.setFailureFactor(2); + testRealm.setFailureFactor(failureFactor); testRealm.setMaxDeltaTimeSeconds(20); testRealm.setMaxFailureWaitSeconds(100); testRealm.setWaitIncrementSeconds(5); @@ -122,7 +125,7 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { clearUserFailures(); clearAllUserFailures(); RealmRepresentation realm = adminClient.realm("test").toRepresentation(); - realm.setFailureFactor(2); + realm.setFailureFactor(failureFactor); realm.setMaxDeltaTimeSeconds(20); realm.setMaxFailureWaitSeconds(100); realm.setWaitIncrementSeconds(5); @@ -316,6 +319,73 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { } + @Test + public void testNumberOfFailuresForDisabledUsersWithPasswordGrantType() throws Exception { + try { + UserRepresentation user = adminClient.realm("test").users().search("test-user@localhost", 0, 1).get(0); + assertUserNumberOfFailures(user.getId(), 0); + user.setEnabled(false); + updateUser(user); + + OAuthClient.AccessTokenResponse response = getTestToken("invalid", "invalid"); + Assert.assertNull(response.getAccessToken()); + Assert.assertEquals(response.getError(), "invalid_grant"); + Assert.assertEquals(response.getErrorDescription(), "Account disabled"); + events.clear(); + + assertUserNumberOfFailures(user.getId(), 0); + } finally { + UserRepresentation user = adminClient.realm("test").users().search("test-user@localhost", 0, 1).get(0); + user.setEnabled(true); + updateUser(user); + events.clear(); + } + } + + @Test + public void testNumberOfFailuresForTemporaryDisabledUsersWithPasswordGrantType() throws Exception { + UserRepresentation user = adminClient.realm("test").users().search("test-user@localhost", 0, 1).get(0); + + // Lock user (temporarily) and make sure the number of failures matches failure factor + lockUserWithPasswordGrant(); + assertUserNumberOfFailures(user.getId(), failureFactor); + + // Try to login with invalid credentials and make sure the number of failures doesn't change during temporary lockout + sendInvalidPasswordPasswordGrant(); + assertUserNumberOfFailures(user.getId(), failureFactor); + + events.clear(); + } + + @Test + public void testNumberOfFailuresForPermanentlyDisabledUsersWithPasswordGrantType() throws Exception { + RealmRepresentation realm = testRealm().toRepresentation(); + try { + // Set permanent lockout for the test + realm.setPermanentLockout(true); + testRealm().update(realm); + + UserRepresentation user = adminClient.realm("test").users().search("test-user@localhost", 0, 1).get(0); + + // Lock user (permanently) and make sure the number of failures matches failure factor + lockUserWithPasswordGrant(); + assertUserNumberOfFailures(user.getId(), failureFactor); + assertUserDisabledReason(BruteForceProtector.DISABLED_BY_PERMANENT_LOCKOUT); + + // Try to login with invalid credentials and make sure the number of failures doesn't change during temporary lockout + sendInvalidPasswordPasswordGrant(); + assertUserNumberOfFailures(user.getId(), failureFactor); + + events.clear(); + } finally { + realm.setPermanentLockout(false); + testRealm().update(realm); + UserRepresentation user = adminClient.realm("test").users().search("test-user@localhost", 0, 1).get(0); + user.setEnabled(true); + updateUser(user); + } + } + @Test public void testBrowserInvalidPassword() throws Exception { loginSuccess(); @@ -725,4 +795,30 @@ public class BruteForceTest extends AbstractTestRealmKeycloakTest { .firstAttribute(UserModel.DISABLED_REASON); assertEquals(expected, actual); } + + private void assertUserNumberOfFailures(String userId, Integer numberOfFailures) { + Map userAttackInfo = adminClient.realm("test").attackDetection().bruteForceUserStatus(userId); + MatcherAssert.assertThat((Integer) userAttackInfo.get("numFailures"), is(numberOfFailures)); + } + + private void sendInvalidPasswordPasswordGrant() throws Exception { + String totpSecret = totp.generateTOTP("totpSecret"); + OAuthClient.AccessTokenResponse response = getTestToken("invalid", totpSecret); + Assert.assertNull(response.getAccessToken()); + Assert.assertEquals(response.getError(), "invalid_grant"); + Assert.assertEquals(response.getErrorDescription(), "Invalid user credentials"); + events.clear(); + } + + private void lockUserWithPasswordGrant() throws Exception { + String totpSecret = totp.generateTOTP("totpSecret"); + OAuthClient.AccessTokenResponse response = getTestToken("password", totpSecret); + Assert.assertNotNull(response.getAccessToken()); + Assert.assertNull(response.getError()); + events.clear(); + + for (int i = 0; i < failureFactor; ++i) { + sendInvalidPasswordPasswordGrant(); + } + } }