FxAccountUtils.getAudienceForURL does not include the port number

RESOLVED FIXED in Firefox 33

Status

()

Firefox for Android
Android Sync
RESOLVED FIXED
4 years ago
9 months ago

People

(Reporter: rfkelly, Assigned: Ben Curtis, Mentored, NeedInfo)

Tracking

unspecified
Firefox 34
All
Android
Points:
---

Firefox Tracking Flags

(firefox31 wontfix, firefox32 wontfix, firefox33 fixed, firefox34 fixed)

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
(From https://github.com/mozilla-services/syncserver/issues/36)

The FxAccountUtils.getAudienceForURL() function does not include the port number in the generated string:

   http://hg.mozilla.org/mozilla-central/file/f61a27b00e05/mobile/android/base/background/fxa/FxAccountUtils.java#l186

It should include the port number if it's non-standard (i.e. if non-80 for http:// urls, and non-443 for https:// urls).
Mentor: nalexander
OS: Windows 7 → Android
Hardware: x86_64 → All
Whiteboard: [lang=java][good first bug]
(Assignee)

Comment 1

4 years ago
I don't believe there is a formal way to contribute to the project, but I set up a dev environment to test this out and can confirm that changing line 188 to:

return new URI(uri.getScheme(), null, uri.getHost(), uri.getPort(), null, null, null).toString();

Functions perfectly for me.
(Assignee)

Comment 2

4 years ago
Created attachment 8466656 [details] [diff] [review]
fixport.patch

Updated audience function to include the URI port for self hosted sync servers on non-standard ports.
Attachment #8466656 - Flags: review?(nalexander)
Hi Ben,

Sorry this took so long to get to, I was on holiday.  This looks great.  I ported to the android-sync repository (with tests); commit is https://github.com/mozilla-services/android-sync/commit/b49a3ea2716b390fa728eae83f2f7cba1707fd10

Hasn't landed on m-c yet due to a large tree closure.  ni to me to land.  Thanks for the bug report and the fix!

Tested against a local sync server running over HTTP at port 5000, and the regular cluster running over HTTPS at port 443.
Assignee: nobody → firefox
Status: NEW → ASSIGNED
Flags: needinfo?(nalexander)
Attachment #8466656 - Flags: review?(nalexander)
Attachment #8466656 - Flags: review+
Attachment #8466656 - Flags: approval-mozilla-aurora?
Attachment #8466656 - Flags: approval-mozilla-aurora?
Flags: needinfo?(nalexander)
Comment on attachment 8466656 [details] [diff] [review]
fixport.patch

Approval Request Comment
[Feature/regressing bug #]: since initial FxA landing (Firefox 29).

[User impact if declined]: custom sync servers cannot be run on non-standard ports on Android.

[Describe test coverage new/current, TBPL]: nothing on TBPL, but there's a test in the android-sync repository.

[Risks and why]: very low.  We'd see a failure on a new Sync account with the regular parameters almost immediately in Nightly.  We'd see custom sync server bustage when the reporter verified.

[String/UUID change made/needed]: none.
Attachment #8466656 - Flags: approval-mozilla-aurora?
This affects Firefox 33, which is the first Android version to support custom sync servers.  It is present from 29 through 32, but can't be seen because there's no way to specify a non-standard port in those versions.
status-firefox31: --- → wontfix
status-firefox32: --- → wontfix
status-firefox33: --- → affected
Flags: needinfo?(nalexander)
(Assignee)

Comment 6

4 years ago
NP, glad I could help, just took a bit to figure out how to submit patches (used to GitHub like with FXA). Out of curiosity, is there a way to track when this will hit nightlies and the Play Store?
(In reply to Ben Curtis from comment #6)
> NP, glad I could help, just took a bit to figure out how to submit patches
> (used to GitHub like with FXA).

There is a github repo for this part of Fennec -- https://github.com/mozilla-services/android-sync.  But all tracking happens on Bugzilla, and the final code lands on mozilla-central.

 Out of curiosity, is there a way to track
> when this will hit nightlies and the Play Store?

Sort of.  It should hit a Nightly tonight; watch for a mozilla-central hg URL like the fx-team hg URL above.  None of this has hit the Play Store yet, since only Release and Beta are on Play, and this is still in Aurora (Firefox 33).  I've requested uplift for this fix, so it should make it to Aurora well before Aurora goes to Beta.  See the tracking-firefox-XX flags on Bugzilla.  That'll be in 4 weeks, IIRC.  You can track the release calendar at https://mail.mozilla.com/home/publiccalendar@mozilla.com/Releases%20Scheduling.html.

Yes, this is complicated.  Feel free to ask questions.  A note if this works for you in Nightly would be much appreciated.  Thanks again!
Flags: needinfo?(nalexander)
https://hg.mozilla.org/mozilla-central/rev/0fd0a2566a51
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
status-firefox34: --- → fixed
Attachment #8466656 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs rebasing for Aurora uplift.
Flags: needinfo?(firefox)
Keywords: branch-patch-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #10)
> Needs rebasing for Aurora uplift.

This surprises me; I'll look into it.
Flags: needinfo?(firefox) → needinfo?(nalexander)
Not sure what happened here:

hocho:gecko nalexander$ hg graft -e -r 0fd0a2566a51
grafting revision 223239
merging mobile/android/base/fxa/authenticator/FxAccountAuthenticatorService.java
merging mobile/android/base/fxa/sync/FxAccountSyncAdapter.java
...

worked fine.  Pushed:

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/8588f150315a
Flags: needinfo?(nalexander)
status-firefox33: affected → fixed
Keywords: branch-patch-needed
Argh.  There was a reason this needed rebasing, and it's 'cuz I reverted a bunch of things that were supposed to be backported.  Sorry for the noise.  The additional commit does not need to be uplifted.

https://hg.mozilla.org/integration/fx-team/rev/be85189fb309

Comment 15

4 years ago
Hi,
sorry to dig in this old bug, but using the current version of firefox mobile from the play store and firefox beta I still get this output from the syncserver:

ERROR:syncserver:The public_url setting does not match the application url.
This will almost certainly cause authentication failures!
    public_url setting is: http://alarmm:5000
    application url is:    http://alarmm

syncserver.ini contained 

public_url = http://alarmm:5000/ 

in this case, fxa-custom-server-addon is properly configured.

Sync does not work for the mobile device in this case.
The solution was to use the nginx proxy ( as recommended in the available documents ) and create a proper setup running on port 80 (using via VPN).
(In reply to robspamm from comment #15)
> Hi,
> sorry to dig in this old bug, but using the current version of firefox
> mobile from the play store and firefox beta I still get this output from the
> syncserver:
> 
> ERROR:syncserver:The public_url setting does not match the application url.
> This will almost certainly cause authentication failures!
>     public_url setting is: http://alarmm:5000
>     application url is:    http://alarmm
> 
> syncserver.ini contained 
> 
> public_url = http://alarmm:5000/ 
> 
> in this case, fxa-custom-server-addon is properly configured.
> 
> Sync does not work for the mobile device in this case.
> The solution was to use the nginx proxy ( as recommended in the available
> documents ) and create a proper setup running on port 80 (using via VPN).

I don't think this is an issue with fxa-custom-server-addon; I think this is an issue with your syncserver configuration.  I'm flagging rfkelly who I'm quite sure will recognize the issue.
Flags: needinfo?(robspamm)
Flags: needinfo?(rfkelly)
(Reporter)

Comment 17

4 years ago
(Just dropping a quick note to ack this, but I'm on PTO so I won't get a chance to look at it until next week)
(Reporter)

Comment 18

4 years ago
> using the current version of firefox mobile from the play store and firefox beta I still get this output
> from the syncserver:
>
> ERROR:syncserver:The public_url setting does not match the application url.
> This will almost certainly cause authentication failures!
>    public_url setting is: http://alarmm:5000
>    application url is:    http://alarmm

If you're getting output from the syncserver app, then the mobile client is successfully hitting the server.  If the server's running on port 5000, this means that the mobile client is successfully talking on port 5000.

Was the server also listening on port 80?  Otherwise I think this is almost certainly a server-side problem
Flags: needinfo?(rfkelly)

Updated

9 months ago
Product: Android Background Services → Firefox for Android
You need to log in before you can comment on or make changes to this bug.