diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index bc83b7d6..92a1b1c3 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -5,6 +5,26 @@ 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 +env: + # Forces the legacy v1/SQLite c1z format instead of baton-sdk v0.25.0's new default + # (Pebble/v3). The `baton` CLI these jobs download (ConductorOne/github-workflows' + # get-baton action, currently v0.4.5) is built against baton-sdk v0.8.24 — long before + # Pebble existed — and fails every read of a Pebble-format file with a bare + # "c1z: invalid file", no matter how the file was produced. Confirmed: syncing with + # this flag set produces a file `baton` v0.4.5 reads correctly; without it, the exact + # same sync fails at the very next step. Safe to drop once a `baton` CLI release built + # against a Pebble-aware baton-sdk ships and get-baton picks it up. + BATON_STORAGE_ENGINE: sqlite jobs: test-groups: runs-on: ubuntu-latest diff --git a/pkg/client/clm_client.go b/pkg/client/clm_client.go index b2bc7da7..d8ec5e9f 100644 --- a/pkg/client/clm_client.go +++ b/pkg/client/clm_client.go @@ -4,18 +4,33 @@ // own OAuth scopes ("spring_read"/"spring_write", see oauth.go) and a different Object // API surface. Endpoints below are derived from DocuSign's CLM API reference (method // tables and request/response schemas). Validate against cmd/test-server during -// development; a production CLM tenant was not available to exercise this integration -// directly. +// development. Live-tested end-to-end against a real CLM tenant (demo/UAT +// environment): Groups/Members/PermissionSets/Roles all synced correctly with their +// entitlements and grants, once the authorizing user had admin rights in that +// environment — an earlier 401 "Access Denied" on every data call (discovery +// succeeded) turned out to be exactly that, not a scope, licensing, or demo-vs- +// production issue as initially suspected. Folders required a separate fix — see +// SearchFolders' doc for the confirmed request/response shape. // // # API Endpoints Used // // Folders: -// - POST /v2/{accountId}/folders/search - Discover folders (no flat list-all exists) +// - POST /v2/{accountId}/foldersearchtasks - Search for folders (async Task API — see +// SearchFolders' doc). Prefer this over the also-documented Folders:Search +// (POST /v2/{accountId}/folders/search), which returns 405 live. // - GET /v2/{accountId}/folders/{id}?expand=Security - Get a folder with its explicit security entries. // Security is three separate collections by principal type (Groups/Roles/Users), confirmed via // DocuSign's own Folders.Patch reference page - see ClmFolderSecurity's doc in clm_models.go. -// - PATCH /v2/{accountId}/folders/{id} - Update folder security (grant: set an AccessType on the -// relevant Groups/Roles/Users entry; revoke: set that entry's AccessType to "NoAccess") +// - POST /v2/{accountId}/changesecuritytasks - Update folder security (grant: set an AccessType on +// the relevant Groups/Roles/Users entry; revoke: set that entry's AccessType to "NoAccess") — see +// PatchFolderSecurity's doc. NOT the generic Folders Patch: that endpoint accepts a Security field +// in its documented schema but silently ignores it live (confirmed: 200 OK, no error, but a fresh +// GET shows no change, while a trivial PATCH of another field on the same folder did apply). CLM's +// own error code list names a distinct "136 - Missing Change Security Task", and the CLM API +// Reference confirms ChangeSecurityTasks as its own Task API resource for this. This rewrite +// matches ChangeSecurityTasks' documented request/response schema but is NOT independently +// verified live — per explicit instruction, this project is done live-testing against the +// customer's tenant. // // Groups: // - GET /v2/{accountId}/groups - List CLM groups (GetAllGroups) @@ -69,10 +84,11 @@ import ( "fmt" "net/http" "net/url" + "strings" + "time" "github.com/conductorone/baton-sdk/pkg/annotations" "github.com/conductorone/baton-sdk/pkg/uhttp" - "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -103,15 +119,32 @@ var clmBaseURLCandidateFields = []string{ // CLM API endpoint constants. const ( - clmSearchFolders = "/v2/%s/folders/search" - clmGetFolder = "/v2/%s/folders/%s" - clmPatchFolder = "/v2/%s/folders/%s" - clmGetGroups = "/v2/%s/groups" - clmGetGroupMembers = "/v2/%s/groups/%s/groupmembers" - clmGetMembers = "/v2/%s/members" - clmGetMemberGroups = "/v2/%s/members/%s/groups" - clmPatchPutMember = "/v2/%s/members/%s" - clmGetPermissionSet = "/v2/%s/permissionsets" + // clmCreateFolderSearchTask creates a CLM FolderSearchTasks task — see + // SearchFolders' doc. The CLM API Reference also documents a synchronous + // Folders:Search at POST /v2/{accountId}/folders/search, but against a real + // tenant that path returns 405 Method Not Allowed, so this connector uses the + // documented async FolderSearchTasks resource instead. + clmCreateFolderSearchTask = "/v2/%s/foldersearchtasks" + // clmCreateChangeSecurityTask creates a CLM ChangeSecurityTasks task — see + // PatchFolderSecurity's doc. The generic Folders Patch this replaced silently + // ignores a Security payload (confirmed live: 200 OK, but no effect). + clmCreateChangeSecurityTask = "/v2/%s/changesecuritytasks" + clmGetFolder = "/v2/%s/folders/%s" + clmGetGroups = "/v2/%s/groups" + clmGetGroupMembers = "/v2/%s/groups/%s/groupmembers" + clmGetMembers = "/v2/%s/members" + clmGetMemberGroups = "/v2/%s/members/%s/groups" + clmPatchPutMember = "/v2/%s/members/%s" + clmGetPermissionSet = "/v2/%s/permissionsets" + + // clmGroupPath and clmMemberPath are path *shapes*, not endpoints this connector + // calls — hrefFor builds a Href string locally from these, issuing no request beyond + // the one-time CLM base-URL discovery ensureClmReady may trigger, so neither + // corresponds to an entry in this file's "API Endpoints Used" doc. clmMemberPath is + // deliberately not clmPatchPutMember despite the identical shape: naming it after a + // real PATCH/PUT endpoint would be just as misleading in the other direction. + clmGroupPath = "/v2/%s/groups/%s" + clmMemberPath = "/v2/%s/members/%s" ) // ensureClmInitialized resolves the CLM Object API base URL, separately from @@ -247,6 +280,56 @@ func (c *Client) prepareClmPagedRequest(endpoint string, options PageOptions, ex return preparePagedRequestClm(baseURL, formatted, options) } +// clmKnownDomains are registrable domains CLM's own product is confirmed to use across +// its different hosts — see this file's package doc "Base URL resolution" section: the +// discovered Object API base URL is on *.clm.docusign.net, while account discovery is a +// separate, hardcoded auth.springcm.com/authuat.springcm.com host on a wholly different +// domain. Since CLM itself already spans two unrelated domains for different purposes, +// a Task API href on yet another CLM-owned host is plausible, which is why +// validateClmURL checks domain family rather than requiring the exact discovered host. +var clmKnownDomains = []string{"docusign.net", "springcm.com"} + +// validateClmURL rejects a URL that isn't a plausible CLM host — a guard for the Task +// API polling/continuation URLs (task Href, SearchFolders' ResultHref) that come from a +// response body or a round-tripped page token rather than being built from clmBaseURI +// like every other CLM request. doClmRequest attaches this connector's bearer token to +// whatever URL it's given, with no host check of its own, so a malformed or tampered +// href here would otherwise send that token to an arbitrary host. +// +// Deliberately not an exact match against the discovered base host: clmPreferredHref's +// doc (pkg/connector/helper.go) notes CLM's Href host isn't guaranteed to match the +// discovered base URL, and this package's own confirmed base-URL-resolution flow proves +// it — the Object API base and the account-discovery host are already two different +// domains. An exact-host check would risk hard-failing every genuine Task API call (and +// so the whole clm_folder sync) the first time CLM legitimately serves one from a +// sibling host. source names the caller/field for the error message. +func (c *Client) validateClmURL(u *url.URL, source string) error { + c.mutex.RLock() + clmBaseURI := c.clmBaseURI + c.mutex.RUnlock() + base, err := url.Parse(clmBaseURI) + if err != nil { + return fmt.Errorf("baton-docusign: invalid CLM base URL: %w", err) + } + if !strings.EqualFold(u.Scheme, base.Scheme) { + return fmt.Errorf("baton-docusign: refusing to send CLM credentials to %s %q — expected scheme %q", source, u.String(), base.Scheme) + } + // Exact match compares the full authority (host+port): a --base-url/mock target like + // http://127.0.0.1:5000 has no domain of its own to fall back on, so a same-host, + // different-port href must still be rejected there. Hostname() (no port) is only + // used in the domain-family loop below, where CLM's real hosts are all on 443. + if strings.EqualFold(u.Host, base.Host) { + return nil + } + host := u.Hostname() + for _, domain := range clmKnownDomains { + if strings.EqualFold(host, domain) || strings.HasSuffix(strings.ToLower(host), "."+domain) { + return nil + } + } + return fmt.Errorf("baton-docusign: refusing to send CLM credentials to %s %q — host %q is not a recognized CLM host", source, u.String(), host) +} + // doClmRequest executes an HTTP request against the CLM API and decodes the response. // Mirrors Client.doRequest but targets the CLM host/error envelope. func (c *Client) doClmRequest(ctx context.Context, method string, reqURL *url.URL, body any, response any, extraOpts ...uhttp.RequestOption) (annotations.Annotations, error) { @@ -274,46 +357,185 @@ func (c *Client) doClmRequest(ctx context.Context, method string, reqURL *url.UR return anno, err } -// SearchFolders discovers folders via the CLM Folders search endpoint (there is no -// flat list-all endpoint for folders, unlike Groups/Members/PermissionSets). +// clmMaxTaskPolls bounds how many times a CLM Task API poll loop (SearchFolders, +// PatchFolderSecurity) retries a not-yet-resolved task before giving up. Against a live +// CLM tenant, FolderSearchTasks always resolved inline (Status "Success" already in the +// POST response), so both polling branches are implemented per the Task API's +// documented contract but unverified live; the cap exists so a task that genuinely +// never resolves fails loudly instead of hanging. +const clmMaxTaskPolls = 30 + +// ClmFolderSearchTaskPollInterval is how long SearchFolders waits between polls of a +// "Processing" FolderSearchTasks task. Exported, like DefaultPageSize, so tests can +// override it — a real poll cadence would make a test exercising this branch +// needlessly slow. +var ClmFolderSearchTaskPollInterval = 2 * time.Second + +// SearchFolders discovers folders via CLM's FolderSearchTasks (CLM API Reference → +// Tasks → FolderSearchTasks). Unlike Groups/Members/PermissionSets there is no +// flat list-all for folders. The Reference also documents Folders:Search +// (POST /v2/{accountId}/folders/search); this connector follows FolderSearchTasks +// because that sync path returns 405 live (see bullets below). A POST creates a +// search task, which either resolves inline or must be polled via its own Href +// until Status leaves "Processing", after which the paginated folder list is read +// from the task's Result. (FolderSearchTasks' Status field is an unenumerated +// string in the schema — live returns Title-Case "Success"/"Processing", distinct +// from ChangeSecurityTasks' documented lowercase success/waiting/failure/processing.) // -// Pagination: offset/limit, see package doc. +// Confirmed live against a real CLM tenant: +// - POST /v2/{accountId}/folders/search (Folders:Search in the Reference — this +// function's original implementation) returns 405 Method Not Allowed, so the +// documented sync search is not usable on the tenants we hit. +// - POST /v2/{accountId}/foldersearchtasks requires a recognized search parameter in +// the body — an empty body, or {"Name": ...} (the field ClmFolder's own JSON tag +// uses), is rejected with CLM ErrorCode 1024 "no valid search parameter" against +// every property name tried except "Title". {"Title": ""} is accepted and matches +// every folder (Title is a substring match, so empty matches everything) — +// confirmed against a real account with 100 folders. Title is a documented +// FolderSearchTask request field in the Reference. +// - The task resolved inline (Status "Success" already in the POST response, Result +// already populated) on every live test; the "Processing" polling branch below is +// unverified live. +// - Continuation pages don't re-POST a new search: they GET the task's own Result +// href (offset/limit appended) directly, confirmed live to return the same flat, +// paginated shape as every other CLM list endpoint. func (c *Client) SearchFolders(ctx context.Context, options PageOptions) ([]ClmFolder, string, annotations.Annotations, error) { if err := c.ensureClmReady(ctx); err != nil { return nil, "", nil, err } - searchURL, requestedPage, err := c.prepareClmPagedRequest(clmSearchFolders, options) + if options.PageToken != "" { + decoded, err := decodeClmPageToken(options.PageToken) + if err != nil { + return nil, "", nil, fmt.Errorf("baton-docusign: invalid CLM page token: %w", err) + } + if decoded.ResultHref == "" { + // A SearchFolders continuation token without ResultHref would fall through + // to POST a brand-new search task (page 1 again) — an unbounded loop when + // more pages remain. Fail loud instead. + return nil, "", nil, fmt.Errorf("baton-docusign: CLM folder search page token missing ResultHref") + } + return c.getClmFolderSearchResultPage(ctx, decoded.ResultHref, options) + } + + createURL, err := c.buildClmClientURL(clmCreateFolderSearchTask) + if err != nil { + return nil, "", nil, err + } + // Page-1 unconfirmed live: FolderSearchTasks' create response embeds a page of + // results inline (see this func's doc), and every other paged CLM request controls + // its page size via pageSortParams.limit on the request URL — applying the same + // convention here on the POST, rather than leaving page 1 to whatever CLM's default + // happens to be, since options.PageSize should govern the first page like it does + // every continuation page (getClmFolderSearchResultPage). + createURL, requestedPage, err := appendClmPageQuery(createURL, options) if err != nil { return nil, "", nil, err } - var page ClmFolderPage - anno, err := c.doClmRequest(ctx, http.MethodPost, searchURL, struct{}{}, &page) + var task ClmFolderSearchTaskResponse + anno, err := c.doClmRequest(ctx, http.MethodPost, createURL, map[string]string{"Title": ""}, &task) if err != nil { - return nil, "", nil, fmt.Errorf("baton-docusign: failed to search CLM folders: %w", err) + return nil, "", nil, fmt.Errorf("baton-docusign: failed to create CLM folder search task: %w", err) } - // The request body is empty (no search criteria) because the schema for scoping - // this search to "all folders" was never confirmed against a live CLM tenant. If - // that empty body means "no criteria -> no matches" rather than "match all", the - // very first page would come back empty and every folder/folder-security sync - // would silently report success while syncing zero folders. Surface that - // possibility in the logs rather than fail silently, without treating it as a - // hard error since an account with genuinely zero folders is also a valid state. - if requestedPage.Offset == 0 && len(page.Items) == 0 { - ctxzap.Extract(ctx).Debug("baton-docusign: CLM folder search returned zero results on the first page; " + - "if this account has CLM folders, this may indicate the empty search body is being interpreted as " + - "'no criteria -> no matches' rather than 'match all' — please report this to ConductorOne") + task, err = c.awaitClmFolderSearchTask(ctx, task) + if err != nil { + return nil, "", anno, err + } + if task.Result == nil { + return nil, "", anno, fmt.Errorf("baton-docusign: CLM folder search task %s succeeded with no Result", task.Href) + } + // Prefer the smaller of the requested and echoed page sizes: applying + // pageSortParams.limit to the create POST is unconfirmed live (see this func's + // doc), so either direction of mismatch is possible. An echoed Limit smaller than + // requested (CLM ignored the param, served its own default) would make a + // genuinely full page look short if left at the requested size; an echoed Limit + // larger than requested (e.g. CLM's max page size rather than what it actually + // applied) would make a genuinely full page look short the other way if trusted + // outright. Taking the minimum is safe either way — worst case it costs one extra + // empty-page request, never lost data. + if task.Result.Limit > 0 && task.Result.Limit < requestedPage.PageSize { + requestedPage.PageSize = task.Result.Limit } - nextToken, err := getClmNextToken(requestedPage, len(page.Items), page.Next != "", page.Total) + nextToken, err := getClmNextToken(requestedPage, len(task.Result.Items), task.Result.Next != "", task.Result.Total, task.Result.Href) + if err != nil { + return nil, "", anno, err + } + // getClmNextToken will happily mint a token with ResultHref:"" when Href is empty. + // The next SearchFolders call would then re-POST (Requests reset to 0) and loop on + // page 1 forever — maxClmListPages never fires. Refuse to emit that token. + if nextToken != "" && task.Result.Href == "" { + return nil, "", anno, fmt.Errorf("baton-docusign: CLM folder search task %s Result has no Href; cannot continue pagination", task.Href) + } + return task.Result.Items, nextToken, anno, nil +} + +// getClmFolderSearchResultPage fetches one continuation page of an already-completed +// CLM folder search — see SearchFolders' doc for why this reads from a server-issued +// Result href instead of re-POSTing a new search task. +func (c *Client) getClmFolderSearchResultPage(ctx context.Context, resultHref string, options PageOptions) ([]ClmFolder, string, annotations.Annotations, error) { + base, err := url.Parse(resultHref) + if err != nil { + return nil, "", nil, fmt.Errorf("baton-docusign: invalid CLM folder search result href %q: %w", resultHref, err) + } + if err := c.validateClmURL(base, "folder search result href"); err != nil { + return nil, "", nil, err + } + pageURL, requestedPage, err := appendClmPageQuery(base, options) + if err != nil { + return nil, "", nil, err + } + + var page ClmFolderPage + anno, err := c.doClmRequest(ctx, http.MethodGet, pageURL, nil, &page) + if err != nil { + return nil, "", nil, fmt.Errorf("baton-docusign: failed to read CLM folder search results: %w", err) + } + + nextToken, err := getClmNextToken(requestedPage, len(page.Items), page.Next != "", page.Total, resultHref) if err != nil { return nil, "", anno, err } return page.Items, nextToken, anno, nil } +// awaitClmFolderSearchTask polls a CLM FolderSearchTasks task until it leaves +// "Processing", per the CLM Task API's documented contract (GET the task's own Href; +// Status becomes "Success" or "Failure") — see SearchFolders' doc for why this branch +// is unverified against a live tenant. +func (c *Client) awaitClmFolderSearchTask(ctx context.Context, task ClmFolderSearchTaskResponse) (ClmFolderSearchTaskResponse, error) { + for attempt := 0; task.Status == "Processing"; attempt++ { + if attempt >= clmMaxTaskPolls { + return task, fmt.Errorf("baton-docusign: CLM folder search task %s did not finish after %d polls", task.Href, clmMaxTaskPolls) + } + select { + case <-ctx.Done(): + return task, ctx.Err() + case <-time.After(ClmFolderSearchTaskPollInterval): + } + + pollURL, err := url.Parse(task.Href) + if err != nil { + return task, fmt.Errorf("baton-docusign: invalid CLM folder search task href %q: %w", task.Href, err) + } + if err := c.validateClmURL(pollURL, "folder search task href"); err != nil { + return task, err + } + // WithNoCache: repeated GETs to the same task URL must observe its latest + // Status, not a memoized first response — see GetFolder's noCache param for the + // same uhttp GET-cache staleness concern. + if _, err := c.doClmRequest(ctx, http.MethodGet, pollURL, nil, &task, uhttp.WithNoCache()); err != nil { + return task, fmt.Errorf("baton-docusign: failed to poll CLM folder search task %s: %w", task.Href, err) + } + } + if task.Status == "Failure" { + return task, fmt.Errorf("baton-docusign: CLM folder search task %s failed", task.Href) + } + return task, nil +} + // GetFolder fetches a single folder, optionally expanding Security to get its explicit // (non-inherited) folder security entries. The response may be served from the shared // HTTP GET cache. @@ -361,39 +583,113 @@ func (c *Client) getFolder(ctx context.Context, folderID string, noCache bool, e // complete security state across all three principal-type collections (see the // connector-layer caller — clmFolderSecurityToWrite builds this from a fresh read, // with one entry changed/added in whichever of Groups/Roles/Users the grant/revoke -// targets), not just the one entry being changed: Folders.Patch's merge-vs-replace +// targets), not just the one entry being changed: ChangeSecurityTasks' merge-vs-replace // semantics for the Security field are undocumented, and sending the complete state // is correct under either interpretation, whereas sending only the one changed entry // would wipe every other principal's access to the folder if the real API replaces // rather than merges. // -// UNVERIFIED against a real CLM tenant, and higher-risk than the merge-semantics -// question above: DocuSign's docs also reference a separate `ChangeSecurityTasks` -// resource (POST /v2/{accountId}/changesecuritytasks), which may be the documented -// way to mutate folder security asynchronously rather than a direct PATCH on the -// folder itself. If that's the actual contract, this method - the entire mechanism -// Grant/Revoke on clm_folder relies on - could silently no-op or 404 against a real -// tenant. Verify which endpoint the real API expects before treating folder -// provisioning here as more than best-effort. +// Sets a folder's security via CLM's ChangeSecurityTasks — a dedicated async Task API +// endpoint, NOT the generic Folders Patch this function used to call. Confirmed live +// that the generic PATCH /v2/{accountId}/folders/{id} silently ignores a Security +// payload entirely (200 OK, no error, but a fresh GET shows no change — a trivial PATCH +// of another field on the same folder DID apply and bump UpdatedDate, isolating the +// failure to Security specifically); CLM's own error code list separately names +// "136 - Missing Change Security Task", and the CLM API Reference confirms a distinct +// ChangeSecurityTasks resource ("Post: Set the security on a folder") exists for +// exactly this. See clm_client.go's package doc "Folders" section for the live +// evidence that led here. +// +// The request body shape (ClmChangeSecurityTaskRequest) is confirmed via the CLM API +// Reference's interactive schema browser, expanded past its default collapsed view — +// the target folder's Href and its new Security both nest under a Folder field; a +// same-shaped top-level Href/Security/Status on the schema are generic Task-wrapper +// fields this doc-generation tool reuses across every Task type, not what +// ChangeSecurityTasks actually reads for a folder security change (an earlier version +// of this code got this wrong from the same page's default collapsed view, which looks +// identical to a flat {Href, Security} pair until "Folder" is expanded). +// +// NOT independently verified live: per explicit instruction, this project is done +// live-testing against the customer's tenant. ChangeSecurityTasks' Status values are +// documented in lowercase (success/waiting/failure/processing) — confirmed distinct +// from FolderSearchTasks' PascalCase (Success/Processing), not a documentation +// inconsistency to normalize away — is the other thing worth a second look if this +// doesn't work in practice. func (c *Client) PatchFolderSecurity(ctx context.Context, folderID string, write ClmFolderSecurityWrite) (annotations.Annotations, error) { if err := c.ensureClmReady(ctx); err != nil { return nil, err } - folderURL, err := c.buildClmClientURL(clmPatchFolder, folderID) + folderURL, err := c.buildClmClientURL(clmGetFolder, folderID) + if err != nil { + return nil, err + } + + createURL, err := c.buildClmClientURL(clmCreateChangeSecurityTask) if err != nil { return nil, err } - body := ClmFolderSecurityPatch{Security: write} - anno, err := c.doClmRequest(ctx, http.MethodPatch, folderURL, body, nil) + body := ClmChangeSecurityTaskRequest{Folder: ClmChangeSecurityTaskFolder{Href: folderURL.String(), Security: write}} + + var task ClmChangeSecurityTaskResponse + anno, err := c.doClmRequest(ctx, http.MethodPost, createURL, body, &task) if err != nil { - return anno, fmt.Errorf("baton-docusign: failed to update CLM folder %s security: %w", folderID, err) + return anno, fmt.Errorf("baton-docusign: failed to create CLM change-security task for folder %s: %w", folderID, err) + } + + task, err = c.awaitClmChangeSecurityTask(ctx, task) + if err != nil { + return anno, err + } + if task.Status != ClmChangeSecurityStatusSuccess { + return anno, fmt.Errorf("baton-docusign: CLM change-security task %s for folder %s did not succeed (status %q)", task.Href, folderID, task.Status) } return anno, nil } +// clmChangeSecurityStatus* are ChangeSecurityTasks' documented Status values — +// lowercase, distinct from FolderSearchTasks' PascalCase. See PatchFolderSecurity's doc. +const ( + ClmChangeSecurityStatusSuccess = "success" + ClmChangeSecurityStatusWaiting = "waiting" + ClmChangeSecurityStatusFailure = "failure" + ClmChangeSecurityStatusProcessing = "processing" +) + +// awaitClmChangeSecurityTask polls a CLM ChangeSecurityTasks task until it leaves +// "waiting"/"processing" — mirrors awaitClmFolderSearchTask's polling loop, but against +// ChangeSecurityTasks' distinct (lowercase) Status vocabulary and leaner response shape +// (no Result field: this task mutates rather than returns data, so success/failure is +// the only thing to observe). Unverified live — see PatchFolderSecurity's doc. +func (c *Client) awaitClmChangeSecurityTask(ctx context.Context, task ClmChangeSecurityTaskResponse) (ClmChangeSecurityTaskResponse, error) { + for attempt := 0; task.Status == ClmChangeSecurityStatusWaiting || task.Status == ClmChangeSecurityStatusProcessing; attempt++ { + if attempt >= clmMaxTaskPolls { + return task, fmt.Errorf("baton-docusign: CLM change-security task %s did not finish after %d polls", task.Href, clmMaxTaskPolls) + } + select { + case <-ctx.Done(): + return task, ctx.Err() + case <-time.After(ClmFolderSearchTaskPollInterval): + } + + pollURL, err := url.Parse(task.Href) + if err != nil { + return task, fmt.Errorf("baton-docusign: invalid CLM change-security task href %q: %w", task.Href, err) + } + if err := c.validateClmURL(pollURL, "change-security task href"); err != nil { + return task, err + } + // WithNoCache: see awaitClmFolderSearchTask's identical comment — repeated polls + // of the same task URL must observe its latest Status, not a memoized first one. + if _, err := c.doClmRequest(ctx, http.MethodGet, pollURL, nil, &task, uhttp.WithNoCache()); err != nil { + return task, fmt.Errorf("baton-docusign: failed to poll CLM change-security task %s: %w", task.Href, err) + } + } + return task, nil +} + // ListGroups lists CLM groups (a distinct object from eSignature groups). // // Pagination: offset/limit, see package doc. @@ -413,13 +709,39 @@ func (c *Client) ListGroups(ctx context.Context, options PageOptions) ([]ClmGrou return nil, "", nil, fmt.Errorf("baton-docusign: failed to list CLM groups: %w", err) } - nextToken, err := getClmNextToken(requestedPage, len(page.Items), page.Next != "", page.Total) + nextToken, err := getClmNextToken(requestedPage, len(page.Items), page.Next != "", page.Total, "") if err != nil { return nil, "", anno, err } return page.Items, nextToken, anno, nil } +// hrefFor builds an object's Href from its native ID and the resolved CLM base URL, +// shared by GroupHref and MemberHref, for callers that only have a ResourceId (no +// hydrated Resource to read a Href from). Assumes the shape "/v2/{account}/{collection}/{id}"; +// unverified against a live tenant. Builds the string locally — issues no request beyond +// the one-time CLM base-URL discovery ensureClmReady may trigger. +func (c *Client) hrefFor(ctx context.Context, pathShape, id string) (string, error) { + if err := c.ensureClmReady(ctx); err != nil { + return "", err + } + objURL, err := c.buildClmClientURL(pathShape, id) + if err != nil { + return "", err + } + return objURL.String(), nil +} + +// GroupHref builds a CLM group's Href from its native ID — see hrefFor's doc. +func (c *Client) GroupHref(ctx context.Context, groupID string) (string, error) { + return c.hrefFor(ctx, clmGroupPath, groupID) +} + +// MemberHref builds a CLM member's Href from its native ID — see hrefFor's doc. +func (c *Client) MemberHref(ctx context.Context, memberID string) (string, error) { + return c.hrefFor(ctx, clmMemberPath, memberID) +} + // GetGroupMembers lists the members of a CLM group. // // Pagination: offset/limit, see package doc. @@ -439,7 +761,7 @@ func (c *Client) GetGroupMembers(ctx context.Context, groupID string, options Pa return nil, "", nil, fmt.Errorf("baton-docusign: failed to list members of CLM group %s: %w", groupID, err) } - nextToken, err := getClmNextToken(requestedPage, len(page.Items), page.Next != "", page.Total) + nextToken, err := getClmNextToken(requestedPage, len(page.Items), page.Next != "", page.Total, "") if err != nil { return nil, "", anno, err } @@ -467,7 +789,7 @@ func (c *Client) ListMembers(ctx context.Context, options PageOptions) ([]ClmMem return nil, "", nil, fmt.Errorf("baton-docusign: failed to list CLM members: %w", err) } - nextToken, err := getClmNextToken(requestedPage, len(page.Items), page.Next != "", page.Total) + nextToken, err := getClmNextToken(requestedPage, len(page.Items), page.Next != "", page.Total, "") if err != nil { return nil, "", anno, err } @@ -540,7 +862,7 @@ func (c *Client) getMemberGroupsPage(ctx context.Context, memberID string, optio return nil, "", nil, fmt.Errorf("baton-docusign: failed to get groups for CLM member %s: %w", memberID, err) } - nextToken, err := getClmNextToken(requestedPage, len(page.Items), page.Next != "", page.Total) + nextToken, err := getClmNextToken(requestedPage, len(page.Items), page.Next != "", page.Total, "") if err != nil { return nil, "", anno, err } @@ -616,7 +938,7 @@ func (c *Client) ListPermissionSets(ctx context.Context, options PageOptions) ([ return nil, "", nil, fmt.Errorf("baton-docusign: failed to list CLM permission sets: %w", err) } - nextToken, err := getClmNextToken(requestedPage, len(page.Items), page.Next != "", page.Total) + nextToken, err := getClmNextToken(requestedPage, len(page.Items), page.Next != "", page.Total, "") if err != nil { return nil, "", anno, err } diff --git a/pkg/client/clm_client_test.go b/pkg/client/clm_client_test.go index 70da9684..5d773de0 100644 --- a/pkg/client/clm_client_test.go +++ b/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" @@ -13,12 +15,14 @@ func TestSearchFolders_Pagination(t *testing.T) { ctx := context.Background() var all []client.ClmFolder + pages := 0 pageToken := "" for i := 0; i < 10; i++ { // safety bound for the test loop itself folders, next, _, err := c.SearchFolders(ctx, client.PageOptions{PageSize: 2, PageToken: pageToken}) if err != nil { t.Fatalf("SearchFolders page %d: %v", i, err) } + pages++ all = append(all, folders...) if next == "" { break @@ -26,17 +30,119 @@ func TestSearchFolders_Pagination(t *testing.T) { pageToken = next } + // Pins the requested PageSize actually reaching the create-task POST (not just + // continuation pages): with PageSize 2 and 3 seeded folders, a real second page is + // the only way this test exercises getClmFolderSearchResultPage and the ResultHref + // token round-trip at all. + if pages < 2 { + t.Fatalf("expected SearchFolders to paginate across at least 2 pages with PageSize 2 and 3 folders, got %d page(s)", pages) + } if len(all) != 3 { t.Fatalf("expected 3 folders across all pages, got %d", len(all)) } // 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) + } +} + +// TestSearchFolders_RejectsTaskHrefOnUnexpectedHost is a regression test for +// Client.validateClmURL: doClmRequest attaches the bearer token to whatever URL it's +// given, so a task Href pointing at a host other than the discovered CLM base URL must +// be rejected before it's ever dispatched, not just parsed. +func TestSearchFolders_RejectsTaskHrefOnUnexpectedHost(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(1) + srv.SetFolderSearchTaskHrefOverride("http://attacker.example.com/v2/acct-clm-test/foldersearchtasks/1") + + _, _, _, err := c.SearchFolders(ctx, client.PageOptions{PageSize: 10}) + if err == nil { + t.Fatal("expected SearchFolders to reject a task href on an unexpected host, got nil error") + } + if !strings.Contains(err.Error(), "refusing to send CLM credentials") { + t.Fatalf("expected a host-validation error, got: %v", err) + } +} + func TestGetFolder_ExpandSecurity(t *testing.T) { _, c := clmtest.NewServer(t) ctx := context.Background() @@ -46,7 +152,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 +162,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 +199,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 +211,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) } } diff --git a/pkg/client/clm_helper.go b/pkg/client/clm_helper.go index 1c4dfcb8..11461933 100644 --- a/pkg/client/clm_helper.go +++ b/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 } diff --git a/pkg/client/clm_helper_test.go b/pkg/client/clm_helper_test.go index b0343ff7..423f864c 100644 --- a/pkg/client/clm_helper_test.go +++ b/pkg/client/clm_helper_test.go @@ -2,6 +2,7 @@ package client import ( "encoding/json" + "net/url" "testing" ) @@ -50,7 +51,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 +79,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,7 +114,7 @@ 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") } @@ -121,7 +122,7 @@ func TestGetClmNextToken_CapsRunawayPagination(t *testing.T) { 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 +135,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 +148,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") } @@ -215,3 +216,53 @@ func TestClmExtractBaseURLField_FindsRecognizedFieldRegardlessOfShape(t *testing }) } } + +// TestValidateClmURL is a regression test for a review finding: an exact-host check +// against the discovered CLM base URL would hard-fail every Task API call the first +// time CLM legitimately serves a task/result href from a sibling host — this package's +// own confirmed base-URL-resolution flow already spans two unrelated domains +// (*.clm.docusign.net for the Object API, auth.springcm.com for discovery), so +// validateClmURL checks domain family instead of exact host equality. +func TestValidateClmURL(t *testing.T) { + c := &Client{clmBaseURI: "https://api.na1.clm.docusign.net"} + + tests := []struct { + name string + rawURL string + wantErr bool + }{ + {"exact host match", "https://api.na1.clm.docusign.net/v2/acct/foldersearchtasks/1", false}, + {"sibling docusign.net host", "https://tasks.na2.clm.docusign.net/v2/acct/foldersearchtasks/1", false}, + {"springcm.com sibling (CLM's other confirmed domain)", "https://auth.springcm.com/v2/acct/foldersearchtasks/1", false}, + {"unrelated host", "https://attacker.example.com/v2/acct/foldersearchtasks/1", true}, + {"docusign.net as a suffix of an unrelated domain is not a match", "https://evil-docusign.net.attacker.com/x", true}, + {"scheme mismatch", "http://api.na1.clm.docusign.net/v2/acct/foldersearchtasks/1", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + u, err := url.Parse(tt.rawURL) + if err != nil { + t.Fatalf("url.Parse: %v", err) + } + err = c.validateClmURL(u, "test href") + if (err != nil) != tt.wantErr { + t.Errorf("validateClmURL(%q) error = %v, wantErr %v", tt.rawURL, err, tt.wantErr) + } + }) + } + + // A --base-url/mock target has no domain of its own to fall back on, so the exact + // match must compare host+port, not just host: switching to Hostname() (which + // strips the port) for that branch would let a same-host, different-port href + // through. + t.Run("exact-match branch rejects a same-host different-port href", func(t *testing.T) { + mockClient := &Client{clmBaseURI: "http://127.0.0.1:5000"} + u, err := url.Parse("http://127.0.0.1:9999/v2/acct/foldersearchtasks/1") + if err != nil { + t.Fatalf("url.Parse: %v", err) + } + if err := mockClient.validateClmURL(u, "test href"); err == nil { + t.Fatal("expected a different port on a non-domain host to be rejected") + } + }) +} diff --git a/pkg/client/clm_models.go b/pkg/client/clm_models.go index aae60104..c8a8f1ff 100644 --- a/pkg/client/clm_models.go +++ b/pkg/client/clm_models.go @@ -1,6 +1,28 @@ package client -import "fmt" +import ( + "encoding/json" + "fmt" + "sort" +) + +// clmTopLevelKeys returns the sorted top-level JSON keys of data, for describing an +// unrecognized security-entry wire shape in an error without echoing the entry's actual +// field values — a member/group entry's raw JSON can carry Email/Name/etc. that +// shouldn't end up verbatim in a sync task error message. Returns nil if data isn't a +// JSON object. +func clmTopLevelKeys(data []byte) []string { + var raw map[string]json.RawMessage + if err := json.Unmarshal(data, &raw); err != nil { + return nil + } + keys := make([]string, 0, len(raw)) + for k := range raw { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} // ClmPage is the pagination metadata CLM's Object API embeds in every list response — // distinct from eSignature's Page (see models.go). Each *Page wrapper type below embeds @@ -17,18 +39,31 @@ type ClmPage struct { Total int `json:"Total"` } -// ClmErrorResponse is CLM's error envelope. DocuSign's API reference does not document -// an error response shape, so this is intentionally loose; callers should log the raw -// body if fields don't populate as expected. +// ClmErrorResponse is CLM's error envelope — confirmed via a live 401 from a real CLM +// tenant: {"Error":{"HttpStatusCode":401,"UserMessage":"Access Denied", +// "DeveloperMessage":"Access Denied","ErrorCode":103,"ReferenceId":"..."}}. Errors are +// nested under "Error", not a top-level "Message" field. type ClmErrorResponse struct { - Msg string `json:"Message"` + Error struct { + UserMessage string `json:"UserMessage"` + DeveloperMessage string `json:"DeveloperMessage"` + ErrorCode int `json:"ErrorCode"` + ReferenceId string `json:"ReferenceId"` + } `json:"Error"` } func (e *ClmErrorResponse) Message() string { - if e.Msg == "" { + primary := e.Error.UserMessage + if primary == "" { + primary = e.Error.DeveloperMessage + } + if primary == "" { return "unknown CLM API error" } - return fmt.Sprintf("CLM API error: %s", e.Msg) + if e.Error.UserMessage != "" && e.Error.DeveloperMessage != "" && e.Error.DeveloperMessage != e.Error.UserMessage { + return fmt.Sprintf("CLM API error %d: %s (%s)", e.Error.ErrorCode, primary, e.Error.DeveloperMessage) + } + return fmt.Sprintf("CLM API error %d: %s", e.Error.ErrorCode, primary) } // ClmFolder represents a CLM Folder object. @@ -60,25 +95,67 @@ type ClmFolderPage struct { Items []ClmFolder `json:"Items"` } -// ClmFolderSecurity is a folder's explicit (non-inherited) security assignments, -// confirmed via DocuSign's own Folders.Patch reference page (pasted live, since the -// site is JS-rendered and unreachable by automated tools) to be three SEPARATE -// collections by principal type — not a single flat list, and not the -// AccessType-vs-boolean-flags dual representation an earlier version of this file -// assumed. No boolean flags (Create/Move/Read/See/SetAccess/Write) appear anywhere in -// the confirmed schema; every entry across all three collections carries AccessType -// directly. +// ClmFolderSearchTaskResponse is CLM's FolderSearchTasks response envelope — the same +// shape whether returned by the initial POST (create) or a poll GET on the task's own +// Href. Confirmed live for the POST/"Success" case (Result already populated with the +// same Items/Offset/Limit/Total/Next fields as every other CLM list endpoint); the CLM +// Task API 101 docs describe the identical {Status, Href, Result} envelope for +// DocumentSearchTasks, CLM's sibling search-task resource — see SearchFolders' doc in +// clm_client.go for what's confirmed vs. assumed. +type ClmFolderSearchTaskResponse struct { + Status string `json:"Status"` + Href string `json:"Href"` + Result *ClmFolderPage `json:"Result,omitempty"` +} + +// ClmChangeSecurityTaskResponse is CLM's ChangeSecurityTasks response envelope — per +// the CLM API Reference's documented "Task.ChangeSecurityTask" schema (confirmed live +// via the rendered doc site, not just its collapsed default view — see +// ClmChangeSecurityTaskRequest's doc for why that distinction mattered here). The full +// schema also lists top-level Folder and Security fields alongside Href/Status, but +// PatchFolderSecurity only needs Status to decide success/failure; Href is this task's +// own poll URL, echoed back from the create call. Status uses a distinct, lowercase +// vocabulary from FolderSearchTasks — see PatchFolderSecurity's doc in clm_client.go. +// Not independently confirmed live (no more live-testing against the customer tenant). +type ClmChangeSecurityTaskResponse struct { + Href string `json:"Href"` + Status string `json:"Status"` +} + +// ClmFolderSecurity is a folder's explicit (non-inherited) security assignments. +// Confirmed live against a real CLM tenant (GetFolder?expand=Security) to be three +// SEPARATE, flat (non-paginated) arrays by principal type — no First/Href/Last/Limit/ +// Next/Offset/Previous/Total pagination envelope wraps them the way every other CLM +// list response does; an earlier version of this struct wrapped each in a page type +// based on an unconfirmed reading of the Folders.Patch reference page, which caused a +// live sync to fail entirely with a JSON unmarshal error (array where an object with an +// Items field was expected). type ClmFolderSecurity struct { - Groups ClmGroupSecurityPage `json:"Groups,omitempty"` - Roles ClmRoleSecurityPage `json:"Roles,omitempty"` - Users ClmUserSecurityPage `json:"Users,omitempty"` + Groups []ClmGroupSecurityEntry `json:"Groups,omitempty"` + Roles []ClmRoleSecurityEntry `json:"Roles,omitempty"` + Users []ClmUserSecurityEntry `json:"Users,omitempty"` } -// ClmGroupSecurityEntry is one folder-security grant to a CLM Group. Confirmed shape: -// the full Group object's own fields (Href/Name/GroupType/Description/CreatedDate/ -// UpdatedDate) plus AccessType — not a lean {Item, AccessType} pair. +// ClmGroupSecurityEntry is one folder-security grant to a CLM Group. Confirmed live: +// the wire shape nests the group's own fields under an "Item" key, sibling to +// AccessType — {"Item": {"Href":...,"Name":...,...}, "AccessType":"View"} — not a flat +// merge of AccessType into the group's fields as an earlier version of this struct +// assumed (that assumption was never actually confirmed against a live tenant, despite +// its doc comment's claim otherwise). Kept as a flat Go struct via custom (Un)MarshalJSON +// so callers in pkg/connector/clm_folders.go don't need to know about the wire-level +// nesting — mirrors ClmRoleSecurityEntry's existing bare-Item shape, just for an object +// Item instead of a string one. type ClmGroupSecurityEntry struct { - AccessType string `json:"AccessType,omitempty"` + AccessType string + Href string + Name string + GroupType string + Description string + CreatedDate string + UpdatedDate string +} + +type clmGroupSecurityItem struct { Href string `json:"Href"` Name string `json:"Name,omitempty"` GroupType string `json:"GroupType,omitempty"` @@ -87,52 +164,223 @@ type ClmGroupSecurityEntry struct { UpdatedDate string `json:"UpdatedDate,omitempty"` } -// ClmGroupSecurityPage is the paginated collection of ClmGroupSecurityEntry returned -// on a read (GetFolder?expand=Security). See ClmFolderSecurityWrite for the plain-list -// shape used on writes. -type ClmGroupSecurityPage struct { - ClmPage - Items []ClmGroupSecurityEntry `json:"Items"` +func (e ClmGroupSecurityEntry) MarshalJSON() ([]byte, error) { + return json.Marshal(struct { + Item clmGroupSecurityItem `json:"Item"` + AccessType string `json:"AccessType,omitempty"` + }{ + Item: clmGroupSecurityItem{ + Href: e.Href, + Name: e.Name, + GroupType: e.GroupType, + Description: e.Description, + CreatedDate: e.CreatedDate, + UpdatedDate: e.UpdatedDate, + }, + AccessType: e.AccessType, + }) +} + +func (e *ClmGroupSecurityEntry) UnmarshalJSON(data []byte) error { + var wire struct { + Item json.RawMessage `json:"Item"` + AccessType string `json:"AccessType"` + } + if err := json.Unmarshal(data, &wire); err != nil { + return err + } + var item clmGroupSecurityItem + if len(wire.Item) > 0 { + if err := json.Unmarshal(wire.Item, &item); err != nil { + return err + } + } else { + // No "Item" key at all — mirrors ClmUserSecurityEntry's flat-shape fallback. + // Groups' nested shape is confirmed live, so this is defense against a + // hypothetical regression rather than a known gap, but the alternative (hard + // error on any flat entry) would take down the whole folder's Grants/Grant/ + // Revoke rather than just this one entry. + if err := json.Unmarshal(data, &item); err != nil { + return err + } + } + if item.Href == "" { + // Same reasoning as ClmUserSecurityEntry's identical check: an empty Href here + // would round-trip into a PatchFolderSecurity body and silently drop this + // group's real folder access under replace semantics. Reports only the + // top-level key names, not data itself — a group entry's raw JSON can carry + // Name/Description that shouldn't end up verbatim in a sync task error. + return fmt.Errorf("baton-docusign: CLM group security entry has no Href — unrecognized wire shape (keys: %v)", clmTopLevelKeys(data)) + } + *e = ClmGroupSecurityEntry{ + AccessType: wire.AccessType, + Href: item.Href, + Name: item.Name, + GroupType: item.GroupType, + Description: item.Description, + CreatedDate: item.CreatedDate, + UpdatedDate: item.UpdatedDate, + } + return nil } -// ClmRoleSecurityEntry is one folder-security grant to a CLM Role. Confirmed shape: -// flat {AccessType, Item} — unlike Groups/Users, a Role has no separate object to -// expand, so Item is just the role name string. +// ClmRoleSecurityEntry is one folder-security grant to a CLM Role. Confirmed shape on +// writes (this connector's own PATCH body): flat {AccessType, Item}, Item the bare role +// name — unlike Groups/Users, a Role has no separate object to expand. Reads use a +// custom UnmarshalJSON tolerating either that flat string or a Groups/Users-style +// nested {Item: {Name: ...}} object: only Groups has been independently confirmed live +// (see ClmUserSecurityEntry's doc for the same gap on Users), so if CLM nests Roles too, +// decoding a JSON object into a bare Go string would otherwise hard-fail every +// clm_folder read/Grant/Revoke on that folder instead of just this one entry. type ClmRoleSecurityEntry struct { AccessType string `json:"AccessType,omitempty"` Item string `json:"Item"` } -// ClmRoleSecurityPage is the paginated collection of ClmRoleSecurityEntry. -type ClmRoleSecurityPage struct { - ClmPage - Items []ClmRoleSecurityEntry `json:"Items"` +func (e ClmRoleSecurityEntry) MarshalJSON() ([]byte, error) { + return json.Marshal(struct { + AccessType string `json:"AccessType,omitempty"` + Item string `json:"Item"` + }{AccessType: e.AccessType, Item: e.Item}) } -// ClmUserSecurityEntry is one folder-security grant to a CLM Member (user). Confirmed -// shape: the Member object's own identifying fields plus AccessType — mirrors -// ClmGroupSecurityEntry's pattern. Deliberately doesn't repeat every field ClmMember -// has (Address*, City, Company, etc.): Grant/Revoke only ever need Href to identify -// the member, never reconstruct a full member profile from a security entry. +func (e *ClmRoleSecurityEntry) UnmarshalJSON(data []byte) error { + var wire struct { + AccessType string `json:"AccessType"` + Item json.RawMessage `json:"Item"` + } + if err := json.Unmarshal(data, &wire); err != nil { + return err + } + var name string + if len(wire.Item) > 0 { + if err := json.Unmarshal(wire.Item, &name); err != nil { + var obj struct { + Name string `json:"Name"` + } + if err2 := json.Unmarshal(wire.Item, &obj); err2 != nil { + return fmt.Errorf("baton-docusign: CLM role security entry Item is neither a string nor an object with Name: %w", err) + } + name = obj.Name + } + } + *e = ClmRoleSecurityEntry{AccessType: wire.AccessType, Item: name} + return nil +} + +// ClmUserSecurityEntry is one folder-security grant to a CLM Member (user). Read via a +// custom UnmarshalJSON tolerating both ClmGroupSecurityEntry's confirmed nested +// {Item: {...}, AccessType} wire shape and a flat {Href, AccessType, ...} one: Users' +// shape isn't independently confirmed live (every populated folder-security entry found +// on the live tenant this was tested against was a Group), and guessing wrong on a +// struct-typed Item field fails silently (a missing key leaves Item's fields at their +// zero value, not a decode error) rather than loudly — see the review finding that +// caught this: every existing user's Href would decode as "", and since +// clmFolderSecurityToWrite round-trips the complete security state on every +// Grant/Revoke, an unrelated write could silently blank and then drop every other +// user's folder access. Deliberately doesn't repeat every field ClmMember has +// (Address*, City, Company, etc.): Grant/Revoke only ever need Href to identify the +// member, never reconstruct a full member profile from a security entry. type ClmUserSecurityEntry struct { - AccessType string `json:"AccessType,omitempty"` - Href string `json:"Href"` - Email string `json:"Email,omitempty"` - UserName string `json:"UserName,omitempty"` - FirstName string `json:"FirstName,omitempty"` - LastName string `json:"LastName,omitempty"` - Role string `json:"Role,omitempty"` + AccessType string + Href string + Email string + UserName string + FirstName string + LastName string + Role string } -// ClmUserSecurityPage is the paginated collection of ClmUserSecurityEntry. -type ClmUserSecurityPage struct { - ClmPage - Items []ClmUserSecurityEntry `json:"Items"` +type clmUserSecurityItem struct { + Href string `json:"Href"` + Email string `json:"Email,omitempty"` + UserName string `json:"UserName,omitempty"` + FirstName string `json:"FirstName,omitempty"` + LastName string `json:"LastName,omitempty"` + Role string `json:"Role,omitempty"` +} + +func (e ClmUserSecurityEntry) MarshalJSON() ([]byte, error) { + return json.Marshal(struct { + Item clmUserSecurityItem `json:"Item"` + AccessType string `json:"AccessType,omitempty"` + }{ + Item: clmUserSecurityItem{ + Href: e.Href, + Email: e.Email, + UserName: e.UserName, + FirstName: e.FirstName, + LastName: e.LastName, + Role: e.Role, + }, + AccessType: e.AccessType, + }) +} + +func (e *ClmUserSecurityEntry) UnmarshalJSON(data []byte) error { + var wire struct { + Item json.RawMessage `json:"Item"` + AccessType string `json:"AccessType"` + } + if err := json.Unmarshal(data, &wire); err != nil { + return err + } + var item clmUserSecurityItem + if len(wire.Item) > 0 { + if err := json.Unmarshal(wire.Item, &item); err != nil { + return err + } + } else { + // No "Item" key at all — the flat-shape fallback (see this type's doc). Decode + // the same fields straight off the top level instead of leaving item at its zero + // value, which would otherwise silently produce Href == "". + if err := json.Unmarshal(data, &item); err != nil { + return err + } + } + if item.Href == "" { + // Neither the nested nor the flat decode found a Href — an unrecognized wire + // shape (e.g. "Item":null, "Item":{}, or the member nested under some other + // key). Fail loud rather than let clmFolderSecurityToWrite round-trip an + // empty-Href entry into a PatchFolderSecurity body, which would silently drop + // this user's real folder access under replace semantics. Reports only the + // top-level key names, not data itself — this error propagates out through + // Grants/Grant/Revoke into the sync task's own error, and a member entry's raw + // JSON can carry Email/FirstName/LastName that shouldn't end up there verbatim. + return fmt.Errorf("baton-docusign: CLM user security entry has no Href — unrecognized wire shape (keys: %v)", clmTopLevelKeys(data)) + } + *e = ClmUserSecurityEntry{ + AccessType: wire.AccessType, + Href: item.Href, + Email: item.Email, + UserName: item.UserName, + FirstName: item.FirstName, + LastName: item.LastName, + Role: item.Role, + } + return nil +} + +// ClmChangeSecurityTaskRequest is the request body for POST .../changesecuritytasks — +// see PatchFolderSecurity's doc in clm_client.go. Confirmed via the CLM API Reference's +// interactive schema browser (its default collapsed view initially looked like a flat +// {Href, Security} pair sibling to Status — an earlier version of this struct assumed +// exactly that — but expanding "Folder" shows it's the *complete* Folder object schema, +// itself carrying its own nested Href and Security fields; the outer Href/Security +// alongside Status are generic Task-wrapper fields this doc-generation tool reuses +// across every Task type, not what ChangeSecurityTasks actually reads for a folder +// security change). The target folder and its new security both nest under Folder. +type ClmChangeSecurityTaskRequest struct { + Folder ClmChangeSecurityTaskFolder `json:"Folder"` } -// ClmFolderSecurityPatch is the request body for PATCH .../folders/{id} when updating -// folder security. -type ClmFolderSecurityPatch struct { +// ClmChangeSecurityTaskFolder is the minimal Folder reference ChangeSecurityTasks' +// POST needs: which folder (Href) and what to set its security to (Security) — see +// ClmChangeSecurityTaskRequest's doc. The real Folder object has many more fields +// (Name, ParentFolder, Path, etc.); none are required here, mirroring how ParentFolder +// references elsewhere in this API only ever need Href. +type ClmChangeSecurityTaskFolder struct { + Href string `json:"Href"` Security ClmFolderSecurityWrite `json:"Security"` } @@ -143,6 +391,16 @@ type ClmFolderSecurityPatch struct { // folder's complete current security (see clm_folders.go's clmFolderSecurityToWrite), // not just the one changed entry: Folders.Patch's merge-vs-replace semantics for // Security are undocumented, and sending the complete state is correct either way. +// +// Entries serialize via ClmGroupSecurityEntry/ClmUserSecurityEntry's MarshalJSON, so a +// PATCH sends the same {Item: {...}, AccessType} shape confirmed live on reads. This +// WAS tested live, against a disposable folder created and deleted solely for the +// check — and had no effect: see clm_client.go's package doc "Folders" section for the +// full evidence chain. Neither this shape nor a flat {Href, AccessType} one worked, +// with either PATCH or PUT; a distinct CLM error code ("136 - Missing Change Security +// Task") suggests the real mechanism is a dedicated Task API endpoint, not this generic +// object Patch. Grant/Revoke on clm_folder are NOT confirmed to work against a real CLM +// tenant — this is a known, open gap, not a residual unconfirmed assumption. type ClmFolderSecurityWrite struct { Groups []ClmGroupSecurityEntry `json:"Groups,omitempty"` Roles []ClmRoleSecurityEntry `json:"Roles,omitempty"` diff --git a/pkg/client/clm_models_test.go b/pkg/client/clm_models_test.go new file mode 100644 index 00000000..786291e9 --- /dev/null +++ b/pkg/client/clm_models_test.go @@ -0,0 +1,178 @@ +package client + +import ( + "encoding/json" + "strings" + "testing" +) + +// TestClmErrorResponse_Message is a regression test: a live 401 from a real CLM tenant +// returned {"Error":{"UserMessage":"Access Denied","DeveloperMessage":"Access +// Denied","ErrorCode":103,...}} — the previous top-level "Message" field assumption +// never matched this shape, so every real CLM error surfaced as "unknown CLM API +// error" regardless of what CLM actually reported. +func TestClmErrorResponse_Message(t *testing.T) { + t.Run("parses a real CLM error envelope", func(t *testing.T) { + body := `{"Error":{"HttpStatusCode":401,"UserMessage":"Access Denied","DeveloperMessage":"Access Denied","ErrorCode":103,"ReferenceId":"4c2e455a-9d5e-42c8-8328-cf25bfec684f"}}` + var e ClmErrorResponse + if err := json.Unmarshal([]byte(body), &e); err != nil { + t.Fatalf("Unmarshal: %v", err) + } + if want := "CLM API error 103: Access Denied"; e.Message() != want { + t.Errorf("Message() = %q, want %q", e.Message(), want) + } + }) + + t.Run("includes DeveloperMessage when it differs from UserMessage", func(t *testing.T) { + body := `{"Error":{"UserMessage":"Access Denied","DeveloperMessage":"token missing impersonation scope","ErrorCode":103}}` + var e ClmErrorResponse + if err := json.Unmarshal([]byte(body), &e); err != nil { + t.Fatalf("Unmarshal: %v", err) + } + if want := "CLM API error 103: Access Denied (token missing impersonation scope)"; e.Message() != want { + t.Errorf("Message() = %q, want %q", e.Message(), want) + } + }) + + t.Run("falls back to a generic message when the body is empty", func(t *testing.T) { + var e ClmErrorResponse + if want := "unknown CLM API error"; e.Message() != want { + t.Errorf("Message() = %q, want %q", e.Message(), want) + } + }) + + t.Run("uses DeveloperMessage as the primary text when UserMessage is empty", func(t *testing.T) { + body := `{"Error":{"DeveloperMessage":"token missing impersonation scope","ErrorCode":103}}` + var e ClmErrorResponse + if err := json.Unmarshal([]byte(body), &e); err != nil { + t.Fatalf("Unmarshal: %v", err) + } + if want := "CLM API error 103: token missing impersonation scope"; e.Message() != want { + t.Errorf("Message() = %q, want %q", e.Message(), want) + } + }) +} + +// TestClmUserSecurityEntry_UnmarshalJSON is a regression test for a review finding: +// ClmUserSecurityEntry's wire shape is unconfirmed live (unlike Groups), and the +// original UnmarshalJSON only handled the nested {Item:{...}} shape — a flat +// {Href,...} response would have decoded with Href == "" silently (a missing "Item" +// key leaves a struct-typed field at its zero value, not a decode error), and since +// clmFolderSecurityToWrite round-trips the complete security state on every +// Grant/Revoke, that would blank and then drop every other user's folder access on an +// unrelated write. +func TestClmUserSecurityEntry_UnmarshalJSON(t *testing.T) { + t.Run("nested Item shape (Groups' confirmed shape)", func(t *testing.T) { + body := `{"Item":{"Href":"https://clm.example.com/v2/acct/members/1","Email":"a@example.com"},"AccessType":"View"}` + var e ClmUserSecurityEntry + if err := json.Unmarshal([]byte(body), &e); err != nil { + t.Fatalf("Unmarshal: %v", err) + } + if e.Href != "https://clm.example.com/v2/acct/members/1" || e.AccessType != "View" { + t.Errorf("got %+v", e) + } + }) + + t.Run("flat shape falls back instead of leaving Href empty", func(t *testing.T) { + body := `{"Href":"https://clm.example.com/v2/acct/members/1","Email":"a@example.com","AccessType":"View"}` + var e ClmUserSecurityEntry + if err := json.Unmarshal([]byte(body), &e); err != nil { + t.Fatalf("Unmarshal: %v", err) + } + if e.Href != "https://clm.example.com/v2/acct/members/1" { + t.Errorf("expected the flat shape's Href to be picked up, got %+v", e) + } + if e.AccessType != "View" { + t.Errorf("got %+v", e) + } + }) + + for _, tt := range []struct { + name string + body string + }{ + {"Item is null", `{"Item":null,"AccessType":"View"}`}, + {"Item is an empty object", `{"Item":{},"AccessType":"View"}`}, + {"member nested under an unrecognized key", `{"Member":{"Href":"https://clm.example.com/v2/acct/members/1"},"AccessType":"View"}`}, + } { + t.Run("fails loud instead of silently blanking Href: "+tt.name, func(t *testing.T) { + var e ClmUserSecurityEntry + if err := json.Unmarshal([]byte(tt.body), &e); err == nil { + t.Fatalf("expected an error for an unrecognized wire shape, got %+v", e) + } + }) + } + + t.Run("error does not leak PII field values", func(t *testing.T) { + body := `{"Member":{"Href":"https://clm.example.com/v2/acct/members/1","Email":"secret@example.com","FirstName":"Alice","LastName":"Smith"},"AccessType":"View"}` + var e ClmUserSecurityEntry + err := json.Unmarshal([]byte(body), &e) + if err == nil { + t.Fatalf("expected an error for an unrecognized wire shape, got %+v", e) + } + for _, leaked := range []string{"secret@example.com", "Alice", "Smith"} { + if strings.Contains(err.Error(), leaked) { + t.Errorf("error message leaked PII field value %q: %v", leaked, err) + } + } + }) +} + +// TestClmGroupSecurityEntry_UnmarshalJSON mirrors ClmUserSecurityEntry's fail-loud +// guard and flat-shape fallback: Groups' nested shape is confirmed live today, but if +// that ever regresses, an empty-Href entry silently round-tripped into a +// PatchFolderSecurity body would drop that group's real folder access under replace +// semantics. +func TestClmGroupSecurityEntry_UnmarshalJSON(t *testing.T) { + t.Run("flat shape falls back instead of hard-failing", func(t *testing.T) { + body := `{"Href":"https://clm.example.com/v2/acct/groups/1","Name":"Legal","AccessType":"View"}` + var e ClmGroupSecurityEntry + if err := json.Unmarshal([]byte(body), &e); err != nil { + t.Fatalf("Unmarshal: %v", err) + } + if e.Href != "https://clm.example.com/v2/acct/groups/1" || e.Name != "Legal" { + t.Errorf("got %+v", e) + } + }) + + t.Run("fails loud on an unrecognized shape without leaking field values", func(t *testing.T) { + body := `{"Item":{},"AccessType":"View","Name":"Legal"}` + var e ClmGroupSecurityEntry + err := json.Unmarshal([]byte(body), &e) + if err == nil { + t.Fatalf("expected an error for an unrecognized wire shape, got %+v", e) + } + if strings.Contains(err.Error(), "Legal") { + t.Errorf("error message leaked a field value: %v", err) + } + }) +} + +// TestClmRoleSecurityEntry_UnmarshalJSON is a regression test for a review finding: +// Item was a bare Go string, confirmed live only as a flat string — if CLM ever nests +// Roles the way Groups turned out to be nested, json.Unmarshal would hard-error +// decoding a JSON object into a string field, failing every clm_folder read/Grant/ +// Revoke on that folder instead of just the one entry. +func TestClmRoleSecurityEntry_UnmarshalJSON(t *testing.T) { + t.Run("flat string shape (confirmed live)", func(t *testing.T) { + body := `{"AccessType":"View","Item":"FullSubscriber"}` + var e ClmRoleSecurityEntry + if err := json.Unmarshal([]byte(body), &e); err != nil { + t.Fatalf("Unmarshal: %v", err) + } + if e.Item != "FullSubscriber" || e.AccessType != "View" { + t.Errorf("got %+v", e) + } + }) + + t.Run("nested object shape doesn't hard-fail", func(t *testing.T) { + body := `{"AccessType":"View","Item":{"Name":"FullSubscriber","Href":"https://clm.example.com/v2/acct/roles/1"}}` + var e ClmRoleSecurityEntry + if err := json.Unmarshal([]byte(body), &e); err != nil { + t.Fatalf("Unmarshal: %v", err) + } + if e.Item != "FullSubscriber" || e.AccessType != "View" { + t.Errorf("got %+v", e) + } + }) +} diff --git a/pkg/client/clmtest/handlers.go b/pkg/client/clmtest/handlers.go index 935579b4..92b5e46f 100644 --- a/pkg/client/clmtest/handlers.go +++ b/pkg/client/clmtest/handlers.go @@ -2,37 +2,102 @@ package clmtest import ( "encoding/json" + "fmt" "io" "net/http" - "strings" + "strconv" "github.com/conductorone/baton-docusign/pkg/client" ) -// idFromHref extracts the trailing path segment of a Href — mirrors -// pkg/connector/helper.go's clmIDFromHref, reimplemented locally so this test package -// has no dependency on the connector package. +// idFromHref extracts the trailing path segment of a Href — see client.IDFromHref's doc. func idFromHref(href string) string { - href = strings.TrimSuffix(href, "/") - if idx := strings.LastIndex(href, "/"); idx != -1 { - return href[idx+1:] - } - return href + return client.IDFromHref(href) } -// Doc URL: https://developers.docusign.com/docs/clm-api/reference/objects/folders/ (Search). -func (s *Server) handleSearchFolders(w http.ResponseWriter, r *http.Request) { - s.mu.Lock() - defer s.mu.Unlock() - +// folderSearchResults builds the flat, paginated ClmFolderPage for a folder search — +// shared by the create/poll (wrapped in {Status, Href, Result}) and continuation +// (bare) responses. Search results are summaries; Security only comes via ?expand on +// Get, mirroring the real API. +func (s *Server) folderSearchResults(r *http.Request) client.ClmFolderPage { page, meta := pageSlice(r, s.folderOrder) items := make([]client.ClmFolder, 0, len(page)) for _, id := range page { f := *s.folders[id] - f.Security = client.ClmFolderSecurity{} // Search results are summaries; Security only comes via ?expand on Get + f.Security = client.ClmFolderSecurity{} items = append(items, f) } - writeJSON(w, client.ClmFolderPage{ClmPage: meta, Items: items}) + return client.ClmFolderPage{ClmPage: meta, Items: items} +} + +// Doc URL: https://developers.docusign.com/docs/clm-api/reference/tasks/foldersearchtasks/ +// (Post). Real CLM requires a recognized search parameter in the body (confirmed live: +// {"Title": ""} matches every folder) — this mock doesn't replicate that validation, +// since every real client call already sends the confirmed-working body. +func (s *Server) handleCreateFolderSearchTask(w http.ResponseWriter, r *http.Request) { + s.mu.Lock() + defer s.mu.Unlock() + + s.nextFolderSearchTaskID++ + taskID := strconv.Itoa(s.nextFolderSearchTaskID) + taskHref := fmt.Sprintf("%s/v2/%s/foldersearchtasks/%s", s.baseURL, AccountID, taskID) + if s.folderSearchTaskHrefOverride != "" { + taskHref = s.folderSearchTaskHrefOverride + } + + status := "Success" + if s.pendingFolderSearchPolls > 0 { + status = "Processing" + } + resp := client.ClmFolderSearchTaskResponse{Status: status, Href: taskHref} + if status == "Success" { + result := s.folderSearchResults(r) + if s.omitFolderSearchResultHref { + // Leave Href empty and force a "more pages remain" shape so SearchFolders' + // empty-Href continuation guard is reachable, regardless of what page size the + // caller requested. + if len(result.Items) > 1 { + result.Items = result.Items[:1] + } + result.Limit = 1 + result.Total = len(s.folderOrder) + result.Next = "more" + result.Offset = 0 + } else { + result.Href = taskHref + "/result" + } + resp.Result = &result + } + writeJSON(w, resp) +} + +// Doc URL: https://developers.docusign.com/docs/clm-api/reference/tasks/foldersearchtasks/get/ +func (s *Server) handlePollFolderSearchTask(w http.ResponseWriter, r *http.Request) { + s.mu.Lock() + defer s.mu.Unlock() + + taskID := r.PathValue("id") + taskHref := fmt.Sprintf("%s/v2/%s/foldersearchtasks/%s", s.baseURL, AccountID, taskID) + + if s.pendingFolderSearchPolls > 0 { + s.pendingFolderSearchPolls-- + writeJSON(w, client.ClmFolderSearchTaskResponse{Status: "Processing", Href: taskHref}) + return + } + + result := s.folderSearchResults(r) + if !s.omitFolderSearchResultHref { + result.Href = taskHref + "/result" + } + writeJSON(w, client.ClmFolderSearchTaskResponse{Status: "Success", Href: taskHref, Result: &result}) +} + +// Doc URL: https://developers.docusign.com/docs/clm-api/reference/tasks/foldersearchtasks/getsearchresult/ +func (s *Server) handleFolderSearchTaskResult(w http.ResponseWriter, r *http.Request) { + s.mu.Lock() + defer s.mu.Unlock() + + writeJSON(w, s.folderSearchResults(r)) } // Doc URL: https://developers.docusign.com/docs/clm-api/reference/objects/folders/get/ @@ -64,17 +129,13 @@ func (s *Server) handleGetFolder(w http.ResponseWriter, r *http.Request) { // because it's safe under replace semantics too. Modeling this endpoint as a merge // would hide a regression to sending just the one changed entry — replace surfaces it // immediately as other principals' entries disappearing. -func (s *Server) handlePatchFolder(w http.ResponseWriter, r *http.Request) { +// Doc URL: https://developers.docusign.com/docs/clm-api/reference/tasks/changesecuritytasks/post/ +// Simulates PatchFolderSecurity's real endpoint (ChangeSecurityTasks), not the generic +// Folders Patch this replaced — see clm_client.go's PatchFolderSecurity doc for why. +func (s *Server) handleCreateChangeSecurityTask(w http.ResponseWriter, r *http.Request) { s.mu.Lock() defer s.mu.Unlock() - id := r.PathValue("id") - f, ok := s.folders[id] - if !ok { - writeNotFound(w) - return - } - bodyBytes, err := io.ReadAll(r.Body) if err != nil { w.WriteHeader(http.StatusBadRequest) @@ -88,14 +149,16 @@ func (s *Server) handlePatchFolder(w http.ResponseWriter, r *http.Request) { // representation here is the only way this mock can actually catch a regression // to sending it, across all three of Groups/Roles/Users. var rawBody struct { - Security struct { - Groups []map[string]any `json:"Groups"` - Roles []map[string]any `json:"Roles"` - Users []map[string]any `json:"Users"` - } `json:"Security"` + Folder struct { + Security struct { + Groups []map[string]any `json:"Groups"` + Roles []map[string]any `json:"Roles"` + Users []map[string]any `json:"Users"` + } `json:"Security"` + } `json:"Folder"` } if json.Unmarshal(bodyBytes, &rawBody) == nil { - for _, entries := range [][]map[string]any{rawBody.Security.Groups, rawBody.Security.Roles, rawBody.Security.Users} { + for _, entries := range [][]map[string]any{rawBody.Folder.Security.Groups, rawBody.Folder.Security.Roles, rawBody.Folder.Security.Users} { for _, entry := range entries { if v, present := entry["AccessType"]; present { if str, ok := v.(string); ok && str == "" { @@ -108,19 +171,49 @@ func (s *Server) handlePatchFolder(w http.ResponseWriter, r *http.Request) { } } - var body client.ClmFolderSecurityPatch + var body client.ClmChangeSecurityTaskRequest if err := json.Unmarshal(bodyBytes, &body); err != nil { w.WriteHeader(http.StatusBadRequest) return } + folderID := idFromHref(body.Folder.Href) + f, ok := s.folders[folderID] + if !ok { + writeNotFound(w) + return + } + f.Security = client.ClmFolderSecurity{ - Groups: client.ClmGroupSecurityPage{Items: body.Security.Groups}, - Roles: client.ClmRoleSecurityPage{Items: body.Security.Roles}, - Users: client.ClmUserSecurityPage{Items: body.Security.Users}, + Groups: body.Folder.Security.Groups, + Roles: body.Folder.Security.Roles, + Users: body.Folder.Security.Users, + } + + s.nextChangeSecurityTaskID++ + taskHref := fmt.Sprintf("%s/v2/%s/changesecuritytasks/%d", s.baseURL, AccountID, s.nextChangeSecurityTaskID) + status := client.ClmChangeSecurityStatusSuccess + if s.pendingChangeSecurityPolls > 0 { + status = client.ClmChangeSecurityStatusWaiting + } + writeJSON(w, client.ClmChangeSecurityTaskResponse{Href: taskHref, Status: status}) +} + +// Doc URL: https://developers.docusign.com/docs/clm-api/reference/tasks/changesecuritytasks/get/ +func (s *Server) handlePollChangeSecurityTask(w http.ResponseWriter, r *http.Request) { + s.mu.Lock() + defer s.mu.Unlock() + + taskID := r.PathValue("id") + taskHref := fmt.Sprintf("%s/v2/%s/changesecuritytasks/%s", s.baseURL, AccountID, taskID) + + if s.pendingChangeSecurityPolls > 0 { + s.pendingChangeSecurityPolls-- + writeJSON(w, client.ClmChangeSecurityTaskResponse{Href: taskHref, Status: client.ClmChangeSecurityStatusWaiting}) + return } - writeJSON(w, *f) + writeJSON(w, client.ClmChangeSecurityTaskResponse{Href: taskHref, Status: client.ClmChangeSecurityStatusSuccess}) } // Doc URL: https://developers.docusign.com/docs/clm-api/reference/objects/groups/ @@ -209,13 +302,16 @@ func (s *Server) handlePatchMember(w http.ResponseWriter, r *http.Request) { return } + hrefs := make([]string, 0, len(body.Groups.Items)) for _, g := range body.Groups.Items { + hrefs = append(hrefs, g.Href) gid := idFromHref(g.Href) if !containsString(s.memberGroups[id], gid) { s.memberGroups[id] = append(s.memberGroups[id], gid) s.groupMembers[gid] = append(s.groupMembers[gid], id) } } + s.lastPatchedMemberGroupHrefs[id] = hrefs writeJSON(w, *m) } diff --git a/pkg/client/clmtest/seed.go b/pkg/client/clmtest/seed.go index cee585c9..b7ce863f 100644 --- a/pkg/client/clmtest/seed.go +++ b/pkg/client/clmtest/seed.go @@ -122,7 +122,7 @@ func seed(s *Server) { Name: "Contracts", Path: "/Contracts", Security: client.ClmFolderSecurity{ - Groups: client.ClmGroupSecurityPage{Items: []client.ClmGroupSecurityEntry{ + Groups: []client.ClmGroupSecurityEntry{ // Known tier granted to a group — tests slug-from-AccessType and // group-Href routing (should carry GrantExpandable at the connector // layer). @@ -130,15 +130,15 @@ func seed(s *Server) { // "Custom" — not one of the 5 grantable tiers — tests that Grants() // skips rather than guesses. {AccessType: client.ClmAccessTypeCustom, Href: s.GroupHref("group-finance")}, - }}, - Roles: client.ClmRoleSecurityPage{Items: []client.ClmRoleSecurityEntry{ + }, + Roles: []client.ClmRoleSecurityEntry{ // Role-granted entry — tests clm_role routing. {AccessType: client.ClmAccessTypeView, Item: roleFullSubscriberName}, - }}, - Users: client.ClmUserSecurityPage{Items: []client.ClmUserSecurityEntry{ + }, + Users: []client.ClmUserSecurityEntry{ // Known tier granted to a member — tests clm_member routing. {AccessType: client.ClmAccessTypeView, Href: s.MemberHref(memberBobID)}, - }}, + }, }, } contractsFolder.Href = s.FolderHref("folder-contracts") diff --git a/pkg/client/clmtest/server.go b/pkg/client/clmtest/server.go index e90f940b..9e4d41b1 100644 --- a/pkg/client/clmtest/server.go +++ b/pkg/client/clmtest/server.go @@ -27,9 +27,12 @@ // // GET /oauth/userinfo — eSignature account discovery (ensureInitialized) // GET /api/v2/{accountId}/account — CLM account discovery (ensureClmInitialized) -// POST /v2/{accountId}/folders/search — SearchFolders +// POST /v2/{accountId}/foldersearchtasks — SearchFolders (create task; resolves inline) +// GET /v2/{accountId}/foldersearchtasks/{id} — SearchFolders (poll a task — see PendingFolderSearchPolls) +// GET /v2/{accountId}/foldersearchtasks/{id}/result — SearchFolders (continuation pages) // GET /v2/{accountId}/folders/{id} — GetFolder (supports ?expand=Security) -// PATCH /v2/{accountId}/folders/{id} — PatchFolderSecurity +// POST /v2/{accountId}/changesecuritytasks — PatchFolderSecurity (create task; resolves inline) +// GET /v2/{accountId}/changesecuritytasks/{id} — PatchFolderSecurity (poll a task — see PendingChangeSecurityPolls) // GET /v2/{accountId}/groups — ListGroups // GET /v2/{accountId}/groups/{id}/groupmembers — GetGroupMembers // GET /v2/{accountId}/members — ListMembers @@ -113,6 +116,70 @@ type Server struct { permissionSetOrder []string memberGroupsRequests int // count of GET .../members/{id}/groups calls, for pagination assertions + + lastPatchedMemberGroupHrefs map[string][]string // memberID -> the raw Href strings the last PATCH request body carried, for tests + + nextFolderSearchTaskID int // incrementing counter for mock FolderSearchTasks task IDs + + // pendingFolderSearchPolls, when > 0, makes the next SearchFolders task created via + // POST /foldersearchtasks come back "Processing" this many times before resolving to + // "Success" on poll — see SetPendingFolderSearchPolls. Decremented on each poll. + pendingFolderSearchPolls int + + // omitFolderSearchResultHref, when true, leaves Result.Href empty on the next + // successful folder-search task response — see SetOmitFolderSearchResultHref. + omitFolderSearchResultHref bool + + // folderSearchTaskHrefOverride, when non-empty, replaces the Href on the next folder + // search task response — see SetFolderSearchTaskHrefOverride. + folderSearchTaskHrefOverride string + + nextChangeSecurityTaskID int // incrementing counter for mock ChangeSecurityTasks task IDs + + // pendingChangeSecurityPolls, when > 0, makes the next PatchFolderSecurity task + // created via POST /changesecuritytasks come back "waiting" this many times before + // resolving to "success" on poll — see SetPendingChangeSecurityPolls. Decremented on + // each poll. + pendingChangeSecurityPolls int +} + +// SetPendingFolderSearchPolls makes the next folder search task created by this server +// require n polls of GET .../foldersearchtasks/{id} before resolving to "Success" — +// exercises SearchFolders' awaitClmFolderSearchTask polling loop, which every live test +// against a real CLM tenant resolved past on the first try (inline in the POST +// response), leaving that branch otherwise untested. +func (s *Server) SetPendingFolderSearchPolls(n int) { + s.mu.Lock() + defer s.mu.Unlock() + s.pendingFolderSearchPolls = n +} + +// SetFolderSearchTaskHrefOverride makes the next folder search task response carry the +// given Href instead of this server's own — used to pin the client's host-validation +// guard (Client.validateClmURL) against a task Href pointing at an unexpected host. +func (s *Server) SetFolderSearchTaskHrefOverride(href string) { + s.mu.Lock() + defer s.mu.Unlock() + s.folderSearchTaskHrefOverride = href +} + +// SetOmitFolderSearchResultHref makes the next successful FolderSearchTasks response +// leave Result.Href empty. Used to pin SearchFolders' guard against minting a +// continuation token that would re-POST a new search (page-1 loop). +func (s *Server) SetOmitFolderSearchResultHref(omit bool) { + s.mu.Lock() + defer s.mu.Unlock() + s.omitFolderSearchResultHref = omit +} + +// SetPendingChangeSecurityPolls makes the next change-security task created by this +// server require n polls of GET .../changesecuritytasks/{id} before resolving to +// "success" — exercises PatchFolderSecurity's awaitClmChangeSecurityTask polling loop, +// unverified against a live tenant (see PatchFolderSecurity's doc in clm_client.go). +func (s *Server) SetPendingChangeSecurityPolls(n int) { + s.mu.Lock() + defer s.mu.Unlock() + s.pendingChangeSecurityPolls = n } // MemberGroupsRequestCount returns how many times GET .../members/{id}/groups has been @@ -125,6 +192,20 @@ func (s *Server) MemberGroupsRequestCount() int { return s.memberGroupsRequests } +// LastPatchedMemberGroupHrefs returns the raw Href strings the most recent PATCH +// .../members/{id} request body carried for memberID — unlike MemberGroups (which +// reduces everything to the trailing ID via idFromHref, the same as the real API's own +// comparison semantics), this exposes the exact Href the connector sent, so a test can +// tell a sample-derived Href from a base-URL-derived one even when the two mock helpers +// that build them happen to produce byte-identical strings. +func (s *Server) LastPatchedMemberGroupHrefs(memberID string) []string { + s.mu.Lock() + defer s.mu.Unlock() + out := make([]string, len(s.lastPatchedMemberGroupHrefs[memberID])) + copy(out, s.lastPatchedMemberGroupHrefs[memberID]) + return out +} + // URL returns the mock server's base URL — also what handleClmAccountDiscovery // returns as the CLM API base URL. func (s *Server) URL() string { return s.baseURL } @@ -139,6 +220,66 @@ func (s *Server) GroupHref(id string) string { return fmt.Sprintf("%s/v2/%s/groups/%s", s.baseURL, AccountID, id) } +// SetGroupHref overrides the Href a seeded group reports on GetMemberGroups, letting a +// test make it observably different from what GroupHref (and so client.GroupHref's +// fallback derivation, which builds the same shape from the discovered base URL) would +// produce — needed to distinguish a sample-derived Href from a fallback-derived one when +// both would otherwise be byte-identical. +func (s *Server) SetGroupHref(id, href string) { + s.mu.Lock() + defer s.mu.Unlock() + if g, ok := s.groups[id]; ok { + g.Href = href + return + } + if s.t != nil { + //nolint:gocritic // s.t is testing.TB; ruleguard only exempts concrete *testing.T/B/F, not the interface + s.t.Fatalf("SetGroupHref: no seeded group %q", id) + } +} + +// SetFolderGroupSecurityHref and SetFolderUserSecurityHref override the Href of an +// existing group/user security entry on a seeded folder (matched by ID via idFromHref), +// letting a test make it observably different from what client.GroupHref/MemberHref's +// fallback would produce for a different, derived ID — the same lever SetGroupHref gives +// GetMemberGroups-based tests, needed here because folder security entries carry no +// separate ID field of their own. +func (s *Server) SetFolderGroupSecurityHref(folderID, groupID, href string) { + s.mu.Lock() + defer s.mu.Unlock() + folder, ok := s.folders[folderID] + if ok { + for i := range folder.Security.Groups { + if idFromHref(folder.Security.Groups[i].Href) == groupID { + folder.Security.Groups[i].Href = href + return + } + } + } + if s.t != nil { + //nolint:gocritic // s.t is testing.TB; ruleguard only exempts concrete *testing.T/B/F, not the interface + s.t.Fatalf("SetFolderGroupSecurityHref: no group %q security entry on folder %q", groupID, folderID) + } +} + +func (s *Server) SetFolderUserSecurityHref(folderID, memberID, href string) { + s.mu.Lock() + defer s.mu.Unlock() + folder, ok := s.folders[folderID] + if ok { + for i := range folder.Security.Users { + if idFromHref(folder.Security.Users[i].Href) == memberID { + folder.Security.Users[i].Href = href + return + } + } + } + if s.t != nil { + //nolint:gocritic // s.t is testing.TB; ruleguard only exempts concrete *testing.T/B/F, not the interface + s.t.Fatalf("SetFolderUserSecurityHref: no member %q security entry on folder %q", memberID, folderID) + } +} + func (s *Server) MemberHref(id string) string { return fmt.Sprintf("%s/v2/%s/members/%s", s.baseURL, AccountID, id) } @@ -163,16 +304,16 @@ func (s *Server) FolderSecurity(folderID string) client.ClmFolderSecurity { if !ok { return client.ClmFolderSecurity{} } - groups := make([]client.ClmGroupSecurityEntry, len(f.Security.Groups.Items)) - copy(groups, f.Security.Groups.Items) - roles := make([]client.ClmRoleSecurityEntry, len(f.Security.Roles.Items)) - copy(roles, f.Security.Roles.Items) - users := make([]client.ClmUserSecurityEntry, len(f.Security.Users.Items)) - copy(users, f.Security.Users.Items) + groups := make([]client.ClmGroupSecurityEntry, len(f.Security.Groups)) + copy(groups, f.Security.Groups) + roles := make([]client.ClmRoleSecurityEntry, len(f.Security.Roles)) + copy(roles, f.Security.Roles) + users := make([]client.ClmUserSecurityEntry, len(f.Security.Users)) + copy(users, f.Security.Users) return client.ClmFolderSecurity{ - Groups: client.ClmGroupSecurityPage{Items: groups}, - Roles: client.ClmRoleSecurityPage{Items: roles}, - Users: client.ClmUserSecurityPage{Items: users}, + Groups: groups, + Roles: roles, + Users: users, } } @@ -180,12 +321,13 @@ func (s *Server) FolderSecurity(folderID string) client.ClmFolderSecurity { // RunStandalone so both construct exactly the same seeded state. func newState() *Server { return &Server{ - folders: make(map[string]*client.ClmFolder), - groups: make(map[string]*client.ClmGroup), - groupMembers: make(map[string][]string), - members: make(map[string]*client.ClmMember), - memberGroups: make(map[string][]string), - permissionSets: make(map[string]*client.ClmPermissionSet), + folders: make(map[string]*client.ClmFolder), + groups: make(map[string]*client.ClmGroup), + groupMembers: make(map[string][]string), + members: make(map[string]*client.ClmMember), + memberGroups: make(map[string][]string), + permissionSets: make(map[string]*client.ClmPermissionSet), + lastPatchedMemberGroupHrefs: make(map[string][]string), } } @@ -195,9 +337,12 @@ func newMux(s *Server) *http.ServeMux { mux := http.NewServeMux() mux.HandleFunc("GET /oauth/userinfo", s.handleUserInfo) mux.HandleFunc("GET /api/v2/{accountId}/account", s.requireAuth(s.handleClmAccountDiscovery)) - mux.HandleFunc("POST /v2/{accountId}/folders/search", s.requireAuth(s.handleSearchFolders)) + mux.HandleFunc("POST /v2/{accountId}/foldersearchtasks", s.requireAuth(s.handleCreateFolderSearchTask)) + mux.HandleFunc("GET /v2/{accountId}/foldersearchtasks/{id}", s.requireAuth(s.handlePollFolderSearchTask)) + mux.HandleFunc("GET /v2/{accountId}/foldersearchtasks/{id}/result", s.requireAuth(s.handleFolderSearchTaskResult)) mux.HandleFunc("GET /v2/{accountId}/folders/{id}", s.requireAuth(s.handleGetFolder)) - mux.HandleFunc("PATCH /v2/{accountId}/folders/{id}", s.requireAuth(s.handlePatchFolder)) + mux.HandleFunc("POST /v2/{accountId}/changesecuritytasks", s.requireAuth(s.handleCreateChangeSecurityTask)) + mux.HandleFunc("GET /v2/{accountId}/changesecuritytasks/{id}", s.requireAuth(s.handlePollChangeSecurityTask)) mux.HandleFunc("GET /v2/{accountId}/groups", s.requireAuth(s.handleListGroups)) mux.HandleFunc("GET /v2/{accountId}/groups/{id}/groupmembers", s.requireAuth(s.handleGroupMembers)) mux.HandleFunc("GET /v2/{accountId}/members", s.requireAuth(s.handleListMembers)) diff --git a/pkg/client/helper.go b/pkg/client/helper.go index 45286c93..4e24a45b 100644 --- a/pkg/client/helper.go +++ b/pkg/client/helper.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "net/url" + "strings" "github.com/conductorone/baton-sdk/pkg/annotations" "github.com/conductorone/baton-sdk/pkg/ratelimit" @@ -14,6 +15,19 @@ import ( const DefaultPageSize = 100 +// IDFromHref extracts the trailing path segment from a CLM object's Href — CLM's +// Object API schemas expose a Href field ("Uri where the object can be retrieved") but +// no separate opaque Id field, so this is the closest thing to a native ID CLM exposes. +// Shared by pkg/connector (reading real Hrefs) and pkg/client/clmtest (generating seed +// Hrefs for the mock server), which otherwise each maintained an identical copy. +func IDFromHref(href string) string { + href = strings.TrimSuffix(href, "/") + if idx := strings.LastIndex(href, "/"); idx != -1 { + return href[idx+1:] + } + return href +} + // BuildURL combines the base API URL with a formatted endpoint path. func buildURL(base, path string, params ...any) (*url.URL, error) { baseURL, err := url.Parse(base) diff --git a/pkg/client/oauth.go b/pkg/client/oauth.go index c792e151..01576516 100644 --- a/pkg/client/oauth.go +++ b/pkg/client/oauth.go @@ -17,21 +17,19 @@ var ( authURLProd = "https://account.docusign.com/oauth/auth" tokenURLProd = "https://account.docusign.com/oauth/token" //nolint:gosec // token URL does not contain sensitive credentials. defaultScope = "signature" - // clmScopes are additional OAuth scopes required to call the DocuSign CLM API. - // "spring_read"/"spring_write" are confirmed via the CLM Authentication Overview - // docs (combine with eSignature's "signature" scope on the same authorization - // request); "springcm_read"/"springcm_write" are ALSO requested defensively - // because a separate DocuSign docs page (for the account-discovery endpoint - // ensureClmInitialized calls, auth.springcm.com/api/v2/{accountId}/account) lists - // the required scope as "springcm_read" instead — possibly a docs typo, possibly - // a genuinely distinct scope given CLM/SpringCM's inconsistent legacy naming, and - // unconfirmed against a real CLM tenant either way. Requesting all 4 covers both - // possibilities at once: an unrecognized/unauthorized scope name is expected to be - // dropped by DocuSign's OAuth consent rather than rejecting the whole - // authorization (standard OAuth2 behavior), so this is a low-risk hedge, not a - // confirmed-safe one — the same "check every plausible candidate" pattern - // ensureClmInitialized uses for the base-URL response field name. - clmScopes = []string{"spring_read", "spring_write", "springcm_read", "springcm_write"} + // clmScopes are the OAuth scopes required to call the DocuSign CLM API, per + // DocuSign's own CLM Authentication Overview docs (pasted live, since the site is + // JS-rendered and unreachable by automated tools): the "Required scopes" section's + // Authorization Code Grant example is exactly signature+spring_read+spring_write, + // with an explicit note that "impersonation" is a JWT-Grant-only requirement — NOT + // needed here. An earlier version of this code added "impersonation" and + // "springcm_read"/"springcm_write" based on a live 401 ("Access Denied", CLM + // ErrorCode 103) against a real CLM tenant plus unreliable secondary docs sources; + // that live tenant already had exactly this scope set granted when it failed, so + // the 401 is not a scope problem — see clm_client.go's package doc for the current + // working theory (demo/UAT environment limitation, per CLM's docs requiring a + // production account). + clmScopes = []string{"spring_read", "spring_write"} ) // buildScopes returns the OAuth scopes to request, adding CLM scopes when includeClm is set. diff --git a/pkg/client/oauth_test.go b/pkg/client/oauth_test.go index e0a46ce1..4f2dee70 100644 --- a/pkg/client/oauth_test.go +++ b/pkg/client/oauth_test.go @@ -5,11 +5,12 @@ import ( "testing" ) -// TestBuildScopes_RequestsBothClmScopeNameVariants is a regression test: the CLM -// scope name is unconfirmed between "spring_read"/"spring_write" (the general CLM API -// docs) and "springcm_read"/"springcm_write" (the account-discovery endpoint's own -// docs) — see clmScopes' doc for why both are requested defensively. -func TestBuildScopes_RequestsBothClmScopeNameVariants(t *testing.T) { +// TestBuildScopes_MatchesDocuSignsAuthCodeGrantExample is a regression test: the scope +// list must match DocuSign's own CLM Authentication Overview docs' Authorization Code +// Grant example exactly (signature+spring_read+spring_write) — see clmScopes' doc for +// why "impersonation" (JWT-Grant-only) and "springcm_read"/"springcm_write" +// (unrecognized) were tried and removed. +func TestBuildScopes_MatchesDocuSignsAuthCodeGrantExample(t *testing.T) { t.Run("includeClm false requests only the base scope", func(t *testing.T) { got := buildScopes(false) want := []string{"signature"} @@ -18,9 +19,9 @@ func TestBuildScopes_RequestsBothClmScopeNameVariants(t *testing.T) { } }) - t.Run("includeClm true requests all 4 CLM scope name candidates", func(t *testing.T) { + t.Run("includeClm true requests exactly the documented CLM scopes", func(t *testing.T) { got := buildScopes(true) - want := []string{"signature", "spring_read", "spring_write", "springcm_read", "springcm_write"} + want := []string{"signature", "spring_read", "spring_write"} if !reflect.DeepEqual(got, want) { t.Errorf("buildScopes(true) = %v, want %v", got, want) } diff --git a/pkg/connector/clm_folders.go b/pkg/connector/clm_folders.go index 016c9791..1e39e10a 100644 --- a/pkg/connector/clm_folders.go +++ b/pkg/connector/clm_folders.go @@ -12,6 +12,8 @@ import ( rs "github.com/conductorone/baton-sdk/pkg/types/resource" "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" "go.uber.org/zap" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) var _ connectorbuilder.StaticEntitlementSyncerV2 = (*clmFolderBuilder)(nil) @@ -131,12 +133,12 @@ func (f *clmFolderBuilder) StaticEntitlements(_ context.Context, _ rs.SyncOpAttr func (f *clmFolderBuilder) Grants(ctx context.Context, folderResource *v2.Resource, _ rs.SyncOpAttrs) ([]*v2.Grant, *rs.SyncOpResults, error) { folder, annos, err := f.client.GetFolder(ctx, folderResource.Id.Resource, "Security") if err != nil { - return nil, nil, fmt.Errorf("getting security for CLM folder %s: %w", folderResource.Id.Resource, err) + return nil, nil, fmt.Errorf("baton-docusign: getting security for CLM folder %s: %w", folderResource.Id.Resource, err) } var grants []*v2.Grant - for _, entry := range folder.Security.Groups.Items { + for _, entry := range folder.Security.Groups { slug, ok := clmSlugForAccessType(entry.AccessType) if !ok { continue @@ -151,7 +153,7 @@ func (f *clmFolderBuilder) Grants(ctx context.Context, folderResource *v2.Resour grants = append(grants, grant.NewGrant(folderResource, slug, principalID, grantOpts...)) } - for _, entry := range folder.Security.Roles.Items { + for _, entry := range folder.Security.Roles { slug, ok := clmSlugForAccessType(entry.AccessType) if !ok { continue @@ -166,7 +168,7 @@ func (f *clmFolderBuilder) Grants(ctx context.Context, folderResource *v2.Resour grants = append(grants, grant.NewGrant(folderResource, slug, principalID)) } - for _, entry := range folder.Security.Users.Items { + for _, entry := range folder.Security.Users { slug, ok := clmSlugForAccessType(entry.AccessType) if !ok { continue @@ -188,21 +190,48 @@ func (f *clmFolderBuilder) Grants(ctx context.Context, folderResource *v2.Resour func (f *clmFolderBuilder) Grant(ctx context.Context, principal *v2.Resource, ent *v2.Entitlement) ([]*v2.Grant, annotations.Annotations, error) { accessType, ok := clmAccessTypeForSlug(ent.Slug) if !ok { - return nil, nil, fmt.Errorf("baton-docusign: unknown CLM folder entitlement slug %q", ent.Slug) + return nil, nil, status.Errorf(codes.InvalidArgument, "baton-docusign: unknown CLM folder entitlement slug %q", ent.Slug) } folderID := ent.Resource.Id.Resource + // Same guard as Revoke, and for the same reason: the group/member branches below are + // only incidentally covered by clmPreferredHref's own empty-id check, but the role + // branch compares principal.Id.Resource to Item by exact string with nothing else + // upstream to reject an empty value — this catches all three uniformly. + if principal.Id.Resource == "" { + return nil, nil, status.Errorf(codes.InvalidArgument, "baton-docusign: granting CLM folder security: principal missing native ID") + } + // folderID has the same theoretical gap: parseIntoClmFolderResource derives it via + // clmIDFromHref(folder.Href) with no non-empty check, so an empty Href would produce + // a folder resource with an empty ID. An empty folderID here would hit the + // collection-root path (buildClmClientURL's "/v2/%s/folders/%s" with an empty last + // segment) instead of failing clearly client-side. + if folderID == "" { + return nil, nil, status.Errorf(codes.InvalidArgument, "baton-docusign: granting CLM folder security: folder missing native ID") + } + folder, getAnnos, err := f.client.GetFolderFresh(ctx, folderID, "Security") if err != nil { - return nil, getAnnos, fmt.Errorf("getting security for CLM folder %s: %w", folderID, err) + return nil, getAnnos, fmt.Errorf("baton-docusign: getting security for CLM folder %s: %w", folderID, err) } write := clmFolderSecurityToWrite(folder.Security) switch principal.Id.ResourceType { case clmGroupResourceType.Id: - groupHref, err := clmGroupHrefFromResource(principal) + // Prefer a real, server-issued Href already on hand over one derived from the + // discovered CLM base URL — see clmPreferredHref's doc. The principal's own + // profile href (parseIntoClmGroupResource still populates it for display) comes + // first: it's this exact group's own recorded Href, not a sibling's to derive + // from, so it's the most direct sample available when the resource happens to + // carry one — only ever a sample, never required, so an identity-only principal + // (no profile at all) still falls through to the other-entries/fallback path + // unchanged. + groupSampleHrefs := clmSampleHrefsFrom(principal, write.Groups, func(e client.ClmGroupSecurityEntry) string { return e.Href }) + groupHref, err := clmPreferredHref(ctx, principal.Id.Resource, groupSampleHrefs, func() (string, error) { + return f.client.GroupHref(ctx, principal.Id.Resource) + }) if err != nil { - return nil, nil, err + return nil, getAnnos, fmt.Errorf("baton-docusign: resolving href for CLM group %s: %w", principal.Id.Resource, err) } if i := clmFindGroupSecurityIndex(write.Groups, groupHref); i >= 0 { if slug, ok := clmSlugForAccessType(write.Groups[i].AccessType); ok && slug == ent.Slug { @@ -223,9 +252,13 @@ func (f *clmFolderBuilder) Grant(ctx context.Context, principal *v2.Resource, en write.Roles = append(write.Roles, client.ClmRoleSecurityEntry{AccessType: accessType, Item: roleName}) } case clmMemberResourceType.Id: - memberHref, err := clmMemberHrefFromResource(principal) + // Same rationale as the group case above. + userSampleHrefs := clmSampleHrefsFrom(principal, write.Users, func(e client.ClmUserSecurityEntry) string { return e.Href }) + memberHref, err := clmPreferredHref(ctx, principal.Id.Resource, userSampleHrefs, func() (string, error) { + return f.client.MemberHref(ctx, principal.Id.Resource) + }) if err != nil { - return nil, nil, err + return nil, getAnnos, fmt.Errorf("baton-docusign: resolving href for CLM member %s: %w", principal.Id.Resource, err) } if i := clmFindUserSecurityIndex(write.Users, memberHref); i >= 0 { if slug, ok := clmSlugForAccessType(write.Users[i].AccessType); ok && slug == ent.Slug { @@ -236,12 +269,12 @@ func (f *clmFolderBuilder) Grant(ctx context.Context, principal *v2.Resource, en write.Users = append(write.Users, client.ClmUserSecurityEntry{AccessType: accessType, Href: memberHref}) } default: - return nil, nil, fmt.Errorf("baton-docusign: invalid principal type for CLM folder security: %s", principal.Id.ResourceType) + return nil, getAnnos, status.Errorf(codes.InvalidArgument, "baton-docusign: invalid principal type for CLM folder security: %s", principal.Id.ResourceType) } patchAnnos, err := f.client.PatchFolderSecurity(ctx, folderID, write) if err != nil { - return nil, patchAnnos, fmt.Errorf("granting CLM folder security: %w", err) + return nil, patchAnnos, fmt.Errorf("baton-docusign: granting CLM folder security: %w", err) } return nil, patchAnnos, nil @@ -252,27 +285,34 @@ func (f *clmFolderBuilder) Grant(ctx context.Context, principal *v2.Resource, en // back to PatchFolderSecurity, taking a defensive copy of each collection so callers // can mutate the result without aliasing the original read. func clmFolderSecurityToWrite(sec client.ClmFolderSecurity) client.ClmFolderSecurityWrite { - groups := make([]client.ClmGroupSecurityEntry, len(sec.Groups.Items)) - copy(groups, sec.Groups.Items) - roles := make([]client.ClmRoleSecurityEntry, len(sec.Roles.Items)) - copy(roles, sec.Roles.Items) - users := make([]client.ClmUserSecurityEntry, len(sec.Users.Items)) - copy(users, sec.Users.Items) + groups := make([]client.ClmGroupSecurityEntry, len(sec.Groups)) + copy(groups, sec.Groups) + roles := make([]client.ClmRoleSecurityEntry, len(sec.Roles)) + copy(roles, sec.Roles) + users := make([]client.ClmUserSecurityEntry, len(sec.Users)) + copy(users, sec.Users) return client.ClmFolderSecurityWrite{Groups: groups, Roles: roles, Users: users} } -// clmFindGroupSecurityIndex returns the index of entries whose Href identifies -// groupHref (compared via clmIDFromHref, since the read-side Href shape isn't -// guaranteed to match exactly — see clmGroupHrefFromResource), or -1 if not found. -func clmFindGroupSecurityIndex(entries []client.ClmGroupSecurityEntry, groupHref string) int { +// clmFindSecurityIndexByHref returns the index of the entry whose Href identifies +// targetHref (compared via clmIDFromHref, since the read-side Href shape isn't +// guaranteed to match exactly — see client.GroupHref/MemberHref), or -1 if not found. +// Shared by clmFindGroupSecurityIndex and clmFindUserSecurityIndex, which differ only in +// entry type. +func clmFindSecurityIndexByHref[T any](entries []T, hrefOf func(T) string, targetHref string) int { + targetID := clmIDFromHref(targetHref) for i, e := range entries { - if clmIDFromHref(e.Href) == clmIDFromHref(groupHref) { + if clmIDFromHref(hrefOf(e)) == targetID { return i } } return -1 } +func clmFindGroupSecurityIndex(entries []client.ClmGroupSecurityEntry, groupHref string) int { + return clmFindSecurityIndexByHref(entries, func(e client.ClmGroupSecurityEntry) string { return e.Href }, groupHref) +} + // clmFindRoleSecurityIndex returns the index of the entry for roleName, or -1 if not // found. Roles are compared by exact name, not clmIDFromHref — a role's Item is // already the bare name, never a Href. @@ -285,15 +325,8 @@ func clmFindRoleSecurityIndex(entries []client.ClmRoleSecurityEntry, roleName st return -1 } -// clmFindUserSecurityIndex returns the index of the entry whose Href identifies -// memberHref (see clmFindGroupSecurityIndex's identical rationale), or -1 if not found. func clmFindUserSecurityIndex(entries []client.ClmUserSecurityEntry, memberHref string) int { - for i, e := range entries { - if clmIDFromHref(e.Href) == clmIDFromHref(memberHref) { - return i - } - } - return -1 + return clmFindSecurityIndexByHref(entries, func(e client.ClmUserSecurityEntry) string { return e.Href }, memberHref) } // Revoke sets the principal's folder-security entry to NoAccess (not removed — same @@ -303,19 +336,33 @@ func (f *clmFolderBuilder) Revoke(ctx context.Context, grantObj *v2.Grant) (anno folderID := grantObj.Entitlement.Resource.Id.Resource principal := grantObj.Principal + // Guards all three branches below in one place: clmFindGroupSecurityIndex and + // clmFindUserSecurityIndex reduce an empty ID to "" via clmIDFromHref, which would + // match any security entry whose Href is empty or ends in a trailing slash; the role + // branch compares principal.Id.Resource to Item by exact string, so an empty ID would + // just as wrongly match an entry with an empty Item. Grant carries the identical + // guard for the identical reason — keep the two in sync. + if principal.Id.Resource == "" { + return nil, status.Errorf(codes.InvalidArgument, "baton-docusign: revoking CLM folder security: principal missing native ID") + } + // Same reasoning as Grant's identical guard: an empty folderID would hit the CLM + // API's folders collection root instead of failing clearly client-side. + if folderID == "" { + return nil, status.Errorf(codes.InvalidArgument, "baton-docusign: revoking CLM folder security: folder missing native ID") + } + folder, getAnnos, err := f.client.GetFolderFresh(ctx, folderID, "Security") if err != nil { - return getAnnos, fmt.Errorf("getting security for CLM folder %s: %w", folderID, err) + return getAnnos, fmt.Errorf("baton-docusign: getting security for CLM folder %s: %w", folderID, err) } write := clmFolderSecurityToWrite(folder.Security) switch principal.Id.ResourceType { case clmGroupResourceType.Id: - groupHref, err := clmGroupHrefFromResource(principal) - if err != nil { - return nil, err - } - i := clmFindGroupSecurityIndex(write.Groups, groupHref) + // No need to build a real Href via client.GroupHref here: clmFindGroupSecurityIndex + // only ever compares by clmIDFromHref, so principal.Id.Resource (already a bare ID) + // works directly and this skips a needless ensureClmReady round trip. + i := clmFindGroupSecurityIndex(write.Groups, principal.Id.Resource) if i < 0 || write.Groups[i].AccessType == client.ClmAccessTypeNoAccess { return annotations.New(&v2.GrantAlreadyRevoked{}), nil } @@ -328,22 +375,20 @@ func (f *clmFolderBuilder) Revoke(ctx context.Context, grantObj *v2.Grant) (anno } write.Roles[i].AccessType = client.ClmAccessTypeNoAccess case clmMemberResourceType.Id: - memberHref, err := clmMemberHrefFromResource(principal) - if err != nil { - return nil, err - } - i := clmFindUserSecurityIndex(write.Users, memberHref) + // Same reasoning as the group case above: clmFindUserSecurityIndex only compares + // by clmIDFromHref, so no need to build a real Href via client.MemberHref. + i := clmFindUserSecurityIndex(write.Users, principal.Id.Resource) if i < 0 || write.Users[i].AccessType == client.ClmAccessTypeNoAccess { return annotations.New(&v2.GrantAlreadyRevoked{}), nil } write.Users[i].AccessType = client.ClmAccessTypeNoAccess default: - return nil, fmt.Errorf("baton-docusign: invalid principal type for CLM folder security: %s", principal.Id.ResourceType) + return getAnnos, status.Errorf(codes.InvalidArgument, "baton-docusign: invalid principal type for CLM folder security: %s", principal.Id.ResourceType) } patchAnnos, err := f.client.PatchFolderSecurity(ctx, folderID, write) if err != nil { - return patchAnnos, fmt.Errorf("revoking CLM folder security: %w", err) + return patchAnnos, fmt.Errorf("baton-docusign: revoking CLM folder security: %w", err) } return patchAnnos, nil @@ -401,14 +446,3 @@ func clmIsKnownRole(name string) bool { } return false } - -// clmMemberHrefFromResource reads back the Href stashed in a CLM member resource's -// profile (see parseIntoClmMemberResource) — needed to reference the member in a -// folder-security grant body. -func clmMemberHrefFromResource(principal *v2.Resource) (string, error) { - href, ok := rs.GetProfileStringValue(rs.GetProfile(principal), "href") - if !ok || href == "" { - return "", fmt.Errorf("baton-docusign: CLM member resource %s is missing its href profile field", principal.Id.Resource) - } - return href, nil -} diff --git a/pkg/connector/clm_folders_test.go b/pkg/connector/clm_folders_test.go index 600b410b..084196ce 100644 --- a/pkg/connector/clm_folders_test.go +++ b/pkg/connector/clm_folders_test.go @@ -2,6 +2,7 @@ package connector import ( "context" + "fmt" "testing" "github.com/conductorone/baton-docusign/pkg/client" @@ -229,7 +230,7 @@ func TestClmFolderBuilder_GrantAndRevoke_Idempotent(t *testing.T) { } else if hasAlreadyExists(annos) { t.Error("first Grant should not report GrantAlreadyExists") } - groups := srv.FolderSecurity("folder-templates").Groups.Items + groups := srv.FolderSecurity("folder-templates").Groups if len(groups) != 1 || groups[0].AccessType != client.ClmAccessTypeViewEdit { t.Fatalf("expected one ViewEdit group entry after Grant, got %+v", groups) } @@ -240,7 +241,7 @@ func TestClmFolderBuilder_GrantAndRevoke_Idempotent(t *testing.T) { } else if !hasAlreadyExists(annos) { t.Error("repeat Grant should report GrantAlreadyExists") } - if groups := srv.FolderSecurity("folder-templates").Groups.Items; len(groups) != 1 { + if groups := srv.FolderSecurity("folder-templates").Groups; len(groups) != 1 { t.Fatalf("expected still exactly one entry after a repeat Grant, got %d", len(groups)) } @@ -251,7 +252,7 @@ func TestClmFolderBuilder_GrantAndRevoke_Idempotent(t *testing.T) { } else if hasAlreadyRevoked(annos) { t.Error("first Revoke should not report GrantAlreadyRevoked") } - groups = srv.FolderSecurity("folder-templates").Groups.Items + groups = srv.FolderSecurity("folder-templates").Groups if len(groups) != 1 || groups[0].AccessType != client.ClmAccessTypeNoAccess { t.Fatalf("expected the entry's AccessType to become NoAccess after Revoke, got %+v", groups) } @@ -280,7 +281,7 @@ func TestClmFolderBuilder_GrantAndRevoke_PreservesOtherPrincipals(t *testing.T) ctx := context.Background() before := srv.FolderSecurity("folder-contracts") - if total := len(before.Groups.Items) + len(before.Roles.Items) + len(before.Users.Items); total != 4 { + if total := len(before.Groups) + len(before.Roles) + len(before.Users); total != 4 { t.Fatalf("expected folder-contracts seeded with 4 entries total, got %d: %+v", total, before) } @@ -301,12 +302,12 @@ func TestClmFolderBuilder_GrantAndRevoke_PreservesOtherPrincipals(t *testing.T) } afterGrant := srv.FolderSecurity("folder-contracts") - if len(afterGrant.Groups.Items) != 3 { // group-legal, group-finance, + the new group-ops - t.Fatalf("expected 3 group entries after granting a new group principal, got %d: %+v", len(afterGrant.Groups.Items), afterGrant.Groups.Items) + if len(afterGrant.Groups) != 3 { // group-legal, group-finance, + the new group-ops + t.Fatalf("expected 3 group entries after granting a new group principal, got %d: %+v", len(afterGrant.Groups), afterGrant.Groups) } - assertGroupsPreserved(t, before.Groups.Items, afterGrant.Groups.Items, "Grant") - assertRolesPreserved(t, before.Roles.Items, afterGrant.Roles.Items, "Grant") - assertUsersPreserved(t, before.Users.Items, afterGrant.Users.Items, "Grant") + assertGroupsPreserved(t, before.Groups, afterGrant.Groups, "Grant") + assertRolesPreserved(t, before.Roles, afterGrant.Roles, "Grant") + assertUsersPreserved(t, before.Users, afterGrant.Users, "Grant") grantObj := &v2.Grant{Principal: groupResource, Entitlement: ent} if annos, err := b.Revoke(ctx, grantObj); err != nil { @@ -316,12 +317,12 @@ func TestClmFolderBuilder_GrantAndRevoke_PreservesOtherPrincipals(t *testing.T) } afterRevoke := srv.FolderSecurity("folder-contracts") - if len(afterRevoke.Groups.Items) != 3 { // still 3 (NoAccess, not removed) - t.Fatalf("expected still 3 group entries after Revoke (NoAccess, not removed), got %d: %+v", len(afterRevoke.Groups.Items), afterRevoke.Groups.Items) + if len(afterRevoke.Groups) != 3 { // still 3 (NoAccess, not removed) + t.Fatalf("expected still 3 group entries after Revoke (NoAccess, not removed), got %d: %+v", len(afterRevoke.Groups), afterRevoke.Groups) } - assertGroupsPreserved(t, before.Groups.Items, afterRevoke.Groups.Items, "Revoke") - assertRolesPreserved(t, before.Roles.Items, afterRevoke.Roles.Items, "Revoke") - assertUsersPreserved(t, before.Users.Items, afterRevoke.Users.Items, "Revoke") + assertGroupsPreserved(t, before.Groups, afterRevoke.Groups, "Revoke") + assertRolesPreserved(t, before.Roles, afterRevoke.Roles, "Revoke") + assertUsersPreserved(t, before.Users, afterRevoke.Users, "Revoke") } func assertGroupsPreserved(t *testing.T, before, after []client.ClmGroupSecurityEntry, when string) { @@ -368,7 +369,7 @@ func assertUsersPreserved(t *testing.T, before, after []client.ClmUserSecurityEn // TestClmFolderBuilder_GrantAndRevoke_ToleratesBareIDOnRead is a regression test: the // CLM API's read-side Href representation isn't confirmed to always match the exact -// Href shape clmGroupHrefFromResource constructs on the write side. Grant/Revoke's +// Href shape client.GroupHref constructs on the write side. Grant/Revoke's // existence checks compare via clmIDFromHref (not raw equality) specifically so a bare // ID and a full Href ending in that ID are still treated as the same principal — if // the API ever returns Href as a bare ID, a raw-equality comparison would never match, @@ -382,7 +383,7 @@ func TestClmFolderBuilder_GrantAndRevoke_ToleratesBareIDOnRead(t *testing.T) { ctx := context.Background() // Seed folder-templates' group Security entry with a bare group ID, not the full - // Href clmGroupHrefFromResource would construct. + // Href client.GroupHref would construct. if _, err := c.PatchFolderSecurity(ctx, "folder-templates", client.ClmFolderSecurityWrite{ Groups: []client.ClmGroupSecurityEntry{{AccessType: client.ClmAccessTypeViewEdit, Href: "group-ops"}}, }); err != nil { @@ -405,7 +406,7 @@ func TestClmFolderBuilder_GrantAndRevoke_ToleratesBareIDOnRead(t *testing.T) { } else if !hasAlreadyExists(annos) { t.Error("Grant should recognize a bare-ID entry.Href as already granted, not duplicate it") } - if groups := srv.FolderSecurity("folder-templates").Groups.Items; len(groups) != 1 { + if groups := srv.FolderSecurity("folder-templates").Groups; len(groups) != 1 { t.Fatalf("expected still exactly one entry, got %d: %+v", len(groups), groups) } @@ -416,12 +417,243 @@ func TestClmFolderBuilder_GrantAndRevoke_ToleratesBareIDOnRead(t *testing.T) { } else if hasAlreadyRevoked(annos) { t.Fatal("Revoke incorrectly reported GrantAlreadyRevoked for a bare-ID entry.Href — access was left in place instead of being revoked") } - groups := srv.FolderSecurity("folder-templates").Groups.Items + groups := srv.FolderSecurity("folder-templates").Groups if len(groups) != 1 || groups[0].AccessType != client.ClmAccessTypeNoAccess { t.Fatalf("expected the entry's AccessType to become NoAccess after Revoke, got %+v", groups) } } +// clmIdentityOnlyResource builds a Resource with nothing but Id — the shape pebble's +// V3EntitlementToV2 hands Grant/Revoke on every read. +func clmIdentityOnlyResource(resourceType *v2.ResourceType, resourceID string) *v2.Resource { + return &v2.Resource{ + Id: &v2.ResourceId{ResourceType: resourceType.Id, Resource: resourceID}, + } +} + +// TestClmFolderBuilder_GrantAndRevoke_SurvivesIdentityOnlyPrincipal is a regression +// test: passes an identity-only principal (unlike every other test in this file, which +// uses a fully-populated resource) to confirm Href is still derivable. +func TestClmFolderBuilder_GrantAndRevoke_SurvivesIdentityOnlyPrincipal(t *testing.T) { + srv, c := clmtest.NewServer(t) + b := newClmFolderBuilder(c) + ctx := context.Background() + + folderResource, err := rs.NewResource("Templates", clmFolderResourceType, "folder-templates") + if err != nil { + t.Fatalf("NewResource: %v", err) + } + + t.Run("clm_group principal", func(t *testing.T) { + principal := clmIdentityOnlyResource(clmGroupResourceType, "group-ops") + ent := &v2.Entitlement{Slug: "view", Resource: folderResource} + + if _, _, err := b.Grant(ctx, principal, ent); err != nil { + t.Fatalf("Grant with an identity-only principal: %v", err) + } + groups := srv.FolderSecurity("folder-templates").Groups + if len(groups) != 1 || groups[0].AccessType != client.ClmAccessTypeView { + t.Fatalf("expected one View group entry after Grant, got %+v", groups) + } + // folder-templates starts with no group entries (clmPreferredHref's fallback + // branch), so this is the specific Href client.GroupHref must have derived. + if want := srv.GroupHref("group-ops"); groups[0].Href != want { + t.Errorf("expected Grant to write Href %q, got %q", want, groups[0].Href) + } + + grantObj := &v2.Grant{Principal: principal, Entitlement: ent} + annos, err := b.Revoke(ctx, grantObj) + if err != nil { + t.Fatalf("Revoke with an identity-only principal: %v", err) + } + // If the derived Href ever stopped matching the entry Grant wrote, + // clmFindGroupSecurityIndex would return -1 and Revoke would report + // GrantAlreadyRevoked with a nil error instead of actually revoking — asserting + // only err == nil would still pass while access was silently left in place. + if hasAlreadyRevoked(annos) { + t.Fatal("Revoke incorrectly reported GrantAlreadyRevoked — the derived Href didn't match the entry Grant wrote") + } + groups = srv.FolderSecurity("folder-templates").Groups + if len(groups) != 1 || groups[0].AccessType != client.ClmAccessTypeNoAccess { + t.Fatalf("expected the entry's AccessType to become NoAccess after Revoke, got %+v", groups) + } + }) + + t.Run("clm_member principal", func(t *testing.T) { + principal := clmIdentityOnlyResource(clmMemberResourceType, "member-dave") + ent := &v2.Entitlement{Slug: "view", Resource: folderResource} + + if _, _, err := b.Grant(ctx, principal, ent); err != nil { + t.Fatalf("Grant with an identity-only principal: %v", err) + } + users := srv.FolderSecurity("folder-templates").Users + if len(users) != 1 || users[0].AccessType != client.ClmAccessTypeView { + t.Fatalf("expected one View user entry after Grant, got %+v", users) + } + // folder-templates starts with no user entries (clmPreferredHref's fallback + // branch), so this is the specific Href client.MemberHref must have derived. + if want := srv.MemberHref("member-dave"); users[0].Href != want { + t.Errorf("expected Grant to write Href %q, got %q", want, users[0].Href) + } + + grantObj := &v2.Grant{Principal: principal, Entitlement: ent} + annos, err := b.Revoke(ctx, grantObj) + if err != nil { + t.Fatalf("Revoke with an identity-only principal: %v", err) + } + // Same rationale as the clm_group subtest above. + if hasAlreadyRevoked(annos) { + t.Fatal("Revoke incorrectly reported GrantAlreadyRevoked — the derived Href didn't match the entry Grant wrote") + } + users = srv.FolderSecurity("folder-templates").Users + if len(users) != 1 || users[0].AccessType != client.ClmAccessTypeNoAccess { + t.Fatalf("expected the entry's AccessType to become NoAccess after Revoke, got %+v", users) + } + }) +} + +// TestClmFolderBuilder_GrantAndRevoke_RejectEmptyPrincipalID is a regression test for +// both bot-flagged findings on this file: an empty principal.Id.Resource must be +// rejected before Grant or Revoke touch write.Groups/Roles/Users, for all three +// principal kinds — the role branch has no other guard (roleName is compared/written +// directly), and the group/member branches' own guard (inside clmPreferredHref, or +// clmIDFromHref's reduction of "" to "") must not be bypassed by this earlier check +// firing instead of it. +func TestClmFolderBuilder_GrantAndRevoke_RejectEmptyPrincipalID(t *testing.T) { + _, c := clmtest.NewServer(t) + b := newClmFolderBuilder(c) + ctx := context.Background() + + folderResource, err := rs.NewResource("Templates", clmFolderResourceType, "folder-templates") + if err != nil { + t.Fatalf("NewResource: %v", err) + } + ent := &v2.Entitlement{Slug: "view", Resource: folderResource} + + for _, resourceType := range []*v2.ResourceType{clmGroupResourceType, clmRoleResourceType, clmMemberResourceType} { + t.Run(resourceType.Id, func(t *testing.T) { + principal := clmIdentityOnlyResource(resourceType, "") + + if _, _, err := b.Grant(ctx, principal, ent); err == nil { + t.Error("expected Grant to reject an empty principal ID, got nil error") + } + + grantObj := &v2.Grant{Principal: principal, Entitlement: ent} + if _, err := b.Revoke(ctx, grantObj); err == nil { + t.Error("expected Revoke to reject an empty principal ID, got nil error") + } + }) + } +} + +// TestClmFolderBuilder_GrantAndRevoke_RejectEmptyFolderID is a regression test for the +// same class of bug as the principal-ID guard above, applied to the folder itself: +// parseIntoClmFolderResource derives a clm_folder's ID via clmIDFromHref(folder.Href) +// with no non-empty check, so an empty folderID must be rejected before it reaches +// GetFolderFresh/PatchFolderSecurity — otherwise it would build a URL hitting the +// folders collection root instead of failing clearly client-side. +func TestClmFolderBuilder_GrantAndRevoke_RejectEmptyFolderID(t *testing.T) { + _, c := clmtest.NewServer(t) + b := newClmFolderBuilder(c) + ctx := context.Background() + + principal := clmIdentityOnlyResource(clmMemberResourceType, "member-dave") + emptyFolder := clmIdentityOnlyResource(clmFolderResourceType, "") + ent := &v2.Entitlement{Slug: "view", Resource: emptyFolder} + + if _, _, err := b.Grant(ctx, principal, ent); err == nil { + t.Error("expected Grant to reject an empty folder ID, got nil error") + } + + grantObj := &v2.Grant{Principal: principal, Entitlement: ent} + if _, err := b.Revoke(ctx, grantObj); err == nil { + t.Error("expected Revoke to reject an empty folder ID, got nil error") + } +} + +// TestClmFolderBuilder_Grant_SurvivesIdentityOnlyPrincipal_SampleBranch covers the +// clmPreferredHref branch TestClmFolderBuilder_GrantAndRevoke_SurvivesIdentityOnlyPrincipal +// doesn't: folder-templates always starts with no security entries, so every Grant +// there hits clmPreferredHref's fallback (client.GroupHref/MemberHref) — the newer, +// riskier sample branch (deriving a written Href from an existing folder-security +// entry) never runs. folder-contracts is already seeded with real group/user security +// entries (clmtest/seed.go), so granting a DIFFERENT group/member there exercises the +// sample branch. Both existing samples are moved onto an alternate host first — +// otherwise the sample-derived Href and the fallback-derived one (both built from the +// same base URL and ID shape) would be byte-identical, and this test would pass whether +// or not the sample branch actually ran. +func TestClmFolderBuilder_Grant_SurvivesIdentityOnlyPrincipal_SampleBranch(t *testing.T) { + srv, c := clmtest.NewServer(t) + b := newClmFolderBuilder(c) + ctx := context.Background() + + const sampleHost = "https://other.example.com" + altGroupLegalHref := fmt.Sprintf("%s/v2/%s/groups/group-legal", sampleHost, clmtest.AccountID) + srv.SetFolderGroupSecurityHref("folder-contracts", "group-legal", altGroupLegalHref) + altGroupFinanceHref := fmt.Sprintf("%s/v2/%s/groups/group-finance", sampleHost, clmtest.AccountID) + srv.SetFolderGroupSecurityHref("folder-contracts", "group-finance", altGroupFinanceHref) + altMemberBobHref := fmt.Sprintf("%s/v2/%s/members/%s", sampleHost, clmtest.AccountID, "member-bob") + srv.SetFolderUserSecurityHref("folder-contracts", "member-bob", altMemberBobHref) + + folderResource, err := rs.NewResource("Contracts", clmFolderResourceType, "folder-contracts") + if err != nil { + t.Fatalf("NewResource: %v", err) + } + + t.Run("clm_group principal", func(t *testing.T) { + // group-ops is not among folder-contracts' existing entries (group-legal, + // group-finance), so clmPreferredHref must derive group-ops' Href from one of + // those samples via clmHrefWithID, not just echo a pre-existing entry. + principal := clmIdentityOnlyResource(clmGroupResourceType, "group-ops") + ent := &v2.Entitlement{Slug: "view", Resource: folderResource} + + if _, _, err := b.Grant(ctx, principal, ent); err != nil { + t.Fatalf("Grant with an identity-only principal: %v", err) + } + groups := srv.FolderSecurity("folder-contracts").Groups + wantHref := fmt.Sprintf("%s/v2/%s/groups/group-ops", sampleHost, clmtest.AccountID) + var found *client.ClmGroupSecurityEntry + for i := range groups { + if groups[i].Href == wantHref { + found = &groups[i] + } + } + if found == nil { + t.Fatalf("expected a group-ops entry with the sample-derived Href %q, got %+v", wantHref, groups) + return + } + if found.AccessType != client.ClmAccessTypeView { + t.Errorf("expected View AccessType, got %q", found.AccessType) + } + }) + + t.Run("clm_member principal", func(t *testing.T) { + // member-dave is not folder-contracts' existing member entry (member-bob), so + // clmPreferredHref must derive member-dave's Href from that sample. + principal := clmIdentityOnlyResource(clmMemberResourceType, "member-dave") + ent := &v2.Entitlement{Slug: "view", Resource: folderResource} + + if _, _, err := b.Grant(ctx, principal, ent); err != nil { + t.Fatalf("Grant with an identity-only principal: %v", err) + } + users := srv.FolderSecurity("folder-contracts").Users + wantHref := fmt.Sprintf("%s/v2/%s/members/member-dave", sampleHost, clmtest.AccountID) + var found *client.ClmUserSecurityEntry + for i := range users { + if users[i].Href == wantHref { + found = &users[i] + } + } + if found == nil { + t.Fatalf("expected a member-dave entry with the sample-derived Href %q, got %+v", wantHref, users) + return + } + if found.AccessType != client.ClmAccessTypeView { + t.Errorf("expected View AccessType, got %q", found.AccessType) + } + }) +} + func hasAlreadyExists(annos annotations.Annotations) bool { return annos.Contains(&v2.GrantAlreadyExists{}) } diff --git a/pkg/connector/clm_groups.go b/pkg/connector/clm_groups.go index 271f0dee..318e0302 100644 --- a/pkg/connector/clm_groups.go +++ b/pkg/connector/clm_groups.go @@ -12,6 +12,8 @@ import ( rs "github.com/conductorone/baton-sdk/pkg/types/resource" "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" "go.uber.org/zap" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) var _ connectorbuilder.StaticEntitlementSyncerV2 = (*clmGroupBuilder)(nil) @@ -110,7 +112,7 @@ func (g *clmGroupBuilder) Grants(ctx context.Context, groupResource *v2.Resource PageToken: pageToken, }) if err != nil { - return nil, nil, fmt.Errorf("getting members for CLM group %s: %w", groupResource.Id.Resource, err) + return nil, nil, fmt.Errorf("baton-docusign: getting members for CLM group %s: %w", groupResource.Id.Resource, err) } grants := make([]*v2.Grant, 0, len(members)) @@ -145,19 +147,28 @@ func (g *clmGroupBuilder) Grants(ctx context.Context, groupResource *v2.Resource // group appended (additive per the confirmed Members.Patch semantics). func (g *clmGroupBuilder) Grant(ctx context.Context, principal *v2.Resource, ent *v2.Entitlement) ([]*v2.Grant, annotations.Annotations, error) { if principal.Id.ResourceType != clmMemberResourceType.Id { - return nil, nil, fmt.Errorf("baton-docusign: invalid principal type: expected %s, got %s", clmMemberResourceType.Id, principal.Id.ResourceType) + return nil, nil, status.Errorf(codes.InvalidArgument, "baton-docusign: invalid principal type: expected %s, got %s", clmMemberResourceType.Id, principal.Id.ResourceType) } memberID := principal.Id.Resource groupID := ent.Resource.Id.Resource - groupHref, err := clmGroupHrefFromResource(ent.Resource) - if err != nil { - return nil, nil, err + + // clmIDFromHref reduces an empty Href to "" too, so an empty groupID or memberID + // would falsely match a degenerate currentGroups entry below (empty groupID hits the + // "already a member" check before clmPreferredHref's own empty-id guard ever runs) — + // same class of bug as clm_folders.go's Grant/Revoke, guarded the same way. + if memberID == "" || groupID == "" { + return nil, nil, status.Errorf(codes.InvalidArgument, "baton-docusign: granting CLM group membership: member or group missing native ID") } + // Don't require the group's Href off ent.Resource: the pebble storage engine hydrates + // an entitlement's Resource as an identity-only stub (no profile, no annotations), so + // it isn't always available. The groupHref this Grant actually writes below is + // resolved via clmPreferredHref, preferring (in order): ent.Resource's own profile + // Href if present, then a real sample Href from currentGroups, else client.GroupHref. currentGroups, annos, err := g.client.GetMemberGroups(ctx, memberID) if err != nil { - return nil, annos, fmt.Errorf("getting current groups for CLM member %s: %w", memberID, err) + return nil, annos, fmt.Errorf("baton-docusign: getting current groups for CLM member %s: %w", memberID, err) } for _, current := range currentGroups { @@ -167,12 +178,27 @@ func (g *clmGroupBuilder) Grant(ctx context.Context, principal *v2.Resource, ent } } + // Prefer a real, server-issued Href already on hand over one derived from the + // discovered CLM base URL — see clmPreferredHref's doc. The target group's own + // profile href (parseIntoClmGroupResource still populates it for display) comes + // first: it's this exact group's own recorded Href, not a sibling's (one of this + // member's OTHER current groups) to derive from — only ever a sample, never + // required, so an identity-only ent.Resource (the pebble-hydrated case this fix + // exists for) still falls through to the other-groups/fallback path unchanged. + sampleHrefs := clmSampleHrefsFrom(ent.Resource, currentGroups, func(g client.ClmGroup) string { return g.Href }) + groupHref, err := clmPreferredHref(ctx, groupID, sampleHrefs, func() (string, error) { + return g.client.GroupHref(ctx, groupID) + }) + if err != nil { + return nil, annos, fmt.Errorf("baton-docusign: resolving href for CLM group %s: %w", groupID, err) + } + newGroups := make([]client.ClmGroup, 0, len(currentGroups)+1) newGroups = append(newGroups, currentGroups...) newGroups = append(newGroups, client.ClmGroup{Href: groupHref}) patchAnnos, err := g.client.PatchMemberGroups(ctx, memberID, newGroups) if err != nil { - return nil, patchAnnos, fmt.Errorf("granting CLM group membership: %w", err) + return nil, patchAnnos, fmt.Errorf("baton-docusign: granting CLM group membership: %w", err) } return nil, patchAnnos, nil @@ -186,9 +212,17 @@ func (g *clmGroupBuilder) Revoke(ctx context.Context, grantObj *v2.Grant) (annot memberID := grantObj.Principal.Id.Resource groupID := grantObj.Entitlement.Resource.Id.Resource + // Same guard as Grant, and for the same reason: an empty groupID would falsely match + // a currentGroups entry with an empty Href (clmIDFromHref("") == ""), excluding it + // from remainingGroups — and PutMemberGroups is a full-replace, so that unrelated + // membership would actually be removed from the real account. + if memberID == "" || groupID == "" { + return nil, status.Errorf(codes.InvalidArgument, "baton-docusign: revoking CLM group membership: member or group missing native ID") + } + currentGroups, annos, err := g.client.GetMemberGroups(ctx, memberID) if err != nil { - return annos, fmt.Errorf("getting current groups for CLM member %s: %w", memberID, err) + return annos, fmt.Errorf("baton-docusign: getting current groups for CLM member %s: %w", memberID, err) } remainingGroups := make([]client.ClmGroup, 0, len(currentGroups)) @@ -207,7 +241,7 @@ func (g *clmGroupBuilder) Revoke(ctx context.Context, grantObj *v2.Grant) (annot putAnnos, err := g.client.PutMemberGroups(ctx, memberID, remainingGroups) if err != nil { - return putAnnos, fmt.Errorf("revoking CLM group membership: %w", err) + return putAnnos, fmt.Errorf("baton-docusign: revoking CLM group membership: %w", err) } return putAnnos, nil @@ -221,14 +255,14 @@ func newClmGroupBuilder(c *client.Client) *clmGroupBuilder { } // parseIntoClmGroupResource maps a client.ClmGroup to a Baton v2.Resource. The Href is -// carried in the profile (not just used to derive the ResourceId) so Grant() can -// reconstruct a reference to this group without needing to guess a URL — see -// clmGroupHrefFromResource. +// kept in the profile both for display and as the preferred sample href for Grant; +// Grant falls back to client.GroupHref when it's absent, since neither a profile nor an +// annotation is guaranteed to survive to where it's needed. func parseIntoClmGroupResource(group *client.ClmGroup) (*v2.Resource, error) { profile := map[string]any{ - "name": group.Name, - "groupType": group.GroupType, - "href": group.Href, + "name": group.Name, + "groupType": group.GroupType, + profileFieldHref: group.Href, } return rs.NewGroupResource( @@ -239,14 +273,3 @@ func parseIntoClmGroupResource(group *client.ClmGroup) (*v2.Resource, error) { rs.WithResourceProfile(profile), ) } - -// clmGroupHrefFromResource reads back the Href stashed in a CLM group resource's -// profile (see parseIntoClmGroupResource) — needed to reference the group in a -// Members.Patch grant body. -func clmGroupHrefFromResource(groupResource *v2.Resource) (string, error) { - href, ok := rs.GetProfileStringValue(rs.GetProfile(groupResource), "href") - if !ok || href == "" { - return "", fmt.Errorf("baton-docusign: CLM group resource %s is missing its href profile field", groupResource.Id.Resource) - } - return href, nil -} diff --git a/pkg/connector/clm_groups_test.go b/pkg/connector/clm_groups_test.go index 02e9dbd3..34c6630f 100644 --- a/pkg/connector/clm_groups_test.go +++ b/pkg/connector/clm_groups_test.go @@ -2,6 +2,7 @@ package connector import ( "context" + "fmt" "testing" "github.com/conductorone/baton-docusign/pkg/client" @@ -176,6 +177,151 @@ func TestClmGroupBuilder_GrantAndRevoke_Idempotent(t *testing.T) { } } +// TestClmGroupBuilder_GrantAndRevoke_RejectEmptyID is a regression test mirroring +// TestClmFolderBuilder_GrantAndRevoke_RejectEmptyPrincipalID: clmIDFromHref reduces an +// empty Href to "", so an empty memberID or groupID must be rejected before Grant/Revoke +// reach the currentGroups matching loop — otherwise an empty groupID could match a +// currentGroups entry with an empty Href, causing Grant to falsely report +// GrantAlreadyExists (bypassing clmPreferredHref's own empty-id check) or Revoke to +// silently drop that unrelated membership via PutMemberGroups' full-replace semantics. +func TestClmGroupBuilder_GrantAndRevoke_RejectEmptyID(t *testing.T) { + _, c := clmtest.NewServer(t) + b := newClmGroupBuilder(c) + ctx := context.Background() + + validMember := clmIdentityOnlyResource(clmMemberResourceType, "member-carol") + validGroup := clmIdentityOnlyResource(clmGroupResourceType, "group-legal") + emptyMember := clmIdentityOnlyResource(clmMemberResourceType, "") + emptyGroup := clmIdentityOnlyResource(clmGroupResourceType, "") + + cases := []struct { + name string + principal *v2.Resource + groupRes *v2.Resource + }{ + {"empty member ID", emptyMember, validGroup}, + {"empty group ID", validMember, emptyGroup}, + {"both empty", emptyMember, emptyGroup}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ent := &v2.Entitlement{Slug: entitlementClmGroupMember, Resource: tc.groupRes} + + if _, _, err := b.Grant(ctx, tc.principal, ent); err == nil { + t.Error("expected Grant to reject an empty member or group ID, got nil error") + } + + grantObj := &v2.Grant{Principal: tc.principal, Entitlement: ent} + if _, err := b.Revoke(ctx, grantObj); err == nil { + t.Error("expected Revoke to reject an empty member or group ID, got nil error") + } + }) + } +} + +// TestClmGroupBuilder_Grant_SurvivesIdentityOnlyEntitlementResource is a regression test: +// passes an identity-only entitlement Resource (pebble's V3EntitlementToV2 shape — no +// profile, no annotations, nothing but Id) to confirm Grant resolves the groupHref to +// write via clmPreferredHref without needing anything on ent.Resource itself. The two +// subtests below exercise both of clmPreferredHref's branches. +func TestClmGroupBuilder_Grant_SurvivesIdentityOnlyEntitlementResource(t *testing.T) { + // An identity-only Resource — exactly what V3EntitlementToV2 hands back on pebble, + // carrying nothing but the group's ID. + identityOnlyGroupResource := clmIdentityOnlyResource(clmGroupResourceType, "group-legal") + ent := &v2.Entitlement{Slug: entitlementClmGroupMember, Resource: identityOnlyGroupResource} + + t.Run("fallback branch: member has no other groups, derives via client.GroupHref", func(t *testing.T) { + srv, c := clmtest.NewServer(t) + b := newClmGroupBuilder(c) + ctx := context.Background() + + // member-dave is seeded with zero groups (clmtest/seed.go), so currentGroups is + // empty and clmPreferredHref has no sample to derive from. + memberResource, err := rs.NewResource("Dave", clmMemberResourceType, "member-dave") + if err != nil { + t.Fatalf("NewResource: %v", err) + } + + if _, annos, err := b.Grant(ctx, memberResource, ent); err != nil { + t.Fatalf("Grant with an identity-only ent.Resource: %v", err) + } else if hasAlreadyExists(annos) { + t.Error("first Grant should not report GrantAlreadyExists") + } + groups := srv.MemberGroups("member-dave") + found := false + for _, g := range groups { + if g == "group-legal" { + found = true + } + } + if !found { + t.Fatalf("expected dave to be granted group-legal, got %v", groups) + } + }) + + t.Run("sample branch: member already has another group, derives via clmPreferredHref's sample", func(t *testing.T) { + srv, c := clmtest.NewServer(t) + b := newClmGroupBuilder(c) + ctx := context.Background() + + // member-carol is seeded into group-finance (clmtest/seed.go), giving + // clmPreferredHref a real sample Href to derive group-legal's Href from instead + // of falling back to client.GroupHref. srv.GroupHref and the fallback + // client.GroupHref build the same shape from the same discovered base URL, so + // they'd be byte-identical here — overriding group-finance's Href to a + // different host makes the sample branch's output actually distinguishable from + // what the fallback branch would have produced. + const sampleHost = "https://other.example.com" + altGroupFinanceHref := fmt.Sprintf("%s/v2/%s/groups/group-finance", sampleHost, clmtest.AccountID) + srv.SetGroupHref("group-finance", altGroupFinanceHref) + + memberResource, err := rs.NewResource("Carol", clmMemberResourceType, "member-carol") + if err != nil { + t.Fatalf("NewResource: %v", err) + } + + if _, annos, err := b.Grant(ctx, memberResource, ent); err != nil { + t.Fatalf("Grant with an identity-only ent.Resource: %v", err) + } else if hasAlreadyExists(annos) { + t.Error("first Grant should not report GrantAlreadyExists") + } + groups := srv.MemberGroups("member-carol") + found := false + for _, g := range groups { + if g == "group-legal" { + found = true + } + } + if !found { + t.Fatalf("expected carol to be granted group-legal, got %v", groups) + } + + // MemberGroups reduces every Href to its trailing ID (matching the real API's + // own comparison semantics), which can't tell a sample-derived Href from a + // fallback-derived one. Asserting on the raw Href the server actually received, + // derived from the overridden group-finance sample (a different host than + // client.GroupHref's fallback would produce), pins that clmPreferredHref + // actually used the sample rather than falling back. + wantHref, err := clmHrefWithID(altGroupFinanceHref, "group-legal") + if err != nil { + t.Fatalf("clmHrefWithID (computing expected Href): %v", err) + } + // PatchMemberGroups sends the member's full current+new list (additive), so the + // PATCH body also carries carol's pre-existing group-finance entry alongside the + // new group-legal one. + hrefs := srv.LastPatchedMemberGroupHrefs("member-carol") + sawWantHref := false + for _, h := range hrefs { + if h == wantHref { + sawWantHref = true + } + } + if !sawWantHref { + t.Errorf("expected the PATCH request to carry Href %q, got %v", wantHref, hrefs) + } + }) +} + // TestClmGroupMemberSlugRegressionPin guards against an accidental rename of // the "member" entitlement slug: since clm_group is a new, unreleased // resource type, add this before the first release locks the slug in. diff --git a/pkg/connector/clm_members.go b/pkg/connector/clm_members.go index 17618a38..74352724 100644 --- a/pkg/connector/clm_members.go +++ b/pkg/connector/clm_members.go @@ -83,7 +83,10 @@ func newClmMemberBuilder(c *client.Client) *clmMemberBuilder { } } -// parseIntoClmMemberResource maps a client.ClmMember to a Baton v2.Resource. +// parseIntoClmMemberResource maps a client.ClmMember to a Baton v2.Resource. The Href is +// kept in the profile both for display and as the preferred sample href for Grant; +// Grant falls back to client.MemberHref when it's absent, since neither a profile nor +// an annotation is guaranteed to survive to where it's needed. func parseIntoClmMemberResource(member *client.ClmMember) (*v2.Resource, error) { profile := map[string]any{ profileFieldEmail: member.Email, @@ -91,7 +94,7 @@ func parseIntoClmMemberResource(member *client.ClmMember) (*v2.Resource, error) "role": member.Role, "exemptFromUserSync": member.ExemptFromUserSync, "portalOnly": member.PortalOnly, - "href": member.Href, + profileFieldHref: member.Href, } displayName := member.UserName diff --git a/pkg/connector/helper.go b/pkg/connector/helper.go index e4532098..1795e533 100644 --- a/pkg/connector/helper.go +++ b/pkg/connector/helper.go @@ -1,10 +1,16 @@ package connector import ( + "context" + "net/url" "strings" + "github.com/conductorone/baton-docusign/pkg/client" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/pagination" + rs "github.com/conductorone/baton-sdk/pkg/types/resource" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" + "go.uber.org/zap" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -15,6 +21,7 @@ const ( profileFieldEmail = "email" profileFieldUsername = "username" profileFieldGroupName = "group_name" + profileFieldHref = "href" ) // parsePageToken deserializes the Baton token and returns the Bag and page number for upstream. @@ -53,7 +60,9 @@ func parsePageToken(i string, resourceID *v2.ResourceId) (*pagination.Bag, strin // - PermissionDenied/Unauthenticated: the account/token lacks the CLM subscription // or OAuth scope — the expected case for most eSignature-only accounts. // - NotFound: the discovery endpoint 404s for an account that was never provisioned -// in the legacy SpringCM system CLM discovery still runs through. +// in the legacy SpringCM system. CLM's Object API returns 404 both for "doesn't +// exist" and "exists but no access", so treat it as a plausible no-access signal, +// not proof the account lacks CLM. // - FailedPrecondition: ensureClmInitialized wraps its "response didn't contain a // recognized base-URL field" error with this code specifically — a non-CLM // account's discovery response plausibly has a different shape entirely (no CLM @@ -75,13 +84,130 @@ func isOptInFeatureUnavailableError(err error) bool { } } -// clmIDFromHref extracts the trailing path segment from a CLM object's Href — CLM's -// Object API schemas expose a Href field ("Uri where the object can be retrieved") but -// no separate opaque Id field, so this is the closest thing to a native ID CLM exposes. +// clmIDFromHref extracts the trailing path segment from a CLM object's Href — see +// client.IDFromHref's doc. pkg/client/clmtest can't import pkg/connector, so the single +// definition lives in pkg/client and both packages delegate to it instead of +// maintaining two copies. func clmIDFromHref(href string) string { - href = strings.TrimSuffix(href, "/") - if idx := strings.LastIndex(href, "/"); idx != -1 { - return href[idx+1:] + return client.IDFromHref(href) +} + +// clmHrefWithID rebuilds sampleHref with its trailing ID segment replaced by newID. +// Assumes same-collection Hrefs share path shape (only the ID differs) — not verified +// against a live tenant. Callers must only pass samples from a single collection; newID +// must be non-empty (see the check below for why). +func clmHrefWithID(sampleHref, newID string) (string, error) { + if newID == "" { + // Without this check, an empty newID passes every shape check below (sampleHref + // still has a valid path) and silently returns a trailing-slash href with no ID + // segment at all — a malformed href that goes on to be sent as-is inside a + // PatchFolderSecurity/PatchMemberGroups request body, surfacing (if at all) as an + // opaque remote validation failure with no link back to "the ID was empty." + return "", status.Errorf(codes.InvalidArgument, "baton-docusign: cannot derive a sibling href from %q — newID is empty", sampleHref) + } + // Split on the parsed URL's Path, not the raw string: a "/" inside a query string or + // fragment (e.g. ".../groups/g1?filter=a/b") would otherwise win over the real path + // separator, corrupting the query instead of replacing the ID segment. A bare + // scheme+host like "https://clm.example.com" parses with an empty Path, so this also + // covers that case — no real path at all to derive from. + u, err := url.Parse(sampleHref) + if err != nil || u.Path == "" { + return "", status.Errorf(codes.InvalidArgument, "baton-docusign: cannot derive a sibling href from %q — no path segment found", sampleHref) + } + // A Path already ending in "/" has no ID segment to replace — the same degenerate + // shape the empty-newID check above guards against on the output side. Trimming it + // instead of rejecting it would silently drop the real collection segment: e.g. + // ".../groups/" trims to ".../groups", and the LastIndex split below would then + // wrongly treat "groups" as the ID to replace, producing ".../" with the + // actual collection segment gone. + if u.Path == "/" || strings.HasSuffix(u.Path, "/") { + return "", status.Errorf(codes.InvalidArgument, "baton-docusign: cannot derive a sibling href from %q — sample has no ID segment (trailing slash)", sampleHref) + } + idx := strings.LastIndex(u.Path, "/") + if idx == -1 { + // A relative, scheme-less sample (e.g. "no-path-separator") parses as an opaque + // Path with no leading "/" at all — url.Parse doesn't error on it, so this catches + // what the raw-string check used to. + return "", status.Errorf(codes.InvalidArgument, "baton-docusign: cannot derive a sibling href from %q — no path separator found", sampleHref) + } + u.Path = u.Path[:idx+1] + newID + // Clear any query/fragment carried over from the sample — the derived href + // identifies a different object than the sample's, so a leftover query string or + // fragment (e.g. ".../group-old?filter=a/b" deriving ".../group-new?filter=a/b") + // would misrepresent the sample's, not the target's, state in a write body. + u.RawQuery = "" + u.Fragment = "" + return u.String(), nil +} + +// clmPreferredHref resolves the href to send in a WRITE targeting id: it prefers +// deriving the href from a real, server-issued sample href (the first entry in +// sampleHrefs that clmHrefWithID can use) over guessing at the host from the discovered +// CLM base URL. CLM's Href host is not guaranteed to match the discovered base URL — a +// write that carries a subtly-wrong host risks CLM rejecting it or storing it +// inconsistently; a read-side comparison doesn't have this risk, since clmIDFromHref +// only ever looks at the trailing ID. Falls back to deriveFallback when no real sample +// href is available (e.g. a member with no other group memberships yet, or a folder +// with no other security entries yet) — the expected, routine case (empty sampleHrefs +// never reaches clmHrefWithID at all). If sampleHrefs is non-empty but every sample +// fails to parse, that's the unexpected case: it means CLM returned a Href shape this +// codebase's assumptions don't cover, so it's logged at Debug before falling back — not +// visible by default, but distinguishable from the routine case for anyone who does +// raise verbosity, rather than discarded with no trace at all. +func clmPreferredHref(ctx context.Context, id string, sampleHrefs []string, deriveFallback func() (string, error)) (string, error) { + if id == "" { + // clmHrefWithID rejects an empty id on the sample path, but deriveFallback (a + // caller-supplied closure, typically client.GroupHref/MemberHref built from this + // same id) has no such guard — an empty id would sail through it and produce the + // same malformed trailing-slash href this function exists to prevent. Reject here + // once, before either path runs, rather than relying on every deriveFallback + // closure to check it independently. + return "", status.Errorf(codes.InvalidArgument, "baton-docusign: cannot resolve a CLM href — id is empty") + } + var lastErr error + for _, sample := range sampleHrefs { + derived, err := clmHrefWithID(sample, id) + if err == nil { + return derived, nil + } + lastErr = err + } + if lastErr != nil { + ctxzap.Extract(ctx).Debug("baton-docusign: every sample href failed to derive a sibling href, falling back to a base-URL-derived href", + zap.Int("sample_count", len(sampleHrefs)), zap.Error(lastErr)) + } else { + // No samples at all (as opposed to samples that failed to parse, above) — the + // routine case for e.g. a group/member with no other memberships yet. Still + // worth a Debug line: deriveFallback's shape is unverified against a live + // tenant, so if CLM ever rejects or silently ignores the derived href, this is + // the only record that a guess (not a real sample) produced it. + ctxzap.Extract(ctx).Debug("baton-docusign: no sample href available, falling back to a base-URL-derived href", + zap.String("id", id)) + } + return deriveFallback() +} + +// clmSampleHrefsFrom builds the sampleHrefs slice clmPreferredHref expects: principal's +// own profile href first (the most direct sample when the resource happens to carry +// one — only ever a sample, never required, so an identity-only principal still falls +// through to the entries/fallback path unchanged), followed by every entry's Href. +func clmSampleHrefsFrom[T any](principal *v2.Resource, entries []T, hrefOf func(T) string) []string { + sampleHrefs := make([]string, 0, len(entries)+1) + // GetProfileStringValue returns ok == true for a present-but-empty "href" key + // (every parseIntoClm*Resource always writes it), so this must also reject "" — + // otherwise an absent-sample account looks identical to an unexpected-shape one: + // the empty string always fails clmHrefWithID, tripping clmPreferredHref's + // unexpected-failure log on what is actually the routine no-sample case. + if href, ok := rs.GetProfileStringValue(rs.GetProfile(principal), profileFieldHref); ok && href != "" { + sampleHrefs = append(sampleHrefs, href) + } + for _, e := range entries { + // Same reasoning as the profile href above: an empty entry Href would fail + // clmHrefWithID the same way and re-trip the unexpected-failure log on + // otherwise-routine degenerate data. + if href := hrefOf(e); href != "" { + sampleHrefs = append(sampleHrefs, href) + } } - return href + return sampleHrefs } diff --git a/pkg/connector/helper_test.go b/pkg/connector/helper_test.go index 034ed990..b6ac2361 100644 --- a/pkg/connector/helper_test.go +++ b/pkg/connector/helper_test.go @@ -1,9 +1,12 @@ package connector import ( + "context" "errors" "testing" + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + rs "github.com/conductorone/baton-sdk/pkg/types/resource" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -32,3 +35,191 @@ func TestIsOptInFeatureUnavailableError(t *testing.T) { }) } } + +func TestClmHrefWithID(t *testing.T) { + got, err := clmHrefWithID("https://clm.example.com/v2/acct-1/groups/group-old", "group-new") + if err != nil { + t.Fatalf("clmHrefWithID: %v", err) + } + if want := "https://clm.example.com/v2/acct-1/groups/group-new"; got != want { + t.Errorf("clmHrefWithID = %q, want %q", got, want) + } + + if _, err := clmHrefWithID("no-path-separator", "x"); err == nil { + t.Error("expected an error for a sample href with no path separator") + } + + // A bare scheme+host contains a "/" too (separating scheme from host), so this must + // be rejected on its own — not accepted as a valid sample producing a garbage + // "https://x"-shaped href with no real path. + if _, err := clmHrefWithID("https://clm.example.com", "x"); err == nil { + t.Error("expected an error for a sample href with no path segment (bare scheme+host)") + } + + // An empty newID would otherwise pass every shape check on sampleHref and silently + // return a trailing-slash href with no ID segment at all (e.g. + // ".../groups/") — a malformed href with nothing pointing back at "the ID was empty". + if _, err := clmHrefWithID("https://clm.example.com/v2/acct-1/groups/group-old", ""); err == nil { + t.Error("expected an error for an empty newID") + } + + // A sample already ending in "/" (e.g. a collection root, or some other degenerate + // shape) must be rejected outright rather than trimmed: trimming would make + // ".../groups/" indistinguishable from ".../groups" (a href whose ID happens to be + // "groups"), so the ID-replacement below would wrongly drop the real collection + // segment and produce ".../" instead of ".../groups/". + if _, err := clmHrefWithID("https://clm.example.com/v2/acct-1/groups/", "group-new"); err == nil { + t.Error("expected an error for a sample href with a trailing slash (no ID segment)") + } + + // Regression test: a "/" inside a query string must not be mistaken for the real + // path separator. Splitting on the raw string instead of the parsed URL's Path used + // to corrupt the query (".../group-old?filter=a/b" became ".../group-old?filter=a/" + // + newID) instead of replacing the actual ID segment. The derived href also drops + // the sample's query entirely — it identifies a different object, so the sample's + // query/fragment (irrelevant, and potentially misleading, for the target) is cleared + // rather than carried over. + got, err = clmHrefWithID("https://clm.example.com/v2/acct-1/groups/group-old?filter=a/b", "group-new") + if err != nil { + t.Fatalf("clmHrefWithID: %v", err) + } + if want := "https://clm.example.com/v2/acct-1/groups/group-new"; got != want { + t.Errorf("clmHrefWithID = %q, want %q", got, want) + } +} + +func TestClmPreferredHref(t *testing.T) { + ctx := context.Background() + fallbackCalled := false + fallback := func() (string, error) { + fallbackCalled = true + return "https://derived.example.com/v2/acct-1/groups/group-target", nil + } + + t.Run("prefers a real sample href over the fallback", func(t *testing.T) { + fallbackCalled = false + got, err := clmPreferredHref(ctx, "group-target", []string{"https://real.example.com/v2/acct-1/groups/group-other"}, fallback) + if err != nil { + t.Fatalf("clmPreferredHref: %v", err) + } + if want := "https://real.example.com/v2/acct-1/groups/group-target"; got != want { + t.Errorf("clmPreferredHref = %q, want %q", got, want) + } + if fallbackCalled { + t.Error("expected the fallback NOT to be called when a real sample href is available") + } + }) + + t.Run("skips empty sample hrefs", func(t *testing.T) { + fallbackCalled = false + got, err := clmPreferredHref(ctx, "group-target", []string{"", "https://real.example.com/v2/acct-1/groups/group-other"}, fallback) + if err != nil { + t.Fatalf("clmPreferredHref: %v", err) + } + if want := "https://real.example.com/v2/acct-1/groups/group-target"; got != want { + t.Errorf("clmPreferredHref = %q, want %q", got, want) + } + }) + + // Regression test: id == "" used to sail through to deriveFallback (e.g. + // client.GroupHref(ctx, "")) whenever no sample href was available, producing a + // malformed trailing-slash href — the exact failure mode clmHrefWithID's own + // empty-newID check exists to catch on the sample path, but that check never ran + // here since an empty id with no samples skips clmHrefWithID entirely. + t.Run("rejects an empty id before trying either path", func(t *testing.T) { + fallbackCalled = false + if _, err := clmPreferredHref(ctx, "", nil, fallback); err == nil { + t.Error("expected an error for an empty id, got nil") + } + if fallbackCalled { + t.Error("expected the fallback NOT to be called for an empty id") + } + }) + + t.Run("falls back when no sample href is available", func(t *testing.T) { + fallbackCalled = false + got, err := clmPreferredHref(ctx, "group-target", nil, fallback) + if err != nil { + t.Fatalf("clmPreferredHref: %v", err) + } + if want := "https://derived.example.com/v2/acct-1/groups/group-target"; got != want { + t.Errorf("clmPreferredHref = %q, want %q", got, want) + } + if !fallbackCalled { + t.Error("expected the fallback to be called when no sample hrefs are available") + } + }) + + // Regression test: sampleHrefs is non-empty but every sample is malformed in an + // unexpected way (not the routine "no sample yet" case) — must still fall back + // safely rather than erroring, even though this case is now logged (see + // clmPreferredHref's doc). + t.Run("falls back when every sample href fails to parse", func(t *testing.T) { + fallbackCalled = false + got, err := clmPreferredHref(ctx, "group-target", []string{"no-path-separator", "https://clm.example.com"}, fallback) + if err != nil { + t.Fatalf("clmPreferredHref: %v", err) + } + if want := "https://derived.example.com/v2/acct-1/groups/group-target"; got != want { + t.Errorf("clmPreferredHref = %q, want %q", got, want) + } + if !fallbackCalled { + t.Error("expected the fallback to be called when every sample href fails to parse") + } + }) +} + +func TestClmSampleHrefsFrom(t *testing.T) { + type entry struct{ Href string } + hrefOf := func(e entry) string { return e.Href } + + t.Run("includes a non-empty profile href first", func(t *testing.T) { + principal, err := rs.NewResource("g", clmGroupResourceType, "group-1", rs.WithResourceProfile(map[string]any{"href": "https://real.example.com/v2/acct-1/groups/group-1"})) + if err != nil { + t.Fatalf("NewResource: %v", err) + } + got := clmSampleHrefsFrom(principal, []entry{{Href: "https://other.example.com/v2/acct-1/groups/group-2"}}, hrefOf) + want := []string{"https://real.example.com/v2/acct-1/groups/group-1", "https://other.example.com/v2/acct-1/groups/group-2"} + if len(got) != len(want) || got[0] != want[0] || got[1] != want[1] { + t.Errorf("clmSampleHrefsFrom = %v, want %v", got, want) + } + }) + + // Regression test: parseIntoClm*Resource always writes the "href" profile key, even + // when the underlying CLM object has no Href yet, so GetProfileStringValue reports + // ok == true for an empty string. Appending it anyway would make an + // identity-only-of-real-samples principal look identical to one with a genuinely + // malformed sample, tripping clmPreferredHref's unexpected-failure log on what is + // actually the routine no-sample case. + t.Run("excludes an empty profile href", func(t *testing.T) { + principal, err := rs.NewResource("g", clmGroupResourceType, "group-1", rs.WithResourceProfile(map[string]any{"href": ""})) + if err != nil { + t.Fatalf("NewResource: %v", err) + } + got := clmSampleHrefsFrom(principal, []entry{{Href: "https://other.example.com/v2/acct-1/groups/group-2"}}, hrefOf) + want := []string{"https://other.example.com/v2/acct-1/groups/group-2"} + if len(got) != len(want) || got[0] != want[0] { + t.Errorf("clmSampleHrefsFrom = %v, want %v", got, want) + } + }) + + // Same reasoning as the profile-href case above, but for an entry's Href — degenerate + // data, not an unexpected shape, so it must be excluded the same way. + t.Run("excludes an empty entry href", func(t *testing.T) { + principal := &v2.Resource{Id: &v2.ResourceId{ResourceType: clmGroupResourceType.Id, Resource: "group-1"}} + got := clmSampleHrefsFrom(principal, []entry{{Href: ""}, {Href: "https://other.example.com/v2/acct-1/groups/group-2"}}, hrefOf) + want := []string{"https://other.example.com/v2/acct-1/groups/group-2"} + if len(got) != len(want) || got[0] != want[0] { + t.Errorf("clmSampleHrefsFrom = %v, want %v", got, want) + } + }) + + t.Run("no profile at all", func(t *testing.T) { + principal := &v2.Resource{Id: &v2.ResourceId{ResourceType: clmGroupResourceType.Id, Resource: "group-1"}} + got := clmSampleHrefsFrom(principal, []entry{{Href: "https://other.example.com/v2/acct-1/groups/group-2"}}, hrefOf) + want := []string{"https://other.example.com/v2/acct-1/groups/group-2"} + if len(got) != len(want) || got[0] != want[0] { + t.Errorf("clmSampleHrefsFrom = %v, want %v", got, want) + } + }) +}