Skip to content

Commit 0fc8748

Browse files
winterhazelwinterhazel
committed
Fix ISOs and templates listing pagination
1 parent c3aeba1 commit 0fc8748

File tree

4 files changed

+149
-19
lines changed

4 files changed

+149
-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: 83 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;
@@ -48,10 +50,13 @@
4850
import org.mockito.Mock;
4951
import org.mockito.MockedStatic;
5052
import org.mockito.Mockito;
53+
import org.mockito.Spy;
5154
import org.mockito.junit.MockitoJUnitRunner;
5255

5356
import java.util.ArrayList;
57+
import java.util.HashSet;
5458
import java.util.List;
59+
import java.util.Set;
5560
import java.util.UUID;
5661

5762
import static org.mockito.Mockito.when;
@@ -61,13 +66,28 @@ public class QueryManagerImplTest {
6166
public static final long USER_ID = 1;
6267
public static final long ACCOUNT_ID = 1;
6368

69+
@Spy
70+
@InjectMocks
71+
private QueryManagerImpl queryManagerImplSpy = new QueryManagerImpl();
72+
6473
@Mock
6574
EntityManager entityManager;
75+
6676
@Mock
6777
AccountManager accountManager;
78+
6879
@Mock
6980
EventJoinDao eventJoinDao;
7081

82+
@Mock
83+
Account accountMock;
84+
85+
@Mock
86+
TemplateJoinDao templateJoinDaoMock;
87+
88+
@Mock
89+
SearchCriteria searchCriteriaMock;
90+
7191
private AccountVO account;
7292
private UserVO user;
7393

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

0 commit comments

Comments
 (0)