783500 - Hook up IdleAPI to permissions manager

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[David Chan [:dchan]]

The idle API currently checks whether an app is certified before attaching idle observers. bug# 780507

This is inconsistent with other apps permissions checks which go through nsIPermissionManager. The concern is that a certified app which does not enumerate the idle permission in a manifest would have access to the idle api.

Comment 1

[Andrew Overholt [:overholt]]

Gregor, can you take this?

Comment 2

[Mounir Lamouri (:mounir)]

I think this has been made on purpose for v1. Justin, do you confirm?

Comment 3

[Gregor Wagner [:gwagner]]

(In reply to Mounir Lamouri (:mounir) from comment #2)
> I think this has been made on purpose for v1.

Right, we don't even have an explicit permission name for the idle-api.

Comment 4

[Justin Lebar (not reading bugmail)]

Which I'm happy to change once we get default permissions landed!

Comment 5

[Mounir Lamouri (:mounir)]

Re-asking for blocking status per the previous comments, assuming it should not block.

Comment 6

[Andrew Overholt [:overholt]]

For post-basecamp we're going to want to make the Idle API available to things *other* than just certified apps and when we do that we should make it go through the permission manager.

Comment 7

[Paul Theriault [:pauljt] (no longer reading bugmail)]

I'm trying to finalize the documentation for Basecamp permissions. AFAIK there is no permission string for this API so I have proposed "idle" as the permission string in the documentation. Let me know if this is NOT ok, and I'll update it.

Comment 8

[Justin Lebar (not reading bugmail)]

That name is fine with me, but please indicate in the docs that that permission does not exist yet.  :)

Comment 9

[Justin Lebar (not reading bugmail)]

Steven, this might be up your alley.

Comment 10

[StevenLee[:slee]]

(In reply to Justin Lebar [:jlebar] from comment #9)
> Steven, this might be up your alley.
Yes, I am glad to take this bug.

Hi Gregor,
Can I take this?
Thanks.

Comment 11

[Gregor Wagner [:gwagner]]

(In reply to StevenLee from comment #10)
> (In reply to Justin Lebar [:jlebar] from comment #9)
> > Steven, this might be up your alley.
> Yes, I am glad to take this bug.
> 
> Hi Gregor,
> Can I take this?
> Thanks.

Thanks!

Comment 12

[StevenLee[:slee]]

Attachment: patch

Comment 13

[Justin Lebar (not reading bugmail)]

Comment on attachment 664393 [details] [diff] [review]
patch

> +  nsresult CheckPermission(const char* type);

Please make this return a bool.  You can do NS_ENSURE_TRUE(foo, false) instead of NS_ENSURE_TRUE(foo, NS_ERROR_FAILURE).

Also, nit, the parameter should be called |aPermission|, please.

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

Then this becomes |return permission == nsIPermissionManager::ALLOW_ACTION|.

Also, please file a Gaia bug to ensure that the correct app (I think the system app) gets the correct permission.  That should be a simple change on their end, but we should hold off landing this until that's fixed, so we don't break them.

r=me with those changes.

Comment 14

[StevenLee[:slee]]

Gaia side modifications, https://github.com/mozilla-b2g/gaia/pull/5280.

Comment 15

[StevenLee[:slee]]

Attachment: patch v2

Comment 16

[StevenLee[:slee]]

(In reply to StevenLee from comment #14)
> Gaia side modifications, https://github.com/mozilla-b2g/gaia/pull/5280.

Hi Justin,

Gaia part has been updated and here is the results on try server https://tbpl.mozilla.org/?tree=Try&rev=d6a5b788f9ab.

Comment 17

[Mounir Lamouri (:mounir)]

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

Steven, could you write a test for this please?

Comment 18

[Ed Morley [:emorley]]

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

Comment 19

[StevenLee[:slee]]

Attachment: test case

Hi Mounir,

Sorry for updating the test case so late. Please check the test case.
Thanks.

Comment 20

[Mounir Lamouri (:mounir)]

Comment on attachment 667025 [details] [diff] [review]
test case

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

Don't be influenced by the number of comments: this test is good. However, we (at Mozilla) tend to have bad tests practices so I did a lot of comments to points how you could have improved things. A lot of them are not very important taken alone but the whole makes a difference.

r=me with those comments applied.

::: dom/tests/mochitest/general/Makefile.in
@@ +45,5 @@
>  		file_clonewrapper.html \
>  		file_moving_nodeList.html \
>  		test_performance_now.html \
>  		test_interfaces.html \
> +		test_bug783500.html \

Please, name your test like:
test_idleapi_permissions.html \

::: dom/tests/mochitest/general/test_bug783500.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test for Bug 785300</title>

Put a real title here.

@@ +4,5 @@
> +  <title>Test for Bug 785300</title>
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +</head>
> +<body onload="run_test();">

Don't use that but do
addLoadEvent(runTest);
instad.

@@ +11,5 @@
> +</div>
> +<pre id="test">
> +<script type="application/javascript">
> +
> +/** Test permission **/

There too. "Test permission" means nothing.

@@ +19,5 @@
> +  onactive: null
> +};
> +
> +function run_test() {
> +  this.idleObserver.time = 100;

Why are you doing that? Could you comment and explain?

@@ +20,5 @@
> +};
> +
> +function run_test() {
> +  this.idleObserver.time = 100;
> +  SpecialPowers.removePermission("idle", document);

It's more than unlikely this page has any idle permission. Don't remove it. You could even check that the permission isn't there (but really don't have to).

@@ +23,5 @@
> +  this.idleObserver.time = 100;
> +  SpecialPowers.removePermission("idle", document);
> +
> +  var added = false;
> +  try {    

nit: trailing whitespaces

@@ +27,5 @@
> +  try {    
> +    navigator.addIdleObserver(this.idleObserver);
> +    added = true;
> +  } catch (e) { }
> +  

nit: trailing whitespaces

@@ +35,5 @@
> +  try {
> +    navigator.addIdleObserver(this.idleObserver);
> +  } catch (e){
> +    ok(false, "Shouldn be able to add idle observer with permission.");
> +  }

nit: typo s/Shouldn/Should/

Also, use the same pattern than above:
added = false;
try {
  navigator.addIdleObserver(this.idleObserver);
  added = true;
} catch(e) { }
ok(added, "Should be able to add idle observer with permission.");

Comment 21

[StevenLee[:slee]]

Hi Mounir,

(In reply to Mounir Lamouri (:mounir) from comment #20)
> @@ +19,5 @@
> > +  onactive: null
> > +};
> > +
> > +function run_test() {
> > +  this.idleObserver.time = 100;
> 
> Why are you doing that? Could you comment and explain?
> 
addIdleObserver checks whether the time value is valid or not. I just pass a value that can pass the examination. 

Thanks for your advises. I learn a lot from them.

Comment 22

[StevenLee[:slee]]

Attachment: test case v2

Comment 23

[Mounir Lamouri (:mounir)]

Steven, have you sent this test to try? Do you need me to push it to m-i?

Comment 24

[StevenLee[:slee]]

(In reply to Mounir Lamouri (:mounir) from comment #23)
> Steven, have you sent this test to try? Do you need me to push it to m-i?

Hi Mounir,
Sorry for that I don't know I should send test cases to try. I have sent the test to try and will let you know the result when it finishes. 
Thanks.

Comment 25

[StevenLee[:slee]]

Mounir, here is the try server log, https://tbpl.mozilla.org/?tree=Try&rev=b8ea11134ae2, please check it. The test case is ran and passed. Please help me push to m-i.
Thanks.

Comment 26

[Mounir Lamouri (:mounir)]

Comment on attachment 668071 [details] [diff] [review]
test case v2

http://hg.mozilla.org/integration/mozilla-inbound/rev/55585f89f069

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55585f89f069