Closed
Bug 1427674
Opened 7 years ago
Closed 7 years ago
Set newly introduced fxa/sync related prefs based on fxa autoconfig uri
Categories
(Firefox :: Sync, defect, P1)
Firefox
Sync
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.
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
(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)
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Comment 3•7 years 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.
Comment 4•7 years ago
|
||
(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.
Updated•7 years ago
|
Assignee: nobody → eoger
Assignee | ||
Comment 5•7 years ago
|
||
We also need to make changes to the Activity Stream system addon.
Assignee | ||
Updated•7 years ago
|
Priority: P2 → P1
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years 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•7 years 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•7 years ago
|
||
Should `services.sync.fxa.{privacyURL,termsURL}` be covered in this bug?
Assignee | ||
Comment 10•7 years ago
|
||
I assumed that these 2 URLs were in a different namespace for legal reasons. Thoughts Mark?
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
(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•7 years 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+
Comment 14•7 years ago
|
||
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•7 years ago
|
||
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b014201f939f
Unify FxA content server URL preferences. r=markh,tcsc
Comment 17•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Comment 18•7 years ago
|
||
Comment 19•7 years 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.
Description
•