Skip to content

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

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

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

Conversation

@HawkForge

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.


Replaces #14428, which got closed when my fork was accidentally detached.

…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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant