Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2642793002:
Don't start device cloud policy connection after Chromad enrollment (Closed)
|
Created:
3 years, 11 months ago by Thiemo Nagel Modified:
3 years, 11 months ago Reviewers:
emaxx CC:
chromium-reviews, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't start device cloud policy connection after Chromad enrollment
After Active Directory enrollment, don't start the connection for the
device cloud policy manager (as would happen for cloud management).
Also ensure that TryToCreateClient() doesn't start a client, as could
e.g. be triggered by OnStoreLoaded().
BUG=677510
Review-Url: https://codereview.chromium.org/2642793002
Cr-Commit-Position: refs/heads/master@{#444451}
Committed: https://chromium.googlesource.com/chromium/src/+/33d0fb8f91ec8be93f0bc56b63c67a9f2dcebec6
Patch Set 1 #
Total comments: 2
Patch Set 2 : Check for enrollment type in EnrollmentCompleted() and TryToCreateClient() #
Depends on Patchset: Messages
Total messages: 26 (17 generated)
The CQ bit was checked by tnagel@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...
tnagel@chromium.org changed reviewers: + emaxx@chromium.org
Hi Maksim, could you please take a look at this tiny change? Thank you, Thiemo
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Regarding the TryToCreateClient() method (which is actually something like "TryToCreateClientAndConnect") - from a brief reading, it seems that it may call StartConnection() in the AD case too. (Maybe you can even test this quickly in a dev build.) If that's the case, then the new condition would have to be added into TryToCreateClient() too. https://codereview.chromium.org/2642793002/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc (right): https://codereview.chromium.org/2642793002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:283: !install_attributes_->IsActiveDirectoryManaged()) { I think that it would be clearer if StartConnection() won't be called for the Active Directory case in the first place. It's easy to miss the condition when it's checked here, or to underestimate its importance in the AD workflow.
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Don't start device cloud policy connection after Chromad enrollment BUG=677510 ========== to ========== Don't start device cloud policy connection after Chromad enrollment After Active Directory enrollment, don't start the connection for the device cloud policy manager (as would happen for cloud management). Also ensure that TryToCreateClient() doesn't start a client, as could e.g. be triggered by OnStoreLoaded(). BUG=677510 ==========
Thanks a lot for your comments (both online and offline). I've moved the check to 2 different places and updated the commit message. Could you please take another look? https://codereview.chromium.org/2642793002/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc (right): https://codereview.chromium.org/2642793002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:283: !install_attributes_->IsActiveDirectoryManaged()) { On 2017/01/18 15:33:56, emaxx wrote: > I think that it would be clearer if StartConnection() won't be called for the > Active Directory case in the first place. It's easy to miss the condition when > it's checked here, or to underestimate its importance in the AD workflow. Done.
The CQ bit was checked by tnagel@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...
lgtm
The CQ bit was unchecked by tnagel@chromium.org
The CQ bit was checked by tnagel@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2640843002 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
On 2017/01/18 17:44:40, emaxx wrote: > lgtm Thank you!
The CQ bit was checked by tnagel@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2640843002 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by tnagel@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": 40001, "attempt_start_ts": 1484769453934630,
"parent_rev": "fcc18800b5155b78baf2d9040fd12c4af2e37ab9", "commit_rev":
"33d0fb8f91ec8be93f0bc56b63c67a9f2dcebec6"}
Message was sent while issue was closed.
Description was changed from ========== Don't start device cloud policy connection after Chromad enrollment After Active Directory enrollment, don't start the connection for the device cloud policy manager (as would happen for cloud management). Also ensure that TryToCreateClient() doesn't start a client, as could e.g. be triggered by OnStoreLoaded(). BUG=677510 ========== to ========== Don't start device cloud policy connection after Chromad enrollment After Active Directory enrollment, don't start the connection for the device cloud policy manager (as would happen for cloud management). Also ensure that TryToCreateClient() doesn't start a client, as could e.g. be triggered by OnStoreLoaded(). BUG=677510 Review-Url: https://codereview.chromium.org/2642793002 Cr-Commit-Position: refs/heads/master@{#444451} Committed: https://chromium.googlesource.com/chromium/src/+/33d0fb8f91ec8be93f0bc56b63c6... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/33d0fb8f91ec8be93f0bc56b63c6... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
