fix: unable to edit member as a team owner/admin (#25587)
* fix: unaable to edit member as owner/admin * Clean up comments in EditMemberSheet.test.tsx Removed comments explaining the fix for permissions prop. * revert
This commit is contained in:
+8
-60
@@ -5,13 +5,11 @@ import { headers, cookies } from "next/headers";
|
||||
import { redirect } from "next/navigation";
|
||||
|
||||
import { getServerSession } from "@calcom/features/auth/lib/getServerSession";
|
||||
import { Resource, CustomAction } from "@calcom/features/pbac/domain/types/permission-registry";
|
||||
import { getSpecificPermissions } from "@calcom/features/pbac/lib/resource-permissions";
|
||||
import { getTeamMemberPermissions } from "@calcom/features/pbac/lib/team-member-permissions";
|
||||
import { RoleManagementFactory } from "@calcom/features/pbac/services/role-management.factory";
|
||||
import SettingsHeader from "@calcom/features/settings/appDir/SettingsHeader";
|
||||
import { PrismaAttributeRepository } from "@calcom/lib/server/repository/PrismaAttributeRepository";
|
||||
import { prisma } from "@calcom/prisma";
|
||||
import { MembershipRole } from "@calcom/prisma/enums";
|
||||
import { viewerTeamsRouter } from "@calcom/trpc/server/routers/viewer/teams/_router";
|
||||
|
||||
import { buildLegacyRequest } from "@lib/buildLegacyCtx";
|
||||
@@ -33,7 +31,7 @@ const getCachedTeamRoles = unstable_cache(
|
||||
try {
|
||||
const roleManager = await RoleManagementFactory.getInstance().createRoleManager(organizationId);
|
||||
return await roleManager.getTeamRoles(teamId);
|
||||
} catch (error) {
|
||||
} catch {
|
||||
// PBAC not enabled or error occurred, return empty array
|
||||
return [];
|
||||
}
|
||||
@@ -49,7 +47,7 @@ const getCachedTeamAttributes = unstable_cache(
|
||||
|
||||
try {
|
||||
return await attributeRepo.findAllByOrgIdWithOptions({ orgId: organizationId });
|
||||
} catch (error) {
|
||||
} catch {
|
||||
return [];
|
||||
}
|
||||
},
|
||||
@@ -79,65 +77,15 @@ const Page = async ({ params }: { params: Promise<{ id: string }> }) => {
|
||||
const organizationId = team.parentId || teamId;
|
||||
|
||||
// Load PBAC roles and attributes if available
|
||||
const [roles, attributes] = await Promise.all([
|
||||
const [roles, attributes, memberPermissions] = await Promise.all([
|
||||
getCachedTeamRoles(teamId, organizationId),
|
||||
getCachedTeamAttributes(organizationId),
|
||||
getTeamMemberPermissions({
|
||||
userId: session.user.id,
|
||||
team,
|
||||
}),
|
||||
]);
|
||||
|
||||
const fallbackRolesCanListMembers: MembershipRole[] = [MembershipRole.ADMIN, MembershipRole.OWNER];
|
||||
|
||||
// If the team is not private we allow members to list other members
|
||||
if (!team.isPrivate) {
|
||||
fallbackRolesCanListMembers.push(MembershipRole.MEMBER);
|
||||
}
|
||||
|
||||
// Get specific PBAC permissions for team member actions
|
||||
const permissions = await getSpecificPermissions({
|
||||
userId: session.user.id,
|
||||
teamId: teamId,
|
||||
resource: Resource.Team,
|
||||
userRole: team.membership.role,
|
||||
actions: [
|
||||
CustomAction.Invite,
|
||||
CustomAction.ChangeMemberRole,
|
||||
CustomAction.Remove,
|
||||
CustomAction.ListMembers,
|
||||
CustomAction.ListMembersPrivate,
|
||||
CustomAction.Impersonate,
|
||||
],
|
||||
fallbackRoles: {
|
||||
[CustomAction.Invite]: {
|
||||
roles: [MembershipRole.ADMIN, MembershipRole.OWNER],
|
||||
},
|
||||
[CustomAction.ChangeMemberRole]: {
|
||||
roles: [MembershipRole.ADMIN, MembershipRole.OWNER],
|
||||
},
|
||||
[CustomAction.Remove]: {
|
||||
roles: [MembershipRole.ADMIN, MembershipRole.OWNER],
|
||||
},
|
||||
[CustomAction.ListMembers]: {
|
||||
roles: fallbackRolesCanListMembers,
|
||||
},
|
||||
[CustomAction.Impersonate]: {
|
||||
roles: [MembershipRole.ADMIN, MembershipRole.OWNER],
|
||||
},
|
||||
[CustomAction.ListMembersPrivate]: {
|
||||
roles: fallbackRolesCanListMembers,
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
// Map specific permissions to member actions
|
||||
const memberPermissions = {
|
||||
canListMembers: team.isPrivate
|
||||
? permissions[CustomAction.ListMembersPrivate]
|
||||
: permissions[CustomAction.ListMembers],
|
||||
canInvite: permissions[CustomAction.Invite],
|
||||
canChangeMemberRole: permissions[CustomAction.ChangeMemberRole],
|
||||
canRemove: permissions[CustomAction.Remove],
|
||||
canImpersonate: permissions[CustomAction.Impersonate],
|
||||
};
|
||||
|
||||
const facetedTeamValues = {
|
||||
roles,
|
||||
teams: [team],
|
||||
|
||||
@@ -0,0 +1,327 @@
|
||||
import { render } from "@testing-library/react";
|
||||
import React, { type ReactNode } from "react";
|
||||
import { vi } from "vitest";
|
||||
|
||||
import type { MemberPermissions } from "@calcom/features/users/components/UserTable/types";
|
||||
import { MembershipRole } from "@calcom/prisma/enums";
|
||||
|
||||
import { EditMemberSheet } from "./EditMemberSheet";
|
||||
import type { State, User } from "./MemberList";
|
||||
|
||||
// Mock dependencies
|
||||
vi.mock("@calcom/lib/hooks/useLocale", () => ({
|
||||
useLocale: () => ({
|
||||
t: (key: string) => key,
|
||||
}),
|
||||
}));
|
||||
|
||||
vi.mock("@calcom/trpc/react", () => ({
|
||||
trpc: {
|
||||
viewer: {
|
||||
pbac: {
|
||||
getTeamRoles: {
|
||||
useQuery: () => ({
|
||||
data: undefined,
|
||||
isPending: false,
|
||||
}),
|
||||
},
|
||||
},
|
||||
teams: {
|
||||
getUserConnectedApps: {
|
||||
useQuery: () => ({
|
||||
data: {},
|
||||
isPending: false,
|
||||
}),
|
||||
},
|
||||
changeMemberRole: {
|
||||
useMutation: () => ({
|
||||
mutate: vi.fn(),
|
||||
mutateAsync: vi.fn(),
|
||||
}),
|
||||
},
|
||||
listMembers: {
|
||||
cancel: vi.fn(),
|
||||
getInfiniteData: vi.fn(),
|
||||
invalidate: vi.fn(),
|
||||
},
|
||||
get: {
|
||||
setData: vi.fn(),
|
||||
invalidate: vi.fn(),
|
||||
},
|
||||
},
|
||||
},
|
||||
useUtils: () => ({
|
||||
viewer: {
|
||||
teams: {
|
||||
listMembers: {
|
||||
cancel: vi.fn(),
|
||||
getInfiniteData: vi.fn(),
|
||||
invalidate: vi.fn(),
|
||||
},
|
||||
get: {
|
||||
setData: vi.fn(),
|
||||
invalidate: vi.fn(),
|
||||
},
|
||||
},
|
||||
},
|
||||
}),
|
||||
},
|
||||
}));
|
||||
|
||||
const mockSetEditMode = vi.fn();
|
||||
const mockSetMutationLoading = vi.fn();
|
||||
|
||||
vi.mock("@calcom/features/users/components/UserTable/EditSheet/store", () => ({
|
||||
useEditMode: vi.fn((selector) => {
|
||||
const state = {
|
||||
editMode: false,
|
||||
setEditMode: mockSetEditMode,
|
||||
mutationLoading: false,
|
||||
setMutationLoading: mockSetMutationLoading,
|
||||
};
|
||||
if (typeof selector === "function") {
|
||||
return selector(state);
|
||||
}
|
||||
return state;
|
||||
}),
|
||||
}));
|
||||
|
||||
// Mock SheetFooterControls to verify props
|
||||
let capturedProps: { canChangeMemberRole?: boolean; canEditAttributesForUser?: boolean }[] = [];
|
||||
vi.mock("@calcom/features/users/components/UserTable/EditSheet/SheetFooterControls", () => ({
|
||||
SheetFooterControls: (props: { canChangeMemberRole?: boolean; canEditAttributesForUser?: boolean }) => {
|
||||
capturedProps.push(props);
|
||||
return React.createElement("div", { "data-testid": "sheet-footer-controls" }, "SheetFooterControls");
|
||||
},
|
||||
}));
|
||||
|
||||
// Mock other UI components
|
||||
vi.mock("@calcom/ui/components/sheet", () => ({
|
||||
Sheet: ({ children }: { children: ReactNode }) =>
|
||||
React.createElement("div", { "data-testid": "sheet" }, children),
|
||||
SheetContent: ({ children }: { children: ReactNode }) =>
|
||||
React.createElement("div", { "data-testid": "sheet-content" }, children),
|
||||
SheetHeader: ({ children }: { children: ReactNode }) =>
|
||||
React.createElement("div", { "data-testid": "sheet-header" }, children),
|
||||
SheetBody: ({ children }: { children: ReactNode }) =>
|
||||
React.createElement("div", { "data-testid": "sheet-body" }, children),
|
||||
SheetFooter: ({ children }: { children: ReactNode }) =>
|
||||
React.createElement("div", { "data-testid": "sheet-footer" }, children),
|
||||
}));
|
||||
|
||||
vi.mock("@calcom/ui/components/form", () => ({
|
||||
Form: ({ children }: { children: ReactNode }) => React.createElement("form", null, children),
|
||||
ToggleGroup: () => React.createElement("div", { "data-testid": "toggle-group" }, "ToggleGroup"),
|
||||
Select: () => React.createElement("div", { "data-testid": "select" }, "Select"),
|
||||
}));
|
||||
|
||||
vi.mock("@calcom/ui/components/avatar", () => ({
|
||||
Avatar: () => React.createElement("div", { "data-testid": "avatar" }, "Avatar"),
|
||||
}));
|
||||
|
||||
vi.mock("@calcom/ui/components/skeleton", () => ({
|
||||
Skeleton: ({ children }: { children: ReactNode }) => React.createElement("div", null, children),
|
||||
Loader: () => React.createElement("div", { "data-testid": "loader" }, "Loading..."),
|
||||
}));
|
||||
|
||||
vi.mock("@calcom/features/users/components/UserTable/EditSheet/DisplayInfo", () => ({
|
||||
DisplayInfo: () => <div data-testid="display-info">DisplayInfo</div>,
|
||||
}));
|
||||
|
||||
describe("EditMemberSheet", () => {
|
||||
const mockDispatch = vi.fn();
|
||||
|
||||
// Create a minimal mock user that satisfies the User type
|
||||
const mockUser = {
|
||||
id: 1,
|
||||
name: "Test User",
|
||||
email: "test@example.com",
|
||||
username: "testuser",
|
||||
role: MembershipRole.MEMBER,
|
||||
accepted: true,
|
||||
avatarUrl: "",
|
||||
bookerUrl: "https://cal.com",
|
||||
lastActiveAt: "2024-01-01",
|
||||
customRoleId: null,
|
||||
} as User;
|
||||
|
||||
const mockState: State = {
|
||||
editSheet: {
|
||||
user: mockUser,
|
||||
showModal: true,
|
||||
},
|
||||
deleteMember: {
|
||||
showModal: false,
|
||||
},
|
||||
impersonateMember: {
|
||||
showModal: false,
|
||||
},
|
||||
teamAvailability: {
|
||||
showModal: false,
|
||||
},
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
mockSetEditMode.mockClear();
|
||||
mockSetMutationLoading.mockClear();
|
||||
capturedProps = [];
|
||||
});
|
||||
|
||||
describe("Fix verification: canChangeMemberRole prop passing", () => {
|
||||
it("should pass canChangeMemberRole=true to SheetFooterControls when permissions.canChangeMemberRole is true", () => {
|
||||
const permissions: MemberPermissions = {
|
||||
canListMembers: false,
|
||||
canInvite: false,
|
||||
canChangeMemberRole: true,
|
||||
canRemove: false,
|
||||
canImpersonate: false,
|
||||
};
|
||||
|
||||
render(
|
||||
<EditMemberSheet
|
||||
state={mockState}
|
||||
dispatch={mockDispatch}
|
||||
currentMember={MembershipRole.OWNER}
|
||||
teamId={1}
|
||||
permissions={permissions}
|
||||
/>
|
||||
);
|
||||
|
||||
// Verify SheetFooterControls was called with canChangeMemberRole=true
|
||||
expect(capturedProps).toContainEqual(
|
||||
expect.objectContaining({
|
||||
canChangeMemberRole: true,
|
||||
})
|
||||
);
|
||||
});
|
||||
|
||||
it("should pass canChangeMemberRole=false to SheetFooterControls when permissions.canChangeMemberRole is false", () => {
|
||||
const permissions: MemberPermissions = {
|
||||
canListMembers: false,
|
||||
canInvite: false,
|
||||
canChangeMemberRole: false,
|
||||
canRemove: false,
|
||||
canImpersonate: false,
|
||||
};
|
||||
|
||||
render(
|
||||
<EditMemberSheet
|
||||
state={mockState}
|
||||
dispatch={mockDispatch}
|
||||
currentMember={MembershipRole.OWNER}
|
||||
teamId={1}
|
||||
permissions={permissions}
|
||||
/>
|
||||
);
|
||||
|
||||
// Verify SheetFooterControls was called with canChangeMemberRole=false
|
||||
expect(capturedProps).toContainEqual(
|
||||
expect.objectContaining({
|
||||
canChangeMemberRole: false,
|
||||
})
|
||||
);
|
||||
});
|
||||
|
||||
it("should pass canChangeMemberRole=undefined to SheetFooterControls when permissions is undefined", () => {
|
||||
render(
|
||||
<EditMemberSheet
|
||||
state={mockState}
|
||||
dispatch={mockDispatch}
|
||||
currentMember={MembershipRole.OWNER}
|
||||
teamId={1}
|
||||
permissions={undefined}
|
||||
/>
|
||||
);
|
||||
|
||||
// Verify SheetFooterControls was called with canChangeMemberRole=undefined
|
||||
expect(capturedProps).toContainEqual(
|
||||
expect.objectContaining({
|
||||
canChangeMemberRole: undefined,
|
||||
})
|
||||
);
|
||||
});
|
||||
|
||||
it("should pass canChangeMemberRole=undefined to SheetFooterControls when permissions object exists but canChangeMemberRole is undefined", () => {
|
||||
const permissions: Partial<MemberPermissions> = {
|
||||
canListMembers: true,
|
||||
canInvite: false,
|
||||
// canChangeMemberRole is intentionally omitted
|
||||
};
|
||||
|
||||
render(
|
||||
<EditMemberSheet
|
||||
state={mockState}
|
||||
dispatch={mockDispatch}
|
||||
currentMember={MembershipRole.OWNER}
|
||||
teamId={1}
|
||||
permissions={permissions as MemberPermissions}
|
||||
/>
|
||||
);
|
||||
|
||||
// Verify SheetFooterControls was called with canChangeMemberRole=undefined
|
||||
expect(capturedProps).toContainEqual(
|
||||
expect.objectContaining({
|
||||
canChangeMemberRole: undefined,
|
||||
})
|
||||
);
|
||||
});
|
||||
|
||||
it("should pass canEditAttributesForUser from permissions to SheetFooterControls", () => {
|
||||
const permissions: MemberPermissions = {
|
||||
canListMembers: false,
|
||||
canInvite: false,
|
||||
canChangeMemberRole: true,
|
||||
canRemove: false,
|
||||
canImpersonate: false,
|
||||
canEditAttributesForUser: true,
|
||||
};
|
||||
|
||||
render(
|
||||
<EditMemberSheet
|
||||
state={mockState}
|
||||
dispatch={mockDispatch}
|
||||
currentMember={MembershipRole.OWNER}
|
||||
teamId={1}
|
||||
permissions={permissions}
|
||||
/>
|
||||
);
|
||||
|
||||
// Verify SheetFooterControls was called with both props
|
||||
expect(capturedProps).toContainEqual(
|
||||
expect.objectContaining({
|
||||
canChangeMemberRole: true,
|
||||
canEditAttributesForUser: true,
|
||||
})
|
||||
);
|
||||
});
|
||||
|
||||
it("should correctly extract canChangeMemberRole from permissions object (verifies fix)", () => {
|
||||
// This test specifically verifies the fix where permissions?.canChangeMemberRole
|
||||
// is now correctly passed to SheetFooterControls
|
||||
const permissions: MemberPermissions = {
|
||||
canListMembers: false,
|
||||
canInvite: false,
|
||||
canChangeMemberRole: true, // This should be passed to SheetFooterControls
|
||||
canRemove: false,
|
||||
canImpersonate: false,
|
||||
};
|
||||
|
||||
render(
|
||||
<EditMemberSheet
|
||||
state={mockState}
|
||||
dispatch={mockDispatch}
|
||||
currentMember={MembershipRole.OWNER}
|
||||
teamId={1}
|
||||
permissions={permissions}
|
||||
/>
|
||||
);
|
||||
|
||||
expect(capturedProps).toContainEqual(
|
||||
expect.objectContaining({
|
||||
canChangeMemberRole: true,
|
||||
})
|
||||
);
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -8,6 +8,7 @@ import { shallow } from "zustand/shallow";
|
||||
import { DisplayInfo } from "@calcom/features/users/components/UserTable/EditSheet/DisplayInfo";
|
||||
import { SheetFooterControls } from "@calcom/features/users/components/UserTable/EditSheet/SheetFooterControls";
|
||||
import { useEditMode } from "@calcom/features/users/components/UserTable/EditSheet/store";
|
||||
import type { MemberPermissions } from "@calcom/features/users/components/UserTable/types";
|
||||
import { useLocale } from "@calcom/lib/hooks/useLocale";
|
||||
import { MembershipRole } from "@calcom/prisma/enums";
|
||||
import { trpc } from "@calcom/trpc/react";
|
||||
@@ -34,11 +35,13 @@ export function EditMemberSheet({
|
||||
dispatch,
|
||||
currentMember,
|
||||
teamId,
|
||||
permissions,
|
||||
}: {
|
||||
state: State;
|
||||
dispatch: Dispatch<Action>;
|
||||
currentMember: MembershipRole;
|
||||
teamId: number;
|
||||
permissions?: MemberPermissions;
|
||||
}) {
|
||||
const { t } = useLocale();
|
||||
const { user } = state.editSheet;
|
||||
@@ -277,7 +280,10 @@ export function EditMemberSheet({
|
||||
</div>
|
||||
</SheetBody>
|
||||
<SheetFooter className="mt-auto">
|
||||
<SheetFooterControls />
|
||||
<SheetFooterControls
|
||||
canChangeMemberRole={permissions?.canChangeMemberRole}
|
||||
canEditAttributesForUser={permissions?.canEditAttributesForUser}
|
||||
/>
|
||||
</SheetFooter>
|
||||
</Form>
|
||||
) : (
|
||||
|
||||
@@ -830,6 +830,7 @@ function MemberListContent(props: Props) {
|
||||
state={state}
|
||||
currentMember={props.team.membership.role}
|
||||
teamId={props.team.id}
|
||||
permissions={props.permissions}
|
||||
/>
|
||||
)}
|
||||
</>
|
||||
|
||||
@@ -0,0 +1,80 @@
|
||||
import type { MemberPermissions } from "@calcom/features/users/components/UserTable/types";
|
||||
import { MembershipRole } from "@calcom/prisma/enums";
|
||||
|
||||
import { Resource, CustomAction } from "../domain/types/permission-registry";
|
||||
import { getSpecificPermissions } from "./resource-permissions";
|
||||
|
||||
interface TeamWithMembership {
|
||||
id: number;
|
||||
isPrivate: boolean;
|
||||
membership: {
|
||||
role: MembershipRole;
|
||||
accepted: boolean;
|
||||
};
|
||||
}
|
||||
|
||||
interface GetTeamMemberPermissionsOptions {
|
||||
userId: number;
|
||||
team: TeamWithMembership;
|
||||
}
|
||||
|
||||
/**
|
||||
* Gets team member permissions using PBAC or fallback to role-based permissions.
|
||||
*/
|
||||
export async function getTeamMemberPermissions({
|
||||
userId,
|
||||
team,
|
||||
}: GetTeamMemberPermissionsOptions): Promise<MemberPermissions> {
|
||||
// Determine fallback roles for ListMembers based on team privacy
|
||||
const fallbackRolesCanListMembers: MembershipRole[] = [MembershipRole.ADMIN, MembershipRole.OWNER];
|
||||
if (!team.isPrivate) {
|
||||
fallbackRolesCanListMembers.push(MembershipRole.MEMBER);
|
||||
}
|
||||
|
||||
// Get specific PBAC permissions for team member actions
|
||||
const permissions = await getSpecificPermissions({
|
||||
userId,
|
||||
teamId: team.id,
|
||||
resource: Resource.Team,
|
||||
userRole: team.membership.role,
|
||||
actions: [
|
||||
CustomAction.Invite,
|
||||
CustomAction.ChangeMemberRole,
|
||||
CustomAction.Remove,
|
||||
CustomAction.ListMembers,
|
||||
CustomAction.ListMembersPrivate,
|
||||
CustomAction.Impersonate,
|
||||
],
|
||||
fallbackRoles: {
|
||||
[CustomAction.Invite]: {
|
||||
roles: [MembershipRole.ADMIN, MembershipRole.OWNER],
|
||||
},
|
||||
[CustomAction.ChangeMemberRole]: {
|
||||
roles: [MembershipRole.ADMIN, MembershipRole.OWNER],
|
||||
},
|
||||
[CustomAction.Remove]: {
|
||||
roles: [MembershipRole.ADMIN, MembershipRole.OWNER],
|
||||
},
|
||||
[CustomAction.ListMembers]: {
|
||||
roles: fallbackRolesCanListMembers,
|
||||
},
|
||||
[CustomAction.Impersonate]: {
|
||||
roles: [MembershipRole.ADMIN, MembershipRole.OWNER],
|
||||
},
|
||||
[CustomAction.ListMembersPrivate]: {
|
||||
roles: fallbackRolesCanListMembers,
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
// Map specific permissions to member actions
|
||||
return {
|
||||
canListMembers: team.isPrivate
|
||||
? permissions[CustomAction.ListMembersPrivate]
|
||||
: permissions[CustomAction.ListMembers],
|
||||
canInvite: permissions[CustomAction.Invite],
|
||||
canChangeMemberRole: permissions[CustomAction.ChangeMemberRole],
|
||||
canRemove: permissions[CustomAction.Remove],
|
||||
canImpersonate: permissions[CustomAction.Impersonate],
|
||||
};
|
||||
}
|
||||
Reference in New Issue
Block a user