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:

  • EntityAccessControlHandlerInterface::access()
  • EntityAccessControlHandler::checkAccess()
  • NodeGrantDatabaseStorageInterface::access()
  • hook_entity_access()
  • hook_ENTITY_TYPE_access()

This therefore affects all implementations of the interfaces and hooks.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because nothing is actually broken.
Issue priority Major because this affects the whole Entity Access API.
Prioritized changes The main goal of this issue is reducing fragility by streamlining the Entity Access API and thus improving security by removing the possibility of writing ambiguous code.
Disruption Disruptive for core, contributed and custom modules because it will require a BC break: the code will need to be adjusted to pass the proper entity translation object instead of specifying a language parameter.
CommentFileSizeAuthor
#72 et-access_langcode-2072945-57.patch74.43 KBeffulgentsia
#58 et-access_langcode-2072945-57.patch74.43 KBplach
#50 et-access_langcode-2072945-50.patch43.94 KBplach
#50 et-access_langcode-2072945-50.interdiff.txt577 bytesplach
#48 et-access_langcode-2072945-48.patch44.22 KBplach
#46 entity_access-remove-langcode-param-2072945-26.patch46.63 KBplach
#29 entity_access-remove-langcode-param-2072945-29.patch42.87 KBSchnitzel
#29 2072945-interdiff-26-29.patch5.4 KBSchnitzel
#26 entity_access-remove-langcode-param-2072945-26.patch46.63 KBBerdir
#26 entity_access-remove-langcode-param-2072945-26-interdiff.txt4.87 KBBerdir
#24 entity_access-remove-langcode-param-2072945-24.patch41.77 KBBerdir
#24 entity_access-remove-langcode-param-2072945-24-interdiff.txt1.52 KBBerdir
#22 entity_access-remove-langcode-param-2072945-22.patch41.26 KBBerdir
#22 entity_access-remove-langcode-param-2072945-22-interdiff.txt2.37 KBBerdir
#20 entity_access-remove-langcode-param-2072945-20.patch39.41 KBBerdir
#20 entity_access-remove-langcode-param-2072945-20-interdiff.txt1.95 KBBerdir
#18 entity_access-remove-langcode-param-2072945-18.patch37.46 KBBerdir
#14 entity_access-remove-langcode-param-2072945-14.patch33.73 KBBerdir
#14 entity_access-remove-langcode-param-2072945-14-interdiff.txt1 KBBerdir
#10 entity_access-remove-langcode-param-2072945-10.patch33.74 KBBerdir
#10 entity_access-remove-langcode-param-2072945-10-interdiff.txt1.02 KBBerdir
entity_access-remove-langcode-param.patch32.72 KBeffulgentsia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Issue tags: +API change

Tagging.

effulgentsia’s picture

Issue tags: +D8MI

More tagging.

Berdir’s picture

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.

Berdir’s picture

Tagging's fun.

Status: Needs review » Needs work

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

effulgentsia’s picture

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.

effulgentsia’s picture

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()?

plach’s picture

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: Site configuration with domain based language negotiation results in redirecting authenticated users to a different domain when accessing a content entity route for translation language different from the interface 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().

Berdir’s picture

Berdir’s picture

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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +language-content
Gábor Hojtsy’s picture

Status: Needs review » Needs work
Berdir’s picture

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.

Berdir’s picture

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
37.46 KB

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.

Berdir’s picture

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.

Berdir’s picture

Fixing tests.

Status: Needs review » Needs work

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

Berdir’s picture

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.

Berdir’s picture

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.

Berdir’s picture

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 :)

Schnitzel’s picture

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)
Schnitzel’s picture

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.

Berdir’s picture

+++ 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.

Schnitzel’s picture

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?

Berdir’s picture

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 :)

Schnitzel’s picture

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:

/**
 * 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)

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
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')

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
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?

plach’s picture

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.

Berdir’s picture

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.

jibran’s picture

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.

andypost’s picture

This is API change that needs approval at this time

plach’s picture

Issue tags: +rc deadline
plach’s picture

Issue tags: +sprint

Started working on #2090983: ContentEntityInterface::getTranslation() should throw an exception when an invalid language is specified to simplify things here. I also have a pending reroll at #2496337-44: [plach] Testing issue (not ported test fixes yet).

plach’s picture

plach’s picture

plach’s picture

Status: Needs work » Needs review
FileSize
46.63 KB

This is an entirely new patch. I discussed this approach with @catch: since we are so close to RC, we are no longer allowed to perform changes that may introduce regressions. He suggested to just deprecate the $langcode parameter for now and mark it for removal before release, so we can do that during the RC phase.

The attached patch just swaps the $langcode parameter so it comes last and makes it optional. All behaviors and tests are unaffected with the sole exception of NodeInterface::prepareLangcode(): I just could not find a way to make that thing work with the new recommended approach, so I decided to simply kill it with fire. However it's a minimal change, I think.

Status: Needs review » Needs work

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

plach’s picture

Status: Needs work » Needs review
FileSize
44.22 KB

Wrong patch, sorry

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

plach’s picture

Removed an unused use statement.

plach’s picture

Title: Remove $langcode parameter from EntityAccessControllerInterface::access() and friends » Deprecate the $langcode parameter in EntityAccessControllerInterface::access() and friends

Better title

plach’s picture

Issue summary: View changes
plach’s picture

Issue tags: +Needs change record

I will write one CR later if no one beats me to it. Reviews welcome.

catch’s picture

Discussed this with @effulgentsia and we would not rule out full removal before RC if that's still doable - on the basis that it reduces risk over the cycle, so is worth some minor risk now.

However if that's not doable, I think swapping the argument order is a good improvement over now, and great to see that patch green.

effulgentsia’s picture

Issue tags: +Security improvements

Yeah, for me, removing the argument entirely seems no more of a BC break than swapping the order. And I am concerned that not removing it will lead to occasional security bugs for contrib modules that don't know which language to check access for when the $langcode argument doesn't match the entity's language, so tagging as a Security improvement. Or am I wrong about that, in which case, let's untag that?

Berdir’s picture

Hm.

This would have been a great change two years ago, when we last worked on it. I'm not convinced its worth it now. Yes, the API is confusing. But it's not like it is the only confusing API that we have now. Compared to the 17 different ways of creating URL's and links, this seems almost harmless ;)

Pretty sure just moving the argument around and deprecating it isn't. That smells like those pre-beta deprecations that we did, without actually having valid replacements for some/many cases (We still have those node access tests that are AFAIK relying on it?). The current patch would break the API a second time on checkAccess() although that would be fixable (making $langcode actually optional but it would still be a change, if only one that doesn't directly break existing code).

I'm also not convinced that removing it is worth it. Just in my local d8 install, I have 30 checkAccess() implementations in 13 different projects, they would all break. Easy to fix, but still, a fair amount of work to coordinate those modules, get the patches committed and so on. A day before RC1, when all kinds of people will install core and want to test those modules.

And I am concerned that not removing it will lead to occasional security bugs for contrib modules that don't know which language to check access for when the $langcode argument doesn't match the entity's language, so tagging as a Security improvement

Aren't there easier ways to prevent that, for example by making sure that $langcode is consistent with $entity, for content entities? If it's default, then replace it with the active langcode of the entity, if not, then make sure the entity is using that translation? Also keep in mind that only content entities have a concept of an active language. I'm not sure if there really is a use case for language-specific access checking of config entities, but possibly for other types of entities in contrib/custom.

That said, if the core committers think that the possibly security issues make the API change worthwhile (and my suggestion above isn't an acceptable workaround) then I won't fight it. It would probably take me an hour or so to work through the affected modules that I care about and commit the fixes for them, so I'll survive ;)

effulgentsia’s picture

I have 30 checkAccess() implementations in 13 different projects, they would all break.

Do any of them do something that depends on language, and if so, do they use the $langcode argument or the entity language?

plach’s picture

Title: Deprecate the $langcode parameter in EntityAccessControllerInterface::access() and friends » Remove the $langcode parameter in EntityAccessControllerInterface::access() and friends
Issue summary: View changes
FileSize
74.43 KB

@Berdir:

I'm also not convinced that removing it is worth it. Just in my local d8 install, I have 30 checkAccess() implementations in 13 different projects, they would all break. Easy to fix, but still, a fair amount of work to coordinate those modules, get the patches committed and so on. A day before RC1, when all kinds of people will install core and want to test those modules.

I think there's a good chance all the SafeMarkup changes already broke many modules, so I'm afraid people can't realistically expect to install D8 modules with RC1 and see them work flawlessly. Aside from the fact that we usually start having a reliable contrib space at least 6 months after a major release.

Aren't there easier ways to prevent that, for example by making sure that $langcode is consistent with $entity, for content entities? If it's default, then replace it with the active langcode of the entity, if not, then make sure the entity is using that translation?

That would be a possible alternative, but it would still require an API break: we'd need to change the default value of $langcode to NULL, because LanguageInterface::LANGCODE_DEFAULT has a very precise meaning, which is not active language, but entity default language. Only when $langcode is not provided, we can safely rely on the active language, otherwise a developer explicitly passing LanguageInterface::LANGCODE_DEFAULT would get an unexpected behavior. Additionally, ensuring that active language and $langcode match would either require us to make $langcode prevail, if they don't, or throw an exception. In the first case we'd end-up ignoring the active language most of the time, unless we changed the $langcode default to NULL. In both cases we'd have a BC break.

Also keep in mind that only content entities have a concept of an active language. I'm not sure if there really is a use case for language-specific access checking of config entities, but possibly for other types of entities in contrib/custom.

In core we only translate config strings, which is hardly a reason to have per-language logic. The config override system does allow to have per-language changes also in non-string values, but those are not limited to the language axis. Actually any variant may require its specific access control logic, so we'd need a different approach anyway for config entities.

@all:

That said, here's a patch that's more or less an up-to-date version of #26. After skimming through all the previous comments I think the only concern @Schnitzel and @xjm had that was not addressed yet was:

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.

This is no longer happening since #2090983: ContentEntityInterface::getTranslation() should throw an exception when an invalid language is specified: basically it is no longer possible to invoke ::getTranslation() with an actual language value if the entity is language neutral, so the case above should be addressed. Access control will always be performed on existing translation objects and those will always have an associated access logic (possibly taking language into account), so we have no room for ambiguity now.

Hopefully we can get this in before RC. I'm going to create a CR documenting the changes performed by this patch. I will be available tomorrow to implement @Berdir's approach if that's what committers think we should do, however that's RC deadline too, I think.

plach’s picture

@effulgentsia:

Do any of them do something that depends on language, and if so, do they use the $langcode argument or the entity language?

Even if the did the fix would probably be trivial, so I guess what's really relevant is that fact that all of them are likely going to break anyway, due to the changes in the methods' signatures.

plach’s picture

plach’s picture

Issue tags: -Needs change record
plach’s picture

Issue summary: View changes

And beta evaluation (hopefully the last one for D8 :)

Status: Needs review » Needs work

The last submitted patch, 58: et-access_langcode-2072945-57.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 58: et-access_langcode-2072945-57.patch, failed testing.

webchick’s picture

Status: Needs work » Needs review

Shut up, PIFR.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

(All quoted bits come from the IS.)

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

+

The main goal of this issue is reducing fragility by streamlining the Entity Access API and thus improving security by removing the possibility of writing ambiguous code.

which means an unavoidable — and an inherently intentional — disruption:

Disruptive for core, contributed and custom modules because it will require a BC break: the code will need to be adjusted to pass the proper entity translation object instead of specifying a language parameter.

Because of that, boldly RTBC'ing. The patch is ready. It just needs a release manager/framework manager/committer to weigh the pros and cons and make a decision.

andypost’s picture

Assigned: Unassigned » Berdir

My RTBC +1 to the change, would be great final sighs of @Berdir and @yched

I used to update 12 custom entities by hand and each change makes me see that code looks saner
When any code using EntityInterface this code supposed to manage language with it's public methods

  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -313,7 +313,7 @@ public function access($operation, AccountInterface $account = NULL, $return_as_
    -      ->access($this, $operation, LanguageInterface::LANGCODE_DEFAULT, $account, $return_as_object);
    +      ->access($this, $operation, $account, $return_as_object);
    

    That means \Drupal\Core\Entity\EntityInterface::language() should be only a way to use language in entity access implementations, +1
    also makes caching of entity access more predictable, +1

  2. +++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
    @@ -53,8 +53,9 @@ public function __construct(EntityTypeInterface $entity_type) {
    -  public function access(EntityInterface $entity, $operation, $langcode = LanguageInterface::LANGCODE_DEFAULT, AccountInterface $account = NULL, $return_as_object = FALSE) {
    +  public function access(EntityInterface $entity, $operation, AccountInterface $account = NULL, $return_as_object = FALSE) {
    

    Having language before account was confused me everytime I faced with implementing custom entity access

plach’s picture

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

Status: Needs review » Reviewed & tested by the community

Didn't mean to change the status

Wim Leers’s picture

I personally would prefer this solution.

This is similar to the EntityViewBuilder $langcode thing from a few days ago: #2073217: Remove the $langcode parameter from the entity view/render system.

This is IMO about Entity API completion/DX/understandability. This helps make it clear that you want $entity->language(), not some separate $langcode variable. i.e. it makes it clear that Entities are self-contained value objects that you interact with, containing all info. Which contrasts with D7, where some metadata had to be passed around and kept in sync.

(Which ironically is exactly what #2581721: Ensure consistency between $langcode parameter and entity language in the Entity Access API demonstrates: $entity and $langcode are not always in sync in HEAD.)

The strongest reason not to do this IMO is the fact that very few people will be confused by this, because, really, how many people actually write entity access code? OTOH, that's also a reason to do this: few people are affected, so disruption is small. (Berdir with his 30 implementations of this, described in #56, is clearly the big exception. Sorry Berdir.)

EDIT: fixed second issue link.

effulgentsia’s picture

Reuploading #58 to get a new DrupalCI and PIFT run. I know that can also be done with "add test" / "retest", but I don't like the clutter of the former.

effulgentsia’s picture

Assigned: Berdir » effulgentsia

I discussed this with the other committers, and it's a tough call. It's definitely unfortunate to be breaking an interface method signature and a protected method signature of a base class so commonly extended so late in (pretty much at the very end of) the beta phase. Especially when 90% of entity access control handlers (at least in core, I don't know if that ratio holds in contrib) don't do anything language-specific anyway, so they get none to little benefit from the break. However, for the access control handlers that do do something language-specific, I think this is a significant improvement that can help prevent security bugs. Possibly, #2581721: Ensure consistency between $langcode parameter and entity language in the Entity Access API would have been sufficient for that, but I'm not as confident in that closing off all mismatch problems, as I am in simply removing the redundant parameter that can be a source of mismatch.

Therefore, we decided to commit this, and I will do so once #72 comes back green.

Apologies to every contrib maintainer who will need to update the signature of their entity access control implementations. Hopefully the pain is mitigated by it being a fast fix to implement.

effulgentsia’s picture

Thank you @andypost and @Wim Leers for help with reviews. Ticking the credit boxes accordingly.

  • effulgentsia committed 190032b on 8.0.x
    Issue #2072945 by Berdir, plach, effulgentsia, Schnitzel, andypost, Wim...
effulgentsia’s picture

Assigned: effulgentsia » Unassigned
Status: Reviewed & tested by the community » Fixed

Pushed to 8.0.x and published the CR!

plach’s picture

I also apologize with the affected people: I simply was not able to allocate the required time to make this happen earlier :(

YesCT’s picture

some unused use statements went in here. taking them out in #2584297: Remove unused use statements (that mostly came in from checkAcess)

YesCT’s picture

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks all!

Status: Fixed » Closed (fixed)

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

xjm’s picture

Retroactively removing tags; both roles signed off.