Closed Bug 819136 Opened 12 years ago Closed 12 years ago

Check application types when granting audio-* permissions

Categories

(Firefox OS Graveyard :: General, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

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

RESOLVED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: pauljt, Assigned: airpingu)

References

Details

(Keywords: feature)

Attachments

(2 files, 2 obsolete files)

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
Assignee: nobody → clian
Attached patch WIP Patch (obsolete) — Splinter Review
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)
(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! :)
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)
Attached patch Patch, V2 (obsolete) — Splinter Review
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.
Sounds great! Thanks for the good point! :) I'll come back with the refined patch soon.
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+
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/mozilla-central/rev/971c2bbc41aa
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [status-b2g18:fixed]
Whiteboard: [status-b2g18:fixed]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: