Closed Bug 1434706 Opened 7 years ago Closed 6 years ago

Add a preference to disable FxA/Sync and hide their UI

Categories

(Firefox :: Sync, enhancement, P1)

Product:
Firefox
Component:
Sync
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: arthur, Assigned: eoger)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor 16488][tor 22564])

Attachments

(1 file)

Bug 1434706 - Add identity.fxaccounts.enabled pref to disable Sync and FxA.
59 bytes, text/x-review-board-request
markh
: review+
Details 
In Tor Browser we're hiding the Sync UI, partially, and intend to fully hide it. The two tickets are
https://trac.torproject.org/16488
https://trac.torproject.org/22564

Once we have finalized our patch, we'd like to propose uplifting it behind a pref (services.sync.ui.hidden).
This sounds like a valid use case.
However, I'm slightly concerned that some users might be syncing without knowing it since the UI will be hidden.

We should resurrect the |services.sync.enabled| pref to both hide the UI AND disable completely the Sync service.
I'd be happy to mentor you through this or take over the patch and add this myself.

Thoughts Mark?
I'm inclined to agree that a pref to just hide the UI without also disabling the service might be a footgun.
Priority: -- → P3
Perhaps a triple state?
0 - do nothing
1 - hide the UI **only** if sync is not used (for FF)
2 - hide the UI **and** disable sync (TBB, PB Mode trials)
(In reply to Simon Mainey from comment #3)
> 1 - hide the UI **only** if sync is not used (for FF)
> 2 - hide the UI **and** disable sync (TBB, PB Mode trials)

I don't understand the distinction between these 2 - why would (1) want to have the sync UI disabled but leave sync itself enabled? ISTM that the UI state and the state of the feature itself should always be the same or we will be left with possible states that make no sense.
(In reply to Mark Hammond [:markh] from comment #4)
> I don't understand the distinction between these 2 - why would (1) want to
> have the sync UI disabled but leave sync itself enabled? ISTM that the UI
> state and the state of the feature itself should always be the same or we
> will be left with possible states that make no sense.

It wouldn't. (1) would only hide the ui **IF** sync is **NOT** used

from comment 1: "However, I'm slightly concerned that some users might be syncing without knowing it since the UI will be hidden"
See Also: → 1429151
Comment on attachment 8951170 [details]
Bug 1434706 - Add identity.fxaccounts.enabled pref to disable Sync and FxA.

This patch sprinkles pref checks all around Sync and the UI.
Note that this pref is only guaranteed to work after a browser restart, since we neuter Sync before it initializes and hide some UI elements on startup.
Attachment #8951170 - Flags: feedback?(markh)
Comment on attachment 8951170 [details]
Bug 1434706 - Add identity.fxaccounts.enabled pref to disable Sync and FxA.

https://reviewboard.mozilla.org/r/220434/#review228072

Thanks for taking this on.

I'm not convinced that a pref starting with identity.fxaccounts actually makes sense given how this patch is implemented - there's nothing here that actually neuters FxAccounts itself - IIUC, it would still be quite possible to have an FxA user signed in if code tried to do that.

I think we should either name the preference "sync", or have FxA itself check this pref and always return null for a signedInUser (possibly throwing any existing account info away at the same time) and throw on any attempt at signing in.

::: browser/base/content/browser-sync.js:134
(Diff revision 1)
>      // We start with every broadcasters hidden, so that we don't need to init
>      // the sync UI on windows like pageInfo.xul (see bug 1384856).
>      let setupBroadcaster = document.getElementById("sync-setup-state");
>      setupBroadcaster.hidden = false;
>  
> +    if (!this.SYNC_ENABLED) {

It seems like this could be moved up a few lines as otherwise we are showing items them immediately re-hiding them?

::: browser/base/content/browser-sync.js:670
(Diff revision 1)
>        }
>      }
>    },
>  
> +  onSyncDisabled() {
> +    const toHide = [...document.querySelectorAll(".sync-menubar-item"),

can we maybe rename "sync-menubar-item" to, say "sync-ui-item" and add that class to the other items, including the sep?

::: browser/components/preferences/in-content/main.js:467
(Diff revision 1)
>        setEventListener("getStarted", "click", gMainPane.onGetStarted);
>  
>        OS.File.stat(ignoreSeparateProfile).then(() => separateProfileModeCheckbox.checked = false,
>          () => separateProfileModeCheckbox.checked = true);
>  
> +      if (!Services.prefs.getBoolPref("identity.fxaccounts.enabled")) {

we really should file a followup bug for this as the new "per channel profiles" effort seems to make this dev-edition specific code either redundant, or should be generic across all channels.

::: browser/components/preferences/in-content/main.xul:36
(Diff revision 1)
>  
>  #ifdef MOZ_DEV_EDITION
>    <vbox id="separateProfileBox">
>      <checkbox id="separateProfileMode"
>                label="&separateProfileMode.label;"/>
> -    <hbox align="center" class="indent">
> +    <hbox id="sync-dev-edition-tip" align="center" class="indent">

nit: "tip" doesn't seem the correct term here - "tip" implies "leaf" to me. -root or -parent seem more appropriate

::: services/sync/Weave.js:126
(Diff revision 1)
>     *
>     * It does *not* perform a robust check to see if the client is working.
>     * For that, you'll want to check Weave.Status.checkSetup().
>     */
>    get enabled() {
> -    return !!syncUsername;
> +    return !!syncUsername && Services.prefs.getBoolPref("identity.fxaccounts.enabled");

I'm wondering if the new preference is better names as ".available" rather than ".enabled" given we already have a broad concept of "enabled" meaning "configured"?

::: services/sync/modules/browserid_identity.js:395
(Diff revision 1)
>        this.resetCredentials();
>        this._ensureValidToken().catch(err =>
>          this._log.error("Error while fetching a new token", err));
>        break;
> +    case "nsPref:changed":
> +      if (!Services.prefs.getBoolPref("identity.fxaccounts.enabled")) {

You document the pref as requiring a restart, although here you are attempting to handle it changing on the fly. We might as well take one position or the other.
Arthur, are you comfortable with the general approach being taken here?
Flags: needinfo?(arthuredelstein)
Yes, this looks like a good approach to me. Thanks Edouard and Mark!
Flags: needinfo?(arthuredelstein)
Comment on attachment 8951170 [details]
Bug 1434706 - Add identity.fxaccounts.enabled pref to disable Sync and FxA.

https://reviewboard.mozilla.org/r/220434/#review228072

> we really should file a followup bug for this as the new "per channel profiles" effort seems to make this dev-edition specific code either redundant, or should be generic across all channels.

Bug 1440201 filled.

> I'm wondering if the new preference is better names as ".available" rather than ".enabled" given we already have a broad concept of "enabled" meaning "configured"?

I think this is fine, especially since "we" want to move away from prefs storing state.
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: P3 → P1
Summary: pref to hide Sync UI → Add a preference to disable FxA/Sync and hide their UI
Comment on attachment 8951170 [details]
Bug 1434706 - Add identity.fxaccounts.enabled pref to disable Sync and FxA.

https://reviewboard.mozilla.org/r/220434/#review228488

Looks great, thanks.
Attachment #8951170 - Flags: review?(markh) → review+
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6e3058771f18
Add identity.fxaccounts.enabled pref to disable Sync and FxA. r=markh
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c3b5aa75b8ac
Backed out changeset 6e3058771f18 for bc6 and X failures in browser/base/content/test/general/browser_contextmenu_input.js and services/fxaccounts/tests/xpcshell/test_accounts.js respectively on a CLOSED TREE
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a2147499c437
Add identity.fxaccounts.enabled pref to disable Sync and FxA. r=markh
Backed out changeset for frequent mochitest browser chrome failures on browser_contextmenu_sendpage.js

Treeherder failures: https://goo.gl/DusGyQ https://goo.gl/WhZJWK https://goo.gl/o3vCHz

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=163984095&repo=autoland&lineNumber=4423

Backout link: https://hg.mozilla.org/integration/autoland/rev/7eddfdfce55f65d089189c94d89d146a91c54ea8
Flags: needinfo?(eoger)
Initially made this intermittent bug https://bugzilla.mozilla.org/show_bug.cgi?id=1440763 for those failures but then seeing they appear very often decided to back it out. You can close that intermittent bug if you fix this issue in this bug. Thank you.
Thanks!
Flags: needinfo?(eoger)
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/22901b9f9199
Add identity.fxaccounts.enabled pref to disable Sync and FxA. r=markh
Backed out for browser-chrome failures browser_contextmenu_sendtab.js

backout: https://hg.mozilla.org/integration/autoland/rev/1c46e9a24298cd10c3ecc19d05129f3d6bead7dd

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=22901b9f91994efdd039dc1a2d0ae0d0cd5f09c3&selectedJob=164398303

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=164398303&repo=autoland&lineNumber=5154

[task 2018-02-26T18:54:15.093Z] 18:54:15     INFO - TEST-PASS | browser/base/content/test/sync/browser_contextmenu_sendtab.js | TabContextMenu context is the expected tab - 
[task 2018-02-26T18:54:15.096Z] 18:54:15     INFO - Buffered messages finished
[task 2018-02-26T18:54:15.098Z] 18:54:15     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/sync/browser_contextmenu_sendtab.js | Send tab to device is shown - Got true, expected false
[task 2018-02-26T18:54:15.100Z] 18:54:15     INFO - Stack trace:
[task 2018-02-26T18:54:15.102Z] 18:54:15     INFO -     chrome://mochikit/content/browser-test.js:test_is:1271
[task 2018-02-26T18:54:15.103Z] 18:54:15     INFO -     chrome://mochitests/content/browser/browser/base/content/test/sync/browser_contextmenu_sendtab.js:test_tab_contextmenu:38
[task 2018-02-26T18:54:15.105Z] 18:54:15     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:1058:9
[task 2018-02-26T18:54:15.105Z] 18:54:15     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:958:9
[task 2018-02-26T18:54:15.106Z] 18:54:15     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
[task 2018-02-26T18:54:15.110Z] 18:54:15     INFO - TEST-PASS | browser/base/content/test/sync/browser_contextmenu_sendtab.js | Send tab to device is enabled - 
[task 2018-02-26T18:54:15.117Z] 18:54:15     INFO - Leaving test bound test_tab_contextmenu
[task 2018-02-26T18:54:15.118Z] 18:54:15     INFO - Entering test bound test_tab_contextmenu_unconfigured
[task 2018-02-26T18:54:15.119Z] 18:54:15     INFO - GECKO(2021) | ++DOCSHELL 0x7f0309c8f800 == 1 [pid = 2134] [id = {a062bd27-eb0a-4f24-be62-c2a14ec9dc15}]
[task 2018-02-26T18:54:15.120Z] 18:54:15     INFO - GECKO(2021) | ++DOMWINDOW == 1 (0x7f0309c20800) [pid = 2134] [serial = 1] [outer = (nil)]
[task 2018-02-26T18:54:15.122Z] 18:54:15     INFO - TEST-PASS | browser/base/content/test/sync/browser_contextmenu_sendtab.js | TabContextMenu context is the expected tab - 
[task 2018-02-26T18:54:15.123Z] 18:54:15     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-02-26T18:54:15.125Z] 18:54:15     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/sync/browser_contextmenu_sendtab.js | Send tab to device is shown - Got true, expected false
Flags: needinfo?(eoger)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a270fb02c677fe2092f6c0897e103b1e85d425ae

Try looks green, let's hope this will be the last one :)
Flags: needinfo?(eoger)
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/109cd0a34ffe
Add identity.fxaccounts.enabled pref to disable Sync and FxA. r=markh
https://hg.mozilla.org/mozilla-central/rev/109cd0a34ffe
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1441965
Depends on: 1443593
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: