Skip to content

server: allow project admins to update permissions of project templates - #14428

Open
HawkForge wants to merge 1 commit into
apache:4.20from
HawkForge:fix/project-admin-template-permissions
Open

server: allow project admins to update permissions of project templates#14428
HawkForge wants to merge 1 commit into
apache:4.20from
HawkForge:fix/project-admin-template-permissions

Conversation

@HawkForge

@HawkForge HawkForge commented Oct 16, 2026

Copy link
Copy Markdown

Description

This PR fixes project admins being unable to change ispublic, isextractable or isfeatured on a template or ISO owned by their project through updateTemplatePermissions / updateIsoPermissions. Thanks to @prashanthr2 for the report and analysis.

Root cause: TemplateManagerImpl.updateTemplateOrIsoPermissions() only allows the change if caller.getId() == ownerId or the caller is an admin. For a project owned template the owner is the project account, so no user account ever matches and the call fails with "Unable to grant permission to Account X as it is neither admin nor owner or the Template", while the same flags can be set when registering the template into the project.

This is an authorization change, limited to members with the project Admin role. The caller is treated as the owner only when the template is owned by a project account and ProjectManager.canModifyProjectAccount(caller, ownerId) returns true. That is the helper DomainChecker already uses for ModifyProject access; for regular users it only returns true for accounts/users with the Admin role in the project. Plain project members are still refused. The same owner flag is used for the existing isextractable owner check. Behaviour for template owners, root/domain admins and non-project templates is unchanged; the project check is only evaluated for non-admin, non-owner callers of project owned templates.

Sent against 4.20 so it can be merged forward; the same code is present on 4.22 and main.

Fixes: #14174

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

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

How Has This Been Tested?

Added unit tests to TemplateManagerImplTest for updateTemplateOrIsoPermissions():

  • project admin on a project owned template succeeds (fails without the fix with the error from the issue)
  • project member who cannot modify the project account is still rejected
  • owner of a normal template succeeds, project check not consulted
  • admin on a project owned template succeeds, project check not consulted
  • non-owner, non-admin on a normal template is still rejected

Ran:

  • mvn -pl server test -Dtest=TemplateManagerImplTest: 42 tests (37 existing + 5 new), 0 failures. With the fix reverted, updateTemplateOrIsoPermissionsTestProjectAdminCanUpdateProjectTemplate fails with the InvalidParameterValueException from the issue.
  • mvn -pl server test -Dtest='com.cloud.template.*Test,com.cloud.projects.*Test' with checkstyle enabled: TemplateManagerImplTest 42, HypervisorTemplateAdapterTest 17, ProjectManagerImplTest 6; 65 tests in total, 0 failures, checkstyle passed.

The full server test suite and integration/Marvin tests were not run.

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

Checked that project members without the Admin role and unrelated users are still rejected, and that the existing owner and admin paths do not call the project check.

@HawkForge
server: allow project admins to update permissions of project templat…
e7a4542 
…es (apache#14174)

updateTemplateOrIsoPermissions() only treated the caller as owner when the
caller's account id matched the template owner. For project owned
templates/ISOs the owner is the project account, so project admins were
rejected even though they can set the same flags at registration. Treat
callers that can modify the project account (project admins) as owners.

Fixes: apache#14174
@boring-cyborg

boring-cyborg Bot commented Oct 16, 2026

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

@wido

wido commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

Thank you for your first PR! Can you explain why this is a bug, isn’t this intended by design and this a change on how it works?

@HawkForge

HawkForge commented Oct 16, 2026

Copy link
Copy Markdown
Author

Thanks for looking at it. I think it's a bug rather than intended behaviour, for three reasons:

  1. Registration and update disagree. When a template is registered into a project, TemplateAdapterBase.prepare() lets the caller set ispublic / isextractable as long as allow.public.user.templates allows it for the template owner (the project account). There is no project-role check there. After registration, updateTemplateOrIsoPermissions() refuses the same flags to everyone except root/domain admins. So nobody in the project can change them.
  2. The project can't undo its own choice. A template that was registered as public can't be made private again by the project's own admin. Only an operator can. That seems unlikely to be the intended permission model.
  3. The method already expects project users to get past this point. It has a project-specific branch, if (owner.getType() == Account.Type.PROJECT) { // if it is a project owned template/iso, the user must at least have access to be allowed to share it. _accountMgr.checkAccess(user, template); }. But the owner check that follows compares caller.getId() with the template's owner, which is the project account. No user's own account is ever the project account, so for non-admin callers that branch can never lead to success.

The change keeps this narrow. It only treats a caller as owner when the template is project-owned and ProjectManager.canModifyProjectAccount() says they may modify that project account, which for regular users means the project's Admin role. Plain project members are still refused (there is a test for that), and nothing changes for templates owned by normal accounts.

If you'd rather keep this admin-only by design, I'm happy to close this. It would probably be worth making registration consistent with that, though.

wido
wido reviewed Oct 16, 2026
View reviewed changes

@wido wido left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that answers my question. I went through the code myself and had Claude Fable double check the reasoning afterwards.

Registration into a project is open to every member, public is only gated by allow.public.user.templates and extractable has no check at all. The update path then rejects everyone in the project because the owner is the project account. So this is an inconsistency, not a design choice. Limiting the fix to the project Admin role is stricter than registration, which is fine with me.

Verified on the 4.20 branch: TemplateManagerImplTest passes (42 tests), and with the fix reverted the project admin test fails with the error from the issue.

One pre-existing thing, not for this PR: the update path lets any owner set isfeatured, while registration forces it to false for non-admins. Could be a small follow-up.

LGTM, CI still has to run.

wido
wido approved these changes Oct 16, 2026
View reviewed changes

@wido wido left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to merge to me!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wido wido wido approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@HawkForge @wido