Closed
Bug 786203
Opened 12 years ago
Closed 12 years ago
Permission Prompt should tell which app it comes from instead only gives the url
Categories
(Firefox OS Graveyard :: General, defect)
Firefox OS Graveyard
General
Tracking
(blocking-basecamp:+, firefox18 fixed)
People
(Reporter: alive, Assigned: fabrice)
Details
(Whiteboard: [WebAPI:P2], [LOE:S])
Attachments
(1 file)
2.30 KB,
patch
|
mounir
:
review+
|
Details | Diff | Splinter Review |
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
Updated•12 years ago
|
blocking-basecamp: --- → ?
Comment 1•12 years ago
|
||
Josh, do you have any thoughts here from a UX perspective?
Keywords: uiwanted
Whiteboard: [blocked-on-input Josh Carpenter]
Comment 2•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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.
Updated•12 years ago
|
Whiteboard: [blocked-on-input Josh Carpenter]
Comment 6•12 years ago
|
||
Marking as a blocker due to security UI considerations. Fabrice offered to handle the Gecko side of things.
Assignee: nobody → fabrice
blocking-basecamp: ? → +
Updated•12 years ago
|
Whiteboard: [WebAPI:P2]
Comment 7•12 years ago
|
||
(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•12 years ago
|
||
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.
Assignee | ||
Comment 9•12 years ago
|
||
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).
Attachment #660241 -
Flags: review?(mounir)
Comment 10•12 years ago
|
||
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);
Attachment #660241 -
Flags: review?(mounir)
Assignee | ||
Comment 11•12 years ago
|
||
(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.
Updated•12 years ago
|
Attachment #660241 -
Flags: review+
Assignee | ||
Comment 12•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b02b848613e
Assignee | ||
Updated•12 years ago
|
Whiteboard: [WebAPI:P2] → [WebAPI:P2][LOE:S]
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0b02b848613e
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Updated•12 years ago
|
status-firefox18:
--- → fixed
Updated•12 years ago
|
Whiteboard: [WebAPI:P2], [LOE:S], [qa-] → [WebAPI:P2], [LOE:S]
Updated•12 years ago
|
QA Contact: jsmith
Comment 14•12 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•