Introduce policy Sync to customize sync state

[1rneh]

Exemplary policy setting:

{
  "policies": {
    "SyncSettings": {
      "SyncEnabled": true,
      "TypesEnabled": [
        "bookmarks",
        "passwords"
      ]
    }
  }
}

[github-actions]

(Automated Close) Please do not file pull requests here, see https://firefox-source-docs.mozilla.org/contributing/how_to_submit_a_patch.html

[gcp]

This can throw from here: https://searchfox.org/firefox-main/rev/150ca809c7243bd6e994f28cc4a3750430783364/services/sync/modules/service.sys.mjs#977

Not sure if it can reasonably happen in our setup, but it would mean all the other policy/settings enforcement code below is skipped. We might want to eat the error and rethrow after we did that?

[1rneh]

Would you suggest doing anything in Policies.sys.mjs with that error besides logging the failure? Logging we could also just do here. We could also send back an event inconsistency event to the console, but then Sync would be the only policy to inform the console about such a failure.

[1rneh]

As a first step I wrapped connectSync and disconnectSync in a try-catch block.

[gcp]

Looking at the callers, this seems to be more of an internal method that's called as part of a larger state change, so I think you want to do this block higher up to ensure consistency?

https://searchfox.org/firefox-main/rev/150ca809c7243bd6e994f28cc4a3750430783364/browser/base/content/browser-sync.js#2473 (okay, here it would only be false telemetry, so not that big of a deal)

https://searchfox.org/firefox-main/rev/150ca809c7243bd6e994f28cc4a3750430783364/services/sync/modules/SyncDisconnect.sys.mjs#135 (here you would still sign out, enable the service, ...so this usage worries me a bit)

https://searchfox.org/firefox-main/source/services/sync/modules/sync_auth.sys.mjs#213 (this seems harmless, i.e. dead code)

[1rneh]

We are actually guarding all entry points that trigger the callers:

• We hide fxa-menu-account-signout-button
• We hide fxaUnlinkButton in about:preferences#sync
• We hide the Disconnect... button in about:preferences#sync if sync state is enabled and locked by Sync policy
• We hide the Turn on syncing... button in about:preferences#sync if sync state is disabled locked by Sync policy

So if the sync state is locked, an update to the Sync policy is the only caller of Weave.startOver() and Weave.configure().

[1rneh]

I added a comment in startOver and configure to inform that the callers are guarded.

[gcp]

I'm not sure we should block startOver() but maybe rather modify its callers to no-op?

You could then maybe change this to assert that this is never called in the faulty state, that is, in Enterprise with changes blocked.

[Mossop]

From what I can tell this looks good but my knowledge of the sync service isn't great. Do we have some plans to add automated tests for this?

[Mossop]

An alternative here is to define this as normal and do the MOZ_ENTERPRISE check within the policy to make it non-functional in the non-enterprise case. That might make it easier to find this as it then appears in the normal alphabetical list of policy declarations. But I don't have a strong opinion, leave this as it is if you like

[1rneh]

The stretch goal is land this in m-c (Bug 2017722), so this should be temporary anyway.

[1rneh]

From what I can tell this looks good but my knowledge of the sync service isn't great. Do we have some plans to add automated tests for this?

I filed Bug 2017837 to add an end-to-end test for the SyncPolicy.