Don't require login to enable CA cert settings

Don't require login to enable CA cert settings


BUG=23658
TEST=See issue


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

[stevenjb, 2011-12-01 23:29:18]

I'm about 95% certain that this is safe and will fix the problem. 

There appears to be a more fundamental problem where we are not loading any
certificates at all when not logged in, but fixing that is going to require a
more substantial effort.

Meanwhile, I will try to shift gears to test this, but if anyone else has a repo
handy they can test on a device, that might be faster.

[Charlie Lee, 2011-12-01 23:50:57]

lgtm

[zel, 2011-12-02 00:07:22]

http://codereview.chromium.org/8771019/diff/1/chrome/browser/chromeos/options...
File chrome/browser/chromeos/options/wifi_config_view.cc (left):

http://codereview.chromium.org/8771019/diff/1/chrome/browser/chromeos/options...
chrome/browser/chromeos/options/wifi_config_view.cc:190: virtual int
GetItemCount() {
if (!UserManager::Get()->user_is_logged_in())
  return what-ever-is-"Do not check";

http://codereview.chromium.org/8771019/diff/1/chrome/browser/chromeos/options...
chrome/browser/chromeos/options/wifi_config_view.cc:222: virtual int
GetItemCount() {
if (!UserManager::Get()->user_is_logged_in())
  return what-ever-is-"Do not check";

http://codereview.chromium.org/8771019/diff/1/chrome/browser/chromeos/options...
chrome/browser/chromeos/options/wifi_config_view.cc:392:
user_cert_combobox_->SetEnabled(user_cert_enabled &&
disable this if user is not logged in by adding condition:

  UserManager::Get()->user_is_logged_in() &&

http://codereview.chromium.org/8771019/diff/1/chrome/browser/chromeos/options...
chrome/browser/chromeos/options/wifi_config_view.cc:402:
server_ca_cert_combobox_->SetEnabled(ca_cert_enabled &&
disable this if user is not logged in by adding condition:

  UserManager::Get()->user_is_logged_in() &&

[stevenjb, 2011-12-02 01:26:33]

PTAL

http://codereview.chromium.org/8771019/diff/1/chrome/browser/chromeos/options...
File chrome/browser/chromeos/options/wifi_config_view.cc (left):

http://codereview.chromium.org/8771019/diff/1/chrome/browser/chromeos/options...
chrome/browser/chromeos/options/wifi_config_view.cc:222: virtual int
GetItemCount() {
On 2011/12/02 00:07:23, zel wrote:
> if (!UserManager::Get()->user_is_logged_in())
>   return what-ever-is-"Do not check";
This should now return "None installed" when not logged in (CertificatesLoaded()
will return true, but num_certs == 0)

http://codereview.chromium.org/8771019/diff/1/chrome/browser/chromeos/options...
chrome/browser/chromeos/options/wifi_config_view.cc:392:
user_cert_combobox_->SetEnabled(user_cert_enabled &&
On 2011/12/02 00:07:23, zel wrote:
> disable this if user is not logged in by adding condition:
> 
>   UserManager::Get()->user_is_logged_in() &&

HaveUserCerts() will now return false, so this will be disabled.

http://codereview.chromium.org/8771019/diff/1/chrome/browser/chromeos/options...
chrome/browser/chromeos/options/wifi_config_view.cc:402:
server_ca_cert_combobox_->SetEnabled(ca_cert_enabled &&
On 2011/12/02 00:07:23, zel wrote:
> disable this if user is not logged in by adding condition:
> 
>   UserManager::Get()->user_is_logged_in() &&

We should probably leave this enabled in case users need to differentiate
between "Use Default" and "Do Not Check". That is definitely the easiest / most
straight forward thing to do.

[zel, 2011-12-02 01:45:26]

http://codereview.chromium.org/8771019/diff/3003/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/cert_library.cc (right):

http://codereview.chromium.org/8771019/diff/3003/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/cert_library.cc:92: certificates_loaded_ = true;
that's not going to work since this same instance of this class is used before
and after user signs in. in other words, the constructor for this singleton
object will always end with certificates_loaded_ set to true since when the
first time we call it, user won't be signed in

[stevenjb, 2011-12-02 02:20:59]

http://codereview.chromium.org/8771019/diff/3003/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/cert_library.cc (right):

http://codereview.chromium.org/8771019/diff/3003/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/cert_library.cc:92: certificates_loaded_ = true;
On 2011/12/02 01:45:26, zel wrote:
> that's not going to work since this same instance of this class is used before
> and after user signs in. in other words, the constructor for this singleton
> object will always end with certificates_loaded_ set to true since when the
> first time we call it, user won't be signed in

That's already handled in RequestCertificates() - the first time it gets called
where UserManager::Get()->user_is_logged_in() is true,  it sets
certificates_loaded_ = false. This is just to handle the case where
Requestcertificates() doesn't get called.

[zel, 2011-12-02 02:37:03]

On 2011/12/02 02:20:59, Steven Bennetts wrote:
>
http://codereview.chromium.org/8771019/diff/3003/chrome/browser/chromeos/cros...
> File chrome/browser/chromeos/cros/cert_library.cc (right):
> 
>
http://codereview.chromium.org/8771019/diff/3003/chrome/browser/chromeos/cros...
> chrome/browser/chromeos/cros/cert_library.cc:92: certificates_loaded_ = true;
> On 2011/12/02 01:45:26, zel wrote:
> > that's not going to work since this same instance of this class is used
before
> > and after user signs in. in other words, the constructor for this singleton
> > object will always end with certificates_loaded_ set to true since when the
> > first time we call it, user won't be signed in
> 
> That's already handled in RequestCertificates() - the first time it gets
called
> where UserManager::Get()->user_is_logged_in() is true,  it sets
> certificates_loaded_ = false. This is just to handle the case where
> Requestcertificates() doesn't get called.

This is still wrong thing to do that will mask another issue - fact that we
haven't even try to load certs from read-only NSSDB.

[stevenjb, 2011-12-02 17:58:12]

gauravsh@ moved the RequestCertificates call that used to be in
WifiConfigView::Init. As I recall his change was designed to always request
certificates, instead of just if the UI needs them, but apparently that only
happens after login? It seems like that flow may need to be re-examined.

For now, I've tried a different approach - I created
CertLibrary::CertificatesLoading() which only returns false if certificates were
requested and are not loaded. This should restore the previous UI behavior and
make the UI code more robust when we make changes to how we load certificates.

[gauravsh, 2011-12-02 18:12:30]

tl;dr: certificates_loaded_ = true pre-login and certificates_loaded_ = true
after user logs in are 2 separate states, that should be split up and handled
appropriately.


My change only ensured that the cert list was re-populated soon after login.
This was to ensure that SPDY or others which needed these certificates weren't
relying on a UI trigger for the certificate to be loaded.

For WiFi certs that are shared system wide, you do need to ensure certificates
are loaded pre-login. However, we still need reload the certificates on login,
since specific hardware backed certs may be user-specific and their handles only
appear when their cryptohome gets loaded.

In terms of the logical flow, it might be advisable to split certificates_loaded
state into 2 separate states - one for loading system-wide certificates, and one
for user-specific certificates. The latter getting reset every time a logout
occurs.


On 2011/12/02 17:58:12, Steven Bennetts wrote:
> gauravsh@ moved the RequestCertificates call that used to be in
> WifiConfigView::Init. As I recall his change was designed to always request
> certificates, instead of just if the UI needs them, but apparently that only
> happens after login? It seems like that flow may need to be re-examined.
> 
> For now, I've tried a different approach - I created
> CertLibrary::CertificatesLoading() which only returns false if certificates
were
> requested and are not loaded. This should restore the previous UI behavior and
> make the UI code more robust when we make changes to how we load certificates.

[zel, 2011-12-02 18:19:57]

lgtm

[stevenjb, 2011-12-02 19:05:19]

On 2011/12/02 18:12:30, gauravsh wrote:
> tl;dr: certificates_loaded_ = true pre-login and certificates_loaded_ = true
> after user logs in are 2 separate states, that should be split up and handled
> appropriately.
> 
> 
> My change only ensured that the cert list was re-populated soon after login.
> This was to ensure that SPDY or others which needed these certificates weren't
> relying on a UI trigger for the certificate to be loaded.
> 
> For WiFi certs that are shared system wide, you do need to ensure certificates
> are loaded pre-login. However, we still need reload the certificates on login,
> since specific hardware backed certs may be user-specific and their handles
only
> appear when their cryptohome gets loaded.
> 
> In terms of the logical flow, it might be advisable to split
certificates_loaded
> state into 2 separate states - one for loading system-wide certificates, and
one
> for user-specific certificates. The latter getting reset every time a logout
> occurs.

Yes, I agree. We need a smarter certificate manager that provides not hardware
backed certs pre-login, and all certs post-login, with indicators when certs are
being loaded.

For now I think this will do, assuming I can get a build up and running to
verify that this works.

[Charlie Lee, 2011-12-02 21:16:36]

lgtm

[commit-bot: I haz the power, 2011-12-03 00:28:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/8771019/1004

[commit-bot: I haz the power, 2011-12-03 04:14:55]

Change committed as 112861