shill: Allow creation and push of DefaultProfile
Add the ability to create a DefaultProfile whose name is not
"default". Add the ability to push a DefaultProfile atop
another DefaultProfile in the Manager. This will enable
autotests to run that can test interactions between profiles
while a user is not logged in.
BUG=chromium-os:24461
TEST=New unit tests; autotests to come
Change-Id: Iccfcff6ec613cf0a8ac55c11c9cc1efac8237807
Reviewed-on: https://gerrit.chromium.org/gerrit/19287
Reviewed-by: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
Commit-Ready: Paul Stewart <pstew@chromium.org>
diff --git a/default_profile.cc b/default_profile.cc
index e6f693d..a3c8be8 100644
--- a/default_profile.cc
+++ b/default_profile.cc
@@ -17,6 +17,7 @@
#include "shill/portal_detector.h"
#include "shill/store_interface.h"
+using std::string;
using std::vector;
namespace shill {
@@ -41,9 +42,11 @@
DefaultProfile::DefaultProfile(ControlInterface *control,
Manager *manager,
const FilePath &storage_path,
+ const string &profile_id,
const Manager::Properties &manager_props)
- : Profile(control, manager, Identifier(kDefaultId), "", true),
+ : Profile(control, manager, Identifier(profile_id), "", true),
storage_path_(storage_path),
+ profile_id_(profile_id),
props_(manager_props) {
PropertyStore *store = this->mutable_store();
store->RegisterConstString(flimflam::kCheckPortalListProperty,
@@ -108,7 +111,8 @@
}
bool DefaultProfile::GetStoragePath(FilePath *path) {
- *path = storage_path_.Append(base::StringPrintf("%s.profile", kDefaultId));
+ *path = storage_path_.Append(base::StringPrintf("%s.profile",
+ profile_id_.c_str()));
return true;
}
diff --git a/default_profile.h b/default_profile.h
index 9c5bfb6..18f2f57 100644
--- a/default_profile.h
+++ b/default_profile.h
@@ -24,9 +24,12 @@
class DefaultProfile : public Profile {
public:
+ static const char kDefaultId[];
+
DefaultProfile(ControlInterface *control,
Manager *manager,
const FilePath &storage_path,
+ const std::string &profile_id,
const Manager::Properties &manager_props);
virtual ~DefaultProfile();
@@ -53,7 +56,6 @@
FRIEND_TEST(DefaultProfileTest, LoadManagerProperties);
FRIEND_TEST(DefaultProfileTest, Save);
- static const char kDefaultId[];
static const char kStorageId[];
static const char kStorageCheckPortalList[];
static const char kStorageHostName[];
@@ -63,6 +65,7 @@
static const char kStoragePortalURL[];
const FilePath storage_path_;
+ const std::string profile_id_;
const Manager::Properties &props_;
DISALLOW_COPY_AND_ASSIGN(DefaultProfile);
diff --git a/default_profile_unittest.cc b/default_profile_unittest.cc
index c49beb4..301dfd4 100644
--- a/default_profile_unittest.cc
+++ b/default_profile_unittest.cc
@@ -38,6 +38,7 @@
: profile_(new DefaultProfile(control_interface(),
manager(),
FilePath(storage_path()),
+ DefaultProfile::kDefaultId,
properties_)),
device_(new MockDevice(control_interface(),
dispatcher(),
diff --git a/manager.cc b/manager.cc
index d9fc96c..2520f66 100644
--- a/manager.cc
+++ b/manager.cc
@@ -160,6 +160,9 @@
// Persist profile, device, service information to disk.
vector<ProfileRefPtr>::iterator it;
for (it = profiles_.begin(); it != profiles_.end(); ++it) {
+ // Since this happens in a loop, the current manager state is stored to
+ // all default profiles in the stack. This is acceptable because the
+ // only time multiple default profiles are loaded are during autotests.
(*it)->Save();
}
@@ -188,6 +191,7 @@
default_profile(new DefaultProfile(control_interface_,
this,
storage_path_,
+ DefaultProfile::kDefaultId,
props_));
CHECK(default_profile->InitStorage(glib_, Profile::kCreateOrOpenExisting,
NULL));
@@ -207,11 +211,22 @@
"Invalid profile name " + name);
return;
}
- ProfileRefPtr profile(new Profile(control_interface_,
- this,
- ident,
- user_storage_format_,
- connect_profiles_to_rpc_));
+
+ ProfileRefPtr profile;
+ if (ident.user.empty()) {
+ profile = new DefaultProfile(control_interface_,
+ this,
+ storage_path_,
+ ident.identifier,
+ props_);
+ } else {
+ profile = new Profile(control_interface_,
+ this,
+ ident,
+ user_storage_format_,
+ connect_profiles_to_rpc_);
+ }
+
if (!profile->InitStorage(glib_, Profile::kCreateNew, error)) {
// |error| will have been populated by InitStorage().
return;
@@ -245,29 +260,46 @@
}
}
+ ProfileRefPtr profile;
if (ident.user.empty()) {
- // The manager will have only one machine-wide profile, and this is the
- // DefaultProfile. This means no other profiles can be loaded that do
- // not have a user component.
- // TODO(pstew): This is all well and good, but WiFi autotests try to
- // creating a default profile (by a name other than "default") in order
- // to avoid leaving permanent side effects to devices under test. This
- // whole thing will need to be reworked in order to allow that to happen,
- // or the autotests (or their expectations) will need to change.
- // crosbug.com/24461
- Error::PopulateAndLog(error, Error::kInvalidArguments,
- "Cannot load non-default global profile " + name);
- return;
- }
+ // Allow a machine-wide-profile to be pushed on the stack only if the
+ // profile stack is empty, or if the topmost profile on the stack is
+ // also a machine-wide (non-user) profile.
+ if (!profiles_.empty() && !profiles_.back()->GetUser().empty()) {
+ Error::PopulateAndLog(error, Error::kInvalidArguments,
+ "Cannot load non-default global profile " + name +
+ " on top of a user profile");
+ return;
+ }
- ProfileRefPtr profile(new Profile(control_interface_,
- this,
- ident,
- user_storage_format_,
- connect_profiles_to_rpc_));
- if (!profile->InitStorage(glib_, Profile::kOpenExisting, error)) {
- // |error| will have been populated by InitStorage().
- return;
+ scoped_refptr<DefaultProfile>
+ default_profile(new DefaultProfile(control_interface_,
+ this,
+ storage_path_,
+ ident.identifier,
+ props_));
+ if (!default_profile->InitStorage(glib_, Profile::kOpenExisting, error)) {
+ // |error| will have been populated by InitStorage().
+ return;
+ }
+
+ if (!default_profile->LoadManagerProperties(&props_)) {
+ Error::PopulateAndLog(error, Error::kInvalidArguments,
+ "Could not load Manager properties from profile " +
+ name);
+ return;
+ }
+ profile = default_profile;
+ } else {
+ profile = new Profile(control_interface_,
+ this,
+ ident,
+ user_storage_format_,
+ connect_profiles_to_rpc_);
+ if (!profile->InitStorage(glib_, Profile::kOpenExisting, error)) {
+ // |error| will have been populated by InitStorage().
+ return;
+ }
}
profiles_.push_back(profile);
diff --git a/manager_unittest.cc b/manager_unittest.cc
index afc2fc8..44ecbf8 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -575,11 +575,12 @@
// Invalid name should be rejected.
EXPECT_EQ(Error::kInvalidArguments, TestCreateProfile(&manager, ""));
- // Valid name is still rejected because we can't create a profile
- // that doesn't have a user component. Such profile names are
- // reserved for the single DefaultProfile the manager creates
- // at startup.
- EXPECT_EQ(Error::kInvalidArguments, TestCreateProfile(&manager, "valid"));
+ // A profile with invalid characters in it should similarly be rejected.
+ EXPECT_EQ(Error::kInvalidArguments,
+ TestCreateProfile(&manager, "valid_profile"));
+
+ // We should be able to create a machine profile.
+ EXPECT_EQ(Error::kSuccess, TestCreateProfile(&manager, "valid"));
// We should succeed in creating a valid user profile.
const char kProfile[] = "~user/profile";
@@ -606,8 +607,8 @@
// Pushing an invalid profile should fail.
EXPECT_EQ(Error::kInvalidArguments, TestPushProfile(&manager, ""));
- // Pushing a default profile name should fail.
- EXPECT_EQ(Error::kInvalidArguments, TestPushProfile(&manager, "default"));
+ // Pushing a default profile that does not exist should fail.
+ EXPECT_EQ(Error::kNotFound, TestPushProfile(&manager, "default"));
const char kProfile0[] = "~user/profile0";
const char kProfile1[] = "~user/profile1";
@@ -693,6 +694,26 @@
// Next pop should fail with "stack is empty".
EXPECT_EQ(Error::kNotFound, TestPopAnyProfile(&manager));
+
+ const char kMachineProfile0[] = "machineprofile0";
+ const char kMachineProfile1[] = "machineprofile1";
+ ASSERT_EQ(Error::kSuccess, TestCreateProfile(&manager, kMachineProfile0));
+ ASSERT_EQ(Error::kSuccess, TestCreateProfile(&manager, kMachineProfile1));
+
+ // Should be able to push a machine profile.
+ EXPECT_EQ(Error::kSuccess, TestPushProfile(&manager, kMachineProfile0));
+
+ // Should be able to push a user profile atop a machine profile.
+ EXPECT_EQ(Error::kSuccess, TestPushProfile(&manager, kProfile0));
+
+ // Pushing a system-wide profile on top of a user profile should fail.
+ EXPECT_EQ(Error::kInvalidArguments,
+ TestPushProfile(&manager, kMachineProfile1));
+
+ // However if we pop the user profile, we should be able stack another
+ // machine profile on.
+ EXPECT_EQ(Error::kSuccess, TestPopAnyProfile(&manager));
+ EXPECT_EQ(Error::kSuccess, TestPushProfile(&manager, kMachineProfile1));
}
// Use this matcher instead of passing RefPtrs directly into the arguments
diff --git a/profile.h b/profile.h
index 5aee3bc..08fa179 100644
--- a/profile.h
+++ b/profile.h
@@ -117,6 +117,9 @@
// Returns whether |name| matches this Profile's |name_|.
virtual bool MatchesIdentifier(const Identifier &name) const;
+ // Returns the username component of the profile identifier.
+ const std::string &GetUser() const { return name_.user; }
+
// Returns a read-only copy of the backing storage of the profile.
// TODO(pstew): Needed by ProfileDBusPropertyExporter crosbug.com/25813
const StoreInterface *GetConstStorage() const { return storage_.get(); }