Refactor WebsiteSettings to operate on a SecurityInfo

Refactor WebsiteSettings to operate on a SecurityInfo

Previously, WebsiteSettings operated on a content::SSLStatus. This gave
us no convenient place to share policies and calculations that
WebsiteSettings shared with other //chrome security UI elements: for
example, SHA1 deprecation and ChromeOS policy certs.

This CL refactors WebsiteSettings to operate on a
SecurityStateModel::SecurityInfo instead of a content::SSLStatus. The
SecurityInfo object already contains information about, for example,
SHA1 deprecation, so that WebsiteSettings doesn't have to compute that
on its own.

BUG=528034
Committed: https://crrev.com/a3121f6b5613d73812729c8197f3dd7877fbefbe
Cr-Commit-Position: refs/heads/master@{#349772}

[estark, 2015-09-09 04:39:25]

estark@chromium.org changed reviewers:
+ palmer@chromium.org

[estark, 2015-09-09 04:39:25]

palmer: woo, another giant refactoring CL for you to take a look at. This is
phase 2 of https://codereview.chromium.org/1314843007/

The main benefit of this one is removing duplicated SHA1 and ChromeOS policy
cert calculations from WebsiteSettings.

This also contains a fix for bug 529569, but that is mostly unrelated. (Only
related in that I found the bug while writing unit tests to ensure that I hadn't
broken WebsiteSettings).

The vast majority of this CL is changing a chain of callsites to take a
SecurityStateModel::SecurityInfo instead of a content::SSLStatus. In most
callsites, we now pass the SecurityInfo for the current WebContents, as obtained
from SecurityStateModel::FromWebContents(). In render_view_context_menu.cc,
though, we compute a SecurityInfo for a subframe and pass that down to
WebsiteSettings (for "View Frame Info").

The part most likely to have bugs is WebsiteSettings::Init(), where I had to
translate the actual logic to operate on a SecurityInfo instead of a SSLStatus.
Added some unit tests to try to cover more code paths.

[estark, 2015-09-16 21:52:32]

Patchset #6 (id:100001) has been deleted

[estark, 2015-09-16 21:56:39]

palmer, this should be ready for you take a look at now, please.

My previous comments/description are still accurate, except that I took the
unrelated bug fix (crbug.com/529569) out into a separate CL
(https://codereview.chromium.org/1349843003/), to keep the changes separate and
to spread the review load around.

Thanks!

[palmer, 2015-09-17 20:23:01]

LGTM, 1 nit.

https://codereview.chromium.org/1314953009/diff/140001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/1314953009/diff/140001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings.cc:543:
SecurityStateModel::DISPLAYED_MIXED_CONTENT;
Nit: Re-phrase this for clarity as discussed in person.

[estark, 2015-09-18 00:26:32]

estark@chromium.org changed reviewers:
+ sky@chromium.org

[estark, 2015-09-18 00:26:33]

Thanks palmer!

sky, could you please do an OWNERS review?

Most of this CL is changing a chain of callsites to take a SecurityStateModel
instead of a content::SSLStatus. The former is a new object in chrome/ that
gives us a place to share code used by multiple chrome security UI elements. The
places with non-trivial changes are chrome/browser/renderer_context_menu/
chrome/browser/ui/website_settings/. Palmer has already done an OWNERS review
for the latter.

I know this is a big CL so let me know if there's any other context I can give
you that would be helpful. Thanks!

https://codereview.chromium.org/1314953009/diff/140001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/1314953009/diff/140001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings.cc:543:
SecurityStateModel::DISPLAYED_MIXED_CONTENT;
On 2015/09/17 20:23:01, palmer wrote:
> Nit: Re-phrase this for clarity as discussed in person.

Done.

[sky, 2015-09-18 15:53:00]

https://codereview.chromium.org/1314953009/diff/180001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1314953009/diff/180001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1750:
SecurityStateModel::SecurityInfo security_info;
How come this code is different than the rest? By that I mean the other sites
like more like 1702 above, you get the SecurityStateModel from a WebContents and
then call GetSecurityInfo()?

https://codereview.chromium.org/1314953009/diff/180001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/1314953009/diff/180001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings.cc:460: if
(security_info.sha1_deprecation_status !=
I know nothing about this code, other than to say it looks different, and given
that this is all security related I'm nervous. Chris, did you review all these
changes?

[estark, 2015-09-18 16:06:09]

https://codereview.chromium.org/1314953009/diff/180001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1314953009/diff/180001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1750:
SecurityStateModel::SecurityInfo security_info;
On 2015/09/18 15:53:00, sky wrote:
> How come this code is different than the rest? By that I mean the other sites
> like more like 1702 above, you get the SecurityStateModel from a WebContents
and
> then call GetSecurityInfo()?

All the other call sites are displaying WebsiteSettings for the current page.
That's what happens when you click on the lock icon and go to the Connection
tab, for example. Or when you right-click on the page and select View Page Info.

This callsite is handling the case when you right click on an iframe and select
View Frame Info. In that case WebsiteSettings opens up and shows you the
information for the main resource of the subframe, rather than the top-level
page.

You can see this difference in the original code too; in this callsite the
original code uses the the security_info from the IPC params (that is, the
security info for the resource on which the menu is showing), whereas the
original line 1697 uses the security info from the navigation entry of the
WebContents.

Happy to add a comment here if that would be helpful.

[sky, 2015-09-18 20:10:33]

On 2015/09/18 16:06:09, estark wrote:
>
https://codereview.chromium.org/1314953009/diff/180001/chrome/browser/rendere...
> File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):
> 
>
https://codereview.chromium.org/1314953009/diff/180001/chrome/browser/rendere...
> chrome/browser/renderer_context_menu/render_view_context_menu.cc:1750:
> SecurityStateModel::SecurityInfo security_info;
> On 2015/09/18 15:53:00, sky wrote:
> > How come this code is different than the rest? By that I mean the other
sites
> > like more like 1702 above, you get the SecurityStateModel from a WebContents
> and
> > then call GetSecurityInfo()?
> 
> All the other call sites are displaying WebsiteSettings for the current page.
> That's what happens when you click on the lock icon and go to the Connection
> tab, for example. Or when you right-click on the page and select View Page
Info.
> 
> This callsite is handling the case when you right click on an iframe and
select
> View Frame Info. In that case WebsiteSettings opens up and shows you the
> information for the main resource of the subframe, rather than the top-level
> page.
> 
> You can see this difference in the original code too; in this callsite the
> original code uses the the security_info from the IPC params (that is, the
> security info for the resource on which the menu is showing), whereas the
> original line 1697 uses the security info from the navigation entry of the
> WebContents.
> 
> Happy to add a comment here if that would be helpful.

No, that's fine. The only thing keeping me from approving this change is Chris
saying he's reviewed it all. I IMd him but he hasn't responded.

[sky, 2015-09-18 20:21:59]

Chris IMd and said he did. SO, rubber stamp LGTM

[palmer, 2015-09-18 20:23:06]

> No, that's fine. The only thing keeping me from approving this change is Chris
> saying he's reviewed it all. I IMd him but he hasn't responded.

Sory; for some reason I did not see your IM. Yes, I reviewed all the code.

[estark, 2015-09-18 20:44:01]

Thanks sky and palmer!

[estark, 2015-09-18 20:44:07]

The CQ bit was checked by estark@chromium.org

[estark, 2015-09-18 20:44:07]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1314953009/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2015-09-18 20:44:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1314953009/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1314953009/180001

[commit-bot: I haz the power, 2015-09-18 21:16:06]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2015-09-18 21:16:48]

Patchset 9 (id:??) landed as
https://crrev.com/a3121f6b5613d73812729c8197f3dd7877fbefbe
Cr-Commit-Position: refs/heads/master@{#349772}