Problem/Motivation

Please refer the problem/motivation section of #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method

This test coverage uncovered a security problem: it's impossible to deploy the entity:file REST resource without allowing anybody (even anonymous users) to view File entities in json, xml or hal_json format.

Proposed resolution

Write EntityResourceTestBase subclass for the File entity, and to address the security problem in a non-invasive way (in a way that is not a BC break in practice), we the File entity's view operation now requires the access content permission.

Remaining tasks

  1. Build consensus, specifically about using the access content permission to be allowed to view File entities.
  2. #2892821: Core modules check node module's "access content" permission for displaying things that have nothing to do with nodes
  3. Green patch.
  4. RTBC.

References

1. Follow-up of #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method
2. Subtask of #2824572: Write EntityResourceTestBase subclasses for every other entity type.

CommentFileSizeAuthor
#121 strong-chord-from-webchick.png379.04 KBvaplas
#109 2843139-109.patch24.12 KBWim Leers
#109 interdiff.txt741 bytesWim Leers
#107 2843139-107.patch23.97 KBWim Leers
#107 interdiff.txt1.41 KBWim Leers
#98 2843139-98.patch25.32 KBWim Leers
#98 interdiff.txt3.09 KBWim Leers
#96 interdiff-94-96.txt1.16 KBvaplas
#96 2843139-96.patch21.09 KBvaplas
#94 2843139-88.patch20.79 KBWim Leers
#88 2843139-88.patch20.79 KBWim Leers
#88 interdiff.txt849 bytesWim Leers
#84 2843139-84.patch20.01 KBWim Leers
#84 interdiff.txt4.13 KBWim Leers
#64 2843139-64.patch19.72 KBWim Leers
#64 interdiff.txt4.88 KBWim Leers
#58 interdiff-40-58.txt2.19 KBvaplas
#58 2843139-58.patch19.01 KBvaplas
#5 2843139.patch8.61 KBharings_rob
#6 get_and_node-2843139-6.patch12.22 KBvaplas
#6 get_patch_delete-2843139-6.patch14.76 KBvaplas
#11 fix_get_and_node.txt1.46 KBvaplas
#14 2843139-14.patch13.93 KBvaplas
#14 interdiff-6-14.txt7.12 KBvaplas
#15 interdiff.txt3.68 KBWim Leers
#15 2843139-15.patch15.25 KBWim Leers
#17 2843139-17.patch18.59 KBvaplas
#17 interdiff-15-17.txt14.85 KBvaplas
#19 2843139-19.txt17.92 KBvaplas
#19 interdiff-17-19.txt4.11 KBvaplas
#21 2843139-21.patch17.82 KBvaplas
#21 interdiff-19-21.txt1.06 KBvaplas
#23 2843139-23.patch17.83 KBvaplas
#23 interdiff-21-23.txt926 bytesvaplas
#34 interdiff.txt1.56 KBWim Leers
#34 2843139-34.patch18.32 KBWim Leers
#39 2843139-39.patch19.36 KBvaplas
#39 interdiff-34-39.txt2.93 KBvaplas
#40 2843139-40.patch18.62 KBvaplas
#40 interdiff-39-40.txt1.52 KBvaplas
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

naveenvalecha created an issue. See original summary.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

harings_rob’s picture

Assigned: Unassigned » harings_rob
Issue tags: +DevDaysSeville
harings_rob’s picture

Version: 8.3.x-dev » 8.4.x-dev
FileSize
8.61 KB

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.

vaplas’s picture

@harings_rob, thank you for draf, it is helpful! Unfortunately, the File does not look friendly to the Rest.

We cann't use POST, because FileAccessControlHandler::checkCreateAccess() constantly return the AccessResult::neutral() (see also #1927648: Allow creation of file entities from binary data via REST requests).

We cann't use public:// storage, because it always return 200 status. We can use trick with private:// and Node Field Access via reference, but only for GET (see patch get_and_node). But this leads to problems with accessing the file in HAL tests.

We can use with switch author of file, but it's looks extremely doubtful (see patch get_patch_delete). And PATCH will pass only after #2853211: EntityResource::post() incorrectly assumes that every entity type has a canonical URL.

PS. I added these patches only for the demonstration, and do not count on anyone's approval, of course :) If I do not mistake, we should first negotiate with the File module, and only then come back here :(

The last submitted patch, 6: get_and_node-2843139-6.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: get_patch_delete-2843139-6.patch, failed testing.

vaplas’s picture

'url' => '/checkout/user/3' on Bot's site, hm.. Both my patches are green localy. Bu I did not make them neat enough, sorry. Should I correct them, if they are not on the right way?

harings_rob’s picture

+++ 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?

vaplas’s picture

FileSize
1.46 KB

You absolutely right. Attached fix for get_and_node. For second patch we need same changes + #2853211-21: EntityResource::post() incorrectly assumes that every entity type has a canonical URL. But before taking care of these patches, I suggest waiting for hints about the right direction.

Wim Leers’s picture

Title: EntityResource: Provide comprehensive test coverage for File entity » [PP-1] EntityResource: Provide comprehensive test coverage for File entity
Status: Needs work » Postponed
Wim Leers’s picture

Title: [PP-1] EntityResource: Provide comprehensive test coverage for File entity » EntityResource: Provide comprehensive test coverage for File entity
Status: Postponed » Needs work
vaplas’s picture

Reroll variant with max test coverage (rest without POST, and hal without POST & PATCH). But the more I try to pass all the tests, the more looks like we need just to wait next issues:

What the problem with PATCH in hal? EntityResourceTestBase in 884 line returns 500 instead of 403:

$response = $this->request('PATCH', $url, $request_options);
$this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('PATCH'), $response);

With next body:

$response = $this->request('PATCH', $url, $request_options);
$this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('PATCH'), $response);

The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">GuzzleHttp\Exception\ClientException</em>: Client error: `GET http://d8x.dev/system/files/drupal.txt` resulted in a `403 Forbidden` response:
<!DOCTYPE html>
<html lang='en' dir='ltr'>
  <head>
    <meta charset='utf-8' />
<meta name='Generator' content='Drupal  (truncated...)
 in <em class="placeholder">GuzzleHttp\Exception\RequestException::create()</em> (line <em class="placeholder">111</em> of <em class="placeholder">vendor\guzzlehttp\guzzle\src\Exception\RequestException.php</em>). <pre class="backtrace">GuzzleHttp\Middleware::GuzzleHttp\{closure}(Object) (Line: 203)
GuzzleHttp\Promise\Promise::callHandler(1, Object, Array) (Line: 156)
GuzzleHttp\Promise\Promise::GuzzleHttp\Promise\{closure}() (Line: 47)
GuzzleHttp\Promise\TaskQueue->run(1) (Line: 246)
GuzzleHttp\Promise\Promise->invokeWaitFn() (Line: 223)
GuzzleHttp\Promise\Promise->waitIfPending() (Line: 267)
GuzzleHttp\Promise\Promise->invokeWaitList() (Line: 225)
GuzzleHttp\Promise\Promise->waitIfPending() (Line: 62)
GuzzleHttp\Promise\Promise->wait() (Line: 129)
GuzzleHttp\Client->request("get", "http://d8x.dev/system/files/drupal.txt", Array) (Line: 87)
GuzzleHttp\Client->__call("get", Array) (Line: 62)
Drupal\hal\Normalizer\FileEntityNormalizer->denormalize(Array, "Drupal\file\Entity\File", "hal_json", Array) (Line: 250)
Symfony\Component\Serializer\Serializer->denormalizeObject(Array, "Drupal\file\Entity\File", "hal_json", Array) (Line: 160)
Symfony\Component\Serializer\Serializer->denormalize(Array, "Drupal\file\Entity\File", "hal_json", Array) (Line: 114)
Drupal\rest\RequestHandler->handle(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array) (Line: 144)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 64)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 656)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
</pre>
Wim Leers’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 15: 2843139-15.patch, failed testing.

vaplas’s picture

Status: Needs work » Needs review
FileSize
18.59 KB
14.85 KB

Fantastic adjustment! Now I feel the ground under feet again !)

15.1: I just wanted to steal an your idea, but array_diff breaks down the sort and assertEquals do not sort by default. Of course, we can just use sort() after array_diff(), but after the #15 patch, we do not need to change anything at all :)

15.3: It is necessary for DELETE test, because this test call createEntity() too, but we can use random postfix for get unique user name instead of check $author.

Also after #15 patch, we haven't problem with GuzzleHttp\Exception\ClientException in PATCH in hal tests!

But now there is a problem with the processing _restSubmittedFields, because FileEntityNormalizer don't adding this property. I tried fix this problem + repaired a couple of foreign tests.

And 'administer files' to 'update/delete' operations.

Wim Leers’s picture

Status: Needs review » Needs work

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! :)

vaplas’s picture

18.5: yeah, I've gone a little too far with this :) This need to Drupal\rest\Plugin\rest\resource\EntityResource::patch() (here) and was like c/p from ContentEntityNormalizer::denormalize().

Wim Leers’s picture

+++ 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?

vaplas’s picture

COOL!

Wait, new changes:

-    $data['uri'] = file_unmanaged_save_data($file_data, $path);
+    $data['uri'] = [file_unmanaged_save_data($file_data, $path)];

array wrapper, why? Because parent call denormalizeFieldData() .. go-go-go .. if (!is_array($data)) in FieldNormalizer::denormalize() (here).

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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.

vaplas’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
17.83 KB
926 bytes

Drupal\Tests\hal\Functional\FileDenormalizeTest::testFileDenormalize() has case:

// Try to denormalize with the file uri only.
...
    $data = [
      'uri' => [
        ['value' => $file_uri],
      ],
    ];
    $denormalized = $serializer->denormalize($data, 'Drupal\file\Entity\File', 'hal_json');

But ContentEntityNormalizer::denormalize() need for '_links':

    // Get type, necessary for determining which bundle to create.
    if (!isset($data['_links']['type'])) {
      throw new UnexpectedValueException('The type link relation must be specified.');
    }

Therefore, I returned the old behavior for the case without _links :(

The last submitted patch, 21: 2843139-21.patch, failed testing.

The last submitted patch, 21: 2843139-21.patch, failed testing.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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.');
    }
damiankloip’s picture

Assigned: harings_rob » Unassigned

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.

Wim Leers’s picture

#27: that sounds great! :)

The last submitted patch, 21: 2843139-21.patch, failed testing.

The last submitted patch, 21: 2843139-21.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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).

vaplas’s picture

Title: EntityResource: Provide comprehensive test coverage for File entity » [PP-1] EntityResource: Provide comprehensive test coverage for File entity
Status: Needs work » Postponed
Related issues: +#2870018: Should the 'access content' permission be used to control access to viewing configuration entities via REST

Atm it feels like we're adding this to make the generic tests pass rather than for reasons of correctness.

Hah! Fair remark! But I can not agree with this, of course. Maybe my opinion is biased, but on the contrary, we patchinig holes in access logic through the passage of the rest-tests :)

31.2: Need to addCacheableDependency for all cases of FileAccessControlHandler::checkAccess(), or only in delete/update?

[PP-1] Postponed by #2870018: Should the 'access content' permission be used to control access to viewing configuration entities via REST + 31.2 point.

Wim Leers’s picture

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.
Wim Leers’s picture

Title: [PP-1] EntityResource: Provide comprehensive test coverage for File entity » EntityResource: Provide comprehensive test coverage for File entity
Status: Postponed » Needs review
FileSize
1.56 KB
18.32 KB

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.

xjm’s picture

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

alexpott’s picture

  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.

Wim Leers’s picture

Status: Needs review » Needs work

NW for #36.2.

Wim Leers’s picture

Title: EntityResource: Provide comprehensive test coverage for File entity » EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler
Category: Task » Bug report
Issue tags: +Security improvements

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?

vaplas’s picture

Status: Needs work » Needs review
FileSize
19.36 KB
2.93 KB

Yes, of course!)

#37 (#36.2) + #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX + bit change in formatExpectedTimestampItemValues for compatibility:

+++ b/core/modules/rest/tests/src/Functional/BcTimestampNormalizerUnixTestTrait.php
@@ -24,7 +24,7 @@
   protected function formatExpectedTimestampItemValues($timestamp) {
...
-      return ['value' => $timestamp];
+      return ['value' => (int) $timestamp];
vaplas’s picture

Or better convert immediately?

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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.

The last submitted patch, 40: 2843139-40.patch, failed testing.

Wim Leers’s picture

#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.

Wim Leers’s picture

Issue tags: +API-First Initiative
catch’s picture

+++ 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.

Wim Leers’s picture

#45: Do you have an alternative proposal?

catch’s picture

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.

Wim Leers’s picture

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.)

tstoeckler’s picture

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.

Wim Leers’s picture

#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?
tstoeckler’s picture

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.

Wim Leers’s picture

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.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review

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)?

vaplas’s picture

  • 53.1: If we have permission 'administer files', we can use it regardless of 'access content'. See also #2870018: Should the 'access content' permission be used to control access to viewing configuration entities via REST.
  • 53.2: Because it's a hole in managing files. How can a file moderator remove an incorrect file from another user without it?
  • 53.3: Yes, from the side it may seem that rest-tests solve all the problems and provide a test coverage for everything, but in fact their main goal is to make sure that entites support rest interface :) So, if 'administer files' is correct step, hence we need not to complicate the test (we have already tried this way in the beginning of the issue and it is quite uncomfortable)
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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

The last submitted patch, 40: 2843139-40.patch, failed testing. View results

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
vaplas’s picture

Rerolled after SA-CORE-2017-003.

NB: now uri is protected, therefore, an additional conditional wrapper in FileEntityNormalizer::denormalize(). The interdiff is made with -w flag for more readability of this change.

Saviktor’s picture

Issue tags: -Needs reroll

I could apply the patch, so no reroll is needed

Jo Fitzgerald’s picture

Wim Leers’s picture

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.

Wim Leers’s picture

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…

Wim Leers’s picture

Assigned: Unassigned » effulgentsia
Issue tags: +Needs framework manager review

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
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs framework manager review
FileSize
4.88 KB
19.72 KB

Discussed with @effulgentsia. We decided on choice 1.

vaplas’s picture

Looks awesome! This is much better than my #14.

Wim Leers’s picture

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

catch’s picture

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

Wim Leers’s picture

Glad to hear that!

Berdir’s picture

+++ 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?

vaplas’s picture

Fair remark. But these sites are most likely rare now. And for sure they will still use the 'node' module. Because without this module we haven't 'access content' permission, which is needed in many places drupal. Including file.ajax_progress (see point @dawehner's in #2870018-17: Should the 'access content' permission be used to control access to viewing configuration entities via REST).

In any case, the heroes who use drupal with super-specific configurations are probably already accustomed to a difficult fate :)

catch’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 64: 2843139-64.patch, failed testing. View results

vaplas’s picture

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

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

Wim Leers’s picture

❤️👏

Wim Leers’s picture

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 :)

Wim Leers’s picture

Title: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler » [PP-1] EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler
Status: Needs review » Postponed
Issue tags: -Novice
tedbow’s picture

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?

Wim Leers’s picture

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!

tedbow’s picture

#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.

Wim Leers’s picture

Priority: Normal » Major
Wim Leers’s picture

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.

Wim Leers’s picture

Title: [PP-1] EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler » [PP-2] EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler
Related issues: +#2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field
Wim Leers’s picture

Title: [PP-2] EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler » [PP-1] EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler
Status: Postponed » Needs review

Status: Needs review » Needs work

The last submitted patch, 84: 2843139-84.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
849 bytes
20.79 KB

#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.

Status: Needs review » Needs work

The last submitted patch, 88: 2843139-88.patch, failed testing. View results

Wim Leers’s picture

vaplas’s picture

Status: Needs work » Needs review

Epic news!

#89: unrelated fail, now green again.

Wim Leers’s picture

Assigned: effulgentsia » Unassigned
Status: Needs review » Postponed
Wim Leers’s picture

Title: [PP-1] EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler » EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler
Assigned: Unassigned » Wim Leers
Status: Postponed » Needs work
Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
FileSize
20.79 KB

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

Wim Leers’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

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.

vaplas’s picture

#88 super forecast! Now allowance for wonderful 'xjm-factor'!)

Wim Leers’s picture

👏😀

Wim Leers’s picture

#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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 98: 2843139-98.patch, failed testing. View results

vaplas’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 98: 2843139-98.patch, failed testing. View results

vaplas’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 98: 2843139-98.patch, failed testing. View results

vaplas’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 98: 2843139-98.patch, failed testing. View results

vaplas’s picture

Status: Needs work » Reviewed & tested by the community
Wim Leers’s picture

FileSize
1.41 KB
23.97 KB
--- 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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 107: 2843139-107.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
741 bytes
24.12 KB

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.

Wim Leers’s picture

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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.

Berdir’s picture

+++ 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 ;))

Wim Leers’s picture

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?

Berdir’s picture

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.

Wim Leers’s picture

YAYYYYYY!

🎉

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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/

  • webchick committed e8b84d9 on 8.5.x
    Issue #2843139 by vaplas, Wim Leers, harings_rob, catch, Berdir,...
webchick’s picture

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.

webchick’s picture

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

vaplas’s picture

🎉❤️🙏👏🎸🤘🌟!!!11

@webchick, you made my day! This is really a significant event, which strikes at uncontrolled access to the file entity. And, of course, allows us to win #2824572: Write EntityResourceTestBase subclasses for every other entity type.! What is the official clarity in accessing the entities! If it's not delicious, then what's delicious?!)

Thanks to everyone, and especially @Wim Leers!

#1927648: Allow creation of file entities from binary data via REST requests incredible work! 💪💪💪

vaplas’s picture

FileSize
379.04 KB

strong chord from webchick

webchick’s picture

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

Wim Leers’s picture

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.
Berdir’s picture

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.

effulgentsia’s picture

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

vaplas’s picture

#119 CR looks nice for me. Especially an example at the end of the record. As a non-native speaker, it immediately becomes clear to me what to do. 💎

Berdir’s picture

Published that, looks good to me.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.