diff --git a/README.md b/README.md index f538ad6..7d45607 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ group-propagator ================ -This service uses REST facades to Grouper and iRODS to copy group memberships from the former to the latter. +This service uses REST facades to the DE groups service and iRODS to copy group memberships from the former to the latter. diff --git a/client/datainfo/client.go b/client/datainfo/client.go index 76d41b1..3ed3f70 100644 --- a/client/datainfo/client.go +++ b/client/datainfo/client.go @@ -8,6 +8,7 @@ import ( "io" "net/http" "net/url" + "time" "github.com/cyverse-de/go-mod/restutils" "github.com/cyverse-de/group-propagator/logging" @@ -27,7 +28,12 @@ type DataInfoClient struct { DataInfoUser string } -var httpClient = http.Client{Transport: otelhttp.NewTransport(http.DefaultTransport)} +// The timeout keeps an unresponsive data-info from hanging the single AMQP +// consumer goroutine indefinitely. +var httpClient = http.Client{ + Transport: otelhttp.NewTransport(http.DefaultTransport), + Timeout: 30 * time.Second, +} func NewDataInfoClient(base, user string) *DataInfoClient { return &DataInfoClient{base, user} @@ -58,7 +64,7 @@ func (d *DataInfoClient) reqJSON(ctx context.Context, method, uri string, body i if err != nil { return errors.Wrap(err, "Failed requesting URL") } - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() if resp.StatusCode < 200 || resp.StatusCode > 299 { var e ServiceError diff --git a/client/groups/client.go b/client/groups/client.go index 476453e..9b49cb0 100644 --- a/client/groups/client.go +++ b/client/groups/client.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "net/url" + "time" "github.com/cyverse-de/go-mod/restutils" "github.com/cyverse-de/group-propagator/logging" @@ -27,41 +28,40 @@ type GroupsClient struct { GroupsID string } -var httpClient = http.Client{Transport: otelhttp.NewTransport(http.DefaultTransport)} +// The timeout keeps an unresponsive groups service from hanging the single +// AMQP consumer goroutine indefinitely. +var httpClient = http.Client{ + Transport: otelhttp.NewTransport(http.DefaultTransport), + Timeout: 30 * time.Second, +} func NewGroupsClient(base string, user string, name string) *GroupsClient { return &GroupsClient{GroupsBase: base, GroupsUser: user, DEUsersGroupName: name} } -func (c *GroupsClient) getDEUsersGroupID(ctx context.Context) (*group, error) { - ctx, span := otel.Tracer(otelName).Start(ctx, "getGroupID") +func (c *GroupsClient) getDEUsersGroup(ctx context.Context) (*Group, error) { + ctx, span := otel.Tracer(otelName).Start(ctx, "getDEUsersGroup") defer span.End() - fullURL, err := url.Parse(c.GroupsBase) + uri, err := c.uriPath(ctx, fmt.Sprintf("group_type=%s&name=%s", GroupTypeSystem, url.QueryEscape(c.DEUsersGroupName)), + "groups", "lookup") if err != nil { - return nil, errors.Wrap(err, "Failed to parse iplant-groups base URL") + return nil, errors.Wrap(err, "Failed to build the de-users lookup URL") } - fullURL = fullURL.JoinPath("groups", c.DEUsersGroupName) - - q := fullURL.Query() - q.Set("user", c.GroupsUser) - - fullURL.RawQuery = q.Encode() - var group group - err = c.getJSON(ctx, fullURL.String(), &group) - if err != nil { + var g Group + if err := c.getJSON(ctx, uri, &g); err != nil { return nil, errors.Wrap(err, "Failed to get group ID") } - return &group, nil + return &g, nil } func (c *GroupsClient) SetGroupsID(ctx context.Context) error { - groups, err := c.getDEUsersGroupID(ctx) + g, err := c.getDEUsersGroup(ctx) if err != nil { return err } - c.GroupsID = *groups.ID + c.GroupsID = g.ID return nil } @@ -89,10 +89,11 @@ func (c *GroupsClient) getJSON(ctx context.Context, uri string, target any) erro resp, err := httpClient.Do(req) if err != nil { return errors.Wrap(err, "Failed requesting URL") - } else if resp.StatusCode < 200 || resp.StatusCode > 299 { + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode < 200 || resp.StatusCode > 299 { return restutils.NewHTTPError(resp.StatusCode, fmt.Sprintf("GET %s returned %d", uri, resp.StatusCode)) } - defer resp.Body.Close() if target != nil { err = json.NewDecoder(resp.Body).Decode(target) @@ -103,56 +104,121 @@ func (c *GroupsClient) getJSON(ctx context.Context, uri string, target any) erro return err } -// Use status endpoint to check our iplant-groups URI +// Check reports whether the groups service is reachable and able to serve group +// data. The service answers its status endpoint even when its database is down, +// so a 200 alone is not enough to start against. func (c *GroupsClient) Check(ctx context.Context) error { uri, err := url.Parse(c.GroupsBase) if err != nil { - return errors.Wrap(err, "Failed to parse iplant-groups base URL") + return errors.Wrap(err, "Failed to parse groups base URL") } - uri.RawQuery = "expecting=iplant-groups" - - return c.getJSON(ctx, uri.String(), nil) + var status Status + if err := c.getJSON(ctx, uri.String(), &status); err != nil { + return err + } + if !status.Database { + return errors.New("the groups service cannot reach its database") + } + return nil } -// List groups under a provided prefix, using the REST service -func (c *GroupsClient) ListGroupsByPrefix(ctx context.Context, prefix, folder string) (GroupList, error) { - ctx, span := otel.Tracer(otelName).Start(ctx, "ListGroupsByPrefix") - defer span.End() +// pageSize is how many groups one listing request asks for. The service caps a +// single response at 1000, so anything larger would be silently trimmed. +const pageSize = 1000 + +// maxListPages bounds a listing crawl. At pageSize per page this covers orders +// of magnitude more groups than a deployment holds, so reaching it means the +// crawl is not making progress rather than that the deployment is large. +const maxListPages = 1000 + +// listGroups follows a group listing to its end, restricted to one group type +// when groupType is non-empty. +func (c *GroupsClient) listGroups(ctx context.Context, groupType string) ([]Group, error) { + // The offset advances by what each page actually held, and only an empty + // page ends the crawl. Keying either off the requested pageSize would + // silently truncate the listing if the service ever clamps responses + // below what we asked for. + var all []Group + seen := make(map[string]struct{}) + for pages := 0; ; pages++ { + // A listing that never empties and never contributes a new group would + // otherwise spin forever, and with the AMQP consumer's concurrency of 1 + // that halts all propagation while the accumulated slice grows. + if pages >= maxListPages { + return all, errors.Errorf( + "the group listing did not end after %d pages (%d groups); this usually means the "+ + "groups service is ignoring the offset parameter", maxListPages, len(all)) + } - var gs GroupList - var uri string - var err error + query := fmt.Sprintf("limit=%d&offset=%d", pageSize, len(all)) + if groupType != "" { + query = fmt.Sprintf("group_type=%s&%s", url.QueryEscape(groupType), query) + } + uri, err := c.uriPath(ctx, query, "groups") + if err != nil { + return all, err + } + log.Debugf("listGroups uri: %s", uri) - if folder != "" { - uri, err = c.uriPath(ctx, fmt.Sprintf("search=%s&folder=%s", prefix, folder), "groups") - } else { - uri, err = c.uriPath(ctx, fmt.Sprintf("search=%s", prefix), "groups") - } - log.Debugf("ListGroupsByPrefix uri: %s", uri) + var page GroupList + if err := c.getJSON(ctx, uri, &page); err != nil { + return all, err + } + if len(page.Groups) == 0 { + return all, nil + } - if err != nil { - return gs, err + fresh := 0 + for _, g := range page.Groups { + if _, dup := seen[g.ID]; dup { + continue + } + seen[g.ID] = struct{}{} + fresh++ + } + if fresh == 0 { + return all, errors.Errorf( + "the group listing repeated a page of %d groups at offset %d; this usually means the "+ + "groups service is ignoring the offset parameter", len(page.Groups), len(all)) + } + all = append(all, page.Groups...) } - - err = c.getJSON(ctx, uri, &gs) - return gs, err } -// Get the basic group information for a group from the REST service, given a name -func (c *GroupsClient) GetGroupByName(ctx context.Context, groupName string) (Group, error) { - ctx, span := otel.Tracer(otelName).Start(ctx, "GetGroupByName") +// ListAllGroups returns every group the service knows about, following +// pagination to the end. The groups service is already scoped to one +// deployment's group data, so there is no folder or prefix to filter on. +func (c *GroupsClient) ListAllGroups(ctx context.Context) ([]Group, error) { + ctx, span := otel.Tracer(otelName).Start(ctx, "ListAllGroups") defer span.End() - var g Group + return c.listGroups(ctx, "") +} - uri, err := c.uriPath(ctx, "", "groups", url.PathEscape(groupName)) +// VerifyAdminListing proves the configured user gets unfiltered group +// listings by checking that the de-users group appears in a system-type +// listing; SetGroupsID must have run first. The check exists because the +// service answers a non-admin with a 200 and an access-filtered page that +// carries no marker -- without it, groups would silently stop propagating. +func (c *GroupsClient) VerifyAdminListing(ctx context.Context) error { + ctx, span := otel.Tracer(otelName).Start(ctx, "VerifyAdminListing") + defer span.End() + + system, err := c.listGroups(ctx, GroupTypeSystem) if err != nil { - return g, err + return errors.Wrap(err, "Failed listing system groups") } - - err = c.getJSON(ctx, uri, &g) - return g, err + for _, g := range system { + if g.ID == c.GroupsID { + return nil + } + } + return errors.Errorf( + "the system group listing does not include %s (%s); this usually means the configured "+ + "groups user %q is not in the groups service's admin-users list, so listings are "+ + "access-filtered and propagation would silently miss groups", + c.DEUsersGroupName, c.GroupsID, c.GroupsUser) } // Get the basic group information for a group from the REST service, given an ID @@ -162,7 +228,7 @@ func (c *GroupsClient) GetGroupByID(ctx context.Context, groupID string) (Group, var g Group - uri, err := c.uriPath(ctx, "", "groups", "id", groupID) + uri, err := c.uriPath(ctx, "", "groups", url.PathEscape(groupID)) if err != nil { return g, err } @@ -171,17 +237,76 @@ func (c *GroupsClient) GetGroupByID(ctx context.Context, groupID string) (Group, return g, err } -// List members of a group using the REST service, given a name -func (c *GroupsClient) GetGroupMembers(ctx context.Context, groupName string) (GroupMembers, error) { - ctx, span := otel.Tracer(otelName).Start(ctx, "GetGroupMembers") +// List members of a group using the REST service, given an ID. Membership is +// keyed by ID rather than name because a nested group's member entry carries +// its own short name, which is not a lookup key on either backend. +func (c *GroupsClient) GetGroupMembersByID(ctx context.Context, groupID string) (GroupMembers, error) { + ctx, span := otel.Tracer(otelName).Start(ctx, "GetGroupMembersByID") defer span.End() + // Always paged, never asked for unbounded: the service refuses an unpaged + // listing of a group larger than its own cap rather than truncating it, and + // propagation needs the whole membership -- the data-info update replaces + // rather than merges, so a short list removes people from the iRODS group. + // + // Termination mirrors listGroups: the offset advances by what the pages + // actually held and a page contributing no new member ends the crawl. + // Stopping on a page shorter than pageSize would truncate silently against a + // service whose own cap is lower than what we ask for. var gm GroupMembers - uri, err := c.uriPath(ctx, "", "groups", url.PathEscape(groupName), "members") - if err != nil { - return gm, err + seen := make(map[string]struct{}) + for pages := 0; ; pages++ { + if pages >= maxListPages { + return gm, errors.Errorf( + "the member listing for group %s did not end after %d pages (%d members); this "+ + "usually means the groups service is ignoring the offset parameter", + groupID, maxListPages, len(gm.Members)) + } + + query := fmt.Sprintf("limit=%d&offset=%d", pageSize, len(gm.Members)) + uri, err := c.uriPath(ctx, query, "groups", url.PathEscape(groupID), "members") + if err != nil { + return gm, err + } + + var page GroupMembers + if err := c.getJSON(ctx, uri, &page); err != nil { + return gm, err + } + // Redaction describes the group, not the page: the membership was + // withheld rather than empty, and the caller must not read it as empty. + if page.Redacted { + return GroupMembers{Redacted: true}, nil + } + gm.Total = page.Total + + fresh := 0 + for _, m := range page.Members { + if _, dup := seen[m.ID]; dup { + continue + } + seen[m.ID] = struct{}{} + gm.Members = append(gm.Members, m) + fresh++ + } + if fresh == 0 { + break + } + // The reported total ends the crawl without a further request that would + // only come back empty. Under-reporting it cannot truncate the result: + // the check below fails when the two disagree in either direction. + if page.Total > 0 && len(gm.Members) >= page.Total { + break + } } - err = c.getJSON(ctx, uri, &gm) - return gm, err + // The service reports the group's whole membership alongside each page, so a + // crawl ending short of it assembled an incomplete list -- which must not + // reach data-info as though it were the whole one. + if gm.Total > 0 && len(gm.Members) != gm.Total { + return gm, errors.Errorf( + "collected %d of the %d members of group %s; refusing to propagate an incomplete membership", + len(gm.Members), gm.Total, groupID) + } + return gm, nil } diff --git a/client/groups/client_test.go b/client/groups/client_test.go new file mode 100644 index 0000000..15396c4 --- /dev/null +++ b/client/groups/client_test.go @@ -0,0 +1,438 @@ +package groups + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "slices" + "strconv" + "strings" + "testing" +) + +// The groups service caps a single listing at 1000, and production carries +// well over that, so a crawler that issues one unpaginated request propagates +// ACLs for the first page and silently ignores the rest. +func TestListAllGroupsPaginates(t *testing.T) { + tests := []struct { + name string + total int + want int + // The crawl stops only on an empty page, so a crawl over N full + // pages costs N+1 requests: the last one comes back empty. + wantRequests int + }{ + {name: "empty", total: 0, want: 0, wantRequests: 1}, + {name: "single short page", total: 17, want: 17, wantRequests: 2}, + {name: "exactly one full page", total: pageSize, want: pageSize, wantRequests: 2}, + {name: "just over one page", total: pageSize + 1, want: pageSize + 1, wantRequests: 3}, + {name: "production scale", total: 2583, want: 2583, wantRequests: 4}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/groups" { + http.NotFound(w, r) + return + } + requests++ + if requests > tt.wantRequests { + // Fail fast rather than hang: a client that never + // advances its offset would otherwise loop forever. + t.Errorf("request %d exceeds the %d expected; the client is probably not advancing its offset", requests, tt.wantRequests) + http.Error(w, "too many requests", http.StatusInternalServerError) + return + } + limit, _ := strconv.Atoi(r.URL.Query().Get("limit")) + offset, _ := strconv.Atoi(r.URL.Query().Get("offset")) + + page := []Group{} + for i := offset; i < offset+limit && i < tt.total; i++ { + page = append(page, Group{ID: fmt.Sprintf("group-%04d", i)}) + } + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(GroupList{Groups: page}); err != nil { + t.Errorf("encoding page: %v", err) + } + })) + defer srv.Close() + + c := NewGroupsClient(srv.URL, "de_grouper", "de-users") + got, err := c.ListAllGroups(context.Background()) + if err != nil { + t.Fatalf("ListAllGroups: %v", err) + } + if len(got) != tt.want { + t.Fatalf("got %d groups over %d requests, want %d", len(got), requests, tt.want) + } + if requests != tt.wantRequests { + t.Errorf("crawl took %d requests, want %d", requests, tt.wantRequests) + } + + // Every group exactly once, in order: an off-by-one in the offset + // arithmetic drops or repeats rows without changing the count. + for i, g := range got { + if want := fmt.Sprintf("group-%04d", i); g.ID != want { + t.Fatalf("group %d is %s, want %s", i, g.ID, want) + } + } + }) + } +} + +// The service caps how many groups one response may carry, and the cap is the +// server's to change. If it ever drops below what the client asks for, the +// crawl must keep paging by what actually came back rather than treating the +// clamped page as the final short one and silently truncating the listing. +func TestListAllGroupsHonorsServerCappedPages(t *testing.T) { + const ( + total = 1200 + serverCap = 500 + // 1200 groups at 500 per page is three data pages plus the empty + // terminator; anything past that means the offset is not advancing. + wantRequests = 4 + ) + + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/groups" { + http.NotFound(w, r) + return + } + requests++ + if requests > wantRequests { + // Fail fast rather than hang if the client ignores page progress. + t.Errorf("request %d exceeds the %d expected; the client is probably not advancing its offset", requests, wantRequests) + http.Error(w, "too many requests", http.StatusInternalServerError) + return + } + limit, _ := strconv.Atoi(r.URL.Query().Get("limit")) + if limit > serverCap { + limit = serverCap + } + offset, _ := strconv.Atoi(r.URL.Query().Get("offset")) + + page := []Group{} + for i := offset; i < offset+limit && i < total; i++ { + page = append(page, Group{ID: fmt.Sprintf("group-%04d", i)}) + } + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(GroupList{Groups: page}); err != nil { + t.Errorf("encoding page: %v", err) + } + })) + defer srv.Close() + + c := NewGroupsClient(srv.URL, "de_grouper", "de-users") + got, err := c.ListAllGroups(context.Background()) + if err != nil { + t.Fatalf("ListAllGroups: %v", err) + } + if len(got) != total { + t.Fatalf("got %d groups over %d requests, want %d", len(got), requests, total) + } + if requests != wantRequests { + t.Errorf("crawl took %d requests, want %d", requests, wantRequests) + } + for i, g := range got { + if want := fmt.Sprintf("group-%04d", i); g.ID != want { + t.Fatalf("group %d is %s, want %s", i, g.ID, want) + } + } +} + +// The crawl ends on an empty page, which assumes the service advances through +// the listing. A service that did not would keep answering with groups the +// crawl already has: with the AMQP consumer's concurrency of 1 that wedges the +// only propagation goroutine while the accumulated listing grows until the pod +// is OOM-killed, and nothing is logged. The crawl has to give up instead. +func TestListAllGroupsBoundsTheCrawl(t *testing.T) { + tests := []struct { + name string + // page answers a listing request made at the given offset. + page func(offset int) []Group + wantRequests int + }{ + { + name: "offset ignored entirely", + page: func(int) []Group { return []Group{{ID: "group-0000"}, {ID: "group-0001"}} }, + wantRequests: 2, + }, + { + name: "listing that never runs out", + page: func(offset int) []Group { return []Group{{ID: fmt.Sprintf("group-%06d", offset)}} }, + wantRequests: maxListPages, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/groups" { + http.NotFound(w, r) + return + } + requests++ + if requests > tt.wantRequests { + t.Errorf("request %d exceeds the %d expected; the crawl is unbounded", requests, tt.wantRequests) + http.Error(w, "too many requests", http.StatusInternalServerError) + return + } + offset, _ := strconv.Atoi(r.URL.Query().Get("offset")) + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(GroupList{Groups: tt.page(offset)}); err != nil { + t.Errorf("encoding page: %v", err) + } + })) + defer srv.Close() + + c := NewGroupsClient(srv.URL, "de_grouper", "de-users") + if _, err := c.ListAllGroups(context.Background()); err == nil { + t.Fatal("expected the crawl to give up, got no error") + } + if requests != tt.wantRequests { + t.Errorf("crawl took %d requests, want %d", requests, tt.wantRequests) + } + }) + } +} + +// The groups service answers a non-admin's listing with a 200 and an +// access-filtered page -- there is no marker distinguishing it from a complete +// one. A propagator running as such a user would crawl an empty-ish listing +// forever and groups would silently stop propagating, so startup has to prove +// admin standing by checking that de-users shows up in the same kind of +// listing the crawl uses. +func TestVerifyAdminListing(t *testing.T) { + tests := []struct { + name string + groups []Group + // serverCap is how many groups the fake will return in one response, + // standing in for a service whose own cap is smaller than what the + // check asks for; 0 means it answers with everything at once. + serverCap int + wantErr bool + }{ + { + name: "de-users visible", + groups: []Group{ + {ID: "abc123", Name: "de-users", GroupType: "system"}, + {ID: "def456", Name: "grouper-all", GroupType: "system"}, + }, + wantErr: false, + }, + { + name: "de-users past the first page", + groups: []Group{ + {ID: "def456", Name: "grouper-all", GroupType: "system"}, + {ID: "abc123", Name: "de-users", GroupType: "system"}, + }, + serverCap: 1, + wantErr: false, + }, + { + name: "listing access-filtered", + groups: []Group{ + {ID: "def456", Name: "grouper-all", GroupType: "system"}, + }, + wantErr: true, + }, + { + name: "listing empty", + groups: []Group{}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var gotQuery url.Values + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/groups" { + http.NotFound(w, r) + return + } + gotQuery = r.URL.Query() + limit, _ := strconv.Atoi(r.URL.Query().Get("limit")) + if tt.serverCap > 0 && limit > tt.serverCap { + limit = tt.serverCap + } + offset, _ := strconv.Atoi(r.URL.Query().Get("offset")) + + page := []Group{} + for i := offset; i < offset+limit && i < len(tt.groups); i++ { + page = append(page, tt.groups[i]) + } + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(GroupList{Groups: page}); err != nil { + t.Errorf("encoding listing: %v", err) + } + })) + defer srv.Close() + + c := NewGroupsClient(srv.URL, "de_grouper", "de-users") + c.GroupsID = "abc123" + + err := c.VerifyAdminListing(context.Background()) + if tt.wantErr && err == nil { + t.Fatal("expected an error for a listing without de-users, got nil") + } + if !tt.wantErr && err != nil { + t.Fatalf("VerifyAdminListing: %v", err) + } + // System groups are the smallest slice of the listing that must + // contain de-users, so the check filters to them. + if got := gotQuery.Get("group_type"); got != "system" { + t.Errorf("group_type is %q, want system", got) + } + }) + } +} + +func TestLookupDEUsersGroup(t *testing.T) { + var gotQuery string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/groups/lookup" { + http.NotFound(w, r) + return + } + gotQuery = r.URL.RawQuery + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(Group{ID: "abc123", Name: "de-users"}); err != nil { + t.Errorf("encoding group: %v", err) + } + })) + defer srv.Close() + + c := NewGroupsClient(srv.URL, "de_grouper", "de-users") + if err := c.SetGroupsID(context.Background()); err != nil { + t.Fatalf("SetGroupsID: %v", err) + } + if c.GroupsID != "abc123" { + t.Fatalf("GroupsID is %q, want abc123", c.GroupsID) + } + + q, err := url.ParseQuery(gotQuery) + if err != nil { + t.Fatalf("parsing query: %v", err) + } + // de-users is a system group, and the type is what keeps the lookup from + // matching a collaborator list that happens to be named de-users. + for key, want := range map[string]string{"group_type": "system", "name": "de-users", "user": "de_grouper"} { + if q.Get(key) != want { + t.Errorf("query %s is %q, want %q", key, q.Get(key), want) + } + } +} + +func TestGetGroupMembersByIDPagesTheListing(t *testing.T) { + member := func(i int) Subject { + return Subject{ID: fmt.Sprintf("u%04d", i), SourceID: SourceUser} + } + + t.Run("collects every page", func(t *testing.T) { + const total = 2500 + var offsets []int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + offset, _ := strconv.Atoi(r.URL.Query().Get("offset")) + limit, _ := strconv.Atoi(r.URL.Query().Get("limit")) + offsets = append(offsets, offset) + + members := []Subject{} + for i := offset; i < total && i < offset+limit; i++ { + members = append(members, member(i)) + } + if err := json.NewEncoder(w).Encode(GroupMembers{Members: members, Total: total}); err != nil { + t.Errorf("encoding page: %v", err) + } + })) + defer srv.Close() + + gm, err := NewGroupsClient(srv.URL, "de_grouper", "de-users"). + GetGroupMembersByID(context.Background(), "g1") + if err != nil { + t.Fatalf("GetGroupMembersByID: %v", err) + } + if len(gm.Members) != total { + t.Errorf("collected %d members, want %d", len(gm.Members), total) + } + if gm.Members[0].ID != "u0000" || gm.Members[total-1].ID != "u2499" { + t.Errorf("unexpected first/last member: %q, %q", gm.Members[0].ID, gm.Members[total-1].ID) + } + // No fourth request: the reported total ends the crawl. + want := []int{0, 1000, 2000} + if !slices.Equal(offsets, want) { + t.Errorf("requested offsets %v, want %v; the offset must advance by what the pages held", offsets, want) + } + }) + + t.Run("refuses a listing that ends short of the reported total", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + members := []Subject{} + if r.URL.Query().Get("offset") == "0" { + members = append(members, member(0)) + } + if err := json.NewEncoder(w).Encode(GroupMembers{Members: members, Total: 99}); err != nil { + t.Errorf("encoding page: %v", err) + } + })) + defer srv.Close() + + _, err := NewGroupsClient(srv.URL, "de_grouper", "de-users"). + GetGroupMembersByID(context.Background(), "g1") + if err == nil { + t.Fatal("an incomplete list must not be returned as though it were whole") + } + if !strings.Contains(err.Error(), "incomplete membership") { + t.Errorf("error %q does not name the incomplete membership", err) + } + }) + + t.Run("stops when a page contributes nothing new", func(t *testing.T) { + requests := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + requests++ + // Ignores offset entirely, replaying the same page forever. + if err := json.NewEncoder(w).Encode(GroupMembers{Members: []Subject{member(0), member(1)}}); err != nil { + t.Errorf("encoding page: %v", err) + } + })) + defer srv.Close() + + gm, err := NewGroupsClient(srv.URL, "de_grouper", "de-users"). + GetGroupMembersByID(context.Background(), "g1") + if err != nil { + t.Fatalf("GetGroupMembersByID: %v", err) + } + if len(gm.Members) != 2 { + t.Errorf("collected %d members, want 2; a replayed page must not accumulate duplicates", len(gm.Members)) + } + if requests != 2 { + t.Errorf("made %d requests, want 2; the crawl must stop rather than spin", requests) + } + }) + + t.Run("a redacted page is redaction, not an empty group", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if err := json.NewEncoder(w).Encode(GroupMembers{Members: []Subject{}, Redacted: true}); err != nil { + t.Errorf("encoding page: %v", err) + } + })) + defer srv.Close() + + gm, err := NewGroupsClient(srv.URL, "de_grouper", "de-users"). + GetGroupMembersByID(context.Background(), "g1") + if err != nil { + t.Fatalf("GetGroupMembersByID: %v", err) + } + if !gm.Redacted || len(gm.Members) != 0 { + t.Errorf("redacted=%v with %d members; want redacted with none", gm.Redacted, len(gm.Members)) + } + }) +} diff --git a/client/groups/model.go b/client/groups/model.go index 3b75266..596aaaa 100644 --- a/client/groups/model.go +++ b/client/groups/model.go @@ -1,5 +1,18 @@ package groups +// Subject source identifiers, matching the values the groups service sets on +// Subject.SourceID. +const ( + SourceUser = "ldap" + SourceGroup = "g:gsa" +) + +// GroupTypeSystem is the group type the DE's own internal groups carry, as +// opposed to the collaborator lists, teams, and communities users create. +const GroupTypeSystem = "system" + +// Subject is one member of a group: an LDAP user or, when SourceID is +// "g:gsa", a nested group whose ID is the nested group's own group ID. type Subject struct { ID string `json:"id"` Name string `json:"name"` @@ -13,25 +26,42 @@ type Subject struct { AttributeValues []string `json:"attribute_values"` } +// Group is the groups service's representation of a single group. type Group struct { - ID string `json:"id"` - Name string `json:"name"` - DisplayName string `json:"display_name"` - Type string `json:"type"` - Description string `json:"description"` - Extension string `json:"extension"` - DisplayExtension string `json:"display_extension"` - IDIndex string `json:"id_index"` -} // should we add the 'detail' here? + ID string `json:"id"` + Name string `json:"name"` + DisplayName string `json:"display_name"` + GroupType string `json:"group_type"` + Owner string `json:"owner"` + Description string `json:"description"` +} +// GroupList is one page of a group listing response. type GroupList struct { Groups []Group `json:"groups"` } +// GroupMembers is a group's member listing response. type GroupMembers struct { Members []Subject `json:"members"` + + // Redacted reports that the groups service withheld the member list + // because the group is public but its membership is not. The list is + // empty in that case but the group is not, so propagating it would strip + // every member from the iRODS group. + Redacted bool `json:"redacted"` + + // Total is the group's whole direct membership, which exceeds Members when + // the response carries one page of it. + Total int `json:"total"` } -type group struct { - ID *string `json:"id"` +// Status is the groups service's GET / response. Keycloak is deliberately not +// treated as fatal: without it names and email addresses degrade, but group +// membership -- the only thing this service reads -- still resolves. +type Status struct { + Service string `json:"service"` + Version string `json:"version"` + Database bool `json:"database"` + Keycloak bool `json:"keycloak"` } diff --git a/config/config.go b/config/config.go index 384cd56..57508ac 100644 --- a/config/config.go +++ b/config/config.go @@ -8,10 +8,9 @@ import ( ) type Config struct { - IplantGroupsBase string - IplantGroupsUser string - IplantGroupsFolderNamePrefix string - IplantGroupsPublicGroup string + GroupsBase string + GroupsUser string + DEUsersGroupName string DataInfoBase string IRODSUser string @@ -24,10 +23,9 @@ type Config struct { func NewFromViper(cfg *viper.Viper) (*Config, error) { c := &Config{ - IplantGroupsBase: cfg.GetString("iplant_groups.base"), - IplantGroupsUser: cfg.GetString("iplant_groups.user"), - IplantGroupsFolderNamePrefix: cfg.GetString("iplant_groups.folder_name_prefix"), - IplantGroupsPublicGroup: cfg.GetString("iplant_groups.public_group"), + GroupsBase: cfg.GetString("groups.base"), + GroupsUser: cfg.GetString("groups.user"), + DEUsersGroupName: cfg.GetString("groups.de_users_group"), DataInfoBase: cfg.GetString("data_info.base"), IRODSUser: cfg.GetString("irods.user"), @@ -48,17 +46,14 @@ func NewFromViper(cfg *viper.Viper) (*Config, error) { func (c *Config) Validate() error { var errorkeys []string - if c.IplantGroupsBase == "" { - errorkeys = append(errorkeys, "iplant_groups.base") + if c.GroupsBase == "" { + errorkeys = append(errorkeys, "groups.base") } - if c.IplantGroupsUser == "" { - errorkeys = append(errorkeys, "iplant_groups.user") + if c.GroupsUser == "" { + errorkeys = append(errorkeys, "groups.user") } - if c.IplantGroupsFolderNamePrefix == "" { - errorkeys = append(errorkeys, "iplant_groups.folder_name_prefix") - } - if c.IplantGroupsPublicGroup == "" { - errorkeys = append(errorkeys, "iplant_groups.public_group") + if c.DEUsersGroupName == "" { + errorkeys = append(errorkeys, "groups.de_users_group") } if c.DataInfoBase == "" { diff --git a/crawl.go b/crawl.go index d87f67f..224b87e 100644 --- a/crawl.go +++ b/crawl.go @@ -5,49 +5,57 @@ import ( "fmt" "github.com/cyverse-de/group-propagator/client/groups" - "github.com/cyverse-de/messaging/v9" "github.com/pkg/errors" "go.opentelemetry.io/otel" ) +// groupPublisher is the part of the messaging client the crawl uses. The +// interface lives here because its implementation is a third-party package. +type groupPublisher interface { + PublishContext(ctx context.Context, key string, body []byte) error +} + type Crawler struct { - groupsClient *groups.GroupsClient - groupBaseFolder string - publicGroup string + groupsClient *groups.GroupsClient + deUsersGroupID string // maybe a data-info client too for irods crawling? - publishClient *messaging.Client + publishClient groupPublisher } -func NewCrawler(groupsClient *groups.GroupsClient, groupBaseFolder, publicGroup string, publishClient *messaging.Client) *Crawler { +func NewCrawler(groupsClient *groups.GroupsClient, deUsersGroupID string, publishClient groupPublisher) *Crawler { return &Crawler{ - groupsClient: groupsClient, - groupBaseFolder: groupBaseFolder, - publicGroup: publicGroup, - publishClient: publishClient, + groupsClient: groupsClient, + deUsersGroupID: deUsersGroupID, + publishClient: publishClient, } } -// Request all groups within the configured base folder/prefix -// This handles new groups and existing groups with updated memberships -// It does not send messages for groups that no longer exist in Grouper -func (c *Crawler) CrawlGrouperGroups(ctx context.Context) error { - ctx, span := otel.Tracer(otelName).Start(ctx, "CrawlGrouperGroups") +// Request propagation of every group users can create, skipping the DE's own +// internal groups. The service holds one deployment's group data, so there is +// no folder or prefix left to scope by. This handles new groups and existing +// groups with updated memberships; it does not send messages for groups that no +// longer exist. +func (c *Crawler) CrawlGroups(ctx context.Context) error { + ctx, span := otel.Tracer(otelName).Start(ctx, "CrawlGroups") defer span.End() - gs, err := c.groupsClient.ListGroupsByPrefix(ctx, c.groupBaseFolder, c.groupBaseFolder) // same thing passed twice: as prefix for group search and for folder to search within + gs, err := c.groupsClient.ListAllGroups(ctx) if err != nil { - return errors.Wrap(err, "Failed listing groups by prefix") + return errors.Wrap(err, "Failed listing groups") } var overallError error - for _, group := range gs.Groups { - if group.ID != c.publicGroup { - err = c.publishClient.PublishContext(ctx, fmt.Sprintf("index.group.%s", group.ID), []byte{}) + for _, group := range gs { + // System groups (de-users among them) are internal bookkeeping with no + // iRODS counterpart. Propagating one would create an @grouper- + // iRODS group that nothing else knows about or ever removes. + if group.GroupType == groups.GroupTypeSystem || group.ID == c.deUsersGroupID { + continue } - if err != nil { + if err := c.publishClient.PublishContext(ctx, fmt.Sprintf("index.group.%s", group.ID), []byte{}); err != nil { log.Error(errors.Wrap(err, fmt.Sprintf("Error publishing message for group %s", group.ID))) overallError = err } diff --git a/crawl_test.go b/crawl_test.go new file mode 100644 index 0000000..441cc7f --- /dev/null +++ b/crawl_test.go @@ -0,0 +1,110 @@ +package main + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "slices" + "strconv" + "testing" + + "github.com/cyverse-de/group-propagator/client/groups" +) + +// recordingPublisher stands in for the AMQP client, keeping the routing keys +// the crawl asked to publish. +type recordingPublisher struct { + keys []string +} + +func (p *recordingPublisher) PublishContext(ctx context.Context, key string, body []byte) error { + p.keys = append(p.keys, key) + return nil +} + +// newListingServer serves a paginated /groups listing over the given groups. +func newListingServer(t *testing.T, gs []groups.Group) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/groups" { + http.NotFound(w, r) + return + } + limit, _ := strconv.Atoi(r.URL.Query().Get("limit")) + offset, _ := strconv.Atoi(r.URL.Query().Get("offset")) + + page := []groups.Group{} + for i := offset; i < offset+limit && i < len(gs); i++ { + page = append(page, gs[i]) + } + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(groups.GroupList{Groups: page}); err != nil { + t.Errorf("encoding listing: %v", err) + } + })) +} + +// The listing carries the DE's own internal groups alongside the ones users +// create. Publishing for a system group would have the propagator create an +// @grouper- iRODS group for it -- an object that never existed while the +// listing was scoped by folder prefix, and that nothing else removes. +func TestCrawlGroupsSkipsSystemGroups(t *testing.T) { + const deUsers = "1111111111111111111111111111aaaa" + + tests := []struct { + name string + groups []groups.Group + want []string + }{ + { + name: "user-created groups only", + groups: []groups.Group{ + {ID: "aaaa1111", Name: "Field Team", GroupType: "team"}, + {ID: "bbbb2222", Name: "default", GroupType: "collaborator_list"}, + {ID: "cccc3333", Name: "Genomics", GroupType: "community"}, + }, + want: []string{"index.group.aaaa1111", "index.group.bbbb2222", "index.group.cccc3333"}, + }, + { + name: "system groups mixed in", + groups: []groups.Group{ + {ID: deUsers, Name: "de-users", GroupType: "system"}, + {ID: "dddd4444", Name: "grouper-all", GroupType: "system"}, + {ID: "aaaa1111", Name: "Field Team", GroupType: "team"}, + }, + want: []string{"index.group.aaaa1111"}, + }, + { + name: "de-users skipped by ID as well as by type", + groups: []groups.Group{ + {ID: deUsers, Name: "de-users", GroupType: "team"}, + }, + want: []string{}, + }, + { + name: "nothing but system groups", + groups: []groups.Group{ + {ID: "dddd4444", Name: "grouper-all", GroupType: "system"}, + }, + want: []string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + srv := newListingServer(t, tt.groups) + defer srv.Close() + + publisher := &recordingPublisher{} + crawler := NewCrawler(groups.NewGroupsClient(srv.URL, "de_grouper", "de-users"), deUsers, publisher) + + if err := crawler.CrawlGroups(context.Background()); err != nil { + t.Fatalf("CrawlGroups: %v", err) + } + if !slices.Equal(publisher.keys, tt.want) { + t.Fatalf("published %v, want %v", publisher.keys, tt.want) + } + }) + } +} diff --git a/main.go b/main.go index 62ed09f..93f8038 100644 --- a/main.go +++ b/main.go @@ -36,11 +36,10 @@ amqp: name: de type: topic -iplant_groups: - base: "http://iplant-groups" - user: GrouperSystem - folder_name_prefix: "iplant:de:notprod" - public_group: "iplant:de:notprod" +groups: + base: "http://groups" + user: de_grouper + de_users_group: de-users data_info: base: "http://data-info" @@ -121,13 +120,13 @@ func main() { go listenClient.Listen() // Create clients - gc := groups.NewGroupsClient(configuration.IplantGroupsBase, configuration.IplantGroupsUser, configuration.IplantGroupsPublicGroup) + gc := groups.NewGroupsClient(configuration.GroupsBase, configuration.GroupsUser, configuration.DEUsersGroupName) err = gc.Check(context.Background()) if err != nil { - log.Fatal(errors.Wrap(err, "Couldn't ping iplant-groups")) + log.Fatal(errors.Wrap(err, "Couldn't ping the groups service")) } else { - log.Info("Pinged iplant-groups successfully") + log.Info("Pinged the groups service successfully") } err = gc.SetGroupsID(context.Background()) @@ -137,6 +136,15 @@ func main() { log.Info("Group information retrieved successfully") } + // A non-admin groups user gets access-filtered listings with no error + // anywhere, so prove admin standing now rather than propagate nothing. + err = gc.VerifyAdminListing(context.Background()) + if err != nil { + log.Fatal(errors.Wrap(err, "Groups service admin check failed")) + } else { + log.Info("Verified that the groups user sees unfiltered group listings") + } + dc := datainfo.NewDataInfoClient(configuration.DataInfoBase, configuration.IRODSUser) err = dc.Check(context.Background()) if err != nil { @@ -146,7 +154,7 @@ func main() { } propagator := NewPropagator(gc, "@grouper-", dc) - crawler := NewCrawler(gc, configuration.IplantGroupsFolderNamePrefix, gc.GroupsID, publishClient) + crawler := NewCrawler(gc, gc.GroupsID, publishClient) queueName := getQueueName(configuration.AMQPQueuePrefix) listenClient.AddConsumerMulti( @@ -158,7 +166,7 @@ func main() { var err error log.Tracef("Got message: %s", del.RoutingKey) if del.RoutingKey == "index.all" || del.RoutingKey == "index.groups" { - err = crawler.CrawlGrouperGroups(ctx) + err = crawler.CrawlGroups(ctx) } else if strings.HasPrefix(del.RoutingKey, "index.group.") { groupID := del.RoutingKey[len("index.group."):] err = propagator.PropagateGroupById(ctx, groupID) diff --git a/propagate.go b/propagate.go index 0e80390..49b3e46 100644 --- a/propagate.go +++ b/propagate.go @@ -13,9 +13,9 @@ import ( ) // To propagate a group: -// * Fetch group details and members via iplant-groups -// -> get a model.GrouperGroup and model.GrouperGroupMembers, probably -// * Determine iRODS group name (@grouper-) +// * Fetch group details and members via the groups service +// -> get a groups.Group and groups.GroupMembers +// * Determine iRODS group name (@grouper-) // * Create or update group with proper membership list via data-info, potentially validating users/etc. type Propagator struct { @@ -37,29 +37,85 @@ func NewPropagator(groupsClient *groups.GroupsClient, groupPrefix string, dataIn } } -func (p *Propagator) getGroupMembers(ctx context.Context, groupName string) ([]string, error) { +func (p *Propagator) getGroupMembers(ctx context.Context, groupID string) ([]string, error) { + m, err := p.getGroupMembersVisiting(ctx, groupID, map[string]struct{}{groupID: {}}) + if err != nil { + return nil, err + } + return dedupeMembers(m), nil +} + +// dedupeMembers keeps the first occurrence of each member. A user reached both +// directly and through a subgroup appears once per path, which data-info would +// take at face value and the propagation log would report as a member count +// larger than the group has. +func dedupeMembers(members []string) []string { + seen := make(map[string]struct{}, len(members)) + deduped := make([]string, 0, len(members)) + for _, m := range members { + if _, dup := seen[m]; dup { + continue + } + seen[m] = struct{}{} + deduped = append(deduped, m) + } + return deduped +} + +// getGroupMembersVisiting is the recursive body of getGroupMembers. The +// visited set keeps a membership cycle (impossible via the service API, but +// reachable by editing the database directly) from recursing forever; as a +// side effect it also fetches a subgroup shared by two parents only once. +func (p *Propagator) getGroupMembersVisiting(ctx context.Context, groupID string, visited map[string]struct{}) ([]string, error) { ctx, span := otel.Tracer(otelName).Start(ctx, "getGroupMembers") defer span.End() var m []string - members, err := p.groupsClient.GetGroupMembers(ctx, groupName) + members, err := p.groupsClient.GetGroupMembersByID(ctx, groupID) if err != nil { - return m, errors.Wrapf(err, "Failed fetching Grouper group members for %s", groupName) + return m, errors.Wrapf(err, "Failed fetching group members for %s", groupID) + } + + // A withheld list is empty but the group is not. Propagating it would PUT + // an empty member list to data-info, which replaces rather than merges, and + // silently strip the iRODS group -- logged as an unremarkable "0 members". + // This means the configured groups user is not an administrative account of + // the groups service; it must be, or it cannot see membership at all. + if members.Redacted { + return m, errors.Errorf( + "groups service withheld the member list for %s; the configured groups.user is not "+ + "an admin of the groups service, so propagating would erase the iRODS group", groupID) } for _, member := range members.Members { - if member.SourceID == "ldap" { + switch member.SourceID { + case groups.SourceUser: m = append(m, member.ID) - } else if member.SourceID == "g:gsa" { - // this is a group that is a member of a group - submem, err := p.getGroupMembers(ctx, member.Name) + case groups.SourceGroup: + // A nested group. Its subject ID is the nested group's own group ID, + // so the recursion stays keyed by ID all the way down. + // Skipping an already-visited group is deliberately silent: a + // shared subgroup (diamond) is legitimate, and its members were + // already collected the first time through. + if _, seen := visited[member.ID]; seen { + continue + } + visited[member.ID] = struct{}{} + submem, err := p.getGroupMembersVisiting(ctx, member.ID, visited) if err != nil { - return m, errors.Wrapf(err, "Failed recursing to fetch members of %s", member.Name) + return m, errors.Wrapf(err, "Failed recursing to fetch members of %s (%s)", member.Name, member.ID) } m = append(m, submem...) - } else { - log.Errorf("Could not add group member %+v", member) + default: + // Skipping the member instead would PUT a short list to data-info, + // which replaces rather than merges, dropping people from the iRODS + // group under a log line that reads like a successful run. + return nil, errors.Errorf( + "member %s (%s) of group %s has the unexpected source id %q; this usually means the "+ + "groups service gained a subject source this propagator does not know how to "+ + "resolve, and propagating without the member would remove it from the iRODS group", + member.Name, member.ID, groupID, member.SourceID) } } @@ -86,12 +142,12 @@ func (p *Propagator) PropagateGroupById(ctx context.Context, groupID string) err } return err } else if err != nil { - return errors.Wrap(err, "Failed fetching Grouper group by ID") + return errors.Wrap(err, "Failed fetching group by ID") } else if groupID != g.ID { - return errors.New(fmt.Sprintf("Fetched Grouper group has an ID of %s, but was fetched using the ID %s", g.ID, groupID)) + return errors.Errorf("Fetched group has an ID of %s, but was fetched using the ID %s", g.ID, groupID) } - irodsMembers, err := p.getGroupMembers(ctx, g.Name) + irodsMembers, err := p.getGroupMembers(ctx, groupID) if err != nil { return errors.Wrap(err, "Failed getting group members") } diff --git a/propagate_test.go b/propagate_test.go new file mode 100644 index 0000000..36055e7 --- /dev/null +++ b/propagate_test.go @@ -0,0 +1,356 @@ +package main + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "slices" + "sort" + "testing" + + "github.com/cyverse-de/group-propagator/client/groups" +) + +// memberFixture describes one group's membership in the fake groups service. +type memberFixture struct { + id string + name string + sourceID string +} + +// newGroupsServer serves member listings keyed by group ID only. A request for +// any other path 404s, so a recursion that follows a nested group's name rather +// than its ID fails the test rather than quietly returning short membership. +// Requests are hard-capped so a recursion that stops terminating (e.g. on a +// membership cycle) fails the test quickly instead of hanging it. +func newGroupsServer(t *testing.T, membership map[string][]memberFixture) *httptest.Server { + t.Helper() + mux := http.NewServeMux() + for groupID, members := range membership { + subjects := make([]groups.Subject, 0, len(members)) + for _, m := range members { + subjects = append(subjects, groups.Subject{ID: m.id, Name: m.name, SourceID: m.sourceID}) + } + body := groups.GroupMembers{Members: subjects} + mux.HandleFunc(fmt.Sprintf("/groups/%s/members", groupID), func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(body); err != nil { + t.Errorf("encoding member listing: %v", err) + } + }) + } + + const maxRequests = 25 + var requests int + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + if requests > maxRequests { + t.Errorf("more than %d requests; the recursion is probably not terminating", maxRequests) + http.Error(w, "too many requests", http.StatusInternalServerError) + return + } + mux.ServeHTTP(w, r) + })) +} + +func TestGetGroupMembersFollowsNestingByID(t *testing.T) { + // Field Team -> Genomics Lab -> msmith's default, mirroring the local test + // dataset: lchen is only reachable at depth 2. + const ( + fieldTeam = "1111111111111111111111111111aaaa" + genomicsLab = "2222222222222222222222222222bbbb" + msmithList = "3333333333333333333333333333cccc" + ) + + tests := []struct { + name string + groupID string + want []string + }{ + { + name: "two levels of nesting", + groupID: fieldTeam, + want: []string{"lchen", "msmith", "rpatel"}, + }, + { + name: "one level of nesting", + groupID: genomicsLab, + want: []string{"lchen", "rpatel"}, + }, + { + name: "no nesting", + groupID: msmithList, + want: []string{"lchen"}, + }, + } + + membership := map[string][]memberFixture{ + fieldTeam: { + {id: "msmith", name: "msmith", sourceID: "ldap"}, + {id: genomicsLab, name: "Genomics Lab", sourceID: "g:gsa"}, + }, + genomicsLab: { + {id: "rpatel", name: "rpatel", sourceID: "ldap"}, + {id: msmithList, name: "default", sourceID: "g:gsa"}, + }, + msmithList: { + {id: "lchen", name: "lchen", sourceID: "ldap"}, + }, + } + + srv := newGroupsServer(t, membership) + defer srv.Close() + + client := groups.NewGroupsClient(srv.URL, "de_grouper", "de-users") + propagator := NewPropagator(client, "@grouper-", nil) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := propagator.getGroupMembers(context.Background(), tt.groupID) + if err != nil { + t.Fatalf("getGroupMembers(%s): %v", tt.groupID, err) + } + sort.Strings(got) + if len(got) != len(tt.want) { + t.Fatalf("got %v, want %v", got, tt.want) + } + for i := range got { + if got[i] != tt.want[i] { + t.Fatalf("got %v, want %v", got, tt.want) + } + } + }) + } +} + +// A membership cycle (A contains B contains A) can't be built through the +// groups service API, but it is reachable by editing the database directly. +// The recursion must terminate on one and return the union of user members; +// with the AMQP consumer's concurrency of 1, an unterminated recursion would +// halt all propagation. A revisited group is skipped silently rather than +// treated as an error, because the same shape occurs legitimately in a +// diamond (two groups sharing a subgroup). +func TestGetGroupMembersTerminatesOnCycle(t *testing.T) { + const ( + groupA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaa1111" + groupB = "bbbbbbbbbbbbbbbbbbbbbbbbbbbb2222" + ) + + membership := map[string][]memberFixture{ + groupA: { + {id: "asturm", name: "asturm", sourceID: "ldap"}, + {id: groupB, name: "Group B", sourceID: "g:gsa"}, + }, + groupB: { + {id: "bcarter", name: "bcarter", sourceID: "ldap"}, + {id: groupA, name: "Group A", sourceID: "g:gsa"}, + }, + } + + srv := newGroupsServer(t, membership) + defer srv.Close() + + client := groups.NewGroupsClient(srv.URL, "de_grouper", "de-users") + propagator := NewPropagator(client, "@grouper-", nil) + + got, err := propagator.getGroupMembers(context.Background(), groupA) + if err != nil { + t.Fatalf("getGroupMembers(%s): %v", groupA, err) + } + sort.Strings(got) + want := []string{"asturm", "bcarter"} + if len(got) != len(want) { + t.Fatalf("got %v, want %v", got, want) + } + for i := range got { + if got[i] != want[i] { + t.Fatalf("got %v, want %v", got, want) + } + } +} + +// A redacted member list must abort propagation rather than be treated as an +// empty group. data-info's member update is a replace, so propagating an empty +// list strips the iRODS group -- and the log line for that ("with 0 members") +// is indistinguishable from the benign case where iRODS has no account for the +// members. The only signal is this refusal. +func TestGetGroupMembersRefusesRedactedList(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/groups/g1/members", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + // Exactly what the groups service returns to a caller that is not an + // admin, for a public group whose membership is not public. + if err := json.NewEncoder(w).Encode(groups.GroupMembers{ + Members: []groups.Subject{}, + Redacted: true, + }); err != nil { + t.Errorf("encoding member listing: %v", err) + } + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + p := &Propagator{groupsClient: groups.NewGroupsClient(srv.URL, "de_grouper", "de-users")} + + members, err := p.getGroupMembers(context.Background(), "g1") + if err == nil { + t.Fatalf("expected a refusal, got %d members and no error", len(members)) + } + if len(members) != 0 { + t.Errorf("no members should be returned alongside the refusal, got %d", len(members)) + } +} + +// A member whose source the propagator cannot resolve has to abort the +// propagation. Logging and skipping it hands data-info a short list, and the +// member update replaces rather than merges, so the unresolved member is +// dropped from the iRODS group under a log line that reads like an ordinary +// successful run -- the same failure the redacted-list refusal exists to stop. +func TestGetGroupMembersRefusesUnknownSource(t *testing.T) { + const ( + parent = "4444444444444444444444444444dddd" + child = "5555555555555555555555555555eeee" + ) + + tests := []struct { + name string + membership map[string][]memberFixture + want []string + wantErr bool + }{ + { + name: "known sources only", + membership: map[string][]memberFixture{ + parent: { + {id: "asturm", name: "asturm", sourceID: "ldap"}, + {id: child, name: "Genomics Lab", sourceID: "g:gsa"}, + }, + child: {{id: "bcarter", name: "bcarter", sourceID: "ldap"}}, + }, + want: []string{"asturm", "bcarter"}, + }, + { + name: "unrecognized source id", + membership: map[string][]memberFixture{ + parent: { + {id: "asturm", name: "asturm", sourceID: "ldap"}, + {id: "svc-account", name: "svc-account", sourceID: "jdbc"}, + }, + }, + wantErr: true, + }, + { + name: "missing source id", + membership: map[string][]memberFixture{ + parent: {{id: "asturm", name: "asturm", sourceID: ""}}, + }, + wantErr: true, + }, + { + name: "unrecognized source id inside a nested group", + membership: map[string][]memberFixture{ + parent: {{id: child, name: "Genomics Lab", sourceID: "g:gsa"}}, + child: {{id: "svc-account", name: "svc-account", sourceID: "jdbc"}}, + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + srv := newGroupsServer(t, tt.membership) + defer srv.Close() + + client := groups.NewGroupsClient(srv.URL, "de_grouper", "de-users") + propagator := NewPropagator(client, "@grouper-", nil) + + got, err := propagator.getGroupMembers(context.Background(), parent) + if tt.wantErr { + if err == nil { + t.Fatalf("expected a refusal, got %v and no error", got) + } + if len(got) != 0 { + t.Errorf("no members should be returned alongside the refusal, got %d", len(got)) + } + return + } + if err != nil { + t.Fatalf("getGroupMembers(%s): %v", parent, err) + } + if !slices.Equal(got, tt.want) { + t.Fatalf("got %v, want %v", got, tt.want) + } + }) + } +} + +// Nesting reaches the same user by more than one path whenever someone belongs +// to a group and to one of its subgroups. The visited set only keeps each group +// from being fetched twice, so the member list itself has to be deduplicated: +// otherwise data-info is handed repeats and the propagation log reports more +// members than the group has. +func TestGetGroupMembersDeduplicates(t *testing.T) { + const ( + parent = "6666666666666666666666666666ffff" + left = "7777777777777777777777777777aaaa" + right = "8888888888888888888888888888bbbb" + ) + + tests := []struct { + name string + membership map[string][]memberFixture + // Order is first occurrence in the crawl, so a change here is a change + // in what data-info is handed. + want []string + }{ + { + name: "member of both a group and its subgroup", + membership: map[string][]memberFixture{ + parent: { + {id: "msmith", name: "msmith", sourceID: "ldap"}, + {id: left, name: "Genomics Lab", sourceID: "g:gsa"}, + }, + left: { + {id: "msmith", name: "msmith", sourceID: "ldap"}, + {id: "rpatel", name: "rpatel", sourceID: "ldap"}, + }, + }, + want: []string{"msmith", "rpatel"}, + }, + { + name: "two subgroups sharing a member", + membership: map[string][]memberFixture{ + parent: { + {id: left, name: "Genomics Lab", sourceID: "g:gsa"}, + {id: right, name: "Field Team", sourceID: "g:gsa"}, + }, + left: {{id: "rpatel", name: "rpatel", sourceID: "ldap"}}, + right: { + {id: "rpatel", name: "rpatel", sourceID: "ldap"}, + {id: "lchen", name: "lchen", sourceID: "ldap"}, + }, + }, + want: []string{"rpatel", "lchen"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + srv := newGroupsServer(t, tt.membership) + defer srv.Close() + + client := groups.NewGroupsClient(srv.URL, "de_grouper", "de-users") + propagator := NewPropagator(client, "@grouper-", nil) + + got, err := propagator.getGroupMembers(context.Background(), parent) + if err != nil { + t.Fatalf("getGroupMembers(%s): %v", parent, err) + } + if !slices.Equal(got, tt.want) { + t.Fatalf("got %v, want %v", got, tt.want) + } + }) + } +}