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()