874196 - Add nsIPermissionManager.getPermissionObject

Status: RESOLVED FIXED

(Core :: Permission Manager, defect, P2)

Description

[Benjamin Smedberg]

For the plugin doorhanger, I need to know several things that are not easy to extract from the current permission-manager API:

* When a permission is present, which parent domain did it match?
* When a permission is present, is it a session or persistent permission?

After talking with mounir on IRC, I believe that the simplest way to solve both of these problems is to add a .getPermission API which returns the permission object associated with a permission request.

Comment 1

[Benjamin Smedberg]

Attachment: Add nsIPermissionManager.getPermission, rev. 1

Passes tryserver: https://tbpl.mozilla.org/?tree=Try&rev=b17a021dec36

Comment 2

[Mounir Lamouri (:mounir)]

Comment on attachment 752181 [details] [diff] [review]
Add nsIPermissionManager.getPermission, rev. 1

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

The patch looks good but I would like a bit more testing. Could you create a new test file (test_permmanager_getpermission.js) and do a bit more intensive tests there?

::: extensions/cookie/nsPermission.cpp
@@ +8,4 @@
>  
>  // nsPermission Implementation
>  
> +NS_IMPL_CLASSINFO(nsPermission, NULL, 0, {0})

Shouldn't that be nullptr?

::: extensions/cookie/nsPermissionManager.cpp
@@ +1055,5 @@
> +  if (!entry) {
> +    return NS_OK;
> +  }
> +
> +  int32_t idx = entry->GetPermissionIndex(typeIndex);

Could you write a comment explaining that you do that because GetPermission(typeIndex) will return a fake UNKNOWN_ACTION permission if there is no permission record for the given type.

@@ +1063,5 @@
> +
> +  PermissionEntry& perm = entry->GetPermissions()[idx];
> +  nsCOMPtr<nsIPermission> r = new nsPermission(entry->GetKey()->mHost,
> +                                               entry->GetKey()->mAppId,
> +                                               entry->GetKey()->mIsInBrowserElement,

I think you could as well use host, appId and isInBrowserElement, no need to use the Entry who anyway will have the same value but will require a bit more work to get them.

::: netwerk/base/public/nsIPermissionManager.idl
@@ +188,5 @@
>                                          in string type);
>  
> +  /**
> +   * Get the permission object associated with the given URI and action.
> +   * @param uri       The principal

In those two lines, you speak about URI ("given URI" and "@param uri") but you want "principal".

@@ +193,5 @@
> +   * @param type      A case-sensitive ASCII string identifying the consumer
> +   * @param exactHost If true, only the specific host will be matched,
> +   *                  @see testExactPermission. If false, subdomains will
> +   *                  also be searched, @see testPermission.
> +   * @returns The matching permission object, or null if not found.

I will try to be more verbose, whether there or in a @note explaining that it returns a permission object iif the permission object has an entry for the given parameters. If there is no returned object, that doesn't mean the permission is denied but that there is nothing registered which could be as well authorisation (eg. chrome code).

@@ +195,5 @@
> +   *                  @see testExactPermission. If false, subdomains will
> +   *                  also be searched, @see testPermission.
> +   * @returns The matching permission object, or null if not found.
> +   */
> +  nsIPermission getPermission(in nsIPrincipal principal,

I wonder if "getPermissionObject()" wouldn't be a better name to make sure consumers don't end up caling this to check for permissions. You can keep the current name if you prefer it.

@@ +198,5 @@
> +   */
> +  nsIPermission getPermission(in nsIPrincipal principal,
> +                              in string type,
> +                              in boolean exactHost);
> +  

nit: trailing ws

Comment 3

[Benjamin Smedberg]

> @@ +1063,5 @@
> > +
> > +  PermissionEntry& perm = entry->GetPermissions()[idx];
> > +  nsCOMPtr<nsIPermission> r = new nsPermission(entry->GetKey()->mHost,
> > +                                               entry->GetKey()->mAppId,
> > +                                               entry->GetKey()->mIsInBrowserElement,
> 
> I think you could as well use host, appId and isInBrowserElement, no need to
> use the Entry who anyway will have the same value but will require a bit
> more work to get them.

Actually at least host will not always have the same value, because it may be a permission for a parent host if we're not using exact matching.

Working on the rest.

Comment 4

[Benjamin Smedberg]

Attachment: Add an API to get the specifics of a permission given a host/type: this will allow the plugin click-to-activate UI to manage permissions by the matching host and determine whether the current permission is per-session or persistent. r?jdm sr?mounir

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 756172 [details] [diff] [review]
Add an API to get the specifics of a permission given a host/type: this will allow the plugin click-to-activate UI to manage permissions by the matching host and determine whether the current permission is per-session or persistent. r?jdm sr?mounir

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

Thanks :)

Comment 6

[Benjamin Smedberg]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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