[DBSC] Separate registration params storage for OTR profiles

This CL adds a new abstraction BoundSessionRegistrationStorage to manage registration params storage. For regular profiles this is mostly a refactoring but the behavior for OTR profiles changes completely. Instead of being saved to prefs, registration params in OTR profiles will be stored in memory. Bug: b/296373254 Change-Id: Iffbcbb2b000c76bcf4759a0ade9585d3b83b3e26 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4793649 Commit-Queue: Alex Ilin <alexilin@chromium.org> Reviewed-by: Monica Basta <msalama@chromium.org> Auto-Submit: Alex Ilin <alexilin@chromium.org> Cr-Commit-Position: refs/heads/main@{#1187288}
chromium · Aug 23, 2023 · 4e2863a · 4e2863a
1 parent 2f83c04
commit 4e2863a
Show file tree

Hide file tree

Showing 9 changed files with 442 additions and 83 deletions.
diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn
@@ -6954,6 +6954,8 @@ static_library("browser") {
       "signin/bound_session_credentials/bound_session_cookie_refresh_service_factory.h",
       "signin/bound_session_credentials/bound_session_cookie_refresh_service_impl.cc",
       "signin/bound_session_credentials/bound_session_cookie_refresh_service_impl.h",
+      "signin/bound_session_credentials/bound_session_params_storage.cc",
+      "signin/bound_session_credentials/bound_session_params_storage.h",
       "signin/bound_session_credentials/bound_session_refresh_cookie_fetcher.cc",
       "signin/bound_session_credentials/bound_session_refresh_cookie_fetcher.h",
       "signin/bound_session_credentials/bound_session_refresh_cookie_fetcher_impl.cc",

diff --git a/.../browser/signin/bound_session_credentials/bound_session_cookie_refresh_service_factory.cc b/.../browser/signin/bound_session_credentials/bound_session_cookie_refresh_service_factory.cc
@@ -10,6 +10,7 @@
 #include "chrome/browser/profiles/profile_keyed_service_factory.h"
 #include "chrome/browser/signin/bound_session_credentials/bound_session_cookie_refresh_service.h"
 #include "chrome/browser/signin/bound_session_credentials/bound_session_cookie_refresh_service_impl.h"
+#include "chrome/browser/signin/bound_session_credentials/bound_session_params_storage.h"
 #include "chrome/browser/signin/bound_session_credentials/unexportable_key_service_factory.h"
 #include "chrome/browser/signin/chrome_signin_client_factory.h"
 #include "components/pref_registry/pref_registry_syncable.h"
@@ -64,7 +65,8 @@ BoundSessionCookieRefreshServiceFactory::BuildServiceInstanceForBrowserContext(
   std::unique_ptr<BoundSessionCookieRefreshService>
       bound_session_cookie_refresh_service =
           std::make_unique<BoundSessionCookieRefreshServiceImpl>(
-              *key_service, profile->GetPrefs(),
+              *key_service,
+              BoundSessionParamsStorage::CreateForProfile(*profile),
               profile->GetDefaultStoragePartition(),
               content::GetNetworkConnectionTracker());
   bound_session_cookie_refresh_service->Initialize();
@@ -73,5 +75,5 @@ BoundSessionCookieRefreshServiceFactory::BuildServiceInstanceForBrowserContext(
 
 void BoundSessionCookieRefreshServiceFactory::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
-  BoundSessionCookieRefreshServiceImpl::RegisterProfilePrefs(registry);
+  BoundSessionParamsStorage::RegisterProfilePrefs(registry);
 }
diff --git a/chrome/browser/signin/bound_session_credentials/bound_session_cookie_refresh_service_impl.cc b/chrome/browser/signin/bound_session_credentials/bound_session_cookie_refresh_service_impl.cc
@@ -12,52 +12,44 @@
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/signin/bound_session_credentials/bound_session_cookie_controller.h"
 #include "chrome/browser/signin/bound_session_credentials/bound_session_cookie_controller_impl.h"
+#include "chrome/browser/signin/bound_session_credentials/bound_session_params_storage.h"
 #include "chrome/browser/signin/bound_session_credentials/bound_session_registration_fetcher_impl.h"
 #include "chrome/common/renderer_configuration.mojom.h"
-#include "components/pref_registry/pref_registry_syncable.h"
-#include "components/prefs/pref_service.h"
 #include "components/signin/public/base/signin_client.h"
 #include "content/public/browser/storage_partition.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
 
 namespace {
-const char kRegistrationParamsPref[] =
-    "bound_session_credentials_registration_params";
 const char kGoogleSessionTerminationHeader[] = "Sec-Session-Google-Termination";
 }
 
 BoundSessionCookieRefreshServiceImpl::BoundSessionCookieRefreshServiceImpl(
     unexportable_keys::UnexportableKeyService& key_service,
-    PrefService* pref_service,
-    content::StoragePartition* storage_partion,
+    std::unique_ptr<BoundSessionParamsStorage> session_params_storage,
+    content::StoragePartition* storage_partition,
     network::NetworkConnectionTracker* network_connection_tracker)
     : key_service_(key_service),
-      pref_service_(pref_service),
-      storage_partition_(storage_partion),
-      network_connection_tracker_(network_connection_tracker) {}
+      session_params_storage_(std::move(session_params_storage)),
+      storage_partition_(storage_partition),
+      network_connection_tracker_(network_connection_tracker) {
+  CHECK(session_params_storage_);
+}
 
 BoundSessionCookieRefreshServiceImpl::~BoundSessionCookieRefreshServiceImpl() =
     default;
 
-// static
-void BoundSessionCookieRefreshServiceImpl::RegisterProfilePrefs(
-    user_prefs::PrefRegistrySyncable* registry) {
-  registry->RegisterStringPref(kRegistrationParamsPref, std::string());
-}
-
 void BoundSessionCookieRefreshServiceImpl::Initialize() {
   absl::optional<bound_session_credentials::RegistrationParams>
-      registration_params = GetRegistrationParams();
+      registration_params = session_params_storage_->ReadParams();
   if (registration_params.has_value()) {
     InitializeBoundSession(registration_params.value());
   }
 }
 
 void BoundSessionCookieRefreshServiceImpl::RegisterNewBoundSession(
     const bound_session_credentials::RegistrationParams& params) {
-  if (!IsValidRegistrationParams(params) ||
-      !PersistRegistrationParams(params)) {
+  if (!session_params_storage_->SaveParams(params)) {
     DVLOG(1) << "Invalid session params or failed to serialize bound session "
                 "registration params.";
     return;
@@ -149,55 +141,14 @@ void BoundSessionCookieRefreshServiceImpl::OnRegistrationRequestComplete(
   active_registration_request_.reset();
 }
 
-bool BoundSessionCookieRefreshServiceImpl::IsValidRegistrationParams(
-    const bound_session_credentials::RegistrationParams& registration_params) {
-  // TODO(crbug.com/1441168): Check for validity of other fields once they are
-  // available.
-  return registration_params.has_session_id() &&
-         registration_params.has_wrapped_key();
-}
-
-bool BoundSessionCookieRefreshServiceImpl::PersistRegistrationParams(
-    const bound_session_credentials::RegistrationParams& registration_params) {
-  std::string serialized_params = registration_params.SerializeAsString();
-  if (serialized_params.empty()) {
-    return false;
-  }
-
-  std::string encoded_serialized_params;
-  base::Base64Encode(serialized_params, &encoded_serialized_params);
-  pref_service_->SetString(kRegistrationParamsPref, encoded_serialized_params);
-  return true;
-}
-
-absl::optional<bound_session_credentials::RegistrationParams>
-BoundSessionCookieRefreshServiceImpl::GetRegistrationParams() {
-  std::string encoded_params_str =
-      pref_service_->GetString(kRegistrationParamsPref);
-  if (encoded_params_str.empty()) {
-    return absl::nullopt;
-  }
-
-  std::string params_str;
-  if (!base::Base64Decode(encoded_params_str, &params_str)) {
-    return absl::nullopt;
-  }
-
-  bound_session_credentials::RegistrationParams params;
-  if (params.ParseFromString(params_str) && IsValidRegistrationParams(params)) {
-    return params;
-  }
-  return absl::nullopt;
-}
-
 void BoundSessionCookieRefreshServiceImpl::
     OnBoundSessionThrottlerParamsChanged() {
   UpdateAllRenderers();
 }
 
 void BoundSessionCookieRefreshServiceImpl::TerminateSession() {
   cookie_controller_.reset();
-  pref_service_->ClearPref(kRegistrationParamsPref);
+  session_params_storage_->ClearParams();
   UpdateAllRenderers();
 }
 

diff --git a/chrome/browser/signin/bound_session_credentials/bound_session_cookie_refresh_service_impl.h b/chrome/browser/signin/bound_session_credentials/bound_session_cookie_refresh_service_impl.h
@@ -25,30 +25,24 @@ namespace unexportable_keys {
 class UnexportableKeyService;
 }
 
-namespace user_prefs {
-class PrefRegistrySyncable;
-}
-
 namespace content {
 class StoragePartition;
 }
 
-class PrefService;
+class BoundSessionParamsStorage;
 
 class BoundSessionCookieRefreshServiceImpl
     : public BoundSessionCookieRefreshService,
       public BoundSessionCookieController::Delegate {
  public:
   explicit BoundSessionCookieRefreshServiceImpl(
       unexportable_keys::UnexportableKeyService& key_service,
-      PrefService* pref_service,
-      content::StoragePartition* storage_partion,
+      std::unique_ptr<BoundSessionParamsStorage> session_params_storage,
+      content::StoragePartition* storage_partition,
       network::NetworkConnectionTracker* network_connection_tracker);
 
   ~BoundSessionCookieRefreshServiceImpl() override;
 
-  static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
-
   // BoundSessionCookieRefreshService:
   void Initialize() override;
   // Can be called iff the kBoundSessionExplicitRegistration feature is enabled.
@@ -97,12 +91,6 @@ class BoundSessionCookieRefreshServiceImpl
   void OnRegistrationRequestComplete(
       absl::optional<bound_session_credentials::RegistrationParams>
           registration_params);
-  bool IsValidRegistrationParams(
-      const bound_session_credentials::RegistrationParams& registration_params);
-  bool PersistRegistrationParams(
-      const bound_session_credentials::RegistrationParams& registration_params);
-  absl::optional<bound_session_credentials::RegistrationParams>
-  GetRegistrationParams();
 
   // BoundSessionCookieController::Delegate
   void OnBoundSessionThrottlerParamsChanged() override;
@@ -118,7 +106,8 @@ class BoundSessionCookieRefreshServiceImpl
   void UpdateAllRenderers();
 
   const raw_ref<unexportable_keys::UnexportableKeyService> key_service_;
-  const raw_ptr<PrefService> pref_service_;
+  // Never null. Stored as `std::unique_ptr` for polymorphism.
+  const std::unique_ptr<BoundSessionParamsStorage> session_params_storage_;
   const raw_ptr<content::StoragePartition> storage_partition_;
   const raw_ptr<network::NetworkConnectionTracker> network_connection_tracker_;
   BoundSessionCookieControllerFactoryForTesting controller_factory_for_testing_;

diff --git a/...er/signin/bound_session_credentials/bound_session_cookie_refresh_service_impl_unittest.cc b/...er/signin/bound_session_credentials/bound_session_cookie_refresh_service_impl_unittest.cc
@@ -16,6 +16,7 @@
 #include "base/test/task_environment.h"
 #include "base/test/test_future.h"
 #include "chrome/browser/signin/bound_session_credentials/bound_session_cookie_controller.h"
+#include "chrome/browser/signin/bound_session_credentials/bound_session_params_storage.h"
 #include "chrome/browser/signin/bound_session_credentials/bound_session_registration_params.pb.h"
 #include "chrome/common/renderer_configuration.mojom.h"
 #include "components/signin/public/base/test_signin_client.h"
@@ -105,8 +106,7 @@ class BoundSessionCookieRefreshServiceImplTest : public testing::Test {
   const GURL kTestGaiaURL = GURL("https://google.com");
 
   BoundSessionCookieRefreshServiceImplTest() {
-    BoundSessionCookieRefreshServiceImpl::RegisterProfilePrefs(
-        prefs_.registry());
+    BoundSessionParamsStorage::RegisterProfilePrefs(prefs_.registry());
   }
 
   ~BoundSessionCookieRefreshServiceImplTest() override = default;
@@ -131,8 +131,9 @@ class BoundSessionCookieRefreshServiceImplTest : public testing::Test {
     if (!cookie_refresh_service_) {
       cookie_refresh_service_ =
           std::make_unique<BoundSessionCookieRefreshServiceImpl>(
-              fake_unexportable_key_service_, &prefs_, &storage_partition_,
-              content::GetNetworkConnectionTracker());
+              fake_unexportable_key_service_,
+              BoundSessionParamsStorage::CreatePrefsStorageForTesting(prefs_),
+              &storage_partition_, content::GetNetworkConnectionTracker());
       cookie_refresh_service_->set_controller_factory_for_testing(
           base::BindRepeating(&BoundSessionCookieRefreshServiceImplTest::
                                   GetBoundSessionCookieController,
@@ -418,7 +419,22 @@ TEST_F(BoundSessionCookieRefreshServiceImplTest, OverrideExistingBoundSession) {
   new_params.set_session_id("test_session_id_2");
   service->RegisterNewBoundSession(new_params);
 
-  EXPECT_TRUE(cookie_controller());
+  VerifyBoundSession();
+  // TODO(http://b/286222327): check registration params once they are
+  // properly passed to controller.
+}
+
+TEST_F(BoundSessionCookieRefreshServiceImplTest,
+       OverrideExistingBoundSessionWithInvalidParams) {
+  BoundSessionCookieRefreshServiceImpl* service = GetCookieRefreshServiceImpl();
+  service->RegisterNewBoundSession(CreateTestRegistrationParams());
+
+  auto invalid_params = CreateTestRegistrationParams();
+  invalid_params.clear_session_id();
+  service->RegisterNewBoundSession(invalid_params);
+
+  // Original session should not be modified.
+  VerifyBoundSession();
   // TODO(http://b/286222327): check registration params once they are
   // properly passed to controller.
 }