Issue #2078473 by blueminds | Berdir: Added Use entity access API for checking...

Issue #2078473 by blueminds | Berdir: Added Use entity access API for checking access to private files.

core/modules/comment/comment.module

@@ -1027,18 +1027,3 @@ function comment_preprocess_field(&$variables) {
 function comment_ranking() {
   return \Drupal::service('comment.statistics')->getRankingInfo();
 }
-
-/**
- * Implements hook_file_download_access().
- */
-function comment_file_download_access($field, EntityInterface $entity, FileInterface $file) {
-  if ($entity instanceof CommentInterface) {
-    $current_user = \Drupal::currentUser();
-    if ($current_user->hasPermission('access comments') && $entity->isPublished() || $current_user->hasPermission('administer comments')) {
-      $commented_entity = $entity->getCommentedEntity();
-      // Check access to parent entity.
-      return $commented_entity->access('view');
-    }
-    return FALSE;
-  }
-}

core/modules/comment/src/CommentAccessController.php

@@ -25,11 +25,13 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
     /** @var \Drupal\Core\Entity\EntityInterface|\Drupal\user\EntityOwnerInterface $entity */
     switch ($operation) {
       case 'view':
-        return $account->hasPermission('access comments');
+        if ($account->hasPermission('access comments') && $entity->isPublished() || $account->hasPermission('administer comments')) {
+          return $entity->getCommentedEntity()->access($operation, $account);
+        }
         break;
 
       case 'update':
-        return ($account->id() && $account->id() == $entity->getOwnerId() && $entity->status->value == CommentInterface::PUBLISHED && $account->hasPermission('edit own comments')) || $account->hasPermission('administer comments');
+        return ($account->id() && $account->id() == $entity->getOwnerId() && $entity->isPublished() && $account->hasPermission('edit own comments')) || $account->hasPermission('administer comments');
         break;
 
       case 'delete':

core/modules/file/file.api.php

@@ -77,61 +77,6 @@ function hook_file_move(Drupal\file\FileInterface $file, Drupal\file\FileInterfa
   }
 }
 
-/**
- * Control download access to files.
- *
- * The hook is typically implemented to limit access based on the entity that
- * references the file; for example, only users with access to a node should be
- * allowed to download files attached to that node.
- *
- * @param $field
- *   The field to which the file belongs.
- * @param \Drupal\Core\Entity\EntityInterface $entity
- *   The entity which references the file.
- * @param \Drupal\file\FileInterface $file
- *   The file entity that is being requested.
- *
- * @return
- *   TRUE is access should be allowed by this entity or FALSE if denied. Note
- *   that denial may be overridden by another entity controller, making this
- *   grant permissive rather than restrictive.
- *
- * @see hook_entity_field_access().
- */
-function hook_file_download_access($field, Drupal\Core\Entity\EntityInterface $entity, Drupal\file\FileInterface $file) {
-  if ($entity->getEntityTypeId() == 'node') {
-    return $entity->access('view');
-  }
-}
-
-/**
- * Alter the access rules applied to a file download.
- *
- * Entities that implement file management set the access rules for their
- * individual files. Module may use this hook to create custom access rules
- * for file downloads.
- *
- * @see hook_file_download_access().
- *
- * @param $grants
- *   An array of grants gathered by hook_file_download_access(). The array is
- *   keyed by the module that defines the entity type's access control; the
- *   values are Boolean grant responses for each module.
- * @param array $context
- *   An associative array containing the following key-value pairs:
- *   - field: The field to which the file belongs.
- *   - entity: The entity which references the file.
- *   - file: The file entity that is being requested.
- *
- * @see hook_file_download_access().
- */
-function hook_file_download_access_alter(&$grants, $context) {
-  // For our example module, we always enforce the rules set by node module.
-  if (isset($grants['node'])) {
-    $grants = array('node' => $grants['node']);
-  }
-}
-
 /**
  * @} End of "addtogroup hooks".
  */

core/modules/file/file.module

@@ -604,13 +604,8 @@ function file_theme() {
 
 /**
  * Implements hook_file_download().
- *
- * This function takes an extra parameter $field_type so that it may
- * be re-used by other File-like modules, such as Image.
  */
-function file_file_download($uri, $field_type = 'file') {
-  $user = \Drupal::currentUser();
-
+function file_file_download($uri) {
   // Get the file record based on the URI. If not in the database just return.
   /** @var \Drupal\file\FileInterface[] $files */
   $files = entity_load_multiple_by_properties('file', array('uri' => $uri));
@@ -629,69 +624,18 @@ function file_file_download($uri, $field_type = 'file') {
   }
 
   // Find out which (if any) fields of this type contain the file.
-  $references = file_get_file_references($file, NULL, EntityStorageInterface::FIELD_LOAD_CURRENT, $field_type);
+  $references = file_get_file_references($file, NULL, EntityStorageInterface::FIELD_LOAD_CURRENT, NULL);
 
   // Stop processing if there are no references in order to avoid returning
   // headers for files controlled by other modules. Make an exception for
   // temporary files where the host entity has not yet been saved (for example,
   // an image preview on a node/add form) in which case, allow download by the
   // file's owner.
-  if (empty($references) && ($file->isPermanent() || $file->getOwnerId() != $user->id())) {
+  if (empty($references) && ($file->isPermanent() || $file->getOwnerId() != \Drupal::currentUser()->id())) {
     return;
   }
 
-  // Default to allow access.
-  $denied = FALSE;
-  // Loop through all references of this file. If a reference explicitly allows
-  // access to the field to which this file belongs, no further checks are done
-  // and download access is granted. If a reference denies access, eventually
-  // existing additional references are checked. If all references were checked
-  // and no reference denied access, access is granted as well. If at least one
-  // reference denied access, access is denied.
-  foreach ($references as $field_name => $field_references) {
-    foreach ($field_references as $entity_type => $entities) {
-      $field_storage_definitions = \Drupal::entityManager()->getFieldStorageDefinitions($entity_type);
-      $field_storage = $field_storage_definitions[$field_name];
-      foreach ($entities as $entity) {
-        // Check if access to this field is not disallowed.
-        if (!$entity->get($field_name)->access('view')) {
-          $denied = TRUE;
-          continue;
-        }
-
-        // Invoke hook and collect grants/denies for download access.
-        // Default to FALSE and let entities overrule this ruling.
-        $grants = array('system' => FALSE);
-        foreach (\Drupal::moduleHandler()->getImplementations('file_download_access') as $module) {
-          $grants = array_merge($grants, array($module => \Drupal::moduleHandler()->invoke($module, 'file_download_access', array($field_storage, $entity, $file))));
-        }
-        // Allow other modules to alter the returned grants/denies.
-        $context = array(
-          'entity' => $entity,
-          'field' => $field_storage,
-          'file' => $file,
-        );
-        \Drupal::moduleHandler()->alter('file_download_access', $grants, $context);
-
-        if (in_array(TRUE, $grants)) {
-          // If TRUE is returned, access is granted and no further checks are
-          // necessary.
-          $denied = FALSE;
-          break 3;
-        }
-
-        if (in_array(FALSE, $grants)) {
-          // If an implementation returns FALSE, access to this entity is denied
-          // but the file could belong to another entity to which the user might
-          // have access. Continue with these.
-          $denied = TRUE;
-        }
-      }
-    }
-  }
-
-  // Access specifically denied.
-  if ($denied) {
+  if (!$file->access('download')) {
     return -1;
   }
 

core/modules/file/src/Entity/File.php

@@ -23,6 +23,7 @@
  *   label = @Translation("File"),
  *   controllers = {
  *     "storage" = "Drupal\file\FileStorage",
+ *     "access" = "Drupal\file\FileAccessController",
  *     "view_builder" = "Drupal\Core\Entity\EntityViewBuilder"
  *   },
  *   base_table = "file_managed",

core/modules/file/src/FileAccessController.php

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\file\FileAccessController.
+ */
+
+namespace Drupal\file;
+
+use Drupal\Core\Entity\EntityAccessController;
+use Drupal\Core\Entity\EntityInterface;
+use Drupal\Core\Entity\EntityStorageInterface;
+use Drupal\Core\Session\AccountInterface;
+
+/**
+ * Provides a File access controller.
+ */
+class FileAccessController extends EntityAccessController {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function checkAccess(EntityInterface $entity, $operation, $langcode, AccountInterface $account) {
+
+    if ($operation == 'download') {
+      foreach ($this->getFileReferences($entity) as $field_name => $entity_map) {
+        foreach ($entity_map as $referencing_entity_type => $referencing_entities) {
+          /** @var \Drupal\Core\Entity\EntityInterface $referencing_entity */
+          foreach ($referencing_entities as $referencing_entity) {
+            if ($referencing_entity->access('view', $account) && $referencing_entity->$field_name->access('view', $account)) {
+              return TRUE;
+            }
+          }
+        }
+      }
+
+      return FALSE;
+    }
+  }
+
+  /**
+   * Wrapper for file_get_file_references().
+   *
+   * @param \Drupal\file\FileInterface $file
+   *   The file object for which to get references.
+   *
+   * @return array
+   *   A multidimensional array. The keys are field_name, entity_type,
+   *   entity_id and the value is an entity referencing this file.
+   *
+   * @see file_get_file_references()
+   */
+  protected function getFileReferences(FileInterface $file) {
+    return file_get_file_references($file, NULL, EntityStorageInterface::FIELD_LOAD_REVISION, NULL);
+  }
+
+}

core/modules/file/src/Tests/FilePrivateTest.php

@@ -6,6 +6,8 @@
  */
 
 namespace Drupal\file\Tests;
+use Drupal\file\Entity\File;
+use Drupal\node\Entity\Node;
 
 /**
  * Uploads a test to a private node and checks access.
@@ -55,9 +57,13 @@ function testPrivateFile() {
     // Test with the field that should deny access through field access.
     $this->drupalLogin($this->admin_user);
     $nid = $this->uploadNodeFile($test_file, $no_access_field_name, $type_name, TRUE, array('private' => TRUE));
-    $node = node_load($nid, TRUE);
-    $node_file = file_load($node->{$no_access_field_name}->target_id);
+    \Drupal::entityManager()->getStorage('node')->resetCache(array($nid));
+    $node = Node::load($nid);
+    $node_file = File::load($node->{$no_access_field_name}->target_id);
+
     // Ensure the file cannot be downloaded.
+    $user = $this->drupalCreateUser(array('access content'));
+    $this->drupalLogin($user);
     $this->drupalGet(file_create_url($node_file->getFileUri()));
     $this->assertResponse(403, 'Confirmed that access is denied for the file without view field access permission.');
   }

core/modules/file/tests/file_module_test/file_module_test.module

-<?php
-
-/**
- * @file
- * Provides File module pages for testing purposes.
- */
-
-use Drupal\Core\Entity\EntityInterface;
-use Drupal\file\Entity\File;
-use Drupal\field\FieldStorageConfigInterface;
-
-/**
- * Implements hook_file_download_access().
- */
-function file_module_test_file_download_access(FieldStorageConfigInterface $field_storage, EntityInterface $entity, File $file) {
-  $field_definitions = \Drupal::entityManager()->getFieldDefinitions($entity->getEntityTypeId(), $entity->bundle());
-  // Allow the file to be downloaded only if the given arguments are correct.
-  if (empty($field_definitions[$field_storage->getName()])) {
-    return FALSE;
-  }
-}

core/modules/image/image.module

@@ -205,17 +205,6 @@ function image_file_download($uri) {
     }
     return -1;
   }
-
-  // Private file access for the original files. Note that we only
-  // check access for non-temporary images, since file.module will
-  // grant access for all temporary files.
-  $files = entity_load_multiple_by_properties('file', array('uri' => $uri));
-  if (count($files)) {
-    $file = reset($files);
-    if ($file->status) {
-      return file_file_download($uri, 'image');
-    }
-  }
 }
 
 /**

core/modules/node/node.module

@@ -1599,15 +1599,6 @@ function node_modules_uninstalled($modules) {
   }
 }
 
-/**
- * Implements hook_file_download_access().
- */
-function node_file_download_access($field, EntityInterface $entity, File $file) {
-  if ($entity->getEntityTypeId() == 'node') {
-    return $entity->access('view');
-  }
-}
-
 /**
  * Implements hook_ENTITY_TYPE_delete() for 'language_entity'.
  */

core/modules/taxonomy/taxonomy.module

@@ -664,15 +664,6 @@ function taxonomy_term_load($tid) {
   return Term::load($tid);
 }
 
-/**
- * Implements hook_file_download_access().
- */
-function taxonomy_file_download_access($field, EntityInterface $entity, FileInterface $file) {
-  if ($entity->getEntityTypeId() == 'taxonomy_term') {
-    return $entity->access('view');
-  }
-}
-
 /**
  * Implodes a list of tags of a certain vocabulary into a string.
  *

core/modules/user/user.module

@@ -1489,15 +1489,6 @@ function user_cookie_delete($cookie_name) {
   setrawcookie('Drupal.visitor.' . $cookie_name, '', REQUEST_TIME - 3600, '/');
 }
 
-/**
- * Implements hook_file_download_access().
- */
-function user_file_download_access($field, EntityInterface $entity, File $file) {
-  if ($entity->getEntityTypeId() == 'user') {
-    return $entity->access('view');
-  }
-}
-
 /**
  * Implements hook_toolbar().
  */