Skip to content

Commit

Permalink
[DBSC] Release blocked requests when offline
Browse files Browse the repository at this point in the history 
Requests are throttled until the cookie rotation request completes or
bound cookies are fresh in the cookie jar. If there is no network
connection, we resume immediately.

Bug: b/296565450
Change-Id: I4ab317893bf1f103da36664635d6e959e089d17e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4793952
Reviewed-by: Alex Ilin <alexilin@chromium.org>
Reviewed-by: Monica Basta <msalama@chromium.org>
Commit-Queue: Elizaveta Sheremetyeva <sheremetyeva@google.com>
Cr-Commit-Position: refs/heads/main@{#1187207}
  • Loading branch information
Elizaveta Sheremetyeva authored and Chromium LUCI CQ committed Aug 23, 2023
1 parent 22e6686 commit 82a7d67
Show file tree
Hide file tree
Showing 7 changed files with 151 additions and 8 deletions.
30 changes: 30 additions & 0 deletions chrome/browser/signin/bound_session_credentials/bound_session_cookie_controller_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "chrome/browser/signin/bound_session_credentials/session_binding_helper.h"
#include "chrome/browser/signin/wait_for_network_callback_helper_chrome.h"
#include "content/public/browser/storage_partition.h"
#include "services/network/public/cpp/network_connection_tracker.h"

namespace {
using Result = BoundSessionRefreshCookieFetcher::Result;
Expand All @@ -23,12 +24,14 @@ using Result = BoundSessionRefreshCookieFetcher::Result;
BoundSessionCookieControllerImpl::BoundSessionCookieControllerImpl(
unexportable_keys::UnexportableKeyService& key_service,
content::StoragePartition* storage_partition,
network::NetworkConnectionTracker* network_connection_tracker,
const bound_session_credentials::RegistrationParams& registration_params,
const base::flat_set<std::string>& cookie_names,
Delegate* delegate)
: BoundSessionCookieController(registration_params, cookie_names, delegate),
key_service_(key_service),
storage_partition_(storage_partition),
network_connection_tracker_(network_connection_tracker),
wait_for_network_callback_helper_(
std::make_unique<WaitForNetworkCallbackHelperChrome>()) {
CHECK(!registration_params.wrapped_key().empty());
Expand All @@ -47,10 +50,31 @@ BoundSessionCookieControllerImpl::~BoundSessionCookieControllerImpl() {
}

void BoundSessionCookieControllerImpl::Initialize() {
network_connection_observer_.Observe(network_connection_tracker_);
CreateBoundCookiesObservers();
MaybeRefreshCookie();
}

void BoundSessionCookieControllerImpl::OnConnectionChanged(
network::mojom::ConnectionType type) {
if (type == network::mojom::ConnectionType::CONNECTION_NONE) {
// Let network requests fail now while there is no internet connection,
// instead of holding them up until the network is back or timeout occurs.
// The network could come back shortly before the timeout which would result
// in requests being released without a valid cookie.
ResumeBlockedRequests();
}
}

bool BoundSessionCookieControllerImpl::IsConnectionTypeAvailableAndOffline() {
network::mojom::ConnectionType type;
return network_connection_tracker_->GetConnectionType(
&type, base::BindOnce(
&BoundSessionCookieControllerImpl::OnConnectionChanged,
weak_ptr_factory_.GetWeakPtr())) &&
type == network::mojom::ConnectionType::CONNECTION_NONE;
}

void BoundSessionCookieControllerImpl::OnRequestBlockedOnCookie(
base::OnceClosure resume_blocked_request) {
if (AreAllCookiesFresh()) {
Expand All @@ -62,6 +86,12 @@ void BoundSessionCookieControllerImpl::OnRequestBlockedOnCookie(
resume_blocked_requests_.push_back(std::move(resume_blocked_request));
MaybeRefreshCookie();

if (IsConnectionTypeAvailableAndOffline()) {
// See the comment in `OnConnectionChanged()` for explanation.
ResumeBlockedRequests();
return;
}

if (!resume_blocked_requests_timer_.IsRunning() &&
!resume_blocked_requests_.empty()) {
// Ensure all blocked requests are released after a timeout.
Expand Down
21 changes: 20 additions & 1 deletion chrome/browser/signin/bound_session_credentials/bound_session_cookie_controller_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@
#include <memory>

#include "base/functional/callback_forward.h"
#include "base/scoped_observation.h"
#include "base/time/time.h"
#include "base/timer/timer.h"
#include "chrome/browser/signin/bound_session_credentials/bound_session_refresh_cookie_fetcher.h"
#include "content/public/browser/storage_partition.h"
#include "services/network/public/cpp/network_connection_tracker.h"
#include "services/network/public/mojom/cookie_manager.mojom.h"
#include "url/gurl.h"

Expand All @@ -29,11 +31,14 @@ class BoundSessionCookieObserver;
class SessionBindingHelper;
class WaitForNetworkCallbackHelper;

class BoundSessionCookieControllerImpl : public BoundSessionCookieController {
class BoundSessionCookieControllerImpl
: public BoundSessionCookieController,
public network::NetworkConnectionTracker::NetworkConnectionObserver {
public:
BoundSessionCookieControllerImpl(
unexportable_keys::UnexportableKeyService& key_service,
content::StoragePartition* storage_partition,
network::NetworkConnectionTracker* network_connection_tracker,
const bound_session_credentials::RegistrationParams& registration_params,
const base::flat_set<std::string>& cookie_names,
Delegate* delegate);
Expand All @@ -51,6 +56,9 @@ class BoundSessionCookieControllerImpl : public BoundSessionCookieController {
void OnRequestBlockedOnCookie(
base::OnceClosure resume_blocked_request) override;

// network::NetworkConnectionTracker::NetworkConnectionObserver:
void OnConnectionChanged(network::mojom::ConnectionType type) override;

private:
friend class BoundSessionCookieControllerImplTest;

Expand All @@ -62,6 +70,8 @@ class BoundSessionCookieControllerImpl : public BoundSessionCookieController {
const GURL& url,
base::flat_set<std::string> cookie_names)>;

bool IsConnectionTypeAvailableAndOffline();

std::unique_ptr<BoundSessionRefreshCookieFetcher> CreateRefreshCookieFetcher()
const;
void CreateBoundCookiesObservers();
Expand All @@ -83,9 +93,15 @@ class BoundSessionCookieControllerImpl : public BoundSessionCookieController {

const raw_ref<unexportable_keys::UnexportableKeyService> key_service_;
const raw_ptr<content::StoragePartition> storage_partition_;
const raw_ptr<network::NetworkConnectionTracker> network_connection_tracker_;
std::vector<std::unique_ptr<BoundSessionCookieObserver>>
bound_cookies_observers_;

base::ScopedObservation<
network::NetworkConnectionTracker,
network::NetworkConnectionTracker::NetworkConnectionObserver>
network_connection_observer_{this};

std::unique_ptr<WaitForNetworkCallbackHelper>
wait_for_network_callback_helper_;
std::unique_ptr<SessionBindingHelper> session_binding_helper_;
Expand All @@ -99,6 +115,9 @@ class BoundSessionCookieControllerImpl : public BoundSessionCookieController {

RefreshCookieFetcherFactoryForTesting
refresh_cookie_fetcher_factory_for_testing_;

base::WeakPtrFactory<BoundSessionCookieControllerImpl> weak_ptr_factory_{
this};
};

#endif // CHROME_BROWSER_SIGNIN_BOUND_SESSION_CREDENTIALS_BOUND_SESSION_COOKIE_CONTROLLER_IMPL_H_
86 changes: 85 additions & 1 deletion ...browser/signin/bound_session_credentials/bound_session_cookie_controller_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,14 @@
#include "components/unexportable_keys/unexportable_key_loader.h"
#include "components/unexportable_keys/unexportable_key_service_impl.h"
#include "components/unexportable_keys/unexportable_key_task_manager.h"
#include "content/public/browser/network_service_instance.h"
#include "content/public/test/test_storage_partition.h"
#include "crypto/scoped_mock_unexportable_key_provider.h"
#include "crypto/signature_verifier.h"
#include "google_apis/gaia/gaia_urls.h"
#include "net/cookies/canonical_cookie.h"
#include "services/network/public/mojom/cookie_manager.mojom.h"
#include "services/network/test/test_network_connection_tracker.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

Expand Down Expand Up @@ -64,9 +66,14 @@ class BoundSessionCookieControllerImplTest
std::string(wrapped_key.begin(), wrapped_key.end()));

storage_partition_.set_cookie_manager_for_browser_process(&cookie_manager_);

SetUpNetworkConnection(true,
network::mojom::ConnectionType::CONNECTION_WIFI);

bound_session_cookie_controller_ =
std::make_unique<BoundSessionCookieControllerImpl>(
unexportable_key_service_, &storage_partition_, registration_params,
unexportable_key_service_, &storage_partition_,
content::GetNetworkConnectionTracker(), registration_params,
base::flat_set<std::string>(
{k1PSIDTSCookieName, k3PSIDTSCookieName}),
this);
Expand Down Expand Up @@ -230,6 +237,25 @@ class BoundSessionCookieControllerImplTest
bound_session_cookie_controller_.reset();
}

void SetUpNetworkConnection(bool respond_synchronously,
network::mojom::ConnectionType connection_type) {
network::TestNetworkConnectionTracker* tracker =
network::TestNetworkConnectionTracker::GetInstance();
tracker->SetRespondSynchronously(respond_synchronously);
tracker->SetConnectionType(connection_type);
}

void SetConnectionType(network::mojom::ConnectionType connection_type) {
network::TestNetworkConnectionTracker::GetInstance()->SetConnectionType(
connection_type);
}

bool IsConnectionTypeAvailableAndOffline() {
CHECK(bound_session_cookie_controller_);
return bound_session_cookie_controller_
->IsConnectionTypeAvailableAndOffline();
}

private:
base::test::TaskEnvironment task_environment_{
base::test::TaskEnvironment::TimeSource::MOCK_TIME};
Expand Down Expand Up @@ -347,6 +373,7 @@ TEST_F(BoundSessionCookieControllerImplTest, CookieChange) {

TEST_F(BoundSessionCookieControllerImplTest,
RequestBlockedOnCookieWhenCookieFresh) {
ASSERT_FALSE(IsConnectionTypeAvailableAndOffline());
// Set fresh cookie.
CompletePendingRefreshRequestIfAny();
BoundSessionCookieController* controller = bound_session_cookie_controller();
Expand All @@ -362,6 +389,7 @@ TEST_F(BoundSessionCookieControllerImplTest,

TEST_F(BoundSessionCookieControllerImplTest,
RequestBlockedOnCookieWhenCookieStaleTriggersARefresh) {
ASSERT_FALSE(IsConnectionTypeAvailableAndOffline());
CompletePendingRefreshRequestIfAny();

BoundSessionCookieController* controller = bound_session_cookie_controller();
Expand Down Expand Up @@ -390,6 +418,7 @@ TEST_F(BoundSessionCookieControllerImplTest,

TEST_F(BoundSessionCookieControllerImplTest,
RequestBlockedWhenNotAllCookiesFresh) {
ASSERT_FALSE(IsConnectionTypeAvailableAndOffline());
CompletePendingRefreshRequestIfAny();
BoundSessionCookieController* controller = bound_session_cookie_controller();

Expand All @@ -415,6 +444,7 @@ TEST_F(BoundSessionCookieControllerImplTest,

TEST_F(BoundSessionCookieControllerImplTest,
RequestBlockedOnCookieRefreshFailedWithPersistentError) {
ASSERT_FALSE(IsConnectionTypeAvailableAndOffline());
CompletePendingRefreshRequestIfAny();
EXPECT_FALSE(on_cookie_refresh_persistent_failure_called());

Expand Down Expand Up @@ -445,6 +475,7 @@ TEST_F(BoundSessionCookieControllerImplTest,
}

TEST_F(BoundSessionCookieControllerImplTest, RefreshFailedTransient) {
ASSERT_FALSE(IsConnectionTypeAvailableAndOffline());
CompletePendingRefreshRequestIfAny();
task_environment()->FastForwardBy(base::Minutes(12));
EXPECT_FALSE(AreAllCookiesFresh());
Expand Down Expand Up @@ -477,6 +508,7 @@ TEST_F(BoundSessionCookieControllerImplTest, RefreshFailedTransient) {

TEST_F(BoundSessionCookieControllerImplTest,
RequestBlockedOnCookieMultipleRequests) {
ASSERT_FALSE(IsConnectionTypeAvailableAndOffline());
CompletePendingRefreshRequestIfAny();
ResetOnBoundSessionParamsChangedCallCount();
// Cookie stale.
Expand All @@ -502,6 +534,7 @@ TEST_F(BoundSessionCookieControllerImplTest,

TEST_F(BoundSessionCookieControllerImplTest,
CookieChangesToFreshWhileRequestBlockedOnCookieIsPending) {
ASSERT_FALSE(IsConnectionTypeAvailableAndOffline());
CompletePendingRefreshRequestIfAny();
// Stale cookie.
task_environment()->FastForwardBy(base::Minutes(12));
Expand All @@ -528,6 +561,7 @@ TEST_F(BoundSessionCookieControllerImplTest,

TEST_F(BoundSessionCookieControllerImplTest,
ControllerDestroyedRequestBlockedOnCookieIsPending) {
ASSERT_FALSE(IsConnectionTypeAvailableAndOffline());
BoundSessionCookieController* controller = bound_session_cookie_controller();
std::array<base::test::TestFuture<void>, 5> futures;
for (auto& future : futures) {
Expand All @@ -542,6 +576,7 @@ TEST_F(BoundSessionCookieControllerImplTest,
}

TEST_F(BoundSessionCookieControllerImplTest, ResumeBlockedRequestsOnTimeout) {
ASSERT_FALSE(IsConnectionTypeAvailableAndOffline());
base::test::TestFuture<void> future;
bound_session_cookie_controller()->OnRequestBlockedOnCookie(
future.GetCallback());
Expand All @@ -558,6 +593,7 @@ TEST_F(BoundSessionCookieControllerImplTest, ResumeBlockedRequestsOnTimeout) {

TEST_F(BoundSessionCookieControllerImplTest,
BlockedRequestsCalculateTimeoutFromFirstRequest) {
ASSERT_FALSE(IsConnectionTypeAvailableAndOffline());
constexpr int kBlockedRequestCount = 2;
constexpr base::TimeDelta kDeltaBetweenRequests = base::Seconds(1);
BoundSessionCookieController* controller = bound_session_cookie_controller();
Expand Down Expand Up @@ -585,6 +621,7 @@ TEST_F(BoundSessionCookieControllerImplTest,
}

TEST_F(BoundSessionCookieControllerImplTest, ResumeBlockedRequestsTimerReset) {
ASSERT_FALSE(IsConnectionTypeAvailableAndOffline());
base::test::TestFuture<void> future;
bound_session_cookie_controller()->OnRequestBlockedOnCookie(
future.GetCallback());
Expand All @@ -597,6 +634,53 @@ TEST_F(BoundSessionCookieControllerImplTest, ResumeBlockedRequestsTimerReset) {
EXPECT_FALSE(resume_blocked_requests_timer()->IsRunning());
}

TEST_F(BoundSessionCookieControllerImplTest,
ResumeNewBlockedRequestsWhenOffline) {
SetUpNetworkConnection(true, network::mojom::ConnectionType::CONNECTION_NONE);
ASSERT_TRUE(IsConnectionTypeAvailableAndOffline());
base::test::TestFuture<void> future;
bound_session_cookie_controller()->OnRequestBlockedOnCookie(
future.GetCallback());
EXPECT_FALSE(AreAllCookiesFresh());
EXPECT_TRUE(cookie_fetcher());
EXPECT_TRUE(future.IsReady());
EXPECT_FALSE(resume_blocked_requests_timer()->IsRunning());
}

TEST_F(BoundSessionCookieControllerImplTest,
BlockRequestsWhenConnectionTypeUnavailable) {
SetUpNetworkConnection(false,
network::mojom::ConnectionType::CONNECTION_WIFI);
ASSERT_FALSE(IsConnectionTypeAvailableAndOffline());
base::test::TestFuture<void> future;
bound_session_cookie_controller()->OnRequestBlockedOnCookie(
future.GetCallback());
EXPECT_FALSE(future.IsReady());
EXPECT_TRUE(resume_blocked_requests_timer()->IsRunning());
task_environment()->RunUntilIdle();
EXPECT_FALSE(future.IsReady());
CompletePendingRefreshRequestIfAny();
EXPECT_TRUE(future.IsReady());
}

TEST_F(BoundSessionCookieControllerImplTest,
ResumePendingBlockedRequestsOnNetworkConnectionChangedToOffline) {
ASSERT_FALSE(IsConnectionTypeAvailableAndOffline());
base::test::TestFuture<void> future;
bound_session_cookie_controller()->OnRequestBlockedOnCookie(
future.GetCallback());
EXPECT_FALSE(future.IsReady());
EXPECT_TRUE(resume_blocked_requests_timer()->IsRunning());

SetConnectionType(network::mojom::ConnectionType::CONNECTION_NONE);
ASSERT_TRUE(IsConnectionTypeAvailableAndOffline());
task_environment()->RunUntilIdle();
EXPECT_FALSE(AreAllCookiesFresh());
EXPECT_TRUE(cookie_fetcher());
EXPECT_TRUE(future.IsReady());
EXPECT_FALSE(resume_blocked_requests_timer()->IsRunning());
}

TEST_F(BoundSessionCookieControllerImplTest,
NotNullCookieExpirationTimeIsReducedByThreshold) {
EXPECT_TRUE(CompletePendingRefreshRequestIfAny());
Expand Down
4 changes: 3 additions & 1 deletion .../browser/signin/bound_session_credentials/bound_session_cookie_refresh_service_factory.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "chrome/browser/signin/chrome_signin_client_factory.h"
#include "components/pref_registry/pref_registry_syncable.h"
#include "components/signin/public/base/signin_switches.h"
#include "content/public/browser/network_service_instance.h"

// static
BoundSessionCookieRefreshServiceFactory*
Expand Down Expand Up @@ -64,7 +65,8 @@ BoundSessionCookieRefreshServiceFactory::BuildServiceInstanceForBrowserContext(
bound_session_cookie_refresh_service =
std::make_unique<BoundSessionCookieRefreshServiceImpl>(
*key_service, profile->GetPrefs(),
profile->GetDefaultStoragePartition());
profile->GetDefaultStoragePartition(),
content::GetNetworkConnectionTracker());
bound_session_cookie_refresh_service->Initialize();
return bound_session_cookie_refresh_service;
}
Expand Down
9 changes: 6 additions & 3 deletions chrome/browser/signin/bound_session_credentials/bound_session_cookie_refresh_service_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,12 @@ const char kGoogleSessionTerminationHeader[] = "Sec-Session-Google-Termination";
BoundSessionCookieRefreshServiceImpl::BoundSessionCookieRefreshServiceImpl(
unexportable_keys::UnexportableKeyService& key_service,
PrefService* pref_service,
content::StoragePartition* storage_partion)
content::StoragePartition* storage_partion,
network::NetworkConnectionTracker* network_connection_tracker)
: key_service_(key_service),
pref_service_(pref_service),
storage_partition_(storage_partion) {}
storage_partition_(storage_partion),
network_connection_tracker_(network_connection_tracker) {}

BoundSessionCookieRefreshServiceImpl::~BoundSessionCookieRefreshServiceImpl() =
default;
Expand Down Expand Up @@ -204,7 +206,8 @@ BoundSessionCookieRefreshServiceImpl::CreateBoundSessionCookieController(
const base::flat_set<std::string>& cookie_names) {
return controller_factory_for_testing_.is_null()
? std::make_unique<BoundSessionCookieControllerImpl>(
key_service_.get(), storage_partition_, registration_params,
key_service_.get(), storage_partition_,
network_connection_tracker_, registration_params,
cookie_names, this)
: controller_factory_for_testing_.Run(registration_params,
cookie_names, this);
Expand Down

0 comments on commit 82a7d67

Please sign in to comment.