feat: add comprehensive permission tests and enhance permission check…#4009
feat: add comprehensive permission tests and enhance permission check…#4009Siumauricio merged 5 commits intocanaryfrom
Conversation
…s in components - Introduced new test files for permission checks, including `check-permission.test.ts`, `enterprise-only-resources.test.ts`, `resolve-permissions.test.ts`, and `service-access.test.ts`. - Implemented permission checks in various components to ensure actions are gated by user permissions, including `ShowTraefikConfig`, `UpdateTraefikConfig`, `ShowVolumes`, `ShowDomains`, and others. - Enhanced the logic for displaying UI elements based on user permissions, ensuring that only authorized users can access or modify resources.
|
@greptile review |
| const merged: Record<string, string[]> = {}; | ||
| for (const entry of customRoles) { | ||
| const parsed = JSON.parse(entry.permission) as Record<string, string[]>; |
There was a problem hiding this comment.
Unhandled JSON.parse can crash permission resolution
JSON.parse(entry.permission) on line 65 can throw a SyntaxError if the permission field contains malformed JSON. This would crash the entire request with an unhandled exception rather than a meaningful error.
Consider wrapping this in a try-catch:
let parsed: Record<string, string[]>;
try {
parsed = JSON.parse(entry.permission) as Record<string, string[]>;
} catch {
continue; // skip malformed role entries
}
| .references(() => organization.id, { onDelete: "cascade" }), | ||
| userId: text("user_id") | ||
| .notNull() | ||
| .references(() => user.id, { onDelete: "cascade" }), |
There was a problem hiding this comment.
Cascade delete will destroy audit logs
Both organizationId and userId foreign keys use onDelete: "cascade". This means deleting a user or organization will permanently delete all associated audit log records. For a compliance/security audit trail, this is typically undesirable — audit logs should be retained even after the actors or organizations are removed.
Consider using onDelete: "set null" (and making the columns nullable) or onDelete: "restrict" to preserve audit history.
…h new definitions - Deleted the old SQL files for organization_role and audit_log. - Introduced new SQL file defining organization_role and audit_log with updated foreign key constraints and indexes. - Updated metadata snapshots to reflect the new table structures and relationships. - Adjusted access control permissions for backup and notification operations to include update capabilities.
|
@greptile review |
| await db | ||
| .update(member) | ||
| .set({ role: input.newRoleName }) | ||
| .where( | ||
| and( | ||
| eq(member.organizationId, ctx.session.activeOrganizationId), | ||
| eq(member.role, input.roleName), | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| validatePermissions(input.permissions); | ||
|
|
||
| await db | ||
| .delete(organizationRole) | ||
| .where( | ||
| and( | ||
| eq( | ||
| organizationRole.organizationId, | ||
| ctx.session.activeOrganizationId, | ||
| ), | ||
| eq(organizationRole.role, input.roleName), | ||
| ), | ||
| ); | ||
|
|
||
| const [created] = await db | ||
| .insert(organizationRole) | ||
| .values({ | ||
| organizationId: ctx.session.activeOrganizationId, | ||
| role: effectiveRoleName, | ||
| permission: JSON.stringify(input.permissions), | ||
| }) | ||
| .returning(); | ||
|
|
There was a problem hiding this comment.
Non-atomic update risks permanent role loss
The update mutation performs three separate database operations without a transaction:
- Line 172: Re-assigns all affected members to the new role name
- Line 185: Deletes the old
organizationRolerecords - Line 197: Inserts the new
organizationRolerecord
If step 3 fails (e.g. a DB constraint violation or transient error) after steps 1 and 2 have already committed, the organization is left in a broken state: members are assigned to a role name that has no corresponding organizationRole row, making the custom role unresolvable and causing every permission check for those members to throw "Invalid role".
Wrap all three operations in a transaction:
await db.transaction(async (tx) => {
if (input.newRoleName && input.newRoleName !== input.roleName) {
// ... existence check omitted for brevity ...
await tx
.update(member)
.set({ role: input.newRoleName })
.where(
and(
eq(member.organizationId, ctx.session.activeOrganizationId),
eq(member.role, input.roleName),
),
);
}
validatePermissions(input.permissions);
await tx
.delete(organizationRole)
.where(
and(
eq(organizationRole.organizationId, ctx.session.activeOrganizationId),
eq(organizationRole.role, input.roleName),
),
);
const [created] = await tx
.insert(organizationRole)
.values({
organizationId: ctx.session.activeOrganizationId,
role: effectiveRoleName,
permission: JSON.stringify(input.permissions),
})
.returning();
return created;
});
| .input(apiUpdateCompose) | ||
| .mutation(async ({ input, ctx }) => { | ||
| const compose = await findComposeById(input.composeId); | ||
| if ( | ||
| compose.environment.project.organizationId !== | ||
| ctx.session.activeOrganizationId | ||
| ) { | ||
| throw new TRPCError({ | ||
| code: "UNAUTHORIZED", | ||
| message: "You are not authorized to update this compose", | ||
| }); | ||
| } | ||
| return updateCompose(input.composeId, input); | ||
| await checkServicePermissionAndAccess(ctx, input.composeId, { | ||
| service: ["create"], | ||
| }); | ||
| const updated = await updateCompose(input.composeId, input); |
There was a problem hiding this comment.
Update operations gated by service: ["create"]
The update mutation (and many other non-create endpoints in this router) use service: ["create"] as the permission gate. service only defines ["create", "read", "delete"] actions — there is no "update" action — but using "create" as the stand-in for update conflates two distinct permission levels:
- A custom role with only
service.readcannot update compose settings, even if that is the intended behaviour. - A custom role with only
service.createis implicitly granted update access, which may be unintended.
The same pattern affects loadServices (line 281), loadMountsByService (line 294), fetchCompose (line 308), randomizeCompose (line 332), isolatedDeployment (line 348), getConvertedCompose (line 367), and equivalent mutations in application.ts.
Consider adding a dedicated "update" action to the service resource in access-control.ts:
service: ["create", "read", "update", "delete"],
and gate mutation endpoints that modify existing services with service: ["update"] rather than service: ["create"].
|
|
||
| await checkPermission(ctx, { environment: ["delete"] }); | ||
|
|
||
| if (memberRecord.role !== "owner" && memberRecord.role !== "admin") { | ||
| if (!memberRecord.accessedProjects.includes(projectId)) { | ||
| throw new TRPCError({ | ||
| code: "UNAUTHORIZED", | ||
| message: "You don't have access to this project", | ||
| }); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| export const addNewProject = async (ctx: PermissionCtx, projectId: string) => { | ||
| const userId = ctx.user.id; | ||
| const organizationId = ctx.session.activeOrganizationId; | ||
| const memberRecord = await findMemberByUserId(userId, organizationId); | ||
| await db | ||
| .update(member) | ||
| .set({ | ||
| accessedProjects: [...memberRecord.accessedProjects, projectId], | ||
| }) |
There was a problem hiding this comment.
addNewProject / addNewEnvironment / addNewService called unconditionally for all roles
Previously these helpers were guarded by if (ctx.user.role === "member"). The guard has been removed in this PR, so every time an owner or admin creates a project, environment, or service, their accessedProjects / accessedEnvironments / accessedServices lists are updated even though those lists are never consulted for owner/admin access decisions. Over time this accumulates unnecessary data and incurs extra DB reads and writes for high-activity organizations.
Adding an early return inside each helper when the role is owner or admin would preserve the current logic while avoiding the unnecessary work:
export const addNewProject = async (ctx: PermissionCtx, projectId: string) => {
const userId = ctx.user.id;
const organizationId = ctx.session.activeOrganizationId;
const memberRecord = await findMemberByUserId(userId, organizationId);
if (memberRecord.role === "owner" || memberRecord.role === "admin") return;
await db.update(member).set({ ... }).where(...);
};- Replaced the delete operation with an update for organization roles, ensuring existing roles are modified instead of removed. - Adjusted the return value to reflect the updated role instead of a newly created entry. - Reintroduced the audit logging functionality for role updates.
…s in components
check-permission.test.ts,enterprise-only-resources.test.ts,resolve-permissions.test.ts, andservice-access.test.ts.ShowTraefikConfig,UpdateTraefikConfig,ShowVolumes,ShowDomains, and others.What is this PR about?
Please describe in a short paragraph what this PR is about.
Checklist
Before submitting this PR, please make sure that:
canarybranch.Issues related (if applicable)
closes
Screenshots (if applicable)
Greptile Summary
This PR introduces a comprehensive permission system for Dokploy, including a new
access-control.tsresource/action registry, apermission.tsservice with role resolution and legacy boolean-override support, awithPermissiontRPC procedure factory, custom role CRUD endpoints, and audit logging. The 116-file changeset refactors all existing routers to use the new permission helpers, replacing the previous ad-hoc organization-ID checks. New test suites cover the core permission logic.Key issues found:
Non-atomic custom role update (
custom-role.ts): Theupdatemutation performs a member-role rename, anorganizationRoledelete, and anorganizationRoleinsert as three separate statements with no transaction. If the insert fails the role definition is permanently gone while members remain assigned to the now-undefined role name, causing all future permission checks for those members to throw "Invalid role".service: ["create"]misused for update/read operations (compose.ts, and similar patterns inapplication.ts): Theupdatemutation,loadServices,getConvertedCompose,randomizeCompose,isolatedDeployment, and several other read/write endpoints are gated withservice: ["create"]because noservice.updateaction exists inaccess-control.ts. This conflates create and update permission levels — a custom role granted only read access cannot view compose details, and a role granted only create access implicitly gains update rights.Double DB query per protected call (
permission.ts):checkServicePermissionAndAccessand related helpers callfindMemberByUserIddirectly and then invokecheckPermission, which callsfindMemberByUserIda second time internally. Every protected endpoint incurs two identical member-record lookups per request.Unconditional
addNewProject/addNewEnvironment/addNewServicefor all roles (permission.ts): The previousif (ctx.user.role === "member")guards have been removed, so owners and admins now accumulate entries in their access lists on every create operation, despite those lists never being consulted for their role.Confidence Score: 2/5
updatemutation has a real data-loss risk: three dependent DB operations are executed without a transaction, and a mid-operation failure leaves members assigned to a non-existent role, permanently breaking permission resolution for those users. Additionally, the widespread use ofservice: ["create"]to gate update/read operations is a semantic design issue that will cause confusing permission behaviour for custom roles. These issues warrant fixes before shipping.Last reviewed commit: 5ffd664