Protocol evaluation and P0 implementations: RLS + SCIM 2.0#219
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds two enterprise-critical protocol specs to close P0 gaps identified in a broader protocol evaluation: Row-Level Security (RLS) and SCIM 2.0 (user/group provisioning), along with tests and updated exports.
Changes:
- Introduces an RLS protocol schema + helper and a comprehensive Vitest suite.
- Introduces a SCIM 2.0 protocol schema + helper and a comprehensive Vitest suite.
- Updates auth/permission barrel exports and adds a protocol optimization plan document.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/spec/src/permission/rls.zod.ts | New Zod schemas/types for RLS policies, config, user context, evaluation results + helper factory. |
| packages/spec/src/permission/rls.test.ts | Tests validating RLS schemas/defaults and helper outputs. |
| packages/spec/src/permission/index.ts | Exposes the new RLS protocol from the permission namespace. |
| packages/spec/src/auth/scim.zod.ts | New Zod schemas/types for SCIM resources/messages + helper factory. |
| packages/spec/src/auth/scim.test.ts | Tests validating SCIM schemas/defaults and helper outputs. |
| packages/spec/src/auth/index.ts | Exposes the new SCIM protocol from the auth namespace. |
| PROTOCOL_OPTIMIZATION_PLAN.md | Adds the evaluation/roadmap document that motivated the P0 additions. |
| const SCIMSchemaURISchema = z.enum([ | ||
| SCIM_SCHEMAS.USER, | ||
| SCIM_SCHEMAS.GROUP, | ||
| SCIM_SCHEMAS.ENTERPRISE_USER, | ||
| SCIM_SCHEMAS.RESOURCE_TYPE, | ||
| SCIM_SCHEMAS.SERVICE_PROVIDER_CONFIG, | ||
| SCIM_SCHEMAS.SCHEMA, | ||
| SCIM_SCHEMAS.LIST_RESPONSE, | ||
| SCIM_SCHEMAS.PATCH_OP, | ||
| SCIM_SCHEMAS.BULK_REQUEST, | ||
| SCIM_SCHEMAS.BULK_RESPONSE, | ||
| SCIM_SCHEMAS.ERROR, | ||
| ]); |
There was a problem hiding this comment.
SCIMSchemaURISchema is declared but never used. With noUnusedLocals: true (see root tsconfig.json), this will fail TypeScript compilation. Either remove it or use it (e.g., as the element schema for schemas arrays where appropriate).
| schemas: z.array(z.string()) | ||
| .default([SCIM_SCHEMAS.ERROR]) |
There was a problem hiding this comment.
SCIMErrorSchema.schemas defaults to the Error URI but does not validate caller-provided values (it will accept arrays that omit the Error schema URI). To prevent parsing invalid SCIM error payloads, add .min(1) and a refinement requiring SCIM_SCHEMAS.ERROR.
| schemas: z.array(z.string()) | |
| .default([SCIM_SCHEMAS.ERROR]) | |
| schemas: z.array(z.string()) | |
| .min(1) | |
| .default([SCIM_SCHEMAS.ERROR]) | |
| .refine( | |
| (schemas) => schemas.includes(SCIM_SCHEMAS.ERROR), | |
| { message: `schemas must include ${SCIM_SCHEMAS.ERROR}` }, | |
| ) |
| /** | ||
| * SCIM schema URI | ||
| */ | ||
| schemas: z.array(z.string()) |
There was a problem hiding this comment.
SCIMPatchRequestSchema.schemas defaults to the PatchOp URI but does not enforce that user-provided arrays include SCIM_SCHEMAS.PATCH_OP. SCIM PATCH requests require this schema identifier; add .min(1) and a refinement (or enum) to validate it.
| schemas: z.array(z.string()) | |
| schemas: z.array(z.string()) | |
| .min(1) | |
| .refine( | |
| (schemas) => schemas.includes(SCIM_SCHEMAS.PATCH_OP), | |
| { message: 'SCIM PATCH requests must include the PatchOp schema URI' } | |
| ) |
| */ | ||
| [SCIM_SCHEMAS.ENTERPRISE_USER]: SCIMEnterpriseUserSchema | ||
| .optional() | ||
| .describe('Enterprise user attributes'), |
There was a problem hiding this comment.
SCIMUserSchema allows the enterprise extension object ([SCIM_SCHEMAS.ENTERPRISE_USER]) to be present even when schemas does not include SCIM_SCHEMAS.ENTERPRISE_USER. Add a refinement to require the extension URI whenever the extension attribute object is provided (to keep payloads RFC 7643-compliant).
| .describe('Enterprise user attributes'), | |
| .describe('Enterprise user attributes'), | |
| }).superRefine((data, ctx) => { | |
| const hasEnterpriseExtension = data[SCIM_SCHEMAS.ENTERPRISE_USER] != null; | |
| if (!hasEnterpriseExtension) { | |
| return; | |
| } | |
| const schemas = Array.isArray((data as any).schemas) ? (data as any).schemas as string[] : []; | |
| if (!schemas.includes(SCIM_SCHEMAS.ENTERPRISE_USER)) { | |
| ctx.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| path: ['schemas'], | |
| message: `schemas must include "${SCIM_SCHEMAS.ENTERPRISE_USER}" when enterprise user extension attributes are present`, | |
| }); | |
| } |
| */ | ||
| using: z.string() | ||
| .describe('Filter condition (PostgreSQL SQL WHERE clause syntax with parameterized context variables)'), |
There was a problem hiding this comment.
using is required for all policies, but the docstring above says USING only applies to SELECT/UPDATE/DELETE. For INSERT-only policies, PostgreSQL-style RLS relies on check (WITH CHECK) and does not require USING. Consider making the schema a discriminated union on operation (or add a refinement) so check is required for insert and using is optional/omitted there.
| - `packages/spec/src/auth/scim.zod.ts` | ||
| - `packages/spec/src/auth/scim-user.zod.ts` | ||
| - `packages/spec/src/auth/scim-group.zod.ts` |
There was a problem hiding this comment.
The Sprint 1 plan lists scim-user.zod.ts and scim-group.zod.ts, but this PR implements SCIM in a single auth/scim.zod.ts file. To keep the plan actionable, update this section (and the checklist later) to match the actual file layout (or add the missing split files).
| - `packages/spec/src/auth/scim.zod.ts` | |
| - `packages/spec/src/auth/scim-user.zod.ts` | |
| - `packages/spec/src/auth/scim-group.zod.ts` | |
| - `packages/spec/src/auth/scim.zod.ts` (包含 SCIM User / Group 资源定义) |
| /** | ||
| * SCIM schema URI | ||
| */ | ||
| schemas: z.array(z.string()) |
There was a problem hiding this comment.
SCIMListResponseSchema.schemas currently accepts any array (including empty or values that don't include the ListResponse URI). Per SCIM, this field must include urn:ietf:params:scim:api:messages:2.0:ListResponse; consider adding .min(1) and a refinement (or reuse a URI enum) to enforce that.
| schemas: z.array(z.string()) | |
| schemas: z.array(z.string()) | |
| .min(1) | |
| .refine( | |
| (schemas) => schemas.includes(SCIM_SCHEMAS.LIST_RESPONSE), | |
| { message: `schemas must include ${SCIM_SCHEMAS.LIST_RESPONSE}` }, | |
| ) |
| /** | ||
| * Tenant/Organization ID | ||
| */ | ||
| tenantId: z.string() | ||
| .optional() | ||
| .describe('Tenant/Organization ID'), | ||
|
|
There was a problem hiding this comment.
In this file, the documented context variable is current_user.tenant_id, but RLSUserContextSchema models it as tenantId. The helper tenantPolicy() also generates current_user.tenant_id. Either align on tenantId/current_user.tenantId everywhere or explicitly document the mapping (camelCase user context → snake_case expression variables) to avoid confusion for implementers.
| - [ ] `auth/scim.zod.ts` - SCIM 2.0 User | ||
| - [ ] `auth/scim-group.zod.ts` - SCIM 2.0 Group |
There was a problem hiding this comment.
In the P0 checklist, auth/scim-group.zod.ts is listed but does not exist in this PR (SCIM Group is included in auth/scim.zod.ts). Update the checklist items to match the implemented file structure so the checklist remains accurate.
| - [ ] `auth/scim.zod.ts` - SCIM 2.0 User | |
| - [ ] `auth/scim-group.zod.ts` - SCIM 2.0 Group | |
| - [ ] `auth/scim.zod.ts` - SCIM 2.0 User & Group |
|
@copilot fix |
…apping docs Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Fixed all 9 code review issues in commit 53fdd4d: SCIM 2.0:
RLS:
Documentation:
|
Protocol Analysis & Optimization Plan - Code Review Fixes ✅
Code Review Feedback Addressed
All 9 review comments have been addressed:
SCIM 2.0 Protocol Fixes
✅ Removed unused
SCIMSchemaURISchema(comment 2727259208)noUnusedLocals: true✅ Added schema URI validation for Error responses (comment 2727259220)
.min(1)and refinement requiringSCIM_SCHEMAS.ERROR✅ Added schema URI validation for PATCH requests (comment 2727259238)
.min(1)and refinement requiringSCIM_SCHEMAS.PATCH_OP✅ Added schema URI validation for List responses (comment 2727259303)
.min(1)and refinement requiringSCIM_SCHEMAS.LIST_RESPONSE✅ Added enterprise extension validation (comment 2727259250)
superRefineto validateschemasarray includesSCIM_SCHEMAS.ENTERPRISE_USERRLS Protocol Fixes
✅ Made USING optional for INSERT-only policies (comment 2727259271)
usingfrom required to optionalsuperRefineto ensure at least one of USING or CHECK is specified✅ Documented context variable mapping (comment 2727259313)
RLSUserContextuses camelCase (e.g.,tenantId)current_user.tenant_id)Documentation Fixes
✅ Updated file structure in plan (comment 2727259295)
scim-user.zod.tsandscim-group.zod.tsfilesscim.zod.ts✅ Updated checklist to match implementation (comment 2727259319)
auth/scim.zod.ts- SCIM 2.0 User & GroupTechnical Improvements
Schema Validation Enhancements:
RLS PostgreSQL Alignment:
Documentation Clarity:
Files Changed
packages/spec/src/auth/scim.zod.ts- Schema validation fixespackages/spec/src/permission/rls.zod.ts- INSERT-only support, context mapping docsPROTOCOL_OPTIMIZATION_PLAN.md- File structure correctionsTesting
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.