From f95418dfc971329627a4834334cf095347f28d70 Mon Sep 17 00:00:00 2001 From: Stian Thorgersen Date: Tue, 12 Aug 2014 12:06:13 +0100 Subject: [PATCH] KEYCLOAK-592 Display login form with error message if trying to login with social provider where email already exists --- .../login/base/messages/messages.properties | 2 + .../DefaultKeycloakTransactionManager.java | 3 +- .../services/resources/SocialResource.java | 144 ++++++++++-------- .../resources/admin/UsersResource.java | 8 + .../testsuite/social/SocialLoginTest.java | 21 +++ 5 files changed, 117 insertions(+), 61 deletions(-) diff --git a/forms/common-themes/src/main/resources/theme/login/base/messages/messages.properties b/forms/common-themes/src/main/resources/theme/login/base/messages/messages.properties index e7a0eb9cc3..a1df56a075 100755 --- a/forms/common-themes/src/main/resources/theme/login/base/messages/messages.properties +++ b/forms/common-themes/src/main/resources/theme/login/base/messages/messages.properties @@ -51,6 +51,8 @@ successTotpRemoved=Google authenticator removed. usernameExists=Username already exists +socialEmailExists=User with email already exists. Please login to account management to link the account. + loginTitle=Log in to loginOauthTitle=Temporary access. loginOauthTitleHtml=Temporary access requested. Login to grant access. diff --git a/services/src/main/java/org/keycloak/services/DefaultKeycloakTransactionManager.java b/services/src/main/java/org/keycloak/services/DefaultKeycloakTransactionManager.java index a63ed9d712..75f609689f 100755 --- a/services/src/main/java/org/keycloak/services/DefaultKeycloakTransactionManager.java +++ b/services/src/main/java/org/keycloak/services/DefaultKeycloakTransactionManager.java @@ -64,7 +64,7 @@ public class DefaultKeycloakTransactionManager implements KeycloakTransactionMan exception = exception == null ? e : exception; } } - + active = false; if (exception != null) { throw exception; } @@ -87,6 +87,7 @@ public class DefaultKeycloakTransactionManager implements KeycloakTransactionMan exception = exception != null ? e : exception; } } + active = false; if (exception != null) { throw exception; } diff --git a/services/src/main/java/org/keycloak/services/resources/SocialResource.java b/services/src/main/java/org/keycloak/services/resources/SocialResource.java index 7867f1f74d..1c68403c5b 100755 --- a/services/src/main/java/org/keycloak/services/resources/SocialResource.java +++ b/services/src/main/java/org/keycloak/services/resources/SocialResource.java @@ -31,10 +31,12 @@ import org.keycloak.audit.Details; import org.keycloak.audit.Errors; import org.keycloak.audit.EventType; import org.keycloak.jose.jws.JWSInput; +import org.keycloak.login.LoginFormsProvider; import org.keycloak.models.AccountRoles; import org.keycloak.models.ClientModel; import org.keycloak.models.Constants; import org.keycloak.models.KeycloakSession; +import org.keycloak.models.ModelDuplicateException; import org.keycloak.models.RealmModel; import org.keycloak.models.SocialLinkModel; import org.keycloak.models.UserModel; @@ -46,6 +48,7 @@ import org.keycloak.services.managers.RealmManager; import org.keycloak.services.managers.TokenManager; import org.keycloak.services.resources.flows.Flows; import org.keycloak.services.resources.flows.OAuthFlows; +import org.keycloak.services.resources.flows.OAuthRedirect; import org.keycloak.services.resources.flows.Urls; import org.keycloak.social.AuthCallback; import org.keycloak.social.SocialAccessDeniedException; @@ -67,6 +70,7 @@ import javax.ws.rs.core.Response; import javax.ws.rs.core.UriBuilder; import javax.ws.rs.core.UriInfo; import java.io.IOException; +import java.net.URI; import java.net.URISyntaxException; import java.util.HashMap; import java.util.List; @@ -182,76 +186,85 @@ public class SocialResource { audit.detail(Details.USERNAME, socialUser.getId() + "@" + provider.getId()); - SocialLinkModel socialLink = new SocialLinkModel(provider.getId(), socialUser.getId(), socialUser.getUsername()); - UserModel user = session.users().getUserBySocialLink(socialLink, realm); + try { + SocialLinkModel socialLink = new SocialLinkModel(provider.getId(), socialUser.getId(), socialUser.getUsername()); + UserModel user = session.users().getUserBySocialLink(socialLink, realm); - // Check if user is already authenticated (this means linking social into existing user account) - String userId = initialRequest.getUser(); - if (userId != null) { - UserModel authenticatedUser = session.users().getUserById(userId, realm); + // Check if user is already authenticated (this means linking social into existing user account) + String userId = initialRequest.getUser(); + if (userId != null) { + UserModel authenticatedUser = session.users().getUserById(userId, realm); - audit.event(EventType.SOCIAL_LINK).user(userId); + audit.event(EventType.SOCIAL_LINK).user(userId); - if (user != null) { - audit.error(Errors.SOCIAL_ID_IN_USE); - return oauth.forwardToSecurityFailure("This social account is already linked to other user"); + if (user != null) { + audit.error(Errors.SOCIAL_ID_IN_USE); + return oauth.forwardToSecurityFailure("This social account is already linked to other user"); + } + + if (!authenticatedUser.isEnabled()) { + audit.error(Errors.USER_DISABLED); + return oauth.forwardToSecurityFailure("User is disabled"); + } + + if (!authenticatedUser.hasRole(realm.getApplicationByName(Constants.ACCOUNT_MANAGEMENT_APP).getRole(AccountRoles.MANAGE_ACCOUNT))) { + audit.error(Errors.NOT_ALLOWED); + return oauth.forwardToSecurityFailure("Insufficient permissions to link social account"); + } + + if (redirectUri == null) { + audit.error(Errors.INVALID_REDIRECT_URI); + return oauth.forwardToSecurityFailure("Unknown redirectUri"); + } + + session.users().addSocialLink(realm, authenticatedUser, socialLink); + logger.debug("Social provider " + provider.getId() + " linked with user " + authenticatedUser.getUsername()); + + audit.success(); + return Response.status(302).location(UriBuilder.fromUri(redirectUri).build()).build(); } - if (!authenticatedUser.isEnabled()) { + if (user == null) { + user = session.users().addUser(realm, KeycloakModelUtils.generateId()); + user.setEnabled(true); + user.setFirstName(socialUser.getFirstName()); + user.setLastName(socialUser.getLastName()); + user.setEmail(socialUser.getEmail()); + + if (realm.isUpdateProfileOnInitialSocialLogin()) { + user.addRequiredAction(UserModel.RequiredAction.UPDATE_PROFILE); + } + + session.users().addSocialLink(realm, user, socialLink); + + audit.clone().user(user).event(EventType.REGISTER) + .detail(Details.REGISTER_METHOD, "social@" + provider.getId()) + .detail(Details.EMAIL, socialUser.getEmail()) + .removeDetail("auth_method") + .success(); + } + + audit.user(user); + + if (!user.isEnabled()) { audit.error(Errors.USER_DISABLED); - return oauth.forwardToSecurityFailure("User is disabled"); + return oauth.forwardToSecurityFailure("Your account is not enabled."); } - if (!authenticatedUser.hasRole(realm.getApplicationByName(Constants.ACCOUNT_MANAGEMENT_APP).getRole(AccountRoles.MANAGE_ACCOUNT))) { - audit.error(Errors.NOT_ALLOWED); - return oauth.forwardToSecurityFailure("Insufficient permissions to link social account"); + String username = socialLink.getSocialUserId() + "@" + socialLink.getSocialProvider(); + + UserSessionModel userSession = session.sessions().createUserSession(realm, user, username, clientConnection.getRemoteAddr(), authMethod, false); + audit.session(userSession); + + Response response = oauth.processAccessCode(scope, state, redirectUri, client, user, userSession, audit); + if (session.getTransaction().isActive()) { + session.getTransaction().commit(); } - - if (redirectUri == null) { - audit.error(Errors.INVALID_REDIRECT_URI); - return oauth.forwardToSecurityFailure("Unknown redirectUri"); - } - - session.users().addSocialLink(realm, authenticatedUser, socialLink); - logger.debug("Social provider " + provider.getId() + " linked with user " + authenticatedUser.getUsername()); - - audit.success(); - return Response.status(302).location(UriBuilder.fromUri(redirectUri).build()).build(); + return response; + } catch (ModelDuplicateException e) { + // Assume email is the duplicate as there's nothing else atm + return returnToLogin(realm, client, initialRequest.getAttributes(), "socialEmailExists"); } - - if (user == null) { - user = session.users().addUser(realm, KeycloakModelUtils.generateId()); - user.setEnabled(true); - user.setFirstName(socialUser.getFirstName()); - user.setLastName(socialUser.getLastName()); - user.setEmail(socialUser.getEmail()); - - if (realm.isUpdateProfileOnInitialSocialLogin()) { - user.addRequiredAction(UserModel.RequiredAction.UPDATE_PROFILE); - } - - session.users().addSocialLink(realm, user, socialLink); - - audit.clone().user(user).event(EventType.REGISTER) - .detail(Details.REGISTER_METHOD, "social@" + provider.getId()) - .detail(Details.EMAIL, socialUser.getEmail()) - .removeDetail("auth_method") - .success(); - } - - audit.user(user); - - if (!user.isEnabled()) { - audit.error(Errors.USER_DISABLED); - return oauth.forwardToSecurityFailure("Your account is not enabled."); - } - - String username = socialLink.getSocialUserId() + "@" + socialLink.getSocialProvider(); - - UserSessionModel userSession = session.sessions().createUserSession(realm, user, username, clientConnection.getRemoteAddr(), authMethod, false); - audit.session(userSession); - - return oauth.processAccessCode(scope, state, redirectUri, client, user, userSession, audit); } @GET @@ -307,6 +320,17 @@ public class SocialResource { } } + private Response returnToLogin(RealmModel realm, ClientModel client, Map attributes, String error) { + MultivaluedMap q = new MultivaluedMapImpl(); + for (Entry e : attributes.entrySet()) { + q.add(e.getKey(), e.getValue()); + } + return Flows.forms(session, realm, client, uriInfo) + .setQueryParams(q) + .setError(error) + .createLogin(); + } + private Map getQueryParams() { Map queryParams = new HashMap(); for (Entry> e : uriInfo.getQueryParameters().entrySet()) { diff --git a/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java b/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java index f39e28f832..278bc29736 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java @@ -106,6 +106,10 @@ public class UsersResource { } updateUserFromRep(user, rep); + if (session.getTransaction().isActive()) { + session.getTransaction().commit(); + } + return Response.noContent().build(); } catch (ModelDuplicateException e) { return Flows.errors().exists("User exists with same username or email"); @@ -128,6 +132,10 @@ public class UsersResource { UserModel user = session.users().addUser(realm, rep.getUsername()); updateUserFromRep(user, rep); + if (session.getTransaction().isActive()) { + session.getTransaction().commit(); + } + return Response.created(uriInfo.getAbsolutePathBuilder().path(user.getUsername()).build()).build(); } catch (ModelDuplicateException e) { return Flows.errors().exists("User exists with same username or email"); diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/social/SocialLoginTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/social/SocialLoginTest.java index 8b5df039dd..efa7cff54d 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/social/SocialLoginTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/social/SocialLoginTest.java @@ -156,6 +156,27 @@ public class SocialLoginTest { events.expectLogin().user(userId).detail(Details.USERNAME, "1@dummy").detail(Details.AUTH_METHOD, "social@dummy").assertEvent(); } + @Test + public void loginEmailExists() throws Exception { + loginSuccess(); + oauth.openLogout(); + events.clear(); + + loginPage.open(); + + loginPage.clickSocial("dummy"); + + driver.findElement(By.id("id")).sendKeys("2"); + driver.findElement(By.id("username")).sendKeys("dummy-user2"); + driver.findElement(By.id("firstname")).sendKeys("Bob2"); + driver.findElement(By.id("lastname")).sendKeys("Builder2"); + driver.findElement(By.id("email")).sendKeys("bob@builder.com"); + driver.findElement(By.id("login")).click(); + + Assert.assertTrue(loginPage.isCurrent()); + Assert.assertEquals("User with email already exists. Please login to account management to link the account.", loginPage.getError()); + } + @Test public void loginCancelled() throws Exception { loginPage.open();