Problem/Motivation

Current file entity access control is limited.

Proposed resolution

Provide a fuller API for controlling access to file entities. Introduce several new permissions and streamline the naming of existing file-related permissions.

Remaining tasks

  • Rename update function to be the latest in the 7200 series.
  • Rename system_file_entity_access to file_entity_file_entity_access.

User interface changes

Provides several new permissions.

API changes

Introduces an API that modules may use to affect file entity access.

Original report by Dave Reid

Another thing that media seems to be doing is adding their own simple file access API (see media_view_page). We can provide a base file access API that provides a hook_file_access() which also provides a fallback for private files for checking hook_file_download().

CommentFileSizeAuthor
#132 file_entity-file-access-1227706-132.patch34.25 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 258 pass(es). View
#130 file_entity-file-access_tests_only-1227706-130.patch6.14 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: run-tests.sh reported no tests were found. See review log for details.. View
#130 file_entity-file-access-1227706-130.patch34.15 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 272 pass(es), 1 fail(s), and 0 exception(s). View
#128 file_entity-file-access_tests_only-1227706-128.patch6.13 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in sites/default/modules/file_entity/tests/file_entity.test. View
#128 file_entity-file-access-1227706-128.patch34.15 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: run-tests.sh reported no tests were found. See review log for details.. View
#126 file_entity-file-access-1227706-126.patch28.74 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: run-tests.sh reported no tests were found. See review log for details.. View
#122 file_entity-file-access-1227706-122.patch28.31 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: run-tests.sh reported no tests were found. See review log for details.. View
#116 file_entity-file-access-1227706-116.patch27.11 KBlogaritmisk
PASSED: [[SimpleTest]]: [MySQL] 110 pass(es). View
#113 file_entity-file-access-1227706-113.patch26.5 KBlogaritmisk
PASSED: [[SimpleTest]]: [MySQL] 110 pass(es). View
#112 file_entity-file-access-1227706-112.patch26.04 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 110 pass(es). View
#107 expose_file_entities.pl_.txt712 bytesj0rd
#102 file_entity-file-access-1227706-102.patch26.04 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 110 pass(es). View
#98 file_entity-file-access-1227706-98.patch21.93 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 110 pass(es). View
#98 interdiff-97v98.txt1.1 KBParisLiakos
#97 file_entity-file-access-1227706-97.patch21.3 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 110 pass(es). View
#91 file_entity-file-access-1227706-91.patch21.59 KBjhedstrom
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_entity-file-access-1227706-91.patch. Unable to apply patch. See the log in the details link for more information. View
#89 file_entity-file-access-1227706-88.patch25.27 KBjhedstrom
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_entity-file-access-1227706-88.patch. Unable to apply patch. See the log in the details link for more information. View
#87 file_entity-file-access-1227706-87.patch25.92 KBjhedstrom
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_entity-file-access-1227706-87.patch. Unable to apply patch. See the log in the details link for more information. View
#87 interdiff.txt1.1 KBjhedstrom
#78 file_entity-file-access-1227706-78.patch21.76 KBJackinloadup
PASSED: [[SimpleTest]]: [MySQL] 106 pass(es). View
#78 innerdiff.txt2.54 KBJackinloadup
#76 file_entity-file-access-1227706-76.patch21.43 KBJackinloadup
PASSED: [[SimpleTest]]: [MySQL] 106 pass(es). View
#71 file_entity-file-access-1227706-71.patch21.43 KBaaron
PASSED: [[SimpleTest]]: [MySQL] 106 pass(es). View
#67 file_entity-file-access-1227706-67.patch20.47 KBjhedstrom
FAILED: [[SimpleTest]]: [MySQL] 88 pass(es), 20 fail(s), and 4 exception(s). View
#62 file_entity-file-access-1227706-62.patch20.37 KBjhedstrom
FAILED: [[SimpleTest]]: [MySQL] 88 pass(es), 20 fail(s), and 4 exception(s). View
#61 file_entity-file-access-1227706-61.patch20.37 KBjhedstrom
FAILED: [[SimpleTest]]: [MySQL] 88 pass(es), 20 fail(s), and 4 exception(s). View
#60 file_entity-file-access-1227706-60.patch21.23 KBjhedstrom
FAILED: [[SimpleTest]]: [MySQL] 88 pass(es), 20 fail(s), and 4 exception(s). View
#59 file_entity-file-access-1227706-59.patch19.45 KBjhedstrom
FAILED: [[SimpleTest]]: [MySQL] 88 pass(es), 20 fail(s), and 4 exception(s). View
#58 file_entity-add-a-file-entity-access-api_1227707-58.patch1.77 KBlslinnet
FAILED: [[SimpleTest]]: [MySQL] 127 pass(es), 6 fail(s), and 4 exception(s). View
#57 file_entity-add-a-file-entity-access-api_1227706-57.patch19.43 KBfabsor
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_entity-add-a-file-entity-access-api_1227706-57.patch. Unable to apply patch. See the log in the details link for more information. View
#51 file_entity-add-a-file-entity-access-api_1227706-51.patch19.16 KBfabsor
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_entity-add-a-file-entity-access-api_1227706-51.patch. Unable to apply patch. See the log in the details link for more information. View
#48 Bildschirmfoto 2012-03-13 um 11.33.55.png75.98 KBSebastian.Buesing
#44 file_entity-add-a-file-entity-access-api_extra.patch3.57 KBSebastian.Buesing
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_entity-add-a-file-entity-access-api_extra.patch. Unable to apply patch. See the log in the details link for more information. View
#41 file_entity-add-a-file-entity-access-api_1227706-40.patch17.92 KBdiqidoq
PASSED: [[SimpleTest]]: [MySQL] 17 pass(es). View
#36 file_entity-add-a-file-entity-access-api_1227706-36.patch18.05 KBshenzhuxi
PASSED: [[SimpleTest]]: [MySQL] 17 pass(es). View
#32 file_entity-add-a-file-entity-access-api_1227706-32.patch8.59 KBdiqidoq
PASSED: [[SimpleTest]]: [MySQL] 17 pass(es). View
#27 file_entity-access_api-1227706.patch10.42 KBshenzhuxi
PASSED: [[SimpleTest]]: [MySQL] 17 pass(es). View
#14 1227706-file-access-api.patch16.87 KBDave Reid
PASSED: [[SimpleTest]]: [MySQL] 37 pass(es). View
#12 1227706-file-access-api.patch15.42 KBDave Reid
FAILED: [[SimpleTest]]: [MySQL] 26 pass(es), 2 fail(s), and 2 exception(es). View
#9 1227706-file-access-api.patch15.33 KBDave Reid
FAILED: [[SimpleTest]]: [MySQL] 26 pass(es), 2 fail(s), and 2 exception(es). View
#7 1227706-file-access-api.patch15.34 KBDave Reid
FAILED: [[SimpleTest]]: [MySQL] 26 pass(es), 2 fail(s), and 2 exception(es). View
#6 1227706-file-access-api.patch0 bytesDave Reid
PASSED: [[SimpleTest]]: [MySQL] 37 pass(es). View
#1 1227706-file-access-api.patch5.23 KBDave Reid
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1227706-file-access-api.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Dave Reid’s picture

Status: Active » Needs work
FileSize
5.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1227706-file-access-api.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

Here's what I've come up with so far.

nedjo’s picture

Yes, this is needed to further move files towards being full fledged content entities. This patch would help clear the way for #1220414: Add created, published, promoted, and sticky fields and provide admin editing interface plus views integration.

If and once this is in, presumably media_access() calls should be revised to call file_access() or else (better, probably) should be replaced by file_access() calls.

Should this patch also introduce a set of file access permissions similar to (and replacing) those currently provided by Media module?

+++ b/file_entity.module
@@ -492,3 +507,121 @@ function template_preprocess_file_entity(&$variables) {
+  $cid = is_object($file) ? $file->nid : $file;

$file->nid should be $file->fid

nedjo’s picture

Dave Reid’s picture

Issue tags: +Media Sprint 2011

.

Dave Reid’s picture

We currently have the following permissions:
administer files
view file
edit file

This issue will change those permissions to:
bypass file access
administer file types
administer files
view files
edit own files
edit any files
delete own files
delete any files

The patch will add a file_access($op, $file, $account = NULL) function which will invoke hook_file_access() and not use any grant system.

We will also add a query tag 'file_access' to any query listing files.

Dave Reid’s picture

FileSize
0 bytes
PASSED: [[SimpleTest]]: [MySQL] 37 pass(es). View

Revised patch - will summarize tonight. :)

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
15.34 KB
FAILED: [[SimpleTest]]: [MySQL] 26 pass(es), 2 fail(s), and 2 exception(es). View

Try that again...

nedjo’s picture

Status: Needs review » Needs work

I read the patch but haven't tested.

Looking good! This, appropriately, closely mirrors the hook_node_access() system in core, with consideration for the special issues related to files.

New related project: http://drupal.org/sandbox/dereine/1284426. There's a possibility that, if that module matures, it could in future be introduced as a dependency, or in D8 that core would provide a generic entity access API.

+++ b/file_entity.api.php
@@ -126,3 +126,65 @@ function hook_file_view($file, $view_mode, $langcode) {
+  $type = is_string($file) ? $file : $node->type;

$node presumably should be $file.

+++ b/file_entity.api.php
@@ -126,3 +126,65 @@ function hook_file_view($file, $view_mode, $langcode) {
+    // Deny access to files in the first hour after uplaod so that they can be moderated.

Typo, should be upload. Also, this and a number of other comments are beyond 80 characters.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
15.33 KB
FAILED: [[SimpleTest]]: [MySQL] 26 pass(es), 2 fail(s), and 2 exception(es). View

Thanks for the review!

Updated patch.

Dave Reid’s picture

Just a note that ideally if we move this access API into core (or merge it as part of an entity access API), we can add a 'download' $op that would deprecate hook_file_download() (and use hook_file_access('download', $file) instead.

robeano’s picture

I pulled the latest 7.x-1.x version of file_entity and the latest 7.x-2.x of media.

I applied the patch and found the following errors.

1. When i run drush cc all, PHP Warnings are displayed:

WD php: Warning: class_implements(): object or string expected in    [warning]
file_get_stream_wrappers() (line 140 of
/Applications/MAMP/htdocs/media/includes/file.inc).
WD php: Warning: in_array(): Wrong datatype for second argument in   [warning]
file_get_stream_wrappers() (line 140 of
/Applications/MAMP/htdocs/media/includes/file.inc).
class_implements(): object or string expected in                     [warning]
file_get_stream_wrappers() (line 140 of
/Applications/MAMP/htdocs/media/includes/file.inc).
in_array(): Wrong datatype for second argument in                    [warning]
file_get_stream_wrappers() (line 140 of
/Applications/MAMP/htdocs/media/includes/file.inc).
'all' cache was cleared                                              [success]

When I go to admin/people/permissions, I see some more PHP notices and warnings:

    Notice: Undefined index: class in file_get_stream_wrappers() (line 140 of /Applications/MAMP/htdocs/media/includes/file.inc).
    Warning: class_implements() [function.class-implements]: object or string expected in file_get_stream_wrappers() (line 140 of /Applications/MAMP/htdocs/media/includes/file.inc).
    Warning: in_array() [function.in-array]: Wrong datatype for second argument in file_get_stream_wrappers() (line 140 of /Applications/MAMP/htdocs/media/includes/file.inc).
    Notice: Undefined index: type in file_get_stream_wrappers() (line 158 of /Applications/MAMP/htdocs/media/includes/file.inc).
    Notice: Undefined index: type in file_get_stream_wrappers() (line 168 of /Applications/MAMP/htdocs/media/includes/file.inc).
Dave Reid’s picture

FileSize
15.42 KB
FAILED: [[SimpleTest]]: [MySQL] 26 pass(es), 2 fail(s), and 2 exception(es). View

Yep, I didn't check if the private stream wrapper didn't exist. Oops.

effulgentsia’s picture

Status: Needs review » Needs work
+++ b/file_entity.api.php
@@ -126,3 +126,65 @@ function hook_file_view($file, $view_mode, $langcode) {
+ * ... Users with the "bypass file access"
+ * permission may always view and edit content through the administrative
+ * interface.

huh?

+++ b/file_entity.module
@@ -154,20 +165,60 @@ function file_entity_menu() {
+    'view public files' => array(

key name should be 'view files' i think.

+++ b/file_entity.module
@@ -154,20 +165,60 @@ function file_entity_menu() {
+  $permissions['view public files']['description'] .= ' ' . t('Includes the following types of files: <em>@wrappers</em>.', array('@wrappers' => implode(', ', $wrappers['public'])));
+  $permissions['view own private files']['description'] .= ' ' . t('Includes the following types of files: <em>@wrappers</em>.', array('@wrappers' => implode(', ', $wrappers['private'])));

s/<em>@wrappers</em>/%wrappers/

+++ b/file_entity.module
@@ -731,3 +782,197 @@ function theme_file_entity_file_link($variables) {
+function file_get_stream_wrapper($scheme) {

Grrr. Seems like core might add this function some day, in which case we'll get a function redefinition error. Not sure how to deal with that though.

+++ b/file_entity.module
@@ -731,3 +782,197 @@ function theme_file_entity_file_link($variables) {
+      if ($file->uid == $account->uid && !empty($account->uid) && user_access('view own private files', $account)) {

Should the !empty() be first?

+++ b/file_entity.module
@@ -731,3 +782,197 @@ function theme_file_entity_file_link($variables) {
+  $type = is_string($node) ? $node : $node->type;

s/$node/$file

+++ b/file_entity.module
@@ -731,3 +782,197 @@ function theme_file_entity_file_link($variables) {
+  if (!file_valid_uri($file->uri)) {

For "create", $file might be a string.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
16.87 KB
PASSED: [[SimpleTest]]: [MySQL] 37 pass(es). View

Revised patch.

Dave Reid’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev

File entity has switched to a 7.x-2.x branch and the 7.x-1.x branch is no longer used. Please make sure to update your Git clones.

Tobbetobbe’s picture

Hi!
Does this patch mean that you can set permissions so a user can add files and edit them but not edit other users files? Just a little unsure if the permission "administer files" is the permission used to allow users to add files. And does it override the permission "edit own files"?

And has this patch been tested with file entity 7.x-2.x? Sorry if I already should be able to make that out of the info here.

Dave Reid’s picture

@Tobbetobbe: In your case you would enable the 'Add and upload files' and then 'Edit own files' permissions. With the patch the only permission that overrides those types of permissions is the 'Bypass file access' permission.

nedjo’s picture

Priority: Normal » Major

This is a much needed, soundly conceived and written, and extremely well documented patch. More than a feature request, it addresses a major problem: that allowing a given user to edit file entities grants that user edit access to all file entities.

The approach successfully implements reasonable file access permissions that will serve in most cases while also introducing documented API methods for other modules to extend file access control.

Questions:

Can we still change function file_entity_update_7103() or do these changes need to go in a separate update?

Given the added complexity of the access system, do we need test coverage?

aaron’s picture

Issue tags: +Needs tests

adding tag that we need tests

Tobbetobbe’s picture

Thanks for the answer, Dave Reid. I will try it.

ar-jan’s picture

I tried the patch in #14. It applied and the database update was without errors.

Question: it seems that only the 'Administer files' permission allows a user to actually use the Add media form. This is using a Media asset field. (The popup after clicking 'Select media' says "You are not authorized to access this page." when the user is not given 'Administer files' permission).

Is this something for file_entity or media itself to handle? (I noticed this media issue: #992978: Add 'View media browser library' permissions).

Dave Reid’s picture

There would be a follow-up in the Media issue queue once this patch is committed to ensure it uses the file api where possible.

Tobbetobbe’s picture

I have the same problem as ArjanLikesDrupal. The patch seemed to apply fine and I have the extended permissions for file entity. But I also get ""You are not authorized to access this page." in an overlay when I click "Select media". This is with a logged in user after marking the permissions "Add an upload files" and "Edit own files" for authenticated users.

Even when I mark "edit any files" for authenticated users, they are not allowed to edit files when I navigate directly to a file like this one http://72.29.78.36/~tobbesla/?q=file/13 (the file was uploaded before I applied the patch).

Dave Reid’s picture

This is because we would have to make changes in the Media module to account for this patch once this is committed to File entity. You cannot just test this patch and attempt to use the Media module.

Tobbetobbe’s picture

Ok, I understand now that´s what you were saying in #22 :)

boombatower’s picture

Why wouldn't we want to implement a generic entity access api instead? (maybe I missed it above as I scanned)

shenzhuxi’s picture

FileSize
10.42 KB
PASSED: [[SimpleTest]]: [MySQL] 17 pass(es). View

I merged #14 patch to 7.x-2.x
Please test.

diqidoq’s picture

hm, while there are many trailing whitespaces which I may can announce here for review, I rather don't know why the patch doesn't apply here on latest git clone from 2 min ago (7.x-2.x)?

anyone else? ( I need more time to dip deeper to check whats wrong in the patch )

diqidoq’s picture

diqidoq’s picture

ok, even patch -p? < nor git apply works here. comparing the patches from #14 and #27 feels like something is missing imho ...

I also don't know what this is about in the first part mentioning file_entity.api.php without changes and the tab spaced + // lines on the file_entity.module ?

diff --cc file_entity.api.php
index 132186c,8d91946..0000000
--- a/file_entity.api.php
+++ b/file_entity.api.php
diff --cc file_entity.module
index d5abdb2,ddffffb..0000000
--- a/file_entity.module
+++ b/file_entity.module
@@@ -111,14 -75,10 +122,14 @@@ function file_entity_menu() 
    );
    // general view, edit, delete for files
    $items['file/%file'] = array(
 +    'title callback' => 'entity_label',
 +    'title arguments' => array('file', 1),
 +    // The page callback also invokes drupal_set_title() in case
 +    // the menu router's title is overridden by a menu link.
      'page callback' => 'file_entity_view_page',
      'page arguments' => array(1),
-     'access callback' => 'file_entity_access',
-     'access arguments' => array('view'),
+     'access callback' => 'file_access',
+     'access arguments' => array('view', 1),
      'file' => 'file_entity.pages.inc',
    );
    $items['file/%file/view'] = array(
@@@ -128,10 -88,10 +139,10 @@@
diqidoq’s picture

Status: Needs review » Needs work

The author of that patch has commented that he has merged it from the patch which was originally made for the 7.x-1.x branch of file_entity to apply now for the 7.x-2.x branch. The @@@ ++ -- line numbers info of his patch includes 3 diff line values now instead of the two I am familiar with. I would try to apply that patch manually in the worsed case to reroll it as a normal git patch, but I need somebody who helps me to interprete this.

Otherwise I doubt that this patch is ok. Correct me if I am wrong.

diqidoq’s picture

FileSize
8.59 KB
PASSED: [[SimpleTest]]: [MySQL] 17 pass(es). View

hm ... ok, I give up. After two days shadow boxing and no helpful input from somewhere I tried to put my hands on this broken patch and have added it all manually again on a clean new git clone of 7.x-2.x.

I'll attach the rerolled patch here, but I had no time to test yet ...

diqidoq’s picture

Status: Needs work » Needs review
shenzhuxi’s picture

commit 4db2744b1d3d1f5e50a2390bb48dc45645365ed6
Author: bevan
Date: Mon Feb 20 09:46:39 2012 -0600

Issue #1431070: Invalidate field caches for entities referencing files when a file is saved.

This is the new commit ahead of the patchs.
I think it's better to move all the functions with "file" namespace to file_entity.file_api.inc.

@Digidog you need to use the right commit index to use the patch. After that pull the latest commits, and manually solving the conflicts shouldn't be difficult.

diqidoq’s picture

shenzhuxi: Your patch was also full of trailing white spaces and maybe also other encoding problems with unknown characters, and 2,3 others on IRC agreed that the patch is broken (or at least too noisy) somehow. Not sure, but all lines of to code with need of change from your patch still exist in latest dev. So I rerolled it all manually and that took me too much time to be willing to use the noisy patch from before again. Sorry. So please review the rerolled patch. There is nothing different to yours, instead of fitting to the latest dev. (and I don't clame for any credits here)

All I want is that it goes on here, so if it definitely doesn't work this way, ok, then I am out of here.

shenzhuxi’s picture

FileSize
18.05 KB
PASSED: [[SimpleTest]]: [MySQL] 17 pass(es). View

@Digidog #32 missed the changes in hook_permission

"full of trailing white spaces" is it because the EOL is different in OSX? Are you using Linux or Windows?

I regenerated a patch ahead of commit a0c0a007bee8d4faecc81b1f2567e3a67011c0be

Fidelix’s picture

shenzhuxi, all patches and code on D.O need to have Unix file endings.

cweagans’s picture

That patch applies okay for me, and I don't see any trailing whitespace from a quick skim of it. I don't know enough about file_entity to do a more detailed review though.

xjm’s picture

@shenzhuxi -- The patch in #27 has lots of places where there is a single trailing whitespace character at the end of the line on blank lines. However, the patch in #36 is fine and does not have this problem.

ejsteven’s picture

Patch #36 works great for me. Thanks!

diqidoq’s picture

FileSize
17.92 KB
PASSED: [[SimpleTest]]: [MySQL] 17 pass(es). View

shenzhuxi: Thanks for your reroll. Awesome! The patch from #36 applies successfully and the new permission settings form works (7.12).

One drop of bitterness: warning: 1 line adds whitespace errors. So if I interpret Dreditor correctly, it is about the very last empty line in your patch. (Sorry, don't wanted to sound picky)

But apart from that, shenzhuxi++ !

I would say RTBC?

Clean patch attached. (I only removed the empty line @ 1105 of entity.module which throws the warning by applying the patch)

Fidelix’s picture

Status: Needs review » Reviewed & tested by the community
gmclelland’s picture

Patch at #41 applies cleanly and I'm seeing the new permissions listed.

Sebastian.Buesing’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
3.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_entity-add-a-file-entity-access-api_extra.patch. Unable to apply patch. See the log in the details link for more information. View

Hi,

I just found a hole in the Access API :-(.

Even if you implement hook_file_access() and deny everything except for the "view" op you can still delete files the following way:

  1. Go to admin/content/file/list
  2. Select some files
  3. Chose delete in the dropdown and hit Submit
  4. Hit Submit again and the files get delete without file_access('delete', $file, $account) being called as it should be i guess.

Added a patch that fixes this for me, but may have some more code than it should.

Sebastian.Buesing’s picture

I realized that if you only install the file_entity and media module that you don't run into this problem. You need to install media_browser_plus which does change the access to 'admin/content/file' from 'administer files' to 'access media backend'.

The reason behind this is to allow certain users to manage all or a subset of file/media items. This does raise the question which module should deal with this issue, but I think that at least on the actual deletion of files inside file_delete_multiple(array $fids) there should be a call to file_access('delete', $file, $account).

Dave Reid’s picture

No, we never should be calling access from inside an API function. So what this patch is missing is ensuring that we check file_access() when generating the table listing in file_entity.

Dave Reid’s picture

Sebastian.Buesing’s picture

So, we should just not allow the user to get that far using the UI?

Like this you mean?

Screen

shenzhuxi’s picture

Right now, I think maybe file_entity should use views and vbo to provide UI.

shenzhuxi’s picture

Why it's still not committed? wait for general entity access API?

fabsor’s picture

FileSize
19.16 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_entity-add-a-file-entity-access-api_1227706-51.patch. Unable to apply patch. See the log in the details link for more information. View

Here is a reroll of the access API patch. I also attached an access check for edit and delete.

I'm leaving this as needs work since we have some tests to write.

brayo4’s picture

the latest 7.x-2.x-dev|2012-May-09 does not have this patch included. should we not be using this patch ???

tsvenson’s picture

@brayo4: Patches are work in progress, but there is no guarantee when/if they gets committed to the project.

This patch for example is needing a lot more work before its ready.

brayo4’s picture

thanks for your response. is there a chipin/equivalent so we can contribute/offer an incentive/fast track this process for those of us who cannot contribute code?? getting this media module and its plugins working and stable will be huge for D7.....no parallel. thank you for the tireless effort you put in........

tsvenson’s picture

@brayo4: Thanks for your offer to help out financially. It is something we have been looking into, but its a difficult beast. Plus that for something like the Media Initiative just defining the scope would require a lot.

However, we are getting off-topic now and I suggest this discussion continues in for example http://groups.drupal.org/media/media-initiative instead.

fbov’s picture

How did you implement the folders?
Also is there a way to set access to a certan file. Like you do with access control for nodes.

fabsor’s picture

FileSize
19.43 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_entity-add-a-file-entity-access-api_1227706-57.patch. Unable to apply patch. See the log in the details link for more information. View

Argh, yet another reroll. This is another straight reroll so that we can keep on working on this.

lslinnet’s picture

FileSize
1.77 KB
FAILED: [[SimpleTest]]: [MySQL] 127 pass(es), 6 fail(s), and 4 exception(s). View

a seperate patch with a starting point for implementing tests for the access layer. (notice currently the test don't pass due to bugs in the code)

jhedstrom’s picture

FileSize
19.45 KB
FAILED: [[SimpleTest]]: [MySQL] 88 pass(es), 20 fail(s), and 4 exception(s). View

Re-rolling #57 against unstable6.

jhedstrom’s picture

FileSize
21.23 KB
FAILED: [[SimpleTest]]: [MySQL] 88 pass(es), 20 fail(s), and 4 exception(s). View

This patch replaces remaining calls to file_entity_access() in file_entity.pages.inc, with calls to file_access().

jhedstrom’s picture

FileSize
20.37 KB
FAILED: [[SimpleTest]]: [MySQL] 88 pass(es), 20 fail(s), and 4 exception(s). View

Oops, that last patch had .info file rewriting in it.

jhedstrom’s picture

FileSize
20.37 KB
FAILED: [[SimpleTest]]: [MySQL] 88 pass(es), 20 fail(s), and 4 exception(s). View

There was a logic error in file_access:

  if (!$file || !in_array($op, array('view', 'update', 'delete', 'create'), TRUE)) {
    // If there was no file to check against, or the $op was not one of the
    // supported ones, we return access denied.
    return FALSE;
  }

Since that uses an OR operator, this function was always returning false when $file wasn't passed (eg, on 'create'). Patch switches this logic to use an AND operator.

I've also created the follow-up issue in media to switch to using file_access once this is ready. #1677054: Use file_entity_access() instead of media_access().

micbar’s picture

This is a good opportunity to make a proposal concerning the permissions of this module.
In version 7.x-2.0-unstable4 the file_delete function has been replaced with the api function file_delete_multiple, which avoids unwanted validation and usage checking.
If a user has the permission to delete files, we cannot restrain him from deleting files used in nodes. He only gets a notice "(in use)" from the delete confirm form.

Couldn't we add another permission, e.g. "delete files in use" and check file_usage_list if a user doesn't have this permission?

lpalgarvio’s picture

is add/create files permission contemplated on this issue?

kenianbei’s picture

By looking at the patch in #62, I wasn't immediately able to see if this will allow per user permission checks.

Will it be possible to have users who create a file only have edit/delete access to the file they create, and no access to admin/content/file, similar to how nodes work?

Also, there is more work starting here on a general entity API: #1696660: Add an entity access API

thrnio’s picture

#62 isn't right, it should be OR, at least the way the function is written currently. Everywhere after that, we assume that $file exists.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
20.47 KB
FAILED: [[SimpleTest]]: [MySQL] 88 pass(es), 20 fail(s), and 4 exception(s). View

Re-roll of #62, with a small change to file_access to take into account #66.

Status: Needs review » Needs work
Issue tags: -Needs tests, -sprint, -Media Initiative

The last submitted patch, file_entity-file-access-1227706-67.patch, failed testing.

The last submitted patch, file_entity-file-access-1227706-67.patch, failed testing.

Jackinloadup’s picture

should the file/add path get the use the "create files" privilege instead of "administer files"?

  $items['file/add'] = array(
    'title' => 'Add file',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('file_entity_add_upload', array()),
    'access arguments' => array('administer files'),
    'file' => 'file_entity.pages.inc',
  );

vs

  $items['file/add'] = array(
    'title' => 'Add file',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('file_entity_add_upload', array()),
    'access callback' => 'file_access',
    'access arguments' => array('create', 1),
    'file' => 'file_entity.pages.inc',
  );
aaron’s picture

Assigned: Dave Reid » Unassigned
Status: Needs work » Needs review
FileSize
21.43 KB
PASSED: [[SimpleTest]]: [MySQL] 106 pass(es). View

took care of #70: it should actually be file_access, with a $op of 'create'. i also took care of the existing tests.

Jackinloadup’s picture

Here are relavent issues to tackle when this issue is in. Which I assume is impending.
Media #1677054: Use file_entity_access() instead of media_access()
Media Browser Plus #1721524: Use file_access() instead of media_access()
Media Colorbox #1730730: Use file_access() instead of file_entity_access()

jpstrikesback’s picture

Tested upload along with the patch in #2 of Media #1677054: Use file_access() instead of media_access(). Works great.

Dave Reid’s picture

Assigned: Unassigned » Dave Reid

Assigning to myself to finish and commit.

caschbre’s picture

I have the same question as Jackinloadup in #70.

I noticed there wasn't a "create file" permission. So to get around that we have to put a file/image field on a node. Even then it looks like the "upload" option isn't available.

Jackinloadup’s picture

Status: Needs review » Needs work
FileSize
21.43 KB
PASSED: [[SimpleTest]]: [MySQL] 106 pass(es). View

After #75 caschbre stated an issue I reviewed my suggestion and noticed there was an issue.

+++ b/file_entity.moduleundefined
@@ -863,7 +919,207 @@ function file_entity_get_hidden_stream_wrappers() {
+  if (!$file && $op == 'create') {
+    return user_access('create files', $account);
+  }

File must not be sent with 'create' if we want to check the right permission

Here is a patch to correct this in hook_menu

Jackinloadup’s picture

Created new issue to follow up with the great point #63 micbar brought up.

#1763024: New permission 'delete files in use'

Jackinloadup’s picture

FileSize
2.54 KB
21.76 KB
PASSED: [[SimpleTest]]: [MySQL] 106 pass(es). View

David Reid proposed in #10 that we add a 'download' option to file_access and has created #1422260: Add a file/%file/download callback

I think this would be great.

If we add the access portion of this now we can follow up with proper functionality.

Attached is a patch to start adding this in.

Jackinloadup’s picture

Status: Needs work » Needs review

grr forgot to set for review

caschbre’s picture

I'll check #78 out. Is this the only patch required? And are there specific things from the patch you want focused on?

Jackinloadup’s picture

@caschbre - In #78 I only added the permissions and access checks. See the innerdiff.txt to see what changed between #76 and #78

To test functionality I rerolled the patch in #1422260-3: Add a file/%file/download callback. Please test and vet issues as I haven't had time as of yet.

You will need both this patch and the patch in the issue above to have any usable functionality.

caschbre’s picture

Ok... I'll pull down patch #78 and #1422260-3 over the weekend and see how things look.

EDIT: just so I'm clear... I'll need the latest media module dev release plus patch #78. I'll also need the latest file entity dev release plus patch #1422260-3. That's what I'm planning to play with.

Thanks!

Jackinloadup’s picture

@caschbre - Yep those are the correct patches. Both of these patches are related to file_entity. Media isn't needed to test this.

LInk to Applying Patches if your not sure.

Thanks for testing ^_^

Dave Reid’s picture

I would prefer to actually leave the download stuff out for now. It makes things harder to review. I'll be uploading a new patch today because I've found a few extra things like ensuring that the file_access tag is automatically added to any Views of files, etc. The only remaining thing is ensuring that the upgrade path is 100% correct for updating/adding the new permissions.

caschbre’s picture

Ok... I'll hold off until Dave posts his patch.

nedjo’s picture

I've posted a patch on the File admin module that can be used to test this patch.

With just a few lines in hook_file_access() I was able to restrict file access by the value of a 'published' flag.

See the issue summary of #1734882: Add file access restrictions once file access improved in file_entity for testing instructions.

jhedstrom’s picture

FileSize
1.1 KB
25.92 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_entity-file-access-1227706-87.patch. Unable to apply patch. See the log in the details link for more information. View

Attached is a reroll of #76 (since Dave wants to keep download stuff out for now). I added some logic to system_file_access() since explicit grants were being ignored (not sure if this is the way to go, but it allows access to files that users legitimately have access to that was previously being denied by file_access)--see interdiff.txt.

Status: Needs review » Needs work

The last submitted patch, file_entity-file-access-1227706-87.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
25.27 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_entity-file-access-1227706-88.patch. Unable to apply patch. See the log in the details link for more information. View

Oops, patch in #87 had .info file changes from drush make. Clean patch attached.

Status: Needs review » Needs work

The last submitted patch, file_entity-file-access-1227706-88.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
21.59 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_entity-file-access-1227706-91.patch. Unable to apply patch. See the log in the details link for more information. View

Sorry for the noise. Same patch as #87 but this one should apply to latest dev.

Devin Carlson’s picture

Jackinloadup’s picture

j0rd’s picture

I just applied patch #91 cleanly.

No view own files permission

It does seem to work as expected in my quick short tests.

There's a problem though, currently there's no "View Own Files" permission and just a "View Files" permission, which I assume means all files.

Since FIDs are also sequential, this allows for a fairly large information leak.

Example:
I just uploaded file #180. I can view this via file/180. This means I can view ever other file on the server via file/[1 ... 179]

Workflow problems

The workflow for a file upload appears to be:

  1. /file/add (Add the file)
  2. /file/$FID/edit (Edit the details of the file)
  3. /admin/content/file (Which requires the "Administer Files" permission)

If the user does not have "Administer Files" permission, upon the uploaded sequence, they will be presented with "Access denied" error. This is no good.

Proposed solution:
Should a user be able to upload a file, I also believe they should be able to view it. This could remove the need for "View Own Files" permission. That's just my opinion though. We could create another non-admin path like /file/list which lists all the files they've uploaded and send them there after they've uploaded the file (like /file/$FID/view). Else send them somewhere else. If the user does have permissions to "Adminsiter Files" /admin/content/file is an appropriate endpoint.

ParisLiakos’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +sprint, +Media Initiative

The last submitted patch, file_entity-file-access-1227706-91.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
21.3 KB
PASSED: [[SimpleTest]]: [MySQL] 110 pass(es). View

reroll

ParisLiakos’s picture

FileSize
1.1 KB
21.93 KB
PASSED: [[SimpleTest]]: [MySQL] 110 pass(es). View

This patch takes care of redirect to admin/content..or at least i think, did not test it.
About the view own files permissions, it doesnt make sense for public files. For private files there is the permission view own private files

j0rd’s picture

I've noticed that some of the function in this module are not namespaced properly in these patches. Most important is file_access, which should be file_entity_access, but there are others. Best to resolve this now, before other modules start writing functionality using these improperly namespaced functions.

function file_get_stream_wrapper($scheme) {
function file_access($op, $file = NULL, $account = NULL) {

Additionally in file_entity there are these functions

function file_is_page($file) {
function file_type_get_name($file) {
function file_type_get_names() {
function system_file_access($op, $file, $account) {

If there's something I don't understand to why this is done, then simply fill me in.

j0rd’s picture

As for the "View own files" permissions for "public" files.

Most people will be using the file_entity module in combination with Media module.

The common use case is that, and this is why we're creating this patch, to allow multiple users to manage their own Media files.

The thing with the Media module is, you can upload a bunch of files, and not necessarily public them with content. You can add your files, then publish them with content as you want. From the end users perspective, they believe these files to be private until the time they are published with content.

Now for images, this isn't very important. But Media and File Entity are useful for other use cases, lets say documents. Someone might upload a bunch of documents to be released in a press report on Friday. It's currently Monday. They upload their documents, create a "Press Release" node, and save it unpublished.

Now some other user on the site figures out the /files/$FID trick and goes through all the files uploaded to the site associated with an FID from 1 until they start hitting 404s, essentially being able to view every file published to the site. From what I can tell, these files dont even need to be uploaded via the file_entity module, it's simply ever file in the file_managed table which is not marked as private.

Now I understand what I'm asking for is "Security Through Obscurity", which is not security...but I think the easy numeric URLs will allow for too much easy "data discovery" for people like me, get intrigued when they see sequential numeric URLs. In fact, that's why I brought it up....I'll be using this method on any site I know has file_entity installed, if this code goes live. It's fun to fish.

I personally think it makes sense, if a person is able to `create files`, that they're able to view their own files only. If they have `view all files` permission, they're able to view own files.

ParisLiakos’s picture

I see what you saying, what i mean though, when saying it doesnt make sense for public files:

Files are not exactly nodes. They live both in filesystem and database not just database.And since public files are accessible from anyone if they know their location (filesystem path) it doesnt make sense restricting access to the database path (URL).

Imagine if someone couldnt see file from file/<id>, but could guess the filesystem path and could see it from sites/default/files/<filename> ... that should be considered as a secuirity issue. You see where i am coming from?

ParisLiakos’s picture

FileSize
26.04 KB
PASSED: [[SimpleTest]]: [MySQL] 110 pass(es). View

Ok this renames functions and constants and cleanups file_entity.module at least with the correct prefix.
there are a lot more in other files, but i left them for other issues..

this looks like a wtf to me as well:

/**
 * Implements hook_file_entity_access() on behalf of system.module and private files.
 */
function system_file_entity_access($op, $file, $account) {

why do we need this?

also:

+ * Update permission names.
+ */
+function file_entity_update_7105() {

shouldnt we use 72XX numbering already?

please fill me up as well, i dont have the background:/

j0rd’s picture

@rootatwc I also get what you're saying. I understand public files are always public under the files/ folder. As I've stated previously, what I'm asking for is Security Through Obscurity, which technically isn't a security technique.

My issue is the easy discovery of files via a numeric URL. We already have a permission for "View All Files", so ideally if this is not checked, then user should only be able to see files he's uploaded via file/FID.

Most people don't leave `Options +Indexes` in their Apache Config for a reason. It provides of lot of easy information to malicious users. Technically any files listed under there are public, but they're not easy to guess. I essentially see not implementing "View Own Files" as leaving `Options +Indexes` enabled in Apache. It's just a bad idea.

Also from an end-users perspective, they believe (incorrectly though), that if they haven't published a node with attached files, that these files are not accessible. With the easy discovery via numeric links, it makes accessing these files, which the end user believes are "unpublished" much easier.

I've already written a short script, which is unfortunately on another computer at the moment, which will download all files uploaded to a site via the sequential FID urls. I'll publish it when I'm on the other computer.

ParisLiakos’s picture

hmm, now i see what you saying..
it is easier to guess file id than filename when its sequencial true...hrmmm

j0rd’s picture

You don't really have to guess the file IDs, they start at 1 and move upwards.

The script I wrote, simply starts at 1 and keeps going up until it finds 10 404s in a row and then stops. Seems to work fine at grabbing all the files. Although there's not a lot of sites running Media 2.x right now to "exploit".

jpstrikesback’s picture

j0rd, should this be solved here and now or as part of another module that generates a UUID for the URI in hook_file_insert?

j0rd’s picture

FileSize
712 bytes

IMHO, it's a simple fix with this patch. It's a couple line fix. If you don't fix it here, there's going to be a lot of people vulnerable.

Here's my "exploit" if anyone is curious. It's a simple perl script.

$ perl expose_file_entities.pl > found.html
semei’s picture

Thank you guys for working on this feature that many of us have been waiting for. Without putting any pressure on anyone, can you estimate when this thing will land? Days, weeks, months?

j0rd’s picture

@semei, already seems to work fairly well with the appropriate patches applied. That's the route I'd go if you're waiting for it to get done.

tigerste’s picture

In our environment we have three main roles, other than Admin: 'Communications', 'Publisher' and 'Author'. I have customised the media browser views so that WYSIWYG only displays options specific to images, and there is an 'attachments' field in the page content type which is for documents.

The roles are defined so that Communications are the only role that are able to upload images (Authors and Publishers can use images already in the library), and all three roles are able to upload documents. Unfortunately, authors and publishers still get the upload button in WYSIWYG, even though the permissions correctly prevent them from submitting the upload.

Will this patch hide the upload option in this scenario, and also allow more granular control of files i.e. so that publishers/authors only have access to edit files they've uploaded themselves? Only Comms and Admin should be able to edit all files. Many thanks!

Kazanir’s picture

I believe this needs to be re-rolled in order to apply to the newest dev of File Entity.

ParisLiakos’s picture

FileSize
26.04 KB
PASSED: [[SimpleTest]]: [MySQL] 110 pass(es). View

Rerolled

logaritmisk’s picture

FileSize
26.5 KB
PASSED: [[SimpleTest]]: [MySQL] 110 pass(es). View

FILE_DEFAULT_ALLOWED_EXTENSIONS is used in file_entity_add_upload_multiple_submit, changed this to FILE_ENTITY_DEFAULT_ALLOWED_EXTENSIONS

Status: Needs review » Needs work
Issue tags: -Needs tests, -sprint, -Media Initiative

The last submitted patch, file_entity-file-access-1227706-113.patch, failed testing.

logaritmisk’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +sprint, +Media Initiative
logaritmisk’s picture

FileSize
27.11 KB
PASSED: [[SimpleTest]]: [MySQL] 110 pass(es). View

Found another problem, file_type_get_names is called in views_handler_filter_file_type, but it is now named file_entity_get_names.

Status: Needs review » Needs work
Issue tags: -Needs tests, -sprint, -Media Initiative

The last submitted patch, file_entity-file-access-1227706-116.patch, failed testing.

logaritmisk’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +sprint, +Media Initiative
Kazanir’s picture

I had to change that file_type_get_names in 2 or 3 of the /views files in the module logaritmisk -- did you get all of them?

Also, I'm running into 1 potential issue and 1 concrete issue:

- Potential: On the roles/permissions page, it says that the "View own private file details" and "View file details" apply to file types: None. The exact wording is:

View file details
For viewing file details, not for downloading files. Includes the following types of files: None.

I'm unclear as to why "None" would be displaying here -- does this reflect the patch needing to be updated to use the new file_entity types instead of the old media_types, or is this something else?

- Concrete: This patch breaks Media Colorbox implementation -- Colorbox creates links of the form: /media_colorbox/679/media_original/file -- and these links cannot be accessed by someone with the "View file details" permission, even if that person CAN access the file entity page directly at /file/679 (e.g.)

Is this something that needs to be fixed on the Colorbox side to integrate with this patch properly?

ParisLiakos’s picture

For viewing file details, not for downloading files. Includes the following types of files: None

This is because none of your file types support private schemes..Edit Actually thats for public files:/

Is this something that needs to be fixed on the Colorbox side to integrate with this patch properly?

Did not check but i am pretty sure colorbox will need to update its code to be compatibile with the new file_entity access api

Kazanir’s picture

Yeah, I fixed media_colorbox_menu to call file_entity_access with the second argument instead of just array('view') and now it works. I suppose I should go give them a patch.

Edit: What I posted here before was wrong. Rootatwc and I are troubleshooting this now.

ParisLiakos’s picture

FileSize
28.31 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: run-tests.sh reported no tests were found. See review log for details.. View

this takes care #119

Status: Needs review » Needs work
Issue tags: -Needs tests, -sprint, -Media Initiative

The last submitted patch, file_entity-file-access-1227706-122.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs tests, +sprint, +Media Initiative

The last submitted patch, file_entity-file-access-1227706-122.patch, failed testing.

ParisLiakos’s picture

Assigned: Dave Reid » Unassigned
Status: Needs work » Needs review
FileSize
28.74 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: run-tests.sh reported no tests were found. See review log for details.. View

This one introduces view own files permissions
Not tested.
@j0rd plz test:)

nedjo’s picture

Assigned: Unassigned » Dave Reid
Status: Needs review » Needs work

I put a placeholder issue summary in place--needs a lot of fleshing out.

Not sure why the hook_file_entity_access() is on behalf of system module--probably better to make it file_entity_file_entity(). Also noted another needed fix to the update function number as noted in #102.

Re namespacing (file_ vs. file_entity_ as dicussed in #99 and later): yes, this is an issue, but it should be addressed in a different issue except as it relates strictly to what's being introduced or changed here.

nedjo’s picture

Issue summary: View changes

Provide rudimentary summary. Needs a lot of fleshing out.

ParisLiakos’s picture

Assigned: Dave Reid » ParisLiakos
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
34.15 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: run-tests.sh reported no tests were found. See review log for details.. View
6.13 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in sites/default/modules/file_entity/tests/file_entity.test. View

Moved system_file_entity_access to file_entity_file_entity_access
Moved update function to 72XX series
Added tests for both file_entity_access and menu callbacks.

This looks like RTBC to me

Status: Needs review » Needs work

The last submitted patch, file_entity-file-access-1227706-128.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
34.15 KB
FAILED: [[SimpleTest]]: [MySQL] 272 pass(es), 1 fail(s), and 0 exception(s). View
6.14 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: run-tests.sh reported no tests were found. See review log for details.. View

hmm yeah

Status: Needs review » Needs work

The last submitted patch, file_entity-file-access-1227706-130.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
34.25 KB
PASSED: [[SimpleTest]]: [MySQL] 258 pass(es). View

There is a freaking test i cant fix...but tested manually and works..
will revisit it another time, i am out of patience now..but still i think its rtbc

gooddesignusa’s picture

I tried to apply the latest patch from #132. I'm using the latest devs of entity, file_entity & media. After applying the patch I cleared cache and ran updatedb from drush which spit out an error. It looked trimmed so I went to update.php and this is what I got when trying to apply this update.

file_entity module : 
  7203 -   Update permission names. 
Failed: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '3-view files' for key 'PRIMARY': UPDATE {role_permission} SET permission=:db_update_placeholder_0 WHERE (permission = :db_condition_placeholder_0) ; Array ( [:db_update_placeholder_0] => view files [:db_condition_placeholder_0] => view file ) in file_entity_update_7203() (line 541 of /media/RAID10/***/htdocs/sites/all/modules/file_entity/file_entity.install).

Also worth mentioning since most people will be using this with media.

Notice: Use of undefined constant FILE_DEFAULT_ALLOWED_EXTENSIONS - assumed 'FILE_DEFAULT_ALLOWED_EXTENSIONS' in media_variable_default() (line 131 of /media/RAID10/***/htdocs/sites/all/modules/media/includes/media.variables.inc).
ParisLiakos’s picture

I guess you have tried this patch again and run the 71XX update which did the very same thing?

gooddesignusa’s picture

I'm sorry i'm a little confused about your response. I did try a earlier patch awhile back ago, could that of added something to my update queue? Last night when I tried out the recent version I grabbed a new copy of all the devs for all the modules, ran updatedb. This error only came up after applying this patch and running update.php. I'm more than happy to get more info if you need to help debug. Everything seems to work great other than that.

gooddesignusa’s picture

So the update issue was due to me applying a different version of this patch awhile back ago.
To get around the issue. I used a drush command to update the db with the help of rootatwc
The schema_version needed to be incremented by 1.

drush sql-query "UPDATE system SET schema_version='7203' WHERE name='file_entity'"

I've tested this patch with media and everything seems to be working as intended.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

Tested #132. Works fine except a PHP notice from Media module:

Notice: Use of undefined constant FILE_DEFAULT_ALLOWED_EXTENSIONS - assumed 'FILE_DEFAULT_ALLOWED_EXTENSIONS' in media_variable_default() (line 131 of /var/www/html/sites/all/modules/media/includes/media.variables.inc).

I opened a placeholder issue there till this will be fixed at #1821262: Expected API change in File Entity.

ParisLiakos’s picture

Status: Reviewed & tested by the community » Fixed

thanks all!
commited
now lets fix media

jpstrikesback’s picture

Flipping right!! Thanks all!!!

Status: Fixed » Closed (fixed)

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

nedjo’s picture

I added file access support to File admin in http://drupalcode.org/project/file_admin.git/patch/2d387dc so that file entity access is controled by a 'published' flag. The hook implementation:

/**
 * Implements hook_file_entity_access().
 *
 * Restrict view access to file entities based on published status.
 */
function file_admin_file_entity_access($op, $file, $account) {
  // Deny view access to unpublished files unless the user has "bypass file
  // access" permission or has "view own published files" and is the author of
  // the file.
  if ($op == 'view' &&
    $file->published == FILE_NOT_PUBLISHED &&
    !user_access('bypass file access', $account) &&
    !(user_access('view own unpublished files', $account) && $account->uid == $file->uid && $account->uid != 0)) {
    return FILE_ENTITY_ACCESS_DENY;
  }
  return FILE_ENTITY_ACCESS_IGNORE;
}
btopro’s picture

Not really sure where to post this but for reference, the way that this is implemented is non-standard relative to other entity access implementations. #1858338: file entity can't be queried because of an inconsistency in the file entity access callback found this but for reference, nodes, users, and field_collection_items all have an entity_access callback that allow for a permission based return of TRUE that overrides all other forms of access control. As the Entity module also implements this in a non-standard way because it doesn't want to add the permission for file serving that overrides all other conditions I'll probably add this in a contrib project.

#1136356: Fix file access indicates entity APIs method for handling file access delegation. I will cross post this thread with that one, unfortunately both are closed at this time.

try calling entity_access('view', 'file') without passing a file, there's no method to return TRUE. While this makes sense to me, the other entity_access routines I've looked at have at least the potential of returning a generic Yes for super users (for example).

Threads;
Entity thread - #1136356: Fix file access
RestWS thread - #1858338: file entity can't be queried because of an inconsistency in the file entity access callback

btopro’s picture

Issue summary: View changes

Fix update numbering.

kenorb’s picture

Assigned: ParisLiakos » Unassigned
Issue summary: View changes