Also use DevicePolicy::GetAuP2PEnabled() to determine if p2p is enabled
With this change update_engine will use p2p either if the user has
manually enabled it (through the crosh flag) OR if the enterprise has
enabled it in Enterprise Policy.
BUG=chromium:260442
TEST=New unit tests + unit tests pass.
Change-Id: I54cd92c481bd2fd7c90232d7137ce2b37fa2ce61
Reviewed-on: https://chromium-review.googlesource.com/168950
Reviewed-by: Chris Sosa <sosa@chromium.org>
Commit-Queue: David Zeuthen <zeuthen@chromium.org>
Tested-by: David Zeuthen <zeuthen@chromium.org>
diff --git a/fake_p2p_manager.h b/fake_p2p_manager.h
index 87788bf..aebc238 100644
--- a/fake_p2p_manager.h
+++ b/fake_p2p_manager.h
@@ -22,6 +22,8 @@
virtual ~FakeP2PManager() {}
// P2PManager overrides.
+ virtual void SetDevicePolicy(const policy::DevicePolicy* device_policy) {}
+
virtual bool IsP2PEnabled() {
return is_p2p_enabled_;
}
diff --git a/mock_p2p_manager.h b/mock_p2p_manager.h
index ff633f8..4d91765 100644
--- a/mock_p2p_manager.h
+++ b/mock_p2p_manager.h
@@ -16,6 +16,9 @@
public:
MockP2PManager() {
// Delegate all calls to the fake instance
+ ON_CALL(*this, SetDevicePolicy(testing::_))
+ .WillByDefault(testing::Invoke(&fake_,
+ &FakeP2PManager::SetDevicePolicy));
ON_CALL(*this, IsP2PEnabled())
.WillByDefault(testing::Invoke(&fake_,
&FakeP2PManager::IsP2PEnabled));
@@ -58,6 +61,7 @@
virtual ~MockP2PManager() {}
// P2PManager overrides.
+ MOCK_METHOD1(SetDevicePolicy, void(const policy::DevicePolicy*));
MOCK_METHOD0(IsP2PEnabled, bool());
MOCK_METHOD0(EnsureP2PRunning, bool());
MOCK_METHOD0(EnsureP2PNotRunning, bool());
diff --git a/p2p_manager.cc b/p2p_manager.cc
index e56f739..7536fc6 100644
--- a/p2p_manager.cc
+++ b/p2p_manager.cc
@@ -97,7 +97,7 @@
const int num_files_to_keep);
// P2PManager methods.
- virtual void SetConfiguration(Configuration *configuration);
+ virtual void SetDevicePolicy(const policy::DevicePolicy* device_policy);
virtual bool IsP2PEnabled();
virtual bool EnsureP2PRunning();
virtual bool EnsureP2PNotRunning();
@@ -134,6 +134,9 @@
// Utility function used by EnsureP2PRunning() and EnsureP2PNotRunning().
bool EnsureP2P(bool should_be_running);
+ // The device policy being used or NULL if no policy is being used.
+ const policy::DevicePolicy* device_policy_;
+
// Configuration object.
scoped_ptr<Configuration> configuration_;
@@ -166,33 +169,51 @@
PrefsInterface *prefs,
const string& file_extension,
const int num_files_to_keep)
- : prefs_(prefs),
+ : device_policy_(NULL),
+ prefs_(prefs),
file_extension_(file_extension),
num_files_to_keep_(num_files_to_keep) {
configuration_.reset(configuration != NULL ? configuration :
new ConfigurationImpl());
}
-void P2PManagerImpl::SetConfiguration(Configuration *configuration) {
- configuration_.reset(configuration);
+void P2PManagerImpl::SetDevicePolicy(
+ const policy::DevicePolicy* device_policy) {
+ device_policy_ = device_policy;
}
bool P2PManagerImpl::IsP2PEnabled() {
- // TODO(deymo,zeuthen)(chromium:260441): See the bug for the bigger
- // picture. For now we just read the state variable so in order for
- // p2p to work the developer will have to manually create the prefs
- // file. Once the fix for bug 260441 has been merged, this can be
- // toggled using the crosh command, as intended.
- bool enabled = false;
- if (prefs_ == NULL) {
- LOG(INFO) << "No prefs object.";
- } else if (!prefs_->Exists(kPrefsP2PEnabled)) {
- LOG(INFO) << "The " << kPrefsP2PEnabled << " pref does not exist.";
- } else if (!prefs_->GetBoolean(kPrefsP2PEnabled, &enabled)) {
- LOG(ERROR) << "Error getting " << kPrefsP2PEnabled << " pref.";
+ bool p2p_enabled = false;
+
+ // The logic we want here is additive, e.g. p2p can be enabled by
+ // either the crosh flag OR by Enterprise Policy, e.g. the following
+ // truth table:
+ //
+ // crosh_flag == FALSE && enterprise_policy == FALSE -> use_p2p == FALSE
+ // crosh_flag == FALSE && enterprise_policy == TRUE -> use_p2p == TRUE
+ // crosh_flag == TRUE && enterprise_policy == FALSE -> use_p2p == TRUE
+ // crosh_flag == TRUE && enterprise_policy == TRUE -> use_p2p == TRUE
+
+ if (device_policy_ != NULL) {
+ if (device_policy_->GetAuP2PEnabled(&p2p_enabled)) {
+ if (p2p_enabled) {
+ LOG(INFO) << "Enterprise Policy indicates that p2p is enabled.";
+ return true;
+ }
+ }
}
- LOG(INFO) << "Returning value " << enabled << " for whether p2p is enabled.";
- return enabled;
+
+ if (prefs_ != NULL &&
+ prefs_->Exists(kPrefsP2PEnabled) &&
+ prefs_->GetBoolean(kPrefsP2PEnabled, &p2p_enabled) &&
+ p2p_enabled) {
+ LOG(INFO) << "The crosh flag indicates that p2p is enabled.";
+ return true;
+ }
+
+ LOG(INFO) << "Neither Enterprise Policy nor crosh flag indicates that p2p "
+ << "is enabled.";
+ return false;
}
bool P2PManagerImpl::EnsureP2P(bool should_be_running) {
diff --git a/p2p_manager.h b/p2p_manager.h
index a38f648..e525746 100644
--- a/p2p_manager.h
+++ b/p2p_manager.h
@@ -12,6 +12,8 @@
#include <base/file_path.h>
#include <base/memory/ref_counted.h>
#include <base/time.h>
+#include <policy/device_policy.h>
+#include <policy/libpolicy.h>
#include "update_engine/prefs_interface.h"
@@ -46,6 +48,10 @@
// If the lookup failed, |url| is empty.
typedef base::Callback<void(const std::string& url)> LookupCallback;
+ // Use the device policy specified by |device_policy|. If this is
+ // NULL, then no device policy is used.
+ virtual void SetDevicePolicy(const policy::DevicePolicy* device_policy) = 0;
+
// Returns true if - and only if - P2P should be used on this
// device. This value is derived from a variety of sources including
// enterprise policy.
diff --git a/p2p_manager_unittest.cc b/p2p_manager_unittest.cc
index f9d428c..6bbe856 100644
--- a/p2p_manager_unittest.cc
+++ b/p2p_manager_unittest.cc
@@ -16,6 +16,8 @@
#include "base/bind.h"
#include "base/callback.h"
#include "base/stringprintf.h"
+#include <policy/libpolicy.h>
+#include <policy/mock_device_policy.h>
#include "update_engine/p2p_manager.h"
#include "update_engine/fake_p2p_manager_configuration.h"
@@ -48,9 +50,9 @@
};
-// Check that result of IsP2PEnabled() correspond to the
-// kPrefsP2PEnabled state variable.
-TEST_F(P2PManagerTest, P2PEnabled) {
+// Check that IsP2PEnabled() returns false if neither the crosh flag
+// nor the Enterprise Policy indicates p2p is to be used.
+TEST_F(P2PManagerTest, P2PEnabledNeitherCroshFlagNotEnterpriseSetting) {
string temp_dir;
Prefs prefs;
EXPECT_TRUE(utils::MakeTempDirectory("/tmp/PayloadStateP2PTests.XXXXXX",
@@ -60,6 +62,75 @@
scoped_ptr<P2PManager> manager(P2PManager::Construct(test_conf_,
&prefs, "cros_au", 3));
EXPECT_FALSE(manager->IsP2PEnabled());
+
+ EXPECT_TRUE(utils::RecursiveUnlinkDir(temp_dir));
+}
+
+// Check that IsP2PEnabled() corresponds to value of the crosh flag
+// when Enterprise Policy is not set.
+TEST_F(P2PManagerTest, P2PEnabledCroshFlag) {
+ string temp_dir;
+ Prefs prefs;
+ EXPECT_TRUE(utils::MakeTempDirectory("/tmp/PayloadStateP2PTests.XXXXXX",
+ &temp_dir));
+ prefs.Init(FilePath(temp_dir));
+
+ scoped_ptr<P2PManager> manager(P2PManager::Construct(test_conf_,
+ &prefs, "cros_au", 3));
+ EXPECT_FALSE(manager->IsP2PEnabled());
+ prefs.SetBoolean(kPrefsP2PEnabled, true);
+ EXPECT_TRUE(manager->IsP2PEnabled());
+ prefs.SetBoolean(kPrefsP2PEnabled, false);
+ EXPECT_FALSE(manager->IsP2PEnabled());
+
+ EXPECT_TRUE(utils::RecursiveUnlinkDir(temp_dir));
+}
+
+// Check that IsP2PEnabled() always returns true if Enterprise Policy
+// indicates that p2p is to be used.
+TEST_F(P2PManagerTest, P2PEnabledEnterpriseSettingTrue) {
+ string temp_dir;
+ Prefs prefs;
+ EXPECT_TRUE(utils::MakeTempDirectory("/tmp/PayloadStateP2PTests.XXXXXX",
+ &temp_dir));
+ prefs.Init(FilePath(temp_dir));
+
+ scoped_ptr<P2PManager> manager(P2PManager::Construct(test_conf_,
+ &prefs, "cros_au", 3));
+ scoped_ptr<policy::MockDevicePolicy> device_policy(
+ new policy::MockDevicePolicy());
+ EXPECT_CALL(*device_policy, GetAuP2PEnabled(testing::_)).WillRepeatedly(
+ DoAll(testing::SetArgumentPointee<0>(bool(true)),
+ testing::Return(true)));
+ manager->SetDevicePolicy(device_policy.get());
+ EXPECT_TRUE(manager->IsP2PEnabled());
+
+ // Should still return true even if crosh flag says otherwise.
+ prefs.SetBoolean(kPrefsP2PEnabled, false);
+ EXPECT_TRUE(manager->IsP2PEnabled());
+
+ EXPECT_TRUE(utils::RecursiveUnlinkDir(temp_dir));
+}
+
+// Check that IsP2PEnabled() corrresponds to the value of the crosh
+// flag if Enterprise Policy indicates that p2p is not to be used.
+TEST_F(P2PManagerTest, P2PEnabledEnterpriseSettingFalse) {
+ string temp_dir;
+ Prefs prefs;
+ EXPECT_TRUE(utils::MakeTempDirectory("/tmp/PayloadStateP2PTests.XXXXXX",
+ &temp_dir));
+ prefs.Init(FilePath(temp_dir));
+
+ scoped_ptr<P2PManager> manager(P2PManager::Construct(test_conf_,
+ &prefs, "cros_au", 3));
+ scoped_ptr<policy::MockDevicePolicy> device_policy(
+ new policy::MockDevicePolicy());
+ EXPECT_CALL(*device_policy, GetAuP2PEnabled(testing::_)).WillRepeatedly(
+ DoAll(testing::SetArgumentPointee<0>(bool(false)),
+ testing::Return(true)));
+ manager->SetDevicePolicy(device_policy.get());
+ EXPECT_FALSE(manager->IsP2PEnabled());
+
prefs.SetBoolean(kPrefsP2PEnabled, true);
EXPECT_TRUE(manager->IsP2PEnabled());
prefs.SetBoolean(kPrefsP2PEnabled, false);
diff --git a/update_attempter.cc b/update_attempter.cc
index ea1c619..ae9be74 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -217,6 +217,7 @@
LOG(INFO) << "No device policies/settings present.";
system_state_->set_device_policy(device_policy);
+ system_state_->p2p_manager()->SetDevicePolicy(device_policy);
}
void UpdateAttempter::CalculateP2PParams(bool interactive) {