-
-
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>
} else { | ||
paths.add( | ||
// Ignore all operators and logical grouping in the field paths | ||
path.filter((part) => part.startsWith('_') === 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.
This should probably explicitly test for all known filter operators since otherwise valid field names that start with an underscore are ignored. (See my added, failing, test case).
* All nested paths used in the current query scope. | ||
* This is generated by flattening the filters and adding in the used sort/aggregate fields. | ||
*/ | ||
const paths: Set<FieldKey[]> = new Set(); |
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.
Set<FieldKey[]>
does not guarantee uniqueness of the path arrays, as ['author'] != ['author']
. So we can switch this over to a list, but need to enforce uniqueness with uniqWith(paths, isEqual)
.
]); | ||
}); | ||
|
||
test('Keeps policies that match the IP cidr block', () => { |
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.
We're effectively only testing if the ipInNetworks
function works, which is already covered in the test suite of the util function. So maybe mock it out and just check that is called correctly?
const key = namespace + '-' + getSimpleHash(JSON.stringify(args[0])); | ||
const cached = await cache.get(key); | ||
|
||
if (cached) { |
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.
This right now only works for functions that return a non falsy value. If a cached function returns null
, 0
, ''
, false
, etc. its value will not be returned from cache.
Should this be changed to
if (cached) { | |
if (cached !== undefined) { |
@@ -74,23 +76,20 @@ export class AuthenticationService { | |||
|
|||
const user = await this.knex |
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.
Just as a note, since the user does not have the admin_access
and app_access
flags anymore, we need to look in to the auth providers and verify that they did not need that info, as the user object is passed down to their login
method.
ast = await authorizationService.processAST(ast, opts?.permissionsAction); | ||
} | ||
ast = await processAst( | ||
{ ast, action: 'read', accountability: this.accountability }, |
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.
Did we drop support for QueryOptions.permissionsAction
or is this accounted for somewhere else, that I'm missing?
if (permissions.fields.includes('*') === false) { | ||
const allowedFields = permissions.fields; | ||
if (allowedFields.includes(field) === false) throw new ForbiddenError(); | ||
if (allowedFields.includes(field) === 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.
This is missing the check for '*'
. Was that done on purpose? As far as I can see fetchAllowedFields
does not expand '*'
to all available fields of a collection, so explicitly checking for '*'
seems necessary?
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.
Does the policy
fk in directus_permissions
need an ON DELETE CASCADE
trigger? Otherwise deleting policies does not 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.
Same goes for all fks in directus_access
I guess?
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
endpointCloses #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 ↩