Feature/else access control permissions#529
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
There was a problem hiding this comment.
PR Summary
Implements comprehensive access control system across the application, transitioning from role-based to permission-based (admin/write/read) access with proper UI feedback and security controls.
- Added new permission management system in
apps/sim/db/migrations/0046_loose_blizzard.sqlwith dedicated permissions table and enum types - Integrated permission checks across UI components with new hooks
useUserPermissionsanduseWorkspacePermissionsto enforce read/write/admin access levels - Added disabled states to all interactive components (SliderInput, Dropdown, DateInput etc.) with consistent visual feedback and tooltips
- Secured workspace member management with atomic transactions in
apps/sim/app/api/workspaces/members/route.tsand invitation flow - Enhanced workspace API endpoints with permission validation in
apps/sim/app/api/workspaces/[id]/permissions/route.ts, preventing admins from removing their own rights
48 files reviewed, 23 comments
Edit PR Review Bot Settings | Greptile
| const handleClick = useCallback(() => { | ||
| if (config.type === 'connectionBlock') return | ||
| if (config.type === 'connectionBlock' || disabled) return |
There was a problem hiding this comment.
logic: 'disabled' check needed in the dependency array of useCallback since it's used in the function
| const handleClick = useCallback(() => { | |
| if (config.type === 'connectionBlock') return | |
| if (config.type === 'connectionBlock' || disabled) return | |
| const handleClick = useCallback(() => { | |
| if (config.type === 'connectionBlock' || disabled) return | |
| // Dispatch a custom event to be caught by the workflow component | |
| const event = new CustomEvent('add-block-from-toolbar', { | |
| detail: { | |
| type: config.type, | |
| }, | |
| }) | |
| window.dispatchEvent(event) | |
| }, [config.type, disabled]) |
| CREATE INDEX "permissions_user_entity_idx" ON "permissions" USING btree ("user_id","entity_type","entity_id");--> statement-breakpoint | ||
| CREATE UNIQUE INDEX "permissions_unique_constraint" ON "permissions" USING btree ("user_id","entity_type","entity_id"); No newline at end of file |
There was a problem hiding this comment.
style: Potential redundancy between permissions_user_entity_idx and permissions_unique_constraint - both cover the same columns
| CREATE INDEX "permissions_user_entity_idx" ON "permissions" USING btree ("user_id","entity_type","entity_id");--> statement-breakpoint | |
| CREATE UNIQUE INDEX "permissions_unique_constraint" ON "permissions" USING btree ("user_id","entity_type","entity_id"); | |
| CREATE UNIQUE INDEX "permissions_unique_constraint" ON "permissions" USING btree ("user_id","entity_type","entity_id"); |
| const updateField = (id: string, field: keyof InputField, value: any) => { | ||
| if (isPreview) return | ||
| if (isPreview || disabled) return | ||
| setStoreValue(fields.map((f: InputField) => (f.id === id ? { ...f, [field]: value } : f))) | ||
| } |
There was a problem hiding this comment.
style: Type 'any' used for value parameter. Consider using more specific types for type safety.
| const updateField = (id: string, field: keyof InputField, value: any) => { | |
| if (isPreview) return | |
| if (isPreview || disabled) return | |
| setStoreValue(fields.map((f: InputField) => (f.id === id ? { ...f, [field]: value } : f))) | |
| } | |
| const updateField = <K extends keyof InputField>(id: string, field: K, value: InputField[K]) => { | |
| if (isPreview || disabled) return | |
| setStoreValue(fields.map((f: InputField) => (f.id === id ? { ...f, [field]: value } : f))) | |
| } |
| const handleFileChange = async (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| if (isPreview) return | ||
| if (isPreview || disabled) return |
There was a problem hiding this comment.
style: Early return for disabled state placed before e.stopPropagation(). Consider moving it after to ensure event propagation is always handled consistently.
| toggleBlockEnabled(blockId) | ||
| } | ||
| }} | ||
| className={cn('text-gray-500', disabled && 'cursor-not-allowed opacity-50')} |
There was a problem hiding this comment.
style: Consider consolidating disabled styles into a reusable className to avoid repetition across buttons
| const updates = Object.entries(existingUserPermissionChanges).map(([userId, changes]) => ({ | ||
| userId, | ||
| permissions: changes.permissionType || 'read', | ||
| })) |
There was a problem hiding this comment.
logic: 'permissions' field name in API request doesn't match the singular 'permission' used in invitation body. This inconsistency may cause issues.
| if (userPermission !== 'read') { | ||
| return NextResponse.json({ error: 'Workspace not found or access denied' }, { status: 404 }) |
There was a problem hiding this comment.
logic: Check should be 'if (!userPermission)' since getUserEntityPermissions returns null for no access. Current check allows write/admin access but denies read.
| await db.insert(permissions).values({ | ||
| id: crypto.randomUUID(), | ||
| entityType: 'workspace' as const, | ||
| entityId: workspaceId, | ||
| userId: userId, | ||
| permissionType: 'admin' as const, | ||
| createdAt: new Date(), | ||
| updatedAt: new Date(), | ||
| }) |
There was a problem hiding this comment.
style: Consider using a transaction to ensure atomicity between workspace, membership, and permission creation. If any step fails, all should be rolled back.
| "name": "permissions", | ||
| "schema": "", | ||
| "columns": { | ||
| "id": { | ||
| "name": "id", | ||
| "type": "text", | ||
| "primaryKey": true, | ||
| "notNull": true | ||
| }, | ||
| "user_id": { | ||
| "name": "user_id", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": true | ||
| }, | ||
| "entity_type": { | ||
| "name": "entity_type", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": true | ||
| }, | ||
| "entity_id": { | ||
| "name": "entity_id", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": true | ||
| }, | ||
| "permission_type": { | ||
| "name": "permission_type", | ||
| "type": "permission_type", | ||
| "typeSchema": "public", |
There was a problem hiding this comment.
logic: Consider using a compound primary key (user_id, entity_type, entity_id) instead of a separate id field since these columns already have a unique constraint
| // Core permission checks | ||
| const canAdmin = userPerms === 'admin' | ||
| const canEdit = userPerms === 'write' || userPerms === 'admin' | ||
| const canRead = true // If user is found in workspace permissions, they have read access |
There was a problem hiding this comment.
logic: This hardcoded canRead=true is problematic. Even if a user is found in workspace permissions, they might have their read access explicitly revoked. Consider checking against userPerms === 'read' || userPerms === 'write' || userPerms === 'admin'
PR thats built on feature/migration-access-control-permissions for all the rest of the changes. We merge this after the migration PR.