Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
985093002:
Fix up Owner settings on first load (Closed)
|
Created:
5 years, 9 months ago by Chris Masone Modified:
5 years, 9 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix up Owner settings on first load
The session_manager has been mitigating some potential Owner settings
edge cases for quite some time, mostly around the user whitelist being
enabled without the owner on it! While these should all be gone, very
old devices being used for the first time could still run into
this. That said, Chrome's in a better position to handle this issue,
so let's move the functionality here.
BUG=chromium:422046
TEST=unit
Committed: https://crrev.com/b6ff3ecba75d2d012785e77f00f686e6849c2012
Cr-Commit-Position: refs/heads/master@{#322402}
Patch Set 1 #Patch Set 2 : based off mattias' patch that injects CrosSettings for testing #
Total comments: 10
Patch Set 3 : modify protobuf directly #
Total comments: 12
Patch Set 4 : Added test to see that changes take effect on ownership. #
Total comments: 1
Patch Set 5 : Fixed Nit #
Total comments: 1
Patch Set 6 : has_pending_changes -> HasPendingChanges #
Messages
Total messages: 22 (5 generated)
cmasone@chromium.org changed reviewers: + mnissler@chromium.org
this is based off your hack. How bad does it look to you?
This doesn't make things worse then they were before, so if you want to land it in this shape, I'm fine. https://codereview.chromium.org/985093002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc (right): https://codereview.chromium.org/985093002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:275: UpdateDeviceSettings(setting, value, settings); nit: blank line here https://codereview.chromium.org/985093002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:280: // We can enforce the first two here, but need to check the whitelist before I don't get why we can't update the whitelist here? https://codereview.chromium.org/985093002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:707: cros_settings_->AppendToList(kAccountsPrefUsers, This is a bit of a step backwards, as the write path for owner settings is supposed to move away from CrosSettings. https://codereview.chromium.org/985093002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/settings/device_settings_test_helper.h (right): https://codereview.chromium.org/985093002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/settings/device_settings_test_helper.h:192: CrosSettings cros_settings_; Sigh, all this cruft in here :) Not your fault.
https://codereview.chromium.org/985093002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc (right): https://codereview.chromium.org/985093002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:280: // We can enforce the first two here, but need to check the whitelist before On 2015/03/16 08:51:42, Mattias Nissler wrote: > I don't get why we can't update the whitelist here? I don't think we're actually assured that we've loaded the policy from the session_manager yet at this point. So if there's already a whitelist in play, if we just set it here, we might clobber it. No? That might allow to the allow_new_users setting as well, huh? https://codereview.chromium.org/985093002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:707: cros_settings_->AppendToList(kAccountsPrefUsers, On 2015/03/16 08:51:42, Mattias Nissler wrote: > This is a bit of a step backwards, as the write path for owner settings is > supposed to move away from CrosSettings. Ah. How's it supposed to work, then?
https://codereview.chromium.org/985093002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc (right): https://codereview.chromium.org/985093002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:280: // We can enforce the first two here, but need to check the whitelist before On 2015/03/16 16:45:57, Chris Masone wrote: > On 2015/03/16 08:51:42, Mattias Nissler wrote: > > I don't get why we can't update the whitelist here? > > I don't think we're actually assured that we've loaded the policy from the > session_manager yet at this point. So if there's already a whitelist in play, if > we just set it here, we might clobber it. No? That might allow to the > allow_new_users setting as well, huh? Ah, right, the |settings| may have come from elsewhere. However, that's only true for the first write that uses |tentative_settings| accumulated before we had a device owner. device_settings_service_->device_settings() as read in line 273 is guaranteed to be a validated settings block coming from session_manager though. Thus, IMHO we should be fine. One thing to consider however is the pending_changes_ mechanism. It may revert any sanitization you apply here. Hence, it might be better to apply your sanitization in StorePendingChanges() before (or within) the call to AssemblePolicy, which is used to produce the settings blob that gets passed to session_manager for writing. https://codereview.chromium.org/985093002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:707: cros_settings_->AppendToList(kAccountsPrefUsers, On 2015/03/16 16:45:57, Chris Masone wrote: > On 2015/03/16 08:51:42, Mattias Nissler wrote: > > This is a bit of a step backwards, as the write path for owner settings is > > supposed to move away from CrosSettings. > > Ah. How's it supposed to work, then? You could just update the protobuf field, i.e. |settings| in StorePendingChanges().
https://codereview.chromium.org/985093002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc (right): https://codereview.chromium.org/985093002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:280: // We can enforce the first two here, but need to check the whitelist before On 2015/03/17 14:05:53, Mattias Nissler wrote: > On 2015/03/16 16:45:57, Chris Masone wrote: > > On 2015/03/16 08:51:42, Mattias Nissler wrote: > > > I don't get why we can't update the whitelist here? > > > > I don't think we're actually assured that we've loaded the policy from the > > session_manager yet at this point. So if there's already a whitelist in play, > if > > we just set it here, we might clobber it. No? That might allow to the > > allow_new_users setting as well, huh? > > Ah, right, the |settings| may have come from elsewhere. However, that's only > true for the first write that uses |tentative_settings| accumulated before we > had a device owner. device_settings_service_->device_settings() as read in line > 273 is guaranteed to be a validated settings block coming from session_manager > though. Thus, IMHO we should be fine. > > One thing to consider however is the pending_changes_ mechanism. It may revert > any sanitization you apply here. Hence, it might be better to apply your > sanitization in StorePendingChanges() before (or within) the call to > AssemblePolicy, which is used to produce the settings blob that gets passed to > session_manager for writing. Yeah, the pending_changes_ stuff is why I did this how I did it. I figured by going through (what seemed to be) the normal path, I'd just stack on top of any pending changes and everything would be fine. If I munge the protobuf directly in StorePendingChanges or AssemblePolicy (which is actually what I was doing initially) my concern is that I might not get called at all unless someone makes a Set() call somewhere. I need to force a no-op change on key-load to ensure that the fixups get applied, something that will actually wind up in pending_changes_ otherwise the code will bail early somewhere.
https://codereview.chromium.org/985093002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc (right): https://codereview.chromium.org/985093002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:280: // We can enforce the first two here, but need to check the whitelist before On 2015/03/17 15:00:48, Chris Masone wrote: > On 2015/03/17 14:05:53, Mattias Nissler wrote: > > On 2015/03/16 16:45:57, Chris Masone wrote: > > > On 2015/03/16 08:51:42, Mattias Nissler wrote: > > > > I don't get why we can't update the whitelist here? > > > > > > I don't think we're actually assured that we've loaded the policy from the > > > session_manager yet at this point. So if there's already a whitelist in > play, > > if > > > we just set it here, we might clobber it. No? That might allow to the > > > allow_new_users setting as well, huh? > > > > Ah, right, the |settings| may have come from elsewhere. However, that's only > > true for the first write that uses |tentative_settings| accumulated before we > > had a device owner. device_settings_service_->device_settings() as read in > line > > 273 is guaranteed to be a validated settings block coming from session_manager > > though. Thus, IMHO we should be fine. > > > > One thing to consider however is the pending_changes_ mechanism. It may revert > > any sanitization you apply here. Hence, it might be better to apply your > > sanitization in StorePendingChanges() before (or within) the call to > > AssemblePolicy, which is used to produce the settings blob that gets passed to > > session_manager for writing. > > Yeah, the pending_changes_ stuff is why I did this how I did it. I figured by > going through (what seemed to be) the normal path, I'd just stack on top of any > pending changes and everything would be fine. > > If I munge the protobuf directly in StorePendingChanges or AssemblePolicy (which > is actually what I was doing initially) my concern is that I might not get > called at all unless someone makes a Set() call somewhere. I need to force a > no-op change on key-load to ensure that the fixups get applied, something that > will actually wind up in pending_changes_ otherwise the code will bail early > somewhere. Well, I think it's fair to just force an initial write. If I'm not mistaken, CommitTentativeDeviceSettings gets called unconditionally after establishing ownership anyways, and so you could just change StorePendingChanges() to always continue if |tentative_changes_| is non-null.
Eh? How does this look?
Removing dependencies on singletons FTW! This looks rather good now. https://codereview.chromium.org/985093002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc (right): https://codereview.chromium.org/985093002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:436: // modifying it, so that will be taken care of in a separate class. stale comment https://codereview.chromium.org/985093002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:437: if (policy->management_mode() == em::PolicyData::LOCAL_OWNER) I think you want to do this both for LOCAL_OWNER and CONSUMER_MANAGED, as the owner key is controlled by a local account in both cases. https://codereview.chromium.org/985093002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:758: has_pending_management_settings_ = false; I'd put the call to FixupLocalOwnerPolicy here, because the management_mode updating code above may change the mode, and you only want to handle the local ownership case. Then again, management mode changes should never cause a transition that changes your decision, but it still seems cleaner to put the code here. https://codereview.chromium.org/985093002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc (right): https://codereview.chromium.org/985093002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc:299: bool FindInListValue(const std::string& needle, const base::Value* haystack) { int: We'd usually put these helpers in the anonymous namespace at the top of the file. https://codereview.chromium.org/985093002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc:314: } Would be good to have an additional test case that verifies the fixups also take place immediately after ownership gets established.
https://codereview.chromium.org/985093002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc (right): https://codereview.chromium.org/985093002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc:314: } On 2015/03/18 08:36:42, Mattias Nissler wrote: > Would be good to have an additional test case that verifies the fixups also take > place immediately after ownership gets established. ANy pointers on how to drive that transition in a test?
https://codereview.chromium.org/985093002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc (right): https://codereview.chromium.org/985093002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc:314: } On 2015/03/20 19:13:11, Chris Masone wrote: > On 2015/03/18 08:36:42, Mattias Nissler wrote: > > Would be good to have an additional test case that verifies the fixups also > take > > place immediately after ownership gets established. > > ANy pointers on how to drive that transition in a test? OwnerSettingsServiceChromeOSTest::Setup() above calls SetPrivateKey() and InitOwner() which trigger the transition. There's a OwnerSettingsServiceChromeOSNoOwnerTest which starts out with no device owner. You could have your test replicate what the Setup() code does above to trigger the transition.
https://codereview.chromium.org/985093002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc (right): https://codereview.chromium.org/985093002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:436: // modifying it, so that will be taken care of in a separate class. On 2015/03/18 08:36:42, Mattias Nissler wrote: > stale comment Done. https://codereview.chromium.org/985093002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:437: if (policy->management_mode() == em::PolicyData::LOCAL_OWNER) On 2015/03/18 08:36:41, Mattias Nissler wrote: > I think you want to do this both for LOCAL_OWNER and CONSUMER_MANAGED, as the > owner key is controlled by a local account in both cases. Done. https://codereview.chromium.org/985093002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:437: if (policy->management_mode() == em::PolicyData::LOCAL_OWNER) On 2015/03/18 08:36:41, Mattias Nissler wrote: > I think you want to do this both for LOCAL_OWNER and CONSUMER_MANAGED, as the > owner key is controlled by a local account in both cases. Done. https://codereview.chromium.org/985093002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:758: has_pending_management_settings_ = false; On 2015/03/18 08:36:41, Mattias Nissler wrote: > I'd put the call to FixupLocalOwnerPolicy here, because the management_mode > updating code above may change the mode, and you only want to handle the local > ownership case. Then again, management mode changes should never cause a > transition that changes your decision, but it still seems cleaner to put the > code here. By this point, the policy is already assembled, so my changes to |settings| are ignored. I've moved all these management settings and such into AssemblePolicy, along with my own changes to the policy. This way it all happens in one place. https://codereview.chromium.org/985093002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc (right): https://codereview.chromium.org/985093002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc:299: bool FindInListValue(const std::string& needle, const base::Value* haystack) { On 2015/03/18 08:36:42, Mattias Nissler wrote: > int: We'd usually put these helpers in the anonymous namespace at the top of the > file. Done.
This LGTM pending green tryjobs (please trigger the default jobs using git cl try). There's a chance that the new behavior breaks expectation checking of some tests... easiest way to find out is via the bots. https://codereview.chromium.org/985093002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc (right): https://codereview.chromium.org/985093002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc:86: } nit: newline here
cmasone@chromium.org changed reviewers: + stevenjb@chromium.org
cmasone@chromium.org changed required reviewers: + stevenjb@chromium.org
Hey, Steven. Can I get an OWNERs review from you?
owner lgtm https://codereview.chromium.org/985093002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h (right): https://codereview.chromium.org/985093002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h:118: bool has_pending_changes() const { optional nit: This should really be named something like HasPendingChanges since it doesn't just return a value.
The CQ bit was checked by cmasone@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mnissler@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/985093002/#ps100001 (title: "has_pending_changes -> HasPendingChanges")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/985093002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b6ff3ecba75d2d012785e77f00f686e6849c2012 Cr-Commit-Position: refs/heads/master@{#322402} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
