781353 - Hook up "power" to Permission Manager

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gregor Wagner [:gwagner]]

We have to remove dom.power.whitelist.
Kanru can you take this?

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Hook up "power" to Permission Manager

Also fixed a bug in specialpowers.js

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

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

Comment 3

[Justin Lebar (not reading bugmail)]

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+9)]

(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+9)]

Attachment: Fix SpecialPowers.addPermission for URL.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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+9)]

Attachment: 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)]

> 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)]

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+9)]

Attachment: Hook up "power" to Permission Manager. v1.2

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

Comment 11

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 650757 [details] [diff] [review]
Fix SpecialPowers.addPermission for URL.

Typo fixed in bug 781929

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f111e43bc076

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f111e43bc076