854940 - Permission prompts need to indicate the app name, not the origin of the content in an app

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[Jason Smith [:jsmith]]

When web content requests access to a PROMPT_ACTION WebAPI, we currently fire a permission prompt that indicates that X origin is asking for access to X WebAPI. For hosted apps, this flow was okay because hosted apps essentially operate under a web-based origin (https://yourhost.com). For packaged apps, this is actually a problem - they do not have a concept of an app origin right now, so what the user ends up getting exposed to instead is app://{UUID} as the app origin when asking for a WebAPI permission. The app://{UUID} is completely non-understandable to an end-user, so we need to expose something different here.

What we did on FxOS here was that for apps in general, we indicate that app X is asking for access to X WebAPI for PROMPT_ACTION requests if you are within the origin of that app. If you are off the origin of the app (whether that be directly, iframe, etc), then we indicate the typical origin as usual. We should follow in alignment here on FxAndroid.

Comment 2

[Erin Lancaster [:elan]]

Note: fit and finish we want this but it isn't core functionality.

Comment 3

[Jason Smith [:jsmith]]

(In reply to Erin Lancaster [:elancaster] from comment #2)
> Note: fit and finish we want this but it isn't core functionality.

No. This breaks virtually any use of a permission prompt within a packaged app in our security model. The fact that the user doesn't know the origin of the content breaks something way too foundational. We held the release on this on b2g v1.0.

Comment 4

[Martyn Haigh (:mhaigh)]

Attachment: Proposed patch

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 739599 [details] [diff] [review]
Proposed patch

>diff --git a/mobile/android/chrome/content/WebAppRT.js b/mobile/android/chrome/content/WebAppRT.js

>         if (app.manifestURL == aUrl) {
>+	  BrowserApp.appName = app.manifest.name;

>         if (manifest.fullLaunchPath() == aUrl) {
>+	  BrowserApp.appName = app.manifest.name;

You have TABs in here. Fix the indent.

>diff --git a/mobile/android/components/ContentPermissionPrompt.js b/mobile/android/components/ContentPermissionPrompt.js

>+    let permissionRequestorName = chromeWin.BrowserApp.appName ? "'" + chromeWin.BrowserApp.appName + "'" : request.principal.URI.host;

naming nit: let's just use "requestor"

>+

No need for the blank line

>     let message = browserBundle.formatStringFromName(entityName + ".ask",
>-                                                     [request.principal.URI.host], 1);
>+                                                     [permissionRequestorName], 1);

While you're here, can you put these lines on a single line? I dislike 80 char breaks in /mobile JS files

>     chromeWin.NativeWindow.doorhanger.show(message,
>                                            entityName + request.principal.URI.host,
>                                            buttons, tab.id, options);

Same here.

Using the host here is probably a good idea since that is used as a cookie.

You also need to address:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/OfflineApps.js#27
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/WebrtcUI.js#86
(both of these files are lazy loaded into the browser.js scope so BrowserApp is easy to access)

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#5336

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 739599 [details] [diff] [review]
Proposed patch

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

Seems good.

::: mobile/android/chrome/content/WebAppRT.js
@@ +67,5 @@
>          // First see if this url matches any manifests we have registered
>          // If so, get the launchUrl from the manifest and we'll launch with that
>          //let app = DOMApplicationRegistry.getAppByManifestURL(aUrl);
>          if (app.manifestURL == aUrl) {
> +	  BrowserApp.appName = app.manifest.name;

Is there a good reason to not store the app object entirely?

Comment 7

[Martyn Haigh (:mhaigh)]

Attachment: Incorporating feedback suggestions

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 741325 [details] [diff] [review]
Incorporating feedback suggestions

>diff --git a/mobile/android/chrome/content/WebAppRT.js b/mobile/android/chrome/content/WebAppRT.js

>         if (app.manifestURL == aUrl) {
>+          BrowserApp.manifest = app.manifest;
>           BrowserApp.manifestUrl = aUrl;

>         if (manifest.fullLaunchPath() == aUrl) {
>+          BrowserApp.manifest = app.manifest;
>           BrowserApp.manifestUrl = app.manifestURL;

We should consider using BrowserApp.app instead of saving off pieces. But that can wait for a new bug.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/64d41174028a

I don't know how you created the patch, but the header information got a bit munged in the process.

Comment 10

[Martyn Haigh (:mhaigh)]

Attachment: fixing patch header

Comment 11

[Jason Smith [:jsmith]]

The patch is already headed to land on birch. No need to flag checkin-needed again.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64d41174028a