Skip to content

Commit 15f5279

Browse files
winterhazelwinterhazel
committed
Fix ISOs and templates listing pagination
1 parent c3aeba1 commit 15f5279

File tree

4 files changed

+151
-19
lines changed

4 files changed

+151
-19
lines changed

‎server/src/main/java/com/cloud/api/query/QueryManagerImpl.java‎

Lines changed: 51 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
import java.util.Map;
3030
import java.util.Set;
3131
import java.util.UUID;
32-
import java.util.function.Predicate;
3332
import java.util.stream.Collectors;
3433
import java.util.stream.Stream;
3534

@@ -3813,11 +3812,62 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN)
38133812
}
38143813
}
38153814

3815+
applyPublicTemplateSharingRestrictions(sc, caller);
3816+
38163817
return templateChecks(isIso, hypers, tags, name, keyword, hyperType, onlyReady, bootable, zoneId, showDomr, caller,
38173818
showRemovedTmpl, parentTemplateId, showUnique, searchFilter, sc);
38183819

38193820
}
38203821

3822+
/**
3823+
* If the caller is not a root admin, restricts the search to return only public templates from the domain which
3824+
* the caller belongs to and domains with the setting 'share.public.templates.with.other.domains' enabled.
3825+
*/
3826+
protected void applyPublicTemplateSharingRestrictions(SearchCriteria<TemplateJoinVO> sc, Account caller) {
3827+
if (caller.getType() == Account.Type.ADMIN) {
3828+
s_logger.debug(String.format("Account [%s] is a root admin. Therefore, it has access to all public templates.", caller));
3829+
return;
3830+
}
3831+
3832+
List<TemplateJoinVO> publicTemplates = _templateJoinDao.listPublicTemplates();
3833+
3834+
Set<Long> unsharableDomainIds = new HashSet<>();
3835+
for (TemplateJoinVO template : publicTemplates) {
3836+
addDomainIdToSetIfDomainDoesNotShareTemplates(template.getDomainId(), caller, unsharableDomainIds);
3837+
}
3838+
3839+
if (!unsharableDomainIds.isEmpty()) {
3840+
s_logger.info(String.format("The public templates belonging to the domains [%s] will not be listed to account [%s] as they have the configuration [%s] marked as 'false'.", unsharableDomainIds, caller, QueryService.SharePublicTemplatesWithOtherDomains.key()));
3841+
sc.addAnd("domainId", SearchCriteria.Op.NOTIN, unsharableDomainIds.toArray());
3842+
}
3843+
}
3844+
3845+
/**
3846+
* Adds the provided domain ID to the set if the domain does not share templates with the account. That is, if:
3847+
* (1) the template does not belong to the domain of the account AND
3848+
* (2) the domain of the template has the setting 'share.public.templates.with.other.domains' disabled.
3849+
*/
3850+
protected void addDomainIdToSetIfDomainDoesNotShareTemplates(long domainId, Account account, Set<Long> unsharableDomainIds) {
3851+
if (domainId == account.getDomainId()) {
3852+
s_logger.trace(String.format("Domain [%s] will not be added to the set of domains with unshared templates since the account [%s] belongs to it.", domainId, account));
3853+
return;
3854+
}
3855+
3856+
if (unsharableDomainIds.contains(domainId)) {
3857+
s_logger.trace(String.format("Domain [%s] is already on the set of domains with unshared templates.", domainId));
3858+
return;
3859+
}
3860+
3861+
if (!checkIfDomainSharesTemplates(domainId)) {
3862+
s_logger.debug(String.format("Domain [%s] will be added to the set of domains with unshared templates as configuration [%s] is false.", domainId, QueryService.SharePublicTemplatesWithOtherDomains.key()));
3863+
unsharableDomainIds.add(domainId);
3864+
}
3865+
}
3866+
3867+
protected boolean checkIfDomainSharesTemplates(Long domainId) {
3868+
return QueryService.SharePublicTemplatesWithOtherDomains.valueIn(domainId);
3869+
}
3870+
38213871
private Pair<List<TemplateJoinVO>, Integer> templateChecks(boolean isIso, List<HypervisorType> hypers, Map<String, String> tags, String name, String keyword,
38223872
HypervisorType hyperType, boolean onlyReady, Boolean bootable, Long zoneId, boolean showDomr, Account caller,
38233873
boolean showRemovedTmpl, Long parentTemplateId, Boolean showUnique,
@@ -3947,27 +3997,9 @@ private Pair<List<TemplateJoinVO>, Integer> findTemplatesByIdOrTempZonePair(Pair
39473997
templates = _templateJoinDao.searchByTemplateZonePair(showRemoved, templateZonePairs);
39483998
}
39493999

3950-
if(caller.getType() != Account.Type.ADMIN) {
3951-
templates = applyPublicTemplateRestriction(templates, caller);
3952-
count = templates.size();
3953-
}
3954-
39554000
return new Pair<List<TemplateJoinVO>, Integer>(templates, count);
39564001
}
39574002

3958-
private List<TemplateJoinVO> applyPublicTemplateRestriction(List<TemplateJoinVO> templates, Account caller){
3959-
List<Long> unsharableDomainIds = templates.stream()
3960-
.map(TemplateJoinVO::getDomainId)
3961-
.distinct()
3962-
.filter(domainId -> domainId != caller.getDomainId())
3963-
.filter(Predicate.not(QueryService.SharePublicTemplatesWithOtherDomains::valueIn))
3964-
.collect(Collectors.toList());
3965-
3966-
return templates.stream()
3967-
.filter(Predicate.not(t -> unsharableDomainIds.contains(t.getDomainId())))
3968-
.collect(Collectors.toList());
3969-
}
3970-
39714003
@Override
39724004
public ListResponse<TemplateResponse> listIsos(ListIsosCmd cmd) {
39734005
Pair<List<TemplateJoinVO>, Integer> result = searchForIsosInternal(cmd);

‎server/src/main/java/com/cloud/api/query/dao/TemplateJoinDao.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,8 @@ public interface TemplateJoinDao extends GenericDao<TemplateJoinVO, Long> {
4848

4949
List<TemplateJoinVO> listActiveTemplates(long storeId);
5050

51+
List<TemplateJoinVO> listPublicTemplates();
52+
5153
Pair<List<TemplateJoinVO>, Integer> searchIncludingRemovedAndCount(final SearchCriteria<TemplateJoinVO> sc, final Filter filter);
5254

5355
List<TemplateJoinVO> findByDistinctIds(Long... ids);

‎server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,8 @@ public class TemplateJoinDaoImpl extends GenericDaoBaseWithTagInformation<Templa
104104

105105
private final SearchBuilder<TemplateJoinVO> activeTmpltSearch;
106106

107+
private final SearchBuilder<TemplateJoinVO> publicTmpltSearch;
108+
107109
protected TemplateJoinDaoImpl() {
108110

109111
tmpltIdPairSearch = createSearchBuilder();
@@ -137,6 +139,10 @@ protected TemplateJoinDaoImpl() {
137139
activeTmpltSearch.cp();
138140
activeTmpltSearch.done();
139141

142+
publicTmpltSearch = createSearchBuilder();
143+
publicTmpltSearch.and("public", publicTmpltSearch.entity().isPublicTemplate(), SearchCriteria.Op.EQ);
144+
publicTmpltSearch.done();
145+
140146
// select distinct pair (template_id, zone_id)
141147
_count = "select count(distinct temp_zone_pair) from template_view WHERE ";
142148
}
@@ -572,6 +578,13 @@ public List<TemplateJoinVO> listActiveTemplates(long storeId) {
572578
return searchIncludingRemoved(sc, null, null, false);
573579
}
574580

581+
@Override
582+
public List<TemplateJoinVO> listPublicTemplates() {
583+
SearchCriteria<TemplateJoinVO> sc = publicTmpltSearch.create();
584+
sc.setParameters("public", Boolean.TRUE);
585+
return listBy(sc);
586+
}
587+
575588
@Override
576589
public Pair<List<TemplateJoinVO>, Integer> searchIncludingRemovedAndCount(final SearchCriteria<TemplateJoinVO> sc, final Filter filter) {
577590
List<TemplateJoinVO> objects = searchIncludingRemoved(sc, filter, null, false);

‎server/src/test/java/com/cloud/api/query/QueryManagerImplTest.java‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,9 @@
1717

1818
package com.cloud.api.query;
1919

20+
import com.cloud.api.query.dao.TemplateJoinDao;
2021
import com.cloud.api.query.vo.EventJoinVO;
22+
import com.cloud.api.query.vo.TemplateJoinVO;
2123
import com.cloud.event.dao.EventJoinDao;
2224
import com.cloud.exception.InvalidParameterValueException;
2325
import com.cloud.exception.PermissionDeniedException;
@@ -34,6 +36,7 @@
3436
import com.cloud.utils.db.SearchBuilder;
3537
import com.cloud.utils.db.SearchCriteria;
3638
import com.cloud.vm.VirtualMachine;
39+
3740
import org.apache.cloudstack.acl.SecurityChecker;
3841
import org.apache.cloudstack.api.ApiCommandResourceType;
3942
import org.apache.cloudstack.api.command.user.event.ListEventsCmd;
@@ -48,10 +51,14 @@
4851
import org.mockito.Mock;
4952
import org.mockito.MockedStatic;
5053
import org.mockito.Mockito;
54+
import org.mockito.Spy;
55+
5156
import org.mockito.junit.MockitoJUnitRunner;
5257

5358
import java.util.ArrayList;
59+
import java.util.HashSet;
5460
import java.util.List;
61+
import java.util.Set;
5562
import java.util.UUID;
5663

5764
import static org.mockito.Mockito.when;
@@ -61,13 +68,28 @@ public class QueryManagerImplTest {
6168
public static final long USER_ID = 1;
6269
public static final long ACCOUNT_ID = 1;
6370

71+
@Spy
72+
@InjectMocks
73+
private QueryManagerImpl queryManagerImplSpy = new QueryManagerImpl();
74+
6475
@Mock
6576
EntityManager entityManager;
77+
6678
@Mock
6779
AccountManager accountManager;
80+
6881
@Mock
6982
EventJoinDao eventJoinDao;
7083

84+
@Mock
85+
Account accountMock;
86+
87+
@Mock
88+
TemplateJoinDao templateJoinDaoMock;
89+
90+
@Mock
91+
SearchCriteria searchCriteriaMock;
92+
7193
private AccountVO account;
7294
private UserVO user;
7395

@@ -176,4 +198,67 @@ public void searchForEventsFailPermissionDenied() {
176198
Mockito.doThrow(new PermissionDeniedException("Denied")).when(accountManager).checkAccess(account, SecurityChecker.AccessType.ListEntry, false, network);
177199
queryManager.searchForEvents(cmd);
178200
}
201+
202+
@Test
203+
public void applyPublicTemplateRestrictionsTestDoesNotApplyRestrictionsWhenCallerIsRootAdmin() {
204+
Mockito.when(accountMock.getType()).thenReturn(Account.Type.ADMIN);
205+
206+
queryManagerImplSpy.applyPublicTemplateSharingRestrictions(searchCriteriaMock, accountMock);
207+
208+
Mockito.verify(searchCriteriaMock, Mockito.never()).addAnd(Mockito.anyString(), Mockito.any(), Mockito.any());
209+
}
210+
211+
@Test
212+
public void applyPublicTemplateRestrictionsTestAppliesRestrictionsWhenCallerIsNotRootAdmin() {
213+
long callerDomainId = 1L;
214+
long sharableDomainId = 2L;
215+
long unsharableDomainId = 3L;
216+
217+
Mockito.when(accountMock.getType()).thenReturn(Account.Type.NORMAL);
218+
219+
Mockito.when(accountMock.getDomainId()).thenReturn(callerDomainId);
220+
TemplateJoinVO templateMock1 = Mockito.mock(TemplateJoinVO.class);
221+
Mockito.when(templateMock1.getDomainId()).thenReturn(callerDomainId);
222+
Mockito.lenient().doReturn(false).when(queryManagerImplSpy).checkIfDomainSharesTemplates(callerDomainId);
223+
224+
TemplateJoinVO templateMock2 = Mockito.mock(TemplateJoinVO.class);
225+
Mockito.when(templateMock2.getDomainId()).thenReturn(sharableDomainId);
226+
Mockito.doReturn(true).when(queryManagerImplSpy).checkIfDomainSharesTemplates(sharableDomainId);
227+
228+
TemplateJoinVO templateMock3 = Mockito.mock(TemplateJoinVO.class);
229+
Mockito.when(templateMock3.getDomainId()).thenReturn(unsharableDomainId);
230+
Mockito.doReturn(false).when(queryManagerImplSpy).checkIfDomainSharesTemplates(unsharableDomainId);
231+
232+
List<TemplateJoinVO> publicTemplates = List.of(templateMock1, templateMock2, templateMock3);
233+
Mockito.when(templateJoinDaoMock.listPublicTemplates()).thenReturn(publicTemplates);
234+
235+
queryManagerImplSpy.applyPublicTemplateSharingRestrictions(searchCriteriaMock, accountMock);
236+
237+
Mockito.verify(searchCriteriaMock).addAnd("domainId", SearchCriteria.Op.NOTIN, unsharableDomainId);
238+
}
239+
240+
@Test
241+
public void addDomainIdToSetIfDomainDoesNotShareTemplatesTestDoesNotAddWhenCallerBelongsToDomain() {
242+
long domainId = 1L;
243+
Set<Long> set = new HashSet<>();
244+
245+
Mockito.when(accountMock.getDomainId()).thenReturn(domainId);
246+
247+
queryManagerImplSpy.addDomainIdToSetIfDomainDoesNotShareTemplates(domainId, accountMock, set);
248+
249+
Assert.assertEquals(0, set.size());
250+
}
251+
252+
@Test
253+
public void addDomainIdToSetIfDomainDoesNotShareTemplatesTestAddsWhenDomainDoesNotShareTemplates() {
254+
long domainId = 1L;
255+
Set<Long> set = new HashSet<>();
256+
257+
Mockito.when(accountMock.getDomainId()).thenReturn(2L);
258+
Mockito.doReturn(false).when(queryManagerImplSpy).checkIfDomainSharesTemplates(domainId);
259+
260+
queryManagerImplSpy.addDomainIdToSetIfDomainDoesNotShareTemplates(domainId, accountMock, set);
261+
262+
Assert.assertTrue(set.contains(domainId));
263+
}
179264
}

0 commit comments

Comments
 (0)