add roles based on sigs #51
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR implements role-based access control by introducing a new "SIG Executive" role that allows specific users to manage events only for their assigned SIGs (Special Interest Groups). Committee members retain full access to all events, while the new SIG Executive role provides scoped permissions based on SIG assignments.
Changes:
- Added
SigExecutiveuser role with SIG-based permissions using a newsigsfield in user metadata and database - Implemented authorization functions (
canManageSig,isEventManager) to control event and registration management based on role and SIG assignments - Updated API routes to enforce SIG-based permissions for event creation, updates, deletion, and registration management
- Added comprehensive test coverage for SIG Executive role across events and registrations
- Added NeuroTechSig as a new SIG with corresponding configuration and styling
Reviewed changes
Copilot reviewed 26 out of 28 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/shared/src/users/constants.ts | Added SigExecutive role constant |
| apps/shared/src/users/schemas.ts | Added canManageSig and isEventManager authorization helper functions |
| apps/shared/src/core/constants.ts | Added NeuroTechSig to the Sigs enum |
| apps/api/src/db/schema.ts | Updated roles enum and added sigs JSON column to users table |
| apps/api/src/types/globals.d.ts | Added sigs field to JWT session claims and Fastify request user |
| apps/api/src/lib/auth-guard.ts | Renamed requireCommittee to requireEventManager and updated to check both roles |
| apps/api/src/lib/errors.ts | Added ForbiddenError class for authorization failures |
| apps/api/src/modules/events/service.ts | Added SIG-based filtering for draft events accessible to SIG executives |
| apps/api/src/modules/events/route.ts | Added authorization checks for create, update, and delete operations based on SIG permissions |
| apps/api/src/modules/events/route.test.ts | Added comprehensive tests for SIG Executive role permissions |
| apps/api/src/modules/registration/service.ts | Updated delete logic to check SIG permissions |
| apps/api/src/modules/registration/route.ts | Added SIG-based authorization checks for all registration management endpoints |
| apps/api/src/modules/registration/route.test.ts | Added tests for SIG Executive registration management permissions |
| apps/api/src/modules/webhooks/clerk.ts | Updated to handle sigs field in user metadata during user creation |
| apps/api/tests/mock-auth.ts | Added helper functions for testing SIG Executive and Member roles |
| apps/api/drizzle/* | Database migration files for role and sigs column changes |
| apps/web/src/lib/auth.ts | Added useEventManagerAuth hook with SIG management utilities |
| apps/web/src/components/layout/protected-route.tsx | Updated to use requireEventManager prop instead of isRequireCommittee |
| apps/web/src/components/forms/modify-event-form.tsx | Filtered available SIGs based on user role and assignments |
| apps/web/src/routes/events/create.tsx | Set default organiser based on user's assigned SIGs |
| apps/web/src/routes/events/$eventId/index.tsx | Updated to check SIG-specific management permissions |
| apps/web/src/routes/events/$eventId/edit.tsx | Added requireEventManager protection and updated UI text |
| apps/web/src/routes/events/$eventId/analytics.tsx | Added requireEventManager protection |
| apps/web/src/routes/events/draft.tsx | Added requireEventManager protection |
| apps/web/src/config/sigs.ts | Added NeuroTechSig configuration with colors and branding |
| apps/web/public/sigs/neurotech.webp | Added NeuroTechSig logo image |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isSigExecutive && sigs && sigs.length > 0 && !filters.state) { | ||
| const draftEvents = await eventStore.get({ | ||
| db, | ||
| filters: { ...filters, state: EventState.Draft }, | ||
| }); | ||
|
|
||
| const sigDraftEvents = draftEvents.filter((e) => sigs.includes(e.organiser as Sigs)); | ||
| events = [...events, ...sigDraftEvents]; | ||
|
|
||
| events = events | ||
| .filter((event, index, self) => index === self.findIndex((e) => e.id === event.id)) | ||
| .sort((a, b) => new Date(b.date).getTime() - new Date(a.date).getTime()); | ||
| } |
There was a problem hiding this comment.
The logic for SIG executives viewing draft events has a potential issue. If a SIG executive explicitly requests draft events with filters.state = EventState.Draft, the condition !filters.state will be false, and they won't be able to see their own SIG's draft events. The check should be filters.state !== EventState.Published or the logic should be restructured to properly handle when a SIG executive explicitly wants to see draft events.
| const defaultOrganiser = isCommittee | ||
| ? Sigs.Compsoc | ||
| : sigs && sigs.length > 0 | ||
| ? sigs[0] | ||
| : Sigs.Compsoc |
There was a problem hiding this comment.
If a SIG executive has no assigned sigs (sigs is undefined or an empty array), the defaultOrganiser will be set to Compsoc. However, when they try to create the event, the API will reject it because they don't have permission to manage Compsoc events. This will result in a confusing user experience. Consider either redirecting users without assigned sigs to an error page, or handling this edge case more gracefully.
| if (!existingEvent) { | ||
| throw new ForbiddenError("Event not found"); | ||
| } |
There was a problem hiding this comment.
When updating a non-existent event, a ForbiddenError is thrown with the message "Event not found" (line 88). This is semantically incorrect - a 403 Forbidden status code should be used for authorization failures, not for missing resources. A NotFoundError (404) would be more appropriate when the event doesn't exist. The same issue exists in the DELETE endpoint at line 123.
| if (!existingEvent) { | ||
| throw new ForbiddenError("Event not found"); | ||
| } |
There was a problem hiding this comment.
When deleting a non-existent event, a ForbiddenError is thrown with the message "Event not found". This is semantically incorrect - a 403 Forbidden status code should be used for authorization failures, not for missing resources. A NotFoundError (404) would be more appropriate when the event doesn't exist.
| children: ReactNode | ||
| activeTab?: string | ||
| isRequireCommittee?: boolean | ||
| requireEventManager?: boolean |
There was a problem hiding this comment.
The prop name was changed from isRequireCommittee to requireEventManager, but there's at least one usage in the codebase that still uses the old prop name. The file apps/web/src/routes/me/index.tsx at line 22 uses isRequireCommittee={false}. This will cause the prop to be ignored since it doesn't match the new parameter name, potentially breaking the intended behavior.
| const existingSigs = data.public_metadata?.sigs; | ||
|
|
||
| if (!existingRole) { | ||
| await clerkClient.users.updateUserMetadata(data.id, { | ||
| publicMetadata: { |
There was a problem hiding this comment.
When a user is created without a role, the webhook sets the role to "member" but doesn't preserve existing sigs metadata. If a user is pre-created with sigs but no role, the sigs will be lost when the metadata is updated. The publicMetadata update should preserve existing sigs by spreading the existing metadata or explicitly including the sigs field.
| const existingSigs = data.public_metadata?.sigs; | |
| if (!existingRole) { | |
| await clerkClient.users.updateUserMetadata(data.id, { | |
| publicMetadata: { | |
| const existingSigs = data.public_metadata?.sigs; | |
| const existingMetadata = data.public_metadata ?? {}; | |
| if (!existingRole) { | |
| await clerkClient.users.updateUserMetadata(data.id, { | |
| publicMetadata: { | |
| ...existingMetadata, |
No description provided.