diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/UserSettings.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/UserSettings.java index 828543183a..498c1647e4 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/UserSettings.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/UserSettings.java @@ -6,9 +6,11 @@ import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; +import java.sql.Statement; import lombok.Data; import lombok.NonNull; +import org.broadinstitute.dsm.exception.DsmInternalError; import org.broadinstitute.dsm.statics.DBConstants; import org.broadinstitute.dsm.statics.QueryExtension; import org.broadinstitute.lddp.db.SimpleResult; @@ -27,6 +29,8 @@ public class UserSettings { + "WHERE user.user_id = settings.user_id AND user.is_active = 1"; private static final String SQL_INSERT_USER_SETTINGS = "INSERT INTO user_settings SET user_id = ?"; + private static final String SQL_DELETE_USER_SETTINGS = "DELETE FROM user_settings WHERE user_id = ?"; + private static final String USER_ID = "userId"; private int rowsOnPage; @@ -104,12 +108,38 @@ public static UserSettings getUserSettings(@NonNull String email) { return us; } - public static void insertUserSetting(Connection conn, int userId) { - try (PreparedStatement insertKit = conn.prepareStatement(SQL_INSERT_USER_SETTINGS)) { - insertKit.setInt(1, userId); - insertKit.executeUpdate(); - } catch (SQLException e) { - throw new RuntimeException("Error inserting new user_settings ", e); + public static int createUserSettings(int userId) { + return inTransaction(conn -> insertUserSetting(conn, userId)); + } + + public static int insertUserSetting(Connection conn, int userId) { + int id = -1; + try (PreparedStatement stmt = conn.prepareStatement(SQL_INSERT_USER_SETTINGS, Statement.RETURN_GENERATED_KEYS)) { + stmt.setInt(1, userId); + int result = stmt.executeUpdate(); + if (result != 1) { + throw new DsmInternalError("Error inserting user setting. Result count was " + result); + } + try (ResultSet rs = stmt.getGeneratedKeys()) { + if (rs.next()) { + id = rs.getInt(1); + } + } + } catch (SQLException ex) { + throw new DsmInternalError("Error inserting user setting", ex); } + return id; + } + + public static int deleteUserSettings(int userId) { + return inTransaction(conn -> { + try (PreparedStatement stmt = conn.prepareStatement(SQL_DELETE_USER_SETTINGS)) { + stmt.setInt(1, userId); + return stmt.executeUpdate(); + } catch (SQLException ex) { + String msg = String.format("Error deleting user settings: userId=%d", userId); + throw new DsmInternalError(msg, ex); + } + }); } } 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 33f3734775..2007f0974d 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 @@ -24,6 +24,8 @@ public class UserDao implements Dao { public static final String PHONE_NUMBER = "phone_number"; public static final String IS_ACTIVE = "is_active"; private static final String SQL_INSERT_USER = "INSERT INTO access_user (name, email, phone_number, is_active) VALUES (?,?,?,?)"; + private static final String SQL_UPDATE_USER = + "UPDATE access_user SET name = ?, phone_number = ?, is_active = ? WHERE user_id = ?"; 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 " @@ -87,7 +89,7 @@ public Optional get(long userId) { @Override public int create(UserDto userDto) { - String email = userDto.getEmail().orElse(null); + String email = userDto.getEmailOrThrow(); if (StringUtils.isBlank(email)) { throw new DsmInternalError("Error inserting user: email is blank"); } @@ -110,6 +112,26 @@ public int create(UserDto userDto) { }); } + public static void update(int userId, UserDto userDto) { + String email = userDto.getEmailOrThrow(); + String errorMsg = "Error updating user " + email; + int res = inTransaction(conn -> { + try (PreparedStatement stmt = conn.prepareStatement(SQL_UPDATE_USER)) { + stmt.setString(1, userDto.getName().orElse(null)); + stmt.setString(2, userDto.getPhoneNumber().orElse(null)); + stmt.setInt(3, userDto.getIsActive().orElse(1)); + stmt.setInt(4, userId); + int result = stmt.executeUpdate(); + if (result != 1) { + throw new DsmInternalError(errorMsg + " Result count was " + result); + } + return result; + } catch (SQLException ex) { + throw new DsmInternalError(errorMsg, ex); + } + }); + } + @Override public int delete(int id) { SimpleResult simpleResult = DaoUtil.deleteById(id, SQL_DELETE_USER_BY_ID); diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dto/user/UserDto.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dto/user/UserDto.java index 30d8a67340..73e2c55380 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dto/user/UserDto.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dto/user/UserDto.java @@ -4,6 +4,7 @@ import lombok.AllArgsConstructor; import lombok.Setter; +import org.broadinstitute.dsm.exception.DsmInternalError; @Setter @AllArgsConstructor @@ -36,10 +37,20 @@ public Optional getEmail() { return Optional.ofNullable(email); } + // TODO: getEmail should always throw on a missing email since it is not nullable + // but there are a lot of callers. Feel free to chase them all down and rename this method -DC + public String getEmailOrThrow() { + return getEmail().orElseThrow(() -> new DsmInternalError("User email cannot be null")); + } + public Optional getPhoneNumber() { return Optional.ofNullable(phoneNumber); } public Optional getIsActive() { return Optional.ofNullable(isActive); } + + public boolean isActive() { + return getIsActive().orElse(0) == 1; + } } diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/OncHistoryUploadRoute.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/OncHistoryUploadRoute.java index d05b2f2e56..a1a1239b71 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/OncHistoryUploadRoute.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/OncHistoryUploadRoute.java @@ -40,7 +40,7 @@ protected Object processRequest(Request request, Response response, String userI String oncHistoryUserId; try { UserDto user = new UserDao().get(Integer.parseInt(userId)).orElseThrow(); - oncHistoryUserId = user.getEmail().orElseThrow(); + oncHistoryUserId = user.getEmailOrThrow(); if (oncHistoryUserId.isEmpty()) { throw new DsmInternalError("Empty email address"); } diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/admin/UserRoute.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/admin/UserRoute.java index fcb49e3906..0c3bbe2a4b 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/admin/UserRoute.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/admin/UserRoute.java @@ -5,6 +5,7 @@ import org.apache.commons.lang3.StringUtils; import org.broadinstitute.dsm.security.RequestHandler; import org.broadinstitute.dsm.service.admin.AddUserRequest; +import org.broadinstitute.dsm.service.admin.UpdateUserRequest; import org.broadinstitute.dsm.service.admin.UserAdminService; import org.broadinstitute.dsm.service.admin.UserRequest; import org.broadinstitute.dsm.statics.RoutePath; @@ -47,6 +48,20 @@ public Object processRequest(Request request, Response response, String userId) } catch (Exception e) { return UserRoleRoute.handleError(e, "adding user", response); } + } else if (requestMethod.equals(RoutePath.RequestMethod.PUT.toString())) { + UpdateUserRequest req; + try { + req = new Gson().fromJson(body, UpdateUserRequest.class); + } catch (Exception e) { + log.info("Invalid request format for {}", body); + response.status(400); + return "Invalid request format"; + } + try { + adminService.updateUser(req); + } catch (Exception e) { + return UserRoleRoute.handleError(e, "updating user", response); + } } else if (requestMethod.equals(RoutePath.RequestMethod.DELETE.toString())) { UserRequest req; try { diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/AddUserRequest.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/AddUserRequest.java index 1290eedaef..335210160d 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/AddUserRequest.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/AddUserRequest.java @@ -3,7 +3,9 @@ import java.util.List; import lombok.Data; +import org.apache.commons.lang3.StringUtils; import org.broadinstitute.dsm.db.dto.user.UserDto; +import org.broadinstitute.dsm.exception.DsmInternalError; @Data public class AddUserRequest { @@ -31,5 +33,18 @@ public User(String email, String name, String phone, List roles) { public UserDto asUserDto() { return new UserDto(name, email, phone); } + + public UserDto asUpdatedUserDto(UserDto userDto) { + if (!email.equalsIgnoreCase(userDto.getEmailOrThrow())) { + throw new DsmInternalError("Assert: email addresses do not match"); + } + if (!StringUtils.isBlank(name)) { + userDto.setName(name); + } + if (!StringUtils.isBlank(phone)) { + userDto.setPhoneNumber(phone); + } + return userDto; + } } } diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UpdateUserRequest.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UpdateUserRequest.java new file mode 100644 index 0000000000..aad675e7eb --- /dev/null +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UpdateUserRequest.java @@ -0,0 +1,33 @@ +package org.broadinstitute.dsm.service.admin; + +import java.util.List; + +import lombok.Data; +import org.broadinstitute.dsm.db.dto.user.UserDto; + +@Data +public class UpdateUserRequest { + + private final List users; + + public UpdateUserRequest(List users) { + this.users = users; + } + + @Data + public static class User { + private final String email; + private final String name; + private final String phone; + + public User(String email, String name, String phone) { + this.email = email; + this.name = name; + this.phone = phone; + } + + public UserDto asUserDto() { + return new UserDto(name, email, phone); + } + } +} 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 b70e873563..e43d2f55af 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 @@ -13,13 +13,16 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; import lombok.Data; import lombok.extern.slf4j.Slf4j; import org.apache.commons.collections4.CollectionUtils; +import org.broadinstitute.dsm.db.UserSettings; import org.broadinstitute.dsm.db.dao.user.UserDao; +import org.broadinstitute.dsm.db.dto.user.UserDto; import org.broadinstitute.dsm.exception.DSMBadRequestException; import org.broadinstitute.dsm.exception.DsmInternalError; import org.broadinstitute.dsm.statics.RoutePath; @@ -71,9 +74,6 @@ public class UserAdminService { private static final String SQL_SELECT_GROUP_ROLE = "SELECT dgr.group_role_id FROM ddp_group_role dgr WHERE dgr.group_id = ? AND dgr.role_id = ?"; - private static final String SQL_SELECT_USER_BY_EMAIL = - "SELECT au.user_id, au.name, au.phone_number, au.is_active FROM access_user au WHERE au.email = ?"; - private static final String SQL_SELECT_USERS_FOR_GROUP = "SELECT au.user_id, au.email, au.name, au.phone_number FROM access_user au " + "JOIN access_user_role_group aurg on aurg.user_id = au.user_id " @@ -138,17 +138,25 @@ public void addUserRoles(UserRoleRequest req) { List roleNames = req.getRoles(); validateRoles(roleNames, studyRoles.keySet()); - for (String userEmail: req.getUsers()) { - if (StringUtils.isBlank(userEmail)) { - throw new DSMBadRequestException("Invalid user email: blank"); - } - addRoles(userEmail, roleNames, groupId, studyRoles); + // pre-check to lessen likelihood of partial operation + Map userIds = new HashMap<>(); + for (String email: req.getUsers()) { + userIds.put(email, verifyUserByEmail(email, groupId).getId()); } - } - protected void addRoles(String email, List roles, int groupId, Map studyRoles) { - int userId = verifyUserByEmail(email, groupId); + for (var entry: userIds.entrySet()) { + addRoles(entry.getValue(), roleNames, groupId, studyRoles); + log.info("Added roles for user {} in study group {}: {}", entry.getKey(), studyGroup, + String.join(", ", roleNames)); + } + } + /** + * Add roles for user + * + * @param roles list of proposed roles, already validated + */ + protected void addRoles(int userId, List roles, int groupId, Map studyRoles) { for (String role : roles) { if (StringUtils.isBlank(role)) { throw new DsmInternalError("Invalid role: blank"); @@ -157,10 +165,8 @@ protected void addRoles(String email, List roles, int groupId, Map roleNames = req.getRoles(); validateRoles(roleNames, studyRoles.keySet()); - for (String userEmail: req.getUsers()) { - if (StringUtils.isBlank(userEmail)) { - throw new DSMBadRequestException("Invalid user email: blank"); + // pre-check to lessen likelihood of partial operation + Map userIds = new HashMap<>(); + for (String email: req.getUsers()) { + int userId = verifyUserByEmail(email, groupId).getId(); + List existingRoles = getRolesForUser(userId, groupId); + if (CollectionUtils.subtract(existingRoles, roleNames).isEmpty()) { + throw new DSMBadRequestException("Cannot remove all roles for user " + email); } - removeUserRoles(userEmail, roleNames, groupId, studyRoles); + userIds.put(email, userId); } - } - protected void removeUserRoles(String email, List roles, int groupId, Map studyRoles) { - // TODO loop is similar to addUserRoles -DC - int userId = verifyUserByEmail(email, groupId); + for (var entry: userIds.entrySet()) { + removeUserRoles(entry.getValue(), roleNames, groupId, studyRoles); + log.info("Removed roles for user {} in study group {}: {}", entry.getKey(), studyGroup, + String.join(", ", roleNames)); + } + } - for (String role : roles) { + /** + * Remove roles for user + * + * @param roles list of proposed roles, already validated + */ + protected void removeUserRoles(int userId, List roles, int groupId, Map studyRoles) { +; for (String role : roles) { if (StringUtils.isBlank(role)) { throw new DsmInternalError("Invalid role: blank"); } @@ -198,10 +216,8 @@ protected void removeUserRoles(String email, List roles, int groupId, Ma try { // ignore failure to remove deleteUserRole(userId, roleId, groupId); - String msg = String.format("Removed role %s for user %s in study group %s", role, email, studyGroup); - log.info(msg); } catch (Exception e) { - String msg = String.format("Error removing user %s from role %s for study group %s", email, role, studyGroup); + String msg = String.format("Error removing user %d from role %s for study group %s", userId, role, studyGroup); throw new DsmInternalError(msg, e); } } @@ -217,12 +233,16 @@ protected void validateRoleRequest(UserRoleRequest req) { } protected void validateRoles(List roleNames, Set validRoleNames) { + String msg = String.format("Invalid roles for study group %s: ", studyGroup); + if (CollectionUtils.isEmpty(roleNames)) { + throw new DSMBadRequestException(msg + "None provided"); + } if (validRoleNames.containsAll(roleNames)) { return; } - Collection badRoles = CollectionUtils.subtract(roleNames, validRoleNames); - String msg = String.format("Invalid roles for study group %s: %s", studyGroup, String.join(",", badRoles)); - throw new DSMBadRequestException(msg); + Collection badRoles = CollectionUtils.subtract(roleNames, validRoleNames).stream() + .map(r -> r.isEmpty() ? "" : r).collect(Collectors.toSet()); + throw new DSMBadRequestException(msg + String.join(", ", badRoles)); } protected String validateEmailRequest(String email) { @@ -236,27 +256,95 @@ protected String validateEmailRequest(String email) { * Add user to study group. User request must include at least one role */ public void addUser(AddUserRequest req) { + int groupId = verifyStudyGroup(studyGroup); + Map studyRoles = getAdminRoles(groupId); + List users = req.getUsers(); if (CollectionUtils.isEmpty(users)) { throw new DSMBadRequestException("Invalid user list: blank"); } // pre-check to lessen likelihood of partial operation + Map emailToUser = new HashMap<>(); for (var user: users) { String email = validateEmailRequest(user.getEmail()); + UserDto userDto = getUserByEmail(email, groupId); + if (userDto != null) { + // update any new info provided + emailToUser.put(email, user.asUpdatedUserDto(verifyExistingUser(userDto, groupId))); + } else { + userDto = user.asUserDto(); + emailToUser.put(email, userDto); + } + // not a strict requirement in DB, but now enforcing - if (StringUtils.isBlank(user.getName())) { + if (StringUtils.isBlank(userDto.getName().orElse(null))) { throw new DSMBadRequestException("Invalid user name: blank"); } - if (getUserByEmail(email, -1) != null) { - throw new DsmInternalError("User already exists: " + email); - } + + validateRoles(user.getRoles(), studyRoles.keySet()); } UserDao userDao = new UserDao(); for (var user: users) { - userDao.create(user.asUserDto()); + 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; + } else { + userId = userDao.create(userDto); + } + if (!hasUserSettings) { + UserSettings.createUserSettings(userId); + } + addRoles(userId, user.getRoles(), groupId, studyRoles); + } + } + + protected UserDto verifyExistingUser(UserDto userDto, int groupId) { + String email = userDto.getEmailOrThrow(); + + // if user has no roles in this study then it is okay to add them + List existingRoles = getRolesForUser(userDto.getId(), groupId); + if (!existingRoles.isEmpty()) { + throw new DsmInternalError(String.format("Cannot add user %s: Already has roles in study %s", + email, studyGroup)); + } + + if (userDto.isActive()) { + log.info("addUser: user {} already exists, is active, but has no roles in study {}. " + + "Updating with new user information", + email, studyGroup); + } else { + log.info("addUser: user {} already exists but is inactive. Activating and updating with new user information", + email); + userDto.setIsActive(1); + } + return userDto; + } + + public void updateUser(UpdateUserRequest req) { + int groupId = validateOperatorAdmin(); + List users = req.getUsers(); + if (CollectionUtils.isEmpty(users)) { + throw new DSMBadRequestException("Invalid user list: blank"); + } + + Map usersById = new HashMap<>(); + // pre-check to lessen likelihood of partial operation + for (var user: users) { + // not a strict requirement in DB, but now enforcing + if (StringUtils.isBlank(user.getName())) { + throw new DSMBadRequestException("Invalid user name: blank"); + } + usersById.put(verifyUserByEmail(user.getEmail(), groupId).getId(), user); + } + + for (var entry: usersById.entrySet()) { + UserDao.update(entry.getKey(), entry.getValue().asUserDto()); } } @@ -264,24 +352,32 @@ public void addUser(AddUserRequest req) { * Remove one or more users and their associated roles */ public void removeUser(UserRequest req) { + int groupId = validateOperatorAdmin(); if (CollectionUtils.isEmpty(req.getUsers())) { throw new DSMBadRequestException("Invalid user list: blank"); } // pre-check all users to decrease likelihood of incomplete operation - List userIds = new ArrayList<>(); + List users = new ArrayList<>(); for (String email: req.getUsers()) { - validateEmailRequest(email); - userIds.add(verifyUserByEmail(email, -1)); + users.add(verifyUserByEmail(email, groupId)); } - UserDao userDao = new UserDao(); - for (int userId: userIds) { - deleteUserRoles(userId); - userDao.delete(userId); + for (UserDto user: users) { + deleteUserRoles(user.getId()); + user.setIsActive(0); + UserDao.update(user.getId(), user); + UserSettings.deleteUserSettings(user.getId()); } } + protected int validateOperatorAdmin() { + int groupId = verifyStudyGroup(studyGroup); + // will throw if operator is not user admin + getAdminRoles(groupId); + return groupId; + } + /** * Return user information and user roles, both assigned and unassigned roles for study * @@ -330,11 +426,8 @@ protected static Map getStudyUsers(int groupId, UserRequest r // theoretically there should be no users without roles for a given study // but there are ways that might occur. log.warn("Found user with no study roles: {}", email); - StudyUser user = getUserByEmail(email, groupId); - if (user == null) { - throw new DSMBadRequestException("Invalid user email " + email); - } - users.put(user.getId(), user.toUserInfo()); + UserDto user = verifyUserByEmail(email, groupId); + users.put(user.getId(), new UserInfo(user)); } } return users; @@ -560,32 +653,25 @@ protected static int getRoleId(String role) { }); } - protected static int verifyUserByEmail(String email, int groupId) { - StudyUser user = getUserByEmail(email, groupId); - // TODO: see if access_user.is_active is used -DC + protected static UserDto verifyUserByEmail(String email, int groupId) { + if (StringUtils.isBlank(email)) { + throw new DSMBadRequestException("Invalid user email: blank"); + } + UserDto user = getUserByEmail(email, groupId); if (user == null) { throw new DSMBadRequestException("Invalid user for study group: " + email); } - return user.getId(); + if (!user.isActive()) { + throw new DSMBadRequestException("Invalid user for study group (inactive): " + email); + } + return user; } - protected static UserAdminService.StudyUser getUserByEmail(String email, int groupId) { + 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 - return inTransaction(conn -> { - UserAdminService.StudyUser user = null; - try (PreparedStatement stmt = conn.prepareStatement(SQL_SELECT_USER_BY_EMAIL)) { - stmt.setString(1, email); - try (ResultSet rs = stmt.executeQuery()) { - if (rs.next()) { - user = new StudyUser(rs.getInt(1), email, rs.getString(2), rs.getString(3)); - } - } - } catch (SQLException e) { - String msg = String.format("Error getting user %s for study group, ID: %d", email, groupId); - throw new DsmInternalError(msg, e); - } - return user; - }); + UserDao userDao = new UserDao(); + Optional userDto = userDao.getUserByEmail(email); + return userDto.orElse(null); } protected static int getUserRole(int userId, int roleId, int groupId) { @@ -813,25 +899,6 @@ public RoleInfo(int roleId, String name, String displayText) { } } - @Data - protected static class StudyUser { - private final int id; - private final String email; - private final String name; - private final String phone; - - public StudyUser(int id, String email, String name, String phone) { - this.id = id; - this.email = email; - this.name = name; - this.phone = phone; - } - - public UserInfo toUserInfo() { - return new UserInfo(email, name, phone); - } - } - protected static class NameAndId { public final String name; public final int id; diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserInfo.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserInfo.java index b74e7032a0..bb9bd99be9 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserInfo.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserInfo.java @@ -3,8 +3,11 @@ import java.util.ArrayList; import java.util.List; +import lombok.AllArgsConstructor; import lombok.Data; +import org.broadinstitute.dsm.db.dto.user.UserDto; +@AllArgsConstructor @Data public class UserInfo { private final String email; @@ -12,17 +15,17 @@ public class UserInfo { private final String phone; private List roles; - public UserInfo(String email, String name, String phone, List roles) { + public UserInfo(String email, String name, String phone) { this.email = email; this.name = name; this.phone = phone; - this.roles = roles; + this.roles = new ArrayList<>(); } - public UserInfo(String email, String name, String phone) { - this.email = email; - this.name = name; - this.phone = phone; + public UserInfo(UserDto userDto) { + this.email = userDto.getEmailOrThrow(); + this.name = userDto.getName().orElse(null); + this.phone = userDto.getPhoneNumber().orElse(null); this.roles = new ArrayList<>(); } } 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 6bb6a7c004..ac1bef9267 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 @@ -10,6 +10,7 @@ import java.sql.SQLException; import java.sql.Statement; import java.util.ArrayList; +import java.util.Collection; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -19,11 +20,13 @@ import lombok.extern.slf4j.Slf4j; import org.broadinstitute.dsm.DbTxnBaseTest; +import org.broadinstitute.dsm.db.UserSettings; import org.broadinstitute.dsm.db.dao.user.UserDao; import org.broadinstitute.dsm.db.dto.user.UserDto; import org.broadinstitute.dsm.exception.DsmInternalError; import org.broadinstitute.dsm.statics.RoutePath; import org.broadinstitute.lddp.db.SimpleResult; +import org.junit.After; import org.junit.AfterClass; import org.junit.Assert; import org.junit.BeforeClass; @@ -65,16 +68,12 @@ public static void setup() { @AfterClass public static void tearDown() { - for (int groupRoleId: createdGroupRoles) { - UserAdminService.deleteGroupRole(groupRoleId); - } - UserDao userDao = new UserDao(); for (var entry: createdUserRoles.entrySet()) { int userId = entry.getKey(); List userRoles = entry.getValue(); - for (int userRole: userRoles) { - UserAdminService.deleteUserRole(userId, userRole, studyGroupId); + for (int roleId: userRoles) { + UserAdminService.deleteUserRole(userId, roleId, studyGroupId); } userDao.delete(userId); } @@ -82,6 +81,13 @@ public static void tearDown() { UserAdminService.deleteStudyGroup(studyGroupId); } + @After + public void cleanup() { + for (int groupRoleId: createdGroupRoles) { + UserAdminService.deleteGroupRole(groupRoleId); + } + } + private int createTestUser(String email, int roleId) { int userId = createUser(email); List roleIds = new ArrayList<>(); @@ -195,12 +201,11 @@ public void testVerifyOperatorGroup() { public void testGetUserByEmailAndGroup() { int roleId = UserAdminService.getRoleId("upload_onc_history"); Assert.assertTrue(roleId > 0); - String email = "testUser@study.org"; + String email = "testUser1@study.org"; int userId = createTestUser(email, -1); int groupId = UserAdminService.verifyStudyGroup(TEST_GROUP); try { - int id = UserAdminService.verifyUserByEmail(email, groupId); - Assert.assertEquals(userId, id); + Assert.assertEquals(userId, UserAdminService.verifyUserByEmail(email, groupId).getId()); } catch (Exception e) { Assert.fail("Exception from UserAdminService.getUserByEmail: " + getStackTrace(e)); } @@ -224,37 +229,59 @@ public void testGetUserByEmailAndGroup() { } @Test - public void testUserRoles() { - int operatorId = createAdminUser("test_admin2@study.org", userAdminRoleId); - int groupId = UserAdminService.verifyStudyGroup(TEST_GROUP); + public void testValidateRoles() { + UserAdminService service = new UserAdminService("not needed for test", TEST_GROUP); + + Set studyRoles = Set.of("A", "B", "C"); try { - Map adminRoles = UserAdminService.getOperatorAdminRoles(operatorId, groupId); - Assert.assertTrue(adminRoles == null || adminRoles.isEmpty()); + service.validateRoles(new ArrayList<>(), studyRoles); + Assert.fail("UserAdminService.validateRoles should fail with no roles"); } catch (Exception e) { - Assert.fail("Exception from UserAdminService.verifyOperatorForGroup: " + getStackTrace(e)); + Assert.assertTrue(e.getMessage().contains("Invalid roles for study group")); + Assert.assertTrue(e.getMessage().contains("None provided")); + } + + try { + service.validateRoles(List.of("A", "", "B"), studyRoles); + Assert.fail("UserAdminService.validateRoles should fail with blank role"); + } catch (Exception e) { + Assert.assertTrue(e.getMessage().contains("Invalid roles for study group")); + Assert.assertTrue(e.getMessage().contains("")); + } + + try { + service.validateRoles(List.of("A", "XXX", "B"), studyRoles); + Assert.fail("UserAdminService.validateRoles should fail with invalid role for study"); + } catch (Exception e) { + Assert.assertTrue(e.getMessage().contains("Invalid roles for study group")); + Assert.assertTrue(e.getMessage().contains("XXX")); } + try { + service.validateRoles(List.of("A", "B"), studyRoles); + } catch (Exception e) { + Assert.fail("UserAdminService.validateRoles should not fail with valid roles for study"); + } + } + + @Test + public void testUserRoles() { String role1 = "upload_onc_history"; - int roleId1 = UserAdminService.getRoleId(role1); - Assert.assertTrue(roleId1 > 0); String role2 = "upload_ror_file"; - int roleId2 = UserAdminService.getRoleId(role2); - Assert.assertTrue(roleId2 > 0); List roles = List.of(role1, role2); - + Map rolesToId = getRoleIds(roles); String user1 = "testUser2@study.org"; - int userId1 = createTestUser(user1, roleId1); - addRoleForUser(roleId2, userId1); String user2 = "testUser3@study.org"; - int userId2 = createTestUser(user2, roleId1); - addRoleForUser(roleId2, userId2); + List users = List.of(user1, user2); + Map usersToId = setupUsers(users, rolesToId.values()); + + int groupId = UserAdminService.verifyStudyGroup(TEST_GROUP); // let operator manage one of the roles - addGroupRole(roleId1, userAdminRoleId); + int operatorId = setupAdmin("test_admin2@study.org", List.of(rolesToId.get(role1)), groupId); UserAdminService service = new UserAdminService(Integer.toString(operatorId), TEST_GROUP); - List users = List.of(user1, user2); UserRoleRequest req = new UserRoleRequest(users, roles); try { service.addUserRoles(req); @@ -264,7 +291,7 @@ public void testUserRoles() { } // both roles are now in the study - addGroupRole(roleId2, userAdminRoleId); + addGroupRole(rolesToId.get(role2), userAdminRoleId); // verify by getting all study roles Set allRoles = Set.of(role1, role2); @@ -278,15 +305,11 @@ public void testUserRoles() { } // verify internally - int userRoleId = UserAdminService.getUserRole(userId1, roleId1, groupId); - Assert.assertNotEquals(-1, userRoleId); - int userRoleId2 = UserAdminService.getUserRole(userId1, roleId2, groupId); - Assert.assertNotEquals(-1, userRoleId2); - - int user2RoleId = UserAdminService.getUserRole(userId2, roleId1, groupId); - Assert.assertNotEquals(-1, user2RoleId); - int user2RoleId2 = UserAdminService.getUserRole(userId2, roleId2, groupId); - Assert.assertNotEquals(-1, user2RoleId2); + for (var userId: usersToId.values()) { + for (var roleId: rolesToId.values()) { + verifyUserHasRole(userId, roleId, groupId); + } + } // get roles and verify UserRequest getReq = new UserRequest(users); @@ -298,11 +321,9 @@ public void testUserRoles() { } List userInfoList = res.getUsers(); - Map> userRoles = getUserRoles(userInfoList); - for (var resRoles: userRoles.entrySet()) { - Assert.assertTrue("GetUserRoles did not include role", resRoles.getValue().contains(role1)); - Assert.assertTrue("GetUserRoles did not include role", resRoles.getValue().contains(role2)); - } + Map> userRoles = getUserRoles(userInfoList); + verifyResponseRoles(userRoles.get(user1), List.of(role1, role2), List.of(role1, role2)); + verifyResponseRoles(userRoles.get(user2), List.of(role1, role2), List.of(role1, role2)); // remove one role for both users UserRoleRequest req2 = new UserRoleRequest(users, List.of(role1)); @@ -321,10 +342,8 @@ public void testUserRoles() { userInfoList = res.getUsers(); userRoles = getUserRoles(userInfoList); - for (var resRoles: userRoles.entrySet()) { - Assert.assertTrue("RemoveUserFromRole removed wrong role", resRoles.getValue().contains(role2)); - Assert.assertFalse("RemoveUserFromRole did not remove role", resRoles.getValue().contains(role1)); - } + verifyResponseRoles(userRoles.get(user1), List.of(role1, role2), List.of(role2)); + verifyResponseRoles(userRoles.get(user2), List.of(role1, role2), List.of(role2)); // check that result also has unassigned roles for (var userInfo: userInfoList) { @@ -332,100 +351,211 @@ public void testUserRoles() { } // adjust cleanup - removeRoleForUser(roleId1, userId1); - removeRoleForUser(roleId1, userId2); + removeRoleForUser(rolesToId.get(role1), usersToId.get(user1)); + removeRoleForUser(rolesToId.get(role1), usersToId.get(user2)); - // remove one role for one user (left with one user that has one role) + // remove last role for one user, which should not be allowed UserRoleRequest req3 = new UserRoleRequest(List.of(user2), List.of(role2)); try { service.removeUserRoles(req3); + Assert.fail("UserAdminService.removeUserRoles should fail to remove all user roles"); } catch (Exception e) { - Assert.fail("Exception from UserAdminService.removeUserFromRoles: " + getStackTrace(e)); + Assert.assertTrue(e.getMessage().contains("Cannot remove all roles for user")); } + } - // get roles and verify - UserRequest getReq2 = new UserRequest(users); + private static void verifyUserHasRole(int userId, int roleId, int groupId) { + int userRoleId = UserAdminService.getUserRole(userId, roleId, groupId); + Assert.assertNotEquals(-1, userRoleId); + } + + private static void verifyResponseRoles(Map userRoles, List allRoles, List assignedRoles) { + Assert.assertTrue(allRoles.containsAll(assignedRoles)); + + for (String role: assignedRoles) { + Boolean isAssigned = userRoles.get(role); + Assert.assertNotNull("GetUserRoles did not include role", isAssigned); + Assert.assertEquals(assignedRoles.contains(role), isAssigned); + } + } + + @Test + public void testAddAndRemoveUser() { + int groupId = UserAdminService.verifyStudyGroup(TEST_GROUP); + + String role1 = "upload_onc_history"; + String role2 = "upload_ror_file"; + List roles = List.of(role1, role2); + Map rolesToId = getRoleIds(roles); + + int operatorId = setupAdmin("test_admin3@study.org", new ArrayList<>(rolesToId.values()), groupId); + + String user = "testUser4@study.org"; + String userName = "testUser4"; + AddUserRequest addUserRequest = new AddUserRequest(List.of(new AddUserRequest.User(user, userName, null, + List.of(role1)))); + + UserAdminService service = new UserAdminService(Integer.toString(operatorId), TEST_GROUP); + try { + service.addUser(addUserRequest); + } catch (Exception e) { + Assert.fail("Exception from UserAdminService.addUser: " + getStackTrace(e)); + } + + // add a role + UserRoleRequest roleReq = new UserRoleRequest(List.of(user), List.of(role2)); + try { + service.addUserRoles(roleReq); + } catch (Exception e) { + Assert.fail("Exception from UserAdminService.addUserRoles: " + getStackTrace(e)); + } + + // verify user info + UserRoleResponse res = null; + try { + res = service.getUserRoles(null); + } catch (Exception e) { + Assert.fail("Exception from UserAdminService.getUserRoles: " + getStackTrace(e)); + } + + List userInfoList = res.getUsers(); + verifyUserInfo(userInfoList, user, userName, null); + + Map> userRoles = getUserRoles(userInfoList); + verifyResponseRoles(userRoles.get(user), List.of(role1), List.of(role1)); + + // update user + String newUserName = "newName"; + String phone = "555-1212"; + UpdateUserRequest.User updateUser = new UpdateUserRequest.User(user, newUserName, phone); + UpdateUserRequest updateReq = new UpdateUserRequest(List.of(updateUser)); + try { + service.updateUser(updateReq); + } catch (Exception e) { + Assert.fail("Exception from UserAdminService.updateUser: " + getStackTrace(e)); + } + + // verify user info again try { - res = service.getUserRoles(getReq2); + res = service.getUserRoles(null); } catch (Exception e) { Assert.fail("Exception from UserAdminService.getUserRoles: " + getStackTrace(e)); } + // user should have user settings + UserSettings settings = UserSettings.getUserSettings(user); + Assert.assertNotNull(settings); + userInfoList = res.getUsers(); - for (var userInfo: userInfoList) { - List resRoles = userInfo.getRoles(); - if (userInfo.getEmail().equals(user1)) { - Assert.assertEquals(2, resRoles.size()); + verifyUserInfo(userInfoList, user, newUserName, phone); - List hasRole = resRoles.stream().filter(UserRole::isHasRole).collect(Collectors.toList()); - Assert.assertEquals(1, hasRole.size()); + userRoles = getUserRoles(userInfoList); + verifyResponseRoles(userRoles.get(user), List.of(role1, role2), List.of(role1, role2)); - List filteredRoles = resRoles.stream().filter(r -> r.getName().equals(role2)).collect(Collectors.toList()); - Assert.assertEquals(1, filteredRoles.size()); - Assert.assertTrue(filteredRoles.get(0).isHasRole()); - } else { - Assert.assertEquals(2, resRoles.size()); + UserRequest removeReq = new UserRequest(List.of(user)); + try { + service.removeUser(removeReq); + } catch (Exception e) { + Assert.fail("Exception from UserAdminService.removeUser: " + getStackTrace(e)); + } - List hasRole = resRoles.stream().filter(UserRole::isHasRole).collect(Collectors.toList()); - Assert.assertTrue("RemoveUserFromRole did not remove role", hasRole.isEmpty()); - } + try { + UserAdminService.verifyUserByEmail(user, groupId); + Assert.fail("UserAdminService.removeUser failed to remove user"); + } catch (Exception e) { + Assert.assertTrue(e.getMessage().contains("Invalid user")); } - // adjust cleanup - removeRoleForUser(roleId2, userId2); + try { + service.updateUser(updateReq); + Assert.fail("UserAdminService.updateUser should fail to update a removed user"); + } catch (Exception e) { + Assert.assertTrue(e.getMessage().contains("Invalid user for study group (inactive)")); + } + + settings = UserSettings.getUserSettings(user); + Assert.assertNull(settings); } + @Test - public void testAddAndRemoveUser() { - int operatorId = createAdminUser("test_admin3@study.org", userAdminRoleId); + public void testAddExistingUser() { int groupId = UserAdminService.verifyStudyGroup(TEST_GROUP); + + String role1 = "upload_onc_history"; + String role2 = "upload_ror_file"; + List roles = List.of(role1, role2); + Map rolesToId = getRoleIds(roles); + + int operatorId = setupAdmin("test_admin4@study.org", new ArrayList<>(rolesToId.values()), groupId); + + String user = "testUser5@study.org"; + String userName = "testUser5"; + AddUserRequest addUserRequest = new AddUserRequest(List.of(new AddUserRequest.User(user, userName, null, + roles))); + + UserAdminService service = new UserAdminService(Integer.toString(operatorId), TEST_GROUP); try { - UserAdminService.getOperatorAdminRoles(operatorId, groupId); + service.addUser(addUserRequest); } catch (Exception e) { - Assert.fail("Exception from UserAdminService.verifyOperatorForGroup: " + getStackTrace(e)); + Assert.fail("Exception from UserAdminService.addUser: " + getStackTrace(e)); } - String email = "testUser4@study.org"; - AddUserRequest req = new AddUserRequest(List.of(new AddUserRequest.User(email, "testUser4", null, null))); + UserRequest removeReq = new UserRequest(List.of(user)); + try { + service.removeUser(removeReq); + } catch (Exception e) { + Assert.fail("Exception from UserAdminService.removeUser: " + getStackTrace(e)); + } - UserAdminService service = new UserAdminService(Integer.toString(operatorId), TEST_GROUP); + // add user back to test the inactive to active transition try { - service.addUser(req); + service.addUser(addUserRequest); } catch (Exception e) { - Assert.fail("Exception from UserAdminService.createUser: " + getStackTrace(e)); + Assert.fail("Exception from UserAdminService.addUser: " + getStackTrace(e)); } - int userId = -1; + // try to add again try { - userId = UserAdminService.verifyUserByEmail(email, -1); + service.addUser(addUserRequest); + Assert.fail("UserAdminService.addUser should fail to add an existing study user"); } catch (Exception e) { - Assert.fail("Exception verifying UserAdminService.createUser: " + getStackTrace(e)); + Assert.assertTrue(e.getMessage().contains("Already has roles in study")); } - // at this point we have an admin and a user, and neither has roles - // add a role for the user, but let the admin service manage it via removeUser - int roleId = UserAdminService.getRoleId("upload_onc_history"); - Assert.assertTrue(roleId > 0); + // user with no roles in this study can be added + int userId = UserAdminService.verifyUserByEmail(user, groupId).getId(); + for (int roleId: rolesToId.values()) { + UserAdminService.deleteUserRole(userId, roleId, studyGroupId); + } try { - UserAdminService.addUserRole(userId, roleId, groupId); + service.addUser(addUserRequest); } catch (Exception e) { - Assert.fail("Exception from UserAdminService.addUserRole: " + getStackTrace(e)); + Assert.fail("Exception from UserAdminService.addUser: " + getStackTrace(e)); } - UserRequest removeReq = new UserRequest(List.of(email)); + // cleanup try { service.removeUser(removeReq); } catch (Exception e) { Assert.fail("Exception from UserAdminService.removeUser: " + getStackTrace(e)); } + } - try { - UserAdminService.verifyUserByEmail(email, -1); - Assert.fail("UserAdminService.removeUser failed to remove user"); - } catch (Exception e) { - Assert.assertTrue(e.getMessage().contains("Invalid user")); + private static void verifyUserInfo(List userInfoList, String email, String name, String phone) { + UserInfo userInfo = null; + for (UserInfo u: userInfoList) { + if (u.getEmail().equals(email)) { + userInfo = u; + break; + } } + Assert.assertNotNull(userInfo); + + Assert.assertEquals(email, userInfo.getEmail()); + Assert.assertEquals(name, userInfo.getName()); + Assert.assertEquals(phone, userInfo.getPhone()); } @Test @@ -467,6 +597,48 @@ private int createAdminUser(String email, int adminRoleId) { return userId; } + private int setupAdmin(String adminEmail, List rolesToManage, int groupId) { + int operatorId = createAdminUser(adminEmail, userAdminRoleId); + try { + Map adminRoles = UserAdminService.getOperatorAdminRoles(operatorId, groupId); + Assert.assertTrue("adminRoles = " + adminRoles.keySet(), adminRoles.isEmpty()); + } catch (Exception e) { + Assert.fail("Exception from UserAdminService.verifyOperatorForGroup: " + getStackTrace(e)); + } + + if (rolesToManage != null) { + for (int roleId: rolesToManage) { + addGroupRole(roleId, userAdminRoleId); + } + } + return operatorId; + } + + private Map setupUsers(List users, Collection roleIds) { + + Map userToId = new HashMap<>(); + for (String user: users) { + int userId = createTestUser(user, -1); + for (int roleId : roleIds) { + addRoleForUser(roleId, userId); + } + userToId.put(user, userId); + } + return userToId; + } + + + private Map getRoleIds(List roles) { + + Map roleToId = new HashMap<>(); + for (String role: roles) { + int roleId = UserAdminService.getRoleId(role); + Assert.assertTrue(roleId > 0); + roleToId.put(role, roleId); + } + return roleToId; + } + private void addAdminRoles(int adminUserId, int groupId, List roles) { try { for (String role : roles) { @@ -480,8 +652,8 @@ private void addAdminRoles(int adminUserId, int groupId, List roles) { private void setUserRoles(String email, List roles, String studyGroup) { try { - int userId = UserAdminService.verifyUserByEmail(email, -1); int groupId = UserAdminService.verifyStudyGroup(studyGroup); + int userId = UserAdminService.verifyUserByEmail(email, groupId).getId(); for (String role : roles) { int roleId = UserAdminService.getRoleId(role); @@ -494,11 +666,11 @@ private void setUserRoles(String email, List roles, String studyGroup) { } } - private Map> getUserRoles(List userInfoList) { - Map> userRoles = new HashMap<>(); + private static Map> getUserRoles(List userInfoList) { + Map> userRoles = new HashMap<>(); for (UserInfo userInfo: userInfoList) { - List roles = userInfo.getRoles().stream().filter(UserRole::isHasRole) - .map(UserRole::getName).collect(Collectors.toList()); + Map roles = userInfo.getRoles().stream() + .collect(Collectors.toMap(UserRole::getName, UserRole::isHasRole)); userRoles.put(userInfo.getEmail(), roles); } return userRoles;