Shib: pick first email if multiple asserted #2512 #2939

- Put email addresses throught the same "find single value" logic
  originally developed in #1608 for multiple first and last names.
- Add `@ValidateEmail` to the "email" field on AuthenticatedUser to
  match BuiltinUser.
- Add null check added to EmailValidator to make it testable.
- Add `INVALID_EMAIL` and `MISSING_REQUIRED_ATTR` modes for Shib testing
  in dev.
- Remove red warning when TestShib doesn't provide "mail" attribute.
- Catch authSvc.createAuthenticatedUser exceptions and handle errors
  better.
- Reformat code (getPrettyFacesHomePageString seems ok).

src/main/java/edu/harvard/iq/dataverse/EMailValidator.java

@@ -33,9 +33,16 @@ public static boolean isEmailValid(String value, ConstraintValidatorContext cont
             //we'll let someone else decide if it's required
             return true;
         }
+        /**
+         * @todo Why are we validating the trimmed value rather than the value
+         * itself? Which are we persisting to the database, the trimmed value or
+         * the non-trimmed value?
+         */
         boolean isValid = EmailValidator.getInstance().isValid(value.trim());
         if (!isValid) {
-            context.buildConstraintViolationWithTemplate(value + " is not a valid email address.").addConstraintViolation();
+            if (context != null) {
+                context.buildConstraintViolationWithTemplate(value + " is not a valid email address.").addConstraintViolation();
+            }
             return false;
         }
         return true;

src/main/java/edu/harvard/iq/dataverse/Shib.java

@@ -34,6 +34,7 @@
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import javax.ejb.EJB;
+import javax.ejb.EJBException;
 import javax.faces.application.FacesMessage;
 import javax.faces.context.ExternalContext;
 import javax.faces.context.FacesContext;
@@ -217,6 +218,7 @@ public class Shib implements java.io.Serializable {
 //    private boolean debug = false;
     private String emailAddress;
     private boolean useHeaders;
+    private final String testShibIdpEntityId = "https://idp.testshib.org/idp/shibboleth";
 
     public enum State {
 
@@ -278,18 +280,36 @@ public void init() {
                 lastName = betterLastName;
             }
         }
+        String emailAddressInAssertion = null;
         try {
-            emailAddress = getRequiredValueFromAssertion(emailAttribute);
+            emailAddressInAssertion = getRequiredValueFromAssertion(emailAttribute);
         } catch (Exception ex) {
-            String testShibIdpEntityId = "https://idp.testshib.org/idp/shibboleth";
             if (shibIdp.equals(testShibIdpEntityId)) {
                 logger.info("For " + testShibIdpEntityId + " (which as of this writing doesn't provide the " + emailAttribute + " attribute) setting email address to value of eppn: " + shibUserIdentifier);
-                emailAddress = shibUserIdentifier;
+                emailAddressInAssertion = shibUserIdentifier;
             } else {
                 // forcing all other IdPs to send us an an email
                 return;
             }
         }
+
+        if (!EMailValidator.isEmailValid(emailAddressInAssertion, null)) {
+            String msg = "The SAML assertion contained an invalid email address: \"" + emailAddressInAssertion + "\".";
+            logger.info(msg);
+            String singleEmailAddress = ShibUtil.findSingleValue(emailAddressInAssertion);
+            if (EMailValidator.isEmailValid(singleEmailAddress, null)) {
+                msg = "Multiple email addresses were asserted by the Identity Provider (" + emailAddressInAssertion + " ). These were sorted and the first was chosen: " + singleEmailAddress;
+                logger.info(msg);
+                emailAddress = singleEmailAddress;
+            } else {
+                msg += " A single valid address could not be found.";
+                FacesContext.getCurrentInstance().addMessage(null, new FacesMessage(FacesMessage.SEVERITY_ERROR, identityProviderProblem, msg));
+                return;
+            }
+        } else {
+            emailAddress = emailAddressInAssertion;
+        }
+
         String usernameAssertion = getValueFromAssertion(usernameAttribute);
         internalUserIdentifer = ShibUtil.generateFriendlyLookingUserIdentifer(usernameAssertion, emailAddress);
         logger.info("friendly looking identifer (backend will enforce uniqueness):" + internalUserIdentifer);
@@ -312,6 +332,7 @@ public void init() {
          */
 //        String displayName = getDisplayName(displayNameAttribute, firstNameAttribute, lastNameAttribute);
         String affiliation = getAffiliation();
+//        emailAddress = "willFailBeanValidation"; // for testing createAuthenticatedUser exceptions
         displayInfo = new AuthenticatedUserDisplayInfo(firstName, lastName, emailAddress, affiliation, null);
 
         userPersistentId = shibIdp + persistentUserIdSeparator + shibUserIdentifier;
@@ -475,6 +496,8 @@ public enum DevShibAccountType {
         HARVARD1,
         HARVARD2,
         TWO_EMAILS,
+        INVALID_EMAIL,
+        MISSING_REQUIRED_ATTR,
     };
 
     private DevShibAccountType getDevShibAccountType() {
@@ -538,6 +561,14 @@ private void possiblyMutateRequestInDev() {
                 mutateRequestForDevConstantTwoEmails();
                 break;
 
+            case INVALID_EMAIL:
+                mutateRequestForDevConstantInvalidEmail();
+                break;
+
+            case MISSING_REQUIRED_ATTR:
+                mutateRequestForDevConstantMissingRequiredAttributes();
+                break;
+
             default:
                 logger.info("Should never reach here");
                 break;
@@ -555,14 +586,22 @@ private void printHeaders() {
     public String confirmAndCreateAccount() {
         ShibAuthenticationProvider shibAuthProvider = new ShibAuthenticationProvider();
         String lookupStringPerAuthProvider = userPersistentId;
-        AuthenticatedUser au = authSvc.createAuthenticatedUser(
-                new UserRecordIdentifier(shibAuthProvider.getId(), lookupStringPerAuthProvider), internalUserIdentifer, displayInfo, true);
+        AuthenticatedUser au = null;
+        try {
+            au = authSvc.createAuthenticatedUser(
+                    new UserRecordIdentifier(shibAuthProvider.getId(), lookupStringPerAuthProvider), internalUserIdentifer, displayInfo, true);
+        } catch (EJBException ex) {
+            /**
+             * @todo Show the ConstraintViolationException, if any.
+             */
+            logger.info("Couldn't create user " + userPersistentId + " due to exception: " + ex.getCause());
+        }
         if (au != null) {
             logger.info("created user " + au.getIdentifier());
+            logInUserAndSetShibAttributes(au);
         } else {
-            logger.info("couldn't create user " + userPersistentId);
+            JsfHelper.addErrorMessage("Couldn't create user.");
         }
-        logInUserAndSetShibAttributes(au);
         return getPrettyFacesHomePageString(true);
     }
 
@@ -650,7 +689,13 @@ private String getRequiredValueFromAssertion(String key) throws Exception {
         if (attributeOrHeader == null) {
             String msg = "The SAML assertion for \"" + key + "\" was null. Please contact support.";
             logger.info(msg);
-            FacesContext.getCurrentInstance().addMessage(null, new FacesMessage(FacesMessage.SEVERITY_ERROR, identityProviderProblem, msg));
+            boolean showMessage = true;
+            if (shibIdp.equals(testShibIdpEntityId) && key.equals(emailAttribute)) {
+                showMessage = false;
+            }
+            if (showMessage) {
+                FacesContext.getCurrentInstance().addMessage(null, new FacesMessage(FacesMessage.SEVERITY_ERROR, identityProviderProblem, msg));
+            }
             throw new Exception(msg);
         }
         String attributeValue = attributeOrHeader.toString();
@@ -704,16 +749,14 @@ public String getPrettyFacesHomePageString(boolean includeFacetDashRedirect) {
             } else {
                 return plainHomepageString + "?faces-redirect=true";
             }
+        } else if (rootDvAlias != null) {
+            /**
+             * @todo Is there a constant for "/dataverse/" anywhere? I guess
+             * we'll just hard-code it here.
+             */
+            return "/dataverse/" + rootDvAlias;
         } else {
-            if (rootDvAlias != null) {
-                /**
-                 * @todo Is there a constant for "/dataverse/" anywhere? I guess
-                 * we'll just hard-code it here.
-                 */
-                return "/dataverse/" + rootDvAlias;
-            } else {
-                return plainHomepageString;
-            }
+            return plainHomepageString;
         }
     }
 
@@ -876,7 +919,7 @@ private void mutateRequestForDevRandom() throws JsonSyntaxException, JsonIOExcep
     }
 
     private void mutateRequestForDevConstantTestShib1() {
-        request.setAttribute(shibIdpAttribute, "https://idp.testshib.org/idp/shibboleth");
+        request.setAttribute(shibIdpAttribute, testShibIdpEntityId);
         // the TestShib "eppn" looks like an email address
         request.setAttribute(uniquePersistentIdentifier, "saml@testshib.org");
 //        request.setAttribute(displayNameAttribute, "Sam El");
@@ -916,4 +959,29 @@ private void mutateRequestForDevConstantTwoEmails() {
         request.setAttribute(usernameAttribute, "eallman");
     }
 
+    private void mutateRequestForDevConstantInvalidEmail() {
+        request.setAttribute(shibIdpAttribute, "https://fake.example.com/idp/shibboleth");
+        request.setAttribute(uniquePersistentIdentifier, "invalidEmail");
+        request.setAttribute(firstNameAttribute, "Invalid");
+        request.setAttribute(lastNameAttribute, "Email");
+        request.setAttribute(emailAttribute, "invalidEmail");
+        request.setAttribute(usernameAttribute, "invalidEmail");
+
+    }
+
+    private void mutateRequestForDevConstantMissingRequiredAttributes() {
+        request.setAttribute(shibIdpAttribute, "https://fake.example.com/idp/shibboleth");
+        /**
+         * @todo When shibIdpAttribute is set to null why don't we see the error
+         * in the GUI?
+         */
+//        request.setAttribute(shibIdpAttribute, null);
+        request.setAttribute(uniquePersistentIdentifier, "missing");
+        request.setAttribute(uniquePersistentIdentifier, null);
+        request.setAttribute(firstNameAttribute, "Missing");
+        request.setAttribute(lastNameAttribute, "Required");
+        request.setAttribute(emailAttribute, "missing@mailinator.com");
+        request.setAttribute(usernameAttribute, "missing");
+    }
+
 }

src/main/java/edu/harvard/iq/dataverse/authorization/providers/shib/ShibUtil.java

@@ -4,6 +4,7 @@
 import com.google.gson.JsonElement;
 import com.google.gson.JsonObject;
 import com.google.gson.JsonParser;
+import edu.harvard.iq.dataverse.EMailValidator;
 import java.util.Arrays;
 import java.util.UUID;
 import java.util.logging.Logger;
@@ -61,26 +62,30 @@ public static String getDisplayNameFromDiscoFeed(String entityIdToFind, String d
      * "displayName" so we'll hold off on implementing anything for now.
      */
     public static ShibUserNameFields findBestFirstAndLastName(String firstName, String lastName, String displayName) {
-        firstName = getSingleName(firstName);
-        lastName = getSingleName(lastName);
+        firstName = findSingleValue(firstName);
+        lastName = findSingleValue(lastName);
         return new ShibUserNameFields(firstName, lastName);
     }
 
-    private static String getSingleName(String name) {
-        String[] parts = name.split(";");
+    public static String findSingleValue(String mayHaveMultipleValues) {
+        if (mayHaveMultipleValues == null) {
+            return null;
+        }
+        String singleValue = mayHaveMultipleValues;
+        String[] parts = mayHaveMultipleValues.split(";");
         if (parts.length != 1) {
             logger.fine("parts (before sorting): " + Arrays.asList(parts));
             // predictable order (sorted alphabetically)
             Arrays.sort(parts);
             logger.fine("parts (after sorting): " + Arrays.asList(parts));
             try {
                 String first = parts[0];
-                name = first;
+                singleValue = first;
             } catch (ArrayIndexOutOfBoundsException ex) {
-                logger.info("Couldn't find first part of " + name);
+                logger.info("Couldn't find first part of " + singleValue);
             }
         }
-        return name;
+        return singleValue;
     }
 
     public static String generateFriendlyLookingUserIdentifer(String usernameAssertion, String email) {
@@ -97,7 +102,8 @@ public static String generateFriendlyLookingUserIdentifer(String usernameAsserti
                     logger.info(ex + " parsing " + email);
                 }
             } else {
-                logger.info("Odd email address. No @ sign: " + email);
+                boolean passedValidation = EMailValidator.isEmailValid(email, null);
+                logger.info("Odd email address. No @ sign ('" + email + "'). Passed email validation: " + passedValidation);
             }
         } else {
             logger.info("email attribute not sent by IdP");

src/main/java/edu/harvard/iq/dataverse/authorization/users/AuthenticatedUser.java

@@ -48,11 +48,7 @@ public class AuthenticatedUser implements User, Serializable {
     @Column(nullable = false, unique=true)
     private String userIdentifier;
 
-    /**
-     * @todo Uncomment the ValidateEmail annotation below for consistency with
-     * the annotation on BuiltinUser.
-     */
-//    @ValidateEmail(message = "Please enter a valid email address.")
+    @ValidateEmail(message = "Please enter a valid email address.")
     @NotNull
     @Column(nullable = false, unique=true)
     private String email;

src/test/java/edu/harvard/iq/dataverse/EMailValidatorTest.java

@@ -0,0 +1,19 @@
+package edu.harvard.iq.dataverse;
+
+import static org.junit.Assert.assertEquals;
+import org.junit.Test;
+
+public class EMailValidatorTest {
+
+    @Test
+    public void testIsEmailValid() {
+        assertEquals(true, EMailValidator.isEmailValid("pete@mailinator.com", null));
+        assertEquals(false, EMailValidator.isEmailValid("pete1@mailinator.com;pete2@mailinator.com", null));
+        assertEquals(false, EMailValidator.isEmailValid("", null));
+        /**
+         * @todo How can null as an email address be valid?!?
+         */
+        assertEquals(true, EMailValidator.isEmailValid(null, null));
+    }
+
+}

src/test/java/edu/harvard/iq/dataverse/authorization/providers/shib/ShibUtilTest.java

@@ -120,6 +120,13 @@ public void testFindBestFirstAndLastName() {
         assertEquals(expected8.getLastName(), actual8.getLastName());
     }
 
+    @Test
+    public void testFindSingleValue() {
+        assertEquals(null, ShibUtil.findSingleValue(null));
+        assertEquals("foo", ShibUtil.findSingleValue("foo"));
+        assertEquals("bar", ShibUtil.findSingleValue("foo;bar"));
+    }
+
     @Test
     public void testGenerateFriendlyLookingUserIdentifer() {
         int lengthOfUuid = UUID.randomUUID().toString().length();