[Origin] Wait for all policy managers before refreshing profile policies (#36193)
* [Origin] Wait for all policy managers before refreshing profile policies `BraveProfilePolicyProvider` observes both `BraveOriginPolicyManager` and `AdBlockOnlyModePolicyManager`. Each fires `OnBravePoliciesReady` independently, so the first notification could call `RefreshPolicies` before the other manager's `Init()` had run -- producing a bundle that reflected only one source and flipping `IsFirstPolicyLoadComplete` true on the empty load. Introduce `BravePolicyManagerBase` with self-registration and a static `AllInitialized()`. `OnBravePoliciesReady` early-returns until every registered manager reports ready; `policies_ready_` is gated on the same check so `SetProfileID` can't trigger a partial refresh either. Future managers inheriting from the base are picked up automatically. Resolves: https://github.com/brave/brave-browser/issues/55276
This commit is contained in:
@@ -68,6 +68,7 @@ static_library("brave_origin") {
|
||||
]
|
||||
deps = [
|
||||
"//brave/brave_domains",
|
||||
"//brave/components/brave_policy:brave_policy_manager_registry",
|
||||
"//brave/components/brave_policy:brave_policy_observer",
|
||||
"//brave/components/skus/browser",
|
||||
"//brave/components/skus/common",
|
||||
|
||||
@@ -6,6 +6,7 @@
|
||||
#include "brave/components/brave_origin/brave_origin_policy_manager.h"
|
||||
|
||||
#include "base/containers/map_util.h"
|
||||
#include "base/functional/bind.h"
|
||||
#include "base/logging.h"
|
||||
#include "base/no_destructor.h"
|
||||
#include "brave/components/brave_origin/brave_origin_utils.h"
|
||||
@@ -218,7 +219,10 @@ bool BraveOriginPolicyManager::GetPolicyValueInternal(
|
||||
return default_value;
|
||||
}
|
||||
|
||||
BraveOriginPolicyManager::BraveOriginPolicyManager() = default;
|
||||
BraveOriginPolicyManager::BraveOriginPolicyManager()
|
||||
: registration_(
|
||||
base::BindRepeating(&BraveOriginPolicyManager::IsInitialized,
|
||||
base::Unretained(this))) {}
|
||||
|
||||
BraveOriginPolicyManager::~BraveOriginPolicyManager() = default;
|
||||
|
||||
|
||||
@@ -12,6 +12,7 @@
|
||||
#include "base/memory/raw_ptr.h"
|
||||
#include "base/observer_list.h"
|
||||
#include "brave/components/brave_origin/brave_origin_policy_info.h"
|
||||
#include "brave/components/brave_policy/brave_policy_manager_registry.h"
|
||||
#include "brave/components/brave_policy/brave_policy_observer.h"
|
||||
|
||||
class PrefService;
|
||||
@@ -102,6 +103,11 @@ class BraveOriginPolicyManager {
|
||||
BraveOriginPolicyMap profile_policy_definitions_;
|
||||
raw_ptr<PrefService> local_state_ = nullptr;
|
||||
base::ObserverList<brave_policy::BravePolicyObserver> observers_;
|
||||
|
||||
// Participates in `BravePolicyManagerRegistry::AllInitialized()`. Declared
|
||||
// last so the readiness callback isn't invoked against partially
|
||||
// constructed state (`initialized_` above must already exist).
|
||||
brave_policy::BravePolicyManagerRegistration registration_;
|
||||
};
|
||||
|
||||
} // namespace brave_origin
|
||||
|
||||
@@ -13,6 +13,7 @@ static_library("brave_policy") {
|
||||
]
|
||||
|
||||
deps = [
|
||||
":brave_policy_manager_registry",
|
||||
":brave_policy_observer",
|
||||
"//base",
|
||||
"//brave/components/brave_origin",
|
||||
@@ -23,6 +24,15 @@ static_library("brave_policy") {
|
||||
]
|
||||
}
|
||||
|
||||
source_set("brave_policy_manager_registry") {
|
||||
sources = [
|
||||
"brave_policy_manager_registry.cc",
|
||||
"brave_policy_manager_registry.h",
|
||||
]
|
||||
|
||||
public_deps = [ "//base" ]
|
||||
}
|
||||
|
||||
source_set("brave_policy_observer") {
|
||||
sources = [ "brave_policy_observer.h" ]
|
||||
|
||||
@@ -34,14 +44,19 @@ source_set("unit_tests") {
|
||||
sources = [
|
||||
"brave_browser_policy_provider_unittest.cc",
|
||||
"brave_policy_conversions_unittest.cc",
|
||||
"brave_policy_manager_registry_unittest.cc",
|
||||
"brave_policy_source_unittest.cc",
|
||||
"brave_profile_policy_provider_unittest.cc",
|
||||
]
|
||||
|
||||
deps = [
|
||||
":brave_policy",
|
||||
":brave_policy_manager_registry",
|
||||
"//base",
|
||||
"//base/test:test_support",
|
||||
"//brave/components/brave_origin",
|
||||
"//brave/components/brave_policy/ad_block_only_mode",
|
||||
"//brave/components/brave_shields/core/common",
|
||||
"//brave/components/resources:strings",
|
||||
"//components/enterprise",
|
||||
"//components/policy/core/browser",
|
||||
|
||||
@@ -17,6 +17,7 @@ static_library("ad_block_only_mode") {
|
||||
|
||||
deps = [
|
||||
"//base",
|
||||
"//brave/components/brave_policy:brave_policy_manager_registry",
|
||||
"//brave/components/brave_policy:brave_policy_observer",
|
||||
"//brave/components/brave_shields/core/common",
|
||||
"//components/content_settings/core/common",
|
||||
|
||||
@@ -6,6 +6,7 @@
|
||||
#include "brave/components/brave_policy/ad_block_only_mode/ad_block_only_mode_policy_manager.h"
|
||||
|
||||
#include "base/feature_list.h"
|
||||
#include "base/functional/bind.h"
|
||||
#include "base/no_destructor.h"
|
||||
#include "brave/components/brave_policy/ad_block_only_mode/buildflags/buildflags.h"
|
||||
#include "brave/components/brave_shields/core/common/features.h"
|
||||
@@ -43,6 +44,10 @@ void AdBlockOnlyModePolicyManager::Shutdown() {
|
||||
local_state_ = nullptr;
|
||||
}
|
||||
|
||||
bool AdBlockOnlyModePolicyManager::IsInitialized() const {
|
||||
return local_state_ != nullptr;
|
||||
}
|
||||
|
||||
void AdBlockOnlyModePolicyManager::AddObserver(BravePolicyObserver* observer) {
|
||||
observers_.AddObserver(observer);
|
||||
|
||||
@@ -71,7 +76,10 @@ AdBlockOnlyModePolicies AdBlockOnlyModePolicyManager::GetPolicies() const {
|
||||
return GetPoliciesImpl();
|
||||
}
|
||||
|
||||
AdBlockOnlyModePolicyManager::AdBlockOnlyModePolicyManager() = default;
|
||||
AdBlockOnlyModePolicyManager::AdBlockOnlyModePolicyManager()
|
||||
: registration_(
|
||||
base::BindRepeating(&AdBlockOnlyModePolicyManager::IsInitialized,
|
||||
base::Unretained(this))) {}
|
||||
|
||||
AdBlockOnlyModePolicyManager::~AdBlockOnlyModePolicyManager() = default;
|
||||
|
||||
|
||||
@@ -10,6 +10,7 @@
|
||||
#include "base/memory/raw_ptr.h"
|
||||
#include "base/observer_list.h"
|
||||
#include "base/values.h"
|
||||
#include "brave/components/brave_policy/brave_policy_manager_registry.h"
|
||||
#include "brave/components/brave_policy/brave_policy_observer.h"
|
||||
#include "components/prefs/pref_change_registrar.h"
|
||||
|
||||
@@ -36,6 +37,8 @@ class AdBlockOnlyModePolicyManager final {
|
||||
void Init(PrefService* local_state);
|
||||
void Shutdown();
|
||||
|
||||
bool IsInitialized() const;
|
||||
|
||||
void AddObserver(BravePolicyObserver* observer);
|
||||
void RemoveObserver(BravePolicyObserver* observer);
|
||||
|
||||
@@ -56,6 +59,11 @@ class AdBlockOnlyModePolicyManager final {
|
||||
raw_ptr<PrefService> local_state_; // Not owned.
|
||||
PrefChangeRegistrar pref_change_registrar_;
|
||||
base::ObserverList<BravePolicyObserver> observers_;
|
||||
|
||||
// Participates in `BravePolicyManagerRegistry::AllInitialized()`. Declared
|
||||
// last so the readiness callback isn't invoked against partially
|
||||
// constructed state (`local_state_` above must already exist).
|
||||
BravePolicyManagerRegistration registration_;
|
||||
};
|
||||
|
||||
} // namespace brave_policy
|
||||
|
||||
@@ -0,0 +1,57 @@
|
||||
/* Copyright (c) 2026 The Brave Authors. All rights reserved.
|
||||
* This Source Code Form is subject to the terms of the Mozilla Public
|
||||
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
|
||||
* You can obtain one at https://mozilla.org/MPL/2.0/. */
|
||||
|
||||
#include "brave/components/brave_policy/brave_policy_manager_registry.h"
|
||||
|
||||
#include <algorithm>
|
||||
#include <utility>
|
||||
#include <vector>
|
||||
|
||||
#include "base/memory/raw_ptr.h"
|
||||
#include "base/no_destructor.h"
|
||||
|
||||
namespace brave_policy {
|
||||
|
||||
namespace {
|
||||
|
||||
// Function-local static to avoid static-init-order issues with policy
|
||||
// managers that may register during static init via `base::NoDestructor`
|
||||
// singletons.
|
||||
std::vector<raw_ptr<BravePolicyManagerRegistration>>& Registry() {
|
||||
static base::NoDestructor<
|
||||
std::vector<raw_ptr<BravePolicyManagerRegistration>>>
|
||||
registry;
|
||||
return *registry;
|
||||
}
|
||||
|
||||
} // namespace
|
||||
|
||||
// static
|
||||
bool BravePolicyManagerRegistry::AllInitialized() {
|
||||
return std::ranges::all_of(Registry(), [](const auto& registration) {
|
||||
return registration->IsInitialized();
|
||||
});
|
||||
}
|
||||
|
||||
// static
|
||||
size_t BravePolicyManagerRegistry::SizeForTesting() {
|
||||
return Registry().size();
|
||||
}
|
||||
|
||||
BravePolicyManagerRegistration::BravePolicyManagerRegistration(
|
||||
IsInitializedCallback is_initialized)
|
||||
: is_initialized_(std::move(is_initialized)) {
|
||||
Registry().push_back(this);
|
||||
}
|
||||
|
||||
BravePolicyManagerRegistration::~BravePolicyManagerRegistration() {
|
||||
std::erase(Registry(), this);
|
||||
}
|
||||
|
||||
bool BravePolicyManagerRegistration::IsInitialized() const {
|
||||
return is_initialized_.Run();
|
||||
}
|
||||
|
||||
} // namespace brave_policy
|
||||
@@ -0,0 +1,65 @@
|
||||
/* Copyright (c) 2026 The Brave Authors. All rights reserved.
|
||||
* This Source Code Form is subject to the terms of the Mozilla Public
|
||||
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
|
||||
* You can obtain one at https://mozilla.org/MPL/2.0/. */
|
||||
|
||||
#ifndef BRAVE_COMPONENTS_BRAVE_POLICY_BRAVE_POLICY_MANAGER_REGISTRY_H_
|
||||
#define BRAVE_COMPONENTS_BRAVE_POLICY_BRAVE_POLICY_MANAGER_REGISTRY_H_
|
||||
|
||||
#include <cstddef>
|
||||
|
||||
#include "base/functional/callback.h"
|
||||
|
||||
namespace brave_policy {
|
||||
|
||||
// Process-wide registry of Brave-side policy managers. Used by callers
|
||||
// (e.g. `BraveProfilePolicyProvider`) that need to wait for every
|
||||
// participating manager to report initialized before refreshing policies.
|
||||
//
|
||||
// Managers participate by holding a `BravePolicyManagerRegistration`
|
||||
// member that captures their `IsInitialized()` predicate. Registration
|
||||
// happens automatically in the registration's constructor and
|
||||
// unregistration in its destructor.
|
||||
//
|
||||
// Single-threaded by design: registration happens during early
|
||||
// browser-process startup on the UI thread, before worker threads access
|
||||
// these managers.
|
||||
class BravePolicyManagerRegistry {
|
||||
public:
|
||||
// Returns true once every registered manager's `IsInitialized()`
|
||||
// callback returns true.
|
||||
static bool AllInitialized();
|
||||
|
||||
// Returns the number of registered managers. For tests only.
|
||||
static size_t SizeForTesting();
|
||||
};
|
||||
|
||||
// Registers itself with `BravePolicyManagerRegistry` on construction and
|
||||
// unregisters on destruction. Construct as a member of any policy-manager
|
||||
// class to participate in `AllInitialized()`.
|
||||
//
|
||||
// Member ordering matters: declare this *after* anything the
|
||||
// `IsInitialized()` callback reads from, so the predicate isn't invoked
|
||||
// against a partially constructed object during a future enclosing-class
|
||||
// refactor.
|
||||
class BravePolicyManagerRegistration {
|
||||
public:
|
||||
using IsInitializedCallback = base::RepeatingCallback<bool()>;
|
||||
|
||||
explicit BravePolicyManagerRegistration(IsInitializedCallback is_initialized);
|
||||
~BravePolicyManagerRegistration();
|
||||
|
||||
BravePolicyManagerRegistration(const BravePolicyManagerRegistration&) =
|
||||
delete;
|
||||
BravePolicyManagerRegistration& operator=(
|
||||
const BravePolicyManagerRegistration&) = delete;
|
||||
|
||||
bool IsInitialized() const;
|
||||
|
||||
private:
|
||||
const IsInitializedCallback is_initialized_;
|
||||
};
|
||||
|
||||
} // namespace brave_policy
|
||||
|
||||
#endif // BRAVE_COMPONENTS_BRAVE_POLICY_BRAVE_POLICY_MANAGER_REGISTRY_H_
|
||||
@@ -0,0 +1,82 @@
|
||||
/* Copyright (c) 2026 The Brave Authors. All rights reserved.
|
||||
* This Source Code Form is subject to the terms of the Mozilla Public
|
||||
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
|
||||
* You can obtain one at https://mozilla.org/MPL/2.0/. */
|
||||
|
||||
#include "brave/components/brave_policy/brave_policy_manager_registry.h"
|
||||
|
||||
#include "base/functional/bind.h"
|
||||
#include "testing/gtest/include/gtest/gtest.h"
|
||||
|
||||
namespace brave_policy {
|
||||
|
||||
namespace {
|
||||
|
||||
// Minimal policy-manager-like host used to exercise the registry without
|
||||
// depending on real singletons. Owns a registration whose callback reports
|
||||
// the host's initialized flag.
|
||||
class FakePolicyManager {
|
||||
public:
|
||||
FakePolicyManager()
|
||||
: initialized_(false),
|
||||
registration_(base::BindRepeating(&FakePolicyManager::IsInitialized,
|
||||
base::Unretained(this))) {}
|
||||
|
||||
bool IsInitialized() const { return initialized_; }
|
||||
void SetInitialized(bool initialized) { initialized_ = initialized; }
|
||||
|
||||
private:
|
||||
// `initialized_` must be declared before `registration_` so it is
|
||||
// initialized first; the registration captures a callback that reads
|
||||
// `initialized_`.
|
||||
bool initialized_;
|
||||
BravePolicyManagerRegistration registration_;
|
||||
};
|
||||
|
||||
} // namespace
|
||||
|
||||
// Construction adds an entry to the registry; destruction removes it.
|
||||
TEST(BravePolicyManagerRegistryTest,
|
||||
ConstructionRegistersAndDestructionUnregisters) {
|
||||
const size_t baseline = BravePolicyManagerRegistry::SizeForTesting();
|
||||
{
|
||||
FakePolicyManager fake;
|
||||
EXPECT_EQ(baseline + 1, BravePolicyManagerRegistry::SizeForTesting());
|
||||
}
|
||||
EXPECT_EQ(baseline, BravePolicyManagerRegistry::SizeForTesting());
|
||||
}
|
||||
|
||||
// `AllInitialized` returns false as long as any registered manager
|
||||
// reports not initialized.
|
||||
TEST(BravePolicyManagerRegistryTest,
|
||||
AllInitializedFalseWithUninitializedManager) {
|
||||
FakePolicyManager fake;
|
||||
EXPECT_FALSE(BravePolicyManagerRegistry::AllInitialized());
|
||||
}
|
||||
|
||||
// Even with an initialized manager present, an uninitialized one
|
||||
// elsewhere forces `AllInitialized()` false.
|
||||
TEST(BravePolicyManagerRegistryTest, AllInitializedFalseWhenAnyUninitialized) {
|
||||
FakePolicyManager fake_initialized;
|
||||
fake_initialized.SetInitialized(true);
|
||||
|
||||
FakePolicyManager fake_uninitialized;
|
||||
EXPECT_FALSE(BravePolicyManagerRegistry::AllInitialized());
|
||||
}
|
||||
|
||||
// After an uninitialized manager is destroyed, it no longer holds
|
||||
// `AllInitialized()` false: the result reverts to the baseline observed
|
||||
// before it existed.
|
||||
TEST(BravePolicyManagerRegistryTest,
|
||||
DestroyedManagerNoLongerAffectsAllInitialized) {
|
||||
FakePolicyManager fake_outer;
|
||||
fake_outer.SetInitialized(true);
|
||||
const bool baseline = BravePolicyManagerRegistry::AllInitialized();
|
||||
{
|
||||
FakePolicyManager fake_inner; // uninitialized
|
||||
EXPECT_FALSE(BravePolicyManagerRegistry::AllInitialized());
|
||||
}
|
||||
EXPECT_EQ(baseline, BravePolicyManagerRegistry::AllInitialized());
|
||||
}
|
||||
|
||||
} // namespace brave_policy
|
||||
@@ -11,6 +11,7 @@
|
||||
#include "base/values.h"
|
||||
#include "brave/components/brave_origin/brave_origin_utils.h"
|
||||
#include "brave/components/brave_policy/ad_block_only_mode/ad_block_only_mode_policy_manager.h"
|
||||
#include "brave/components/brave_policy/brave_policy_manager_registry.h"
|
||||
#include "components/policy/core/common/policy_bundle.h"
|
||||
#include "components/policy/core/common/policy_map.h"
|
||||
#include "components/policy/core/common/policy_namespace.h"
|
||||
@@ -85,6 +86,20 @@ bool BraveProfilePolicyProvider::IsFirstPolicyLoadComplete(
|
||||
}
|
||||
|
||||
void BraveProfilePolicyProvider::OnBravePoliciesReady() {
|
||||
// Wait until every registered policy manager reports `IsInitialized()`
|
||||
// before treating policies as ready. Each manager fires this callback
|
||||
// independently from its own `AddObserver`; acting on the first one
|
||||
// would push a partial bundle (e.g. AdBlockOnlyMode policies present
|
||||
// but BraveOrigin policies missing because `IsBraveOriginPurchased()`
|
||||
// short-circuits to false on an uninitialized manager), which then
|
||||
// propagates through `PolicyServiceImpl` and flips
|
||||
// `IsFirstPolicyLoadComplete` on the empty load. Likewise,
|
||||
// `policies_ready_` is gated on full readiness so a `SetProfileID`
|
||||
// call in this window doesn't trigger a partial refresh.
|
||||
if (!BravePolicyManagerRegistry::AllInitialized()) {
|
||||
return;
|
||||
}
|
||||
|
||||
policies_ready_ = true;
|
||||
|
||||
// Once we have Brave policies and a profile ID trigger Refresh policies
|
||||
|
||||
@@ -9,11 +9,17 @@
|
||||
|
||||
#include "base/files/file_path.h"
|
||||
#include "base/test/task_environment.h"
|
||||
#include "brave/components/brave_origin/brave_origin_policy_info.h"
|
||||
#include "brave/components/brave_origin/brave_origin_policy_manager.h"
|
||||
#include "brave/components/brave_origin/pref_names.h"
|
||||
#include "brave/components/brave_policy/ad_block_only_mode/ad_block_only_mode_policy_manager.h"
|
||||
#include "brave/components/brave_shields/core/common/pref_names.h"
|
||||
#include "components/policy/core/common/configuration_policy_provider.h"
|
||||
#include "components/policy/core/common/policy_bundle.h"
|
||||
#include "components/policy/core/common/policy_namespace.h"
|
||||
#include "components/policy/core/common/policy_types.h"
|
||||
#include "components/policy/core/common/schema_registry.h"
|
||||
#include "components/prefs/pref_registry_simple.h"
|
||||
#include "components/prefs/testing_pref_service.h"
|
||||
#include "testing/gtest/include/gtest/gtest.h"
|
||||
|
||||
@@ -24,14 +30,36 @@ class BraveProfilePolicyProviderTest : public ::testing::Test {
|
||||
BraveProfilePolicyProviderTest() = default;
|
||||
~BraveProfilePolicyProviderTest() override = default;
|
||||
|
||||
void SetUp() override {
|
||||
// Register prefs needed by both managers so their `Init` calls succeed
|
||||
// and they report `IsInitialized() == true`. Without this the
|
||||
// `BravePolicyManagerRegistry::AllInitialized()` gate inside
|
||||
// `BraveProfilePolicyProvider::OnBravePoliciesReady` would early-return
|
||||
// and the provider would never call `RefreshPolicies`.
|
||||
pref_service_.registry()->RegisterDictionaryPref(
|
||||
brave_origin::kBraveOriginPolicies);
|
||||
pref_service_.registry()->RegisterBooleanPref(
|
||||
brave_origin::kOriginPurchaseValidated, false);
|
||||
pref_service_.registry()->RegisterBooleanPref(
|
||||
brave_shields::prefs::kAdBlockOnlyModeEnabled, false);
|
||||
|
||||
brave_origin::BraveOriginPolicyManager::GetInstance()->Init(
|
||||
brave_origin::BraveOriginPolicyMap(),
|
||||
brave_origin::BraveOriginPolicyMap(), &pref_service_);
|
||||
AdBlockOnlyModePolicyManager::GetInstance()->Init(&pref_service_);
|
||||
}
|
||||
|
||||
void TearDown() override {
|
||||
if (provider_.IsInitializationComplete(policy::POLICY_DOMAIN_CHROME)) {
|
||||
provider_.Shutdown();
|
||||
}
|
||||
AdBlockOnlyModePolicyManager::GetInstance()->Shutdown();
|
||||
brave_origin::BraveOriginPolicyManager::GetInstance()->Shutdown();
|
||||
}
|
||||
|
||||
protected:
|
||||
base::test::TaskEnvironment task_environment_;
|
||||
TestingPrefServiceSimple pref_service_;
|
||||
policy::SchemaRegistry schema_registry_;
|
||||
BraveProfilePolicyProvider provider_;
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user