Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2898003002:
Allow the powerwash shortcut only for devices without FRE (Closed)
DescriptionAllow the powerwash shortcut only for devices without FRE.
The devices that have FRE enabled shouldn't be allowed to powerwash. This
CL makes the keyboard shortcut for powerwash available only if the device
doesn't have FRE enabled.
BUG=724332
TEST=Manually tested for FRE enabled and FRE disabled functionality.
Review-Url: https://codereview.chromium.org/2898003002
Cr-Commit-Position: refs/heads/master@{#476222}
Committed: https://chromium.googlesource.com/chromium/src/+/ba2086693a48256aa1e74d7755e5e425464797bb
Patch Set 1 : Let the powerwash shortcut only for devices without FRE #
Total comments: 8
Patch Set 2 : Fixed review comments #
Total comments: 4
Patch Set 3 : Fixed review comments #
Messages
Total messages: 31 (18 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Disable powerwash shortcut for devices with FRE enabled Merged Default unspecified value added to enum Policy definition for encryptfs to ext4 migration strategy BUG= ========== to ========== Allow the powerwash shortcut only for devices without FRE The devices that have FRE enabled shouldn't be allowed to powerwash. This CL doesn't include the keyboard shortcut on login screen for those devices. BUG=724332 ==========
Description was changed from ========== Allow the powerwash shortcut only for devices without FRE The devices that have FRE enabled shouldn't be allowed to powerwash. This CL doesn't include the keyboard shortcut on login screen for those devices. BUG=724332 ========== to ========== Allow the powerwash shortcut only for devices without FRE The devices that have FRE enabled shouldn't be allowed to powerwash. This CL doesn't include the keyboard shortcut on login screen for those devices. BUG=724332 ==========
Description was changed from ========== Allow the powerwash shortcut only for devices without FRE The devices that have FRE enabled shouldn't be allowed to powerwash. This CL doesn't include the keyboard shortcut on login screen for those devices. BUG=724332 ========== to ========== Allow the powerwash shortcut only for devices without FRE. The devices that have FRE enabled shouldn't be allowed to powerwash. This CL doesn't include the keyboard shortcut on login screen for those devices. BUG=724332 ==========
Description was changed from ========== Allow the powerwash shortcut only for devices without FRE. The devices that have FRE enabled shouldn't be allowed to powerwash. This CL doesn't include the keyboard shortcut on login screen for those devices. BUG=724332 ========== to ========== Allow the powerwash shortcut only for devices without FRE. The devices that have FRE enabled shouldn't be allowed to powerwash. This CL makes the keyboard shortcut for powerwash available only if the device doesn't have FRE enabled. BUG=724332 ==========
Description was changed from ========== Allow the powerwash shortcut only for devices without FRE. The devices that have FRE enabled shouldn't be allowed to powerwash. This CL makes the keyboard shortcut for powerwash available only if the device doesn't have FRE enabled. BUG=724332 ========== to ========== Allow the powerwash shortcut only for devices without FRE. The devices that have FRE enabled shouldn't be allowed to powerwash. This CL makes the keyboard shortcut for powerwash available only if the device doesn't have FRE enabled. BUG=724332 TEST=Manually tested for FRE enabled and FRE disabled functionality. ==========
igorcov@chromium.org changed reviewers: + atwilson@chromium.org, derat@chromium.org, tnagel@chromium.org
atwilson@chromium.org: PTAL in auto_enrollment_controller tnagel@chromium.org: PTAL in auto_enrollment_controller derat@chromium.org: PTAL webui_login_view
please add a reviewer from chrome/browser/chromeos/login/OWNERS -- use more-specific OWNERS files when available: achuith@chromium.org alemate@chromium.org piman@chromium.org (actually, i'm guessing that piman@ should be removed from that file since i don't think he works on the login code.)
igorcov@chromium.org changed reviewers: + achuith@chromium.org
achuith@, PTAL webui_login_view. Thank you,
https://codereview.chromium.org/2898003002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/ui/webui_login_view.cc (right): https://codereview.chromium.org/2898003002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/ui/webui_login_view.cc:146: AutoEnrollmentController::FRERequirement requirement = nit const. nit newline before this line Also, please add a comment. https://codereview.chromium.org/2898003002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/ui/webui_login_view.cc:149: requirement == AutoEnrollmentController::EXPLICITLY_NOT_REQUIRED) Add {} https://codereview.chromium.org/2898003002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/ui/webui_login_view.cc:152: ui::EF_SHIFT_DOWN)] = kAccelNameReset; nit newline after this line
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM but please don't check this CL in until you've confirmed with someone on the PM side that blocking powerwash + recovery shortcuts is desirable/acceptable. It is a speed bump at best since attackers can always fall back to USB recovery. https://codereview.chromium.org/2898003002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc (right): https://codereview.chromium.org/2898003002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:110: AutoEnrollmentController::GetFRERequirement() { I'm not a huge fan of moving code around unnecessarily - I'm going to presume that: a) You're doing this because of some uncontrollable impulse to keep the .h declarations in the same order as the definitions in the .cc file. b) and that you didn't actually change the code. If either of these aren't the case, please let me know.
Thank you Achuith and Drew for your comments. Please take a look again. https://codereview.chromium.org/2898003002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc (right): https://codereview.chromium.org/2898003002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:110: AutoEnrollmentController::GetFRERequirement() { On 2017/05/23 05:32:06, Andrew T Wilson (Slow) wrote: > I'm not a huge fan of moving code around unnecessarily - I'm going to presume > that: > > a) You're doing this because of some uncontrollable impulse to keep the .h > declarations in the same order as the definitions in the .cc file. > > b) and that you didn't actually change the code. > > If either of these aren't the case, please let me know. I need this function to be visible from outside, so I put it as a static member of the class. In the previous version is was a global function. The code inside the function didn't change. https://codereview.chromium.org/2898003002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/ui/webui_login_view.cc (right): https://codereview.chromium.org/2898003002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/ui/webui_login_view.cc:146: AutoEnrollmentController::FRERequirement requirement = On 2017/05/22 19:21:29, achuithb wrote: > nit const. > > nit newline before this line > > Also, please add a comment. Done. https://codereview.chromium.org/2898003002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/ui/webui_login_view.cc:149: requirement == AutoEnrollmentController::EXPLICITLY_NOT_REQUIRED) On 2017/05/22 19:21:28, achuithb wrote: > Add {} Done. https://codereview.chromium.org/2898003002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/ui/webui_login_view.cc:152: ui::EF_SHIFT_DOWN)] = kAccelNameReset; On 2017/05/22 19:21:29, achuithb wrote: > nit newline after this line Done.
lgtm https://codereview.chromium.org/2898003002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h (right): https://codereview.chromium.org/2898003002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h:74: static FRERequirement GetFRERequirement(); Why not have: bool IsForcedAutoEnrollmentRequired(). That way webui_login_view doesn't need to know about NOT_REQUIRED and EXPLICITLY_NOT_REQUIRED, etc. https://codereview.chromium.org/2898003002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/ui/webui_login_view.cc (right): https://codereview.chromium.org/2898003002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/ui/webui_login_view.cc:147: // The devices with forced re-enrollment enabled shouldn't be able to nit: drop 'The'
AutoEnrollmentController lgtm % nits https://codereview.chromium.org/2898003002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc (right): https://codereview.chromium.org/2898003002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:82: AutoEnrollmentController::Mode AutoEnrollmentController::GetMode() { Nit: Add "// static" above. https://codereview.chromium.org/2898003002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:109: AutoEnrollmentController::FRERequirement Nit: Add "// static" above.
The CQ bit was checked by igorcov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from atwilson@chromium.org, tnagel@chromium.org, achuith@chromium.org Link to the patchset: https://codereview.chromium.org/2898003002/#ps60001 (title: "Fixed review comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Still lgtm.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by igorcov@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496302744985640,
"parent_rev": "3889aeb8e6d2eb20d8d8c4ad9688890d34645a23", "commit_rev":
"ba2086693a48256aa1e74d7755e5e425464797bb"}
Message was sent while issue was closed.
Description was changed from ========== Allow the powerwash shortcut only for devices without FRE. The devices that have FRE enabled shouldn't be allowed to powerwash. This CL makes the keyboard shortcut for powerwash available only if the device doesn't have FRE enabled. BUG=724332 TEST=Manually tested for FRE enabled and FRE disabled functionality. ========== to ========== Allow the powerwash shortcut only for devices without FRE. The devices that have FRE enabled shouldn't be allowed to powerwash. This CL makes the keyboard shortcut for powerwash available only if the device doesn't have FRE enabled. BUG=724332 TEST=Manually tested for FRE enabled and FRE disabled functionality. Review-Url: https://codereview.chromium.org/2898003002 Cr-Commit-Position: refs/heads/master@{#476222} Committed: https://chromium.googlesource.com/chromium/src/+/ba2086693a48256aa1e74d7755e5... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/ba2086693a48256aa1e74d7755e5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
