From 472f316bbd907e01c3c0e6b7e758f9c8b759c362 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Fri, 6 Feb 2015 10:50:36 -0800 Subject: [PATCH] Auth - allow for "auto login" providers Summary: Ref T7153. I am not sure if this is 100% correct because sometimes you have to POST vs GET and I don't know if the redirect response will / can do the right thing? I think options to fix this would be to 1) restrict this functionality to JUST the Phabricator OAuth provider type or 2) something really fancy with an HTTP(S) future. The other rub right now is when you logout you get half auto-logged in again... Thoughts on that? Test Plan: setup my local instance to JUST have phabricator oauth available to login. was presented with the dialog automagically...! Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin, epriestley Maniphest Tasks: T7153 Differential Revision: https://secure.phabricator.com/D11701 --- .../20150205.authprovider.autologin.sql | 2 ++ .../PhabricatorAuthApplication.php | 1 + .../PhabricatorAuthStartController.php | 16 +++++++++++-- .../PhabricatorLogoutController.php | 4 ++-- .../config/PhabricatorAuthEditController.php | 24 +++++++++++++++++++ .../PhabricatorAuthProviderConfigEditor.php | 7 ++++++ .../auth/provider/PhabricatorAuthProvider.php | 2 +- .../storage/PhabricatorAuthProviderConfig.php | 2 ++ ...abricatorAuthProviderConfigTransaction.php | 12 ++++++++++ 9 files changed, 65 insertions(+), 5 deletions(-) create mode 100644 resources/sql/autopatches/20150205.authprovider.autologin.sql diff --git a/resources/sql/autopatches/20150205.authprovider.autologin.sql b/resources/sql/autopatches/20150205.authprovider.autologin.sql new file mode 100644 index 0000000000..7fb0bbf255 --- /dev/null +++ b/resources/sql/autopatches/20150205.authprovider.autologin.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_auth.auth_providerconfig + ADD shouldAutoLogin TINYINT(1) NOT NULL DEFAULT '0'; diff --git a/src/applications/auth/application/PhabricatorAuthApplication.php b/src/applications/auth/application/PhabricatorAuthApplication.php index 70d1966fc2..181f7986b8 100644 --- a/src/applications/auth/application/PhabricatorAuthApplication.php +++ b/src/applications/auth/application/PhabricatorAuthApplication.php @@ -97,6 +97,7 @@ final class PhabricatorAuthApplication extends PhabricatorApplication { ), 'login/(?P[^/]+)/(?:(?P[^/]+)/)?' => 'PhabricatorAuthLoginController', + '(?Ploggedout)/' => 'PhabricatorAuthStartController', 'register/(?:(?P[^/]+)/)?' => 'PhabricatorAuthRegisterController', 'start/' => 'PhabricatorAuthStartController', 'validate/' => 'PhabricatorAuthValidateController', diff --git a/src/applications/auth/controller/PhabricatorAuthStartController.php b/src/applications/auth/controller/PhabricatorAuthStartController.php index c2b8c918ab..34ea44030d 100644 --- a/src/applications/auth/controller/PhabricatorAuthStartController.php +++ b/src/applications/auth/controller/PhabricatorAuthStartController.php @@ -7,8 +7,7 @@ final class PhabricatorAuthStartController return false; } - public function processRequest() { - $request = $this->getRequest(); + public function handleRequest(AphrontRequest $request) { $viewer = $request->getUser(); if ($viewer->isLoggedIn()) { @@ -97,6 +96,19 @@ 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()); + } + } + $not_buttons = array(); $are_buttons = array(); $providers = msort($providers, 'getLoginOrder'); diff --git a/src/applications/auth/controller/PhabricatorLogoutController.php b/src/applications/auth/controller/PhabricatorLogoutController.php index c7543991c9..cc9c63770c 100644 --- a/src/applications/auth/controller/PhabricatorLogoutController.php +++ b/src/applications/auth/controller/PhabricatorLogoutController.php @@ -21,7 +21,7 @@ final class PhabricatorLogoutController return true; } - public function processRequest() { + public function handleRequest(AphrontRequest $request) { $request = $this->getRequest(); $user = $request->getUser(); @@ -49,7 +49,7 @@ final class PhabricatorLogoutController $request->clearCookie(PhabricatorCookies::COOKIE_SESSION); return id(new AphrontRedirectResponse()) - ->setURI('/login/'); + ->setURI('/auth/loggedout/'); } if ($user->getPHID()) { diff --git a/src/applications/auth/controller/config/PhabricatorAuthEditController.php b/src/applications/auth/controller/config/PhabricatorAuthEditController.php index 15927bb518..09742c3726 100644 --- a/src/applications/auth/controller/config/PhabricatorAuthEditController.php +++ b/src/applications/auth/controller/config/PhabricatorAuthEditController.php @@ -83,6 +83,7 @@ final class PhabricatorAuthEditController $v_link = $config->getShouldAllowLink(); $v_unlink = $config->getShouldAllowUnlink(); $v_trust_email = $config->getShouldTrustEmails(); + $v_auto_login = $config->getShouldAutoLogin(); if ($request->isFormPost()) { @@ -123,6 +124,13 @@ final class PhabricatorAuthEditController PhabricatorAuthProviderConfigTransaction::TYPE_TRUST_EMAILS) ->setNewValue($request->getInt('trustEmails', 0)); + if ($provider instanceof PhabricatorPhabricatorAuthProvider) { + $xactions[] = id(new PhabricatorAuthProviderConfigTransaction()) + ->setTransactionType( + PhabricatorAuthProviderConfigTransaction::TYPE_AUTO_LOGIN) + ->setNewValue($request->getInt('autoLogin', 0)); + } + foreach ($properties as $key => $value) { $xactions[] = id(new PhabricatorAuthProviderConfigTransaction()) ->setTransactionType( @@ -224,6 +232,12 @@ final class PhabricatorAuthEditController pht( 'Phabricator will skip email verification for accounts registered '. 'through this provider.')); + $str_auto_login = hsprintf( + '%s: %s', + pht('Allow Auto Login'), + pht( + 'Phabricator will automatically login with this provider if it is '. + 'the only available provider.')); $status_tag = id(new PHUITagView()) ->setType(PHUITagView::TYPE_STATE); @@ -285,6 +299,16 @@ final class PhabricatorAuthEditController $v_trust_email)); } + if ($provider instanceof PhabricatorPhabricatorAuthProvider) { + $form->appendChild( + id(new AphrontFormCheckboxControl()) + ->addCheckbox( + 'autoLogin', + 1, + $str_auto_login, + $v_auto_login)); + } + $provider->extendEditForm($request, $form, $properties, $issues); $form diff --git a/src/applications/auth/editor/PhabricatorAuthProviderConfigEditor.php b/src/applications/auth/editor/PhabricatorAuthProviderConfigEditor.php index 684b6cc13d..cdb021678a 100644 --- a/src/applications/auth/editor/PhabricatorAuthProviderConfigEditor.php +++ b/src/applications/auth/editor/PhabricatorAuthProviderConfigEditor.php @@ -19,6 +19,7 @@ final class PhabricatorAuthProviderConfigEditor $types[] = PhabricatorAuthProviderConfigTransaction::TYPE_LINK; $types[] = PhabricatorAuthProviderConfigTransaction::TYPE_UNLINK; $types[] = PhabricatorAuthProviderConfigTransaction::TYPE_TRUST_EMAILS; + $types[] = PhabricatorAuthProviderConfigTransaction::TYPE_AUTO_LOGIN; $types[] = PhabricatorAuthProviderConfigTransaction::TYPE_PROPERTY; return $types; @@ -43,6 +44,8 @@ final class PhabricatorAuthProviderConfigEditor return (int)$object->getShouldAllowUnlink(); case PhabricatorAuthProviderConfigTransaction::TYPE_TRUST_EMAILS: return (int)$object->getShouldTrustEmails(); + case PhabricatorAuthProviderConfigTransaction::TYPE_AUTO_LOGIN: + return (int)$object->getShouldAutoLogin(); case PhabricatorAuthProviderConfigTransaction::TYPE_PROPERTY: $key = $xaction->getMetadataValue( PhabricatorAuthProviderConfigTransaction::PROPERTY_KEY); @@ -60,6 +63,7 @@ final class PhabricatorAuthProviderConfigEditor case PhabricatorAuthProviderConfigTransaction::TYPE_LINK: case PhabricatorAuthProviderConfigTransaction::TYPE_UNLINK: case PhabricatorAuthProviderConfigTransaction::TYPE_TRUST_EMAILS: + case PhabricatorAuthProviderConfigTransaction::TYPE_AUTO_LOGIN: case PhabricatorAuthProviderConfigTransaction::TYPE_PROPERTY: return $xaction->getNewValue(); } @@ -80,6 +84,8 @@ final class PhabricatorAuthProviderConfigEditor return $object->setShouldAllowUnlink($v); case PhabricatorAuthProviderConfigTransaction::TYPE_TRUST_EMAILS: return $object->setShouldTrustEmails($v); + case PhabricatorAuthProviderConfigTransaction::TYPE_AUTO_LOGIN: + return $object->setShouldAutoLogin($v); case PhabricatorAuthProviderConfigTransaction::TYPE_PROPERTY: $key = $xaction->getMetadataValue( PhabricatorAuthProviderConfigTransaction::PROPERTY_KEY); @@ -104,6 +110,7 @@ final class PhabricatorAuthProviderConfigEditor case PhabricatorAuthProviderConfigTransaction::TYPE_LINK: case PhabricatorAuthProviderConfigTransaction::TYPE_UNLINK: case PhabricatorAuthProviderConfigTransaction::TYPE_TRUST_EMAILS: + case PhabricatorAuthProviderConfigTransaction::TYPE_AUTO_LOGIN: // For these types, last transaction wins. return $v; } diff --git a/src/applications/auth/provider/PhabricatorAuthProvider.php b/src/applications/auth/provider/PhabricatorAuthProvider.php index 6c34babacb..0745fd2afc 100644 --- a/src/applications/auth/provider/PhabricatorAuthProvider.php +++ b/src/applications/auth/provider/PhabricatorAuthProvider.php @@ -449,7 +449,7 @@ abstract class PhabricatorAuthProvider { return null; } - protected function getAuthCSRFCode(AphrontRequest $request) { + public function getAuthCSRFCode(AphrontRequest $request) { $phcid = $request->getCookie(PhabricatorCookies::COOKIE_CLIENTID); if (!strlen($phcid)) { throw new Exception( diff --git a/src/applications/auth/storage/PhabricatorAuthProviderConfig.php b/src/applications/auth/storage/PhabricatorAuthProviderConfig.php index cced9a0cc7..14d68b352b 100644 --- a/src/applications/auth/storage/PhabricatorAuthProviderConfig.php +++ b/src/applications/auth/storage/PhabricatorAuthProviderConfig.php @@ -16,6 +16,7 @@ final class PhabricatorAuthProviderConfig protected $shouldAllowLink = 0; protected $shouldAllowUnlink = 0; protected $shouldTrustEmails = 0; + protected $shouldAutoLogin = 0; protected $properties = array(); @@ -42,6 +43,7 @@ final class PhabricatorAuthProviderConfig 'shouldAllowLink' => 'bool', 'shouldAllowUnlink' => 'bool', 'shouldTrustEmails' => 'bool', + 'shouldAutoLogin' => 'bool', ), self::CONFIG_KEY_SCHEMA => array( 'key_provider' => array( diff --git a/src/applications/auth/storage/PhabricatorAuthProviderConfigTransaction.php b/src/applications/auth/storage/PhabricatorAuthProviderConfigTransaction.php index 1dfdf033b7..a0be28f3a9 100644 --- a/src/applications/auth/storage/PhabricatorAuthProviderConfigTransaction.php +++ b/src/applications/auth/storage/PhabricatorAuthProviderConfigTransaction.php @@ -8,6 +8,7 @@ final class PhabricatorAuthProviderConfigTransaction const TYPE_LINK = 'config:link'; const TYPE_UNLINK = 'config:unlink'; const TYPE_TRUST_EMAILS = 'config:trustEmails'; + const TYPE_AUTO_LOGIN = 'config:autoLogin'; const TYPE_PROPERTY = 'config:property'; const PROPERTY_KEY = 'auth:property'; @@ -133,6 +134,17 @@ final class PhabricatorAuthProviderConfigTransaction $this->renderHandleLink($author_phid)); } break; + case self::TYPE_AUTO_LOGIN: + if ($new) { + return pht( + '%s enabled auto login.', + $this->renderHandleLink($author_phid)); + } else { + return pht( + '%s disabled auto login.', + $this->renderHandleLink($author_phid)); + } + break; case self::TYPE_PROPERTY: $provider = $this->getProvider(); if ($provider) {