refactor/issue-25 Implement lock/unlock user admin action#44
Conversation
|
Warning Review limit reached
More reviews will be available in 15 minutes and 39 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR implements admin-controlled user account locking and unlocking with validation, session management, and audit logging. Changes span the data model (lock state checking), service business logic (authorization and persistence), HTTP handlers (endpoint mapping), and comprehensive integration tests for both service and HTTP layers. ChangesLock/unlock account management
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 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 |
|
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: 3
🧹 Nitpick comments (1)
internal/service/auth_service_test.go (1)
87-125: ⚡ Quick winCover the security-critical side effects in these service tests.
These tests only assert
LockedUntil, so they would still pass if lock/unlock wrote the wrong audit actor or failed to revoke sessions. Add a refresh token before locking, then assert it is revoked and that the resulting audit record attributes the action toadmin.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 `@internal/service/auth_service_test.go` around lines 87 - 125, The tests TestAuthService_LockUser and TestAuthService_UnlockUser only assert LockedUntil; create a refresh token/session for the target user (using the app's session/refresh token repository, e.g. repository.NewSessionRepository or NewRefreshTokenRepository) before calling authSvc.LockUser, then after LockUser assert that the refresh token is revoked/invalidated; also fetch the audit record for the lock action (via the audit repository, e.g. repository.NewAuditRepository or similar) and assert its actor/initiator equals admin.ID. Do the same in TestAuthService_UnlockUser: add a refresh token before LockUser, assert it was revoked by LockUser, then after authSvc.UnlockUser assert any session state restored if applicable and verify the unlock audit record actor equals admin.ID. Ensure you use existing helpers registerUser, promoteAdmin and authSvc.LockUser/UnlockUser to locate where to add these checks.
🤖 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 `@internal/service/auth_service.go`:
- Line 814: The audit call is recording the target user as the acting user;
change the first argument passed to s.auditService.LogEvent from &userId to
&adminId so AuditLog.UserID contains the admin performing the action while
keeping userId as the entityID (e.g., in the call
s.auditService.LogEvent(&adminId, "USER_LOCKED", "USER", userId, ipAddress,
userAgent, ...)); apply the same fix to the analogous call around the unlock
flow noted (line ~832) so all admin actions record &adminId as the actor.
- Around line 791-794: The current handling around s.userRepo.FindByID in
auth_service.go unconditionally maps any repository error to ErrUserNotFound;
change the error handling in both places (the FindByID call at the shown block
and the similar block at 819-822) to preserve repository failures: after calling
s.userRepo.FindByID(userId) check if errors.Is(err, repository.ErrUserNotFound)
and only then return ErrUserNotFound, otherwise return or wrap the original err
(or a 500-level service error) so DB outages/timeouts are not converted to 404s;
update the branches around s.userRepo.FindByID and ErrUserNotFound accordingly.
- Around line 807-813: The current flow updates the user's locked_until via
s.userRepo.Update and then calls s.tokenRepo.RevokeAllUserTokens, which can
leave the account locked but tokens not revoked if the second call fails; change
this to an atomic or compensating flow: either (1) perform both operations
inside a shared transaction if both repositories share the same datastore (wrap
userRepo.Update and tokenRepo.RevokeAllUserTokens in a transaction API and
commit/rollback together), or (2) if token revocation is external, reverse the
order to call s.tokenRepo.RevokeAllUserTokens first then s.userRepo.Update and
on Update failure call a compensating s.tokenRepo.RestoreTokens or retry revoke,
and/or make RevokeAllUserTokens idempotent and modify the handler to catch
ErrAlreadyLocked (from the Update path) and still call RevokeAllUserTokens so
retries succeed; update the code around s.userRepo.Update,
s.tokenRepo.RevokeAllUserTokens, and the ErrAlreadyLocked handling accordingly.
---
Nitpick comments:
In `@internal/service/auth_service_test.go`:
- Around line 87-125: The tests TestAuthService_LockUser and
TestAuthService_UnlockUser only assert LockedUntil; create a refresh
token/session for the target user (using the app's session/refresh token
repository, e.g. repository.NewSessionRepository or NewRefreshTokenRepository)
before calling authSvc.LockUser, then after LockUser assert that the refresh
token is revoked/invalidated; also fetch the audit record for the lock action
(via the audit repository, e.g. repository.NewAuditRepository or similar) and
assert its actor/initiator equals admin.ID. Do the same in
TestAuthService_UnlockUser: add a refresh token before LockUser, assert it was
revoked by LockUser, then after authSvc.UnlockUser assert any session state
restored if applicable and verify the unlock audit record actor equals admin.ID.
Ensure you use existing helpers registerUser, promoteAdmin and
authSvc.LockUser/UnlockUser to locate where to add these checks.
🪄 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: 9b9a1b21-5c9e-4ceb-9191-d4ad07a9edda
📒 Files selected for processing (4)
internal/handler/admin_handler.gointernal/handler/admin_handler_test.gointernal/service/auth_service.gointernal/service/auth_service_test.go
…ment-lock/unlock-user-admin-action
|
@Samikshaaggarwal handle all the coderabbit suggestions |
|
@Samikshaaggarwal ErrUserNotFound in auth_service.go conflicts with repository.ErrUserNotFound |
|
@Samikshaaggarwal ErrAlreadyLocked check is wrong — permanently locked users won't be caught |
|
@Samikshaaggarwal Test file is in the wrong package — package handler instead of package handler_test |
|
@Samikshaaggarwal UnlockUser handler has its switch indented inside the if but return is outside — wrong control flow |
|
@Samikshaaggarwal add test for the LockUser success path or token revocation |
|
@Samikshaaggarwal Service tests use file::memory:?cache=shared — tests can interfere with each other |
|
@Samikshaaggarwal adminId parameter is unused in UnlockUser service method |
|
run go fmt locally |
|
@Samikshaaggarwal adminId should be adminID per Go conventions |
…on' of https://github.com/samikshaaggarwal/auth-server into refactor/issue-25-implement-lock/unlock-user-admin-action
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/handler/admin_handler_test.go (1)
111-115:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse context-aware
httptestrequests in admin handler testsLine 111 and Line 171 trigger the
noctxlint warning; switch both call sites tohttptest.NewRequestWithContext(context.Background(), ...)(the CI workflow runs tests/build and doesn’t invokegolangci-lint, but the lint target exists via the Makefile).Proposed fix
import ( + "context" "net/http" "net/http/httptest" "testing" @@ - req := httptest.NewRequest( + req := httptest.NewRequestWithContext( + context.Background(), http.MethodPost, "/api/admin/users/"+tt.userID+"/lock", nil, ) @@ - req := httptest.NewRequest( + req := httptest.NewRequestWithContext( + context.Background(), http.MethodPost, "/api/admin/users/"+tt.userID+"/unlock", nil, )Also applies to: 171-175
🤖 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/admin_handler_test.go` around lines 111 - 115, Replace plain httptest.NewRequest calls with context-aware versions: change httptest.NewRequest(...) to httptest.NewRequestWithContext(context.Background(), ...) at both call sites (the one creating POST "/api/admin/users/"+tt.userID+"/lock" and the other at lines around 171-175). Also add the context import (import "context") to internal/handler/admin_handler_test.go if it's not already imported so the context.Background() symbol resolves.
🧹 Nitpick comments (1)
internal/handler/admin_handler_test.go (1)
146-167: ⚡ Quick winAdd unlock success-path assertion using the already prepared
lockedUser.
lockedUseris set up and locked, but this test only validates error branches. Add a200 OKcase forlockedUser.IDto verify the handler’s success mapping as well.🤖 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/admin_handler_test.go` around lines 146 - 167, Add a success-case to the tests table that uses the prepared lockedUser to assert the unlock handler returns 200 OK: include an entry like { "unlock success", lockedUser.ID, http.StatusOK } (or similar name) alongside the existing cases so the test validates the success path for lockedUser.ID after calling the handler; ensure the test uses the same token (tokenSvc.GenerateAccessToken(admin)) and request setup as the other cases so the handler paths for lockedUser are exercised.
🤖 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 `@internal/repository/user_repository.go`:
- Around line 81-94: LockUser and UnlockUser currently treat zero-row updates as
success; change both methods in UserRepository (the functions LockUser and
UnlockUser that call
r.db.Model(&models.User{})...Where(...).Update/Updates(...)) to capture the gorm
result, return result.Error if non-nil, and if result.RowsAffected == 0 return
ErrUserNotFound instead of nil; keep the existing update logic and ensure
ErrUserNotFound (the package-level sentinel error) is referenced.
In `@internal/service/auth_service_integration_test.go`:
- Around line 91-115: The tests TestAuthService_LockUser (and the sibling lock
test at lines 117-133) currently assert token revocation by counting
RefreshToken rows but never create any tokens first, so revocation isn't
actually verified; update these tests to create at least one refresh token tied
to the test user (e.g., using the existing registerUser helper and the
RefreshToken model or a test helper like createRefreshToken), assert the token
exists pre-lock, call authSvc.LockUser(user.ID, admin.ID, ...), then re-query
the RefreshToken count or lookup to assert the token(s) were removed (verify
zero afterwards); reference LockUser, TestAuthService_LockUser, RefreshToken,
and the repository/user helpers when adding the token creation and pre/post
assertions.
---
Outside diff comments:
In `@internal/handler/admin_handler_test.go`:
- Around line 111-115: Replace plain httptest.NewRequest calls with
context-aware versions: change httptest.NewRequest(...) to
httptest.NewRequestWithContext(context.Background(), ...) at both call sites
(the one creating POST "/api/admin/users/"+tt.userID+"/lock" and the other at
lines around 171-175). Also add the context import (import "context") to
internal/handler/admin_handler_test.go if it's not already imported so the
context.Background() symbol resolves.
---
Nitpick comments:
In `@internal/handler/admin_handler_test.go`:
- Around line 146-167: Add a success-case to the tests table that uses the
prepared lockedUser to assert the unlock handler returns 200 OK: include an
entry like { "unlock success", lockedUser.ID, http.StatusOK } (or similar name)
alongside the existing cases so the test validates the success path for
lockedUser.ID after calling the handler; ensure the test uses the same token
(tokenSvc.GenerateAccessToken(admin)) and request setup as the other cases so
the handler paths for lockedUser are exercised.
🪄 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: 65e6f91c-847d-48af-8416-8e02d0003469
📒 Files selected for processing (6)
internal/handler/admin_handler.gointernal/handler/admin_handler_test.gointernal/models/user.gointernal/repository/user_repository.gointernal/service/auth_service.gointernal/service/auth_service_integration_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/handler/admin_handler.go
|
|
@Samikshaaggarwal can you open a fresh new pr it got too much chnages to review now |
|
@roshankumar0036singh |
2 participants
Checklist
I have read the CLA and agree to its terms.on this PR.Summary by CodeRabbit
New Features
Tests