[CXH-2208] fix: derive CLM hrefs for Grant/Revoke instead of requiring a profile field - #63
[CXH-2208] fix: derive CLM hrefs for Grant/Revoke instead of requiring a profile field#63FeliLucero1 wants to merge 35 commits into
Conversation
FeliLucero1
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Connector PR Review: [CXH-2208] fix: derive CLM hrefs for Grant/Revoke instead of requiring a profile fieldBlocking Issues: 0 | Suggestions: 4 | Threads Resolved: 0 Review SummaryThe new commit reworks Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
422de89 to
94c2054
Compare
- Remove clmPreferredHref's redundant empty-string guard — clmHrefWithID already rejects an empty sample on its own. - Rephrase clmPreferredHref's doc away from "no live tenant confirmed this" hedging into a factual statement of the write-vs-read Href asymmetry. - Update two clm_folders_test.go comments that still named the removed clmGroupHrefFromResource helper as current write-side behavior.
- Remove the redundant client.GroupHref/client.MemberHref calls from clm_folder's Revoke — clmFindGroupSecurityIndex/clmFindUserSecurityIndex only ever compare by clmIDFromHref, so principal.Id.Resource (already a bare ID) works directly without the extra ensureClmReady round trip and discarded error path. - Update stale comments (clm_folders_test.go, clm_groups.go) that still described Grant as deriving the Href straight from the ID, or named the deleted clmGroupHrefFromResource/clmMemberHrefFromResource helpers. - Split TestClmGroupBuilder_Grant_SurvivesIdentityOnlyEntitlementResource into two subtests so both of clmPreferredHref's branches (fallback via client.GroupHref, and the real-sample branch) get exercised. - Assert the exact Href a Grant call writes in TestClmFolderBuilder_GrantAndRevoke_SurvivesIdentityOnlyPrincipal, not just the resulting AccessType.
clmHrefWithID located the ID segment to replace via strings.LastIndex
on the raw sampleHref string, even though url.Parse was already called
to validate it. A "/" inside a query string or fragment (e.g.
".../group-old?filter=a/b") would win over the real path separator,
corrupting the query instead of replacing the ID
(".../group-old?filter=a/" + newID). Now splits on the parsed URL's
Path and reassembles via url.URL, so query/fragment content can't be
mistaken for the path separator. Added a regression test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
test-groups/test-signing-groups/test-permission-profiles are needs-chained within a single workflow run but share no concurrency group at all, so two different branches' runs against this repo's one live demo account can still execute concurrently -- one run's mid-cycle Grant/Revoke corrupting another's "should be zero grants after Revoke" assertion. Confirmed directly: this same failure just hit test-groups here while PR #64 (stacked on this branch) ran its own, already-fixed version of this same job at the same time. Adds a workflow-level concurrency group so the whole three-job run queues/cancels as one unit against the shared account. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The all-samples-failed-to-parse branch already logged before falling back to a derived href; the more common no-samples-at-all branch (a group/member with no other memberships yet) fell back completely silently. Since deriveFallback's shape is unverified against a live tenant, a silently-wrong derived href written into a PatchFolderSecurity body would leave a grant with no observable trace of why it didn't take effect. Caught by the automated PR reviewer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
14806b3 to
1a72aec
Compare
PR #63 and this branch independently fixed the same tautological-test finding on TestClmFolderBuilder_Grant_SurvivesIdentityOnlyPrincipal_SampleBranch (a byte-identical sample-derived vs. fallback-derived Href) via two different mechanisms — Server.SetFolderGroupSecurityHref/ SetFolderUserSecurityHref (surgical Href override) vs. a full PatchFolderSecurity re-seed — which the rebase concatenated into one function body (duplicate sampleHost/wantHref declarations, and the re-seed silently overriding the override). Keep the surgical version: it preserves folder-contracts' original seed shape (both group entries) instead of replacing it.
Rebasing this branch onto PR #63's latest tip silently reapplied an older PR #64 commit's import-block patch over helper.go's newly-added codes/status imports (no conflict was flagged since the patch context didn't include them), breaking the build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| concurrency: | ||
| group: docusign-demo-account | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
🟡 Suggestion: The group key is a constant, so every PR run and every push-to-main run shares one queue. With cancel-in-progress: false GitHub still keeps only one pending run per group — "any previously pending job or workflow in the concurrency group will be canceled" — so with 3+ runs in flight the oldest queued run is cancelled rather than eventually run, and its checks land as cancelled needing a manual re-run. That's an acceptable trade for a shared demo account, but worth noting in the comment (or scoping the group to just the sync-test steps) so the cancellations aren't read as flakes.
There was a problem hiding this comment.
Same accepted trade-off already discussed on this and PR #64's ci.yaml — one shared demo account, workflow-level concurrency keeps the whole 3-job run serialized as one unit at the cost of an occasional cancelled-and-rerun on a busy day. Leaving as-is.
| // 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 |
There was a problem hiding this comment.
🟡 Suggestion: only u.Path is rewritten, so the sample's RawQuery and Fragment ride along into the derived href — TestClmHrefWithID pins this (.../group-old?filter=a/b → .../group-new?filter=a/b). The result is a write href identifying a different object, so carrying another entry's query/fragment is never wanted; clearing them (u.RawQuery = "", u.Fragment = "") makes the derivation shape-only. Low likelihood in practice since CLM Hrefs don't appear to carry query strings, but it costs two lines to rule out.
There was a problem hiding this comment.
Good catch — fixed in 71407ba: clmHrefWithID now clears RawQuery and Fragment before returning, since the derived href identifies a different object than the sample's. Updated TestClmHrefWithID's regression case to match (the derived href no longer carries the sample's ?filter=a/b).
staticcheck doesn't recognize t.Fatalf as terminating the goroutine, so it flags the found.AccessType dereference just after a "found == nil" guard as a possible nil pointer dereference. Adding an explicit return after each Fatalf breaks the control-flow path staticcheck otherwise sees, the standard fix for this well-known false-positive pattern. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Only Path was rewritten, so a sample carrying a query string or fragment leaked it into the derived href for a different object. Since the derived href identifies a distinct target, the sample's query/fragment is never meaningful there -- clear both so the derivation is shape-only. Caught by the automated PR reviewer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| // clmIDFromHref extracts the trailing path segment from a CLM object's Href — see | ||
| // client.IDFromHref's doc. pkg/client/clmtest reimplements the same logic locally to | ||
| // avoid depending on pkg/connector, so both packages call the one shared definition in | ||
| // pkg/client instead of maintaining two copies. |
There was a problem hiding this comment.
🟡 Suggestion: this doc contradicts itself — "pkg/client/clmtest reimplements the same logic locally" is exactly what this PR removed (clmtest's idFromHref now just calls client.IDFromHref), yet the next clause correctly says both packages call the one shared definition. The reason clmtest can't import pkg/connector is still worth stating; the "reimplements locally" part is now stale. Something like: "pkg/client/clmtest can't import pkg/connector, so the single definition lives in pkg/client and both packages delegate to it."
There was a problem hiding this comment.
Fixed in e395125 — reworded to state the actual reason (clmtest can't import pkg/connector) instead of the stale 'reimplements locally' claim.
| } | ||
| default: | ||
| return nil, nil, fmt.Errorf("baton-docusign: invalid principal type for CLM folder security: %s", principal.Id.ResourceType) | ||
| return nil, nil, status.Errorf(codes.InvalidArgument, "baton-docusign: invalid principal type for CLM folder security: %s", principal.Id.ResourceType) |
There was a problem hiding this comment.
🟡 Suggestion: this PR changed the group/member href-resolution error paths (lines 234, 261) to propagate getAnnos instead of nil, but the default: branch here — and the matching one in Revoke (line 386) — still returns nil, discarding the rate-limit annotations GetFolderFresh already produced. Returning getAnnos in both default branches would make the whole method consistent.
There was a problem hiding this comment.
Fixed in e395125 — both default branches (Grant and Revoke) now propagate getAnnos, consistent with the group/member branches.
Grant/Revoke's invalid-principal-type default branches still returned nil annotations, discarding the rate-limit annotations GetFolderFresh already produced -- inconsistent with the group/member branches in this same file, which already propagate them. Also fixed clmIDFromHref's doc comment, which claimed clmtest "reimplements the same logic locally" when this PR made it delegate to the same shared definition instead. Caught by the automated PR reviewer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| 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, "/") |
There was a problem hiding this comment.
🟡 Suggestion (low confidence): the split uses u.Path, which is the decoded path, so a sample href whose ID segment contains an encoded slash (e.g. .../groups/a%2Fb) decodes to /v2/acct/groups/a/b and LastIndex would then replace only b, yielding .../groups/a/<newID> instead of .../groups/<newID>. u.EscapedPath() (with the result assigned back via u.RawPath/url.PathUnescape) would be exact. Practically unreachable if CLM IDs are GUID-like, so this is robustness only, not a live bug.
…lope Live-tested against a real CLM tenant: with only spring_read/spring_write granted, account discovery succeeded but every CLM data call (ListGroups, ListPermissionSets) failed with 401 "Access Denied" (ErrorCode 103) — CLM requires the impersonation scope for data-plane access, confirmed against DocuSign's own CLM Authentication Overview example scope string. Also fixes ClmErrorResponse: the real error body nests fields under "Error" (UserMessage/DeveloperMessage/ErrorCode), not a top-level "Message" — every real CLM error was surfacing as "unknown CLM API error" instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| // account-discovery scope as "springcm_read" instead); confirmed harmless to | ||
| // request even when unrecognized — DocuSign's OAuth consent silently drops them | ||
| // rather than rejecting the whole grant. | ||
| clmScopes = []string{"impersonation", "spring_read", "spring_write", "springcm_read", "springcm_write"} |
There was a problem hiding this comment.
🟡 Suggestion: adding impersonation makes the documented CLM scope list stale. docs/doc-info.md:173 still tells operators to grant only spring_read/spring_write, and the in-code docs at pkg/client/clm_client.go:4 and pkg/connector/helper.go:47 say the same. Worth also noting that x/oauth2's refresh-token grant doesn't resend Scopes, so an already-connected CLM install keeps its old consent and must be re-authorized (--configure) before this fix takes effect.
| if e.Error.DeveloperMessage != "" && e.Error.DeveloperMessage != e.Error.UserMessage { | ||
| return fmt.Sprintf("CLM API error %d: %s (%s)", e.Error.ErrorCode, e.Error.UserMessage, e.Error.DeveloperMessage) | ||
| } | ||
| return fmt.Sprintf("CLM API error %d: %s", e.Error.ErrorCode, e.Error.UserMessage) |
There was a problem hiding this comment.
🟡 Suggestion: when CLM returns a DeveloperMessage but no UserMessage, the first branch is taken and renders an empty slot — CLM API error 103: (token missing impersonation scope) (double space, no primary text). Falling back to DeveloperMessage as the primary when UserMessage is empty would keep the message well-formed. Low confidence that CLM ever emits that combination, since the live sample populates both.
Message() rendered a malformed "CLM API error 103: (dev msg)" (empty primary slot, double space) whenever CLM returned a DeveloperMessage with no UserMessage. Now falls back to DeveloperMessage as the primary text in that case; added a regression test. clm_client.go's package doc still listed only spring_read/spring_write as CLM's OAuth scopes, left stale when impersonation was added to clmScopes. Both were flagged by the CI reviewer on the very first pass over this PR and never fixed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…eal cause DocuSign's CLM Authentication Overview docs (pasted live) confirm the Authorization Code Grant scope example is exactly signature+spring_read+ spring_write, with impersonation explicitly noted as JWT-Grant-only. The live tenant that returned 401 "Access Denied" (ErrorCode 103) already had this exact scope set granted, so the 401 was never a scope problem — current working theory is a demo/UAT environment limitation, per the same docs requiring a production account for CLM API access (see clm_client.go). Also drops springcm_read/springcm_write: DocuSign's official scopes reference lists no such scopes at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…wire shape
SearchFolders previously called a synchronous POST /folders/search endpoint
that doesn't exist for folders (confirmed live: 405 Method Not Allowed).
Folder search is CLM's async Task API instead: POST /foldersearchtasks with
{"Title": ""} (the only accepted "match everything" parameter — other names
including the "Name" field ClmFolder itself uses are rejected) creates a
search task that resolves inline or must be polled via its own Href until
Status leaves "Processing"; continuation pages GET the task's own Result
href directly rather than re-POSTing a new search. Verified live against a
100-folder real CLM tenant, list and grants both sync cleanly end-to-end.
Also fixes ClmFolderSecurity: the real wire shape is three flat arrays
(Groups/Roles/Users), each entry nesting the referenced object under an
"Item" key sibling to AccessType — not the paginated Page-wrapper types an
earlier version assumed, which broke JSON unmarshaling outright on any
folder with populated security. Confirmed via GetFolder?expand=Security
against real data.
Grant/Revoke on clm_folder (PatchFolderSecurity) remains unconfirmed to
work against a real tenant — tested live against a disposable test folder
with no effect despite matching the documented shape; see clm_client.go's
package doc for the evidence and the likely real mechanism (a dedicated
Task API endpoint, not this generic object Patch).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| requestedPage := clmRequestedPage{Offset: 0, PageSize: task.Result.Limit} | ||
| nextToken, err := getClmNextToken(requestedPage, len(task.Result.Items), task.Result.Next != "", task.Result.Total, task.Result.Href) |
There was a problem hiding this comment.
🟠 Bug: if CLM ever returns a Result with an empty Href, this mints a token with ResultHref: "", so the next SearchFolders call falls through the decoded.ResultHref != "" check at line 352 and POSTs a brand new search task — which returns page 1 again and, because requestedPage is rebuilt here as {Offset: 0, Requests: 0}, computes the byte-identical token every time. That's an unbounded loop re-emitting the same folders forever: maxClmListPages can't fire because both Offset and Requests reset on each create. Every other response field here (Next/Total/Offset) is explicitly distrusted per getClmNextToken's doc, so Result.Href deserves the same guard — e.g. return the first page with an empty next token (or a hard error) when task.Result.Href == "".
Secondary, same two lines: PageSize: task.Result.Limit trusts a response field for the short-page stop, and the create POST never sends pageSortParams.limit, so the caller's options.PageSize is ignored on page 1. If Limit comes back 0, itemCount < requested.PageSize is never true and every folder sync issues one extra out-of-range request past its last page.
| @@ -103,31 +176,68 @@ type ClmRoleSecurityEntry struct { | |||
| Item string `json:"Item"` | |||
| } | |||
There was a problem hiding this comment.
🟡 Suggestion: this doc still claims a "Confirmed shape" while the two structs on either side of it were just corrected away from identically-worded "confirmed" claims — and ClmUserSecurityEntry's new doc concedes that every populated folder-security entry on the tested tenant was a Group, so Roles cannot have been confirmed live either. If Roles is nested like Groups/Users ({"Item":{...},"AccessType":...}), unmarshalling an object into Item string returns an error, which fails the whole GetFolder?expand=Security call — the same sync-breaking failure this PR is fixing for Groups. Consider a tolerant unmarshaler (bare string, else Item.Name), or at minimum reword "Confirmed" to match the actual evidence.
| var grants []*v2.Grant | ||
|
|
||
| for _, entry := range folder.Security.Groups.Items { | ||
| for _, entry := range folder.Security.Groups { |
There was a problem hiding this comment.
🟡 Suggestion: entry.Href (line 146, and line 176 for Users) now comes from the nested Item.Href via the new UnmarshalJSON. If CLM ever returns the flat shape instead, json.Unmarshal does not error — unknown top-level keys are ignored — so Href silently decodes as "", and clmIDFromHref("") yields an empty principal resource ID that still gets emitted as a grant. That's the inverse of the wire-shape bug this PR fixes, and it fails silently rather than loudly. Worth skipping (with a Warn) entries whose Href is empty in both loops, matching the defensiveness of the new empty-ID guards in Grant/Revoke.
| 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) | ||
| } | ||
| // 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 { |
There was a problem hiding this comment.
🟡 Suggestion (security, low likelihood): this issues a bearer-token-authenticated GET to a URL taken verbatim from a response body, with no check that its scheme/host matches the discovered CLM base URL. The same applies to resultHref in getClmFolderSearchResultPage (line 388), which is worse in one respect: that URL is persisted into the pagination token and handed back to the connector on resume, so a tampered token would redirect an authenticated request — and the DocuSign access token with it — to an arbitrary host. Validating u.Scheme/u.Host against the resolved CLM base URL before calling doClmRequest closes both.
7 participants
Description
Grant/Revoke for CLM folders/groups/members previously required a
hrefprofile field to be present on the principal/entitlement resource — which
pebble's identity-only
V3EntitlementToV2hydration doesn't carry, soprovisioning silently failed for any resource synced that way.
This PR drops that requirement. The href to write is now resolved via
clmPreferredHref, in priority order:member's current groups, another folder-security entry)
client.GroupHref/MemberHref, derived from the discovered CLM base URLRevoke compares by bare ID instead, so it no longer needs a href at all.
Also includes several rounds of hardening from automated review: empty-ID
guards on every Grant/Revoke path (principal, folder, member/group), rejecting
malformed sample hrefs (trailing-slash/collection-root, bare scheme+host, no
path segment), and consistent
codes.InvalidArgumentclassification on allthe new validation errors.
Useful links: