Notification emails should not send to blocked users

Thanks @alloyroger for the patch, i can confirm this is an issue.

Hello,

Thanks for the patch.

Patch #2 failed to apply. Rerolled the patch.
Please review
Thannks

The change here is a good one. However, we need tests (as the summary already points out) before this can be committed. Setting to 'Needs work' and tagging accordingly.

Meanwhile, upgrading this to a major bug. I pinged the security team about this issue. It's basically an access bypass security hole. 😢 Blocked users are never supposed to have access to any site content that wouldn't be visible to an anonymous user. And this bug violates that premise. But since this issue has already been open in the public queue for ~2 years, it's kinda pointless to unpublish it now and work on fixing it "in secret" and putting out an official security announcement about it. But it's at least a major bug, not a normal feature request. 😉 Escalating as such.

I'm going to take a quick stab at some test coverage. Stay tuned.

Thanks!
-Derek

Ran into a bit of a snag since Kernel tests do not create any User entities automatically. So once we enable the 'author' setting in the notification in this test, it would die inside src/Notification.php here:

      if ($notification->author and ($entity instanceof EntityOwnerInterface)) {
        if ($entity->getOwner()->isActive()) {
          $data['to'][] = $entity->getOwner()->getEmail();
        }
      }

$entity->getOwner() always returns NULL if there are no User entities to load. 😉 So we've got to use Drupal\Tests\user\Traits\UserCreationTrait to actually create a User for UID 1.

Anyway, with that magic uncovered, here's pretty straight-forward test coverage to prove the bug, and that the attached patch fixes it. This version only touches the first of the two new code paths, the author. No coverage yet for the $role_user side of life. But that'd be easy enough to add if we need it.

The test-only patch is the interdiff.

Thoughts?

Thanks!
-Derek

Hrm, test-only results are a little weird. That's because of #3226605: TokenNotificationsTest should not extend NotificationsTest

But, other than running everything in NotificationsTest twice, this seems sufficient to fix this bug. Unless the maintainers want me to add test coverage for the 'role user' stuff (which has no actual test coverage yet) regarding this bug. Is that necessary?

Cheers,
-Derek

- Failing test-only patch
- Working actual patch
- "Wonky" test results explained

For me this is RTBC, although I think a test for 'role_user' would be welcomed (basically a c/p of the current test), but that's up for the maintainer to decide IMHO. Could even be a follow-up in my eyes.

Thanks!