From 77a44751a69f875682bb34ed085697f8ca374627 Mon Sep 17 00:00:00 2001 From: mposolda Date: Wed, 26 Mar 2014 09:49:47 +0100 Subject: [PATCH] KEYCLOAK-388 - Auth SPI should be able to differentiate between the INVALID_USERNAME and INVALID_CREDENTIALS --- .../test/AuthProvidersExternalModelTest.java | 7 ++- .../model/test/AuthProvidersLDAPTest.java | 46 +++++++++++++++++-- .../managers/AuthenticationManager.java | 5 +- .../AbstractModelAuthenticationProvider.java | 5 +- .../PicketlinkAuthenticationProvider.java | 11 +++-- .../authentication/AuthProviderStatus.java | 4 +- .../AuthenticationProvider.java | 2 +- .../AuthenticationProviderManager.java | 13 ++++-- 8 files changed, 70 insertions(+), 23 deletions(-) diff --git a/model/tests/src/test/java/org/keycloak/model/test/AuthProvidersExternalModelTest.java b/model/tests/src/test/java/org/keycloak/model/test/AuthProvidersExternalModelTest.java index 41b107170b..f8ce23b442 100644 --- a/model/tests/src/test/java/org/keycloak/model/test/AuthProvidersExternalModelTest.java +++ b/model/tests/src/test/java/org/keycloak/model/test/AuthProvidersExternalModelTest.java @@ -13,7 +13,9 @@ import javax.ws.rs.core.MultivaluedMap; import org.jboss.resteasy.spi.ResteasyProviderFactory; import org.junit.Assert; import org.junit.Before; +import org.junit.FixMethodOrder; import org.junit.Test; +import org.junit.runners.MethodSorters; import org.keycloak.models.AuthenticationLinkModel; import org.keycloak.models.AuthenticationProviderModel; import org.keycloak.models.KeycloakSession; @@ -30,6 +32,7 @@ import org.keycloak.spi.authentication.AuthenticationProviderManager; /** * @author Marek Posolda */ +@FixMethodOrder(MethodSorters.NAME_ASCENDING) public class AuthProvidersExternalModelTest extends AbstractModelTest { private RealmModel realm1; @@ -63,14 +66,14 @@ public class AuthProvidersExternalModelTest extends AbstractModelTest { @Test - public void testExternalModelPasswordValidation() { + public void testExternalModelAuthentication() { MultivaluedMap formData = createFormData("john", "password"); // Authenticate user with realm1 Assert.assertEquals(AuthenticationManager.AuthenticationStatus.SUCCESS, am.authenticateForm(realm1, formData)); // Verify that user doesn't exists in realm2 and can't authenticate here - Assert.assertEquals(AuthenticationManager.AuthenticationStatus.INVALID_CREDENTIALS, am.authenticateForm(realm2, formData)); + Assert.assertEquals(AuthenticationManager.AuthenticationStatus.INVALID_USER, am.authenticateForm(realm2, formData)); Assert.assertNull(realm2.getUser("john")); // Add externalModel authenticationProvider into realm2 and point to realm1 diff --git a/model/tests/src/test/java/org/keycloak/model/test/AuthProvidersLDAPTest.java b/model/tests/src/test/java/org/keycloak/model/test/AuthProvidersLDAPTest.java index 2165e2f073..06a498c3b1 100644 --- a/model/tests/src/test/java/org/keycloak/model/test/AuthProvidersLDAPTest.java +++ b/model/tests/src/test/java/org/keycloak/model/test/AuthProvidersLDAPTest.java @@ -10,10 +10,13 @@ import org.jboss.resteasy.spi.ResteasyProviderFactory; import org.junit.After; import org.junit.Assert; import org.junit.Before; +import org.junit.FixMethodOrder; import org.junit.Test; +import org.junit.runners.MethodSorters; import org.keycloak.models.AuthenticationLinkModel; import org.keycloak.models.AuthenticationProviderModel; import org.keycloak.models.RealmModel; +import org.keycloak.models.UserCredentialModel; import org.keycloak.models.UserModel; import org.keycloak.representations.idm.CredentialRepresentation; import org.keycloak.services.managers.AuthenticationManager; @@ -25,6 +28,7 @@ import org.keycloak.util.KeycloakRegistry; /** * @author Marek Posolda */ +@FixMethodOrder(MethodSorters.NAME_ASCENDING) public class AuthProvidersLDAPTest extends AbstractModelTest { private RealmModel realm; @@ -64,11 +68,11 @@ public class AuthProvidersLDAPTest extends AbstractModelTest { } @Test - public void testLdapPasswordValidation() { + public void testLdapAuthentication() { MultivaluedMap formData = AuthProvidersExternalModelTest.createFormData("john", "password"); // Verify that user doesn't exists in realm2 and can't authenticate here - Assert.assertEquals(AuthenticationManager.AuthenticationStatus.INVALID_CREDENTIALS, am.authenticateForm(realm, formData)); + Assert.assertEquals(AuthenticationManager.AuthenticationStatus.INVALID_USER, am.authenticateForm(realm, formData)); Assert.assertNull(realm.getUser("john")); // Add ldap authenticationProvider @@ -87,9 +91,6 @@ public class AuthProvidersLDAPTest extends AbstractModelTest { Assert.assertEquals("Doe", john.getLastName()); Assert.assertEquals("john@email.org", john.getEmail()); - formData = AuthProvidersExternalModelTest.createFormData("john", "invalid"); - Assert.assertEquals(AuthenticationManager.AuthenticationStatus.INVALID_CREDENTIALS, am.authenticateForm(realm, formData)); - // Verify link exists Set authLinks = realm.getAuthenticationLinks(john); Assert.assertEquals(1, authLinks.size()); @@ -101,6 +102,41 @@ public class AuthProvidersLDAPTest extends AbstractModelTest { } + @Test + public void testLdapInvalidAuthentication() { + setupAuthenticationProviders(); + + try { + ResteasyProviderFactory.pushContext(KeycloakRegistry.class, new KeycloakRegistry()); + + // Add some user and password to realm + UserModel realmUser = realm.addUser("realmUser"); + UserCredentialModel credential = new UserCredentialModel(); + credential.setType(CredentialRepresentation.PASSWORD); + credential.setValue("pass"); + realm.updateCredential(realmUser, credential); + + // User doesn't exists + MultivaluedMap formData = AuthProvidersExternalModelTest.createFormData("invalid", "invalid"); + Assert.assertEquals(AuthenticationManager.AuthenticationStatus.INVALID_USER, am.authenticateForm(realm, formData)); + + // User exists in ldap + formData = AuthProvidersExternalModelTest.createFormData("john", "invalid"); + Assert.assertEquals(AuthenticationManager.AuthenticationStatus.INVALID_CREDENTIALS, am.authenticateForm(realm, formData)); + + // User exists in realm + formData = AuthProvidersExternalModelTest.createFormData("realmUser", "invalid"); + Assert.assertEquals(AuthenticationManager.AuthenticationStatus.INVALID_CREDENTIALS, am.authenticateForm(realm, formData)); + + // User disabled + realmUser.setEnabled(false); + formData = AuthProvidersExternalModelTest.createFormData("realmUser", "pass"); + Assert.assertEquals(AuthenticationManager.AuthenticationStatus.ACCOUNT_DISABLED, am.authenticateForm(realm, formData)); + } finally { + ResteasyProviderFactory.clearContextData(); + } + } + @Test public void testLdapPasswordUpdate() { // Add ldap diff --git a/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java b/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java index 0d8773b7c2..fe86e5b603 100755 --- a/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java +++ b/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java @@ -224,9 +224,12 @@ public class AuthenticationManager { logger.debug("validating password for user: " + username); AuthResult authResult = AuthenticationProviderManager.getManager(realm).validatePassword(username, password); - if (authResult.getAuthProviderStatus() == AuthProviderStatus.FAILED) { + if (authResult.getAuthProviderStatus() == AuthProviderStatus.INVALID_CREDENTIALS) { logger.debug("invalid password for user: " + username); return AuthenticationStatus.INVALID_CREDENTIALS; + } else if (authResult.getAuthProviderStatus() == AuthProviderStatus.USER_NOT_FOUND) { + logger.debug("User " + username + " not found in any Authentication provider"); + return AuthenticationStatus.INVALID_USER; } if (authResult.getAuthenticatedUser() != null) { diff --git a/spi/authentication-model/src/main/java/org/keycloak/spi/authentication/model/AbstractModelAuthenticationProvider.java b/spi/authentication-model/src/main/java/org/keycloak/spi/authentication/model/AbstractModelAuthenticationProvider.java index 94652a558b..58bf42390f 100644 --- a/spi/authentication-model/src/main/java/org/keycloak/spi/authentication/model/AbstractModelAuthenticationProvider.java +++ b/spi/authentication-model/src/main/java/org/keycloak/spi/authentication/model/AbstractModelAuthenticationProvider.java @@ -29,14 +29,13 @@ public abstract class AbstractModelAuthenticationProvider implements Authenticat UserModel user = KeycloakModelUtils.findUserByNameOrEmail(realm, username); - // Ignore if user doesn't exists, so that other providers have opportunity to authenticate (and possibly create) him if (user == null) { - return new AuthResult(AuthProviderStatus.IGNORE); + return new AuthResult(AuthProviderStatus.USER_NOT_FOUND); } boolean result = realm.validatePassword(user, password); if (!result) { - return new AuthResult(AuthProviderStatus.IGNORE); + return new AuthResult(AuthProviderStatus.INVALID_CREDENTIALS); } AuthenticatedUser authUser = createAuthenticatedUserInstance(user); diff --git a/spi/authentication-picketlink/src/main/java/org/keycloak/spi/authentication/picketlink/PicketlinkAuthenticationProvider.java b/spi/authentication-picketlink/src/main/java/org/keycloak/spi/authentication/picketlink/PicketlinkAuthenticationProvider.java index 4494bdee6c..2e9375d3ac 100644 --- a/spi/authentication-picketlink/src/main/java/org/keycloak/spi/authentication/picketlink/PicketlinkAuthenticationProvider.java +++ b/spi/authentication-picketlink/src/main/java/org/keycloak/spi/authentication/picketlink/PicketlinkAuthenticationProvider.java @@ -39,23 +39,26 @@ public class PicketlinkAuthenticationProvider implements AuthenticationProvider public AuthResult validatePassword(RealmModel realm, Map configuration, String username, String password) throws AuthenticationProviderException { IdentityManager identityManager = getIdentityManager(realm); + User picketlinkUser = BasicModel.getUser(identityManager, username); + if (picketlinkUser == null) { + return new AuthResult(AuthProviderStatus.USER_NOT_FOUND); + } + UsernamePasswordCredentials credential = new UsernamePasswordCredentials(); credential.setUsername(username); credential.setPassword(new Password(password.toCharArray())); identityManager.validateCredentials(credential); - AuthResult result; if (credential.getStatus() == Credentials.Status.VALID) { - result = new AuthResult(AuthProviderStatus.SUCCESS); + AuthResult result = new AuthResult(AuthProviderStatus.SUCCESS); - User picketlinkUser = BasicModel.getUser(identityManager, username); AuthenticatedUser authenticatedUser = new AuthenticatedUser(picketlinkUser.getId(), picketlinkUser.getLoginName()) .setName(picketlinkUser.getFirstName(), picketlinkUser.getLastName()) .setEmail(picketlinkUser.getEmail()); result.setUser(authenticatedUser).setProviderName(getName()); return result; } else { - return new AuthResult(AuthProviderStatus.IGNORE); + return new AuthResult(AuthProviderStatus.INVALID_CREDENTIALS); } } diff --git a/spi/authentication-spi/src/main/java/org/keycloak/spi/authentication/AuthProviderStatus.java b/spi/authentication-spi/src/main/java/org/keycloak/spi/authentication/AuthProviderStatus.java index cfd16a7f79..56d05d0543 100644 --- a/spi/authentication-spi/src/main/java/org/keycloak/spi/authentication/AuthProviderStatus.java +++ b/spi/authentication-spi/src/main/java/org/keycloak/spi/authentication/AuthProviderStatus.java @@ -7,6 +7,6 @@ package org.keycloak.spi.authentication; */ public enum AuthProviderStatus { - // Ignore means that AuthenticationProvider wasn't able to authenticate result, but it should postpone authentication to next provider (for example user didn't exists in realm) - SUCCESS, FAILED, IGNORE + SUCCESS, INVALID_CREDENTIALS, USER_NOT_FOUND + } diff --git a/spi/authentication-spi/src/main/java/org/keycloak/spi/authentication/AuthenticationProvider.java b/spi/authentication-spi/src/main/java/org/keycloak/spi/authentication/AuthenticationProvider.java index a960be90b3..1ab1836ef0 100644 --- a/spi/authentication-spi/src/main/java/org/keycloak/spi/authentication/AuthenticationProvider.java +++ b/spi/authentication-spi/src/main/java/org/keycloak/spi/authentication/AuthenticationProvider.java @@ -16,7 +16,7 @@ public interface AuthenticationProvider { * * @param username * @param password - * @return + * @return result of authentication, which might eventually encapsulate info about authenticated user and provider which successfully authenticated */ AuthResult validatePassword(RealmModel realm, Map configuration, String username, String password) throws AuthenticationProviderException; diff --git a/spi/authentication-spi/src/main/java/org/keycloak/spi/authentication/AuthenticationProviderManager.java b/spi/authentication-spi/src/main/java/org/keycloak/spi/authentication/AuthenticationProviderManager.java index f5a0d9ce85..669153e4b8 100644 --- a/spi/authentication-spi/src/main/java/org/keycloak/spi/authentication/AuthenticationProviderManager.java +++ b/spi/authentication-spi/src/main/java/org/keycloak/spi/authentication/AuthenticationProviderManager.java @@ -17,7 +17,6 @@ import org.keycloak.util.ProviderLoader; * * Example of usage: AuthenticationProviderManager.getManager(realm).validateUser("joe", "password"); * - * * @author Marek Posolda */ public class AuthenticationProviderManager { @@ -50,6 +49,7 @@ public class AuthenticationProviderManager { public AuthResult validatePassword(String username, String password) { List configuredProviders = getConfiguredProviders(realm); + boolean userExists = false; for (AuthenticationProviderModel authProviderConfig : configuredProviders) { String providerName = authProviderConfig.getProviderName(); @@ -63,17 +63,20 @@ public class AuthenticationProviderManager { AuthResult currentResult = delegate.validatePassword(realm, authProviderConfig.getConfig(), username, password); logger.debugf("Authentication provider '%s' finished with '%s' for authentication of '%s'", delegate.getName(), currentResult.getAuthProviderStatus().toString(), username); - if (currentResult.getAuthProviderStatus() == AuthProviderStatus.SUCCESS || currentResult.getAuthProviderStatus() == AuthProviderStatus.FAILED) { + if (currentResult.getAuthProviderStatus() == AuthProviderStatus.SUCCESS) { return currentResult; + } else if (currentResult.getAuthProviderStatus() == AuthProviderStatus.INVALID_CREDENTIALS) { + userExists = true; } } catch (AuthenticationProviderException ape) { logger.warn(ape.getMessage(), ape); } } - logger.debugf("Not able to authenticate '%s' with any authentication provider", username); + AuthProviderStatus status = userExists ? AuthProviderStatus.INVALID_CREDENTIALS : AuthProviderStatus.USER_NOT_FOUND; + logger.debugf("Not able to authenticate '%s' with any authentication provider. Status: '%s'", username, status.toString()); - return new AuthResult(AuthProviderStatus.FAILED); + return new AuthResult(status); } public void updatePassword(String username, String password) throws AuthenticationProviderException { @@ -97,7 +100,7 @@ public class AuthenticationProviderManager { } } catch (AuthenticationProviderException ape) { // Rethrow it to upper layer - logger.warn("Failed to update password", ape); + logger.warn("Failed to update password: " + ape.getMessage()); throw ape; } } else {