You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
227 lines
11 KiB
227 lines
11 KiB
commit 73e9d865abd6b636280c4bb45720af2ff2c1e374
|
|
Author: Monica Basta <msalama@chromium.org>
|
|
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 <msalama@chromium.org>
|
|
Reviewed-by: Alex Ilin <alexilin@chromium.org>
|
|
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<int> response_code;
|
|
+ enum class Result {
|
|
+ kSuccess = 0,
|
|
+ kConnectionError = 1,
|
|
+ kServerTransientError = 2,
|
|
+ kServerPersistentError = 3,
|
|
};
|
|
+
|
|
// Reports the result of the fetch request.
|
|
using RefreshCookieCompleteCallback = base::OnceCallback<void(Result)>;
|
|
|
|
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<net::HttpResponseHeaders> headers) {
|
|
net::Error net_error = static_cast<net::Error>(url_loader_->NetError());
|
|
|
|
- std::move(callback_).Run(
|
|
- Result(net_error, headers ? absl::optional<int>(headers->response_code())
|
|
- : absl::nullopt));
|
|
+ Result result = GetResultFromNetErrorAndHttpStatusCode(
|
|
+ net_error,
|
|
+ headers ? absl::optional<int>(headers->response_code()) : absl::nullopt);
|
|
+ std::move(callback_).Run(result);
|
|
+}
|
|
+
|
|
+BoundSessionRefreshCookieFetcher::Result
|
|
+BoundSessionRefreshCookieFetcherImpl::GetResultFromNetErrorAndHttpStatusCode(
|
|
+ net::Error net_error,
|
|
+ absl::optional<int> 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<net::HttpResponseHeaders> headers);
|
|
+ Result GetResultFromNetErrorAndHttpStatusCode(
|
|
+ net::Error net_error,
|
|
+ absl::optional<int> response_code);
|
|
|
|
const raw_ptr<SigninClient> client_;
|
|
const scoped_refptr<network::SharedURLLoaderFactory> 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
|
|
}
|