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
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. |
Comment | File | Size | Author |
---|---|---|---|
#72 | et-access_langcode-2072945-57.patch | 74.43 KB | effulgentsia |
#58 | et-access_langcode-2072945-57.patch | 74.43 KB | plach |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedTagging.
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedMore tagging.
Comment #3
BerdirYay.
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.
Comment #4
BerdirTagging's fun.
Comment #6
effulgentsia CreditAttribution: effulgentsia commented+1. Wanna open an issue for that now, or after we've gotten this one done?
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.Comment #7
effulgentsia CreditAttribution: effulgentsia commentedNow we need some help from the D8MI folks. One of the failing tests is NodeAccessLanguageTest::testNodeAccess() calling this:
In the above, $node_public_no_language is a node that only has a LANGCODE_NOT_SPECIFIED language. Meanwhile, EntityNG::getTranslation() has this code:
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()?
Comment #8
plachYayayay! +1000 for #3.
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.
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).
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.
It should be probably act on a translation retrieved through
EntityInterface::getExistingTranslation()
.Comment #9
BerdirOpened #2073217: Remove the $langcode parameter from the entity view/render system
Comment #10
BerdirFixed the simple stuff.
I'd also like to understand why we need so many different node access language tests... :)
Comment #12
Gábor HojtsyComment #13
Gábor HojtsyComment #14
BerdirThere was a second call on the same line...
Comment #16
Berdir#14: entity_access-remove-langcode-param-2072945-14.patch queued for re-testing.
Comment #18
BerdirSimple re-roll, will probably require a few updates in tests and access controllers.
Comment #20
BerdirThis should fix a lot of those.
Comment #22
BerdirFixing tests.
Comment #24
BerdirI don't understand how those language aware tests work, but this should at least fix the revision tests.
Comment #26
BerdirOk, 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.
Comment #27
BerdirOk, 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 :)
Comment #28
Schnitzel CreditAttribution: Schnitzel commentedjust 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:
Comment #29
Schnitzel CreditAttribution: Schnitzel commentedhere a way how to get this back. Not really sure if this is the best way.
Comment #31
BerdirThat 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.
Comment #32
Schnitzel CreditAttribution: Schnitzel commentedMhh 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?
Comment #33
BerdirSorry, 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 :)
Comment #34
Schnitzel CreditAttribution: Schnitzel commentedMhh 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:
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)
Result:
Result:
Node ID 2 with Language = LANGUAGE_NOT_SPECIFIED (node_access_test_language_node_access_records() creates one entry for langcode = 'und')
Result:
Result:
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?
Comment #35
plachMaybe I am missing something (I didn't study per-language access in depth), but I don't see big problems here:
We have to ways to deal with single node access:
und
language and the user is only granted access tohu
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.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.
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.
Comment #36
BerdirThis 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.
Comment #37
plachComment #38
plachComment #39
jibran29: entity_access-remove-langcode-param-2072945-29.patch queued for re-testing.
Comment #41
andypostThis is API change that needs approval at this time
Comment #42
plachComment #43
plachStarted 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).
Comment #44
plachComment #45
plachComment #46
plachThis 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 ofNodeInterface::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.Comment #48
plachWrong patch, sorry
Comment #50
plachRemoved an unused
use
statement.Comment #51
plachBetter title
Comment #52
plachComment #53
plachI will write one CR later if no one beats me to it. Reviews welcome.
Comment #54
catchDiscussed 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.
Comment #55
effulgentsia CreditAttribution: effulgentsia at Acquia commentedYeah, 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?
Comment #56
BerdirHm.
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.
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 ;)
Comment #57
effulgentsia CreditAttribution: effulgentsia at Acquia commentedDo any of them do something that depends on language, and if so, do they use the $langcode argument or the entity language?
Comment #58
plach@Berdir:
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.
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, becauseLanguageInterface::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 passingLanguageInterface::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.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:
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.
Comment #59
plach@effulgentsia:
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.
Comment #60
plachCR at https://www.drupal.org/node/2581447
Comment #61
plachComment #62
plachAnd beta evaluation (hopefully the last one for D8 :)
Comment #66
webchickShut up, PIFR.
Comment #67
Wim Leers(All quoted bits come from the IS.)
+
which means an unavoidable — and an inherently intentional — disruption:
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.
Comment #68
andypostMy 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 methodsThat means
\Drupal\Core\Entity\EntityInterface::language()
should be only a way to use language in entity access implementations, +1also makes caching of entity access more predictable, +1
Having language before account was confused me everytime I faced with implementing custom entity access
Comment #69
plachI've created #2581721: Ensure consistency between $langcode parameter and entity language in the Entity Access API to implement @Berdir's suggestion.
Comment #70
plachDidn't mean to change the status
Comment #71
Wim LeersI 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.
Comment #72
effulgentsia CreditAttribution: effulgentsia at Acquia commentedReuploading #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.
Comment #73
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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.
Comment #74
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThank you @andypost and @Wim Leers for help with reviews. Ticking the credit boxes accordingly.
Comment #76
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 8.0.x and published the CR!
Comment #77
plachI also apologize with the affected people: I simply was not able to allocate the required time to make this happen earlier :(
Comment #78
YesCT CreditAttribution: YesCT commentedsome unused use statements went in here. taking them out in #2584297: Remove unused use statements (that mostly came in from checkAcess)
Comment #79
YesCT CreditAttribution: YesCT commented#1953892: Move language node access logic into NodeAccessController was postponed on this. re-activating that.
Comment #80
Gábor HojtsyThanks all!
Comment #82
xjmRetroactively removing tags; both roles signed off.