Closed Bug 1123846 Opened 5 years ago Closed 4 years ago

Allow apps to create addons for themselves without any permissions

Categories

(Firefox OS Graveyard :: General, defect, P1)

defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Tracking Status
firefox40 --- fixed

People

(Reporter: drs, Assigned: fabrice)

References

Details

(Whiteboard: [spark])

Attachments

(1 file, 5 obsolete files)

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.
Attached patch import-for-all.patch (obsolete) — Splinter Review
Doug, can you test with this patch?
Attachment #8552079 - Flags: feedback?(drs.bugzilla)
Priority: -- → P1
Whiteboard: [lightsaber]
Blocks: 1144989
Blocks: cypress
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-
No longer blocks: spark
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)
See Also: → 1146657
Attached patch import-for-all.patch (obsolete) — Splinter Review
Rebased.
Attachment #8552079 - Attachment is obsolete: true
Whiteboard: [lightsaber] → [ignite]
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)
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)
Attached patch import-for-all.patch (obsolete) — Splinter Review
Rebased.
Attachment #8584011 - Attachment is obsolete: true
Attached patch bug1123846.patch (obsolete) — Splinter Review
Rebased.
Attachment #8594920 - Attachment is obsolete: true
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)
(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)
Attached patch system-app-activities.patch (obsolete) — Splinter Review
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
Whiteboard: [ignite] → [spark]
Now with tests. Fernando, ping me on irc if you need any explanation!
Attachment #8597512 - Attachment is obsolete: true
Attachment #8599610 - Flags: review?(ferjmoreno)
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/4fbdf2a177ab
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Depends on: 1161537
Blocks: spark-addons
You need to log in before you can comment on or make changes to this bug.