Fix: Propagate credential refresh exceptions in blocking refresh.

googleapis · Dec 20, 2024 · acfb11b · acfb11b
1 parent 13fd80f
commit acfb11b
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 26 deletions.
diff --git a/...a/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java b/...a/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java
@@ -123,29 +123,26 @@ void refreshCredentialsIfRequired() throws IOException {
     switch (refreshType) {
       case BLOCKING:
         if (refreshTask.isNew) {
-          // Execute the new refresh task synchronously on a direct executor.
-          // This blocks until the refresh is complete.
+          // Start a new refresh task only if the task is new
           MoreExecutors.directExecutor().execute(refreshTask.task);
-        } else {
-          // A refresh is already in progress, wait for it to complete.
-          try {
-            refreshTask.task.get();
-          } catch (InterruptedException e) {
-            // Restore the interrupted status and throw an exception.
-            Thread.currentThread().interrupt();
-            throw new IOException(
-                "Interrupted while asynchronously refreshing the intermediate credentials", e);
-          } catch (ExecutionException e) {
-            // Unwrap the underlying cause of the execution exception.
-            Throwable cause = e.getCause();
-            if (cause instanceof IOException) {
-              throw (IOException) cause;
-            } else if (cause instanceof RuntimeException) {
-              throw (RuntimeException) cause;
-            } else {
-              // Wrap other exceptions in an IOException.
-              throw new IOException("Unexpected error refreshing intermediate credentials", cause);
-            }
+        }
+        try {
+          refreshTask.task.get(); // Wait for the refresh task to complete.
+        } catch (InterruptedException e) {
+          // Restore the interrupted status and throw an exception.
+          Thread.currentThread().interrupt();
+          throw new IOException(
+              "Interrupted while asynchronously refreshing the intermediate credentials", e);
+        } catch (ExecutionException e) {
+          // Unwrap the underlying cause of the execution exception.
+          Throwable cause = e.getCause();
+          if (cause instanceof IOException) {
+            throw (IOException) cause;
+          } else if (cause instanceof RuntimeException) {
+            throw (RuntimeException) cause;
+          } else {
+            // Wrap other exceptions in an IOException.
+            throw new IOException("Unexpected error refreshing intermediate credentials", cause);
           }
         }
         break;
@@ -296,12 +293,11 @@ private static AccessToken getTokenFromResponse(
    * exceptions during the refresh are caught and suppressed to prevent indefinite blocking of
    * subsequent refresh attempts.
    */
-  private void finishRefreshTask(ListenableFuture<IntermediateCredentials> finishedTask) {
+  private void finishRefreshTask(ListenableFuture<IntermediateCredentials> finishedTask)
+      throws ExecutionException {
     synchronized (refreshLock) {
       try {
         this.intermediateCredentials = Futures.getDone(finishedTask);
-      } catch (Exception e) {
-        // noop
       } finally {
         if (this.refreshTask != null && this.refreshTask.task == finishedTask) {
           this.refreshTask = null;
@@ -372,7 +368,16 @@ class RefreshTask extends AbstractFuture<IntermediateCredentials> implements Run
       this.isNew = isNew;
 
       // Add listener to update factory's credentials when the task completes.
-      task.addListener(() -> finishRefreshTask(task), MoreExecutors.directExecutor());
+      task.addListener(
+          () -> {
+            try {
+              finishRefreshTask(task);
+            } catch (ExecutionException e) {
+              Throwable cause = e.getCause();
+              RefreshTask.this.setException(cause);
+            }
+          },
+          MoreExecutors.directExecutor());
 
       // Add callback to set the result or exception based on the outcome.
       Futures.addCallback(

diff --git a/...m/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactoryTest.java b/...m/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactoryTest.java
@@ -347,6 +347,29 @@ public void refreshCredentialsIfRequired_asyncMultiThread()
     assertEquals(2, mockStsTransportFactory.transport.getRequestCount());
   }
 
+  @Test
+  public void refreshCredentialsIfRequired_sourceCredentialCannotRefresh_throwsIOException()
+      throws Exception {
+    // Simulate error when refreshing the source credential.
+    mockTokenServerTransportFactory.transport.setError(new IOException());
+
+    GoogleCredentials sourceCredentials =
+        getServiceAccountSourceCredentials(mockTokenServerTransportFactory);
+
+    ClientSideCredentialAccessBoundaryFactory factory =
+        ClientSideCredentialAccessBoundaryFactory.newBuilder()
+            .setSourceCredential(sourceCredentials)
+            .setHttpTransportFactory(mockStsTransportFactory)
+            .build();
+
+    try {
+      factory.refreshCredentialsIfRequired(); // Expecting an IOException
+      fail("Should fail as the source credential should not be able to be refreshed.");
+    } catch (IOException e) {
+      assertEquals("Unable to refresh the provided source credential.", e.getMessage());
+    }
+  }
+
   // Tests related to the builder methods
   @Test
   public void builder_noSourceCredential_throws() {