Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
197873012:
WiFiService auto detects WPA(2) encryption type on Windows (Closed)
|
Created:
6 years, 9 months ago by mef Modified:
6 years, 9 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionWiFiService StartConnect after CreateNetwork will try TKIP encryption if AES encryption fails to connect to newly created network.
BUG=328959
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260165
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address codereview comments. #
Total comments: 4
Patch Set 3 : Fix typo. #Patch Set 4 : Delete Profile if it was created implicitly or failed to connect. #
Total comments: 12
Patch Set 5 : Address code review comments. #Messages
Total messages: 33 (0 generated)
Hi guys, please take a look.
https://codereview.chromium.org/197873012/diff/1/components/wifi/wifi_service... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/197873012/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_win.cc:323: bool use_default_encryption, can you pass an enum {TKIP, AES} instead of bool https://codereview.chromium.org/197873012/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_win.cc:594: tkip_profile_.SetString(kNetworkGuidKey, network_properties.guid); Should tkip_profile be saved be per network_guid? Otherwise, something like: CreateNetwork(A); CreateNetwork(B); ConnectNetwork(A); would not try to use TKIP encryption. https://codereview.chromium.org/197873012/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_win.cc:836: error_code = Connect(network_guid, kFrequencyAny); original connect request may have had the frequency set. In that case, the frequency should be restricted for the retry too.
Thanks Toni, PTAL! https://codereview.chromium.org/197873012/diff/1/components/wifi/wifi_service... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/197873012/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_win.cc:323: bool use_default_encryption, On 2014/03/14 22:34:45, tbarzic wrote: > can you pass an enum {TKIP, AES} instead of bool Done. https://codereview.chromium.org/197873012/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_win.cc:594: tkip_profile_.SetString(kNetworkGuidKey, network_properties.guid); On 2014/03/14 22:34:45, tbarzic wrote: > Should tkip_profile be saved be per network_guid? > > Otherwise, something like: > CreateNetwork(A); > CreateNetwork(B); > ConnectNetwork(A); > > would not try to use TKIP encryption. Done. https://codereview.chromium.org/197873012/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_win.cc:836: error_code = Connect(network_guid, kFrequencyAny); On 2014/03/14 22:34:45, tbarzic wrote: > original connect request may have had the frequency set. In that case, the > frequency should be restricted for the retry too. Done.
lgtm
On 2014/03/15 16:26:02, tbarzic wrote: > lgtm Thanks, Toni! Antonio, could you take a look to validate the idea?
https://codereview.chromium.org/197873012/diff/20001/components/wifi/wifi_ser... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/197873012/diff/20001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:373: // Create |profile_xml| based on |network_properties|. If |encryptio_type| typo encryptio_type https://codereview.chromium.org/197873012/diff/20001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:1622: &reason_code); Why WlanSetProfile instead of WlanSaveTemporaryProfile?
Thanks, Antonio! https://codereview.chromium.org/197873012/diff/20001/components/wifi/wifi_ser... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/197873012/diff/20001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:373: // Create |profile_xml| based on |network_properties|. If |encryptio_type| On 2014/03/17 19:56:55, afontan wrote: > typo encryptio_type Done. https://codereview.chromium.org/197873012/diff/20001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:1622: &reason_code); On 2014/03/17 19:56:55, afontan wrote: > Why WlanSetProfile instead of WlanSaveTemporaryProfile? Hmm, no apparently good reason. WlanSaveTemporaryProfile is not available pre-Vista, but I don't think it is a good excuse as we don't support XP at the moment anyway. WlanSetProfile was also used in initial CL (https://codereview.chromium.org/105153002) that implemented CreateNetwork, but I'm completely open to changing it if there are compelling reasons. I'm not sure I understand the lifespan of profile saved by 'WlanSaveTemporaryProfile', does it get auto-deleted at some point?
On 2014/03/17 21:40:08, mef wrote: > Thanks, Antonio! > > https://codereview.chromium.org/197873012/diff/20001/components/wifi/wifi_ser... > File components/wifi/wifi_service_win.cc (right): > > https://codereview.chromium.org/197873012/diff/20001/components/wifi/wifi_ser... > components/wifi/wifi_service_win.cc:373: // Create |profile_xml| based on > |network_properties|. If |encryptio_type| > On 2014/03/17 19:56:55, afontan wrote: > > typo encryptio_type > > Done. > > https://codereview.chromium.org/197873012/diff/20001/components/wifi/wifi_ser... > components/wifi/wifi_service_win.cc:1622: &reason_code); > On 2014/03/17 19:56:55, afontan wrote: > > Why WlanSetProfile instead of WlanSaveTemporaryProfile? > > Hmm, no apparently good reason. WlanSaveTemporaryProfile is not available > pre-Vista, but I don't think it is a good excuse as we don't support XP at the > moment anyway. > > WlanSetProfile was also used in initial CL > (https://codereview.chromium.org/105153002) that implemented CreateNetwork, but > I'm completely open to changing it if there are compelling reasons. > > I'm not sure I understand the lifespan of profile saved by > 'WlanSaveTemporaryProfile', does it get auto-deleted at some point? Actually hold on, WlanSaveTemporaryProfile doesn't take the profile xml, how would it create a profile?
On 2014/03/17 21:49:00, mef wrote: > On 2014/03/17 21:40:08, mef wrote: > > Thanks, Antonio! > > > > > https://codereview.chromium.org/197873012/diff/20001/components/wifi/wifi_ser... > > File components/wifi/wifi_service_win.cc (right): > > > > > https://codereview.chromium.org/197873012/diff/20001/components/wifi/wifi_ser... > > components/wifi/wifi_service_win.cc:373: // Create |profile_xml| based on > > |network_properties|. If |encryptio_type| > > On 2014/03/17 19:56:55, afontan wrote: > > > typo encryptio_type > > > > Done. > > > > > https://codereview.chromium.org/197873012/diff/20001/components/wifi/wifi_ser... > > components/wifi/wifi_service_win.cc:1622: &reason_code); > > On 2014/03/17 19:56:55, afontan wrote: > > > Why WlanSetProfile instead of WlanSaveTemporaryProfile? > > > > Hmm, no apparently good reason. WlanSaveTemporaryProfile is not available > > pre-Vista, but I don't think it is a good excuse as we don't support XP at the > > moment anyway. > > > > WlanSetProfile was also used in initial CL > > (https://codereview.chromium.org/105153002) that implemented CreateNetwork, > but > > I'm completely open to changing it if there are compelling reasons. > > > > I'm not sure I understand the lifespan of profile saved by > > 'WlanSaveTemporaryProfile', does it get auto-deleted at some point? > > Actually hold on, WlanSaveTemporaryProfile doesn't take the profile xml, how > would it create a profile? You are right, the idea here would be to create the temp profile and then set its content. The goal is to avoid the profiles to be written to the profile store permanently, (specially profiles for non-setup Chromecasts hotspots, of profiles with a wrong password...). In the current app, we do not actually create the profile, we just pass it as a string to the connect API. However, if the connection is successful, windows does create the profile permanently. I do not think this is the behavior we want but I did not have time to investigate it. My assumption is that creating the temporary profile could avoid these issues, otherwise you need to clean up the profile manually yourself. I understand that this is not what this CL is about so feel free to tackle this issue later, but I just thought I would point it out.
Hi guys, PTAL. Antonio, per our discussion I've added a method to delete newly created profile if it failed to connect or got created implicitly by connection to infrastructure mode. Toni, please take a look again as changes were not completely trivial. thanks, -m
Looks good, some minor comments. https://codereview.chromium.org/197873012/diff/60001/components/wifi/wifi_ser... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/197873012/diff/60001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:829: // network change notifications and stop waiting. The comment should be updated to reflect the TKIP retry logic https://codereview.chromium.org/197873012/diff/60001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:857: } We should probably log the case where error_code != ERROR_SUCCESS https://codereview.chromium.org/197873012/diff/60001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:860: RemoveCreatedProfile(network_guid, true); Should we check the return value and log if we could not delete the profile? https://codereview.chromium.org/197873012/diff/60001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:875: // There is no need to keep created profile as network is connected. In this case we are not deleting the profile created but just removing it from the dictionary. Is this the intention? I can go either way as the profile is clearly valid. Whatever is the intention, it should be clearly stated here. The current comment may be a bit misleading. https://codereview.chromium.org/197873012/diff/60001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:876: RemoveCreatedProfile(network_guid, false); We should check the return value and log. https://codereview.chromium.org/197873012/diff/60001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:1722: *key_type = kKeyTypePassphrase; This if/else is getting a bit long so is harder to read. May be we could use a switch and avoid some duplication using an helper method.
Hi Antonio, thanks, PTAL! I've removed the code to handle deletion of implicitly created profile as in my testing profile doesn't get created until user connects to different network, so deletion doesn't help. This means that your suggestion of WlanSaveTemporaryProfile may work better, but this is not related to TKIP encryption detection, so I'd rather do that in separate CL. thanks, -m https://codereview.chromium.org/197873012/diff/60001/components/wifi/wifi_ser... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/197873012/diff/60001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:829: // network change notifications and stop waiting. On 2014/03/26 17:33:23, afontan wrote: > The comment should be updated to reflect the TKIP retry logic Done. https://codereview.chromium.org/197873012/diff/60001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:857: } On 2014/03/26 17:33:23, afontan wrote: > We should probably log the case where error_code != ERROR_SUCCESS Done. https://codereview.chromium.org/197873012/diff/60001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:860: RemoveCreatedProfile(network_guid, true); On 2014/03/26 17:33:23, afontan wrote: > Should we check the return value and log if we could not delete the profile? Done. https://codereview.chromium.org/197873012/diff/60001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:875: // There is no need to keep created profile as network is connected. On 2014/03/26 17:33:23, afontan wrote: > In this case we are not deleting the profile created but just removing it from > the dictionary. Is this the intention? I can go either way as the profile is > clearly valid. > > Whatever is the intention, it should be clearly stated here. The current comment > may be a bit misleading. Done. https://codereview.chromium.org/197873012/diff/60001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:876: RemoveCreatedProfile(network_guid, false); On 2014/03/26 17:33:23, afontan wrote: > We should check the return value and log. Done. I've changed it to explicitly remove it from the dictionary, so no error checking is needed. https://codereview.chromium.org/197873012/diff/60001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:1722: *key_type = kKeyTypePassphrase; On 2014/03/26 17:33:23, afontan wrote: > This if/else is getting a bit long so is harder to read. May be we could use a > switch and avoid some duplication using an helper method. I've added a helper method. A switch doesn't work here as |security| is actually a string.
lgtm
On 2014/03/26 20:59:09, afontan wrote: > lgtm lgtm
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/197873012/90001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/197873012/90001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/197873012/90001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) content_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/197873012/90001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on ios_dbg_simulator for step(s) crypto_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/197873012/90001
Message was sent while issue was closed.
Change committed as 260165 |
