diff --git a/src/applications/auth/application/PhabricatorAuthApplication.php b/src/applications/auth/application/PhabricatorAuthApplication.php --- a/src/applications/auth/application/PhabricatorAuthApplication.php +++ b/src/applications/auth/application/PhabricatorAuthApplication.php @@ -62,7 +62,7 @@ '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 --- a/src/applications/auth/controller/PhabricatorAuthConfirmLinkController.php +++ b/src/applications/auth/controller/PhabricatorAuthConfirmLinkController.php @@ -20,7 +20,15 @@ $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 @@ 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 --- a/src/applications/auth/controller/PhabricatorAuthController.php +++ b/src/applications/auth/controller/PhabricatorAuthController.php @@ -213,19 +213,19 @@ 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 --- a/src/applications/auth/controller/PhabricatorAuthLinkController.php +++ b/src/applications/auth/controller/PhabricatorAuthLinkController.php @@ -6,14 +6,20 @@ 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 @@ 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 @@ } break; case 'refresh': - if (!$account) { + if (!$accounts) { return $this->renderErrorPage( pht('No Account Linked'), array( @@ -76,11 +82,6 @@ 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 --- a/src/applications/auth/controller/PhabricatorAuthUnlinkController.php +++ b/src/applications/auth/controller/PhabricatorAuthUnlinkController.php @@ -31,7 +31,7 @@ $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 @@ } } + $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 --- a/src/applications/auth/query/PhabricatorExternalAccountQuery.php +++ b/src/applications/auth/query/PhabricatorExternalAccountQuery.php @@ -21,6 +21,7 @@ private $userPHIDs; private $needImages; private $accountSecrets; + private $providerConfigPHIDs; public function withUserPHIDs(array $user_phids) { $this->userPHIDs = $user_phids; @@ -62,6 +63,11 @@ return $this; } + public function withProviderConfigPHIDs(array $phids) { + $this->providerConfigPHIDs = $phids; + return $this; + } + public function newResultObject() { return new PhabricatorExternalAccount(); } @@ -181,6 +187,13 @@ $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 --- a/src/applications/people/controller/PhabricatorPeopleProfilePictureController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfilePictureController.php @@ -157,13 +157,10 @@ 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 --- a/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php @@ -44,17 +44,13 @@ 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 @@ '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 @@ ->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 @@ } // 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')