Last Comment Bug 775713 - nsStrictTransportSecurityService should use Principal to access the PermissionManager
: nsStrictTransportSecurityService should use Principal to access the Permissio...
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on: 758258 769583
Blocks: app-data-jars
  Show dependency treegraph
 
Reported: 2012-07-19 13:06 PDT by Mounir Lamouri (:mounir)
Modified: 2012-09-01 08:00 PDT (History)
4 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Patch (7.70 KB, patch)
2012-07-19 13:06 PDT, Mounir Lamouri (:mounir)
brian: review+
Details | Diff | Splinter Review
Patch (7.33 KB, patch)
2012-07-21 18:03 PDT, Mounir Lamouri (:mounir)
honzab.moz: superreview+
mounir: checkin+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2012-07-19 13:06:51 PDT
Created attachment 643986 [details] [diff] [review]
Patch
Comment 1 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-07-19 13:19:24 PDT
Comment on attachment 643986 [details] [diff] [review]
Patch

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

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +602,5 @@
>  
>      // Use whatever we ended up with, which defaults to UNKNOWN_ACTION.
>      return NS_OK;
>  }
> +

namespace {

@@ +603,5 @@
>      // Use whatever we ended up with, which defaults to UNKNOWN_ACTION.
>      return NS_OK;
>  }
> +
> +/* static */ nsresult

remove /* static */

@@ +615,5 @@
> +      do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
> +   NS_ENSURE_SUCCESS(rv, rv);
> +
> +   return securityManager->GetNoAppCodebasePrincipal(aURI, aPprincipal);
> +}

} // unnamed namespace.

::: security/manager/boot/src/nsStrictTransportSecurityService.h
@@ +124,5 @@
> +  /**
> +   * Returns a principal (aPrincipal) corresponding to aURI.
> +   * This is used to interact with the permission manager.
> +   */
> +  static nsresult GetPrincipalForURI(nsIURI* aURI, nsIPrincipal** aPrincipal);

Remove this declaration. This does not need to be a member function of nsSTSS.
Comment 2 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-07-19 13:43:12 PDT
Comment on attachment 643986 [details] [diff] [review]
Patch

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

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +608,5 @@
> +GetPrincipalForURI(nsIURI* aURI, nsIPrincipal** aPrincipal)
> +{
> +   // The permission manager wants a principal but don't actually check a
> +   // permission but a data we saved in the permission manager so we are good by
> +   // creating a no-app codebase principal and send it to the permission manager.

Replace this comment with this one:

// We want all apps to share HSTS state, so this is one
// of the few places where we do not silo persistent
// state by extended origin.
Comment 3 Honza Bambas (:mayhemer) 2012-07-19 15:33:43 PDT
Comment on attachment 643986 [details] [diff] [review]
Patch

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

This should, from fairness, land (and also merge) after the patch for bug 760307 since this one is much more simpler.  But depends on priorities.

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +614,5 @@
> +   nsCOMPtr<nsIScriptSecurityManager> securityManager =
> +      do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
> +   NS_ENSURE_SUCCESS(rv, rv);
> +
> +   return securityManager->GetNoAppCodebasePrincipal(aURI, aPprincipal);

typo: aPprincipal

What exactly GetNoAppCodebasePrincipal does?  It probably still just sits in some patch yet landed that is not listed in deps, so I cannot do the proper review here.
Comment 4 Mounir Lamouri (:mounir) 2012-07-19 15:38:24 PDT
(In reply to Honza Bambas (:mayhemer) from comment #3)
> Comment on attachment 643986 [details] [diff] [review]
> Patch
> 
> Review of attachment 643986 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This should, from fairness, land (and also merge) after the patch for bug
> 760307 since this one is much more simpler.  But depends on priorities.
> 
> ::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
> @@ +614,5 @@
> > +   nsCOMPtr<nsIScriptSecurityManager> securityManager =
> > +      do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
> > +   NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +   return securityManager->GetNoAppCodebasePrincipal(aURI, aPprincipal);
> 
> typo: aPprincipal
> 
> What exactly GetNoAppCodebasePrincipal does?  It probably still just sits in
> some patch yet landed that is not listed in deps, so I cannot do the proper
> review here.

It's like getCodebasePrincipal(), see bug 758258. Basically, we have getAppCodebasePrincipal() and getNoAppCodebasePrincipal() to get a principal with app info or not. GetNoAppCodebasePrincipal() is exactly siimilar to getCodebasePrincipal() because it has no app info. However, we are going to remove getCodebasePrincipal() to make sure people know what they are doing.
Comment 5 Mounir Lamouri (:mounir) 2012-07-19 15:39:56 PDT
Comment on attachment 643986 [details] [diff] [review]
Patch

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

I fixed the typo and apply bsmith review comments locally.
Comment 6 Honza Bambas (:mayhemer) 2012-07-19 15:42:44 PDT
Mounir, can you please add dep bug where GetNoAppCodebasePrincipal() has been introduced?
Comment 7 Honza Bambas (:mayhemer) 2012-07-21 14:58:03 PDT
(In reply to Mounir Lamouri (:mounir) from comment #5)
> I fixed the typo and apply bsmith review comments locally.

Can you please submit the newer patch?
Comment 8 Mounir Lamouri (:mounir) 2012-07-21 18:03:29 PDT
Created attachment 644701 [details] [diff] [review]
Patch
Comment 9 Ed Morley [:emorley] 2012-07-24 03:02:01 PDT
https://hg.mozilla.org/mozilla-central/rev/09d63c143799

Note You need to log in before you can comment on or make changes to this bug.