This repository was archived by the owner on May 12, 2026. It is now read-only.
feat(auth): Implement Login Flow(BE-007) - #15
Merged
Merged
Conversation
…on in createSession
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds DB and Redis-backed failed-attempt tracking and lockout to login, updates AuthenticationService to enforce lockout and create sessions with refresh tokens, adds DTO password max-length validation, migrates schema, updates Swagger docs, and expands tests. ChangesLogin Lockout & Rate-Limiting Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant AuthService
participant Redis
participant Database
participant JwtService
Client->>Controller: POST /login (email, password, IP)
Controller->>AuthService: loginUser(dto, ip)
AuthService->>Redis: Check lockout counter (fast-path)
alt Account Locked
AuthService-->>Client: 403 Account Locked
else Not Locked
AuthService->>Database: Query user by email
alt User Not Found
AuthService->>AuthService: Timing-safe dummy bcrypt compare
AuthService->>Database: Increment failed_attempts + lock
AuthService->>Redis: Increment fail counter
AuthService-->>Client: 401 Invalid credentials
else User Found
AuthService->>Database: Check locked_until timestamp
alt Database Lockout Active
AuthService-->>Client: 403 Account Locked
else Lockout Expired
AuthService->>AuthService: Bcrypt verify password
alt Password Invalid
AuthService->>Database: Atomic: failed_attempts++, locked_until update
AuthService->>Redis: Increment fail counter
AuthService-->>Client: 401 Invalid credentials
else Password Valid
AuthService->>Database: Create UserSession + clear old lockouts (txn)
AuthService->>Redis: Write session keys
AuthService->>JwtService: Sign access token with sid
AuthService-->>Client: 200 access_token + refresh_token
end
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
sage-ali
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/auth/auth.service.ts`:
- Around line 6-7: Persisting raw refresh tokens is insecure; change the flow so
create/generate refresh token functions (e.g.,
createRefreshToken/generateRefreshToken) continue to return the raw token to the
client but before persisting call a one-way hash (e.g., SHA-256) and store that
hash in the user_sessions record instead of the raw token; then in
validate/refresh/revoke paths (e.g.,
validateRefreshToken/refreshToken/revokeRefreshToken or any DB lookup using
user_sessions) hash the incoming raw token the same way and compare/delete based
on the hash. Update the code that writes/queries the user_sessions column to use
the hashed value and ensure the raw token is never persisted or logged.
- Around line 151-156: Add a DB-level uniqueness constraint on
auth_metadata.user_id and change loadOrCreateMeta to use an atomic upsert/select
instead of findOneBy-then-create: attempt to insert (via
authMetadataRepository.save/create or a repository.query with INSERT ... ON
CONFLICT DO NOTHING / ON CONFLICT (...) DO UPDATE SET ... RETURNING *) for {
user_id, failed_attempts: 0 } and if the insert did not return a row, select the
row afterwards; ensure loadOrCreateMeta always returns the single row and handle
unique-violation races by selecting the existing record on conflict.
- Around line 159-176: recordFailedAttempt currently always increments
failed_attempts so an expired lockout remains at threshold and re-locks on the
first new failure; update the UPDATE in recordFailedAttempt to reset the counter
when the prior lockout has expired by using locked_until < CURRENT_TIMESTAMP to
branch: if lock expired then set failed_attempts = 1 (this new failure) else
failed_attempts = failed_attempts + 1, and keep the existing logic that sets
locked_until when failed_attempts reaches MAX_FAILED_ATTEMPTS (use
MAX_FAILED_ATTEMPTS and LOCKOUT_MINUTES as before); keep the call to
incrementRedisFailCounter(email).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b9cc3a1c-661e-44c7-940d-0425d65cc0d4
📒 Files selected for processing (6)
src/database/migrations/1778323146003-migration.tssrc/modules/auth/auth.controller.tssrc/modules/auth/auth.service.tssrc/modules/auth/dto/login.dto.tssrc/modules/auth/entities/auth-metadata.entity.tssrc/modules/auth/tests/auth.service.spec.ts
sage-ali
changed the title
ibraheembello
approved these changes
May 10, 2026
Nuel-09
self-requested a review
May 10, 2026 08:00
Nuel-09
approved these changes
May 10, 2026
| @ApiOperation({ summary: 'Log a user in' }) | ||
| @ApiBody({ type: LoginDto }) | ||
| @ApiResponse({ status: HttpStatus.OK, description: SYS_MSG.LOGIN_SUCCESSFUL }) | ||
| @ApiResponse({ |
Contributor
There was a problem hiding this comment.
the controller should be clean. All swagger documentation should go into the docs folder then decorate the controller with the docs
Contributor
Author
There was a problem hiding this comment.
resolved in new commit
| try { | ||
| const redisFailCount = await this.redisService.get(`fail:${loginDto.email}`); | ||
| if (redisFailCount !== null && +redisFailCount >= MAX_FAILED_ATTEMPTS) { | ||
| throw new CustomHttpException(`Account locked. Please try again later.`, HttpStatus.FORBIDDEN); |
Contributor
There was a problem hiding this comment.
use system messages for this
Contributor
Author
There was a problem hiding this comment.
resolved in new commit
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/database/migrations/1778406115466-add-unique-user-id-to-auth-metadata.ts`:
- Around line 4-8: The migration's up() adds a UNIQUE constraint
"UQ_auth_metadata_user_id" on "auth_metadata" but will fail if duplicate user_id
rows already exist; update up(queryRunner: QueryRunner) to first dedupe
"auth_metadata" (e.g., use a SQL CTE or queryRunner.query to delete duplicate
rows keeping a single row per user_id based on a deterministic criterion like
MIN(id) or MAX(created_at)), then add the ALTER TABLE ... ADD CONSTRAINT
"UQ_auth_metadata_user_id" UNIQUE ("user_id") statement; reference the up
function, queryRunner, table "auth_metadata" and constraint name
"UQ_auth_metadata_user_id" when making the change.
In `@src/modules/auth/auth.service.ts`:
- Around line 192-194: The auditLog function currently writes raw email (PII) to
logs; change it to avoid logging raw emails by: update auditLog (and its
callers) to prefer logging a userId when available (e.g., userId?: string) and
otherwise log a non-reversible salted hash or truncated/redacted form of the
email; compute the salted hash using a secure HMAC/sha256 with an application
secret/salt (do not store the raw email) and include only the userId OR
hashedEmail in the logger payload (keep reason, event, ip, timestamp); update
callers that pass email to instead pass userId when possible, and when only
email exists, pass the salted hash/redacted value to auditLog.
- Around line 215-236: The transaction catch currently swallows the original
error in the dataSource.manager.transaction call (saving savedSession) by
throwing a generic CustomHttpException; update the catch to capture the original
error (e.g., err), log it via the appropriate logger and include or attach the
original error details when re-throwing (either by passing err.message or err as
a cause) instead of losing it; ensure references are to the existing
savedSession/ dataSource.manager.transaction block and the thrown
CustomHttpException(SYS_MSG.ERROR_OCCURED, HttpStatus.INTERNAL_SERVER_ERROR) so
the log includes the original error and the re-thrown exception preserves useful
diagnostics.
- Around line 167-187: The UPDATE query in auth.service.ts (executed via
this.dataSource.query with parameters [MAX_FAILED_ATTEMPTS,
String(LOCKOUT_MINUTES), metaId]) must clear expired locks so they don't
persist; change the query logic to set locked_until = NULL when locked_until IS
NOT NULL AND locked_until < CURRENT_TIMESTAMP, then compute failed_attempts and
the new locked_until based on the adjusted failed_attempts (so the inner CASE
sees the reset count), ensuring re-locks can occur; update the SQL referenced
(the UPDATE on auth_metadata that uses CASE blocks) accordingly and add an
integration test that simulates 5 failures → expire lock → 1 failure → 4 more
failures → expect 403 to verify the account is re-locked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0d647648-c510-43d0-aabd-edad4a49c7a1
📒 Files selected for processing (6)
src/database/migrations/1778406115466-add-unique-user-id-to-auth-metadata.tssrc/modules/auth/auth.controller.tssrc/modules/auth/auth.service.tssrc/modules/auth/docs/auth-swagger.doc.tssrc/modules/auth/entities/auth-metadata.entity.tssrc/shared/constants/SystemMessages.ts
Comment on lines
+4
to
+8
| public async up(queryRunner: QueryRunner): Promise<void> { | ||
| await queryRunner.query( | ||
| `ALTER TABLE "auth_metadata" ADD CONSTRAINT "UQ_auth_metadata_user_id" UNIQUE ("user_id")` | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Migration may fail on environments with pre-existing duplicate user_id rows.
The previous loadOrCreateMeta implementation (read-then-insert) is racy and could have produced multiple auth_metadata rows per user_id in any environment that already ran it. Adding a UNIQUE constraint will fail with a unique-violation if such duplicates exist, blocking deployment. Dedupe inside up() before adding the constraint.
🛡️ Suggested dedupe step
public async up(queryRunner: QueryRunner): Promise<void> {
+ // Collapse any duplicate rows produced by the prior racy load-or-create path,
+ // keeping the most-recently-updated record per user_id.
+ await queryRunner.query(`
+ DELETE FROM "auth_metadata" a
+ USING "auth_metadata" b
+ WHERE a.user_id = b.user_id
+ AND a.created_at < b.created_at
+ `);
await queryRunner.query(
`ALTER TABLE "auth_metadata" ADD CONSTRAINT "UQ_auth_metadata_user_id" UNIQUE ("user_id")`
);
}📝 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.
Suggested change
| public async up(queryRunner: QueryRunner): Promise<void> { | |
| await queryRunner.query( | |
| `ALTER TABLE "auth_metadata" ADD CONSTRAINT "UQ_auth_metadata_user_id" UNIQUE ("user_id")` | |
| ); | |
| } | |
| public async up(queryRunner: QueryRunner): Promise<void> { | |
| // Collapse any duplicate rows produced by the prior racy load-or-create path, | |
| // keeping the most-recently-updated record per user_id. | |
| await queryRunner.query(` | |
| DELETE FROM "auth_metadata" a | |
| USING "auth_metadata" b | |
| WHERE a.user_id = b.user_id | |
| AND a.created_at < b.created_at | |
| `); | |
| await queryRunner.query( | |
| `ALTER TABLE "auth_metadata" ADD CONSTRAINT "UQ_auth_metadata_user_id" UNIQUE ("user_id")` | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/database/migrations/1778406115466-add-unique-user-id-to-auth-metadata.ts`
around lines 4 - 8, The migration's up() adds a UNIQUE constraint
"UQ_auth_metadata_user_id" on "auth_metadata" but will fail if duplicate user_id
rows already exist; update up(queryRunner: QueryRunner) to first dedupe
"auth_metadata" (e.g., use a SQL CTE or queryRunner.query to delete duplicate
rows keeping a single row per user_id based on a deterministic criterion like
MIN(id) or MAX(created_at)), then add the ALTER TABLE ... ADD CONSTRAINT
"UQ_auth_metadata_user_id" UNIQUE ("user_id") statement; reference the up
function, queryRunner, table "auth_metadata" and constraint name
"UQ_auth_metadata_user_id" when making the change.
Comment on lines
+167
to
+187
| await this.dataSource.query( | ||
| `UPDATE auth_metadata | ||
| SET failed_attempts = CASE | ||
| WHEN locked_until IS NOT NULL AND locked_until < CURRENT_TIMESTAMP | ||
| THEN 1 | ||
| ELSE failed_attempts + 1 | ||
| END, | ||
| locked_until = CASE | ||
| WHEN ( | ||
| CASE | ||
| WHEN locked_until IS NOT NULL AND locked_until < CURRENT_TIMESTAMP | ||
| THEN 1 | ||
| ELSE failed_attempts + 1 | ||
| END | ||
| ) >= $1 | ||
| THEN CURRENT_TIMESTAMP + ($2 || ' minutes')::interval | ||
| ELSE locked_until | ||
| END | ||
| WHERE id = $3`, | ||
| [MAX_FAILED_ATTEMPTS, String(LOCKOUT_MINUTES), metaId] | ||
| ); |
Contributor
There was a problem hiding this comment.
Re-lock bypass: locked_until is never reset to NULL after expiry, so accounts can never be re-locked.
Trace with failed_attempts=5, locked_until=T-15min (expired):
- Bad password: outer
CASEresetsfailed_attemptsto1. InnerCASEforlocked_untilevaluates the new attempt count (1);1 >= 5is false, solocked_untilis kept at the old (expired) value. - Bad password
#2: conditionlocked_until IS NOT NULL AND locked_until < CURRENT_TIMESTAMPis still true →failed_attemptsis reset to1again. - The cycle repeats indefinitely —
failed_attemptsis pinned at1and the account is never re-locked, defeating the brute-force protection. Only a successful login (which setslocked_until = NULLincreateSession) restores normal counting.
Reset locked_until to NULL in the same UPDATE whenever the previous lockout has expired.
🔒 Proposed fix
await this.dataSource.query(
`UPDATE auth_metadata
SET failed_attempts = CASE
WHEN locked_until IS NOT NULL AND locked_until < CURRENT_TIMESTAMP
THEN 1
ELSE failed_attempts + 1
END,
locked_until = CASE
+ WHEN locked_until IS NOT NULL AND locked_until < CURRENT_TIMESTAMP
+ AND (failed_attempts + 1) < $1
+ THEN NULL
WHEN (
CASE
WHEN locked_until IS NOT NULL AND locked_until < CURRENT_TIMESTAMP
THEN 1
ELSE failed_attempts + 1
END
) >= $1
THEN CURRENT_TIMESTAMP + ($2 || ' minutes')::interval
ELSE locked_until
END
WHERE id = $3`,
[MAX_FAILED_ATTEMPTS, String(LOCKOUT_MINUTES), metaId]
);Please add an integration test that walks 5 failures → wait/expire → 1 failure → 4 more failures → expect 403 again.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/auth/auth.service.ts` around lines 167 - 187, The UPDATE query in
auth.service.ts (executed via this.dataSource.query with parameters
[MAX_FAILED_ATTEMPTS, String(LOCKOUT_MINUTES), metaId]) must clear expired locks
so they don't persist; change the query logic to set locked_until = NULL when
locked_until IS NOT NULL AND locked_until < CURRENT_TIMESTAMP, then compute
failed_attempts and the new locked_until based on the adjusted failed_attempts
(so the inner CASE sees the reset count), ensuring re-locks can occur; update
the SQL referenced (the UPDATE on auth_metadata that uses CASE blocks)
accordingly and add an integration test that simulates 5 failures → expire lock
→ 1 failure → 4 more failures → expect 403 to verify the account is re-locked.
TheCodeGhinux
approved these changes
May 10, 2026
22 tasks
sage-ali
changed the title
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
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.
Pull Request
Description
This PR implements the secure Login Flow & Rate Limiting system (BE-007). It introduces a robust authentication mechanism designed to protect users against brute-force attacks and credential stuffing through a multi-layered security shield involving PostgreSQL for persistent metadata and Redis for session orchestration.
Key Changes:
POST /auth/loginwith Bcrypt credential validation.user_sessionstable.Related Issue
Fixes [BE-007 Login Flow & Rate Limiting](BE-007 Login Flow & Rate Limiting)
Type of Change
How Has This Been Tested?
Test Evidence
Screenshots (if applicable)
Documentation Screenshots (if applicable)
Checklist
Additional Notes
To ensure consistency across distributed nodes, the lockout logic relies on
CURRENT_TIMESTAMPfrom the database. The system also includes a persistence fallback; if Redis is unavailable, security metadata remains enforced via the PostgreSQLauth_metadatatable. Standardized error codes (401, 403, 429) have been implemented as per the acceptance criteria.Summary by CodeRabbit
New Features
Security Improvements
Documentation