-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement permission policies in the API #22384
base: auditus
Are you sure you want to change the base?
Conversation
Co-authored-by: Daniel Biegler <DanielBiegler@users.noreply.github.com>
Co-authored-by: Daniel Biegler <DanielBiegler@users.noreply.github.com>
router.search('/', validateBatch('read'), readHandler, respond); | ||
|
||
router.get( | ||
'/me/flags', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would just /me
work? 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give an quick example of how the current response looks like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
throw new ForbiddenError({ | ||
reason: `You don't have permission to access collection "${collection}" or it does not exist. Queried in ${pathSuffix}.`, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
…us-21765 # Conflicts: # api/src/services/users.ts # api/src/telemetry/lib/get-report.test.ts # api/src/telemetry/lib/get-report.ts # api/src/telemetry/utils/get-user-count.test.ts # api/src/telemetry/utils/get-user-count.ts
# Conflicts: # api/src/services/users.ts
vi.mock('./middleware/check-ip', () => ({ | ||
checkIP: Router(), | ||
})); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mocking for ./middleware/get-permissions
below should be removed too
id: roleID, | ||
...defaultAdminRole, | ||
}); | ||
await db('directus_roles').insert({ ...defaultAdminRole, id: roleID }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new policy should be created
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 }); |
router.search('/', validateBatch('read'), readHandler, respond); | ||
|
||
router.get( | ||
'/me/flags', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would just /me
work? 👀
api/src/controllers/roles.ts
Outdated
const query = { ...req.sanitizedQuery, limit: -1 }; | ||
|
||
const roles = await service.readMany(req.accountability.roles, query); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add the same comment as per other usages of this limit: -1
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); |
if (this.accountability?.admin) { | ||
return mapValues(this.schema.collections, () => | ||
Object.fromEntries( | ||
['create', 'read', 'update', 'delete', 'share'].map((action) => [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we extract this array as a constant? This array will be used in the App as well.
const query = { ...req.sanitizedQuery, limit: -1 }; | ||
|
||
try { | ||
const roles = await service.readMany(req.accountability.roles, query); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we include this comment about it being temporary to be consistent with other usages?
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 <4376726+hanneskuettner@users.noreply.github.com>
api/src/database/run/run.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this file be run-ast.ts
?
); | ||
|
||
for (const [path, { collection, fields }] of fieldMap.entries()) { | ||
validatePath(path, permissions, collection, fields); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validatePath
errors when querying with the default fields=*
when there is no permission for the related collection in relational fields.
The previous behaviour "ignored" the relational field without permission, by not including it within the AST.
Previous logic that is now in parse-fields.ts
with the permission check removed:
directus/api/src/utils/get-ast-from-query.ts
Lines 246 to 249 in 9e91405
} else if (relatedCollection) { | |
if (permissions && permissions.some((permission) => permission.collection === relatedCollection) === false) { | |
continue; | |
} |
Scope
What's changed:
api/src/permissions
folderroles
flag to accountability object. This is an ordered array of all the parent roles of the current userget-ast-from-query
by splitting it into multiple filescases
andwhenCase
. This allows us to dynamically generate the case/when SQL to have dynamic field output per item.run-ast
by splitting it up into smaller filesPotential Risks / Drawbacks
Review Notes / Questions
Todos
whenCases
inrun-ast
clear
method in memory/cache/permissions
endpointdirectus_access
changesdirectus_roles
changesdirectus_permissions
changesdirectus_policies
changesdown
migrationwithCache
to the known keysapp_access
andadmin_access
false
Closes #21778, closes #21765, closes #22163, closes #21769, closes #21768, closes #21767, closes #21766
Footnotes
Eg check to make sure there's still >=1 admin left after the mutation is done ↩