Skip to content

fix(acl): keep the account whitelist in the update response - #4122

Closed
zjncs wants to merge 1 commit into
apache:rocketmq-studiofrom
zjncs:fix/acl-user-update-keeps-whitelist
Closed

fix(acl): keep the account whitelist in the update response#4122
zjncs wants to merge 1 commit into
apache:rocketmq-studiofrom
zjncs:fix/acl-user-update-keeps-whitelist

Conversation

@zjncs

@zjncs zjncs commented Sep 8, 2026

Copy link
Copy Markdown

Motivation

AclService.updateUser (local plain-access path) carefully preserves every existing field of the account through a partial update — accessKey, secretKey, admin, clusters, gmtCreate — but the merge builder omits whiteRemoteAddress:

AclUserVO merged = AclUserVO.builder()
        .id(existing.getId())
        .username(...)
        .accessKey(existing.getAccessKey())
        .secretKey(existing.getSecretKey())
        .admin(...)
        .clusters(...)
        .gmtCreate(existing.getGmtCreate())
        .build();

whiteRemoteAddress is a real, persisted field of local plain-access accounts (rmq_acl_user.white_remote_address, written by createAndUpdatePlainAccessConfig, read back by toUserVO). The stored row survives the update (MyBatis-Plus updateById skips null columns), but replaceUser returns the merged VO, so POST /api/acl/users/update responds with whiteRemoteAddress: null after a harmless rename — the API reports the account's IP whitelist as gone even though it is still enforced.

Modifications

Carry existing.getWhiteRemoteAddress() through the merge, like every other preserved field.

Verification

New test updateUserShouldKeepTheExistingWhiteRemoteAddress (model: updateUserShouldSaveExistingUser): existing user with whiteRemoteAddress = "10.0.1.0/24", rename via DTO.

  • Before the fix: fails — whiteRemoteAddress in the response is null.
  • After the fix: passes.
  • ACL suite regression: mvn -f server/pom.xml test -Dtest='AclServiceTest,MybatisPlusAclRepositoryTest,AclControllerTest'Tests run: 114, Failures: 0, Errors: 0.

updateUser's merge builder preserves every existing field it re-reads
(id, accessKey, secretKey, admin, clusters, gmtCreate) but drops
whiteRemoteAddress, so the API response after a harmless rename reports
the IP whitelist as gone. The stored row survives (updateById skips
null columns), yet the update endpoint's reply - and anything the
caller derives from it - loses the value. Carry it through the merge.

Signed-off-by: zjncs <18910855655@163.com>
Copilot AI lite review requested due to automatic review settings September 8, 2026 03:52

Copilot 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@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

Fixes a field omission in AclService.updateUser where whiteRemoteAddress was not carried through the merge builder, causing the API to respond with null after a harmless rename even though the stored row was preserved. One-line fix. LGTM.


Automated review by RockteMQ-AI

lizhimins pushed a commit that referenced this pull request Sep 9, 2026
…ate (#4113)

Two ACL account defects, folded into one change because they touch the same account read/write path.

`toPlainAccessConfig` read an account's rules through `ruleQuery()`, whose principal filter is a substring `LIKE`. Accounts whose access keys contain one another were merged: reading `svc-a` also returned the rules of `svc-a-v2`, and because `upsertPlainAccessRules` deletes by exact principal before writing, the absorbed rules were written back verbatim onto the wrong account. The read now uses an exact `eq("principal", ...)` wrapper with the same ordering, mirroring the delete.

`AclService.updateUser` merged the incoming account onto the stored one but never carried `whiteRemoteAddress` across, so the update response reported a null whitelist even though `replaceUser` does not write that column at all. The web ACL page replaces its local row with that response without refetching, so the whitelist visibly disappeared until the next reload. The merged builder now keeps the existing value.

Folded in from #4122, which made the same one-line whitelist fix independently; that PR is closed as superseded by this one.
@lizhimins

Copy link
Copy Markdown
Member

Closing as folded into #4113, which was merged into rocketmq-studio as 0c54d98.

Your diagnosis and your fix here were correct and are now on the branch: AclService.updateUser merges the incoming account onto the stored one but never carried whiteRemoteAddress across, so the update response reported a null whitelist even though MybatisPlusAclRepository.toUserEntity does not map white_remote_address at all — meaning the column is untouched by replaceUser and this was purely a response-shape defect. That matters in practice because web/src/pages/instance/acl.tsx replaces its local row with the response and does not refetch, so the whitelist visibly disappeared until the next reload. The one-line .whiteRemoteAddress(existing.getWhiteRemoteAddress()) in the merged builder is exactly what shipped.

The reason this landed inside #4113 rather than on its own: both PRs fix the same ACL account read/write path, arrived within an hour of each other from the same author, and neither touches a file the other does. We group related same-author changes into one commit on the target PR so the branch history stays readable, and #4113 was the target here. Your updateUserShouldKeepTheExistingWhiteRemoteAddress test came across with the fix and passes as part of AclServiceTest (65 tests green).

Nothing further needed from you — thank you for the fix and for the precise write-up of why a null whitelist in the response is user-visible rather than cosmetic.

@lizhimins lizhimins closed this Sep 9, 2026
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.

4 participants