Fixed -> 1. getRequestMetadata calls refreshTrustBoundaryIfExpired without a try catch block. 2. Lock acquiral for refreshFuture.compareAndSet(null, future) now fixed. 3. Oauth2Credentials isn't caching RAB which was earlier leading to serialization issues.

vverman · vverman · commit b4dd7f6316e3 · 2026-03-19T19:14:07.000-07:00
diff --git a/oauth2_http/java/com/google/auth/oauth2/GoogleCredentials.java b/oauth2_http/java/com/google/auth/oauth2/GoogleCredentials.java
@@ -440,7 +440,12 @@ void refreshRegionalAccessBoundaryWithSelfSignedJwtIfExpired(
   public Map<String, List<String>> getRequestMetadata(URI uri) throws IOException {
     Map<String, List<String>> metadata = super.getRequestMetadata(uri);
     metadata = addRegionalAccessBoundaryToRequestMetadata(metadata);
-    refreshRegionalAccessBoundaryIfExpired(uri, getAccessToken(), null);
+    try {
+      // Sets off an async refresh for request-metadata.
+      refreshRegionalAccessBoundaryIfExpired(uri, getAccessToken(), null);
+    } catch (IOException e) {
+      // Ignore failure in async refresh trigger.
+    }
     return metadata;
   }
 
@@ -535,8 +540,9 @@ static Map<String, List<String>> addQuotaProjectIdToRequestMetadata(
 
   /**
    * Adds Regional Access Boundary header to requestMetadata if available. Overwrites if present.
+   * If the current RAB is null, it removes any stale header that might have survived serialization.
    *
-   * @return a new map with Regional Access Boundary header added or updated
+   * @return a new map with Regional Access Boundary header added, updated, or removed
    */
   Map<String, List<String>> addRegionalAccessBoundaryToRequestMetadata(
       Map<String, List<String>> requestMetadata) {
@@ -550,6 +556,12 @@ Map<String, List<String>> addRegionalAccessBoundaryToRequestMetadata(
           RegionalAccessBoundary.X_ALLOWED_LOCATIONS_HEADER_KEY,
           Collections.singletonList(rab.getEncodedLocations()));
       return ImmutableMap.copyOf(newMetadata);
+    } else if (requestMetadata.containsKey(RegionalAccessBoundary.X_ALLOWED_LOCATIONS_HEADER_KEY)) {
+      // If RAB is null but the header exists (e.g., from a serialized cache), we must strip it
+      // to prevent sending stale data to the server.
+      Map<String, List<String>> newMetadata = new HashMap<>(requestMetadata);
+      newMetadata.remove(RegionalAccessBoundary.X_ALLOWED_LOCATIONS_HEADER_KEY);
+      return ImmutableMap.copyOf(newMetadata);
     }
     return requestMetadata;
   }
@@ -558,13 +570,6 @@ Map<String, List<String>> addRegionalAccessBoundaryToRequestMetadata(
   protected Map<String, List<String>> getAdditionalHeaders() {
     Map<String, List<String>> headers = new HashMap<>(super.getAdditionalHeaders());
 
-    RegionalAccessBoundary rab = regionalAccessBoundaryManager.getCachedRAB();
-    if (rab != null) {
-      headers.put(
-          RegionalAccessBoundary.X_ALLOWED_LOCATIONS_HEADER_KEY,
-          Collections.singletonList(rab.getEncodedLocations()));
-    }
-
     String quotaProjectId = this.getQuotaProjectId();
     return addQuotaProjectIdToRequestMetadata(quotaProjectId, headers);
   }
diff --git a/oauth2_http/java/com/google/auth/oauth2/RegionalAccessBoundaryManager.java b/oauth2_http/java/com/google/auth/oauth2/RegionalAccessBoundaryManager.java
@@ -139,17 +139,26 @@ void triggerAsyncRefresh(
             }
           };
 
-      if (executor != null) {
-        executor.execute(refreshTask);
-      } else {
-        // We use new Thread() here instead of
-        // CompletableFuture.runAsync() (which uses ForkJoinPool.commonPool()).
-        // This avoids consuming CPU resources since
-        // The common pool has a small, fixed number of threads designed for
-        // CPU-bound tasks.
-        Thread refreshThread = new Thread(refreshTask, "RAB-refresh-thread");
-        refreshThread.setDaemon(true);
-        refreshThread.start();
+      try {
+        if (executor != null) {
+          executor.execute(refreshTask);
+        } else {
+          // We use new Thread() here instead of
+          // CompletableFuture.runAsync() (which uses ForkJoinPool.commonPool()).
+          // This avoids consuming CPU resources since
+          // The common pool has a small, fixed number of threads designed for
+          // CPU-bound tasks.
+          Thread refreshThread = new Thread(refreshTask, "RAB-refresh-thread");
+          refreshThread.setDaemon(true);
+          refreshThread.start();
+        }
+      } catch (Exception | Error e) {
+        // If scheduling fails (e.g., RejectedExecutionException, OutOfMemoryError for threads),
+        // the task's finally block will never execute. We must release the lock here.
+        refreshFuture.set(null);
+        future.completeExceptionally(e);
+        handleRefreshFailure(
+            new Exception("Regional Access Boundary background refresh failed to schedule", e));
       }
     }
   }
diff --git a/oauth2_http/javatests/com/google/auth/oauth2/GoogleCredentialsTest.java b/oauth2_http/javatests/com/google/auth/oauth2/GoogleCredentialsTest.java
@@ -44,6 +44,7 @@
 import com.google.api.client.json.GenericJson;
 import com.google.api.client.util.Clock;
 import com.google.auth.Credentials;
+import com.google.auth.RequestMetadataCallback;
 import com.google.auth.TestUtils;
 import com.google.auth.http.HttpTransportFactory;
 import com.google.auth.oauth2.ExternalAccountAuthorizedUserCredentialsTest.MockExternalAccountAuthorizedUserCredentialsTransportFactory;
@@ -56,6 +57,7 @@
 import java.util.*;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.atomic.AtomicReference;
+import javax.annotation.Nullable;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -801,6 +803,51 @@ public void serialize() throws IOException, ClassNotFoundException {
     assertNotNull(deserializedCredentials.regionalAccessBoundaryManager);
   }
 
+  @Test
+  public void serialize_removesStaleRabHeaders() throws Exception {
+    GoogleCredentials.disableRabRefreshForTest = false;
+
+    MockTokenServerTransportFactory transportFactory = new MockTokenServerTransportFactory();
+    RegionalAccessBoundary rab =
+        new RegionalAccessBoundary(
+            "test-encoded", Collections.singletonList("test-loc"), System.currentTimeMillis());
+    transportFactory.transport.setRegionalAccessBoundary(rab);
+    transportFactory.transport.addServiceAccount(SA_CLIENT_EMAIL, ACCESS_TOKEN);
+
+    GoogleCredentials credentials =
+        new ServiceAccountCredentials.Builder()
+            .setClientEmail(SA_CLIENT_EMAIL)
+            .setPrivateKey(OAuth2Utils.privateKeyFromPkcs8(SA_PRIVATE_KEY_PKCS8))
+            .setPrivateKeyId(SA_PRIVATE_KEY_ID)
+            .setHttpTransportFactory(transportFactory)
+            .setScopes(SCOPES)
+            .build();
+
+    // 1. Trigger request metadata to start async RAB refresh
+    credentials.getRequestMetadata(URI.create("https://foo.com"));
+
+    // Wait for the RAB to be fetched and cached
+    waitForRegionalAccessBoundary(credentials);
+
+    // 2. Verify the live credential has the RAB header
+    Map<String, List<String>> metadata = credentials.getRequestMetadata();
+    assertEquals(
+        Collections.singletonList("test-encoded"),
+        metadata.get(RegionalAccessBoundary.X_ALLOWED_LOCATIONS_HEADER_KEY));
+
+    // 3. Serialize and deserialize.
+    GoogleCredentials deserialized = serializeAndDeserialize(credentials);
+
+    // 4. Verify.
+    // The manager is transient, so it should be empty.
+    assertNull(deserialized.getRegionalAccessBoundary());
+
+    // The metadata should NOT contain the RAB header anymore, preventing stale headers.
+    Map<String, List<String>> deserializedMetadata = deserialized.getRequestMetadata();
+    assertNull(
+        deserializedMetadata.get(RegionalAccessBoundary.X_ALLOWED_LOCATIONS_HEADER_KEY));
+  }
+
   @Test
   public void toString_containsFields() throws IOException {
     String expectedToString =
@@ -1173,6 +1220,70 @@ public void regionalAccessBoundary_shouldSkipRefreshForRegionalEndpoints() throw
     assertEquals(0, transport.getRegionalAccessBoundaryRequestCount());
   }
 
+  @Test
+  public void getRequestMetadata_ignoresRabRefreshException() throws IOException {
+    GoogleCredentials credentials =
+        new GoogleCredentials() {
+          @Override
+          public AccessToken refreshAccessToken() throws IOException {
+            return new AccessToken("token", null);
+          }
+
+          @Override
+          void refreshRegionalAccessBoundaryIfExpired(
+              @Nullable URI uri,
+              @Nullable AccessToken token,
+              @Nullable java.util.concurrent.Executor executor)
+              throws IOException {
+            throw new IOException("Simulated RAB failure");
+          }
+        };
+
+    // This should not throw the IOException from refreshRegionalAccessBoundaryIfExpired
+    Map<String, List<String>> metadata =
+        credentials.getRequestMetadata(URI.create("https://foo.com"));
+    assertTrue(metadata.containsKey("Authorization"));
+  }
+
+  @Test
+  public void getRequestMetadataAsync_ignoresRabRefreshException() throws IOException {
+    GoogleCredentials credentials =
+        new GoogleCredentials() {
+          @Override
+          public AccessToken refreshAccessToken() throws IOException {
+            return new AccessToken("token", null);
+          }
+
+          @Override
+          void refreshRegionalAccessBoundaryIfExpired(
+              @Nullable URI uri,
+              @Nullable AccessToken token,
+              @Nullable java.util.concurrent.Executor executor)
+              throws IOException {
+            throw new IOException("Simulated RAB failure");
+          }
+        };
+
+    java.util.concurrent.atomic.AtomicBoolean success =
+        new java.util.concurrent.atomic.AtomicBoolean(false);
+    credentials.getRequestMetadata(
+        URI.create("https://foo.com"),
+        Runnable::run,
+        new RequestMetadataCallback() {
+          @Override
+          public void onSuccess(Map<String, List<String>> metadata) {
+            success.set(true);
+          }
+
+          @Override
+          public void onFailure(Throwable exception) {
+            fail("Should not have failed");
+          }
+        });
+
+    assertTrue(success.get());
+  }
+
   private GoogleCredentials createTestCredentials(MockTokenServerTransport transport)
       throws IOException {
     transport.addServiceAccount(SA_CLIENT_EMAIL, ACCESS_TOKEN);
diff --git a/oauth2_http/javatests/com/google/auth/oauth2/RegionalAccessBoundaryTest.java b/oauth2_http/javatests/com/google/auth/oauth2/RegionalAccessBoundaryTest.java
@@ -182,6 +182,46 @@ public void testManagerTriggersRefreshInGracePeriod() throws InterruptedExceptio
     assertEquals(newerEncoded, resultRab.getEncodedLocations());
   }
 
+  @Test
+  public void testManagerReleasesLockOnSchedulingFailure() {
+    RegionalAccessBoundaryManager manager = new RegionalAccessBoundaryManager();
+    HttpTransportFactory transportFactory = () -> new MockHttpTransport();
+    RegionalAccessBoundaryProvider provider = () -> "https://dummy";
+    AccessToken token =
+        new AccessToken("token", new java.util.Date(System.currentTimeMillis() + 10 * 3600000L));
+
+    java.util.concurrent.Executor rejectingExecutor =
+        new java.util.concurrent.Executor() {
+          @Override
+          public void execute(Runnable command) {
+            throw new java.util.concurrent.RejectedExecutionException("Simulated rejection");
+          }
+        };
+
+    manager.triggerAsyncRefresh(transportFactory, provider, token, rejectingExecutor);
+
+    // After rejection, the lock should be released, but it should be in cooldown.
+    assertTrue(manager.isCooldownActive());
+
+    // Advance the clock to bypass cooldown
+    testClock.set(
+        testClock.currentTimeMillis() + RegionalAccessBoundaryManager.MAX_COOLDOWN_MILLIS + 1000);
+    assertFalse(manager.isCooldownActive());
+
+    // Schedule again with a valid executor to prove the lock was released.
+    java.util.concurrent.atomic.AtomicBoolean taskRan =
+        new java.util.concurrent.atomic.AtomicBoolean(false);
+    java.util.concurrent.Executor workingExecutor =
+        new java.util.concurrent.Executor() {
+          @Override
+          public void execute(Runnable command) {
+            taskRan.set(true);
+          }
+        };
+    manager.triggerAsyncRefresh(transportFactory, provider, token, workingExecutor);
+    assertTrue(taskRan.get());
+  }
+
   private static class TestClock implements Clock {
     private final AtomicLong currentTime = new AtomicLong(System.currentTimeMillis());
 
