Implement JWT authentication with signup, login, and refresh endpoints#42
Merged
saudzahirr merged 7 commits intomainfrom Mar 5, 2026
Merged
Implement JWT authentication with signup, login, and refresh endpoints#42saudzahirr merged 7 commits intomainfrom
saudzahirr merged 7 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds JWT-based vendor authentication (signup, login, refresh), token creation/validation, RLS-aware DB cursor dependency, vendor CRUD and auth service, auth routes, config updates, and extensive unit and integration tests; also updates middleware, exception handling, and project deps. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as FastAPI
participant Auth as AuthService
participant CRUD as VendorCRUD
participant DB as Database
participant Sec as Security
rect rgba(200,150,255,0.5)
Note over Client,API: Signup
Client->>API: POST /auth/signup {email,password,client_id}
API->>Auth: signup(cursor, email, password, client_id, settings)
Auth->>CRUD: get_vendor_by_email(cursor, email)
CRUD->>DB: SELECT vendor WHERE LOWER(email)=...
DB-->>CRUD: None
Auth->>Sec: hash_password(password)
Sec-->>Auth: password_hash
Auth->>CRUD: create_vendor(cursor, email, password_hash)
CRUD->>DB: INSERT ... RETURNING id,email
DB-->>CRUD: {id,email}
CRUD-->>Auth: {id,email}
Auth-->>API: SignupResponse{vendor}
API-->>Client: 201 {vendor}
end
rect rgba(200,200,150,0.5)
Note over Client,API: Login
Client->>API: POST /auth/login {email,password,client_id}
API->>Auth: login(cursor, email, password, client_id, settings)
Auth->>CRUD: get_vendor_by_email(cursor, email)
CRUD->>DB: SELECT ... password_hash
DB-->>CRUD: {id,email,password_hash}
Auth->>Sec: verify_password(password, password_hash)
Sec-->>Auth: ok
Auth->>Sec: create_access_token(vendor_id, settings)
Sec-->>Auth: access_token
Auth->>Sec: create_refresh_token(vendor_id, settings)
Sec-->>Auth: refresh_token
Auth-->>API: TokenPair{access,refresh}
API-->>Client: 200 {access,refresh,token_type}
end
rect rgba(150,200,255,0.5)
Note over Client,API: Protected request (RLS)
Client->>API: GET /protected Authorization: Bearer {access_token}
API->>Sec: decode_token(access_token, settings)
Sec-->>API: {vendor_id, token_type}
API->>API: validate token_type == "access"
API->>API: get_rls_cursor(vendor_id)
API->>DB: connection.run(app.set_app_context(vendor_id))
DB-->>API: Cursor (RLS context)
API->>DB: query with cursor (RLS enforced)
DB-->>API: filtered rows
API-->>Client: 200 {response}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 7
🤖 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/api/deps.py`:
- Around line 61-65: The code only checks presence of vendor_id but not its UUID
format, so validate vendor_id is a valid UUID string before returning: attempt
to parse it (e.g., with uuid.UUID or equivalent) and if parsing fails raise
AuthenticationException("Invalid token payload") so callers like
app.set_app_context(vendor_id) never receive an invalid UUID; update the return
path in the function that reads payload->vendor_id and ensure
AuthenticationException is raised on invalid format.
In `@backend/app/core/security.py`:
- Around line 10-11: The PasswordHash configuration currently only includes
BcryptHasher, which removes verification for existing Argon2-hashed passwords;
update the password_hash declaration (the PasswordHash(...) tuple) to include
Argon2Hasher alongside BcryptHasher so existing Argon2 hashes remain verifiable
while new passwords continue to use bcrypt (i.e., add Argon2Hasher to the tuple
used to construct password_hash).
In `@backend/app/crud/vendor.py`:
- Around line 39-49: The create_vendor function can raise a UniqueViolation
during concurrent signups; modify create_vendor(cursor, email, password_hash) to
use an INSERT ... ON CONFLICT DO NOTHING with the same functional index
(conflict target on LOWER("email") or the approriate index expression) and
RETURNING "id", "email", then if cursor.fetchone() is None return None instead
of asserting; update the caller in backend/app/services/auth.py where vendor =
create_vendor(...) to check for None and raise a ConflictException("A vendor
with this email already exists") to surface a 409 instead of letting a 500
bubble up.
In `@backend/app/schemas/auth.py`:
- Around line 15-21: LoginRequest.password and RefreshRequest.refresh_token are
unbounded; add the same validation bounds used by SignupRequest to harden input
(e.g. use Field(..., min_length=8, max_length=128)) so both password and
refresh_token enforce min/max lengths; update the type declarations for
LoginRequest.password and RefreshRequest.refresh_token to use Field with
min_length=8 and max_length=128 to match SignupRequest.
In `@backend/app/services/auth.py`:
- Around line 36-42: Wrap the call to create_vendor(cursor, email, hashed) in a
try/except that catches the database UniqueViolation (the DB-level
UniqueViolation exception) and converts it into a ConflictException("A vendor
with this email already exists"); keep the existing pre-check with
get_vendor_by_email but handle UniqueViolation around create_vendor in auth.py
so concurrent inserts map to 409 instead of bubbling as a 500.
In `@backend/tests/integration/test_auth.py`:
- Around line 245-268: The test is directly calling get_current_vendor_id
instead of exercising the HTTP layer and there is no protected route to validate
dependency wiring; add a temporary protected test endpoint that uses the
dependency (e.g., def _test_protected(vendor_id=Depends(get_current_vendor_id))
or use the CurrentVendorId dependency) mounted on the test app/router, then use
the TestClient to send a GET to that endpoint with no Authorization header and
assert a 401 response; update the test to call the endpoint via client (not
get_current_vendor_id directly) so Bearer parsing, dependency injection, and
exception-to-HTTP translation are all exercised.
In `@backend/tests/unit/test_auth.py`:
- Around line 141-152: Add a new unit test that exercises the race-path where
pre-read shows no existing vendor but the insert collides: call signup (function
signup in app.services.auth) with get_vendor_by_email patched to return None (so
the pre-check passes) and patch create_vendor (or the internal call that
performs the insert, e.g., create_vendor in app.services.auth) to simulate a
concurrent-insert conflict by raising or returning a
ConflictException-equivalent; then assert signup raises ConflictException.
Ensure you still pass a MagicMock cursor and the same settings fixture so the
test isolates the insert-time conflict path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8e1c4da7-8b9a-40ee-9d5d-762e862d79f8
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
backend/app/api/deps.pybackend/app/api/main.pybackend/app/api/routes/auth.pybackend/app/core/config.pybackend/app/core/security.pybackend/app/crud/__init__.pybackend/app/crud/vendor.pybackend/app/schemas/auth.pybackend/app/services/__init__.pybackend/app/services/auth.pybackend/pyproject.tomlbackend/tests/integration/test_auth.pybackend/tests/unit/test_auth.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/services/auth.py`:
- Around line 105-113: In refresh(), validate the format/type of
payload["vendor_id"] before calling get_vendor_by_id to avoid DB cast/runtime
errors: check the vendor_id value (e.g., ensure it's an int or valid UUID
depending on your schema) and attempt a safe parse/cast with a try/except (or
isinstance check), and if parsing fails raise AuthenticationException("Invalid
token payload") so get_vendor_by_id(cursor, vendor_id) only receives a
properly-typed id; update references to vendor_id, refresh(),
get_vendor_by_id(), and AuthenticationException accordingly.
In `@backend/tests/unit/test_auth.py`:
- Around line 236-275: Add a new unit test in TestGetCurrentVendorId that
constructs an access token whose "vendor_id" claim is present but not a valid
UUID (e.g., "invalid-uuid"), using create_access_token(...) with settings, then
call get_current_vendor_id(creds, settings) and assert it raises
AuthenticationException (match a message indicating invalid vendor id or UUID);
reuse the existing pattern of creating creds = MagicMock(); creds.credentials =
token and follow other tests' structure to ensure the malformed vendor_id path
is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d0a2e7c-2591-4f5f-a0f9-44f353b3f289
📒 Files selected for processing (7)
backend/app/api/deps.pybackend/app/core/security.pybackend/app/crud/vendor.pybackend/app/schemas/auth.pybackend/app/services/auth.pybackend/tests/integration/test_auth.pybackend/tests/unit/test_auth.py
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/tests/conftest.py (1)
48-55:⚠️ Potential issue | 🔴 CriticalCritical: dependency override lifecycle is indented into
_get_db, breaking fixture behavior.Lines 53–55 are inside
_get_dbinstead of at theoverride_get_dblevel. This causes two problems:
- The dependency override is never installed at fixture setup time—it only executes inside the generator body, which may never run if dependencies are mocked elsewhere.
_get_dbbecomes a multi-yield generator (yields on line 51 and line 54), which violates FastAPI's dependency cleanup contract and can cause runtime errors.Move lines 53–55 to the
override_get_dbfunction level (matching the indentation of line 48) and wrap the yield in a try/finally block to ensure cleanup:Fix
`@pytest.fixture`(scope="module") def override_get_db(test_container: PostgresContainer): """Start a dedicated Testcontainers Postgres for this module and override the get_db dependency so endpoints receive a cursor from it. Each test module gets its own isolated container, avoiding cross-module side effects when tests run in parallel. """ def _get_db() -> typing.Generator[Cursor, None, None]: with connect(test_container.get_connection_url()) as conn: with conn.cursor() as cursor: yield cursor + app.dependency_overrides[get_db] = _get_db + try: + yield + finally: + app.dependency_overrides.pop(get_db, None) - app.dependency_overrides[get_db] = _get_db - yield - app.dependency_overrides.pop(get_db, None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/conftest.py` around lines 48 - 55, The dependency override setup/teardown are incorrectly indented inside the `_get_db` generator, making it a multi-yield generator and preventing the override from being installed at fixture setup; fix by moving the lines that set and pop `app.dependency_overrides[get_db]` out to the `override_get_db` fixture scope (call `app.dependency_overrides[get_db] = _get_db` before yielding from the fixture and `app.dependency_overrides.pop(get_db, None)` in a finally after the fixture yield), and change `_get_db` so it yields the `cursor` only once (wrap the single yield in a try/finally if needed to ensure DB cursor/connection cleanup) so `_get_db` remains a single-yield FastAPI dependency generator.
🤖 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/api/middlewares.py`:
- Around line 6-11: The call_next parameter in add_request_id is loosely typed
as object; change it to the Starlette/FastAPI request handler type by importing
and using RequestResponseEndpoint from starlette.types and annotate call_next:
RequestResponseEndpoint so the signature reads async def add_request_id(request:
Request, call_next: RequestResponseEndpoint) -> Response; ensure the import of
RequestResponseEndpoint is added alongside Request/Response imports and update
any related type hints accordingly.
In `@backend/pyproject.toml`:
- Line 16: Update the PyJWT dependency constraint currently specified as the
string "pyjwt>=2.8.0,<3.0.0" so it excludes the vulnerable 2.10.0 release;
change it to either "pyjwt>=2.8.0,!=2.10.0,<3.0.0" or bump to a safe minimum
like "pyjwt>=2.10.1,<3.0.0" (preferably "pyjwt>=2.11.0,<3.0.0" to pick a current
secure release) in the dependency entry so CVE-2024-53861 is avoided.
In `@backend/tests/integration/test_auth.py`:
- Around line 46-101: The pg_container fixture is module-scoped so DB state is
shared across tests; change test isolation by making pg_container (and client)
function-scoped or ensure each test gets a fresh DB/transaction: update the
pg_container fixture (and corresponding client) to scope="function" or wrap each
test in a DB transaction/rollback created in _override_get_db or a per-test
setup that reinitializes the database, referencing the pg_container and client
fixtures and the _override_get_db function to locate the code to modify.
---
Outside diff comments:
In `@backend/tests/conftest.py`:
- Around line 48-55: The dependency override setup/teardown are incorrectly
indented inside the `_get_db` generator, making it a multi-yield generator and
preventing the override from being installed at fixture setup; fix by moving the
lines that set and pop `app.dependency_overrides[get_db]` out to the
`override_get_db` fixture scope (call `app.dependency_overrides[get_db] =
_get_db` before yielding from the fixture and
`app.dependency_overrides.pop(get_db, None)` in a finally after the fixture
yield), and change `_get_db` so it yields the `cursor` only once (wrap the
single yield in a try/finally if needed to ensure DB cursor/connection cleanup)
so `_get_db` remains a single-yield FastAPI dependency generator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b904356b-42bf-4eca-8894-4ef08613f04a
📒 Files selected for processing (23)
.gitignorebackend/app/api/deps.pybackend/app/api/main.pybackend/app/api/middlewares.pybackend/app/api/routes/auth.pybackend/app/api/routes/health.pybackend/app/api/routes/login.pybackend/app/core/config.pybackend/app/core/exception_handlers.pybackend/app/core/exceptions.pybackend/app/core/security.pybackend/app/crud/vendor.pybackend/app/main.pybackend/app/pre_start.pybackend/app/schemas/auth.pybackend/app/schemas/response.pybackend/app/services/auth.pybackend/pyproject.tomlbackend/tests/__init__.pybackend/tests/conftest.pybackend/tests/integration/test_auth.pybackend/tests/integration/test_smoke.pybackend/tests/unit/test_auth.py
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/api/middlewares.py (1)
15-18:⚠️ Potential issue | 🟠 MajorValidate incoming
X-Request-IDbefore accepting it.Line 15-Line 17 trusts arbitrary client input and stores it in
request.state, then echoes it in Line 21. This allows request-id spoofing and low-quality/oversized correlation values to pollute tracing/logs. Accept only a strict format/length (e.g., UUID-like), otherwise generate a new UUID.♻️ Proposed hardening
+import re import uuid @@ +REQUEST_ID_RE = re.compile(r"^[A-Za-z0-9\-_.]{1,64}$") @@ - raw_header = request.headers.get("X-Request-ID") - stripped_header = raw_header.strip() if raw_header else "" - request_id = stripped_header if stripped_header else str(uuid.uuid4()) + raw_header = request.headers.get("X-Request-ID") + stripped_header = raw_header.strip() if raw_header else "" + request_id = ( + stripped_header + if stripped_header and REQUEST_ID_RE.fullmatch(stripped_header) + else str(uuid.uuid4()) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/middlewares.py` around lines 15 - 18, Validate the incoming X-Request-ID before accepting it: in the code that reads raw_header/stripped_header and sets request.state.request_id, check that stripped_header matches a strict UUID-like pattern and length (e.g., /^[0-9a-fA-F-]{36}$/ or use a UUID parsing/validation helper) and only use it as request_id if it passes; otherwise generate a new UUID via uuid.uuid4() and assign that to request.state.request_id. Ensure trimming still happens, reject empty/oversized values, and centralize the check in the same block that currently sets request_id so the echoed ID is always validated or freshly generated.
🤖 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/pyproject.toml`:
- Line 59: Update the typo in the lint-rule comment text: change the comment
that reads "flake8-buitins" to "flake8-builtins" (the string/comment on the same
line as "A", which documents flake8-builtins checking variable/function names to
avoid shadowing builtins).
In `@backend/tests/integration/test_auth.py`:
- Around line 41-43: The test route _protected_test currently only returns the
JWT claim (vendor_id) and doesn't assert that the DB session RLS context was
set; modify the handler used in tests (function _protected_test that accepts
CurrentVendorId) to query the database for current_setting('app.vendor_id',
true) within the same request and return that value (or add an additional route
used by the test that does the same), then update the assertions in the test to
compare both the echoed JWT vendor_id and the DB session value; apply the same
change to the other test route referenced at the 275-278 area so tests verify
the DB session context is actually set.
- Around line 298-307: Replace the direct call to create_access_token in
test_valid_token_returns_vendor_id with a real login flow: create or use a
vendor identity (vendor_id), POST to the /auth/login endpoint via the TestClient
(client.post(f"{API_V1}/auth/login", ...)) using the expected login payload for
your auth endpoint, assert the login response is successful, extract the
returned access_token from the login JSON response, and use that token in the
Authorization header when calling f"{API_V1}/protected-test"; update assertions
to ensure both the login returned a usable access_token and the protected
endpoint returns 200. Ensure you remove the direct create_access_token usage in
test_valid_token_returns_vendor_id.
---
Outside diff comments:
In `@backend/app/api/middlewares.py`:
- Around line 15-18: Validate the incoming X-Request-ID before accepting it: in
the code that reads raw_header/stripped_header and sets
request.state.request_id, check that stripped_header matches a strict UUID-like
pattern and length (e.g., /^[0-9a-fA-F-]{36}$/ or use a UUID parsing/validation
helper) and only use it as request_id if it passes; otherwise generate a new
UUID via uuid.uuid4() and assign that to request.state.request_id. Ensure
trimming still happens, reject empty/oversized values, and centralize the check
in the same block that currently sets request_id so the echoed ID is always
validated or freshly generated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a6e5e9be-a8f2-4e9e-96b5-16c02c5f80d2
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
backend/app/api/middlewares.pybackend/pyproject.tomlbackend/tests/conftest.pybackend/tests/integration/test_auth.py
- Add UUID validation for vendor_id in get_current_vendor_id. - Update create_vendor to handle email conflicts gracefully. - Refactor auth service to raise ConflictException on duplicate vendor email. - Improve request schemas for better validation. - Add integration tests for protected endpoints and concurrent signup conflicts.
- Updated .gitignore to exclude log files. - Improved docstrings in deps.py, main.py, middlewares.py, auth.py, health.py, and other modules for better clarity on function behavior and return types. - Refactored exception handling in exception_handlers.py and exceptions.py to include return type hints in constructors. - Added type hints and improved formatting in various service and CRUD functions for consistency and readability. - Integrated Ruff configuration in pyproject.toml for enhanced linting and code style enforcement. - Updated integration and unit tests for better readability and maintainability.
…sts for vendor ID retrieval
… and consistency; enhance test coverage for error handling
saudzahirr
force-pushed
the
feature/jwt-authentication
branch
from
ee4d8cf to
10a7615
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
backend/app/crud/vendor.py (1)
59-63:⚠️ Potential issue | 🔴 CriticalFix
ON CONFLICTexpression-target syntax to avoid runtime SQL failure.Line 62 uses
ON CONFLICT (LOWER("email")), but PostgreSQL requires expression targets in conflict inference to be wrapped in their own parentheses:ON CONFLICT ((LOWER("email"))). The outer parentheses denote the conflict target list, and the inner parentheses wrap the index expression itself. Without double parentheses, PostgreSQL will not recognize this as an expression form and will fail to match against your expression index, breaking insert handling on signup.Suggested fix
cursor.execute( 'INSERT INTO app."vendors" ("email", "password_hash") ' "VALUES (%s, %s) " - 'ON CONFLICT (LOWER("email")) DO NOTHING ' + 'ON CONFLICT ((LOWER("email"))) DO NOTHING ' 'RETURNING "id", "email"', (email, password_hash), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/crud/vendor.py` around lines 59 - 63, The INSERT SQL in the cursor.execute call (in vendor.py) uses ON CONFLICT (LOWER("email")) which is invalid for an expression index; change that conflict target to use double parentheses so it reads ON CONFLICT ((LOWER("email"))) in the same SQL string passed to cursor.execute to match the expression index and avoid runtime SQL errors.
🤖 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/api/routes/login.py`:
- Line 4: The file defines an empty APIRouter instance named router in login.py
but no routes are attached while backend/app/api/main.py still mounts it at
'/login'; either remove this empty router export and the corresponding
include_router(...) call in main.py, or implement the intended login endpoints
(e.g., add handler functions and router.post/get registrations to the router in
login.py) and ensure the router variable is exported; update main.py to only
include_router(router, prefix="/login") when router contains handlers.
In `@backend/tests/integration/test_auth.py`:
- Around line 302-323: The test test_valid_token_returns_vendor_id only asserts
non-empty vendor IDs; modify it to assert the protected endpoint returns the
exact vendor created by _signup. Capture the created vendor id from the _signup
call (update _signup to return the new vendor id if it doesn't already), store
it as created_vendor_id, then after the GET to f"{API_V1}/protected-test" assert
body["vendor_id"] == created_vendor_id and body["db_vendor_id"] ==
created_vendor_id (keeping the existing token flow using login_resp and token).
- Around line 42-47: The helper _protected_test currently overrides DB context
by calling app.set_app_context, which hides auth-dependency regressions; remove
the call to cursor.execute("SELECT app.set_app_context(%s)", (vendor_id,))
inside _protected_test and only execute the query that reads
current_setting('app.vendor_id', true) so the test asserts the upstream
dependency (CurrentVendorId injection) actually set app.vendor_id; keep
returning {"vendor_id": vendor_id, "db_vendor_id": db_vendor_id} so the test can
compare the provided vendor_id to the DB value.
---
Duplicate comments:
In `@backend/app/crud/vendor.py`:
- Around line 59-63: The INSERT SQL in the cursor.execute call (in vendor.py)
uses ON CONFLICT (LOWER("email")) which is invalid for an expression index;
change that conflict target to use double parentheses so it reads ON CONFLICT
((LOWER("email"))) in the same SQL string passed to cursor.execute to match the
expression index and avoid runtime SQL errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e319c01-dc2e-4b5a-b098-9662effa83f9
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
.gitignorebackend/app/api/deps.pybackend/app/api/main.pybackend/app/api/middlewares.pybackend/app/api/routes/auth.pybackend/app/api/routes/health.pybackend/app/api/routes/login.pybackend/app/core/config.pybackend/app/core/exception_handlers.pybackend/app/core/exceptions.pybackend/app/core/security.pybackend/app/crud/__init__.pybackend/app/crud/vendor.pybackend/app/main.pybackend/app/pre_start.pybackend/app/schemas/auth.pybackend/app/schemas/response.pybackend/app/services/__init__.pybackend/app/services/auth.pybackend/pyproject.tomlbackend/tests/__init__.pybackend/tests/api/routes/test_health.pybackend/tests/conftest.pybackend/tests/core/test_exception_handlers.pybackend/tests/core/test_exceptions.pybackend/tests/integration/test_auth.pybackend/tests/schemas/test_response.pybackend/tests/unit/test_auth.py
…dor creation SQL to handle email conflicts correctly; enhance integration tests with RLS cursor dependency
Member
Author
|
@coderabbitai review! |
|
✅ Actions performedReview triggered.
|
1 similar comment
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/integration/test_auth.py`:
- Around line 123-150: In _signup and _login helper functions, stop calling
.json() directly on the client.post result; instead capture the response, assert
its HTTP status code (e.g., for _signup assert expected creation code like 201
or at least response.ok, and for _login assert expected success like 200 or
response.ok) to fail tests with clear HTTP assertions, then call and return
response.json(); update the bodies of _signup and _login to use a response
variable, assert response.status_code (or response.ok) before returning
response.json().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e9b76b4e-9c5f-45e4-bd06-e5f5e833a2c5
📒 Files selected for processing (3)
backend/app/api/main.pybackend/app/crud/vendor.pybackend/tests/integration/test_auth.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/tests/integration/test_auth.py (1)
123-150:⚠️ Potential issue | 🟡 MinorAssert HTTP status before parsing JSON in helper methods.
Line 128 and Line 143 parse JSON unconditionally; when setup calls fail, failures become less actionable than explicit status assertions.
Suggested patch
def _signup( client: TestClient, email: str = "vendor@test.com", password: str = "SecurePass123!", ) -> dict: - return client.post( + resp = client.post( f"{API_V1}/auth/signup", json={ "email": email, "password": password, "client_id": "integration-test", }, - ).json() + ) + assert resp.status_code == 201, resp.text + return resp.json() @@ def _login( client: TestClient, email: str = "vendor@test.com", password: str = "SecurePass123!", ) -> dict: - return client.post( + resp = client.post( f"{API_V1}/auth/login", json={ "email": email, "password": password, "client_id": "integration-test", }, - ).json() + ) + assert resp.status_code == 200, resp.text + return resp.json()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/integration/test_auth.py` around lines 123 - 150, The helpers _signup and _login currently call .json() without checking response status; change both to first assert the response.status_code is 200 (or the expected success code) before calling .json() so test failures surface clearly—locate the _signup and _login functions and add an assertion on the returned response.status_code (with an explanatory message if desired) prior to invoking .json().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/tests/integration/test_auth.py`:
- Around line 123-150: The helpers _signup and _login currently call .json()
without checking response status; change both to first assert the
response.status_code is 200 (or the expected success code) before calling
.json() so test failures surface clearly—locate the _signup and _login functions
and add an assertion on the returned response.status_code (with an explanatory
message if desired) prior to invoking .json().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1987fbd7-678a-45b8-9c88-6efd382a2f68
📒 Files selected for processing (3)
backend/app/api/main.pybackend/app/crud/vendor.pybackend/tests/integration/test_auth.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #6
Summary by CodeRabbit
New Features
Configuration
Observability
Tests