admin auth follow-ups: account-keyed audit/sessions, atomic revocation, drop dev-login change-password#495
Conversation
The botplatform /api/v1/password/change endpoint was removed on the backend; password rotation now happens in the bot registration web. Drop the forced change-password step from the /dev-login flow: BotLoginPage connects the session directly on a successful login regardless of requirePasswordChange, the changePassword API wrapper is removed, and the ChangePasswordPage is deleted.
…create audit Follow-up consistency after moving the admin user/session endpoints to account: - Audit list filter is now `?targetAccount=` (AuditFilter.TargetAccount), fixing a gap where the old `?targetUserId=` filter could not match the account-only audit entries written by user.update / user.password.set / session.revoke*. - sessionView now includes `account` (only roles stay out of the projection). - createUser audit records the target by account only, matching the other mutating handlers. - admin-frontend: audit filter input + API client (AuditFilter.targetAccount, AdminSession.account) and docs (client-api.md §9.6/§9.9, SessionView) updated to match.
…a Mongo transaction Deactivating a user or resetting their password now revokes the account's sessions in the same multi-document transaction as the state change, so a failed revoke can no longer leave live tokens for a disabled user or an old credential — and the API never reports success unless both succeeded. - store_mongo: UpdateUser (deactivate path) and UpdateUserPassword run the $set + session DeleteMany inside a transaction (new withTransaction helper); non-deactivate updates keep the plain single-doc path. - handler: drop the separate best-effort DeleteSessionsByAccount calls. - testutil: add a single-node replica-set Mongo helper (MongoDBReplicaSet / EnsureMongoReplicaSet / TerminateMongoReplicaSet), connected with directConnection=true so transactions work from the host. admin-service integration tests run on it; the rest of the suite keeps the standalone shared Mongo. - integration tests assert sessions are revoked atomically on both paths.
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdmin audit filters now use account names, session responses expose accounts, and user deactivation/password updates revoke sessions atomically through MongoDB transactions. Bot login connects immediately without a password-change step. Integration tests use a MongoDB replica set. ChangesAdmin services and console
Chat bot authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AdminHandler
participant storeMongo
participant MongoDB
AdminHandler->>storeMongo: Update user or password
storeMongo->>MongoDB: Update account in transaction
storeMongo->>MongoDB: Revoke account sessions
MongoDB-->>storeMongo: Return transaction result
storeMongo-->>AdminHandler: Return operation result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin-service/store_mongo.go (1)
330-340: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAdd an index for audit
targetAccountlookups.
ListAuditnow filters ontargetAccount, but the only audit-specific index is still{targetUserId, timestamp}. The current audit write paths leavetargetUserIDempty, so that index no longer helps these queries; add a{siteId, targetAccount, timestamp}index here.🤖 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 `@admin-service/store_mongo.go` around lines 330 - 340, Add a MongoDB audit index covering siteId, targetAccount, and timestamp alongside the existing audit indexes, so ListAudit targetAccount queries use the appropriate compound index. Update the audit index initialization or migration logic rather than changing ListAudit’s filter behavior.
🧹 Nitpick comments (2)
pkg/testutil/mongo_replicaset.go (2)
91-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSwitch the pkg/testutil cleanup logs to
slog. The samefmt.Fprintf(os.Stderr, ...)pattern appears in the other terminate helpers too, so this is better handled as a small package-wide cleanup than a one-off change.🤖 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 `@pkg/testutil/mongo_replicaset.go` around lines 91 - 92, Replace the fmt.Fprintf(os.Stderr) cleanup logging in the mongo replica-set test utilities, including the Disconnect path and other terminate helpers, with the package’s slog-based logging. Preserve each existing error message and cleanup behavior while applying the change consistently across pkg/testutil.Source: Coding guidelines
59-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
mongoutil.Connecthere instead ofmongo.Connect— it already accepts the rewritten URI and keeps the ping/logging behavior consistent with the rest of the Mongo helpers.🤖 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 `@pkg/testutil/mongo_replicaset.go` at line 59, In the connection setup around the mongo client variable, replace mongo.Connect with mongoutil.Connect while preserving the existing rewritten directURI and error handling. Use the helper’s established ping and logging behavior without changing surrounding replica-set setup.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 `@pkg/testutil/mongo_replicaset.go`:
- Line 44: Document each intentionally discarded error at the `_ =
container.Terminate(ctx)` calls in the Mongo replica-set test utility, including
the occurrences near lines 44, 55, 61, and 123. Add concise comments explaining
why termination errors are intentionally ignored, without changing the cleanup
behavior.
---
Outside diff comments:
In `@admin-service/store_mongo.go`:
- Around line 330-340: Add a MongoDB audit index covering siteId, targetAccount,
and timestamp alongside the existing audit indexes, so ListAudit targetAccount
queries use the appropriate compound index. Update the audit index
initialization or migration logic rather than changing ListAudit’s filter
behavior.
---
Nitpick comments:
In `@pkg/testutil/mongo_replicaset.go`:
- Around line 91-92: Replace the fmt.Fprintf(os.Stderr) cleanup logging in the
mongo replica-set test utilities, including the Disconnect path and other
terminate helpers, with the package’s slog-based logging. Preserve each existing
error message and cleanup behavior while applying the change consistently across
pkg/testutil.
- Line 59: In the connection setup around the mongo client variable, replace
mongo.Connect with mongoutil.Connect while preserving the existing rewritten
directURI and error handling. Use the helper’s established ping and logging
behavior without changing surrounding replica-set setup.
🪄 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
Run ID: 42cb1885-3429-41cf-8bd7-d7900cf637bb
📒 Files selected for processing (20)
admin-frontend/src/api/admin/admin.test.tsadmin-frontend/src/api/admin/index.tsadmin-frontend/src/components/AuditView/AuditView.jsxadmin-frontend/src/components/AuditView/AuditView.test.jsxadmin-service/handler.goadmin-service/handler_test.goadmin-service/integration_test.goadmin-service/store.goadmin-service/store_mongo.gochat-frontend/src/api/auth/botAuth.jschat-frontend/src/api/auth/botAuth.test.jschat-frontend/src/pages/BotLoginPage/BotLoginPage.jsxchat-frontend/src/pages/BotLoginPage/BotLoginPage.test.jsxchat-frontend/src/pages/ChangePasswordPage/ChangePasswordForm.jsxchat-frontend/src/pages/ChangePasswordPage/ChangePasswordForm.test.jsxchat-frontend/src/pages/ChangePasswordPage/index.jsxchat-frontend/src/pages/ChangePasswordPage/style.cssdocs/client-api.mdpkg/testutil/mongo_replicaset.gopkg/testutil/terminate.go
💤 Files with no reviewable changes (5)
- chat-frontend/src/pages/ChangePasswordPage/style.css
- chat-frontend/src/pages/ChangePasswordPage/index.jsx
- chat-frontend/src/pages/ChangePasswordPage/ChangePasswordForm.jsx
- chat-frontend/src/api/auth/botAuth.js
- chat-frontend/src/pages/ChangePasswordPage/ChangePasswordForm.test.jsx
Code Review
|
… test helper Addresses CodeRabbit review on #495: - Replace the now-dead {targetUserId, timestamp} admin_audit index with {siteId, targetAccount, timestamp}, backing the ListAudit targetAccount filter (audit entries are keyed by account, not internal user ID). - Document the intentionally-discarded errors on the best-effort container.Terminate / db.Drop calls in the replica-set test helper. Skipped (mirror the existing pkg/testutil/mongo.go on purpose): the slog and mongoutil.Connect nitpicks — matching the sibling standalone helper; a package-wide change is out of scope here.
4 participants
Follow-up work after #472 (admin/botplatform backend) and #473 (admin-frontend + chat-frontend) merged.
chat-frontend — remove
/dev-loginchange-password step (e8fec18)The botplatform
POST /api/v1/password/changeendpoint was removed in #472 (rotation moves to the bot registration web), so the forced change-password step in the dev-login flow called a dead endpoint.BotLoginPageconnects the session directly on a successful login, regardless ofrequirePasswordChange.changePasswordAPI wrapper and deletedpages/ChangePasswordPage/.admin-service — finish account-keying (
232b31d)The user/session endpoints moved to
:accountin #472; this closes the remaininguserId-shaped spots:?targetAccount=(was?targetUserId=, which could not match the account-only audit entries written byuser.update/user.password.set/session.revoke*).sessionViewresponse now includesaccount(onlyrolesstays out of the projection).createUseraudit records the target by account only, matching the other mutating handlers.docs/client-api.md(§9.6/§9.9,SessionView) updated to match.admin-service — atomic session revocation (
1fe2eb9)Deactivating a user or resetting a password now revokes the account's sessions in the same Mongo transaction as the state change, so a failed revoke can't leave live tokens for a disabled user (or an old credential), and the API never reports success unless both succeeded.
UpdateUser(deactivate path) andUpdateUserPasswordrun the$set+ sessionDeleteManyinside a transaction; non-deactivate updates keep the plain single-doc path.pkg/testutilsingle-node replica-set Mongo helper (MongoDBReplicaSet/EnsureMongoReplicaSet/TerminateMongoReplicaSet), connected withdirectConnection=true. Only admin-service integration tests use it; the rest of the suite keeps the standalone shared Mongo.Verification
make lint→ 0 issues ·make test SERVICE=admin-service·make sast-gosecclean.update_deactivated_revokes_sessions_atomically,revokes_the_account's_sessions_atomically).vitest119/119 +tsc; chat-frontendvitest681/681 +tsc+vite build.Not included (separate efforts)
Cross-site fan-out on user create/update, and the bot registration web for password rotation.
🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
Changes
Documentation