Unpublished media entity can not be accessed by owner and update any media/delete any media access possibly cached by user

This test fails before the patch and passes after it.

Can we have a fail (test-only) patch, just to prove that this fix works? Once that's here, this is squarely RTBC from me.

Sure!

Yay! RTBC for #5.

Cacheability metadata is now incorrect.

Looks good to me.

+++ b/core/modules/media/src/MediaAccessControlHandler.php
@@ -25,6 +25,7 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
         return AccessResult::allowedIf($account->hasPermission('view media') && ($is_owner || $entity->isPublished()))
           ->cachePerPermissions()
+          ->cachePerUser()
           ->addCacheableDependency($entity);

+++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
@@ -58,11 +58,11 @@ public function testMediaAccess() {
-    $this->assertCacheContext('user.permissions');
+    $this->assertCacheContext('user');
...
-    $this->assertCacheContext('user.permissions');
+    $this->assertCacheContext('user');

As the tests show, this results in worse cacheability for 100% of cases.

You can improve this by doing:

return AccessResult::allowedIf($account->hasPermission('view media') && $entity->isPublished())
  ->cachePerPermissions()
  ->addCacheableDependency($entity)
  ->orIf(AccessResult::allowedIf($account->hasPermission('view media') && $is_owner)
      ->cachePerUser()
      ->addCacheableDependency($entity)
  );

Now it'll vary by user.permissions (better cacheability) unless the entity is unpublished, then it'll vary by user.

Also, MediaAccessControlHandler is missing unit test coverage. See \Drupal\Tests\field\Unit\FieldStorageConfigAccessControlHandlerTest for an example. Doesn't necessarily need to happen here, but it'll make things easier going forward for sure.

Used #14.

Now it'll vary by user.permissions (better cacheability) unless the entity is unpublished, then it'll vary by user.

Actually, that's not true. See \Drupal\Tests\Core\Access\AccessResultTest::testOrIfCacheabilityMerging(). Perhaps we should change the behavior of orIf(), but that'd be out of scope here.

Fix attached.

Tagging Needs tests for

Also, MediaAccessControlHandler is missing unit test coverage. See \Drupal\Tests\field\Unit\FieldStorageConfigAccessControlHandlerTest for an example. Doesn't necessarily need to happen here, but it'll make things easier going forward for sure.

Great, now MediaAccessTest is failing, not MediaCacheTagsTest!

1. +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   @@ -58,11 +58,11 @@ public function testMediaAccess() {
   -    $this->assertCacheContext('user.permissions');
   +    $this->assertCacheContext('user');
        $assert_session->statusCodeEquals(403);
        $this->grantPermissions($role, ['view media']);
        $this->drupalGet('media/' . $media->id());
   -    $this->assertCacheContext('user.permissions');
   +    $this->assertCacheContext('user');
   

   These should be reverted.

2. +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   @@ -89,6 +89,11 @@ public function testMediaAccess() {
   +    // Verify that the author can always view the media item even if it is
   +    // unpublished.
   +    $user_media->setPublished(FALSE)->save();
   +    $this->drupalGet('media/' . $user_media->id());
   +    $assert_session->statusCodeEquals(200);
   

   And *this* should get $this->assertCacheContext('user').

Here is the updated patch and interdiff.txt

All tests should be fixed now

I added a dedicated MediaAccessControlHandlerTest.

Wanted to RTBC this, but can't just yet — it's getting very close now though:

1. +++ b/core/modules/media/src/MediaAccessControlHandler.php
   @@ -23,9 +23,18 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +          $ownerAccess = AccessResult::allowedIf($account->hasPermission('view media') && $is_owner)
   +            ->cachePerUser()
   +            ->addCacheableDependency($entity);
   +          if ($ownerAccess->isAllowed()) {
   +            return $ownerAccess;
   +          }
   

   This is wrong: it's losing the user.permissions cache context. This was changed in #22.

   (Also, FYI: in Drupal core, we never use camelCase in local variables, only for class properties. I think you and I are both bored of pointing this out and fixing it :))

2. +++ b/core/modules/media/tests/src/Unit/MediaAccessControlHandlerTest.php
   @@ -0,0 +1,191 @@
   +  public function assertAllowOperations(array $allow_operations, AccountInterface $user) {
   +    foreach (['view', 'update', 'delete'] as $operation) {
   +      $expected = in_array($operation, $allow_operations);
   +      $actual = $this->accessControlHandler->access($this->media, $operation, $user);
   +      $this->assertSame($expected, $actual, "Access problem with '$operation' operation.");
   +      $this->accessControlHandler->resetCache();
   +    }
   +  }
   

   This is not asserting cacheability. If it were, it'd help with sorting out the other thing I mentioned above :)

EDIT: to help your understanding, run this:

  var_dump(\Drupal\Core\Access\AccessResult::allowedIf(TRUE)->cachePerPermissions()->orIf(\Drupal\Core\Access\AccessResult::allowedIf(TRUE)->cachePerUser()));
  exit;

1. So you mean i have to add cachePerPermissions() to $ownerAccess as well? I had to change it like this, because if the first AccessResult is already forbidden, it's not possible to get it positive with a orIf join.

Addressed #25. Added a bunch of testcases for different combinations of permissions, ownership and published state. Also added asserts for the cache tags.

Please review!

1. +++ b/core/modules/media/tests/src/Unit/MediaAccessControlHandlerTest.php
   @@ -0,0 +1,285 @@
   +class MediaAccessControlHandlerTest extends UnitTestCase {
   

   This test is doing a lot of universe-mocking. Maybe it would make more sense as a kernel test?

2. +++ b/core/modules/media/tests/src/Unit/MediaAccessControlHandlerTest.php
   @@ -0,0 +1,285 @@
   +    $this->anon
   +      ->expects($this->any())
   +      ->method('hasPermission')
   +      ->will($this->returnValue(FALSE));
   +    $this->anon
   +      ->expects($this->any())
   +      ->method('id')
   +      ->will($this->returnValue(0));
   +    $this->anon
   +      ->expects($this->any())
   +      ->method('getDisplayName')
   +      ->will($this->returnValue('Anonymous'));
   

   This is a nit for readability, but all of these can be rewritten as such (obviously no need to do this if we change it to a kernel test):

   $this->anon->method('hasPermission)->willReturn(...)

#30.1: that may very well be better indeed — see \Drupal\Tests\system\Kernel\MenuAccessControlHandlerTest and \Drupal\Tests\system\Kernel\DateFormatAccessControlHandlerTest. Note that over there, we also can test cacheability metadata easily!

Here is a new patch, based on KernelTestBase

This needed a re-roll because of #2905748: MediaAccessControlHandler does not provide a helpful reason for when access is denied for the view operation

This looks good. However, I found the test rather difficult to understand (especially the $which_user/$which_entity stuff), so I refactored it a bit...

Here is some review comments:

1. +++ b/core/modules/media/src/MediaAccessControlHandler.php
   @@ -27,6 +27,15 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +        if (!$access_result->isAllowed()) {
   

   Do we still need this additional condition? could be moved inside above if?

2. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
   @@ -0,0 +1,121 @@
   +    // We must always create user 1, so that a "normal" user has a ID >1.
   +    $this->createUser();
   

   Really? MediaAccessControlHandlerTest(parent::setup()) already creates user and assign to ->user.

3. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
   @@ -0,0 +1,121 @@
   +  public function testAccess(array $permissions, array $entity_values, AccessResult $view_access_result, AccessResult $update_access_result, AccessResult $delete_access_result, AccessResult $create_access_result) {
   

   Can we add params in docblock?

4. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
   @@ -0,0 +1,121 @@
   +  public function testAccessProvider() {
   

   Missing docblock.

5. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
   @@ -0,0 +1,121 @@
   +    $c = new ContainerBuilder();
   

   Can we have better variable name?

6. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
   @@ -0,0 +1,121 @@
   +    $cache_contexts_manager = $this->prophesize(CacheContextsManager::class);
   +    $cache_contexts_manager->assertValidTokens()->willReturn(TRUE);
   +    $c->set('cache_contexts_manager', $cache_contexts_manager);
   +    \Drupal::setContainer($c);
   

   Whole container setup can be moved to ::setup().

Functionally tested by following the steps in IS and fix looks working as expected.

Addressing #37:

1. This was already discussed in #25, so leaving as is after consulting with @seanB.
2. Nice catch. Fixed.
3. Fixed.
4. Fixed.
5. No longer relevant :)
6. I had to substantially refactor the test, but I was able to remove this entirely. This was a hack to get around the fact that, in the data provider, the container is not yet initialized -- yet AccessResult's caching methods call container services. I changed all of this so that the mocking is no longer necessary. The test just uses the real cache context manager, as it should be in a kernel test.

+++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
@@ -51,43 +60,110 @@
+  protected function assertAccess(array $expected, AccessResult $actual) {

cool method. now we are missing docblock for this method :)

Werd to that.

Thanks @phenaproxima. Looks good to go!

+++ b/core/modules/media/src/MediaAccessControlHandler.php
@@ -27,6 +27,13 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
           ->addCacheableDependency($entity);
         if (!$access_result->isAllowed()) {
+          $owner_access = AccessResult::allowedIf($account->hasPermission('view media') && $is_owner)
+            ->cachePerPermissions()
+            ->cachePerUser()
+            ->addCacheableDependency($entity);
+          if ($owner_access->isAllowed()) {
+            return $owner_access;
+          }
           $access_result->setReason("The 'view media' permission is required and the media item must be published.");
         }

Does this need a check for $account->id() > 0? For example a user cancels their account, their content gets re-assigned to user/0, now anonymous users could potentially see the previously unpublished and invisible to everyone else content.

Also is not having a specific 'view own unpublished' permissions like nodes do intentional?

Good points.

After quick discussion with @catch, I think we'll need to ensure that anonymous users do NOT have permission to view unpublished media at any time. So let's account for that case, and add a test to ensure it.

My feeling is that we should also add a 'view own unpublished', but I'm open to debate about that.

I think 'view own unpublished' makes sense for media as well. There could be couple of use cases where this would be useful.

Added the permission and updated the AccessHandler to obey new permission with test coverage.

+++ b/core/modules/media/src/MediaAccessControlHandler.php
@@ -23,10 +23,17 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
+        $access_result = AccessResult::allowedIf($account->hasPermission('view media') && ($entity->isPublished() || ($account->hasPermission('view own unpublished media') && $account->id() > 0)))
...
+          $owner_access = AccessResult::allowedIf($account->hasPermission('view media') && $is_owner)

The check for "view own unpublished media" should probably be moved to the same check as for $is_owner. Right now it seems owner can still get access to own unpublished media without the permission.

This also seems to indicate that we should add some more tests for this.

The check for "view own unpublished media" should probably be moved to the same check as for $is_owner. Right now it seems owner can still get access to own unpublished media without the permission.

not sure @seanB. $is_owener checks if $account->id(). so anonymous user (id=0) still won't be able to see, even if own media?

1. +++ b/core/modules/media/src/MediaAccessControlHandler.php
   @@ -23,7 +23,7 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +        $access_result = AccessResult::allowedIf($account->hasPermission('view media') && ($entity->isPublished() || ($account->hasPermission('view own unpublished media') && $account->id() > 0)))
   

   I do find this logic a little confusing and hard to follow. Can it be refactored for clarity? Perhaps we could use the ->orIf() functionality of AccessResult. If not, can we at least add a comment?

2. +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   @@ -116,4 +115,33 @@ public function testMediaAccess() {
   +    // Create media.
   +    $media = Media::create([
   +      'bundle' => $media_type->id(),
   +      'name' => 'Unnamed',
   +    ]);
   +    $media->save();
   

   $media doesn't seem to be used after this.

3. +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   @@ -116,4 +115,33 @@ public function testMediaAccess() {
   +    $role = Role::load(RoleInterface::ANONYMOUS_ID);
   +    $this->grantPermissions($role, ['view media', 'view own unpublished media']);
   

   I don't understand why we're granting permissions to the anonymous role here?

4. Can MediaAccessControlHandlerTest also receive additional test cases for the new scenarios we're testing?

not sure @seanB. $is_owener checks if $account->id(). so anonymous user (id=0) still won't be able to see, even if own media?

Good one! Missed that. I do think "view own unpublished media" should still be linked to the $is_owner check. I think currently the first one checks for the permission, but doesn't actually verify that you are the owner.

#51.1 - Sure, I have updated with orIf().
#51.2 - Good catch! removed it.
#51.3 - This is to validate if we give 'view own unpublished media' to anonymous for some reason, still won't be accessible.
#51.4 - Added few cases.
#52 - ok, I feel $is_owner is purely account specific, not permission. Do you still feel (after #51.1 fix) we need an update on $is_owner?

P.S: just to be clear on uid in Drupal\Tests\media\Kernel\MediaAccessControlHandlerTest::testAccessProvider: passing uid=>0 would make the entity created by anonymous user as !isset($entity_values['uid']) would still be FALSE and skip $entity_values['uid'] = $user->id();.

If I'm not mistaken allowedIf($account->hasPermission('view own unpublished media') && $account->id() > 0) will allow access to a media item when I have the "view own" permission, even when I'm not the owner right?

It should probably be something like this:

 $access_result = AccessResult::allowedIf($account->hasPermission('view media') && $entity->isPublished())
   ->cachePerPermissions()
   ->addCacheableDependency($entity);
 if (!$access_result->isAllowed()) {
   $owner_access = AccessResult::allowedIf($account->hasPermission('view media') && $entity->isPublished() && $is_owner)
     ->orIf(AccessResult::allowedIf($account->hasPermission('view own unpublished media') && $is_owner))
     ->cachePerPermissions()
     ->cachePerUser()
     ->addCacheableDependency($entity);
   if ($owner_access->isAllowed()) {
     return $owner_access;
   }
   $access_result->setReason("The 'view media' permission is required and the media item must be published.");
 }

If I'm not mistaken allowedIf($account->hasPermission('view own unpublished media') && $account->id() > 0) will allow access to a media item when I have the "view own" permission, even when I'm not the owner right?

True, my bad. Updated as per suggestion and tests are green locally:

php core/scripts/run-tests.sh --verbose  --url http://d8/ --class "Drupal\Tests\media\Kernel\MediaAccessControlHandlerTest"

Drupal test run
---------------

Tests to be run:
  - Drupal\Tests\media\Kernel\MediaAccessControlHandlerTest

Test run started:
  Monday, October 2, 2017 - 13:17

Test summary
------------

Drupal\Tests\media\Kernel\MediaAccessControlHandlerTest        9 passes

Test run duration: 17 sec

Detailed test results
---------------------


---- Drupal\Tests\media\Kernel\MediaAccessControlHandlerTest ----


Status    Group      Filename          Line Function
--------------------------------------------------------------------------------
Pass      Other      MediaAccessContro    0 Drupal\Tests\media\Kernel\MediaAcce

Pass      Other      MediaAccessContro    0 Drupal\Tests\media\Kernel\MediaAcce

Pass      Other      MediaAccessContro    0 Drupal\Tests\media\Kernel\MediaAcce

Pass      Other      MediaAccessContro    0 Drupal\Tests\media\Kernel\MediaAcce

Pass      Other      MediaAccessContro    0 Drupal\Tests\media\Kernel\MediaAcce

Pass      Other      MediaAccessContro    0 Drupal\Tests\media\Kernel\MediaAcce

Pass      Other      MediaAccessContro    0 Drupal\Tests\media\Kernel\MediaAcce

Pass      Other      MediaAccessContro    0 Drupal\Tests\media\Kernel\MediaAcce

Pass      Other      MediaAccessContro  103 Drupal\Tests\media\Kernel\MediaAcce

Sorry, I think we are almost there. The tests pass even though I think we still have an issue.

1. +++ b/core/modules/media/src/MediaAccessControlHandler.php
   @@ -27,6 +27,14 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +          $owner_access = AccessResult::allowedIf($account->hasPermission('view media') && $is_owner)
   
   --- a/core/modules/media/tests/src/Functional/MediaAccessTest.php
   +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   

   This is still missing the $entity->isPublished() check. Currently, owners can seem to view unpublished media with only the "view media" permission?

2. +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   @@ -91,6 +90,13 @@ public function testMediaAccess() {
   +    // Verify that the author can always view the media item even if it is
   +    // unpublished.
   

   With the new "view own" permission, I think we no longer want this to be true. The test should be updated accordingly. This one should have failed.

3. +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   @@ -109,4 +115,28 @@ public function testMediaAccess() {
   +  public function testMediaAnonymousUserAccess() {
   

   Could we add an extra assert here to verify the anonymous user is able to view published? So we assert a 200 for published media, unpublish it and assert for 403 directly after it. Just to be sure.

I had to re-roll as #2905213: Bring back "access media overview" permission got in.

Sorry, I think we are almost there.

don't feel sorry, we are improving it every step :)
#56.1 - It feels repeating as the 'view media' and 'published' status are the first checks (which $access_result holds) and I see your point that we don't want to allow owners to see unpublished without 'own unpublished' access. In that case we don't need to do AccessResult::allowedIf($account->hasPermission('view media') && $entity->isPublished() && $is_owner) inside if condition as we know for sure, it would fail?
#56.2 - Updated
#56.3 - Done.

Tested this patch at #1970534-155: Patch testing issue to make sure, it's all green

I agree, the "view media" permission shouldn't do anything special for your own published media. This looks good to me, nice work. Thanks!

+++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
@@ -131,4 +145,36 @@ public function testMediaAccess() {
+
+    // Make sure anonymous users can not access unpublished media
+    // even though role has 'view own unpublished media' permission.
+    $user_media->setUnpublished()->save();
+    $this->drupalGet('media/' . $user_media->id());
+    $assert_session->statusCodeEquals(403);
+    $this->assertNoCacheContext('user');
+  }

This isn't testing the case where the media is owned by user 0 from #44/#45. The recent changes fix bugs that were trying to handle that case, but it seems like that's happened by not preventing it at all. I think it would be good to expand the test coverage first so we know we're covering everything.

This isn't testing the case where the media is owned by user 0 from #44/#45.

Thanks @catch. You are right, that case is covered in MediaAccessControlHandlerTest not in MediaAccessTest. We can certainly extend it.

I think it would be good to expand the test coverage first so we know we're covering everything.

Not sure if I understand this correctly. does it mean test-only patch?

Best I can tell, we're good to go here...

Adding @catch for review credit

1. +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   @@ -92,6 +91,21 @@ public function testMediaAccess() {
   +    $assert_session->statusCodeEquals(403);
   

   Do we need to assert absence/presence of cache context here too.

2. +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   @@ -92,6 +91,21 @@ public function testMediaAccess() {
   +    $this->grantPermissions($role, ['view media', 'view own unpublished media']);
   

   nit: do we need the 'view media' here given we added it in previous hunk?

3. +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   @@ -92,6 +91,21 @@ public function testMediaAccess() {
   +    $user_media->setUnpublished()->save();
   

   We already did this above. By saving here, I think we invalidate based on addCacheableDependency($entity) i.e we're not testing that granting the role/user the permission invalidates the cache. It should (because we use cachePerPermissions but we're not testing that. So I think we should remove this line.

Fixed everything in #64. Als moved the tests for 'view own unpublished media' a bit to the top to make the order of the test cases more logical.

Changes are looking good

+++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
@@ -0,0 +1,326 @@
+    $this->assertAccess($update_access_result, $access_handler->access($entity, 'update', $user, TRUE));
+    $this->assertAccess($delete_access_result, $access_handler->access($entity, 'delete', $user, TRUE));
+    $this->assertAccess($create_access_result, $access_handler->createAccess(NULL, $user, [], TRUE));
...
+    list ($expected, $expected_cache_contexts, $expected_cache_tags) = $expected_access_result;

passing this as an array feels a bit icky. Should we just separate them into separate arguments in the assert and provider?

Should we just separate them into separate arguments in the assert and provider?

We could, but then the test method is going to have, like, a bazillion arguments. We are asserting four access results, so each one would require three arguments (allowed/forbidden, cache tags, and cache contexts), for a total of twelve arguments...plus the other arguments the test method takes. Even for a test method, that's too many arguments :) Collapsing them into arrays isn't optimal, but it ultimately makes for more readable code. It'd be even nicer if PHP had tuples, but...well, I don't want to get rant-y.

There was, unfortunately, no other way to do it whilst continuing to use the data provider pattern (which we aren't going to drop, because it allows us to easily test so many different cases). Trying to create the full AccessResult and apply cache tags to it in the data provider would cause exceptions, because the cache context manager makes various static service calls, and the container is not initialized in the data provider. So this is the workaround we came up with -- the previous workaround was a lot ickier. If you have other ideas, I am open to them, but this is the best way we could collectively come up with.

Trying to test all the operations and create in the same case is the issue I think.

Something like this makes the operation a parameter and splits out create.

So we have more granular test cases, which makes it easier to debug and resolve a fail too.

Doing shows that create is unrelated to the other permissions as there are no entity values, and hence reveal that many of the create tests duplicate each other, as the permissions don't change.

What do you think of this approach instead?

+++ b/core/modules/media/src/MediaAccessControlHandler.php
@@ -27,6 +27,13 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
+            ->cachePerUser()

Don't you need to add this also for the case that $is_owner is false, see #2909138: NodeAccessControlHandler doesn't set current user cache context

Re #69 -- that looks good, and is cleaner. Thanks, @larowlan! I have learned from this. :)

For #70

I rewrote the node test in #2909138: NodeAccessControlHandler doesn't set current user cache context for media and it has thrown an error. So i added the ->cachePerUser() to fix it.
After that, i had to adjust some contexts from other tests.

1. +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   @@ -172,7 +175,80 @@
   +    $build = $view_builder->view($media_parent);
   +    $result = \Drupal::service('renderer')->renderPlain($build);
   

   Since this is a functional test, it might be preferable to visit the media item itself and assert a response code, no?

2. +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   @@ -172,7 +175,80 @@
   +    $this->assertNotContains('div class="field__label">field_reference</div>', (string) $result);
   

   I'm not sure we should be asserting raw markup. I think we could probably use XPath here to assert the presence of the element, rather than specific text.

Thanks @phenaproxima,
i think that makes sense.

So visiting the actual media object and checking for the elements.

The failing tests in #75 let think about the changes we did in #73, because basically all unauthorized requests are now un-cacheable.

Ok, maybe this might help. Acting differently when we are media owner.

Good that #2765959: Make 4xx REST responses cacheable by (Dynamic) Page Cache + comprehensive cacheability test coverage landed in between...

#77 is correct — #2765959: Make 4xx REST responses cacheable by (Dynamic) Page Cache + comprehensive cacheability test coverage landed between #73 and #75, and is pointing out A) a bug that this patch is introducing, B) the lack of test coverage :)

+++ b/core/modules/media/src/MediaAccessControlHandler.php
@@ -27,6 +27,16 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
         if (!$access_result->isAllowed()) {
+          if ($is_owner) {
+            $owner_access = AccessResult::allowedIf($account->hasPermission('view own unpublished media'))
+              ->cachePerPermissions()
+              ->cachePerUser()
+              ->addCacheableDependency($entity);
+            if (!$owner_access->isAllowed()) {
+              $owner_access->setReason("The 'view own unpublished media' permission is required.");
+            }
+            return $owner_access;
+          }
           $access_result->setReason("The 'view media' permission is required and the media item must be published.");

I'd write this differently (see rerolled patch), which makes for this diff instead:

         }
+        $owner_access_result = AccessResult::allowedIfHasPermission($account, 'view own unpublished media')
+          ->cachePerUser()
+          ->addCacheableDependency($entity);
+        $access_result = $access_result->orIf($owner_access_result);
         return $access_result;

+++ b/core/modules/media/src/MediaAccessControlHandler.php
@@ -27,18 +27,12 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
+        $owner_access_result = AccessResult::allowedIfHasPermission($account, 'view own unpublished media')
+          ->cachePerUser()
+          ->addCacheableDependency($entity);
+        $access_result = $access_result->orIf($owner_access_result);
         return $access_result;

Unconditional cache per user is AFAIK exactly what the previous patch was trying to avoid, exactly because it in reality disables dynamic page completely on media/X.

NodeAccessControlHandler does something similar, for the same reason (and obviously there it is even more important).

Unconditional cache per user is AFAIK exactly what the previous patch was trying to avoid, exactly because it in reality disables dynamic page completely on media/X.

That's not what that does. orIf() should result in the second access result only being incorporated into the final value if the first one didn't already allow access.

Fair enough, but the previous code was also wrapped in an owner check, so even if access is denied we don't add the additional cache context unless the current user is the owner.

Of course that is actually a bug, thinking about it, same for nodes. If someone with the same permissions (and other contexts) access a node that you own and are allowed to view as owner, then it will be cached and not re-checked for you, denying you access.

Which perfectly shows the problem with the view own permissions. Doing that correctly would disable dynamic page cache for all nodes, making it useless on most sites.. :)

This needs work if the patch in #2936652: Add "view unpublished $bundle media" permissions for each media bundle gets commited

@govind.maloo

@Wim Leers provided a patch in #82 with several tests, namely :

public function testMediaAnonymousUserAccess
public function testReferencedRendering
class MediaAccessControlHandlerTest

Is there a reason you have not included these?

Re-rolling the patch from @govind.maloo in #90 for use with Drupal 8.4.x branch.

This is now a potential blocker for #2878119: Whether queued or not, update the media thumbnail and metadata before beginning the entity save database transaction, so I'm escalating it to Media critical.

Discussed with @Berdir in Slack. With regard to caching, the long and short of it is:

• If the media item is published, we want to cache the access result per media item. So addCacheableDependency($media).
• If it's not published, we want to cache the access result per user. So cachePerUser().
• That's it.

I started to implement the new proposal based on #78. First I had to reroll the patch. I think the proposed logic is now implemented, but a lot of tests will fail. Couldn't start to fix them, because I have to leave for the airport very soon.

Here is the interdiff

Fixed a small issue in the latest patch. We should return access neutral when the node is unpublished and the user is not the owner. Also tried to fix the tests.

Since it's been a year since I worked on this, and the resulting patch is significantly different than what it was at that time, I feel it shouldn't be a problem if I RTBC it.

Looking good, thanks @seanB and @chr.fritsch !

Found the following minor issues and one major one:

1. +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   @@ -171,4 +189,98 @@ public function testMediaAccess() {
   +   * Test media access of anonymous user.
   

   Should be tests.

2. +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   @@ -171,4 +189,98 @@ public function testMediaAccess() {
   +   * Test access for embedded medias.
   

   Tests access for embedded media items.

Also:

FILE: .../8.x/core/modules/media/tests/src/Functional/MediaAccessTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 10 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 77ms; Memory: 8Mb


FILE: ...modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 65 | ERROR | Doc comment for parameter $expected_cache_contexts does
    |       | not match actual variable name <undefined>
----------------------------------------------------------------------

+++ b/core/modules/media/src/MediaAccessControlHandler.php
@@ -24,11 +24,25 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
+        }
+        elseif ($is_owner) {
+          $access_result = AccessResult::allowedIf($account->hasPermission('view own unpublished media') && $is_owner)
+            ->cachePerPermissions()
+            ->cachePerUser()
+            ->addCacheableDependency($entity);
+          if (!$access_result->isAllowed()) {
+            $access_result->setReason("The 'view own unpublished media' permission is required.");
+          }
+        }
+        else {
+          $access_result = AccessResult::neutral()->cachePerPermissions();

#70 is still an issue as far as I can see.

I think the logic should be like this:

1. If the user doesn't have the 'view own unpublished media' permission, then return AccessResult::neutral() and cache by permission.
2. If the user does have the 'view own unpublished media', then always cache per user, because in this case, we always need to check if the current user is the owner or not.

And this should probably have additional test coverage for that case (i.e. two different users, both of whom have 'view own unpublished media' try to view the same item).

Discussed with @Berdir and @catch in Slack.

I admit I find this issue hopelessly confusing, so I just threw up my hands and asked them what we want to test. We need to be sure we have a test scenario like this:

do you have a test that creates an unpublished media as user A, then clears caches, then accesses it as user B (both users must have the same roles), should get access denied, thn again with user A and he should be allowed to access it

Let's make sure we have that.

Fixed all items in #104 and test scenario in #105

1. If the user doesn't have the 'view own unpublished media' permission, then return AccessResult::neutral() and cache by permission.
2. If the user does have the 'view own unpublished media', then always cache per user, because in this case, we always need to check if the current user is the owner or not.

Hopefully, I got this right.

Was also working on this, sorry vijaycs85. Just posting what I got since 102.

Fixed the coding standards and points 1/2 from #104. Also expanded the tests in MediaAccessControlHandlerTest to make it more obvious what we are testing (we were also missing some of the cases).

If the user does have the 'view own unpublished media', then always cache per user, because in this case, we always need to check if the current user is the owner or not.

If this is true, the cache contexts for 'update media' / 'delete media' permissions (which are confusingly enough the permissions for update own / delete own) are probably not correct as well:

        if ($account->hasPermission('update media') && $is_owner) {
          return AccessResult::allowed()->cachePerPermissions()->cachePerUser()->addCacheableDependency($entity);
        }

Added a new patch with the fixes, the extra tests and the fixes in MediaAccessControlHandler.

Was also working on this, sorry vijaycs85. Just posting what I got since 102.

no problem. I am sorry for stepping on @seanB. I thought of ping at #media slack, but I didn't :(

+++ b/core/modules/media/src/MediaAccessControlHandler.php
@@ -48,18 +48,25 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
-        if ($account->hasPermission('edit own ' . $type . ' media') && $is_owner) {
-          return AccessResult::allowed()->cachePerPermissions()->cachePerUser()->addCacheableDependency($entity);
+        if ($account->hasPermission('edit own ' . $type . ' media')) {
+          return AccessResult::allowedIf($is_owner)
+            ->cachePerPermissions()
+            ->cachePerUser()
+            ->addCacheableDependency($entity);
         }

This is not the same as before.

Now you always return if the user has the edit own permission, before only if he had access.

So if he has that permission and is not the owner, then he gets access denied now, even if he for example also has the "update any media" permission.

That said, agreed that before it wasn't correct either and could have been cached for a user that has the permission but isn't the owner.

We might need to switch away from early returns, and instead do something like adding by-user cache context always to a created $access result operation is update/delete and user has the respective own-permission.

no problem. I am sorry for stepping on @seanB.

Please, don't be sorry. I'm adding the test you wrote to the patch as well, it is a good test to have! Thanks for helping out.

#111 You are right, adding more tests and going to fix this now.

New patch attached.

Lots of great stuff in this patch. I haven't worked on it for a while, so I'd feel comfortable RTBCing with a few changes...

1. +++ b/core/modules/media/src/MediaAccessControlHandler.php
   @@ -24,45 +24,54 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +        // @todo Deprecate 'delete media' and 'delete any media' permission in
   

   Both permissions are still in media.permissions.yml, which would imply they're not actually deprecated.

2. +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   @@ -96,6 +97,21 @@ public function testMediaAccess() {
   +    $this->grantPermissions($role, ['view own unpublished media']);
   +    $this->drupalGet('media/' . $user_media->id());
   +    $this->assertCacheContext('user');
   +    $assert_session->statusCodeEquals(200);
   

   Should we also assert the presence of the user.permissions cache context?

3. +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   @@ -171,4 +187,133 @@ public function testMediaAccess() {
   +    $media_child = Media::create([
   +      'title' => 'Child media',
   +      'bundle' => $media_type->id(),
   +      'status' => FALSE,
   +      'uid' => $author->id(),
   +    ]);
   +    $media_child->save();
   

   Let's use $media_child->setUnpublished()->save() instead of setting 'status' directly.

4. +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   @@ -171,4 +187,133 @@ public function testMediaAccess() {
   +    $view_display = entity_get_display('media', $media_type->id(), 'full');
   +    $view_display->set('content', []);
   +    $view_display->setComponent('title', [
   +      'type' => 'string',
   +    ]);
   +    $view_display->setComponent('field_reference', [
   +      'type' => 'entity_reference_entity_id',
   +    ]);
   +    $view_display->save();
   

   All of these calls can be chained: entity_get_display()->set()->setComponent()->setComponent()->save(). Also, I think we should display the label of the referenced entity (and the field itself) so that we can assert the absence of known strings, rather than just a CSS selector which we don't control.

5. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
   @@ -0,0 +1,1009 @@
   +   * @param array $permissions
   +   *   The permissions that the user should be given.
   

   Should be string[].

6. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
   @@ -0,0 +1,1009 @@
   +   * @param string $operation
   +   *   The operation, one of view, update or delete.
   

   The operations should be single-quoted (e.g. 'view')

7. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
   @@ -0,0 +1,1009 @@
   +   * @param array $expected_cache_contexts
   +   *   Expected cache contexts.
   +   * @param array $expected_cache_tags
   +   *   Expected cache tags.
   

   Both of these should be string[].

8. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
   @@ -0,0 +1,1009 @@
   +   * @dataProvider testAccessProvider
   

   This should be providerAccess, in keeping with existing core conventions.

9. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
   @@ -0,0 +1,1009 @@
   +    $entity_values += [
   +      'status' => FALSE,
   +    ];
   +    if (!isset($entity_values['uid'])) {
   +      $entity_values['uid'] = $user->id();
   +    }
   +
   +    $entity_values['bundle'] = $this->createMediaType('test')->id();
   

   Can't this just be:

   $entity_values += [
     'status' => FALSE,
     'uid' => $user->id(),
     'bundle' => $this->createMediaType('test')->id(),
   ]

10. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
    @@ -0,0 +1,1009 @@
    +  /**
    +   * @param array $permissions
    +   *   User permissions.
    +   * @param \Drupal\Core\Access\AccessResultInterface $expected_result
    +   *   Expected result.
    +   * @param array $expected_cache_contexts
    +   *   Expected cache contexts.
    +   * @param array $expected_cache_tags
    +   *   Expected cache tags.
    

    Everything except $expected_result should be string[].

11. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
    @@ -0,0 +1,1009 @@
    +   * @param array|\Drupal\Core\Access\AccessResultInterface $expected_access_result
    +   *   The expected access result.
    

    This is never going to be an array.

12. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
    @@ -0,0 +1,1009 @@
    +   * @param array $expected_cache_contexts
    +   *   Expected contexts.
    +   * @param array $expected_cache_tags
    +   *   Expected cache tags
    

    Should be string[]

13. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
    @@ -0,0 +1,1009 @@
    +   * @param \Drupal\Core\Access\AccessResult $actual
    +   *   The actual access result.
    

    Why isn't this AccessResultInterface?

14. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
    @@ -0,0 +1,1009 @@
    +  public function testAccessProvider() {
    

    Needs to be called providerAccess(), because any method that starts with test() will be called a test by PHPUnit.

15. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
    @@ -0,0 +1,1009 @@
    +    $test_data = [];
    

    This has got to be one of the most thorough tests in all of core. Amazing work!

16. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
    @@ -0,0 +1,1009 @@
    +    $test_data['owner permissionless / published / view'] = [
    

    Can we change every occurrence of the odd-sounding 'owner permissionless' to 'owner, no permissions'?

17. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
    @@ -0,0 +1,1009 @@
    +    $test_data['non owner permissionless / published / view'] = [
    

    "not owner, no permissions"

18. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
    @@ -0,0 +1,1009 @@
    +    $test_data['owner view / published / view'] = [
    

    "owner, can view media"

19. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
    @@ -0,0 +1,1009 @@
    +    $test_data['non owner view / published / view'] = [
    

    "not owner, can view media"

20. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
    @@ -0,0 +1,1009 @@
    +    $test_data['owner view unpublished / published / view'] = [
    

    "owner, can view unpublished media"

21. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
    @@ -0,0 +1,1009 @@
    +    $test_data['non owner view unpublished / published / view'] = [
    

    "not owner, can view unpublished media"

22. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
    @@ -0,0 +1,1009 @@
    +    $test_data['owner view unpublished any update any delete / published / view'] = [
    

    "owner, can view unpublished media and update or delete any media"

23. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
    @@ -0,0 +1,1009 @@
    +    $test_data['owner view unpublished own update own delete / published / view'] = [
    

    "owner, can view unpublished media and update or delete own media"

24. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
    @@ -0,0 +1,1009 @@
    +    $test_data['non owner view unpublished own update own delete / published / view'] = [
    

    "not owner, can view unpublished media and update or delete own media"

25. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
    @@ -0,0 +1,1009 @@
    +    $test_data['owner view unpublished all update all delete / published / view'] = [
    

    "owner, can view unpublished media and update or delete all media"

26. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
    @@ -0,0 +1,1009 @@
    +    $test_data['non owner view unpublished all update all delete / published / view'] = [
    

    "not owner, can view unpublished media and update or delete all media"

27. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
    @@ -0,0 +1,1009 @@
    +  public function providerCreateAccess() {
    

    I think all the array keys in this data provider also need to be more clearly phrased...

#115

1. That's why it's still a todo with a link to the other issue right?
2. I believe the 'user.permissions' cache context is omitted here because it's folded in the parent 'user' context.
3. Fixed
4. Fixed
5. Fixed
6. Fixed
7. Fixed
8. Fixed
9. Fixed
10. Fixed
11. Fixed
12. Fixed
13. Fixed
14. Fixed
15. Yay!
16. Fixed
17. Fixed
18. Fixed
19. Fixed
20. Fixed
21. Fixed
22. Fixed
23. Fixed
24. Fixed
25. Fixed
26. Fixed
27. Fixed. Basically started over with the cases since some didn't seem to make sense.

Interdiff looks good. This is a critical Media issue that has been in limbo long enough; I feel okay about it.

1. +++ b/core/modules/media/src/MediaAccessControlHandler.php
   @@ -24,45 +24,54 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +        else {
   +          $access_result = AccessResult::neutral()->cachePerPermissions();
            }
   
   +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
   @@ -0,0 +1,1064 @@
   +    // media item with only the 'view media' permission.
   

   This result should still have ->addCacheableDependency($entity); as the result can change if the entity changes.

2. +++ b/core/modules/media/src/MediaAccessControlHandler.php
   @@ -24,45 +24,54 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +          return AccessResult::allowedIf($is_owner)
   +            ->cachePerPermissions()
   +            ->cachePerUser()
   +            ->addCacheableDependency($entity);
            }
            return AccessResult::neutral("The following permissions are required: 'update any media' OR 'update own media' OR '$type: edit any media' OR '$type: edit own media'.")->cachePerPermissions();
   ...
   +          return AccessResult::allowedIf($is_owner)
   +            ->cachePerPermissions()
   +            ->cachePerUser()
   +            ->addCacheableDependency($entity);
            }
            return AccessResult::neutral("The following permissions are required: 'delete any media' OR 'delete own media' OR '$type: delete any media' OR '$type: delete own media'.")->cachePerPermissions();
   

   The new early return for non-allowed case means that a reason will no longer be set if the user is not the owner.

#118

1. Fixed
2. Fixed

Changes look correct to me. Back to RTBC once green on all backends.

Upgrading this to critical since this is a blocker to commit #2878119: Whether queued or not, update the media thumbnail and metadata before beginning the entity save database transaction, which is also a critical issue.

In #2878119: Whether queued or not, update the media thumbnail and metadata before beginning the entity save database transaction the decision was made to temporarily unpublish media when metadata fetching is queued. We do this to make sure incomplete media items are not shown on the site and/or possibly indexed by search engines. Without the permission added in this issue, there is no way for a user to see the media item that it just created.

+++ b/core/modules/media/src/MediaAccessControlHandler.php
@@ -24,45 +24,63 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
       case 'update':
...
       case 'delete':
...

Are the 'update' and 'delete' changes in this patch relevant to this issue? If so, how? If not, can we split them out into a separate issue?

Possibly related, the issue summary's proposed resolution still says "Allow user with create/update/delete permission to view her/his unpublished node", but it doesn't look like that's what the patch implements anymore.

Are the 'update' and 'delete' changes in this patch relevant to this issue? If so, how? If not, can we split them out into a separate issue?

In this issue we've noted missing test coverage that allowed us to have this "bug" in the first place, so we spent a lot of time getting the tests right. In doing so, we uncovered more bugs, some related to update/delete access. We could remove all the update/delete related test coverage we added and fix the update/delete bugs in a separate issue, but that would mean a lot of work with little added value. I hope we can avoid that.

Updated the IS to reflect the new permission a little better.

The issue this is blocking (#2878119: Whether queued or not, update the media thumbnail and metadata before beginning the entity save database transaction) is no longer critical.

@efflugentsia's question is answered, so I think we can go back to RTBC.

In doing so, we uncovered more bugs, some related to update/delete access.

Can the IS be updated to include what those bugs were and how they were fixed?

We could remove all the update/delete related test coverage we added and fix the update/delete bugs in a separate issue, but that would mean a lot of work with little added value.

I think the added value would be a commit history that's clear about what got fixed in what commit. But I don't want to add a lot of extra work to achieve that, so perhaps just a more complete issue summary (and maybe also including it in the issue title too, if possible, since that's what goes into the commit message) is sufficient.

Additional value would be if it turned out we needed a revert for one of the changes (either the change to view access logic, or the change to update/delete logic), we'd be able to do it without reverting the other change. So if it weren't a lot of work to split it, I'd prefer that, but since it is a lot of work to split, I'm okay with taking the risk of keeping it together.

Updated the IS. Looked over the patch once more. Updated the reasons since they could be improved. Also added the media entity as cacheable dependency for the 'edit any $type media' and 'delete any $type media' since it was missing. This made me notice that we are missing tests for the 'edit any $type media' / 'edit own $type media' and 'delete any $type media' / 'delete own $type media' permissions, so that got added as well.

Test fixes.

Discussed this in Slack with @seanB.

We understand that this issue has spiraled way out of control, and the patch we have so far now contains both bug fixes and new features. This is obviously very committer-unfriendly, and although the test coverage is stunningly comprehensive, the mixed nature of the patch makes it prone to being reverted. Which would shatter our hearts.

This, according to Sean, is the laundry list of permissions-related bugs we are facing (and trying to fix in this one patch):

- Unpublished items can not be viewed by owner
- '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
- The reasons returned when update / delete access is forbidden contain permissions that do not exist

Although it saddens us to do it, we have agreed to deal with all of these problems in separate issues, which will be much smaller in scope and a lot easier to review and commit. We're hoping that as many of them as possible can land in 8.6, since they are mostly bug fixes. We will add as much test coverage as we can for each one, drawing from the amazing work that has already been done -- although no single one of them can hope to be as extensive as what we've got in #129 -- and hopefully, once all these are fixed we can add a new mega-test (i.e., the colossal data provider from the latest patch) which extra-confirms all of this stuff works as expected.

So...I'm thinking of turning this into a meta-issue and filing sub-issues for everything mentioned above. Thoughts?

So...I'm thinking of turning this into a meta-issue and filing sub-issues for everything mentioned above. Thoughts?

That sounds great, thank you! Also, if you want to RTBC #129, and then if any sub-issue's patch contains only a subset of changes from #129, without introducing any additional changes, you can just file the sub-issue as RTBC.

Feel free to split how you'd like, but I would suggest the following:

1. A patch that changes no behavior at all, but adds any test coverage from #129 that already passes on HEAD. Including MediaAccessControlHandlerTest, with providerAccess, and only the items in that provider that pass on HEAD. I can commit that first, so that the remaining ones can be rolled on top of it.
2. A patch that fixes the reason messages, and adds providerAccess items from #129 that pass as a result. If none do until other steps are done, then that's ok too, I'd be fine committing without test coverage, and punting that coverage to a later step.
3. A patch that adds 'view own unpublished media' and providerAccess items that that enables.
4. A patch that fixes all the 'update' and 'delete' bugs and the remaining test coverage that that enables.

The attached patch removes all the changes related to the update/delete access. So this should just add some the new permission and extra tests for existing functionality.

Created follow ups:
#2998824: MediaAccessControlHandler update/delete access caching is not correct
#2998825: MediaAccessControlHandler update/delete access reason contains not existing permissions

Updated the IS to remove the update/delete access cache issue since that is now a followup.

So:

• points #131 points 1+3 = #132
• #131.2 = #2998825: MediaAccessControlHandler update/delete access reason contains not existing permissions
• #131.4 = #2998824: MediaAccessControlHandler update/delete access caching is not correct

Correct?

I think that is already a big simplification in scope: the patch became ~55% smaller.

That is correct :). Hopefully this is enough to get this done.