MidiPermissionContext::GetPermission() returns BLOCK for insecure origins.

MidiPermissionContext::GetPermission() returns BLOCK for insecure origins.

This is fixing an issue with querying the permission returning 'prompt'
but requesting it would always fail on insecure origins.

It is also removing the special casing in PermissionContextBase and
instead have the midi-specific logic in MidiPermissionContext.

BUG=474545
Committed: https://crrev.com/b05018dde502ec43da128d6a98cc18ccf546a742
Cr-Commit-Position: refs/heads/master@{#331781}

[mlamouri (slow - plz ping), 2015-05-22 10:43:06]

mlamouri@chromium.org changed reviewers:
+ bauerb@chromium.org, ddorwin@chromium.org

[mlamouri (slow - plz ping), 2015-05-22 10:46:05]

mlamouri@chromium.org changed reviewers:
+ toyoshim@chromium.org

[mlamouri (slow - plz ping), 2015-05-22 10:46:05]

bauerb@, can you review the changes in:
 chrome/browser/content_settings/

ddorwin@, can you review the changes in:
 chrome/browser/media/

toyoshim@, FYI

[Bernhard Bauer, 2015-05-22 15:17:54]

lgtm

[ddorwin, 2015-05-22 16:18:59]

https://codereview.chromium.org/1146403004/diff/1/chrome/browser/media/midi_p...
File chrome/browser/media/midi_permission_context.cc (right):

https://codereview.chromium.org/1146403004/diff/1/chrome/browser/media/midi_p...
chrome/browser/media/midi_permission_context.cc:24:
!content::IsOriginSecure(requesting_origin)) {
This is something we'll probably have to deal with for multiple permissions in
the future.

Would it be better to put this logic in the base and add an
AreInsecureOriginsAllowed() pure virtual function that children must implement?
That way, each child implements a single bool and all will have the same
behavior (with no risk of forgetting to add it).

If Base needs to be concrete, the method could return false by default.

https://codereview.chromium.org/1146403004/diff/1/chrome/browser/media/midi_p...
File chrome/browser/media/midi_permission_context.h (right):

https://codereview.chromium.org/1146403004/diff/1/chrome/browser/media/midi_p...
chrome/browser/media/midi_permission_context.h:22: void
DecidePermission(content::WebContents* web_contents,
Should be protected like the parent class?

[mlamouri (slow - plz ping), 2015-05-22 18:16:00]

PTAL.

I've also created/moved some tests, see changes in
permission_context_base_unittest.cc and midi_permission_context_unittest.cc

https://codereview.chromium.org/1146403004/diff/1/chrome/browser/media/midi_p...
File chrome/browser/media/midi_permission_context.cc (right):

https://codereview.chromium.org/1146403004/diff/1/chrome/browser/media/midi_p...
chrome/browser/media/midi_permission_context.cc:24:
!content::IsOriginSecure(requesting_origin)) {
On 2015/05/22 at 16:18:59, ddorwin wrote:
> This is something we'll probably have to deal with for multiple permissions in
the future.
> 
> Would it be better to put this logic in the base and add an
AreInsecureOriginsAllowed() pure virtual function that children must implement?
That way, each child implements a single bool and all will have the same
behavior (with no risk of forgetting to add it).
> 
> If Base needs to be concrete, the method could return false by default.

I would happily do that when there will be more than one context doing that kind
of logic. It's actually something I was thinking about. However, at the moment,
Midi is the only one because the other ones check for the security origin at the
Blink level. I'm not a big fan of doing design for things that might happen in
the future.

https://codereview.chromium.org/1146403004/diff/1/chrome/browser/media/midi_p...
File chrome/browser/media/midi_permission_context.h (right):

https://codereview.chromium.org/1146403004/diff/1/chrome/browser/media/midi_p...
chrome/browser/media/midi_permission_context.h:22: void
DecidePermission(content::WebContents* web_contents,
On 2015/05/22 at 16:18:59, ddorwin wrote:
> Should be protected like the parent class?

Done.

[ddorwin, 2015-05-22 19:57:54]

I still think doing this in the base class is the better option. Let me know
what you think, then I'll review whatever you decide on.

https://codereview.chromium.org/1146403004/diff/1/chrome/browser/media/midi_p...
File chrome/browser/media/midi_permission_context.cc (right):

https://codereview.chromium.org/1146403004/diff/1/chrome/browser/media/midi_p...
chrome/browser/media/midi_permission_context.cc:24:
!content::IsOriginSecure(requesting_origin)) {
On 2015/05/22 18:15:59, Mounir Lamouri wrote:
> On 2015/05/22 at 16:18:59, ddorwin wrote:
> > This is something we'll probably have to deal with for multiple permissions
in
> the future.
> > 
> > Would it be better to put this logic in the base and add an
> AreInsecureOriginsAllowed() pure virtual function that children must
implement?
> That way, each child implements a single bool and all will have the same
> behavior (with no risk of forgetting to add it).
> > 
> > If Base needs to be concrete, the method could return false by default.
> 
> I would happily do that when there will be more than one context doing that
kind
> of logic. It's actually something I was thinking about. However, at the
moment,
> Midi is the only one because the other ones check for the security origin at
the
> Blink level. I'm not a big fan of doing design for things that might happen in
> the future.

OOC, what other permissions are checked at the Blink level? Via the individual
API paths or the Permissions API? If we make that assumption here, we should
probably have DCHECKs for it.

It's potentially not obvious to someone working on a feature that these methods
support the Permissions API and would need a check similar to the one in Blink.
An explicit pure virtual method would require that this be considered.

I understand the desire to avoid over-engineering for what might be, but a) we
do know that more features will require secure origins
(https://www.chromium.org/Home/chromium-security/deprecating-powerful-features...)
and b) the current solution uses inheritance when all that is needed is a
Boolean to choose the behavior. Even if YAGNI for other permissions, it seems
that avoiding the overrides would be the better option.

[mlamouri (slow - plz ping), 2015-05-23 13:49:01]

I've created https://codereview.chromium.org/1153873003 which I believe should
match your request. I would prefer to keep this CL with a small footprint so we
can send it to Beta if needed (the issue is actually developer facing).
Re-designing PermissionContextBase would be an unnecessary risk.

PTAL :)

[mlamouri (slow - plz ping), 2015-05-23 13:50:26]

Oh, I forgot. Very rare are the permissions restricted to secure origins today,
unfortunately. MIDI does check for secure origin in PermissionContext but Push
for example does Blink checks.
FWIW, the Permissions API is directly plugged into PermissionContext.

[Takashi Toyoshima, 2015-05-25 02:20:10]

Thank you for fixing the issue, but I didn't understand why this cleanup CL can
fix the issue.
What makes difference between before and after?

[mlamouri (slow - plz ping), 2015-05-25 10:47:22]

On 2015/05/25 at 02:20:10, toyoshim wrote:
> Thank you for fixing the issue, but I didn't understand why this cleanup CL
can fix the issue.
> What makes difference between before and after?

It's not a cleanup CL. MidiPermissionContext::GetPermissionStatus() returns
BLOCK if the origin isn't secure. This is the fix.

[Takashi Toyoshima, 2015-05-26 02:21:28]

Ah, that's the point I missed.
Thanks, LGTM.

[ddorwin, 2015-05-26 17:23:50]

LGTM % comment.

Thanks for creating the other CL!

https://codereview.chromium.org/1146403004/diff/40001/chrome/browser/media/mi...
File chrome/browser/media/midi_permission_context_unittest.cc (right):

https://codereview.chromium.org/1146403004/diff/40001/chrome/browser/media/mi...
chrome/browser/media/midi_permission_context_unittest.cc:135: insecure_url,
secure_url));
Isn't this impossible (secure embedding insecure)? Should this test be the
opposite order?

[mlamouri (slow - plz ping), 2015-05-28 13:10:11]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-05-28 13:10:11]

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

[mlamouri (slow - plz ping), 2015-05-28 13:10:22]

https://codereview.chromium.org/1146403004/diff/40001/chrome/browser/media/mi...
File chrome/browser/media/midi_permission_context_unittest.cc (right):

https://codereview.chromium.org/1146403004/diff/40001/chrome/browser/media/mi...
chrome/browser/media/midi_permission_context_unittest.cc:135: insecure_url,
secure_url));
On 2015/05/26 at 17:23:50, ddorwin wrote:
> Isn't this impossible (secure embedding insecure)? Should this test be the
opposite order?

It's possible. Also, the opposite wouldn't be useful because we want to test
insecure origins permission status.

[commit-bot: I haz the power, 2015-05-28 13:10:27]

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

[commit-bot: I haz the power, 2015-05-28 15:05:05]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-28 15:05:52]

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