fix: issues found in PBAC creation (#22659)

Author: sean-brydon

apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx

@@ -184,9 +184,11 @@ const organizationAdminKeys = [
 const useTabs = ({
   isDelegationCredentialEnabled,
   isPbacEnabled,
+  canViewRoles,
 }: {
   isDelegationCredentialEnabled: boolean;
   isPbacEnabled: boolean;
+  canViewRoles?: boolean;
 }) => {
   const session = useSession();
   const { data: user } = trpc.viewer.me.get.useQuery({ includePasswordAdded: true });
@@ -223,8 +225,9 @@ const useTabs = ({
           });
         }
 
-        // Add pbac menu item only if feature flag is enabled
-        if (isPbacEnabled) {
+        // Add pbac menu item only if feature flag is enabled AND user has permission to view roles
+        // This prevents showing the menu item when user has no organization permissions
+        if (isPbacEnabled && canViewRoles) {
           newArray.push({
             name: "roles_and_permissions",
             href: "/settings/organizations/roles",
@@ -291,6 +294,7 @@ interface SettingsSidebarContainerProps {
   navigationIsOpenedOnMobile?: boolean;
   bannersHeight?: number;
   teamFeatures?: Record<number, TeamFeatures>;
+  canViewRoles?: boolean;
 }
 
 const TeamRolesNavItem = ({
@@ -483,6 +487,7 @@ const SettingsSidebarContainer = ({
   navigationIsOpenedOnMobile,
   bannersHeight,
   teamFeatures,
+  canViewRoles,
 }: SettingsSidebarContainerProps) => {
   const searchParams = useCompatSearchParams();
   const orgBranding = useOrgBranding();
@@ -512,6 +517,7 @@ const SettingsSidebarContainer = ({
   const tabsWithPermissions = useTabs({
     isDelegationCredentialEnabled,
     isPbacEnabled,
+    canViewRoles,
   });
 
   const { data: otherTeams } = trpc.viewer.organizations.listOtherTeams.useQuery(undefined, {
@@ -786,9 +792,15 @@ export type SettingsLayoutProps = {
   children: React.ReactNode;
   containerClassName?: string;
   teamFeatures?: Record<number, TeamFeatures>;
+  canViewRoles?: boolean;
 } & ComponentProps<typeof Shell>;
 
-export default function SettingsLayoutAppDirClient({ children, teamFeatures, ...rest }: SettingsLayoutProps) {
+export default function SettingsLayoutAppDirClient({
+  children,
+  teamFeatures,
+  canViewRoles,
+  ...rest
+}: SettingsLayoutProps) {
   const pathname = usePathname();
   const state = useState(false);
   const [sideContainerOpen, setSideContainerOpen] = state;
@@ -821,6 +833,7 @@ export default function SettingsLayoutAppDirClient({ children, teamFeatures, ...
           sideContainerOpen={sideContainerOpen}
           setSideContainerOpen={setSideContainerOpen}
           teamFeatures={teamFeatures}
+          canViewRoles={canViewRoles}
         />
       }
       drawerState={state}
@@ -843,13 +856,15 @@ type SidebarContainerElementProps = {
   bannersHeight?: number;
   setSideContainerOpen: React.Dispatch<React.SetStateAction<boolean>>;
   teamFeatures?: Record<number, TeamFeatures>;
+  canViewRoles?: boolean;
 };
 
 const SidebarContainerElement = ({
   sideContainerOpen,
   bannersHeight,
   setSideContainerOpen,
   teamFeatures,
+  canViewRoles,
 }: SidebarContainerElementProps) => {
   const { t } = useLocale();
   return (
@@ -866,6 +881,7 @@ const SidebarContainerElement = ({
         navigationIsOpenedOnMobile={sideContainerOpen}
         bannersHeight={bannersHeight}
         teamFeatures={teamFeatures}
+        canViewRoles={canViewRoles}
       />
     </>
   );

apps/web/app/(use-page-wrapper)/settings/(settings-layout)/layout.tsx

@@ -6,6 +6,9 @@ import React from "react";
 import { getServerSession } from "@calcom/features/auth/lib/getServerSession";
 import type { TeamFeatures } from "@calcom/features/flags/config";
 import { FeaturesRepository } from "@calcom/features/flags/features.repository";
+import { PermissionMapper } from "@calcom/features/pbac/domain/mappers/PermissionMapper";
+import { Resource, CrudAction } from "@calcom/features/pbac/domain/types/permission-registry";
+import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service";
 
 import { buildLegacyRequest } from "@lib/buildLegacyCtx";
 
@@ -23,6 +26,15 @@ const getTeamFeatures = unstable_cache(
   }
 );
 
+const getCachedResourcePermissions = unstable_cache(
+  async (userId: number, teamId: number, resource: Resource) => {
+    const permissionService = new PermissionCheckService();
+    return permissionService.getResourcePermissions({ userId, teamId, resource });
+  },
+  ["resource-permissions"],
+  { revalidate: 120 }
+);
+
 export default async function SettingsLayoutAppDir(props: SettingsLayoutProps) {
   const session = await getServerSession({ req: buildLegacyRequest(await headers(), await cookies()) });
   const userId = session?.user?.id;
@@ -31,20 +43,30 @@ export default async function SettingsLayoutAppDir(props: SettingsLayoutProps) {
   }
 
   let teamFeatures: Record<number, TeamFeatures> | null = null;
+  let canViewRoles = false;
   const orgId = session?.user?.profile?.organizationId ?? session?.user.org?.id;
+
   // For now we only grab organization features but it would be nice to fetch these on the server side for specific team feature flags
   if (orgId) {
-    const features = await getTeamFeatures(orgId);
+    const [features, rolePermissions] = await Promise.all([
+      getTeamFeatures(orgId),
+      getCachedResourcePermissions(userId, orgId, Resource.Role),
+    ]);
+
     if (features) {
       teamFeatures = {
         [orgId]: features,
       };
+
+      // Check if user has permission to read roles
+      const roleActions = PermissionMapper.toActionMap(rolePermissions, Resource.Role);
+      canViewRoles = roleActions[CrudAction.Read] ?? false;
     }
   }
 
   return (
     <>
-      <SettingsLayoutAppDirClient {...props} teamFeatures={teamFeatures ?? {}} />
+      <SettingsLayoutAppDirClient {...props} teamFeatures={teamFeatures ?? {}} canViewRoles={canViewRoles} />
     </>
   );
 }

apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/AdvancedPermissionGroup.tsx

@@ -54,6 +54,12 @@ export function AdvancedPermissionGroup({
     }
   };
 
+  const handleCheckedChange = (checked: boolean | string) => {
+    if (!disabled) {
+      onChange(toggleResourcePermissionLevel(resource, checked ? "all" : "none", selectedPermissions));
+    }
+  };
+
   // Helper function to check if read permission is auto-enabled
   const isReadAutoEnabled = (action: string) => {
     if (action === CrudAction.Read) return false;
@@ -68,25 +74,38 @@ export function AdvancedPermissionGroup({
       <button
         type="button"
         className="flex cursor-pointer items-center justify-between gap-1.5 p-4"
-        onClick={() => setIsExpanded(!isExpanded)}>
-        <Icon
-          name={isAllResources ? "chevron-right" : "chevron-down"}
-          className={classNames(
-            "h-4 w-4 transition-transform",
-            isExpanded && !isAllResources ? "rotate-180" : ""
-          )}
-        />
+        onClick={(e) => {
+          // Only toggle expansion if clicking on the button itself, not child elements
+          if (e.target === e.currentTarget) {
+            setIsExpanded(!isExpanded);
+          }
+        }}>
+        <div className="flex items-center gap-1.5" onClick={() => setIsExpanded(!isExpanded)}>
+          <Icon
+            name="chevron-right"
+            className={classNames(
+              "h-4 w-4 transition-transform",
+              isExpanded && !isAllResources ? "rotate-90" : ""
+            )}
+          />
+        </div>
         <div className="flex items-center gap-2">
           <Checkbox
             checked={isAllSelected}
-            onCheckedChange={() => handleToggleAll}
+            onCheckedChange={handleCheckedChange}
             onClick={handleToggleAll}
             disabled={disabled}
           />
-          <span className="text-default text-sm font-medium leading-none">
+          <span
+            className="text-default cursor-pointer text-sm font-medium leading-none"
+            onClick={() => setIsExpanded(!isExpanded)}>
             {t(resourceConfig._resource?.i18nKey || "")}
           </span>
-          <span className="text-muted text-sm font-medium leading-none">{t("all_permissions")}</span>
+          <span
+            className="text-muted cursor-pointer text-sm font-medium leading-none"
+            onClick={() => setIsExpanded(!isExpanded)}>
+            {t("all_permissions")}
+          </span>
         </div>
       </button>
       {isExpanded && !isAllResources && (

apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts

@@ -19,9 +19,11 @@ export function usePermissions(): UsePermissionsReturn {
     const permissions: string[] = [];
     Object.entries(PERMISSION_REGISTRY).forEach(([resource, config]) => {
       if (resource !== "*") {
-        Object.keys(config).forEach((action) => {
-          permissions.push(`${resource}.${action}`);
-        });
+        Object.keys(config)
+          .filter((action) => !action.startsWith("_"))
+          .forEach((action) => {
+            permissions.push(`${resource}.${action}`);
+          });
       }
     });
     return permissions;
@@ -30,7 +32,9 @@ export function usePermissions(): UsePermissionsReturn {
   const hasAllPermissions = (permissions: string[]) => {
     return Object.entries(PERMISSION_REGISTRY).every(([resource, config]) => {
       if (resource === "*") return true;
-      return Object.keys(config).every((action) => permissions.includes(`${resource}.${action}`));
+      return Object.keys(config)
+        .filter((action) => !action.startsWith("_"))
+        .every((action) => permissions.includes(`${resource}.${action}`));
     });
   };
 
@@ -42,7 +46,10 @@ export function usePermissions(): UsePermissionsReturn {
     const resourceConfig = PERMISSION_REGISTRY[resource as keyof typeof PERMISSION_REGISTRY];
     if (!resourceConfig) return "none";
 
-    const allResourcePerms = Object.keys(resourceConfig).map((action) => `${resource}.${action}`);
+    // Filter out internal keys like _resource when checking permissions
+    const allResourcePerms = Object.keys(resourceConfig)
+      .filter((action) => !action.startsWith("_"))
+      .map((action) => `${resource}.${action}`);
     const hasAllPerms = allResourcePerms.every((p) => permissions.includes(p));
     const hasReadPerm = permissions.includes(`${resource}.${CrudAction.Read}`);
 
@@ -73,6 +80,9 @@ export function usePermissions(): UsePermissionsReturn {
 
       if (!resourceConfig) return currentPermissions;
 
+      // Declare variable before switch to avoid scope issues
+      let allResourcePerms: string[];
+
       switch (level) {
         case "none":
           // No permissions to add, just keep other permissions
@@ -82,8 +92,10 @@ export function usePermissions(): UsePermissionsReturn {
           newPermissions.push(`${resource}.${CrudAction.Read}`);
           break;
         case "all":
-          // Add all permissions for this resource
-          const allResourcePerms = Object.keys(resourceConfig).map((action) => `${resource}.${action}`);
+          // Add all permissions for this resource (excluding internal keys)
+          allResourcePerms = Object.keys(resourceConfig)
+            .filter((action) => !action.startsWith("_"))
+            .map((action) => `${resource}.${action}`);
           newPermissions.push(...allResourcePerms);
           break;
       }

apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/actions.ts

@@ -9,7 +9,12 @@ export async function revalidateTeamRoles(teamId: number) {
   // Revalidate team roles paths (dynamic routes)
   revalidatePath("/settings/teams/[id]/roles", "page");
 
-  // Invalidate team-specific cache tags
+  // Invalidate cache tags that match the unstable_cache keys
+  revalidateTag("team-roles");
+  revalidateTag("resource-permissions");
+  revalidateTag("team-feature");
+
+  // Also invalidate team-specific cache tags for completeness
   revalidateTag(`team-roles-${teamId}`);
   revalidateTag(`resource-permissions-${teamId}`);
   revalidateTag(`team-members-${teamId}`);