Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2890843002:
Policy implementation for encryptfs to ext4 migration strategy (Closed)
DescriptionPolicy implementation for encryptfs to ext4 migration strategy
Chrome OS encrypts each user’s data with the user’s password. With M60,
Chrome OS is switching from ecryptfs to ext4 encryption. ARC N requires
ext4 encryption. The policy DeviceEcryptfsMigrationStrategy states
whether the device is allowed to use ARC or not. If it's not allowed,
the process of data migration should not be launched.
Design doc: https://docs.google.com/document/d/18Nz2OClU6CDb7TpZ2MbO4yySvgiOYWsFUxrR4LQYzXU/edit?ts=5915e354#
This implementation is planned to become obsolete, and be rolled back
after the migration process will finish. The issue for that:
http://crbug.com/725493
BUG=722371
TEST=Manual tests and unit tests.
Review-Url: https://codereview.chromium.org/2890843002
Cr-Commit-Position: refs/heads/master@{#481499}
Committed: https://chromium.googlesource.com/chromium/src/+/24be4caf84c7c14e68c4c10206c3ea0a506c0ee6
Patch Set 1 : Implementation #
Total comments: 25
Patch Set 2 : Fixed review comments #
Total comments: 4
Patch Set 3 : Fixed review comments #
Total comments: 10
Patch Set 4 : Fixed review comments #Patch Set 5 : Changed flag value #
Total comments: 11
Patch Set 6 : Fixed review comments #
Total comments: 47
Patch Set 7 : Fixed review comments #
Total comments: 15
Patch Set 8 : Fixed review comments #
Total comments: 2
Patch Set 9 : Fixed review comments #Patch Set 10 : Rebase done #Patch Set 11 : Fixed the test #
Total comments: 2
Patch Set 12 : Merge done #Messages
Total messages: 80 (54 generated)
Description was changed from ========== Policy implementation for encryptfs to ext4 migration strategy Chrome OS encrypts each user’s data with the user’s password. With M60, Chrome OS is switching from ecryptfs to ext4 encryption. ARC N requires ext4 encryption. The policy DeviceEcryptfsMigrationStrategy states whether the device is allowed to use ARC or not. If it's not allowed, the process of data migration should not be launched. This is the implementation for the policy. BUG=722371 ========== to ========== Policy implementation for encryptfs to ext4 migration strategy Chrome OS encrypts each user’s data with the user’s password. With M60, Chrome OS is switching from ecryptfs to ext4 encryption. ARC N requires ext4 encryption. The policy DeviceEcryptfsMigrationStrategy states whether the device is allowed to use ARC or not. If it's not allowed, the process of data migration should not be launched. Design doc: https://docs.google.com/document/d/18Nz2OClU6CDb7TpZ2MbO4yySvgiOYWsFUxrR4LQYz... This implementation is planned to become obsolete, and be rolled back after the migration process will finish. The issue for that: crbug.com/725493 BUG=722371 ==========
Description was changed from ========== Policy implementation for encryptfs to ext4 migration strategy Chrome OS encrypts each user’s data with the user’s password. With M60, Chrome OS is switching from ecryptfs to ext4 encryption. ARC N requires ext4 encryption. The policy DeviceEcryptfsMigrationStrategy states whether the device is allowed to use ARC or not. If it's not allowed, the process of data migration should not be launched. Design doc: https://docs.google.com/document/d/18Nz2OClU6CDb7TpZ2MbO4yySvgiOYWsFUxrR4LQYz... This implementation is planned to become obsolete, and be rolled back after the migration process will finish. The issue for that: crbug.com/725493 BUG=722371 ========== to ========== Policy implementation for encryptfs to ext4 migration strategy Chrome OS encrypts each user’s data with the user’s password. With M60, Chrome OS is switching from ecryptfs to ext4 encryption. ARC N requires ext4 encryption. The policy DeviceEcryptfsMigrationStrategy states whether the device is allowed to use ARC or not. If it's not allowed, the process of data migration should not be launched. Design doc: https://docs.google.com/document/d/18Nz2OClU6CDb7TpZ2MbO4yySvgiOYWsFUxrR4LQYz... This implementation is planned to become obsolete, and be rolled back after the migration process will finish. The issue for that: crbug.com/725493 BUG=722371 ==========
Description was changed from ========== Policy implementation for encryptfs to ext4 migration strategy Chrome OS encrypts each user’s data with the user’s password. With M60, Chrome OS is switching from ecryptfs to ext4 encryption. ARC N requires ext4 encryption. The policy DeviceEcryptfsMigrationStrategy states whether the device is allowed to use ARC or not. If it's not allowed, the process of data migration should not be launched. Design doc: https://docs.google.com/document/d/18Nz2OClU6CDb7TpZ2MbO4yySvgiOYWsFUxrR4LQYz... This implementation is planned to become obsolete, and be rolled back after the migration process will finish. The issue for that: crbug.com/725493 BUG=722371 ========== to ========== Policy implementation for encryptfs to ext4 migration strategy Chrome OS encrypts each user’s data with the user’s password. With M60, Chrome OS is switching from ecryptfs to ext4 encryption. ARC N requires ext4 encryption. The policy DeviceEcryptfsMigrationStrategy states whether the device is allowed to use ARC or not. If it's not allowed, the process of data migration should not be launched. Design doc: https://docs.google.com/document/d/18Nz2OClU6CDb7TpZ2MbO4yySvgiOYWsFUxrR4LQYz... This implementation is planned to become obsolete, and be rolled back after the migration process will finish. The issue for that: http://crbug.com/725493 BUG=722371 ==========
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
igorcov@chromium.org changed reviewers: + bartfab@chromium.org, hidehiko@chromium.org
bartfab@chromium.org: Please review changes in policy related files. hidehiko@chromium.org: Please review changes in ARC related files. Thank you,
https://codereview.chromium.org/2890843002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2890843002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:640: arc::GetArcAvailabilityPolicyStatus() != Setting kArcDataRemoveRequested pref needs to be guarded. Otherwise, at sometime (e.g. on restarting), the data could be removed. https://codereview.chromium.org/2890843002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/startup_utils.cc (right): https://codereview.chromium.org/2890843002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/startup_utils.cc:235: void StartupUtils::SetExt4MigrationForArcAllowed() { Calling this function in various places sounds not a good for maintenance. When exactly this can be called? Could you call exact once in the initialization? Note that; my understanding is the value could be fixed at very beginning of chrome. If not, please let me know. https://codereview.chromium.org/2890843002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/startup_utils.cc:250: const enterprise_management::ChromeDeviceSettingsProto* policy = nit: auto help to shorten the code. Something like. const auto* device_settings = chromeos::DeviceSettingsService::Get()->device_settings(); if (!policy || !policy->has_device_ecryptfs_migration_strategy()) { // Set some value. return; } const auto& container = policy->device_ecryptfs_migration_strategy(); bool is_available = container.has_migration_strategy() && container.migration_strategy() == ALLOW_MIGRATION; arc::SetArcAvailablilityPolicyStatus( is_available ? AVAILABLE : DISABLED); https://codereview.chromium.org/2890843002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/startup_utils.cc:265: } What value should be assigned in case of no proto? https://codereview.chromium.org/2890843002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/startup_utils.h (right): https://codereview.chromium.org/2890843002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/startup_utils.h:79: static void SetExt4MigrationForArcAllowed(); Which thread this can be called? Only UI thread? Please comment. https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... File components/arc/arc_util.cc (right): https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... components/arc/arc_util.cc:34: ARC_AVAILABILITY_POLICY_STATUS arc_availability_policy_status = UNKNOWN; Please move this into anonymous namespace. sytle: g_ prefix please. https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... components/arc/arc_util.cc:37: const auto* command_line = base::CommandLine::ForCurrentProcess(); Please DCHECK arc_availability_policy_status is not UNKNOWN. Also, could you at least manually check the code by switching the check to CHECK temporarily for both enterprise policy applied case and regular case? https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... components/arc/arc_util.cc:47: // In the case of supported with migration, the ARC availability is defined Please add detailed unit tests for new behavior. https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... components/arc/arc_util.cc:52: if (arc_availability_policy_status == AVAILABLE) nit: return arc_availability_policy_status == AVAILABLE; https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... components/arc/arc_util.cc:72: arc_availability_policy_status = status; To ensure this is called exact once, DCHECK_EQ(UNKNOWN, arc_availability_policy_status); please. https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_util.h File components/arc/arc_util.h (right): https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... components/arc/arc_util.h:23: "officially-supported-with-migration"; As commented in design doc, could you introduce another flag, rather than adding new value to the support status. I heard, what you need is whether the device was *originally* launched with ecryptfs, regardless of the current file system. So, it sounds independent from ARC's support status. https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... components/arc/arc_util.h:29: enum ARC_AVAILABILITY_POLICY_STATUS { nit: please use enum class. style: Please use CamelCase. ArcAvailabilityPolicyStatus. https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... components/arc/arc_util.h:42: extern ARC_AVAILABILITY_POLICY_STATUS arc_availability_policy_status; Please remove. extern decl is not needed. https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... components/arc/arc_util.h:96: void SetArcAvailabilityPolicyStatus(ARC_AVAILABILITY_POLICY_STATUS status); Please explicitly document by-when this must be called.
Patchset #2 (id:160001) has been deleted
Patchset #2 (id:180001) has been deleted
Patchset #2 (id:190001) has been deleted
Patchset #2 (id:210001) has been deleted
Patchset #2 (id:230001) has been deleted
Patchset #2 (id:250001) has been deleted
Patchset #2 (id:270001) has been deleted
Patchset #2 (id:290001) has been deleted
Patchset #2 (id:310001) has been deleted
Thank you, hidehiko@ for your comments. Those were valuable and I've changed the code to address them. Please take a look again. https://codereview.chromium.org/2890843002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2890843002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:640: arc::GetArcAvailabilityPolicyStatus() != On 2017/05/25 12:41:55, hidehiko wrote: > Setting kArcDataRemoveRequested pref needs to be guarded. > Otherwise, at sometime (e.g. on restarting), the data could be removed. Done. https://codereview.chromium.org/2890843002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/startup_utils.cc (right): https://codereview.chromium.org/2890843002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/startup_utils.cc:235: void StartupUtils::SetExt4MigrationForArcAllowed() { On 2017/05/25 12:41:55, hidehiko wrote: > Calling this function in various places sounds not a good for maintenance. > When exactly this can be called? Could you call exact once in the > initialization? > > Note that; my understanding is the value could be fixed at very beginning of > chrome. If not, please let me know. You are right, that's a good point. I've changed to check the Prefs. Those are updated when the policy is loaded. https://codereview.chromium.org/2890843002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/startup_utils.cc:265: } On 2017/05/25 12:41:55, hidehiko wrote: > What value should be assigned in case of no proto? The function is called a few times before the policy is loaded. In case there's no policy data, and there's no sign that the device is consumer owned, the value should remain UNKNOWN while the function returns false. https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... File components/arc/arc_util.cc (right): https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... components/arc/arc_util.cc:34: ARC_AVAILABILITY_POLICY_STATUS arc_availability_policy_status = UNKNOWN; On 2017/05/25 12:41:55, hidehiko wrote: > Please move this into anonymous namespace. > sytle: g_ prefix please. Done. https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... components/arc/arc_util.cc:37: const auto* command_line = base::CommandLine::ForCurrentProcess(); On 2017/05/25 12:41:56, hidehiko wrote: > Please DCHECK arc_availability_policy_status is not UNKNOWN. > > Also, could you at least manually check the code by switching the check to CHECK > temporarily for both enterprise policy applied case and regular case? Can't DCHECK that, because this function is called a few times before the policy is loaded. In that case, the status remains as UNKNOWN and arc is disabled for the time being. I've done manual tests for both, enrolled device and consumer owned device, simulating different policy values with YAPS. https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... components/arc/arc_util.cc:47: // In the case of supported with migration, the ARC availability is defined On 2017/05/25 12:41:55, hidehiko wrote: > Please add detailed unit tests for new behavior. Done. https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... components/arc/arc_util.cc:52: if (arc_availability_policy_status == AVAILABLE) On 2017/05/25 12:41:55, hidehiko wrote: > nit: return arc_availability_policy_status == AVAILABLE; Done. https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_util.h File components/arc/arc_util.h (right): https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... components/arc/arc_util.h:23: "officially-supported-with-migration"; On 2017/05/25 12:41:56, hidehiko wrote: > As commented in design doc, could you introduce another flag, rather than adding > new value to the support status. > > I heard, what you need is whether the device was *originally* launched with > ecryptfs, regardless of the current file system. > So, it sounds independent from ARC's support status. Done. https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... components/arc/arc_util.h:29: enum ARC_AVAILABILITY_POLICY_STATUS { On 2017/05/25 12:41:56, hidehiko wrote: > nit: please use enum class. > > style: Please use CamelCase. ArcAvailabilityPolicyStatus. Done. https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... components/arc/arc_util.h:42: extern ARC_AVAILABILITY_POLICY_STATUS arc_availability_policy_status; On 2017/05/25 12:41:56, hidehiko wrote: > Please remove. > extern decl is not needed. Done.
Description was changed from ========== Policy implementation for encryptfs to ext4 migration strategy Chrome OS encrypts each user’s data with the user’s password. With M60, Chrome OS is switching from ecryptfs to ext4 encryption. ARC N requires ext4 encryption. The policy DeviceEcryptfsMigrationStrategy states whether the device is allowed to use ARC or not. If it's not allowed, the process of data migration should not be launched. Design doc: https://docs.google.com/document/d/18Nz2OClU6CDb7TpZ2MbO4yySvgiOYWsFUxrR4LQYz... This implementation is planned to become obsolete, and be rolled back after the migration process will finish. The issue for that: http://crbug.com/725493 BUG=722371 ========== to ========== Policy implementation for encryptfs to ext4 migration strategy Chrome OS encrypts each user’s data with the user’s password. With M60, Chrome OS is switching from ecryptfs to ext4 encryption. ARC N requires ext4 encryption. The policy DeviceEcryptfsMigrationStrategy states whether the device is allowed to use ARC or not. If it's not allowed, the process of data migration should not be launched. Design doc: https://docs.google.com/document/d/18Nz2OClU6CDb7TpZ2MbO4yySvgiOYWsFUxrR4LQYz... This implementation is planned to become obsolete, and be rolled back after the migration process will finish. The issue for that: http://crbug.com/725493 BUG=722371 TEST=Manual tests and unit tests. ==========
+ext4 migration related engs, just in case. tl;dr; Please avoid changing IsArcAvailable() value in process. If it is impossible because of policy loading timing, I recommend to think about another way not to depend IsArcAvailable(). If necessary, I'm happy to help discuss alternative design. https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... File components/arc/arc_util.cc (right): https://codereview.chromium.org/2890843002/diff/140001/components/arc/arc_uti... components/arc/arc_util.cc:37: const auto* command_line = base::CommandLine::ForCurrentProcess(); On 2017/05/31 17:25:35, igorcov wrote: > On 2017/05/25 12:41:56, hidehiko wrote: > > Please DCHECK arc_availability_policy_status is not UNKNOWN. > > > > Also, could you at least manually check the code by switching the check to > CHECK > > temporarily for both enterprise policy applied case and regular case? > > Can't DCHECK that, because this function is called a few times before the policy > is loaded. In that case, the status remains as UNKNOWN and arc is disabled for > the time being. > > I've done manual tests for both, enrolled device and consumer owned device, > simulating different policy values with YAPS. I think this is must-have thing. Otherwise IsArcAvailable() could change the value in a process. If impossible, maybe it is not yet the time to code review, as designdoc is not yet fixed AFAIK? https://codereview.chromium.org/2890843002/diff/330001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right): https://codereview.chromium.org/2890843002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:395: } What value should be set if has_migration_strategy() is false? https://codereview.chromium.org/2890843002/diff/330001/components/arc/DEPS File components/arc/DEPS (right): https://codereview.chromium.org/2890843002/diff/330001/components/arc/DEPS#ne... components/arc/DEPS:2: "+chrome/common", Depends on chrome files from components is not allowed. https://codereview.chromium.org/2890843002/diff/330001/components/arc/arc_uti... File components/arc/arc_util.cc (right): https://codereview.chromium.org/2890843002/diff/330001/components/arc/arc_uti... components/arc/arc_util.cc:108: return IsMigrationAllowed(); Because, IsArcAvailable() is designed to return a fixed value in a process, and used so. OTOH, this can change the value, so this is probably not a way to go, I think. I'm much worried about changing the timing convention, here. Maybe code review is not an efficient tool to discuss this kind of strategy. Could you fix the design doc first? https://codereview.chromium.org/2890843002/diff/330001/components/arc/arc_util.h File components/arc/arc_util.h (right): https://codereview.chromium.org/2890843002/diff/330001/components/arc/arc_uti... components/arc/arc_util.h:30: enum ArcAvailabilityPolicyStatus { enum class, please.
Patchset #3 (id:350001) has been deleted
Patchset #3 (id:370001) has been deleted
Patchset #3 (id:390001) has been deleted
Patchset #3 (id:410001) has been deleted
Thank you hidehiko for your help making a better design for the implementation. I've moved the changes in IsArcAvailableForProfile. Please take a look again.
Patchset #4 (id:450001) has been deleted
Patchset #3 (id:430001) has been deleted
Patchset #3 (id:470001) has been deleted
The direction looks good from ARC's point of view. My main point in this iteration is "FS is still ecryptfs and Android is M, but migration is NG" case. Other comments are minor. https://codereview.chromium.org/2890843002/diff/490001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2890843002/diff/490001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:93: if (g_arc_availability_policy_status == nit/optional: How about using base::Optional<bool> ? Something like; namespace { base::Optional<bool> g_is_arc_migration_allowed; bool IsArcMigrationAllowedInternal() { if (... if not enterprise managed ...) return true; return pref->GetValue()->GetInt == ALLOW_MIGRATION; } } // namespace bool IsArcMigrationAllowed() { if (!g_is_arc_migration_allowed.has_value()) g_is_arc_migration_allowed = IsArcMigrationAllowedInternal(); return g_is_arc_migration_allowed.value(); } https://codereview.chromium.org/2890843002/diff/490001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:176: if (command_line->HasSwitch(chromeos::switches::kInitialEncryptionEcryptfs) && IIUC, This won't work as expected if; - FS is currently ecryptfs, but - ARC is still M. right? You'd need to check ARC version and/or FS encryption, too? https://codereview.chromium.org/2890843002/diff/490001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2890843002/diff/490001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.h:129: ArcAvailabilityPolicyStatus GetArcAvailabilityPolicyStatus(); Can IsMigrationAllowed be usable directly? I do not think callers do not want to take care about UNKNOWN. https://codereview.chromium.org/2890843002/diff/490001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2890843002/diff/490001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:521: TEST_F(ChromeArcUtilTest, IsMigrationAllowedDeviceOwned) { Nice test coverage! https://codereview.chromium.org/2890843002/diff/490001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:557: scoped_refptr<TestingPrefStore> managed_pref_store(new TestingPrefStore); nit/style: Please use "new T()", instead of "new T". cf) 2. of https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer...
https://codereview.chromium.org/2890843002/diff/490001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2890843002/diff/490001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:176: if (command_line->HasSwitch(chromeos::switches::kInitialEncryptionEcryptfs) && On 2017/06/07 12:22:20, hidehiko wrote: > IIUC, This won't work as expected if; > - FS is currently ecryptfs, but > - ARC is still M. > > right? > You'd need to check ARC version and/or FS encryption, too? Do you mean the case when user had ARC M, had ecryptfs and now is not allowed to migrate? In this case according to design doc, ARC will disappear. We just need to make sure the ARC data is not deleted. Or is it other case and I'm missing something?
Patchset #4 (id:510001) has been deleted
Thank you hidehiko for reviewing this. I've addressed your comments, except the scenario where ARC M is present. Currently if I understand correctly your point, the implementation is according to the design doc and we can discuss there if it makes sense to be changed. Please take a look again. https://codereview.chromium.org/2890843002/diff/490001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2890843002/diff/490001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:93: if (g_arc_availability_policy_status == On 2017/06/07 12:22:20, hidehiko wrote: > nit/optional: How about using base::Optional<bool> ? Something like; > > namespace { > > base::Optional<bool> g_is_arc_migration_allowed; > > bool IsArcMigrationAllowedInternal() { > if (... if not enterprise managed ...) > return true; > return pref->GetValue()->GetInt == ALLOW_MIGRATION; > } > > } // namespace > > bool IsArcMigrationAllowed() { > if (!g_is_arc_migration_allowed.has_value()) > g_is_arc_migration_allowed = IsArcMigrationAllowedInternal(); > return g_is_arc_migration_allowed.value(); > } Done. https://codereview.chromium.org/2890843002/diff/490001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2890843002/diff/490001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.h:129: ArcAvailabilityPolicyStatus GetArcAvailabilityPolicyStatus(); On 2017/06/07 12:22:20, hidehiko wrote: > Can IsMigrationAllowed be usable directly? > I do not think callers do not want to take care about UNKNOWN. Good point, I've replaced it. Thank you. https://codereview.chromium.org/2890843002/diff/490001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2890843002/diff/490001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:521: TEST_F(ChromeArcUtilTest, IsMigrationAllowedDeviceOwned) { On 2017/06/07 12:22:21, hidehiko wrote: > Nice test coverage! Thanks. https://codereview.chromium.org/2890843002/diff/490001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:557: scoped_refptr<TestingPrefStore> managed_pref_store(new TestingPrefStore); On 2017/06/07 12:22:20, hidehiko wrote: > nit/style: Please use "new T()", instead of "new T". > cf) 2. of > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer... Done.
Looks good with minor comments, except stale comment. https://codereview.chromium.org/2890843002/diff/550001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2890843002/diff/550001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:155: // In the case the initial encryption was ecryptfs, the user data require This comment looks now stale. Maybe; If migration policy check is needed (specified by commandline flag), check the policy, which should be already available here. If policy says migration is not allowed, do not run ARC, regardless whether file system migration is actually needed. For example, even if file system is still ecryptfs and ARC version is M, or file system is already migrated into ext4 crypt and ARC version is N or later, if policy says migration is not allowed, ARC will never run. Practically, in the former example case, --need-arc-migration-policy-check is not set, so this check passes and user can use ARC. In latter case, policy should say migration is allowed, so also user can use ARC then. TODO: ... https://codereview.chromium.org/2890843002/diff/550001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2890843002/diff/550001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.h:120: bool IsMigrationAllowed(); nit: could you rename to IsArcMigrationAllowed() for consistency? https://codereview.chromium.org/2890843002/diff/550001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.h:124: void ResetGlobalDataForTesting(); nit: maybe ResetArcMigrationAllowedForTesting() ? https://codereview.chromium.org/2890843002/diff/550001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2890843002/diff/550001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:113: if (is_managed) nit/style/optional: I'd recommend ?: for this case to avoid |registration_mode_| twice. https://codereview.chromium.org/2890843002/diff/550001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:128: if (pref_service_) style: if (pref_service_) return pref_service_; return test_local_state_.get(); https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2890843002/diff/550001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:150: std::unique_ptr<FakeInstallAttributesManaged> attributes = nit/optional: auto may help you to avoid the longer type twice. auto attirbutes = base::MakeUnique<FakeInstallAttributesManaged>();
Patchset #6 (id:570001) has been deleted
Patchset #6 (id:590001) has been deleted
I've addressed the review comments, but also added a small change from previous version: The check for presence of the flag is added now in IsArcMigrationAllowed(). The reason - only the devices in category F should not delete ARC data, and those will have the flag set. In previous version any enrolled device with policy unset, regardless of the category it is in, were disallowed to delete ARC data. https://codereview.chromium.org/2890843002/diff/550001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2890843002/diff/550001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.h:120: bool IsMigrationAllowed(); On 2017/06/09 09:40:39, hidehiko wrote: > nit: could you rename to IsArcMigrationAllowed() for consistency? Done. https://codereview.chromium.org/2890843002/diff/550001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.h:124: void ResetGlobalDataForTesting(); On 2017/06/09 09:40:39, hidehiko wrote: > nit: maybe ResetArcMigrationAllowedForTesting() ? Done. https://codereview.chromium.org/2890843002/diff/550001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2890843002/diff/550001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:113: if (is_managed) On 2017/06/09 09:40:39, hidehiko wrote: > nit/style/optional: I'd recommend ?: for this case to avoid |registration_mode_| > twice. Done. https://codereview.chromium.org/2890843002/diff/550001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:128: if (pref_service_) On 2017/06/09 09:40:39, hidehiko wrote: > style: > if (pref_service_) > return pref_service_; > return test_local_state_.get(); > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2890843002/diff/550001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:150: std::unique_ptr<FakeInstallAttributesManaged> attributes = On 2017/06/09 09:40:39, hidehiko wrote: > nit/optional: auto may help you to avoid the longer type twice. > > auto attirbutes = base::MakeUnique<FakeInstallAttributesManaged>(); Done.
LGTM. Please wait for batfab@'s review. Thank you for clean up!
kinaba@chromium.org changed reviewers: + kinaba@chromium.org
https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:114: bool IsArcAllowedForProfile(const Profile* profile) { Returning false from this utility method does not block the migration UI (contrarily to IsArcAvailable() that indeed blocks the migration.) Will there be additional CL coming for blocking the migration to happen?
On 2017/06/12 00:22:07, kinaba wrote: > https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/arc_util.cc (right): > > https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/arc_util.cc:114: bool IsArcAllowedForProfile(const > Profile* profile) { > Returning false from this utility method does not block the migration UI > (contrarily to IsArcAvailable() that indeed blocks the migration.) > > Will there be additional CL coming for blocking the migration to happen? No other CL is planned for migration. I will test again, and check where the check for migration UI is needed. Thank you.
On 2017/06/12 09:14:10, igorcov wrote: > On 2017/06/12 00:22:07, kinaba wrote: > > > https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... > > File chrome/browser/chromeos/arc/arc_util.cc (right): > > > > > https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... > > chrome/browser/chromeos/arc/arc_util.cc:114: bool IsArcAllowedForProfile(const > > Profile* profile) { > > Returning false from this utility method does not block the migration UI > > (contrarily to IsArcAvailable() that indeed blocks the migration.) > > > > Will there be additional CL coming for blocking the migration to happen? > > No other CL is planned for migration. I will test again, and check where > the check for migration UI is needed. Thank you. These two references are checking the necessity of bringing up migration wizard UI: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/existing_u... https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/screens/us... you might want to have additional condition here.
https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:638: return; Can we add a unit test for this? It would be very valuable to have a test that ensures we do not accidentally cause data loss. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:50: base::Optional<bool> g_is_arc_migration_allowed; Nit: #include "base/optional.h" https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:87: if (!g_browser_process->platform_part() Nit: #include "chrome/browser/browser_process_platform_part.h" https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:93: const auto* command_line = base::CommandLine::ForCurrentProcess(); Nit: const pointer to const https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:101: const PrefService* pref_service = Nit: const pointer to const https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:103: const PrefService::Preference* pref = Nit: const pointer to const https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:107: pref->GetValue()->GetInt() == Nit: #include "base/values.h" https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:173: VLOG(1) << "ARC requires migration, but is not allowed by the policy."; Nit: The log statement contradicts the comment above it. ARC may be disabled, even though no migration is needed. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.h:116: // flag specifying the need to check the policy is missing or if the device is "the flag specifying the need to check the policy" - Which flag? Which policy? Assume that the reader of this comment has no context. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.h:119: // used, and the policy update won't change the return value after that. 1: Nit: s/the/a/ 2: Nit: s/after that/after that until the next Chrome restart/ 3: If Chrome crashes, comes back up again and the policy has changed, will that cause problems? https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:113: registration_mode_ = is_managed ? policy::DEVICE_MODE_ENTERPRISE Nit: #include "components/policy/core/common/cloud/cloud_policy_constants.h" https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:137: PrefService* pref_service_ = nullptr; How about calling this |local_state_| instead so that it is clear which PrefService you mean (local state or a specific user's prefs)? https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:527: TEST_F(ChromeArcUtilTest, IsMigrationAllowedDeviceOwned) { Nit: What do you mean by "DeviceOwned"? Should this not rather be "Consumer" or "ConsumerOwned"? https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:529: auto* command_line = base::CommandLine::ForCurrentProcess(); Nit: const pointer. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:558: FakeUserManagerWithLocalState* fake_user_manager = Nit: const pointer https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:582: TEST_F(ChromeArcUtilTest, IsMigrationAllowedPolicyDisabled) { Nit: There is a lot of copy & paste between these tests. Could you refactor them to share more code? https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right): https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:181: registry->RegisterIntegerPref(prefs::kDeviceEcryptfsMigrationStrategy, 0); Nit: You could use enterprise_management::DeviceEcryptfsMigrationStrategyProto::UNSET as initial value. https://codereview.chromium.org/2890843002/diff/610001/chromeos/settings/cros... File chromeos/settings/cros_settings_names.cc (right): https://codereview.chromium.org/2890843002/diff/610001/chromeos/settings/cros... chromeos/settings/cros_settings_names.cc:248: // An integer pref, specifying the ecryptfs to ext4 migration strategy. Nit: Follow the style of the other enums in this file (lines 200, 208): Colon at the end of the header, = instead of - for each item, no full stop at the end of items. https://codereview.chromium.org/2890843002/diff/610001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2890843002/diff/610001/components/policy/reso... components/policy/resources/policy_templates.json:9576: 'enum': [ 0, 1 ], This does not match the actual policy values (0, 1, 2). https://codereview.chromium.org/2890843002/diff/610001/components/policy/reso... components/policy/resources/policy_templates.json:9599: 'desc': '''Specifies how a device should behave that shipped before <ph name="PRODUCT_OS_NAME">$2<ex>Google Chrome OS</ex></ph> 60 and may have home directories using ecryptfs instead of ext4 encryption. Some devices will stay on ecryptfs in M60 as well. This is tied to the switch to ext4 encryption only, not to a specific CrOS version.
Thank you Bartosz for review. I've addressed the comments, please take a look again. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:638: return; On 2017/06/12 12:49:03, bartfab (slow) wrote: > Can we add a unit test for this? It would be very valuable to have a test that > ensures we do not accidentally cause data loss. As far as I understand the code, this should be a DCHECK because in current implementation the |profile_| can be set only if IsArcAllowedForProfile() returns true. I've left it as an if for safety reason, but don't see a way to make a test for it (meaning hitting this case). https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:50: base::Optional<bool> g_is_arc_migration_allowed; On 2017/06/12 12:49:03, bartfab (slow) wrote: > Nit: #include "base/optional.h" Done. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:87: if (!g_browser_process->platform_part() On 2017/06/12 12:49:03, bartfab (slow) wrote: > Nit: #include "chrome/browser/browser_process_platform_part.h" Done. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:93: const auto* command_line = base::CommandLine::ForCurrentProcess(); On 2017/06/12 12:49:04, bartfab (slow) wrote: > Nit: const pointer to const Done. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:101: const PrefService* pref_service = On 2017/06/12 12:49:03, bartfab (slow) wrote: > Nit: const pointer to const Done. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:103: const PrefService::Preference* pref = On 2017/06/12 12:49:04, bartfab (slow) wrote: > Nit: const pointer to const Done. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:107: pref->GetValue()->GetInt() == On 2017/06/12 12:49:03, bartfab (slow) wrote: > Nit: #include "base/values.h" Done. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:114: bool IsArcAllowedForProfile(const Profile* profile) { On 2017/06/12 00:22:07, kinaba wrote: > Returning false from this utility method does not block the migration UI > (contrarily to IsArcAvailable() that indeed blocks the migration.) > > Will there be additional CL coming for blocking the migration to happen? Added additional checks for migration UI. Thank you. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:173: VLOG(1) << "ARC requires migration, but is not allowed by the policy."; On 2017/06/12 12:49:04, bartfab (slow) wrote: > Nit: The log statement contradicts the comment above it. ARC may be disabled, > even though no migration is needed. Fixed the log statement. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.h:116: // flag specifying the need to check the policy is missing or if the device is On 2017/06/12 12:49:04, bartfab (slow) wrote: > "the flag specifying the need to check the policy" - Which flag? Which policy? > Assume that the reader of this comment has no context. Done. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.h:119: // used, and the policy update won't change the return value after that. On 2017/06/12 12:49:04, bartfab (slow) wrote: > 1: Nit: s/the/a/ > 2: Nit: s/after that/after that until the next Chrome restart/ > 3: If Chrome crashes, comes back up again and the policy has changed, will that > cause problems? 1,2: Done 3: After Chrome crash, user has to sign in and the session starts as if after the boot. I think it would be the same flow. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:113: registration_mode_ = is_managed ? policy::DEVICE_MODE_ENTERPRISE On 2017/06/12 12:49:04, bartfab (slow) wrote: > Nit: #include "components/policy/core/common/cloud/cloud_policy_constants.h" Done. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:137: PrefService* pref_service_ = nullptr; On 2017/06/12 12:49:04, bartfab (slow) wrote: > How about calling this |local_state_| instead so that it is clear which > PrefService you mean (local state or a specific user's prefs)? Done. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:527: TEST_F(ChromeArcUtilTest, IsMigrationAllowedDeviceOwned) { On 2017/06/12 12:49:04, bartfab (slow) wrote: > Nit: What do you mean by "DeviceOwned"? Should this not rather be "Consumer" or > "ConsumerOwned"? Done. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:529: auto* command_line = base::CommandLine::ForCurrentProcess(); On 2017/06/12 12:49:04, bartfab (slow) wrote: > Nit: const pointer. const not accepted by the compiler here. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:558: FakeUserManagerWithLocalState* fake_user_manager = On 2017/06/12 12:49:04, bartfab (slow) wrote: > Nit: const pointer Compiler complains. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:582: TEST_F(ChromeArcUtilTest, IsMigrationAllowedPolicyDisabled) { On 2017/06/12 12:49:04, bartfab (slow) wrote: > Nit: There is a lot of copy & paste between these tests. Could you refactor them > to share more code? Done. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right): https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:181: registry->RegisterIntegerPref(prefs::kDeviceEcryptfsMigrationStrategy, 0); On 2017/06/12 12:49:04, bartfab (slow) wrote: > Nit: You could use > enterprise_management::DeviceEcryptfsMigrationStrategyProto::UNSET as initial > value. Done. https://codereview.chromium.org/2890843002/diff/610001/chromeos/settings/cros... File chromeos/settings/cros_settings_names.cc (right): https://codereview.chromium.org/2890843002/diff/610001/chromeos/settings/cros... chromeos/settings/cros_settings_names.cc:248: // An integer pref, specifying the ecryptfs to ext4 migration strategy. On 2017/06/12 12:49:04, bartfab (slow) wrote: > Nit: Follow the style of the other enums in this file (lines 200, 208): Colon at > the end of the header, = instead of - for each item, no full stop at the end of > items. Done. https://codereview.chromium.org/2890843002/diff/610001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2890843002/diff/610001/components/policy/reso... components/policy/resources/policy_templates.json:9576: 'enum': [ 0, 1 ], On 2017/06/12 12:49:04, bartfab (slow) wrote: > This does not match the actual policy values (0, 1, 2). Done. https://codereview.chromium.org/2890843002/diff/610001/components/policy/reso... components/policy/resources/policy_templates.json:9599: 'desc': '''Specifies how a device should behave that shipped before <ph name="PRODUCT_OS_NAME">$2<ex>Google Chrome OS</ex></ph> 60 and may have home directories using ecryptfs instead of ext4 encryption. On 2017/06/12 12:49:04, bartfab (slow) wrote: > Some devices will stay on ecryptfs in M60 as well. This is tied to the switch to > ext4 encryption only, not to a specific CrOS version. Done.
https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.h:119: // used, and the policy update won't change the return value after that. On 2017/06/12 16:50:10, igorcov wrote: > On 2017/06/12 12:49:04, bartfab (slow) wrote: > > 1: Nit: s/the/a/ > > 2: Nit: s/after that/after that until the next Chrome restart/ > > 3: If Chrome crashes, comes back up again and the policy has changed, will > that > > cause problems? > > 1,2: Done > 3: After Chrome crash, user has to sign in and the session starts as if after > the boot. I think it would be the same flow. Re 3: Actually, no. login_manager is a watchdog that catches Chrome crashes and restarts the browser, without requiring you to log in again. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:529: auto* command_line = base::CommandLine::ForCurrentProcess(); On 2017/06/12 16:50:10, igorcov wrote: > On 2017/06/12 12:49:04, bartfab (slow) wrote: > > Nit: const pointer. > > const not accepted by the compiler here. Are you sure you were trying to make it a const pointer, not a pointer to const? https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:198: VLOG(1) << "ARC migration is not allowed by the policy."; Nit: s/the // https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.h:116: // flag --need-arc-migration-policy-check is missing or if the device is Nit: Let's say "not set" instead of "missing." "Missing" sounds like something is wrong. https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:196: int pref_value = migration_allowed ? kMigrationAllowedPolicyEnabled Nit: const https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:198: auto* command_line = base::CommandLine::ForCurrentProcess(); Nit: const pointer https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/login/existing_user_controller.cc:195: if (!arc::IsArcMigrationAllowed()) Can we test this? https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right): https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/user_selection_screen.cc:150: arc::IsArcAvailable() && arc::IsArcMigrationAllowed(); Can we test this? https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right): https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:183: enterprise_management::DeviceEcryptfsMigrationStrategyProto::UNSET); Nit: #include "chrome/browser/chromeos/policy/proto/chrome_device_policy.pb.h" https://codereview.chromium.org/2890843002/diff/650001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2890843002/diff/650001/components/policy/reso... components/policy/resources/policy_templates.json:9604: 'desc': '''Specifies how a device should behave that shipped with ecryptfs encryption. To be precise, this only affects the behavior of devices that transitioned from ecryptfs to ext4 encryption. Devices that are keeping ecryptfs as the default for now are not affected.
lgtm for the points I commented (blocking migration UIs)
Patchset #9 (id:670001) has been deleted
Just FYI. https://codereview.chromium.org/2890843002/diff/710001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2890843002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:638: if (!arc::IsArcMigrationAllowed()) { nit: Could you elide the brace for one-line if-statement? https://codereview.chromium.org/2890843002/diff/710001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right): https://codereview.chromium.org/2890843002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:51: #include "chrome/browser/chromeos/policy/proto/chrome_device_policy.pb.h" nit: looks unnecessary?
Patchset #11 (id:730001) has been deleted
https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.h:119: // used, and the policy update won't change the return value after that. On 2017/06/13 09:56:01, bartfab (slow) wrote: > On 2017/06/12 16:50:10, igorcov wrote: > > On 2017/06/12 12:49:04, bartfab (slow) wrote: > > > 1: Nit: s/the/a/ > > > 2: Nit: s/after that/after that until the next Chrome restart/ > > > 3: If Chrome crashes, comes back up again and the policy has changed, will > > that > > > cause problems? > > > > 1,2: Done > > 3: After Chrome crash, user has to sign in and the session starts as if after > > the boot. I think it would be the same flow. > > Re 3: Actually, no. login_manager is a watchdog that catches Chrome crashes and > restarts the browser, without requiring you to log in again. Changed the code so that policy is decoded every time we set the value. The reason is, in previous version the policy though decoded was not propagated in local state after the crash, so ARC became disabled for every enrolled device. Current functionality makes ARC disabled after crash if policy was changed from migration allowed to disabled, and in other case the user is notified that sign out + sign in is required for a critical update. This functionality looks good to me. Thank you. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:529: auto* command_line = base::CommandLine::ForCurrentProcess(); On 2017/06/13 09:56:01, bartfab (slow) wrote: > On 2017/06/12 16:50:10, igorcov wrote: > > On 2017/06/12 12:49:04, bartfab (slow) wrote: > > > Nit: const pointer. > > > > const not accepted by the compiler here. > > Are you sure you were trying to make it a const pointer, not a pointer to const? I'm sorry, I guess C++ syntax is not that intuitive. I've tried to do something similar to https://cs.chromium.org/chromium/src/remoting/host/daemon_process.cc?type=cs&... initially. Seems like compiler interprets that as pointer to const. https://codereview.chromium.org/2890843002/diff/610001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:558: FakeUserManagerWithLocalState* fake_user_manager = On 2017/06/12 16:50:10, igorcov wrote: > On 2017/06/12 12:49:04, bartfab (slow) wrote: > > Nit: const pointer > > Compiler complains. Const-ed this too. https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:198: VLOG(1) << "ARC migration is not allowed by the policy."; On 2017/06/13 09:56:01, bartfab (slow) wrote: > Nit: s/the // Done. https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.h:116: // flag --need-arc-migration-policy-check is missing or if the device is On 2017/06/13 09:56:01, bartfab (slow) wrote: > Nit: Let's say "not set" instead of "missing." "Missing" sounds like something > is wrong. Done. https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:196: int pref_value = migration_allowed ? kMigrationAllowedPolicyEnabled On 2017/06/13 09:56:02, bartfab (slow) wrote: > Nit: const Done. https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:198: auto* command_line = base::CommandLine::ForCurrentProcess(); On 2017/06/13 09:56:02, bartfab (slow) wrote: > Nit: const pointer Done. https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right): https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/user_selection_screen.cc:150: arc::IsArcAvailable() && arc::IsArcMigrationAllowed(); On 2017/06/13 09:56:02, bartfab (slow) wrote: > Can we test this? Couldn't find an easy way to make this. https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right): https://codereview.chromium.org/2890843002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:183: enterprise_management::DeviceEcryptfsMigrationStrategyProto::UNSET); On 2017/06/13 09:56:02, bartfab (slow) wrote: > Nit: #include "chrome/browser/chromeos/policy/proto/chrome_device_policy.pb.h" Done. https://codereview.chromium.org/2890843002/diff/650001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2890843002/diff/650001/components/policy/reso... components/policy/resources/policy_templates.json:9604: 'desc': '''Specifies how a device should behave that shipped with ecryptfs encryption. On 2017/06/13 09:56:02, bartfab (slow) wrote: > To be precise, this only affects the behavior of devices that transitioned from > ecryptfs to ext4 encryption. Devices that are keeping ecryptfs as the default > for now are not affected. Done.
igorcov@chromium.org changed reviewers: + achuith@chromium.org
achuith@chromium.org: Please review changes in chromeos/login.
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
On 2017/06/16 11:23:25, igorcov wrote: > mailto:achuith@chromium.org: Please review changes in chromeos/login. chromeos/login lgtm
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by igorcov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #9 (id:690001) has been deleted
The CQ bit was checked by igorcov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #7 (id:630001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #11 (id:790001) has been deleted
lgtm https://codereview.chromium.org/2890843002/diff/810001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2890843002/diff/810001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:527: TEST_F(ArcMigrationTest, IsMigrationAllowedNoPolicy) { You used to have tests for policy set to disallow/allow. Is this no longer possible?
The CQ bit was checked by igorcov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org, kinaba@chromium.org, achuith@chromium.org, bartfab@chromium.org Link to the patchset: https://codereview.chromium.org/2890843002/#ps830001 (title: "Merge done")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you Bartosz. https://codereview.chromium.org/2890843002/diff/810001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2890843002/diff/810001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util_unittest.cc:527: TEST_F(ArcMigrationTest, IsMigrationAllowedNoPolicy) { On 2017/06/21 21:31:16, bartfab (slow) wrote: > You used to have tests for policy set to disallow/allow. Is this no longer > possible? Yes, I've removed the tests that involved the policy being present because the latest code is taking the policy data from device_settings which is static. To include a test for that would require some injector for testing in that class and some fake class added to simulate the policy here. Might add this as a separate CL later.
CQ is committing da patch.
Bot data: {"patchset_id": 830001, "attempt_start_ts": 1498123041806230,
"parent_rev": "067810b6cf159b12c9956147a6d0217526c627f9", "commit_rev":
"24be4caf84c7c14e68c4c10206c3ea0a506c0ee6"}
Message was sent while issue was closed.
Description was changed from ========== Policy implementation for encryptfs to ext4 migration strategy Chrome OS encrypts each user’s data with the user’s password. With M60, Chrome OS is switching from ecryptfs to ext4 encryption. ARC N requires ext4 encryption. The policy DeviceEcryptfsMigrationStrategy states whether the device is allowed to use ARC or not. If it's not allowed, the process of data migration should not be launched. Design doc: https://docs.google.com/document/d/18Nz2OClU6CDb7TpZ2MbO4yySvgiOYWsFUxrR4LQYz... This implementation is planned to become obsolete, and be rolled back after the migration process will finish. The issue for that: http://crbug.com/725493 BUG=722371 TEST=Manual tests and unit tests. ========== to ========== Policy implementation for encryptfs to ext4 migration strategy Chrome OS encrypts each user’s data with the user’s password. With M60, Chrome OS is switching from ecryptfs to ext4 encryption. ARC N requires ext4 encryption. The policy DeviceEcryptfsMigrationStrategy states whether the device is allowed to use ARC or not. If it's not allowed, the process of data migration should not be launched. Design doc: https://docs.google.com/document/d/18Nz2OClU6CDb7TpZ2MbO4yySvgiOYWsFUxrR4LQYz... This implementation is planned to become obsolete, and be rolled back after the migration process will finish. The issue for that: http://crbug.com/725493 BUG=722371 TEST=Manual tests and unit tests. Review-Url: https://codereview.chromium.org/2890843002 Cr-Commit-Position: refs/heads/master@{#481499} Committed: https://chromium.googlesource.com/chromium/src/+/24be4caf84c7c14e68c4c10206c3... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:830001) as https://chromium.googlesource.com/chromium/src/+/24be4caf84c7c14e68c4c10206c3... |
