Make external link/refresh use provider IDs, switch external account MFA to one-shot

Summary: Depends on D20113. Ref T6703. Continue moving toward a future where multiple copies of a given type of provider may exist. Switch MFA from session-MFA at the start to one-shot MFA at the actual link action. Add one-shot MFA to the unlink action. This theoretically prevents an attacker from unlinking an account while you're getting coffee, registering `alIce` which they control, adding a copy of your profile picture, and then trying to trick you into writing a private note with your personal secrets or something. Test Plan: Linked and unlinked accounts. Refreshed account. Unlinked, then registered a new account. Unlinked, then relinked to my old account. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T6703 Differential Revision: https://secure.phabricator.com/D20117
2024-11-25 08:12:40 +01:00 · 2019-02-06 17:12:12 -08:00 · 2019-02-06 17:12:12 -08:00 · d22495a820
commit d22495a820
parent e5ee656fff
8 changed files with 76 additions and 60 deletions
--- a/src/applications/auth/application/PhabricatorAuthApplication.php
+++ b/src/applications/auth/application/PhabricatorAuthApplication.php
@ -62,7 +62,7 @@ final class PhabricatorAuthApplication extends PhabricatorApplication {
        'validate/' => 'PhabricatorAuthValidateController',
        'finish/' => 'PhabricatorAuthFinishController',
        'unlink/(?P<id>\d+)/' => 'PhabricatorAuthUnlinkController',
-        '(?P<action>link|refresh)/(?P<pkey>[^/]+)/'
+        '(?P<action>link|refresh)/(?P<id>\d+)/'
          => 'PhabricatorAuthLinkController',
        'confirmlink/(?P<akey>[^/]+)/'
          => 'PhabricatorAuthConfirmLinkController',
--- a/src/applications/auth/controller/PhabricatorAuthConfirmLinkController.php
+++ b/src/applications/auth/controller/PhabricatorAuthConfirmLinkController.php
@ -20,7 +20,15 @@ final class PhabricatorAuthConfirmLinkController

    $panel_uri = '/settings/panel/external/';

-    if ($request->isFormPost()) {
+    if ($request->isFormOrHisecPost()) {
+      $workflow_key = sprintf(
+        'account.link(%s)',
+        $account->getPHID());
+
+      $hisec_token = id(new PhabricatorAuthSessionEngine())
+        ->setWorkflowKey($workflow_key)
+        ->requireHighSecurityToken($viewer, $request, $panel_uri);
+
      $account->setUserPHID($viewer->getPHID());
      $account->save();

@ -31,14 +39,7 @@ final class PhabricatorAuthConfirmLinkController
      return id(new AphrontRedirectResponse())->setURI($panel_uri);
    }

-    // TODO: Provide more information about the external account. Clicking
-    // through this form blindly is dangerous.
-
-    // TODO: If the user has password authentication, require them to retype
-    // their password here.
-
-    $dialog = id(new AphrontDialogView())
-      ->setUser($viewer)
+    $dialog = $this->newDialog()
      ->setTitle(pht('Confirm %s Account Link', $provider->getProviderName()))
      ->addCancelButton($panel_uri)
      ->addSubmitButton(pht('Confirm Account Link'));
--- a/src/applications/auth/controller/PhabricatorAuthController.php
+++ b/src/applications/auth/controller/PhabricatorAuthController.php
@ -213,19 +213,19 @@ abstract class PhabricatorAuthController extends PhabricatorController {
      return array($account, $provider, $response);
    }

-    $provider = PhabricatorAuthProvider::getEnabledProviderByKey(
-      $account->getProviderKey());
-
-    if (!$provider) {
+    $config = $account->getProviderConfig();
+    if (!$config->getIsEnabled()) {
      $response = $this->renderError(
        pht(
-          'The account you are attempting to register with uses a nonexistent '.
-          'or disabled authentication provider (with key "%s"). An '.
-          'administrator may have recently disabled this provider.',
-          $account->getProviderKey()));
+          'The account you are attempting to register with uses a disabled '.
+          'authentication provider ("%s"). An administrator may have '.
+          'recently disabled this provider.',
+          $config->getDisplayName()));
      return array($account, $provider, $response);
    }

+    $provider = $config->getProvider();
+
    return array($account, $provider, null);
  }

--- a/src/applications/auth/controller/PhabricatorAuthLinkController.php
+++ b/src/applications/auth/controller/PhabricatorAuthLinkController.php
@ -6,14 +6,20 @@ final class PhabricatorAuthLinkController
  public function handleRequest(AphrontRequest $request) {
    $viewer = $this->getViewer();
    $action = $request->getURIData('action');
-    $provider_key = $request->getURIData('pkey');

-    $provider = PhabricatorAuthProvider::getEnabledProviderByKey(
-      $provider_key);
-    if (!$provider) {
+    $id = $request->getURIData('id');
+
+    $config = id(new PhabricatorAuthProviderConfigQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($id))
+      ->withIsEnabled(true)
+      ->executeOne();
+    if (!$config) {
      return new Aphront404Response();
    }

+    $provider = $config->getProvider();
+
    switch ($action) {
      case 'link':
        if (!$provider->shouldAllowAccountLink()) {
@ -37,15 +43,15 @@ final class PhabricatorAuthLinkController
        return new Aphront400Response();
    }

-    $account = id(new PhabricatorExternalAccount())->loadOneWhere(
-      'accountType = %s AND accountDomain = %s AND userPHID = %s',
-      $provider->getProviderType(),
-      $provider->getProviderDomain(),
-      $viewer->getPHID());
+    $accounts = id(new PhabricatorExternalAccountQuery())
+      ->setViewer($viewer)
+      ->withUserPHIDs(array($viewer->getPHID()))
+      ->withProviderConfigPHIDs(array($config->getPHID()))
+      ->execute();

    switch ($action) {
      case 'link':
-        if ($account) {
+        if ($accounts) {
          return $this->renderErrorPage(
            pht('Account Already Linked'),
            array(
@ -56,7 +62,7 @@ final class PhabricatorAuthLinkController
        }
        break;
      case 'refresh':
-        if (!$account) {
+        if (!$accounts) {
          return $this->renderErrorPage(
            pht('No Account Linked'),
            array(
@ -76,11 +82,6 @@ final class PhabricatorAuthLinkController

    switch ($action) {
      case 'link':
-        id(new PhabricatorAuthSessionEngine())->requireHighSecuritySession(
-          $viewer,
-          $request,
-          $panel_uri);
-
        $form = $provider->buildLinkForm($this);
        break;
      case 'refresh':
--- a/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
+++ b/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
@ -31,7 +31,7 @@ final class PhabricatorAuthUnlinkController
    $confirmations = $request->getStrList('confirmations');
    $confirmations = array_fuse($confirmations);

-    if (!$request->isFormPost() || !isset($confirmations['unlink'])) {
+    if (!$request->isFormOrHisecPost() || !isset($confirmations['unlink'])) {
      return $this->renderConfirmDialog($confirmations, $config, $done_uri);
    }

@ -59,6 +59,14 @@ final class PhabricatorAuthUnlinkController
      }
    }

+    $workflow_key = sprintf(
+      'account.unlink(%s)',
+      $account->getPHID());
+
+    $hisec_token = id(new PhabricatorAuthSessionEngine())
+      ->setWorkflowKey($workflow_key)
+      ->requireHighSecurityToken($viewer, $request, $done_uri);
+
    $account->delete();

    id(new PhabricatorAuthSessionEngine())->terminateLoginSessions(
--- a/src/applications/auth/query/PhabricatorExternalAccountQuery.php
+++ b/src/applications/auth/query/PhabricatorExternalAccountQuery.php
@ -21,6 +21,7 @@ final class PhabricatorExternalAccountQuery
  private $userPHIDs;
  private $needImages;
  private $accountSecrets;
+  private $providerConfigPHIDs;

  public function withUserPHIDs(array $user_phids) {
    $this->userPHIDs = $user_phids;
@ -62,6 +63,11 @@ final class PhabricatorExternalAccountQuery
    return $this;
  }

+  public function withProviderConfigPHIDs(array $phids) {
+    $this->providerConfigPHIDs = $phids;
+    return $this;
+  }
+
  public function newResultObject() {
    return new PhabricatorExternalAccount();
  }
@ -181,6 +187,13 @@ final class PhabricatorExternalAccountQuery
        $this->accountSecrets);
    }

+    if ($this->providerConfigPHIDs !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'providerConfigPHID IN (%Ls)',
+        $this->providerConfigPHIDs);
+    }
+
    return $where;
  }

--- a/src/applications/people/controller/PhabricatorPeopleProfilePictureController.php
+++ b/src/applications/people/controller/PhabricatorPeopleProfilePictureController.php
@ -157,13 +157,10 @@ final class PhabricatorPeopleProfilePictureController
        continue;
      }

-      $provider = PhabricatorAuthProvider::getEnabledProviderByKey(
-        $account->getProviderKey());
-      if ($provider) {
-        $tip = pht('Picture From %s', $provider->getProviderName());
-      } else {
-        $tip = pht('Picture From External Account');
-      }
+      $config = $account->getProviderConfig();
+      $provider = $config->getProvider();
+
+      $tip = pht('Picture From %s', $provider->getProviderName());

      if ($file->isTransformableImage()) {
        $images[$file->getPHID()] = array(
--- a/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php
@ -44,17 +44,13 @@ final class PhabricatorExternalAccountsSettingsPanel
    foreach ($accounts as $account) {
      $item = new PHUIObjectItemView();

-      $provider = idx($providers, $account->getProviderKey());
-      if ($provider) {
-        $item->setHeader($provider->getProviderName());
-        $can_unlink = $provider->shouldAllowAccountUnlink();
-        if (!$can_unlink) {
-          $item->addAttribute(pht('Permanently Linked'));
-        }
-      } else {
-        $item->setHeader(
-          pht('Unknown Account ("%s")', $account->getProviderKey()));
-        $can_unlink = true;
+      $config = $account->getProviderConfig();
+      $provider = $config->getProvider();
+
+      $item->setHeader($provider->getProviderName());
+      $can_unlink = $provider->shouldAllowAccountUnlink();
+      if (!$can_unlink) {
+        $item->addAttribute(pht('Permanently Linked'));
      }

      $can_login = $account->isUsableForLogin();
@ -65,12 +61,12 @@ final class PhabricatorExternalAccountsSettingsPanel
            'account provider).'));
      }

-      $can_refresh = $provider && $provider->shouldAllowAccountRefresh();
+      $can_refresh = $provider->shouldAllowAccountRefresh();
      if ($can_refresh) {
        $item->addAction(
          id(new PHUIListItemView())
            ->setIcon('fa-refresh')
-            ->setHref('/auth/refresh/'.$account->getProviderKey().'/'));
+            ->setHref('/auth/refresh/'.$config->getID().'/'));
      }

      $item->addAction(
@ -94,14 +90,15 @@ final class PhabricatorExternalAccountsSettingsPanel
      ->setNoDataString(
        pht('Your account is linked with all available providers.'));

-    $accounts = mpull($accounts, null, 'getProviderKey');
-
    $configs = id(new PhabricatorAuthProviderConfigQuery())
      ->setViewer($viewer)
      ->withIsEnabled(true)
      ->execute();
    $configs = msort($configs, 'getSortVector');

+    $account_map = mgroup($accounts, 'getProviderConfigPHID');
+
+
    foreach ($configs as $config) {
      $provider = $config->getProvider();

@ -110,12 +107,11 @@ final class PhabricatorExternalAccountsSettingsPanel
      }

      // Don't show the user providers they already have linked.
-      $provider_key = $config->getProvider()->getProviderKey();
-      if (isset($accounts[$provider_key])) {
+      if (isset($account_map[$config->getPHID()])) {
        continue;
      }

-      $link_uri = '/auth/link/'.$provider->getProviderKey().'/';
+      $link_uri = '/auth/link/'.$config->getID().'/';

      $link_button = id(new PHUIButtonView())
        ->setTag('a')