keycloak-uncached

Merge pull request #1062 from velias/KEYCLOAK-1053 KEYCLOAK-1053

3/20/2015 1:37:16 AM

Details

diff --git a/events/api/src/main/java/org/keycloak/events/Errors.java b/events/api/src/main/java/org/keycloak/events/Errors.java
index 282b5e4..7a4404d 100755
--- a/events/api/src/main/java/org/keycloak/events/Errors.java
+++ b/events/api/src/main/java/org/keycloak/events/Errors.java
@@ -41,7 +41,6 @@ public interface Errors {
     String NOT_ALLOWED = "not_allowed";
 
     String FEDERATED_IDENTITY_EMAIL_EXISTS = "federated_identity_email_exists";
-    String FEDERATED_IDENTITY_REGISTRATION_EMAIL_MISSING = "federated_identity_registration_email_missing";
     String FEDERATED_IDENTITY_USERNAME_EXISTS = "federated_identity_username_exists";
     String SSL_REQUIRED = "ssl_required";
 
diff --git a/forms/common-themes/src/main/resources/theme/login/base/messages/messages_de.properties b/forms/common-themes/src/main/resources/theme/login/base/messages/messages_de.properties
index 70d8aba..c4c2bef 100644
--- a/forms/common-themes/src/main/resources/theme/login/base/messages/messages_de.properties
+++ b/forms/common-themes/src/main/resources/theme/login/base/messages/messages_de.properties
@@ -79,7 +79,6 @@ invalidTotpMessage=Ung�ltiger One-time Code.
 usernameExistsMessage=Benutzermane exisitert bereits.
 emailExistsMessage=E-Mail existiert bereits.
 
-federatedIdentityRegistrationEmailMissingMessage=Die E-Mail Adresse ist nicht vorhanden. Bitte verwenden Sie einen anderen Provider um das Benutzerkonto zu erstellen.
 federatedIdentityEmailExistsMessage=Es exisitert bereits ein Benutzer mit dieser E-Mail Adresse. Bitte melden Sie sich bei der Benutzerverwaltung an um das Benutzerkonto zu verkn�pfen.
 federatedIdentityUsernameExistsMessage=Es exisitert bereits ein Benutzer mit diesem Benutzernamen. Bitte melden Sie sich bei der Benutzerverwaltung an um das Benutzerkonto zu verkn�pfen.
 
diff --git a/forms/common-themes/src/main/resources/theme/login/base/messages/messages_en.properties b/forms/common-themes/src/main/resources/theme/login/base/messages/messages_en.properties
index a33d676..afbe584 100755
--- a/forms/common-themes/src/main/resources/theme/login/base/messages/messages_en.properties
+++ b/forms/common-themes/src/main/resources/theme/login/base/messages/messages_en.properties
@@ -143,4 +143,3 @@ backToApplication=« Back to Application
 missingParameterMessage=Missing parameters\: {0}
 clientNotFoundMessage=Client not found.
 invalidParameterMessage=Invalid parameter\: {0}
-federatedIdentityRegistrationEmailMissingMessage=Email is not provided. Use another provider to create account please.
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 f37b837..ffc61d3 100755
--- a/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java
+++ b/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java
@@ -20,6 +20,7 @@ import org.keycloak.models.RequiredCredentialModel;
 import org.keycloak.models.RoleModel;
 import org.keycloak.models.UserCredentialModel;
 import org.keycloak.models.UserModel;
+import org.keycloak.models.UserModel.RequiredAction;
 import org.keycloak.models.UserSessionModel;
 import org.keycloak.models.utils.KeycloakModelUtils;
 import org.keycloak.protocol.LoginProtocol;
@@ -30,6 +31,7 @@ import org.keycloak.services.resources.LoginActionsService;
 import org.keycloak.services.resources.RealmsResource;
 import org.keycloak.services.resources.flows.Flows;
 import org.keycloak.services.util.CookieHelper;
+import org.keycloak.services.validation.Validation;
 import org.keycloak.util.Time;
 
 import javax.ws.rs.core.Cookie;
@@ -43,6 +45,7 @@ import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Set;
+import java.util.Iterator;
 
 /**
  * Stateless object that manages authentication
@@ -366,17 +369,28 @@ public class AuthenticationManager {
 
         Set<UserModel.RequiredAction> requiredActions = user.getRequiredActions();
         if (!requiredActions.isEmpty()) {
-            UserModel.RequiredAction action = user.getRequiredActions().iterator().next();
-            accessCode.setRequiredAction(action);
-
-            LoginFormsProvider loginFormsProvider = Flows.forms(session, realm, client, uriInfo, request.getHttpHeaders()).setClientSessionCode(accessCode.getCode()).setUser(user);
-            if (action.equals(UserModel.RequiredAction.VERIFY_EMAIL)) {
-                event.clone().event(EventType.SEND_VERIFY_EMAIL).detail(Details.EMAIL, user.getEmail()).success();
-                LoginActionsService.createActionCookie(realm, uriInfo, clientConnection, userSession.getId());
+            Iterator<RequiredAction> i = user.getRequiredActions().iterator();
+            UserModel.RequiredAction action = i.next();
+            
+            if (action.equals(UserModel.RequiredAction.VERIFY_EMAIL) && Validation.isEmpty(user.getEmail())) {
+                if (i.hasNext())
+                    action = i.next();
+                else
+                    action = null;
             }
 
-            return loginFormsProvider
-                    .createResponse(action);
+            if (action != null) {
+                accessCode.setRequiredAction(action);
+
+                LoginFormsProvider loginFormsProvider = Flows.forms(session, realm, client, uriInfo, request.getHttpHeaders()).setClientSessionCode(accessCode.getCode())
+                        .setUser(user);
+                if (action.equals(UserModel.RequiredAction.VERIFY_EMAIL)) {
+                    event.clone().event(EventType.SEND_VERIFY_EMAIL).detail(Details.EMAIL, user.getEmail()).success();
+                    LoginActionsService.createActionCookie(realm, uriInfo, clientConnection, userSession.getId());
+                }
+
+                return loginFormsProvider.createResponse(action);
+            }
         }
 
         if (!isResource) {
diff --git a/services/src/main/java/org/keycloak/services/messages/Messages.java b/services/src/main/java/org/keycloak/services/messages/Messages.java
index a1cc072..cf59818 100755
--- a/services/src/main/java/org/keycloak/services/messages/Messages.java
+++ b/services/src/main/java/org/keycloak/services/messages/Messages.java
@@ -171,6 +171,4 @@ public class Messages {
     public static final String CLIENT_NOT_FOUND = "clientNotFoundMessage";
 
     public static final String INVALID_PARAMETER = "invalidParameterMessage";
-
-    public static final String FEDERATED_IDENTITY_REGISTRATION_EMAIL_MISSING = "federatedIdentityRegistrationEmailMissingMessage";
 }
diff --git a/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java b/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java
index 2b3402b..cd1ac13 100755
--- a/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java
+++ b/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java
@@ -49,6 +49,7 @@ import org.keycloak.services.managers.EventsManager;
 import org.keycloak.services.messages.Messages;
 import org.keycloak.services.resources.flows.Flows;
 import org.keycloak.services.resources.flows.Urls;
+import org.keycloak.services.validation.Validation;
 import org.keycloak.social.SocialIdentityProvider;
 
 import javax.ws.rs.Consumes;
@@ -521,15 +522,10 @@ public class IdentityBrokerService {
         }
 
         String username = updatedIdentity.getUsername();
-        if (this.realmModel.isRegistrationEmailAsUsername()) {
+        if (this.realmModel.isRegistrationEmailAsUsername() && !Validation.isEmpty(updatedIdentity.getEmail())) {
             username = updatedIdentity.getEmail();
-            if (username == null || username.trim().length() == 0) {
-                fireErrorEvent(Errors.FEDERATED_IDENTITY_REGISTRATION_EMAIL_MISSING);
-                throw new IdentityBrokerException(Messages.FEDERATED_IDENTITY_REGISTRATION_EMAIL_MISSING);
-                // TODO KEYCLOAK-1053 (ask user to enter email address) should be implemented instead of plain exception as better solution for this case
-            }
-            username = username.trim();
-        } else if (username != null) {
+        } 
+        if (username != null) {
             username = username.trim();
         }
 
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 d20309a..aeeb899 100644
--- a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java
+++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java
@@ -32,6 +32,7 @@ import org.keycloak.models.IdentityProviderModel;
 import org.keycloak.models.KeycloakSession;
 import org.keycloak.models.RealmModel;
 import org.keycloak.models.UserModel;
+import org.keycloak.models.UserModel.RequiredAction;
 import org.keycloak.representations.IDToken;
 import org.keycloak.services.resources.flows.Urls;
 import org.keycloak.testsuite.OAuthClient;
@@ -70,7 +71,6 @@ import static com.thoughtworks.selenium.SeleneseTestBase.fail;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
 /**
@@ -127,7 +127,7 @@ public abstract class AbstractIdentityProviderTest {
     public void testSuccessfulAuthentication() {
         IdentityProviderModel identityProviderModel = getIdentityProviderModel();
 
-        assertSuccessfulAuthentication(identityProviderModel, "test-user");
+        assertSuccessfulAuthentication(identityProviderModel, "test-user", "new@email.com");
     }
 
     @Test
@@ -135,7 +135,29 @@ public abstract class AbstractIdentityProviderTest {
         IdentityProviderModel identityProviderModel = getIdentityProviderModel();
         identityProviderModel.setUpdateProfileFirstLogin(false);
 
-        assertSuccessfulAuthentication(identityProviderModel, "test-user");
+        assertSuccessfulAuthentication(identityProviderModel, "test-user", "test-user@localhost");
+    }
+
+    /**
+     * Test for KEYCLOAK-1053 - verify email action is not performed if email is not provided, login is normal, but action stays in set to be performed later
+     */
+    @Test
+    public void testSuccessfulAuthenticationWithoutUpdateProfile_emailNotProvided_emailVerifyEnabled() {
+        getRealm().setVerifyEmail(true);
+        brokerServerRule.stopSession(this.session, true);
+        this.session = brokerServerRule.startSession();
+
+        try {
+            IdentityProviderModel identityProviderModel = getIdentityProviderModel();
+            identityProviderModel.setUpdateProfileFirstLogin(false);
+
+            UserModel federatedUser = assertSuccessfulAuthentication(identityProviderModel, "test-user-noemail", null);
+
+            federatedUser.getRequiredActions().contains(RequiredAction.VERIFY_EMAIL);
+
+        } finally {
+            getRealm().setVerifyEmail(false);
+        }
     }
 
     @Test
@@ -163,7 +185,7 @@ public abstract class AbstractIdentityProviderTest {
 
             assertEquals("test-user@localhost", federatedUser.getUsername());
 
-            doAssertFederatedUser(federatedUser, identityProviderModel);
+            doAssertFederatedUser(federatedUser, identityProviderModel, "test-user@localhost");
 
             Set<FederatedIdentityModel> federatedIdentities = this.session.users().getFederatedIdentities(federatedUser, realm);
 
@@ -196,18 +218,38 @@ public abstract class AbstractIdentityProviderTest {
 
             authenticateWithIdentityProvider(identityProviderModel, "test-user-noemail");
 
+            // check correct user is created with username from provider as email is not available
             RealmModel realm = getRealm();
-            UserModel federatedUser = session.users().getUserByUsername("test-user-noemail", realm);
-            assertNull(federatedUser);
+            UserModel federatedUser = getFederatedUser();
+            assertNotNull(federatedUser);
 
-            // assert page is shown with correct error message
-            assertEquals("Email is not provided. Use another provider to create account please.", this.driver.findElement(By.className("kc-feedback-text")).getText());
+            doAssertFederatedUserNoEmail(federatedUser);
+
+            Set<FederatedIdentityModel> federatedIdentities = this.session.users().getFederatedIdentities(federatedUser, realm);
+
+            assertEquals(1, federatedIdentities.size());
+
+            FederatedIdentityModel federatedIdentityModel = federatedIdentities.iterator().next();
+
+            assertEquals(getProviderId(), federatedIdentityModel.getIdentityProvider());
+
+            driver.navigate().to("http://localhost:8081/test-app/logout");
+            driver.navigate().to("http://localhost:8081/test-app");
+
+            assertTrue(this.driver.getCurrentUrl().startsWith("http://localhost:8081/auth/realms/realm-with-broker/protocol/openid-connect/auth"));
 
         } finally {
             getRealm().setRegistrationEmailAsUsername(false);
         }
     }
 
+    protected void doAssertFederatedUserNoEmail(UserModel federatedUser) {
+        assertEquals("test-user-noemail", federatedUser.getUsername());
+        assertEquals(null, federatedUser.getEmail());
+        assertEquals("Test", federatedUser.getFirstName());
+        assertEquals("User", federatedUser.getLastName());
+    }
+
     @Test
     public void testDisabled() {
         IdentityProviderModel identityProviderModel = getIdentityProviderModel();
@@ -508,7 +550,7 @@ public abstract class AbstractIdentityProviderTest {
 
     protected abstract void doAssertTokenRetrieval(String pageSource);
 
-    private void assertSuccessfulAuthentication(IdentityProviderModel identityProviderModel, String username) {
+    private UserModel assertSuccessfulAuthentication(IdentityProviderModel identityProviderModel, String username, String expectedEmail) {
         authenticateWithIdentityProvider(identityProviderModel, username);
 
         // authenticated and redirected to app
@@ -518,7 +560,7 @@ public abstract class AbstractIdentityProviderTest {
 
         assertNotNull(federatedUser);
 
-        doAssertFederatedUser(federatedUser, identityProviderModel);
+        doAssertFederatedUser(federatedUser, identityProviderModel, expectedEmail);
 
         RealmModel realm = getRealm();
 
@@ -535,6 +577,7 @@ public abstract class AbstractIdentityProviderTest {
         driver.navigate().to("http://localhost:8081/test-app");
 
         assertTrue(this.driver.getCurrentUrl().startsWith("http://localhost:8081/auth/realms/realm-with-broker/protocol/openid-connect/auth"));
+        return federatedUser;
     }
 
     private void authenticateWithIdentityProvider(IdentityProviderModel identityProviderModel, String username) {
@@ -593,17 +636,16 @@ public abstract class AbstractIdentityProviderTest {
         return this.session.realms().getRealm("realm-with-broker");
     }
 
-    protected void doAssertFederatedUser(UserModel federatedUser, IdentityProviderModel identityProviderModel) {
+    protected void doAssertFederatedUser(UserModel federatedUser, IdentityProviderModel identityProviderModel, String expectedEmail) {
         if (identityProviderModel.isUpdateProfileFirstLogin()) {
-            String userEmail = "new@email.com";
             String userFirstName = "New first";
             String userLastName = "New last";
 
-            assertEquals(userEmail, federatedUser.getEmail());
+            assertEquals(expectedEmail, federatedUser.getEmail());
             assertEquals(userFirstName, federatedUser.getFirstName());
             assertEquals(userLastName, federatedUser.getLastName());
         } else {
-            assertEquals("test-user@localhost", federatedUser.getEmail());
+            assertEquals(expectedEmail, federatedUser.getEmail());
             assertEquals("Test", federatedUser.getFirstName());
             assertEquals("User", federatedUser.getLastName());
         }
diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/SAMLKeyCloakServerBrokerBasicTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/SAMLKeyCloakServerBrokerBasicTest.java
index 7783665..90edbda 100755
--- a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/SAMLKeyCloakServerBrokerBasicTest.java
+++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/SAMLKeyCloakServerBrokerBasicTest.java
@@ -45,17 +45,27 @@ public class SAMLKeyCloakServerBrokerBasicTest extends AbstractIdentityProviderT
     }
 
     @Override
-    protected void doAssertFederatedUser(UserModel federatedUser, IdentityProviderModel identityProviderModel) {
+    protected void doAssertFederatedUser(UserModel federatedUser, IdentityProviderModel identityProviderModel, String expectedEmail) {
         if (identityProviderModel.isUpdateProfileFirstLogin()) {
-            super.doAssertFederatedUser(federatedUser, identityProviderModel);
+            super.doAssertFederatedUser(federatedUser, identityProviderModel, expectedEmail);
         } else {
-            assertEquals("test-user@localhost", federatedUser.getEmail());
+            if (expectedEmail == null)
+                expectedEmail = "";
+            assertEquals(expectedEmail, federatedUser.getEmail());
             assertNull(federatedUser.getFirstName());
             assertNull(federatedUser.getLastName());
         }
     }
 
     @Override
+    protected void doAssertFederatedUserNoEmail(UserModel federatedUser) {
+        assertEquals("", federatedUser.getUsername());
+        assertEquals("", federatedUser.getEmail());
+        assertEquals(null, federatedUser.getFirstName());
+        assertEquals(null, federatedUser.getLastName());
+    }
+
+    @Override
     protected void doAssertTokenRetrieval(String pageSource) {
         try {
             SAML2Request saml2Request = new SAML2Request();
diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/SAMLKeyCloakServerBrokerWithSignatureTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/SAMLKeyCloakServerBrokerWithSignatureTest.java
index e387097..b55baae 100755
--- a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/SAMLKeyCloakServerBrokerWithSignatureTest.java
+++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/SAMLKeyCloakServerBrokerWithSignatureTest.java
@@ -45,17 +45,27 @@ public class SAMLKeyCloakServerBrokerWithSignatureTest extends AbstractIdentityP
     }
 
     @Override
-    protected void doAssertFederatedUser(UserModel federatedUser, IdentityProviderModel identityProviderModel) {
+    protected void doAssertFederatedUser(UserModel federatedUser, IdentityProviderModel identityProviderModel, String expectedEmail) {
         if (identityProviderModel.isUpdateProfileFirstLogin()) {
-            super.doAssertFederatedUser(federatedUser, identityProviderModel);
+            super.doAssertFederatedUser(federatedUser, identityProviderModel, expectedEmail);
         } else {
-            assertEquals("test-user@localhost", federatedUser.getEmail());
+            if (expectedEmail == null)
+                expectedEmail = "";
+            assertEquals(expectedEmail, federatedUser.getEmail());
             assertNull(federatedUser.getFirstName());
             assertNull(federatedUser.getLastName());
         }
     }
 
     @Override
+    protected void doAssertFederatedUserNoEmail(UserModel federatedUser) {
+        assertEquals("", federatedUser.getUsername());
+        assertEquals("", federatedUser.getEmail());
+        assertEquals(null, federatedUser.getFirstName());
+        assertEquals(null, federatedUser.getLastName());
+    }
+
+    @Override
     protected void doAssertTokenRetrieval(String pageSource) {
         try {
             SAML2Request saml2Request = new SAML2Request();