Added a permission for editing member roles, disallow removal of edit permissions from admin, and fixed issues with user edit/delete.

ocielliottc · ocielliottc · commit fbe0b184e788 · 2025-01-20T07:42:48.000-06:00
diff --git a/server/src/main/java/com/objectcomputing/checkins/services/memberprofile/MemberProfileServicesImpl.java b/server/src/main/java/com/objectcomputing/checkins/services/memberprofile/MemberProfileServicesImpl.java
@@ -160,7 +160,7 @@ public boolean deleteProfile(@NotNull UUID id) {
         } else if (!teamMemberServices.findByFields(null, id, null).isEmpty()) {
             throw new BadArgException(String.format("User %s cannot be deleted since TeamMember record(s) exist", MemberProfileUtils.getFullName(memberProfile)));
         } else if (!userRoles.isEmpty()) {
-            throw new BadArgException(String.format("User %s cannot be deleted since user has PDL role", MemberProfileUtils.getFullName(memberProfile)));
+            throw new BadArgException(String.format("User %s cannot be deleted since user has one or more roles", MemberProfileUtils.getFullName(memberProfile)));
         }
 
         // Update PDL ID for all associated members before termination
diff --git a/server/src/main/java/com/objectcomputing/checkins/services/permissions/Permission.java b/server/src/main/java/com/objectcomputing/checkins/services/permissions/Permission.java
@@ -19,6 +19,7 @@ public enum Permission {
   CAN_IMPERSONATE_MEMBERS("Impersonate organization members", "Security"),
   CAN_VIEW_ROLE_PERMISSIONS("View role permissions", "Security"),
   CAN_ASSIGN_ROLE_PERMISSIONS("Assign role permissions", "Security"),
+  CAN_EDIT_MEMBER_ROLES("Edit member roles", "Security"),
   CAN_VIEW_PERMISSIONS("View all permissions", "Security"),
   CAN_VIEW_SKILLS_REPORT("View skills report", "Reporting"),
   CAN_VIEW_RETENTION_REPORT("View retention report", "Reporting"),
diff --git a/server/src/main/java/com/objectcomputing/checkins/services/role/member_roles/MemberRoleController.java b/server/src/main/java/com/objectcomputing/checkins/services/role/member_roles/MemberRoleController.java
@@ -1,5 +1,8 @@
 package com.objectcomputing.checkins.services.role.member_roles;
 
+import com.objectcomputing.checkins.services.permissions.Permission;
+import com.objectcomputing.checkins.services.permissions.RequiredPermission;
+
 import io.micronaut.http.HttpResponse;
 import io.micronaut.http.annotation.*;
 import io.micronaut.scheduling.TaskExecutors;
@@ -29,13 +32,15 @@ HttpResponse<List<MemberRole>> getAllAssignedMemberRoles() {
     }
 
     @Delete("/{roleId}/{memberId}")
+    @RequiredPermission(Permission.CAN_EDIT_MEMBER_ROLES)
     HttpResponse<?> deleteMemberRole(@NotNull UUID roleId, @NotNull UUID memberId){
         memberRoleServices.delete(new MemberRoleId(memberId, roleId));
         return HttpResponse.ok();
     }
 
     @Post
     @CacheInvalidate(cacheNames = {"role-permission-cache"})
+    @RequiredPermission(Permission.CAN_EDIT_MEMBER_ROLES)
     HttpResponse<MemberRole> saveMemberRole(@NotNull @Body MemberRoleId id){
         MemberRole memberRole = memberRoleServices.saveByIds(id.getMemberId(), id.getRoleId());
         return HttpResponse.ok(memberRole);
diff --git a/server/src/main/java/com/objectcomputing/checkins/services/role/role_permissions/RolePermissionServicesImpl.java b/server/src/main/java/com/objectcomputing/checkins/services/role/role_permissions/RolePermissionServicesImpl.java
@@ -1,9 +1,11 @@
 package com.objectcomputing.checkins.services.role.role_permissions;
 
+import com.objectcomputing.checkins.exceptions.BadArgException;
 import com.objectcomputing.checkins.services.permissions.Permission;
 import com.objectcomputing.checkins.services.permissions.PermissionDTO;
 import com.objectcomputing.checkins.services.permissions.PermissionServices;
 import com.objectcomputing.checkins.services.role.Role;
+import com.objectcomputing.checkins.services.role.RoleType;
 import com.objectcomputing.checkins.services.role.RoleServices;
 import io.micronaut.cache.annotation.CacheConfig;
 import io.micronaut.cache.annotation.CacheInvalidate;
@@ -70,6 +72,13 @@ public RolePermission save(UUID roleId, Permission permissionId) {
     @CacheInvalidate(all = true)
     @Override
     public void delete(UUID roleId, Permission permissionId) {
+        // Disallow removal of Assign Role Permissions from ADMIN
+        if (permissionId == Permission.CAN_ASSIGN_ROLE_PERMISSIONS) {
+          Role role = roleServices.read(roleId);
+          if (role.getRole().equals(RoleType.Constants.ADMIN_ROLE)) {
+              throw new BadArgException("Cannot remove Assign Role Permissions from ADMIN");
+          }
+        }
         rolePermissionRepository.deleteByIds(roleId.toString(), permissionId);
     }
 
diff --git a/server/src/main/resources/db/dev/R__Load_testing_data.sql b/server/src/main/resources/db/dev/R__Load_testing_data.sql
@@ -633,6 +633,11 @@ VALUES
 ('fa2319a4-8db6-4b83-bba7-70a3e4d7670f', '2024-05-21', '1b4f99da-ef70-4a76-9b37-8bb783b749ad',  PGP_SYM_ENCRYPT('internal #9','${aeskey}'), PGP_SYM_ENCRYPT('extenal #9','${aeskey}'), 4, 5);
 
 -- Admin Permissions
+insert into role_permissions
+    (roleid, permission)
+values
+    ('e8a4fff8-e984-4e59-be84-a713c9fa8d23', 'CAN_EDIT_MEMBER_ROLES');
+
 insert into role_permissions
     (roleid, permission)
 values
diff --git a/server/src/test/java/com/objectcomputing/checkins/services/fixture/PermissionFixture.java b/server/src/test/java/com/objectcomputing/checkins/services/fixture/PermissionFixture.java
@@ -47,6 +47,7 @@ public interface PermissionFixture extends RolePermissionFixture {
 
     // Add ADMIN Permissions here
     List<Permission> adminPermissions = List.of(
+        Permission.CAN_EDIT_MEMBER_ROLES,
         Permission.CAN_VIEW_FEEDBACK_REQUEST,
         Permission.CAN_CREATE_FEEDBACK_REQUEST,
         Permission.CAN_DELETE_FEEDBACK_REQUEST,
diff --git a/server/src/test/java/com/objectcomputing/checkins/services/member_role/MemberRoleControllerTest.java b/server/src/test/java/com/objectcomputing/checkins/services/member_role/MemberRoleControllerTest.java
@@ -14,6 +14,7 @@
 import io.micronaut.http.HttpStatus;
 import io.micronaut.http.client.HttpClient;
 import io.micronaut.http.client.annotation.Client;
+import io.micronaut.http.client.exceptions.HttpClientResponseException;
 import jakarta.inject.Inject;
 import org.junit.jupiter.api.Test;
 
@@ -69,6 +70,18 @@ void testDeleteMemberRole() {
         assertEquals(HttpStatus.OK, response.getStatus());
     }
 
+    @Test
+    void testDeleteMemberRoleNoPermission() {
+        MemberProfile member = createADefaultMemberProfile();
+
+        Role memberRole = createAndAssignRole(RoleType.MEMBER, member);
+
+        final HttpRequest<Object> request = HttpRequest.DELETE(String.format("/%s/%s", memberRole.getId(), member.getId()))
+                .basicAuth(member.getWorkEmail(), RoleType.Constants.MEMBER_ROLE);
+        HttpClientResponseException e = assertThrows(HttpClientResponseException.class, () -> client.toBlocking().exchange(request, MemberRole.class));
+        assertEquals(HttpStatus.FORBIDDEN, e.getStatus());
+    }
+
     @Test
     void testSaveMemberRole() {
         MemberProfile member = createADefaultMemberProfile();
@@ -89,4 +102,17 @@ void testSaveMemberRole() {
         assertEquals(adminRole.getId(), response.getBody().get().getMemberRoleId().getRoleId());
     }
 
+    @Test
+    void testSaveMemberRoleNoPermission() {
+        MemberProfile member = createADefaultMemberProfile();
+        Role memberRole = createAndAssignRole(RoleType.MEMBER, member);
+
+        MemberRoleId memberRoleId = new MemberRoleId(member.getId(), memberRole.getId());
+
+        final HttpRequest<?> request = HttpRequest.POST("", memberRoleId)
+                .basicAuth(member.getWorkEmail(), RoleType.Constants.MEMBER_ROLE);
+
+        HttpClientResponseException e = assertThrows(HttpClientResponseException.class, () -> client.toBlocking().exchange(request, MemberRole.class));
+        assertEquals(HttpStatus.FORBIDDEN, e.getStatus());
+    }
 }
diff --git a/server/src/test/java/com/objectcomputing/checkins/services/role/role_permissions/RolePermissionsControllerTest.java b/server/src/test/java/com/objectcomputing/checkins/services/role/role_permissions/RolePermissionsControllerTest.java
@@ -112,4 +112,19 @@ void testRemovePermissionFromRole() {
         assertEquals(HttpStatus.OK, response.getStatus());
         assertEquals(0, getRolePermissionRepository().findByIds(memberRole.getId().toString(), birthdayPermission).size());
     }
+
+    @Test
+    void testRemoveAssignRolePermissionFromAdminRole() {
+        MemberProfile sender = createADefaultMemberProfile();
+        assignAdminRole(sender);
+
+        Role adminRole = getRoleRepository().findByRole(RoleType.ADMIN.name()).orElseThrow();
+        Permission permission = Permission.CAN_ASSIGN_ROLE_PERMISSIONS;
+
+        final HttpRequest<?> request = HttpRequest.DELETE("/", new RolePermissionDTO(adminRole.getId(), permission))
+                .basicAuth(sender.getWorkEmail(), RoleType.Constants.ADMIN_ROLE);
+        HttpClientResponseException e = assertThrows(HttpClientResponseException.class,
+                () -> client.toBlocking().exchange(request, Map.class));
+        assertEquals(HttpStatus.BAD_REQUEST, e.getStatus());
+    }
 }
diff --git a/web-ui/src/components/admin/permissions/Permissions.jsx b/web-ui/src/components/admin/permissions/Permissions.jsx
@@ -2,9 +2,17 @@ import { useMediaQuery } from '@mui/material';
 import React from 'react';
 import DesktopTable from './DesktopTable';
 import MobileTable from './MobileTable';
+import { AppContext } from '../../../context/AppContext';
+import {
+  selectHasPermissionAssignmentPermission,
+  noPermission,
+} from '../../../context/selectors';
 
 export default function Permissions() {
+  const { state } = useContext(AppContext);
   const showDesktop = useMediaQuery('(min-width:650px)', { noSsr: true });
 
-  return <div>{showDesktop ? <DesktopTable /> : <MobileTable />}</div>;
+  return selectHasPermissionAssignmentPermission(state) ?
+           (<div>{showDesktop ? <DesktopTable /> : <MobileTable />}</div>) :
+           (<h3>{noPermission}</h3>);
 }
diff --git a/web-ui/src/components/admin/roles/Roles.jsx b/web-ui/src/components/admin/roles/Roles.jsx
@@ -13,7 +13,7 @@ import {
   updateRole
 } from '../../../api/roles';
 import {
-  selectHasRoleAssignmentPermission,
+  selectCanEditMemberRolesPermission,
   noPermission,
 } from '../../../context/selectors';
 import RoleUserCards from './RoleUserCards';
@@ -215,7 +215,7 @@ const Roles = () => {
     setEditedRole({ ...editedRole, description: event?.target?.value });
   };
 
-  return selectHasRoleAssignmentPermission(state) ? (
+  return selectCanEditMemberRolesPermission(state) ? (
     <div className="roles-content">
       <div className="roles">
         <div className="roles-top">
diff --git a/web-ui/src/components/member-directory/AdminMemberCard.jsx b/web-ui/src/components/member-directory/AdminMemberCard.jsx
@@ -76,34 +76,39 @@ const AdminMemberCard = ({ member, index }) => {
   const handleClose = () => setOpen(false);
   const handleCloseDeleteConfirmation = () => setOpenDelete(false);
 
+  const actionFunctions = [];
+
   const options = () => {
     let entries = [];
+    actionFunctions.length = 0;
+
     // This is "Create" permission because there is no "Edit" permission.  This
     // is due to the fact that users can edit their own profiles.  But, only
     // certain users can create new profiles.  So, we associate the edit feature
     // with profile creation.
     if (selectHasCreateMembersPermission(state)) {
       entries.push('Edit');
+      actionFunctions.push(handleOpen);
     }
     if (selectHasDeleteMembersPermission(state)) {
       entries.push('Delete');
+      actionFunctions.push(handleOpenDeleteConfirmation);
     }
     if (selectHasImpersonateMembersPermission(state)) {
       // If we have not already impersonated a user, we can provide that option.
       if (document.cookie.indexOf("OJWT=") == -1) {
         entries.push('Impersonate');
+        actionFunctions.push(handleImpersonate);
       }
     }
     return entries;
   }
 
   const handleAction = (e, index) => {
-    if (index === 0) {
-      handleOpen();
-    } else if (index === 1) {
-      handleOpenDeleteConfirmation();
-    } else if (index === 2) {
-      handleImpersonate();
+    if (index < actionFunctions.length) {
+      actionFunctions[index]();
+    } else {
+      console.warn(`${index} is out of range for the action functions`);
     }
   };
 
diff --git a/web-ui/src/components/menu/Menu.jsx b/web-ui/src/components/menu/Menu.jsx
@@ -19,14 +19,15 @@ import {
   selectHasVolunteeringEventsPermission,
   selectHasVolunteeringRelationshipsPermission,
   selectHasAdministerKudosPermission,
-  selectHasRoleAssignmentPermission,
+  selectCanEditMemberRolesPermission,
   selectHasSendEmailPermission,
   selectHasViewSettingsPermission,
   selectCanEditSkills,
   selectHasCreateMembersPermission,
   selectHasDeleteMembersPermission,
   selectHasImpersonateMembersPermission,
   selectHasUploadHoursPermission,
+  selectHasPermissionAssignmentPermission,
 } from '../../context/selectors';
 import { UPDATE_TOAST } from '../../context/actions';
 
@@ -139,8 +140,8 @@ function Menu({ children }) {
 
   const adminLinks = [
     ['/admin/manage-kudos', 'Manage Kudos', () => selectHasAdministerKudosPermission(state)],
-    ['/admin/permissions', 'Permissions', () => isAdmin],
-    ['/admin/roles', 'Roles', () => selectHasRoleAssignmentPermission(state)],
+    ['/admin/permissions', 'Permissions', () => selectHasPermissionAssignmentPermission(state)],
+    ['/admin/roles', 'Roles', () => selectCanEditMemberRolesPermission(state)],
     ['/admin/users', 'Users', () => selectHasCreateMembersPermission(state) ||
                                     selectHasDeleteMembersPermission(state) ||
                                     selectHasImpersonateMembersPermission(state)
diff --git a/web-ui/src/context/selectors.js b/web-ui/src/context/selectors.js
@@ -52,6 +52,10 @@ export const selectHasPermissionAssignmentPermission = hasPermission(
   'CAN_ASSIGN_ROLE_PERMISSIONS'
 );
 
+export const selectHasViewPermissionPermission = hasPermission(
+  'CAN_VIEW_ROLE_PERMISSIONS'
+);
+
 export const selectHasReportPermission = hasPermission('REPORT');
 
 export const selectCanViewFeedbackAnswerPermission = hasPermission(
@@ -154,8 +158,8 @@ export const selectHasVolunteeringRelationshipsPermission = hasPermission(
   'CAN_ADMINISTER_VOLUNTEERING_RELATIONSHIPS'
 );
 
-export const selectHasRoleAssignmentPermission = hasPermission(
-  'CAN_ASSIGN_ROLE_PERMISSIONS'
+export const selectCanEditMemberRolesPermission = hasPermission(
+  'CAN_EDIT_MEMBER_ROLES'
 );
 
 export const selectHasCreateFeedbackPermission = hasPermission(
diff --git a/web-ui/src/pages/ManageKudosPage.jsx b/web-ui/src/pages/ManageKudosPage.jsx
@@ -79,20 +79,24 @@ const ManageKudosPage = () => {
   };
 
   const loadPendingKudos = useCallback(async () => {
-    setPendingKudosLoading(true);
-    const res = await getAllKudos(csrf, true);
-    if (res?.payload?.data && !res.error) {
-      setPendingKudosLoading(false);
-      return res.payload.data;
+    if (selectHasAdministerKudosPermission(state)) {
+      setPendingKudosLoading(true);
+      const res = await getAllKudos(csrf, true);
+      if (res?.payload?.data && !res.error) {
+        setPendingKudosLoading(false);
+        return res.payload.data;
+      }
     }
   }, [csrf, dispatch]);
 
   const loadApprovedKudos = useCallback(async () => {
-    setApprovedKudosLoading(true);
-    const res = await getAllKudos(csrf, false);
-    if (res?.payload?.data && !res.error) {
-      setApprovedKudosLoading(false);
-      return res.payload.data;
+    if (selectHasAdministerKudosPermission(state)) {
+      setApprovedKudosLoading(true);
+      const res = await getAllKudos(csrf, false);
+      if (res?.payload?.data && !res.error) {
+        setApprovedKudosLoading(false);
+        return res.payload.data;
+      }
     }
   }, [csrf, dispatch]);
 
diff --git a/web-ui/src/pages/PermissionsPage.jsx b/web-ui/src/pages/PermissionsPage.jsx
@@ -18,6 +18,7 @@ import { AppContext } from '../context/AppContext';
 import {
   selectRoles,
   selectHasPermissionAssignmentPermission,
+  selectHasViewPermissionPermission,
   noPermission,
 } from '../context/selectors';
 import { useQueryParameters } from '../helpers/query-parameters';
@@ -91,10 +92,12 @@ const EditPermissionsPage = () => {
 
   useEffect(() => {
     const getRolePermissions = async () => {
-      const res = await getRolePermissionsList(csrf);
-      const data = res.payload?.data && !res.error ? res.payload.data : null;
-      if (data) {
-        setRolePermissionsList(data);
+      if (selectHasViewPermissionPermission(state)) {
+        const res = await getRolePermissionsList(csrf);
+        const data = res.payload?.data && !res.error ? res.payload.data : null;
+        if (data) {
+          setRolePermissionsList(data);
+        }
       }
     };
     const getPermissions = async () => {

Original file line number	Diff line number	Diff line change
`@@ -160,7 +160,7 @@ public boolean deleteProfile(@NotNull UUID id) {`
`160`	`160`	`} else if (!teamMemberServices.findByFields(null, id, null).isEmpty()) {`
`161`	`161`	`throw new BadArgException(String.format("User %s cannot be deleted since TeamMember record(s) exist", MemberProfileUtils.getFullName(memberProfile)));`
`162`	`162`	`} else if (!userRoles.isEmpty()) {`
`163`		`- throw new BadArgException(String.format("User %s cannot be deleted since user has PDL role", MemberProfileUtils.getFullName(memberProfile)));`
	`163`	`+ throw new BadArgException(String.format("User %s cannot be deleted since user has one or more roles", MemberProfileUtils.getFullName(memberProfile)));`
`164`	`164`	`}`
`165`	`165`
`166`	`166`	`// Update PDL ID for all associated members before termination`