[CXH-2281] - Fix SCIM account-lifecycle defects and rebuild test-server for doc fidelity by sergiocorral-conductorone · Pull Request #60 · ConductorOne/baton-lucidchart

sergiocorral-conductorone · 2026-08-20T16:16:16Z

Fixes CXH-2281.

Fixes four defects on the SCIM account-lifecycle paths added by CXH-1488, and rebuilds the bundled test-server from Lucid's published OpenAPI so the mock reproduces the real contract instead of the connector's own assumptions. The test-server is the repro harness that the sibling tickets CXH-2282–2285 depend on, so getting it right is the point of this ticket.

Prior art

This builds directly on QA's reference PR #59 ([CXH-2281] fix: SCIM account-lifecycle defects and test-server fidelity), which diagnosed all four defects and wrote the first test-server rewrite. I treated #59 as a verified starting hypothesis rather than porting it as-is: its CI review left 1 blocking issue and 3 suggestions open, and it carried unrelated dependency/vendor churn from an older base. This branch keeps #59's sound approach and closes the review.

Defects fixed

Delete aborted on Lucid's documented 403. GET /v1/users/{id} answers 403 — never 404 — for a user that does not exist (docs), so the not-found guard failed and the SCIM delete was never attempted; platform retries of an already-processed deprovision could not converge. Delete now disambiguates via a SCIM GET /Users/{id} (which 404s specifically for absence) and refuses only when the user still exists and their content could not be transferred — deleting then would destroy documents the operator asked to retain.
SCIM delete 409 surfaced as AlreadyExists. Lucid returns 409 for a user that can never be deleted, e.g. an account owner (docs). uhttp maps that to codes.AlreadyExists — the code the SDK uses for idempotent no-ops — so a refused delete read as success. Now FailedPrecondition, carrying Lucid's reason.
User status was pinned to STATUS_ENABLED. client.User now decodes Lucid's enabled field and the status derives from it; roles are emitted in the profile so an update_user role change is observable too.
client.User read usernames (plural). Lucid emits username (singular), so the field was empty on every user. Corrected.

Note — profile key rename usernames → username: the emitted user-resource profile key was renamed from usernames (plural) to username (singular) to match Lucid's actual field, not aliased alongside the old key. The plural key is dropped entirely rather than kept as an alias. This is low risk — Lucid never populated the plural field in prod, so it was always empty — but calling it out here so the removal doesn't read as an accidental drop.

What changed vs PR #59 (and why)

[was blocking in [CXH-2281] fix: SCIM account-lifecycle defects and test-server fidelity #59] Enabled is *bool, and userResource fails safe to ENABLED. [CXH-2281] fix: SCIM account-lifecycle defects and test-server fidelity #59 used a non-pointer bool and defaulted the status to DISABLED, so any GET /users payload missing enabled would have synced every user as deactivated — a mass-deactivation footgun the CI reviewer flagged. A pointer distinguishes "absent" from "explicit false"; status flips to DISABLED only on enabled == false. Confirmed against Lucid's docs that GET /users (listusers) returns the same User schema as getuser — enabled + username are both present on the sync path — so the pointer is defence-in-depth at no cost.
Collapsed the duplicate IsConflictError / IsAlreadyExistsError into a single 409 predicate so they cannot drift.
Clamped test-server -page-size <= 0 to Lucid's 200. Unclamped it returned an empty page while still emitting a Link next-token at the same offset — infinite pagination for the connector under test.
Fixed the stale ci.yaml "any non-empty bearer" comment and wired -scim-token (from BATON_LUCID_SCIM_TOKEN) through the start-test-server composite action, so the mock's REST/SCIM tokens cannot silently diverge.
Excluded [CXH-2281] fix: SCIM account-lifecycle defects and test-server fidelity #59's unrelated churn: its go.mod/go.sum/vendor/**/.versions.yaml bumps (it was cut from 79714b3; this branch is based on the newer main 3420066, which already carries those) and its deletion of tickets/CXH-1488/*.md. Also rebased [CXH-2281] fix: SCIM account-lifecycle defects and test-server fidelity #59's userResource edits onto main's newer WithUserProfile/WithStatus form instead of reintroducing the removed userTraitStatusToResourceStatus helper.

Verification

Local, against the rebuilt mock:

Check	before (`main`)	after
Status of a disabled user (105)	`RESOURCE_STATUS_ENABLED`	`RESOURCE_STATUS_DISABLED`
`username` in profile	absent	populated
Delete retry after 403	exit 1, `PermissionDenied`	exit 0
Delete a protected user (409)	`AlreadyExists`	`FailedPrecondition`

Full connector sync against the mock exits 0 (5 users; Link-header pagination); reading the synced c1z back shows user 105 RESOURCE_STATUS_DISABLED, all others ENABLED.
curl against the mock confirms the documented contract: REST GET /v1/users/999 → 403, SCIM GET /Users/lucid-999 → 404, SCIM DELETE lucid-101 (protected) → 409, DELETE lucid-105 → 204, SCIM token rejected on REST routes → 401, -page-size 0 clamped to a single 200-record page.
pkg/connector/users_test.go: the seven delete/status/username regression tests (both 403 branches, the 409 path, happy path, no-transfer-email) plus a table-driven enabled mapping test asserting absent → ENABLED, false → DISABLED, true → ENABLED.
go build ./..., go vet ./..., gofmt -l, and go test ./... are all clean.

Not in scope (tracked separately, per #59)

CXH-2282 SCIM wire contract (Content-Type / PatchOp schemas URN — needs a live Enterprise tenant; modelled in the mock behind -strict-scim-doc, off by default) · CXH-2283 ungated capabilities · CXH-2284 remaining SCIM surface gaps · CXH-2285 folder/document grant idempotency.

🤖 Generated with Claude Code

… for doc fidelity Fixes four defects on the SCIM account-lifecycle paths added by CXH-1488, and rebuilds the bundled test-server from Lucid's published OpenAPI so the mock reproduces the real contract instead of the connector's assumptions. Findings fixed: 1. Delete aborted on Lucid's documented 403. GET /v1/users/{id} answers 403 (never 404) for an absent user, so the not-found guard failed and the SCIM delete was never attempted; platform retries could not converge. Delete now disambiguates via a SCIM GET (which 404s specifically for absence) and refuses only when the user still exists and their content could not be transferred. 2. SCIM delete 409 surfaced as AlreadyExists (the idempotent-success code). Now mapped to FailedPrecondition, carrying Lucid's reason. 3. User status was pinned to STATUS_ENABLED. client.User now decodes `enabled` and the status derives from it; roles are emitted in the profile so an update_user role change is observable. 4. client.User read `usernames` (plural); Lucid emits `username` (singular), so the field was empty on every user. Corrected. Deltas vs reference PR #59 (its CI review left 1 blocking issue + 3 suggestions): - BLOCKING: Enabled is a *bool (not bool) and userResource defaults to ENABLED, flipping to DISABLED only on an explicit enabled=false. A non-pointer bool could not tell an absent field from false, and PR #59 defaulted to DISABLED — a GET /users payload missing `enabled` would have synced every user as deactivated. - Collapsed the duplicate IsConflictError/IsAlreadyExistsError into one predicate. - Clamped test-server -page-size <= 0 to Lucid's 200 (unclamped it paginated forever). - Fixed the stale ci.yaml bearer comment and wired -scim-token through the start-test-server action so REST/SCIM tokens cannot silently diverge. Validated against the rebuilt mock: full sync exits 0, disabled user 105 reads back RESOURCE_STATUS_DISABLED, mock returns documented 403/404/409 + dual-token auth, page-size clamp holds. go build, go vet, gofmt and go test ./... are clean. Fixes CXH-2281 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

linear-code · 2026-08-20T16:16:19Z

CXH-2281

github-actions · 2026-08-20T16:25:55Z

Connector PR Review: [CXH-2281] - Fix SCIM account-lifecycle defects and rebuild test-server for doc fidelity

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 3420066d2dab.
Review mode: incremental since 5b1a22b5
View review run

Review Summary

The full PR diff was re-scanned for security and correctness (connector, client, test-server, CI workflow, docs; no go.mod/go.sum changes are in scope). The new commit 0d077cf is a pure refactor that closes the three open review nits: it extracts the 403 probe-failure switch into classifyProbeFailure (pkg/connector/users.go:391), replaces the hand-rolled retry-code check with a new client.IsRetryableError predicate (pkg/connector/client/helpers.go:38), and collapses the err == nil / IsNotFoundError cases in Delete (pkg/connector/users.go:287) — all three verified behavior-identical, and the IsRetryableError doc comment matches the vendored SDK (uhttp/wrapper.go:295 and retry/retry.go:61). Earlier feedback on the 409 error detail, the SCIM probe on an undocumented REST 404, the seedUsers ID-collision and seed cap, trait/resource status divergence, and the 408 to DeadlineExceeded classification is all present in the current tree. No new issues found.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

…rd ParseForm - users.go: revert rebase regression from deprecated WithUserProfile/WithStatus (UserTraitOption) back to WithResourceProfile/WithResourceStatus (ResourceOption), clearing staticcheck SA1019. Restores the userTraitStatusToResourceStatus helper main uses to map the trait status enum to the resource status enum. - test-server: wrap the OAuth token handler body with http.MaxBytesReader (1 MiB) before ParseForm to satisfy gosec. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…ID collision, trait status, docs - users.go: 409 delete branch now appends Lucid's actual err via (%v) so FailedPrecondition carries the real reason, matching the 403 sibling. - users.go: split transferContentBeforeDelete's REST-404 from REST-403. A definite 404 falls through to the SCIM delete without probing SCIM, so a probe outage (403/405/501/5xx) can no longer abort a valid delete; the SCIM probe stays strict only for the ambiguous 403 case. - users.go: sync the (deprecated) user trait status via WithStatus so a disabled user no longer reports Resource.Status=DISABLED alongside UserTrait.Status=ENABLED; users_test asserts the trait status too. - test-server: seedUsers advances nextID past the seeded range so POST /users cannot mint a colliding userId after -users N. - test-server: reword the startup banner and three doc comments to describe the mock's behaviour without narrating the now-fixed connector defect. - Remove ticket-brief.md (internal process artifact, no repo convention). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…403 delete path - test-server: cap POST /_test/users?count= at maxSeedUsers (100k) so a mistyped count returns 400 instead of OOMing the mock in makeslice. - docs/connector.mdx: document the 403 → SCIM-probe behaviour, the FailedPrecondition refusal when a user exists but their email is unreadable, and the 409 (account/document owner) refusal — the note previously covered only the REST-404 skip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The double-failure branch in transferContentBeforeDelete (REST 403 then a failing SCIM existence probe) was the only place wrapping two errors with %w in one fmt.Errorf, so its gRPC status code depended on errors.As DFS order rather than a deliberate choice. Return codes.Unknown explicitly to signal the genuinely-indeterminate state, keeping both underlying error details in the message text via %v. Add TestDelete_RestForbiddenAndScimProbeFails_AbortsWithoutDeleting covering the previously-untested path: delete aborts with codes.Unknown and SCIM DELETE is never reached. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Lucid's GET /v1/users/{id} documents only 200 and 403 (403 is its "does not exist" response); 404 is undocumented and of unknown meaning. The prior code short-circuited a 404 straight to the hard SCIM delete, so if a 404 ever occurred for a user who still exists, offboarding would destroy content the operator asked to retain. Now the 404 path probes SCIM (like the 403 path) and refuses only when SCIM affirmatively reports the user still present. Unlike the 403 path, a SCIM probe error is treated as "proceed" so a probe outage still cannot block an otherwise-valid delete, preserving the original motive. Add tests for 404+gone (proceed), 404+exists (refuse), and 404+probe-error (proceed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

… Unknown On the ambiguous REST 403 path, transferContentBeforeDelete previously collapsed every SCIM existence-probe failure into codes.Unknown. That hid two distinguishable cases from the platform: - A probe cancelled/timed-out mid-flight lost its context error, so errors.Is(err, context.Canceled/DeadlineExceeded) returned false downstream. - A transient probe failure (429/5xx, which the SDK maps to Unavailable/ResourceExhausted) looked non-retryable, discouraging the platform from re-attempting a deprovision that would likely succeed. Classify in priority order — cancellation (preserved via %w), then retryable code (preserved), then a deliberate codes.Unknown fallback for genuinely indeterminate failures. Update the 500-probe test (500 now maps to the retryable Unavailable) and add coverage for the retryable, indeterminate, and cancellation branches. Also refresh docs/connector.mdx to describe one disambiguation rule covering both the 403 and undocumented-404 ambiguous responses: probe SCIM, and only a failed probe falls through to the delete. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…t error errorlint (verify / lint) flagged the cancellation branch's fmt.Errorf for formatting the REST error with %v alongside the %w context error. Pass err.Error() as a string so only the context error is wrapped — this keeps errors.Is(context.Canceled/DeadlineExceeded) matching without pulling the REST PermissionDenied status into the error chain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…ourceExhausted The 403 SCIM-probe error switch checked codes.ResourceExhausted, which GrpcCodeFromHTTPStatus never produces for any HTTP status, and omitted codes.DeadlineExceeded, the code a real 408 probe timeout produces. A 408 therefore fell through to the non-retryable codes.Unknown branch — backwards from intent. - Add codes.DeadlineExceeded to the retryable check; remove the unreachable codes.ResourceExhausted (the only producer in the vendored SDK is the gRPC ratelimit interceptor, not the uhttp client path ScimUserExists uses — no speculative defensive branch). - Correct the function comment and docs/connector.mdx: 429/5xx surface as Unavailable, 408 as DeadlineExceeded (not ResourceExhausted). - Extend the transient-probe test with a 408 -> DeadlineExceeded case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…able Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

501 maps to codes.Unimplemented (not Unavailable), so the probe correctly does not retry it. Reword the comment so it no longer overclaims that all 5xx map to Unavailable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

FeliLucero1 · 2026-08-24T17:35:41Z

 	userTraitOptions := []rs.UserTraitOption{
 		rs.WithEmail(user.Email, true),
 		rs.WithUserLogin(user.Email),
+		// Keep the (deprecated) trait status in sync with the resource status.


Is it worth creating a ticket about this? @sergiocorral-conductorone @luisina-santos

for the engine team I mean

FeliLucero1 · 2026-08-24T17:38:01Z

+// ScimUserExists reports whether the user still exists, via SCIM GET /Users/{id}.
+// SCIM 404s specifically for absence, which disambiguates REST's overloaded 403.
+// A non-nil error means "unknown" — never treat it as "gone".
+func (c *LucidchartClient) ScimUserExists(ctx context.Context, userID string) (bool, error) {


FYI: worst-case Delete path is now 3 calls instead of 1-2

GetUser -> ScimUserExists -> ScimDeleteUser on the ambiguous 403/404 paths. Not a real scale concern since Delete runs per-account not in a sync loop, just flagging for awareness.

Thanks — agreed, no change needed. The worst-case 3-call Delete path is fine since Delete runs on-demand for deprovisioning and is not in a sync loop, so the extra probe calls do not compound.

FeliLucero1 · 2026-08-24T17:38:01Z

   If you also want C1 to transfer a deleted user's documents to another user before removing the account, add the `account.user.transfercontent` scope. Without it, Delete calls to `POST /v1/transferUserContent` will return 403.

-   Note: if the user's REST record is already gone when deletion runs (GetUser returns 404), the content transfer is skipped and the SCIM delete proceeds — this is intentional idempotent retry behavior.
+   Note: when the user's REST record can't be read at deletion time, C1 applies one disambiguation rule for both ambiguous responses. Lucid's `GET /v1/users/{id}` returns 403 for both "not permitted" and "does not exist", and can also return an undocumented 404; neither on its own proves the user is gone. So on either a 403 or a 404, C1 probes SCIM (which returns 404 specifically for absence) before deleting: if SCIM affirmatively reports the user is still present, C1 refuses the delete with a `FailedPrecondition` error rather than hard-deleting content it could not transfer — grant the `account.user:readonly` scope so the email can be read. Only a *failed* probe falls through to the delete, and even then the two paths differ: on a 404 a probe outage proceeds to the SCIM delete (idempotent retry, so an outage can't block an otherwise-valid delete), whereas on a 403 an unresolved probe blocks the delete and surfaces its cause — a cancelled sync keeps its context error, a transient probe failure returns a retryable code so the platform retries (429/5xx surface as `Unavailable`, a 408 timeout as `DeadlineExceeded`), and a genuinely indeterminate failure returns `Unknown`. A raw SCIM 409 (the user is an account owner or default document owner and cannot be deleted) also surfaces as `FailedPrecondition` carrying Lucid's reason.


FYI: README doesn't mention any of this

This paragraph (and the SCIM token / content-transfer flag in general) has no equivalent in README.md. Looks like it predates this PR though, not something to fix here — just a heads up for whenever someone does a README pass.

Thanks — acknowledged, leaving out of scope here. The README/docs gap predates this PR and is not something this change introduced.

mateoHernandez123

LGTM. The four SCIM lifecycle fixes look right — the *bool enabled fail-safe, the 403/404 SCIM-probe disambiguation, and the 409 → FailedPrecondition mapping. Confirmed the 409-predicate collapse is safe and the SCIM id prefix is applied on both delete and existence checks. Feli's comments are non-blocking nits.

Address PR #60 review nits: - Add IsRetryableError(err) to client/helpers.go (Unavailable + DeadlineExceeded, matching the SDK retry gate) and use it instead of inlining the status.Code() checks in the 403 probe path. - Pull the deeply-nested probe-failure classifier switch out of transferContentBeforeDelete into a small classifyProbeFailure helper. - Collapse the identical err==nil / IsNotFoundError cases in Delete into a single multi-value case. Behavior-preserving; go build/vet/test/gofmt all clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

github-actions

No blocking issues found.

sergiocorral-conductorone self-assigned this Aug 20, 2026

github-actions Bot reviewed Aug 20, 2026

View reviewed changes

Comment thread pkg/connector/users.go Outdated

github-actions Bot reviewed Aug 20, 2026

View reviewed changes

Comment thread pkg/connector/users.go Outdated

github-actions Bot reviewed Aug 20, 2026

View reviewed changes

Comment thread cmd/test-server/main.go

github-actions Bot reviewed Aug 20, 2026

View reviewed changes

Comment thread ticket-brief.md Outdated

This comment was marked as outdated.

Sign in to view

github-actions Bot reviewed Aug 20, 2026

View reviewed changes

Comment thread pkg/connector/users.go

github-actions Bot reviewed Aug 20, 2026

View reviewed changes

Comment thread cmd/test-server/main.go Outdated

This comment was marked as outdated.

Sign in to view