Skip to content

Commit

Permalink
[DBSC] Multi-session storage
Browse files Browse the repository at this point in the history 
This CL changes the type of the
"bound_session_credentials_bound_session_params" pref from a string to
a dictionary. This unblocks the support for multiple bound sessions
running at the same time.

In order to facilitate a session look-up (to perform session updates,
for example), the storage has two levels:
- the first level is keyed by a site
- the second level is keyed by a session_id

We update the storage first, before it reached the first users, to avoid
migrations in the future.

Bug: b/300627729
Change-Id: Iea3ab38fb351a3c3c44d1465ce86ba5052afa624
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4935366
Commit-Queue: Alex Ilin <alexilin@chromium.org>
Reviewed-by: Monica Basta <msalama@chromium.org>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1216969}
  • Loading branch information
Alex Ilin authored and Chromium LUCI CQ committed Oct 30, 2023
1 parent bf2f84a commit a523ead
Show file tree
Hide file tree
Showing 7 changed files with 300 additions and 116 deletions.
20 changes: 14 additions & 6 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 @@ -44,10 +44,12 @@ BoundSessionCookieRefreshServiceImpl::~BoundSessionCookieRefreshServiceImpl() =
default;

void BoundSessionCookieRefreshServiceImpl::Initialize() {
absl::optional<bound_session_credentials::BoundSessionParams>
bound_session_params = session_params_storage_->ReadParams();
if (bound_session_params.has_value()) {
InitializeBoundSession(bound_session_params.value());
std::vector<bound_session_credentials::BoundSessionParams>
bound_session_params = session_params_storage_->ReadAllParams();
if (!bound_session_params.empty()) {
// Only a single bound session is currently supported.
// TODO(http://b/274774185): support multiple parallel bound sessions.
InitializeBoundSession(bound_session_params.front());
}
}

Expand All @@ -58,7 +60,11 @@ void BoundSessionCookieRefreshServiceImpl::RegisterNewBoundSession(
return;
}
// New session should override an existing one.
cookie_controller_.reset();
if (cookie_controller_) {
session_params_storage_->ClearParams(cookie_controller_->url().spec(),
cookie_controller_->session_id());
cookie_controller_.reset();
}
InitializeBoundSession(params);
}

Expand Down Expand Up @@ -162,7 +168,9 @@ void BoundSessionCookieRefreshServiceImpl::

void BoundSessionCookieRefreshServiceImpl::TerminateSession() {
cookie_controller_.reset();
session_params_storage_->ClearParams();
// TODO(b/300627729): stop clearing all params once multiple sessions are
// supported.
session_params_storage_->ClearAllParams();
UpdateAllRenderers();
}

Expand Down
112 changes: 64 additions & 48 deletions ...er/signin/bound_session_credentials/bound_session_cookie_refresh_service_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@
#include <utility>

#include "base/base64.h"
#include "base/containers/span.h"
#include "base/functional/bind.h"
#include "base/functional/callback_forward.h"
#include "base/memory/raw_ptr.h"
#include "base/test/bind.h"
#include "base/test/mock_callback.h"
#include "base/test/protobuf_matchers.h"
#include "base/test/scoped_feature_list.h"
#include "base/test/task_environment.h"
#include "base/test/test_future.h"
Expand All @@ -36,12 +38,28 @@
namespace {
constexpr char k1PSIDTSCookieName[] = "__Secure-1PSIDTS";
constexpr char k3PSIDTSCookieName[] = "__Secure-3PSIDTS";
constexpr char kBoundSessionParamsPref[] =
"bound_session_credentials_bound_session_params";
const char kSessionTerminationHeader[] = "Sec-Session-Google-Termination";
constexpr char kWrappedKey[] = "wrapped_key";
constexpr char kTestSessionId[] = "test_session_id";

// Matches a cookie name against a `bound_session_credentials::Credential`.
// `arg` type is std::tuple<std::string, bound_session_credentials::Credential>
MATCHER(IsCookieCredential, "") {
const auto& [cookie_name, credential] = arg;
if (!credential.has_cookie_credential()) {
return false;
}

return cookie_name == credential.cookie_credential().name();
}

// Checks equality of the two protos in an std::tuple. Useful for matching two
// two protos using ::testing::Pointwise or ::testing::UnorderedPointwise.
MATCHER(TupleEqualsProto, "") {
return testing::ExplainMatchResult(base::EqualsProto(std::get<1>(arg)),
std::get<0>(arg), result_listener);
}

class FakeBoundSessionCookieController : public BoundSessionCookieController {
public:
explicit FakeBoundSessionCookieController(
Expand Down Expand Up @@ -110,6 +128,8 @@ class BoundSessionCookieRefreshServiceImplTest : public testing::Test {

BoundSessionCookieRefreshServiceImplTest() {
BoundSessionParamsStorage::RegisterProfilePrefs(prefs_.registry());
test_storage_ =
BoundSessionParamsStorage::CreatePrefsStorageForTesting(prefs_);
}

~BoundSessionCookieRefreshServiceImplTest() override = default;
Expand Down Expand Up @@ -181,35 +201,42 @@ class BoundSessionCookieRefreshServiceImplTest : public testing::Test {
return cookie_controller_;
}

sync_preferences::TestingPrefServiceSyncable* prefs() { return &prefs_; }
BoundSessionParamsStorage* storage() { return test_storage_.get(); }

// Emulates an existing session that resumes after `cookie_refresh_service_`
// is created
// is created.
void SetupPreConditionForBoundSession() {
CHECK(!cookie_refresh_service_)
<< "If the cookie refresh service is already created, consider using "
"`RegisterNewBoundSession` to start a new bound session.";
bound_session_credentials::BoundSessionParams params =
CreateTestBoundSessionParams();
std::string bound_session_params;
base::Base64Encode(params.SerializeAsString(), &bound_session_params);
prefs()->SetString(kBoundSessionParamsPref, bound_session_params);
"`RegisterNewBoundSession()` to start a new bound session.";
ASSERT_TRUE(storage()->SaveParams(CreateTestBoundSessionParams()));
}

void RunUntilIdle() { task_environment_.RunUntilIdle(); }

void VerifyBoundSession() {
void VerifyBoundSession(
const bound_session_credentials::BoundSessionParams& expected_params) {
CHECK(cookie_refresh_service_);
EXPECT_TRUE(cookie_refresh_service_->GetBoundSessionThrottlerParams());
EXPECT_TRUE(prefs()->HasPrefPath(kBoundSessionParamsPref));
EXPECT_TRUE(cookie_controller());
EXPECT_THAT(storage()->ReadAllParams(),
testing::Pointwise(TupleEqualsProto(), {expected_params}));
ASSERT_TRUE(cookie_controller());

EXPECT_EQ(cookie_controller()->session_id(), expected_params.session_id());
EXPECT_EQ(cookie_controller()->url(), GURL(expected_params.site()));
EXPECT_THAT(cookie_controller()->wrapped_key(),
testing::ElementsAreArray(base::as_bytes(
base::make_span(expected_params.wrapped_key()))));
EXPECT_THAT(cookie_controller()->cookie_names(),
testing::UnorderedPointwise(IsCookieCredential(),
expected_params.credentials()));
}

void VerifyNoBoundSession() {
CHECK(cookie_refresh_service_);
EXPECT_FALSE(cookie_refresh_service_->GetBoundSessionThrottlerParams());
EXPECT_FALSE(cookie_controller());
EXPECT_FALSE(prefs()->HasPrefPath(kBoundSessionParamsPref));
EXPECT_THAT(storage()->ReadAllParams(), testing::IsEmpty());
}

bound_session_credentials::Credential CreateCookieCredential(
Expand Down Expand Up @@ -244,6 +271,7 @@ class BoundSessionCookieRefreshServiceImplTest : public testing::Test {
base::test::TaskEnvironment task_environment_{
base::test::TaskEnvironment::TimeSource::MOCK_TIME};
sync_preferences::TestingPrefServiceSyncable prefs_;
std::unique_ptr<BoundSessionParamsStorage> test_storage_;
content::TestStoragePartition storage_partition_;
std::unique_ptr<BoundSessionCookieRefreshServiceImpl> cookie_refresh_service_;
unexportable_keys::FakeUnexportableKeyService fake_unexportable_key_service_;
Expand All @@ -253,19 +281,7 @@ class BoundSessionCookieRefreshServiceImplTest : public testing::Test {
TEST_F(BoundSessionCookieRefreshServiceImplTest, VerifyControllerParams) {
SetupPreConditionForBoundSession();
GetCookieRefreshServiceImpl();
VerifyBoundSession();
FakeBoundSessionCookieController* controller = cookie_controller();
EXPECT_TRUE(controller);
EXPECT_EQ(controller->url(), kTestGoogleURL);
EXPECT_EQ(controller->session_id(), kTestSessionId);
EXPECT_EQ(
controller->cookie_names(),
base::flat_set<std::string>({k1PSIDTSCookieName, k3PSIDTSCookieName}));
EXPECT_EQ(controller->min_cookie_expiration_time(), base::Time());
EXPECT_EQ(controller->wrapped_key(),
std::vector<uint8_t>(std::begin(kWrappedKey),
// Omit `\0`.
std::end(kWrappedKey) - 1));
VerifyBoundSession(CreateTestBoundSessionParams());
}

TEST_F(BoundSessionCookieRefreshServiceImplTest,
Expand Down Expand Up @@ -407,7 +423,7 @@ TEST_F(BoundSessionCookieRefreshServiceImplTest,

BoundSessionCookieRefreshServiceImpl* service = GetCookieRefreshServiceImpl();
service->MaybeTerminateSession(headers.get());
VerifyBoundSession();
VerifyBoundSession(CreateTestBoundSessionParams());
}

TEST_F(BoundSessionCookieRefreshServiceImplTest,
Expand All @@ -418,7 +434,7 @@ TEST_F(BoundSessionCookieRefreshServiceImplTest,

BoundSessionCookieRefreshServiceImpl* service = GetCookieRefreshServiceImpl();
service->MaybeTerminateSession(headers.get());
VerifyBoundSession();
VerifyBoundSession(CreateTestBoundSessionParams());
}

TEST_F(BoundSessionCookieRefreshServiceImplTest,
Expand Down Expand Up @@ -451,10 +467,9 @@ TEST_F(BoundSessionCookieRefreshServiceImplTest, RegisterNewBoundSession) {
BoundSessionCookieRefreshServiceImpl* service = GetCookieRefreshServiceImpl();
VerifyNoBoundSession();

service->RegisterNewBoundSession(CreateTestBoundSessionParams());
VerifyBoundSession();
// TODO(http://b/286222327): check bound session params once they are
// properly passed to controller.
auto params = CreateTestBoundSessionParams();
service->RegisterNewBoundSession(params);
VerifyBoundSession(params);
}

TEST_F(BoundSessionCookieRefreshServiceImplTest, OverrideExistingBoundSession) {
Expand All @@ -465,24 +480,21 @@ TEST_F(BoundSessionCookieRefreshServiceImplTest, OverrideExistingBoundSession) {
new_params.set_session_id("test_session_id_2");
service->RegisterNewBoundSession(new_params);

VerifyBoundSession();
// TODO(http://b/286222327): check bound session params once they are
// properly passed to controller.
VerifyBoundSession(new_params);
}

TEST_F(BoundSessionCookieRefreshServiceImplTest,
OverrideExistingBoundSessionWithInvalidParams) {
BoundSessionCookieRefreshServiceImpl* service = GetCookieRefreshServiceImpl();
service->RegisterNewBoundSession(CreateTestBoundSessionParams());
auto original_params = CreateTestBoundSessionParams();
service->RegisterNewBoundSession(original_params);

auto invalid_params = CreateTestBoundSessionParams();
invalid_params.clear_session_id();
service->RegisterNewBoundSession(invalid_params);

// Original session should not be modified.
VerifyBoundSession();
// TODO(http://b/286222327): check bound session params once they are
// properly passed to controller.
VerifyBoundSession(original_params);
}

TEST_F(BoundSessionCookieRefreshServiceImplTest, ClearMatchingData) {
Expand All @@ -497,41 +509,45 @@ TEST_F(BoundSessionCookieRefreshServiceImplTest, ClearMatchingData) {
TEST_F(BoundSessionCookieRefreshServiceImplTest,
ClearMatchingDataTypeMismatch) {
BoundSessionCookieRefreshServiceImpl* service = GetCookieRefreshServiceImpl();
service->RegisterNewBoundSession(CreateTestBoundSessionParams());
auto params = CreateTestBoundSessionParams();
service->RegisterNewBoundSession(params);

ClearOriginData(content::StoragePartition::REMOVE_DATA_MASK_CACHE_STORAGE,
url::Origin::Create(kTestGoogleURL));
VerifyBoundSession();
VerifyBoundSession(params);
}

TEST_F(BoundSessionCookieRefreshServiceImplTest,
ClearMatchingDataOriginMismatch) {
BoundSessionCookieRefreshServiceImpl* service = GetCookieRefreshServiceImpl();
service->RegisterNewBoundSession(CreateTestBoundSessionParams());
auto params = CreateTestBoundSessionParams();
service->RegisterNewBoundSession(params);

ClearOriginData(content::StoragePartition::REMOVE_DATA_MASK_COOKIES,
url::Origin::Create(GURL("https://example.org")));
VerifyBoundSession();
VerifyBoundSession(params);
}

TEST_F(BoundSessionCookieRefreshServiceImplTest,
ClearMatchingDataOriginMismatchSuborigin) {
BoundSessionCookieRefreshServiceImpl* service = GetCookieRefreshServiceImpl();
service->RegisterNewBoundSession(CreateTestBoundSessionParams());
auto params = CreateTestBoundSessionParams();
service->RegisterNewBoundSession(params);

ClearOriginData(content::StoragePartition::REMOVE_DATA_MASK_COOKIES,
url::Origin::Create(GURL("https://accounts.google.com")));
VerifyBoundSession();
VerifyBoundSession(params);
}

TEST_F(BoundSessionCookieRefreshServiceImplTest,
ClearMatchingDataCreationTimeMismatch) {
BoundSessionCookieRefreshServiceImpl* service = GetCookieRefreshServiceImpl();
service->RegisterNewBoundSession(CreateTestBoundSessionParams());
auto params = CreateTestBoundSessionParams();
service->RegisterNewBoundSession(params);

ClearOriginData(content::StoragePartition::REMOVE_DATA_MASK_COOKIES,
url::Origin::Create(kTestGoogleURL),
base::Time::Now() - base::Seconds(5),
base::Time::Now() - base::Seconds(3));
VerifyBoundSession();
VerifyBoundSession(params);
}

0 comments on commit a523ead

Please sign in to comment.