Problem/Motivation
Currently the Content Translation module performs its access checks through a confusing combination of swappable/overridable OO code, procedural callbacks and entity-info-driven callbacks. This makes it hard for entity-defining modules to customize access checks to fit their business logic in clean way.
Proposed resolution
Unify and streamline access checks by relying on a proper entity access controller implementation for translation operations.
Remaining tasks
- Answer #33
- Agree on an implementation plan
- Write a patch
- Reviews
User interface changes
None
API changes
Three new entity access operations used by content_translation:
- translation_create
- translation_update
- translation_delete
Comment | File | Size | Author |
---|---|---|---|
#52 | 2155787-interdiff_39-52.txt | 3.65 KB | fathima.asmat |
#52 | 2155787-52.patch | 8.74 KB | fathima.asmat |
#51 | 2155787-interdiff_50-51.txt | 1.22 KB | fathima.asmat |
#51 | 2155787-interdiff_39-51.txt | 2.77 KB | fathima.asmat |
#51 | 2155787-51.patch | 8.14 KB | fathima.asmat |
Comments
Comment #1
plachComment #2
plachComment #3
Wim LeersI ran into the problematicness of the current code today, and apparently #2188675: "Translate" local task always visible, leads to WSOD did too. It'd be great if this could still be fixed before 8.0.0 ships.
If anybody wants to work on this: I promise reviews!
Comment #4
plachIt would really be better to have this sorted out before beta.
Comment #5
Wim LeersWhile working on #2287071: Add cacheability metadata to access checks, I also noticed the strangeness in this area. I agree it'd be nice if we could solve this.
Comment #6
Carsten Müller CreditAttribution: Carsten Müller commentedI will try to start with this, but because i'm new to D8 i will need some help from an experienced person.
Comment #7
plachSince beta is out we will need a BC layer. I am at the sprint venue, I can provide directions live or via IRC.
Comment #8
Carsten Müller CreditAttribution: Carsten Müller commentedi think this is still a level too high for me. i first have to become more familiar with D8. Sorry.
Comment #9
XanoLet's route all of this through entity access control. It's easier *and* it improves security.
Comment #11
XanoComment #13
XanoBecause of The War on Bugs™.
Comment #14
xjmThis issue was marked as a beta target for the 8.0.x beta, but is not applicable as an 8.1.x beta target, so untagging.
This could potentially be implemented in a minor with BC, so moving to 8.2.x.
Comment #19
hchonovCurrently in
\Drupal\content_translation\ContentTranslationHandler::getTranslationAccess()
we are only checking if the current user has the entity translate permissions.In order to provide more flexible access control and let custom and contrib influence the decision we could simply call the access method on the entity object with the content translation operation to be used. As documented in
\Drupal\content_translation\ContentTranslationHandlerInterface::getTranslationAccess()
content translation uses three operations - "create", "update" and "delete". We cannot use those operations when calling the entity access method as then there will be no information that those are access checks made by content translation. I think as this is a new addition we could simply prefix the operations with "translation_" when calling the entity access method. This introduces three new access operations which will be called on the entity level - "translation_create", "translation_update" and "translation_delete". Before calling\Drupal\content_translation\ContentTranslationHandler::getTranslationAccess()
we always check if the user has an edit and delete access on the entity itself through$entity->access('update', NULL, TRUE);
and$entity->access('delete', NULL, TRUE);
, therefore it is safe to introduce and additionally call those new translation operations.One more thing to consider is that when we allow for modules to hook in into the access decisions, then there might be no operations available. In this case I think it is better to hide the languages without available operations. What do you think about this?
All of this is implemented in the attached patch.
Comment #21
hchonovThanks to the change here we caught a bug -
\Drupal\menu_link_content\MenuLinkContentAccessControlHandler::checkAccess()
is returning an AccessResult object only for the operations "view", "update" and "delete" and for all others it is returning NULL, but it is required to return an AccessResult object no matter what operation the method is called with.Comment #22
kfritscheI think this is a good start, but not sure if its enough in the long run.
Specially for the translation_create the language to create would be super handy, if I want to create a translator role per language, so that a translator only can create the specific language.
In addition shouldn't the check be added to content_translation_translate_access() function as well?
Reviewed the code, which looks good, just a super minor coding standard issue with missing spaces.
Comment #23
kfritscheComment #24
hchonovDo you have any suggestion how this can be accomplished? The entity access method accepts parameters only for the operation and the current user. This would require adding a context parameter to the access check methods, which is a bigger change and will be used only for passing the target language for the translation_create operation. IMHO I am not sure if we should make this possible here and now.
Comment #25
hchonovcontent_translation_translate_access()
is used to check the access to the content translation overview page only and here the conditions are, which makes it just a general check. The more specific checks per language are then made in
\Drupal\content_translation\Controller\ContentTranslationController::overview()
by calling\Drupal\content_translation\ContentTranslationHandler::getTranslationAccess()
.Actually I've now noticed, that the access checks are not made for each entity translation, but always the same entity object is being used, which is not correct for the "update" and "delete" operations. I am fixing this here so that the patch is complete, but I've opened a separate issue just for that bug fix, so that we can separate the task and the bug issues - [#2985553].
Comment #26
kfritscheI'm completely agreeing that this is not needed in this issue, but wanted to point out that maybe current solution wouldn't be enough. And maybe somebody has different idea here. And currently I do not have a good idea how to check this. For update and deletion this would probably easily possible with just ensuring the entity in the access check has the translation to be checked. For create the only idea I have right now would be adding a property to the entity.
Exactly this is the reason why I think this check should be added here. Because if the user doesn't have access to C/U/D a translation, why should the user see the overview page for this entity at all? As all translation rows are removed by this patch the user only sees the source language in this case, so the page is pretty useless for the user. But on a entity list view the user has the operation "translate" and then comes to a page where its not possible to translate the entity. Which would be a bad UX. If this function would return access denied for this already, the translate operation would also not be available for the user. Same point for the translate tab when viewing an entity.
Very good catch!
Comment #27
hchonovI've created a separate issue for this - #2985657: Content translation create access check with context for the target translation.
Ok, I understand your point. You are right, that if there is nothing to do and no information for the user on that page that we should prevent from accessing it at all.
Think about it again however I am not sure if it is correct to remove rows without operations. The rows even without operations might still be useful for the information whether the entity is translated or not. I am just not definitively sure about this... I think both has pros and cons. If we however decide to to remove the rows, then we should prevent access to the content overview page if there are no rows with operations but the row for the default translation.
Comment #29
BerdirI'm not quite sure what benefit the patch really has as it is now.
See #2972308: Allow users to translate content they can edit for a related issue, which basically ties it to the default update operation, anything you can update, you can be default also translate (if you have the permission to enable that mode). As written there, the current API/permissions is written for a scenario where users can *only* translate stuff, but it is in many ways pretty weird.
This adds translation_update/delete/create operations, but it without additional context, I don't quite see what it allows me to do that is not already covered with the issue above.
Because I honestly have no idea what the difference would be between $correct_translation->access('translation_update') and $correct_translation->access('update'). Being able to update a given translation of an entity *is* the same as translating it?
The use case we have in our projects is that we need to limit whether or not you can translate a node into a specific language, could be based on permissions/skills of a user or in our case, it's depending on the group.module relation a given node has. This isn't possible in HEAD, not with our editable patch nor with this issue, it doesn't really bring is closer to that, in fact. Not without the issue created in #27, and adding more arguments to the relevant methods and hooks isn't possible for BC. The only place to put it would be a property/method/something on the entity object itself.
The easier way to achieve just that would be a create-specific custom hook that would make it easy to pass the relevant context along.
What I'm interested in is whether our editable issue would also be enough to cover the use case you're trying to support with the current patch here?
Comment #30
hchonovOur use case is that based on a entity field value we wan't to restrict the access regarding creating and deleting translations, but allow the access for creating translations.
Because of the decision being made based on a field value only a permission would not be sufficient.
The prefix
translation_
is the context. By using a different name for those operations we are aware that they represent the access checks made by content translation.Comment #31
BerdirYes, but as I said, there is technically no difference between operation translation_update and update *if* the correct translation object is passed in (which would allow you to check that the active language != default language and then deny access based on your logic).
And yes, what we need to fix is make sure that the right translation is passed in.
I would expect that directly going to LANG/node/x/edit will not check for translation access explicitly but just operation update, but I could be wrong. If I'm not, then your access logic would not be very reliable.
Comment #32
hchonovYes, I agree about the update operation. What about the create and delete operations regarding translations?
Comment #33
BerdirThat's a good question,.
The problem with create is that a translation_create operation isn't very useful without having the language as context, as I mentioned in the related/follow-up issue that you created, I'm not sure we can extend the API there to support that. What your patch does would support your use case where it's a per-entity decision, but I imagine at least as many use cases need to be able to decide per-language. The easiest approach would IMHO be to have a new, dedicated hook just for that, similar to how we split the create operation into a separate API/method back in #1979094: Separate create access operation entity access controllers to avoid costly EntityNG instantiation.
delete I'm not sure. What are we checking there at the moment if you go to a translation-delete page of a node? I suppose we are already using the regular delete operation for that? Not sure right now if making $translation->access('delete') specific to only deleting a translation and not the entity is an API/behavior change or if it is already kind of working like that, maybe not on the translation overview page but on the delete route. It's just /language/node/X/delete, so that means it's the regular _entity_access route requirement?
Comment #34
BerdirLooks like this needs a reroll.
Comment #36
matthiasm11 CreditAttribution: matthiasm11 at Randstad Digital commented#3018576: Content translation should allow for a language-aware access checks deprecates getTranslationAccess() which means this issue can't make it into core if so.
Please use #3056020: Content translation access control to agree on an implementation plan.
Comment #39
BerdirA reroll now that #2972308: Allow users to translate content they can edit landed. Questions in #33 remain.
Comment #41
BerdirComment #42
guilhermevp CreditAttribution: guilhermevp at CI&T commentedPatch applies cleanly in 9.2.x
Comment #45
Dmitriy.trt CreditAttribution: Dmitriy.trt at FFW commentedJust for info, I've used some changes of the patch #39 to fix #3257030: Inconsistent access check of the update/delete access on content translations results in 403.
Comment #47
recrit CreditAttribution: recrit at Phase2 commentedComment #48
recrit CreditAttribution: recrit at Phase2 commentedI would expect that the "create" access call should get the same updates as in this patch, so that the creation of specific translations could be restricted as well.
However, the translation entity cannot be created at that time so we would need to pass the language or just the language code.
Comment #50
fathima.asmat CreditAttribution: fathima.asmat at CTI Digital commentedYes, agreed with #48. Rerolled patch 39 with extra langcode argument for
getTranslationAccess()
.The
langccde
then can be picked up from a custom translation handler as below, in case it's not passed for update/delete actions:Comment #51
fathima.asmat CreditAttribution: fathima.asmat at CTI Digital commentedI also see that translation entity operation for overview link doesn't invoke the correct handlers for access checks when custom callbacks are used for the translation handlers form entity type alter hooks.
For instance, if I override the default callback for translation overview as below:
content_translation_entity_operation()
doesn't invoke the custom handler for access check, but instead just calls the default content_translation_translate_access() method that was registered as part of thecontent_translation
module entity type alter.Patch attached for fixing that too.
Comment #52
fathima.asmat CreditAttribution: fathima.asmat at CTI Digital commentedThe
langcode
as addressed in #50 should be passed to Access checker aswell for better results. Patch is attached to resolve that.Comment #53
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
Believe #33 still needs answer.
This almost seems like it could be fixing a bug? Even though it's a task I do think it should have test coverage.
Comment #54
kristiaanvandeneyndeRe #33:
It absolutely is, just look at revisions. All of the delete/update/whatever revision operations are being check against the main entity. I don't see why we wouldn't check translation operations against the original entity either.