[Autofill Assistant] Fix 403 for users without access token.

This CL also adds more log messages to make debugging those kinds of
errors easier in the future.

The bug was introduced in a refactoring some time ago, where the special
handling for empty access tokens was lost and there was no unit test to
catch this.

Bug: b/170934170
Change-Id: Ib251a84fd032f6b72b278bc87aad2a8f48c8df9b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2517441
Commit-Queue: Clemens Arbesser <arbesser@google.com>
Reviewed-by: Marian Fechete <marianfe@google.com>
Reviewed-by: Sandro Maggi <sandromaggi@google.com>
Cr-Commit-Position: refs/heads/master@{#823568}

components/autofill_assistant/browser/service/service_request_sender.cc

@@ -102,6 +102,7 @@ void SendRequestNoAuth(
   add_key.SetQueryStr(query_str);
   GURL modified_url = url.ReplaceComponents(add_key);
 
+  VLOG(2) << "Sending request with api key to backend";
   SendRequestImpl(CreateResourceRequest(modified_url), request_body, context,
                   loader_factory, std::move(callback));
 }
@@ -153,17 +154,17 @@ void ServiceRequestSender::OnFetchAccessToken(GURL url,
                                               ResponseCallback callback,
                                               bool access_token_fetched,
                                               const std::string& access_token) {
-  if (!access_token_fetched) {
+  if (!access_token_fetched || access_token.empty()) {
     if (disable_auth_if_no_access_token_) {
       // Give up on authentication for this run. Without access token, requests
       // might be successful or rejected, depending on the server configuration.
       auth_enabled_ = false;
+      VLOG(1) << "No access token, falling back to api key";
       SendRequestNoAuth(url, request_body, context_, loader_factory_.get(),
                         api_key_, std::move(callback));
       return;
     }
-    VLOG(1) << "Failed to fetch access token, but "
-               "disable_auth_if_no_access_token not set";
+    VLOG(1) << "No access token, but disable_auth_if_no_access_token not set";
     std::move(callback).Run(net::HTTP_UNAUTHORIZED, std::string());
     return;
   }
@@ -187,6 +188,7 @@ void ServiceRequestSender::SendRequestAuth(const GURL& url,
                               weak_ptr_factory_.GetWeakPtr(), url, access_token,
                               request_body, std::move(callback));
   }
+  VLOG(2) << "Sending request with access token to backend";
   SendRequestImpl(std::move(resource_request), request_body, context_,
                   loader_factory_.get(), std::move(callback));
 }
@@ -199,6 +201,8 @@ void ServiceRequestSender::RetryIfUnauthorized(const GURL& url,
                                                const std::string& response) {
   // On first UNAUTHORIZED error, invalidate access token and try again.
   if (auth_enabled_ && http_status == net::HTTP_UNAUTHORIZED) {
+    VLOG(1) << "Request with access token returned with 401 UNAUTHORIZED, "
+               "fetching a fresh access token and trying again";
     DCHECK(!retried_with_fresh_access_token_);
     retried_with_fresh_access_token_ = true;
     access_token_fetcher_->InvalidateAccessToken(access_token);

components/autofill_assistant/browser/service/service_request_sender_unittest.cc

@@ -141,6 +141,91 @@ TEST_F(ServiceRequestSenderTest, SendAuthenticatedRequest) {
                              mock_response_callback_.Get());
 }
 
+TEST_F(ServiceRequestSenderTest,
+       AuthRequestFallsBackToApiKeyOnEmptyAccessToken) {
+  EXPECT_CALL(mock_access_token_fetcher_, OnFetchAccessToken)
+      .Times(1)
+      .WillOnce(RunOnceCallback<0>(true, /*access_token = */ ""));
+
+  auto loader_factory =
+      std::make_unique<NiceMock<MockSimpleURLLoaderFactory>>();
+  auto loader = std::make_unique<NiceMock<MockURLLoader>>();
+  EXPECT_CALL(*loader_factory, OnCreateLoader(_, _))
+      .WillOnce([&](::network::ResourceRequest* resource_request,
+                    const ::net::NetworkTrafficAnnotationTag& annotation_tag) {
+        EXPECT_FALSE(resource_request->headers.HasHeader("Authorization"));
+        EXPECT_EQ(resource_request->url,
+                  GURL("https://www.example.com/?key=fake_api_key"));
+        return std::move(loader);
+      });
+  EXPECT_CALL(*loader,
+              AttachStringForUpload(std::string("request"),
+                                    std::string("application/x-protobuffer")))
+      .Times(1);
+  EXPECT_CALL(*loader, DownloadToStringOfUnboundedSizeUntilCrashAndDie(_, _))
+      .WillOnce(WithArgs<1>([&](auto&& callback) {
+        std::move(callback).Run(std::make_unique<std::string>("response"));
+      }));
+  auto response_info = CreateResponseInfo(net::HTTP_OK, "OK");
+  EXPECT_CALL(*loader, ResponseInfo)
+      .WillRepeatedly(Return(response_info.get()));
+
+  EXPECT_CALL(mock_response_callback_, Run(net::HTTP_OK, "response"));
+  ServiceRequestSender request_sender{
+      &context_,
+      /* access_token_fetcher = */ &mock_access_token_fetcher_,
+      std::move(loader_factory),
+      /* api_key = */ std::string("fake_api_key"),
+      /* auth_enabled = */ true,
+      /* disable_auth_if_no_access_token = */ true};
+  request_sender.SendRequest(GURL("https://www.example.com"),
+                             std::string("request"),
+                             mock_response_callback_.Get());
+}
+
+TEST_F(ServiceRequestSenderTest,
+       AuthRequestFallsBackToApiKeyIfFetchingAccessTokenFails) {
+  EXPECT_CALL(mock_access_token_fetcher_, OnFetchAccessToken)
+      .Times(1)
+      .WillOnce(
+          RunOnceCallback<0>(/*success = */ false, /*access_token = */ ""));
+
+  auto loader_factory =
+      std::make_unique<NiceMock<MockSimpleURLLoaderFactory>>();
+  auto loader = std::make_unique<NiceMock<MockURLLoader>>();
+  EXPECT_CALL(*loader_factory, OnCreateLoader(_, _))
+      .WillOnce([&](::network::ResourceRequest* resource_request,
+                    const ::net::NetworkTrafficAnnotationTag& annotation_tag) {
+        EXPECT_FALSE(resource_request->headers.HasHeader("Authorization"));
+        EXPECT_EQ(resource_request->url,
+                  GURL("https://www.example.com/?key=fake_api_key"));
+        return std::move(loader);
+      });
+  EXPECT_CALL(*loader,
+              AttachStringForUpload(std::string("request"),
+                                    std::string("application/x-protobuffer")))
+      .Times(1);
+  EXPECT_CALL(*loader, DownloadToStringOfUnboundedSizeUntilCrashAndDie(_, _))
+      .WillOnce(WithArgs<1>([&](auto&& callback) {
+        std::move(callback).Run(std::make_unique<std::string>("response"));
+      }));
+  auto response_info = CreateResponseInfo(net::HTTP_OK, "OK");
+  EXPECT_CALL(*loader, ResponseInfo)
+      .WillRepeatedly(Return(response_info.get()));
+
+  EXPECT_CALL(mock_response_callback_, Run(net::HTTP_OK, "response"));
+  ServiceRequestSender request_sender{
+      &context_,
+      /* access_token_fetcher = */ &mock_access_token_fetcher_,
+      std::move(loader_factory),
+      /* api_key = */ std::string("fake_api_key"),
+      /* auth_enabled = */ true,
+      /* disable_auth_if_no_access_token = */ true};
+  request_sender.SendRequest(GURL("https://www.example.com"),
+                             std::string("request"),
+                             mock_response_callback_.Get());
+}
+
 // TODO(b/170934170): Add tests for full unit test coverage of
 // service_request_sender.