From 43dc3d7c47c3f6926072306b7405b7b1de6f1637 Mon Sep 17 00:00:00 2001
From: Lai Jiang <jianglai@google.com>
Date: Thu, 8 Aug 2019 14:44:33 -0400
Subject: [PATCH] Fix expired access token bug in the proxy (#217)

https://github.com/google/nomulus/pull/129 migrated `GoogleCredential`
to `GoogleCredentialsBundle` and introduced a subtle bug. I don't fully
understand why but there are times when the access token is null but
`credentials.refresh()` is not called, resulting in NullPointerException
when `credentials.getAccessToken().getTokenValue()` is called.

Since the new GoogleCredentials class supports `shouldRefresh()`, we now
just rely on it to make sure that we always get a value access token.
---
 .../google/registry/proxy/ProxyConfig.java    |  1 -
 .../google/registry/proxy/ProxyModule.java    | 19 ++++---------------
 .../registry/proxy/config/default-config.yaml | 19 -------------------
 3 files changed, 4 insertions(+), 35 deletions(-)

diff --git a/proxy/src/main/java/google/registry/proxy/ProxyConfig.java b/proxy/src/main/java/google/registry/proxy/ProxyConfig.java
index 1f30113a9..79ea599c4 100644
--- a/proxy/src/main/java/google/registry/proxy/ProxyConfig.java
+++ b/proxy/src/main/java/google/registry/proxy/ProxyConfig.java
@@ -39,7 +39,6 @@ public class ProxyConfig {
 
   public String projectId;
   public List<String> gcpScopes;
-  public int accessTokenRefreshBeforeExpirationSeconds;
   public int serverCertificateCacheSeconds;
   public Gcs gcs;
   public Kms kms;
diff --git a/proxy/src/main/java/google/registry/proxy/ProxyModule.java b/proxy/src/main/java/google/registry/proxy/ProxyModule.java
index 361ea866d..694548d37 100644
--- a/proxy/src/main/java/google/registry/proxy/ProxyModule.java
+++ b/proxy/src/main/java/google/registry/proxy/ProxyModule.java
@@ -23,7 +23,6 @@ import com.beust.jcommander.ParameterException;
 import com.google.api.services.cloudkms.v1.CloudKMS;
 import com.google.api.services.cloudkms.v1.model.DecryptRequest;
 import com.google.api.services.storage.Storage;
-import com.google.auth.oauth2.AccessToken;
 import com.google.auth.oauth2.GoogleCredentials;
 import com.google.common.flogger.LoggerConfig;
 import com.google.monitoring.metrics.MetricReporter;
@@ -49,7 +48,6 @@ import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.Base64;
-import java.util.Date;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ExecutorService;
@@ -223,19 +221,10 @@ public class ProxyModule {
       GoogleCredentialsBundle credentialsBundle, ProxyConfig config) {
     return () -> {
       GoogleCredentials credentials = credentialsBundle.getGoogleCredentials();
-      AccessToken accessToken = credentials.getAccessToken();
-      Date nextExpirationTime =
-          new Date(
-              System.currentTimeMillis() + config.accessTokenRefreshBeforeExpirationSeconds * 1000);
-      // If we never obtained an access token, the expiration time is null.
-      if (accessToken == null
-          // If we have an access token, make sure to refresh it ahead of time.
-          || accessToken.getExpirationTime().before(nextExpirationTime)) {
-        try {
-          credentials.refresh();
-        } catch (IOException e) {
-          throw new RuntimeException("Cannot refresh access token.", e);
-        }
+      try {
+        credentials.refreshIfExpired();
+      } catch (IOException e) {
+        throw new RuntimeException("Cannot refresh access token.", e);
       }
       return credentials.getAccessToken().getTokenValue();
     };
diff --git a/proxy/src/main/java/google/registry/proxy/config/default-config.yaml b/proxy/src/main/java/google/registry/proxy/config/default-config.yaml
index bc8b6c3e0..301cf5be9 100644
--- a/proxy/src/main/java/google/registry/proxy/config/default-config.yaml
+++ b/proxy/src/main/java/google/registry/proxy/config/default-config.yaml
@@ -20,25 +20,6 @@ gcpScopes:
   # to authenticate.
   - https://www.googleapis.com/auth/userinfo.email
 
-# Refresh the access token 5 minutes before it expires.
-#
-# Depending on how the credential is obtained, its renewal behavior is
-# different. A credential backed by a private key (like the ADC obtained
-# locally) will get a different token when #refreshToken() is called. On GCE,
-# the credential is just a wrapper around tokens sent from the metadata server,
-# which is valid from 3599 seconds to 1699 seconds (this is no documentation on
-# this, I got this number by logging in a GCE VM, calling curl on the metatdata
-# server every minute, and check the expiration time of the response). Calling
-# refreshToken() does *not* get a new token. The token is only refreshed by
-# metadata server itself (every 3599 - 1699 = 1900 seconds).
-#
-# We refresh the token 5 minutes before it expires, which should work in both
-# cases. This is better than caching the token for a pre-defined period, because
-# even right after #refreshToken() is called on the client side, tokens obtained
-# from GCE metadata server may not be valid for the entirety of 3599 seconds.
-
-accessTokenRefreshBeforeExpirationSeconds: 300
-
 # Server certificate is cached for 30 minutes.
 #
 # Encrypted server server certificate and private keys are stored on GCS. They