Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

We also need to consider moving the following code into a hook_query_file_access_alter():

  if (user_access('bypass file access')) {
    // Administrators don't need to be restricted to only permanent files.
    $query->condition('fm.status', FILE_STATUS_PERMANENT);
  }
  elseif (user_access('view files')) {
    // For non-private files, users can view if they have the 'view files'
    // permission.
    $query->condition('fm.status', FILE_STATUS_PERMANENT);
  }
  elseif (user_access('view private files') && user_is_logged_in()) {
    // For private files, users can view private files if the
    // user is not anonymous, and has the 'view private files' permission.
    $query->condition('fm.uri', db_like('private://') . '%', 'LIKE');
    $query->condition('fm.status', FILE_STATUS_PERMANENT);
  }
  elseif (user_access('view own files')) {
    // For non-private files, allow to see if user owns the file.
    $query->condition('fm.uid', $user->uid, '=');
    $query->condition('fm.status', FILE_STATUS_PERMANENT);
  }
  elseif (user_access('view own private files') && user_is_logged_in()) {
    // For private files, users can view their own private files if the
    // user is not anonymous, and has the 'view own private files' permission.
    $query->condition('fm.uri', db_like('private://') . '%', 'LIKE');
    $query->condition('fm.uid', $user->uid, '=');
    $query->condition('fm.status', FILE_STATUS_PERMANENT);
  }

  foreach (array_keys(file_entity_get_hidden_stream_wrappers()) as $name) {
    $query->condition('fm.uri', $name . '%', 'NOT LIKE');
  }
Devin Carlson’s picture

Assigned: Dave Reid » Devin Carlson
Status: Active » Needs review
FileSize
6.15 KB

This has been on my list for a while, as it will help simplify file access restrictions in CKEditor Link File.

Devin Carlson’s picture

Status: Needs review » Fixed

Committed to 7.x-2.x.

Dave Reid’s picture

Status: Fixed » Needs work

You're going too fast for me to be able to make sure we have the relevant information. We need this added to views as well.

We need the following added to file_entity_views_data_alter():

  // Add access tag for all queries against file_managed.
  $data['file_managed']['table']['base']['access query tag'] = 'file_access';
Dave Reid’s picture

Priority: Normal » Major

This also fails if the main query doesn't alias the file managed table to 'fm'. Our query alter needs to do some introspection to the exiting tables and find the alias used for the file_managed table.

Laurent Ren’s picture

From my (duplicate) issue #1974002: EntityReference autocomplete field with file entity :

One way to remove 'fm.' alias is to use this piece of code :

$tables = $query->getTables();
  $alias = isset($tables['file_managed']['alias']) ? $tables['file_managed']['alias'] : 'fm';

(I let 'fm' as default alias)

Hope it helps.

Laurent Ren’s picture

Status: Needs work » Needs review
Scott Ellis’s picture

I'm pleased to report that Laurent's patch in #6 corrected the error I was getting from an entity reference field.

Dave Reid’s picture

Status: Needs review » Needs work

Hrm, I'm afraid we're going to have to also ensure that this works on EntityFieldQuery queries as well, which is going to take some more work. It looks like we might have to duplicate a lot of logic from _node_query_node_access_alter().

We're also missing the following change for Views:

diff --git a/file_entity.views.inc b/file_entity.views.inc
index 95e0b58..ea8b5a1 100644
--- a/file_entity.views.inc
+++ b/file_entity.views.inc
@@ -113,6 +113,9 @@ function file_entity_views_data() {
  * Implements hook_views_data_alter().
  */
 function file_entity_views_data_alter(&$data) {
+  // Add access tag for all queries against file_managed.
+  $data['file_managed']['table']['base']['access query tag'] = 'file_access';
+  // Override the filename field handler.
   $data['file_managed']['filename']['field']['handler'] = 'views_handler_field_file_filename';
 }
 
ayalon’s picture

For documentation:
The patch #2 makes the linkit module with files unusable. The patch #6 solves the problem.

http://drupal.org/node/1975176#comment-7355854

The module adds a status condition to the query:

  function getQueryInstance() {
    // Call the parent getQueryInstance method.
    parent::getQueryInstance();
    // Only search for permanent files.
    $this->query->propertyCondition('status', FILE_STATUS_PERMANENT);
  }

Error:
PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'fm.status' in 'where clause': SELECT file_managed.fid AS entity_id, file_managed.type AS bundle, :entity_type AS entity_type, NULL AS revision_id FROM {file_managed} file_managed WHERE (file_managed.status = :db_condition_placeholder_0) AND (file_managed.filename LIKE :db_condition_placeholder_1 ESCAPE '\\') AND (file_managed.type IN (:db_condition_placeholder_2)) AND (fm.status = :db_condition_placeholder_3) AND (fm.uri NOT LIKE :db_condition_placeholder_4 ESCAPE '\\') ORDER BY file_managed.filename ASC; Array ( [:db_condition_placeholder_0] => 1 [:db_condition_placeholder_1] => %test% [:db_condition_placeholder_2] => document [:db_condition_placeholder_3] => 1 [:db_condition_placeholder_4] => temporary% [:entity_type] => file ) in EntityFieldQuery->execute() (Zeile 1145 von \includes\entity.inc).

Reason: Hardcoded table alias "fm"

nonom’s picture

I had the same issue and #6 works for me.

Dave Reid’s picture

FileSize
8.3 KB

Here is the code I was attempting to work on to make our file access tag work the same as node access, for both direct queries, and EntityFieldQuery queries. This also includes the necessary change to ensure that any Views query gets automatically tagged with 'file_access'.

aaron’s picture

Status: Needs work » Needs review

Change the status to needs review.

ayalon’s picture

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

Status: Needs review » Reviewed & tested by the community

Tested and works! I'm using the latest patch with the linkit module.

Dave Reid’s picture

Status: Needs review » Needs work

The patch in #12 is definitely not ready to be committed.

markisatacomputer’s picture

The latest patch cleared up the aliasing problem for me. Thanks!

Anonymous’s picture

I can also confirm that linkit works with patched file_entity (patch from #12 applied cleanly to current dev).

aaron’s picture

Why is it not ready to commit, Dave Reid?

Dave Reid’s picture

Because the patch in #12 was not actually ready to go at all, it was a work in progress. Functions like file_entity_query_entity_field_access_alter() are empty and therefore access integration with EntityFieldQuery is not complete.

Niko_K’s picture

Will a solution to this be part of the dev version some day soon?
The patch in #6 fixes my problems with the linkit and also with filedepot_linkit module.

Devin Carlson’s picture

Status: Needs work » Needs review
FileSize
7.79 KB

An updated version of #12 which:

Status: Needs review » Needs work

The last submitted patch, add-file-access-tag-to-all-queries-1959260-22.patch, failed testing.

Devin Carlson’s picture

Status: Needs work » Needs review
FileSize
7.75 KB

Oops, forgot to merge in the latest changes.

Status: Needs review » Needs work

The last submitted patch, add-file-access-tag-to-all-queries-1959260-24.patch, failed testing.

Dave Reid’s picture

Wow this is really coming along Devin, nice work. Just a few failures to fix!

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
378 bytes

I think this resolves the query altering from all queries.

Status: Needs review » Needs work

The last submitted patch, 1959260-file-access-api-query-alters.patch, failed testing.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
8.23 KB

Try again.

Dave Reid’s picture

Title: Ensure a 'file_access' tag is used on all queries of outputted files » [Needs tests] Ensure a 'file_access' tag is used on all queries of outputted files
Issue tags: -7.x-2.0 alpha blocker +Needs tests, +7.x-2.0 beta blocker

ROCK ON. Committed #29 to 7.x-2.x just to unblock the alpha release. I did not write any tests, but I heavily tested this with Views, EntityFieldQuery, etc. This is now an beta blocker for tests.

jaydub’s picture

I just updated to the alpha release and an EntityFieldQuery that I had running successfully is now no longer returning results. I've dumped my queries in MySQL and can see that the query alter introduced here is the culprit.

My EFQ:

$query = new EntityFieldQuery();

$query->entityCondition('entity_type', 'node')
  ->entityCondition('bundle', 'csm_user_review')
  ->propertyCondition('status', 1)
  ->fieldCondition('field_reference_csm_review', 'target_id', $nid, '=')
  ->propertyOrderBy('nid', 'ASC');

So just a basic query for nodes of type csm_user_review where the entityreference field_reference_csm_review is the nid for a given node. This query should return 702 rows. It returns 0 rows now. The SQL before this patch would be something like:

SELECT 
field_data_field_reference_csm_review0.entity_type AS entity_type, 
field_data_field_reference_csm_review0.entity_id AS entity_id, 
field_data_field_reference_csm_review0.revision_id AS revision_id, 
field_data_field_reference_csm_review0.bundle AS bundle
FROM 
{field_data_field_reference_csm_review} field_data_field_reference_csm_review0
INNER JOIN 
{node} node ON node.nid = field_data_field_reference_csm_review0.entity_id
WHERE
(field_data_field_reference_csm_review0.field_reference_csm_review_target_id = :db_condition_placeholder_0) AND
(field_data_field_reference_csm_review0.deleted = :db_condition_placeholder_1) AND 
(node.status = :db_condition_placeholder_2) AND
(field_data_field_reference_csm_review0.entity_type = :db_condition_placeholder_3) AND 
(field_data_field_reference_csm_review0.bundle = :db_condition_placeholder_4) 
ORDER BY 
node.nid ASC

where the placeholders are target_id = 1261672, deleted = 0, status = 1, entity_type = node, bundle = csm_user_review

The rewritten query is:

SELECT
field_data_field_reference_csm_review0.entity_type AS entity_type, 
field_data_field_reference_csm_review0.entity_id AS entity_id, 
field_data_field_reference_csm_review0.revision_id AS revision_id, 
field_data_field_reference_csm_review0.bundle AS bundle
FROM
field_data_field_reference_csm_review field_data_field_reference_csm_review0
INNER JOIN
node node ON node.nid = field_data_field_reference_csm_review0.entity_id
INNER JOIN
file_managed fm ON fm.fid = field_data_field_reference_csm_review0.entity_id
WHERE
(field_data_field_reference_csm_review0.field_reference_csm_review_target_id = '1261672') AND
(field_data_field_reference_csm_review0.deleted = '0') AND 
(node.status = '1') AND
(field_data_field_reference_csm_review0.entity_type = 'node') AND 
(field_data_field_reference_csm_review0.bundle = 'csm_user_review') AND 
(
  (
  (fm.uri NOT LIKE 'temporary%' ESCAPE '\\') AND 
  (fm.uri NOT LIKE 'module%' ESCAPE '\\') AND 
  (fm.uri NOT LIKE 'theme%' ESCAPE '\\') AND 
  (fm.uri NOT LIKE 'profile%' ESCAPE '\\') AND 
  (fm.uri NOT LIKE 'library%' ESCAPE '\\') AND 
  (field_data_field_reference_csm_review0.entity_type = 'file')
  ) 
  OR 
  (field_data_field_reference_csm_review0.entity_type <> 'file')
)
ORDER BY 
node.nid ASC;

This query returns 0 rows.

IF I change the JOIN to file_managed from INNER JOIN to LEFT JOIN, I get back the expected 702 rows.

At this point I am not sure if the query I had written should expect to be rewritten here as I had no interest whatsoever in access to files via my EFQ so it's a bit disconcerting that this query alter is happening at all but I can't claim to understand the recent changes.

aaron’s picture

Status: Needs review » Needs work
czigor’s picture

I have the same issue as in #31 with the following EFQ:

$result = $query->entityCondition('entity_type', 'commerce_product')
    ->entityCondition('bundle', 'ticket')
    ->propertyCondition('status', 1)
    ->fieldCondition('field_concert', 'target_id', $concert_nid)
    ->fieldCondition('field_seat', 'target_id', $seat_nids)
    ->execute();

When running this query as anonymous user the result set is empty because the file_managed table gets inner joined in.

For uid1 it gives the correct result set without the file_managed INNER JOIN.

If I comment out the two fieldCondition lines the INNER JOIN is gone and I get the correct result set.

czigor’s picture

Status: Needs work » Needs review
FileSize
558 bytes

Maybe it's this simple?

mkalkbrenner’s picture

Title: [Needs tests] Ensure a 'file_access' tag is used on all queries of outputted files » Ensure a 'file_access' tag is used on all queries of outputted files => alpha1 breaks EntityFieldQuery not releated to files
Category: task » bug
Priority: Major » Critical
Status: Needs review » Needs work

As described in #33, the alpha1 release that includes this feature breaks every EntityFiledQuery tha is not related to File Entities.

Due to the popularity of the Media module, that's a critical issue that needs to be fixed ASAP!

Every contrib module that uses EntityFieldQuery is broken!

The patch in #34 is not the right solution.

Dave Reid’s picture

Assigned: Devin Carlson » Dave Reid

It's strange because I haven't actually been able to duplicate this yet locally. I'm working on a set of extensive tests to confirm that this either is working or not, but likely not.

mkalkbrenner’s picture

I run this script with drush scr query.php:

$query = new EntityFieldQuery();
$query->entityCondition('entity_type', 'node')
  ->entityCondition('bundle', 'page');

var_dump($query->execute());

Results:

  • file_entity module disabled: all "pages"
  • file_entity module enabled, no file entities in database: no "pages"
  • file_entity module enabled, some file entities in database: some "pages"

The third test returns an incomplete list of pages. It's not a random list. Due to the join with the table file_managed on fid=entity_id, it returns all pages having the same entity_id than a fid of a file managed by file_entity but is not related to the page at all.

If you have many of files, than you won't face the error.

From my point of view the problem is caused by function file_entity_query_entity_field_access_alter(). It must not add the join of table file_managed if the entity is something else than a file_entity.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
0 bytes

Ok I think this should resolve the major issue with EntityFieldQuery. Not sure why I didn't do this part in the same style as the node query altering. Also, it made me question some of the conditions we had inside the query alter, if the user has the 'view own files' permission, we were restricting the listing to only files the user can view. That permission itself doesn't necessarily mean that the user doesn't have the 'view all files' permission, so for now I'm going to uncomment them out and leave in the 'hidden' stream wrapper condition which is valid.

Dave Reid’s picture

FileSize
3 KB

Wrong patch.

Status: Needs review » Needs work

The last submitted patch, 1959260-fix.patch, failed testing.

Murz’s picture

Status: Needs work » Needs review

Confirm this issue. Patch from #39 solves it, but as I see it always adds sub-query for each EntityFieldQuery execution, but seems that it is not necessary, and slow-down query, because it executes on entities that don't use any file links or fields.

For example, I have entity (created via ECK module) with only one custom text field, not any file_entity dependencies.
So I do the query:

    $equery = new EntityFieldQuery;
      ->entityCondition('entity_type', 'workflow_log')
      ->fieldCondition('field_order_status', 'value', 'test')
      ->execute();

and all works quckly via simple query:

SELECT field_data_field_commerce_order0.entity_type AS entity_type, field_data_field_commerce_order0.entity_id AS entity_id, field_data_field_commerce_order0.revision_id AS revision_id, field_data_field_commerce_order0.bundle AS bundle FROM {field_data_field_commerce_order} field_data_field_commerce_order0 WHERE (field_data_field_commerce_order0.field_commerce_order_target_id = :db_condition_placeholder_0) AND (field_data_field_commerce_order0.deleted = :db_condition_placeholder_1) AND (field_data_field_commerce_order0.entity_type = :db_condition_placeholder_2)

But when I enable file_entity module, it adds sub-query for searching files:

SELECT field_data_field_order_status0.entity_type AS entity_type, field_data_field_order_status0.entity_id AS entity_id, field_data_field_order_status0.revision_id AS revision_id, field_data_field_order_status0.bundle AS bundle FROM field_data_field_order_status field_data_field_order_status0 WHERE (field_data_field_order_status0.field_order_status_value = :db_condition_placeholder_3) AND (field_data_field_order_status0.deleted = :db_condition_placeholder_4) AND (field_data_field_order_status0.entity_type = :db_condition_placeholder_5) AND(( ( EXISTS (SELECT fm_access.fid AS fid FROM file_managed fm_access WHERE (fm_access.uri NOT LIKE :db_condition_placeholder_6 ESCAPE '\\') AND (field_data_field_order_status0.entity_id = fm_access.fid) )) AND (field_data_field_order_status0.entity_type = :db_condition_placeholder_7) )OR (field_data_field_order_status0.entity_type <> :db_condition_placeholder_8) )

Why it try to search file if this entity isn't use files?

Are you sure that we need to execute additional query for each entity? Maybe exists some easier way to detect is this entity have file dependencies?

Dave Reid’s picture

It should only add the query altering if there is a fieldCondition() on a field that is an image or file field. I'll investigate some more.

Dave Reid’s picture

FileSize
3.41 KB

I am getting super frustrated with EntityFieldQuery, and not understanding how this actually works or could ever work. This is my solution for now - disabling any alters for EntityFieldQuery() for now until we can figure out how to do it properly. This leaves in the query altering for anything tagged with 'file_access'.

Status: Needs review » Needs work

The last submitted patch, 1959260-fix.patch, failed testing.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
5.76 KB

Ok, this should work!

Dave Reid’s picture

HOORAY

Dave Reid’s picture

Status: Needs review » Fixed
Issue tags: -Needs tests

Ok committed to 7.x-2.x: http://drupalcode.org/project/file_entity.git/commit/5f4e8a9

Started follow-up for re-enabling the query alteration on EntityFieldQueries: #2067671: Ensure file query alteration is performed when using EntityFieldQuery

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