985652 - Refactor preference actor permissions check

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

At the moment, the preference actor from bug 943251 guards all of its calls to ensure you have the proper pref set.

It's probably simpler to remove all these guards in the actor itself, and then just not list it at all in the "restricted" set[1] of global actors.

[1]: http://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#1153

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Only register pref actor on b2g if certified is allowed

When reviewing bug 943251, I forgot about the outer block list we have on b2g.

So, this removes all the checks inside the actor, and just removes the actor entirely when we forbid certified.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=fd1d437fc82b

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Only register pref actor on b2g if certified is allowed (v2)

Forgot to update the test...

Try: https://tbpl.mozilla.org/?tree=Try&rev=8e6a651e96c3

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8396031 [details] [diff] [review]
Only register pref actor on b2g if certified is allowed (v2)

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

::: b2g/chrome/content/shell.js
@@ -1153,5 @@
>            // unexpected actors
>            globalActorFactories: restrictPrivileges ? {
>              webappsActor: DebuggerServer.globalActorFactories.webappsActor,
>              deviceActor: DebuggerServer.globalActorFactories.deviceActor,
> -            preferenceActor: DebuggerServer.globalActorFactories.preferenceActor,

I know it is kind of duplicate effort to also not register the actor from main.js,
but I would be at ease if we also explicitely prevent it from being registered over here:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/main.js#365

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Alexandre Poirot (:ochameau) from comment #4)
> Comment on attachment 8396031 [details] [diff] [review]
> Only register pref actor on b2g if certified is allowed (v2)
> 
> Review of attachment 8396031 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/chrome/content/shell.js
> @@ -1153,5 @@
> >            // unexpected actors
> >            globalActorFactories: restrictPrivileges ? {
> >              webappsActor: DebuggerServer.globalActorFactories.webappsActor,
> >              deviceActor: DebuggerServer.globalActorFactories.deviceActor,
> > -            preferenceActor: DebuggerServer.globalActorFactories.preferenceActor,
> 
> I know it is kind of duplicate effort to also not register the actor from
> main.js,
> but I would be at ease if we also explicitely prevent it from being
> registered over here:
>  
> http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/main.
> js#365

Well, there has been talk of using this in the browser too (see bug 986855), so we'd need to register it somewhere, right?

Comment 6

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] from comment #5)
> Well, there has been talk of using this in the browser too (see bug 986855),
> so we'd need to register it somewhere, right?

Sure thing, I'm just suggesting to respect the restrictPrivileges variable:

-  this.registerModule("devtools/server/actors/preference");
+  if (!restrictPrivileges) {
+    this.registerModule("devtools/server/actors/preference");
+  }

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Alexandre Poirot (:ochameau) from comment #6)
> (In reply to J. Ryan Stinnett [:jryans] from comment #5)
> > Well, there has been talk of using this in the browser too (see bug 986855),
> > so we'd need to register it somewhere, right?
> 
> Sure thing, I'm just suggesting to respect the restrictPrivileges variable:
> 
> -  this.registerModule("devtools/server/actors/preference");
> +  if (!restrictPrivileges) {
> +    this.registerModule("devtools/server/actors/preference");
> +  }

Oh!  Yeah, that makes total sense! :)

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Only register pref actor on b2g if certified is allowed (v3) (ochameau: r+)

Removed from main.js as well.

Try: https://tbpl.mozilla.org/?tree=Try&rev=d543a37106a7

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/14b1f8d4496d

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/14b1f8d4496d

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8396687 [details] [diff] [review]
Only register pref actor on b2g if certified is allowed (v3) (ochameau: r+)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 943251
User impact if declined: No direct user impact, but we won't have a consistent interface for accessing prefs if this is left out of B2G 1.4 / Gecko 30.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/08574bbc72a6