[DBSC] Fix AreParamsValid

This CL does the following:
- Moves `AreParamsValid` to `bound_session_params_util`.
- Ensures not only required fields are set but that they don't have
  empty values.
- Checks for the presence of non-empty site.

Bug: b/293985274
Change-Id: I139b08cbfaad3e0798ccdca7a5fc59db36933681
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4942159
Reviewed-by: Alex Ilin <alexilin@chromium.org>
Commit-Queue: Monica Basta <msalama@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1210744}

chrome/browser/signin/bound_session_credentials/bound_session_params_storage.cc

@@ -7,6 +7,7 @@
 #include "base/base64.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/bound_session_credentials/bound_session_params.pb.h"
+#include "chrome/browser/signin/bound_session_credentials/bound_session_params_util.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
@@ -41,7 +42,7 @@ BoundSessionParamsPrefsStorage::~BoundSessionParamsPrefsStorage() = default;
 
 bool BoundSessionParamsPrefsStorage::SaveParams(
     const bound_session_credentials::BoundSessionParams& params) {
-  if (!AreParamsValid(params)) {
+  if (!bound_session_credentials::AreParamsValid(params)) {
     return false;
   }
 
@@ -70,7 +71,8 @@ BoundSessionParamsPrefsStorage::ReadParams() const {
   }
 
   bound_session_credentials::BoundSessionParams params;
-  if (params.ParseFromString(params_str) && AreParamsValid(params)) {
+  if (params.ParseFromString(params_str) &&
+      bound_session_credentials::AreParamsValid(params)) {
     return params;
   }
   return absl::nullopt;
@@ -105,7 +107,7 @@ BoundSessionParamsInMemoryStorage::~BoundSessionParamsInMemoryStorage() =
 
 bool BoundSessionParamsInMemoryStorage::SaveParams(
     const bound_session_credentials::BoundSessionParams& params) {
-  if (!AreParamsValid(params)) {
+  if (!bound_session_credentials::AreParamsValid(params)) {
     return false;
   }
 
@@ -145,12 +147,3 @@ void BoundSessionParamsStorage::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterStringPref(kBoundSessionParamsPref, std::string());
 }
-
-// static
-bool BoundSessionParamsStorage::AreParamsValid(
-    const bound_session_credentials::BoundSessionParams& bound_session_params) {
-  // TODO(crbug.com/1441168): Check for validity of other fields once they are
-  // available.
-  return bound_session_params.has_session_id() &&
-         bound_session_params.has_wrapped_key();
-}

chrome/browser/signin/bound_session_credentials/bound_session_params_storage.h

@@ -45,10 +45,6 @@ class BoundSessionParamsStorage {
 
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
-  static bool AreParamsValid(
-      const bound_session_credentials::BoundSessionParams&
-          bound_session_params);
-
   // Saves `params` to storage. Overwrites existing params if any. `params` are
   // verified before being saved.
   // Returns whether the new parameters were saved. In case of a failure, keeps

chrome/browser/signin/bound_session_credentials/bound_session_params_storage_unittest.cc

@@ -35,25 +35,6 @@ MATCHER_P(EqualsProto, message, "") {
 
 }  // namespace
 
-TEST(BoundSessionParamsStorageAreParamsValidTest, Valid) {
-  EXPECT_TRUE(BoundSessionParamsStorage::AreParamsValid(
-      CreateValidBoundSessionParams()));
-}
-
-TEST(BoundSessionParamsStorageAreParamsValidTest, InvalidMissingSessionId) {
-  bound_session_credentials::BoundSessionParams params =
-      CreateValidBoundSessionParams();
-  params.clear_session_id();
-  EXPECT_FALSE(BoundSessionParamsStorage::AreParamsValid(params));
-}
-
-TEST(BoundSessionParamsStorageAreParamsValidTest, InvalidMissingWrappedKey) {
-  bound_session_credentials::BoundSessionParams params =
-      CreateValidBoundSessionParams();
-  params.clear_wrapped_key();
-  EXPECT_FALSE(BoundSessionParamsStorage::AreParamsValid(params));
-}
-
 class BoundSessionParamsStorageTest : public testing::TestWithParam<bool> {
  public:
   BoundSessionParamsStorageTest() : storage_(CreateStorage()) {}

chrome/browser/signin/bound_session_credentials/bound_session_params_util.cc

@@ -20,4 +20,14 @@ base::Time TimestampToTime(const Timestamp& timestamp) {
       base::Microseconds(timestamp.microseconds()));
 }
 
+bool AreParamsValid(const BoundSessionParams& bound_session_params) {
+  // TODO(crbug.com/1441168): Check for validity of other fields once they are
+  // available.
+  // Note: The check for params validity checks for empty value as
+  // `bound_session_params.has*()` doesn't check against explicitly set empty
+  // value.
+  return !bound_session_params.session_id().empty() &&
+         !bound_session_params.site().empty() &&
+         !bound_session_params.wrapped_key().empty();
+}
 }  // namespace bound_session_credentials

chrome/browser/signin/bound_session_credentials/bound_session_params_util.h

@@ -14,6 +14,8 @@ Timestamp TimeToTimestamp(base::Time time);
 
 base::Time TimestampToTime(const Timestamp& timestamp);
 
+bool AreParamsValid(const BoundSessionParams& bound_session_params);
+
 }  // namespace bound_session_credentials
 
 #endif  // CHROME_BROWSER_SIGNIN_BOUND_SESSION_CREDENTIALS_BOUND_SESSION_PARAMS_UTIL_H_

chrome/browser/signin/bound_session_credentials/bound_session_params_util_unittest.cc

@@ -8,11 +8,51 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace bound_session_credentials {
+namespace {
+BoundSessionParams CreateValidBoundSessionParams() {
+  BoundSessionParams params;
+  params.set_session_id("123");
+  params.set_site("https://example.org");
+  params.set_wrapped_key("456");
+  return params;
+}
+}  // namespace
 
 TEST(BoundSessionParamsUtilTest, Timestamp) {
   base::Time time =
       base::Time::UnixEpoch() + base::Milliseconds(987984);  // arbitrary
   EXPECT_EQ(TimestampToTime(TimeToTimestamp(time)), time);
 }
 
+TEST(BoundSessionParamsUtilTest, ParamsValid) {
+  EXPECT_TRUE(AreParamsValid(CreateValidBoundSessionParams()));
+}
+
+TEST(BoundSessionParamsUtilTest, ParamsInvalidMissingSessionId) {
+  BoundSessionParams params = CreateValidBoundSessionParams();
+  params.set_session_id("");
+  EXPECT_FALSE(AreParamsValid(params));
+
+  params.clear_session_id();
+  EXPECT_FALSE(AreParamsValid(params));
+}
+
+TEST(BoundSessionParamsUtilTest, ParamsInvalidMissingWrappedKey) {
+  BoundSessionParams params = CreateValidBoundSessionParams();
+  params.set_wrapped_key("");
+  EXPECT_FALSE(AreParamsValid(params));
+
+  params.clear_wrapped_key();
+  EXPECT_FALSE(AreParamsValid(params));
+}
+
+TEST(BoundSessionParamsUtilTest, ParamsInvalidMissingSite) {
+  BoundSessionParams params = CreateValidBoundSessionParams();
+  params.set_site("");
+  EXPECT_FALSE(AreParamsValid(params));
+
+  params.clear_site();
+  EXPECT_FALSE(AreParamsValid(params));
+}
+
 }  // namespace bound_session_credentials

chrome/browser/signin/bound_session_credentials/bound_session_registration_fetcher_impl.cc

@@ -138,7 +138,7 @@ void BoundSessionRegistrationFetcherImpl::OnURLLoaderComplete(
           net::SchemefulSite(registration_params_.RegistrationEndpoint())
               .Serialize(),
           *session_id, wrapped_key_str_);
-  if (!BoundSessionParamsStorage::AreParamsValid(session_params)) {
+  if (!bound_session_credentials::AreParamsValid(session_params)) {
     RunCallbackAndRecordMetrics(
         base::unexpected(RegistrationError::kInvalidSessionParams));
     return;