list only own zones for resource admin#11087
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR tightens zone-level access for resource managers and applies broad Java refactoring to modernize code style.
- Enforce per-zone permissions in
listDataCentersInternalby renamingidtozoneIdand invokingcheckAccessAndSpecifyAuthority. - Remove unused imports and private methods across
QueryManagerImpl, and standardize Java 7+ features (diamond operators,toArray(new T[0])). - Simplify API commands by dropping redundant static names/overrides and adopting diamond operators in response list declarations.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/src/main/java/com/cloud/api/query/QueryManagerImpl.java | Enforce zone access control, remove unused code, and refactor to use diamond operators and array patterns. |
| api/src/main/java/org/apache/cloudstack/api/command/user/zone/ListZonesCmd.java | Remove obsolete getCommandName() override and static response name. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/pod/ListPodsByCmd.java | Adopt diamond operators in ListResponse and collection initializations. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/cluster/ListClustersCmd.java | Simplify Pair constructions and apply diamond operators to list declarations. |
Comments suppressed due to low confidence (2)
server/src/main/java/com/cloud/api/query/QueryManagerImpl.java:3151
- The internal method
searchForStorageTagsInternalno longer accepts the command parameter, so any filtering based onListStorageTagsCmdfields may be skipped. Restore or pass the command to preserve expected filtering behavior.
Pair<List<StoragePoolTagVO>, Integer> result = searchForStorageTagsInternal();
server/src/main/java/com/cloud/api/query/QueryManagerImpl.java:3192
- The signature for
searchForHostTagsInternalwas changed to drop theListHostTagsCmdargument, which likely removes filtering by command parameters. Confirm that host-tag filters are still applied or reintroduce the parameter.
Pair<List<HostTagVO>, Integer> result = searchForHostTagsInternal();
|
@blueorangutan package |
|
@DaanHoogland 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 ✔️ debian ✔️ suse15. SL-JID 13913 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.20 #11087 +/- ##
==========================================
Coverage 16.15% 16.16%
- Complexity 13273 13275 +2
==========================================
Files 5657 5656 -1
Lines 497898 497767 -131
Branches 60374 60363 -11
==========================================
+ Hits 80435 80443 +8
+ Misses 408505 408370 -135
+ Partials 8958 8954 -4
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:
|
|
[SF] Trillian test result (tid-13660)
|
|
Few comments;
|
|
@blueorangutan package |
|
@DaanHoogland 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 ✔️ debian ✔️ suse15. SL-JID 14163 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-13770)
|
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 14212 |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14233 |
|
[SF] Trillian test result (tid-13811)
|
|
@nvazquez can you share your results here or on the issue, please? I want to move this forwards. |
nvazquez
left a comment
There was a problem hiding this comment.
LGTM - Manually tested:
As an admin:
- Create domain and zone
- Dedicate the zone to the domain
- Create a new Role based on type = Resource Admin and create an account from it
- Assigned the API rules to the new role according to the issue #10906
- Changed the value of the setting:
allow.user.view.all.zonesto false
As the resource admin from the new domain:
- Listed zones -> verified only the dedicated zone is visible
- Tested adding a cluster -> verified only the dedicated zone is listed on the Zones dropdown, similar on Pods, Hosts.
Just a small remark:
When the setting allow.user.view.all.zones is true, the resource admin is not able to list Disabled zones. I think this is out of scope from this PR, or perhaps the role needs extra API permissions
@DaanHoogland can you please update the PR description with a fuctional description?
Co-authored-by: Suresh Kumar Anaparti <sureshkumar.anaparti@gmail.com>
|
@blueorangutan package |
|
@DaanHoogland 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 ✔️ debian ✔️ suse15. SL-JID 14298 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-13851)
|
Co-authored-by: Suresh Kumar Anaparti <sureshkumar.anaparti@gmail.com>
Description
This PR...
Fixes: #10906
By adding a global setting
allow.user.view.all.zonedepending on this setting the resource admin from the issue description will be able to see zones that do not fall withing their dedication domain, or not. The default istrue, meaning that onlistZonesall zones are shown. If set tofalsethe user will only see the zone from their dedication domain.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?