From d48f350c1ef3914b266b33385325eeaafcae5440 Mon Sep 17 00:00:00 2001 From: John Wregglesworth Date: Tue, 4 Aug 2026 12:03:07 -0700 Subject: [PATCH 01/14] Recurse into nested groups by ID rather than by name getGroupMembers followed member.Name when descending into a nested group. That works against iplant-groups only because a group member's name is its full colon-delimited Grouper path, which doubles as a lookup key. The groups service names a group by its short name and keys membership by ID, so the same recursion would 404 on the first nested group and fail the propagation. A member entry already carries the nested group's own ID in its subject ID, and iplant-groups has served /groups/id/:id/members all along, so keying the whole recursion on IDs works against both backends. Verified against the local cluster's iplant-groups: the depth-2 chain Field Team -> Genomics Lab -> msmith's default list returns the same three members before and after. Adds the repo's first test, covering nesting at depth 0, 1, and 2 against a fake service that serves by-ID lookups only, so a regression to name-based recursion fails rather than silently returning short membership. Co-Authored-By: Claude Opus 5 --- client/groups/client.go | 10 ++-- propagate.go | 15 +++--- propagate_test.go | 113 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 127 insertions(+), 11 deletions(-) create mode 100644 propagate_test.go diff --git a/client/groups/client.go b/client/groups/client.go index 476453e..f03c9c7 100644 --- a/client/groups/client.go +++ b/client/groups/client.go @@ -171,13 +171,15 @@ 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() var gm GroupMembers - uri, err := c.uriPath(ctx, "", "groups", url.PathEscape(groupName), "members") + uri, err := c.uriPath(ctx, "", "groups", "id", url.PathEscape(groupID), "members") if err != nil { return gm, err } diff --git a/propagate.go b/propagate.go index 0e80390..51b6fbf 100644 --- a/propagate.go +++ b/propagate.go @@ -37,25 +37,26 @@ 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) { 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) } for _, member := range members.Members { if member.SourceID == "ldap" { 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) + // 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. + submem, err := p.getGroupMembers(ctx, member.ID) 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 { @@ -91,7 +92,7 @@ func (p *Propagator) PropagateGroupById(ctx context.Context, groupID string) err return errors.New(fmt.Sprintf("Fetched Grouper 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..74cb931 --- /dev/null +++ b/propagate_test.go @@ -0,0 +1,113 @@ +package main + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "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, which is the +// one lookup both iplant-groups and the groups service agree on. A request for +// any other path 404s, so a recursion that follows names rather than IDs fails +// the test rather than quietly returning short results. +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/id/%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) + } + }) + } + return httptest.NewServer(mux) +} + +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) + } + } + }) + } +} From 7559b260d0a7f34b9d8aedff6461c404974b9d61 Mon Sep 17 00:00:00 2001 From: John Wregglesworth Date: Tue, 4 Aug 2026 12:08:05 -0700 Subject: [PATCH 02/14] Read groups from the groups service instead of iplant-groups Repoints the client at the groups service's routes: /groups/lookup for the de-users group, /groups/:id and /groups/:id/members in place of the /groups/id/... forms, and GET / for the status check. The status check now requires the reported database connectivity rather than accepting any 200, because the groups service answers its status endpoint even when it cannot reach group storage. Keycloak connectivity is deliberately not fatal: without it names degrade, but membership -- all this service reads -- still resolves. The crawler now paginates. It listed groups in one unpaginated request, which the groups service caps at 1000; production carries 2,583, so the crawl would have propagated ACLs for the first page and silently ignored the rest. The folder/prefix scoping is gone with it: the service holds exactly one deployment's group data, so there is nothing left to filter on. Config keys move from iplant_groups.* to groups.*, dropping folder_name_prefix and replacing public_group with de_users_group, which is now a bare name resolved by type rather than a colon-delimited path. GetGroupByName is deleted -- it was already unreachable, and the groups service has no by-name lookup for it to use. Co-Authored-By: Claude Opus 5 --- client/groups/client.go | 102 +++++++++++++++------------------ client/groups/client_test.go | 107 +++++++++++++++++++++++++++++++++++ client/groups/model.go | 26 +++++---- config/config.go | 29 ++++------ crawl.go | 31 +++++----- main.go | 10 ++-- propagate_test.go | 9 ++- 7 files changed, 205 insertions(+), 109 deletions(-) create mode 100644 client/groups/client_test.go diff --git a/client/groups/client.go b/client/groups/client.go index f03c9c7..c4e8a3e 100644 --- a/client/groups/client.go +++ b/client/groups/client.go @@ -33,35 +33,29 @@ 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=system&name=%s", 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 } @@ -103,56 +97,54 @@ 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) -} - -// 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() - - var gs GroupList - var uri string - var err error - - 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") + var status Status + if err := c.getJSON(ctx, uri.String(), &status); err != nil { + return err } - log.Debugf("ListGroupsByPrefix uri: %s", uri) - - if err != nil { - return gs, err + if !status.Database { + return errors.New("the groups service cannot reach its database") } - - err = c.getJSON(ctx, uri, &gs) - return gs, err + return nil } -// 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") +// 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 + +// 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 + var all []Group + for offset := 0; ; offset += pageSize { + uri, err := c.uriPath(ctx, fmt.Sprintf("limit=%d&offset=%d", pageSize, offset), "groups") + if err != nil { + return all, err + } + log.Debugf("ListAllGroups uri: %s", uri) - uri, err := c.uriPath(ctx, "", "groups", url.PathEscape(groupName)) - if err != nil { - return g, err - } + var page GroupList + if err := c.getJSON(ctx, uri, &page); err != nil { + return all, err + } - err = c.getJSON(ctx, uri, &g) - return g, err + all = append(all, page.Groups...) + if len(page.Groups) < pageSize { + return all, nil + } + } } // Get the basic group information for a group from the REST service, given an ID @@ -162,7 +154,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 } @@ -179,7 +171,7 @@ func (c *GroupsClient) GetGroupMembersByID(ctx context.Context, groupID string) defer span.End() var gm GroupMembers - uri, err := c.uriPath(ctx, "", "groups", "id", url.PathEscape(groupID), "members") + uri, err := c.uriPath(ctx, "", "groups", url.PathEscape(groupID), "members") if err != nil { return gm, err } diff --git a/client/groups/client_test.go b/client/groups/client_test.go new file mode 100644 index 0000000..856b54e --- /dev/null +++ b/client/groups/client_test.go @@ -0,0 +1,107 @@ +package groups + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "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 + }{ + {name: "empty", total: 0, want: 0}, + {name: "single short page", total: 17, want: 17}, + {name: "exactly one full page", total: pageSize, want: pageSize}, + {name: "just over one page", total: pageSize + 1, want: pageSize + 1}, + {name: "production scale", total: 2583, want: 2583}, + } + + 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++ + 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) + } + + // 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) + } + } + }) + } +} + +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) + } + } +} diff --git a/client/groups/model.go b/client/groups/model.go index 3b75266..af6e97a 100644 --- a/client/groups/model.go +++ b/client/groups/model.go @@ -14,15 +14,13 @@ type Subject struct { } 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"` +} type GroupList struct { Groups []Group `json:"groups"` @@ -32,6 +30,12 @@ type GroupMembers struct { Members []Subject `json:"members"` } -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..a93aec5 100644 --- a/crawl.go +++ b/crawl.go @@ -12,38 +12,37 @@ import ( ) type Crawler struct { - groupsClient *groups.GroupsClient - groupBaseFolder string - publicGroup string + groupsClient *groups.GroupsClient + publicGroup string // maybe a data-info client too for irods crawling? publishClient *messaging.Client } -func NewCrawler(groupsClient *groups.GroupsClient, groupBaseFolder, publicGroup string, publishClient *messaging.Client) *Crawler { +func NewCrawler(groupsClient *groups.GroupsClient, publicGroup string, publishClient *messaging.Client) *Crawler { return &Crawler{ - groupsClient: groupsClient, - groupBaseFolder: groupBaseFolder, - publicGroup: publicGroup, - publishClient: publishClient, + groupsClient: groupsClient, + publicGroup: publicGroup, + 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 every group the groups service knows about. 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 { + for _, group := range gs { if group.ID != c.publicGroup { err = c.publishClient.PublishContext(ctx, fmt.Sprintf("index.group.%s", group.ID), []byte{}) } diff --git a/main.go b/main.go index 62ed09f..54049b6 100644 --- a/main.go +++ b/main.go @@ -121,13 +121,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()) @@ -146,7 +146,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 +158,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_test.go b/propagate_test.go index 74cb931..3d2e808 100644 --- a/propagate_test.go +++ b/propagate_test.go @@ -19,10 +19,9 @@ type memberFixture struct { sourceID string } -// newGroupsServer serves member listings keyed by group ID only, which is the -// one lookup both iplant-groups and the groups service agree on. A request for -// any other path 404s, so a recursion that follows names rather than IDs fails -// the test rather than quietly returning short results. +// 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. func newGroupsServer(t *testing.T, membership map[string][]memberFixture) *httptest.Server { t.Helper() mux := http.NewServeMux() @@ -32,7 +31,7 @@ func newGroupsServer(t *testing.T, membership map[string][]memberFixture) *httpt subjects = append(subjects, groups.Subject{ID: m.id, Name: m.name, SourceID: m.sourceID}) } body := groups.GroupMembers{Members: subjects} - mux.HandleFunc(fmt.Sprintf("/groups/id/%s/members", groupID), func(w http.ResponseWriter, r *http.Request) { + 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) From 39e09071789e5be44ba4823e5ee79cb39fc3274c Mon Sep 17 00:00:00 2001 From: John Wregglesworth Date: Wed, 5 Aug 2026 13:01:07 -0700 Subject: [PATCH 03/14] Refuse to propagate a withheld member list A public group whose membership is not public answers a non-admin caller with an empty list. data-info's member update replaces rather than merges, so propagating that empties the iRODS group -- and the log line is "Updated group X with 0 members", indistinguishable from the benign case where iRODS has no accounts for the members. The groups service now marks a withheld list, and this refuses it with a message naming the cause: the configured groups.user is not an admin of the groups service. Also replaces the embedded default config's iplant_groups block, which no longer satisfied Validate() after the switch to groups.*, so any deployment relying on defaults failed startup. Co-Authored-By: Claude Opus 5 --- client/groups/model.go | 6 ++++++ main.go | 9 ++++----- propagate.go | 11 +++++++++++ propagate_test.go | 32 ++++++++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 5 deletions(-) diff --git a/client/groups/model.go b/client/groups/model.go index af6e97a..69b9e07 100644 --- a/client/groups/model.go +++ b/client/groups/model.go @@ -28,6 +28,12 @@ type GroupList struct { 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"` } // Status is the groups service's GET / response. Keycloak is deliberately not diff --git a/main.go b/main.go index 54049b6..ce3d4f2 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" diff --git a/propagate.go b/propagate.go index 51b6fbf..e0697c4 100644 --- a/propagate.go +++ b/propagate.go @@ -48,6 +48,17 @@ func (p *Propagator) getGroupMembers(ctx context.Context, groupID string) ([]str 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" { m = append(m, member.ID) diff --git a/propagate_test.go b/propagate_test.go index 3d2e808..95ca0aa 100644 --- a/propagate_test.go +++ b/propagate_test.go @@ -110,3 +110,35 @@ func TestGetGroupMembersFollowsNestingByID(t *testing.T) { }) } } + +// 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)) + } +} From 0c2a4fe1912afe5ec386de783e9ddbe13552155c Mon Sep 17 00:00:00 2001 From: John Wregglesworth Date: Wed, 5 Aug 2026 13:01:39 -0700 Subject: [PATCH 04/14] Clear lint in the touched propagator files Unchecked Close on the groups client, and the source-id chain reads as a tagged switch. Co-Authored-By: Claude Opus 5 --- client/groups/client.go | 2 +- propagate.go | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/client/groups/client.go b/client/groups/client.go index c4e8a3e..51f2494 100644 --- a/client/groups/client.go +++ b/client/groups/client.go @@ -86,7 +86,7 @@ func (c *GroupsClient) getJSON(ctx context.Context, uri string, target any) erro } else 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() + defer func() { _ = resp.Body.Close() }() if target != nil { err = json.NewDecoder(resp.Body).Decode(target) diff --git a/propagate.go b/propagate.go index e0697c4..8537781 100644 --- a/propagate.go +++ b/propagate.go @@ -60,9 +60,10 @@ func (p *Propagator) getGroupMembers(ctx context.Context, groupID string) ([]str } for _, member := range members.Members { - if member.SourceID == "ldap" { + switch member.SourceID { + case "ldap": m = append(m, member.ID) - } else if member.SourceID == "g:gsa" { + case "g:gsa": // 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. submem, err := p.getGroupMembers(ctx, member.ID) @@ -70,7 +71,7 @@ func (p *Propagator) getGroupMembers(ctx context.Context, groupID string) ([]str return m, errors.Wrapf(err, "Failed recursing to fetch members of %s (%s)", member.Name, member.ID) } m = append(m, submem...) - } else { + default: log.Errorf("Could not add group member %+v", member) } } From 255874b9da95e39e7f7865de0a06b7a316c65a73 Mon Sep 17 00:00:00 2001 From: John Wregglesworth Date: Wed, 5 Aug 2026 15:35:11 -0700 Subject: [PATCH 05/14] Decouple listing pagination from the server's page cap ListAllGroups asked for 1000 groups per page and stopped on the first page shorter than that. The 1000-row cap belongs to the groups service, though, so if a future release lowers it, every page comes back "short" and the crawl silently truncates to the first response -- groups past the cap would just stop propagating, with no error anywhere. Advance the offset by the number of groups each page actually returned and terminate only on an empty page, so the crawl tracks whatever cap the server enforces. The fake servers in the tests hard-cap the request count so an offset-ignoring client fails fast instead of looping. Co-Authored-By: Claude Fable 5 --- client/groups/client.go | 11 +++-- client/groups/client_test.go | 84 +++++++++++++++++++++++++++++++++--- 2 files changed, 86 insertions(+), 9 deletions(-) diff --git a/client/groups/client.go b/client/groups/client.go index 51f2494..4c89f6c 100644 --- a/client/groups/client.go +++ b/client/groups/client.go @@ -127,8 +127,12 @@ func (c *GroupsClient) ListAllGroups(ctx context.Context) ([]Group, error) { ctx, span := otel.Tracer(otelName).Start(ctx, "ListAllGroups") defer span.End() + // 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 - for offset := 0; ; offset += pageSize { + for offset := 0; ; offset = len(all) { uri, err := c.uriPath(ctx, fmt.Sprintf("limit=%d&offset=%d", pageSize, offset), "groups") if err != nil { return all, err @@ -139,11 +143,10 @@ func (c *GroupsClient) ListAllGroups(ctx context.Context) ([]Group, error) { if err := c.getJSON(ctx, uri, &page); err != nil { return all, err } - - all = append(all, page.Groups...) - if len(page.Groups) < pageSize { + if len(page.Groups) == 0 { return all, nil } + all = append(all, page.Groups...) } } diff --git a/client/groups/client_test.go b/client/groups/client_test.go index 856b54e..2a427b5 100644 --- a/client/groups/client_test.go +++ b/client/groups/client_test.go @@ -19,12 +19,15 @@ func TestListAllGroupsPaginates(t *testing.T) { 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}, - {name: "single short page", total: 17, want: 17}, - {name: "exactly one full page", total: pageSize, want: pageSize}, - {name: "just over one page", total: pageSize + 1, want: pageSize + 1}, - {name: "production scale", total: 2583, want: 2583}, + {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 { @@ -36,6 +39,13 @@ func TestListAllGroupsPaginates(t *testing.T) { 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")) @@ -58,6 +68,9 @@ func TestListAllGroupsPaginates(t *testing.T) { 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. @@ -70,6 +83,67 @@ func TestListAllGroupsPaginates(t *testing.T) { } } +// 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) + } + } +} + func TestLookupDEUsersGroup(t *testing.T) { var gotQuery string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From b7f144596a26148f0c5b4f92def26416a948cce2 Mon Sep 17 00:00:00 2001 From: John Wregglesworth Date: Wed, 5 Aug 2026 15:36:27 -0700 Subject: [PATCH 06/14] Guard the membership recursion against cycles getGroupMembers recursed into nested groups with no visited set. The groups service API can't create a membership cycle today, but a direct SQL edit can, and one cycle would spin the recursion forever issuing HTTP GETs -- and with the AMQP consumer concurrency at 1, that halts all group propagation, not just the poisoned group. Thread a visited set through the recursion and silently skip any group ID already seen. Silent is the right response because the same shape occurs legitimately in a diamond (two parents sharing a subgroup), whose members were already collected on the first visit. The fake groups server in the tests now hard-caps its request count so an unterminated recursion fails the suite in milliseconds, not at the test timeout. Co-Authored-By: Claude Fable 5 --- propagate.go | 17 ++++++++++++- propagate_test.go | 61 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 76 insertions(+), 2 deletions(-) diff --git a/propagate.go b/propagate.go index 8537781..f5a7015 100644 --- a/propagate.go +++ b/propagate.go @@ -38,6 +38,14 @@ func NewPropagator(groupsClient *groups.GroupsClient, groupPrefix string, dataIn } func (p *Propagator) getGroupMembers(ctx context.Context, groupID string) ([]string, error) { + return p.getGroupMembersVisiting(ctx, groupID, map[string]struct{}{groupID: {}}) +} + +// 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() @@ -66,7 +74,14 @@ func (p *Propagator) getGroupMembers(ctx context.Context, groupID string) ([]str case "g:gsa": // 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. - submem, err := p.getGroupMembers(ctx, member.ID) + // 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 (%s)", member.Name, member.ID) } diff --git a/propagate_test.go b/propagate_test.go index 95ca0aa..4a9cbc8 100644 --- a/propagate_test.go +++ b/propagate_test.go @@ -22,6 +22,8 @@ type memberFixture struct { // 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() @@ -38,7 +40,18 @@ func newGroupsServer(t *testing.T, membership map[string][]memberFixture) *httpt } }) } - return httptest.NewServer(mux) + + 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) { @@ -111,6 +124,52 @@ func TestGetGroupMembersFollowsNestingByID(t *testing.T) { } } +// 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") From 43e3cb96d751deeb946149a38647695f5ff23831 Mon Sep 17 00:00:00 2001 From: John Wregglesworth Date: Wed, 5 Aug 2026 15:37:41 -0700 Subject: [PATCH 07/14] Fail startup when the groups user lacks admin standing The groups service answers a non-admin's listing requests with a 200 and an access-filtered page -- there is no marker distinguishing it from a complete listing. A propagator configured with a groups user missing from the service's admin-users list would therefore start cleanly, crawl a filtered listing, and groups would silently stop propagating. After resolving the de-users group ID at startup, verify that the same group shows up in a system-type group listing (the same listing endpoint the crawl uses, narrowed to the smallest slice that must contain it) and refuse to start otherwise, naming the probable cause in the error. Co-Authored-By: Claude Fable 5 --- client/groups/client.go | 30 ++++++++++++++++ client/groups/client_test.go | 69 ++++++++++++++++++++++++++++++++++++ main.go | 9 +++++ 3 files changed, 108 insertions(+) diff --git a/client/groups/client.go b/client/groups/client.go index 4c89f6c..2a0fdad 100644 --- a/client/groups/client.go +++ b/client/groups/client.go @@ -150,6 +150,36 @@ func (c *GroupsClient) ListAllGroups(ctx context.Context) ([]Group, error) { } } +// 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() + + uri, err := c.uriPath(ctx, fmt.Sprintf("group_type=system&limit=%d&offset=0", pageSize), "groups") + if err != nil { + return errors.Wrap(err, "Failed to build the system group listing URL") + } + + var page GroupList + if err := c.getJSON(ctx, uri, &page); err != nil { + return errors.Wrap(err, "Failed listing system groups") + } + for _, g := range page.Groups { + 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 func (c *GroupsClient) GetGroupByID(ctx context.Context, groupID string) (Group, error) { ctx, span := otel.Tracer(otelName).Start(ctx, "GetGroupByID") diff --git a/client/groups/client_test.go b/client/groups/client_test.go index 2a427b5..be7b310 100644 --- a/client/groups/client_test.go +++ b/client/groups/client_test.go @@ -144,6 +144,75 @@ func TestListAllGroupsHonorsServerCappedPages(t *testing.T) { } } +// 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 + 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: "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() + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(GroupList{Groups: tt.groups}); 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) { diff --git a/main.go b/main.go index ce3d4f2..93f8038 100644 --- a/main.go +++ b/main.go @@ -136,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 { From f6f0ceb84963627e4d66fba20cdfe7c621a1a2f4 Mon Sep 17 00:00:00 2001 From: John Wregglesworth Date: Wed, 5 Aug 2026 15:38:05 -0700 Subject: [PATCH 08/14] Set a 30s timeout on both REST clients Neither package-level http.Client had a Timeout, so a downstream that accepts the connection but never answers would hang a request forever. The stakes went up on this branch: the AMQP consumer runs with concurrency 1, so one wedged request to the groups service or data-info stops all propagation, and only the crawl's context (if any deadline is ever attached upstream) could unstick it. Co-Authored-By: Claude Fable 5 --- client/datainfo/client.go | 8 +++++++- client/groups/client.go | 8 +++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/client/datainfo/client.go b/client/datainfo/client.go index 76d41b1..b2d4f9e 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} diff --git a/client/groups/client.go b/client/groups/client.go index 2a0fdad..5ad8d1b 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,7 +28,12 @@ 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} From 0b7eaf5f28d98eb28a8ed4e08a4e4626a9146dd0 Mon Sep 17 00:00:00 2001 From: John Wregglesworth Date: Wed, 5 Aug 2026 15:38:20 -0700 Subject: [PATCH 09/14] Close the response body on non-2xx responses in getJSON The early return for a non-2xx status ran before the deferred close was registered, leaking the response body -- and with it the underlying connection -- every time the groups service answered with an error. Register the close as soon as the request succeeds so every exit path is covered. (The data-info client already had this right.) Co-Authored-By: Claude Fable 5 --- client/groups/client.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/client/groups/client.go b/client/groups/client.go index 5ad8d1b..88ded4b 100644 --- a/client/groups/client.go +++ b/client/groups/client.go @@ -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 { - return restutils.NewHTTPError(resp.StatusCode, fmt.Sprintf("GET %s returned %d", uri, resp.StatusCode)) } 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)) + } if target != nil { err = json.NewDecoder(resp.Body).Decode(target) From 0f265d1276efe9082938e5ae491cc61c84f4eb57 Mon Sep 17 00:00:00 2001 From: John Wregglesworth Date: Wed, 5 Aug 2026 15:38:37 -0700 Subject: [PATCH 10/14] Scope the publish error to its own loop iteration The err variable in CrawlGroups was declared outside the loop and only assigned when a group was actually published. On the iteration that skips the public group, the error check re-read whatever the previous iteration left behind, logging a stale publish failure against the wrong group and re-recording it as the overall error. Scope the error to the publish call so each check sees only its own result. Co-Authored-By: Claude Fable 5 --- crawl.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crawl.go b/crawl.go index a93aec5..a1f89bc 100644 --- a/crawl.go +++ b/crawl.go @@ -43,10 +43,10 @@ func (c *Crawler) CrawlGroups(ctx context.Context) error { var overallError error for _, group := range gs { - if group.ID != c.publicGroup { - err = c.publishClient.PublishContext(ctx, fmt.Sprintf("index.group.%s", group.ID), []byte{}) + if group.ID == c.publicGroup { + 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 } From fc764b2d7b7110c9b619784e17c4e21d94e1998d Mon Sep 17 00:00:00 2001 From: John Wregglesworth Date: Wed, 5 Aug 2026 15:38:57 -0700 Subject: [PATCH 11/14] Document the exported groups client model types Status and the Redacted field carried doc comments while Group, GroupList, GroupMembers, and Subject had none; bring the older types up to the same standard with one-sentence comments. Co-Authored-By: Claude Fable 5 --- client/groups/model.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/client/groups/model.go b/client/groups/model.go index 69b9e07..da519dd 100644 --- a/client/groups/model.go +++ b/client/groups/model.go @@ -1,5 +1,7 @@ package groups +// 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,6 +15,7 @@ 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"` @@ -22,10 +25,12 @@ type Group struct { 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"` From 32eb6d0f65722fa855fa95e75125dc598034d7e7 Mon Sep 17 00:00:00 2001 From: John Wregglesworth Date: Wed, 5 Aug 2026 15:39:30 -0700 Subject: [PATCH 12/14] Clear the remaining errcheck finding in the data-info client The unchecked deferred Close was the one lint issue left in a file this branch touches; discard the error explicitly to match how the groups client handles it. Co-Authored-By: Claude Fable 5 --- client/datainfo/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/datainfo/client.go b/client/datainfo/client.go index b2d4f9e..3ed3f70 100644 --- a/client/datainfo/client.go +++ b/client/datainfo/client.go @@ -64,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 From 9eb45f4576a3ff4dea980c842dd53aedc4ede283 Mon Sep 17 00:00:00 2001 From: John Wregglesworth Date: Thu, 3 Sep 2026 14:16:14 -0700 Subject: [PATCH 13/14] Keep the crawl bounded and stop dropping members silently The group listing crawl ended only on an empty page, so a service that stopped honoring offset would wedge the single AMQP consumer goroutine while the accumulated slice grew, with nothing logged. Bound it: a page that contributes no group the crawl has not already seen, or a crawl that runs past maxListPages, is an error naming the likely cause. The startup admin check now pages the system listing through the same helper instead of judging admin standing from a single response. An unrecognized member source id was logged and skipped, which handed data-info a short member list; that update replaces rather than merges, so the members would have been removed from the iRODS group under a log line reading like an ordinary run. Refuse the propagation instead, as the redacted-list guard already does. Nested expansion also returned a user once per path that reached them, so deduplicate the list before returning it. Scope the crawl to the groups users create. The listing that replaced the prefix-scoped Grouper search carries the DE's own system groups too, and publishing for those has the propagator create an @grouper- iRODS group per system group that nothing ever removes. Skipping the system type in the crawl keeps this to one listing request and, unlike naming the three user-created types, does not silently drop a group type the groups service adds later. Retire the remaining Grouper-era wording and names the switch left behind: the README summary, the propagation header comment, two error strings, and the crawler's publicGroup field, which holds the de-users group ID. The @grouper- iRODS name prefix stays; existing iRODS groups carry it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Wjd21NTp4Ead7JYhx5sQUT --- README.md | 2 +- client/groups/client.go | 68 ++++++++++++---- client/groups/client_test.go | 88 +++++++++++++++++++- client/groups/model.go | 11 +++ crawl.go | 35 +++++--- crawl_test.go | 110 +++++++++++++++++++++++++ propagate.go | 46 ++++++++--- propagate_test.go | 153 +++++++++++++++++++++++++++++++++++ 8 files changed, 469 insertions(+), 44 deletions(-) create mode 100644 crawl_test.go 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/groups/client.go b/client/groups/client.go index 88ded4b..b7470ae 100644 --- a/client/groups/client.go +++ b/client/groups/client.go @@ -43,7 +43,7 @@ func (c *GroupsClient) getDEUsersGroup(ctx context.Context) (*Group, error) { ctx, span := otel.Tracer(otelName).Start(ctx, "getDEUsersGroup") defer span.End() - uri, err := c.uriPath(ctx, fmt.Sprintf("group_type=system&name=%s", url.QueryEscape(c.DEUsersGroupName)), + 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 build the de-users lookup URL") @@ -127,24 +127,39 @@ func (c *GroupsClient) Check(ctx context.Context) error { // single response at 1000, so anything larger would be silently trimmed. const pageSize = 1000 -// 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() +// 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 - for offset := 0; ; offset = len(all) { - uri, err := c.uriPath(ctx, fmt.Sprintf("limit=%d&offset=%d", pageSize, offset), "groups") + 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)) + } + + 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("ListAllGroups uri: %s", uri) + log.Debugf("listGroups uri: %s", uri) var page GroupList if err := c.getJSON(ctx, uri, &page); err != nil { @@ -153,10 +168,34 @@ func (c *GroupsClient) ListAllGroups(ctx context.Context) ([]Group, error) { if len(page.Groups) == 0 { return all, nil } + + 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...) } } +// 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() + + return c.listGroups(ctx, "") +} + // 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 @@ -166,16 +205,11 @@ func (c *GroupsClient) VerifyAdminListing(ctx context.Context) error { ctx, span := otel.Tracer(otelName).Start(ctx, "VerifyAdminListing") defer span.End() - uri, err := c.uriPath(ctx, fmt.Sprintf("group_type=system&limit=%d&offset=0", pageSize), "groups") + system, err := c.listGroups(ctx, GroupTypeSystem) if err != nil { - return errors.Wrap(err, "Failed to build the system group listing URL") - } - - var page GroupList - if err := c.getJSON(ctx, uri, &page); err != nil { return errors.Wrap(err, "Failed listing system groups") } - for _, g := range page.Groups { + for _, g := range system { if g.ID == c.GroupsID { return nil } diff --git a/client/groups/client_test.go b/client/groups/client_test.go index be7b310..c3a20ab 100644 --- a/client/groups/client_test.go +++ b/client/groups/client_test.go @@ -144,6 +144,63 @@ func TestListAllGroupsHonorsServerCappedPages(t *testing.T) { } } +// 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 @@ -152,9 +209,13 @@ func TestListAllGroupsHonorsServerCappedPages(t *testing.T) { // listing the crawl uses. func TestVerifyAdminListing(t *testing.T) { tests := []struct { - name string - groups []Group - wantErr bool + 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", @@ -164,6 +225,15 @@ func TestVerifyAdminListing(t *testing.T) { }, 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{ @@ -187,8 +257,18 @@ func TestVerifyAdminListing(t *testing.T) { 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: tt.groups}); err != nil { + if err := json.NewEncoder(w).Encode(GroupList{Groups: page}); err != nil { t.Errorf("encoding listing: %v", err) } })) diff --git a/client/groups/model.go b/client/groups/model.go index da519dd..44ba453 100644 --- a/client/groups/model.go +++ b/client/groups/model.go @@ -1,5 +1,16 @@ 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 { diff --git a/crawl.go b/crawl.go index a1f89bc..224b87e 100644 --- a/crawl.go +++ b/crawl.go @@ -5,33 +5,39 @@ 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 - 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, publicGroup string, publishClient *messaging.Client) *Crawler { +func NewCrawler(groupsClient *groups.GroupsClient, deUsersGroupID string, publishClient groupPublisher) *Crawler { return &Crawler{ - groupsClient: groupsClient, - publicGroup: publicGroup, - publishClient: publishClient, + groupsClient: groupsClient, + deUsersGroupID: deUsersGroupID, + publishClient: publishClient, } } -// Request every group the groups service knows about. 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. +// 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() @@ -43,7 +49,10 @@ func (c *Crawler) CrawlGroups(ctx context.Context) error { var overallError error for _, group := range gs { - if group.ID == c.publicGroup { + // 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 := c.publishClient.PublishContext(ctx, fmt.Sprintf("index.group.%s", group.ID), []byte{}); err != nil { 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/propagate.go b/propagate.go index f5a7015..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 { @@ -38,7 +38,28 @@ func NewPropagator(groupsClient *groups.GroupsClient, groupPrefix string, dataIn } func (p *Propagator) getGroupMembers(ctx context.Context, groupID string) ([]string, error) { - return p.getGroupMembersVisiting(ctx, groupID, map[string]struct{}{groupID: {}}) + 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 @@ -69,9 +90,9 @@ func (p *Propagator) getGroupMembersVisiting(ctx context.Context, groupID string for _, member := range members.Members { switch member.SourceID { - case "ldap": + case groups.SourceUser: m = append(m, member.ID) - case "g:gsa": + 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 @@ -87,7 +108,14 @@ func (p *Propagator) getGroupMembersVisiting(ctx context.Context, groupID string } m = append(m, submem...) default: - log.Errorf("Could not add group member %+v", member) + // 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) } } @@ -114,9 +142,9 @@ 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, groupID) diff --git a/propagate_test.go b/propagate_test.go index 4a9cbc8..36055e7 100644 --- a/propagate_test.go +++ b/propagate_test.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "slices" "sort" "testing" @@ -201,3 +202,155 @@ func TestGetGroupMembersRefusesRedactedList(t *testing.T) { 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) + } + }) + } +} From db08882b6c0ca8f605be0ba70fda333dfea8dccf Mon Sep 17 00:00:00 2001 From: John Wregglesworth Date: Thu, 3 Sep 2026 15:20:15 -0700 Subject: [PATCH 14/14] Page the member listing when propagating a group The groups service now caps a member listing and refuses an unpaged request for a group larger than the cap, rather than truncating it. Propagation needs the whole membership -- the data-info update replaces rather than merges, so a short list removes people from the iRODS group -- so ask for it a page at a time. Termination mirrors ListAllGroups: the offset advances by what the pages held and a page contributing no new member ends the crawl, because stopping at a page shorter than the requested size would truncate silently against a service whose own cap is lower than what we asked for. The reported total ends the crawl one request earlier when it is present, and a crawl that ends disagreeing with it is refused instead of propagated. Redaction is read from the page as a property of the group: an empty member list flagged redacted means the membership was withheld, not that the group is empty. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Wjd21NTp4Ead7JYhx5sQUT --- client/groups/client.go | 67 ++++++++++++++++++++-- client/groups/client_test.go | 108 +++++++++++++++++++++++++++++++++++ client/groups/model.go | 4 ++ 3 files changed, 174 insertions(+), 5 deletions(-) diff --git a/client/groups/client.go b/client/groups/client.go index b7470ae..9b49cb0 100644 --- a/client/groups/client.go +++ b/client/groups/client.go @@ -244,12 +244,69 @@ func (c *GroupsClient) GetGroupMembersByID(ctx context.Context, groupID string) 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(groupID), "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 index c3a20ab..15396c4 100644 --- a/client/groups/client_test.go +++ b/client/groups/client_test.go @@ -7,7 +7,9 @@ import ( "net/http" "net/http/httptest" "net/url" + "slices" "strconv" + "strings" "testing" ) @@ -328,3 +330,109 @@ func TestLookupDEUsersGroup(t *testing.T) { } } } + +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 44ba453..596aaaa 100644 --- a/client/groups/model.go +++ b/client/groups/model.go @@ -50,6 +50,10 @@ type GroupMembers struct { // 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"` } // Status is the groups service's GET / response. Keycloak is deliberately not