Enable testing permissions using URIs without having to mint principals

RESOLVED FIXED in Firefox 55

Status

()

Core
Permission Manager
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 months ago
The permissions manager store uses principal origins with suffix in the
key entry, but for the API entry points where we accept a raw nsIURI, we
currently mint a new codebase principal with a blank OriginAttributes
only to read out the origin string effectively, since the suffix is
guaranteed to always be an empty string in this case.

This can be slow, so this patch adds a fast path to bypass minting a new
principal and uses ContentPrincipal::GenerateOriginNoSuffixFromURI() to
generate the origin string from the input nsIURI directly.
(Assignee)

Comment 1

6 months ago
Created attachment 8865145 [details] [diff] [review]
Enable testing permissions using URIs without having to mint principals
Attachment #8865145 - Flags: review?(michael)
(Assignee)

Updated

6 months ago
Assignee: nobody → ehsan
(Assignee)

Updated

6 months ago
Blocks: 1353179
(Assignee)

Updated

6 months ago
Blocks: 1347376
No longer blocks: 1353179
Comment on attachment 8865145 [details] [diff] [review]
Enable testing permissions using URIs without having to mint principals

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

::: extensions/cookie/nsPermissionManager.cpp
@@ +238,5 @@
>  
>  // This function produces a nsIPrincipal which is identical to the current
>  // nsIPrincipal, except that it has one less subdomain segment. It returns
>  // `nullptr` if there are no more segments to remove.
> +static already_AddRefed<nsIPrincipal>

I don't think that the `static` here is necessary, as this function is within an anonymous namespace.

@@ +243,1 @@
>  GetNextSubDomainPrincipal(nsIPrincipal* aPrincipal)

Please change this method to use GetNextSubdomainURI internally to avoid code duplication.

@@ +286,5 @@
>  
> +// This function produces a nsIURI which is identical to the current
> +// nsIURI, except that it has one less subdomain segment. It returns
> +// `nullptr` if there are no more segments to remove.
> +static already_AddRefed<nsIURI>

Same with this static.

@@ +2219,5 @@
>  }
>  
>  nsresult
>  nsPermissionManager::CommonTestPermission(nsIPrincipal* aPrincipal,
> +                                          nsIURI     *aURI,

I don't like that we need to pass in both aURI and aPrincipal here. Can we split this into 3 methods, one which performs the prelude up to the call to GetPermissionHashKey for aPrincipal, one for aURI, and then a common internal function for the suffix?

@@ +2262,5 @@
>  
>      return NS_OK;
>    }
>  
> +  MOZ_ASSERT_IF(aPrincipal, PermissionAvaliable(aPrincipal, aType));

In debug builds, please fabricate a principal here and pass it to PermissionAvaliable. I don't want to lose this assertion if you call through the nsIURI*-based bindings.
Attachment #8865145 - Flags: review?(michael)
(Assignee)

Comment 3

5 months ago
(In reply to Michael Layzell [:mystor] from comment #2)
> @@ +2219,5 @@
> >  }
> >  
> >  nsresult
> >  nsPermissionManager::CommonTestPermission(nsIPrincipal* aPrincipal,
> > +                                          nsIURI     *aURI,
> 
> I don't like that we need to pass in both aURI and aPrincipal here. Can we
> split this into 3 methods, one which performs the prelude up to the call to
> GetPermissionHashKey for aPrincipal, one for aURI, and then a common
> internal function for the suffix?

I don't think that really improves the code, and it would make things less efficient by adding a potential function call and more importantly duplicate the logic across the URL and principal code paths, which makes it likely for a bug fix in the future in one code to not be made in the other, etc.  If your objection is to the interface of this function, I can fix that at the interface level by providing two overloads.  What do you think?
Flags: needinfo?(michael)
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #3)
> (In reply to Michael Layzell [:mystor] from comment #2)
> > @@ +2219,5 @@
> > >  }
> > >  
> > >  nsresult
> > >  nsPermissionManager::CommonTestPermission(nsIPrincipal* aPrincipal,
> > > +                                          nsIURI     *aURI,
> > 
> > I don't like that we need to pass in both aURI and aPrincipal here. Can we
> > split this into 3 methods, one which performs the prelude up to the call to
> > GetPermissionHashKey for aPrincipal, one for aURI, and then a common
> > internal function for the suffix?
> 
> I don't think that really improves the code, and it would make things less
> efficient by adding a potential function call and more importantly duplicate
> the logic across the URL and principal code paths, which makes it likely for
> a bug fix in the future in one code to not be made in the other, etc.  If
> your objection is to the interface of this function, I can fix that at the
> interface level by providing two overloads.  What do you think?

Yeah, that's fine.
Flags: needinfo?(michael)
(Assignee)

Comment 5

5 months ago
Created attachment 8869591 [details] [diff] [review]
Enable testing permissions using URIs without having to mint principals

The permissions manager store uses principal origins with suffix in the
key entry, but for the API entry points where we accept a raw nsIURI, we
currently mint a new codebase principal with a blank OriginAttributes
only to read out the origin string effectively, since the suffix is
guaranteed to always be an empty string in this case.

This can be slow, so this patch adds a fast path to bypass minting a new
principal and uses ContentPrincipal::GenerateOriginNoSuffixFromURI() to
generate the origin string from the input nsIURI directly.
Attachment #8869591 - Flags: review?(michael)
(Assignee)

Updated

5 months ago
Attachment #8865145 - Attachment is obsolete: true
Comment on attachment 8869591 [details] [diff] [review]
Enable testing permissions using URIs without having to mint principals

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

::: extensions/cookie/nsPermissionManager.cpp
@@ +248,3 @@
>  {
>    nsCOMPtr<nsIURI> uri;
> +  nsresult rv = aURI->Clone(getter_AddRefs(uri));

Can we move the clone until after GetNextSubDomainForHost so that we don't bother copying it unless we actually need it to return?

@@ +2227,3 @@
>  {
> +  MOZ_ASSERT(aPrincipal || aURI);
> +  NS_ENSURE_ARG_POINTER(aPrincipal || aURI);

Can we have something like MOZ_ASSERT_IF(aPrincipal, !aURI) as well here, just to make sure that we don't have both?
Attachment #8869591 - Flags: review?(michael) → review+

Comment 7

5 months ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b115265af36
Enable testing permissions using URIs without having to mint principals; r=mystor
https://hg.mozilla.org/mozilla-central/rev/8b115265af36
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

4 months ago
Blocks: 1379345
No longer blocks: 1379345
Depends on: 1377109
You need to log in before you can comment on or make changes to this bug.