Feature: Support ordinary users to modify personal information#5511
Feature: Support ordinary users to modify personal information#5511nobodyiam merged 1 commit intoapolloconfig:masterfrom
Conversation
dosubot
bot
added
the
size:L
WalkthroughAdds runtime permission checks to allow ordinary users to modify only their own info, wires UnifiedPermissionValidator into UserInfoController, updates frontend to load user data based on root permission, adjusts templates to hide admin controls for non-root users, and expands tests for admin/non-admin paths. (50 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 0
♻️ Duplicate comments (1)
apollo-portal/src/main/resources/static/scripts/controller/UserController.js (1)
43-67: LGTM!The conditional data loading logic correctly differentiates between root and non-root users:
- Root users receive all created users for full management capability
- Non-root users receive only their own info for self-service editing
The missing semicolons on lines 56 and 65 were already flagged in previous review comments.
🧹 Nitpick comments (2)
apollo-portal/src/main/resources/static/user-manage.html (1)
158-167: Consider hiding the "Enabled" field from non-admin users in the edit form.Non-admin users can see and interact with the enabled/disabled radio buttons when editing their profile. While the current backend allows the update to proceed (since username matches), exposing this field to non-admin users is unnecessary and could lead to confusion or unintended self-disabling.
Apply this diff to hide the enabled field for non-admin users:
- <div class="form-group" valdr-form-group> + <div class="form-group" valdr-form-group ng-show="isRootUser"> <label class="col-sm-2 control-label"> <apollorequiredfield></apollorequiredfield> {{'UserMange.Enabled' | translate }} </label> <div class="col-sm-5"> <input type="radio" name="enabled" ng-model="user.enabled" ng-value="1"/> {{'UserMange.Enable' | translate }} <input type="radio" name="enabled" ng-model="user.enabled" ng-value="0"/> {{'UserMange.Disable' | translate }} </div> </div>apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/UserInfoController.java (1)
62-89: Consider restricting field modifications for non-admin users.The current implementation correctly prevents non-admin users from modifying other users' data. However, it allows non-admin users to modify all fields of their own profile, including the
enabledstatus. A user could potentially disable their own account via a direct API call.Consider adding server-side protection to prevent non-admin users from modifying sensitive fields:
if (!unifiedPermissionValidator.isSuperAdmin() && !user.getUsername().equals(userInfoHolder.getUser().getUserId())) { throw new UnsupportedOperationException("Create or update user operation is unsupported"); } + + // Prevent non-admin users from modifying their own enabled status + if (!unifiedPermissionValidator.isSuperAdmin()) { + UserInfo currentUser = userInfoHolder.getUser(); + user.setEnabled(currentUser.getEnabled()); + }This ensures non-admin users can only update allowed fields (name, email, password) while preserving their current enabled status.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGES.md(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/UserInfoController.java(3 hunks)apollo-portal/src/main/resources/static/scripts/controller/UserController.js(1 hunks)apollo-portal/src/main/resources/static/user-manage.html(3 hunks)apollo-portal/src/main/resources/static/views/common/nav.html(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/UserInfoControllerTest.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/UserInfoControllerTest.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
🔇 Additional comments (10)
CHANGES.md (1)
26-26: LGTM!The changelog entry correctly documents the new feature and follows the established format.
apollo-portal/src/main/resources/static/user-manage.html (2)
38-72: LGTM!The UI gating correctly hides admin-only controls (add user button, search/filter functionality) from non-admin users while keeping the user table visible.
99-101: LGTM!The Enable/Disable button is properly gated to be visible only for root users.
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/UserInfoControllerTest.java (4)
49-60: LGTM!Good test coverage for the admin path, verifying that super admins can create/update users and that password validation is still invoked.
62-79: LGTM!Test correctly validates that non-admin users can update their own information when the username matches.
81-96: LGTM!Test correctly verifies that non-admin users attempting to update another user's information results in
UnsupportedOperationException.
98-115: LGTM!Updated test appropriately mocks the super admin check before testing password validation failure.
apollo-portal/src/main/resources/static/scripts/controller/UserController.js (1)
35-41: LGTM!Good refactoring to defer
getCreatedUsers()until after the permission check completes, ensuring data loading is permission-aware.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/UserInfoController.java (1)
50-60: LGTM!The
UnifiedPermissionValidatordependency is properly injected and follows the existing constructor injection pattern used by other dependencies in this controller.apollo-portal/src/main/resources/static/views/common/nav.html (1)
73-81: LGTM!The non-admin tools dropdown appropriately mirrors the structure of the admin tools dropdown while limiting functionality to user self-management. The condition style is consistent with the existing admin tools check at line 56. The translation key
Common.Nav.NonAdminToolsis properly defined in both locale files (English: "Tools", Chinese: "工具").
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apollo-portal/src/main/resources/static/scripts/controller/UserController.js (1)
35-41: Handle failures fromhas_root_permission()to avoid blank user listRight now
getCreatedUsers()is only called in the success handler ofPermissionService.has_root_permission(). If that promise ever rejects (network/server error), the controller never populatescreatedUsers/filterUser, leaving the user list blank with no feedback.Consider adding an error handler (second argument to
thenor a.catch) that at least surfaces an error viaAppUtil.showErrorMsgand/or falls back to treating the user as non-root and callinggetCreatedUsers()with$scope.isRootUser = false.apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/UserInfoControllerTest.java (1)
97-112: Consider verifying the exception message for better test diagnostics.While the test correctly expects
UnsupportedOperationException, verifying the exception message would make failures easier to diagnose.Apply this diff to verify the exception message:
- @Test(expected = UnsupportedOperationException.class) public void testUpdateOtherUserFailedForNoAdmin() { UserPO user = new UserPO(); user.setUsername("username"); user.setUserDisplayName("displayName"); user.setPassword("password"); UserInfo currentUserInfo = new UserInfo(); currentUserInfo.setUserId("username_other"); currentUserInfo.setName("displayName_other"); Mockito.when(unifiedPermissionValidator.isSuperAdmin()).thenReturn(false); Mockito.when(userInfoHolder.getUser()).thenReturn(currentUserInfo); - userInfoController.createOrUpdateUser(true, user); + try { + userInfoController.createOrUpdateUser(true, user); + Assert.fail("Expected UnsupportedOperationException"); + } catch (UnsupportedOperationException e) { + // Optionally verify message contains relevant info + // Assert.assertTrue(e.getMessage().contains("username")); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGES.md(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/UserInfoController.java(3 hunks)apollo-portal/src/main/resources/static/scripts/controller/UserController.js(1 hunks)apollo-portal/src/main/resources/static/user-manage.html(4 hunks)apollo-portal/src/main/resources/static/views/common/nav.html(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/UserInfoControllerTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apollo-portal/src/main/resources/static/views/common/nav.html
- apollo-portal/src/main/resources/static/user-manage.html
- CHANGES.md
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/UserInfoController.java
🧰 Additional context used
🧬 Code graph analysis (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/UserInfoControllerTest.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
🔇 Additional comments (3)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/UserInfoControllerTest.java (3)
20-21: LGTM!The new mock dependencies align with the permission-checking requirements for admin vs non-admin flows.
Also applies to: 23-23, 44-47
114-130: LGTM!The test correctly validates that non-admin users cannot disable their own accounts, which is an appropriate security restriction.
132-167: LGTM!Both test methods correctly validate failure scenarios:
- Non-admin users cannot disable other users.
- Weak password validation properly throws
BadRequestExceptionwith the expected message.
| function getCreatedUsers() { | ||
| UserService.find_users("",true) | ||
| .then(function (result) { | ||
| if (!result || result.length === 0) { | ||
| return; | ||
| } | ||
| $scope.createdUsers = []; | ||
| $scope.filterUser = []; | ||
| result.forEach(function (user) { | ||
| $scope.createdUsers.push(user); | ||
| $scope.filterUser.push(user); | ||
| if ($scope.isRootUser) { | ||
| UserService.find_users("",true) | ||
| .then(function (result) { | ||
| if (!result || result.length === 0) { | ||
| return; | ||
| } | ||
| $scope.createdUsers = []; | ||
| $scope.filterUser = []; | ||
| result.forEach(function (user) { | ||
| $scope.createdUsers.push(user); | ||
| $scope.filterUser.push(user); | ||
| }); | ||
| }); | ||
| }) | ||
| } else { | ||
| UserService.load_user() | ||
| .then(function (result) { | ||
| if (!result) { | ||
| return; | ||
| } | ||
| $scope.createdUsers = [result]; | ||
| $scope.filterUser = [result]; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Avoid stale createdUsers state and unify update logic for root vs non‑root
The root/non-root split in getCreatedUsers() is directionally correct for limiting non-root users to their own data, but there are a couple of edge cases worth tightening up:
- Stale data on empty/failed responses
-
Root branch:
.then(function (result) { if (!result || result.length === 0) { return; } $scope.createdUsers = []; $scope.filterUser = []; // ... });
-
Non-root branch:
.then(function (result) { if (!result) { return; } $scope.createdUsers = [result]; $scope.filterUser = [result]; });
If a previous call succeeded and a later call returns null, undefined, or an empty array (e.g., transient error or all users filtered out server-side), these early returns keep the old createdUsers / filterUser values, so the UI will still show stale data.
It’s safer to reset the arrays before the if (!result ...) checks so that a failed/empty response results in an empty UI state instead of old data. For example:
- .then(function (result) {
- if (!result || result.length === 0) {
- return;
- }
- $scope.createdUsers = [];
- $scope.filterUser = [];
+ .then(function (result) {
+ $scope.createdUsers = [];
+ $scope.filterUser = [];
+ if (!result || result.length === 0) {
+ return;
+ }
result.forEach(function (user) {
$scope.createdUsers.push(user);
$scope.filterUser.push(user);
});
});
@@
- .then(function (result) {
- if (!result) {
- return;
- }
- $scope.createdUsers = [result];
- $scope.filterUser = [result];
- });
+ .then(function (result) {
+ $scope.createdUsers = [];
+ $scope.filterUser = [];
+ if (!result) {
+ return;
+ }
+ $scope.createdUsers.push(result);
+ $scope.filterUser.push(result);
+ });- Minor duplication / consistency
Both branches are conceptually doing the same thing: “load a list of users and populate createdUsers / filterUser.” You could optionally extract a small helper (e.g., setUsers(users)), or at least keep the array update pattern consistent between branches (either always clear-and-push, or always assign new arrays) to reduce cognitive load when maintaining this controller.
These are small edge-case and readability improvements; the main permission split looks good.
🤖 Prompt for AI Agents
In apollo-portal/src/main/resources/static/scripts/controller/UserController.js
around lines 43 to 66, the current getCreatedUsers() leaves previous
createdUsers/filterUser intact when subsequent calls return
null/undefined/empty, and the root vs non-root branches duplicate logic; to fix,
reset $scope.createdUsers and $scope.filterUser to empty arrays before the
promise result checks, then consistently populate them from the response
(convert non-root single user into an array or always assign a new array), or
extract a small setUsers(users) helper and call it from both branches so
empty/failure responses clear the UI and both branches share the same update
logic.
| @Test | ||
| public void testCreateOrUpdateUserForAdmin() { | ||
| UserPO user = new UserPO(); | ||
| user.setUsername("username"); | ||
| user.setPassword("password"); | ||
| user.setEnabled(1); | ||
|
|
||
| Mockito.when(unifiedPermissionValidator.isSuperAdmin()).thenReturn(true); | ||
| Mockito.when(userPasswordChecker.checkWeakPassword(Mockito.anyString())) | ||
| .thenReturn(new CheckResult(Boolean.TRUE, "")); | ||
|
|
||
| userInfoController.createOrUpdateUser(true, user); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add verification to ensure the service method is called correctly.
The test only confirms that no exception is thrown, but it doesn't verify that userService.createOrUpdateUser() was actually invoked with the expected parameters.
Apply this diff to add verification:
Mockito.when(userPasswordChecker.checkWeakPassword(Mockito.anyString()))
.thenReturn(new CheckResult(Boolean.TRUE, ""));
userInfoController.createOrUpdateUser(true, user);
+ Mockito.verify(userService).createOrUpdateUser(user);
}📝 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.
| @Test | |
| public void testCreateOrUpdateUserForAdmin() { | |
| UserPO user = new UserPO(); | |
| user.setUsername("username"); | |
| user.setPassword("password"); | |
| user.setEnabled(1); | |
| Mockito.when(unifiedPermissionValidator.isSuperAdmin()).thenReturn(true); | |
| Mockito.when(userPasswordChecker.checkWeakPassword(Mockito.anyString())) | |
| .thenReturn(new CheckResult(Boolean.TRUE, "")); | |
| userInfoController.createOrUpdateUser(true, user); | |
| } | |
| @Test | |
| public void testCreateOrUpdateUserForAdmin() { | |
| UserPO user = new UserPO(); | |
| user.setUsername("username"); | |
| user.setPassword("password"); | |
| user.setEnabled(1); | |
| Mockito.when(unifiedPermissionValidator.isSuperAdmin()).thenReturn(true); | |
| Mockito.when(userPasswordChecker.checkWeakPassword(Mockito.anyString())) | |
| .thenReturn(new CheckResult(Boolean.TRUE, "")); | |
| userInfoController.createOrUpdateUser(true, user); | |
| Mockito.verify(userService).createOrUpdateUser(user); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/UserInfoControllerTest.java
around lines 49 to 61, the test testCreateOrUpdateUserForAdmin currently only
exercises the controller but does not verify that
userService.createOrUpdateUser(...) was invoked; add a Mockito.verify call after
invoking userInfoController.createOrUpdateUser(true, user) to assert
userService.createOrUpdateUser(true, user) was called (optionally verify no more
interactions with userService).
| @Test | ||
| public void testCreateOrUpdateUser() { | ||
| public void testDisableUserForAdmin() { | ||
| UserPO user = new UserPO(); | ||
| user.setUsername("username"); | ||
| user.setPassword("password"); | ||
| user.setEnabled(0); | ||
|
|
||
| Mockito.when(unifiedPermissionValidator.isSuperAdmin()).thenReturn(true); | ||
| Mockito.when(userPasswordChecker.checkWeakPassword(Mockito.anyString())) | ||
| .thenReturn(new CheckResult(Boolean.TRUE, "")); | ||
|
|
||
| userInfoController.createOrUpdateUser(true, user); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add verification to ensure the service method is called correctly.
Similar to testCreateOrUpdateUserForAdmin, this test should verify that the underlying service method is invoked.
Apply this diff to add verification:
Mockito.when(userPasswordChecker.checkWeakPassword(Mockito.anyString()))
.thenReturn(new CheckResult(Boolean.TRUE, ""));
userInfoController.createOrUpdateUser(true, user);
+ Mockito.verify(userService).createOrUpdateUser(user);
}📝 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.
| @Test | |
| public void testCreateOrUpdateUser() { | |
| public void testDisableUserForAdmin() { | |
| UserPO user = new UserPO(); | |
| user.setUsername("username"); | |
| user.setPassword("password"); | |
| user.setEnabled(0); | |
| Mockito.when(unifiedPermissionValidator.isSuperAdmin()).thenReturn(true); | |
| Mockito.when(userPasswordChecker.checkWeakPassword(Mockito.anyString())) | |
| .thenReturn(new CheckResult(Boolean.TRUE, "")); | |
| userInfoController.createOrUpdateUser(true, user); | |
| } | |
| @Test | |
| public void testDisableUserForAdmin() { | |
| UserPO user = new UserPO(); | |
| user.setUsername("username"); | |
| user.setPassword("password"); | |
| user.setEnabled(0); | |
| Mockito.when(unifiedPermissionValidator.isSuperAdmin()).thenReturn(true); | |
| Mockito.when(userPasswordChecker.checkWeakPassword(Mockito.anyString())) | |
| .thenReturn(new CheckResult(Boolean.TRUE, "")); | |
| userInfoController.createOrUpdateUser(true, user); | |
| Mockito.verify(userService).createOrUpdateUser(user); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/UserInfoControllerTest.java
around lines 63 to 75, add a Mockito verification to assert the controller
delegated to the service: after calling
userInfoController.createOrUpdateUser(true, user); add a verify call such as
Mockito.verify(userInfoService, Mockito.times(1)).createOrUpdateUser(true,
user); (adjust the service field name if different) to ensure the underlying
service method was invoked with the expected arguments.
| @Test | ||
| public void testUpdateUserForNoAdmin() { | ||
| UserPO user = new UserPO(); | ||
| user.setUsername("username"); | ||
| user.setUserDisplayName("displayName"); | ||
| user.setPassword("password"); | ||
| user.setEnabled(1); | ||
|
|
||
| UserInfo currentUserInfo = new UserInfo(); | ||
| currentUserInfo.setUserId("username"); | ||
| currentUserInfo.setName("displayName"); | ||
|
|
||
| Mockito.when(unifiedPermissionValidator.isSuperAdmin()).thenReturn(false); | ||
| Mockito.when(userInfoHolder.getUser()).thenReturn(currentUserInfo); | ||
| Mockito.when(userPasswordChecker.checkWeakPassword(Mockito.anyString())) | ||
| .thenReturn(new CheckResult(Boolean.TRUE, "")); | ||
|
|
||
| userInfoController.createOrUpdateUser(true, user); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add verification to ensure non-admin self-update succeeds.
This test validates the core feature of this PR—ordinary users modifying their own info. It should verify that userService.createOrUpdateUser() is called with the correct parameters.
Apply this diff to add verification:
Mockito.when(userPasswordChecker.checkWeakPassword(Mockito.anyString()))
.thenReturn(new CheckResult(Boolean.TRUE, ""));
userInfoController.createOrUpdateUser(true, user);
+ Mockito.verify(userService).createOrUpdateUser(user);
}🤖 Prompt for AI Agents
In
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/UserInfoControllerTest.java
around lines 77 to 95, the test does not assert that the controller forwarded
the update to the service; add a Mockito.verify call after
userInfoController.createOrUpdateUser(true, user) to assert
userService.createOrUpdateUser was invoked with the expected arguments (verify
the boolean flag and the same user instance, e.g.
Mockito.verify(userService).createOrUpdateUser(Mockito.eq(true),
Mockito.eq(user))).
|
Ordinary users modify their personal information page |
2 participants
What's the purpose of this PR
Support ordinary users to modify personal information
Brief changelog
Support ordinary users to modify personal information
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.mvn spotless:applyto format your code.CHANGESlog.Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.