Use Chrome to provide remote logging permission
The Google-branded updater should use stable Chrome to infer remote
logging opt-in. Inject the permission provides as an external constant
so that it may be overridden in integration tests.
Bug: 371595849
Change-Id: Icc2c4f68d06d2bffa8543fab8292146382ea421e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6612962
Commit-Queue: Noah Rose Ledesma <[email protected]>
Reviewed-by: Joshua Pawlicki <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1468771}
diff --git a/chrome/updater/BUILD.gn b/chrome/updater/BUILD.gn
index e8716897..4582e54 100644
--- a/chrome/updater/BUILD.gn
+++ b/chrome/updater/BUILD.gn
@@ -474,6 +474,7 @@
":branding_header",
":constants_header",
"//base",
+ "//build:branding_buildflags",
"//components/crx_file",
"//url",
]
diff --git a/chrome/updater/apply_updater_branding.gni b/chrome/updater/apply_updater_branding.gni
index 5a45eeca..c8c7527 100644
--- a/chrome/updater/apply_updater_branding.gni
+++ b/chrome/updater/apply_updater_branding.gni
@@ -21,6 +21,8 @@
"-e",
"BROWSER_PRODUCT_NAME=\"$browser_product_name\"",
"-e",
+ "BROWSER_APPID=\"$browser_appid\"",
+ "-e",
"COMPANY_FULLNAME=\"$updater_company_full_name\"",
"-e",
"COMPANY_SHORTNAME=\"$updater_company_short_name\"",
diff --git a/chrome/updater/branding.gni b/chrome/updater/branding.gni
index d9e28c39..43539ce 100644
--- a/chrome/updater/branding.gni
+++ b/chrome/updater/branding.gni
@@ -32,6 +32,7 @@
updater_metainstaller_name = "Chromium Installer"
mac_team_identifier = "PLACEHOLDER"
updater_appid = "{6e8ffa8f-e7e2-4000-9884-589283c27015}"
+ browser_appid = "{5d8d08af-2df9-4da2-86c1-eac353a0ca32}"
qualification_appid = "{43f3a046-04b3-4443-a770-d67dae90e440}"
legacy_service_name_prefix = "cupdate"
prefs_access_mutex = "{A6B9ECD5-772A-4D3F-BFEB-CF9340534A3E}"
diff --git a/chrome/updater/constants.h b/chrome/updater/constants.h
index 150a6c4..a84b5a6 100644
--- a/chrome/updater/constants.h
+++ b/chrome/updater/constants.h
@@ -5,6 +5,8 @@
#ifndef CHROME_UPDATER_CONSTANTS_H_
#define CHROME_UPDATER_CONSTANTS_H_
+#include <optional>
+
#include "base/time/time.h"
#include "build/build_config.h"
#include "components/update_client/update_client_errors.h"
@@ -261,6 +263,8 @@
"crx_verifier_format";
inline constexpr char kDevOverrideKeyMinumumEventLoggingCooldownSeconds[] =
"minimum_event_logging_cooldown_seconds";
+inline constexpr char kDevOverrideKeyEventLoggingPermissionProvider[] =
+ "event_logging_permission_provider";
inline constexpr char kDevOverrideKeyDictPolicies[] = "dict_policies";
// TODO(crbug.com/389965546): remove this once the checked-in old updater builds
diff --git a/chrome/updater/external_constants.h b/chrome/updater/external_constants.h
index bbc5966..c3c459b 100644
--- a/chrome/updater/external_constants.h
+++ b/chrome/updater/external_constants.h
@@ -60,6 +60,13 @@
// Minimum amount of time between successive event logging transmissions.
virtual base::TimeDelta MinimumEventLoggingCooldown() const = 0;
+ // Indicates which application remote event logging permissions should be
+ // inferred from. Nullopt indicates that logging is unconditionally disabled.
+ // The meaning of the provider is platform specific; on macOS it is the
+ // basename of an application directory, on Windows it is an AppId.
+ virtual std::optional<std::string> GetEventLoggingPermissionProvider()
+ const = 0;
+
// Policies for the `PolicyManager` surfaced by external constants.
virtual base::Value::Dict DictPolicies() const = 0;
diff --git a/chrome/updater/external_constants_default.cc b/chrome/updater/external_constants_default.cc
index 551de40..690899d 100644
--- a/chrome/updater/external_constants_default.cc
+++ b/chrome/updater/external_constants_default.cc
@@ -10,6 +10,8 @@
#include "base/memory/scoped_refptr.h"
#include "base/time/time.h"
#include "base/values.h"
+#include "build/branding_buildflags.h"
+#include "build/build_config.h"
#include "chrome/updater/constants.h"
#include "chrome/updater/external_constants.h"
#include "chrome/updater/updater_branding.h"
@@ -68,6 +70,17 @@
return kMinimumEventLoggingCooldown;
}
+ std::optional<std::string> GetEventLoggingPermissionProvider()
+ const override {
+#if BUILDFLAG(GOOGLE_CHROME_BRANDING) && BUILDFLAG(IS_MAC)
+ return BROWSER_NAME_STRING;
+#elif BUILDFLAG(GOOGLE_CHROME_BRANDING) && BUILDFLAG(IS_WIN)
+ return BROWSER_APP_ID;
+#else
+ return std::nullopt;
+#endif
+ }
+
private:
~DefaultExternalConstants() override = default;
};
diff --git a/chrome/updater/external_constants_override.cc b/chrome/updater/external_constants_override.cc
index 47e5dc7..7e952a1 100644
--- a/chrome/updater/external_constants_override.cc
+++ b/chrome/updater/external_constants_override.cc
@@ -214,6 +214,22 @@
return base::Seconds(minimum_event_logging_cooldown_seconds->GetInt());
}
+std::optional<std::string>
+ExternalConstantsOverrider::GetEventLoggingPermissionProvider() const {
+ if (!override_values_.contains(
+ kDevOverrideKeyEventLoggingPermissionProvider)) {
+ return next_provider_->GetEventLoggingPermissionProvider();
+ }
+
+ const base::Value* event_logging_permission_provider =
+ override_values_.Find(kDevOverrideKeyEventLoggingPermissionProvider);
+ CHECK(event_logging_permission_provider->is_string())
+ << "Unexpected type of override["
+ << kDevOverrideKeyEventLoggingPermissionProvider << "]: "
+ << base::Value::GetTypeName(event_logging_permission_provider->type());
+ return event_logging_permission_provider->GetString();
+}
+
base::Value::Dict ExternalConstantsOverrider::DictPolicies() const {
if (!override_values_.contains(kDevOverrideKeyDictPolicies)) {
return next_provider_->DictPolicies();
diff --git a/chrome/updater/external_constants_override.h b/chrome/updater/external_constants_override.h
index be03c71..9757fea 100644
--- a/chrome/updater/external_constants_override.h
+++ b/chrome/updater/external_constants_override.h
@@ -56,6 +56,7 @@
base::TimeDelta ServerKeepAliveTime() const override;
crx_file::VerifierFormat CrxVerifierFormat() const override;
base::TimeDelta MinimumEventLoggingCooldown() const override;
+ std::optional<std::string> GetEventLoggingPermissionProvider() const override;
base::Value::Dict DictPolicies() const override;
base::TimeDelta OverinstallTimeout() const override;
base::TimeDelta IdleCheckPeriod() const override;
diff --git a/chrome/updater/update_usage_stats_task.h b/chrome/updater/update_usage_stats_task.h
index 845061b1..0ffc409 100644
--- a/chrome/updater/update_usage_stats_task.h
+++ b/chrome/updater/update_usage_stats_task.h
@@ -39,14 +39,16 @@
// Returns true if the updater is allowed to send detailed event logs to an
// external endpoint. Logging is allowed only if the following conditions are
// all met:
-// 1) The updater manages an app (an "event logging permission provider")
+// 1) The updater manages an app (an `event_logging_permission_provider`)
// responsible for granting the updater permission to send remote
// logging events.
-// 2) The event logging permission provider app has usage stats enabled.
+// 2) The `event_logging_permission_provider` app has usage stats enabled.
// 3) The updater manages no other apps. That is, the apps managed by the
// updater are a subset of {Updater, Enterprise Companion App,
-// _event logging permission provider_}.
-bool RemoteEventLoggingAllowed(UpdaterScope scope);
+// `event_logging_permission_provider`}.
+bool RemoteEventLoggingAllowed(
+ UpdaterScope scope,
+ std::optional<std::string> event_logging_permission_provider);
#if BUILDFLAG(IS_MAC)
bool AnyAppEnablesUsageStats(
diff --git a/chrome/updater/update_usage_stats_task_linux.cc b/chrome/updater/update_usage_stats_task_linux.cc
index 954f21d..1391bdad 100644
--- a/chrome/updater/update_usage_stats_task_linux.cc
+++ b/chrome/updater/update_usage_stats_task_linux.cc
@@ -5,6 +5,7 @@
#include "chrome/updater/update_usage_stats_task.h"
#include <memory>
+#include <optional>
#include <utility>
#include "base/functional/bind.h"
@@ -20,7 +21,7 @@
return false;
}
-bool RemoteEventLoggingAllowed(UpdaterScope) {
+bool RemoteEventLoggingAllowed(UpdaterScope, std::optional<std::string>) {
return false;
}
diff --git a/chrome/updater/update_usage_stats_task_mac.mm b/chrome/updater/update_usage_stats_task_mac.mm
index 7b3d902a..a0c96626 100644
--- a/chrome/updater/update_usage_stats_task_mac.mm
+++ b/chrome/updater/update_usage_stats_task_mac.mm
@@ -15,6 +15,7 @@
#include "base/files/file_path.h"
#include "base/files/file_util.h"
#include "chrome/enterprise_companion/installer_paths.h"
+#include "chrome/updater/external_constants.h"
#include "chrome/updater/updater_branding.h"
#include "chrome/updater/updater_scope.h"
#include "chrome/updater/util/mac_util.h"
@@ -102,11 +103,12 @@
GetApplicationSupportDirectoriesForUsers(scope));
}
-bool RemoteEventLoggingAllowed(UpdaterScope scope) {
- // TODO(crbug.com/371595849): Inject the permission provider.
+bool RemoteEventLoggingAllowed(
+ UpdaterScope scope,
+ std::optional<std::string> event_logging_permission_provider) {
return RemoteEventLoggingAllowed(
GetApplicationSupportDirectoriesForUsers(scope),
- /*event_logging_permission_provider=*/std::nullopt);
+ std::move(event_logging_permission_provider));
}
} // namespace updater
diff --git a/chrome/updater/update_usage_stats_task_unittest.cc b/chrome/updater/update_usage_stats_task_unittest.cc
index 2034f10c..54a6e546 100644
--- a/chrome/updater/update_usage_stats_task_unittest.cc
+++ b/chrome/updater/update_usage_stats_task_unittest.cc
@@ -173,7 +173,7 @@
#if BUILDFLAG(IS_LINUX)
TEST_F(UpdateUsageStatsTaskTest, LinuxAlwaysFalse) {
ASSERT_FALSE(AnyAppEnablesUsageStats(scope_));
- ASSERT_FALSE(RemoteEventLoggingAllowed(scope_));
+ ASSERT_FALSE(RemoteEventLoggingAllowed(scope_, fake_permission_provider_));
}
#else
diff --git a/chrome/updater/update_usage_stats_task_win.cc b/chrome/updater/update_usage_stats_task_win.cc
index 667bdac..dd1eea6 100644
--- a/chrome/updater/update_usage_stats_task_win.cc
+++ b/chrome/updater/update_usage_stats_task_win.cc
@@ -17,6 +17,7 @@
#include "base/win/registry.h"
#include "base/win/windows_types.h"
#include "chrome/updater/app/app_utils.h"
+#include "chrome/updater/external_constants.h"
#include "chrome/updater/persisted_data.h"
#include "chrome/updater/updater_scope.h"
#include "chrome/updater/util/win_util.h"
@@ -109,11 +110,12 @@
GetClientStatePathsForScope(scope));
}
-bool RemoteEventLoggingAllowed(UpdaterScope scope) {
- // TODO(crbug.com/371595849): Inject the permission provider.
+bool RemoteEventLoggingAllowed(
+ UpdaterScope scope,
+ std::optional<std::string> event_logging_permission_provider) {
return RemoteEventLoggingAllowed(
UpdaterScopeToHKeyRoot(scope), GetClientStatePathsForScope(scope),
- /*event_logging_permission_provider=*/std::nullopt);
+ std::move(event_logging_permission_provider));
}
} // namespace updater
diff --git a/chrome/updater/updater_branding.h.in b/chrome/updater/updater_branding.h.in
index 109e64fd..fa282e9d 100644
--- a/chrome/updater/updater_branding.h.in
+++ b/chrome/updater/updater_branding.h.in
@@ -5,6 +5,7 @@
// Branding Information
#define BROWSER_NAME_STRING "@BROWSER_NAME@"
#define BROWSER_PRODUCT_NAME_STRING "@BROWSER_PRODUCT_NAME@"
+#define BROWSER_APP_ID "@BROWSER_APP_ID@"
#define COMPANY_SHORTNAME_STRING "@COMPANY_SHORTNAME@"
#define COMPANY_SHORTNAME_LOWERCASE_STRING "@COMPANY_SHORTNAME_LOWERCASE@"
#define COMPANY_SHORTNAME_UPPERCASE_STRING "@COMPANY_SHORTNAME_UPPERCASE@"
