See parent issue #3200985: [meta] Fix undesirable access checking on entity query usages for context.

When cleaning up data consequent on field, entity type or module removal, we should not care whether the curreny user can access the data. We should trust that access was properly checked for the field, entity type or module removal itself.

Fixes needed:
- core/modules/shortcut/src/Entity/ShortcutSet.php preDelete
- core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php all in file, we are uninstalling which means deleting everything
- core/modules/node/node.module case 'user_cancel_block_unpublish' we want to unpublish everything the user owns
- core/modules/user/user.api.php hook_user_cancel
- core/modules/comment/comment.module comment_field_config_delete and comment_entity_predelete

Issue fork drupal-3201714

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jonathanshaw created an issue. See original summary.

jonathanshaw opened merge request !385

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented

I cannot find any test coverage for PrepareModulesEntityUninstallForm or the ContentUninstallValidator that calls it. This one could be affected by node_access, but I'm not sure it really matters. To suffer from the bug you'd need to
(a) have a node acess module installed
(b) not be using drush to manage module uninstallation
(c) want to uninstall node
(d) be someone with permission to uninstall node but not to view all your nodes
(e) fail to realise that uninstalling node will remove all your site's nodes

I suggest not requiring test coverage for this fix.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Doesn't drush run with the anonymous user though? So if the anonymous user doesn't have access to view nodes it would be affected?

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented

Drush doesn't warn you about content that might be deleted when uninstalling a module, AFAIK. And if it does, it wouldn't be using a form or class or batch job like PrepareModulesEntityUninstallForm to do it (I'd expect).

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman as a volunteer commented

Drush doesn't warn you about content that might be deleted when uninstalling a module, AFAIK. And if it does, it wouldn't be using a form or class or batch job like PrepareModulesEntityUninstallForm to do it (I'd expect).

Drush doesn’t have special messages or special code here. We call Drupal's validation and then uninstall methods. Patch makes sense to me.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented

Looking at the code, Drush calls ModuleHandler::uninstall which ultimately will call ContentUninstallValidator. However, this will not trigger PrepareModulesEntityUninstallForm, it simply provides an optional link to it:

ContentUninstallValidator:

      if ($module == $entity_type->getProvider() && $entity_type instanceof ContentEntityTypeInterface && $this->entityTypeManager->getStorage($entity_type->id())->hasData()) {
        $reasons[] = $this->t('There is content for the entity type: @entity_type. <a href=":url">Remove @entity_type_plural</a>.', [
          '@entity_type' => $entity_type->getLabel(),
          '@entity_type_plural' => $entity_type->getPluralLabel(),
          ':url' => Url::fromRoute('system.prepare_modules_entity_uninstall', ['entity_type_id' => $entity_type->id()])->toString(),
        ]);
      }

There are therefore basically 2 possible impacts of a bug in PrepareModulesEntityUninstallForm that someone using the form to remove content might find:
- (1) not all content being removed, despite apparent success of the batch job, so module will persist in being uninstallable
- (2) being given an underestimate of the number of content items there are to remove

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented
Status: Active » Needs review

The test-only commit failed, and so now I've restored the fix. This is ready for RTBC.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented
Issue summary: View changes

Fixed the hook_user_cancel docs in this issue too.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work

One style nit but otherwise this looks good to go.

ravi.shankar made their first commit to this issue’s fork.

ravi.shankar’s picture

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented
Status: Needs work » Needs review

I have changed the variable name as suggested, please review.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs work

The content entity impact on uninstalling is tested in \Drupal\Tests\system\Functional\Module\PrepareUninstallTest

I also left a review on the merge request.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented
Status: Needs work » Needs review

The content entity impact on uninstalling is tested in \Drupal\Tests\system\Functional\Module\PrepareUninstallTest

Thanks a lot for finding that @alexpott, I had missed it and it made things easy.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

Seems like the latter part of the test should need updating:

    // All 10 nodes should be listed.
    foreach ($this->nodes as $node) {
      $this->assertText($node->label());
    }

Only 5 should be listed here now, I think - let's see what the bot says.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented

#16 was right.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work

The test fix wasn't quite right.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented
Status: Needs work » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

All review points were addressed, the test is now quite comprehensive I think.

  • catch committed 37a5b1a on 9.2.x 
    Issue #3201714 by jonathanshaw, ravi.shankar, longwave, catch, alexpott...

  • catch committed ad0084b on 9.1.x 
    Issue #3201714 by jonathanshaw, ravi.shankar, longwave, catch, alexpott...
catch’s picture

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

Committed 37a5b1a and pushed to 9.2.x. Thanks! Cherry-picked to 9.1.x too.

Status: Fixed » Closed (fixed)

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