Ensure policies are correctly prioritized (#22967)

Co-authored-by: Hannes Küttner <kuettner.hannes@gmail.com>

.changeset/tasty-guests-fry.md

@@ -0,0 +1,5 @@
+---
+'@directus/api': patch
+---
+
+Ensured that policies attached to a user, role and parent roles are correctly prioritized

api/src/permissions/lib/fetch-policies.test.ts

@@ -1,9 +1,8 @@
 import type { Accountability } from '@directus/types';
 import { beforeEach, expect, test, vi } from 'vitest';
 import { AccessService } from '../../services/access.js';
-import type { AccessRow } from '../modules/process-ast/types.js';
 import type { Context } from '../types.js';
-import { _fetchPolicies as fetchPolicies } from './fetch-policies.js';
+import { _fetchPolicies as fetchPolicies, type AccessRow } from './fetch-policies.js';
 
 vi.mock('../../services/access.js', () => ({
 	AccessService: vi.fn(),
@@ -42,7 +41,7 @@ test('Fetches policies for public role and user when user is given without role'
 				},
 			],
 		},
-		fields: ['policy.id', 'policy.ip_access'],
+		fields: ['policy.id', 'policy.ip_access', 'role'],
 		limit: -1,
 	});
 
@@ -69,7 +68,7 @@ test('Fetches policies for public role when no roles and user are given', async
 				},
 			],
 		},
-		fields: ['policy.id', 'policy.ip_access'],
+		fields: ['policy.id', 'policy.ip_access', 'role'],
 		limit: -1,
 	});
 
@@ -87,7 +86,7 @@ test('Fetched policies for user roles', async () => {
 				_in: ['role-a', 'role-b'],
 			},
 		},
-		fields: ['policy.id', 'policy.ip_access'],
+		fields: ['policy.id', 'policy.ip_access', 'role'],
 		limit: -1,
 	});
 
@@ -114,7 +113,7 @@ test('Fetches policies for user roles and user if user is passed', async () => {
 				},
 			],
 		},
-		fields: ['policy.id', 'policy.ip_access'],
+		fields: ['policy.id', 'policy.ip_access', 'role'],
 		limit: -1,
 	});
 
@@ -130,12 +129,14 @@ test('Filters policies based on ip access on access row', async () => {
 				id: 'policy-a',
 				ip_access: ['127.0.0.0/29'],
 			},
+			role: null,
 		},
 		{
 			policy: {
 				id: 'policy-b',
 				ip_access: ['1.1.1.1/32'],
 			},
+			role: null,
 		},
 	);
 
@@ -143,3 +144,42 @@ test('Filters policies based on ip access on access row', async () => {
 
 	expect(policies).toEqual(['policy-a']);
 });
+
+test('Sorts policies by priority', async () => {
+	const acc = { roles: ['role-a', 'role-b'], user: 'user-a' } as unknown as Accountability;
+
+	rows.push(
+		{
+			policy: {
+				id: 'policy-c',
+				ip_access: null,
+			},
+			role: null,
+		},
+		{
+			policy: {
+				id: 'policy-d',
+				ip_access: null,
+			},
+			role: null,
+		},
+		{
+			policy: {
+				id: 'policy-b',
+				ip_access: null,
+			},
+			role: 'role-b',
+		},
+		{
+			policy: {
+				id: 'policy-a',
+				ip_access: null,
+			},
+			role: 'role-a',
+		},
+	);
+
+	const policies = await fetchPolicies(acc, {} as Context);
+
+	expect(policies).toEqual(['policy-a', 'policy-b', 'policy-c', 'policy-d']);
+});

api/src/permissions/lib/fetch-policies.ts

@@ -1,8 +1,12 @@
 import type { Accountability, Filter } from '@directus/types';
-import type { AccessRow } from '../modules/process-ast/types.js';
+import type { Context } from '../types.js';
 import { filterPoliciesByIp } from '../utils/filter-policies-by-ip.js';
 import { withCache } from '../utils/with-cache.js';
-import type { Context } from '../types.js';
+
+export interface AccessRow {
+	policy: { id: string; ip_access: string[] | null };
+	role: string | null;
+}
 
 export const fetchPolicies = withCache('policies', _fetchPolicies, ({ roles, user, ip }) => ({ roles, user, ip }));
 
@@ -30,11 +34,26 @@ export async function _fetchPolicies(
 
 	const accessRows = (await accessService.readByQuery({
 		filter,
-		fields: ['policy.id', 'policy.ip_access'],
+		fields: ['policy.id', 'policy.ip_access', 'role'],
 		limit: -1,
 	})) as AccessRow[];
 
 	const filteredAccessRows = filterPoliciesByIp(accessRows, ip);
+
+	/*
+	 * Sort rows by priority (goes bottom up):
+	 * - Parent role policies
+	 * - Child role policies
+	 * - User policies
+	 */
+	filteredAccessRows.sort((a, b) => {
+		if (!a.role && !b.role) return 0;
+		if (!a.role) return 1;
+		if (!b.role) return -1;
+
+		return roles.indexOf(a.role) - roles.indexOf(b.role);
+	});
+
 	const ids = filteredAccessRows.map(({ policy }) => policy.id);
 
 	return ids;

api/src/permissions/modules/process-ast/types.ts

@@ -15,7 +15,3 @@ export type FieldMap = {
 	read: FieldMapEntries;
 	other: FieldMapEntries;
 };
-
-export interface AccessRow {
-	policy: { id: string; ip_access: string[] | null };
-}

api/src/permissions/utils/filter-policies-by-ip.ts

@@ -1,5 +1,5 @@
 import { ipInNetworks } from '../../utils/ip-in-networks.js';
-import type { AccessRow } from '../modules/process-ast/types.js';
+import type { AccessRow } from '../lib/fetch-policies.js';
 
 export function filterPoliciesByIp(policies: AccessRow[], ip: string | null | undefined) {
 	return policies.filter(({ policy }) => {