Skip to content

Commit

Permalink
Support email validation and email case-insensitive matching for user…
Browse files Browse the repository at this point in the history
… admin (#2642)

* Support email validation and email case-insensitive matching

* One more email comparison case
  • Loading branch information
denniscunningham authored Aug 3, 2023
1 parent a81bf1e commit d126c98
Show file tree
Hide file tree
Showing 5 changed files with 98 additions and 41 deletions.
5 changes: 5 additions & 0 deletions pepper-apis/dsm-core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@
<!-- <version>1.4.11</version> -->
</dependency>

<dependency>
<groupId>commons-validator</groupId>
<artifactId>commons-validator</artifactId>
</dependency>

<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-dbcp2</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public class UserDao implements Dao<UserDto> {
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 = ?";
Expand All @@ -38,7 +38,7 @@ public Optional<UserDto> 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),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -265,8 +266,9 @@ protected Map<String, Integer> validateUsers(List<String> users, int groupId) {
throw new DSMBadRequestException("Invalid users: empty");
}
Map<String, Integer> 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;
}
Expand All @@ -288,52 +290,61 @@ 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<String, RoleInfo> studyRoles = getAdminRoles(groupId);

List<String> removeUsers = req.getRemoveUsers();
List<UserRequest.User> addUsers = req.getAddUsers();
boolean hasAddUsers = !CollectionUtils.isEmpty(addUsers);
boolean hasRemoveUsers = !CollectionUtils.isEmpty(removeUsers);

// verify request data
Map<String, UserDto> emailToAddUser = null;
if (hasAddUsers) {
emailToAddUser = verifyAddUser(addUsers, groupId, studyRoles.keySet());
}

List<UserDto> 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) {
throw new DSMBadRequestException("Invalid user request: no users");
}

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<UserRequest.User> users) {
int groupId = verifyStudyGroup(studyGroup);
Map<String, RoleInfo> studyRoles = getAdminRoles(groupId);

// pre-check to lessen likelihood of partial operation
Map<String, UserDto> emailToUser = verifyAddUser(users, groupId, studyRoles.keySet());

protected void addUser(List<UserRequest.User> users, Map<String, UserDto> emailToUser, int groupId,
Map<String, RoleInfo> studyRoles) {
UserDao userDao = new UserDao();
for (var user: users) {
UserDto userDto = emailToUser.get(user.getEmail());
int userId = userDto.getId();
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);
}
Expand All @@ -348,6 +359,9 @@ protected Map<String, UserDto> verifyAddUser(List<UserRequest.User> users, int g
Map<String, UserDto> 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) {
Expand Down Expand Up @@ -415,15 +429,7 @@ public void updateUser(UpdateUserRequest req) {
/**
* Remove one or more users and their associated roles
*/
protected void removeUser(List<String> users) {
int groupId = validateOperatorAdmin();

// pre-check all users to decrease likelihood of incomplete operation
List<UserDto> userDto = new ArrayList<>();
for (String email: users) {
userDto.add(verifyUserByEmail(email, groupId));
}

protected void removeUser(List<UserDto> userDto) {
for (UserDto user: userDto) {
deleteUserRoles(user.getId());
user.setIsActive(0);
Expand Down Expand Up @@ -476,11 +482,11 @@ protected static Map<Integer, UserInfo> getStudyUsers(int groupId, UserRoleReque
return allStudyUsers;
}

Map<String, Integer> emailToId = allStudyUsers.entrySet().stream().collect(Collectors.toMap(e -> e.getValue().getEmail(),
Map.Entry::getKey));
Map<String, Integer> emailToId = allStudyUsers.entrySet().stream().collect(Collectors.toMap(e ->
e.getValue().getEmail().toUpperCase(), Map.Entry::getKey));
Map<Integer, UserInfo> 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 {
Expand Down Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> roles;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -500,10 +500,20 @@ public void testAddAndRemoveUser() {

String user = "[email protected]";
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) {
Expand Down Expand Up @@ -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"));
}
Expand Down Expand Up @@ -598,6 +608,7 @@ public void testAddExistingUser() {
int operatorId = setupAdmin("[email protected]", new ArrayList<>(rolesToId.values()), groupId);

String user = "[email protected]";
String userVariation = "[email protected]";
String userName = "test_user7";
UserRequest addUserRequest = new UserRequest(List.of(new UserRequest.User(user, userName, null,
roles)), null);
Expand All @@ -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"));
}
Expand All @@ -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"));
}
}

Expand Down Expand Up @@ -693,6 +712,27 @@ public void testGetStudyGroup() {
}
}

@Test
public void testValidateEmailFormat() {
try {
UserAdminService.validateEmailFormat("[email protected]");
} 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);
Expand Down

0 comments on commit d126c98

Please sign in to comment.