cros: Fix flaky owner detection

cros: Fix flaky owner detection

When owner key is generated for a consumer owner, OwnerKeySet is
called on both DeviceSettingsService and OwnerSettingsServiceChromeOS.
Code that watches for ownership signal from DeviceSettingsService
(either as an observer or via NOTIFICATION_OWNERSHIP_STATUS_CHANGED)
also expects that the private part of the owner is available at the
time the signal is generated. However, the private key loading
in OwnerSettingsServiceChromeOS is independent of load operations
in DeviceSettingsService. The private key may or may not be loaded
when a load operation finishes. The CL adds an explicit flag about
whether a consumer ownership is going to be established. When the
flag is set, DeviceSettingsService defers all loads until InitOwner
is called, which happens when the private key is loaded.
                                                                         
Another problem is that a bool flag |is_current_user_owner_| is
used but it is not updated when switching active user. This causes
incorrect value returned for IsCurrentUserOwner call. The CL fixes
the problems by replacing the bool flag with comparing active user
AccountId with owner AccountId. Security is not reduced because the
owner account id is part of the signed policy blob and only set to
UserManager after policy blob is validated.

BUG=702308, 706820
TEST=DeviceSettingsServiceTest.LoadDeferredDuringOwnershipEastablishment

Review-Url: https://codereview.chromium.org/2779973007
Cr-Commit-Position: refs/heads/master@{#461835}
Committed: https://chromium.googlesource.com/chromium/src/+/c8310bb706ce377ab64aac321f6e8f286826b360

[xiyuan, 2017-03-29 23:02:59]

The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 23:03:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 00:55:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 00:55:31]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-03-30 16:31:49]

Description was changed from

==========
cros: Fix regression of IsCurrentUserOwner

ChroemUserManagerImpl::UpdateOwnership calls SetCurrentUserIsOwner
using the presence of private part of the owner key to update
LoginState etc on NOTIFICATION_OWNERSHIP_STATUS_CHANGED. However,
the notification is sent when the public key part of the owner
key is loaded. This is far earlier than the private part of the
owner key is loaded. Hence SetCurrentUserIsOwner is always invoked
with false.

Another problem is that a bool flag |is_current_user_owner_| is
used but it is not updated when switching active user. This causes
incorrect value returned for IsCurrentUserOwner call.

The CL fixes the problems by replacing the bool flag with comparing
active user AccountId with owner AccountId. Security is not reduced
because the owner account id is part of the signed policy blob and
only set to UserManager after policy blob is validated.

BUG=702308
==========

to

==========
cros: Fix regression of IsCurrentUserOwner

ChromeUserManagerImpl::UpdateOwnership calls SetCurrentUserIsOwner
using the presence of private part of the owner key to update
LoginState etc on NOTIFICATION_OWNERSHIP_STATUS_CHANGED. However,
the notification is sent when the public key part of the owner
key is loaded. This is far earlier than the private part of the
owner key is loaded. Hence SetCurrentUserIsOwner is always invoked
with false.

Another problem is that a bool flag |is_current_user_owner_| is
used but it is not updated when switching active user. This causes
incorrect value returned for IsCurrentUserOwner call.

The CL fixes the problems by replacing the bool flag with comparing
active user AccountId with owner AccountId. Security is not reduced
because the owner account id is part of the signed policy blob and
only set to UserManager after policy blob is validated.

BUG=702308
==========

[xiyuan, 2017-03-31 23:12:24]

Description was changed from

==========
cros: Fix regression of IsCurrentUserOwner

ChromeUserManagerImpl::UpdateOwnership calls SetCurrentUserIsOwner
using the presence of private part of the owner key to update
LoginState etc on NOTIFICATION_OWNERSHIP_STATUS_CHANGED. However,
the notification is sent when the public key part of the owner
key is loaded. This is far earlier than the private part of the
owner key is loaded. Hence SetCurrentUserIsOwner is always invoked
with false.

Another problem is that a bool flag |is_current_user_owner_| is
used but it is not updated when switching active user. This causes
incorrect value returned for IsCurrentUserOwner call.

The CL fixes the problems by replacing the bool flag with comparing
active user AccountId with owner AccountId. Security is not reduced
because the owner account id is part of the signed policy blob and
only set to UserManager after policy blob is validated.

BUG=702308
==========

to

==========
cros: Fix flaky owner detection

When owner key is generated for a consumer owner, OwnerKeySet is
called on both DeviceSettingsService and OwnerSettingsServiceChromeOS.
Code that watches for ownership signal from DeviceSettingsService
(either as an observer or via NOTIFICATION_OWNERSHIP_STATUS_CHANGED)
also expects that the private part of the owner is available at the
time the signal is generated. However, the private key loading
in OwnerSettingsServiceChromeOS is independent of load operations
in DeviceSettingsService. The private key may or may not be loaded
when a load operation finishes. The CL adds an explicit flag about
whether a consumer ownership is going to be established. When the
flag is set, DeviceSettingsService defers all loads until InitOwner
is called, which happens when the private key is loaded.
                                                                         
Second problem is that STORE_KEY_UNAVAILABLE is mapped to                
PERMANENTLY_UNTRUSTED. Per the doc, PERMANENTLY_UNTRUSTED should         
not be changed until chrome restarts. This is not true when              
the ownership has not established. The CL changes to only map            
to PERMANENTLY_UNTRUSTED when ownership is established.                  

Another problem is that a bool flag |is_current_user_owner_| is
used but it is not updated when switching active user. This causes
incorrect value returned for IsCurrentUserOwner call. The CL fixes
the problems by replacing the bool flag with comparing active user
AccountId with owner AccountId. Security is not reduced because the
owner account id is part of the signed policy blob and only set to
UserManager after policy blob is validated.
BUG=702308
==========

[xiyuan, 2017-03-31 23:14:47]

The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 23:15:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-01 00:54:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-01 00:54:35]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-04-03 18:35:27]

Description was changed from

==========
cros: Fix flaky owner detection

When owner key is generated for a consumer owner, OwnerKeySet is
called on both DeviceSettingsService and OwnerSettingsServiceChromeOS.
Code that watches for ownership signal from DeviceSettingsService
(either as an observer or via NOTIFICATION_OWNERSHIP_STATUS_CHANGED)
also expects that the private part of the owner is available at the
time the signal is generated. However, the private key loading
in OwnerSettingsServiceChromeOS is independent of load operations
in DeviceSettingsService. The private key may or may not be loaded
when a load operation finishes. The CL adds an explicit flag about
whether a consumer ownership is going to be established. When the
flag is set, DeviceSettingsService defers all loads until InitOwner
is called, which happens when the private key is loaded.
                                                                         
Second problem is that STORE_KEY_UNAVAILABLE is mapped to                
PERMANENTLY_UNTRUSTED. Per the doc, PERMANENTLY_UNTRUSTED should         
not be changed until chrome restarts. This is not true when              
the ownership has not established. The CL changes to only map            
to PERMANENTLY_UNTRUSTED when ownership is established.                  

Another problem is that a bool flag |is_current_user_owner_| is
used but it is not updated when switching active user. This causes
incorrect value returned for IsCurrentUserOwner call. The CL fixes
the problems by replacing the bool flag with comparing active user
AccountId with owner AccountId. Security is not reduced because the
owner account id is part of the signed policy blob and only set to
UserManager after policy blob is validated.
BUG=702308
==========

to

==========
cros: Fix flaky owner detection

When owner key is generated for a consumer owner, OwnerKeySet is
called on both DeviceSettingsService and OwnerSettingsServiceChromeOS.
Code that watches for ownership signal from DeviceSettingsService
(either as an observer or via NOTIFICATION_OWNERSHIP_STATUS_CHANGED)
also expects that the private part of the owner is available at the
time the signal is generated. However, the private key loading
in OwnerSettingsServiceChromeOS is independent of load operations
in DeviceSettingsService. The private key may or may not be loaded
when a load operation finishes. The CL adds an explicit flag about
whether a consumer ownership is going to be established. When the
flag is set, DeviceSettingsService defers all loads until InitOwner
is called, which happens when the private key is loaded.
                                                                         
Another problem is that a bool flag |is_current_user_owner_| is
used but it is not updated when switching active user. This causes
incorrect value returned for IsCurrentUserOwner call. The CL fixes
the problems by replacing the bool flag with comparing active user
AccountId with owner AccountId. Security is not reduced because the
owner account id is part of the signed policy blob and only set to
UserManager after policy blob is validated.
BUG=702308
==========

[xiyuan, 2017-04-03 18:40:08]

Description was changed from

==========
cros: Fix flaky owner detection

When owner key is generated for a consumer owner, OwnerKeySet is
called on both DeviceSettingsService and OwnerSettingsServiceChromeOS.
Code that watches for ownership signal from DeviceSettingsService
(either as an observer or via NOTIFICATION_OWNERSHIP_STATUS_CHANGED)
also expects that the private part of the owner is available at the
time the signal is generated. However, the private key loading
in OwnerSettingsServiceChromeOS is independent of load operations
in DeviceSettingsService. The private key may or may not be loaded
when a load operation finishes. The CL adds an explicit flag about
whether a consumer ownership is going to be established. When the
flag is set, DeviceSettingsService defers all loads until InitOwner
is called, which happens when the private key is loaded.
                                                                         
Another problem is that a bool flag |is_current_user_owner_| is
used but it is not updated when switching active user. This causes
incorrect value returned for IsCurrentUserOwner call. The CL fixes
the problems by replacing the bool flag with comparing active user
AccountId with owner AccountId. Security is not reduced because the
owner account id is part of the signed policy blob and only set to
UserManager after policy blob is validated.
BUG=702308
==========

to

==========
cros: Fix flaky owner detection

When owner key is generated for a consumer owner, OwnerKeySet is
called on both DeviceSettingsService and OwnerSettingsServiceChromeOS.
Code that watches for ownership signal from DeviceSettingsService
(either as an observer or via NOTIFICATION_OWNERSHIP_STATUS_CHANGED)
also expects that the private part of the owner is available at the
time the signal is generated. However, the private key loading
in OwnerSettingsServiceChromeOS is independent of load operations
in DeviceSettingsService. The private key may or may not be loaded
when a load operation finishes. The CL adds an explicit flag about
whether a consumer ownership is going to be established. When the
flag is set, DeviceSettingsService defers all loads until InitOwner
is called, which happens when the private key is loaded.
                                                                         
Another problem is that a bool flag |is_current_user_owner_| is
used but it is not updated when switching active user. This causes
incorrect value returned for IsCurrentUserOwner call. The CL fixes
the problems by replacing the bool flag with comparing active user
AccountId with owner AccountId. Security is not reduced because the
owner account id is part of the signed policy blob and only set to
UserManager after policy blob is validated.

BUG=702308, 706820
TEST=DeviceSettingsServiceTest.LoadDeferredDuringOwnershipEastablishment
==========

[xiyuan, 2017-04-03 19:02:11]

The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run

[xiyuan, 2017-04-03 19:02:55]

xiyuan@chromium.org changed reviewers:
+ achuith@chromium.org

[xiyuan, 2017-04-03 19:02:56]

https://codereview.chromium.org/2779973007/diff/60001/components/user_manager...
File components/user_manager/user_manager_base.h (left):

https://codereview.chromium.org/2779973007/diff/60001/components/user_manager...
components/user_manager/user_manager_base.h:351: mutable base::Lock
is_current_user_owner_lock_;
The comment and the lock is obsolete.

Both IsCurrentUserOwner and SetCurrentUserIsOwner has 
  DCHECK(task_runner_->RunsTasksOnCurrentThread());

Think that means all calls must happen on the same thread now.

[commit-bot: I haz the power, 2017-04-03 19:03:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 19:53:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-03 19:53:43]

Dry run: This issue passed the CQ dry run.

[achuithb, 2017-04-04 19:38:34]

lgtm with nits

https://codereview.chromium.org/2779973007/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_service_unittest.cc
(right):

https://codereview.chromium.org/2779973007/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service_unittest.cc:461:
TEST_F(DeviceSettingsServiceTest, LoadDeferredDuringOwnershipEastablishment) {
Please add a comment explaining what this test is testing.

Also Eastablishmet->Establishment

[achuithb, 2017-04-04 19:39:57]

The commentary in the description is very useful - it would be nice to have it
somewhere in the code if possible.

[xiyuan, 2017-04-04 20:17:51]

https://codereview.chromium.org/2779973007/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_service_unittest.cc
(right):

https://codereview.chromium.org/2779973007/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service_unittest.cc:461:
TEST_F(DeviceSettingsServiceTest, LoadDeferredDuringOwnershipEastablishment) {
On 2017/04/04 19:38:34, achuithb wrote:
> Please add a comment explaining what this test is testing.
> 
> Also Eastablishmet->Establishment

Done and done.

[xiyuan, 2017-04-04 20:18:42]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2017-04-04 20:18:43]

The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2779973007/#ps80001
(title: "add comment and fix typo")

[commit-bot: I haz the power, 2017-04-04 20:19:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 21:05:45]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491337122780430,
"parent_rev": "69166ca88041e455f8db47cd558e98a525601583", "commit_rev":
"c8310bb706ce377ab64aac321f6e8f286826b360"}

[commit-bot: I haz the power, 2017-04-04 21:06:37]

Description was changed from

==========
cros: Fix flaky owner detection

When owner key is generated for a consumer owner, OwnerKeySet is
called on both DeviceSettingsService and OwnerSettingsServiceChromeOS.
Code that watches for ownership signal from DeviceSettingsService
(either as an observer or via NOTIFICATION_OWNERSHIP_STATUS_CHANGED)
also expects that the private part of the owner is available at the
time the signal is generated. However, the private key loading
in OwnerSettingsServiceChromeOS is independent of load operations
in DeviceSettingsService. The private key may or may not be loaded
when a load operation finishes. The CL adds an explicit flag about
whether a consumer ownership is going to be established. When the
flag is set, DeviceSettingsService defers all loads until InitOwner
is called, which happens when the private key is loaded.
                                                                         
Another problem is that a bool flag |is_current_user_owner_| is
used but it is not updated when switching active user. This causes
incorrect value returned for IsCurrentUserOwner call. The CL fixes
the problems by replacing the bool flag with comparing active user
AccountId with owner AccountId. Security is not reduced because the
owner account id is part of the signed policy blob and only set to
UserManager after policy blob is validated.

BUG=702308, 706820
TEST=DeviceSettingsServiceTest.LoadDeferredDuringOwnershipEastablishment
==========

to

==========
cros: Fix flaky owner detection

When owner key is generated for a consumer owner, OwnerKeySet is
called on both DeviceSettingsService and OwnerSettingsServiceChromeOS.
Code that watches for ownership signal from DeviceSettingsService
(either as an observer or via NOTIFICATION_OWNERSHIP_STATUS_CHANGED)
also expects that the private part of the owner is available at the
time the signal is generated. However, the private key loading
in OwnerSettingsServiceChromeOS is independent of load operations
in DeviceSettingsService. The private key may or may not be loaded
when a load operation finishes. The CL adds an explicit flag about
whether a consumer ownership is going to be established. When the
flag is set, DeviceSettingsService defers all loads until InitOwner
is called, which happens when the private key is loaded.
                                                                         
Another problem is that a bool flag |is_current_user_owner_| is
used but it is not updated when switching active user. This causes
incorrect value returned for IsCurrentUserOwner call. The CL fixes
the problems by replacing the bool flag with comparing active user
AccountId with owner AccountId. Security is not reduced because the
owner account id is part of the signed policy blob and only set to
UserManager after policy blob is validated.

BUG=702308, 706820
TEST=DeviceSettingsServiceTest.LoadDeferredDuringOwnershipEastablishment

Review-Url: https://codereview.chromium.org/2779973007
Cr-Commit-Position: refs/heads/master@{#461835}
Committed:
https://chromium.googlesource.com/chromium/src/+/c8310bb706ce377ab64aac321f6e...
==========

[commit-bot: I haz the power, 2017-04-04 21:06:38]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c8310bb706ce377ab64aac321f6e...