feat: add unrecognized device email alerts (#168)#224
Conversation
Implemented device fingerprinting to detect logins from new devices. Unrecognized logins now trigger an async email alert with an account lock link via Mailpit. Also fixed local SMTP routing for Docker.
|
All contributors have signed the CLA. ✅ Thank you! |
📝 WalkthroughWalkthroughAdds device fingerprint tracking to the login flow, sends unrecognized-login alert emails with lock tokens, exposes a lock-account endpoint, and updates startup, test, and compose wiring for the new device and email paths. ChangesDevice Fingerprinting and Account Lock Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I have read the CLA and agree to its terms |
![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
🧹 Nitpick comments (2)
internal/testutils/setup.go (1)
38-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCapture the lock token in the mock too.
The new feature’s critical contract is the lock link, but this mock only records the recipient, so tests cannot assert that a usable token was generated and passed through. Storing
lockTokenhere would make the new alert flow much easier to verify. As per coding guidelines, "internal/testutils/**/*.go: Provide helper utilities ininternal/testutils/for DB setup, fixtures, and mock clients used across tests".🤖 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 `@internal/testutils/setup.go` around lines 38 - 43, The MockEmailSender.SendUnrecognizedLoginAlert helper only records the recipient email, so tests can’t verify the lock token is generated and passed through. Update this mock to also store the lockToken in its LastEmail map alongside the unrecognized_login entry, using the existing MockEmailSender and SendUnrecognizedLoginAlert symbols so the alert flow can assert both values.Source: Coding guidelines
internal/service/token_service.go (1)
188-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse service-level typed errors for lock-token failures.
Line 194 introduces a raw string error, and Line 189 returns parser-specific errors directly; this weakens stable handler-side error mapping. Return a service error type from
service/errors.goand wrap/normalize parse failures with it.As per coding guidelines,
internal/{service,handler}/**/*.go: Implement custom error types inservice/errors.goand convert service errors to HTTP status codes in handlers.🤖 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 `@internal/service/token_service.go` around lines 188 - 195, Use service-level typed errors for lock-token failures in token_service.go: the parse failure path and the invalid claims/token path should not return raw parser errors or a string error. Update the lock-token flow around the JWT parsing and the JWTClaims validation to normalize those failures into a custom error type defined in service/errors.go, then ensure callers can map that typed error consistently in the handler layer.Source: Coding guidelines
🤖 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 `@cmd/server/main.go`:
- Around line 53-56: The refresh token backfill in main should only be
downgraded when the refresh_tokens table is actually missing, not for every Exec
failure. Update the startup migration logic around db.Exec and the existing
backfill block to first detect absence with HasTable or an equivalent
missing-table check, and keep treating all other UPDATE refresh_tokens SET
family_id = id failures as fatal so startup stops on real migration, permission,
locking, or query errors.
In `@docker-compose.yml`:
- Line 15: The app startup order in the Compose configuration is missing a
dependency on Mailpit, so SMTP alerts can race `auth-mailpit` before it is
ready. Update the service definition that currently depends on Postgres and
Redis to also wait for the Mailpit service, using the existing app service block
and the `SMTP_HOST=auth-mailpit` configuration as the anchor, so local startup
does not attempt SMTP delivery too early.
In `@internal/handler/auth_handler.go`:
- Around line 173-180: The LockAccount failure path in auth_handler.go currently
collapses all service errors into HTTP 400 and still uses the old error
envelope, so update the handler around h.authService.LockAccount to distinguish
typed service errors (for example invalid token vs LockUser/RevokeAllUserTokens
failures) and map them to the correct 4xx/5xx status codes. Return the required
JSON shape with error and code for this endpoint, and if utils.ErrorResponse is
the shared response path, extend that helper first so the Login/lock-account
handler stays consistent with the rest of the file.
In `@internal/models/device_fingerprint.go`:
- Around line 12-13: The DeviceFingerprint model currently allows duplicate rows
for the same user and fingerprint under concurrent logins because `UserID` and
`FingerprintHash` are only indexed, not uniquely constrained. Update the
`DeviceFingerprint` struct tags to enforce a composite uniqueness rule on
`user_id` + `fingerprint_hash` at the DB level, and ensure any create/upsert
logic that uses this model handles the resulting unique constraint correctly.
In `@internal/routes/routes.go`:
- Line 156: The auth.GET("/lock-account", authHandler.LockAccount) route should
not perform the destructive lock action directly because it can be triggered by
scanners or previews. Update the LockAccount flow in authHandler so the GET
endpoint only lands on a confirmation page, then move the actual account-lock
operation to a POST-based one-time submission handled by the same LockAccount
logic or a dedicated confirm action. Ensure the email link points to the safe
confirmation page rather than executing the lock immediately.
In `@internal/service/auth_service.go`:
- Around line 307-315: Return typed service errors from the lock-account flow
instead of ad-hoc errors.New values. In AuthService’s lock logic around
ValidateLockToken and LockUser, map token validation failures to the existing
invalid/expired lock-token error from service/errors.go, and map repository
failures to the service-defined internal lock-account error so the handler can
distinguish 4xx from 5xx responses. Keep the control flow the same, but replace
the generic error creation with the shared error values used elsewhere in the
service package.
- Around line 646-675: The fingerprint lookup error path in auth_service.go is
being treated like a missing device, which incorrectly creates a new
DeviceFingerprint and sends an unrecognized login alert. In the
FindByFingerprint handling inside the device check flow, distinguish a real
error from a nil result: if s.deviceRepo.FindByFingerprint returns an error, log
it and return or otherwise skip the newDevice creation and alert goroutine. Keep
the unrecognized-device branch only for the device == nil case when no error
occurred, so the existing deviceRepo.Create, GenerateLockToken, and
SendUnrecognizedLoginAlert logic only runs on true cache misses.
- Around line 318-322: In the account-lock flow in auth_service’s session
revocation path, the error from RevokeAllUserTokens is currently ignored, so the
method can report success even when tokens were not revoked. Update the method
that handles locking the account to check and propagate the RevokeAllUserTokens
error before continuing, and only write the audit event or return nil after
revocation succeeds.
In `@internal/service/email_service.go`:
- Line 107: The lock-account email link in EmailService should not point
directly to the state-changing API endpoint. Update EmailService’s lockURL
construction so the email sends users to a non-mutating confirmation page first,
and have the actual account lock happen only after an explicit confirmed action
such as a POST handled by the auth flow.
In `@internal/utils/device.go`:
- Around line 13-18: The IP normalization in device hashing only handles IPv4,
so update the subnet masking logic in the device utility to also normalize IPv6
addresses before hashing. Extend the existing subnet derivation in the function
handling ipAddress parsing so IPv6 clients are reduced to a stable subnet/prefix
representation rather than using the full address, while keeping the current
IPv4 behavior unchanged. Use the same hashing input path after normalization so
the same client does not appear as a new device on each IPv6 address change.
---
Nitpick comments:
In `@internal/service/token_service.go`:
- Around line 188-195: Use service-level typed errors for lock-token failures in
token_service.go: the parse failure path and the invalid claims/token path
should not return raw parser errors or a string error. Update the lock-token
flow around the JWT parsing and the JWTClaims validation to normalize those
failures into a custom error type defined in service/errors.go, then ensure
callers can map that typed error consistently in the handler layer.
In `@internal/testutils/setup.go`:
- Around line 38-43: The MockEmailSender.SendUnrecognizedLoginAlert helper only
records the recipient email, so tests can’t verify the lock token is generated
and passed through. Update this mock to also store the lockToken in its
LastEmail map alongside the unrecognized_login entry, using the existing
MockEmailSender and SendUnrecognizedLoginAlert symbols so the alert flow can
assert both values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a2e795f-9d7f-4894-8d6c-1dfb33249bad
📒 Files selected for processing (13)
.env.examplecmd/server/main.godocker-compose.ymlinternal/handler/auth_handler.gointernal/models/device_fingerprint.gointernal/repository/device_repository.gointernal/routes/routes.gointernal/service/auth_service.gointernal/service/email_service.gointernal/service/token_service.gointernal/testutils/setup.gointernal/utils/device.gotemplates/unrecognized_login.html
| // Backfill existing tokens using their own ID as the family ID | ||
| if err := db.Exec("UPDATE refresh_tokens SET family_id = id WHERE family_id IS NULL").Error; err != nil { | ||
| log.Fatal("Failed to backfill family_id for existing refresh tokens:", err) | ||
| log.Printf("Warning: Failed to backfill family_id for existing refresh tokens (expected on fresh DB): %v", err) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Only downgrade the backfill error when the table is actually absent.
After this change, any failure in UPDATE refresh_tokens SET family_id = id ... is treated as "expected on fresh DB". On an existing database, the same branch would also swallow real permission/locking/query errors and continue with partially migrated token data. Check HasTable/missing-table explicitly and keep failing startup for all other backfill failures.
🤖 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 `@cmd/server/main.go` around lines 53 - 56, The refresh token backfill in main
should only be downgraded when the refresh_tokens table is actually missing, not
for every Exec failure. Update the startup migration logic around db.Exec and
the existing backfill block to first detect absence with HasTable or an
equivalent missing-table check, and keep treating all other UPDATE
refresh_tokens SET family_id = id failures as fatal so startup stops on real
migration, permission, locking, or query errors.
| - PORT=8080 | ||
| - DATABASE_URL=host=postgres user=postgres password=postgres dbname=auth_db port=5432 sslmode=disable | ||
| - REDIS_URL=redis://auth-redis:6379 | ||
| - SMTP_HOST=auth-mailpit |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add Mailpit to the app startup dependencies.
The app now sends SMTP traffic to auth-mailpit, but Compose still only orders startup against Postgres and Redis. In local Docker runs, the first unrecognized-login alert can race Mailpit startup and fail with connection refused.
Also applies to: 52-60
🤖 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 `@docker-compose.yml` at line 15, The app startup order in the Compose
configuration is missing a dependency on Mailpit, so SMTP alerts can race
`auth-mailpit` before it is ready. Update the service definition that currently
depends on Postgres and Redis to also wait for the Mailpit service, using the
existing app service block and the `SMTP_HOST=auth-mailpit` configuration as the
anchor, so local startup does not attempt SMTP delivery too early.
| if token == "" { | ||
| c.JSON(http.StatusBadRequest, utils.ValidationErrorResponse("Token is required")) | ||
| return | ||
| } | ||
|
|
||
| if err := h.authService.LockAccount(token); err != nil { | ||
| c.JSON(http.StatusBadRequest, utils.ErrorResponse("Failed to lock account", err)) | ||
| return |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Map lock-account failures to the right status/code payload.
This branch turns every LockAccount error into 400, so an invalid token and an internal LockUser/RevokeAllUserTokens failure become indistinguishable to clients. It also keeps the old helper envelope instead of the handler contract required here. Please translate typed service errors into 4xx/5xx responses and emit the required {"error","code"} shape for this endpoint; if utils.ErrorResponse is still the shared path, extend that helper first so this handler stays consistent with the rest of the file. As per coding guidelines, "internal/handler/**/*.go: Return JSON error responses in format {\"error\": \"message\", \"code\": \"ERROR_CODE\"} from HTTP handlers" and "internal/{service,handler}/**/*.go: Implement custom error types in service/errors.go and convert service errors to HTTP status codes in handlers". Based on learnings, this codebase currently centralizes handler errors through utils.ErrorResponse(...), so changing the helper first avoids introducing a one-off response shape.
🤖 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 `@internal/handler/auth_handler.go` around lines 173 - 180, The LockAccount
failure path in auth_handler.go currently collapses all service errors into HTTP
400 and still uses the old error envelope, so update the handler around
h.authService.LockAccount to distinguish typed service errors (for example
invalid token vs LockUser/RevokeAllUserTokens failures) and map them to the
correct 4xx/5xx status codes. Return the required JSON shape with error and code
for this endpoint, and if utils.ErrorResponse is the shared response path,
extend that helper first so the Login/lock-account handler stays consistent with
the rest of the file.
Sources: Coding guidelines, Learnings
| UserID string `gorm:"type:uuid;not null;index" json:"userId"` | ||
| FingerprintHash string `gorm:"not null;index" json:"fingerprintHash"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce uniqueness for user_id + fingerprint_hash.
The current read-then-create flow can insert duplicate device rows under concurrent logins without a DB-level composite unique constraint.
Suggested fix
- UserID string `gorm:"type:uuid;not null;index" json:"userId"`
- FingerprintHash string `gorm:"not null;index" json:"fingerprintHash"`
+ UserID string `gorm:"type:uuid;not null;uniqueIndex:idx_user_fingerprint" json:"userId"`
+ FingerprintHash string `gorm:"not null;uniqueIndex:idx_user_fingerprint" json:"fingerprintHash"`📝 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.
| UserID string `gorm:"type:uuid;not null;index" json:"userId"` | |
| FingerprintHash string `gorm:"not null;index" json:"fingerprintHash"` | |
| UserID string `gorm:"type:uuid;not null;uniqueIndex:idx_user_fingerprint" json:"userId"` | |
| FingerprintHash string `gorm:"not null;uniqueIndex:idx_user_fingerprint" json:"fingerprintHash"` |
🤖 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 `@internal/models/device_fingerprint.go` around lines 12 - 13, The
DeviceFingerprint model currently allows duplicate rows for the same user and
fingerprint under concurrent logins because `UserID` and `FingerprintHash` are
only indexed, not uniquely constrained. Update the `DeviceFingerprint` struct
tags to enforce a composite uniqueness rule on `user_id` + `fingerprint_hash` at
the DB level, and ensure any create/upsert logic that uses this model handles
the resulting unique constraint correctly.
| auth.POST("/resend-verification", authHandler.ResendVerification) | ||
| auth.POST("/forgot-password", authHandler.ForgotPassword) | ||
| auth.POST("/reset-password", authHandler.ResetPassword) | ||
| auth.GET("/lock-account", authHandler.LockAccount) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Avoid a destructive action on a GET route.
This endpoint locks the account, and the alert email links to it directly. Mail scanners, link preview bots, and safe-browsing crawlers routinely fetch GET links automatically, which can lock accounts without the user ever choosing to do so. Make the email land on a confirmation page and perform the lock with a POST/one-time form submission instead.
🤖 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 `@internal/routes/routes.go` at line 156, The auth.GET("/lock-account",
authHandler.LockAccount) route should not perform the destructive lock action
directly because it can be triggered by scanners or previews. Update the
LockAccount flow in authHandler so the GET endpoint only lands on a confirmation
page, then move the actual account-lock operation to a POST-based one-time
submission handled by the same LockAccount logic or a dedicated confirm action.
Ensure the email link points to the safe confirmation page rather than executing
the lock immediately.
| userID, err := s.tokenService.ValidateLockToken(tokenString) | ||
| if err != nil { | ||
| return errors.New("invalid or expired lock token") | ||
| } | ||
|
|
||
| // Lock the account (100 years) | ||
| lockedUntil := time.Now().Add(100 * 365 * 24 * time.Hour) | ||
| if err := s.userRepo.LockUser(userID, lockedUntil); err != nil { | ||
| return errors.New("failed to lock account") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Return typed lock-account errors here.
These ad-hoc errors.New(...) values erase whether the failure was token validation or an internal repository problem, so the handler cannot do the required 4xx/5xx mapping for this endpoint. Please return service-defined errors from service/errors.go instead.
🤖 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 `@internal/service/auth_service.go` around lines 307 - 315, Return typed
service errors from the lock-account flow instead of ad-hoc errors.New values.
In AuthService’s lock logic around ValidateLockToken and LockUser, map token
validation failures to the existing invalid/expired lock-token error from
service/errors.go, and map repository failures to the service-defined internal
lock-account error so the handler can distinguish 4xx from 5xx responses. Keep
the control flow the same, but replace the generic error creation with the
shared error values used elsewhere in the service package.
Source: Coding guidelines
| // Revoke all existing sessions for security | ||
| s.tokenRepo.RevokeAllUserTokens(userID) | ||
|
|
||
| // Audit Log | ||
| s.auditService.LogEvent(&userID, "ACCOUNT_LOCKED_BY_USER", "USER", userID, "", "", map[string]interface{}{"reason": "unrecognized_device_alert"}) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Don't report success before session revocation succeeds.
RevokeAllUserTokens returns an error, but this method ignores it and still returns nil. If that update fails, the account is marked locked while existing refresh tokens remain valid.
Suggested fix
- // Revoke all existing sessions for security
- s.tokenRepo.RevokeAllUserTokens(userID)
+ // Revoke all existing sessions for security
+ if err := s.tokenRepo.RevokeAllUserTokens(userID); err != nil {
+ return errors.New("failed to revoke user sessions")
+ }
// Audit Log
- s.auditService.LogEvent(&userID, "ACCOUNT_LOCKED_BY_USER", "USER", userID, "", "", map[string]interface{}{"reason": "unrecognized_device_alert"})
+ if err := s.auditService.LogEvent(&userID, "ACCOUNT_LOCKED_BY_USER", "USER", userID, "", "", map[string]interface{}{"reason": "unrecognized_device_alert"}); err != nil {
+ log.Printf("failed to write audit log for locked account %s: %v", userID, err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Revoke all existing sessions for security | |
| s.tokenRepo.RevokeAllUserTokens(userID) | |
| // Audit Log | |
| s.auditService.LogEvent(&userID, "ACCOUNT_LOCKED_BY_USER", "USER", userID, "", "", map[string]interface{}{"reason": "unrecognized_device_alert"}) | |
| // Revoke all existing sessions for security | |
| if err := s.tokenRepo.RevokeAllUserTokens(userID); err != nil { | |
| return errors.New("failed to revoke user sessions") | |
| } | |
| // Audit Log | |
| if err := s.auditService.LogEvent(&userID, "ACCOUNT_LOCKED_BY_USER", "USER", userID, "", "", map[string]interface{}{"reason": "unrecognized_device_alert"}); err != nil { | |
| log.Printf("failed to write audit log for locked account %s: %v", userID, err) | |
| } |
🤖 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 `@internal/service/auth_service.go` around lines 318 - 322, In the account-lock
flow in auth_service’s session revocation path, the error from
RevokeAllUserTokens is currently ignored, so the method can report success even
when tokens were not revoked. Update the method that handles locking the account
to check and propagate the RevokeAllUserTokens error before continuing, and only
write the audit event or return nil after revocation succeeds.
|
|
||
| // SendUnrecognizedLoginAlert sends an alert for an unfamiliar device login | ||
| func (s *EmailService) SendUnrecognizedLoginAlert(email, ip, userAgent, lockToken, appURL string) error { | ||
| lockURL := fmt.Sprintf("%s/api/auth/lock-account?token=%s", appURL, lockToken) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Avoid one-click state-changing GET lock links in email alerts.
Line 107 builds a direct /api/auth/lock-account?token=... link. Since that endpoint performs the lock action, mail-client link prefetch/scanning can unintentionally lock accounts. Send users to a non-mutating confirmation page first, then execute lock via explicit confirmed action (e.g., POST).
🤖 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 `@internal/service/email_service.go` at line 107, The lock-account email link
in EmailService should not point directly to the state-changing API endpoint.
Update EmailService’s lockURL construction so the email sends users to a
non-mutating confirmation page first, and have the actual account lock happen
only after an explicit confirmed action such as a POST handled by the auth flow.
|
I have read the CLA and agree to its terms. |
…#223) * fix: align docker compose environment variables * fix: address CodeRabbit review feedback
|
@devd-328 handle the coderabbit suggestion and resolve the conflict |
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docker-compose.yml (2)
16-17: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse the env names the app reads
JWT_ACCESS_SECRETis ignored here;internal/config/config.goloadsJWT_SECRETandJWT_REFRESH_SECRET. Thesecret/refreshfallbacks are also below the 32-byte minimum, so this stack still depends on.envsupplying valid secrets.🤖 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 `@docker-compose.yml` around lines 16 - 17, The JWT secret environment variables in the compose configuration do not match what internal/config/config.go actually reads, so update the service definition to use the same names as the app’s config loader, especially JWT_SECRET and JWT_REFRESH_SECRET. Also remove the weak default fallbacks currently used for these values and ensure the stack relies on properly sized secrets being provided through the environment or .env, with the compose entries aligned to the existing config constants and secret-loading logic.
15-15: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSet the Mailpit SMTP port in compose
docker-compose.yml:15still falls back to port587, butauth-mailpitonly listens on1025, so these alerts can’t be delivered in this setup.SMTP_USE_TLSis not used by the sender; the missing port override is the blocker.🤖 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 `@docker-compose.yml` at line 15, The SMTP configuration for the Mailpit-backed alerts is incomplete in the compose setup, so the sender still defaults to the wrong port. Update the docker-compose environment for the mail sender alongside SMTP_HOST so it explicitly uses the Mailpit port that auth-mailpit listens on, and do this in the same section that defines SMTP_HOST and related SMTP_* settings. Make sure the fix is applied in the compose service configuration rather than relying on SMTP_USE_TLS, since that setting is not used by the sender.
🤖 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.
Outside diff comments:
In `@docker-compose.yml`:
- Around line 16-17: The JWT secret environment variables in the compose
configuration do not match what internal/config/config.go actually reads, so
update the service definition to use the same names as the app’s config loader,
especially JWT_SECRET and JWT_REFRESH_SECRET. Also remove the weak default
fallbacks currently used for these values and ensure the stack relies on
properly sized secrets being provided through the environment or .env, with the
compose entries aligned to the existing config constants and secret-loading
logic.
- Line 15: The SMTP configuration for the Mailpit-backed alerts is incomplete in
the compose setup, so the sender still defaults to the wrong port. Update the
docker-compose environment for the mail sender alongside SMTP_HOST so it
explicitly uses the Mailpit port that auth-mailpit listens on, and do this in
the same section that defines SMTP_HOST and related SMTP_* settings. Make sure
the fix is applied in the compose service configuration rather than relying on
SMTP_USE_TLS, since that setting is not used by the sender.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ea450620-8c42-439a-8653-aa71fce72d08
📒 Files selected for processing (4)
docker-compose.ymlinternal/service/auth_service.gointernal/service/email_service.gotemplates/unrecognized_login.html
✅ Files skipped from review due to trivial changes (1)
- templates/unrecognized_login.html
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/service/email_service.go
- internal/service/auth_service.go
|
@devd-328 handle the coderabbit sugesstions |
|
@devd-328 please attend to the coderabbit suggestions to get you pr checked and merged. Also please update the pr description linking the issue you solved |
4 participants
Implemented device fingerprinting to detect logins from new devices. Unrecognized logins now trigger an async email alert with an account lock link via Mailpit. Also fixed local SMTP routing for Docker.
Checklist
I have read the CLA and agree to its terms.on this PR.Summary by CodeRabbit