When a user logs in to "new" JIRA, we also can't easily tell if they have an existing account link based on the presence of an accountId.

(We could only do this -- possibly -- by calling the user endpoint with every key we know about, which may take an arbitrarily long time.)

So the flow needs to be something like this:

• Add a new flag to the provider, like "ProviderHasAnyOldAccountKeys".
  • Set this flag on Phabricator upgrade if any external JIRA account with a key exists.
  • Set this flag on account link if we link with the key field. This isn't trivial because account linking is abstracted from ID extraction.
• If we attempt a link with "ProviderHasOldAccountKeys" set and encounter both a key field and an accountId field, upgrade the account transparently. This code can only be tested on a narrow range of JIRA versions.
  • After upgrading, we could attempt to clear the "ProviderHasOldKeys" key.
• If we attempt a link with "ProviderHasOldAccountKeys" set and encounter only an accountId field, we may not continue. Emit an error message informing the user about the upgrade and pointing them toward administrative resolution.
• Provide a script which does the offline iterate-lookup-migrate via rest/api/(2|3)/user using key. If it succeeds for all accounts, it clears the "ProviderHasOldAccountKeys" flag. This code can only be tested by running an old version of JIRA, then upgrading to a new version of JIRA.

This is such a ridiculous mess.

If you're feeling ambitious, here's a likely patch:

diff --git a/src/applications/auth/adapter/PhutilJIRAAuthAdapter.php b/src/applications/auth/adapter/PhutilJIRAAuthAdapter.php
index a045065590..e4cf3207b5 100644
--- a/src/applications/auth/adapter/PhutilJIRAAuthAdapter.php
+++ b/src/applications/auth/adapter/PhutilJIRAAuthAdapter.php
@@ -27,7 +27,18 @@ final class PhutilJIRAAuthAdapter extends PhutilOAuth1AuthAdapter {
     // side effect by Auth providers.
     $this->getHandshakeData();
 
-    return idx($this->getUserInfo(), 'key');
+    $info = $this->getUserInfo();
+
+    // See T13493. JIRA has changed from providing a "key" to providing an
+    // "accountId". These fields have different values and are tricky to
+    // migrate.
+
+    $account_id = idx($info, 'accountId');
+    if ($account_id !== null) {
+      return sprintf('accountId(%s)', $account_id);
+    }
+
+    return idx($info, 'key');
   }
 
   public function getAccountName() {

After applying this patch, you can either:

• instruct all users to unlink and relink their JIRA accounts in Settings → External Accounts. Note that the provider must support linking and unlinking (in Auth) for this option to be available. Users who use JIRA to log in and do not currently have a session can use "Forgot your password?" or "Send a login link to your email address." to gain access to their accounts;
• and/or run the script below.

Users who try to login with JIRA normally after you apply the patch (without linking/unlinking) will (by default) create new accounts instead of logging into their old accounts, since it will look like they are logging in with a totally novel JIRA account which does not exist elsewhere in the system. There is no way to merge accounts. The flow is very obvious that it's creating a new account and will require them to pick a new username, but 95% of users are likely to complete it anyway. You could temporarily disable registration in Auth to avoid this.

This script will try to migrate existing accounts from key to accountId. It may or may not be successful since it's hard for us to guarantee we have credentials we can use to do this:

<?php

require_once 'scripts/init/init-script.php';

$viewer = PhabricatorUser::getOmnipotentUser();

$jira_type = id(new PhutilJIRAAuthAdapter())
  ->getAdapterType();

$external_accounts = id(new PhabricatorExternalAccountQuery())
  ->setViewer($viewer)
  ->withAccountTypes(
    array(
      $jira_type,
    ));

$iterator = new PhabricatorQueryIterator($external_accounts);

$warning_count = 0;
$error_count = 0;
$upgrade_count = 0;
$queue = array();

foreach ($iterator as $account) {
  $jira_display = $account->getRealName();
  $user_phid = $account->getUserPHID();

  // If this account has credentials, add them to the list of possible valid
  // credentials we may be able to use to perform lookups. We'll use the
  // credentials of any account we have available.

  $account_phid = $account->getPHID();

  $oauth1_token = $account->getProperty('oauth1.token');
  $oauth1_secret = $account->getProperty('oauth1.token.secret');

  if (($oauth1_token !== null) && ($oauth1_secret !== null)) {
    $credentials[$account_phid] = array(
      'token' => $oauth1_token,
      'secret' => $oauth1_secret,
    );
  }

  // If an account isn't linked to a Phabricator user, it's fine to just
  // leave it orphaned forever.
  if ($user_phid === null) {
    echo tsprintf(
      "[%s] %s\n",
      pht('SKIP'),
      pht(
        'JIRA account "%s" is not linked to anything, ignoring.',
        $jira_display));
    continue;
  }

  $user = id(new PhabricatorPeopleQuery())
    ->setViewer($viewer)
    ->withPHIDs(array($user_phid))
    ->executeOne();
  if (!$user) {
    $warning_count++;
    echo tsprintf(
      "[%s] %s\n",
      pht('WARN'),
      pht(
        'JIRA account "%s" is linked to invalid user "%s".',
        $jira_display,
        $user_phid));
    continue;
  }

  $phabricator_display = $user->getMonogram();

  $account_id = $account->getAccountID();
  if (preg_match('/^accountId\(.*\)\z/', $account_id)) {
    echo tsprintf(
      "[%s] %s\n",
      pht('OK'),
      pht(
        'JIRA account "%s" is already up to date and '.
        'properly linked (to "%s").',
        $jira_display,
        $phabricator_display));
   continue;
  }

  $queue[$account_phid] = $account;
}

shuffle($credentials);

foreach ($queue as $account_phid => $account) {
  $jira_display = $account->getRealName();

  echo tsprintf(
    "[%s] %s\n",
    pht('MIGRATE'),
    pht(
      'Attempting to migrate account "%s" to a modern "accountId".',
      $jira_display));

  // If we have credentials for this account, try them first.

  $credentials = array_select_keys(
    $credentials,
    array($account_phid)) + $credentials;

  $provider_config = $account->getProviderConfig();
  $provider = $provider_config->getProvider();
  $adapter = $provider->getAdapter();

  if (!$credentials) {
    throw new Exception(
      pht(
        'No JIRA OAuth credentials are available on any account. Link or '.
        'refresh at least one account, then try again.'));
  }

  $attempts = 0;
  $updated_id = null;
  foreach ($credentials as $credential_set) {
    $oauth1_token = $credential_set['token'];
    $oauth1_secret = $credential_set['secret'];

    $adapter = id(clone $adapter)
      ->setToken($oauth1_token)
      ->setTokenSecret($oauth1_secret);

    $params = array(
      'key' => $account_id,
    );

    $future = $adapter->newJIRAFuture('rest/api/3/user', 'GET', $params);

    $attempts++;

    try {
      $result = $future->resolveJSON();

      if (idx($result, 'accountId') === null) {
        throw new Exception(
          pht(
            'Call to JIRA returned no "accountId". Your version of JIRA '.
            'is probably too old to support this migration.'));
      }

      $updated_id = sprintf(
        'accountId(%s)',
        $result['accountId']);
      break;
    } catch (Exception $ex) {
      // Ignore any exceptions, we'll just try again.
    }

    if ($attempts >= 3) {
      break;
    }
  }

  if ($updated_id === null) {
    echo tsprintf(
      "[%s] %s\n",
      pht('FAIL'),
      pht(
        'Unable to find "accountId" for key "%s" after %s tries.',
        $account_id,
        $attempts));
    $error_count++;
    continue;
  }

  $duplicate = id(new PhabricatorExternalAccountQuery())
    ->setViewer($viewer)
    ->withAccountTypes(
      array(
        $jira_type,
      ))
    ->withAccountIDs(
      array(
        $updated_id,
      ))
    ->executeOne();
  if ($duplicate) {
    echo tsprintf(
      "[%s] %s\n",
      pht('DUPLICATE'),
      pht(
        'Old account key "%s" is a duplicate of existing external '.
        'account "%s", unable to upgrade.',
        $account_id,
        $updated_id));
    $error_count++;
    continue;
  }

  $table = new PhabricatorExternalAccount();
  $conn = $table->establishConnection('w');

  queryfx(
    $conn,
    'UPDATE %R SET accountID = %s WHERE id = %d',
    $table,
    $updated_id,
    $account->getID());

  echo tsprintf(
    "[%s] %s\n",
    pht('UPGRADED'),
    pht(
      'Upgraded account key "%s" to accountID "%s".',
      $account_id,
      $updated_id));

  $upgrade_count++;
}

echo tsprintf(
  "%s\n",
  pht(
    "Done, with %s upgrade(s), %s warning(s), and %s error(s).",
    new PhutilNumber($upgrade_count),
    new PhutilNumber($warning_count),
    new PhutilNumber($error_count)));

To run it, put it in phabricator/ and run it as:

phabricator/ $ php -f jira-migrate-account-id.php

If this works (or doesn't work) for you, let me know either here or on Discourse.

If you have a way to generate a master mapping or just want to look at what's going on here, you can examine the external account link table like this:

`
mysql> SELECT u.username PhabricatorUser, x.accountID JiraUser FROM user_externalaccount x LEFT JOIN user u ON u.phid = x.userPHID WHERE x.accountType = 'jira';
+-----------------+-------------------------------------+
| PhabricatorUser | JiraUser                            |
+-----------------+-------------------------------------+
| NULL            | 5e4d8d1b052b790c9750c4d2            |
| epriestley      | accountId(5e4d8d1b052b790c9750c4d2) |
+-----------------+-------------------------------------+
2 rows in set (0.00 sec)

Here's what an individual user_externalaccount record looks like:

mysql> SELECT * FROM user_externalaccount WHERE id = 40\G
*************************** 1. row ***************************
                id: 40
              phid: PHID-XUSR-bon6ybuzyqz3hzmwprki
          userPHID: PHID-USER-icyixzkx3f4ttv67avbn
       accountType: jira
     accountDomain: jiraphi1125
     accountSecret: jnpl5ziod22a5ixipqhtlnvjq7dk6suc
         accountID: accountId(5e4d8d1b052b790c9750c4d2)
       displayName: NULL
       dateCreated: 1582151064
      dateModified: 1582151076
          username: NULL
          realName: ASDF 1234
             email: NULL
     emailVerified: 0
        accountURI: NULL
  profileImagePHID: PHID-FILE-tyh2pbikrhoplb5ttlro
        properties: {"oauth1.token":"6uhrsS5Uw0QlF12L5mMaknPtcm7glR0Y","oauth1.token.secret":"3ooP7DDay1Uh8kokP7HcAPue4HfR9tBa","registrationKey":"c5b7123263fd092f8270c7ffc98562544818125b"}
providerConfigPHID: PHID-AUTH-3za22myz7kqpltaezotp
1 row in set (0.01 sec)

For old accounts, the accountID in this record will look like evan or evan@mycompany.com (this is an old key value).

For new accounts, the accountID in this record will look like accountId(a1b2c3d4).

Updating this cell from an old key to a new accountId is sufficient to fix account links, so if you have a mapping from key values to accountId values you can execute queries like this to effect a fix:

mysql> UPDATE phabricator_user.user_externalaccount SET accountID = 'accountId(a1b2c3d4)' WHERE accountID = 'evan@mycompany.com' AND accountType = 'jira';

Note that the MySQL column is called accountID (capital D) but the JIRA field is called accountId (lowercase d).

The script above will try to build and apply such a mapping automatically.

I think the patch above is a piece of the solution here, but makes behavior worse for some installs: installs with a version of JIRA which returns both key and accountId will have worse behavior under the patch than without it (since it will break all the existing account links immediately). It also doesn't smoothly migrate these installs, even though it's theoretically easy/desirable to do that.

One possible way to get away from this is to let each account have multiple unique identifiers instead of a single accountID. Then, we can operate on both key and accountId. I believe Google auth may eventually have a similar issue because the account identifiers at time of implementation were not GUIDs, so this isn't necessarily completely in vain.

This touches somewhat on T6703.

I think a safe first step here is:

• Create an externalidentifier table with <providerConfigPHID, externalAccountPHID, identifier>, with a unique key on <providerConfigPHID, identifier>.
• Copy all accountID values to this table.
• Update all the query/read/write pathways to use this table instead of the column.
• (Some day, remove the accountID column.)

Then, intermediate versions of JIRA can return two identifiers (key and accountId) and we can match on either one. This will let installs that run an intermediate version of JIRA for long enough that every user does an OAuth handshake to upgrade completely transparently. It doesn't help much if you skip the intermediate versions, but it's never worse than the status quo for any install and dramatically better for some reasonable upgrade pathways.

These callers use accountId:

• An old piece of code (loadExternalUserActors()) which tries to CC external users who created objects by sending email. This never actually sends mail after T12237 / D17329, but I left the code around in case we wanted to revisit this. This can just be removed.
• The Asana publisher integration. This can be moved to some getAsanaAPIUserAccountID(PhabircatorExternalAccount $account) method which expects (and returns) exactly one accountID.
• PhabricatorAuthAccountView, which is the janky mess of a UI visible in Settings → External Accounts. This shouldn't really show account IDs anyway.
• Passwords use the user PHID as an account ID. This can sort out whichever way is easiest.
• PhabricatorAuthProvider->loadOrCreateAccount() does most of the heavy lifting for registration and linking.
  • The OAuth1 and OAuth2 providers have very similar logic for invoking it.
  • The LDAP provider invokes it.
• bin/auth ldap has a trivial usage.
• All auth adapters implement getAccountID().

This doesn't look too terribly bad.

This change sequence is almost ready to remove readers and writers to accountID, but there's still a unique <accountType, accountDomain, accountID> key on the table. Removing accountID writers completely will mean that the second user to link an account of a particular type (say, an Asana account) will run into a unique key error (since they'll write a second "Asana" account with the same empty accountID as the first "Asana" account).

On its own, D21014 introduces this problem for "Password" auth, although that would be trivial to overcome narrowly by writing a random value instead of the empty string.

The desired state is to remove this key, although not necessarily to remove the "you can only be linked to one account per provider" constraint (see T2549, previously).

Today, some callers use withAccountTypes() or withAccountDomains() constraints on ExternalAccountQuery objects. It's not clear if any of these are "valid".

If none are valid, we could remove the key entirely and possibly the columns.

If some are valid, we can shorten the key to <accountType, accountDomain> or <accountType> (and possibly remove accountDomain).

The callers are:

PhabricatorAuthLoginController: When users link or refresh an account link, we look for another account of the same type. This can and should be changed to look for another account of the same provider.

PhabricatorAuthManagementRefreshWorkflow: This is bin/auth refresh, which isn't really in use. It can be removed for the moment; a provider selector would be better in the long term.

AsanaBridge: The Asana bridge queries for Asana accounts, but can query for accounts on Asana providers instead.

JIRABridge: Likewise, the JIRA bridge queries for JIRA accounts, but can query for accounts on JIRA providers instead.

AsanaConfigOptions: Same as AsanaBridge.

AsanaFeedWorker: Same as AsanaBridge.

JIRAFeedWorker: Same as JIRABridge.

These all seem straightforward to fix, and support complete removal of the key now and eventual removal of the accountType and accountDomain columns.

I stumbled across what appears to be a very mild security issue in JIRA that impacts this flow. I've reported it to Atlassian's bug bounty program here (this link may or may not be visible to anyone else):

https://bugcrowd.com/submissions/0d4b6aabf05c26bba28a610863def3103293bbde4318670f1bcbfb5587e38f1a

This issue breaks the OAuth flow in a perplexing way (that is, if you hit the issue as a user, it probably won't be obvious to you what's going on) in Chrome, but fixing it will disclose details about the issue, so I'm going to give them a chance to review it and respond before upstreaming a fix. Only Chrome is impacted, so you can use another browser as a workaround if the JIRA OAuth flow "does something weird" for you in Chrome.

I landed everything so far to master. The new behavior in master should be:

• New JIRA account interactions (that is, linking or working with a JIRA account which Phabricator has not previously interacted with) always work against old ("key" only), intermediate (both "key" + "accountId"), or new ("accountId" only) versions of JIRA.
• Unlinking and relinking will always work, for any version of JIRA you originally linked the account with and any version of JIRA you are now running.
• JIRA account interactions gracefully upgrade from "old" JIRA to "new" JIRA on their own if you run an intermediate version of JIRA in between and authenticate against it at least once, so Phabricator can learn the "key"/"accountId" mapping.

So this is a large subset of cases which should "just work" now without requiring anyone to do anything.

A separate set of cases still do not "just work", where:

• you previously ran "old" JIRA, or ran "intermediate" JIRA on an older Phabricator version which does not include D21023 (i.e., versions before now); and
• you upgrade directly to "new" JIRA (or upgrade to "intermediate" JIRA but never refresh the account link, then upgrade to "new" JIRA).

In these cases, Phabricator is either missing the capability to learn the mapping (provided by D21023) or the opportunity to learn the mapping (provided by refreshing an account link against an "intermediate" version of JIRA).

Users can still fix these cases by manually unlinking/relinking the account link, but this requires that link/unlink permissions be enabled and is likely error-prone for larger organizations.

Due to schema changes to support this behavior, the script above no longer works. (If you ran it before these changes you're fine, but it won't do anything after these changes.)

I'm currently planning to improve the administrative tools for managing account links and then see how much of an issue this is in practice and go from there.

See private correspondence ("Re: Contributing / Jira Oauth Patch"), which suggests the call to rest/auth/1/session should be (and may urgently need to be) replaced with a call to rest/api/2/myself. See also https://developer.atlassian.com/cloud/jira/platform/deprecation-notice-basic-auth-and-cookie-based-auth/#which-apis-and-methods-will-be-restricted-.

At time of writing, calls to rest/auth/1/session return a result like this:

array(2) {
  ["self"]=>
  string(71) "https://jira-test-701.atlassian.net/rest/api/latest/user?username=admin"
  ["name"]=>
  string(5) "admin"
}

The self URI does not actually seem to function:

[HTTP/404] Not Found
{"errorMessages":["The 'accountId' query parameter needs to be provided"],"errors":{}}

At time of writing, calls to rest/api/3/myself now return this:

array(10) {
  ["self"]=>
  string(86) "https://jira-test-701.atlassian.net/rest/api/3/user?accountId=5ea49be51067100c19902032"
  ["accountId"]=>
  string(24) "5ea49be51067100c19902032"
  ["avatarUrls"]=>
  array(4) {
    ["48x48"]=>
    string(180) "https://secure.gravatar.com/avatar/6ff92b8e3edd6a080c581a1ee468024b?d=https%3A%2F%2Favatar-management--avatars.us-west-2.prod.public.atl-paas.net%2Finitials%2FJT-0.png&size=48&s=48"
    ["24x24"]=>
    string(180) "https://secure.gravatar.com/avatar/6ff92b8e3edd6a080c581a1ee468024b?d=https%3A%2F%2Favatar-management--avatars.us-west-2.prod.public.atl-paas.net%2Finitials%2FJT-0.png&size=24&s=24"
    ["16x16"]=>
    string(180) "https://secure.gravatar.com/avatar/6ff92b8e3edd6a080c581a1ee468024b?d=https%3A%2F%2Favatar-management--avatars.us-west-2.prod.public.atl-paas.net%2Finitials%2FJT-0.png&size=16&s=16"
    ["32x32"]=>
    string(180) "https://secure.gravatar.com/avatar/6ff92b8e3edd6a080c581a1ee468024b?d=https%3A%2F%2Favatar-management--avatars.us-west-2.prod.public.atl-paas.net%2Finitials%2FJT-0.png&size=32&s=32"
  }
  ["displayName"]=>
  string(11) "Jira Tester"
  ["active"]=>
  bool(true)
  ["timeZone"]=>
  string(19) "America/Los_Angeles"
  ["locale"]=>
  string(5) "en_US"
  ["groups"]=>
  array(2) {
    ["size"]=>
    int(1)
    ["items"]=>
    array(0) {
    }
  }
  ["applicationRoles"]=>
  array(2) {
    ["size"]=>
    int(1)
    ["items"]=>
    array(0) {
    }
  }
  ["expand"]=>
  string(23) "groups,applicationRoles"
}

For our purposes, this looks like it's "close enough" to the 2/user API return structure.