[GCPW] Set correct timeout in BuildRequestAndFetchResultFromHttpService

Currently the timeout is enforced only on the lifetime of the thread making the request and the request itself is using default values. If a timeout is set that is greater than the system default then the timeout won't be as expected.

Bug: 1049803
Change-Id: I3146a5a99714d756c2e214b0db59fbd388a17e0a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2042260Reviewed-by: Tien Mai <tienmai@chromium.org>
Commit-Queue: Robin Lewis <wrlewis@google.com>
Cr-Commit-Position: refs/heads/master@{#740336}

chrome/credential_provider/gaiacp/gaia_credential_base_unittests.cc

@@ -2787,9 +2787,9 @@ TEST_F(GcpGaiaCredentialBaseTest, FullNameUpdated) {
 
 // Test event logs upload to GEM service with different failure scenarios.
 // Parameters are:
-// 1. int - 0: HTTP call to upload logs succeeds.
-//          1: Fails the upload call due to invalid response from the GEM
-//             http server.
+// 1. bool  true:  HTTP call to upload logs succeeds.
+//          false: Fails the upload call due to invalid response from the GEM
+//                 http server.
 // 2. int - The number of fake events to seed the fake event log with.
 class GcpGaiaCredentialBaseUploadEventLogsTest
     : public GcpGaiaCredentialBaseTest,
@@ -2822,7 +2822,7 @@ TEST_P(GcpGaiaCredentialBaseUploadEventLogsTest, UploadEventViewerLogs) {
                 kDefaultUsername, L"password", L"Full Name", L"comment",
                 base::UTF8ToUTF16(kDefaultGaiaId), base::string16(), &sid));
 
-  // Change token response to an invalid one.
+  // Change token response to an valid one.
   SetDefaultTokenHandleResponse(kDefaultValidTokenHandleResponse);
 
   fake_http_url_fetcher_factory()->SetFakeResponse(

chrome/credential_provider/gaiacp/win_http_url_fetcher.cc

@@ -354,6 +354,8 @@ HRESULT WinHttpUrlFetcher::BuildRequestAndFetchResultFromHttpService(
     const base::Value& request_dict,
     const base::TimeDelta& request_timeout,
     base::Optional<base::Value>* request_result) {
+  DCHECK(request_result);
+
   auto url_fetcher = WinHttpUrlFetcher::Create(request_url);
   if (!url_fetcher) {
     LOGFN(ERROR) << "Could not create valid fetcher for url="
@@ -383,6 +385,10 @@ HRESULT WinHttpUrlFetcher::BuildRequestAndFetchResultFromHttpService(
     }
   }
 
+  if (!request_timeout.is_zero()) {
+    url_fetcher->SetHttpRequestTimeout(request_timeout.InMilliseconds());
+  }
+
   auto extracted_param = (new HttpServiceRequest(std::move(url_fetcher)))
                              ->WaitForResponseFromHttpService(request_timeout);
 

chrome/credential_provider/gaiacp/win_http_url_fetcher_unittests.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
+#include "chrome/credential_provider/test/gls_runner_test_base.h"
+
+namespace credential_provider {
+
+namespace testing {
+
+// Test the WinHttpUrlFetcher::BuildRequestAndFetchResultFromHttpService method
+// used to make various HTTP requests.
+// Parameters are:
+// bool  true:  HTTP call succeeds.
+//       false: Fails due to invalid response from server.
+class GcpWinHttpUrlFetcherTest : public GlsRunnerTestBase,
+                                 public ::testing::WithParamInterface<bool> {};
+
+TEST_P(GcpWinHttpUrlFetcherTest,
+       BuildRequestAndFetchResultFromHttpServiceTest) {
+  bool invalid_response = GetParam();
+
+  const int timeout_in_millis = 12000;
+  const std::string header1 = "test-header-1";
+  const std::string header1_value = "test-value-1";
+  const GURL test_url =
+      GURL("https://test-service.googleapis.com/v1/testEndpoint");
+  const std::string access_token = "test-access-token";
+
+  base::Value request(base::Value::Type::DICTIONARY);
+  request.SetStringKey("request-str-key", "request-str-value");
+  request.SetIntKey("request-int-key", 1234);
+  base::TimeDelta request_timeout =
+      base::TimeDelta::FromMilliseconds(timeout_in_millis);
+  base::Optional<base::Value> request_result;
+
+  base::Value expected_result(base::Value::Type::DICTIONARY);
+  expected_result.SetStringKey("response-str-key", "response-str-value");
+  expected_result.SetIntKey("response-int-key", 4321);
+  std::string expected_response;
+  base::JSONWriter::Write(expected_result, &expected_response);
+
+  fake_http_url_fetcher_factory()->SetFakeResponse(
+      test_url, FakeWinHttpUrlFetcher::Headers(),
+      invalid_response ? "Invalid json response" : expected_response);
+  fake_http_url_fetcher_factory()->SetCollectRequestData(true);
+
+  HRESULT hr = WinHttpUrlFetcher::BuildRequestAndFetchResultFromHttpService(
+      test_url, access_token, {{header1, header1_value}}, request,
+      request_timeout, &request_result);
+
+  if (invalid_response) {
+    ASSERT_TRUE(FAILED(hr));
+  } else {
+    ASSERT_EQ(S_OK, hr);
+    ASSERT_EQ(expected_result, request_result.value());
+  }
+
+  ASSERT_TRUE(fake_http_url_fetcher_factory()->requests_created() > 0);
+  for (size_t idx = 0;
+       idx < fake_http_url_fetcher_factory()->requests_created(); ++idx) {
+    FakeWinHttpUrlFetcherFactory::RequestData request_data =
+        fake_http_url_fetcher_factory()->GetRequestData(idx);
+
+    ASSERT_EQ(timeout_in_millis, request_data.timeout_in_millis);
+    ASSERT_EQ(1u, request_data.headers.count("Authorization"));
+    ASSERT_NE(std::string::npos,
+              request_data.headers.at("Authorization").find(access_token));
+    ASSERT_EQ(1u, request_data.headers.count(header1));
+    ASSERT_EQ(header1_value, request_data.headers.at(header1));
+    base::Optional<base::Value> body_value =
+        base::JSONReader::Read(request_data.body);
+    ASSERT_EQ(request, body_value.value());
+  }
+}
+
+INSTANTIATE_TEST_SUITE_P(All,
+                         GcpWinHttpUrlFetcherTest,
+                         ::testing::Values(true, false));
+
+}  // namespace testing
+}  // namespace credential_provider

chrome/credential_provider/test/BUILD.gn

@@ -13,6 +13,7 @@ test("gcp_unittests") {
     "../gaiacp/gaia_credential_unittests.cc",
     "../gaiacp/gcp_utils_unittests.cc",
     "../gaiacp/reauth_credential_unittests.cc",
+    "../gaiacp/win_http_url_fetcher_unittests.cc",
     "com_fakes.cc",
     "com_fakes.h",
     "gcp_fakes.cc",

chrome/credential_provider/test/gcp_fakes.cc

@@ -582,6 +582,16 @@ HRESULT FakeScopedUserProfile::SaveAccountInfo(const base::Value& properties) {
 
 ///////////////////////////////////////////////////////////////////////////////
 
+FakeWinHttpUrlFetcherFactory::RequestData::RequestData()
+    : timeout_in_millis(-1) {}  // Set default timeout to an invalid value.
+
+FakeWinHttpUrlFetcherFactory::RequestData::RequestData(const RequestData& rhs)
+    : headers(rhs.headers),
+      body(rhs.body),
+      timeout_in_millis(rhs.timeout_in_millis) {}
+
+FakeWinHttpUrlFetcherFactory::RequestData::~RequestData() = default;
+
 FakeWinHttpUrlFetcherFactory::Response::Response() {}
 
 FakeWinHttpUrlFetcherFactory::Response::Response(const Response& rhs)
@@ -625,6 +635,13 @@ void FakeWinHttpUrlFetcherFactory::SetFakeFailedResponse(const GURL& url,
   failed_http_fetch_hr_[url] = failed_hr;
 }
 
+FakeWinHttpUrlFetcherFactory::RequestData
+FakeWinHttpUrlFetcherFactory::GetRequestData(size_t request_index) const {
+  if (request_index < requests_data_.size())
+    return requests_data_[request_index];
+  return RequestData();
+}
+
 std::unique_ptr<WinHttpUrlFetcher> FakeWinHttpUrlFetcherFactory::Create(
     const GURL& url) {
   if (fake_responses_.count(url) == 0 && failed_http_fetch_hr_.count(url) == 0)
@@ -642,6 +659,12 @@ std::unique_ptr<WinHttpUrlFetcher> FakeWinHttpUrlFetcherFactory::Create(
     DCHECK(failed_http_fetch_hr_.count(url) > 0);
     fetcher->response_hr_ = failed_http_fetch_hr_[url];
   }
+
+  if (collect_request_data_) {
+    requests_data_.push_back(RequestData());
+    fetcher->request_data_ = &requests_data_.back();
+  }
+
   ++requests_created_;
 
   return std::unique_ptr<WinHttpUrlFetcher>(fetcher);
@@ -672,6 +695,26 @@ HRESULT FakeWinHttpUrlFetcher::Close() {
   return S_OK;
 }
 
+HRESULT FakeWinHttpUrlFetcher::SetRequestHeader(const char* name,
+                                                const char* value) {
+  if (request_data_)
+    request_data_->headers[name] = value;
+  return S_OK;
+}
+
+HRESULT FakeWinHttpUrlFetcher::SetRequestBody(const char* body) {
+  if (request_data_)
+    request_data_->body = body;
+  return S_OK;
+}
+
+HRESULT FakeWinHttpUrlFetcher::SetHttpRequestTimeout(
+    const int timeout_in_millis) {
+  if (request_data_)
+    request_data_->timeout_in_millis = timeout_in_millis;
+  return S_OK;
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 
 FakeAssociatedUserValidator::FakeAssociatedUserValidator()

chrome/credential_provider/test/gcp_fakes.h

@@ -308,6 +308,23 @@ class FakeWinHttpUrlFetcherFactory {
   // to this method.
   void SetFakeFailedResponse(const GURL& url, HRESULT failed_hr);
 
+  // Sets the option to collect request data for each URL fetcher created.
+  void SetCollectRequestData(bool value) { collect_request_data_ = value; }
+
+  // Data used to make each HTTP request by the fetcher.
+  struct RequestData {
+    RequestData();
+    RequestData(const RequestData& rhs);
+    ~RequestData();
+    WinHttpUrlFetcher::Headers headers;
+    std::string body;
+    int timeout_in_millis;
+  };
+
+  // Returns the request data for the request identified by |request_index|.
+  RequestData GetRequestData(size_t request_index) const;
+
+  // Returns the number of requests created.
   size_t requests_created() const { return requests_created_; }
 
  private:
@@ -330,6 +347,8 @@ class FakeWinHttpUrlFetcherFactory {
   std::map<GURL, Response> fake_responses_;
   std::map<GURL, HRESULT> failed_http_fetch_hr_;
   size_t requests_created_ = 0;
+  bool collect_request_data_ = false;
+  std::vector<RequestData> requests_data_;
 };
 
 class FakeWinHttpUrlFetcher : public WinHttpUrlFetcher {
@@ -343,16 +362,21 @@ class FakeWinHttpUrlFetcher : public WinHttpUrlFetcher {
 
   // WinHttpUrlFetcher
   bool IsValid() const override;
+  HRESULT SetRequestHeader(const char* name, const char* value) override;
+  HRESULT SetRequestBody(const char* body) override;
+  HRESULT SetHttpRequestTimeout(const int timeout_in_millis) override;
   HRESULT Fetch(std::vector<char>* response) override;
   HRESULT Close() override;
 
  private:
   friend FakeWinHttpUrlFetcherFactory;
+  typedef FakeWinHttpUrlFetcherFactory::RequestData RequestData;
 
   Headers response_headers_;
   std::string response_;
   HANDLE send_response_event_handle_;
   HRESULT response_hr_ = S_OK;
+  RequestData* request_data_ = nullptr;
 };
 
 ///////////////////////////////////////////////////////////////////////////////