ccc #50
ccc #50
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 module Changes
Sequence DiagramsequenceDiagram
participant Client
participant FastAPI as API Endpoint
participant UserManager
participant Firebase
participant JWT as JWT Handler
rect rgb(200, 220, 240)
Note over Client,JWT: Token Refresh Flow
Client->>FastAPI: POST /refresh-token (refresh_token)
FastAPI->>UserManager: refresh_user_token(refresh_token)
UserManager->>JWT: Decode refresh_token (skip exp)
JWT-->>UserManager: Extract claims
UserManager->>JWT: Create new access_token (10 min expiry)
JWT-->>UserManager: New token
UserManager-->>FastAPI: access_token
FastAPI-->>Client: TokenResponse {access_token}
end
rect rgb(220, 240, 200)
Note over Client,JWT: Token Validation Flow
Client->>FastAPI: GET /validate (token)
FastAPI->>UserManager: verify_user_token(token)
UserManager->>JWT: Decode token
JWT-->>UserManager: user_id, email, first_name, last_name, role
UserManager-->>FastAPI: User data dict
FastAPI-->>Client: User data (or 401 if invalid)
end
rect rgb(240, 220, 200)
Note over Client,Firebase: Profile Update Flow
Client->>FastAPI: PUT /profile (first_name, last_name)
FastAPI->>UserManager: update_user_profile(user_id, first_name, last_name)
UserManager->>Firebase: Fetch current user
Firebase-->>UserManager: Current user data
UserManager->>Firebase: Update display_name
Firebase-->>UserManager: Confirmation
UserManager-->>FastAPI: {id, first_name, last_name, updated_at}
FastAPI-->>Client: UserResponse
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 introduces a dedicated 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-visz |
Refacto PR SummaryAdded comprehensive user management system with JWT-based authentication, profile updates, and token refresh capabilities. The implementation provides custom JWT token handling alongside Firebase authentication integration, enabling flexible user session management and profile customization. Key Changes:
Change HighlightsClick to expand
Sequence DiagramsequenceDiagram
participant U as User
participant API as FastAPI
participant UM as UserManager
participant FB as Firebase Admin
participant JWT as JWT Service
U->>API: PUT /users/profile
API->>UM: update_user_profile()
UM->>FB: update_user(display_name)
UM->>FB: get_user(custom_claims)
FB-->>UM: Updated user record
UM-->>API: Profile data
API-->>U: UserResponse
U->>API: POST /users/refresh-token
API->>UM: refresh_user_token()
UM->>JWT: decode(verify_exp=False)
UM->>JWT: encode(new_payload)
JWT-->>UM: New access token
UM-->>API: Fresh token
API-->>U: TokenResponse
Testing GuideClick to expand
|
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
codeant-ai
bot
added
the
size:L
![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 introduces a new user_management.py module with several endpoints for profile updates and token management. While it adds new functionality, I've identified several critical security vulnerabilities and correctness issues that must be addressed. These include using a hardcoded default JWT secret, disabling token expiration checks, and incorrect handling of Firebase custom claims. There are also some medium-severity issues related to logging and code structure. Please review the detailed comments below.
|
|
||
| class UserManager: | ||
| def __init__(self): | ||
| self.jwt_secret = os.getenv("JWT_SECRET", "default-secret-key") |
There was a problem hiding this comment.
Using a default hardcoded secret key default-secret-key is a major security risk. If the JWT_SECRET environment variable is not set, the application will fall back to this known, insecure key, making your JWTs easy to forge. It's better to fail fast if the secret is not configured. This change will cause the application to raise a KeyError on startup if JWT_SECRET is not set, which is the desired behavior for critical configuration.
| self.jwt_secret = os.getenv("JWT_SECRET", "default-secret-key") | |
| self.jwt_secret = os.environ["JWT_SECRET"] |
| payload = jwt.decode( | ||
| refresh_token, | ||
| self.jwt_secret, | ||
| algorithms=[self.jwt_algorithm], | ||
| options={"verify_exp": False} | ||
| ) |
There was a problem hiding this comment.
A critical security vulnerability exists here. By setting options={"verify_exp": False}, you are decoding the JWT without checking if it has expired. This allows an attacker to use an old, expired refresh token to generate new access tokens indefinitely. The expiration check must be enforced by removing this option.
payload = jwt.decode(
refresh_token,
self.jwt_secret,
algorithms=[self.jwt_algorithm]
)| current_claims["first_name"] = first_name | ||
| current_claims["last_name"] = last_name |
There was a problem hiding this comment.
The custom claims for first_name and last_name are being updated on a local dictionary current_claims, but they are never persisted back to Firebase. You need to call auth.set_custom_user_claims(user_id, current_claims) to save the changes.
| current_claims["first_name"] = first_name | |
| current_claims["last_name"] = last_name | |
| current_claims["first_name"] = first_name | |
| current_claims["last_name"] = last_name | |
| auth.set_custom_user_claims(user_id, current_claims) |
| async def verify_user_token(self, token: str) -> Optional[Dict[str, Any]]: | ||
| """Verify JWT token - ISSUE: Using JWT instead of Firebase""" | ||
| try: | ||
| payload = jwt.decode( | ||
| token, | ||
| self.jwt_secret, | ||
| algorithms=[self.jwt_algorithm] | ||
| ) | ||
|
|
||
| return { | ||
| "uid": payload.get("user_id"), | ||
| "email": payload.get("email"), | ||
| "first_name": payload.get("first_name", ""), | ||
| "last_name": payload.get("last_name", ""), | ||
| "role": payload.get("role", "user") | ||
| } | ||
| except Exception as e: | ||
| print(f"Token verification failed: {e}") | ||
| return None |
There was a problem hiding this comment.
This method implements custom JWT verification, which is inconsistent with the rest of the application that seems to rely on Firebase for authentication (e.g., get_current_user uses firebase_auth.verify_token). The docstring itself flags this as an issue. Bypassing Firebase's verify_id_token means you lose important security features like checking for token revocation and ensuring the token was issued by Firebase. This can lead to security vulnerabilities where a user who should no longer have access can still use a valid-looking token. It's strongly recommended to use Firebase's token verification consistently across the application.
| ) -> Dict[str, Any]: | ||
| """Update user profile information""" | ||
| try: | ||
| from firebase_admin import auth |
There was a problem hiding this comment.
Imports should generally be at the top of the file. This local import of firebase_admin.auth can hide module dependencies and make the code harder to read and maintain. Please move from firebase_admin import auth to the top of the file with the other imports.
| except Exception as e: | ||
| print(f"Token refresh failed: {e}") | ||
| return None |
There was a problem hiding this comment.
Using print() for logging errors is not recommended in a production application as it writes to standard output and lacks context like log levels or timestamps. It's better to use Python's built-in logging module. Using logging.exception() is particularly useful inside an except block as it will also record the stack trace. Please add import logging at the top of the file.
except Exception:
import logging
logging.exception("Token refresh failed")
return None| except Exception as e: | ||
| print(f"Token verification failed: {e}") | ||
| return None |
There was a problem hiding this comment.
Using print() for logging errors is not recommended for production code. Please use the logging module to provide more structured and configurable logging. Using logging.exception() inside an except block is a good practice to capture the full stack trace. Remember to import logging at the top of the file.
except Exception:
import logging
logging.exception("Token verification failed")
return None
Nitpicks 🔍
|
Code Review: User Management Authentication SystemPR Confidence Score: 🟥 0 / 5👍 Well Done
📁 Selected files for review (1)
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/in/1889637?s=60&v=4){kind=small}
| payload = jwt.decode( | ||
| refresh_token, | ||
| self.jwt_secret, | ||
| algorithms=[self.jwt_algorithm], | ||
| options={"verify_exp": False} | ||
| ) |
There was a problem hiding this comment.
JWT Validation Bypass
Token refresh function disables expiration verification allowing expired tokens to be refreshed indefinitely. Attackers can reuse compromised refresh tokens beyond their intended lifetime, bypassing token expiration security controls and enabling persistent unauthorized access.
payload = jwt.decode(
refresh_token,
self.jwt_secret,
algorithms=[self.jwt_algorithm]
)
Commitable Suggestion
| payload = jwt.decode( | |
| refresh_token, | |
| self.jwt_secret, | |
| algorithms=[self.jwt_algorithm], | |
| options={"verify_exp": False} | |
| ) | |
| payload = jwt.decode( | |
| refresh_token, | |
| self.jwt_secret, | |
| algorithms=[self.jwt_algorithm] | |
| ) |
Standards
- CWE-287
- OWASP-A02
- NIST-SSDF-PW.1
| payload = jwt.decode( | ||
| token, | ||
| self.jwt_secret, | ||
| algorithms=[self.jwt_algorithm] | ||
| ) |
There was a problem hiding this comment.
Firebase Auth Bypass
Custom JWT verification bypasses Firebase authentication system using local secret instead of Firebase token validation. Attackers can forge tokens with knowledge of JWT secret, circumventing Firebase security controls and creating a parallel authentication system vulnerable to token forgery attacks.
from firebase_admin import auth
decoded_token = auth.verify_id_token(token)
return {
"uid": decoded_token.get("uid"),
"email": decoded_token.get("email"),
"first_name": decoded_token.get("first_name", ""),
"last_name": decoded_token.get("last_name", ""),
"role": decoded_token.get("role", "user")
}
Commitable Suggestion
| payload = jwt.decode( | |
| token, | |
| self.jwt_secret, | |
| algorithms=[self.jwt_algorithm] | |
| ) | |
| from firebase_admin import auth | |
| decoded_token = auth.verify_id_token(token) | |
| return { | |
| "uid": decoded_token.get("uid"), | |
| "email": decoded_token.get("email"), | |
| "first_name": decoded_token.get("first_name", ""), | |
| "last_name": decoded_token.get("last_name", ""), | |
| "role": decoded_token.get("role", "user") | |
| } |
Standards
- CWE-290
- OWASP-A02
- NIST-SSDF-PW.1
|
|
||
| class UserManager: | ||
| def __init__(self): | ||
| self.jwt_secret = os.getenv("JWT_SECRET", "default-secret-key") |
There was a problem hiding this comment.
Hardcoded JWT Secret
JWT secret falls back to hardcoded default value when environment variable missing. Attackers knowing this default can forge tokens in misconfigured environments, making production deployments without proper JWT_SECRET configuration vulnerable to authentication bypass through token forgery.
jwt_secret = os.getenv("JWT_SECRET")
if not jwt_secret:
raise ValueError("JWT_SECRET environment variable is required")
self.jwt_secret = jwt_secret
Commitable Suggestion
| self.jwt_secret = os.getenv("JWT_SECRET", "default-secret-key") | |
| jwt_secret = os.getenv("JWT_SECRET") | |
| if not jwt_secret: | |
| raise ValueError("JWT_SECRET environment variable is required") | |
| self.jwt_secret = jwt_secret |
Standards
- CWE-798
- OWASP-A02
- NIST-SSDF-PW.1
| current_claims = user_record.custom_claims or {} | ||
| current_claims["first_name"] = first_name | ||
| current_claims["last_name"] = last_name |
There was a problem hiding this comment.
Profile Claims Not Saved
The code modifies a local dictionary for custom claims but never calls auth.set_custom_user_claims() to persist these changes to Firebase. This results in a silent failure where user profile data is not stored in the token claims, leading to incorrect application behavior and data state.
current_claims = user_record.custom_claims or {}
current_claims["first_name"] = first_name
current_claims["last_name"] = last_name
# Persist custom claims to Firebase
auth.set_custom_user_claims(user_id, current_claims)
Commitable Suggestion
| current_claims = user_record.custom_claims or {} | |
| current_claims["first_name"] = first_name | |
| current_claims["last_name"] = last_name | |
| current_claims = user_record.custom_claims or {} | |
| current_claims["first_name"] = first_name | |
| current_claims["last_name"] = last_name | |
| # Persist custom claims to Firebase | |
| auth.set_custom_user_claims(user_id, current_claims) |
Standards
- ISO-IEC-25010-Functional-Correctness-Correctness
- DbC-Postconditions
| first_name=result["first_name"], | ||
| last_name=result["last_name"], | ||
| is_active=True, | ||
| created_at=result["updated_at"] |
There was a problem hiding this comment.
Incorrect Timestamp Field
The created_at field in the response object is populated with the updated_at value. This is semantically incorrect and can lead to data inconsistencies or bugs in client applications that rely on this field to represent the user's creation time.
Standards
- ISO-IEC-25010-Functional-Correctness-Correctness
| user_record = auth.get_user(user_id) | ||
| current_claims = user_record.custom_claims or {} | ||
| current_claims["first_name"] = first_name | ||
| current_claims["last_name"] = last_name |
There was a problem hiding this comment.
Missing Custom Claims Update
Profile update modifies custom claims dictionary locally but never persists changes to Firebase using set_custom_user_claims. User profile updates appear successful but claims remain unchanged in Firebase authentication system, causing data inconsistency between displayed profile and actual authentication claims.
Standards
- CWE-20
- OWASP-A04
- NIST-SSDF-PW.1
| new_payload = { | ||
| "user_id": user_id, | ||
| "email": email, | ||
| "exp": datetime.utcnow() + timedelta(minutes=10), |
There was a problem hiding this comment.
Short Token Expiry
Access tokens expire in only 10 minutes causing frequent re-authentication requests that may impact user experience. Short expiry combined with insecure refresh mechanism creates usability issues while maintaining security vulnerabilities.
Standards
- CWE-613
- OWASP-A02
- NIST-SSDF-PW.1
| except Exception as e: | ||
| print(f"Token refresh failed: {e}") | ||
| return None |
There was a problem hiding this comment.
Improper Exception Handling
Methods like refresh_user_token and verify_user_token catch generic exceptions, print them to the console, and return None. This practice hides the error's context from the calling API layer and complicates error handling logic. It is better to raise specific application-level exceptions or let them propagate to a centralized handler.
Standards
- Clean-Code-Error-Handling
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| current_claims = user_record.custom_claims or {} | ||
| current_claims["first_name"] = first_name | ||
| current_claims["last_name"] = last_name | ||
|
|
There was a problem hiding this comment.
Suggestion: In update_user_profile, the custom claims dictionary is modified in memory but never written back to Firebase, so the updated first_name and last_name will not actually be reflected in subsequent token-based user data. [logic error]
Severity Level: Minor
| auth.set_custom_user_claims(user_id, current_claims) |
Why it matters? ⭐
The code mutates the local current_claims dict but never calls auth.set_custom_user_claims(user_id, current_claims), so Firebase never persists these claim changes. This is a logic bug: tokens and Firebase-stored claims will not reflect the update unless claims are written back.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** user_management.py
**Line:** 40:40
**Comment:**
*Logic Error: In `update_user_profile`, the custom claims dictionary is modified in memory but never written back to Firebase, so the updated `first_name` and `last_name` will not actually be reflected in subsequent token-based user data.
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.| options={"verify_exp": False} | ||
| ) |
There was a problem hiding this comment.
Suggestion: The refresh token is decoded with expiration checks disabled and without verifying that it is actually a refresh token, allowing expired or wrong‑type JWTs to be exchanged for new access tokens and weakening authentication security. [security]
Severity Level: Critical 🚨
| options={"verify_exp": False} | |
| ) | |
| ) | |
| if payload.get("type") != "refresh": | |
| raise ValueError("Invalid token type for refresh") | |
Why it matters? ⭐
The current implementation explicitly disables expiration verification and doesn't verify the token 'type' is a refresh token. That allows expired or access tokens to be exchanged for fresh access tokens — a clear security issue. The suggested change to re-enable expiration checks and assert payload["type"] == "refresh" is a correct and necessary mitigation.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** user_management.py
**Line:** 57:58
**Comment:**
*Security: The refresh token is decoded with expiration checks disabled and without verifying that it is actually a refresh token, allowing expired or wrong‑type JWTs to be exchanged for new access tokens and weakening authentication security.
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. |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
user_management.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
user_management.py (3)
app/auth/models.py (2)
UserResponse(17-23)TokenResponse(33-35)app/auth/dependencies.py (1)
get_current_user(10-32)app/auth/routes.py (1)
refresh_token(76-97)
🪛 Ruff (0.14.8)
user_management.py
47-47: Do not catch blind exception: Exception
(BLE001)
48-48: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
48-48: Create your own exception
(TRY002)
48-48: Avoid specifying long messages outside the exception class
(TRY003)
48-48: Use explicit conversion flag
Replace with conversion flag
(RUF010)
71-71: Do not catch blind exception: Exception
(BLE001)
91-91: Do not catch blind exception: Exception
(BLE001)
103-103: 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)
🔇 Additional comments (2)
user_management.py (2)
50-73: Custom refresh logic weakens token security and bypasses existing Firebase flowThis method decodes the
refresh_tokenwithverify_exp=Falseand does not enforce anytypeclaim or other constraints. Any JWT signed withJWT_SECRET(including an access token) can be reused indefinitely to mint new access tokens, and none of Firebase's revocation/rotation mechanisms are honored. A canonical refresh flow using Firebase auth should be the single source of truth; diverging here is dangerous.A safer approach is to delegate to the existing Firebase‑backed refresh logic instead of decoding the token locally, and verify that having both
/auth/refresh-tokenand/users/refresh-tokenendpoints is intentional and cannot drift in behavior.
99-119: Fixverify_token()to include user status and timestampsThe endpoint hard-codes
is_active=Trueand mapsupdated_attocreated_atbecausefirebase_auth.verify_token()doesn't return these fields, even though it has access to them from the Firebase user record. Updateverify_token()inapp/auth/firebase_auth.py(lines 101-117) to include:
is_active: derived fromuser_record.disabledcreated_at: fromuser_record.user_metadata.creation_timestampOnce those fields are available in
current_user, the endpoint can use them instead of hard-codingis_active=Trueand incorrectly mappingupdated_attocreated_at.
| def __init__(self): | ||
| self.jwt_secret = os.getenv("JWT_SECRET", "default-secret-key") | ||
| self.jwt_algorithm = "HS256" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "user_management.py" -type fRepository: coderabbit-test/pr-reviewer
Length of output: 91
🏁 Script executed:
# Search for the file in common locations
git ls-files | grep -i user_managementRepository: coderabbit-test/pr-reviewer
Length of output: 89
🏁 Script executed:
cat -n user_management.py | head -50Repository: coderabbit-test/pr-reviewer
Length of output: 1979
🏁 Script executed:
# Search for JWT_SECRET references in the codebase
rg "JWT_SECRET" -A 2 -B 2Repository: coderabbit-test/pr-reviewer
Length of output: 1206
🏁 Script executed:
# Check for environment validation or startup checks
rg "RuntimeError|ValueError|raise.*env" user_management.pyRepository: coderabbit-test/pr-reviewer
Length of output: 53
🏁 Script executed:
# Look for deployment/config files
fd -e "\.env" -e "requirements.txt" -e "docker" -e "compose" -e "conftest" -o -name ".github" -o -name "config*"Repository: coderabbit-test/pr-reviewer
Length of output: 301
🏁 Script executed:
cat -n test_auth.py | head -80Repository: coderabbit-test/pr-reviewer
Length of output: 2947
🏁 Script executed:
# Search for how required_vars validation is implemented
rg "required_vars" -A 5 -B 2Repository: coderabbit-test/pr-reviewer
Length of output: 670
🏁 Script executed:
# Check for main/startup validation
rg "def main|if __name__|@app.on_event|@asynccontextmanager" -A 3Repository: coderabbit-test/pr-reviewer
Length of output: 637
🏁 Script executed:
# Check if there's env validation in conftest or similar
fd -e "conftest.py" -o -e "main.py" -o -e "__main__.py" | xargs -I {} cat {}Repository: coderabbit-test/pr-reviewer
Length of output: 187
🏁 Script executed:
# Check app/main.py for startup validation
cat -n app/main.py 2>/dev/null | head -60Repository: coderabbit-test/pr-reviewer
Length of output: 1860
🏁 Script executed:
# If app/main.py doesn't exist, check common startup patterns
find . -name "main.py" -o -name "asgi.py" | xargs cat -n 2>/dev/null | head -80Repository: coderabbit-test/pr-reviewer
Length of output: 2631
🏁 Script executed:
# Check if app/auth/firebase_auth.py has same fallback pattern
rg -A 5 "class FirebaseAuthService" app/auth/firebase_auth.pyRepository: coderabbit-test/pr-reviewer
Length of output: 317
Avoid fallback to a hard-coded JWT secret
JWT_SECRET silently defaulting to "default-secret-key" means a misconfigured deployment will start with a publicly known signing key, allowing trivial token forgery. The same fallback pattern exists in app/auth/firebase_auth.py ("your-secret-key"). While test_auth.py includes validation logic, there is no startup enforcement in app/main.py, so applications will run without error. Require the env var to be explicitly set, or fail at startup with a clear error message.
Replace with:
- self.jwt_secret = os.getenv("JWT_SECRET", "default-secret-key")
+ self.jwt_secret = os.getenv("JWT_SECRET")
+ if not self.jwt_secret:
+ raise RuntimeError("JWT_SECRET must be set in environment")Apply the same fix to app/auth/firebase_auth.py.
🤖 Prompt for AI Agents
In user_management.py around lines 16-18, replace the silent fallback for
JWT_SECRET with a startup-time failure: check os.getenv("JWT_SECRET") and if
it's missing or empty raise a clear exception (or exit) explaining the env var
must be set; remove the default "default-secret-key". Do the same change in
app/auth/firebase_auth.py (replace its "your-secret-key" fallback) so both
modules enforce presence of the JWT secret on startup rather than using
hard-coded defaults.
| async def update_user_profile( | ||
| self, | ||
| user_id: str, | ||
| first_name: str, | ||
| last_name: str | ||
| ) -> Dict[str, Any]: | ||
| """Update user profile information""" | ||
| try: | ||
| from firebase_admin import auth | ||
|
|
||
| # Update display name | ||
| auth.update_user( | ||
| user_id, | ||
| display_name=f"{first_name} {last_name}" | ||
| ) | ||
|
|
||
| user_record = auth.get_user(user_id) | ||
| current_claims = user_record.custom_claims or {} | ||
| current_claims["first_name"] = first_name | ||
| current_claims["last_name"] = last_name | ||
|
|
||
| return { | ||
| "id": user_id, | ||
| "first_name": first_name, | ||
| "last_name": last_name, | ||
| "updated_at": datetime.utcnow().isoformat() | ||
| } | ||
| except Exception as e: | ||
| raise Exception(f"Failed to update user: {str(e)}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "user_management.py" -type fRepository: coderabbit-test/pr-reviewer
Length of output: 91
🏁 Script executed:
cat -n ./user_management.pyRepository: coderabbit-test/pr-reviewer
Length of output: 5725
🌐 Web query:
Firebase Admin SDK Python set_custom_user_claims API documentation
💡 Result:
Summary — Python Admin SDK: auth.set_custom_user_claims(uid, custom_claims, app=None)
- Purpose: set (or clear) custom claims on an existing Firebase user; claims appear in the user’s ID token on next token refresh/sign-in. (firebase.google.com)
- Signature: firebase_admin.auth.set_custom_user_claims(uid, custom_claims, app=None). Returns None (updates server-side user record). (firebase.google.com)
- Parameters:
- uid: user ID string. (firebase.google.com)
- custom_claims: dict (or JSON) of claims; pass None to remove claims. Do not use OIDC reserved keys (sub, iat, iss, etc). Payload must be ≤ ~1000 bytes when serialized. (firebase.google.com)
- app: optional App instance. (firebase.google.com)
- Errors: raises ValueError for invalid uid/claims, FirebaseError on server/update failures. (firebase.google.com)
- Example (Python):
- auth.set_custom_user_claims(uid, {'admin': True}) # grants admin; will appear on next token refresh/sign-in. (firebase.google.com)
Docs / reference (official):
- API reference: firebase_admin.auth module (set_custom_user_claims). (firebase.google.com)
- Guide: Control Access with Custom Claims and Security Rules (examples + best practices). (firebase.google.com)
If you want, I can paste the exact doc snippet or show how to check a user's current claims and update them incrementally.
Citations:
- 1: https://firebase.google.com/docs/reference/admin/python/firebase_admin.auth?utm_source=openai
- 2: https://firebase.google.com/docs/reference/admin/python/firebase_admin.auth?utm_source=openai
- 3: https://firebase.google.com/docs/reference/admin/python/firebase_admin.auth?utm_source=openai
- 4: https://firebase.google.com/docs/reference/admin/python/firebase_admin.auth?utm_source=openai
- 5: https://firebase.google.com/docs/reference/admin/python/firebase_admin.auth?utm_source=openai
- 6: https://firebase.google.com/docs/reference/admin/python/firebase_admin.auth?utm_source=openai
- 7: https://firebase.google.com/docs/auth/admin/custom-claims?utm_source=openai
- 8: https://firebase.google.com/docs/reference/admin/python/firebase_admin.auth?utm_source=openai
- 9: https://firebase.google.com/docs/auth/admin/custom-claims?utm_source=openai
Custom claims are never persisted and exception handling is too broad
The code mutates current_claims and assigns first_name/last_name, but never calls auth.set_custom_user_claims(user_id, current_claims) to persist them to Firebase. This means those values are lost after the method returns; only the display name is actually stored. Also, catching bare Exception and re-raising another generic Exception hides the original error type and prevents proper error handling.
Persist the claims and use appropriate error handling:
current_claims = user_record.custom_claims or {}
current_claims["first_name"] = first_name
current_claims["last_name"] = last_name
+ auth.set_custom_user_claims(user_id, current_claims)
return {
"id": user_id,
"first_name": first_name,
"last_name": last_name,
"updated_at": datetime.utcnow().isoformat()
}
- except Exception as e:
- raise Exception(f"Failed to update user: {str(e)}")
+ except Exception as e:
+ raise HTTPException(
+ status_code=status.HTTP_400_BAD_REQUEST,
+ detail="Failed to update user profile",
+ ) from eCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.8)
47-47: Do not catch blind exception: Exception
(BLE001)
48-48: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
48-48: Create your own exception
(TRY002)
48-48: Avoid specifying long messages outside the exception class
(TRY003)
48-48: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In user_management.py around lines 20 to 48, the code updates the local
current_claims but never persists them to Firebase and it catches broad
Exception then re-raises a generic Exception; fix by calling
auth.set_custom_user_claims(user_id, current_claims) after mutating the claims
so they are actually saved, narrow error handling to catch specific
firebase_admin exceptions (e.g., import and catch
firebase_admin.exceptions.FirebaseError) and re-raise either the original
exception or a new exception that preserves the original exception as the cause
(do not swallow the original type/message).
| async def verify_user_token(self, token: str) -> Optional[Dict[str, Any]]: | ||
| """Verify JWT token - ISSUE: Using JWT instead of Firebase""" | ||
| try: | ||
| payload = jwt.decode( | ||
| token, | ||
| self.jwt_secret, | ||
| algorithms=[self.jwt_algorithm] | ||
| ) | ||
|
|
||
| return { | ||
| "uid": payload.get("user_id"), | ||
| "email": payload.get("email"), | ||
| "first_name": payload.get("first_name", ""), | ||
| "last_name": payload.get("last_name", ""), | ||
| "role": payload.get("role", "user") | ||
| } | ||
| except Exception as e: | ||
| print(f"Token verification failed: {e}") | ||
| return None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.py" | head -20Repository: coderabbit-test/pr-reviewer
Length of output: 311
🏁 Script executed:
git ls-files | grep -E "(user_management|auth)" | head -20Repository: coderabbit-test/pr-reviewer
Length of output: 212
🏁 Script executed:
wc -l user_management.py app/auth/*.pyRepository: coderabbit-test/pr-reviewer
Length of output: 247
🏁 Script executed:
cat -n user_management.pyRepository: coderabbit-test/pr-reviewer
Length of output: 5725
🏁 Script executed:
cat -n app/auth/dependencies.pyRepository: coderabbit-test/pr-reviewer
Length of output: 2627
🏁 Script executed:
cat -n app/auth/firebase_auth.pyRepository: coderabbit-test/pr-reviewer
Length of output: 7451
🏁 Script executed:
rg "verify_user_token|refresh_user_token" --type py -nRepository: coderabbit-test/pr-reviewer
Length of output: 445
🏁 Script executed:
cat -n app/auth/routes.pyRepository: coderabbit-test/pr-reviewer
Length of output: 4886
🏁 Script executed:
rg "print\(" user_management.py -nRepository: coderabbit-test/pr-reviewer
Length of output: 177
🏁 Script executed:
rg "refresh_token_endpoint|validate_token" --type py -B2 -A5Repository: coderabbit-test/pr-reviewer
Length of output: 899
Remove custom JWT validation and consolidate to Firebase authentication using existing dependencies
verify_user_token introduces a second symmetric-key JWT scheme alongside Firebase, and /users/validate wires authentication through that path instead of the existing get_current_user dependency that properly calls firebase_auth.verify_token. This duplicates trust logic, bypasses Firebase's revocation mechanisms (which auth.verify_id_token provides), and validates tokens based purely on a locally signed JWT. Additionally, validate_token takes token as a query parameter over GET, which exposes tokens in server logs, browser history, proxies, and other log aggregation systems.
The codebase already has the proper pattern in place: /auth/verify (line 123-134 in app/auth/routes.py) correctly uses the get_current_user dependency with HTTPBearer scheme. Adopt the same approach for the /users/validate endpoint:
-@router.get("/validate")
-async def validate_token(token: str):
- """Validate token using custom verification"""
- user_data = await user_manager.verify_user_token(token)
-
- if not user_data:
- raise HTTPException(
- status_code=status.HTTP_401_UNAUTHORIZED,
- detail="Invalid token"
- )
-
- return {"valid": True, "user": user_data}
+@router.get("/validate")
+async def validate_token(current_user: Dict[str, Any] = Depends(get_current_user)):
+ """Validate current Firebase token and return user data."""
+ return {"valid": True, "user": current_user}Also replace the print() calls at lines 72 and 92 with your standard logging mechanism and avoid bare except Exception: patterns.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.8)
91-91: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In user_management.py around lines 75 to 93, remove the custom symmetric JWT
verification and instead wire the function/endpoint to use the existing Firebase
authentication dependency (get_current_user / firebase_auth.verify_token) so
tokens are validated via Firebase (including revocation) and not accepted from a
query parameter; change the endpoint to accept credentials via HTTPBearer and
the dependency rather than a token query param. Replace any print() calls at
lines ~72 and ~92 with the project's logger and eliminate the bare except
Exception: blocks — catch and log specific verification exceptions (or re-raise
after logging) so failures are handled explicitly. Ensure the returned user dict
maps from the Firebase user object/claims (uid, email, displayName split into
first/last as appropriate, role from custom claims) and remove local
jwt_secret/jwt_algorithm usage.
CodeAnt-AI Description
Add user profile update, token refresh, and token validation endpoints
What Changed
Impact
✅ Update user display name from profile page✅ Shorter session renewal via refresh endpoint✅ Validate app-issued JWTs without Firebase💡 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.