commit 73e9d865abd6b636280c4bb45720af2ff2c1e374 Author: Monica Basta Date: Fri Jun 2 13:25:42 2023 +0000 [BSC]: Add BoundSessionRefreshCookieFetcher::Result Bug: b/273920907 Change-Id: I6508dcb79592420bfa3ebe3aac872c097a303a02 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4574672 Commit-Queue: Monica Basta Reviewed-by: Alex Ilin Cr-Commit-Position: refs/heads/main@{#1152484} diff --git a/chrome/browser/signin/bound_session_credentials/bound_session_cookie_controller_impl.cc b/chrome/browser/signin/bound_session_credentials/bound_session_cookie_controller_impl.cc index 4e7e0b092a568..1c8c0110e3516 100644 --- a/chrome/browser/signin/bound_session_credentials/bound_session_cookie_controller_impl.cc +++ b/chrome/browser/signin/bound_session_credentials/bound_session_cookie_controller_impl.cc @@ -93,7 +93,7 @@ void BoundSessionCookieControllerImpl::MaybeRefreshCookie() { void BoundSessionCookieControllerImpl::OnCookieRefreshFetched( BoundSessionRefreshCookieFetcher::Result result) { refresh_cookie_fetcher_.reset(); - if (result.net_error == net::OK && result.response_code == net::HTTP_OK) { + if (result == BoundSessionRefreshCookieFetcher::Result::kSuccess) { // Requests are resumed once the cookie is set in the cookie jar. The // cookie is expected to be fresh and `this` is notified with its // expiration date before `OnCookieRefreshFetched` is called. diff --git a/chrome/browser/signin/bound_session_credentials/bound_session_refresh_cookie_fetcher.h b/chrome/browser/signin/bound_session_credentials/bound_session_refresh_cookie_fetcher.h index db62988635a26..f7a8b3693346f 100644 --- a/chrome/browser/signin/bound_session_credentials/bound_session_refresh_cookie_fetcher.h +++ b/chrome/browser/signin/bound_session_credentials/bound_session_refresh_cookie_fetcher.h @@ -14,10 +14,13 @@ // created per request. class BoundSessionRefreshCookieFetcher { public: - struct Result { - net::Error net_error; - absl::optional response_code; + enum class Result { + kSuccess = 0, + kConnectionError = 1, + kServerTransientError = 2, + kServerPersistentError = 3, }; + // Reports the result of the fetch request. using RefreshCookieCompleteCallback = base::OnceCallback; diff --git a/chrome/browser/signin/bound_session_credentials/bound_session_refresh_cookie_fetcher_impl.cc b/chrome/browser/signin/bound_session_credentials/bound_session_refresh_cookie_fetcher_impl.cc index 46be6f06b147a..a6f038b158311 100644 --- a/chrome/browser/signin/bound_session_credentials/bound_session_refresh_cookie_fetcher_impl.cc +++ b/chrome/browser/signin/bound_session_credentials/bound_session_refresh_cookie_fetcher_impl.cc @@ -8,6 +8,7 @@ #include "components/signin/public/base/signin_client.h" #include "google_apis/gaia/gaia_urls.h" +#include "net/http/http_status_code.h" #include "net/traffic_annotation/network_traffic_annotation.h" #include "services/network/public/cpp/resource_request.h" #include "services/network/public/cpp/shared_url_loader_factory.h" @@ -102,7 +103,36 @@ void BoundSessionRefreshCookieFetcherImpl::OnURLLoaderComplete( scoped_refptr headers) { net::Error net_error = static_cast(url_loader_->NetError()); - std::move(callback_).Run( - Result(net_error, headers ? absl::optional(headers->response_code()) - : absl::nullopt)); + Result result = GetResultFromNetErrorAndHttpStatusCode( + net_error, + headers ? absl::optional(headers->response_code()) : absl::nullopt); + std::move(callback_).Run(result); +} + +BoundSessionRefreshCookieFetcher::Result +BoundSessionRefreshCookieFetcherImpl::GetResultFromNetErrorAndHttpStatusCode( + net::Error net_error, + absl::optional response_code) { + if ((net_error != net::OK && + net_error != net::ERR_HTTP_RESPONSE_CODE_FAILURE) || + !response_code) { + return BoundSessionRefreshCookieFetcher::Result::kConnectionError; + } + + if (response_code == net::HTTP_OK) { + return BoundSessionRefreshCookieFetcher::Result::kSuccess; + } + + if (response_code >= net::HTTP_INTERNAL_SERVER_ERROR) { + // Server error 5xx. + return BoundSessionRefreshCookieFetcher::Result::kServerTransientError; + } + + if (response_code >= net::HTTP_BAD_REQUEST) { + // Server error 4xx. + return BoundSessionRefreshCookieFetcher::Result::kServerPersistentError; + } + + // Unexpected response code. + return BoundSessionRefreshCookieFetcher::Result::kServerPersistentError; } diff --git a/chrome/browser/signin/bound_session_credentials/bound_session_refresh_cookie_fetcher_impl.h b/chrome/browser/signin/bound_session_credentials/bound_session_refresh_cookie_fetcher_impl.h index 733ffbaae088c..52943f0194c32 100644 --- a/chrome/browser/signin/bound_session_credentials/bound_session_refresh_cookie_fetcher_impl.h +++ b/chrome/browser/signin/bound_session_credentials/bound_session_refresh_cookie_fetcher_impl.h @@ -31,8 +31,14 @@ class BoundSessionRefreshCookieFetcherImpl void Start(RefreshCookieCompleteCallback callback) override; private: + FRIEND_TEST_ALL_PREFIXES(BoundSessionRefreshCookieFetcherImplTest, + GetResultFromNetErrorAndHttpStatusCode); + void StartRefreshRequest(); void OnURLLoaderComplete(scoped_refptr headers); + Result GetResultFromNetErrorAndHttpStatusCode( + net::Error net_error, + absl::optional response_code); const raw_ptr client_; const scoped_refptr url_loader_factory_; diff --git a/chrome/browser/signin/bound_session_credentials/bound_session_refresh_cookie_fetcher_impl_unittest.cc b/chrome/browser/signin/bound_session_credentials/bound_session_refresh_cookie_fetcher_impl_unittest.cc index d018592022d55..36ae64f83e4ee 100644 --- a/chrome/browser/signin/bound_session_credentials/bound_session_refresh_cookie_fetcher_impl_unittest.cc +++ b/chrome/browser/signin/bound_session_credentials/bound_session_refresh_cookie_fetcher_impl_unittest.cc @@ -19,8 +19,6 @@ #include "services/network/test/test_utils.h" #include "testing/gtest/include/gtest/gtest.h" -namespace { - class BoundSessionRefreshCookieFetcherImplTest : public ::testing::Test { public: BoundSessionRefreshCookieFetcherImplTest() { @@ -55,8 +53,7 @@ TEST_F(BoundSessionRefreshCookieFetcherImplTest, Success) { pending_request->request.url.spec(), ""); EXPECT_TRUE(future.Wait()); BoundSessionRefreshCookieFetcher::Result result = future.Get(); - EXPECT_EQ(result.net_error, net::OK); - EXPECT_EQ(result.response_code, net::HTTP_OK); + EXPECT_EQ(result, BoundSessionRefreshCookieFetcher::Result::kSuccess); } TEST_F(BoundSessionRefreshCookieFetcherImplTest, FailureNetError) { @@ -75,8 +72,7 @@ TEST_F(BoundSessionRefreshCookieFetcherImplTest, FailureNetError) { EXPECT_TRUE(future.Wait()); BoundSessionRefreshCookieFetcher::Result result = future.Get(); - EXPECT_EQ(result.net_error, status.error_code); - EXPECT_FALSE(result.response_code.has_value()); + EXPECT_EQ(result, BoundSessionRefreshCookieFetcher::Result::kConnectionError); } TEST_F(BoundSessionRefreshCookieFetcherImplTest, FailureHttpError) { @@ -93,8 +89,38 @@ TEST_F(BoundSessionRefreshCookieFetcherImplTest, FailureHttpError) { EXPECT_TRUE(future.Wait()); BoundSessionRefreshCookieFetcher::Result result = future.Get(); - EXPECT_EQ(result.net_error, net::ERR_HTTP_RESPONSE_CODE_FAILURE); - EXPECT_EQ(result.response_code, net::HTTP_UNAUTHORIZED); + EXPECT_EQ(result, + BoundSessionRefreshCookieFetcher::Result::kServerPersistentError); +} + +TEST_F(BoundSessionRefreshCookieFetcherImplTest, + GetResultFromNetErrorAndHttpStatusCode) { + // Connection error. + EXPECT_EQ(fetcher_->GetResultFromNetErrorAndHttpStatusCode( + net::ERR_CONNECTION_TIMED_OUT, absl::nullopt), + BoundSessionRefreshCookieFetcher::Result::kConnectionError); + // net::OK. + EXPECT_EQ( + fetcher_->GetResultFromNetErrorAndHttpStatusCode(net::OK, net::HTTP_OK), + BoundSessionRefreshCookieFetcher::Result::kSuccess); + // net::ERR_HTTP_RESPONSE_CODE_FAILURE + EXPECT_EQ(fetcher_->GetResultFromNetErrorAndHttpStatusCode( + net::ERR_HTTP_RESPONSE_CODE_FAILURE, net::HTTP_BAD_REQUEST), + BoundSessionRefreshCookieFetcher::Result::kServerPersistentError); + // Persistent error. + EXPECT_EQ(fetcher_->GetResultFromNetErrorAndHttpStatusCode( + net::OK, net::HTTP_BAD_REQUEST), + BoundSessionRefreshCookieFetcher::Result::kServerPersistentError); + EXPECT_EQ(fetcher_->GetResultFromNetErrorAndHttpStatusCode( + net::OK, net::HTTP_NOT_FOUND), + BoundSessionRefreshCookieFetcher::Result::kServerPersistentError); + // Transient error. + EXPECT_EQ(fetcher_->GetResultFromNetErrorAndHttpStatusCode( + net::OK, net::HTTP_INTERNAL_SERVER_ERROR), + BoundSessionRefreshCookieFetcher::Result::kServerTransientError); + EXPECT_EQ(fetcher_->GetResultFromNetErrorAndHttpStatusCode( + net::OK, net::HTTP_GATEWAY_TIMEOUT), + BoundSessionRefreshCookieFetcher::Result::kServerTransientError); } TEST_F(BoundSessionRefreshCookieFetcherImplTest, NetworkDelayed) { @@ -114,5 +140,3 @@ TEST_F(BoundSessionRefreshCookieFetcherImplTest, NetworkDelayed) { EXPECT_TRUE(future.Wait()); } - -} // namespace diff --git a/chrome/browser/signin/bound_session_credentials/fake_bound_session_refresh_cookie_fetcher.cc b/chrome/browser/signin/bound_session_credentials/fake_bound_session_refresh_cookie_fetcher.cc index b4b1a07e687cb..fcfa9305d04e9 100644 --- a/chrome/browser/signin/bound_session_credentials/fake_bound_session_refresh_cookie_fetcher.cc +++ b/chrome/browser/signin/bound_session_credentials/fake_bound_session_refresh_cookie_fetcher.cc @@ -51,7 +51,8 @@ void FakeBoundSessionRefreshCookieFetcher::SimulateCompleteRefreshRequest( // Synchronous since tests use `BoundSessionTestCookieManager`. OnRefreshCookieCompleted(CreateFakeCookie(cookie_expiration.value())); } else { - std::move(callback_).Run(Result(net::Error::OK, net::HTTP_FORBIDDEN)); + std::move(callback_).Run( + BoundSessionRefreshCookieFetcher::Result::kServerPersistentError); } } @@ -83,9 +84,11 @@ void FakeBoundSessionRefreshCookieFetcher::OnCookieSet( net::CookieAccessResult access_result) { bool success = access_result.status.IsInclude(); if (!success) { - std::move(callback_).Run(Result(net::Error::OK, net::HTTP_FORBIDDEN)); + std::move(callback_).Run( + BoundSessionRefreshCookieFetcher::Result::kServerPersistentError); } else { - std::move(callback_).Run(Result(net::Error::OK, net::HTTP_OK)); + std::move(callback_).Run( + BoundSessionRefreshCookieFetcher::Result::kSuccess); } // |This| may be destroyed }