[TrustedVault] Handle HTTP_BAD_REQUEST response status

HTTP_BAD_REQUEST status needs to be special-cased, because it can be
used to distinguish inactionable (potentially transient) errors from
the case when local data is outdated and user action is required to go
out from this state.

Bug: 1113598
Change-Id: If7389a7ecf534139e7a610b1f7c8b93fdc25ce1f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2526348
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#830176}

components/sync/trusted_vault/trusted_vault_connection_impl.cc

@@ -35,12 +35,22 @@ void AssignBytesToProtoString(base::span<const uint8_t> bytes,
 
 void ProcessRegisterDeviceResponse(
     TrustedVaultConnection::RegisterAuthenticationFactorCallback callback,
-    bool success,
+    TrustedVaultRequest::HttpStatus http_status,
     const std::string& response_body) {
-  // TODO(crbug.com/1113598): distinguish kLocalDataObsolete and kOtherError
-  // (based on http error code?).
-  std::move(callback).Run(success ? TrustedVaultRequestStatus::kSuccess
-                                  : TrustedVaultRequestStatus::kOtherError);
+  TrustedVaultRequestStatus registration_status;
+  switch (http_status) {
+    case TrustedVaultRequest::HttpStatus::kSuccess:
+      registration_status = TrustedVaultRequestStatus::kSuccess;
+      break;
+    case TrustedVaultRequest::HttpStatus::kOtherError:
+      registration_status = TrustedVaultRequestStatus::kOtherError;
+      break;
+    case TrustedVaultRequest::HttpStatus::kBadRequest:
+      // Bad request response indicates that client data is outdated (e.g.
+      // locally available trusted vault key is not the recent one).
+      registration_status = TrustedVaultRequestStatus::kLocalDataObsolete;
+  }
+  std::move(callback).Run(registration_status);
 }
 
 std::vector<uint8_t> ComputeWrappedKey(

components/sync/trusted_vault/trusted_vault_connection_impl_unittest.cc

@@ -227,6 +227,29 @@ TEST_F(TrustedVaultConnectionImplTest,
       RespondToJoinSecurityDomainsRequest(net::HTTP_INTERNAL_SERVER_ERROR));
 }
 
+TEST_F(TrustedVaultConnectionImplTest,
+       ShouldHandleFailedJoinSecurityDomainsRequestWithBadRequestStatus) {
+  std::unique_ptr<SecureBoxKeyPair> key_pair = MakeTestKeyPair();
+  ASSERT_THAT(key_pair, NotNull());
+
+  base::MockCallback<
+      TrustedVaultConnection::RegisterAuthenticationFactorCallback>
+      callback;
+
+  std::unique_ptr<TrustedVaultConnection::Request> request =
+      connection()->RegisterAuthenticationFactor(
+          /*account_info=*/CoreAccountInfo(), kTrustedVaultKey,
+          /*last_trusted_vault_key_version=*/1, key_pair->public_key(),
+          callback.Get());
+  ASSERT_THAT(request, NotNull());
+
+  // In particular, HTTP_BAD_REQUEST indicates that
+  // |last_trusted_vault_key_version| is not actually the last on the server
+  // side.
+  EXPECT_CALL(callback, Run(Eq(TrustedVaultRequestStatus::kLocalDataObsolete)));
+  EXPECT_TRUE(RespondToJoinSecurityDomainsRequest(net::HTTP_BAD_REQUEST));
+}
+
 TEST_F(
     TrustedVaultConnectionImplTest,
     ShouldHandleAccessTokenFetchingFailureWhenRegisteringAuthenticationFactor) {

components/sync/trusted_vault/trusted_vault_request.cc

@@ -100,7 +100,7 @@ void TrustedVaultRequest::OnAccessTokenFetched(
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     base::Optional<signin::AccessTokenInfo> access_token_info) {
   if (!access_token_info.has_value()) {
-    RunCompletionCallbackAndMaybeDestroySelf(/*success=*/false,
+    RunCompletionCallbackAndMaybeDestroySelf(HttpStatus::kOtherError,
                                              /*response_body=*/std::string());
     return;
   }
@@ -120,12 +120,21 @@ void TrustedVaultRequest::OnURLLoadComplete(
   if (url_loader_->ResponseInfo() && url_loader_->ResponseInfo()->headers) {
     http_response_code = url_loader_->ResponseInfo()->headers->response_code();
   }
+  if (http_response_code == net::HTTP_BAD_REQUEST) {
+    // Bad request can indicate client-side data being obsolete, distinguish it
+    // to allow API users to decide how to handle.
+    RunCompletionCallbackAndMaybeDestroySelf(HttpStatus::kBadRequest,
+                                             std::string());
+    return;
+  }
   if (http_response_code != net::HTTP_OK &&
       http_response_code != net::HTTP_NO_CONTENT) {
-    RunCompletionCallbackAndMaybeDestroySelf(/*success=*/false, std::string());
+    RunCompletionCallbackAndMaybeDestroySelf(HttpStatus::kOtherError,
+                                             std::string());
     return;
   }
-  RunCompletionCallbackAndMaybeDestroySelf(/*success=*/true, *response_body);
+  RunCompletionCallbackAndMaybeDestroySelf(HttpStatus::kSuccess,
+                                           *response_body);
 }
 
 std::unique_ptr<network::SimpleURLLoader> TrustedVaultRequest::CreateURLLoader(
@@ -158,9 +167,9 @@ std::unique_ptr<network::SimpleURLLoader> TrustedVaultRequest::CreateURLLoader(
 }
 
 void TrustedVaultRequest::RunCompletionCallbackAndMaybeDestroySelf(
-    bool success,
+    HttpStatus status,
     const std::string& response_body) {
-  std::move(completion_callback_).Run(success, response_body);
+  std::move(completion_callback_).Run(status, response_body);
 }
 
 }  // namespace syncer

components/sync/trusted_vault/trusted_vault_request.h

@@ -32,11 +32,22 @@ class TrustedVaultAccessTokenFetcher;
 // Allows calling VaultService API using proto-over-http.
 class TrustedVaultRequest : public TrustedVaultConnection::Request {
  public:
-  using CompletionCallback =
-      base::OnceCallback<void(bool success, const std::string& response_body)>;
+  enum class HttpStatus {
+    // Reported when server returns http status code 200 or 204.
+    kSuccess,
+    // Reported when server return http status code 400 (bad request).
+    kBadRequest,
+    // Reported when other error occurs: unable to fetch access token, network
+    // and http errors (except 400).
+    kOtherError
+  };
 
   enum class HttpMethod { kGet, kPost };
 
+  using CompletionCallback =
+      base::OnceCallback<void(HttpStatus status,
+                              const std::string& response_body)>;
+
   // |callback| will be run upon completion and it's allowed to delete this
   // object upon |callback| call. For GET requests, |serialized_request_proto|
   // must be null. For |POST| requests, it can be either way (optional payload).
@@ -70,7 +81,7 @@ class TrustedVaultRequest : public TrustedVaultConnection::Request {
   // Running |completion_callback_| may cause destroying of this object, so all
   // callers of this method must not run any code afterwards.
   void RunCompletionCallbackAndMaybeDestroySelf(
-      bool success,
+      HttpStatus status,
       const std::string& response_body);
 
   const HttpMethod http_method_;

components/sync/trusted_vault/trusted_vault_request_unittest.cc

@@ -142,7 +142,9 @@ TEST_F(TrustedVaultRequestTest, ShouldSendGetRequestAndHandleSuccess) {
   EXPECT_THAT(network::GetUploadData(resource_request), IsEmpty());
 
   // |completion_callback| should be called after receiving response.
-  EXPECT_CALL(completion_callback, Run(/*success=*/true, Eq(kResponseBody)));
+  EXPECT_CALL(
+      completion_callback,
+      Run(TrustedVaultRequest::HttpStatus::kSuccess, Eq(kResponseBody)));
   EXPECT_TRUE(RespondToHttpRequest(net::OK, net::HTTP_OK, kResponseBody));
 }
 
@@ -165,7 +167,9 @@ TEST_F(TrustedVaultRequestTest,
   EXPECT_THAT(network::GetUploadData(resource_request), IsEmpty());
 
   // |completion_callback| should be called after receiving response.
-  EXPECT_CALL(completion_callback, Run(/*success=*/true, Eq(kResponseBody)));
+  EXPECT_CALL(
+      completion_callback,
+      Run(TrustedVaultRequest::HttpStatus::kSuccess, Eq(kResponseBody)));
   EXPECT_TRUE(RespondToHttpRequest(net::OK, net::HTTP_OK, kResponseBody));
 }
 
@@ -189,7 +193,9 @@ TEST_F(TrustedVaultRequestTest,
   EXPECT_THAT(network::GetUploadData(resource_request), Eq(kRequestBody));
 
   // |completion_callback| should be called after receiving response.
-  EXPECT_CALL(completion_callback, Run(/*success=*/true, Eq(kResponseBody)));
+  EXPECT_CALL(
+      completion_callback,
+      Run(TrustedVaultRequest::HttpStatus::kSuccess, Eq(kResponseBody)));
   EXPECT_TRUE(RespondToHttpRequest(net::OK, net::HTTP_OK, kResponseBody));
 }
 
@@ -201,7 +207,8 @@ TEST_F(TrustedVaultRequestTest, ShouldHandleNetworkFailures) {
       /*request_body=*/base::nullopt, completion_callback.Get());
 
   // |completion_callback| should be called after receiving response.
-  EXPECT_CALL(completion_callback, Run(/*success=*/false, _));
+  EXPECT_CALL(completion_callback,
+              Run(TrustedVaultRequest::HttpStatus::kOtherError, _));
   EXPECT_TRUE(RespondToHttpRequest(net::ERR_FAILED, base::nullopt,
                                    /*response_body=*/std::string()));
 }
@@ -214,17 +221,33 @@ TEST_F(TrustedVaultRequestTest, ShouldHandleHttpErrors) {
       /*request_body=*/base::nullopt, completion_callback.Get());
 
   // |completion_callback| should be called after receiving response.
-  EXPECT_CALL(completion_callback, Run(/*success=*/false, _));
+  EXPECT_CALL(completion_callback,
+              Run(TrustedVaultRequest::HttpStatus::kOtherError, _));
   EXPECT_TRUE(RespondToHttpRequest(net::OK, net::HTTP_INTERNAL_SERVER_ERROR,
                                    /*response_body=*/""));
 }
 
+TEST_F(TrustedVaultRequestTest, ShouldHandleBadRequestStatus) {
+  base::MockCallback<TrustedVaultRequest::CompletionCallback>
+      completion_callback;
+  std::unique_ptr<TrustedVaultRequest> request = StartNewRequestWithAccessToken(
+      kAccessToken, TrustedVaultRequest::HttpMethod::kGet,
+      /*request_body=*/base::nullopt, completion_callback.Get());
+
+  // |completion_callback| should be called after receiving response.
+  EXPECT_CALL(completion_callback,
+              Run(TrustedVaultRequest::HttpStatus::kBadRequest, _));
+  EXPECT_TRUE(RespondToHttpRequest(net::OK, net::HTTP_BAD_REQUEST,
+                                   /*response_body=*/""));
+}
+
 TEST_F(TrustedVaultRequestTest, ShouldHandleAccessTokenFetchingFailures) {
   base::MockCallback<TrustedVaultRequest::CompletionCallback>
       completion_callback;
   // Access token fetching failure propagated immediately in this test, so
   // |completion_callback| should be called immediately as well.
-  EXPECT_CALL(completion_callback, Run(/*success=*/false, _));
+  EXPECT_CALL(completion_callback,
+              Run(TrustedVaultRequest::HttpStatus::kOtherError, _));
   std::unique_ptr<TrustedVaultRequest> request = StartNewRequestWithAccessToken(
       /*access_token=*/base::nullopt, TrustedVaultRequest::HttpMethod::kGet,
       /*request_body=*/base::nullopt, completion_callback.Get());