Skip to content

Allow project admin to update template/ISO permissions in projects #14510

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.

Sign up for GitHub

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

Open
DaanHoogland wants to merge 1 commit into
base: 4.20
Choose a base branch
from
Open

Allow project admin to update template/ISO permissions in projects #14510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions server/src/main/java/com/cloud/template/TemplateManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@
import com.cloud.hypervisor.HypervisorGuruManager;
import com.cloud.projects.Project;
import com.cloud.projects.ProjectManager;
import com.cloud.projects.dao.ProjectAccountDao;
import com.cloud.storage.DataStoreRole;
import com.cloud.storage.GuestOSVO;
import com.cloud.storage.ImageStoreUploadMonitorImpl;
Expand Down Expand Up @@ -277,6 +278,8 @@ public class TemplateManagerImpl extends ManagerBase implements TemplateManager,
@Inject
private ProjectManager _projectMgr;
@Inject
private ProjectAccountDao _projectAccountDao;
@Inject
private VolumeDataFactory _volFactory;
@Inject
private TemplateDataFactory _tmplFactory;
Expand Down Expand Up @@ -1544,6 +1547,24 @@ public List<String> listTemplatePermissions(BaseListTemplateOrIsoPermissionsCmd
return accountNames;
}

/**
* Checks whether the caller is a Project Admin of the project that owns the template, resolving the project from the
* owner account itself rather than from the project of the API call.
*/
protected boolean isAdminOfOwningProject(Account caller, User user, Account owner) {
if (owner.getType() != Account.Type.PROJECT) {
return false;
}
Project project = _projectMgr.findByProjectAccountId(owner.getId());
if (project == null) {
return false;
}
if (_projectAccountDao.findByProjectIdUserId(project.getId(), caller.getId(), user.getId()) != null) {
return _projectAccountDao.canUserModifyProject(project.getId(), caller.getId(), user.getId());
}
return _projectAccountDao.canModifyProjectAccount(caller.getId(), owner.getId());
}

@DB
@Override
public boolean updateTemplateOrIsoPermissions(BaseUpdateTemplateOrIsoPermissionsCmd cmd) {
Expand Down Expand Up @@ -1627,6 +1648,7 @@ public boolean updateTemplateOrIsoPermissions(BaseUpdateTemplateOrIsoPermissions
// check configuration parameter(allow.public.user.templates) value for
// the template owner
boolean isAdmin = _accountMgr.isAdmin(caller.getId());
boolean isProjectTemplateOwnerAdmin = isAdminOfOwningProject(caller, user, owner);
boolean allowPublicUserTemplates = AllowPublicUserTemplates.valueIn(template.getAccountId());
if (!isAdmin && !allowPublicUserTemplates && isPublic != null && isPublic) {
throw new InvalidParameterValueException("Only private " + mediaType + "s can be created.");
Expand All @@ -1640,8 +1662,8 @@ public boolean updateTemplateOrIsoPermissions(BaseUpdateTemplateOrIsoPermissions
}
}

//Only admin or owner of the template should be able to change its permissions
if (caller.getId() != ownerId && !isAdmin) {
//Only admin, owner, or the project admin of a project-owned template should be able to change its permissions
if (caller.getId() != ownerId && !isAdmin && !isProjectTemplateOwnerAdmin) {
throw new InvalidParameterValueException("Unable to grant permission to Account " + caller.getAccountName() + " as it is neither admin nor owner or the Template");
}

Expand All @@ -1657,7 +1679,7 @@ public boolean updateTemplateOrIsoPermissions(BaseUpdateTemplateOrIsoPermissions

if (isExtractable != null) {
// Only Root admins and owners are allowed to change it for templates
if (!template.getFormat().equals(ImageFormat.ISO) && caller.getId() != ownerId && !isAdmin) {
if (!template.getFormat().equals(ImageFormat.ISO) && caller.getId() != ownerId && !isAdmin && !isProjectTemplateOwnerAdmin) {
throw new InvalidParameterValueException("Only ROOT admins and Template owners are allowed to modify isExtractable attribute.");
} else {
// For Isos normal user can change it, as their are no derivatives.
Expand Down
112 changes: 112 additions & 0 deletions server/src/test/java/com/cloud/template/TemplateManagerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,17 @@
package com.cloud.template;


import com.cloud.domain.DomainVO;
import com.cloud.domain.dao.DomainDao;
import com.cloud.exception.InvalidParameterValueException;
import com.cloud.exception.ResourceAllocationException;
import com.cloud.host.Status;
import com.cloud.hypervisor.Hypervisor;
import com.cloud.hypervisor.HypervisorGuruManager;
import com.cloud.projects.Project;
import com.cloud.projects.ProjectAccountVO;
import com.cloud.projects.ProjectManager;
import com.cloud.projects.dao.ProjectAccountDao;
import com.cloud.resourcelimit.CheckedReservation;
import com.cloud.storage.DataStoreRole;
import com.cloud.storage.GuestOSVO;
Expand Down Expand Up @@ -66,6 +72,7 @@
import org.apache.cloudstack.api.command.user.template.RegisterTemplateCmd;
import org.apache.cloudstack.api.command.user.template.RegisterVnfTemplateCmd;
import org.apache.cloudstack.api.command.user.template.UpdateTemplateCmd;
import org.apache.cloudstack.api.command.user.template.UpdateTemplatePermissionsCmd;
import org.apache.cloudstack.api.command.user.template.UpdateVnfTemplateCmd;
import org.apache.cloudstack.api.command.user.userdata.LinkUserDataToTemplateCmd;
import org.apache.cloudstack.context.CallContext;
Expand Down Expand Up @@ -177,6 +184,15 @@ public class TemplateManagerImplTest extends TestCase {
@Mock
AccountManager _accountMgr;

@Mock
ProjectManager _projectMgr;

@Mock
ProjectAccountDao _projectAccountDao;

@Mock
DomainDao _domainDao;

@Mock
VnfTemplateManager vnfTemplateManager;

Expand Down Expand Up @@ -763,6 +779,102 @@ public void verifyHeuristicRulesForZoneTestTemplateIsISOFormatShouldCheckForISOH
Mockito.verify(heuristicRuleHelperMock, Mockito.times(1)).getImageStoreIfThereIsHeuristicRule(1L, HeuristicType.ISO, vmTemplateVOMock);
}

private UpdateTemplatePermissionsCmd mockProjectTemplatePermissionsUpdate() {
UpdateTemplatePermissionsCmd cmd = mock(UpdateTemplatePermissionsCmd.class);
when(cmd.getId()).thenReturn(10L);
when(cmd.getAccountNames()).thenReturn(null);
when(cmd.getProjectIds()).thenReturn(null);
when(cmd.isFeatured()).thenReturn(null);
when(cmd.isPublic()).thenReturn(null);
when(cmd.isExtractable()).thenReturn(false);
when(cmd.getOperation()).thenReturn(null);

VMTemplateVO template = mock(VMTemplateVO.class);
when(template.getFormat()).thenReturn(Storage.ImageFormat.QCOW2);
when(template.getRemoved()).thenReturn(null);
when(template.getAccountId()).thenReturn(5L);
when(vmTemplateDao.findById(10L)).thenReturn(template);

Account projectOwnerAccount = mock(Account.class);
when(projectOwnerAccount.getId()).thenReturn(5L);
when(projectOwnerAccount.getType()).thenReturn(Account.Type.PROJECT);
when(_accountMgr.getAccount(5L)).thenReturn(projectOwnerAccount);

Project project = mock(Project.class);
when(project.getId()).thenReturn(7L);
when(_projectMgr.findByProjectAccountId(5L)).thenReturn(project);
return cmd;
}

@Test
public void testUpdateTemplatePermissionsAllowsProjectAdminToActAsOwner() {
UpdateTemplatePermissionsCmd cmd = mockProjectTemplatePermissionsUpdate();
VMTemplateVO template = vmTemplateDao.findById(10L);
when(template.getId()).thenReturn(10L);
when(vmTemplateDao.createForUpdate()).thenReturn(mock(VMTemplateVO.class));
Account projectOwnerAccount = _accountMgr.getAccount(5L);
when(projectOwnerAccount.getDomainId()).thenReturn(1L);
when(_domainDao.findById(1L)).thenReturn(mock(DomainVO.class));

// caller is a project member who is not the template's owner account (the project) and not a
// CloudStack admin, but is the Project Admin of the project that owns the template
when(_projectAccountDao.canModifyProjectAccount(anyLong(), eq(5L))).thenReturn(true);

boolean result = templateManager.updateTemplateOrIsoPermissions(cmd);

Assert.assertTrue("Project admin should be allowed to update permissions on a project-owned template", result);
}

@Test(expected = InvalidParameterValueException.class)
public void testUpdateTemplatePermissionsRejectsProjectMemberThatIsNotProjectAdmin() {
UpdateTemplatePermissionsCmd cmd = mockProjectTemplatePermissionsUpdate();
when(_projectAccountDao.canModifyProjectAccount(anyLong(), eq(5L))).thenReturn(false);

templateManager.updateTemplateOrIsoPermissions(cmd);
}

@Test
public void isAdminOfOwningProjectTestOwnerIsNotAProject() {
Account owner = mock(Account.class);
when(owner.getType()).thenReturn(Account.Type.NORMAL);

Assert.assertFalse(templateManager.isAdminOfOwningProject(mock(Account.class), mock(User.class), owner));
Mockito.verifyNoInteractions(_projectMgr, _projectAccountDao);
}

@Test
public void isAdminOfOwningProjectTestProjectNotFound() {
Account owner = mock(Account.class);
when(owner.getId()).thenReturn(5L);
when(owner.getType()).thenReturn(Account.Type.PROJECT);
when(_projectMgr.findByProjectAccountId(5L)).thenReturn(null);

Assert.assertFalse(templateManager.isAdminOfOwningProject(mock(Account.class), mock(User.class), owner));
Mockito.verifyNoInteractions(_projectAccountDao);
}

@Test
public void isAdminOfOwningProjectTestUsesUserRoleInOwningProject() {
Account caller = mock(Account.class);
when(caller.getId()).thenReturn(2L);
User user = mock(User.class);
when(user.getId()).thenReturn(3L);
Account owner = mock(Account.class);
when(owner.getType()).thenReturn(Account.Type.PROJECT);
when(owner.getId()).thenReturn(5L);
Project project = mock(Project.class);
when(project.getId()).thenReturn(7L);
when(_projectMgr.findByProjectAccountId(5L)).thenReturn(project);
when(_projectAccountDao.findByProjectIdUserId(7L, 2L, 3L)).thenReturn(mock(ProjectAccountVO.class));
when(_projectAccountDao.canUserModifyProject(7L, 2L, 3L)).thenReturn(false);

Assert.assertFalse(templateManager.isAdminOfOwningProject(caller, user, owner));
Mockito.verify(_projectAccountDao, Mockito.never()).canModifyProjectAccount(anyLong(), anyLong());

when(_projectAccountDao.canUserModifyProject(7L, 2L, 3L)).thenReturn(true);
Assert.assertTrue(templateManager.isAdminOfOwningProject(caller, user, owner));
}

@Test
public void verifyHeuristicRulesForZoneTestTemplateNotISOFormatShouldCheckForTemplateHeuristicType() {
VMTemplateVO vmTemplateVOMock = Mockito.mock(VMTemplateVO.class);
Expand Down
Loading