Closed Bug 1427674 Opened 3 years ago Closed 2 years ago

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

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: hectorz, Assigned: eoger)

References

Details

Attachments

(3 files)

"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
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
We also need to make changes to the Activity Stream system addon.
Priority: P2 → P1
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 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+
Should `services.sync.fxa.{privacyURL,termsURL}` be covered in this bug?
I assumed that these 2 URLs were in a different namespace for legal reasons. Thoughts Mark?
(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 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)
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
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
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.