Problem/Motivation

_node_access_rebuild_batch_operation() is using entity queries that do access checks. Just after all grants are deleted. This means that only users with bypass node access or user 1 can rebuild the grants. Which prevents a hook_update_N of doing this. And potentially any user with the permission administer site configuration but not that permission can complete break node access.

Proposed resolution

Make the entity queries ignore node access.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#13 2785155-12.patch6.98 KBalexpott
#13 6-12-interdiff.txt6.36 KBalexpott
#13 2785155-12.test-only.patch5.22 KBalexpott
#6 2785155-6.patch1.41 KBalexpott
#6 4-6-interdiff.txt543 bytesalexpott
#4 2785155-4.patch1.66 KBalexpott
#4 2785155-4.test-only.patch835 bytesalexpott
#2 2785155-2.patch867 bytesalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Active » Needs review
FileSize
2785155-2.patch867 bytes

The D7 version of this uses db_query() and does not suffer from this.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2785155-4.test-only.patch835 bytes
2785155-4.patch1.66 KB

This is quite close to a critical because if a user did this there site would have plenty of access denied.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented 
+++ b/core/modules/node/node.module
@@ -1213,12 +1213,13 @@ function _node_access_rebuild_batch_operation(&$context) {
+    ->accessCheck(FALSE)
...
     // Disable access checking since all nodes must be processed even if the

There is an ->accessCheck(FALSE) just a few lines below :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
4-6-interdiff.txt543 bytes
2785155-6.patch1.41 KB

So it's just the count query that is wrong.

The last submitted patch, 4: 2785155-4.test-only.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Priority: Major » Critical

Promoting to critical because if you run the node access rebuild without the bypass node access permission or being user 1 the first thing it does is delete all node access records. And then it attempts to write them. Given that the max could be 0 this means only 20 nodes will get processed. Leaving all the other nodes without node access records. Nodes without node access records with be inaccessible so there's not a security problem but the only way to fix it is to get a user with the correct permissions to re-run the rebuild. However, given that there is no indication that something has gone horribly wrong people might not guess the correct work around.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

By reading \Drupal\node\Tests\NodeAccessRebuildNodeGrantsTest::testNodeAccessRebuildNodeGrants we should IMHO ensure that we got grants for everything we expect. This test coverage feels really loose.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@dawehner well it is better than \Drupal\node\Tests\NodeAccessRebuildTest which doesn't really test anything.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs review » Needs work

@dawehner but I agree we need to have more than 20 nodes and nodes which the user running the test can't access.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

@dawehner well it is better than \Drupal\node\Tests\NodeAccessRebuildTest which doesn't really test anything.

Wait we don't trust our messages?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
2785155-12.test-only.patch5.22 KB
6-12-interdiff.txt6.36 KB
2785155-12.patch6.98 KB

Expanded the test coverage.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented 
+++ /dev/null
@@ -1,36 +0,0 @@
-class NodeAccessRebuildTest extends NodeTestBase {

I've removed this test because it is totally not useful. It is duplicating what is in the other test without really test anything at all. Rebuilding node access without a node access module is tested by \Drupal\node\Tests\NodeAccessRebuildNodeGrantsTest::testNodeAccessRebuildNoAccessModules() and it does it better. Also this test should have caught this bug because the user that rebuilds node access doesn't have permissions to bypass node access. But there are no nodes and there are no node access modules installed :(

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review

DrupalCI error - something about going offline

The last submitted patch, 13: 2785155-12.patch, failed testing.

The last submitted patch, 13: 2785155-12.test-only.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs review » Reviewed & tested by the community

Nice find. Patch looks great to me.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

+1, looks good to me as well.

Related issue: #2671916: node_access_rebuild() will never work after since entityQuery has now accessCheck default set to true and the grants are deleted beforehand

not sure why we didn't cover this back then.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.2.x-dev » 8.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to all three 8.x branches, thanks!

  • catch committed e025875 on 8.3.x 
    Issue #2785155 by alexpott: _node_access_rebuild_batch_operation uses...

  • catch committed 81499c2 on 8.2.x 
    Issue #2785155 by alexpott: _node_access_rebuild_batch_operation uses...

  • catch committed fb17f9f on 8.1.x 
    Issue #2785155 by alexpott: _node_access_rebuild_batch_operation uses...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.