Ensure file query alteration is performed when using EntityFieldQuery

I was just writing a patch for #2073085: Issues with _file_entity_query_file_entity_access_alter(). It only expects inner joins. that seemed related before noticing that the code effecting the EntityFieldQuery was commented out a month ago... http://drupalcode.org/project/file_entity.git/commit/5f4e8a9

I am using the Relation modules RelationQuery, which extends FieldEntityQuery. This is tagged with 'entity_field_access' in field_sql_storage_field_storage_query() via RelationQuery::execute()

The base table $base_table = $query->getMetaData('base_table'); was picked up as 'field_data_endpoints' and this sets off the "INNER JOIN file_managed fm ON fm.fid = field_data_endpoints0.entity_id".

Note that there are two inner joins in total from two Relation entity type fields that match the base table.

Without much knowledge of things here, maybe exclude the queries base table unless this is file managed?

  // Do not use the base table for general entity queries unless this is
  // querying the file_managed table directly.
  if ($base_table && $type == 'entity' && $base_table != 'file_managed') {
    $base_table = '';
  }

  // If no base table is specified explicitly, search for one.
  if (!$base_table) {
    ...
    // If there is nothing else, use the fallback.
    if (!$base_table) {
      if ($fallback) {
        ...
      }
      else {
        // Ignore this query as it was a general field query and no
        // relationships were found to the file_managed table. 
        if ($type == 'entity') {
          return;
        }
        throw new Exception(t('Query tagged for file access but there is no fid. Add foreign keys to file_managed.fid in schema to fix.'));
      }
    }
  }

Is there any known test scenario to test the patch? Shouldn't we create test covering the access check for altered queries?

I'm currently writing tests for the query alteration as review of the patch. Mu current plan for the test is to use the following query for the 'view own files', 'view own private files', 'view files', 'view private files' and 'bypass file access' permissions.

    $results = (new EntityFieldQuery())
      ->entityCondition('entity_type', 'file')
      ->entityCondition('entity_id', $fids)
      ->execute();

The attached patch adds (currently failing) tests for the file entity access check in EntityFieldQuery.

Here is an update of the patch, all the tests should pass now.

Here are the issues within the query alteration that I remember having fixed today:

• When doing a EFQ that does not uses the {file_managed} table, the field data storage table wasn't found as base table. Which triggered a early exist of the function with an unaltered query.
• When a use had no file permissions at all, not condition where added to the subquery on {fm_access} table, so no access check where added. The first table that looks like a field data storage table will now be used as base table.
• When the base table was {file_managed}, entity_type and entity_id

were added as column of the base table.

There is currently not tests to cover the alteration of a non-EFQ query. The same logic is used for both, we may want to write test to avoid creating regressions with this patch. There is also no test to ensure we don't break non-file entity EFQ (and I've not tested for it manually neither). Again, we may want to write such a tests to avoid regression.

reroll for testing

minor changes to satisfy lint test

maybe more necessary.

this time should help

php 5.6 , not related to this issue, just testing.

ok, this time I think I got it licked.

see interdiff from patch 6 to patch 26.

In my local environment with php 5.6.x , patch #26 passes every test.

committed to 7.x-2.x dev , will be in a tagged release shortly

This commit appears to cause regressions in organic groups permissions when used with the openatrium 2.623

I'm going to create a patch for openatrium to roll this back

see #2932229: Unable to create or edit a Document Page in Section unless administrator

For openatrium or others using file_entity with organic groups; Here is the patch to revert this so that openatrium works correctly with the latest file_entity (>= 7.x-2.0 )

the revert patch is intended for 7.x-2.12

If you have an issue like when using organic groups and cannot create a document unless admin
or get an error similar to
Column not found: 1054 Unknown column 'field_data_og_group_ref0.entity_id' in 'where clause'

then most likely you'll need to use patch 35 which backs off the EntityFieldQuery altering.

Due to breaking contrib alter EFQ:
#2932229: Unable to create or edit a Document Page in Section unless administrator
I'm planning to apply the revert patch to revert this functionality this weekend and publish a new release 7.x-2.13.

For now, rather than fully reverting this commit, mitigating this problem, committed a patch that deals with the regression with organic_groups (used by openatrium 2) .

If you experience a similar problem that you think might be related first upgrade to 7.x-2.14 of file_entity
if that doesn't help then use the revert patch I provided above.File query alteration conflicts with OGthat we know of

right now the plan is to apply the revert patch to the 2.x branch for the next release 2.16

however leave it in the 3.x branch in case someone opens a related support request we can direct them to the appropriate 3.x release , future development of this functionality should focus on 3.x , remove it from 2.x as it seems a bit risky.

Tested against openatrium use case, in that case the _file_entity_query_file_entity_access_alter($query, 'entity'); is where the regression comes in, simply commenting that call out avoids the regression.

Here's a patch for review by the testbot.

Ok, due to the lack of interest in this functionality and the fact that it's proven to cause at least one known regression I am going to apply the revert patch 35 and set this issue back to needs work.

I'll release the next file_entity without this functionality as I am not sure of anyone that was actually using it.

Reverted functionality.
tagged 7.x-2.16

setting this back to needs work