feat: enable root admin to manage all user API keys - #6786
Conversation
WalkthroughRoot users can manage tokens for other users. Backend model and controller paths support filtering, creation, retrieval, updates, deletions, batch operations, and audit events. The frontend adds administrator user selection, filtering, ownership display, and creation support. ChangesAdministrative token backend
Frontend administrator controls
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Administrator
participant ApiKeysTable
participant KeysApi
participant TokenController
participant TokenModel
Administrator->>ApiKeysTable: Select user or create a key
ApiKeysTable->>KeysApi: Send optional user_id
KeysApi->>TokenController: Request token operation
TokenController->>TokenModel: Execute root-scoped operation
TokenModel-->>TokenController: Return token data
TokenController-->>KeysApi: Return response
KeysApi-->>ApiKeysTable: Render filtered or created key
Possibly related PRs
Suggested reviewers: Poem
🚥 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 |
9373a26 to
12a6871
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/features/keys/components/api-keys-mutate-drawer.tsx (1)
227-284: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
isAdminandauthUserto the effect dependency array.The effect reads
isAdminandauthUser?.idat line 233 to resettargetUserId. Neither value appears in the dependency array at lines 266-284. If either value changes after the drawer mounts, the effect will not rerun with the new value.Add both to the dependency array.
🔧 Proposed fix
}, [ open, isUpdate, currentRow, form, defaultUseAutoGroup, statusLoading, backendHasAuto, groupsFetched, groupsFetching, autoGroupsFetched, autoGroupsFetching, apiKeyData, apiKeyFetched, apiKeyFetching, availableAutoGroupNames, maxAutoGroups, initializedTarget, + isAdmin, + authUser, ])As per coding guidelines: "完成代码改动前必须对涉及文件执行 lint，并修复所有 lint error。"
🤖 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 `@web/src/features/keys/components/api-keys-mutate-drawer.tsx` around lines 227 - 284, Add isAdmin and authUser (or the established authUser identifier) to the useEffect dependency array alongside the existing dependencies so targetUserId resets when either value changes; run lint for the affected file and fix any resulting lint errors.Source: Coding guidelines
🧹 Nitpick comments (7)
model/token.go (3)
586-617: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the two batch-delete implementations.
BatchDeleteTokensAdminrepeatsBatchDeleteTokensverbatim except for theuser_idpredicate. Extract one internal function that takes the scope, so the transaction handling and the Redis cleanup stay in one place.♻️ Suggested shape
func batchDeleteTokens(ids []int, userId *int) (int, error) { if len(ids) == 0 { return 0, errors.New("ids 不能为空！") } tx := DB.Begin() scope := tx.Where("id IN (?)", ids) if userId != nil { scope = scope.Where("user_id = ?", *userId) } // ...load, delete, commit, cache cleanup }🤖 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 `@model/token.go` around lines 586 - 617, Extract the shared transaction and Redis cleanup logic from BatchDeleteTokens and BatchDeleteTokensAdmin into an internal batchDeleteTokens(ids []int, userId *int) helper. Apply the id filter through a shared scope, add the user_id predicate only when userId is non-nil, and update both public functions to delegate to this helper while preserving their existing behavior.
17-17: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExclude
Usernamefrom migrations.
gorm:"->"still addstokens.usernameduringAutoMigrate. Add-:migrationto prevent this unused column. This change does not remove the column from existing deployments.🤖 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 `@model/token.go` at line 17, Update the Username field in the token model to include GORM’s migration-exclusion tag alongside its existing read-only configuration, preventing AutoMigrate from creating tokens.username while preserving the current field behavior and existing deployed columns.Source: Coding guidelines
257-267: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winQualify the ordering column for portability.
SearchTokensAdminjoinstokensandusers. UseOrder("tokens.id desc")instead of unqualifiedid. Do not addSession(&gorm.Session{}); GORM clones the statement forCountand later chain methods. Also,Limit(maxTokens).Count(&total)does not cap the count becauseLIMITapplies afterCOUNT(*); use a limited subquery only when a capped total is intended.🤖 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 `@model/token.go` around lines 257 - 267, Update SearchTokensAdmin’s ordering clause to use the qualified column tokens.id desc instead of unqualified id. Do not add a new GORM session; retain the existing Count and query chaining, and leave count-capping behavior unchanged unless a capped total is explicitly intended.Source: Coding guidelines
controller/token_test.go (3)
615-639: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFold
newAdminContextintonewAuthenticatedContext.
newAdminContextrepeats the body ofnewAuthenticatedContext(Lines 184-206) and addsroleandusername. Add optional role handling to the existing helper instead of maintaining two copies of the request-building logic.🤖 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 `@controller/token_test.go` around lines 615 - 639, Fold newAdminContext into newAuthenticatedContext by adding optional role and username handling to the existing helper, preserving its current request construction and authentication setup. Update callers of newAdminContext to use newAuthenticatedContext with the needed role values, then remove the duplicate helper.
924-959: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the root batch endpoints.
The suite covers list, get, create, update, and single delete, but not
DeleteTokenBatchandGetTokenKeysBatch. Those are the two handlers whose root fallback never triggers, as raised oncontroller/token.goLines 543-557 and 581-584. A table test that submits another user's token IDs as root would catch both defects.Do you want me to generate those two 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 `@controller/token_test.go` around lines 924 - 959, Add table-driven coverage alongside TestAdminDeleteTokenDeletesOtherUserToken for the root batch handlers DeleteTokenBatch and GetTokenKeysBatch, submitting token IDs owned by another user through the corresponding request payloads. Assert the root role receives the expected successful response and that both handlers operate on the supplied IDs, exposing the root fallback behavior without changing existing single-token tests.Source: Coding guidelines
585-612: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the package-level database handles after each test.
setupAdminTokenTestDBassignsmodel.DBandmodel.LOG_DB. The cleanup closes the connection but leaves both globals pointing at a closed handle. A later test in the same package that forgets to call a setup helper then runs against a closed database, and the failure mode is confusing. Save and restore the previous values.🛠️ Proposed fix
+ prevDB, prevLogDB := model.DB, model.LOG_DB model.DB = db model.LOG_DB = db if err := db.AutoMigrate(&model.Token{}, &model.User{}); err != nil { t.Fatalf("failed to migrate tables: %v", err) } t.Cleanup(func() { + model.DB, model.LOG_DB = prevDB, prevLogDB sqlDB, err := db.DB() if err == nil { _ = sqlDB.Close() } })🤖 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 `@controller/token_test.go` around lines 585 - 612, Update setupAdminTokenTestDB to save the existing model.DB and model.LOG_DB values before assigning the test database, then restore both globals in the t.Cleanup callback after closing the test connection.web/src/features/keys/components/api-keys-table.tsx (1)
199-212: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDuplicated admin-users query with no error handling in two files. Both files independently fetch
getUsers({ p: 1, page_size: 200 })for the admin user selector, using different query keys and neither surfacing a failure to the user.
web/src/features/keys/components/api-keys-table.tsx#L199-L212: extract a shared hook (for exampleuseAdminUsersList()) and add error feedback on failure, matching thekeysquery'stoast.errorpattern later in the same file.web/src/features/keys/components/api-keys-mutate-drawer.tsx#L117-L130: switch to the same shared hook and add the same error feedback.🤖 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 `@web/src/features/keys/components/api-keys-table.tsx` around lines 199 - 212, The admin user list query is duplicated and silently ignores failures. In web/src/features/keys/components/api-keys-table.tsx:199-212, extract the getUsers({ p: 1, page_size: 200 }) logic into a shared useAdminUsersList() hook, add toast.error feedback matching the existing keys query pattern, and replace the local query with the hook. In web/src/features/keys/components/api-keys-mutate-drawer.tsx:117-130, replace the duplicate query with the same shared hook and add the same error feedback.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 `@controller/audit.go`:
- Line 56: Update the token.admin_batch_delete audit template and its associated
batch-delete audit flow to avoid assuming a single target_username; use an
owner-independent message for mixed batches, or emit separate audit events per
owner. Also address the missing audit emission in the batch-delete handler near
the token deletion flow so the corrected action is actually recorded.
In `@controller/token_test.go`:
- Around line 829-853: The test name TestAdminAddTokenRejectsHigherRoleTarget
contradicts its successful same-role token-creation assertion; rename it to
describe that a root user can create a token for another root user, while
leaving the test behavior and assertions unchanged.
- Around line 661-683: Update all newly added administrator tests in the
affected range, including TestAdminGetAllTokensReturnsAllUserTokens, to use
testify assertions: replace fatal setup/error checks with require and non-fatal
value checks with assert. Add or reuse the repository’s testify imports while
preserving each test’s existing assertions and behavior.
In `@controller/token.go`:
- Around line 407-429: Move the administrative audit call in the root-user
branch of the token deletion flow to execute only after token.Delete() succeeds.
Check the returned err before invoking recordManageAuditFor, while preserving
the existing target-user condition and audit payload.
- Around line 391-397: Update the admin-create audit block in the token creation
flow to populate target_username from the user loaded earlier in the handler,
rather than cleanToken.Username. Keep the existing audit event and other fields
unchanged so token.admin_create receives the loaded user’s username.
- Around line 543-557: The root batch operations currently choose administrator
fallbacks only when scoped model calls return errors, so root actions on other
users’ tokens do nothing. In controller/token.go lines 543-557, branch on the
root role before using the scoped deletion, call model.BatchDeleteTokensAdmin,
and record a token.admin_batch_delete audit event after successful deletion; in
lines 581-584, likewise select model.GetTokenKeysByIdsAdmin by role instead of
by error.
In `@web/src/i18n/locales/en.json`:
- Line 238: Rename the translation key from “Admin filter” to “Admin search” in
web/src/i18n/locales/en.json:238-238 and web/src/i18n/locales/zh.json:238-238,
preserving the Chinese value; update the corresponding t('Admin filter') call in
web/src/features/keys/components/api-keys-table.tsx:345-373 to use “Admin
search”.
---
Outside diff comments:
In `@web/src/features/keys/components/api-keys-mutate-drawer.tsx`:
- Around line 227-284: Add isAdmin and authUser (or the established authUser
identifier) to the useEffect dependency array alongside the existing
dependencies so targetUserId resets when either value changes; run lint for the
affected file and fix any resulting lint errors.
---
Nitpick comments:
In `@controller/token_test.go`:
- Around line 615-639: Fold newAdminContext into newAuthenticatedContext by
adding optional role and username handling to the existing helper, preserving
its current request construction and authentication setup. Update callers of
newAdminContext to use newAuthenticatedContext with the needed role values, then
remove the duplicate helper.
- Around line 924-959: Add table-driven coverage alongside
TestAdminDeleteTokenDeletesOtherUserToken for the root batch handlers
DeleteTokenBatch and GetTokenKeysBatch, submitting token IDs owned by another
user through the corresponding request payloads. Assert the root role receives
the expected successful response and that both handlers operate on the supplied
IDs, exposing the root fallback behavior without changing existing single-token
tests.
- Around line 585-612: Update setupAdminTokenTestDB to save the existing
model.DB and model.LOG_DB values before assigning the test database, then
restore both globals in the t.Cleanup callback after closing the test
connection.
In `@model/token.go`:
- Around line 586-617: Extract the shared transaction and Redis cleanup logic
from BatchDeleteTokens and BatchDeleteTokensAdmin into an internal
batchDeleteTokens(ids []int, userId *int) helper. Apply the id filter through a
shared scope, add the user_id predicate only when userId is non-nil, and update
both public functions to delegate to this helper while preserving their existing
behavior.
- Line 17: Update the Username field in the token model to include GORM’s
migration-exclusion tag alongside its existing read-only configuration,
preventing AutoMigrate from creating tokens.username while preserving the
current field behavior and existing deployed columns.
- Around line 257-267: Update SearchTokensAdmin’s ordering clause to use the
qualified column tokens.id desc instead of unqualified id. Do not add a new GORM
session; retain the existing Count and query chaining, and leave count-capping
behavior unchanged unless a capped total is explicitly intended.
In `@web/src/features/keys/components/api-keys-table.tsx`:
- Around line 199-212: The admin user list query is duplicated and silently
ignores failures. In
web/src/features/keys/components/api-keys-table.tsx:199-212, extract the
getUsers({ p: 1, page_size: 200 }) logic into a shared useAdminUsersList() hook,
add toast.error feedback matching the existing keys query pattern, and replace
the local query with the hook. In
web/src/features/keys/components/api-keys-mutate-drawer.tsx:117-130, replace the
duplicate query with the same shared hook and add the same error feedback.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: d16c9b84-78cf-4627-aaf4-940ce6b0f1fe
📒 Files selected for processing (12)
controller/audit.gocontroller/token.gocontroller/token_test.gomodel/token.goweb/src/features/keys/api.tsweb/src/features/keys/components/api-keys-columns.tsxweb/src/features/keys/components/api-keys-mutate-drawer.tsxweb/src/features/keys/components/api-keys-provider.tsxweb/src/features/keys/components/api-keys-table.tsxweb/src/features/keys/types.tsweb/src/i18n/locales/en.jsonweb/src/i18n/locales/zh.json
| "token.admin_create": "Created token for user ${target_username} (ID: ${target_user_id})", | ||
| "token.admin_update": "Updated token ${token_name} (ID: ${token_id}) of user ${target_username}", | ||
| "token.admin_delete": "Deleted token ${token_name} (ID: ${token_id}) of user ${target_username}", | ||
| "token.admin_batch_delete": "Batch deleted ${count} tokens of user ${target_username}", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
The batch template assumes one owner.
token.admin_batch_delete renders ${target_username}, but a batch can contain tokens from several users. For a mixed batch the placeholder expands to an empty string, because auditContentEN returns "" for missing keys. Drop the owner from the template, or record one audit event per owner.
- "token.admin_batch_delete": "Batch deleted ${count} tokens of user ${target_username}",
+ "token.admin_batch_delete": "Batch deleted ${count} tokens (IDs: ${token_ids})",No handler currently emits this action; that gap is raised on controller/token.go Lines 543-557.
📝 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.
| "token.admin_batch_delete": "Batch deleted ${count} tokens of user ${target_username}", | |
| "token.admin_batch_delete": "Batch deleted ${count} tokens (IDs: ${token_ids})", |
🤖 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 `@controller/audit.go` at line 56, Update the token.admin_batch_delete audit
template and its associated batch-delete audit flow to avoid assuming a single
target_username; use an owner-independent message for mixed batches, or emit
separate audit events per owner. Also address the missing audit emission in the
batch-delete handler near the token deletion flow so the corrected action is
actually recorded.
| func TestAdminGetAllTokensReturnsAllUserTokens(t *testing.T) { | ||
| db := setupAdminTokenTestDB(t) | ||
| seedTestUser(t, db, 1, common.RoleRootUser, "root") | ||
| seedTestUser(t, db, 2, common.RoleCommonUser, "common-user") | ||
| seedToken(t, db, 1, "root-token", "r001xxxxxxxxxxxx") | ||
| seedToken(t, db, 2, "user-token", "u001xxxxxxxxxxxx") | ||
|
|
||
| ctx, recorder := newAdminContext(t, http.MethodGet, "/api/token/?p=1&size=10", nil, 1, common.RoleRootUser) | ||
| GetAllTokens(ctx) | ||
|
|
||
| response := decodeAPIResponse(t, recorder) | ||
| if !response.Success { | ||
| t.Fatalf("expected success response, got message: %s", response.Message) | ||
| } | ||
|
|
||
| var page tokenPageResponse | ||
| if err := common.Unmarshal(response.Data, &page); err != nil { | ||
| t.Fatalf("failed to decode page response: %v", err) | ||
| } | ||
| if len(page.Items) != 2 { | ||
| t.Fatalf("expected 2 tokens, got %d", len(page.Items)) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use testify in the new tests.
The new administrator tests assert only with t.Fatalf. The repository guideline requires testify: "New or substantially rewritten tests must use testify/require for setup and fatal assertions and testify/assert for non-fatal checks." Apply this to every test added in Lines 661-959.
🛠️ Example for this test
response := decodeAPIResponse(t, recorder)
- if !response.Success {
- t.Fatalf("expected success response, got message: %s", response.Message)
- }
-
var page tokenPageResponse
- if err := common.Unmarshal(response.Data, &page); err != nil {
- t.Fatalf("failed to decode page response: %v", err)
- }
- if len(page.Items) != 2 {
- t.Fatalf("expected 2 tokens, got %d", len(page.Items))
- }
+ require.True(t, response.Success, response.Message)
+ require.NoError(t, common.Unmarshal(response.Data, &page))
+ assert.Len(t, page.Items, 2)As per coding guidelines: "New or substantially rewritten tests must use testify/require for setup and fatal assertions and testify/assert for non-fatal checks."
🤖 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 `@controller/token_test.go` around lines 661 - 683, Update all newly added
administrator tests in the affected range, including
TestAdminGetAllTokensReturnsAllUserTokens, to use testify assertions: replace
fatal setup/error checks with require and non-fatal value checks with assert.
Add or reuse the repository’s testify imports while preserving each test’s
existing assertions and behavior.
Source: Coding guidelines
| func TestAdminAddTokenRejectsHigherRoleTarget(t *testing.T) { | ||
| db := setupAdminTokenTestDB(t) | ||
|
|
||
| seedTestUser(t, db, 1, common.RoleRootUser, "root") | ||
| seedTestUser(t, db, 2, common.RoleRootUser, "root2") | ||
|
|
||
| // Root 不能为另一个 Root 创建令牌 | ||
| body := map[string]any{ | ||
| "user_id": 2, | ||
| "name": "bad-token", | ||
| "expired_time": -1, | ||
| "remain_quota": 100, | ||
| "unlimited_quota": false, | ||
| "group": "default", | ||
| } | ||
|
|
||
| ctx, recorder := newAdminContext(t, http.MethodPost, "/api/token/", body, 1, common.RoleRootUser) | ||
| AddToken(ctx) | ||
|
|
||
| // Root 现在可以给任何用户创建令牌，包括同级 Root | ||
| response := decodeAPIResponse(t, recorder) | ||
| if !response.Success { | ||
| t.Fatalf("expected success when creating token for same-role user, got message: %s", response.Message) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The test name contradicts the assertion.
TestAdminAddTokenRejectsHigherRoleTarget asserts success, and the comment at Line 848 states that root may create tokens for a same-role user. The name describes a rejection that the code does not implement. Rename the test to match the verified behavior.
-func TestAdminAddTokenRejectsHigherRoleTarget(t *testing.T) {
+func TestAdminAddTokenAllowsSameRoleTarget(t *testing.T) {📝 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.
| func TestAdminAddTokenRejectsHigherRoleTarget(t *testing.T) { | |
| db := setupAdminTokenTestDB(t) | |
| seedTestUser(t, db, 1, common.RoleRootUser, "root") | |
| seedTestUser(t, db, 2, common.RoleRootUser, "root2") | |
| // Root 不能为另一个 Root 创建令牌 | |
| body := map[string]any{ | |
| "user_id": 2, | |
| "name": "bad-token", | |
| "expired_time": -1, | |
| "remain_quota": 100, | |
| "unlimited_quota": false, | |
| "group": "default", | |
| } | |
| ctx, recorder := newAdminContext(t, http.MethodPost, "/api/token/", body, 1, common.RoleRootUser) | |
| AddToken(ctx) | |
| // Root 现在可以给任何用户创建令牌，包括同级 Root | |
| response := decodeAPIResponse(t, recorder) | |
| if !response.Success { | |
| t.Fatalf("expected success when creating token for same-role user, got message: %s", response.Message) | |
| } | |
| } | |
| func TestAdminAddTokenAllowsSameRoleTarget(t *testing.T) { | |
| db := setupAdminTokenTestDB(t) | |
| seedTestUser(t, db, 1, common.RoleRootUser, "root") | |
| seedTestUser(t, db, 2, common.RoleRootUser, "root2") | |
| // Root 不能为另一个 Root 创建令牌 | |
| body := map[string]any{ | |
| "user_id": 2, | |
| "name": "bad-token", | |
| "expired_time": -1, | |
| "remain_quota": 100, | |
| "unlimited_quota": false, | |
| "group": "default", | |
| } | |
| ctx, recorder := newAdminContext(t, http.MethodPost, "/api/token/", body, 1, common.RoleRootUser) | |
| AddToken(ctx) | |
| // Root 现在可以给任何用户创建令牌，包括同级 Root | |
| response := decodeAPIResponse(t, recorder) | |
| if !response.Success { | |
| t.Fatalf("expected success when creating token for same-role user, got message: %s", response.Message) | |
| } | |
| } |
🤖 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 `@controller/token_test.go` around lines 829 - 853, The test name
TestAdminAddTokenRejectsHigherRoleTarget contradicts its successful same-role
token-creation assertion; rename it to describe that a root user can create a
token for another root user, while leaving the test behavior and assertions
unchanged.
| if isAdminCreate { | ||
| recordManageAuditFor(c, token.UserId, "token.admin_create", map[string]interface{}{ | ||
| "target_user_id": token.UserId, | ||
| "target_username": cleanToken.Username, | ||
| "token_name": cleanToken.Name, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
target_username is always empty in the create audit.
cleanToken is built at Line 370 without a Username value, and Insert() does not populate it. The audit template token.admin_create in controller/audit.go expands ${target_username} to an empty string. Reuse the user already loaded at Line 318.
🛠️ Proposed fix
isAdminCreate := c.GetInt("role") == common.RoleRootUser && token.UserId > 0 && token.UserId != c.GetInt("id")
+ var targetUser *model.User
if isAdminCreate {
- if _, err := model.GetUserCache(token.UserId); err != nil {
+ user, err := model.GetUserCache(token.UserId)
+ if err != nil {
common.ApiErrorI18n(c, i18n.MsgInvalidParams)
return
}
+ targetUser = user
} else { if isAdminCreate {
recordManageAuditFor(c, token.UserId, "token.admin_create", map[string]interface{}{
"target_user_id": token.UserId,
- "target_username": cleanToken.Username,
+ "target_username": targetUser.Username,
"token_name": cleanToken.Name,
})
}📝 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.
| if isAdminCreate { | |
| recordManageAuditFor(c, token.UserId, "token.admin_create", map[string]interface{}{ | |
| "target_user_id": token.UserId, | |
| "target_username": cleanToken.Username, | |
| "token_name": cleanToken.Name, | |
| }) | |
| } | |
| if isAdminCreate { | |
| recordManageAuditFor(c, token.UserId, "token.admin_create", map[string]interface{}{ | |
| "target_user_id": token.UserId, | |
| "target_username": targetUser.Username, | |
| "token_name": cleanToken.Name, | |
| }) | |
| } |
🤖 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 `@controller/token.go` around lines 391 - 397, Update the admin-create audit
block in the token creation flow to populate target_username from the user
loaded earlier in the handler, rather than cleanToken.Username. Keep the
existing audit event and other fields unchanged so token.admin_create receives
the loaded user’s username.
| // Root 可删除任意用户的令牌，非 Root 仅限自己的 | ||
| var err error | ||
| if c.GetInt("role") == common.RoleRootUser { | ||
| token, tErr := model.GetTokenById(id) | ||
| if tErr != nil { | ||
| common.ApiError(c, tErr) | ||
| return | ||
| } | ||
| if token.UserId != userId { | ||
| targetUser, _ := model.GetUserCache(token.UserId) | ||
| if targetUser != nil { | ||
| recordManageAuditFor(c, token.UserId, "token.admin_delete", map[string]interface{}{ | ||
| "target_user_id": token.UserId, | ||
| "target_username": targetUser.Username, | ||
| "token_id": token.Id, | ||
| "token_name": token.Name, | ||
| }) | ||
| } | ||
| } | ||
| err = token.Delete() | ||
| } else { | ||
| err = model.DeleteTokenById(id, userId) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Record the delete audit after the delete succeeds.
recordManageAuditFor runs before token.Delete(). If the delete fails, the audit trail still reports a completed administrative deletion. Move the audit call after the error check.
🛠️ Proposed fix
- if token.UserId != userId {
- targetUser, _ := model.GetUserCache(token.UserId)
- if targetUser != nil {
- recordManageAuditFor(c, token.UserId, "token.admin_delete", map[string]interface{}{
- "target_user_id": token.UserId,
- "target_username": targetUser.Username,
- "token_id": token.Id,
- "token_name": token.Name,
- })
- }
- }
err = token.Delete()
+ if err == nil && token.UserId != userId {
+ targetUser, _ := model.GetUserCache(token.UserId)
+ if targetUser != nil {
+ recordManageAuditFor(c, token.UserId, "token.admin_delete", map[string]interface{}{
+ "target_user_id": token.UserId,
+ "target_username": targetUser.Username,
+ "token_id": token.Id,
+ "token_name": token.Name,
+ })
+ }
+ }📝 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.
| // Root 可删除任意用户的令牌，非 Root 仅限自己的 | |
| var err error | |
| if c.GetInt("role") == common.RoleRootUser { | |
| token, tErr := model.GetTokenById(id) | |
| if tErr != nil { | |
| common.ApiError(c, tErr) | |
| return | |
| } | |
| if token.UserId != userId { | |
| targetUser, _ := model.GetUserCache(token.UserId) | |
| if targetUser != nil { | |
| recordManageAuditFor(c, token.UserId, "token.admin_delete", map[string]interface{}{ | |
| "target_user_id": token.UserId, | |
| "target_username": targetUser.Username, | |
| "token_id": token.Id, | |
| "token_name": token.Name, | |
| }) | |
| } | |
| } | |
| err = token.Delete() | |
| } else { | |
| err = model.DeleteTokenById(id, userId) | |
| } | |
| // Root 可删除任意用户的令牌，非 Root 仅限自己的 | |
| var err error | |
| if c.GetInt("role") == common.RoleRootUser { | |
| token, tErr := model.GetTokenById(id) | |
| if tErr != nil { | |
| common.ApiError(c, tErr) | |
| return | |
| } | |
| err = token.Delete() | |
| if err == nil && token.UserId != userId { | |
| targetUser, _ := model.GetUserCache(token.UserId) | |
| if targetUser != nil { | |
| recordManageAuditFor(c, token.UserId, "token.admin_delete", map[string]interface{}{ | |
| "target_user_id": token.UserId, | |
| "target_username": targetUser.Username, | |
| "token_id": token.Id, | |
| "token_name": token.Name, | |
| }) | |
| } | |
| } | |
| } else { | |
| err = model.DeleteTokenById(id, userId) | |
| } |
🤖 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 `@controller/token.go` around lines 407 - 429, Move the administrative audit
call in the root-user branch of the token deletion flow to execute only after
token.Delete() succeeds. Check the returned err before invoking
recordManageAuditFor, while preserving the existing target-user condition and
audit payload.
| // Root 可批量删除任意用户的令牌 | ||
| if err != nil && c.GetInt("role") == common.RoleRootUser { | ||
| for _, id := range tokenBatch.Ids { | ||
| _, tErr := model.GetTokenById(id) | ||
| if tErr != nil { | ||
| common.ApiError(c, tErr) | ||
| return | ||
| } | ||
| } | ||
| count, err = model.BatchDeleteTokensAdmin(tokenBatch.Ids) | ||
| if err != nil { | ||
| common.ApiError(c, err) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Root batch fallbacks are unreachable because the scoped model functions do not return an error. model.BatchDeleteTokens and model.GetTokenKeysByIds return an empty result with a nil error when no row matches the user_id scope. Both handlers trigger the administrator path only on err != nil, so a root user operating on another user's tokens receives a success response with no effect.
controller/token.go#L543-L557: selectmodel.BatchDeleteTokensAdminby role instead of by error, and record atoken.admin_batch_deleteaudit event after a successful root deletion.controller/token.go#L581-L584: selectmodel.GetTokenKeysByIdsAdminby role instead of by error.
📍 Affects 1 file
controller/token.go#L543-L557(this comment)controller/token.go#L581-L584
🤖 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 `@controller/token.go` around lines 543 - 557, The root batch operations
currently choose administrator fallbacks only when scoped model calls return
errors, so root actions on other users’ tokens do nothing. In
controller/token.go lines 543-557, branch on the root role before using the
scoped deletion, call model.BatchDeleteTokensAdmin, and record a
token.admin_batch_delete audit event after successful deletion; in lines
581-584, likewise select model.GetTokenKeysByIdsAdmin by role instead of by
error.
| "Admin access required": "Admin access required", | ||
| "Admin area": "Admin area", | ||
| "Admin Channel Permissions": "Admin Channel Permissions", | ||
| "Admin filter": "Admin search", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Translation key no longer matches its displayed English text. The English value for key Admin filter was changed to Admin search, but the key itself and its call site were not renamed, breaking the file convention that the key equals the English source string.
web/src/i18n/locales/en.json#L238-L238: rename the key fromAdmin filtertoAdmin search.web/src/i18n/locales/zh.json#L238-L238: rename the same key toAdmin search; the Chinese value管理员搜索stays unchanged.web/src/features/keys/components/api-keys-table.tsx#L345-L373: updatet('Admin filter')tot('Admin search').
📍 Affects 3 files
web/src/i18n/locales/en.json#L238-L238(this comment)web/src/i18n/locales/zh.json#L238-L238web/src/features/keys/components/api-keys-table.tsx#L345-L373
🤖 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 `@web/src/i18n/locales/en.json` at line 238, Rename the translation key from
“Admin filter” to “Admin search” in web/src/i18n/locales/en.json:238-238 and
web/src/i18n/locales/zh.json:238-238, preserving the Chinese value; update the
corresponding t('Admin filter') call in
web/src/features/keys/components/api-keys-table.tsx:345-373 to use “Admin
search”.
Source: Coding guidelines
1 participant
📝 变更描述 / Description
后端：
原有 /api/token/* 接口内部按角色分支：Root 走跨用户查询，支持 ?user_id=X 筛选目标用户；非 Root 逻辑不变
Token 结构新增 username 字段，Admin 查询时 LEFT JOIN users 表返回用户名
Admin 操作（创建/编辑/删除/批量删除他人令牌）通过 recordManageAuditFor 记录审计日志
前端：
Root 用户令牌页顶部新增「管理员搜索」用户筛选框，可按用户过滤；表格新增 Owner 列显示用户名
创建抽屉新增目标用户选择器，默认为自己，可搜索切换
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
超管角色 令牌管理页：
超管角色 创建令牌页：
普通角色 令牌管理页：
普通角色创建令牌页：
Summary by CodeRabbit
New Features
Bug Fixes