809944 - Require access fields in manifests.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gregor Wagner [:gwagner]]

We don't want to grant base permissions for settings and contacts and use the access fields in the manifest files.

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 2

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 679881 [details] [diff] [review]
patch

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

Looks good to me!

Comment 4

[Gregor Wagner [:gwagner]]

Attachment: fix Tests

Comment 5

[Gregor Wagner [:gwagner]]

Comment on attachment 681790 [details] [diff] [review]
fix Tests

Hm it seems that we need access fields here. Will fix it tomorrow.

Comment 6

[Gregor Wagner [:gwagner]]

Attachment: patch

Cu.reportError doesn't show up in logcat so I added dump as well.

Comment 7

[Gregor Wagner [:gwagner]]

Attachment: tests

Comment 8

[David Dahl :ddahl]

Comment on attachment 682184 [details] [diff] [review]
tests

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

Looks good.

::: dom/apps/src/PermissionsInstaller.jsm
@@ +287,5 @@
>        aAccess && !tableEntry.access) {
>      Cu.reportError("PermissionsTable.jsm: expandPermissions: Invalid Manifest : " +
>                     aPermName + " " + aAccess + "\n");
> +    dump("PermissionsTable.jsm: expandPermissions: Invalid Manifest: " +
> +         aPermName + " " + aAccess + "\n");

I thought we wanted to throw here in case the manifest is invalid? Has that thinking changed?

::: dom/tests/browser/browser_webapps_permissions.js
@@ +29,5 @@
>    "alarms": "allow",
> +  "contacts-write": "prompt",
> +  "contacts-read": "prompt",
> +  "device-storage:apps-read": "prompt",
> +  "device-stroage:apps-write": "unknown"

nit: spelling "device-storage:apps-write"

@@ +69,5 @@
>        var nav = XPCNativeWrapper.unwrap(browser.contentWindow.navigator);
>        ok(nav.mozApps, "we have a mozApps property");
>        var navMozPerms = nav.mozPermissionSettings;
>        ok(navMozPerms, "mozPermissions is available");
> +

Thanks! that was some flotsom and jetsom from me!

Comment 9

[Gregor Wagner [:gwagner]]

(In reply to David Dahl :ddahl from comment #8)
> Comment on attachment 682184 [details] [diff] [review]
> tests
> 
> Review of attachment 682184 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> ::: dom/apps/src/PermissionsInstaller.jsm
> @@ +287,5 @@
> >        aAccess && !tableEntry.access) {
> >      Cu.reportError("PermissionsTable.jsm: expandPermissions: Invalid Manifest : " +
> >                     aPermName + " " + aAccess + "\n");
> > +    dump("PermissionsTable.jsm: expandPermissions: Invalid Manifest: " +
> > +         aPermName + " " + aAccess + "\n");
> 
> I thought we wanted to throw here in case the manifest is invalid? Has that
> thinking changed?
> 

The problem is following test:
// test unknown permission
{
  permission: UNKNOWN,
  access: READWRITE,
  expected: []
}

We throw and never return an empty array.

Comment 10

[Gregor Wagner [:gwagner]]

Oh ignore my last comment. The real problem is the access field for storage:

// test substitute
{
  permission: "storage",
  access: READWRITE,
  expected: ["indexedDB-unlimited", "offline-app", "pin-app"]
},

I will remove the access field and keep the throw.

Comment 11

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80f4c2456dee
https://hg.mozilla.org/integration/mozilla-inbound/rev/98ff02ab98cc

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/98ff02ab98cc
https://hg.mozilla.org/mozilla-central/rev/80f4c2456dee

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ac67728fb89b
https://hg.mozilla.org/releases/mozilla-aurora/rev/6c776a1f9392

https://hg.mozilla.org/releases/mozilla-beta/rev/29573b125247
https://hg.mozilla.org/releases/mozilla-beta/rev/c0d49ede1478