Closed Bug 896436 Opened 7 years ago Closed 7 years ago

Notes Plus app update - fail during oauth redirect

Categories

(Core Graveyard :: DOM: Apps, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:leo+, firefox23 wontfix, firefox24 wontfix, firefox25 fixed, b2g18 verified, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

RESOLVED FIXED
mozilla25
blocking-b2g leo+
Tracking Status
firefox23 --- wontfix
firefox24 --- wontfix
firefox25 --- fixed
b2g18 --- verified
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: eviljeff, Assigned: fabrice)

References

()

Details

(Whiteboard: [LeoVB+])

Attachments

(2 files)

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.
Attached file manifest.webapp
See Also: → 889356
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.
Flags: needinfo?(poirot.alex)
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.
Blocks: b2g-notes
(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?
I'm going to let fabrice clarify more of details here. I need to dig into how this a little more myself.
Flags: needinfo?(fabrice)
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.
Flags: needinfo?(poirot.alex)
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.
Flags: needinfo?(fabrice)
Putting needinfo on me to investigate.
Flags: needinfo?(jsmith)
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
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
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).
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.
No longer blocks: b2g-notes
Component: Preinstalled B2G Apps → DOM: Apps
Flags: needinfo?(jsmith)
Product: Tech Evangelism → Core
Version: unspecified → Trunk
blocking-b2g: --- → leo?
Attached patch patchSplinter Review
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.
Assignee: nobody → fabrice
Attachment #779941 - Flags: review?(ferjmoreno)
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
Attachment #779941 - Flags: review?(ferjmoreno)
Comment on attachment 779941 [details] [diff] [review]
patch

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

This was clarified via IRC
Attachment #779941 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2cda8a170ea4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Has this been merged into 1.0.1 and 1.1 branches also?
(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.
blocking-b2g: leo? → leo+
(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?
It's a "not yet"
Note to Andrew - this is being fixed in 1.1, not 1.01.
Keywords: verifyme
QA Contact: jsmith
(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.
The redirected sanity test is passing now. So marking as verified.
Whiteboard: [LeoVB+]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.