[DBSC] Explicitly start and terminate a session

Remove `OnBoundSessionUpdated` and `IsBoundSession` from `BoundSessionCookieRefreshService`. Explicitly create/terminate the session. Bug: b/263264391 Change-Id: I133d7b1f13781f96136271cb93796b9c06282a57 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4802150 Commit-Queue: Monica Basta <msalama@chromium.org> Reviewed-by: Alex Ilin <alexilin@chromium.org> Cr-Commit-Position: refs/heads/main@{#1187172}
chromium · Aug 23, 2023 · 41fe8e7 · 41fe8e7
1 parent 6f3bf7c
commit 41fe8e7
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 64 deletions.
diff --git a/chrome/browser/signin/bound_session_credentials/bound_session_cookie_refresh_service.h b/chrome/browser/signin/bound_session_credentials/bound_session_cookie_refresh_service.h
@@ -45,9 +45,6 @@ class BoundSessionCookieRefreshService
   virtual void MaybeTerminateSession(
       const net::HttpResponseHeaders* headers) = 0;
 
-  // Returns true if session is bound.
-  virtual bool IsBoundSession() const = 0;
-
   // Returns bound session params.
   virtual chrome::mojom::BoundSessionParamsPtr GetBoundSessionParams()
       const = 0;

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
@@ -8,7 +8,6 @@
 
 #include "base/base64.h"
 #include "base/check.h"
-#include "base/containers/span.h"
 #include "base/functional/bind.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/signin/bound_session_credentials/bound_session_cookie_controller.h"
@@ -19,7 +18,6 @@
 #include "components/prefs/pref_service.h"
 #include "components/signin/public/base/signin_client.h"
 #include "content/public/browser/storage_partition.h"
-#include "google_apis/gaia/gaia_urls.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
 
@@ -47,7 +45,11 @@ void BoundSessionCookieRefreshServiceImpl::RegisterProfilePrefs(
 }
 
 void BoundSessionCookieRefreshServiceImpl::Initialize() {
-  OnBoundSessionUpdated();
+  absl::optional<bound_session_credentials::RegistrationParams>
+      registration_params = GetRegistrationParams();
+  if (registration_params.has_value()) {
+    InitializeBoundSession(registration_params.value());
+  }
 }
 
 void BoundSessionCookieRefreshServiceImpl::RegisterNewBoundSession(
@@ -59,9 +61,8 @@ void BoundSessionCookieRefreshServiceImpl::RegisterNewBoundSession(
     return;
   }
   // New session should override an existing one.
-  ResetBoundSession();
-
-  OnBoundSessionUpdated();
+  cookie_controller_.reset();
+  InitializeBoundSession(params);
 }
 
 void BoundSessionCookieRefreshServiceImpl::MaybeTerminateSession(
@@ -78,10 +79,6 @@ void BoundSessionCookieRefreshServiceImpl::MaybeTerminateSession(
   }
 }
 
-bool BoundSessionCookieRefreshServiceImpl::IsBoundSession() const {
-  return pref_service_->HasPrefPath(kRegistrationParamsPref);
-}
-
 chrome::mojom::BoundSessionParamsPtr
 BoundSessionCookieRefreshServiceImpl::GetBoundSessionParams() const {
   if (!cookie_controller_) {
@@ -106,12 +103,11 @@ void BoundSessionCookieRefreshServiceImpl::
 
 void BoundSessionCookieRefreshServiceImpl::OnRequestBlockedOnCookie(
     OnRequestBlockedOnCookieCallback resume_blocked_request) {
-  if (!IsBoundSession()) {
+  if (!cookie_controller_) {
     // Session has been terminated.
     std::move(resume_blocked_request).Run();
     return;
   }
-  CHECK(cookie_controller_);
   cookie_controller_->OnRequestBlockedOnCookie(
       std::move(resume_blocked_request));
 }
@@ -197,8 +193,9 @@ void BoundSessionCookieRefreshServiceImpl::OnBoundSessionParamsChanged() {
 }
 
 void BoundSessionCookieRefreshServiceImpl::TerminateSession() {
+  cookie_controller_.reset();
   pref_service_->ClearPref(kRegistrationParamsPref);
-  OnBoundSessionUpdated();
+  UpdateAllRenderers();
 }
 
 std::unique_ptr<BoundSessionCookieController>
@@ -213,33 +210,15 @@ BoundSessionCookieRefreshServiceImpl::CreateBoundSessionCookieController(
                                                    cookie_names, this);
 }
 
-void BoundSessionCookieRefreshServiceImpl::InitializeBoundSession() {
+void BoundSessionCookieRefreshServiceImpl::InitializeBoundSession(
+    const bound_session_credentials::RegistrationParams& registration_params) {
   CHECK(!cookie_controller_);
   constexpr char k1PSIDTSCookieName[] = "__Secure-1PSIDTS";
   constexpr char k3PSIDTSCookieName[] = "__Secure-3PSIDTS";
 
-  absl::optional<bound_session_credentials::RegistrationParams>
-      registration_params = GetRegistrationParams();
-  if (!registration_params) {
-    TerminateSession();
-    return;
-  }
-
   cookie_controller_ = CreateBoundSessionCookieController(
-      registration_params.value(), {k1PSIDTSCookieName, k3PSIDTSCookieName});
+      registration_params, {k1PSIDTSCookieName, k3PSIDTSCookieName});
   cookie_controller_->Initialize();
-}
-
-void BoundSessionCookieRefreshServiceImpl::ResetBoundSession() {
-  cookie_controller_.reset();
-}
-
-void BoundSessionCookieRefreshServiceImpl::OnBoundSessionUpdated() {
-  if (!IsBoundSession()) {
-    ResetBoundSession();
-  } else {
-    InitializeBoundSession();
-  }
   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
@@ -53,7 +53,6 @@ class BoundSessionCookieRefreshServiceImpl
   void RegisterNewBoundSession(
       const bound_session_credentials::RegistrationParams& params) override;
   void MaybeTerminateSession(const net::HttpResponseHeaders* headers) override;
-  bool IsBoundSession() const override;
   chrome::mojom::BoundSessionParamsPtr GetBoundSessionParams() const override;
   void AddBoundSessionRequestThrottledListenerReceiver(
       mojo::PendingReceiver<chrome::mojom::BoundSessionRequestThrottledListener>
@@ -109,9 +108,8 @@ class BoundSessionCookieRefreshServiceImpl
   CreateBoundSessionCookieController(
       const bound_session_credentials::RegistrationParams& registration_params,
       const base::flat_set<std::string>& cookie_names);
-  void InitializeBoundSession();
-  void ResetBoundSession();
-  void OnBoundSessionUpdated();
+  void InitializeBoundSession(
+      const bound_session_credentials::RegistrationParams& registration_params);
 
   void UpdateAllRenderers();
 

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
@@ -189,16 +189,16 @@ class BoundSessionCookieRefreshServiceImplTest : public testing::Test {
 
   void VerifyBoundSession() {
     CHECK(cookie_refresh_service_);
-    EXPECT_TRUE(cookie_refresh_service_->IsBoundSession());
     EXPECT_TRUE(cookie_refresh_service_->GetBoundSessionParams());
+    EXPECT_TRUE(prefs()->HasPrefPath(kRegistrationParamsPref));
     EXPECT_TRUE(cookie_controller());
   }
 
   void VerifyNoBoundSession() {
     CHECK(cookie_refresh_service_);
-    EXPECT_FALSE(cookie_refresh_service_->IsBoundSession());
     EXPECT_FALSE(cookie_refresh_service_->GetBoundSessionParams());
     EXPECT_FALSE(cookie_controller());
+    EXPECT_FALSE(prefs()->HasPrefPath(kRegistrationParamsPref));
   }
 
  private:
@@ -213,8 +213,8 @@ class BoundSessionCookieRefreshServiceImplTest : public testing::Test {
 
 TEST_F(BoundSessionCookieRefreshServiceImplTest, VerifyControllerParams) {
   SetupPreConditionForBoundSession();
-  BoundSessionCookieRefreshServiceImpl* service = GetCookieRefreshServiceImpl();
-  EXPECT_TRUE(service->IsBoundSession());
+  GetCookieRefreshServiceImpl();
+  VerifyBoundSession();
   FakeBoundSessionCookieController* controller = cookie_controller();
   EXPECT_TRUE(controller);
   EXPECT_EQ(controller->url(), kTestGaiaURL);
@@ -238,7 +238,7 @@ TEST_F(BoundSessionCookieRefreshServiceImplTest,
        VerifyBoundSessionParamsBoundSession) {
   SetupPreConditionForBoundSession();
   BoundSessionCookieRefreshServiceImpl* service = GetCookieRefreshServiceImpl();
-  EXPECT_TRUE(service->IsBoundSession());
+  ASSERT_TRUE(cookie_controller());
 
   chrome::mojom::BoundSessionParamsPtr bound_session_params =
       service->GetBoundSessionParams();
@@ -250,10 +250,9 @@ TEST_F(BoundSessionCookieRefreshServiceImplTest,
        RefreshBoundSessionCookieBoundSession) {
   SetupPreConditionForBoundSession();
   BoundSessionCookieRefreshServiceImpl* service = GetCookieRefreshServiceImpl();
-  EXPECT_TRUE(service->IsBoundSession());
+  EXPECT_TRUE(cookie_controller());
   base::test::TestFuture<void> future;
   service->OnRequestBlockedOnCookie(future.GetCallback());
-  EXPECT_TRUE(cookie_controller());
 
   EXPECT_FALSE(future.IsReady());
   cookie_controller()->SimulateRefreshBoundSessionCompleted();
@@ -263,7 +262,7 @@ TEST_F(BoundSessionCookieRefreshServiceImplTest,
 TEST_F(BoundSessionCookieRefreshServiceImplTest,
        RefreshBoundSessionCookieUnboundSession) {
   BoundSessionCookieRefreshServiceImpl* service = GetCookieRefreshServiceImpl();
-  EXPECT_FALSE(service->IsBoundSession());
+  EXPECT_FALSE(cookie_controller());
 
   // Unbound session, the callback should be called immediately.
   base::test::TestFuture<void> future;
@@ -274,15 +273,16 @@ TEST_F(BoundSessionCookieRefreshServiceImplTest,
 TEST_F(BoundSessionCookieRefreshServiceImplTest,
        UpdateAllRenderersOnBoundSessionStarted) {
   BoundSessionCookieRefreshServiceImpl* service = GetCookieRefreshServiceImpl();
-  EXPECT_FALSE(service->IsBoundSession());
+  EXPECT_FALSE(cookie_controller());
+  EXPECT_FALSE(service->GetBoundSessionParams());
   base::MockRepeatingCallback<void()> renderer_updater;
   EXPECT_CALL(renderer_updater, Run()).Times(0);
   SetRendererUpdater(renderer_updater.Get());
   testing::Mock::VerifyAndClearExpectations(&renderer_updater);
 
   // Create bound session.
   EXPECT_CALL(renderer_updater, Run()).WillOnce([&] {
-    EXPECT_TRUE(service->IsBoundSession());
+    EXPECT_TRUE(cookie_controller());
     EXPECT_FALSE(service->GetBoundSessionParams().is_null());
   });
   service->RegisterNewBoundSession(CreateTestRegistrationParams());
@@ -295,7 +295,7 @@ TEST_F(BoundSessionCookieRefreshServiceImplTest,
   EXPECT_CALL(renderer_updater, Run()).Times(0);
   SetupPreConditionForBoundSession();
   BoundSessionCookieRefreshServiceImpl* service = GetCookieRefreshServiceImpl();
-  EXPECT_TRUE(service->IsBoundSession());
+  EXPECT_TRUE(cookie_controller());
   SetRendererUpdater(renderer_updater.Get());
   testing::Mock::VerifyAndClearExpectations(&renderer_updater);
 
@@ -305,7 +305,7 @@ TEST_F(BoundSessionCookieRefreshServiceImplTest,
   testing::Mock::VerifyAndClearExpectations(&renderer_updater);
 
   EXPECT_CALL(renderer_updater, Run()).WillOnce([&] {
-    EXPECT_TRUE(service->IsBoundSession());
+    EXPECT_TRUE(cookie_controller());
     EXPECT_FALSE(service->GetBoundSessionParams().is_null());
   });
   cookie_controller()->SimulateOnCookieExpirationDateChanged(k3PSIDTSCookieName,
@@ -318,8 +318,8 @@ TEST_F(BoundSessionCookieRefreshServiceImplTest,
   base::MockRepeatingCallback<void()> renderer_updater;
   EXPECT_CALL(renderer_updater, Run()).Times(0);
   SetupPreConditionForBoundSession();
-  BoundSessionCookieRefreshServiceImpl* service = GetCookieRefreshServiceImpl();
-  EXPECT_TRUE(service->IsBoundSession());
+  GetCookieRefreshServiceImpl();
+  EXPECT_TRUE(cookie_controller());
   SetRendererUpdater(renderer_updater.Get());
   testing::Mock::VerifyAndClearExpectations(&renderer_updater);
 
@@ -333,7 +333,6 @@ TEST_F(BoundSessionCookieRefreshServiceImplTest,
 TEST_F(BoundSessionCookieRefreshServiceImplTest, TerminateSession) {
   SetupPreConditionForBoundSession();
   BoundSessionCookieRefreshServiceImpl* service = GetCookieRefreshServiceImpl();
-  EXPECT_TRUE(service->IsBoundSession());
   EXPECT_TRUE(service->GetBoundSessionParams());
 
   cookie_controller()->SimulateTerminateSession();
@@ -377,7 +376,7 @@ TEST_F(BoundSessionCookieRefreshServiceImplTest,
        AddBoundSessionRequestThrottledListenerReceivers) {
   SetupPreConditionForBoundSession();
   BoundSessionCookieRefreshServiceImpl* service = GetCookieRefreshServiceImpl();
-  EXPECT_TRUE(service->IsBoundSession());
+  ASSERT_TRUE(cookie_controller());
   mojo::Remote<chrome::mojom::BoundSessionRequestThrottledListener> listener_1;
   mojo::Remote<chrome::mojom::BoundSessionRequestThrottledListener> listener_2;
   service->AddBoundSessionRequestThrottledListenerReceiver(
@@ -401,12 +400,10 @@ TEST_F(BoundSessionCookieRefreshServiceImplTest,
 
 TEST_F(BoundSessionCookieRefreshServiceImplTest, RegisterNewBoundSession) {
   BoundSessionCookieRefreshServiceImpl* service = GetCookieRefreshServiceImpl();
-  EXPECT_FALSE(service->IsBoundSession());
-  EXPECT_FALSE(cookie_controller());
+  VerifyNoBoundSession();
 
   service->RegisterNewBoundSession(CreateTestRegistrationParams());
-  EXPECT_TRUE(service->IsBoundSession());
-  EXPECT_TRUE(cookie_controller());
+  VerifyBoundSession();
   // TODO(http://b/286222327): check registration params once they are
   // properly passed to controller.
 }
@@ -419,7 +416,6 @@ TEST_F(BoundSessionCookieRefreshServiceImplTest, OverrideExistingBoundSession) {
   new_params.set_session_id("test_session_id_2");
   service->RegisterNewBoundSession(new_params);
 
-  EXPECT_TRUE(service->IsBoundSession());
   EXPECT_TRUE(cookie_controller());
   // TODO(http://b/286222327): check registration params once they are
   // properly passed to controller.

diff --git a/...und_session_credentials/bound_session_request_throttled_listener_browser_impl_unittest.cc b/...und_session_credentials/bound_session_request_throttled_listener_browser_impl_unittest.cc
@@ -34,8 +34,6 @@ class FakeBoundSessionCookieRefreshService
   void MaybeTerminateSession(const net::HttpResponseHeaders* headers) override {
   }
 
-  bool IsBoundSession() const override { return true; }
-
   chrome::mojom::BoundSessionParamsPtr GetBoundSessionParams() const override {
     return chrome::mojom::BoundSessionParams::New();
   }