EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler

I have prepared a raw draft of the implementation, however I am running into some issues I do not know how to resolve (yet)

I could not find any documentation or resources about file entity endpoints.

If someone could assist a bit on getting me up and running that would be great.

+++ b/core/modules/hal/tests/src/Functional/EntityResource/File/FileHalJsonAnonTest.php
@@ -0,0 +1,155 @@
+        'http://d8x.dev/rest/relation/file/file/uid' => [

Where do you get this url from? I think it should be using $this->baseUrl as well?

Did you find this information because you know where to or was I looking in wrong places?

This is at minimum blocked on #2853211: EntityResource::post() incorrectly assumes that every entity type has a canonical URL, possibly also blocked on #1927648: Allow creation of file entities from binary data via REST requests.

#2853211: EntityResource::post() incorrectly assumes that every entity type has a canonical URL landed, this is now (at least partially) unblocked!

But the more I try to pass all the tests, the more looks like we need just to wait next issues:

Well the thing is that all those issues would hugely benefit if this test coverage already existed. Because then they can just update this test coverage!

However, I do think it's fine to use $this->markTestSkipped() for the POST and PATCH testing, because as you say, those are riddled with problems/bugs right now. You can then just add an explicit @todo to the relevant issues, to indicate that it's their responsibility to add this test coverage.

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -404,7 +404,7 @@ public function testGet() {
   -    $this->assertEquals($this->getExpectedCacheTags(), empty($cache_tags_header_value) ? [] : explode(' ', $cache_tags_header_value));
   +    $this->assertEquals($this->getExpectedCacheTags(), empty($cache_tags_header_value) ? [] : explode(' ', $cache_tags_header_value), '', .0, 2, TRUE);
   

   :O What is this doing? I have no idea!

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/File/FileResourceTestBase.php
   @@ -0,0 +1,208 @@
   +  /**
   +   * Ugly trick.
   +   */
   +  protected function correctFileOwner() {
   +    $account = static::$auth ? User::load(2) : User::load(0);
   +    $this->entity->setOwnerId($account->id());
   +    $this->entity->setOwner($account);
   +    $this->entity->save();
   +  }
   ...
   +  protected function grantPermissionsToTestedRole(array $permissions) {
   +    if ($permissions === ['restful delete entity:file']) {
   +      $this->correctFileOwner();
   +    }
   ...
   +  protected function setUpAuthorization($method) {
   +    $this->correctFileOwner();
   ...
   +    $author = User::load(3);
   

   I have to admit, I was completely confused by this…

   So I applied the patch locally, and tried to make it simpler: I tried to let the created File entity have the "correct" owner (the current user), which is user 0 for anon tests, and user 2 for auth tests.

   But then this in FileAccessControlHandler gets in the way:

         elseif ($entity->getOwnerId() == $account->id()) {
           // This case handles new nodes, or detached files. The user who uploaded
           // the file can always access if it's not yet used.
           return AccessResult::allowed();
         }
   

   … because that means the EntityResourceTestBase::testGet() coverage that verifies that without the necessary authorization, you get a 403.

   So, that's why you create the File entity with user 3 as the owner.

   Okay.

   So then the next problem that this solves is that in order for this file to be accessible, you then need to change the file owner to 0 or 2, precisely to get the cited code to allow access!
   But why? Because you use a private:// file, not a public:// file! And hence the very first check in FileAccessControlHandler does not apply.
   So why use private://? It's rather uncommon — the 90% use case is public://. So IMHO we should test that.

   So, in this reroll, I'm changing it from private:// to public://.

   But then you again have the problem that the public file is always granted access. So let's do the same here that we've done in many other tests: update FileAccessControlHandler to require the access content permission in order for it to be allowed.
   So I did that this in reroll too.

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/File/FileResourceTestBase.php
   @@ -0,0 +1,208 @@
   +    if (!$author) {
   

   AFAIK user 3 will never exist? So let's just go ahed and always create this, that will slightly simplify this code.

4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/File/FileResourceTestBase.php
   @@ -0,0 +1,208 @@
   +      'uid' => $uid,
   +      'filename' => 'drupal.txt',
   

   Let's use setOwnerId(), setFilename() etc.

5. +++ b/core/modules/rest/tests/src/Functional/EntityResource/File/FileResourceTestBase.php
   @@ -0,0 +1,208 @@
   +  protected function getExpectedCacheContexts() {
   +    return [];
   +  }
   

   That's … interesting. Why doesn't this at least vary by the user.permissions cache context?

   Because

     protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
       /** @var \Drupal\file\FileInterface $entity */
       if ($operation == 'download' || $operation == 'view') {
         if (\Drupal::service('file_system')->uriScheme($entity->getFileUri()) === 'public') {
           // Always allow access to file in public file system.
           return AccessResult::allowed();
         }
   
   

   If so, let's add a comment here: @see \Drupal\file\FileAccessControlHandler::checkAccess().

6. +++ b/core/modules/rest/tests/src/Functional/EntityResource/File/FileResourceTestBase.php
   @@ -0,0 +1,208 @@
   +  public function testPost() {
   +    $this->markTestSkipped();
   +  }
   

   So, yes, like this. But let's add an @todo https://www.drupal.org/node/1927648 :)

   Let's do something similar for testPatch().

7. +++ b/core/modules/rest/tests/src/Functional/EntityResource/File/FileResourceTestBase.php
   @@ -0,0 +1,208 @@
   +  }
   +}
   

   Nit: needs \n in between.

Yay! :) It's one thing for me to spend some time digging into this. But if you then take that, and push it further, that makes it totally worthwhile! Thank you, @vaplas! :) Once again, great work!

WOOOT GREEN PATCH!

1. +++ b/core/modules/file/tests/src/Functional/FileManagedAccessTest.php
   @@ -29,7 +34,7 @@ public function testFileAccess() {
   -    $account = $this->createUser(['access site reports']);
   +    $account = $this->createUser(['access content']);
   
   @@ -54,7 +59,7 @@ public function testFileAccess() {
   -    $account = $this->createUser(['access site reports']);
   +    $account = $this->createUser(['access content']);
   

   Heh… this means that the access site reports permission that was there before was not necessary.

   Still, let's not change this here. Let's just add the access content permission. We only want to update these tests in the most minimal way possible, and that means only adding the necessary permissions for file access, not fixing existing permissions/bugs.

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/File/FileResourceTestBase.php
   @@ -0,0 +1,199 @@
   +   * Additional user to prevent access to file by uid.
   

   s/uid/UID/

   Also, I don't think this comment is entirely accurate anymore, since my changes in #15?

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/File/FileResourceTestBase.php
   @@ -0,0 +1,199 @@
   +      'status' => 1,
   

   Nit: let's use ->activate() instead :)

4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/File/FileResourceTestBase.php
   @@ -0,0 +1,199 @@
   +    $file->setOwnerId($this->author->id());
   +    $file->setFilename('drupal.txt');
   +    $file->setMimeType('text/plain');
   +    $file->setFileUri('public://drupal.txt');
   +    $file->set('status', FILE_STATUS_PERMANENT);
   

   I was going to ask: why not chain these calls? — but then I noticed why: these methods don't allow that :(

   So, this code is good as it is right now: fixing those setters is out of scope here.

5. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
   @@ -64,7 +64,31 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
   -    return $this->entityManager->getStorage('file')->create($data);
   +    $entity = $this->entityManager->getStorage('file')->create($data);
   +
   +
   +    // Remove links from data array.
   +    unset($data['_links']);
   +    // Get embedded resources and remove from data array.
   +    $embedded = [];
   +    if (isset($data['_embedded'])) {
   +      $embedded = $data['_embedded'];
   +      unset($data['_embedded']);
   +    }
   +
   +    // Flatten the embedded values.
   +    foreach ($embedded as $relation => $field) {
   +      $field_ids = $this->linkManager->getRelationInternalIds($relation);
   +      if (!empty($field_ids)) {
   +        $field_name = $field_ids['field_name'];
   +        $data[$field_name] = $field;
   +      }
   +    }
   +
   +    // Pass the names of the fields whose values can be merged.
   +    // @todo https://www.drupal.org/node/2456257 remove this.
   +    $entity->_restSubmittedFields = array_keys($data);
   +    return $entity;
   

   This is by far the most complex or even "wtf" part of this patch. You already mentioned this in #17.

   I still don't fully understand this though. I'd like to ask you to:

   1. Move as much of that logic as possible into a protected helper method.
   2. Then add thorough documentation to that helper method to explain why that's necessary.

This is very close now! :)

+++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
@@ -64,7 +64,14 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
+    // Pass the names of the fields whose values can be merged.
+    // @todo https://www.drupal.org/node/2456257 remove this.
+    unset($data['_links'], $data['_embedded']);
+    $entity->_restSubmittedFields = array_keys($data);

This is much better, but still not clear enough.

Why is the default value for $entity->_restSubmittedFields not correct?

And why unset _links and _embedded?

Oh, wait! It's simply that \Drupal\hal\Normalizer\FileEntityNormalizer subclasses \Drupal\hal\Normalizer\ContentEntityNormalizer but its ::denormalize() method does not call the parent implementation! The stuff you were doing before you simply copied over from the parent implementation.

So, then my question becomes: why can't we call the parent method?

Thanks! Much better! RTBC assuming this comes back green :)

Also, I want to note that the changes made to \Drupal\hal\Normalizer\FileEntityNormalizer::denormalize() imply that PATCHing File entities via hal_json simply is broken in HEAD, and likely never worked.

Makes sense. Because indeed, in \Drupal\hal\Normalizer\ContentEntityNormalizer:

  public function denormalize($data, $class, $format = NULL, array $context = []) {
    // Get type, necessary for determining which bundle to create.
    if (!isset($data['_links']['type'])) {
      throw new UnexpectedValueException('The type link relation must be specified.');
    }

This also looks good to me, as a side note #1927648: Allow creation of file entities from binary data via REST requests will hopefully just remove the FileEntityNormalizer altogether or just keep a small part for normalization only.

#27: that sounds great! :)

1. +++ b/core/modules/file/src/FileAccessControlHandler.php
   @@ -22,8 +22,7 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   -        // Always allow access to file in public file system.
   -        return AccessResult::allowed();
   +        return AccessResult::allowedIfHasPermissions($account, ['access content', 'administer files'], 'OR');
   

   I'm not sure about this change. I guess we have an issue here in that access here means access to the entity info whereas access in the original comment was implying access to the file. I'm not sure it's really correct to change this. As per othe issues I think we need an issue to discuss the overall strategy here and whether or not 'access content' is appropriate for these type of things. Atm it feels like we're adding this to make the generic tests pass rather than for reasons of correctness.

2. +++ b/core/modules/file/src/FileAccessControlHandler.php
   @@ -48,8 +47,9 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   -      // Only the file owner can delete and update the file entity.
   -      if ($account->id() == $file_uid[0]['target_id']) {
   +      // Only the file owner or user with 'administer files' permission can
   +      // delete and update the file entity.
   +      if ($account->id() === $file_uid[0]['target_id'] || $account->hasPermission('administer files')) {
            return AccessResult::allowed();
          }
          return AccessResult::forbidden();
   

   We're missing caching the result by permissions no? Also the owner can change so we should be caching by entity change too (although this looks like it is an existing bug).

Yep, this is blocked on #2870018: Should the 'access content' permission be used to control access to viewing configuration entities via REST reaching consensus.

#31:

1. See #2870018-6: Should the 'access content' permission be used to control access to viewing configuration entities via REST. 'access content' is the "baseline access" permission. The 'administer files' permission would be consistent with other entity admin_permissions, and typically allows doing anything to any file.
2. You're right, good catches.

Consensus was achieved! Quoting #2870018-35: Should the 'access content' permission be used to control access to viewing configuration entities via REST:

1. config entities that do not yet have an access control handler nor an admin_permission (RdfMapping, Tour): add an admin_permission.
2. update all access control handlers for entity types that currently grant access without requiring any permission (View, Menu, File, DateFormat): change them to require the entity type's admin_permission or the entity-type specific "view" permission (yet to be added), so for example: AccessResult::allowedIfHasPermissions($account, ['view menu', 'administer menu'], 'OR').

Since File has an access control handler that grants blanket access, we need to implement the second option. But the File entity does not have a "view" permission, nor an "admin" permission. So it's not clear how to continue. Furthermore, there's an important distinction between "viewing" and "downloading".

Next, downloading ($op === 'download') public:// files must always be free of permissions: that's how we shipped D8, we can't change that now. But we can change the requirements for viewing ($op === 'view') IMO, because likely nobody has been using that so far.

So I propose we strike a middle ground here: we only change the behavior for:

1. public:// files
2. that are being viewed, not downloaded
3. and rather than requiring a new view files permission, we just require a access content permission, because that aligns more closely with user expectations, further reducing disruption, and matching expectations

Since this is diverging (by necessity) from the simple consensus rules we arrived at, marking this NR, not RTBC.

This issue is fixing bugs, not just adding test coverage, so let's retitle and rescope it appropriately.

1. +++ b/core/modules/file/src/FileAccessControlHandler.php
   @@ -22,8 +22,12 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +        if ($operation === 'download') {
   +          return AccessResult::allowed();
   +        }
   +        else {
   +          return AccessResult::allowedIfHasPermissions($account, ['access content', 'administer files'], 'OR');
   +        }
   

   I thought that these access results ought to cache until entity changes - because of the scheme check - but @Wim Leers pointed out you shouldn't be able to change scheme.

2. +++ b/core/modules/file/src/FileAccessControlHandler.php
   @@ -48,8 +52,9 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   -      // Only the file owner can delete and update the file entity.
   -      if ($account->id() == $file_uid[0]['target_id']) {
   +      // Only the file owner or user with 'administer files' permission can
   +      // delete and update the file entity.
   +      if ($account->id() === $file_uid[0]['target_id'] || $account->hasPermission('administer files')) {
            return AccessResult::allowed();
          }
          return AccessResult::forbidden();
   

   These access results should be cached until the entity changes because the owner can change.

NW for #36.2.

Complying with #35, even though I don't fully agree with it.

The primary scope of this issue is a task: adding missing test coverage. This unveiled something that was forgotten before, which can be considered either a task to add it, or a bug, depending how you look at it. But that doesn't change the primary scope of the issue: that's certainly a task.

Anyway, let's get this patch updated. Any volunteers?

Thanks, @vaplas!

I wrote this at #34:

Since this is diverging (by necessity) from the simple consensus rules we arrived at, marking this NR, not RTBC.

But nobody is ever going to review this, except a committer. It's been sitting here for >2 weeks. In the end, a committer needs to sign off on this anyway. I didn't upload the patch in #40, so I'm going to RTBC again.

#40 is green again. It was re-tested on June 5 and failed. It was re-tested twice since then, and was green each time. Likely a random failure.

+++ b/core/modules/file/file.permissions.yml
@@ -1,2 +1,6 @@
   title: 'Access the Files overview page'
+
+administer files:
+  title: 'Administer files'
+  restrict access: true
diff --git a/core/modules/file/src/FileAccessControlHandler.php b/core/modules/file/src/FileAccessControlHandler.php

I understand why it's necessary for access, but I'm not convinced from a usability point of view about adding a generic 'administer files' permission that unless REST is enabled, doesn't actually allow you to administer files at all - it ends up being a no-op permission.

#45: Do you have an alternative proposal?

Not really yet. On config entities we used 'administer site configuration' which was a really good compromise, that's not appropriate here.

I did think about something horrible like hiding the permission in the UI then altering it to visible, but not sure that's a good trade-off at all.

I think administer files would actually be a useful permission for content administrators/moderators. It could allow them to see all files at /admin/content/files, even private files.

(The patch does not currently do that, to minimize changes. But I think that'd be a good justification, and it solves an actual need.)

I like #48, but until we have that, we could also dynamically define this permission in rest.module if the file resource is enabled. That way we could avoid the UX problem.

#49: that's a no-go, because:

1. we'd be changing altering access control in the REST module, so it'd suddenly behave different, which is bad practice
2. more importantly, then what about https://www.drupal.org/project/jsonapi and https://www.drupal.org/project/graphql?

we'd be changing altering access control in the REST module, so it'd suddenly behave different, which is bad practice

That's precisely not true. "it" in your case is solely the file resource, so nothing changes. The thing that you claim changes commences its existance at the precise time the permission does.

Sorry, #51 does not make sense to me. If you have both REST and JSON API installed, then having the REST module changing access control to the File entity will also affect JSON API.

I'm confused by the conversation from #45 on. Why is it exactly that we need this patch to add an 'administer files' permission? Specifically:

1. +++ b/core/modules/file/src/FileAccessControlHandler.php
   @@ -22,8 +22,12 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +          return AccessResult::allowedIfHasPermissions($account, ['access content', 'administer files'], 'OR');
   

   Why not just base this on 'access content' alone?

2. +++ b/core/modules/file/src/FileAccessControlHandler.php
   @@ -48,11 +52,12 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   -      // Only the file owner can delete and update the file entity.
   -      if ($account->id() == $file_uid[0]['target_id']) {
   -        return AccessResult::allowed();
   +      // Only the file owner or user with 'administer files' permission can
   +      // delete and update the file entity.
   +      if ($account->id() === $file_uid[0]['target_id'] || $account->hasPermission('administer files')) {
   +        return AccessResult::allowed()->addCacheableDependency($entity);
          }
   

   Why do we want this change?

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/File/FileResourceTestBase.php
   @@ -0,0 +1,192 @@
   +      case 'POST':
   +      case 'PATCH':
   +      case 'DELETE':
   +        $this->grantPermissionsToTestedRole(['access content', 'administer files']);
   

   Instead of granting the tested role 'administer files' permission, can we instead have the test agent authenticate as the file owner (if we want to test success) and as someone other than the file owner (if we want to test a permission denied)?

Back to RTBC, thanks @vaplas for answering #53!

I could apply the patch, so no reroll is needed

The only non-trivial change in #58 is this:

+++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
@@ -59,6 +59,7 @@ public function normalize($entity, $format = NULL, array $context = []) {
+    if (isset($data['uri'])) {

I first wondered why this is necessary. So I looked into it.

Without this change PATCHing of file entities is broken 100% of the time when using the hal_json format because \Drupal\hal\Normalizer\FileEntityNormalizer::denormalize() unconditionally uses $data['uri'], which is forbidden as of http://cgit.drupalcode.org/drupal/commit/?h=HEAD&id=c732355412b84a6f7079....

So this is breaking a regression introduced by SA-CORE-2017-003. That regression would never have happened if this test coverage had already been part of Drupal core.

Therefore +1 for this change.

I also took another good look at the feedback @effulgentsia posted in #53, which @vaplas replied to in #54.

Perhaps it's useful for me to expand on what @vaplas already wrote.

EntityResourceTestBase makes one key assumption wrt access control: you should be able to restrict access for every operation (GET/PATCH/POST/DELETE) for every entity type. i.e. you should first have to grant some permission before some action is allowed. Otherwise there's no way to restrict API access for a given operation for a given entity type, which means every user is allowed to do it.

Therefore as part of that, this issue is changing FileAccessControlHandler to at least require the access content permission to be allowed to GET a File. This is not contested by @effulgentsia.
However, the other change that's necessary, is to have some permission to be allowed to PATCH or DELETE a File (POSTing doesn't work yet until #1927648: Allow creation of file entities from binary data via REST requests lands, so there's no point tackling that here). Right now, there is no such restriction. The only requirement is that the user trying to perform that action is also the owner.
This is the contentious part, raised in #53.2. (#53.1 is merely a consequence of #53.2 — any user able to administer files should also be able to view them.)

So to answer #53.3: if the owner of the file is in fact the test user, that means EntityResourceTestBase's minimum requirement wrt access control is violated: there's no way to restrict a particular operation on a particular entity type! @vaplas came up with a clever but super confusing work-around, which I analyzed and critiqued in #15.2. I proposed switching to this administer files permission.

@catch already raised concerns about adding this administer files permission in #47. Now @effulgentsia is raising that concern again in #53.

So I think we'll need to come up with some alternative. Because what we have right now, is confusing and has poor security. Adding an administer files permission is a solution.

I'd ping the File module maintainer, but there's no maintainer…

So now we have two choices:

1. either we revert most of the changes in #15 — i.e. we go back to the correctFileOwner() work-around
2. or @catch or @effulgentsia need to propose an alternative solution

Discussed with @effulgentsia. We decided on choice 1.

It's still very close to your #14, but I tried to make it a little bit cleaner — glad you like it! :)

Need to review again properly, but conceptually #64 looks fine to me and a good compromise.

Glad to hear that!

+++ b/core/modules/file/tests/src/Functional/FileManagedAccessTest.php
@@ -12,6 +12,11 @@
   /**
+   * {@inheritdoc}
+   */
+  public static $modules = ['node'];
+

That does mean that file introduces a hidden dependency on the node module, which it currently doesn't have?

What if someone has a site that doesn't have node enabled but uses files for e.g. custom entity types?

We should probably move the access content permission to system module or at least discuss that in a follow-up?

I just opened up an issue about moving 'access content' to system module: #2907121: Move 'access content' permission to system module

❤️👏

So, this was RTBC in #64, on June 28. That's >2 months ago.

@Berdir raised concerns in #69, about this introducing a hidden dependency on the node module. @vaplas replied to this in #70, elaborating on it, and @catch mentioned in #71 that we probably want to move it to system module. So @dawehner just created the issue in #76 to actually do that.

Once that's done, hopefully this can become an easy commit :)

Apparently #2907121: Move 'access content' permission to system module is a duplicate of #2892821: Core modules check node module's "access content" permission for displaying things that have nothing to do with nodes, which we didn't notice, despite @vaplas helpfully linking it!

Marking this postponed per #78.

So since this issue deals with "tighten access control handler" for files not expanding this might not be right issue but just thought I would comment here because I added patch to #1927648: Allow creation of file entities from binary data via REST requests expand file access handler and it was mentioned it might more sense here. You can see the patch on the other but the gist of it is when dealing with updating or deleting file entities

Shouldn't we be checking if the user has an admin permission for any of the entity types that file is attached to?

Also it seems like if the user can delete all the entities that reference the file that the user should also be able to delete the file. I imagine there will be many situations that the file is only attached to 1 entity.

Also if the user can update 1 entity that references the file shouldn't that user be able to update the file.

Thoughts? Belong in another issue?

Belong in another issue?

Belongs in another issue. The scope of this issue really is "add File REST test coverage". Sadly, this requires a change to the existing access control logic — although it arguably should be descoped from this issue and get an issue of its own. But in any case, "totally revamp file access control handling", which is what #80 implies, deserves its own issue!

#81 ok that makes sense. Maybe what I was describing in #80 should really only happen if users ask for it rather imagine how users might want to use the file resource.

See #2824572-55: Write EntityResourceTestBase subclasses for every other entity type. and #2824572-57: Write EntityResourceTestBase subclasses for every other entity type.: this is the last remaining non-RTBC blocker for completing REST test coverage for all core entity types!

Not only does this block #2824572: Write EntityResourceTestBase subclasses for every other entity type., it also kinda/probably blocks #1927648: Allow creation of file entities from binary data via REST requests. Both are major.

This is still blocked on #2892821: Core modules check node module's "access content" permission for displaying things that have nothing to do with nodes, but that patch is RTBC and will hopefully soon be committed. So let's already reroll this patch on top of that.

Now there's no longer a dependency on the node module, so the tests can be updated, which addresses Berdir's concerns.

This should probably be blocked on #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field too.

#2892821: Core modules check node module's "access content" permission for displaying things that have nothing to do with nodes landed!

#84 was a straight rebase of #64. #64 was green, #84 has 6 failures: all the Media entity REST PATCH tests. #64 didn't have any failures, because it predates the Media entity REST tests.

The root cause? This patch requires you to have the access content permission to be allowed to view File entities. When PATCHing a Media entity, we point to a File entity, and for that, you need the access content permission. So just granting the access content permission solves the problem (and 99.999% of sites will have this enabled already anyway, so it's not a behavior change for them).
Of course, in theory, you shouldn't need that permission if you're not actually changing which File entity is referenced by the Media entity! That is being fixed by #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior. So for now, I'm granting the access content permission, and am including a @todo to remove that in #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior.

Great, #88 is green! But this is still blocked on #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field, so postponing once more…

Per #2825487-128: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field.17, I think this should probably go in before #2825487.

Apparently the patch from #88 still applies. Reuploading to see if it still passes tests.

I think the patch is actually ready — we only need an IS update here, and updated the CR.

Did that.

I rolled the last few patches, but the bulk of the work here was actually done by @vaplas! At some point, we were stuck on picking one of two approach. Core committer @effulgentsia picked one, and I applied that in #64. That was trivial — I even RTBC'd #64 in the same go. It was un-RTBC'd because it should've been blocked on #2892821: Core modules check node module's "access content" permission for displaying things that have nothing to do with nodes, which has since gone in. So I rerolled for that too, which was even more trivial.
Therefore it should still be okay for me to RTBC this.

👏😀

#2800873: Add XML GET REST test coverage, work around XML encoder quirks just landed, which means this now needs to add XML REST test coverage for File too!

EDIT: queued a PHP 7.1 test for #96, which should now fail, because #2800873 landed. And this patch should of course be green.

--- a/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
+++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
   public function denormalize($data, $class, $format = NULL, array $context = []) {

Upon re-reviewing this, I noticed these changes to HAL's FileEntityNormalizer::denormalize().

These changes are out of scope here, and also unnecessary. Because \Drupal\Tests\rest\Functional\EntityResource\File\FileResourceTestBase is not yet testing the uploading of files. That's being taken care of in #1927648: Allow creation of file entities from binary data via REST requests.

Simply reverting that change.

Ok, so 107's reverting of the changes made to HAL's FileEntityNormalizer::denormalize() causes the following failures:

1) Drupal\Tests\hal\Functional\EntityResource\File\FileHalJsonAnonTest::testPatch
Exception: Notice: Undefined index: uri
Drupal\hal\Normalizer\FileEntityNormalizer->denormalize()() (Line: 62)

…

1) Drupal\Tests\hal\Functional\EntityResource\File\FileHalJsonBasicAuthTest::testPatch
Exception: Notice: Undefined index: uri
Drupal\hal\Normalizer\FileEntityNormalizer->denormalize()() (Line: 62)

…

1) Drupal\Tests\hal\Functional\EntityResource\File\FileHalJsonCookieTest::testPatch
Exception: Notice: Undefined index: uri
Drupal\hal\Normalizer\FileEntityNormalizer->denormalize()() (Line: 62)

Unsurprisingly, 3 failures: the HAL+JSON PATCH test in all 3 of core's authentication mechanisms.

Well, that means we'll just have to skip PATCH File testing of HAL+JSON. It's up to #1927648: Allow creation of file entities from binary data via REST requests to fix it. But this then goes to show that it doesn't work at all.

@vaplas, do you want to re-RTBC? :)

Hm, all I did was descope one change in #107, and then update the tests accordingly in #109. That's no significant change.

This patch has been ready for such a long time already, and is doing two things only:

1. updating FileAccessControlHandler to require the access content permission to view files.
   This has been agreed upon for ages.
2. adding FileResourceTestBase + subclasses for comprehensive File REST test coverage, which unblocks #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field, which in turn unblocks #1927648: Allow creation of file entities from binary data via REST requests. There's nothing to agree about here: it's merely testing the status quo, so that we don't accidentally break BC!

So given all that, it's okay for me to re-RTBC.

+++ b/core/modules/file/src/FileAccessControlHandler.php
@@ -22,8 +22,12 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
       if (\Drupal::service('file_system')->uriScheme($entity->getFileUri()) === 'public') {
-        // Always allow access to file in public file system.
-        return AccessResult::allowed();
+        if ($operation === 'download') {
+          return AccessResult::allowed();
+        }
+        else {
+          return AccessResult::allowedIfHasPermission($account, 'access content');
+        }

I still think this is wrong simply because it can't be enforced. Reality is that anyone can access public files, you can't prevent it by removing that permission, so it is at least misleading and implies functionality that doesn't exist.

You could make a difference between download and view, which would likely make your rest tests happy, but at least in core, it's not consistently used and there is no real difference between the two (and file_entity makes an even bigger mess of it ;))

You could make a difference between download and view, which would likely make your rest tests happy, but at least in core […]

… that is exactly what the patch is doing?

[…] implies functionality that doesn't exist.

You mean you can't read the contents of a File entity with just core? That's true. The REST module is the first to add that. Which is exactly why this issue is the first one to raise the need to be able to restrict access to File entities' data. Without this, it is public knowledge that https://www.drupal.org/project/jsonapi allows any user to see every single File entity that exists, which can lead to information disclosure.

Ensuring no entity type has information disclosures by default is exactly what \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::testGet() is testing generically. And which is why we need to make this change here.

[…] but at least in core, it's not consistently used and there is no real difference between the two

Those are then yet more bugs elsewhere in core. We can fix those. That shouldn't prevent us from making this pragmatic, minimal step forward, right?

Right, sorry, I misread that about download/view access.

So we have two tests that have to be changed here:

* FileManagedAccessTest: Because it specifically checks access for both view and download. That makes sense, nothing else in core does that, so theoretically we could also update the test to more expliictly check the if/else, but fine.
* FileItemTest: Because without that permission, saving of the file entity fails. This could have *very* theoretical BC breaks if someone e.g. allowed someone to upload files on a custom entity that did not require the access content permission, but Wim correctly pointed out that several things would not work without that anyway, e.g. file upload progress. So it is extremely unlikely that anyone has this.

So.. lets do this, it's time to finally fix #1927648: Allow creation of file entities from binary data via REST requests.

YAYYYYYY!

🎉

Thanks, @Berdir.

Ok! Wim pinged me and we stepped through this patch together.

We spent a lot of time talking about the hunk @Berdir highlighted in #112. Unlike other aspects of this patch, which are specific to the REST system, this one touches the generic File entity and therefore has broader implications. However, trying to come up with a real, actual use case of a site that uses the public file system, and does not use nodes, and does not assign users "access content" permissions is pretty difficult to do. (Whereas I can definitely think of such use cases using the private file system, and have built at least one in the past. :)) And as Wim pointed out, this is the least bad of all possible options, the others being leave files completely open and insecure, or disallow REST access to files altogether.

The other hunk that stood out was:

+        // @todo Remove this in https://www.drupal.org/node/2824851.
+        $this->grantPermissionsToTestedRole(['access content']);

But this is already linked off to a sub-issue, the dependency of which is also RTBC currently, so hopefully this won't be around for too long.

Otherwise, this looks like a really solid step for the API-First initiative. I'm actually really impressed with how the tests are structured, so that we can test various permutations (anon + hal, anon + json + cookie auth, etc.) with relatively few lines of code.

This is an important patch to establish a baseline for when we do ultimately fix the ability to deal with files in REST, so while the couple of $this->markTestSkipped(); lines feel suboptimal, they also feel necessary to move forward.

Committed and pushed to 8.5.x, officially bringing REST's entity test coverage to 100%. ROCK! \m/

Also, tagging for a release notes mention (given the change to FileAccessControlHandler::checkAccess()), and also a highlights mention (for the 100% entity REST test coverage :)).

I think this could also benefit from a change record, so I'll work on that next.

Here's a draft change record: https://www.drupal.org/node/2927918

Haha, wow! That's the most epic 'thank you' I've ever gotten for a Drupal core commit. 😁 Thanks right back to you, vaplas!

YAY!

1. #2824572: Write EntityResourceTestBase subclasses for every other entity type. is now marked fixed too.
2. #2910883: Move all entity type REST tests to the providing modules is unblocked now, because we finally can say that each module providing an entity type should respect the API-First nature of Drupal 8.
3. Perhaps most importantly, #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field is now unblocked too. Which is also the last blocker for #1927648: Allow creation of file entities from binary data via REST requests.

For the record, this *did* break tests in token module which was uploading user pictures without having node module installed/access content .
See #2929240: Drupal\token\Tests\TokenUserTest::testUserTokens fails in dev branch.

Given #124, let's publish the CR in #119? Before doing so, can someone who's worked on this issue confirm its accuracy? Thanks.

Published that, looks good to me.