(I guess this is rather an issue for Entity module, but I'll let you folks do the proper routing)

The JSON and XML outputs of a node show empty entries for file fields, and only the 'tag' entry for image fields.

CommentFileSizeAuthor
#10 1136356-entity-file-access.patch1.62 KBklausi
#8 1136356-entity-file-access.patch1.47 KBklausi
#6 1136356-entity-file-access.patch1.44 KBklausi
#4 1136356-entity-file-access.patch1.4 KBklausi

Comments

Loocor’s picture

Loocor commented

Mark!

fago’s picture

fago
Primary language German
Location Vienna
commented
Title: No proper output of file and image field types » Fix handlinge of files

hm, in my test it did output something. E.g. this is the xml output I get of a node with a file and an image attached: 

<?xml version="1.0" encoding="utf-8"?>
<node><nid>3</nid><vid>3</vid><is_new></is_new><type>article</type><title>asdfdsf</title><language>und</language><url>http://mysite.com/node/3</url><edit_url>http://mysite.com/node/3/edit</edit_url><status>1</status><promote>1</promote><sticky>0</sticky><created>1294913241</created><changed>1303982320</changed><author resource="user" id="1">http://mysite.com/user/1</author><log></log><revision></revision><comment>2</comment><comment_count>1</comment_count><comment_count_new>0</comment_count_new><body><value>&lt;p&gt;test2&lt;/p&gt;

</value><summary>&lt;p&gt;ha&lt;/p&gt;
</summary><format>filtered_html</format></body><field_tags/><field_image><file resource="file" id="2">http://mysite.com/file/2</file><alt></alt></field_image><field_test>1</field_test><field_file><item><file resource="file" id="3">http://mysite.com/file/3</file></item></field_file></node>

It looks like the "http://mysite.com/file/3" path doesn't work properly though.

fago’s picture

fago
Primary language German
Location Vienna
commented
Title: Fix handlinge of files » Fix handling of files
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Title: Fix handling of files » Fix file access
Project: RESTful Web Services » Entity API
Component: Code » Core integration
Status: Active » Needs review
StatusFileSize
new 1136356-entity-file-access.patch1.4 KB

We need to fix the file access handling in the entity API module, thanks fago for the idea. Patch attached.

This should fix the issue with file fields in RESTWS, too.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Needs work

1. Shouldn't we check $op and only give green lights in case $op == 'view'? I don't think the hook does deal with editing. Indeed, I think files are never edited but replaced by new ones. So returning FALSE should be fine then.

2. Let's make use of existing helpers, i.e. module_invoke().

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Needs review
StatusFileSize
new 1136356-entity-file-access.patch1.44 KB

New patch fixes that issues.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Needs work

patch fails tests, as $entity may be unset.

Trying to get property of non-object Notice callbacks.inc 615 entity_metadata_file_access() Exception
Trying to get property of non-object Notice callbacks.inc 615 entity_metadata_file_access() Exception
Trying to get property of non-object Notice callbacks.inc 615 entity_metadata_file_access()

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Needs review
StatusFileSize
new 1136356-entity-file-access.patch1.47 KB

Fixed that.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Needs work

hm, spotted another problem: If $account is set to another user, the function will still check access for the global user. Thus, I guess we need to return FALSE in that cases too - as we do not know access permissions for other users.

+      $result = module_invoke($module, 'file_download', $entity->uri);

This was confusing me, as there is no $entity->uri in general, but a $file->uri. So if $entity is $file, let's call it that way.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Needs review
StatusFileSize
new 1136356-entity-file-access.patch1.62 KB

ok, patch attached.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

FYI this hook only applies if the file is private or temporary, not any other schemes.

fago’s picture

fago
Primary language German
Location Vienna
commented

indeed, so probably we should bypass the hook and just return TRUE by default for public files. You can determine the scheme with file_uri_scheme().

I'm not sure how we should handle files of an arbitrary scheme though. We really cannot know whether the user has access to them, so maybe we should just return FALSE until we figure out something better.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Needs work
dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

My solution in file_entity.module has been to alter in 'uses file_download' into hook_stream_wrappers_alter() and then check the file's uri's stream wrapper for that property, if so, run the file_download() checks.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

Closed #1273334: Image and file fields are omitted from the response as duplicate.

larjohn’s picture

larjohn commented

Does this issue also include the fact that the image fields have no access callback?

[file] => Array
        (
            [label] => File
            [base table] => file_managed
            [entity keys] => Array
                (
                    [id] => fid
                    [label] => filename
                    [revision] => 
                    [bundle] => 
                )

            [static cache] => 
            [fieldable] => 
            [controller class] => DrupalDefaultEntityController
            [field cache] => 1
            [load hook] => file_load
            [bundles] => Array
                (
                    [file] => Array
                        (
                            [label] => File
                            [rdf_mapping] => Array
                                (
                                )

                        )

                )

            [view modes] => Array
                (
                    [entityreference_view_widget] => Array
                        (
                            [label] => Entity Reference View Widget
                            [custom settings] => 1
                        )

                    [token] => Array
                        (
                            [label] => Tokens
                            [custom settings] => 
                        )

                )

            [translation] => Array
                (
                )

            [schema_fields_sql] => Array
                (
                    [base table] => Array
                        (
                            [0] => fid
                            [1] => uid
                            [2] => filename
                            [3] => uri
                            [4] => filemime
                            [5] => filesize
                            [6] => status
                            [7] => timestamp
                        )

                )

            [token type] => file
            [plural label] => Files
            [description] => Uploaded file.
            [save callback] => file_save
            [deletion callback] => entity_metadata_delete_file
            [configuration] => 
        )
sepgil’s picture

sepgil commented
Status: Needs work » Reviewed & tested by the community

Today I tested the patch (using restws) and I noticed that it also worked public files.
Why?
Well, because the foreach loop returns true if the access is restricted. Since public files aren't affected by hook_file_download, the function will skip the loop and return true.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Needs work

I agree, we can just ignore the file scheme as our callback returns TRUE if nobody returned -1 in hook_file_download().

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Reviewed & tested by the community

Status change accident, sorry.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Reviewed & tested by the community » Fixed

Indeed, it shouldn't harm to run it for public files as well. At worst, it will return FALSE - similar to the situation now. Thus, this can be only an improvement, so I've committed #10. Thanks!

Status: Fixed » Closed (fixed)

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

btopro’s picture

btopro commented

Not sure where to post this so I'll leave a note that based on the way this is setup it's not possible to use this call to answer the question "can a user view files" in a generic sense. The user / node access callbacks have this capability in the form of a permission that overrides all other access checks, for consistency I think this one should as well.

File Entity thread - #1227706: Add a file entity access API
RestWS thread - #1858338: file entity can't be queried because of an inconsistency in the file entity access callback