-
Notifications
You must be signed in to change notification settings - Fork 1.2k
server,ui: prevent role change for default accounts #11761
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 4.20
Are you sure you want to change the base?
Conversation
Fixes apache#10931 Role for default accounts shouldn't be changed. Appropriate error should be returned by the server and UI should not present option for them. Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.20 #11761 +/- ##
=========================================
Coverage 16.17% 16.17%
- Complexity 13296 13299 +3
=========================================
Files 5656 5656
Lines 498223 498248 +25
Branches 60454 60460 +6
=========================================
+ Hits 80577 80588 +11
- Misses 408676 408688 +12
- Partials 8970 8972 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@blueorangutan package |
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15231 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR prevents role changes for default accounts by adding server-side validation and UI restrictions. When users attempt to modify roles for default accounts, the server returns an appropriate error message and the UI hides the role selection option.
- Added server-side validation to prevent role changes for default accounts
- Modified UI to conditionally hide role selection for default accounts
- Added comprehensive test coverage for the new validation logic
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
ui/src/views/iam/EditAccount.vue | Conditionally hides role form field for default accounts and safely handles roleid parameter |
server/src/main/java/com/cloud/user/AccountManagerImpl.java | Adds validation to prevent role changes for default accounts with early return optimization |
server/src/test/java/com/cloud/user/AccountManagerImplTest.java | Adds test case to verify default account role change restriction |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@blueorangutan package |
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15250 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm and makes sense
Description
Fixes #10931
Role for default accounts shouldn't be changed. Appropriate error should be returned by the server and UI should not present option for them.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?