773114 - Permission UI events for apps

Status: VERIFIED FIXED

(Core :: General, defect, P1)

Description

[Lucas Adamski [:ladamski]]

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.

Comment 1

[Lucas Adamski [:ladamski]]

See also https://github.com/mozilla-b2g/gaia/issues/2357

Comment 2

[David Dahl :ddahl]

Is there a feature page/spec for what the event names are and what the event target / observer data to handle/provide here?

Comment 3

[David Dahl :ddahl]

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

Comment 4

[Lucas Adamski [:ladamski]]

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.

Comment 5

[David Dahl :ddahl]

(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.

Comment 6

[David Dahl :ddahl]

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

Comment 7

[David Dahl :ddahl]

Attachment: Patch 1 - no tests yet

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

Comment 8

[David Dahl :ddahl]

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.

Comment 9

[David Dahl :ddahl]

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

Comment 10

[Lucas Adamski [:ladamski]]

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.

Comment 11

[David Dahl :ddahl]

Attachment: Patch 3 - no tests yet

depends on bug 758269 bug 770731

Comment 12

[Josh Carpenter [:jcarpenter]]

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

Comment 13

[Gregor Wagner [:gwagner]]

Comment on attachment 653928 [details] [diff] [review]
Patch 3 - no tests yet

Looks good to me!

Comment 14

[Dietrich Ayala (:dietrich)]

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?

Comment 15

[Josh Carpenter [:jcarpenter]]

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.

Comment 16

[David Dahl :ddahl]

(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.

Comment 17

[Andrew Overholt [:overholt]]

Gregor, do you have time for this right now?

Comment 18

[Gregor Wagner [:gwagner]]

(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.

Comment 19

[David Dahl :ddahl]

(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.

Comment 20

[David Dahl :ddahl]

Attachment: Patch 4, WIP

I need to change the name of the PermissionService to "PermissionPromptService"

Comment 21

[David Dahl :ddahl]

Attachment: Patch 5 WIP - renamed service

Comment 22

[Dietrich Ayala (:dietrich)]

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.

Comment 23

[David Dahl :ddahl]

(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

Comment 24

[David Dahl :ddahl]

Attachment: Patch 6 WIP

Not sure if this should be a JSM-as-singleton or an XPCOM service

Comment 25

[David Dahl :ddahl]

Attachment: Patch

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?

Comment 26

[David Dahl :ddahl]

Attachment: WIP Patch 2

Marionette test is set up - a few issues here yet, but at a goog point to get some feedback.

Comment 27

[David Dahl :ddahl]

Attachment: Patch 4

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

Comment 28

[Etienne Segonzac (:etienne)]

Comment on attachment 668174 [details] [diff] [review]
Patch 4

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

The mozChromeEvent/mozContentEvent dialog looks good to me.

Comment 29

[David Dahl :ddahl]

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.

Comment 30

[David Dahl :ddahl]

Attachment: Latest Patch

Comment 31

[David Dahl :ddahl]

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

Comment 32

[David Dahl :ddahl]

Attachment: Patch for 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.

Comment 33

[(inactive after 6/18) Alive Kuo [:alive]]

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.

Comment 34

[David Dahl :ddahl]

(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

Comment 35

[Gregor Wagner [:gwagner]]

(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.

Comment 37

[Dietrich Ayala (:dietrich)]

Jonas, this patch is waiting for review 6 days. Do you have ETA for review?

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 39

[David Dahl :ddahl]

Attachment: Updated Patch with Jonas' comments addressed

Comment 40

[Gregor Wagner [:gwagner]]

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!

Comment 41

[David Dahl :ddahl]

(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.

Comment 42

[David Dahl :ddahl]

Attachment: Patch for inbound

Going to apply this to my local inbound repo before pushing

Comment 43

[David Dahl :ddahl]

Attachment: Pushed to inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/88693039c414

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Bugs aren't resolved until they land on m-c.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/88693039c414

Comment 46

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 47

[David Dahl :ddahl]

(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.

Comment 48

[David Dahl :ddahl]

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]

Comment 49

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 50

[Ryan VanderMeulen [:RyanVM]]

Actually, bug 796217 landed before the uplift as well, so hopefully bug 803452 landing takes care of things.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b1f9bffd2e4f

Comment 52

[Jason Smith [:jsmith]]

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.

Comment 53

[David Dahl :ddahl]

(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

Comment 54

[Jason Smith [:jsmith]]

(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.

Comment 55

[Jason Smith [:jsmith]]

Verified on 11/26 through sanity checks. I'll do a more deeper analysis later as part TCM testing.