Remove unnecessary if-else branch in template permission validation

[sudo87]

Description

Permission access check looks incorrect for non-public template and non admin user.

Provided non-public templates are not shareable, check must ensure that template is owned by user. For public, ownership flag is passed as false, which makes sense as template can be created by other user.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)
• Build/CI
• Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

[codecov]

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 18.00%. Comparing base (30e6c22) to head (fe7fc5a).
⚠️ Report is 65 commits behind head on main.

Files with missing lines	Patch %	Lines
...ain/java/com/cloud/api/query/QueryManagerImpl.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #12683 +/- ##
============================================
+ Coverage 17.92% 18.00% +0.08% - Complexity 16154 16465 +311 
============================================
Files 5939 5977 +38 Lines 533181 537728 +4547 Branches 65237 66025 +788 ============================================
+ Hits 95585 96840 +1255 - Misses 426856 429968 +3112 - Partials 10740 10920 +180 

Flag	Coverage Δ
uitests	3.52% <ø> (-0.15%)	⬇️
unittests	19.17% <0.00%> (+0.13%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[sudo87]

@blueorangutan package

[blueorangutan]

@sudo87 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16937

[Copilot]

Pull request overview

This PR fixes an access control issue for non-public templates accessed by non-admin users. The fix ensures that ownership semantics are properly enforced by changing the sameOwner parameter from false to true in the access check for non-public templates.

Changes:

• Fixed access control check for non-public templates to enforce ownership semantics by setting sameOwner=true instead of false
• Minor code formatting change (consolidating else-if statement onto one line)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[winterhazel]

@sudo87 is this change related to a specific issue/situation? This flag is just used to ensure that all the controlled entities provided to checkAccess are owned by the same account. As only a single template is provided, changing it should not affect anything.

[sudo87]

@sudo87 is this change related to a specific issue/situation? This flag is just used to ensure that all the controlled entities provided to checkAccess are owned by the same account. As only a single template is provided, changing it should not affect anything.

Hi @winterhazel, this change is based on how checkAccess is used here. Right now we pass sameOwner = false, which effectively relaxes the ownership check.
For a non-public template accessed by a non-admin, we should be enforcing that it belongs to the caller’s account. Setting sameOwner = true makes that explicit and aligns with the intended access control.

Please let me know if change makes sense.

[winterhazel]

@sudo87 is this change related to a specific issue/situation? This flag is just used to ensure that all the controlled entities provided to checkAccess are owned by the same account. As only a single template is provided, changing it should not affect anything.

Hi @winterhazel, this change is based on how checkAccess is used here. Right now we pass sameOwner = false, which effectively relaxes the ownership check. For a non-public template accessed by a non-admin, we should be enforcing that it belongs to the caller’s account. Setting sameOwner = true makes that explicit and aligns with the intended access control.

Please let me know if change makes sense.

@sudo87 have a look at what sameOwner is used for in the implementation of com.cloud.user.AccountManagerImpl#checkAccess(com.cloud.user.Account, org.apache.cloudstack.acl.SecurityChecker.AccessType, boolean, org.apache.cloudstack.acl.ControlledEntity...). It does not ensure that the controlled entity (the template) belongs to the calling account.

[sudo87]

Thanks @winterhazel for the hint, I misunderstood the flow.

Since both branches were effectively doing the same checkAccess call, I have consolidated them. Please review if changes are good.

[winterhazel]

LGTM, just need to change the PR's title

[sudo87]

@blueorangutan package

[DaanHoogland]

clgtm

[blueorangutan]

@sudo87 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

[sureshanaparti]

clgtm

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17731

[sudo87]

@blueorangutan test

[blueorangutan]

@sudo87 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[blueorangutan]

[SF] Trillian test result (tid-16021)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 48802 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr12683-t16021-kvm-ol8.zip
Smoke tests completed. 151 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

[winterhazel]

As this patch does not change the behavior of the code, I'm merging it already.