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

Apply workaround for update permission check for rules with relational fields #19728

Merged
merged 4 commits into from
Sep 19, 2023

Conversation

paescuj
Copy link
Member

@paescuj paescuj commented Sep 18, 2023

Addresses #19327

Temporary workaround, currently necessary for permission rules containing relational fields because those relational values aren't available in the isAllowed update check and it would therefore always fail.
For now, the check is instead always considered successful in such a case, ensuring items can be update again.
Though, this means items might be presented to the user as "updatable" even if they aren't, but the API will correctly refuse the update in that case and the You don't have permission to access this message will be shown.

A complete solution will be provided at a later date.

@changeset-bot
Copy link

changeset-bot bot commented Sep 18, 2023

🦋 Changeset detected

Latest commit: c4b79ba

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages
Name Type
@directus/app Patch
@directus/utils Patch
@directus/api Patch
@directus/composables Patch
@directus/extensions-sdk Patch
@directus/pressure Patch
@directus/storage-driver-azure Patch
@directus/storage-driver-cloudinary Patch
@directus/storage-driver-gcs Patch
@directus/storage-driver-local Patch
@directus/storage-driver-s3 Patch
@directus/storage-driver-supabase Patch
@directus/validation Patch
directus Patch
create-directus-extension Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@paescuj paescuj marked this pull request as ready for review September 18, 2023 19:29
@paescuj paescuj requested review from a team, rijkvanzanten, br41nslug and DanielBiegler and removed request for a team September 18, 2023 19:33
Copy link
Contributor

@azrikahar azrikahar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this is a workaround, it works and still technically somewhat reverts to the old isAllowed behavior. Although isAllowed used to work for cases such as #19327, it actually did not work properly hence the incorrect impression of it working.

For additional context, the isAllowed util before #18871 was technically not checking the perms correctly, hence #16731 was an issue where a uneditable field was shown as editable.

However the issue now is when we do check perms recursively in the App, the current value that it is checking against is not as detailed as what the API uses (as API fetches nested values). If we were to go down the route of fetching the nested values retroactively on the App side, we may be re-doing the API's logic over in the App. 🤔

A better fix would potentially be that permissions are checked solely in the API rather than the App, which will ideally resolve #11059 as well.

@paescuj
Copy link
Member Author

paescuj commented Sep 19, 2023

Although this is a workaround, it works and still technically somewhat reverts to the old isAllowed behavior. Although isAllowed used to work for cases such as #19327, it actually did not work properly hence the incorrect impression of it working.

For additional context, the isAllowed util before #18871 was technically not checking the perms correctly, hence #16731 was an issue where a uneditable field was shown as editable.

However the issue now is when we do check perms recursively in the App, the current value that it is checking against is not as detailed as what the API uses (as API fetches nested values). If we were to go down the route of fetching the nested values retroactively on the App side, we may be re-doing the API's logic over in the App. 🤔

A better fix would potentially be that permissions are checked solely in the API rather than the App, which will ideally resolve #11059 as well.

Great summary 😇👍

@rijkvanzanten rijkvanzanten merged commit c413788 into main Sep 19, 2023
7 checks passed
@rijkvanzanten rijkvanzanten deleted the workaround-app-update-permission-check branch September 19, 2023 15:30
@github-actions github-actions bot added this to the Next Release milestone Sep 19, 2023
br-rafaelbarros pushed a commit to personal-forks/directus-source that referenced this pull request Nov 7, 2023
…l fields (directus#19728)

* Apply workaround for update permission check for rules with relational fields

* Add basic update tests for isAllowed

* Enhance parseFilter tests

* Add changeset
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants