896436 - Notes Plus app update - fail during oauth redirect

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Andrew Williamson [:eviljeff]]

the most recent update adds a /redirects/ manifest entry to log in via evernote.  Unfortunately this isn't working.  After providing evernote login details and authentificating an error is shown because the page 'http://ffos-notes.local/redirect.html' is loaded, rather than /redirect.html.

I'm installing via the reviewer tools so this is a signed package installed properly, rather than sideloaded, etc.

Comment 1

[Andrew Williamson [:eviljeff]]

Attachment: manifest.webapp

Comment 2

[:Harald Kirschner :digitarald]

Hey ochameau, as this looks very similar to bug 889356, maybe you can have a look. I only saw redirects working in preloaded gaia apps; maybe something in the webapps source is not picking them up on install?

Or is this even simpler and might be fixed by the patch from bug 889356.

Comment 3

[Jason Smith [:jsmith]]

That's a bug in the app. http://ffos-notes.local/redirect.html isn't a valid URL. The from part of a redirect needs to be a valid URL that reports a 302.

Comment 4

[Andrew Williamson [:eviljeff]]

(In reply to Jason Smith [:jsmith] from comment #3)
> That's a bug in the app. http://ffos-notes.local/redirect.html isn't a valid
> URL. The from part of a redirect needs to be a valid URL that reports a 302.

Oh, okay.  I read the manifest docs on MDN as meaning that the url (http://ffos-notes.local/redirect.html) had to be /requested/ via a 302 from the evernote website.

What Location does the from URL have to report a redirect to?

Comment 5

[Jason Smith [:jsmith]]

I'm going to let fabrice clarify more of details here. I need to dig into how this a little more myself.

Comment 6

[Alexandre Poirot [:ochameau]]

The `from` field has to be the redirection target, not the redirection source,
so let say evernote.com/signed-in/ redirect to mozilla.org/notes/, the `from` field being set to http://mozilla.org/notes/, the final redirection of evernotes.com/signed-in will be replaced by the value of `to` field.

I manually tested that feature with the following test case in bug 889356 comment 26. But only when sideloading the app, with the required fix. I haven't tried via the marketplace.

Comment 7

[Jason Smith [:jsmith]]

Ah, comment 6 is right, I think I misinterpreted something here.

It isn't the URL reporting the 302, it's the URL target that you 302 sends you to should be set in the from field.

I do wonder if the URL has to be a valid source, however.

Comment 8

[Jason Smith [:jsmith]]

Putting needinfo on me to investigate.

Comment 9

[Andrew Williamson [:eviljeff]]

so I guess its either:
a) from needs to be a valid url
b) evernote isn't using a 302 redirect
c) the implementation on fxos is broken

Comment 10

[Andrew Williamson [:eviljeff]]

okay, 
I took the test case that ochameau provided; finished and uploaded it to Marketplace:

https://marketplace.firefox.com/reviewers/apps/review/redirected

- it fails when installed via Marketplace.  Assuming the server config is still correct then I'm leaning towards c) in #comment #9

Comment 11

[[:fabrice] Fabrice Desré]

Can you check that this is a privileged app? redirects are not usable by non-privileged apps.
Also, I'd like to see the webapps.json file (in /data/local/webapps/webapps.json on a device).

Comment 12

[Jason Smith [:jsmith]]

Checked this myself - I can confirm what Andrew is talking about. This privileged app redirects property works when sideloaded, but fails when installed from marketplace. This definitely looks like a bug on the DOM Apps side.

Comment 13

[[:fabrice] Fabrice Desré]

Attachment: patch

We were not sending the updated app object to child processes after downloading the package and reading the redirects in the manifest. So if you start the app right after installing it, it uses the preallocated process that has an outdated version.

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 779941 [details] [diff] [review]
patch

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

::: dom/apps/src/AppsService.js
@@ +110,5 @@
>            return Services.io.newURI(to, null, null);
>          }
>        }
> +      // No matching redirect.
> +      return null;

nit: I think you can just use the |return null;| at line 116 removing the else.

::: dom/apps/src/Webapps.jsm
@@ +2004,5 @@
>          app.downloading = false;
>          app.downloadAvailable = false;
>          this._saveApps((function() {
>            this.updateAppHandlers(null, aManifest, appObject);
> +          this.broadcastMessage("Webapps:AddApp", { id: aId, app: appObject });

Shouldn't we update and notify with 'app' [1] instead? Like:

this.updateAppHandlers(null, aManifest, app);
this.broadcastMessage("Webapps:AddApp", { id: aId, app: app });


[1] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#2088

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 779941 [details] [diff] [review]
patch

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

This was clarified via IRC

Comment 16

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/birch/rev/2cda8a170ea4

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2cda8a170ea4

Comment 18

[Andrew Williamson [:eviljeff]]

Has this been merged into 1.0.1 and 1.1 branches also?

Comment 19

[Alex Keybl [:akeybl]]

(In reply to Andrew Williamson [:eviljeff] from comment #18)
> Has this been merged into 1.0.1 and 1.1 branches also?

leo+, will be merged soon.

Comment 20

[Andrew Williamson [:eviljeff]]

(In reply to Alex Keybl [:akeybl] from comment #19)
> (In reply to Andrew Williamson [:eviljeff] from comment #18)
> > Has this been merged into 1.0.1 and 1.1 branches also?
> 
> leo+, will be merged soon.

... is that a no?

Comment 21

[[:fabrice] Fabrice Desré]

It's a "not yet"

Comment 22

[Jason Smith [:jsmith]]

Note to Andrew - this is being fixed in 1.1, not 1.01.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/c196ea4390f8

Comment 24

[Andrew Williamson [:eviljeff]]

(In reply to Jason Smith [:jsmith] from comment #22)
> Note to Andrew - this is being fixed in 1.1, not 1.01.

okay.

If we approve the update now then everyone on 1.0.1 will get a broken app.  So generally we won't approve such an update/submission until 1.1 is pushed to everyone on 1.0.1.  

Although in this case the app is broken anyway due to evernote changes so we'll probably approve it regardless.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/c196ea4390f8

Comment 26

[Jason Smith [:jsmith]]

The redirected sanity test is passing now. So marking as verified.