827895 - Settings app should not show the session permissions of apps

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

The test for this bug covers the bug 827327.

Comment 2

[Mounir Lamouri (:mounir)]

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

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

I do not think that patch is related to app isolation. Putting your tests there seems wrong.

Regarding the new method, we should discuss what you guys are really trying to do here. I wonder if there are not other solutions.

::: netwerk/base/public/nsIPermissionManager.idl
@@ +182,5 @@
> +   * @param return  see add(), param permission. returns UNKNOWN_ACTION when
> +   *                there is no stored permission for this uri and / or type.
> +   */
> +  uint32_t testExactPermanentPermission(in nsIURI uri,
> +                                        in string type);

No need to pollute the IDL with that method that isn't needed. We want to use the principal as much as we can.

Comment 3

[Mounir Lamouri (:mounir)]

According to a discussion I had with Andrea on IRC, the use case of PermissionSettings.get() is to get all permissions known by the Settings app for a given app.

So, I wonder why we are not simply returning all the permissions of a given app. Is there a real difference between the permissions the Settings app knows and all the possible permissions an app can have? Do we really want to have the Settings app not knowing them? Is it more for a UX perspective?

What I think would be a better patch here is to change nsIPermissionManager to add a GetPermissions(nsIPrincipal principal) method that would return all known permissions associated with that principal (temporary or not).

Then, PermissionsSettings could have one of the following changes:
- getAll() instead of get() that would return all permissions of a given application, or
- get() that could take an array of permissions.

On the nsIPermissionManager side having something like GetPermissions() would be way more useful than a new way to check if a permission is there.

I understand that this would take a bit more time to implement than the current proposal. Maybe we could land the current patch in b2g18 and have a clean fix in m-c done later?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yup. I agree with that. The current API was mostly designed to do something that was low-level so that we could implement any possible behavior that the settings app might want, while also being very simple to implement.

Having something that returns a JSON object with a description of all the permissions granted to an app would indeed be nice. We should still allow setting things on a per-permission basis though.

But I don't think any of that is important to do right now. So I think it's followup which we can do on mozilla-central only. Preferrably once things have slowed down so that we have more time and would have to deal with merge issues less.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch b

1. separated test
2. idl UUID untouched.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: patch c

Comment 7

[Mounir Lamouri (:mounir)]

Comment on attachment 700424 [details] [diff] [review]
patch c

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

The test should be a xpcshell test. No need to suffer with a mochitest here... (I will fix that)

r=me, because the deadline is quite close.

::: extensions/cookie/nsPermissionManager.cpp
@@ +921,5 @@
> +{
> +  NS_ENSURE_ARG_POINTER(aPrincipal);
> +
> +  // System principals do not have URI so we can't try to get
> +  // retro-compatibility here.

Please, remove that comments. Retro-compatibility means nothing for this new method.

@@ +1031,5 @@
>                                                    typeIndex, aExactHostMatch);
> +  if (!entry ||
> +      (!aIncludingSession &&
> +       entry->GetPermission(typeIndex).mNonSessionExpireType ==
> +         nsIPermissionManager::EXPIRE_SESSION)) {

Can we really have mNonSessionExpireType being EXPIRE_SESSION? That is very odd.

@@ +1037,5 @@
>    }
>  
> +  *aPermission = aIncludingSession
> +                 ? entry->GetPermission(typeIndex).mPermission
> +                 : entry->GetPermission(typeIndex).mNonSessionPermission;

nit: indentation for ? and :.

::: netwerk/base/public/nsIPermissionManager.idl
@@ +176,5 @@
> +  /**
> +   * Test whether a website has permission to perform the given action
> +   * ignoring active sessions.
> +   * System principals will always have permissions granted.
> +   * @param principal the principal

nit: leave a blank line between the description and the list of @param.

@@ +181,5 @@
> +   * @param type      a case-sensitive ASCII string, identifying the consumer
> +   * @param return    see add(), param permission. returns UNKNOWN_ACTION when
> +   *                  there is no stored permission for this uri and / or type.
> +   */
> +  uint32_t testExactPermanentPermissionFromPrincipal(in nsIPrincipal principal,

You do not need to name this method ...FromPrincipal(). It just takes a principal.

Comment 8

[Andrea Marchesini [:baku]]

> Can we really have mNonSessionExpireType being EXPIRE_SESSION? That is very
> odd.

hehe. Yes, it can happen. When the object is created, the expireType value is copied into mNonSessionExpireType too. So a new session permission is created (not updated) its mNonSessionExpireType will be EXPIRE_SESSION.

New patch is coming.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: patch d

Comment 10

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4a0d7140eaf

https://hg.mozilla.org/releases/mozilla-b2g18/rev/7a37fdbef00c

Comment 11

[Johnny Stenback (:jst)]

Blocking, and marking fixed since this already landed :)

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b4a0d7140eaf

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b4a0d7140eaf

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b4a0d7140eaf