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:

  1. translation_create
  2. translation_update
  3. translation_delete
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Issue summary: View changes
plach’s picture

Issue summary: View changes
Wim Leers’s picture

Priority: Normal » Major
Issue tags: +DX (Developer Experience)
Related issues: +#2188675: "Translate" local task always visible, leads to WSOD

I 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!

plach’s picture

Issue tags: +beta target

It would really be better to have this sorted out before beta.

Wim Leers’s picture

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

Carsten Müller’s picture

Assigned: Unassigned » Carsten Müller
Issue tags: +Amsterdam2014

I will try to start with this, but because i'm new to D8 i will need some help from an experienced person.

plach’s picture

Since beta is out we will need a BC layer. I am at the sprint venue, I can provide directions live or via IRC.

Carsten Müller’s picture

Assigned: Carsten Müller » Unassigned

i think this is still a level too high for me. i first have to become more familiar with D8. Sorry.

Xano’s picture

Status: Active » Needs review
FileSize
6.85 KB

Let's route all of this through entity access control. It's easier *and* it improves security.

Status: Needs review » Needs work

The last submitted patch, 9: drupal_2155787_9.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
1 KB
6.96 KB

Status: Needs review » Needs work

The last submitted patch, 11: drupal_2155787_11.patch, failed testing.

Xano’s picture

Because of The War on Bugs™.

xjm’s picture

Title: Remove ContentTranslationController::getTranslationAccess() in favor of a proper access controller » Deprecate ContentTranslationController::getTranslationAccess() in favor of a proper access controller
Version: 8.0.x-dev » 8.2.x-dev
Issue tags: -beta target

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

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

hchonov’s picture

Title: Deprecate ContentTranslationController::getTranslationAccess() in favor of a proper access controller » Introduce more flexible access control for content translation operations
Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.73 KB

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

Status: Needs review » Needs work

The last submitted patch, 19: 2155787-19.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

hchonov’s picture

Status: Needs work » Needs review
FileSize
3.46 KB
574 bytes

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

kfritsche’s picture

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

+++ b/core/modules/content_translation/src/ContentTranslationHandler.php
@@ -277,7 +277,14 @@ public function getTranslationAccess(EntityInterface $entity, $op) {
+      $entity_access = $entity->access('translation_'.$op, $this->currentUser, TRUE);
kfritsche’s picture

Status: Needs review » Needs work
hchonov’s picture

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

hchonov’s picture

Status: Needs work » Needs review
FileSize
6.56 KB
3.97 KB

In addition shouldn't the check be added to content_translation_translate_access() function as well?

content_translation_translate_access() is used to check the access to the content translation overview page only and here the conditions are

  • entity view access
  • entity is enabled for content translation
  • current user has at least one of the permission "create/update/delete content translations"

, 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].

kfritsche’s picture

Do you have any suggestion how this can be accomplished? The entity access method accepts parameters only for the operation and the current user. ... IMHO I am not sure if we should make this possible here and now.

I'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.

current user has at least one of the permission "create/update/delete content translations"

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.

Actually I've now noticed, that the access checks are not made for each entity translation

Very good catch!

hchonov’s picture

I'm completely agreeing that this is not needed in this issue, but wanted to point out that maybe current solution wouldn't be enough.

I've created a separate issue for this - #2985657: Content translation create access check with context for the target translation.

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.

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.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Berdir’s picture

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

hchonov’s picture

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

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.

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.

Berdir’s picture

Yes, 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.

hchonov’s picture

Yes, 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).

Yes, I agree about the update operation. What about the create and delete operations regarding translations?

Berdir’s picture

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

Berdir’s picture

Looks like this needs a reroll.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

matthiasm11’s picture

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

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Berdir’s picture

A reroll now that #2972308: Allow users to translate content they can edit landed. Questions in #33 remain.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Berdir’s picture

guilhermevp’s picture

Patch applies cleanly in 9.2.x

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Dmitriy.trt’s picture

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

recrit’s picture

recrit’s picture

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

-  $create_translation_access = $handler->getTranslationAccess($entity, 'create');
+ $create_translation_access = $handler->getTranslationAccess($entity, 'create', $langcode);

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

fathima.asmat’s picture

Yes, 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:

public function getTranslationAccess(EntityInterface $entity, $op, ?string $langcode = '') {
        if (!$langcode) {
          $langcode = $entity->language()->getId();
        }
....
fathima.asmat’s picture

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

/**
 * Implements hook_entity_type_alter().
 */
function MODULE_entity_type_alter(array &$entity_types) {
// Node translation/access handlers.
  $node_translation = $node_entity_type->get('translation');
  if (!$node_translation || !isset($node_translation['content_translation'])) {
    $node_translation['content_translation'] = [];
  }
  $node_translation['content_translation'] += [
    'access_callback' => 'my_custom_callback_handler',
  ];
  $node_entity_type->set('translation', $node_translation);
}

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 the content_translation module entity type alter.

Patch attached for fixing that too.

fathima.asmat’s picture

The langcode as addressed in #50 should be passed to Access checker aswell for better results. Patch is attached to resolve that.

smustgrave’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

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

kristiaanvandeneynde’s picture

Re #33:

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?

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.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.