commonscomputer/cal.diy
4
0
Fork 0
Code Issues Pull Requests Actions 2.2k Packages Projects Releases Wiki Activity

revert: "fix: remove fallback permssions on organization Id. (#22769)" (#23013)

Browse Source 
* Revert "fix: remove fallback permssions on organization Id. (#22769)"

This reverts commit 30061c0d58.

Removes the manage permission concept and fallback logic to simplify
permission checking back to direct role-based checks.

* remove manage from workflows

* fix type error and remove manage from components
This commit is contained in:
sean-brydon
2025-08-26 08:32:49 -03:00
committed by GitHub
parent 2922c8fe77
commit ae738fae35
9 changed files with 22 additions and 434 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/__tests__/usePermissions.test.ts
-12
View File
@@ -44,17 +44,5 @@ describe("usePermissions", () => {
expect(getResourcePermissionLevel("eventType", permissions)).toBe("all");
});
it("should return 'all' for resource with manage permission", () => {
const permissions = ["eventType.manage"];
expect(getResourcePermissionLevel("eventType", permissions)).toBe("all");
});
it("should return 'all' for resource with manage permission even if other permissions are missing", () => {
const permissions = ["eventType.manage", "eventType.read"]; // Has manage and read, but missing create, update, delete
expect(getResourcePermissionLevel("eventType", permissions)).toBe("all");
});
});
});
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts
+5 -11
View File
@@ -51,20 +51,14 @@ export function usePermissions(): UsePermissionsReturn {
return "all";
}
// Check if user has manage permission for this resource
const hasManagePermission = permissions.includes(`${resource}.manage`);
if (hasManagePermission) {
return "all";
}
// Filter out internal keys like _resource and manage when checking for individual permissions
const crudPermissions = Object.keys(resourceConfig)
.filter((action) => !action.startsWith("_") && action !== "manage")
// Filter out internal keys like _resource when checking permissions
const allResourcePerms = Object.keys(resourceConfig)
.filter((action) => !action.startsWith("_"))
.map((action) => `${resource}.${action}`);
const hasAllCrudPerms = crudPermissions.every((p) => permissions.includes(p));
const hasAllPerms = allResourcePerms.every((p) => permissions.includes(p));
const hasReadPerm = permissions.includes(`${resource}.${CrudAction.Read}`);
if (hasAllCrudPerms) return "all";
if (hasAllPerms) return "all";
if (hasReadPerm) return "read";
return "none";
};
apps/web/public/static/locales/en/common.json
+3 -4
View File
@@ -3336,7 +3336,6 @@
"pbac_desc_view_roles": "View roles",
"pbac_desc_update_roles": "Update roles",
"pbac_desc_delete_roles": "Delete roles",
"pbac_desc_manage_roles": "All actions on roles across organization teams",
"pbac_desc_create_workflows": "Create and set up new workflows",
"pbac_desc_view_workflows": "View existing workflows and their configurations",
"pbac_desc_update_workflows": "Edit and modify workflow settings",
@@ -3346,7 +3345,7 @@
"pbac_desc_view_event_types": "View event types",
"pbac_desc_update_event_types": "Update event types",
"pbac_desc_delete_event_types": "Delete event types",
"pbac_desc_manage_event_types": "All actions on event types across organization teams",
"pbac_desc_manage_event_types": "All actions on event types",
"pbac_desc_create_teams": "Create teams",
"pbac_desc_view_team_details": "View team details",
"pbac_desc_update_team_settings": "Update team settings",
@@ -3354,7 +3353,7 @@
"pbac_desc_invite_team_members": "Invite team members",
"pbac_desc_remove_team_members": "Remove team members",
"pbac_desc_change_team_member_role": "Change role of team members",
"pbac_desc_manage_teams": "All actions on teams across organization teams",
"pbac_desc_manage_teams": "All actions on teams",
"pbac_desc_create_organization": "Create organization",
"pbac_desc_view_organization_details": "View organization details",
"pbac_desc_list_organization_members": "List organization members",
@@ -3369,7 +3368,7 @@
"pbac_desc_view_organization_bookings": "View organization bookings",
"pbac_desc_view_booking_recordings": "View booking recordings",
"pbac_desc_update_bookings": "Update bookings",
"pbac_desc_manage_bookings": "All actions on bookings across organization teams",
"pbac_desc_manage_bookings": "All actions on bookings",
"pbac_desc_view_team_insights": "View team insights",
"pbac_desc_manage_team_insights": "Manage team insights",
"read_permission_auto_enabled_tooltip": "Read permission is automatically enabled when creating, updating, or deleting a resource",
packages/features/ee/workflows/components/WorkflowDetailsPage.tsx
+1 -3
View File
@@ -27,7 +27,6 @@ interface WorkflowPermissions {
canView: boolean;
canUpdate: boolean;
canDelete: boolean;
canManage: boolean;
readOnly: boolean; // Keep for backward compatibility
}
@@ -62,7 +61,6 @@ export default function WorkflowDetailsPage(props: Props) {
canView: !teamId ? true : !props.readOnly,
canUpdate: !teamId ? true : !props.readOnly,
canDelete: !teamId ? true : !props.readOnly,
canManage: !teamId ? true : !props.readOnly,
readOnly: !teamId ? false : props.readOnly,
};
@@ -119,7 +117,7 @@ export default function WorkflowDetailsPage(props: Props) {
numberVerificationPending: false,
includeCalendarEvent: false,
verifiedAt: SCANNING_WORKFLOW_STEPS ? null : new Date(),
agentId: null
agentId: null,
};
steps?.push(step);
form.setValue("steps", steps);
packages/features/pbac/domain/types/permission-registry.ts
-43
View File
@@ -17,7 +17,6 @@ export enum CrudAction {
Read = "read",
Update = "update",
Delete = "delete",
Manage = "manage",
}
export enum CustomAction {
@@ -142,13 +141,6 @@ export const PERMISSION_REGISTRY: PermissionRegistry = {
i18nKey: "pbac_action_delete",
descriptionI18nKey: "pbac_desc_delete_roles",
},
[CrudAction.Manage]: {
description: "Manage roles on all sub-teams",
category: "role",
i18nKey: "pbac_action_manage",
descriptionI18nKey: "pbac_desc_manage_roles",
scope: [Scope.Organization], // Only organizations should have "Manage" permissions
},
},
[Resource.EventType]: {
_resource: {
@@ -178,13 +170,6 @@ export const PERMISSION_REGISTRY: PermissionRegistry = {
i18nKey: "pbac_action_delete",
descriptionI18nKey: "pbac_desc_delete_event_types",
},
[CrudAction.Manage]: {
description: "Manage event types",
category: "event",
i18nKey: "pbac_action_manage",
descriptionI18nKey: "pbac_desc_manage_event_types",
scope: [Scope.Organization], // Only organizations should have "Manage" permissions
},
},
[Resource.Team]: {
_resource: {
@@ -233,13 +218,6 @@ export const PERMISSION_REGISTRY: PermissionRegistry = {
i18nKey: "pbac_action_change_member_role",
descriptionI18nKey: "pbac_desc_change_team_member_role",
},
[CrudAction.Manage]: {
description: "Manage team members",
category: "team",
i18nKey: "pbac_action_manage",
descriptionI18nKey: "pbac_desc_manage_team_members",
scope: [Scope.Organization], // Only organizations should have "Manage" permissions
},
},
[Resource.Organization]: {
_resource: {
@@ -338,13 +316,6 @@ export const PERMISSION_REGISTRY: PermissionRegistry = {
i18nKey: "pbac_action_update",
descriptionI18nKey: "pbac_desc_update_bookings",
},
[CrudAction.Manage]: {
description: "Manage bookings",
category: "booking",
i18nKey: "pbac_action_manage",
descriptionI18nKey: "pbac_desc_manage_bookings",
scope: [Scope.Organization], // Only organizations should have "Manage" permissions
},
},
[Resource.Insights]: {
_resource: {
@@ -385,13 +356,6 @@ export const PERMISSION_REGISTRY: PermissionRegistry = {
i18nKey: "pbac_action_delete",
descriptionI18nKey: "pbac_desc_delete_workflows",
},
[CrudAction.Manage]: {
description: "Manage workflows",
category: "workflow",
i18nKey: "pbac_action_manage",
descriptionI18nKey: "pbac_desc_manage_workflows",
scope: [Scope.Organization],
},
},
[Resource.Attributes]: {
_resource: {
@@ -450,12 +414,5 @@ export const PERMISSION_REGISTRY: PermissionRegistry = {
i18nKey: "pbac_action_delete",
descriptionI18nKey: "pbac_desc_delete_routing_forms",
},
[CrudAction.Manage]: {
description: "Manage routing forms",
category: "routing",
i18nKey: "pbac_action_manage",
descriptionI18nKey: "pbac_desc_manage_routing_forms",
scope: [Scope.Organization],
},
},
};
packages/features/pbac/services/__tests__/permission-check.service.test.ts
+10 -249
View File
@@ -267,6 +267,15 @@ describe("PermissionCheckService", () => {
(MembershipRepository.findUniqueByUserIdAndTeamId as Mock).mockResolvedValueOnce(membership);
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
mockRepository.getMembershipByMembershipId.mockResolvedValueOnce({
id: membership.id,
teamId: membership.teamId,
userId: membership.userId,
customRoleId: membership.customRoleId,
team: { parentId: null },
});
mockRepository.getOrgMembership.mockResolvedValueOnce(null);
mockRepository.checkRolePermissions.mockResolvedValueOnce(false);
const result = await service.checkPermissions({
userId: 1,
@@ -276,8 +285,7 @@ describe("PermissionCheckService", () => {
});
expect(result).toBe(false);
// Should not call repository methods for empty permissions array (security measure)
expect(mockRepository.checkRolePermissions).not.toHaveBeenCalled();
expect(mockRepository.checkRolePermissions).toHaveBeenCalledWith("admin_role", []);
});
});
@@ -624,252 +632,5 @@ describe("PermissionCheckService", () => {
Resource.EventType
);
});
it("should expand permissions when user has manage permission", async () => {
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce({
id: 1,
teamId: 1,
userId: 1,
customRoleId: "admin_role",
team: { parentId: null },
});
// User has manage permission
mockRepository.getResourcePermissionsByRoleId.mockResolvedValueOnce(["manage", "read"]);
const result = await service.getResourcePermissions({
userId: 1,
teamId: 1,
resource: Resource.EventType,
});
// Should include all possible actions for eventType resource
expect(result).toContain("eventType.manage");
expect(result).toContain("eventType.create");
expect(result).toContain("eventType.read");
expect(result).toContain("eventType.update");
expect(result).toContain("eventType.delete");
expect(result.length).toBeGreaterThan(2); // More than just manage and read
});
it("should expand permissions when user has manage permission at org level", async () => {
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce({
id: 1,
teamId: 1,
userId: 1,
customRoleId: "team_role",
team: { parentId: 2 },
});
mockRepository.getOrgMembership.mockResolvedValueOnce({
id: 2,
teamId: 2,
userId: 1,
customRoleId: "admin_role",
});
// Team has basic permissions, org has manage
mockRepository.getResourcePermissionsByRoleId
.mockResolvedValueOnce(["read"]) // team permissions
.mockResolvedValueOnce(["manage"]); // org permissions
const result = await service.getResourcePermissions({
userId: 1,
teamId: 1,
resource: Resource.Role,
});
// Should include all possible actions for role resource due to manage permission
expect(result).toContain("role.manage");
expect(result).toContain("role.create");
expect(result).toContain("role.read");
expect(result).toContain("role.update");
expect(result).toContain("role.delete");
});
});
describe("hasPermission with manage permissions", () => {
it("should return true when user has manage permission for the resource", async () => {
const membership = {
id: 1,
teamId: 1,
userId: 1,
accepted: true,
role: "ADMIN" as MembershipRole,
customRoleId: "admin_role",
disableImpersonation: false,
createdAt: new Date(),
updatedAt: new Date(),
};
(MembershipRepository.findUniqueByUserIdAndTeamId as Mock).mockResolvedValueOnce(membership);
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
mockRepository.getMembershipByMembershipId.mockResolvedValueOnce({
id: membership.id,
teamId: membership.teamId,
userId: membership.userId,
customRoleId: membership.customRoleId,
team: { parentId: null },
});
mockRepository.getOrgMembership.mockResolvedValueOnce(null);
// User doesn't have explicit permission but has manage permission
mockRepository.checkRolePermission
.mockResolvedValueOnce(false) // explicit permission check fails
.mockResolvedValueOnce(true); // manage permission check succeeds
const result = await service.checkPermission({
userId: 1,
teamId: 1,
permission: "eventType.create",
fallbackRoles: ["ADMIN", "OWNER"],
});
expect(result).toBe(true);
expect(mockRepository.checkRolePermission).toHaveBeenCalledWith("admin_role", "eventType.create");
expect(mockRepository.checkRolePermission).toHaveBeenCalledWith("admin_role", "eventType.manage");
});
it("should return true when user has manage permission at org level", async () => {
const membership = {
id: 1,
teamId: 1,
userId: 1,
accepted: true,
role: "MEMBER" as MembershipRole,
customRoleId: "member_role",
disableImpersonation: false,
createdAt: new Date(),
updatedAt: new Date(),
};
(MembershipRepository.findUniqueByUserIdAndTeamId as Mock).mockResolvedValueOnce(membership);
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
mockRepository.getMembershipByMembershipId.mockResolvedValueOnce({
id: membership.id,
teamId: membership.teamId,
userId: membership.userId,
customRoleId: membership.customRoleId,
team: { parentId: 2 },
});
mockRepository.getOrgMembership.mockResolvedValueOnce({
id: 2,
teamId: 2,
userId: 1,
customRoleId: "admin_role",
});
// Team level permissions fail, org level manage permission succeeds
mockRepository.checkRolePermission
.mockResolvedValueOnce(false) // team explicit permission
.mockResolvedValueOnce(false) // team manage permission
.mockResolvedValueOnce(true); // org manage permission
const result = await service.checkPermission({
userId: 1,
teamId: 1,
permission: "role.delete",
fallbackRoles: ["ADMIN", "OWNER"],
});
expect(result).toBe(true);
expect(mockRepository.checkRolePermission).toHaveBeenCalledWith("admin_role", "role.manage");
});
});
describe("hasPermissions with manage permissions", () => {
it("should return true when user has manage permissions for all requested resources", async () => {
const membership = {
id: 1,
teamId: 1,
userId: 1,
accepted: true,
role: "ADMIN" as MembershipRole,
customRoleId: "admin_role",
disableImpersonation: false,
createdAt: new Date(),
updatedAt: new Date(),
};
(MembershipRepository.findUniqueByUserIdAndTeamId as Mock).mockResolvedValueOnce(membership);
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
mockRepository.getMembershipByMembershipId.mockResolvedValueOnce({
id: membership.id,
teamId: membership.teamId,
userId: membership.userId,
customRoleId: membership.customRoleId,
team: { parentId: null },
});
mockRepository.getOrgMembership.mockResolvedValueOnce(null);
// Explicit permissions check fails, but manage permissions succeed
mockRepository.checkRolePermissions.mockResolvedValueOnce(false);
mockRepository.checkRolePermission
.mockResolvedValueOnce(true) // eventType.manage
.mockResolvedValueOnce(true); // role.manage
const result = await service.checkPermissions({
userId: 1,
teamId: 1,
permissions: ["eventType.create", "eventType.update", "role.delete"],
fallbackRoles: ["ADMIN", "OWNER"],
});
expect(result).toBe(true);
expect(mockRepository.checkRolePermission).toHaveBeenCalledWith("admin_role", "eventType.manage");
expect(mockRepository.checkRolePermission).toHaveBeenCalledWith("admin_role", "role.manage");
});
it("should return false when user has manage permission for some but not all requested resources", async () => {
const membership = {
id: 1,
teamId: 1,
userId: 1,
accepted: true,
role: "ADMIN" as MembershipRole,
customRoleId: "admin_role",
disableImpersonation: false,
createdAt: new Date(),
updatedAt: new Date(),
};
(MembershipRepository.findUniqueByUserIdAndTeamId as Mock).mockResolvedValueOnce(membership);
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
mockRepository.getMembershipByMembershipId.mockResolvedValueOnce({
id: membership.id,
teamId: membership.teamId,
userId: membership.userId,
customRoleId: membership.customRoleId,
team: { parentId: 2 },
});
mockRepository.getOrgMembership.mockResolvedValueOnce({
id: 2,
teamId: 2,
userId: 1,
customRoleId: "admin_role",
});
// All explicit permission checks fail
mockRepository.checkRolePermissions
.mockResolvedValueOnce(false) // team permissions
.mockResolvedValueOnce(false); // org permissions
// Has manage for eventType but not for booking
mockRepository.checkRolePermission
.mockResolvedValueOnce(true) // team eventType.manage
.mockResolvedValueOnce(false) // team booking.manage
.mockResolvedValueOnce(true) // org eventType.manage (duplicate check)
.mockResolvedValueOnce(false); // org booking.manage
const result = await service.checkPermissions({
userId: 1,
teamId: 1,
permissions: ["eventType.create", "booking.update"],
fallbackRoles: ["ADMIN", "OWNER"],
});
expect(result).toBe(false);
});
});
});
packages/features/pbac/services/permission-check.service.ts
+2 -82
View File
@@ -13,7 +13,6 @@ import type {
CrudAction,
CustomAction,
} from "../domain/types/permission-registry";
import { PERMISSION_REGISTRY, filterResourceConfig } from "../domain/types/permission-registry";
import { PermissionRepository } from "../infrastructure/repositories/PermissionRepository";
import { PermissionService } from "./permission.service";
@@ -81,19 +80,6 @@ export class PermissionCheckService {
orgActions.forEach((action) => actions.add(action));
}
// Check if user has "manage" permission - if so, grant all actions for this resource
if (actions.has("manage" as CrudAction)) {
// Get all possible actions for this resource from the permission registry
const resourceConfig = PERMISSION_REGISTRY[resource];
if (resourceConfig) {
const allActions = Object.keys(filterResourceConfig(resourceConfig)) as (
| CrudAction
| CustomAction
)[];
allActions.forEach((action) => actions.add(action));
}
}
return Array.from(actions).map((action) => PermissionMapper.toPermissionString({ resource, action }));
} catch (error) {
this.logger.error(error);
@@ -212,22 +198,11 @@ export class PermissionCheckService {
permission
);
if (hasTeamPermission) return true;
// Check if user has manage permission for this resource
const [resource] = permission.split(".");
const managePermission = `${resource}.manage` as PermissionString;
const hasManagePermission = await this.repository.checkRolePermission(
membership.customRoleId,
managePermission
);
if (hasManagePermission) return true;
}
// If no team permission, check org-level permissions
if (orgMembership?.customRoleId) {
const [resource] = permission.split(".");
const managePermission = `${resource}.manage` as PermissionString;
return this.repository.checkRolePermission(orgMembership.customRoleId, managePermission);
return this.repository.checkRolePermission(orgMembership.customRoleId, permission);
}
return false;
@@ -237,11 +212,6 @@ export class PermissionCheckService {
* Internal method to check multiple permissions for a specific role
*/
private async hasPermissions(query: PermissionCheck, permissions: PermissionString[]): Promise<boolean> {
// Return false for empty permissions array to prevent privilege escalation
if (permissions.length === 0) {
return false;
}
const { membership, orgMembership } = await this.getMembership(query);
// First check team-level permissions
@@ -251,61 +221,11 @@ export class PermissionCheckService {
permissions
);
if (hasTeamPermissions) return true;
// Check if user has manage permissions for all requested resources
const resourcesWithManage = new Set<string>();
for (const permission of permissions) {
const [resource] = permission.split(".");
if (!resourcesWithManage.has(resource)) {
const managePermission = `${resource}.manage` as PermissionString;
const hasManagePermission = await this.repository.checkRolePermission(
membership.customRoleId,
managePermission
);
if (hasManagePermission) {
resourcesWithManage.add(resource);
}
}
}
// Check if all requested permissions are covered by manage permissions
const allPermissionsCovered = permissions.every((permission) => {
const [resource] = permission.split(".");
return resourcesWithManage.has(resource);
});
if (allPermissionsCovered) return true;
}
// If no team permissions, check org-level permissions
if (orgMembership?.customRoleId) {
const hasOrgPermissions = await this.repository.checkRolePermissions(
orgMembership.customRoleId,
permissions
);
if (hasOrgPermissions) return true;
// Check if user has manage permissions for all requested resources at org level
const resourcesWithManage = new Set<string>();
for (const permission of permissions) {
const [resource] = permission.split(".");
if (!resourcesWithManage.has(resource)) {
const managePermission = `${resource}.manage` as PermissionString;
const hasManagePermission = await this.repository.checkRolePermission(
orgMembership.customRoleId,
managePermission
);
if (hasManagePermission) {
resourcesWithManage.add(resource);
}
}
}
// Check if all requested permissions are covered by manage permissions
return permissions.every((permission) => {
const [resource] = permission.split(".");
return resourcesWithManage.has(resource);
});
return this.repository.checkRolePermissions(orgMembership.customRoleId, permissions);
}
return false;
packages/lib/server/repository/workflow-permissions.ts
+1 -6
View File
@@ -6,7 +6,6 @@ export interface WorkflowPermissions {
canView: boolean;
canUpdate: boolean;
canDelete: boolean;
canManage: boolean;
readOnly: boolean; // Keep for backward compatibility
}
@@ -36,18 +35,16 @@ export class WorkflowPermissionsBuilder {
const mockWorkflow = { id: 0, teamId, userId: null };
// Check all permissions in parallel for better performance
const [canView, canUpdate, canDelete, canManage] = await Promise.all([
const [canView, canUpdate, canDelete] = await Promise.all([
isAuthorized(mockWorkflow, this.currentUserId, "workflow.read"),
isAuthorized(mockWorkflow, this.currentUserId, "workflow.update"),
isAuthorized(mockWorkflow, this.currentUserId, "workflow.delete"),
isAuthorized(mockWorkflow, this.currentUserId, "workflow.manage"),
]);
const permissions = {
canView,
canUpdate,
canDelete,
canManage,
readOnly: !canUpdate,
};
@@ -64,7 +61,6 @@ export class WorkflowPermissionsBuilder {
canView: isOwner,
canUpdate: isOwner,
canDelete: isOwner,
canManage: isOwner,
readOnly: !isOwner,
};
}
@@ -80,7 +76,6 @@ export class WorkflowPermissionsBuilder {
canView: false,
canUpdate: false,
canDelete: false,
canManage: false,
readOnly: true,
};
}
packages/prisma/migrations/20250728091301_add_manage_permissions_to_default_roles/migration.sql
-24
View File
@@ -1,24 +0,0 @@
-- Add manage permissions to admin and owner roles
-- These permissions are for organization-level management capabilities
-- Insert manage permissions for admin role
INSERT INTO "RolePermission" (id, "roleId", resource, action, "createdAt")
SELECT
gen_random_uuid(), 'admin_role', resource, action, NOW()
FROM (
VALUES
-- Role management permissions (organization scope)
('role', 'manage'),
-- Event Type management permissions (organization scope)
('eventType', 'manage'),
-- Team management permissions (organization scope)
('team', 'manage'),
-- Booking management permissions (organization scope)
('booking', 'manage')
) AS permissions(resource, action)
ON CONFLICT ("roleId", resource, action) DO NOTHING;
-- Note: Owner role already has wildcard permissions (*.*) so it inherits all manage permissions automatically