KEYCLOAK-2024 - username guessing

keycloak · Nov 10, 2015 · 1437f7d · 1437f7d
1 parent 2b23497
commit 1437f7d
Show file tree

Hide file tree

Showing 4 changed files with 38 additions and 23 deletions.
diff --git a/.../main/java/org/keycloak/authentication/authenticators/broker/IdpUsernamePasswordForm.java b/.../main/java/org/keycloak/authentication/authenticators/broker/IdpUsernamePasswordForm.java
@@ -37,7 +37,7 @@ protected boolean validateForm(AuthenticationFlowContext context, MultivaluedMap
         // Restore formData for the case of error
         setupForm(context, formData, existingUser);
 
-        return validatePassword(context, formData);
+        return validatePassword(context, existingUser, formData);
     }
 
     protected LoginFormsProvider setupForm(AuthenticationFlowContext context, MultivaluedMap<String, String> formData, UserModel existingUser) {

diff --git a/...org/keycloak/authentication/authenticators/browser/AbstractUsernameFormAuthenticator.java b/...org/keycloak/authentication/authenticators/browser/AbstractUsernameFormAuthenticator.java
@@ -71,26 +71,30 @@ public boolean invalidUser(AuthenticationFlowContext context, UserModel user) {
             context.failureChallenge(AuthenticationFlowError.INVALID_USER, challengeResponse);
             return true;
         }
+        return false;
+    }
+
+    public boolean enabledUser(AuthenticationFlowContext context, UserModel user) {
         if (!user.isEnabled()) {
             context.getEvent().user(user);
             context.getEvent().error(Errors.USER_DISABLED);
             Response challengeResponse = disabledUser(context);
             context.failureChallenge(AuthenticationFlowError.USER_DISABLED, challengeResponse);
-            return true;
+            return false;
         }
         if (context.getRealm().isBruteForceProtected()) {
             if (context.getProtector().isTemporarilyDisabled(context.getSession(), context.getRealm(), user.getUsername())) {
                 context.getEvent().user(user);
                 context.getEvent().error(Errors.USER_TEMPORARILY_DISABLED);
                 Response challengeResponse = temporarilyDisabledUser(context);
                 context.failureChallenge(AuthenticationFlowError.USER_TEMPORARILY_DISABLED, challengeResponse);
-                return true;
+                return false;
             }
         }
-        return false;
+        return true;
     }
 
-    public boolean validateUser(AuthenticationFlowContext context, MultivaluedMap<String, String> inputData) {
+    public boolean validateUserAndPassword(AuthenticationFlowContext context, MultivaluedMap<String, String> inputData) {
         String username = inputData.getFirst(AuthenticationManager.FORM_USERNAME);
         if (username == null) {
             context.getEvent().error(Errors.USER_NOT_FOUND);
@@ -117,7 +121,18 @@ public boolean validateUser(AuthenticationFlowContext context, MultivaluedMap<St
             return false;
         }
 
-        if (invalidUser(context, user)) return false;
+        if (invalidUser(context, user)){
+            return false;
+        }
+
+        if (!validatePassword(context, user, inputData)){
+            return false;
+        }
+
+        if(!enabledUser(context, user)){
+            return false;
+        }
+
         String rememberMe = inputData.getFirst("rememberMe");
         boolean remember = rememberMe != null && rememberMe.equalsIgnoreCase("on");
         if (remember) {
@@ -130,29 +145,27 @@ public boolean validateUser(AuthenticationFlowContext context, MultivaluedMap<St
         return true;
     }
 
-    public boolean validatePassword(AuthenticationFlowContext context, MultivaluedMap<String, String> inputData) {
+    public boolean validatePassword(AuthenticationFlowContext context, UserModel user, MultivaluedMap<String, String> inputData) {
         List<UserCredentialModel> credentials = new LinkedList<>();
         String password = inputData.getFirst(CredentialRepresentation.PASSWORD);
         if (password == null || password.isEmpty()) {
-            if (context.getUser() != null) {
-                context.getEvent().user(context.getUser());
-            }
-            context.getEvent().error(Errors.INVALID_USER_CREDENTIALS);
-            Response challengeResponse = invalidCredentials(context);
-            context.failureChallenge(AuthenticationFlowError.INVALID_CREDENTIALS, challengeResponse);
-            context.clearUser();
+            invalidPassword(context, user);
             return false;
         }
         credentials.add(UserCredentialModel.password(password));
-        boolean valid = context.getSession().users().validCredentials(context.getRealm(), context.getUser(), credentials);
+        boolean valid = context.getSession().users().validCredentials(context.getRealm(), user, credentials);
         if (!valid) {
-            context.getEvent().user(context.getUser());
-            context.getEvent().error(Errors.INVALID_USER_CREDENTIALS);
-            Response challengeResponse = invalidCredentials(context);
-            context.failureChallenge(AuthenticationFlowError.INVALID_CREDENTIALS, challengeResponse);
-            context.clearUser();
+            invalidPassword(context, user);
             return false;
         }
         return true;
     }
+
+    private void invalidPassword(AuthenticationFlowContext context, UserModel user) {
+        context.getEvent().user(user);
+        context.getEvent().error(Errors.INVALID_USER_CREDENTIALS);
+        Response challengeResponse = invalidCredentials(context);
+        context.failureChallenge(AuthenticationFlowError.INVALID_CREDENTIALS, challengeResponse);
+        context.clearUser();
+    }
 }
diff --git a/...rc/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java b/...rc/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java
@@ -38,7 +38,7 @@ public void action(AuthenticationFlowContext context) {
     }
 
     protected boolean validateForm(AuthenticationFlowContext context, MultivaluedMap<String, String> formData) {
-        return validateUser(context, formData) && validatePassword(context, formData);
+        return validateUserAndPassword(context, formData);
     }
 
     @Override

diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/forms/LoginTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/forms/LoginTest.java
@@ -215,9 +215,10 @@ public void config(RealmManager manager, RealmModel adminstrationRealm, RealmMod
             Assert.assertEquals("login-test", loginPage.getUsername());
             Assert.assertEquals("", loginPage.getPassword());
 
-            Assert.assertEquals("Account is disabled, contact admin.", loginPage.getError());
+            // KEYCLOAK-2024
+            Assert.assertEquals("Invalid username or password.", loginPage.getError());
 
-            events.expectLogin().user(userId).session((String) null).error("user_disabled")
+            events.expectLogin().user(userId).session((String) null).error("invalid_user_credentials")
                     .detail(Details.USERNAME, "login-test")
                     .removeDetail(Details.CONSENT)
                     .assertEvent();
@@ -250,6 +251,7 @@ public void config(RealmManager manager, RealmModel adminstrationRealm, RealmMod
             Assert.assertEquals("login-test", loginPage.getUsername());
             Assert.assertEquals("", loginPage.getPassword());
 
+            // KEYCLOAK-2024
             Assert.assertEquals("Account is disabled, contact admin.", loginPage.getError());
 
             events.expectLogin().user(userId).session((String) null).error("user_disabled")