Safeguarding against UnblockUser::execute()'s method unblocking the anonymous user

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

At this time we would need a D10.1.x patch or MR for this issue.

Patch for 10.1.x.

Hiding files to avoid confusion.

Ran the tests locally without the fix and got

Failed asserting that exception of type "LogicException" is thrown.

And clearly it passes with the fix.

Changes look good. Not sure if a security review should happen first (question for committer)

Added a question to the MR. This also needs a change record to note that we are making User::isAnonymous() more strict, as other code may have copied the pattern in core and used isAnonymous() to detect new users, which will now stop working.

Added a Change Record as per #19.
Will have a look at the failing tests now.

This one is up for a review again, the change record is in place, although a draft that could use some eyes on.
The points in the MR have all been addressed.

See \Drupal\Core\Session\AccountInterface is being replaced now. Is this change going to be disruptive to anyone using that interface now?

The reason I replaced \Drupal\Core\Session\AccountInterface with \Drupal\user\UserInterface is because it was causing a breakage in that Unit test class due to the following change:

diff --git a/core/modules/user/src/UserAccessControlHandler.php b/core/modules/user/src/UserAccessControlHandler.php
index a231d5214d..46b97cfa2e 100644
--- a/core/modules/user/src/UserAccessControlHandler.php
+++ b/core/modules/user/src/UserAccessControlHandler.php
@@ -101,7 +101,7 @@ protected function checkFieldAccess($operation, FieldDefinitionInterface $field_
       case 'name':
         // Allow view access to anyone with access to the entity.
         // The username field is editable during the registration process.
-        if ($operation == 'view' || ($items && $items->getEntity()->isAnonymous())) {
+        if ($operation == 'view' || ($items && $items->getEntity()->isNew())) {
           return AccessResult::allowed()->cachePerPermissions();
         }
         // Allow edit access for the own user name if the permission is

That change is however crucial to this issue and also makes sense in that class since it is an access control handler specific to the user entity type. And that type has a isNew() method.

The Unit test class however that is testing that handler has been using so far a \Drupal\Core\Session\AccountInterface mock that does not have an isNew() method (which causes the breakage) but most importantly makes less sense to use for testing the access control handler for the user entity type instead of the \Drupal\user\UserInterface itself.

So, all in all, that change is not disruptive, but the Unit test needed an adjustment IMHO.

Thanks for the explanation. Will move it on and see what the committer thinks too.

There's a very small chance that this could break some existing code (for example using ::isAnonymous() as a proxy for ::isNew()) but that could would already be broken as this issue shows, and it might even fix some hidden bugs for people. So I think the change record here (which looks good) is enough.

Committed c704db9 and pushed to 10.1.x. Thanks!

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