Closed Bug 823385 Opened 12 years ago Closed 7 years ago

When requesting permissions to a WebAPI that has PROMPT_ACTION, the prompt should show data usage intention in the permission description

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g18-)

RESOLVED WONTFIX
Tracking Status
b2g18 - ---

People

(Reporter: jsmith, Assigned: marta)

References

Details

Attachments

(3 files, 7 obsolete files)

Build: B2G 18 12/19/2012 Device: Unagi Steps: 1. Request access to a WebAPI that requires PROMPT_ACTION with a description provided - example below: "permissions": { "device-storage:pictures": { "access": "readonly", "description": "Can read what your pictures are from your device" }, "contacts": { "access": "readwrite", "description": "Come see my contacts" } }, Expected: The permission prompt should show data usage intention for the WebAPI. For example, if I requested access to my pictures storage, I should see in the prompt the "description" provided in the prompt to give clear indication to the user what this app intends to do with my data. Actual: No data usage intention is seen in the permission prompt. I originally thought this was a security requirement, but apparently it's not working right now.
Into triage since this was originally a security requirement. Someone from security should comment here to determine if they are or are not comfortable with shipping without this.
blocking-basecamp: --- → ?
Flags: needinfo?(ladamski)
We discussed this back and forth a whole lot on the various mailing lists, but I can't remember what the ultimate decision was. Argument against: It's probably a bad idea to show the description for non-privileged (or at least non-signed) apps since the description is totally unverified. So this would be prime relestate to try to social engineer the user. Argument for: If someone has reviewed the description, it's silly not to display it since it can provide valuable information to the user. This is one of the prime complaints about the android security model, that authors have no way to motivate why they need a particular privilege. Argument against: It's inconsistent to only show it for privileged apps, but not for "normal" apps. Argument for: If we can make it clear in the UI that it's a developer-provided description, then we might be able to always display it.
Triage: BB- , tracking-b2g+. UI Prompt
blocking-basecamp: ? → -
tracking-b2g18: --- → +
(In reply to Joe Cheng from comment #3) > Triage: BB- , tracking-b2g+. UI Prompt Irrelevant triage argument. This talking about data usage intention. This requires a product decision to cut this feature. Back into triage it goes.
blocking-basecamp: - → ?
tracking-b2g18: + → ---
Flags: needinfo?(pdolanjski)
I'll be sad if we don't have this as its a nice differentiating improvement for our model, but it doesn't actually expose the user to additional risk if we ship without this. It will likely just makes it harder for the user to be comfortable granting permissions. In terms of behavior, yes we should show it for privileged apps but not unprivileged (i.e. unreviewed/unsigned) apps.
Flags: needinfo?(ladamski)
Sounds like we shouldn't block then. But we should probably track it.
blocking-basecamp: ? → ---
tracking-b2g18: --- → ?
Flags: needinfo?(pdolanjski)
More appropriate for a v2 given scope, so no need to track.
Assignee: nobody → marta
Posible solution to this bug: https://github.com/mozilla-b2g/B2G/pull/261
Attached patch Proposed solution to the bug (obsolete) — Splinter Review
Pull request posted below by one of my students, but now moving it for review request.
Attachment #780960 - Flags: review?(timdream)
(In reply to marta from comment #10) > Created attachment 780960 [details] [diff] [review] > Proposed solution to the bug > > Pull request posted below by one of my students, but now moving it for > review request. Note - this pull request needs to be opened up against Gaia, not B2G.
(In reply to Jason Smith [:jsmith] from comment #11) > (In reply to marta from comment #10) > > Created attachment 780960 [details] [diff] [review] > > Proposed solution to the bug > > > > Pull request posted below by one of my students, but now moving it for > > review request. > > Note - this pull request needs to be opened up against Gaia, not B2G. Or actually - the gaia pieces should have a gaia pull request, where as the gecko pieces should have a diff file included here.
Comment on attachment 780960 [details] [diff] [review] Proposed solution to the bug Hi, Thank you for the contribution. Unfortunately it's not possible to review the patch as-is; you would have to provide patches separately for Gaia and Gecko. Please find the Gaia here: https://github.com/mozilla-b2g/gaia and basic documentation on how to submit patch here: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Platform/Gaia/Hacking For Gecko, here is a link as the starting point: https://developer.mozilla.org/en-US/docs/Developer_Guide
Attachment #780960 - Flags: review?(timdream)
Attached file Gaia pull request (obsolete) —
Attachment #781558 - Flags: review?(timdream)
Attached patch 823385.patch (obsolete) — Splinter Review
Attachment #781558 - Attachment is obsolete: true
Attachment #781558 - Flags: review?(timdream)
Attachment #781560 - Flags: review?(timdream)
Attachment #781558 - Attachment is obsolete: false
Attachment #781558 - Flags: review?(timdream)
Comment on attachment 781560 [details] [diff] [review] 823385.patch This patch is not prepared correctly. Also I am not be able to review this part of code -- please find :vingtetun.
Attachment #781560 - Flags: review?(timdream)
Comment on attachment 781558 [details] [review] Gaia pull request Generally looks good but you need to find another reviewer. Also, please squash your commits into one with an acceptable commit message.
Attachment #781558 - Flags: review?(timdream) → feedback+
The string is not localizable. Is it acceptable to show always english permission description to the user? Does this help non-English user to understand the permission request?
Attachment #781560 - Flags: review?(21)
Attachment #781558 - Flags: review?(21)
What Alive says in #c18 needs to be answered before going further here. What is the way to translate the permission description here? The usual way to localize a string from the manifest is to use the |locales| field of the manifest. See https://developer.mozilla.org/en-US/docs/Web/Apps/Manifest#locales Based on http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#510 I think that it is supposed to work. Can you verify this?
Comment on attachment 781558 [details] [review] Gaia pull request I made some comments on github.
Attachment #781558 - Flags: review?(21)
Comment on attachment 781560 [details] [diff] [review] 823385.patch Can you provide a diff of the change? The current patch is displayed like if you have just created the file - it makes it hard for me to understand what has been change. I assume that you the biggest part of this patch should be to expose permDescription. First let's rename it descrition since the event is already about a permission. Second look at my comment in github about |isApp| versus |appType|. Third you will have to check about localization and ensure it works. Thanks for trying to solve this issue though :) I remove the r? flag. Feel free to ask again once you think the patch is ready for a new review.
Attachment #781560 - Flags: review?(21)
Attached patch 823385.patch (obsolete) — Splinter Review
Attachment #780960 - Attachment is obsolete: true
Attachment #781558 - Attachment is obsolete: true
Attachment #781560 - Attachment is obsolete: true
Attachment #784988 - Flags: review?(21)
Attached file Gaia pull request (obsolete) —
Attachment #784989 - Flags: review?(21)
One question about the translation. How do you expect it to be done? Should the developer supply strings in different languages and than the notification should search for the one that fits to the user settings? (exemplary file attached) Or maybe it would be better to say that the developer introduces the description only in English and we should check the local settings of the phone and translate it on the run (is it even possible on FxOS)?
Flags: needinfo?(21)
Hi contributor. About manifest property l10n, see this manifest for reference https://github.com/mozilla-b2g/gaia/blob/master/apps/music/manifest.webapp#L35 The object of locales[current_language] would replace the original property in whole manifest.
Yes, but this does not explain where and how will the rational be given - will it be an additional field to the "locales"? eg. "en-US": { "name": "Music", "description": "Gaia Music" "rational": "Need access to the music database" }, or will it be in the part "permissions" eg. "permissions": { "storage":{ "description": {en: "need to play the music"}}},
Manifests have a localization method which is used for all properties. The way you use it is something like: "permissions": { "contacts": { "description": "To share highscore with your friends.", "access": "readonly" } }, "locales": { "se-SV": { "permissions": { "contacts": { "description": "För att dela dina högsta poäng med dina vänner." } } } } If we don't already have a utility functions for reading localized data from the manifest, we really should add them.
What Jonas says :) Sorry I forgot to explain it in my last comment. (In reply to marta from comment #28) > Yes, but this does not explain where and how will the rational be given - > will it be an additional field to the "locales"? eg. > "en-US": { > "name": "Music", > "description": "Gaia Music" > "rational": "Need access to the music database" > }, > or will it be in the part "permissions" eg. > "permissions": { > "storage":{ "description": {en: "need to play the music"}}},
Comment on attachment 784989 [details] [review] Gaia pull request Looks like there is a bit of misunderstanding here. The gaia system app isn't chrome. So you can't access things like |Ci.nsIPrincipal.APP_STATUS_PRIVILEGED|. You probably need to bubble the information you want from chrome. Also, the |Provided by developer: | string needs to be internationalized (a good example of how this work is the permissions names). Finally we should maybe reuse the "more info box" from bug 901383 here.
Attachment #784989 - Flags: review?(etienne) → review-
Comment on attachment 784988 [details] [diff] [review] 823385.patch Review of attachment 784988 [details] [diff] [review]: ----------------------------------------------------------------- ::: gecko/b2g/components/ContentPermissionPrompt.js @@ +193,5 @@ > let details = { > type: "permission-prompt", > permission: request.type, > + //text to show when permission does not contain any description in the manifest > + description: "Description not found", we probably want this to be undefined when the developer didn't provide a description. Otherwise we would display an english string for everybody.
Attachment #784988 - Flags: review?(etienne)
(In reply to Jonas Sicking (:sicking) from comment #29) > If we don't already have a utility functions for reading localized data from > the manifest, we really should add them. We have [1], but we probably to add some support for the permission description. [1] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#535
Hey :aalvarezmartinez, lots of new stuff here :) To sum up: * We need to follow the manifest localization method mentioned in Comment 29. * We need to add support for getting a localized permission description in AppUtils.jsm * ContentPermissionPrompt.js should send a mozChromeEvent with details.description undefined when the developer did not provide a description * ContentPermissionPrompt.js should send a mozChromeEvent with a flag indicating that the "Provided by the developer" string should be displayed (since the gaia system app wont be able to perform this check) * The "Provided by the developer" key should be localized on the gaia side Hope this is clear enough to move forward with this bug, cheers!
Flags: needinfo?(21)
Attached patch 823385.patch (obsolete) — Splinter Review
Attachment #784988 - Attachment is obsolete: true
Attachment #784989 - Attachment is obsolete: true
Attachment #785358 - Attachment is obsolete: true
Attachment #803291 - Flags: review?(21)
Attachment #803291 - Flags: review?(fabrice)
Attachment #803291 - Flags: review?(etienne)
Attachment #803291 - Flags: review?(21)
Comment on attachment 803291 [details] [diff] [review] 823385.patch Review of attachment 803291 [details] [diff] [review]: ----------------------------------------------------------------- Note: you need to split up the gecko and gaia parts. I only reviewed the gecko parts. ::: gecko/b2g/components/ContentPermissionPrompt.js @@ +185,4 @@ > > let principal = request.principal; > let isApp = principal.appStatus != Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED; > + let isWebApp = principal.appStatus == Ci.nsIPrincipal.APP_STATUS_INSTALLED; I don't understand why you need this flag. APP_STATUS_INSTALLED means that this is a non privileged app, but it's still different from a web page. ::: gecko/dom/apps/src/AppsUtils.jsm @@ +511,4 @@ > let locale = chrome.getSelectedLocale("global").toLowerCase(); > this._localeRoot = this._manifest; > > + if (this._manifest.locales) { Can you add a comment block describing the algorithm you implemented here? @@ +515,2 @@ > let lang = locale.split('-')[0]; > + for (let manLoc in this._manifest.locales){ nit: formatting should be: (...locales) { @@ +515,3 @@ > let lang = locale.split('-')[0]; > + for (let manLoc in this._manifest.locales){ > + if ( manLoc.toLowerCase() == locale ){ nit: if (manLoc.... == locale) { @@ +520,5 @@ > + } else if ( manLoc.split('-')[0] == lang ){ > + // try with the language part of the locale ("en" for "en-GB") only > + if (lang != locale && this._manifest.locales[lang]){ > + this._localeRoot = this._manifest.locales[lang]; > + // try with the language part of the locales ("en-US" for "en-GB") Move this comment in the branch of code it's describing. Also, Start comments with a capital letter and end with a full stop. @@ +521,5 @@ > + // try with the language part of the locale ("en" for "en-GB") only > + if (lang != locale && this._manifest.locales[lang]){ > + this._localeRoot = this._manifest.locales[lang]; > + // try with the language part of the locales ("en-US" for "en-GB") > + } else{ nit: } else { @@ +586,4 @@ > return {}; > }, > > + permissionDescription: function(permission){ nit: function(aPermission) { @@ +589,5 @@ > + permissionDescription: function(permission){ > + let permissions = this._localeProp("permissions"); > + if (!permissions){ > + permissions = this._manifest.permissions; > + } _localeProp already takes care of the _manifest fallback. So if permissions is null, you must return null. @@ +590,5 @@ > + let permissions = this._localeProp("permissions"); > + if (!permissions){ > + permissions = this._manifest.permissions; > + } > + for (let perm in permissions){ nit: ) { @@ +592,5 @@ > + permissions = this._manifest.permissions; > + } > + for (let perm in permissions){ > + if (perm == permission) > + return permissions[perm].description; Even for single line ifs, use: if () { doThat(); } @@ +594,5 @@ > + for (let perm in permissions){ > + if (perm == permission) > + return permissions[perm].description; > + } > + return {}; Doesn't that return a string? I think you want |return null|
Attachment #803291 - Flags: review?(fabrice) → feedback+
Flags: sec-review?(ptheriault)
Comment on attachment 803291 [details] [diff] [review] 823385.patch Review of attachment 803291 [details] [diff] [review]: ----------------------------------------------------------------- (looked only at the gaia part) We should probably have a dedicated l10n key for "Description provided by developer: ", this way we could compose the string more easily, keep the l10n diff shorter and we'll be able to add this string _only_ if the developer gave a description. ::: gaia/apps/system/js/permission_manager.js @@ +58,5 @@ > + // The app hasn't permission description. This shouldn't happen > + // because permission's description is mandatory in manifests > + } else { //Avoid of show "undefined" when description is not present > + str = _(permissionID + '-appRequest', { 'app': detail.appName }); > + } Looks like we can simplify this and make it more readable with something like: ``` var requestType = detail.isWebApp ? 'webAppRequest' : 'appRequest'; str = _(permissionID + '-' + requestType, {app: detail.appName}); if (detail.description) { str += detail.description; } ```
Attachment #803291 - Flags: review?(etienne)
isWebApp is needed to show a "Description provided by developer:" before the description (localizable in gaia side), for those apps which aren't certified or priviledged.
Attachment #803291 - Attachment is obsolete: true
Attachment #807825 - Flags: review?(21)
Attached patch 823385gaia.patchSplinter Review
Attachment #807827 - Flags: review?(etienne)
Comment on attachment 807825 [details] [diff] [review] 823385gecko.patch Review of attachment 807825 [details] [diff] [review]: ----------------------------------------------------------------- ::: gecko/b2g/components/ContentPermissionPrompt.js @@ +185,4 @@ > > let principal = request.principal; > let isApp = principal.appStatus != Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED; > + let isWebApp = principal.appStatus == Ci.nsIPrincipal.APP_STATUS_INSTALLED; isUnprivilegedApp would be clearer, even if it's way longer. ::: gecko/dom/apps/src/AppsUtils.jsm @@ +596,5 @@ > + } > + } > + let permissions = this._manifest.permissions; > + // If permission not contains description in manifest locale > + // try to get the default permission description. _localeProp() will do the fallback to the default manifest permissions if there are no locale ones. So you don't need that, but should return null if |permissions| is falsy after this._localeProp("permissions");
Attachment #807825 - Flags: review?(21) → review-
(In reply to Fabrice Desré [:fabrice] from comment #40) > ::: gecko/dom/apps/src/AppsUtils.jsm > @@ +596,5 @@ > > + } > > + } > > + let permissions = this._manifest.permissions; > > + // If permission not contains description in manifest locale > > + // try to get the default permission description. > > _localeProp() will do the fallback to the default manifest permissions if > there are no locale ones. So you don't need that, but should return null if > |permissions| is falsy after this._localeProp("permissions"); That piece of code tries to cover the event in which permission is present in locale, but the description is not contained within it. In this case it tries to get the field by default instead of returning null. Maybe it would be clearer coded as follows? permissionDescription: function(aPermission) { let permissions = this._localeProp("permissions"); for (let perm in permissions) { if (perm == aPermission) { return permissions[perm].description; } } if (permissions != this._manifest.permissions) { // If permission is present in manifest locale // but it doesn't contain permission description // try to get the default permission description. let permissions = this._manifest.permissions; for (let perm in permissions) { if (perm == aPermission) { return permissions[perm].description; } } } // If the manifest not contains permission description // returns null. This shouldn't happen because permission's // description is mandatory in manifests. return null; },
(In reply to Agus from comment #41) > (In reply to Fabrice Desré [:fabrice] from comment #40) > > > ::: gecko/dom/apps/src/AppsUtils.jsm > > @@ +596,5 @@ > > > + } > > > + } > > > + let permissions = this._manifest.permissions; > > > + // If permission not contains description in manifest locale > > > + // try to get the default permission description. > > > > _localeProp() will do the fallback to the default manifest permissions if > > there are no locale ones. So you don't need that, but should return null if > > |permissions| is falsy after this._localeProp("permissions"); > > That piece of code tries to cover the event in which permission is present > in locale, but the description is not contained within it. In this case it > tries to get the field by default instead of returning null. > > Maybe it would be clearer coded as follows? > > permissionDescription: function(aPermission) { > let permissions = this._localeProp("permissions"); > for (let perm in permissions) { > if (perm == aPermission) { > return permissions[perm].description; > } > } > if (permissions != this._manifest.permissions) { > // If permission is present in manifest locale > // but it doesn't contain permission description > // try to get the default permission description. > let permissions = this._manifest.permissions; > for (let perm in permissions) { > if (perm == aPermission) { > return permissions[perm].description; > } > } > } > // If the manifest not contains permission description > // returns null. This shouldn't happen because permission's > // description is mandatory in manifests. > return null; > }, Ha, that's not your fault but I really think that duplicating all the permissions objects in locales is terribly error prone. Anyway, here's what I would do (untested, use at your own risk!) : permissionDescription:function(aPermission) { let permissions = this._localeProp("permissions"); // Check if we have the description in the localized permission. if (permissions[aPermission] && permissions[aPermission].description) { return permissions[aPermission].description; } // Fallback to the non-localized one. if (permissions !== this._manifest.permissions && this._manifest.permissions && this._manifest.permissions[aPermission] && this._manifest.permissions[aPermission].description) { return this._manifest.permissions[aPermission].description; } // Nothing found. return null; } We also need tests for these new features.
Comment on attachment 807827 [details] [diff] [review] 823385gaia.patch Review of attachment 807827 [details] [diff] [review]: ----------------------------------------------------------------- The code change looks good. We just need to add the corresponding test. Some pointers: - the tests for this part are in gaia/apps/system/test/unit/permission_manager_test.js - you will probably want to complete the handlePermissionPrompt suite - you should be able to check the |str| passed by making |stubReq| a spy [1] [1] http://sinonjs.org/docs/#spies ::: gaia/shared/locales/permissions/permissions.en-US.properties @@ +42,5 @@ > perm-geolocation-appRequest={{app}} would like to know your location. > perm-geolocation-webRequest={{site}} would like to know your location. > perm-geolocation-more-info = {{brandShortName}} uses data such as GPS, Wi-Fi, and mobile networks near you to help determine your approximate location. Data is sent to Mozilla and other service providers and will be used in the aggregate to improve those location databases. Data may be stored on your device and data collection may occur even when no apps are running. > + > +perm-dev-description=Description provided by developer: nit: "Description provided by the developer:" ::: gaia/test_apps/geoloc/manifest.webapp @@ +33,5 @@ > "name": "Geoloc", > + "description": "Enregistrement de géolocalisation", > + "permissions": { > + "geolocation": { > + "description": "Nécessaires pour vous guider." nit: "Nécessaire pour vous guider" without the S. @@ +36,5 @@ > + "geolocation": { > + "description": "Nécessaires pour vous guider." > + }, > + "device-storage:sdcard": { > + "description": "Nécessaires pour la lecture des cartes." nit: idem
Attachment #807827 - Flags: review?(etienne) → feedback+
Comment on attachment 807827 [details] [diff] [review] 823385gaia.patch Review of attachment 807827 [details] [diff] [review]: ----------------------------------------------------------------- ::: gaia/apps/system/js/permission_manager.js @@ +131,5 @@ > str = _(permissionID + '-appRequest', { 'app': detail.appName }); > + if (detail.description) { > + if (detail.isWebApp){ > + str += ' ' + _('perm-dev-description'); > + } Why would we only show this for web apps? The description is always provided by the developer, so shouldn't we always show "Description is provider by the developer"? (as per comment 2). @@ +132,5 @@ > + if (detail.description) { > + if (detail.isWebApp){ > + str += ' ' + _('perm-dev-description'); > + } > + str += ' ' + detail.description; We need to be a bit more careful with the description I think. If nothing else, we need to check length, and handle the situation when the description is bigger than the space that is shown. Maybe show it in a scrollable input box or something too. (question for UI people maybe?)
Sorry for the drive by comment, but can you explain how we deal with the situation when the permissions specified in the locales section differs from the the permissions attribute in the root of the manifest? I assume that locales permissions is ONLY used for text and differences are just ignored.
Flags: needinfo?(marta)
Not sure who the best person to provide the answer to my above question is, so I will needinfo both of you ;) PS apart from these two questions, this approach looks good to me. Any chance you could provide a screenshot just to indicate what this looks like?
Flags: needinfo?(aalvarezmartinez)
Sorry for the delay in response. Text is always obtained from locales if the user language is present in them and contains description for the permission, if permission is not present on the locale part, or it doesn't contains description, the description is extracted from the root of the manifest, just like in the situation in which the user's language is not present.
Flags: needinfo?(marta)
Flags: needinfo?(aalvarezmartinez)
Attached image Screenshot
(In reply to Paul Theriault [:pauljt] On PTO back 28th Oct from comment #44) > Comment on attachment 807827 [details] [diff] [review] > 823385gaia.patch > > Review of attachment 807827 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gaia/apps/system/js/permission_manager.js > @@ +131,5 @@ > > str = _(permissionID + '-appRequest', { 'app': detail.appName }); > > + if (detail.description) { > > + if (detail.isWebApp){ > > + str += ' ' + _('perm-dev-description'); > > + } > > Why would we only show this for web apps? The description is always provided > by the developer, so shouldn't we always show "Description is provider by > the developer"? (as per comment 2). It's shown only for webapps as a responsability disclaim to indicate that the content of the description is unverified and is provided by the developer. Anyway, if it's preferred that don't display description por webapps, and/or shows "Description provided by the developer" for all kind of app, or any other change I can easily do it. > @@ +132,5 @@ > > + if (detail.description) { > > + if (detail.isWebApp){ > > + str += ' ' + _('perm-dev-description'); > > + } > > + str += ' ' + detail.description; > > We need to be a bit more careful with the description I think. If nothing > else, we need to check length, and handle the situation when the description > is bigger than the space that is shown. Maybe show it in a scrollable input > box or something too. (question for UI people maybe?) It's already shown in a scrollable box. I have tested it even with a more than one million chars' description and it works properly.
Attachment mime type: text/plain → text/x-github-pull-request
Attachment mime type: text/plain → text/x-github-pull-request
Flags: sec-review?(ptheriault)
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: