Closed
Bug 1123846
Opened 9 years ago
Closed 9 years ago
Allow apps to create addons for themselves without any permissions
Categories
(Firefox OS Graveyard :: General, defect, P1)
Firefox OS Graveyard
General
Tracking
(firefox40 fixed)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: drs, Assigned: fabrice)
References
Details
(Whiteboard: [spark])
Attachments
(1 file, 5 obsolete files)
14.28 KB,
patch
|
ferjm
:
review+
|
Details | Diff | Splinter Review |
On-device dev tools are going to be injected into apps as an addon. Since the dev tools then become one with the app that they're injected into, they inherit their permissions. Customizations to the app then need to be saved and subsequently reloaded each time the app is opened. For this saving to happen, the app must have app management permissions. Alternatively, we could allow apps to save addons to themselves without any special permissions at all. That's what we're going to do in this bug.
Assignee | ||
Comment 1•9 years ago
|
||
Doug, can you test with this patch?
Attachment #8552079 -
Flags: feedback?(drs.bugzilla)
Reporter | ||
Updated•9 years ago
|
Priority: -- → P1
Whiteboard: [lightsaber]
Reporter | ||
Comment 2•9 years ago
|
||
Comment on attachment 8552079 [details] [diff] [review] import-for-all.patch Fabrice, with this patch, add-on creation is silently failing. Without it, I'm getting `mgmt is null` errors. Is there any debugging you'd like me to do?
Flags: needinfo?(fabrice)
Attachment #8552079 -
Flags: feedback?(drs) → feedback-
Reporter | ||
Comment 3•9 years ago
|
||
This patch actually works, and it seems that there's some other problem in the Customizer. I created a reduced test case here: https://github.com/DouglasSherk/bug-1123846 However, as a result of this test case, I found out that we need to unlock DeviceStorage permissions for every app. I'm going to file a bug for this.
Flags: needinfo?(fabrice)
Updated•9 years ago
|
Whiteboard: [lightsaber] → [ignite]
Assignee | ||
Comment 5•9 years ago
|
||
Hm, I'm not sure how to convert this one to land on m-c. import() is on the mgmt object so one option is to open mgmt to everyone in developer mode. An other option would be to move import() to be available on the moazApps, gated on having either the webapps-manage permission or being in dev mode. Paul, any opinion or other idea?
Flags: needinfo?(ptheriault)
Assignee | ||
Comment 6•9 years ago
|
||
I have another idea, which I like better: instead of having the customizer do the import(), why not have it trigger an activity, sending the blob as a payload and implement this activity in the system app?
(In reply to Fabrice Desré [:fabrice] from comment #6) > I have another idea, which I like better: instead of having the customizer > do the import(), why not have it trigger an activity, sending the blob as a > payload and implement this activity in the system app? I mostly have no clue what you are talking about in this bug (I have a lot of catching up to do on lightsaber/developer mode). But in comment 6 I assume you have considered the risk of a 3rd party app implementing the same activity handler or initiating this activity. (ie use a protected activity name, I think we do this elsewhere already).
Flags: needinfo?(ptheriault)
Comment 10•9 years ago
|
||
This patch no longer works. See the revised minimal test case here: https://github.com/justindarc/bug-1123846 When you try tapping the "Install Add-on" link this test case injects in an app that does not have the 'webapps-manage' permission (such as FM Radio), you get the following error: W/FM Radio( 6939): [JavaScript Error: "TypeError: navigator.mozApps.mgmt.import is not a function" {file: "app://40404a18-524d-a44b-a9bb-0a1f5076c6c3/js/script.js" line: 61}] NOTE: It seems as though `navigator.mozApps.mgmt` is properly defined as a result of applying this patch, however, `navigator.mozApps.mgmt.import` is `undefined`.
Flags: needinfo?(fabrice)
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Paul Theriault [:pauljt] from comment #7) > (In reply to Fabrice Desré [:fabrice] from comment #6) > > I have another idea, which I like better: instead of having the customizer > > do the import(), why not have it trigger an activity, sending the blob as a > > payload and implement this activity in the system app? > > I mostly have no clue what you are talking about in this bug (I have a lot > of catching up to do on lightsaber/developer mode). But in comment 6 I > assume you have considered the risk of a 3rd party app implementing the same > activity handler or initiating this activity. (ie use a protected activity > name, I think we do this elsewhere already). Yes, I should have been more precise. What I envision is: - the system app implements and "import-app" activity. - when an app calls this activity, gecko will check that: 1) dev mode is on, returning immediately an error if this is not the case. 2) that only the system app can provide this activity. Justin, I'll do the gecko part in this bug, can you file another one for the gaia implementation in the system app? This way you won't even depend on the gecko patch since it's a "belt and suspenders" one.
Flags: needinfo?(fabrice)
Assignee | ||
Comment 12•9 years ago
|
||
Justin, with this patch you should only be able to use the import-app activity in dev mode and when it's provided by the system app.
Attachment #8594928 -
Attachment is obsolete: true
Updated•9 years ago
|
Whiteboard: [ignite] → [spark]
Assignee | ||
Comment 13•9 years ago
|
||
Now with tests. Fernando, ping me on irc if you need any explanation!
Attachment #8597512 -
Attachment is obsolete: true
Attachment #8599610 -
Flags: review?(ferjmoreno)
Assignee | ||
Comment 14•9 years ago
|
||
Try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=acc8fa2f0d55
Comment 15•9 years ago
|
||
Comment on attachment 8599610 [details] [diff] [review] system-app-activities.patch Review of attachment 8599610 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/activities/ActivitiesService.jsm @@ +367,5 @@ > + Services.prefs.getCharPref("dom.activities.developer_mode_only") > + .split(",").indexOf(aMsg.options.name) !== -1; > + } catch(e) {} > + > + if (isDevModeActivity && !isSystemApp) { I think it would perform a bit better if we get the system app manifest url preference and we do this check when registering the activity instead of every time we start an activity. We still need to check if we are on dev mode and the dev mode only activities though. ::: dom/activities/ActivityProxy.js @@ +64,2 @@ > // Only let certified app to initiate this activitiy. > if (aOptions.name === 'internal-system-engineering-mode' && It would be great to see a preference ("dom.activities.certified_only") and something easier to extend for this like what you are adding in this patch :| @@ +68,5 @@ > Services.obs.notifyObservers(null, "Activity:Error", null); > return; > } > > + // Check the activities that are restricted to be used in dev mode. Shouldn't these checks live on the parent process?
Attachment #8599610 -
Flags: review?(ferjmoreno) → review+
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #15) > ::: dom/activities/ActivitiesService.jsm > @@ +367,5 @@ > > + Services.prefs.getCharPref("dom.activities.developer_mode_only") > > + .split(",").indexOf(aMsg.options.name) !== -1; > > + } catch(e) {} > > + > > + if (isDevModeActivity && !isSystemApp) { > > I think it would perform a bit better if we get the system app manifest url > preference and we do this check when registering the activity instead of > every time we start an activity. We still need to check if we are on dev > mode and the dev mode only activities though. I didn't do this change because we would need to register and unregister when toggling the devmode from webIDE and that's more pain that what we need. I also don't think that we are paying a performance price here compared to eg. the indexedDB request that we will also do. > ::: dom/activities/ActivityProxy.js > @@ +64,2 @@ > > // Only let certified app to initiate this activitiy. > > if (aOptions.name === 'internal-system-engineering-mode' && > > It would be great to see a preference ("dom.activities.certified_only") and > something easier to extend for this like what you are adding in this patch :| Thank you for volunteering to do a follow up! I'll be happy to review ;) > @@ +68,5 @@ > > Services.obs.notifyObservers(null, "Activity:Error", null); > > return; > > } > > > > + // Check the activities that are restricted to be used in dev mode. > > Shouldn't these checks live on the parent process? We do early checks in the child to save IPC traffic. To compromise that the child would need to be able to change a gecko pref, and if that happens we have bigger problems.
Reporter | ||
Updated•9 years ago
|
Blocks: spark-addons
You need to log in
before you can comment on or make changes to this bug.
Description
•