Do not allow text format without usage permission check

If the user doesn't have access to the format, then we must not use a textfield with format.

In fact, users shouldn't even be allowed to see the content at all then, but considering that we send it out to translators, I'm not sure how to handle this. Maybe we shouldn't show it and don't allow to accept such job items.

Imagine someone using the php input filter, then you could inject PHP code into the site.

1. +++ b/translators/tmgmt_local/src/Form/LocalTaskItemForm.php
   @@ -166,10 +167,24 @@ class LocalTaskItemForm extends ContentEntityForm {
   +          $text_format_permission = TRUE;
   

   Additionally to disabling the reject/finish buttons, we need to disable being able to accept this at all. You might need to store the value of this in a global $form_state key, if any is FALSE, save as completed must not be shown.

2. +++ b/translators/tmgmt_local/src/Form/LocalTaskItemForm.php
   @@ -166,10 +167,24 @@ class LocalTaskItemForm extends ContentEntityForm {
   +          if (!$format || !\Drupal::currentUser()->hasPermission($format->getPermissionName())) {
   

   You can simplify this a bit with $format->access('use')

3. +++ b/translators/tmgmt_local/src/Form/LocalTaskItemForm.php
   @@ -166,10 +167,24 @@ class LocalTaskItemForm extends ContentEntityForm {
   +            // In case a user has no permission to translate the content using
   +            // selected text format, change default text format and disable
   +            // the translation field.
   +            $form[$target_key]['source']['#format'] = 'plain_text';
   +            $form[$target_key]['translation']['#format'] = 'plain_text';
   +            $form[$target_key]['translation']['#allow_focus'] = FALSE;
   

   I think they can still see the content then? Which is what we want to prevent. See how TextFormat uses filter_form_access_denied()

   Since we don't allow to save anyway, it should be safe to switch out the text completely with a similar explanation that additionally says that it's not possible to accept this job item.

Duplicate the message is exactly what we need. Or show that in the source and keep translation empty, that works too.

Yes, what should happen with the translator is a problem.

See #1415234: Deal with text format permissions and security. text formats, security and permissions are never an easy problem. We've had that critical open since we started TMGMT. This is a good step towards solving that. #2688419: To create a continuous job you must have admin tmgmt permission was another part of the puzzle.

Oh. I didn't notice that you did this (only) in the local translator.

I'm actually not sure if we need this for the local translator at all. It will be quite common that they will be asked to translate content that they would not have permissions to, including formats. There is 1-2 problems for them, however: a) preview and b) auto-accept.

So for now, lets do this in this issue:

* Implement the same logic for reviewers, do not allow save as completed
* Simplify the logic again for local translator, simply do not use a text format if the user does not have access, display as a plain text field.

To solve the problems mentioned above, I discussed that with mIro and we agreed on this (new issue, possibly use #1415234: Deal with text format permissions and security for that): A new setting in tmgmt that lists all text formats and allows to check them. If nothing is selected, nothing is checked. If some are checked and a given job has a format that is not in the list of checked formats, do not provide preview links (directly in tmgmt_content) and skip auto-accept.

Close

1. +++ b/src/Tests/TMGMTUiTest.php
   @@ -890,6 +890,49 @@ class TMGMTUiTest extends EntityTestBase {
      /**
   +   * Tests text format permissions on translation fields.
   +   */
   +  public function testTextFormatPermissions() {
   +    // Create a job.
   +    $job1 = $this->createJob();
   +    $job1->save();
   +    $job1->setState(Job::STATE_ACTIVE);
   

   can we move this to the review test? I recently spent quite some time to remove methods from this test class as it is very long and slow (which means it can not be executed in parallel on testbot)

2. +++ b/src/Tests/TMGMTUiTest.php
   @@ -890,6 +890,49 @@ class TMGMTUiTest extends EntityTestBase {
   +    $this->assertNoLink(t('Save as completed'));
   

   that's a button, not a link? I don't think that would fail right now.

3. +++ b/src/Tests/TMGMTUiTest.php
   @@ -890,6 +890,49 @@ class TMGMTUiTest extends EntityTestBase {
   +
   +    // Translator should see enabled translation field again.
   +    $this->drupalGet('admin/tmgmt/items/' . $item1->id());
   +    $this->assertNoText(t('This field has been disabled because you do not have sufficient permissions to edit it. It is not possible to review or accept this job item.'));
   

   you should do a positive check for the button now here, an assertRaw() or so.

4. +++ b/translators/tmgmt_local/src/Form/LocalTaskItemForm.php
   @@ -248,10 +249,20 @@ class LocalTaskItemForm extends ContentEntityForm {
   +
   +          if ($format && $format->access('use')) {
   +            // In case a user has permission to translate the content using
   +            // selected text format, add a format id into the list of allowed
   +            // text formats. If no sufficient permission, plain text format will
   +            // be used.
   

   since we don't use #type text_format then, we won't use the plain text format, we won't have a format at all then.

5. +++ b/translators/tmgmt_local/src/Tests/LocalTranslatorTest.php
   @@ -430,8 +430,12 @@ class LocalTranslatorTest extends LocalTranslatorTestBase {
   +    // Assert that translator can see "Dummy" field.
        $this->clickLink(t('Translate'));
   +    $this->assertNoText(t('This field has been disabled because you do not have sufficient permissions to edit it. It is not possible to review or accept this job item.'));
   +
   

   like the example above, assertNo*() only asserts are dangerous as they can easily assert something that never exists (e.g. you could have a typo in the string that you're looking for, so it would never match).

   Instead, try to do a positive match. text_formt fields have a different, nested structure. So you can verify that you do not have that here but only a normal textfield.

Didn't do a lot of testing but this looks good now I think.