-
-
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>
// const policies = await fetchPolicies(this.accessService, this.accountability); | ||
// const permissions = await fetchPermissions(this.permissionsService, 'read', policies, collection ? [collection] : undefined); | ||
|
||
// TODO replace with applyCases (TBD) | ||
// applyFilter(this.knex, this.schema, dbQuery, permissions, collection, {}); |
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.
When we bring this back, we need to make sure to include && this.accountability.admin !== true
in the if above, otherwise all fields will be filtered out.
// TODO rely on applyCases | ||
// const permissions = permissionsRecord.permissions ?? {}; | ||
|
||
// if (Object.keys(filter).length > 0) { | ||
// filter = { _and: [permissions, filter] }; | ||
// } else { | ||
// filter = permissions; | ||
// } |
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.
When we bring this back, we need to make sure to include && this.accountability.admin !== true
in the if above, otherwise all fields will be filtered out.
…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
…e might missbehave with the permissions obtained from PermissionsService.readByQuery so it better to go the source of the problem
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.
…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.
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
changesCloses #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 ↩