Downloads broken for translated private files

Add "beta blocker" tag
It is important to fix this i18n bug
so file_file_download() function can manage such a simple thing as calling the correct translated file

Not all important bugs are beta blockers; there's a specific set of criteria for beta blockers (critical issues that affect the data model or change the public entity, routing, plugin, caching, or configuration APIs). Also, the beta blocker tag should only be added by (or with) a core maintainer. Thanks!

I've been testing this one and can confirm the issue. But it is actually much worse than the original report (which only talks about the file API not working as expected).

This issue also blocks completely downloading private files when they are translated, resulting in an access denied page.

To reproduce it:
- Follo steps 1 to 6 of the report, but making the field a private file.
- Visit the node page in the translated language, click the file download link -> Access denied.

I can confirm the patch, though not the nicest solution, works and fixes both issues, with private and public files.
A better fix would mean actually fixing the file API, getting the 'file_usage' table to also store the language version of the field where it is used, but that's way more complex...

I also think this one should be raised to critical, since downloading of private files is just broken.

Updated title and issue description

A test to verify that access to private files attached to translated content is broken.

I'm also looking into the more advanced solution proposed in #18 but it would definitely be a larger change.

The code changes themselves look fine. Mostly nitpicks about the tests :)

1. +++ b/core/modules/file/file.module
   @@ -1865,6 +1865,23 @@ function file_get_file_references(FileInterface $file, FieldDefinitionInterface
   +          // because a file can be linked to a language other than default.
   

   s/than default/than the default/

2. +++ b/core/modules/file/file.module
   @@ -1865,6 +1865,23 @@ function file_get_file_references(FileInterface $file, FieldDefinitionInterface
   +            foreach($translations as $langcode => $language) {
   

   Missing space after foreach.

3. +++ b/core/modules/file/src/Tests/FilePrivateLanguageTest.php
   @@ -0,0 +1,125 @@
   + * Definition of Drupal\file\Tests\FilePrivateLanguageTest.
   

   s/Definition of Drupal/Contains \/

4. +++ b/core/modules/file/src/Tests/FilePrivateLanguageTest.php
   @@ -0,0 +1,125 @@
   + * Uploads private files to translated node and checks access.
   ...
   +class FilePrivateLanguageTest extends FileFieldTestBase {
   

   The sentence makes more sense to me than the test name. Perhaps PrivateFileOnTranslatedEntity makes more sense?

5. +++ b/core/modules/file/src/Tests/FilePrivateLanguageTest.php
   @@ -0,0 +1,125 @@
   +   * The name of the field used to test translation.
   

   Broken sentence.

6. +++ b/core/modules/file/src/Tests/FilePrivateLanguageTest.php
   @@ -0,0 +1,125 @@
   +  protected function setUp() {
   

   Missing {@inheritdoc}.

7. +++ b/core/modules/file/src/Tests/FilePrivateLanguageTest.php
   @@ -0,0 +1,125 @@
   +    // Create file field.
   

   … on page node.

8. +++ b/core/modules/file/src/Tests/FilePrivateLanguageTest.php
   @@ -0,0 +1,125 @@
   +    $permissions = array(
   +      'access administration pages',
   +      'administer content translation',
   +      'administer content types',
   +      'administer languages',
   +      'create content translations',
   +      'create page content',
   +      'edit any page content',
   +      'translate any entity',
   +    );
   +    // Create and login user.
   +    $this->web_user = $this->drupalCreateUser($permissions);
   

   Looks like the comment belongs above $permissions?

9. +++ b/core/modules/file/src/Tests/FilePrivateLanguageTest.php
   @@ -0,0 +1,125 @@
   +    // Verify node page field is translatable.
   

   s/field/file field/

10. +++ b/core/modules/file/src/Tests/FilePrivateLanguageTest.php
    @@ -0,0 +1,125 @@
    +    $field_name = $this->fieldName;
    

    Why not just use $this->fieldName everywhere?

11. +++ b/core/modules/file/src/Tests/FilePrivateLanguageTest.php
    @@ -0,0 +1,125 @@
    +    // Create an English node.
    +    $english_node = $this->drupalCreateNode(array('type' => 'page', 'langcode' => 'en'));
    

    I'd say "default language" instead of "English", and I'd even try to omit the explicit en langcode.

12. +++ b/core/modules/file/src/Tests/FilePrivateLanguageTest.php
    @@ -0,0 +1,125 @@
    +    // Languages are cached on many levels, and we need to clear those caches.
    +    $this->container->get('language_manager')->reset();
    +    $this->rebuildContainer();
    ...
    +    // Languages are cached on many levels, and we need to clear those caches.
    +    $this->container->get('language_manager')->reset();
    

    I'd understand this could be necessary when changing language settings. But here, this doesn't make any sense to me.

13. +++ b/core/modules/file/src/Tests/FilePrivateLanguageTest.php
    @@ -0,0 +1,125 @@
    +    $node = node_load($english_node->id(), TRUE);
    ...
    +    $english_node = node_load($english_node->id(), TRUE);
    

    Node::load()

14. +++ b/core/modules/file/src/Tests/FilePrivateLanguageTest.php
    @@ -0,0 +1,125 @@
    +    $node_file = file_load($node->{$field_name}->target_id);
    ...
    +    $node_file = file_load($french_node->{$field_name}->target_id);
    

    File::load()

Just a minor note, @Devin Carlson -- attach your test-only patch first, and then the full patch second. Testbot automatically retests the last parch in the issue once it's RTBC, so we want to make sure that's the real thing. :) Also that avoids the System Message NW noise for an expected fail. Thanks! And thanks @Wim Leers for the review.

An updated patch including all of the changes from #24, except that I haven't found a way to remove $this->rebuildContainer();.

This is also found in PathLanguageTest's testAliasTranslation():

    $this->drupalPostForm(NULL, $edit, t('Save (this translation)'));

    // Languages are cached on many levels, and we need to clear those caches.
    $this->container->get('language_manager')->reset();
    $this->rebuildContainer();
    $languages = $this->container->get('language_manager')->getLanguages();

    // Ensure the node was created.
    $english_node = node_load($english_node->id(), TRUE);

Could you please provide an interdiff?

Interdiff between #20 and #26.

+++ b/core/modules/file/src/Tests/PrivateFileOnTranslatedEntity.php
@@ -2,17 +2,20 @@
+class PrivateFileOnTranslatedEntity extends FileFieldTestBase {

Erm, should still end in Test. I see Wim suggested a specific name without test, but AFAIK it should still end in Test.

https://www.drupal.org/node/325974 The class name should end in "UnitTest" if it's a unit test (Unit or DrupalUnit), and just "Test" if it's a web-based test.

Sorry about that :(

1. +++ b/core/modules/file/file.module
   @@ -1865,6 +1865,23 @@ function file_get_file_references(FileInterface $file, FieldDefinitionInterface
   +            foreach ($translations as $langcode => $language) {
   +              $translation = $entity->getTranslation($langcode);
   +              if ($items = $translation->get($field_name)) {
   +                foreach ($items as $item) {
   +                  if ($file->id() == $item->{$field_column}) {
   +                    $match = TRUE;
   +                    break;
   +                  }
   +                }
   +              }
   +            }
   

   This break is not sufficient, it only breaks out of the inner foreach, while it should also break out of the outer one.

2. +++ b/core/modules/file/src/Tests/PrivateFileOnTranslatedEntity.php
   @@ -0,0 +1,127 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   +  public static $modules = array('language', 'content_translation');
   +
   

   This is overriding the parent property, so document it with {@inheritdoc}.

3. +++ b/core/modules/file/src/Tests/PrivateFileOnTranslatedEntity.php
   @@ -0,0 +1,127 @@
   +    $this->web_user = $this->drupalCreateUser($permissions);
   +    $this->drupalLogin($this->web_user);
   

   Use lowerCamelCase for properties. Also declare and document this property.

4. +++ b/core/modules/file/src/Tests/PrivateFileOnTranslatedEntity.php
   @@ -0,0 +1,127 @@
   +  /**
   +   * Tests file access for file uploaded to a private node.
   +   */
   +  function testPrivateLanguageFile() {
   

   It's not the node that is private, but rather the file.

Fixed my remarks and Gábor's remark from #29 + some more small fixes:

1. Used Entity::getTranslations() instead of Entity::translations() which was throwing fatal errors since that method doesn't exist.
2. Moved an assert out of the setUp() into the test itself.
3. Fixed code sniffer warnings.
4. Some documentation updates.

Looks good except the following things I found:

1. +++ b/core/modules/file/src/Tests/PrivateFileOnTranslatedEntityTest.php
   @@ -0,0 +1,126 @@
   +    // Languages are cached on many levels, and we need to clear those caches.
   +    $this->rebuildContainer();
   

   This looks odd at this point. Eg. this same code does not appear after the second file is uploaded. Should this happen after the new language is added? Not sure what cache is this meant to clean?

2. +++ b/core/modules/file/src/Tests/PrivateFileOnTranslatedEntityTest.php
   @@ -0,0 +1,126 @@
   +    $french_node = $default_language_node->getTranslation('fr');
   +    $this->assertTrue($default_language_node->hasTranslation('fr'), 'Node found in database.');
   

   This is valid but looks odd. If there was no fr translation, getTranslation() returns a new one. I would do the getTranslation() after this assert to be in a logical order. (This does not change anything about the behaviour just how the code reads).

3. +++ b/core/modules/file/src/Tests/PrivateFileOnTranslatedEntityTest.php
   @@ -0,0 +1,126 @@
   +    // Ensure the file attached to the translated node can be downloaded.
   +    $node_file = File::load($french_node->{$this->fieldName}->target_id);
   +    $this->drupalGet(file_create_url($node_file->getFileUri()));
   +    $this->assertResponse(200, 'Confirmed that the file attached to the French node can be downloaded.');
   

   This does not ensure the two file target ids are different, that in fact this was a translated file download :)

I don't think this is a lot of work to do, hopefully not a big setback?

Needed a few changes after #2304969: Port private files access bypass from SA-CORE-2014-003.

1. +++ b/core/modules/file/src/Tests/PrivateFileOnTranslatedEntityTest.php
   @@ -0,0 +1,126 @@
   +    // Languages are cached on many levels, and we need to clear those caches.
   +    $this->rebuildContainer();

   I still haven't managed to track down why the container rebuild is necessary (as is done in testAliasTranslation). I get a InvalidArgumentException: Invalid translation language (fr) specified. if it is removed.

2. +++ b/core/modules/file/src/Tests/PrivateFileOnTranslatedEntityTest.php
   @@ -0,0 +1,126 @@
   +    $french_node = $default_language_node->getTranslation('fr');
   +    $this->assertTrue($default_language_node->hasTranslation('fr'), 'Node found in database.');

   Moved.

3. +++ b/core/modules/file/src/Tests/PrivateFileOnTranslatedEntityTest.php
   @@ -0,0 +1,126 @@
   +    // Ensure the file attached to the translated node can be downloaded.
   +    $node_file = File::load($french_node->{$this->fieldName}->target_id);
   +    $this->drupalGet(file_create_url($node_file->getFileUri()));
   +    $this->assertResponse(200, 'Confirmed that the file attached to the French node can be downloaded.');

   I'm not sure I follow 100%. Should we verify that the default language and french nodes are using different files? If so, I added a check to the attached patch.

@Devin: yeah I had checking that the two files are different in mind, so that the upload of files worked and is indeed different per language. The test before that only ensured that the files could be downloaded but they could still be the same. Thanks for that update.

As for the container, I would reset the language manager instead as an attempt. \Drupal::languageManager()->reset(); I think that would be enough to help with the entity translation stuff. The reason that fails is the language list is cached in the language manager and is not reset for you since you use the API to access the translation but the UI to add the language. Probably better: use ConfigurableLanguage::save() to save a new language (after instantiated). That would clear the cache I guess.

Looking at ConfigurableLanguage.php it does not seem like save() would clear the languageManager internal language list cache. BTW #1879930: Language selectors are not showing localized to the page language is removing that cache for good, so if that is committed before this one lands, then the cache clear may not be needed. So I would try with \Drupal::languageManager()->reset(); then instead of the whole container clearing. That may not be enough but worth a try to be less intrusive :)

#1879930: Language selectors are not showing localized to the page language did not end up removing the cache, in fact it added a per language cache. The clearing will be needed. Did you try with \Drupal::languageManager()->reset(); ?

Retested #39 against the latest HEAD.

With only languageManager()->reset(), the test fails with array_filter() expects parameter 1 to be array, null given in Drupal\content_translation\Controller\ContentTranslationController->overview().

$entity->translation is NULL instead of

array (
  'en' => 
  array (
    'source' => '',
    'outdated' => '0',
    'uid' => '3',
    'status' => '1',
    'created' => '1415127760',
    'changed' => '1415127760',
  ),
)

I tried to run the patch locally but it segfaults my local setup (in its curently posted unmodified form):

[Wed Nov 05 15:37:27 2014] [warn] [client 127.0.0.1] mod_fcgid: error reading data, FastCGI server closed connection
[Wed Nov 05 15:37:27 2014] [error] [client 127.0.0.1] Premature end of script headers: index.php
[Wed Nov 05 15:37:28 2014] [error] mod_fcgid: process /Applications/Dev Desktop/php5_4/bin/php-cgi(97108) exit(communication error), get unexpected signal 11

So I could not try moving the kernel rebuilding around. I would have done it at the end of setUp() or right after adding French in setUp(). Not in the middle of the test function. There does not seem to be anything in that test function that necessitates a kernel rebuild, no?

Anyway, this is a minor nit compared to how important this bugfix and test is, so I am not going to keep blocking it. It looks great overall.

(BTW it is not just this test that segfaults for me, other existing file upload tests in core, eg. the responsive image field test does the same. So I am sure that is not the fault of this test, rather bugs in my environment).

I had a play with moving the container rebuild around. The most likely issue here is that the test running environment is getting out of sync with the child environment.

This issue addresses a critical bug and is allowed per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?. Committed c6742dd and pushed to 8.0.x. Thanks!

Thanks all!

By the way, the Drupal 7 version of this patch is at #2112287: file_file_download() does not specify language when calling field_get_items(). Please help with the port there.