[codex] add read-only admin role - #5442
Draft
cyf1124906008-ai wants to merge 1 commit into
Draft
Conversation
Contributor
WalkthroughThis PR introduces a new read-only administrator role (value 9) across the entire system. The backend validates the role and enforces mutation restrictions via middleware. The frontend updates role helpers, authorization checks, and UI components to recognize read-only admins as capable of viewing admin areas but unable to perform mutations. User management UIs add role selection fields, and route guards are refactored to use consistent permission helpers. ChangesRead-only Admin Role Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (5)
model/user.go (2)
521-533:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist
roleinEdit()before the new role-edit flows ship.
controller.UpdateUsernow accepts role changes, butEdit()never writesnewUser.Roleto the database. The request will return success, invalidate caches, and still leave the stored role unchanged.Suggested change
updates := map[string]interface{}{ "username": newUser.Username, "display_name": newUser.DisplayName, "group": newUser.Group, + "role": newUser.Role, "remark": newUser.Remark, }Based on PR objectives, the new user-management flows are expected to edit roles for the read-only-admin feature, so dropping
rolehere breaks that contract.🤖 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/user.go` around lines 521 - 533, The Edit() path fails to persist role changes because the updates map doesn't include newUser.Role; update the map used by DB.Model(user).Updates(updates) (in the Edit() method where newUser := *user and updates := map[string]interface{}{...}) to include "role": newUser.Role when appropriate (same conditional logic as updatePassword if needed) so controller.UpdateUser's role changes are written to the DB and caches/invalidation remain correct.
154-161: 🛠️ Refactor suggestion | 🟠 MajorReplace
json.Marshalwithcommon.Marshalin default-sidebar config serialization (model/user.golines 154-161).This business code must use the JSON wrappers from
common/json.goinstead of callingencoding/jsondirectly.Suggested change
- configBytes, err := json.Marshal(defaultConfig) + configBytes, err := common.Marshal(defaultConfig) if err != nil { common.SysLog("生成默认边栏配置失败: " + err.Error()) return "" }🤖 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/user.go` around lines 154 - 161, Replace the direct call to encoding/json's json.Marshal when serializing defaultConfig with the project's wrapper common.Marshal: call common.Marshal(defaultConfig) to produce configBytes and handle the returned (bytes, err) the same way; update the import list to remove "encoding/json" if no longer used and ensure the common package is imported so the code compiles (references: defaultConfig, configBytes, err).Sources: Coding guidelines, Learnings
middleware/auth.go (1)
70-75:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReload current
role/statusfor session-authenticated requests.
authHelperstill authorizes from the values stored in the session at login time. Invalidating the user/token caches on role edits does not touch those session fields, so a demoted or disabled account can keep its old privileges until the session expires.Suggested direction
status := session.Get("status") useAccessToken := false if username == nil { // access-token path ... } + + if !useAccessToken { + currentUser, err := model.GetUserCache(id.(int)) + if err != nil { + c.JSON(http.StatusInternalServerError, gin.H{ + "success": false, + "message": common.TranslateMessage(c, i18n.MsgDatabaseError), + }) + c.Abort() + return + } + role = currentUser.Role + status = currentUser.Status + }Based on PR objectives, role changes are supposed to take effect promptly; the current session-based authorization path prevents that and leaves a privilege-revocation gap.
Also applies to: 156-165
🤖 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 `@middleware/auth.go` around lines 70 - 75, Session-based authorization is using stale session fields (session.Get("role")/session.Get("status")) so role/status changes don't take effect; modify the authentication flow in middleware/auth.go (the block where sessions.Default(c) is read and authHelper is later invoked) to, when a session exists, fetch the current user record (or call the same user cache lookup used for token-authenticated requests) and overwrite the role and status values from that latest source before calling authHelper; ensure you still fall back to using session id/username but replace session.Get("role") and session.Get("status") with the freshly reloaded values so demotions/disablements take effect immediately.controller/user.go (1)
529-536:⚠️ Potential issue | 🟡 MinorUse
common.Marshal()for the default sidebar JSON serialization ingenerateDefaultSidebarConfig.
controller/user.gostill usesjson.Marshal(defaultConfig)for this config string; switch it tocommon.Marshal(defaultConfig)to follow the repo’scommon/json.goJSON wrapper rule.Suggested change
- configBytes, err := json.Marshal(defaultConfig) + configBytes, err := common.Marshal(defaultConfig) if err != nil { common.SysLog("生成默认边栏配置失败: " + err.Error()) return "" }🤖 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/user.go` around lines 529 - 536, The generateDefaultSidebarConfig function currently uses json.Marshal(defaultConfig); replace that call with common.Marshal(defaultConfig) so the repo's json wrapper is used consistently (keep the same error handling: check the returned error, call common.SysLog on failure and return an empty string, then return the serialized string on success). Locate the json.Marshal usage in generateDefaultSidebarConfig and swap it to common.Marshal while preserving the existing variable names (configBytes, err) and return behavior.Sources: Coding guidelines, Learnings
web/classic/src/components/topup/modals/TopupHistoryModal.jsx (1)
71-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't freeze role checks for the lifetime of the modal.
loadTopups()re-evaluatescanViewAdmin()on each fetch, butuserCanViewAdminanduserIsAdminare memoized with[]. After a role change, this component can fetch one permission level and render the other — for example, keep the补单action visible after demotion or keep admin columns hidden after promotion.Suggested fix
- const userCanViewAdmin = useMemo(() => canViewAdmin(), []); - const userIsAdmin = useMemo(() => isAdmin(), []); + const userCanViewAdmin = canViewAdmin(); + const userIsAdmin = isAdmin();Also applies to: 160-161, 163-254
🤖 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/classic/src/components/topup/modals/TopupHistoryModal.jsx` around lines 71 - 75, The component memoizes userCanViewAdmin and userIsAdmin with empty deps, freezing role checks for the modal's lifetime; update these to reflect role changes by either removing the useMemo and calling canViewAdmin() / isAdmin() directly where needed (e.g., in loadTopups(), endpoint construction, and render logic) or include the proper dependencies in the memo (the user/roles or whatever state/fromContext canViewAdmin depends on). Locate references to userCanViewAdmin, userIsAdmin, loadTopups, and any endpoint construction using canViewAdmin() and change them to re-evaluate permissions on each render/fetch so UI and fetch endpoints stay in sync with current roles.
🧹 Nitpick comments (2)
web/classic/src/components/table/users/UsersColumnDefs.jsx (1)
52-57: ⚡ Quick winHardcoded role values span three user management files.
UsersColumnDefs.jsx(role display),AddUserModal.jsx(role selection for new users), andEditUserModal.jsx(role selection for existing users) all use hardcoded role values (1,9,10, and100in the first file) instead of importing theROLE_COMMON,ROLE_READ_ONLY_ADMIN,ROLE_ADMIN, andROLE_ROOTconstants fromutils.jsx. The shared root cause is bypassing the centralized role definitions; importing these constants across all three files would improve maintainability and prevent inconsistencies if role values 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 `@web/classic/src/components/table/users/UsersColumnDefs.jsx` around lines 52 - 57, Replace the hardcoded numeric role literals with the centralized role constants from utils.jsx: import ROLE_COMMON, ROLE_READ_ONLY_ADMIN, ROLE_ADMIN, ROLE_ROOT into UsersColumnDefs.jsx, AddUserModal.jsx, and EditUserModal.jsx and use those constants wherever role values are checked or rendered (e.g., the switch/case in UsersColumnDefs.jsx that currently matches 1, 9, 10, 100); ensure imports are added at the top of each file and update any comparisons, select option values, and Tag renderings to use the named constants so role logic stays consistent across components.web/default/src/routes/_authenticated/users/index.tsx (1)
22-23: ⚡ Quick winDerive the role filter enum from the shared role constants.
This route is now another hardcoded copy of the role contract. Role
9had to be added here manually in this PR, and the next role change will need the same update again. Building the tuple fromROLE/USER_ROLEkeeps deep-linked filters aligned with the actual frontend role definitions.Suggested change
-import { canViewAdminArea } from '`@/lib/roles`' +import { ROLE, canViewAdminArea } from '`@/lib/roles`' import { Users } from '`@/features/users`' +const userRoleFilterValues = [ + String(ROLE.USER), + String(ROLE.READ_ONLY_ADMIN), + String(ROLE.ADMIN), + String(ROLE.SUPER_ADMIN), +] as const + const usersSearchSchema = z.object({ page: z.number().optional().catch(1), pageSize: z.number().optional().catch(undefined), filter: z.string().optional().catch(''), status: z .array(z.enum(['1', '2'])) .optional() .catch([]), role: z - .array(z.enum(['1', '9', '10', '100'])) + .array(z.enum(userRoleFilterValues)) .optional() .catch([]), group: z.string().optional().catch(''), })Also applies to: 33-35
🤖 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/default/src/routes/_authenticated/users/index.tsx` around lines 22 - 23, The route currently duplicates the role filter enum as hardcoded numeric values; instead derive the filter tuple from the shared role constants (e.g., ROLE or USER_ROLE) so the deep-linked filters stay in sync with the frontend role definitions—locate the role filter definition in this module (near imports like canViewAdminArea and the Users component and the filter usage around the block covering the other hardcoded values at lines referenced 33-35) and replace the manual numeric tuple with a computed tuple/array built from the shared ROLE/USER_ROLE constants exported by the central role module.
🤖 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 `@middleware/readonly_admin_test.go`:
- Around line 20-31: The test table in readonly_admin_test.go is missing
assertions for the two GET paths that isReadOnlyAdminAllowed() now rejects; add
two entries to the existing cases table with method http.MethodGet, paths
"/api/user/token" and "/api/user/aff", and allowed: false so the test explicitly
fails on regressions — update the same test cases slice used by the existing
loop that exercises isReadOnlyAdminAllowed().
In `@web/classic/src/helpers/utils.jsx`:
- Around line 40-45: getCurrentUserRole is fragile:
JSON.parse(localStorage.getItem('user')) can throw on invalid JSON or return
null which then causes user.role to throw; wrap the read/parse in a try/catch,
validate that the parsed value is a non-null object and that role is a safe
number/string before returning it, and otherwise return the guest fallback 0.
Specifically modify getCurrentUserRole to (1) read localStorage.getItem('user')
into a variable, (2) try to JSON.parse it and catch any SyntaxError, (3) verify
parsedUser && typeof parsedUser === 'object' and typeof parsedUser.role !==
'undefined' (and coerce/validate role if needed), and (4) return parsedUser.role
or 0 on any error or invalid shape.
In `@web/default/src/features/playground/components/message-error.tsx`:
- Around line 22-23: Replace the view-only guard canViewAdminArea() in
message-error.tsx with the write/manage permission guard used for billing (e.g.,
canManageBilling or the equivalent "manage" admin check) so the CTA only shows
to users allowed to mutate billing settings; update the import to pull the
manage-check instead of canViewAdminArea and use that manage-check in both
places where the CTA is gated (the current canViewAdminArea() usages around the
CTA). Ensure the new guard aligns with the billing route's intended admin guard
(the billing $section route) so read-only admins cannot see the mutation
surface.
---
Outside diff comments:
In `@controller/user.go`:
- Around line 529-536: The generateDefaultSidebarConfig function currently uses
json.Marshal(defaultConfig); replace that call with
common.Marshal(defaultConfig) so the repo's json wrapper is used consistently
(keep the same error handling: check the returned error, call common.SysLog on
failure and return an empty string, then return the serialized string on
success). Locate the json.Marshal usage in generateDefaultSidebarConfig and swap
it to common.Marshal while preserving the existing variable names (configBytes,
err) and return behavior.
In `@middleware/auth.go`:
- Around line 70-75: Session-based authorization is using stale session fields
(session.Get("role")/session.Get("status")) so role/status changes don't take
effect; modify the authentication flow in middleware/auth.go (the block where
sessions.Default(c) is read and authHelper is later invoked) to, when a session
exists, fetch the current user record (or call the same user cache lookup used
for token-authenticated requests) and overwrite the role and status values from
that latest source before calling authHelper; ensure you still fall back to
using session id/username but replace session.Get("role") and
session.Get("status") with the freshly reloaded values so demotions/disablements
take effect immediately.
In `@model/user.go`:
- Around line 521-533: The Edit() path fails to persist role changes because the
updates map doesn't include newUser.Role; update the map used by
DB.Model(user).Updates(updates) (in the Edit() method where newUser := *user and
updates := map[string]interface{}{...}) to include "role": newUser.Role when
appropriate (same conditional logic as updatePassword if needed) so
controller.UpdateUser's role changes are written to the DB and
caches/invalidation remain correct.
- Around line 154-161: Replace the direct call to encoding/json's json.Marshal
when serializing defaultConfig with the project's wrapper common.Marshal: call
common.Marshal(defaultConfig) to produce configBytes and handle the returned
(bytes, err) the same way; update the import list to remove "encoding/json" if
no longer used and ensure the common package is imported so the code compiles
(references: defaultConfig, configBytes, err).
In `@web/classic/src/components/topup/modals/TopupHistoryModal.jsx`:
- Around line 71-75: The component memoizes userCanViewAdmin and userIsAdmin
with empty deps, freezing role checks for the modal's lifetime; update these to
reflect role changes by either removing the useMemo and calling canViewAdmin() /
isAdmin() directly where needed (e.g., in loadTopups(), endpoint construction,
and render logic) or include the proper dependencies in the memo (the user/roles
or whatever state/fromContext canViewAdmin depends on). Locate references to
userCanViewAdmin, userIsAdmin, loadTopups, and any endpoint construction using
canViewAdmin() and change them to re-evaluate permissions on each render/fetch
so UI and fetch endpoints stay in sync with current roles.
---
Nitpick comments:
In `@web/classic/src/components/table/users/UsersColumnDefs.jsx`:
- Around line 52-57: Replace the hardcoded numeric role literals with the
centralized role constants from utils.jsx: import ROLE_COMMON,
ROLE_READ_ONLY_ADMIN, ROLE_ADMIN, ROLE_ROOT into UsersColumnDefs.jsx,
AddUserModal.jsx, and EditUserModal.jsx and use those constants wherever role
values are checked or rendered (e.g., the switch/case in UsersColumnDefs.jsx
that currently matches 1, 9, 10, 100); ensure imports are added at the top of
each file and update any comparisons, select option values, and Tag renderings
to use the named constants so role logic stays consistent across components.
In `@web/default/src/routes/_authenticated/users/index.tsx`:
- Around line 22-23: The route currently duplicates the role filter enum as
hardcoded numeric values; instead derive the filter tuple from the shared role
constants (e.g., ROLE or USER_ROLE) so the deep-linked filters stay in sync with
the frontend role definitions—locate the role filter definition in this module
(near imports like canViewAdminArea and the Users component and the filter usage
around the block covering the other hardcoded values at lines referenced 33-35)
and replace the manual numeric tuple with a computed tuple/array built from the
shared ROLE/USER_ROLE constants exported by the central role module.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e16e0962-c8cd-47a5-bd01-f7f4a5fd9249
📒 Files selected for processing (42)
common/constants.gocontroller/user.gomiddleware/auth.gomiddleware/readonly_admin_test.gomodel/user.goweb/classic/src/components/layout/SiderBar.jsxweb/classic/src/components/settings/personal/components/UserInfoHeader.jsxweb/classic/src/components/table/users/UsersColumnDefs.jsxweb/classic/src/components/table/users/modals/AddUserModal.jsxweb/classic/src/components/table/users/modals/EditUserModal.jsxweb/classic/src/components/topup/modals/TopupHistoryModal.jsxweb/classic/src/helpers/auth.jsxweb/classic/src/helpers/utils.jsxweb/classic/src/hooks/dashboard/useDashboardData.jsweb/classic/src/hooks/mj-logs/useMjLogsData.jsweb/classic/src/hooks/task-logs/useTaskLogsData.jsweb/classic/src/hooks/usage-logs/useUsageLogsData.jsxweb/default/src/features/dashboard/components/models/log-stat-cards.tsxweb/default/src/features/dashboard/components/models/models-filter-dialog.tsxweb/default/src/features/dashboard/components/overview/overview-dashboard.tsxweb/default/src/features/dashboard/index.tsxweb/default/src/features/playground/components/message-error.tsxweb/default/src/features/profile/components/tabs/notification-tab.tsxweb/default/src/features/usage-logs/components/common-logs-filter-bar.tsxweb/default/src/features/usage-logs/components/common-logs-stats.tsxweb/default/src/features/usage-logs/components/task-logs-filter-bar.tsxweb/default/src/features/usage-logs/components/usage-logs-table.tsxweb/default/src/features/users/components/users-mutate-drawer.tsxweb/default/src/features/users/constants.tsweb/default/src/features/users/types.tsweb/default/src/features/wallet/hooks/use-billing-history.tsweb/default/src/hooks/use-admin.tsweb/default/src/hooks/use-sidebar-view.tsweb/default/src/i18n/locales/en.jsonweb/default/src/i18n/locales/zh.jsonweb/default/src/lib/roles.tsweb/default/src/routes/_authenticated/channels/index.tsxweb/default/src/routes/_authenticated/models/$section.tsxweb/default/src/routes/_authenticated/models/index.tsxweb/default/src/routes/_authenticated/redemption-codes/index.tsxweb/default/src/routes/_authenticated/subscriptions/index.tsxweb/default/src/routes/_authenticated/users/index.tsx
Comment on lines
+20
to
+31
| {name: "allows ordinary get", method: http.MethodGet, path: "/api/user/self", allowed: true}, | ||
| {name: "allows admin list get", method: http.MethodGet, path: "/api/user/?p=0", allowed: true}, | ||
| {name: "allows head", method: http.MethodHead, path: "/api/user/self", allowed: true}, | ||
| {name: "allows options", method: http.MethodOptions, path: "/api/user/self", allowed: true}, | ||
| {name: "blocks post", method: http.MethodPost, path: "/api/user/", allowed: false}, | ||
| {name: "blocks put", method: http.MethodPut, path: "/api/user/", allowed: false}, | ||
| {name: "blocks delete", method: http.MethodDelete, path: "/api/user/1", allowed: false}, | ||
| {name: "blocks status test get", method: http.MethodGet, path: "/api/status/test", allowed: false}, | ||
| {name: "blocks channel test get", method: http.MethodGet, path: "/api/channel/test/1", allowed: false}, | ||
| {name: "blocks fetch models get", method: http.MethodGet, path: "/api/channel/fetch_models/1", allowed: false}, | ||
| {name: "blocks update balance get", method: http.MethodGet, path: "/api/channel/update_balance/1", allowed: false}, | ||
| } |
Contributor
There was a problem hiding this comment.
Add the two blocked /api/user/* GET cases to this table.
isReadOnlyAdminAllowed() now rejects /api/user/token and /api/user/aff, but this test never asserts either path. A regression there would weaken the new read-only-admin restriction without failing CI.
Suggested change
{name: "blocks status test get", method: http.MethodGet, path: "/api/status/test", allowed: false},
+ {name: "blocks token generate get", method: http.MethodGet, path: "/api/user/token", allowed: false},
+ {name: "blocks aff generate get", method: http.MethodGet, path: "/api/user/aff", allowed: false},
{name: "blocks channel test get", method: http.MethodGet, path: "/api/channel/test/1", allowed: false},
{name: "blocks fetch models get", method: http.MethodGet, path: "/api/channel/fetch_models/1", allowed: false},
{name: "blocks update balance get", method: http.MethodGet, path: "/api/channel/update_balance/1", allowed: false},Based on PR objectives, token generation and aff-code generation are part of the blocked side-effect GET contract for read-only admins.
📝 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.
Suggested change
| {name: "allows ordinary get", method: http.MethodGet, path: "/api/user/self", allowed: true}, | |
| {name: "allows admin list get", method: http.MethodGet, path: "/api/user/?p=0", allowed: true}, | |
| {name: "allows head", method: http.MethodHead, path: "/api/user/self", allowed: true}, | |
| {name: "allows options", method: http.MethodOptions, path: "/api/user/self", allowed: true}, | |
| {name: "blocks post", method: http.MethodPost, path: "/api/user/", allowed: false}, | |
| {name: "blocks put", method: http.MethodPut, path: "/api/user/", allowed: false}, | |
| {name: "blocks delete", method: http.MethodDelete, path: "/api/user/1", allowed: false}, | |
| {name: "blocks status test get", method: http.MethodGet, path: "/api/status/test", allowed: false}, | |
| {name: "blocks channel test get", method: http.MethodGet, path: "/api/channel/test/1", allowed: false}, | |
| {name: "blocks fetch models get", method: http.MethodGet, path: "/api/channel/fetch_models/1", allowed: false}, | |
| {name: "blocks update balance get", method: http.MethodGet, path: "/api/channel/update_balance/1", allowed: false}, | |
| } | |
| {name: "allows ordinary get", method: http.MethodGet, path: "/api/user/self", allowed: true}, | |
| {name: "allows admin list get", method: http.MethodGet, path: "/api/user/?p=0", allowed: true}, | |
| {name: "allows head", method: http.MethodHead, path: "/api/user/self", allowed: true}, | |
| {name: "allows options", method: http.MethodOptions, path: "/api/user/self", allowed: true}, | |
| {name: "blocks post", method: http.MethodPost, path: "/api/user/", allowed: false}, | |
| {name: "blocks put", method: http.MethodPut, path: "/api/user/", allowed: false}, | |
| {name: "blocks delete", method: http.MethodDelete, path: "/api/user/1", allowed: false}, | |
| {name: "blocks status test get", method: http.MethodGet, path: "/api/status/test", allowed: false}, | |
| {name: "blocks token generate get", method: http.MethodGet, path: "/api/user/token", allowed: false}, | |
| {name: "blocks aff generate get", method: http.MethodGet, path: "/api/user/aff", allowed: false}, | |
| {name: "blocks channel test get", method: http.MethodGet, path: "/api/channel/test/1", allowed: false}, | |
| {name: "blocks fetch models get", method: http.MethodGet, path: "/api/channel/fetch_models/1", allowed: false}, | |
| {name: "blocks update balance get", method: http.MethodGet, path: "/api/channel/update_balance/1", allowed: false}, |
🤖 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 `@middleware/readonly_admin_test.go` around lines 20 - 31, The test table in
readonly_admin_test.go is missing assertions for the two GET paths that
isReadOnlyAdminAllowed() now rejects; add two entries to the existing cases
table with method http.MethodGet, paths "/api/user/token" and "/api/user/aff",
and allowed: false so the test explicitly fails on regressions — update the same
test cases slice used by the existing loop that exercises
isReadOnlyAdminAllowed().
Comment on lines
+40
to
+45
| export function getCurrentUserRole() { | ||
| let user = localStorage.getItem('user'); | ||
| if (!user) return false; | ||
| if (!user) return 0; | ||
| user = JSON.parse(user); | ||
| return user.role >= 10; | ||
| return user.role || 0; | ||
| } |
Contributor
There was a problem hiding this comment.
Harden the shared role reader against corrupt localStorage.
Line 43 and Line 44 can now throw on invalid JSON or a parsed null value. Since every updated permission check flows through getCurrentUserRole(), one bad localStorage.user entry can break canViewAdmin(), isAdmin(), route guards, and the affected hooks instead of safely falling back to guest permissions.
Suggested fix
export function getCurrentUserRole() {
- let user = localStorage.getItem('user');
- if (!user) return 0;
- user = JSON.parse(user);
- return user.role || 0;
+ const rawUser = localStorage.getItem('user');
+ if (!rawUser) return 0;
+ try {
+ const user = JSON.parse(rawUser);
+ return Number(user?.role) || 0;
+ } catch {
+ return 0;
+ }
}📝 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.
Suggested change
| export function getCurrentUserRole() { | |
| let user = localStorage.getItem('user'); | |
| if (!user) return false; | |
| if (!user) return 0; | |
| user = JSON.parse(user); | |
| return user.role >= 10; | |
| return user.role || 0; | |
| } | |
| export function getCurrentUserRole() { | |
| const rawUser = localStorage.getItem('user'); | |
| if (!rawUser) return 0; | |
| try { | |
| const user = JSON.parse(rawUser); | |
| return Number(user?.role) || 0; | |
| } catch { | |
| return 0; | |
| } | |
| } |
🤖 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/classic/src/helpers/utils.jsx` around lines 40 - 45, getCurrentUserRole
is fragile: JSON.parse(localStorage.getItem('user')) can throw on invalid JSON
or return null which then causes user.role to throw; wrap the read/parse in a
try/catch, validate that the parsed value is a non-null object and that role is
a safe number/string before returning it, and otherwise return the guest
fallback 0. Specifically modify getCurrentUserRole to (1) read
localStorage.getItem('user') into a variable, (2) try to JSON.parse it and catch
any SyntaxError, (3) verify parsedUser && typeof parsedUser === 'object' and
typeof parsedUser.role !== 'undefined' (and coerce/validate role if needed), and
(4) return parsedUser.role or 0 on any error or invalid shape.
Comment on lines
+22
to
23
| import { canViewAdminArea } from '@/lib/roles' | ||
| import { Alert, AlertDescription, AlertTitle } from '@/components/ui/alert' |
Contributor
There was a problem hiding this comment.
Gate this billing-settings CTA with manage permission, not view permission.
canViewAdminArea() now shows the button to role 9 users, but the target route (web/default/src/routes/_authenticated/system-settings/billing/$section.tsx, Lines 19-39) does not add its own admin guard and lands on a write-oriented billing settings screen. That gives read-only admins a direct path into a mutation surface this PR is supposed to keep non-mutating.
Suggested change
-import { canViewAdminArea } from '`@/lib/roles`'
+import { canManageAdminArea } from '`@/lib/roles`'
...
- const isAdmin = canViewAdminArea(user?.role)
+ const canManageBilling = canManageAdminArea(user?.role)
...
- {isAdmin && (
+ {canManageBilling && (Also applies to: 40-62
🤖 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/default/src/features/playground/components/message-error.tsx` around
lines 22 - 23, Replace the view-only guard canViewAdminArea() in
message-error.tsx with the write/manage permission guard used for billing (e.g.,
canManageBilling or the equivalent "manage" admin check) so the CTA only shows
to users allowed to mutate billing settings; update the import to pull the
manage-check instead of canViewAdminArea and use that manage-check in both
places where the CTA is gated (the current canViewAdminArea() usages around the
CTA). Ensure the new guard aligns with the billing route's intended admin guard
(the billing $section route) so read-only admins cannot see the mutation
surface.
Calcium-Ion
force-pushed
the
main
branch
2 times, most recently
from
August 30, 2026 15:03
51fdfc5 to
2b6f1df
Compare
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
1 participant
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.
What changed
Adds a read-only administrator role for New API. The role can view administrator pages and data, but backend middleware blocks mutations and known side-effect GET endpoints.
Patch placement
common/constants.go,middleware/auth.go,controller/user.go,model/user.gomiddleware/readonly_admin_test.goweb/classic/src/helpers/*,web/classic/src/components/**/*,web/classic/src/hooks/**/*web/default/src/lib/roles.ts,web/default/src/hooks/*,web/default/src/routes/**/*,web/default/src/features/**/*,web/default/src/i18n/locales/*Behavior
9(RoleReadOnlyAdminUser/READ_ONLY_ADMIN)Validation
go test ./common ./middlewaregit diff --cached --checkhttp://127.0.0.1:3001/console/userSummary by CodeRabbit
Release Notes
New Features
Improvements