refactor permissions feature to use zustand#124
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive role-based permission system under src/features/Permissions/v1. It integrates permission boundaries, gates, and hooks across several existing features (Add Member, Contact, Events, and Projects) to conditionally render UI elements and restrict unauthorized actions. Key feedback points out a critical security vulnerability where user permissions could leak when switching accounts due to a non-reset state, non-reactive user state retrieval in the permission service, an unstable dependency in the usePermissionMap hook, and redundant permission wrappers in the member header.
| import { useEffect } from "react"; | ||
| import { useQuery } from "@tanstack/react-query"; | ||
| import { | ||
| fetchUserPermissions, | ||
| getCurrentUser, | ||
| getPermissionErrorMessage, | ||
| permissionQueryKeys, | ||
| } from "./permissions.service"; | ||
| import { usePermissionStore } from "./permissions.store"; | ||
|
|
||
| export function PermissionBootstrap() { | ||
| const startLoading = usePermissionStore((state) => state.startLoading); | ||
| const setPermissions = usePermissionStore((state) => state.setPermissions); | ||
| const setError = usePermissionStore((state) => state.setError); | ||
| const user = getCurrentUser(); | ||
|
|
||
| const query = useQuery({ | ||
| queryKey: permissionQueryKeys.byRole(user.role), | ||
| queryFn: () => fetchUserPermissions(user), | ||
| staleTime: 1000 * 60 * 5, | ||
| }); | ||
|
|
||
| useEffect(() => { | ||
| if (query.isPending) { | ||
| startLoading(); | ||
| return; | ||
| } | ||
|
|
||
| if (query.isError) { | ||
| setError(getPermissionErrorMessage(query.error)); | ||
| return; | ||
| } | ||
|
|
||
| if (query.data) { | ||
| setPermissions(query.data); | ||
| } | ||
| }, [query.data, query.error, query.isError, query.isPending, setError, setPermissions, startLoading]); | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Security Vulnerability: Privilege Leakage & Non-Reactive User State
- Privilege Leakage on User Switch: When a user logs out and a different user logs in, the global permission store is not reset. Because
startLoadingsets the status to"success"ifpermissions.length > 0, the new user will temporarily inherit the previous user's permissions during the loading phase (which takes at least 250ms due to the mock delay). This can lead to privilege escalation or information disclosure. - Non-Reactive User State:
getCurrentUser()is not reactive because it accesses the Zustand store state imperatively viauseAuthStore.getState().userduring render, meaning permission fetching won't trigger automatically when the authenticated user changes.
Solution
- Refactor
getCurrentUserinto a reactiveuseCurrentUserhook. - Call
reset()on the permission store whenever the user's role changes to prevent privilege leakage.
import { useEffect } from "react";
import { useQuery } from "@tanstack/react-query";
import {
fetchUserPermissions,
useCurrentUser,
getPermissionErrorMessage,
permissionQueryKeys,
} from "./permissions.service";
import { usePermissionStore } from "./permissions.store";
export function PermissionBootstrap() {
const startLoading = usePermissionStore((state) => state.startLoading);
const setPermissions = usePermissionStore((state) => state.setPermissions);
const setError = usePermissionStore((state) => state.setError);
const reset = usePermissionStore((state) => state.reset);
const user = useCurrentUser();
const query = useQuery({
queryKey: permissionQueryKeys.byRole(user.role),
queryFn: () => fetchUserPermissions(user),
staleTime: 1000 * 60 * 5,
});
useEffect(() => {
reset();
}, [user.role, reset]);
useEffect(() => {
if (query.isPending) {
startLoading();
return;
}
if (query.isError) {
setError(getPermissionErrorMessage(query.error));
return;
}
if (query.data) {
setPermissions(query.data);
}
}, [query.data, query.error, query.isError, query.isPending, setError, setPermissions, startLoading]);
return null;
}
| import useAuthStore from "@/features/Auth/v1/Store/Auth.Store"; | ||
| import { dashboardData } from "@/features/Member/v1/mock/dashboardData"; | ||
| import { type User } from "@/features/Dashboard/Member/v1/Type/dashboard"; | ||
| import { | ||
| App_Permissions, | ||
| Contact_Permissions, | ||
| Event_Permissions, | ||
| Member_Permissions, | ||
| Project_Permissions, | ||
| type Permission, | ||
| } from "./constants"; | ||
| import { type PermissionQueryUser } from "./types"; | ||
|
|
||
| const ROLE_PERMISSION_MAP: Record<User["role"], Permission[]> = { | ||
| Admin: Object.values(App_Permissions), | ||
| Member: [ | ||
| Event_Permissions.VIEW_EVENT, | ||
| Event_Permissions.JOIN_EVENT, | ||
| Event_Permissions.LEAVE_EVENT, | ||
| Member_Permissions.VIEW_MEMBER, | ||
| Contact_Permissions.VIEW_CONTACT_DIRECTORY, | ||
| Contact_Permissions.SUBMIT_SUPPORT_TICKET, | ||
| Project_Permissions.VIEW_PROJECT, | ||
| ], | ||
| }; | ||
|
|
||
| export const permissionQueryKeys = { | ||
| all: ["permissions"] as const, | ||
| byRole: (role: string) => [...permissionQueryKeys.all, role] as const, | ||
| }; | ||
|
|
||
| export async function fetchUserPermissions( | ||
| user: PermissionQueryUser = getCurrentUser(), | ||
| ): Promise<Permission[]> { | ||
| await new Promise((resolve) => setTimeout(resolve, 250)); | ||
| return ROLE_PERMISSION_MAP[user.role as User["role"]] ?? []; | ||
| } | ||
|
|
||
| export function getCurrentUser(): PermissionQueryUser { | ||
| const authenticatedUser = useAuthStore.getState().user; | ||
|
|
||
| if (authenticatedUser?.role === "Admin" || authenticatedUser?.role === "Member") { | ||
| return { | ||
| name: authenticatedUser.email, | ||
| role: authenticatedUser.role, | ||
| }; | ||
| } | ||
|
|
||
| return dashboardData.user; | ||
| } |
There was a problem hiding this comment.
Non-Reactive User State Retrieval
The getCurrentUser function retrieves the user state using useAuthStore.getState().user. This is non-reactive and will not trigger a re-render or re-fetch when the authenticated user changes (e.g., on login/logout).
Refactoring this into a custom React hook useCurrentUser that subscribes to the store via useAuthStore((state) => state.user) ensures reactivity and correctness.
import { useMemo } from "react";
import useAuthStore from "@/features/Auth/v1/Store/Auth.Store";
import { dashboardData } from "@/features/Member/v1/mock/dashboardData";
import { type User } from "@/features/Dashboard/Member/v1/Type/dashboard";
import {
App_Permissions,
Contact_Permissions,
Event_Permissions,
Member_Permissions,
Project_Permissions,
type Permission,
} from "./constants";
import { type PermissionQueryUser } from "./types";
const ROLE_PERMISSION_MAP: Record<User["role"], Permission[]> = {
Admin: Object.values(App_Permissions),
Member: [
Event_Permissions.VIEW_EVENT,
Event_Permissions.JOIN_EVENT,
Event_Permissions.LEAVE_EVENT,
Member_Permissions.VIEW_MEMBER,
Contact_Permissions.VIEW_CONTACT_DIRECTORY,
Contact_Permissions.SUBMIT_SUPPORT_TICKET,
Project_Permissions.VIEW_PROJECT,
],
};
export const permissionQueryKeys = {
all: ["permissions"] as const,
byRole: (role: string) => [...permissionQueryKeys.all, role] as const,
};
export async function fetchUserPermissions(
user: PermissionQueryUser,
): Promise<Permission[]> {
await new Promise((resolve) => setTimeout(resolve, 250));
return ROLE_PERMISSION_MAP[user.role as User["role"]] ?? [];
}
export function useCurrentUser(): PermissionQueryUser {
const authenticatedUser = useAuthStore((state) => state.user);
return useMemo(() => {
if (authenticatedUser?.role === "Admin" || authenticatedUser?.role === "Member") {
return {
name: authenticatedUser.email,
role: authenticatedUser.role,
};
}
return dashboardData.user;
}, [authenticatedUser]);
}| const accessMap = useMemo(() => { | ||
| return Object.entries(config).reduce( | ||
| (result, [key, requirement]) => { | ||
| result[key as keyof T] = hasRequiredPermissions( | ||
| permissions, | ||
| normalizePermissions(requirement), | ||
| "any", | ||
| ); | ||
|
|
||
| return result; | ||
| }, | ||
| {} as { [Key in keyof T]: boolean }, | ||
| ); | ||
| }, [config, permissions]); |
There was a problem hiding this comment.
Unstable Dependency in useMemo
The config object passed to usePermissionMap is typically an object literal created on every render. Because it is used directly in the dependency array of useMemo, the memoized value will be re-computed on every single render, defeating the purpose of useMemo.
Serializing the config object using JSON.stringify(config) in the dependency array ensures that the memoized value is only re-computed when the actual permission requirements change.
| const accessMap = useMemo(() => { | |
| return Object.entries(config).reduce( | |
| (result, [key, requirement]) => { | |
| result[key as keyof T] = hasRequiredPermissions( | |
| permissions, | |
| normalizePermissions(requirement), | |
| "any", | |
| ); | |
| return result; | |
| }, | |
| {} as { [Key in keyof T]: boolean }, | |
| ); | |
| }, [config, permissions]); | |
| const accessMap = useMemo(() => { | |
| return Object.entries(config).reduce( | |
| (result, [key, requirement]) => { | |
| result[key as keyof T] = hasRequiredPermissions( | |
| permissions, | |
| normalizePermissions(requirement), | |
| "any", | |
| ); | |
| return result; | |
| }, | |
| {} as { [Key in keyof T]: boolean }, | |
| ); | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [JSON.stringify(config), permissions]); |
| <PermissionGate permission={Member_Permissions.CREATE_MEMBER}> | ||
| <Button | ||
| text="Discard Draft" | ||
| variant="secondary" | ||
| onClick={() => alert("Discard Draft clicked")} | ||
| /> | ||
| </PermissionGate> | ||
| <PermissionGate permission={Member_Permissions.CREATE_MEMBER}> | ||
| <Button text="Create Member" onClick={() => alert("Create Member clicked")} /> | ||
| </PermissionGate> |
There was a problem hiding this comment.
Redundant PermissionGate Wrappers
Both the "Discard Draft" and "Create Member" buttons are wrapped in separate PermissionGate components with the exact same permission (Member_Permissions.CREATE_MEMBER). Wrapping them in a single PermissionGate is cleaner, reduces JSX nesting, and avoids redundant permission evaluations.
<PermissionGate permission={Member_Permissions.CREATE_MEMBER}>
<Button
text="Discard Draft"
variant="secondary"
onClick={() => alert("Discard Draft clicked")}
/>
<Button text="Create Member" onClick={() => alert("Create Member clicked")} />
</PermissionGate>
Closes #60
Summary
This PR implements a centralized permission-based UI rendering system so pages and actions are shown only when the current user has the required permissions.
It introduces a reusable permissions feature, loads permissions with TanStack Query, stores permission state globally with Zustand, and updates the affected pages/components to render UI conditionally based on access.
What Changed
src/features/Permissions/v1Event_PermissionsMember_PermissionsContact_PermissionsProject_PermissionsPermissionBootstrapto fetch and initialize permissions on app loaduseAuthorizationusePermissionMapPermissionGatePermissionBoundaryAccessDeniedPermissionLoadingAffected Areas
Why
This makes permission checks easier to understand, easier to reuse, and more consistent across the app. It also gives us a single place to extend permission handling in future work without duplicating logic across pages.
Notes
5 minutestale timeTesting
npm run buildis still blocked by unrelated pre-existing syntax issues in other files and not by this permission feature