-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix: enforce admin permissions for services (Backend) #3418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: canary
Are you sure you want to change the base?
fix: enforce admin permissions for services (Backend) #3418
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses a critical backend authorization issue where admin and owner roles were not properly checked when performing service and project operations. The fix ensures that admins and owners have full access to create, delete, and access services and projects without requiring explicit permission flags.
- Adds admin/owner role checks to all service and project permission functions
- Introduces new environment management permission functions with role-based access
- Adds email validation to the
updateUserfunction
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/server/src/services/user.ts
Outdated
| throw new Error("Email is required and cannot be empty"); | ||
| } | ||
|
|
||
| // Basic email format validation | ||
| const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | ||
| if (!emailRegex.test(userData.email)) { | ||
| throw new Error("Please enter a valid email address"); |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handling in this function uses throw new Error instead of TRPCError which is the pattern used throughout the rest of this file for validation and permission errors. This inconsistency could lead to errors being handled differently by the TRPC framework and may not provide proper error codes to API consumers.
| throw new Error("Email is required and cannot be empty"); | |
| } | |
| // Basic email format validation | |
| const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | |
| if (!emailRegex.test(userData.email)) { | |
| throw new Error("Please enter a valid email address"); | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Email is required and cannot be empty", | |
| }); | |
| } | |
| // Basic email format validation | |
| const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | |
| if (!emailRegex.test(userData.email)) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Please enter a valid email address", | |
| }); |
packages/server/src/services/user.ts
Outdated
| export const canPerformAccessEnvironment = async ( | ||
| userId: string, | ||
| environmentId: string, | ||
| organizationId: string, | ||
| ) => { | ||
| const { accessedEnvironments } = await findMemberById(userId, organizationId); | ||
| const haveAccessToEnvironment = accessedEnvironments.includes(environmentId); | ||
|
|
||
| if (haveAccessToEnvironment) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| }; |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The canPerformAccessEnvironment function is missing the admin/owner role check that has been added to all other similar permission functions. Admins and owners should have access to all environments without needing to be in the accessedEnvironments list. This inconsistency means admins/owners may be incorrectly denied access to environments.
packages/server/src/services/user.ts
Outdated
| if (!member) { | ||
| throw new TRPCError({ | ||
| code: "UNAUTHORIZED", | ||
| message: "User not found in organization", | ||
| }); | ||
| } | ||
|
|
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null check for member is redundant because findMemberById already throws a TRPCError with code "UNAUTHORIZED" if the member is not found (line 381-386). This check will never be reached since the function would have already thrown an exception.
| if (!member) { | |
| throw new TRPCError({ | |
| code: "UNAUTHORIZED", | |
| message: "User not found in organization", | |
| }); | |
| } |
packages/server/src/services/user.ts
Outdated
| // Get user's member record | ||
| const member = await findMemberById(userId, organizationId); | ||
|
|
||
| if (!member) { | ||
| throw new TRPCError({ | ||
| code: "UNAUTHORIZED", | ||
| message: "User not found in organization", | ||
| }); | ||
| } | ||
|
|
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null check for member is redundant because findMemberById already throws a TRPCError with code "UNAUTHORIZED" if the member is not found (line 381-386). This check will never be reached since the function would have already thrown an exception.
| // Get user's member record | |
| const member = await findMemberById(userId, organizationId); | |
| if (!member) { | |
| throw new TRPCError({ | |
| code: "UNAUTHORIZED", | |
| message: "User not found in organization", | |
| }); | |
| } | |
| // Get user's member record (throws if not found) | |
| const member = (await findMemberById(userId, organizationId))!; |
Siumauricio
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, these changes are not necessary. This method is only used when dealing with a member role; it does not apply when you are an owner or administrator. The issue you mentioned as related fixes all permission issues that were not showing up in the user interface, but there should be no problem in the backend. If you can give me an example of a use case, I can look into it, thanks
This PR fixes the issue where admins were not able to create services or projects because the backend was checking for specific permissions flags instead of the admin role.
This complements PR #3410 which addresses the UI side of this issue.
Fixes #3417