Skip to content

Commit 136fd66

Browse files
winterhazelwinterhazel
committed
Fix ISOs and templates listing pagination
1 parent 9c9b178 commit 136fd66

File tree

4 files changed

+150
-19
lines changed

4 files changed

+150
-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

@@ -3782,11 +3781,62 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN)
37823781
}
37833782
}
37843783

3784+
applyPublicTemplateSharingRestrictions(sc, caller);
3785+
37853786
return templateChecks(isIso, hypers, tags, name, keyword, hyperType, onlyReady, bootable, zoneId, showDomr, caller,
37863787
showRemovedTmpl, parentTemplateId, showUnique, searchFilter, sc);
37873788

37883789
}
37893790

3791+
/**
3792+
* If the caller is not a root admin, restricts the search to return only public templates from the domain which
3793+
* the caller belongs to and domains with the setting 'share.public.templates.with.other.domains' enabled.
3794+
*/
3795+
protected void applyPublicTemplateSharingRestrictions(SearchCriteria<TemplateJoinVO> sc, Account caller) {
3796+
if (caller.getType() == Account.Type.ADMIN) {
3797+
s_logger.debug(String.format("Account [%s] is a root admin. Therefore, it has access to all public templates.", caller));
3798+
return;
3799+
}
3800+
3801+
List<TemplateJoinVO> publicTemplates = _templateJoinDao.listPublicTemplates();
3802+
3803+
Set<Long> unsharableDomainIds = new HashSet<>();
3804+
for (TemplateJoinVO template : publicTemplates) {
3805+
addDomainIdToSetIfDomainDoesNotShareTemplates(template.getDomainId(), caller, unsharableDomainIds);
3806+
}
3807+
3808+
if (!unsharableDomainIds.isEmpty()) {
3809+
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()));
3810+
sc.addAnd("domainId", SearchCriteria.Op.NOTIN, unsharableDomainIds.toArray());
3811+
}
3812+
}
3813+
3814+
/**
3815+
* Adds the provided domain ID the set if the domain does not share templates with the account. That is, if:
3816+
* (1) the template does not belong to the domain of the account AND
3817+
* (2) the domain of the template has the setting 'share.public.templates.with.other.domains' disabled.
3818+
*/
3819+
protected void addDomainIdToSetIfDomainDoesNotShareTemplates(long domainId, Account account, Set<Long> unsharableDomainIds) {
3820+
if (domainId == account.getDomainId()) {
3821+
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));
3822+
return;
3823+
}
3824+
3825+
if (unsharableDomainIds.contains(domainId)) {
3826+
s_logger.trace(String.format("Domain [%s] is already on the set of domains with unshared templates.", domainId));
3827+
return;
3828+
}
3829+
3830+
if (!checkIfDomainSharesTemplates(domainId)) {
3831+
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()));
3832+
unsharableDomainIds.add(domainId);
3833+
}
3834+
}
3835+
3836+
protected boolean checkIfDomainSharesTemplates(Long domainId) {
3837+
return QueryService.SharePublicTemplatesWithOtherDomains.valueIn(domainId);
3838+
}
3839+
37903840
private Pair<List<TemplateJoinVO>, Integer> templateChecks(boolean isIso, List<HypervisorType> hypers, Map<String, String> tags, String name, String keyword,
37913841
HypervisorType hyperType, boolean onlyReady, Boolean bootable, Long zoneId, boolean showDomr, Account caller,
37923842
boolean showRemovedTmpl, Long parentTemplateId, Boolean showUnique,
@@ -3916,27 +3966,9 @@ private Pair<List<TemplateJoinVO>, Integer> findTemplatesByIdOrTempZonePair(Pair
39163966
templates = _templateJoinDao.searchByTemplateZonePair(showRemoved, templateZonePairs);
39173967
}
39183968

3919-
if(caller.getType() != Account.Type.ADMIN) {
3920-
templates = applyPublicTemplateRestriction(templates, caller);
3921-
count = templates.size();
3922-
}
3923-
39243969
return new Pair<List<TemplateJoinVO>, Integer>(templates, count);
39253970
}
39263971

3927-
private List<TemplateJoinVO> applyPublicTemplateRestriction(List<TemplateJoinVO> templates, Account caller){
3928-
List<Long> unsharableDomainIds = templates.stream()
3929-
.map(TemplateJoinVO::getDomainId)
3930-
.distinct()
3931-
.filter(domainId -> domainId != caller.getDomainId())
3932-
.filter(Predicate.not(QueryService.SharePublicTemplatesWithOtherDomains::valueIn))
3933-
.collect(Collectors.toList());
3934-
3935-
return templates.stream()
3936-
.filter(Predicate.not(t -> unsharableDomainIds.contains(t.getDomainId())))
3937-
.collect(Collectors.toList());
3938-
}
3939-
39403972
@Override
39413973
public ListResponse<TemplateResponse> listIsos(ListIsosCmd cmd) {
39423974
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: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,13 @@
2020
import static org.mockito.Mockito.when;
2121

2222
import java.util.ArrayList;
23+
import java.util.HashSet;
2324
import java.util.List;
25+
import java.util.Set;
2426
import java.util.UUID;
2527

28+
import com.cloud.api.query.dao.TemplateJoinDao;
29+
import com.cloud.api.query.vo.TemplateJoinVO;
2630
import org.apache.cloudstack.acl.SecurityChecker;
2731
import org.apache.cloudstack.api.ApiCommandResourceType;
2832
import org.apache.cloudstack.api.command.user.event.ListEventsCmd;
@@ -36,6 +40,7 @@
3640
import org.mockito.InjectMocks;
3741
import org.mockito.Mock;
3842
import org.mockito.Mockito;
43+
import org.mockito.Spy;
3944
import org.powermock.api.mockito.PowerMockito;
4045
import org.powermock.core.classloader.annotations.PrepareForTest;
4146
import org.powermock.modules.junit4.PowerMockRunner;
@@ -66,13 +71,29 @@ public class QueryManagerImplTest {
6671
public static final long USER_ID = 1;
6772
public static final long ACCOUNT_ID = 1;
6873

74+
@Spy
75+
@InjectMocks
76+
private QueryManagerImpl queryManagerImplSpy = new QueryManagerImpl();
77+
6978
@Mock
7079
EntityManager entityManager;
80+
7181
@Mock
7282
AccountManager accountManager;
83+
7384
@Mock
7485
EventJoinDao eventJoinDao;
7586

87+
@Mock
88+
Account accountMock;
89+
90+
@Mock
91+
TemplateJoinDao templateJoinDaoMock;
92+
93+
@Mock
94+
SearchCriteria searchCriteriaMock;
95+
96+
7697
private AccountVO account;
7798
private UserVO user;
7899

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

0 commit comments

Comments
 (0)