Mounir Lamouri (:mounir) Reporter
	Description • 12 years ago

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.

	Philipp von Weitershausen [:philikon]
	Comment 1 • 12 years ago

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.

	Salvador de la Puente González [:salva]
	Comment 2 • 12 years ago

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

	Salvador de la Puente González [:salva]
	Comment 3 • 12 years ago

* tu user --> "to use"

	Mounir Lamouri (:mounir) Reporter
	Comment 4 • 12 years ago

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

	[:fabrice] Fabrice Desré
	Comment 5 • 12 years ago

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.

	Salvador de la Puente González [:salva]
	Comment 6 • 12 years ago

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

	Salvador de la Puente González [:salva]
	Comment 7 • 12 years ago

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.

	Mounir Lamouri (:mounir) Reporter
	Comment 8 • 12 years ago

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.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 11 • 12 years ago

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!

	[:fabrice] Fabrice Desré
	Comment 12 • 12 years ago

Sure Gene, it's all yours.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 13 • 12 years ago

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

	Gene Lian (I already quit Mozilla) Assignee
	Comment 14 • 12 years ago

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

	Gene Lian (I already quit Mozilla) Assignee
	Comment 15 • 12 years ago

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?

	[:fabrice] Fabrice Desré
	Comment 16 • 12 years ago

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

	Mounir Lamouri (:mounir) Reporter
	Comment 17 • 12 years ago

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

	Gene Lian (I already quit Mozilla) Assignee
	Comment 18 • 12 years ago

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

	Gene Lian (I already quit Mozilla) Assignee
	Comment 19 • 12 years ago

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": {}
  }
};

	Gene Lian (I already quit Mozilla) Assignee
	Comment 21 • 12 years ago

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.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 22 • 12 years ago

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.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 23 • 12 years ago

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.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 24 • 12 years ago

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!

	Gene Lian (I already quit Mozilla) Assignee
	Comment 25 • 12 years ago

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

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

	Mounir Lamouri (:mounir) Reporter
	Comment 26 • 12 years ago

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.

	Mounir Lamouri (:mounir) Reporter
	Comment 27 • 12 years ago

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") {

	Mounir Lamouri (:mounir) Reporter
	Comment 28 • 12 years ago

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

	Gene Lian (I already quit Mozilla) Assignee
	Comment 29 • 12 years ago

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

	Gene Lian (I already quit Mozilla) Assignee
	Comment 30 • 12 years ago

(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)?

	Mounir Lamouri (:mounir) Reporter
	Comment 31 • 12 years ago

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

	Gene Lian (I already quit Mozilla) Assignee
	Comment 32 • 12 years ago

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!

	Gene Lian (I already quit Mozilla) Assignee
	Comment 33 • 12 years ago

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

	Gene Lian (I already quit Mozilla) Assignee
	Comment 34 • 12 years ago

Addressing comment #27.

sr=mounir

	Mounir Lamouri (:mounir) Reporter
	Comment 35 • 12 years ago

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.

	Mounir Lamouri (:mounir) Reporter
	Comment 36 • 12 years ago

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.

	Mounir Lamouri (:mounir) Reporter
	Comment 37 • 12 years ago

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.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 38 • 12 years ago

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

	Gene Lian (I already quit Mozilla) Assignee
	Comment 39 • 12 years ago

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

	Gene Lian (I already quit Mozilla) Assignee
	Comment 40 • 12 years ago

r=mounir in the commit comment.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 41 • 12 years ago

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

r=mounir in the commit comment.

	Gregor Wagner [:gwagner]
	Comment 42 • 12 years ago

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.

	[:fabrice] Fabrice Desré
	Comment 43 • 12 years ago

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": { }
}

	Gene Lian (I already quit Mozilla) Assignee
	Comment 44 • 12 years ago

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

	Gene Lian (I already quit Mozilla) Assignee
	Comment 45 • 12 years ago

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!

	Gene Lian (I already quit Mozilla) Assignee
	Comment 46 • 12 years ago

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!

	Gene Lian (I already quit Mozilla) Assignee
	Comment 47 • 12 years ago

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

	Gene Lian (I already quit Mozilla) Assignee
	Comment 48 • 12 years ago

Rebased.

r=fabrice,sr=mounir

	Gene Lian (I already quit Mozilla) Assignee
	Comment 49 • 12 years ago

Rebased.

r=mounir

	Gene Lian (I already quit Mozilla) Assignee
	Comment 50 • 12 years ago

r=fabrice,mounir

	Gene Lian (I already quit Mozilla) Assignee
	Comment 51 • 12 years ago

Rebased.

r=fabrice,mounir

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 52 • 12 years ago

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

	Ed Morley [:emorley]
	Comment 53 • 12 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 54 • 12 years ago

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

	Gene Lian (I already quit Mozilla) Assignee
	Comment 55 • 12 years ago

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

	Gene Lian (I already quit Mozilla) Assignee
	Comment 56 • 12 years ago

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

	[:fabrice] Fabrice Desré
	Comment 57 • 12 years ago

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.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 58 • 12 years ago

Part-1 patch for beta-specific.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 59 • 12 years ago

Part-2 patch for beta-specific.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 60 • 12 years ago

Part-3 patch for beta-specific.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 61 • 12 years ago

Part-4 patch for beta-specific.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 62 • 12 years ago

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???!!!

	Mounir Lamouri (:mounir) Reporter
	Comment 63 • 12 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 64 • 12 years ago

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

	Gene Lian (I already quit Mozilla) Assignee
	Comment 65 • 12 years ago

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