From 0768bcc452c76b4dc4922613e729de485f65d7db Mon Sep 17 00:00:00 2001 From: mposolda Date: Tue, 1 Mar 2016 10:11:26 +0100 Subject: [PATCH] KEYCLOAK-2542 User can't set password for account created over social login if UserFedarationProvider used --- .../models/UserFederationManager.java | 4 +- .../models/utils/KeycloakModelUtils.java | 8 +++ .../model/AccountFederatedIdentityBean.java | 2 +- .../services/resources/AccountService.java | 28 +++++++-- .../DummyUserFederationProvider.java | 22 +++++-- .../broker/AbstractIdentityProviderTest.java | 4 ++ .../AbstractKeycloakIdentityProviderTest.java | 63 +++++++++++++++++++ 7 files changed, 118 insertions(+), 13 deletions(-) diff --git a/server-spi/src/main/java/org/keycloak/models/UserFederationManager.java b/server-spi/src/main/java/org/keycloak/models/UserFederationManager.java index 45077e236a..8f809f3389 100755 --- a/server-spi/src/main/java/org/keycloak/models/UserFederationManager.java +++ b/server-spi/src/main/java/org/keycloak/models/UserFederationManager.java @@ -55,9 +55,7 @@ public class UserFederationManager implements UserProvider { } protected UserFederationProvider getFederationProvider(UserFederationProviderModel model) { - UserFederationProviderFactory factory = (UserFederationProviderFactory)session.getKeycloakSessionFactory().getProviderFactory(UserFederationProvider.class, model.getProviderName()); - return factory.getInstance(session, model); - + return KeycloakModelUtils.getFederationProviderInstance(session, model); } @Override diff --git a/server-spi/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java b/server-spi/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java index ff5c38e1d6..c4906d659e 100755 --- a/server-spi/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java +++ b/server-spi/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java @@ -35,6 +35,8 @@ import org.keycloak.models.RoleModel; import org.keycloak.models.ScopeContainerModel; import org.keycloak.models.UserCredentialModel; import org.keycloak.models.UserFederationMapperModel; +import org.keycloak.models.UserFederationProvider; +import org.keycloak.models.UserFederationProviderFactory; import org.keycloak.models.UserFederationProviderModel; import org.keycloak.models.UserModel; import org.keycloak.representations.idm.CertificateRepresentation; @@ -406,6 +408,12 @@ public final class KeycloakModelUtils { return mapperModel; } + public static UserFederationProvider getFederationProviderInstance(KeycloakSession session, UserFederationProviderModel model) { + UserFederationProviderFactory factory = (UserFederationProviderFactory)session.getKeycloakSessionFactory().getProviderFactory(UserFederationProvider.class, model.getProviderName()); + return factory.getInstance(session, model); + + } + // END USER FEDERATION RELATED STUFF public static String toLowerCaseSafe(String str) { diff --git a/services/src/main/java/org/keycloak/forms/account/freemarker/model/AccountFederatedIdentityBean.java b/services/src/main/java/org/keycloak/forms/account/freemarker/model/AccountFederatedIdentityBean.java index ec02ebafa3..a20810e8a0 100755 --- a/services/src/main/java/org/keycloak/forms/account/freemarker/model/AccountFederatedIdentityBean.java +++ b/services/src/main/java/org/keycloak/forms/account/freemarker/model/AccountFederatedIdentityBean.java @@ -79,7 +79,7 @@ public class AccountFederatedIdentityBean { this.identities = new LinkedList(orderedSet); // Removing last social provider is not possible if you don't have other possibility to authenticate - this.removeLinkPossible = availableIdentities > 1 || user.getFederationLink() != null || AccountService.isPasswordSet(user); + this.removeLinkPossible = availableIdentities > 1 || user.getFederationLink() != null || AccountService.isPasswordSet(session, realm, user); } private FederatedIdentityModel getIdentity(Set identities, String providerId) { diff --git a/services/src/main/java/org/keycloak/services/resources/AccountService.java b/services/src/main/java/org/keycloak/services/resources/AccountService.java index 324136361f..4907707d90 100755 --- a/services/src/main/java/org/keycloak/services/resources/AccountService.java +++ b/services/src/main/java/org/keycloak/services/resources/AccountService.java @@ -30,16 +30,20 @@ import org.keycloak.models.ClientModel; import org.keycloak.models.ClientSessionModel; import org.keycloak.models.FederatedIdentityModel; import org.keycloak.models.IdentityProviderModel; +import org.keycloak.models.KeycloakSession; import org.keycloak.models.ModelDuplicateException; import org.keycloak.models.ModelException; import org.keycloak.models.ModelReadOnlyException; import org.keycloak.models.RealmModel; import org.keycloak.models.UserCredentialModel; import org.keycloak.models.UserCredentialValueModel; +import org.keycloak.models.UserFederationProvider; +import org.keycloak.models.UserFederationProviderModel; import org.keycloak.models.UserModel; import org.keycloak.models.UserSessionModel; import org.keycloak.models.utils.CredentialValidation; import org.keycloak.models.utils.FormMessage; +import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.models.utils.ModelToRepresentation; import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.protocol.oidc.utils.RedirectUtils; @@ -298,7 +302,7 @@ public class AccountService extends AbstractSecuredLocalService { @GET public Response passwordPage() { if (auth != null) { - account.setPasswordSet(isPasswordSet(auth.getUser())); + account.setPasswordSet(isPasswordSet(session, realm, auth.getUser())); } return forwardToPage("password", AccountPages.PASSWORD); @@ -601,7 +605,7 @@ public class AccountService extends AbstractSecuredLocalService { csrfCheck(formData); UserModel user = auth.getUser(); - boolean requireCurrent = isPasswordSet(user); + boolean requireCurrent = isPasswordSet(session, realm, user); account.setPasswordSet(requireCurrent); String password = formData.getFirst("password"); @@ -723,7 +727,7 @@ public class AccountService extends AbstractSecuredLocalService { if (link != null) { // Removing last social provider is not possible if you don't have other possibility to authenticate - if (session.users().getFederatedIdentities(user, realm).size() > 1 || user.getFederationLink() != null || isPasswordSet(user)) { + if (session.users().getFederatedIdentities(user, realm).size() > 1 || user.getFederationLink() != null || isPasswordSet(session, realm, user)) { session.users().removeFederatedIdentity(realm, user, providerId); logger.debugv("Social provider {0} removed successfully from user {1}", providerId, user.getUsername()); @@ -758,11 +762,25 @@ public class AccountService extends AbstractSecuredLocalService { return Urls.accountBase(uriInfo.getBaseUri()).path("/").build(realm.getName()); } - public static boolean isPasswordSet(UserModel user) { + public static boolean isPasswordSet(KeycloakSession session, RealmModel realm, UserModel user) { boolean passwordSet = false; + // See if password is set for user on linked UserFederationProvider if (user.getFederationLink() != null) { - passwordSet = true; + + UserFederationProvider federationProvider = null; + for (UserFederationProviderModel fedProviderModel : realm.getUserFederationProviders()) { + if (fedProviderModel.getId().equals(user.getFederationLink())) { + federationProvider = KeycloakModelUtils.getFederationProviderInstance(session, fedProviderModel); + } + } + + if (federationProvider != null) { + Set supportedCreds = federationProvider.getSupportedCredentialTypes(user); + if (supportedCreds.contains(UserCredentialModel.PASSWORD)) { + passwordSet = true; + } + } } for (UserCredentialValueModel c : user.getCredentialsDirectly()) { diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/DummyUserFederationProvider.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/DummyUserFederationProvider.java index 974e5c2c26..50ff01bb8f 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/DummyUserFederationProvider.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/DummyUserFederationProvider.java @@ -25,8 +25,10 @@ import org.keycloak.models.UserCredentialModel; import org.keycloak.models.UserFederationProvider; import org.keycloak.models.UserModel; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -97,27 +99,39 @@ public class DummyUserFederationProvider implements UserFederationProvider { @Override public boolean isValid(RealmModel realm, UserModel local) { - return false; + String username = local.getUsername(); + return users.containsKey(username); } @Override public Set getSupportedCredentialTypes(UserModel user) { - return Collections.emptySet(); + // Just user "test-user" is able to validate password with this federationProvider + if (user.getUsername().equals("test-user")) { + return Collections.singleton(UserCredentialModel.PASSWORD); + } else { + return Collections.emptySet(); + } } @Override public Set getSupportedCredentialTypes() { - return Collections.emptySet(); + return Collections.singleton(UserCredentialModel.PASSWORD); } @Override public boolean validCredentials(RealmModel realm, UserModel user, List input) { + if (user.getUsername().equals("test-user") && input.size() == 1) { + UserCredentialModel password = input.get(0); + if (password.getType().equals(UserCredentialModel.PASSWORD)) { + return "secret".equals(password.getValue()); + } + } return false; } @Override public boolean validCredentials(RealmModel realm, UserModel user, UserCredentialModel... input) { - return false; + return validCredentials(realm, user, Arrays.asList(input)); } @Override diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java index 241116e751..51a9044d8b 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java @@ -38,6 +38,7 @@ import org.keycloak.testsuite.broker.util.UserSessionStatusServlet; import org.keycloak.testsuite.broker.util.UserSessionStatusServlet.UserSessionStatus; import org.keycloak.testsuite.pages.AccountFederatedIdentityPage; import org.keycloak.testsuite.pages.AccountPasswordPage; +import org.keycloak.testsuite.pages.AccountUpdateProfilePage; import org.keycloak.testsuite.pages.LoginPage; import org.keycloak.testsuite.pages.LoginUpdateProfilePage; import org.keycloak.testsuite.pages.OAuthGrantPage; @@ -103,6 +104,9 @@ public abstract class AbstractIdentityProviderTest { @WebResource protected OAuthGrantPage grantPage; + @WebResource + AccountUpdateProfilePage accountUpdateProfilePage; + @WebResource protected AccountPasswordPage changePasswordPage; diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractKeycloakIdentityProviderTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractKeycloakIdentityProviderTest.java index 402f91ee90..b5f21a0876 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractKeycloakIdentityProviderTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractKeycloakIdentityProviderTest.java @@ -19,6 +19,7 @@ package org.keycloak.testsuite.broker; import java.io.IOException; import java.net.URI; +import java.util.HashMap; import java.util.Set; import javax.mail.MessagingException; @@ -39,9 +40,11 @@ import org.keycloak.models.FederatedIdentityModel; import org.keycloak.models.IdentityProviderModel; import org.keycloak.models.RealmModel; import org.keycloak.models.RoleModel; +import org.keycloak.models.UserFederationProviderModel; import org.keycloak.models.UserModel; import org.keycloak.representations.idm.IdentityProviderRepresentation; import org.keycloak.services.Urls; +import org.keycloak.testsuite.DummyUserFederationProviderFactory; import org.keycloak.testsuite.broker.util.UserSessionStatusServlet; import org.openqa.selenium.By; import org.openqa.selenium.NoSuchElementException; @@ -531,4 +534,64 @@ public abstract class AbstractKeycloakIdentityProviderTest extends AbstractIdent protected abstract void doAssertTokenRetrieval(String pageSource); + @Test + public void testWithLinkedFederationProvider() throws Exception { + // Add federationProvider to realm. It's configured with sync registrations + RealmModel realm = getRealm(); + UserFederationProviderModel dummyModel = realm.addUserFederationProvider(DummyUserFederationProviderFactory.PROVIDER_NAME, new HashMap(), 1, "test-dummy", -1, -1, 0); + setUpdateProfileFirstLogin(IdentityProviderRepresentation.UPFLM_OFF); + + brokerServerRule.stopSession(session, true); + session = brokerServerRule.startSession(); + + try { + // Login as user "test-user" to account management. + authenticateWithIdentityProvider(getIdentityProviderModel(), "test-user", false); + changePasswordPage.realm("realm-with-broker"); + changePasswordPage.open(); + assertTrue(changePasswordPage.isCurrent()); + + // Assert changing password with old password "secret" as this is the password from federationProvider (See DummyUserFederationProvider) + changePasswordPage.changePassword("new-password", "new-password"); + Assert.assertEquals("Please specify password.", accountUpdateProfilePage.getError()); + + changePasswordPage.changePassword("bad", "new-password", "new-password"); + Assert.assertEquals("Invalid existing password.", accountUpdateProfilePage.getError()); + + changePasswordPage.changePassword("secret", "new-password", "new-password"); + Assert.assertEquals("Your password has been updated.", accountUpdateProfilePage.getSuccess()); + + // Logout + driver.navigate().to("http://localhost:8081/test-app/logout"); + + + // Login as user "test-user-noemail" . + authenticateWithIdentityProvider(getIdentityProviderModel(), "test-user-noemail", false); + changePasswordPage.open(); + assertTrue(changePasswordPage.isCurrent()); + + // Assert old password is not required as federationProvider doesn't have it for this user + changePasswordPage.changePassword("new-password", "new-password"); + Assert.assertEquals("Your password has been updated.", accountUpdateProfilePage.getSuccess()); + + // Now it is required as it's set on model + changePasswordPage.changePassword("new-password2", "new-password2"); + Assert.assertEquals("Please specify password.", accountUpdateProfilePage.getError()); + + changePasswordPage.changePassword("new-password", "new-password2", "new-password2"); + Assert.assertEquals("Your password has been updated.", accountUpdateProfilePage.getSuccess()); + + // Logout + driver.navigate().to("http://localhost:8081/test-app/logout"); + } finally { + + // remove dummy federation provider for this realm + realm = getRealm(); + realm.removeUserFederationProvider(dummyModel); + + brokerServerRule.stopSession(session, true); + session = brokerServerRule.startSession(); + } + } + }