From 69ddb0ced631e5d23a35f47f96e66681aaf01514 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 23 Jan 2014 14:03:22 -0800 Subject: [PATCH] Issue "anonymous" sessions for logged-out users Summary: Ref T4339. Ref T4310. Currently, sessions look like `"afad85d675fda87a4fadd54"`, and are only issued for logged-in users. To support logged-out CSRF and (eventually) external user sessions, I made two small changes: - First, sessions now have a "kind", which is indicated by a prefix, like `"A/ab987asdcas7dca"`. This mostly allows us to issue session queries more efficiently: we don't have to issue a query at all for anonymous sessions, and can join the correct table for user and external sessions and save a query. Generally, this gives us more debugging information and more opportunity to recover from issues in a user-friendly way, as with the "invalid session" error in this diff. - Secondly, if you load a page and don't have a session, we give you an anonymous session. This is just a secret with no special significance. This does not implement CSRF yet, but gives us a client secret we can use to implement it. Test Plan: - Logged in. - Logged out. - Browsed around. - Logged in again. - Went through link/register. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4310, T4339 Differential Revision: https://secure.phabricator.com/D8043 --- .../PhabricatorAuthStartController.php | 33 +++++-- .../engine/PhabricatorAuthSessionEngine.php | 95 +++++++++++++++++-- .../auth/storage/PhabricatorAuthSession.php | 1 - .../base/controller/PhabricatorController.php | 2 +- .../people/storage/PhabricatorUserLog.php | 1 + src/view/page/PhabricatorStandardPageView.php | 11 +++ 6 files changed, 123 insertions(+), 20 deletions(-) diff --git a/src/applications/auth/controller/PhabricatorAuthStartController.php b/src/applications/auth/controller/PhabricatorAuthStartController.php index ab29c5f43f..223acaa757 100644 --- a/src/applications/auth/controller/PhabricatorAuthStartController.php +++ b/src/applications/auth/controller/PhabricatorAuthStartController.php @@ -24,18 +24,33 @@ final class PhabricatorAuthStartController return $this->processConduitRequest(); } - if (strlen($request->getCookie(PhabricatorCookies::COOKIE_SESSION))) { - // The session cookie is invalid, so clear it. - $request->clearCookie(PhabricatorCookies::COOKIE_USERNAME); - $request->clearCookie(PhabricatorCookies::COOKIE_SESSION); + // If the user gets this far, they aren't logged in, so if they have a + // user session token we can conclude that it's invalid: if it was valid, + // they'd have been logged in above and never made it here. Try to clear + // it and warn the user they may need to nuke their cookies. - 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.")); + $session_token = $request->getCookie(PhabricatorCookies::COOKIE_SESSION); + if (strlen($session_token)) { + $kind = PhabricatorAuthSessionEngine::getSessionKindFromToken( + $session_token); + switch ($kind) { + case PhabricatorAuthSessionEngine::KIND_ANONYMOUS: + // If this is an anonymous session. It's expected that they won't + // be logged in, so we can just continue. + break; + default: + // The session cookie is invalid, so clear it. + $request->clearCookie(PhabricatorCookies::COOKIE_USERNAME); + $request->clearCookie(PhabricatorCookies::COOKIE_SESSION); + + 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()) { diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php index 0db6871ee5..fe40e41536 100644 --- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -2,7 +2,79 @@ final class PhabricatorAuthSessionEngine extends Phobject { - public function loadUserForSession($session_type, $session_key) { + /** + * Session issued to normal users after they login through a standard channel. + * Associates the client with a standard user identity. + */ + const KIND_USER = 'U'; + + + /** + * Session issued to users who login with some sort of credentials but do not + * have full accounts. These are sometimes called "grey users". + * + * TODO: We do not currently issue these sessions, see T4310. + */ + const KIND_EXTERNAL = 'X'; + + + /** + * Session issued to logged-out users which has no real identity information. + * Its purpose is to protect logged-out users from CSRF. + */ + const KIND_ANONYMOUS = 'A'; + + + /** + * Session kind isn't known. + */ + const KIND_UNKNOWN = '?'; + + + /** + * Get the session kind (e.g., anonymous, user, external account) from a + * session token. Returns a `KIND_` constant. + * + * @param string Session token. + * @return const Session kind constant. + */ + public static function getSessionKindFromToken($session_token) { + if (strpos($session_token, '/') === false) { + // Old-style session, these are all user sessions. + return self::KIND_USER; + } + + list($kind, $key) = explode('/', $session_token, 2); + + switch ($kind) { + case self::KIND_ANONYMOUS: + case self::KIND_USER: + case self::KIND_EXTERNAL: + return $kind; + default: + return self::KIND_UNKNOWN; + } + } + + + public function loadUserForSession($session_type, $session_token) { + $session_kind = self::getSessionKindFromToken($session_token); + switch ($session_kind) { + case self::KIND_ANONYMOUS: + // Don't bother trying to load a user for an anonymous session, since + // neither the session nor the user exist. + return null; + case self::KIND_UNKNOWN: + // If we don't know what kind of session this is, don't go looking for + // it. + return null; + case self::KIND_USER: + break; + case self::KIND_EXTERNAL: + // TODO: Implement these (T4310). + return null; + } + $session_table = new PhabricatorAuthSession(); $user_table = new PhabricatorUser(); $conn_r = $session_table->establishConnection('r'); @@ -18,7 +90,7 @@ final class PhabricatorAuthSessionEngine extends Phobject { $user_table->getTableName(), $session_table->getTableName(), $session_type, - PhabricatorHash::digest($session_key)); + PhabricatorHash::digest($session_token)); if (!$info) { return null; @@ -63,19 +135,24 @@ final class PhabricatorAuthSessionEngine extends Phobject { * You can configure the maximum number of concurrent sessions for various * session types in the Phabricator configuration. * - * @param const Session type constant (see - * @{class:PhabricatorAuthSession}). - * @param phid Identity to establish a session for, usually a user PHID. - * @return string Newly generated session key. + * @param const Session type constant (see + * @{class:PhabricatorAuthSession}). + * @param phid|null Identity to establish a session for, usually a user + * PHID. With `null`, generates an anonymous session. + * @return string Newly generated session key. */ public function establishSession($session_type, $identity_phid) { - $session_table = new PhabricatorAuthSession(); - $conn_w = $session_table->establishConnection('w'); - // Consume entropy to generate a new session key, forestalling the eventual // heat death of the universe. $session_key = Filesystem::readRandomCharacters(40); + if ($identity_phid === null) { + return self::KIND_ANONYMOUS.'/'.$session_key; + } + + $session_table = new PhabricatorAuthSession(); + $conn_w = $session_table->establishConnection('w'); + // This has a side effect of validating the session type. $session_ttl = PhabricatorAuthSession::getSessionTypeTTL($session_type); diff --git a/src/applications/auth/storage/PhabricatorAuthSession.php b/src/applications/auth/storage/PhabricatorAuthSession.php index bb5230dbe2..f9d2e41f6e 100644 --- a/src/applications/auth/storage/PhabricatorAuthSession.php +++ b/src/applications/auth/storage/PhabricatorAuthSession.php @@ -50,7 +50,6 @@ final class PhabricatorAuthSession extends PhabricatorAuthDAO } } - /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php index 6de134f7cd..421a0a30ca 100644 --- a/src/applications/base/controller/PhabricatorController.php +++ b/src/applications/base/controller/PhabricatorController.php @@ -36,7 +36,7 @@ abstract class PhabricatorController extends AphrontController { $user = new PhabricatorUser(); $phsid = $request->getCookie(PhabricatorCookies::COOKIE_SESSION); - if ($phsid) { + if (strlen($phsid)) { $session_user = id(new PhabricatorAuthSessionEngine()) ->loadUserForSession(PhabricatorAuthSession::TYPE_WEB, $phsid); if ($session_user) { diff --git a/src/applications/people/storage/PhabricatorUserLog.php b/src/applications/people/storage/PhabricatorUserLog.php index 61d3ecab46..b82799ae6f 100644 --- a/src/applications/people/storage/PhabricatorUserLog.php +++ b/src/applications/people/storage/PhabricatorUserLog.php @@ -68,6 +68,7 @@ final class PhabricatorUserLog extends PhabricatorUserDAO { if (!$this->session) { // TODO: This is not correct if there's a cookie prefix. This object // should take an AphrontRequest. + // TODO: Maybe record session kind, or drop this for anonymous sessions? $this->setSession(idx($_COOKIE, PhabricatorCookies::COOKIE_SESSION)); } $this->details['host'] = php_uname('n'); diff --git a/src/view/page/PhabricatorStandardPageView.php b/src/view/page/PhabricatorStandardPageView.php index d31c3567b8..d3eeccb28c 100644 --- a/src/view/page/PhabricatorStandardPageView.php +++ b/src/view/page/PhabricatorStandardPageView.php @@ -163,6 +163,17 @@ final class PhabricatorStandardPageView extends PhabricatorBarePageView { Javelin::initBehavior('history-install'); Javelin::initBehavior('phabricator-gesture'); + // If the client doesn't have a session token, generate an anonymous + // session. This is used to provide CSRF protection to logged-out users. + $session_token = $request->getCookie(PhabricatorCookies::COOKIE_SESSION); + if (!strlen($session_token)) { + $anonymous_session = id(new PhabricatorAuthSessionEngine()) + ->establishSession('web', null); + $request->setCookie( + PhabricatorCookies::COOKIE_SESSION, + $anonymous_session); + } + $current_token = null; if ($user) { $current_token = $user->getCSRFToken();