diff --git a/modules/betterbird/README.md b/modules/betterbird/README.md index 6ac56909..b86e73b9 100644 --- a/modules/betterbird/README.md +++ b/modules/betterbird/README.md @@ -54,8 +54,9 @@ reading the new range there (ADR 0316 §8). **The policy** turns Betterbird's own updater off (`DisableAppUpdate`, which Betterbird's update check honours) and installs Owl into every profile from `file:///opt/betterbird/owl/owl.xpi` as `force_installed`, with `updates_disabled`. A profile that already runs another Owl keeps it: the policy -installs the pinned one where Owl is missing, and `betterbird_check` says when the profile's Owl is not -the pin. **Owl's licence stays the person's**: the mesh holds no licence, key or payment (ADR 0316 §9). +installs the pinned one where Owl is missing. A newer Owl in the profile (g14 runs 1.5.7) is said as a +fact by `betterbird_status`; Owl installed by policy cannot be removed in Betterbird, so the remedy is +moving the pin. An older one is a finding of `betterbird_check`, with the same remedy. **Owl's licence stays the person's**: the mesh holds no licence, key or payment (ADR 0316 §9). **No start of its own.** Betterbird is started by the operator from the launcher. The start (`/usr/local/bin/betterbird`) first runs `betterbird-tools prepare`, which, with no Betterbird of the @@ -120,7 +121,12 @@ declared account, in its place in the setting, is (ADR 0317 §1): **Refused, for that account alone and with the remedy** (§7): two accounts with the address (remove the duplicate in Betterbird, or name the `key`), Owl's account at the address, a named key that does not qualify, an account of another kind than declared (never converted), an identity that names no outgoing -server. The other accounts go ahead; the start and `betterbird_check` say it. +server, and an outgoing server that another identity, neither adopted nor the module's, also uses +(Betterbird lets identities share one, and the declared values would change that other account's sending; +ADR 0317 §2 leaves the identity's server and the lists as they are, so the module refuses rather than give +it a server of its own). A recorded adoption follows the identity's current outgoing server at every apply, +under the same rule. The other accounts go ahead; the start and `betterbird_check` say it. +`betterbird_user_js` answers an account's preferences by name with their values withheld. The declared fields are written in the user.js under the account's keys, so they win at every start; only the fields the setting names, never the account's directory, folders, filters, caches, mail or kept diff --git a/modules/betterbird/cmd/betterbird-tools/accounts.go b/modules/betterbird/cmd/betterbird-tools/accounts.go index 94f928e6..cefe7ec7 100644 --- a/modules/betterbird/cmd/betterbird-tools/accounts.go +++ b/modules/betterbird/cmd/betterbird-tools/accounts.go @@ -124,6 +124,10 @@ func parseAccounts(raw []byte) (map[string]Account, error) { return nil, fmt.Errorf("~/%s is not JSON: %v", accountsFile, err) } if where := secretKeyIn(tree, ""); where != "" { + // An account's name is said by nothing: its segment of the path is withheld. + if parts := strings.SplitN(where, ".", 3); len(parts) == 3 && parts[0] == "accounts" { + where = "accounts.." + parts[2] + } return nil, fmt.Errorf("the setting accounts carries %s, a key shaped like a credential: an account is declared "+ "without its password or token, which it signs in with at its first use (novox/hq ADR 0316 §10, §14). "+ "Take it out at the controller's terminal", where) diff --git a/modules/betterbird/cmd/betterbird-tools/accounts_test.go b/modules/betterbird/cmd/betterbird-tools/accounts_test.go index bc2429bf..5642af6a 100644 --- a/modules/betterbird/cmd/betterbird-tools/accounts_test.go +++ b/modules/betterbird/cmd/betterbird-tools/accounts_test.go @@ -279,7 +279,8 @@ func TestAnAmbiguousOwlsOrOtherKindOfMatchIsRefusedForThatAccountAlone(t *testin `user_pref("mail.account.account4.server", "server4");` + "\n" + `user_pref("mail.account.account4.identities", "id4");` + "\n" + `user_pref("mail.identity.id4.useremail", "other@mail.example.org");` + "\n" + - `user_pref("mail.identity.id4.smtpServer", "smtp1");` + "\n" + + `user_pref("mail.identity.id4.smtpServer", "smtp4");` + "\n" + + `user_pref("mail.smtpserver.smtp4.hostname", "smtp.example.org");` + "\n" + `user_pref("mail.server.server4.type", "imap");` + "\n" f.write(profileDir+"/prefs.js", prefs) both := strings.Replace(adoptable, `{"accounts": {`, `{"accounts": { @@ -403,3 +404,82 @@ func TestBetterbirdAccountsAnswersCountsAndProposesTheValueWithoutOwlOrACredenti t.Fatalf("the controller's words come back with every address masked: %s", a.Proposed) } } + +func TestAnAdoptedAccountNeverWritesIntoAnOutgoingServerAnotherIdentityUses(t *testing.T) { + f := newFake(t) + f.profileWithAccounts() + f.noPackage() + links(t, nil) + f.declare("// none") + os.RemoveAll(filepath.Join(f.Root, "proc", "4000")) + // A second account of the person's, not declared, sends through smtp1 too. + prefs := strings.Replace(f.read(profileDir+"/prefs.js"), `"account1,account3,account2"`, `"account1,account3,account2,account5"`, 1) + + `user_pref("mail.account.account5.server", "server5");` + "\n" + + `user_pref("mail.account.account5.identities", "id5");` + "\n" + + `user_pref("mail.identity.id5.useremail", "fifth@mail.example.org");` + "\n" + + `user_pref("mail.identity.id5.smtpServer", "smtp1");` + "\n" + + `user_pref("mail.server.server5.type", "imap");` + "\n" + f.write(profileDir+"/prefs.js", prefs) + f.declareAccounts(adoptable) + err := f.Prepare() + if err == nil || !strings.Contains(err.Error(), "also used by identity id5") || strings.Contains(err.Error(), "example.org") { + t.Fatalf("refused with the remedy, by keys: %v", err) + } + if user, _ := os.ReadFile(filepath.Join(f.Root, profileDir, "user.js")); strings.Contains(string(user), "smtpserver.smtp1") { + t.Fatal("the shared outgoing server was written") + } + + // Recorded adoption, then the person moves the identity to a server of its own: the module follows it. + f.write(profileDir+"/prefs.js", strings.Replace(prefs, `"mail.identity.id5.smtpServer", "smtp1"`, `"mail.identity.id5.smtpServer", "smtp5"`, 1)) + if err := f.Prepare(); err != nil { + t.Fatal(err) + } + moved := strings.Replace(f.read(profileDir+"/prefs.js"), `"mail.identity.id1.smtpServer", "smtp1"`, `"mail.identity.id1.smtpServer", "smtp9"`, 1) + + `user_pref("mail.smtpserver.smtp9.hostname", "smtp.example.org");` + "\n" + f.write(profileDir+"/prefs.js", moved) + u, err := f.userJS(true) + if err != nil || u.Each[0].How != adopted || u.Each[0].Keys.SMTP != "smtp9" { + t.Fatalf("the identity's current outgoing server is followed: %+v %v", u.Each, err) + } + user := f.read(profileDir + "/user.js") + if strings.Contains(user, "smtpserver.smtp1.") || !strings.Contains(user, "smtpserver.smtp9.hostname") { + t.Fatalf("never the old one:\n%s", user) + } + // And the move back onto a shared server is refused again. + f.write(profileDir+"/prefs.js", strings.Replace(moved, `"mail.identity.id5.smtpServer", "smtp5"`, `"mail.identity.id5.smtpServer", "smtp9"`, 1)) + if err := f.Prepare(); err == nil || !strings.Contains(err.Error(), "also used by identity id5") { + t.Fatalf("%v", err) + } +} + +func TestTheUserJSAnswerWithholdsTheAccountsValues(t *testing.T) { + f := newFake(t) + f.profileWithAccounts() + f.noPackage() + links(t, nil) + f.declare(`user_pref("mail.compose.default_to", "x");`) + f.declareAccounts(personal) + u, err := f.userJS(false) + if err != nil { + t.Fatal(err) + } + for _, never := range []string{"example.org", `"me"`, `"Me"`} { + if strings.Contains(u.Content, never) { + t.Errorf("the answer carries %s:\n%s", never, u.Content) + } + } + if !strings.Contains(u.Content, `user_pref("mail.server.server_mesh_personal.hostname", "");`) { + t.Fatalf("an account's preference is named and its value withheld:\n%s", u.Content) + } + // What is placed in the profile keeps the values: only the answer withholds them. + if f.read(testHome+"/"+accountsFile) == "" { + t.Fatal("no declaration") + } +} + +func TestACredentialRefusalNamesNoAccount(t *testing.T) { + _, err := parseAccounts([]byte(`{"accounts": {"personal": {"incoming": {"password": "hunter2"}}}}`)) + if err == nil || strings.Contains(err.Error(), "personal") || !strings.Contains(err.Error(), "accounts..incoming.password") { + t.Fatalf("%v", err) + } +} diff --git a/modules/betterbird/cmd/betterbird-tools/adoption.go b/modules/betterbird/cmd/betterbird-tools/adoption.go index f8b3291f..ecf137e6 100644 --- a/modules/betterbird/cmd/betterbird-tools/adoption.go +++ b/modules/betterbird/cmd/betterbird-tools/adoption.go @@ -28,6 +28,7 @@ import ( "fmt" "os" "path/filepath" + "sort" "strings" ) @@ -138,7 +139,61 @@ func resolveAccounts(accounts map[string]Account, prefs map[string]string, rec m } out = append(out, r) } - return out + return refuseSharedOutgoing(out, prefs, have) +} + +// refuseSharedOutgoing refuses an adopted account whose outgoing server another identity uses that is neither +// adopted nor the module's (review 34 of mesh-catalog #232): Betterbird lets identities share an outgoing +// server, and the declared values written into it would change that other account's sending at every start. +// ADR 0317 §2 adopts the outgoing server the identity names and leaves the lists as they are, so the module +// does not give the identity a server of its own; it refuses, with the remedy. +func refuseSharedOutgoing(resolved []resolvedAccount, prefs map[string]string, have []profileAccount) []resolvedAccount { + ours := map[string]bool{} + for _, r := range resolved { + if r.How != refused { + ours[r.Keys.Identity] = true + } + } + defaultServer := unquote(prefs["mail.smtp.defaultserver"]) + users := map[string][]string{} + seen := map[string]bool{} + use := func(id string) { + if seen[id] || ours[id] { + return + } + seen[id] = true + server := unquote(prefs["mail.identity."+id+".smtpServer"]) + if server == "" { + server = defaultServer + } + if server != "" { + users[server] = append(users[server], id) + } + } + for _, a := range have { + for _, id := range a.Identities { + use(id) + } + } + for k := range prefs { + if strings.HasPrefix(k, "mail.identity.") && strings.HasSuffix(k, ".smtpServer") { + use(strings.TrimSuffix(strings.TrimPrefix(k, "mail.identity."), ".smtpServer")) + } + } + for i, r := range resolved { + if r.How != adopted { + continue + } + if others := users[r.Keys.SMTP]; len(others) > 0 { + sort.Strings(others) + resolved[i] = resolvedAccount{Place: r.Place, How: refused, name: r.name, Why: fmt.Sprintf( + "declared account %d of %d: the outgoing server %s of the profile's account %s is also used by "+ + "identity %s, whose sending the declared values would change: give the account an outgoing "+ + "server of its own in Betterbird's account settings, then start Betterbird again", + r.Place, len(resolved), r.Keys.SMTP, r.Keys.Account, strings.Join(others, ", "))} + } + } + return resolved } func resolveOne(place, of int, name string, a Account, prefs map[string]string, have []profileAccount, @@ -153,6 +208,15 @@ func resolveOne(place, of int, name string, a Account, prefs map[string]string, if was, ok := rec[name]; ok { if _, there := inList[was.Keys.Account]; there || was.How == created { r.How, r.Keys = was.How, was.Keys + if was.How == adopted { + // The identity's outgoing server as it is now: a person may have moved it since. + smtpKey := unquote(prefs["mail.identity."+was.Keys.Identity+".smtpServer"]) + if smtpKey == "" { + return refuse("the profile's account %s has an identity that names no outgoing server: choose one "+ + "for it in Betterbird's account settings, then start Betterbird again", was.Keys.Account) + } + r.Keys.SMTP = smtpKey + } return r } } diff --git a/modules/betterbird/cmd/betterbird-tools/betterbird.go b/modules/betterbird/cmd/betterbird-tools/betterbird.go index 5a4af023..3759e49c 100644 --- a/modules/betterbird/cmd/betterbird-tools/betterbird.go +++ b/modules/betterbird/cmd/betterbird-tools/betterbird.go @@ -502,7 +502,7 @@ func (m *Machine) userJS(apply bool) (UserJS, error) { a.Valid = verr.Error() } want = []byte(composeUserJS(string(declaredText), accounts, a.Each)) - a.Content = string(want) + a.Content = m.answeredUserJS(string(want)) for _, line := range strings.Split(string(want), "\n") { for _, st := range splitStatements(strings.TrimSpace(line)) { if statement.MatchString(st) { @@ -574,6 +574,26 @@ func (m *Machine) userJS(apply bool) (UserJS, error) { return a, nil } +// answeredUserJS is the user.js as a tool answers it (novox/hq ADR 0316 §16, ADR 0317 §9): an account's +// preferences with their values withheld, every other value masked as betterbird_prefs masks it. +func (m *Machine) answeredUserJS(text string) string { + lines := strings.Split(text, "\n") + for i, line := range lines { + for _, st := range splitStatements(strings.TrimSpace(line)) { + mm := prefLine.FindStringSubmatch(st) + if mm == nil { + continue + } + value := m.maskValue(mm[2]) + if why := withheld(mm[1]); why != "" { + value = `""` + } + lines[i] = fmt.Sprintf("user_pref(%s, %s);", quoted(mm[1]), value) + } + } + return strings.Join(lines, "\n") +} + // Prepare is what the module's start runs before Betterbird (`betterbird-tools prepare`): with no Betterbird // of this account running, the user.js placed and the declared accounts written into the account lists. With // one running it does nothing, since the start then only hands over to it. Its error is said by the start, @@ -861,9 +881,9 @@ func (m *Machine) Check() (CheckAnswer, error) { add("Owl's xpi is not placed ("+owlFile+")", "push the module to this node") case in.Owl.InProfile == "": add("Owl is not installed in the profile yet", "Betterbird installs it from the policy at its next start") - case !in.Owl.AtThePin: - add("Owl "+in.Owl.InProfile+" runs in the profile and the mesh pins "+in.Owl.Pinned, - "the policy installs the pinned version only where Owl is missing: a person decides whether to remove the other one in Betterbird's add-ons") + case !in.Owl.AtThePin && !newerVersion(in.Owl.InProfile, in.Owl.Pinned): + add("Owl "+in.Owl.InProfile+" runs in the profile, older than the mesh's pin "+in.Owl.Pinned, + "Owl is installed by policy and cannot be removed in Betterbird: move the module's Owl pin in a reviewed change (novox/hq ADR 0316 §6)") case !in.Owl.Active: add("Owl is installed and not active", "turn it on in Betterbird's add-ons") } diff --git a/modules/betterbird/cmd/betterbird-tools/install.go b/modules/betterbird/cmd/betterbird-tools/install.go index 40cfab26..9e852c5b 100644 --- a/modules/betterbird/cmd/betterbird-tools/install.go +++ b/modules/betterbird/cmd/betterbird-tools/install.go @@ -73,6 +73,30 @@ type OwlState struct { InProfile string `json:"in_profile,omitempty"` Active bool `json:"active"` AtThePin bool `json:"at_the_pin"` + // Note says, as a fact, a profile running an Owl newer than the pin: the policy's install keeps a newer + // version, and Owl installed by policy cannot be removed in Betterbird. + Note string `json:"note,omitempty"` +} + +// newerVersion says whether version a is newer than b, comparing dotted numbers. +func newerVersion(a, b string) bool { + if a == "" || b == "" { + return false + } + pa, pb := strings.Split(a, "."), strings.Split(b, ".") + for i := 0; i < len(pa) || i < len(pb); i++ { + var x, y int + if i < len(pa) { + fmt.Sscanf(pa[i], "%d", &x) + } + if i < len(pb) { + fmt.Sscanf(pb[i], "%d", &y) + } + if x != y { + return x > y + } + } + return false } // installation reads what is installed. profile may be empty when none is found yet. @@ -104,6 +128,11 @@ func (m *Machine) installation(profile string) Installation { } } owl.AtThePin = owl.Pinned != "" && owl.Pinned == owl.InProfile + if newerVersion(owl.InProfile, owl.Pinned) { + owl.Note = "the profile runs Owl " + owl.InProfile + ", newer than the mesh's pin " + owl.Pinned + ": the policy " + + "keeps a newer version and installs the pin only where Owl is missing. To run the same Owl everywhere, " + + "move the module's pin in a reviewed change" + } if owl.Pinned != "" || owl.InProfile != "" { in.Owl = owl } diff --git a/modules/betterbird/cmd/betterbird-tools/install_test.go b/modules/betterbird/cmd/betterbird-tools/install_test.go index fc8a7156..94838b04 100644 --- a/modules/betterbird/cmd/betterbird-tools/install_test.go +++ b/modules/betterbird/cmd/betterbird-tools/install_test.go @@ -87,6 +87,27 @@ func TestTheCommunityPackageAPolicyThatIsNotTheModulesAndAnOwlOffItsPinAreSaid(t if !strings.Contains(in.Policy, "DisableAppUpdate") || in.Owl.AtThePin || in.Owl.InProfile != "1.5.7" { t.Fatalf("%+v %+v", in, in.Owl) } + // A newer Owl than the pin is a fact with the real remedy, never a finding to remove it. + if !strings.Contains(in.Owl.Note, "move the module's pin") { + t.Fatalf("%+v", in.Owl) + } + f.write(policyFile, `{"policies": {"DisableAppUpdate": true, "ExtensionSettings": {"owl@beonex.com": {"installation_mode": "force_installed", + "install_url": "file:///opt/betterbird/owl/owl.xpi", "updates_disabled": true}}}}`) + c, _ := f.Check() + for _, x := range c.Findings { + if strings.Contains(x.What, "Owl") { + t.Errorf("a newer Owl is not a finding: %+v", x) + } + } + f.write(profileDir+"/extensions.json", `{"addons":[{"id":"owl@beonex.com","version":"1.5.5","active":true,"location":"app-profile"}]}`) + c, _ = f.Check() + found := false + for _, x := range c.Findings { + found = found || (strings.Contains(x.What, "older than the mesh's pin") && strings.Contains(x.Do, "move the module's Owl pin")) + } + if !found { + t.Fatalf("an older Owl is a finding whose remedy is the pin: %+v", c.Findings) + } f.write(policyFile, `{"policies": {"DisableAppUpdate": true, "ExtensionSettings": {"owl@beonex.com": {"installation_mode": "force_installed", "install_url": "https://vendor.example/owl.xpi", "updates_disabled": true}}}}`) if in := f.installation(""); !strings.Contains(in.Policy, "not from the mesh's xpi") { @@ -142,3 +163,12 @@ func TestTheReminderTestIsVisiblyATestAndSaysItOpensAWindow(t *testing.T) { } } } + +func TestTheUserJSDescriptionSaysAdoption(t *testing.T) { + for _, tool := range tools() { + if tool.Name == "betterbird_user_js" && (!strings.Contains(tool.Description, "adopted under its own keys") || + strings.Contains(tool.Description, "refuses the apply")) { + t.Fatalf("%s", tool.Description) + } + } +} diff --git a/modules/betterbird/cmd/betterbird-tools/main.go b/modules/betterbird/cmd/betterbird-tools/main.go index fcf5bf7e..27778fe6 100644 --- a/modules/betterbird/cmd/betterbird-tools/main.go +++ b/modules/betterbird/cmd/betterbird-tools/main.go @@ -71,9 +71,10 @@ func tools() []stdio.Tool { "accounts' preferences under the module's own keys (setting accounts) — against the profile's user.js: " + "whether it is placed, and whether the running Betterbird started after it was. With apply, place it in " + "the profile (only where the profile has no user.js or the module's own; an operator's file is never " + - "overwritten) and, when Betterbird is not running, add the declared accounts' keys to the profile's " + - "account lists. A declared account whose address the profile already holds under another key refuses " + - "the apply, before anything is written. Betterbird reads it at its next start: betterbird_restart. " + + "overwritten). A declared account the profile already holds at its address is adopted under its own keys; " + + "one it does not hold is created under the module's keys, which are added to the account lists when " + + "Betterbird is not running; an ambiguous one is refused alone, with the remedy (novox/hq ADR 0317). " + + "Accounts' values are withheld in the answer. Betterbird reads it at its next start: betterbird_restart. " + "(r; a with apply)", Input: map[string]any{ "apply": map[string]any{"type": "boolean", "description": "place the declared user.js in the profile (default false)"},