Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
390443006:
Support for remote enrollment. (Closed)
DescriptionSupport for remote enrollment.
BUG=374990
TEST=manual
Committed: https://crrev.com/5f3f02eac7c8f32606a2913d204c9e46e550136b
Cr-Commit-Position: refs/heads/master@{#291782}
Patch Set 1 : . #
Total comments: 18
Patch Set 2 : bartfab feedback #
Total comments: 2
Patch Set 3 : sent_auth_token_ #
Total comments: 6
Patch Set 4 : nits #
Messages
Total messages: 14 (0 generated)
I'll edit the description before I commit.
* When a brand new device boots up, it normally stops at the language/keyboard/network selection screen and requires the user to explicitly click "Continue" before proceeding. Is it expected that a device which receives a token from a shark will move forward without waiting for user action? * You could persist the token to Local State so that it can survive reboots. This will not prevent it from expiring of course but if the token happens to still be valid, it will allow it to be used without resending. https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:149: GetDeviceCloudPolicyManager()->IsSharkRequisition(); Nit: IsSharkRequisition() should not be a part of the DeviceCloudPolicyManagerChromeOS. It is used during enrollment only and should be moved to a class that handles enrollment, e.g. EnrollmentHandlerChromeOS. https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:153: auth_token_ = token; |auth_token_| plays a dual role that makes it difficult to follow the code: * On a shark device, |auth_token_| is used as Boolean indicator that tells us whether we are currently fetching the first or the second token. The shark device must never read and process |auth_token_| itself because that token was sent to another device and will be used by it. * On a device that receives a token from a shark, |auth_token_| holds the token that will actually be used during enrollment. How about introducing a Boolean |sent_token_| that is used on shark only to keep track of whether a token was retrieved and sent to another device already? https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:241: } Nit: Add a TODO that briefly explains what this method will do in the future. https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:1265: void WizardController::OnEnrollmentAuthTokenReceived( Nit: There are no callers of this method yet. Document in a TODO who will be calling this in the future. https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:1269: StartupUtils::MarkEulaAccepted(); Is it OK to just silently accept the EULA? https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:1273: LOG(WARNING) << "Not in device enrollment"; Nit: Add full stop to make this a complete sentence. https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.h (right): https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.h:348: std::string auth_token_; Nit: There are no users of this memeber yet. Document in a TODO who will be using this in the future.
PTAL, Bartosz. https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:149: GetDeviceCloudPolicyManager()->IsSharkRequisition(); On 2014/07/16 10:09:15, bartfab wrote: > Nit: IsSharkRequisition() should not be a part of the > DeviceCloudPolicyManagerChromeOS. It is used during enrollment only and should > be moved to a class that handles enrollment, e.g. EnrollmentHandlerChromeOS. I tried doing this here: https://codereview.chromium.org/408013003/ It doesn't work because EnrollmentHandlerChromeOS is created too late for our needs. I've taken over the bug and am happy to implement a solution that would be satisfactory to the enterprise team. https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:153: auth_token_ = token; On 2014/07/16 10:09:15, bartfab wrote: > |auth_token_| plays a dual role that makes it difficult to follow the code: > > * On a shark device, |auth_token_| is used as Boolean indicator that tells us > whether we are currently fetching the first or the second token. The shark > device must never read and process |auth_token_| itself because that token was > sent to another device and will be used by it. > > * On a device that receives a token from a shark, |auth_token_| holds the token > that will actually be used during enrollment. > > How about introducing a Boolean |sent_token_| that is used on shark only to keep > track of whether a token was retrieved and sent to another device already? What you say is true. However, in the current implementation of the test code, we use an asynchronous call to base::WriteFile to write the auth_token to a file, and that function very inconveniently does not allow us to use base::Owned or any other mechanism to delete the auth_token storage after we're done writing it. Two options here are to simply leak the string, or use PostTaskAndReply instead. I've added a comment and opened a bug to track this for now. https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:241: } On 2014/07/16 10:09:15, bartfab wrote: > Nit: Add a TODO that briefly explains what this method will do in the future. Done. I'd like to keep this brief since this isn't public yet. https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:1265: void WizardController::OnEnrollmentAuthTokenReceived( On 2014/07/16 10:09:15, bartfab wrote: > Nit: There are no callers of this method yet. Document in a TODO who will be > calling this in the future. Done, kept it brief. https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:1269: StartupUtils::MarkEulaAccepted(); On 2014/07/16 10:09:15, bartfab wrote: > Is it OK to just silently accept the EULA? Yup, the EULA is accepted on the controller side, so we run with it. https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:1273: LOG(WARNING) << "Not in device enrollment"; On 2014/07/16 10:09:15, bartfab wrote: > Nit: Add full stop to make this a complete sentence. Done. https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.h (right): https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.h:348: std::string auth_token_; On 2014/07/16 10:09:15, bartfab wrote: > Nit: There are no users of this memeber yet. Document in a TODO who will be > using this in the future. That's not true? It's set by OnEnrollmentAuthTokenReceived, and used by EnrollmentScreen::SetParameters.
https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:149: GetDeviceCloudPolicyManager()->IsSharkRequisition(); On 2014/08/13 01:57:42, achuithb wrote: > On 2014/07/16 10:09:15, bartfab wrote: > > Nit: IsSharkRequisition() should not be a part of the > > DeviceCloudPolicyManagerChromeOS. It is used during enrollment only and should > > be moved to a class that handles enrollment, e.g. EnrollmentHandlerChromeOS. > > I tried doing this here: > https://codereview.chromium.org/408013003/ > > It doesn't work because EnrollmentHandlerChromeOS is created too late for our > needs. I've taken over the bug and am happy to implement a solution that would > be satisfactory to the enterprise team. Yes, we can discuss this further in crbug.com/383695. I am fine with IsSharkRequisition() living alongside the existing IsRemoraRequisition() until we figure out a better home for both. https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:153: auth_token_ = token; Just make a wrapper around base::WriteFile that takes a |const std::string&| instead of a |const char*|. You can then change CL 395983002 to do: namespace { void WriteStringToFile(const FilePath& filename, const std::string& data) { base::WriteFile(filename, data.c_str(), data.size()); } } void EnrollmentScreen::SendEnrollmentAuthToken(const std::string& token) { content::BrowserThread::GetBlockingPool()->PostTask( FROM_HERE, base::Bind(&WriteStringToFile, base::FilePath::FromUTF8Unsafe("/var/tmp/auth_token"), token)); } Et voilà, no more need to keep the |auth_token_| around on shark. On 2014/08/13 01:57:42, achuithb wrote: > On 2014/07/16 10:09:15, bartfab wrote: > > |auth_token_| plays a dual role that makes it difficult to follow the code: > > > > * On a shark device, |auth_token_| is used as Boolean indicator that tells us > > whether we are currently fetching the first or the second token. The shark > > device must never read and process |auth_token_| itself because that token was > > sent to another device and will be used by it. > > > > * On a device that receives a token from a shark, |auth_token_| holds the > token > > that will actually be used during enrollment. > > > > How about introducing a Boolean |sent_token_| that is used on shark only to > keep > > track of whether a token was retrieved and sent to another device already? > > What you say is true. However, in the current implementation of the test code, > we use an asynchronous call to base::WriteFile to write the auth_token to a > file, and that function very inconveniently does not allow us to use base::Owned > or any other mechanism to delete the auth_token storage after we're done writing > it. Two options here are to simply leak the string, or use PostTaskAndReply > instead. > > I've added a comment and opened a bug to track this for now. https://codereview.chromium.org/390443006/diff/110001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/390443006/diff/110001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:142: GetDeviceCloudPolicyManager()->IsSharkRequisition(); Nit: Indent four more spaces.
I've introduced remora_token_sent_ to avoid overloading auth_token_ as Bartosz suggests. PTAL, Bartosz! https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:149: GetDeviceCloudPolicyManager()->IsSharkRequisition(); On 2014/08/13 14:20:14, bartfab wrote: > On 2014/08/13 01:57:42, achuithb wrote: > > On 2014/07/16 10:09:15, bartfab wrote: > > > Nit: IsSharkRequisition() should not be a part of the > > > DeviceCloudPolicyManagerChromeOS. It is used during enrollment only and > should > > > be moved to a class that handles enrollment, e.g. EnrollmentHandlerChromeOS. > > > > I tried doing this here: > > https://codereview.chromium.org/408013003/ > > > > It doesn't work because EnrollmentHandlerChromeOS is created too late for our > > needs. I've taken over the bug and am happy to implement a solution that would > > be satisfactory to the enterprise team. > > Yes, we can discuss this further in crbug.com/383695. I am fine with > IsSharkRequisition() living alongside the existing IsRemoraRequisition() until > we figure out a better home for both. Acknowledged. https://codereview.chromium.org/390443006/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:153: auth_token_ = token; On 2014/08/13 14:20:14, bartfab wrote: > Just make a wrapper around base::WriteFile that takes a |const std::string&| > instead of a |const char*|. You can then change CL 395983002 to do: > > namespace { > > void WriteStringToFile(const FilePath& filename, const std::string& data) { > base::WriteFile(filename, data.c_str(), data.size()); > } > > } > > void EnrollmentScreen::SendEnrollmentAuthToken(const std::string& token) { > content::BrowserThread::GetBlockingPool()->PostTask( > FROM_HERE, > base::Bind(&WriteStringToFile, > base::FilePath::FromUTF8Unsafe("/var/tmp/auth_token"), > token)); > } > > Et voilà, no more need to keep the |auth_token_| around on shark. > > On 2014/08/13 01:57:42, achuithb wrote: > > On 2014/07/16 10:09:15, bartfab wrote: > > > |auth_token_| plays a dual role that makes it difficult to follow the code: > > > > > > * On a shark device, |auth_token_| is used as Boolean indicator that tells > us > > > whether we are currently fetching the first or the second token. The shark > > > device must never read and process |auth_token_| itself because that token > was > > > sent to another device and will be used by it. > > > > > > * On a device that receives a token from a shark, |auth_token_| holds the > > token > > > that will actually be used during enrollment. > > > > > > How about introducing a Boolean |sent_token_| that is used on shark only to > > keep > > > track of whether a token was retrieved and sent to another device already? > > > > What you say is true. However, in the current implementation of the test code, > > we use an asynchronous call to base::WriteFile to write the auth_token to a > > file, and that function very inconveniently does not allow us to use > base::Owned > > or any other mechanism to delete the auth_token storage after we're done > writing > > it. Two options here are to simply leak the string, or use PostTaskAndReply > > instead. > > > > I've added a comment and opened a bug to track this for now. > Done. https://codereview.chromium.org/390443006/diff/110001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/390443006/diff/110001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:142: GetDeviceCloudPolicyManager()->IsSharkRequisition(); On 2014/08/13 14:20:14, bartfab wrote: > Nit: Indent four more spaces. Done.
Friendly ping
On 2014/08/19 09:37:55, achuithb wrote: > Friendly ping Sorry, I was OOO for 4 days. I will do the first review first thing tomorrow.
On 2014/08/19 16:32:22, bartfab wrote: > On 2014/08/19 09:37:55, achuithb wrote: > > Friendly ping > > Sorry, I was OOO for 4 days. I will do the first review first thing tomorrow. No worries! Hope you had a good break.
lgtm https://codereview.chromium.org/390443006/diff/130001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/390443006/diff/130001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:142: g_browser_process->platform_part()->browser_policy_connector_chromeos()-> Nit: #include "chrome/browser/process/browser_process_platform_part.h" https://codereview.chromium.org/390443006/diff/130001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:236: // TODO(achuith, zork): Send token via bluetooth to remote device. Nit: s/bluetooth/Bluetooth/ https://codereview.chromium.org/390443006/diff/130001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/390443006/diff/130001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:1304: // TODO(achuith, zork): This is called via bluetooth from a remote controller. Nit 1: More like s/is/will be/ or s/is/should be/? Nit 2: s/bluetooth/Bluetooth/
https://codereview.chromium.org/390443006/diff/130001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right): https://codereview.chromium.org/390443006/diff/130001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:142: g_browser_process->platform_part()->browser_policy_connector_chromeos()-> On 2014/08/20 09:31:34, bartfab wrote: > Nit: #include "chrome/browser/process/browser_process_platform_part.h" Done. https://codereview.chromium.org/390443006/diff/130001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:236: // TODO(achuith, zork): Send token via bluetooth to remote device. On 2014/08/20 09:31:34, bartfab wrote: > Nit: s/bluetooth/Bluetooth/ Done. https://codereview.chromium.org/390443006/diff/130001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/390443006/diff/130001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:1304: // TODO(achuith, zork): This is called via bluetooth from a remote controller. On 2014/08/20 09:31:34, bartfab wrote: > Nit 1: More like s/is/will be/ or s/is/should be/? > Nit 2: s/bluetooth/Bluetooth/ Done.
The CQ bit was checked by achuith@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/achuith@chromium.org/390443006/150001
Message was sent while issue was closed.
Committed patchset #4 (150001) as 7b16f52b25cf569191300071d528c4130c4c06d6
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5f3f02eac7c8f32606a2913d204c9e46e550136b Cr-Commit-Position: refs/heads/master@{#291782} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
