Add language support to node access grants and records

I can dedicate time for this issue

I attached the first version of the patch. Pls review it!

Kalman

Thanks Gabor, I attached a new one.

attached is a non-functioning tests and patch of what I was working on. I basically copied assertNodeAccess function, and testNodeAccess, and started reworking the tests to work with node_access_grants function. I'm leaving for vacation this weekend for 2 weeks, so I won't be able to work on this anymore until I get back. Un-assigning in case someone else wants to pick up where I left off.

I'm back from my holiday.

I will able to continue the work on this after wednesday.

working on this

here a first version which was discussed with Gabor:

- There is a new column in the node_access called "fallback", this defines which node_access record should be used if no language metatag is defined in db_select or elsewhere.
- The record with the same language as the original node language ($node->langcode) will be the fallback.
- There is a new Metadata for db_select called "langcode", if this is set the node_access table will use the records with this language. If langcode is not set it uses the fallback records

Open Todos

- right now the fallback column is only used with db_select, have to check if the node_access table is queried elsewhere
- is "langcode" the right Metadata key? Or should we use "access_langcode"?
- It does not test yet what happens if there are two node_access records with the same Nid but different languages. How should we test this? Because as far as I know this is not yet possible via the UI and also not really via code.

new patch with fixes from @gabors mentions, thanks for review!

the LANGUAGE_NOT_SPECIFIED issue is here: #1738368: Not possible to use the entity getter to retrieve non-translatable field values

Still needs upgrade path tests

just did my own review and found this:

function hook_install() {
  // Populate the default {node_access} record.
  db_insert('node_access')
    ->fields(array(
      'nid' => 0,
      'gid' => 0,
      'realm' => 'all',
      'grant_view' => 1,
      'grant_update' => 0,
      'grant_delete' => 0,
    ))
    ->execute();
}

I would add fallback = 1, but what are we using for language? because if we add something (like default language) and a query is made with Metadata langcode, it could return no results, where actually this record is made for return all nodes all the time (eg: no node_access enabled)

--> own conclusion: I changed the default of the fallback column to be 1, so then the default record is set as fallback automatically.
I also checked node_access and I changed it to this:

      $nids = db_and()
        ->condition('nid', $node->nid)
        ->condition('langcode', $langcode);
      if ($node->status) {
        $nids = db_or()
                  ->condition($nids)
                  ->condition('nid', 0);
      }

so actually I'm only adding the langcode check for specific nid checks, not for the global nid=0 check.

attached new patch with node_access() change and fallback default

#19: drupal-1658846-node_access_grants-19.patch queued for re-testing.

because I updated node_access() this was obviously that it failed.
So the reason is, that for the requested languages there is no node_access record, so the user gets now an access denied.
which @gabor and I both agree that this is the expected behavior.

attached patch changes the test

I really dislike the use of a separate "fallback" column. Is it not possible to store that information in the langcode column?

If it is not, do we have a better, more consistent column name to use? (e.g. "default" or "is_default")

Note #2: The use of "langcode" is consistent with the rest of core. No need to change the name of that column (reference comment 15).

Looked at this more in-depth today, and I still don't understand the "fallback" column. In my tests, all node access records validate as "fallback". This is due to the logic used for determining fallback, which is:

if ($grants['langcode'] == $node->langcode)

I fail to see any cases where this might be not be true, which means that every {node_access} row is marked as 'fallback.' Instead, I am experimenting with setting the original node as the fallback. That code looks like so:

if (empty($node->tnid) || $node->nid == $node->tnid)

Here's a revised patch with that approach, plus an update to HEAD, and a fix for some if/else code format errors.

However, I also fail to see any cases where a record with a duplicate nid but a unique langcode can be saved. Is there something I'm missing regarding the translation system that would allow for that (say, selecting Multiple languages for a node)?

I see the benefit to adding langcode regardless, since it allows language-based access to site alongside other access control rules. (I have a module for testing that I need to sandbox to help others with this.)

See https://drupal.org/sandbox/agentrickard/1747908 for a sandbox module that implements node access for testing.

@agentrickard

I fail to see any cases where this might be not be true, which means that every {node_access} row is marked as 'fallback.'

yes you are perfectly right, right now I'm did not manage yet to write a test which would test against this, I mentioned this in http://drupal.org/node/1658846#comment-6365354 so I first have to check with fago how to create via the entity api another language for the node so that there are two records in node_access.

if (empty($node->tnid) || $node->nid == $node->tnid)

this will only work for content translation, but not with entity translation (and we try to get rid of content translation).

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageTest.php
@@ -60,30 +65,110 @@ class NodeAccessLanguageTest extends NodeTestBase {
+    $language = (object) array(
+      'langcode' => 'hu',
+    );
+    language_save($language);
+    $language = (object) array(
+      'langcode' => 'ca',
+    );
+    language_save($language);

Be carefully, if #1739994: Use the Language class universally instead of stdObj instances issue will be committed you must correct this lines use "new Language(array(..))" instead of "(object) array(..)"

sorry...

Well, for completeness' sake, you might as well make the change anyway, since it is the more correct way. Not marking "needs work", though.

yes, agree, we should do this, added the new Language object and also fixxed codestyle.
but did not add the changes from #26

@Schnitzel Yes. Gabor explained that to me in Munich. Backing away from this patch for now...

Right, but it is hard to test the desired behavior given that we don't want to use translation nodes.

@Gabor:

Do you think this can be achieved in contrib?

@Plach:

Yes this can be achieved in contrib, it will probably not as fast as in core (because additional joins from the node_access table), but it's possible yes.

Well, it's not critical then. And we might be able to classify this as clean-up/security improvement and work on it after feature freeze.

Ok, and what about

we might be able to classify this as clean-up/security improvement and work on it after feature freeze.

?

I am interested, it's just that I have more critical issues to work on :)

I'm interested in making this happen, but my understanding is that it is blocked because we can't write proper tests because the new translation system -- field based rather than node based -- isn't in yet.

Eg. if you want either French or German, and a node would not give you access to the French variant but would to the German variant, we need to filter it down to the German variant or deny access?

Please help me understand: without node access, if you ask for French or German and a node has both, would you see the node twice?

With node access, if only German were accessible, the node access system would have to return a list of accessible languages (with only 'de' in this case) rather than just a Boolean, right?

What about NA priority? Should it apply per nid or per nid+lang_code? Either one could be completely wrong, depending on your use case...

The case where German is accessible but French is not (even though a French version of the node exists) — is that really relevant in practice? What is the justification for that? Editorial workflow? That's not node access. published already exists as a language-specific property.

Is the German language protected, too? What keeps the user from simply switching to German? If he can do that, then node access is not the proper layer to implement this. Even if you could protect the German language, why would you want to do that?

You will really alienate users on a site if they find out that some content is available in one language but blocked in another (even though it exists!), meaning they have to constantly switch around languages or even maintain multiple accounts in order to get the whole picture.

I've re-read the summary of #1658814: Make node_access() language-aware, but I'm unconvinced that any site would really want to use language-specific node access, especially given its costs in overhead, complexity and maintenance. It's hard enough to keep track of which users have access to which nodes; who would want to multiply that burden and do this per language? and what for?

Sorry if this has been discussed before, but at the point where we are right now, it makes sense to take a step back and reconsider what we're trying to achieve.

Gabor-

The problem I ran into was that the old model of "Content Translation", wherein a translation of a node is, in fact, a new node, is something we want to discard. However, last I checked, there was no UI and no test framework for running checks for entity-based translations. That was the status we discussed in Denver (see comments 26-29 above).

If that framework is in place, then we have a query / data storage issue that we should be able to solve. But if we can't test entity translation using core UIs, then this can't move forward.

OK. This would unblock me. I'm traveling this week but hope to take a look this weekend. At the least, I can make some time at BadCamp to try to knock this out.

Straight re-roll of #34 to get moving again.

Have to rename the update hook.

The idea of 'fallback' needs serious consideration. Now that we have per-field translation, the access system doesn't quite know what to do with each field translation.

In my tests, the only workaround is as follows:

* Make the hook_node_access_records() return value sensitive to all active translations, using $node->getTranslationLanguages()
* Check for "fallback" status in the cases where:
** The node type is not translatable.
** The node langcode is "multiple"
** The node langcode is "not applicable"
** The node langcode is "not specified"

Here's the fugly code for that check:

/**
 * Determines if a translation should be the default for access.
 *
 * @param Drupal\node\Node $node
 *   The $node being written to. All that is necessary is that it contains a
 *   nid.
 * @param array $grant
 *   A single grant array taken from hook_node_access_records().
 *
 * @return Boolean
 *   TRUE if this node is default for all languages.
 */
function node_grant_is_fallback(Node $node, $grant) {
  // The node must be enabled for translation.
  if (!translation_entity_enabled('node', $node->type)) {
    return TRUE;
  }
  elseif ($node->langcode == LANGUAGE_NOT_APPLICABLE) {
    return TRUE;
  }
  elseif ($node->langcode == LANGUAGE_NOT_SPECIFIED) {
    return TRUE;
  }
  elseif ($node->langcode == LANGUAGE_MULTIPLE) {
    return TRUE;
  }
  return FALSE;
}

as discussed with xjm and Gabor at badcamp: I will update my patch to provide a new test node access module which is language aware to not change the current existing test node access module as most of the contrib node access module will not be language aware.

So I think we need to add test coverage for various combinations of languages (nodes that have no language, or that have one or multiple translations with different default languages) with:

• A node access module that ignores the language key and lets node take care of writing the grants.
• A node access module that writes specific grants per language per node.
• The combination of the two.

@Gábor Hojtsy and @Schnitzel and I talked about this awhile and we concluded that it's not possible to use grant realms to implement this functionality, but not for the reasons described in #42. While it would be possible to define grant realms per language and write grant records per language, it would only solve the problem of allowing a role granted the realm (e.g.) translator_de to edit any node that has a German translation, not the problem of allowing that role access to edit only the German translation of that node (and not the French).

Implemented everything as discussed with @xjm and @gabor:
- fixed wrong node_access Table Primary key and added it with langcode column
- added code that node_access() function does correctly load the langcode from the content negotiation
- added new node_access_test_language.module which is a new node_access module which is language aware, so it creates different grants for each language the node is translated to
- new Test NodeAccessLanguageAwareTest.php which tests node access with node_access_test_language.module

#62: interdiff-55-62.patch queued for re-testing.

#62: drupal-1658846-node_access_grants-62.patch queued for re-testing.

checked with @xjm and decided that we will also have to write some combined tests, so where a non language aware module and a language aware node_access module is installed, did this in NodeAccessLanguageAwareCombinationTest.
Also updated NodeAccessLanguageTest to test when actually a private node is created.
And also updated NodeAccessLanguageTest with tests when a node is created without a language defined (so as when no language module is installed).

oh, node install has changed.

adding tag for needs issue summary since this seems to be pretty involved at this point. :)

Rerolling to clean up some spinach. :)

@GaborHojtsy and @Schnitzel took the time to go over this issue with me in detail during BADCamp, and I'm fairly confident now that this is a good solution given the current node access implementation. The updated summary explains it pretty clearly.

The attached reroll adds the following fixes:

1. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageAwareCombinationTest.phpundefined
   @@ -0,0 +1,365 @@
   +  /**
   +   * Asserts node_access correctly grants or denies access.
   +   */
   +  function assertNodeAccess($ops, $node, $account, $langcode = NULL) {
   +    foreach ($ops as $op => $result) {
   +      $msg = t("node_access returns @result with operation '@op', language code @langcode.", array('@result' => $result ? 'true' : 'false', '@op' => $op, '@langcode' => !empty($langcode) ? "'$langcode'" : 'empty'));
   +      $this->assertEqual($result, node_access($op, $node, $account, $langcode), $msg);
   +    }
   +  }
   

   This method is repeated in all three tests classes, so I've moved it into the base class.

2. +++ b/core/modules/node/node.moduleundefined
   @@ -2771,8 +2771,24 @@ function node_access($op, $node, $account = NULL, $langcode = NULL) {
   -  if (empty($langcode)) {
   -    $langcode = (is_object($node) && isset($node->nid)) ? $node->langcode : '';
   +  if (empty($langcode) && (is_object($node) && isset($node->nid))) {
   +    if (module_exists('language')) {
   +      // Load language from content negotiation.
   +      $content_negotiation_langcode = language(LANGUAGE_TYPE_CONTENT)->langcode;
   +      // Load languages the node exists in.
   +      $node_translations = $node->getTranslationLanguages();
   +      // If the node does not exist in the language from content negotiation
   +      // return the default language of the node.
   +      if (isset($node_translations[$content_negotiation_langcode])) {
   +        $langcode = $content_negotiation_langcode;
   +      } else {
   +        $langcode = $node->langcode;
   +      }
   +    } else {
   +      $langcode = $node->langcode;
   +    }
   +  } else if (empty($langcode)){
   +    $langcode = '';
   

   The logic here was a bit convoluted, so I've rewritten it somewhat (see interdiff).

3. I clarified several parts of the patch's documentation.
4. There was also some missing required documentation (docblocks and parameter documentation) that I've added, as well as some nonstandard one-line summaries (which I've fixed). Summaries should be only one line of 80 characters or fewer, starting with a third-person verb.

The test coverage is excellent. One thing we might want to do is simplify the new node access test module a bit so that it doesn't rely on a field and a magic variable_set() flag. (The new module is modeled on the existing node_access_test.module, which could use its own cleanup anyway. As I recall, I started to do that once and ended up spending four hours just documenting how it works. Maybe a followup.)

I'll take another look at this patch tomorrow; been staring at it too long today. :)

Missed one.

Couple other cleanups I missed:

• http://drupal.org/simpletest-tutorial-drupal7#t
• Function and method names should always have parens in text, e.g. db_select().

Hm, the same fails from #78 occur with #69 rebased. Either something went funky with the rebase, or an intermediate commit has broken this patch.

This is some combination of "Ask and ye shall receive" and "be careful what you wish for": #1814402: Convert 'node_access_test_private' variable to state() system

Some search-and-replace cleanups (adding articles and periods, fixing capitalization, removing t() from assertion message texts, etc.).

• Does the update hook need a call to node access rebuild or at least to flag for rebuild?
• Why are we putting the pressure on implementing modules and not the API to run the $node->getTranslationLanguages() loop? Shouldn't that be in the storage API issue.

Why are we putting the pressure on implementing modules and not the API to run the $node->getTranslationLanguages() loop? Shouldn't that be in the storage API issue.

I'm confused by this comment. In this patch, the API takes care of it. Modules don't need to unless they want to. The fact that the new module here does is just part of its dummy implementation; the existing node access test module is unchanged, demonstrating that non-language-aware node access modules can just ignore it.

Edit: Maybe we need to change the test module so we have better coverage for the fallback functionality (and avoid this confusion).

Does the update hook need a call to node access rebuild or at least to flag for rebuild?

Yes probably. I thought it did but I guess not.

Refactoring the tests a bit.

I don't see that part of the API in the patch. I would expect to see a loop in node_access_acquire_grants() or _node_access_write_grants(), but I don't see one.

I think my confusion is that I expect all node access modules to be language aware. But I think what you're saying is that if the $grants don't specify a language, then we just assert a fallback and default langcode. This was one of the issues we discussed at BadCamp, and I think you have solved this problem.

Yep, the default value for the fallback column is 1, so things node access records are a fallback unless they aren't.

I simplified the test module significantly and refactored the tests for clarity. Not bothering with an interdiff because pretty much the whole tests have changed, though the same assertions are present.

A couple of the assertions demonstrate behavior I'm not sure about. I'll follow up more later.

Edit: #89 also still needs to be addressed.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageAwareCombinationTest.phpundefined
@@ -0,0 +1,267 @@
+    // If the node is marked public, any existing translations should be
+    // accessible, regardless of whether they are marked private. Access should
+    // only be denied when a translation that does not exist is specifically
+    // requested.
+    // With the Hungarian translation marked as private, but the node public:
+    $this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_hu_private'], $this->web_user);
+    $this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_hu_private'], $this->web_user, 'hu');
+    $this->assertNodeAccess($expected_node_access, $this->nodes['public_hu_private'], $this->web_user, 'ca');
+    $this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_hu_private'], $this->web_user, 'en');

So this is the part that seems suspect to me. In this case, the comment actually contradicts the assertions below it. (It's the same in the earlier versions of the patch.) The normal behavior for node access would be additive, such that when the non-language-aware node access module grants access without specifying a langcode, access is granted. Here, the fact that the langcode is not specified is resulting in language-aware node access modules always overriding non-language-aware ones, regardless of circumstance. (The node access query tag shows the same behavior.) This seems like a bug to me.

Yeah, I think that's what @agentrickard was getting at in #91 and at BADCamp. If a particular record in hook_node_access_records() does not specify a language code, I think we can assume it applies to all translations. So we'd need to either save a record for each language when no specific language is specified, or alter the logic in both node_access() and node_query_node_access_alter() somehow such that both records get selected.

I'd also like to add some additional coverage to see what happens when a language-neutral node is saved with one or both of the access control modules active.

That sounds like the loop I was talking about, and the query parameters I think I added in my last patch.

yes, will do!

So I tried to change it, but actually I'm more confused then before ^^

I agree that this:

    $this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_hu_private'], $this->web_user);
    $this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_hu_private'], $this->web_user, 'hu');
    $this->assertNodeAccess($expected_node_access, $this->nodes['public_hu_private'], $this->web_user, 'ca');
    $this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_hu_private'], $this->web_user, 'en');

looks confusing, especially because of this:

    $this->assertNodeAccess($expected_node_access, $this->nodes['public_ca_private'], $this->web_user);
    $this->assertNodeAccess($expected_node_access, $this->nodes['public_ca_private'], $this->web_user, 'hu');
    $this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_ca_private'], $this->web_user, 'ca');
    $this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_ca_private'], $this->web_user, 'en');

Short info:

$this->nodes['public_hu_private'] has this definitions:
- non-language aware node_access module: public
- language aware node_access module: hu private, ca: public
- Default language: hu <-- important!

$this->nodes['public_ca_private'] has this definitions:
- non-language aware node_access module: public
- language aware node_access module: hu public, ca: private
- Default language: hu <-- important!

So in the first case:
$this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_hu_private'], $this->web_user);
is denied for no language code, because the default language (which will be used as fallback) is denied.

in the second case:
$this->assertNodeAccess($expected_node_access, $this->nodes['public_ca_private'], $this->web_user);
is granted for no language code, because the default language is granted and this is used for as fallback.

So one thing we figured out during the D8MI Meeting in IRC today: the non-language aware node access module creates grantes only when the node is specifically private. If it is set to public then it returns no grants and depends on the Core behavior of creating an "all 1 1 1" grant. Whereas the language aware node access module creates grants for the node no mater if the node itself actually has one translation private.

So I could imagine three things:

1. Change the non-language aware node access module to return grantes even the node is public
   --> this would be a change from the current behavior so not the right thing to do
2. Change the language aware node access module to return grantes only if there is actually a translation for the current node which is public
   -->this would then fit with the non-language aware node access module but would not change something from the cases on top
3. Change the Core to create grant entries for each translation of the node, when some of them are missing in the passed in grants
   -->this could maybe be a good case and actually also my opinion, BUT: It would not change any of the cases below, the first case: $this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_hu_private'], $this->web_user); would still be denied access because there is an grant for this.

so you see I don't really see a case where changing this:

save records in all language codes if none was provided

would actually make a difference? :/

okay, so for me this sounds like, these two things are fine, but should we now still change the core behavior to: "save records in all language codes if none was provided" ?

should we now still change the core behavior to: "save records in all language codes if none was provided" ?

Doubling the number of records if any language (besides English) is installed is not so great, especially because it's unclear what percentage of sites really need language-specific node access.

Say you want to run a site in German — you're automatically punished, even though you have no English content and you couldn't care less about language-specific node access.

Can we find a better solution?

In the absence of complete and sensible UI translations, removing English is probably not a viable option, but it's a nice idea.

What about LANGUAGE_ALL, LANGUAGE_DEFAULT, LANGUAGE_NOT_APPLICABLE, LANGUAGE_NOT_SPECIFIED, LANGUAGE_SITE_DEFAULT?

@Gábor Hojtsy:

I'd avoid further discussing this unrelated issue here.

Yes, definitely, I didn't intend to do that.

Have you considered the special language codes? Even if we have nothing but German, we probably need a way to answer queries with LANGUAGE_ALL, for example. And if we have content with LANGUAGE_NOT_APPLICABLE, we need to answer to queries with 'de'.

this issue is listed as part of an example of a concrete use case meta issue #1860522: [META] support (basis for a contrib) Translation Editorial Workflow (support for revisions for translations of entities)

I can work on this in a few days if none wants to pick this up before.

Sorry, I don't think I'll be able to work on this earlier than a week. If someone else wishes to step-up feel free.

Rerolled patch from #94 "as is".

Add a grant for each language this node can be translated to if a specific language is not given in grants.

Quoting #111:

Back on topic, the work that seems to be left is that if no langcode is provided with a grant, core would save the grant for all langcodes applicable to the node not just the original langcode for the node.

If I understood, we want to add every langcode this node can be translated to.
My previous patch added a reference to translation_entity_enabled in _node_access_write_grants and this is not a valid call, because translation_entity could be disabled.

So maybe this functionality should be moved to translation_entity by implementing hook_node_access_records_alter and adding a new grant for each available language. Is this true of I'm missing the point?

Added a patch for showing my view, but I'm sure that more simpletests should be implemented if this is the right path to follow.
Interdiff is with #117, not #118.

sorry for my way too long non answering :/
But unfortunately everybody needs something from me right now, as I only have time to write at 0100...

But yes @penyaskito the idea is, that core adds a grant for each language of a node where any other module does not create a grant.
A really good point is what you mentioned in #120, but actually getTranslationLanguages() is a function of Entity and not of entity_translation. So why not just use getTranslationLanguages() and never check translation_entity_enabled()?
Because Multilanguage is something which Entity is capable of, Entity Translation only enables you better handling of this Multilanguage system.

Thanks for the feedback @Schnitzel!
Attached patch should work then, let's see if its back to green.

Oops, failures based on using the language object instead of langcode.

We should check that the entry for that node-grant-language does not already exist, or we get duplicate key errors as in those tests.

I'll work on it ASAP if no-one tries it before.

Added check and reworded comment.

mhh wait, why can it even happen that there are two grants for the same nodeID and the same translation in there?
All Grants for a Node should be deleted before the new Grants are created, so this should no happen. I think there is something else wrong with the array merge or so.

New patch for moving forward.

I were looping over the languages without looping in the grants first. Locally I have some test errors, but they look unrelated. Let's what the testbot has to say.

Removed forgotten debug statement.

Thanks @penyaskito. That looks like the solution we were discussing.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageAwareCombinationTest.phpundefined
@@ -0,0 +1,267 @@
+    // If the node is marked public, any existing translations should be
+    // accessible, regardless of whether they are marked private. Access should
+    // only be denied when a translation that does not exist is specifically
+    // requested.
+    // With the Hungarian translation marked as private, but the node public:
+    $this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_hu_private'], $this->web_user);
+    $this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_hu_private'], $this->web_user, 'hu');
+    $this->assertNodeAccess($expected_node_access, $this->nodes['public_hu_private'], $this->web_user, 'ca');
+    $this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_hu_private'], $this->web_user, 'en');
+
+    // With the Catalan translation marked as private, but the node public:
+    $this->assertNodeAccess($expected_node_access, $this->nodes['public_ca_private'], $this->web_user);
+    $this->assertNodeAccess($expected_node_access, $this->nodes['public_ca_private'], $this->web_user, 'hu');
+    $this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_ca_private'], $this->web_user, 'ca');
+    $this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_ca_private'], $this->web_user, 'en');
+
+    // With both translations marked as private, but the node public:
+    $this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_both_private'], $this->web_user);
+    $this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_both_private'], $this->web_user, 'hu');
+    $this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_both_private'], $this->web_user, 'ca');
+    $this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_both_private'], $this->web_user, 'en');

The comment here still contradicts the assertions in the tests. I expected some of these assertions to change as part of the "write grants for all languages" change. It might just be a result of the existing test node access module having unintuitive behavior, as @Schnitzel points out in #102. (I'd almost consider that a bug in that module.)

@Gábor Hojtsy is definitely right that the language access should not make any assumptions about what other modules do, but the result is that we're missing test coverage for the case where there are actually explicit "public" grant records written from another module, because the existing test module doesn't do that. The solution to this problem is to either add another test module, or to change the behavior of the existing module. That's a lot of overhead for this already-gnarly issue, so I'm inclined change the comment so it is at least accurate for the current behavior, smack on a @todo, add more coverage in a followup, and open a new bug if it turns out there's a bug for that case.

I'd also like to add some additional coverage to see what happens when a language-neutral node is saved with one or both of the access control modules active.

We also still need this test coverage, I think. This scenario seems more likely to me than the complicated situation above, so I'm less comfortable with deferring that to a followup, but please let me know if you feel differently.

+++ b/core/modules/node/node.moduleundefined
@@ -2788,9 +2805,13 @@ function node_access($op, $node, $account = NULL, $langcode = NULL) {
       $query = db_select('node_access');
       $query->addExpression('1');
       $query->condition('grant_' . $op, 1, '>=');
-      $nids = db_or()->condition('nid', $node->nid);
+      $nids = db_and()
+        ->condition('nid', $node->nid)
+        ->condition('langcode', $langcode);
       if ($node->status) {
-        $nids->condition('nid', 0);
+        $nids = db_or()
+          ->condition($nids)
+          ->condition('nid', 0);
       }
       $query->condition($nids);
       $query->range(0, 1);
@@ -3037,6 +3058,9 @@ function node_query_node_access_alter(AlterableInterface $query) {

@@ -3037,6 +3058,9 @@ function node_query_node_access_alter(AlterableInterface $query) {
   if (!$op = $query->getMetaData('op')) {
     $op = 'view';
   }
+  if (!$langcode = $query->getMetaData('langcode')) {
+    $langcode = FALSE;
+  }
 
   // If $account can bypass node access, or there are no node access modules,
   // or the operation is 'view' and the $account has a global view grant
@@ -3100,6 +3124,13 @@ function node_query_node_access_alter(AlterableInterface $query) {

@@ -3100,6 +3124,13 @@ function node_query_node_access_alter(AlterableInterface $query) {
         $subquery->condition($grant_conditions);
       }
       $subquery->condition('na.grant_' . $op, 1, '>=');
+      if ($langcode === FALSE) {
+        $subquery->condition('na.fallback', 1, '=');
+      }
+      else {
+        $subquery->condition('na.langcode', $langcode, '=');
+      }

Finally, I should have brought this up previously, but there's a lot of obscure, weird query logic here and no inline comments explaining it. (It's a pre-existing problem, but it becomes worse now that the query and query alter are more complicated.) Can we add some inline comments explaining what's going on in these two functions?

Thanks everyone for your perseverance on this issue. Node access is complicated...

I'm not a node grants expert, so I don't fully understand yet what is needed.

Pending (from my understanding):

1. Change assertions from NodeAccessLanguageAwareCombinationTest or improve comments.

2. Add more coverage for the case where there are actually explicit "public" grant records written from another module or write a follow-up issue.

3. Add coverage to see what happens when a language-neutral node is saved with one or both of the access control modules active.

4. Document obscure, weird query logic in node access checks.

This patch only covers 4) since I need more time for understanding the other points (I'll ping @GaborHojtsy or @xjm in IRC this week if needed).

Re-rolling...

Re-rolling with current code base and comment wrapping for (4).

Addressing 1).

w0000t Schnitzel is back!
I'm super sorry for not taking care about this :/
unfortunately I was super busy during Nov-Jan to finish customer projects and letting my employees into holidays, but I finally found time to check my node access issue again!

So I read through all the things still open:

1. Change assertions from NodeAccessLanguageAwareCombinationTest or improve comments.

I think @penyaskito did a good Job with this in #145, so nothing to do here.

2. Add more coverage for the case where there are actually explicit "public" grant records written from another module or write a follow-up issue.

Fully agree, but as this is not yet covered currently I think we can make a follow up Issue: "Refactor Core Node Access Tests to not only test for denies, also test for public grants"

3. Add coverage to see what happens when a language-neutral node is saved with one or both of the access control modules active.

Done! I added more nodes to both Tests: "Language Aware" and "Lanuage Aware Combination", as there where already tests for language-neutral (eq: no specific language").

4. Document obscure, weird query logic in node access checks.

Checked @penyaskito work here, rewrote it a bit and also fixed the 80chars length issue.

So from my point of view this is ready to go, and we need a follow up.

What do you think @xjm @gabor?

LGTM if a second pair of eyes are needed :-)

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageAwareCombinationTest.phpundefined
@@ -0,0 +1,325 @@
+    // Query the nodes table as user 1 (full access) with the node access tag
+    // and no specific langcode.
+    $select = db_select('node', 'n')
+    ->fields('n', array('nid'))
+    ->addTag('node_access');
+    $nids = $select->execute()->fetchAllAssoc('nid');
+
+    // All nodes are returned.
+    $this->assertEqual(count($nids), 10, 'db_select() returns all nodes.');
+
+    // Query the nodes table as user 1 (full access) with the node access tag
+    // and langcode de.
+    $select = db_select('node', 'n')
+    ->fields('n', array('nid'))
+    ->addMetaData('langcode', 'de')
+    ->addTag('node_access');
+    $nids = $select->execute()->fetchAllAssoc('nid');

Where's user 1 set up? I don't see any difference here from the assertions above in terms of which user is current during the queries - also isn't it possible to use addMetaData() to specify the account, that'd be more explicit.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageAwareCombinationTest.phpundefined
@@ -0,0 +1,325 @@
+    // Even though there is no German translation, all nodes are returned
+    // because node access filtering does not occr when the user is user 1.
+    $this->assertEqual(count($nids), 10, 'db_select() returns all nodes.');

s/occr/occur.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageAwareTest.phpundefined
@@ -0,0 +1,265 @@
+  function setUp() {
+    parent::setUp();
+
+    node_access_rebuild();

Is that really necessary to rebuild node access, wouldn't it happen on module_enable() anyway?

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageAwareTest.phpundefined
@@ -0,0 +1,265 @@
+    // Clear permissions for authenticated users.
+    db_delete('role_permission')
+      ->condition('rid', DRUPAL_AUTHENTICATED_RID)
+      ->execute();

This too - what default permissions are messing up the test?

+++ b/core/modules/node/node.api.phpundefined
@@ -271,8 +275,9 @@ function hook_node_access_records(Drupal\node\Node $node) {
-    // Only published nodes should be viewable to all users. If we allow access
-    // blindly here, then all users could view an unpublished node.
+    // Only published Catalan translations of private nodes should be viewable
+    // to all users. If we fail to check $node->status, all users would be able

This changes the meaning of the comment quite a bit, I'd add a new hunk I think?

+++ b/core/modules/node/node.moduleundefined
@@ -2604,7 +2604,24 @@ function node_access($op, $node, $account = NULL, $langcode = NULL) {
@@ -2649,10 +2666,18 @@ function node_access($op, $node, $account = NULL, $langcode = NULL) {

@@ -2649,10 +2666,18 @@ function node_access($op, $node, $account = NULL, $langcode = NULL) {
     if (module_implements('node_grants')) {
       $query = db_select('node_access');
       $query->addExpression('1');
+      // Only interested for granting in the current operation.
       $query->condition('grant_' . $op, 1, '>=');
-      $nids = db_or()->condition('nid', $node->nid);
+      // Check for grants for this node and the correct langcode.
+      $nids = db_and()
+        ->condition('nid', $node->nid)
+        ->condition('langcode', $langcode);
+      // If node is published also take the default grant into account, the
+      // default is saved with nid = 0.
       if ($node->status) {
-        $nids->condition('nid', 0);
+        $nids = db_or()
+          ->condition($nids)
+          ->condition('nid', 0);
       }
       $query->condition($nids);
       $query->range(0, 1);
@@ -2907,6 +2932,9 @@ function node_query_node_access_alter(AlterableInterface $query) {

@@ -2907,6 +2932,9 @@ function node_query_node_access_alter(AlterableInterface $query) {
   if (!$op = $query->getMetaData('op')) {
     $op = 'view';
   }
+  if (!$langcode = $query->getMetaData('langcode')) {
+    $langcode = FALSE;

Have any explains been run on this?

Also why change the query regardless of whether multiple languages are enabled?

At first glance this looks like a lot of overhead for sites that may never use this feature, it ought to be possible to only add that if it's likely to be used - i.e. if there's more than one language on the site. Alternatively ET module could alter the query?

Realm is always added to the query - at least if the user has any grants, so possibly language could be put after in key order? Or add another index?

Thanks @catch for the review!

Here a new Version of the Patch, Changelog:

• The node_access_rebuild(); is necessary, as this does not happen during module_enable
• Specifically added a the user1 admin user for the db_selects(). Actually it is run with user 1 per default, but it makes more sense now
• The db_delete('role_permission') is really not necessary. I copied it from the already existing core node access test. Removed it now in all places. Lets see if testbot likes this :)
• Changed the order of the primary keys from ('nid', 'gid', 'langcode', 'realm') to ('nid', 'gid', 'realm', 'langcode')

Here some EXPLAIN on the db_select(), looks good to me? If this needs more performance testing, I'm happy to explain a person which knows more about performance what the whole thing does.

Current Drupal 8 Core:

SQL query: EXPLAIN EXTENDED SELECT 1 AS expression FROM node_access node_access WHERE (grant_view >= '1') AND(( (nid = '1') AND (langcode = 'en') )OR (nid = '0') )AND(( (gid = '0') AND (realm = 'all') )OR( (gid = '0') AND (realm = 'node_access_test_author') )OR( (gid = '0') AND (realm = 'node_access_all') )) LIMIT 1 OFFSET 0; 
Rows: 1

id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
1	SIMPLE	node_access	range	PRIMARY	PRIMARY	813	NULL	4	100.00	Using where

With Patch:

SQL query: EXPLAIN EXTENDED SELECT 1 AS expression FROM node_access node_access WHERE (grant_view >= '1') AND(( (nid = '1') AND (langcode = 'en') )OR (nid = '0') )AND(( (gid = '0') AND (realm = 'all') )OR( (gid = '0') AND (realm = 'node_access_test_author') )OR( (gid = '0') AND (realm = 'node_access_all') )) LIMIT 1 OFFSET 0; 
Rows: 1

id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
1	SIMPLE	node_access	range	PRIMARY	PRIMARY	813	NULL	4	100.00	Using where

@gabor

I changed the order of the keys, but honestely I don't know if this really fixes what catch ment. Any advises here?

re the possibility of having ET UI modify the query, mentioned by @catch in #149:

Gabor suggested testing to see if content displays in the translated languages without et enabled.

Tested (with and without the patch, behavior the same) by:

1. enable ET (content translation module)
2. add a couple languages (af, de)
3. create a node
4. translate node
5. verify /node/1 and /de/node/1 display their respective translated content (use language switcher block if desired)
6. disable ET
7. verify again /node/1 and /de/node/1 show the translated content

so, putting the query in ET would not be advised.

[edit, added below]
maybe not common, but that situation might happen if content is translated not on the live site.

Possible for deployment, where you want to optimise performance on live and not edit content directly there.

I don't think the query alter running when Entity Translation module is disabled is a good reason to bake this into node module, that's an extreme edge case vs. additional complexity for sites that will never use this at all.

I'm not clear on the concern.

What about this situation:
No ET ever enabled, just languages.

Some content in some languages, some content in other languages. Would this be useful in that case? [edit: or maybe that case is covered without this patch]

Perhaps the issue summary needs some more explanation.

I'd be OK with language module holding the query alter.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageAwareCombinationTest.phpundefined
@@ -46,13 +46,9 @@ public static function getInfo() {
-    // Clear permissions for authenticated users.
-    db_delete('role_permission')
-      ->condition('rid', DRUPAL_AUTHENTICATED_RID)
-      ->execute();

I think this hunk was mostly there to ensure there weren't any false positives down the line.

spoke with @xjm and @catch about the performance issue in node_query_node_access_alter(), moved now the langcode check into language module, as it really makes only sense here.

oh, btw not 100% if the naming "node_access_tag_subquery" for the alter is correct.
I thought about node_access_subquery first, but actually in node_access() there is no subquery, and the alter is called in node_query_node_access_alter() which is the node_access TAG function. If anybody has a better idea, I'm open to change it ;)

new alter, so needs documentation in case this is how we want to do it.

First of all, I checked with @webchick and she agrees that this is appropriate as a task (meaning, not subject to the Feb. 18 deadline).

Moving this alter into language.module makes more sense to me, and actually helps address a concern I've had for awhile about the node module including all this functionality itself. However, now we have support for this functionality split between Node and Language. I'm wondering if it would make more sense to move more of the functionality to the Language module?

I think we can drop the word "tag" from the tag name; it's a bit redundant. node_access_subquery probably works, but there might be something that better describes its purpose.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageAwareTest.phpundefined
@@ -0,0 +1,266 @@
+    // Create four Hungarian nodes with Catalan translations:
+    // 1. One with neither language marked as private.
+    // 2. One with only the Hungarian translation private.
+    // 3. One with only the Catalan translation private.
+    // 4. One with both the Hungarian and Catalan translations private.

This comment needs to be updated now that there are six nodes.

+++ b/core/modules/node/node.api.phpundefined
@@ -234,11 +234,16 @@ function hook_node_grants($account, $op) {
+ * - langcode: (optional) The language code of a specific translation of the
+ *   node, if any. Modules may add this key to grant different access to
+ *   different translations of a node, such that (e.g.) a particular group
+ *   is granted access to edit the Catalan version of the node, but not the
+ *   Hungarian version. If no value is provided, the langcode is set
+ *   set automatically from the $node parameter and the node's original
+ *   language (if specified) is used as a fallback.

This would probably be the place to add documentation about the subquery alter. Also, do we need to specify here that the functionality is dependent on the Language module?

+++ b/core/modules/node/node.installundefined
@@ -714,6 +728,34 @@ function node_update_8014() {
+ * Add language.langcode and fallback field to node_access table.

{node_access} should be in curlies here.

+++ b/core/modules/node/node.moduleundefined
@@ -2968,10 +2993,13 @@ function node_query_node_access_alter(AlterableInterface $query) {
+      drupal_alter('node_access_tag_subquery', $subquery, $query);

We probably need a comment for this line.

+++ b/core/modules/node/node.moduleundefined
@@ -3041,18 +3066,35 @@ function _node_access_write_grants(Node $node, $grants, $realm = NULL, $delete =
+      if (isset($grant['langcode'])) {
+        $grant_languages = array($grant['langcode'] => language_load($grant['langcode']));
+      }
+      else {
+        $grant_languages = $node->getTranslationLanguages(TRUE);
+      }
+      foreach ($grant_languages as $grant_langcode => $grant_language) {

What happens when the Language module is not enabled but someone tries to set a langcode in their hook implementation? Hopefully node access gets rebuilt when language is disabled in this case, and any modules that have langcode-aware node access would specify Language as a dependency? (That also adds credence to this functionality belonging to Language instead of Node.)

There are also a number of comments that could use some clarification. I can help with that once we decide what else (if anything) we want to move over to Language. If we do update the patch, let's also be sure to post the test-only patch alongside the full one to be sure that our tests are still providing the expected coverage. (I'm also still not 100% sold on removing the permissions cleanup.)

Leaving at NR for feedback.

I completely agree with Gabor: Language as well as ET are providing their features in an entity-type agnostic way. Hence it would be architecturally wrong to force them to add code explictly dealing with a particular entity.

If we want to reduce the overhead for node access on monolingual sites, which is a sensible and desirable goal, we should just do so and have Node natively make its node acess support language-aware when multiple language are available.

we should just do so and have Node natively make its node acess support language-aware when multiple language are available.

That's also fine with me..

Patch looks good, and lots of tests ++

Some minor comment issues

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageAwareCombinationTest.phpundefined
@@ -0,0 +1,327 @@
+    // translation public:
...
+    // the Catalan nor the English ones:
...
+    // access should be denied in all cases:

':' at the end?

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageAwareCombinationTest.phpundefined
@@ -0,0 +1,327 @@
+    // denies access
...
+    // Access only for request with no language defined
...
+    // deny access
...
+    // denies access

dot missing at the end

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageAwareCombinationTest.phpundefined
@@ -0,0 +1,327 @@
+    // The results should be the same as the for default.

for = four? but assert uses 3

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageAwareTest.phpundefined
@@ -0,0 +1,270 @@
+    // The results should be the same as the for default.

.. as the default.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageTest.phpundefined
@@ -61,31 +47,196 @@ function testNodeAccess() {
+    // Module enabled.

Module -> module

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageTest.phpundefined
@@ -61,31 +47,196 @@ function testNodeAccess() {
+    // Tests that access provided if requested with no language.

access is granted

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageTest.phpundefined
@@ -61,31 +47,196 @@ function testNodeAccess() {
+    // Tests that access not provided if requested with Hungarian language.

access is not granted

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageTest.phpundefined
@@ -61,31 +47,196 @@ function testNodeAccess() {
+    // Module enabled.

Module -> module

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageTest.phpundefined
@@ -61,31 +47,196 @@ function testNodeAccess() {
+    // Tests that access not provided if requested with no language.
...
+    // Tests that access not provided if requested with Hungarian language.
...
+    // Tests that access not provided if requested with no language.

.. is not granted ...

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageTest.phpundefined
@@ -61,31 +47,196 @@ function testNodeAccess() {
+    // Module enabled.