Enable device disabling during normal operation

Enable device disabling during normal operation

This CL enables device disabling during normal operation: The
management server can disable a device by sending the device disabled
flag with the next device policy update. The server can re-enable the
device by clearing the flag with a future policy update.

The flag is persisted along with the entire device policy blob on the
device. As long as the flag is set, the device will prevent logins and
will show a device disabled screen instead. When the flag first
becomes set, the device terminates the current session (if any) and
switches to the device disabled screen immediately.

BUG=425574
TEST=New unit and browser tests

Committed: https://crrev.com/9ce0dc311ee3397218bd1b45f94e481f0b7b1ecd
Cr-Commit-Position: refs/heads/master@{#303241}

[bartfab (slow), 2014-11-06 15:47:28]

bartfab@chromium.org changed reviewers:
+ achuith@chromium.org, antrim@chromium.org, thestig@chromium.org

[bartfab (slow), 2014-11-06 15:47:29]

Hi Lei,

Could you please take a look at:
chrome/browser/browser_process_platform_part_chromeos.cc

Hi Achuith,

Could you please take a look at:
chrome/browser/chromeos/chrome_browser_main_chromeos.cc
chrome/browser/chromeos/system/*

Hi Denis,

Could you please take a look at:
chrome/browser/chromeos/login/*
chrome/browser/ui/webui/chromeos/login/*

[Denis Kuznetsov (DE-MUC), 2014-11-06 17:03:41]

lgtm for
chrome/browser/chromeos/login/*
chrome/browser/ui/webui/chromeos/login/*

[Lei Zhang, 2014-11-06 18:50:56]

chrome/browser/browser_process_platform_part_chromeos.cc lgtm

[bartfab (slow), 2014-11-06 19:10:46]

Patchset #4 (id:60001) has been deleted

[achuithb, 2014-11-07 00:15:17]

lgtm

Do you have a primary reviewer? I just see the various owners. I didn't do a
full review of this and only looked at the various files under my owner purview.
You should probably get someone from MUC familiar with this feature to do a full
review.

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/device_disabling_browsertest.cc (right):

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_browsertest.cc:27: void
SetUpInProcessBrowserTestFixture() override;
You can make these derived methods private since they are only accessed with the
base pointer.

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_browsertest.cc:30:
policy::DevicePolicyBuilder* GetDevicePolicy();
You should add function comments as per style guide, plus these aren't necessary
clear.

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_browsertest.cc:83:
EXPECT_EQ(WizardController::default_controller()->GetScreen(
I was expecting use of the local var wizard_controller here?

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/device_disabling_manager.cc (right):

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager.cc:38: //
DeviceDisablingManager::Delegate:
make these private

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager.cc:87: return;
It's a little bit weird to have an early return from a ctor - it also really
feels like this ctor is doing too much and should probably be split into an
Init() method. Not sure.

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager.cc:222: // only action
required. Return.
Drop 'Return' since it's pretty obvious that's what you plan to do.

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/device_disabling_manager.h (right):

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager.h:112:
scoped_ptr<Delegate> owned_delegate_;
This owned_delegate_, delegate_ business seems so awkward :(

Can't something else own the default delegate and be responsible for its
lifetime?

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/device_disabling_manager_unittest.cc
(right):

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager_unittest.cc:66: const
std::string& GetDisabledMessage() const;
Why special case this since you're providing access to
device_disabling_manager_?

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager_unittest.cc:69:
scoped_ptr<DeviceDisablingManager> device_disabling_manager_;
Might be nicer to provide a pointer to subclasses instead, like
DeviceDisablingManager* GetDeviceDisablingManager() { return
device_disabling_manager_.get(); }

[bartfab (slow), 2014-11-07 10:11:36]

bartfab@chromium.org changed reviewers:
+ pastarmovj@chromium.org

[bartfab (slow), 2014-11-07 10:11:37]

Hi Julian,

Could you take a look at the feature overall?

I have approval from all the owners. As Achuith correctly pointed out, it would
be good to get the overall logic checked before this lands.

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/device_disabling_browsertest.cc (right):

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_browsertest.cc:27: void
SetUpInProcessBrowserTestFixture() override;
On 2014/11/07 00:15:17, achuithb wrote:
> You can make these derived methods private since they are only accessed with
the
> base pointer.

Done.

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_browsertest.cc:30:
policy::DevicePolicyBuilder* GetDevicePolicy();
On 2014/11/07 00:15:17, achuithb wrote:
> You should add function comments as per style guide, plus these aren't
necessary
> clear.

Done.

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_browsertest.cc:83:
EXPECT_EQ(WizardController::default_controller()->GetScreen(
On 2014/11/07 00:15:17, achuithb wrote:
> I was expecting use of the local var wizard_controller here?

Oops. Kind of pointless to set up a local variable and then not use it :).
Thanks for catching this.

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/device_disabling_manager.cc (right):

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager.cc:38: //
DeviceDisablingManager::Delegate:
On 2014/11/07 00:15:17, achuithb wrote:
> make these private

Done.

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager.cc:87: return;
On 2014/11/07 00:15:17, achuithb wrote:
> It's a little bit weird to have an early return from a ctor - it also really
> feels like this ctor is doing too much and should probably be split into an
> Init() method. Not sure.

Done.

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager.cc:222: // only action
required. Return.
On 2014/11/07 00:15:17, achuithb wrote:
> Drop 'Return' since it's pretty obvious that's what you plan to do.

Done.

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/device_disabling_manager.h (right):

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager.h:112:
scoped_ptr<Delegate> owned_delegate_;
On 2014/11/07 00:15:17, achuithb wrote:
> This owned_delegate_, delegate_ business seems so awkward :(
> 
> Can't something else own the default delegate and be responsible for its
> lifetime?

Done.

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/device_disabling_manager_unittest.cc
(right):

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager_unittest.cc:66: const
std::string& GetDisabledMessage() const;
On 2014/11/07 00:15:17, achuithb wrote:
> Why special case this since you're providing access to
> device_disabling_manager_?

Removed.

https://codereview.chromium.org/711433002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager_unittest.cc:69:
scoped_ptr<DeviceDisablingManager> device_disabling_manager_;
On 2014/11/07 00:15:17, achuithb wrote:
> Might be nicer to provide a pointer to subclasses instead, like
> DeviceDisablingManager* GetDeviceDisablingManager() { return
> device_disabling_manager_.get(); }

Done.

[pastarmovj, 2014-11-07 15:32:50]

lgtm

[bartfab (slow), 2014-11-07 15:33:27]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-11-07 15:33:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/711433002/120001

[commit-bot: I haz the power, 2014-11-07 16:28:20]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2014-11-07 16:29:03]

Patchset 6 (id:??) landed as
https://crrev.com/9ce0dc311ee3397218bd1b45f94e481f0b7b1ecd
Cr-Commit-Position: refs/heads/master@{#303241}