From 657732e4318dd95377c1ea57531b8a9f8f43d9be Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Thu, 20 Aug 2026 13:15:39 -0300 Subject: [PATCH 01/12] [CXH-2281] Fix SCIM account-lifecycle defects and rebuild test-server for doc fidelity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .github/actions/start-test-server/action.yml | 6 +- .github/workflows/ci.yaml | 7 +- cmd/test-server/main.go | 846 +++++++++++++++++-- pkg/connector/client/helpers.go | 17 +- pkg/connector/client/models.go | 20 +- pkg/connector/client/scim.go | 23 + pkg/connector/users.go | 116 ++- pkg/connector/users_test.go | 202 ++++- ticket-brief.md | 95 +++ 9 files changed, 1181 insertions(+), 151 deletions(-) create mode 100644 ticket-brief.md diff --git a/.github/actions/start-test-server/action.yml b/.github/actions/start-test-server/action.yml index 9de1c8f8..a39319cc 100644 --- a/.github/actions/start-test-server/action.yml +++ b/.github/actions/start-test-server/action.yml @@ -15,7 +15,11 @@ runs: run: go build -o test-server ./cmd/test-server shell: bash - name: Start test server - run: ./test-server & + # Pass the SCIM bearer through so the mock's -scim-token stays in lockstep + # with BATON_LUCID_SCIM_TOKEN. The mock requires distinct REST/SCIM tokens; + # if these diverge, every SCIM route 401s. Falls back to the mock's own + # default when the var is unset (e.g. a fork without repo secrets). + run: ./test-server -scim-token "${BATON_LUCID_SCIM_TOKEN:-test-scim-token}" & shell: bash - name: Wait for test server run: | diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index c10d0c47..c4666983 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -9,8 +9,11 @@ env: BATON_LOG_LEVEL: debug # Credentials + endpoints for the in-process mock (cmd/test-server). No real # Lucid tenant, OAuth credentials, or rotating refresh token are involved: the - # mock serves the OAuth2 token endpoint, the REST API and SCIM. Any non-empty - # bearer is accepted. The connector derives the OAuth token URL from BATON_BASE_URL. + # mock serves the OAuth2 token endpoint, the REST API and SCIM. As in + # production, the REST and SCIM surfaces require DIFFERENT bearer tokens: the + # mock rejects the SCIM token on REST routes and enforces BATON_LUCID_SCIM_TOKEN + # on SCIM routes, so it is passed through to the mock's -scim-token by the + # start-test-server action. The connector derives the OAuth token URL from BATON_BASE_URL. BATON_LUCID_API_KEY: test-api-key BATON_LUCID_CLIENT_ID: test-client-id BATON_LUCID_CLIENT_SECRET: test-client-secret diff --git a/cmd/test-server/main.go b/cmd/test-server/main.go index e6a452fb..354c9a99 100644 --- a/cmd/test-server/main.go +++ b/cmd/test-server/main.go @@ -8,11 +8,46 @@ // // --base-url http://localhost:8080 // --scim-base-url http://localhost:8080/scim/v2 -// --lucid-scim-token test-scim-token (any non-empty value enables SCIM) +// --lucid-scim-token test-scim-token // // The OAuth2 token endpoint is derived from --base-url by the connector, so the // refresh_token grant is served here too — no real (rotating) token is involved. -// Any non-empty bearer is accepted on authenticated routes. +// +// # Fidelity +// +// This mock is written from Lucid's PUBLISHED API contract, not from the +// connector's client code. That distinction is the whole point: a mock that +// mirrors the connector reproduces its bugs and turns the test suite into a +// fiction. Where the connector and the documented contract disagree, this server +// follows the documentation and the connector fails — which is the correct +// outcome, because the live API would fail the same way. +// +// Behaviours deliberately modelled from the docs (with the doc citation): +// +// - GET /v1/users/{id} returns 403 for a user that does not exist, never 404 +// ("if the user does not belong to the authenticated account or if the user +// does not exist" — reference/getuser). Use -legacy-user-404 to restore the +// permissive 404 for an A/B comparison. +// - SCIM PATCH actually applies and persists the requested `active` value and +// profile operations, and echoes the real state back (reference/modifyuserpatch). +// - SCIM DELETE returns 404 for an unknown user and 409 for a protected one +// ("if the user cannot be deleted (e.g., account owner or default document +// owner)" — reference/deleteuser). +// - The REST user model exposes `username` (singular) and `enabled` +// (reference/getuser), not the `usernames` field the connector currently reads. +// - SCIM Content-Type and the PatchOp `schemas` URN are ACCEPTED in both the +// RFC 7644 form the connector sends and the form Lucid's spec documents, with +// a DIVERGENCE line logged for the former. Lucid declares application/json +// and a core-User schemas example; "application/scim+json" and the PatchOp +// URN appear in zero Lucid doc pages. The docs and the RFC genuinely +// disagree, so failing by default would over-claim — use -strict-scim-doc to +// enforce Lucid's documented shape and watch the connector fail. +// - Errors use Lucid's envelope {code, message, requestId} (reference-rest). +// - GET /users paginates via an opaque pageToken carried in the Link header, +// 200 records per page (reference-rest). +// - REST and SCIM require DIFFERENT bearer tokens, as they do in production. +// +// Run with -h to see the scenario flags. package main import ( @@ -23,37 +58,167 @@ import ( "log" "net/http" "os" + "strconv" "strings" "sync" + "sync/atomic" "time" ) -// user mirrors client.User (pkg/connector/client/models.go). +const ( + // scimIDPrefix is the prefix Lucid puts on SCIM resource IDs: the SCIM id for + // REST user 1234 is "lucid-1234" (reference/overview-scim, User.id). + scimIDPrefix = "lucid-" + + // lucidPageSize is Lucid's page size for paginated REST endpoints. The docs + // state a 200-record default and that a larger requested pageSize is clamped + // to 200 (reference-rest). + lucidPageSize = 200 + + // scimContentType is what RFC 7644 mandates and what the connector sends. + // docContentType is what Lucid's OpenAPI actually declares for every SCIM + // operation — "application/scim+json" appears in ZERO Lucid doc pages. + // Both are accepted by default because the docs and the RFC disagree and we + // cannot resolve it without a live Enterprise tenant; -strict-scim-doc + // enforces the documented shape so the divergence can be demonstrated. + scimContentType = "application/scim+json" + docContentType = "application/json" + + // scimPatchOpSchema is the RFC 7644 PatchOp URN the connector sends. + // docPatchSchema is the value Lucid's PATCH requestBody gives as its example + // — the PatchOp URN appears in ZERO Lucid doc pages. + scimPatchOpSchema = "urn:ietf:params:scim:api:messages:2.0:PatchOp" + docPatchSchema = "urn:ietf:params:scim:schemas:core:2.0:User" + + scimListSchema = "urn:ietf:params:scim:api:messages:2.0:ListResponse" +) + +// config carries the scenario switches. Defaults are the documented behaviour; +// every flag exists to reproduce a specific test case from the CXH-1488 plan. +type config struct { + legacyUser404 bool + protectedUsers map[int]bool + transferLimit bool + pageSize int + scimToken string + // strictSCIMDoc rejects SCIM requests that follow RFC 7644 where Lucid's own + // spec documents something different (Content-Type, PatchOp schemas URN). + // Off by default: the docs and the RFC genuinely disagree and only a live + // Enterprise tenant can settle it, so failing by default would over-claim. + strictSCIMDoc bool +} + +// user mirrors Lucid's REST User model (reference/getuser). +// +// NOTE the field names. Lucid documents `username` (singular) and `enabled`. +// The connector's client.User reads `usernames` (plural) and has no `enabled` +// field at all, so both will come through empty/absent on the connector side. +// That is a real connector defect surfaced by this mock, not a mock bug — do not +// "fix" it here by renaming these fields to match the connector. type user struct { AccountId int `json:"accountId"` Email string `json:"email"` Name string `json:"name"` UserId int `json:"userId"` - Usernames string `json:"usernames"` + Username string `json:"username"` + Enabled bool `json:"enabled"` Roles []string `json:"roles"` } +// scimName is the SCIM 2.0 complex name attribute. +type scimName struct { + GivenName string `json:"givenName,omitempty"` + FamilyName string `json:"familyName,omitempty"` +} + +// scimEmail is a SCIM 2.0 multi-valued email entry. +type scimEmail struct { + Value string `json:"value"` + Type string `json:"type,omitempty"` + Primary bool `json:"primary"` +} + +// scimUser is the SCIM representation returned by PATCH and GET /Users. +type scimUser struct { + Schemas []string `json:"schemas"` + ID string `json:"id"` + UserName string `json:"userName"` + Name scimName `json:"name"` + Emails []scimEmail `json:"emails"` + Active bool `json:"active"` + Roles []scimRole `json:"roles,omitempty"` +} + +type scimRole struct { + Value string `json:"value"` +} + +// scimPatchOp is the request body for PATCH /Users/{id}. +type scimPatchOp struct { + Schemas []string `json:"schemas"` + Operations []scimPatchOpEntry `json:"Operations"` +} + +type scimPatchOpEntry struct { + Op string `json:"op"` + Path string `json:"path"` + Value interface{} `json:"value"` +} + +// lucidError is Lucid's documented error envelope (reference-rest). +type lucidError struct { + Code string `json:"code"` + Message string `json:"message"` + RequestId string `json:"requestId"` +} + type store struct { - mu sync.Mutex - users []user - nextID int + mu sync.Mutex + users []user + nextID int +} + +var requestCounter atomic.Int64 + +func nextRequestID() string { + return fmt.Sprintf("req-%06d", requestCounter.Add(1)) } func newStore() *store { return &store{ users: []user{ - {AccountId: 1, Email: "owner@example.com", Name: "Olivia Owner", UserId: 101, Usernames: "owner@example.com", Roles: []string{"admin"}}, - {AccountId: 1, Email: "editor@example.com", Name: "Eddie Editor", UserId: 102, Usernames: "editor@example.com", Roles: []string{"member"}}, + {AccountId: 1, Email: "owner@example.com", Name: "Olivia Owner", UserId: 101, Username: "owner@example.com", Enabled: true, Roles: []string{"account-owner"}}, + {AccountId: 1, Email: "editor@example.com", Name: "Eddie Editor", UserId: 102, Username: "editor@example.com", Enabled: true, Roles: []string{"document-admin"}}, + // Deliberate diversification beyond the dev's happy path: a user with + // no roles, a unicode display name, and a user that starts disabled so + // a sync can be checked for whether it reports state at all. + {AccountId: 1, Email: "no-roles@example.com", Name: "Nora NoRoles", UserId: 103, Username: "no-roles@example.com", Enabled: true, Roles: nil}, + {AccountId: 1, Email: "unicode@example.com", Name: "Zoë Ünicode-Ñame 🎨", UserId: 104, Username: "unicode@example.com", Enabled: true, Roles: []string{"team-admin"}}, + {AccountId: 1, Email: "disabled@example.com", Name: "Dana Disabled", UserId: 105, Username: "disabled@example.com", Enabled: false, Roles: []string{"developer"}}, }, nextID: 1000, } } +// seedUsers replaces the user set with exactly n generated users. Used by +// -users N and POST /_test/users to hit pagination boundaries (PAG-03). +func (s *store) seedUsers(n int) { + s.mu.Lock() + defer s.mu.Unlock() + s.users = make([]user, 0, n) + for i := 0; i < n; i++ { + id := 1 + i + s.users = append(s.users, user{ + AccountId: 1, + Email: fmt.Sprintf("user%d@example.com", id), + Name: fmt.Sprintf("User %d", id), + UserId: id, + Username: fmt.Sprintf("user%d@example.com", id), + Enabled: true, + }) + } +} + func (s *store) listUsers() []user { s.mu.Lock() defer s.mu.Unlock() @@ -74,32 +239,48 @@ func (s *store) addUser(u user) user { return u } -// getUserByID returns the user with the given numeric UserId string, or false -// if not found. Used by GET /v1/users/{id}. +// getUserByID returns the user with the given numeric UserId string. func (s *store) getUserByID(id string) (user, bool) { s.mu.Lock() defer s.mu.Unlock() for _, u := range s.users { - if fmt.Sprintf("%d", u.UserId) == id { + if strconv.Itoa(u.UserId) == id { return u, true } } return user{}, false } -// deleteUserByScimID removes the user whose UserId matches the numeric suffix -// of a SCIM resource ID (e.g. "lucid-101" → removes UserId=101). -// Returns true if a user was found and removed. -func (s *store) deleteUserByScimID(scimID string) bool { - const prefix = "lucid-" - if !strings.HasPrefix(scimID, prefix) { - return false +// updateUser applies mutate to the stored user with the given numeric id. +func (s *store) updateUser(id int, mutate func(*user)) (user, bool) { + s.mu.Lock() + defer s.mu.Unlock() + for i := range s.users { + if s.users[i].UserId == id { + mutate(&s.users[i]) + return s.users[i], true + } } - rawID := scimID[len(prefix):] + return user{}, false +} + +func (s *store) userExistsByEmail(email string) bool { + s.mu.Lock() + defer s.mu.Unlock() + for _, u := range s.users { + if strings.EqualFold(u.Email, email) { + return true + } + } + return false +} + +// deleteUserByID removes the user with the given numeric id. +func (s *store) deleteUserByID(id int) bool { s.mu.Lock() defer s.mu.Unlock() for i, u := range s.users { - if fmt.Sprintf("%d", u.UserId) == rawID { + if u.UserId == id { s.users = append(s.users[:i], s.users[i+1:]...) return true } @@ -107,35 +288,182 @@ func (s *store) deleteUserByScimID(scimID string) bool { return false } +// parseScimID converts a SCIM resource ID ("lucid-101") to its numeric REST id. +// A SCIM ID without the documented prefix is not a valid Lucid SCIM ID. +func parseScimID(scimID string) (int, bool) { + if !strings.HasPrefix(scimID, scimIDPrefix) { + return 0, false + } + id, err := strconv.Atoi(strings.TrimPrefix(scimID, scimIDPrefix)) + if err != nil { + return 0, false + } + return id, true +} + +func toScimUser(u user) scimUser { + roles := make([]scimRole, 0, len(u.Roles)) + for _, r := range u.Roles { + roles = append(roles, scimRole{Value: r}) + } + parts := strings.SplitN(u.Name, " ", 2) + given := parts[0] + family := "" + if len(parts) > 1 { + family = parts[1] + } + return scimUser{ + Schemas: []string{"urn:ietf:params:scim:schemas:core:2.0:User"}, + ID: scimIDPrefix + strconv.Itoa(u.UserId), + UserName: u.Username, + Name: scimName{GivenName: given, FamilyName: family}, + Emails: []scimEmail{{Value: u.Email, Type: "work", Primary: true}}, + Active: u.Enabled, + Roles: roles, + } +} + func writeJSON(w http.ResponseWriter, code int, v interface{}) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(code) _ = json.NewEncoder(w).Encode(v) } -// requireBearer accepts any non-empty bearer token. The mock does not validate -// credentials — it only checks the connector wired auth through. -func requireBearer(w http.ResponseWriter, r *http.Request) bool { - if !strings.HasPrefix(r.Header.Get("Authorization"), "Bearer ") { - http.Error(w, "missing bearer token", http.StatusUnauthorized) - return false - } - return true +func writeSCIMJSON(w http.ResponseWriter, code int, v interface{}) { + w.Header().Set("Content-Type", scimContentType) + w.WriteHeader(code) + _ = json.NewEncoder(w).Encode(v) +} + +// writeOAuthError emits the RFC 6749 token-endpoint error shape. +func writeOAuthError(w http.ResponseWriter, status int, code, description string) { + log.Printf("-> %d oauth %s: %s", status, code, description) //nolint:gosec // test-server: message is diagnostic only + writeJSON(w, status, map[string]string{ + "error": code, + "error_description": description, + }) +} + +// writeLucidError emits Lucid's documented error envelope. +func writeLucidError(w http.ResponseWriter, status int, code, message string) { + rid := nextRequestID() + log.Printf("-> %d %s: %s (requestId=%s)", status, code, message, rid) //nolint:gosec // test-server: message is diagnostic only + writeJSON(w, status, lucidError{Code: code, Message: message, RequestId: rid}) } -func newMux(s *store) *http.ServeMux { +func newMux(s *store, cfg config) *http.ServeMux { mux := http.NewServeMux() + // requireToken checks for the specific bearer the surface expects. REST and + // SCIM use different tokens in production; accepting either here would hide a + // connector that sends the wrong one. + requireToken := func(want, surface string) func(http.ResponseWriter, *http.Request) bool { + return func(w http.ResponseWriter, r *http.Request) bool { + auth := r.Header.Get("Authorization") + if !strings.HasPrefix(auth, "Bearer ") { + writeLucidError(w, http.StatusUnauthorized, "unauthorized", "missing bearer token") + return false + } + got := strings.TrimPrefix(auth, "Bearer ") + if want != "" && got != want { + writeLucidError(w, http.StatusUnauthorized, "unauthorized", + fmt.Sprintf("%s surface received the wrong bearer token", surface)) + return false + } + return true + } + } + // The REST surface accepts both the OAuth access token this server mints and + // the configured API key, because the connector legitimately uses each for a + // different route family. + requireRest := func(w http.ResponseWriter, r *http.Request) bool { + auth := r.Header.Get("Authorization") + if !strings.HasPrefix(auth, "Bearer ") { + writeLucidError(w, http.StatusUnauthorized, "unauthorized", "missing bearer token") + return false + } + got := strings.TrimPrefix(auth, "Bearer ") + if got == cfg.scimToken { + writeLucidError(w, http.StatusUnauthorized, "unauthorized", + "REST surface received the SCIM bearer token") + return false + } + return true + } + requireScim := requireToken(cfg.scimToken, "SCIM") + // Health check (unauthenticated) — the CI start-test-server action polls this. mux.HandleFunc("GET /health", func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte("ok")) }) - // OAuth2 token endpoint (unauthenticated). Serves the refresh_token grant the - // connector performs at startup; returns a static access token so no real - // (rotating) refresh token is needed. - mux.HandleFunc("POST /oauth2/token", func(w http.ResponseWriter, _ *http.Request) { + // Test-control endpoints (unauthenticated, prefixed /_test/). Not part of the + // Lucid contract — they exist so a test case can reshape the dataset between + // runs without restarting the process. + 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) + writeJSON(w, http.StatusOK, map[string]int{"users": n}) + }) + mux.HandleFunc("POST /_test/reset", func(w http.ResponseWriter, _ *http.Request) { + fresh := newStore() + s.mu.Lock() + s.users, s.nextID = fresh.users, fresh.nextID + s.mu.Unlock() + log.Printf("[_test] store reset to seed fixtures") + writeJSON(w, http.StatusOK, map[string]string{"status": "reset"}) + }) + + // POST /oauth2/token — https://lucid.readme.io/reference/createorrefreshaccesstoken + // Serves the refresh_token grant the connector performs at startup, and + // rejects what the real endpoint rejects: a permissive token endpoint has + // shipped broken grant_type params to production before. + mux.HandleFunc("POST /oauth2/token", func(w http.ResponseWriter, r *http.Request) { + if err := r.ParseForm(); err != nil { + writeOAuthError(w, http.StatusBadRequest, "invalid_request", "malformed form body") + return + } + grantType := r.Form.Get("grant_type") + clientID := r.Form.Get("client_id") + clientSecret := r.Form.Get("client_secret") + if clientID == "" || clientSecret == "" { + // The Go OAuth2 library may send credentials via Basic auth instead. + if u, p, ok := r.BasicAuth(); ok { + clientID, clientSecret = u, p + } + } + + switch grantType { + case "refresh_token": + if r.Form.Get("refresh_token") == "" { + writeOAuthError(w, http.StatusBadRequest, "invalid_request", "refresh_token is required") + return + } + case "authorization_code": + if r.Form.Get("code") == "" || r.Form.Get("redirect_uri") == "" { + writeOAuthError(w, http.StatusBadRequest, "invalid_request", "code and redirect_uri are required") + return + } + case "": + writeOAuthError(w, http.StatusBadRequest, "invalid_request", "grant_type is required") + return + default: + writeOAuthError(w, http.StatusBadRequest, "unsupported_grant_type", + fmt.Sprintf("unsupported grant_type %q", grantType)) + return + } + + if clientID == "" || clientSecret == "" { + writeOAuthError(w, http.StatusBadRequest, "invalid_request", "client_id and client_secret are required") + return + } + writeJSON(w, http.StatusOK, map[string]interface{}{ "access_token": "mock-access-token", "token_type": "Bearer", @@ -144,64 +472,128 @@ func newMux(s *store) *http.ServeMux { }) }) - // REST: list users (OAuth2). Single page (no Link header). + // GET /users — https://lucid.readme.io/reference/listusers + // Paginated exactly as Lucid documents: an opaque + // pageToken carried ONLY in the Link header, default/max 200 per page. mux.HandleFunc("GET /users", func(w http.ResponseWriter, r *http.Request) { - if !requireBearer(w, r) { + if !requireRest(w, r) { return } - writeJSON(w, http.StatusOK, s.listUsers()) + all := s.listUsers() + + offset := 0 + if tok := r.URL.Query().Get("pageToken"); tok != "" { + parsed, err := strconv.Atoi(tok) + if err != nil || parsed < 0 || parsed > len(all) { + writeLucidError(w, http.StatusBadRequest, "badRequest", "invalid pageToken") + return + } + offset = parsed + } + + 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)) + } + log.Printf("GET /users offset=%d returned=%d total=%d hasNext=%v", offset, len(page), len(all), end < len(all)) + writeJSON(w, http.StatusOK, page) }) - // REST: get single user by ID (GET /v1/users/{id}). Used by the connector - // to resolve a user's email address before calling transferUserContent. + // GET /v1/users/{id} — https://lucid.readme.io/reference/getuser + // + // Lucid documents 403 — NOT 404 — for a user that does not exist: + // "if the user does not belong to the authenticated account or if the user + // does not exist" (reference/getuser). The connector's Delete path treats + // only 404 as "already deleted", so against the documented behaviour its + // idempotent-retry aborts. -legacy-user-404 restores the permissive 404 so a + // test can A/B the two. mux.HandleFunc("GET /v1/users/{id}", func(w http.ResponseWriter, r *http.Request) { - if !requireBearer(w, r) { + if !requireRest(w, r) { return } id := r.PathValue("id") u, ok := s.getUserByID(id) if !ok { - log.Printf("GET /v1/users/%s — not found", id) //nolint:gosec // test-server: path value is diagnostic only - http.NotFound(w, r) + if cfg.legacyUser404 { + log.Printf("GET /v1/users/%s — not found (LEGACY 404 mode)", id) //nolint:gosec // test-server: path value is diagnostic only + writeLucidError(w, http.StatusNotFound, "notFound", "user not found") + return + } + log.Printf("GET /v1/users/%s — absent; returning documented 403", id) //nolint:gosec // test-server: path value is diagnostic only + writeLucidError(w, http.StatusForbidden, "accessForbidden", + "the user does not belong to the authenticated account or does not exist") return } log.Printf("GET /v1/users/%s → email=%s", id, u.Email) //nolint:gosec // test-server: path value is diagnostic only writeJSON(w, http.StatusOK, u) }) - // REST: create user (POST /users) — parses request body and persists the - // new user so the next GET /users sync can find it. This is required for - // baton-test's provisioning flow, which creates then re-syncs to verify. + // POST /users — https://lucid.readme.io/reference/createuser + // Lucid documents 201 (not 200), 409 on a duplicate email, + // and 400 when a required field is missing (reference/createuser). mux.HandleFunc("POST /users", func(w http.ResponseWriter, r *http.Request) { - if !requireBearer(w, r) { + if !requireRest(w, r) { return } var payload struct { - FirstName string `json:"firstName"` - LastName string `json:"lastName"` - Email string `json:"email"` - Username string `json:"username"` + FirstName string `json:"firstName"` + LastName string `json:"lastName"` + Email string `json:"email"` + Username string `json:"username"` + Roles []string `json:"roles"` } if err := json.NewDecoder(r.Body).Decode(&payload); err != nil { - http.Error(w, "bad request", http.StatusBadRequest) + writeLucidError(w, http.StatusBadRequest, "badRequest", "malformed request body") + return + } + if payload.Email == "" || payload.FirstName == "" || payload.LastName == "" { + writeLucidError(w, http.StatusBadRequest, "badRequest", + "email, firstName and lastName are required") + return + } + if s.userExistsByEmail(payload.Email) { + writeLucidError(w, http.StatusConflict, "alreadyExists", + "a user with the same email or username already exists") return } - name := payload.FirstName + " " + payload.LastName username := payload.Username if username == "" { username = payload.Email } - u := s.addUser(user{AccountId: 1, Email: payload.Email, Name: name, Usernames: username}) - log.Printf("POST /users created userId=%d email=%s", u.UserId, u.Email) - writeJSON(w, http.StatusOK, u) + u := s.addUser(user{ + AccountId: 1, + Email: payload.Email, + Name: payload.FirstName + " " + payload.LastName, + Username: username, + Enabled: true, + Roles: payload.Roles, + }) + log.Printf("POST /users created userId=%d email=%s roles=%v", u.UserId, u.Email, u.Roles) + writeJSON(w, http.StatusCreated, u) }) - // REST: content transfer before delete. The Lucid API requires email - // addresses for fromUser and toUser ("Email of the user whose content will - // be transferred"). Returns 400 if either field is missing '@' — this - // catches the pre-fix bug where numeric IDs were passed instead of emails. + // POST /v1/transferUserContent — https://lucid.readme.io/reference/transferusercontent + // Lucid requires EMAIL addresses for + // both fields, documents 400 when they are the same user, 403 when either + // user does not exist, and a 30-request/5-second per-account rate limit + // (reference/transferusercontent). + var transferTimes []time.Time + var transferMu sync.Mutex mux.HandleFunc("POST /v1/transferUserContent", func(w http.ResponseWriter, r *http.Request) { - if !requireBearer(w, r) { + if !requireRest(w, r) { return } var body struct { @@ -209,23 +601,57 @@ func newMux(s *store) *http.ServeMux { ToUser string `json:"toUser"` } if err := json.NewDecoder(r.Body).Decode(&body); err != nil { - http.Error(w, "bad request body", http.StatusBadRequest) + writeLucidError(w, http.StatusBadRequest, "badRequest", "malformed request body") return } log.Printf("POST /v1/transferUserContent fromUser=%s toUser=%s", body.FromUser, body.ToUser) + + if cfg.transferLimit { + transferMu.Lock() + now := time.Now() + kept := transferTimes[:0] + for _, t := range transferTimes { + if now.Sub(t) < 5*time.Second { + kept = append(kept, t) + } + } + transferTimes = kept + over := len(transferTimes) >= 30 + if !over { + transferTimes = append(transferTimes, now) + } + transferMu.Unlock() + if over { + w.Header().Set("Retry-After", "5") + writeLucidError(w, http.StatusTooManyRequests, "tooManyRequests", + "You have sent too many requests in a given amount of time.") + return + } + } + + // The API requires emails. A numeric ID here is the pre-fix connector bug. if !strings.Contains(body.FromUser, "@") || !strings.Contains(body.ToUser, "@") { - msg := fmt.Sprintf("fromUser and toUser must be email addresses, got fromUser=%q toUser=%q", body.FromUser, body.ToUser) - log.Printf("POST /v1/transferUserContent 400: %s", msg) - http.Error(w, msg, http.StatusBadRequest) + writeLucidError(w, http.StatusBadRequest, "badRequest", + fmt.Sprintf("fromUser and toUser must be email addresses, got fromUser=%q toUser=%q", body.FromUser, body.ToUser)) return } - w.WriteHeader(http.StatusOK) + if strings.EqualFold(body.FromUser, body.ToUser) { + writeLucidError(w, http.StatusBadRequest, "badRequest", "fromUser and toUser must differ") + return + } + if !s.userExistsByEmail(body.FromUser) || !s.userExistsByEmail(body.ToUser) { + writeLucidError(w, http.StatusForbidden, "accessForbidden", + "the users do not exist or are not on the authenticated account") + return + } + w.WriteHeader(http.StatusNoContent) }) - // REST: folder content — empty so the sync completes with no folders/documents - // (and therefore no collaborator grants to enumerate). API-key auth. + // GET /folders/root/contents — https://lucid.readme.io/reference/listrootfoldercontents + // GET /folders/{id}/contents — https://lucid.readme.io/reference/listfoldercontents + // Empty so the sync completes with no folders/documents. emptyContents := func(w http.ResponseWriter, r *http.Request) { - if !requireBearer(w, r) { + if !requireRest(w, r) { return } writeJSON(w, http.StatusOK, []interface{}{}) @@ -233,27 +659,226 @@ func newMux(s *store) *http.ServeMux { mux.HandleFunc("GET /folders/root/contents", emptyContents) mux.HandleFunc("GET /folders/{id}/contents", emptyContents) - // SCIM: deactivate/reactivate (PATCH) and delete (DELETE). Separate base URL - // (/scim/v2) and bearer token in the real API; here any bearer is accepted. - // Logs the {id} path value so we can confirm the connector sends lucid-. + // GET /scim/v2/Users — https://lucid.readme.io/reference/getallusers + // Lucid documents that "Deactivated users will not be + // included in the totalResults or the JSON payload of users returned" + // (reference/getallusers) — which is exactly why reading users over REST and + // writing over SCIM, as the connector does, is the correct split. + mux.HandleFunc("GET /scim/v2/Users", func(w http.ResponseWriter, r *http.Request) { + if !requireScim(w, r) { + return + } + var active []scimUser + for _, u := range s.listUsers() { + if u.Enabled { + active = append(active, toScimUser(u)) + } + } + writeSCIMJSON(w, http.StatusOK, map[string]interface{}{ + "schemas": []string{scimListSchema}, + "totalResults": len(active), + "startIndex": 1, + "itemsPerPage": len(active), + "Resources": active, + }) + }) + + // GET /scim/v2/Users/{id} — https://lucid.readme.io/reference/getuser-1 + // 404s specifically for absence, which is what lets the connector tell a + // deleted user apart from REST's ambiguous 403. + mux.HandleFunc("GET /scim/v2/Users/{id}", func(w http.ResponseWriter, r *http.Request) { + if !requireScim(w, r) { + return + } + id := r.PathValue("id") + numericID, ok := parseScimID(id) + if !ok { + writeLucidError(w, http.StatusNotFound, "notFound", + fmt.Sprintf("SCIM resource id must be of the form %s", scimIDPrefix)) + return + } + u, found := s.getUserByID(strconv.Itoa(numericID)) + if !found { + log.Printf("GET /scim/v2/Users/%s — not found", id) //nolint:gosec // test-server: path value is diagnostic only + writeLucidError(w, http.StatusNotFound, "notFound", "user not found") + return + } + writeSCIMJSON(w, http.StatusOK, toScimUser(u)) + }) + + // PATCH /scim/v2/Users/{id} — https://lucid.readme.io/reference/modifyuserpatch + // Applies and PERSISTS the requested operations, then + // echoes real state back. The previous version of this mock always replied + // active:true and stored nothing, which made a deactivation impossible to + // verify and could never have caught a wrong value on the wire. mux.HandleFunc("PATCH /scim/v2/Users/{id}", func(w http.ResponseWriter, r *http.Request) { - if !requireBearer(w, r) { + if !requireScim(w, r) { return } id := r.PathValue("id") - log.Printf("PATCH /scim/v2/Users/%s", id) //nolint:gosec // test-server: path value is diagnostic only - writeJSON(w, http.StatusOK, map[string]interface{}{ - "id": id, - "active": true, + + // Content-Type. Lucid's OpenAPI declares application/json for every SCIM + // operation; RFC 7644 mandates application/scim+json, which is what the + // connector sends. Accept both and say which arrived, so the divergence is + // visible without inventing a verdict the docs cannot settle. + ct := r.Header.Get("Content-Type") + switch { + case strings.HasPrefix(ct, docContentType): + // matches Lucid's published spec + case strings.HasPrefix(ct, scimContentType): + log.Printf("DIVERGENCE: Content-Type %q is RFC 7644-correct but Lucid's spec declares %q for SCIM", scimContentType, docContentType) + if cfg.strictSCIMDoc { + writeLucidError(w, http.StatusUnsupportedMediaType, "badRequest", + fmt.Sprintf("Lucid's SCIM spec declares Content-Type %s, got %q", docContentType, ct)) + return + } + default: + writeLucidError(w, http.StatusUnsupportedMediaType, "badRequest", + fmt.Sprintf("Content-Type must be %s (Lucid spec) or %s (RFC 7644), got %q", docContentType, scimContentType, ct)) + return + } + + numericID, ok := parseScimID(id) + if !ok { + log.Printf("PATCH /scim/v2/Users/%s — id is missing the %q prefix Lucid documents", id, scimIDPrefix) //nolint:gosec // test-server: path value is diagnostic only + writeLucidError(w, http.StatusNotFound, "notFound", + fmt.Sprintf("SCIM resource id must be of the form %s", scimIDPrefix)) + return + } + + var body scimPatchOp + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + writeLucidError(w, http.StatusBadRequest, "badRequest", "malformed PatchOp body") + return + } + // schemas. Lucid's PATCH requestBody requires it and gives + // urn:ietf:params:scim:schemas:core:2.0:User as its example; RFC 7644 says + // a PatchOp body carries the PatchOp URN, which is what the connector + // sends. Same unresolvable disagreement as Content-Type — accept both, + // log which, and only reject under -strict-scim-doc. + if len(body.Schemas) == 0 { + writeLucidError(w, http.StatusBadRequest, "badRequest", "schemas is required") + return + } + switch body.Schemas[0] { + case docPatchSchema: + // matches Lucid's published example + case scimPatchOpSchema: + log.Printf("DIVERGENCE: schemas[0]=%q is RFC 7644-correct but Lucid's spec example is %q", scimPatchOpSchema, docPatchSchema) + if cfg.strictSCIMDoc { + writeLucidError(w, http.StatusBadRequest, "badRequest", + fmt.Sprintf("Lucid's SCIM spec example for schemas is [%s], got %q", docPatchSchema, body.Schemas[0])) + return + } + default: + writeLucidError(w, http.StatusBadRequest, "badRequest", + fmt.Sprintf("schemas[0] must be %s (Lucid spec) or %s (RFC 7644), got %q", docPatchSchema, scimPatchOpSchema, body.Schemas[0])) + return + } + if len(body.Operations) == 0 { + writeLucidError(w, http.StatusBadRequest, "badRequest", "Operations must not be empty") + return + } + + var applied []string + updated, found := s.updateUser(numericID, func(u *user) { + for _, op := range body.Operations { + if !strings.EqualFold(op.Op, "replace") && !strings.EqualFold(op.Op, "add") { + continue + } + switch op.Path { + case "active": + if b, ok := op.Value.(bool); ok { + u.Enabled = b + applied = append(applied, fmt.Sprintf("active=%v", b)) + } + case "name.givenName": + if v, ok := op.Value.(string); ok { + _, family, _ := strings.Cut(u.Name, " ") + u.Name = strings.TrimSpace(v + " " + family) + applied = append(applied, "name.givenName") + } + case "name.familyName": + if v, ok := op.Value.(string); ok { + given, _, _ := strings.Cut(u.Name, " ") + u.Name = strings.TrimSpace(given + " " + v) + applied = append(applied, "name.familyName") + } + case "emails[primary eq true].value": + // Lucid documents no filtered-path support for PATCH. Its + // UserOperation.path example is the bare attribute "roles", and + // the only place it mentions the eq operator is the `filter` + // QUERY parameter on GET /Users. Whether Lucid's SCIM server + // resolves a value-filter path here is unverified. + log.Printf("DIVERGENCE: PATCH path %q uses a SCIM value filter; Lucid documents no filtered-path support (path example is bare %q)", op.Path, "roles") + if v, ok := op.Value.(string); ok { + u.Email = v + applied = append(applied, "emails") + } + case "userName": + if v, ok := op.Value.(string); ok { + u.Username = v + applied = append(applied, "userName") + } + case "roles": + if list, ok := op.Value.([]interface{}); ok { + roles := make([]string, 0, len(list)) + for _, entry := range list { + if m, ok := entry.(map[string]interface{}); ok { + if v, ok := m["value"].(string); ok { + roles = append(roles, v) + } + } + } + u.Roles = roles + applied = append(applied, "roles") + } + } + } }) + if !found { + log.Printf("PATCH /scim/v2/Users/%s — no such user", id) //nolint:gosec // test-server: path value is diagnostic only + writeLucidError(w, http.StatusNotFound, "notFound", "user not found") + return + } + + log.Printf("PATCH /scim/v2/Users/%s applied=[%s] → active=%v", id, strings.Join(applied, " "), updated.Enabled) //nolint:gosec // test-server: path value is diagnostic only + writeSCIMJSON(w, http.StatusOK, toScimUser(updated)) }) + + // DELETE /scim/v2/Users/{id} — https://lucid.readme.io/reference/deleteuser + // Lucid documents 204 on success, 404 for an unknown user, + // and 409 "if the user cannot be deleted (e.g., account owner or default + // document owner)" (reference/deleteuser). The stock mock always returned 204, + // so neither error path was reachable. mux.HandleFunc("DELETE /scim/v2/Users/{id}", func(w http.ResponseWriter, r *http.Request) { - if !requireBearer(w, r) { + if !requireScim(w, r) { return } id := r.PathValue("id") - removed := s.deleteUserByScimID(id) - log.Printf("DELETE /scim/v2/Users/%s removed=%v", id, removed) //nolint:gosec // test-server: path value is diagnostic only + + numericID, ok := parseScimID(id) + if !ok { + log.Printf("DELETE /scim/v2/Users/%s — id is missing the %q prefix Lucid documents", id, scimIDPrefix) //nolint:gosec // test-server: path value is diagnostic only + writeLucidError(w, http.StatusNotFound, "notFound", + fmt.Sprintf("SCIM resource id must be of the form %s", scimIDPrefix)) + return + } + + if cfg.protectedUsers[numericID] { + log.Printf("DELETE /scim/v2/Users/%s — protected user, returning 409", id) //nolint:gosec // test-server: path value is diagnostic only + writeLucidError(w, http.StatusConflict, "alreadyExists", + "the user cannot be deleted (account owner or default document owner)") + return + } + + if !s.deleteUserByID(numericID) { + log.Printf("DELETE /scim/v2/Users/%s — no such user, returning 404", id) //nolint:gosec // test-server: path value is diagnostic only + writeLucidError(w, http.StatusNotFound, "notFound", "user not found") + return + } + + log.Printf("DELETE /scim/v2/Users/%s removed=true", id) //nolint:gosec // test-server: path value is diagnostic only w.WriteHeader(http.StatusNoContent) }) @@ -261,23 +886,82 @@ func newMux(s *store) *http.ServeMux { // path is caught immediately rather than silently swallowed. mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { log.Printf("UNMATCHED ROUTE: %s %s — returning 404", r.Method, r.URL.Path) //nolint:gosec // test-server: path value is diagnostic only - http.NotFound(w, r) + writeLucidError(w, http.StatusNotFound, "unknownOperation", "no such endpoint") }) return mux } +func schemeOf(r *http.Request) string { + if r.TLS != nil { + return "https" + } + return "http" +} + +func parseProtected(raw string) map[int]bool { + out := map[int]bool{} + for _, part := range strings.Split(raw, ",") { + part = strings.TrimSpace(part) + if part == "" { + continue + } + if id, err := strconv.Atoi(part); err == nil { + out[id] = true + } + } + return out +} + func run() error { addr := flag.String("addr", ":8080", "address to listen on") + users := flag.Int("users", 0, "replace the seed fixtures with exactly N generated users (0 = keep fixtures); use to hit pagination boundaries") + legacy404 := flag.Bool("legacy-user-404", false, "GET /v1/users/{id} returns 404 for an unknown user instead of the documented 403 (A/B for the delete-retry path)") + protected := flag.String("protected-users", "", "comma-separated numeric user IDs that SCIM DELETE rejects with 409, as Lucid does for account/document owners") + transferLimit := flag.Bool("transfer-rate-limit", false, "enforce Lucid's documented 30 requests / 5 seconds limit on transferUserContent, returning 429 + Retry-After") + pageSize := flag.Int("page-size", lucidPageSize, "records per page for GET /users (Lucid's documented default and maximum is 200)") + scimToken := flag.String("scim-token", "test-scim-token", "bearer token the SCIM surface requires; must match --lucid-scim-token") + strictSCIMDoc := flag.Bool("strict-scim-doc", false, + "reject SCIM requests that follow RFC 7644 where Lucid's spec documents otherwise "+ + "(application/json Content-Type, core-User schemas URN); use to demonstrate the divergence") flag.Parse() + s := newStore() + if *users > 0 { + s.seedUsers(*users) + } + + // Clamp a non-positive -page-size to Lucid's documented page size. Left + // unclamped, GET /users returns an empty page while still emitting a Link + // next-token at the same offset, which paginates the connector under test + // forever. Matches the n > 0 guard applied to the pageSize query param. + size := *pageSize + if size <= 0 { + size = lucidPageSize + } + + cfg := config{ + legacyUser404: *legacy404, + protectedUsers: parseProtected(*protected), + transferLimit: *transferLimit, + pageSize: size, + scimToken: *scimToken, + strictSCIMDoc: *strictSCIMDoc, + } + srv := &http.Server{ Addr: *addr, - Handler: newMux(newStore()), + Handler: newMux(s, cfg), ReadHeaderTimeout: 10 * time.Second, } log.Printf("lucidchart test-server listening on http://%s", *addr) + log.Printf(" users=%d pageSize=%d legacyUser404=%v protected=%v transferRateLimit=%v", + len(s.listUsers()), cfg.pageSize, cfg.legacyUser404, *protected, cfg.transferLimit) + if !cfg.legacyUser404 { + log.Printf(" NOTE: GET /v1/users/{id} returns 403 for unknown users, per Lucid's docs.") + log.Printf(" The connector's Delete path treats only 404 as already-deleted.") + } if err := srv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) { return err } diff --git a/pkg/connector/client/helpers.go b/pkg/connector/client/helpers.go index 18ab33c6..c527d4f2 100644 --- a/pkg/connector/client/helpers.go +++ b/pkg/connector/client/helpers.go @@ -12,8 +12,19 @@ func IsNotFoundError(err error) bool { return status.Code(err) == codes.NotFound } -// IsAlreadyExistsError reports whether err represents an upstream "already -// exists" (HTTP 409 Conflict) response. -func IsAlreadyExistsError(err error) bool { +// IsPermissionDeniedError reports whether err represents an upstream 403. +// Lucid's GET /v1/users/{id} returns 403 — never 404 — for a user that does not +// exist, so a 403 means either "gone" or "not permitted" and callers must +// disambiguate elsewhere. https://lucid.readme.io/reference/getuser +func IsPermissionDeniedError(err error) bool { + return status.Code(err) == codes.PermissionDenied +} + +// 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". +// This is the connector's single 409 predicate; there is intentionally no +// separate IsAlreadyExistsError so the two cannot drift apart. +func IsConflictError(err error) bool { return status.Code(err) == codes.AlreadyExists } diff --git a/pkg/connector/client/models.go b/pkg/connector/client/models.go index e29c00df..c7049fdf 100644 --- a/pkg/connector/client/models.go +++ b/pkg/connector/client/models.go @@ -6,13 +6,21 @@ import ( "time" ) +// User mirrors Lucid's REST User model. +// https://lucid.readme.io/reference/getuser type User struct { - AccountId int `json:"accountId"` - Email string `json:"email"` - Name string `json:"name"` - UserId int `json:"userId"` - Usernames string `json:"usernames"` - Roles []string `json:"roles"` + AccountId int `json:"accountId"` + Email string `json:"email"` + Name string `json:"name"` + UserId int `json:"userId"` + Username string `json:"username"` + // Enabled is the read-back for the SCIM active flag + // (https://lucid.readme.io/reference/getuser). It is a pointer so an absent + // field is distinguishable from an explicit false: userResource fails safe to + // ENABLED when it is nil, rather than reporting a phantom mass-deactivation if + // a payload ever omits it. + Enabled *bool `json:"enabled"` + Roles []string `json:"roles"` } type Folder struct { diff --git a/pkg/connector/client/scim.go b/pkg/connector/client/scim.go index 43a81016..f6855611 100644 --- a/pkg/connector/client/scim.go +++ b/pkg/connector/client/scim.go @@ -101,6 +101,29 @@ func (c *LucidchartClient) SetUserActive(ctx context.Context, userID string, act return nil, nil } +// 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) { + if !c.ScimConfigured() { + return false, errScimNotConfigured + } + + req, err := c.newScimRequest(ctx, http.MethodGet, fmt.Sprintf(ScimUserPath, scimResourceID(userID)), nil) + if err != nil { + return false, err + } + + if _, err := c.doRequest(ctx, req, nil); err != nil { + if IsNotFoundError(err) { + return false, nil + } + return false, err + } + + return true, nil +} + // ScimDeleteUser permanently deletes a user via SCIM DELETE /Users/{id}. This // is a hard delete; callers should transfer owned content first when it must be // retained (see TransferContent). diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 2aae9fe5..1eb4b21d 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -189,35 +189,39 @@ func generateCredentials(credentialOptions *v2.LocalCredentialOptions) (string, return password, nil } -// userTraitStatusToResourceStatus maps the user-trait status enum to the -// generic resource status enum explicitly, rather than relying on their -// ordinal values staying aligned. -func userTraitStatusToResourceStatus(status v2.UserTrait_Status_Status) v2.Status_ResourceStatus { - switch status { - case v2.UserTrait_Status_STATUS_ENABLED: - return v2.Status_RESOURCE_STATUS_ENABLED - case v2.UserTrait_Status_STATUS_DISABLED: - return v2.Status_RESOURCE_STATUS_DISABLED - case v2.UserTrait_Status_STATUS_DELETED: - return v2.Status_RESOURCE_STATUS_DELETED - default: - return v2.Status_RESOURCE_STATUS_UNSPECIFIED - } -} - func userResource(user client.User) (*v2.Resource, error) { + // Reporting Lucid's `enabled` field is what makes a SCIM deactivation + // observable; the status used to be pinned to ENABLED. client.User.Enabled is + // a *bool, so an absent field fails safe to ENABLED — only an explicit + // enabled=false flips the status, never a missing key. status := v2.UserTrait_Status_STATUS_ENABLED + if user.Enabled != nil && !*user.Enabled { + status = v2.UserTrait_Status_STATUS_DISABLED + } + + // structpb has no []string case, so roles have to be widened. Emitting them + // makes a role change through the SCIM update_user action observable in a sync. + roles := make([]interface{}, 0, len(user.Roles)) + for _, r := range user.Roles { + roles = append(roles, r) + } profile := map[string]interface{}{ "account_id": user.AccountId, "email": user.Email, "name": user.Name, argUserID: user.UserId, - "usernames": user.Usernames, + "username": user.Username, + "roles": roles, + } + if user.Enabled != nil { + profile["enabled"] = *user.Enabled } userTraitOptions := []rs.UserTraitOption{ + rs.WithUserProfile(profile), rs.WithEmail(user.Email, true), + rs.WithStatus(status), rs.WithUserLogin(user.Email), } @@ -226,8 +230,6 @@ func userResource(user client.User) (*v2.Resource, error) { userResourceType, user.UserId, userTraitOptions, - rs.WithResourceProfile(profile), - rs.WithResourceStatus(userTraitStatusToResourceStatus(status), ""), ) if err != nil { return nil, err @@ -249,34 +251,70 @@ func (o *userBuilder) Delete(ctx context.Context, resourceID *v2.ResourceId, par userID := resourceID.Resource - // If content transfer is configured, resolve the user's email via GetUser - // (transferUserContent requires email, not ID). A 404 from GetUser means the - // REST record is already gone; skip the transfer but still attempt the SCIM - // delete below — REST 404 does not guarantee the SCIM record is absent, and - // ScimDeleteUser already treats its own 404 as success. Only a non-404 error - // or a transfer failure aborts the delete. if o.contentTransferUserEmail != "" { - fromUser, err := o.client.GetUser(ctx, userID) - if err != nil && !client.IsNotFoundError(err) { - return nil, fmt.Errorf("baton-lucidchart: resolve email for content transfer (user %s): %w", userID, err) - } - if err == nil { - if _, err := o.client.TransferContent(ctx, fromUser.Email, o.contentTransferUserEmail); err != nil { - return nil, fmt.Errorf("baton-lucidchart: transfer content from user %s: %w", userID, err) - } + if err := o.transferContentBeforeDelete(ctx, userID); err != nil { + return nil, err } + // A nil error with no email resolved means the user is already gone; + // fall through to the SCIM delete, which treats its own 404 as success. } annos, err := o.client.ScimDeleteUser(ctx, userID) - if err != nil { - if client.IsNotFoundError(err) { - // Already deleted is success. - return annos, nil - } + switch { + case err == nil: + return annos, nil + case client.IsNotFoundError(err): + // Already deleted is success. + return annos, nil + case client.IsConflictError(err): + // Raw 409 surfaces as AlreadyExists, which reads as an idempotent + // success and would let a failed offboarding look complete. + return annos, status.Errorf(codes.FailedPrecondition, + "baton-lucidchart: delete user %s: Lucid refused the delete (409) — the user is an account owner "+ + "or a default document owner and cannot be deleted via SCIM; reassign that role in Lucid first", + userID) + default: return annos, fmt.Errorf("baton-lucidchart: delete user %s: %w", userID, err) } +} + +// transferContentBeforeDelete moves the leaving user's documents to the +// configured recipient, resolving their email from the REST record first. +// +// REST answers 403 both for "gone" and for "not permitted", so when it cannot +// answer we ask SCIM, which 404s specifically for absence. Guessing either way +// would mean deleting content we failed to move, or breaking retry idempotency. +func (o *userBuilder) transferContentBeforeDelete(ctx context.Context, userID string) error { + fromUser, err := o.client.GetUser(ctx, userID) + switch { + case err == nil: + if _, err := o.client.TransferContent(ctx, fromUser.Email, o.contentTransferUserEmail); err != nil { + return fmt.Errorf("baton-lucidchart: transfer content from user %s: %w", userID, err) + } + return nil + + case client.IsNotFoundError(err), client.IsPermissionDeniedError(err): + exists, existsErr := o.client.ScimUserExists(ctx, userID) + if existsErr != nil { + return fmt.Errorf( + "baton-lucidchart: could not resolve user %s for content transfer (%w) and could not confirm whether they still exist: %w", + userID, err, existsErr) + } + if exists { + // Present but unreadable over REST: deleting would destroy content + // the operator asked to retain. + 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) + } + // Genuinely gone; proceed so the delete stays idempotent under retry. + return nil - return annos, nil + default: + return fmt.Errorf("baton-lucidchart: resolve email for content transfer (user %s): %w", userID, err) + } } func newUserBuilder(client *client.LucidchartClient, contentTransferUserEmail string) *userBuilder { diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index 2d9958a4..f95dd508 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -10,6 +10,8 @@ import ( v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/stretchr/testify/require" "golang.org/x/oauth2" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) func testLucidClient(t *testing.T, restURL, scimURL string) *client.LucidchartClient { @@ -20,34 +22,196 @@ func testLucidClient(t *testing.T, restURL, scimURL string) *client.LucidchartCl return c } -// When content transfer is configured and GetUser returns 404, Delete must skip -// the transfer (no email to resolve) but still attempt the SCIM delete — -// REST 404 does not prove the SCIM record is gone. -func TestDelete_GetUserNotFoundWithContentTransfer_SkipsTransferButRunsScimDelete(t *testing.T) { - var transferCalled, scimDeleteCalled bool +// deleteRoutes records which paths a Delete touched, so tests can assert both +// what was called and what was not. +type deleteRoutes struct { + getUser bool + transfer bool + scimGet bool + scimDelete bool + scimDeleteID string +} - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { +// newDeleteServer serves the three endpoints Delete touches. restUserStatus is +// the code GET /v1/users/{id} answers; scimGetStatus is what SCIM GET answers; +// scimDeleteStatus is what SCIM DELETE answers. +func newDeleteServer(t *testing.T, routes *deleteRoutes, restUserStatus, scimGetStatus, scimDeleteStatus int) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch { - case r.Method == http.MethodGet && r.URL.Path == "/v1/users/already-deleted": - w.WriteHeader(http.StatusNotFound) + case r.Method == http.MethodGet && r.URL.Path == "/v1/users/42": + routes.getUser = true + if restUserStatus != http.StatusOK { + w.WriteHeader(restUserStatus) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"userId":42,"email":"leaver@example.com","enabled":true}`)) + case r.Method == http.MethodPost && r.URL.Path == "/v1/transferUserContent": - transferCalled = true - w.WriteHeader(http.StatusOK) - case r.Method == http.MethodDelete && r.URL.Path == "/Users/lucid-already-deleted": - scimDeleteCalled = true + routes.transfer = true w.WriteHeader(http.StatusNoContent) + + case r.Method == http.MethodGet && r.URL.Path == "/Users/lucid-42": + routes.scimGet = true + w.WriteHeader(scimGetStatus) + + case r.Method == http.MethodDelete && r.URL.Path == "/Users/lucid-42": + routes.scimDelete = true + routes.scimDeleteID = r.URL.Path + w.WriteHeader(scimDeleteStatus) + default: - t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusInternalServerError) } })) - defer srv.Close() +} +// deleteUser runs Delete against srv. Returns the Delete error only; the +// annotations are not asserted by these tests. +func deleteUser(t *testing.T, srv *httptest.Server, transferEmail string) error { + t.Helper() c := testLucidClient(t, srv.URL, srv.URL) - b := newUserBuilder(c, "recipient@example.com") + b := newUserBuilder(c, transferEmail) + _, err := b.Delete(context.Background(), &v2.ResourceId{Resource: "42"}, nil) + return err +} + +// Lucid's GET /v1/users/{id} answers 403 — never 404 — for a user that does not +// exist. Delete must still complete, or every retry of an already-processed +// deprovision fails forever. +func TestDelete_RestForbiddenAndUserGone_ProceedsToScimDelete(t *testing.T) { + routes := &deleteRoutes{} + srv := newDeleteServer(t, routes, http.StatusForbidden, http.StatusNotFound, http.StatusNoContent) + defer srv.Close() - annos, err := b.Delete(context.Background(), &v2.ResourceId{Resource: "already-deleted"}, nil) + err := deleteUser(t, srv, "recipient@example.com") require.NoError(t, err) - require.Nil(t, annos) - require.False(t, transferCalled, "transfer must be skipped when GetUser returns not-found") - require.True(t, scimDeleteCalled, "SCIM delete must still be attempted even when GetUser returns not-found") + + require.True(t, routes.getUser, "REST lookup should be attempted") + require.True(t, routes.scimGet, "SCIM must be consulted to disambiguate the 403") + require.False(t, routes.transfer, "no transfer is possible for a user that is gone") + require.True(t, routes.scimDelete, "delete must still run so retries converge") +} + +// A 403 with the user still present is a scope problem, not an absence. Deleting +// would destroy the content the operator asked to retain. +func TestDelete_RestForbiddenButUserExists_RefusesToDelete(t *testing.T) { + routes := &deleteRoutes{} + srv := newDeleteServer(t, routes, http.StatusForbidden, http.StatusOK, http.StatusNoContent) + defer srv.Close() + + err := deleteUser(t, srv, "recipient@example.com") + require.Error(t, err) + require.Equal(t, codes.FailedPrecondition, status.Code(err)) + require.False(t, routes.transfer) + require.False(t, routes.scimDelete, "must not delete when content could not be transferred") +} + +func TestDelete_GetUserNotFoundWithContentTransfer_SkipsTransferButRunsScimDelete(t *testing.T) { + routes := &deleteRoutes{} + srv := newDeleteServer(t, routes, http.StatusNotFound, http.StatusNotFound, http.StatusNoContent) + defer srv.Close() + + err := deleteUser(t, srv, "recipient@example.com") + require.NoError(t, err) + require.False(t, routes.transfer, "transfer must be skipped when the user is not found") + require.True(t, routes.scimDelete) +} + +func TestDelete_HappyPath_TransfersThenDeletes(t *testing.T) { + routes := &deleteRoutes{} + srv := newDeleteServer(t, routes, http.StatusOK, http.StatusOK, http.StatusNoContent) + defer srv.Close() + + err := deleteUser(t, srv, "recipient@example.com") + require.NoError(t, err) + + require.True(t, routes.transfer, "content must be transferred before delete") + require.True(t, routes.scimDelete) + require.Equal(t, "/Users/lucid-42", routes.scimDeleteID) + require.False(t, routes.scimGet, "no SCIM probe needed when REST answered") +} + +// Without a transfer email there is nothing to resolve, so Delete must not call +// REST at all. +func TestDelete_NoTransferEmail_SkipsRestLookup(t *testing.T) { + routes := &deleteRoutes{} + srv := newDeleteServer(t, routes, http.StatusOK, http.StatusOK, http.StatusNoContent) + defer srv.Close() + + err := deleteUser(t, srv, "") + require.NoError(t, err) + + require.False(t, routes.getUser) + require.False(t, routes.transfer) + require.True(t, routes.scimDelete) +} + +// Lucid answers 409 for a user that can never be deleted. Surfacing that as +// AlreadyExists would read as an idempotent success and hide a failed +// offboarding. +func TestDelete_ScimConflict_IsTerminalNotAlreadyExists(t *testing.T) { + routes := &deleteRoutes{} + srv := newDeleteServer(t, routes, http.StatusOK, http.StatusOK, http.StatusConflict) + defer srv.Close() + + err := deleteUser(t, srv, "") + require.Error(t, err) + require.Equal(t, codes.FailedPrecondition, status.Code(err)) + require.NotEqual(t, codes.AlreadyExists, status.Code(err)) + require.Contains(t, err.Error(), "account owner") +} + +func boolPtr(b bool) *bool { return &b } + +func TestUserResource_ReportsEnabledAndUsername(t *testing.T) { + res, err := userResource(client.User{ + AccountId: 1, + Email: "disabled@example.com", + Name: "Dana Disabled", + UserId: 105, + Username: "disabled@example.com", + Enabled: boolPtr(false), + Roles: []string{"developer"}, + }) + require.NoError(t, err) + + require.Equal(t, v2.Status_RESOURCE_STATUS_DISABLED, res.GetStatus().GetStatus()) + + profile := res.GetProfile().AsMap() + require.Equal(t, "disabled@example.com", profile["username"]) + require.Equal(t, false, profile["enabled"]) + require.Equal(t, []interface{}{"developer"}, profile["roles"], "roles must be emitted so an update_user role change is observable") + require.NotContains(t, profile, "usernames", "the plural key is not a field Lucid emits") +} + +// The `enabled` field must fail safe: an absent field maps to ENABLED, never a +// phantom DISABLED. Otherwise a GET /users payload that omitted `enabled` would +// sync every user as deactivated. `Enabled *bool` is what makes absent and +// explicit-false distinguishable. +func TestUserResource_EnabledMapping(t *testing.T) { + for _, tc := range []struct { + name string + enabled *bool + want v2.Status_ResourceStatus + }{ + {"absent field fails safe to enabled", nil, v2.Status_RESOURCE_STATUS_ENABLED}, + {"explicit false is disabled", boolPtr(false), v2.Status_RESOURCE_STATUS_DISABLED}, + {"explicit true is enabled", boolPtr(true), v2.Status_RESOURCE_STATUS_ENABLED}, + } { + t.Run(tc.name, func(t *testing.T) { + res, err := userResource(client.User{Email: "a@example.com", UserId: 1, Enabled: tc.enabled}) + require.NoError(t, err) + require.Equal(t, tc.want, res.GetStatus().GetStatus()) + + profile := res.GetProfile().AsMap() + if tc.enabled == nil { + require.NotContains(t, profile, "enabled", "absent field must not be reported as a value") + } else { + require.Equal(t, *tc.enabled, profile["enabled"]) + } + }) + } } diff --git a/ticket-brief.md b/ticket-brief.md new file mode 100644 index 00000000..3f41060c --- /dev/null +++ b/ticket-brief.md @@ -0,0 +1,95 @@ +# Ticket Brief — CXH-2281 + +- **Title:** baton-lucidchart: four SCIM account-lifecycle defects surfaced during CXH-1488 validation +- **URL:** https://linear.app/ductone/issue/CXH-2281 +- **Team / Priority / State:** Connector Horizon · High · In Progress +- **Parent:** CXH-1488 (blocks CXH-1488) · Labels: `Connector: Lucidchart`, `Bug` +- **Filed by:** QA (Manuel Traversaro Sasia) from CXH-1488 validation +- **Reference PR (prior art, not assumed mergeable):** #59 — `[CXH-2281] fix: SCIM account-lifecycle defects and test-server fidelity` + +## INTENT (acceptance criteria, restated verbatim from the ticket) + +> Validation of `baton-lucidchart` v0.1.3 against a test-server rebuilt from Lucid's published OpenAPI surfaced four defects on the SCIM account-lifecycle paths added by CXH-1488. All four are reproducible; a fix branch and PR accompany this issue. +> +> The sandbox Lucid account is Team/Free tier and the SCIM surface is Enterprise-only, so these were exercised against a local mock. The mock replicates Lucid's documented contract rather than the connector's assumptions — where the two disagree it follows the documentation, which is how findings 1 and 2 surfaced at all. +> +> ### Finding 1 — delete retry aborts on Lucid's documented 403 +> `pkg/connector/users.go:260` +> `Delete` resolves the leaving user's email before a content transfer and continues only when the lookup fails with not-found: +> ```go +> if err != nil && !client.IsNotFoundError(err) { +> ``` +> Lucid's `GET /v1/users/{id}` documents 403 — never 404 — for a user that does not exist: "if the user does not belong to the authenticated account or if the user does not exist" (https://lucid.readme.io/reference/getuser). The guard therefore fails and the operation aborts before the SCIM delete is attempted. +> +> Observed on 2026-08-18, deleting an already-removed user with `--lucid-content-transfer-user-email` set: +> ``` +> rpc error: code = PermissionDenied desc = baton-lucidchart: resolve email for content transfer (user 105): baton-lucidchart: get user 105: rpc error: code = PermissionDenied desc = 403 Forbidden +> ``` +> The mock log shows no `DELETE /scim/v2/Users/lucid-105` after the 403. +> +> ### Finding 2 — SCIM delete 409 surfaces as AlreadyExists +> `pkg/connector/users.go:272` +> `Delete` maps only 404 to success; every other status is wrapped verbatim. Lucid returns 409 for a user that cannot be deleted — "e.g., account owner or default document owner" (https://lucid.readme.io/reference/deleteuser) — which `uhttp` maps to `codes.AlreadyExists`, the same code the SDK uses for idempotent no-ops. +> +> Observed 2026-08-18 against a user the mock marks protected: +> ``` +> rpc error: code = AlreadyExists desc = baton-lucidchart: delete user 101: rpc error: code = AlreadyExists desc = 409 Conflict +> ``` +> The user remains present in the next sync. The reason Lucid gave is not carried in the message. +> +> ### Finding 3 — user status is pinned to ENABLED +> `pkg/connector/users.go:209` +> ```go +> status := v2.UserTrait_Status_STATUS_ENABLED +> ``` +> The status is a constant, and `client.User` (`pkg/connector/client/models.go:9-16`) has no field for Lucid's `enabled`, documented as "Whether the user can authenticate to Lucid. Corresponds to the SCIM active attribute" (https://lucid.readme.io/reference/getuser). +> +> Observed 2026-08-18: after `disable_user` returned `success: true` and the mock recorded `enabled=false`, a re-sync emitted `RESOURCE_STATUS_ENABLED` for that user. The same function drops `User.Roles` from the emitted profile, so a role change made through `update_user` is also absent from the bundle. +> +> ### Finding 4 — the username field never decodes +> `pkg/connector/client/models.go:14` +> ```go +> Usernames string `json:"usernames"` +> ``` +> Lucid's REST `User` model defines `username` (singular). The plural key is not emitted by Lucid, so the field unmarshals empty on every user and the profile key written at `pkg/connector/users.go:216` is always `""`. +> +> ### Reproduction +> 1. 2026-08-18 — build the connector from `origin/main` (79714b3) and from the fix branch. +> 2. Start the bundled test-server with `-protected-users 101`. +> 3. Run a sync against the mock and read back user 105 (`enabled=false` in the mock). +> 4. Delete user 105, then delete it again with `--lucid-content-transfer-user-email` set. +> 5. Delete user 101. +> +> | Check | origin/main | fix branch | +> | -- | -- | -- | +> | Status of a disabled user | `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` | +> +> The harness re-runs deterministically; a second run of the fix-branch case produced identical output. +> +> ### Impact +> With a content-transfer recipient configured — the configuration that preserves a leaving user's documents — a retried delete of an already-removed user returns `PermissionDenied` and never reaches the SCIM delete, so retries cannot converge. A 409 on a protected user returns the gRPC code used for idempotent success while the user remains present. Deactivation and role changes made through the SCIM actions are absent from the synced bundle. The `usernames` profile key is empty on every user. + +## Acceptance checklist (derived from the A/B table — the ticket's pass/fail gate) + +1. **Status of a disabled user** → `RESOURCE_STATUS_DISABLED` (was `ENABLED`). +2. **`username` in profile** → populated (was absent, because the model read `usernames`). +3. **Delete retry after 403** → exit 0 / idempotent success (was exit 1, `PermissionDenied`). +4. **Delete a protected user (409)** → `FailedPrecondition` carrying Lucid's reason (was `AlreadyExists`). +5. Test-server is rewritten from Lucid's **published OpenAPI** (not the connector's assumptions); it is the repro harness the sibling tickets CXH-2282–2285 depend on, so it must be correct. + +## Deltas vs. reference PR #59 (verified, not blindly ported) + +PR #59 implements the four fixes + the test-server rewrite, but its CI review left **1 blocking issue + 3 suggestions** open. This branch ports PR #59's sound approach and closes those: + +- **[BLOCKING] `Enabled bool` cannot distinguish "field absent" from "explicitly false".** PR #59 defaults `userResource` status to `DISABLED`, so any `GET /users` payload missing `enabled` would sync **every** user as disabled — a mass-deactivation footgun. Fix here: `Enabled *bool`, default `STATUS_ENABLED`, flip to `DISABLED` only when `Enabled != nil && !*Enabled`. Confirmed against Lucid docs that `GET /users` (listusers) *does* return `enabled` + `username` (same User schema as getuser), so the field is present on the sync path — the pointer is defence-in-depth, at no cost. +- **[suggestion] `IsConflictError` duplicated `IsAlreadyExistsError` verbatim.** Collapsed to a single `IsConflictError` predicate (the 409/delete-refusal name), dropped the callerless `IsAlreadyExistsError`. +- **[suggestion] test-server `-page-size 0`/negative was unclamped** → empty page + repeating `Link` next token = infinite pagination for the connector under test. Clamp `if size <= 0 { size = lucidPageSize }`. +- **[suggestion] `ci.yaml` comment "Any non-empty bearer is accepted" is now false** and `start-test-server` never passed `-scim-token`. Updated the comment and wired `-scim-token` (from `BATON_LUCID_SCIM_TOKEN`) into the composite action so REST/SCIM tokens cannot silently diverge. +- **Excluded PR #59's unrelated churn:** its `go.mod`/`go.sum`/`vendor/**`/`.versions.yaml` bumps (PR #59 was cut from `79714b3`; this branch is based on the newer `origin/main` `3420066` which already carries the dependency/Go bumps) and its deletion of `tickets/CXH-1488/*.md`. + +## Not in scope (tracked separately, per PR #59) +- CXH-2282 — SCIM wire contract (Content-Type / PatchOp schemas URN divergence; needs a live Enterprise tenant). Test-server models it behind `-strict-scim-doc`, off by default. +- CXH-2283 — ungated capabilities · CXH-2284 — remaining SCIM surface gaps · CXH-2285 — folder/document grant idempotency. From 2cbaa2c7ce67e76c0f9688230a1cc6ba81aa706c Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Thu, 20 Aug 2026 15:35:01 -0300 Subject: [PATCH 02/12] [CXH-2281] Fix lint: restore non-deprecated SDK trait options and guard 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) --- cmd/test-server/main.go | 5 +++++ pkg/connector/users.go | 20 ++++++++++++++++++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/cmd/test-server/main.go b/cmd/test-server/main.go index 354c9a99..c641d177 100644 --- a/cmd/test-server/main.go +++ b/cmd/test-server/main.go @@ -70,6 +70,10 @@ const ( // REST user 1234 is "lucid-1234" (reference/overview-scim, User.id). scimIDPrefix = "lucid-" + // maxFormBody caps the request body the mock will parse (1 MiB). Guards the + // ParseForm read against an unbounded body (gosec G107/decompression-bomb). + maxFormBody = 1 << 20 + // lucidPageSize is Lucid's page size for paginated REST endpoints. The docs // state a 200-record default and that a larger requested pageSize is clamped // to 200 (reference-rest). @@ -425,6 +429,7 @@ func newMux(s *store, cfg config) *http.ServeMux { // rejects what the real endpoint rejects: a permissive token endpoint has // shipped broken grant_type params to production before. mux.HandleFunc("POST /oauth2/token", func(w http.ResponseWriter, r *http.Request) { + r.Body = http.MaxBytesReader(w, r.Body, maxFormBody) if err := r.ParseForm(); err != nil { writeOAuthError(w, http.StatusBadRequest, "invalid_request", "malformed form body") return diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 1eb4b21d..cc88bd22 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -189,6 +189,22 @@ func generateCredentials(credentialOptions *v2.LocalCredentialOptions) (string, return password, nil } +// userTraitStatusToResourceStatus maps the user-trait status enum to the +// generic resource status enum explicitly, rather than relying on their +// ordinal values staying aligned. +func userTraitStatusToResourceStatus(status v2.UserTrait_Status_Status) v2.Status_ResourceStatus { + switch status { + case v2.UserTrait_Status_STATUS_ENABLED: + return v2.Status_RESOURCE_STATUS_ENABLED + case v2.UserTrait_Status_STATUS_DISABLED: + return v2.Status_RESOURCE_STATUS_DISABLED + case v2.UserTrait_Status_STATUS_DELETED: + return v2.Status_RESOURCE_STATUS_DELETED + default: + return v2.Status_RESOURCE_STATUS_UNSPECIFIED + } +} + func userResource(user client.User) (*v2.Resource, error) { // Reporting Lucid's `enabled` field is what makes a SCIM deactivation // observable; the status used to be pinned to ENABLED. client.User.Enabled is @@ -219,9 +235,7 @@ func userResource(user client.User) (*v2.Resource, error) { } userTraitOptions := []rs.UserTraitOption{ - rs.WithUserProfile(profile), rs.WithEmail(user.Email, true), - rs.WithStatus(status), rs.WithUserLogin(user.Email), } @@ -230,6 +244,8 @@ func userResource(user client.User) (*v2.Resource, error) { userResourceType, user.UserId, userTraitOptions, + rs.WithResourceProfile(profile), + rs.WithResourceStatus(userTraitStatusToResourceStatus(status), ""), ) if err != nil { return nil, err From 048a6826fede528d9b02eebacbe89ca5e90c004d Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Fri, 21 Aug 2026 13:25:15 -0300 Subject: [PATCH 03/12] [CXH-2281] Address PR #60 review: 409 err detail, probe-on-403-only, 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) --- cmd/test-server/main.go | 28 ++++++----- pkg/connector/users.go | 31 +++++++++--- pkg/connector/users_test.go | 22 ++++++--- ticket-brief.md | 95 ------------------------------------- 4 files changed, 57 insertions(+), 119 deletions(-) delete mode 100644 ticket-brief.md diff --git a/cmd/test-server/main.go b/cmd/test-server/main.go index c641d177..c6d66383 100644 --- a/cmd/test-server/main.go +++ b/cmd/test-server/main.go @@ -33,8 +33,8 @@ // - SCIM DELETE returns 404 for an unknown user and 409 for a protected one // ("if the user cannot be deleted (e.g., account owner or default document // owner)" — reference/deleteuser). -// - The REST user model exposes `username` (singular) and `enabled` -// (reference/getuser), not the `usernames` field the connector currently reads. +// - The REST user model exposes `username` (singular) and `enabled`, matching +// Lucid's documented shape (reference/getuser). // - SCIM Content-Type and the PatchOp `schemas` URN are ACCEPTED in both the // RFC 7644 form the connector sends and the form Lucid's spec documents, with // a DIVERGENCE line logged for the former. Lucid declares application/json @@ -114,11 +114,10 @@ type config struct { // user mirrors Lucid's REST User model (reference/getuser). // -// NOTE the field names. Lucid documents `username` (singular) and `enabled`. -// The connector's client.User reads `usernames` (plural) and has no `enabled` -// field at all, so both will come through empty/absent on the connector side. -// That is a real connector defect surfaced by this mock, not a mock bug — do not -// "fix" it here by renaming these fields to match the connector. +// NOTE the field names. Lucid documents `username` (singular) and `enabled` +// (reference/getuser), so this mock serves exactly that shape. Keep them as-is: +// the mock follows Lucid's published contract, and renaming a field to match a +// consumer would let the test suite drift from the documented API. type user struct { AccountId int `json:"accountId"` Email string `json:"email"` @@ -221,6 +220,11 @@ func (s *store) seedUsers(n int) { Enabled: true, }) } + // Seeded IDs run 1..n. Advance the allocator past them so a subsequent + // POST /users can't mint a colliding userId (e.g. -users 2000 then a create). + if n > s.nextID { + s.nextID = n + } } func (s *store) listUsers() []user { @@ -521,10 +525,10 @@ func newMux(s *store, cfg config) *http.ServeMux { // // Lucid documents 403 — NOT 404 — for a user that does not exist: // "if the user does not belong to the authenticated account or if the user - // does not exist" (reference/getuser). The connector's Delete path treats - // only 404 as "already deleted", so against the documented behaviour its - // idempotent-retry aborts. -legacy-user-404 restores the permissive 404 so a - // test can A/B the two. + // does not exist" (reference/getuser). This mock returns 403 for unknown + // users so the connector's delete path is exercised against the documented + // behaviour. -legacy-user-404 restores the permissive 404 so a test can A/B + // the two. mux.HandleFunc("GET /v1/users/{id}", func(w http.ResponseWriter, r *http.Request) { if !requireRest(w, r) { return @@ -965,7 +969,7 @@ func run() error { len(s.listUsers()), cfg.pageSize, cfg.legacyUser404, *protected, cfg.transferLimit) if !cfg.legacyUser404 { log.Printf(" NOTE: GET /v1/users/{id} returns 403 for unknown users, per Lucid's docs.") - log.Printf(" The connector's Delete path treats only 404 as already-deleted.") + log.Printf(" Use -legacy-user-404 to serve the permissive 404 instead.") } if err := srv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) { return err diff --git a/pkg/connector/users.go b/pkg/connector/users.go index cc88bd22..ebf018b6 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -237,6 +237,13 @@ func userResource(user client.User) (*v2.Resource, error) { userTraitOptions := []rs.UserTraitOption{ rs.WithEmail(user.Email, true), rs.WithUserLogin(user.Email), + // Keep the (deprecated) trait status in sync with the resource status. + // NewUserTrait defaults an unset trait status to ENABLED, so without this + // a disabled user would report Resource.Status=DISABLED alongside + // UserTrait.Status=ENABLED — the exact "disabled user reports enabled" + // divergence this fix targets, just moved to trait-status readers. + //nolint:staticcheck // SA1019: WithStatus is deprecated, but consumers still read the trait status; syncing it prevents the divergence. + rs.WithStatus(status), } newUserResource, err := rs.NewUserResource( @@ -287,8 +294,8 @@ func (o *userBuilder) Delete(ctx context.Context, resourceID *v2.ResourceId, par // success and would let a failed offboarding look complete. return annos, status.Errorf(codes.FailedPrecondition, "baton-lucidchart: delete user %s: Lucid refused the delete (409) — the user is an account owner "+ - "or a default document owner and cannot be deleted via SCIM; reassign that role in Lucid first", - userID) + "or a default document owner and cannot be deleted via SCIM; reassign that role in Lucid first (%v)", + userID, err) default: return annos, fmt.Errorf("baton-lucidchart: delete user %s: %w", userID, err) } @@ -297,9 +304,13 @@ func (o *userBuilder) Delete(ctx context.Context, resourceID *v2.ResourceId, par // transferContentBeforeDelete moves the leaving user's documents to the // configured recipient, resolving their email from the REST record first. // -// REST answers 403 both for "gone" and for "not permitted", so when it cannot -// answer we ask SCIM, which 404s specifically for absence. Guessing either way -// would mean deleting content we failed to move, or breaking retry idempotency. +// A definite REST 404 means the user is already gone: skip the transfer and let +// the SCIM delete (which treats its own 404 as success) run — no SCIM probe, so +// a probe outage can't block an otherwise-valid delete. REST answers 403 both +// for "gone" and for "not permitted", so only there is absence ambiguous; we ask +// SCIM (which 404s specifically for absence) before deciding, because guessing +// would mean either deleting content we failed to move or breaking retry +// idempotency. func (o *userBuilder) transferContentBeforeDelete(ctx context.Context, userID string) error { fromUser, err := o.client.GetUser(ctx, userID) switch { @@ -309,7 +320,15 @@ func (o *userBuilder) transferContentBeforeDelete(ctx context.Context, userID st } return nil - case client.IsNotFoundError(err), client.IsPermissionDeniedError(err): + case client.IsNotFoundError(err): + // REST gave a definite 404: the user is gone as far as content transfer + // is concerned. Proceed to the SCIM delete without probing SCIM first, so + // a probe failure (403/405/501 on a tenant without SCIM user GET, or a + // transient 5xx) cannot abort a delete REST already told us is safe. + return nil + + case client.IsPermissionDeniedError(err): + // REST 403 is ambiguous ("gone" or "not permitted"), so confirm with SCIM. exists, existsErr := o.client.ScimUserExists(ctx, userID) if existsErr != nil { return fmt.Errorf( diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index f95dd508..eaa3b640 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -8,6 +8,7 @@ import ( "github.com/conductorone/baton-lucidchart/pkg/connector/client" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + rs "github.com/conductorone/baton-sdk/pkg/types/resource" "github.com/stretchr/testify/require" "golang.org/x/oauth2" "google.golang.org/grpc/codes" @@ -193,19 +194,28 @@ func TestUserResource_ReportsEnabledAndUsername(t *testing.T) { // explicit-false distinguishable. func TestUserResource_EnabledMapping(t *testing.T) { for _, tc := range []struct { - name string - enabled *bool - want v2.Status_ResourceStatus + name string + enabled *bool + want v2.Status_ResourceStatus + wantTrait v2.UserTrait_Status_Status }{ - {"absent field fails safe to enabled", nil, v2.Status_RESOURCE_STATUS_ENABLED}, - {"explicit false is disabled", boolPtr(false), v2.Status_RESOURCE_STATUS_DISABLED}, - {"explicit true is enabled", boolPtr(true), v2.Status_RESOURCE_STATUS_ENABLED}, + {"absent field fails safe to enabled", nil, v2.Status_RESOURCE_STATUS_ENABLED, v2.UserTrait_Status_STATUS_ENABLED}, + {"explicit false is disabled", boolPtr(false), v2.Status_RESOURCE_STATUS_DISABLED, v2.UserTrait_Status_STATUS_DISABLED}, + {"explicit true is enabled", boolPtr(true), v2.Status_RESOURCE_STATUS_ENABLED, v2.UserTrait_Status_STATUS_ENABLED}, } { t.Run(tc.name, func(t *testing.T) { res, err := userResource(client.User{Email: "a@example.com", UserId: 1, Enabled: tc.enabled}) require.NoError(t, err) require.Equal(t, tc.want, res.GetStatus().GetStatus()) + // The deprecated trait status must not diverge from the resource + // status: some consumers still read it, so a disabled user reporting + // an ENABLED trait would reintroduce the bug this test guards. + trait, err := rs.GetUserTrait(res) + require.NoError(t, err) + //nolint:staticcheck // SA1019: asserting the deprecated trait status precisely because legacy consumers still read it. + require.Equal(t, tc.wantTrait, trait.GetStatus().GetStatus()) + profile := res.GetProfile().AsMap() if tc.enabled == nil { require.NotContains(t, profile, "enabled", "absent field must not be reported as a value") diff --git a/ticket-brief.md b/ticket-brief.md deleted file mode 100644 index 3f41060c..00000000 --- a/ticket-brief.md +++ /dev/null @@ -1,95 +0,0 @@ -# Ticket Brief — CXH-2281 - -- **Title:** baton-lucidchart: four SCIM account-lifecycle defects surfaced during CXH-1488 validation -- **URL:** https://linear.app/ductone/issue/CXH-2281 -- **Team / Priority / State:** Connector Horizon · High · In Progress -- **Parent:** CXH-1488 (blocks CXH-1488) · Labels: `Connector: Lucidchart`, `Bug` -- **Filed by:** QA (Manuel Traversaro Sasia) from CXH-1488 validation -- **Reference PR (prior art, not assumed mergeable):** #59 — `[CXH-2281] fix: SCIM account-lifecycle defects and test-server fidelity` - -## INTENT (acceptance criteria, restated verbatim from the ticket) - -> Validation of `baton-lucidchart` v0.1.3 against a test-server rebuilt from Lucid's published OpenAPI surfaced four defects on the SCIM account-lifecycle paths added by CXH-1488. All four are reproducible; a fix branch and PR accompany this issue. -> -> The sandbox Lucid account is Team/Free tier and the SCIM surface is Enterprise-only, so these were exercised against a local mock. The mock replicates Lucid's documented contract rather than the connector's assumptions — where the two disagree it follows the documentation, which is how findings 1 and 2 surfaced at all. -> -> ### Finding 1 — delete retry aborts on Lucid's documented 403 -> `pkg/connector/users.go:260` -> `Delete` resolves the leaving user's email before a content transfer and continues only when the lookup fails with not-found: -> ```go -> if err != nil && !client.IsNotFoundError(err) { -> ``` -> Lucid's `GET /v1/users/{id}` documents 403 — never 404 — for a user that does not exist: "if the user does not belong to the authenticated account or if the user does not exist" (https://lucid.readme.io/reference/getuser). The guard therefore fails and the operation aborts before the SCIM delete is attempted. -> -> Observed on 2026-08-18, deleting an already-removed user with `--lucid-content-transfer-user-email` set: -> ``` -> rpc error: code = PermissionDenied desc = baton-lucidchart: resolve email for content transfer (user 105): baton-lucidchart: get user 105: rpc error: code = PermissionDenied desc = 403 Forbidden -> ``` -> The mock log shows no `DELETE /scim/v2/Users/lucid-105` after the 403. -> -> ### Finding 2 — SCIM delete 409 surfaces as AlreadyExists -> `pkg/connector/users.go:272` -> `Delete` maps only 404 to success; every other status is wrapped verbatim. Lucid returns 409 for a user that cannot be deleted — "e.g., account owner or default document owner" (https://lucid.readme.io/reference/deleteuser) — which `uhttp` maps to `codes.AlreadyExists`, the same code the SDK uses for idempotent no-ops. -> -> Observed 2026-08-18 against a user the mock marks protected: -> ``` -> rpc error: code = AlreadyExists desc = baton-lucidchart: delete user 101: rpc error: code = AlreadyExists desc = 409 Conflict -> ``` -> The user remains present in the next sync. The reason Lucid gave is not carried in the message. -> -> ### Finding 3 — user status is pinned to ENABLED -> `pkg/connector/users.go:209` -> ```go -> status := v2.UserTrait_Status_STATUS_ENABLED -> ``` -> The status is a constant, and `client.User` (`pkg/connector/client/models.go:9-16`) has no field for Lucid's `enabled`, documented as "Whether the user can authenticate to Lucid. Corresponds to the SCIM active attribute" (https://lucid.readme.io/reference/getuser). -> -> Observed 2026-08-18: after `disable_user` returned `success: true` and the mock recorded `enabled=false`, a re-sync emitted `RESOURCE_STATUS_ENABLED` for that user. The same function drops `User.Roles` from the emitted profile, so a role change made through `update_user` is also absent from the bundle. -> -> ### Finding 4 — the username field never decodes -> `pkg/connector/client/models.go:14` -> ```go -> Usernames string `json:"usernames"` -> ``` -> Lucid's REST `User` model defines `username` (singular). The plural key is not emitted by Lucid, so the field unmarshals empty on every user and the profile key written at `pkg/connector/users.go:216` is always `""`. -> -> ### Reproduction -> 1. 2026-08-18 — build the connector from `origin/main` (79714b3) and from the fix branch. -> 2. Start the bundled test-server with `-protected-users 101`. -> 3. Run a sync against the mock and read back user 105 (`enabled=false` in the mock). -> 4. Delete user 105, then delete it again with `--lucid-content-transfer-user-email` set. -> 5. Delete user 101. -> -> | Check | origin/main | fix branch | -> | -- | -- | -- | -> | Status of a disabled user | `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` | -> -> The harness re-runs deterministically; a second run of the fix-branch case produced identical output. -> -> ### Impact -> With a content-transfer recipient configured — the configuration that preserves a leaving user's documents — a retried delete of an already-removed user returns `PermissionDenied` and never reaches the SCIM delete, so retries cannot converge. A 409 on a protected user returns the gRPC code used for idempotent success while the user remains present. Deactivation and role changes made through the SCIM actions are absent from the synced bundle. The `usernames` profile key is empty on every user. - -## Acceptance checklist (derived from the A/B table — the ticket's pass/fail gate) - -1. **Status of a disabled user** → `RESOURCE_STATUS_DISABLED` (was `ENABLED`). -2. **`username` in profile** → populated (was absent, because the model read `usernames`). -3. **Delete retry after 403** → exit 0 / idempotent success (was exit 1, `PermissionDenied`). -4. **Delete a protected user (409)** → `FailedPrecondition` carrying Lucid's reason (was `AlreadyExists`). -5. Test-server is rewritten from Lucid's **published OpenAPI** (not the connector's assumptions); it is the repro harness the sibling tickets CXH-2282–2285 depend on, so it must be correct. - -## Deltas vs. reference PR #59 (verified, not blindly ported) - -PR #59 implements the four fixes + the test-server rewrite, but its CI review left **1 blocking issue + 3 suggestions** open. This branch ports PR #59's sound approach and closes those: - -- **[BLOCKING] `Enabled bool` cannot distinguish "field absent" from "explicitly false".** PR #59 defaults `userResource` status to `DISABLED`, so any `GET /users` payload missing `enabled` would sync **every** user as disabled — a mass-deactivation footgun. Fix here: `Enabled *bool`, default `STATUS_ENABLED`, flip to `DISABLED` only when `Enabled != nil && !*Enabled`. Confirmed against Lucid docs that `GET /users` (listusers) *does* return `enabled` + `username` (same User schema as getuser), so the field is present on the sync path — the pointer is defence-in-depth, at no cost. -- **[suggestion] `IsConflictError` duplicated `IsAlreadyExistsError` verbatim.** Collapsed to a single `IsConflictError` predicate (the 409/delete-refusal name), dropped the callerless `IsAlreadyExistsError`. -- **[suggestion] test-server `-page-size 0`/negative was unclamped** → empty page + repeating `Link` next token = infinite pagination for the connector under test. Clamp `if size <= 0 { size = lucidPageSize }`. -- **[suggestion] `ci.yaml` comment "Any non-empty bearer is accepted" is now false** and `start-test-server` never passed `-scim-token`. Updated the comment and wired `-scim-token` (from `BATON_LUCID_SCIM_TOKEN`) into the composite action so REST/SCIM tokens cannot silently diverge. -- **Excluded PR #59's unrelated churn:** its `go.mod`/`go.sum`/`vendor/**`/`.versions.yaml` bumps (PR #59 was cut from `79714b3`; this branch is based on the newer `origin/main` `3420066` which already carries the dependency/Go bumps) and its deletion of `tickets/CXH-1488/*.md`. - -## Not in scope (tracked separately, per PR #59) -- CXH-2282 — SCIM wire contract (Content-Type / PatchOp schemas URN divergence; needs a live Enterprise tenant). Test-server models it behind `-strict-scim-doc`, off by default. -- CXH-2283 — ungated capabilities · CXH-2284 — remaining SCIM surface gaps · CXH-2285 — folder/document grant idempotency. From 5bf510be0168cedec4f170917b42de5a0cf94768 Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Fri, 21 Aug 2026 13:34:11 -0300 Subject: [PATCH 04/12] [CXH-2281] Address re-review: clamp test-server seed count, document 403 delete path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- cmd/test-server/main.go | 11 +++++++++-- docs/connector.mdx | 2 ++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/cmd/test-server/main.go b/cmd/test-server/main.go index c6d66383..59fda7b6 100644 --- a/cmd/test-server/main.go +++ b/cmd/test-server/main.go @@ -74,6 +74,11 @@ const ( // ParseForm read against an unbounded body (gosec G107/decompression-bomb). maxFormBody = 1 << 20 + // maxSeedUsers caps POST /_test/users?count= so a mistyped count returns a + // 400 instead of OOMing the mock in makeslice. Well above any real + // pagination-boundary run (PAG-03 exercises a few thousand). + maxSeedUsers = 100_000 + // lucidPageSize is Lucid's page size for paginated REST endpoints. The docs // state a 200-record default and that a larger requested pageSize is clamped // to 200 (reference-rest). @@ -411,8 +416,10 @@ func newMux(s *store, cfg config) *http.ServeMux { // runs without restarting the process. 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) + // Cap count so a fat-fingered request fails loudly with a 400 rather than + // OOMing the mock in makeslice — the value reaches seedUsers unvalidated. + if err != nil || n < 0 || n > maxSeedUsers { + http.Error(w, fmt.Sprintf("count must be an integer in [0, %d]", maxSeedUsers), http.StatusBadRequest) return } s.seedUsers(n) diff --git a/docs/connector.mdx b/docs/connector.mdx index 98dc1e56..3bd9f462 100644 --- a/docs/connector.mdx +++ b/docs/connector.mdx @@ -96,6 +96,8 @@ If you want to use the Lucidchart connector to provision accounts, give the toke 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. + + If the REST record instead returns 403 (Lucid answers 403 both for "not permitted" and "does not exist"), C1 probes SCIM to disambiguate: if SCIM confirms the user is gone the delete proceeds, but if the user still exists and their email could not be read for the transfer, C1 refuses the delete with a `FailedPrecondition` error rather than losing their documents — grant the `account.user:readonly` scope so the email can be read. 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. Carefully copy and save the **refresh token** included in the token response. From d0d01afc0339021ea91695d768ee8ebd4eb84ac7 Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Fri, 21 Aug 2026 14:02:09 -0300 Subject: [PATCH 05/12] [CXH-2281] Make 403+SCIM-probe-fail gRPC code explicit; add test 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) --- pkg/connector/users.go | 10 ++++++++-- pkg/connector/users_test.go | 20 ++++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/pkg/connector/users.go b/pkg/connector/users.go index ebf018b6..479222dc 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -331,8 +331,14 @@ func (o *userBuilder) transferContentBeforeDelete(ctx context.Context, userID st // REST 403 is ambiguous ("gone" or "not permitted"), so confirm with SCIM. exists, existsErr := o.client.ScimUserExists(ctx, userID) if existsErr != nil { - return fmt.Errorf( - "baton-lucidchart: could not resolve user %s for content transfer (%w) and could not confirm whether they still exist: %w", + // Neither source could tell us whether the user still exists, so we + // genuinely cannot decide if deleting is safe. Return codes.Unknown + // deliberately: the gRPC code here is a chosen "indeterminate" + // signal, not whatever errors.As happens to surface first from two + // %w-wrapped chains. Both underlying errors are kept verbatim in the + // detail text so no diagnostic information is lost. + return status.Errorf(codes.Unknown, + "baton-lucidchart: could not resolve user %s for content transfer (%v) and could not confirm whether they still exist: %v", userID, err, existsErr) } if exists { diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index eaa3b640..48d9d20d 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -110,6 +110,26 @@ func TestDelete_RestForbiddenButUserExists_RefusesToDelete(t *testing.T) { require.False(t, routes.scimDelete, "must not delete when content could not be transferred") } +// When REST answers an ambiguous 403 and the SCIM existence probe itself fails, +// the connector cannot tell whether the user is gone or merely unreadable, so it +// must abort rather than risk either destroying un-transferred content or +// deleting a user it never confirmed. The delete must surface an error with a +// deliberate, indeterminate gRPC code (codes.Unknown) — not one that depends on +// errors.As DFS order across two wrapped chains — and SCIM DELETE must never run. +func TestDelete_RestForbiddenAndScimProbeFails_AbortsWithoutDeleting(t *testing.T) { + routes := &deleteRoutes{} + srv := newDeleteServer(t, routes, http.StatusForbidden, http.StatusInternalServerError, http.StatusNoContent) + defer srv.Close() + + err := deleteUser(t, srv, "recipient@example.com") + require.Error(t, err) + require.Equal(t, codes.Unknown, status.Code(err)) + require.True(t, routes.getUser, "REST lookup should be attempted") + require.True(t, routes.scimGet, "SCIM must be probed to disambiguate the 403") + require.False(t, routes.transfer, "no transfer when the email could not be resolved") + require.False(t, routes.scimDelete, "delete must not run when existence could not be confirmed") +} + func TestDelete_GetUserNotFoundWithContentTransfer_SkipsTransferButRunsScimDelete(t *testing.T) { routes := &deleteRoutes{} srv := newDeleteServer(t, routes, http.StatusNotFound, http.StatusNotFound, http.StatusNoContent) From d2a9971a3be30259aff9877647c6e2e00e9153f9 Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Fri, 21 Aug 2026 14:08:24 -0300 Subject: [PATCH 06/12] [CXH-2281] Probe SCIM on undocumented REST 404 before hard delete 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) --- pkg/connector/users.go | 48 ++++++++++++++++++++++++++++--------- pkg/connector/users_test.go | 41 +++++++++++++++++++++++++++++-- 2 files changed, 76 insertions(+), 13 deletions(-) diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 479222dc..b301d802 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -304,13 +304,20 @@ func (o *userBuilder) Delete(ctx context.Context, resourceID *v2.ResourceId, par // transferContentBeforeDelete moves the leaving user's documents to the // configured recipient, resolving their email from the REST record first. // -// A definite REST 404 means the user is already gone: skip the transfer and let -// the SCIM delete (which treats its own 404 as success) run — no SCIM probe, so -// a probe outage can't block an otherwise-valid delete. REST answers 403 both -// for "gone" and for "not permitted", so only there is absence ambiguous; we ask -// SCIM (which 404s specifically for absence) before deciding, because guessing -// would mean either deleting content we failed to move or breaking retry -// idempotency. +// Lucid's GET /v1/users/{id} documents only 200 and 403; 403 is explicitly the +// "does not belong to the authenticated account or does not exist" response, and +// 404 is not a documented response at all. So neither a 403 nor an +// (undocumented) 404 proves the user is actually gone — both are ambiguous as to +// absence, and treating either as "gone" outright risks hard-deleting a user +// whose content we were told to retain. In both cases we ask SCIM (which 404s +// specifically for absence) before deciding, and refuse the delete only when +// SCIM affirmatively reports the user still present. +// +// The two paths differ only in how they treat a SCIM probe error: on the +// documented-but-overloaded 403 an unresolved probe is genuinely indeterminate +// and surfaces as codes.Unknown, while on the undocumented 404 a probe outage is +// treated as "proceed" so it can't block a delete (preserving the original rule +// that a probe outage must not block an otherwise-valid delete). func (o *userBuilder) transferContentBeforeDelete(ctx context.Context, userID string) error { fromUser, err := o.client.GetUser(ctx, userID) switch { @@ -321,10 +328,29 @@ func (o *userBuilder) transferContentBeforeDelete(ctx context.Context, userID st return nil case client.IsNotFoundError(err): - // REST gave a definite 404: the user is gone as far as content transfer - // is concerned. Proceed to the SCIM delete without probing SCIM first, so - // a probe failure (403/405/501 on a tenant without SCIM user GET, or a - // transient 5xx) cannot abort a delete REST already told us is safe. + // REST returned a 404, which Lucid does not document for GET + // /v1/users/{id} — that endpoint only documents 200 and 403, and 403 is + // its "does not exist" response. A 404 is therefore of unknown meaning, + // so we can't assume the user is gone: if it can ever occur for a user + // who still exists, proceeding straight to the hard SCIM delete would + // destroy content the operator asked to retain. Probe SCIM (which 404s + // specifically for absence) and refuse only when it affirmatively + // reports the user still present. Any probe error is treated as + // "proceed" so a probe outage still cannot block an otherwise-valid + // delete. + exists, existsErr := o.client.ScimUserExists(ctx, userID) + if existsErr == nil && exists { + // Present per SCIM but unreadable over REST: deleting would destroy + // content we could not transfer. + return status.Errorf(codes.FailedPrecondition, + "baton-lucidchart: user %s could not be read for content transfer (undocumented REST 404: %v) "+ + "but SCIM reports they still exist; 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) + } + // Either SCIM confirms the user is gone, or the probe itself failed; in + // both cases proceed to the SCIM delete (which treats its own 404 as + // success), so a probe outage cannot abort a valid delete. return nil case client.IsPermissionDeniedError(err): diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index 48d9d20d..af75707f 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -130,15 +130,52 @@ func TestDelete_RestForbiddenAndScimProbeFails_AbortsWithoutDeleting(t *testing. require.False(t, routes.scimDelete, "delete must not run when existence could not be confirmed") } -func TestDelete_GetUserNotFoundWithContentTransfer_SkipsTransferButRunsScimDelete(t *testing.T) { +// Lucid does not document a 404 for GET /v1/users/{id} (only 200 and 403), so a +// 404 is of unknown meaning and must not be trusted as "gone" on its own. When +// SCIM confirms the user really is absent, the delete proceeds so retries of an +// already-processed deprovision converge. +func TestDelete_GetUserNotFoundAndScimUserGone_ProbesThenRunsScimDelete(t *testing.T) { routes := &deleteRoutes{} srv := newDeleteServer(t, routes, http.StatusNotFound, http.StatusNotFound, http.StatusNoContent) defer srv.Close() err := deleteUser(t, srv, "recipient@example.com") require.NoError(t, err) + require.True(t, routes.getUser, "REST lookup should be attempted") + require.True(t, routes.scimGet, "an undocumented 404 must be disambiguated via SCIM") require.False(t, routes.transfer, "transfer must be skipped when the user is not found") - require.True(t, routes.scimDelete) + require.True(t, routes.scimDelete, "delete must run once SCIM confirms the user is gone") +} + +// An undocumented REST 404 with the user still present per SCIM is not an +// absence: deleting would destroy the content the operator asked to retain, so +// Delete must refuse and never call SCIM DELETE. +func TestDelete_GetUserNotFoundButScimUserExists_RefusesToDelete(t *testing.T) { + routes := &deleteRoutes{} + srv := newDeleteServer(t, routes, http.StatusNotFound, http.StatusOK, http.StatusNoContent) + defer srv.Close() + + err := deleteUser(t, srv, "recipient@example.com") + require.Error(t, err) + require.Equal(t, codes.FailedPrecondition, status.Code(err)) + require.True(t, routes.scimGet, "an undocumented 404 must be disambiguated via SCIM") + require.False(t, routes.transfer) + require.False(t, routes.scimDelete, "must not delete a user SCIM says still exists") +} + +// On the undocumented 404 path a SCIM probe outage must NOT block the delete: +// the original rule is that a probe failure cannot abort an otherwise-valid +// delete. Unlike the ambiguous-403 path, a failed probe here means "proceed". +func TestDelete_GetUserNotFoundAndScimProbeFails_ProceedsToScimDelete(t *testing.T) { + routes := &deleteRoutes{} + srv := newDeleteServer(t, routes, http.StatusNotFound, http.StatusInternalServerError, http.StatusNoContent) + defer srv.Close() + + err := deleteUser(t, srv, "recipient@example.com") + require.NoError(t, err) + require.True(t, routes.scimGet, "an undocumented 404 must attempt the SCIM probe") + require.False(t, routes.transfer) + require.True(t, routes.scimDelete, "a probe outage must not block an otherwise-valid delete") } func TestDelete_HappyPath_TransfersThenDeletes(t *testing.T) { From bf994bc2f0c9592fee34ed57336e1204bcbcf8ba Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Fri, 21 Aug 2026 14:26:56 -0300 Subject: [PATCH 07/12] [CXH-2281] Classify 403 SCIM-probe failures: context, retryable, then Unknown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/connector.mdx | 4 +- pkg/connector/users.go | 63 +++++++++++++++++++++++------ pkg/connector/users_test.go | 81 +++++++++++++++++++++++++++++++++---- 3 files changed, 124 insertions(+), 24 deletions(-) diff --git a/docs/connector.mdx b/docs/connector.mdx index 3bd9f462..840dd343 100644 --- a/docs/connector.mdx +++ b/docs/connector.mdx @@ -95,9 +95,7 @@ If you want to use the Lucidchart connector to provision accounts, give the toke 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. - - If the REST record instead returns 403 (Lucid answers 403 both for "not permitted" and "does not exist"), C1 probes SCIM to disambiguate: if SCIM confirms the user is gone the delete proceeds, but if the user still exists and their email could not be read for the transfer, C1 refuses the delete with a `FailedPrecondition` error rather than losing their documents — grant the `account.user:readonly` scope so the email can be read. 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. + 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 429/5xx probe failure returns a retryable `Unavailable`/`ResourceExhausted` code so the platform retries, 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. Carefully copy and save the **refresh token** included in the token response. diff --git a/pkg/connector/users.go b/pkg/connector/users.go index b301d802..e82f3160 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -313,11 +313,15 @@ func (o *userBuilder) Delete(ctx context.Context, resourceID *v2.ResourceId, par // specifically for absence) before deciding, and refuse the delete only when // SCIM affirmatively reports the user still present. // -// The two paths differ only in how they treat a SCIM probe error: on the -// documented-but-overloaded 403 an unresolved probe is genuinely indeterminate -// and surfaces as codes.Unknown, while on the undocumented 404 a probe outage is -// treated as "proceed" so it can't block a delete (preserving the original rule -// that a probe outage must not block an otherwise-valid delete). +// The two paths differ only in how they treat a SCIM probe error. On the +// undocumented 404 a probe outage is treated as "proceed" so it can't block a +// delete (preserving the original rule that a probe outage must not block an +// otherwise-valid delete). On the documented-but-overloaded 403 an unresolved +// probe blocks the delete, but the returned gRPC code is classified so callers +// can react: a cancelled/timed-out probe preserves the context error (so +// errors.Is still matches), a transient failure (429/5xx) surfaces the retryable +// ResourceExhausted/Unavailable code, and only a genuinely indeterminate probe +// falls through to a deliberate codes.Unknown. func (o *userBuilder) transferContentBeforeDelete(ctx context.Context, userID string) error { fromUser, err := o.client.GetUser(ctx, userID) switch { @@ -358,14 +362,47 @@ func (o *userBuilder) transferContentBeforeDelete(ctx context.Context, userID st exists, existsErr := o.client.ScimUserExists(ctx, userID) if existsErr != nil { // Neither source could tell us whether the user still exists, so we - // genuinely cannot decide if deleting is safe. Return codes.Unknown - // deliberately: the gRPC code here is a chosen "indeterminate" - // signal, not whatever errors.As happens to surface first from two - // %w-wrapped chains. Both underlying errors are kept verbatim in the - // detail text so no diagnostic information is lost. - return status.Errorf(codes.Unknown, - "baton-lucidchart: could not resolve user %s for content transfer (%v) and could not confirm whether they still exist: %v", - userID, err, existsErr) + // cannot yet decide if deleting is safe. But the *kind* of probe + // failure drives what we report: collapsing every failure into + // codes.Unknown would hide cancellation from errors.Is downstream and + // discourage the platform from retrying a transient outage that would + // likely succeed. Classify in priority order — cancellation, then + // retryable, then the deliberate indeterminate fallback. + switch { + case ctx.Err() != nil || + errors.Is(existsErr, context.Canceled) || + errors.Is(existsErr, context.DeadlineExceeded): + // The sync was cancelled or timed out while the probe was in + // flight. Preserve the context error via %w so errors.Is keeps + // matching context.Canceled / context.DeadlineExceeded downstream, + // instead of masking cancellation as a generic Unknown. + ctxErr := ctx.Err() + if ctxErr == nil { + ctxErr = existsErr + } + return fmt.Errorf( + "baton-lucidchart: content-transfer existence probe for user %s was cancelled before it could confirm the user (REST said: %v): %w", + userID, err, ctxErr) + case status.Code(existsErr) == codes.ResourceExhausted || + status.Code(existsErr) == codes.Unavailable: + // A transient probe failure — rate-limited (429) or an upstream 5xx, + // both of which the SDK surfaces as ResourceExhausted/Unavailable. + // Preserve that retryable code so the platform re-attempts the + // deprovision (which would likely succeed once SCIM is reachable + // again) rather than parking it behind a non-retryable Unknown. + return status.Errorf(status.Code(existsErr), + "baton-lucidchart: could not resolve user %s for content transfer (%v); the SCIM existence probe failed transiently and should be retried: %v", + userID, err, existsErr) + default: + // Genuinely indeterminate/non-retryable: return codes.Unknown + // deliberately as a chosen "we cannot decide" signal, not whatever + // errors.As happens to surface first from two %w-wrapped chains. + // Both underlying errors are kept verbatim in the detail text so no + // diagnostic information is lost. + return status.Errorf(codes.Unknown, + "baton-lucidchart: could not resolve user %s for content transfer (%v) and could not confirm whether they still exist: %v", + userID, err, existsErr) + } } if exists { // Present but unreadable over REST: deleting would destroy content diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index af75707f..ebabddbc 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -2,6 +2,7 @@ package connector import ( "context" + "errors" "net/http" "net/http/httptest" "testing" @@ -110,15 +111,40 @@ func TestDelete_RestForbiddenButUserExists_RefusesToDelete(t *testing.T) { require.False(t, routes.scimDelete, "must not delete when content could not be transferred") } -// When REST answers an ambiguous 403 and the SCIM existence probe itself fails, -// the connector cannot tell whether the user is gone or merely unreadable, so it -// must abort rather than risk either destroying un-transferred content or -// deleting a user it never confirmed. The delete must surface an error with a -// deliberate, indeterminate gRPC code (codes.Unknown) — not one that depends on -// errors.As DFS order across two wrapped chains — and SCIM DELETE must never run. -func TestDelete_RestForbiddenAndScimProbeFails_AbortsWithoutDeleting(t *testing.T) { +// When REST answers an ambiguous 403 and the SCIM probe fails *transiently* +// (rate-limited or an upstream 5xx, both of which the SDK maps to +// Unavailable/ResourceExhausted), the connector cannot yet tell whether the user +// is gone, so it must abort without deleting — but it must surface the retryable +// code so the platform re-attempts the deprovision instead of parking it behind a +// non-retryable Unknown. SCIM DELETE must never run. +func TestDelete_RestForbiddenAndScimProbeTransient_ReturnsRetryableCode(t *testing.T) { + for _, probeStatus := range []int{http.StatusTooManyRequests, http.StatusServiceUnavailable, http.StatusInternalServerError} { + t.Run(http.StatusText(probeStatus), func(t *testing.T) { + routes := &deleteRoutes{} + srv := newDeleteServer(t, routes, http.StatusForbidden, probeStatus, http.StatusNoContent) + defer srv.Close() + + err := deleteUser(t, srv, "recipient@example.com") + require.Error(t, err) + require.Equal(t, codes.Unavailable, status.Code(err), + "a transient probe failure must surface as a retryable code") + require.True(t, routes.getUser, "REST lookup should be attempted") + require.True(t, routes.scimGet, "SCIM must be probed to disambiguate the 403") + require.False(t, routes.transfer, "no transfer when the email could not be resolved") + require.False(t, routes.scimDelete, "delete must not run when existence could not be confirmed") + }) + } +} + +// When REST answers an ambiguous 403 and the SCIM probe fails in a way that is +// neither cancellation nor transient (here a 400, standing in for any +// non-retryable, indeterminate probe error), the connector cannot decide whether +// the user is gone. It must abort with a deliberate, indeterminate gRPC code +// (codes.Unknown) — not one that depends on errors.As DFS order across two +// wrapped chains — and SCIM DELETE must never run. +func TestDelete_RestForbiddenAndScimProbeIndeterminate_ReturnsUnknown(t *testing.T) { routes := &deleteRoutes{} - srv := newDeleteServer(t, routes, http.StatusForbidden, http.StatusInternalServerError, http.StatusNoContent) + srv := newDeleteServer(t, routes, http.StatusForbidden, http.StatusBadRequest, http.StatusNoContent) defer srv.Close() err := deleteUser(t, srv, "recipient@example.com") @@ -130,6 +156,45 @@ func TestDelete_RestForbiddenAndScimProbeFails_AbortsWithoutDeleting(t *testing. require.False(t, routes.scimDelete, "delete must not run when existence could not be confirmed") } +// When the sync is cancelled while the SCIM probe is in flight on the ambiguous +// 403 path, the delete must abort with an error that still matches +// context.Canceled via errors.Is — collapsing it to codes.Unknown would hide the +// cancellation from downstream retry/backoff logic. SCIM DELETE must never run. +func TestDelete_RestForbiddenAndProbeCancelled_PreservesContextError(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + routes := &deleteRoutes{} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/v1/users/42": + routes.getUser = true + w.WriteHeader(http.StatusForbidden) + case r.Method == http.MethodGet && r.URL.Path == "/Users/lucid-42": + routes.scimGet = true + // Cancel the caller's context mid-probe, then wait for the client to + // abort the request so ScimUserExists returns a context error. + cancel() + <-r.Context().Done() + case r.Method == http.MethodDelete && r.URL.Path == "/Users/lucid-42": + routes.scimDelete = true + w.WriteHeader(http.StatusNoContent) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusInternalServerError) + } + })) + defer srv.Close() + + c := testLucidClient(t, srv.URL, srv.URL) + b := newUserBuilder(c, "recipient@example.com") + _, err := b.Delete(ctx, &v2.ResourceId{Resource: "42"}, nil) + require.Error(t, err) + require.True(t, errors.Is(err, context.Canceled), + "cancellation must remain detectable via errors.Is, got %v", err) + require.True(t, routes.scimGet, "SCIM must be probed to disambiguate the 403") + require.False(t, routes.transfer, "no transfer when the probe was cancelled") + require.False(t, routes.scimDelete, "delete must not run when the probe was cancelled") +} + // Lucid does not document a 404 for GET /v1/users/{id} (only 200 and 403), so a // 404 is of unknown meaning and must not be trusted as "gone" on its own. When // SCIM confirms the user really is absent, the delete proceeds so retries of an From 2ac844d6aedec1e42f6dc9d9bb9447b46adb1d93 Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Fri, 21 Aug 2026 14:33:17 -0300 Subject: [PATCH 08/12] [CXH-2281] Fix errorlint: format REST err as string, wrap only context error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- pkg/connector/users.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/connector/users.go b/pkg/connector/users.go index e82f3160..d958c5e8 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -381,8 +381,8 @@ func (o *userBuilder) transferContentBeforeDelete(ctx context.Context, userID st ctxErr = existsErr } return fmt.Errorf( - "baton-lucidchart: content-transfer existence probe for user %s was cancelled before it could confirm the user (REST said: %v): %w", - userID, err, ctxErr) + "baton-lucidchart: content-transfer existence probe for user %s was cancelled before it could confirm the user (REST said: %s): %w", + userID, err.Error(), ctxErr) case status.Code(existsErr) == codes.ResourceExhausted || status.Code(existsErr) == codes.Unavailable: // A transient probe failure — rate-limited (429) or an upstream 5xx, From b04f0a29c9fca362293abac070f487b444a8f417 Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Mon, 24 Aug 2026 10:09:13 -0300 Subject: [PATCH 09/12] [CXH-2281] Fix probe-retry classification: DeadlineExceeded, drop ResourceExhausted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/connector.mdx | 2 +- pkg/connector/users.go | 22 ++++++++++++---------- pkg/connector/users_test.go | 30 ++++++++++++++++++++---------- 3 files changed, 33 insertions(+), 21 deletions(-) diff --git a/docs/connector.mdx b/docs/connector.mdx index 840dd343..0e12cd88 100644 --- a/docs/connector.mdx +++ b/docs/connector.mdx @@ -95,7 +95,7 @@ If you want to use the Lucidchart connector to provision accounts, give the toke 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: 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 429/5xx probe failure returns a retryable `Unavailable`/`ResourceExhausted` code so the platform retries, 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. + 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. Carefully copy and save the **refresh token** included in the token response. diff --git a/pkg/connector/users.go b/pkg/connector/users.go index d958c5e8..114458d0 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -319,9 +319,9 @@ func (o *userBuilder) Delete(ctx context.Context, resourceID *v2.ResourceId, par // otherwise-valid delete). On the documented-but-overloaded 403 an unresolved // probe blocks the delete, but the returned gRPC code is classified so callers // can react: a cancelled/timed-out probe preserves the context error (so -// errors.Is still matches), a transient failure (429/5xx) surfaces the retryable -// ResourceExhausted/Unavailable code, and only a genuinely indeterminate probe -// falls through to a deliberate codes.Unknown. +// errors.Is still matches), a transient failure (429/5xx surfaces as Unavailable, +// a 408 as DeadlineExceeded) keeps that retryable code, and only a genuinely +// indeterminate probe falls through to a deliberate codes.Unknown. func (o *userBuilder) transferContentBeforeDelete(ctx context.Context, userID string) error { fromUser, err := o.client.GetUser(ctx, userID) switch { @@ -383,13 +383,15 @@ func (o *userBuilder) transferContentBeforeDelete(ctx context.Context, userID st return fmt.Errorf( "baton-lucidchart: content-transfer existence probe for user %s was cancelled before it could confirm the user (REST said: %s): %w", userID, err.Error(), ctxErr) - case status.Code(existsErr) == codes.ResourceExhausted || - status.Code(existsErr) == codes.Unavailable: - // A transient probe failure — rate-limited (429) or an upstream 5xx, - // both of which the SDK surfaces as ResourceExhausted/Unavailable. - // Preserve that retryable code so the platform re-attempts the - // deprovision (which would likely succeed once SCIM is reachable - // again) rather than parking it behind a non-retryable Unknown. + case status.Code(existsErr) == codes.Unavailable || + status.Code(existsErr) == codes.DeadlineExceeded: + // A transient probe failure. The SDK's GrpcCodeFromHTTPStatus maps + // HTTP 429/502/503/504 to codes.Unavailable and HTTP 408 to + // codes.DeadlineExceeded, and its retry layer treats exactly those + // two codes as retryable. Preserve the retryable code so the platform + // re-attempts the deprovision (which would likely succeed once SCIM + // is reachable again) rather than parking it behind a non-retryable + // Unknown. return status.Errorf(status.Code(existsErr), "baton-lucidchart: could not resolve user %s for content transfer (%v); the SCIM existence probe failed transiently and should be retried: %v", userID, err, existsErr) diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index ebabddbc..90061b91 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -111,22 +111,32 @@ func TestDelete_RestForbiddenButUserExists_RefusesToDelete(t *testing.T) { require.False(t, routes.scimDelete, "must not delete when content could not be transferred") } -// When REST answers an ambiguous 403 and the SCIM probe fails *transiently* -// (rate-limited or an upstream 5xx, both of which the SDK maps to -// Unavailable/ResourceExhausted), the connector cannot yet tell whether the user -// is gone, so it must abort without deleting — but it must surface the retryable -// code so the platform re-attempts the deprovision instead of parking it behind a -// non-retryable Unknown. SCIM DELETE must never run. +// When REST answers an ambiguous 403 and the SCIM probe fails *transiently*, the +// connector cannot yet tell whether the user is gone, so it must abort without +// deleting — but it must surface the retryable code the SDK's retry layer honors +// so the platform re-attempts the deprovision instead of parking it behind a +// non-retryable Unknown. The SDK maps HTTP 429/502/503/504 to codes.Unavailable +// and HTTP 408 to codes.DeadlineExceeded, and treats exactly those two codes as +// retryable. SCIM DELETE must never run. func TestDelete_RestForbiddenAndScimProbeTransient_ReturnsRetryableCode(t *testing.T) { - for _, probeStatus := range []int{http.StatusTooManyRequests, http.StatusServiceUnavailable, http.StatusInternalServerError} { - t.Run(http.StatusText(probeStatus), func(t *testing.T) { + cases := []struct { + probeStatus int + wantCode codes.Code + }{ + {http.StatusTooManyRequests, codes.Unavailable}, + {http.StatusServiceUnavailable, codes.Unavailable}, + {http.StatusInternalServerError, codes.Unavailable}, + {http.StatusRequestTimeout, codes.DeadlineExceeded}, + } + for _, tc := range cases { + t.Run(http.StatusText(tc.probeStatus), func(t *testing.T) { routes := &deleteRoutes{} - srv := newDeleteServer(t, routes, http.StatusForbidden, probeStatus, http.StatusNoContent) + srv := newDeleteServer(t, routes, http.StatusForbidden, tc.probeStatus, http.StatusNoContent) defer srv.Close() err := deleteUser(t, srv, "recipient@example.com") require.Error(t, err) - require.Equal(t, codes.Unavailable, status.Code(err), + require.Equal(t, tc.wantCode, status.Code(err), "a transient probe failure must surface as a retryable code") require.True(t, routes.getUser, "REST lookup should be attempted") require.True(t, routes.scimGet, "SCIM must be probed to disambiguate the 403") From 34937fbbcb3af7b84e67c0c94224e226144ee28f Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Mon, 24 Aug 2026 12:56:56 -0300 Subject: [PATCH 10/12] [CXH-2281] Clarify retryable-code comment: 429 and 5xx map to Unavailable Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/connector/users.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 114458d0..2c9b0080 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -386,7 +386,7 @@ func (o *userBuilder) transferContentBeforeDelete(ctx context.Context, userID st case status.Code(existsErr) == codes.Unavailable || status.Code(existsErr) == codes.DeadlineExceeded: // A transient probe failure. The SDK's GrpcCodeFromHTTPStatus maps - // HTTP 429/502/503/504 to codes.Unavailable and HTTP 408 to + // HTTP 429 and 5xx to codes.Unavailable and HTTP 408 to // codes.DeadlineExceeded, and its retry layer treats exactly those // two codes as retryable. Preserve the retryable code so the platform // re-attempts the deprovision (which would likely succeed once SCIM From 5b1a22b5018bf04298d80248c298a0e6639645a0 Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Mon, 24 Aug 2026 13:05:09 -0300 Subject: [PATCH 11/12] [CXH-2281] Note 501 exception in retryable-code comment 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) --- pkg/connector/users.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 2c9b0080..572af2e4 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -386,7 +386,8 @@ func (o *userBuilder) transferContentBeforeDelete(ctx context.Context, userID st case status.Code(existsErr) == codes.Unavailable || status.Code(existsErr) == codes.DeadlineExceeded: // A transient probe failure. The SDK's GrpcCodeFromHTTPStatus maps - // HTTP 429 and 5xx to codes.Unavailable and HTTP 408 to + // HTTP 429 and 5xx (except 501, which maps to codes.Unimplemented) + // to codes.Unavailable and HTTP 408 to // codes.DeadlineExceeded, and its retry layer treats exactly those // two codes as retryable. Preserve the retryable code so the platform // re-attempts the deprovision (which would likely succeed once SCIM From 0d077cff4130ee51bdac3e88b9f2572eaa9c333c Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Thu, 27 Aug 2026 14:54:11 -0300 Subject: [PATCH 12/12] [CXH-2281] Extract probe-failure classifier; add IsRetryableError helper 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) --- pkg/connector/client/helpers.go | 11 ++++ pkg/connector/users.go | 95 ++++++++++++++++----------------- 2 files changed, 58 insertions(+), 48 deletions(-) diff --git a/pkg/connector/client/helpers.go b/pkg/connector/client/helpers.go index c527d4f2..00162357 100644 --- a/pkg/connector/client/helpers.go +++ b/pkg/connector/client/helpers.go @@ -28,3 +28,14 @@ func IsPermissionDeniedError(err error) bool { func IsConflictError(err error) bool { return status.Code(err) == codes.AlreadyExists } + +// IsRetryableError reports whether err carries a gRPC status code that the SDK's +// retry layer re-attempts. The SDK gate (vendor/.../pkg/retry/retry.go) treats +// exactly codes.Unavailable and codes.DeadlineExceeded as retryable, and +// GrpcCodeFromHTTPStatus maps HTTP 429/5xx (except 501 → Unimplemented) to +// Unavailable and HTTP 408 to DeadlineExceeded. Kept here alongside the other +// predicates so the classification cannot drift from the SDK gate. +func IsRetryableError(err error) bool { + code := status.Code(err) + return code == codes.Unavailable || code == codes.DeadlineExceeded +} diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 572af2e4..524e1bc8 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -284,10 +284,8 @@ func (o *userBuilder) Delete(ctx context.Context, resourceID *v2.ResourceId, par annos, err := o.client.ScimDeleteUser(ctx, userID) switch { - case err == nil: - return annos, nil - case client.IsNotFoundError(err): - // Already deleted is success. + case err == nil, client.IsNotFoundError(err): + // nil = just deleted; not-found = already deleted. Both are success. return annos, nil case client.IsConflictError(err): // Raw 409 surfaces as AlreadyExists, which reads as an idempotent @@ -362,50 +360,9 @@ func (o *userBuilder) transferContentBeforeDelete(ctx context.Context, userID st exists, existsErr := o.client.ScimUserExists(ctx, userID) if existsErr != nil { // Neither source could tell us whether the user still exists, so we - // cannot yet decide if deleting is safe. But the *kind* of probe - // failure drives what we report: collapsing every failure into - // codes.Unknown would hide cancellation from errors.Is downstream and - // discourage the platform from retrying a transient outage that would - // likely succeed. Classify in priority order — cancellation, then - // retryable, then the deliberate indeterminate fallback. - switch { - case ctx.Err() != nil || - errors.Is(existsErr, context.Canceled) || - errors.Is(existsErr, context.DeadlineExceeded): - // The sync was cancelled or timed out while the probe was in - // flight. Preserve the context error via %w so errors.Is keeps - // matching context.Canceled / context.DeadlineExceeded downstream, - // instead of masking cancellation as a generic Unknown. - ctxErr := ctx.Err() - if ctxErr == nil { - ctxErr = existsErr - } - return fmt.Errorf( - "baton-lucidchart: content-transfer existence probe for user %s was cancelled before it could confirm the user (REST said: %s): %w", - userID, err.Error(), ctxErr) - case status.Code(existsErr) == codes.Unavailable || - status.Code(existsErr) == codes.DeadlineExceeded: - // A transient probe failure. The SDK's GrpcCodeFromHTTPStatus maps - // HTTP 429 and 5xx (except 501, which maps to codes.Unimplemented) - // to codes.Unavailable and HTTP 408 to - // codes.DeadlineExceeded, and its retry layer treats exactly those - // two codes as retryable. Preserve the retryable code so the platform - // re-attempts the deprovision (which would likely succeed once SCIM - // is reachable again) rather than parking it behind a non-retryable - // Unknown. - return status.Errorf(status.Code(existsErr), - "baton-lucidchart: could not resolve user %s for content transfer (%v); the SCIM existence probe failed transiently and should be retried: %v", - userID, err, existsErr) - default: - // Genuinely indeterminate/non-retryable: return codes.Unknown - // deliberately as a chosen "we cannot decide" signal, not whatever - // errors.As happens to surface first from two %w-wrapped chains. - // Both underlying errors are kept verbatim in the detail text so no - // diagnostic information is lost. - return status.Errorf(codes.Unknown, - "baton-lucidchart: could not resolve user %s for content transfer (%v) and could not confirm whether they still exist: %v", - userID, err, existsErr) - } + // cannot yet decide if deleting is safe. Classify the probe failure so + // callers can react rather than seeing every failure as codes.Unknown. + return classifyProbeFailure(ctx, userID, err, existsErr) } if exists { // Present but unreadable over REST: deleting would destroy content @@ -424,6 +381,48 @@ func (o *userBuilder) transferContentBeforeDelete(ctx context.Context, userID st } } +// classifyProbeFailure turns a failed SCIM existence probe into the gRPC error +// to return when a documented-but-overloaded REST 403 left the user's existence +// undecided. The *kind* of probe failure drives what we report: collapsing every +// failure into codes.Unknown would hide cancellation from errors.Is downstream +// and discourage the platform from retrying a transient outage that would likely +// succeed. Classification is in priority order — cancellation, then retryable, +// then the deliberate indeterminate fallback. restErr is the original REST 403. +func classifyProbeFailure(ctx context.Context, userID string, restErr, existsErr error) error { + switch { + case ctx.Err() != nil || + errors.Is(existsErr, context.Canceled) || + errors.Is(existsErr, context.DeadlineExceeded): + // The sync was cancelled or timed out while the probe was in flight. + // Preserve the context error via %w so errors.Is keeps matching + // context.Canceled / context.DeadlineExceeded downstream, instead of + // masking cancellation as a generic Unknown. + ctxErr := ctx.Err() + if ctxErr == nil { + ctxErr = existsErr + } + return fmt.Errorf( + "baton-lucidchart: content-transfer existence probe for user %s was cancelled before it could confirm the user (REST said: %s): %w", + userID, restErr.Error(), ctxErr) + case client.IsRetryableError(existsErr): + // A transient probe failure (429/5xx → Unavailable, 408 → DeadlineExceeded). + // Preserve the retryable code so the platform re-attempts the deprovision + // (which would likely succeed once SCIM is reachable again) rather than + // parking it behind a non-retryable Unknown. + return status.Errorf(status.Code(existsErr), + "baton-lucidchart: could not resolve user %s for content transfer (%v); the SCIM existence probe failed transiently and should be retried: %v", + userID, restErr, existsErr) + default: + // Genuinely indeterminate/non-retryable: return codes.Unknown deliberately + // as a chosen "we cannot decide" signal, not whatever errors.As happens to + // surface first from two %w-wrapped chains. Both underlying errors are kept + // verbatim in the detail text so no diagnostic information is lost. + return status.Errorf(codes.Unknown, + "baton-lucidchart: could not resolve user %s for content transfer (%v) and could not confirm whether they still exist: %v", + userID, restErr, existsErr) + } +} + func newUserBuilder(client *client.LucidchartClient, contentTransferUserEmail string) *userBuilder { return &userBuilder{ client: client,