From e1a9473eda04bd76da1c96814727ec404a1d284e Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 6 Jun 2016 13:04:21 -0700 Subject: [PATCH] Make auth provider autologin modular and implement it for all OAuth2 adapters Summary: Ref T10785. Around the time we launched Phacility SAAS we implemented this weird autologin hack. It works fine, so clean it up, get rid of the `instanceof` stuff, and support it for any OAuth2 provider. (We could conceivably support OAuth1 as well, but no one has expressed an interest in it and I don't think I have any OAuth1 providers configured correctly locally so it would take a little bit to set up and test.) Test Plan: - Configured OAuth2 adapters (Facebook) for auto-login. - Saw no config option on other adapters (LDAP). - Nuked all options but one, did autologin with Facebook and Phabricator. - Logged out, got logout screen. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10785 Differential Revision: https://secure.phabricator.com/D16060 --- .../PhabricatorAuthStartController.php | 45 ++++++++++++++----- .../config/PhabricatorAuthEditController.php | 4 +- .../auth/provider/PhabricatorAuthProvider.php | 8 ++++ .../PhabricatorOAuth2AuthProvider.php | 13 ++++++ 4 files changed, 57 insertions(+), 13 deletions(-) diff --git a/src/applications/auth/controller/PhabricatorAuthStartController.php b/src/applications/auth/controller/PhabricatorAuthStartController.php index 633339a356..982252d29f 100644 --- a/src/applications/auth/controller/PhabricatorAuthStartController.php +++ b/src/applications/auth/controller/PhabricatorAuthStartController.php @@ -113,17 +113,9 @@ final class PhabricatorAuthStartController PhabricatorCookies::setClientIDCookie($request); } - if (!$request->getURIData('loggedout') && count($providers) == 1) { - $auto_login_provider = head($providers); - $auto_login_config = $auto_login_provider->getProviderConfig(); - if ($auto_login_provider instanceof PhabricatorPhabricatorAuthProvider && - $auto_login_config->getShouldAutoLogin()) { - $auto_login_adapter = $provider->getAdapter(); - $auto_login_adapter->setState($provider->getAuthCSRFCode($request)); - return id(new AphrontRedirectResponse()) - ->setIsExternal(true) - ->setURI($provider->getAdapter()->getAuthenticateURI()); - } + $auto_response = $this->tryAutoLogin($providers); + if ($auto_response) { + return $auto_response; } $invite = $this->loadInvite(); @@ -282,4 +274,35 @@ final class PhabricatorAuthStartController array($message)); } + private function tryAutoLogin(array $providers) { + $request = $this->getRequest(); + + // If the user just logged out, don't immediately log them in again. + if ($request->getURIData('loggedout')) { + return null; + } + + // If we have more than one provider, we can't autologin because we + // don't know which one the user wants. + if (count($providers) != 1) { + return null; + } + + $provider = head($providers); + if (!$provider->supportsAutoLogin()) { + return null; + } + + $config = $provider->getProviderConfig(); + if (!$config->getShouldAutoLogin()) { + return null; + } + + $auto_uri = $provider->getAutoLoginURI($request); + + return id(new AphrontRedirectResponse()) + ->setIsExternal(true) + ->setURI($auto_uri); + } + } diff --git a/src/applications/auth/controller/config/PhabricatorAuthEditController.php b/src/applications/auth/controller/config/PhabricatorAuthEditController.php index 049edfacef..ec2941a3fb 100644 --- a/src/applications/auth/controller/config/PhabricatorAuthEditController.php +++ b/src/applications/auth/controller/config/PhabricatorAuthEditController.php @@ -130,7 +130,7 @@ final class PhabricatorAuthEditController PhabricatorAuthProviderConfigTransaction::TYPE_TRUST_EMAILS) ->setNewValue($request->getInt('trustEmails', 0)); - if ($provider instanceof PhabricatorPhabricatorAuthProvider) { + if ($provider->supportsAutoLogin()) { $xactions[] = id(new PhabricatorAuthProviderConfigTransaction()) ->setTransactionType( PhabricatorAuthProviderConfigTransaction::TYPE_AUTO_LOGIN) @@ -314,7 +314,7 @@ final class PhabricatorAuthEditController $v_trust_email)); } - if ($provider instanceof PhabricatorPhabricatorAuthProvider) { + if ($provider->supportsAutoLogin()) { $form->appendChild( id(new AphrontFormCheckboxControl()) ->addCheckbox( diff --git a/src/applications/auth/provider/PhabricatorAuthProvider.php b/src/applications/auth/provider/PhabricatorAuthProvider.php index 9484109943..c949764c9a 100644 --- a/src/applications/auth/provider/PhabricatorAuthProvider.php +++ b/src/applications/auth/provider/PhabricatorAuthProvider.php @@ -495,4 +495,12 @@ abstract class PhabricatorAuthProvider extends Phobject { } } + public function supportsAutoLogin() { + return false; + } + + public function getAutoLoginURI(AphrontRequest $request) { + throw new PhutilMethodNotImplementedException(); + } + } diff --git a/src/applications/auth/provider/PhabricatorOAuth2AuthProvider.php b/src/applications/auth/provider/PhabricatorOAuth2AuthProvider.php index aff6367840..a3300126af 100644 --- a/src/applications/auth/provider/PhabricatorOAuth2AuthProvider.php +++ b/src/applications/auth/provider/PhabricatorOAuth2AuthProvider.php @@ -273,4 +273,17 @@ abstract class PhabricatorOAuth2AuthProvider parent::willRenderLinkedAccount($viewer, $item, $account); } + public function supportsAutoLogin() { + return true; + } + + public function getAutoLoginURI(AphrontRequest $request) { + $csrf_code = $this->getAuthCSRFCode($request); + + $adapter = $this->getAdapter(); + $adapter->setState($csrf_code); + + return $adapter->getAuthenticateURI(); + } + }