From c108ada7e4cb9e7f5c5134f5e3f16febf3690e63 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 16 Jun 2013 10:15:16 -0700 Subject: [PATCH] Provide start screen and full registration flow on the new auth stuff Summary: Ref T1536. Code is intentionally made unreachable (see PhabricatorAuthProviderOAuthFacebook->isEnabled()). This adds: - A provider-driven "start" screen (this has the list of ways you can login/register). - Registration actually works. - Facebook OAuth works. @chad, do you have any design ideas on the start screen? I think we poked at it before, but the big issue was that there were a limitless number of providers. Today, we have: - Password - LDAP - Facebook - GitHub - Phabricator - Disqus - Google We plan to add: - Asana - An arbitrary number of additional instances of Phabricator Users want to add: - OpenID - Custom providers And I'd like to have these at some point: - Stripe - WePay - Amazon - Bitbucket So basically any UI for this has to accommodate 300 zillion auth options. I don't think we need to solve any UX problems here (realistically, installs enable 1-2 auth options and users don't actually face an overwhelming number of choices) but making the login forms less ugly would be nice. No combination of prebuilt elements seems to look very good for this use case. Test Plan: Registered a new acount with Facebook. Reviewers: btrahan, chad Reviewed By: btrahan CC: aran Maniphest Tasks: T1536 Differential Revision: https://secure.phabricator.com/D6161 --- src/__phutil_library_map__.php | 6 + .../PhabricatorApplicationAuth.php | 5 + .../PhabricatorAuthStartController.php | 157 ++++++++++++++++++ .../auth/provider/PhabricatorAuthProvider.php | 43 ++++- .../provider/PhabricatorAuthProviderOAuth.php | 73 ++++++++ .../PhabricatorAuthProviderOAuthFacebook.php | 51 ++++++ .../storage/PhabricatorExternalAccount.php | 2 +- 7 files changed, 327 insertions(+), 10 deletions(-) create mode 100644 src/applications/auth/controller/PhabricatorAuthStartController.php create mode 100644 src/applications/auth/provider/PhabricatorAuthProviderOAuthFacebook.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f6f7ff9354..6d984f7488 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -817,7 +817,10 @@ phutil_register_library_map(array( 'PhabricatorAuthController' => 'applications/auth/controller/PhabricatorAuthController.php', 'PhabricatorAuthLoginController' => 'applications/auth/controller/PhabricatorAuthLoginController.php', 'PhabricatorAuthProvider' => 'applications/auth/provider/PhabricatorAuthProvider.php', + 'PhabricatorAuthProviderOAuth' => 'applications/auth/provider/PhabricatorAuthProviderOAuth.php', + 'PhabricatorAuthProviderOAuthFacebook' => 'applications/auth/provider/PhabricatorAuthProviderOAuthFacebook.php', 'PhabricatorAuthRegisterController' => 'applications/auth/controller/PhabricatorAuthRegisterController.php', + 'PhabricatorAuthStartController' => 'applications/auth/controller/PhabricatorAuthStartController.php', 'PhabricatorAuthenticationConfigOptions' => 'applications/config/option/PhabricatorAuthenticationConfigOptions.php', 'PhabricatorBarePageExample' => 'applications/uiexample/examples/PhabricatorBarePageExample.php', 'PhabricatorBarePageView' => 'view/page/PhabricatorBarePageView.php', @@ -2672,7 +2675,10 @@ phutil_register_library_map(array( 'PhabricatorAuditReplyHandler' => 'PhabricatorMailReplyHandler', 'PhabricatorAuthController' => 'PhabricatorController', 'PhabricatorAuthLoginController' => 'PhabricatorAuthController', + 'PhabricatorAuthProviderOAuth' => 'PhabricatorAuthProvider', + 'PhabricatorAuthProviderOAuthFacebook' => 'PhabricatorAuthProviderOAuth', 'PhabricatorAuthRegisterController' => 'PhabricatorAuthController', + 'PhabricatorAuthStartController' => 'PhabricatorAuthController', 'PhabricatorAuthenticationConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorBarePageExample' => 'PhabricatorUIExample', 'PhabricatorBarePageView' => 'AphrontPageView', diff --git a/src/applications/auth/application/PhabricatorApplicationAuth.php b/src/applications/auth/application/PhabricatorApplicationAuth.php index 253bddd997..6b5618151a 100644 --- a/src/applications/auth/application/PhabricatorApplicationAuth.php +++ b/src/applications/auth/application/PhabricatorApplicationAuth.php @@ -10,6 +10,10 @@ final class PhabricatorApplicationAuth extends PhabricatorApplication { return false; } + public function getBaseURI() { + return '/auth/'; + } + public function buildMainMenuItems( PhabricatorUser $user, PhabricatorController $controller = null) { @@ -34,6 +38,7 @@ final class PhabricatorApplicationAuth extends PhabricatorApplication { '/auth/' => array( 'login/(?P[^/]+)/' => 'PhabricatorAuthLoginController', 'register/(?P[^/]+)/' => 'PhabricatorAuthRegisterController', + 'start/' => 'PhabricatorAuthStartController', ), ); } diff --git a/src/applications/auth/controller/PhabricatorAuthStartController.php b/src/applications/auth/controller/PhabricatorAuthStartController.php new file mode 100644 index 0000000000..5796ad1807 --- /dev/null +++ b/src/applications/auth/controller/PhabricatorAuthStartController.php @@ -0,0 +1,157 @@ +getRequest(); + $viewer = $request->getUser(); + + if ($viewer->isLoggedIn()) { + // Kick the user home if they are already logged in. + return id(new AphrontRedirectResponse())->setURI('/'); + } + + if ($request->isAjax()) { + return $this->processAjaxRequest(); + } + + if ($request->isConduit()) { + return $this->processConduitRequest(); + } + + if ($request->getCookie('phusr') && $request->getCookie('phsid')) { + // The session cookie is invalid, so clear it. + $request->clearCookie('phusr'); + $request->clearCookie('phsid'); + + return $this->renderError( + pht( + "Your login session is invalid. Try reloading the page and logging ". + "in again. If that does not work, clear your browser cookies.")); + } + + + $providers = PhabricatorAuthProvider::getAllEnabledProviders(); + foreach ($providers as $key => $provider) { + if (!$provider->shouldAllowLogin()) { + unset($providers[$key]); + } + } + + if (!$providers) { + return $this->renderError( + pht( + "This Phabricator install is not configured with any enabled ". + "authentication providers which can be used to log in.")); + } + + $next_uri = $request->getStr('next'); + if (!$next_uri) { + $next_uri_path = $this->getRequest()->getPath(); + if ($next_uri_path == '/auth/start/') { + $next_uri = '/'; + } else { + $next_uri = $this->getRequest()->getRequestURI(); + } + } + + if (!$request->isFormPost()) { + $request->setCookie('next_uri', $next_uri); + } + + $out = array(); + foreach ($providers as $provider) { + $out[] = $provider->buildLoginForm($this); + } + + $login_message = PhabricatorEnv::getEnvConfig('auth.login-message'); + $login_message = phutil_safe_html($login_message); + + $crumbs = $this->buildApplicationCrumbs(); + $crumbs->addCrumb( + id(new PhabricatorCrumbView()) + ->setName(pht('Login'))); + + return $this->buildApplicationPage( + array( + $crumbs, + $login_message, + $out, + ), + array( + 'title' => pht('Login to Phabricator'), + 'device' => true, + 'dust' => true, + )); + } + + + private function processAjaxRequest() { + $request = $this->getRequest(); + $viewer = $request->getViewer(); + + // We end up here if the user clicks a workflow link that they need to + // login to use. We give them a dialog saying "You need to login...". + + if ($request->isDialogFormPost()) { + return id(new AphrontRedirectResponse())->setURI( + $request->getRequestURI()); + } + + $dialog = new AphrontDialogView(); + $dialog->setUser($viewer); + $dialog->setTitle(pht('Login Required')); + $dialog->appendChild(pht('You must login to continue.')); + $dialog->addSubmitButton(pht('Login')); + $dialog->addCancelButton('/'); + + return id(new AphrontDialogResponse())->setDialog($dialog); + } + + + private function processConduitRequest() { + $request = $this->getRequest(); + $viewer = $request->getViewer(); + + // A common source of errors in Conduit client configuration is getting + // the request path wrong. The client will end up here, so make some + // effort to give them a comprehensible error message. + + $request_path = $this->getRequest()->getPath(); + $conduit_path = '/api/'; + $example_path = '/api/conduit.ping'; + + $message = pht( + 'ERROR: You are making a Conduit API request to "%s", but the correct '. + 'HTTP request path to use in order to access a COnduit method is "%s" '. + '(for example, "%s"). Check your configuration.', + $request_path, + $conduit_path, + $example_path); + + return id(new AphrontPlainTextResponse())->setContent($message); + } + + private function renderError($message) { + $title = pht('Authentication Failure'); + + $view = new AphrontErrorView(); + $view->setTitle($title); + $view->appendChild($message); + + return $this->buildApplicationPage( + $view, + array( + 'title' => $title, + 'device' => true, + 'dust' => true, + )); + } + + +} diff --git a/src/applications/auth/provider/PhabricatorAuthProvider.php b/src/applications/auth/provider/PhabricatorAuthProvider.php index 3169bc2678..86fa4071dd 100644 --- a/src/applications/auth/provider/PhabricatorAuthProvider.php +++ b/src/applications/auth/provider/PhabricatorAuthProvider.php @@ -38,7 +38,7 @@ abstract class PhabricatorAuthProvider { return $providers; } - public static function getEnabledProviders() { + public static function getAllEnabledProviders() { $providers = self::getAllProviders(); foreach ($providers as $key => $provider) { if (!$provider->isEnabled()) { @@ -49,15 +49,20 @@ abstract class PhabricatorAuthProvider { } public static function getEnabledProviderByKey($provider_key) { - return idx(self::getEnabledProviders(), $provider_key); + return idx(self::getAllEnabledProviders(), $provider_key); } abstract public function getProviderName(); - abstract public function getAdapater(); + abstract public function getAdapter(); abstract public function isEnabled(); abstract public function shouldAllowLogin(); abstract public function shouldAllowRegistration(); abstract public function shouldAllowAccountLink(); + abstract public function shouldAllowAccountUnlink(); + + abstract public function buildLoginForm( + PhabricatorAuthStartController $controller); + abstract public function processLoginRequest( PhabricatorAuthLoginController $controller); @@ -71,22 +76,38 @@ abstract class PhabricatorAuthProvider { protected function loadOrCreateAccount($account_id) { if (!strlen($account_id)) { - throw new Exception("loadOrCreateAccount(...): empty account ID!"); + throw new Exception( + "loadOrCreateAccount(...): empty account ID!"); } $adapter = $this->getAdapter(); + $adapter_class = get_class($adapter); + + if (!strlen($adapter->getAdapterType())) { + throw new Exception( + "AuthAdapter (of class '{$adapter_class}') has an invalid ". + "implementation: no adapter type."); + } + + if (!strlen($adapter->getAdapterDomain())) { + throw new Exception( + "AuthAdapter (of class '{$adapter_class}') has an invalid ". + "implementation: no adapter domain."); + } + $account = id(new PhabricatorExternalAccount())->loadOneWhere( 'accountType = %s AND accountDomain = %s AND accountID = %s', - $adapter->getProviderType(), - $adapter->getProviderDomain(), + $adapter->getAdapterType(), + $adapter->getAdapterDomain(), $account_id); if (!$account) { $account = id(new PhabricatorExternalAccount()) - ->setAccountType($adapter->getProviderType()) - ->setAccountDomain($adapter->getProviderDomain()) + ->setAccountType($adapter->getAdapterType()) + ->setAccountDomain($adapter->getAdapterDomain()) ->setAccountID($account_id); } + $account->setDisplayName(''); $account->setUsername($adapter->getAccountName()); $account->setRealName($adapter->getAccountRealName()); $account->setEmail($adapter->getAccountEmail()); @@ -120,6 +141,10 @@ abstract class PhabricatorAuthProvider { return $account; } - + protected function getLoginURI() { + $app = PhabricatorApplication::getByClass('PhabricatorApplicationAuth'); + $uri = $app->getApplicationURI('/login/'.$this->getProviderKey().'/'); + return PhabricatorEnv::getURI($uri); + } } diff --git a/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php b/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php index d4fbd08f49..0ee70aa8a9 100644 --- a/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php +++ b/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php @@ -2,6 +2,79 @@ abstract class PhabricatorAuthProviderOAuth extends PhabricatorAuthProvider { + protected $adapter; + + abstract protected function getOAuthClientID(); + abstract protected function getOAuthClientSecret(); + abstract protected function newOAuthAdapter(); + + public function getAdapter() { + if (!$this->adapter) { + $adapter = $this->newOAuthAdapter(); + $this->adapter = $adapter; + $this->configureAdapter($adapter); + } + return $this->adapter; + } + + public function isEnabled() { + return $this->getOAuthClientID() && $this->getOAuthClientSecret(); + } + + protected function configureAdapter(PhutilAuthAdapterOAuth $adapter) { + $adapter->setClientID($this->getOAuthClientID()); + $adapter->setClientSecret($this->getOAuthClientSecret()); + $adapter->setRedirectURI($this->getLoginURI()); + return $adapter; + } + + public function buildLoginForm( + PhabricatorAuthStartController $controller) { + + $request = $controller->getRequest(); + $viewer = $request->getUser(); + + $form = id(new AphrontFormView()) + ->setUser($viewer); + + $submit = new AphrontFormSubmitControl(); + + if ($this->shouldAllowRegistration()) { + $submit->setValue( + pht("Login or Register with %s \xC2\xBB", $this->getProviderName())); + $header = pht("Login or Register with %s", $this->getProviderName()); + } else { + $submit->setValue( + pht("Login with %s \xC2\xBB", $this->getProviderName())); + $header = pht("Login with %s", $this->getProviderName()); + } + + $form->appendChild($submit); + + // TODO: This is pretty hideous. + $panel = new AphrontPanelView(); + $panel->setHeader($header); + $panel->setWidth(AphrontPanelView::WIDTH_FORM); + $panel->setNoBackground(true); + $panel->appendChild($form); + + $adapter = $this->getAdapter(); + + + $uri = new PhutilURI($adapter->getAuthenticateURI()); + $params = $uri->getQueryParams(); + $uri->setQueryParams(array()); + + $form->setAction((string)$uri); + foreach ($params as $key => $value) { + $form->addHiddenInput($key, $value); + } + + $form->setMethod('GET'); + + return $panel; + } + public function processLoginRequest( PhabricatorAuthLoginController $controller) { diff --git a/src/applications/auth/provider/PhabricatorAuthProviderOAuthFacebook.php b/src/applications/auth/provider/PhabricatorAuthProviderOAuthFacebook.php new file mode 100644 index 0000000000..2dd1795015 --- /dev/null +++ b/src/applications/auth/provider/PhabricatorAuthProviderOAuthFacebook.php @@ -0,0 +1,51 @@ +getAccountType().':'.$this->accountDomain(); + return $this->getAccountType().':'.$this->getAccountDomain(); } public function save() {