[CXH-2208] fix: derive CLM hrefs for Grant/Revoke instead of requiring a profile field

.github/workflows/ci.yaml

@@ -5,6 +5,16 @@ on:
   push:
     branches:
       - main
+# Workflow-level (not per-job): all three jobs below hit the same shared DocuSign demo
+# account, and running two runs' Grant/Revoke cycles concurrently races on that account's
+# real state (one run's mid-cycle Grant/Revoke can make another run's "should be zero
+# grants after Revoke" assertion fail). A per-job concurrency block only protects a
+# *running* job from cancellation — GitHub Actions still cancels a *pending* job in the
+# same group when a newer one queues. Declaring it once here makes the whole
+# needs-chained run (all three jobs) queue/cancel as one unit against the shared group.
+concurrency:
+  group: docusign-demo-account
+  cancel-in-progress: false
 jobs:
   test-groups:
     runs-on: ubuntu-latest

pkg/client/clm_client_test.go

@@ -2,7 +2,9 @@ package client_test
 
 import (
 	"context"
+	"strings"
 	"testing"
+	"time"
 
 	"github.com/conductorone/baton-docusign/pkg/client"
 	"github.com/conductorone/baton-docusign/pkg/client/clmtest"
@@ -31,12 +33,83 @@ func TestSearchFolders_Pagination(t *testing.T) {
 	}
 	// Search results are summaries — no Security field.
 	for _, f := range all {
-		if len(f.Security.Groups.Items) != 0 || len(f.Security.Roles.Items) != 0 || len(f.Security.Users.Items) != 0 {
+		if len(f.Security.Groups) != 0 || len(f.Security.Roles) != 0 || len(f.Security.Users) != 0 {
 			t.Errorf("folder %s: expected Search to omit Security, got %+v", f.Name, f.Security)
 		}
 	}
 }
 
+// TestSearchFolders_EmptyResultHrefFailsLoud is a regression for the page-1 loop that
+// happens when Result.Href is empty but more pages remain: getClmNextToken would mint a
+// token with ResultHref:"", and the next SearchFolders call would re-POST a new search
+// (resetting Requests) forever. SearchFolders must error instead of emitting that token.
+func TestSearchFolders_EmptyResultHrefFailsLoud(t *testing.T) {
+	srv, c := clmtest.NewServer(t)
+	ctx := context.Background()
+	srv.SetOmitFolderSearchResultHref(true)
+
+	folders, next, _, err := c.SearchFolders(ctx, client.PageOptions{PageSize: 2})
+	if err == nil {
+		t.Fatalf("expected error when Result.Href is empty and more pages remain, got folders=%d next=%q", len(folders), next)
+	}
+	if next != "" {
+		t.Errorf("expected empty next token on failure, got %q", next)
+	}
+	if !strings.Contains(err.Error(), "Result has no Href") {
+		t.Errorf("expected Result-Href error, got: %v", err)
+	}
+}
+
+// TestSearchFolders_PollsUntilSuccess is a regression test for SearchFolders'
+// awaitClmFolderSearchTask branch: every live test against a real CLM tenant resolved
+// the task inline (Status "Success" already in the POST response), leaving the polling
+// branch itself unexercised until now.
+func TestSearchFolders_PollsUntilSuccess(t *testing.T) {
+	original := client.ClmFolderSearchTaskPollInterval
+	client.ClmFolderSearchTaskPollInterval = time.Millisecond
+	defer func() { client.ClmFolderSearchTaskPollInterval = original }()
+
+	srv, c := clmtest.NewServer(t)
+	ctx := context.Background()
+
+	srv.SetPendingFolderSearchPolls(2)
+
+	folders, _, _, err := c.SearchFolders(ctx, client.PageOptions{PageSize: 10})
+	if err != nil {
+		t.Fatalf("SearchFolders: %v", err)
+	}
+	if len(folders) != 3 {
+		t.Fatalf("expected all 3 seeded folders once the task resolves, got %d", len(folders))
+	}
+}
+
+// TestPatchFolderSecurity_PollsUntilSuccess is a regression test for
+// PatchFolderSecurity's awaitClmChangeSecurityTask branch — unverified against a live
+// tenant (see PatchFolderSecurity's doc in clm_client.go), so this mock-driven test is
+// this branch's only coverage.
+func TestPatchFolderSecurity_PollsUntilSuccess(t *testing.T) {
+	original := client.ClmFolderSearchTaskPollInterval
+	client.ClmFolderSearchTaskPollInterval = time.Millisecond
+	defer func() { client.ClmFolderSearchTaskPollInterval = original }()
+
+	srv, c := clmtest.NewServer(t)
+	ctx := context.Background()
+
+	groupHref := srv.GroupHref("group-ops")
+	srv.SetPendingChangeSecurityPolls(2)
+
+	if _, err := c.PatchFolderSecurity(ctx, "folder-templates", client.ClmFolderSecurityWrite{
+		Groups: []client.ClmGroupSecurityEntry{{AccessType: client.ClmAccessTypeView, Href: groupHref}},
+	}); err != nil {
+		t.Fatalf("PatchFolderSecurity: %v", err)
+	}
+
+	sec := srv.FolderSecurity("folder-templates")
+	if len(sec.Groups) != 1 || sec.Groups[0].AccessType != client.ClmAccessTypeView || sec.Groups[0].Href != groupHref {
+		t.Fatalf("expected one View entry for %s once the task resolves, got %+v", groupHref, sec.Groups)
+	}
+}
+
 func TestGetFolder_ExpandSecurity(t *testing.T) {
 	_, c := clmtest.NewServer(t)
 	ctx := context.Background()
@@ -46,7 +119,7 @@ func TestGetFolder_ExpandSecurity(t *testing.T) {
 		if err != nil {
 			t.Fatalf("GetFolder: %v", err)
 		}
-		if len(folder.Security.Groups.Items) != 0 || len(folder.Security.Roles.Items) != 0 || len(folder.Security.Users.Items) != 0 {
+		if len(folder.Security.Groups) != 0 || len(folder.Security.Roles) != 0 || len(folder.Security.Users) != 0 {
 			t.Errorf("expected no Security without ?expand=Security, got %+v", folder.Security)
 		}
 	})
@@ -56,14 +129,14 @@ func TestGetFolder_ExpandSecurity(t *testing.T) {
 		if err != nil {
 			t.Fatalf("GetFolder: %v", err)
 		}
-		if len(folder.Security.Groups.Items) != 2 {
-			t.Fatalf("expected 2 seeded group security entries, got %d: %+v", len(folder.Security.Groups.Items), folder.Security.Groups.Items)
+		if len(folder.Security.Groups) != 2 {
+			t.Fatalf("expected 2 seeded group security entries, got %d: %+v", len(folder.Security.Groups), folder.Security.Groups)
 		}
-		if len(folder.Security.Roles.Items) != 1 {
-			t.Fatalf("expected 1 seeded role security entry, got %d: %+v", len(folder.Security.Roles.Items), folder.Security.Roles.Items)
+		if len(folder.Security.Roles) != 1 {
+			t.Fatalf("expected 1 seeded role security entry, got %d: %+v", len(folder.Security.Roles), folder.Security.Roles)
 		}
-		if len(folder.Security.Users.Items) != 1 {
-			t.Fatalf("expected 1 seeded user security entry, got %d: %+v", len(folder.Security.Users.Items), folder.Security.Users.Items)
+		if len(folder.Security.Users) != 1 {
+			t.Fatalf("expected 1 seeded user security entry, got %d: %+v", len(folder.Security.Users), folder.Security.Users)
 		}
 	})
 
@@ -93,8 +166,8 @@ func TestPatchFolderSecurity_SendsExactEntries(t *testing.T) {
 	}
 
 	sec := srv.FolderSecurity("folder-templates")
-	if len(sec.Groups.Items) != 1 || sec.Groups.Items[0].AccessType != client.ClmAccessTypeView || sec.Groups.Items[0].Href != groupHref {
-		t.Fatalf("expected one View entry for %s, got %+v", groupHref, sec.Groups.Items)
+	if len(sec.Groups) != 1 || sec.Groups[0].AccessType != client.ClmAccessTypeView || sec.Groups[0].Href != groupHref {
+		t.Fatalf("expected one View entry for %s, got %+v", groupHref, sec.Groups)
 	}
 
 	// Sending a single-entry Groups list for the same Href again replaces the prior entry.
@@ -105,11 +178,11 @@ func TestPatchFolderSecurity_SendsExactEntries(t *testing.T) {
 	}
 
 	sec = srv.FolderSecurity("folder-templates")
-	if len(sec.Groups.Items) != 1 {
-		t.Fatalf("expected the existing entry to be updated in place, not duplicated: %+v", sec.Groups.Items)
+	if len(sec.Groups) != 1 {
+		t.Fatalf("expected the existing entry to be updated in place, not duplicated: %+v", sec.Groups)
 	}
-	if sec.Groups.Items[0].AccessType != client.ClmAccessTypeNoAccess {
-		t.Errorf("expected AccessType NoAccess after revoke, got %q", sec.Groups.Items[0].AccessType)
+	if sec.Groups[0].AccessType != client.ClmAccessTypeNoAccess {
+		t.Errorf("expected AccessType NoAccess after revoke, got %q", sec.Groups[0].AccessType)
 	}
 }
 

pkg/client/clm_helper.go

@@ -31,7 +31,16 @@ func preparePagedRequestClm(baseURL *url.URL, endpoint string, options PageOptio
 		return nil, clmRequestedPage{}, fmt.Errorf("baton-docusign: invalid CLM endpoint: %w", err)
 	}
 
-	fullURL := baseURL.ResolveReference(endpointURL)
+	return appendClmPageQuery(baseURL.ResolveReference(endpointURL), options)
+}
+
+// appendClmPageQuery appends CLM's pageSortParams.offset/limit query params to an
+// already-resolved absolute URL and decodes options.PageToken — the part of
+// preparePagedRequestClm that doesn't depend on resolving a relative endpoint against
+// the CLM base URL. Split out for SearchFolders' continuation pages, which paginate
+// against a server-issued Result href (a per-search URL CLM hands back, not one of this
+// package's own static endpoint constants) rather than a fixed collection endpoint.
+func appendClmPageQuery(fullURL *url.URL, options PageOptions) (*url.URL, clmRequestedPage, error) {
 	q := fullURL.Query()
 
 	offset := 0
@@ -58,10 +67,15 @@ func preparePagedRequestClm(baseURL *url.URL, endpoint string, options PageOptio
 
 // clmPageToken is the internal offset-based continuation token for CLM pagination.
 // Requests counts how many requests this pagination sequence has made so far — see
-// maxClmListPages.
+// maxClmListPages. ResultHref is only set by SearchFolders' continuation pages: unlike
+// every other CLM list endpoint (a fixed collection URL re-queried with a different
+// offset), a folder search's results live at a per-search URL CLM hands back from the
+// FolderSearchTasks create call, so the token must carry it forward — the collection
+// URL isn't otherwise derivable from the resource type alone.
 type clmPageToken struct {
-	Offset   int `json:"offset"`
-	Requests int `json:"requests"`
+	Offset     int    `json:"offset"`
+	Requests   int    `json:"requests"`
+	ResultHref string `json:"resultHref,omitempty"`
 }
 
 func encodeClmPageToken(pt *clmPageToken) string {
@@ -171,7 +185,9 @@ func decodeClmPageToken(token string) (*clmPageToken, error) {
 //     matters when the floor is the larger of the two estimates.
 const maxClmListPages = 1000
 
-func getClmNextToken(requested clmRequestedPage, itemCount int, hasNext bool, total int) (string, error) {
+// resultHref is embedded in the returned token as-is (see clmPageToken's doc) — pass ""
+// for every endpoint except SearchFolders' continuation pages.
+func getClmNextToken(requested clmRequestedPage, itemCount int, hasNext bool, total int, resultHref string) (string, error) {
 	if itemCount == 0 {
 		return "", nil
 	}
@@ -195,5 +211,5 @@ func getClmNextToken(requested clmRequestedPage, itemCount int, hasNext bool, to
 		return "", fmt.Errorf("baton-docusign: exceeded %d pages paginating a CLM list — the API may be ignoring the requested offset", maxClmListPages)
 	}
 
-	return encodeClmPageToken(&clmPageToken{Offset: nextOffset, Requests: nextRequests}), nil
+	return encodeClmPageToken(&clmPageToken{Offset: nextOffset, Requests: nextRequests, ResultHref: resultHref}), nil
 }

pkg/client/clm_helper_test.go

@@ -50,7 +50,7 @@ func TestGetClmNextToken_ComputesFromRequestNotResponse(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := getClmNextToken(tt.requested, tt.itemCount, tt.hasNext, tt.total)
+			got, err := getClmNextToken(tt.requested, tt.itemCount, tt.hasNext, tt.total, "")
 			if err != nil {
 				t.Fatalf("getClmNextToken: %v", err)
 			}
@@ -78,7 +78,7 @@ func TestGetClmNextToken_ComputesFromRequestNotResponse(t *testing.T) {
 // multiple of the page size, a full page landing exactly on Total stops immediately —
 // no need to wait for an empty page in this case, since Total confirms it.
 func TestGetClmNextToken_ExactBoundaryDoesNotLoop(t *testing.T) {
-	got, err := getClmNextToken(clmRequestedPage{Offset: 100, PageSize: 100}, 100, false, 200)
+	got, err := getClmNextToken(clmRequestedPage{Offset: 100, PageSize: 100}, 100, false, 200, "")
 	if err != nil {
 		t.Fatalf("getClmNextToken: %v", err)
 	}
@@ -113,15 +113,15 @@ func TestGetClmNextToken_ExactBoundaryDoesNotLoop(t *testing.T) {
 func TestGetClmNextToken_CapsRunawayPagination(t *testing.T) {
 	t.Run("reaches the cap through normal advancement", func(t *testing.T) {
 		requested := clmRequestedPage{Offset: (maxClmListPages - 1) * 100, PageSize: 100}
-		_, err := getClmNextToken(requested, 100, false, 0)
+		_, err := getClmNextToken(requested, 100, false, 0, "")
 		if err == nil {
 			t.Fatal("expected an error once maxClmListPages is reached, got nil")
 		}
 	})
 
 	t.Run("does not fire just below the cap", func(t *testing.T) {
 		requested := clmRequestedPage{Offset: (maxClmListPages - 2) * 100, PageSize: 100}
-		got, err := getClmNextToken(requested, 100, false, 0)
+		got, err := getClmNextToken(requested, 100, false, 0, "")
 		if err != nil {
 			t.Fatalf("expected no error just below the cap, got: %v", err)
 		}
@@ -134,7 +134,7 @@ func TestGetClmNextToken_CapsRunawayPagination(t *testing.T) {
 		// Offset alone, with Requests at its zero value (as a pre-cap or
 		// round-tripped token would decode to), must still trigger the cap.
 		requested := clmRequestedPage{Offset: maxClmListPages * 100, PageSize: 100}
-		_, err := getClmNextToken(requested, 100, false, 0)
+		_, err := getClmNextToken(requested, 100, false, 0, "")
 		if err == nil {
 			t.Fatal("expected the cap to fire from Offset alone, got nil error")
 		}
@@ -147,7 +147,7 @@ func TestGetClmNextToken_CapsRunawayPagination(t *testing.T) {
 		// requested.Requests to reach the same cap. Set Requests to exactly one below
 		// the cap to isolate that it alone is what trips it here.
 		requested := clmRequestedPage{Offset: maxClmListPages - 1, PageSize: 100, Requests: maxClmListPages - 1}
-		_, err := getClmNextToken(requested, 1, true, 0)
+		_, err := getClmNextToken(requested, 1, true, 0, "")
 		if err == nil {
 			t.Fatal("expected the request-count estimate to trip the cap even though the offset floor would not have")
 		}