Description of the bug

The docblock of that function in file.entity.inc states:

   * The "view" operation is not supported on the File Entity itself because it
   * has no page callback (yet), but there is separately file_download_access()
   * to check if a file is downloadable.

And accordingly the code only checks for create, update and delete operations.

However, that's not correct anymore. A few months after that code and comment was added, several additional permissions have been added, also "view" permissions.

So the comment is outdated and the access check is incomplete. Instead of trying to get File:access in sync with file_access, we decided to consolidate them to let one do the job and the other one is just a wrapper.

Expected behavior

Function access() should be up to date with actual permissions. Otherwise entity_access() would lead to unexpected results.

Additional information

  • Backdrop CMS version: latest dev

Some communication happened in Zulip, so I'm posting initial concerns and findings re file_access() here. Yes, we had some trouble to understand the current code. :wink: This matters here (File::access()), because the logic gets consolidated.

  1. This function does not actually handle $file = NULL, but claims to. After all it returns FALSE, anyway, if $file is neither an object nor a string. Why is it optional then?
  2. if (!$file && !in_array($op, array('view', 'download', 'update', 'delete', 'create'), TRUE) what does this check try to achieve? Shouldn't that be two different checks, or, honestly be an OR instead of an AND?
  3. // Fall back to default behaviors on view. why is "view" the only operation not handled in file_file_access? Means: no modules can hook in, no? Aren't they supposed to?
  4. @param $file ... file object or file type. But that makes no sense, as actually the 'type' isn't considered when creating files: user_access('create files', $account). There's no create permission per file type.

Minor release update: - [ ] Add a conclusion suitable for blog post + release notes - [x] N/A Add Documentation label if the feature needs to be documented - [x] Add Needs change record label if the the issue contains an API change. - [x] N/A Add Needs translation label if the the issue contains string translations.

conclusion suitable for blog post

GitHub Issue #: 
5480