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

BUGFIX: Ensure cache flushing for asset changes works with foreign workspaces #5415

Conversation

mficzel
Copy link
Member

@mficzel mficzel commented Dec 25, 2024

If metadata of assets that are already used in workspaces the current user has no access to is changed the AssetChangeHandlerForCacheFlushing yields an exception Read access denied for workspace ....

This is fixed by flushing the caches inside inside a securityContext->withoutAuthorizationChecks closure.

Resolves: #5414

Upgrade instructions

Review instructions

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

…rkspaces

This is done by flushing the caches inside inside a securityContext->withoutAuthorizationChecks closure.

Resolves: neos#5414
@mficzel
Copy link
Member Author

mficzel commented Dec 25, 2024

While this solves the problem from #5414 i am not sure wether and how to test this.

@mficzel mficzel marked this pull request as ready for review December 25, 2024 14:26
Copy link
Contributor

@dlubitz dlubitz left a comment

Choose a reason for hiding this comment

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

Thank you. Works as expected.

@dlubitz
Copy link
Contributor

dlubitz commented Dec 29, 2024

AFAIS the ContentRepositorySecurityTrait provides some helper for testing this. I gueess we need to extend some AssetUsage tests with an authenticated user and enabled contentrepository security.

See example:

When content repository security is enabled
And the shared workspace "shared-workspace" is created with the target workspace "live" and role assignments:
| Role | Type | Value |
| COLLABORATOR | GROUP | Neos.Neos:AbstractEditor |

Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

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

jup using the withoutAuthorizationChecks is the way for now ... if this would have been triggered by a catchup hook we could use the ContentRepositoryReadModel directly (which ignored security) which is here not the case ... but just for reference: #5328


still if the time allows i think a test would be neat ^^ we seem to have steps like Then the asset "an-asset-to-change" has the title "First changed asset" already in place, but its not that critical either i guess?

Copy link
Member

@nezaniel nezaniel left a comment

Choose a reason for hiding this comment

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

lgtm

@nezaniel nezaniel merged commit f20b28c into neos:9.0 Jan 7, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

BUG: Flushing of asset tags after changing media metadata causes "Read access denied for workspace" errors
4 participants