Give back only the groups the mesh added, and say when a new login is needed (hq ADR 0251, issue 247)

A module puts the operator's account in a group by declaring the account with that group alone.
The node-engine now records each group it added, takes back only those when nothing declared still
asks for them, refuses a group the machine lacks before usermod runs, and states each such account
as its module's resource of kind account: relogin needed while the running session lacks the group.

internal/apply/apply.go

@@ -70,6 +70,9 @@ type Outcome struct {
 	// linger is, for a user, whether it lingered before the mesh changed that, and what the mesh
 	// set (novox/hq ADR 0177).
 	linger *store.Lingering
+	// groups is, for a user, the groups the mesh put it in that it was not in before (novox/hq ADR
+	// 0251).
+	groups []string
 	// unpacked is, for an archive, what it put on the machine (novox/hq issue 162).
 	unpacked *store.Unpacked
 	// reads is, for a container, the digest of each file it was created reading, by path — so
@@ -261,6 +264,11 @@ func ApplyMindingWindows(
 	heldFiles := filesHeld(d.Resources)
 	handedFiles := map[string]store.Applied{}
 
+	// **A group still asked for by another resource is not given back when one record of it goes**
+	// (novox/hq ADR 0251), on the unit's rule above: two modules may each want the account in one group,
+	// and the one going takes nothing the other still needs.
+	wanted := groupsWanted(d.Resources)
+
 	removeOrphan := func(orphan store.Applied) error {
 		var action, detail string
 		var err error
@@ -280,9 +288,12 @@ func ApplyMindingWindows(
 				return nil
 			}
 		}
-		if declaration.Type(orphan.Type) == declaration.TypeOpening {
+		switch declaration.Type(orphan.Type) {
+		case declaration.TypeOpening:
 			action, detail, err = removeOpening(ctx, orphan, run, known.Firewall)
-		} else {
+		case declaration.TypeUser:
+			action, detail, err = removeUser(ctx, sys, orphan, run, wanted)
+		default:
 			action, detail, err = remove(ctx, sys, orphan, run, made)
 		}
 		if errors.Is(err, errNoRemoval) && store.IsFormer(orphan.ID) {
@@ -447,7 +458,8 @@ func ApplyMindingWindows(
 	// change one apply late, and never misses it.
 	// And one patience with the artifact store for the whole apply (novox/hq issue 291): a fetch it
 	// does not answer during a maintenance window waits for the window instead of failing.
-	in := inputs{declares: map[string]string{}, known: &known, windows: windows, away: newStoreAway(windows, log)}
+	in := inputs{declares: map[string]string{}, known: &known, windows: windows, away: newStoreAway(windows, log),
+		groups: wanted}
 	for _, resource := range d.Resources {
 		in.declares[resource.Identity()] = declaredDigest(resource)
 	}
@@ -711,6 +723,7 @@ func ApplyMindingWindows(
 			Found:     outcome.found,
 			Shell:     outcome.shell,
 			Linger:    outcome.linger,
+			Groups:    outcome.groups,
 			Unpacked:  outcome.unpacked,
 			Holds:     holds(resource),
 		})
@@ -917,7 +930,7 @@ func applyOne(ctx context.Context, sys system.System, r declaration.Resource, ru
 	case *declaration.Container:
 		return applyContainer(ctx, res, run, changed, in, previous)
 	case *declaration.User:
-		return applyUser(ctx, sys, res, run, previous)
+		return applyUser(ctx, sys, res, run, previous, in.groups)
 	case *declaration.Archive:
 		return applyArchive(ctx, res, previous, in.away)
 	case *declaration.Process:
@@ -1687,7 +1700,7 @@ func remove(ctx context.Context, sys system.System, a store.Applied, run Runner,
 
 	case declaration.TypeUser:
 		// Kept, with its shell given back when that is safe (novox/hq ADR 0176 §2, issue 228).
-		return removeUser(ctx, sys, a, run)
+		return removeUser(ctx, sys, a, run, nil)
 
 	case declaration.TypeNetwork:
 		// **The reason this is a shape at all** (novox/hq ADR 0029). Orphans are removed in
@@ -1834,6 +1847,9 @@ type inputs struct {
 	// away is the apply's patience with an artifact store that does not answer (novox/hq issue
 	// 291). Nil fetches once.
 	away *storeAway
+	// groups is every group the declaration asks an account to be in, and by which resources
+	// (novox/hq ADR 0251): a group one user resource stops asking for stays while another asks.
+	groups Wanted
 }
 
 // fileDigest is what a file the container reads holds, by digest.

internal/apply/groups.go

@@ -0,0 +1,221 @@
+package apply
+
+import (
+	"context"
+	"fmt"
+	"strings"
+
+	"github.com/novox/mesh-host/internal/declaration"
+	"github.com/novox/mesh-host/internal/store"
+	"github.com/novox/mesh-host/internal/system"
+)
+
+// An account's groups, from any module (novox/hq ADR 0251, issue 247).
+//
+// **A module that needs the account in a group declares the account with that group, and nothing else
+// of it.** The shell's module sets the account's shell; the lighting daemon's module puts the same
+// account in `openrazer`; the container runtime's in `docker`. A shell is one value and stays one
+// module's (the controller refuses two); a group is added, so several modules' groups never contradict.
+//
+// **The mesh takes back only what it gave.** A group the account was in before is the machine's or the
+// operator's, and stays when every resource that named it goes. A group the mesh put the account in is
+// recorded on the resource that put it there, and given back when that resource stops asking for it —
+// unless another declared resource still asks for the same group, which keeps it.
+//
+// **A group takes effect at the next login.** The machine's group database changes at once; every
+// process already running, the account's own service manager and everything it started included, keeps
+// the groups it started with. So the outcome says a new login is needed, and the node-engine's account
+// judge (internal/accounts) says so on every look until the account's running manager has the group.
+
+// Wanted is every group a declaration asks an account to be in, and which resources ask: account →
+// group → resource ids.
+type Wanted map[string]map[string][]string
+
+// groupsWanted reads every user resource's groups from a declaration.
+func groupsWanted(resources []declaration.Resource) Wanted {
+	w := Wanted{}
+	for _, r := range resources {
+		u, ok := r.(*declaration.User)
+		if !ok || u.Name == "" {
+			continue
+		}
+		for _, g := range u.Groups {
+			if w[u.Name] == nil {
+				w[u.Name] = map[string][]string{}
+			}
+			w[u.Name][g] = append(w[u.Name][g], u.ID)
+		}
+	}
+	return w
+}
+
+// othersAsk is every resource other than one that asks for an account to be in a group.
+func (w Wanted) othersAsk(account, group, except string) []string {
+	var others []string
+	for _, id := range w[account][group] {
+		if id != except {
+			others = append(others, id)
+		}
+	}
+	return others
+}
+
+// applyGroups makes the account be in every group the resource declares, and takes it out of every group
+// this resource put it in and no longer asks for. What the mesh put the account in is recorded on out.
+func applyGroups(ctx context.Context, sys system.System, r *declaration.User, run Runner,
+	previous store.Applied, wanted Wanted, out *Outcome) error {
+	// What this resource put the account in before, for this account only: a declaration that renamed its
+	// user says nothing about the new one's groups.
+	var added []string
+	if previous.Target == r.Name {
+		added = append(added, previous.Groups...)
+	}
+	if len(r.Groups) == 0 && len(added) == 0 {
+		return nil
+	}
+	in, err := system.GroupsOf(ctx, system.Runner(run), r.Name)
+	if err != nil {
+		return err
+	}
+	already := setOf(in)
+	declared := setOf(r.Groups)
+
+	var put []string
+	for _, want := range r.Groups {
+		if already[want] {
+			continue
+		}
+		exists, err := system.GroupExists(ctx, system.Runner(run), want)
+		if err != nil {
+			return err
+		}
+		if !exists {
+			return fmt.Errorf("%q was not put in the group %q: this machine has no such group yet. The package "+
+				"that makes it is not installed, or is declared after the account", r.Name, want)
+		}
+		if err := sys.AddUserToGroup(ctx, system.Runner(run), r.Name, want); err != nil {
+			return err
+		}
+		already[want] = true
+		put = append(put, want)
+		if !contains(added, want) {
+			added = append(added, want)
+		}
+	}
+
+	// What this resource put the account in and no longer asks for.
+	var kept, said []string
+	for _, g := range added {
+		switch {
+		case declared[g]:
+			kept = append(kept, g)
+		case !already[g]:
+			// Taken out since, by a person: nothing to give back.
+		case len(wanted.othersAsk(r.Name, g, r.ID)) > 0:
+			said = append(said, fmt.Sprintf("left in %s, which %s still asks for", g,
+				strings.Join(wanted.othersAsk(r.Name, g, r.ID), ", ")))
+		default:
+			gone, why := leaveGroup(ctx, sys, r.Name, g, run)
+			if !gone {
+				kept = append(kept, g)
+			}
+			said = append(said, why)
+		}
+	}
+	out.groups = kept
+	if len(put) > 0 {
+		said = append([]string{fmt.Sprintf("put in %s; a session that began before has %s only after a new "+
+			"login", strings.Join(put, ", "), them(len(put)))}, said...)
+	}
+	if len(put) > 0 || len(said) > 0 {
+		if out.Action == "unchanged" {
+			out.Action = "updated"
+		}
+		out.Detail = joinDetail(out.Detail, strings.Join(said, "; "))
+	}
+	return nil
+}
+
+// giveGroupsBack is removeUser's groups: every group the mesh put the account in, taken back unless
+// another declared resource still asks for it. Never fatal, for the shell's reason: a removal that failed
+// would stay recorded and fail the same way on every apply after. Empty when there was nothing to say.
+func giveGroupsBack(ctx context.Context, sys system.System, a store.Applied, run Runner, wanted Wanted) (bool, string) {
+	if len(a.Groups) == 0 {
+		return false, ""
+	}
+	in, err := system.GroupsOf(ctx, system.Runner(run), a.Target)
+	if err != nil {
+		return false, fmt.Sprintf("the groups the mesh put it in (%s) were not given back: %v",
+			strings.Join(a.Groups, ", "), err)
+	}
+	already := setOf(in)
+	gave := false
+	var said []string
+	for _, g := range a.Groups {
+		if !already[g] {
+			continue
+		}
+		if others := wanted.othersAsk(a.Target, g, a.ID); len(others) > 0 {
+			said = append(said, fmt.Sprintf("left in %s, which %s still asks for", g, strings.Join(others, ", ")))
+			continue
+		}
+		gone, why := leaveGroup(ctx, sys, a.Target, g, run)
+		gave = gave || gone
+		said = append(said, why)
+	}
+	return gave, strings.Join(said, "; ")
+}
+
+// leaveGroup takes an account out of one group the mesh put it in, read back from the machine, and says
+// what came of it.
+func leaveGroup(ctx context.Context, sys system.System, account, group string, run Runner) (bool, string) {
+	l, ok := sys.(system.GroupLeaver)
+	if !ok {
+		return false, fmt.Sprintf("left in %s: this machine's system cannot take an account out of one group", group)
+	}
+	if err := l.RemoveUserFromGroup(ctx, system.Runner(run), account, group); err != nil {
+		return false, fmt.Sprintf("left in %s: %v", group, err)
+	}
+	in, err := system.GroupsOf(ctx, system.Runner(run), account)
+	if err != nil {
+		return false, fmt.Sprintf("taken out of %s, and the group database could not be read back: %v", group, err)
+	}
+	if setOf(in)[group] {
+		return false, fmt.Sprintf("taken out of %s, and the group database still lists it there", group)
+	}
+	return true, fmt.Sprintf("taken out of %s, which the mesh had put it in", group)
+}
+
+func setOf(items []string) map[string]bool {
+	s := map[string]bool{}
+	for _, i := range items {
+		s[i] = true
+	}
+	return s
+}
+
+func contains(items []string, want string) bool {
+	for _, i := range items {
+		if i == want {
+			return true
+		}
+	}
+	return false
+}
+
+func them(n int) string {
+	if n == 1 {
+		return "it"
+	}
+	return "them"
+}
+
+func joinDetail(a, b string) string {
+	switch {
+	case a == "":
+		return b
+	case b == "":
+		return a
+	}
+	return a + "; " + b
+}

internal/apply/groups_test.go

@@ -0,0 +1,231 @@
+package apply
+
+import (
+	"context"
+	"errors"
+	"sort"
+	"strings"
+	"testing"
+
+	"github.com/novox/mesh-host/internal/store"
+)
+
+// Defends novox/hq ADR 0251 and issue 247: a module puts the operator's account in a group by declaring
+// the account with that group alone; the account's other groups are never touched; a new login is said;
+// and the mesh gives back only a group it put the account in, and only when nobody declared still asks.
+
+// groupDB is a fake user and group database: the account's groups, the groups the machine has, and every
+// command it was asked.
+type groupDB struct {
+	account string
+	in      map[string]bool
+	exists  map[string]bool
+	asked   []string
+}
+
+func newGroupDB(in []string, exists ...string) *groupDB {
+	g := &groupDB{account: "operator", in: map[string]bool{}, exists: map[string]bool{}}
+	for _, x := range in {
+		g.in[x], g.exists[x] = true, true
+	}
+	for _, x := range exists {
+		g.exists[x] = true
+	}
+	return g
+}
+
+func (g *groupDB) run(_ context.Context, name string, args ...string) (string, error) {
+	g.asked = append(g.asked, name+" "+strings.Join(args, " "))
+	switch {
+	case name == "getent" && args[0] == "passwd":
+		if args[1] == g.account {
+			return g.account + ":x:1500:1500::/home/" + g.account + ":/bin/bash\n", nil
+		}
+		return "", errors.New("getent exited 2: ")
+	case name == "getent" && args[0] == "group":
+		if g.exists[args[1]] {
+			return args[1] + ":x:900:\n", nil
+		}
+		return "", errors.New("getent exited 2: ")
+	case name == "id":
+		var out []string
+		for x := range g.in {
+			out = append(out, x)
+		}
+		sort.Strings(out)
+		return strings.Join(out, " ") + "\n", nil
+	case name == "usermod" && args[0] == "--append":
+		if !g.exists[args[2]] {
+			return "", errors.New("usermod exited 6: group '" + args[2] + "' does not exist")
+		}
+		g.in[args[2]] = true
+	case name == "gpasswd" && args[0] == "--delete":
+		delete(g.in, args[2])
+	}
+	return "", nil
+}
+
+func (g *groupDB) groups() string {
+	var out []string
+	for x := range g.in {
+		out = append(out, x)
+	}
+	sort.Strings(out)
+	return strings.Join(out, " ")
+}
+
+func applyGroupsOf(t *testing.T, g *groupDB, known store.State, resources ...string) (Report, store.State, error) {
+	t.Helper()
+	if len(resources) == 0 {
+		resources = []string{`{"id":"other.dir","type":"directory","path":"` + t.TempDir() + `/other"}`}
+	}
+	return Apply(context.Background(), archHost(t), parse(t, `{"declaration":1,"resources":[`+
+		strings.Join(resources, ",")+`]}`), known, store.OriginDeclared, g.run, nil, nil)
+}
+
+const (
+	razer  = `{"id":"openrazer.account","type":"user","name":"operator","groups":["openrazer"]}`
+	docker = `{"id":"docker.account","type":"user","name":"operator","groups":["docker"]}`
+	shell  = `{"id":"zsh.login","type":"user","name":"operator","groups":[]}`
+)
+
+func TestAModulePutsTheAccountInAGroupAndSaysANewLoginIsNeeded(t *testing.T) {
+	g := newGroupDB([]string{"wheel", "plugdev"}, "openrazer")
+	report, state, err := applyGroupsOf(t, g, store.State{}, shell, razer)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := g.groups(); got != "openrazer plugdev wheel" {
+		t.Fatalf("the account's groups are %q", got)
+	}
+	o := outcomeOf(report, "openrazer.account")
+	if o.Action != "updated" || !strings.Contains(o.Detail, "put in openrazer") || !strings.Contains(o.Detail, "new login") {
+		t.Errorf("the outcome did not say the group and the new login: %+v", o)
+	}
+	a, _ := state.Find("openrazer.account")
+	if strings.Join(a.Groups, " ") != "openrazer" {
+		t.Errorf("the record holds %v, want the one group the mesh added", a.Groups)
+	}
+	for _, asked := range g.asked {
+		if strings.HasPrefix(asked, "usermod") && !strings.HasPrefix(asked, "usermod --append --groups openrazer ") {
+			t.Errorf("usermod was asked something other than appending the one group: %q", asked)
+		}
+	}
+	// Applied again: nothing to do, and the record still says the mesh added it.
+	report, state, err = applyGroupsOf(t, g, state, shell, razer)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if o := outcomeOf(report, "openrazer.account"); o.Action != "unchanged" {
+		t.Errorf("a second apply changed something: %+v", o)
+	}
+	if a, _ := state.Find("openrazer.account"); strings.Join(a.Groups, " ") != "openrazer" {
+		t.Errorf("a second apply lost what the mesh added: %v", a.Groups)
+	}
+}
+
+func TestAGroupTheAccountWasAlreadyInIsNeverTakenBack(t *testing.T) {
+	g := newGroupDB([]string{"wheel", "openrazer"})
+	_, state, err := applyGroupsOf(t, g, store.State{}, razer)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if a, _ := state.Find("openrazer.account"); len(a.Groups) != 0 {
+		t.Fatalf("a group found was recorded as the mesh's: %v", a.Groups)
+	}
+	if _, _, err := applyGroupsOf(t, g, state); err != nil {
+		t.Fatal(err)
+	}
+	if got := g.groups(); got != "openrazer wheel" {
+		t.Errorf("undeclaring took a found group: %q", got)
+	}
+}
+
+func TestTheGroupTheMeshAddedIsGivenBackWhenItsModuleGoes(t *testing.T) {
+	g := newGroupDB([]string{"wheel"}, "openrazer")
+	_, state, err := applyGroupsOf(t, g, store.State{}, razer)
+	if err != nil {
+		t.Fatal(err)
+	}
+	report, state, err := applyGroupsOf(t, g, state)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := g.groups(); got != "wheel" {
+		t.Errorf("the account's groups after its module went: %q, want wheel alone", got)
+	}
+	o := outcomeOf(report, "openrazer.account")
+	if o.Action != "restored" || !strings.Contains(o.Detail, "taken out of openrazer") {
+		t.Errorf("the removal did not say the group was given back: %+v", o)
+	}
+	if _, still := state.Find("openrazer.account"); still {
+		t.Error("the record stayed")
+	}
+}
+
+func TestAGroupAnotherResourceStillAsksForStays(t *testing.T) {
+	both := `{"id":"openrazer.account","type":"user","name":"operator","groups":["openrazer","video"]}`
+	video := `{"id":"media.account","type":"user","name":"operator","groups":["video"]}`
+	g := newGroupDB(nil, "openrazer", "video")
+	_, state, err := applyGroupsOf(t, g, store.State{}, both, video)
+	if err != nil {
+		t.Fatal(err)
+	}
+	report, _, err := applyGroupsOf(t, g, state, video)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := g.groups(); got != "video" {
+		t.Errorf("the account's groups: %q, want video kept for media and openrazer given back", got)
+	}
+	if o := outcomeOf(report, "openrazer.account"); !strings.Contains(o.Detail, "media.account still asks for") {
+		t.Errorf("the removal did not say why video stayed: %+v", o)
+	}
+}
+
+func TestAGroupNoLongerDeclaredIsGivenBackWhileTheAccountStaysDeclared(t *testing.T) {
+	g := newGroupDB(nil, "openrazer", "input")
+	two := `{"id":"openrazer.account","type":"user","name":"operator","groups":["openrazer","input"]}`
+	_, state, err := applyGroupsOf(t, g, store.State{}, two)
+	if err != nil {
+		t.Fatal(err)
+	}
+	_, state, err = applyGroupsOf(t, g, state, razer)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := g.groups(); got != "openrazer" {
+		t.Errorf("the account's groups: %q, want input given back", got)
+	}
+	if a, _ := state.Find("openrazer.account"); strings.Join(a.Groups, " ") != "openrazer" {
+		t.Errorf("the record holds %v", a.Groups)
+	}
+}
+
+func TestAGroupThatDoesNotExistYetFailsTheResourceSayingSo(t *testing.T) {
+	g := newGroupDB([]string{"wheel"})
+	_, _, err := applyGroupsOf(t, g, store.State{}, razer)
+	if err == nil || !strings.Contains(err.Error(), "no such group yet") {
+		t.Fatalf("a missing group was not said: %v", err)
+	}
+	for _, asked := range g.asked {
+		if strings.HasPrefix(asked, "usermod") {
+			t.Errorf("usermod was run for a group the machine does not have: %q", asked)
+		}
+	}
+}
+
+func TestAGroupAPersonTookTheAccountOutOfSinceIsPutBackWhileDeclared(t *testing.T) {
+	g := newGroupDB(nil, "openrazer")
+	_, state, err := applyGroupsOf(t, g, store.State{}, razer)
+	if err != nil {
+		t.Fatal(err)
+	}
+	delete(g.in, "openrazer")
+	if _, _, err := applyGroupsOf(t, g, state, razer); err != nil {
+		t.Fatal(err)
+	}
+	if got := g.groups(); got != "openrazer" {
+		t.Errorf("a declared group was not put back: %q", got)
+	}
+}

internal/apply/user.go

@@ -28,8 +28,11 @@ import (
 //
 // previous is this resource's record, which carries the shell the account had before the mesh
 // first changed it, so removal can give it back (novox/hq ADR 0176 §2, issue 228).
+//
+// wanted is every group the whole declaration asks an account to be in (novox/hq ADR 0251), so a group
+// this resource stops asking for is kept while another resource asks for it.
 func applyUser(ctx context.Context, sys system.System, r *declaration.User, run Runner,
-	previous store.Applied) (Outcome, error) {
+	previous store.Applied, wanted Wanted) (Outcome, error) {
 	out := begin(r)
 	out.Action = "unchanged"
 	// What was found is carried from the record for as long as the resource is recorded — for this
@@ -82,26 +85,8 @@ func applyUser(ctx context.Context, sys system.System, r *declaration.User, run
 	// Groups before the shell, so that a failure here comes before the shell is changed: a record
 	// is written only for an apply that worked, and a shell changed by a failed one would be read
 	// next time as the account's own, and the one it replaced lost.
-	if len(r.Groups) > 0 {
-		in, err := system.GroupsOf(ctx, system.Runner(run), r.Name)
-		if err != nil {
-			return out, err
-		}
-		already := map[string]bool{}
-		for _, g := range in {
-			already[g] = true
-		}
-		for _, want := range r.Groups {
-			if already[want] {
-				continue
-			}
-			if err := sys.AddUserToGroup(ctx, system.Runner(run), r.Name, want); err != nil {
-				return out, err
-			}
-			if out.Action == "unchanged" {
-				out.Action = "updated"
-			}
-		}
+	if err := applyGroups(ctx, sys, r, run, previous, wanted, &out); err != nil {
+		return out, err
 	}
 
 	// The shell, only when it differs. Absent means the host asserts nothing — a field that
@@ -203,7 +188,11 @@ func onOff(on bool) string {
 // uninstalled since would break the very logins the giving back is for. Otherwise it is left, and
 // the outcome says why. Never errNoRemoval: an orphaned login that failed removal stopped the
 // whole apply, on every apply after.
-func removeUser(ctx context.Context, sys system.System, a store.Applied, run Runner) (string, string, error) {
+//
+// And every group the mesh put the account in, while no other declared resource asks for it (novox/hq
+// ADR 0251), on the same rule: never fatal, and never a group the account was in before.
+func removeUser(ctx context.Context, sys system.System, a store.Applied, run Runner,
+	wanted Wanted) (string, string, error) {
 	const kept = "the account is kept; the host never deletes a login"
 	login, exists, err := system.LookUpUser(ctx, system.Runner(run), a.Target)
 	if err != nil {
@@ -222,6 +211,12 @@ func removeUser(ctx context.Context, sys system.System, a store.Applied, run Run
 			action = "restored"
 		}
 	}
+	if gave, said := giveGroupsBack(ctx, sys, a, run, wanted); said != "" {
+		detail += "; " + said
+		if gave {
+			action = "restored"
+		}
+	}
 	return action, detail, nil
 }