From d126c98fa035713fe3a439fcbc22e946099f6975 Mon Sep 17 00:00:00 2001 From: denniscunningham <129115982+denniscunningham@users.noreply.github.com> Date: Wed, 2 Aug 2023 22:22:00 -0400 Subject: [PATCH] Support email validation and email case-insensitive matching for user admin (#2642) * Support email validation and email case-insensitive matching * One more email comparison case --- pepper-apis/dsm-core/pom.xml | 5 ++ .../dsm/db/dao/user/UserDao.java | 4 +- .../dsm/service/admin/UserAdminService.java | 66 +++++++++++-------- .../dsm/service/admin/UserRequest.java | 2 +- .../service/admin/UserAdminServiceTest.java | 62 +++++++++++++---- 5 files changed, 98 insertions(+), 41 deletions(-) diff --git a/pepper-apis/dsm-core/pom.xml b/pepper-apis/dsm-core/pom.xml index 6dddcf81c2..3341425567 100644 --- a/pepper-apis/dsm-core/pom.xml +++ b/pepper-apis/dsm-core/pom.xml @@ -57,6 +57,11 @@ + + commons-validator + commons-validator + + org.apache.commons commons-dbcp2 diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dao/user/UserDao.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dao/user/UserDao.java index 2007f0974d..4f40736e79 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dao/user/UserDao.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dao/user/UserDao.java @@ -29,7 +29,7 @@ public class UserDao implements Dao { private static final String SQL_DELETE_USER_BY_ID = "DELETE FROM access_user WHERE user_id = ?"; private static final String SQL_SELECT_USER_BY_EMAIL = "SELECT user.user_id, user.name, user.email, user.phone_number, user.is_active FROM access_user user " - + "WHERE user.email = ?"; + + "WHERE UPPER(user.email) = ?"; private static final String SQL_SELECT_USER_BY_ID = "SELECT user.user_id, user.name, user.email, user.phone_number, user.is_active FROM access_user user " + "WHERE user.user_id = ?"; @@ -38,7 +38,7 @@ public Optional getUserByEmail(@NonNull String email) { SimpleResult results = inTransaction((conn) -> { SimpleResult dbVals = new SimpleResult(); try (PreparedStatement stmt = conn.prepareStatement(SQL_SELECT_USER_BY_EMAIL)) { - stmt.setString(1, email); + stmt.setString(1, email.toUpperCase()); try (ResultSet rs = stmt.executeQuery()) { if (rs.next()) { dbVals.resultValue = new UserDto(rs.getInt(USER_ID), diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserAdminService.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserAdminService.java index ff7f07deef..c66ce34040 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserAdminService.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserAdminService.java @@ -20,6 +20,7 @@ import lombok.Data; import lombok.extern.slf4j.Slf4j; import org.apache.commons.collections4.CollectionUtils; +import org.apache.commons.validator.routines.EmailValidator; import org.broadinstitute.dsm.db.UserSettings; import org.broadinstitute.dsm.db.dao.user.UserDao; import org.broadinstitute.dsm.db.dto.user.UserDto; @@ -265,8 +266,9 @@ protected Map validateUsers(List users, int groupId) { throw new DSMBadRequestException("Invalid users: empty"); } Map userIds = new HashMap<>(); - for (String email: users) { - userIds.put(email, verifyUserByEmail(email, groupId).getId()); + for (String user: users) { + UserDto userDto = verifyUserByEmail(user, groupId); + userIds.put(userDto.getEmailOrThrow(), userDto.getId()); } return userIds; } @@ -288,19 +290,33 @@ protected static String validateEmailRequest(String email) { if (StringUtils.isBlank(email)) { throw new DSMBadRequestException("Invalid user email: blank"); } - return email; + return email.trim(); } public void addAndRemoveUsers(UserRequest req) { + int groupId = verifyStudyGroup(studyGroup); + Map studyRoles = getAdminRoles(groupId); + List removeUsers = req.getRemoveUsers(); List addUsers = req.getAddUsers(); boolean hasAddUsers = !CollectionUtils.isEmpty(addUsers); boolean hasRemoveUsers = !CollectionUtils.isEmpty(removeUsers); + // verify request data + Map emailToAddUser = null; + if (hasAddUsers) { + emailToAddUser = verifyAddUser(addUsers, groupId, studyRoles.keySet()); + } + + List removeUserDto = new ArrayList<>(); if (hasRemoveUsers) { + for (String user: removeUsers) { + removeUserDto.add(verifyUserByEmail(user, groupId)); + } + // ensure no union of add and remove users - if (hasAddUsers && CollectionUtils.containsAny(removeUsers, - addUsers.stream().map(UserRequest.User::getEmail).collect(Collectors.toList()))) { + if (hasAddUsers && CollectionUtils.containsAny(emailToAddUser.keySet(), + removeUserDto.stream().map(UserDto::getEmailOrThrow).collect(Collectors.toList()))) { throw new DSMBadRequestException("Invalid user request: Cannot add and remove the same user"); } } else if (!hasAddUsers) { @@ -308,24 +324,19 @@ public void addAndRemoveUsers(UserRequest req) { } if (hasRemoveUsers) { - removeUser(removeUsers); + removeUser(removeUserDto); } if (hasAddUsers) { - addUser(addUsers); + addUser(addUsers, emailToAddUser, groupId, studyRoles); } } /** * Add user to study group. User request must include at least one role */ - protected void addUser(List users) { - int groupId = verifyStudyGroup(studyGroup); - Map studyRoles = getAdminRoles(groupId); - - // pre-check to lessen likelihood of partial operation - Map emailToUser = verifyAddUser(users, groupId, studyRoles.keySet()); - + protected void addUser(List users, Map emailToUser, int groupId, + Map studyRoles) { UserDao userDao = new UserDao(); for (var user: users) { UserDto userDto = emailToUser.get(user.getEmail()); @@ -333,7 +344,7 @@ protected void addUser(List users) { boolean hasUserSettings = false; if (userId != 0) { UserDao.update(userId, userDto); - hasUserSettings = UserSettings.getUserSettings(user.getEmail()) != null; + hasUserSettings = UserSettings.getUserSettings(userDto.getEmailOrThrow()) != null; } else { userId = userDao.create(userDto); } @@ -348,6 +359,9 @@ protected Map verifyAddUser(List users, int g Map emailToUser = new HashMap<>(); for (var user: users) { String email = validateEmailRequest(user.getEmail()); + validateEmailFormat(email); + // update email in request since we use it as a key + user.setEmail(email); UserDto userDto = getUserByEmail(email, groupId); if (userDto != null) { @@ -415,15 +429,7 @@ public void updateUser(UpdateUserRequest req) { /** * Remove one or more users and their associated roles */ - protected void removeUser(List users) { - int groupId = validateOperatorAdmin(); - - // pre-check all users to decrease likelihood of incomplete operation - List userDto = new ArrayList<>(); - for (String email: users) { - userDto.add(verifyUserByEmail(email, groupId)); - } - + protected void removeUser(List userDto) { for (UserDto user: userDto) { deleteUserRoles(user.getId()); user.setIsActive(0); @@ -476,11 +482,11 @@ protected static Map getStudyUsers(int groupId, UserRoleReque return allStudyUsers; } - Map emailToId = allStudyUsers.entrySet().stream().collect(Collectors.toMap(e -> e.getValue().getEmail(), - Map.Entry::getKey)); + Map emailToId = allStudyUsers.entrySet().stream().collect(Collectors.toMap(e -> + e.getValue().getEmail().toUpperCase(), Map.Entry::getKey)); Map users = new HashMap<>(); for (String email: req.getUsers()) { - Integer id = emailToId.get(email); + Integer id = emailToId.get(email.trim().toUpperCase()); if (id != null) { users.put(id, allStudyUsers.get(id)); } else { @@ -728,6 +734,12 @@ protected static UserDto verifyUserByEmail(String email, int groupId) { return user; } + protected static void validateEmailFormat(String email) { + if (!EmailValidator.getInstance().isValid(email)) { + throw new DSMBadRequestException("Invalid email address format: " + email); + } + } + protected static UserDto getUserByEmail(String email, int groupId) { // TODO: Currently we do not track users for a group, but get by groupId once we do -DC UserDao userDao = new UserDao(); diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserRequest.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserRequest.java index 7bd6fde7f2..6389dad548 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserRequest.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserRequest.java @@ -17,7 +17,7 @@ public class UserRequest { @Data public static class User { - private final String email; + private String email; private final String name; private final String phone; private final List roles; diff --git a/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/service/admin/UserAdminServiceTest.java b/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/service/admin/UserAdminServiceTest.java index ccc690ce00..eae9f44db3 100644 --- a/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/service/admin/UserAdminServiceTest.java +++ b/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/service/admin/UserAdminServiceTest.java @@ -500,10 +500,20 @@ public void testAddAndRemoveUser() { String user = "test_user6@study.org"; String userName = "test_user6"; + + UserAdminService service = new UserAdminService(Integer.toString(operatorId), TEST_GROUP); + UserRequest badUserRequest = new UserRequest(List.of(new UserRequest.User(user, userName, null, + List.of("bad_role"))), null); + try { + service.addAndRemoveUsers(badUserRequest); + Assert.fail("UserAdminService.addUser should fail with bad role names"); + } catch (Exception e) { + Assert.assertTrue(e.getMessage().contains("Invalid roles for study group")); + } + UserRequest userRequest = new UserRequest(List.of(new UserRequest.User(user, userName, null, List.of(role1))), null); - UserAdminService service = new UserAdminService(Integer.toString(operatorId), TEST_GROUP); try { service.addAndRemoveUsers(userRequest); } catch (Exception e) { @@ -564,12 +574,12 @@ public void testAddAndRemoveUser() { try { service.addAndRemoveUsers(removeReq); } catch (Exception e) { - Assert.fail("Exception from UserAdminService.removeUser: " + getStackTrace(e)); + Assert.fail("Exception from UserAdminService.addAndRemoveUsers: " + getStackTrace(e)); } try { UserAdminService.verifyUserByEmail(user, groupId); - Assert.fail("UserAdminService.removeUser failed to remove user"); + Assert.fail("UserAdminService.addAndRemoveUsers failed to remove user"); } catch (Exception e) { Assert.assertTrue(e.getMessage().contains("Invalid user")); } @@ -598,6 +608,7 @@ public void testAddExistingUser() { int operatorId = setupAdmin("test_admin5@study.org", new ArrayList<>(rolesToId.values()), groupId); String user = "test_user7@study.org"; + String userVariation = "Test_User7@study.org"; String userName = "test_user7"; UserRequest addUserRequest = new UserRequest(List.of(new UserRequest.User(user, userName, null, roles)), null); @@ -606,27 +617,27 @@ public void testAddExistingUser() { try { service.addAndRemoveUsers(addUserRequest); } catch (Exception e) { - Assert.fail("Exception from UserAdminService.addUser: " + getStackTrace(e)); + Assert.fail("Exception from UserAdminService.addAndRemoveUsers: " + getStackTrace(e)); } UserRequest removeReq = new UserRequest(null, List.of(user)); try { service.addAndRemoveUsers(removeReq); } catch (Exception e) { - Assert.fail("Exception from UserAdminService.removeUser: " + getStackTrace(e)); + Assert.fail("Exception from UserAdminService.addAndRemoveUsers: " + getStackTrace(e)); } // add user back to test the inactive to active transition try { service.addAndRemoveUsers(addUserRequest); } catch (Exception e) { - Assert.fail("Exception from UserAdminService.addUser: " + getStackTrace(e)); + Assert.fail("Exception from UserAdminService.addAndRemoveUsers: " + getStackTrace(e)); } // try to add again try { service.addAndRemoveUsers(addUserRequest); - Assert.fail("UserAdminService.addUser should fail to add an existing study user"); + Assert.fail("UserAdminService.addAndRemoveUsers should fail to add an existing study user"); } catch (Exception e) { Assert.assertTrue(e.getMessage().contains("Already has roles in study")); } @@ -640,14 +651,22 @@ public void testAddExistingUser() { try { service.addAndRemoveUsers(addUserRequest); } catch (Exception e) { - Assert.fail("Exception from UserAdminService.addUser: " + getStackTrace(e)); + Assert.fail("Exception from UserAdminService.addAndRemoveUsers: " + getStackTrace(e)); } - // cleanup + // cleanup (with case variation on email address) + UserRequest removeReq2 = new UserRequest(null, List.of(userVariation)); try { - service.addAndRemoveUsers(removeReq); + service.addAndRemoveUsers(removeReq2); } catch (Exception e) { - Assert.fail("Exception from UserAdminService.removeUser: " + getStackTrace(e)); + Assert.fail("Exception from UserAdminService.addAndRemoveUsers: " + getStackTrace(e)); + } + + try { + service.addAndRemoveUsers(removeReq2); + Assert.fail("UserAdminService.addAndRemoveUsers should fail to a remove study user that does not exist"); + } catch (Exception e) { + Assert.assertTrue(e.getMessage().contains("Invalid user for study group")); } } @@ -693,6 +712,27 @@ public void testGetStudyGroup() { } } + @Test + public void testValidateEmailFormat() { + try { + UserAdminService.validateEmailFormat("joe@gmail.com"); + } catch (Exception e) { + Assert.fail("Exception from UserAdminService.validateEmailFormat: " + e.toString()); + } + try { + UserAdminService.validateEmailFormat("justjoe"); + Assert.fail("UserAdminService.validateEmailFormat should fail with bad email address: justjoe"); + } catch (Exception e) { + Assert.assertTrue(e.getMessage().contains("Invalid email address format")); + } + try { + UserAdminService.validateEmailFormat("joe@gmail"); + Assert.fail("UserAdminService.validateEmailFormat should fail with bad email address: joe@gmail"); + } catch (Exception e) { + Assert.assertTrue(e.getMessage().contains("Invalid email address format")); + } + } + private int createAdminUser(String email, int adminRoleId) { int userId = createTestUser(email, adminRoleId); int groupId = UserAdminService.verifyStudyGroup(TEST_GROUP);