805655 - System messages are received by applications that should not be allowed to

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Mounir Lamouri (:mounir)]

Some system messages should be linked to a permission and shouldn't be sent to an application that doesn't have the related permission. For example, sms-received should require sms permission. The same way for dial system messages, etc.

Comment 1

[Philipp von Weitershausen [:philikon]]

I agree! Thanks for filing this.

I think sendMessage/broadcastMessage should take a permission argument and enforce that permission on the child process (using target.assertPermission()). I almost feel like we should make this permission argument mandatory, or at least the first argument to those methods so that you have to explicitly pass `null` or something if there's no permission involved. I would argue that *most* if not *all* system messages require fairly involved privileges.

Comment 2

[Salvador de la Puente González [:salva]]

(In reply to Mounir Lamouri (:mounir) from comment #0)
> Some system messages should be linked to a permission and shouldn't be sent
> to an application that doesn't have the related permission. For example,
> sms-received should require sms permission. The same way for dial system
> messages, etc.

Yes, mi approach is tu user some kind of permission matrix:

{
  "sms-received": ["sms"]
  "alarm": ["alarms"]
}

Then we have two options:

  1. We can implicitly give those permissions (and, of course ask the user to accept them when installing the application)
  2. We can require them explicitly in the permission list of the manifest and prevent the system message to reach the application if not provided.

Comment 3

[Salvador de la Puente González [:salva]]

* tu user --> "to use"

Comment 4

[Mounir Lamouri (:mounir)]

I think it would be better to check the app's permission instead of the process' permission. Checking the process' permission is assuming that there is a 1:1 relationship between apps and processes which might not be the case.

In other words, we should always check permissions with the PermissionManager except for some specific cases (like killing a process).

Comment 5

[[:fabrice] Fabrice Desré]

I agree with Mounir we need to check the app's permission.

This needs to happen in quite a few places:

- When an app registers a message in its manifest, we should just not register it as a potential handler if the app doesn't have the right permissions.
- When an app calls navigator.mozHasPendingMessage('some-type'), do we throw if the apps has no permission for this type?
- When an app calls navigator.mozSetMessageHandler('some-type', ...). I'm not 100% sure about this one since we won't deliver messages to apps that should not receive them.

With this in place, I think we don't need to do anything in the internal sendMessage() and broadcastMessage() function. If we want defense in depth, we can also:

- When calling the internal sendMessage(), we can just check the permission at this point and not bail out early.

- When calling the internal broadcastMessage(), gail out when looking for matching apps.

Comment 6

[Salvador de la Puente González [:salva]]

(In reply to Fabrice Desré [:fabrice] from comment #5)
> I agree with Mounir we need to check the app's permission.
> (In reply to Fabrice Desré [:fabrice] from comment #5)
> I agree with Mounir we need to check the app's permission.

Yes, of course. +1

> - When an app registers a message in its manifest, we should just not
> register it as a potential handler if the app doesn't have the right
> permissions.

This is the explicit way in my first post. Then, if permission is not present, the system-message should be ignored. But please, give some feedback to the developer because it could be a bug.

> - When an app calls navigator.mozHasPendingMessage('some-type'), do we throw
> if the apps has no permission for this type?

I think this should behave as when you ask for pending messages with a non registered message. If we already throw an exception, then throw an exception.

Better, we can track of system-messages registered but with no proper permissions declared. Then, inside the exception, give the information to recover from this bug.

> - When an app calls navigator.mozSetMessageHandler('some-type', ...). I'm
> not 100% sure about this one since we won't deliver messages to apps that
> should not receive them.

Again, if permission is a necessary condition, then lets mozSetMessageHandler() behave as when that system message is not present.

> With this in place, I think we don't need to do anything in the internal
> sendMessage() and broadcastMessage() function. If we want defense in depth,
> we can also:

> - When calling the internal sendMessage(), we can just check the permission
> at this point and not bail out early.
> 
> - When calling the internal broadcastMessage(), gail out when looking for
> matching apps.

+1. This is redundancy we can but checking in the app should suffice. Anyway, see next comment for an alternative.

Comment 7

[Salvador de la Puente González [:salva]]

I was thinking about how to remove the object for system messages if all system-message already requires a set of permissions to properly work. But you need it for the handler registration and I think is better for the backend to know exactly what kind of system messages the application is waiting for.

Comment 8

[Mounir Lamouri (:mounir)]

Fabrice, I think we should:
- ignore registration of system messages if the associated permission isn't granted;
- check if the associated permission is granted before sending the system message;
- return false if mozHasPendingMessage is called but that should actually happen automatically;
- do as usual for setMessageHandler() but, that will never be called anyway.

Comment 11

[Gene Lian (I already quit Mozilla)]

Hi guys, I'm interested in this one and glad to take it. If no one is working on it, please feel free to let me know if I can. Thanks!

Comment 12

[[:fabrice] Fabrice Desré]

Sure Gene, it's all yours.

Comment 13

[Gene Lian (I already quit Mozilla)]

Attachment: WIP Patch

A WIP patch. Need more tests and clean-ups before asking a formal review.

Comment 14

[Gene Lian (I already quit Mozilla)]

Attachment: WIP Patch V2

A couple of issues were concerned during the implementation. I'll summarize and post them in the next comment.

Comment 15

[Gene Lian (I already quit Mozilla)]

Hi Mounir and Fabrice,

I encountered some issues needing your suggestions when you have a chance. Thanks for your help in advance!

  1. Might the system message be associated with multiple permissions? For example, a "bluetooth-dialer-command" seems to need "telephony" and "bluetooth" permissions. If yes, the system message can only be sent to the apps/pages that have *all* the related permissions, or matching just one of that is granted?

  2. I'm not quite sure how to deal with the "activity" system messages. I'm wondering do we really need to associate it with a certain permission set? If yes, how to assign different permissions for different activities that all share the same "activity" type? Taking the activity name into consideration?

The following 2 are not issues. Just implementation questions:

  1. I'm using the existing PermissionsTable in PermissionsInstaller.jsm to specify the relations between system messages and permissions like below, so that we don't need to maintain another new table and we'll construct the system-message-to-permissions mapping table by reading the "systemMessages" property backward. Does that sound reasonable to you?

                        alarms: {
                          app: ALLOW_ACTION,
                          privileged: ALLOW_ACTION,
-                         certified: ALLOW_ACTION
+                         certified: ALLOW_ACTION,
+                         systemMessages: ["alarm"]
                        },
                        telephony: {
                          app: DENY_ACTION,
                          privileged: DENY_ACTION,
-                         certified: ALLOW_ACTION
+                         certified: ALLOW_ACTION,
+                         systemMessages: ["telephony-new-call",
+                                          "bluetooth-dialer-command"]
                        },

  2. I have difficulties using testExactPermission() or testPermission() to check the page's permissions before sending/broadcasting system messages to it, but both of them return unknown permission by either feeding the page URI or the manifest URI. Did I wrongly use the functions (you might check the SystemMessageInternal.js part in the WIP patch)? Could you please give some hints?

Comment 16

[[:fabrice] Fabrice Desré]

(In reply to Gene Lian [:gene] from comment #15)
> Hi Mounir and Fabrice,
> 
> I encountered some issues needing your suggestions when you have a chance.
> Thanks for your help in advance!
> 
>   1. Might the system message be associated with multiple permissions? For
> example, a "bluetooth-dialer-command" seems to need "telephony" and
> "bluetooth" permissions. If yes, the system message can only be sent to the
> apps/pages that have *all* the related permissions, or matching just one of
> that is granted?

Yes, several permissions can be needed, and the page needs to have them all.

>   2. I'm not quite sure how to deal with the "activity" system messages. I'm
> wondering do we really need to associate it with a certain permission set?
> If yes, how to assign different permissions for different activities that
> all share the same "activity" type? Taking the activity name into
> consideration?

I think we don't need any permission for the activity message.

> The following 2 are not issues. Just implementation questions:
> 
>   1. I'm using the existing PermissionsTable in PermissionsInstaller.jsm to
> specify the relations between system messages and permissions like below, so
> that we don't need to maintain another new table and we'll construct the
> system-message-to-permissions mapping table by reading the "systemMessages"
> property backward. Does that sound reasonable to you?

I like the idea of adding that to PermissionsInstaller.jsm, but not too much the way you are piggy backing on this array. I also wonder how this would work with permissions that have access modes. I'm not opposed to have another array in the same file.

>   2. I have difficulties using testExactPermission() or testPermission() to
> check the page's permissions before sending/broadcasting system messages to
> it, but both of them return unknown permission by either feeding the page
> URI or the manifest URI. Did I wrongly use the functions (you might check
> the SystemMessageInternal.js part in the WIP patch)? Could you please give
> some hints?

You must use the principal to check the permission with testExactPermissionFromPrincipal() (llok at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#640 for an example).

Comment 17

[Mounir Lamouri (:mounir)]

I agree with Fabrice regarding the two first points.
I have nothing to add regarding the two later.

Comment 18

[Gene Lian (I already quit Mozilla)]

Thank you guys! Responses are clear to me. :)

Comment 19

[Gene Lian (I already quit Mozilla)]

Hi Fabrice and Mounir,

Eventually, I decided to construct another matrix table in PermissionsInstaller.jsm, which can take multiple permissions and multiple access modes into considerations. Does that sound reasonable to you guys? If you agree on this design, I should be able to upload a formal patch tomorrow.


// This table maps system message to permission(s), indicating only
// the system messages granted by the page's permissions are allowed
// to be registered or sent to that page. Note the empty permission
// set means this type of system message is always permitted.

this.SystemMessagePermissionsTable = {
  "activity": {
  },
  "alarm": {
    "alarms": {}
  },
  "bluetooth-dialer-command": {
    "telephony": {}
  },
  "bluetooth-requestconfirmation": {
    "mozBluetooth": {}
  },
  "bluetooth-requestpasskey": {
    "mozBluetooth": {}
  },
  "bluetooth-requestpincode": {
    "mozBluetooth": {}
  },
  "bluetooth-authorize": {
    "mozBluetooth": {}
  },
  "bluetooth-cancel": {
    "mozBluetooth": {}
  },
  "bluetooth-pairedstatuschanged": {
    "mozBluetooth": {}
  },
  "bluetooth-hfp-status-changed": {
    "mozBluetooth": {}
  },
  "bluetooth-opp-transfer-complete": {
    "mozBluetooth": {}
  },
  "bluetooth-opp-update-progress": {
    "mozBluetooth": {}
  },
  "bluetooth-opp-receiving-file-confirmation": {
    "mozBluetooth": {}
  },
  "bluetooth-opp-transfer-start": {
    "mozBluetooth": {}
  },
  "icc-stkcommand": {
    "settings": {
      access: ["read", "write"]
    }
  },
  "sms-received": {
    "sms": {}
  },
  "telephony-new-call": {
    "telephony": {}
  }
};

Comment 20

[Gene Lian (I already quit Mozilla)]

Attachment: WIP Patch V3

Comment 21

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, PermissionsInstaller.jsm

Hi Mounir and Fabrice,

I'm not sure who should be more proper to review this part, where I believe both of you are definitely experts on the permission things. Could either of you take this review? Thanks! The following summarizes the changes:

1. A new |SystemMessagePermissionsTable| is constructed to map each system message to its valid permission(s).

2. A new |isSystemMessagePermitted()| can help Webapps.js to decide whether the app's permission(s) is granted or not to register the system message. Note that we'll report some errors to remind clients if the needed permissions are not added.

3. s/mapSuffixes()/addSuffixes(), which sounds more proper. Also, if the |aSuffixes| is an empty array, just returns |[aPermName]|, which is going to be used later.

4. Refactor |_getAppPermType()| which is called twice.

5. Code clean-up: the code-indent really shocks me! Clean them up!

6. Code clean-up: sometime we use |perm| to refer to the permission name but sometimes we use |permName|, which is very confusing and hard to read. Let's do:

  - permName(s): permission name(s) like "sms" or "telephony"... etc.
  - permValue: permission value like "allow" or "deny"... etc.
  - permType: permission type like app/privileged/certified.
  - expandedPermNames: expanded permission name(s) returned by .expandedPermNames.

7. Code clean-up: add some missing comments or rephrase them.

Comment 22

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, Webapps.jsm

Hi Fabrice,

The changes are trivial. I use |PermissionsInstaller.isSystemMessagePermitted()| (part 1) to decide if we need to register the system message based on whether the app's manifest has claimed needed permissions or not. The rest are some minor code clean-ups.

Comment 23

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, SystemMessageInternal.js

Hi Fabrice,

1. I use the |SystemMessagePermissionsTable| constructed in the PermissionsInstaller.jsm (part 1) to check if the permission(s) of the system message we're going to send/broadcast is installed in the target app or not by calling an existing function |PermissionSettingsModule.getPermission()|.

2. Code clean-ups: refactor |winTargets| which is going to be used multiple times and rephrase some comments.

Comment 24

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, PermissionsInstaller.jsm, V2

A new |.getSystemMessagePermissions()| is refactored for the common part with part-3 patch.

Please see comment #21 for the main summary of this patch. Thanks!

Comment 25

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, SystemMessageInternal.js, V2

Use the new refactored |.getSystemMessagePermissions()| provided in the part-1 patch.

Please see comment #23 for the main summary of this patch. Thanks!

Comment 26

[Mounir Lamouri (:mounir)]

Comment on attachment 684333 [details] [diff] [review]
Part 2, Webapps.jsm

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

r=me, assuming it includes the change I (will) request for part 1.

::: dom/apps/src/Webapps.jsm
@@ +335,5 @@
>        }
> +
> +      if (PermissionsInstaller.isSystemMessagePermitted(messageName,
> +                                                        { origin: aApp.origin,
> +                                                          manifest: aManifest })) {

You should pass aApp instead of this object. Otherwise, you should change the method to take an origin and a manifest.

@@ +385,5 @@
>  
>        let launchPath = Services.io.newURI(description.href, null, null);
>        let manifestURL = Services.io.newURI(aApp.manifestURL, null, null);
> +
> +      if (PermissionsInstaller.isSystemMessagePermitted("activity", 

nit: trailing whitespace.

I would be tempted to say we probably don't need that because "activity" requires no permission but it's probably better for maintanability to not do any special case.

Comment 27

[Mounir Lamouri (:mounir)]

Comment on attachment 684572 [details] [diff] [review]
Part 3, SystemMessageInternal.js, V2

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

Could you factorise the code that checks for the permissions, get the winIds and send the sendAsyncMessage? Seems like it's quite a copy-pasta.

r=me with the requested changes (and the possible changes requested from part 1) but I would prefer Fabrice to have a look too.

::: dom/messages/SystemMessageInternal.js
@@ +380,5 @@
>    },
>  
> +  _isSystemMessagePermitted: function _isSystemMessagePermitted(aType, aPageURI, aManifestURL) {
> +    let permNames = PermissionsInstaller.getSystemMessagePermissions(aType);
> +    if (!permNames) {

if (permNames === null) {

@@ +396,5 @@
> +                                                 aManifestURL,
> +                                                 pageURI.prePath,
> +                                                 false);
> +
> +        if (permValue == "unknown" || permValue == "deny") {

if (permValue != "allow") {

You could even do:
if (PermissionSettingsModule.getPermission(...) != "allow") {

Comment 28

[Mounir Lamouri (:mounir)]

Comment on attachment 684568 [details] [diff] [review]
Part 1, PermissionsInstaller.jsm, V2

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

Thanks for the clean-up :) Nice patch in general.

However, there are a few high-level issues here. I think it would be better to move SystemMessagePermissionsTable, getSystemMessagePermissions and isSystemMessagePermitted out of PermissionInstaller.jsm. Those things are using permissions but are not related to permissions. I see that we don't have a SystemMessageSomething.jsm so I'm not sure where to put this. Fabrice might have some ideas.
Also, we might want to make a clear distinction between checking for system messages permissions when registering and when executing. I know that your code make that distinction but it's quite confusing to know what those methods are doing without reading the code. That should be understandable by reading the name.

In a nutshell, your code is good, I just think we should move it somewhere else and rename a method or two.

::: dom/apps/src/PermissionsInstaller.jsm
@@ +476,5 @@
>          }
>        }
>  
> +      // Check to see if the 'webapp' is app/privileged/certified.
> +      let permType = this._getAppPermType(aApp.manifest);

Could you name that "appStatus" instead of "permType"?

@@ +510,5 @@
> +   * Return all the needed permission names for the given system message name.
> +   * @param string aSysMsgName
> +   *        The system messsage name.
> +   * @returns object
> +   *        Format: { oldPermName (string): newPermNames (string array), ... }

Format: { permName (string) : permWithAccess (string array), ... }

"oldPermName" is just too confusing because you expect a "newPermName" or that means the method is deprecated, or the name is... While it's actually a generic name.

@@ +519,5 @@
> +   *        Ex, the system message we want to search is not searchable.
> +   **/
> +  getSystemMessagePermissions: function getSystemMessagePermissions(aSysMsgName) {
> +    let permNames = SystemMessagePermissionsTable[aSysMsgName];
> +    if (!permNames) {

Maybe you could be explicit and do:
if (permNames === undefined) {
}

@@ +528,5 @@
> +    }
> +
> +    let object = { };
> +    for (let permName in permNames) {
> +      if (!PermissionsTable[permName]) {

if (PermissionsTable[permName] === undefined) {

@@ +546,5 @@
> +                       "'" + permName + "' is not associated with valid access array. " +
> +                       "Please correct it in the SystemMessagePermissionsTable.");
> +        return null;
> +      }
> +      object[permName] = addSuffixes(permName, access);

Ok, so you return:
{
  "foo": { "foo-read", "foo-write" },
  "bar": { "" },
}
instead of:
[ "foo-read", "foo-write", "bar" ]
because the method below needs to know about "foo" and "bar" but a method to check permissions at runtime would only care about "foo-read", "foo-write" and "bar". Why not...

@@ +563,5 @@
> +   *        The system message is permitted or not.
> +   **/
> +  isSystemMessagePermitted: function isSystemMessagePermitted(aSysMsgName, aApp) {
> +    let permNames = this.getSystemMessagePermissions(aSysMsgName);
> +    if (!permNames) {

if (permNames === null) {

@@ +566,5 @@
> +    let permNames = this.getSystemMessagePermissions(aSysMsgName);
> +    if (!permNames) {
> +      return false;
> +    }
> +

If I understand this method correctly, you are checking if the required permissions are listed in the manifest and if they are allowed given the app status. This would indeed work to *register* the system handler but it wouldn't work at all at run-time.

I guess we need such a mechanism to register but we also need to be able to check at run-time if the app has the required permissions.

I saw that part 3 has this method but it should be clear that this one isn't doing runtime stuff. Maybe it should be named something like canSystemMessageBeRegistered()?

@@ +571,5 @@
> +    // Check to see if the 'webapp' is app/privileged/certified.
> +    let permType = this._getAppPermType(aApp.manifest);
> +    let newManifest = new ManifestHelper(aApp.manifest, aApp.origin);
> +
> +    for (let oldPermName in permNames) {

nit: oldPermName is a pretty confusing name. See a comment above.

@@ +638,5 @@
> +   * @param object aManifest
> +   *        The app's manifest.
> +   * @returns string app/privileged/certified
> +   **/
> +  _getAppPermType: function getAppPermType(aManifest) {

_getAppStatus

Comment 29

[Gene Lian (I already quit Mozilla)]

(In reply to Mounir Lamouri (:mounir) from comment #28)
> Comment on attachment 684568 [details] [diff] [review]
> Part 1, PermissionsInstaller.jsm, V2
> 
> Review of attachment 684568 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the clean-up :) Nice patch in general.

Thanks for your detailed review! Happy to know the my patch is fine in general. I'll come back with the refined patch based on your feedback. :)

> However, there are a few high-level issues here. I think it would be better
> to move SystemMessagePermissionsTable, getSystemMessagePermissions and
> isSystemMessagePermitted out of PermissionInstaller.jsm. Those things are
> using permissions but are not related to permissions. I see that we don't
> have a SystemMessageSomething.jsm so I'm not sure where to put this. Fabrice
> might have some ideas.

I have a suggestion: how about moving the functions and table to an independent dom/messages/SystemMessagePermissionsChecker.jsm (new), which contains the following functions:

  1. getSystemMessagePermissions()
  2. isSystemMessagePermittedToRegister()
  3. isSystemMessagePermittedToSend()

PS. #3 is the original SystemMessageInternal._isSystemMessagePermitted().

Compared to your suggested canSystemMessageBeRegistered(), I would prefer having "...Permitted..." because we can know it has something to do with "permissions" at least. How do you feel?

Comment 30

[Gene Lian (I already quit Mozilla)]

(In reply to Mounir Lamouri (:mounir) from comment #27)
> Comment on attachment 684572 [details] [diff] [review]
> Part 3, SystemMessageInternal.js, V2
> ::: dom/messages/SystemMessageInternal.js
> @@ +396,5 @@
> > +                                                 aManifestURL,
> > +                                                 pageURI.prePath,
> > +                                                 false);
> > +
> > +        if (permValue == "unknown" || permValue == "deny") {
> 
> if (permValue != "allow") {
> 
> You could even do:
> if (PermissionSettingsModule.getPermission(...) != "allow") {

One quick question here: but how about "promt", which would also be excluded in this case. Is that OK (honestly, I'm not very sure what does "prompt" mean for n webapp)?

Comment 31

[Mounir Lamouri (:mounir)]

(In reply to Gene Lian [:gene] from comment #29)
> I have a suggestion: how about moving the functions and table to an
> independent dom/messages/SystemMessagePermissionsChecker.jsm (new), which
> contains the following functions:
> 
>   1. getSystemMessagePermissions()
>   2. isSystemMessagePermittedToRegister()
>   3. isSystemMessagePermittedToSend()
> 
> PS. #3 is the original SystemMessageInternal._isSystemMessagePermitted().
> 
> Compared to your suggested canSystemMessageBeRegistered(), I would prefer
> having "...Permitted..." because we can know it has something to do with
> "permissions" at least. How do you feel?

Sounds good to me.

(In reply to Gene Lian [:gene] from comment #30)
> (In reply to Mounir Lamouri (:mounir) from comment #27)
> > Comment on attachment 684572 [details] [diff] [review]
> > Part 3, SystemMessageInternal.js, V2
> > ::: dom/messages/SystemMessageInternal.js
> > @@ +396,5 @@
> > > +                                                 aManifestURL,
> > > +                                                 pageURI.prePath,
> > > +                                                 false);
> > > +
> > > +        if (permValue == "unknown" || permValue == "deny") {
> > 
> > if (permValue != "allow") {
> > 
> > You could even do:
> > if (PermissionSettingsModule.getPermission(...) != "allow") {
> 
> One quick question here: but how about "promt", which would also be excluded
> in this case. Is that OK (honestly, I'm not very sure what does "prompt"
> mean for n webapp)?

Given that it is at runtime, I think we shouldn't consider prompt as okay. We will not prompt. However, for registration, we can register is prompt is an option because the user might be prompted during the application runtime.

Comment 32

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, SystemMessagePermissionsChecker.jsm, V3

Addressing comment #28 and comment #31. The main changes in this patch are summarized as below:

1. Create a new dom/messages/SystemMessagePermissionsChecker.jsm to contain the following main table/functions:

  1. SystemMessagePermissionsTable
  2. getSystemMessagePermissions(...)
  3. isSystemMessagePermittedToRegister(...)
  4. isSystemMessagePermittedToSend(...)

2. Expose s/mapSuffixes(...)/appendAccessToPermName(...), which will be used both in the original PermissionsInstaller.jsm and the new SystemMessagePermissionsChecker.jsm.

3. The reset is minor code clean-up.

Thanks for the review again!

Comment 33

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, Webapps.jsm, V2

Addressing comment #26. Not very sure if we need to directly remove the check for activity or better to keep it due to maintainability.

sr=mounir

Comment 34

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, SystemMessageInternal.js, V3

Addressing comment #27.

sr=mounir

Comment 35

[Mounir Lamouri (:mounir)]

Comment on attachment 685076 [details] [diff] [review]
Part 2, Webapps.jsm, V2

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

I said r=me, not sr.
No need for Fabrice's review here.

Comment 36

[Mounir Lamouri (:mounir)]

Comment on attachment 685085 [details] [diff] [review]
Part 3, SystemMessageInternal.js, V3

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

I said r=me, not sr.
However, I would like Fabrice to double-check because that code changed a lot since a looked at it.

Comment 37

[Mounir Lamouri (:mounir)]

Comment on attachment 685072 [details] [diff] [review]
Part 1, SystemMessagePermissionsChecker.jsm, V3

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

Thank you for this patch :)

sr=me with the comments bellow taken into account.

::: dom/apps/src/AppsUtils.jsm
@@ +212,5 @@
>      let type = aManifest.type || "web";
>  
>      switch(type) {
> +      case "web":
> +        return Ci.nsIPrincipal.APP_STATUS_INSTALLED;

Are you sure our coding style says to indent stuff like that?
In C++, our coding style is quite clear that the previous indentation is the correct one. I don't know for JS though.

::: dom/messages/SystemMessagePermissionsChecker.jsm
@@ +40,5 @@
> +  "bluetooth-dialer-command": {
> +    "telephony": {}
> +  },
> +  "bluetooth-requestconfirmation": {
> +    "mozBluetooth": {}

... why is this permission prefixed? Could you file a follow-up? this should be named "bluetooth".

@@ +177,5 @@
> +    let newManifest = new ManifestHelper(aManifest, aOrigin);
> +
> +    for (let permName in permNames) {
> +      // The app doesn't claim valid permissions for this sytem message.
> +      if (!newManifest.permissions || !newManifest.permissions[permName]) {

The check for |newManifest.permissions| could be done before going inside the loop:
if permNames has at least one element (isn't empty) and newManifest.permissions doesn't exist, reportError() and return false.

By the way, I'm not sure what reportError() exactly does. This shouldn't be considered as a Gecko/Firefox error. However, you might want to log/display the error.
That applies for all reportError() in this file.

@@ +184,5 @@
> +        return false;
> +      }
> +      let permValue = PermissionsTable[permName][appStatus];
> +      if (permValue == Ci.nsIPermissionManager.UNKNOWN_ACTION ||
> +          permValue == Ci.nsIPermissionManager.DENY_ACTION) {

Maybe:
!= PROMPT_ACTION && != ALLOW_ACTION
just to be explicit.

Comment 38

[Gene Lian (I already quit Mozilla)]

(In reply to Mounir Lamouri (:mounir) from comment #37)
> Comment on attachment 685072 [details] [diff] [review]
> Part 1, SystemMessagePermissionsChecker.jsm, V3
> 
> ::: dom/apps/src/AppsUtils.jsm
> @@ +212,5 @@
> >      let type = aManifest.type || "web";
> >  
> >      switch(type) {
> > +      case "web":
> > +        return Ci.nsIPrincipal.APP_STATUS_INSTALLED;
> 
> Are you sure our coding style says to indent stuff like that?
> In C++, our coding style is quite clear that the previous indentation is the
> correct one. I don't know for JS though.

I'd say this is very chaotic... Both are present in our current JS codes but indenting the cases seems a majority. I'll try to confirm that (if I cannot find a concrete rule, I'll keep the original ones in AppsUtils.jsm as it was).

> ::: dom/messages/SystemMessagePermissionsChecker.jsm
> @@ +40,5 @@
> > +  "bluetooth-dialer-command": {
> > +    "telephony": {}
> > +  },
> > +  "bluetooth-requestconfirmation": {
> > +    "mozBluetooth": {}
> 
> ... why is this permission prefixed? Could you file a follow-up? this should
> be named "bluetooth".

The Gaia's guys told me they prefer using mozBluetooth. I'll confirm it again to see which one is actually deprecated.

> By the way, I'm not sure what reportError() exactly does. This shouldn't be
> considered as a Gecko/Firefox error. However, you might want to log/display
> the error.
> That applies for all reportError() in this file.

Good point! Most of Cu.reportError() are applied for catching an exception, which is not the case we want:

  catch (e) {
    Cu.reportError(e);
  }

I'll try to find a better logging function. If I cannot find that, I'll simply use dump(), though.

Thanks Mounir for all the reviews!

Comment 39

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, SystemMessagePermissionsChecker.jsm, V3.1

Hi Fabrice, we need your second review for this patch. Could you please take a look when you have a chance? Thanks!

This patch solves comment #37 as below:

1. I cannot find a JS coding style for switch-case indentation. Both are present in our current codes. Eventually, I follow the codes written by David Flanagan because he is the bible. :P

2. s/mozBluetooth/bluetooth.

3. s/reportError()/debug().

4. I didn't do the following one because I think the current logic exactly does the same thing.

> The check for |newManifest.permissions| could be done before going inside
> the loop:
> if permNames has at least one element (isn't empty) and
> newManifest.permissions doesn't exist, reportError() and return false.

5. sr=mounir

Comment 40

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, Webapps.jsm, V2.1

r=mounir in the commit comment.

Comment 41

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, SystemMessageInternal.js, V3.1

Hi Fabrice, need your double review for this patch. Could you please take a look? Thanks again!

r=mounir in the commit comment.

Comment 42

[Gregor Wagner [:gwagner]]

As much as I like attempts to unify coding style, in the current situation where we do other permission changes it just causes merge conflicts.

Comment 43

[[:fabrice] Fabrice Desré]

Comment on attachment 686043 [details] [diff] [review]
Part 1, SystemMessagePermissionsChecker.jsm, V3.1

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

Gene, can you split up your patch in different parts, especially one with the new stuff and real changes? There are files that are touched only to change the style or renaming variables. That makes the review harder and also will change the blame on these files for a not-so-good reason.

::: dom/messages/SystemMessagePermissionsChecker.jsm
@@ +21,5 @@
> +  if (DEBUG) {
> +    dump("SystemMessagePermissionsChecker.jsm: " + aStr + "\n");
> +  }
> +}
> +

simply use:
function debug(aStr) {
  //dump("SystemMessagePermissionsChecker.jsm: " + aStr + "\n");
}

@@ +83,5 @@
> +  "telephony-new-call": {
> +    "telephony": {}
> +  }
> +};
> +

What about a simpler format like:
SystemMessagePermissionsTable = {
  
  "icc-stkcommand": {
    "settings": ["read", "write"]
  },

  "sms-received": {
    "sms": []
  },

  "headset-button": { }
}

Comment 44

[Gene Lian (I already quit Mozilla)]

(In reply to Fabrice Desré [:fabrice] from comment #43)
> Gene, can you split up your patch in different parts, especially one with
> the new stuff and real changes? There are files that are touched only to
> change the style or renaming variables. That makes the review harder and
> also will change the blame on these files for a not-so-good reason.

Sorry for letting you confused. Indeed, there are too much unrelated clean-ups included in this patch at one time. I'll try to split them up to make the patches more accurate and readable for their own purposes.

> What about a simpler format like:
> SystemMessagePermissionsTable = {
>   
>   "icc-stkcommand": {
>     "settings": ["read", "write"]
>   },

I originally preferred adding an extra |access| property because our existing |PermissionsTable| did the same thing. Also, the structure might make the table more extendable. I'm fine to remove that for now and revisit this in the future since we shouldn't have backward-compatibility issue here. Thanks for pointing it out!

Comment 45

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, SystemMessagePermissionsChecker.jsm, V4

Hi Fabrice,

0. sr=mounir
1. I remove all the code clean-up things from this patch.
2. Don't include the |access| property until we really need it.
3. Don't use DEBUG flag for the debugging function.
4. Add the following entry, which is under way at Bug 814074:

  "notification": {
    "desktop-notification": []
  },

Please let me know if you have any other suggestions! Thanks!

Comment 46

[Gene Lian (I already quit Mozilla)]

Hi Vivien,

Nothing bugging. Just letting you know I added an restriction based on your on-going patch at Bug 814074, making the "notification" system message can only be allowed to be sent to the app which has the "desktop-notification" permission:

  "notification": {
    "desktop-notification": []
  },

Please let me know if this is too restrictive for you. Thanks!

Comment 47

[Gene Lian (I already quit Mozilla)]

Attachment: Part 4, Code Clean-Up!

Hi Fabrice,

I'm not very sure we can put this clean-up patch within this bug or should put it in another new-fired bug. Summary:

1. The code-indent really shocks me! Clean them up!
2. Sometime we use |perm| to refer to the permission name but sometimes we use |permName|, which is very confusing and hard to read. Let's do:

  - permName(s): permission name(s) like "sms" or "telephony"... etc.
  - permValue: permission value like "allow" or "deny"... etc.
  - appStatus: app status like app/privileged/certified.
  - expandedPermNames: expanded permission name(s) returned by .expandedPermNames().

3. Add some missing comments or rephrase them.

Well, if you think item #2 or #3 is not really necessary because it depends on different people's coding style and reading habits then I'm fine with them. :) However, I think item #1 is something we need to indent for sure, which makes me hard to fall asleep at night...

Comment 48

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, SystemMessagePermissionsChecker.jsm, V4.1

Rebased.

r=fabrice,sr=mounir

Comment 49

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, Webapps.jsm, V2.2

Rebased.

r=mounir

Comment 50

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, SystemMessageInternal.js, V3.2

r=fabrice,mounir

Comment 51

[Gene Lian (I already quit Mozilla)]

Attachment: Part 4, Code Clean-Up, V1.1

Rebased.

r=fabrice,mounir

Comment 52

[Hsin-Yi Tsai (she/her) [:hsinyi]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e40b353c4fb5
https://hg.mozilla.org/integration/mozilla-inbound/rev/5be6adba0df9
https://hg.mozilla.org/integration/mozilla-inbound/rev/964c90e1e1e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/06abc5d30f26

Comment 53

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e40b353c4fb5
https://hg.mozilla.org/mozilla-central/rev/5be6adba0df9
https://hg.mozilla.org/mozilla-central/rev/964c90e1e1e2
https://hg.mozilla.org/mozilla-central/rev/06abc5d30f26

Comment 54

[Ryan VanderMeulen [:RyanVM]]

Part 4 doesn't apply cleanly to aurora/beta. Can you post a rebased version of that for uplift please?

Comment 55

[Gene Lian (I already quit Mozilla)]

(In reply to Ryan VanderMeulen from comment #54)
> Part 4 doesn't apply cleanly to aurora/beta. Can you post a rebased version
> of that for uplift please?

OK, no problem. :) I can imagine we'll have conflicts when checking in the patch into aurora/beta, where the PermissionsInstaller.jsm is not completely sync'ed because some of the previous patches are bb+ but some are not unfortunately. Not sure which of the following preferences sounds more proper in general?

  1. Found out all the previous non-bb+ patches, ask them to be checked in first and then come back to this one.

  2. Create aurora/beta specific patches (for this bug only) and then check it in.

Comment 56

[Gene Lian (I already quit Mozilla)]

OK. I thought you're talking about method #2. I'll do it later.

Comment 57

[[:fabrice] Fabrice Desré]

Actually, there are many other changes coming to PermissionsInstaller.jsm, so that would be nice to know which patches touched it that were not uplifted.

Comment 58

[Gene Lian (I already quit Mozilla)]

Attachment: [beta] Part 1, SystemMessagePermissionsChecker.jsm

Part-1 patch for beta-specific.

Comment 59

[Gene Lian (I already quit Mozilla)]

Attachment: [beta] Part 2, Webapps.jsm

Part-2 patch for beta-specific.

Comment 60

[Gene Lian (I already quit Mozilla)]

Attachment: [beta] Part 3, SystemMessageInternal.js

Part-3 patch for beta-specific.

Comment 61

[Gene Lian (I already quit Mozilla)]

Attachment: [beta] Part 4, Code Clean-Up

Part-4 patch for beta-specific.

Comment 62

[Gene Lian (I already quit Mozilla)]

Fabrice suggested we need to uplift other patches first which touches the PermissionsInstaller.jsm but weren't marked by bb+ or not yet checked in properly for aurora/beta. Let's wait for them to void constant rebasing, which is very tedious and error-prone.

As far as I know, we need to have the following patches checked in for aurora/beta first:

  1. Bug 814293 - Remove unused permissions 
     - Not done for both aurora and beta.

  2. Bug 805333 - Need a Policy & Mechanism for Audio Competing & Control
     - Only done for beta but not for aurora. Why???!!!

Comment 63

[Mounir Lamouri (:mounir)]

(In reply to Gene Lian [:gene] from comment #62)
>   2. Bug 805333 - Need a Policy & Mechanism for Audio Competing & Control
>      - Only done for beta but not for aurora. Why???!!!

Pushing to aurora isn't really needed. It done for convenience (and makes a lot of people waste their time...). This patch should have been pushed to aurora but having it in beta and m-c is enough to have things rolling.

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f8a027f57d11
https://hg.mozilla.org/releases/mozilla-aurora/rev/0ac2b512f6d4
https://hg.mozilla.org/releases/mozilla-aurora/rev/16e657d4d003
https://hg.mozilla.org/releases/mozilla-aurora/rev/ad0e460ed356

https://hg.mozilla.org/releases/mozilla-beta/rev/dab60493dfb2
https://hg.mozilla.org/releases/mozilla-beta/rev/b79a83d11649
https://hg.mozilla.org/releases/mozilla-beta/rev/08bae05a809e
https://hg.mozilla.org/releases/mozilla-beta/rev/c4435d393cac

Comment 65

[Gene Lian (I already quit Mozilla)]

Awesome! Everything is cleanly applied. Thanks Ryan very much for all the hard work! :D