Implement permission policies in the API #22384
Conversation
Co-authored-by: Daniel Biegler <DanielBiegler@users.noreply.github.com>
Co-authored-by: Daniel Biegler <DanielBiegler@users.noreply.github.com>
rijkvanzanten
force-pushed
the
auditus-21765
branch
from
a8009f7
to
cd032c2
Compare
| } 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.
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.
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.
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.
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.
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.
Did we drop support for QueryOptions.permissionsAction or is this accounted for somewhere else, that I'm missing?
api/src/services/meta.ts
Outdated
There was a problem hiding this comment.
If we're planning to drop support for meta anyways (see #15665) I would suggest not investing any time in how to make this work and consider it a good point to drop it now rather than later? Maybe in a follow-up PR?
| 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.
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?
Scope
What's changed:
api/src/permissionsfolderrolesflag to accountability object. This is an ordered array of all the parent roles of the current userget-ast-from-queryby splitting it into multiple filescasesandwhenCase. This allows us to dynamically generate the case/when SQL to have dynamic field output per item.run-astby splitting it up into smaller filesPotential Risks / Drawbacks
Review Notes / Questions
Todos
whenCasesinrun-astclearmethod in memory/cache/permissionsendpointCloses #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 ↩