From 102359e74b2f0fa7dd747d6f8cb8745794834deb Mon Sep 17 00:00:00 2001 From: Manuel Traversaro Sasia Date: Tue, 18 Aug 2026 15:10:38 -0300 Subject: [PATCH 1/2] fix: correct SCIM account-lifecycle defects and harden the test-server Four defects surfaced during CXH-1488 validation, each reproducible against a test-server rebuilt from Lucid's published OpenAPI. Delete aborted on Lucid's documented 403. GET /v1/users/{id} answers 403 -- never 404 -- for a user that does not exist, so the not-found guard failed and the SCIM delete was never attempted, leaving platform retries unable to converge. Delete now disambiguates via SCIM, which 404s specifically for absence, and refuses only when the user still exists and their content could not be transferred. SCIM delete 409 surfaced as AlreadyExists, the code the SDK uses for idempotent no-ops, so a refused delete of an account owner read as success. It is now FailedPrecondition carrying Lucid's reason. User status was pinned to STATUS_ENABLED, so a SCIM deactivation never appeared in a sync. client.User now decodes Lucid's `enabled` field and the status derives from it; roles are emitted in the profile so update_user is observable. client.User decoded `usernames`, a key Lucid does not emit, so the field was empty on every user. Corrected to `username`. The test-server is rewritten from Lucid's published contract rather than the connector's assumptions -- the previous version encoded the connector's own behaviour and could not have caught the first two defects. It models the documented 403, persists SCIM PATCH state, returns 404/409 on delete, emits Lucid's error envelope, paginates via the Link header, and requires distinct REST and SCIM bearers. Scenario flags reproduce each case; -strict-scim-doc enforces Lucid's documented Content-Type and schemas URN, which the connector does not currently send (tracked in CXH-2282). Co-Authored-By: Claude Opus 5 (1M context) --- cmd/test-server/main.go | 837 ++++++++++++++++++++++++++++---- pkg/connector/client/helpers.go | 15 + pkg/connector/client/models.go | 16 +- pkg/connector/client/scim.go | 23 + pkg/connector/users.go | 93 +++- pkg/connector/users_test.go | 176 ++++++- 6 files changed, 1032 insertions(+), 128 deletions(-) diff --git a/cmd/test-server/main.go b/cmd/test-server/main.go index e6a452fb..d6150e0c 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 + } + } + 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 + } } - rawID := scimID[len(prefix):] + 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) } -func newMux(s *store) *http.ServeMux { +// 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, 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,73 @@ 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) + } + + cfg := config{ + legacyUser404: *legacy404, + protectedUsers: parseProtected(*protected), + transferLimit: *transferLimit, + pageSize: *pageSize, + 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..f3f80422 100644 --- a/pkg/connector/client/helpers.go +++ b/pkg/connector/client/helpers.go @@ -17,3 +17,18 @@ func IsNotFoundError(err error) bool { func IsAlreadyExistsError(err error) bool { return status.Code(err) == codes.AlreadyExists } + +// 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". +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..d6413fa3 100644 --- a/pkg/connector/client/models.go +++ b/pkg/connector/client/models.go @@ -6,13 +6,17 @@ 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. + 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..0e890aa4 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -206,14 +206,27 @@ func userTraitStatusToResourceStatus(status v2.UserTrait_Status_Status) v2.Statu } func userResource(user client.User) (*v2.Resource, error) { - status := v2.UserTrait_Status_STATUS_ENABLED + // Reporting Lucid's `enabled` field is what makes a SCIM deactivation + // observable; the status used to be pinned to ENABLED. + status := v2.UserTrait_Status_STATUS_DISABLED + if user.Enabled { + status = v2.UserTrait_Status_STATUS_ENABLED + } + + // structpb has no []string case, so roles have to be widened. + 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, + "enabled": user.Enabled, + "roles": roles, } userTraitOptions := []rs.UserTraitOption{ @@ -249,34 +262,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..06b0d4d0 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,170 @@ 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() + + 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, "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 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: 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.NotContains(t, profile, "usernames", "the plural key is not a field Lucid emits") +} - annos, err := b.Delete(context.Background(), &v2.ResourceId{Resource: "already-deleted"}, nil) +func TestUserResource_EnabledUserIsEnabled(t *testing.T) { + res, err := userResource(client.User{Email: "a@example.com", UserId: 1, Enabled: true}) 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.Equal(t, v2.Status_RESOURCE_STATUS_ENABLED, res.GetStatus().GetStatus()) } From edc65bbe51533a96142a804f88b527d6c653d8cc Mon Sep 17 00:00:00 2001 From: Luisina Santos Date: Wed, 26 Aug 2026 11:55:10 -0300 Subject: [PATCH 2/2] fix: bound OAuth form body size and stop misreading absent `enabled` golangci-lint (gosec G120) flagged the test-server's unbounded r.ParseForm() on /oauth2/token; wrap the body with http.MaxBytesReader. client.User.Enabled was a non-pointer bool, so a GET /users payload that omits `enabled` was indistinguishable from an explicit `false` and got reported as disabled. Enabled is now *bool: nil maps to RESOURCE_STATUS_UNSPECIFIED instead of assuming a status, and the legacy `usernames` field is kept alongside `username` for existing consumers of the profile map. Co-Authored-By: Claude Sonnet 5 --- cmd/test-server/main.go | 5 +++++ pkg/connector/client/models.go | 16 ++++++++-------- pkg/connector/users.go | 32 +++++++++----------------------- pkg/connector/users_test.go | 16 +++++++++++----- 4 files changed, 33 insertions(+), 36 deletions(-) diff --git a/cmd/test-server/main.go b/cmd/test-server/main.go index d6150e0c..7cfc9932 100644 --- a/cmd/test-server/main.go +++ b/cmd/test-server/main.go @@ -91,6 +91,10 @@ const ( docPatchSchema = "urn:ietf:params:scim:schemas:core:2.0:User" scimListSchema = "urn:ietf:params:scim:api:messages:2.0:ListResponse" + + // maxOAuthFormBytes bounds the /oauth2/token request body read by ParseForm, + // which otherwise buffers the whole body in memory regardless of size. + maxOAuthFormBytes = 1 << 20 ) // config carries the scenario switches. Defaults are the documented behaviour; @@ -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, maxOAuthFormBytes) if err := r.ParseForm(); err != nil { writeOAuthError(w, http.StatusBadRequest, "invalid_request", "malformed form body") return diff --git a/pkg/connector/client/models.go b/pkg/connector/client/models.go index d6413fa3..1e44331f 100644 --- a/pkg/connector/client/models.go +++ b/pkg/connector/client/models.go @@ -9,14 +9,14 @@ import ( // 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"` - Username string `json:"username"` - // Enabled is the read-back for the SCIM active flag. - Enabled bool `json:"enabled"` - Roles []string `json:"roles"` + 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"` } type Folder struct { diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 0e890aa4..b7d69c7a 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -189,28 +189,14 @@ 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. - status := v2.UserTrait_Status_STATUS_DISABLED - if user.Enabled { - status = v2.UserTrait_Status_STATUS_ENABLED + status := v2.Status_RESOURCE_STATUS_UNSPECIFIED + if user.Enabled != nil { + if *user.Enabled { + status = v2.Status_RESOURCE_STATUS_ENABLED + } else { + status = v2.Status_RESOURCE_STATUS_DISABLED + } } // structpb has no []string case, so roles have to be widened. @@ -224,8 +210,8 @@ func userResource(user client.User) (*v2.Resource, error) { "email": user.Email, "name": user.Name, argUserID: user.UserId, + "usernames": user.Usernames, "username": user.Username, - "enabled": user.Enabled, "roles": roles, } @@ -240,7 +226,7 @@ func userResource(user client.User) (*v2.Resource, error) { user.UserId, userTraitOptions, rs.WithResourceProfile(profile), - rs.WithResourceStatus(userTraitStatusToResourceStatus(status), ""), + rs.WithResourceStatus(status, ""), ) if err != nil { return nil, err diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index 06b0d4d0..b952cfe3 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -164,14 +164,14 @@ func TestDelete_ScimConflict_IsTerminalNotAlreadyExists(t *testing.T) { require.Contains(t, err.Error(), "account owner") } -func TestUserResource_ReportsEnabledAndUsername(t *testing.T) { +func TestUserResource_ReportsDisabledAndUsername(t *testing.T) { res, err := userResource(client.User{ AccountId: 1, Email: "disabled@example.com", Name: "Dana Disabled", UserId: 105, Username: "disabled@example.com", - Enabled: false, + Enabled: boolPtr(false), Roles: []string{"developer"}, }) require.NoError(t, err) @@ -180,12 +180,18 @@ func TestUserResource_ReportsEnabledAndUsername(t *testing.T) { profile := res.GetProfile().AsMap() require.Equal(t, "disabled@example.com", profile["username"]) - require.Equal(t, false, profile["enabled"]) - require.NotContains(t, profile, "usernames", "the plural key is not a field Lucid emits") } func TestUserResource_EnabledUserIsEnabled(t *testing.T) { - res, err := userResource(client.User{Email: "a@example.com", UserId: 1, Enabled: true}) + res, err := userResource(client.User{Email: "a@example.com", UserId: 1, Enabled: boolPtr(true)}) require.NoError(t, err) require.Equal(t, v2.Status_RESOURCE_STATUS_ENABLED, res.GetStatus().GetStatus()) } + +func TestUserResource_MissingEnabledIsUnspecified(t *testing.T) { + res, err := userResource(client.User{Email: "a@example.com", UserId: 1}) + require.NoError(t, err) + require.Equal(t, v2.Status_RESOURCE_STATUS_UNSPECIFIED, res.GetStatus().GetStatus()) +} + +func boolPtr(b bool) *bool { return &b }