Skip to content
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

Draft
wants to merge 211 commits into
base: auditus
Choose a base branch
from
Draft

Conversation

rijkvanzanten
Copy link
Member

@rijkvanzanten rijkvanzanten commented May 3, 2024

Scope

What's changed:

  • Implements policy-system based permissions handling on the API
  • Replaces AuthorizationService with new set of functions in the api/src/permissions folder
  • Allows roles to be nested
  • Adds new roles flag to accountability object. This is an ordered array of all the parent roles of the current user
  • Cleans up get-ast-from-query by splitting it into multiple files
  • Permissions are now injected into the AST through cases and whenCase. This allows us to dynamically generate the case/when SQL to have dynamic field output per item.
  • Cleans up run-ast by splitting it up into smaller files

Potential Risks / Drawbacks

  • The risks are very high. This replaces the full permissions system, and thus needs a lot of testing

Review Notes / Questions

  • This PR now compiles, but doesn't run yet.
  • There was some weird logic happening in the users controller for TFA enable/disable that I'm not sure we need to keep. Needs a bit more testing

Todos

  • Add permissions processing for $CURRENT_USER etc flags
    • Introduce $CURRENT_POLICIES and $CURRENT_ROLES for permissions
    • Decide if $CURRENT_POLICIES and $CURRENT_ROLES should be available in presets as well
  • Add permissions merging when you're accessing from a share
  • Add caching to:
    • Fetching Policies
    • Fetching permissions
    • Fetching the roles tree
    • Fetching the field map
    • Fetching allowed fields
    • Fetching allowed collections
  • Enable validation for admin users for wrong paths
  • Figure out what we want to do with Presets
  • Use whenCases in run-ast
  • Use applyCases in Meta Service for permissions aware counts
  • Handle admin-checks in roles and users services1
  • Add unit testing for clear method in memory/cache
  • Make sure graphql gracefully handles optional fields when you have different fields for the same collection in multiple permission sets
  • Add changeset
  • Unbork /permissions endpoint
  • Invalidate cache on permission changes
    • On directus_access changes
    • On directus_roles changes
    • On directus_permissions changes
    • On directus_policies changes
  • Make public access work again, by introducing a real, public role
    • Prevent public role to be used as parent, and the public role to have any parent
  • Before merging this one in main, either retarget Add roles and permissions to the app #22654 to main, or merge it in here

Closes #21778, closes #21765, closes #22163, closes #21769, closes #21768, closes #21767, closes #21766

Footnotes

  1. Eg check to make sure there's still >=1 admin left after the mutation is done

Comment on lines +54 to +58
// 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, {});
Copy link
Contributor

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.

Comment on lines +85 to +92
// TODO rely on applyCases
// const permissions = permissionsRecord.permissions ?? {};

// if (Object.keys(filter).length > 0) {
// filter = { _and: [permissions, filter] };
// } else {
// filter = permissions;
// }
Copy link
Contributor

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.

@directus directus deleted a comment from 2chiefk Jun 3, 2024
router.search('/', validateBatch('read'), readHandler, respond);

router.get(
'/me/flags',
Copy link
Contributor

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.

Copy link
Contributor

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.

Comment on lines +12 to +14
throw new ForbiddenError({
reason: `You don't have permission to access collection "${collection}" or it does not exist. Queried in ${pathSuffix}.`,
});
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

None yet

6 participants