Check application types when granting audio-* permissions

RESOLVED FIXED

Status

Firefox OS
General
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: pauljt, Assigned: Gene Lian (I already quit Mozilla))

Tracking

({feature})

unspecified
ARM
Gonk (Firefox OS)
feature
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed, firefox20 fixed, b2g18 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Bug 805333 adds an audio permission which allows content to compete for audio channels. More important audio channels are only available to more trusted apps types. Each channel has an associated permission, and the app type (web, privileged, certified) needs to be checked at the time of granting the permission to ensure that the type requirement is satisfied. The correspondence between app types and audio permission is:

audio-normal: web, privileged, certified
audio-content: web, privileged, certified
audio-notification: privileged, certified
audio-alarm:  privileged, certified
audio-telephony: certified
audio-ringer: certified
audio-publicnotification: certified

I think there most likely just needs to be a check of this app type as part of the "expandPermissions" function in PermissionsInstaller.jsm
(Reporter)

Updated

5 years ago
Blocks: 815565
(Assignee)

Updated

5 years ago
Assignee: nobody → clian
(Assignee)

Comment 1

5 years ago
Created attachment 689646 [details] [diff] [review]
WIP Patch

Hi Mounir,

This is just a WIP patch but almost done. Hoping to have your feedback first for how to deploy the channel-to-permission mapping table. My design is shown as below:

   audio: {
     app: [
       ALLOW_ACTION, // "normal"
       ALLOW_ACTION, // "content"
       DENY_ACTION,  // "notification"
       DENY_ACTION,  // "alarm"
       DENY_ACTION,  // "telephony"
       DENY_ACTION,  // "ringer"
       DENY_ACTION   // "publicnotification"
     ],
     privileged: [
       ALLOW_ACTION, // "normal"
       ALLOW_ACTION, // "content"
       ALLOW_ACTION, // "notification"
       ALLOW_ACTION, // "alarm"
       DENY_ACTION,  // "telephony"
       DENY_ACTION,  // "ringer"
       DENY_ACTION   // "publicnotification"
     ],
     certified: [
       ALLOW_ACTION, // "normal"
       ALLOW_ACTION, // "content"
       ALLOW_ACTION, // "notification"
       ALLOW_ACTION, // "alarm"
       ALLOW_ACTION, // "telephony"
       ALLOW_ACTION, // "ringer"
       ALLOW_ACTION  // "publicnotification"
     ],
     channels: [
       "normal",
       "content",
       "notification",
       "alarm",
       "telephony",
       "ringer",
       "publicnotification"
     ]
   },

Does that make sense to you?
Attachment #689646 - Flags: feedback?(mounir)
Comment on attachment 689646 [details] [diff] [review]
WIP Patch

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

Can't we simply name the permissions: "audio-normal", "audio-content", "audio-notification", "audio-alarm", ... ?

I feel like it would make that change *way* simpler and I would believe we do not have that much apps that would require to be fixed. The fix would actually be quite simple: just push a change to Gaia adding those permissions to the app that require them, then push this patch to m-b.

Anyway, whatever you decide to do, Gene, I will be 100% offline for the next 8 days so you should unfortunately find someone else for the review.
Attachment #689646 - Flags: feedback?(mounir)
(Assignee)

Comment 3

5 years ago
(In reply to Mounir Lamouri (:mounir) (on vacation 100% offline until the 16th of December) from comment #2)
> Comment on attachment 689646 [details] [diff] [review]
> WIP Patch
> 
> Review of attachment 689646 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can't we simply name the permissions: "audio-normal", "audio-content",
> "audio-notification", "audio-alarm", ... ?

I was following the design at Bug 805333, where they hoped to use an extra |channels| property to specify the multiple types for a single "audio" permission. That's why I have to deploy the app types in this way. I think it's still advantageous to refactor the "audio" part, which looks a bit cleaner from the point of view of the app's manifest.

> Anyway, whatever you decide to do, Gene, I will be 100% offline for the next
> 8 days so you should unfortunately find someone else for the review.

No worries! Have a good vacation! :)
(Assignee)

Comment 4

5 years ago
Comment on attachment 689646 [details] [diff] [review]
WIP Patch

Hi Jonas,

May I have your feedback on this patch please? This is almost done but before asking for a formal review, I'd like to have your confirmation to see if comment #1 is a proper way to specify the channel-to-permission mapping given the app type.
Attachment #689646 - Flags: feedback?(jonas)
(Assignee)

Comment 5

5 years ago
Created attachment 690303 [details] [diff] [review]
Patch, V2

Hi Jonas & Fabrice,

1. Please see comment #1 for how I deploy the channel-to-permission mapping table (by the given app type).

2. Due the consideration of *channels*, another big change is to make |.expandPermissions()| return an object with the following format: 

  { 'permNames': (string array), 'permValues': (number array) }

where each value in the 'permValues' array specifies the permission value (allow, deny... etc) for each permission name (audio-channel-normal, audio-channel-telephony... etc) in the 'permNames' array. Why we need to do this is because different expanded permissions could now have different permission values even if they are under the same app type (privileged, certified... etc).

3. All the callers of |.expandPermissions()| need to have the corresponding changes, including: 

  a. SystemMessagePermissionsChecker.jsm
  b. PermissionPromptHelper.jsm
  c. test_bug808734.js

4. A minor code clean-up: s/PermissionsTable.jsm/PermissionsInstaller.jsm. Please feel free to let me know if I need to do this in another patch.
Attachment #689646 - Attachment is obsolete: true
Attachment #689646 - Flags: feedback?(jonas)
Attachment #690303 - Flags: review?(jonas)
Attachment #690303 - Flags: review?(fabrice)
Actually, I think Mounir has a good point. I think we ended up creating something overly complicated here rather than just sticking to the pattern that we had.

So maybe we should simply add "audio-channel-normal", "audio-channel-content", etc to the table, just like all the other types that we have.

That mean we can revert expandPermissions to the simpler code that we had before. And it means that future gaia UI around implicit permissions doesn't need to have special code for audio channels.

It would mean that we have to change the various gaia manifests to use the new syntax. I.e. the alarm app would have to change from

permissions: {
  audio: {
    description: "...",
    channels: ["normal", "alarm"]
  },
  ...
}

to

permissions: {
  audio-channel-normal: {
    description: "...",
  },
  audio-channel-alarm: {
    description: "...",
  },
  ...
}


Gene: would you mind making this change. I'm sorry about the churn. The old syntax seemed like a good idea, but the amount of complexity just doesn't seem worth it.

And the new syntax has the advantage that developers are forced to motivate each channel with a description.
Attachment #690303 - Flags: review?(jonas) → review-
(Assignee)

Comment 7

5 years ago
Sounds great! Thanks for the good point! :) I'll come back with the refined patch soon.
(Assignee)

Comment 8

5 years ago
Created attachment 690760 [details]
Part 1, Gaia Change for Audio Permissions in Manifests
Attachment #690760 - Flags: review?(alive)

Updated

5 years ago
QA Contact: fyen
Comment on attachment 690760 [details]
Part 1, Gaia Change for Audio Permissions in Manifests

r=me, be careful to land gaia side as fast as possible when gecko lands.
Attachment #690760 - Flags: review?(alive) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 690762 [details] [diff] [review]
Part 2, Gecko Change for Audio Permissions Models, V3

Life is simpler now. ;) Thanks for your review!
Attachment #690303 - Attachment is obsolete: true
Attachment #690303 - Flags: review?(fabrice)
Attachment #690762 - Flags: review?(jonas)
Comment on attachment 690762 [details] [diff] [review]
Part 2, Gecko Change for Audio Permissions Models, V3

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

Yay! Thanks!
Attachment #690762 - Flags: review?(jonas) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/971c2bbc41aa
https://hg.mozilla.org/releases/mozilla-aurora/rev/267652713bbb
https://hg.mozilla.org/releases/mozilla-beta/rev/337aa6725d31
status-firefox18: --- → fixed
status-firefox19: --- → fixed
Gaia merge: https://github.com/mozilla-b2g/gaia/commit/e580fb74e3430520168d388ef3759dd3d6ada870
https://hg.mozilla.org/mozilla-central/rev/971c2bbc41aa
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
status-firefox20: --- → fixed
Whiteboard: [status-b2g18:fixed]
status-b2g18: --- → fixed
Whiteboard: [status-b2g18:fixed]
You need to log in before you can comment on or make changes to this bug.