Page MenuHomePhabricator

Can't verify primary emails for existing users
Closed, ResolvedPublic

Description

I recently upgraded our Phabricator instance (https://reviews.llvm.org), and some existing users are getting "Primary Email Unverified" warning. However, users could not find the way/button to verify the primary email. Note that in the screenshot, there are "Verify" buttons for secondary emails but not the primary email.

2017-04-24.png (162×672 px, 26 KB)

prompt.png (243×334 px, 13 KB)

Version info:
o phabricator 644529ab98061ce6ed3acf14a96001398290ff3a (Mon, Apr 24)
o arcanist 40f7d68f75ea9e062f4018b00e1e012cdfb13c9d (Sat, Apr 22)
o phutil 7c7b02216f335adfdf48cf7ec28e21776af8a3c2 (Fri, Apr 14)

Event Timeline

I think some very old users got into a bugged state where their primary email is verified but the "is verified" flag was never set on their account. We saw one of these users on admin.phacility.com earlier, but I wasn't able to identify a way that a modern account could get into this state.

As a workaround, you can use bin/accountadmin to verify these users, although there's a possibility that a malicious user could trick you into verifying an address they do not actually own by asking you to verify their account with bin/accountadmin.

As another workaround, I think if they verify a secondary email, swap to it, then swap back, that will reset their account flag, but that's a big pain.

I'll write a script to identify the scope of this issue to start with. If there are recent accounts affected by it, we probably have a bug and should keep digging. If all affected accounts are, say, 18+ months old, we can probably just do a migration.

Here's a script which will identify affected users. Output is:

<username> <account created date> <primary email address> <primary email created> <primary email modified>
<?php

require_once 'scripts/__init_script__.php';

$table = new PhabricatorUser();
$invalid = array();
foreach (new LiskMigrationIterator($table) as $user) {
  // Ignore users who are verified.
  if ($user->getIsEmailVerified()) {
    continue;
  }

  // Ignore unverified users with unverified primary emails: this state is
  // correct.
  $primary = $user->loadPrimaryEmail();
  if (!$primary) {
    continue;
  }

  if (!$primary->getIsVerified()) {
    continue;
  }

  $invalid[] = array(
    'user' => $user,
    'email' => $primary,
    'epoch' => $primary->getDateCreated(),
  );
}

$invalid = isort($invalid, 'epoch');

if (!$invalid) {
  echo "No users in bad states.\n";
}

foreach ($invalid as $spec) {
  $username = $spec['user']->getUsername();
  $userdate = date('Y-m-d', $spec['user']->getDateCreated());

  $address = $spec['email']->getAddress();
  $created_date = date('Y-m-d', $spec['email']->getDateCreated());
  $modified_date = date('Y-m-d', $spec['email']->getDateModified());

  echo
    "{$username}\t{$userdate}\t".
    "{$address}\t{$created_date}\t{$modified_date}\n";
}

To run this, put it in phabricator/ and run it with:

phabricator/ $ php -f verified.php

We have 117 affected users out of 45,000 accounts, but no account or email address was created or modified in 2017 and no account was created later than June 2016. That's not completely conclusive but does sort of suggest that this might be something we've already fixed.

One thing I do see is that a lot of the primary emails have creation dates later than the account dates, so that might point at a pathway I missed when trying to figure out how this would happen.

Actually I think there's a real path to this which remains available:

  • Register an account.
  • Don't verify your email.
  • Add a second email.
  • Verify that.
  • Make that your primary.

It looks like that will fail to set the flag on your account.

This is consistent with your screenshot, which shows two email addresses. It's also consistent with the earlier report in T12528, where the user also had two email addresses.

I'll verify this locally.

This should now be resolved at HEAD:

  • After upgrading past D17785, users should no longer be able to get into this state.
  • The migration in D17786 should fix existing users who have ended up in this state.

Per above, bin/accountadmin should be able to fix these users immediately, until you have time to upgrade. verify.php, above, can make sure that users who claim they're affected are really affected so you could do this safely, if you want to be paranoid about it.

And thanks for the report! Let us know if you run into anything else..

Thank you for the quick response and fixes!