From d22495a820c67a7e5e156f2a8792620bea44bbdc Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 6 Feb 2019 17:12:12 -0800 Subject: [PATCH] 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 --- .../PhabricatorAuthApplication.php | 2 +- .../PhabricatorAuthConfirmLinkController.php | 19 ++++++----- .../controller/PhabricatorAuthController.php | 16 ++++----- .../PhabricatorAuthLinkController.php | 33 ++++++++++--------- .../PhabricatorAuthUnlinkController.php | 10 +++++- .../query/PhabricatorExternalAccountQuery.php | 13 ++++++++ ...bricatorPeopleProfilePictureController.php | 11 +++---- ...abricatorExternalAccountsSettingsPanel.php | 32 ++++++++---------- 8 files changed, 76 insertions(+), 60 deletions(-) diff --git a/src/applications/auth/application/PhabricatorAuthApplication.php b/src/applications/auth/application/PhabricatorAuthApplication.php index 0b3487b71b..52cf01b2aa 100644 --- 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\d+)/' => 'PhabricatorAuthUnlinkController', - '(?Plink|refresh)/(?P[^/]+)/' + '(?Plink|refresh)/(?P\d+)/' => 'PhabricatorAuthLinkController', 'confirmlink/(?P[^/]+)/' => 'PhabricatorAuthConfirmLinkController', diff --git a/src/applications/auth/controller/PhabricatorAuthConfirmLinkController.php b/src/applications/auth/controller/PhabricatorAuthConfirmLinkController.php index 664a97885f..9ceb10df8b 100644 --- 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')); diff --git a/src/applications/auth/controller/PhabricatorAuthController.php b/src/applications/auth/controller/PhabricatorAuthController.php index 9b7267ec96..e81301218c 100644 --- 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); } diff --git a/src/applications/auth/controller/PhabricatorAuthLinkController.php b/src/applications/auth/controller/PhabricatorAuthLinkController.php index 44176a278e..4b127b9ad1 100644 --- 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': diff --git a/src/applications/auth/controller/PhabricatorAuthUnlinkController.php b/src/applications/auth/controller/PhabricatorAuthUnlinkController.php index 004ddf4f9a..43e7b1b362 100644 --- 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( diff --git a/src/applications/auth/query/PhabricatorExternalAccountQuery.php b/src/applications/auth/query/PhabricatorExternalAccountQuery.php index f67fb4b581..1c5b3fe14f 100644 --- 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; } diff --git a/src/applications/people/controller/PhabricatorPeopleProfilePictureController.php b/src/applications/people/controller/PhabricatorPeopleProfilePictureController.php index 5cab924255..92bb2e0b86 100644 --- 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( diff --git a/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php b/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php index 9904c7369f..60389e1590 100644 --- 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')