feat: implement JWT authentication and user registration system#302
feat: implement JWT authentication and user registration system#302shivansh023023 wants to merge 5 commits intoAOSSIE-Org:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds JWT-based authentication: new pydantic Settings, hashed password field on User, password hashing and JWT creation, get_current_user dependency to validate tokens, auth routes (signup, login, me), and requirements updates for crypto and settings. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthRouter as Auth Router (/auth)
participant DB as Database
participant AuthService as Auth Service
Client->>AuthRouter: POST /auth/login (email, password)
AuthRouter->>DB: query user by email
DB-->>AuthRouter: user row (hashed_password)
AuthRouter->>AuthService: verify_password(plain, hashed)
AuthService-->>AuthRouter: valid / invalid
alt valid
AuthRouter->>AuthService: create_access_token({ "sub": user.id })
AuthService-->>AuthRouter: access_token (JWT)
AuthRouter-->>Client: 200 { access_token, token_type }
else invalid
AuthRouter-->>Client: 401 Unauthorized
end
sequenceDiagram
participant Client
participant AuthRouter as Auth Router (/auth)
participant GetCurrent as get_current_user
participant DB as Database
participant AuthService as Auth Service
Client->>AuthRouter: GET /auth/me (Authorization: Bearer <token>)
AuthRouter->>GetCurrent: depends(token)
GetCurrent->>AuthService: decode JWT using SECRET_KEY/ALGORITHM
AuthService-->>GetCurrent: payload or error
alt payload valid
GetCurrent->>DB: query user by id (payload.sub)
DB-->>GetCurrent: user row / none
alt user found
GetCurrent-->>AuthRouter: User
AuthRouter-->>Client: 200 { id, username, email, role }
else not found
GetCurrent-->>AuthRouter: raise 401
AuthRouter-->>Client: 401 Unauthorized
end
else invalid token
GetCurrent-->>AuthRouter: raise 401
AuthRouter-->>Client: 401 Unauthorized
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Backend/requirements.txt (1)
34-34:⚠️ Potential issue | 🟠 MajorRemove unused
PyJWTdependency (line 34).
PyJWTis listed in requirements.txt but is completely unused in the codebase. The application exclusively usespython-josefor all JWT operations (imported inBackend/app/services/auth_service.pyandBackend/app/services/get_current_user.py). RemovingPyJWTeliminates unnecessary clutter, reduces the attack surface, and avoids confusion about which JWT library is in use.Also applies to: 57-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend/requirements.txt` at line 34, Remove the unused PyJWT dependency from requirements.txt: delete any lines specifying "PyJWT==..." (including the duplicate occurrence indicated) because the codebase uses python-jose for JWT handling (see auth_service.py and get_current_user.py); ensure no other references to PyJWT remain in the repo and run dependency install/tests to verify nothing breaks after removing PyJWT.
🧹 Nitpick comments (3)
Backend/app/services/get_current_user.py (1)
28-29: Re-raise with exception chaining to preserve the original traceback (Ruff B904).Without chaining, the original
JWTErroris silently swallowed, making debugging harder and violating PEP 3134.♻️ Proposed fix
except JWTError: - raise credentials_exception + raise credentials_exception from None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend/app/services/get_current_user.py` around lines 28 - 29, In the JWT error handler inside get_current_user, preserve the original traceback by capturing the caught JWTError and re-raising credentials_exception with exception chaining; change the bare "except JWTError:" to "except JWTError as e:" and raise credentials_exception from e so the original JWTError is attached to the new exception.Backend/app/config.py (1)
6-11: Consider using PydanticBaseSettingsfor environment-backed configuration.The plain class approach gives no validation, no type coercion, and no
.envloading feedback.pydantic-settings(already transitively available throughpydantic) provides all of this with minimal changes and is the idiomatic FastAPI approach.♻️ Proposed refactor
-import os -from dotenv import load_dotenv - -load_dotenv() - -class Settings: - SECRET_KEY: str = os.getenv("SECRET_KEY", "your-super-secret-key-change-me") - ALGORITHM: str = "HS256" - ACCESS_TOKEN_EXPIRE_MINUTES: int = 60 * 24 # 24 hours - -settings = Settings() +from pydantic_settings import BaseSettings + +class Settings(BaseSettings): + SECRET_KEY: str + ALGORITHM: str = "HS256" + ACCESS_TOKEN_EXPIRE_MINUTES: int = 60 * 24 + + model_config = {"env_file": ".env"} + +settings = Settings()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend/app/config.py` around lines 6 - 11, Replace the plain Settings class with a pydantic BaseSettings-backed class: make Settings inherit from pydantic.BaseSettings, keep the same attributes (SECRET_KEY, ALGORITHM, ACCESS_TOKEN_EXPIRE_MINUTES) but define defaults via Field() if you want custom env names/coercion, add an inner Config (or model_config) to point to an .env file (env_file=".env") and enable any desired case/validation settings, then instantiate settings = Settings() so values are loaded/validated from the environment and .env automatically; update references to Settings, SECRET_KEY, ALGORITHM, and ACCESS_TOKEN_EXPIRE_MINUTES as needed.Backend/app/routes/auth.py (1)
22-22: Addresponse_modeldeclarations to all three endpoints.Without
response_model, FastAPI serialises the full ORM object or a raw dict with no contract, risking accidental leakage of sensitive fields (e.g.hashed_passwordif the ORM object is ever returned directly) and providing no schema in OpenAPI docs.Define Pydantic response schemas (e.g.
UserResponse,TokenResponse) and annotate each route.Also applies to: 39-39, 53-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend/app/routes/auth.py` at line 22, Add explicit Pydantic response_model annotations to each route decorator (e.g., the `@router.post`("/signup"), the `@router.post`("/login"), and the `@router.post`("/refresh") endpoints) and import/define the corresponding response schemas such as UserResponse and TokenResponse; update the decorators to read like `@router.post`("/signup", response_model=UserResponse) and `@router.post`("/login", response_model=TokenResponse) (and response_model=TokenResponse for refresh), and ensure each endpoint returns data converted to the Pydantic schema (use schema.from_orm(...) or dict projection) rather than returning raw ORM objects so sensitive fields like hashed_password are never exposed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Backend/app/config.py`:
- Line 7: The current Settings.SECRET_KEY uses a public placeholder default
which is unsafe; change Settings to require the env var instead of
defaulting—read SECRET_KEY from os.getenv (or load_dotenv) and if it's missing
or equals the placeholder, raise an exception at startup; additionally validate
minimal entropy/length (e.g., minimum byte/char length) and fail fast so code
that signs JWTs (using SECRET_KEY/ALGORITHM) never runs with a weak key.
In `@Backend/app/models/models.py`:
- Around line 73-75: Change the naive UTC defaults to timezone-aware datetimes
for all DateTime(timezone=True) timestamp columns: replace uses of
datetime.utcnow (or datetime.utcnow()) with datetime.now(timezone.utc) as the
default factory for AudienceInsights.created_at, Sponsorship.created_at,
UserPost.created_at, SponsorshipApplication.applied_at,
Collaboration.created_at, and SponsorshipPayment.transaction_date; ensure you
import timezone from datetime and apply the same change consistently across
those column definitions so the DB receives tz-aware timestamps.
- Line 34: seed_db() is failing because the User model's hashed_password Column
is non-nullable yet seed.py creates User records without setting it; fix by
updating the seeding logic (seed_db()/seed.py) to populate the
User.hashed_password field with a proper hashed value (e.g., use your project's
password utility or something like werkzeug.security.generate_password_hash /
bcrypt.hashpw) before inserting, leaving Column(String, nullable=False) as-is;
alternatively, if you prefer not to hash in seed, change the
models.User.hashed_password Column to include a server_default or make
nullable=True—prefer hashing in seed_db() to keep the model constraint.
In `@Backend/app/routes/auth.py`:
- Around line 22-37: The signup endpoint currently only checks email uniqueness
and can raise an unhandled IntegrityError for duplicate usernames; update the
signup function to explicitly check for existing username by querying
select(User).where(User.username == user_data.username) and raise
HTTPException(status_code=400, detail="Username already registered") before
creating the User, and additionally wrap the db.commit()/db.refresh(new_user) in
a try/except catching sqlalchemy.exc.IntegrityError to convert any remaining
constraint violations into HTTP 400 errors for robustness.
- Around line 13-16: UserCreate currently lacks a role field so all signups
default to "creator"; add a role attribute to the UserCreate Pydantic model
(e.g., role: Optional[Literal["creator","brand"]] = "creator" or a required
Literal if you want explicit choice) and update the signup handler (the function
that consumes UserCreate and creates the User record) to pass this role into
user creation; also validate allowed values (Literal or Enum) to prevent invalid
inputs and adjust any downstream logic or docs if brand signups should be
restricted to a separate flow.
In `@Backend/app/services/auth_service.py`:
- Line 16: Replace the naive UTC timestamp creation for the token expiry (the
line assigning expire in auth_service.py) with a timezone-aware datetime: use
datetime.now(timezone.utc) +
timedelta(minutes=settings.ACCESS_TOKEN_EXPIRE_MINUTES) and update imports to
include timezone from datetime so the expire value is timezone-aware for
JWT/JOSE handling (locate the assignment to expire and the function that builds
the token).
In `@Backend/app/services/get_current_user.py`:
- Line 11: The OAuth2 scheme declaration oauth2_scheme =
OAuth2PasswordBearer(tokenUrl="auth/login") points to /auth/login which accepts
JSON, breaking Swagger's "Authorize"; fix by either (A) changing the /auth/login
endpoint handler to accept an OAuth2PasswordRequestForm (replace JSON UserLogin
input with OAuth2PasswordRequestForm and extract username/password to issue the
token) so Swagger form-based auth works, or (B) if you must keep JSON, replace
the scheme with an HTTPBearer scheme (e.g., use fastapi.security.HTTPBearer) and
document the JSON login contract separately; update any uses of oauth2_scheme
accordingly.
- Line 5: Replace the deprecated import "from sqlalchemy.future import select"
with the SQLAlchemy 2.0 import "from sqlalchemy import select" in the modules
that use it (e.g., get_current_user.py and auth.py); locate the top-level import
statement that references sqlalchemy.future and change it to import select
directly from sqlalchemy so the code uses the 2.0-style API.
In `@Backend/requirements.txt`:
- Line 56: The requirement line "passlib[bcrypt]==1.7.4" can fail at runtime
with bcrypt>=4.1.0; update the dependency list to explicitly pin bcrypt to a
compatible upper bound (e.g., add "bcrypt<4.1.0" or "bcrypt<=4.0.1") alongside
the existing passlib entry so fresh installs won't pull the breaking bcrypt 4.1+
release; modify the requirements file to include that bcrypt constraint near the
existing passlib[bcrypt]==1.7.4 entry.
- Line 57: Upgrade the vulnerable dependency entry
python-jose[cryptography]==3.3.0 to at least python-jose[cryptography]>=3.4.0 in
requirements.txt to remediate CVE-2024-33663 and CVE-2024-33664; after updating,
regenerate any lock files or virtualenv (pip-compile/pip-tools or pip freeze),
run the test suite, and verify JWT usage in your codebase (places calling
jwt.decode) still work and explicitly specify allowed algorithms when decoding
to avoid algorithm-confusion issues.
---
Outside diff comments:
In `@Backend/requirements.txt`:
- Line 34: Remove the unused PyJWT dependency from requirements.txt: delete any
lines specifying "PyJWT==..." (including the duplicate occurrence indicated)
because the codebase uses python-jose for JWT handling (see auth_service.py and
get_current_user.py); ensure no other references to PyJWT remain in the repo and
run dependency install/tests to verify nothing breaks after removing PyJWT.
---
Duplicate comments:
In `@Backend/app/routes/auth.py`:
- Line 3: The import "from sqlalchemy.future import select" is deprecated;
replace it with "from sqlalchemy import select" in this file (aligning with the
same fix applied in get_current_user.py) — update the import statement that
brings in the select symbol and ensure any usages of select in this module
continue to reference the same symbol.
---
Nitpick comments:
In `@Backend/app/config.py`:
- Around line 6-11: Replace the plain Settings class with a pydantic
BaseSettings-backed class: make Settings inherit from pydantic.BaseSettings,
keep the same attributes (SECRET_KEY, ALGORITHM, ACCESS_TOKEN_EXPIRE_MINUTES)
but define defaults via Field() if you want custom env names/coercion, add an
inner Config (or model_config) to point to an .env file (env_file=".env") and
enable any desired case/validation settings, then instantiate settings =
Settings() so values are loaded/validated from the environment and .env
automatically; update references to Settings, SECRET_KEY, ALGORITHM, and
ACCESS_TOKEN_EXPIRE_MINUTES as needed.
In `@Backend/app/routes/auth.py`:
- Line 22: Add explicit Pydantic response_model annotations to each route
decorator (e.g., the `@router.post`("/signup"), the `@router.post`("/login"), and
the `@router.post`("/refresh") endpoints) and import/define the corresponding
response schemas such as UserResponse and TokenResponse; update the decorators
to read like `@router.post`("/signup", response_model=UserResponse) and
`@router.post`("/login", response_model=TokenResponse) (and
response_model=TokenResponse for refresh), and ensure each endpoint returns data
converted to the Pydantic schema (use schema.from_orm(...) or dict projection)
rather than returning raw ORM objects so sensitive fields like hashed_password
are never exposed.
In `@Backend/app/services/get_current_user.py`:
- Around line 28-29: In the JWT error handler inside get_current_user, preserve
the original traceback by capturing the caught JWTError and re-raising
credentials_exception with exception chaining; change the bare "except
JWTError:" to "except JWTError as e:" and raise credentials_exception from e so
the original JWTError is attached to the new exception.
| created_at = Column( | ||
| DateTime(timezone=True), default=lambda: datetime.now(timezone.utc) | ||
| DateTime(timezone=True), default=datetime.utcnow | ||
| ) |
There was a problem hiding this comment.
DateTime(timezone=True) paired with datetime.utcnow produces naive datetimes — use datetime.now(timezone.utc) across all updated timestamp columns.
datetime.utcnow() returns a naive datetime (no tzinfo). Pairing it with DateTime(timezone=True) means the default value inserted into the DB lacks timezone information, which is inconsistent with the declared column type and causes silent data integrity issues on timezone-aware DB backends (e.g. PostgreSQL TIMESTAMPTZ). This affects all the updated DateTime(timezone=True) columns: AudienceInsights.created_at, Sponsorship.created_at, UserPost.created_at, SponsorshipApplication.applied_at, Collaboration.created_at, and SponsorshipPayment.transaction_date.
Additionally, datetime.utcnow is deprecated as of Python 3.12.
♻️ Proposed fix (representative — apply to all 6 columns)
-from datetime import datetime
+from datetime import datetime, timezone
...
- DateTime(timezone=True), default=datetime.utcnow
+ DateTime(timezone=True), default=lambda: datetime.now(timezone.utc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Backend/app/models/models.py` around lines 73 - 75, Change the naive UTC
defaults to timezone-aware datetimes for all DateTime(timezone=True) timestamp
columns: replace uses of datetime.utcnow (or datetime.utcnow()) with
datetime.now(timezone.utc) as the default factory for
AudienceInsights.created_at, Sponsorship.created_at, UserPost.created_at,
SponsorshipApplication.applied_at, Collaboration.created_at, and
SponsorshipPayment.transaction_date; ensure you import timezone from datetime
and apply the same change consistently across those column definitions so the DB
receives tz-aware timestamps.
| class UserCreate(BaseModel): | ||
| username: str | ||
| email: EmailStr | ||
| password: str |
There was a problem hiding this comment.
UserCreate has no role field — all users silently default to "creator", locking out brand signups.
The User model supports "creator" and "brand" roles, but there is no way for a brand to register through this endpoint. If brand-specific signup is intentional (e.g. a separate flow), document it; otherwise add the field.
♻️ Proposed fix
class UserCreate(BaseModel):
username: str
email: EmailStr
password: str
+ role: str = "creator" # 'creator' or 'brand'And in the signup handler:
new_user = User(
username=user_data.username,
email=user_data.email,
hashed_password=hash_password(user_data.password),
+ role=user_data.role,
)📝 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.
| class UserCreate(BaseModel): | |
| username: str | |
| email: EmailStr | |
| password: str | |
| class UserCreate(BaseModel): | |
| username: str | |
| email: EmailStr | |
| password: str | |
| role: str = "creator" # 'creator' or 'brand' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Backend/app/routes/auth.py` around lines 13 - 16, UserCreate currently lacks
a role field so all signups default to "creator"; add a role attribute to the
UserCreate Pydantic model (e.g., role: Optional[Literal["creator","brand"]] =
"creator" or a required Literal if you want explicit choice) and update the
signup handler (the function that consumes UserCreate and creates the User
record) to pass this role into user creation; also validate allowed values
(Literal or Enum) to prevent invalid inputs and adjust any downstream logic or
docs if brand signups should be restricted to a separate flow.
| @router.post("/signup") | ||
| async def signup(user_data: UserCreate, db: AsyncSession = Depends(get_db)): | ||
| # Check if user already exists | ||
| result = await db.execute(select(User).where(User.email == user_data.email)) | ||
| if result.scalars().first(): | ||
| raise HTTPException(status_code=400, detail="Email already registered") | ||
|
|
||
| new_user = User( | ||
| username=user_data.username, | ||
| email=user_data.email, | ||
| hashed_password=hash_password(user_data.password) | ||
| ) | ||
| db.add(new_user) | ||
| await db.commit() | ||
| await db.refresh(new_user) | ||
| return {"message": "User created successfully"} |
There was a problem hiding this comment.
Signup silently raises a 500 on duplicate username — check uniqueness before inserting.
The endpoint checks for duplicate email (line 25) but not username. Since username is declared unique=True in the User model, submitting a duplicate username will surface as an unhandled IntegrityError (HTTP 500) rather than a clean HTTP 400.
🐛 Proposed fix
`@router.post`("/signup")
async def signup(user_data: UserCreate, db: AsyncSession = Depends(get_db)):
# Check if user already exists
result = await db.execute(select(User).where(User.email == user_data.email))
if result.scalars().first():
raise HTTPException(status_code=400, detail="Email already registered")
+ result = await db.execute(select(User).where(User.username == user_data.username))
+ if result.scalars().first():
+ raise HTTPException(status_code=400, detail="Username already taken")
+
new_user = User(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @router.post("/signup") | |
| async def signup(user_data: UserCreate, db: AsyncSession = Depends(get_db)): | |
| # Check if user already exists | |
| result = await db.execute(select(User).where(User.email == user_data.email)) | |
| if result.scalars().first(): | |
| raise HTTPException(status_code=400, detail="Email already registered") | |
| new_user = User( | |
| username=user_data.username, | |
| email=user_data.email, | |
| hashed_password=hash_password(user_data.password) | |
| ) | |
| db.add(new_user) | |
| await db.commit() | |
| await db.refresh(new_user) | |
| return {"message": "User created successfully"} | |
| `@router.post`("/signup") | |
| async def signup(user_data: UserCreate, db: AsyncSession = Depends(get_db)): | |
| # Check if user already exists | |
| result = await db.execute(select(User).where(User.email == user_data.email)) | |
| if result.scalars().first(): | |
| raise HTTPException(status_code=400, detail="Email already registered") | |
| result = await db.execute(select(User).where(User.username == user_data.username)) | |
| if result.scalars().first(): | |
| raise HTTPException(status_code=400, detail="Username already taken") | |
| new_user = User( | |
| username=user_data.username, | |
| email=user_data.email, | |
| hashed_password=hash_password(user_data.password) | |
| ) | |
| db.add(new_user) | |
| await db.commit() | |
| await db.refresh(new_user) | |
| return {"message": "User created successfully"} |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 23-23: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Backend/app/routes/auth.py` around lines 22 - 37, The signup endpoint
currently only checks email uniqueness and can raise an unhandled IntegrityError
for duplicate usernames; update the signup function to explicitly check for
existing username by querying select(User).where(User.username ==
user_data.username) and raise HTTPException(status_code=400, detail="Username
already registered") before creating the User, and additionally wrap the
db.commit()/db.refresh(new_user) in a try/except catching
sqlalchemy.exc.IntegrityError to convert any remaining constraint violations
into HTTP 400 errors for robustness.
| from fastapi.security import OAuth2PasswordBearer | ||
| from jose import JWTError, jwt | ||
| from sqlalchemy.ext.asyncio import AsyncSession | ||
| from sqlalchemy.future import select |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🌐 Web query:
SQLAlchemy 2.0 sqlalchemy.future deprecated removed
💡 Result:
In SQLAlchemy 1.4, sqlalchemy.future was a transitional package (e.g., from sqlalchemy.future import select, sqlalchemy.future.Engine/Connection) used to run in “2.0 mode” while still on 1.4. (docs.sqlalchemy.org)
In SQLAlchemy 2.0, those “future” APIs are effectively the default way of working, so you should stop importing from sqlalchemy.future and import from the normal locations instead (e.g., from sqlalchemy import select, create_engine). The SQLAlchemy 2.0 docs describe the old “future mode” page as a 1.4→2.0 transition artifact. (docs.sqlalchemy.org)
Also in 2.0, the future= parameter on create_engine() / Session is kept only for backwards compatibility; if you specify it, it must be True (so you can usually just remove it). (docs.sqlalchemy.org)
Citations:
- 1: https://docs.sqlalchemy.org/en/14/core/future.html?utm_source=openai
- 2: https://docs.sqlalchemy.org/20/core/future.html?utm_source=openai
- 3: https://docs.sqlalchemy.org/20/core/future.html?utm_source=openai
🏁 Script executed:
# Check if the files contain the imports mentioned
find Backend -name "get_current_user.py" -o -name "auth.py" | head -20Repository: AOSSIE-Org/InPactAI
Length of output: 130
🏁 Script executed:
# Find and check the requirements/setup files for SQLAlchemy version
fd -e txt -e cfg -e toml -e ini -e lock | xargs grep -l -i "sqlalchemy" 2>/dev/null | head -10Repository: AOSSIE-Org/InPactAI
Length of output: 87
🏁 Script executed:
# Check the imports in both files
cat -n Backend/app/services/get_current_user.py | head -10Repository: AOSSIE-Org/InPactAI
Length of output: 513
🏁 Script executed:
cat -n Backend/app/routes/auth.py | head -10Repository: AOSSIE-Org/InPactAI
Length of output: 586
🏁 Script executed:
# Check SQLAlchemy version in requirements
cat Backend/requirements.txt | grep -i sqlalchemyRepository: AOSSIE-Org/InPactAI
Length of output: 81
Replace from sqlalchemy.future import select with from sqlalchemy import select.
sqlalchemy.future was a forward-compatibility shim in SQLAlchemy 1.4. In SQLAlchemy 2.0 (in use here: SQLAlchemy==2.0.40), the 2.0-style select() is the standard API, and imports should come directly from sqlalchemy. The same fix applies to Backend/app/routes/auth.py line 3.
♻️ Fix
-from sqlalchemy.future import select
+from sqlalchemy import select📝 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.
| from sqlalchemy.future import select | |
| from sqlalchemy import select |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Backend/app/services/get_current_user.py` at line 5, Replace the deprecated
import "from sqlalchemy.future import select" with the SQLAlchemy 2.0 import
"from sqlalchemy import select" in the modules that use it (e.g.,
get_current_user.py and auth.py); locate the top-level import statement that
references sqlalchemy.future and change it to import select directly from
sqlalchemy so the code uses the 2.0-style API.
| from ..config import settings | ||
|
|
||
| # This tells FastAPI where to look for the token (the /auth/login route) | ||
| oauth2_scheme = OAuth2PasswordBearer(tokenUrl="auth/login") |
There was a problem hiding this comment.
OAuth2PasswordBearer(tokenUrl="auth/login") points to a JSON-based endpoint — Swagger "Authorize" will not work.
OAuth2PasswordBearer expects the tokenUrl endpoint to accept application/x-www-form-urlencoded (OAuth2 standard with username/password form fields). The /auth/login endpoint instead accepts a JSON body (UserLogin). The token URL in the scheme is only used for Swagger UI's "Authorize" dialog, so this won't cause a runtime auth failure, but the interactive docs will be broken and misleading for API consumers.
Either change /auth/login to accept OAuth2PasswordRequestForm (form data), or use a plain HTTPBearer scheme and document the JSON login contract separately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Backend/app/services/get_current_user.py` at line 11, The OAuth2 scheme
declaration oauth2_scheme = OAuth2PasswordBearer(tokenUrl="auth/login") points
to /auth/login which accepts JSON, breaking Swagger's "Authorize"; fix by either
(A) changing the /auth/login endpoint handler to accept an
OAuth2PasswordRequestForm (replace JSON UserLogin input with
OAuth2PasswordRequestForm and extract username/password to issue the token) so
Swagger form-based auth works, or (B) if you must keep JSON, replace the scheme
with an HTTPBearer scheme (e.g., use fastapi.security.HTTPBearer) and document
the JSON login contract separately; update any uses of oauth2_scheme
accordingly.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Backend/app/db/seed.py (1)
16-16:⚠️ Potential issue | 🟡 MinorHardcoded plaintext seed credentials flagged by secret scanners
The plaintext
"password123"values (lines 16 and 26), referenced directly at line 47, are weak hardcoded credentials committed to source control. Secret-scanning tools (e.g., GitHub's push protection, truffleHog) may flag them. For seed/test data this is low risk, but consider loading credentials from an env variable or a local.envfile that is git-ignored:+import os users = [ { ... - "password": "password123", + "password": os.getenv("SEED_USER_PASSWORD", "changeme_dev_only"), ... },Also applies to: 26-26, 47-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend/app/db/seed.py` at line 16, Replace the hardcoded "password": "password123" seed values with values read from a non-committed configuration source: fetch the password from an environment variable (e.g., os.getenv("SEED_PASSWORD") with a sensible default for local dev) or use python-dotenv to load from a git-ignored .env file; update the seed data structures that currently contain the literal "password": "password123" (and any references at the place where the seed is used) to use the environment-backed variable instead, and add a comment documenting the expected env var name for local testing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Backend/app/db/seed.py`:
- Line 20: The seed fails because seed.py is writing timezone-aware datetimes
into a TIMESTAMP WITHOUT TIME ZONE column (User.created_at); fix by choosing one
consistent approach and applying it everywhere you set created_at: either
convert all seed datetimes to naive UTC (e.g., use datetime.utcnow() or call
.replace(tzinfo=None) before assigning) to match the existing Column(TIMESTAMP,
default=...) in models.py, or (recommended) update the model definition for
User.created_at in models.py to use
TIMESTAMP(timezone=True)/DateTime(timezone=True) and replace deprecated
datetime.utcnow defaults so the column accepts timezone-aware
datetime.now(timezone.utc); pick one option and make the corresponding change
across seed.py and models.py wherever created_at is written.
- Line 20: Seed values are passing timezone-aware datetimes to a TIMESTAMP
WITHOUT TIME ZONE column: replace calls to datetime.now(timezone.utc) used for
User.created_at in Backend/app/db/seed.py with naive datetimes (e.g., use
datetime.utcnow() or strip tzinfo) so they match the model's User.created_at
(Column(TIMESTAMP, default=datetime.utcnow)) and avoid asyncpg DataError on
session.commit(). Ensure all occurrences (the three places setting "created_at")
are updated.
- Line 47: The seed file currently uses a hardcoded weak password via
user_data["password"] which gets passed to hash_password when creating users;
replace this by reading the seed password from a configurable source (e.g., an
environment variable like SEED_USER_PASSWORD) or generate a secure random
password at runtime and store it in user_data before calling hash_password so no
literal "password123" is committed; update any references to user_data and
ensure the fallback behavior is explicit (fail fast if env var is required or
log/save the generated password securely) and keep the call to
hash_password(hashed_password=...) unchanged.
---
Outside diff comments:
In `@Backend/app/db/seed.py`:
- Line 16: Replace the hardcoded "password": "password123" seed values with
values read from a non-committed configuration source: fetch the password from
an environment variable (e.g., os.getenv("SEED_PASSWORD") with a sensible
default for local dev) or use python-dotenv to load from a git-ignored .env
file; update the seed data structures that currently contain the literal
"password": "password123" (and any references at the place where the seed is
used) to use the environment-backed variable instead, and add a comment
documenting the expected env var name for local testing.
| "bio": "Lifestyle and travel content creator", | ||
| "profile_image": None, | ||
| "created_at": datetime.utcnow() | ||
| "created_at": datetime.now(timezone.utc) # Timezone-aware |
There was a problem hiding this comment.
datetime.now(timezone.utc) passed to a TIMESTAMP WITHOUT TIME ZONE column will crash the seed under asyncpg
User.created_at is Column(TIMESTAMP, default=datetime.utcnow) — bare TIMESTAMP in SQLAlchemy maps to TIMESTAMP WITHOUT TIME ZONE in PostgreSQL. asyncpg raises a DataError because timezone-aware Python datetime objects are being passed to TIMESTAMP WITHOUT TIME ZONE columns. The specific error is: asyncpg.exceptions.DataError: invalid input for query argument: datetime.datetime(..., tzinfo=datetime.timezone.utc) (can't subtract offset-naive and offset-aware datetimes). This is triggered when code changes from database-generated timestamps to explicit Python datetime.now(timezone.utc) datetimes on a TIMESTAMP WITHOUT TIME ZONE column.
The seed will fail at session.commit() on every fresh run.
Two consistent fixes — pick one and apply it everywhere created_at is written:
Option A — keep columns naive (minimal change in seed only):
🐛 Proposed fix (Option A)
- "created_at": datetime.now(timezone.utc) # Timezone-aware
+ "created_at": datetime.now(timezone.utc).replace(tzinfo=None)Option B — make the column timezone-aware throughout (preferred long-term): Apply this to Backend/app/models/models.py so the column type and seed value are consistent:
- created_at = Column(TIMESTAMP, default=datetime.utcnow)
+ created_at = Column(TIMESTAMP(timezone=True), default=lambda: datetime.now(timezone.utc))With Option B the seed values (datetime.now(timezone.utc)) are already correct and no change to seed.py is needed. This also eliminates the use of the deprecated datetime.utcnow in the model.
Also applies to: 30-30, 51-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Backend/app/db/seed.py` at line 20, The seed fails because seed.py is writing
timezone-aware datetimes into a TIMESTAMP WITHOUT TIME ZONE column
(User.created_at); fix by choosing one consistent approach and applying it
everywhere you set created_at: either convert all seed datetimes to naive UTC
(e.g., use datetime.utcnow() or call .replace(tzinfo=None) before assigning) to
match the existing Column(TIMESTAMP, default=...) in models.py, or (recommended)
update the model definition for User.created_at in models.py to use
TIMESTAMP(timezone=True)/DateTime(timezone=True) and replace deprecated
datetime.utcnow defaults so the column accepts timezone-aware
datetime.now(timezone.utc); pick one option and make the corresponding change
across seed.py and models.py wherever created_at is written.
🧩 Analysis chain
🏁 Script executed:
# Find and read the User model definition
find . -name "models.py" -path "*/Backend/app/models/*" | head -5Repository: AOSSIE-Org/InPactAI
Length of output: 93
🏁 Script executed:
# Read the seed.py file to confirm the timezone-aware datetime usage
find . -name "seed.py" -path "*/Backend/app/db/*" | head -5Repository: AOSSIE-Org/InPactAI
Length of output: 87
🌐 Web query:
asyncpg timezone-aware datetime TIMESTAMP WITHOUT TIME ZONE error
💡 Result:
In asyncpg, PostgreSQL type mappings are strict:
TIMESTAMP WITHOUT TIME ZONE(timestamp) ↔ offset-naïvedatetime(notzinfo)TIMESTAMP WITH TIME ZONE(timestamptz) ↔ offset-awaredatetime(hastzinfo)
[1]
So you’ll get an error when you pass a timezone-aware Python datetime (e.g. tzinfo=UTC) into a SQL parameter/column typed TIMESTAMP WITHOUT TIME ZONE, because asyncpg refuses to silently drop timezone info. This is commonly described as asyncpg raising if you try to store a timezone-aware datetime into a “no timezone” column. [2]
Fix options (pick one)
- Use the right DB type (recommended): change the column to timestamptz
ALTER TABLE my_table
ALTER COLUMN created_at TYPE timestamptz
USING created_at AT TIME ZONE 'UTC';Postgres conversions between timestamp types typically assume the session timezone, and AT TIME ZONE lets you specify it explicitly. [3]
- If the column must stay
timestamp, pass a naïve datetime (commonly “UTC but naïve”)
from datetime import timezone
dt_aware = dt # tz-aware
dt_naive_utc = dt_aware.astimezone(timezone.utc).replace(tzinfo=None)
await conn.execute("INSERT INTO t(ts) VALUES($1)", dt_naive_utc)- Convert in SQL at insert time (store a UTC “wall clock” timestamp in a
timestampcolumn)
INSERT INTO t(ts)
VALUES (($1::timestamptz AT TIME ZONE 'UTC'));(AT TIME ZONE is the standard Postgres way to control the interpretation/conversion.) [3]
Rule of thumb: if you want an “instant in time”, use timestamptz; if you want a “local calendar time” (no timezone meaning), use timestamp. [3]
Sources: asyncpg type mapping table [1], discussion of asyncpg refusing tz-aware datetimes for “no timezone” columns [2], PostgreSQL timestamp/timezone semantics and AT TIME ZONE behavior [3].
[1] (magicstack.github.io)
[2] (safir.lsst.io)
[3] (postgresql.org)
Citations:
- 1: https://magicstack.github.io/asyncpg/devel/usage.html?utm_source=openai
- 2: https://safir.lsst.io/v/tickets-DM-37589a/user-guide/database.html?utm_source=openai
- 3: https://www.postgresql.org/docs/16/datatype-datetime.html?utm_source=openai
🏁 Script executed:
# Read the User model definition
cat -n ./Backend/app/models/models.py | grep -A 20 "class User"Repository: AOSSIE-Org/InPactAI
Length of output: 1925
🏁 Script executed:
# Read seed.py to verify the timezone-aware datetime usage at lines 20, 30, 51
cat -n ./Backend/app/db/seed.py | head -60Repository: AOSSIE-Org/InPactAI
Length of output: 2613
Timezone-aware datetime passed to TIMESTAMP WITHOUT TIME ZONE column will cause asyncpg to fail
User.created_at is defined as Column(TIMESTAMP, default=datetime.utcnow) — SQLAlchemy maps bare TIMESTAMP to TIMESTAMP WITHOUT TIME ZONE in PostgreSQL. The seed is passing datetime.now(timezone.utc) (timezone-aware) to this column at lines 20, 30, and 51, which will cause asyncpg to raise asyncpg.exceptions.DataError at session.commit().
Recommended fix — strip timezone from seed values (matches your preference for naive datetimes):
Proposed fix
- "created_at": datetime.now(timezone.utc) # Timezone-aware
+ "created_at": datetime.now(timezone.utc).replace(tzinfo=None)This keeps the column naive (consistent with the model default datetime.utcnow and your broader preference for naive datetimes in the codebase).
Alternatively, you could make the column timezone-aware throughout (TIMESTAMP(timezone=True) in the model), but this would require migrating the column type and updating other naive timestamp columns in the schema.
Also applies to: 30-30, 51-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Backend/app/db/seed.py` at line 20, Seed values are passing timezone-aware
datetimes to a TIMESTAMP WITHOUT TIME ZONE column: replace calls to
datetime.now(timezone.utc) used for User.created_at in Backend/app/db/seed.py
with naive datetimes (e.g., use datetime.utcnow() or strip tzinfo) so they match
the model's User.created_at (Column(TIMESTAMP, default=datetime.utcnow)) and
avoid asyncpg DataError on session.commit(). Ensure all occurrences (the three
places setting "created_at") are updated.
| id=user_data["id"], | ||
| username=user_data["username"], | ||
| email=user_data["email"], | ||
| hashed_password=hash_password(user_data["password"]), # Fixes seed failure |
There was a problem hiding this comment.
Hardcoded weak seed credentials are flagged by secret scanners
user_data["password"] (lines 16 and 26) holds the literal "password123", which is committed to source control and will be flagged by secret-scanning tools. For a seed file, this is low risk, but externalising it is a simple improvement:
🔒 Suggested improvement
+import os
...
- "password": "password123",
+ "password": os.getenv("SEED_USER_PASSWORD", "changeme_dev_only"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Backend/app/db/seed.py` at line 47, The seed file currently uses a hardcoded
weak password via user_data["password"] which gets passed to hash_password when
creating users; replace this by reading the seed password from a configurable
source (e.g., an environment variable like SEED_USER_PASSWORD) or generate a
secure random password at runtime and store it in user_data before calling
hash_password so no literal "password123" is committed; update any references to
user_data and ensure the fallback behavior is explicit (fail fast if env var is
required or log/save the generated password securely) and keep the call to
hash_password(hashed_password=...) unchanged.
|
I have completed a full refactor of the authentication infrastructure to address all 10 points raised in the CodeRabbit review. Key updates include: Security Hardening: Upgraded python-jose to v3.4.0 to remediate CVE-2024-33663/4 and pinned bcrypt to resolve compatibility issues with passlib. Configuration Reform: Replaced the plain Settings class with Pydantic BaseSettings to ensure secure, validated environment variable loading from .env. Auth Logic Improvements: Added an explicit duplicate username check in the signup flow and implemented Pydantic response models to prevent sensitive data leakage. Data Integrity: Standardized all database models to use timezone-aware UTC timestamps and updated the seed.py script to support the new hashed_password requirements. Documentation: Achieved full docstring coverage across all new service and route functions. This PR now represents a production-ready security foundation for the project. I'm looking forward to your feedback! |
1 participant
Since you have now integrated the authentication logic directly into the project's core models.py, this description emphasizes architectural integrity and security compliance. It shows mentors you didn't just "add a feature," but you "improved the system".
PR Description: Centralized Identity & JWT Security Framework
Overview
This PR establishes a robust, production-ready Identity and Access Management (IAM) system for InPactAI. By restoring password security to the core User model and implementing JWT-based session handling, this work provides the necessary security gates for all current and future API endpoints.
Key Implementation Details
Schema Evolution: Updated the central User model in models.py to support local credentials storage using hashed_password, moving away from unauthenticated/external mock setups.
Stateless Authentication: Developed a high-performance JWT lifecycle in auth.py, utilizing HS256 signing for secure, stateless user verification.
Cryptographic Security: Implemented bcrypt hashing via passlib to ensure industry-standard protection of user credentials.
Global Dependency Injection: Created the get_current_user FastAPI dependency, allowing any route in the system to become "Protected" simply by adding a single parameter.
Route Registration: Fully integrated the new Auth service into the main.py application entry point.
Why this is necessary?
As highlighted in previous code reviews, the application lacked a mechanism to verify requester identity, creating a critical vulnerability for data-modifying routes. This PR resolves that gap, enabling secure interactions for the Collaboration Hub and upcoming Sponsorship features.
Summary by CodeRabbit
New Features
Data Model
Chores