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?

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

.

Revised patch - will summarize tonight. :)

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.

$node presumably should be $file.

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

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.

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

Revised patch.

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?

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

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.

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.

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

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 )

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 ?

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.

@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

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.

Patch #36 works great for me. Thanks!

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.

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.

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

Like this you mean?

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

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

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

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.

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)

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

There was a logic error in file_access:

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

is add/create files permission contemplated on this issue?

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

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

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

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

Assigning to myself to finish and commit.

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

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

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.

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

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!

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.

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.

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

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

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

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.

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

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

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.

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:

why do we need this?

also:

shouldnt we use 72XX numbering already?

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

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

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?

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?

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!

Rerolled

The last submitted patch, file_entity-file-access-1227706-113.patch, failed 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.

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

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

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 guess you have tried this patch again and run the 71XX update which did the very same thing?

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.

thanks all!
commited
now lets fix media

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

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