Revert "Moved the chaps database directory under /home/root."
This reverts commit d083d86bc53dbc180b0a44d9e388bd122e090e24
This commit likely caused <https://uberchromegw.corp.google.com/i/chromeos/builders/lumpy%20canary/builds/2797/steps/UnitTest/logs/stdio> and other failures.
Change-Id: I4f0beafc20c410b7e58c43770c040883ab156f43
Reviewed-on: https://gerrit.chromium.org/gerrit/60869
Reviewed-by: Peter Mayo <petermayo@chromium.org>
Commit-Queue: Elly Jones <ellyjones@chromium.org>
Tested-by: Elly Jones <ellyjones@chromium.org>
diff --git a/homedirs.cc b/homedirs.cc
index 8281a2d..a794b99 100644
--- a/homedirs.cc
+++ b/homedirs.cc
@@ -555,16 +555,11 @@
}
namespace {
- const char *kChapsDaemonName = "chaps";
const char *kChapsDirName = ".chaps";
const char *kChapsSaltName = "auth_data_salt";
}
FilePath HomeDirs::GetChapsTokenDir(const std::string& user) const {
- return chromeos::cryptohome::home::GetDaemonPath(user, kChapsDaemonName);
-}
-
-FilePath HomeDirs::GetLegacyChapsTokenDir(const std::string& user) const {
return chromeos::cryptohome::home::GetUserPath(user).Append(kChapsDirName);
}
diff --git a/homedirs.h b/homedirs.h
index 326567b..b4fda82 100644
--- a/homedirs.h
+++ b/homedirs.h
@@ -109,9 +109,6 @@
// Returns the path to the user's chaps token directory.
virtual FilePath GetChapsTokenDir(const std::string& username) const;
- // Returns the path to the user's legacy chaps token directory.
- virtual FilePath GetLegacyChapsTokenDir(const std::string& username) const;
-
// Returns the path to the user's token salt.
virtual FilePath GetChapsTokenSaltPath(const std::string& username) const;
diff --git a/mock_platform.cc b/mock_platform.cc
index defd540..cb2993a 100644
--- a/mock_platform.cc
+++ b/mock_platform.cc
@@ -6,8 +6,7 @@
namespace cryptohome {
-MockPlatform::MockPlatform()
- : mock_enumerator_(new NiceMock<MockFileEnumerator>()) {
+MockPlatform::MockPlatform() : mock_enumerator_(new MockFileEnumerator()) {
ON_CALL(*this, GetOwnership(_, _, _))
.WillByDefault(Invoke(this, &MockPlatform::MockGetOwnership));
ON_CALL(*this, SetOwnership(_, _, _))
diff --git a/mock_platform.h b/mock_platform.h
index 56c580a..48c8618 100644
--- a/mock_platform.h
+++ b/mock_platform.h
@@ -14,7 +14,6 @@
namespace cryptohome {
using ::testing::_;
-using ::testing::NiceMock;
using ::testing::Invoke;
using ::testing::Return;
@@ -135,7 +134,7 @@
MOCK_METHOD0(FirmwareWriteProtected, bool(void));
MOCK_METHOD1(SyncFile, bool(const std::string&));
- MockFileEnumerator* mock_enumerator() { return mock_enumerator_.get(); }
+ MockFileEnumerator* get_mock_enumerator() { return mock_enumerator_.get(); }
private:
bool MockGetOwnership(const std::string& path, uid_t* user_id,
@@ -165,7 +164,7 @@
bool recursive,
int file_type) {
MockFileEnumerator* e = mock_enumerator_.release();
- mock_enumerator_.reset(new NiceMock<MockFileEnumerator>());
+ mock_enumerator_.reset(new MockFileEnumerator());
mock_enumerator_->entries_.assign(e->entries_.begin(), e->entries_.end());
return e;
}
diff --git a/mount.cc b/mount.cc
index 75581d4..b921f8f 100644
--- a/mount.cc
+++ b/mount.cc
@@ -1245,44 +1245,28 @@
}
bool Mount::CheckChapsDirectory(const std::string& dir,
- const std::string& legacy_dir,
bool* permissions_status) {
+ // If the Chaps database directory does not exist, create it.
+ DCHECK(permissions_status);
+ *permissions_status = true;
// 0750: u + rwx, g + rx
const mode_t chaps_permissions = S_IRWXU | S_IRGRP | S_IXGRP;
const mode_t permissions_mask = S_IRWXU | S_IRWXG | S_IRWXO;
-
- DCHECK(permissions_status);
- *permissions_status = true;
- // If the Chaps database directory does not exist, create it.
if (!platform_->DirectoryExists(dir)) {
- if (platform_->DirectoryExists(legacy_dir)) {
- LOG(INFO) << "Moving chaps directory from " << legacy_dir << " to "
- << dir;
- if (!platform_->CopyWithPermissions(legacy_dir, dir)) {
- *permissions_status = false;
- return false;
- }
- if (!platform_->DeleteFile(legacy_dir, true)) {
- PLOG(WARNING) << "Failed to clean up " << legacy_dir;
- *permissions_status = false;
- return false;
- }
- } else {
- if (!platform_->CreateDirectory(dir)) {
- LOG(ERROR) << "Failed to create " << dir;
- *permissions_status = false;
- return false;
- }
- if (!platform_->SetOwnership(dir, chaps_user_, default_access_group_)) {
- LOG(ERROR) << "Couldn't set file ownership for " << dir;
- *permissions_status = false;
- return false;
- }
- if (!platform_->SetPermissions(dir, chaps_permissions)) {
- LOG(ERROR) << "Couldn't set permissions for " << dir;
- *permissions_status = false;
- return false;
- }
+ if (!platform_->CreateDirectory(dir)) {
+ LOG(ERROR) << "Failed to create " << dir;
+ *permissions_status = false;
+ return false;
+ }
+ if (!platform_->SetOwnership(dir, chaps_user_, default_access_group_)) {
+ LOG(ERROR) << "Couldn't set file ownership for " << dir;
+ *permissions_status = false;
+ return false;
+ }
+ if (!platform_->SetPermissions(dir, chaps_permissions)) {
+ LOG(ERROR) << "Couldn't set permissions for " << dir;
+ *permissions_status = false;
+ return false;
}
return true;
}
@@ -1326,11 +1310,8 @@
bool permissions_status = false;
std::string username = current_user_->username();
FilePath token_dir = homedirs_->GetChapsTokenDir(username);
- FilePath legacy_token_dir = homedirs_->GetLegacyChapsTokenDir(username);
- if (!CheckChapsDirectory(token_dir.value(),
- legacy_token_dir.value(),
- &permissions_status))
+ if (!CheckChapsDirectory(token_dir.value(), &permissions_status))
return false;
// We may create a salt file and, if so, we want to restrict access to it.
ScopedUmask scoped_umask(platform_, kDefaultUmask);
diff --git a/mount.h b/mount.h
index 779b010..bd48670 100644
--- a/mount.h
+++ b/mount.h
@@ -507,16 +507,12 @@
// does not exist, it is created and initialzed with the correct
// values. If the directory or its attributes cannot be checked,
// set or created, a fatal error has occured and the function
- // returns false. If the directory does not exist and a legacy directory
- // exists, the legacy directory will be moved to the new location.
+ // returns false.
//
// Parameters
// dir - directory to check
- // legacy_dir - legacy directory location
// permissions_check - set to false if permissions, uid or gid is incorrect
- bool CheckChapsDirectory(const std::string& dir,
- const std::string& legacy_dir,
- bool* permissions_check);
+ bool CheckChapsDirectory(const std::string& dir, bool* permissions_check);
// Ensures that the device policy is loaded.
//
@@ -781,7 +777,6 @@
FRIEND_TEST(MountTest,
CheckChapsDirectoryCalledWithExistingDirWithFatalError);
FRIEND_TEST(MountTest, CheckChapsDirectoryCalledWithExistingDir);
- FRIEND_TEST(MountTest, CheckChapsDirectoryMigration);
DISALLOW_COPY_AND_ASSIGN(Mount);
};
diff --git a/mount_unittest.cc b/mount_unittest.cc
index a7562f5..a45938d 100644
--- a/mount_unittest.cc
+++ b/mount_unittest.cc
@@ -300,8 +300,7 @@
SetArgumentPointee<2>(shared_gid_),
Return(true)));
bool permissions_status = false;
- EXPECT_TRUE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
+ EXPECT_TRUE(mount->CheckChapsDirectory("/fake", &permissions_status));
EXPECT_TRUE(permissions_status);
}
@@ -328,8 +327,7 @@
EXPECT_CALL(platform, GetPermissions("/fake", _))
.WillRepeatedly(DoAll(SetArgumentPointee<1>(bad_perms), Return(true)));
bool permissions_status = false;
- EXPECT_TRUE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
+ EXPECT_TRUE(mount->CheckChapsDirectory("/fake", &permissions_status));
EXPECT_FALSE(permissions_status);
}
@@ -364,8 +362,7 @@
Return(true)));
bool permissions_status = false;
- EXPECT_TRUE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
+ EXPECT_TRUE(mount->CheckChapsDirectory("/fake", &permissions_status));
EXPECT_FALSE(permissions_status);
}
@@ -396,8 +393,7 @@
SetArgumentPointee<2>(bad_gid),
Return(true)));
bool permissions_status = false;
- EXPECT_TRUE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
+ EXPECT_TRUE(mount->CheckChapsDirectory("/fake", &permissions_status));
EXPECT_FALSE(permissions_status);
}
@@ -421,8 +417,6 @@
EXPECT_TRUE(mount->Init());
EXPECT_CALL(platform, DirectoryExists("/fake"))
.WillRepeatedly(Return(false));
- EXPECT_CALL(platform, DirectoryExists("/fake_legacy"))
- .WillRepeatedly(Return(false));
EXPECT_CALL(platform, CreateDirectory("/fake"))
.WillOnce(Return(false))
.WillRepeatedly(Return(true));
@@ -434,14 +428,11 @@
EXPECT_CALL(platform, GetPermissions("/fake", _))
.WillRepeatedly(Return(false));
bool permissions_status = false;
- EXPECT_FALSE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
+ EXPECT_FALSE(mount->CheckChapsDirectory("/fake", &permissions_status));
EXPECT_FALSE(permissions_status);
- EXPECT_FALSE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
+ EXPECT_FALSE(mount->CheckChapsDirectory("/fake", &permissions_status));
EXPECT_FALSE(permissions_status);
- EXPECT_FALSE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
+ EXPECT_FALSE(mount->CheckChapsDirectory("/fake", &permissions_status));
EXPECT_FALSE(permissions_status);
}
@@ -471,74 +462,12 @@
EXPECT_CALL(platform, GetOwnership("/fake", _, _))
.WillRepeatedly(Return(false));
bool permissions_status = false;
- EXPECT_FALSE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
+ EXPECT_FALSE(mount->CheckChapsDirectory("/fake", &permissions_status));
EXPECT_FALSE(permissions_status);
- EXPECT_FALSE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
+ EXPECT_FALSE(mount->CheckChapsDirectory("/fake", &permissions_status));
EXPECT_FALSE(permissions_status);
}
-TEST_F(MountTest, CheckChapsDirectoryMigration) {
- scoped_refptr<Mount> mount = new Mount();
- NiceMock<MockPlatform> platform;
- NiceMock<MockTpm> tpm;
- mount->crypto()->set_tpm(&tpm);
- mount->crypto()->set_platform(&platform);
- mount->set_use_tpm(false);
- mount->set_platform(&platform);
-
- // Configure stub methods.
- EXPECT_CALL(platform, Copy(_, _))
- .WillRepeatedly(Return(true));
- EXPECT_CALL(platform, DeleteFile(_, _))
- .WillRepeatedly(Return(true));
-
- // Stubs which will trigger the migration code path.
- EXPECT_CALL(platform, DirectoryExists("/fake"))
- .WillRepeatedly(Return(false));
- EXPECT_CALL(platform, DirectoryExists("/fake_legacy"))
- .WillRepeatedly(Return(true));
-
- // Configure stat for the base directory.
- struct stat base_stat = {0};
- base_stat.st_mode = 040123;
- base_stat.st_uid = 1;
- base_stat.st_gid = 2;
- EXPECT_CALL(platform, Stat(_, _))
- .WillRepeatedly(DoAll(SetArgumentPointee<1>(base_stat), Return(true)));
-
- // Configure a fake enumerator.
- MockFileEnumerator* enumerator = platform.mock_enumerator();
- enumerator->entries_.push_back("/fake_legacy/test_file1");
- enumerator->entries_.push_back("test_file2");
- FileEnumerator::FindInfo find_info1 = {{0}, "/fake_legacy/test_file1"};
- find_info1.stat.st_mode = 0555;
- find_info1.stat.st_uid = 3;
- find_info1.stat.st_gid = 4;
- FileEnumerator::FindInfo find_info2 = {{0}, "test_file2"};
- find_info2.stat.st_mode = 0777;
- find_info2.stat.st_uid = 5;
- find_info2.stat.st_gid = 6;
- EXPECT_CALL(*enumerator, GetFindInfo(_))
- .WillOnce(SetArgumentPointee<0>(find_info1))
- .WillOnce(SetArgumentPointee<0>(find_info2));
-
- // These expectations will ensure the ownership and permissions are being
- // correctly applied after the directory has been moved.
- EXPECT_CALL(platform, SetOwnership("/fake/test_file1", 3, 4)).Times(1);
- EXPECT_CALL(platform, SetPermissions("/fake/test_file1", 0555)).Times(1);
- EXPECT_CALL(platform, SetOwnership("/fake/test_file2", 5, 6)).Times(1);
- EXPECT_CALL(platform, SetPermissions("/fake/test_file2", 0777)).Times(1);
- EXPECT_CALL(platform, SetOwnership("/fake", 1, 2)).Times(1);
- EXPECT_CALL(platform, SetPermissions("/fake", 0123)).Times(1);
-
- bool permissions_status = false;
- EXPECT_TRUE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
- EXPECT_TRUE(permissions_status);
-}
-
TEST_F(MountTest, CreateCryptohomeTest) {
InsertTestUsers(&kDefaultUsers[5], 1);
// creates a cryptohome and tests credentials
diff --git a/platform.cc b/platform.cc
index 95f9b55..9b43439 100644
--- a/platform.cc
+++ b/platform.cc
@@ -43,27 +43,6 @@
using base::SplitString;
using std::string;
-namespace {
-
-class ScopedPath {
- public:
- ScopedPath(cryptohome::Platform* platform, const string& dir)
- : platform_(platform), dir_(dir) {}
- ~ScopedPath() {
- if (!dir_.empty() && !platform_->DeleteFile(dir_, true)) {
- PLOG(WARNING) << "Failed to clean up " << dir_;
- }
- }
- void release() {
- dir_.clear();
- }
- private:
- cryptohome::Platform* platform_;
- string dir_;
-};
-
-} // namespace
-
namespace cryptohome {
const int kDefaultMountOptions = MS_NOEXEC | MS_NOSUID | MS_NODEV;
@@ -565,71 +544,6 @@
return file_util::CopyDirectory(from_path, to_path, true);
}
-bool Platform::CopyWithPermissions(const std::string& from_path,
- const std::string& to_path) {
- // Save stat for every file.
- std::vector<FileEnumerator::FindInfo> entry_list;
- FileEnumerator::FindInfo base_entry_info;
- base_entry_info.filename = from_path;
- if (!Stat(from_path, &base_entry_info.stat)) {
- PLOG(ERROR) << "Failed to stat " << from_path;
- return false;
- }
- entry_list.push_back(base_entry_info);
- if (FileEnumerator::IsDirectory(base_entry_info)) {
- int file_types = FileEnumerator::FILES | FileEnumerator::DIRECTORIES;
- scoped_ptr<FileEnumerator> file_enumerator(
- GetFileEnumerator(from_path, true, file_types));
- string entry_path;
- while (!(entry_path = file_enumerator->Next()).empty()) {
- FileEnumerator::FindInfo entry_info;
- file_enumerator->GetFindInfo(&entry_info);
- // Keep the full path.
- entry_info.filename = entry_path;
- entry_list.push_back(entry_info);
- }
- }
-
- if (!Copy(from_path, to_path)) {
- PLOG(ERROR) << "Failed to copy " << from_path;
- return false;
- }
-
- // If something goes wrong we want to blow away the half-baked path.
- ScopedPath scoped_new_path(this, to_path);
-
- // Unfortunately, ownership and permissions are not always retained.
- // Apply the old ownership / permissions on a per-file basis.
- const FilePath new_base(to_path);
- const FilePath old_base(from_path);
- for (size_t i = 0; i < entry_list.size(); ++i) {
- FilePath old_path(entry_list[i].filename);
- FilePath new_path(new_base);
- if (old_path != old_base) {
- if (old_path.IsAbsolute()) {
- DCHECK(old_base.AppendRelativePath(old_path, &new_path));
- } else {
- new_path = new_base.Append(old_path);
- }
- }
- if (!SetOwnership(new_path.value(),
- entry_list[i].stat.st_uid,
- entry_list[i].stat.st_gid)) {
- PLOG(ERROR) << "Failed to set ownership for " << new_path.value();
- return false;
- }
- const mode_t permissions_mask = 07777;
- if (!SetPermissions(new_path.value(),
- entry_list[i].stat.st_mode & permissions_mask)) {
- PLOG(ERROR) << "Failed to set permissions for " << new_path.value();
- return false;
- }
- }
- // The copy is done, keep the new path.
- scoped_new_path.release();
- return true;
-}
-
bool Platform::StatVFS(const std::string& path, struct statvfs* vfs) {
return statvfs(path.c_str(), vfs) == 0;
}
diff --git a/platform.h b/platform.h
index 36f987e..3e317bd 100644
--- a/platform.h
+++ b/platform.h
@@ -295,10 +295,6 @@
// Copies from to to.
virtual bool Copy(const std::string& from, const std::string& to);
- // Copies and retains permissions and ownership.
- virtual bool CopyWithPermissions(const std::string& from,
- const std::string& to);
-
// Moves a given path on the filesystem
//
// Parameters
