From 656116e15943e03c66056bef24fdbf448f887c2e Mon Sep 17 00:00:00 2001 From: Tim Smith Date: Wed, 23 Sep 2026 21:34:04 -0700 Subject: [PATCH] fix(group): report a member the server dropped instead of claiming it was added erchef accepts a group PUT that names an actor it cannot find, silently drops that name, and answers with the body as sent. group member add therefore printed "Added bob, ghost to group ..." and exited 0 when the ghost user never joined. After an add the command now reads the group back, reports only the members it holds, and fails naming any the server dropped so a typo or a missing actor is noticed. Signed-off-by: Tim Smith --- apps/cinc/cmd/group.go | 40 ++++++++++++++++++- apps/cinc/cmd/group_member_test.go | 22 ++++++++++ apps/cinc/cmd/group_test.go | 15 +++++++ integration/cincserverng/cincserverng_test.go | 3 -- 4 files changed, 75 insertions(+), 5 deletions(-) diff --git a/apps/cinc/cmd/group.go b/apps/cinc/cmd/group.go index e54dbef..06cd3c5 100644 --- a/apps/cinc/cmd/group.go +++ b/apps/cinc/cmd/group.go @@ -204,13 +204,28 @@ cinc group member remove admins alice`), if _, _, err := c.Groups.Update(cmd.Context(), current); err != nil { return err } + // erchef accepts a group PUT naming an actor that does not exist + // and silently drops it, answering with the body as sent. Read + // the group back so an add reports only what actually landed. + var dropped []string + if add { + if changed, dropped, err = keptMembers(cmd, c, group, memberKind(kind), changed); err != nil { + return err + } + } note := "" if len(unchanged) > 0 { note = fmt.Sprintf(" (%s %s it)", strings.Join(unchanged, ", "), memberState(add, len(unchanged) > 1, true)) } - fmt.Fprintf(out, "%s %s %s group %q%s\n", - cases(add, "Added", "Removed"), strings.Join(changed, ", "), preposition, group, note) + if len(changed) > 0 { + fmt.Fprintf(out, "%s %s %s group %q%s\n", + cases(add, "Added", "Removed"), strings.Join(changed, ", "), preposition, group, note) + } + if len(dropped) > 0 { + return fmt.Errorf("the server didn't add %s to group %q. Check that a %s by that name exists in this organization", + strings.Join(dropped, ", "), group, kind) + } return nil }, } @@ -218,6 +233,27 @@ cinc group member remove admins alice`), return cmd } +// keptMembers reads group back after an add and splits the names the add +// sent into those the group now holds and those the server dropped. +func keptMembers(cmd *cobra.Command, c *cinc.Client, group string, kind memberKind, added []string) (kept, dropped []string, err error) { + after, _, err := c.Groups.Get(cmd.Context(), group) + if err != nil { + return nil, nil, err + } + members, err := memberSlice(after, kind) + if err != nil { + return nil, nil, err + } + for _, name := range added { + if slices.Contains(*members, name) { + kept = append(kept, name) + } else { + dropped = append(dropped, name) + } + } + return kept, dropped, nil +} + // applyMemberChange adds or removes names from the actor list of the // given kind on group, in place. It reports which names it changed and // which were already as asked (already a member for add, not a member for diff --git a/apps/cinc/cmd/group_member_test.go b/apps/cinc/cmd/group_member_test.go index 4f8fb53..b3e07b1 100644 --- a/apps/cinc/cmd/group_member_test.go +++ b/apps/cinc/cmd/group_member_test.go @@ -129,3 +129,25 @@ func TestGroupMemberRemoveReportsOnlyRemovedMembers(t *testing.T) { t.Errorf("output = %q, want %q", out, want) } } + +// TestGroupMemberAddReportsDroppedMembers adds a real user and one the +// server has never heard of. erchef accepts the group PUT but silently +// drops the unknown name, so the command checks what the group holds +// afterwards: it reports only the member that was added, and fails naming +// the one that wasn't. +func TestGroupMemberAddReportsDroppedMembers(t *testing.T) { + var gotUsers []string + srv := groupMemberServerKnowing(t, "admins", []string{"alice"}, &gotUsers, []string{"alice", "bob"}) + cfg := groupMemberConfig(t, srv) + + out, err := runGroupMember(t, cfg, "add", "admins", "bob", "ghost") + if err == nil { + t.Fatalf("adding an unknown user succeeded: %q", out) + } + if !strings.Contains(err.Error(), "ghost") || strings.Contains(err.Error(), "bob") { + t.Errorf("error = %v, want it to name ghost and only ghost", err) + } + if want := "Added bob to group \"admins\"\n"; out != want { + t.Errorf("output = %q, want %q", out, want) + } +} diff --git a/apps/cinc/cmd/group_test.go b/apps/cinc/cmd/group_test.go index 119aa31..926ec2d 100644 --- a/apps/cinc/cmd/group_test.go +++ b/apps/cinc/cmd/group_test.go @@ -185,6 +185,14 @@ client_key = %q // groupMemberServer serves GET and PUT for a single group, recording // the actors.users slice from any PUT body. func groupMemberServer(t *testing.T, name string, users []string, gotUsers *[]string) *httptest.Server { + t.Helper() + return groupMemberServerKnowing(t, name, users, gotUsers, nil) +} + +// groupMemberServerKnowing is groupMemberServer for a server that knows +// only the users in known (nil means every name exists): like erchef, a PUT +// silently drops a member it cannot find, and later GETs show what it kept. +func groupMemberServerKnowing(t *testing.T, name string, users []string, gotUsers *[]string, known []string) *httptest.Server { t.Helper() mux := http.NewServeMux() mux.HandleFunc("/organizations/acme/groups/"+name, func(w http.ResponseWriter, r *http.Request) { @@ -200,6 +208,13 @@ func groupMemberServer(t *testing.T, name string, users []string, gotUsers *[]st } _ = json.NewDecoder(r.Body).Decode(&body) *gotUsers = body.Actors.Users + users = nil + for _, u := range body.Actors.Users { + if known == nil || slices.Contains(known, u) { + users = append(users, u) + } + } + // erchef answers with the body as sent, not what it stored. _ = json.NewEncoder(w).Encode(cinc.Group{GroupName: name, Name: name, Users: body.Actors.Users}) default: t.Errorf("unexpected method %q", r.Method) diff --git a/integration/cincserverng/cincserverng_test.go b/integration/cincserverng/cincserverng_test.go index 085c083..b87e759 100644 --- a/integration/cincserverng/cincserverng_test.go +++ b/integration/cincserverng/cincserverng_test.go @@ -75,9 +75,6 @@ func run(m *testing.M) int { Gaps: map[string]string{ "keys/client-edit-partial": "key PUT drops fields the body omits: https://github.com/cinc-project/cinc-server-ng/issues/163 (fixed on main by #202, not yet released)", "keys/client-edit-rename": "key PUT ignores a new name: https://github.com/cinc-project/cinc-server-ng/issues/163 (fixed on main by #202, not yet released)", - // v0.14.0 drops unknown members as erchef does; what remains is - // the CLI reporting a dropped member as added, fixed separately. - "groups/member-unknown": "group PUT dropping unknown members (https://github.com/cinc-project/cinc-server-ng/issues/186) is fixed; the CLI does not yet report a dropped member", }, } return m.Run()