Record extension request timestamp

Record the timestamp when user request the extension for better UX on the
server side. The timestamp will be stored in the Pref as a
DictionaryValue with the format:
{ "$extension-id" : {"timestamp : "$java-time-as-string"}, }

The pref type is chagned from list to dict. It's safe to do so because the
only code path to set the value is behind a private API which is used by
no one now.

Also, allow ExtensionApiUnittest to pass TaskEnvironmentTraits to
BrowserWithTestWindowTest so that we can pause the clock for the test.


Bug: 1006899
Change-Id: I3c0463ed3847f6d14a14d31f6073355d6dc43716
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1940670
Commit-Queue: Owen Min <zmin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721624}

chrome/browser/enterprise_reporting/prefs.cc

@@ -23,7 +23,7 @@ void RegisterLocalStatePrefs(PrefRegistrySimple* registry) {
 
 void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterBooleanPref(prefs::kCloudExtensionRequestEnabled, false);
-  registry->RegisterListPref(prefs::kCloudExtensionRequestIds);
+  registry->RegisterDictionaryPref(prefs::kCloudExtensionRequestIds);
 }
 
 }  // namespace enterprise_reporting

chrome/browser/enterprise_reporting/profile_report_generator.cc

@@ -7,6 +7,7 @@
 #include "base/files/file_path.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/util/values/values_util.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/enterprise_reporting/extension_info.h"
 #include "chrome/browser/enterprise_reporting/policy_info.h"
@@ -14,6 +15,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
+#include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/pref_names.h"
 #include "components/prefs/pref_service.h"
 #include "components/signin/public/identity_manager/account_info.h"
@@ -101,11 +103,21 @@ void ProfileReportGenerator::GetPolicyFetchTimestampInfo() {
 void ProfileReportGenerator::GetExtensionRequest() {
   if (!extension_request_enabled_)
     return;
-  const auto pending_list = profile_->GetPrefs()
-                                ->GetList(prefs::kCloudExtensionRequestIds)
-                                ->GetList();
-  for (const base::Value& pending_request : pending_list)
-    report_->add_extension_requests()->set_id(pending_request.GetString());
+  const base::DictionaryValue* pending_requests =
+      profile_->GetPrefs()->GetDictionary(prefs::kCloudExtensionRequestIds);
+
+  // In case a corrupted profile prefs causing |pending_requests| to be null.
+  if (!pending_requests)
+    return;
+
+  for (const auto& it : *pending_requests) {
+    auto* request = report_->add_extension_requests();
+    request->set_id(it.first);
+    base::Optional<base::Time> timestamp = ::util::ValueToTime(
+        it.second->FindKey(extension_misc::kExtensionRequestTimestamp));
+    if (timestamp)
+      request->set_request_timestamp(timestamp->ToJavaTime());
+  }
 }
 
 }  // namespace enterprise_reporting

chrome/browser/enterprise_reporting/profile_report_generator_unittest.cc

@@ -6,8 +6,10 @@
 
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/util/values/values_util.h"
 #include "chrome/browser/profiles/profile_attributes_storage.h"
 #include "chrome/browser/signin/identity_test_environment_profile_adaptor.h"
+#include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile_manager.h"
@@ -22,6 +24,7 @@ namespace {
 constexpr char kProfile[] = "Profile";
 constexpr char kIdleProfile[] = "IdleProfile";
 constexpr char kExtensionId[] = "abcdefghijklmnopabcdefghijklmnop";
+constexpr int kFakeTime = 123456;
 
 }  // namespace
 
@@ -59,12 +62,17 @@ class ProfileReportGeneratorTest : public ::testing::Test {
   }
 
   void SetExtensionToPendingList(const std::vector<std::string>& ids) {
-    base::Value::ListStorage id_values;
-    for (auto id : ids)
-      id_values.push_back(base::Value(id));
+    std::unique_ptr<base::Value> id_values =
+        std::make_unique<base::Value>(base::Value::Type::DICTIONARY);
+    for (const auto& id : ids) {
+      base::Value request_data(base::Value::Type::DICTIONARY);
+      request_data.SetKey(
+          extension_misc::kExtensionRequestTimestamp,
+          ::util::TimeToValue(base::Time::FromJavaTime(kFakeTime)));
+      id_values->SetKey(id, std::move(request_data));
+    }
     profile()->GetTestingPrefService()->SetUserPref(
-        prefs::kCloudExtensionRequestIds,
-        std::make_unique<base::Value>(std::move(id_values)));
+        prefs::kCloudExtensionRequestIds, std::move(id_values));
   }
 
   TestingProfile* profile() { return profile_; }
@@ -114,8 +122,9 @@ TEST_F(ProfileReportGeneratorTest, PendingRequest) {
   SetExtensionToPendingList(ids);
 
   auto report = GenerateReport();
-  EXPECT_EQ(1, report->extension_requests_size());
+  ASSERT_EQ(1, report->extension_requests_size());
   EXPECT_EQ(kExtensionId, report->extension_requests(0).id());
+  EXPECT_EQ(kFakeTime, report->extension_requests(0).request_timestamp());
 }
 
 TEST_F(ProfileReportGeneratorTest, NoPendingRequestWhenItsDisabled) {

chrome/browser/extensions/BUILD.gn

@@ -773,6 +773,7 @@ jumbo_static_library("extensions") {
   ]
   deps = [
     "//apps",
+    "//base/util/values:values_util",
     "//build:branding_buildflags",
     "//chrome:extra_resources",
     "//chrome:resources",

chrome/browser/extensions/api/webstore_private/extension_install_status.cc

@@ -59,12 +59,9 @@ ExtensionInstallStatus GetWebstoreExtensionInstallStatus(
   if (extension_management->IsInstallationExplicitlyBlocked(extension_id))
     return kBlockedByPolicy;
 
-  const auto pending_list =
-      profile->GetPrefs()->GetList(prefs::kCloudExtensionRequestIds)->GetList();
-  if (std::any_of(pending_list.begin(), pending_list.end(),
-                  [&extension_id](const base::Value& pending_id) {
-                    return pending_id.GetString() == extension_id;
-                  })) {
+  if (profile->GetPrefs()
+          ->GetDictionary(prefs::kCloudExtensionRequestIds)
+          ->FindKey(extension_id)) {
     return kRequestPending;
   }
 

chrome/browser/extensions/api/webstore_private/extension_install_status_unittest.cc

@@ -9,6 +9,9 @@
 #include "base/json/json_reader.h"
 #include "base/macros.h"
 #include "base/strings/stringprintf.h"
+#include "base/time/time.h"
+#include "base/util/values/values_util.h"
+#include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
@@ -42,6 +45,7 @@ constexpr char kExtensionSettingsWithIdAllowed[] = R"({
     "installation_mode": "allowed"
   }
 })";
+
 }  // namespace
 
 class ExtensionInstallStatusTest : public BrowserWithTestWindowTest {
@@ -70,13 +74,17 @@ class ExtensionInstallStatusTest : public BrowserWithTestWindowTest {
                                                        std::move(value));
   }
 
-  void AddExtensionsToPendingList(const std::vector<ExtensionId>& ids) {
-    base::Value::ListStorage id_values;
-    for (const auto& id : ids)
-      id_values.push_back(base::Value(id));
+  void SetPendingList(const std::vector<ExtensionId>& ids) {
+    std::unique_ptr<base::Value> id_values =
+        std::make_unique<base::Value>(base::Value::Type::DICTIONARY);
+    for (const auto& id : ids) {
+      base::Value request_data(base::Value::Type::DICTIONARY);
+      request_data.SetKey(extension_misc::kExtensionRequestTimestamp,
+                          ::util::TimeToValue(base::Time::Now()));
+      id_values->SetKey(id, std::move(request_data));
+    }
     profile()->GetTestingPrefService()->SetUserPref(
-        prefs::kCloudExtensionRequestIds,
-        std::make_unique<base::Value>(std::move(id_values)));
+        prefs::kCloudExtensionRequestIds, std::move(id_values));
   }
 
  private:
@@ -181,7 +189,7 @@ TEST_F(ExtensionInstallStatusTest, PendingExtenisonIsWaitingToBeReviewed) {
   SetPolicy(prefs::kCloudExtensionRequestEnabled,
             std::make_unique<base::Value>(true));
   std::vector<ExtensionId> ids = {kExtensionId};
-  AddExtensionsToPendingList(ids);
+  SetPendingList(ids);
 
   // The extension is blocked by wildcard and pending approval.
   SetExtensionSettings(kExtensionSettingsWithWildcardBlocking);

chrome/browser/extensions/api/webstore_private/webstore_private_api.cc

@@ -17,6 +17,7 @@
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/util/values/values_util.h"
 #include "base/values.h"
 #include "base/version.h"
 #include "chrome/browser/bitmap_fetcher/bitmap_fetcher.h"
@@ -829,11 +830,14 @@ WebstorePrivateRequestExtensionFunction::Run() {
 
 void WebstorePrivateRequestExtensionFunction::AddExtensionToPendingList(
     const ExtensionId& id) {
-  ListPrefUpdate pending_list_update(
+  DictionaryPrefUpdate pending_requests_update(
       Profile::FromBrowserContext(browser_context())->GetPrefs(),
       prefs::kCloudExtensionRequestIds);
-  DCHECK(!base::Contains(pending_list_update->GetList(), base::Value(id)));
-  pending_list_update->GetList().push_back(base::Value(id));
+  DCHECK(!pending_requests_update->FindKey(id));
+  base::Value request_data(base::Value::Type::DICTIONARY);
+  request_data.SetKey(extension_misc::kExtensionRequestTimestamp,
+                      ::util::TimeToValue(base::Time::Now()));
+  pending_requests_update->SetKey(id, std::move(request_data));
 }
 
 }  // namespace extensions

chrome/browser/extensions/api/webstore_private/webstore_private_unittest.cc

@@ -8,8 +8,10 @@
 
 #include "base/macros.h"
 #include "base/strings/stringprintf.h"
+#include "base/util/values/values_util.h"
 #include "chrome/browser/extensions/extension_api_unittest.h"
 #include "chrome/browser/extensions/extension_function_test_utils.h"
+#include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/pref_names.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "extensions/browser/extension_registry.h"
@@ -19,12 +21,20 @@ namespace extensions {
 namespace {
 constexpr char kInvalidId[] = "Invalid id";
 constexpr char kExtensionId[] = "abcdefghijklmnopabcdefghijklmnop";
+constexpr int kFakeTime = 12345;
+
+base::Time GetFaketime() {
+  return base::Time::FromJavaTime(kFakeTime);
+}
+
 }  // namespace
 
 class WebstorePrivateExtensionInstallRequestBase : public ExtensionApiUnittest {
  public:
   using ExtensionInstallStatus = api::webstore_private::ExtensionInstallStatus;
-  WebstorePrivateExtensionInstallRequestBase() = default;
+  WebstorePrivateExtensionInstallRequestBase()
+      : ExtensionApiUnittest(
+            base::test::TaskEnvironment::TimeSource::MOCK_TIME) {}
 
   std::string GenerateArgs(const char* id) {
     return base::StringPrintf(R"(["%s"])", id);
@@ -71,6 +81,8 @@ TEST_F(WebstorePrivateGetExtensionStatusTest, ExtensionEnabled) {
 class WebstorePrivateRequestExtensionTest
     : public WebstorePrivateExtensionInstallRequestBase {
  public:
+  WebstorePrivateRequestExtensionTest() = default;
+
   void SetUp() override {
     WebstorePrivateExtensionInstallRequestBase::SetUp();
     profile()->GetTestingPrefService()->SetManagedPref(
@@ -80,24 +92,29 @@ class WebstorePrivateRequestExtensionTest
   }
 
   void SetPendingList(const std::vector<ExtensionId>& ids) {
-    base::Value::ListStorage id_values;
-    for (const auto& id : ids)
-      id_values.push_back(base::Value(id));
+    std::unique_ptr<base::Value> id_values =
+        std::make_unique<base::Value>(base::Value::Type::DICTIONARY);
+    for (const auto& id : ids) {
+      base::Value request_data(base::Value::Type::DICTIONARY);
+      request_data.SetKey(extension_misc::kExtensionRequestTimestamp,
+                          ::util::TimeToValue(GetFaketime()));
+      id_values->SetKey(id, std::move(request_data));
+    }
     profile()->GetTestingPrefService()->SetUserPref(
-        prefs::kCloudExtensionRequestIds,
-        std::make_unique<base::Value>(std::move(id_values)));
+        prefs::kCloudExtensionRequestIds, std::move(id_values));
   }
 
   void VerifyPendingList(
-      const std::vector<ExtensionId>& expected_pending_list) {
-    const base::span<const base::Value> actual_pending_list =
-        profile()
-            ->GetPrefs()
-            ->GetList(prefs::kCloudExtensionRequestIds)
-            ->GetList();
-    ASSERT_EQ(expected_pending_list.size(), actual_pending_list.size());
-    for (unsigned int i = 0; i < expected_pending_list.size(); i++)
-      EXPECT_EQ(expected_pending_list[i], actual_pending_list[i].GetString());
+      const std::map<ExtensionId, base::Time>& expected_pending_requests) {
+    const base::DictionaryValue* actual_pending_requests =
+        profile()->GetPrefs()->GetDictionary(prefs::kCloudExtensionRequestIds);
+    ASSERT_EQ(expected_pending_requests.size(),
+              actual_pending_requests->size());
+    for (const auto& expected_request : expected_pending_requests) {
+      EXPECT_EQ(::util::TimeToValue(expected_request.second),
+                *actual_pending_requests->FindKey(expected_request.first)
+                     ->FindKey(extension_misc::kExtensionRequestTimestamp));
+    }
   }
 };
 
@@ -123,7 +140,7 @@ TEST_F(WebstorePrivateRequestExtensionTest, UnrequestableExtension) {
 
 TEST_F(WebstorePrivateRequestExtensionTest, AlreadyPendingExtension) {
   SetPendingList({kExtensionId});
-  VerifyPendingList({kExtensionId});
+  VerifyPendingList({{kExtensionId, GetFaketime()}});
   auto function =
       base::MakeRefCounted<WebstorePrivateRequestExtensionFunction>();
   std::unique_ptr<base::Value> response =
@@ -131,7 +148,7 @@ TEST_F(WebstorePrivateRequestExtensionTest, AlreadyPendingExtension) {
   VerifyResponse(
       ExtensionInstallStatus::EXTENSION_INSTALL_STATUS_REQUEST_PENDING,
       response.get());
-  VerifyPendingList({kExtensionId});
+  VerifyPendingList({{kExtensionId, GetFaketime()}});
 }
 
 TEST_F(WebstorePrivateRequestExtensionTest, RequestExtension) {
@@ -142,7 +159,7 @@ TEST_F(WebstorePrivateRequestExtensionTest, RequestExtension) {
   VerifyResponse(
       ExtensionInstallStatus::EXTENSION_INSTALL_STATUS_REQUEST_PENDING,
       response.get());
-  VerifyPendingList({kExtensionId});
+  VerifyPendingList({{kExtensionId, base::Time::Now()}});
 }
 
 }  // namespace extensions

chrome/browser/extensions/extension_api_unittest.cc

@@ -17,9 +17,6 @@ namespace utils = extension_function_test_utils;
 
 namespace extensions {
 
-ExtensionApiUnittest::ExtensionApiUnittest() {
-}
-
 ExtensionApiUnittest::~ExtensionApiUnittest() {
 }
 

chrome/browser/extensions/extension_api_unittest.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 #include <string>
+#include <utility>
 
 #include "base/memory/ref_counted.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
@@ -35,7 +36,10 @@ namespace extensions {
 // in extensions/browser/api_unittest.h.
 class ExtensionApiUnittest : public BrowserWithTestWindowTest {
  public:
-  ExtensionApiUnittest();
+  template <typename... TaskEnvironmentTraits>
+  explicit ExtensionApiUnittest(TaskEnvironmentTraits&&... traits)
+      : BrowserWithTestWindowTest(
+            std::forward<TaskEnvironmentTraits>(traits)...) {}
   ~ExtensionApiUnittest() override;
 
   const Extension* extension() const { return extension_.get(); }

chrome/common/extensions/extension_constants.cc

@@ -131,4 +131,5 @@ const char kAppStateCannotRun[] = "cannot_run";
 const char kAppStateReadyToRun[] = "ready_to_run";
 
 const char kMediaFileSystemPathPart[] = "_";
+const char kExtensionRequestTimestamp[] = "timestamp";
 }  // namespace extension_misc

chrome/common/extensions/extension_constants.h

@@ -267,6 +267,10 @@ extern const char kAppStateReadyToRun[];
 
 // The path part of the file system url used for media file systems.
 extern const char kMediaFileSystemPathPart[];
+
+// The key name of extension request timestamp used by the
+// prefs::kCloudExtensionRequestIds preference.
+extern const char kExtensionRequestTimestamp[];
 }  // namespace extension_misc
 
 #endif  // CHROME_COMMON_EXTENSIONS_EXTENSION_CONSTANTS_H_