-
Notifications
You must be signed in to change notification settings - Fork 4
Implement user management endpoints in FastAPI #64
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: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,79 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from pydantic import BaseModel, EmailStr | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from firebase_admin import auth | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router = APIRouter(prefix="/users", tags=["user-management"]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class UserUpdateRequest(BaseModel): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| email: EmailStr | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| first_name: str | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| last_name: str | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class BulkUserRequest(BaseModel): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| user_ids: List[str] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| action: str | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Comment on lines
+17
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validate the action field to prevent unintended operations. The 🔎 Proposed fix using Literal for action validation+from typing import Dict, Any, List, Literal
-from typing import Dict, Any, List
class BulkUserRequest(BaseModel):
user_ids: List[str]
- action: str
+ action: Literal["disable", "delete"]📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @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"]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The current user dict coming from the authentication dependency exposes a Severity Level: Minor
Suggested change
Why it matters? ⭐The dependency get_current_user returns the dict produced by firebase_auth.verify_token, which contains "uid" (see app/auth/firebase_auth.py: lines returning {"uid": ...}) and does not provide an "id" key. Indexing current_user["id"] will raise a KeyError at runtime. Changing to current_user["uid"] fixes a real runtime bug. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** app/auth/user_management.py
**Line:** 26:26
**Comment:**
*Logic Error: The current user dict coming from the authentication dependency exposes a `uid` field (not `id`), so indexing `current_user["id"]` will raise a KeyError at runtime and prevent the profile from being fetched; use `current_user["uid"]` instead to look up the Firebase user.
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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+31
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Accessing custom claims directly with
Suggested change
Comment on lines
+31
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing Custom Claims Null SafetyDirect dictionary access on custom_claims without null checking causes KeyError when claims are empty or missing. Service crashes with 500 error when users lack first_name/last_name claims, making authentication service unreliable for users without complete profile data. Commitable Suggestion
Suggested change
Standards
Context References
Comment on lines
+27
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Severity Level: Critical 🚨
Suggested change
Why it matters? ⭐firebase_admin.auth.get_custom_user_claims can return None or omit keys. The current code indexes custom_claims["first_name"] / ["last_name"] directly, which will raise a KeyError (or TypeError if custom_claims is None). Normalizing to {} and using .get(..., "") prevents runtime errors for users without those claims. This is a defensive fix that prevents 500s for legitimate users. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** app/auth/user_management.py
**Line:** 27:32
**Comment:**
*Possible Bug: `auth.get_custom_user_claims` can return `None` or omit `first_name`/`last_name` for some users, and indexing like `custom_claims["first_name"]` will raise a KeyError; normalizing to an empty dict and using `.get(..., "")` avoids runtime failures for users without these claims.
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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "is_active": not user_record.disabled, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "created_at": str(user_record.user_metadata.creation_timestamp) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+25
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix incorrect Firebase Admin SDK usage and handle missing custom claims. Two critical issues:
🔎 Proposed fix try:
user_record = auth.get_user(current_user["id"])
- custom_claims = auth.get_custom_user_claims(user_record.uid)
+ custom_claims = user_record.custom_claims 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)
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=str(e)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+36
to
+37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Catching a generic
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @router.put("/profile") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def update_user_profile( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| update_data: UserUpdateRequest, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| current_user: Dict[str, Any] = Depends(get_current_user) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Update current user's profile information""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| user_record = auth.get_user(current_user["id"]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: As with the profile GET handler, the current user data produced by the auth dependency contains a Severity Level: Minor
Suggested change
Why it matters? ⭐Same root cause as suggestion 1: get_current_user returns a dict with "uid" (from verify_token) not "id". The update_user_profile function uses current_user["id"] and will raise KeyError. Switching to current_user["uid"] resolves a real logic/runtime error. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** app/auth/user_management.py
**Line:** 47:47
**Comment:**
*Logic Error: As with the profile GET handler, the current user data produced by the auth dependency contains a `uid` field rather than `id`, so using `current_user["id"]` here will raise a KeyError and break profile updates; it should reference `current_user["uid"]` instead.
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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+50
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Custom Claims Data LossComplete custom claims replacement overwrites existing claims beyond first_name, last_name, and role. User loses additional custom claims causing feature degradation. Authorization and personalization data permanently lost during profile updates. Standards
Context References
Comment on lines
+46
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Risk of overwriting existing custom claims. The 🔎 Proposed fix to preserve existing claims 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)
+
+ # Preserve existing custom claims
+ existing_claims = user_record.custom_claims 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, {
- "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"}🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return {"message": "Profile updated successfully"} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+56
to
+57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The exception handling is too broad. Catching specific exceptions like
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @router.post("/bulk-action") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def bulk_user_action( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request: BulkUserRequest, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| current_user: Dict[str, Any] = Depends(get_current_user) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """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") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+61
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This endpoint performs a manual authorization check for the admin role. It's better to use the
Suggested change
Comment on lines
+60
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Authorization Race ConditionAuthorization check occurs after function execution begins rather than using dependency injection. This creates a race condition where unauthorized users can trigger function execution before authorization validation, potentially allowing timing attacks or resource consumption before being blocked. Commitable Suggestion
Suggested change
Standards
Context References
Comment on lines
+60
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Use the The 🔎 Proposed refactor @router.post("/bulk-action")
async def bulk_user_action(
request: BulkUserRequest,
- current_user: Dict[str, Any] = Depends(get_current_user)
+ current_user: Dict[str, Any] = Depends(require_admin)
):
"""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 = []🧰 Tools🪛 Ruff (0.14.10)63-63: Do not perform function call (B008) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| results = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The bulk action endpoint silently accepts any Severity Level: Minor
Suggested change
Why it matters? ⭐As written, unsupported actions are silently ignored and callers get an empty/partial results list which is misleading. Validating request.action early and returning HTTP 400 for unsupported actions is a correctness improvement and prevents clients from thinking the call succeeded when it did nothing. 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 action endpoint silently accepts any `action` string and just returns an empty `results` list when the action is unsupported, which is misleading and hides misuse of the API; it should explicitly reject invalid actions with a 400 error before processing.
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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for user_id in request.user_ids: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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"}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+71
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing Action Type ValidationBulk action logic only handles 'disable' and 'delete' actions but lacks validation for invalid action types. Unrecognized actions silently skip processing without error indication, creating incomplete business rule enforcement. Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| results.append({"user_id": user_id, "status": "error", "message": str(e)}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+77
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The exception handling here is too broad. Catching specific exceptions like
Suggested change
Comment on lines
+69
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sequential User Processing BottleneckSequential Firebase auth operations in loop create O(n) blocking I/O pattern. Each user operation waits for Firebase API response before processing next user. Bulk operations with hundreds of users will cause significant response delays and poor scalability. Commitable Suggestion
Suggested change
Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return {"results": results} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+68
to
+79
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add safeguards for bulk operations. Several concerns with the current implementation:
🔎 Proposed improvementsAdd a limit check at the beginning: # Add after the docstring
MAX_BULK_SIZE = 100
if len(request.user_ids) > MAX_BULK_SIZE:
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail=f"Cannot process more than {MAX_BULK_SIZE} users at once"
)Consider more specific exception handling in the loop: try:
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"})
+ except auth.UserNotFoundError as e:
+ results.append({"user_id": user_id, "status": "error", "message": "User not found"})
except Exception as e:
results.append({"user_id": user_id, "status": "error", "message": str(e)})For the deletion operation, consider implementing an audit log or requiring additional confirmation.
🧰 Tools🪛 Ruff (0.14.10)77-77: Do not catch blind exception: (BLE001) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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
firebase_authimport is unused in this file and can be removed to improve code cleanliness.