Implement authentication module with JWT and Firebase integration #3
Conversation
|
""" WalkthroughThis change introduces a complete backend and frontend implementation for the Splitwiser application. The backend, built with FastAPI and MongoDB, provides authentication (email/password and Google OAuth), JWT token management, password reset, and user management. The frontend, built with React Native (Expo), implements authentication flows, secure token storage, and basic navigation. The update also includes deployment scripts, configuration files, CI workflows, and comprehensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant MongoDB
participant Firebase
User->>Frontend: Submits signup/login form
Frontend->>Backend: POST /auth/signup/email or /auth/login/email
Backend->>MongoDB: Check/create user, hash/verify password
Backend-->>Frontend: Return JWT tokens and user info
User->>Frontend: Chooses Google login
Frontend->>Firebase: Get Google ID token
Frontend->>Backend: POST /auth/login/google (with ID token)
Backend->>Firebase: Verify ID token
Backend->>MongoDB: Create/update user
Backend-->>Frontend: Return JWT tokens and user info
Frontend->>Backend: POST /auth/refresh (with refresh token)
Backend->>MongoDB: Validate and rotate refresh token
Backend-->>Frontend: Return new JWT tokens
User->>Frontend: Requests password reset
Frontend->>Backend: POST /auth/password/reset/request
Backend->>MongoDB: Generate and store reset token
Backend-->>Frontend: Success response (reset link sent/logged)
User->>Frontend: Confirms password reset
Frontend->>Backend: POST /auth/password/reset/confirm
Backend->>MongoDB: Validate token, update password, revoke tokens
Backend-->>Frontend: Success response
Possibly related PRs
Poem
""" 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 Biome (1.9.4)frontend/contexts/AuthContext.tsx[error] 136-136: The catch clause that only rethrows the original error is useless. An unnecessary catch clause can be confusing. (lint/complexity/noUselessCatch) [error] 156-156: The catch clause that only rethrows the original error is useless. An unnecessary catch clause can be confusing. (lint/complexity/noUselessCatch) ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Docstrings generation was requested by @Devasy23. * #3 (comment) The following files were modified: * `backend/app/auth/routes.py` * `backend/app/auth/security.py` * `backend/app/auth/service.py` * `backend/app/database.py` * `backend/app/dependencies.py` * `backend/generate_secret.py` * `backend/main.py`
|
Note Generated docstrings for this pull request at #4 |
…zJ8HjX74294ggpC0hBqSwJSX2nqTXN 📝 Add docstrings to `feature/auth-service-workflow`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (18)
backend/generate_secret.py (1)
4-9: Optional: Simplify secret generationThe
generate_jwt_secretfunction is secure, but you could usesecrets.token_urlsafe(48)for a concise, URL-safe key of similar length.Apply this diff if adopted:
def generate_jwt_secret(): """Generate a secure JWT secret key""" - alphabet = string.ascii_letters + string.digits + "!@#$%^&*" - secret_key = ''.join(secrets.choice(alphabet) for _ in range(64)) - return secret_key + # Generate a URL-safe secret (~64 characters) + return secrets.token_urlsafe(48)backend/app/config.py (2)
1-1: Remove unused importThe
osmodule is imported but never used. Please remove this import to clean up dependencies.- import os
14-14: Fix indentation for consistencyThe
# Firebasecomment is over-indented compared to other section headers. Align it with the class indentation.- # Firebase + # Firebasebackend/requirements.txt (1)
1-13: Be deliberate about hard-pinning every dependency
All packages are frozen to an exact patch version. That guarantees reproducibility but also blocks receiving upstream security patches and bug-fix releases (e.g.uvicorn 0.34.4landed a CVE fix). Consider:-fastapi==0.115.12 +fastapi~=0.115 # receive compatible patch releasesIf deterministic builds are required in CI, a separate
requirements-lock.txtgenerated viapip-compileis a safer compromise.backend/start_server.sh (2)
6-12: Missingset -euo pipefail
Without it the script continues after failed commands (e.g. venv creation). Add defensive flags at top:-#!/bin/bash +#!/usr/bin/env bash +set -euo pipefail
18-22:pip installon every start hurts startup time
Runningpip install -r requirements.txteach launch negates the benefit of the persistent venv. Suggest gating with a hash-check:if ! pip check >/dev/null 2>&1; then echo "Installing/Updating dependencies..." pip install -r requirements.txt fibackend/README.md (2)
34-36: Fix markdown-lint “bare URL” warnings-API: http://localhost:8000 -Interactive docs: http://localhost:8000/docs -ReDoc: http://localhost:8000/redoc +API: <http://localhost:8000> +Interactive docs: <http://localhost:8000/docs> +ReDoc: <http://localhost:8000/redoc>
56-56: Specify language for fenced block-``` +```textThis silences MD040 and improves readability.
backend/app/dependencies.py (1)
9-9: Ignore Ruff B008: FastAPI pattern is intentional
TheDepends(security)default is the canonical FastAPI style; safe to# noqa: B008if you want to silence linters.backend/main.py (2)
1-1: Remove unusedHTTPExceptionimport
HTTPExceptionis imported but never referenced in this module.
Keeping dead imports bloats byte-code and may hide real “unused” warnings in the future.-from fastapi import FastAPI, HTTPException +from fastapi import FastAPI
41-48:uvicorn.run("main:app")is fragile when executed outside thebackendfolderWhen this file is launched from the repo root (e.g.,
python -m backend.main) the module path becomesbackend.main.
Hard-coding"main:app"will fail hot-reload workers.
Preferuvicorn.run(app, …)or pass the fully-qualified module path:- "main:app", + "backend.main:app",Or simply:
uvicorn.run(app, host="0.0.0.0", port=8000, reload=settings.debug)backend/app/database.py (1)
16-20: Useloggerinstead of
AsyncIOMotorClient.close()is synchronous; calling it in an async function does not requireawait.
Also, prefer structured logs.- mongodb.client.close() - print("Disconnected from MongoDB") + mongodb.client.close() + logger.info("Disconnected from MongoDB")backend/app/auth/security.py (2)
10-15:bcrypt__roundsfallback silently swallows real mis-configurationCatching all exceptions when instantiating
CryptContextmay mask unrelated errors (e.g., missing algorithm).
CatchValueError/ImportErrorspecifically or remove the broad fallback; the first failure usually means the wheel is missing.
40-50: Preserve original exception when re-raisingRaising without
fromdiscards the traceback, hampering debugging tools that rely on exception chaining.- except JWTError: - raise HTTPException( + except JWTError as err: + raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Could not validate credentials", headers={"WWW-Authenticate": "Bearer"}, - ) + ) from err.gitignore (1)
46-60: Duplicate ignore patterns
*.pem,*.key,*.p12appear twice (lines 46-47 and 57-60).
Trivial, but removing duplicates keeps the file concise.backend/app/auth/schemas.py (1)
31-42: Minor:currencydefault might be better expressed as an EnumIf multiple currencies will be supported, introduce an
Enumnow to prevent free-form strings later.from enum import Enum class Currency(str, Enum): USD = "USD" EUR = "EUR" # … class UserResponse(BaseModel): currency: Currency = Currency.USDbackend/app/auth/routes.py (1)
1-8: Prune unused imports to keep module clean
DependsandErrorResponseare never referenced. Drop them to avoid lint errors (F401) and reduce byte-code size.-from fastapi import APIRouter, HTTPException, status, Depends +from fastapi import APIRouter, HTTPException, status ... - TokenVerifyRequest, AuthResponse, TokenResponse, SuccessResponse, - UserResponse, ErrorResponse + TokenVerifyRequest, AuthResponse, TokenResponse, SuccessResponse, + UserResponsebackend/app/auth/service.py (1)
1-9: Remove dead imports
OptionalandUserResponseare not used, generatingF401warnings.-from typing import Optional, Dict, Any +from typing import Dict, Any ... -from app.auth.schemas import UserResponse
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.gitignore(2 hunks)backend/.env.example(1 hunks)backend/README.md(1 hunks)backend/app/__init__.py(1 hunks)backend/app/auth/__init__.py(1 hunks)backend/app/auth/routes.py(1 hunks)backend/app/auth/schemas.py(1 hunks)backend/app/auth/security.py(1 hunks)backend/app/auth/service.py(1 hunks)backend/app/config.py(1 hunks)backend/app/database.py(1 hunks)backend/app/dependencies.py(1 hunks)backend/generate_secret.py(1 hunks)backend/main.py(1 hunks)backend/requirements.txt(1 hunks)backend/start_server.bat(1 hunks)backend/start_server.sh(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
backend/app/auth/__init__.py (3)
backend/app/auth/routes.py (1)
verify_token(145-160)backend/app/auth/security.py (2)
verify_token(40-50)create_access_token(24-34)backend/app/auth/schemas.py (1)
UserResponse(32-41)
backend/main.py (1)
backend/app/database.py (2)
connect_to_mongo(10-14)close_mongo_connection(16-20)
backend/app/dependencies.py (2)
backend/app/auth/security.py (1)
verify_token(40-50)backend/app/database.py (1)
get_database(22-24)
backend/app/auth/security.py (1)
backend/app/auth/routes.py (1)
verify_token(145-160)
backend/app/auth/schemas.py (1)
backend/app/auth/routes.py (1)
refresh_token(107-142)
backend/app/auth/routes.py (4)
backend/app/auth/schemas.py (11)
EmailSignupRequest(6-9)EmailLoginRequest(11-13)GoogleLoginRequest(15-16)RefreshTokenRequest(18-19)PasswordResetRequest(21-22)PasswordResetConfirm(24-26)TokenVerifyRequest(28-29)AuthResponse(43-46)TokenResponse(48-50)SuccessResponse(52-54)UserResponse(32-41)backend/app/auth/security.py (2)
create_access_token(24-34)verify_token(40-50)backend/app/auth/service.py (7)
create_user_with_email(31-70)authenticate_user_with_email(72-89)authenticate_with_google(91-163)refresh_access_token(165-199)verify_access_token(200-222)request_password_reset(224-251)confirm_password_reset(253-289)backend/app/database.py (1)
get_database(22-24)
backend/app/auth/service.py (4)
backend/app/database.py (1)
get_database(22-24)backend/app/auth/security.py (5)
get_password_hash(20-22)verify_password(16-18)create_refresh_token(36-38)generate_reset_token(52-54)verify_token(40-50)backend/app/auth/schemas.py (1)
UserResponse(32-41)backend/app/auth/routes.py (4)
refresh_token(107-142)verify_token(145-160)request_password_reset(163-175)confirm_password_reset(178-195)
🪛 dotenv-linter (3.3.0)
backend/.env.example
[warning] 4-4: [UnorderedKey] The DATABASE_NAME key should go before the DEBUG key
[warning] 6-6: [UnorderedKey] The ALGORITHM key should go before the DATABASE_NAME key
[warning] 7-7: [UnorderedKey] The ACCESS_TOKEN_EXPIRE_MINUTES key should go before the ALGORITHM key
[warning] 8-8: [UnorderedKey] The REFRESH_TOKEN_EXPIRE_DAYS key should go before the SECRET_KEY key
🪛 markdownlint-cli2 (0.17.2)
backend/README.md
34-34: Bare URL used
null
(MD034, no-bare-urls)
35-35: Bare URL used
null
(MD034, no-bare-urls)
36-36: Bare URL used
null
(MD034, no-bare-urls)
56-56: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 Ruff (0.11.9)
backend/app/config.py
1-1: os imported but unused
Remove unused import: os
(F401)
backend/main.py
1-1: fastapi.HTTPException imported but unused
Remove unused import: fastapi.HTTPException
(F401)
backend/app/dependencies.py
9-9: 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)
40-40: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
41-45: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/app/auth/security.py
46-50: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/app/auth/routes.py
1-1: fastapi.Depends imported but unused
Remove unused import: fastapi.Depends
(F401)
6-6: app.auth.schemas.ErrorResponse imported but unused
Remove unused import: app.auth.schemas.ErrorResponse
(F401)
42-45: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
73-76: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
101-104: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
139-142: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
156-156: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
157-160: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
172-175: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
192-195: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/app/auth/service.py
2-2: typing.Optional imported but unused
Remove unused import: typing.Optional
(F401)
8-8: app.auth.schemas.UserResponse imported but unused
Remove unused import: app.auth.schemas.UserResponse
(F401)
67-70: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
155-158: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
160-163: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🪛 Pylint (3.3.7)
backend/app/config.py
[refactor] 21-21: Too few public methods (0/2)
(R0903)
[refactor] 4-4: Too few public methods (0/2)
(R0903)
backend/app/database.py
[refactor] 4-4: Too few public methods (0/2)
(R0903)
backend/app/auth/schemas.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
[refactor] 11-11: Too few public methods (0/2)
(R0903)
[refactor] 15-15: Too few public methods (0/2)
(R0903)
[refactor] 18-18: Too few public methods (0/2)
(R0903)
[refactor] 21-21: Too few public methods (0/2)
(R0903)
[refactor] 24-24: Too few public methods (0/2)
(R0903)
[refactor] 28-28: Too few public methods (0/2)
(R0903)
[refactor] 40-40: Too few public methods (0/2)
(R0903)
[refactor] 32-32: Too few public methods (0/2)
(R0903)
[refactor] 43-43: Too few public methods (0/2)
(R0903)
[refactor] 48-48: Too few public methods (0/2)
(R0903)
[refactor] 52-52: Too few public methods (0/2)
(R0903)
[refactor] 56-56: Too few public methods (0/2)
(R0903)
🔇 Additional comments (9)
backend/app/__init__.py (1)
1-2: No review neededThe
__init__.pyfile correctly initializes theapppackage without any functional code.backend/generate_secret.py (1)
11-16: CLI output is clearThe script prints a clear message and ready-to-use
.enventry. No changes needed here.backend/.env.example (1)
1-13: Env template grouping is intentionalVariables are grouped by functionality for readability. Ensure the
SECRET_KEYplaceholder is updated before production.backend/app/auth/__init__.py (1)
1-7: Define public API surfaceThe
__all__list cleanly exposes the auth router, service, security functions, and schema. Good module encapsulation.backend/start_server.bat (1)
22-35: Script assumes execution frombackend\folder
uvicorn main:appwill fail if the user runs the script from repository root (backendis not onPYTHONPATH). Prependcd /d "%~dp0"to ensure the working directory is the script’s own folder.backend/start_server.sh (1)
30-35: Same directory assumption as BAT script
InsertSCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"andcd "$SCRIPT_DIR"so the command works regardless of caller’s CWD.backend/main.py (1)
16-22: ```shell
#!/bin/bash
set -eecho "Displaying the first 200 lines of backend/app/config.py:"
sed -n '1,200p' backend/app/config.pyecho -e "\nSearching for any 'cors' related fields in Settings:"
rg -n "cors" backend/app/config.pyecho -e "\nSearching for a 'debug' field in Settings:"
rg -n "debug" backend/app/config.py</details> <details> <summary>backend/app/auth/security.py (1)</summary> `24-34`: ```shell #!/bin/bash set -e echo "Extracting full verify_access_token function..." rg -P "async def verify_access_token" -A 50 backend/app/auth/service.pybackend/app/auth/service.py (1)
155-163: Use exception chaining for Google auth pathSame rationale as routes file – helps distinguish Firebase failures from our handling issues.
- except firebase_auth.InvalidIdTokenError: + except firebase_auth.InvalidIdTokenError as err: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid Google ID token" - ) + ) from err
| @REM REM Check if virtual environment exists | ||
| @REM if not exist "venv\" ( | ||
| @REM echo Creating virtual environment... | ||
| @REM python -m venv venv | ||
| @REM echo. | ||
| @REM ) | ||
|
|
||
| @REM REM Activate virtual environment | ||
| @REM echo Activating virtual environment... | ||
| @REM call venv\Scripts\activate.bat | ||
| @REM echo. | ||
|
|
||
| @REM REM Install dependencies | ||
| @REM echo Installing dependencies... | ||
| @REM pip install -r requirements.txt | ||
| @REM echo. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Commented-out venv bootstrap will confuse new contributors
The BAT script skips venv creation / dependency install while the *nix script performs them. That asymmetry will bite Windows users.
-@REM REM Check if virtual environment exists
-@REM if not exist "venv\" (
-@REM echo Creating virtual environment...
-@REM python -m venv venv
-@REM echo.
-@REM )
+REM Check if virtual environment exists
+if not exist "venv\" (
+ echo Creating virtual environment...
+ python -m venv venv
+ echo.
+)Same for activation / pip install. Either delete the dead code or enable it.
🤖 Prompt for AI Agents
In backend/start_server.bat around lines 5 to 21, the virtual environment setup,
activation, and dependency installation commands are commented out, causing
inconsistency with the *nix script and confusing Windows users. To fix this,
uncomment the sections that check for the virtual environment, create it if
missing, activate it, and install dependencies from requirements.txt, ensuring
the Windows script mirrors the *nix setup process.
| async def connect_to_mongo(): | ||
| """Create database connection""" | ||
| mongodb.client = AsyncIOMotorClient(settings.mongodb_url) | ||
| mongodb.database = mongodb.client[settings.database_name] | ||
| print("Connected to MongoDB") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
No error handling when connecting to MongoDB
If the connection string is invalid or the server is unavailable, the app will crash at startup with an unhandled exception.
Wrap the connection in try/except and log failures clearly so Kubernetes/Lambda health-checks don’t hang indefinitely.
- mongodb.client = AsyncIOMotorClient(settings.mongodb_url)
- mongodb.database = mongodb.client[settings.database_name]
- print("Connected to MongoDB")
+ try:
+ mongodb.client = AsyncIOMotorClient(
+ settings.mongodb_url,
+ serverSelectionTimeoutMS=5_000 # fail fast
+ )
+ # Force an immediate handshake
+ await mongodb.client.admin.command("ping")
+ mongodb.database = mongodb.client[settings.database_name]
+ logger.info("Connected to MongoDB")
+ except Exception as exc:
+ logger.exception("MongoDB connection failed")
+ raise📝 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.
| async def connect_to_mongo(): | |
| """Create database connection""" | |
| mongodb.client = AsyncIOMotorClient(settings.mongodb_url) | |
| mongodb.database = mongodb.client[settings.database_name] | |
| print("Connected to MongoDB") | |
| async def connect_to_mongo(): | |
| """Create database connection""" | |
| try: | |
| mongodb.client = AsyncIOMotorClient( | |
| settings.mongodb_url, | |
| serverSelectionTimeoutMS=5_000 # fail fast | |
| ) | |
| # Force an immediate handshake | |
| await mongodb.client.admin.command("ping") | |
| mongodb.database = mongodb.client[settings.database_name] | |
| logger.info("Connected to MongoDB") | |
| except Exception as exc: | |
| logger.exception("MongoDB connection failed") | |
| raise |
🤖 Prompt for AI Agents
In backend/app/database.py around lines 10 to 14, the MongoDB connection code
lacks error handling, which can cause the app to crash on startup if the
connection fails. Wrap the connection logic in a try/except block, catch
exceptions related to connection failures, and log the error clearly. This will
prevent unhandled exceptions and allow health-check systems to detect failures
properly.
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail=f"Registration failed: {str(e)}" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve root cause when re-raising
Every except Exception as e: block discards the traceback. Use exception chaining to aid debugging:
- except Exception as e:
- raise HTTPException(status_code=..., detail=...) # truncated
+ except Exception as err:
+ raise HTTPException(status_code=..., detail=...) from errAlso applies to: 73-76, 101-104, 139-142, 157-160, 172-175, 192-195
🧰 Tools
🪛 Ruff (0.11.9)
42-45: 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 backend/app/auth/routes.py around lines 42 to 45 and similarly at lines
73-76, 101-104, 139-142, 157-160, 172-175, and 192-195, the current exception
handling raises HTTPException without preserving the original traceback. Modify
each except block to use exception chaining by adding "from e" when raising
HTTPException, e.g., "raise HTTPException(...) from e", to retain the root cause
and improve debugging.
| # For development/free tier: just log the reset token | ||
| # In production, you would send this via email | ||
| print(f"Password reset token for {email}: {reset_token}") | ||
| print(f"Reset link: https://yourapp.com/reset-password?token={reset_token}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid logging sensitive reset tokens in plaintext
Reset tokens are effectively credentials. Printing them to stdout risks leakage via logs. Gate this behind a debug flag or remove once email delivery is implemented.
[security]
🤖 Prompt for AI Agents
In backend/app/auth/service.py around lines 246 to 249, the reset token is being
printed in plaintext, which risks leaking sensitive credentials in logs. Modify
the code to only print the reset token when a debug or development mode flag is
enabled, or remove these print statements entirely once email delivery is
implemented to securely send the token.
| async def verify_access_token(self, token: str) -> Dict[str, Any]: | ||
| """Verify access token and return user""" | ||
| from app.auth.security import verify_token | ||
|
|
||
| payload = verify_token(token) | ||
| user_id = payload.get("sub") | ||
|
|
||
| if not user_id: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_401_UNAUTHORIZED, | ||
| detail="Invalid token" | ||
| ) | ||
|
|
||
| db = self.get_db() | ||
| user = await db.users.find_one({"_id": user_id}) | ||
|
|
There was a problem hiding this comment.
Token verification fails for all users – _id type mismatch
verify_access_token looks up the user with the string value stored in JWT, but MongoDB documents keep _id as ObjectId. The query therefore returns None, causing false “User not found” errors.
+from bson import ObjectId
...
- user = await db.users.find_one({"_id": user_id})
+ user = await db.users.find_one({"_id": ObjectId(user_id)})Add minimal validation that user_id is a valid ObjectId string to avoid InvalidId crashes.
📝 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.
| async def verify_access_token(self, token: str) -> Dict[str, Any]: | |
| """Verify access token and return user""" | |
| from app.auth.security import verify_token | |
| payload = verify_token(token) | |
| user_id = payload.get("sub") | |
| if not user_id: | |
| raise HTTPException( | |
| status_code=status.HTTP_401_UNAUTHORIZED, | |
| detail="Invalid token" | |
| ) | |
| db = self.get_db() | |
| user = await db.users.find_one({"_id": user_id}) | |
| # At the top of the file | |
| from bson import ObjectId | |
| # … | |
| async def verify_access_token(self, token: str) -> Dict[str, Any]: | |
| """Verify access token and return user""" | |
| from app.auth.security import verify_token | |
| payload = verify_token(token) | |
| user_id = payload.get("sub") | |
| if not user_id: | |
| raise HTTPException( | |
| status_code=status.HTTP_401_UNAUTHORIZED, | |
| detail="Invalid token" | |
| ) | |
| db = self.get_db() | |
| user = await db.users.find_one({"_id": ObjectId(user_id)}) | |
| # … |
🤖 Prompt for AI Agents
In backend/app/auth/service.py around lines 200 to 215, the user lookup fails
because the JWT token's "sub" field is a string, but MongoDB expects an ObjectId
for the "_id" field. To fix this, convert the user_id string to an ObjectId
before querying the database. Also, add validation to check if the user_id
string is a valid ObjectId to prevent InvalidId exceptions, and handle invalid
cases by raising an appropriate HTTPException.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
backend/app/dependencies.py (3)
41-44: Avoid mutating the cached Motor document in-place
Previous reviews flagged the same risk: converting_idtostrdirectly on the dictionary can pollute the Motor cache. Return a copy instead.
47-52: Remove unused variable and chain exceptionRuff F841/B904:
eis never used and traceback is lost.- except Exception as e: - raise HTTPException( + except Exception as exc: + raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Could not validate credentials", headers={"WWW-Authenticate": "Bearer"}, - ) + ) from exc
31-33:_idmust be anObjectId, not a string
user_idin the token is a string, whereas Mongo stores_idasbson.ObjectId.
The current query will silently returnNone, causing a false-positive “User not found”.- user = await db.users.find_one({"_id": user_id}) + from bson import ObjectId + user = await db.users.find_one({"_id": ObjectId(user_id)})backend/app/database.py (1)
16-18: Wrap Mongo connection in try/except and ping the serverA transient network failure will crash the app at startup. Reuse the refactor suggested in previous reviews to fail fast and log clearly.
backend/app/auth/routes.py (1)
152-156: Ensureuser_idis JSON-serialisable
token_record["user_id"]is abson.ObjectId;python-josewill refuse to encode it. Cast tostr.- data={"sub": token_record["user_id"]}, + data={"sub": str(token_record["user_id"])},backend/app/auth/service.py (2)
270-272: Still querying_idas plain string – ObjectId mismatchThis was flagged earlier. A MongoDB
_idis anObjectId; querying with a string makes every lookup fail.- user = await db.users.find_one({"_id": user_id}) + from bson.errors import InvalidId + try: + object_id = ObjectId(user_id) + except (InvalidId, TypeError): + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Invalid token subject" + ) + user = await db.users.find_one({"_id": object_id})
307-311: Reset token logged in plaintextReset tokens are credentials. Printing them (and the reset link) leaks PII in logs. Gate this behind a debug flag or remove entirely once email is wired up.
🧹 Nitpick comments (3)
backend/main.py (1)
1-1: Drop unusedHTTPExceptionimport-from fastapi import FastAPI, HTTPException +from fastapi import FastAPIbackend/app/auth/routes.py (2)
1-8: Remove unused imports (Depends,ErrorResponse)They trigger Ruff F401 and add noise.
-from fastapi import APIRouter, HTTPException, status, Depends +from fastapi import APIRouter, HTTPException, status ... - TokenVerifyRequest, AuthResponse, TokenResponse, SuccessResponse, - UserResponse, ErrorResponse + TokenVerifyRequest, AuthResponse, TokenResponse, SuccessResponse, + UserResponse
139-140: Move database import to the top for clarity & speedDynamic imports in hot paths hurt readability and cold-start performance.
-from app.database import get_databaseMove this to the module-level import section.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/app/auth/routes.py(1 hunks)backend/app/auth/security.py(1 hunks)backend/app/auth/service.py(1 hunks)backend/app/database.py(1 hunks)backend/app/dependencies.py(1 hunks)backend/generate_secret.py(1 hunks)backend/main.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/generate_secret.py
🧰 Additional context used
🧬 Code Graph Analysis (4)
backend/app/auth/routes.py (4)
backend/app/auth/schemas.py (12)
EmailSignupRequest(6-9)EmailLoginRequest(11-13)GoogleLoginRequest(15-16)RefreshTokenRequest(18-19)PasswordResetRequest(21-22)PasswordResetConfirm(24-26)TokenVerifyRequest(28-29)AuthResponse(43-46)TokenResponse(48-50)SuccessResponse(52-54)UserResponse(32-41)ErrorResponse(56-57)backend/app/auth/security.py (2)
create_access_token(41-62)verify_token(73-88)backend/app/auth/service.py (7)
create_user_with_email(35-89)authenticate_user_with_email(91-115)authenticate_with_google(117-199)refresh_access_token(201-245)verify_access_token(246-279)request_password_reset(281-312)confirm_password_reset(314-364)backend/app/database.py (1)
get_database(31-37)
backend/app/auth/security.py (1)
backend/app/auth/routes.py (1)
verify_token(171-191)
backend/app/dependencies.py (2)
backend/app/auth/security.py (1)
verify_token(73-88)backend/app/database.py (1)
get_database(31-37)
backend/main.py (1)
backend/app/database.py (2)
connect_to_mongo(10-18)close_mongo_connection(20-29)
🪛 Ruff (0.11.9)
backend/app/auth/routes.py
1-1: fastapi.Depends imported but unused
Remove unused import: fastapi.Depends
(F401)
6-6: app.auth.schemas.ErrorResponse imported but unused
Remove unused import: app.auth.schemas.ErrorResponse
(F401)
53-56: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
88-91: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
120-123: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
165-168: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
187-187: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
188-191: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
208-211: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
239-242: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/app/auth/service.py
26-26: SyntaxError: Simple statements must be separated by newlines or semicolons
26-26: SyntaxError: Simple statements must be separated by newlines or semicolons
26-26: SyntaxError: Simple statements must be separated by newlines or semicolons
26-27: SyntaxError: Expected an identifier
backend/app/auth/security.py
84-88: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/app/dependencies.py
9-9: 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)
47-47: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
48-52: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/main.py
1-1: fastapi.HTTPException imported but unused
Remove unused import: fastapi.HTTPException
(F401)
🪛 Pylint (3.3.7)
backend/app/auth/routes.py
[error] 8-8: Cannot import 'app.auth.service' due to 'invalid syntax (app.auth.service, line 26)'
(E0001)
[error] 8-8: No name 'service' in module 'app.auth'
(E0611)
backend/app/auth/service.py
[error] 26-26: Parsing failed: 'invalid syntax (app.auth.service, line 26)'
(E0001)
backend/app/database.py
[refactor] 4-4: Too few public methods (0/2)
(R0903)
🔇 Additional comments (1)
backend/main.py (1)
17-22: CORS set to*– tighten before productionWide-open CORS is fine for local dev but dangerous in prod. Ensure
allow_originsis restricted via env/config.
| except JWTError: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_401_UNAUTHORIZED, | ||
| detail="Could not validate credentials", | ||
| headers={"WWW-Authenticate": "Bearer"}, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Chain raised HTTPException to preserve original traceback
Swallowing the original JWTError hides valuable debugging data and triggers Ruff B904. Re-raise with from err (or from None if you don’t want the traceback) and drop the unused variable.
- except JWTError:
- raise HTTPException(
+ except JWTError as err:
+ raise HTTPException(
status_code=status.HTTP_401_UNAUTHORIZED,
detail="Could not validate credentials",
headers={"WWW-Authenticate": "Bearer"},
- )
+ ) from err📝 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.
| except JWTError: | |
| raise HTTPException( | |
| status_code=status.HTTP_401_UNAUTHORIZED, | |
| detail="Could not validate credentials", | |
| headers={"WWW-Authenticate": "Bearer"}, | |
| ) | |
| except JWTError as err: | |
| raise HTTPException( | |
| status_code=status.HTTP_401_UNAUTHORIZED, | |
| detail="Could not validate credentials", | |
| headers={"WWW-Authenticate": "Bearer"}, | |
| ) from err |
🧰 Tools
🪛 Ruff (0.11.9)
84-88: 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 backend/app/auth/security.py around lines 83 to 88, the except block catches
JWTError but raises HTTPException without chaining the original exception, which
loses traceback information. Modify the except clause to capture the JWTError as
a variable, then raise HTTPException using "from err" to preserve the original
traceback for better debugging. Remove any unused variables accordingly.
| except Exception as e: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail=f"Registration failed: {str(e)}" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consistently chain exceptions & drop unused e variables
All these except Exception as e: blocks:
- Create an unused variable (
F841). - Discard the original traceback (
B904).
Apply the pattern below everywhere:
-except Exception as e:
- raise HTTPException(..., detail=f"...: {str(e)}")
+except Exception as exc:
+ raise HTTPException(..., detail="Internal server error") from exc(Consider omitting the original message in the client-facing detail to avoid leaking internals.)
Also applies to: 88-91, 120-123, 165-168, 208-211, 239-242
🧰 Tools
🪛 Ruff (0.11.9)
53-56: 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 backend/app/auth/routes.py at lines 52-56 and also at 88-91, 120-123,
165-168, 208-211, and 239-242, the except blocks catch exceptions as 'e' but do
not use 'e' and raise new HTTPExceptions without chaining the original
exceptions, causing unused variable warnings and loss of traceback. Remove the
'as e' part from the except clause if the variable is unused, and when raising
HTTPException, use 'from e' to chain the original exception. Also, consider
removing the original error message from the detail to avoid leaking internal
information.
| try: | ||
| result = await db.users.insert_one(user_doc) | ||
| user_doc["_id"] = str(result.inserted_id) | ||
|
|
||
| # Create refresh token | ||
| refresh_token = await self._create_refresh_token_record(str(result.inserted_id)) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent type for _id across code paths
create_user_with_email converts inserted_id to str, while the Google-auth branch keeps it an ObjectId. Returned payloads therefore vary unpredictably. Pick one representation (commonly str for outward-facing responses, ObjectId internally) and normalise before returning.
Also applies to: 178-181
🤖 Prompt for AI Agents
In backend/app/auth/service.py around lines 74 to 80 and also lines 178 to 181,
the `_id` field is inconsistently typed as a string in one code path and as an
ObjectId in another, causing unpredictable payloads. To fix this, choose a
single representation for `_id` (preferably convert ObjectId to string for
outward-facing responses) and ensure all code paths normalize `_id` to this type
before returning any user data.
| def __init__(self): | ||
| Initializes the AuthService instance. | ||
| pass |
There was a problem hiding this comment.
Syntax error: missing docstring quotes in __init__
Line 26 is treated as a bare identifier, breaking the whole module.
- def __init__(self):
- Initializes the AuthService instance.
- pass
+ def __init__(self):
+ """Initializes the AuthService instance."""
+ pass📝 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.
| def __init__(self): | |
| Initializes the AuthService instance. | |
| pass | |
| def __init__(self): | |
| """Initializes the AuthService instance.""" | |
| pass |
🧰 Tools
🪛 Ruff (0.11.9)
26-26: SyntaxError: Simple statements must be separated by newlines or semicolons
26-26: SyntaxError: Simple statements must be separated by newlines or semicolons
26-26: SyntaxError: Simple statements must be separated by newlines or semicolons
26-27: SyntaxError: Expected an identifier
🪛 Pylint (3.3.7)
[error] 26-26: Parsing failed: 'invalid syntax (app.auth.service, line 26)'
(E0001)
🤖 Prompt for AI Agents
In backend/app/auth/service.py at lines 25 to 27, the __init__ method has a
docstring missing the enclosing quotes, causing a syntax error. Fix this by
adding triple quotes around the docstring text to properly define it as a string
literal inside the method.
…ove AuthService docstring
Devasy
added
documentation
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/app/auth/service.py (2)
307-311: Reset tokens are still logged in cleartextLogging credentials is a security risk; gate behind
settings.debugor remove.
259-272:verify_access_tokenstill queries with a string_id– lookup will always failMongo stores
_idasObjectId; passing a raw string yields no match and every token appears invalid. Same issue was flagged earlier.- db = self.get_db() - user = await db.users.find_one({"_id": user_id}) + from bson import ObjectId + db = self.get_db() + try: + oid = ObjectId(user_id) + except Exception: + raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, + detail="Invalid token subject") + user = await db.users.find_one({"_id": oid})
🧹 Nitpick comments (5)
backend/app/config.py (1)
1-2: Remove unusedosimportThe
osmodule is imported but never referenced.
Cleaning up unused imports avoids unnecessary namespace pollution and keeps Ruff quiet.-import osbackend/main.py (2)
1-1:HTTPExceptionimport is dead codeNothing in this module raises
HTTPException; delete the import.-from fastapi import FastAPI, HTTPException +from fastapi import FastAPI
55-61:uvicorn.run("main:app", …)breaks when module isn’t top-levelIf this file is executed outside the
backend/directory,mainrefers to the wrong module.
Safer:- "main:app", + "backend.main:app",or simply call
uvicorn.run(app, …).backend/app/auth/service.py (2)
1-8: Cull unused imports to keep the module lean
OptionalandUserResponseare never referenced. Dropping them removes Ruff F401 noise.-from typing import Optional, Dict, Any +from typing import Dict, Any ... -from app.auth.schemas import UserResponse
25-27: Prefer a docstring over a comment in__init__Docstrings are discoverable by IDEs and tooling.
- def __init__(self): - # Initializes the AuthService instance. - pass + def __init__(self): + """Initialises the AuthService instance.""" + pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/Procfile(1 hunks)backend/app/auth/service.py(1 hunks)backend/app/config.py(1 hunks)backend/main.py(1 hunks)backend/runtime.txt(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- backend/runtime.txt
- backend/Procfile
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/app/auth/service.py (4)
backend/app/database.py (1)
get_database(31-37)backend/app/auth/security.py (5)
get_password_hash(29-39)verify_password(16-27)create_refresh_token(64-71)generate_reset_token(90-97)verify_token(73-88)backend/app/auth/schemas.py (1)
UserResponse(32-41)backend/app/auth/routes.py (4)
refresh_token(126-168)verify_token(171-191)request_password_reset(194-211)confirm_password_reset(214-242)
backend/main.py (1)
backend/app/database.py (2)
connect_to_mongo(10-18)close_mongo_connection(20-29)
🪛 Ruff (0.11.9)
backend/app/auth/service.py
2-2: typing.Optional imported but unused
Remove unused import: typing.Optional
(F401)
8-8: app.auth.schemas.UserResponse imported but unused
Remove unused import: app.auth.schemas.UserResponse
(F401)
86-89: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
191-194: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
196-199: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/app/config.py
1-1: os imported but unused
Remove unused import: os
(F401)
backend/main.py
1-1: fastapi.HTTPException imported but unused
Remove unused import: fastapi.HTTPException
(F401)
🪛 Pylint (3.3.7)
backend/app/config.py
[refactor] 23-23: Too few public methods (0/2)
(R0903)
[refactor] 4-4: Too few public methods (0/2)
(R0903)
🔇 Additional comments (2)
backend/app/config.py (1)
10-13: Hard-coded fallbacksecret_keyis a security foot-gunCommitting a real-looking default key means the service might run in staging/production without supplying a secure value via the environment, compromising JWT integrity.
Either:
- Make the field mandatory (no default) so the app fails fast when
SECRET_KEYis missing, or- Replace the placeholder with an obviously invalid sentinel such as
"__CHANGE_ME__"and assert on start-up that it was overridden.[sraise_critical_issue]
backend/main.py (1)
3-5: Absolute import pathapp.*relies onbackend/being onPYTHONPATHRunning the project with
python -m backend.mainworks, but invokinguvicorn backend.main:appfrom repo root will fail becauseappcannot be resolved.
Consider using fully-qualified imports (backend.app…) or makingbackend/a package root viaPYTHONPATH/setup.cfg.
…, .dockerignore, and environment setup
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
DEPLOYMENT.md (3)
28-28: Remove trailing punctuation from headings
To comply with markdownlint’s MD026 rule, drop the trailing colons in your section headers:- ### How to Set Environment Variables in Railway: + ### How to Set Environment Variables in Railway - ### Example of Setting Variables in Railway: + ### Example of Setting Variables in Railway - ### Common Issues: + ### Common Issues - ### Logs: + ### LogsAlso applies to: 50-50, 96-96, 102-102
34-38: Specify language for fenced code blocks
Add a language identifier to your triple-backtick blocks—e.g., use```envor```bash—to satisfy MD040 and improve readability:- ``` + ```env MONGODB_URL=... ... - ``` + ```Also applies to: 41-48
26-26: Polish minor articles and determiners
For consistency and clarity, insert “the” in these lines:- Instead, set environment variables through Railway's dashboard. + Instead, set the environment variables through the Railway dashboard. - In Railway dashboard, go to Variables tab + In the Railway dashboard, go to the Variables tabAlso applies to: 73-74
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.dockerignore(1 hunks)DEPLOYMENT.md(1 hunks)Dockerfile(1 hunks)backend/.env.example(1 hunks)nixpacks.toml(1 hunks)railway.toml(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- .dockerignore
- railway.toml
- Dockerfile
- nixpacks.toml
🧰 Additional context used
🪛 LanguageTool
DEPLOYMENT.md
[uncategorized] ~26-~26: You might be missing the article “the” here.
Context: ...tead, set environment variables through Railway's dashboard. ### How to Set Environmen...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~73-~73: You might be missing the article “the” here.
Context: ...nfigure Environment Variables** - In Railway dashboard, go to Variables tab - Add...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~73-~73: You might be missing the article “the” here.
Context: ...bles** - In Railway dashboard, go to Variables tab - Add all required environment v...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~74-~74: You might be missing the article “the” here.
Context: ...board, go to Variables tab - Add all required environment variables listed above 4. ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~78-~78: A period might be missing here.
Context: ...loy - Monitor the build logs for any issues ## Build Process The build process wi...
(AI_EN_LECTOR_MISSING_PUNCTUATION_PERIOD)
🪛 markdownlint-cli2 (0.17.2)
DEPLOYMENT.md
28-28: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
34-34: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
41-41: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
50-50: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
96-96: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
102-102: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🪛 dotenv-linter (3.3.0)
backend/.env.example
[warning] 4-4: [UnorderedKey] The DATABASE_NAME key should go before the DEBUG key
[warning] 6-6: [UnorderedKey] The ALGORITHM key should go before the DATABASE_NAME key
[warning] 7-7: [UnorderedKey] The ACCESS_TOKEN_EXPIRE_MINUTES key should go before the ALGORITHM key
[warning] 8-8: [UnorderedKey] The REFRESH_TOKEN_EXPIRE_DAYS key should go before the SECRET_KEY key
🔇 Additional comments (1)
backend/.env.example (1)
1-16: Environment variables template is well structured
The file cleanly groups settings for debugging, MongoDB, JWT, Firebase, and CORS. Ensure that in production you overrideSECRET_KEYwith a strong, unpredictable value and confirm that the referencedfirebase-service-account.jsonpath is excluded in.gitignoreto avoid accidental commits.
…y.toml for backend directory structure, and add requirements.txt
… configuration to disable venv creation
…ion and execution commands
… support and improve CORS configuration for production
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
backend/app/auth/service.py (2)
288-306: User lookup still fails due to_idtype mismatchSame issue flagged earlier: Mongo stores
_idasObjectId; JWTsubis a string. Convert before querying.-from app.auth.security import verify_token +from app.auth.security import verify_token +from bson import ObjectId ... -user = await db.users.find_one({"_id": user_id}) +user = await db.users.find_one({"_id": ObjectId(user_id)})
336-339: Logging sensitive reset tokens – remove before productionPlaintext reset tokens in logs are credentials. Gate behind
settings.debugor delete.
🧹 Nitpick comments (8)
backend/app/config.py (3)
1-1: Remove unused import
osis imported but never used.-import os
30-34: Minor formatting / lintThe comment
# Appis indented two spaces deeper than the surrounding code – makes the block look like part of the previous attribute. Align it with the others for readability.
36-38: Pydantic-v2 style config
pydantic_settings.BaseSettings(v2) expectsmodel_config = SettingsConfigDict(env_file=".env")instead of an innerConfigclass. The current syntax still works with pydantic-v1 but will be ignored in v2. Confirm the version you pin inrequirements.txt; update if you’re on v2.nixpacks.toml (1)
16-21: Copying backend twice is redundantAfter changing the install path you can drop the
cp -r backend/* .or, if you keep it, remember that relative imports expecting thebackendpackage path will break. Decide on one layout (run frombackend/or from repo root) and keep the phases consistent.DEPLOYMENT.md (1)
34-34: Add language identifiers to fenced code blocksMarkdown renderers provide syntax highlighting and copy-button support when a language is specified:
-``` +```bashApply to all fenced blocks without a language tag.
Also applies to: 41-41, 52-52, 66-66
backend/main.py (1)
1-1: Unused import
HTTPExceptionis not referenced in this module.-from fastapi import FastAPI, HTTPException +from fastapi import FastAPIbackend/app/auth/service.py (2)
2-14: Prune unused imports
Optional,UserResponse, andjsonare never referenced. Clean them up to reduce noise.-from typing import Optional, Dict, Any +from typing import Dict, Any ... -from app.auth.schemas import UserResponse -import json
114-118: Preserve original traceback when re-raisingUse
raise … from eto keep the exception context:- except DuplicateKeyError: + except DuplicateKeyError as e: ... - except firebase_auth.InvalidIdTokenError: + except firebase_auth.InvalidIdTokenError as e:Also applies to: 220-228
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
DEPLOYMENT.md(1 hunks)backend/app/auth/service.py(1 hunks)backend/app/config.py(1 hunks)backend/main.py(1 hunks)nixpacks.toml(1 hunks)railway.toml(1 hunks)requirements.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- requirements.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- railway.toml
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/app/auth/service.py
2-2: typing.Optional imported but unused
Remove unused import: typing.Optional
(F401)
8-8: app.auth.schemas.UserResponse imported but unused
Remove unused import: app.auth.schemas.UserResponse
(F401)
13-13: json imported but unused
Remove unused import: json
(F401)
115-118: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
220-223: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
225-228: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/app/config.py
1-1: os imported but unused
Remove unused import: os
(F401)
backend/main.py
1-1: fastapi.HTTPException imported but unused
Remove unused import: fastapi.HTTPException
(F401)
🪛 LanguageTool
DEPLOYMENT.md
[uncategorized] ~100-~100: A period might be missing here.
Context: ...loy - Monitor the build logs for any issues ## Build Process The build process wi...
(AI_EN_LECTOR_MISSING_PUNCTUATION_PERIOD)
🪛 markdownlint-cli2 (0.17.2)
DEPLOYMENT.md
28-28: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
34-34: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
41-41: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
52-52: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
66-66: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
72-72: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
118-118: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
124-124: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🪛 Pylint (3.3.7)
backend/app/config.py
[refactor] 36-36: Too few public methods (0/2)
(R0903)
[refactor] 5-5: Too few public methods (0/2)
(R0903)
| [phases.install] | ||
| cmds = [ | ||
| "python -m pip install --upgrade pip", | ||
| "pip install -r requirements.txt" | ||
| ] |
There was a problem hiding this comment.
pip install -r requirements.txt will fail – file lives in backend/
During the install phase the working directory is project root, but requirements.txt is inside backend/. The build will error out with “No such file”.
- "pip install -r requirements.txt"
+ "pip install -r backend/requirements.txt"📝 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.
| [phases.install] | |
| cmds = [ | |
| "python -m pip install --upgrade pip", | |
| "pip install -r requirements.txt" | |
| ] | |
| [phases.install] | |
| cmds = [ | |
| "python -m pip install --upgrade pip", | |
| "pip install -r backend/requirements.txt" | |
| ] |
🤖 Prompt for AI Agents
In nixpacks.toml around lines 5 to 9, the install phase runs "pip install -r
requirements.txt" from the project root, but the requirements.txt file is
located inside the backend/ directory. To fix this, update the command to
reference the correct path by changing it to "pip install -r
backend/requirements.txt" so the file can be found and installed properly.
| allowed_origins = [] | ||
| if settings.allowed_origins: | ||
| allowed_origins = [origin.strip() for origin in settings.allowed_origins.split(",") if origin.strip()] | ||
| else: | ||
| # Fallback to allow all origins if not specified (not recommended for production) | ||
| allowed_origins = ["*"] | ||
|
|
||
| app.add_middleware( | ||
| CORSMiddleware, | ||
| allow_origins=allowed_origins, | ||
| allow_credentials=True, | ||
| allow_methods=["GET", "POST", "PUT", "DELETE", "OPTIONS", "HEAD", "PATCH"], |
There was a problem hiding this comment.
Wildcard CORS with credentials = True will crash on startup
When allowed_origins falls back to "*", Starlette raises a ValueError if allow_credentials is True. Either:
- Disable credentials in the wildcard branch, or
- Require explicit origins in production.
-if settings.allowed_origins:
- allowed_origins = [...]
-else:
- allowed_origins = ["*"]
+if settings.allowed_origins:
+ allowed_origins = [...]
+else:
+ allowed_origins = ["*"]
+ allow_credentials_flag = False
...
- allow_credentials=True,
+ allow_credentials=allow_credentials_flag,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/main.py around lines 16 to 27, the code sets allowed_origins to ["*"]
when no origins are specified, but this causes a ValueError on startup because
allow_credentials is True with a wildcard origin. To fix this, modify the code
so that if allowed_origins is ["*"], set allow_credentials to False in the
middleware configuration, or alternatively enforce that allowed_origins must be
explicitly set in production to avoid using the wildcard with credentials
enabled.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/run-tests.yml (3)
12-12: Remove trailing whitespace
YAML lint reports trailing spaces on these lines. Eliminating them will satisfy linting and prevent silent parsing issues.Apply this diff:
@@ -11,2 +11,2 @@ - runs-on: ubuntu-latest␣ + runs-on: ubuntu-latest @@ -15,2 +15,2 @@ - - uses: actions/checkout@v4␣ + - uses: actions/checkout@v4 @@ -20,2 +20,2 @@ - with:␣ + with: @@ -26,2 +26,2 @@ - pip install -r requirements.txt␣ + pip install -r requirements.txtAlso applies to: 15-15, 20-20, 26-26
16-20: Enable pip caching for faster installs
Leverage the built-incache: 'pip'option inactions/setup-python@v5to avoid reinstalling dependencies on every run.- name: Set up Python uses: actions/setup-python@v5 with: - python-version: '3.12' + python-version: '3.12' + cache: 'pip'
28-32: Remove unnecessarycd
The runner’s default directory is already$GITHUB_WORKSPACE. You can dropcd $GITHUB_WORKSPACEto streamline the script.- name: Run tests run: | - cd $GITHUB_WORKSPACE export PYTHONPATH=$GITHUB_WORKSPACE:$GITHUB_WORKSPACE/backend pytest --cov=./backend --cov-report=xml:coverage.xml backend/tests/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/run-tests.yml(1 hunks)backend/requirements.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/requirements.txt
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/run-tests.yml
[error] 12-12: trailing spaces
(trailing-spaces)
[error] 15-15: trailing spaces
(trailing-spaces)
[error] 20-20: trailing spaces
(trailing-spaces)
[error] 26-26: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
.github/workflows/run-tests.yml (2)
3-8: Verify workflow trigger configuration
The workflow runs on PRs forfeature/*,main, andmaster, but onpushonly formainandmaster. If you intend to execute CI on direct pushes to feature branches, consider addingfeature/*underpush.branches.
34-39: Confirm Codecov action version
You’re usingcodecov/codecov-action@v4, which is a major version bump. Ensure it’s stable for your setup or pin to the latest stablev3.xrelease if you encounter upload failures.
…plitwiser into feature/auth-service-workflow
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/app/(auth)/login.tsx (4)
21-24: Consider strengthening password requirements.The current minimum password length of 6 characters may be insufficient for security. Modern security practices typically recommend at least 8-12 characters with complexity requirements.
- password: z.string().min(6, 'Password must be at least 6 characters'), + password: z.string().min(8, 'Password must be at least 8 characters'),
47-58: Enhance token validation in auth check.The current auth check only verifies token existence. Consider validating token expiry or making a lightweight API call to verify token validity.
const checkAuth = async () => { try { const token = await AsyncStorage.getItem('access_token'); if (token) { + // Optional: Validate token expiry or make API call to verify router.replace('/'); } } catch (error) { console.error('Error checking auth:', error); } finally { setIsCheckingAuth(false); } };
54-54: Replace console.error with proper logging in production.Console statements should be replaced with a proper logging solution for production builds to avoid exposing sensitive information and improve performance.
Consider using a logging library or conditional logging:
- console.error('Error checking auth:', error); + if (__DEV__) { + console.error('Error checking auth:', error); + }- console.error('Login error:', error.response?.data || error.message); + if (__DEV__) { + console.error('Login error:', error.response?.data || error.message); + }Also applies to: 78-78
112-120: Add accessibility properties to form inputs.The form inputs are missing accessibility properties that would improve usability for users with disabilities.
<TextInput style={styles.textInput} placeholder="Email" keyboardType="email-address" autoCapitalize="none" autoComplete="email" + accessibilityLabel="Email address" + accessibilityHint="Enter your email address" onChangeText={onChange} value={value} /><TextInput style={styles.textInput} placeholder="Password" secureTextEntry autoComplete="password" + accessibilityLabel="Password" + accessibilityHint="Enter your password" onChangeText={onChange} value={value} />Also applies to: 134-140
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/(auth)/components/GoogleSignInButton.tsx(1 hunks)frontend/app/(auth)/login.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/(auth)/components/GoogleSignInButton.tsx
🔇 Additional comments (3)
frontend/app/(auth)/login.tsx (3)
60-92: LGTM! Robust error handling and token management.The form submission logic is well-implemented with:
- Proper loading state management
- Comprehensive error handling for different HTTP status codes
- Secure token storage in AsyncStorage
- Appropriate navigation flow
The implementation correctly handles both authentication errors (401) and general API errors.
152-162: Good loading state implementation.The button properly shows loading state with disabled interaction and visual feedback through the ActivityIndicator. This provides excellent user experience during form submission.
187-243: Well-structured styles with TypeScript integration.The StyleSheet implementation is excellent with:
- Proper TypeScript type definitions for styles
- Consistent spacing and color scheme
- Responsive design considerations
- Clear separation of style concerns
frontend/app/(auth)/login.tsx
Outdated
|
|
||
| type LoginFormData = z.infer<typeof loginSchema>; | ||
|
|
||
| const API_URL = 'https://splitwiser-production.up.railway.app'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract API URL to environment configuration.
The hardcoded production API URL should be externalized to support different environments (development, staging, production) and make the app more maintainable.
Consider using environment variables or a configuration file:
-const API_URL = 'https://splitwiser-production.up.railway.app';
+const API_URL = process.env.EXPO_PUBLIC_API_URL || 'https://splitwiser-production.up.railway.app';Then set the environment variable in your .env file:
EXPO_PUBLIC_API_URL=http://localhost:8000
📝 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.
| const API_URL = 'https://splitwiser-production.up.railway.app'; | |
| // frontend/app/(auth)/login.tsx — line 28 | |
| const API_URL = process.env.EXPO_PUBLIC_API_URL || 'https://splitwiser-production.up.railway.app'; |
🤖 Prompt for AI Agents
In frontend/app/(auth)/login.tsx at line 28, the API URL is hardcoded which
reduces flexibility across environments. Replace the hardcoded URL with a
reference to an environment variable, such as process.env.EXPO_PUBLIC_API_URL,
and ensure this variable is defined in your .env file for different environments
like development and production.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/app/config.py (1)
17-21: Fix duplicate field declarations that override defaults.The
firebase_project_idfield is declared twice (lines 17 and 21), causing the second declaration to silently override the first. This means the default value becomesNoneinstead of an empty string, which will break Firebase initialization.- # Firebase - firebase_project_id: str = "" - firebase_service_account_path: str = "./firebase-service-account.json" - # Firebase service account credentials as environment variables - firebase_type: Optional[str] = None - firebase_project_id: Optional[str] = None + # Firebase + firebase_service_account_path: str = "./firebase-service-account.json" + # Firebase service account credentials as environment variables + firebase_type: Optional[str] = None + firebase_project_id: Optional[str] = Nonebackend/main.py (1)
18-34: Fix CORS wildcard with credentials issue.When
allowed_originsis set to["*"](lines 20, 27) butallow_credentials=True(line 34), FastAPI will raise aValueErroron startup. Wildcard origins cannot be used with credentials enabled.+# Determine if we should allow credentials based on origins +allow_credentials_flag = True if settings.allow_all_origins: # Allow all origins in development mode allowed_origins = ["*"] + allow_credentials_flag = False # Cannot use credentials with wildcard print("Development mode: CORS configured to allow all origins") elif settings.allowed_origins: # Use specified origins in production mode allowed_origins = [origin.strip() for origin in settings.allowed_origins.split(",") if origin.strip()] else: # Fallback to allow all origins if not specified (not recommended for production) allowed_origins = ["*"] + allow_credentials_flag = False # Cannot use credentials with wildcard print(f"Allowed CORS origins: {allowed_origins}") app.add_middleware( CORSMiddleware, allow_origins=allowed_origins, - allow_credentials=True, + allow_credentials=allow_credentials_flag,
🧹 Nitpick comments (6)
backend/.env.example (2)
5-5: Enhance security of the JWT secret key placeholder.The current placeholder secret key could potentially be left unchanged in development environments, creating a security risk. Consider using a more obviously unsafe placeholder.
-SECRET_KEY=your-super-secret-jwt-key-change-this-in-production +SECRET_KEY=CHANGE-THIS-INSECURE-SECRET-KEY-BEFORE-USE
7-8: Consider documenting token expiration rationale.The current token lifetimes (15 minutes for access, 30 days for refresh) are reasonable but could benefit from inline comments explaining the security trade-offs.
-ACCESS_TOKEN_EXPIRE_MINUTES=15 -REFRESH_TOKEN_EXPIRE_DAYS=30 +ACCESS_TOKEN_EXPIRE_MINUTES=15 # Short-lived for security +REFRESH_TOKEN_EXPIRE_DAYS=30 # Balance between security and UXbackend/app/config.py (1)
1-1: Remove unused import.The
osmodule is imported but never used in this file.-import osbackend/main.py (3)
1-1: Remove unused import.
HTTPExceptionis imported but never used in this file.-from fastapi import FastAPI, HTTPException, Request +from fastapi import FastAPI, Request
52-75: Consider removing redundant OPTIONS handler.The manual OPTIONS handler duplicates functionality already provided by the CORS middleware. This can lead to inconsistent behavior and adds unnecessary complexity.
The CORS middleware already handles OPTIONS requests automatically. Unless there's a specific need for custom OPTIONS handling, consider removing this handler:
-# Add a catch-all OPTIONS handler that should work for any path -@app.options("/{path:path}") -async def options_handler(request: Request, path: str): - """Handle all OPTIONS requests""" - print(f"OPTIONS request received for path: /{path}") - print(f"Origin: {request.headers.get('origin', 'No origin header')}") - - response = Response(status_code=200) - - # Manually set CORS headers for debugging - origin = request.headers.get("origin") - if origin and (origin in allowed_origins or "*" in allowed_origins): - response.headers["Access-Control-Allow-Origin"] = origin - response.headers["Access-Control-Allow-Methods"] = "GET, POST, PUT, DELETE, OPTIONS, HEAD, PATCH" - response.headers["Access-Control-Allow-Headers"] = "Accept, Accept-Language, Content-Language, Content-Type, Authorization, X-Requested-With, Origin, Cache-Control, Pragma, X-CSRFToken" - response.headers["Access-Control-Allow-Credentials"] = "true" - response.headers["Access-Control-Max-Age"] = "3600" - elif "*" in allowed_origins: - response.headers["Access-Control-Allow-Origin"] = "*" - response.headers["Access-Control-Allow-Methods"] = "GET, POST, PUT, DELETE, OPTIONS, HEAD, PATCH" - response.headers["Access-Control-Allow-Headers"] = "Accept, Accept-Language, Content-Language, Content-Type, Authorization, X-Requested-With, Origin, Cache-Control, Pragma, X-CSRFToken" - response.headers["Access-Control-Max-Age"] = "3600" - - return response
78-90: Consider using FastAPI's lifespan context manager.The
@app.on_event("startup")and@app.on_event("shutdown")decorators are deprecated in favor of the lifespan context manager for better resource management.+from contextlib import asynccontextmanager +@asynccontextmanager +async def lifespan(app: FastAPI): + # Startup + await connect_to_mongo() + yield + # Shutdown + await close_mongo_connection() app = FastAPI( title="Splitwiser API", description="Backend API for Splitwiser expense tracking application", version="1.0.0", docs_url="/docs", - redoc_url="/redoc" + redoc_url="/redoc", + lifespan=lifespan ) -# Database events -@app.on_event("startup") -async def startup_event(): - """ - Initializes the MongoDB connection when the application starts. - """ - await connect_to_mongo() -@app.on_event("shutdown") -async def shutdown_event(): - """ - Closes the MongoDB connection when the application shuts down. - """ - await close_mongo_connection()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/.env.example(1 hunks)backend/app/config.py(1 hunks)backend/main.py(1 hunks)
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
backend/.env.example
[warning] 4-4: [UnorderedKey] The DATABASE_NAME key should go before the DEBUG key
[warning] 6-6: [UnorderedKey] The ALGORITHM key should go before the DATABASE_NAME key
[warning] 7-7: [UnorderedKey] The ACCESS_TOKEN_EXPIRE_MINUTES key should go before the ALGORITHM key
[warning] 8-8: [UnorderedKey] The REFRESH_TOKEN_EXPIRE_DAYS key should go before the SECRET_KEY key
🪛 Ruff (0.11.9)
backend/app/config.py
1-1: os imported but unused
Remove unused import: os
(F401)
backend/main.py
1-1: fastapi.HTTPException imported but unused
Remove unused import: fastapi.HTTPException
(F401)
🪛 Pylint (3.3.7)
backend/app/config.py
[refactor] 36-36: Too few public methods (0/2)
(R0903)
[refactor] 5-5: Too few public methods (0/2)
(R0903)
…plitwiser into feature/auth-service-workflow
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/app/(auth)/components/GoogleSignInButton.tsx (1)
16-18: Hard-coded Google client ID should be externalized.The Google client ID is still hard-coded, which was previously flagged as a security and flexibility concern. This should be moved to environment variables or runtime configuration.
🧹 Nitpick comments (2)
frontend/app/(auth)/components/GoogleSignInButton.tsx (2)
12-12: Remove unused API_URL constant.The
API_URLvariable is defined but never used in this component, making it dead code that should be removed.-const API_URL = 'https://splitwiser-production.up.railway.app';
48-51: Use local asset for Google icon instead of remote URL.Loading the Google icon from a remote URL creates a dependency on external resources and could fail due to network issues or URL changes. Consider bundling the Google icon as a local asset for better reliability and performance.
- <Image - source={{ uri: 'https://www.gstatic.com/firebasejs/ui/2.0.0/images/auth/google.svg' }} - style={styles.googleIcon} - /> + <Image + source={require('../../../assets/icons/google-icon.png')} + style={styles.googleIcon} + />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
frontend/app.json(1 hunks)frontend/app/(auth)/components/GoogleSignInButton.tsx(1 hunks)frontend/config/config.ts(1 hunks)frontend/eas.json(1 hunks)frontend/metro.config.js(1 hunks)frontend/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- frontend/metro.config.js
- frontend/config/config.ts
- frontend/eas.json
- frontend/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app.json
🔇 Additional comments (1)
frontend/app/(auth)/components/GoogleSignInButton.tsx (1)
20-39: Well-implemented authentication flow.The Google authentication implementation follows Firebase best practices with proper error handling, loading state management, and user navigation. The useEffect hook correctly handles the OAuth response and the Firebase credential flow is properly implemented.
* feat: Update frontend package and add authentication screens - Changed project name in package.json to "splitwiser" and updated main entry point. - Updated dependencies to newer versions for better performance and security. - Added HomeScreen component for user profile display and logout functionality. - Created LoginScreen component for user authentication with email/password and Google sign-in options. - Modified tsconfig.json for improved TypeScript support and configuration. * feat: update app configuration and remove deprecated Google authentication code * fix: updates the package.json * feat(auth): enhance login/signup error handling and add basic form validation
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
frontend/package.json (1)
17-17: Consider updating to the latest Expo SDK version.Expo SDK ~49.0.15 appears to be an older version. Using the latest stable version ensures better security, performance, and access to new features.
What is the latest stable version of Expo SDK and are there any breaking changes from version 49.0.15?frontend/screens/HomeScreen.tsx (1)
52-55: Consider removing User ID from profile display.Displaying the user ID in the profile section may not provide value to end users and could potentially expose internal system information. Consider removing this field or moving it to a developer/debug section.
- <View style={styles.infoRow}> - <Text style={styles.infoLabel}>User ID:</Text> - <Text style={styles.infoValue}>{user?.id || 'Not provided'}</Text> - </View>frontend/screens/LoginScreen.tsx (1)
37-42: Consider using a more robust email validation.While the current regex works for basic cases, consider using a more comprehensive email validation library or a more robust regex pattern for better validation coverage.
- const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + const emailRegex = /^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$/;frontend/contexts/AuthContext.tsx (1)
162-172: Consider calling backend logout endpoint to revoke tokens.The current logout only clears local state but doesn't invalidate tokens on the backend, which could be a security risk if tokens are compromised.
Consider implementing a backend logout call:
const logout = async () => { try { // Call backend to revoke tokens if (refreshToken) { await axios.post('/auth/logout', { refresh_token: refreshToken }); } } catch (error) { // Log error but continue with local logout console.error('Backend logout failed:', error); } // Clear auth state setAccessToken(null); setRefreshToken(null); setUser(null); setIsAuthenticated(false); // Clear secure storage try { await SecureStore.deleteItemAsync('refreshToken'); } catch (error) { console.error('Failed to clear refresh token from secure storage:', error); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
frontend/App.tsx(1 hunks)frontend/README.md(1 hunks)frontend/app.json(1 hunks)frontend/contexts/AuthContext.tsx(1 hunks)frontend/package.json(1 hunks)frontend/screens/HomeScreen.tsx(1 hunks)frontend/screens/LoginScreen.tsx(1 hunks)frontend/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/app.json
- frontend/README.md
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/contexts/AuthContext.tsx
[error] 136-136: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the catch clause.
(lint/complexity/noUselessCatch)
[error] 156-156: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the catch clause.
(lint/complexity/noUselessCatch)
🔇 Additional comments (6)
frontend/package.json (1)
16-16: Verify axios version for security vulnerabilities.The axios version (^1.6.2) may contain security vulnerabilities. Please ensure you're using the latest secure version.
What are the latest security advisories for axios version 1.6.2 and what is the current stable version?frontend/App.tsx (1)
1-42: Clean and well-structured app architecture.The implementation follows React Navigation best practices with proper authentication flow handling. The conditional navigation based on authentication status is correctly implemented, and the provider hierarchy is appropriate.
frontend/tsconfig.json (1)
1-14: Well-configured TypeScript setup.The TypeScript configuration is appropriate for a React Native Expo project with strict type checking enabled and proper compiler options for development.
frontend/screens/HomeScreen.tsx (1)
17-23: Good error handling implementation.The logout error handling is properly implemented with try-catch and user feedback via Alert.
frontend/screens/LoginScreen.tsx (1)
54-70: Excellent error handling implementation.The error message extraction logic handles multiple error response structures effectively, providing good user feedback for authentication failures.
frontend/contexts/AuthContext.tsx (1)
163-172: Await the SecureStore deletion to prevent race conditions.The function is async but doesn't await the secure storage operation, which could lead to race conditions.
const logout = async () => { // Clear auth state setAccessToken(null); setRefreshToken(null); setUser(null); setIsAuthenticated(false); // Clear secure storage - await SecureStore.deleteItemAsync('refreshToken'); + try { + await SecureStore.deleteItemAsync('refreshToken'); + } catch (error) { + // Log error but don't throw - logout should always succeed + console.error('Failed to clear refresh token from secure storage:', error); + } };Likely an incorrect or invalid review comment.
| // Define the shape of our authentication context | ||
| type AuthContextType = { | ||
| isAuthenticated: boolean; | ||
| user: any; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define a proper User interface instead of using any.
Using any type removes TypeScript's type safety benefits. Define a proper interface for the user object to improve type safety and developer experience.
Add a User interface definition:
interface User {
id: string;
email: string;
name: string;
// Add other user properties as needed
}Then update the type definition:
- user: any;
+ user: User | null;🤖 Prompt for AI Agents
In frontend/contexts/AuthContext.tsx at line 10, replace the use of 'any' for
the user type with a properly defined User interface. Define a User interface
including properties like id, email, and name, then update the user variable's
type to this User interface to enhance type safety and developer experience.
| import * as SecureStore from 'expo-secure-store'; | ||
| import React, { createContext, useContext, useEffect, useState } from 'react'; | ||
|
|
||
| const API_URL = 'https://splitwiser-production.up.railway.app'; // Replace with your actual backend URL |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use environment variables for API URL configuration.
Hardcoding the production URL makes it difficult to develop locally or deploy to different environments. Consider using environment variables instead.
-const API_URL = 'https://splitwiser-production.up.railway.app'; // Replace with your actual backend URL
+const API_URL = process.env.EXPO_PUBLIC_API_URL || 'http://localhost:8000';For Expo/React Native, you can set this in your .env file:
EXPO_PUBLIC_API_URL=https://splitwiser-production.up.railway.app
📝 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.
| const API_URL = 'https://splitwiser-production.up.railway.app'; // Replace with your actual backend URL | |
| // frontend/contexts/AuthContext.tsx | |
| - const API_URL = 'https://splitwiser-production.up.railway.app'; // Replace with your actual backend URL | |
| + const API_URL = process.env.EXPO_PUBLIC_API_URL || 'http://localhost:8000'; |
🤖 Prompt for AI Agents
In frontend/contexts/AuthContext.tsx at line 5, the API URL is hardcoded, which
reduces flexibility for different environments. Replace the hardcoded URL with a
reference to an environment variable, such as process.env.EXPO_PUBLIC_API_URL,
and ensure this variable is set in your .env file for local development and
production deployment.
…r and update access token creation
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/app/dependencies.py (1)
50-55: Fix exception handling: remove unused variable and preserve traceback.The exception variable
eis unused and the original traceback is not preserved.- except Exception as e: + except Exception: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Could not validate credentials", headers={"WWW-Authenticate": "Bearer"}, - ) + ) from Nonebackend/app/auth/routes.py (1)
52-56: Fix exception handling throughout the file.Multiple exception handlers capture exceptions but don't preserve the traceback. Additionally, line 186 has an unused variable.
Apply this pattern to all exception handlers:
- except Exception as e: + except Exception as exc: raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=f"Operation failed: {str(e)}" - ) + detail="Internal server error" + ) from excFor line 186 specifically, remove the unused variable:
- except Exception as e: + except Exception: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid or expired token" - ) + ) from NoneNote: Consider avoiding exposure of internal error details in client-facing messages for security reasons.
Also applies to: 87-91, 119-123, 163-167, 186-190, 206-210, 237-241
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/auth/routes.py(1 hunks)backend/app/config.py(1 hunks)backend/app/dependencies.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/app/auth/routes.py
1-1: fastapi.Depends imported but unused
Remove unused import: fastapi.Depends
(F401)
6-6: app.auth.schemas.ErrorResponse imported but unused
Remove unused import: app.auth.schemas.ErrorResponse
(F401)
53-56: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
88-91: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
120-123: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
164-167: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
186-186: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
187-190: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
207-210: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
238-241: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/app/config.py
1-1: os imported but unused
Remove unused import: os
(F401)
backend/app/dependencies.py
10-10: 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)
50-50: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
51-55: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🪛 Pylint (3.3.7)
backend/app/config.py
[refactor] 36-36: Too few public methods (0/2)
(R0903)
[refactor] 5-5: Too few public methods (0/2)
(R0903)
Docstrings generation was requested by @vrajpatelll. * #3 (comment) The following files were modified: * `backend/app/auth/routes.py` * `backend/app/auth/security.py` * `backend/app/auth/service.py` * `backend/app/database.py` * `backend/app/dependencies.py` * `backend/generate_secret.py` * `backend/main.py` * `backend/tests/auth/test_auth_routes.py` * `backend/tests/conftest.py` * `frontend/App.tsx`
|
Note Generated docstrings for this pull request at #10 |
…irmation to incorporate review chnages
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
frontend/screens/LoginScreen.tsx (1)
74-86: Good implementation of signup validation!The password confirmation check and name validation properly address the requirements for the signup flow. The error messages are clear and user-friendly.
frontend/contexts/AuthContext.tsx (2)
5-5: Use environment variables for API URL configuration.Hardcoding the production URL makes it difficult to develop locally or deploy to different environments.
-const API_URL = 'https://splitwiser-production.up.railway.app'; // Replace with your actual backend URL +const API_URL = process.env.EXPO_PUBLIC_API_URL || 'http://localhost:8000';
10-10: Define a proper User interface instead of usingany.Using
anytype removes TypeScript's type safety benefits.Add a User interface definition at the top of the file:
interface User { id: string; email: string; name: string; // Add other user properties as needed }Then update the type:
- user: any; + user: User | null;
🧹 Nitpick comments (2)
frontend/screens/LoginScreen.tsx (1)
143-149: Fix indentation for the password input field.The TextInput for password has incorrect indentation which affects code readability.
- <TextInput + <TextInput style={styles.input} placeholder="Password" value={password} onChangeText={setPassword} secureTextEntry />frontend/contexts/AuthContext.tsx (1)
151-151: Remove unnecessary dependencies from useEffect.The setter functions (setAccessToken, setRefreshToken, etc.) and logout are stable references that don't need to be in the dependencies array. Including them can cause unnecessary re-renders.
- }, [accessToken, refreshToken, logout, setAccessToken, setRefreshToken, setUser, setIsAuthenticated, setLoading]); + }, [accessToken, refreshToken]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/contexts/AuthContext.tsx(1 hunks)frontend/screens/LoginScreen.tsx(1 hunks)
🔇 Additional comments (2)
frontend/screens/LoginScreen.tsx (1)
27-50: Excellent password validation implementation!The password validation function comprehensively checks for length, character types, and provides clear error messages. This properly addresses security requirements.
frontend/contexts/AuthContext.tsx (1)
153-173: Well-structured authentication functions!The login and signup functions properly use the shared
handleAuthSuccesshelper, manage loading state correctly, and allow errors to propagate naturally without unnecessary catch blocks.
…and confirmation to incorporate review chnages" This reverts commit 7883d98.
3 participants
Introduce a new authentication module that supports JWT for secure user authentication and integrates with Firebase for additional functionality. This update includes necessary configurations, environment settings, and models for user management.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores