To reproduce

create a user, add some content, make sure you have a node access module
call $account->delete() via the API anonymously (e.g. in drush) - delete will fail.

CommentFileSizeAuthor
#14 2844293-14.patch1.43 KBswentel
#4 2844293-4-fail.patch891 bytesswentel
#4 2844293-4.patch1.37 KBswentel
#11 2844293-11.patch1.46 KBswentel
#11 2844293-11-interdiff.txt1.08 KBswentel

Comments

swentel created an issue. See original summary.

swentel’s picture

swentel commented

So this might be an explicit drush issue as I'm not able to create a failing test for now.

swentel’s picture

swentel commented
Status: Active » Closed (works as designed)

Ok, entirely my fault

swentel’s picture

swentel commented
Title: Manually calling $account->delete() will throw an exception if the user has nodes in an anonymous environment » Manually calling $account->delete() will throw an exception if the user has nodes in an anonymous environment with node access enabled
Status: Closed (works as designed) » Active
StatusFileSize
new 2844293-4-fail.patch891 bytes
new 2844293-4.patch1.37 KB

Actually I could finally reproduce this with core. The culprit is the entityQuery in node_user_predelete() which shouldn't do an access check. It just needs to get the nodes it wants to delete.

Quickly hacked a fail patch which mimics my 'anonymous' environment. Adding this check makes it all work for me nicely.
I'm guessing users who want to cancel where suddenly they don't have access anymore (e.g. auto unpublish at spam for instance) for some reason to a node will get this failure too.

swentel’s picture

swentel commented
Status: Active » Needs review
swentel’s picture

swentel commented
Issue summary: View changes
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Ouch of course, yet another entity query that shouldn't do access checks.

We really should go through non-test entity queries and evaluate if they should do access checking or not.

The last submitted patch, 4: 2844293-4-fail.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
+++ b/core/modules/user/src/Tests/UserCancelTest.php
@@ -594,4 +594,17 @@ function testMassUserCancelByAdmin() {
+    $this->container->get('module_installer')->install(['node_access_test']);

#2066993: Use magic methods to sync container property to \Drupal::getContainer in functional tests

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

swentel’s picture

swentel commented
Status: Needs work » Needs review
StatusFileSize
new 2844293-11-interdiff.txt1.08 KB
new 2844293-11.patch1.46 KB
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Version: 8.4.x-dev » 8.2.x-dev
Status: Needs review » Reviewed & tested by the community

Looks good to me.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Changed to 8.2.x as that is AFAIK still the target for bugfixes that aren't causing any changes.

swentel’s picture

swentel commented
StatusFileSize
new 2844293-14.patch1.43 KB

Changed something small in the comment just before the call of node_access_rebuild() (to lazy for interdiff)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2844293-14.patch, failed testing.

swentel’s picture

swentel commented
Status: Needs work » Reviewed & tested by the community
swentel’s picture

swentel commented

so node_user_cancel has this.

function node_user_cancel($edit, $account, $method) {
  switch ($method) {
    case 'user_cancel_block_unpublish':
      // Unpublish nodes (current revisions).
      $nids = \Drupal::entityQuery('node')
        ->condition('uid', $account->id())
        ->execute();

I suspect we'll have the same problem here in case of cancelling, but maybe for a different issue ?

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • catch committed 2740e70 on 8.4.x 
    Issue #2844293 by swentel: Manually calling $account->delete() will...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Opened #2850423: node_user_cancel() shouldn't do access checks.

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

  • catch committed e91c57e on 8.3.x 
    Issue #2844293 by swentel: Manually calling $account->delete() will...

Status: Fixed » Closed (fixed)

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