Last Comment Bug 777224 - Alarm API - .getAll() and .remove() can only interact with alarms scheduled by the same app
: Alarm API - .getAll() and .remove() can only interact with alarms scheduled b...
Status: RESOLVED FIXED
: privacy
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Gene Lian [:gene] (I already quit Mozilla)
:
Mentors:
Depends on:
Blocks: alarm-api
  Show dependency treegraph
 
Reported: 2012-07-24 20:25 PDT by Gene Lian [:gene] (I already quit Mozilla)
Modified: 2012-08-02 19:09 PDT (History)
7 users (show)
airpingu: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
WIP Patch (5.82 KB, patch)
2012-07-25 21:00 PDT, Gene Lian [:gene] (I already quit Mozilla)
no flags Details | Diff | Splinter Review
Patch (8.79 KB, patch)
2012-07-31 23:19 PDT, Gene Lian [:gene] (I already quit Mozilla)
21: review+
Details | Diff | Splinter Review
Patch for Checkin (8.78 KB, patch)
2012-08-02 03:51 PDT, Gene Lian [:gene] (I already quit Mozilla)
airpingu: review+
Details | Diff | Splinter Review

Description Gene Lian [:gene] (I already quit Mozilla) 2012-07-24 20:25:17 PDT
Please see: https://groups.google.com/d/msg/mozilla.dev.webapi/pkx1uz_pnhQ/qzCco0g1414J
Comment 1 Mounir Lamouri (:mounir) 2012-07-25 09:53:33 PDT
That should block because there are security/privacy issues in having any app with read/write access to any other app alarms.
Comment 2 Gene Lian [:gene] (I already quit Mozilla) 2012-07-25 20:54:02 PDT
(In reply to Mounir Lamouri (:mounir) (on VACATION until August 5th) from comment #1)
> That should block because there are security/privacy issues in having any
> app with read/write access to any other app alarms.

Hi Mounir,

Regarding the implementation details, I think we can simply compare if the manifestURL is matched or not to decide if .getAll()/.remove() can return/delete the alarms, since the manifestURL of an app has been saved to identify the app when adding alarms (for system message purpose). That is, an app can only see the alarms it added before.

Does this solution sound reasonable to you? ;) I'll come back with a WIP patch later.
Comment 3 Gene Lian [:gene] (I already quit Mozilla) 2012-07-25 21:00:44 PDT
Created attachment 646006 [details] [diff] [review]
WIP Patch

Hi Mounir and Vivien,

This is quick WIP patch. Although it's not yet cleaned up and tested, should be good to go. ;) Need your feedback to make sure I'm on the right way to fix this. Please also see comment #2 for the proposed solution.

Thanks,
Gene
Comment 4 Mounir Lamouri (:mounir) 2012-07-25 21:58:54 PDT
Comment on attachment 646006 [details] [diff] [review]
WIP Patch

I'm on vacation, so you should find someone else to do this review on the DOM side. :sicking or :jlebar maybe?
Comment 5 Gene Lian [:gene] (I already quit Mozilla) 2012-07-25 22:16:52 PDT
(In reply to Mounir Lamouri (:mounir) (on VACATION until August 5th) from comment #4)
> Comment on attachment 646006 [details] [diff] [review]
> WIP Patch
> 
> I'm on vacation, so you should find someone else to do this review on the
> DOM side. :sicking or :jlebar maybe?

Ah! Sorry for bothering. Bon vacances! :D
Comment 6 Dietrich Ayala (:dietrich) 2012-07-31 13:55:42 PDT
Blocking on privacy concern (maybe minor) as well as shipping an api that behaves per the spec.
Comment 7 Gene Lian [:gene] (I already quit Mozilla) 2012-07-31 23:19:47 PDT
Created attachment 647853 [details] [diff] [review]
Patch

Hi Vivien again :),

This patch covers the following changes:

1. In AlarmDB.jsm, I modify .remove and .getAll() by adding a new parameter aManifestURL, which is used to check if the requesting app can remove or get the alarms.

2. In AlarmService.jsm, we will pass the json.manifestURL from the content process to _db.getAll() and _db.remove(). Note that we don't need to do so in the _onAlarmFired() and _restoreAlarmsFromDb() because it's not requested from a specific content processes.

3. In AlarmsManager.js, for .remove() and .getAll(), we pass the app's manifestURL into the message to be sent to AlarmService.

Please feel free to let me know if you don't agree with any of these changes. Btw, do we need to ask for another DOM peer's review if we only have internal logic changes to fix this issue?

Thanks,
Gene
Comment 8 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-08-01 18:59:15 PDT
(In reply to Gene Lian [:gene] from comment #7)
> Created attachment 647853 [details] [diff] [review]
> Patch
> 
> Hi Vivien again :),
> 

Hey :)
 
> Please feel free to let me know if you don't agree with any of these
> changes. Btw, do we need to ask for another DOM peer's review if we only
> have internal logic changes to fix this issue?

I would say that it should be fine.
Comment 9 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-08-01 19:03:31 PDT
Comment on attachment 647853 [details] [diff] [review]
Patch

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

::: dom/alarm/AlarmService.jsm
@@ +161,5 @@
>                return;
>              }
>  
>              // check if the alarm to be removed is in the queue
> +            // by ID and whether it belongs to the requsting app

requsting -> requesting
Comment 10 Gene Lian [:gene] (I already quit Mozilla) 2012-08-02 03:51:40 PDT
Created attachment 648298 [details] [diff] [review]
Patch for Checkin

Vivien has reviewed+ at comment #9.
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-08-02 19:09:37 PDT
https://hg.mozilla.org/mozilla-central/rev/5e80ccb26f3d

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