Last Comment Bug 854940 - Permission prompts need to indicate the app name, not the origin of the content in an app
: Permission prompts need to indicate the app name, not the origin of the conte...
Status: VERIFIED FIXED
[A4A] [packagedapps]
:
Product: Firefox for Android
Classification: Client Software
Component: Web Apps (show other bugs)
: Trunk
: ARM Android
: P1 normal (vote)
: Firefox 23
Assigned To: Martyn Haigh (:mhaigh)
: Aaron Train [:aaronmt]
: Myk Melez [:myk] [@mykmelez]
Mentors:
Depends on:
Blocks: 862014
  Show dependency treegraph
 
Reported: 2013-03-26 09:49 PDT by Jason Smith [:jsmith]
Modified: 2013-05-06 11:56 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
24+


Attachments
Proposed patch (2.55 KB, patch)
2013-04-19 07:01 PDT, Martyn Haigh (:mhaigh)
mark.finkle: feedback+
wjohnston2000: feedback+
Details | Diff | Splinter Review
Incorporating feedback suggestions (6.18 KB, patch)
2013-04-24 07:58 PDT, Martyn Haigh (:mhaigh)
mark.finkle: review+
mark.finkle: feedback+
Details | Diff | Splinter Review
fixing patch header (4.95 KB, patch)
2013-05-02 07:03 PDT, Martyn Haigh (:mhaigh)
myk: checkin+
Details | Diff | Splinter Review

Description Jason Smith [:jsmith] 2013-03-26 09:49:47 PDT
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 1 Mark Finkle (:mfinkle) (use needinfo?) 2013-04-11 08:45:24 PDT
*** Bug 858414 has been marked as a duplicate of this bug. ***
Comment 2 Erin Lancaster [:elan] 2013-04-18 09:03:16 PDT
Note: fit and finish we want this but it isn't core functionality.
Comment 3 Jason Smith [:jsmith] 2013-04-18 09:06:01 PDT
(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) 2013-04-19 07:01:15 PDT
Created attachment 739599 [details] [diff] [review]
Proposed patch
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2013-04-19 20:30:41 PDT
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) 2013-04-23 13:11:22 PDT
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) 2013-04-24 07:58:26 PDT
Created attachment 741325 [details] [diff] [review]
Incorporating feedback suggestions
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2013-04-24 08:54:35 PDT
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] 2013-05-02 05:06:51 PDT
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) 2013-05-02 07:03:07 PDT
Created attachment 744612 [details] [diff] [review]
fixing patch header
Comment 11 Jason Smith [:jsmith] 2013-05-02 07:22:14 PDT
The patch is already headed to land on birch. No need to flag checkin-needed again.
Comment 12 Ryan VanderMeulen [:RyanVM] 2013-05-02 12:18:48 PDT
https://hg.mozilla.org/mozilla-central/rev/64d41174028a

Note You need to log in before you can comment on or make changes to this bug.