diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/admin/UserRoleRoute.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/admin/UserRoleRoute.java index 5eb171490a..8787560618 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/admin/UserRoleRoute.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/admin/UserRoleRoute.java @@ -101,17 +101,17 @@ protected static Object getUserRoles(String body, Response response, UserAdminSe protected static String handleError(Throwable e, String operation, Response response) { if (e instanceof DSMBadRequestException) { response.status(400); - log.info("DSMBadRequestException {}: {}", operation, e.getMessage()); - return e.getMessage(); + log.info("DSMBadRequestException {}: {}", operation, e.toString()); + return e.toString(); } else if (e instanceof DsmInternalError) { - log.error("Error {}: {}", operation, e.getMessage()); + log.error("Error {}: {}", operation, e.toString()); response.status(500); return "Internal error. Contact development team"; } // any other exception - log.error("Error {}: {}", operation, e.getMessage()); + log.error("Error {}: {}", operation, e.toString()); response.status(500); - return e.getMessage(); + return e.toString(); } } 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 46e78b8328..08dcc6a028 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 @@ -169,7 +169,8 @@ public void setUserRoles(SetUserRoleRequest req) { } for (var entry: userIds.entrySet()) { - List existingRoles = userRoles.get(entry.getKey()); + // filter existing user roles that are not actually study roles + Collection existingRoles = cleanRoles(userRoles.get(entry.getKey()), studyRoles.keySet()); Collection rolesToAdd = CollectionUtils.subtract(roleNames, existingRoles); Collection rolesToRemove = CollectionUtils.subtract(existingRoles, roleNames); int userId = entry.getValue(); @@ -223,7 +224,8 @@ public void updateUserRoles(UpdateUserRoleRequest req) { if (hasRemoveRoles) { validateRoles(removeRoles, studyRoles.keySet()); for (var entry: userIds.entrySet()) { - List existingRoles = getRolesForUser(entry.getValue(), groupId); + // filter existing user roles that are not actually study roles + Collection existingRoles = cleanRoles(getRolesForUser(entry.getValue(), groupId), studyRoles.keySet()); if (CollectionUtils.subtract(existingRoles, removeRoles).isEmpty()) { throw new DSMBadRequestException("Cannot remove all roles for user " + entry.getKey()); } @@ -309,6 +311,10 @@ protected void validateRoles(List roleNames, Set validRoleNames) throw new DSMBadRequestException(msg + String.join(", ", badRoles)); } + protected Collection cleanRoles(List roleNames, Set validRoleNames) { + return CollectionUtils.retainAll(roleNames, validRoleNames); + } + protected static String validateEmailRequest(String email) { if (StringUtils.isBlank(email)) { throw new DSMBadRequestException("Invalid user email: blank"); 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 fd0a7e5a88..1557f3d26f 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 @@ -334,10 +334,17 @@ public void testSetUserRoles() { // remove one role for both users SetUserRoleRequest req2 = new SetUserRoleRequest(users, List.of(role2)); + + // user has old role that is not in the study + String badRole = "study_admin"; + setUserRoles(usersToId.get(user1), List.of(badRole), groupId); + try { service.setUserRoles(req2); } catch (Exception e) { Assert.fail("Exception from UserAdminService.setUserRoles: " + getStackTrace(e)); + } finally { + addRoleForUser(UserAdminService.getRoleId(badRole), usersToId.get(user1)); } // get roles and verify @@ -463,12 +470,19 @@ public void testUpdateUserRoles() { removeRoleForUser(rolesToId.get(role1), usersToId.get(user2)); // remove last role for one user, which should not be allowed + + // user has old role that is not in the study + String badRole = "study_admin"; + setUserRoles(usersToId.get(user2), List.of(badRole), groupId); + UpdateUserRoleRequest req3 = new UpdateUserRoleRequest(List.of(user2), null, List.of(role2)); try { service.updateUserRoles(req3); Assert.fail("UserAdminService.updateUserRoles should fail to remove all user roles"); } catch (Exception e) { Assert.assertTrue(e.getMessage().contains("Cannot remove all roles for user")); + } finally { + addRoleForUser(UserAdminService.getRoleId(badRole), usersToId.get(user2)); } } @@ -799,16 +813,11 @@ private void addAdminRoles(int adminUserId, int groupId, List roles) { } } - private void setUserRoles(String email, List roles, String studyGroup) { + private void setUserRoles(int userId, List roles, int groupId) { try { - int groupId = UserAdminService.verifyStudyGroup(studyGroup); - int userId = UserAdminService.verifyUserByEmail(email, groupId).getId(); - for (String role : roles) { int roleId = UserAdminService.getRoleId(role); UserAdminService.addUserRole(userId, roleId, groupId); - String msg = String.format("Set up role %s for user %s in study group %s", role, email, studyGroup); - log.info(msg); } } catch (Exception e) { Assert.fail("Exception from UserAdminService.addUserRole: " + getStackTrace(e));