Skip to content

Commit 53fdd4d

Browse files
Copilothotlong
andcommitted
Fix code review issues: schema validation, INSERT-only RLS, context mapping docs
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
1 parent b5a608a commit 53fdd4d

3 files changed

Lines changed: 55 additions & 25 deletions

File tree

PROTOCOL_OPTIMIZATION_PLAN.md

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -664,9 +664,8 @@ export const RowLevelSecuritySchema = z.object({
664664

665665
#### 2. SCIM 2.0 Protocol 👥
666666
**文件**:
667-
- `packages/spec/src/auth/scim.zod.ts`
668-
- `packages/spec/src/auth/scim-user.zod.ts`
669-
- `packages/spec/src/auth/scim-group.zod.ts`
667+
- `packages/spec/src/auth/scim.zod.ts` (包含 User, Group, Enterprise Extension)
668+
- `packages/spec/src/auth/scim.test.ts`
670669

671670
**测试**: `packages/spec/src/auth/scim.test.ts`
672671
**文档**: `content/docs/specifications/auth/scim.mdx`
@@ -749,8 +748,7 @@ export const RowLevelSecuritySchema = z.object({
749748

750749
### P0 - 必须立即实现
751750
- [ ] `permission/rls.zod.ts` - Row-Level Security
752-
- [ ] `auth/scim.zod.ts` - SCIM 2.0 User
753-
- [ ] `auth/scim-group.zod.ts` - SCIM 2.0 Group
751+
- [ ] `auth/scim.zod.ts` - SCIM 2.0 User & Group
754752
- [ ] `auth/scim.test.ts` - SCIM Tests
755753

756754
### P1 - 高优先级 (1个月内)

packages/spec/src/auth/scim.zod.ts

Lines changed: 30 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -404,24 +404,6 @@ export const SCIMEnterpriseUserSchema = z.object({
404404

405405
export type SCIMEnterpriseUser = z.infer<typeof SCIMEnterpriseUserSchema>;
406406

407-
/**
408-
* SCIM Schema URI Validator
409-
* Validates that schema URIs are known SCIM schema identifiers
410-
*/
411-
const SCIMSchemaURISchema = z.enum([
412-
SCIM_SCHEMAS.USER,
413-
SCIM_SCHEMAS.GROUP,
414-
SCIM_SCHEMAS.ENTERPRISE_USER,
415-
SCIM_SCHEMAS.RESOURCE_TYPE,
416-
SCIM_SCHEMAS.SERVICE_PROVIDER_CONFIG,
417-
SCIM_SCHEMAS.SCHEMA,
418-
SCIM_SCHEMAS.LIST_RESPONSE,
419-
SCIM_SCHEMAS.PATCH_OP,
420-
SCIM_SCHEMAS.BULK_REQUEST,
421-
SCIM_SCHEMAS.BULK_RESPONSE,
422-
SCIM_SCHEMAS.ERROR,
423-
]);
424-
425407
/**
426408
* SCIM User Schema (Core)
427409
* Complete SCIM 2.0 User resource
@@ -638,6 +620,21 @@ export const SCIMUserSchema = z.object({
638620
[SCIM_SCHEMAS.ENTERPRISE_USER]: SCIMEnterpriseUserSchema
639621
.optional()
640622
.describe('Enterprise user attributes'),
623+
}).superRefine((data, ctx) => {
624+
// Validate that enterprise extension schema URI is present when extension data is provided
625+
const hasEnterpriseExtension = data[SCIM_SCHEMAS.ENTERPRISE_USER] != null;
626+
if (!hasEnterpriseExtension) {
627+
return;
628+
}
629+
630+
const schemas = data.schemas || [];
631+
if (!schemas.includes(SCIM_SCHEMAS.ENTERPRISE_USER)) {
632+
ctx.addIssue({
633+
code: z.ZodIssueCode.custom,
634+
path: ['schemas'],
635+
message: `schemas must include "${SCIM_SCHEMAS.ENTERPRISE_USER}" when enterprise user extension attributes are present`,
636+
});
637+
}
641638
});
642639

643640
export type SCIMUser = z.infer<typeof SCIMUserSchema>;
@@ -752,6 +749,11 @@ export const SCIMListResponseSchema = z.object({
752749
* SCIM schema URI
753750
*/
754751
schemas: z.array(z.string())
752+
.min(1)
753+
.refine(
754+
(schemas) => schemas.includes(SCIM_SCHEMAS.LIST_RESPONSE),
755+
{ message: `schemas must include ${SCIM_SCHEMAS.LIST_RESPONSE}` }
756+
)
755757
.default([SCIM_SCHEMAS.LIST_RESPONSE])
756758
.describe('SCIM schema URIs'),
757759

@@ -800,6 +802,11 @@ export const SCIMErrorSchema = z.object({
800802
* SCIM schema URI
801803
*/
802804
schemas: z.array(z.string())
805+
.min(1)
806+
.refine(
807+
(schemas) => schemas.includes(SCIM_SCHEMAS.ERROR),
808+
{ message: `schemas must include ${SCIM_SCHEMAS.ERROR}` }
809+
)
803810
.default([SCIM_SCHEMAS.ERROR])
804811
.describe('SCIM schema URIs'),
805812

@@ -876,6 +883,11 @@ export const SCIMPatchRequestSchema = z.object({
876883
* SCIM schema URI
877884
*/
878885
schemas: z.array(z.string())
886+
.min(1)
887+
.refine(
888+
(schemas) => schemas.includes(SCIM_SCHEMAS.PATCH_OP),
889+
{ message: 'SCIM PATCH requests must include the PatchOp schema URI' }
890+
)
879891
.default([SCIM_SCHEMAS.PATCH_OP])
880892
.describe('SCIM schema URIs'),
881893

packages/spec/src/permission/rls.zod.ts

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,9 @@ export const RowLevelSecurityPolicySchema = z.object({
261261
* This is a SQL-like expression evaluated for each row.
262262
* Only rows where this expression returns TRUE are accessible.
263263
*
264+
* **Note**: For INSERT-only policies, USING is not required (only CHECK is needed).
265+
* For SELECT/UPDATE/DELETE operations, USING is required.
266+
*
264267
* **Security Note**: RLS conditions are executed at the database level with
265268
* parameterized queries. The implementation must use prepared statements
266269
* to prevent SQL injection. Never concatenate user input directly into
@@ -272,14 +275,18 @@ export const RowLevelSecurityPolicySchema = z.object({
272275
*
273276
* Available context variables:
274277
* - `current_user.id` - Current user's ID
275-
* - `current_user.tenant_id` - Current user's tenant
278+
* - `current_user.tenant_id` - Current user's tenant (maps to `tenantId` in RLSUserContext)
276279
* - `current_user.role` - Current user's role
277280
* - `current_user.department` - Current user's department
278281
* - `current_user.*` - Any custom user field
279282
* - `NOW()` - Current timestamp
280283
* - `CURRENT_DATE` - Current date
281284
* - `CURRENT_TIME` - Current time
282285
*
286+
* **Context Variable Mapping**: The RLSUserContext schema uses camelCase (e.g., `tenantId`),
287+
* but expressions use snake_case with `current_user.` prefix (e.g., `current_user.tenant_id`).
288+
* Implementations must handle this mapping.
289+
*
283290
* Supported operators:
284291
* - Comparison: =, !=, <, >, <=, >=, <> (not equal)
285292
* - Logical: AND, OR, NOT
@@ -298,7 +305,8 @@ export const RowLevelSecurityPolicySchema = z.object({
298305
* @example "status = 'active' AND expiry_date > NOW()"
299306
*/
300307
using: z.string()
301-
.describe('Filter condition (PostgreSQL SQL WHERE clause syntax with parameterized context variables)'),
308+
.optional()
309+
.describe('Filter condition for SELECT/UPDATE/DELETE (PostgreSQL SQL WHERE clause syntax with parameterized context variables). Optional for INSERT-only policies.'),
302310

303311
/**
304312
* CHECK clause - Validation for INSERT/UPDATE operations.
@@ -373,6 +381,18 @@ export const RowLevelSecurityPolicySchema = z.object({
373381
tags: z.array(z.string())
374382
.optional()
375383
.describe('Policy categorization tags'),
384+
}).superRefine((data, ctx) => {
385+
// Ensure at least one of USING or CHECK is provided
386+
if (!data.using && !data.check) {
387+
ctx.addIssue({
388+
code: z.ZodIssueCode.custom,
389+
message: 'At least one of "using" or "check" must be specified. For SELECT/UPDATE/DELETE operations, provide "using". For INSERT operations, provide "check".',
390+
});
391+
}
392+
393+
// For non-insert operations, USING should typically be present
394+
// This is a soft warning through documentation, not enforced here
395+
// since 'all' and mixed operation types are valid
376396
});
377397

378398
/**

0 commit comments

Comments
 (0)