From ca39be60914b844d00df2cdc8356d610d7b1f372 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 18 Dec 2018 11:54:05 -0800
Subject: [PATCH] Make partial sessions expire after 30 minutes, and do not
 extend them

Summary:
Depends on D19904. Ref T13226. Ref T13222. Currently, partial sessions (where you've provided a primary auth factor like a password, but not yet provided MFA) work like normal sessions: they're good for 30 days and extend indefinitely under regular use.

This behavior is convenient for full sessions, but normal users don't ever spend 30 minutes answering MFA, so there's no real reason to do it for partial sessions. If we add login alerts in the future, limiting partial sessions to a short lifetime will make them more useful, since an attacker can't get one partial session and keep extending it forever while waiting for an opportunity to get past your MFA.

Test Plan:
  - Did a partial login (to the MFA prompt), checked database, saw a ~29 minute partial session.
  - Did a full login, saw session extend to ~30 days.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13226, T13222

Differential Revision: https://secure.phabricator.com/D19905
---
 .../engine/PhabricatorAuthSessionEngine.php   | 61 ++++++++++++-------
 .../auth/storage/PhabricatorAuthSession.php   |  8 ++-
 2 files changed, 46 insertions(+), 23 deletions(-)

diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
index cc317c894d..ff3b2adc05 100644
--- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
+++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
@@ -213,26 +213,7 @@ final class PhabricatorAuthSessionEngine extends Phobject {
 
     $session = id(new PhabricatorAuthSession())->loadFromArray($session_dict);
 
-    $ttl = PhabricatorAuthSession::getSessionTypeTTL($session_type);
-
-    // If more than 20% of the time on this session has been used, refresh the
-    // TTL back up to the full duration. The idea here is that sessions are
-    // good forever if used regularly, but get GC'd when they fall out of use.
-
-    // NOTE: If we begin rotating session keys when extending sessions, the
-    // CSRF code needs to be updated so CSRF tokens survive session rotation.
-
-    if (time() + (0.80 * $ttl) > $session->getSessionExpires()) {
-      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-        $conn_w = $session_table->establishConnection('w');
-        queryfx(
-          $conn_w,
-          'UPDATE %T SET sessionExpires = UNIX_TIMESTAMP() + %d WHERE id = %d',
-          $session->getTableName(),
-          $ttl,
-          $session->getID());
-      unset($unguarded);
-    }
+    $this->extendSession($session);
 
     // TODO: Remove this, see T13225.
     if ($is_weak) {
@@ -284,7 +265,9 @@ final class PhabricatorAuthSessionEngine extends Phobject {
     $conn_w = $session_table->establishConnection('w');
 
     // This has a side effect of validating the session type.
-    $session_ttl = PhabricatorAuthSession::getSessionTypeTTL($session_type);
+    $session_ttl = PhabricatorAuthSession::getSessionTypeTTL(
+      $session_type,
+      $partial);
 
     $digest_key = PhabricatorAuthSession::newSessionDigest(
       new PhutilOpaqueEnvelope($session_key));
@@ -1086,4 +1069,40 @@ final class PhabricatorAuthSessionEngine extends Phobject {
     }
   }
 
+  private function extendSession(PhabricatorAuthSession $session) {
+    $is_partial = $session->getIsPartial();
+
+    // Don't extend partial sessions. You have a relatively short window to
+    // upgrade into a full session, and your session expires otherwise.
+    if ($is_partial) {
+      return;
+    }
+
+    $session_type = $session->getType();
+
+    $ttl = PhabricatorAuthSession::getSessionTypeTTL(
+      $session_type,
+      $session->getIsPartial());
+
+    // If more than 20% of the time on this session has been used, refresh the
+    // TTL back up to the full duration. The idea here is that sessions are
+    // good forever if used regularly, but get GC'd when they fall out of use.
+
+    $now = PhabricatorTime::getNow();
+    if ($now + (0.80 * $ttl) <= $session->getSessionExpires()) {
+      return;
+    }
+
+    $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+      queryfx(
+        $session->establishConnection('w'),
+        'UPDATE %R SET sessionExpires = UNIX_TIMESTAMP() + %d
+          WHERE id = %d',
+        $session,
+        $ttl,
+        $session->getID());
+    unset($unguarded);
+  }
+
+
 }
diff --git a/src/applications/auth/storage/PhabricatorAuthSession.php b/src/applications/auth/storage/PhabricatorAuthSession.php
index 6d54dda781..e007272f7f 100644
--- a/src/applications/auth/storage/PhabricatorAuthSession.php
+++ b/src/applications/auth/storage/PhabricatorAuthSession.php
@@ -72,10 +72,14 @@ final class PhabricatorAuthSession extends PhabricatorAuthDAO
     return $this->assertAttached($this->identityObject);
   }
 
-  public static function getSessionTypeTTL($session_type) {
+  public static function getSessionTypeTTL($session_type, $is_partial) {
     switch ($session_type) {
       case self::TYPE_WEB:
-        return phutil_units('30 days in seconds');
+        if ($is_partial) {
+          return phutil_units('30 minutes in seconds');
+        } else {
+          return phutil_units('30 days in seconds');
+        }
       case self::TYPE_CONDUIT:
         return phutil_units('24 hours in seconds');
       default: