Issue 676773002: Add device disabling to OOBE flow

Issue 676773002: Add device disabling to OOBE flow (Closed)

Created:
6 years, 2 months ago by bartfab (slow)

Modified:
6 years, 2 months ago

Reviewers:
Joao da Silva, Nikita (slow)

CC:
chromium-reviews, arv+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Add device disabling to OOBE flow Chrome OS retrieves device state from DMServer during OOBE. If this state indicates that the device should be disabled, it will show a corresponding screen and prevent the user from proceeding further. The feature is on by default because we are aiming for M40 but a flag is provided that can easily be flipped to turn it off if we do not make the deadline. BUG=425574 TEST=Unit and browser tests Committed: https://crrev.com/48e7faaff0bf2007694a9325a6b1c0ffbefcb0d9 Cr-Commit-Position: refs/heads/master@{#301092}

Patch Set 1 #

Total comments: 24

Patch Set 2 : Addressed nits. #

Patch Set 3 : Update browser tests. #

Created: 6 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+619 lines, -85 lines)			Patch
M	chrome/app/chromeos_strings.grdp	View	1	1 chunk	+0 lines, -3 lines	0 comments	Download
M	chrome/browser/chromeos/login/screens/device_disabled_screen.h	View		2 chunks	+10 lines, -0 lines	0 comments	Download
M	chrome/browser/chromeos/login/screens/device_disabled_screen.cc	View	1	3 chunks	+65 lines, -5 lines	0 comments	Download
M	chrome/browser/chromeos/login/screens/device_disabled_screen_actor.h	View		2 chunks	+3 lines, -1 line	0 comments	Download
A	chrome/browser/chromeos/login/screens/device_disabled_screen_unittest.cc	View		1 chunk	+214 lines, -0 lines	0 comments	Download
A	chrome/browser/chromeos/login/screens/mock_device_disabled_screen_actor.h	View		1 chunk	+31 lines, -0 lines	0 comments	Download
A	chrome/browser/chromeos/login/screens/mock_device_disabled_screen_actor.cc	View	1 2	1 chunk	+29 lines, -0 lines	0 comments	Download
M	chrome/browser/chromeos/login/screens/screen_observer.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/chromeos/login/wizard_controller.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/chromeos/login/wizard_controller.cc	View		5 chunks	+16 lines, -14 lines	0 comments	Download
M	chrome/browser/chromeos/login/wizard_controller_browsertest.cc	View	1 2	24 chunks	+97 lines, -17 lines	0 comments	Download
M	chrome/browser/chromeos/policy/auto_enrollment_client.cc	View		1 chunk	+11 lines, -0 lines	0 comments	Download
M	chrome/browser/chromeos/policy/auto_enrollment_client_unittest.cc	View		16 chunks	+102 lines, -27 lines	0 comments	Download
M	chrome/browser/chromeos/policy/server_backed_device_state.h	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/browser/chromeos/policy/server_backed_device_state.cc	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/chromeos/login/screen_device_disabled.js	View		1 chunk	+2 lines, -4 lines	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/device_disabled_screen_handler.h	View		3 chunks	+6 lines, -1 line	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/device_disabled_screen_handler.cc	View		3 chunks	+4 lines, -4 lines	0 comments	Download
M	chrome/chrome_tests_unit.gypi	View	1	3 chunks	+11 lines, -8 lines	0 comments	Download
M	chromeos/chromeos_switches.h	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	chromeos/chromeos_switches.cc	View	1	1 chunk	+3 lines, -0 lines	0 comments	Download
M	components/policy/proto/device_management_backend.proto	View		2 chunks	+8 lines, -0 lines	0 comments	Download

Messages

Total messages: 18 (4 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

bartfab (slow)

Hi Nikita, Could you please review: chrome/browser/chromeos/login/* chrome/browser/resources/chromeos/login/* chrome/browser/ui/webui/chromeos/login/* Hi Joao, Could you please review: ...

6 years, 2 months ago (2014-10-23 15:28:51 UTC) #2

bartfab (slow)

Joao, Nikita, Friendly ping. I would like to land this today to get it into ...

6 years, 2 months ago (2014-10-24 08:15:43 UTC) #3

Joao da Silva

lgtm What's the story to disable devices that are already owned? https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/login/screens/device_disabled_screen.cc File chrome/browser/chromeos/login/screens/device_disabled_screen.cc (right): ...

6 years, 2 months ago (2014-10-24 09:07:31 UTC) #4

Nikita (slow)

https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/login/screens/device_disabled_screen.cc File chrome/browser/chromeos/login/screens/device_disabled_screen.cc (right): https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/login/screens/device_disabled_screen.cc#newcode52 chrome/browser/chromeos/login/screens/device_disabled_screen.cc:52: prefs::kServerBackedDeviceState)->GetBoolean(policy::kDeviceStateDisabled, If device_mode is policy::DEVICE_MODE_PENDING then this boolean is ...

6 years, 2 months ago (2014-10-24 10:26:12 UTC) #5

Nikita (slow)

https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/login/wizard_controller_browsertest.cc File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right): https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/login/wizard_controller_browsertest.cc#newcode726 chrome/browser/chromeos/login/wizard_controller_browsertest.cc:726: : install_attributes_("", "", "", policy::DEVICE_MODE_NOT_SET) { nit: std::string() https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/login/wizard_controller_browsertest.cc#newcode726 ...

6 years, 2 months ago (2014-10-24 10:31:26 UTC) #6

bartfab (slow)

https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/login/screens/device_disabled_screen.cc File chrome/browser/chromeos/login/screens/device_disabled_screen.cc (right): https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/login/screens/device_disabled_screen.cc#newcode40 chrome/browser/chromeos/login/screens/device_disabled_screen.cc:40: actor_->SetDelegate(NULL); On 2014/10/24 09:07:30, Joao da Silva wrote: > ...

6 years, 2 months ago (2014-10-24 11:13:52 UTC) #8

https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screens/device_disabled_screen.cc (right):

https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/device_disabled_screen.cc:40:
actor_->SetDelegate(NULL);
On 2014/10/24 09:07:30, Joao da Silva wrote:
> nullptr

Done.

https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/device_disabled_screen.cc:47: if (!actor_)
On 2014/10/24 09:07:30, Joao da Silva wrote:
> || showing_?

Done.

https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/device_disabled_screen.cc:52:
prefs::kServerBackedDeviceState)->GetBoolean(policy::kDeviceStateDisabled,
On 2014/10/24 10:26:12, Nikita Kostylev wrote:
> If device_mode is policy::DEVICE_MODE_PENDING then this boolean is initialized
> to true?

The value of |is_device_disabled| is independent of |device_mode|'s
initialization status. If |is_device_disabled| is false, we should not show the
screen and we can return straight away. If |is_device_disabled| is true, we
would like to show the screen. But we want to make sure we only do it during
OOBE, not due to some kind of corruption or attack that flipped this flag to
true on an already owned device. This is why we proceed with further checks if
|is_device_disabled| is true only.

https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/device_disabled_screen.cc:68:
base::Bind(&DeviceDisabledScreen::Show,
On 2014/10/24 10:26:12, Nikita Kostylev wrote:
> What would be the UI transition if this check results in DEVICE_NOT_DISABLED?
> Will UI continue to show spinner from previous screen?

Yes, the previous screen will still be showing. Only when we reach
actor_->Show() at the end of this method does the previous screen get replaced
by this one in the UI.

https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/device_disabled_screen.cc:75: // screen
should only be shown during OOBE.
On 2014/10/24 09:07:30, Joao da Silva wrote:
> Just so I understand, how do we disable devices that are already owned?

In this CL, we don't :). It will be done in a follow-up CL.

https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/device_disabled_screen.cc:105: actor_ =
NULL;
On 2014/10/24 09:07:30, Joao da Silva wrote:
> nullptr

Done.

https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right):

https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:726: :
install_attributes_("", "", "", policy::DEVICE_MODE_NOT_SET) {
On 2014/10/24 10:31:26, Nikita Kostylev wrote:
> nit: std::string()

Done.

https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:726: :
install_attributes_("", "", "", policy::DEVICE_MODE_NOT_SET) {
On 2014/10/24 10:31:26, Nikita Kostylev wrote:
> You're setting  policy::DEVICE_MODE_NOT_SET here mostly for
> ControlFlowDeviceDisabled test case. Won't this affect other (and future) test
> cases?

This is the value that the device state would have on a real device. It starts
out as pending and then quickly changes to not set. This setup replicates
reality better now.

https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:856: // Let
update screen smooth time process (time = 0ms).
On 2014/10/24 10:31:26, Nikita Kostylev wrote:
> nit: insert empty line before comment.

Done, here and elsewhere in the file (this line is copy&pasted all over).

https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:870: // Wait for
auto-enrollment controller to encounter the connection error.
On 2014/10/24 10:31:26, Nikita Kostylev wrote:
> nit: insert empty line before comment.

Done.

https://codereview.chromium.org/676773002/diff/1/chrome/chrome_tests_unit.gypi
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/676773002/diff/1/chrome/chrome_tests_unit.gyp...
chrome/chrome_tests_unit.gypi:1727:
'browser/chromeos/login/users/avatar/mock_user_image_manager.cc',
On 2014/10/24 09:07:30, Joao da Silva wrote:
> nit: login/users after login/ui

Done.

commit-bot: I haz the power

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

6 years, 2 months ago (2014-10-24 11:14:15 UTC) #10

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel/builds/4771)

6 years, 2 months ago (2014-10-24 12:20:46 UTC) #12

commit-bot: I haz the power

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

6 years, 2 months ago (2014-10-24 13:03:10 UTC) #14

commit-bot: I haz the power

Patchset 3 (id:??) landed as https://crrev.com/48e7faaff0bf2007694a9325a6b1c0ffbefcb0d9 Cr-Commit-Position: refs/heads/master@{#301092}

6 years, 2 months ago (2014-10-24 13:51:51 UTC) #16

Nikita (slow)

https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/login/screens/device_disabled_screen.cc File chrome/browser/chromeos/login/screens/device_disabled_screen.cc (right): https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/login/screens/device_disabled_screen.cc#newcode52 chrome/browser/chromeos/login/screens/device_disabled_screen.cc:52: prefs::kServerBackedDeviceState)->GetBoolean(policy::kDeviceStateDisabled, On 2014/10/24 11:13:52, bartfab wrote: > On 2014/10/24 ...

6 years, 2 months ago (2014-10-24 13:57:28 UTC) #17

bartfab (slow)

6 years, 2 months ago (2014-10-24 14:02:19 UTC) #18

Message was sent while issue was closed.

https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screens/device_disabled_screen.cc (right):

https://codereview.chromium.org/676773002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/device_disabled_screen.cc:52:
prefs::kServerBackedDeviceState)->GetBoolean(policy::kDeviceStateDisabled,
On 2014/10/24 13:57:28, Nikita Kostylev wrote:
> On 2014/10/24 11:13:52, bartfab wrote:
> > On 2014/10/24 10:26:12, Nikita Kostylev wrote:
> > > If device_mode is policy::DEVICE_MODE_PENDING then this boolean is
> initialized
> > > to true?
> > 
> > The value of |is_device_disabled| is independent of |device_mode|'s
> > initialization status. If |is_device_disabled| is false, we should not show
> the
> > screen and we can return straight away. If |is_device_disabled| is true, we
> > would like to show the screen. But we want to make sure we only do it during
> > OOBE, not due to some kind of corruption or attack that flipped this flag to
> > true on an already owned device. This is why we proceed with further checks
if
> > |is_device_disabled| is true only.
> 
> So if is_device_disabled is true and you proceed in this method and then
there's
> additional check for policy::DEVICE_MODE_PENDING. Is it possible that
> is_device_disabled can changed after that?

In theory, yes, it could happen. And we handled it correctly: If the mode is
policy::DEVICE_MODE_PENDING, we wait for the mode to change and then run the
whole method from the start again. If is_device_disabled has changed to false at
that point, we bail out on the second run.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages