Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(4954)

Unified Diff: chrome/browser/chromeos/policy/active_directory_policy_manager.cc

Issue 2954293002: Chromad: Prevent session from starting without policy (Closed)
Patch Set: Address nits Created 3 years, 5 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: chrome/browser/chromeos/policy/active_directory_policy_manager.cc
diff --git a/chrome/browser/chromeos/policy/active_directory_policy_manager.cc b/chrome/browser/chromeos/policy/active_directory_policy_manager.cc
index 195140d9e972a2b2efc033267aa4707a4a941935..90776fa27b46c6bc67ded2245de8a5942152e3c4 100644
--- a/chrome/browser/chromeos/policy/active_directory_policy_manager.cc
+++ b/chrome/browser/chromeos/policy/active_directory_policy_manager.cc
@@ -31,17 +31,24 @@ ActiveDirectoryPolicyManager::~ActiveDirectoryPolicyManager() {}
std::unique_ptr<ActiveDirectoryPolicyManager>
ActiveDirectoryPolicyManager::CreateForDevicePolicy(
std::unique_ptr<CloudPolicyStore> store) {
- return base::WrapUnique(
- new ActiveDirectoryPolicyManager(EmptyAccountId(), std::move(store)));
+ // Can't use MakeUnique<> because the constructor is private.
+ return base::WrapUnique(new ActiveDirectoryPolicyManager(
+ EmptyAccountId(), false, base::TimeDelta(), base::OnceClosure(),
+ std::move(store)));
}
// static
std::unique_ptr<ActiveDirectoryPolicyManager>
ActiveDirectoryPolicyManager::CreateForUserPolicy(
const AccountId& account_id,
+ bool wait_for_policy_fetch,
+ base::TimeDelta initial_policy_fetch_timeout,
+ base::OnceClosure exit_session,
std::unique_ptr<CloudPolicyStore> store) {
- return base::WrapUnique(
- new ActiveDirectoryPolicyManager(account_id, std::move(store)));
+ // Can't use MakeUnique<> because the constructor is private.
+ return base::WrapUnique(new ActiveDirectoryPolicyManager(
+ account_id, wait_for_policy_fetch, initial_policy_fetch_timeout,
+ std::move(exit_session), std::move(store)));
}
void ActiveDirectoryPolicyManager::Init(SchemaRegistry* registry) {
@@ -56,7 +63,7 @@ void ActiveDirectoryPolicyManager::Init(SchemaRegistry* registry) {
PublishPolicy();
scheduler_ = base::MakeUnique<PolicyScheduler>(
- base::BindRepeating(&ActiveDirectoryPolicyManager::DoFetch,
+ base::BindRepeating(&ActiveDirectoryPolicyManager::DoPolicyFetch,
weak_ptr_factory_.GetWeakPtr()),
base::BindRepeating(&ActiveDirectoryPolicyManager::OnPolicyFetched,
weak_ptr_factory_.GetWeakPtr()),
@@ -70,6 +77,9 @@ void ActiveDirectoryPolicyManager::Shutdown() {
bool ActiveDirectoryPolicyManager::IsInitializationComplete(
PolicyDomain domain) const {
+ if (waiting_for_initial_policy_fetch_) {
+ return false;
+ }
if (domain == POLICY_DOMAIN_CHROME) {
return store_->is_initialized();
}
@@ -84,6 +94,12 @@ void ActiveDirectoryPolicyManager::OnStoreLoaded(
CloudPolicyStore* cloud_policy_store) {
DCHECK_EQ(store_.get(), cloud_policy_store);
PublishPolicy();
+ if (fetch_ever_completed_) {
+ // Policy is guaranteed to be up to date with the previous fetch result
+ // because OnPolicyFetched() cancels any potentially running Load()
+ // operations.
+ CancelWaitForInitialPolicy(fetch_ever_succeeded_ /* success */);
+ }
}
void ActiveDirectoryPolicyManager::OnStoreError(
@@ -93,12 +109,40 @@ void ActiveDirectoryPolicyManager::OnStoreError(
// complete on the ConfigurationPolicyProvider interface. Technically, this is
// only required on the first load, but doesn't hurt in any case.
PublishPolicy();
+ if (fetch_ever_completed_) {
+ CancelWaitForInitialPolicy(false /* success */);
+ }
+}
+
+void ActiveDirectoryPolicyManager::ForceTimeoutForTest() {
+ DCHECK(initial_policy_timeout_.IsRunning());
+ // Stop the timer to mimic what happens when a real timer fires, then invoke
+ // the timer callback directly.
+ initial_policy_timeout_.Stop();
+ OnBlockingFetchTimeout();
}
ActiveDirectoryPolicyManager::ActiveDirectoryPolicyManager(
const AccountId& account_id,
+ bool wait_for_policy_fetch,
+ base::TimeDelta initial_policy_fetch_timeout,
+ base::OnceClosure exit_session,
std::unique_ptr<CloudPolicyStore> store)
- : account_id_(account_id), store_(std::move(store)) {}
+ : account_id_(account_id),
+ waiting_for_initial_policy_fetch_(wait_for_policy_fetch),
+ exit_session_(std::move(exit_session)),
+ store_(std::move(store)) {
+ // Delaying initialization complete is intended for user policy only.
+ DCHECK(!(account_id == EmptyAccountId() && wait_for_policy_fetch));
+ DCHECK_NE(wait_for_policy_fetch, initial_policy_fetch_timeout.is_zero());
stevenjb 2017/07/17 16:07:37 nit: Do we need |wait_for_policy_fetch| as a param
Thiemo Nagel 2017/07/18 13:02:18 I like that very much. Thanks!
+ initial_policy_fetch_may_fail_ = !initial_policy_fetch_timeout.is_max();
+ if (wait_for_policy_fetch && !initial_policy_fetch_timeout.is_max()) {
+ initial_policy_timeout_.Start(
+ FROM_HERE, initial_policy_fetch_timeout,
+ base::Bind(&ActiveDirectoryPolicyManager::OnBlockingFetchTimeout,
+ weak_ptr_factory_.GetWeakPtr()));
+ }
+}
void ActiveDirectoryPolicyManager::PublishPolicy() {
if (!store_->is_initialized()) {
@@ -117,7 +161,7 @@ void ActiveDirectoryPolicyManager::PublishPolicy() {
UpdatePolicy(std::move(bundle));
}
-void ActiveDirectoryPolicyManager::DoFetch(
+void ActiveDirectoryPolicyManager::DoPolicyFetch(
base::OnceCallback<void(bool success)> callback) {
chromeos::DBusThreadManager* thread_manager =
chromeos::DBusThreadManager::Get();
@@ -133,12 +177,65 @@ void ActiveDirectoryPolicyManager::DoFetch(
}
void ActiveDirectoryPolicyManager::OnPolicyFetched(bool success) {
- if (!success) {
+ fetch_ever_completed_ = true;
+ if (success) {
+ fetch_ever_succeeded_ = true;
+ } else {
LOG(ERROR) << "Active Directory policy fetch failed.";
+ if (store_->is_initialized()) {
+ CancelWaitForInitialPolicy(false /* success */);
+ }
}
- // Load independently of success or failure to keep up to date with whatever
- // has happened on the authpolicyd / session manager side.
+ // Load independently of success or failure to keep in sync with the state in
+ // session manager. This cancels any potentially running Load() operations
+ // thus it is guaranteed that at the next OnStoreLoaded() invocation the
+ // policy is up-to-date with what was fetched.
store_->Load();
}
+void ActiveDirectoryPolicyManager::OnBlockingFetchTimeout() {
+ DCHECK(waiting_for_initial_policy_fetch_);
+ LOG(WARNING) << "Timed out while waiting for the policy fetch. "
+ << "The session will start with the cached policy.";
+ CancelWaitForInitialPolicy(false);
+}
+
+void ActiveDirectoryPolicyManager::CancelWaitForInitialPolicy(bool success) {
+ if (!waiting_for_initial_policy_fetch_)
+ return;
+
+ initial_policy_timeout_.Stop();
+
+ // If the conditions to continue profile initialization are not met, the user
+ // session is exited and initialization is not set as completed.
+ // TODO(tnagel): Maybe add code to retry policy fetch?
+ if (!store_->has_policy()) {
+ // If there's no policy at all (not even cached) the user session must not
+ // continue.
+ LOG(ERROR) << "Policy could not be obtained. "
+ << "Aborting profile initialization";
+ // Prevent duplicate exit session calls.
+ if (exit_session_) {
+ std::move(exit_session_).Run();
+ }
+ return;
+ }
+ if (!success && !initial_policy_fetch_may_fail_) {
+ LOG(ERROR) << "Policy fetch failed for the user. "
+ << "Aborting profile initialization";
+ // Prevent duplicate exit session calls.
+ if (exit_session_) {
+ std::move(exit_session_).Run();
+ }
+ return;
+ }
+
+ // Set initialization complete.
+ waiting_for_initial_policy_fetch_ = false;
+
+ // Publish policy (even though it hasn't changed) in order to signal load
+ // complete on the ConfigurationPolicyProvider interface.
+ PublishPolicy();
+}
+
} // namespace policy

Powered by Google App Engine
This is Rietveld 408576698