diff --git a/modules/betterbird/README.md b/modules/betterbird/README.md index cc54075f..13bf2484 100644 --- a/modules/betterbird/README.md +++ b/modules/betterbird/README.md @@ -117,8 +117,17 @@ the layers, and flags **drift**: a saved value that beats the module's default w `betterbird_check` reports it. There are two remedies: - `betterbird_config_apply` with `apply` takes the drifting values out of `prefs.js`, so the module's value - holds at the next start. It acts only while Betterbird is quit, because Betterbird rewrites `prefs.js` when it - quits, and it says what it cleared. Without `apply` it lists what it would clear. + holds at the next start. It acts only while Betterbird is quit (no main process, and no `lock` in the profile), + because Betterbird rewrites `prefs.js` when it quits, and it says what it cleared. Without `apply` it lists + what it would clear. +- **The theme is the exception.** Betterbird keeps the active theme in its add-on database (`extensions.json`), + and `extensions.activeThemeID` only mirrors it. So the tools read the theme from the add-on database, and the + module's value is `default-theme@mozilla.org`. Another theme is drift whose remedy is to enable the default + theme in the add-on manager. `betterbird_config_apply` never clears it, and `keep` refuses it. + +**Never an account.** A name the tools withhold (a credential's, an account's, a server's), or an account's own +preference, is never managed, even when the defaults file lists it. `betterbird_config` leaves it out and names it +in `refused`, `betterbird_config_apply` never clears it, and `betterbird_check` says to take it out of the file. - `betterbird_config` with `keep` proposes, through the controller's settings verb, a node layer of `prefs` that holds the profile's value for this machine. The operator approves it on a channel that proves who answers. The tool never writes `user.js` itself. The setting is one string, so the layer carries what `prefs` says on diff --git a/modules/betterbird/cmd/betterbird-tools/betterbird.go b/modules/betterbird/cmd/betterbird-tools/betterbird.go index 6bd15030..3cbc09e0 100644 --- a/modules/betterbird/cmd/betterbird-tools/betterbird.go +++ b/modules/betterbird/cmd/betterbird-tools/betterbird.go @@ -908,11 +908,27 @@ func (m *Machine) Check() (CheckAnswer, error) { if in.From == "the mesh" && !exists(m.path(meshDefaults)) { add("the module's defaults are not placed ("+meshDefaults+")", "push the module to this node") } - if c, _, err := m.config(); err == nil && len(c.Drifting) > 0 { - add(fmt.Sprintf("%d managed preference(s) drift from the module's defaults: the profile saved its own value and no "+ - "node layer says so (%s)", len(c.Drifting), strings.Join(c.Drifting, ", ")), - "betterbird_config shows each; to keep a value on this node, betterbird_config with keep proposes a node "+ - "layer; otherwise quit Betterbird and call betterbird_config_apply with apply") + if c, _, err := m.config(); err == nil { + var cleared []string + for _, cp := range c.Prefs { + switch { + case cp.Drift == "": + case cp.Remedy != "": + add(cp.Name+" drifts from the module's: "+cp.Drift, cp.Remedy) + default: + cleared = append(cleared, cp.Name) + } + } + if len(cleared) > 0 { + add(fmt.Sprintf("%d managed preference(s) drift from the module's defaults: the profile saved its own value and no "+ + "node layer says so (%s)", len(cleared), strings.Join(cleared, ", ")), + "betterbird_config shows each; to keep a value on this node, betterbird_config with keep proposes a node "+ + "layer; otherwise quit Betterbird and call betterbird_config_apply with apply") + } + if len(c.Refused) > 0 { + add(fmt.Sprintf("the module's defaults list %d preference(s) the tools never manage (%s)", len(c.Refused), strings.Join(c.Refused, "; ")), + "take them out of install/defaults-pref/mesh-defaults.js in a reviewed catalogue change: an account is declared in the setting accounts") + } } if why := m.defaultClientAsk(profilePath); why != "" && in.From != "not installed" { add("Betterbird asks at its start to be the default mail client: "+why, diff --git a/modules/betterbird/cmd/betterbird-tools/config.go b/modules/betterbird/cmd/betterbird-tools/config.go index 94d45e81..eb6a0651 100644 --- a/modules/betterbird/cmd/betterbird-tools/config.go +++ b/modules/betterbird/cmd/betterbird-tools/config.go @@ -48,6 +48,71 @@ var ( keepsDefault = regexp.MustCompile(`^//\s*keeps Betterbird's default:\s*([A-Za-z0-9_.@{}\-]+)\s*$`) ) +// builtIn are the defaults compiled into Betterbird of the preferences the module keeps at Betterbird's default, +// which no defaults file gives: so a value saved equal to them is not drift. +var builtIn = map[string]string{ + themePref: `"` + defaultTheme + `"`, + "browser.display.use_system_colors": "false", + "layout.css.always_underline_links": "false", +} + +const ( + themePref = "extensions.activeThemeID" + defaultTheme = "default-theme@mozilla.org" + // themeRemedy is how the theme comes back: Betterbird keeps the active theme in its add-on database + // (extensions.json), and the preference only mirrors it (XPIDatabase addonChanged), so clearing it changes + // nothing and betterbird_config_apply never touches it. + themeRemedy = "enable the default theme in Betterbird's add-on manager (Add-ons and Themes, Themes): the active " + + "theme is kept in the add-on database, and its preference only mirrors it, so clearing prefs.js would not change it" +) + +// activeTheme is the theme the profile's add-on database marks active, or "" when it cannot be read. +func activeTheme(profile string) string { + raw, err := readBounded(filepath.Join(profile, "extensions.json")) + if err != nil { + return "" + } + var ext struct { + Addons []struct { + ID string `json:"id"` + Type string `json:"type"` + Active bool `json:"active"` + } `json:"addons"` + } + if json.Unmarshal(raw, &ext) != nil { + return "" + } + for _, a := range ext.Addons { + if a.Type == "theme" && a.Active { + return a.ID + } + } + return "" +} + +// unmanageable says why a name in the module's defaults is never managed by these tools, or "": a credential's, +// an account's or a server's preference, which betterbird_config never answers and betterbird_config_apply never +// clears, whatever the defaults file lists. +func unmanageable(name string) string { + if why := withheld(name); why != "" { + return why + } + for _, p := range accountOwned { + if strings.HasPrefix(name, p) { + return "an account's preference, the setting accounts' alone" + } + } + return "" +} + +// answer is a value as these tools answer it: masked, but for the theme's id. +func (m *Machine) answer(name, v string) string { + if allowedExtensions[name] { + return v + } + return m.maskValue(v) +} + // nodeName is the machine this runtime serves, as the mesh names it: a variable so a test answers. var nodeName = func() string { return os.Getenv("MESH_NODE") } @@ -269,6 +334,8 @@ type ConfigPref struct { From string `json:"effective_from"` // Drift says how the profile's own value beats the module's without a node layer saying so. Drift string `json:"drift,omitempty"` + // Remedy is how drift that clearing prefs.js cannot fix is fixed (the theme). + Remedy string `json:"remedy,omitempty"` // module is the value the module means: its default, or Betterbird's. module string } @@ -280,6 +347,8 @@ type ConfigAnswer struct { Prefs []ConfigPref `json:"prefs"` // Drifting are the names that drift. Drifting []string `json:"drifting"` + // Refused are names the module's defaults list that the tools never manage: an account's or a credential's. + Refused []string `json:"refused,omitempty"` // Proposed is the controller's answer to a node layer proposed with keep. Proposed string `json:"proposed,omitempty"` Note string `json:"note,omitempty"` @@ -320,11 +389,23 @@ func (m *Machine) config() (ConfigAnswer, Profile, error) { names = append(names, n) } } + theme := activeTheme(p.path) for _, n := range names { + if why := unmanageable(n); why != "" { + a.Refused = append(a.Refused, n+": "+why) + continue + } c := ConfigPref{Name: n, NodeLayer: node[n], Profile: saved[n]} + if n == themePref && theme != "" { + // The add-on database says which theme is active; the preference only mirrors it. + c.Profile = quoted(theme) + } bb, hasBB := own[n] if hasBB { c.BetterbirdDefault = bb.Value + } else if v, ok := builtIn[n]; ok { + c.BetterbirdDefault = v + bb.From = "Betterbird's built-in default" } switch v, isLocked := locked[n]; { case isLocked: @@ -351,9 +432,12 @@ func (m *Machine) config() (ConfigAnswer, Profile, error) { } if why := drift(c); why != "" { c.Drift = why + if n == themePref { + c.Remedy = themeRemedy + } a.Drifting = append(a.Drifting, n) } - c.NodeLayer, c.Profile, c.Effective = m.maskValue(c.NodeLayer), m.maskValue(c.Profile), m.maskValue(c.Effective) + c.NodeLayer, c.Profile, c.Effective = m.answer(n, c.NodeLayer), m.answer(n, c.Profile), m.answer(n, c.Effective) a.Prefs = append(a.Prefs, c) } return a, p, nil @@ -369,7 +453,11 @@ func drift(c ConfigPref) string { if want == "" { want = "Betterbird's built-in default" } - return fmt.Sprintf("the profile's prefs.js holds %s, which beats the module's %s, and no node layer says so", c.Profile, want) + where := "the profile's prefs.js holds" + if c.Name == themePref { + where = "the profile's active theme is" + } + return fmt.Sprintf("%s %s, which beats the module's %s, and no node layer says so", where, c.Profile, want) } // Config is what betterbird_config answers; with keep it proposes a node layer of the setting prefs that holds @@ -385,20 +473,12 @@ func (m *Machine) Config(keep []string) (ConfigAnswer, error) { if len(keep) == 0 { return a, nil } - node := nodeName() - if node == "" { - return a, errors.New("nothing was proposed: this runtime does not say which machine it serves (MESH_NODE)") - } by := map[string]ConfigPref{} for _, c := range a.Prefs { by[c.Name] = c } prefsRaw, _ := readBounded(filepath.Join(p.path, "prefs.js")) saved := readPrefs(prefsRaw) - declaredRaw, err := readBounded(m.home(declared)) - if err != nil { - return a, fmt.Errorf("nothing was proposed: the module's user.js is not on this machine (~/%s)", declared) - } kept := map[string]string{} for _, n := range keep { c, managed := by[n] @@ -407,11 +487,21 @@ func (m *Machine) Config(keep []string) (ConfigAnswer, error) { return a, fmt.Errorf("nothing was proposed: %s is not a preference the module manages (betterbird_config lists them)", n) case c.Locked != "": return a, fmt.Errorf("nothing was proposed: %s is locked by the module's policy; a lock changes only in the catalogue", n) + case n == themePref: + return a, fmt.Errorf("nothing was proposed: a node layer cannot choose the theme; the add-on manager does (%s)", themeRemedy) case saved[n] == "": return a, fmt.Errorf("nothing was proposed: the profile saved no value of its own for %s", n) } kept[n] = saved[n] } + declaredRaw, err := readBounded(m.home(declared)) + if err != nil { + return a, fmt.Errorf("nothing was proposed: the module's user.js is not on this machine (~/%s)", declared) + } + node := nodeName() + if node == "" { + return a, errors.New("nothing was proposed: this runtime does not say which machine it serves (MESH_NODE)") + } // The setting is one string, so a node layer replaces it whole: it carries what the setting says on this // machine today, with the kept values in place of any the same names had. var parts []string @@ -464,7 +554,9 @@ type ConfigApplyAnswer struct { Profile string `json:"profile"` Applied bool `json:"applied"` Clears []Cleared `json:"clears"` - Note string `json:"note"` + // NotCleared is drift that clearing prefs.js cannot fix, each with how it is fixed. + NotCleared []string `json:"not_cleared,omitempty"` + Note string `json:"note"` } // ConfigApply takes the drifting values out of the profile's prefs.js, so the module's value holds at the next @@ -481,9 +573,13 @@ func (m *Machine) ConfigApply(apply bool) (ConfigApplyAnswer, error) { } drifting := map[string]bool{} for _, cp := range c.Prefs { + if cp.Drift != "" && cp.Remedy != "" { + a.NotCleared = append(a.NotCleared, cp.Name+": "+cp.Remedy) + continue + } if cp.Drift != "" { drifting[cp.Name] = true - now := m.maskValue(cp.module) + now := m.answer(cp.Name, cp.module) if now == "" { now = "Betterbird's built-in default" } @@ -497,8 +593,13 @@ func (m *Machine) ConfigApply(apply bool) (ConfigApplyAnswer, error) { if ps := m.mainProcs(); len(ps) > 0 { return a, fmt.Errorf("Betterbird is running (pid %d): it rewrites prefs.js when it quits, so nothing was cleared; quit it, then apply again", ps[0].PID) } + // The profile's own lock is Gecko's sign that a Betterbird holds it, whichever installation started it. + if _, err := os.Lstat(filepath.Join(p.path, "lock")); err == nil { + return a, errors.New("the profile is locked (its lock file is there): a Betterbird holds it, or one ended " + + "without quitting; nothing was cleared. Quit Betterbird (or start and quit it once), then apply again") + } if len(drifting) == 0 { - a.Applied, a.Note = true, "nothing drifts: prefs.js was left as it was" + a.Applied, a.Note = true, "nothing in prefs.js drifts: it was left as it was" return a, nil } target := filepath.Join(p.path, "prefs.js") @@ -519,7 +620,8 @@ func (m *Machine) ConfigApply(apply bool) (ConfigApplyAnswer, error) { kept.WriteString(l) } tmp := target + ".mesh-new" - if err := os.WriteFile(tmp, []byte(kept.String()), st.Mode().Perm()); err != nil { + if err := writeSynced(tmp, []byte(kept.String()), st.Mode().Perm()); err != nil { + os.Remove(tmp) return a, err } if err := os.Rename(tmp, target); err != nil { @@ -530,3 +632,21 @@ func (m *Machine) ConfigApply(apply bool) (ConfigApplyAnswer, error) { a.Note = fmt.Sprintf("cleared %d value(s) from prefs.js; the module's values hold from Betterbird's next start", len(a.Clears)) return a, nil } + +// writeSynced writes a file and syncs it to disk before it is renamed into place, so a crash never leaves an +// empty prefs.js. +func writeSynced(path string, data []byte, mode os.FileMode) error { + f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, mode) + if err != nil { + return err + } + if _, err := f.Write(data); err != nil { + f.Close() + return err + } + if err := f.Sync(); err != nil { + f.Close() + return err + } + return f.Close() +} diff --git a/modules/betterbird/cmd/betterbird-tools/config_test.go b/modules/betterbird/cmd/betterbird-tools/config_test.go index 9c6107d5..c4f8b669 100644 --- a/modules/betterbird/cmd/betterbird-tools/config_test.go +++ b/modules/betterbird/cmd/betterbird-tools/config_test.go @@ -269,8 +269,9 @@ func TestTheConfigShowsTheLayersAndTheDrift(t *testing.T) { p.Profile != "true" || p.Effective != "true" || p.From != "the profile's prefs.js" || !strings.Contains(p.Drift, "beats the module's false") { t.Errorf("%+v", p) } - if p := by["extensions.activeThemeID"]; p.Layer != "Betterbird's default" || !strings.Contains(p.Drift, "built-in default") { - t.Errorf("a kept default with a saved value is drift: %+v", p) + if p := by["extensions.activeThemeID"]; p.Layer != "Betterbird's default" || !strings.Contains(p.Drift, `"default-theme@mozilla.org"`) || + !strings.Contains(p.Remedy, "add-on manager") || p.Profile != `"thunderbird-compact-dark@mozilla.org"` { + t.Errorf("another theme is drift whose remedy is the add-on manager, its id answered whole: %+v", p) } if p := by["mail.spam.manualMark"]; p.Drift != "" || p.BetterbirdDefault != "false" { t.Errorf("a saved value equal to Betterbird's default is not drift: %+v", p) @@ -291,10 +292,15 @@ func TestTheConfigShowsTheLayersAndTheDrift(t *testing.T) { } found := false for _, x := range ch.Findings { - found = found || (strings.Contains(x.What, "2 managed preference(s) drift") && strings.Contains(x.What, "mail.biff.show_alert") && + found = found || (strings.Contains(x.What, "1 managed preference(s) drift") && strings.Contains(x.What, "mail.biff.show_alert") && strings.Contains(x.Do, "betterbird_config_apply") && strings.Contains(x.Do, "keep")) } - if !found { + theme := false + for _, x := range ch.Findings { + theme = theme || (strings.HasPrefix(x.What, "extensions.activeThemeID drifts") && strings.Contains(x.Do, "add-on manager") && + !strings.Contains(x.Do, "betterbird_config_apply")) + } + if !found || !theme { t.Fatalf("the check reports the drift: %+v", ch.Findings) } os.Remove(filepath.Join(f.Root, meshDefaults)) @@ -324,7 +330,7 @@ func TestConfigApplyClearsTheDriftOnlyWhileBetterbirdIsQuit(t *testing.T) { `user_pref("mail.uidensity", 0);` + "\n" f.write(profileDir+"/prefs.js", prefs) dry, err := f.ConfigApply(false) - if err != nil || dry.Applied || len(dry.Clears) != 2 { + if err != nil || dry.Applied || len(dry.Clears) != 1 || len(dry.NotCleared) != 1 { t.Fatalf("a dry run lists and changes nothing: %+v %v", dry, err) } if f.read(profileDir+"/prefs.js") != prefs { @@ -337,11 +343,19 @@ func TestConfigApplyClearsTheDriftOnlyWhileBetterbirdIsQuit(t *testing.T) { t.Fatal("prefs.js changed while Betterbird runs") } stopBetterbird(t, f) + f.write(profileDir+"/lock", "") + if _, err := f.ConfigApply(true); err == nil || !strings.Contains(err.Error(), "locked") { + t.Fatalf("refused while the profile's own lock is there: %v", err) + } + if err := os.Remove(filepath.Join(f.Root, profileDir, "lock")); err != nil { + t.Fatal(err) + } a, err := f.ConfigApply(true) if err != nil || !a.Applied { t.Fatalf("%+v %v", a, err) } want := "// Mozilla User Preferences\n" + + `user_pref("extensions.activeThemeID", "thunderbird-compact-dark@mozilla.org");` + "\n" + `user_pref("mail.identity.id1.useremail", "someone@example.org");` + "\n" + `user_pref("mail.uidensity", 0);` + "\n" if got := f.read(profileDir + "/prefs.js"); got != want { @@ -354,15 +368,15 @@ func TestConfigApplyClearsTheDriftOnlyWhileBetterbirdIsQuit(t *testing.T) { if c := said["mail.biff.show_alert"]; c.Was != "true" || c.Now != "false" { t.Errorf("it says what it cleared and what holds now: %+v", c) } - if c := said["extensions.activeThemeID"]; c.Now != "Betterbird's built-in default" { - t.Errorf("%+v", c) + if _, cleared := said["extensions.activeThemeID"]; cleared || len(a.NotCleared) != 1 || !strings.Contains(a.NotCleared[0], "add-on manager") { + t.Errorf("the theme is never cleared, and said with its remedy: %+v", a) } raw, _ := json.Marshal(a) if strings.Contains(string(raw), "example.org") { t.Fatalf("no account value leaves: %s", raw) } - if c, _ := f.Config(nil); len(c.Drifting) != 0 { - t.Fatalf("no drift once applied: %v", c.Drifting) + if c, _ := f.Config(nil); !reflect.DeepEqual(c.Drifting, []string{"extensions.activeThemeID"}) { + t.Fatalf("once applied, only the theme drifts, until the add-on manager changes it: %v", c.Drifting) } os.Remove(filepath.Join(f.Root, meshDefaults)) if _, err := f.ConfigApply(true); err == nil || !strings.Contains(err.Error(), "not placed") { @@ -435,3 +449,72 @@ func TestTheDefaultClientAskIsDecidedFromThePreferencesEntry(t *testing.T) { t.Fatalf("the lock holds whatever else the policy lacks: %s", why) } } + +// The review of pull request 256: the theme is the add-on database's, and a saved default is not drift. +func TestTheThemeIsTheAddOnDatabasesAndASavedDefaultIsNotDrift(t *testing.T) { + f := newFake(t) + f.betterbird() + f.placedByTheMesh() + f.withDefaults() + links(t, nil) + f.write(profileDir+"/prefs.js", `user_pref("browser.display.use_system_colors", false);`+"\n"+ + `user_pref("layout.css.always_underline_links", false);`+"\n") + f.write(profileDir+"/extensions.json", `{"addons":[{"id":"thunderbird-compact-dark@mozilla.org","type":"theme","active":true,"location":"app-builtin"},`+ + `{"id":"default-theme@mozilla.org","type":"theme","active":false,"location":"app-builtin"}]}`) + c, err := f.Config(nil) + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(c.Drifting, []string{"extensions.activeThemeID"}) { + t.Fatalf("the active theme from the add-on database, with no preference saved, is drift; a saved default is not: %v", c.Drifting) + } + f.write(profileDir+"/prefs.js", `user_pref("extensions.activeThemeID", "default-theme@mozilla.org");`+"\n") + f.write(profileDir+"/extensions.json", `{"addons":[{"id":"default-theme@mozilla.org","type":"theme","active":true,"location":"app-builtin"}]}`) + if c, _ := f.Config(nil); len(c.Drifting) != 0 { + t.Fatalf("a profile back on the default theme does not drift: %v", c.Drifting) + } + if _, err := f.Config([]string{"extensions.activeThemeID"}); err == nil || !strings.Contains(err.Error(), "add-on manager") { + t.Fatalf("a node layer cannot choose the theme: %v", err) + } +} + +// The review of pull request 256: a credential's or an account's name in the defaults file is never managed. +func TestAnAccountsNameInTheDefaultsIsNeverManaged(t *testing.T) { + f := newFake(t) + f.betterbird() + f.placedByTheMesh() + links(t, nil) + f.write(meshDefaults, string(repoFile(t, "install/defaults-pref/mesh-defaults.js"))+ + `pref("mail.identity.id1.useremail", "x");`+"\n"+"// keeps Betterbird's default: mail.server.server1.hostname\n"+ + `pref("mail.smtpservers", "");`+"\n") + prefs := `user_pref("mail.identity.id1.useremail", "someone@example.org");` + "\n" + + `user_pref("mail.server.server1.hostname", "imap.example.org");` + "\n" + + `user_pref("mail.smtpservers", "smtp1");` + "\n" + f.write(profileDir+"/prefs.js", prefs) + c, err := f.Config(nil) + if err != nil { + t.Fatal(err) + } + if len(c.Refused) != 3 || len(c.Drifting) != 0 { + t.Fatalf("refused, never managed: %+v", c) + } + raw, _ := json.Marshal(c) + if strings.Contains(string(raw), "example.org") { + t.Fatalf("no account value is answered: %s", raw) + } + stopBetterbird(t, f) + if _, err := f.ConfigApply(true); err != nil { + t.Fatal(err) + } + if f.read(profileDir+"/prefs.js") != prefs { + t.Fatal("prefs.js keeps every account's value") + } + ch, _ := f.Check() + found := false + for _, x := range ch.Findings { + found = found || (strings.Contains(x.What, "3 preference(s) the tools never manage") && strings.Contains(x.Do, "mesh-defaults.js")) + } + if !found { + t.Fatalf("the check says so: %+v", ch.Findings) + } +} diff --git a/modules/betterbird/install/defaults-pref/mesh-defaults.js b/modules/betterbird/install/defaults-pref/mesh-defaults.js index fee2b32b..6134173c 100644 --- a/modules/betterbird/install/defaults-pref/mesh-defaults.js +++ b/modules/betterbird/install/defaults-pref/mesh-defaults.js @@ -11,7 +11,9 @@ // --- Theme and display --- // keeps Betterbird's default: extensions.activeThemeID -// The default theme follows the desktop's dark mode; compactness comes from mail.uidensity, not a theme. +// The default theme (default-theme@mozilla.org) follows the desktop's dark mode; compactness comes from +// mail.uidensity, not a theme. Betterbird keeps the active theme in its add-on database, so another theme is +// changed back in the add-on manager, never by clearing prefs.js. // keeps Betterbird's default: browser.display.use_system_colors // Betterbird's own colours, which the default theme is drawn for. // keeps Betterbird's default: layout.css.always_underline_links