From 828f9f7916c8d32d1ea7b6bc48fe63468bfa9ab9 Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Wed, 18 Jun 2025 03:34:56 -0300 Subject: [PATCH] Mark user as disabled if reaching max login failures and permanent lockout is enabled Closes #40159 Signed-off-by: Pedro Igor --- .../idm/AbstractUserRepresentation.java | 9 +++ .../idm/UserRepresentation.java | 9 --- .../models/utils/ModelToRepresentation.java | 8 ++- .../userprofile/DefaultUserProfile.java | 13 ++++ .../managers/DefaultBruteForceProtector.java | 6 +- .../testsuite/forms/BruteForceTest.java | 59 +++++++++++++++++++ 6 files changed, 92 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/org/keycloak/representations/idm/AbstractUserRepresentation.java b/core/src/main/java/org/keycloak/representations/idm/AbstractUserRepresentation.java index c4fe9cb7a76..b17c72f360b 100644 --- a/core/src/main/java/org/keycloak/representations/idm/AbstractUserRepresentation.java +++ b/core/src/main/java/org/keycloak/representations/idm/AbstractUserRepresentation.java @@ -45,6 +45,7 @@ public abstract class AbstractUserRepresentation { @JsonDeserialize(using = StringListMapDeserializer.class) protected Map> attributes; private UserProfileMetadata userProfileMetadata; + protected Boolean enabled; public String getId() { @@ -154,4 +155,12 @@ public abstract class AbstractUserRepresentation { public UserProfileMetadata getUserProfileMetadata() { return userProfileMetadata; } + + public Boolean isEnabled() { + return enabled; + } + + public void setEnabled(Boolean enabled) { + this.enabled = enabled; + } } diff --git a/core/src/main/java/org/keycloak/representations/idm/UserRepresentation.java b/core/src/main/java/org/keycloak/representations/idm/UserRepresentation.java index 69c1ee06133..889301b77fc 100755 --- a/core/src/main/java/org/keycloak/representations/idm/UserRepresentation.java +++ b/core/src/main/java/org/keycloak/representations/idm/UserRepresentation.java @@ -30,7 +30,6 @@ public class UserRepresentation extends AbstractUserRepresentation{ protected String self; // link protected String origin; protected Long createdTimestamp; - protected Boolean enabled; protected Boolean totp; protected String federationLink; protected String serviceAccountClientId; // For rep, it points to clientId (not DB ID) @@ -104,14 +103,6 @@ public class UserRepresentation extends AbstractUserRepresentation{ this.createdTimestamp = createdTimestamp; } - public Boolean isEnabled() { - return enabled; - } - - public void setEnabled(Boolean enabled) { - this.enabled = enabled; - } - @Deprecated public Boolean isTotp() { return totp; diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/ModelToRepresentation.java b/server-spi-private/src/main/java/org/keycloak/models/utils/ModelToRepresentation.java index ddebf234b4b..5a9a5d3917a 100755 --- a/server-spi-private/src/main/java/org/keycloak/models/utils/ModelToRepresentation.java +++ b/server-spi-private/src/main/java/org/keycloak/models/utils/ModelToRepresentation.java @@ -242,7 +242,9 @@ public class ModelToRepresentation { if (setUserAttributes) { rep.setEmail(user.getEmail()); } - rep.setEnabled(user.isEnabled()); + if (rep.isEnabled() == null) { + rep.setEnabled(user.isEnabled()); + } rep.setEmailVerified(user.isEmailVerified()); rep.setTotp(user.credentialManager().isConfiguredFor(OTPCredentialModel.TYPE)); rep.setDisableableCredentialTypes(user.credentialManager() @@ -290,7 +292,9 @@ public class ModelToRepresentation { if (setUserAttributes) { rep.setEmail(user.getEmail()); } - rep.setEnabled(user.isEnabled()); + if (rep.isEnabled() == null) { + rep.setEnabled(user.isEnabled()); + } rep.setEmailVerified(user.isEmailVerified()); rep.setFederationLink(user.getFederationLink()); addAttributeToBriefRep(user, rep, IS_TEMP_ADMIN_ATTR_NAME); diff --git a/server-spi-private/src/main/java/org/keycloak/userprofile/DefaultUserProfile.java b/server-spi-private/src/main/java/org/keycloak/userprofile/DefaultUserProfile.java index 451eac54efb..bb1a88af204 100644 --- a/server-spi-private/src/main/java/org/keycloak/userprofile/DefaultUserProfile.java +++ b/server-spi-private/src/main/java/org/keycloak/userprofile/DefaultUserProfile.java @@ -41,6 +41,7 @@ import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; import org.keycloak.models.utils.ModelToRepresentation; import org.keycloak.representations.idm.AbstractUserRepresentation; +import org.keycloak.services.managers.BruteForceProtector; import org.keycloak.storage.ReadOnlyException; import org.keycloak.utils.StringUtil; @@ -250,6 +251,18 @@ public final class DefaultUserProfile implements UserProfile { setAttributeIfExists(user, DISABLED_REASON, attributesRep); setAttributeIfExists(user, IS_TEMP_ADMIN_ATTR_NAME, attributesRep); + RealmModel realm = session.getContext().getRealm(); + + if (realm.isBruteForceProtected() && !attributesRep.containsKey(DISABLED_REASON)) { + BruteForceProtector protector = session.getProvider(BruteForceProtector.class); + boolean lockedOut = protector.isPermanentlyLockedOut(session, realm, user); + + if (lockedOut) { + rep.setEnabled(false); + attributesRep.put(DISABLED_REASON, List.of(BruteForceProtector.DISABLED_BY_PERMANENT_LOCKOUT)); + } + } + rep.setId(user.getId()); rep.setAttributes(attributesRep.isEmpty() ? null : attributesRep); rep.setUserProfileMetadata(createUserProfileMetadata(session, this)); diff --git a/services/src/main/java/org/keycloak/services/managers/DefaultBruteForceProtector.java b/services/src/main/java/org/keycloak/services/managers/DefaultBruteForceProtector.java index 5049c1bea8e..337d7bc6a95 100644 --- a/services/src/main/java/org/keycloak/services/managers/DefaultBruteForceProtector.java +++ b/services/src/main/java/org/keycloak/services/managers/DefaultBruteForceProtector.java @@ -250,8 +250,12 @@ public class DefaultBruteForceProtector implements BruteForceProtector { @Override public void cleanUpPermanentLockout(KeycloakSession session, RealmModel realm, UserModel user) { - if (DISABLED_BY_PERMANENT_LOCKOUT.equals(user.getFirstAttribute(DISABLED_REASON))) { + if (DISABLED_BY_PERMANENT_LOCKOUT.equals(user.getFirstAttribute(DISABLED_REASON)) || isPermanentlyLockedOut(session, realm, user)) { user.removeAttribute(DISABLED_REASON); + + if (!isTemporarilyDisabled(session, realm, user)) { + session.loginFailures().removeUserLoginFailure(realm, user.getId()); + } } } 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 65e14f093bd..d5ae9a32a7f 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 @@ -23,6 +23,7 @@ import org.junit.Assert; import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.keycloak.admin.client.resource.UsersResource; import org.keycloak.events.Details; import org.keycloak.events.Errors; import org.keycloak.events.EventType; @@ -63,6 +64,7 @@ import static org.hamcrest.CoreMatchers.is; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; /** @@ -553,6 +555,57 @@ public class BruteForceTest extends AbstractChangeImportedUserPasswordsTest { assertEquals(Boolean.TRUE, testRealm().attackDetection().bruteForceUserStatus(userId).get("disabled")); } + @Test + public void testUserDisabledAfterSwitchFromMixedToPermanentLockout() throws Exception { + UsersResource users = testRealm().users(); + UserRepresentation user = users.search("test-user@localhost", null, null, null, 0, 1).get(0); + + // temporarily lockout + loginInvalidPassword(); + loginInvalidPassword(); + expectTemporarilyDisabled(); + assertUserNumberOfFailures(user.getId(), 2); + // user is still enabled during temporary lockout + assertTrue(users.get(user.getId()).toRepresentation().isEnabled()); + assertTrue(users.search("test-user@localhost", true).get(0).isEnabled()); + assertEquals(Boolean.TRUE, testRealm().attackDetection().bruteForceUserStatus(user.getId()).get("disabled")); + + RealmRepresentation realm = testRealm().toRepresentation(); + + try { + // switch to permanent lockout before waiting to successful login + realm.setPermanentLockout(true); + testRealm().update(realm); + + // expires the temporary lockout + this.setTimeOffset(60); + + // after switching to permanent lockout the user status is disabled because there are login failures + // the user did not try to successfully authenticate yet to clear the login failures + user = users.get(user.getId()).toRepresentation(); + assertFalse(user.isEnabled()); + assertFalse(users.search("test-user@localhost", true).get(0).isEnabled()); + assertEquals(Boolean.TRUE, testRealm().attackDetection().bruteForceUserStatus(user.getId()).get("disabled")); + expectPermanentlyDisabled(); + + // attempt to re-enable the user and login successfully + user.setEnabled(true); + users.get(user.getId()).update(user); + user = users.get(user.getId()).toRepresentation(); + assertTrue(user.isEnabled()); + assertTrue(users.search("test-user@localhost", true).get(0).isEnabled()); + Map userAttackInfo = testRealm().attackDetection().bruteForceUserStatus(user.getId()); + assertEquals(Boolean.FALSE, userAttackInfo.get("disabled")); + assertThat((Integer) userAttackInfo.get("numFailures"), is(0)); + // login failures should be removed after re-enabling the user and the user able to authenticate + loginSuccess(); + } finally { + resetTimeOffset(); + realm.setPermanentLockout(false); + testRealm().update(realm); + } + } + @Test public void testBrowserMissingPassword() throws Exception { loginSuccess(); @@ -977,6 +1030,12 @@ public class BruteForceTest extends AbstractChangeImportedUserPasswordsTest { .detail(Details.USERNAME, username) .removeDetail(Details.CONSENT); event.assertEvent(); + UserRepresentation user = testRealm().users().search(username, true).get(0); + user = testRealm().users().get(user.getId()).toRepresentation(); + List disabledReason = user.getAttributes().get(UserModel.DISABLED_REASON); + assertNotNull(disabledReason); + assertEquals(1, disabledReason.size()); + assertEquals(BruteForceProtector.DISABLED_BY_PERMANENT_LOCKOUT, disabledReason.get(0)); } public void loginSuccess() {