Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1415683013:
Let AutoConnectHandler wait for user policy before disconnecting (Closed)
|
Created:
5 years, 1 month ago by fqj Modified:
5 years, 1 month ago CC:
chromium-reviews, stevenjb+watch_chromium.org, oshima+watch_chromium.org, pneubeck (no reviews) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLet AutoConnectHandler wait for user policy before disconnecting
AutoConnectHandler didn't wait for user policy before disconnecting from
unmanaged network. This will lead to user policy not downloaded or
applied.
This commit make AutoConnectHandler to wait for user policy first before
disconnecting from unmanaged network if device policy requires to do so.
BUG=553497
Committed: https://crrev.com/ece043573566d4759b9c0e6756747dc4d1ecec40
Cr-Commit-Position: refs/heads/master@{#358730}
Patch Set 1 #Patch Set 2 : fix tests #
Total comments: 8
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Messages
Total messages: 25 (6 generated)
fqj@chromium.org changed reviewers: + stevenjb@chromium.org
PTAL.
Please add an issue for this (BUG=)
https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... File chromeos/network/auto_connect_handler.cc (right): https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... chromeos/network/auto_connect_handler.cc:107: DisconnectIfPolicyRequires(); I assume device policy is guaranteed to already be set when user_policy_applied_ is set to true? Might be worth adding a DCHECK(device_policy_applied_) here to document that.
cschuet@chromium.org changed reviewers: + cschuet@chromium.org
https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... File chromeos/network/auto_connect_handler.cc (right): https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... chromeos/network/auto_connect_handler.cc:107: DisconnectIfPolicyRequires(); On 2015/11/09 17:42:53, stevenjb wrote: > I assume device policy is guaranteed to already be set when user_policy_applied_ > is set to true? Might be worth adding a DCHECK(device_policy_applied_) here to > document that. > I don't think that is the case. Managed users can be signed into an unmanaged Chromebook.
On 2015/11/09 17:44:38, cschuet wrote: > https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... > File chromeos/network/auto_connect_handler.cc (right): > > https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... > chromeos/network/auto_connect_handler.cc:107: DisconnectIfPolicyRequires(); > On 2015/11/09 17:42:53, stevenjb wrote: > > I assume device policy is guaranteed to already be set when > user_policy_applied_ > > is set to true? Might be worth adding a DCHECK(device_policy_applied_) here to > > document that. > > > > I don't think that is the case. Managed users can be signed into an unmanaged > Chromebook. Actually scratch that. For an unmanaged device the SetPolicy is probably applied with an empty policy.
Description was changed from ========== Let AutoConnectHandler wait for user policy before disconnecting AutoConnectHandler didn't wait for user policy before disconnecting from unmanaged network. This will lead to user policy not downloaded or applied. This commit make AutoConnectHandler to wait for user policy first before disconnecting from unmanaged network if device policy requires to do so. BUG= ========== to ========== Let AutoConnectHandler wait for user policy before disconnecting AutoConnectHandler didn't wait for user policy before disconnecting from unmanaged network. This will lead to user policy not downloaded or applied. This commit make AutoConnectHandler to wait for user policy first before disconnecting from unmanaged network if device policy requires to do so. BUG=553497 ==========
https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... File chromeos/network/auto_connect_handler.cc (right): https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... chromeos/network/auto_connect_handler.cc:107: DisconnectIfPolicyRequires(); On 2015/11/09 17:44:38, cschuet wrote: > On 2015/11/09 17:42:53, stevenjb wrote: > > I assume device policy is guaranteed to already be set when > user_policy_applied_ > > is set to true? Might be worth adding a DCHECK(device_policy_applied_) here to > > document that. > > > > I don't think that is the case. Managed users can be signed into an unmanaged > Chromebook. I see. So what happens if a device policy is updated and requires disconnecting from the current network?
On 2015/11/09 17:52:24, cschuet wrote: > On 2015/11/09 17:44:38, cschuet wrote: > > > https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... > > File chromeos/network/auto_connect_handler.cc (right): > > > > > https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... > > chromeos/network/auto_connect_handler.cc:107: DisconnectIfPolicyRequires(); > > On 2015/11/09 17:42:53, stevenjb wrote: > > > I assume device policy is guaranteed to already be set when > > user_policy_applied_ > > > is set to true? Might be worth adding a DCHECK(device_policy_applied_) here > to > > > document that. > > > > > > > I don't think that is the case. Managed users can be signed into an unmanaged > > Chromebook. > > Actually scratch that. For an unmanaged device the SetPolicy is probably applied > with an empty policy. We should confirm and document the behavior here to ensure that DisconnectIfPolicyRequires() always gets called.
https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... File chromeos/network/auto_connect_handler.cc (right): https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... chromeos/network/auto_connect_handler.cc:107: DisconnectIfPolicyRequires(); On 2015/11/09 18:05:51, stevenjb wrote: > On 2015/11/09 17:44:38, cschuet wrote: > > On 2015/11/09 17:42:53, stevenjb wrote: > > > I assume device policy is guaranteed to already be set when > > user_policy_applied_ > > > is set to true? Might be worth adding a DCHECK(device_policy_applied_) here > to > > > document that. > > > > > > > I don't think that is the case. Managed users can be signed into an unmanaged > > Chromebook. > > I see. So what happens if a device policy is updated and requires disconnecting > from the current network? > then PoliciesChanged will be called, which will call DisconnectIfPolicyRequires().
https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... File chromeos/network/auto_connect_handler.cc (right): https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... chromeos/network/auto_connect_handler.cc:107: DisconnectIfPolicyRequires(); On 2015/11/09 18:05:51, stevenjb wrote: > On 2015/11/09 17:44:38, cschuet wrote: > > On 2015/11/09 17:42:53, stevenjb wrote: > > > I assume device policy is guaranteed to already be set when > > user_policy_applied_ > > > is set to true? Might be worth adding a DCHECK(device_policy_applied_) here > to > > > document that. > > > > > > > I don't think that is the case. Managed users can be signed into an unmanaged > > Chromebook. > > I see. So what happens if a device policy is updated and requires disconnecting > from the current network? > then PoliciesChanged will be called, which will call DisconnectIfPolicyRequires().
https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... File chromeos/network/auto_connect_handler.cc (right): https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... chromeos/network/auto_connect_handler.cc:107: DisconnectIfPolicyRequires(); On 2015/11/09 18:26:20, fqj wrote: > On 2015/11/09 18:05:51, stevenjb wrote: > > On 2015/11/09 17:44:38, cschuet wrote: > > > On 2015/11/09 17:42:53, stevenjb wrote: > > > > I assume device policy is guaranteed to already be set when > > > user_policy_applied_ > > > > is set to true? Might be worth adding a DCHECK(device_policy_applied_) > here > > to > > > > document that. > > > > > > > > > > I don't think that is the case. Managed users can be signed into an > unmanaged > > > Chromebook. > > > > I see. So what happens if a device policy is updated and requires > disconnecting > > from the current network? > > > > then PoliciesChanged will be called, which will call > DisconnectIfPolicyRequires(). I see. It looks like we never execute DisconnectIfPolicyRequires() until the user logs in (i.e. we assume that a device policy update doesn't require a disconnect, which may be fine). https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... chromeos/network/auto_connect_handler.cc:203: return; Merge this if() with the one above; I missed the !IsUserLoggedIn() condition above when looking at this before, it will be more clear to combine them all.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... File chromeos/network/auto_connect_handler.cc (right): https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... chromeos/network/auto_connect_handler.cc:203: return; On 2015/11/09 18:43:50, stevenjb wrote: > Merge this if() with the one above; I missed the !IsUserLoggedIn() condition > above when looking at this before, it will be more clear to combine them all. Done.
https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... File chromeos/network/auto_connect_handler.cc (right): https://codereview.chromium.org/1415683013/diff/20001/chromeos/network/auto_c... chromeos/network/auto_connect_handler.cc:107: DisconnectIfPolicyRequires(); On 2015/11/09 18:43:50, stevenjb wrote: > On 2015/11/09 18:26:20, fqj wrote: > > On 2015/11/09 18:05:51, stevenjb wrote: > > > On 2015/11/09 17:44:38, cschuet wrote: > > > > On 2015/11/09 17:42:53, stevenjb wrote: > > > > > I assume device policy is guaranteed to already be set when > > > > user_policy_applied_ > > > > > is set to true? Might be worth adding a DCHECK(device_policy_applied_) > > here > > > to > > > > > document that. > > > > > > > > > > > > > I don't think that is the case. Managed users can be signed into an > > unmanaged > > > > Chromebook. > > > > > > I see. So what happens if a device policy is updated and requires > > disconnecting > > > from the current network? > > > > > > > then PoliciesChanged will be called, which will call > > DisconnectIfPolicyRequires(). > > I see. It looks like we never execute DisconnectIfPolicyRequires() until the > user logs in (i.e. we assume that a device policy update doesn't require a > disconnect, which may be fine). Qijian, please update the comments in the header file as well to reflect the modification in the behavior.
lgtm w/ nit https://codereview.chromium.org/1415683013/diff/60001/chromeos/network/auto_c... File chromeos/network/auto_connect_handler.cc (right): https://codereview.chromium.org/1415683013/diff/60001/chromeos/network/auto_c... chromeos/network/auto_connect_handler.cc:201: return; Add {}
Hi cschuet, please have a look at current comments. https://codereview.chromium.org/1415683013/diff/60001/chromeos/network/auto_c... File chromeos/network/auto_connect_handler.cc (right): https://codereview.chromium.org/1415683013/diff/60001/chromeos/network/auto_c... chromeos/network/auto_connect_handler.cc:201: return; On 2015/11/09 18:56:10, stevenjb wrote: > Add {} Done.
On 2015/11/09 19:00:50, fqj wrote: > Hi cschuet, > please have a look at current comments. > > https://codereview.chromium.org/1415683013/diff/60001/chromeos/network/auto_c... > File chromeos/network/auto_connect_handler.cc (right): > > https://codereview.chromium.org/1415683013/diff/60001/chromeos/network/auto_c... > chromeos/network/auto_connect_handler.cc:201: return; > On 2015/11/09 18:56:10, stevenjb wrote: > > Add {} > > Done. LGTM
The CQ bit was checked by fqj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1415683013/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415683013/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415683013/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ece043573566d4759b9c0e6756747dc4d1ecec40 Cr-Commit-Position: refs/heads/master@{#358730} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
