From b040f889dee2707c70b6f108bf13c1895f5af9cd Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 17 Jun 2013 06:12:45 -0700 Subject: [PATCH] Move all account link / unlink to new registration flow Summary: Ref T1536. Currently, we have separate panels for each link/unlink and separate controllers for OAuth vs LDAP. Instead, provide a single "External Accounts" panel which shows all linked accounts and allows you to link/unlink more easily. Move link/unlink over to a full externalaccount-based workflow. Test Plan: - Linked and unlinked OAuth accounts. - Linked and unlinked LDAP accounts. - Registered new accounts. - Exercised most/all of the error cases. Reviewers: btrahan, chad Reviewed By: btrahan CC: aran, mbishopim3 Maniphest Tasks: T1536 Differential Revision: https://secure.phabricator.com/D6189 --- src/__celerity_resource_map__.php | 2 +- src/__phutil_library_map__.php | 16 +- .../PhabricatorApplicationAuth.php | 6 +- .../PhabricatorAuthConfirmLinkController.php | 89 ++++++++++ .../controller/PhabricatorAuthController.php | 105 ++++++++++++ .../PhabricatorAuthLinkController.php | 81 +++++++++ .../PhabricatorAuthLoginController.php | 42 +++-- .../PhabricatorAuthRegisterController.php | 152 ++++------------- .../PhabricatorAuthUnlinkController.php | 140 ++++++++++++++++ .../PhabricatorEmailTokenController.php | 8 +- .../PhabricatorLDAPUnlinkController.php | 38 ----- .../PhabricatorOAuthUnlinkController.php | 51 ------ .../provider/PhabricatorOAuthProvider.php | 4 +- .../auth/provider/PhabricatorAuthProvider.php | 36 +++- .../provider/PhabricatorAuthProviderLDAP.php | 19 +-- .../provider/PhabricatorAuthProviderOAuth.php | 11 +- .../PhabricatorAuthProviderPassword.php | 13 +- .../storage/PhabricatorExternalAccount.php | 15 ++ ...abricatorSettingsPanelExternalAccounts.php | 120 ++++++++++++++ .../panel/PhabricatorSettingsPanelLDAP.php | 92 ----------- .../panel/PhabricatorSettingsPanelOAuth.php | 156 ------------------ src/view/phui/PHUIButtonView.php | 2 +- src/view/phui/PHUIListItemView.php | 17 +- webroot/rsrc/css/application/auth/auth.css | 6 +- 24 files changed, 691 insertions(+), 530 deletions(-) create mode 100644 src/applications/auth/controller/PhabricatorAuthConfirmLinkController.php create mode 100644 src/applications/auth/controller/PhabricatorAuthLinkController.php create mode 100644 src/applications/auth/controller/PhabricatorAuthUnlinkController.php delete mode 100644 src/applications/auth/controller/PhabricatorLDAPUnlinkController.php delete mode 100644 src/applications/auth/controller/PhabricatorOAuthUnlinkController.php create mode 100644 src/applications/settings/panel/PhabricatorSettingsPanelExternalAccounts.php delete mode 100644 src/applications/settings/panel/PhabricatorSettingsPanelLDAP.php delete mode 100644 src/applications/settings/panel/PhabricatorSettingsPanelOAuth.php diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index ca2ed17ab5..e8eb3716c9 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -923,7 +923,7 @@ celerity_register_resource_map(array( ), 'auth-css' => array( - 'uri' => '/res/8a95bad7/rsrc/css/application/auth/auth.css', + 'uri' => '/res/81f72bfa/rsrc/css/application/auth/auth.css', 'type' => 'css', 'requires' => array( diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8444aca369..8f634a4bf8 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -814,7 +814,9 @@ phutil_register_library_map(array( 'PhabricatorAuditQuery' => 'applications/audit/query/PhabricatorAuditQuery.php', 'PhabricatorAuditReplyHandler' => 'applications/audit/mail/PhabricatorAuditReplyHandler.php', 'PhabricatorAuditStatusConstants' => 'applications/audit/constants/PhabricatorAuditStatusConstants.php', + 'PhabricatorAuthConfirmLinkController' => 'applications/auth/controller/PhabricatorAuthConfirmLinkController.php', 'PhabricatorAuthController' => 'applications/auth/controller/PhabricatorAuthController.php', + 'PhabricatorAuthLinkController' => 'applications/auth/controller/PhabricatorAuthLinkController.php', 'PhabricatorAuthLoginController' => 'applications/auth/controller/PhabricatorAuthLoginController.php', 'PhabricatorAuthProvider' => 'applications/auth/provider/PhabricatorAuthProvider.php', 'PhabricatorAuthProviderLDAP' => 'applications/auth/provider/PhabricatorAuthProviderLDAP.php', @@ -826,6 +828,7 @@ phutil_register_library_map(array( 'PhabricatorAuthProviderPassword' => 'applications/auth/provider/PhabricatorAuthProviderPassword.php', 'PhabricatorAuthRegisterController' => 'applications/auth/controller/PhabricatorAuthRegisterController.php', 'PhabricatorAuthStartController' => 'applications/auth/controller/PhabricatorAuthStartController.php', + 'PhabricatorAuthUnlinkController' => 'applications/auth/controller/PhabricatorAuthUnlinkController.php', 'PhabricatorAuthValidateController' => 'applications/auth/controller/PhabricatorAuthValidateController.php', 'PhabricatorAuthenticationConfigOptions' => 'applications/config/option/PhabricatorAuthenticationConfigOptions.php', 'PhabricatorBarePageExample' => 'applications/uiexample/examples/PhabricatorBarePageExample.php', @@ -1111,7 +1114,6 @@ phutil_register_library_map(array( 'PhabricatorLDAPProvider' => 'applications/auth/ldap/PhabricatorLDAPProvider.php', 'PhabricatorLDAPRegistrationController' => 'applications/auth/controller/PhabricatorLDAPRegistrationController.php', 'PhabricatorLDAPUnknownUserException' => 'applications/auth/ldap/PhabricatorLDAPUnknownUserException.php', - 'PhabricatorLDAPUnlinkController' => 'applications/auth/controller/PhabricatorLDAPUnlinkController.php', 'PhabricatorLintEngine' => 'infrastructure/lint/PhabricatorLintEngine.php', 'PhabricatorLipsumArtist' => 'applications/lipsum/image/PhabricatorLipsumArtist.php', 'PhabricatorLipsumGenerateWorkflow' => 'applications/lipsum/management/PhabricatorLipsumGenerateWorkflow.php', @@ -1241,7 +1243,6 @@ phutil_register_library_map(array( 'PhabricatorOAuthServerTestCase' => 'applications/oauthserver/__tests__/PhabricatorOAuthServerTestCase.php', 'PhabricatorOAuthServerTestController' => 'applications/oauthserver/controller/PhabricatorOAuthServerTestController.php', 'PhabricatorOAuthServerTokenController' => 'applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php', - 'PhabricatorOAuthUnlinkController' => 'applications/auth/controller/PhabricatorOAuthUnlinkController.php', 'PhabricatorObjectHandle' => 'applications/phid/PhabricatorObjectHandle.php', 'PhabricatorObjectHandleConstants' => 'applications/phid/handle/const/PhabricatorObjectHandleConstants.php', 'PhabricatorObjectHandleData' => 'applications/phid/handle/PhabricatorObjectHandleData.php', @@ -1444,9 +1445,8 @@ phutil_register_library_map(array( 'PhabricatorSettingsPanelDisplayPreferences' => 'applications/settings/panel/PhabricatorSettingsPanelDisplayPreferences.php', 'PhabricatorSettingsPanelEmailAddresses' => 'applications/settings/panel/PhabricatorSettingsPanelEmailAddresses.php', 'PhabricatorSettingsPanelEmailPreferences' => 'applications/settings/panel/PhabricatorSettingsPanelEmailPreferences.php', + 'PhabricatorSettingsPanelExternalAccounts' => 'applications/settings/panel/PhabricatorSettingsPanelExternalAccounts.php', 'PhabricatorSettingsPanelHomePreferences' => 'applications/settings/panel/PhabricatorSettingsPanelHomePreferences.php', - 'PhabricatorSettingsPanelLDAP' => 'applications/settings/panel/PhabricatorSettingsPanelLDAP.php', - 'PhabricatorSettingsPanelOAuth' => 'applications/settings/panel/PhabricatorSettingsPanelOAuth.php', 'PhabricatorSettingsPanelPassword' => 'applications/settings/panel/PhabricatorSettingsPanelPassword.php', 'PhabricatorSettingsPanelProfile' => 'applications/settings/panel/PhabricatorSettingsPanelProfile.php', 'PhabricatorSettingsPanelSSHKeys' => 'applications/settings/panel/PhabricatorSettingsPanelSSHKeys.php', @@ -2678,7 +2678,9 @@ phutil_register_library_map(array( 'PhabricatorAuditMailReceiver' => 'PhabricatorObjectMailReceiver', 'PhabricatorAuditPreviewController' => 'PhabricatorAuditController', 'PhabricatorAuditReplyHandler' => 'PhabricatorMailReplyHandler', + 'PhabricatorAuthConfirmLinkController' => 'PhabricatorAuthController', 'PhabricatorAuthController' => 'PhabricatorController', + 'PhabricatorAuthLinkController' => 'PhabricatorAuthController', 'PhabricatorAuthLoginController' => 'PhabricatorAuthController', 'PhabricatorAuthProviderLDAP' => 'PhabricatorAuthProvider', 'PhabricatorAuthProviderOAuth' => 'PhabricatorAuthProvider', @@ -2689,6 +2691,7 @@ phutil_register_library_map(array( 'PhabricatorAuthProviderPassword' => 'PhabricatorAuthProvider', 'PhabricatorAuthRegisterController' => 'PhabricatorAuthController', 'PhabricatorAuthStartController' => 'PhabricatorAuthController', + 'PhabricatorAuthUnlinkController' => 'PhabricatorAuthController', 'PhabricatorAuthValidateController' => 'PhabricatorAuthController', 'PhabricatorAuthenticationConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorBarePageExample' => 'PhabricatorUIExample', @@ -2971,7 +2974,6 @@ phutil_register_library_map(array( 'PhabricatorLDAPLoginController' => 'PhabricatorAuthController', 'PhabricatorLDAPRegistrationController' => 'PhabricatorAuthController', 'PhabricatorLDAPUnknownUserException' => 'Exception', - 'PhabricatorLDAPUnlinkController' => 'PhabricatorAuthController', 'PhabricatorLintEngine' => 'PhutilLintEngine', 'PhabricatorLipsumGenerateWorkflow' => 'PhabricatorLipsumManagementWorkflow', 'PhabricatorLipsumManagementWorkflow' => 'PhutilArgumentWorkflow', @@ -3093,7 +3095,6 @@ phutil_register_library_map(array( 'PhabricatorOAuthServerTestCase' => 'PhabricatorTestCase', 'PhabricatorOAuthServerTestController' => 'PhabricatorOAuthServerController', 'PhabricatorOAuthServerTokenController' => 'PhabricatorAuthController', - 'PhabricatorOAuthUnlinkController' => 'PhabricatorAuthController', 'PhabricatorObjectHandleStatus' => 'PhabricatorObjectHandleConstants', 'PhabricatorObjectItemListExample' => 'PhabricatorUIExample', 'PhabricatorObjectItemListView' => 'AphrontTagView', @@ -3302,9 +3303,8 @@ phutil_register_library_map(array( 'PhabricatorSettingsPanelDisplayPreferences' => 'PhabricatorSettingsPanel', 'PhabricatorSettingsPanelEmailAddresses' => 'PhabricatorSettingsPanel', 'PhabricatorSettingsPanelEmailPreferences' => 'PhabricatorSettingsPanel', + 'PhabricatorSettingsPanelExternalAccounts' => 'PhabricatorSettingsPanel', 'PhabricatorSettingsPanelHomePreferences' => 'PhabricatorSettingsPanel', - 'PhabricatorSettingsPanelLDAP' => 'PhabricatorSettingsPanel', - 'PhabricatorSettingsPanelOAuth' => 'PhabricatorSettingsPanel', 'PhabricatorSettingsPanelPassword' => 'PhabricatorSettingsPanel', 'PhabricatorSettingsPanelProfile' => 'PhabricatorSettingsPanel', 'PhabricatorSettingsPanelSSHKeys' => 'PhabricatorSettingsPanel', diff --git a/src/applications/auth/application/PhabricatorApplicationAuth.php b/src/applications/auth/application/PhabricatorApplicationAuth.php index 0a5a263042..a918561eb9 100644 --- a/src/applications/auth/application/PhabricatorApplicationAuth.php +++ b/src/applications/auth/application/PhabricatorApplicationAuth.php @@ -40,6 +40,10 @@ final class PhabricatorApplicationAuth extends PhabricatorApplication { 'register/(?:(?P[^/]+)/)?' => 'PhabricatorAuthRegisterController', 'start/' => 'PhabricatorAuthStartController', 'validate/' => 'PhabricatorAuthValidateController', + 'unlink/(?P[^/]+)/' => 'PhabricatorAuthUnlinkController', + 'link/(?P[^/]+)/' => 'PhabricatorAuthLinkController', + 'confirmlink/(?P[^/]+)/' + => 'PhabricatorAuthConfirmLinkController', ), '/login/' => array( @@ -56,13 +60,11 @@ final class PhabricatorApplicationAuth extends PhabricatorApplication { '(?P\w+)/' => array( 'login/' => 'PhabricatorOAuthLoginController', 'diagnose/' => 'PhabricatorOAuthDiagnosticsController', - 'unlink/' => 'PhabricatorOAuthUnlinkController', ), ), '/ldap/' => array( 'login/' => 'PhabricatorLDAPLoginController', - 'unlink/' => 'PhabricatorLDAPUnlinkController', ), ); } diff --git a/src/applications/auth/controller/PhabricatorAuthConfirmLinkController.php b/src/applications/auth/controller/PhabricatorAuthConfirmLinkController.php new file mode 100644 index 0000000000..6f7f30b015 --- /dev/null +++ b/src/applications/auth/controller/PhabricatorAuthConfirmLinkController.php @@ -0,0 +1,89 @@ +accountKey = idx($data, 'akey'); + } + + public function processRequest() { + $request = $this->getRequest(); + $viewer = $request->getUser(); + + $result = $this->loadAccountForRegistrationOrLinking($this->accountKey); + list($account, $provider, $response) = $result; + + if ($response) { + return $response; + } + + if (!$provider->shouldAllowAccountLink()) { + return $this->renderError(pht('This account is not linkable.')); + } + + $panel_uri = '/settings/panel/external/'; + + if ($request->isFormPost()) { + $account->setUserPHID($viewer->getPHID()); + $account->save(); + + $this->clearRegistrationCookies(); + + // TODO: Send the user email about the new account link. + + 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) + ->setTitle(pht('Confirm %s Account Link', $provider->getProviderName())) + ->addCancelButton($panel_uri) + ->addSubmitButton(pht('Confirm Account Link')); + + $form = id(new AphrontFormLayoutView()) + ->setFullWidth(true) + ->appendChild( + phutil_tag( + 'div', + array( + 'class' => 'aphront-form-instructions', + ), + pht( + "Confirm the link with this %s account. This account will be ". + "able to log in to your Phabricator account.", + $provider->getProviderName()))); + + $dialog->appendChild($form); + + $crumbs = $this->buildApplicationCrumbs(); + $crumbs->addCrumb( + id(new PhabricatorCrumbView()) + ->setName(pht('Confirm Link')) + ->setHref($panel_uri)); + $crumbs->addCrumb( + id(new PhabricatorCrumbView()) + ->setName($provider->getProviderName())); + + return $this->buildApplicationPage( + array( + $crumbs, + $dialog, + ), + array( + 'title' => pht('Confirm External Account Link'), + 'dust' => true, + 'device' => true, + )); + } + + +} diff --git a/src/applications/auth/controller/PhabricatorAuthController.php b/src/applications/auth/controller/PhabricatorAuthController.php index 173aae06e4..d37aed6912 100644 --- a/src/applications/auth/controller/PhabricatorAuthController.php +++ b/src/applications/auth/controller/PhabricatorAuthController.php @@ -29,6 +29,7 @@ abstract class PhabricatorAuthController extends PhabricatorController { } + /** * Log a user into a web session and return an @{class:AphrontResponse} which * corresponds to continuing the login process. @@ -99,4 +100,108 @@ abstract class PhabricatorAuthController extends PhabricatorController { )); } + protected function loadAccountForRegistrationOrLinking($account_key) { + $request = $this->getRequest(); + $viewer = $request->getUser(); + + $account = null; + $provider = null; + $response = null; + + if (!$account_key) { + $response = $this->renderError( + pht('Request did not include account key.')); + return array($account, $provider, $response); + } + + $account = id(new PhabricatorExternalAccount())->loadOneWhere( + 'accountSecret = %s', + $account_key); + if (!$account) { + $response = $this->renderError(pht('No valid linkable account.')); + return array($account, $provider, $response); + } + + if ($account->getUserPHID()) { + if ($account->getUserPHID() != $viewer->getUserPHID()) { + $response = $this->renderError( + pht( + 'The account you are attempting to register or link is already '. + 'linked to another user.')); + } else { + $response = $this->renderError( + pht( + 'The account you are attempting to link is already linked '. + 'to your account.')); + } + return array($account, $provider, $response); + } + + $registration_key = $request->getCookie('phreg'); + + // NOTE: This registration key check is not strictly necessary, because + // we're only creating new accounts, not linking existing accounts. It + // might be more hassle than it is worth, especially for email. + // + // The attack this prevents is getting to the registration screen, then + // copy/pasting the URL and getting someone else to click it and complete + // the process. They end up with an account bound to credentials you + // control. This doesn't really let you do anything meaningful, though, + // since you could have simply completed the process yourself. + + if (!$registration_key) { + $response = $this->renderError( + pht( + 'Your browser did not submit a registration key with the request. '. + 'You must use the same browser to begin and complete registration. '. + 'Check that cookies are enabled and try again.')); + return array($account, $provider, $response); + } + + // We store the digest of the key rather than the key itself to prevent a + // theoretical attacker with read-only access to the database from + // hijacking registration sessions. + + $actual = $account->getProperty('registrationKey'); + $expect = PhabricatorHash::digest($registration_key); + if ($actual !== $expect) { + $response = $this->renderError( + pht( + 'Your browser submitted a different registration key than the one '. + 'associated with this account. You may need to clear your cookies.')); + return array($account, $provider, $response); + } + + $other_account = id(new PhabricatorExternalAccount())->loadAllWhere( + 'accountType = %s AND accountDomain = %s AND accountID = %s + AND id != %d', + $account->getAccountType(), + $account->getAccountDomain(), + $account->getAccountID(), + $account->getID()); + + if ($other_account) { + $response = $this->renderError( + pht( + 'The account you are attempting to register with already belongs '. + 'to another user.')); + return array($account, $provider, $response); + } + + $provider = PhabricatorAuthProvider::getEnabledProviderByKey( + $account->getProviderKey()); + + if (!$provider) { + $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())); + return array($account, $provider, $response); + } + + return array($account, $provider, null); + } + } diff --git a/src/applications/auth/controller/PhabricatorAuthLinkController.php b/src/applications/auth/controller/PhabricatorAuthLinkController.php new file mode 100644 index 0000000000..20e309337e --- /dev/null +++ b/src/applications/auth/controller/PhabricatorAuthLinkController.php @@ -0,0 +1,81 @@ +providerKey = $data['pkey']; + } + + public function processRequest() { + $request = $this->getRequest(); + $viewer = $request->getUser(); + + $provider = PhabricatorAuthProvider::getEnabledProviderByKey( + $this->providerKey); + if (!$provider) { + return new Aphront404Response(); + } + + if (!$provider->shouldAllowAccountLink()) { + return $this->renderErrorPage( + pht('Account Not Linkable'), + array( + pht('This provider is not configured to allow linking.'), + )); + } + + $account = id(new PhabricatorExternalAccount())->loadOneWhere( + 'accountType = %s AND accountDomain = %s AND userPHID = %s', + $provider->getProviderType(), + $provider->getProviderDomain(), + $viewer->getPHID()); + if ($account) { + return $this->renderErrorPage( + pht('Account Already Linked'), + array( + pht( + 'Your Phabricator account is already linked to an external '. + 'account for this provider.'), + )); + } + + $panel_uri = '/settings/panel/external/'; + + $request->setCookie('phcid', Filesystem::readRandomCharacters(16)); + $form = $provider->buildLinkForm($this); + + if ($provider->isLoginFormAButton()) { + require_celerity_resource('auth-css'); + $form = phutil_tag( + 'div', + array( + 'class' => 'phabricator-link-button pl', + ), + $form); + } + + $crumbs = $this->buildApplicationCrumbs(); + $crumbs->addCrumb( + id(new PhabricatorCrumbView()) + ->setName(pht('Link Account')) + ->setHref($panel_uri)); + $crumbs->addCrumb( + id(new PhabricatorCrumbView()) + ->setName($provider->getProviderName())); + + return $this->buildApplicationPage( + array( + $crumbs, + $form, + ), + array( + 'title' => pht('Link %s Account', $provider->getProviderName()), + 'dust' => true, + 'device' => true, + )); + } + +} diff --git a/src/applications/auth/controller/PhabricatorAuthLoginController.php b/src/applications/auth/controller/PhabricatorAuthLoginController.php index 4a4711a34d..29ecfaa55e 100644 --- a/src/applications/auth/controller/PhabricatorAuthLoginController.php +++ b/src/applications/auth/controller/PhabricatorAuthLoginController.php @@ -111,8 +111,23 @@ final class PhabricatorAuthLoginController } private function processRegisterUser(PhabricatorExternalAccount $account) { + $account_secret = $account->getAccountSecret(); + $register_uri = $this->getApplicationURI('register/'.$account_secret.'/'); + return $this->setAccountKeyAndContinue($account, $register_uri); + } + + private function processLinkUser(PhabricatorExternalAccount $account) { + $account_secret = $account->getAccountSecret(); + $confirm_uri = $this->getApplicationURI('confirmlink/'.$account_secret.'/'); + return $this->setAccountKeyAndContinue($account, $confirm_uri); + } + + private function setAccountKeyAndContinue( + PhabricatorExternalAccount $account, + $next_uri) { + if ($account->getUserPHID()) { - throw new Exception("Account is already registered."); + throw new Exception("Account is already registered or linked."); } // Regenerate the registration secret key, set it on the external account, @@ -131,14 +146,7 @@ final class PhabricatorAuthLoginController $this->getRequest()->setCookie('phreg', $registration_key); - $account_secret = $account->getAccountSecret(); - $register_uri = $this->getApplicationURI('register/'.$account_secret.'/'); - return id(new AphrontRedirectResponse())->setURI($register_uri); - } - - private function processLinkUser(PhabricatorExternalAccount $account) { - // TODO: Implement. - return new Aphront404Response(); + return id(new AphrontRedirectResponse())->setURI($next_uri); } private function loadProvider() { @@ -160,19 +168,9 @@ final class PhabricatorAuthLoginController } protected function renderError($message) { - $title = pht('Login Failed'); - - $view = new AphrontErrorView(); - $view->setTitle($title); - $view->appendChild($message); - - return $this->buildApplicationPage( - $view, - array( - 'title' => $title, - 'device' => true, - 'dust' => true, - )); + return $this->renderErrorPage( + pht('Login Failed'), + array($message)); } public function buildProviderPageResponse( diff --git a/src/applications/auth/controller/PhabricatorAuthRegisterController.php b/src/applications/auth/controller/PhabricatorAuthRegisterController.php index a5df51f255..9c90f34582 100644 --- a/src/applications/auth/controller/PhabricatorAuthRegisterController.php +++ b/src/applications/auth/controller/PhabricatorAuthRegisterController.php @@ -4,8 +4,6 @@ final class PhabricatorAuthRegisterController extends PhabricatorAuthController { private $accountKey; - private $account; - private $provider; public function shouldRequireLogin() { return false; @@ -23,16 +21,30 @@ final class PhabricatorAuthRegisterController } if (strlen($this->accountKey)) { - $response = $this->loadAccount(); + $result = $this->loadAccountForRegistrationOrLinking($this->accountKey); + list($account, $provider, $response) = $result; } else { - $response = $this->loadDefaultAccount(); + list($account, $provider, $response) = $this->loadDefaultAccount(); } if ($response) { return $response; } - $account = $this->account; + if (!$provider->shouldAllowRegistration()) { + + // TODO: This is a routine error if you click "Login" on an external + // auth source which doesn't allow registration. The error should be + // more tailored. + + return $this->renderError( + pht( + 'The account you are attempting to register with uses an '. + 'authentication provider ("%s") which does not allow registration. '. + 'An administrator may have recently disabled registration with this '. + 'provider.', + $provider->getProviderName())); + } $user = new PhabricatorUser(); @@ -88,7 +100,7 @@ final class PhabricatorAuthRegisterController $can_edit_email = $profile->getCanEditEmail(); $can_edit_realname = $profile->getCanEditRealName(); - $must_set_password = $this->provider->shouldRequireRegistrationPassword(); + $must_set_password = $provider->shouldRequireRegistrationPassword(); $can_edit_anything = $profile->getCanEditAnything() || $must_set_password; $force_verify = $profile->getShouldVerifyEmail(); @@ -200,7 +212,7 @@ final class PhabricatorAuthRegisterController } $account->setUserPHID($user->getPHID()); - $this->provider->willRegisterAccount($account); + $provider->willRegisterAccount($account); $account->save(); $user->saveTransaction(); @@ -315,134 +327,38 @@ final class PhabricatorAuthRegisterController )); } - private function loadAccount() { - $request = $this->getRequest(); - - if (!$this->accountKey) { - return $this->renderError(pht('Request did not include account key.')); - } - - $account = id(new PhabricatorExternalAccount())->loadOneWhere( - 'accountSecret = %s', - $this->accountKey); - - if (!$account) { - return $this->renderError(pht('No registration account.')); - } - - if ($account->getUserPHID()) { - return $this->renderError( - pht( - 'The account you are attempting to register with is already '. - 'registered to another user.')); - } - - $registration_key = $request->getCookie('phreg'); - - // NOTE: This registration key check is not strictly necessary, because - // we're only creating new accounts, not linking existing accounts. It - // might be more hassle than it is worth, especially for email. - // - // The attack this prevents is getting to the registration screen, then - // copy/pasting the URL and getting someone else to click it and complete - // the process. They end up with an account bound to credentials you - // control. This doesn't really let you do anything meaningful, though, - // since you could have simply completed the process yourself. - - if (!$registration_key) { - return $this->renderError( - pht( - 'Your browser did not submit a registration key with the request. '. - 'You must use the same browser to begin and complete registration. '. - 'Check that cookies are enabled and try again.')); - } - - // We store the digest of the key rather than the key itself to prevent a - // theoretical attacker with read-only access to the database from - // hijacking registration sessions. - - $actual = $account->getProperty('registrationKey'); - $expect = PhabricatorHash::digest($registration_key); - if ($actual !== $expect) { - return $this->renderError( - pht( - 'Your browser submitted a different registration key than the one '. - 'associated with this account. You may need to clear your cookies.')); - } - - $other_account = id(new PhabricatorExternalAccount())->loadAllWhere( - 'accountType = %s AND accountDomain = %s AND accountID = %s - AND id != %d', - $account->getAccountType(), - $account->getAccountDomain(), - $account->getAccountID(), - $account->getID()); - - if ($other_account) { - return $this->renderError( - pht( - 'The account you are attempting to register with already belongs '. - 'to another user.')); - } - - $provider = PhabricatorAuthProvider::getEnabledProviderByKey( - $account->getProviderKey()); - - if (!$provider) { - return $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())); - } - - if (!$provider->shouldAllowRegistration()) { - - // TODO: This is a routine error if you click "Login" on an external - // auth source which doesn't allow registration. The error should be - // more tailored. - - return $this->renderError( - pht( - 'The account you are attempting to register with uses an '. - 'authentication provider ("%s") which does not allow registration. '. - 'An administrator may have recently disabled registration with this '. - 'provider.', - $provider->getProviderName())); - } - - $this->account = $account; - $this->provider = $provider; - - return null; - } - private function loadDefaultAccount() { $providers = PhabricatorAuthProvider::getAllEnabledProviders(); - foreach ($providers as $key => $provider) { - if (!$provider->shouldAllowRegistration()) { + $account = null; + $provider = null; + $response = null; + + foreach ($providers as $key => $candidate_provider) { + if (!$candidate_provider->shouldAllowRegistration()) { unset($providers[$key]); continue; } - if (!$provider->isDefaultRegistrationProvider()) { + if (!$candidate_provider->isDefaultRegistrationProvider()) { unset($providers[$key]); } } if (!$providers) { - return $this->renderError( + $response = $this->renderError( pht( "There are no configured default registration providers.")); + return array($account, $provider, $response); } else if (count($providers) > 1) { - return $this->renderError( + $response = $this->renderError( pht( "There are too many configured default registration providers.")); + return array($account, $provider, $response); } - $this->account = $provider->getDefaultExternalAccount(); - $this->provider = $provider; - return null; + $provider = head($providers); + $account = $provider->getDefaultExternalAccount(); + + return array($account, $provider, $response); } private function loadProfilePicture(PhabricatorExternalAccount $account) { diff --git a/src/applications/auth/controller/PhabricatorAuthUnlinkController.php b/src/applications/auth/controller/PhabricatorAuthUnlinkController.php new file mode 100644 index 0000000000..a6b1e06f67 --- /dev/null +++ b/src/applications/auth/controller/PhabricatorAuthUnlinkController.php @@ -0,0 +1,140 @@ +providerKey = $data['pkey']; + } + + public function processRequest() { + $request = $this->getRequest(); + $viewer = $request->getUser(); + + list($type, $domain) = explode(':', $this->providerKey, 2); + + // Check that this account link actually exists. We don't require the + // provider to exist because we want users to be able to delete links to + // dead accounts if they want. + $account = id(new PhabricatorExternalAccount())->loadOneWhere( + 'accountType = %s AND accountDomain = %s AND userPHID = %s', + $type, + $domain, + $viewer->getPHID()); + if (!$account) { + return $this->renderNoAccountErrorDialog(); + } + + // Check that the provider (if it exists) allows accounts to be unlinked. + $provider_key = $this->providerKey; + $provider = PhabricatorAuthProvider::getEnabledProviderByKey($provider_key); + if ($provider) { + if (!$provider->shouldAllowAccountUnlink()) { + return $this->renderNotUnlinkableErrorDialog($provider); + } + } + + // Check that this account isn't the last account which can be used to + // login. We prevent you from removing the last account. + if ($account->isUsableForLogin()) { + $other_accounts = id(new PhabricatorExternalAccount())->loadAllWhere( + 'userPHID = %s', + $viewer->getPHID()); + + $valid_accounts = 0; + foreach ($other_accounts as $other_account) { + if ($other_account->isUsableForLogin()) { + $valid_accounts++; + } + } + + if ($valid_accounts < 2) { + return $this->renderLastUsableAccountErrorDialog(); + } + } + + if ($request->isDialogFormPost()) { + $account->delete(); + return id(new AphrontRedirectResponse())->setURI($this->getDoneURI()); + } + + return $this->renderConfirmDialog($account); + } + + 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) { + + $dialog = id(new AphrontDialogView()) + ->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.", + $provider->getProviderName())) + ->addCancelButton($this->getDoneURI()); + + return id(new AphrontDialogResponse())->setDialog($dialog); + } + + private function renderLastUsableAccountErrorDialog() { + $dialog = id(new AphrontDialogView()) + ->setUser($this->getRequest()->getUser()) + ->setTitle(pht('Last Valid Account')) + ->appendChild( + pht( + "You can not unlink this account because you have no other ". + "valid login accounts. If you removed it, you would be unable ". + "to login. Add another authentication method before removing ". + "this one.")) + ->addCancelButton($this->getDoneURI()); + + return id(new AphrontDialogResponse())->setDialog($dialog); + } + + private function renderConfirmDialog() { + $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.'); + } + + $dialog = id(new AphrontDialogView()) + ->setUser($this->getRequest()->getUser()) + ->setTitle($title) + ->appendChild($body) + ->addSubmitButton(pht('Unlink Account')) + ->addCancelButton($this->getDoneURI()); + + return id(new AphrontDialogResponse())->setDialog($dialog); + } + +} diff --git a/src/applications/auth/controller/PhabricatorEmailTokenController.php b/src/applications/auth/controller/PhabricatorEmailTokenController.php index 5b4401564b..2f8d51f061 100644 --- a/src/applications/auth/controller/PhabricatorEmailTokenController.php +++ b/src/applications/auth/controller/PhabricatorEmailTokenController.php @@ -75,13 +75,7 @@ final class PhabricatorEmailTokenController $next = '/'; if (!PhabricatorEnv::getEnvConfig('auth.password-auth-enabled')) { - $panels = id(new PhabricatorSettingsPanelOAuth())->buildPanels(); - foreach ($panels as $panel) { - if ($panel->isEnabled()) { - $next = $panel->getPanelURI(); - break; - } - } + $next = '/settings/panel/external/'; } else if (PhabricatorEnv::getEnvConfig('account.editable')) { $next = (string)id(new PhutilURI('/settings/panel/password/')) ->setQueryParams( diff --git a/src/applications/auth/controller/PhabricatorLDAPUnlinkController.php b/src/applications/auth/controller/PhabricatorLDAPUnlinkController.php deleted file mode 100644 index 16a5952d75..0000000000 --- a/src/applications/auth/controller/PhabricatorLDAPUnlinkController.php +++ /dev/null @@ -1,38 +0,0 @@ -getRequest(); - $user = $request->getUser(); - - $ldap_account = id(new PhabricatorExternalAccount())->loadOneWhere( - 'userPHID = %s AND accountType = %s AND accountDomain = %s', - $user->getPHID(), - 'ldap', - 'self'); - - if (!$ldap_account) { - return new Aphront400Response(); - } - - if (!$request->isDialogFormPost()) { - $dialog = new AphrontDialogView(); - $dialog->setUser($user); - $dialog->setTitle(pht('Really unlink account?')); - $dialog->appendChild(phutil_tag('p', array(), pht( - 'You will not be able to login using this account '. - 'once you unlink it. Continue?'))); - $dialog->addSubmitButton(pht('Unlink Account')); - $dialog->addCancelButton('/settings/panel/ldap/'); - - return id(new AphrontDialogResponse())->setDialog($dialog); - } - - $ldap_account->delete(); - - return id(new AphrontRedirectResponse()) - ->setURI('/settings/panel/ldap/'); - } - -} diff --git a/src/applications/auth/controller/PhabricatorOAuthUnlinkController.php b/src/applications/auth/controller/PhabricatorOAuthUnlinkController.php deleted file mode 100644 index 38ab3c12e9..0000000000 --- a/src/applications/auth/controller/PhabricatorOAuthUnlinkController.php +++ /dev/null @@ -1,51 +0,0 @@ -provider = PhabricatorOAuthProvider::newProvider($data['provider']); - } - - public function processRequest() { - $request = $this->getRequest(); - $user = $request->getUser(); - - $provider = $this->provider; - - if ($provider->isProviderLinkPermanent()) { - throw new Exception( - pht("You may not unlink accounts from this OAuth provider.")); - } - - $provider_key = $provider->getProviderKey(); - - $oauth_info = PhabricatorUserOAuthInfo::loadOneByUserAndProviderKey( - $user, - $provider_key); - - if (!$oauth_info) { - return new Aphront400Response(); - } - - if (!$request->isDialogFormPost()) { - $dialog = new AphrontDialogView(); - $dialog->setUser($user); - $dialog->setTitle(pht('Really unlink account?')); - $dialog->appendChild(phutil_tag('p', array(), pht( - 'You will not be able to login using this account '. - 'once you unlink it. Continue?'))); - $dialog->addSubmitButton(pht('Unlink Account')); - $dialog->addCancelButton($provider->getSettingsPanelURI()); - - return id(new AphrontDialogResponse())->setDialog($dialog); - } - - $oauth_info->delete(); - - return id(new AphrontRedirectResponse()) - ->setURI($provider->getSettingsPanelURI()); - } - -} diff --git a/src/applications/auth/oauth/provider/PhabricatorOAuthProvider.php b/src/applications/auth/oauth/provider/PhabricatorOAuthProvider.php index c8cbf1274c..f124e9981b 100644 --- a/src/applications/auth/oauth/provider/PhabricatorOAuthProvider.php +++ b/src/applications/auth/oauth/provider/PhabricatorOAuthProvider.php @@ -23,9 +23,7 @@ abstract class PhabricatorOAuthProvider { abstract public function getTestURIs(); public function getSettingsPanelURI() { - $panel = new PhabricatorSettingsPanelOAuth(); - $panel->setOAuthProvider($this); - return $panel->getPanelURI(); + return '/settings/panel/external/'; } /** diff --git a/src/applications/auth/provider/PhabricatorAuthProvider.php b/src/applications/auth/provider/PhabricatorAuthProvider.php index 25d039992b..72d901b488 100644 --- a/src/applications/auth/provider/PhabricatorAuthProvider.php +++ b/src/applications/auth/provider/PhabricatorAuthProvider.php @@ -6,6 +6,14 @@ abstract class PhabricatorAuthProvider { return $this->getAdapter()->getAdapterKey(); } + public function getProviderType() { + return $this->getAdapter()->getAdapterType(); + } + + public function getProviderDomain() { + return $this->getAdapter()->getAdapterDomain(); + } + public static function getAllProviders() { static $providers; @@ -56,8 +64,7 @@ abstract class PhabricatorAuthProvider { abstract public function getAdapter(); public function isEnabled() { - // TODO: Remove once we switch to the new auth stuff. - return false; + return true; } abstract public function shouldAllowLogin(); @@ -65,12 +72,25 @@ abstract class PhabricatorAuthProvider { abstract public function shouldAllowAccountLink(); abstract public function shouldAllowAccountUnlink(); - abstract public function buildLoginForm( - PhabricatorAuthStartController $controller); + public function buildLoginForm( + PhabricatorAuthStartController $controller) { + return $this->renderLoginForm($controller->getRequest(), $is_link = false); + } abstract public function processLoginRequest( PhabricatorAuthLoginController $controller); + public function buildLinkForm( + PhabricatorAuthLinkController $controller) { + return $this->renderLoginForm($controller->getRequest(), $is_link = true); + } + + protected function renderLoginForm( + AphrontRequest $request, + $is_link) { + throw new Exception("Not implemented!"); + } + public function createProviders() { return array($this); } @@ -144,7 +164,9 @@ abstract class PhabricatorAuthProvider { $this->willSaveAccount($account); - $account->save(); + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $account->save(); + unset($unguarded); return $account; } @@ -155,6 +177,10 @@ abstract class PhabricatorAuthProvider { return PhabricatorEnv::getURI($uri); } + protected function getCancelLinkURI() { + return '/settings/panel/external/'; + } + public function isDefaultRegistrationProvider() { return false; } diff --git a/src/applications/auth/provider/PhabricatorAuthProviderLDAP.php b/src/applications/auth/provider/PhabricatorAuthProviderLDAP.php index e45aa1c085..2778ac10c8 100644 --- a/src/applications/auth/provider/PhabricatorAuthProviderLDAP.php +++ b/src/applications/auth/provider/PhabricatorAuthProviderLDAP.php @@ -49,29 +49,24 @@ final class PhabricatorAuthProviderLDAP } public function shouldAllowAccountLink() { - return false; + return true; } public function shouldAllowAccountUnlink() { - return false; + return true; } - public function buildLoginForm( - PhabricatorAuthStartController $controller) { - - $request = $controller->getRequest(); - return $this->renderLoginForm($request); - } - - private function renderLoginForm(AphrontRequest $request) { - + protected function renderLoginForm(AphrontRequest $request, $is_link) { $viewer = $request->getUser(); $dialog = id(new AphrontDialogView()) ->setSubmitURI($this->getLoginURI()) ->setUser($viewer); - if ($this->shouldAllowRegistration()) { + if ($is_link) { + $dialog->setTitle(pht('Link LDAP Account')); + $dialog->addSubmitButton(pht('Link Accounts')); + } else if ($this->shouldAllowRegistration()) { $dialog->setTitle(pht('Login or Register with LDAP')); $dialog->addSubmitButton(pht('Login or Register')); } else { diff --git a/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php b/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php index 493d0436df..874727fc10 100644 --- a/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php +++ b/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php @@ -40,13 +40,12 @@ abstract class PhabricatorAuthProviderOAuth extends PhabricatorAuthProvider { return true; } - public function buildLoginForm( - PhabricatorAuthStartController $controller) { - - $request = $controller->getRequest(); + protected function renderLoginForm(AphrontRequest $request, $is_link) { $viewer = $request->getUser(); - if ($this->shouldAllowRegistration()) { + if ($is_link) { + $button_text = pht('Link External Account'); + } else if ($this->shouldAllowRegistration()) { $button_text = pht('Login or Register'); } else { $button_text = pht('Login'); @@ -57,7 +56,6 @@ abstract class PhabricatorAuthProviderOAuth extends PhabricatorAuthProvider { ->setSpriteIcon($this->getLoginIcon()); $button = id(new PHUIButtonView()) - ->setTag('a') ->setSize(PHUIButtonView::BIG) ->setColor(PHUIButtonView::GREY) ->setIcon($icon) @@ -91,7 +89,6 @@ abstract class PhabricatorAuthProviderOAuth extends PhabricatorAuthProvider { ), $content); } - public function processLoginRequest( PhabricatorAuthLoginController $controller) { diff --git a/src/applications/auth/provider/PhabricatorAuthProviderPassword.php b/src/applications/auth/provider/PhabricatorAuthProviderPassword.php index 83e043acf8..8c0448061d 100644 --- a/src/applications/auth/provider/PhabricatorAuthProviderPassword.php +++ b/src/applications/auth/provider/PhabricatorAuthProviderPassword.php @@ -51,13 +51,16 @@ final class PhabricatorAuthProviderPassword public function buildLoginForm( PhabricatorAuthStartController $controller) { - $request = $controller->getRequest(); - - return $this->renderLoginForm($request); + return $this->renderPasswordLoginForm($request); } - private function renderLoginForm( + public function buildLinkForm( + PhabricatorAuthLinkController $controller) { + throw new Exception("Password providers can't be linked."); + } + + private function renderPasswordLoginForm( AphrontRequest $request, $require_captcha = false, $captcha_valid = false) { @@ -195,7 +198,7 @@ final class PhabricatorAuthProviderPassword $response = $controller->buildProviderPageResponse( $this, - $this->renderLoginForm( + $this->renderPasswordLoginForm( $request, $require_captcha, $captcha_valid)); diff --git a/src/applications/people/storage/PhabricatorExternalAccount.php b/src/applications/people/storage/PhabricatorExternalAccount.php index 438fa7d898..31cd25287e 100644 --- a/src/applications/people/storage/PhabricatorExternalAccount.php +++ b/src/applications/people/storage/PhabricatorExternalAccount.php @@ -57,4 +57,19 @@ final class PhabricatorExternalAccount extends PhabricatorUserDAO { return idx($this->properties, $key, $default); } + public function isUsableForLogin() { + $key = $this->getProviderKey(); + $provider = PhabricatorAuthProvider::getEnabledProviderByKey($key); + + if (!$provider) { + return false; + } + + if (!$provider->shouldAllowLogin()) { + return false; + } + + return true; + } + } diff --git a/src/applications/settings/panel/PhabricatorSettingsPanelExternalAccounts.php b/src/applications/settings/panel/PhabricatorSettingsPanelExternalAccounts.php new file mode 100644 index 0000000000..a10b651876 --- /dev/null +++ b/src/applications/settings/panel/PhabricatorSettingsPanelExternalAccounts.php @@ -0,0 +1,120 @@ +getUser(); + + $providers = PhabricatorAuthProvider::getAllProviders(); + $accounts = id(new PhabricatorExternalAccount())->loadAllWhere( + 'userPHID = %s', + $viewer->getPHID()); + + $linked_head = id(new PhabricatorHeaderView()) + ->setHeader(pht('Linked Accounts and Authentication')); + + $linked = id(new PhabricatorObjectItemListView()) + ->setUser($viewer) + ->setNoDataString(pht('You have no linked accounts.')); + + $login_accounts = 0; + foreach ($accounts as $account) { + if ($account->isUsableForLogin()) { + $login_accounts++; + } + } + + foreach ($accounts as $account) { + $item = id(new PhabricatorObjectItemView()); + + $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; + } + + $can_login = $account->isUsableForLogin(); + if (!$can_login) { + $item->addAttribute( + pht( + 'Disabled (an administrator has disabled login for this '. + 'account provider).')); + } + + $can_unlink = $can_unlink && (!$can_login || ($login_accounts > 1)); + + $item->addAction( + id(new PHUIListItemView()) + ->setIcon('delete') + ->setWorkflow(true) + ->setDisabled(!$can_unlink) + ->setHref('/auth/unlink/'.$account->getProviderKey().'/')); + + $linked->addItem($item); + } + + $linkable_head = id(new PhabricatorHeaderView()) + ->setHeader(pht('Add External Account')); + + $linkable = id(new PhabricatorObjectItemListView()) + ->setUser($viewer) + ->setNoDataString( + pht('Your account is linked with all available providers.')); + + $accounts = mpull($accounts, null, 'getProviderKey'); + $providers = msort($providers, 'getProviderName'); + foreach ($providers as $key => $provider) { + if (isset($accounts[$key])) { + continue; + } + + if (!$provider->shouldAllowAccountLink()) { + continue; + } + + $link_uri = '/auth/link/'.$provider->getProviderKey().'/'; + + $item = id(new PhabricatorObjectItemView()); + $item->setHeader($provider->getProviderName()); + $item->setHref($link_uri); + $item->addAction( + id(new PHUIListItemView()) + ->setIcon('link') + ->setHref($link_uri)); + + $linkable->addItem($item); + } + + return array( + $linked_head, + $linked, + $linkable_head, + $linkable, + ); + } + +} diff --git a/src/applications/settings/panel/PhabricatorSettingsPanelLDAP.php b/src/applications/settings/panel/PhabricatorSettingsPanelLDAP.php deleted file mode 100644 index 37d82c66cf..0000000000 --- a/src/applications/settings/panel/PhabricatorSettingsPanelLDAP.php +++ /dev/null @@ -1,92 +0,0 @@ -isProviderEnabled(); - } - - public function processRequest(AphrontRequest $request) { - $user = $request->getUser(); - - $ldap_account = id(new PhabricatorExternalAccount())->loadOneWhere( - 'userPHID = %s AND accountType = %s AND accountDomain = %s', - $user->getPHID(), - 'ldap', - 'self'); - - $forms = array(); - - if (!$ldap_account) { - $unlink = pht('Link LDAP Account'); - $unlink_form = new AphrontFormView(); - $unlink_form - ->setUser($user) - ->setAction('/ldap/login/') - ->appendChild(hsprintf( - '

%s

', - pht('There is currently no LDAP account linked to your Phabricator '. - 'account. You can link an account, which will allow you to use it '. - 'to log into Phabricator.'))) - ->appendChild( - id(new AphrontFormTextControl()) - ->setLabel(pht('LDAP username')) - ->setName('username')) - ->appendChild( - id(new AphrontFormPasswordControl()) - ->setLabel(pht('Password')) - ->setName('password')) - ->appendChild( - id(new AphrontFormSubmitControl()) - ->setValue(pht("Link LDAP Account \xC2\xBB"))); - - $forms['Link Account'] = $unlink_form; - } else { - $unlink = pht('Unlink LDAP Account'); - $unlink_form = new AphrontFormView(); - $unlink_form - ->setUser($user) - ->appendChild(hsprintf( - '

%s

', - pht('You may unlink this account from your LDAP account. This will '. - 'prevent you from logging in with your LDAP credentials.'))) - ->appendChild( - id(new AphrontFormSubmitControl()) - ->addCancelButton('/ldap/unlink/', $unlink)); - - $forms['Unlink Account'] = $unlink_form; - } - - $header = new PhabricatorHeaderView(); - $header->setHeader(pht('LDAP Account Settings')); - - $formbox = new PHUIBoxView(); - foreach ($forms as $name => $form) { - if ($name) { - $head = new PhabricatorHeaderView(); - $head->setHeader($name); - $formbox->appendChild($head); - } - $formbox->appendChild($form); - } - - return array( - $header, - $formbox, - ); - } -} diff --git a/src/applications/settings/panel/PhabricatorSettingsPanelOAuth.php b/src/applications/settings/panel/PhabricatorSettingsPanelOAuth.php deleted file mode 100644 index 8143d21865..0000000000 --- a/src/applications/settings/panel/PhabricatorSettingsPanelOAuth.php +++ /dev/null @@ -1,156 +0,0 @@ -provider->getProviderKey(); - } - - public function getPanelName() { - return $this->provider->getProviderName(); - } - - public function getPanelGroup() { - return pht('Linked Accounts'); - } - - public function buildPanels() { - $panels = array(); - - $providers = PhabricatorOAuthProvider::getAllProviders(); - foreach ($providers as $provider) { - $panel = clone $this; - $panel->setOAuthProvider($provider); - $panels[] = $panel; - } - - return $panels; - } - - public function isEnabled() { - return $this->provider->isProviderEnabled(); - } - - private $provider; - - public function setOAuthProvider(PhabricatorOAuthProvider $oauth_provider) { - $this->provider = $oauth_provider; - return $this; - } - - private function prepareAuthForm(AphrontFormView $form) { - $provider = $this->provider; - - $auth_uri = $provider->getAuthURI(); - $client_id = $provider->getClientID(); - $redirect_uri = $provider->getRedirectURI(); - $minimum_scope = $provider->getMinimumScope(); - - $form - ->setAction($auth_uri) - ->setMethod('GET') - ->addHiddenInput('redirect_uri', $redirect_uri) - ->addHiddenInput('client_id', $client_id) - ->addHiddenInput('scope', $minimum_scope); - - foreach ($provider->getExtraAuthParameters() as $key => $value) { - $form->addHiddenInput($key, $value); - } - - return $form; - } - - public function processRequest(AphrontRequest $request) { - $user = $request->getUser(); - $provider = $this->provider; - $notice = null; - $provider_name = $provider->getProviderName(); - $provider_key = $provider->getProviderKey(); - - $oauth_info = PhabricatorUserOAuthInfo::loadOneByUserAndProviderKey( - $user, - $provider_key); - - $form = new AphrontFormView(); - $form->setUser($user); - - $forms = array(); - $forms[] = $form; - if (!$oauth_info) { - $form - ->appendChild(hsprintf( - '

%s

', - pht('There is currently no %s '. - 'account linked to your Phabricator account. You can link an '. - 'account, which will allow you to use it to log into Phabricator.', - $provider_name))); - - $this->prepareAuthForm($form); - - $form - ->appendChild( - id(new AphrontFormSubmitControl()) - ->setValue(pht("Link %s Account \xC2\xBB", $provider_name))); - } else { - $form - ->appendChild(hsprintf( - '

%s

', - pht('Your account is linked with '. - 'a %s account. You may use your %s credentials to log into '. - 'Phabricator.', - $provider_name, - $provider_name))) - ->appendChild( - id(new AphrontFormStaticControl()) - ->setLabel(pht('%s ID', $provider_name)) - ->setValue($oauth_info->getOAuthUID())) - ->appendChild( - id(new AphrontFormStaticControl()) - ->setLabel(pht('%s Name', $provider_name)) - ->setValue($oauth_info->getAccountName())) - ->appendChild( - id(new AphrontFormStaticControl()) - ->setLabel(pht('%s URI', $provider_name)) - ->setValue($oauth_info->getAccountURI())); - - if (!$provider->isProviderLinkPermanent()) { - $unlink = pht('Unlink %s Account', $provider_name); - $unlink_form = new AphrontFormView(); - $unlink_form - ->setUser($user) - ->appendChild(hsprintf( - '

%s

', - pht('You may unlink this account from your %s account. This will '. - 'prevent you from logging in with your %s credentials.', - $provider_name, - $provider_name))) - ->appendChild( - id(new AphrontFormSubmitControl()) - ->addCancelButton('/oauth/'.$provider_key.'/unlink/', $unlink)); - $forms['Unlink Account'] = $unlink_form; - } - } - - $header = new PhabricatorHeaderView(); - $header->setHeader(pht('%s Account Settings', $provider_name)); - - $formbox = new PHUIBoxView(); - foreach ($forms as $name => $form) { - if ($name) { - $head = new PhabricatorHeaderView(); - $head->setHeader($name); - $formbox->appendChild($head); - } - $formbox->appendChild($form); - } - - return id(new AphrontNullView()) - ->appendChild( - array( - $notice, - $header, - $formbox, - )); - } -} diff --git a/src/view/phui/PHUIButtonView.php b/src/view/phui/PHUIButtonView.php index 439dae6964..3970b882f2 100644 --- a/src/view/phui/PHUIButtonView.php +++ b/src/view/phui/PHUIButtonView.php @@ -14,7 +14,7 @@ final class PHUIButtonView extends AphrontTagView { private $text; private $subtext; private $color; - private $tag = 'a'; + private $tag = 'button'; private $dropdown; private $icon; diff --git a/src/view/phui/PHUIListItemView.php b/src/view/phui/PHUIListItemView.php index df24a426ae..fd2b7cd241 100644 --- a/src/view/phui/PHUIListItemView.php +++ b/src/view/phui/PHUIListItemView.php @@ -18,6 +18,7 @@ final class PHUIListItemView extends AphrontTagView { private $icon; private $selected; private $containerAttrs; + private $disabled; public function setSelected($selected) { $this->selected = $selected; @@ -104,6 +105,15 @@ final class PHUIListItemView extends AphrontTagView { ); } + public function setDisabled($disabled) { + $this->disabled = $disabled; + return $this; + } + + public function getDisabled() { + return $this->disabled; + } + protected function getTagContent() { $name = null; $icon = null; @@ -126,10 +136,15 @@ final class PHUIListItemView extends AphrontTagView { } if ($this->icon) { + $icon_name = $this->icon; + if ($this->getDisabled()) { + $icon_name .= '-grey'; + } + $icon = id(new PHUIIconView()) ->addClass('phui-list-item-icon') ->setSpriteSheet(PHUIIconView::SPRITE_ICONS) - ->setSpriteIcon($this->icon); + ->setSpriteIcon($icon_name); } return phutil_tag( diff --git a/webroot/rsrc/css/application/auth/auth.css b/webroot/rsrc/css/application/auth/auth.css index 65952174d2..052ab472a9 100644 --- a/webroot/rsrc/css/application/auth/auth.css +++ b/webroot/rsrc/css/application/auth/auth.css @@ -8,7 +8,7 @@ } .phabricator-login-buttons .phabricator-login-button .button { - width: 180px; + width: 216px; } .device-desktop .phabricator-login-buttons .aphront-multi-column-column-last { @@ -18,3 +18,7 @@ .device .phabricator-login-buttons { text-align: center; } + +.phabricator-link-button { + text-align: center; +}