Make external account unlinking use account IDs, not "providerType + providerDomain" nonsense

Summary: Depends on D20112. Ref T6703. When you go to unlink an account, unlink it by ID. Crazy!

Test Plan: Unlinked and relinked Google accounts.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20113

src/applications/auth/application/PhabricatorAuthApplication.php

@@ -61,7 +61,7 @@ final class PhabricatorAuthApplication extends PhabricatorApplication {
         'start/' => 'PhabricatorAuthStartController',
         'validate/' => 'PhabricatorAuthValidateController',
         'finish/' => 'PhabricatorAuthFinishController',
-        'unlink/(?P<pkey>[^/]+)/' => 'PhabricatorAuthUnlinkController',
+        'unlink/(?P<id>\d+)/' => 'PhabricatorAuthUnlinkController',
         '(?P<action>link|refresh)/(?P<pkey>[^/]+)/'
           => 'PhabricatorAuthLinkController',
         'confirmlink/(?P<akey>[^/]+)/'

src/applications/auth/controller/PhabricatorAuthUnlinkController.php

@@ -3,48 +3,45 @@
 final class PhabricatorAuthUnlinkController
   extends PhabricatorAuthController {
 
-  private $providerKey;
-
   public function handleRequest(AphrontRequest $request) {
     $viewer = $this->getViewer();
-    $this->providerKey = $request->getURIData('pkey');
+    $id = $request->getURIData('id');
 
-    list($type, $domain) = explode(':', $this->providerKey, 2);
+    $account = id(new PhabricatorExternalAccountQuery())
-
+      ->setViewer($viewer)
-    // Check that this account link actually exists. We don't require the
+      ->withIDs(array($id))
-    // provider to exist because we want users to be able to delete links to
+      ->requireCapabilities(
-    // dead accounts if they want.
+        array(
-    $account = id(new PhabricatorExternalAccount())->loadOneWhere(
+          PhabricatorPolicyCapability::CAN_VIEW,
-      'accountType = %s AND accountDomain = %s AND userPHID = %s',
+          PhabricatorPolicyCapability::CAN_EDIT,
-      $type,
+        ))
-      $domain,
+      ->executeOne();
-      $viewer->getPHID());
     if (!$account) {
-      return $this->renderNoAccountErrorDialog();
+      return new Aphront404Response();
     }
 
-    // Check that the provider (if it exists) allows accounts to be unlinked.
+    $done_uri = '/settings/panel/external/';
-    $provider_key = $this->providerKey;
+
-    $provider = PhabricatorAuthProvider::getEnabledProviderByKey($provider_key);
+    $config = $account->getProviderConfig();
-    if ($provider) {
+    $provider = $config->getProvider();
     if (!$provider->shouldAllowAccountUnlink()) {
-        return $this->renderNotUnlinkableErrorDialog($provider);
+      return $this->renderNotUnlinkableErrorDialog($provider, $done_uri);
-      }
     }
 
     $confirmations = $request->getStrList('confirmations');
     $confirmations = array_fuse($confirmations);
 
     if (!$request->isFormPost() || !isset($confirmations['unlink'])) {
-      return $this->renderConfirmDialog($confirmations);
+      return $this->renderConfirmDialog($confirmations, $config, $done_uri);
     }
 
     // Check that this account isn't the only account which can be used to
     // login. We warn you when you remove your only login account.
     if ($account->isUsableForLogin()) {
-      $other_accounts = id(new PhabricatorExternalAccount())->loadAllWhere(
+      $other_accounts = id(new PhabricatorExternalAccountQuery())
-        'userPHID = %s',
+        ->setViewer($viewer)
-        $viewer->getPHID());
+        ->withUserPHIDs(array($viewer->getPHID()))
+        ->execute();
 
       $valid_accounts = 0;
       foreach ($other_accounts as $other_account) {
@@ -55,7 +52,9 @@ final class PhabricatorAuthUnlinkController
 
       if ($valid_accounts < 2) {
         if (!isset($confirmations['only'])) {
-          return $this->renderOnlyUsableAccountConfirmDialog($confirmations);
+          return $this->renderOnlyUsableAccountConfirmDialog(
+            $confirmations,
+            $done_uri);
         }
       }
     }
@@ -67,42 +66,27 @@ final class PhabricatorAuthUnlinkController
       new PhutilOpaqueEnvelope(
         $request->getCookie(PhabricatorCookies::COOKIE_SESSION)));
 
-    return id(new AphrontRedirectResponse())->setURI($this->getDoneURI());
+    return id(new AphrontRedirectResponse())->setURI($done_uri);
-  }
-
-  private function getDoneURI() {
-    return '/settings/panel/external/';
-  }
-
-  private function renderNoAccountErrorDialog() {
-    $dialog = id(new AphrontDialogView())
-      ->setUser($this->getRequest()->getUser())
-      ->setTitle(pht('No Such Account'))
-      ->appendChild(
-        pht(
-          'You can not unlink this account because it is not linked.'))
-      ->addCancelButton($this->getDoneURI());
-
-    return id(new AphrontDialogResponse())->setDialog($dialog);
   }
 
   private function renderNotUnlinkableErrorDialog(
-    PhabricatorAuthProvider $provider) {
+    PhabricatorAuthProvider $provider,
+    $done_uri) {
 
-    $dialog = id(new AphrontDialogView())
+    return $this->newDialog()
-      ->setUser($this->getRequest()->getUser())
       ->setTitle(pht('Permanent Account Link'))
       ->appendChild(
         pht(
           'You can not unlink this account because the administrator has '.
-          'configured Phabricator to make links to %s accounts permanent.',
+          'configured Phabricator to make links to "%s" accounts permanent.',
           $provider->getProviderName()))
-      ->addCancelButton($this->getDoneURI());
+      ->addCancelButton($done_uri);
-
-    return id(new AphrontDialogResponse())->setDialog($dialog);
   }
 
-  private function renderOnlyUsableAccountConfirmDialog(array $confirmations) {
+  private function renderOnlyUsableAccountConfirmDialog(
+    array $confirmations,
+    $done_uri) {
+
     $confirmations[] = 'only';
 
     return $this->newDialog()
@@ -116,28 +100,23 @@ final class PhabricatorAuthUnlinkController
         pht(
           'If you lose access to your account, you can recover access by '.
           'sending yourself an email login link from the login screen.'))
-      ->addCancelButton($this->getDoneURI())
+      ->addCancelButton($done_uri)
       ->addSubmitButton(pht('Unlink External Account'));
   }
 
-  private function renderConfirmDialog(array $confirmations) {
+  private function renderConfirmDialog(
+    array $confirmations,
+    PhabricatorAuthProviderConfig $config,
+    $done_uri) {
+
     $confirmations[] = 'unlink';
+    $provider = $config->getProvider();
 
-    $provider_key = $this->providerKey;
-    $provider = PhabricatorAuthProvider::getEnabledProviderByKey($provider_key);
-
-    if ($provider) {
     $title = pht('Unlink "%s" Account?', $provider->getProviderName());
     $body = pht(
       'You will no longer be able to use your %s account to '.
       'log in to Phabricator.',
       $provider->getProviderName());
-    } else {
-      $title = pht('Unlink Account?');
-      $body = pht(
-        'You will no longer be able to use this account to log in '.
-        'to Phabricator.');
-    }
 
     return $this->newDialog()
       ->setTitle($title)
@@ -148,7 +127,7 @@ final class PhabricatorAuthUnlinkController
           'Note: Unlinking an authentication provider will terminate any '.
           'other active login sessions.'))
       ->addSubmitButton(pht('Unlink Account'))
-      ->addCancelButton($this->getDoneURI());
+      ->addCancelButton($done_uri);
   }
 
 }

src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php

@@ -78,7 +78,7 @@ final class PhabricatorExternalAccountsSettingsPanel
           ->setIcon('fa-times')
           ->setWorkflow(true)
           ->setDisabled(!$can_unlink)
-          ->setHref('/auth/unlink/'.$account->getProviderKey().'/'));
+          ->setHref('/auth/unlink/'.$account->getID().'/'));
 
       if ($provider) {
         $provider->willRenderLinkedAccount($viewer, $item, $account);