Don't generate a certificate browse button or passphrase field if cert path contains SETTINGS:

Don't generate a certificate browse button or passphrase field if cert path contains SETTINGS:
Made certificate_path_ a string instead of a FilePath.
Modify ConnectToWifiNetwork to take std::string.
BUG=None 
TEST=manual

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=51290

[stevenjb, 2010-06-23 20:25:01]

http://codereview.chromium.org/2869019/diff/4001/5005
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/2869019/diff/4001/5005#newcode321
chrome/browser/chromeos/cros/network_library.h:321: bool auto_connect) = 0;
ConnectToWifiNetwork uses UTF8, so move any UTF16 to UTF8 conversion into the UI
code to avoid unnecessary UFT8 -> UTF16 -> UTF8 conversions.

http://codereview.chromium.org/2869019/diff/4001/5009
File chrome/browser/chromeos/options/network_config_view.cc (right):

http://codereview.chromium.org/2869019/diff/4001/5009#newcode148
chrome/browser/chromeos/options/network_config_view.cc:148: return result;
Allow sizing of this dialog to be dynamic since not all network types require
every possible UI element (e.g. passohrase).

http://codereview.chromium.org/2869019/diff/4001/5010
File chrome/browser/chromeos/options/wifi_config_view.cc (right):

http://codereview.chromium.org/2869019/diff/4001/5010#newcode74
chrome/browser/chromeos/options/wifi_config_view.cc:74: }
Since we may not require a passphrase, check that the field exists, and change
all of the logic to &= so that all required elements are checked before enabling
login.

http://codereview.chromium.org/2869019/diff/4001/5010#newcode250
chrome/browser/chromeos/options/wifi_config_view.cc:250: }
This is the meat of the change. If we have a certificate and it's in the tpm,
show some text saying that it is "Installed." Otherwise show the browse button.

http://codereview.chromium.org/2869019/diff/4001/5011
File chrome/browser/chromeos/options/wifi_config_view.h (right):

http://codereview.chromium.org/2869019/diff/4001/5011#newcode91
chrome/browser/chromeos/options/wifi_config_view.h:91: std::string
certificate_path_;
certificate_path_ does not always contain a file path, so store as a string.
(variable name corresponds to flimflam service property "CertPath" so leaving
the name as-is).

[Charlie Lee, 2010-06-23 23:58:55]

http://codereview.chromium.org/2869019/diff/4001/5010
File chrome/browser/chromeos/options/wifi_config_view.cc (right):

http://codereview.chromium.org/2869019/diff/4001/5010#newcode239
chrome/browser/chromeos/options/wifi_config_view.cc:239: certificate_in_tpm =
(certificate_path_.find("SETTINGS:") == 0);
Is this how we tell if certificate is in TPM? Is this just a guess based on what
we are seeing now? Or is this a fail-safe way to determine if certificate is in
TPM?

Please add comments about this.

http://codereview.chromium.org/2869019/diff/4001/5010#newcode248
chrome/browser/chromeos/options/wifi_config_view.cc:248: this,
UTF8ToWide(wifi_.cert_path()));
use certificate_path_ here.

http://codereview.chromium.org/2869019/diff/4001/5010#newcode256
chrome/browser/chromeos/options/wifi_config_view.cc:256:
layout->AddView(certificate_browse_button_);
Combine this code with the above code. Basically code should look like:
if (cert in tpm) {
  do tpm cert ui
} else {
  do file cert ui. if cert is empty, use default button label.
}

http://codereview.chromium.org/2869019/diff/4001/5010#newcode264
chrome/browser/chromeos/options/wifi_config_view.cc:264:
(wifi_.passphrase_required())) {
what's the reasoning for adding passphrase_required here?
as far as i know passphrase_required is set to false if the passphrase is stored
in flimflam. so what we care about is wifi.encrypted and not
passphrase_required.

[stevenjb, 2010-06-24 01:10:37]

http://codereview.chromium.org/2869019/diff/4001/5010
File chrome/browser/chromeos/options/wifi_config_view.cc (right):

http://codereview.chromium.org/2869019/diff/4001/5010#newcode239
chrome/browser/chromeos/options/wifi_config_view.cc:239: certificate_in_tpm =
(certificate_path_.find("SETTINGS:") == 0);
On 2010/06/23 23:58:55, chocobo wrote:
> Is this how we tell if certificate is in TPM? Is this just a guess based on
what
> we are seeing now? Or is this a fail-safe way to determine if certificate is
in
> TPM?
> 
> Please add comments about this.

Heh. I had this documented and then I simplified it a bunch and lost the
documentation. Will make this more clear. But yeah, flimflam uses "SETTINGS:" to
specify parameters for reading the certificate from a pkcs#11 device (i.e. the
TPM).
Done.

http://codereview.chromium.org/2869019/diff/4001/5010#newcode248
chrome/browser/chromeos/options/wifi_config_view.cc:248: this,
UTF8ToWide(wifi_.cert_path()));
On 2010/06/23 23:58:55, chocobo wrote:
> use certificate_path_ here.

Done.

http://codereview.chromium.org/2869019/diff/4001/5010#newcode256
chrome/browser/chromeos/options/wifi_config_view.cc:256:
layout->AddView(certificate_browse_button_);
On 2010/06/23 23:58:55, chocobo wrote:
> Combine this code with the above code. Basically code should look like:
> if (cert in tpm) {
>   do tpm cert ui
> } else {
>   do file cert ui. if cert is empty, use default button label.
> }

Done.

http://codereview.chromium.org/2869019/diff/4001/5010#newcode264
chrome/browser/chromeos/options/wifi_config_view.cc:264:
(wifi_.passphrase_required())) {
On 2010/06/23 23:58:55, chocobo wrote:
> what's the reasoning for adding passphrase_required here?
> as far as i know passphrase_required is set to false if the passphrase is
stored
> in flimflam. so what we care about is wifi.encrypted and not
> passphrase_required.
Ah, I misunderstood the meaning of passphrase_required; I should have read more
carefully and asked about it. I will revert the related changes.
Done.

[Nathan Williams, 2010-06-24 14:06:11]

On Wed, Jun 23, 2010 at 9:10 PM, <stevenjb@chromium.org> wrote:

>
> http://codereview.chromium.org/2869019/diff/4001/5010
> File chrome/browser/chromeos/options/wifi_config_view.cc (right):
>
> http://codereview.chromium.org/2869019/diff/4001/5010#newcode239
> chrome/browser/chromeos/options/wifi_config_view.cc:239:
> certificate_in_tpm = (certificate_path_.find("SETTINGS:") == 0);
> On 2010/06/23 23:58:55, chocobo wrote:
>
>> Is this how we tell if certificate is in TPM? Is this just a guess
>>
> based on what
>
>> we are seeing now? Or is this a fail-safe way to determine if
>>
> certificate is in
>
>> TPM?
>>
>
>  Please add comments about this.
>>
>
> Heh. I had this documented and then I simplified it a bunch and lost the
> documentation. Will make this more clear. But yeah, flimflam uses
> "SETTINGS:" to specify parameters for reading the certificate from a
> pkcs#11 device (i.e. the TPM).
> Done.


SETTINGS: can cover the certificate-file case as well; there's nothing that
restricts it to pkcs#11 operation, and in the fullness of time, I'd like to
just use this mechanism for both. I think the code should look inside the
string and see if a key_id field is present - that's what really tells you
that you aren't using a (possibly) password-protected key file.

    - Nathan

[Nathan Williams, 2010-06-24 14:06:13]

On Wed, Jun 23, 2010 at 9:10 PM, <stevenjb@chromium.org> wrote:

>
> http://codereview.chromium.org/2869019/diff/4001/5010
> File chrome/browser/chromeos/options/wifi_config_view.cc (right):
>
> http://codereview.chromium.org/2869019/diff/4001/5010#newcode239
> chrome/browser/chromeos/options/wifi_config_view.cc:239:
> certificate_in_tpm = (certificate_path_.find("SETTINGS:") == 0);
> On 2010/06/23 23:58:55, chocobo wrote:
>
>> Is this how we tell if certificate is in TPM? Is this just a guess
>>
> based on what
>
>> we are seeing now? Or is this a fail-safe way to determine if
>>
> certificate is in
>
>> TPM?
>>
>
>  Please add comments about this.
>>
>
> Heh. I had this documented and then I simplified it a bunch and lost the
> documentation. Will make this more clear. But yeah, flimflam uses
> "SETTINGS:" to specify parameters for reading the certificate from a
> pkcs#11 device (i.e. the TPM).
> Done.


SETTINGS: can cover the certificate-file case as well; there's nothing that
restricts it to pkcs#11 operation, and in the fullness of time, I'd like to
just use this mechanism for both. I think the code should look inside the
string and see if a key_id field is present - that's what really tells you
that you aren't using a (possibly) password-protected key file.

    - Nathan

[stevenjb, 2010-06-24 17:38:09]

On 2010/06/24 14:06:13, Nathan Williams wrote:
> On Wed, Jun 23, 2010 at 9:10 PM, <mailto:stevenjb@chromium.org> wrote:
> 
> SETTINGS: can cover the certificate-file case as well; there's nothing that
> restricts it to pkcs#11 operation, and in the fullness of time, I'd like to
> just use this mechanism for both. I think the code should look inside the
> string and see if a key_id field is present - that's what really tells you
> that you aren't using a (possibly) password-protected key file.
> 
>     - Nathan
> 

Ah. I misread the docs. Pulled the parsing out into a pair of helper functions
and now look for key_id=.

[Charlie Lee, 2010-06-24 21:41:45]

http://codereview.chromium.org/2869019/diff/5013/17010
File chrome/browser/chromeos/options/wifi_config_view.cc (right):

http://codereview.chromium.org/2869019/diff/5013/17010#newcode185
chrome/browser/chromeos/options/wifi_config_view.cc:185: static bool
find_settings_token(const std::string& path,
inline this method in the one below. i don't see a reason to have a separate
method that's only called once.

http://codereview.chromium.org/2869019/diff/5013/17010#newcode197
chrome/browser/chromeos/options/wifi_config_view.cc:197: // flimflam recognizes
the string "SETTINGS:" to specify authentication
please add comments on what exactly this method does.

http://codereview.chromium.org/2869019/diff/5013/17010#newcode199
chrome/browser/chromeos/options/wifi_config_view.cc:199: static bool
parse_settings(const std::string& path, bool* pkcs11) {
it makes more sense to turn this into:

static bool is_certificate_in_pkcs11(const std::string& path)

[stevenjb, 2010-06-24 22:29:13]

http://codereview.chromium.org/2869019/diff/5013/17010
File chrome/browser/chromeos/options/wifi_config_view.cc (right):

http://codereview.chromium.org/2869019/diff/5013/17010#newcode185
chrome/browser/chromeos/options/wifi_config_view.cc:185: static bool
find_settings_token(const std::string& path,
On 2010/06/24 21:41:45, chocobo wrote:
> inline this method in the one below. i don't see a reason to have a separate
> method that's only called once.
My thinking was that we may need to do additional parsing of SETTINGS: in the
future. That being said, if we do it may not be here, so it makes sense to avoid
the added complexity for now, we can always refactor later.

http://codereview.chromium.org/2869019/diff/5013/17010#newcode197
chrome/browser/chromeos/options/wifi_config_view.cc:197: // flimflam recognizes
the string "SETTINGS:" to specify authentication
On 2010/06/24 21:41:45, chocobo wrote:
> please add comments on what exactly this method does.

Done.

http://codereview.chromium.org/2869019/diff/5013/17010#newcode199
chrome/browser/chromeos/options/wifi_config_view.cc:199: static bool
parse_settings(const std::string& path, bool* pkcs11) {
On 2010/06/24 21:41:45, chocobo wrote:
> it makes more sense to turn this into:
> 
> static bool is_certificate_in_pkcs11(const std::string& path)

Done.

[Charlie Lee, 2010-06-24 22:43:15]

http://codereview.chromium.org/2869019/diff/1013/24010
File chrome/browser/chromeos/options/wifi_config_view.cc (right):

http://codereview.chromium.org/2869019/diff/1013/24010#newcode194
chrome/browser/chromeos/options/wifi_config_view.cc:194: std::string::size_type
idx = path.find(pkcs11_key);
The code you have works, but can it be simpler?

why not just search for "key_id =" and if that is found, then return true? Do
you expect more than 1 space or other whitespace characters in between key_id
and =?

Or better yet, maybe having "key_id" is good enough.

http://codereview.chromium.org/2869019/diff/1013/24010#newcode260
chrome/browser/chromeos/options/wifi_config_view.cc:260: if
(certificate_in_pkcs11) {
it might be simpler to call is_certificate_in_pkcs11() here.
it shouldn't be a problem if certificate_path_ is an empty string.
what do you think?

[stevenjb, 2010-06-24 22:57:48]

http://codereview.chromium.org/2869019/diff/1013/24010
File chrome/browser/chromeos/options/wifi_config_view.cc (right):

http://codereview.chromium.org/2869019/diff/1013/24010#newcode194
chrome/browser/chromeos/options/wifi_config_view.cc:194: std::string::size_type
idx = path.find(pkcs11_key);
On 2010/06/24 22:43:15, chocobo wrote:
> The code you have works, but can it be simpler?
> 
> why not just search for "key_id =" and if that is found, then return true? Do
> you expect more than 1 space or other whitespace characters in between key_id
> and =?
> 
> Or better yet, maybe having "key_id" is good enough.
I've had enough bad experiences with soft parsing that I prefer to err on the
side of being robust. I'd hate for the code to break because elsewhere it
changed to "key_id = 3" (currently it is actually "key_id=3"), and I wouldn't
want it to break on "SETTINGS:file=hotkey_ident".

http://codereview.chromium.org/2869019/diff/1013/24010#newcode260
chrome/browser/chromeos/options/wifi_config_view.cc:260: if
(certificate_in_pkcs11) {
On 2010/06/24 22:43:15, chocobo wrote:
> it might be simpler to call is_certificate_in_pkcs11() here.
> it shouldn't be a problem if certificate_path_ is an empty string.
> what do you think?
We also use certificate_in_pkcs11 below to check whether or not to show the
passphrase.

[Charlie Lee, 2010-06-24 22:59:45]

Fair enough, LGTM