Closed Bug 778277 Opened 8 years ago Closed 6 years ago

Add support of installing of multiple apps off of the same origin

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: jsmith, Assigned: fabrice)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 9 obsolete files)

2.25 KB, patch
myk
: review+
Details | Diff | Splinter Review
4.05 KB, patch
myk
: review+
Details | Diff | Splinter Review
In bug 775847, the proposal has been put out to add support for installing of multiple apps off of the same origin in the DOM registry for the mozapps API. This bug aims to provide the implementation to support installing of multiple apps off of the same origin in the desktop web runtime. Steps to figure this out include:

- Identifying any implementation areas that assume the one app per origin rule (e.g. the folders in APPDATA on windows)
- Identifying any UI exposure areas that establish a mentality for one app per origin that may need to be changed
- Implementing those said pieces needed to allow for multiple apps per origin
Depends on: 775847
Blocks: 778297
This should remove the only problem we have in desktop webrt for multiple-apps-per-origin.
It's also preparatory work for packaged apps.

_isLaunchable will probably become obsolete with bug 892020.
Attachment #773590 - Flags: feedback?(fabrice)
Comment on attachment 773590 [details] [diff] [review]
Use manifestURL as an unique name for an application

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

::: dom/apps/src/Webapps.jsm
@@ +3059,4 @@
>      if (this.allAppsLaunchable)
>        return true;
>  
> +	let sanManifestURL = aManifestURL.replace(/[^a-z0-9_\-]/gi, "");

That does not guarantee that there will be no collisions for some manifest names. You should rather hash the manifestURL.

::: toolkit/webapps/WebappsInstaller.jsm
@@ +895,5 @@
>  /**
> + * Sanitize a filename (conservative, accepts only a-z, 0-9, - and _)
> + */
> +function sanitizeFilename(aMostProbablyBadFilenameString) {
> +  return aMostProbablyBadFilenameString.replace(/[^a-z0-9_\-]/gi, "");

Same here...
Attachment #773590 - Flags: feedback?(fabrice)
Assignee: nobody → mar.castelluccio
Attachment #773590 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #774475 - Flags: review?(fabrice)
Marco - Can you add a mochitest for this patch?
Attachment #774475 - Attachment is obsolete: true
Attachment #774475 - Flags: review?(fabrice)
Attachment #774914 - Flags: review?(fabrice)
Attachment #774914 - Attachment is obsolete: true
Attachment #774914 - Flags: review?(fabrice)
Comment on attachment 775091 [details] [diff] [review]
Use manifest url hash as unique name for apps v3

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

I would rather have _isLaunchable take an app object, and compute the manifest hash here instead of doing it at each caller site. What do you think?
Attachment #775091 - Flags: review?(fabrice) → review-
(In reply to Fabrice Desré [:fabrice] from comment #7)
> Comment on attachment 775091 [details] [diff] [review]
> Use manifest url hash as unique name for apps v3
> 
> Review of attachment 775091 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would rather have _isLaunchable take an app object, and compute the
> manifest hash here instead of doing it at each caller site. What do you
> think?

Sounds right ;)

I forgot there's also WebappOSUtils.jsm, the new patch also contains changes to that file.
Attachment #775091 - Attachment is obsolete: true
Attachment #775747 - Flags: review?(fabrice)
Using a hash internally is fine, but using it as the name for an installation directory or other local file will make it difficult to identify that directory/file in the filesystem.  This is not only a concern for runtime developers, as users sometimes interact with such directories/files when investigating an issue.

A better option would be to craft a name that includes both the hash and other identifying information, like the host and a subset of the path (leaving out the protocol and port unless they vary from the default).

Either way, it isn't clear why this is necessary to support packaged apps, since we should be able to craft a unique name for a packaged app without supporting multiple apps per origin generally.

Also, while it's fine to prepare the codebase for multiple apps per origin, we should not actually start supporting them without looping in product and engineering leads like Vishy, sicking, and tchoma (cc:ed), given that the restriction is a core part of our current model and has security and other implications.

https://developer.mozilla.org/en-US/docs/Web/Apps/FAQs/About_app_manifests
(In reply to Myk Melez [:myk] [@mykmelez] from comment #9)
> Using a hash internally is fine, but using it as the name for an
> installation directory or other local file will make it difficult to
> identify that directory/file in the filesystem.  This is not only a concern
> for runtime developers, as users sometimes interact with such
> directories/files when investigating an issue.
> 
> A better option would be to craft a name that includes both the hash and
> other identifying information, like the host and a subset of the path
> (leaving out the protocol and port unless they vary from the default).

Yes, we can use any name, as long as it's unique. Using an hash of the manifest name is extremely simple, but any unique name would work. If we think users should be able to identify the application directory, we could use the name we used before concatenated with the manifest url hash. Is it ok?

> Either way, it isn't clear why this is necessary to support packaged apps,
> since we should be able to craft a unique name for a packaged app without
> supporting multiple apps per origin generally.

It's not strictly needed, but I thought it would be better to have the same naming scheme (and so the same installation code) for hosted and packaged apps.

> Also, while it's fine to prepare the codebase for multiple apps per origin,
> we should not actually start supporting them without looping in product and
> engineering leads like Vishy, sicking, and tchoma (cc:ed), given that the
> restriction is a core part of our current model and has security and other
> implications.
> 
> https://developer.mozilla.org/en-US/docs/Web/Apps/FAQs/About_app_manifests

Ok, I'll add a check to disallow that.
Without going into a lot of detail on the bug and causing a 200 comment bug, I'm going to suggest that we start a thread on dev-webapps on the design proposal for multiple apps per origin that's provided in this bug. We need to be mindful that we try to build consensus across groups such as:

* The developers driving the SysApps W3C spec (e.g. Mounir)
* Developer leads across each platform the DOM Apps API is used
** Desktop (e.g. the people on this bug)
** Android (e.g. mfinkle, wesj, jhugman)
** Firefox OS (e.g. fabrice, julienw, fernando)
* Marketplace developers (e.g. basta, clouserw)
* Product Team (e.g. vishy)
* Security Team (e.g. pauljt, amac)
* QA Team (e.g. myself)
* etc.

There's probably others I'm not thinking of in that list, but you get the idea that there's a lot of groups that need to integrate with one another. This is because as Myk states, this represents a foundational piece of the DOM Apps API that many groups integrate with. So we need to be careful on how this implemented such that the implementation aligns across each possible group involved here. We especially need to avoid causing integration failures in one or more major components that a particular group oversees.
The only reason we did not multiple apps per origin from day 1 is an unfortunate decision. The security model fully support them (the app key is the manifest URL, not the origin). There's no doubt we want that to work.
Comment on attachment 775747 [details] [diff] [review]
Use manifest url hash as unique name for apps v4

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

I support Myk's idea to not use just the hash but also add something more human readable.
Attachment #775747 - Flags: review?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #12)
> The only reason we did not multiple apps per origin from day 1 is an
> unfortunate decision. The security model fully support them (the app key is
> the manifest URL, not the origin). There's no doubt we want that to work.

Agreed. I'm just wanting to make sure we're consistent from an integration perspective on the architecture such that the approach works with each stakeholder involved, so that we avoid serious regressions from assumptions we may have made from the one app per origin rule.
So, I'd like to use this naming scheme: AppName + manifest url hash. This would make the directory names even more human readable than before.

The case of apps with the same name isn't common and anyway the directory name would still be unique.
(In reply to Jason Smith [:jsmith] from comment #14)
> (In reply to Fabrice Desré [:fabrice] from comment #12)
> > The only reason we did not multiple apps per origin from day 1 is an
> > unfortunate decision. The security model fully support them (the app key is
> > the manifest URL, not the origin). There's no doubt we want that to work.
> 
> Agreed. I'm just wanting to make sure we're consistent from an integration
> perspective on the architecture such that the approach works with each
> stakeholder involved, so that we avoid serious regressions from assumptions
> we may have made from the one app per origin rule.

Just to be clear, I also agree that we should support multiple apps per origin.  I just want to make sure we do it with everyone's eyes wide open, since we've already written a bunch of code that didn't need to account for it.


(In reply to Marco Castelluccio [:marco] from comment #15)
> So, I'd like to use this naming scheme: AppName + manifest url hash. This
> would make the directory names even more human readable than before.
> 
> The case of apps with the same name isn't common and anyway the directory
> name would still be unique.

I like this idea a lot.  It's the best combination of human-identifiable and unique identifiers.
(In reply to Myk Melez [:myk] [@mykmelez] from comment #16)
> (In reply to Jason Smith [:jsmith] from comment #14)
> > (In reply to Fabrice Desré [:fabrice] from comment #12)
> > > The only reason we did not multiple apps per origin from day 1 is an
> > > unfortunate decision. The security model fully support them (the app key is
> > > the manifest URL, not the origin). There's no doubt we want that to work.
> > 
> > Agreed. I'm just wanting to make sure we're consistent from an integration
> > perspective on the architecture such that the approach works with each
> > stakeholder involved, so that we avoid serious regressions from assumptions
> > we may have made from the one app per origin rule.
> 
> Just to be clear, I also agree that we should support multiple apps per
> origin.  I just want to make sure we do it with everyone's eyes wide open,
> since we've already written a bunch of code that didn't need to account for
> it.

Agreed. It is a welcome simplification for the app developers. 

In the past, we had made a an assertion that multiple apps was a bad idea

Quoting from MDN https://developer.mozilla.org/en-US/docs/Web/Apps/FAQs/About_app_manifests
"Can I have more than one app at my origin?
    No, there can be only one app per origin. If multiple apps were allowed for a single origin, they would live in a single web sandbox -- they could examine each other's localStorage, do Ajax requests to each other's APIs, or even steal access to privileged APIs that should have been granted to only one of the apps. This would be especially dangerous for domains that publish user-generated content from many users.
    We recommend that you use a separate subdomain for each of your apps. For example, spreadsheet.mycoolapps.com for one app and texteditor.mycoolapps.com for another. For more information, see Adding a subdomain for an app.
    Many resources and permissions on the Web are already scoped to a single origin. By defining an app and an origin as the same thing we use the same security restrictions that are used elsewhere on the Web and in HTML5."

If our security model is capable of providing the app sandbox per app (from the same origin) we can reflect that in the FAQ
(In reply to Myk Melez [:myk] [@mykmelez] from comment #16)
> (In reply to Marco Castelluccio [:marco] from comment #15)
> > So, I'd like to use this naming scheme: AppName + manifest url hash. This
> > would make the directory names even more human readable than before.
> > 
> > The case of apps with the same name isn't common and anyway the directory
> > name would still be unique.
> 
> I like this idea a lot.  It's the best combination of human-identifiable and
> unique identifiers.

I've a single concern here, what if the name changes?
There's a note on MDN that says: "Note: If you change the name of your app after distribution the name will not be updated for any existing installations.".
But AFAICS the webapps registry will be updated with the new name. So again we're going to have problems while checking if an app is locally installed.
We call AppsUtils.ensureSameAppName() when doing updates, so we should be safe here.
Attachment #775747 - Attachment is obsolete: true
Attachment #777340 - Flags: review?(myk)
Attachment #777340 - Flags: review?(fabrice)
Comment on attachment 777340 [details] [diff] [review]
Use app name + manifest url hash as unique name for apps

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

r=me with comments addressed

::: dom/apps/src/Webapps.jsm
@@ +1785,5 @@
> +        let appid = this._appId(app.origin);
> +        if (appid !== null && this._isLaunchable(app)) {
> +          sendError("REINSTALL_FORBIDDEN");
> +          return;
> +        } 

nit: trailing whitespace.

::: toolkit/webapps/WebappOSUtils.jsm
@@ +21,5 @@
> +    } else {
> +      name = aApp.manifest.name;
> +    }
> +
> +    return this.stripStringForFilename(name).toLowerCase() + "-" + AppsUtils.computeHash(aApp.manifestURL);

stripStringForFilename() is also defined in WebappsInstaller.jsm but has a slightly different behavior. Can you rename it to something like sanitizeStringForFilename ?

@@ +86,5 @@
> +
> +      if (appPath) {
> +        mwaUtils.launchAppWithIdentifier(aApp.origin);
> +        return true;
> +      } 

nit: trailing whitespace

@@ +97,5 @@
>  
>      let exeFile = Services.dirsvc.get("Home", Ci.nsIFile);
> +    exeFileMan = exeFile.clone();
> +    exeFileMan.append("." + uniqueName);
> +    

nit: trailing whitespace.
(In reply to Fabrice Desré [:fabrice] from comment #21)
> ::: toolkit/webapps/WebappOSUtils.jsm
> @@ +21,5 @@
> > +    } else {
> > +      name = aApp.manifest.name;
> > +    }
> > +
> > +    return this.stripStringForFilename(name).toLowerCase() + "-" + AppsUtils.computeHash(aApp.manifestURL);
> 
> stripStringForFilename() is also defined in WebappsInstaller.jsm but has a
> slightly different behavior. Can you rename it to something like
> sanitizeStringForFilename ?

Yes, that's a better name.
Anyway I've removed that function from WebappsInstaller.jsm and I'm using the new one everywhere (I think this "naive" sanitization would solve all the naming issues and would only reject unusual characters).
Have you tested to make sure this patch works on other platforms other Desktop - specifically FxAndroid & Firefox OS? You might want to spin a try build and sanity test this on those other two platforms to check.
(In reply to Jason Smith [:jsmith] from comment #23)
> Have you tested to make sure this patch works on other platforms other
> Desktop - specifically FxAndroid & Firefox OS? You might want to spin a try
> build and sanity test this on those other two platforms to check.

The try build in bug 777402 contained also this patch, anyway I'll spin another try build before landing (because the patch has changed a bit) and I'll manually test on all desktop platforms as soon as possible.
(In reply to Marco Castelluccio [:marco] from comment #24)
> (In reply to Jason Smith [:jsmith] from comment #23)
> > Have you tested to make sure this patch works on other platforms other
> > Desktop - specifically FxAndroid & Firefox OS? You might want to spin a try
> > build and sanity test this on those other two platforms to check.
> 
> The try build in bug 777402 contained also this patch, anyway I'll spin
> another try build before landing (because the patch has changed a bit) and
> I'll manually test on all desktop platforms as soon as possible.

I was actually suggesting regression testing FxAndroid & Firefox OS specifically just so that no serious regressions happen on those platforms.
(In reply to vkrishnamoorthy@mozilla.com [:Vishy] from comment #17)
> Quoting from MDN
> https://developer.mozilla.org/en-US/docs/Web/Apps/FAQs/About_app_manifests
> "Can I have more than one app at my origin?
>     No, there can be only one app per origin. If multiple apps were allowed
> for a single origin, they would live in a single web sandbox -- they could
> examine each other's localStorage, do Ajax requests to each other's APIs, or
> even steal access to privileged APIs that should have been granted to only
> one of the apps. This would be especially dangerous for domains that publish
> user-generated content from many users.
>     We recommend that you use a separate subdomain for each of your apps.
> For example, spreadsheet.mycoolapps.com for one app and
> texteditor.mycoolapps.com for another. For more information, see Adding a
> subdomain for an app.
>     Many resources and permissions on the Web are already scoped to a single
> origin. By defining an app and an origin as the same thing we use the same
> security restrictions that are used elsewhere on the Web and in HTML5."
> 
> If our security model is capable of providing the app sandbox per app (from
> the same origin) we can reflect that in the FAQ

-> dev-doc-needed, the quoted text is plain wrong.
Keywords: dev-doc-needed
Turns out there were other places where the origin was used instead of the manifest, this fixes them.

Try run here: https://tbpl.mozilla.org/?tree=Try&rev=b94be5ad6866
Attachment #777340 - Attachment is obsolete: true
Attachment #777340 - Flags: review?(myk)
Attachment #777340 - Flags: review?(fabrice)
Attachment #778169 - Flags: review?(myk)
Attachment #778169 - Flags: review?(fabrice)
Comment on attachment 778169 [details] [diff] [review]
Use app name + manifest url hash as unique name for apps v2

I'm going to add myself as feedback to check on FxAndroid & B2G to ensure that this patch doesn't introduce serious regressions on those platforms.
Attachment #778169 - Flags: feedback?(jsmith)
There's one test that is failing (dom/tests/mochitest/localstorage/test_app_uninstall.html) that leaves an application installed.
This causes other tests to fail, because they expect no apps to be installed.
The problem is that in the test data for the webapps registry there were't the app names, but they're a required field that must be always present.

I think you can use the try builds to test on Android and b2g, this new patch shouldn't change anything.

Anyway, new try build at https://tbpl.mozilla.org/?tree=Try&rev=e17633d77e47
Attachment #778169 - Attachment is obsolete: true
Attachment #778169 - Flags: review?(myk)
Attachment #778169 - Flags: review?(fabrice)
Attachment #778169 - Flags: feedback?(jsmith)
Attachment #778328 - Flags: review?(myk)
Attachment #778328 - Flags: review?(fabrice)
Attachment #778328 - Flags: feedback?(jsmith)
(In reply to Marco Castelluccio [:marco] from comment #29)
> Anyway, new try build at https://tbpl.mozilla.org/?tree=Try&rev=e17633d77e47

Looks fairly green :)
Sanity testing on unagi:

Focus area - I focused on checking that the phone remains functional with the attached patch. Additionally, I did some sanity testing around app installs, including testing the multiple apps per origin case cited on this bug. I did not do any app update testing.

Results - Mostly okay, although there is one bad bug present. Looks like what's happening is that if you have multiple apps installed from the same origin, Gaia will only allow you to have one app context open at a time for a single origin. This means that if I try the following STR:

1. Install two apps from the same origin
2. Launch app #1 and scroll down the page
3. Launch app #2

Expected - app #2 should go to it's launch path and it's initial starting point.

Actual - app #2 just loads up the same app context as app #1

I'm guessing this is a bug in Gaia, right? If so, we should get a followup bug filed for that.

As for B2G testing, given that this doesn't cause serious regressions, I'll mark a conditional pass on testing for this noting that we need to investigate and fix bugs around assumptions made in Gaia on the app origin rules.
Sanity testing on FxAndroid on a Galaxy Nexus:

Focus area - Focuses on testing app installs, including multiple apps per origin.

Results - Almost entirely broken. Apps fail to launch with this patch.
Comment on attachment 778328 [details] [diff] [review]
Use app name + manifest url hash as unique name for apps v2

Conditional pass for Firefox OS, fail for FxAndroid regressions.
Attachment #778328 - Flags: feedback?(jsmith) → feedback-
Mark - Do you know why the patch here is going to break app launch on FxAndroid?
Flags: needinfo?(mark.finkle)
If I had to guess, I'd assume the change from 'origin' to 'manifestURL' in the code is not carried through to the Fennec java code. We use 'origin' heavily to create a mapping between WebApps and profiles used to run the WebApps.

GeckoAppShell.java, GeckoApp.java, WebAppImpl.java, browser.js and WebAppRT.js are places where we use 'origin' currently. We also tie the 'origin' to the intent used to start the WebApp from a shortcut icon, iirc.
Flags: needinfo?(mark.finkle)
(In reply to Mark Finkle (:mfinkle) from comment #35)
> If I had to guess, I'd assume the change from 'origin' to 'manifestURL' in
> the code is not carried through to the Fennec java code. We use 'origin'
> heavily to create a mapping between WebApps and profiles used to run the
> WebApps.
> 
> GeckoAppShell.java, GeckoApp.java, WebAppImpl.java, browser.js and
> WebAppRT.js are places where we use 'origin' currently. We also tie the
> 'origin' to the intent used to start the WebApp from a shortcut icon, iirc.

Yes, that's what I thought. But this patch shouldn't break apps launch. It should be possible to launch apps, but it shouldn't be possible to launch multiple apps per origin.
I'll try to fix also the Java code (that is the only thing I didn't fix with this patch) and see what happens.
We also probably want a sec-review here. This changes a fundamental piece with Firefox OS, so Paul might want to take a look here.
Flags: sec-review?(ptheriault)
Attached patch Android stuff (obsolete) — Splinter Review
(In reply to Marco Castelluccio [:marco] from comment #36)
> Yes, that's what I thought. But this patch shouldn't break apps launch. It
> should be possible to launch apps, but it shouldn't be possible to launch
> multiple apps per origin.
> I'll try to fix also the Java code (that is the only thing I didn't fix with
> this patch) and see what happens.

So it's slightly difficult for me as I haven't a device to use for debug (on my phone apps installation doesn't work, even with the release version) and I don't know your code well.

Anyway I think it should be possible to support multiple apps per origin by:
1) Adding a manifest url argument to the WebAppAllocator functions (and for new installations store only the manifestURL).
2) WebAppAllocator.getIndexForApp should initially search by manifest URL and then by origin (to support old installations).
3) Where you needed an origin, for example to launch an application, you can do something like WebAppAllocator.getAppForIndex().getPrePath() that works for both manifest urls and origins.

So, I believe something like this should work.

James, could you look into this?
Flags: needinfo?(jhugman)
Attachment #778652 - Attachment is patch: true
From the title of this bug, this sounds desktop specific, but am I to assume that this is planned for all apps platforms (ie desktop, firefox mobile, firefox OS) ? I note the parent bug, but I don't see a Firefox OS specific one? Also what is the priority of this (ie when are we aiming to make this change?)
Comment on attachment 778328 [details] [diff] [review]
Use app name + manifest url hash as unique name for apps v2

I've written a reduced patch for the things I need in order to support packaged apps on desktop, uploaded in bug 777402.

I'll post here a patch that contains the remaining fixes for multiple apps per origin (but I'd like someone else to tackle Android support).

Anyway, I think hosted apps support on Android may be broken on the current nightly due to this changeset: http://hg.mozilla.org/mozilla-central/rev/d11b778458e9.
We should pass app.manifest and not manifest to the callback here: (http://hg.mozilla.org/mozilla-central/rev/d11b778458e9#l1.52). At least, this is true on desktop.
Attachment #778328 - Attachment is obsolete: true
Attachment #778328 - Flags: review?(myk)
Attachment #778328 - Flags: review?(fabrice)
Summary: Add support of installing of multiple apps off of the same origin for the desktop web runtime → Add support of installing of multiple apps off of the same origin
Version: 16 Branch → Trunk
Component: Webapp Runtime → DOM: Apps
Product: Firefox → Core
No longer depends on: 775847
Duplicate of this bug: 775847
Depends on: 786303
Depends on: 779982
Blocks: 778279
No longer blocks: 778297
Depends on: 778297
Blocks: 778297
No longer depends on: 778297
No longer blocks: 778279
Depends on: 778279
Depends on: 896127
Just a quick comment (I did not take a look at the patch) : I'm not surprised this is breaking b2g. Gaia is unfortunately using origins as key in the homescreen, and there are other changes needed in Webapps.jsm

What I want mostly, is that every new code written be ready to support multiple hosted apps by origin (we can't have several packaged apps sharing the same origin, for other reasons). So basically, never rely on the origin but on the manifest URL.
(In reply to Fabrice Desré [:fabrice] from comment #12)
> The only reason we did not multiple apps per origin from day 1 is an
> unfortunate decision. The security model fully support them (the app key is
> the manifest URL, not the origin). There's no doubt we want that to work.

To be clear, the above is entirely correct. We've made the decision many times to allow multiple apps per origin. And we have authors that are asking for it.

We also kept this in mind when designing the Apps API.

That said, we do have a bunch of work needed internally to make this work.
Depends on: 909573
Unassigning myself, with bug 909573 landed we should be ready on desktop.
Assignee: mcastelluccio → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(jhugman)
Is anyone still working on this? Maybe as part of the new window manager since they seem to share dependent bugs somewhat?
(In reply to Paul Theriault [:pauljt] from comment #45)
> Is anyone still working on this? Maybe as part of the new window manager
> since they seem to share dependent bugs somewhat?

Not really. We probably can put the sec-review here on hold until this picks up again.
Flags: sec-review?(ptheriault)
Just to give my two cents since I'm working with a lot of Firefox OS developers: it's a feature requested quite often or at least, a problem/question that come back often.
Blocks: 1033360
Attached patch dom/apps changesSplinter Review
Surprisingly few changes to be done on the dom side. I will check gaia and marionette, could you confirm that the other runtimes are ok?
Assignee: nobody → fabrice
Attachment #778652 - Attachment is obsolete: true
Attachment #8453543 - Flags: review?(myk)
Attached patch new testsSplinter Review
And the new tests, where we install & uninstall 2 apps from the same origin.
Attachment #8453544 - Flags: review?(myk)
Depends on: 1036788
(In reply to Fabrice Desré [:fabrice] from comment #48)
> Created attachment 8453543 [details] [diff] [review]
> dom/apps changes
> 
> Surprisingly few changes to be done on the dom side. I will check gaia and
> marionette, could you confirm that the other runtimes are ok?

I did some changes to dom/ and desktop specific bits last year to support this, so desktop should be ready.
Android and B2G weren't ready, there are some bugs filed.
B2G: bug 896127, bug 913323, bug 1036788 (they're probably duplicates).
Android: bug 778279 (this was filed for the old implementation, I don't know if something has changed recently)

(In reply to Fabrice Desré [:fabrice] from comment #49)
> Created attachment 8453544 [details] [diff] [review]
> new tests
> 
> And the new tests, where we install & uninstall 2 apps from the same origin.

It'd be great if you could write an equivalent test for desktop in toolkit/webapps/tests/. Otherwise, please file a new bug to track its implementation and I'll take care of it.
Comment on attachment 8453543 [details] [diff] [review]
dom/apps changes

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

(In reply to Fabrice Desré [:fabrice] from comment #48)
> Surprisingly few changes to be done on the dom side.

There may be hidden assumptions that regress behavior subtly.  Is it urgent to land this in the current cycle, or can we wait and land it at the beginning of the next one (July 21), so we have a full cycle to shake it out?


(In reply to Marco Castelluccio [:marco] from comment #50)
> Android: bug 778279 (this was filed for the old implementation, I don't know
> if something has changed recently)

Android still identifies apps by origin in some places: mobile/android/chrome/content/aboutApps.js, mobile/android/base/webapp/Allocator.java, and all the callers of Allocator.getIndexForOrigin/getOrigin/putOrigin.

aboutApps.js should be trivial to fix; but Allocator and its consumers might be trickier, because users have existing apps that are indexed by origin in their Android SharedPreferences stores, and we'll need to update those existing stores to ensure the apps continue to work.
Attachment #8453543 - Flags: review?(myk) → review+
Attachment #8453544 - Flags: review?(myk) → review+
I'm fine with waiting for the next cycle. That would be nice to check the android state asap though!
Myk, are we good to land now or do you want to wait for the android side to be fixed too? (I could not find a bug for it)
Flags: needinfo?(myk)
The Android runtime changes are bug 778279, for which the current status is that I finished the patch for it yesterday, and it's awaiting review from mfinkle.

Bug 778279 is marked as blocking this bug, but I don't think we need to wait for that one to land before we land this one, as there's relatively little bustage.  The only weird behavior folks may see is when launching an app from about:apps: if the app has the same origin as another app, tapping the icon to launch it might launch the other app instead.

But about:apps is a secondary interface for launching apps on Android.  The primary interface is the native All Apps screen.  And tapping app icons on that screen will continue to work as expected.
Flags: needinfo?(myk)
(In reply to Myk Melez [:myk] [@mykmelez] from comment #54)
> The Android runtime changes are bug 778279, for which the current status is
> that I finished the patch for it yesterday, and it's awaiting review from
> mfinkle.

Update: mfinkle just granted review, and I pushed the patch to fx-team.
Fabrice, have you filed the bug about the desktop tests? If yes, could you CC me?
(In reply to Marco Castelluccio [:marco] from comment #57)
> Fabrice, have you filed the bug about the desktop tests? If yes, could you
> CC me?

I had not, but it's now bug 1042409
(In reply to Fabrice Desré [:fabrice] from comment #58)
> (In reply to Marco Castelluccio [:marco] from comment #57)
> > Fabrice, have you filed the bug about the desktop tests? If yes, could you
> > CC me?
> 
> I had not, but it's now bug 1042409

Thank you ;)
I see this is in Resolve Fixed status. Do we know when will it be pushed to production?
Will be happy to help if needed in QA.
Given that the target milestone is mozilla34, that means that this will be in the Gecko 34 release. According to the release calendar [1] that means that this will be in the FirefoxOS 2.1 release. It should also be in Firefox 34 for desktop and Android.

[1] https://wiki.mozilla.org/RapidRelease/Calendar
I wrote the following information to explain the change (thanks to everyone who helped with the explanation!)

Please note that the single-app-per-origin restriction has been lifted (see {{Bug("778277")}}) — this change was landed in Gecko 34, meaning that Firefox OS 2.1+, Firefox Desktop 34+ and Firefox for Android 34+ will be able to install multiple apps from the same origin.

Bear in mind that the overall apps security model hasn’t changed, but this change does have a security implications. If you have multiple apps on the same origin, the content on this origin effectively shares all permissions across all apps, as there is no concept of App Scopes (these are being worked on). Given the permissions currently exposed to hosted apps, this isn’t much of a security concern, but developers should not expect any type of segregation between apps.

However, in the same manner as standard web development, you shouldn't host high security web apps (e.g. banking, customer records) on the same origin as other web content with lower security requirements. If you have a vulnerability in App A on your domain, it might be possible for it to be exploited via App B on the same domain.

---

Articles updated to recognise this change:

* https://developer.mozilla.org/en-US/Apps/Build/installable_apps_for_Firefox_OS/App_manifest_FAQ
* https://developer.mozilla.org/en-US/Apps/Build/installable_apps_for_Firefox_OS/Firefox_OS_app_essentials
* https://developer.mozilla.org/en-US/Firefox_OS/Releases/2.1
* https://developer.mozilla.org/en-US/Marketplace/Publishing/Adding_a_subdomain
* https://developer.mozilla.org/en-US/docs/Web/API/Apps.getInstalled
* https://developer.mozilla.org/en-US/docs/Web/API/Apps.getSelf
* https://developer.mozilla.org/en-US/docs/Web/API/Apps.install
* https://developer.mozilla.org/en-US/Marketplace/Options/Hosted_apps
* https://developer.mozilla.org/en-US/Marketplace/Options/Packaged_or_hosted_

Let me know if you notice any others that need edits!
That is not correct. Apps do not share permissions even if they live on the same origin. If the user opens App A then pages on As origin will have As permission. If the user opens App B, then pages on Bs origin will have Bs permission. Note that apps are kept visually separate so users always know which app they are running and permissions are granted based on that.
(In reply to Jonas Sicking (:sicking) from comment #64)
> That is not correct. Apps do not share permissions even if they live on the
> same origin. If the user opens App A then pages on As origin will have As
> permission. If the user opens App B, then pages on Bs origin will have Bs
> permission. Note that apps are kept visually separate so users always know
> which app they are running and permissions are granted based on that.

Ack, dammnit. Looks like I've severely misunderstood something Paul wrote on the recent thread about it. I've removed that stuff from MDN for now. So what are the security implications of having apps on the same origin (as mentioned above)? the relevant mail thread is here: https://groups.google.com/forum/#!topic/mozilla.dev.gaia/TNaXDQmNhCk
The FAQ entry now starts with "Can I have more than one app at my origin? This used to be the case. [...]", but it of course used *not* to be the case.

The description of Apps.getSelf() suggests undefined behaviour for multiple apps per origin. "Returns information about [...] an installed app [which if multiple?] whose domain matches the domain of the calling app." I presume this should simply read "Returns information about the calling app, if any.". Similarly, "You can use this to determine if an app is installed." should probably be changed into "You can use this to determine if the web page is running as an app, and if so, which." (multiple apps can be installed, but the script is running in either the browser or in one of the installed apps).
So here's the deal:

Say that there are three apps on example.com: A, B and C. All three apps have different permissions.

The user installs apps A and B.

The general rule is that only pages in app A will have As permissions. And only pages in app B will have Bs permissions. And no pages will have Cs permissions. "Pages in app A" here means pages that are rendered inside the piece of UI that is created for App A. I.e. inside App As windows.

So the fact that we allow multiple apps from the same origin didn't affect the permission model at all.

This is the main message that we should deliver.

However. There are some complexities to consider. Especially for anyone that can upload files to example.com. Which includes the developers of Apps A, B and C.

Any page which is uploaded to example.com might get navigated to by App A. Or app B. Or app C. That means that such a page *might* potentially run with As or Bs or Cs permissions. I.e. there is a chance that the user will install app A, and that app A might navigate to the uploaded page. In which case the page would run with As permissions. Same thing with B or C of course.

So hosting an app with lots of permissions on the same server as other content that you don't expect to have the same level of permissions is a bad idea. That is independent of of that "other content" is part of another app or not.


This is what Paul was trying to say (I feel fairly certain about).

Also note that Paul was wrong about one thing. App scopes does not affect this at all since even with app scopes any page that is part of App A can open an <iframe> to other pages on the same server. In which case those pages will have As permissions. Independent of if those pages are part of As scope or not.
(In reply to Jeroen van der Gun from comment #66)
> The FAQ entry now starts with "Can I have more than one app at my origin?
> This used to be the case. [...]", but it of course used *not* to be the case.

Thanks for pointing out this error - I've fixed it now.

> The description of Apps.getSelf() suggests undefined behaviour for multiple
> apps per origin. "Returns information about [...] an installed app [which if
> multiple?] whose domain matches the domain of the calling app." I presume
> this should simply read "Returns information about the calling app, if
> any.".Similarly, "You can use this to determine if an app is installed."
> should probably be changed into "You can use this to determine if the web
> page is running as an app, and if so, which." (multiple apps can be
> installed, but the script is running in either the browser or in one of the
> installed apps).

I've had a go at improving the language on this article - let me know if this sounds ok. https://developer.mozilla.org/en-US/docs/Web/API/Apps.getSelf
(In reply to Jonas Sicking (:sicking) from comment #67)
> So here's the deal:
> 
> Say that there are three apps on example.com: A, B and C. All three apps
> have different permissions.
> 
> The user installs apps A and B.
> 
> The general rule is that only pages in app A will have As permissions. And
> only pages in app B will have Bs permissions. And no pages will have Cs
> permissions. "Pages in app A" here means pages that are rendered inside the
> piece of UI that is created for App A. I.e. inside App As windows.
> 
> So the fact that we allow multiple apps from the same origin didn't affect
> the permission model at all.
> 
> This is the main message that we should deliver.
> 
> However. There are some complexities to consider. Especially for anyone that
> can upload files to example.com. Which includes the developers of Apps A, B
> and C.
> 
> Any page which is uploaded to example.com might get navigated to by App A.
> Or app B. Or app C. That means that such a page *might* potentially run with
> As or Bs or Cs permissions. I.e. there is a chance that the user will
> install app A, and that app A might navigate to the uploaded page. In which
> case the page would run with As permissions. Same thing with B or C of
> course.
> 
> So hosting an app with lots of permissions on the same server as other
> content that you don't expect to have the same level of permissions is a bad
> idea. That is independent of of that "other content" is part of another app
> or not.
> 
> 
> This is what Paul was trying to say (I feel fairly certain about).
> 
> Also note that Paul was wrong about one thing. App scopes does not affect
> this at all since even with app scopes any page that is part of App A can
> open an <iframe> to other pages on the same server. In which case those
> pages will have As permissions. Independent of if those pages are part of As
> scope or not.

Thanks Jonas - this really helps. I have written up your explanation here:

https://developer.mozilla.org/en-US/Apps/Build/installable_apps_for_Firefox_OS/App_manifest_FAQ#Can_I_have_more_than_one_app_at_one_origin.3F

Is this ok?
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.