805322 - Device Storage - Application Permissions do not match that of the Permission Matrix

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P1)

Description

[Doug Turner (:dougt)]

+++ This bug was initially created as a clone of Bug #805178 +++

1) fixes device storage so that it knows about app status
2) adds a bunch of tests to ensure that we don't do anything terrible stupid (like expose device storage to the wrong appstatus)
3) that's it.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

https://tbpl.mozilla.org/?tree=Try&rev=e4d50602f288

Comment 2

[Mounir Lamouri (:mounir)]

Comment on attachment 675802 [details] [diff] [review]
patch v.1

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

I think the specialPowersAPI change should be fixed and I wonder what we should do regarding the appStatus check.

Everything else is mostly nits.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +1700,5 @@
>    mPrincipal = doc->NodePrincipal();
>  
> +  // if we are testing, ignore application settings
> +  if (mozilla::Preferences::GetBool("device.storage.testing", false)) {
> +    return NS_OK;

I'm not a big fan of that. I don't see anything that requires this hack.

@@ +1704,5 @@
> +    return NS_OK;
> +  }
> +
> +  // Device storage only works in APP_STATUS_PRIVILEGED or APP_STATUS_CERTIFIED apps.
> +  if (mPrincipal->GetAppStatus() < nsIPrincipal::APP_STATUS_PRIVILEGED) {

I would prefer the check to be explicit.

I wonder if we should make this check here though. I'm not aware of other APIs doing those checks. AFAIK, they assume that if the permission is there that means the caller is allowed to use the API. We don't have something doing the app status check when the permission is requested? I don't think the permission manager does that but something else maybe?

If not, I think it might be better to have that kind of stuff in the permission manager than on each API.

@@ +1712,5 @@
> +  nsCOMPtr<nsIPermissionManager> pm = do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);
> +  NS_ENSURE_TRUE(pm, NS_ERROR_FAILURE);
> +
> +  nsCString type;
> +  nsresult rv = DeviceStorageTypeChecker::GetPermissionForType(aType, type);

Sic... this method doesn't do what it names would make me believe it does.
I would have name it "GetPermissionNameForType" because it's not a permission you are getting.

Also, I think you should rename |type| to |permissionName|.

@@ +1714,5 @@
> +
> +  nsCString type;
> +  nsresult rv = DeviceStorageTypeChecker::GetPermissionForType(aType, type);
> +  if (NS_FAILED(rv)) {
> +    return rv;

NS_ENSURE_SUCCESS(rv, rv);

@@ +1720,5 @@
> +
> +  uint32_t permission;
> +  rv = pm->TestPermissionFromPrincipal(mPrincipal, type.get(), &permission);
> +  if (NS_FAILED(rv)) {
> +    return rv;

NS_ENSURE_SUCCESS(rv, rv);

::: dom/devicestorage/test/test_app_permissions.html
@@ +26,5 @@
> +function VerifyDenyAccess(iframe, data) {
> +  var types = ["pictures", "music", "videos", "sdcard", "apps"];
> +  for (i in types) {
> +    var storage = iframe.contentDocument.defaultView.navigator.getDeviceStorage(types[i]);
> +    ok(storage == null, "device storage type " + types[i] + " should not be available to " + data.description);

nit: is(storage, null, [...]);

@@ +36,5 @@
> +function VerifyAllowAccess(iframe, data) {
> +  var types = ["pictures", "music", "videos", "sdcard"];
> +  for (i in types) {
> +    var storage = iframe.contentDocument.defaultView.navigator.getDeviceStorage(types[i]);
> +    ok(storage != null, "device storage type " + types[i] + " should be available to " + data.description);

nit: isnot(storage, null, [...]);

@@ +43,5 @@
> +}
> +
> +function VerifyAllowAccessToApps(iframe, data) {
> +  var storage = iframe.contentDocument.defaultView.navigator.getDeviceStorage("apps");
> +  ok(storage != null, "device storage type apps should be available to " + data.description);

nit: isnot(storage, null, [...]);

@@ +50,5 @@
> +}
> +
> +function VerifyDenyAccessToApps(iframe, data) {
> +  var storage = iframe.contentDocument.defaultView.navigator.getDeviceStorage("apps");
> +  ok(storage == null, "device storage type apps should not be available to " + data.description);

nit: is(storage, null, [..]);

@@ +187,5 @@
> +  }
> +}
> +
> +var gTestRunner = runTest();
> +SpecialPowers.addPermission("browser", true, gTestUri);

You should remove that permission at the end of the test.

@@ +197,5 @@
> +SpecialPowers.pushPrefEnv({'set': [["dom.mozBrowserFramesEnabled", true],
> +                                   ["device.storage.enabled", true],
> +                                   ["device.storage.prompt.testing", false],
> +                                   ["security.apps.privileged.CSP.default", DEFAULT_CSP_PRIV],
> +                                   ["security.apps.certified.CSP.default", DEFAULT_CSP_CERT]]},

You should revert those pref at the end of the test.

::: testing/specialpowers/content/specialpowersAPI.js
@@ +1238,5 @@
> +      case 2:
> +        permission = Ci.nsIPermissionManager.PROMPT_ACTION;
> +        break;
> +      default:
> +        permission = Ci.nsIPermissionManager.UNKNOWN_ACTION;

I'm not a big fan of this change because someone might pass Ci.nsIPermissionManager.*_ACTION and the values do not match (passing Ci.nsIPermissionManager.UNKNOWN_ACTION would end up setting DENY_ACTION).

A solution would be to change addPermission() to take strings or boolean.
Booleans, would have the same behaviour as it has now (true => ALLOW, false => DENY). Strings would do what you want ("deny" => DENY, "allow" => ALLOW, "prompt" => PROMPT). How does that sound?

Comment 3

[Mounir Lamouri (:mounir)]

With the permission table [1] we shouldn't allow apps to get permissions they can't have so I think you shouldn't test the app status.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsInstaller.jsm#53

Comment 4

[Doug Turner (:dougt)]

> You should remove that permission at the end of the test.

and

> You should revert those pref at the end of the test.

<dougt> jmaher: q - if I call SpecialPowers.pushPrefEnv(), do i need to revert those changes at the end of a test run?

<jmaher> dougt: it gets cleaned up by itself
<jmaher> dougt: so should pushPermissionsEnv()

Comment 5

[Doug Turner (:dougt)]

mounir, so, what i did was to rely on the permission manager.  This is called via the nsIContentPermissionPrompt.  Hence, I do not need to directly test the AppStatus.  I do not need to make changes to the specialpowersAPI for permissions.

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 676371 [details] [diff] [review]
patch v.2

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

mounir, we probably also want joel to review.

joel, feedback on the approach appreciated.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

Comment on attachment 676371 [details] [diff] [review]
patch v.2

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

if we can address all the nit's below especially the one about the MockFilePicker name in marionette.

::: dom/devicestorage/test/test_app_permissions.html
@@ +37,5 @@
> +SimpleTest.waitForExplicitFinish();
> +
> +function TestAdd(iframe, data) {
> +
> +  createTestFile();

we don't need to do this for TestAdd

@@ +40,5 @@
> +
> +  createTestFile();
> +
> +  var storage = iframe.contentDocument.defaultView.navigator.getDeviceStorage(data.type);
> +  isnot(storage, null, "Should be able to get storage object for " + data.type);

would we want to check a value of the request, not just that it is null?

@@ +42,5 @@
> +
> +  var storage = iframe.contentDocument.defaultView.navigator.getDeviceStorage(data.type);
> +  isnot(storage, null, "Should be able to get storage object for " + data.type);
> +
> +  var blob = new Blob(["Kyle Huey is not a helicopter."], {type: data.mimeType});

this is an awesome test string.

@@ +45,5 @@
> +
> +  var blob = new Blob(["Kyle Huey is not a helicopter."], {type: data.mimeType});
> +
> +  request = storage.addNamed(blob, randomFilename(100) + "hi" + data.fileExtension);
> +  isnot(request, null, "Should be able to get request");

would we want to check a value of the request, not just that it is null?

::: testing/marionette/jar.mn
@@ +28,5 @@
>    content/MozillaLogger.js (../specialpowers/content/MozillaLogger.js)
>  
>  % resource specialpowers %modules/
>    modules/MockFilePicker.jsm (../specialpowers/content/MockFilePicker.jsm)
> +  modules/MockFilePicker.jsm (../specialpowers/content/MockPermissionPrompt.jsm)

this should be in modules/MockPermissionsPrompt, not MockFilePicker

::: testing/specialpowers/content/MockPermissionPrompt.jsm
@@ +62,5 @@
> +  prompt: function(request) {
> +
> +    this.promptResult = Services.perms.testExactPermissionFromPrincipal(request.principal,
> +                                                                        request.type);
> +dump("test resulted in " + this.promptResult + "\n");

this looks like extra debugging left behind.

@@ +65,5 @@
> +                                                                        request.type);
> +dump("test resulted in " + this.promptResult + "\n");
> +
> +    if (this.promptResult == Ci.nsIPermissionManager.ALLOW_ACTION) {
> +      dump("Firing allow\n");

extra debugging?  if not, lets make this a message that we can understand rather than a random message.

@@ +69,5 @@
> +      dump("Firing allow\n");
> +      request.allow();
> +    }
> +    else {
> +      dump("Firing cancel\n");

extra debugging?  if not, lets make this a message that we can understand rather than a random message.

Comment 9

[Doug Turner (:dougt)]

>  would we want to check a value of the request, not just that it is null?

We have other tests that introspect these objects.  I just care if they exist or not in this new test case.

Everything else fixed.  Thanks Joel.

Comment 10

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10310453f2c2

Comment 11

[Mounir Lamouri (:mounir)]

Comment on attachment 676371 [details] [diff] [review]
patch v.2

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

Doug, could you push a follow-up to clean-up the test before calling SimpleTest.finish()?

::: dom/devicestorage/test/test_app_permissions.html
@@ +606,5 @@
> +}
> +
> +createTestFile();
> +var gTestRunner = runTest();
> +SpecialPowers.addPermission("browser", true, gTestUri);

Do you remove this permission at the end of the test?

@@ +617,5 @@
> +                                   ["device.storage.enabled", true],
> +                                   ["device.storage.testing", true],
> +                                   ["device.storage.prompt.testing", false],
> +                                   ["security.apps.privileged.CSP.default", DEFAULT_CSP_PRIV],
> +                                   ["security.apps.certified.CSP.default", DEFAULT_CSP_CERT]]},

SpecialPowers API isn't nice enough to remove those prefs. You should remove them yourself.

Comment 12

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/10310453f2c2

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/261667b3cbf5