Conversation
📝 WalkthroughWalkthroughThe PR removes the PermissionEntity/table and related repository/interfaces/utilities, migrates permission logic to Cedar policies stored on GroupEntity, replaces permission-table queries with Cedar policy parsing, and adds a migration to drop permission DB tables; multiple tests and use-cases were adjusted accordingly. Changes
Sequence DiagramsequenceDiagram
participant Admin as Admin
participant UC as UseCase
participant Generator as CedarGenerator
participant DB as GroupRepository
participant Parser as CedarParser
Admin->>UC: Request create/update permissions
UC->>Generator: Generate cedarPolicy from classical permissions
Generator-->>UC: cedarPolicy string
UC->>DB: Save group.cedarPolicy
DB-->>UC: Persisted Group
Admin->>UC: Request get permissions for group
UC->>DB: Fetch Group (includes cedarPolicy)
DB-->>UC: Group + cedarPolicy
UC->>Parser: parse cedarPolicy -> classical permissions
Parser-->>UC: Connection/Group/Table access set
UC-->>Admin: Return permissions derived from cedarPolicy
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors backend authorization to rely more heavily on Cedar policies (and policy parsing) rather than legacy PermissionEntity rows, and updates the “readonly” table permission semantics to be derived rather than explicitly set.
Changes:
- Update Cedar policy parsing so
readonlyis computed from other table actions (and adjust related tests). - Remove several legacy permission-building helpers and a legacy user-access repository extension.
- Update connection/demo/test-connection flows to set
group.cedarPolicydirectly, and simplify the “create/update permissions” use case to only persist Cedar policy.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/test/ava-tests/saas-tests/saas-cedar-save-policy-e2e.test.ts | Updates expected readonly semantics in SaaS E2E test. |
| backend/test/ava-tests/saas-tests/saas-cedar-migration-existing-users-e2e.test.ts | Adjusts migration test setup to simulate pre-Cedar permissions without removed helper. |
| backend/test/ava-tests/non-saas-tests/non-saas-cedar-save-policy-e2e.test.ts | Updates expected readonly semantics in non-SaaS E2E test. |
| backend/test/ava-tests/non-saas-tests/non-saas-cedar-policy-parser.test.ts | Updates parser unit test expectation for derived readonly. |
| backend/src/microservices/saas-microservice/use-cases/create-connection-for-hosted-db.use.case.ts | Stops creating default admin PermissionEntity rows; sets Cedar policy on admin group. |
| backend/src/entities/user/utils/build-default-admin-permissions.ts | Deletes legacy helper for default admin permission entities. |
| backend/src/entities/user-access/repository/user-access-custom-repository-extension.ts | Deletes legacy user-access repository extension based on PermissionEntity rows. |
| backend/src/entities/permission/utils/build-new-permission-entity-group.ts | Deletes legacy permission-entity builder utility. |
| backend/src/entities/permission/utils/build-new-permission-entity-connection.ts | Deletes legacy permission-entity builder utility. |
| backend/src/entities/permission/utils/build-final-tables-permissions.ts | Deletes legacy table-permissions aggregation helper. |
| backend/src/entities/permission/use-cases/create-or-update-permissions.use.case.ts | Simplifies to generating/saving Cedar policy and returning request-shaped permissions. |
| backend/src/entities/permission/repository/permission.repository.interface.ts | Removes unused legacy permission-repo methods from the interface. |
| backend/src/entities/permission/repository/permission-custom-repository-extension.ts | Removes unused legacy repository methods (table-specific fetch, user table perms). |
| backend/src/entities/demo-data/demo-data.service.ts | Removes creation of default admin permission entities; sets Cedar policy for demo groups. |
| backend/src/entities/connection/use-cases/get-permissions-for-group-in-connection.use.case.ts | Computes group permissions by parsing Cedar policy instead of querying legacy permissions. |
| backend/src/entities/connection/use-cases/find-all-connections.use.case.ts | Creates Cedar policies for test connections instead of legacy default permissions. |
| backend/src/entities/connection/use-cases/create-connection.use.case.ts | Stops creating default admin PermissionEntity rows; sets Cedar policy on admin group. |
| backend/src/entities/cedar-authorization/cedar-policy-parser.ts | Derives readonly after parsing table actions; stops setting it directly on table:read. |
| backend/src/entities/cedar-authorization/cedar-authorization.service.ts | Stops syncing Cedar policies back into legacy PermissionEntity rows. |
Comments suppressed due to low confidence (1)
backend/src/entities/permission/use-cases/create-or-update-permissions.use.case.ts:77
- This use case no longer creates/updates
PermissionEntityrecords at all. There are still runtime code paths that rely on classical permissions stored ingroup.permissions(e.g.,groupRepository.findAllUsersInGroupsWhereUserIsAdminis used by table log queries to decide whether a non-connection-admin can see other users’ logs). With this change, those queries will stop recognizing group admins unless they’re also connection admins. Either migrate those remaining call sites to Cedar evaluation or keep classical permissions in sync until the migration is complete.
// Generate and save Cedar policy for this group
const cedarPolicy = generateCedarPolicyForGroup(connectionId, groupToUpdate.isMain, resultPermissions);
groupToUpdate.cedarPolicy = cedarPolicy;
await this._dbContext.groupRepository.saveNewOrUpdatedGroup(groupToUpdate);
Cacher.invalidateCedarPolicyCache(connectionId);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| result.tables = Array.from(tableMap.values()); | ||
| for (const table of result.tables) { | ||
| const a = table.accessLevel; |
There was a problem hiding this comment.
parseCedarPolicyToClassicalPermissions currently sets visibility only when it sees table:read. If a Cedar policy permits table:add/table:edit/table:delete without an explicit table:read (which is syntactically valid and passes validatePolicyReferences), the parser will return add/edit/delete: true but visibility: false, producing an inconsistent classical permission shape. To keep parity with CedarPermissionsService (where visibility = canRead || canAdd || canEdit || canDelete), consider deriving visibility from any of the table actions and then computing readonly from the derived booleans.
Suggested change
| const a = table.accessLevel; | |
| const a = table.accessLevel; | |
| // Ensure visibility matches any allowed table action, in parity with CedarPermissionsService | |
| if (!a.visibility && (a.add || a.edit || a.delete)) { | |
| a.visibility = true; | |
| } |
Comment on lines
+26
to
+37
| const group = await this._dbContext.groupRepository.findGroupWithPermissionsById(inputData.groupId); | ||
|
|
||
| let connectionAccessLevel = AccessLevelEnum.none; | ||
| let groupAccessLevel = AccessLevelEnum.none; | ||
| const tablePermissionsMap = new Map<string, TablePermissionDs>(); | ||
|
|
||
| if (group?.cedarPolicy) { | ||
| const parsed = parseCedarPolicyToClassicalPermissions( | ||
| group.cedarPolicy, | ||
| inputData.connectionId, | ||
| inputData.groupId, | ||
| ); |
There was a problem hiding this comment.
GetPermissionsForGroupInConnectionUseCase now loads the group by groupId only and parses its Cedar policy using the request connectionId, but it never verifies that the group actually belongs to inputData.connectionId. This can misreport permissions (connection/group access is derived without checking the policy resource id) and can disclose another connection’s group policy while listing tables from the requested connection. Suggest fetching the group scoped to the connection (e.g., via findGroupInConnection(groupId, connectionId) or by joining group.connection and validating IDs) before parsing, and failing fast if the group isn’t in the connection.
| groupAccessLevel = parsed.group.accessLevel; | ||
| for (const table of parsed.tables) { | ||
| tablePermissionsMap.set(table.tableName, table); | ||
| } |
There was a problem hiding this comment.
This use case now returns AccessLevelEnum.none for all permissions when group.cedarPolicy is null/empty, with no fallback to legacy PermissionEntity rows. Since GroupEntity.cedarPolicy is nullable by default, this breaks permission visibility for unmigrated groups. If the system still has pre-Cedar groups in production, consider falling back to the legacy permission repository (or triggering migration) when cedarPolicy is missing.
Suggested change
| } | |
| } | |
| } else if (group && Array.isArray((group as any).permissions)) { | |
| const legacyPermissions = (group as any).permissions as Array<any>; | |
| let hasAnyPermission = false; | |
| for (const perm of legacyPermissions) { | |
| const tableName = perm.tableName ?? perm.table_name; | |
| if (!tableName) { | |
| continue; | |
| } | |
| const access = { | |
| add: Boolean(perm.add), | |
| delete: Boolean(perm.delete), | |
| edit: Boolean(perm.edit), | |
| readonly: Boolean(perm.readonly), | |
| visibility: Boolean(perm.visibility), | |
| }; | |
| if ( | |
| access.add || | |
| access.delete || | |
| access.edit || | |
| access.readonly || | |
| access.visibility | |
| ) { | |
| hasAnyPermission = true; | |
| } | |
| tablePermissionsMap.set(tableName, { | |
| tableName, | |
| accessLevel: access, | |
| }); | |
| } | |
| if (hasAnyPermission) { | |
| // Fall back to a non-none access level when legacy permissions exist. | |
| connectionAccessLevel = AccessLevelEnum.readonly; | |
| groupAccessLevel = AccessLevelEnum.readonly; | |
| } |
Comment on lines
+61
to
+71
| const resultPermissions: PermissionsDs = { | ||
| connection: { | ||
| accessLevel: inputData.permissions.connection.accessLevel, | ||
| tableName: '', | ||
| groupId: groupId, | ||
| }; | ||
| const updated = Object.assign(currentConnectionPermission, updatedPermissionData); | ||
| await this._dbContext.permissionRepository.saveNewOrUpdatedPermission(updated); | ||
| await this._dbContext.groupRepository.saveNewOrUpdatedGroup(groupToUpdate); | ||
| resultPermissions.connection.accessLevel = updated.accessLevel; | ||
| } else { | ||
| const newPermission = buildNewPermissionEntityConnection(inputData.permissions.connection.accessLevel); | ||
| const savedPermission = await this._dbContext.permissionRepository.saveNewOrUpdatedPermission(newPermission); | ||
| groupToUpdate.permissions.push(savedPermission); | ||
| await this._dbContext.groupRepository.saveNewOrUpdatedGroup(groupToUpdate); | ||
| resultPermissions.connection.accessLevel = newPermission.accessLevel as AccessLevelEnum; | ||
| } | ||
|
|
||
| // *** GROUP PERMISSION | ||
| if (currentGroupPermission) { | ||
| const updatedPermissionData = { | ||
| type: PermissionTypeEnum.Group, | ||
| connectionId: inputData.permissions.connection.connectionId, | ||
| }, | ||
| group: { | ||
| accessLevel: inputData.permissions.group.accessLevel, | ||
| tableName: '', | ||
| groupId: groupId, | ||
| }; | ||
| const updated = Object.assign(currentGroupPermission, updatedPermissionData); | ||
| await this._dbContext.permissionRepository.saveNewOrUpdatedPermission(updated); | ||
| await this._dbContext.groupRepository.saveNewOrUpdatedGroup(groupToUpdate); | ||
| resultPermissions.group.accessLevel = updated.accessLevel; | ||
| } else { | ||
| const newPermission = buildNewPermissionEntityGroup(inputData.permissions.group.accessLevel); | ||
| const savedPermission = await this._dbContext.permissionRepository.saveNewOrUpdatedPermission(newPermission); | ||
| groupToUpdate.permissions.push(savedPermission); | ||
| await this._dbContext.groupRepository.saveNewOrUpdatedGroup(groupToUpdate); | ||
| resultPermissions.group.accessLevel = newPermission.accessLevel as AccessLevelEnum; | ||
| } | ||
|
|
||
| // *** TABLES PERMISSIONS | ||
| const tablePermissions = inputData.permissions.tables; | ||
| // delete falsy permissions | ||
| const deletedPermissions: Array<PermissionEntity> = []; | ||
| await Promise.all( | ||
| tablePermissions.map(async (tablePermission: TablePermissionDs) => { | ||
| const { accessLevel, tableName } = tablePermission; | ||
| for (const key in accessLevel) { | ||
| // has own property check added to avoid object injection | ||
| // eslint-disable-next-line security/detect-object-injection | ||
| if (isObjectPropertyExists(accessLevel, key) && !accessLevel[key]) { | ||
| const permissionIndex = groupToUpdate.permissions.findIndex( | ||
| (permission: PermissionEntity) => permission.accessLevel === key && tableName === permission.tableName, | ||
| ); | ||
| if (permissionIndex >= 0) { | ||
| const permissionInGroup = groupToUpdate.permissions.at(permissionIndex); | ||
| const deletedPermission = | ||
| await this._dbContext.permissionRepository.removePermissionEntity(permissionInGroup); | ||
| deletedPermissions.push(deletedPermission); | ||
| } | ||
| } | ||
| } | ||
| }), | ||
| ); | ||
| await this._dbContext.groupRepository.saveNewOrUpdatedGroup(groupToUpdate); | ||
|
|
||
| //create truthy permissions | ||
| const createdPermissions: Array<PermissionEntity> = []; | ||
| await Promise.all( | ||
| tablePermissions.map(async (tablePermission: TablePermissionDs) => { | ||
| const { accessLevel, tableName } = tablePermission; | ||
| for (const key in accessLevel) { | ||
| // has own property check added to avoid object injection | ||
| // eslint-disable-next-line security/detect-object-injection | ||
| if (isObjectPropertyExists(accessLevel, key) && accessLevel[key]) { | ||
| const permissionIndex = groupToUpdate.permissions.findIndex( | ||
| (permission: PermissionEntity) => permission.accessLevel === key && tableName === permission.tableName, | ||
| ); | ||
| if (permissionIndex < 0) { | ||
| const permissionEntity = new PermissionEntity(); | ||
| permissionEntity.type = PermissionTypeEnum.Table; | ||
| permissionEntity.accessLevel = key; | ||
| permissionEntity.tableName = tableName; | ||
| groupToUpdate.permissions.push(permissionEntity); | ||
| const createdPermission = | ||
| await this._dbContext.permissionRepository.saveNewOrUpdatedPermission(permissionEntity); | ||
| createdPermissions.push(createdPermission); | ||
| } | ||
| } | ||
| } | ||
| }), | ||
| ); | ||
| await this._dbContext.groupRepository.saveNewOrUpdatedGroup(groupToUpdate); | ||
| resultPermissions.tables = buildFinalTablesPermissions( | ||
| allTablePermissionsInGroup, | ||
| deletedPermissions, | ||
| createdPermissions, | ||
| ); | ||
| groupId: inputData.permissions.group.groupId, | ||
| }, | ||
| tables: inputData.permissions.tables, | ||
| }; |
There was a problem hiding this comment.
CreateOrUpdatePermissionsUseCase now just echoes inputData.permissions.tables back to the client and uses it to generate the Cedar policy. Because readonly is now treated as a derived flag (see parseCedarPolicyToClassicalPermissions / CedarPermissionsService), returning the request payload can produce inconsistent readonly values vs what the stored Cedar policy actually grants. Suggest normalizing table access levels before generating (derive visibility / readonly from actions) and/or parsing the generated policy back and returning the parsed permissions as the source of truth.
Comment on lines
105
to
109
| const classicalPermissions = parseCedarPolicyToClassicalPermissions(cedarPolicy, connectionId, groupId); | ||
|
|
||
| await this.syncClassicalPermissions(group, classicalPermissions); | ||
|
|
||
| group.cedarPolicy = cedarPolicy; | ||
| await this.globalDbContext.groupRepository.saveNewOrUpdatedGroup(group); | ||
| Cacher.invalidateCedarPolicyCache(connectionId); |
There was a problem hiding this comment.
saveCedarPolicy no longer calls syncClassicalPermissions, which means any features still querying group.permissions / PermissionEntity will become stale or stop working after a Cedar policy update. For example, groupRepository.findAllUsersInGroupsWhereUserIsAdmin (used in table log filtering) currently checks group.permissions for PermissionTypeEnum.Group + AccessLevelEnum.edit. If the intent is to fully deprecate classical permissions, those remaining queries should be migrated to Cedar evaluation before removing synchronization.
Comment on lines
+83
to
+84
| const a = table.accessLevel; | ||
| a.readonly = a.visibility && !a.add && !a.edit && !a.delete; |
There was a problem hiding this comment.
The new post-processing loop uses a non-descriptive variable name a for table.accessLevel, which makes the logic harder to read/maintain. Consider renaming it to something explicit like access/accessLevel to match surrounding conventions.
Suggested change
| const a = table.accessLevel; | |
| a.readonly = a.visibility && !a.add && !a.edit && !a.delete; | |
| const accessLevel = table.accessLevel; | |
| accessLevel.readonly = | |
| accessLevel.visibility && !accessLevel.add && !accessLevel.edit && !accessLevel.delete; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/entities/permission/use-cases/create-or-update-permissions.use.case.ts (1)
61-79:⚠️ Potential issue | 🔴 CriticalKeep writing the legacy
PermissionEntityrows while the non-Cedar path still exists.This implementation now only persists
group.cedarPolicyand echoesinputData.permissions. The existing migration E2Es still callPUT /permissions/...before Cedar is enabled and expect those changes to take effect immediately, so removing the classical writes here breaks that mode. Gate this Cedar-only branch behindCEDAR_AUTHORIZATION_ENABLEDor keep dual-writing until the legacy flow is retired.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/permission/use-cases/create-or-update-permissions.use.case.ts` around lines 61 - 79, The code currently only writes the Cedar policy (generateCedarPolicyForGroup -> groupToUpdate.cedarPolicy -> this._dbContext.groupRepository.saveNewOrUpdatedGroup) and returns resultPermissions, which drops legacy PermissionEntity writes; either gate this Cedar-only flow behind CEDAR_AUTHORIZATION_ENABLED or perform dual-writing until legacy is removed: wrap the Cedar generation+save in a feature-flag check (process.env.CEDAR_AUTHORIZATION_ENABLED) and when the flag is false (or absent) also persist the legacy permission rows (use the existing Permission repository/save method used elsewhere in this module — e.g., call the permission save routine that writes PermissionEntity rows) so E2E migrations that call PUT /permissions/... continue to take effect immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/src/entities/connection/use-cases/get-permissions-for-group-in-connection.use.case.ts`:
- Around line 26-43: The code currently loads group by groupId and parses its
cedarPolicy against the supplied inputData.connectionId, which can misapply
policies from a different connection; before calling
parseCedarPolicyToClassicalPermissions or trusting group.cedarPolicy, validate
the pair by checking the group's connection association (e.g., ensure
group.connectionId === inputData.connectionId or call the scoped repo method
used by CreateOrUpdatePermissionsUseCase) and throw or return the existing
GROUP_NOT_FROM_THIS_CONNECTION guard if they don't match; then only proceed to
call parseCedarPolicyToClassicalPermissions and populate connectionAccessLevel,
groupAccessLevel, and tablePermissionsMap.
---
Outside diff comments:
In
`@backend/src/entities/permission/use-cases/create-or-update-permissions.use.case.ts`:
- Around line 61-79: The code currently only writes the Cedar policy
(generateCedarPolicyForGroup -> groupToUpdate.cedarPolicy ->
this._dbContext.groupRepository.saveNewOrUpdatedGroup) and returns
resultPermissions, which drops legacy PermissionEntity writes; either gate this
Cedar-only flow behind CEDAR_AUTHORIZATION_ENABLED or perform dual-writing until
legacy is removed: wrap the Cedar generation+save in a feature-flag check
(process.env.CEDAR_AUTHORIZATION_ENABLED) and when the flag is false (or absent)
also persist the legacy permission rows (use the existing Permission
repository/save method used elsewhere in this module — e.g., call the permission
save routine that writes PermissionEntity rows) so E2E migrations that call PUT
/permissions/... continue to take effect immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 70854f65-322b-485a-857e-acd9d03898f3
📒 Files selected for processing (19)
backend/src/entities/cedar-authorization/cedar-authorization.service.tsbackend/src/entities/cedar-authorization/cedar-policy-parser.tsbackend/src/entities/connection/use-cases/create-connection.use.case.tsbackend/src/entities/connection/use-cases/find-all-connections.use.case.tsbackend/src/entities/connection/use-cases/get-permissions-for-group-in-connection.use.case.tsbackend/src/entities/demo-data/demo-data.service.tsbackend/src/entities/permission/repository/permission-custom-repository-extension.tsbackend/src/entities/permission/repository/permission.repository.interface.tsbackend/src/entities/permission/use-cases/create-or-update-permissions.use.case.tsbackend/src/entities/permission/utils/build-final-tables-permissions.tsbackend/src/entities/permission/utils/build-new-permission-entity-connection.tsbackend/src/entities/permission/utils/build-new-permission-entity-group.tsbackend/src/entities/user-access/repository/user-access-custom-repository-extension.tsbackend/src/entities/user/utils/build-default-admin-permissions.tsbackend/src/microservices/saas-microservice/use-cases/create-connection-for-hosted-db.use.case.tsbackend/test/ava-tests/non-saas-tests/non-saas-cedar-policy-parser.test.tsbackend/test/ava-tests/non-saas-tests/non-saas-cedar-save-policy-e2e.test.tsbackend/test/ava-tests/saas-tests/saas-cedar-migration-existing-users-e2e.test.tsbackend/test/ava-tests/saas-tests/saas-cedar-save-policy-e2e.test.ts
💤 Files with no reviewable changes (11)
- backend/src/entities/permission/utils/build-new-permission-entity-group.ts
- backend/src/entities/connection/use-cases/create-connection.use.case.ts
- backend/src/entities/permission/utils/build-new-permission-entity-connection.ts
- backend/src/microservices/saas-microservice/use-cases/create-connection-for-hosted-db.use.case.ts
- backend/src/entities/demo-data/demo-data.service.ts
- backend/src/entities/user/utils/build-default-admin-permissions.ts
- backend/src/entities/permission/repository/permission.repository.interface.ts
- backend/src/entities/permission/repository/permission-custom-repository-extension.ts
- backend/src/entities/permission/utils/build-final-tables-permissions.ts
- backend/src/entities/user-access/repository/user-access-custom-repository-extension.ts
- backend/src/entities/cedar-authorization/cedar-authorization.service.ts
Comment on lines
+26
to
+43
| const group = await this._dbContext.groupRepository.findGroupWithPermissionsById(inputData.groupId); | ||
|
|
||
| let connectionAccessLevel = AccessLevelEnum.none; | ||
| let groupAccessLevel = AccessLevelEnum.none; | ||
| const tablePermissionsMap = new Map<string, TablePermissionDs>(); | ||
|
|
||
| if (group?.cedarPolicy) { | ||
| const parsed = parseCedarPolicyToClassicalPermissions( | ||
| group.cedarPolicy, | ||
| inputData.connectionId, | ||
| inputData.groupId, | ||
| ); | ||
| connectionAccessLevel = parsed.connection.accessLevel; | ||
| groupAccessLevel = parsed.group.accessLevel; | ||
| for (const table of parsed.tables) { | ||
| tablePermissionsMap.set(table.tableName, table); | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate the group/connection pair before interpreting the policy.
Line 26 now loads the group only by groupId, then parses its Cedar policy against whatever inputData.connectionId the caller supplied. That regresses the old repository scoping: a mismatched pair can now return non-none connection/group access for the wrong connection because the parser promotes connection:* / group:* actions by name. Reuse the same GROUP_NOT_FROM_THIS_CONNECTION guard as CreateOrUpdatePermissionsUseCase here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/entities/connection/use-cases/get-permissions-for-group-in-connection.use.case.ts`
around lines 26 - 43, The code currently loads group by groupId and parses its
cedarPolicy against the supplied inputData.connectionId, which can misapply
policies from a different connection; before calling
parseCedarPolicyToClassicalPermissions or trusting group.cedarPolicy, validate
the pair by checking the group's connection association (e.g., ensure
group.connectionId === inputData.connectionId or call the scoped repo method
used by CreateOrUpdatePermissionsUseCase) and throw or return the existing
GROUP_NOT_FROM_THIS_CONNECTION guard if they don't match; then only proceed to
call parseCedarPolicyToClassicalPermissions and populate connectionAccessLevel,
groupAccessLevel, and tablePermissionsMap.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/src/entities/connection/use-cases/get-permissions-for-group-in-connection.use.case.ts (1)
26-43:⚠️ Potential issue | 🟠 MajorMissing group/connection ownership validation.
The code loads the group by
groupIdalone and parses its Cedar policy againstinputData.connectionIdwithout verifying the group actually belongs to that connection. This mirrors the validation gap flagged in a previous review.A request with a valid
groupIdfrom connection A paired withconnectionIdfrom connection B would parse policies incorrectly, potentially exposing permission data or causing authorization logic errors.Apply the same guard used in
CedarAuthorizationService.saveCedarPolicy:🛡️ Proposed fix
- const group = await this._dbContext.groupRepository.findGroupById(inputData.groupId); + const group = await this._dbContext.groupRepository.findGroupByIdWithConnectionAndUsers(inputData.groupId); + + if (group && group.connection?.id !== inputData.connectionId) { + // Group does not belong to this connection - treat as no permissions + return { + connection: { connectionId: inputData.connectionId, accessLevel: AccessLevelEnum.none }, + group: { groupId: inputData.groupId, accessLevel: AccessLevelEnum.none }, + tables: [], + }; + } let connectionAccessLevel = AccessLevelEnum.none;Alternatively, throw an
HttpExceptionwithMessages.GROUP_NOT_FROM_THIS_CONNECTIONto match the pattern inCreateOrUpdatePermissionsUseCase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/connection/use-cases/get-permissions-for-group-in-connection.use.case.ts` around lines 26 - 43, The code calls this._dbContext.groupRepository.findGroupById(inputData.groupId) and then parses the group's cedarPolicy with parseCedarPolicyToClassicalPermissions using inputData.connectionId without verifying the group belongs to that connection; add the same ownership guard used in CedarAuthorizationService.saveCedarPolicy/CreateOrUpdatePermissionsUseCase by checking that the loaded group's connectionId equals inputData.connectionId and if not throw an HttpException with Messages.GROUP_NOT_FROM_THIS_CONNECTION (or return early as per project pattern) before calling parseCedarPolicyToClassicalPermissions and populating tablePermissionsMap.
🧹 Nitpick comments (4)
backend/src/entities/group/repository/group-custom-repository-extension.ts (1)
80-93: Cedar policy detection via LIKE patterns is fragile.The LIKE patterns
'%principal, action, resource%'and'%group:edit%'may produce false positives (matching comments, string literals, or partial action names likegroup:edit_members) or false negatives (if policy formatting varies). Consider reusing the Cedar policy parser (parseCedarPolicyToClassicalPermissions) post-fetch for accurate detection, or document this as an intentional performance trade-off.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/group/repository/group-custom-repository-extension.ts` around lines 80 - 93, The current admin-group lookup uses fragile SQL LIKE checks on group.cedarPolicy in the createQueryBuilder that populates adminGroupIds; instead, remove the cedarPolicy LIKE predicates from the query, fetch candidate groups (or fetch all groups the user is in for the connection) using createQueryBuilder('group')...where('user.id = :userId', { userId }).andWhere('connection.id = :connectionId', { connectionId }).getMany() and then apply the parseCedarPolicyToClassicalPermissions function in-memory to each group's cedarPolicy to determine true admin rights, keeping only groups that the parser says grant the relevant permissions (or alternatively add a code comment documenting the intentional performance tradeoff if you choose to keep the LIKE filters).backend/src/entities/cedar-authorization/cedar-authorization.service.ts (2)
247-249: Same untyped exception access pattern.Apply consistent exception handling here as well.
🔧 Suggested fix
} catch (e) { - throw new HttpException({ message: `Invalid cedar policy: ${e.message}` }, HttpStatus.BAD_REQUEST); + const message = e instanceof Error ? e.message : String(e); + throw new HttpException({ message: `Invalid cedar policy: ${message}` }, HttpStatus.BAD_REQUEST); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/cedar-authorization/cedar-authorization.service.ts` around lines 247 - 249, The catch block in cedar-authorization.service.ts currently accesses e.message unsafely when rethrowing an HttpException (the throw new HttpException({ message: `Invalid cedar policy: ${e.message}` }, ... ) line); change the catch to catch (e: unknown) and derive a safe message (e.g., const msg = e instanceof Error ? e.message : String(e)) before passing it into the HttpException so you never assume the thrown value has a message property.
158-161: Untyped exception access.The catch block accesses
e.messagewithout type narrowing. In TypeScript strict mode or edge cases,ecould be a non-Error type.🔧 Suggested fix
} catch (e) { if (e instanceof HttpException) throw e; - throw new HttpException({ message: `Invalid cedar schema: ${e.message}` }, HttpStatus.BAD_REQUEST); + const message = e instanceof Error ? e.message : String(e); + throw new HttpException({ message: `Invalid cedar schema: ${message}` }, HttpStatus.BAD_REQUEST); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/cedar-authorization/cedar-authorization.service.ts` around lines 158 - 161, The catch block in CedarAuthorizationService currently reads `catch (e) { if (e instanceof HttpException) throw e; throw new HttpException({ message: \`Invalid cedar schema: ${e.message}\` }, HttpStatus.BAD_REQUEST); }` and accesses e.message without type narrowing; update the handler to safely derive a message (e.g., const msg = e instanceof Error ? e.message : String(e)) and use that msg in the HttpException payload so non-Error throws are handled; apply this change in the catch inside the CedarAuthorizationService method where the schema validation occurs.backend/src/migrations/1771545700000-RemovePermissionTables.ts (1)
15-23: Consider adding an index ongroupIdfor the join table rollback.While the composite primary key covers lookups by
permissionId, queries filtering bygroupId(e.g., "get all permissions for a group") would benefit from an index. This is a minor optimization for the rollback scenario.📊 Suggested improvement for rollback completeness
await queryRunner.query( `ALTER TABLE "permission_groups_group" ADD CONSTRAINT "FK_permission_groups_groupId" FOREIGN KEY ("groupId") REFERENCES "group"("id") ON DELETE CASCADE ON UPDATE CASCADE`, ); + await queryRunner.query( + `CREATE INDEX "IDX_permission_groups_groupId" ON "permission_groups_group" ("groupId")`, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/migrations/1771545700000-RemovePermissionTables.ts` around lines 15 - 23, Add an index on the join table's groupId to improve lookups during rollback: after creating the permission_groups_group table (and/or after adding the FK constraints) add a CREATE INDEX for groupId (e.g., name it IDX_permission_groups_group_groupId) referencing the permission_groups_group table and the "groupId" column; update the up/down or rollback sections in RemovePermissionTables migration so the index is created on migrate up and dropped on rollback (use the existing table name permission_groups_group and column name groupId to locate where to insert the index creation/drop).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/entities/group/repository/group-custom-repository-extension.ts`:
- Around line 99-105: The current query in group-custom-repository-extension
(the chain starting with
this.manager.getRepository(UserEntity).createQueryBuilder('user') and using
leftJoin('user.groups','group').where('group.id IN (:...groupIds)', { groupIds
}).getMany()) can return duplicate UserEntity rows if a user is in multiple
admin groups; modify the query to enforce uniqueness by adding a DISTINCT
qualifier (e.g., call .distinct(true) on the QueryBuilder or select DISTINCT
user id before fetching) so the returned user list has no duplicates.
---
Duplicate comments:
In
`@backend/src/entities/connection/use-cases/get-permissions-for-group-in-connection.use.case.ts`:
- Around line 26-43: The code calls
this._dbContext.groupRepository.findGroupById(inputData.groupId) and then parses
the group's cedarPolicy with parseCedarPolicyToClassicalPermissions using
inputData.connectionId without verifying the group belongs to that connection;
add the same ownership guard used in
CedarAuthorizationService.saveCedarPolicy/CreateOrUpdatePermissionsUseCase by
checking that the loaded group's connectionId equals inputData.connectionId and
if not throw an HttpException with Messages.GROUP_NOT_FROM_THIS_CONNECTION (or
return early as per project pattern) before calling
parseCedarPolicyToClassicalPermissions and populating tablePermissionsMap.
---
Nitpick comments:
In `@backend/src/entities/cedar-authorization/cedar-authorization.service.ts`:
- Around line 247-249: The catch block in cedar-authorization.service.ts
currently accesses e.message unsafely when rethrowing an HttpException (the
throw new HttpException({ message: `Invalid cedar policy: ${e.message}` }, ... )
line); change the catch to catch (e: unknown) and derive a safe message (e.g.,
const msg = e instanceof Error ? e.message : String(e)) before passing it into
the HttpException so you never assume the thrown value has a message property.
- Around line 158-161: The catch block in CedarAuthorizationService currently
reads `catch (e) { if (e instanceof HttpException) throw e; throw new
HttpException({ message: \`Invalid cedar schema: ${e.message}\` },
HttpStatus.BAD_REQUEST); }` and accesses e.message without type narrowing;
update the handler to safely derive a message (e.g., const msg = e instanceof
Error ? e.message : String(e)) and use that msg in the HttpException payload so
non-Error throws are handled; apply this change in the catch inside the
CedarAuthorizationService method where the schema validation occurs.
In `@backend/src/entities/group/repository/group-custom-repository-extension.ts`:
- Around line 80-93: The current admin-group lookup uses fragile SQL LIKE checks
on group.cedarPolicy in the createQueryBuilder that populates adminGroupIds;
instead, remove the cedarPolicy LIKE predicates from the query, fetch candidate
groups (or fetch all groups the user is in for the connection) using
createQueryBuilder('group')...where('user.id = :userId', { userId
}).andWhere('connection.id = :connectionId', { connectionId }).getMany() and
then apply the parseCedarPolicyToClassicalPermissions function in-memory to each
group's cedarPolicy to determine true admin rights, keeping only groups that the
parser says grant the relevant permissions (or alternatively add a code comment
documenting the intentional performance tradeoff if you choose to keep the LIKE
filters).
In `@backend/src/migrations/1771545700000-RemovePermissionTables.ts`:
- Around line 15-23: Add an index on the join table's groupId to improve lookups
during rollback: after creating the permission_groups_group table (and/or after
adding the FK constraints) add a CREATE INDEX for groupId (e.g., name it
IDX_permission_groups_group_groupId) referencing the permission_groups_group
table and the "groupId" column; update the up/down or rollback sections in
RemovePermissionTables migration so the index is created on migrate up and
dropped on rollback (use the existing table name permission_groups_group and
column name groupId to locate where to insert the index creation/drop).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53347701-6353-497c-ae66-9bf372a9b584
📒 Files selected for processing (30)
backend/src/common/application/global-database-context.interface.tsbackend/src/common/application/global-database-context.tsbackend/src/entities/cedar-authorization/cedar-authorization.service.tsbackend/src/entities/cedar-authorization/cedar-policy-parser.tsbackend/src/entities/cedar-authorization/scripts/migrate-permissions-to-cedar.tsbackend/src/entities/company-info/company-info.module.tsbackend/src/entities/connection/connection.module.tsbackend/src/entities/connection/use-cases/get-permissions-for-group-in-connection.use.case.tsbackend/src/entities/connection/utils/build-new-group-entity-for-connection-with-user.tsbackend/src/entities/custom-field/custom-field.module.tsbackend/src/entities/group/group.entity.tsbackend/src/entities/group/group.module.tsbackend/src/entities/group/repository/group-custom-repository-extension.tsbackend/src/entities/group/repository/group.repository.interface.tsbackend/src/entities/permission/permission.entity.tsbackend/src/entities/permission/permission.module.tsbackend/src/entities/permission/repository/permission-custom-repository-extension.tsbackend/src/entities/permission/repository/permission.repository.interface.tsbackend/src/entities/permission/use-cases/create-or-update-permissions.use.case.tsbackend/src/entities/table-settings/common-table-settings/table-settings.module.tsbackend/src/entities/table/table.module.tsbackend/src/entities/user/user.module.tsbackend/src/entities/user/utils/build-default-admin-groups.tsbackend/src/enums/index.tsbackend/src/enums/permission-type.enum.tsbackend/src/main.tsbackend/src/migrations/1771545700000-RemovePermissionTables.tsbackend/test/ava-tests/non-saas-tests/non-saas-cedar-migration-existing-users-e2e.test.tsbackend/test/ava-tests/saas-tests/saas-cedar-migration-existing-users-e2e.test.tsbackend/test/mock.factory.ts
💤 Files with no reviewable changes (22)
- backend/src/entities/user/utils/build-default-admin-groups.ts
- backend/src/entities/connection/connection.module.ts
- backend/src/entities/user/user.module.ts
- backend/src/enums/index.ts
- backend/src/main.ts
- backend/src/entities/custom-field/custom-field.module.ts
- backend/src/entities/connection/utils/build-new-group-entity-for-connection-with-user.ts
- backend/src/entities/table/table.module.ts
- backend/src/entities/group/group.module.ts
- backend/src/common/application/global-database-context.interface.ts
- backend/src/entities/table-settings/common-table-settings/table-settings.module.ts
- backend/src/entities/company-info/company-info.module.ts
- backend/src/enums/permission-type.enum.ts
- backend/src/entities/permission/permission.module.ts
- backend/src/common/application/global-database-context.ts
- backend/src/entities/group/group.entity.ts
- backend/src/entities/permission/permission.entity.ts
- backend/src/entities/cedar-authorization/scripts/migrate-permissions-to-cedar.ts
- backend/test/ava-tests/non-saas-tests/non-saas-cedar-migration-existing-users-e2e.test.ts
- backend/src/entities/permission/repository/permission.repository.interface.ts
- backend/test/ava-tests/saas-tests/saas-cedar-migration-existing-users-e2e.test.ts
- backend/src/entities/permission/repository/permission-custom-repository-extension.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/entities/cedar-authorization/cedar-policy-parser.ts
- backend/src/entities/permission/use-cases/create-or-update-permissions.use.case.ts
Comment on lines
+99
to
+105
| const groupIds = adminGroupIds.map((g) => g.id); | ||
| return await this.manager | ||
| .getRepository(UserEntity) | ||
| .createQueryBuilder('user') | ||
| .leftJoinAndSelect('user.groups', 'group') | ||
| .leftJoinAndSelect('group.connection', 'connection') | ||
| .leftJoinAndSelect('group.permissions', 'permission') | ||
| .andWhere('user.id = :userId', { userId: userId }) | ||
| .andWhere('connection.id = :connectionId', { connectionId: connectionId }) | ||
| .andWhere('permission.type = :permissionType', { | ||
| permissionType: PermissionTypeEnum.Group, | ||
| }) | ||
| .andWhere('permission.accessLevel = :permissionAccessLevel', { | ||
| permissionAccessLevel: AccessLevelEnum.edit, | ||
| }); | ||
| return await userQb.getMany(); | ||
| .leftJoin('user.groups', 'group') | ||
| .where('group.id IN (:...groupIds)', { groupIds }) | ||
| .getMany(); |
There was a problem hiding this comment.
Potential duplicate users in result set.
If a user belongs to multiple admin groups within the connection, they will appear multiple times in the result. Add DISTINCT to deduplicate.
🔧 Proposed fix
const groupIds = adminGroupIds.map((g) => g.id);
return await this.manager
.getRepository(UserEntity)
.createQueryBuilder('user')
+ .distinct(true)
.leftJoin('user.groups', 'group')
.where('group.id IN (:...groupIds)', { groupIds })
.getMany();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const groupIds = adminGroupIds.map((g) => g.id); | |
| return await this.manager | |
| .getRepository(UserEntity) | |
| .createQueryBuilder('user') | |
| .leftJoinAndSelect('user.groups', 'group') | |
| .leftJoinAndSelect('group.connection', 'connection') | |
| .leftJoinAndSelect('group.permissions', 'permission') | |
| .andWhere('user.id = :userId', { userId: userId }) | |
| .andWhere('connection.id = :connectionId', { connectionId: connectionId }) | |
| .andWhere('permission.type = :permissionType', { | |
| permissionType: PermissionTypeEnum.Group, | |
| }) | |
| .andWhere('permission.accessLevel = :permissionAccessLevel', { | |
| permissionAccessLevel: AccessLevelEnum.edit, | |
| }); | |
| return await userQb.getMany(); | |
| .leftJoin('user.groups', 'group') | |
| .where('group.id IN (:...groupIds)', { groupIds }) | |
| .getMany(); | |
| const groupIds = adminGroupIds.map((g) => g.id); | |
| return await this.manager | |
| .getRepository(UserEntity) | |
| .createQueryBuilder('user') | |
| .distinct(true) | |
| .leftJoin('user.groups', 'group') | |
| .where('group.id IN (:...groupIds)', { groupIds }) | |
| .getMany(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/entities/group/repository/group-custom-repository-extension.ts`
around lines 99 - 105, The current query in group-custom-repository-extension
(the chain starting with
this.manager.getRepository(UserEntity).createQueryBuilder('user') and using
leftJoin('user.groups','group').where('group.id IN (:...groupIds)', { groupIds
}).getMany()) can return duplicate UserEntity rows if a user is in multiple
admin groups; modify the query to enforce uniqueness by adding a DISTINCT
qualifier (e.g., call .distinct(true) on the QueryBuilder or select DISTINCT
user id before fetching) so the returned user list has no duplicates.
This was referenced Mar 23, 2026
Merged
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 by CodeRabbit