Last Comment Bug 781353 - Hook up "power" to Permission Manager
: Hook up "power" to Permission Manager
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Kan-Ru Chen [:kanru] (UTC+8)
:
Mentors:
Depends on:
Blocks: 774716 707626 737341 764618
  Show dependency treegraph
 
Reported: 2012-08-08 14:50 PDT by Gregor Wagner [:gwagner]
Modified: 2012-08-13 11:11 PDT (History)
5 users (show)
kchen: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Hook up "power" to Permission Manager (13.88 KB, patch)
2012-08-09 01:54 PDT, Kan-Ru Chen [:kanru] (UTC+8)
justin.lebar+bug: review-
Details | Diff | Review
Fix SpecialPowers.addPermission for URL. (1.10 KB, patch)
2012-08-09 19:08 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Hook up "power" to Permission Manager. v1.1 (12.84 KB, patch)
2012-08-10 02:39 PDT, Kan-Ru Chen [:kanru] (UTC+8)
justin.lebar+bug: review+
Details | Diff | Review
Hook up "power" to Permission Manager. v1.2 (13.30 KB, patch)
2012-08-12 18:54 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review

Description Gregor Wagner [:gwagner] 2012-08-08 14:50:25 PDT
We have to remove dom.power.whitelist.
Kanru can you take this?
Comment 1 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-09 01:54:47 PDT
Created attachment 650469 [details] [diff] [review]
Hook up "power" to Permission Manager

Also fixed a bug in specialpowers.js
Comment 2 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-09 03:30:17 PDT
https://tbpl.mozilla.org/?tree=Try&rev=f7ebadadc2f0
Comment 3 Justin Lebar (not reading bugmail) 2012-08-09 16:42:05 PDT
Comment on attachment 650469 [details] [diff] [review]
Hook up "power" to Permission Manager

> NS_IMETHODIMP
> Navigator::GetMozPower(nsIDOMMozPowerManager** aPower)
> {
>   *aPower = nullptr;
> 
>   if (!mPowerManager) {
>-    nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow);
>-    NS_ENSURE_TRUE(win, NS_OK);
>+    nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
>+    NS_ENSURE_TRUE(window, NS_OK);
> 
>-    mPowerManager = new power::PowerManager();
>-    mPowerManager->Init(win);
>+    nsresult rv = NS_NewPowerManager(window, getter_AddRefs(mPowerManager));
>+    NS_ENSURE_SUCCESS(rv, rv);
>   }

We should be mindful of the error codes we return to the DOM.  We shouldn't
return NS_NOINTERFACE, for example.

I'd be OK if any failure from NS_NewPowerManager was translated into
NS_ERROR_DOM_SECURITY_ERR here.

But there's a bigger question which is: Before this patch, navigator.mozPower
was accessible to all content, but only some content could call methods on it.

After this patch, anybody accessing mozPower without permission will throw an
exception.

We should be consistent here with the other privileged APIs we have.  mozSms
and mozMobileConnetion return null on my desktop, but that may be because I'm
not on a phone?  Can you figure out what's the right thing to do here?

>diff --git a/dom/power/PowerManager.cpp b/dom/power/PowerManager.cpp
>--- a/dom/power/PowerManager.cpp
>+++ b/dom/power/PowerManager.cpp

>+namespace mozilla {
>+namespace dom {
>+
>+nsresult
>+NS_NewPowerManager(nsPIDOMWindow* aWindow,
>+                   power::PowerManager** aPowerManager)

Nit: This idiom is usually used for trivial constructors -- it's often what
backs CreateInstance.  I think in this case it would be more idiomatic to have
a static factory method on PowerManager.

>+{
>+  NS_ASSERTION(aWindow, "Null pointer!");
>+
>+  nsPIDOMWindow* innerWindow = aWindow->IsInnerWindow() ?
>+    aWindow :
>+    aWindow->GetCurrentInnerWindow();
>+
>+  // Need the document for security check.
>+  nsCOMPtr<nsIDocument> document = innerWindow->GetExtantDoc();
>+  NS_ENSURE_TRUE(document, NS_NOINTERFACE);
>+
>+  nsCOMPtr<nsIPrincipal> principal = document->NodePrincipal();
>+  NS_ENSURE_TRUE(principal, NS_ERROR_UNEXPECTED);

NodePrincipal() never returns false (see nsINode), so this isn't necessary.

We have a rough convention that methods which can return null are named Get*,
whereas methods which never return null don't have Get.

>+  nsCOMPtr<nsIPermissionManager> permMgr =
>+    do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);
>+  NS_ENSURE_TRUE(permMgr, NS_ERROR_UNEXPECTED);
>+
>+  PRUint32 permission;
>+  nsresult rv =
>+    permMgr->TestPermissionFromPrincipal(principal, "power", &permission);
>+  NS_ENSURE_SUCCESS(rv, rv);

If you prefer, you could instead do

    PRUint32 permission = nsIPermissionManager::DENY_ACTION;
    permMgr->TestPermissionFromPrincipal(...);
    // no NS_ENSURE_SUCCESS.

If the TestPermissionFromPrincipal call fails, it would certainly be wrong for
it to stick ALLOW_ACTION in the permission out param.

>+  if (permission != nsIPermissionManager::ALLOW_ACTION) {
>+    *aPowerManager = nullptr;
>+    return NS_OK;
>+  }

If we do this, then the DOM will return NULL for navigator.powerManager,
instead of throwing an exception (like the other cases).

We should be consistent, and either always return NULL, or always throw.  Or
maintain our current behavior, of allowing access to the object but throwing
when you touch it.

>diff --git a/testing/mochitest/tests/SimpleTest/specialpowersAPI.js b/testing/mochitest/tests/SimpleTest/specialpowersAPI.js
>--- a/testing/mochitest/tests/SimpleTest/specialpowersAPI.js
>+++ b/testing/mochitest/tests/SimpleTest/specialpowersAPI.js
>@@ -1111,17 +1111,17 @@ SpecialPowersAPI.prototype = {
>     var pm = Cc["@mozilla.org/permissionmanager;1"].getService(Ci.nsIPermissionManager);
>     pm.removeFromPrincipal(document.nodePrincipal, "fullscreen");
>   },
> 
>   _getURI: function(urlOrDocument) {
>     if (typeof(urlOrDocument) == "string") {
>       return Cc["@mozilla.org/network/io-service;1"].
>                getService(Ci.nsIIOService).
>-               newURI(url, null, null);
>+               newURI(urlOrDocument, null, null);
>     }
>     // Assume document.
>     return this.getDocumentURIObject(urlOrDocument);
>   },

This seems obviously correct, but I'd feel more comfortable getting a review from ted.  It's probably a good idea to split it out into a separate patch, so the commit history will reflect that this is a bugfix independent of your power manager changes.  (If you're using hg, hg qref can take a filename or paths; that makes splitting patches easy.  It's also easy with git add --interactive.)

r- because we need to figure out what we're doing when the window doesn't have permission.

r?ted on the specialpowers change.
Comment 4 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-09 18:54:19 PDT
(In reply to Justin Lebar [:jlebar] from comment #3)
> Comment on attachment 650469 [details] [diff] [review]
> Hook up "power" to Permission Manager
> 
> > NS_IMETHODIMP
> > Navigator::GetMozPower(nsIDOMMozPowerManager** aPower)
> > {
> >   *aPower = nullptr;
> > 
> >   if (!mPowerManager) {
> >-    nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow);
> >-    NS_ENSURE_TRUE(win, NS_OK);
> >+    nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
> >+    NS_ENSURE_TRUE(window, NS_OK);
> > 
> >-    mPowerManager = new power::PowerManager();
> >-    mPowerManager->Init(win);
> >+    nsresult rv = NS_NewPowerManager(window, getter_AddRefs(mPowerManager));
> >+    NS_ENSURE_SUCCESS(rv, rv);
> >   }
> 
> We should be mindful of the error codes we return to the DOM.  We shouldn't
> return NS_NOINTERFACE, for example.
> 
> I'd be OK if any failure from NS_NewPowerManager was translated into
> NS_ERROR_DOM_SECURITY_ERR here.
> 
> But there's a bigger question which is: Before this patch, navigator.mozPower
> was accessible to all content, but only some content could call methods on
> it.
> 
> After this patch, anybody accessing mozPower without permission will throw an
> exception.
> 
> We should be consistent here with the other privileged APIs we have.  mozSms
> and mozMobileConnetion return null on my desktop, but that may be because I'm
> not on a phone?  Can you figure out what's the right thing to do here?

From my previous discussion with :timdream it is desirable to return null for inaccessible API. I opened bug 737341 for this but didn't make any progress because whether to return null or not was still not clear. It seems mozTelephony and mozBluetooth will return null, mozSettings and mozContacts will return NS_ERROR_NOT_IMPLEMENTED when inaccessible.
Comment 5 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-09 19:08:09 PDT
Created attachment 650757 [details] [diff] [review]
Fix SpecialPowers.addPermission for URL.
Comment 6 Tim Guan-tin Chien [:timdream] (please needinfo; on leave in July) 2012-08-09 20:48:08 PDT
(In reply to Kan-Ru Chen [:kanru] from comment #4)
> (In reply to Justin Lebar [:jlebar] from comment #3)
> > 
> > We should be consistent here with the other privileged APIs we have.  mozSms
> > and mozMobileConnetion return null on my desktop, but that may be because I'm
> > not on a phone?  Can you figure out what's the right thing to do here?
> 
> From my previous discussion with :timdream it is desirable to return null
> for inaccessible API. I opened bug 737341 for this but didn't make any
> progress because whether to return null or not was still not clear. It seems
> mozTelephony and mozBluetooth will return null, mozSettings and mozContacts
> will return NS_ERROR_NOT_IMPLEMENTED when inaccessible.

Yes, return null is the right things to do here, at least for consistency.
Comment 7 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-10 02:39:20 PDT
Created attachment 650836 [details] [diff] [review]
Hook up "power" to Permission Manager. v1.1

Nits addressed. This version still returns null when the object is inaccessible.
Comment 8 Justin Lebar (not reading bugmail) 2012-08-10 08:06:11 PDT
> mozSettings and mozContacts will return NS_ERROR_NOT_IMPLEMENTED when inaccessible.

Can we please file bugs to get these to return null?
Comment 9 Justin Lebar (not reading bugmail) 2012-08-10 09:26:53 PDT
Comment on attachment 650836 [details] [diff] [review]
Hook up "power" to Permission Manager. v1.1

Thanks for figuring out that we should return null here.

r=me with nits addressed.

>diff --git a/dom/base/Navigator.cpp b/dom/base/Navigator.cpp
>--- a/dom/base/Navigator.cpp
>+++ b/dom/base/Navigator.cpp
>@@ -1024,21 +1024,21 @@ Navigator::GetBattery(nsIDOMBatteryManag
> }
> 
> NS_IMETHODIMP
> Navigator::GetMozPower(nsIDOMMozPowerManager** aPower)
> {
>   *aPower = nullptr;
> 
>   if (!mPowerManager) {
>-    nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow);
>-    NS_ENSURE_TRUE(win, NS_OK);
>+    nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
>+    NS_ENSURE_TRUE(window, NS_OK);
> 
>-    mPowerManager = new power::PowerManager();
>-    mPowerManager->Init(win);
>+    mPowerManager = power::PowerManager::CreateInstanceAndCheckPermission(window);
>+    NS_ENSURE_TRUE(mPowerManager, NS_OK);

Nit: CheckPermissionAndCreateInstance.

Nit: We usually prefer a |using| statement to explicit namespace usage, in cpp
files (as opposed to in header files, where you usually can't use |using|).

>+already_AddRefed<PowerManager>
>+PowerManager::CreateInstanceAndCheckPermission(nsPIDOMWindow* aWindow)
>+{
>+  NS_ASSERTION(aWindow, "Null pointer!");

This assertion doesn't add very much, because we're going to crash immediately
after, regardless.  :)

>+  nsPIDOMWindow* innerWindow = aWindow->IsInnerWindow() ?
>+    aWindow :
>+    aWindow->GetCurrentInnerWindow();
>+
>+  // Need the document for security check.
>+  nsCOMPtr<nsIDocument> document = innerWindow->GetExtantDoc();
>+  NS_ENSURE_TRUE(document, nullptr);
>+
>+  nsCOMPtr<nsIPrincipal> principal = document->NodePrincipal();
>+
>+  nsCOMPtr<nsIPermissionManager> permMgr =
>+    do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);
>+  NS_ENSURE_TRUE(permMgr, nullptr);
>+
>+  PRUint32 permission = nsIPermissionManager::DENY_ACTION;
>+  nsresult rv =
>+    permMgr->TestPermissionFromPrincipal(principal, "power", &permission);

Don't need |nsresult rv|.

I've used up my quota of burning the tree for the year, so can we push this to
try before we push it to m-i?  :)
Comment 10 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-12 18:54:24 PDT
Created attachment 651247 [details] [diff] [review]
Hook up "power" to Permission Manager. v1.2

https://tbpl.mozilla.org/?tree=Try&rev=8f685fcf1ef5
Comment 11 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-12 19:27:24 PDT
Comment on attachment 650757 [details] [diff] [review]
Fix SpecialPowers.addPermission for URL.

Typo fixed in bug 781929
Comment 12 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-12 19:35:57 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f111e43bc076
Comment 13 Ed Morley [:emorley] 2012-08-13 11:11:49 PDT
https://hg.mozilla.org/mozilla-central/rev/f111e43bc076

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