Set newly introduced fxa/sync related prefs based on fxa autoconfig uri

RESOLVED FIXED in Firefox 60

Status

()

defect
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: hectorz, Assigned: eoger)

Tracking

unspecified
Firefox 60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(3 attachments)

Reporter

Description

a year ago
"identity.fxaccounts.autoconfig.uri" was introduced in bug 1249520 to make it easier to connect to alternative fxa/sync services.

In bug 1356112, bug 1411714 and bug 1418466, new fxa/sync related prefs were introduced, but the corresponding updates to "/services/fxaccounts/FxAccountsConfig.jsm" only cover users not signed into sync yet.

For users already signed into alternative fxa/sync services, such new prefs would stay at the default values, which won't work.
Just to make sure I'm understanding, this bug is more about when we read the data from /.well-known/fxa-client-configuration and not that we're missing some preferences?
Flags: needinfo?(bzhao)
(In reply to Thom Chiovoloni [:tcsc] from comment #1)
> Just to make sure I'm understanding, this bug is more about when we read the
> data from /.well-known/fxa-client-configuration and not that we're missing
> some preferences?

Yes - there are now a number of preferences that would be impacted by using a custom server, but which aren't set using that endpoint. Thus, those prefs will remain incorrect for self-hosters.

It *should* be possible to fix this without changing the server given we set these prefs based on just a "root url" - we should include a test that attempts to prevent this happening in the future (eg, the test could apply based on a synthesized config payload, then enumerate all identity.* prefs and check that accounts.firefox.com doesn't appear in any, which would then fail when new prefs are added)
Flags: needinfo?(bzhao)
Priority: -- → P2
Assignee

Comment 3

a year ago
I would like to see only one canonical pref describing the FxA root, and construct all "remote" URLs dynamically using that pref.
We already have a bunch of FxAccounts#promise[...]URL methods: it wouldn't be to hard to change uses of these preferences.
Here what I'm thinking:

+ identity.fxaccounts.remote.root = "https://accounts.firefox.com/" (replaced by FxAccountsConfig if necessary)
- identity.fxaccounts.remote.webchannel.uri
- identity.fxaccounts.remote.email.uri
- identity.fxaccounts.remote.connectdevice.uri
- identity.fxaccounts.remote.force_auth.uri
- identity.fxaccounts.settings.devices.uri
- identity.fxaccounts.settings.uri
- identity.fxaccounts.remote.signin.uri
- identity.fxaccounts.remote.signup.uri

As you can see we have a LOT of different URLs, and I don't see why we would stop adding new ones.
(In reply to Edouard Oger [:eoger] from comment #3)
> I would like to see only one canonical pref describing the FxA root, and
> construct all "remote" URLs dynamically using that pref.

+1 - it makes no sense to duplicate the origin in those prefs.
Assignee: nobody → eoger
Assignee

Comment 5

a year ago
We also need to make changes to the Activity Stream system addon.
Assignee

Updated

a year ago
Priority: P2 → P1
Comment hidden (mozreview-request)
Assignee

Comment 7

a year ago
Quick note:
I spoke with Ursula from the Activity Stream team this afternoon.
It turns out that I can make these changes directly on m-c, but I will have to backport them on the activity-stream GH repo in any case.

Comment 8

a year ago
mozreview-review
Comment on attachment 8946906 [details]
Bug 1427674 - Unify FxA content server URL preferences.

https://reviewboard.mozilla.org/r/216762/#review222624

Looks great, thanks!

::: services/fxaccounts/FxAccounts.jsm:418
(Diff revision 1)
>        });
>      }
>      return this._profile;
>    },
>  
> +  get config() {

out of interest, is there any reason this can't just be on the prototype?

::: services/fxaccounts/FxAccountsConfig.jsm:43
(Diff revision 1)
> +    await this.ensureConfigured();
> +    return this._buildURL("signup", {entrypoint});
> +  },
>  
> -  async _getPrefURL(prefName) {
> +  async promiseSignInURI(entrypoint) {
>      await this.ensureConfigured();

Why do some of these call ensureConfigured but others don't? It seems like that call could just be added to _buildURL?

::: services/fxaccounts/FxAccountsConfig.jsm:58
(Diff revision 1)
> +    await this.ensureConfigured();
> +    return this._buildURL("force_auth", {entrypoint}, true);
>    },
>  
> -  // Returns a promise that resolves with the URI of the remote UI flows.
> -  promiseAccountsSignInURI() {
> +  async promiseManageURI(entrypoint) {
> +    return this._buildURL("settings", {entrypoint}, true);

why don't the rest of these call ensureConfigured()? ISTM that every function should (which also implies it could be in `_buildURL`?

::: services/fxaccounts/FxAccountsConfig.jsm:138
(Diff revision 1)
>      if (!isSignedIn) {
>        await this.fetchConfigURLs();
>      }
>    },
>  
> +  async tryPrefsMigration() {

let's comment this a little better to point at this bug and make it clear that in (say) 65 or later what parts we can nuke.
Attachment #8946906 - Flags: review?(markh) → review+
Reporter

Comment 9

a year ago
Should `services.sync.fxa.{privacyURL,termsURL}` be covered in this bug?
Assignee

Comment 10

a year ago
I assumed that these 2 URLs were in a different namespace for legal reasons. Thoughts Mark?
Comment hidden (mozreview-request)
(In reply to Edouard Oger [:eoger] from comment #10)
> I assumed that these 2 URLs were in a different namespace for legal reasons.
> Thoughts Mark?

Yeah, I think that's risky as it is possible to provide a custom config while still using moz services. If anyone cares enough, we should do it in a different bug with legal review.

Comment 13

a year ago
mozreview-review
Comment on attachment 8946906 [details]
Bug 1427674 - Unify FxA content server URL preferences.

https://reviewboard.mozilla.org/r/216762/#review224312

This looks good and largely mechanical so I don't have a lot of comments, thanks!
Attachment #8946906 - Flags: review?(tchiovoloni) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 4216f83a08431d04b3f928ad9848c1afe6996f79 -d 5d5ebee61687: rebasing 446315:4216f83a0843 "Bug 1427674 - Unify FxA content server URL preferences. r=markh,tcsc" (tip)
merging browser/app/profile/firefox.js
merging browser/components/nsBrowserGlue.js
merging browser/components/preferences/in-content/main.js
merging browser/components/uitour/UITour.jsm
merging browser/extensions/activity-stream/lib/SnippetsFeed.jsm
merging services/fxaccounts/FxAccounts.jsm
merging services/fxaccounts/FxAccountsConfig.jsm
merging services/fxaccounts/FxAccountsWebChannel.jsm
warning: conflicts while merging browser/components/nsBrowserGlue.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)

Comment 16

a year ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b014201f939f
Unify FxA content server URL preferences. r=markh,tcsc
https://hg.mozilla.org/mozilla-central/rev/b014201f939f
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60

Comment 19

a year ago
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/a77359dcf84a51216cdb80122bd3e3250f014aec
fix(snippets): Port Bug 1427674 - Use the correct FxAccounts#promiseSignUpURI method (#3957)
You need to log in before you can comment on or make changes to this bug.