[dev] [tofikwest] tofik/update-organization-owner #1876
Conversation
…le the section del organization only for owner
PR SummaryAdds POST
Written by Cursor Bugbot for commit 5342d90. This will update automatically on new commits. Configure here. |
Graphite Automations"Auto-assign PRs to Author" took an action on this PR • (12/08/25)1 reviewer was added to this PR based on Mariano Fuentes's automation. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // Get current user's member record | ||
| const currentUserMember = await db.member.findFirst({ | ||
| where: { organizationId: orgId, userId: currentUserId }, | ||
| }); |
There was a problem hiding this comment.
Bug: Missing deactivated check allows deactivated owner transfer
The currentUserMember query does not filter by deactivated: false, unlike similar patterns elsewhere in the codebase (e.g., add-comment.ts, change-organization.ts). This inconsistency means a deactivated member who still has the owner role could potentially execute the ownership transfer action. The new owner lookup correctly includes the deactivated: false check, but the current user's permission check does not.
| params: Promise<{ orgId: string }>; | ||
| }) { | ||
| const { orgId } = await params; | ||
| console.log('[OrganizationSettings Debug] orgId:', orgId); |
There was a problem hiding this comment.
Bug: Debug console.log statement left in production code
A console.log statement with a debug label [OrganizationSettings Debug] was left in the code. This appears to be debugging code that was accidentally committed and will output to logs in production, potentially leaking organization IDs in server logs.
apps/app/src/components/forms/organization/transfer-ownership.tsx
Outdated
Show resolved
Hide resolved
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| const allMembers = await db.member.findMany({ | ||
| where: { | ||
| organizationId: orgId, | ||
| }, | ||
| select: { | ||
| id: true, | ||
| userId: true, | ||
| deactivated: true, | ||
| role: true, | ||
| user: { | ||
| select: { | ||
| name: true, | ||
| email: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The allMembers query is executed but never used. This performs an unnecessary database query on every page load, fetching all members including deactivated ones. This wastes database resources and adds latency.
// Remove this unused query:
// const allMembers = await db.member.findMany({
// where: {
// organizationId: orgId,
// },
// ...
// });If this was intended for debugging, it should be removed before merging to production.
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
| if (!authContext.userId) { | ||
| return { | ||
| success: false, | ||
| message: 'User ID is required for this operation', | ||
| }; | ||
| } |
There was a problem hiding this comment.
Returns HTTP 200 with success: false when userId is missing, but the endpoint is already protected by @UseGuards(HybridAuthGuard). If authentication passes but userId is somehow missing, this indicates a critical auth system issue that should throw an exception, not return a 200 response.
This creates inconsistent error handling - other validation errors throw exceptions (400, 403, 404) but this returns 200. API consumers cannot reliably check HTTP status codes.
if (!authContext.userId) {
throw new UnauthorizedException('User ID is required for this operation');
}| if (!authContext.userId) { | |
| return { | |
| success: false, | |
| message: 'User ID is required for this operation', | |
| }; | |
| } | |
| if (!authContext.userId) { | |
| throw new UnauthorizedException('User ID is required for this operation'); | |
| } |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
…ransfer ownership
|
|
||
| return organization; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Bug: Missing authorization check allows cross-organization data access
The organizationDetails function fetches organization data using the orgId directly from URL params without validating that it matches the user's activeOrganizationId from their session. Previously this function used session?.session.activeOrganizationId from the cached session. Other similar functions in the codebase (like getContextEntries) properly verify session.session.activeOrganizationId === orgId before returning data. This could allow users to view organization details for any organization by manipulating the URL.
| // Get current user's member record | ||
| const currentUserMember = await db.member.findFirst({ | ||
| where: { organizationId, userId: currentUserId }, | ||
| }); |
There was a problem hiding this comment.
Bug: Deactivated owner can still transfer ownership
The currentUserMember query in transferOwnership doesn't filter by deactivated: false, unlike the query for newOwnerMember and other similar member queries elsewhere in the codebase (e.g., in hybrid-auth.guard.ts and comments.service.ts). This inconsistency could allow a deactivated member who still has the owner role to transfer ownership.
|
🎉 This PR is included in version 1.70.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This is an automated pull request to merge tofik/update-organization-owner into dev.
It was created by the [Auto Pull Request] action.