Closed Bug 775796 Opened 7 years ago Closed 7 years ago

URL-Classifier code should use nsIPrincipal when calling the permission manager

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: mounir, Assigned: mounir)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files)

No description provided.
Attachment #644098 - Flags: review? → review?(bsmith)
Comment on attachment 644098 [details] [diff] [review]
Part 2 - nsIUrlClassifierDBService.lookup() takes principal

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

Seems reasonable. It is not good that we do not have any tests that actually check to ensure that the data is being isolated on a per-app-ID basis though. I guess that is the case for the other related changes too. What's the plan for testing that?

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +4285,5 @@
>    NS_ENSURE_TRUE(gDbBackgroundThread, NS_ERROR_NOT_INITIALIZED);
> +  NS_ENSURE_ARG(aPrincipal);
> +
> +  nsCOMPtr<nsIURI> uri;
> +  nsresult rv = aPrincipal->GetURI(getter_AddRefs(uri));

This needs to use the innermost URI.

@@ +4311,5 @@
>          do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);
>  
>        if (permissionManager) {
>          PRUint32 perm;
> +        permissionManager->TestPermissionFromPrincipal(aPrincipal,

Why are we not checking the return value here.

I bet this is a common pattern in callers, so TestPermissionFromPrincipal and related Test* functions should be made to set the value of *perm to the safest possible value (permission not granted).

::: toolkit/components/url-classifier/nsUrlClassifierProxies.cpp
@@ +29,5 @@
>  
>  NS_IMETHODIMP
>  UrlClassifierDBServiceWorkerProxy::LookupRunnable::Run()
>  {
> +  mTarget->Lookup(mPrincipal, mCB);

nit: (void) since you are ignoring the return value.
Attachment #644098 - Flags: review?(bsmith) → review+
Comment on attachment 644096 [details] [diff] [review]
Part 1 - nsIURIClassifier.classify() takes a principal

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

Again, seems reasonable but we're not actually testing the intended effect of this change; we're just testing that we didn't break the existing functionality.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +4255,5 @@
>      new nsUrlClassifierClassifyCallback(c, mCheckMalware, mCheckPhishing);
>    if (!callback) return NS_ERROR_OUT_OF_MEMORY;
>  
> +  nsCOMPtr<nsIURI> uri;
> +  aPrincipal->GetURI(getter_AddRefs(uri));

check return value or (void) if GetURI is infallable.
Attachment #644096 - Flags: review?(bsmith) → review+
I am not a toolkit reviewer, so CCing dolske so he's aware I'm playing in his yard.
I'm fine with Brian reviewing this stuff
Summary: URL-Classifier code should use → URL-Classifier code should use nsIPrincipal when calling the permission manager
Flags: in-testsuite+
Target Milestone: --- → mozilla17
Version: unspecified → Trunk
Attachment #644096 - Flags: checkin+
Attachment #644098 - Flags: checkin+
Whiteboard: [qa-]
Depends on: 811193
Depends on: 813897
You need to log in before you can comment on or make changes to this bug.