[CXH-2281] fix: SCIM account-lifecycle defects and test-server fidelity - #59
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| status := v2.UserTrait_Status_STATUS_DISABLED | ||
| if user.Enabled { | ||
| status = v2.UserTrait_Status_STATUS_ENABLED | ||
| } |
There was a problem hiding this comment.
🟠 Bug: client.User.Enabled is a non-pointer bool, so "field absent from the payload" and "user is disabled" are indistinguishable — and the default chosen here is the destructive one. The enabled/username evidence in the PR body cites reference/getuser (GET /v1/users/{id}), but every user in a sync comes from ListUser → GET /users, a different endpoint whose response shape is only verified against the mock this PR also wrote. If that list payload omits enabled, every user syncs as RESOURCE_STATUS_DISABLED, which reads downstream as a mass deactivation.
Suggest failing safe: make it Enabled *bool in models.go and treat nil as ENABLED (only an explicit false disables), plus a test asserting a payload with no enabled key maps to ENABLED. (Confidence: medium — the code-level ambiguity is certain; whether GET /users returns enabled is unverified.)
| // IsConflictError reports whether err represents an upstream 409. SCIM delete | ||
| // uses it for a user that can never be deleted (account owner, default document | ||
| // owner) — terminal, not an idempotent "already done". | ||
| func IsConflictError(err error) bool { | ||
| return status.Code(err) == codes.AlreadyExists | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: IsConflictError is byte-identical to IsAlreadyExistsError above (both status.Code(err) == codes.AlreadyExists), which is the correct mapping for a 409 — but two exported predicates with the same body and different doc comments will drift the moment someone edits one. IsAlreadyExistsError currently has no callers, so consider deleting it (or renaming it to IsConflictError) rather than keeping both.
| size := cfg.pageSize | ||
| if requested := r.URL.Query().Get("pageSize"); requested != "" { | ||
| if n, err := strconv.Atoi(requested); err == nil && n > 0 && n < size { | ||
| size = n | ||
| } | ||
| } | ||
|
|
||
| end := offset + size | ||
| if end > len(all) { | ||
| end = len(all) | ||
| } | ||
| page := all[offset:end] | ||
|
|
||
| if end < len(all) { | ||
| next := fmt.Sprintf("%s://%s/users?pageSize=%d&pageToken=%d", schemeOf(r), r.Host, size, end) | ||
| w.Header().Set("Link", fmt.Sprintf("<%s>; rel=\"next\"", next)) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: -page-size 0 (or a negative value) is accepted by the flag and never clamped, so size <= 0 gives end == offset — an empty page that still emits Link: <...pageToken=offset>; rel="next" with the same offset. A connector under test then paginates forever on identical tokens. The query-param path already guards with n > 0; add the same clamp to cfg.pageSize (e.g. if size <= 0 { size = lucidPageSize }) so the mock can't manufacture an infinite pagination loop.
Connector PR Review: [CXH-2281] fix: SCIM account-lifecycle defects and test-server fidelityBlocking Issues: 0 | Suggestions: 6 | Threads Resolved: 0 Review SummaryScanned the full PR diff (6 files, +1038/-137) for security and correctness: the Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
Four defects surfaced during CXH-1488 validation, each reproducible against a
test-server rebuilt from Lucid's published OpenAPI.
Delete aborted on Lucid's documented 403. GET /v1/users/{id} answers 403 --
never 404 -- for a user that does not exist, so the not-found guard failed and
the SCIM delete was never attempted, leaving platform retries unable to
converge. Delete now disambiguates via SCIM, which 404s specifically for
absence, and refuses only when the user still exists and their content could
not be transferred.
SCIM delete 409 surfaced as AlreadyExists, the code the SDK uses for idempotent
no-ops, so a refused delete of an account owner read as success. It is now
FailedPrecondition carrying Lucid's reason.
User status was pinned to STATUS_ENABLED, so a SCIM deactivation never appeared
in a sync. client.User now decodes Lucid's `enabled` field and the status
derives from it; roles are emitted in the profile so update_user is observable.
client.User decoded `usernames`, a key Lucid does not emit, so the field was
empty on every user. Corrected to `username`.
The test-server is rewritten from Lucid's published contract rather than the
connector's assumptions -- the previous version encoded the connector's own
behaviour and could not have caught the first two defects. It models the
documented 403, persists SCIM PATCH state, returns 404/409 on delete, emits
Lucid's error envelope, paginates via the Link header, and requires distinct
REST and SCIM bearers. Scenario flags reproduce each case; -strict-scim-doc
enforces Lucid's documented Content-Type and schemas URN, which the connector
does not currently send (tracked in CXH-2282).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
golangci-lint (gosec G120) flagged the test-server's unbounded r.ParseForm() on /oauth2/token; wrap the body with http.MaxBytesReader. client.User.Enabled was a non-pointer bool, so a GET /users payload that omits `enabled` was indistinguishable from an explicit `false` and got reported as disabled. Enabled is now *bool: nil maps to RESOURCE_STATUS_UNSPECIFIED instead of assuming a status, and the legacy `usernames` field is kept alongside `username` for existing consumers of the profile map. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
5266f08 to
edc65bb
Compare
| return status.Errorf(codes.FailedPrecondition, | ||
| "baton-lucidchart: user %s exists but their email could not be read for content transfer (%v); "+ | ||
| "refusing to delete and lose their documents — check that the OAuth token carries "+ | ||
| "account.user:readonly and that the user is on this account", | ||
| userID, err) |
There was a problem hiding this comment.
🟡 Suggestion: this tells the operator to check for account.user:readonly, a scope name that appears nowhere else in the repo — docs/connector.mdx:92 documents the provisioning scope as account.user and line 96 as account.user.transfercontent. An operator following this message will look for a scope that may not exist. Confidence: high (the string mismatch is verifiable in-repo); pick whichever name Lucid actually publishes and make the docs and this message agree.
| if _, err := c.doRequest(ctx, req, nil); err != nil { | ||
| if IsNotFoundError(err) { | ||
| return false, nil | ||
| } | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: treating SCIM 404 as "definitively gone" is load-bearing for the delete path, but cmd/test-server/main.go:719-723 quotes Lucid as documenting that "Deactivated users will not be included in the totalResults or the JSON payload of users returned" for the SCIM /Users collection. If the single-resource GET /Users/{id} filters the same way, then for a user who was deactivated first (the normal offboard order) and whose REST record 403s on a scope problem, transferContentBeforeDelete returns nil → ScimDeleteUser hard-deletes without transferring content, which is the exact data loss the 403 guard exists to prevent. Confidence: medium — the collection-level exclusion is documented, the single-GET behaviour is not. Worth confirming against a live Enterprise tenant (alongside CXH-2282) and noting the answer here.
| status := v2.Status_RESOURCE_STATUS_UNSPECIFIED | ||
| if user.Enabled != nil { | ||
| if *user.Enabled { | ||
| status = v2.Status_RESOURCE_STATUS_ENABLED | ||
| } else { | ||
| status = v2.Status_RESOURCE_STATUS_DISABLED | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: enabled is cited from reference/getuser (the single-user endpoint), but the field that drives every synced resource comes from ListUser → GET /users, and the A/B in the PR body was run against this repo's own mock, which was written to emit it. If Lucid's list response omits enabled, every user syncs RESOURCE_STATUS_UNSPECIFIED — a regression from the previous unconditional ENABLED, across the whole account rather than the one-user case. Same applies to the POST /users create response feeding CreateAccount. Confidence: medium. Worth confirming the list payload carries enabled before this ships.
| mux.HandleFunc("POST /_test/users", func(w http.ResponseWriter, r *http.Request) { | ||
| n, err := strconv.Atoi(r.URL.Query().Get("count")) | ||
| if err != nil || n < 0 { | ||
| http.Error(w, "count must be a non-negative integer", http.StatusBadRequest) | ||
| return | ||
| } | ||
| s.seedUsers(n) | ||
| log.Printf("[_test] reseeded with %d users", n) |
There was a problem hiding this comment.
🟡 Suggestion: count is only checked for n < 0, so POST /_test/users?count=100000000 drives seedUsers into allocating 100M structs and OOMs the process. The endpoint is unauthenticated and the server defaults to :8080 on all interfaces. Same class as the maxOAuthFormBytes bound added in the last commit — add an upper bound (e.g. reject n > 100_000) so a typo in a test script can't kill the mock mid-run.
| Email string `json:"email"` | ||
| Name string `json:"name"` | ||
| UserId int `json:"userId"` |
There was a problem hiding this comment.
🟡 Suggestion: if Lucid emits username (singular), then Usernames is now permanently empty — and users.go:213 still writes it into the profile, so every synced user carries a usernames: "" key alongside the real username. Dropping the field and the profile entry avoids shipping a value that is guaranteed to be wrong; keep it only if some tenant/API version is known to send the plural form.
There was a problem hiding this comment.
it is a potential breaking change
4 participants
Fixes four defects on the SCIM account-lifecycle paths added by CXH-1488, and rewrites the bundled test-server to replicate Lucid's published contract.
Linear: CXH-2281
Why the test-server changed first
The previous mock was written alongside the connector and encoded the connector's own assumptions — it returned 404 where Lucid documents 403, and answered SCIM PATCH with a hardcoded
active: truewithout persisting anything. Two of the four defects below are invisible against that mock and against the unit tests, both of which hand-wrote the wrong status code. Rebuilding the mock from Lucid's published OpenAPI is what surfaced them.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.Deletenow disambiguates via SCIM, which 404s specifically for absence, and refuses only when the user still exists and their content could not be transferred — deleting in that case 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, such as an account owner (docs).uhttpmaps that tocodes.AlreadyExists— the code the SDK uses for idempotent no-ops — so a refused delete read as success. NowFailedPrecondition, carrying Lucid's reason.User status was pinned to
STATUS_ENABLED. A SCIM deactivation never appeared in a sync.client.Usernow decodes Lucid'senabledfield and the status derives from it; roles are emitted in the profile soupdate_useris observable too.client.Userdecodedusernames. Lucid emitsusername(singular), so the field was empty on every user. Corrected.Verification
A/B against the same mock, connector built from
origin/mainvs this branch:RESOURCE_STATUS_ENABLEDRESOURCE_STATUS_DISABLEDusernamein profilePermissionDeniedAlreadyExistsFailedPreconditionMock-side trace for the retry case, before and after:
Seven regression tests added in
pkg/connector/users_test.gocovering both 403 branches, the 409 path, the happy path, and the status/username mapping.go build,go vet,golangci-lint runandgo test ./pkg/...are clean.Test-server changes
Rebuilt from Lucid's published OpenAPI, with the doc URL on every handler. Models the documented 403; persists SCIM PATCH state and echoes it back; returns 404/409 on delete; emits Lucid's
{code, message, requestId}error envelope; paginates via theLinkheader at Lucid's 200-record page size; requires distinct REST and SCIM bearers; and hardens the OAuth token endpoint against badgrant_typeand missing credentials. Seed data now includes a disabled user, a user with no roles, and a unicode display name.Scenario flags reproduce each case:
-legacy-user-404,-protected-users,-users N,-transfer-rate-limit,-strict-scim-doc.-strict-scim-docenforces Lucid's documented Content-Type and schemas URN, which the connector does not currently send. That divergence is deliberately not changed here — it needs one call against an Enterprise tenant to settle, and is tracked in CXH-2282.Not in scope
CXH-2282 SCIM wire contract · CXH-2283 ungated capabilities · CXH-2284 remaining SCIM surface gaps · CXH-2285 folder/document grant idempotency
🤖 Generated with Claude Code