Implement permission policies in the API #22384

rijkvanzanten · 2024-05-03T22:33:45Z

Scope

What's changed:

Implements policy-system based permissions handling on the API
Replaces AuthorizationService with new set of functions in the api/src/permissions folder
Allows roles to be nested
Adds new roles flag to accountability object. This is an ordered array of all the parent roles of the current user
Cleans up get-ast-from-query by splitting it into multiple files
Permissions are now injected into the AST through cases and whenCase. This allows us to dynamically generate the case/when SQL to have dynamic field output per item.
Cleans up run-ast by splitting it up into smaller files

Potential Risks / Drawbacks

The risks are very high. This replaces the full permissions system, and thus needs a lot of testing

Review Notes / Questions

This PR now compiles, but doesn't run yet.
There was some weird logic happening in the users controller for TFA enable/disable that I'm not sure we need to keep. Needs a bit more testing

Todos

Closes #21778, closes #21765, closes #22163, closes #21769, closes #21768, closes #21767, closes #21766

Eg check to make sure there's still >=1 admin left after the mutation is done ↩

Co-authored-by: Daniel Biegler <[email protected]>

…access service

…dmin policy and add on delete trigger.

to be fair: unit test is also testing implementation details but thats a problem there in general and for future us

…ission fetching

…e might missbehave with the permissions obtained from PermissionsService.readByQuery so it better to go the source of the problem

hanneskuettner · 2024-06-06T08:48:12Z

api/src/controllers/policies.ts

+router.search('/', validateBatch('read'), readHandler, respond);
+
+router.get(
+	'/me/flags',


Anyone have a better name for this? Since in the future it might not only be flags. Should it follow the naming of the libs and maybe be called /me/global? The idea behind this endpoint is to accumulate any properties that are derived when taking all policies of a user into account, like global access and currently the enfore_tfa flag.

Mhh agreed /me/globals sounds better than flags to me too.

The idea behind this endpoint is to accumulate any properties

Mhh so maybe /me/properties ? 😄

If we dont expand on it /me/access sounds pretty fitting for right now, but yeah that might become less good in the future.

Would just /me work? 👀

Hmm, I personally dislike just /me because the endpoint does not return any policies, but some aggregation of all policies of the user, and we might end up with an actual /me endpoint that returns all policies at some point? wdyt?

Can you give an quick example of how the current response looks like?

It is effectively a condensed version of whatever flags we have on policies right now, that apply to a user.

{ "app_access": true, "admin_access": false, "enforce_tfa": false }

Okay, I think global(s) sounds more appropriate, that's ultimately what we're retrieving here: global flags/properties/whatever.

but some aggregation of all policies of the user, and we might end up with an actual /me endpoint that returns all policies at some point? wdyt?

You mean at a later point or is that something you'd like to look into now?

At a later point. Currently I don't see the need of retrieving all the policies of a user efficiently, as the app does not need to know them to work. But maybe that need arises at some point in the future, so I'd like to keep that option alive.

…tiple permissions for collection+action

…ccess) and policy updates (directus_policies) and permissions updates (directus_permissions)

hanneskuettner · 2024-06-06T14:55:25Z

api/src/permissions/modules/process-ast/utils/validate-path.ts

+		throw new ForbiddenError({
+			reason: `You don't have permission to access collection "${collection}" or it does not exist. Queried in ${pathSuffix}.`,
+		});


Even though this, and the other errors are really helpful, they do expose that the collection + field exists, as it differs from the standard ForbiddenError in case of a non-existent collection.

…imilar to /users/me

licitdev · 2024-06-10T06:07:13Z

api/src/app.test.ts

-vi.mock('./middleware/check-ip', () => ({
-	checkIP: Router(),
-}));
-


The mocking for ./middleware/get-permissions below should be removed too

licitdev · 2024-06-10T06:56:51Z

api/src/cli/commands/init/index.ts

-		id: roleID,
-		...defaultAdminRole,
-	});
+	await db('directus_roles').insert({ ...defaultAdminRole, id: roleID });


A new policy should be created

Suggested change

await db('directus_roles').insert({ ...defaultAdminRole, id: roleID });

await db('directus_roles').insert({ ...defaultAdminRole, id: roleID });

await db('directus_policies').insert({ ...defaultAdminPolicy, id: policyID });

await db('directus_access').insert({ policy: policyID, role: roleID });

licitdev · 2024-06-10T07:54:04Z

api/src/controllers/policies.ts

+router.search('/', validateBatch('read'), readHandler, respond);
+
+router.get(
+	'/me/flags',


Would just /me work? 👀

licitdev · 2024-06-10T08:01:09Z

api/src/controllers/roles.ts

+		const query = { ...req.sanitizedQuery, limit: -1 };
+
+		const roles = await service.readMany(req.accountability.roles, query);


Should we add the same comment as per other usages of this limit: -1 query?

Suggested change

const query = { ...req.sanitizedQuery, limit: -1 };

const roles = await service.readMany(req.accountability.roles, query);

// TODO fix this at the service level

const temporaryQuery = { ...req.sanitizedQuery, limit: -1 };

const roles = await service.readMany(req.accountability.roles, temporaryQuery);

licitdev · 2024-06-10T09:05:33Z

api/src/services/permissions.ts

+		if (this.accountability?.admin) {
+			return mapValues(this.schema.collections, () =>
+				Object.fromEntries(
+					['create', 'read', 'update', 'delete', 'share'].map((action) => [


Could we extract this array as a constant? This array will be used in the App as well.

licitdev · 2024-06-10T11:13:10Z

api/src/controllers/roles.ts

+		const query = { ...req.sanitizedQuery, limit: -1 };
+
+		try {
+			const roles = await service.readMany(req.accountability.roles, query);


Should we include this comment about it being temporary to be consistent with other usages?

Suggested change

const query = { ...req.sanitizedQuery, limit: -1 };

try {

const roles = await service.readMany(req.accountability.roles, query);

// TODO fix this at the service level

const temporaryQuery = { ...req.sanitizedQuery, limit: -1 };

try {

const roles = await service.readMany(req.accountability.roles, temporaryQuery);

Co-authored-by: Hannes Küttner <[email protected]>

rijkvanzanten and others added 30 commits March 21, 2024 14:21

WIP start on migrations

54b67d4

Add migration

9faf285

Don't insert if there's no rows

afde4d7

Use service to read/write permissions

69479f4

Use payload service rather than itemsservice

3bf7a80

Start on downgrade command

054bb60

Update system data structure

0a226a3

Update migrations to keep structure flat

7ab5c01

Remove icon from policies

716cf58

Drop policies table on downgrade

eb54dd9

Rearchitect migrations to structure v3

458eb13

Add down migration

a8a15ec

Update system fields

831913f

Add policy to fields import

5cb5af9

Merge branch 'auditus' into auditus-21760

f1b4257

Merge branch 'auditus' into auditus-21760

376bd0a

Merge branch 'auditus' into auditus-21760

b322ae8

Fix public role attachment

4b152bf

Update packages/system-data/src/fields/index.ts

4c7d828

Co-authored-by: Daniel Biegler <[email protected]>

Update packages/system-data/src/fields/policies.yaml

93efe32

Co-authored-by: Daniel Biegler <[email protected]>

Add nested roles

9f20d13

Remove unused step

ab7b03c

Use two o2ms instead of m2a for attachments

a3bdef0

Update system data

655c0c4

[WIP] Start reorging permissions handling

4a0946d

Merge branch 'auditus' into auditus-21765

4badbff

Setup field extraction

275fcf6

Remove watch from vitest

4534ffe

Finish fieldMap creation logic

c2cd7a2

Add tests for utils

f274910

2chiefk approved these changes Jun 3, 2024

View reviewed changes

directus deleted a comment from 2chiefk Jun 3, 2024

hanneskuettner and others added 11 commits June 4, 2024 17:50

Add access and policies controller, add util methods to policies and …

d840e21

…access service

Change name and description of public policy, update description of a…

c9f4735

…dmin policy and add on delete trigger.

Make sure access row uuids are auto generated

82407fe

optimize kvredis clear function and add a unit test

10b4c5c

to be fair: unit test is also testing implementation details but thats a problem there in general and for future us

Add minimal app permission and dynamic variable injection to the perm…

3c0b6be

…ission fetching

Fix m2o collection name in extractFieldsFromChildren

366212d

Make sure to clone permission before injecting dynamic variables

ccf9537

Actually do the cloning in with withAppMinimalPermissions since peopl…

422b2a5

…e might missbehave with the permissions obtained from PermissionsService.readByQuery so it better to go the source of the problem

Use knex transaction in createOne -> processPayload - otherwise deadlock

1b581f5

Make sure to respect '*' field in allowed fields

5e977ec

Fix extractFieldsFromChildren for o2m as well - classic

f5ac386

hanneskuettner reviewed Jun 6, 2024

View reviewed changes

hanneskuettner and others added 8 commits June 6, 2024 11:17

Fix allowed field check in FieldsService.readAll to account for mul…

26bb625

…tiple permissions for collection+action

Skip case/when if allowedFields includes '*'

0a9e551

Restructure the way the current users permissions are returned

653ca57

add ability to clear all keys from memory cache

d033298

add test for clear method

30c7640

add await to clear function

ea9759f

Clear permissions caches on changes to policy attachments (directus_a…

244be69

…ccess) and policy updates (directus_policies) and permissions updates (directus_permissions)

Make the public role a real role rather than a virtual one

c45681d

hanneskuettner reviewed Jun 6, 2024

View reviewed changes

hanneskuettner and others added 5 commits June 6, 2024 17:14

Inject the public role, we're it previously was null

c753987

Revert adding a fix public role

305fee4

remove unused variable

b2fa779

Ensure that a user without a role can still use the /me util endpoints

91c7a30

Make sure that the /me endpoints always return minimal information, s…

5bcdedc

…imilar to /users/me

licitdev reviewed Jun 10, 2024

View reviewed changes

Update api/src/permissions/utils/with-cache.ts

90a9594

Co-authored-by: Hannes Küttner <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement permission policies in the API #22384

Implement permission policies in the API #22384

rijkvanzanten commented May 3, 2024 •

edited by hanneskuettner

hanneskuettner Jun 6, 2024

DanielBiegler Jun 6, 2024 •

edited

licitdev Jun 10, 2024

hanneskuettner Jun 10, 2024

paescuj Jun 10, 2024

hanneskuettner Jun 10, 2024

paescuj Jun 10, 2024

hanneskuettner Jun 10, 2024

hanneskuettner Jun 6, 2024

licitdev Jun 10, 2024

licitdev Jun 10, 2024

licitdev Jun 10, 2024

licitdev Jun 10, 2024

licitdev Jun 10, 2024

licitdev Jun 10, 2024

		const query = { ...req.sanitizedQuery, limit: -1 };

		const roles = await service.readMany(req.accountability.roles, query);

-		const query = { ...req.sanitizedQuery, limit: -1 };
-		const roles = await service.readMany(req.accountability.roles, query);
+		// TODO fix this at the service level
+		const temporaryQuery = { ...req.sanitizedQuery, limit: -1 };
+		const roles = await service.readMany(req.accountability.roles, temporaryQuery);

Implement permission policies in the API #22384

Are you sure you want to change the base?

Implement permission policies in the API #22384

Conversation

rijkvanzanten commented May 3, 2024 • edited by hanneskuettner

Scope

Potential Risks / Drawbacks

Review Notes / Questions

Todos

Footnotes

Choose a reason for hiding this comment

DanielBiegler Jun 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rijkvanzanten commented May 3, 2024 •

edited by hanneskuettner

DanielBiegler Jun 6, 2024 •

edited