786203 - Permission Prompt should tell which app it comes from instead only gives the url

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[(inactive after 6/18) Alive Kuo [:alive]]

the mozChromeEvent of `permission-prompt` is fired at content window, therefore we couldn't tell which app the permission is coming from.
For example, I could write an appA ask geolocation permission in `setTimeout`,
if the user switches to appB, and the appA fires a mozChromeEvent in the meantime,
it looks like appB is asking permission.

`event.detail.url` is not reliable in browser app case.

Could the even be fired at right target to distinguish where the app it belong to?

Related gaia issue:
https://github.com/mozilla-b2g/gaia/issues/3378

Comment 1

[Andrew Overholt [:overholt]]

Josh, do you have any thoughts here from a UX perspective?

Comment 2

[Andrew Overholt [:overholt]]

Mounir brought up a good point:  what if an iframe within an app is requesting a permission?  We probably want to show the domain of the iframe and not the containing app in this case.

Comment 3

[Mounir Lamouri (:mounir)]

IMO, if we show the domain, it's less user friendly but still secure because the user will understand that "google.com" is expected from the "Google Maps" app.
If we show the app, it will be nicer for the user ("Google Maps" > "google.com") but less secure because in App A, the SocialNetwork.com iframe might ask for permissions and it will marked as App A asking for permissions.
The best solution would be to show the app name if URI matches the app's origin and show the domain otherwise. So "Google Maps" for the geoloc' request of the Google Maps app but "socialnetwork.com" would be shown for the permission request coming from the iframe in App A.

Given that we pass a principal for app's, we should get that information by using principal.getStatus(). If it's NOT_INSTALLED, we should show the domain, otherwise the app's name.

Comment 4

[Josh Carpenter [:jcarpenter]]

Just in case people have not seen them, specs are here: https://www.dropbox.com/sh/ug5dd6d7rub0p5x/a5r79NCwo3

> The best solution would be to show the app name if URI matches the app's origin and show the domain otherwise

Excellent point. Agreed. I will add this nuance to next version of specs (to come by end of this week).

Comment 5

[Josh Carpenter [:jcarpenter]]

I've incoporated this idea into the new Install flows:

* App Install specs: https://www.dropbox.com/sh/b0kyykhzcfkpm8b/ReFTX_E72X

The Install prompt shows both domain and app name. I'll add the same to our Update prompts, if people agree with this approach.

Comment 6

[Andrew Overholt [:overholt]]

Marking as a blocker due to security UI considerations.

Fabrice offered to handle the Gecko side of things.

Comment 7

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Josh Carpenter [:jcarpenter] from comment #5)
> I've incoporated this idea into the new Install flows:
> 
> * App Install specs: https://www.dropbox.com/sh/b0kyykhzcfkpm8b/ReFTX_E72X
> 
> The Install prompt shows both domain and app name. I'll add the same to our
> Update prompts, if people agree with this approach.

I don't think we should show both since it confuses the user as to what is asking for the permission, as mounir suggests above. I think one or the other is better - was there a reason for having both ?

Comment 8

[Josh Carpenter [:jcarpenter]]

I'm easy going on this, but one reason to show both is context. It's a bit jarring to present prompts devoid of any parent-app context.

Another nit-picky reason is accuracy. iframe permissions are subsidiaries of the parent app. If I grant geolocation access to iframe-d Google Maps in App A, that permission does not extend to Google Maps in App B. So displaying App name + URL more accurately reflects the system model.

Comment 9

[[:fabrice] Fabrice Desré]

Attachment: patch

With this patch, we always send:
- the principal origin
- a 'isApp' boolean to know if we're in a app or in some other content.

If this is an app, we also set an 'appName' property on the chrome event. This lets gaia build the UI depending on who ask for the permission.

I tested by loading a geolocation page from the browser (not an app case), and with the geolocation test in the UI Tests app (is an app case).

Comment 10

[Mounir Lamouri (:mounir)]

Comment on attachment 660241 [details] [diff] [review]
patch

Review of attachment 660241 [details] [diff] [review]:
-----------------------------------------------------------------

This patch sends the app name if needed but I doesn't seem to be used by anything. Is that simply the part 1 of a series of patches or do you expect the other part to be done by another patch? I'm not sure why we would like this patch as-is for the moment.

::: b2g/components/ContentPermissionPrompt.js
@@ +66,5 @@
> +
> +    if (!isApp) {
> +      browser.shell.sendChromeEvent(details);
> +      return;
> +    }

Could you do:
if (isApp) {
  // add .appName to |details|.
}

browser.shell.sendChromeEvent(details);

Comment 11

[[:fabrice] Fabrice Desré]

(In reply to Mounir Lamouri (:mounir) from comment #10)
> Comment on attachment 660241 [details] [diff] [review]
> patch
> 
> Review of attachment 660241 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch sends the app name if needed but I doesn't seem to be used by
> anything. Is that simply the part 1 of a series of patches or do you expect
> the other part to be done by another patch? I'm not sure why we would like
> this patch as-is for the moment.

The user is in gaia-land. I have a branch showing that it works, but that does not follow the design from UX. Tim, will you update gaia after this lands?

> ::: b2g/components/ContentPermissionPrompt.js
> @@ +66,5 @@
> > +
> > +    if (!isApp) {
> > +      browser.shell.sendChromeEvent(details);
> > +      return;
> > +    }
> 
> Could you do:
> if (isApp) {
>   // add .appName to |details|.
> }
> 
> browser.shell.sendChromeEvent(details);

No I can't, because we retrieve the name in the callback of an async call.

Comment 12

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b02b848613e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0b02b848613e

Comment 14

[Jason Smith [:jsmith]]

Going to sanity test this by doing the following:

1. Request access to geolocation from the browser
2. Request access to geolocation from a hosted web app

Obviously I'll do more testing on this eventually (including packaged apps when that's fixed), but this will at least make sure this patch at a sanity level works.

Comment 15

[Jason Smith [:jsmith]]

Verified on 11/26.