Differentiate between 'connect-failed' and 'bad-passphrase'

Differentiate between 'connect-failed' and 'bad-passphrase'

Now that all connection requests are going through 
NetworkConnectionHandler we can start to improve the connect flow 
logic.

'connect-failed' can indicate a lot of things; we shouldn't assume that
configuration is required when that Error state is set.


BUG=278559
R=pneubeck@chromium.org

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

[pneubeck (no reviews), 2013-08-26 08:14:10]

non-VPN looks good to me.
For VPN see the comments, which we could address in another CL.

https://codereview.chromium.org/22867045/diff/3001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.cc (left):

https://codereview.chromium.org/22867045/diff/3001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:257: if (error ==
flimflam::kErrorConnectFailed) {
for non-VPN this looks fine, as it's handled later in
VerifyConfiguredAndConnect.

But I'm a bit concerned that we silently ignore a connect-failed error in case
of VPN. See my comment in VerifyConfiguredAndConnect.

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

https://codereview.chromium.org/22867045/diff/3001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:464:
client_cert::SetShillProperties(client_cert_type,
For VPN with certs, we will always reach this point...

https://codereview.chromium.org/22867045/diff/3001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:499: return;
...and then call CallShillConnect/return here.

Instead (in another CL), we should at first check if the new cert configuration
is different from the existing one.
If not and Shill reported a failure from a previous connect attempt, then we
should return with an error.

https://codereview.chromium.org/22867045/diff/3001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.h (right):

https://codereview.chromium.org/22867045/diff/3001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:95: //  
flimflam::kErrorBadPassphrase if the Shill network property (from a
'network property' ? maybe error property

[stevenjb, 2013-08-27 15:52:07]

It might be easier to discuss in person, but I am unconvinced that this change
is incorrect for VPN.

https://codereview.chromium.org/22867045/diff/3001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.cc (left):

https://codereview.chromium.org/22867045/diff/3001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:257: if (error ==
flimflam::kErrorConnectFailed) {
On 2013/08/26 08:14:11, pneubeck wrote:
> for non-VPN this looks fine, as it's handled later in
> VerifyConfiguredAndConnect.
> 
> But I'm a bit concerned that we silently ignore a connect-failed error in case
> of VPN. See my comment in VerifyConfiguredAndConnect.

The purpose of this code was to try to guess the user's intent:
* If we know that the network is misconfigured (i.e. Error == bad-passphrase),
then we should show the configure UI instead of attempting to connect.
* Sometimes, when Error == connect-failed, it was because of a configuration
error, in which case we would like to show the configure UI. However, unless we
know that the failure was due to a configuration error, showing the configure UI
will be confusing (e.g for WiMax).
* If we try to connect and it fails (again), we will show the configure UI
anyway, it just may take a little longer.

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

https://codereview.chromium.org/22867045/diff/3001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:499: return;
On 2013/08/26 08:14:11, pneubeck wrote:
> ...and then call CallShillConnect/return here.
> 
> Instead (in another CL), we should at first check if the new cert
configuration
> is different from the existing one.
> If not and Shill reported a failure from a previous connect attempt, then we
> should return with an error.

I'm not sure that we can rely on 'connect-failed' meaning 'bad certificate' even
if the certificate hasn't changed. We have seen issues with Google-A failing to
connect due to AP flakiness, then showing the configure dialog which is
confusing. Even with this change we will still show the configure UI on failure
to connect, but instead of showing the configure UI again on the next connect
attempt, we will try to connect again first, which seems more correct.

Ideally we would like more information from Shill (e.g. Error =
'bad-configuration') if we know that the certificate is incorrect, but I do not
know whether we can get that information.

https://codereview.chromium.org/22867045/diff/3001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.h (right):

https://codereview.chromium.org/22867045/diff/3001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:95: //  
flimflam::kErrorBadPassphrase if the Shill network property (from a
On 2013/08/26 08:14:11, pneubeck wrote:
> 'network property' ? maybe error property

Done.

[pneubeck (no reviews), 2013-08-28 10:02:35]

lgtm to commit, but still might need a follow up for VPN. I setup a meeting for
today.

https://codereview.chromium.org/22867045/diff/3001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.cc (left):

https://codereview.chromium.org/22867045/diff/3001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:257: if (error ==
flimflam::kErrorConnectFailed) {
My bad.. I should have placed this comment to the new code and not the old... I
meant: Removing this looks fine for non-VPN.

On 2013/08/27 15:52:08, stevenjb (chromium) wrote:
> On 2013/08/26 08:14:11, pneubeck wrote:
> > for non-VPN this looks fine, as it's handled later in
> > VerifyConfiguredAndConnect.
> > 
> > But I'm a bit concerned that we silently ignore a connect-failed error in
case
> > of VPN. See my comment in VerifyConfiguredAndConnect.
> 
> The purpose of this code was to try to guess the user's intent:
> * If we know that the network is misconfigured (i.e. Error == bad-passphrase),
> then we should show the configure UI instead of attempting to connect.
> * Sometimes, when Error == connect-failed, it was because of a configuration
> error, in which case we would like to show the configure UI. However, unless
we
> know that the failure was due to a configuration error, showing the configure
UI
> will be confusing (e.g for WiMax).
> * If we try to connect and it fails (again), we will show the configure UI
> anyway, it just may take a little longer.

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

https://codereview.chromium.org/22867045/diff/3001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:499: return;
Still, it doesn't look right to change control flow or set cert properties, if
we did so during an earlier attempt.

On 2013/08/27 15:52:08, stevenjb (chromium) wrote:
> On 2013/08/26 08:14:11, pneubeck wrote:
> > ...and then call CallShillConnect/return here.
> > 
> > Instead (in another CL), we should at first check if the new cert
> configuration
> > is different from the existing one.
> > If not and Shill reported a failure from a previous connect attempt, then we
> > should return with an error.
> 
> I'm not sure that we can rely on 'connect-failed' meaning 'bad certificate'
even
> if the certificate hasn't changed. We have seen issues with Google-A failing
to
> connect due to AP flakiness, then showing the configure dialog which is
> confusing. Even with this change we will still show the configure UI on
failure
> to connect,

Where in the code is this "configure UI on failure to connect" in case of VPN
with CertificatePattern?

> but instead of showing the configure UI again on the next connect
> attempt, we will try to connect again first, which seems more correct.
> 
> Ideally we would like more information from Shill (e.g. Error =
> 'bad-configuration') if we know that the certificate is incorrect, but I do
not
> know whether we can get that information.
>

[stevenjb, 2013-08-30 16:00:50]

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