Unauthorized users able to view unpublished translations

This patch needs review.

Patch applies cleanly, but does not work right.

Having the default language (English) version of a node published and the secondary version published may or may not be viewed by the anonymous user.

With the patch in place, translated nodes may or may not been viewed. I see no pattern, yet.

The API documentation is here:
https://api.drupal.org/api/drupal/core!modules!node!node.api.php/functio...

Is this still the only issue blocking a stable release?

It is the single known issue.

However, I need to see a lot more testing from the 50000 users of this project before going for a stable release.

when you say you need to see a lot more testing from the users... what exactly do you mean? You need new unit tests to be coded? Or you want to see the user count increased--more users means more people "testing" the functionality of the module by using it and either creating issues or not?

Sorry for my confusion. Thanks for the help.

when you say you need to see a lot more testing from the users... what exactly do you mean?

For instance, take this issue. It passes the automatic tests, but no human has bothered testing the contributed patch except me (my test concluded that it does not to the job, and needs work).

How can the 50000+ users of this project expect this to fixed if they can't even bothered to test it, point out what is wrong, and propose a better solution?

thanks for the clarification. If I get a change to test this, I will.

As the maintainer, I'd think you have final say to declare the patch unsatisfactory. If you can reproduce the problem described in this issue, then adjusting the patch to conform to those requirements should be within your power. That said, I assume you are maintaining this module unpaid, out of the kindness of your heart, so I can understand why you might not have time to devote to developing patches of your own and rely primarily on patches submitted by the community.

In any case, thanks for the work, we appreciate it. I will try to get more involved if/when I can.

We just realised we might be seeing the same problem happening, even though we're coming to it from a different angle:

1: have a published english node
2: create and unpublished translation of that node

i'd expect as anonymous to be able to see the english node and not the translation. Instead i can't access them both.

3: i save the english node once more

now i can see both translations as anonymous.

So we'll install the patch and run some testing :). Thanks a lot!

So we tested the patch.

From what it seems to us, the patch is not working properly.

1: create a published En page
2: create an unpublished IT page

as anon i can't access EN nor IT

save the english page once more, then it seems to work.

save the Italian unpublished page => as anonymous i can't now access EN nor IT.

thanks @joey-santiago for testing and reporting!

The thing is the content_access module doesn't actually insert into node_access any information about the language of the node.

I tried tweaking content_access_node_access_records, adding there some code to load all translations of the current node and then add the node_access records needed, but to me it seems the content_access_optimize_grants should also be enabled to take language information into account.

I also wonder is it correct using

node->isPublished() === TRUE

to insert node_access information in the table? Maybe yes, if we only want people to access published contents?

This is the patch i'm working on. Still faulty, but maybe it can be used as a starting point?

Sorry guys, could you help me out understanding this? So what should happen is we insert for every language we have published a new record in the node_access table, right?

SELECT * FROM node_access WHERE nid=145724;
+--------+----------+----------+-----+-------+------------+--------------+--------------+
| nid    | langcode | fallback | gid | realm | grant_view | grant_update | grant_delete |
+--------+----------+----------+-----+-------+------------+--------------+--------------+
| 145724 | de       |        0 |   0 | all   |          1 |            0 |            0 |
| 145724 | en       |        1 |   0 | all   |          1 |            0 |            0 |
+--------+----------+----------+-----+-------+------------+--------------+--------------+

then when we unpublish the English translation, we remove that record:

SELECT * FROM node_access WHERE nid=145724;
+--------+----------+----------+-----+-------+------------+--------------+--------------+
| nid    | langcode | fallback | gid | realm | grant_view | grant_update | grant_delete |
+--------+----------+----------+-----+-------+------------+--------------+--------------+
| 145724 | de       |        0 |   0 | all   |          1 |            0 |            0 |
+--------+----------+----------+-----+-------+------------+--------------+--------------+

And so on?

joey-santiago, thank you for sharing your insight into this!

I'm the current maintainer of the project, and I am following this closely. However, I don't have any multilingual webites, my insight into the multilingual aspect of Drupal is pretty shallow. It is good to someone who actually uses it on the ball.

FWIIW (not much I am afraid), here is my feedback.

Do we want people only to access published content?

There are core permissions "Bypass content access control" and "View own unpublished content". My take is that these permissions supersede any content access restrictions set by this project.

As for the questions in comment #14, I see that your patch in comment #13 alters hook_node_access_records() to take language into account. You may be into something, but I need to dig deeper to be able to answer.

Here's a new patch with also a test for this. Currently the test fails. When it runs, you see anonymous is able to access the English node. Then i add an unpublished French translation and voilà: anonymous user is not able to access the English node anymore.

So now that i read your comment, it would make imho sense not to do anything in the content_access module IF the translation is not published... It would make sense then leaving core dealing with those unpublished node's permissions.

Sorry i forgot the patch :(

The way the module works with the current patch is definitely still wrong. Here's what it does:

    // Login as admin and save the English node.
    $this->drupalLogin($this->adminUser);
    $this->node1->save();
    // Now anonymous can access both translations.
    $this->drupalLogout();
    $this->drupalGet('fr/node/' . $this->node1->id()); // This is unpublished!!
    $this->assertSession()->pageTextContains($this->frenchTitle);
    $this->drupalGet('node/' . $this->node1->id());
    $this->assertSession()->pageTextContains($this->englishTitle);
    // Login as admin and save the French node.
    $this->drupalLogin($this->adminUser);
    $nodeFr->save();
    // Logout, anonymous now won't get anywhere.
    $this->drupalLogout();
    $this->drupalGet('fr/node/' . $this->node1->id());
    $this->assertSession()->pageTextContains($this->t('Access denied'));
    $this->drupalGet('node/' . $this->node1->id()); // This is published!!
    $this->assertSession()->pageTextContains($this->t('Access denied'));

Digging deeper into this, to me it seems that this module was simply not designed to take languages into account :). What happens in the function content_access_optimize_grants is it expects the array of grants to be language agnostic, having either a role id or a string as indexes. The thing is instead grants should be inserted per language:

+--------+----------+----------+-----+----------------------+------------+--------------+--------------+
| nid    | langcode | fallback | gid | realm                | grant_view | grant_update | grant_delete |
+--------+----------+----------+-----+----------------------+------------+--------------+--------------+
| 145725 | en       |        1 |   1 | content_access_roles |          1 |            0 |            0 |
| 145725 | es       |        0 |   1 | content_access_roles |          1 |            0 |            0 |
| 145725 | en       |        1 |   2 | content_access_roles |          1 |            0 |            0 |
| 145725 | es       |        0 |   2 | content_access_roles |          1 |            0 |            0 |
| 145725 | en       |        1 |   3 | content_access_roles |          1 |            0 |            0 |
| 145725 | es       |        0 |   3 | content_access_roles |          1 |            0 |            0 |
+--------+----------+----------+-----+----------------------+------------+--------------+--------------+

I really hope i made sense and i understood correctly the docs from https://api.drupal.org/api/drupal/core%21modules%21node%21node.api.php/f....

And here's a new patch that hopefully would make sense out of this.

The tests pass, while with the old patch on content_access module, this test was failing... so i'm positive we're now one step closer to an end.

thanks for the awesome contributions and clear explanations @joey-santiago!

@joey-santiago should this be "Needs review"?

I had the same issue joey-santiago experienced as explained in #10.

After applying the patch from #20, the nodes that weren't accessible started showing in the correct views and became accessible again.

If anything else about this issue needs to be tested let me know.

Thx @joey-santiago for the fix!

August 2021 was a long time ago.

Patch now fails to apply. It needs to be re-rolled against the latest 8.x-1.x-dev snapshot.

Unfortunately. I don't have the time right now, and need to rely on the community to get this re-rolled - but it looks promising.

Applied patch 20 with commit f0a0c56f successfully but got error when rebuilding permissions:
"Duplicate entry for key PRIMARY in node_access"

So could not test if it worked correctly, no time atm to debug.

I can confirm that this still happens on 8.x-1.x-dev.

To recap, here are all the steps to reproduce:
- Install clean Drupal and Content Access module
- drush si -y && drush en admin_toolbar admin_toolbar_tools language content_access content_translation -y && drush uli
- Rebuild permissions due to Content Access installation (/admin/reports/status/rebuild)
- Add a new language for the site (/admin/config/regional/language)
- Allow translating the Article content type (/admin/structure/types/manage/article → Language settings → Enable translation)
- Create a new Article in the default language (EN) and leave it published
- Translate the Article in the newly created language (FI in this case) and don't publish it
- Access to both EN and FI pages now return 403 for anonymous user even though the EN page can be seen published on the edit form
- Edit the EN content and re-save it
- Both contents are now visible for anonymous users and the unpublished FI translation shows the red "unpublished" background on its content

Patch didn't apply cleanly against 8.x-1.x, so here's a reroll. This will now include the changes done in https://www.drupal.org/project/content_access/issues/3262813 as well.

What happened before these changes:
When saving a node, access records were by default created for all translations, because the isPublished() check was done only for the content being saved (not the translations). This meant that saving the published EN content added allowing access record for FI (or any other translations) as well.

What should happen after the patch has been installed:
When saving a node, access records are only created for the translations that are published.

We'd still need to double-check that all this doesn't mess up things for other content access modules (like the quite popular View Unpublished).

We always fix the latest version.

Thanks @gisle, and thank you @iSampo for the clear replication steps and new patch.

I am going to be out of touch for a week or two, but if this hasn’t been wrapped up by the time I get back I will test your replication steps to see if I can reproduce the issue, then I can confirm/reroll the patch.

Would be cool to move out of alpha :)

Rerolled patch from comment #27 against HEAD.

Having another go at it.

Automatic tests pass. We also need manual reviews by community members.

I spent some time today testing the patch supplied by iSampo in comment #27 (after rerolling it for the HEAD of 2.0.x-dev – i.e. the patch in comment #32), it the results are very encouraging 😁.

I would like to see some more testing, but I plan to commit this to the repo soon.

It looks like it has been fixed. This has been committed to the latest snapshot of 2.0.x-dev.

I've checked the the Drupal 7 branch (version 7.x-1.2) for this bug, but it looks that the Drupal 7 branch is OK.

THANK YOU!

Guys patch is fine, but it missed something. When node view is not for all users it appears this error on translation:
Drupal\Core\Entity\EntityStorageException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '689868-7-content_access_roles-es' for key 'node_access.PRIMARY': INSERT INTO "node_access"
To fix it, content_access_get_type_grant function "$defaults" static variable should take in consideration node language. So this function should looks like:

/**
 * Returns the default grants for a given node type.
 */
function content_access_get_type_grant(NodeInterface $node) {
  // Cache per type default grants in a static array.
  static $defaults = [];

  $node_type = $node->getType();

  if (!isset($defaults[$node_type][$node->language()->getId()])) {
    $grants = [];

    // Only process the 'view' op as node_access() will take care of
    // edit and delete.
    foreach (content_access_get_settings('view', $node_type) as $rid) {
      $gid = content_access_get_role_gid($rid);
      $grant['grant_view'] = 1;
      $grants[] = content_access_proccess_grant($grant, $gid, $node);
    }
    $defaults[$node_type][$node->language()->getId()] = $grants;
  }

  // Care for the author grant.
  $grant = $grants = [];
  $settings = [
    'view' => content_access_get_settings('view', $node_type),
    'view_own' => content_access_get_settings('view_own', $node_type),
  ];
  $grant['grant_view'] = content_access_own_op($node, $settings['view'], $settings['view_own']);
  if ($grant['grant_view']) {
    $grant['realm'] = 'content_access_author';
    $grants = [
      'author' => content_access_proccess_grant($grant, $node->getOwnerId(), $node),
    ];
  }

  return $defaults[$node_type][$node->language()->getId()] + $grants;
}

Reopening, based on comment #39.

I posted Ion Macaria's patch based on his comment 39 in issue 3364261. Please review.

The fix for #3364261: Integrity constraint violation: 1062 Duplicate entry after upgrade module has recently been committed. Does this problem still exist?

@Liam Morland yeah, that's the exact same issue isn't it? So although @ion.macaria hasn't confirmed that it fixed their issue, given that the patch came from their suggested fix, I think we can be pretty sure it does.
Also, others tested and confirmed that #3364261: Integrity constraint violation: 1062 Duplicate entry after upgrade module was fixed etc.

Thus, I'm closing this ticket again.

For version 8.x