[GCPW] Add retry mechanism for WinHttpUrlFetcher

Failed HTTP calls in BuildRequestAndFetchResultFromHttpService are re-tried specified number of times before failing.

Bug: 1064502
Change-Id: Id7f89ea42ee8f6076052b9a9f3bd2d84f9ca57e1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2119668Reviewed-by: Rakesh Soma <rakeshsoma@google.com>
Reviewed-by: Tien Mai <tienmai@chromium.org>
Reviewed-by: Yusuf Sengul <yusufsn@google.com>
Commit-Queue: Robin Lewis <wrlewis@google.com>
Cr-Commit-Position: refs/heads/master@{#753757}

chrome/credential_provider/gaiacp/event_logs_upload_manager.cc

@@ -56,6 +56,9 @@ constexpr int kDefaultNumberOfEventsToRead = 10;
 // Maximum number of upload requests to make per upload invocation.
 constexpr int kMaxAllowedNumberOfUploadRequests = 5;
 
+// Maximum number of retries if a HTTP call to the backend fails.
+constexpr unsigned int kMaxNumHttpRetries = 3;
+
 // Maximum size of the log entries payload in bytes per HTTP request.
 // TODO (crbug.com/1043195): Change this to use an experiment flag once an
 // experiment framework for GCPW is available.
@@ -498,7 +501,7 @@ HRESULT EventLogsUploadManager::MakeUploadLogChunkRequest(
   hr = WinHttpUrlFetcher::BuildRequestAndFetchResultFromHttpService(
       EventLogsUploadManager::Get()->GetGcpwServiceUploadEventViewerLogsUrl(),
       access_token, {}, request_dict, kDefaultUploadLogsRequestTimeout,
-      &request_result);
+      kMaxNumHttpRetries, &request_result);
 
   if (FAILED(hr)) {
     LOGFN(ERROR) << "BuildRequestAndFetchResultFromHttpService hr="
@@ -506,13 +509,6 @@ HRESULT EventLogsUploadManager::MakeUploadLogChunkRequest(
     return hr;
   }
 
-  if (!request_result.has_value() ||
-      request_result->FindDictKey(kErrorKeyInRequestResult)) {
-    LOGFN(ERROR) << "error="
-                 << *request_result->FindDictKey(kErrorKeyInRequestResult);
-    return E_FAIL;
-  }
-
   // Store the chunk id which is the last uploaded event log id
   // in registry so we know where to start next time.
   SetGlobalFlag(kEventLogUploadLastUploadedIdRegKey, chunk_id);

chrome/credential_provider/gaiacp/gem_device_details_manager.cc

@@ -49,6 +49,9 @@ const char kIsAdJoinedUserParameterName[] = "is_ad_joined_user";
 const char kMacAddressParameterName[] = "wlan_mac_addr";
 const char kUploadDeviceDetailsResponseDeviceResourceIdParameterName[] =
     "deviceResourceId";
+
+// Maximum number of retries if a HTTP call to the backend fails.
+constexpr unsigned int kMaxNumHttpRetries = 3;
 }  // namespace
 
 // static
@@ -127,7 +130,7 @@ HRESULT GemDeviceDetailsManager::UploadDeviceDetails(
   hr = WinHttpUrlFetcher::BuildRequestAndFetchResultFromHttpService(
       GemDeviceDetailsManager::Get()->GetGemServiceUploadDeviceDetailsUrl(),
       access_token, {}, *request_dict_, upload_device_details_request_timeout_,
-      &request_result);
+      kMaxNumHttpRetries, &request_result);
 
   if (FAILED(hr)) {
     LOGFN(ERROR) << "BuildRequestAndFetchResultFromHttpService hr="
@@ -135,13 +138,6 @@ HRESULT GemDeviceDetailsManager::UploadDeviceDetails(
     return E_FAIL;
   }
 
-  base::Value* error_detail =
-      request_result->FindDictKey(kErrorKeyInRequestResult);
-  if (error_detail) {
-    LOGFN(ERROR) << "error=" << *error_detail;
-    hr = E_FAIL;
-  }
-
   std::string* resource_id = request_result->FindStringKey(
       kUploadDeviceDetailsResponseDeviceResourceIdParameterName);
   if (resource_id) {

chrome/credential_provider/gaiacp/password_recovery_manager.cc

@@ -80,6 +80,9 @@ constexpr size_t kNonceLength = 12;
 
 constexpr size_t kSessionKeyLength = 32;
 
+// Maximum number of retries if a HTTP call to the backend fails.
+constexpr unsigned int kMaxNumHttpRetries = 3;
+
 bool Base64DecodeCryptographicKey(const std::string& cryptographic_key,
                                   std::string* out) {
   std::string cryptographic_key_copy;
@@ -275,7 +278,8 @@ HRESULT EncryptUserPasswordUsingEscrowService(
   // |public_key| to be used for encryption.
   HRESULT hr = WinHttpUrlFetcher::BuildRequestAndFetchResultFromHttpService(
       PasswordRecoveryManager::Get()->GetEscrowServiceGenerateKeyPairUrl(),
-      access_token, {}, request_dict, request_timeout, &request_result);
+      access_token, {}, request_dict, request_timeout, kMaxNumHttpRetries,
+      &request_result);
 
   if (FAILED(hr)) {
     LOGFN(ERROR) << "BuildRequestAndFetchResultFromHttpService hr="
@@ -363,7 +367,8 @@ HRESULT DecryptUserPasswordUsingEscrowService(
   HRESULT hr = WinHttpUrlFetcher::BuildRequestAndFetchResultFromHttpService(
       PasswordRecoveryManager::Get()->GetEscrowServiceGetPrivateKeyUrl(
           *resource_id),
-      access_token, {}, {}, request_timeout, &request_result);
+      access_token, {}, {}, request_timeout, kMaxNumHttpRetries,
+      &request_result);
 
   if (FAILED(hr)) {
     LOGFN(ERROR) << "BuildRequestAndFetchResultFromHttpService hr="

chrome/credential_provider/gaiacp/win_http_url_fetcher.cc

@@ -10,6 +10,8 @@
 #include <atlconv.h>
 #include <process.h>
 
+#include <set>
+
 #include "base/base64.h"
 #include "base/containers/span.h"
 #include "base/json/json_reader.h"
@@ -20,6 +22,18 @@
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/credential_provider/gaiacp/logging.h"
+#include "chrome/credential_provider/gaiacp/mdm_utils.h"
+
+namespace {
+// Key name containing the HTTP error code within the dictionary returned by the
+// server in case of errors.
+constexpr char kHttpErrorCodeKeyNameInResponse[] = "code";
+
+// The HTTP response codes for which the request is re-tried on failure.
+const std::set<int> kRetryableHttpErrorCodes = {
+    503,  // Service Unavailable
+    504   // Gateway Timeout
+};
 
 // Self deleting http service requester. This class will try to make a query
 // using the given url fetcher. It will delete itself when the request is
@@ -41,11 +55,12 @@
 //       thread can self delete.
 class HttpServiceRequest {
  public:
-  explicit HttpServiceRequest(
-      std::unique_ptr<credential_provider::WinHttpUrlFetcher> fetcher)
-      : fetcher_(std::move(fetcher)) {
-    DCHECK(fetcher_);
-  }
+  static HttpServiceRequest* Create(
+      const GURL& request_url,
+      const std::string& access_token,
+      const std::vector<std::pair<std::string, std::string>>& headers,
+      const std::string& request_body,
+      const base::TimeDelta& request_timeout);
 
   // Tries to fetch the request stored in |fetcher_| in a background thread
   // within the given |request_timeout|. If the background thread returns before
@@ -103,6 +118,12 @@ class HttpServiceRequest {
   }
 
  private:
+  explicit HttpServiceRequest(
+      std::unique_ptr<credential_provider::WinHttpUrlFetcher> fetcher)
+      : fetcher_(std::move(fetcher)) {
+    DCHECK(fetcher_);
+  }
+
   void OrphanRequest() {
     bool delete_self = false;
     {
@@ -158,6 +179,44 @@ class HttpServiceRequest {
   bool is_processing_ = true;
 };
 
+HttpServiceRequest* HttpServiceRequest::Create(
+    const GURL& request_url,
+    const std::string& access_token,
+    const std::vector<std::pair<std::string, std::string>>& headers,
+    const std::string& request_body,
+    const base::TimeDelta& request_timeout) {
+  auto url_fetcher =
+      credential_provider::WinHttpUrlFetcher::Create(request_url);
+  if (!url_fetcher) {
+    LOGFN(ERROR) << "Could not create valid fetcher for url="
+                 << request_url.spec();
+    return nullptr;
+  }
+
+  url_fetcher->SetRequestHeader("Content-Type", "application/json");
+  url_fetcher->SetRequestHeader("Authorization",
+                                ("Bearer " + access_token).c_str());
+  for (auto& header : headers)
+    url_fetcher->SetRequestHeader(header.first.c_str(), header.second.c_str());
+
+  if (!request_body.empty()) {
+    HRESULT hr = url_fetcher->SetRequestBody(request_body.c_str());
+    if (FAILED(hr)) {
+      LOGFN(ERROR) << "fetcher.SetRequestBody hr="
+                   << credential_provider::putHR(hr);
+      return nullptr;
+    }
+  }
+
+  if (!request_timeout.is_zero()) {
+    url_fetcher->SetHttpRequestTimeout(request_timeout.InMilliseconds());
+  }
+
+  return (new HttpServiceRequest(std::move(url_fetcher)));
+}
+
+}  // namespace
+
 namespace credential_provider {
 
 // static
@@ -353,49 +412,55 @@ HRESULT WinHttpUrlFetcher::BuildRequestAndFetchResultFromHttpService(
     const std::vector<std::pair<std::string, std::string>>& headers,
     const base::Value& request_dict,
     const base::TimeDelta& request_timeout,
+    unsigned int request_retries,
     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="
-                 << request_url.spec();
-    return E_FAIL;
-  }
-
-  url_fetcher->SetRequestHeader("Content-Type", "application/json");
-  url_fetcher->SetRequestHeader("Authorization",
-                                ("Bearer " + access_token).c_str());
-  for (auto& header : headers)
-    url_fetcher->SetRequestHeader(header.first.c_str(), header.second.c_str());
-
   HRESULT hr = S_OK;
 
+  std::string request_body;
   if (request_dict.is_dict()) {
-    std::string body;
-    if (!base::JSONWriter::Write(request_dict, &body)) {
+    if (!base::JSONWriter::Write(request_dict, &request_body)) {
       LOGFN(ERROR) << "base::JSONWriter::Write failed";
       return E_FAIL;
     }
-
-    hr = url_fetcher->SetRequestBody(body.c_str());
-    if (FAILED(hr)) {
-      LOGFN(ERROR) << "fetcher.SetRequestBody hr=" << putHR(hr);
-      return E_FAIL;
-    }
   }
 
-  if (!request_timeout.is_zero()) {
-    url_fetcher->SetHttpRequestTimeout(request_timeout.InMilliseconds());
-  }
+  for (unsigned int try_count = 0; try_count <= request_retries; ++try_count) {
+    HttpServiceRequest* request = HttpServiceRequest::Create(
+        request_url, access_token, headers, request_body, request_timeout);
+
+    if (!request)
+      return E_FAIL;
 
-  auto extracted_param = (new HttpServiceRequest(std::move(url_fetcher)))
-                             ->WaitForResponseFromHttpService(request_timeout);
+    auto extracted_param =
+        request->WaitForResponseFromHttpService(request_timeout);
 
-  if (!extracted_param)
-    return E_FAIL;
+    if (!extracted_param) {
+      hr = E_FAIL;
+      continue;
+    }
+    *request_result = std::move(extracted_param);
+
+    base::Value* error_detail =
+        (*request_result)->FindDictKey(kErrorKeyInRequestResult);
+    if (error_detail) {
+      hr = E_FAIL;
+      LOGFN(ERROR) << "error: " << *error_detail;
+
+      // If error code is known, retry only on retryable server errors.
+      base::Optional<int> error_code =
+          error_detail->FindIntKey(kHttpErrorCodeKeyNameInResponse);
+      if (error_code.has_value() &&
+          kRetryableHttpErrorCodes.find(error_code.value()) ==
+              kRetryableHttpErrorCodes.end())
+        break;
+
+      continue;
+    }
 
-  *request_result = std::move(extracted_param);
+    hr = S_OK;
+    break;
+  }
 
   return hr;
 }

chrome/credential_provider/gaiacp/win_http_url_fetcher.h

@@ -30,13 +30,17 @@ class WinHttpUrlFetcher {
   // value pairs to be sent with the request. |request_dict| is a dictionary of
   // json parameters to be sent with the request. This argument will be
   // converted to a json string and sent as the body of the request.
-  // |request_timeout| is the maximum time to wait for a response.
+  // |request_timeout| is the maximum time to wait for a response. If the HTTP
+  // request times out or fails with an error response code that signifies an
+  // internal server error (HTTP codes >= 500) then the request will be retried
+  // |request_retries| number of times before the call is marked failed.
   static HRESULT BuildRequestAndFetchResultFromHttpService(
       const GURL& request_url,
       std::string access_token,
       const std::vector<std::pair<std::string, std::string>>& headers,
       const base::Value& request_dict,
       const base::TimeDelta& request_timeout,
+      unsigned int request_retries,
       base::Optional<base::Value>* request_result);
 
   virtual ~WinHttpUrlFetcher();

chrome/credential_provider/gaiacp/win_http_url_fetcher_unittests.cc

@@ -13,14 +13,22 @@ 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> {};
+// 1.  int - 0:  HTTP call succeeds.
+//           1:  Fails due to invalid response from server.
+//           2:  Fails due to a retryable HTTP error (like 503).
+//           3:  Fails due to a non-retryable HTTP error (like 404).
+// 2.  int - Number of retries allowed for a HTTP request.
+class GcpWinHttpUrlFetcherTest
+    : public GlsRunnerTestBase,
+      public ::testing::WithParamInterface<std::tuple<int, int>> {};
 
 TEST_P(GcpWinHttpUrlFetcherTest,
        BuildRequestAndFetchResultFromHttpServiceTest) {
-  bool invalid_response = GetParam();
+  bool valid_response = std::get<0>(GetParam()) == 0;
+  bool invalid_response = std::get<0>(GetParam()) == 1;
+  bool retryable_error_response = std::get<0>(GetParam()) == 2;
+  bool nonretryable_error_response = std::get<0>(GetParam()) == 3;
+  int num_retries = std::get<1>(GetParam());
 
   const int timeout_in_millis = 12000;
   const std::string header1 = "test-header-1";
@@ -42,23 +50,64 @@ TEST_P(GcpWinHttpUrlFetcherTest,
   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);
+  std::string response;
+  if (invalid_response) {
+    response = "Invalid json response";
+  } else if (retryable_error_response) {
+    response =
+        "{\n\"error\": {"
+        "\"code\": 503,\n"
+        "\"message\": \"Service unavailable\","
+        "\"status\": \"UNAVAILABLE\"\n}\n}";
+  } else if (nonretryable_error_response) {
+    response =
+        "{\n\"error\": {"
+        "\"code\": 403,\n"
+        "\"message\": \"The caller does not have permission\","
+        "\"status\": \"PERMISSION_DENIED\"\n}\n}";
+  } else {
+    response = expected_response;
+  }
+
+  if (num_retries == 0) {
+    fake_http_url_fetcher_factory()->SetFakeResponse(
+        test_url, FakeWinHttpUrlFetcher::Headers(), response);
+  } else {
+    fake_http_url_fetcher_factory()->SetFakeResponseForSpecifiedNumRequests(
+        test_url, FakeWinHttpUrlFetcher::Headers(), response, num_retries);
+    fake_http_url_fetcher_factory()->SetFakeResponseForSpecifiedNumRequests(
+        test_url, FakeWinHttpUrlFetcher::Headers(), expected_response, 1);
+  }
   fake_http_url_fetcher_factory()->SetCollectRequestData(true);
 
   HRESULT hr = WinHttpUrlFetcher::BuildRequestAndFetchResultFromHttpService(
       test_url, access_token, {{header1, header1_value}}, request,
-      request_timeout, &request_result);
+      request_timeout, num_retries, &request_result);
 
-  if (invalid_response) {
-    ASSERT_TRUE(FAILED(hr));
+  if (num_retries == 0) {
+    if (invalid_response || retryable_error_response ||
+        nonretryable_error_response) {
+      ASSERT_TRUE(FAILED(hr));
+    } else {
+      ASSERT_EQ(S_OK, hr);
+      ASSERT_EQ(expected_result, request_result.value());
+    }
+  } else {
+    if (nonretryable_error_response) {
+      ASSERT_TRUE(FAILED(hr));
+    } else {
+      ASSERT_EQ(S_OK, hr);
+      ASSERT_EQ(expected_result, request_result.value());
+    }
+  }
+
+  if (valid_response || nonretryable_error_response) {
+    ASSERT_EQ(1UL, fake_http_url_fetcher_factory()->requests_created());
   } else {
-    ASSERT_EQ(S_OK, hr);
-    ASSERT_EQ(expected_result, request_result.value());
+    ASSERT_EQ(num_retries + 1UL,
+              fake_http_url_fetcher_factory()->requests_created());
   }
 
-  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 =
@@ -78,7 +127,8 @@ TEST_P(GcpWinHttpUrlFetcherTest,
 
 INSTANTIATE_TEST_SUITE_P(All,
                          GcpWinHttpUrlFetcherTest,
-                         ::testing::Values(true, false));
+                         ::testing::Combine(::testing::Values(0, 1, 2, 3),
+                                            ::testing::Values(0, 1, 3)));
 
 }  // namespace testing
 }  // namespace credential_provider

chrome/credential_provider/test/gcp_fakes.cc

@@ -641,8 +641,26 @@ void FakeWinHttpUrlFetcherFactory::SetFakeResponse(
     const WinHttpUrlFetcher::Headers& headers,
     const std::string& response,
     HANDLE send_response_event_handle /*=INVALID_HANDLE_VALUE*/) {
-  fake_responses_[url] =
-      Response(headers, response, send_response_event_handle);
+  fake_responses_[url].clear();
+  fake_responses_[url].push_back(
+      Response(headers, response, send_response_event_handle));
+  remove_fake_response_when_created_ = false;
+}
+
+void FakeWinHttpUrlFetcherFactory::SetFakeResponseForSpecifiedNumRequests(
+    const GURL& url,
+    const WinHttpUrlFetcher::Headers& headers,
+    const std::string& response,
+    unsigned int num_requests,
+    HANDLE send_response_event_handle /* =INVALID_HANDLE_VALUE */) {
+  if (fake_responses_.find(url) == fake_responses_.end()) {
+    fake_responses_[url] = std::deque<Response>();
+  }
+  for (unsigned int i = 0; i < num_requests; ++i) {
+    fake_responses_[url].push_back(
+        Response(headers, response, send_response_event_handle));
+  }
+  remove_fake_response_when_created_ = true;
 }
 
 void FakeWinHttpUrlFetcherFactory::SetFakeFailedResponse(const GURL& url,
@@ -667,11 +685,17 @@ std::unique_ptr<WinHttpUrlFetcher> FakeWinHttpUrlFetcherFactory::Create(
   FakeWinHttpUrlFetcher* fetcher = new FakeWinHttpUrlFetcher(std::move(url));
 
   if (fake_responses_.count(url) != 0) {
-    const Response& response = fake_responses_[url];
+    const Response& response = fake_responses_[url].front();
 
     fetcher->response_headers_ = response.headers;
     fetcher->response_ = response.response;
     fetcher->send_response_event_handle_ = response.send_response_event_handle;
+
+    if (remove_fake_response_when_created_) {
+      fake_responses_[url].pop_front();
+      if (fake_responses_[url].empty())
+        fake_responses_.erase(url);
+    }
   } else {
     DCHECK(failed_http_fetch_hr_.count(url) > 0);
     fetcher->response_hr_ = failed_http_fetch_hr_[url];

chrome/credential_provider/test/gcp_fakes.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_CREDENTIAL_PROVIDER_TEST_GCP_FAKES_H_
 #define CHROME_CREDENTIAL_PROVIDER_TEST_GCP_FAKES_H_
 
+#include <deque>
 #include <map>
 #include <memory>
 #include <string>
@@ -296,12 +297,23 @@ class FakeWinHttpUrlFetcherFactory {
   FakeWinHttpUrlFetcherFactory();
   ~FakeWinHttpUrlFetcherFactory();
 
+  // Sets the given |response| for any number of HTTP requests made for |url|.
   void SetFakeResponse(
       const GURL& url,
       const WinHttpUrlFetcher::Headers& headers,
       const std::string& response,
       HANDLE send_response_event_handle = INVALID_HANDLE_VALUE);
 
+  // Queues the given |response| for the specified |num_requests| number of HTTP
+  // requests made for |url|. Different responses for the URL can be set by
+  // calling this function multiple times with different responses.
+  void SetFakeResponseForSpecifiedNumRequests(
+      const GURL& url,
+      const WinHttpUrlFetcher::Headers& headers,
+      const std::string& response,
+      unsigned int num_requests,
+      HANDLE send_response_event_handle = INVALID_HANDLE_VALUE);
+
   // Sets the response as a failed http attempt. The return result
   // from http_url_fetcher.Fetch() would be set as the input HRESULT
   // to this method.
@@ -343,10 +355,11 @@ class FakeWinHttpUrlFetcherFactory {
     HANDLE send_response_event_handle;
   };
 
-  std::map<GURL, Response> fake_responses_;
+  std::map<GURL, std::deque<Response>> fake_responses_;
   std::map<GURL, HRESULT> failed_http_fetch_hr_;
   size_t requests_created_ = 0;
   bool collect_request_data_ = false;
+  bool remove_fake_response_when_created_ = false;
   std::vector<RequestData> requests_data_;
 };