Enable specific users to have admin powers to delete any poll #124
Conversation
|
Caution Review failedThe pull request is closed. """ WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UserController
participant UserService
participant Database
Client->>UserController: GET /listAdmins
UserController->>UserService: listAdmins()
UserService->>Database: Query users where isAdmin == true
Database-->>UserService: List of admin users
UserService-->>UserController: Array of admin worldIDs
UserController-->>Client: Array of admin worldIDs
sequenceDiagram
participant Client
participant PollService
participant UserService
participant Database
Client->>PollService: deletePoll(pollId, worldID)
PollService->>Database: Get poll by pollId
PollService->>Database: Get user by worldID (select id, isAdmin)
PollService->>PollService: Check if user is author or isAdmin
alt Authorized
PollService->>Database: Delete poll
PollService-->>Client: Success
else Unauthorized
PollService-->>Client: UnauthorizedActionException
end
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/poll/poll.service.ts (1)
564-567: Authorization logic is correct, consider adding audit logging.The authorization logic properly allows both admins and poll authors to delete polls. However, consider adding audit logging for admin deletions to track when admins delete polls they didn't create.
Consider adding audit logging for admin actions:
// Allow deletion if user is admin or if user is the poll author if (!user.isAdmin && poll.authorUserId !== user.id) { throw new UnauthorizedActionException() } + // Log admin deletions for audit purposes + if (user.isAdmin && poll.authorUserId !== user.id) { + // Add your logging implementation here + console.log(`Admin ${worldID} deleted poll ${pollId} created by user ${poll.authorUserId}`) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
prisma/schema.prisma(1 hunks)src/poll/poll.service.ts(2 hunks)src/user/user.controller.ts(1 hunks)src/user/user.dto.ts(1 hunks)src/user/user.service.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/user/user.controller.ts (1)
src/auth/jwt-auth.guard.ts (1)
Public(16-16)
🔇 Additional comments (4)
prisma/schema.prisma (1)
15-15: LGTM! Well-implemented admin flag.The
isAdminBoolean field with a default value offalsefollows security best practices by ensuring users are not granted admin privileges by default.src/user/user.dto.ts (1)
44-46: LGTM! Proper DTO validation implementation.The
isAdminfield is correctly implemented with appropriate validation decorators (@IsBoolean()and@IsNotEmpty()).src/poll/poll.service.ts (1)
553-553: Good implementation of admin field selection.The user query correctly includes the
isAdminfield needed for the updated authorization logic.src/user/user.service.ts (1)
114-114: Admin status exposure in response.The
isAdminfield is now returned in the user data response. This maintains consistency with the database query but raises the same security consideration about exposing administrative privileges.
| @Get('listAdmins') | ||
| @Public() | ||
| async listAdmins(): Promise<string[]> { | ||
| return await this.userService.listAdmins() | ||
| } |
There was a problem hiding this comment.
Security Risk: Public admin user list exposure.
The listAdmins endpoint is marked with @Public(), making it accessible without authentication. This exposes the list of admin users to anyone, which could be a significant security risk as attackers could use this information for targeted attacks.
Consider implementing proper authorization to restrict access to this endpoint.
Apply this diff to add proper authorization:
@Get('listAdmins')
- @Public()
async listAdmins(): Promise<string[]> {
return await this.userService.listAdmins()
}Or if admin-only access is needed, add an admin guard:
@Get('listAdmins')
- @Public()
+ @UseGuards(AdminGuard) // You may need to implement this guard
async listAdmins(): Promise<string[]> {
return await this.userService.listAdmins()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Get('listAdmins') | |
| @Public() | |
| async listAdmins(): Promise<string[]> { | |
| return await this.userService.listAdmins() | |
| } | |
| @Get('listAdmins') | |
| - @Public() | |
| + @UseGuards(AdminGuard) // You may need to implement and import this guard | |
| async listAdmins(): Promise<string[]> { | |
| return await this.userService.listAdmins() | |
| } |
🤖 Prompt for AI Agents
In src/user/user.controller.ts around lines 75 to 79, the listAdmins endpoint is
marked as @Public(), exposing the admin user list without authentication. To fix
this, remove the @Public() decorator and apply an authorization guard that
restricts access to authenticated users or specifically to admins. Use an
appropriate guard or role-based access control to ensure only authorized users
can call this endpoint.
| worldID: true, | ||
| name: true, | ||
| profilePicture: true, | ||
| isAdmin: true, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify if admin status should be publicly exposed.
The isAdmin field is now included in the database query for user data retrieval. While the implementation is correct, consider whether admin status should be publicly accessible through the getUserData endpoint.
🏁 Script executed:
#!/bin/bash
# Description: Check if getUserData endpoint has authorization restrictions
# Expected: Find controller method and any authorization decorators
ast-grep --pattern $'getUserData($$$) {
$$$
}'
# Also check for any authorization decorators in the user controller
rg -A 5 -B 5 "getUserData|@.*Auth|@.*Guard" src/user/user.controller.tsLength of output: 505
Restrict public exposure of admin status
The getUserData endpoint in src/user/user.controller.ts is publicly accessible (no authorization decorators), which means the new isAdmin flag can be fetched by any caller. Exposing admin status in an open API poses a security risk and may aid malicious reconnaissance.
• File: src/user/user.controller.ts
– Method: getUserData (no @UseGuards(...) or equivalent)
• Snippet:
@Get('getUserData')
- async getUserData(
+ @UseGuards(AuthGuard) // <-- add appropriate guard
+ @ApiBearerAuth() // <-- document secured endpoint
+ @ApiOperation({ summary: 'Retrieve user data (authenticated)' })
async getUserData(Please either restrict this endpoint with proper authorization checks (e.g. JWT/session guard) or remove isAdmin from the public response.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/user/user.service.ts at line 100, the isAdmin field is included in the
user data returned by the getUserData endpoint, which is publicly accessible
without authorization in src/user/user.controller.ts. To fix this, either remove
the isAdmin field from the data returned by getUserData to prevent exposing
admin status publicly, or add proper authorization guards (e.g., JWT or session
guards) to the getUserData controller method to restrict access only to
authorized users.
| async listAdmins(): Promise<string[]> { | ||
| const admins = await this.databaseService.user.findMany({ | ||
| where: { isAdmin: true }, | ||
| select: { worldID: true }, | ||
| }) | ||
| return admins.map(admin => admin.worldID) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Critical: Missing authorization for admin list endpoint.
The listAdmins method returns worldIDs of all admin users without any authorization checks. This could expose sensitive information about system administrators to unauthorized users.
Consider adding proper authorization to restrict access to this endpoint:
+ // Add authorization decorator in controller
+ @UseGuards(AdminGuard) // or similar authorization mechanism
async listAdmins(): Promise<string[]> {
const admins = await this.databaseService.user.findMany({
where: { isAdmin: true },
select: { worldID: true },
})
return admins.map(admin => admin.worldID)
}Additionally, verify if returning admin worldIDs is necessary or if the endpoint should be restricted to specific use cases.
🏁 Script executed:
#!/bin/bash
# Description: Check how this method is used in the controller and if there are authorization guards
# Expected: Find the controller method and any authorization decorators
rg -A 10 -B 5 "listAdmins" src/user/user.controller.ts
rg -A 5 -B 5 "@.*Guard|@.*Auth" src/user/user.controller.tsLength of output: 384
Critical: Secure the listAdmins endpoint with proper authorization
The controller’s listAdmins handler is currently marked @Public() and exposes all admin worldIDs without any guard.
• File: src/user/user.controller.ts
Method: async listAdmins()
Proposed fix – restrict access to authenticated admins only:
- @Get('listAdmins')
- @Public()
+ @Get('listAdmins')
+ @UseGuards(JwtAuthGuard, RolesGuard) // or your existing AdminGuard
+ @Roles('admin') // ensure only users with isAdmin=true
async listAdmins(): Promise<string[]> {
return await this.userService.listAdmins();
}• Remove the @Public() decorator.
• Apply your authentication guard (e.g. JwtAuthGuard) and a roles/admin guard.
• Verify that your RolesGuard (or AdminGuard) checks user.isAdmin.
• Double-check whether returning a list of all admin worldIDs is strictly necessary or can be scoped further.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/user/user.service.ts lines 462 to 468, the listAdmins method returns all
admin worldIDs without any authorization checks, exposing sensitive data. To fix
this, remove the @Public() decorator from the corresponding controller method in
src/user/user.controller.ts, add authentication guards like JwtAuthGuard, and
apply a roles or admin guard that verifies user.isAdmin. Also, review if
returning all admin worldIDs is necessary or if the data should be scoped or
restricted further to minimize exposure.
RamRamez
deleted the
enable-specific-users-to-have-admin-powers-to-delete-any-poll
branch
Summary by CodeRabbit