1556276 - Expose more FxA URLs: metrics-flow and legal

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect, P1)

Description

[Ryan Kelly [:rfkelly]]

I noticed that the STR for Bug 1555860 involved setting two prefs to point the browser at FxA staging environment:

To be able to see the about:welcome page on a version where this is not implemented
please go to about:config and set:
identity.fxaccounts.autoconfig.uri to https://accounts.stage.mozaws.net
browser.newtabpage.activity-stream.fxaccounts.endpoint to https://accounts.stage.mozaws.net

At first glance, it seems unlikely to me that we'd ever want newtab using a different accounts server than the rest of Firefox, and that setting the autoconfig URI should just make newtab do the right thing. But of course I could be missing something - I see in Bug 1471514 Comment 11 that this was deliberately made its own pref rather than re-using an existing one.

It feels like we should do something to prevent newtab using a different account service to the rest of the browser. Options could include:

• Add helper functions in FxA to generate the various URLs that the newtab page needs, rather than requiring it to construct its own URLs.
• Have the newtab page use an existing FxA pref to determine the right server root, such as identity.fxaccounts.remote.root.
• Have FxA's autoconfig logic set the pref used by newtab.

I listed them in my own naive order of preference, but I don't have much context on why the current code works the way it does, so my preferences could easily be wrong. :Mardak, :stomlinson, what do you think?

Comment 1

[Ed Lee :Mardak]

Yeah, the way about:welcome (old control and new trailhead) gets the pref is quite different from the rest of activity stream, so we briefly looked at changing it for trailhead but decided to just do the same as before to reduce risk.

I suppose we don't want to duplicate logic, e.g., check identity.fxaccounts.autoconfig.uri then fall back to identity.fxaccounts.remote.root which is probably why your first choice of an API is preferable. The main constraint is that about:welcome is one of the very first thing loaded for a new profile, and loading fxa code and/or awaiting for something async could delay the rest of the page from getting loaded.

Arguably, we could potentially refactor the page (ideally once we decide on a final about:welcome e.g., bug 1552280) so lazily use a FxA url. I believe the only uses are for the form submission endpoint as well as the flow request -- both should result in no user visible difference if delayed (although worst case is we delay so much that we don't get a flow response by the time the user submits).

Comment 2

[Ryan Kelly [:rfkelly]]

The main constraint is that about:welcome is one of the very first thing loaded for a new profile,
and loading fxa code and/or awaiting for something async could delay the rest of the page from getting loaded.

We could also consider factoring out a more standalone "fxa config helper" that you can load without having to initialize all of FxA and sync.

Comment 3

[Ed Lee :Mardak]

For the refactoring, one approach is from about:welcome trailhead react component, we add a hook on first mount to dispatch an action to the main process, e.g., FXA_ENDPOINT_REQUEST, so we only load the FxA module and call API (can be async) for about:welcome and not about:home/newtab.

This is in contrast to the current approach to read a special activity-stream pref once at startup (thus very slightly impacting all startup even those without about:welcome, but also not responding to url changes nor handling trailing slashes consistently bug 1554102).

Comment 4

[Shane Tomlinson [:stomlinson]]

:stomlinson, what do you think?

Yeah, I thought both this and about:newinstall.

The main constraint is that about:welcome is one of the very first thing loaded for a new profile, and loading fxa code and/or awaiting for something async could delay the rest of the page from getting loaded.

identity.fxaccounts.autoconfig.uri is not set by default, only if the user explicitly updates to point to a test server. I can imagine the default path still being fast - check whether that config value is empty and if so, use the default values for the rest of the services, no asynchronous operations needed.

Comment 5

[Ed Lee :Mardak]

rfkelly, is there a bug to expose a FxA jsm API to get identity.fxaccounts.autoconfig.uri || identity.fxaccounts.remote.root? Should that just be added as part of this bug and get reviewed by.. stomlinson ?

Adding dependency on bug 1560065 as it's unifying how browser.newtabpage.activity-stream.fxaccounts.endpoint is read and used across the various firstrun experiences.

Comment 6

[Ryan Kelly [:rfkelly]]

rfkelly, is there a bug to expose a FxA jsm API to get identity.fxaccounts.autoconfig.uri || identity.fxaccounts.remote.root?

There is not. We should definitely add such an API. Adding this as a blocker to Bug 1565960, which is our nascent metabug for "clean up FxAccounts.jsm API surface for public consumpion".

Should that just be added as part of this bug and get reviewed by.. stomlinson ?

Doing it as part of this bug sounds great, and :markh is probably the best person for detailed guidance and review. Mark, does that sound right?

Comment 7

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

CC Ed as he designed the most recent improvements to the autoconfig stuff.

(In reply to Ryan Kelly [:rfkelly] from comment #6)

rfkelly, is there a bug to expose a FxA jsm API to get identity.fxaccounts.autoconfig.uri || identity.fxaccounts.remote.root?

There is not. We should definitely add such an API. Adding this as a blocker to Bug 1565960, which is our nascent metabug for "clean up FxAccounts.jsm API surface for public consumpion".

Should that just be added as part of this bug and get reviewed by.. stomlinson ?

Doing it as part of this bug sounds great, and :markh is probably the best person for detailed guidance and review. Mark, does that sound right?

I'm not sure what this API should look like though - for clarity (and to expand on what Shane said):

• identity.fxaccounts.autoconfig.uri is not set by default. If it is set, the configuration values that end-point returns could be on a completely different server - for example, while it probably doesn't make much sense, there's no reason a custom autoconfig URL couldn't return our FxA servers. Further, this pref only makes sense to look at if there's no user signed in.

• identity.fxaccounts.remote.root has a default value pointing at our FxA servers - but if no user is signed in, it will be overwritten if identity.fxaccounts.autoconfig.uri is set. IOW, this only makes sense to look at when a user is signed in.

It seems you probably only care about this when a user is signed in, right? If so, it probably makes sense to just add a promiseRootURI() method to FxAccountsConfig.jsm, which you'd access via fxAccounts.config.promiseRootURI()

However, this begs the question of exactly how this would be used - for example, I notice that AS links to things like /legal/terms and /legal/privacy - which about:preferences#sync also links to - and sadly, these are done in a different way via yet different preferences sob - so really, I think we should consider adding promiseLegalTermsURI() and promiseLegalPrivacyURI() methods instead and ensure they are used consistently. I recall some earlier discussion deciding that these URLs should always point to our notices, even if a custom server is used, so adding these methods to FxAccountsConfig.jsm would ensure the same policy is used everywhere.

In what other cases would the root URI be necessary? ie, I'm trying to work out if exposing the 'root' URI is a future footgun and we should just add multiple methods which target a specific end-point to discourage consumers from building their own URLs at all? Regardless, adding new entries is very easy (but a complication is that they all return promises, whereas prefs are sync)

Ed/Ryan, wdyt?

Comment 8

[Ryan Kelly [:rfkelly]]

we should just add multiple methods which target a specific end-point to discourage consumers from building their own URLs at all?

I support this FWIW; see also recent work in the FxA rust component to add things like "get_account_management_url".

Comment 9

[Ed Lee :Mardak]

The specific urls that get used for what from first run on a default profile with the common case of users not logged in yet.

ENDPOINT = https://accounts.firefox.com

• "$ENDPOINT" is <form> action submitting various get params including email
• "$ENDPOINT/metrics-flow" is fetched with various params then response passed back as part of the submit url
• "$ENDPOINT/legal/terms" and "$ENDPOINT/legal/privacy" are linked

I do see Trailhead.jsx has direct accounts.firefox.com links no $ENDPOINT while StartupOverlay.jsx does use $ENDPOINT. I also see OnboardingMessageProvider.jsm using direct accounts.firefox.com for the onboarding card's url (where for reference, other urls hardcode blog links or home pages, e.g., monitor.firefox.com -- so maybe these don't have to be $ENDPOINT-ified ??)

At least during Trailhead, there were issues related to test fxa server not accepting extra-slashed paths, e.g., test.fxa.com//metrics-flow when QA steps said to set the pref to "test.fxa.com/" although production server happily handles the extra slash. Another issue was that changing browser.newtabpage.activity-stream.fxaccounts.endpoint required a restart. Although these 2 issues are probably not necessary to fix here.

For the uses on about:welcome, async/promises are fine as we can render the html and switch to actual endpoint before user interaction.

Comment 10

[Edouard Oger [:eoger]]

Attachment: Bug 1556276 p1 - Allow extraParams to be passed when constructing FxA URLs.

Comment 11

[Edouard Oger [:eoger]]

Attachment: Bug 1556276 p2 - Build metrics-flow and legal FxA URLs.

Comment 12

[Ed Lee :Mardak]

k88hudson, ha. Looks like we'll re-add the FxAccountsConfig that was just removed ;) https://github.com/mozilla/activity-stream/pull/5185/files#r305060589

We'll probably want to have a message to have some feed that makes multiple requests and passes them back to content state, e.g.,:

Promise.all([
  FxAccountsConfig.promiseEmailURI(…),
  FxAccountsConfig.promiseMetricsFlowURI,
  FxAccountsConfig.promiseLegalTermsURI,
  FxAccountsConfig.promiseLegalPrivacyURI,
])
dispatch(OnlyToOneContent(…))

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f861296caf4
p1 - Allow extraParams to be passed when constructing FxA URLs. r=markh
https://hg.mozilla.org/integration/autoland/rev/98bedd1bb061
p2 - Build metrics-flow and legal FxA URLs. r=markh

Comment 14

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/0f861296caf4
https://hg.mozilla.org/mozilla-central/rev/98bedd1bb061

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Is this something we need to keep on the radar for uplift or can this fix ride Fx70 to release?

Comment 16

[Edouard Oger [:eoger]]

How soon do you need this?

Comment 17

[Ed Lee :Mardak]

We haven't fixed bug 1568949 yet, and I don't think we'll uplift. These issues were triggered by testing but not so much regular users.