Problem/Motivation

The status report now contains a check for duplicate email address and reports them with the heading "Conflicting user emails". However, accounts may be created without an email, i.e. with a NULL email, and these accounts incorrectly trigger the warning. Further, the drupal.org page linked to by the message contains a SQL query that does not return the NULL email messages, confusing the administrator as to which accounts are causing the warning.

Steps to reproduce

Create two different user accounts, both without an email address. View the status report and note the warning about conflicting user emails.

Proposed resolution

Exclude NULL email messages from the query that triggers the warning. In user.install, userRequirements():

  $query = \Drupal::database()->select('users_field_data');
  $query->addExpression('LOWER(mail)', 'lower_mail');
  $query->groupBy('lower_mail');
  $query->having('COUNT(uid) > :matches', [':matches' => 1]);
  $conflicts = $query->countQuery()->execute()->fetchField();

add

  $query->isNotNull('mail');
CommentFileSizeAuthor
#13 3488835-drupal--false-report.diff3.85 KBrobloach

Issue fork drupal-3488835

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:

Comments

danchadwick created an issue. See original summary.

atul_ghate’s picture

Assigned: Unassigned » atul_ghate

I will work on this issue.

quietone’s picture

Assigned: atul_ghate » Unassigned

@atul_ghate, thanks for working on this issue. I am going to unassign because for Drupal core, it is preferred that contributors add a comment that they are working on an issue instead of assigning it to themselves. See Assigning ownership of a Drupal core issue.

zaryab_drupal made their first commit to this issue’s fork.

danchadwick’s picture

Issue summary: View changes
atul_ghate’s picture

Status: Active » Needs work

mcdruid made their first commit to this issue’s fork.

mcdruid’s picture

I've added the tests that the Security Team used when working on SA-CORE-2024-004; we'd typically wait a few weeks before committing tests that accompany a security fix, but there's no need to do so in this case.

We should add to these tests to verify the behaviour of user_requirements() when there are multiple users with a blank email.

We might want to account for the fact that could mean a couple of different things e.g. a truly NULL field in the db, or an empty string.

Plus we need to check that all 3 of core's db drivers behave themselves.

mcdruid’s picture

Added a new test that verifies user_requirements() doesn't incorrectly flag a couple of user accounts that both have their mail set to an empty string.

This passes locally for me with mysql.

mcdruid’s picture

Status: Needs work » Needs review
mcdruid’s picture

Tests pass with mysql, sqlite, and postgres (including the new one).

Anyone happy to RTBC this on that basis?

robloach’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new3.85 KB

Tested this out, and worked flawlessly. Was confused when running the related SQL it didn't output any results. The MR fixes the false positive.

The diff applies to Drupal 10.3 as well, so just a rebase/cherry-pick should work fine, but here's the same patch if needed for Drupal 10.3.x

wylbur’s picture

Tested this on websites reporting the error, but which lacked accounts that differed by upper/lower case characters. Applying the patch resolved the error message on the status page.

My testing was on with mysql databases. Will see if I can setup tests on the others.

larowlan credited catch.

larowlan credited cilefen.

larowlan credited greggles.

larowlan credited longwave.

larowlan credited poker10.

larowlan’s picture

Issue credits from the private issue

  • larowlan committed ed1baed5 on 10.4.x
    Issue #3488835 by mcdruid, atul_ghate, benjifisher, catch, cilefen,...

  • larowlan committed c4dfbbe2 on 10.5.x
    Issue #3488835 by mcdruid, atul_ghate, benjifisher, catch, cilefen,...

  • larowlan committed 2a82a293 on 11.1.x
    Issue #3488835 by mcdruid, atul_ghate, benjifisher, catch, cilefen,...

  • larowlan committed 9ed8a79f on 11.x
    Issue #3488835 by mcdruid, atul_ghate, benjifisher, catch, cilefen,...
larowlan’s picture

Version: 11.x-dev » 10.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed to 11.x and backported to 10.4.x, 10.5.x and 11.1.x

10.3.x and 11.0.x are security only.

Status: Fixed » Closed (fixed)

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

w01f’s picture

This says committed for 11.1.x, but I have several sites that were updated to the most recent 11.1.6 and they still show that error. I've checked and there are no valid cases of duplicate emails.

poker10’s picture

@w01f have you tried running the SQL query mentioned here: https://www.drupal.org/node/3486109 , to check, what is causing the warning message?