Add a file entity access API

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

Related core issue: #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter().

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.

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).

+++ 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.

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.

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?

adding tag that we need tests

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

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).

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).

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

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

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

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 )

#27: file_entity-access_api-1227706.patch queued for re-testing.

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 @@@

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.

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 ...

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.

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.

@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

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

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.

@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.

Patch #36 works great for me. Thanks!

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)

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

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.

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).

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

Like this you mean?

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

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

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.

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

@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.

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........

@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.

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.

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

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)

Re-rolling #57 against unstable6.

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

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

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().

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?

is add/create files permission contemplated on this issue?

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

#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.

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

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',
  );

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

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()

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

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.

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

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

#1763024: New permission 'delete files in use'

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.

grr forgot to set for review

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

@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.

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!

@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 ^_^

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

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.

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.

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

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

Marked #1697152: Authenticated role able to see all files as a duplicate.

This patch will affect code in #1778370: Edit and Delete links as Views Field handlers.

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.

#91: file_entity-file-access-1227706-91.patch queued for re-testing.

reroll

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

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.

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.

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?

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:/

@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.

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

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".

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?

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

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?

@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.

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!

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

Rerolled

FILE_DEFAULT_ALLOWED_EXTENSIONS is used in file_entity_add_upload_multiple_submit, changed this to FILE_ENTITY_DEFAULT_ALLOWED_EXTENSIONS

#113: file_entity-file-access-1227706-113.patch queued for re-testing.

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

#116: file_entity-file-access-1227706-116.patch queued for re-testing.

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?

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

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.

this takes care #119

#122: file_entity-file-access-1227706-122.patch queued for re-testing.

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

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.

Provide rudimentary summary. Needs a lot of fleshing out.

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

hmm yeah

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

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).

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

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.

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.

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.

thanks all!
commited
now lets fix media

Flipping right!! Thanks all!!!

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;
}

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

Fix update numbering.