refactor/issue-25 | Implemented lock/unlock user admin action#49
Open
Samikshaaggarwal wants to merge 3 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 39 minutes and 57 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 (1)
📝 WalkthroughWalkthroughThis PR adds admin-controlled user lock/unlock: service-level DB-backed LockUser/UnlockUser with sentinel errors, repository methods and model support, handler mappings to HTTP status codes, updated wiring to provide DB to the service, and integration tests for service and handler behavior. ChangesAdmin User Locking and Unlocking
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 |
3 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
internal/service/auth_service_integration_test.go (1)
179-188: ⚡ Quick winIsolate admin-target behavior in
TestAuthService_LockUser_AdminLock.At Line 187, the actor is a non-admin (
user.ID). That couples this test to caller-authorization behavior instead of strictly validating the “target account is admin” rule (ErrAdminLock). Use two admin users (actor + target) to keep intent stable.Refactor sketch
func TestAuthService_LockUser_AdminLock(t *testing.T) { authSvc, db, _ := testutils.SetupIntegrationTest(t) - admin := registerUser(t, authSvc, "admin5@example.com") - user := registerUser(t, authSvc, "user5@example.com") + targetAdmin := registerUser(t, authSvc, "target-admin@example.com") + actingAdmin := registerUser(t, authSvc, "acting-admin@example.com") - promoteAdmin(t, db, admin.ID) + promoteAdmin(t, db, targetAdmin.ID) + promoteAdmin(t, db, actingAdmin.ID) - err := authSvc.LockUser(admin.ID, user.ID, "", "") + err := authSvc.LockUser(targetAdmin.ID, actingAdmin.ID, "", "") assert.ErrorIs(t, err, service.ErrAdminLock) }🤖 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_integration_test.go` around lines 179 - 188, TestAuthService_LockUser_AdminLock currently uses a non-admin actor (user.ID) which makes the test exercise caller-authorization instead of the “target is admin” rule; change the setup so both actor and target are admins: call registerUser twice for adminActor and adminTarget, call promoteAdmin(t, db, adminActor.ID) and promoteAdmin(t, db, adminTarget.ID), then invoke authSvc.LockUser(adminActor.ID, adminTarget.ID, "", "") and assert.ErrorIs(t, err, service.ErrAdminLock). Update references in the test function TestAuthService_LockUser_AdminLock and keep use of registerUser, promoteAdmin, authSvc.LockUser, and service.ErrAdminLock.
🤖 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/handler/admin_handler_test.go`:
- Around line 111-115: The tests create HTTP requests using httptest.NewRequest
which triggers the noctx lint rule; replace those calls with
httptest.NewRequestWithContext and pass an explicit context (e.g.,
context.Background() or a test-specific ctx) so the requests satisfy the lint
checker; update the two locations that build req for the lock endpoint (the call
that currently uses httptest.NewRequest(http.MethodPost,
"/api/admin/users/"+tt.userID+"/lock", nil)) and the other similar call around
lines 171–175 to use httptest.NewRequestWithContext(ctx, http.MethodPost,
"/api/admin/users/"+tt.userID+"/lock", nil). Ensure you import context if not
already present.
In `@internal/service/auth_service_integration_test.go`:
- Around line 81-88: The promoteAdmin helper currently only asserts that the
GORM Update returned no Error, which can miss cases where 0 rows were updated;
modify promoteAdmin to also assert that the update affected exactly one row by
inspecting the result returned from db.Model(&models.User{}).Where("id = ?",
userID).Update("role", "admin") and asserting result.RowsAffected == 1 (in
addition to require.NoError) so test setup fails fast if no user was promoted.
- Around line 92-223: The tests calling SetupIntegrationTest (e.g.,
TestAuthService_LockUser_TokenRevocation, TestAuthService_LockUser_SelfLock,
TestAuthService_LockUser_AdminLock, TestAuthService_UnlockUser,
TestAuthService_UnlockUser_WhenNotLocked and the other LockUser tests) ignore
the third return value (miniredis) and never close it; update each test that
does authSvc, db, _ := testutils.SetupIntegrationTest(t) to capture the third
return (e.g., mr) and ensure it is closed via defer mr.Close() or
t.Cleanup(func(){ mr.Close() }) immediately after setup so the miniredis
instance is always cleaned up.
In `@internal/service/auth_service.go`:
- Around line 837-859: The UnlockUser method is starting a DB transaction but
still uses s.userRepo directly (bypassing tx); update UnlockUser to mirror the
LockUser fix by creating transaction-scoped repository instances from tx and
using those for all DB ops: replace calls to s.userRepo.FindByID and
s.userRepo.UnlockUser with the tx-bound repo (e.g., userRepoTx :=
s.userRepo.WithTx(tx) or the same factory used in LockUser) so both the lookup
and the unlock run inside the transaction; keep s.auditService.LogEvent as-is
(after the repo calls) unless LockUser changed audit handling as well.
- Around line 802-834: The transaction callback currently calls
s.userRepo.FindByID, s.userRepo.LockUser and s.tokenRepo.RevokeAllUserTokens
which operate on the global DB, so they bypass s.db.Transaction; update the
callback to use transaction-scoped repositories (or repository methods that
accept tx) — e.g. create txRepo := userRepo.WithTx(tx) / tokenRepo :=
tokenRepo.WithTx(tx) or pass tx into FindByID/LockUser/RevokeAllUserTokens so
those calls run within tx, keep error wrapping as-is, and move
s.auditService.LogEvent outside (or after) the transaction commit so audit
entries are only written for successful transactions.
---
Nitpick comments:
In `@internal/service/auth_service_integration_test.go`:
- Around line 179-188: TestAuthService_LockUser_AdminLock currently uses a
non-admin actor (user.ID) which makes the test exercise caller-authorization
instead of the “target is admin” rule; change the setup so both actor and target
are admins: call registerUser twice for adminActor and adminTarget, call
promoteAdmin(t, db, adminActor.ID) and promoteAdmin(t, db, adminTarget.ID), then
invoke authSvc.LockUser(adminActor.ID, adminTarget.ID, "", "") and
assert.ErrorIs(t, err, service.ErrAdminLock). Update references in the test
function TestAuthService_LockUser_AdminLock and keep use of registerUser,
promoteAdmin, authSvc.LockUser, and service.ErrAdminLock.
🪄 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: 877f8dce-df6b-49a3-882d-26448df3d4a5
📒 Files selected for processing (8)
internal/handler/admin_handler.gointernal/handler/admin_handler_test.gointernal/models/user.gointernal/repository/user_repository.gointernal/routes/routes.gointernal/service/auth_service.gointernal/service/auth_service_integration_test.gointernal/testutils/setup.go
Owner
|
handle the coderabbit suggestions |
Contributor
Author
working on it |
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)
internal/service/auth_service.go (1)
804-830:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftLocking still races with in-flight logins.
LockUserrevokes the refresh tokens that exist inside this transaction, butLoginlater in this file readsLockedUntilbefore password verification and inserts a new refresh token without rechecking the row. A login that starts just before this lock commits can therefore finish with a brand-new session after the admin lock. This needs a DB-level gate around the user row on both flows, not just a transaction around the lock path.🤖 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 804 - 830, The admin lock races with concurrent logins because LockUser only revokes tokens within the transaction but the Login path reads LockedUntil and inserts a refresh token without holding a DB row lock; fix by acquiring a DB-level row lock on the user in both flows: in the lock path (inside the Transaction where you call repository.NewUserRepository(tx) and userRepo.FindByID/LockUser) ensure you SELECT the user FOR UPDATE before checking Role/IsLocked and performing LockUser/RevokeAllUserTokens, and in the Login flow (the Login function that reads LockedUntil and later inserts a refresh token) acquire the same SELECT ... FOR UPDATE (or GORM clause.Locking{Strength:"UPDATE"}) for the user row before verifying password and issuing tokens so the two paths serialize on the same row.
🤖 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`:
- Around line 826-831: When calling userRepo.LockUser/userRepo.UnlockUser and
tokenRepo.RevokeAllUserTokens, preserve the sentinel repository.ErrUserNotFound
instead of wrapping it so handlers can return 404; after each write call (in the
blocks around userRepo.LockUser / tokenRepo.RevokeAllUserTokens and the similar
UnlockUser block) check errors.Is(err, repository.ErrUserNotFound) and return
repository.ErrUserNotFound directly, otherwise wrap/return other errors as
fmt.Errorf("...: %w", err). This ensures the NotFound sentinel propagates
unchanged from the repository to the HTTP handler.
- Around line 883-892: Currently UnlockUser (the code path that calls
s.auditService.LogEvent with "USER_UNLOCKED") returns the audit error, causing a
500 after the unlock has already committed; change this to best-effort: invoke
s.auditService.LogEvent but do not return its error from UnlockUser—handle the
error locally (e.g., swallow it and emit an internal warning/log via the service
logger) so the unlocked result is not reported as failed. Ensure you reference
and modify the call to s.auditService.LogEvent (the "USER_UNLOCKED" audit) and
adjust the surrounding return behavior so the function only returns the unlock
result, never the post-commit audit error.
- Around line 855-857: The code declares var err separately then assigns it with
err = s.db.Transaction(...), triggering staticcheck S1021; fix by merging
declaration and assignment (use short declaration :=) when calling
s.db.Transaction, or return the result of s.db.Transaction directly; update the
usage sites that reference err after the call to use the newly declared variable
from the Transaction call (reference: s.db.Transaction and the err variable in
this function).
---
Outside diff comments:
In `@internal/service/auth_service.go`:
- Around line 804-830: The admin lock races with concurrent logins because
LockUser only revokes tokens within the transaction but the Login path reads
LockedUntil and inserts a refresh token without holding a DB row lock; fix by
acquiring a DB-level row lock on the user in both flows: in the lock path
(inside the Transaction where you call repository.NewUserRepository(tx) and
userRepo.FindByID/LockUser) ensure you SELECT the user FOR UPDATE before
checking Role/IsLocked and performing LockUser/RevokeAllUserTokens, and in the
Login flow (the Login function that reads LockedUntil and later inserts a
refresh token) acquire the same SELECT ... FOR UPDATE (or GORM
clause.Locking{Strength:"UPDATE"}) for the user row before verifying password
and issuing tokens so the two paths serialize on the same row.
🪄 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: 2e9b657d-acb6-416c-ae9d-d9eabf3c101c
📒 Files selected for processing (3)
internal/handler/admin_handler_test.gointernal/service/auth_service.gointernal/service/auth_service_integration_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/service/auth_service_integration_test.go
Owner
|
@Samikshaaggarwal handle the rest one left other that it looks good to me |
|
Contributor
Author
|
recheck |
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
2 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.
Checklist
I have read the CLA and agree to its terms.on this PR.Summary by CodeRabbit
New Features
Tests