Skip to content

[Studio] feat: add user credential security insights - #4093

Closed
coder999o wants to merge 1 commit into
apache:rocketmq-studiofrom
coder999o:fix-090801
Closed

coder999o wants to merge 1 commit into
apache:rocketmq-studiofrom
coder999o:fix-090801

Conversation

@coder999o

Copy link
Copy Markdown
Contributor

What is the purpose of the change

This PR adds user credential security insights to RocketMQ Studio.

The Studio user management page now shows password strength guidance when creating or resetting users, summarizes visible account security risks, and highlights password rotation status from existing user metadata. This helps administrators spot weak passwords, stale administrator credentials, unknown password age, and single active administrator risk before changing account state.

Brief changelog

  • Add a Studio user security analyzer for password strength, password rotation, and account risk summaries.
  • Add a reusable credential security panel for password forms and the user list overview.
  • Show password rotation status and password changed time in the Studio user table.
  • Add focused unit tests and page rendering tests for the new security insights.

Verifying this change

  • npm run test -- --run src/utils/studioUserSecurity.test.ts src/pages/studio/__tests__/UserManagement.test.tsx
  • git diff --check
  • npm run lint -- src/utils/studioUserSecurity.ts src/utils/studioUserSecurity.test.ts src/components/StudioUserSecurityPanel.tsx src/pages/studio/UserManagement.tsx src/pages/studio/__tests__/UserManagement.test.tsx
  • npm run build

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Adds a user credential security panel to Studio with password strength evaluation, rotation tracking, and security overview for user management. Well-structured with clean separation of concerns (utility functions in studioUserSecurity.ts, UI components in StudioUserSecurityPanel.tsx), comprehensive test coverage, and proper TypeScript types.

LGTM.


Automated review by github-manager-bot

@lizhimins lizhimins left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The password-strength meter in here is good work — the scoring is real, the detectors exercise genuine branches, and the tests are substantive rather than mock echo. We hand-recomputed two of your fixtures and they check out (operator123 → score 20 → weak; Ops#2026-ClusterSafe → 100 → strong). But the "credential security insights" half cannot be merged, because several of its findings are computed from data that does not exist or from a slice that cannot support the claim. Details below.

First: rebase required

#4060 has just been merged into rocketmq-studio as 0a59666. It makes activeSessionCount: number a required field on StudioUser, and this branch constructs four StudioUser literals without it, so it will not typecheck until you rebase. Expect roughly nine overlapping regions in UserManagement.tsx — both PRs rewrite the same columns array (trunk :223), both edit the same passwordTarget literal (:292-300), both insert a statistics panel in the same slot, and both add scroll={{ x: tableScrollX(columns) }} to the same <Table> (which would produce a duplicate JSX attribute if merged naively). Please resolve deliberately rather than accepting one side wholesale.

Blocking: findings computed from data that cannot exist

1. "当前 TPS / 今日消息"-class problem — the overview aggregates the current page only

analyzeStudioUsers computes counts, distinct-admin detection, risk buckets and a weighted score in the browser, over whatever page of users happens to be loaded. Every figure therefore changes meaning under pagination, role filtering and status filtering, while being presented as a property of the deployment.

This project has returned several PRs for exactly this pattern (#3129 computed a "global coverage rate" from a post-filter slice of at most 20 rows). The fix is to move the aggregation server-side — AuditSummaryVO and MybatisPlusAuditRepository.summarize (:110-140) are the established shape — or to drop the overview and keep only the password meter, which operates on the form in front of the user and so is legitimately client-side.

2. ONLY_ONE_ACTIVE_ADMIN will fire permanently on a correct deployment

The server already enforces this invariant, with a row lock: AuthService.setUserEnabled:219-231 uses last("FOR UPDATE"). Your check re-implements it over the current page, unlocked. Consequences:

  • Any legitimate single-admin deployment — which is the normal case — shows a permanent error-severity finding.
  • Under a role or status filter, or on page 2 of a paginated list, the count is of the visible slice, so the finding's meaning changes silently.

Either drop it and let the server own the invariant, or compute it server-side where the count is real.

3. UNKNOWN_PASSWORD_AGE is dead code

password_changed_at is NOT NULL DEFAULT CURRENT_TIMESTAMP (deploy/mysql/upgrade-studio-user.sql:13) and is set on every write path (AuthService.java:203, :251, :351). There is no reachable state in which the password age is unknown. Your test only passes by hand-feeding '' and 'not-a-date' into the analyser, i.e. it tests a branch production cannot enter.

4. The policy is display-only, so it enforces nothing

Four checks are painted severity: 'error', but they are not wired into the Form.Item rules, and the server accepts any password of 8-256 characters (AuthService.validatePassword:406-410). The 180-day rotation constant has no server-side counterpart and no setting. So the panel tells an admin their password is invalid while the system happily accepts it.

Either enforce on both sides (form rules plus validatePassword), or downgrade these to warning and stop implying a policy the product does not have.

Blocking: project rules

  1. fontSize: 12 in UserManagement.tsx. The project minimum is 14px, and trunk currently has zero sub-14px font sizes in web/src. This would be the first regression.
  2. Persistent notice uses Alert instead of InfoBanner. Page-level standing explanations go in web/src/components/InfoBanner; antd Alert is reserved for semantic error/warning/success states. Worse, with zero users the panel renders a green success Alert (info-severity risk → status: 'healthy', score 100), so an empty deployment reads as "all clear".
  3. Hand-rolled panel div with manual border/padding/background, instead of Card or the existing AuditSummaryCards.tsx:52-70. Related: the filename has no matching default export and neither named export matches the filename — trunk convention is that they do (InfoBanner.tsx:63, PageHeader.tsx:49, StatusBadge.tsx:42).
  4. Declared column widths total 1230px, which overflows a 1440px viewport's content area (≈1172px) and produces a default horizontal scrollbar. Note #4060 just solved the same problem on this same table by bringing its total to 1116px — after you rebase, use one agreed set of widths, not two.

Should fix

  1. The description overclaims. It says the panel can "spot weak passwords". It cannot: no hash or plaintext for an existing account is available to the frontend, or should be. What it actually does is grade a password being typed right now, and count password age. Please describe those two things, because "finds weak passwords in your user base" implies a capability that would be a security problem if it existed.
  2. Split out the drive-by fixes. Two unrelated changes ride along: closeCreateUserModal now resets fields on cancel (trunk :379 did not), and username: userName ?? '' fixes a genuine trunk bug where :294 passed '', making the dialog title read "重置 的密码". The second one is a real fix and we want it — as its own small PR, so it can be merged immediately instead of waiting on this one.

One weak assertion

.not.toEqual(expect.arrayContaining([...])) passes if any one of the three listed findings is absent, so it does not pin what it appears to. Assert the exact array, or assert each absence separately.

Confirmed clean, so you do not need to re-check

We verified the security surface and found no widening: this PR touches no backend files, reads only the already admin-gated GET /api/studio-users (AuthInterceptor.java:128), and exposes no secret, hash, partial key or last-used value. Password grading uses local form state only. authStore.user does exist (authStore.ts:25), so the userName selector is valid. The 12 new it() blocks are genuine tests, not tautologies.

Suggested path

Shrink this to the password-strength meter on the create/reset forms — that part is good and is legitimately client-side — and either drop the insights overview or re-propose it as a server-side endpoint returning real figures. We would merge the meter quickly.

@lizhimins lizhimins left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues, and the branch now conflicts with #4231 (merged as 09b7b1f) on the same file.

  • studioUserSecurity.ts:438: inspectedCount > 1 && activeAdmins.length === 1 raises a warning on a normal single-administrator deployment, i.e. permanently for the common case.
  • UNKNOWN_PASSWORD_AGE (:64, :329-337, :428) is unreachable: schema.sql:24 declares password_changed_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, so the server always has a value.
  • UserManagement.tsx:251 uses fontSize: 12; the project minimum is 14px.

Please also decide how this panel relates to the per-user session details that just landed — the two overlap on the same page and the same AuthService data.

@lizhimins
lizhimins deleted the branch apache:rocketmq-studio September 16, 2026 12:48
@lizhimins lizhimins closed this Sep 16, 2026
@coder999o
coder999o deleted the fix-090801 branch September 25, 2026 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants