Closed Bug 827895 Opened 7 years ago Closed 7 years ago

Settings app should not show the session permissions of apps

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
blocking-basecamp +
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 3 obsolete files)

No description provided.
OS: Linux → All
Hardware: x86_64 → All
Attached patch patch (obsolete) — Splinter Review
The test for this bug covers the bug 827327.
Attachment #699621 - Flags: review?(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.
Attachment #699621 - Flags: review?(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?
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.
Attached patch patch b (obsolete) — Splinter Review
1. separated test
2. idl UUID untouched.
Attachment #699621 - Attachment is obsolete: true
Attachment #699861 - Flags: review?(mounir)
Attached patch patch c (obsolete) — Splinter Review
Attachment #699861 - Attachment is obsolete: true
Attachment #699861 - Flags: review?(mounir)
Attachment #700424 - Flags: review?(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.
Attachment #700424 - Flags: review?(mounir) → review+
> 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.
Attached patch patch dSplinter Review
Attachment #700424 - Attachment is obsolete: true
Attachment #700607 - Flags: review+
Attachment #700607 - Attachment description: patch 3c - encoding + dictionary → patch d
blocking-basecamp: --- → ?
Blocking, and marking fixed since this already landed :)
Status: NEW → RESOLVED
blocking-basecamp: ? → +
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.