Handle user roles that are not study roles

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();
     }
 }

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<String> existingRoles = userRoles.get(entry.getKey());
+            // filter existing user roles that are not actually study roles
+            Collection<String> existingRoles = cleanRoles(userRoles.get(entry.getKey()), studyRoles.keySet());
             Collection<String> rolesToAdd = CollectionUtils.subtract(roleNames, existingRoles);
             Collection<String> 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<String> existingRoles = getRolesForUser(entry.getValue(), groupId);
+                // filter existing user roles that are not actually study roles
+                Collection<String> 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<String> roleNames, Set<String> validRoleNames)
         throw new DSMBadRequestException(msg + String.join(", ", badRoles));
     }
 
+    protected Collection<String> cleanRoles(List<String> roleNames, Set<String> validRoleNames) {
+        return CollectionUtils.retainAll(roleNames, validRoleNames);
+    }
+
     protected static String validateEmailRequest(String email) {
         if (StringUtils.isBlank(email)) {
             throw new DSMBadRequestException("Invalid user email: blank");

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<String> roles) {
         }
     }
 
-    private void setUserRoles(String email, List<String> roles, String studyGroup) {
+    private void setUserRoles(int userId, List<String> 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));