diff --git a/pkg/client/entitlement.go b/pkg/client/entitlement.go index a23c63e2..2bf77dae 100644 --- a/pkg/client/entitlement.go +++ b/pkg/client/entitlement.go @@ -3,6 +3,7 @@ package client import ( "context" "errors" + "fmt" "net/http" "github.com/conductorone/conductorone-sdk-go/pkg/models/operations" @@ -97,36 +98,80 @@ func (e *ExpandableEntitlementWithBindings) SetPath(pathname string, value int) e.ExpandedMap[pathname] = value } +// maxRepeatedSearchEntitlementsPageToken bounds how many times the server may +// hand back a page token identical to the one just sent before we treat paging +// as stuck. Without it a server that never advances the token spins forever. +const maxRepeatedSearchEntitlementsPageToken = 8 + func (c *client) SearchEntitlements(ctx context.Context, filter *SearchEntitlementsFilter) ([]*EntitlementWithBindings, error) { - // TODO(morgabra) Pagination // TODO(morgabra) Should we abstract the OpenAPI objects from the rest of cone? Kinda... no? But they aren't typed... - req := shared.RequestCatalogSearchServiceSearchEntitlementsRequest{ - EntitlementAlias: stringPtr(filter.EntitlementAlias), - GrantedStatus: filter.GrantedStatus.ToPointer(), - PageSize: intPtr(100), - PageToken: nil, - Query: stringPtr(filter.Query), - AppDisplayName: stringPtr(filter.AppDisplayName), - IncludeDeleted: &filter.IncludeDeleted, - ExpandMask: &filter.AppEntitlementExpandMask, - } - resp, err := c.sdk.RequestCatalogSearch.SearchEntitlements(ctx, &req) - if err != nil { - return nil, err - } + rv := make([]*EntitlementWithBindings, 0) + pageToken := "" + repeatedToken := 0 - if err := NewHTTPError(resp.RawResponse); err != nil { - return nil, err + for { + req := shared.RequestCatalogSearchServiceSearchEntitlementsRequest{ + EntitlementAlias: stringPtr(filter.EntitlementAlias), + GrantedStatus: filter.GrantedStatus.ToPointer(), + PageSize: intPtr(100), + PageToken: stringPtr(pageToken), + Query: stringPtr(filter.Query), + AppDisplayName: stringPtr(filter.AppDisplayName), + IncludeDeleted: &filter.IncludeDeleted, + ExpandMask: &filter.AppEntitlementExpandMask, + } + resp, err := c.sdk.RequestCatalogSearch.SearchEntitlements(ctx, &req) + if err != nil { + return nil, err + } + + if err := NewHTTPError(resp.RawResponse); err != nil { + return nil, err + } + + page, err := convertSearchEntitlementsPage(resp.RequestCatalogSearchServiceSearchEntitlementsResponse) + if err != nil { + return nil, err + } + rv = append(rv, page...) + + // Stop on an empty token, never on an empty page: the server filters + // granted status after it cuts the page, so a page can come back short or + // empty while later pages still hold results. + nextPageToken := StringFromPtr(resp.RequestCatalogSearchServiceSearchEntitlementsResponse.NextPageToken) + if nextPageToken == "" { + break + } + if nextPageToken == pageToken { + repeatedToken++ + if repeatedToken >= maxRepeatedSearchEntitlementsPageToken { + return nil, fmt.Errorf("search-entitlements: page token repeated %d times, pagination is not advancing", repeatedToken) + } + } else { + repeatedToken = 0 + } + pageToken = nextPageToken } - list := resp.RequestCatalogSearchServiceSearchEntitlementsResponse.List - if list == nil { - return nil, errors.New("search-entitlements: list is nil") + return rv, nil +} + +// convertSearchEntitlementsPage expands and converts one response page. +// +// This runs per page rather than once over a concatenated list because the +// response's expanded array is indexed per response: ExpandedMap holds offsets +// into THIS page's expanded objects, so merging raw pages first would resolve +// those offsets against the wrong array. +func convertSearchEntitlementsPage( + resp *shared.RequestCatalogSearchServiceSearchEntitlementsResponse, +) ([]*EntitlementWithBindings, error) { + if resp == nil { + return nil, errors.New("search-entitlements: response is nil") } // Unmarshal the expanded fields - expanded := make([]any, 0, len(resp.RequestCatalogSearchServiceSearchEntitlementsResponse.Expanded)) - for _, x := range resp.RequestCatalogSearchServiceSearchEntitlementsResponse.Expanded { + expanded := make([]any, 0, len(resp.Expanded)) + for _, x := range resp.Expanded { x := x converted, err := UnmarshalAnyType[shared.RequestCatalogSearchServiceSearchEntitlementsResponseExpanded](&x) if err != nil { @@ -135,9 +180,11 @@ func (c *client) SearchEntitlements(ctx context.Context, filter *SearchEntitleme expanded = append(expanded, converted) } - // Convert the list of entitlements to a list of expandable entitlements - expandableList := make([]*ExpandableEntitlementWithBindings, 0, len(list)) - for _, v := range list { + // Convert the list of entitlements to a list of expandable entitlements. A + // nil list is an empty page, not a fault -- the wire format omits an empty + // repeated field entirely. + expandableList := make([]*ExpandableEntitlementWithBindings, 0, len(resp.List)) + for _, v := range resp.List { ent := NewExpandableEntitlementWithBindings(v) if ent == nil { return nil, errors.New("search-entitlements: entitlement is nil") @@ -147,7 +194,7 @@ func (c *client) SearchEntitlements(ctx context.Context, filter *SearchEntitleme } // Populate the expandable objects with the indexes of related objects - err = ExpandableReponse[*ExpandableEntitlementWithBindings]{ + err := ExpandableReponse[*ExpandableEntitlementWithBindings]{ List: expandableList, }.PopulateExpandedIndexes() @@ -156,7 +203,7 @@ func (c *client) SearchEntitlements(ctx context.Context, filter *SearchEntitleme } // Iterate over the expandable objects and convert them to the final response - rv := make([]*EntitlementWithBindings, 0, len(list)) + rv := make([]*EntitlementWithBindings, 0, len(expandableList)) for _, v := range expandableList { rv = append(rv, &EntitlementWithBindings{ Entitlement: AppEntitlement(*v.Entitlement.AppEntitlement), diff --git a/pkg/client/entitlement_pagination_test.go b/pkg/client/entitlement_pagination_test.go new file mode 100644 index 00000000..5b8f3e42 --- /dev/null +++ b/pkg/client/entitlement_pagination_test.go @@ -0,0 +1,168 @@ +package client + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// entitlementPage is one canned SearchEntitlements response. +type entitlementPage struct { + ids []string + nextPageToken string +} + +// serveEntitlementPages returns a server that hands back pages in order and +// records the page_token it was sent for each request. +func serveEntitlementPages(t *testing.T, pages []entitlementPage) (*httptest.Server, *[]string) { + t.Helper() + seen := make([]string, 0, len(pages)) + call := 0 + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var req map[string]any + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + t.Errorf("decode request: %v", err) + } + token, _ := req["pageToken"].(string) + seen = append(seen, token) + + if call >= len(pages) { + t.Errorf("server called %d times, only %d pages configured", call+1, len(pages)) + w.WriteHeader(http.StatusInternalServerError) + return + } + page := pages[call] + call++ + + entries := make([]string, 0, len(page.ids)) + for _, id := range page.ids { + entries = append(entries, fmt.Sprintf( + `{"entitlement":{"appEntitlement":{"id":%q,"appId":"app1","displayName":%q}}}`, id, id)) + } + body := fmt.Sprintf(`{"list":[%s]`, strings.Join(entries, ",")) + if page.nextPageToken != "" { + body += fmt.Sprintf(`,"nextPageToken":%q`, page.nextPageToken) + } + body += "}" + + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(server.Close) + + return server, &seen +} + +func entitlementIDs(got []*EntitlementWithBindings) []string { + ids := make([]string, 0, len(got)) + for _, e := range got { + ids = append(ids, e.Entitlement.GetAppId()+"/"+StringFromPtr(e.Entitlement.ID)) + } + return ids +} + +// TestSearchEntitlementsFollowsNextPageToken is the regression this whole change +// exists for: before it, the client made exactly one call and silently dropped +// every entitlement past the first page. +func TestSearchEntitlementsFollowsNextPageToken(t *testing.T) { + server, seen := serveEntitlementPages(t, []entitlementPage{ + {ids: []string{"ent1", "ent2"}, nextPageToken: "tok1"}, + {ids: []string{"ent3"}, nextPageToken: "tok2"}, + {ids: []string{"ent4"}}, + }) + + c := newPaperSecretTestClient(server.URL, server.Client()) + got, err := c.SearchEntitlements(context.Background(), &SearchEntitlementsFilter{}) + if err != nil { + t.Fatalf("SearchEntitlements: %v", err) + } + + if len(got) != 4 { + t.Fatalf("got %d entitlements (%v), want 4 across 3 pages", len(got), entitlementIDs(got)) + } + want := []string{"app1/ent1", "app1/ent2", "app1/ent3", "app1/ent4"} + for i, w := range want { + if entitlementIDs(got)[i] != w { + t.Errorf("entitlement %d = %q, want %q (page order must be preserved)", i, entitlementIDs(got)[i], w) + } + } + + wantTokens := []string{"", "tok1", "tok2"} + if len(*seen) != len(wantTokens) { + t.Fatalf("server saw %d requests (%v), want %d", len(*seen), *seen, len(wantTokens)) + } + for i, w := range wantTokens { + if (*seen)[i] != w { + t.Errorf("request %d sent pageToken %q, want %q", i, (*seen)[i], w) + } + } +} + +// TestSearchEntitlementsDoesNotStopOnEmptyPage covers the server's post-filter: +// granted_status is applied after the page is cut, so an intermediate page can be +// empty while later pages still hold results. Stopping on a short page would +// silently truncate. +func TestSearchEntitlementsDoesNotStopOnEmptyPage(t *testing.T) { + server, _ := serveEntitlementPages(t, []entitlementPage{ + {ids: []string{"ent1"}, nextPageToken: "tok1"}, + {ids: nil, nextPageToken: "tok2"}, + {ids: []string{"ent2"}}, + }) + + c := newPaperSecretTestClient(server.URL, server.Client()) + got, err := c.SearchEntitlements(context.Background(), &SearchEntitlementsFilter{}) + if err != nil { + t.Fatalf("SearchEntitlements: %v", err) + } + + if len(got) != 2 { + t.Fatalf("got %d entitlements (%v), want 2: an empty middle page must not end pagination", + len(got), entitlementIDs(got)) + } +} + +// TestSearchEntitlementsEmptyResultIsNotAnError pins that a wholly empty result +// returns an empty slice. The wire format omits an empty repeated field, so the +// list arrives nil. +func TestSearchEntitlementsEmptyResultIsNotAnError(t *testing.T) { + server, _ := serveEntitlementPages(t, []entitlementPage{{ids: nil}}) + + c := newPaperSecretTestClient(server.URL, server.Client()) + got, err := c.SearchEntitlements(context.Background(), &SearchEntitlementsFilter{}) + if err != nil { + t.Fatalf("SearchEntitlements: %v", err) + } + if len(got) != 0 { + t.Fatalf("got %d entitlements, want 0", len(got)) + } +} + +// TestSearchEntitlementsStopsOnStuckPageToken keeps a server that never advances +// the token from spinning the client forever. +func TestSearchEntitlementsStopsOnStuckPageToken(t *testing.T) { + calls := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls++ + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"list":[{"entitlement":{"appEntitlement":{"id":"ent1","appId":"app1"}}}],"nextPageToken":"stuck"}`)) + })) + defer server.Close() + + c := newPaperSecretTestClient(server.URL, server.Client()) + _, err := c.SearchEntitlements(context.Background(), &SearchEntitlementsFilter{}) + if err == nil { + t.Fatal("SearchEntitlements returned nil error on a non-advancing page token") + } + if !strings.Contains(err.Error(), "not advancing") { + t.Errorf("error = %v, want a non-advancing pagination error", err) + } + if calls > maxRepeatedSearchEntitlementsPageToken+2 { + t.Errorf("server called %d times, guard should have stopped it near %d", + calls, maxRepeatedSearchEntitlementsPageToken) + } +}