From f8ed6422f8e2492ca80985a5d49ac375eaafb3f9 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 24 Jun 2013 15:56:01 -0700
Subject: [PATCH] Provide an auto-refresh mechanism for OAuth providers to
 deliver fresh tokens

Summary:
Ref T2852. Give OAuth providers a formal method so you can ask them for tokens; they issue a refresh request if necessary.

We could automatically refresh these tokens in daemons as they near expiry to improve performance; refreshes are blocking in-process round trip requests. If we do this for all tokens, it's a lot of requests (say, 20k users * 2 auth mechanisms * 1-hour tokens ~= a million requests a day). We could do it selectively for tokens that are actually in use (i.e., if we refresh a token in response to a user request, we keep refreshing it for 24 hours automatically). For now, I'm not pursuing any of this.

If we fail to refresh a token, we don't have a great way to communicate it to the user right now. The remedy is "log out and log in again", but there's no way for them to figure this out. The major issue is that a lot of OAuth integrations should not throw if they fail, or can't reasonably be rasied to the user (e.g., activity in daemons, loading profile pictures, enriching links, etc). For now, this shouldn't really happen. In future diffs, I plan to make the "External Accounts" settings page provide some information about tokens again, and possibly push some flag to accounts like "you should refresh your X link", but we'll see if issues crop up.

Test Plan: Used `bin/auth refresh` to verify refreshes. I'll wait an hour and reload a page with an Asana link to verify the auto-refresh part.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2852

Differential Revision: https://secure.phabricator.com/D6280
---
 ...abricatorAuthManagementRefreshWorkflow.php |  5 ++-
 .../provider/PhabricatorAuthProviderOAuth.php | 43 +++++++++++++++++++
 .../bridge/DoorkeeperBridgeAsana.php          |  2 +-
 3 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/src/applications/auth/management/PhabricatorAuthManagementRefreshWorkflow.php b/src/applications/auth/management/PhabricatorAuthManagementRefreshWorkflow.php
index e9b65abac4..97957d7c91 100644
--- a/src/applications/auth/management/PhabricatorAuthManagementRefreshWorkflow.php
+++ b/src/applications/auth/management/PhabricatorAuthManagementRefreshWorkflow.php
@@ -126,13 +126,14 @@ final class PhabricatorAuthManagementRefreshWorkflow
           new PhutilNumber(
             $account->getProperty('oauth.token.access.expires') - time())));
 
-      $adapter->refreshAccessToken($refresh_token);
+      $provider->getOAuthAccessToken($account, $force_refresh = true);
 
       $console->writeOut(
         "+ %s\n",
         pht(
           "Refreshed token, new token expires in %s seconds.",
-          new PhutilNumber($adapter->getAccessTokenExpires() - time())));
+          new PhutilNumber(
+            $account->getProperty('oauth.token.access.expires') - time())));
 
     }
 
diff --git a/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php b/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php
index e83aa81e69..16b45d53cb 100644
--- a/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php
+++ b/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php
@@ -283,7 +283,11 @@ abstract class PhabricatorAuthProviderOAuth extends PhabricatorAuthProvider {
 
   protected function willSaveAccount(PhabricatorExternalAccount $account) {
     parent::willSaveAccount($account);
+    $this->synchronizeOAuthAccount($account);
+  }
 
+  protected function synchronizeOAuthAccount(
+    PhabricatorExternalAccount $account) {
     $adapter = $this->getAdapter();
 
     $oauth_token = $adapter->getAccessToken();
@@ -300,4 +304,43 @@ abstract class PhabricatorAuthProviderOAuth extends PhabricatorAuthProvider {
     $account->setProperty('oauth.token.access.expires', $expires);
   }
 
+  public function getOAuthAccessToken(
+    PhabricatorExternalAccount $account,
+    $force_refresh = false) {
+
+    if ($account->getProviderKey() !== $this->getProviderKey()) {
+      throw new Exception("Account does not match provider!");
+    }
+
+    if (!$force_refresh) {
+      $access_expires = $account->getProperty('oauth.token.access.expires');
+      $access_token = $account->getProperty('oauth.token.access');
+
+      // Don't return a token with fewer than this many seconds remaining until
+      // it expires.
+      $shortest_token = 60;
+
+      if ($access_token) {
+        if ($access_expires > (time() + $shortest_token)) {
+          return $access_token;
+        }
+      }
+    }
+
+    $refresh_token = $account->getProperty('oauth.token.refresh');
+    if ($refresh_token) {
+      $adapter = $this->getAdapter();
+      if ($adapter->supportsTokenRefresh()) {
+        $adapter->refreshAccessToken($refresh_token);
+
+        $this->synchronizeOAuthAccount($account);
+        $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+          $account->save();
+        unset($unguarded);
+      }
+    }
+
+    return null;
+  }
+
 }
diff --git a/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php b/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php
index d40e08dad0..21ed1f5427 100644
--- a/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php
+++ b/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php
@@ -35,7 +35,7 @@ final class DoorkeeperBridgeAsana extends DoorkeeperBridge {
     // right now so this is currently moot.
     $account = head($accounts);
 
-    $token = $account->getProperty('oauth.token.access');
+    $token = $provider->getOAuthAccessToken($account);
     if (!$token) {
       return;
     }