Permission UI events for apps

VERIFIED FIXED in Firefox 18

Status

()

defect
P1
normal
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: ladamski, Assigned: ddahl)

Tracking

(Blocks 1 bug, {feature})

unspecified
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-kilimanjaro:?, blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

(Whiteboard: [LOE:M])

Attachments

(1 attachment, 12 obsolete attachments)

When an explicit permission has been requested but not yet granted, the platform should fire an event to let Gaia know that a permission dialog is necessary.
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
blocking-basecamp: ? → +
Assignee: nobody → ddahl
Is there a feature page/spec for what the event names are and what the event target / observer data to handle/provide here?
Depends on: 770731
After looking at the webapps code and docs on MDN, it looks like the code for this bug will require patches from bug 770731 and bug 758269
Depends on: 758269
I don't think that's defined honestly.  Gregor is implementing the backend of the permissions manager, and Etienne is leading the Gaia implementation.  I think between the three of you, you can figure out a viable event model.
(In reply to Lucas Adamski from comment #4)
> I don't think that's defined honestly.  Gregor is implementing the backend
> of the permissions manager, and Etienne is leading the Gaia implementation. 
> I think between the three of you, you can figure out a viable event model.

Yep. Gotcha.

After a bit more digging, I think what we want to do here is notifyObservers or call the messageManager with a JSON string that describes the Webapp's desire to access a device or perform an action it was explicitly allowed to do via the app manifest's list of permissions. The permissions in the manifest will be added to the permissionManager via bug 773114. 

Gregor: are you writing a new interface that inherits from nsIPermissionMananger or are we using it directly?

A "chicken and egg" question: how do we catch the web app code before it accesess webAPIs/devices/etc that require permission? I assume this is what we are trying to build, but I can imagine there are APIs that the webapp can just call regardless, no?

I would like to see a list of the actual permission names we are requiring apps to get approval for.
After digging through a bunch of permissionManager bugs, patches and mxr, here is a concept that might work. I am still figuring out how nsIContentPermissionRequest fits in here...

// GAIA permission events concept
// For each capability/device API that we need to confirm user permission for:

// (Inside of b2g/chrome/content/shell.js)
// Listen for system permission requests and relay them to Gaia.
Services.obs.addObserver(function onSystemPermissionRequest(subject, topic, data) {
   let msg = JSON.parse(data);
   let origin = Services.io.newURI(msg.manifest, null, null).prePath;
   shell.sendChromeEvent({
     type: 'permission-request',
     url: msg.uri,
     origin: origin,
     manifest: msg.manifest,
     permission: msg.permission,
     action: msg.action // may be null or Ci.nsIPermissionManager.PROMPT_REMEMBER_ACTION as you will only ever get this notification if you need to prompt the user
   });
}, 'system-messages-permission-request', false);


// Device APIs/services calls need to check with the permissionsManager to see if a prompt is required:
// Example of a routine to run inside of each device or webAPI:

let permValue =
    Services.perms.testExactPermissionFromPrincipal(aPrincipal, aPermission);
switch (permValue) {
  case Ci.nsIPermissionManager.UNKNOWN_ACTION:
    Cu.reportError("Permission '" + aPermission + "' for " +
                   principal.origin + "is UNKNOWN_ACTION");
    break;
  case Ci.nsIPermissionManager.DENY_ACTION:
    Cu.reportError("Permission '" + aPermission + "' for " +
                   principal.origin + "denied");
    break;
  case Ci.nsIPermissionManager.PROMPT_ACTION:
  case Ci.nsIPermissionManager.PROMPT_REMEMBER_ACTION:
    let msg = JSON.stringify({
      type: 'permission-request',
      url: aUrl, // XXX: not sure where this comes from
      origin: aPrincipal.origin,
      manifest: aApp.manifest, // XXX: again, a bit hand-wavy where this comes from
      permission: aPermission,
      action: permValue
    });
    Services.obs.notifyObservers(null, "apps-permission-required", msg);
    break;
  default:
    break;
}

// Apps infrastructure needs to observe a topic like 'apps-permission-required' topic
// ifdefing here in case release targets have different topics
// Inside of Webapps.jsm
#ifdef MOZ_B2G_RIL
#ifdef MOZ_B2G_BT // Not sure of the best value here
Services.obs.addObserver(function onAppsPermissionRequired(subject, topic, data) {
  Services.obs.notifyObservers(null, "system-messages-permission-request", data);
}, 'apps-permission-required', false);
#endif
#endif

#ifdef ANDROID
// ...
#endif

#ifndef ANDROID
#ifndef MOZ_B2G_RIL
#ifndef MOZ_B2G_BT
// ...
#endif
#endif
#endif
Posted patch Patch 1 - no tests yet (obsolete) — Splinter Review
This patch is a stab at the 'ContentPermissionService' dougt talked about in an email: 

"5) Need to have a content permission service in the parent process that handles the child permission requests, (http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIContentPermissionPrompt.idl#52).  This service needs to handle both prompting as well as explicit permission granted by manifest files." 

We probably need a new bug for this patch
Attachment #650672 - Flags: feedback?(doug.turner)
No longer blocks: basecamp-security
Attachment #650672 - Flags: feedback?(anygregor)
Gregor: can you also look at the patch here, make sure we are not duplicating functionality. Also, should we file another bug for this? The patch does not belong here.
Gregor and I seem to have worked some things out here on irc...

More questions:

This would indicate to me that we set the permission permanently on any prompt, is that the case?
https://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/WebappsUI.js#91

In b2g, we don't do that https://mxr.mozilla.org/mozilla-central/source/b2g/components/ContentPermissionPrompt.js
Adding Josh... the last discussion we had was that we should show a "remember" box, that is checked on by default for privileged apps, and unchecked for web content and installed.
Posted patch Patch 3 - no tests yet (obsolete) — Splinter Review
depends on bug 758269 bug 770731
Attachment #650672 - Attachment is obsolete: true
Attachment #650672 - Flags: feedback?(doug.turner)
Attachment #650672 - Flags: feedback?(anygregor)
Attachment #653928 - Flags: feedback?(doug.turner)
Attachment #653928 - Flags: feedback?(anygregor)
QA Contact: jsmith
Whiteboard: [WebAPI:P1]
Apps Security specs are here. I'll be updating them over next few days, as per feedback in dev-gaia thread, and here: https://www.dropbox.com/sh/ug5dd6d7rub0p5x/a5r79NCwo3
Keywords: feature
Whiteboard: [WebAPI:P1] → [LOE:M][WebAPI:P1]
Comment on attachment 653928 [details] [diff] [review]
Patch 3 - no tests yet

Looks good to me!
Attachment #653928 - Flags: feedback?(anygregor) → feedback+
Blocks: 781605
Also blocking https://github.com/mozilla-b2g/gaia/issues/5121

David, do you have an idea when this will be ready for front-end consumption?
v3 specs uploaded: 

Gaia_AppSecurity_20120924.pdf — https://www.dropbox.com/sh/ug5dd6d7rub0p5x/a5r79NCwo3

And as per Dietrich, we are tracking at issue #5121. Please let me know if there are any questions.
(In reply to Dietrich Ayala (:dietrich) from comment #14)
> Also blocking https://github.com/mozilla-b2g/gaia/issues/5121
> 
> David, do you have an idea when this will be ready for front-end consumption?

No idea. Probably not this week.
Assignee: ddahl → nobody
Gregor, do you have time for this right now?
Assignee: nobody → anygregor
(In reply to Andrew Overholt [:overholt] from comment #17)
> Gregor, do you have time for this right now?

I will take it. Can't promise that I will have time for my other blockers now.
(In reply to Gregor Wagner [:gwagner] from comment #18)
> (In reply to Andrew Overholt [:overholt] from comment #17)
> > Gregor, do you have time for this right now?
> 
> I will take it. Can't promise that I will have time for my other blockers
> now.

I thought dietrich had someone in mind to take this bug. I will take it back.
Assignee: anygregor → ddahl
Posted patch Patch 4, WIP (obsolete) — Splinter Review
I need to change the name of the PermissionService to "PermissionPromptService"
Attachment #653928 - Attachment is obsolete: true
Attachment #653928 - Flags: feedback?(doug.turner)
Posted patch Patch 5 WIP - renamed service (obsolete) — Splinter Review
Attachment #665630 - Attachment is obsolete: true
Thanks David. Are you going to be able to close this over the weekend? If not, I can try to find someone to shepherd it the rest of the way.
(In reply to Dietrich Ayala (:dietrich) from comment #22)
> Thanks David. Are you going to be able to close this over the weekend? If
> not, I can try to find someone to shepherd it the rest of the way.

I will not. The dependent bug 758269 may not even make it
Posted patch Patch 6 WIP (obsolete) — Splinter Review
Not sure if this should be a JSM-as-singleton or an XPCOM service
Attachment #665766 - Attachment is obsolete: true
Attachment #666103 - Flags: feedback?(anygregor)
Posted patch Patch (obsolete) — Splinter Review
This builds. I think we need to have an API that needs to check its permission to use this service in order to test it. Since this needs to use the nsIContentPermissionPrompt in b2g/components, should the test be a marionette test or can I use Mochitests for a b2g desktop build?
Attachment #666103 - Attachment is obsolete: true
Attachment #666103 - Flags: feedback?(anygregor)
Posted patch WIP Patch 2 (obsolete) — Splinter Review
Marionette test is set up - a few issues here yet, but at a goog point to get some feedback.
Attachment #667224 - Attachment is obsolete: true
Attachment #667734 - Flags: feedback?(anygregor)
Posted patch Patch 4 (obsolete) — Splinter Review
I think this patch is at a point where the Gaia front end needs to support a checkbox in the permission prompt to remember a prompted permission request: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/permission_manager.js#L171

The marionette test is run like so:
$ testing/marionette/client/marionette$ python runtests.py --address=localhost:2828 ../../../../dom/permission/tests/marionette/test_permissions_prompt.js

You need to set up a python virtual env, etc... my notes are here:

# run the emulator:

~/code/moz/b2g-desktop/gaia$ ../build/dist/bin/b2g -profile profile

# get the marionette tests going:

cd ~/code/moz/b2g-desktop/src/testing/marionette/client/marionette_venv

. bin/activate

# run the tests:

(marionette_venv)ddahl@X220:~/code/moz/b2g-desktop/src/testing/marionette/client/marionette$ python runtests.py --address=localhost:2828 ../../../../dom/permission/tests/marionette/test_permissions_prompt.js
Attachment #667734 - Attachment is obsolete: true
Attachment #667734 - Flags: feedback?(anygregor)
Attachment #668174 - Flags: feedback?(etienne)
Attachment #668174 - Flags: feedback?(anygregor)
Comment on attachment 668174 [details] [diff] [review]
Patch 4

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

The mozChromeEvent/mozContentEvent dialog looks good to me.
Attachment #668174 - Flags: feedback?(etienne) → feedback+
I really have no idea how to test this reliably - or at all...

TEST-UNEXPECTED-FAIL : JavascriptException: Error: Access to 'app://communications.gaiamobile.org/contacts/' from script denied

I get this when trying to load the contacts app from a marionette test. The contacts app has to be loaded or any actual installed app that I can set a permission for dynamically in the test to prompt before running the checks that instantiate the ContentPermissionPrompt. This is because the ContentPermissionPrompt uses the old standby way of finding out what url we are "at": let browser = Services.wm.getMostRecentWindow("navigator:browser");

See: https://mxr.mozilla.org/mozilla-central/source/b2g/components/ContentPermissionPrompt.js#38

As it stands now, we can not check the permissions as we always get the current url, something like: 192.168.1.6:45679 - which has no registered app in the Webapps registry.

I test this by breaking on js::math_sin and adding a call to Math.sin() to the test and looking in sqlite while things are paused. Incidentally, the marionette tests do not appear to ever reset the databases before running like mochitest does, this seems like a bug to me.

Any ideas here? I will attach the latest patch.
Posted patch Latest Patch (obsolete) — Splinter Review
Attachment #668174 - Attachment is obsolete: true
Attachment #668174 - Flags: feedback?(anygregor)
Attachment #670159 - Flags: feedback?(anygregor)
Have been trying a new angle here, this should be testable via marionette python tests, however, you apparently cannot load the contacts app at all in an automated fashion: see bug 800011

sigh...

also, not sure if you can call navigator.mozPermissions.set() 

Unfortunately there is no existing permission that is already set to 'prompt' in the database of clean install of gaia
Depends on: 801075
Priority: -- → P1
Whiteboard: [LOE:M][WebAPI:P1] → [LOE:M]
Posted patch Patch for Review (obsolete) — Splinter Review
There are no tests as automating the test for this feels like a fool's errand until marionette has more functionality. This was manually tested by changing the contact permission to prompt via sqlite3 command line, starting gaia and adding a contact. the prompt is displayed to allow and optionally remember the permission.
Attachment #670159 - Attachment is obsolete: true
Attachment #670159 - Flags: feedback?(anygregor)
Attachment #671618 - Flags: review?(jonas)
David, I had tested your patch.
I tested with UITest->Geolocation test.
I saw rememberPermission is called, but next time when I use the same app to ask for geolocation,
I still got a mozChromeEvent and then surely the prompt is shown again.
event.detail.remember contains nothing.

Is this expected? Thanks.
(In reply to Alive Kuo [:alive] from comment #33)
> David, I had tested your patch.
> I tested with UITest->Geolocation test.
> I saw rememberPermission is called, but next time when I use the same app to
> ask for geolocation,
> I still got a mozChromeEvent and then surely the prompt is shown again.
> event.detail.remember contains nothing.
> 
> Is this expected? Thanks.

Probably as I am unsure if the geolocation code is using the PermissionPromptHelper API, which as far as I know, only contacts is using, see: http://mxr.mozilla.org/mozilla-central/source/dom/contacts/ContactManager.js#457
(In reply to David Dahl :ddahl from comment #34)
> (In reply to Alive Kuo [:alive] from comment #33)
> > David, I had tested your patch.
> > I tested with UITest->Geolocation test.
> > I saw rememberPermission is called, but next time when I use the same app to
> > ask for geolocation,
> > I still got a mozChromeEvent and then surely the prompt is shown again.
> > event.detail.remember contains nothing.
> > 
> > Is this expected? Thanks.
> 
> Probably as I am unsure if the geolocation code is using the
> PermissionPromptHelper API, which as far as I know, only contacts is using,
> see:
> http://mxr.mozilla.org/mozilla-central/source/dom/contacts/ContactManager.
> js#457

Right, the geolocation code isn't connected yet.
Duplicate of this bug: 781605
Jonas, this patch is waiting for review 6 days. Do you have ETA for review?
Comment on attachment 671618 [details] [diff] [review]
Patch for Review

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

This looks generally good to me, but someone that knows this code better needs to review the integration points. r=me if you fix the things below and get Gregor's or Fabrice's review.

::: b2g/components/ContentPermissionPrompt.js
@@ +35,5 @@
> +  function convertPermToAllow(aPerm, aAccess, aPrincipal)
> +  {
> +    let permName;
> +    if (aAccess) {
> +       permName = aPerm + "-" + aAccess;

Fix indentation.

@@ +54,5 @@
> +  // Expand the permission to see if we have multiple access properties to convert
> +  let access = PermissionsTable[aPermission].access;
> +  if (access) {
> +    for (let idx in access) {
> +      convertPermToAllow(aPermission, access[idx], aPrincipal);

Would be simpler to simply do 'aPermission + "-" + access[idx]' here. Up to you.

@@ +63,5 @@
> +  }
> +}
> +
> +function ContentPermissionPrompt() {
> +  dump("ContentPermissionPrompt: constr.\n\n");

Remove this dump

@@ +87,2 @@
>    prompt: function(request) {
> +    dump("prompt.prompt()...\n\n");

There's a few dumps in this function. Remove them.

::: dom/contacts/ContactManager.js
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +const DEBUG = true;

Revert this

::: dom/permission/PermissionPromptHelper.jsm
@@ +78,5 @@
> +    for (let idx in installedPerms) {
> +      // if any of the installedPerms are deny, run aCallbacks.cancel
> +      if (installedPerms[idx] == Ci.nsIPermissionManager.DENY_ACTION ||
> +          installedPerms[idx] == Ci.nsIPermissionManager.UNKNOWN_ACTION) {
> +        aCallbacks.cancel();

It's unfortunate that we are preventing "read" operations even if just "write" has been set to DENY_ACTION. This is fine for now since we don't get enough context in this function to know whether a "read" or "write" operation is about to be performed. But please file a followup on this.
Attachment #671618 - Flags: review?(jonas)
Attachment #671618 - Flags: review?(anygregor)
Attachment #671618 - Flags: review+
Attachment #674245 - Flags: review?(anygregor)
Comment on attachment 674245 [details] [diff] [review]
Updated Patch with Jonas' comments addressed

>       evt.target.removeEventListener(evt.type, contentEvent);
> 
>       if (evt.detail.type == "permission-allow") {
>-
>+        if (evt.detail.remember) {
>+          rememberPermission(request.type, request.principal);
>+        }
>         if (evt.detail.remember) {
>           Services.perms.addFromPrincipal(request.principal, request.type,
>                                           Ci.nsIPermissionManager.ALLOW_ACTION);
>         }
> 

combine?


>+++ b/dom/interfaces/permission/Makefile.in
>@@ -10,15 +10,16 @@ VPATH          = @srcdir@
> include $(DEPTH)/config/autoconf.mk
> 
> MODULE         = dom
> XPIDL_MODULE   = dom_permissionsettings
> GRE_MODULE     = 1
> 
> XPIDLSRCS =                             \
>             nsIDOMPermissionSettings.idl    \
>+						nsIPermissionPromptService.idl \
>             $(NULL)
> 

indentation 


>+
>+    let perm =
>+      permissionManager.testExactPermissionFromPrincipal(aRequest.principal,
>+                                                         aRequest.type);
>+    switch (perm) {
>+    case Ci.nsIPermissionManager.ALLOW_ACTION:
>+      aRequest.allow();
>+      break;

indentation


>diff --git a/dom/permission/tests/marionette/manifest.ini b/dom/permission/tests/marionette/manifest.ini
>new file mode 100644
>--- /dev/null
>+++ b/dom/permission/tests/marionette/manifest.ini
>@@ -0,0 +1,6 @@
>+[DEFAULT]
>+b2g = true
>+browser = false
>+qemu = true
>+
>+[test_permissions_prmopt.js]

Where are the tests?

Thx!
Attachment #674245 - Flags: review?(anygregor) → review+
Depends on: 805038
(In reply to Gregor Wagner [:gwagner] from comment #40)
> Comment on attachment 674245 [details] [diff] [review]

> >+++ b/dom/permission/tests/marionette/manifest.ini
> >@@ -0,0 +1,6 @@
> >+[DEFAULT]
> >+b2g = true
> >+browser = false
> >+qemu = true
> >+
> >+[test_permissions_prmopt.js]
> 
> Where are the tests?
Whoops, removing that manifest file in my next patch... see bug 805038 - punted the test for now.
Posted patch Patch for inbound (obsolete) — Splinter Review
Going to apply this to my local inbound repo before pushing
Attachment #671618 - Attachment is obsolete: true
Attachment #674245 - Attachment is obsolete: true
Attachment #671618 - Flags: review?(anygregor)
Attachment #674683 - Flags: review+
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Bugs aren't resolved until they land on m-c.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/88693039c414
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
This doesn't apply cleanly to Aurora. Does it need to be landed on top of something else or does it need a branch-specific patch?
(In reply to Ryan VanderMeulen from comment #46)
> This doesn't apply cleanly to Aurora. Does it need to be landed on top of
> something else or does it need a branch-specific patch?

I think you are correct, I just tried to apply it and there are too many differences in b2g/components/ContentPermissionPrompt.js - looks like we need to do some digging. I am traveling tomorrow afternoon, ping me early if you need any help.
RyanVM:

Looks like there are 3 intervening patches:

diff
browse
annotate	ab22c6590cfd
2012-10-22 22:22 -0400	Andrea Marchesini - Bug 803452 - Remember my choice for geolocation doesn't work. r=vingtetun
diff
browse
annotate	2cc0da49ccb5
2012-10-02 22:38 -0700	Fabrice Desré - Bug 796217 - Refactor Webapps.jsm and related files - Part 1: move DOMApplicationManifest outside of Webapps.jsm [r=gwagner]
diff
browse
annotate	0b02b848613e
2012-09-12 11:00 -0700	Fabrice Desré - Bug 786203 - Permission Prompt should tell which app it comes from instead only gives the url [r=mounir]
Bug 786203 landed before 18 moved to Aurora, no?
Bug 796217 isn't marked as blocking-basecamp+ nor has Aurora uplift been requested.
Bug 803452 landed on Aurora today.

I'll look tomorrow to see how this applies in the current state of things.
Actually, bug 796217 landed before the uplift as well, so hopefully bug 803452 landing takes care of things.
Keywords: verifyme
Sanity checking this with the following tests:

Check that a permission dialog is fired for

- Geolocation (web)

Check that permission dialog is not fired for and results in immediate permissions

- Alarm API (hosted app)

Check that no permission dialog is fired and results in no permissions

- Contacts API (web)

I'll do something more extensive later, but this will at least make sure nothing scary shows up.
(In reply to Jason Smith [:jsmith] from comment #52)
> Sanity checking this with the following tests:
> 
> Check that a permission dialog is fired for
> 
> - Geolocation (web)
> 
> ...
I am curious if you have identified all APIs (and bugs) where this prompt API is going to be consumed - you might want to make all of the bugs block this as followups
(In reply to David Dahl :ddahl from comment #53)
> (In reply to Jason Smith [:jsmith] from comment #52)
> > Sanity checking this with the following tests:
> > 
> > Check that a permission dialog is fired for
> > 
> > - Geolocation (web)
> > 
> > ...
> I am curious if you have identified all APIs (and bugs) where this prompt
> API is going to be consumed - you might want to make all of the bugs block
> this as followups

Right. I've been following the permission matrix to figure this out and will be adding test cases in our TCM for it.
Verified on 11/26 through sanity checks. I'll do a more deeper analysis later as part TCM testing.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.