Closed Bug 808734 Opened 7 years ago Closed 7 years ago

PermissionsInstaller.jsm AllPossiblePermissions doesn't expand correctly

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set

Tracking

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

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: dchanm+bugzilla, Assigned: dchanm+bugzilla)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

Incorrect access strings are passed to expandPermissions in AllPossiblePermissions which results in inccorect output [1]

361     for each (let access in PermissionsTable[permName].access) {


expandPermissions expects access to be one of the const defined above
23 // Permission access flags
24 const READONLY = "readonly";
25 const CREATEONLY = "createonly";
26 const READCREATE = "readcreate";
27 const READWRITE = "readwrite";

The fix looks simple, changing from iterating over access and passing in READWRITE. expandPermissions should strip the extra permissions at [2]



[1] - http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsInstaller.jsm#361
[2] - http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsInstaller.jsm#340
Attached patch Fix permission expansion (obsolete) — Splinter Review
Attachment #678433 - Flags: review?(ddahl)
Comment on attachment 678433 [details] [diff] [review]
Fix permission expansion

Clearing flag for now. It appears that there is a separate issue where AllPossiblePermissions ends up with a nested structure

[
resource-lock,
alarms,
...,
[contacts, contact-read, contacts-write, ...],
power,
...
]

I don't think contacts is suppose to be nested as such since this line just iterates over AllPossiblePermissions

http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsInstaller.jsm#403
Attachment #678433 - Flags: review?(ddahl)
Comment on attachment 678433 [details] [diff] [review]
Fix permission expansion

This works on nightly build, will look into the other issue more which is occurring on my b2g arm emulator build.
Attachment #678433 - Flags: review?(ddahl)
(In reply to David Chan [:dchan] from comment #3)
> This works on nightly build, will look into the other issue more which is
> occurring on my b2g arm emulator build.

What is the other issue?
Comment on attachment 678433 [details] [diff] [review]
Fix permission expansion

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

Ok, I think his will work, however, I think you might consider an xpcshell test to exercise these changes? Perhaps the existing tests also work ok. (I think we might consider xpcshell tests when we can isolate things down to the JSM.)

Can you also ask gwagner for a look at this?

::: dom/apps/src/PermissionsInstaller.jsm
@@ +365,5 @@
>        AllPossiblePermissions.concat(expandPermissions(permName));
>    }
>  }
>  
>  this.PermissionsInstaller = {

I don't understand this change and the apparent whitespace change below is probably not required, correct?
Attachment #678433 - Flags: review?(ddahl) → review+
(In reply to David Dahl :ddahl from comment #5)
> Comment on attachment 678433 [details] [diff] [review]
> Fix permission expansion
> 
> Review of attachment 678433 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, I think his will work, however, I think you might consider an xpcshell
> test to exercise these changes? Perhaps the existing tests also work ok. (I
> think we might consider xpcshell tests when we can isolate things down to
> the JSM.)
> 
> Can you also ask gwagner for a look at this?
> 
> ::: dom/apps/src/PermissionsInstaller.jsm
> @@ +365,5 @@
> >        AllPossiblePermissions.concat(expandPermissions(permName));
> >    }
> >  }
> >  
> >  this.PermissionsInstaller = {
> 
> I don't understand this change and the apparent whitespace change below is
> probably not required, correct?

The trailing whitespace change can be removed. The only change needed is the change to READWRITE in the expandPermissions call. I'll work on tests.

The other bug appears to be a mochitest bug. See bug 808807
I removed the extraneous whitespace change from PermissionsInstaller.jsm and wrote a xpcshell test. The test passes but will need to be updated once a permissions hack is removed from PermissionsInstaller.jsm.
Attachment #678433 - Attachment is obsolete: true
Attachment #678944 - Flags: review?(ddahl)
Comment on attachment 678944 [details] [diff] [review]
Fix permission expansion and add tests, v3

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

Looks good. Please run this by gwagner ( in case he has not seen it yet )
Attachment #678944 - Flags: review?(ddahl) → review+
:gwagner

See above, what are the next steps?

I would need someone with commit access to checkin the patch
Flags: needinfo?(anygregor)
I was missing 2 tests. Sorry for the bug spam
Attachment #678944 - Attachment is obsolete: true
Attachment #678961 - Flags: review?(ddahl)
Attachment #678961 - Flags: review?(ddahl)
Attachment #678961 - Flags: review?(anygregor)
Attachment #678961 - Flags: review+
Comment on attachment 678961 [details] [diff] [review]
Fix permission expansion and add tests, v4

The patch looks good to me.

So the plan is to remove the 'settings' and 'contacts' only permission. That is only there to give gaia some time for the changes. All the access field support has landed so we can get rid of it.
Attachment #678961 - Flags: review?(anygregor) → review+
Flags: needinfo?(anygregor)
blocking-basecamp: --- → ?
https://hg.mozilla.org/mozilla-central/rev/006e5f86f6a8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Whiteboard: [qa-]
blocking-basecamp: ? → +
blocking-kilimanjaro: ? → ---
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.