Problem/Motivation

Per #1953892-24: Move language node access logic into NodeAccessController, as of #1810370: Entity Translation API improvements, an entity translation is also an entity, so the $langcode parameter to the access controller is:
- extraneous, because the $entity parameter that's passed in should already be the desired translation
- confusing, because if the passed in $langcode is different from $entity->language()->id, it's not clear what the expected behavior should be

This issue is critical, because it is required by #1953892: Move language node access logic into NodeAccessController, which is required by #1947880: Replace node_access() by $entity->access(), which is required by #1938318: Convert book_remove_form to a new-style Form object and other places where core or contrib needs to move a route to the new routing system but requires a node_access() check.

Proposed resolution

Remove the parameter. Require calling code to pass in the desired translation, rather than the original entity (or a random translation of it) and a separate $langcode.

API changes

$langcode parameter removed from:
- EntityAccessControllerInterface::access()
- EntityAccessController::checkAccess()
- NodeGrantDatabaseStorageInterface::access()
- hook_ENTITY_TYPE_access()
This therefore affects all implementations of the interfaces and hooks.

Comments

Issue tags:+API change

Tagging.

Issue tags:+D8MI

More tagging.

Yay.

I guess we should do the same for other cases where we pass through language codes, like viewing an entity.

What I'm not sure is if we need to call $entity->getTranslation($content_language) somewhere, so that we actually check access for the right language. Maybe the route access checker should do that? Not sure.

Tagging's fun.

Status:Needs review» Needs work

The last submitted patch, entity_access-remove-langcode-param.patch, failed testing.

I guess we should do the same for other cases where we pass through language codes, like viewing an entity.

+1. Wanna open an issue for that now, or after we've gotten this one done?

What I'm not sure is if we need to call $entity->getTranslation($content_language) somewhere

Currently, we sort of do that in the node_access() wrapper function. I agree that'll need to move somewhere else, but not sure where yet. My current hunch is it should be done as part of the {node} upcaster, either always or only if something like an options.parameters.node.localize: TRUE is set on the route. That would result in the correct translation getting passed to node_view() / render controller as well. But I think that can be punted to one of the other issues mentioned in the issue summary.

Now we need some help from the D8MI folks. One of the failing tests is NodeAccessLanguageTest::testNodeAccess() calling this:

// Tests that access is not granted if requested with Hungarian language.
$this->assertNodeAccess($expected_node_access_no_access, $node_public_no_language, $web_user, 'hu');

In the above, $node_public_no_language is a node that only has a LANGCODE_NOT_SPECIFIED language. Meanwhile, EntityNG::getTranslation() has this code:

// If the entity or the requested language  is not a configured
// language, we fall back to the entity itself, since in this case it
// cannot have translations.
$translation = empty($this->getDefaultLanguage()->locked) && empty($languages[$langcode]->locked) ? $this->addTranslation($langcode) : $this;

So, what's happening is the $node_public_no_language->getTranslation('hu') returns the original $node_public_no_language, whose language is still LANGCODE_NOT_SPECIFIED, and therefore, access is granted rather than denied.

The question is:
- is getTranslation() incorrect in returning the original node?
- is the test incorrect in expecting 'hu' to be denied when the node doesn't have that translation?
- if neither of the above, does node_access() need to be changed to do something other than just call getTranslation()?

Yayayay! +1000 for #3.

What I'm not sure is if we need to call $entity->getTranslation($content_language) somewhere, so that we actually check access for the right language. Maybe the route access checker should do that? Not sure.

I think we should try to instantiate the proper translation whenever we can reliably determine it, however there should be no ambiguity: for instance in the case of entity forms all form handlers can be sure to deal with the correct translation. We should try to achieve the same kind of reliability in every context where the active language can be clearly identified.

In this case I guess it depends on the operation to be checked for access: if it's create/update/delete we should use the same language the entity form will pick. In many cases it will be the current language, but it might also be forced to something else (see #1810394: Exploit hook_entity_prepare_form() to set the form language without having to rely on the current language).

We should also keep in mind that a translation for the specified language might not exist, in which case the EntityInterface::getExistingTranslation() mehod introduced in #2019055: Switch from field-level language fallback to entity-level language fallback should provide us the correct translation to act on.

For a view operation the current language should be used, falling back to an existing translation.

- is getTranslation() incorrect in returning the original node?

This is probably the only point it's not very clear to me about the new ET API: on one hand I think that throwing an exception when a translation for the specified language does not exist would be the most correct behavior. OTOH being able to fallback to a valid translation object has several advantages, including but not limited to DX. I'm a bit torn here, honestly. This should probably be discussed in #2019055: Switch from field-level language fallback to entity-level language fallback, which among the rest is dealing with entity language negotiation (that is picking the correct translation for a particular context).

- is the test incorrect in expecting 'hu' to be denied when the node doesn't have that translation?

I think the test should not try to retrieve a non-existing translation, especially on a language-neutral node: this shouldn't be a very common use case.

- if neither of the above, does node_access() need to be changed to do something other than just call getTranslation()?

It should be probably act on a translation retrieved through EntityInterface::getExistingTranslation().

Status:Needs work» Needs review
StatusFileSize
new1.02 KB
new33.74 KB
FAILED: [[SimpleTest]]: [MySQL] 58,058 pass(es), 41 fail(s), and 37 exception(s).
[ View ]

Fixed the simple stuff.

I'd also like to understand why we need so many different node access language tests... :)

Status:Needs review» Needs work

The last submitted patch, entity_access-remove-langcode-param-2072945-10.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+language-content

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new1 KB
new33.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity_access-remove-langcode-param-2072945-14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

There was a second call on the same line...

Status:Needs review» Needs work
Issue tags:-API change, -D8MI, -language-content, -Entity Access, -Entity Field API

The last submitted patch, entity_access-remove-langcode-param-2072945-14.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+API change, +D8MI, +language-content, +Entity Access, +Entity Field API

The last submitted patch, entity_access-remove-langcode-param-2072945-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new37.46 KB
FAILED: [[SimpleTest]]: [MySQL] 53,645 pass(es), 1,745 fail(s), and 2,821 exception(s).
[ View ]

Simple re-roll, will probably require a few updates in tests and access controllers.

Status:Needs review» Needs work

The last submitted patch, entity_access-remove-langcode-param-2072945-18.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.95 KB
new39.41 KB
FAILED: [[SimpleTest]]: [MySQL] 53,885 pass(es), 622 fail(s), and 761 exception(s).
[ View ]

This should fix a lot of those.

Status:Needs review» Needs work

The last submitted patch, entity_access-remove-langcode-param-2072945-20.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.37 KB
new41.26 KB
FAILED: [[SimpleTest]]: [MySQL] 58,343 pass(es), 41 fail(s), and 34 exception(s).
[ View ]

Fixing tests.

Status:Needs review» Needs work

The last submitted patch, entity_access-remove-langcode-param-2072945-22.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.52 KB
new41.77 KB
FAILED: [[SimpleTest]]: [MySQL] 59,006 pass(es), 11 fail(s), and 0 exception(s).
[ View ]

I don't understand how those language aware tests work, but this should at least fix the revision tests.

Status:Needs review» Needs work

The last submitted patch, entity_access-remove-langcode-param-2072945-24.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.87 KB
new46.63 KB
PASSED: [[SimpleTest]]: [MySQL] 59,168 pass(es).
[ View ]

Ok, those test fails are all the same.

They test that when checking access for a specific language on an entity with language UND (not specificed), you should get access denied. That worked because langcode is just a string. They even tested with langcodes that don't exist (in that test environment).

That's not something you can do anymore with the new Entity Translation API, so we can no longer support this case. This removes those tests, if you need to do something like that, you first need to check if you actually have an entity in a given language and then check access for it.

We might want to postpone this on the entity level language fallback issue, that adds a method to get the fallback language, then you check access for that.

Ok, discussed this a bit more, and I think I really understand this now and I think it's ok.

This is specifically about the case where an entity is LANGUAGE_NOT_SPECIFIED. Such an entity can't have language-specific access logic, it can never have a different translation. So I think it's fine to remove those tests. And as a bonus, they also remove a bogus and broken variable_set() and drupal_static_reset() call :)

just discussed this with xjm. Outcome is that we both think that it should be the case that access is denied when the Node is LANGUAGE_NOT_SPECIFIED and you specifically request Access with a language code.

Reasons:

  • If the Node has a language and if you request access in a language the node does not exit, then access is denied
  • It could be a security issue that when a User has access only to Hungarian Nodes that a Node with LANGUAGE_NOT_SPECIFIED can still be seen
  • We not only have node_access(), there is also the 'node_access' Tag in db_select() and this still works the way it was before, so actually we would need to change the behavior there as well! (yes NodeAccess is crazy hard)

StatusFileSize
new5.4 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2072945-interdiff-26-29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new42.87 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity_access-remove-langcode-param-2072945-29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

here a way how to get this back. Not really sure if this is the best way.

Status:Needs review» Needs work

The last submitted patch, 2072945-interdiff-26-29.patch, failed testing.

+++ b/core/modules/node/node.module
@@ -1635,6 +1635,10 @@ function node_access($op, $node, $account = NULL, $langcode = NULL) {
   if ($langcode) {
     $node = $node->getTranslation($langcode);
+    // If the language could not be loaded, bail.
+    if ($node->language()->id !== $langcode) {
+      return FALSE;
+    }
   }
   return $access_controller->access($node, $op, $account);

That doesn't work.

node_access() is going away too.

I understand what you are saying, but that is impossible with this approach.

Just like you did, that approach would have to be done in the calling code. Note that there is an issue that wants to add an exception to getTranslation() if it doesn't exist (once we have the methods for getting a translation or the fallback).

If that's not good enough then I fear this issue is a won't fix. It's impossible to achieve this from within the access system when there isn't a separate $langcode argument.

Mhh I honestly don't really understand why this should not work, but I just want to bring in the knowledge as I wrote most of the Tests.

So If this is not possible I see two possibilities:

- Accept the fact that there is no language aware node access for Nodes which have LANGUAGE_NOT_SPECIFIED defined. As currently there is no Drupal 7 language aware Node Access, I would not call this a regression, but it's not like how Sitebuilders would expect this. But then we would need to adapt the behavior of node_access Tag to the same as node_access().
- Don't fix this, and maybe think about this later when all the other Node Access Changes have been fixed?

Sorry, I was a bit overhasty...*

The point of this issue is to remove the separation of $entity and $langcode (checking access on $entity for $langcode) in favor of checking assess for an $entity_translation. An entity with the langcode NOT_SPECIFIED can't have translations. So we can't pass that information through because that concept simply does not exist.

This is a 7.x oriented API, where the $entity and the $langcode are two separate things. If you get an $entity in 8.x, you *know* which the active language of it is ( there's one problem though, see #2019055-116: Switch from field-level language fallback to entity-level language fallback).

My problem with those tests is that they're made up. They're have a hardcoded "for $langcode" enforcement, which we can't port directly. I think we need to look at actual use cases and how we can solve them using the 8.x API's.

If your use case is that a given user may only view hu content, then you can just compare $entity->language()->id == 'hu' and bail out if that's not TRUE. Same result, but inversed process. I think.

Another point to consider is the fallback logic. The issue above removes per-field fallbacks completely and replaces it with a pluggable and alterable per-entity fallback system. So per-language access is also something that could be placed there. Your user might have access to hu and de content, but not en and fr. So you might want limit the fallback languages to those that he has access to, as the entity access allows you only to say yes/no but not provide an alternative. I'm not sure if having "no fallbackable language" as a concept there is possible, but you might simply fall back to the original language of the entity and then deny access to it later on.

Gabor mentioned a buy-access-per-node-and-language system. Should work fine too, you just check your records for the active language and id combination.

I don't know how language specific node access exactly works, but I don't think it's affected by this. I assume you can somehow control which languages should be included, if a node doesn't have a hu translation then he won't have any records for hu and won't be displayed.

Are there use cases that we can not cover once the falllback issue is in?

* entity access + language tends to be a tough topic.. just ask @xjm :)

Mhh I all agree with your points, Language aware Node Access is something new and we defined the Testcases out of our mind, without actually really knowing what all the Sites will need to do. As Node Access is pluggable and you need to implement a custom module to actually use this, this is even harder.
The Testcases are based on this:

<?php
/**
* Implements hook_node_access_records().
*/
function node_access_test_language_node_access_records(EntityInterface $node) {
 
$grants = array();
 
// Create grants for each translation of the node.
 
foreach ($node->getTranslationLanguages() as $langcode => $language) {
   
// If the translation is not marked as private, grant access.
   
$translation = $node->getTranslation($langcode);
   
$grants[] = array(
     
'realm' => 'node_access_language_test',
     
'gid' => 7888,
     
'grant_view' => empty($translation->field_private->value) ? 1 : 0,
     
'grant_update' => 0,
     
'grant_delete' => 0,
     
'priority' => 0,
     
'langcode' => $langcode,
    );
  }
  return
$grants;
}
?>

which is a language aware node_access implementation, it just creates records for all existing languages. So if you request access for a language which the node is not in, you get an Access Denied.

The Problem now: If the Node has LANGUAGE_NOT_SPECIFIED, and you request access for language "hu", you will get access! And we think this is not how it should be.
But as said, this depends on the implementation of the custom module.
The Issue I see is that when the Node is LANGUAGE_NOT_SPECIFIED the request to node_access is made all the time with langcode = 'und'. Where it is made with the langcode you requested if the Node has a language.

Example:

Node ID 1 with Language = "DE", Translation for "HU" does not exist (node_access_test_language_node_access_records() does not create entries for hu)

<?php
node_access
('view', node_load(1), user_load(9), 'hu');
?>

Result:
SELECT 1 AS expression
FROM
{node_access} node_access
WHERE  (grant_view >= '1') AND(( (nid = '1') AND (langcode = 'hu') )OR (nid = '0') )AND(( (gid = '0') AND (realm = 'all') )OR( (gid = '7888') AND (realm = 'node_access_language_test') ))
LIMIT 1 OFFSET 0

<?php
node_access
('view', node_load(1), user_load(9), 'de');
?>

Result:
SELECT 1 AS expression
FROM
{node_access} node_access
WHERE  (grant_view >= '1') AND(( (nid = '1') AND (langcode = 'de') )OR (nid = '0') )AND(( (gid = '0') AND (realm = 'all') )OR( (gid = '7888') AND (realm = 'node_access_language_test') ))
LIMIT 1 OFFSET 0

Node ID 2 with Language = LANGUAGE_NOT_SPECIFIED (node_access_test_language_node_access_records() creates one entry for langcode = 'und')

<?php
node_access
('view', node_load(2), user_load(9), 'hu');
?>

Result:
SELECT 1 AS expression
FROM
{node_access} node_access
WHERE  (grant_view >= '1') AND(( (nid = '2') AND (langcode = 'und') )OR (nid = '0') )AND(( (gid = '0') AND (realm = 'all') )OR( (gid = '7888') AND (realm = 'node_access_language_test') ))
LIMIT 1 OFFSET 0

<?php
node_access
('view', node_load(2), user_load(9), 'de');
?>

Result:
SELECT 1 AS expression
FROM
{node_access} node_access
WHERE  (grant_view >= '1') AND(( (nid = '2') AND (langcode = 'und') )OR (nid = '0') )AND(( (gid = '0') AND (realm = 'all') )OR( (gid = '7888') AND (realm = 'node_access_language_test') ))
LIMIT 1 OFFSET 0

So you see, no mater which language you request, the query to the node_access Table is all the time the same. So a custom node_access Module couldn't not even work around this case with creating grant_view = 0 for all other existing languages enabled on the Site.

And this is actually the only Issue that I'm worried about, and as far as I discussed with @xjm for her as well.

I unfortunately do not fully understand the fallback system, maybe you can tell us if we can fix this?

Maybe I am missing something (I didn't study per-language access in depth), but I don't see big problems here:

  • If the Node has a language and if you request access in a language the node does not exit, then access is denied
  • It could be a security issue that when a User has access only to Hungarian Nodes that a Node with LANGUAGE_NOT_SPECIFIED can still be seen

We have to ways to deal with single node access:

  • As Berdir was saying above, we have an issue to throw an exception when code tries to retrieve a non existing translation. If that happens, for instance, in a route access check, the sanest thing to do is returning a 403. This is in line with the current behavior.
  • Alternatively, if we apply language fallback either by relying on the entity system or manually, we are simply saying we wish to act on an existing translation (the entity system will return the one that best fits the current context). In this case access is still checked, hence if we have an entity object with und language and the user is only granted access to hu translations, access will be refused anyway. In the case of the logic implemented in the test node, the user would get access, but that's because she has it. I don't see the security concern.

We not only have node_access(), there is also the 'node_access' Tag in db_select() and this still works the way it was before, so actually we would need to change the behavior there as well! (yes NodeAccess is crazy hard)

If the user has a grant matching the requested language, the node will appear in the results, otherwise it won't: as long as grants match existing translations, which is a reasonable assumption, things keep working fine, as the user sees data she's allowed to access. The only potential problem I see is whether the node appears in the result but for some reason there is no translation for that language. In this case if fallback is applied and no further access checks are performed, some unaccessible data might be disclosed. But I think this would be a very weird access logic.

The Issue I see is that when the Node is LANGUAGE_NOT_SPECIFIED the request to node_access is made all the time with langcode = 'und'. Where it is made with the langcode you requested if the Node has a language.

Not exactly: in both cases language fallback would be applied and the actual language used to query grants would be the one of an existing translation. The behavior would be consistent.

To sum up I think we don't need to support the case where we check access for non-existing translations.

Priority:Critical» Major
Issue summary:View changes

This was critical because it blocked routing conversions, but those went with an (ugly) workaround, and we also came to the conclusion that we can't remove $langcode completely, so degrading to major for now. Would still be nice to get rid of $langcode in the internal methods as it's confusing right now as to what language to use.

Parent issue:» #2072945: Remove $langcode parameter from EntityAccessControllerInterface::access() and friends

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 29: entity_access-remove-langcode-param-2072945-29.patch, failed testing.