diff --git a/src/main/java/edu/harvard/iq/dataverse/EMailValidator.java b/src/main/java/edu/harvard/iq/dataverse/EMailValidator.java index a0bf2319dca..4cdbe345ba8 100644 --- a/src/main/java/edu/harvard/iq/dataverse/EMailValidator.java +++ b/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; diff --git a/src/main/java/edu/harvard/iq/dataverse/Shib.java b/src/main/java/edu/harvard/iq/dataverse/Shib.java index 74a539e3357..5de0279f816 100644 --- a/src/main/java/edu/harvard/iq/dataverse/Shib.java +++ b/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"); + } + } diff --git a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/shib/ShibUtil.java b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/shib/ShibUtil.java index 0b73895ee74..97e13b787af 100644 --- a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/shib/ShibUtil.java +++ b/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,13 +62,17 @@ 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) @@ -75,12 +80,12 @@ private static String getSingleName(String name) { 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"); diff --git a/src/main/java/edu/harvard/iq/dataverse/authorization/users/AuthenticatedUser.java b/src/main/java/edu/harvard/iq/dataverse/authorization/users/AuthenticatedUser.java index e789a8dfb70..0185c2978ea 100644 --- a/src/main/java/edu/harvard/iq/dataverse/authorization/users/AuthenticatedUser.java +++ b/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; diff --git a/src/test/java/edu/harvard/iq/dataverse/EMailValidatorTest.java b/src/test/java/edu/harvard/iq/dataverse/EMailValidatorTest.java new file mode 100644 index 00000000000..ab92ffeabfd --- /dev/null +++ b/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)); + } + +} diff --git a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/shib/ShibUtilTest.java b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/shib/ShibUtilTest.java index ed68e195d78..1258f788e62 100644 --- a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/shib/ShibUtilTest.java +++ b/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();