Contrib.social

MediaAccessControlHandler update/delete access caching is not correct

Open on Drupal.org →
Created on 11 September 2018, over 6 years ago
Updated 10 September 2024, 5 months ago

Problem/Motivation

While working on #2889855: Unpublished media entity can not be accessed by owner and update any media/delete any media access possibly cached by user → we've noticed a couple of bugs in MediaAccessControlHandler regarding the cache for delete/update access.

  • 'edit own type media' permissions is checked and returned before 'update any media' permission, which causes cache per user when it's not needed
  • 'delete own type media' permissions is checked and returned before 'delete any media' permission, which causes cache per user when it's not needed
  • 'edit any type media' does not have the entity as cacheable dependency
  • 'delete any type media' does not have the entity as cacheable dependency

We also have issues when returning a neutral result - we need to be caching per user. We can't just cache per permissions when we return a neutral response as this would cache incorrectly for the use case where a user is denied access, then another user is meant to be granted access based on the fact they are the owner but they have identical permissions (see #9)

1. Create a roll with 'edit own $type media' and 'update any media' permission
2. Create a user and assign the role created in step 1.
3. Create a media item of $type
4. Login as the user from step 2
5. Try to edit the media item of step 3

Expected:
The access result is cached per permission.

Actual:
The access result is cached per user.

1. Create a roll with 'delete own $type media' and 'delete any media' permission
2. Create a user and assign the role created in step 1.
3. Create a media item of $type
4. Login as the user from step 2
5. Try to delete the media item of step 3

Expected:
The access result is cached per permission.

Actual:
The access result is cached per user.

1. Create a roll with 'delete own $type media'
2. Create 2 users and assign the role created in step 1.
3. Create a media item of $type and make user A the owner
4. Login as the user B from step 2
5. Try to delete the media item of step 3 and get access denied
6. Login as user A from step 2
7. Try to delete media item

Expected:
Access is granted.

Actual:
Access is not granted because previous result was cached only by permissions.

Proposed resolution

Fix update/delete access cache issues and add extensive tests for it.

  • Make sure the generic 'update any media' / 'edit any $type media' permissions are checked before the user specific 'update own media' / 'edit own $type media'.
  • Make sure the generic 'delete any media' / 'delete any $type media' permissions are checked before the user specific 'delete own media' / 'delete own $type media'.

Remaining tasks

User interface changes

API changes

Data model changes

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
Media  →

Last updated about 2 hours ago

Created by

🇳🇱Netherlands seanB Netherlands

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • @acbramley opened merge request.
  • Comment almost 2 years ago →
    🇦🇺Australia acbramley

    Rebased onto an MR and attempted to fix the MediaTest::testRevisions failure. I got us past the cache context failure but now since the response is uncacheable due to the user context that bit fails... these tests extending ResourceTestBase are really tough to work with....

  • First commit to issue fork.
  • Merge request !5697[11.x] Issue #2998824: MediaAccessControlHandler update/delete access caching is not correct → (Open) created by mstrelan
  • Comment about 1 year ago →
    🇦🇺Australia mstrelan

    Re-rolled MR for 11.x, we should probably close the old one and hide patches.

    That data provider is massive, and even more so since ✨ Add Media revision UI Fixed .

  • Pipeline finished with Failed
    about 1 year ago
    #59715
  • Comment about 1 year ago →
    🇦🇺Australia acbramley

    acbramley → changed the visibility of the branch 2998824-mediaaccesscontrolhandler-updatedelete-access to hidden.

  • Comment about 1 year ago →
    🇦🇺Australia acbramley
  • Pipeline finished with Failed
    about 1 year ago
    #59720
  • Pipeline finished with Failed
    about 1 year ago
    #59728
  • Pipeline finished with Failed
    about 1 year ago
    #59734
  • Pipeline finished with Failed
    12 months ago
    #82172
  • Pipeline finished with Running
    12 months ago
    #82181
  • Comment 12 months ago →
    🇦🇺Australia acbramley

    Rebased against 11.x and attempted yet again to fix the cache context issues.

    We have even more issues now that we have the checkFieldAccess in EntityAccessControlerHandler checking the update operation which can randomly add the user context in too. I honestly can't see how these tests are fixable without a massive refactor to ResourceTestBase.

  • Comment 12 months ago →
    🇦🇺Australia acbramley

    acbramley → changed the visibility of the branch 11.x to hidden.

  • Pipeline finished with Failed
    5 months ago
    Total: 163s
    #274106
  • Pipeline finished with Failed
    5 months ago
    Total: 485s
    #274110
  • Pipeline finished with Failed
    5 months ago
    Total: 396s
    #274124
  • Pipeline finished with Failed
    5 months ago
    Total: 167s
    #275190
  • Pipeline finished with Failed
    5 months ago
    Total: 579s
    #275191
  • First commit to issue fork.
  • Pipeline finished with Canceled
    5 months ago
    Total: 67s
    #279185
  • Pipeline finished with Failed
    5 months ago
    #279187
  • Comment 5 months ago →
    🇭🇺Hungary mxr576 Hungary

    Copy-pasting from Drupal Slack:

    mxr576
    Today at 12:49 PM
    @acbramley
    Maybe you will find some inspiration in my MR for fixing these test failures. testRevisions() killed my nerves so many times...
    https://git.drupalcode.org/project/drupal/-/merge_requests/8198

    mxr576
    Just now
    Pushed those changes from the other MR to yours, you can decide if you keep those or not. If we need these here as well either my PR should be merged first or these changes on ResourceTestBase has to be merged independently.
    Test went far further now (previously it failed on core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:2930), but it is still going to fail on something that should be work...
    I also had to commit two changes in a dedicated commit over the ported changes and I would like to understand why those were necessary.
    Generally speaking, the changes on core/modules/media/src/MediaAccessControlHandler.php because that makes many things uncacheable by adding the user context.
    The ported changes on ResourceTestBase only warrants that assertions are aligned with expectations and not hardcoded on the header value.

  • Pipeline finished with Failed
    5 months ago
    Total: 446s
    #279196
  • Pipeline finished with Failed
    5 months ago
    Total: 447s
    #279257
  • Comment 5 months ago →
    🇭🇺Hungary mxr576 Hungary
  • Comment 5 months ago →
    🇭🇺Hungary mxr576 Hungary

    Clean up GIT history in the MR, now we just need to figure out why the remaining tests fail...

  • Pipeline finished with Failed
    5 months ago
    Total: 576s
    #279566
contrib.social Blog FAQ Discussions
Production build 0.71.5 2024