trim whitespace for user email, identifier, etc. #3044 #2945

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

@@ -33,14 +33,7 @@ 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? See also
-         * https://github.com/IQSS/dataverse/issues/2945 and
-         * https://github.com/IQSS/dataverse/issues/3044
-         */
-        boolean isValid = EmailValidator.getInstance().isValid(value.trim());
+        boolean isValid = EmailValidator.getInstance().isValid(value);
         if (!isValid) {
             if (context != null) {
                 context.buildConstraintViolationWithTemplate(value + " is not a valid email address.").addConstraintViolation();

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

@@ -357,21 +357,34 @@ public String cancel() {
     }
 
     /**
-     * @return The value of a Shib attribute (if non-empty) or null.
+     * @return The trimmed value of a Shib attribute (if non-empty) or null.
+     *
+     * @todo Move this to ShibUtil
      */
     private String getValueFromAssertion(String key) {
         Object attribute = request.getAttribute(key);
         if (attribute != null) {
             String attributeValue = attribute.toString();
-            logger.fine("The SAML assertion for \"" + key + "\" (optional) was \"" + attributeValue + "\".");
-            if (!attributeValue.isEmpty()) {
-                return attributeValue;
+            String trimmedValue = attributeValue.trim();
+            if (!trimmedValue.isEmpty()) {
+                logger.fine("The SAML assertion for \"" + key + "\" (optional) was \"" + attributeValue + "\" and was trimmed to \"" + trimmedValue + "\".");
+                return trimmedValue;
+            } else {
+                logger.fine("The SAML assertion for \"" + key + "\" (optional) was \"" + attributeValue + "\" and was trimmed to \"" + trimmedValue + "\" (empty string). Returing null.");
+                return null;
             }
+        } else {
+            logger.fine("The SAML assertion for \"" + key + "\" (optional) was null.");
+            return null;
         }
-        logger.info("The SAML assertion for \"" + key + "\" (optional) was null.");
-        return null;
     }
 
+    /**
+     * @return The trimmed value of a Shib attribute (if non-empty) or null.
+     *
+     * @todo Move this to ShibUtil. More objects might be required since
+     * sometimes we want to show messages, etc.
+     */
     private String getRequiredValueFromAssertion(String key) throws Exception {
         Object attribute = request.getAttribute(key);
         if (attribute == null) {
@@ -390,8 +403,9 @@ private String getRequiredValueFromAssertion(String key) throws Exception {
         if (attributeValue.isEmpty()) {
             throw new Exception(key + " was empty");
         }
-        logger.fine("The SAML assertion for \"" + key + "\" (required) was \"" + attributeValue + "\".");
-        return attributeValue;
+        String trimmedValue = attributeValue.trim();
+        logger.fine("The SAML assertion for \"" + key + "\" (required) was \"" + attributeValue + "\" and was trimmed to \"" + trimmedValue + "\".");
+        return trimmedValue;
     }
 
     public String getRootDataverseAlias() {

src/main/java/edu/harvard/iq/dataverse/authorization/AuthenticationServiceBean.java

@@ -268,6 +268,11 @@ public AuthenticatedUser authenticate( String authenticationProviderId, Authenti
             // yay! see if we already have this user.
             AuthenticatedUser user = lookupUser(authenticationProviderId, resp.getUserId());
 
+            /**
+             * @todo Why does a method called "authenticate" have the potential
+             * to call "createAuthenticatedUser"? Isn't the creation of a user a
+             * different action than authenticating?
+             */
             return ( user == null ) ?
                 AuthenticationServiceBean.this.createAuthenticatedUser(
                         new UserRecordIdentifier(authenticationProviderId, resp.getUserId()), resp.getUserId(), resp.getUserDisplayInfo(), true )
@@ -390,6 +395,7 @@ public ApiToken save( ApiToken aToken ) {
      * @param userDisplayInfo
      * @param generateUniqueIdentifier if {@code true}, create a new, unique user identifier for the created user, if the suggested one exists.
      * @return the newly created user, or {@code null} if the proposed identifier exists and {@code generateUniqueIdentifier} was {@code false}.
+     * @throws EJBException which may wrap an ConstraintViolationException if the proposed user does not pass bean validation.
      */
     public AuthenticatedUser createAuthenticatedUser(UserRecordIdentifier userRecordId,
                                                      String proposedAuthenticatedUserIdentifier,
@@ -398,6 +404,10 @@ public AuthenticatedUser createAuthenticatedUser(UserRecordIdentifier userRecord
         AuthenticatedUser authenticatedUser = new AuthenticatedUser();
         authenticatedUser.applyDisplayInfo(userDisplayInfo);
 
+        // we have no desire for leading or trailing whitespace in identifiers
+        if (proposedAuthenticatedUserIdentifier != null) {
+            proposedAuthenticatedUserIdentifier = proposedAuthenticatedUserIdentifier.trim();
+        }
         // we now select a username for the generated AuthenticatedUser, or give up
         String internalUserIdentifer = proposedAuthenticatedUserIdentifier;
         // TODO should lock table authenticated users for write here

src/main/java/edu/harvard/iq/dataverse/authorization/providers/builtin/BuiltinUserServiceBean.java

@@ -7,6 +7,7 @@
 import edu.harvard.iq.dataverse.passwordreset.PasswordResetInitResponse;
 import edu.harvard.iq.dataverse.passwordreset.PasswordResetServiceBean;
 import java.util.List;
+import java.util.Set;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import javax.ejb.EJB;
@@ -16,6 +17,10 @@
 import javax.persistence.NoResultException;
 import javax.persistence.NonUniqueResultException;
 import javax.persistence.PersistenceContext;
+import javax.validation.ConstraintViolation;
+import javax.validation.Validation;
+import javax.validation.Validator;
+import javax.validation.ValidatorFactory;
 
 /**
  *
@@ -41,6 +46,28 @@ public String encryptPassword(String plainText) {
     }
 
     public BuiltinUser save(BuiltinUser dataverseUser) {
+        /**
+         * Trim the email address no matter what the user entered or is entered
+         * on their behalf in the case of Shibboleth assertions.
+         *
+         * @todo Why doesn't Bean Validation report that leading and trailing
+         * whitespace in an email address is a problem?
+         */
+        dataverseUser.setEmail(dataverseUser.getEmail().trim());
+        /**
+         * We throw a proper IllegalArgumentException here because otherwise
+         * from the API you get a 500 response and "Can't save user: null".
+         */
+        ValidatorFactory factory = Validation.buildDefaultValidatorFactory();
+        Validator validator = factory.getValidator();
+        Set<ConstraintViolation<BuiltinUser>> violations = validator.validate(dataverseUser);
+        if (violations.size() > 0) {
+            StringBuilder sb = new StringBuilder();
+            violations.stream().forEach((violation) -> {
+                sb.append(" Invalid value: <<<").append(violation.getInvalidValue()).append(">>> for ").append(violation.getPropertyPath()).append(" at ").append(violation.getLeafBean()).append(" - ").append(violation.getMessage());
+            });
+            throw new IllegalArgumentException("BuiltinUser could not be saved to due constraint violations: " + sb);
+        }
         if ( dataverseUser.getId() == null ) {
             // see that the username is unique
             if ( em.createNamedQuery("BuiltinUser.findByUserName")

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

@@ -62,6 +62,9 @@ public enum DevShibAccountType {
         HARVARD2,
         TWO_EMAILS,
         INVALID_EMAIL,
+        EMAIL_WITH_LEADING_SPACE,
+        UID_WITH_LEADING_SPACE,
+        IDENTIFIER_WITH_LEADING_SPACE,
         MISSING_REQUIRED_ATTR,
     };
 
@@ -129,6 +132,18 @@ public void possiblyMutateRequestInDev(HttpServletRequest request) {
                 ShibUtil.mutateRequestForDevConstantInvalidEmail(request);
                 break;
 
+            case EMAIL_WITH_LEADING_SPACE:
+                ShibUtil.mutateRequestForDevConstantEmailWithLeadingSpace(request);
+                break;
+
+            case UID_WITH_LEADING_SPACE:
+                ShibUtil.mutateRequestForDevConstantUidWithLeadingSpace(request);
+                break;
+
+            case IDENTIFIER_WITH_LEADING_SPACE:
+                ShibUtil.mutateRequestForDevConstantIdentifierWithLeadingSpace(request);
+                break;
+
             case MISSING_REQUIRED_ATTR:
                 ShibUtil.mutateRequestForDevConstantMissingRequiredAttributes(request);
                 break;

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

@@ -199,10 +199,38 @@ static void mutateRequestForDevConstantInvalidEmail(HttpServletRequest request)
         request.setAttribute(ShibUtil.uniquePersistentIdentifier, "invalidEmail");
         request.setAttribute(ShibUtil.firstNameAttribute, "Invalid");
         request.setAttribute(ShibUtil.lastNameAttribute, "Email");
-        request.setAttribute(ShibUtil.emailAttribute, "invalidEmail");
+        request.setAttribute(ShibUtil.emailAttribute, "elisah.da mota@example.com");
         request.setAttribute(ShibUtil.usernameAttribute, "invalidEmail");
     }
 
+    static void mutateRequestForDevConstantEmailWithLeadingSpace(HttpServletRequest request) {
+        request.setAttribute(ShibUtil.shibIdpAttribute, "https://fake.example.com/idp/shibboleth");
+        request.setAttribute(ShibUtil.uniquePersistentIdentifier, "leadingWhitespace");
+        request.setAttribute(ShibUtil.firstNameAttribute, "leadingWhitespace");
+        request.setAttribute(ShibUtil.lastNameAttribute, "leadingWhitespace");
+        request.setAttribute(ShibUtil.emailAttribute, " leadingWhitespace@mailinator.com");
+        request.setAttribute(ShibUtil.usernameAttribute, "leadingWhitespace");
+    }
+
+    static void mutateRequestForDevConstantUidWithLeadingSpace(HttpServletRequest request) {
+        request.setAttribute(ShibUtil.shibIdpAttribute, "https://fake.example.com/idp/shibboleth");
+        request.setAttribute(ShibUtil.uniquePersistentIdentifier, "leadingWhitespace");
+        request.setAttribute(ShibUtil.firstNameAttribute, "leadingWhitespace");
+        request.setAttribute(ShibUtil.lastNameAttribute, "leadingWhitespace");
+        request.setAttribute(ShibUtil.emailAttribute, "leadingWhitespace@mailinator.com");
+        request.setAttribute(ShibUtil.usernameAttribute, " leadingWhitespace");
+    }
+
+    // the identifier is the IdP plus the eppn separated by a |
+    static void mutateRequestForDevConstantIdentifierWithLeadingSpace(HttpServletRequest request) {
+        request.setAttribute(ShibUtil.shibIdpAttribute, " https://fake.example.com/idp/shibboleth");
+        request.setAttribute(ShibUtil.uniquePersistentIdentifier, "leadingWhitespace");
+        request.setAttribute(ShibUtil.firstNameAttribute, "leadingWhitespace");
+        request.setAttribute(ShibUtil.lastNameAttribute, "leadingWhitespace");
+        request.setAttribute(ShibUtil.emailAttribute, "leadingWhitespace@mailinator.com");
+        request.setAttribute(ShibUtil.usernameAttribute, "leadingWhitespace");
+    }
+
     static void mutateRequestForDevConstantMissingRequiredAttributes(HttpServletRequest request) {
         request.setAttribute(ShibUtil.shibIdpAttribute, "https://fake.example.com/idp/shibboleth");
         /**

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

@@ -49,6 +49,15 @@ public class AuthenticatedUser implements User, Serializable {
     @GeneratedValue(strategy = GenerationType.IDENTITY)
     Long id;
 
+    /**
+     * @todo Shouldn't there be some constraints on what the userIdentifier is
+     * allowed to be? It can't be as restrictive as the "userName" field on
+     * BuiltinUser because we can't predict what Shibboleth Identity Providers
+     * (IdPs) will send (typically in the "eppn" SAML assertion) but perhaps
+     * spaces, for example, should be disallowed. Right now "elisah.da mota" can
+     * be persisted as a userIdentifier per
+     * https://github.com/IQSS/dataverse/issues/2945
+     */
     @NotNull
     @Column(nullable = false, unique=true)
     private String userIdentifier;

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

@@ -8,6 +8,17 @@ public class EMailValidatorTest {
     @Test
     public void testIsEmailValid() {
         assertEquals(true, EMailValidator.isEmailValid("pete@mailinator.com", null));
+        /**
+         * @todo How can " leadingWhitespace@mailinator.com" be a valid email
+         * address?
+         */
+        assertEquals(true, EMailValidator.isEmailValid(" leadingWhitespace@mailinator.com", null));
+        /**
+         * @todo How can "trailingWhitespace@mailinator.com " be a valid email
+         * address?
+         */
+        assertEquals(true, EMailValidator.isEmailValid("trailingWhitespace@mailinator.com ", null));
+        assertEquals(false, EMailValidator.isEmailValid("elisah.da mota@example.com", null));
         assertEquals(false, EMailValidator.isEmailValid("pete1@mailinator.com;pete2@mailinator.com", null));
         boolean issue2998resolved = false;
         /**

src/test/java/edu/harvard/iq/dataverse/api/BuiltinUsersIT.java

@@ -23,7 +23,8 @@ public class BuiltinUsersIT {
     @Test
     public void testUserId() {
 
-        Response createUserResponse = createUser(getRandomUsername(), "firstName", "lastName");
+        String email = null;
+        Response createUserResponse = createUser(getRandomUsername(), "firstName", "lastName", email);
         createUserResponse.prettyPrint();
         assertEquals(200, createUserResponse.getStatusCode());
 
@@ -53,11 +54,34 @@ public void testUserId() {
         assertEquals(userIdFromDatabase, userIdFromJsonCreateResponse);
     }
 
+    @Test
+    public void testLeadingWhitespaceInEmailAddress() {
+        String randomUsername = getRandomUsername();
+        String email = " " + randomUsername + "@mailinator.com";
+        Response createUserResponse = createUser(randomUsername, "firstName", "lastName", email);
+        createUserResponse.prettyPrint();
+        assertEquals(200, createUserResponse.statusCode());
+        String emailActual = JsonPath.from(createUserResponse.body().asString()).getString("data.user." + emailKey);
+        // the backend will trim the email address
+        String emailExpected = email.trim();
+        assertEquals(emailExpected, emailActual);
+    }
+
+    @Test
+    public void testLeadingWhitespaceInUsername() {
+        String randomUsername = " " + getRandomUsername();
+        String email = randomUsername + "@mailinator.com";
+        Response createUserResponse = createUser(randomUsername, "firstName", "lastName", email);
+        createUserResponse.prettyPrint();
+        assertEquals(400, createUserResponse.statusCode());
+    }
+
     @Test
     public void testLogin() {
 
         String usernameToCreate = getRandomUsername();
-        Response createUserResponse = createUser(usernameToCreate, "firstName", "lastName");
+        String email = null;
+        Response createUserResponse = createUser(usernameToCreate, "firstName", "lastName", email);
         createUserResponse.prettyPrint();
         assertEquals(200, createUserResponse.getStatusCode());
 
@@ -87,8 +111,8 @@ public void testLogin() {
 
     }
 
-    private Response createUser(String username, String firstName, String lastName) {
-        String userAsJson = getUserAsJsonString(username, firstName, lastName);
+    private Response createUser(String username, String firstName, String lastName, String email) {
+        String userAsJson = getUserAsJsonString(username, firstName, lastName, email);
         String password = getPassword(userAsJson);
         Response response = given()
                 .body(userAsJson)
@@ -127,12 +151,16 @@ private static String getRandomUsername() {
         return UUID.randomUUID().toString().substring(0, 8);
     }
 
-    private static String getUserAsJsonString(String username, String firstName, String lastName) {
+    private static String getUserAsJsonString(String username, String firstName, String lastName, String email) {
         JsonObjectBuilder builder = Json.createObjectBuilder();
         builder.add(usernameKey, username);
         builder.add("firstName", firstName);
         builder.add("lastName", lastName);
-        builder.add(emailKey, getEmailFromUserName(username));
+        if (email == null) {
+            builder.add(emailKey, getEmailFromUserName(username));
+        } else {
+            builder.add(emailKey, email);
+        }
         String userAsJson = builder.build().toString();
         logger.fine("User to create: " + userAsJson);
         return userAsJson;

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

@@ -136,6 +136,7 @@ public void testFindSingleValue() {
     public void testGenerateFriendlyLookingUserIdentifer() {
         int lengthOfUuid = UUID.randomUUID().toString().length();
         assertEquals("uid1", ShibUtil.generateFriendlyLookingUserIdentifer("uid1", null));
+        assertEquals(" leadingWhiteSpace", ShibUtil.generateFriendlyLookingUserIdentifer(" leadingWhiteSpace", null));
         assertEquals("uid1", ShibUtil.generateFriendlyLookingUserIdentifer("uid1", "email1@example.com"));
         assertEquals("email1", ShibUtil.generateFriendlyLookingUserIdentifer(null, "email1@example.com"));
         assertEquals(lengthOfUuid, ShibUtil.generateFriendlyLookingUserIdentifer(null, null).length());