From 1437f7da3555fdfa5f9ee545b96b1b3198abc776 Mon Sep 17 00:00:00 2001 From: Michael Gerber Date: Tue, 10 Nov 2015 09:04:10 +0100 Subject: [PATCH] KEYCLOAK-2024 - username guessing --- .../broker/IdpUsernamePasswordForm.java | 2 +- .../AbstractUsernameFormAuthenticator.java | 51 ++++++++++++------- .../browser/UsernamePasswordForm.java | 2 +- .../keycloak/testsuite/forms/LoginTest.java | 6 ++- 4 files changed, 38 insertions(+), 23 deletions(-) diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpUsernamePasswordForm.java b/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpUsernamePasswordForm.java index b293d1bb4396..5e732d8be790 100644 --- a/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpUsernamePasswordForm.java +++ b/services/src/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 formData, UserModel existingUser) { 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 294d220696f7..0506f2156bbe 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 @@ -71,12 +71,16 @@ 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())) { @@ -84,13 +88,13 @@ public boolean invalidUser(AuthenticationFlowContext context, UserModel 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 inputData) { + public boolean validateUserAndPassword(AuthenticationFlowContext context, MultivaluedMap 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 inputData) { + public boolean validatePassword(AuthenticationFlowContext context, UserModel user, MultivaluedMap inputData) { List 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/services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java b/services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java index 70d9fd990242..74636384bc70 100755 --- a/services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java @@ -38,7 +38,7 @@ public void action(AuthenticationFlowContext context) { } protected boolean validateForm(AuthenticationFlowContext context, MultivaluedMap 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 index 950c182a3f68..30d94fae7830 100755 --- 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")