Don't use user.manage permission for packet management permission checks

Author: david-mears-2

api/app/src/main/kotlin/packit/security/PermissionChecker.kt

@@ -10,7 +10,6 @@ interface PermissionChecker {
     fun hasGlobalReadPermission(authorities: List<String> = emptyList()): Boolean
 
     /** Manage packets */
-    fun canManageAllPackets(authorities: List<String> = emptyList()): Boolean
     fun hasPacketManagePermissionForGroup(authorities: List<String> = emptyList(), packetGroupName: String): Boolean
     fun canManagePacketGroup(authorities: List<String> = emptyList(), packetGroupName: String): Boolean
     fun hasPacketManagePermissionForPacket(
@@ -50,17 +49,14 @@ class BasePermissionChecker(private val permissionService: PermissionService) :
         authorities.contains("packet.read")
 
     /** Manage packets */
-    override fun canManageAllPackets(authorities: List<String>): Boolean =
-        hasUserManagePermission(authorities) || hasGlobalPacketManagePermission(authorities)
-
     override fun hasPacketManagePermissionForGroup(
         authorities: List<String>,
         packetGroupName: String
     ): Boolean =
         authorities.contains(permissionService.buildScopedPermission("packet.manage", packetGroupName))
 
     override fun canManagePacketGroup(authorities: List<String>, packetGroupName: String): Boolean =
-        canManageAllPackets(authorities) || hasPacketManagePermissionForGroup(authorities, packetGroupName)
+        hasGlobalPacketManagePermission(authorities) || hasPacketManagePermissionForGroup(authorities, packetGroupName)
 
     override fun hasPacketManagePermissionForPacket(
         authorities: List<String>,
@@ -79,7 +75,7 @@ class BasePermissionChecker(private val permissionService: PermissionService) :
 
     /** Read packets */
     override fun canReadAllPackets(authorities: List<String>): Boolean =
-        canManageAllPackets(authorities) || hasGlobalReadPermission(authorities)
+        hasGlobalPacketManagePermission(authorities) || hasGlobalReadPermission(authorities)
 
     override fun hasPacketReadPermissionForGroup(authorities: List<String>, packetGroupName: String): Boolean =
         authorities.contains(permissionService.buildScopedPermission("packet.read", packetGroupName))

api/app/src/main/kotlin/packit/service/PacketGroupService.kt

@@ -68,7 +68,7 @@ class BasePacketGroupService(
         val allPacketGroups = packetGroupRepository.findAll()
         val authorities = SecurityContextHolder.getContext().authentication.authorities.map { it.authority }
 
-        return if (permissionChecker.canManageAllPackets(authorities)) {
+        return if (permissionChecker.hasGlobalPacketManagePermission(authorities)) {
             allPacketGroups
         } else {
             allPacketGroups.filter {

api/app/src/test/kotlin/packit/unit/service/PacketGroupServiceTest.kt

@@ -178,7 +178,7 @@ class PacketGroupServiceTest {
         whenever(packetGroupRepository.findAll()).thenReturn(allPacketGroups)
         val authorities = listOf("MANAGE_GROUP1", "MANAGE_GROUP2")
         setupSecurityContext(authorities)
-        whenever(permissionChecker.canManageAllPackets(authorities)).thenReturn(false)
+        whenever(permissionChecker.hasGlobalPacketManagePermission(authorities)).thenReturn(false)
         whenever(permissionChecker.hasPacketManagePermissionForGroup(authorities, "group1")).thenReturn(true)
         whenever(permissionChecker.hasPacketManagePermissionForGroup(authorities, "group2")).thenReturn(true)
         whenever(permissionChecker.hasPacketManagePermissionForGroup(authorities, "group3")).thenReturn(false)
@@ -192,7 +192,7 @@ class PacketGroupServiceTest {
         assertEquals(2, result.size)
         assertEquals("group1", result[0].name)
         assertEquals("group2", result[1].name)
-        verify(permissionChecker).canManageAllPackets(authorities)
+        verify(permissionChecker).hasGlobalPacketManagePermission(authorities)
         verify(permissionChecker, times(3)).hasPacketManagePermissionForGroup(any(), any())
     }
 
@@ -207,7 +207,7 @@ class PacketGroupServiceTest {
         whenever(packetGroupRepository.findAll()).thenReturn(allPacketGroups)
         val authorities = listOf("NO_PERMISSIONS")
         setupSecurityContext(authorities)
-        whenever(permissionChecker.canManageAllPackets(authorities)).thenReturn(false)
+        whenever(permissionChecker.hasGlobalPacketManagePermission(authorities)).thenReturn(false)
         whenever(permissionChecker.hasPacketManagePermissionForGroup(eq(authorities), any())).thenReturn(false)
 
         val sut = BasePacketGroupService(packetGroupRepository, packetService, permissionChecker)
@@ -217,7 +217,7 @@ class PacketGroupServiceTest {
 
         // Verify
         assertTrue { result.isEmpty() }
-        verify(permissionChecker).canManageAllPackets(authorities)
+        verify(permissionChecker).hasGlobalPacketManagePermission(authorities)
         verify(permissionChecker, times(3)).hasPacketManagePermissionForGroup(any(), any())
     }
 
@@ -227,7 +227,7 @@ class PacketGroupServiceTest {
         whenever(packetGroupRepository.findAll()).thenReturn(emptyList())
         val authorities = listOf("MANAGE_ALL")
         setupSecurityContext(authorities)
-        whenever(permissionChecker.canManageAllPackets(authorities)).thenReturn(true)
+        whenever(permissionChecker.hasGlobalPacketManagePermission(authorities)).thenReturn(true)
 
         val sut = BasePacketGroupService(packetGroupRepository, packetService, permissionChecker)
 
@@ -236,7 +236,7 @@ class PacketGroupServiceTest {
 
         // Verify
         assertTrue(result.isEmpty())
-        verify(permissionChecker).canManageAllPackets(authorities)
+        verify(permissionChecker).hasGlobalPacketManagePermission(authorities)
         verify(permissionChecker, never()).hasPacketManagePermissionForGroup(any(), any())
     }
 

api/app/src/test/kotlin/packit/unit/service/PermissionCheckerTest.kt

@@ -70,23 +70,6 @@ class PermissionCheckerTest {
 
     @Nested
     inner class ManagePacketsPermissions {
-
-        @Test
-        fun canManageAllPacketsWithUserManage() {
-            assertTrue(permissionChecker.canManageAllPackets(listOf("user.manage")))
-        }
-
-        @Test
-        fun canManageAllPacketsWithGlobalPacketManage() {
-            assertTrue(permissionChecker.canManageAllPackets(listOf("packet.manage")))
-        }
-
-        @Test
-        fun cannotManageAllPacketsWithoutProperPermissions() {
-            assertFalse(permissionChecker.canManageAllPackets(listOf("other.permission")))
-            assertFalse(permissionChecker.canManageAllPackets(emptyList()))
-        }
-
         @Test
         fun hasPacketManagePermissionForGroup() {
             assertTrue(

app/src/lib/auth/hasPermission.ts

@@ -7,14 +7,11 @@ export const hasGlobalPacketManagePermission = (authorities: string[] = []) => a
 export const hasGlobalReadPermission = (authorities: string[] = []) => authorities.includes("packet.read");
 
 /** Manage packets */
-export const canManageAllPackets = (authorities: string[] = []) =>
-  hasUserManagePermission(authorities) || hasGlobalPacketManagePermission(authorities);
-
 export const hasPacketManagePermissionForGroup = (authorities: string[] = [], packetGroupName: string) =>
   authorities.includes(buildScopedPermission("packet.manage", packetGroupName));
 
 export const canManagePacketGroup = (authorities: string[] = [], packetGroupName: string) =>
-  canManageAllPackets(authorities) || hasPacketManagePermissionForGroup(authorities, packetGroupName);
+  hasGlobalPacketManagePermission(authorities) || hasPacketManagePermissionForGroup(authorities, packetGroupName);
 
 export const hasPacketManagePermissionForPacket = (
   authorities: string[] = [],

app/src/tests/lib/auth/hasPermission.test.ts

@@ -1,5 +1,4 @@
 import {
-  canManageAllPackets,
   canManagePacket,
   canManagePacketGroup,
   hasGlobalPacketManagePermission,
@@ -65,20 +64,6 @@ describe("hasPermission functions", () => {
   });
 
   describe("Packet management permission functions", () => {
-    describe("canManageAllPackets", () => {
-      it("returns true when has 'user.manage' authority", () => {
-        expect(canManageAllPackets(["user.manage"])).toBe(true);
-      });
-
-      it("returns true when has 'packet.manage' authority", () => {
-        expect(canManageAllPackets(["packet.manage"])).toBe(true);
-      });
-
-      it("returns false when has neither 'user.manage' nor 'packet.manage' authorities", () => {
-        expect(canManageAllPackets(["packet.manage:packetGroup:groupA", "packet.run"])).toBe(false);
-      });
-    });
-
     describe("hasPacketManagePermissionForGroup", () => {
       it("returns true when has scoped manage permission for the group", () => {
         expect(hasPacketManagePermissionForGroup(["packet.manage:packetGroup:groupA"], "groupA")).toBe(true);