From 50376aad04d2f25f84a18ecfcad59a85fb1800b0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 1 May 2014 10:23:02 -0700 Subject: [PATCH] Require multiple auth factors to establish web sessions Summary: Ref T4398. This prompts users for multi-factor auth on login. Roughly, this introduces the idea of "partial" sessions, which we haven't finished constructing yet. In practice, this means the session has made it through primary auth but not through multi-factor auth. Add a workflow for bringing a partial session up to a full one. Test Plan: - Used Conduit. - Logged in as multi-factor user. - Logged in as no-factor user. - Tried to do non-login-things with a partial session. - Reviewed account activity logs. {F149295} Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4398 Differential Revision: https://secure.phabricator.com/D8922 --- .../autopatches/20140430.auth.1.partial.sql | 2 + src/__phutil_library_map__.php | 2 + src/aphront/console/DarkConsoleController.php | 4 ++ .../console/DarkConsoleDataController.php | 4 ++ .../PhabricatorApplicationAuth.php | 1 + .../controller/PhabricatorAuthController.php | 2 +- .../PhabricatorAuthFinishController.php | 71 +++++++++++++++++++ .../PhabricatorAuthValidateController.php | 14 ++-- .../PhabricatorLogoutController.php | 4 ++ .../engine/PhabricatorAuthSessionEngine.php | 71 ++++++++++++++++--- .../auth/storage/PhabricatorAuthSession.php | 1 + .../base/controller/PhabricatorController.php | 23 ++++-- .../ConduitAPI_conduit_connect_Method.php | 8 +-- .../people/storage/PhabricatorUserLog.php | 6 +- .../celerity/CelerityResourceController.php | 4 ++ 15 files changed, 190 insertions(+), 27 deletions(-) create mode 100644 resources/sql/autopatches/20140430.auth.1.partial.sql create mode 100644 src/applications/auth/controller/PhabricatorAuthFinishController.php diff --git a/resources/sql/autopatches/20140430.auth.1.partial.sql b/resources/sql/autopatches/20140430.auth.1.partial.sql new file mode 100644 index 0000000000..7f104ac8d6 --- /dev/null +++ b/resources/sql/autopatches/20140430.auth.1.partial.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_user.phabricator_session + ADD isPartial BOOL NOT NULL DEFAULT 0; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 877553e5ab..56b8535eed 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1213,6 +1213,7 @@ phutil_register_library_map(array( 'PhabricatorAuthFactorConfig' => 'applications/auth/storage/PhabricatorAuthFactorConfig.php', 'PhabricatorAuthFactorTOTP' => 'applications/auth/factor/PhabricatorAuthFactorTOTP.php', 'PhabricatorAuthFactorTOTPTestCase' => 'applications/auth/factor/__tests__/PhabricatorAuthFactorTOTPTestCase.php', + 'PhabricatorAuthFinishController' => 'applications/auth/controller/PhabricatorAuthFinishController.php', 'PhabricatorAuthHighSecurityRequiredException' => 'applications/auth/exception/PhabricatorAuthHighSecurityRequiredException.php', 'PhabricatorAuthHighSecurityToken' => 'applications/auth/data/PhabricatorAuthHighSecurityToken.php', 'PhabricatorAuthLinkController' => 'applications/auth/controller/PhabricatorAuthLinkController.php', @@ -3980,6 +3981,7 @@ phutil_register_library_map(array( 'PhabricatorAuthFactorConfig' => 'PhabricatorAuthDAO', 'PhabricatorAuthFactorTOTP' => 'PhabricatorAuthFactor', 'PhabricatorAuthFactorTOTPTestCase' => 'PhabricatorTestCase', + 'PhabricatorAuthFinishController' => 'PhabricatorAuthController', 'PhabricatorAuthHighSecurityRequiredException' => 'Exception', 'PhabricatorAuthLinkController' => 'PhabricatorAuthController', 'PhabricatorAuthListController' => 'PhabricatorAuthProviderConfigController', diff --git a/src/aphront/console/DarkConsoleController.php b/src/aphront/console/DarkConsoleController.php index 9135e1c77c..eef85de76a 100644 --- a/src/aphront/console/DarkConsoleController.php +++ b/src/aphront/console/DarkConsoleController.php @@ -16,6 +16,10 @@ final class DarkConsoleController extends PhabricatorController { return !PhabricatorEnv::getEnvConfig('darkconsole.always-on'); } + public function shouldAllowPartialSessions() { + return true; + } + public function processRequest() { $request = $this->getRequest(); $user = $request->getUser(); diff --git a/src/aphront/console/DarkConsoleDataController.php b/src/aphront/console/DarkConsoleDataController.php index 3bf9f902db..b1df699407 100644 --- a/src/aphront/console/DarkConsoleDataController.php +++ b/src/aphront/console/DarkConsoleDataController.php @@ -15,6 +15,10 @@ final class DarkConsoleDataController extends PhabricatorController { return !PhabricatorEnv::getEnvConfig('darkconsole.always-on'); } + public function shouldAllowPartialSessions() { + return true; + } + public function willProcessRequest(array $data) { $this->key = $data['key']; } diff --git a/src/applications/auth/application/PhabricatorApplicationAuth.php b/src/applications/auth/application/PhabricatorApplicationAuth.php index 0230fe35c1..097f370f45 100644 --- a/src/applications/auth/application/PhabricatorApplicationAuth.php +++ b/src/applications/auth/application/PhabricatorApplicationAuth.php @@ -83,6 +83,7 @@ final class PhabricatorApplicationAuth extends PhabricatorApplication { 'register/(?:(?P[^/]+)/)?' => 'PhabricatorAuthRegisterController', 'start/' => 'PhabricatorAuthStartController', 'validate/' => 'PhabricatorAuthValidateController', + 'finish/' => 'PhabricatorAuthFinishController', 'unlink/(?P[^/]+)/' => 'PhabricatorAuthUnlinkController', '(?Plink|refresh)/(?P[^/]+)/' => 'PhabricatorAuthLinkController', diff --git a/src/applications/auth/controller/PhabricatorAuthController.php b/src/applications/auth/controller/PhabricatorAuthController.php index 9212c4c49c..c646de7322 100644 --- a/src/applications/auth/controller/PhabricatorAuthController.php +++ b/src/applications/auth/controller/PhabricatorAuthController.php @@ -82,7 +82,7 @@ abstract class PhabricatorAuthController extends PhabricatorController { $should_login = $event->getValue('shouldLogin'); if ($should_login) { $session_key = id(new PhabricatorAuthSessionEngine()) - ->establishSession($session_type, $user->getPHID()); + ->establishSession($session_type, $user->getPHID(), $partial = true); // NOTE: We allow disabled users to login and roadblock them later, so // there's no check for users being disabled here. diff --git a/src/applications/auth/controller/PhabricatorAuthFinishController.php b/src/applications/auth/controller/PhabricatorAuthFinishController.php new file mode 100644 index 0000000000..e68349f01f --- /dev/null +++ b/src/applications/auth/controller/PhabricatorAuthFinishController.php @@ -0,0 +1,71 @@ +getRequest(); + $viewer = $request->getUser(); + + // If the user already has a full session, just kick them out of here. + $has_partial_session = $viewer->hasSession() && + $viewer->getSession()->getIsPartial(); + if (!$has_partial_session) { + return id(new AphrontRedirectResponse())->setURI('/'); + } + + $engine = new PhabricatorAuthSessionEngine(); + + try { + $token = $engine->requireHighSecuritySession( + $viewer, + $request, + '/logout/'); + } catch (PhabricatorAuthHighSecurityRequiredException $ex) { + $form = id(new PhabricatorAuthSessionEngine())->renderHighSecurityForm( + $ex->getFactors(), + $ex->getFactorValidationResults(), + $viewer, + $request); + + return $this->newDialog() + ->setTitle(pht('Provide Multi-Factor Credentials')) + ->setShortTitle(pht('Multi-Factor Login')) + ->setWidth(AphrontDialogView::WIDTH_FORM) + ->addHiddenInput(AphrontRequest::TYPE_HISEC, true) + ->appendParagraph( + pht( + 'Welcome, %s. To complete the login process, provide your '. + 'multi-factor credentials.', + phutil_tag('strong', array(), $viewer->getUsername()))) + ->appendChild($form->buildLayoutView()) + ->setSubmitURI($request->getPath()) + ->addCancelButton($ex->getCancelURI()) + ->addSubmitButton(pht('Continue')); + } + + // Upgrade the partial session to a full session. + $engine->upgradePartialSession($viewer); + + // TODO: It might be nice to add options like "bind this session to my IP" + // here, even for accounts without multi-factor auth attached to them. + + $next = PhabricatorCookies::getNextURICookie($request); + $request->clearCookie(PhabricatorCookies::COOKIE_NEXTURI); + + if (!PhabricatorEnv::isValidLocalWebResource($next)) { + $next = '/'; + } + + return id(new AphrontRedirectResponse())->setURI($next); + } + +} diff --git a/src/applications/auth/controller/PhabricatorAuthValidateController.php b/src/applications/auth/controller/PhabricatorAuthValidateController.php index 18524163e0..124984400a 100644 --- a/src/applications/auth/controller/PhabricatorAuthValidateController.php +++ b/src/applications/auth/controller/PhabricatorAuthValidateController.php @@ -7,6 +7,10 @@ final class PhabricatorAuthValidateController return false; } + public function shouldAllowPartialSessions() { + return true; + } + public function processRequest() { $request = $this->getRequest(); $viewer = $request->getUser(); @@ -54,14 +58,8 @@ final class PhabricatorAuthValidateController return $this->renderErrors($failures); } - $next = PhabricatorCookies::getNextURICookie($request); - $request->clearCookie(PhabricatorCookies::COOKIE_NEXTURI); - - if (!PhabricatorEnv::isValidLocalWebResource($next)) { - $next = '/'; - } - - return id(new AphrontRedirectResponse())->setURI($next); + $finish_uri = $this->getApplicationURI('finish/'); + return id(new AphrontRedirectResponse())->setURI($finish_uri); } private function renderErrors(array $messages) { diff --git a/src/applications/auth/controller/PhabricatorLogoutController.php b/src/applications/auth/controller/PhabricatorLogoutController.php index 91e6ebff26..c7543991c9 100644 --- a/src/applications/auth/controller/PhabricatorLogoutController.php +++ b/src/applications/auth/controller/PhabricatorLogoutController.php @@ -17,6 +17,10 @@ final class PhabricatorLogoutController return false; } + public function shouldAllowPartialSessions() { + return true; + } + public function processRequest() { $request = $this->getRequest(); $user = $request->getUser(); diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php index 1acf6ffe9c..cba44e7a9a 100644 --- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -94,6 +94,7 @@ final class PhabricatorAuthSessionEngine extends Phobject { s.sessionExpires AS s_sessionExpires, s.sessionStart AS s_sessionStart, s.highSecurityUntil AS s_highSecurityUntil, + s.isPartial AS s_isPartial, u.* FROM %T u JOIN %T s ON u.phid = s.userPHID AND s.type = %s AND s.sessionKey = %s', @@ -159,9 +160,10 @@ final class PhabricatorAuthSessionEngine extends Phobject { * @{class:PhabricatorAuthSession}). * @param phid|null Identity to establish a session for, usually a user * PHID. With `null`, generates an anonymous session. + * @param bool True to issue a partial session. * @return string Newly generated session key. */ - public function establishSession($session_type, $identity_phid) { + public function establishSession($session_type, $identity_phid, $partial) { // Consume entropy to generate a new session key, forestalling the eventual // heat death of the universe. $session_key = Filesystem::readRandomCharacters(40); @@ -176,26 +178,32 @@ final class PhabricatorAuthSessionEngine extends Phobject { // This has a side effect of validating the session type. $session_ttl = PhabricatorAuthSession::getSessionTypeTTL($session_type); + $digest_key = PhabricatorHash::digest($session_key); + // Logging-in users don't have CSRF stuff yet, so we have to unguard this // write. $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); id(new PhabricatorAuthSession()) ->setUserPHID($identity_phid) ->setType($session_type) - ->setSessionKey(PhabricatorHash::digest($session_key)) + ->setSessionKey($digest_key) ->setSessionStart(time()) ->setSessionExpires(time() + $session_ttl) + ->setIsPartial($partial ? 1 : 0) ->save(); $log = PhabricatorUserLog::initializeNewLog( null, $identity_phid, - PhabricatorUserLog::ACTION_LOGIN); + ($partial + ? PhabricatorUserLog::ACTION_LOGIN_PARTIAL + : PhabricatorUserLog::ACTION_LOGIN)); + $log->setDetails( array( 'session_type' => $session_type, )); - $log->setSession($session_key); + $log->setSession($digest_key); $log->save(); unset($unguarded); @@ -287,6 +295,12 @@ final class PhabricatorAuthSessionEngine extends Phobject { new PhabricatorAuthTryFactorAction(), -1); + if ($session->getIsPartial()) { + // If we have a partial session, just issue a token without + // putting it in high security mode. + return $this->issueHighSecurityToken($session, true); + } + $until = time() + phutil_units('15 minutes in seconds'); $session->setHighSecurityUntil($until); @@ -303,9 +317,6 @@ final class PhabricatorAuthSessionEngine extends Phobject { PhabricatorUserLog::ACTION_ENTER_HISEC); $log->save(); } else { - - - $log = PhabricatorUserLog::initializeNewLog( $viewer, $viewer->getPHID(), @@ -331,11 +342,15 @@ final class PhabricatorAuthSessionEngine extends Phobject { * Issue a high security token for a session, if authorized. * * @param PhabricatorAuthSession Session to issue a token for. + * @param bool Force token issue. * @return PhabricatorAuthHighSecurityToken|null Token, if authorized. */ - private function issueHighSecurityToken(PhabricatorAuthSession $session) { + private function issueHighSecurityToken( + PhabricatorAuthSession $session, + $force = false) { + $until = $session->getHighSecurityUntil(); - if ($until > time()) { + if ($until > time() || $force) { return new PhabricatorAuthHighSecurityToken(); } return null; @@ -390,4 +405,42 @@ final class PhabricatorAuthSessionEngine extends Phobject { $log->save(); } + + /** + * Upgrade a partial session to a full session. + * + * @param PhabricatorAuthSession Session to upgrade. + * @return void + */ + public function upgradePartialSession(PhabricatorUser $viewer) { + if (!$viewer->hasSession()) { + throw new Exception( + pht('Upgrading partial session of user with no session!')); + } + + $session = $viewer->getSession(); + + if (!$session->getIsPartial()) { + throw new Exception(pht('Session is not partial!')); + } + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $session->setIsPartial(0); + + queryfx( + $session->establishConnection('w'), + 'UPDATE %T SET isPartial = %d WHERE id = %d', + $session->getTableName(), + 0, + $session->getID()); + + $log = PhabricatorUserLog::initializeNewLog( + $viewer, + $viewer->getPHID(), + PhabricatorUserLog::ACTION_LOGIN_FULL); + $log->save(); + unset($unguarded); + + } + } diff --git a/src/applications/auth/storage/PhabricatorAuthSession.php b/src/applications/auth/storage/PhabricatorAuthSession.php index f78a06baec..3566e796ae 100644 --- a/src/applications/auth/storage/PhabricatorAuthSession.php +++ b/src/applications/auth/storage/PhabricatorAuthSession.php @@ -12,6 +12,7 @@ final class PhabricatorAuthSession extends PhabricatorAuthDAO protected $sessionStart; protected $sessionExpires; protected $highSecurityUntil; + protected $isPartial; private $identityObject = self::ATTACHABLE; diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php index 1709d5b28d..a461de5095 100644 --- a/src/applications/base/controller/PhabricatorController.php +++ b/src/applications/base/controller/PhabricatorController.php @@ -20,6 +20,10 @@ abstract class PhabricatorController extends AphrontController { return false; } + public function shouldAllowPartialSessions() { + return false; + } + public function shouldRequireEmailVerification() { return PhabricatorUserEmail::isEmailVerificationRequired(); } @@ -53,7 +57,8 @@ abstract class PhabricatorController extends AphrontController { // session. This is used to provide CSRF protection to logged-out users. $phsid = $session_engine->establishSession( PhabricatorAuthSession::TYPE_WEB, - null); + null, + $partial = false); // This may be a resource request, in which case we just don't set // the cookie. @@ -133,14 +138,24 @@ abstract class PhabricatorController extends AphrontController { return $this->delegateToController($checker_controller); } + $auth_class = 'PhabricatorApplicationAuth'; + $auth_application = PhabricatorApplication::getByClass($auth_class); + + // Require partial sessions to finish login before doing anything. + if (!$this->shouldAllowPartialSessions()) { + if ($user->hasSession() && + $user->getSession()->getIsPartial()) { + $login_controller = new PhabricatorAuthFinishController($request); + $this->setCurrentApplication($auth_application); + return $this->delegateToController($login_controller); + } + } + if ($this->shouldRequireLogin()) { // This actually means we need either: // - a valid user, or a public controller; and // - permission to see the application. - $auth_class = 'PhabricatorApplicationAuth'; - $auth_application = PhabricatorApplication::getByClass($auth_class); - $allow_public = $this->shouldAllowPublic() && PhabricatorEnv::getEnvConfig('policy.allow-public'); diff --git a/src/applications/conduit/method/ConduitAPI_conduit_connect_Method.php b/src/applications/conduit/method/ConduitAPI_conduit_connect_Method.php index 2e87c7cc82..bbe5376506 100644 --- a/src/applications/conduit/method/ConduitAPI_conduit_connect_Method.php +++ b/src/applications/conduit/method/ConduitAPI_conduit_connect_Method.php @@ -145,10 +145,10 @@ final class ConduitAPI_conduit_connect_Method extends ConduitAPIMethod { if ($valid != $signature) { throw new ConduitException('ERR-INVALID-CERTIFICATE'); } - $session_key = id(new PhabricatorAuthSessionEngine()) - ->establishSession( - PhabricatorAuthSession::TYPE_CONDUIT, - $user->getPHID()); + $session_key = id(new PhabricatorAuthSessionEngine())->establishSession( + PhabricatorAuthSession::TYPE_CONDUIT, + $user->getPHID(), + $partial = false); } else { throw new ConduitException('ERR-NO-CERTIFICATE'); } diff --git a/src/applications/people/storage/PhabricatorUserLog.php b/src/applications/people/storage/PhabricatorUserLog.php index 74f399595c..5a0dfdfc39 100644 --- a/src/applications/people/storage/PhabricatorUserLog.php +++ b/src/applications/people/storage/PhabricatorUserLog.php @@ -4,6 +4,8 @@ final class PhabricatorUserLog extends PhabricatorUserDAO implements PhabricatorPolicyInterface { const ACTION_LOGIN = 'login'; + const ACTION_LOGIN_PARTIAL = 'login-partial'; + const ACTION_LOGIN_FULL = 'login-full'; const ACTION_LOGOUT = 'logout'; const ACTION_LOGIN_FAILURE = 'login-fail'; const ACTION_RESET_PASSWORD = 'reset-pass'; @@ -46,7 +48,9 @@ final class PhabricatorUserLog extends PhabricatorUserDAO public static function getActionTypeMap() { return array( self::ACTION_LOGIN => pht('Login'), - self::ACTION_LOGIN_FAILURE => pht('Login Failure'), + self::ACTION_LOGIN_PARTIAL => pht('Login: Partial Login'), + self::ACTION_LOGIN_FULL => pht('Login: Upgrade to Full'), + self::ACTION_LOGIN_FAILURE => pht('Login: Failure'), self::ACTION_LOGOUT => pht('Logout'), self::ACTION_RESET_PASSWORD => pht('Reset Password'), self::ACTION_CREATE => pht('Create Account'), diff --git a/src/infrastructure/celerity/CelerityResourceController.php b/src/infrastructure/celerity/CelerityResourceController.php index 51ed9c914b..88f4a84969 100644 --- a/src/infrastructure/celerity/CelerityResourceController.php +++ b/src/infrastructure/celerity/CelerityResourceController.php @@ -14,6 +14,10 @@ abstract class CelerityResourceController extends PhabricatorController { return false; } + public function shouldAllowPartialSessions() { + return true; + } + abstract public function getCelerityResourceMap(); protected function serveResource($path, $package_hash = null) {