Closed Bug 854940 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 23
Tracking Status
firefox23 --- verified
fennec 24+ ---

People

(Reporter: jsmith, Assigned: mhaigh)

References

Details

(Whiteboard: [A4A] [packagedapps])

Attachments

(1 file, 2 obsolete files)

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.
tracking-fennec: --- → ?
Priority: -- → P1
Summary: Permission prompts need to indicate the app name, not the origin of the web content → Permission prompts need to indicate the app name, not the origin of the content in an app
Whiteboard: [A4A]
Assignee: nobody → wjohnston
tracking-fennec: ? → 24+
Assignee: wjohnston → jhugman
Whiteboard: [A4A] → [A4A] [packagedapps]
Assignee: jhugman → mhaigh
Duplicate of this bug: 858414
Status: NEW → ASSIGNED
Blocks: 862014
Note: fit and finish we want this but it isn't core functionality.
Priority: P1 → P2
(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.
Priority: P2 → P1
Attached patch Proposed patch (obsolete) — Splinter Review
Attachment #739599 - Flags: feedback?(wjohnston)
Attachment #739599 - Flags: feedback?(mark.finkle)
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
Attachment #739599 - Flags: feedback?(mark.finkle) → feedback+
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?
Attachment #739599 - Flags: feedback?(wjohnston) → feedback+
Attachment #739599 - Attachment is obsolete: true
Attachment #741325 - Flags: feedback?(wjohnston)
Attachment #741325 - Flags: feedback?(mark.finkle)
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.
Attachment #741325 - Flags: feedback?(mark.finkle) → feedback+
Attachment #741325 - Flags: review?(mark.finkle)
Attachment #741325 - Flags: review?(mark.finkle)
Attachment #741325 - Flags: review+
Attachment #741325 - Flags: feedback?(wjohnston)
Keywords: checkin-needed
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.
Keywords: checkin-needed
Attachment #741325 - Attachment is obsolete: true
Keywords: checkin-needed
The patch is already headed to land on birch. No need to flag checkin-needed again.
Keywords: checkin-needed
Attachment #744612 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/64d41174028a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.