In #1958950: file_entity_access fail check with "administer files" permission it was pointed out that the following code lives in file_entity_access() in the beginning portion of the logic:

  if (!$file && $op == 'create') {
    return user_access('create files', $account) || user_access('bypass file access', $account);
  }

This check is redundant because:

  • 'bypass file access' is checked just a couple lines below in the same function as expected since it should return TRUE regardless of $op, $file, or $account.
  • 'create files' is checked in file_entity_file_entity_access()

Comments

dave reid’s picture

Status: Active » Needs review
StatusFileSize
new602 bytes
dave reid’s picture

Let's add a test for the 'administer files' permission as well since it came up in the other issue.

Status: Needs review » Needs work

The last submitted patch, 1959026-file-entity-access-redundant-check.patch, failed testing.

dave reid’s picture

Status: Needs work » Needs review
StatusFileSize
new1.47 KB

Try this again.

dave reid’s picture

Component: Documentation » Code
Assigned: Unassigned » dave reid

Status: Needs review » Needs work

The last submitted patch, 1959026-file-entity-access-redundant-check.patch, failed testing.

devin carlson’s picture

Status: Needs work » Needs review
StatusFileSize
new1.47 KB

Good catch!

#4 looks good; it just needs the permissions to match the comment.

dave reid’s picture

I was confused when testing this. file_entity when installed should be adding the 'view files' permission to the authenticated user role. Why when I create a new testing user should they not be able to view files?

devin carlson’s picture

That makes sense to me. I know node.module cheats by having the access content permission added to both anonymous and authenticated roles by the standard and minimal installation profiles.

Should automatically granting view permissions during installation be done in this issue or in a follow-up?

Also, testFileEntityAccess() basically tests each permission in isolation, so should any automatically granted permissions be removed during the test setup?

devin carlson’s picture

Status: Needs review » Needs work

The last submitted patch, 1959026-7-file-entity-access-redundant-check.patch, failed testing.

devin carlson’s picture

Status: Needs work » Needs review
devin carlson’s picture

Status: Needs review » Fixed

Committed #7 to 7.x-2.x. Leaving any permission/test changes to follow-ups.

Status: Fixed » Closed (fixed)

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