1046020 - FxAccountUtils.getAudienceForURL does not include the port number

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, defect)

Description

[Ryan Kelly [:rfkelly]]

(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).

Comment 1

[Ben Curtis]

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.

Comment 2

[Ben Curtis]

Attachment: fixport.patch

Updated audience function to include the URI port for self hosted sync servers on non-standard ports.

Comment 3

[Nick Alexander :nalexander [he/him]]

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.

Comment 4

[Nick Alexander :nalexander [he/him]]

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.

Comment 5

[Nick Alexander :nalexander [he/him]]

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.

Comment 6

[Ben Curtis]

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?

Comment 7

[Nick Alexander :nalexander [he/him]]

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

Comment 8

[Nick Alexander :nalexander [he/him]]

(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/[e-mail]/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!

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0fd0a2566a51

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora uplift.

Comment 11

[Nick Alexander :nalexander [he/him]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #10)
> Needs rebasing for Aurora uplift.

This surprises me; I'll look into it.

Comment 12

[Nick Alexander :nalexander [he/him]]

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

Comment 13

[Nick Alexander :nalexander [he/him]]

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 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/be85189fb309

Comment 15

[robspamm]

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).

Comment 16

[Nick Alexander :nalexander [he/him]]

(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.

Comment 17

[Ryan Kelly [:rfkelly]]

(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)

Comment 18

[Ryan Kelly [:rfkelly]]

> 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