testing changes for codebase context #48
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughA new FastAPI user management module is introduced with Firebase-backed profile and bulk user management endpoints, integrated into the main application. The module handles user profile retrieval, profile updates, and admin-restricted bulk user operations (disable/delete) with role-based access control. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Summary of ChangesHello @visz11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's user administration capabilities by introducing a new module dedicated to user management. It provides self-service options for users to view and update their profiles, alongside powerful bulk action functionalities for administrators to manage user accounts efficiently. This change centralizes user-related operations and improves the overall administrative control within the system. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Refacto PR SummaryAdded comprehensive user management functionality with profile management and admin bulk operations. The implementation provides authenticated users with profile viewing/updating capabilities while enabling administrators to perform bulk actions (disable/delete) on multiple users through Firebase Admin SDK integration. Key Changes:
Change HighlightsClick to expand
Sequence DiagramsequenceDiagram
participant U as User
participant API as FastAPI App
participant Auth as Firebase Auth
participant Admin as Firebase Admin
U->>API: GET /users/profile
API->>Auth: get_current_user()
Auth-->>API: User token validated
API->>Admin: get_user(user_id)
Admin-->>API: User record + custom claims
API-->>U: Profile data response
U->>API: PUT /users/profile
API->>Admin: update_user() + set_custom_user_claims()
Admin-->>API: Update confirmation
API-->>U: Success response
U->>API: POST /users/bulk-action
API->>API: Check admin role
API->>Admin: update_user(disabled=True) / delete_user()
Admin-->>API: Operation results
API-->>U: Bulk action results
Testing GuideClick to expand
|
codeant-ai
bot
added
the
size:M
|
@refacto-visz |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds new user management functionality, including endpoints to get/update user profiles and perform bulk actions. The changes are a good step forward. My review focuses on improving correctness, security, and maintainability. I've pointed out a few critical bugs related to incorrect key usage that would cause runtime errors. I've also suggested using existing authorization dependencies for consistency, improving input validation with stricter types, handling exceptions more gracefully to avoid information leakage, and optimizing bulk operations for better performance. Addressing these points will make the new endpoints more robust and secure.
| async def get_user_profile(current_user: Dict[str, Any] = Depends(get_current_user)): | ||
| """Get detailed user profile information""" | ||
| try: | ||
| user_record = auth.get_user(current_user["id"]) |
There was a problem hiding this comment.
The current_user dictionary from the get_current_user dependency contains the user's ID under the key "uid", not "id". Using "id" will result in a KeyError at runtime. This seems to be a recurring issue in this file.
| user_record = auth.get_user(current_user["id"]) | |
| user_record = auth.get_user(current_user["uid"]) |
| ): | ||
| """Update current user's profile information""" | ||
| try: | ||
| user_record = auth.get_user(current_user["id"]) |
There was a problem hiding this comment.
The current_user dictionary from the get_current_user dependency contains the user's ID under the key "uid", not "id". Using "id" will result in a KeyError at runtime.
| user_record = auth.get_user(current_user["id"]) | |
| user_record = auth.get_user(current_user["uid"]) |
|
|
||
| class BulkUserRequest(BaseModel): | ||
| user_ids: List[str] | ||
| action: str |
There was a problem hiding this comment.
The action field is a plain string, which is prone to typos and doesn't clearly define the allowed operations. It's better to use typing.Literal to restrict the possible values to "disable" and "delete". This improves type safety and makes the API contract clearer for clients. You'll need to add Literal to your typing import at the top of the file (e.g., from typing import Dict, Any, List, Literal).
| action: str | |
| action: Literal["disable", "delete"] |
| "first_name": custom_claims["first_name"], | ||
| "last_name": custom_claims["last_name"], |
There was a problem hiding this comment.
Accessing custom_claims with bracket notation ([]) can raise a KeyError if the first_name or last_name claims are not set for a user. It's safer to use the .get() method with a default value to avoid unexpected crashes. This is also consistent with how claims are accessed in other parts of the codebase (e.g., firebase_auth.py).
| "first_name": custom_claims["first_name"], | |
| "last_name": custom_claims["last_name"], | |
| "first_name": custom_claims.get("first_name", ""), | |
| "last_name": custom_claims.get("last_name", ""), |
| if current_user.get("role") != "admin": | ||
| raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Admin access required") |
There was a problem hiding this comment.
Instead of manually checking for the 'admin' role, you should use the require_admin dependency from app.auth.dependencies. This centralizes authorization logic, making the code cleaner and more consistent with other protected endpoints in the application.
To fix this, you would:
- Import
require_admininapp/auth/user_management.py. - Change the
current_userdependency in the function signature toDepends(require_admin). - Remove this manual
ifcheck.
| except Exception as e: | ||
| raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=str(e)) |
There was a problem hiding this comment.
Catching a generic Exception is too broad and can obscure the actual error. For instance, if a user is not found, firebase_admin.auth.UserNotFoundError is raised, and a 404 Not Found response would be more appropriate than a 500 Internal Server Error. Additionally, exposing raw exception messages via str(e) can be a security risk by leaking internal implementation details. It's better to catch specific exceptions and return generic error messages to the user, while logging the detailed error for debugging purposes.
| except Exception as e: | ||
| raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) |
There was a problem hiding this comment.
Catching a generic Exception and always returning a 400 Bad Request may not be accurate for all failure scenarios. For example, a UserNotFoundError should result in a 404, and other Firebase errors might indicate a server-side issue (5xx). It's better to handle specific exceptions. Also, returning str(e) can leak sensitive information.
| if current_user.get("role") != "admin": | ||
| raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Admin access required") | ||
| results = [] | ||
| for user_id in request.user_ids: |
There was a problem hiding this comment.
For the 'delete' action, the current implementation iterates through user_ids and deletes users one by one. The Firebase Admin SDK provides a delete_users(uids) method that can delete up to 1000 users in a single API call, which is much more efficient. Consider refactoring to group user IDs for deletion and use this bulk method. Note that there is no bulk update for disabling users, so that part would correctly remain in a loop.
| if request.action == "disable": | ||
| auth.update_user(user_id, disabled=True) | ||
| results.append({"user_id": user_id, "status": "disabled"}) | ||
| elif request.action == "delete": | ||
| auth.delete_user(user_id) | ||
| results.append({"user_id": user_id, "status": "deleted"}) |
There was a problem hiding this comment.
The current logic doesn't handle cases where request.action is something other than "disable" or "delete". In such cases, the loop continues without performing any action or providing feedback for that user. It would be better to handle this case explicitly by adding an else block to append an error status to the results, informing the client about the invalid action.
| if request.action == "disable": | |
| auth.update_user(user_id, disabled=True) | |
| results.append({"user_id": user_id, "status": "disabled"}) | |
| elif request.action == "delete": | |
| auth.delete_user(user_id) | |
| results.append({"user_id": user_id, "status": "deleted"}) | |
| if request.action == "disable": | |
| auth.update_user(user_id, disabled=True) | |
| results.append({"user_id": user_id, "status": "disabled"}) | |
| elif request.action == "delete": | |
| auth.delete_user(user_id) | |
| results.append({"user_id": user_id, "status": "deleted"}) | |
| else: | |
| results.append({"user_id": user_id, "status": "error", "message": f"Invalid action: {request.action}"}) |
| except Exception as e: | ||
| results.append({"user_id": user_id, "status": "error", "message": str(e)}) |
There was a problem hiding this comment.
While it's good that the exception is caught inside the loop to allow other operations to continue, catching a generic Exception is too broad. It's better to catch specific exceptions from the Firebase Admin SDK (e.g., auth.UserNotFoundError). Also, exposing raw exception details with str(e) can be a security risk. Consider logging the full error and returning a more generic error message in the response.
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| custom_claims = auth.get_custom_user_claims(user_record.uid) | ||
| return { | ||
| "id": user_record.uid, | ||
| "email": user_record.email, | ||
| "first_name": custom_claims["first_name"], | ||
| "last_name": custom_claims["last_name"], |
There was a problem hiding this comment.
Suggestion: Directly indexing custom_claims["first_name"] and custom_claims["last_name"] without handling the case where get_custom_user_claims returns None or missing keys can lead to TypeError or KeyError when a user has no custom claims set. [null pointer]
Severity Level: Minor
| custom_claims = auth.get_custom_user_claims(user_record.uid) | |
| return { | |
| "id": user_record.uid, | |
| "email": user_record.email, | |
| "first_name": custom_claims["first_name"], | |
| "last_name": custom_claims["last_name"], | |
| custom_claims = auth.get_custom_user_claims(user_record.uid) or {} | |
| return { | |
| "id": user_record.uid, | |
| "email": user_record.email, | |
| "first_name": custom_claims.get("first_name", ""), | |
| "last_name": custom_claims.get("last_name", ""), |
Why it matters? ⭐
This is a legitimate runtime risk: auth.get_custom_user_claims(...) can return None or a dict missing keys, and indexing directly will raise TypeError/KeyError. The improved code (coalesce to {} and use .get with defaults) prevents these exceptions and returns sensible empty strings instead of failing the whole request. This addresses a real potential bug with minimal, safe change.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** app/auth/user_management.py
**Line:** 27:32
**Comment:**
*Null Pointer: Directly indexing `custom_claims["first_name"]` and `custom_claims["last_name"]` without handling the case where `get_custom_user_claims` returns `None` or missing keys can lead to TypeError or KeyError when a user has no custom claims set.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| """Perform bulk actions on multiple users (admin only)""" | ||
| if current_user.get("role") != "admin": | ||
| raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Admin access required") | ||
| results = [] |
There was a problem hiding this comment.
Suggestion: The bulk user action endpoint silently accepts any action value and returns a 200 response with an empty results list when the action is not "disable" or "delete", which is misleading and makes invalid input appear successful. [logic error]
Severity Level: Minor
| results = [] | |
| if request.action not in ("disable", "delete"): | |
| raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="Invalid action") |
Why it matters? ⭐
Currently if request.action is not "disable" or "delete" the endpoint simply returns an empty results list and 200 OK, which is misleading and lets invalid input appear to succeed. Validating the action and returning a 400 for unsupported actions is correct behavior and prevents silent no-ops. This change fixes an input validation bug and improves API correctness.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** app/auth/user_management.py
**Line:** 68:68
**Comment:**
*Logic Error: The bulk user action endpoint silently accepts any `action` value and returns a 200 response with an empty `results` list when the action is not `"disable"` or `"delete"`, which is misleading and makes invalid input appear successful.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| from fastapi.responses import JSONResponse | ||
| from .auth.routes import router as auth_router | ||
| from .example_protected_routes import router as protected_router | ||
| from .auth.user_management import router as user_management_router |
There was a problem hiding this comment.
Suggestion: The new import for the user management router will raise an ImportError at application startup because the .auth.user_management module does not exist in the codebase, causing the whole FastAPI app to fail to load; wrapping this import in a try/except with a safe fallback router prevents the crash while still using the router if/when the module is added. [possible bug]
Severity Level: Critical 🚨
| from .auth.user_management import router as user_management_router | |
| try: | |
| from .auth.user_management import router as user_management_router | |
| except ImportError: | |
| from fastapi import APIRouter | |
| user_management_router = APIRouter() |
Why it matters? ⭐
The suggestion correctly identifies a real startup failure: the PR added "from .auth.user_management import router as user_management_router"
but there is no app/auth/user_management.py in the current tree. I verified that the only reference to "user_management" is in app/main.py and no module file exists, so the direct import will raise ImportError when the app is started. Wrapping the import in a try/except and providing a safe APIRouter fallback prevents the whole FastAPI app from crashing while the optional module is absent. This is a pragmatic, low-risk mitigation that fixes an actual runtime problem introduced by the PR.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** app/main.py
**Line:** 6:6
**Comment:**
*Possible Bug: The new import for the user management router will raise an ImportError at application startup because the `.auth.user_management` module does not exist in the codebase, causing the whole FastAPI app to fail to load; wrapping this import in a try/except with a safe fallback router prevents the crash while still using the router if/when the module is added.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
Code Review: User Management Authentication SystemPR Confidence Score: 🟥 1 / 5👍 Well Done
📁 Selected files for review (2)
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/in/1889637?s=60&v=4){kind=small}
| "first_name": custom_claims["first_name"], | ||
| "last_name": custom_claims["last_name"], |
There was a problem hiding this comment.
Unsafe Custom Claim Access
Direct dictionary access on custom_claims will raise a KeyError if a user profile lacks these specific claims. This causes an unhandled exception and a 500 error, making the profile endpoint unreliable for certain users.
"first_name": custom_claims.get("first_name", ""),
"last_name": custom_claims.get("last_name", ""),
Commitable Suggestion
| "first_name": custom_claims["first_name"], | |
| "last_name": custom_claims["last_name"], | |
| "first_name": custom_claims.get("first_name", ""), | |
| "last_name": custom_claims.get("last_name", ""), |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Correctness
| async def get_user_profile(current_user: Dict[str, Any] = Depends(get_current_user)): | ||
| """Get detailed user profile information""" | ||
| try: | ||
| user_record = auth.get_user(current_user["id"]) |
There was a problem hiding this comment.
Blocking I/O in Async Endpoints
Synchronous functions from the firebase-admin SDK, like auth.get_user, perform blocking network I/O. Calling them directly within an async function blocks the server's event loop, preventing it from handling other requests concurrently.
user_record = await asyncio.get_event_loop().run_in_executor(None, auth.get_user, current_user["id"])
custom_claims = await asyncio.get_event_loop().run_in_executor(None, auth.get_custom_user_claims, user_record.uid)
Commitable Suggestion
| user_record = auth.get_user(current_user["id"]) | |
| user_record = await asyncio.get_event_loop().run_in_executor(None, auth.get_user, current_user["id"]) | |
| custom_claims = await asyncio.get_event_loop().run_in_executor(None, auth.get_custom_user_claims, user_record.uid) |
Standards
- ISO-IEC-25010-Performance-Efficiency-Time-Behavior
| elif request.action == "delete": | ||
| auth.delete_user(user_id) | ||
| results.append({"user_id": user_id, "status": "deleted"}) |
There was a problem hiding this comment.
Inefficient Bulk Deletion
The code iterates through user IDs to delete them one by one, causing an N+1 query problem where N is the number of users. This results in N separate network requests, which is inefficient and scales poorly.
elif request.action == "delete":
delete_result = auth.delete_users(request.user_ids)
for uid in delete_result.success_count:
results.append({"user_id": uid, "status": "deleted"})
for error in delete_result.errors:
results.append({"user_id": error.uid, "status": "error", "message": error.reason})
break
Commitable Suggestion
| elif request.action == "delete": | |
| auth.delete_user(user_id) | |
| results.append({"user_id": user_id, "status": "deleted"}) | |
| elif request.action == "delete": | |
| delete_result = auth.delete_users(request.user_ids) | |
| for uid in delete_result.success_count: | |
| results.append({"user_id": uid, "status": "deleted"}) | |
| for error in delete_result.errors: | |
| results.append({"user_id": error.uid, "status": "error", "message": error.reason}) | |
| break |
Standards
- ISO-IEC-25010-Performance-Efficiency-Time-Behavior
- Optimization-Pattern-Batch-Processing
| if current_user.get("role") != "admin": | ||
| raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Admin access required") |
There was a problem hiding this comment.
Inconsistent Admin Authorization
Manual role checking bypasses centralized authorization controls and creates inconsistent security patterns. The require_admin dependency is imported but not used, leading to potential authorization bypass if role validation logic differs.
async def bulk_user_action(
request: BulkUserRequest,
current_user: Dict[str, Any] = Depends(require_admin)
):
Commitable Suggestion
| if current_user.get("role") != "admin": | |
| raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Admin access required") | |
| async def bulk_user_action( | |
| request: BulkUserRequest, | |
| current_user: Dict[str, Any] = Depends(require_admin) | |
| ): |
Standards
- CWE-862
- OWASP-A01
- NIST-SSDF-PW.1
|
|
||
| class BulkUserRequest(BaseModel): | ||
| user_ids: List[str] | ||
| action: str |
There was a problem hiding this comment.
Missing Action Validation
The action field in BulkUserRequest is a generic string, but the endpoint logic only handles 'disable' and 'delete'. Providing an invalid action results in a misleading 200 OK response with no work done.
Standards
- Business-Rule-Input-Validation
- Logic-Verification-Data-Integrity
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/auth/user_management.py (2)
22-37: Avoid blocking the event loop with synchronous Firebase Admin calls inasyncroutesAll three handlers are declared
asyncbut callfirebase_admin.authfunctions (get_user,set_custom_user_claims,update_user,delete_user) which are synchronous and can block the event loop under load.Consider either:
- Making these path operations regular
deffunctions so FastAPI runs them in a threadpool, or- Wrapping the Firebase calls in
fastapi.concurrency.run_in_threadpool.This keeps other requests responsive while Firebase operations are in flight.
Also applies to: 40-57, 60-79
40-57: Avoid clobbering unrelated custom claims and mapping all failures to 400
update_user_profilecurrently replaces the entire custom claims dict with onlyfirst_name,last_name, androle. Any other existing claims (e.g., flags, tenant IDs, feature toggles) would be silently dropped. In addition, all exceptions are surfaced to the client as a 400 with the raw error message, even when they’re clearly server‑side issues.A safer pattern is to merge with existing claims and treat unexpected failures as 500s with a generic message:
"""Update current user's profile information""" try: - user_record = auth.get_user(current_user["id"]) - if update_data.email != user_record.email: - auth.update_user(user_record.uid, email=update_data.email) - auth.set_custom_user_claims(user_record.uid, { - "first_name": update_data.first_name, - "last_name": update_data.last_name, - "role": current_user.get("role", "user") - }) - return {"message": "Profile updated successfully"} - except Exception as e: - raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) + user_record = auth.get_user(current_user["id"]) + if update_data.email != user_record.email: + auth.update_user(user_record.uid, email=update_data.email) + + existing_claims = auth.get_custom_user_claims(user_record.uid) or {} + updated_claims = { + **existing_claims, + "first_name": update_data.first_name, + "last_name": update_data.last_name, + "role": current_user.get("role", "user"), + } + auth.set_custom_user_claims(user_record.uid, updated_claims) + return {"message": "Profile updated successfully"} + except Exception as e: + # Optionally log `e` here for diagnostics + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Failed to update profile", + ) from eThis preserves other claims and better reflects error semantics to clients.
app/main.py (1)
6-6: Keep the root endpoint’s advertised endpoints in sync with included routersYou now include
user_management_routerunder/users, but the root JSON still only listsauthandprotected. For better discoverability, consider adding the new route group:return { "message": "Welcome to Authentication API", "docs": "/docs", "redoc": "/redoc", "endpoints": { - "auth": "/auth", - "protected": "/protected" + "auth": "/auth", + "protected": "/protected", + "users": "/users", } }Also applies to: 31-32, 48-57
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/auth/user_management.py(1 hunks)app/main.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/auth/user_management.py (1)
app/auth/dependencies.py (1)
get_current_user(10-32)
🪛 Ruff (0.14.8)
app/auth/user_management.py
23-23: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
36-36: Do not catch blind exception: Exception
(BLE001)
37-37: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
43-43: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
55-55: Consider moving this statement to an else block
(TRY300)
56-56: Do not catch blind exception: Exception
(BLE001)
57-57: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
63-63: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
77-77: Do not catch blind exception: Exception
(BLE001)
| from fastapi import APIRouter, Depends, HTTPException, status | ||
| from .firebase_auth import firebase_auth | ||
| from .dependencies import get_current_user, require_admin | ||
| from typing import Dict, Any, List |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l app/auth/user_management.pyRepository: coderabbit-test/pr-reviewer
Length of output: 101
🏁 Script executed:
cat -n app/auth/user_management.py | head -100Repository: coderabbit-test/pr-reviewer
Length of output: 3575
Add Literal type constraint and explicit fallback for bulk operation actions
BulkUserRequest.action is currently a plain str, but bulk_user_action only handles "disable" and "delete". Any other value silently produces an empty or partial results array, which masks client bugs.
Strengthen the type safety and add explicit handling by:
- Import
Literalfromtyping(line 4) - Constrain
BulkUserRequest.actiontoLiteral["disable", "delete"](line 19) - Add an explicit
elseclause in the action handling loop to return a "skipped" status with an explanatory message for any unexpected action values
Also, switch the admin check from manual role verification (if current_user.get("role") != "admin") to use require_admin dependency for consistency with the rest of the auth layer (line 63).
🤖 Prompt for AI Agents
In app/auth/user_management.py around lines 4, 19 and 63, strengthen action
typing and handling: import Literal on line 4, change the BulkUserRequest.action
type (around line 19) to Literal["disable","delete"], and in the bulk action
loop add an explicit else branch that appends a "skipped" result with an
explanatory message for any unexpected action value; also replace the manual
admin role check at line 63 (if current_user.get("role") != "admin") with the
existing require_admin dependency for consistency and update imports
accordingly.
| @router.get("/profile") | ||
| async def get_user_profile(current_user: Dict[str, Any] = Depends(get_current_user)): | ||
| """Get detailed user profile information""" | ||
| try: | ||
| user_record = auth.get_user(current_user["id"]) | ||
| custom_claims = auth.get_custom_user_claims(user_record.uid) | ||
| return { | ||
| "id": user_record.uid, | ||
| "email": user_record.email, | ||
| "first_name": custom_claims["first_name"], | ||
| "last_name": custom_claims["last_name"], | ||
| "is_active": not user_record.disabled, | ||
| "created_at": str(user_record.user_metadata.creation_timestamp) | ||
| } | ||
| except Exception as e: | ||
| raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=str(e)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the file to verify the code at lines 22-37
find . -name "user_management.py" -type f | head -20Repository: coderabbit-test/pr-reviewer
Length of output: 100
🏁 Script executed:
# Read the entire file to understand context and verify lines 22-37
cat -n ./app/auth/user_management.pyRepository: coderabbit-test/pr-reviewer
Length of output: 3575
🏁 Script executed:
# Find and read the dependencies file
find . -name "dependencies.py" -type f | grep -E "auth|dependencies"Repository: coderabbit-test/pr-reviewer
Length of output: 97
🏁 Script executed:
cat -n ./app/auth/dependencies.pyRepository: coderabbit-test/pr-reviewer
Length of output: 2627
🏁 Script executed:
# Find and read firebase_auth.py
find . -name "firebase_auth.py" -type fRepository: coderabbit-test/pr-reviewer
Length of output: 98
🏁 Script executed:
cat -n ./app/auth/firebase_auth.pyRepository: coderabbit-test/pr-reviewer
Length of output: 7451
🏁 Script executed:
# Search for where "id" key might be set in current_user or user_data
rg -n "current_user\[.id.\]|\"id\":" ./app/auth/ -A 2 -B 2Repository: coderabbit-test/pr-reviewer
Length of output: 2074
Fix critical bug: use current_user["uid"] instead of current_user["id"], and harden against missing custom claims
The endpoint has three issues:
- Critical bug:
current_user["id"]does not exist. Theverify_token()function returnscurrent_user["uid"], so line 26 will raiseKeyErrorat runtime. Usecurrent_user["uid"]instead. auth.get_custom_user_claims()can returnNone, and indexing withcustom_claims["first_name"]/["last_name"]will raiseKeyErrorfor users who haven't set profile data. Use.get()with defaults.- The
except Exceptionblock leaks internal error details to clients viastr(e), which can expose Firebase/Admin SDK internals.
Use this fix:
- user_record = auth.get_user(current_user["id"])
+ user_record = auth.get_user(current_user["uid"])
custom_claims = auth.get_custom_user_claims(user_record.uid) or {}
return {
"id": user_record.uid,
"email": user_record.email,
- "first_name": custom_claims["first_name"],
- "last_name": custom_claims["last_name"],
+ "first_name": custom_claims.get("first_name"),
+ "last_name": custom_claims.get("last_name"),
"is_active": not user_record.disabled,
"created_at": str(user_record.user_metadata.creation_timestamp)
}
except Exception as e:
- raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=str(e))
+ raise HTTPException(
+ status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+ detail="Failed to fetch user profile",
+ ) from eAlso apply the same current_user["uid"] fix to line 47 in the update_user_profile function.
📝 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.
| @router.get("/profile") | |
| async def get_user_profile(current_user: Dict[str, Any] = Depends(get_current_user)): | |
| """Get detailed user profile information""" | |
| try: | |
| user_record = auth.get_user(current_user["id"]) | |
| custom_claims = auth.get_custom_user_claims(user_record.uid) | |
| return { | |
| "id": user_record.uid, | |
| "email": user_record.email, | |
| "first_name": custom_claims["first_name"], | |
| "last_name": custom_claims["last_name"], | |
| "is_active": not user_record.disabled, | |
| "created_at": str(user_record.user_metadata.creation_timestamp) | |
| } | |
| except Exception as e: | |
| raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=str(e)) | |
| @router.get("/profile") | |
| async def get_user_profile(current_user: Dict[str, Any] = Depends(get_current_user)): | |
| """Get detailed user profile information""" | |
| try: | |
| user_record = auth.get_user(current_user["uid"]) | |
| custom_claims = auth.get_custom_user_claims(user_record.uid) or {} | |
| return { | |
| "id": user_record.uid, | |
| "email": user_record.email, | |
| "first_name": custom_claims.get("first_name"), | |
| "last_name": custom_claims.get("last_name"), | |
| "is_active": not user_record.disabled, | |
| "created_at": str(user_record.user_metadata.creation_timestamp) | |
| } | |
| except Exception as e: | |
| raise HTTPException( | |
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| detail="Failed to fetch user profile", | |
| ) from e |
🧰 Tools
🪛 Ruff (0.14.8)
23-23: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
36-36: Do not catch blind exception: Exception
(BLE001)
37-37: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In app/auth/user_management.py around lines 22 to 37, the code incorrectly uses
current_user["id"] (should be current_user["uid"]), assumes
auth.get_custom_user_claims() always returns a dict, and returns internal
exception text to clients; change the lookup to current_user["uid"], read
custom_claims using .get("first_name", "") and .get("last_name", "") (or
suitable defaults) to avoid KeyError when claims are missing, and replace
raising HTTPException(status=500, detail=str(e)) with a generic error message
(e.g., "Internal server error") while logging the original exception internally;
also apply the same current_user["uid"] fix at line 47 in update_user_profile.
CodeAnt-AI Description
Add user management endpoints for viewing/updating profiles and performing admin bulk actions
What Changed
Impact
✅ Can view detailed user profile via API✅ Users can update their email and display names✅ Admins can disable or delete multiple users in bulk💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.