Add button to page info to revoke user certificate decisions.

chrome/browser/ui/website_settings/website_settings_ui.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_UI_WEBSITE_SETTINGS_WEBSITE_SETTINGS_UI_H_
 #define CHROME_BROWSER_UI_WEBSITE_SETTINGS_WEBSITE_SETTINGS_UI_H_
 
 #include <string>
 #include <vector>
 
@@ skipping 77 matching lines @@
     // The ID is the server certificate of a secure connection or 0.
     int cert_id;
     // Signed Certificate Timestamp ids and status
     content::SignedCertificateTimestampIDStatusList
         signed_certificate_timestamp_ids;
     // Status of the site's connection.
     WebsiteSettings::SiteConnectionStatus connection_status;
     // Textual description of the site's connection status that is displayed to
     // the user.
     std::string connection_status_description;
+    // Set when the user has explicitly bypassed an SSL error for this host.
+    // When |certificate_decision_made| is true, the connection area of the
+    // page info will include an option for the user to revoke their decision to
+    // bypass the SSL error for this host.

[Peter Kasting, 2014/08/07 03:56:58]

This comment is much better now!  Maybe the variable should be named
"[have_]bypassed_ssl_error" or "show_revoke_button" or something to be more
directly connected to the description you give.

It seems a little odd as well that this comment sounds like this is only about
"bypassing an error" but in the code we check if the user has allowed _or
denied_ a cert.  Is that an actual mismatch or am I just misinterpreting the
code?

[jww, 2014/08/08 16:48:50]

have_bypassed_ssl_error sounds great to me.
Good point. Unfortunately, it's a little of both. We do check both because,
theoretically, there is "Allow" and "Deny," but in practice, "Deny" is never
called. I suppose there is a theoretic world in which we implement a content
setting to "always deny" or something like that, but it doesn't exist.

I'm ambivalent about how to express this in the comment. I suppose it should
probably say something additionally like, "Set when the user has explicitly
bypassed an SSL error for this host or explicitly denied it (the latter of which
is not currently possible in the Chrome UI)." What do you think?

[Peter Kasting, 2014/08/08 17:15:12]

If denying isn't possible in the Chrome UI, I think the underlying APIs here
should reflect that.  So a function like "AllowOrDenyX" would be named "AllowX"
instead.  The confusion you're struggling with comes from the fact that the
existing APIs you're calling claim to expose a distinction that never
practically occurs, so the bug is really with them rather than your new code. 
Once the APIs only talk about "allow", the names and comments here should be
fairly obvious.

I hope making these changes is relatively straightforward.  If there are a lot
of then maybe this would have to happen as a separate patch.

[jww, 2014/08/11 19:21:28]

There might be a reason to blacklist specific certs someday, so I'd like to keep
that option open in the API (additionally, a different embeder other than Chrome
might want to do this), but I agree it's definitely confusing. Addtionally, the
underlying implementation would work if "Deny" was ever called, so if anyone
ever used "Deny," they would need to go and change a renamed "HasAllowedCert"
method to be more complex.

So instead of making "HasAllowedCert," I've renamed it to be slightly more
generic ("HasUserDecision"), which I hope makes it less confusing.This also has
the advantage that if someone does start using "Deny" to blacklist certs, the UI
will still do the correct thing. Let me know if that sounds okay, or if you
think we need to change the API further (I also changed the Revoke* methods to
be clearer).

[Peter Kasting, 2014/08/11 20:24:20]

This is dangerous reasoning.  If this doesn't come to pass, we've basically made
the code more confusing than it needs to be for no benefit.  Even if it does
come to pass, we still pay that cost in the meantime.  My argument is always: if
you have strong reason to believe this _will_ be changed within a definite
timeframe (e.g. six months), then this is OK; otherwise please make the design
reflect only the capabilities you need right now.  We can always expand the API
later if we do decide we need to implement blacklisting.

To be clear, I don't think your current design is hideously awful and the end of
the world.  I just think it could be even better, and probably should be better,
as long as the reason for having it this way is purely hypothetical.

+    bool certificate_decision_made;
   };
 
   typedef std::vector<CookieInfo> CookieInfoList;
   typedef std::vector<PermissionInfo> PermissionInfoList;
 
   virtual ~WebsiteSettingsUI();
 
   // Returns the UI string for the given permission |type|.
   static base::string16 PermissionTypeToUIString(ContentSettingsType type);
 
@@ skipping 54 matching lines @@
   virtual void SetFirstVisit(const base::string16& first_visit) = 0;
 
   // Selects the tab with the given |tab_id|.
   virtual void SetSelectedTab(TabId tab_id) = 0;
 };
 
 typedef WebsiteSettingsUI::CookieInfoList CookieInfoList;
 typedef WebsiteSettingsUI::PermissionInfoList PermissionInfoList;
 
 #endif  // CHROME_BROWSER_UI_WEBSITE_SETTINGS_WEBSITE_SETTINGS_UI_H_