Add SCIM 2.0 support to enterprise IdP plugin#20
Open
Tbsheff wants to merge 17 commits intovercel-labs:mainfrom
Open
Add SCIM 2.0 support to enterprise IdP plugin#20Tbsheff wants to merge 17 commits intovercel-labs:mainfrom
Tbsheff wants to merge 17 commits intovercel-labs:mainfrom
Conversation
New first-class `idp` service plugin providing a local enterprise identity provider for end-to-end testing of OIDC and SAML SSO flows. OIDC Provider: - OpenID Connect discovery, JWKS, authorize, token, userinfo, revoke, logout - RS256 JWT signing with auto-generated or seeded RSA keys - Authorization code flow with PKCE (S256/plain) - Refresh token support with rotation (offline_access scope) - Custom claim mappings via dot-path resolution - client_secret_basic and client_secret_post authentication - Browser sign-in picker matching existing emulate UX SAML 2.0 IdP: - IdP metadata endpoint with X.509 signing certificate - SP-initiated SSO with HTTP-Redirect binding - Signed SAML assertions (RSA-SHA256) via xml-crypto - Auto-submit POST form to Assertion Consumer Service - Microsoft Entra ID claim URI presets (nameidentifier, emailaddress, etc.) - Configurable attribute_mappings and name_id_format per service provider - RelayState roundtrip support Shared: - Strict mode: enforces client/SP validation, requires PKCE, disables debug - Seeded users with groups, roles, and custom attributes - Debug endpoint for inspecting sessions, codes, and tokens - CLI integration: start, list, init, programmatic API - Zero-config boot with sensible defaults Dependencies: - xml-crypto ^6 (XML signing with Exclusive C14N) - @peculiar/x509 ^2 (self-signed X.509 certificate generation) Testing: - 67 plugin tests (TDD: crypto, seed, OIDC integration, SAML integration) - All 160+ monorepo tests passing
- Fix require() in ESM module — use static import - Validate redirect_uri before constructing URL (prevents 500) - Validate redirect_uri and client_id match in token exchange - Strict mode requires PKCE code_challenge - Fix TokenMap id type (use entity id, not uid string) - Validate client_id on refresh token exchange - Escape clientName in HTML output (XSS prevention) - XML-escape all interpolated SAML response parameters - Add xs namespace declaration for xsi:type attribute - Add TTL expiry check for pending SAML requests - Strict mode rejects unknown SPs even when none configured - Update service list label to include SAML
- Type grant handler context as Context instead of any - Add comment explaining reflect-metadata import - Add test: JSON Content-Type on /token endpoint - Add test: unsupported_grant_type rejection - Add test: invalid SAML request ref rejection - Use timingSafeEqual for PKCE verification
…d code - Add eviction cap on revokedTokens and tokenClients (10k max) - Remove dead sessions code (never written to) - Validate response_type=code in /authorize (reject unsupported types) - Wrap new URL() in try/catch in /logout endpoint - Fix client_id binding: reject mismatch when code was bound to a client - Only issue refresh token on refresh grant if offline_access in scope - HTML-escape user inputs in error messages (XSS prevention) - Remove dead getCertificatePem from crypto.ts - Move @xmldom/xmldom to devDependencies - Consolidate duplicate @internal/core imports in saml.ts
1. Regenerate lockfile to fix frozen-lockfile CI failures 2. Check revoked tokens in /userinfo so revocation is enforced 3. Return error for unknown uid instead of silently falling back to first user 4. Update all tests to use real user uids from the store 5. Add /logout endpoint test coverage (5 cases) 6. Add test verifying revoked tokens are rejected at /userinfo
…idp-plugin # Conflicts: # packages/emulate/package.json # packages/emulate/src/api.ts # packages/emulate/src/commands/init.ts # packages/emulate/src/commands/start.ts # pnpm-lock.yaml
- CRITICAL: Remove SAML user fallback (auth bypass) — unknown uid now returns error - Add memory cap on pendingSamlRequests and pendingCodes maps - Increase eviction threshold to 50k with safety-valve documentation - Add test for response_type validation - Add test for PKCE strict mode enforcement - Document intentional ID token claim inclusion
- SCIM schema URNs, ServiceProviderConfig, ResourceTypes - TypeScript interfaces for SCIM User, Group, ListResponse, Error, PatchOp - Bidirectional schema mapper (IdpUser ↔ ScimUser, IdpGroup ↔ ScimGroup) - Enterprise User extension mapping (department, employeeNumber, manager) - Response builders (ListResponse, Error, pagination) - 12 schema mapper tests
- Supports add, replace, remove operations - Dot-path and URN-prefixed path resolution - Array element removal with value filters - Top-level merge when no path specified - Multiple sequential operations - 11 PATCH handler tests
- Recursive descent parser for SCIM filter grammar - Tokenizer, AST parser, evaluator - All operators: eq, ne, co, sw, ew, gt, ge, lt, le, pr - Logical: and, or, not with nested parentheses - Value path filters: emails[type eq "work"].value - URN-prefixed enterprise extension paths - Case-insensitive string comparison per spec - 20 filter parser tests
- SCIM bearer token auth middleware with constant-time comparison - IdpSeedConfig extended with scim.bearer_token and scim.clients - 16 SCIM provider endpoints: ServiceProviderConfig, Schemas, ResourceTypes, Users CRUD (list, get, create, replace, patch, delete), Groups CRUD (list, get, create, replace, patch, delete) - Group membership sync: PATCH add/remove members updates user.groups[] - Full filter support on list endpoints - Pagination with SCIM 1-based startIndex - Content-Type: application/scim+json - userName uniqueness enforcement (409 on conflict) - 20+ route integration tests
- ScimClient class for push provisioning to configured targets - Fire-and-forget with error logging - getScimClients() helper reads from store config - Init template includes SCIM bearer token - List description updated with SCIM endpoints - 5 client tests (mock fetch) - All tests passing
- CRITICAL: Group PATCH returns fresh data after mutation (re-fetch from store) - MAJOR: Validate userName required on POST User (return 400) - MAJOR: Type-aware comparison for gt/ge/lt/le filter operators - MAJOR: Group PATCH replace handles members path (full member list replacement) - MAJOR: Wire ScimClient push provisioning into route handlers - MINOR: Remove dead code loop in filter parser - MINOR: Validate count param for NaN
…pe coercion - CRITICAL: undefined treated as null for eq/ne (SCIM spec compliance) - CRITICAL: URN + dot-path resolution (urn:...:User:name.givenName) - MAJOR: Recursion depth limit of 50 (prevents stack overflow DoS) - MAJOR: Type coercion consistency for eq (number/string interop) - MAJOR: Empty string not coerced to 0 in comparisons - Short-circuit evaluation for and/or
…hip sync, type safety - CRITICAL: Group POST now handles members in request body - CRITICAL: PUT/PATCH preserves attributes via deep merge (no more data destruction) - CRITICAL: Schema mapper uses !== undefined checks (empty strings allowed) - MAJOR: Auth errors return application/scim+json content type - MAJOR: Patch add creates intermediate objects for dot-paths - MAJOR: Patch add single value to array appends instead of replacing - MAJOR: Client URL strips trailing slash (prevents double-slash) - MAJOR: Group rename updates user.groups references
- Groups CRUD: get, PUT replace, DELETE with user cleanup - Missing userName validation (400 on POST) - Exact count assertions replacing weak toBeGreaterThanOrEqual - Manager attribute mapping (object and string forms) - Filter: missing attribute treated as null, recursion depth limit - Client: updateUser PUT, deleteGroup, trailing slash - SCIM error response body format verification - Auth: open access mode, missing auth header - 20+ new test cases
- Add debug logging for all SCIM CRUD operations - Wrap c.req.json() in try-catch for malformed JSON (returns 400 SCIM error) - Validate parsed IDs for NaN (returns 400 instead of misleading 404) - Type scimJson helper properly (Context instead of any) - Import TokenMap type in test instead of inlining - Add comment explaining SCIM auth middleware pattern
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
@Tbsheff is attempting to deploy a commit to the Vercel Labs Team on Vercel. A member of the Team first needs to authorize it. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Adds a new enterprise IdP plugin package (@internal/idp) and wires it into the emulate CLI/API, extending the emulator suite with OIDC + SAML + SCIM 2.0 capabilities for local/CI identity testing.
Changes:
- Introduces
@internal/idpplugin with OIDC provider, SAML IdP, and SCIM v2 provider/client utilities. - Registers the new
idpservice inemulateCLI commands and programmatic API, including default init config. - Adds extensive Vitest coverage for OIDC/SAML/SCIM behavior and bundling updates for Node ESM/CJS interop.
Reviewed changes
Copilot reviewed 36 out of 37 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds lock entries for IdP deps |
| packages/emulate/tsup.config.ts | Node platform + ESM require banner |
| packages/emulate/src/commands/start.ts | Registers/seeds idp service |
| packages/emulate/src/commands/list.ts | Lists IdP endpoints in CLI |
| packages/emulate/src/commands/init.ts | Adds default IdP seed config |
| packages/emulate/src/api.ts | Adds idp to programmatic API |
| packages/emulate/src/tests/api.test.ts | Smoke test for IdP discovery |
| packages/emulate/package.json | Adds @internal/idp workspace dep |
| packages/@internal/idp/tsup.config.ts | Builds IdP package with tsup |
| packages/@internal/idp/tsconfig.json | TS config for IdP package |
| packages/@internal/idp/src/store.ts | Defines IdP store collections |
| packages/@internal/idp/src/scim/types.ts | SCIM type definitions |
| packages/@internal/idp/src/scim/schema-mapper.ts | IdP ↔ SCIM mapping helpers |
| packages/@internal/idp/src/scim/response.ts | SCIM list/error response helpers |
| packages/@internal/idp/src/scim/patch-handler.ts | SCIM PATCH operation executor |
| packages/@internal/idp/src/scim/filter-parser.ts | RFC7644 filter parser/evaluator |
| packages/@internal/idp/src/scim/constants.ts | SCIM URNs + resource metadata |
| packages/@internal/idp/src/scim/client.ts | SCIM push-provisioning client |
| packages/@internal/idp/src/scim/auth.ts | SCIM bearer auth middleware |
| packages/@internal/idp/src/saml-xml.ts | SAML XML builders + signing |
| packages/@internal/idp/src/saml-constants.ts | SAML constants + Entra mappings |
| packages/@internal/idp/src/routes/scim.ts | SCIM v2 route handlers |
| packages/@internal/idp/src/routes/saml.ts | SAML metadata + SSO routes |
| packages/@internal/idp/src/routes/oidc.ts | OIDC provider routes |
| packages/@internal/idp/src/index.ts | Plugin entry + seeding |
| packages/@internal/idp/src/helpers.ts | Ephemeral maps + helpers |
| packages/@internal/idp/src/entities.ts | IdP entity interfaces |
| packages/@internal/idp/src/crypto.ts | JWT signing, PKCE, X509 cert gen |
| packages/@internal/idp/src/tests/scim.test.ts | SCIM integration tests |
| packages/@internal/idp/src/tests/scim-patch.test.ts | PATCH handler unit tests |
| packages/@internal/idp/src/tests/scim-mapper.test.ts | Mapper unit tests |
| packages/@internal/idp/src/tests/scim-filter.test.ts | Filter parser unit tests |
| packages/@internal/idp/src/tests/scim-client.test.ts | SCIM client unit tests |
| packages/@internal/idp/src/tests/saml.test.ts | SAML unit + integration tests |
| packages/@internal/idp/src/tests/idp.test.ts | OIDC integration test suite |
| packages/@internal/idp/src/tests/crypto.test.ts | Crypto utility unit tests |
| packages/@internal/idp/package.json | New IdP package manifest |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+323
to
+327
| const input = scimGroupToIdpGroupInput(body); | ||
| const newGroup = idp.groups.insert({ | ||
| name: (input.name as string) ?? "", | ||
| display_name: (input.display_name as string) ?? "", | ||
| }); |
There was a problem hiding this comment.
Create Group does not validate required fields. If the request omits displayName, scimGroupToIdpGroupInput returns {} and this inserts a group with empty name / display_name, which will break membership sync and filtering. Validate displayName is a non-empty string and return a SCIM 400 invalidValue when missing/invalid.
Comment on lines
+407
to
+409
| const operations = body.Operations ?? []; | ||
|
|
||
| for (const op of operations) { |
There was a problem hiding this comment.
PATCH /Groups assumes every operation has a string op ((op.op as string).toLowerCase()). If op is missing or not a string, this will throw and bypass SCIM error formatting. Validate that each operation is an object with op as a string before processing, and return a SCIM 400 invalidValue for invalid operations.
Suggested change
| const operations = body.Operations ?? []; | |
| for (const op of operations) { | |
| const operations = body.Operations; | |
| if (!Array.isArray(operations)) { | |
| return scimJson( | |
| c, | |
| scimError(400, "Invalid PATCH body: Operations must be an array", "invalidValue"), | |
| 400, | |
| ); | |
| } | |
| for (const op of operations) { | |
| if (!op || typeof op !== "object" || typeof (op as { op?: unknown }).op !== "string") { | |
| return scimJson( | |
| c, | |
| scimError(400, "Invalid PATCH operation: missing or invalid 'op'", "invalidValue"), | |
| 400, | |
| ); | |
| } |
Comment on lines
+251
to
+252
| const updated = idp.users.update(id, { | ||
| email: (input.email as string) ?? user.email, |
There was a problem hiding this comment.
PATCH /Users updates email from the patched resource but does not re-check uniqueness the way POST and PUT do. A PATCH that replaces userName can silently create duplicate emails. After deriving input.email, run the same uniqueness check as PUT (allowing the current user id).
Suggested change
| const updated = idp.users.update(id, { | |
| email: (input.email as string) ?? user.email, | |
| const newEmail = (input.email as string) ?? user.email; | |
| // Ensure email uniqueness (same behavior as PUT; allow current user id) | |
| if (newEmail !== user.email) { | |
| const existingUsers = idp.users.all(); | |
| const conflictingUser = existingUsers.find(u => u.email === newEmail && u.id !== id); | |
| if (conflictingUser) { | |
| return scimJson( | |
| c, | |
| scimError(409, "A user with this email already exists", "uniqueness"), | |
| 409, | |
| ); | |
| } | |
| } | |
| const updated = idp.users.update(id, { | |
| email: newEmail, |
Comment on lines
+143
to
+155
| parent = current; | ||
| if (current == null || typeof current !== "object") { | ||
| if (createIntermediates) { | ||
| (parent as Record<string, unknown>)[parts[i]] = {}; | ||
| current = (parent as Record<string, unknown>)[parts[i]]; | ||
| } else { | ||
| return { target: undefined, parent: null, key }; | ||
| } | ||
| } else { | ||
| current = (current as Record<string, unknown>)[parts[i]]; | ||
| if (current === undefined && createIntermediates) { | ||
| (parent as Record<string, unknown>)[parts[i]] = {}; | ||
| current = (parent as Record<string, unknown>)[parts[i]]; |
There was a problem hiding this comment.
In resolvePatchPath, when createIntermediates is true and an intermediate segment is non-object, the code sets parent = current and then writes (parent as Record)[parts[i]] = {}. If current is a primitive (e.g., string), this will throw at runtime. Track the previous container object separately and either replace the non-object with {} (if allowed) or surface a SCIM invalidPath/invalidValue error.
Suggested change
| parent = current; | |
| if (current == null || typeof current !== "object") { | |
| if (createIntermediates) { | |
| (parent as Record<string, unknown>)[parts[i]] = {}; | |
| current = (parent as Record<string, unknown>)[parts[i]]; | |
| } else { | |
| return { target: undefined, parent: null, key }; | |
| } | |
| } else { | |
| current = (current as Record<string, unknown>)[parts[i]]; | |
| if (current === undefined && createIntermediates) { | |
| (parent as Record<string, unknown>)[parts[i]] = {}; | |
| current = (parent as Record<string, unknown>)[parts[i]]; | |
| const part = parts[i]; | |
| if (current == null || typeof current !== "object") { | |
| if (!createIntermediates) { | |
| return { target: undefined, parent: null, key }; | |
| } | |
| // current is not an object; create an intermediate object on the last known parent | |
| if (parent == null || typeof parent !== "object") { | |
| throw new Error("Invalid SCIM path: cannot create intermediate on non-object"); | |
| } | |
| (parent as Record<string, unknown>)[part] = {}; | |
| current = (parent as Record<string, unknown>)[part]; | |
| } else { | |
| // current is a non-null object; descend into it | |
| parent = current; | |
| current = (current as Record<string, unknown>)[part]; | |
| if (current === undefined && createIntermediates) { | |
| (parent as Record<string, unknown>)[part] = {}; | |
| current = (parent as Record<string, unknown>)[part]; |
| } | ||
|
|
||
| // Sync members if provided | ||
| if (body.members) { |
There was a problem hiding this comment.
PUT /Groups calls syncGroupMembers(newGroupName, body.members) when body.members is truthy, but it does not ensure members is an array of { value: string }. A non-array value will cause runtime errors inside syncGroupMembers. Check Array.isArray(body.members) (and validate member ids) before syncing, otherwise return a SCIM 400 invalidValue.
Suggested change
| if (body.members) { | |
| if (body.members !== undefined) { | |
| if (!Array.isArray(body.members)) { | |
| return scimJson( | |
| c, | |
| scimError(400, "Invalid 'members' format; expected an array", "invalidValue"), | |
| 400, | |
| ); | |
| } | |
| const invalidMember = body.members.find( | |
| (m: any) => !m || typeof m.value !== "string" || m.value.trim().length === 0, | |
| ); | |
| if (invalidMember) { | |
| return scimJson( | |
| c, | |
| scimError( | |
| 400, | |
| "Invalid 'members' entries; each member must have a non-empty string 'value'", | |
| "invalidValue", | |
| ), | |
| 400, | |
| ); | |
| } |
Comment on lines
+118
to
+119
| const matcher = parseFilter(filterParam); | ||
| scimUsers = scimUsers.filter(u => matcher(u as unknown as Record<string, unknown>)); |
There was a problem hiding this comment.
filter parsing errors from parseFilter(filterParam) are not handled. If a client sends an invalid filter, the handler will throw and Hono will fall back to the app-level error handler, returning a non-SCIM error body/content-type instead of a SCIM 400 (e.g., scimType=invalidFilter). Wrap parseFilter/matcher execution in try/catch and return a SCIM error response with application/scim+json.
Suggested change
| const matcher = parseFilter(filterParam); | |
| scimUsers = scimUsers.filter(u => matcher(u as unknown as Record<string, unknown>)); | |
| try { | |
| const matcher = parseFilter(filterParam); | |
| scimUsers = scimUsers.filter(u => matcher(u as unknown as Record<string, unknown>)); | |
| } catch (_err) { | |
| return scimJson(c, scimError(400, "Invalid filter", "invalidFilter"), 400); | |
| } |
| }); | ||
|
|
||
| app.get("/scim/v2/ResourceTypes", (c) => { | ||
| return scimJson(c, RESOURCE_TYPES); |
There was a problem hiding this comment.
/scim/v2/ResourceTypes currently returns a bare array. SCIM list endpoints are expected to return a ListResponse envelope, and some SCIM clients will fail to parse a raw array. Consider returning a SCIM ListResponse (schemas, totalResults, Resources, etc.).
Suggested change
| return scimJson(c, RESOURCE_TYPES); | |
| const resources = RESOURCE_TYPES; | |
| return scimJson(c, scimListResponse(resources, resources.length, 1, resources.length)); |
| }); | ||
|
|
||
| app.get("/scim/v2/Schemas", (c) => { | ||
| return scimJson(c, SCHEMAS); |
There was a problem hiding this comment.
/scim/v2/Schemas currently returns a bare array. SCIM list endpoints are expected to return a ListResponse envelope, and some SCIM clients will fail to parse a raw array. Consider returning a SCIM ListResponse (schemas, totalResults, Resources, etc.).
Suggested change
| return scimJson(c, SCHEMAS); | |
| return scimJson(c, scimListResponse(SCHEMAS, SCHEMAS.length, 1, SCHEMAS.length)); |
Comment on lines
+245
to
+247
| // Convert user to SCIM representation for patching | ||
| const scimUser = idpUserToScimUser(user, baseUrl, allGroups) as unknown as Record<string, unknown>; | ||
| const patched = applyPatchOps(scimUser, body.Operations); |
There was a problem hiding this comment.
PATCH /Users does not validate body.Operations before passing it to applyPatchOps. If Operations is missing or not an array, this will throw and the response will bypass SCIM error formatting. Validate the PATCH payload shape and return a SCIM 400 (invalidValue/invalidSyntax) when malformed.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| // Convert user to SCIM representation for patching | ||
| const scimUser = idpUserToScimUser(user, baseUrl, allGroups) as unknown as Record<string, unknown>; | ||
| const patched = applyPatchOps(scimUser, body.Operations); | ||
|
|
Contributor
There was a problem hiding this comment.
SCIM PATCH /Users/:id crashes with TypeError: undefined is not iterable when the request body lacks an Operations array, instead of handling it gracefully.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add full SCIM 2.0 support to the
idpplugin (from PR #9) — both as a provider (expose/scim/v2/*API) and a client (push provisioning to configured targets). Supports Users, Groups, Enterprise User extension, full RFC 7644 filter spec, and PATCH operations. Zero new npm dependencies.SCIM Provider Endpoints (16)
GET /scim/v2/ServiceProviderConfigGET /scim/v2/SchemasGET /scim/v2/ResourceTypesGET /scim/v2/UsersGET /scim/v2/Users/:idPOST /scim/v2/UsersPUT /scim/v2/Users/:idPATCH /scim/v2/Users/:idDELETE /scim/v2/Users/:idGET /scim/v2/GroupsGET /scim/v2/Groups/:idPOST /scim/v2/GroupsPUT /scim/v2/Groups/:idPATCH /scim/v2/Groups/:idDELETE /scim/v2/Groups/:idKey Features
RFC 7644 Filter Parser
Full recursive descent parser — zero dependencies:
eq,ne,co,sw,ew,gt,ge,lt,le,prand,or,notwith nested parenthesesemails[type eq "work"].value eq "x"PATCH Operations (RFC 7644 §3.5.2)
add/replace/removewith dot-path and URN-path resolutionaddEnterprise User Extension
urn:ietf:params:scim:schemas:extension:enterprise:2.0:Usermaps toIdpUser.attributes:department,employeeNumber,managerSCIM Client (Push Provisioning)
ScimClientclass pushes create/update/patch/delete to configured targetsscim.clients[]in seed configAuthentication & Security
timingSafeEqualconstant-time comparisonapplication/scim+jsoncontent type on all responses (including errors)uniquenessscimType)Group Membership Sync
groups[]arraysUsage
Architecture
Zero new npm dependencies beyond what PR #9 added. All SCIM code is pure TypeScript:
Testing
67 SCIM-specific tests (TDD) across 5 test files:
164 total IDP tests passing. All existing OIDC + SAML tests unaffected.
Review History