KEYCLOAK-17835 Account Permanent Lockout and login error messages

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);
 }

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<UserModel> COMPARE_BY_USERNAME = Comparator.comparing(UserModel::getUsername, String.CASE_INSENSITIVE_ORDER);
 

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,14 +129,14 @@ 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);
             Response challengeResponse = challenge(context, Messages.ACCOUNT_DISABLED);
             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;
             }

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;
     }
 

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();
     }

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 <a href="mailto:pnalyvayko@agi.com">Peter Nalyvayko</a>
@@ -118,22 +119,25 @@ 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);
             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)) {
-                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();
     }

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 <a href="mailto:pnalyvayko@agi.com">Peter Nalyvayko</a>
@@ -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

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);
                 }
             }

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() {
 

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()) {

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);
+    }
 }