betterbird: withhold account values in the user.js answer, never write a shared outgoing server, and say a newer Owl as a fact (review 34, hq issue 345, ADR 0317)

The user.js answer carried declared addresses and hosts (ADR 0316 §16). An
adopted identity's outgoing server may be shared with an account the module
does not own, whose sending the declared values would change at every start:
refused with the remedy, since ADR 0317 §2 leaves the identity's server and
the lists as they are; a recorded adoption follows the identity's current
server. Owl installed by policy cannot be removed, so a newer Owl than the
pin is a fact whose remedy is moving the pin.

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.<an account>." + 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)

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", "<withheld>");`) {
+		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.<an account>.incoming.password") {
+		t.Fatalf("%v", err)
+	}
+}

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
 		}
 	}

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 = `"<withheld>"`
+			}
+			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")
 	}

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
 	}

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)
+		}
+	}
+}

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)"},