-
-
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>
policies dont [yet] have an icon
new default keys would override the manually set keys, potentially leading to unintended behavior
const flatQuery = knex.from(table); | ||
const query = await applyQuery(knex, table, flatQuery, queryCopy, schema, cases).query; | ||
return query.select(fieldNodes.map(preProcess)); |
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.
Any reason why the query building was changed from
const flatQuery = knex.select(fieldNodes.map(preProcess)).from(table);
Does the order where using to build the query matter in any way?
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.
Order shouldn't matter. In this case it has to go through the applyQuery
function; that's the change. Don't remember if there was a reason to feed it through applyQuery
without the field selection yet..
): Knex.Raw { | ||
const caseQuery = knex.queryBuilder(); | ||
|
||
applyFilter(knex, schema, caseQuery, { _or: columnCases }, table, aliasMap, cases); |
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 to verify, this will reuse existing aliases generated during the first applyFilter
through the aliasMap
and not result in additional, probably expensive, joins being introduced?
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.
That's the thinking, but we have to double check that 👍🏻
if (!start) return []; | ||
|
||
let parent: string | null = start; | ||
const roles: string[] = []; |
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.
For performance this should probably be a set. Which does preserve insertion order1, so roles.reverse()
becomes const roleList [...roles].reversed()
.
Now we have the overhead of creating that array, but it still cuts down the includes
/has
down to O(1)
.
Footnotes
api/src/permissions/modules/fetch-allowed-fields/fetch-allowed-fields.test.ts
Outdated
Show resolved
Hide resolved
export async function fetchGlobalAccessForRoles(knex: Knex, roles: string[]): Promise<GlobalAccess> { | ||
const query = knex.where('role', 'in', roles); | ||
return await fetchGlobalAccessForQuery(query, `global-access-roles-${getSimpleHash(JSON.stringify(roles))}`); | ||
} |
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.
fetchGlobalAccessForQuery
should probably be refactored to use withCache
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.
All these cases should be covered in the individual tests for extractFieldsFromChildren
and extractFieldsFromQuery
and we should just make sure that both of them are called with the same fieldMap
and the correct parameters?
for (const [index, { rule, fields }] of rules.entries()) { | ||
// If none of the fields in the current permissions rule overlap with the actually requested | ||
// fields in the AST, we can ignore this case altogether | ||
if (fields.has('*') === false && Array.from(fields).every((field) => requestedKeys.includes(field) === false)) { | ||
continue; | ||
} | ||
|
||
cases.push(rule); | ||
|
||
for (const field of fields) { | ||
caseMap[field] = [...(caseMap[field] ?? []), index]; | ||
} | ||
} |
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.
What does it mean if rule
is null
here? As far as I can tell it means that this permission does not have a permission filter, so it matches the globalWhenCase
filter? So effectively another question is what is the difference between Permission.permissions == null
and == {}
.
Right now processChildren
returns (Filter|null)[]
, which is an invalid type for AST.cases
.
So we should only add it to cases, if rule != null
and use an independently incrementing index instead of the loop index?
for (const [index, { rule, fields }] of rules.entries()) { | |
// If none of the fields in the current permissions rule overlap with the actually requested | |
// fields in the AST, we can ignore this case altogether | |
if (fields.has('*') === false && Array.from(fields).every((field) => requestedKeys.includes(field) === false)) { | |
continue; | |
} | |
cases.push(rule); | |
for (const field of fields) { | |
caseMap[field] = [...(caseMap[field] ?? []), index]; | |
} | |
} | |
let index = 0; | |
for (const { rule, fields } of rules) { | |
// If none of the fields in the current permissions rule overlap with the actually requested | |
// fields in the AST, we can ignore this case altogether | |
if (fields.has('*') === false && Array.from(fields).every((field) => requestedKeys.includes(field) === false)) { | |
continue; | |
} | |
if (rule === null) continue; | |
cases.push(rule); | |
for (const field of fields) { | |
caseMap[field] = [...(caseMap[field] ?? []), index]; | |
} | |
index++; | |
} |
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.
So effectively another question is what is the difference between Permission.permissions == null and == {}.
There's no difference between these two. null
and {}
should be treated the same in the case of the permission rules
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.
So we should only add it to cases, if rule != null and use an independently incrementing index instead of the loop index?
This is a good idea, as we can ignore any case that's null or {}
anyways
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.
You wanna do the honors, or should I?
api/src/permissions/modules/fetch-global-access/utils/fetch-global-access-for-query.ts
Show resolved
Hide resolved
admin: false, | ||
app: false, | ||
roles: [], | ||
role: null, |
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 return null
for role
, the invalid role '456-789'
or throw an error as it did before ?
a8009f7
to
cd032c2
Compare
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 ↩