From e8b84d93fffa0ef8f2730b13cc41af2ae0d0db0a Mon Sep 17 00:00:00 2001 From: webchick Date: Fri, 1 Dec 2017 11:35:08 -0800 Subject: [PATCH] Issue #2843139 by vaplas, Wim Leers, harings_rob, catch, Berdir, alexpott: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler --- .../src/Tests/String/StringFieldTest.php | 2 +- .../file/src/FileAccessControlHandler.php | 8 +- .../src/Functional/FileManagedAccessTest.php | 18 +- .../file/tests/src/Kernel/FileItemTest.php | 9 + .../File/FileHalJsonAnonTest.php | 111 +++++++++ .../File/FileHalJsonBasicAuthTest.php | 24 ++ .../File/FileHalJsonCookieTest.php | 19 ++ .../image/tests/src/Kernel/ImageItemTest.php | 9 + .../EntityResourceRestTestCoverageTest.php | 9 - .../EntityResource/File/FileJsonAnonTest.php | 24 ++ .../File/FileJsonBasicAuthTest.php | 34 +++ .../File/FileJsonCookieTest.php | 29 +++ .../File/FileResourceTestBase.php | 227 ++++++++++++++++++ .../EntityResource/File/FileXmlAnonTest.php | 26 ++ .../File/FileXmlBasicAuthTest.php | 36 +++ .../EntityResource/File/FileXmlCookieTest.php | 31 +++ .../Media/MediaResourceTestBase.php | 2 + 17 files changed, 604 insertions(+), 14 deletions(-) create mode 100644 core/modules/hal/tests/src/Functional/EntityResource/File/FileHalJsonAnonTest.php create mode 100644 core/modules/hal/tests/src/Functional/EntityResource/File/FileHalJsonBasicAuthTest.php create mode 100644 core/modules/hal/tests/src/Functional/EntityResource/File/FileHalJsonCookieTest.php create mode 100644 core/modules/rest/tests/src/Functional/EntityResource/File/FileJsonAnonTest.php create mode 100644 core/modules/rest/tests/src/Functional/EntityResource/File/FileJsonBasicAuthTest.php create mode 100644 core/modules/rest/tests/src/Functional/EntityResource/File/FileJsonCookieTest.php create mode 100644 core/modules/rest/tests/src/Functional/EntityResource/File/FileResourceTestBase.php create mode 100644 core/modules/rest/tests/src/Functional/EntityResource/File/FileXmlAnonTest.php create mode 100644 core/modules/rest/tests/src/Functional/EntityResource/File/FileXmlBasicAuthTest.php create mode 100644 core/modules/rest/tests/src/Functional/EntityResource/File/FileXmlCookieTest.php diff --git a/core/modules/field/src/Tests/String/StringFieldTest.php b/core/modules/field/src/Tests/String/StringFieldTest.php index 97e470e92b9..fdedb79907a 100644 --- a/core/modules/field/src/Tests/String/StringFieldTest.php +++ b/core/modules/field/src/Tests/String/StringFieldTest.php @@ -32,7 +32,7 @@ class StringFieldTest extends WebTestBase { protected function setUp() { parent::setUp(); - $this->webUser = $this->drupalCreateUser(['view test entity', 'administer entity_test content']); + $this->webUser = $this->drupalCreateUser(['view test entity', 'administer entity_test content', 'access content']); $this->drupalLogin($this->webUser); } diff --git a/core/modules/file/src/FileAccessControlHandler.php b/core/modules/file/src/FileAccessControlHandler.php index f4bda3cb7d2..e378b648dfb 100644 --- a/core/modules/file/src/FileAccessControlHandler.php +++ b/core/modules/file/src/FileAccessControlHandler.php @@ -22,8 +22,12 @@ class FileAccessControlHandler extends EntityAccessControlHandler { /** @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 ($operation === 'download') { + return AccessResult::allowed(); + } + else { + return AccessResult::allowedIfHasPermission($account, 'access content'); + } } elseif ($references = $this->getFileReferences($entity)) { foreach ($references as $field_name => $entity_map) { diff --git a/core/modules/file/tests/src/Functional/FileManagedAccessTest.php b/core/modules/file/tests/src/Functional/FileManagedAccessTest.php index 01419d44630..c62b1cbf160 100644 --- a/core/modules/file/tests/src/Functional/FileManagedAccessTest.php +++ b/core/modules/file/tests/src/Functional/FileManagedAccessTest.php @@ -3,6 +3,7 @@ namespace Drupal\Tests\file\Functional; use Drupal\file\Entity\File; +use Drupal\user\Entity\Role; /** * Tests access to managed files. @@ -11,6 +12,19 @@ use Drupal\file\Entity\File; */ class FileManagedAccessTest extends FileManagedTestBase { + /** + * {@inheritdoc} + */ + protected function setUp() { + parent::setUp(); + + // Give anonymous users permission to access content, so they can view and + // download public files. + $anonymous_role = Role::load(Role::ANONYMOUS_ID); + $anonymous_role->grantPermission('access content'); + $anonymous_role->save(); + } + /** * Tests if public file is always accessible. */ @@ -29,7 +43,7 @@ class FileManagedAccessTest extends FileManagedTestBase { $file->save(); // Create authenticated user to check file access. - $account = $this->createUser(['access site reports']); + $account = $this->createUser(['access site reports', 'access content']); $this->assertTrue($file->access('view', $account), 'Public file is viewable to authenticated user'); $this->assertTrue($file->access('download', $account), 'Public file is downloadable to authenticated user'); @@ -54,7 +68,7 @@ class FileManagedAccessTest extends FileManagedTestBase { $file->save(); // Create authenticated user to check file access. - $account = $this->createUser(['access site reports']); + $account = $this->createUser(['access site reports', 'access content']); $this->assertFalse($file->access('view', $account), 'Private file is not viewable to authenticated user'); $this->assertFalse($file->access('download', $account), 'Private file is not downloadable to authenticated user'); diff --git a/core/modules/file/tests/src/Kernel/FileItemTest.php b/core/modules/file/tests/src/Kernel/FileItemTest.php index 8219af15d9c..ac6cd8899fe 100644 --- a/core/modules/file/tests/src/Kernel/FileItemTest.php +++ b/core/modules/file/tests/src/Kernel/FileItemTest.php @@ -10,6 +10,7 @@ use Drupal\field\Entity\FieldConfig; use Drupal\Tests\field\Kernel\FieldKernelTestBase; use Drupal\field\Entity\FieldStorageConfig; use Drupal\file\Entity\File; +use Drupal\user\Entity\Role; /** * Tests using entity fields of the file field type. @@ -42,6 +43,14 @@ class FileItemTest extends FieldKernelTestBase { protected function setUp() { parent::setUp(); + $this->installEntitySchema('user'); + $this->installConfig(['user']); + // Give anonymous users permission to access content, so they can view and + // download public files. + $anonymous_role = Role::load(Role::ANONYMOUS_ID); + $anonymous_role->grantPermission('access content'); + $anonymous_role->save(); + $this->installEntitySchema('file'); $this->installSchema('file', ['file_usage']); diff --git a/core/modules/hal/tests/src/Functional/EntityResource/File/FileHalJsonAnonTest.php b/core/modules/hal/tests/src/Functional/EntityResource/File/FileHalJsonAnonTest.php new file mode 100644 index 00000000000..ff89f745903 --- /dev/null +++ b/core/modules/hal/tests/src/Functional/EntityResource/File/FileHalJsonAnonTest.php @@ -0,0 +1,111 @@ +applyHalFieldNormalization($default_normalization); + + $url = file_create_url($this->entity->getFileUri()); + $normalization['uri'][0]['value'] = $url; + $uid = $this->author->id(); + + return $normalization + [ + '_embedded' => [ + $this->baseUrl . '/rest/relation/file/file/uid' => [ + [ + '_links' => [ + 'self' => [ + 'href' => $this->baseUrl . "/user/$uid?_format=hal_json", + ], + 'type' => [ + 'href' => $this->baseUrl . '/rest/type/user/user', + ], + ], + 'uuid' => [ + [ + 'value' => $this->author->uuid(), + ], + ], + ], + ], + ], + '_links' => [ + 'self' => [ + 'href' => $url, + ], + 'type' => [ + 'href' => $this->baseUrl . '/rest/type/file/file', + ], + $this->baseUrl . '/rest/relation/file/file/uid' => [ + [ + 'href' => $this->baseUrl . "/user/$uid?_format=hal_json", + ], + ], + ], + ]; + } + + /** + * {@inheritdoc} + */ + protected function getNormalizedPostEntity() { + return parent::getNormalizedPostEntity() + [ + '_links' => [ + 'type' => [ + 'href' => $this->baseUrl . '/rest/type/file/file', + ], + ], + ]; + } + + /** + * {@inheritdoc} + */ + protected function getExpectedCacheContexts() { + return [ + 'url.site', + 'user.permissions', + ]; + } + + /** + * {@inheritdoc} + */ + public function testPatch() { + // @todo https://www.drupal.org/node/1927648 + $this->markTestSkipped(); + } + +} diff --git a/core/modules/hal/tests/src/Functional/EntityResource/File/FileHalJsonBasicAuthTest.php b/core/modules/hal/tests/src/Functional/EntityResource/File/FileHalJsonBasicAuthTest.php new file mode 100644 index 00000000000..3b1e3fe6149 --- /dev/null +++ b/core/modules/hal/tests/src/Functional/EntityResource/File/FileHalJsonBasicAuthTest.php @@ -0,0 +1,24 @@ +installEntitySchema('user'); + $this->installConfig(['user']); + // Give anonymous users permission to access content, so that we can view + // and download public file. + $anonymous_role = Role::load(Role::ANONYMOUS_ID); + $anonymous_role->grantPermission('access content'); + $anonymous_role->save(); + $this->installEntitySchema('file'); $this->installSchema('file', ['file_usage']); diff --git a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceRestTestCoverageTest.php b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceRestTestCoverageTest.php index e26ecd2c5cf..d10d55c245b 100644 --- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceRestTestCoverageTest.php +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceRestTestCoverageTest.php @@ -124,15 +124,6 @@ class EntityResourceRestTestCoverageTest extends BrowserTestBase { } $all = count($this->definitions); $good = $all - count($problems); - // @todo Remove this in https://www.drupal.org/node/2843139. Having this - // work-around in here until then means we can ensure we don't add more - // entity types without adding REST test coverage. - if ($problems === ['file: File (Drupal\file\Entity\File), default normalization (expected tests: FileJsonAnonTest, FileJsonBasicAuthTest, FileJsonCookieTest, FileXmlAnonTest, FileXmlBasicAuthTest, FileXmlCookieTest)', 'file: File (Drupal\file\Entity\File), hal normalization (expected tests: FileHalJsonAnonTest, FileHalJsonBasicAuthTest, FileHalJsonCookieTest)']) { - $problems = []; - } - elseif ($problems === []) { - $this->fail('Drupal\file\Entity\File now supports REST test coverage. The work-around for it to pass until that test coverage was added should now be removed.'); - } $this->assertSame([], $problems, $this->getLlamaMessage($good, $all)); } diff --git a/core/modules/rest/tests/src/Functional/EntityResource/File/FileJsonAnonTest.php b/core/modules/rest/tests/src/Functional/EntityResource/File/FileJsonAnonTest.php new file mode 100644 index 00000000000..b3adf6e433c --- /dev/null +++ b/core/modules/rest/tests/src/Functional/EntityResource/File/FileJsonAnonTest.php @@ -0,0 +1,24 @@ +grantPermissionsToTestedRole(['access content']); + break; + + case 'PATCH': + case 'DELETE': + // \Drupal\file\FileAccessControlHandler::checkAccess() grants 'update' + // and 'delete' access only to the user that owns the file. So there is + // no permission to grant: instead, the file owner must be changed from + // its default (user 1) to the current user. + $this->makeCurrentUserFileOwner(); + break; + } + } + + /** + * {@inheritdoc} + */ + protected function grantPermissionsToTestedRole(array $permissions) { + // testPatch() and testDelete() test the 'bc_entity_resource_permissions' BC + // layer; also call makeCurrentUserFileOwner() then. + if ($permissions === ['restful patch entity:file'] || $permissions === ['restful delete entity:file']) { + $this->makeCurrentUserFileOwner(); + } + parent::grantPermissionsToTestedRole($permissions); + } + + /** + * Makes the current user the file owner. + */ + protected function makeCurrentUserFileOwner() { + $account = static::$auth ? User::load(2) : User::load(0); + $this->entity->setOwnerId($account->id()); + $this->entity->setOwner($account); + $this->entity->save(); + } + + /** + * {@inheritdoc} + */ + protected function createEntity() { + $this->author = User::load(1); + + $file = File::create(); + $file->setOwnerId($this->author->id()); + $file->setFilename('drupal.txt'); + $file->setMimeType('text/plain'); + $file->setFileUri('public://drupal.txt'); + $file->set('status', FILE_STATUS_PERMANENT); + $file->save(); + + file_put_contents($file->getFileUri(), 'Drupal'); + + return $file; + } + + /** + * {@inheritdoc} + */ + protected function getExpectedNormalizedEntity() { + return [ + 'changed' => [ + $this->formatExpectedTimestampItemValues($this->entity->getChangedTime()), + ], + 'created' => [ + $this->formatExpectedTimestampItemValues((int) $this->entity->getCreatedTime()), + ], + 'fid' => [ + [ + 'value' => 1, + ], + ], + 'filemime' => [ + [ + 'value' => 'text/plain', + ], + ], + 'filename' => [ + [ + 'value' => 'drupal.txt', + ], + ], + 'filesize' => [ + [ + 'value' => (int) $this->entity->getSize(), + ], + ], + 'langcode' => [ + [ + 'value' => 'en', + ], + ], + 'status' => [ + [ + 'value' => TRUE, + ], + ], + 'uid' => [ + [ + 'target_id' => (int) $this->author->id(), + 'target_type' => 'user', + 'target_uuid' => $this->author->uuid(), + 'url' => base_path() . 'user/' . $this->author->id(), + ], + ], + 'uri' => [ + [ + 'value' => 'public://drupal.txt', + ], + ], + 'uuid' => [ + [ + 'value' => $this->entity->uuid(), + ], + ], + ]; + } + + /** + * {@inheritdoc} + */ + protected function getNormalizedPostEntity() { + return [ + 'uid' => [ + [ + 'target_id' => $this->author->id(), + ], + ], + 'filename' => [ + [ + 'value' => 'drupal.txt', + ], + ], + ]; + } + + /** + * {@inheritdoc} + */ + protected function getNormalizedPatchEntity() { + return array_diff_key($this->getNormalizedPostEntity(), ['uid' => TRUE]); + } + + /** + * {@inheritdoc} + */ + protected function getExpectedCacheContexts() { + return [ + 'user.permissions', + ]; + } + + /** + * {@inheritdoc} + */ + public function testPost() { + // @todo https://www.drupal.org/node/1927648 + $this->markTestSkipped(); + } + + /** + * {@inheritdoc} + */ + protected function getExpectedUnauthorizedAccessMessage($method) { + if ($this->config('rest.settings')->get('bc_entity_resource_permissions')) { + return parent::getExpectedUnauthorizedAccessMessage($method); + } + + if ($method === 'GET') { + return "The 'access content' permission is required."; + } + if ($method === 'PATCH') { + return 'You are not authorized to update this file entity.'; + } + return parent::getExpectedUnauthorizedAccessMessage($method); + } + +} diff --git a/core/modules/rest/tests/src/Functional/EntityResource/File/FileXmlAnonTest.php b/core/modules/rest/tests/src/Functional/EntityResource/File/FileXmlAnonTest.php new file mode 100644 index 00000000000..2fad0870b09 --- /dev/null +++ b/core/modules/rest/tests/src/Functional/EntityResource/File/FileXmlAnonTest.php @@ -0,0 +1,26 @@ +grantPermissionsToTestedRole(['update any media']); + // @todo Remove this in https://www.drupal.org/node/2824851. + $this->grantPermissionsToTestedRole(['access content']); break; case 'DELETE':