diff --git a/server-spi-private/src/main/java/org/keycloak/services/managers/BruteForceProtector.java b/server-spi-private/src/main/java/org/keycloak/services/managers/BruteForceProtector.java index 02be48d5dfc0..8b296eb21e26 100755 --- a/server-spi-private/src/main/java/org/keycloak/services/managers/BruteForceProtector.java +++ b/server-spi-private/src/main/java/org/keycloak/services/managers/BruteForceProtector.java @@ -28,9 +28,21 @@ * @version $Revision: 1 $ */ public interface BruteForceProtector extends Provider { + String DISABLED_BY_PERMANENT_LOCKOUT = "permanentLockout"; + void failedLogin(RealmModel realm, UserModel user, ClientConnection clientConnection); void successfulLogin(RealmModel realm, UserModel user, ClientConnection clientConnection); boolean isTemporarilyDisabled(KeycloakSession session, RealmModel realm, UserModel user); + + boolean isPermanentlyLockedOut(KeycloakSession session, RealmModel realm, UserModel user); + + /** + * Clears any remaining traces of the permanent lockout. Does not enable the user as such! + * @param session + * @param realm + * @param user + */ + void cleanUpPermanentLockout(KeycloakSession session, RealmModel realm, UserModel user); } diff --git a/server-spi/src/main/java/org/keycloak/models/UserModel.java b/server-spi/src/main/java/org/keycloak/models/UserModel.java index ef73d5e75b66..4e0d0d615d6f 100755 --- a/server-spi/src/main/java/org/keycloak/models/UserModel.java +++ b/server-spi/src/main/java/org/keycloak/models/UserModel.java @@ -46,6 +46,7 @@ public interface UserModel extends RoleMapperModel { String GROUPS = "keycloak.session.realm.users.query.groups"; String SEARCH = "keycloak.session.realm.users.query.search"; String EXACT = "keycloak.session.realm.users.query.exact"; + String DISABLED_REASON = "disabledReason"; Comparator COMPARE_BY_USERNAME = Comparator.comparing(UserModel::getUsername, String.CASE_INSENSITIVE_ORDER); 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 80a471ad7846..92dc020a69f4 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 @@ -34,8 +34,8 @@ import org.keycloak.representations.idm.CredentialRepresentation; import org.keycloak.services.ServicesLogger; import org.keycloak.services.managers.AuthenticationManager; +import org.keycloak.services.managers.BruteForceProtector; import org.keycloak.services.messages.Messages; -import org.keycloak.services.validation.Validation; import javax.ws.rs.core.MultivaluedMap; import javax.ws.rs.core.Response; @@ -80,11 +80,11 @@ protected Response createLoginForm(LoginFormsProvider form) { return form.createLoginUsernamePassword(); } - protected String tempDisabledError() { + protected String disabledByBruteForceError() { return Messages.INVALID_USER; } - protected String tempDisabledFieldError(){ + protected String disabledByBruteForceFieldError(){ return FIELD_USERNAME; } @@ -129,6 +129,7 @@ public void testInvalidUser(AuthenticationFlowContext context, UserModel user) { } public boolean enabledUser(AuthenticationFlowContext context, UserModel user) { + if (isDisabledByBruteForce(context, user)) return false; if (!user.isEnabled()) { context.getEvent().user(user); context.getEvent().error(Errors.USER_DISABLED); @@ -136,7 +137,6 @@ public boolean enabledUser(AuthenticationFlowContext context, UserModel user) { context.forceChallenge(challengeResponse); return false; } - if (isTemporarilyDisabledByBruteForce(context, user)) return false; return true; } @@ -213,7 +213,7 @@ public boolean validatePassword(AuthenticationFlowContext context, UserModel use return badPasswordHandler(context, user, clearUser,true); } - if (isTemporarilyDisabledByBruteForce(context, user)) return false; + if (isDisabledByBruteForce(context, user)) return false; if (password != null && !password.isEmpty() && context.getSession().userCredentialManager().isValid(context.getRealm(), user, UserCredentialModel.password(password))) { return true; @@ -239,12 +239,15 @@ private boolean badPasswordHandler(AuthenticationFlowContext context, UserModel return false; } - protected boolean isTemporarilyDisabledByBruteForce(AuthenticationFlowContext context, UserModel user) { + protected boolean isDisabledByBruteForce(AuthenticationFlowContext context, UserModel user) { if (context.getRealm().isBruteForceProtected()) { - if (context.getProtector().isTemporarilyDisabled(context.getSession(), context.getRealm(), user)) { + BruteForceProtector protector = context.getProtector(); + boolean isPermanentlyLockedOut = protector.isPermanentlyLockedOut(context.getSession(), context.getRealm(), user); + + if (isPermanentlyLockedOut || protector.isTemporarilyDisabled(context.getSession(), context.getRealm(), user)) { context.getEvent().user(user); - context.getEvent().error(Errors.USER_TEMPORARILY_DISABLED); - Response challengeResponse = challenge(context, tempDisabledError(), tempDisabledFieldError()); + context.getEvent().error(isPermanentlyLockedOut ? Errors.USER_DISABLED : Errors.USER_TEMPORARILY_DISABLED); + Response challengeResponse = challenge(context, disabledByBruteForceError(), disabledByBruteForceFieldError()); context.forceChallenge(challengeResponse); return true; } diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/browser/OTPFormAuthenticator.java b/services/src/main/java/org/keycloak/authentication/authenticators/browser/OTPFormAuthenticator.java index 04a02137b892..3a33c4f98492 100755 --- a/services/src/main/java/org/keycloak/authentication/authenticators/browser/OTPFormAuthenticator.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/browser/OTPFormAuthenticator.java @@ -88,7 +88,7 @@ public void validateOTP(AuthenticationFlowContext context) { UserModel userModel = context.getUser(); if (!enabledUser(context, userModel)) { - // error in context is set in enabledUser/isTemporarilyDisabledByBruteForce + // error in context is set in enabledUser/isDisabledByBruteForce return; } @@ -115,12 +115,12 @@ public boolean requiresUser() { } @Override - protected String tempDisabledError() { + protected String disabledByBruteForceError() { return Messages.INVALID_TOTP; } @Override - protected String tempDisabledFieldError() { + protected String disabledByBruteForceFieldError() { return Validation.FIELD_OTP_CODE; } 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 c11c59426941..56b48eb62c39 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 @@ -31,6 +31,7 @@ import org.keycloak.provider.ProviderConfigProperty; import org.keycloak.services.ServicesLogger; import org.keycloak.services.managers.AuthenticationManager; +import org.keycloak.services.managers.BruteForceProtector; import javax.ws.rs.core.MultivaluedMap; import javax.ws.rs.core.Response; @@ -74,22 +75,25 @@ public void authenticate(AuthenticationFlowContext context) { context.failure(AuthenticationFlowError.INVALID_USER, challengeResponse); return; } - if (!user.isEnabled()) { - 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); - return; - } if (context.getRealm().isBruteForceProtected()) { - if (context.getProtector().isTemporarilyDisabled(context.getSession(), context.getRealm(), user)) { + BruteForceProtector protector = context.getProtector(); + boolean isPermanentlyLockedOut = protector.isPermanentlyLockedOut(context.getSession(), context.getRealm(), user); + + if (isPermanentlyLockedOut || protector.isTemporarilyDisabled(context.getSession(), context.getRealm(), user)) { context.getEvent().user(user); - context.getEvent().error(Errors.USER_TEMPORARILY_DISABLED); + context.getEvent().error(isPermanentlyLockedOut ? Errors.USER_DISABLED : Errors.USER_TEMPORARILY_DISABLED); Response challengeResponse = errorResponse(Response.Status.UNAUTHORIZED.getStatusCode(), "invalid_grant", "Invalid user credentials"); context.failure(AuthenticationFlowError.INVALID_USER, challengeResponse); return; } } + if (!user.isEnabled()) { + 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); + return; + } context.setUser(user); context.success(); } diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/x509/ValidateX509CertificateUsername.java b/services/src/main/java/org/keycloak/authentication/authenticators/x509/ValidateX509CertificateUsername.java index c86eafb63fd2..789b6d9f98ea 100644 --- a/services/src/main/java/org/keycloak/authentication/authenticators/x509/ValidateX509CertificateUsername.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/x509/ValidateX509CertificateUsername.java @@ -30,6 +30,7 @@ import org.keycloak.models.ModelDuplicateException; import org.keycloak.models.UserModel; import org.keycloak.services.ServicesLogger; +import org.keycloak.services.managers.BruteForceProtector; /** * @author Peter Nalyvayko @@ -118,6 +119,18 @@ public void authenticate(AuthenticationFlowContext context) { context.failure(AuthenticationFlowError.INVALID_USER, challengeResponse); return; } + if (context.getRealm().isBruteForceProtected()) { + BruteForceProtector protector = context.getProtector(); + boolean isPermanentlyLockedOut = protector.isPermanentlyLockedOut(context.getSession(), context.getRealm(), user); + + if (isPermanentlyLockedOut || protector.isTemporarilyDisabled(context.getSession(), context.getRealm(), user)) { + context.getEvent().user(user); + context.getEvent().error(isPermanentlyLockedOut ? Errors.USER_DISABLED : Errors.USER_TEMPORARILY_DISABLED); + Response challengeResponse = errorResponse(Response.Status.BAD_REQUEST.getStatusCode(), "invalid_grant", "Invalid user credentials"); + context.failure(AuthenticationFlowError.INVALID_USER, challengeResponse); + return; + } + } if (!user.isEnabled()) { context.getEvent().user(user); context.getEvent().error(Errors.USER_DISABLED); @@ -125,15 +138,6 @@ public void authenticate(AuthenticationFlowContext context) { context.failure(AuthenticationFlowError.INVALID_USER, challengeResponse); return; } - 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 = errorResponse(Response.Status.BAD_REQUEST.getStatusCode(), "invalid_grant", "Account temporarily disabled"); - context.failure(AuthenticationFlowError.INVALID_USER, challengeResponse); - return; - } - } context.setUser(user); context.success(); } diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/x509/X509ClientCertificateAuthenticator.java b/services/src/main/java/org/keycloak/authentication/authenticators/x509/X509ClientCertificateAuthenticator.java index 8f6f895f1bbc..2836e6d076dc 100644 --- a/services/src/main/java/org/keycloak/authentication/authenticators/x509/X509ClientCertificateAuthenticator.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/x509/X509ClientCertificateAuthenticator.java @@ -35,6 +35,7 @@ import org.keycloak.models.ModelDuplicateException; import org.keycloak.models.UserModel; import org.keycloak.models.utils.FormMessage; +import org.keycloak.services.managers.BruteForceProtector; /** * @author Peter Nalyvayko @@ -135,28 +136,32 @@ public void authenticate(AuthenticationFlowContext context) { return; } - if (!userEnabled(context, user)) { - // TODO use specific locale to load error messages - String errorMessage = "X509 certificate authentication's failed."; - // TODO is calling form().setErrors enough to show errors on login screen? - context.challenge(createErrorResponse(context, certs[0].getSubjectDN().getName(), - errorMessage, "User is disabled")); - context.attempted(); - return; - } if (context.getRealm().isBruteForceProtected()) { - if (context.getProtector().isTemporarilyDisabled(context.getSession(), context.getRealm(), user)) { + BruteForceProtector protector = context.getProtector(); + boolean isPermanentlyLockedOut = protector.isPermanentlyLockedOut(context.getSession(), context.getRealm(), user); + + if (isPermanentlyLockedOut || protector.isTemporarilyDisabled(context.getSession(), context.getRealm(), user)) { context.getEvent().user(user); - context.getEvent().error(Errors.USER_TEMPORARILY_DISABLED); + context.getEvent().error(isPermanentlyLockedOut ? Errors.USER_DISABLED : Errors.USER_TEMPORARILY_DISABLED); // TODO use specific locale to load error messages String errorMessage = "X509 certificate authentication's failed."; // TODO is calling form().setErrors enough to show errors on login screen? context.challenge(createErrorResponse(context, certs[0].getSubjectDN().getName(), - errorMessage, "User is temporarily disabled. Contact administrator.")); + errorMessage, "Invalid user")); context.attempted(); return; } } + + if (!userEnabled(context, user)) { + // TODO use specific locale to load error messages + String errorMessage = "X509 certificate authentication's failed."; + // TODO is calling form().setErrors enough to show errors on login screen? + context.challenge(createErrorResponse(context, certs[0].getSubjectDN().getName(), + errorMessage, "User is disabled")); + context.attempted(); + return; + } context.setUser(user); // Check whether to display the identity confirmation diff --git a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java index 13cd917c6e57..1f9fb7e9b971 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java @@ -1235,8 +1235,11 @@ protected UserModel importUserFromExternalIdentity(BrokeredIdentityContext conte throw new CorsErrorResponseException(cors, Errors.INVALID_TOKEN, "Invalid Token", Response.Status.BAD_REQUEST); } if (realm.isBruteForceProtected()) { - if (session.getProvider(BruteForceProtector.class).isTemporarilyDisabled(session, realm, user)) { - event.error(Errors.USER_TEMPORARILY_DISABLED); + BruteForceProtector protector = session.getProvider(BruteForceProtector.class); + boolean isPermanentlyLockedOut = protector.isPermanentlyLockedOut(session, realm, user); + + if (isPermanentlyLockedOut || protector.isTemporarilyDisabled(session, realm, user)) { + event.error(isPermanentlyLockedOut ? Errors.USER_DISABLED : Errors.USER_TEMPORARILY_DISABLED); throw new CorsErrorResponseException(cors, Errors.INVALID_TOKEN, "Invalid Token", Response.Status.BAD_REQUEST); } } 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 831db86bae93..11dc16a29256 100644 --- a/services/src/main/java/org/keycloak/services/managers/DefaultBruteForceProtector.java +++ b/services/src/main/java/org/keycloak/services/managers/DefaultBruteForceProtector.java @@ -33,6 +33,8 @@ import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.TimeUnit; +import static org.keycloak.models.UserModel.DISABLED_REASON; + /** * A single thread will log failures. This is so that we can avoid concurrent writes as we want an accurate failure count * @@ -128,6 +130,7 @@ public void failure(KeycloakSession session, LoginEvent event) { } logger.debugv("user {0} locked permanently due to too many login attempts", user.getUsername()); user.setEnabled(false); + user.setSingleAttribute(DISABLED_REASON, DISABLED_BY_PERMANENT_LOCKOUT); return; } @@ -318,6 +321,19 @@ public boolean isTemporarilyDisabled(KeycloakSession session, RealmModel realm, return false; } + + @Override + public boolean isPermanentlyLockedOut(KeycloakSession session, RealmModel realm, UserModel user) { + return !user.isEnabled() && DISABLED_BY_PERMANENT_LOCKOUT.equals(user.getFirstAttribute(DISABLED_REASON)); + } + + @Override + public void cleanUpPermanentLockout(KeycloakSession session, RealmModel realm, UserModel user) { + if (DISABLED_BY_PERMANENT_LOCKOUT.equals(user.getFirstAttribute(DISABLED_REASON))) { + user.removeAttribute(DISABLED_REASON); + } + } + @Override public void close() { diff --git a/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java b/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java index 19632587fe67..796b4241b63a 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java @@ -99,7 +99,6 @@ import javax.ws.rs.core.UriBuilder; import java.net.URI; import java.text.MessageFormat; -import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; import java.util.LinkedList; @@ -162,11 +161,13 @@ public Response updateUser(final UserRepresentation rep) { auth.users().requireManage(user); try { + boolean wasPermanentlyLockedOut = false; if (rep.isEnabled() != null && rep.isEnabled()) { UserLoginFailureModel failureModel = session.loginFailures().getUserLoginFailure(realm, user.getId()); if (failureModel != null) { failureModel.clearFailures(); } + wasPermanentlyLockedOut = session.getProvider(BruteForceProtector.class).isPermanentlyLockedOut(session, realm, user); } Response response = validateUserProfile(user, rep, session); @@ -175,6 +176,12 @@ public Response updateUser(final UserRepresentation rep) { } updateUserFromRep(user, rep, session, true); RepresentationToModel.createCredentials(rep, session, realm, user, true); + + // we need to do it here as the attributes would be overwritten by what is in the rep + if (wasPermanentlyLockedOut) { + session.getProvider(BruteForceProtector.class).cleanUpPermanentLockout(session, realm, user); + } + adminEvent.operation(OperationType.UPDATE).resourcePath(session.getContext().getUri()).representation(rep).success(); if (session.getTransactionManager().isActive()) { 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 9e4a87a65fd9..274d15d62b8e 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 @@ -26,9 +26,11 @@ import org.keycloak.events.Errors; import org.keycloak.events.EventType; import org.keycloak.models.Constants; +import org.keycloak.models.UserModel; import org.keycloak.models.utils.TimeBasedOTP; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.representations.idm.UserRepresentation; +import org.keycloak.services.managers.BruteForceProtector; import org.keycloak.testsuite.AbstractTestRealmKeycloakTest; import org.keycloak.testsuite.AssertEvents; import org.keycloak.testsuite.AssertEvents.ExpectedEvent; @@ -419,7 +421,14 @@ public void testPermanentLockout() throws Exception { // assert expectPermanentlyDisabled(); - assertFalse(adminClient.realm("test").users().search("test-user@localhost", 0, 1).get(0).isEnabled()); + + UserRepresentation user = adminClient.realm("test").users().search("test-user@localhost", 0, 1).get(0); + assertFalse(user.isEnabled()); + assertUserDisabledReason(BruteForceProtector.DISABLED_BY_PERMANENT_LOCKOUT); + + user.setEnabled(true); + updateUser(user); + assertUserDisabledReason(null); } finally { realm.setPermanentLockout(false); testRealm().update(realm); @@ -563,7 +572,7 @@ public void expectPermanentlyDisabled(String username, String userId) throws Exc loginPage.login(username, "password"); loginPage.assertCurrent(); - Assert.assertEquals("Account is disabled, contact your administrator.", loginPage.getError()); + Assert.assertEquals("Invalid username or password.", loginPage.getInputError()); ExpectedEvent event = events.expectLogin() .session((String) null) .error(Errors.USER_DISABLED) @@ -708,4 +717,12 @@ public void registerUser(String username) { private void assertUserDisabledEvent() { events.expect(EventType.LOGIN_ERROR).error(Errors.USER_TEMPORARILY_DISABLED).assertEvent(); } + + private void assertUserDisabledReason(String expected) { + String actual = adminClient.realm("test").users() + .search("test-user@localhost", 0, 1) + .get(0) + .firstAttribute(UserModel.DISABLED_REASON); + assertEquals(expected, actual); + } }