From 6dc851a549c2cffeaad0e87986a77d7379872efc Mon Sep 17 00:00:00 2001 From: Lopez Date: Mon, 9 Jan 2023 17:18:05 -0300 Subject: [PATCH 1/6] allow filtering of listDiskOffering and listServiceOffering APIs by account or project --- .../user/offering/ListDiskOfferingsCmd.java | 15 ++ .../offering/ListServiceOfferingsCmd.java | 15 ++ .../com/cloud/api/query/QueryManagerImpl.java | 77 +++++++++- .../cloud/api/query/QueryManagerImplTest.java | 144 ++++++++++++++++-- 4 files changed, 234 insertions(+), 17 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListDiskOfferingsCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListDiskOfferingsCmd.java index 5fa24ec16301..e2d28c0937dc 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListDiskOfferingsCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListDiskOfferingsCmd.java @@ -17,6 +17,7 @@ package org.apache.cloudstack.api.command.user.offering; import org.apache.cloudstack.api.response.StoragePoolResponse; +import org.apache.cloudstack.api.response.ProjectResponse; import org.apache.cloudstack.api.response.VolumeResponse; import org.apache.cloudstack.api.response.ZoneResponse; import org.apache.log4j.Logger; @@ -44,6 +45,12 @@ public class ListDiskOfferingsCmd extends BaseListDomainResourcesCmd { @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, description = "name of the disk offering") private String diskOfferingName; + @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "list resources by account. Must be used with the domainId parameter.") + private String accountName; + + @Parameter(name = ApiConstants.PROJECT_ID, type = CommandType.UUID, entityType = ProjectResponse.class, description = "list objects by project.") + private Long projectId; + @Parameter(name = ApiConstants.ZONE_ID, type = CommandType.UUID, entityType = ZoneResponse.class, @@ -84,6 +91,14 @@ public Long getVolumeId() { public Boolean getEncrypt() { return encrypt; } + public String getAccountName() { + return accountName; + } + + public Long getProjectId() { + return projectId; + } + ///////////////////////////////////////////////////// /////////////// API Implementation/////////////////// ///////////////////////////////////////////////////// diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java index 3208ef58a4fa..098cad5e4692 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java @@ -16,6 +16,7 @@ // under the License. package org.apache.cloudstack.api.command.user.offering; +import org.apache.cloudstack.api.response.ProjectResponse; import org.apache.cloudstack.api.response.ZoneResponse; import org.apache.log4j.Logger; @@ -82,6 +83,12 @@ public class ListServiceOfferingsCmd extends BaseListDomainResourcesCmd { since = "4.15") private Integer cpuSpeed; + @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "list resources by account. Must be used with the domainId parameter.") + private String accountName; + + @Parameter(name = ApiConstants.PROJECT_ID, type = CommandType.UUID, entityType = ProjectResponse.class, description = "list objects by project.") + private Long projectId; + @Parameter(name = ApiConstants.ENCRYPT_ROOT, type = CommandType.BOOLEAN, description = "listed offerings support root disk encryption", @@ -130,6 +137,14 @@ public Integer getCpuSpeed() { public Boolean getEncryptRoot() { return encryptRoot; } + public String getAccountName() { + return accountName; + } + + public Long getProjectId() { + return projectId; + } + ///////////////////////////////////////////////////// /////////////// API Implementation/////////////////// ///////////////////////////////////////////////////// diff --git a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java index 827a41eeb02f..196f9e10ee76 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -2969,6 +2969,8 @@ private Pair, Integer> searchForDiskOfferingsInternal(L Object id = cmd.getId(); Object keyword = cmd.getKeyword(); Long domainId = cmd.getDomainId(); + Long projectId = cmd.getProjectId(); + String accountName = cmd.getAccountName(); Boolean isRootAdmin = _accountMgr.isRootAdmin(account.getAccountId()); Boolean isRecursive = cmd.isRecursive(); Long zoneId = cmd.getZoneId(); @@ -2978,7 +2980,7 @@ private Pair, Integer> searchForDiskOfferingsInternal(L // Keeping this logic consistent with domain specific zones // if a domainId is provided, we just return the disk offering // associated with this domain - if (domainId != null) { + if (domainId != null && accountName == null) { if (_accountMgr.isRootAdmin(account.getId()) || isPermissible(account.getDomainId(), domainId)) { // check if the user's domain == do's domain || user's domain is // a child of so's domain for non-root users @@ -3055,9 +3057,9 @@ private Pair, Integer> searchForDiskOfferingsInternal(L // Filter offerings that are not associated with caller's domain // Fetch the offering ids from the details table since theres no smart way to filter them in the join ... yet! - Account caller = CallContext.current().getCallingAccount(); - if (caller.getType() != Account.Type.ADMIN) { - Domain callerDomain = _domainDao.findById(caller.getDomainId()); + account = getCallerAccordingToProjectIdAndAccountNameAndDomainId(account, projectId, accountName, domainId); + if (!Account.Type.ADMIN.equals(account.getType())) { + Domain callerDomain = _domainDao.findById(account.getDomainId()); List domainIds = findRelatedDomainIds(callerDomain, isRecursive); List ids = _diskOfferingDetailsDao.findOfferingIdsByDomainIds(domainIds); @@ -3136,6 +3138,8 @@ private Pair, Integer> searchForServiceOfferingsInte searchFilter.addOrderBy(ServiceOfferingJoinVO.class, "id", true); Account caller = CallContext.current().getCallingAccount(); + Long projectId = cmd.getProjectId(); + String accountName = cmd.getAccountName(); Object name = cmd.getServiceOfferingName(); Object id = cmd.getId(); Object keyword = cmd.getKeyword(); @@ -3151,9 +3155,11 @@ private Pair, Integer> searchForServiceOfferingsInte Integer cpuSpeed = cmd.getCpuSpeed(); Boolean encryptRoot = cmd.getEncryptRoot(); + caller = getCallerAccordingToProjectIdAndAccountNameAndDomainId(caller, projectId, accountName, domainId); + SearchCriteria sc = _srvOfferingJoinDao.createSearchCriteria(); if (!_accountMgr.isRootAdmin(caller.getId()) && isSystem) { - throw new InvalidParameterValueException("Only ROOT admins can access system's offering"); + throw new InvalidParameterValueException("Only ROOT admins can access system offerings."); } // Keeping this logic consistent with domain specific zones @@ -3236,9 +3242,9 @@ private Pair, Integer> searchForServiceOfferingsInte } else { // for root users if (caller.getDomainId() != 1 && isSystem) { // NON ROOT admin - throw new InvalidParameterValueException("Non ROOT admins cannot access system's offering"); + throw new InvalidParameterValueException("Non ROOT admins cannot access system's offering."); } - if (domainId != null) { + if (domainId != null && accountName == null) { sc.addAnd("domainId", Op.FIND_IN_SET, String.valueOf(domainId)); } } @@ -3378,6 +3384,63 @@ private Pair, Integer> searchForServiceOfferingsInte return _srvOfferingJoinDao.searchAndCount(sc, searchFilter); } + /** + * Retrieves the API caller. If the projectId or accountName parameters were provided in the API call, consider the account or project as the API caller. + * With that, the APIs return will be filtered by the account defined in the API call. + * @param originalCaller Account that made the API call + * @param projectId projectId provided in API call + * @param accountName accountName provided in API call + * @param domainId domainId provided in API call + * @return Account object + */ + protected Account getCallerAccordingToProjectIdAndAccountNameAndDomainId(Account originalCaller, Long projectId, String accountName, Long domainId) { + if (accountName != null && domainId != null) { + return getCallerAccordingToAccountNameAndDomainId(originalCaller, accountName, domainId); + } + + if (projectId != null ) { + return getCallerAccordingToProjectId(originalCaller, projectId); + } + return originalCaller; + } + + /** + * Retrieves the API caller. If the projectId parameter were provided in the API call, consider the account as the API caller. + * @param originalCaller + * @param projectId + * @return Account object + */ + protected Account getCallerAccordingToProjectId(Account originalCaller, Long projectId) { + Project project = _projectMgr.getProject(projectId); + if (project == null ) { + throw new InvalidParameterValueException("Unable to find project by specified id"); + } + Account owner = _accountMgr.getActiveAccountById(project.getProjectAccountId()); + if (owner == null) { + throw new InvalidParameterValueException("Unable to find account for the specified project id"); + } + if (!_projectMgr.canAccessProjectAccount(originalCaller, owner.getId())) { + throw new InvalidParameterValueException(String.format("Account [%s] cannot access specified project id [%s]", originalCaller.getUuid(), owner.getUuid())); + } + _accountMgr.checkAccess(originalCaller, null, true, owner); + return owner; + } + + /** + * Retrieves the API caller. If the accountName and domainId parameters were provided in the API call, consider the account as the API caller. + * @param originalCaller + * @param accountName + * @return Account object + */ + protected Account getCallerAccordingToAccountNameAndDomainId(Account originalCaller, String accountName, Long domainId) { + Account owner = _accountMgr.getActiveAccountByName(accountName, domainId); + if (owner == null) { + throw new InvalidParameterValueException(String.format("Unable to find account [%s] in specified domain id [%s]", accountName, domainId)); + } + _accountMgr.checkAccess(originalCaller, null, true, owner); + return owner; + } + @Override public ListResponse listDataCenters(ListZonesCmd cmd) { Pair, Integer> result = listDataCentersInternal(cmd); diff --git a/server/src/test/java/com/cloud/api/query/QueryManagerImplTest.java b/server/src/test/java/com/cloud/api/query/QueryManagerImplTest.java index e0093743845c..e3faf5e3a8f8 100644 --- a/server/src/test/java/com/cloud/api/query/QueryManagerImplTest.java +++ b/server/src/test/java/com/cloud/api/query/QueryManagerImplTest.java @@ -17,25 +17,27 @@ package com.cloud.api.query; +import static org.junit.Assert.assertEquals; import static org.mockito.Mockito.when; import java.util.ArrayList; import java.util.List; import java.util.UUID; +import com.cloud.projects.ProjectManager; import org.apache.cloudstack.acl.SecurityChecker; import org.apache.cloudstack.api.ApiCommandResourceType; import org.apache.cloudstack.api.command.user.event.ListEventsCmd; import org.apache.cloudstack.api.response.EventResponse; import org.apache.cloudstack.api.response.ListResponse; import org.apache.cloudstack.context.CallContext; -import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.Mockito; +import org.mockito.Spy; import org.powermock.api.mockito.PowerMockito; import org.powermock.core.classloader.annotations.PrepareForTest; import org.powermock.modules.junit4.PowerMockRunner; @@ -65,14 +67,25 @@ public class QueryManagerImplTest { public static final long USER_ID = 1; public static final long ACCOUNT_ID = 1; + private long projId = 1l; + private String accountName = "name"; + private long domId = 1l; + @Spy + @InjectMocks + private QueryManagerImpl queryManagerImplSpy = new QueryManagerImpl(); @Mock EntityManager entityManager; @Mock - AccountManager accountManager; + AccountManager accountManagerMock; + @Mock + ProjectManager projectManagerMock; @Mock EventJoinDao eventJoinDao; - + @Mock + Account accountMock; + @Mock + Project projectMock; private AccountVO account; private UserVO user; @@ -90,12 +103,12 @@ private void setupCommonMocks() { user = new UserVO(1, "testuser", "password", "firstname", "lastName", "email", "timezone", UUID.randomUUID().toString(), User.Source.UNKNOWN); CallContext.register(user, account); - Mockito.when(accountManager.isRootAdmin(account.getId())).thenReturn(false); - Mockito.doNothing().when(accountManager).buildACLSearchParameters(Mockito.any(Account.class), Mockito.anyLong(), Mockito.anyString(), Mockito.anyLong(), Mockito.anyList(), + Mockito.when(accountManagerMock.isRootAdmin(account.getId())).thenReturn(false); + Mockito.doNothing().when(accountManagerMock).buildACLSearchParameters(Mockito.any(Account.class), Mockito.anyLong(), Mockito.anyString(), Mockito.anyLong(), Mockito.anyList(), Mockito.any(), Mockito.anyBoolean(), Mockito.anyBoolean()); - Mockito.doNothing().when(accountManager).buildACLSearchBuilder(Mockito.any(SearchBuilder.class), Mockito.anyLong(), Mockito.anyBoolean(), Mockito.anyList(), + Mockito.doNothing().when(accountManagerMock).buildACLSearchBuilder(Mockito.any(SearchBuilder.class), Mockito.anyLong(), Mockito.anyBoolean(), Mockito.anyList(), Mockito.any(Project.ListProjectResourcesCriteria.class)); - Mockito.doNothing().when(accountManager).buildACLViewSearchCriteria(Mockito.any(), Mockito.anyLong(), Mockito.anyBoolean(), Mockito.anyList(), + Mockito.doNothing().when(accountManagerMock).buildACLViewSearchCriteria(Mockito.any(), Mockito.anyLong(), Mockito.anyBoolean(), Mockito.anyList(), Mockito.any(Project.ListProjectResourcesCriteria.class)); final SearchBuilder searchBuilder = Mockito.mock(SearchBuilder.class); final SearchCriteria searchCriteria = Mockito.mock(SearchCriteria.class); @@ -128,7 +141,7 @@ public void searchForEventsSuccess() { Mockito.when(network.getId()).thenReturn(1L); Mockito.when(network.getAccountId()).thenReturn(account.getId()); Mockito.when(entityManager.findByUuidIncludingRemoved(Network.class, uuid)).thenReturn(network); - Mockito.doNothing().when(accountManager).checkAccess(account, SecurityChecker.AccessType.ListEntry, true, network); + Mockito.doNothing().when(accountManagerMock).checkAccess(account, SecurityChecker.AccessType.ListEntry, true, network); Mockito.when(eventJoinDao.searchAndCount(Mockito.any(), Mockito.any(Filter.class))).thenReturn(pair); List respList = new ArrayList(); for (EventJoinVO vt : events) { @@ -137,7 +150,7 @@ public void searchForEventsSuccess() { PowerMockito.mockStatic(ViewResponseHelper.class); Mockito.when(ViewResponseHelper.createEventResponse(Mockito.any())).thenReturn(respList); ListResponse result = queryManager.searchForEvents(cmd); - Assert.assertEquals((int) result.getCount(), events.size()); + assertEquals((int) result.getCount(), events.size()); } @Test(expected = InvalidParameterValueException.class) @@ -184,7 +197,118 @@ public void searchForEventsFailPermissionDenied() { Mockito.when(network.getId()).thenReturn(1L); Mockito.when(network.getAccountId()).thenReturn(2L); Mockito.when(entityManager.findByUuidIncludingRemoved(Network.class, uuid)).thenReturn(network); - Mockito.doThrow(new PermissionDeniedException("Denied")).when(accountManager).checkAccess(account, SecurityChecker.AccessType.ListEntry, false, network); + Mockito.doThrow(new PermissionDeniedException("Denied")).when(accountManagerMock).checkAccess(account, SecurityChecker.AccessType.ListEntry, false, network); queryManager.searchForEvents(cmd); } + + @Test + public void getCallerAccordingToProjectIdAndAccountNameAndDomainIdTestReturnOriginalCaller() { + Account result = queryManagerImplSpy.getCallerAccordingToProjectIdAndAccountNameAndDomainId(accountMock, null, null, null); + Mockito.verify(queryManagerImplSpy, Mockito.times(0)).getCallerAccordingToAccountNameAndDomainId(Mockito.any(Account.class), Mockito.anyString(), Mockito.anyLong()); + Mockito.verify(queryManagerImplSpy, Mockito.times(0)).getCallerAccordingToProjectId(Mockito.any(Account.class), Mockito.anyLong()); + assertEquals(result, accountMock); + } + + @Test + public void getCallerAccordingToProjectIdAndAccountNameAndDomainIdTestProjectIdNull() { + Account anotherAccount = Mockito.mock(Account.class); + Mockito.doReturn(anotherAccount).when(queryManagerImplSpy).getCallerAccordingToAccountNameAndDomainId(Mockito.any(Account.class), Mockito.anyString(), Mockito.anyLong()); + Account result = queryManagerImplSpy.getCallerAccordingToProjectIdAndAccountNameAndDomainId(accountMock, null, accountName, domId); + Mockito.verify(queryManagerImplSpy, Mockito.times(1)).getCallerAccordingToAccountNameAndDomainId(Mockito.any(Account.class), Mockito.anyString(), Mockito.anyLong()); + Mockito.verify(queryManagerImplSpy, Mockito.times(0)).getCallerAccordingToProjectId(Mockito.any(Account.class), Mockito.anyLong()); + assertEquals(result, anotherAccount); + } + + @Test + public void getCallerAccordingToProjectIdAndAccountNameAndDomainIdTestAccountNameAndDomainIdNull() { + Account projectAccount = Mockito.mock(Account.class); + Mockito.doReturn(projectAccount).when(queryManagerImplSpy).getCallerAccordingToProjectId(Mockito.any(Account.class), Mockito.anyLong()); + Account result = queryManagerImplSpy.getCallerAccordingToProjectIdAndAccountNameAndDomainId(accountMock, projId, null, null); + Mockito.verify(queryManagerImplSpy, Mockito.times(0)).getCallerAccordingToAccountNameAndDomainId(Mockito.any(Account.class), Mockito.anyString(), Mockito.anyLong()); + Mockito.verify(queryManagerImplSpy, Mockito.times(1)).getCallerAccordingToProjectId(Mockito.any(Account.class), Mockito.anyLong()); + assertEquals(result, projectAccount); + } + + @Test + public void getCallerAccordingToProjectIdAndAccountNameAndDomainIdTestAccountNameNull() { + Account projectAccount = Mockito.mock(Account.class); + Mockito.doReturn(projectAccount).when(queryManagerImplSpy).getCallerAccordingToProjectId(Mockito.any(Account.class), Mockito.anyLong()); + Account result = queryManagerImplSpy.getCallerAccordingToProjectIdAndAccountNameAndDomainId(accountMock, projId, null, domId); + Mockito.verify(queryManagerImplSpy, Mockito.times(0)).getCallerAccordingToAccountNameAndDomainId(Mockito.any(Account.class), Mockito.anyString(), Mockito.anyLong()); + Mockito.verify(queryManagerImplSpy, Mockito.times(1)).getCallerAccordingToProjectId(Mockito.any(Account.class), Mockito.anyLong()); + assertEquals(result, projectAccount); + } + + @Test + public void getCallerAccordingToProjectIdAndAccountNameAndDomainIdTestDomainIdNull() { + Account projectAccount = Mockito.mock(Account.class); + Mockito.doReturn(projectAccount).when(queryManagerImplSpy).getCallerAccordingToProjectId(Mockito.any(Account.class), Mockito.anyLong()); + Account result = queryManagerImplSpy.getCallerAccordingToProjectIdAndAccountNameAndDomainId(accountMock, projId, accountName, null); + Mockito.verify(queryManagerImplSpy, Mockito.times(0)).getCallerAccordingToAccountNameAndDomainId(Mockito.any(Account.class), Mockito.anyString(), Mockito.anyLong()); + Mockito.verify(queryManagerImplSpy, Mockito.times(1)).getCallerAccordingToProjectId(Mockito.any(Account.class), Mockito.anyLong()); + assertEquals(result, projectAccount); + } + + @Test(expected = InvalidParameterValueException.class) + public void getCallerAccordingToAccountNameAndDomainIdTestThrowInvalidParameterValueException() { + Mockito.doReturn(null).when(accountManagerMock).getActiveAccountByName(Mockito.anyString(), Mockito.anyLong()); + queryManagerImplSpy.getCallerAccordingToAccountNameAndDomainId(accountMock, accountName, domId); + } + + @Test(expected = PermissionDeniedException.class) + public void getCallerAccordingToAccountNameAndDomainIdTestThrowPermissionDeniedException() { + Account anotherAccount = Mockito.mock(Account.class); + Mockito.doReturn(anotherAccount).when(accountManagerMock).getActiveAccountByName(Mockito.anyString(), Mockito.anyLong()); + Mockito.doThrow(PermissionDeniedException.class).when(accountManagerMock).checkAccess(accountMock, null, true, anotherAccount); + queryManagerImplSpy.getCallerAccordingToAccountNameAndDomainId(accountMock, accountName, domId); + } + + public void getCallerAccordingToAccountNameAndDomainIdTestReturnAccount() { + Account anotherAccount = Mockito.mock(Account.class); + Mockito.doReturn(anotherAccount).when(accountManagerMock).getActiveAccountByName(Mockito.anyString(), Mockito.anyLong()); + Account result = queryManagerImplSpy.getCallerAccordingToAccountNameAndDomainId(accountMock, accountName, domId); + assertEquals(result, anotherAccount); + } + + @Test + public void getCallerAccordingToProjectIdTestReturnProjectAccount() { + Account anotherAccount = Mockito.mock(Account.class); + Mockito.doReturn(projectMock).when(projectManagerMock).getProject(projId); + Mockito.doReturn(anotherAccount).when(accountManagerMock).getActiveAccountById(Mockito.anyLong()); + Mockito.doReturn(true).when(projectManagerMock).canAccessProjectAccount(Mockito.any(Account.class), Mockito.anyLong()); + Account result = queryManagerImplSpy.getCallerAccordingToProjectId(accountMock, projId); + assertEquals(result, anotherAccount); + } + + @Test(expected = InvalidParameterValueException.class) + public void getCallerAccordingToProjectIdTestProjectNotFound() { + Mockito.doReturn(null).when(projectManagerMock).getProject(projId); + queryManagerImplSpy.getCallerAccordingToProjectId(accountMock, projId); + } + + @Test(expected = InvalidParameterValueException.class) + public void getCallerAccordingToProjectIdTestAccountNotFound() { + Mockito.doReturn(projectMock).when(projectManagerMock).getProject(projId); + Mockito.doReturn(null).when(accountManagerMock).getActiveAccountById(Mockito.anyLong()); + queryManagerImplSpy.getCallerAccordingToProjectId(accountMock, projId); + } + + @Test(expected = InvalidParameterValueException.class) + public void getCallerAccordingToProjectIdTestAccountCanAccessProject() { + Account anotherAccount = Mockito.mock(Account.class); + Mockito.doReturn(projectMock).when(projectManagerMock).getProject(projId); + Mockito.doReturn(anotherAccount).when(accountManagerMock).getActiveAccountById(Mockito.anyLong()); + Mockito.doReturn(false).when(projectManagerMock).canAccessProjectAccount(Mockito.any(Account.class), Mockito.anyLong()); + queryManagerImplSpy.getCallerAccordingToProjectId(accountMock, projId); + } + + @Test(expected = PermissionDeniedException.class) + public void getCallerAccordingToProjectIdTestCheckAccess() { + Account anotherAccount = Mockito.mock(Account.class); + Mockito.doReturn(projectMock).when(projectManagerMock).getProject(projId); + Mockito.doReturn(anotherAccount).when(accountManagerMock).getActiveAccountById(Mockito.anyLong()); + Mockito.doReturn(true).when(projectManagerMock).canAccessProjectAccount(Mockito.any(Account.class), Mockito.anyLong()); + Mockito.doThrow(PermissionDeniedException.class).when(accountManagerMock).checkAccess(accountMock, null, true, anotherAccount); + queryManagerImplSpy.getCallerAccordingToProjectId(accountMock, projId); + } } \ No newline at end of file From 90a416ddb9ac9f02a989b6122ae10b879dc24866 Mon Sep 17 00:00:00 2001 From: Lopez Date: Wed, 18 Jan 2023 17:13:33 -0300 Subject: [PATCH 2/6] address Wei review --- .../com/cloud/api/query/QueryManagerImpl.java | 62 +------- .../cloud/api/query/QueryManagerImplTest.java | 144 ++---------------- 2 files changed, 12 insertions(+), 194 deletions(-) diff --git a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java index 196f9e10ee76..3d3a2235541a 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -3057,7 +3057,7 @@ private Pair, Integer> searchForDiskOfferingsInternal(L // Filter offerings that are not associated with caller's domain // Fetch the offering ids from the details table since theres no smart way to filter them in the join ... yet! - account = getCallerAccordingToProjectIdAndAccountNameAndDomainId(account, projectId, accountName, domainId); + account = _accountMgr.finalizeOwner(account, accountName, domainId, projectId); if (!Account.Type.ADMIN.equals(account.getType())) { Domain callerDomain = _domainDao.findById(account.getDomainId()); List domainIds = findRelatedDomainIds(callerDomain, isRecursive); @@ -3155,8 +3155,7 @@ private Pair, Integer> searchForServiceOfferingsInte Integer cpuSpeed = cmd.getCpuSpeed(); Boolean encryptRoot = cmd.getEncryptRoot(); - caller = getCallerAccordingToProjectIdAndAccountNameAndDomainId(caller, projectId, accountName, domainId); - + caller = _accountMgr.finalizeOwner(caller, accountName, domainId, projectId); SearchCriteria sc = _srvOfferingJoinDao.createSearchCriteria(); if (!_accountMgr.isRootAdmin(caller.getId()) && isSystem) { throw new InvalidParameterValueException("Only ROOT admins can access system offerings."); @@ -3384,63 +3383,6 @@ private Pair, Integer> searchForServiceOfferingsInte return _srvOfferingJoinDao.searchAndCount(sc, searchFilter); } - /** - * Retrieves the API caller. If the projectId or accountName parameters were provided in the API call, consider the account or project as the API caller. - * With that, the APIs return will be filtered by the account defined in the API call. - * @param originalCaller Account that made the API call - * @param projectId projectId provided in API call - * @param accountName accountName provided in API call - * @param domainId domainId provided in API call - * @return Account object - */ - protected Account getCallerAccordingToProjectIdAndAccountNameAndDomainId(Account originalCaller, Long projectId, String accountName, Long domainId) { - if (accountName != null && domainId != null) { - return getCallerAccordingToAccountNameAndDomainId(originalCaller, accountName, domainId); - } - - if (projectId != null ) { - return getCallerAccordingToProjectId(originalCaller, projectId); - } - return originalCaller; - } - - /** - * Retrieves the API caller. If the projectId parameter were provided in the API call, consider the account as the API caller. - * @param originalCaller - * @param projectId - * @return Account object - */ - protected Account getCallerAccordingToProjectId(Account originalCaller, Long projectId) { - Project project = _projectMgr.getProject(projectId); - if (project == null ) { - throw new InvalidParameterValueException("Unable to find project by specified id"); - } - Account owner = _accountMgr.getActiveAccountById(project.getProjectAccountId()); - if (owner == null) { - throw new InvalidParameterValueException("Unable to find account for the specified project id"); - } - if (!_projectMgr.canAccessProjectAccount(originalCaller, owner.getId())) { - throw new InvalidParameterValueException(String.format("Account [%s] cannot access specified project id [%s]", originalCaller.getUuid(), owner.getUuid())); - } - _accountMgr.checkAccess(originalCaller, null, true, owner); - return owner; - } - - /** - * Retrieves the API caller. If the accountName and domainId parameters were provided in the API call, consider the account as the API caller. - * @param originalCaller - * @param accountName - * @return Account object - */ - protected Account getCallerAccordingToAccountNameAndDomainId(Account originalCaller, String accountName, Long domainId) { - Account owner = _accountMgr.getActiveAccountByName(accountName, domainId); - if (owner == null) { - throw new InvalidParameterValueException(String.format("Unable to find account [%s] in specified domain id [%s]", accountName, domainId)); - } - _accountMgr.checkAccess(originalCaller, null, true, owner); - return owner; - } - @Override public ListResponse listDataCenters(ListZonesCmd cmd) { Pair, Integer> result = listDataCentersInternal(cmd); diff --git a/server/src/test/java/com/cloud/api/query/QueryManagerImplTest.java b/server/src/test/java/com/cloud/api/query/QueryManagerImplTest.java index e3faf5e3a8f8..e0093743845c 100644 --- a/server/src/test/java/com/cloud/api/query/QueryManagerImplTest.java +++ b/server/src/test/java/com/cloud/api/query/QueryManagerImplTest.java @@ -17,27 +17,25 @@ package com.cloud.api.query; -import static org.junit.Assert.assertEquals; import static org.mockito.Mockito.when; import java.util.ArrayList; import java.util.List; import java.util.UUID; -import com.cloud.projects.ProjectManager; import org.apache.cloudstack.acl.SecurityChecker; import org.apache.cloudstack.api.ApiCommandResourceType; import org.apache.cloudstack.api.command.user.event.ListEventsCmd; import org.apache.cloudstack.api.response.EventResponse; import org.apache.cloudstack.api.response.ListResponse; import org.apache.cloudstack.context.CallContext; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.Mockito; -import org.mockito.Spy; import org.powermock.api.mockito.PowerMockito; import org.powermock.core.classloader.annotations.PrepareForTest; import org.powermock.modules.junit4.PowerMockRunner; @@ -67,25 +65,14 @@ public class QueryManagerImplTest { public static final long USER_ID = 1; public static final long ACCOUNT_ID = 1; - private long projId = 1l; - private String accountName = "name"; - private long domId = 1l; - @Spy - @InjectMocks - private QueryManagerImpl queryManagerImplSpy = new QueryManagerImpl(); @Mock EntityManager entityManager; @Mock - AccountManager accountManagerMock; - @Mock - ProjectManager projectManagerMock; + AccountManager accountManager; @Mock EventJoinDao eventJoinDao; - @Mock - Account accountMock; - @Mock - Project projectMock; + private AccountVO account; private UserVO user; @@ -103,12 +90,12 @@ private void setupCommonMocks() { user = new UserVO(1, "testuser", "password", "firstname", "lastName", "email", "timezone", UUID.randomUUID().toString(), User.Source.UNKNOWN); CallContext.register(user, account); - Mockito.when(accountManagerMock.isRootAdmin(account.getId())).thenReturn(false); - Mockito.doNothing().when(accountManagerMock).buildACLSearchParameters(Mockito.any(Account.class), Mockito.anyLong(), Mockito.anyString(), Mockito.anyLong(), Mockito.anyList(), + Mockito.when(accountManager.isRootAdmin(account.getId())).thenReturn(false); + Mockito.doNothing().when(accountManager).buildACLSearchParameters(Mockito.any(Account.class), Mockito.anyLong(), Mockito.anyString(), Mockito.anyLong(), Mockito.anyList(), Mockito.any(), Mockito.anyBoolean(), Mockito.anyBoolean()); - Mockito.doNothing().when(accountManagerMock).buildACLSearchBuilder(Mockito.any(SearchBuilder.class), Mockito.anyLong(), Mockito.anyBoolean(), Mockito.anyList(), + Mockito.doNothing().when(accountManager).buildACLSearchBuilder(Mockito.any(SearchBuilder.class), Mockito.anyLong(), Mockito.anyBoolean(), Mockito.anyList(), Mockito.any(Project.ListProjectResourcesCriteria.class)); - Mockito.doNothing().when(accountManagerMock).buildACLViewSearchCriteria(Mockito.any(), Mockito.anyLong(), Mockito.anyBoolean(), Mockito.anyList(), + Mockito.doNothing().when(accountManager).buildACLViewSearchCriteria(Mockito.any(), Mockito.anyLong(), Mockito.anyBoolean(), Mockito.anyList(), Mockito.any(Project.ListProjectResourcesCriteria.class)); final SearchBuilder searchBuilder = Mockito.mock(SearchBuilder.class); final SearchCriteria searchCriteria = Mockito.mock(SearchCriteria.class); @@ -141,7 +128,7 @@ public void searchForEventsSuccess() { Mockito.when(network.getId()).thenReturn(1L); Mockito.when(network.getAccountId()).thenReturn(account.getId()); Mockito.when(entityManager.findByUuidIncludingRemoved(Network.class, uuid)).thenReturn(network); - Mockito.doNothing().when(accountManagerMock).checkAccess(account, SecurityChecker.AccessType.ListEntry, true, network); + Mockito.doNothing().when(accountManager).checkAccess(account, SecurityChecker.AccessType.ListEntry, true, network); Mockito.when(eventJoinDao.searchAndCount(Mockito.any(), Mockito.any(Filter.class))).thenReturn(pair); List respList = new ArrayList(); for (EventJoinVO vt : events) { @@ -150,7 +137,7 @@ public void searchForEventsSuccess() { PowerMockito.mockStatic(ViewResponseHelper.class); Mockito.when(ViewResponseHelper.createEventResponse(Mockito.any())).thenReturn(respList); ListResponse result = queryManager.searchForEvents(cmd); - assertEquals((int) result.getCount(), events.size()); + Assert.assertEquals((int) result.getCount(), events.size()); } @Test(expected = InvalidParameterValueException.class) @@ -197,118 +184,7 @@ public void searchForEventsFailPermissionDenied() { Mockito.when(network.getId()).thenReturn(1L); Mockito.when(network.getAccountId()).thenReturn(2L); Mockito.when(entityManager.findByUuidIncludingRemoved(Network.class, uuid)).thenReturn(network); - Mockito.doThrow(new PermissionDeniedException("Denied")).when(accountManagerMock).checkAccess(account, SecurityChecker.AccessType.ListEntry, false, network); + Mockito.doThrow(new PermissionDeniedException("Denied")).when(accountManager).checkAccess(account, SecurityChecker.AccessType.ListEntry, false, network); queryManager.searchForEvents(cmd); } - - @Test - public void getCallerAccordingToProjectIdAndAccountNameAndDomainIdTestReturnOriginalCaller() { - Account result = queryManagerImplSpy.getCallerAccordingToProjectIdAndAccountNameAndDomainId(accountMock, null, null, null); - Mockito.verify(queryManagerImplSpy, Mockito.times(0)).getCallerAccordingToAccountNameAndDomainId(Mockito.any(Account.class), Mockito.anyString(), Mockito.anyLong()); - Mockito.verify(queryManagerImplSpy, Mockito.times(0)).getCallerAccordingToProjectId(Mockito.any(Account.class), Mockito.anyLong()); - assertEquals(result, accountMock); - } - - @Test - public void getCallerAccordingToProjectIdAndAccountNameAndDomainIdTestProjectIdNull() { - Account anotherAccount = Mockito.mock(Account.class); - Mockito.doReturn(anotherAccount).when(queryManagerImplSpy).getCallerAccordingToAccountNameAndDomainId(Mockito.any(Account.class), Mockito.anyString(), Mockito.anyLong()); - Account result = queryManagerImplSpy.getCallerAccordingToProjectIdAndAccountNameAndDomainId(accountMock, null, accountName, domId); - Mockito.verify(queryManagerImplSpy, Mockito.times(1)).getCallerAccordingToAccountNameAndDomainId(Mockito.any(Account.class), Mockito.anyString(), Mockito.anyLong()); - Mockito.verify(queryManagerImplSpy, Mockito.times(0)).getCallerAccordingToProjectId(Mockito.any(Account.class), Mockito.anyLong()); - assertEquals(result, anotherAccount); - } - - @Test - public void getCallerAccordingToProjectIdAndAccountNameAndDomainIdTestAccountNameAndDomainIdNull() { - Account projectAccount = Mockito.mock(Account.class); - Mockito.doReturn(projectAccount).when(queryManagerImplSpy).getCallerAccordingToProjectId(Mockito.any(Account.class), Mockito.anyLong()); - Account result = queryManagerImplSpy.getCallerAccordingToProjectIdAndAccountNameAndDomainId(accountMock, projId, null, null); - Mockito.verify(queryManagerImplSpy, Mockito.times(0)).getCallerAccordingToAccountNameAndDomainId(Mockito.any(Account.class), Mockito.anyString(), Mockito.anyLong()); - Mockito.verify(queryManagerImplSpy, Mockito.times(1)).getCallerAccordingToProjectId(Mockito.any(Account.class), Mockito.anyLong()); - assertEquals(result, projectAccount); - } - - @Test - public void getCallerAccordingToProjectIdAndAccountNameAndDomainIdTestAccountNameNull() { - Account projectAccount = Mockito.mock(Account.class); - Mockito.doReturn(projectAccount).when(queryManagerImplSpy).getCallerAccordingToProjectId(Mockito.any(Account.class), Mockito.anyLong()); - Account result = queryManagerImplSpy.getCallerAccordingToProjectIdAndAccountNameAndDomainId(accountMock, projId, null, domId); - Mockito.verify(queryManagerImplSpy, Mockito.times(0)).getCallerAccordingToAccountNameAndDomainId(Mockito.any(Account.class), Mockito.anyString(), Mockito.anyLong()); - Mockito.verify(queryManagerImplSpy, Mockito.times(1)).getCallerAccordingToProjectId(Mockito.any(Account.class), Mockito.anyLong()); - assertEquals(result, projectAccount); - } - - @Test - public void getCallerAccordingToProjectIdAndAccountNameAndDomainIdTestDomainIdNull() { - Account projectAccount = Mockito.mock(Account.class); - Mockito.doReturn(projectAccount).when(queryManagerImplSpy).getCallerAccordingToProjectId(Mockito.any(Account.class), Mockito.anyLong()); - Account result = queryManagerImplSpy.getCallerAccordingToProjectIdAndAccountNameAndDomainId(accountMock, projId, accountName, null); - Mockito.verify(queryManagerImplSpy, Mockito.times(0)).getCallerAccordingToAccountNameAndDomainId(Mockito.any(Account.class), Mockito.anyString(), Mockito.anyLong()); - Mockito.verify(queryManagerImplSpy, Mockito.times(1)).getCallerAccordingToProjectId(Mockito.any(Account.class), Mockito.anyLong()); - assertEquals(result, projectAccount); - } - - @Test(expected = InvalidParameterValueException.class) - public void getCallerAccordingToAccountNameAndDomainIdTestThrowInvalidParameterValueException() { - Mockito.doReturn(null).when(accountManagerMock).getActiveAccountByName(Mockito.anyString(), Mockito.anyLong()); - queryManagerImplSpy.getCallerAccordingToAccountNameAndDomainId(accountMock, accountName, domId); - } - - @Test(expected = PermissionDeniedException.class) - public void getCallerAccordingToAccountNameAndDomainIdTestThrowPermissionDeniedException() { - Account anotherAccount = Mockito.mock(Account.class); - Mockito.doReturn(anotherAccount).when(accountManagerMock).getActiveAccountByName(Mockito.anyString(), Mockito.anyLong()); - Mockito.doThrow(PermissionDeniedException.class).when(accountManagerMock).checkAccess(accountMock, null, true, anotherAccount); - queryManagerImplSpy.getCallerAccordingToAccountNameAndDomainId(accountMock, accountName, domId); - } - - public void getCallerAccordingToAccountNameAndDomainIdTestReturnAccount() { - Account anotherAccount = Mockito.mock(Account.class); - Mockito.doReturn(anotherAccount).when(accountManagerMock).getActiveAccountByName(Mockito.anyString(), Mockito.anyLong()); - Account result = queryManagerImplSpy.getCallerAccordingToAccountNameAndDomainId(accountMock, accountName, domId); - assertEquals(result, anotherAccount); - } - - @Test - public void getCallerAccordingToProjectIdTestReturnProjectAccount() { - Account anotherAccount = Mockito.mock(Account.class); - Mockito.doReturn(projectMock).when(projectManagerMock).getProject(projId); - Mockito.doReturn(anotherAccount).when(accountManagerMock).getActiveAccountById(Mockito.anyLong()); - Mockito.doReturn(true).when(projectManagerMock).canAccessProjectAccount(Mockito.any(Account.class), Mockito.anyLong()); - Account result = queryManagerImplSpy.getCallerAccordingToProjectId(accountMock, projId); - assertEquals(result, anotherAccount); - } - - @Test(expected = InvalidParameterValueException.class) - public void getCallerAccordingToProjectIdTestProjectNotFound() { - Mockito.doReturn(null).when(projectManagerMock).getProject(projId); - queryManagerImplSpy.getCallerAccordingToProjectId(accountMock, projId); - } - - @Test(expected = InvalidParameterValueException.class) - public void getCallerAccordingToProjectIdTestAccountNotFound() { - Mockito.doReturn(projectMock).when(projectManagerMock).getProject(projId); - Mockito.doReturn(null).when(accountManagerMock).getActiveAccountById(Mockito.anyLong()); - queryManagerImplSpy.getCallerAccordingToProjectId(accountMock, projId); - } - - @Test(expected = InvalidParameterValueException.class) - public void getCallerAccordingToProjectIdTestAccountCanAccessProject() { - Account anotherAccount = Mockito.mock(Account.class); - Mockito.doReturn(projectMock).when(projectManagerMock).getProject(projId); - Mockito.doReturn(anotherAccount).when(accountManagerMock).getActiveAccountById(Mockito.anyLong()); - Mockito.doReturn(false).when(projectManagerMock).canAccessProjectAccount(Mockito.any(Account.class), Mockito.anyLong()); - queryManagerImplSpy.getCallerAccordingToProjectId(accountMock, projId); - } - - @Test(expected = PermissionDeniedException.class) - public void getCallerAccordingToProjectIdTestCheckAccess() { - Account anotherAccount = Mockito.mock(Account.class); - Mockito.doReturn(projectMock).when(projectManagerMock).getProject(projId); - Mockito.doReturn(anotherAccount).when(accountManagerMock).getActiveAccountById(Mockito.anyLong()); - Mockito.doReturn(true).when(projectManagerMock).canAccessProjectAccount(Mockito.any(Account.class), Mockito.anyLong()); - Mockito.doThrow(PermissionDeniedException.class).when(accountManagerMock).checkAccess(accountMock, null, true, anotherAccount); - queryManagerImplSpy.getCallerAccordingToProjectId(accountMock, projId); - } } \ No newline at end of file From f135a515dba75cccaa8f349cb74b1413e2741988 Mon Sep 17 00:00:00 2001 From: Lopez Date: Mon, 14 Aug 2023 14:33:18 -0300 Subject: [PATCH 3/6] changes BaseListDomainResourcesCmd by BaseListProjectAndAccountResourcesCmd --- .../api/command/user/offering/ListDiskOfferingsCmd.java | 6 +++--- .../command/user/offering/ListServiceOfferingsCmd.java | 9 +++++---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListDiskOfferingsCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListDiskOfferingsCmd.java index e2d28c0937dc..d06cbb8431f3 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListDiskOfferingsCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListDiskOfferingsCmd.java @@ -16,6 +16,7 @@ // under the License. package org.apache.cloudstack.api.command.user.offering; +import org.apache.cloudstack.api.BaseListProjectAndAccountResourcesCmd; import org.apache.cloudstack.api.response.StoragePoolResponse; import org.apache.cloudstack.api.response.ProjectResponse; import org.apache.cloudstack.api.response.VolumeResponse; @@ -24,14 +25,13 @@ import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiConstants; -import org.apache.cloudstack.api.BaseListDomainResourcesCmd; import org.apache.cloudstack.api.Parameter; import org.apache.cloudstack.api.response.DiskOfferingResponse; import org.apache.cloudstack.api.response.ListResponse; @APICommand(name = "listDiskOfferings", description = "Lists all available disk offerings.", responseObject = DiskOfferingResponse.class, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) -public class ListDiskOfferingsCmd extends BaseListDomainResourcesCmd { +public class ListDiskOfferingsCmd extends BaseListProjectAndAccountResourcesCmd { public static final Logger s_logger = Logger.getLogger(ListDiskOfferingsCmd.class.getName()); @@ -45,7 +45,7 @@ public class ListDiskOfferingsCmd extends BaseListDomainResourcesCmd { @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, description = "name of the disk offering") private String diskOfferingName; - @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "list resources by account. Must be used with the domainId parameter.") + @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "list resources by account.") private String accountName; @Parameter(name = ApiConstants.PROJECT_ID, type = CommandType.UUID, entityType = ProjectResponse.class, description = "list objects by project.") diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java index 098cad5e4692..1f7a935ad2fc 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java @@ -16,13 +16,14 @@ // under the License. package org.apache.cloudstack.api.command.user.offering; +import org.apache.cloudstack.api.BaseCmd; +import org.apache.cloudstack.api.BaseListProjectAndAccountResourcesCmd; import org.apache.cloudstack.api.response.ProjectResponse; import org.apache.cloudstack.api.response.ZoneResponse; import org.apache.log4j.Logger; import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiConstants; -import org.apache.cloudstack.api.BaseListDomainResourcesCmd; import org.apache.cloudstack.api.Parameter; import org.apache.cloudstack.api.response.ListResponse; import org.apache.cloudstack.api.response.ServiceOfferingResponse; @@ -30,7 +31,7 @@ @APICommand(name = "listServiceOfferings", description = "Lists all available service offerings.", responseObject = ServiceOfferingResponse.class, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) -public class ListServiceOfferingsCmd extends BaseListDomainResourcesCmd { +public class ListServiceOfferingsCmd extends BaseListProjectAndAccountResourcesCmd { public static final Logger s_logger = Logger.getLogger(ListServiceOfferingsCmd.class.getName()); @@ -38,7 +39,7 @@ public class ListServiceOfferingsCmd extends BaseListDomainResourcesCmd { //////////////// API parameters ///////////////////// ///////////////////////////////////////////////////// - @Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = ServiceOfferingResponse.class, description = "ID of the service offering") + @Parameter(name = ApiConstants.ID, type = BaseCmd.CommandType.UUID, entityType = ServiceOfferingResponse.class, description = "ID of the service offering") private Long id; @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, description = "name of the service offering") @@ -83,7 +84,7 @@ public class ListServiceOfferingsCmd extends BaseListDomainResourcesCmd { since = "4.15") private Integer cpuSpeed; - @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "list resources by account. Must be used with the domainId parameter.") + @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "list resources by account.") private String accountName; @Parameter(name = ApiConstants.PROJECT_ID, type = CommandType.UUID, entityType = ProjectResponse.class, description = "list objects by project.") From d9e5868e3dead2c822268fc908b61a6b3b8648d8 Mon Sep 17 00:00:00 2001 From: "Rodrigo D. Lopez" <19981369+RodrigoDLopez@users.noreply.github.com> Date: Wed, 16 Aug 2023 08:17:07 -0300 Subject: [PATCH 4/6] Apply suggestions from code review Co-authored-by: dahn --- .../user/offering/ListDiskOfferingsCmd.java | 14 -------------- .../user/offering/ListServiceOfferingsCmd.java | 14 -------------- 2 files changed, 28 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListDiskOfferingsCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListDiskOfferingsCmd.java index d06cbb8431f3..6ac13fb42952 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListDiskOfferingsCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListDiskOfferingsCmd.java @@ -45,12 +45,6 @@ public class ListDiskOfferingsCmd extends BaseListProjectAndAccountResourcesCmd @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, description = "name of the disk offering") private String diskOfferingName; - @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "list resources by account.") - private String accountName; - - @Parameter(name = ApiConstants.PROJECT_ID, type = CommandType.UUID, entityType = ProjectResponse.class, description = "list objects by project.") - private Long projectId; - @Parameter(name = ApiConstants.ZONE_ID, type = CommandType.UUID, entityType = ZoneResponse.class, @@ -91,14 +85,6 @@ public Long getVolumeId() { public Boolean getEncrypt() { return encrypt; } - public String getAccountName() { - return accountName; - } - - public Long getProjectId() { - return projectId; - } - ///////////////////////////////////////////////////// /////////////// API Implementation/////////////////// ///////////////////////////////////////////////////// diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java index 1f7a935ad2fc..e50b060452e6 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java @@ -84,12 +84,6 @@ public class ListServiceOfferingsCmd extends BaseListProjectAndAccountResourcesC since = "4.15") private Integer cpuSpeed; - @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "list resources by account.") - private String accountName; - - @Parameter(name = ApiConstants.PROJECT_ID, type = CommandType.UUID, entityType = ProjectResponse.class, description = "list objects by project.") - private Long projectId; - @Parameter(name = ApiConstants.ENCRYPT_ROOT, type = CommandType.BOOLEAN, description = "listed offerings support root disk encryption", @@ -138,14 +132,6 @@ public Integer getCpuSpeed() { public Boolean getEncryptRoot() { return encryptRoot; } - public String getAccountName() { - return accountName; - } - - public Long getProjectId() { - return projectId; - } - ///////////////////////////////////////////////////// /////////////// API Implementation/////////////////// ///////////////////////////////////////////////////// From 2ce4f62a404b74898596fb5b2459948648a58614 Mon Sep 17 00:00:00 2001 From: Lopez Date: Wed, 16 Aug 2023 10:20:01 -0300 Subject: [PATCH 5/6] removes unused imports and address Wei reviews --- .../user/offering/ListDiskOfferingsCmd.java | 1 - .../offering/ListServiceOfferingsCmd.java | 4 +--- .../com/cloud/api/query/QueryManagerImpl.java | 22 +++++++++---------- 3 files changed, 12 insertions(+), 15 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListDiskOfferingsCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListDiskOfferingsCmd.java index 6ac13fb42952..5ab675ae435f 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListDiskOfferingsCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListDiskOfferingsCmd.java @@ -18,7 +18,6 @@ import org.apache.cloudstack.api.BaseListProjectAndAccountResourcesCmd; import org.apache.cloudstack.api.response.StoragePoolResponse; -import org.apache.cloudstack.api.response.ProjectResponse; import org.apache.cloudstack.api.response.VolumeResponse; import org.apache.cloudstack.api.response.ZoneResponse; import org.apache.log4j.Logger; diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java index e50b060452e6..cb155d24ad8f 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java @@ -16,9 +16,7 @@ // under the License. package org.apache.cloudstack.api.command.user.offering; -import org.apache.cloudstack.api.BaseCmd; import org.apache.cloudstack.api.BaseListProjectAndAccountResourcesCmd; -import org.apache.cloudstack.api.response.ProjectResponse; import org.apache.cloudstack.api.response.ZoneResponse; import org.apache.log4j.Logger; @@ -39,7 +37,7 @@ public class ListServiceOfferingsCmd extends BaseListProjectAndAccountResourcesC //////////////// API parameters ///////////////////// ///////////////////////////////////////////////////// - @Parameter(name = ApiConstants.ID, type = BaseCmd.CommandType.UUID, entityType = ServiceOfferingResponse.class, description = "ID of the service offering") + @Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = ServiceOfferingResponse.class, description = "ID of the service offering") private Long id; @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, description = "name of the service offering") diff --git a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java index d7a2750eaacb..bd01ced48ff9 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -3109,20 +3109,20 @@ private Pair, Integer> searchForServiceOfferingsInte Integer cpuSpeed = cmd.getCpuSpeed(); Boolean encryptRoot = cmd.getEncryptRoot(); - caller = _accountMgr.finalizeOwner(caller, accountName, domainId, projectId); + final Account owner = _accountMgr.finalizeOwner(caller, accountName, domainId, projectId); SearchCriteria sc = _srvOfferingJoinDao.createSearchCriteria(); - if (!_accountMgr.isRootAdmin(caller.getId()) && isSystem) { + if (!_accountMgr.isRootAdmin(owner.getId()) && isSystem) { throw new InvalidParameterValueException("Only ROOT admins can access system offerings."); } // Keeping this logic consistent with domain specific zones // if a domainId is provided, we just return the so associated with this // domain - if (domainId != null && !_accountMgr.isRootAdmin(caller.getId())) { + if (domainId != null && !_accountMgr.isRootAdmin(owner.getId())) { // check if the user's domain == so's domain || user's domain is a // child of so's domain - if (!isPermissible(caller.getDomainId(), domainId)) { - throw new PermissionDeniedException("The account:" + caller.getAccountName() + " does not fall in the same domain hierarchy as the service offering"); + if (!isPermissible(owner.getDomainId(), domainId)) { + throw new PermissionDeniedException("The account:" + owner.getAccountName() + " does not fall in the same domain hierarchy as the service offering"); } } @@ -3134,7 +3134,7 @@ private Pair, Integer> searchForServiceOfferingsInte throw ex; } - _accountMgr.checkAccess(caller, null, true, vmInstance); + _accountMgr.checkAccess(owner, null, true, vmInstance); currentVmOffering = _srvOfferingDao.findByIdIncludingRemoved(vmInstance.getId(), vmInstance.getServiceOfferingId()); if (! currentVmOffering.isDynamic()) { @@ -3182,19 +3182,19 @@ private Pair, Integer> searchForServiceOfferingsInte } // boolean includePublicOfferings = false; - if ((_accountMgr.isNormalUser(caller.getId()) || _accountMgr.isDomainAdmin(caller.getId())) || caller.getType() == Account.Type.RESOURCE_DOMAIN_ADMIN) { + if ((_accountMgr.isNormalUser(owner.getId()) || _accountMgr.isDomainAdmin(owner.getId())) || owner.getType() == Account.Type.RESOURCE_DOMAIN_ADMIN) { // For non-root users. if (isSystem) { throw new InvalidParameterValueException("Only root admins can access system's offering"); } if (isRecursive) { // domain + all sub-domains - if (caller.getType() == Account.Type.NORMAL) { + if (owner.getType() == Account.Type.NORMAL) { throw new InvalidParameterValueException("Only ROOT admins and Domain admins can list service offerings with isrecursive=true"); } } } else { // for root users - if (caller.getDomainId() != 1 && isSystem) { // NON ROOT admin + if (owner.getDomainId() != 1 && isSystem) { // NON ROOT admin throw new InvalidParameterValueException("Non ROOT admins cannot access system's offering."); } if (domainId != null && accountName == null) { @@ -3281,8 +3281,8 @@ private Pair, Integer> searchForServiceOfferingsInte // Filter offerings that are not associated with caller's domain // Fetch the offering ids from the details table since theres no smart way to filter them in the join ... yet! - if (caller.getType() != Account.Type.ADMIN) { - Domain callerDomain = _domainDao.findById(caller.getDomainId()); + if (owner.getType() != Account.Type.ADMIN) { + Domain callerDomain = _domainDao.findById(owner.getDomainId()); List domainIds = findRelatedDomainIds(callerDomain, isRecursive); List ids = _srvOfferingDetailsDao.findOfferingIdsByDomainIds(domainIds); From 0d04398e9a72e7bf602497a42e2d631de34de7ec Mon Sep 17 00:00:00 2001 From: Lopez Date: Thu, 5 Oct 2023 10:31:07 -0300 Subject: [PATCH 6/6] small adjustment when use caller or owner --- .../src/main/java/com/cloud/api/query/QueryManagerImpl.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java index bd01ced48ff9..ca2ed80d85d9 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -3111,14 +3111,14 @@ private Pair, Integer> searchForServiceOfferingsInte final Account owner = _accountMgr.finalizeOwner(caller, accountName, domainId, projectId); SearchCriteria sc = _srvOfferingJoinDao.createSearchCriteria(); - if (!_accountMgr.isRootAdmin(owner.getId()) && isSystem) { + if (!_accountMgr.isRootAdmin(caller.getId()) && isSystem) { throw new InvalidParameterValueException("Only ROOT admins can access system offerings."); } // Keeping this logic consistent with domain specific zones // if a domainId is provided, we just return the so associated with this // domain - if (domainId != null && !_accountMgr.isRootAdmin(owner.getId())) { + if (domainId != null && !_accountMgr.isRootAdmin(caller.getId())) { // check if the user's domain == so's domain || user's domain is a // child of so's domain if (!isPermissible(owner.getDomainId(), domainId)) {