fix: return response body from revoke! for logging pipeline (#562)

* fix: return response body from revoke! for logging pipeline
* chore: speed up tests by stubbing gcloud and sleep
* fix: ensure log_response actually logs the message

---------

Co-authored-by: Viacheslav Rostovtsev <virost@google.com>

Author: katsu223

lib/googleauth/signet.rb

@@ -210,10 +210,12 @@ def log_response token_response
           digest = Digest::SHA256.hexdigest response_hash["id_token"]
           response_hash["id_token"] = "(sha256:#{digest})"
         end
-        Google::Logging::Message.from(
-          message: "Received auth token response: #{response_hash}",
-          "credentialsId" => object_id
-        )
+        logger&.debug do
+          Google::Logging::Message.from(
+            message: "Received auth token response: #{response_hash}",
+            "credentialsId" => object_id
+          )
+        end
       end
 
       def log_auth_error err

lib/googleauth/user_refresh.rb

@@ -146,6 +146,8 @@ def revoke! options = {}
               principal: principal
             )
           end
+
+          resp.body
         end
       end
 

spec/googleauth/signet_spec.rb

@@ -176,6 +176,7 @@ def make_auth_stubs opts
         response = mocked_responses.shift
         response == :raise ? raise(Signet::RemoteServerError) : response
       end
+      expect(@client).to receive(:sleep).exactly(2).times.with(kind_of(Numeric))
       expect(@client.fetch_access_token!).to eq("success")
     end
 
@@ -185,6 +186,7 @@ def make_auth_stubs opts
         response = mocked_responses.shift
         response == :raise ? raise(Signet::RemoteServerError) : response
       end
+      expect(@client).to receive(:sleep).exactly(5).times.with(kind_of(Numeric))
       expect { @client.fetch_access_token! }.to raise_error Signet::AuthorizationError
     end
 

spec/googleauth/user_refresh_spec.rb

@@ -326,37 +326,53 @@ def cred_json_text_with_universe_domain missing = nil
   end
 
   describe "when revoking a refresh token" do
+    let(:response_body) { "{}" }
     let :stub do
       stub_request(:post, "https://oauth2.googleapis.com/revoke")
         .with(body: hash_including("token" => "refreshtoken"))
         .to_return(status:  200,
+                   body: response_body,
                    headers: { "Content-Type" => "application/json" })
     end
 
     before :example do
       stub
-      @client.revoke!
+      @result = @client.revoke!
     end
 
     it_behaves_like "revoked token"
+
+    # The return value is passed through retry_with_error's logging pipeline,
+    # which expects a JSON-parseable string.
+    it "returns the response body" do
+      expect(@result).to eq(response_body)
+    end
   end
 
   describe "when revoking an access token" do
+    let(:response_body) { "{}" }
     let :stub do
       stub_request(:post, "https://oauth2.googleapis.com/revoke")
         .with(body: hash_including("token" => "accesstoken"))
         .to_return(status:  200,
+                   body: response_body,
                    headers: { "Content-Type" => "application/json" })
     end
 
     before :example do
       stub
       @client.refresh_token = nil
       @client.access_token = "accesstoken"
-      @client.revoke!
+      @result = @client.revoke!
     end
 
     it_behaves_like "revoked token"
+
+    # The return value is passed through retry_with_error's logging pipeline,
+    # which expects a JSON-parseable string.
+    it "returns the response body" do
+      expect(@result).to eq(response_body)
+    end
   end
 
   describe "when revoking an invalid token" do
@@ -378,17 +394,54 @@ def cred_json_text_with_universe_domain missing = nil
     end
   end
 
+  describe "logging during revoke" do
+    let(:response_body) { '{"foo": "bar"}' }
+    let :stub do
+      stub_request(:post, "https://oauth2.googleapis.com/revoke")
+        .with(body: hash_including("token" => "refreshtoken"))
+        .to_return(status:  200,
+                   body: response_body,
+                   headers: { "Content-Type" => "application/json" })
+    end
+
+    it "logs the response body" do
+      stub
+      strio = StringIO.new
+      logger = Logger.new strio
+      logger.level = Logger::DEBUG
+      @client.logger = logger
+      @client.revoke!
+      expect(strio.string).to include("Received auth token response")
+    end
+
+    it "logs transient errors when they occur" do
+      allow_any_instance_of(Faraday::Connection).to receive(:post).and_raise(Faraday::TimeoutError)
+      strio = StringIO.new
+      logger = Logger.new strio
+      @client.logger = logger
+      
+      # Stub sleep to avoid slow tests
+      allow(@client).to receive(:sleep)
+
+      expect { @client.revoke! }.to raise_error Signet::AuthorizationError
+      expect(strio.string).to include("Transient error when fetching auth token")
+      expect(strio.string).to include("Exhausted retries when fetching auth token")
+    end
+  end
+
   describe "when errors occurred with request" do
     it "should fail with Signet::AuthorizationError if request times out" do
       allow_any_instance_of(Faraday::Connection).to receive(:post)
         .and_raise(Faraday::TimeoutError)
+      expect(@client).to receive(:sleep).exactly(5).times.with(kind_of(Numeric))
       expect { @client.revoke! }
         .to raise_error Signet::AuthorizationError
     end
 
     it "should fail with Signet::AuthorizationError if request fails" do
       allow_any_instance_of(Faraday::Connection).to receive(:post)
         .and_raise(Faraday::ConnectionFailed, nil)
+      expect(@client).to receive(:sleep).exactly(5).times.with(kind_of(Numeric))
       expect { @client.revoke! }
         .to raise_error Signet::AuthorizationError
     end

spec/spec_helper.rb

@@ -42,6 +42,10 @@
   config.include WebMock::API
   config.filter_run focus: true
   config.run_all_when_everything_filtered = true
+
+  config.before(:each) do
+    allow(Google::Auth::CredentialsLoader).to receive(:load_gcloud_project_id).and_return("my-project-id")
+  end
 end
 
 module TestHelpers

test/helper.rb

@@ -18,6 +18,17 @@
 
 require "googleauth"
 
+# Proactively stub the gcloud CLI call for all future Minitest tests
+module Google
+  module Auth
+    module CredentialsLoader
+      def load_gcloud_project_id
+        "my-project-id"
+      end
+    end
+  end
+end
+
 ##
 # A simple in-memory implementation of TokenStore
 # for UserAuthorizer initialization when testing