Problem/Motivation

The permission "edit any page content", "delete any page content" are broken if other modules implement hook_node_grants.

This is because #2204363: [sechole] Returning TRUE from hook_entity_access()/hook_ENTITYTYPE_access() must not bypass EntityAccessController::checkAccess() broke the logic in handling node grants.

Proposed resolution

Make the grants storage return neutral rather than forbidden.

Remaining tasks

User interface changes

None.

API changes

Before: The access() method of a grant storage implementation can return either allowed, forbidden or neutral, where neutral signals to the caller (NodeAccessControlHandler) that the default grant should be used.
After: The access() method should return either allowed or neutral. It can no longer rely on the caller to be aware of the default grant.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webflo’s picture

Status: Active » Needs review
FileSize
3.15 KB

Status: Needs review » Needs work

The last submitted patch, 1: node-access-grant-test-2461049.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: node-access-grant-test-2461049.patch, failed testing.

webflo’s picture

FileSize
2.97 KB
webflo’s picture

Status: Needs work » Needs review
agentrickard’s picture

AFAIK this is by design.

agentrickard’s picture

Poked d7 code. This is a regression.

agentrickard’s picture

Looks like a chicken-and-egg problem in these two methods:

Node::access() -- the entity handler.
NodeAccessControlHandler::access()

The NodeAccessControlHandler likely needs to enforce these permissions before passing to the module handler.

  public function access(EntityInterface $entity, $operation, $langcode = LanguageInterface::LANGCODE_DEFAULT, AccountInterface $account = NULL, $return_as_object = FALSE) {
    $account = $this->prepareUser($account);

    if ($account->hasPermission('bypass node access')) {
      $result = AccessResult::allowed()->cachePerRole();
      return $return_as_object ? $result : $result->isAllowed();
    }
    if (!$account->hasPermission('access content')) {
      $result = AccessResult::forbidden()->cachePerRole();
      return $return_as_object ? $result : $result->isAllowed();
    }
    // @TODO: Insert node type checks here.
    $result = parent::access($entity, $operation, $langcode, $account, TRUE)->cachePerRole();
    return $return_as_object ? $result : $result->isAllowed();
  }
agentrickard’s picture

Priority: Normal » Critical

Sadly, a critical regression.

webflo’s picture

I think it should be part of NodeAccessControlHandler:checkAccess similar to "view own unpublished content". But thats just a quick fix, the main bug is that the NodeAccessControlHandler has three states (allowed, neutral, forbidden) but the grant system has only two states and handles "neutral" as "forbidden".

agentrickard’s picture

The three states isn't an issue. Some permission has to affirm allowed; neutral == FALSE is fine because security insists we default to FALSE. This use-case dropped the "allowed" for a set of conditions.

It actually looks like the EntityAccessHandler code expects node_node_access() to still be present, but that was removed, which suggests that using checkAccess() may be correct. The code flow is a bit odd to me.

Adding the proper permission checks in checkAccess() should fix the issue, though.

Let me try a quick patch.

agentrickard’s picture

Status: Needs review » Needs work
FileSize
1.99 KB

You don't actually need the new module to test this behavior.

Here's a test that proves the permissions are not working as designed.

agentrickard’s picture

The "edit own..." and "delete own..." permissions are also not being enforced.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
3.21 KB

Here's a patch for the first two cases (with tests) and a placeholder for the "edit own" and "delete own" which also need to be included in these tests.

agentrickard’s picture

The test logic is good but the code needs work. node_node_access() still exists (but seems to be failing). Note that the last 3 new tests pass, even though I didn't write any new code for them.

The last submitted patch, 15: node-access-grant-test-2461049-15.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 16: node-access-grant-test-2461049-16.patch, failed testing.

The last submitted patch, 5: node-access-grant-test-2461049-2.patch, failed testing.

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
4.01 KB
1.58 KB

Fixed a couple of nitpicks. I noticed that core/modules/node/src/Tests/Views/BulkFormAccessTest.php is using something like:

$this->assertEqual(FALSE, $this->accessHandler->access($node, 'update', $node->prepareLangcode(), $account), 'The node may not be edited.');

instead

$this->assertEqual(FALSE, $node->access('update', $account), 'The node may not be edited.');

Not sure what's the right one.

Status: Needs review » Needs work

The last submitted patch, 20: node-access-grant-test-2461049-19.patch, failed testing.

agentrickard’s picture

We should actually use the convenience method in NodeTestBase $this->assertNodeAccess(array('view' => TRUE, 'update' => TRUE, 'delete' => TRUE), $node2, $web_user2);

agentrickard’s picture

I'm going to move these tests into the base NodeAccessTest, which is where they probably belong. I don't think that node_grants() actually matters here, as there is a bug earlier in the code (in node_node_access()).

agentrickard’s picture

Here's the fundamental problem, which causes the behavior change from Drupal 7.

In D7, hook_node_access() trumps the Node Access storage (grants) system.

In Drupal 8, we have this logic in the EntityAccessHandler:

     // We grant access to the entity if both of these conditions are met:
    // - No modules say to deny access.
    // - At least one module says to grant access.
    $access = array_merge(
      $this->moduleHandler()->invokeAll('entity_access', array($entity, $operation, $account, $langcode)),
      $this->moduleHandler()->invokeAll($entity->getEntityTypeId() . '_access', array($entity, $operation, $account, $langcode))
    );

    $return = $this->processAccessHookResults($access);

    // Also execute the default access check except when the access result is
    // already forbidden, as in that case, it can not be anything else.
    if (!$return->isForbidden()) {
      $return = $return->orIf($this->checkAccess($entity, $operation, $langcode, $account));
    }

This says "If hook_entity_access or hook_ENTITY_TYPE_access returns TRUE, then check the checkAccess() method for the entity in question."

Ok, fine, but then in NodeAccessHandler, we discard the hook checks entirely. The code is:

    // Check if authors can view their own unpublished nodes.
    if ($operation === 'view' && !$status && $account->hasPermission('view own unpublished content') && $account->isAuthenticated() && $account->id() == $uid) {
      return AccessResult::allowed()->cachePerPermissions()->cachePerUser()->cacheUntilEntityChanges($node);
    }

    // If no module specified either ALLOW or KILL, we fall back to the
    // node_access table.
    $grants = $this->grantStorage->access($node, $operation, $langcode, $account);

That means we never actually CHECK for the ALLOW or KILL, we just leap straight to the grantStorage check, so these permissions always fail.

This does seem to be a regression, as I don't think this logic is intentional. The problem now is that we cannot just call EntityAccessHandler to get the proper values, as that causes an endless loop.

So it looks like we have to call the hooks again in order to test their values.

agentrickard’s picture

Hm. This behavior was introduced in #2204363: [sechole] Returning TRUE from hook_entity_access()/hook_ENTITYTYPE_access() must not bypass EntityAccessController::checkAccess() and is deliberate. But I don't understand why we are invalidating the hooks, which were designed to allow modules to enhance / bypass the grants system in the first place.

Berdir’s picture

So, it took me a while to understand this issue.

In short, this issue basically says that at least as far as nodes are concerned, #2204363: [sechole] Returning TRUE from hook_entity_access()/hook_ENTITYTYPE_access() must not bypass EntityAccessController::checkAccess() was *not* a security issue, it was a feature.

Which means we have two options.

a) We say that this issue was not a security issue for any entity type, that's just how it works. And revert the patch from there and move on. I did consider the performance implications of the change, and already said "do it or won't fix" in comment #38 because of that, but it didn't click for me (or anyone else ;)) that it actually breaks stuff.

b) Implement option c) that I proposed in comment #39 there, which means passing AccessResult $result to checkAccess() so that implementations there can consider the access as returned by the hooks.

I'm not sure what the correct fix is.

agentrickard’s picture

IMO, the current behavior is wrong. We specifically allow the hooks in D7 to cover use cases like the following that cannot be handled by the grants or permissions system:

* Do not allow any use whose account is less than 3 days old to EDIT any content.
* After 3 days, let them have the permission as granted.

The grants system has always been a fallback. What the current approach has done -- see the tests above -- is destroy the permissions "edit any FOO" if a node access module is used.

We could restore that fairly easily, but the exception code in NodeAccessHandler::checkAccess() that deals with unpublished nodes shows why the grants system is a fallback -- it can't handle complex cases, so it cannot be asked to have the final word on all cases.

The code in checkAccess() even shows this discrepancy through this comment, which exists even though checkAccess() cannot read the result of the parent operation:

    // If no module specified either ALLOW or KILL, we fall back to the
    // node_access table.
agentrickard’s picture

Status: Needs work » Needs review
FileSize
2.66 KB

Here's the current status.

* I removed the !$result->isForbidden() logic in the access handler, as that logic breaks the desired functionality.
* I replaced that check with $result->isNeutral() to run the access handler if no hook gave a definitive answer.
* The NodeAccessTest class adds missing tests for the "edit/delete all BUNDLE" checks.
* A new NodeAccessGrantsTest class simply wraps NodeAccessTest and enables a node access module in order to test that the default module behaviors are still respected when a hook_node_grants() are present.

This new test seems to indicate a bug, either in implementation or testing, as the two fails suggest:

* View permissions on unpublished content are not enforced by Node Access tests because they must be enforced at the query level. If that's true, we have to rewrite NodeTestBase::assertNodeAccess() to adjust.

* The update check that is passing makes no sense to me, and might be a bug.

agentrickard’s picture

agentrickard’s picture

Last patch forgot to add the new file. Sorry; it's been awhile.

The last submitted patch, 28: node-access-grant-test-2461049-28.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 30: node-access-grant-test-2461049-30.patch, failed testing.

Berdir’s picture

I didn't check the other test fails yet, but EntityAccessControlHandlerTest is exactly the test that the other issue added.

Crell’s picture

Based on the discussions and analysis we did back in Drupal 7 when hook_node_access() was first added, I would argue that we should go a step further and just remove edit/delete checks from the node access system entirely and use it just for View.

Short version of the argument: There's 5 operations on a node: Create, Read, Update, Delete, and List. Of those, the first four are reasonable and safe to do entirely at runtime in PHP, and considerably more flexible (the "edit my own X for 3 hours" type case that Ken mentioned). The only one that is problematic is List, ie, "view as part of a list", partially because of performance but primarily because of paging. You physically can't do paging and userspace access control at the same time. That's not an issue for any of the other operations. (The cases where you want to have a list of updatable or deleteable objects are few, are almost all admin screens, and will generally have the node loaded anyway and displayed; you're just showing/not showing a link, which doesn't have the same problem. If you can edit something then you also have view access to it.)

YesCT’s picture

Issue tags: +Security

sounds security related.

xjm’s picture

Issue tags: -Security

Regarding #35, there is not actually a direct security issue. The bug is that access is not granted when it should be; there is no unintended disclosure. @agentrickard's test patch demonstrates this.

Regarding #34, that's out of scope for 8.x at this point, even if I were convinced it wouldn't regress functionality from Drupal 7 (which I'm not). Getting rid of the node access system as a special flower and handling list grants in a scalable way for all entities is something we should aspire for in the long term, but let's not scope-creep this issue. :)

Arla’s picture

Assigned: Unassigned » Arla

I'm looking into this during #drupaldevdays.

xjm’s picture

Issue tags: +D8 Accelerate Dev Days
Arla’s picture

Status: Needs work » Needs review
Issue tags: +drupaldevdays
FileSize
5.18 KB
4.22 KB

So. Have been digging into this and trying to understand it all :) Got some help from @Berdir and @xjm.

Reverted the !isForbidden() -> isNeutral() change, we think it is attacking the faulty logic in the wrong way. The grant storage now returns allowed/neutral (rather than allowed/forbidden), and the result is simply or'ed with the 'allow if published' fallback.

For NodeAccessGrantsTest, we need to skip the actual grants in the grants hook, because it "Grants users view, update, and delete privileges on nodes they have authored" which breaks the "User cannot 'view own unpublished content'" assertion.

The failing assertion in BulkFormAccessTest was wrong to begin with. It was added in #2172017: Bulk operations does not respect entity access which was committed after the broken #2204363: [sechole] Returning TRUE from hook_entity_access()/hook_ENTITYTYPE_access() must not bypass EntityAccessController::checkAccess() that this issue is supposed to fix. It fails now because the Allowed in node_node_access() is taken into account—as it should. Not sure if we can just invert/remove the assertion, though...?

Status: Needs review » Needs work

The last submitted patch, 39: node-access-grant-test-2461049-39.patch, failed testing.

Arla’s picture

I confused myself by re-reading that last paragraph... Trying to rephrase it.

The assertion in BulkFormAccessTest expects non-access to updating the "private" node.
The relevant points are:

  1. node_node_access(), which returns Allowed in that case, invoked by EntityACH
  2. the condition that determines whether to additionally ask the entity-type-specific ACH (i.e. $this->checkAccess())
  3. the access result from the grants storage, which in this case has so far been Forbidden, but #39 changes it to Neutral
  4. the way NodeACH handles the grants result

In HEAD, the assertion passes because NodeACH or's the node_node_access() result with the grants result (Allowed OR Forbidden => Forbidden)
In patch #30, the assertion fails because NodeACH is not considered (Allowed => Allowed)
In patch #39, the assertion still fails, but now because NodeACH or's the node_node_access() result with the changed grants result (Allowed OR Neutral => Allowed)

Arla’s picture

Status: Needs work » Needs review
FileSize
7.57 KB
4.15 KB

1. Lots of fails because when there are grants hooks returning Neutral, it is or'ed with the positive 'view if published' fallback. The fallback is only supposed to be considered if there are no grants hooks. Here I moved that fallback into the grants storage, because it's just a code equivalent of the default grant anyway.

2. Corrected BulkFormAccessTest... I think. But I am confused because it applies Unpublish on a node that is editable (now, at least) and then successfully asserts that nothing happens.

Arla’s picture

Talked to @dawehner who did most of the BulkFormAccessTest issue #2172017: Bulk operations does not respect entity access. The assertion was correct; the point of it is to check that updating through the bulk form is denied if normal updating is denied. And it failed because normal updating was not denied anymore. So I just make sure it is denied another way.

xjm’s picture

Assigned: Arla » xjm

Reviewing this!

xjm’s picture

Went down the rabbit hole a bit and filed #2471663: Undeprecate EntityHandlerBase.

xjm’s picture

Assigned: xjm » Unassigned
Issue summary: View changes
Status: Needs review » Needs work

Sorry for the delay in reviewing this; there was a lot of background to understand for a 7K patch. :) Overall this approach makes more sense to me.

  1. +++ b/core/modules/node/src/NodeAccessControlHandler.php
    @@ -109,21 +109,8 @@ protected function checkAccess(EntityInterface $node, $operation, $langcode, Acc
    -    // If no modules implement hook_node_grants(), the default behavior is to
    -    // allow all users to view published nodes, so reflect that here.
    -    if ($operation === 'view') {
    -      return AccessResult::allowedIf($status)->cacheUntilEntityChanges($node);
    -    }
    -
    -    // No opinion.
    -    return AccessResult::neutral();
    
    +++ b/core/modules/node/src/NodeGrantDatabaseStorage.php
    @@ -69,8 +69,14 @@ public function access(NodeInterface $node, $operation, $langcode, AccountInterf
    +      if ($operation === 'view') {
    +        return AccessResult::allowedIf($node->getTranslation($langcode)->isPublished())->cacheUntilEntityChanges($node);
    +      }
    +      else {
    +        return AccessResult::neutral();
    +      }
    

    So we're essentially moving this logic from the access control handler into the storage. This kind of makes sense, because it is actually the default behavior of the node grant storage. And it also makes a lot of sense for the logic to be in one place, rather than half in the grant storage and half in the handler.

    One concern I have though is that this is a conceptual behavior for the node access system, not specific to any one storage, but now we're moving it specifically into the database storage. So a non-database storage would need to duplicate this code. And on the flip it seems to me that it is something that alternate node access implementations might want to override.

    Maybe part of my hesitancy is that we don't have a generic implementation -- there is only NodeGrantDatabaseStorageInterface. So my concern might be out of scope for this issue.

    Not 100% sure, and it's definitely much better to do it this way than changing the behavior of all access control handlers in a way that was essentially reverting #2204363: [sechole] Returning TRUE from hook_entity_access()/hook_ENTITYTYPE_access() must not bypass EntityAccessController::checkAccess().

     

  2. +++ b/core/modules/node/src/NodeAccessControlHandler.php
    @@ -109,21 +109,8 @@ protected function checkAccess(EntityInterface $node, $operation, $langcode, Acc
    +    // Evaluate node_access table records created by hook_node_grants().
    +    return $this->grantStorage->access($node, $operation, $langcode, $account);
    

    So this comment is a little confusing... hook_node_grants() does not create node access records. It defines grant realms that are used in node access records. Related: filed #2473041: Deprecate node_access_grants() and move its functionality to NodeAccessControlHandler.

     

  3. +++ b/core/modules/node/src/NodeGrantDatabaseStorage.php
    @@ -69,8 +69,14 @@ public function access(NodeInterface $node, $operation, $langcode, AccountInterf
    +      // Return the equivalent of the fallback grant: allow to view if node is
    +      // published.
    

    Minor point: This mentions the fallback grant (that the global view grant for all realms), but there's not actually any explanation of what it is here. And the only mentions of "fallback" in the class are for translation fallbacks, which are a different thing. It's actually called the "default grant" (see NodeGrantDatabaseStorage::writeDefault() and its kin).

    Filed #2473093: Node access default grant behavior is not clear to explore this more. (I'm fine with doing this here for now and then having a followup). However, let's improve the documentation here and at least add @see to the relevant methods etc.

     

  4. +++ b/core/modules/node/src/NodeGrantDatabaseStorage.php
    @@ -117,7 +123,7 @@ public function access(NodeInterface $node, $operation, $langcode, AccountInterf
    -      return $set_cacheability(AccessResult::forbidden());
    +      return $set_cacheability(AccessResult::neutral());
    
    +++ b/core/modules/node/src/NodeGrantDatabaseStorageInterface.php
    @@ -106,7 +106,7 @@ public function writeDefault();
    -   *   The access result.
    +   *   The access result, either allowed or neutral.
    

    This makes total sense. The database storage shouldn't try to have the final word on everything; that's counter to the overall paradigm of entity access in D8. The new behavior here is also in keeping with the D7 behavior of the node grant system which only was additive and never explicitly forbade access except in the case where the node grant system was active but nothing granted access. (There are other ways to explicitly forbid access when desired.)

     

  5. +++ b/core/modules/node/src/Tests/NodeAccessGrantsTest.php
    @@ -0,0 +1,42 @@
    +    // Make sure the grants in node_access_test_node_access_records() are not
    +    // actually considered, because they break assertions in NodeAccessTest. The
    +    // fact that the hook is implemented is enough for this test.
    +    \Drupal::state()->set('node_access_test.private', TRUE);
    +    node_access_test_add_field(NodeType::load('page'));
    +  }
    

    Hmm, even with this comment, I don't understand what's going on here. It's not clear to me how the comment has anything to do with the call to node_access_field_test_add_field().

    Shouldn't we just provide a different, simpler test module rather than the one that is so burdened with behavior we don't want for this test? It seems fragile. Granted (pun not intended but there it is), the whole node access test suite is pretty intense to begin with. :)

    A loosely related followup might be to improve the documentation for node_access_test_add_field(). (I swear I had a patch to do this among lots of other work on the test implementation's documentation) like two years ago, but it got blocked on nitpicks and then core moved on.) :P

     

  6. Finally, since we moved the test coverage around, can we provide a new test-only patch to expose the coverage?

     

(I also still need to read and grok #41 and #42 WRT the bulk form change, but I want to get the rest of this feedback posted first.)

Excellent sleuthing and patching @Arla!

xjm’s picture

(And trying out the newly storable contributor credit for reviewers that apparently I can set as a maintainer.)

xjm’s picture

Arla’s picture

Great, fixed the comments. Really looking forward to having those grants docs/API fixed in the other issues, because currently this stuff is quite difficult to understand. (I still don't, really.)

Status: Needs review » Needs work

The last submitted patch, 50: node-access-grant-test-2461049-50-TESTS_ONLY.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Remember to upload test only patches first so the issue isn't reset.

This looks good to me. @xjm, do you think we need more reviews or do you feel comfortable with RTBC'ing this?

Feedback from @webflo and/or @agentrickard would be great.

The last submitted patch, 13: node-access-grant-test-2461049-13.patch, failed testing.

larowlan’s picture

+++ b/core/modules/node/src/Tests/NodeAccessTest.php
@@ -18,6 +18,7 @@
+

nit:unneeded change

Other than that, RTBC+1

Arla’s picture

rteijeiro’s picture

Issue summary: View changes
rteijeiro’s picture

Code looks fine to me. Does it need manual testing?

Arla’s picture

Updated IS with current solution.

Arla’s picture

Issue summary: View changes

Trying again.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

I discussed this more with @Arla and @Berdir. All the changes in #50 are just what I think is needed. We can explore other improvements in the followups that I've filed, but the patch here covers everything that's in scope to fix the critical. I like that we were able to restore the expected behavior without changing anything outside the node system.

Thanks @Arla for taking this on!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I've reviewed the patch and it looks good. I've read through @agentrickard's comments and the fix looks like it is inline with his expectations. The patch has the expected test coverage and documentation. Committed 3153536 and pushed to 8.0.x. Thanks!

  • alexpott committed 3153536 on 8.0.x
    Issue #2461049 by Arla, agentrickard, webflo, rteijeiro, xjm, Berdir:...
Wim Leers’s picture

Great clean-up! :) And glad to have this thoroughly reviewed & criticized by agentrickard & xjm — having their sign-off is reassuring IMO :)

Status: Fixed » Closed (fixed)

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