Handle connecting to hidden networks correctly

Handle connecting to hidden networks correctly

The new network connect code does not handle hidden networks correctly.
This fixes that, which also fixed the associated issue.

BUG=270936, 166999
R=pneubeck@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217133

[stevenjb, 2013-08-10 00:38:22]

I noticed this bug while testing / fixing some of the edge case bugs that are
now more fixable with the re-factor (woot!)
Dependent on https://chromiumcodereview.appspot.com/22340006/ (in CQ)

[pneubeck (no reviews), 2013-08-10 19:44:10]

while reading some the code, I commented on some not directly related issues.

https://codereview.chromium.org/22674011/diff/1/chromeos/network/network_conn...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/22674011/diff/1/chromeos/network/network_conn...
chromeos/network/network_connection_handler.cc:234: const NetworkState* network
=
add a comment somewhere what it means if network==NULL.

Actually, I don't know exactly what Shill's behavior is here:
I thought Shill searches for Endpoints of hidden networks. If it finds an
endpoint the service will be listed in kServicesProperty and here it should end
up with network!=NULL.
If Shill didn't find an Endpoint, shouldn't then any connect attempt be in vain?


If network==NULL, you might consider checking if the service path is in the
FavoriteList to prevent handling and forwarding invalid service paths (from
potentially insecure sources, like JS) to Shill.

https://codereview.chromium.org/22674011/diff/1/chromeos/network/network_conn...
chromeos/network/network_connection_handler.cc:255: if (!network) {
this looks a bit contradicting to the handling if !check_error_state.

This really needs some comments what your intend is.

https://codereview.chromium.org/22674011/diff/1/chromeos/network/network_conn...
chromeos/network/network_connection_handler.cc:285: if (network->connectable()
&& network->type() != flimflam::kTypeVPN) {
For network==NULL, the connectable property will never be checked, although it's
available in VerifyConfiguredAndConnect.

https://codereview.chromium.org/22674011/diff/1/chromeos/network/network_conn...
chromeos/network/network_connection_handler.cc:295: // Try to connect anyway.
Failure may succeed anyway, or provide a
"Failure may succeed"
sounds wrong to me.

https://codereview.chromium.org/22674011/diff/1/chromeos/network/network_conn...
chromeos/network/network_connection_handler.cc:553: DCHECK(request);
considering the complexity of the overall logic and the number of accesses to
pending_request_,
it would be safer to handle request==NULL in release and log an error.

similarly in HandleShillConnect*

concrete issue could be:
CheckPendingRequest might be called with IsConnectedState (because of an
autoconnect) and remove the pending_request.

https://codereview.chromium.org/22674011/diff/1/chromeos/network/network_conn...
chromeos/network/network_connection_handler.cc:588: ConnectRequest* request =
pending_request(service_path);
Every time I read "pending_request", I wonder what it does.
Could you rename it to e.g. GetPendingRequest(...). IMO that would be clearer.

[stevenjb, 2013-08-12 17:48:29]

Thanks for the thorough review. As you noted, this is some tricky code so more
eyes are greatly appreciated.

https://codereview.chromium.org/22674011/diff/1/chromeos/network/network_conn...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/22674011/diff/1/chromeos/network/network_conn...
chromeos/network/network_connection_handler.cc:234: const NetworkState* network
=
On 2013/08/10 19:44:11, pneubeck wrote:
> add a comment somewhere what it means if network==NULL.
> 
> Actually, I don't know exactly what Shill's behavior is here:
> I thought Shill searches for Endpoints of hidden networks. If it finds an
> endpoint the service will be listed in kServicesProperty and here it should
end
> up with network!=NULL.
> If Shill didn't find an Endpoint, shouldn't then any connect attempt be in
vain?
> 
> 
> If network==NULL, you might consider checking if the service path is in the
> FavoriteList to prevent handling and forwarding invalid service paths (from
> potentially insecure sources, like JS) to Shill.

This should only happen if we call ConnectToNetwork immediately after
configuring a network. We have two choices here:
1. Request a NetworkState update and wait for it to complete (requires an adding
a callback to RequestUpdateForNetwork).
2. Skip the checks that require |network| and rely on the error handling for
VerifyConfiguredAndConnect.

I went with 2, since:
a) We already do a round-trip request with VerifyConfiguredAndConnect.
b) The checks requiring 'network' mostly don't apply in this case, except
'Connectable' as you noted.
c) This removes any direct dependency on NetworkState, i.e. we use NetworkState
when available but don't rely on it.

I will improve the comments.

https://codereview.chromium.org/22674011/diff/1/chromeos/network/network_conn...
chromeos/network/network_connection_handler.cc:255: if (!network) {
On 2013/08/10 19:44:11, pneubeck wrote:
> this looks a bit contradicting to the handling if !check_error_state.
> 
> This really needs some comments what your intend is.

Yeah, this just ends up being confusing, and really doesn't inform the UI, so I
removed this check.

https://codereview.chromium.org/22674011/diff/1/chromeos/network/network_conn...
chromeos/network/network_connection_handler.cc:285: if (network->connectable()
&& network->type() != flimflam::kTypeVPN) {
On 2013/08/10 19:44:11, pneubeck wrote:
> For network==NULL, the connectable property will never be checked, although
it's
> available in VerifyConfiguredAndConnect.

Yes, I should add that there.
Done.

https://codereview.chromium.org/22674011/diff/1/chromeos/network/network_conn...
chromeos/network/network_connection_handler.cc:295: // Try to connect anyway.
Failure may succeed anyway, or provide a
On 2013/08/10 19:44:11, pneubeck wrote:
> "Failure may succeed"
> sounds wrong to me.

Oops, bad edit. 
Looking at the code, I actually removed this check, since we need to handle this
case in VerifyConfiguredAndConnect anyway.

https://codereview.chromium.org/22674011/diff/1/chromeos/network/network_conn...
chromeos/network/network_connection_handler.cc:553: DCHECK(request);
On 2013/08/10 19:44:11, pneubeck wrote:
> considering the complexity of the overall logic and the number of accesses to
> pending_request_,
> it would be safer to handle request==NULL in release and log an error.
> 
> similarly in HandleShillConnect*
> 
> concrete issue could be:
> CheckPendingRequest might be called with IsConnectedState (because of an
> autoconnect) and remove the pending_request.

Hmm, definitely an edge case, but possible. Good catch. I'll fix this throughout
the code.

https://codereview.chromium.org/22674011/diff/1/chromeos/network/network_conn...
chromeos/network/network_connection_handler.cc:588: ConnectRequest* request =
pending_request(service_path);
On 2013/08/10 19:44:11, pneubeck wrote:
> Every time I read "pending_request", I wonder what it does.
> Could you rename it to e.g. GetPendingRequest(...). IMO that would be clearer.
Done.

[pneubeck (no reviews), 2013-08-12 18:32:44]

lgtm

https://codereview.chromium.org/22674011/diff/6001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/22674011/diff/6001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:283: if (network->connectable()
&& network->type() != flimflam::kTypeVPN) {
NIT: combine the two ifs

[pneubeck (no reviews), 2013-08-12 18:35:01]

https://codereview.chromium.org/22674011/diff/1/chromeos/network/network_conn...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/22674011/diff/1/chromeos/network/network_conn...
chromeos/network/network_connection_handler.cc:234: const NetworkState* network
=
On 2013/08/12 17:48:29, stevenjb (chromium) wrote:
> On 2013/08/10 19:44:11, pneubeck wrote:
> > add a comment somewhere what it means if network==NULL.
> > 
> > Actually, I don't know exactly what Shill's behavior is here:
> > I thought Shill searches for Endpoints of hidden networks. If it finds an
> > endpoint the service will be listed in kServicesProperty and here it should
> end
> > up with network!=NULL.
> > If Shill didn't find an Endpoint, shouldn't then any connect attempt be in
> vain?
> > 
> > 
> > If network==NULL, you might consider checking if the service path is in the
> > FavoriteList to prevent handling and forwarding invalid service paths (from
> > potentially insecure sources, like JS) to Shill.
> 
> This should only happen if we call ConnectToNetwork immediately after
> configuring a network. We have two choices here:
> 1. Request a NetworkState update and wait for it to complete (requires an
adding
> a callback to RequestUpdateForNetwork).
> 2. Skip the checks that require |network| and rely on the error handling for
> VerifyConfiguredAndConnect.
> 
> I went with 2, since:
> a) We already do a round-trip request with VerifyConfiguredAndConnect.
> b) The checks requiring 'network' mostly don't apply in this case, except
> 'Connectable' as you noted.
> c) This removes any direct dependency on NetworkState, i.e. we use
NetworkState
> when available but don't rely on it.

Sounds reasonable. In particular c) is IMO very important.

[stevenjb, 2013-08-13 00:26:07]

Committed patchset #5 manually as r217133 (presubmit successful).