Extend the enterprise policy for SSL overrides to the "danger" command

Extend the enterprise policy for SSL overrides to the "danger" command

If overriding cert errors is disabed by policy, users shouldn't be able
to use the "danger" command to bypass it.

BUG=440949
R=meacer@chromium.org

Committed: https://crrev.com/2d3f65ef3c809cc17b16d556762e10878c9d5796
Cr-Commit-Position: refs/heads/master@{#330365}

[felt, 2015-05-05 18:18:33]

meacer,

PTAL?

I'm not entirely sure how to test this. The existing tests check to see whether
or not the proceed link is visible; I don't know of any tests that actually try
to proceed through by entering keystrokes. Any ideas? I'm stuck.

felt

[meacer, 2015-05-05 18:30:22]

On 2015/05/05 18:18:33, felt wrote:
> meacer,
> 
> PTAL?
> 
> I'm not entirely sure how to test this. The existing tests check to see
whether
> or not the proceed link is visible; I don't know of any tests that actually
try
> to proceed through by entering keystrokes. Any ideas? I'm stuck.
> 
> felt

Would it make sense to use RenderViewImpl::SendKeyEvent? Here is a test case
that does something similar:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

If that fails, I think you can also simulate key press events using
content::ExecuteScript with |new KeyboardEvent| JS calls, but I'm not sure if
that's going to work on interstitials.

[felt, 2015-05-15 18:40:42]

I added a test, I think this is the simplest & most robust way. PTAL.

[meacer, 2015-05-15 20:37:02]

On 2015/05/15 18:40:42, felt wrote:
> I added a test, I think this is the simplest & most robust way. PTAL.

I think you can also do |content::ExecuteScript| with
"window.sendDomAutomationMessage" or something to cover a couple more lines of
code, but meh.

LGTM once you resolve the comment about the cast.

[felt, 2015-05-15 20:38:24]

On 2015/05/15 20:37:02, Mustafa Emre Acer wrote:
> On 2015/05/15 18:40:42, felt wrote:
> > I added a test, I think this is the simplest & most robust way. PTAL.
> 
> I think you can also do |content::ExecuteScript| with
> "window.sendDomAutomationMessage" or something to cover a couple more lines of
> code, but meh.

I don't actually think that gets us any additional test coverage... since all
that would do is cause CommandReceived to fire, which we're already testing here

> LGTM once you resolve the comment about the cast.

I don't see a comment about a cast, did it not get mailed w/ the message?

[meacer, 2015-05-15 20:41:10]

On 2015/05/15 20:38:24, felt wrote:
> On 2015/05/15 20:37:02, Mustafa Emre Acer wrote:
> > On 2015/05/15 18:40:42, felt wrote:
> > > I added a test, I think this is the simplest & most robust way. PTAL.
> > 
> > I think you can also do |content::ExecuteScript| with
> > "window.sendDomAutomationMessage" or something to cover a couple more lines
of
> > code, but meh.
> 
> I don't actually think that gets us any additional test coverage... since all
> that would do is cause CommandReceived to fire, which we're already testing
here

Additional as in 10 lines of code handling sendDomAutomationMessage, so yeah no
benefit doing that really.

> 
> > LGTM once you resolve the comment about the cast.
> 
> I don't see a comment about a cast, did it not get mailed w/ the message?

I forgot how to rietveld.

[meacer, 2015-05-15 20:41:18]

https://codereview.chromium.org/1119963004/diff/60001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/1119963004/diff/60001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:3694: SSLBlockingPage*
interstitial_delegate = static_cast<SSLBlockingPage*>(
Do you need the cast here? |CommandReceived| is an |InterstitialPageDelegate|
method, so you should be fine without the cast.

If not, you might want to check the type of the interstitial before doing the
cast (via InterstitialPageDelegate::GetTypeForTesting).

[felt, 2015-05-15 20:44:43]

https://codereview.chromium.org/1119963004/diff/60001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/1119963004/diff/60001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:3694: SSLBlockingPage*
interstitial_delegate = static_cast<SSLBlockingPage*>(
On 2015/05/15 20:41:18, Mustafa Emre Acer wrote:
> Do you need the cast here? |CommandReceived| is an |InterstitialPageDelegate|
> method, so you should be fine without the cast.
> 
> If not, you might want to check the type of the interstitial before doing the
> cast (via InterstitialPageDelegate::GetTypeForTesting).

I need to friend class this test because CommandReceived is protected. It seemed
more appropriate to friend class the SSLBlockingPage than to friend class
something in content/. WDYT?

[meacer, 2015-05-15 20:53:41]

https://codereview.chromium.org/1119963004/diff/60001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/1119963004/diff/60001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:3694: SSLBlockingPage*
interstitial_delegate = static_cast<SSLBlockingPage*>(
On 2015/05/15 20:44:43, felt wrote:
> On 2015/05/15 20:41:18, Mustafa Emre Acer wrote:
> > Do you need the cast here? |CommandReceived| is an
|InterstitialPageDelegate|
> > method, so you should be fine without the cast.
> > 
> > If not, you might want to check the type of the interstitial before doing
the
> > cast (via InterstitialPageDelegate::GetTypeForTesting).
> 
> I need to friend class this test because CommandReceived is protected. It
seemed
> more appropriate to friend class the SSLBlockingPage than to friend class
> something in content/. WDYT?

I see. Then perhaps window.domAutomationController.send thingy is actually
useful since you don't need anyone's friendship? Looks like SSLBrowserTest uses
it too:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl...

I'll leave the decision to you. But if you want to keep the cast I think you
should check the type.

[felt, 2015-05-15 21:11:44]

https://codereview.chromium.org/1119963004/diff/60001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/1119963004/diff/60001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:3694: SSLBlockingPage*
interstitial_delegate = static_cast<SSLBlockingPage*>(
On 2015/05/15 20:53:41, Mustafa Emre Acer wrote:
> On 2015/05/15 20:44:43, felt wrote:
> > On 2015/05/15 20:41:18, Mustafa Emre Acer wrote:
> > > Do you need the cast here? |CommandReceived| is an
> |InterstitialPageDelegate|
> > > method, so you should be fine without the cast.
> > > 
> > > If not, you might want to check the type of the interstitial before doing
> the
> > > cast (via InterstitialPageDelegate::GetTypeForTesting).
> > 
> > I need to friend class this test because CommandReceived is protected. It
> seemed
> > more appropriate to friend class the SSLBlockingPage than to friend class
> > something in content/. WDYT?
> 
> I see. Then perhaps window.domAutomationController.send thingy is actually
> useful since you don't need anyone's friendship? Looks like SSLBrowserTest
uses
> it too:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl...

That test is flaky: the JS call is async, so it has to poll and then time out.
(And sometimes it does fail just bc of the timeout.) The Safe Browsing
interstitial does the equivalent test this way (and is not flaky).

> 
> I'll leave the decision to you. But if you want to keep the cast I think you
> should check the type.

Done.

[felt, 2015-05-15 21:13:41]

felt@chromium.org changed reviewers:
+ atwilson@chromium.org

[felt, 2015-05-15 21:13:41]

atwilson@chromium.org: Please review changes in c/b/policy/?

[meacer, 2015-05-15 21:26:58]

https://codereview.chromium.org/1119963004/diff/60001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/1119963004/diff/60001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:3694: SSLBlockingPage*
interstitial_delegate = static_cast<SSLBlockingPage*>(
On 2015/05/15 21:11:44, felt wrote:
> On 2015/05/15 20:53:41, Mustafa Emre Acer wrote:
> > On 2015/05/15 20:44:43, felt wrote:
> > > On 2015/05/15 20:41:18, Mustafa Emre Acer wrote:
> > > > Do you need the cast here? |CommandReceived| is an
> > |InterstitialPageDelegate|
> > > > method, so you should be fine without the cast.
> > > > 
> > > > If not, you might want to check the type of the interstitial before
doing
> > the
> > > > cast (via InterstitialPageDelegate::GetTypeForTesting).
> > > 
> > > I need to friend class this test because CommandReceived is protected. It
> > seemed
> > > more appropriate to friend class the SSLBlockingPage than to friend class
> > > something in content/. WDYT?
> > 
> > I see. Then perhaps window.domAutomationController.send thingy is actually
> > useful since you don't need anyone's friendship? Looks like SSLBrowserTest
> uses
> > it too:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl...
> 
> That test is flaky: the JS call is async, so it has to poll and then time out.
> (And sometimes it does fail just bc of the timeout.) The Safe Browsing
> interstitial does the equivalent test this way (and is not flaky).

Well, SafeBrowsingBlockingPageBrowserTest.RedirectInIFrameCanceled uses
GetCommand and is still flaky :) But that might be something else, so this is
still LGTM.

I think most of the flaky interstitial tests are flaky because they try to
interact with the interstitial too early, without first waiting the interstitial
load to complete. I suspect adding |WaitForInterstitialAttach| and
|WaitForRenderFrameReady| to the individual tests can fix some of them. I've
been meaning to go over the flaky tests and try this but haven't had a chance
yet.

> 
> > 
> > I'll leave the decision to you. But if you want to keep the cast I think you
> > should check the type.
> 
> Done.

https://codereview.chromium.org/1119963004/diff/80001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/1119963004/diff/80001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:3699:
interstitial_delegate->GetTypeForTesting());
nit: I think it's better to do this before doing the cast. If the runtime type
of the interstitial is of an incorrect type |interstitial_delegate| will be an
invalid pointer (or whatever it's called). Better to be safe, even though the
test will fail immediately in that case.

[felt, 2015-05-15 21:48:34]

https://codereview.chromium.org/1119963004/diff/80001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/1119963004/diff/80001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:3699:
interstitial_delegate->GetTypeForTesting());
On 2015/05/15 21:26:57, Mustafa Emre Acer wrote:
> nit: I think it's better to do this before doing the cast. If the runtime type
> of the interstitial is of an incorrect type |interstitial_delegate| will be an
> invalid pointer (or whatever it's called). Better to be safe, even though the
> test will fail immediately in that case.

Ah I now realize what you meant. Done.

[Andrew T Wilson (Slow), 2015-05-18 11:48:54]

LGTM

+tnagel fyi

[felt, 2015-05-18 14:25:40]

The CQ bit was checked by felt@chromium.org

[felt, 2015-05-18 14:25:41]

The patchset sent to the CQ was uploaded after l-g-t-m from meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/1119963004/#ps120001
(title: "Remove parens")

[commit-bot: I haz the power, 2015-05-18 14:26:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1119963004/120001

[commit-bot: I haz the power, 2015-05-18 15:26:13]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-18 15:27:10]

Patchset 7 (id:??) landed as
https://crrev.com/2d3f65ef3c809cc17b16d556762e10878c9d5796
Cr-Commit-Position: refs/heads/master@{#330365}