Closed Bug 783500 Opened 7 years ago Closed 7 years ago

Hook up IdleAPI to permissions manager

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-kilimanjaro +
blocking-basecamp -

People

(Reporter: dchanm+bugzilla, Assigned: slee)

References

Details

(Whiteboard: [mentor=jlebar][lang=C++][LOE:S])

Attachments

(2 files, 2 obsolete files)

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.
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Gregor, can you take this?
Assignee: nobody → anygregor
I think this has been made on purpose for v1. Justin, do you confirm?
(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.
Which I'm happy to change once we get default permissions landed!
Re-asking for blocking status per the previous comments, assuming it should not block.
blocking-basecamp: + → ?
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.
blocking-basecamp: ? → -
blocking-kilimanjaro: --- → +
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.
That name is fine with me, but please indicate in the docs that that permission does not exist yet.  :)
Whiteboard: [mentor=jlebar][lang=C++][LOE:S]
Steven, this might be up your alley.
(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.
(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!
Assignee: anygregor → slee
Attached patch patch (obsolete) — Splinter Review
Attachment #664393 - Flags: review?(justin.lebar+bug)
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.
Attachment #664393 - Flags: review?(justin.lebar+bug) → review+
Gaia side modifications, https://github.com/mozilla-b2g/gaia/pull/5280.
Attached patch patch v2Splinter Review
Attachment #664393 - Attachment is obsolete: true
Attachment #665276 - Flags: review+
(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.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/c25274efa6f6

Steven, could you write a test for this please?
Flags: in-testsuite?
Keywords: checkin-needed
Target Milestone: --- → mozilla18
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/c25274efa6f6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Attached patch test case (obsolete) — Splinter Review
Hi Mounir,

Sorry for updating the test case so late. Please check the test case.
Thanks.
Attachment #667025 - Flags: review?(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.");
Attachment #667025 - Flags: review?(mounir) → review+
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.
Attached patch test case v2Splinter Review
Attachment #667025 - Attachment is obsolete: true
Attachment #668071 - Flags: review+
Steven, have you sent this test to try? Do you need me to push it to m-i?
(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.
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.
You need to log in before you can comment on or make changes to this bug.