Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9405035:
Implement ephemeral users (Closed)
|
Created:
8 years, 10 months ago by use bartfab instead Modified:
6 years, 8 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org, rkc, pastarmovj, merkulova, dzhioev (left Google) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionImplement ephemeral users
This CL completes the implementation of http://crosbug.com/20004.
When the ephemeral users policy is enabled, users are not added to
UserManager's persistent list and are not asked to pick a user image.
UserManager additionally checks for stale entries created before the
policy was enabled and removes these, along with downloaded user
images.
See http://goo.gl/wTPEb for a design doc.
The CL includes unit tests for the new feature.
BUG=chromium-os:20004
TEST=unittest UserManagerTest*
TEST=manual feature test in numerous scenarios on a Chromebook
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126198
Patch Set 1 #Patch Set 2 : This version is actually feature-complete but still untested. #Patch Set 3 : Next iteration, including a small fix and a set of unit tests. #Patch Set 4 : Complete, tested implementation - ready for review. #
Total comments: 4
Patch Set 5 : Comments addressed. #
Total comments: 57
Patch Set 6 : Comments addressed. #
Total comments: 19
Patch Set 7 : Comments addressed. #
Total comments: 14
Patch Set 8 : Rebased and refactored after recent changes in Chrome. #Patch Set 9 : As discussed, removed initial image selection. #
Total comments: 2
Messages
Total messages: 39 (0 generated)
Hi Chris, Денис, I have a CL that matured over the past few weeks and is ready for review now. Could you please have a look as owners of the affected files? Thanks, - Bartosz
https://chromiumcodereview.appspot.com/9405035/diff/10001/chrome/browser/reso... File chrome/browser/resources/options/personal_options.js (right): https://chromiumcodereview.appspot.com/9405035/diff/10001/chrome/browser/reso... chrome/browser/resources/options/personal_options.js:126: } else if (localStrings.getString('current_user_is_ephemeral')) { should be: if (localStrings.getString('current_user_is_ephemeral') == 'true') https://chromiumcodereview.appspot.com/9405035/diff/10001/chrome/browser/ui/w... File chrome/browser/ui/webui/options/personal_options_handler.cc (right): https://chromiumcodereview.appspot.com/9405035/diff/10001/chrome/browser/ui/w... chrome/browser/ui/webui/options/personal_options_handler.cc:210: ASCIIToUTF16("true") : ASCIIToUTF16("false")); Please move this into account_options_handler grouped with related UserManager values such as 'current_user_is_owner'. Use the same pattern.
Comments addressed. https://chromiumcodereview.appspot.com/9405035/diff/10001/chrome/browser/reso... File chrome/browser/resources/options/personal_options.js (right): https://chromiumcodereview.appspot.com/9405035/diff/10001/chrome/browser/reso... chrome/browser/resources/options/personal_options.js:126: } else if (localStrings.getString('current_user_is_ephemeral')) { On 2012/03/01 02:10:42, csilv wrote: > should be: > if (localStrings.getString('current_user_is_ephemeral') == 'true') Done. https://chromiumcodereview.appspot.com/9405035/diff/10001/chrome/browser/ui/w... File chrome/browser/ui/webui/options/personal_options_handler.cc (right): https://chromiumcodereview.appspot.com/9405035/diff/10001/chrome/browser/ui/w... chrome/browser/ui/webui/options/personal_options_handler.cc:210: ASCIIToUTF16("true") : ASCIIToUTF16("false")); On 2012/03/01 02:10:42, csilv wrote: > Please move this into account_options_handler grouped with related UserManager > values such as 'current_user_is_owner'. Use the same pattern. Done.
+Ivan, he previously did UserManager refactoring.
https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... File chrome/browser/chromeos/login/user_manager.cc (right): https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:1243: delete users; scoped_ptr<>?
http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/user_manager.cc (right): http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:557: // Do not update local store if the user is ephemeral. You don't need most of these checks, only those where persistent storage is modified. http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:579: // Ignore for ephemeral users. Not needed. http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:590: if (IsEphemeralUser(username)) Not needed. http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:599: if (IsEphemeralUser(username)) Not needed. http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:610: if (IsEphemeralUser(username)) Not needed. http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:635: if (current_user_is_ephemeral_) { Not needed. http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:732: return; Multiline if conditions require braces, too. http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:739: owner_.reset(new std::string); There are alredy pieces of code that fetch owner (see RemoveUserInternal). I suggest to add method GetOwner() that will call GetTrusted/GetString from all places. http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:939: if (IsEphemeralUser(username)) Not needed. http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:951: if (IsEphemeralUser(username)) Not needed. http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:987: if (IsEphemeralUser(username)) This should be after SetUserImage. http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:1012: if (IsEphemeralUser(username)) Not needed (never reached). http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:1042: // Ignore for ephemeral users. Here it's ok. http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:1198: if (!AreEphemeralUsersEnabled()) No point in this check. http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:1201: const std::string owner_email = *owner_; This looks overly complicated. Since this should be doing any real job only once, when ephemeral users are enabled, I'd just call RemoveUserFromList for every user other than owner and logged-in user. http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/user_manager.h (right): http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.h:185: void RetrieveTrustedDevicePolicies(); This can be made private, right? http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/user_manager_unittest.cc (right): http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager_unittest.cc:26: class UserManagerTest : public testing::Test { Looks like this ought to be an InProcessBrowserTest, really. http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager_unittest.cc:56: // Text missing? http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager_unittest.cc:80: // One does not simply log out of UserManager. The UserManager is normally This looks very fragile. In case of InProcessBrowserTest (when you don't care about teardown and this is only need to add multiple users) you can just reset user_is_logged_in_ before each UserLoggedIn() call. http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager_unittest.cc:98: scoped_ptr<base::Value> owner_value( You don't need heep-allocated values here. http://codereview.chromium.org/9405035/diff/17001/chrome/browser/resources/op... File chrome/browser/resources/options/personal_options.js (right): http://codereview.chromium.org/9405035/diff/17001/chrome/browser/resources/op... chrome/browser/resources/options/personal_options.js:125: $('change-picture-button').disabled = true; Please update options2 accordingly.
Comments addressed. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... File chrome/browser/chromeos/login/user_manager.cc (right): https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:557: // Do not update local store if the user is ephemeral. On 2012/03/01 13:37:50, Ivan Korotkov wrote: > You don't need most of these checks, only those where persistent storage is > modified. Done. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:579: // Ignore for ephemeral users. On 2012/03/01 13:37:50, Ivan Korotkov wrote: > Not needed. Done. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:590: if (IsEphemeralUser(username)) On 2012/03/01 13:37:50, Ivan Korotkov wrote: > Not needed. Done. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:599: if (IsEphemeralUser(username)) On 2012/03/01 13:37:50, Ivan Korotkov wrote: > Not needed. Done. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:610: if (IsEphemeralUser(username)) On 2012/03/01 13:37:50, Ivan Korotkov wrote: > Not needed. Done. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:635: if (current_user_is_ephemeral_) { On 2012/03/01 13:37:50, Ivan Korotkov wrote: > Not needed. Done. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:732: return; On 2012/03/01 13:37:50, Ivan Korotkov wrote: > Multiline if conditions require braces, too. Done. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:739: owner_.reset(new std::string); The problem is that GetTrusted() must be provided with a callback to retry whatever action was desired if trusted information is not available yet. If GetTrusted() was called from within a new GetOwner(), this method would need to take a callback parameter to tell it how to repeat the entire operation if trusted information is not yet available. This would seem more complicated to me but if you wish, I can change the code. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:939: if (IsEphemeralUser(username)) On 2012/03/01 13:37:50, Ivan Korotkov wrote: > Not needed. Done. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:951: if (IsEphemeralUser(username)) On 2012/03/01 13:37:50, Ivan Korotkov wrote: > Not needed. Done. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:987: if (IsEphemeralUser(username)) On 2012/03/01 13:37:50, Ivan Korotkov wrote: > This should be after SetUserImage. Done. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:1012: if (IsEphemeralUser(username)) On 2012/03/01 13:37:50, Ivan Korotkov wrote: > Not needed (never reached). Done. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:1042: // Ignore for ephemeral users. On 2012/03/01 13:37:50, Ivan Korotkov wrote: > Here it's ok. Done. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:1198: if (!AreEphemeralUsersEnabled()) On 2012/03/01 13:37:50, Ivan Korotkov wrote: > No point in this check. Done. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:1201: const std::string owner_email = *owner_; I considered this but it will not work: RemoveUserFromList() calls EnsureUsersLoaded(). If any user has a custom profile image, this will kick off a download of that image in the background. If the user is deleted a moment later, the download will continue and will leave a stale image file behind. This is why I rolled my own removal that does not download anything. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:1243: delete users; On 2012/03/01 10:01:58, Nikita Kostylev wrote: > scoped_ptr<>? Done. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... File chrome/browser/chromeos/login/user_manager.h (right): https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.h:185: void RetrieveTrustedDevicePolicies(); On 2012/03/01 13:37:50, Ivan Korotkov wrote: > This can be made private, right? Done. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... File chrome/browser/chromeos/login/user_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager_unittest.cc:26: class UserManagerTest : public testing::Test { It does not need the complete browser. I can change it of course, but why make it more heavyweight? https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager_unittest.cc:56: // On 2012/03/01 13:37:50, Ivan Korotkov wrote: > Text missing? Done. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager_unittest.cc:80: // One does not simply log out of UserManager. The UserManager is normally It would also be sufficient to reset user_is_logged_in_ for a unit test. I am resetting the rest of the internal state to make the test is less fragile, in case some code in UserManager comes to rely on it being valid and consistent in the future. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager_unittest.cc:98: scoped_ptr<base::Value> owner_value( On 2012/03/01 13:37:50, Ivan Korotkov wrote: > You don't need heep-allocated values here. Done. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/reso... File chrome/browser/resources/options/personal_options.js (right): https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/reso... chrome/browser/resources/options/personal_options.js:125: $('change-picture-button').disabled = true; On 2012/03/01 13:37:50, Ivan Korotkov wrote: > Please update options2 accordingly. Done.
https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... File chrome/browser/chromeos/login/user_manager.cc (right): https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:739: owner_.reset(new std::string); On 2012/03/01 18:33:43, bartfab wrote: > The problem is that GetTrusted() must be provided with a callback to retry > whatever action was desired if trusted information is not available yet. If > GetTrusted() was called from within a new GetOwner(), this method would need to > take a callback parameter to tell it how to repeat the entire operation if > trusted information is not yet available. This would seem more complicated to me > but if you wish, I can change the code. Right, I'm sorry for the confusion. In that case, please make RemoveUserInternal a member function so that it makes use of owner_. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:987: if (IsEphemeralUser(username)) On 2012/03/01 18:33:43, bartfab wrote: > On 2012/03/01 13:37:50, Ivan Korotkov wrote: > > This should be after SetUserImage. > > Done. Hey, it's gone now. I meant that it should be after the SetUserImage call (before the SaveImageToFile task is posted). https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:1201: const std::string owner_email = *owner_; On 2012/03/01 18:33:43, bartfab wrote: > I considered this but it will not work: > > RemoveUserFromList() calls EnsureUsersLoaded(). If any user has a custom profile > image, this will kick off a download of that image in the background. If the > user is deleted a moment later, the download will continue and will leave a > stale image file behind. This is why I rolled my own removal that does not > download anything. Ah, I see. Add a comment about that, please. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... File chrome/browser/chromeos/login/user_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager_unittest.cc:26: class UserManagerTest : public testing::Test { On 2012/03/01 18:33:43, bartfab wrote: > It does not need the complete browser. I can change it of course, but why make > it more heavyweight? > Ok, let's leave it a unittest. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager_unittest.cc:80: // One does not simply log out of UserManager. The UserManager is normally On 2012/03/01 18:33:43, bartfab wrote: > It would also be sufficient to reset user_is_logged_in_ for a unit test. I am > resetting the rest of the internal state to make the test is less fragile, in > case some code in UserManager comes to rely on it being valid and consistent in > the future. Hmmmm ok. What about adding a method UserManager::LogoutForTest then? https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager_unittest.cc:87: user_manager_->logged_in_user_ = NULL; Safer to set it to stub_user_ as done initially. https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... File chrome/browser/chromeos/login/user.h (right): https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... chrome/browser/chromeos/login/user.h:74: friend class UserManagerTest; Why do you need this? https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... File chrome/browser/chromeos/login/user_manager.cc (right): https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:332: current_user_is_ephemeral_ = true; What's the point of making guest/demo user ephemeral btw? https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... File chrome/browser/chromeos/login/user_manager.h (right): https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.h:326: scoped_ptr<bool> ephemeral_users_enabled_; I'm not sure if you really need the exact value from the policy. Why not just a simple bool ephemeral_user_enabled_ = false and in RetrieveDevicePolicy() set it to GetString(kAccountsPrefEphemeralUsersEnabled) && owner ? https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.h:330: scoped_ptr<std::string> owner_; Please, rename this to owner_email_ as owner_ suggests that it is a User instance. https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... File chrome/browser/chromeos/login/user_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager_unittest.cc:98: stub_settings_provider_.Set(kDeviceOwner, base::StringValue(owner)); I'm not sure if clang is gonna like this. Previously it required you to write base::StringValue smth(...); and then pass smth as argument. https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/reso... File chrome/browser/resources/options2/browser_options.js (right): https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/reso... chrome/browser/resources/options2/browser_options.js:201: // ephemeral. #change-picture-caption should be made invisible as well.
http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/user_manager.cc (right): http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:1201: const std::string owner_email = *owner_; On 2012/03/01 18:33:43, bartfab wrote: > I considered this but it will not work: > > RemoveUserFromList() calls EnsureUsersLoaded(). If any user has a custom profile > image, this will kick off a download of that image in the background. If the > user is deleted a moment later, the download will continue and will leave a > stale image file behind. This is why I rolled my own removal that does not > download anything. Another option could be to refactor RemoveUserFromList to first call EnsureUsersLoaded() then call a RemoveUserFromListWithoutLoadedCheck(); your function could call the latter method directly. It would avoid all this repeated code and keep the remove user logic in a central method.
Another couple of thought. Also: what about the case when browser crashes in the middle of ephemeral user session? Local State will contain no info about the user. It should be ok for user names and images, but OAuth token status will be INVALID. Please check this case. http://codereview.chromium.org/9405035/diff/25001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/user_manager.cc (right): http://codereview.chromium.org/9405035/diff/25001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:843: RemoveAllExceptOwnerFromList(); Discussed this with Nikita: Please consider delaying user login until these trusted settings are fetched. Otherwise, a user can login before the trusted settings are verified and will become persistent. For example, see LoginPerformer::CompleteLogin waiting for "allow new user" before completing login. http://codereview.chromium.org/9405035/diff/25001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:1163: // users were enabled, this user should not be deleted yet either. Are we sure about that decision? As far as I understand, enabling ephemeral users means that ALL users except the owner should not be preserved after logging out.
http://codereview.chromium.org/9405035/diff/25001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/user_manager.cc (right): http://codereview.chromium.org/9405035/diff/25001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:1163: // users were enabled, this user should not be deleted yet either. On 2012/03/02 12:39:38, Ivan Korotkov wrote: > ALL users except the owner should not be preserved after > logging out. And this only makes sense when we want "Enable ephemeral users" as a setting for non-enterprise owned devices (i.e. having an actual user-owner). If that's not the case, your change should be simplified as in enterprise scenario all users are not owner and may be deleted. Simplifying this change means removing flag "is_ephemeral_user_" and such users would not be different from any other user. Policy would be applied only at sign out / boot i.e. cleanup list of users. http://codereview.chromium.org/9405035/diff/25001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/user_manager.h (right): http://codereview.chromium.org/9405035/diff/25001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.h:318: // Cached flag of whether the currently logged-in user is ephemeral. Please expand this comment on what "currently logged-in user is ephemeral" actually means. Otherwise this knowledge is not concentrated in one place.
All inline comments addressed. Regarding Ivan's question about OAuth: The OAuth handling is not changed by the ephemeral user setting. User manager retrieves the OAuth torken as before, whether the user is ephemeral or not. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... File chrome/browser/chromeos/login/user_manager.cc (right): https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:739: owner_.reset(new std::string); This will not work I fear. What if the owner_ has not been successfully fetched yet? RemoveUserInternal needs to find out somehow and schedule a callback to itself in this case. Even if RemoveUserInternal is made a member function, it will still need to make its own call to GetTrusted() so that it can schedule its own callback. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:987: if (IsEphemeralUser(username)) Done. I do not understand how this makes sense though. SaveUserImageInternal() now sets the image index to either kExternalImageIndex or kProfileImageIndex but then returns before creating an image file. So you have an index implying an external image which does not actually exist. Would it not be better to move the IsEphemeralUser() check to the top of the function so that the index is not set in the first place? https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:1201: const std::string owner_email = *owner_; Good idea. I implemented it like this using a new RemoveUserFromListInternal() function for the shared code. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... File chrome/browser/chromeos/login/user_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager_unittest.cc:80: // One does not simply log out of UserManager. The UserManager is normally On 2012/03/01 20:16:30, Ivan Korotkov wrote: > On 2012/03/01 18:33:43, bartfab wrote: > > It would also be sufficient to reset user_is_logged_in_ for a unit test. I am > > resetting the rest of the internal state to make the test is less fragile, in > > case some code in UserManager comes to rely on it being valid and consistent > in > > the future. > > Hmmmm ok. What about adding a method UserManager::LogoutForTest then? Done. https://chromiumcodereview.appspot.com/9405035/diff/17001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager_unittest.cc:87: user_manager_->logged_in_user_ = NULL; I set it to NULL intentionally. This is the value that logged_in_user_ normally has until a user logs in. The stub_user_ is a hack needed by other specific test cases only. https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... File chrome/browser/chromeos/login/user.h (right): https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... chrome/browser/chromeos/login/user.h:74: friend class UserManagerTest; This friend declaration existed because UserManagerTest::ResetUserManagerLoggedInUser() was calling the private User::~User(). It is no longer necessary as the destructor is called from UserManager::LogoutForTest() now. https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... File chrome/browser/chromeos/login/user_manager.cc (right): https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:332: current_user_is_ephemeral_ = true; It is logically consistent. The guest user's recent visits are not recorded and the guest cryptohome uses tmpfs. This also simplifies the check for whether persistent information (such as a user image) is permitted for the current user from (current_user_is_guest_ || current_user_is_ephemeral_) to just (current_user_is_ephemeral_). https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:843: RemoveAllExceptOwnerFromList(); I discussed this with the folks here at the office. It turns out that trusted policy will always be available before login already. Without it, features such as the user whitelist would not work. Thus, no additional check and potential login delay is necessary. https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.cc:1163: // users were enabled, this user should not be deleted yet either. Even with ephemeral users exposed as a consumer setting, this code should never run while a user is logged in, except for very rare corner cases. The check is mostly a safety net. That said, I agree with Ivan that it is actually safe to delete a logged-in user. In *cryptohomed*, a logged-in user's home directory cannot immediately be removed. In the user manager, it is (mostly) safe. The only risk I can see is that if a custom image download is currently in progress for the user, this image will not be deleted. This is such an extremely rare corner case that I guess it is OK to ignore it and just do away with the check for the logged-in user altogether. https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... File chrome/browser/chromeos/login/user_manager.h (right): https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.h:318: // Cached flag of whether the currently logged-in user is ephemeral. On 2012/03/02 12:51:42, Nikita Kostylev wrote: > Please expand this comment on what "currently logged-in user is ephemeral" > actually means. Otherwise this knowledge is not concentrated in one place. Done. https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.h:326: scoped_ptr<bool> ephemeral_users_enabled_; I changed it so that ephemeral_users_enabled_ is a plain bool defaulting to false and owner_email_ is a plain std::string defaulting to "". With this change, it is impossible to tell whether the ephemeral users setting is explicitly set to false or has not been retrieved yet. Similarly, it is impossible to tell whether the owner is "" (device enterprise owned) or has not been retrieved yet. This is OK for the specific case of ephemeral users. The previous code was more general and would have worked for other settings where this distinction actually matters. https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager.h:330: scoped_ptr<std::string> owner_; On 2012/03/01 20:16:30, Ivan Korotkov wrote: > Please, rename this to owner_email_ as owner_ suggests that it is a User > instance. Done. https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... File chrome/browser/chromeos/login/user_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/chro... chrome/browser/chromeos/login/user_manager_unittest.cc:98: stub_settings_provider_.Set(kDeviceOwner, base::StringValue(owner)); On 2012/03/01 20:16:30, Ivan Korotkov wrote: > I'm not sure if clang is gonna like this. Previously it required you to write > base::StringValue smth(...); and then pass smth as argument. Done. https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/reso... File chrome/browser/resources/options2/browser_options.js (right): https://chromiumcodereview.appspot.com/9405035/diff/25001/chrome/browser/reso... chrome/browser/resources/options2/browser_options.js:201: // ephemeral. On 2012/03/01 20:16:30, Ivan Korotkov wrote: > #change-picture-caption should be made invisible as well. Done.
Note that I have 2 big refactoring CLs touching UserManager. Depending on the timing one of us would merge. http://codereview.chromium.org/9348022/ http://codereview.chromium.org/9355059/
Having had a quick glance at yours CLs, it looks like they do not change much/any logic but mostly rename and clean up things. Our changes should be mostly orthogonal to each other and fairly easy to merge (I hope). - Bartosz
First CL landed https://src.chromium.org/viewvc/chrome?view=rev&revision=125165
LGTM, sorry for a slow review. http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/user_manager.cc (right): http://codereview.chromium.org/9405035/diff/17001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:987: if (IsEphemeralUser(username)) On 2012/03/05 18:07:32, bartfab wrote: > Done. I do not understand how this makes sense though. SaveUserImageInternal() > now sets the image index to either kExternalImageIndex or kProfileImageIndex but > then returns before creating an image file. So you have an index implying an > external image which does not actually exist. Would it not be better to move the > IsEphemeralUser() check to the top of the function so that the index is not set > in the first place? The point is that the picture should be still set in memory (so that it will be displayed on the lock screen/prefs page).
Second CL landed. http://src.chromium.org/viewvc/chrome?view=rev&revision=125230
http://codereview.chromium.org/9405035/diff/30001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/user_manager.cc (right): http://codereview.chromium.org/9405035/diff/30001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:789: owner_email_ = ""; You could always call cros_settings->GetString(kDeviceOwner, &owner_email_); In case of enterprise managed device it will return empty string. http://codereview.chromium.org/9405035/diff/30001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/user_manager.h (right): http://codereview.chromium.org/9405035/diff/30001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.h:148: bool current_user_is_ephemeral() const { needs merge: Rename to IsCurrentUserEphemeral(). Add to the UserManager interface and mock. http://codereview.chromium.org/9405035/diff/30001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.h:325: bool current_user_is_ephemeral_; nit: Should follow naming that other members in UserManagerImpl use. is_current_user_ephemeral_ http://codereview.chromium.org/9405035/diff/30001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.h:336: std::string owner_email_; We already have cached versions of trusted settings available. Why do we need to cache ephemeral_users_enabled_, owner_email_ in one more place? http://codereview.chromium.org/9405035/diff/30001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/wizard_controller.cc (right): http://codereview.chromium.org/9405035/diff/30001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/wizard_controller.cc:267: if (chromeos::UserManager::Get()->current_user_is_ephemeral()) { I think we should alter UX for these users as less as possible. They are the same as normal users but all their data is removed on sign out. Not sure with this one. Was this decided? http://codereview.chromium.org/9405035/diff/30001/chrome/browser/resources/op... File chrome/browser/resources/options/personal_options.js (right): http://codereview.chromium.org/9405035/diff/30001/chrome/browser/resources/op... chrome/browser/resources/options/personal_options.js:125: $('change-picture-button').disabled = true; Same here, please enable back. We should not disable functionality. Users may want to test all features of Chrome OS. Changing picture is one of them. Code in UserManager makes sure images wouldn't be saved to LocalState/disk but they'll be in memory. http://codereview.chromium.org/9405035/diff/30001/chrome/browser/resources/op... File chrome/browser/resources/options2/browser_options.js (right): http://codereview.chromium.org/9405035/diff/30001/chrome/browser/resources/op... chrome/browser/resources/options2/browser_options.js:216: // Hide picture changing option for ephemeral users. Please remove this code. Already taken care for Guest mode in https://chromiumcodereview.appspot.com/9464053/ Ephemeral users should be able to change picture and enable screen lock. While they are inside their user session they are normal users.
Comments addressed. http://codereview.chromium.org/9405035/diff/30001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/user_manager.cc (right): http://codereview.chromium.org/9405035/diff/30001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.cc:789: owner_email_ = ""; On 2012/03/07 10:03:04, Nikita Kostylev wrote: > You could always call > cros_settings->GetString(kDeviceOwner, &owner_email_); > > In case of enterprise managed device it will return empty string. Done. http://codereview.chromium.org/9405035/diff/30001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/user_manager.h (right): http://codereview.chromium.org/9405035/diff/30001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.h:148: bool current_user_is_ephemeral() const { After addressing your other comments, the method is actually gone completely now. http://codereview.chromium.org/9405035/diff/30001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.h:325: bool current_user_is_ephemeral_; On 2012/03/07 10:03:04, Nikita Kostylev wrote: > nit: Should follow naming that other members in UserManagerImpl use. > is_current_user_ephemeral_ Done. http://codereview.chromium.org/9405035/diff/30001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/user_manager.h:336: std::string owner_email_; Before accessing cached trusted settings, GetTrusted() must be called to check whether they are available. If the settings are not yet available, this sets up a callback. Every access to cached trusted settings can therefore lead to an additional callback being set up. Caching the settings locally allows a single well-defined callback to be used for their retrieval. http://codereview.chromium.org/9405035/diff/30001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/wizard_controller.cc (right): http://codereview.chromium.org/9405035/diff/30001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/wizard_controller.cc:267: if (chromeos::UserManager::Get()->current_user_is_ephemeral()) { I made the change as requested but I think skipping image selection is the better UX: Setting an image is a once-off necessity for regular users. Should ephemeral users not be presented with the most common UX path, i.e. that of a returning regular Chrome OS user? http://codereview.chromium.org/9405035/diff/30001/chrome/browser/resources/op... File chrome/browser/resources/options/personal_options.js (right): http://codereview.chromium.org/9405035/diff/30001/chrome/browser/resources/op... chrome/browser/resources/options/personal_options.js:125: $('change-picture-button').disabled = true; On 2012/03/07 10:03:04, Nikita Kostylev wrote: > Same here, please enable back. We should not disable functionality. Users may > want to test all features of Chrome OS. > > Changing picture is one of them. Code in UserManager makes sure images wouldn't > be saved to LocalState/disk but they'll be in memory. Done. http://codereview.chromium.org/9405035/diff/30001/chrome/browser/resources/op... File chrome/browser/resources/options2/browser_options.js (right): http://codereview.chromium.org/9405035/diff/30001/chrome/browser/resources/op... chrome/browser/resources/options2/browser_options.js:216: // Hide picture changing option for ephemeral users. On 2012/03/07 10:03:04, Nikita Kostylev wrote: > Please remove this code. Already taken care for Guest mode in > https://chromiumcodereview.appspot.com/9464053/ > > Ephemeral users should be able to change picture and enable screen lock. While > they are inside their user session they are normal users. Done.
One more comment regarding image selection (or lack thereof) on login: This entire feature was envisaged as part of demo mode. However, it will *not* actually be used there (demo mode has its own custom login screen and uses a cryptohome guest mount, bypassing all of this code entirely). Ephemeral users are a feature for enterprise (and, hopefully, some day also consumer) devices. The use case is now that I have a device that I let people with existing accounts log into temporarily, allowing them to swap devices at will without leaving cruft behind. This is specifically not a situation where we are showcasing Chrome OS to new users. It is a situation where regular Chrome OS users temporarily log into a device. Presenting them with the image selection screen is just an annoyance. How about we skip the image selection on login but still leave the option enabled to change the image afterwards if the user feels like playing with it?
On 2012/03/07 11:26:22, bartfab wrote: > One more comment regarding image selection (or lack thereof) on login: > > This entire feature was envisaged as part of demo mode. However, it will *not* > actually be used there (demo mode has its own custom login screen and uses a > cryptohome guest mount, bypassing all of this code entirely). > > Ephemeral users are a feature for enterprise (and, hopefully, some day also > consumer) devices. The use case is now that I have a device that I let people > with existing accounts log into temporarily, allowing them to swap devices at > will without leaving cruft behind. This is specifically not a situation where we > are showcasing Chrome OS to new users. It is a situation where regular Chrome OS > users temporarily log into a device. Presenting them with the image selection > screen is just an annoyance. > > How about we skip the image selection on login but still leave the option > enabled to change the image afterwards if the user feels like playing with it? That sounds reasonable. These users may use ChromeOS on a Chromebook during flight so they might want to play with all features.
Hi Nikita, As discussed, I removed initial profile image selection. Users are still able to change their profile image after login, allowing them to explore the entire Chrome OS experience. Could you please review once more?
lgtm thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartfab@google.com/9405035/42001
Change committed as 126198
This broke compile on Linux Chromium (clang), so was reverted. http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...
s/Chromium/ChromiumOS
Message was sent while issue was closed.
Hi Bartosz! While investigating one bug got question related to this CL. FYI the bug is the one we discussed: oddities on preference change at options page. https://codereview.chromium.org/9405035/diff/42001/chrome/browser/chromeos/lo... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://codereview.chromium.org/9405035/diff/42001/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/user_manager_impl.cc:775: owner_email_ = ""; I wonder why we need this string? We can initialize owner_email in constructor instead if it's an initialization. Also why we generally need this cache? Currently it's not possible that the owner will be changed 'on-the-flight'.
Message was sent while issue was closed.
https://codereview.chromium.org/9405035/diff/42001/chrome/browser/chromeos/lo... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://codereview.chromium.org/9405035/diff/42001/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/user_manager_impl.cc:775: owner_email_ = ""; On 2014/04/08 10:34:59, merkulova wrote: > I wonder why we need this string? > We can initialize owner_email in constructor instead if it's an initialization. > > Also why we generally need this cache? Currently it's not possible that the > owner will be changed 'on-the-flight'. Sorry for the late reply. I will summarize here what we discussed offline: 1) Ownership currently cannot go from set to unset. But it can go the other way: It is possible that there is no owner at construction time but a user becomes the owner later. 2) This method retrieves owner information from trusted (signed and verified) device settings. Even if owner information were available during construction, verifying the signature takes time. Thus, trusted owner information will only ever become available after a delay. 3) Caching is important for two reasons: One, loading the information from disk and verifying the signature every time would be extremely wasteful, it would take time and resources. Two, the trusted status can go backwards in some rare cases. So trusted owner information may be available at some point and can then go away. If it is cached, the UserManager will still remember who the owner is.
Message was sent while issue was closed.
On 2014/04/14 11:36:41, bartfab wrote: > https://codereview.chromium.org/9405035/diff/42001/chrome/browser/chromeos/lo... > File chrome/browser/chromeos/login/user_manager_impl.cc (right): > > https://codereview.chromium.org/9405035/diff/42001/chrome/browser/chromeos/lo... > chrome/browser/chromeos/login/user_manager_impl.cc:775: owner_email_ = ""; > On 2014/04/08 10:34:59, merkulova wrote: > > I wonder why we need this string? > > We can initialize owner_email in constructor instead if it's an > initialization. > > > > Also why we generally need this cache? Currently it's not possible that the > > owner will be changed 'on-the-flight'. > > Sorry for the late reply. I will summarize here what we discussed offline: > > 1) Ownership currently cannot go from set to unset. But it can go the other way: > It is possible that there is no owner at construction time but a user becomes > the owner later. > > 2) This method retrieves owner information from trusted (signed and verified) > device settings. Even if owner information were available during construction, > verifying the signature takes time. Thus, trusted owner information will only > ever become available after a delay. > > 3) Caching is important for two reasons: One, loading the information from disk > and verifying the signature every time would be extremely wasteful, it would > take time and resources. Two, the trusted status can go backwards in some rare > cases. So trusted owner information may be available at some point and can then > go away. If it is cached, the UserManager will still remember who the owner is. My proposal was to eliminate owner_email_ cache in UserManager completely and rely on CrosSettings cache. Otherwise we now have a situation when you update one signed setting and owner_email_ is set to empty string in UserManager. This caused a bug since other callers led to believe that current user is not an owner during that process. Bartosz, do you see any issues with that?
Message was sent while issue was closed.
On 2014/04/14 11:56:05, Nikita Kostylev wrote: > Otherwise we now have a situation when you update one signed setting and > owner_email_ is set to empty string in UserManager. Which breaks an important assumption: Ownership currently cannot go from set to unset.
Message was sent while issue was closed.
On 2014/04/14 11:56:56, Nikita Kostylev wrote: > On 2014/04/14 11:56:05, Nikita Kostylev wrote: > > Otherwise we now have a situation when you update one signed setting and > > owner_email_ is set to empty string in UserManager. > > Which breaks an important assumption: > Ownership currently cannot go from set to unset. No, ownership still does not go from set to unset. Only the trusted/signed status can go backwards. IIRC, this happens at some point during enrollment.
Message was sent while issue was closed.
On 2014/04/14 11:56:05, Nikita Kostylev wrote: > On 2014/04/14 11:36:41, bartfab wrote: > > > https://codereview.chromium.org/9405035/diff/42001/chrome/browser/chromeos/lo... > > File chrome/browser/chromeos/login/user_manager_impl.cc (right): > > > > > https://codereview.chromium.org/9405035/diff/42001/chrome/browser/chromeos/lo... > > chrome/browser/chromeos/login/user_manager_impl.cc:775: owner_email_ = ""; > > On 2014/04/08 10:34:59, merkulova wrote: > > > I wonder why we need this string? > > > We can initialize owner_email in constructor instead if it's an > > initialization. > > > > > > Also why we generally need this cache? Currently it's not possible that the > > > owner will be changed 'on-the-flight'. > > > > Sorry for the late reply. I will summarize here what we discussed offline: > > > > 1) Ownership currently cannot go from set to unset. But it can go the other > way: > > It is possible that there is no owner at construction time but a user becomes > > the owner later. > > > > 2) This method retrieves owner information from trusted (signed and verified) > > device settings. Even if owner information were available during construction, > > verifying the signature takes time. Thus, trusted owner information will only > > ever become available after a delay. > > > > 3) Caching is important for two reasons: One, loading the information from > disk > > and verifying the signature every time would be extremely wasteful, it would > > take time and resources. Two, the trusted status can go backwards in some rare > > cases. So trusted owner information may be available at some point and can > then > > go away. If it is cached, the UserManager will still remember who the owner > is. > > My proposal was to eliminate owner_email_ cache in UserManager completely and > rely on CrosSettings cache. > Otherwise we now have a situation when you update one signed setting and > owner_email_ is set to empty string in UserManager. > This caused a bug since other callers led to believe that current user is not an > owner during that process. > > Bartosz, do you see any issues with that? In general, I see no problem with retrieving the owner's e-mail address from CrosSettings directly. But this would have to be done via PrepareTrustedValues, which is asynchronous, so there would be no more easy, synchronous way to grab the owner's e-mail address and do something with it.
Message was sent while issue was closed.
On 2014/04/14 12:05:56, bartfab wrote: > On 2014/04/14 11:56:56, Nikita Kostylev wrote: > > On 2014/04/14 11:56:05, Nikita Kostylev wrote: > > > Otherwise we now have a situation when you update one signed setting and > > > owner_email_ is set to empty string in UserManager. > > > > Which breaks an important assumption: > > Ownership currently cannot go from set to unset. > > No, ownership still does not go from set to unset. Only the trusted/signed > status can go backwards. IIRC, this happens at some point during enrollment. It does if one checks it by ProfileHelper::IsOwnerProfile() which in turn uses UserManager::GetOwnerEmail(). And owner_email_ will be set to empty string if policy signing is in progress.
Message was sent while issue was closed.
On 2014/04/14 12:08:04, bartfab wrote: > On 2014/04/14 11:56:05, Nikita Kostylev wrote: > > On 2014/04/14 11:36:41, bartfab wrote: > > > > > > https://codereview.chromium.org/9405035/diff/42001/chrome/browser/chromeos/lo... > > > File chrome/browser/chromeos/login/user_manager_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/9405035/diff/42001/chrome/browser/chromeos/lo... > > > chrome/browser/chromeos/login/user_manager_impl.cc:775: owner_email_ = ""; > > > On 2014/04/08 10:34:59, merkulova wrote: > > > > I wonder why we need this string? > > > > We can initialize owner_email in constructor instead if it's an > > > initialization. > > > > > > > > Also why we generally need this cache? Currently it's not possible that > the > > > > owner will be changed 'on-the-flight'. > > > > > > Sorry for the late reply. I will summarize here what we discussed offline: > > > > > > 1) Ownership currently cannot go from set to unset. But it can go the other > > way: > > > It is possible that there is no owner at construction time but a user > becomes > > > the owner later. > > > > > > 2) This method retrieves owner information from trusted (signed and > verified) > > > device settings. Even if owner information were available during > construction, > > > verifying the signature takes time. Thus, trusted owner information will > only > > > ever become available after a delay. > > > > > > 3) Caching is important for two reasons: One, loading the information from > > disk > > > and verifying the signature every time would be extremely wasteful, it would > > > take time and resources. Two, the trusted status can go backwards in some > rare > > > cases. So trusted owner information may be available at some point and can > > then > > > go away. If it is cached, the UserManager will still remember who the owner > > is. > > > > My proposal was to eliminate owner_email_ cache in UserManager completely and > > rely on CrosSettings cache. > > Otherwise we now have a situation when you update one signed setting and > > owner_email_ is set to empty string in UserManager. > > This caused a bug since other callers led to believe that current user is not > an > > owner during that process. > > > > Bartosz, do you see any issues with that? > > In general, I see no problem with retrieving the owner's e-mail address from > CrosSettings directly. But this would have to be done via PrepareTrustedValues, > which is asynchronous, so there would be no more easy, synchronous way to grab > the owner's e-mail address and do something with it. When such retrieval is done during the policy signing is in progress and only for UI elements it is fine to use just. cros_settings_->GetString(kDeviceOwner, &owner_email); Since we can use the fact that if kDeviceOwner is not empty string it will most likely stay the same. And we're just using this value for UI. Backend check will enforce trusted value.
Message was sent while issue was closed.
On 2014/04/14 17:00:42, Nikita Kostylev wrote: > On 2014/04/14 12:08:04, bartfab wrote: > > On 2014/04/14 11:56:05, Nikita Kostylev wrote: > > > On 2014/04/14 11:36:41, bartfab wrote: > > > > > > > > > > https://codereview.chromium.org/9405035/diff/42001/chrome/browser/chromeos/lo... > > > > File chrome/browser/chromeos/login/user_manager_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/9405035/diff/42001/chrome/browser/chromeos/lo... > > > > chrome/browser/chromeos/login/user_manager_impl.cc:775: owner_email_ = ""; > > > > On 2014/04/08 10:34:59, merkulova wrote: > > > > > I wonder why we need this string? > > > > > We can initialize owner_email in constructor instead if it's an > > > > initialization. > > > > > > > > > > Also why we generally need this cache? Currently it's not possible that > > the > > > > > owner will be changed 'on-the-flight'. > > > > > > > > Sorry for the late reply. I will summarize here what we discussed offline: > > > > > > > > 1) Ownership currently cannot go from set to unset. But it can go the > other > > > way: > > > > It is possible that there is no owner at construction time but a user > > becomes > > > > the owner later. > > > > > > > > 2) This method retrieves owner information from trusted (signed and > > verified) > > > > device settings. Even if owner information were available during > > construction, > > > > verifying the signature takes time. Thus, trusted owner information will > > only > > > > ever become available after a delay. > > > > > > > > 3) Caching is important for two reasons: One, loading the information from > > > disk > > > > and verifying the signature every time would be extremely wasteful, it > would > > > > take time and resources. Two, the trusted status can go backwards in some > > rare > > > > cases. So trusted owner information may be available at some point and can > > > then > > > > go away. If it is cached, the UserManager will still remember who the > owner > > > is. > > > > > > My proposal was to eliminate owner_email_ cache in UserManager completely > and > > > rely on CrosSettings cache. > > > Otherwise we now have a situation when you update one signed setting and > > > owner_email_ is set to empty string in UserManager. > > > This caused a bug since other callers led to believe that current user is > not > > an > > > owner during that process. > > > > > > Bartosz, do you see any issues with that? > > > > In general, I see no problem with retrieving the owner's e-mail address from > > CrosSettings directly. But this would have to be done via > PrepareTrustedValues, > > which is asynchronous, so there would be no more easy, synchronous way to grab > > the owner's e-mail address and do something with it. > > When such retrieval is done during the policy signing is in progress and only > for UI elements it is fine to use > just. > cros_settings_->GetString(kDeviceOwner, &owner_email); > > Since we can use the fact that if kDeviceOwner is not empty string it will most > likely stay the same. > And we're just using this value for UI. Backend check will enforce trusted > value. I talked to Julian who implemented the signature checking back in the day. Originally, the trusted status could never go from "permanently untrusted" to "trusted". Something regressed at some point and this is possible now. That regression is part of what is making things complicated. I agree that purely for display purposes, cros_settings_->GetString(kDeviceOwner, &owner_email) without any signature checks should be fine. But the owner_email_ here is used for other things as well, where having an untrusted value is a bad idea.
Message was sent while issue was closed.
On 2014/04/15 13:52:15, bartfab wrote: > I talked to Julian who implemented the signature checking back in the day. > Originally, the trusted status could never go from "permanently untrusted" to > "trusted". Something regressed at some point and this is possible now. That > regression is part of what is making things complicated. I don't think this was the case that we've seen here. > > I agree that purely for display purposes, > cros_settings_->GetString(kDeviceOwner, &owner_email) without any signature > checks should be fine. But the owner_email_ here is used for other things as > well, where having an untrusted value is a bad idea. Even for an owner value? As we know once set it is not changed.
Message was sent while issue was closed.
On 2014/04/15 14:46:53, Nikita Kostylev wrote: > On 2014/04/15 13:52:15, bartfab wrote: > > I talked to Julian who implemented the signature checking back in the day. > > Originally, the trusted status could never go from "permanently untrusted" to > > "trusted". Something regressed at some point and this is possible now. That > > regression is part of what is making things complicated. > > I don't think this was the case that we've seen here. The status is always allowed to go from trusted to temporarily untrusted and back. This is probably what you are observing. That is all within spec. It means that trusted value may be inaccessible for some amount of time though, which makes it necessary to either cache it or to wait for the callback that trusted values are available again. > > > > > I agree that purely for display purposes, > > cros_settings_->GetString(kDeviceOwner, &owner_email) without any signature > > checks should be fine. But the owner_email_ here is used for other things as > > well, where having an untrusted value is a bad idea. > > Even for an owner value? As we know once set it is not changed. Actually, the consumer management team is planning to add some kind of ownership transfer and unenrollment/re-enrollment. I think it is safer to only ever access signed CrosSettings to stay on the safe side.
Message was sent while issue was closed.
On 2014/04/15 14:59:46, bartfab wrote: > On 2014/04/15 14:46:53, Nikita Kostylev wrote: > > On 2014/04/15 13:52:15, bartfab wrote: > > > I talked to Julian who implemented the signature checking back in the day. > > > Originally, the trusted status could never go from "permanently untrusted" > to > > > "trusted". Something regressed at some point and this is possible now. That > > > regression is part of what is making things complicated. > > > > I don't think this was the case that we've seen here. > > The status is always allowed to go from trusted to temporarily untrusted and > back. This is probably what you are observing. That is all within spec. It means > that trusted value may be inaccessible for some amount of time though, which > makes it necessary to either cache it or to wait for the callback that trusted > values are available again. > > > > > > > > > I agree that purely for display purposes, > > > cros_settings_->GetString(kDeviceOwner, &owner_email) without any signature > > > checks should be fine. But the owner_email_ here is used for other things as > > > well, where having an untrusted value is a bad idea. > > > > Even for an owner value? As we know once set it is not changed. > > Actually, the consumer management team is planning to add some kind of ownership > transfer and unenrollment/re-enrollment. I think it is safer to only ever access > signed CrosSettings to stay on the safe side. But what I'm trying to say here is that there was a bug: 0. UserManager was subscribed to monitor supervised users enabled pref updates. 1. In consumer mode when owner unchecked this checkbox 1.1 UserManager::ReloadDevicePolicies is called 1.2 owner_email_ is reset 1.3 Settings are in process of signing in 1.4 chrome://settings handler got called and checks whether current user is owner (he still is!) by ProfileHelper::IsOwnerProfile() which callse UserManager::GetOwnerEmail() So this returns empty string, current user is not considered as being an owner (!) 1.5 Supervised users enabled preference checkbox goes into permanent "grayed out" state So the fix was that UserManager is no longer subscribed to supervised users enabled pref (there's no real need). https://codereview.chromium.org/228553002/ We've saw that originally ReloadDevicePolicies was called from UserManager ctor so setting owner_email_ to empty string was fine. Then later every observer of different device settings in UserManager started to call ReloadDevicePolicies so this resulted in a side effects as I've described above. So while I understand that in the future owner_email_ could be reset to empty string that's not the case now and it brings a real bugs. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
