Problem/Motivation

On bulk forms no access checking is performed. This means that once you got access to admin/people you can pretty much do whatever you want

Entity access checks are not performed when executing a bulk operation from entity overview pages, such as the content overview page (admin/content) or the user overview page (admin/people).

Proposed resolution

  • Add a new method on the ActionInterface:
    +  public function access($operation, $object, AccountInterface $account = NULL, $return_as_object = FALSE);
    
  • Each action plugin has to implement based upon the object, whether the account can perform actions on it
  • We rely on the field access checking, when we just update $node->status, but we rely on
    entity access checking for things deleting nodes
  • In the actual bulk operation code we check access for the chosen action on each row, and execute it, if access was granted

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Reroll the patch if it no longer applies. Instructions
(after a reroll) Create a patch that is fixed so installs and tests run. Include an interdiff Instructions
Add automated tests Instructions

User interface changes

None

API changes

See beta evaluation

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category It is a bug, because not checking access on a misconfigured view could allow people to add roles, for example and so
get any permission
Issue priority Critical, because of security
Prioritized changes The main goal of this issue is security.
Disruption Somehow disruptive because of the following reasons:
  • Action plugins (probably all defined by rules modules) have to implement the ::access() method

Original issue

How to reproduce

For user admin view

  1. Create a user (useradmin with uid 2) with the following permissions:
    • administer users
    • access user profiles
  2. Implement hook_user_access() in a custom module and deny delete access on any user entity (see code example below).
  3. Create an authenticated user (dummy with uid 3).
  4. Login as useradmin.
  5. Try to go the cancel page of the dummy user (user/3/cancel). You should get an access denied page, so you are not allowed to cancel the user.
  6. Go the user overview page (admin/people).
  7. Select the dummy user from the overview and try to apply the action "Cancel the selected user account(s)" on it.
    You will go to admin/people/cancel and you are able to select a cancel method. If you choose, for example, "Delete the account and its content." the user is deleted although you shouldn't be able to do that. $entity->access('delete') will return FALSE for user 3.

For node admin view

The steps to reproduce for nodes are similar, though if the user does not have the permission "administer nodes", access to admin/content/node/delete is denied even if the user is allowed to delete the selected node.

Implementation of hook_user_access()

/**
 * Implements hook_ENTITY_TYPE_access() for entity type "user".
 */
function mymodule_user_access($entity, $operation, $account) {
  if ($operation == 'delete') {
    return FALSE;
  }
}
Files: 
CommentFileSizeAuthor
#132 2172017-132.patch43.97 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,831 pass(es). View
#130 interdiff.txt979 bytesdawehner
#130 2172017-130.patch44.47 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2172017-130.patch. Unable to apply patch. See the log in the details link for more information. View
#126 interdiff.txt1.88 KBdawehner
#126 2172017-126.patch44.2 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,822 pass(es). View
#123 interdiff.txt1.99 KBdawehner
#123 2172017-123.patch44.52 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,740 pass(es). View
#119 interdiff.txt7.08 KBdawehner
#119 2172017-119.patch44.51 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,720 pass(es). View

Comments

xjm’s picture

Version: 8.0-alpha7 » 8.x-dev
Priority: Normal » Critical
Issue tags: +VDC

Thanks @MegaChriz, good find!

@digiv, @larowlan, @dawehner and I discussed this issue today and followed similar steps for nodes in D7 with VBO. From @digiv:

I installed content access, I created 3 nodes, one with no changes, one that denies changes to everyone and one that allows anyone to edit. I built a view to display the 3 nodes, and I disabled query rewrites. All of them show up. When I go to use VBO to make changes, it says that the changes was good, however, the change fails except for the one where the user was granted rights.

@larowlan confirmed with similar steps and a different node access setup. So, it looks like VBO is properly prevented from peforming the operation on rows the user doesn't have access to. Therefore, this issue can continue as a D8-only public issue.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
2.37 KB
FAILED: [[SimpleTest]]: [MySQL] 59,852 pass(es), 1 fail(s), and 0 exception(s). View

The confirm form ones are easy enough, not sure about the rest of them...

That said, when someone has 'administer users', I'm not really worried about hook_user_access().

Status: Needs review » Needs work

The last submitted patch, 2: action-2172017-2.patch, failed testing.

dawehner’s picture

@tim.plunkett
I would assume that every action basically has an entity operation, so we need to check access using this. Therefore we basically need to add a method on the AccessInterface?

MegaChriz’s picture

Maybe a good start would be to write a test for this case. I've some experience with writing tests for d7 contrib modules and I'm in the process of learning PHPUnit, so I think I should be able to write a test for this one.

Now how should we test this? And should it be done via SimpleTest or PHPUnit?

Possible things to test:

  • The test could follow the steps to reproduce as described in the issue summary, though then you would only catch access issues based on the user entity and not on other entities.
  • There could be test case for every entity type that has an admin view with bulk operations, so one for "node", one for "user", one for "comment" (perhaps). Are there other entities in Drupal core that have an admin view with bulk operations?
MegaChriz’s picture

Status: Needs work » Needs review
FileSize
5.1 KB
FAILED: [[SimpleTest]]: [MySQL] 64,662 pass(es), 2 fail(s), and 0 exception(s). View

I made a start with writing a test for this issue. I used SimpleTest for now, because I've more experience with that and also because the related \Drupal\user\Tests\Views\BulkFormTest is also based on SimpleTest.

The test tests the following:

  • If an account can not be blocked if the active user has no permission to "update" the account.
  • If an account can not be deleted if the active user has no permission to "delete" the account.

The test should have two failed assertions because the bug is not fixed yet.

Details

  • I considered to add the two test methods to the existing BulkFormTest, but as it tests two things in relation (bulk form + entity access) I put it in a separate class.
  • A module called "user_access_test" was added to implement the hook hook_user_access().
  • The same view as in BulkFormTest is used.
  • The test user subjects in the tests have fixed usernames, so only one module implementing hook_user_access() needed to be added.
  • In the delete access test, two test user subjects are added, so the cancel conformation page is still displayed once the bug is fixed. This way, I could make an assertion on that the critical user is not deleted. Else, the assertion should have been made on not displaying the cancel conformation page which I found a less good test cover. Hm, maybe there should be two test methods for delete access?

I would like some feedback on this test, before I make a similar one for the node entity.

MegaChriz’s picture

Side note: I found out that any user who has access to an user view that provides the operation "Block the selected user(s)" can block any user (except perhaps user 1, not tested). I would consider this a misconfiguration, so I didn't include this in the tests.

Status: Needs review » Needs work

The last submitted patch, 6: drupal.bulk-form-entity-access-2172017-6.patch, failed testing.

EclipseGc’s picture

So, fwiw, I really oppose tying any sort of access check to actions. Drupal has done similar historically and I've provided a number of patches over the past couple version to undo this in the worst places. A great example is node_add. At one time there was a check beyond what the menu item had on it meaning if you menu_altered that you'd get a white screen if you didn't have the permission to create nodes of that type, which is counter intuitive since you altered the menu to explicitly allow that. I fixed this in either d6 or d7, I don't recall.

This same logical problem exists for forms and is why you should never define an if statement around an element of form definition, always use #access.

In short, this is more of a "profile" problem than a Drupal problem. We continue to approach access with the idea that we should provide some sort of access paradigm beyond "menu" and while I acknowledge the general need for such a thing, we should loosely couple that as much as possible and fall back to depending on menu based access whenever possible. If you build a view with actions and give a user access to that view, then the assumption should follow that they can perform those actions. This is far more useful than the opposite which comes with mounds of code undoing Drupal's basic assumptions and is the primary problem in creating really great new install profiles and also has historically been the primary complaint from people performing that task. We all talk about Drupal as a platform, but the coupling of permissions and actions (as just one example) is a really good example of how we really prevent greater flexibility of Drupal in that space. I'd really encourage us to NOT add an access restriction component to actions.

Eclipse

MegaChriz’s picture

@EclipseGc
You have a point. Adding an access layer to actions just feels conceptually wrong. I'm reading about the plugin API lately (I'm still in the process of exploring the D8 concepts) and I read the following about it:

A plugin should do just one thing and do it well.

So, yeah, an action plugin should not have to bother about access.

However, I don't agree with you that access checks should be limited to menu's/router paths. In that light you could also argue that the node access table shouldn't be queried when building a list of nodes.

My thought about solving this issue is that the list of entities should be filtered *before* it is handed over to the action plugin. So, that should be Views' responsability I guess. Additionally, there should be an option in Views to skip access checks for bulk operations (just as you can now with disabling SQL writing to skip node view access checks) to address use cases where access checks are undesired. I'm not sure yet if this should be set per bulk operation or for the complete display only, though I think for the complete display makes more sense.

Berdir’s picture

My thought about solving this issue is that the list of entities should be
filtered *before* it is handed over to the action plugin.

Yes. But the action needs to say what kind of access is required for it. You might have a action that just flags a node and another one deletes it.

This is also how VBO works, see _views_bulk_operations_entity_access(), which verifies entity access based on an access mask that the action returns.

And yes, when it's then outside of the plugin, it could be possible to not check it and execute it anyway, that's up to the caller. But we have an entity access API and anyone who is working with entities in the context of a user must respect it unless told otherwise, everything else is a bug.

EclipseGc’s picture

I'm not going to fight on this, I said my peace and I'll leave this be, but all of my points still stand. Treating Drupal this way is prioritizing the needs of the Standard install profile over the needs of all other install profiles, which has been a common theme going on for forever now and is one of the biggest contributing factors to making install profiles more difficult than necessary. Do what you will, but just make it as easy to override as possible, cause you are putting assumptions in place that people are going to want to undo (more often then not). Also keep in mind the override behavior for this has to be simple both inside and outside of VBO.

Eclipse

tim.plunkett’s picture

I agree with @Berdir.

We can add a new access method to action plugins.
The "simple VBO" can choose to check it.
Other code can choose to bypass it.

MegaChriz’s picture

Status: Needs work » Needs review
FileSize
25.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,565 pass(es), 24 fail(s), and 0 exception(s). View
9.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,648 pass(es), 4 fail(s), and 0 exception(s). View

We can add a new access method to action plugins.

I'm not sure. This makes action plugins dependent on Drupal\Core\Access\AccessibleInterface and Drupal\Core\Session\AccountInterface. I read about plugins that they should be focussed on one task and they eventually should be reusable outside of Drupal. It seems to me that adding these dependencies would defeat that thought.

I can see the benefits of adding an access layer to action plugins, though. That way it's easier for code interacting with actions to perform access checks instead of figuring out themselves how to check access, thus the chance for security issues decreases.
Still, that would make an action plugin make assumptions about in which context it is run and because a plugin shouldn't be bothered with in which context it is run (so I understand), I'm not sure if we should add knowledge about entity access to action plugins, even when calling the access() method is optional.

Patch

I now attempted to fix this issue using a similar approach as in Views Bulk Operation 7.x-3.x. I also added a test for for the node bulk operation form. Both probably still need some work.

The fix

  • I've added a property called "operations" on the action plugin definition. This is similar to what's called 'behavior' in Drupal 7 (see hook_action_info().
  • I've defined "operations" for action plugins of modules node, user and comment.
  • In Drupal\system\Plugin\views\field\BulkForm I've added access checks on entities.
  • When access checks fail in BulkForm, the message "Skipped %operation on @type %title due to insufficient permissions." is displayed for each entity the access check failed on. This is the same message as in VBO 7.x-3.x.
  • I added methods checkAccess() and executeAction() to BulkForm as an attempt to break things up in smaller pieces and to hopefully make BulkForm easier overrideable.

Additionally, there should be an option in Views to skip access checks for bulk operations to address use cases where access checks are undesired.

Note that I've not added this option yet.

Node: Bulk form entity access

Access is tested using node access records and the the private node feature from the node_access_test module. This may not be the ideal way of testing this bug, because the node access records and the the private node feature were originally designed for an other test. Also, the test testNodeDeleteAccess() currently would also pass if only "update" access checks are performed. So, this test would still needs some work.

Attached are two patches:

  • One with the proposed fix and the tests (the bulk form entity access tests passes locally using Drupal 8.0-alpha10).
  • One with only the tests to ensure they fail without the fix. There should be four failed assertions.

The last submitted patch, 14: drupal.bulk-form-entity-access-fix-2172017-14.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 14: drupal.bulk-form-entity-access-tests-only-2172017-14.patch, failed testing.

dawehner’s picture

Well, your approach makes actions, which are not bound to entities yet, bound to entity operations.

Given that I fear that we need to add an access method.

MegaChriz’s picture

Well, your approach makes actions, which are not bound to entities yet, bound to entity operations.

Yeah, I can see why that's wrong.

Just checking access for update/delete might be to limited

I've been playing with ideas of how to solve this issue. Now that I have ported the User protect module to Drupal 8 (which implements hook_user_access()), I came to the conclusion that just checking for one of the known entity operations (create, view, update, delete) might not be enough. Modules (like User protect) may want to block certain properties from being edited. Say that there is an operation that changes user names. It should then respect the permission "Change own username". Same would count if a module invents a permission for publishing content, that module should then be able somehow to get access checks performed for the "node publish" action. If access checks for actions would be limited to update and delete, then how can such module make the distinction between which actions are allowed?

Action as operation?

The following idea popped up in my mind: what if actions themselves are the operation? So to check if a certain action is permitted on an entity, one would simply do this:

$entity->access($action->id());

Modules that provide actions for entities would then implement hook_entity_access() to provide a fallback access check for each action:

/**
 * Implements hook_entity_access().
 */
function node_entity_access($entity, $operation, $account, $langcode) {
  switch ($operation) {
    case 'node_assign_owner_action':
    case 'node_unpromote_action':
    case 'node_promote_action':
    case 'node_publish_action':
    case 'node_save_action':
    case 'node_make_sticky_action':
    case 'node_unpublish_by_keyword_action':
    case 'node_unpublish_action':
    case 'node_make_unsticky_action':
      return $entity->access('update', $account, $langcode);

    case 'node_delete_action':
      return $entity->access('delete', $account, $langcode);
  }
}

I've done something similar in the User protect module. Check userprotect_user_access() for an example:
http://cgit.drupalcode.org/userprotect/tree/userprotect.module?h=8.x-1.x

Caveats

  • I'm not sure how this would work out for actions that are not entity based.
  • This model may not work well for actions that make changes to multiple entities at once. But then again, are such actions available anywhere in Views?
fago’s picture

For Rules, actions definitely need access control - that's a pre-existing feature in d7.

In d7, there are actually two kind of access control mechanism what can be confusing and should probably be avoided in d8. Firts off, there is the 'access callback' which you can add to your action_info, it's used for determining access to configure an action in the first place. Then, there is also an access() method on the action obejct (and a respective _access callback for action implementors) which allow you to determine access control for a configured action object.

For d8, this reminds me of entities, which have create access + access for acting on entity objects as well. However, the problem of which plugins are available for a given user to configure in terms of access control seems to be a plugin-generic problem to me.

Limiting action access to certain entity operations is certainly to limited, in particular given #2011038: Use Context system for actions.

fago’s picture

Issue tags: +d8rules

Tagging d8rules as this is related.

dawehner’s picture

Thank you for your feedback?

Indeed, you could think of providing different plugins for different users, based, for example on permissions. On the block side, allow
different people to place different kind of blocks, which is totally different from accessing the block itself.

So if we want to add access, we have to add both a "listAccess" and an "access".
While access is pretty obvious, I wonder whether "list access" depends on the interiors of the plugin itself. Won't this be
something people might want to "configure" depending on their editorial workflow of the page? Using one permission
per plugin does not seem to be scalable at all for the UI.

EclipseGc’s picture

We should NOT be considering any sort of default functionality of this sort of plugins at large. If your plugin type needs something like this, fine. If you want that in a trait so you can reuse it elsewhere, fine, but don't put this on all plugins everywhere. And while we're on the topic, I don't want to be permission bound on use of plugins. I've said for a very very long time now that allowing access of a particular action within a particular vbo is completely kosher. The view has access controls already and bypassing some other set of access controls by giving access to the view is a perfectly relevant use case. Could it cause problems? absolutely, but it will cause far fewer problems than the development pain of undoing ridiculous access checks.

I said it 6 months ago, I'll say it again, I think even considering this line of reasoning is folly. You will, of course, do whatever it is that you will do, but keeping the needs of install profiles OTHER than standard in mind would be a really great way to operate going forward into the future.

Eclipse

dawehner’s picture

FileSize
4.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

So, what about this kind of interface as first step?

Sharique’s picture

Status: Needs work » Needs review

Submitting patch in last comment for testing.

Status: Needs review » Needs work

The last submitted patch, 23: 2172017-access-23.patch, failed testing.

tim.plunkett’s picture

I like @dawehner's proposal to keep it simple and let each plugin handle things. But each plugin might not have enough data to make a decision when this is called? That needs more investigation.

That said, as far as Actions go I'd probably defer to @fago, as I'd very much like to prevent Rules from having to fork core actions if possible.

Here are some notes about the patch itself.

  1. +++ b/core/lib/Drupal/Core/Plugin/AccessAwarePluginInterface.php
    @@ -0,0 +1,32 @@
    +interface AccessAwarePluginInterface {
    

    Does this need to be plugin specific? We have \Drupal\Core\Access\AccessibleInterface... Also not sure how #2287071: Add cacheability metadata to access checks might affect this.

  2. +++ b/core/lib/Drupal/Core/Plugin/AccessAwarePluginInterface.php
    @@ -0,0 +1,32 @@
    +   *   TRUE if the plugin should be shown, or FALSE otherwise.
    

    Oh I guess this is less complicated (no NULL)

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

I like @dawehner's proposal to keep it simple and let each plugin handle things. But each plugin might not have enough data to make a decision when this is called? That needs more investigation.

In case this is true, some wrapper should care about that. The plugin should just know about itself.

Let's just get the patch pass to not demotivate people to discuss it here.

In general there are cases where the access actually depends on the object itself, what should we do in this case?

Status: Needs review » Needs work

The last submitted patch, 27: access-2172017-27.patch, failed testing.

fago’s picture

That said, as far as Actions go I'd probably defer to @fago, as I'd very much like to prevent Rules from having to fork core actions if possible.

Thanks! :)

In case this is true, some wrapper should care about that. The plugin should just know about itself.

Agreed. The plugin can take its configuration and other set stuff into account if necessary, but the interface used for checking should stay clean. Howsoever, this should probably be just AccessibleInterface ?

dawehner’s picture

Agreed. The plugin can take its configuration and other set stuff into account if necessary, but the interface used for checking should stay clean. Howsoever, this should probably be just AccessibleInterface ?

Agreed.

xjm’s picture

Issue tags: +Needs tests
Désiré’s picture

Assigned: Unassigned » Désiré
Désiré’s picture

Assigned: Désiré » Unassigned
Status: Needs work » Needs review
FileSize
2 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,777 pass(es), 11 fail(s), and 0 exception(s). View
7.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,816 pass(es), 1 fail(s), and 0 exception(s). View

Here I rerolled the last patch (seems it's broken), and created a test case for it.

The last submitted patch, 33: views_bulk_access-2172017-test_only-33.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 33: views_bulk_access-2172017-33.patch, failed testing.

Antti J. Salminen’s picture

Assigned: Unassigned » Antti J. Salminen

Working a bit with the patch from #27. Am I missing something or is the one posted in #33 not based on that one at all?

Antti J. Salminen’s picture

Assigned: Antti J. Salminen » Unassigned
FileSize
24.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Hopefully this is better, it should make use of AccessibleInterface and the install pass but not much more.

marthinal’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 37: access-2172017-37.patch, failed testing.

dawehner’s picture

Does someone know whether we will be able to change the API here?

--- a/core/lib/Drupal/Core/Block/BlockBase.php
+++ b/core/lib/Drupal/Core/Block/BlockBase.php
@@ -151,7 +151,7 @@ public function calculateDependencies() {
   /**
    * {@inheritdoc}
    */
-  public function access(AccountInterface $account) {
+  public function access($operation, AccountInterface $account = NULL, $return_as_object = FALSE) {

Its not such a distruptive change to be honest.

EclipseGc’s picture

Why? What is the use case?

Eclipse

Sam Hermans’s picture

FileSize
24.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

re-rolled without the unused $operation

-  public function access(AccountInterface $account) {
+  public function access(AccountInterface $account = NULL, $return_as_object = FALSE) {
Sam Hermans’s picture

FileSize
30.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

installs correctly now

tim.plunkett’s picture

Status: Needs work » Needs review

The last submitted patch, 42: access-2172017-38.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 43: access-2172017-39.patch, failed testing.

The last submitted patch, 43: access-2172017-39.patch, failed testing.

YesCT’s picture

Issue summary: View changes
Issue tags: +Needs reroll

also needs a reroll.

added remaining tasks (the dreditor insert tasks button is useful to do that). It is important to keep the issue summary up to date (especially if this issue is to be used for the Friday critical office hours).

larowlan’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
30.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
larowlan’s picture

Issue tags: +CriticalADay

Status: Needs review » Needs work

The last submitted patch, 49: bulk-access-2172017.49.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.99 KB
31.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Status: Needs review » Needs work

The last submitted patch, 52: bulk-access-2172017.45.patch, failed testing.

larowlan’s picture

Patch at 42 made incorrect changes, reverting

larowlan’s picture

Re-roll of 37

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.07 KB
25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,115 pass(es), 2,417 fail(s), and 843 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 56: bulk-access-2172017.56.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.34 KB
26.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,782 pass(es), 28 fail(s), and 9 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 58: bulk-access-2172017.58.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
661 bytes
26.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,823 pass(es), 7 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 60: bulk-access-2172017.60.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
2.02 KB
28.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,898 pass(es), 4 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 62: bulk-access-2172017.62.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.11 KB
29.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,093 pass(es). View
larowlan’s picture

FileSize
237.98 KB

green - anyone care to review?

horse playing piano

MegaChriz’s picture

I put reviewing the patch on my todo list. First glance, it seems to me that checking for a permission for using a particular operation is too limited. I think it should be possible to override access with hook_entity_access(). Also, I see that the tests I wrote in #14 are no longer included in the latest patch.
Note that I haven't followed this issue for a while, so I should take a closer look at the patch first to know if what I'm saying here still makes sense.

Damien Tournoud’s picture

Status: Needs review » Needs work

The changes in Drupal\views\Plugin\views\access\AccessPluginBase and Drupal\Core\Block\BlockPluginInterface, while they might be desirable are orthogonal to this issue and are API breaks. I think they should belong to a separate issue.

Also, I don't understand why the new AccessibleInterface added to Drupal\Core\Action\ActionInterface is not used anywhere. Isn't that the whole point of this issue?

+  /**
+   * {@inheritdoc}
+   */
+  public function access($operation, AccountInterface $account = NULL, $return_as_object = FALSE) {
+    return $account->hasPermission('administer comments');
+  }

^ Most of those are bogus: $account is optional and $return_as_object is not respected.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +Ghent DA sprint
FileSize
29.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,399 pass(es), 47 fail(s), and 25 exception(s). View
24.94 KB

The changes in Drupal\views\Plugin\views\access\AccessPluginBase and Drupal\Core\Block\BlockPluginInterface, while they might be desirable are orthogonal to this issue and are API breaks. I think they should belong to a separate issue.

I have to agree ... #2393041: Use AccessibleInterface for views access plugins. #2393045: Use AccessibleInterface for block plugins.

Also, I don't understand why the new AccessibleInterface added to Drupal\Core\Action\ActionInterface is not used anywhere. Isn't that the whole point of this issue?

Well yeah that issue got started with just some discussion about how the API looked like and than implementation started, stopped for a while,
so knowledge about it got totally forgotten.

^ Most of those are bogus: $account is optional and $return_as_object is not respected.

At lesat we return booleans everywhere, much better than objects.
This interdiff should reflect this problem, hopefully.

Adapt the code to do so, and also changed the existing test, which just totally ignores access checking ...
Let's just allow everything!

Status: Needs review » Needs work

The last submitted patch, 68: 2172017-68.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
30.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,475 pass(es). View
2.09 KB

This could be it.

jibran’s picture

Issue tags: -Needs tests

Over all patch look good to me. Just a minor doc issue. I think we can remove needs test tag now. Other then that it is RTBC.

+++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
@@ -223,6 +224,12 @@ protected function getBulkOptions($filtered = TRUE) {
@@ -261,6 +268,11 @@ public function viewsFormSubmit(&$form, FormStateInterface $form_state) {

We are throwing an exception in the function now so function doc needs update.

dawehner’s picture

FileSize
31.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,494 pass(es). View
834 bytes

Alright

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thanks. I think it is ready.

PS: Love the horse playing piano gif.

dawehner’s picture

We most probably need a change record and beta evaluation!

MegaChriz’s picture

Assigned: Unassigned » MegaChriz

I will the test the patch in #72 in a few days.

dawehner’s picture

Issue summary: View changes
dawehner’s picture

Issue summary: View changes
tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/system/src/Entity/ActionAccessControlHandler.php
    @@ -0,0 +1,27 @@
    +    /** @var \Drupal\system\ActionConfigEntityInterface $entity $f */
    

    Extraneous $f

  2. +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
    @@ -223,6 +224,12 @@ protected function getBulkOptions($filtered = TRUE) {
    +        // @fixme Find out the right operation.
    +        if (!$action->access('view', $this->view->getUser())) {
    
    @@ -261,6 +271,11 @@ public function viewsFormSubmit(&$form, FormStateInterface $form_state) {
    +      if (!$action->access('view', $this->view->getUser())) {
    

    Yes, let's actually fix this :-)

    I would suggest 'execute' as we have $action->execute()

dawehner’s picture

Status: Needs work » Needs review
FileSize
31.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,568 pass(es). View
1.76 KB

Agreed

Status: Needs review » Needs work

The last submitted patch, 79: 2172017-79.patch, failed testing.

almaudoh queued 79: 2172017-79.patch for re-testing.

dawehner’s picture

Status: Needs work » Needs review

Mh, so its actually green.

larowlan’s picture

@jibran if you like the horse playing piano - you can watch *and* listen thanks to this handy 10 hour mix https://www.youtube.com/watch?v=OWFBqiUgspg

Slightly nsfw.

I like to play this via the office speakers every now and again to make sure people are paying attention.

;)

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Other then this minor nit it is RTBC

+++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
@@ -227,7 +227,7 @@ protected function getBulkOptions($filtered = TRUE) {
         // @fixme Find out the right operation.

This can be removed now.

PS: @larowlan 10 hrs that is crazy.

tstoeckler’s picture

+++ b/core/modules/action/src/Plugin/Action/GotoAction.php
@@ -113,4 +115,12 @@ public function submitConfigurationForm(array &$form, FormStateInterface $form_s
+  public function access($operation, AccountInterface $account = NULL, $return_as_object = FALSE) {
+    $result = AccessResult::allowed();
+    return $return_as_object ? $result : $result->isAllowed();
+  }

Not downgrading, because I *think* the current code is OK, but want to make sure: Should we be checking for the 'execute' operation here?

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update
  1. +++ b/core/modules/node/src/Plugin/Action/PromoteNode.php
    @@ -29,4 +31,12 @@ public function execute($entity = NULL) {
    +    $result = AccessResult::allowedIfHasPermission($account, 'administer nodes');
    

    Shouldn't this (and elsewhere) be checking 'bypass node access'?

    Or does it assume that the user already has access to the node itself before it gets this far? Not sure why we don't check actual entity access here rather than the permission at all, and this isn't clear from the issue summary either.

  2. +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
    @@ -223,6 +224,12 @@ protected function getBulkOptions($filtered = TRUE) {
    +        // @fixme Find out the right operation.
    

    That probably needs to be fixed?

dawehner’s picture

Shouldn't this (and elsewhere) be checking 'bypass node access'?

Or does it assume that the user already has access to the node itself before it gets this far? Not sure why we don't check actual entity access here rather than the permission at all, and this isn't clear from the issue summary either.

This is yet another of these cases where things get tricky:

  1. Yes $entity->access() would be ideal for things like promote.
  2. Sadly for bulk operations you want to have a list of actions, before you actually have the $entity
  3. Given that I would assume we should use $entity, in case its available, and call that in case we actually execute the actions.
    For the listing we have to fallback on the current way of doing

Does that sound sane?

xjm’s picture

Issue tags: +Triaged D8 critical
MegaChriz’s picture

Assigned: MegaChriz » Unassigned
Status: Needs work » Needs review
FileSize
29.3 KB
41.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,897 pass(es), 5 fail(s), and 0 exception(s). View
10.31 KB

Two issues in one

So there actually two issues in one here:

  1. Check access for which of the operations the current user may use at all (no individual entity context).
  2. Check if the selected operation may be performed for each selected entity in the View.

I originally reported this issue for the second one: for example that you could delete an entity via a bulk operation even though entity access for deleting that same entity returns false.

Maybe issue 2 can not be fixed before fixing issue 1, but the title of this issue (and summary) still reflects issue 2.

Patch in #79

Entity access is ignored

I tested the patch in #79 specifically on entity access by implementing hook_ENTITY_TYPE_access() and returning Drupal\Core\Access\AccessResult::forbidden();. Logged in as an user with the permission "administer users", this resulted in an access denied page for user/x/cancel, but the user could still be deleted via the bulk form.

Empty bulk form field

I edited the user admin view to be viewable by everyone and logged as an user with just the "access content" permission. This resulted into an "empty" bulk form field as seen in the image below.

I think the bulk form field should be hidden when there are no operations to choose. Maybe fixing this should be handled in a follow-up issue so to keep the focus on fixing entity access.

New patch

The attached patch adds in back the tests from #14 (which I updated to work with the latest core):

  • Drupal\node\Tests\Views\BulkFormAccessTest
    Tests if someone with the "administer nodes" permission can not edit/delete nodes in bulk that are protected by node access records. Note that the node access handler specifically returns FALSE when asked if the node may be edited or deleted.
  • Drupal\user\Tests\Views\BulkFormAccessTest
    Tests if someone with the "administer users" permission can not edit/delete users in bulk that are protected by the module user_access_test which implements hook_ENTITY_TYPE_access() for entity type "user". Note that user/x/edit and user/x/cancel result into a 403 page.

These tests should report 4 failed assertions. Moving state to "Needs review" so the testbot will evaluate these tests, though the issue status should still be "Needs work".

Status: Needs review » Needs work

The last submitted patch, 89: 2172017-89.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
24.4 KB
41.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,845 pass(es), 43 fail(s), and 6 exception(s). View

Thank you a lot to add the tests, really helpful.

After talking with @bojanz and @tim.plunkett we agreed that we just check access on the row level but not for the list of available plugins.

Status: Needs review » Needs work

The last submitted patch, 91: 2172017-91.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
5.85 KB
41.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,904 pass(es), 6 fail(s), and 0 exception(s). View

Fixed the test failure, partially caused by new test coverage introduced earlier.

Status: Needs review » Needs work

The last submitted patch, 93: 2172017-93.patch, failed testing.

dawehner’s picture

So using 'administer nodes' as 'bypass node edit access' seems to be a bad idea.

After talking with @berdir on IRC, we realized that maybe we could use checkFieldAccess and not directly
entity access. This would directly has the advantage that 'administer nodes' will work for those actions, given that field access has specific code for 'edit' which
uses 'administer nodes'.

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.42 KB
42.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,979 pass(es). View

Iterated on top of the idea in #95

damiankloip’s picture

  1. +++ b/core/modules/comment/src/Plugin/Action/PublishComment.php
    @@ -36,7 +36,7 @@ public function execute($comment = NULL) {
    +    return $object->status->access('edit', $account, $return_as_object);
    
    +++ b/core/modules/comment/src/Plugin/Action/UnpublishComment.php
    @@ -37,7 +37,7 @@ public function execute($comment = NULL) {
    +      return $object->status->access('edit', $account, $return_as_object);
    

    Nice, I like this!!

  2. +++ b/core/modules/user/src/Tests/Views/BulkFormAccessTest.php
    @@ -14,7 +14,7 @@
    +class   BulkFormAccessTest extends UserTestBase {
    

    Rogue spaces

  3. +++ b/core/modules/user/src/Tests/Views/BulkFormAccessTest.php
    @@ -53,9 +53,23 @@ public function testUserEditAccess() {
    +    $this->drupalPostForm('test-user-bulk-form', $edit, t('Apply'));
    

    Should we assert the response from the submitted form? What is the expected behaviour there? Nothing? error message?

dawehner’s picture

FileSize
694 bytes
42.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,621 pass(es). View

Rogue spaces

Can't find that in the recent patch ...

Should we assert the response from the submitted form? What is the expected behaviour there? Nothing? error message?

We test later that we have blocked the user.

MegaChriz’s picture

For the empty bulk form issue (see #89) I created a new issue: #2402155: Empty bulk form field when no actions are available. I noticed though that that issue no longer occurs with the patch from #98.
@dawehner Is access for available actions postponed or completely out of the question? Anyway, great that the issue is focussing now on the thing I originally created it for.

I've shortly tested the patch in #98 and I noticed the following:

  • I'm able to block users (with the "administer users" permission) if I would not be allowed to update (edit) them. Is this intentional?
  • Changing roles is prevented when I'm not allowed to edit the user in question. This is good.
  • Deleting an user is prevented when I'm not allowed to deleted the user in question. This is good.
  • When trying to apply an action which is not allowed for the selected entity, nothing happens. Maybe there should be an error message displayed? Or would that message be unhelpful since it may not be able to tell how to resolve the error? (The action could be prevented by any module.)
dawehner’s picture

@dawehner Is access for available actions postponed or completely out of the question? Anyway, great that the issue is focussing now on the thing I originally created it for.

I'd kind of prefer fixing that in another issue, given that its a different problem space and not, as you said, the actual described problem we have.

I'm able to block users (with the "administer users" permission) if I would not be allowed to update (edit) them. Is this intentional?

That is a good question why. UserAccessControlHandler::checkFieldAccess() has the following piece of code:

    // Administrative users are allowed to edit and view all fields.
    if (!in_array($field_definition->getName(), $explicit_check_fields) && $account->hasPermission('administer users')) {
      return AccessResult::allowed()->cachePerRole();
    }

so this seems to be intended ... asked berdir about those kind of issues in the node module, and it was totally not clear whether we have a strategy here.

When trying to apply an action which is not allowed for the selected entity, nothing happens. Maybe there should be an error message displayed? Or would that message be unhelpful since it may not be able to tell how to resolve the error? (The action could be prevented by any module.)

What would you expect to happen? Maybe getting a 403 in case you could not update any item (and send the message on top of it?)

dawehner’s picture

Issue summary: View changes

Worked quite a lot of the issue summary to bring it up to date.

dawehner’s picture

Issue summary: View changes
Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: -Needs issue summary update
  1. From the proposed resolution in the IS:

    We rely on the field access checking, when we just update $node->status, but we rely on entity access checking for things deleting nodes

    That sounds wrong. Field access checking for updating individual fields is great, but it should also check access to the entity. Quick Edit does this also for example. See EditEntityFieldAccessCheck.

  2. Related to above: I'm missing tests for bulk field updates access checking.
  3. +++ b/core/lib/Drupal/Core/Action/ActionInterface.php
    @@ -44,4 +46,27 @@
    +   * Checks data value access.
    

    s/data value/object/

  4. +++ b/core/lib/Drupal/Core/Action/ActionInterface.php
    @@ -44,4 +46,27 @@
    +   * @param string $operation
    +   *   The operation to be performed. 'execute' for checking access whether the
    +   *   $account can execute the action on the particular $object.
    

    Can an action ever have multiple operations? I don't think so?

    Then why always pass in 'execute' instead of just omitting the $operation parameter?

  5. +++ b/core/modules/action/src/Tests/BulkFormTest.php
    @@ -63,6 +63,13 @@ public function testBulkForm() {
    +    // Login as a user with 'administer nodes' permission to have access
    +    // to the bulk operation.
    

    - s/login/log in/
    - 80 cols

  6. +++ b/core/modules/comment/src/Plugin/Action/UnpublishComment.php
    @@ -29,4 +31,14 @@ public function execute($comment = NULL) {
    +    if ($operation == 'execute') {
    

    See my first remark. If we check this here, we should check if *everywhere*. Hence it makes more sense IMHO to just omit $operation altogether.

  7. +++ b/core/modules/node/src/Plugin/Action/PromoteNode.php
    @@ -29,4 +31,15 @@ public function execute($entity = NULL) {
    +  public function access($operation, $object, AccountInterface $account = NULL, $return_as_object = FALSE) {
    +    /** @var \Drupal\node\NodeInterface $object */
    +    $status_access = $object->status->access('edit', $account, TRUE);
    +    $promoted_access = $object->promote->access('edit', $account, TRUE);
    +    $access = $status_access->andIf($promoted_access);
    +    return $return_as_object ? $access : $access->isAllowed();
    +  }
    
    +++ b/core/modules/node/src/Plugin/Action/StickyNode.php
    @@ -29,4 +31,15 @@ public function execute($entity = NULL) {
    +  public function access($operation, $object, AccountInterface $account = NULL, $return_as_object = FALSE) {
    +    /** @var \Drupal\node\NodeInterface $object */
    +    $status_access = $object->status->access('edit', $account, TRUE);
    +    $sticky_access = $object->sticky->access('edit', $account, TRUE);
    +    $access = $status_access->andIf($sticky_access);
    +    return $return_as_object ? $access : $access->isAllowed();
    +  }
    

    Should also check access to the Node 'update' operation. See EditEntityFieldAccessCheck.

  8. +++ b/core/modules/node/src/Tests/Views/BulkFormAccessTest.php
    @@ -0,0 +1,165 @@
    + * Tests if entity access is respected on a node bulk form.
    

    s/node bulk form/node bulk operations form/

    ?

  9. +++ b/core/modules/node/src/Tests/Views/BulkFormAccessTest.php
    @@ -0,0 +1,165 @@
    +    $node = entity_load('node', $node->id(), TRUE);
    ...
    +    $node = entity_load('node', $node->id(), TRUE);
    ...
    +    $own_node = entity_load('node', $own_node->id(), TRUE);
    

    Node::load()

  10. +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
    @@ -249,18 +250,30 @@ protected function getBulkOptions($filtered = TRUE) {
    +        // Skip execution if the user did not had access.
    +        if (!$action->getPlugin()->access('execute', $entity, $this->view->getUser())) {
    +          continue;
    +        }
    

    Don't we want to inform the user about which objects he could not modify, due to insufficient permissions?

  11. +++ b/core/modules/user/src/Plugin/Action/AddRoleUser.php
    @@ -7,6 +7,8 @@
    +use Drupal\Core\Access\AccessResult;
    +use Drupal\Core\Session\AccountInterface;
    
    +++ b/core/modules/user/src/Plugin/Action/RemoveRoleUser.php
    @@ -7,6 +7,8 @@
    +use Drupal\Core\Access\AccessResult;
    +use Drupal\Core\Session\AccountInterface;
    

    Useless 'use' statements, because these are the only changes to these files :) Probably a remnant from earlier patch iterations.

  12. +++ b/core/modules/user/src/Tests/Views/BulkFormAccessTest.php
    @@ -0,0 +1,112 @@
    +    $account = entity_load('user', $account->id(), TRUE);
    ...
    +    $account = entity_load('user', $account->id(), TRUE);
    ...
    +    $account = entity_load('user', $account2->id(), TRUE);
    

    User::load()

kim.pepper’s picture

Assigned: Unassigned » kim.pepper

Taking a look.

larowlan’s picture

Issue tags: +Critical Office Hours
kim.pepper’s picture

Status: Needs work » Needs review

Fixes issues in #103 except 10. Not sure how we do this? drupal_set_message?

Also, the entity_load() changes were resetting cache. Assume this isn't needed.

kim.pepper’s picture

FileSize
41.09 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,668 pass(es), 13 fail(s), and 0 exception(s). View
26.9 KB

...and the patches.

larowlan’s picture

Can an action ever have multiple operations? I don't think so?

Then why always pass in 'execute' instead of just omitting the $operation parameter?

I think for interface parity.

So this might blow up.

Status: Needs review » Needs work

The last submitted patch, 107: 2172017-bulkops-access-104.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
42.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,678 pass(es), 17 fail(s), and 0 exception(s). View
9.17 KB

Some works ...

Status: Needs review » Needs work

The last submitted patch, 110: 2172017-110.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
44.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,711 pass(es), 2 fail(s), and 0 exception(s). View
8.66 KB
  • Rewrote the tests partially to make sense.
  • Added a message for every missing access.

Status: Needs review » Needs work

The last submitted patch, 112: 2172017-112.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
44.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,710 pass(es). View
810 bytes

There we go.

kim.pepper’s picture

RTBC +1 from me.

kim.pepper’s picture

Assigned: kim.pepper » Unassigned
larowlan’s picture

Assigned: Unassigned » Wim Leers
+++ b/core/modules/user/src/Tests/Views/BulkFormAccessTest.php
@@ -0,0 +1,140 @@
+    debug(String::format('No access to execute %action on the @entity_type_label %entity_label.', [
+      '%action' => 'Block the selected user(s)',
+      '@entity_type_label' => 'User',
+      '%entity_label' => $no_edit_user->label(),
+    ]));

Leftover? other than that I think this is ready (and really this could stay if pressed)
Back to Wim for a final look.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned

Looking great!

  1. Relative nit: I think s/$result/$access/ wherever you're dealing with an AccessResult object would make the code more legible.
  2. +++ b/core/modules/node/src/Tests/Views/BulkFormAccessTest.php
    @@ -90,19 +91,27 @@ public function testNodeEditAccess() {
    +    $this->assertEqual(TRUE, $node->status->access('edit', $account), 'The node can be edited.');
    

    s/node/node status/

  3. +++ b/core/modules/user/src/Tests/Views/BulkFormAccessTest.php
    @@ -36,41 +37,67 @@ class BulkFormAccessTest extends UserTestBase {
    +    debug(String::format('No access to execute %action on the @entity_type_label %entity_label.', [
    +      '%action' => 'Block the selected user(s)',
    +      '@entity_type_label' => 'User',
    +      '%entity_label' => $no_edit_user->label(),
    +    ]));
    

    Like larowlan, I think this is a debug leftover, but like larowlan I think the committer can probably remove this on commit.

Leaving at NR because of point 1. dawehner, if you (or others) agree with point 1, I could reroll it for you if you want.

dawehner’s picture

FileSize
44.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,720 pass(es). View
7.08 KB

Let's fix those points quickly.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Needs review

Couple of actual questions. Couple of very minor nits.

  1. +++ b/core/modules/comment/src/Plugin/Action/UnpublishByKeywordComment.php
    @@ -67,4 +67,15 @@ public function submitConfigurationForm(array &$form, FormStateInterface $form_s
    +    $result = $object->access('update', $account, TRUE)
    

    Is there somewhere we can document the requirement to have both entity and field access. It seems a bit fragile having to call both from within the actions each time.

  2. +++ b/core/modules/node/src/Plugin/Action/StickyNode.php
    @@ -29,4 +30,16 @@ public function execute($entity = NULL) {
    +    $sticky_access = $object->sticky->access('edit', $account, TRUE);
    

    Why does the sticky/promoted etc. access check also check status access?

  3. +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
    @@ -249,18 +250,35 @@ protected function getBulkOptions($filtered = TRUE) {
    +        // Skip execution if the user did not had access.
    

    s/had/have/

  4. +++ b/core/modules/user/src/Tests/Views/BulkFormAccessTest.php
    @@ -0,0 +1,135 @@
    + * Tests if entity access is respected on an user bulk form.
    

    nit - a user.

  5. +++ b/core/modules/user/src/Tests/Views/BulkFormTest.php
    @@ -35,8 +35,15 @@ class BulkFormTest extends UserTestBase {
    +    // Login as a user without 'administer users.
    

    nit 'administer users'

Berdir’s picture

1. Is documented on EntityAccessControlHandlerInterface::fieldAccess(). The reason is that we do not want to call entity access 20 times if we are checking access to twenty different fields. It would also not always be clear how to map between the two exactly, as they work a bit different, especially for edit/create/update.

dawehner’s picture

FileSize
44.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,740 pass(es). View
1.99 KB

Why does the sticky/promoted etc. access check also check status access?

I think checking both makes sense here, because the corresponding code action changes both:

  public function execute($entity = NULL) {
    $entity->status = NODE_PUBLISHED;
    $entity->sticky = NODE_STICKY;
    $entity->save();

Fixed the nits.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Afaik all feedback from @catch got adressed

catch’s picture

Status: Reviewed & tested by the community » Needs review

The action itself looks wrong to me. If you have access to set something sticky/promoted (or unsticky/unpromote) that should work without requiring extra permissions that are unrelated. The action is trying to be 'helpful' by also publishing but that's causing inconsistent behaviour. Not introduced here we don't unpublish if un-promoting or un-stickying content, so that action would have a different access check despite being for exactly the same field. It's minor in relation to the rest of the patch but people will use these as examples for their own code so I think it's worth trying to get right here.

On the entity vs. field access, those docs are OK I think at least given the limitations of the current actions API.

dawehner’s picture

FileSize
44.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,822 pass(es). View
1.88 KB

Alright, let's update them. It's right, the intention of the user is to just promote them, so let's adapt access for it.

Wim Leers’s picture

Status: Needs review » Needs work

#125: great catch!

#126 almost looks good… but I read #125 as saying that PromoteNode should no longer do $entity->setPublished(TRUE);, and StickyNode should no longer $entity->status = NODE_PUBLISHED;.

dawehner’s picture

#126 almost looks good… but I read #125 as saying that PromoteNode should no longer do $entity->setPublished(TRUE);, and StickyNode should no longer $entity->status = NODE_PUBLISHED;.

I do not read it like that, seems like an out of scope change for me personally.
For me the access should be coupled to the concept of the action not to the congret implementation.

Wim Leers’s picture

In my reading, that's why catch said The action itself looks wrong to me […] Not introduced here […] It's minor in relation to the rest of the patch but people will use these as examples for their own code so I think it's worth trying to get right here.

dawehner’s picture

Status: Needs work » Needs review
FileSize
44.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2172017-130.patch. Unable to apply patch. See the log in the details link for more information. View
979 bytes

Alright, asked @catch on IRC. He meant it to change it.

On top of that I did a bit of research:

  • It used to be like the new patch in D7
  • It used to be like that before it got converted to plugins in D8
  • https://www.drupal.org/node/1846172#comment-7483604 asked exactly that question, tim answered it, but if you look at the underlying hunks you could
    get the impress that tim did not changed the actual code or misread it

Based upon that, let's change it.

Status: Needs review » Needs work

The last submitted patch, 130: 2172017-130.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
43.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,831 pass(es). View

Just a rebase

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Looks perfect now!

  • catch committed a042df2 on 8.0.x
    Issue #2172017 by dawehner, larowlan, MegaChriz, kim.pepper, Désiré, Sam...
catch’s picture

Status: Reviewed & tested by the community » Fixed

OK. I still don't feel 100% comfortable with this - simply because I feel like we ought to be able to generically check entity access and not have to individually implement that for every actions access check. Easy for a module to add an action and get this wrong.

However that's more of a complaint about the actions API than this patch, so committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

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

TR’s picture

Issue tags: +Needs change record