Closed Bug 983350 Opened 6 years ago Closed 6 years ago

Send explicit user agent for FxA and token server requests

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox29 --- fixed
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: kparlante, Assigned: nalexander)

References

Details

Attachments

(3 files)

As per: https://bugzilla.mozilla.org/show_bug.cgi?id=968990#c4, send the same user agent string used for sync requests.

We're currently hacking around this by keeping the user agent string around in elasticsearch/kibana and querying on HttpClient for Android segmentations in kibana.
Assignee: nobody → nalexander
OS: Mac OS X → Android
Priority: -- → P1
Hardware: x86 → ARM
Blocks: 968990
Status: NEW → ASSIGNED
Attachment #8390873 - Flags: review?(rnewman) → review+
Those user agents are not RFC 2068 compliant (http://www.faqs.org/rfcs/rfc2068.html)
The product cannot have a space and the version should be slash delimited.

User-Agent 	=	"User-Agent" ":" 1*( product | comment )
product 	=	token ["/" product-version ]
product-version =	token
comment 	=	"(" *( ctext | comment ) ")"
ctext 	        =	<any TEXT excluding "(" and ")">
token 	        =	1*<any CHAR except CTLs or tspecials>
tspecials 	=	"(" | ")" | "<" | ">" | "@" | "," | ";" | ":" | "\" | <"> | "/" | "[" | "]" | "?" | "=" | "{" | "}" | SP | HT
(In reply to Mike Trinkala [:trink] from comment #2)
> Those user agents are not RFC 2068 compliant
> (http://www.faqs.org/rfcs/rfc2068.html)
> The product cannot have a space and the version should be slash delimited.
> 
> User-Agent 	=	"User-Agent" ":" 1*( product | comment )
> product 	=	token ["/" product-version ]
> product-version =	token
> comment 	=	"(" *( ctext | comment ) ")"
> ctext 	        =	<any TEXT excluding "(" and ")">
> token 	        =	1*<any CHAR except CTLs or tspecials>
> tspecials 	=	"(" | ")" | "<" | ">" | "@" | "," | ";" | ":" | "\" | <"> | "/"
> | "[" | "]" | "?" | "=" | "{" | "}" | SP | HT

Interesting.  I'm not sure we care enough to address this now.  For the record, Sync's User-Agent has had spaces for about 2 years; and Apache's HttpClient lib has used for 
CoreProtocolPNames.USER_AGENT: Apache-HttpClient/ (java 1.5) (on Android) for ever.  So clearly this doesn't cause major issues in the wild.
(In reply to Nick Alexander :nalexander from comment #3)
> (In reply to Mike Trinkala [:trink] from comment #2)
> > Those user agents are not RFC 2068 compliant
> > (http://www.faqs.org/rfcs/rfc2068.html)
> > The product cannot have a space and the version should be slash delimited.
> > 
> > User-Agent 	=	"User-Agent" ":" 1*( product | comment )
> > product 	=	token ["/" product-version ]
> > product-version =	token
> > comment 	=	"(" *( ctext | comment ) ")"
> > ctext 	        =	<any TEXT excluding "(" and ")">
> > token 	        =	1*<any CHAR except CTLs or tspecials>
> > tspecials 	=	"(" | ")" | "<" | ">" | "@" | "," | ";" | ":" | "\" | <"> | "/"
> > | "[" | "]" | "?" | "=" | "{" | "}" | SP | HT
> 
> Interesting.  I'm not sure we care enough to address this now.  For the
> record, Sync's User-Agent has had spaces for about 2 years; and Apache's
> HttpClient lib has used for 
> CoreProtocolPNames.USER_AGENT: Apache-HttpClient/ (java 1.5) (on Android)
> for ever.  So clearly this doesn't cause major issues in the wild.

Ah, sorry, I was mis-reading.  I see that Apache's UA is fine.
Landed with the new User-Agent headers obeying the RFC (at least, I hope so :) and the old left untouched (for existing ops query stability, etc).

https://hg.mozilla.org/integration/fx-team/rev/f5109b04e083
https://hg.mozilla.org/mozilla-central/rev/f5109b04e083
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attached patch f5109b04e083Splinter Review
[Approval Request Comment]
Bug caused by (feature/regressing bug #): initial FxA landing.

User impact if declined: *user* impact: almost none.  Ops/metrics impact: more difficult for FxA metrics to determine which version of Android FxA client is hitting endpoints.  This changes a generic User-Agent like:

Apache-HttpClient/UNAVAILABLE (java 1.5)

to a more informative User-Agent including the Firefox for Android version, like:

Firefox-Android-FxAccounts/ (Nightly 31.0a1)

Testing completed (on m-c, etc.): a few days on Nightly and now Aurora; lots of correct User-Agent headers in logs.

Risk to taking this patch (and alternatives if risky): very low.

String or IDL/UUID changes made by this patch: none.
Attachment #8394240 - Flags: approval-mozilla-beta?
kparlante: this is the ticket tracking uplifting the beta User-Agent string.
Flags: needinfo?(kparlante)
Attachment #8394240 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Nick Alexander :nalexander from comment #8)
> kparlante: this is the ticket tracking uplifting the beta User-Agent string.

Thanks!
Flags: needinfo?(kparlante)
Katie, a lot of these user agents will parse with no version number according to https://github.com/mozilla/persona/blob/dev/lib/coarse_user_agent_parser.js.  Is that ok?
Flags: needinfo?(kparlante)
(In reply to Mike Trinkala [:trink] from comment #10)
> Katie, a lot of these user agents will parse with no version number
> according to
> https://github.com/mozilla/persona/blob/dev/lib/coarse_user_agent_parser.js.
> Is that ok?

No, we need the version number from this Android user agent. Being consistent with the persona coarse_user_agent_parser is not particularly important fwiw. I'll file a bug on the puppet repo (unless you want to reopen this bug with a different proposal for the user agent string).
"Firefox-Android-FxAccounts/ (Nightly 31.0a1)" is a new addition and it is incorrect according to the user agent grammar, ideally it would be fixed at the source instead of tweaking downstream consumers (i.e., https://github.com/mozilla-services/puppet-config/issues/312)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Mike Trinkala [:trink] from comment #13)
> "Firefox-Android-FxAccounts/ (Nightly 31.0a1)" is a new addition and it is
> incorrect according to the user agent grammar, ideally it would be fixed at
> the source instead of tweaking downstream consumers (i.e.,
> https://github.com/mozilla-services/puppet-config/issues/312)

Tell me what you want and I'll do it.  I made an error parsing the spec once, and was thrown off by what HttpClient does, so just say what you want the formatting to be and I'll make it happen.
Flags: needinfo?(mtrinkala)
Product/Version is fine
Firefox-Android-FxAccounts/31.0a1 (Nightly)
Flags: needinfo?(mtrinkala)
Flags: needinfo?(kparlante)
(In reply to Mike Trinkala [:trink] from comment #15)
> Product/Version is fine
> Firefox-Android-FxAccounts/31.0a1 (Nightly)

Thanks for the quick response!  Should land in the next few days, and probably in place by EOW (considering uplifts).
rnewman: rubber-stamp?  My perspective is that this is minimal enough to carry forward the a= and triple-land all the way to beta.  What say you?
Attachment #8400103 - Flags: review?(rnewman)
(In reply to Nick Alexander :nalexander from comment #17)
> Created attachment 8400103 [details] [review]
> Link to Github pull-request:
> https://github.com/mozilla-services/android-sync/pull/444
> 
> rnewman: rubber-stamp?  My perspective is that this is minimal enough to
> carry forward the a= and triple-land all the way to beta.  What say you?

From the pull request:  This replaces

Firefox-Android-FxAccounts/ (Nightly 31.0a1)

with

Firefox-Android-FxAccounts/31.0a1 (Nightly)

kparlante: your stamp of approval?
Flags: needinfo?(kparlante)
Comment on attachment 8400103 [details] [review]
Link to Github pull-request: https://github.com/mozilla-services/android-sync/pull/444

Yes, it's a trivial fix. a=follow-up.
Attachment #8400103 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/d198b1b29ccb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Approved, thx
Flags: needinfo?(kparlante)
Product: Android Background Services → Firefox for Android
You need to log in before you can comment on or make changes to this bug.