From e94c1e20edf9c33b97c66304e8e61b266ecf4969 Mon Sep 17 00:00:00 2001 From: Sina Kashipazha Date: Mon, 8 Mar 2021 16:13:12 +0100 Subject: [PATCH 01/16] Added configuration and Integration test to restrict public template access. --- .../com/cloud/template/TemplateManager.java | 4 + .../java/com/cloud/acl/DomainChecker.java | 11 + .../com/cloud/api/query/QueryManagerImpl.java | 98 ++- .../cloud/template/TemplateManagerImpl.java | 2 +- .../test_template_access_across_domains.py | 592 ++++++++++++++++++ 5 files changed, 682 insertions(+), 25 deletions(-) create mode 100644 test/integration/component/test_template_access_across_domains.py diff --git a/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java b/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java index a3d9c1b79f1a..0d7077364cfd 100644 --- a/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java +++ b/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java @@ -41,6 +41,7 @@ public interface TemplateManager { static final String AllowPublicUserTemplatesCK = "allow.public.user.templates"; static final String TemplatePreloaderPoolSizeCK = "template.preloader.pool.size"; + static final String RestrictPublicTemplateAccessToDomainCK = "restrict.public.template.access.to.domain"; static final ConfigKey AllowPublicUserTemplates = new ConfigKey("Advanced", Boolean.class, AllowPublicUserTemplatesCK, "true", "If false, users will not be able to create public templates.", true, ConfigKey.Scope.Account); @@ -48,6 +49,9 @@ public interface TemplateManager { static final ConfigKey TemplatePreloaderPoolSize = new ConfigKey("Advanced", Integer.class, TemplatePreloaderPoolSizeCK, "8", "Size of the TemplateManager threadpool", false, ConfigKey.Scope.Global); + static final ConfigKey RestrictPublicTemplateAccessToDomain = new ConfigKey<>("Advanced", Boolean.class, RestrictPublicTemplateAccessToDomainCK, "false", + "If true, the public template wouldn't share between domains", true, ConfigKey.Scope.Global); + static final String VMWARE_TOOLS_ISO = "vmware-tools.iso"; static final String XS_TOOLS_ISO = "xs-tools.iso"; diff --git a/server/src/main/java/com/cloud/acl/DomainChecker.java b/server/src/main/java/com/cloud/acl/DomainChecker.java index 387535dc19c2..fd971c0a5ce5 100644 --- a/server/src/main/java/com/cloud/acl/DomainChecker.java +++ b/server/src/main/java/com/cloud/acl/DomainChecker.java @@ -55,6 +55,7 @@ import com.cloud.service.dao.ServiceOfferingDetailsDao; import com.cloud.storage.LaunchPermissionVO; import com.cloud.storage.dao.LaunchPermissionDao; +import com.cloud.template.TemplateManager; import com.cloud.template.VirtualMachineTemplate; import com.cloud.user.Account; import com.cloud.user.AccountService; @@ -167,6 +168,16 @@ public boolean checkAccess(Account caller, ControlledEntity entity, AccessType a throw new PermissionDeniedException("Domain Admin and regular users can modify only their own Public templates"); } } + } else if (TemplateManager.RestrictPublicTemplateAccessToDomain.value() && caller.getType() != Account.ACCOUNT_TYPE_ADMIN) { // public template can be used by other accounts in the same domain or in sub-domains, and domain admin of parent domains + if (caller.getDomainId() != owner.getDomainId() && !_domainDao.isChildDomain(owner.getDomainId(), caller.getDomainId())) { + if (caller.getType() == Account.ACCOUNT_TYPE_NORMAL || caller.getType() == Account.ACCOUNT_TYPE_PROJECT) { + throw new PermissionDeniedException(caller + "is not allowed to access the template " + template); + } else if (caller.getType() == Account.ACCOUNT_TYPE_DOMAIN_ADMIN || caller.getType() == Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN) { + if (!_domainDao.isChildDomain(caller.getDomainId(), owner.getDomainId())) { + throw new PermissionDeniedException(caller + "is not allowed to access the template " + template); + } + } + } } } 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 e009aaaffd89..eca74522287c 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -232,6 +232,7 @@ import com.cloud.storage.dao.VolumeDao; import com.cloud.tags.ResourceTagVO; import com.cloud.tags.dao.ResourceTagDao; +import com.cloud.template.TemplateManager; import com.cloud.template.VirtualMachineTemplate.State; import com.cloud.template.VirtualMachineTemplate.TemplateFilter; import com.cloud.user.Account; @@ -3567,21 +3568,48 @@ private Pair, Integer> searchForTemplatesInternal(ListTempl List permittedAccountIds = new ArrayList(); Ternary domainIdRecursiveListProject = new Ternary(cmd.getDomainId(), cmd.isRecursive(), null); _accountMgr.buildACLSearchParameters(caller, id, cmd.getAccountName(), cmd.getProjectId(), permittedAccountIds, domainIdRecursiveListProject, listAll, false); + Long domainId = domainIdRecursiveListProject.first(); ListProjectResourcesCriteria listProjectResourcesCriteria = domainIdRecursiveListProject.third(); List permittedAccounts = new ArrayList(); for (Long accountId : permittedAccountIds) { permittedAccounts.add(_accountMgr.getAccount(accountId)); } - boolean showDomr = ((templateFilter != TemplateFilter.selfexecutable) && (templateFilter != TemplateFilter.featured)); + boolean showDomr = ((templateFilter != TemplateFilter.selfexecutable) && (templateFilter != TemplateFilter.featured) && _accountMgr.isRootAdmin(caller.getId())); HypervisorType hypervisorType = HypervisorType.getType(cmd.getHypervisor()); return searchForTemplatesInternal(id, cmd.getTemplateName(), cmd.getKeyword(), templateFilter, false, null, cmd.getPageSizeVal(), cmd.getStartIndex(), cmd.getZoneId(), hypervisorType, - showDomr, cmd.listInReadyState(), permittedAccounts, caller, listProjectResourcesCriteria, tags, showRemovedTmpl, cmd.getIds(), parentTemplateId, cmd.getShowUnique()); + showDomr, cmd.listInReadyState(), permittedAccounts, caller, domainId, listProjectResourcesCriteria, tags, showRemovedTmpl, cmd.getIds(), parentTemplateId, cmd.getShowUnique()); + } + + private DomainVO getDomainOf(TemplateFilter templateFilter, Account account ) { + boolean publicTemplates = (templateFilter == TemplateFilter.featured || templateFilter == TemplateFilter.community || templateFilter == TemplateFilter.executable); + + // get all parent domain ID's all the way till root domain + //if template filter is featured, or community, all child domains should be included in search + if (publicTemplates) + return _domainDao.findById(Domain.ROOT_DOMAIN); + else + return _domainDao.findById(account.getDomainId()); + } + + private ArrayList getChildDomainIds(boolean isAdmin, TemplateFilter templateFilter, DomainVO domainTreeNode ){ + boolean publicTemplates = (templateFilter == TemplateFilter.featured || templateFilter == TemplateFilter.community || templateFilter == TemplateFilter.executable); + ArrayList domainIds = new ArrayList<>(); + + // get all child domain ID's + if (isAdmin || publicTemplates) { + List allChildDomains = _domainDao.findAllChildren(domainTreeNode.getPath(), domainTreeNode.getId()); + for (DomainVO childDomain : allChildDomains) { + domainIds.add(childDomain.getId()); + } + } + + return domainIds; } private Pair, Integer> searchForTemplatesInternal(Long templateId, String name, String keyword, TemplateFilter templateFilter, boolean isIso, Boolean bootable, Long pageSize, - Long startIndex, Long zoneId, HypervisorType hyperType, boolean showDomr, boolean onlyReady, List permittedAccounts, Account caller, + Long startIndex, Long zoneId, HypervisorType hyperType, boolean showDomr, boolean onlyReady, List permittedAccounts, Account caller, Long domainId, ListProjectResourcesCriteria listProjectResourcesCriteria, Map tags, boolean showRemovedTmpl, List ids, Long parentTemplateId, Boolean showUnique) { // check if zone is configured, if not, just return empty list @@ -3609,6 +3637,8 @@ private Pair, Integer> searchForTemplatesInternal(Long temp } SearchCriteria sc = sb.create(); + boolean restrictPublicTemplatesToDomain = TemplateManager.RestrictPublicTemplateAccessToDomain.value(); + // verify templateId parameter and specially handle it if (templateId != null) { template = _templateDao.findByIdIncludingRemoved(templateId); // Done for backward compatibility - Bug-5221 @@ -3636,6 +3666,8 @@ private Pair, Integer> searchForTemplatesInternal(Long temp // if template is not public, perform permission check here else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) { _accountMgr.checkAccess(caller, null, false, template); + } else if (template.isPublicTemplate()) { + _accountMgr.checkAccess(caller, null, false, template); } // if templateId is specified, then we will just use the id to @@ -3646,6 +3678,8 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) DomainVO domain = null; if (!permittedAccounts.isEmpty()) { domain = _domainDao.findById(permittedAccounts.get(0).getDomainId()); + } else if (restrictPublicTemplatesToDomain && domainId != null) { + domain = _domainDao.findById(domainId); } else { domain = _domainDao.findById(Domain.ROOT_DOMAIN); } @@ -3670,16 +3704,11 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) if (!permittedAccounts.isEmpty()) { for (Account account : permittedAccounts) { permittedAccountIds.add(account.getId()); - boolean publicTemplates = (templateFilter == TemplateFilter.featured || templateFilter == TemplateFilter.community); - // get all parent domain ID's all the way till root domain - DomainVO domainTreeNode = null; - //if template filter is featured, or community, all child domains should be included in search - if (publicTemplates) { - domainTreeNode = _domainDao.findById(Domain.ROOT_DOMAIN); + DomainVO domainTreeNode = domain; - } else { - domainTreeNode = _domainDao.findById(account.getDomainId()); + if(! restrictPublicTemplatesToDomain) { + domainTreeNode = getDomainOf(templateFilter, account); } relatedDomainIds.add(domainTreeNode.getId()); while (domainTreeNode.getParent() != null) { @@ -3687,14 +3716,22 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) relatedDomainIds.add(domainTreeNode.getId()); } - // get all child domain ID's - if (_accountMgr.isAdmin(account.getId()) || publicTemplates) { - List allChildDomains = _domainDao.findAllChildren(domainTreeNode.getPath(), domainTreeNode.getId()); - for (DomainVO childDomain : allChildDomains) { - relatedDomainIds.add(childDomain.getId()); - } + boolean isAdmin = _accountMgr.isAdmin(account.getId()); + if(! restrictPublicTemplatesToDomain) { + relatedDomainIds.addAll(getChildDomainIds(isAdmin, templateFilter, domainTreeNode)); + } else if (isAdmin) { + relatedDomainIds.addAll(getChildDomainIds(isAdmin, templateFilter, domain)); } } + } else if (restrictPublicTemplatesToDomain && domainId != null) { + // templatefilter=all or domainid is passed, called by root admin/domain admin + DomainVO domainTreeNode = domain; + relatedDomainIds.add(domainTreeNode.getId()); + while (domainTreeNode.getParent() != null) { + domainTreeNode = _domainDao.findById(domainTreeNode.getParent()); + relatedDomainIds.add(domainTreeNode.getId()); + } + relatedDomainIds.addAll(getChildDomainIds(true, templateFilter, domain)); } // control different template filters @@ -3719,18 +3756,30 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) // only show templates shared by others sc.addAnd("sharedAccountId", SearchCriteria.Op.IN, permittedAccountIds.toArray()); } else if (templateFilter == TemplateFilter.executable) { + SearchCriteria scc2 = _templateJoinDao.createSearchCriteria(); + scc2.addAnd("publicTemplate", SearchCriteria.Op.EQ, true); + if (restrictPublicTemplatesToDomain) { + SearchCriteria scc3 = _templateJoinDao.createSearchCriteria(); + scc3.addOr("domainId", SearchCriteria.Op.IN, relatedDomainIds.toArray()); + scc3.addOr("domainId", SearchCriteria.Op.NULL); + scc2.addAnd("domainId", SearchCriteria.Op.SC, scc3); + } SearchCriteria scc = _templateJoinDao.createSearchCriteria(); - scc.addOr("publicTemplate", SearchCriteria.Op.EQ, true); + scc.addOr("publicTemplate", SearchCriteria.Op.SC, scc2); if (!permittedAccounts.isEmpty()) { scc.addOr("accountId", SearchCriteria.Op.IN, permittedAccountIds.toArray()); } sc.addAnd("publicTemplate", SearchCriteria.Op.SC, scc); } else if (templateFilter == TemplateFilter.all && caller.getType() != Account.Type.ADMIN) { SearchCriteria scc = _templateJoinDao.createSearchCriteria(); - scc.addOr("publicTemplate", SearchCriteria.Op.EQ, true); + scc.addOr("publicTemplate", SearchCriteria.Op.SC, scc2); if (listProjectResourcesCriteria == ListProjectResourcesCriteria.SkipProjectResources) { - scc.addOr("domainPath", SearchCriteria.Op.LIKE, _domainDao.findById(caller.getDomainId()).getPath() + "%"); + if (domainId != null) { + scc.addOr("domainPath", SearchCriteria.Op.LIKE, _domainDao.findById(domainId).getPath() + "%"); + } else { + scc.addOr("domainPath", SearchCriteria.Op.LIKE, _domainDao.findById(caller.getDomainId()).getPath() + "%"); + } } else { if (!permittedAccounts.isEmpty()) { scc.addOr("accountId", SearchCriteria.Op.IN, permittedAccountIds.toArray()); @@ -3741,13 +3790,13 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) } } - return templateChecks(isIso, hypers, tags, name, keyword, hyperType, onlyReady, bootable, zoneId, showDomr, + return templateChecks(isIso, hypers, tags, name, keyword, hyperType, onlyReady, bootable, zoneId, showDomr, domainId, showRemovedTmpl, parentTemplateId, showUnique, searchFilter, sc); } private Pair, Integer> templateChecks(boolean isIso, List hypers, Map tags, String name, String keyword, - HypervisorType hyperType, boolean onlyReady, Boolean bootable, Long zoneId, boolean showDomr, + HypervisorType hyperType, boolean onlyReady, Boolean bootable, Long zoneId, boolean showDomr, Long domainId, boolean showRemovedTmpl, Long parentTemplateId, Boolean showUnique, Filter searchFilter, SearchCriteria sc) { if (!isIso) { @@ -3808,7 +3857,7 @@ private Pair, Integer> templateChecks(boolean isIso, List, Integer> searchForIsosInternal(ListIsosCmd cm List permittedAccountIds = new ArrayList(); Ternary domainIdRecursiveListProject = new Ternary(cmd.getDomainId(), cmd.isRecursive(), null); _accountMgr.buildACLSearchParameters(caller, id, cmd.getAccountName(), cmd.getProjectId(), permittedAccountIds, domainIdRecursiveListProject, listAll, false); + Long domainId = domainIdRecursiveListProject.first(); ListProjectResourcesCriteria listProjectResourcesCriteria = domainIdRecursiveListProject.third(); List permittedAccounts = new ArrayList(); for (Long accountId : permittedAccountIds) { @@ -3918,7 +3968,7 @@ private Pair, Integer> searchForIsosInternal(ListIsosCmd cm HypervisorType hypervisorType = HypervisorType.getType(cmd.getHypervisor()); return searchForTemplatesInternal(cmd.getId(), cmd.getIsoName(), cmd.getKeyword(), isoFilter, true, cmd.isBootable(), cmd.getPageSizeVal(), cmd.getStartIndex(), cmd.getZoneId(), - hypervisorType, true, cmd.listInReadyState(), permittedAccounts, caller, listProjectResourcesCriteria, tags, showRemovedISO, null, null, cmd.getShowUnique()); + hypervisorType, true, cmd.listInReadyState(), permittedAccounts, caller, domainId, listProjectResourcesCriteria, tags, showRemovedISO, null, null, cmd.getShowUnique()); } @Override diff --git a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java index 527be5d8cd09..b707a9d1098b 100755 --- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java @@ -2255,7 +2255,7 @@ public String getConfigComponentName() { @Override public ConfigKey[] getConfigKeys() { - return new ConfigKey[] {AllowPublicUserTemplates, TemplatePreloaderPoolSize}; + return new ConfigKey[] {AllowPublicUserTemplates, TemplatePreloaderPoolSize, RestrictPublicTemplateAccessToDomain}; } public List getTemplateAdapters() { diff --git a/test/integration/component/test_template_access_across_domains.py b/test/integration/component/test_template_access_across_domains.py new file mode 100644 index 000000000000..2abb7a5f6cea --- /dev/null +++ b/test/integration/component/test_template_access_across_domains.py @@ -0,0 +1,592 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Import Local Modules +from nose.plugins.attrib import attr +from marvin.cloudstackTestCase import cloudstackTestCase, unittest +from marvin.cloudstackAPI import listZones, deleteTemplate, updateConfiguration +from marvin.lib.utils import (cleanup_resources) +from marvin.lib.base import (Account, + Domain, + Network, + NetworkOffering, + Template, + ServiceOffering, + VirtualMachine, + Snapshot, + Volume) +from marvin.lib.common import (get_domain, + get_zone, + get_template, + get_builtin_template_info) +# Import System modules +import time +import logging + +class TestTemplateAccessAcrossDomains(cloudstackTestCase): + @classmethod + def setUpClass(cls): + cls.testClient = super(TestTemplateAccessAcrossDomains, cls).getClsTestClient() + cls.apiclient = cls.testClient.getApiClient() + + cls.services = cls.testClient.getParsedTestDataConfig() + # Get Zone, Domain and templates + cls.domain = get_domain(cls.apiclient) + cls.zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests()) + cls.services['mode'] = cls.zone.networktype + cls.logger = logging.getLogger("TestRouterResources") + cls._cleanup = [] + cls.unsupportedHypervisor = False + cls.hypervisor = cls.testClient.getHypervisorInfo() + if cls.hypervisor.lower() in ['lxc']: + cls.unsupportedHypervisor = True + return + cls.services["virtual_machine"]["zoneid"] = cls.zone.id + + # Create new domain1 + cls.domain1 = Domain.create( + cls.apiclient, + services=cls.services["acl"]["domain1"], + parentdomainid=cls.domain.id) + + # Create account1 + cls.account1 = Account.create( + cls.apiclient, + cls.services["acl"]["accountD1"], + domainid=cls.domain1.id + ) + + # Create new sub-domain + cls.sub_domain = Domain.create( + cls.apiclient, + services=cls.services["acl"]["domain11"], + parentdomainid=cls.domain1.id) + + # Create account for sub-domain + cls.sub_account = Account.create( + cls.apiclient, + cls.services["acl"]["accountD11"], + domainid=cls.sub_domain.id + ) + + # Create new domain2 + cls.domain2 = Domain.create( + cls.apiclient, + services=cls.services["acl"]["domain2"], + parentdomainid=cls.domain.id) + + # Create account2 + cls.account2 = Account.create( + cls.apiclient, + cls.services["acl"]["accountD2"], + domainid=cls.domain2.id + ) + + cls.service_offering = ServiceOffering.create( + cls.apiclient, + cls.services["service_offering"] + ) + cls._cleanup.append(cls.service_offering) + if cls.hypervisor.lower() in ['kvm']: + # register template under ROOT domain + cls.root_template = Template.register(cls.apiclient, + cls.services["test_templates"]["kvm"], + zoneid=cls.zone.id, + domainid=cls.domain.id, + hypervisor=cls.hypervisor.lower()) + cls.services["test_templates"]["kvm"]["name"] = cls.account1.name + cls.template1 = Template.register(cls.apiclient, + cls.services["test_templates"]["kvm"], + zoneid=cls.zone.id, + account=cls.account1.name, + domainid=cls.domain1.id, + hypervisor=cls.hypervisor.lower()) + cls.services["test_templates"]["kvm"]["name"] = cls.sub_account.name + cls.sub_template = Template.register(cls.apiclient, + cls.services["test_templates"]["kvm"], + zoneid=cls.zone.id, + account=cls.sub_account.name, + domainid=cls.sub_domain.id, + hypervisor=cls.hypervisor.lower()) + cls.services["test_templates"]["kvm"]["name"] = cls.account2.name + cls.template2 = Template.register(cls.apiclient, + cls.services["test_templates"]["kvm"], + zoneid=cls.zone.id, + account=cls.account2.name, + domainid=cls.domain2.id, + hypervisor=cls.hypervisor.lower()) + + cls._cleanup.append(cls.root_template) + cls._cleanup.append(cls.template1) + cls._cleanup.append(cls.template2) + cls._cleanup.append(cls.sub_template) + else: + return + + cls._cleanup.append(cls.account1) + cls._cleanup.append(cls.account2) + cls._cleanup.append(cls.sub_account) + cls._cleanup.append(cls.sub_domain) + cls._cleanup.append(cls.domain1) + cls._cleanup.append(cls.domain2) + + @classmethod + def tearDownClass(cls): + try: + cls.apiclient = super( + TestTemplateAccessAcrossDomains, + cls).getClsTestClient().getApiClient() + # Cleanup resources used + cleanup_resources(cls.apiclient, cls._cleanup) + + except Exception as e: + raise Exception("Warning: Exception during cleanup : %s" % e) + + return + + @attr(tags=["advanced", "basic", "sg"], required_hardware="false") + def test_01_check_cross_domain_template_access(self): + """ + Verify that templates belonging to one domain should not be accessible + by other domains except for parent and ROOT domains + + Steps: + 1. Set global setting restrict.public.access.to.templates to true + 2. Make sure template of domain2 should not be accessible by domain1 + 3. Make sure template of domain1 should not be accessible by domain2 + 4. Make sure parent and ROOT domain can still access above templates + :return: + """ + + # Step 1 + self.update_configuration("true") + + self.validate_uploaded_template(self.apiclient, self.template1.id) + + # Step 2 + self.validate_template_ownership(self.template2, self.domain1, self.domain2, False) + + self.validate_uploaded_template(self.apiclient, self.template2.id) + + # Step 3 + self.validate_template_ownership(self.template1, self.domain2, self.domain1, False) + + # Make sure root domain can still access all subdomain templates + # Step 4 + self.validate_template_ownership(self.template1, self.domain, self.domain1, True) + self.validate_template_ownership(self.template2, self.domain, self.domain2, True) + + + @attr(tags=["advanced", "basic", "sg"], required_hardware="false") + def test_02_create_template(self): + """ + Verify that templates belonging to one domain can be accessible + by other domains by default + + Steps: + 1. Set global setting restrict.public.access.to.templates to false (default behavior) + 2. Make sure template of domain2 can be accessible by domain1 + 3. Make sure template of domain1 can be accessible by domain2 + 4. Make sure parent and ROOT domain can still access above templates + 5. Deploy virtual machine in domain1 using template from domain2 + 6. Make sure that virtual machine can be deployed and is in running state + :return: + """ + + # Step 1 + self.update_configuration("false") + + # Step 2 + self.validate_template_ownership(self.template2, self.domain1, self.domain2, True) + + # Step 3 + self.validate_template_ownership(self.template1, self.domain2, self.domain1, True) + + # Step 4 + # Make sure root domain can still access all subdomain templates + self.validate_template_ownership(self.template1, self.domain, self.domain1, True) + self.validate_template_ownership(self.template2, self.domain, self.domain2, True) + + # Step 5 + # Deploy new virtual machine using template + virtual_machine = VirtualMachine.create( + self.apiclient, + self.services["virtual_machine"], + templateid=self.template2.id, + accountid=self.account1.name, + domainid=self.account1.domainid, + serviceofferingid=self.service_offering.id, + ) + self.debug("creating an instance with template ID: %s" % self.template2.id) + vm_response = VirtualMachine.list(self.apiclient, + id=virtual_machine.id, + account=self.account1.name, + domainid=self.account1.domainid) + self.assertEqual( + isinstance(vm_response, list), + True, + "Check for list VMs response after VM deployment" + ) + # Verify VM response to check whether VM deployment was successful + self.assertNotEqual( + len(vm_response), + 0, + "Check VMs available in List VMs response" + ) + + # Step 6 + vm = vm_response[0] + self.assertEqual( + vm.state, + 'Running', + "Check the state of VM created from Template" + ) + + @attr(tags=["advanced", "basic", "sg"], required_hardware="false") + def test_03_check_subdomain_template_access(self): + """ + Verify that templates belonging to parent domain can be accessible + by sub domains + + Steps: + 1. Set global setting restrict.public.access.to.templates to true + 2. Make sure template of ROOT domain can be accessible by domain1 + 3. Make sure template of ROOT domain can be accessible by domain2 + """ + + # Step 1 + self.update_configuration("true") + # Make sure child domains can still access parent domain templates + self.validate_uploaded_template(self.apiclient, self.root_template.id) + + # Step 2 + self.validate_template_ownership(self.root_template, self.domain1, self.domain, True) + + # Step 3 + self.validate_template_ownership(self.root_template, self.domain2, self.domain, True) + + @attr(tags=["advanced", "basic", "sg"], required_hardware="false") + def test_04_check_non_public_template_access(self): + """ + Verify that non public templates belonging to one domain + should not be accessible by other domains by default + + Steps: + 1. Set global setting restrict.public.access.to.templates to true + 2. Change the permission level of "ispublic" of template to false + 3. Make sure other domains should not be able to access the template + 4. Make sure that ONLY ROOT domain can access the non public template + 5. Set global setting restrict.public.access.to.templates to false + 6. Repeat the steps 3 and 4 + """ + + # Step 1 + self.update_configuration("true") + + # Step 2 + self.template2.updatePermissions(self.apiclient, + ispublic="False") + + list_template_response = self.list_templates('all', self.domain2) + self.assertEqual( + isinstance(list_template_response, list), + True, + "Check list response returns a valid list" + ) + for template_response in list_template_response: + if template_response.id == self.template2.id: + break + + self.assertIsNotNone( + template_response, + "Check template %s failed" % self.template2.id + ) + self.assertEqual( + template_response.ispublic, + int(False), + "Check ispublic permission of template" + ) + + # Step 3 + # Other domains should not access non public template + self.validate_template_ownership(self.template2, self.domain1, self.domain2, False) + + # Step 4 + # Only ROOT domain can access non public templates of child domain + self.validate_template_ownership(self.template2, self.domain, self.domain2, True) + + # Step 5 + self.update_configuration("false") + + # Step 6 + self.validate_template_ownership(self.template2, self.domain1, self.domain2, False) + self.validate_template_ownership(self.template2, self.domain, self.domain2, True) + + @attr(tags=["advanced", "basic", "sg"], required_hardware="false") + def test_05_check_non_public_template_subdomain_access(self): + """ + Verify that non public templates belonging to ROOT domain + should not be accessible by sub domains by default + + Steps: + 1. Set global setting restrict.public.access.to.templates to true + 2. Change the permission level of "ispublic" of template to false + 3. Make sure other domains should not be able to access the template + 4. Make sure that ONLY ROOT domain can access the non public template + 5. Set global setting restrict.public.access.to.templates to false + 6. Repeat the steps 3 and 4 + """ + self.update_configuration("true") + self.root_template.updatePermissions(self.apiclient, + ispublic="False") + + list_template_response = self.list_templates('all', self.domain) + self.assertEqual( + isinstance(list_template_response, list), + True, + "Check list response returns a valid list" + ) + for template_response in list_template_response: + if template_response.id == self.root_template.id: + break + + self.assertIsNotNone( + template_response, + "Check template %s failed" % self.root_template.id + ) + self.assertEqual( + template_response.ispublic, + int(False), + "Check ispublic permission of template" + ) + + # Other domains should not access non public template + self.validate_template_ownership(self.root_template, self.domain1, self.domain, False) + # Only ROOT domain can access non public templates of child domain + self.validate_template_ownership(self.root_template, self.domain2, self.domain, False) + + self.update_configuration("false") + self.validate_template_ownership(self.root_template, self.domain1, self.domain2, False) + self.validate_template_ownership(self.root_template, self.domain2, self.domain2, False) + + @attr(tags=["advanced", "basic", "sg"], required_hardware="false") + def test_06_check_sub_public_template_sub_domain_access(self): + """ + Verify that non root admin sub-domains can access parents templates + + Steps: + 1. Set global setting restrict.public.access.to.templates to true + 2. Make sure that sub-domain account can access root templates + 3. Make sure that sub-domain account can access parent templates + 4. Make sure that ROOT domain can access the sub-domain template + 5. Make sure that sibling domain cannot access templates of sub-domain + """ + + self.root_template.updatePermissions(self.apiclient, + ispublic="True") + # Step 1 + self.update_configuration("true") + # Make sure child domains can still access parent domain templates + self.validate_uploaded_template(self.apiclient, self.sub_template.id) + + # Step 2 + self.validate_template_ownership(self.root_template, self.sub_domain, self.domain, True) + + # Step 3 + self.validate_template_ownership(self.template1, self.sub_domain, self.domain1, True) + + # Step 4 + self.validate_template_ownership(self.sub_template, self.domain, self.sub_domain, True) + + # Step 5 + self.validate_template_ownership(self.sub_template, self.domain2, self.sub_domain, False) + + @attr(tags=["advanced", "basic", "sg"], required_hardware="false") + def test_07_check_default_public_template_sub_domain_access(self): + """ + Verify that non root admin sub-domains can access parents templates by default + + Steps: + 1. Set global setting restrict.public.access.to.templates to false + 2. Make sure that sub-domain account can access root templates + 3. Make sure that sub-domain account can access parent templates + 4. Make sure that ROOT domain can access the sub-domain template + 5. Make sure that sibling domain cannot access templates of sub-domain + """ + + # Step 1 + self.update_configuration("false") + # Make sure child domains can still access parent domain templates + self.validate_uploaded_template(self.apiclient, self.sub_template.id) + + # Step 2 + self.validate_template_ownership(self.root_template, self.sub_domain, self.domain, True) + + # Step 3 + self.validate_template_ownership(self.template1, self.sub_domain, self.domain1, True) + + # Step 4 + self.validate_template_ownership(self.sub_template, self.domain, self.sub_domain, True) + + # Step 5 + self.validate_template_ownership(self.sub_template, self.domain2, self.sub_domain, True) + + @attr(tags=["advanced", "basic", "sg"], required_hardware="false") + def test_08_check_non_public_template_sub_domain_access(self): + """ + Verify that non public templates belonging to one domain + should not be accessible by other domains by default except ROOT domain + + Steps: + 1. Set global setting restrict.public.access.to.templates to true + 2. Change the permission level of "ispublic" of template1 to false + 3. Make sure other domains should not be able to access the template + 4. Make sure that ONLY ROOT domain can access the non public template + 5. Set global setting restrict.public.access.to.templates to false + 6. Repeat the steps 3 and 4 + """ + + # Step 1 + self.update_configuration("true") + + # Step 2 + self.template1.updatePermissions(self.apiclient, + ispublic="False") + + list_template_response = self.list_templates('all', self.domain1) + for template_response in list_template_response: + if template_response.id == self.template1.id: + break + + self.assertEqual( + isinstance(list_template_response, list), + True, + "Check list response returns a valid list" + ) + self.assertIsNotNone( + template_response, + "Check template %s failed" % self.template1.id + ) + self.assertEqual( + template_response.ispublic, + int(False), + "Check ispublic permission of template" + ) + + # Step 3 + # Other domains should not access non public template + self.validate_template_ownership(self.template1, self.domain2, self.domain1, False) + + # Even child domain should not access non public template + self.validate_template_ownership(self.template1, self.sub_domain, self.domain1, False) + + # Step 4 + # Only ROOT domain can access non public templates of child domain + self.validate_template_ownership(self.template1, self.domain, self.domain1, True) + + # Step 5 + self.update_configuration("false") + + # Step 6 + self.validate_template_ownership(self.template1, self.domain2, self.domain1, False) + self.validate_template_ownership(self.template1, self.sub_domain, self.domain1, False) + self.validate_template_ownership(self.template1, self.domain, self.domain1, True) + + def validate_uploaded_template(self, apiclient, template_id, retries=70, interval=5): + """Check if template download will finish in 1 minute""" + while retries > -1: + time.sleep(interval) + template_response = Template.list( + apiclient, + id=template_id, + zoneid=self.zone.id, + templatefilter='self' + ) + + if isinstance(template_response, list): + template = template_response[0] + if not hasattr(template, 'status') or not template or not template.status: + retries = retries - 1 + continue + if 'Failed' in template.status: + raise Exception( + "Failed to download template: status - %s" % + template.status) + + elif template.status == 'Download Complete' and template.isready: + return + + elif 'Downloaded' in template.status: + retries = retries - 1 + continue + + elif 'Installing' not in template.status: + if retries >= 0: + retries = retries - 1 + continue + raise Exception( + "Error in downloading template: status - %s" % + template.status) + + else: + retries = retries - 1 + raise Exception("Template download failed exception.") + + def list_templates(self, templatefilter, domain): + return Template.list( + self.apiclient, + templatefilter=templatefilter, + zoneid=self.zone.id, + domainid=domain.id) + + def validate_template_ownership(self, template, owner, nonowner, include_cross_domain_template): + """List the template belonging to domain which created it + Make sure that other domain can't access it. + """ + list_template_response = self.list_templates('all', owner) + if list_template_response is not None: + """If global setting is false then public templates of any domain should + be accessible by any other domain + """ + if include_cross_domain_template: + for temp in list_template_response: + if template.name == temp.name: + return + + raise Exception("Template %s belonging to domain %s should " + "be accessible by domain %s" + % (template.name, nonowner.name, owner.name)) + else: + """If global setting is true then public templates of any domain should not + be accessible by any other domain except for root domain + """ + for temp in list_template_response: + if template.name == temp.name: + raise Exception("Template %s belonging to domain %s should " + "not be accessible by domain %s" + % (template.name, nonowner.name, owner.name)) + + def update_configuration(self, value): + """ + Function to update the global setting "restrict.public.access.to.templates" + :param value: + :return: + """ + update_configuration_cmd = updateConfiguration.updateConfigurationCmd() + update_configuration_cmd.name = "restrict.public.template.access.to.domain" + update_configuration_cmd.value = value + return self.apiclient.updateConfiguration(update_configuration_cmd) From b601a1bfdcfce7e149c04846df013e8ca7a30c00 Mon Sep 17 00:00:00 2001 From: Sina Kashipazha Date: Tue, 16 Mar 2021 15:10:12 +0100 Subject: [PATCH 02/16] Move settings to domain. --- .../main/java/org/apache/cloudstack/query/QueryService.java | 5 +++++ .../src/main/java/com/cloud/template/TemplateManager.java | 4 +--- server/src/main/java/com/cloud/acl/DomainChecker.java | 4 ++-- .../src/main/java/com/cloud/api/query/QueryManagerImpl.java | 6 +++--- .../main/java/com/cloud/template/TemplateManagerImpl.java | 2 +- 5 files changed, 12 insertions(+), 9 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/query/QueryService.java b/api/src/main/java/org/apache/cloudstack/query/QueryService.java index bb418f98408f..a6c4f41fa4c2 100644 --- a/api/src/main/java/org/apache/cloudstack/query/QueryService.java +++ b/api/src/main/java/org/apache/cloudstack/query/QueryService.java @@ -90,6 +90,8 @@ */ public interface QueryService { + static final String RestrictPublicTemplateAccessToDomainCK = "restrict.public.template.access.to.domain"; + // Config keys ConfigKey AllowUserViewDestroyedVM = new ConfigKey<>("Advanced", Boolean.class, "allow.user.view.destroyed.vm", "false", "Determines whether users can view their destroyed or expunging vm ", true, ConfigKey.Scope.Account); @@ -111,6 +113,9 @@ public interface QueryService { "allow.user.view.all.domain.accounts", "false", "Determines whether users can view all user accounts within the same domain", true, ConfigKey.Scope.Domain); + static final ConfigKey RestrictPublicTemplateAccessToDomain = new ConfigKey<>("Advanced", Boolean.class, RestrictPublicTemplateAccessToDomainCK, "false", + "If true, other domains public templates will not visible in this domain.", true, ConfigKey.Scope.Domain); + ListResponse searchForUsers(ListUsersCmd cmd) throws PermissionDeniedException; ListResponse searchForUsers(Long domainId, boolean recursive) throws PermissionDeniedException; diff --git a/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java b/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java index 0d7077364cfd..f8deb46c7e15 100644 --- a/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java +++ b/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java @@ -41,7 +41,6 @@ public interface TemplateManager { static final String AllowPublicUserTemplatesCK = "allow.public.user.templates"; static final String TemplatePreloaderPoolSizeCK = "template.preloader.pool.size"; - static final String RestrictPublicTemplateAccessToDomainCK = "restrict.public.template.access.to.domain"; static final ConfigKey AllowPublicUserTemplates = new ConfigKey("Advanced", Boolean.class, AllowPublicUserTemplatesCK, "true", "If false, users will not be able to create public templates.", true, ConfigKey.Scope.Account); @@ -49,8 +48,7 @@ public interface TemplateManager { static final ConfigKey TemplatePreloaderPoolSize = new ConfigKey("Advanced", Integer.class, TemplatePreloaderPoolSizeCK, "8", "Size of the TemplateManager threadpool", false, ConfigKey.Scope.Global); - static final ConfigKey RestrictPublicTemplateAccessToDomain = new ConfigKey<>("Advanced", Boolean.class, RestrictPublicTemplateAccessToDomainCK, "false", - "If true, the public template wouldn't share between domains", true, ConfigKey.Scope.Global); + static final String VMWARE_TOOLS_ISO = "vmware-tools.iso"; static final String XS_TOOLS_ISO = "xs-tools.iso"; diff --git a/server/src/main/java/com/cloud/acl/DomainChecker.java b/server/src/main/java/com/cloud/acl/DomainChecker.java index fd971c0a5ce5..22cee47c0718 100644 --- a/server/src/main/java/com/cloud/acl/DomainChecker.java +++ b/server/src/main/java/com/cloud/acl/DomainChecker.java @@ -28,6 +28,7 @@ import org.apache.cloudstack.acl.SecurityChecker; import org.apache.cloudstack.affinity.AffinityGroup; import org.apache.cloudstack.context.CallContext; +import org.apache.cloudstack.query.QueryService; import org.apache.cloudstack.resourcedetail.dao.DiskOfferingDetailsDao; import org.apache.log4j.Logger; import org.springframework.stereotype.Component; @@ -55,7 +56,6 @@ import com.cloud.service.dao.ServiceOfferingDetailsDao; import com.cloud.storage.LaunchPermissionVO; import com.cloud.storage.dao.LaunchPermissionDao; -import com.cloud.template.TemplateManager; import com.cloud.template.VirtualMachineTemplate; import com.cloud.user.Account; import com.cloud.user.AccountService; @@ -168,7 +168,7 @@ public boolean checkAccess(Account caller, ControlledEntity entity, AccessType a throw new PermissionDeniedException("Domain Admin and regular users can modify only their own Public templates"); } } - } else if (TemplateManager.RestrictPublicTemplateAccessToDomain.value() && caller.getType() != Account.ACCOUNT_TYPE_ADMIN) { // public template can be used by other accounts in the same domain or in sub-domains, and domain admin of parent domains + } else if (QueryService.RestrictPublicTemplateAccessToDomain.valueIn(caller.getDomainId()) && caller.getType() != Account.ACCOUNT_TYPE_ADMIN) { // public template can be used by other accounts in the same domain or in sub-domains, and domain admin of parent domains if (caller.getDomainId() != owner.getDomainId() && !_domainDao.isChildDomain(owner.getDomainId(), caller.getDomainId())) { if (caller.getType() == Account.ACCOUNT_TYPE_NORMAL || caller.getType() == Account.ACCOUNT_TYPE_PROJECT) { throw new PermissionDeniedException(caller + "is not allowed to access the template " + template); 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 eca74522287c..732d4ce43d1e 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -232,7 +232,6 @@ import com.cloud.storage.dao.VolumeDao; import com.cloud.tags.ResourceTagVO; import com.cloud.tags.dao.ResourceTagDao; -import com.cloud.template.TemplateManager; import com.cloud.template.VirtualMachineTemplate.State; import com.cloud.template.VirtualMachineTemplate.TemplateFilter; import com.cloud.user.Account; @@ -3637,7 +3636,7 @@ private Pair, Integer> searchForTemplatesInternal(Long temp } SearchCriteria sc = sb.create(); - boolean restrictPublicTemplatesToDomain = TemplateManager.RestrictPublicTemplateAccessToDomain.value(); + boolean restrictPublicTemplatesToDomain = QueryService.RestrictPublicTemplateAccessToDomain.valueIn(caller.getDomainId()); // verify templateId parameter and specially handle it if (templateId != null) { @@ -4407,6 +4406,7 @@ public String getConfigComponentName() { @Override public ConfigKey[] getConfigKeys() { - return new ConfigKey[] {AllowUserViewDestroyedVM, UserVMDeniedDetails, UserVMReadOnlyDetails, SortKeyAscending, AllowUserViewAllDomainAccounts}; + return new ConfigKey[] {AllowUserViewDestroyedVM, UserVMDeniedDetails, UserVMReadOnlyDetails, SortKeyAscending, + AllowUserViewAllDomainAccounts, RestrictPublicTemplateAccessToDomain}; } } diff --git a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java index b707a9d1098b..527be5d8cd09 100755 --- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java @@ -2255,7 +2255,7 @@ public String getConfigComponentName() { @Override public ConfigKey[] getConfigKeys() { - return new ConfigKey[] {AllowPublicUserTemplates, TemplatePreloaderPoolSize, RestrictPublicTemplateAccessToDomain}; + return new ConfigKey[] {AllowPublicUserTemplates, TemplatePreloaderPoolSize}; } public List getTemplateAdapters() { From acfcd7eefcc474d43743be770a7cc1352a9029ca Mon Sep 17 00:00:00 2001 From: Sina Kashipazha Date: Tue, 28 Sep 2021 11:49:12 +0200 Subject: [PATCH 03/16] Updated integration test. --- .../test_template_access_across_domains.py | 109 ++++++++++++------ 1 file changed, 72 insertions(+), 37 deletions(-) diff --git a/test/integration/component/test_template_access_across_domains.py b/test/integration/component/test_template_access_across_domains.py index 2abb7a5f6cea..e564e824dc09 100644 --- a/test/integration/component/test_template_access_across_domains.py +++ b/test/integration/component/test_template_access_across_domains.py @@ -18,7 +18,10 @@ # Import Local Modules from nose.plugins.attrib import attr from marvin.cloudstackTestCase import cloudstackTestCase, unittest -from marvin.cloudstackAPI import listZones, deleteTemplate, updateConfiguration +from marvin.cloudstackAPI import (listZones, + deleteTemplate, + listConfigurations, + updateConfiguration) from marvin.lib.utils import (cleanup_resources) from marvin.lib.base import (Account, Domain, @@ -62,6 +65,7 @@ def setUpClass(cls): cls.apiclient, services=cls.services["acl"]["domain1"], parentdomainid=cls.domain.id) + cls._cleanup.append(cls.domain1) # Create account1 cls.account1 = Account.create( @@ -69,12 +73,14 @@ def setUpClass(cls): cls.services["acl"]["accountD1"], domainid=cls.domain1.id ) + cls._cleanup.append(cls.account1) # Create new sub-domain cls.sub_domain = Domain.create( cls.apiclient, services=cls.services["acl"]["domain11"], parentdomainid=cls.domain1.id) + cls._cleanup.append(cls.sub_domain) # Create account for sub-domain cls.sub_account = Account.create( @@ -82,12 +88,14 @@ def setUpClass(cls): cls.services["acl"]["accountD11"], domainid=cls.sub_domain.id ) + cls._cleanup.append(cls.sub_account) # Create new domain2 cls.domain2 = Domain.create( cls.apiclient, services=cls.services["acl"]["domain2"], parentdomainid=cls.domain.id) + cls._cleanup.append(cls.domain2) # Create account2 cls.account2 = Account.create( @@ -95,6 +103,7 @@ def setUpClass(cls): cls.services["acl"]["accountD2"], domainid=cls.domain2.id ) + cls._cleanup.append(cls.account2) cls.service_offering = ServiceOffering.create( cls.apiclient, @@ -108,6 +117,8 @@ def setUpClass(cls): zoneid=cls.zone.id, domainid=cls.domain.id, hypervisor=cls.hypervisor.lower()) + cls.root_template.download(cls.apiclient) + cls._cleanup.append(cls.root_template) cls.services["test_templates"]["kvm"]["name"] = cls.account1.name cls.template1 = Template.register(cls.apiclient, cls.services["test_templates"]["kvm"], @@ -115,6 +126,8 @@ def setUpClass(cls): account=cls.account1.name, domainid=cls.domain1.id, hypervisor=cls.hypervisor.lower()) + cls.template1.download(cls.apiclient) + cls._cleanup.append(cls.template1) cls.services["test_templates"]["kvm"]["name"] = cls.sub_account.name cls.sub_template = Template.register(cls.apiclient, cls.services["test_templates"]["kvm"], @@ -122,40 +135,39 @@ def setUpClass(cls): account=cls.sub_account.name, domainid=cls.sub_domain.id, hypervisor=cls.hypervisor.lower()) - cls.services["test_templates"]["kvm"]["name"] = cls.account2.name + cls.sub_template.download(cls.apiclient) + cls._cleanup.append(cls.sub_template) cls.template2 = Template.register(cls.apiclient, cls.services["test_templates"]["kvm"], zoneid=cls.zone.id, account=cls.account2.name, domainid=cls.domain2.id, hypervisor=cls.hypervisor.lower()) - - cls._cleanup.append(cls.root_template) - cls._cleanup.append(cls.template1) + cls.template2.download(cls.apiclient) cls._cleanup.append(cls.template2) - cls._cleanup.append(cls.sub_template) else: return - cls._cleanup.append(cls.account1) - cls._cleanup.append(cls.account2) - cls._cleanup.append(cls.sub_account) - cls._cleanup.append(cls.sub_domain) - cls._cleanup.append(cls.domain1) - cls._cleanup.append(cls.domain2) - @classmethod def tearDownClass(cls): - try: - cls.apiclient = super( - TestTemplateAccessAcrossDomains, - cls).getClsTestClient().getApiClient() - # Cleanup resources used - cleanup_resources(cls.apiclient, cls._cleanup) + super(TestTemplateAccessAcrossDomains, cls).tearDownClass() + + def setUp(self): + self.apiclient = self.testClient.getApiClient() + self.domain1_config = self.get_restrict_template_configuration(self.domain1.id) + self.domain2_config = self.get_restrict_template_configuration(self.domain2.id) + self.sub_domain_config = self.get_restrict_template_configuration(self.sub_domain.id) + self.cleanup = [] + return + def tearDown(self): + try: + self.update_restrict_template_configuration(self.domain1.id, self.domain1_config) + self.update_restrict_template_configuration(self.domain2.id, self.domain2_config) + self.update_restrict_template_configuration(self.sub_domain.id, self.sub_domain_config) + cleanup_resources(self.apiclient, self.cleanup) except Exception as e: raise Exception("Warning: Exception during cleanup : %s" % e) - return @attr(tags=["advanced", "basic", "sg"], required_hardware="false") @@ -173,7 +185,8 @@ def test_01_check_cross_domain_template_access(self): """ # Step 1 - self.update_configuration("true") + self.update_restrict_template_configuration(self.domain1.id, "true") + self.update_restrict_template_configuration(self.domain2.id, "true") self.validate_uploaded_template(self.apiclient, self.template1.id) @@ -208,7 +221,8 @@ def test_02_create_template(self): """ # Step 1 - self.update_configuration("false") + self.update_restrict_template_configuration(self.domain1.id, "false") + self.update_restrict_template_configuration(self.domain2.id, "false") # Step 2 self.validate_template_ownership(self.template2, self.domain1, self.domain2, True) @@ -223,7 +237,7 @@ def test_02_create_template(self): # Step 5 # Deploy new virtual machine using template - virtual_machine = VirtualMachine.create( + self.virtual_machine = VirtualMachine.create( self.apiclient, self.services["virtual_machine"], templateid=self.template2.id, @@ -231,9 +245,10 @@ def test_02_create_template(self): domainid=self.account1.domainid, serviceofferingid=self.service_offering.id, ) + self.cleanup.append(self.virtual_machine) self.debug("creating an instance with template ID: %s" % self.template2.id) vm_response = VirtualMachine.list(self.apiclient, - id=virtual_machine.id, + id=self.virtual_machine.id, account=self.account1.name, domainid=self.account1.domainid) self.assertEqual( @@ -269,7 +284,8 @@ def test_03_check_subdomain_template_access(self): """ # Step 1 - self.update_configuration("true") + self.update_restrict_template_configuration(self.domain1.id, "true") + self.update_restrict_template_configuration(self.domain2.id, "true") # Make sure child domains can still access parent domain templates self.validate_uploaded_template(self.apiclient, self.root_template.id) @@ -295,7 +311,8 @@ def test_04_check_non_public_template_access(self): """ # Step 1 - self.update_configuration("true") + self.update_restrict_template_configuration(self.domain1.id, "true") + self.update_restrict_template_configuration(self.domain2.id, "true") # Step 2 self.template2.updatePermissions(self.apiclient, @@ -330,7 +347,8 @@ def test_04_check_non_public_template_access(self): self.validate_template_ownership(self.template2, self.domain, self.domain2, True) # Step 5 - self.update_configuration("false") + self.update_restrict_template_configuration(self.domain1.id, "false") + self.update_restrict_template_configuration(self.domain2.id, "false") # Step 6 self.validate_template_ownership(self.template2, self.domain1, self.domain2, False) @@ -350,7 +368,8 @@ def test_05_check_non_public_template_subdomain_access(self): 5. Set global setting restrict.public.access.to.templates to false 6. Repeat the steps 3 and 4 """ - self.update_configuration("true") + self.update_restrict_template_configuration(self.domain1.id, "true") + self.update_restrict_template_configuration(self.domain2.id, "true") self.root_template.updatePermissions(self.apiclient, ispublic="False") @@ -379,7 +398,8 @@ def test_05_check_non_public_template_subdomain_access(self): # Only ROOT domain can access non public templates of child domain self.validate_template_ownership(self.root_template, self.domain2, self.domain, False) - self.update_configuration("false") + self.update_restrict_template_configuration(self.domain1.id, "false") + self.update_restrict_template_configuration(self.domain2.id, "false") self.validate_template_ownership(self.root_template, self.domain1, self.domain2, False) self.validate_template_ownership(self.root_template, self.domain2, self.domain2, False) @@ -399,7 +419,8 @@ def test_06_check_sub_public_template_sub_domain_access(self): self.root_template.updatePermissions(self.apiclient, ispublic="True") # Step 1 - self.update_configuration("true") + self.update_restrict_template_configuration(self.domain1.id, "true") + self.update_restrict_template_configuration(self.domain2.id, "true") # Make sure child domains can still access parent domain templates self.validate_uploaded_template(self.apiclient, self.sub_template.id) @@ -429,7 +450,8 @@ def test_07_check_default_public_template_sub_domain_access(self): """ # Step 1 - self.update_configuration("false") + self.update_restrict_template_configuration(self.domain1.id, "false") + self.update_restrict_template_configuration(self.domain2.id, "false") # Make sure child domains can still access parent domain templates self.validate_uploaded_template(self.apiclient, self.sub_template.id) @@ -461,7 +483,8 @@ def test_08_check_non_public_template_sub_domain_access(self): """ # Step 1 - self.update_configuration("true") + self.update_restrict_template_configuration(self.domain1.id, "true") + self.update_restrict_template_configuration(self.domain2.id, "true") # Step 2 self.template1.updatePermissions(self.apiclient, @@ -499,7 +522,8 @@ def test_08_check_non_public_template_sub_domain_access(self): self.validate_template_ownership(self.template1, self.domain, self.domain1, True) # Step 5 - self.update_configuration("false") + self.update_restrict_template_configuration(self.domain1.id, "false") + self.update_restrict_template_configuration(self.domain2.id, "false") # Step 6 self.validate_template_ownership(self.template1, self.domain2, self.domain1, False) @@ -580,13 +604,24 @@ def validate_template_ownership(self, template, owner, nonowner, include_cross_d "not be accessible by domain %s" % (template.name, nonowner.name, owner.name)) - def update_configuration(self, value): + def get_restrict_template_configuration(self, domain_id): """ - Function to update the global setting "restrict.public.access.to.templates" - :param value: - :return: + Function to get the global setting "restrict.public.access.to.templates" for domain + """ + list_configurations_cmd = listConfigurations.listConfigurationsCmd() + list_configurations_cmd.name = "restrict.public.template.access.to.domain" + list_configurations_cmd.scopename = "domain" + list_configurations_cmd.scopeid = domain_id + response = self.apiclient.listConfigurations(list_configurations_cmd) + return response[0].value + + def update_restrict_template_configuration(self, domain_id, value): + """ + Function to update the global setting "restrict.public.access.to.templates" for domain """ update_configuration_cmd = updateConfiguration.updateConfigurationCmd() update_configuration_cmd.name = "restrict.public.template.access.to.domain" update_configuration_cmd.value = value + update_configuration_cmd.scopename = "domain" + update_configuration_cmd.scopeid = domain_id return self.apiclient.updateConfiguration(update_configuration_cmd) From d9a6f7a95859083328a90ab1b6bb279a57be6a3c Mon Sep 17 00:00:00 2001 From: Sina Kashipazha Date: Tue, 5 Oct 2021 12:57:24 +0200 Subject: [PATCH 04/16] Changed Config key's name and description. --- .../main/java/org/apache/cloudstack/query/QueryService.java | 6 ++---- server/src/main/java/com/cloud/acl/DomainChecker.java | 2 +- .../src/main/java/com/cloud/api/query/QueryManagerImpl.java | 4 ++-- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/query/QueryService.java b/api/src/main/java/org/apache/cloudstack/query/QueryService.java index a6c4f41fa4c2..a3d3efd79418 100644 --- a/api/src/main/java/org/apache/cloudstack/query/QueryService.java +++ b/api/src/main/java/org/apache/cloudstack/query/QueryService.java @@ -90,8 +90,6 @@ */ public interface QueryService { - static final String RestrictPublicTemplateAccessToDomainCK = "restrict.public.template.access.to.domain"; - // Config keys ConfigKey AllowUserViewDestroyedVM = new ConfigKey<>("Advanced", Boolean.class, "allow.user.view.destroyed.vm", "false", "Determines whether users can view their destroyed or expunging vm ", true, ConfigKey.Scope.Account); @@ -113,8 +111,8 @@ public interface QueryService { "allow.user.view.all.domain.accounts", "false", "Determines whether users can view all user accounts within the same domain", true, ConfigKey.Scope.Domain); - static final ConfigKey RestrictPublicTemplateAccessToDomain = new ConfigKey<>("Advanced", Boolean.class, RestrictPublicTemplateAccessToDomainCK, "false", - "If true, other domains public templates will not visible in this domain.", true, ConfigKey.Scope.Domain); + static final ConfigKey RestrictPublicTemplatesOfOtherDomains = new ConfigKey<>("Advanced", Boolean.class, "restrict.public.templates.of.other.domains", "false", + "If true, other domains' public templates will not be visible in this domain.", true, ConfigKey.Scope.Domain); ListResponse searchForUsers(ListUsersCmd cmd) throws PermissionDeniedException; diff --git a/server/src/main/java/com/cloud/acl/DomainChecker.java b/server/src/main/java/com/cloud/acl/DomainChecker.java index 22cee47c0718..e3c152d1c7c2 100644 --- a/server/src/main/java/com/cloud/acl/DomainChecker.java +++ b/server/src/main/java/com/cloud/acl/DomainChecker.java @@ -168,7 +168,7 @@ public boolean checkAccess(Account caller, ControlledEntity entity, AccessType a throw new PermissionDeniedException("Domain Admin and regular users can modify only their own Public templates"); } } - } else if (QueryService.RestrictPublicTemplateAccessToDomain.valueIn(caller.getDomainId()) && caller.getType() != Account.ACCOUNT_TYPE_ADMIN) { // public template can be used by other accounts in the same domain or in sub-domains, and domain admin of parent domains + } else if (QueryService.RestrictPublicTemplatesOfOtherDomains.valueIn(caller.getDomainId()) && caller.getType() != Account.ACCOUNT_TYPE_ADMIN) { // public template can be used by other accounts in the same domain or in sub-domains, and domain admin of parent domains if (caller.getDomainId() != owner.getDomainId() && !_domainDao.isChildDomain(owner.getDomainId(), caller.getDomainId())) { if (caller.getType() == Account.ACCOUNT_TYPE_NORMAL || caller.getType() == Account.ACCOUNT_TYPE_PROJECT) { throw new PermissionDeniedException(caller + "is not allowed to access the template " + template); 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 732d4ce43d1e..23ce2d38b52c 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -3636,7 +3636,7 @@ private Pair, Integer> searchForTemplatesInternal(Long temp } SearchCriteria sc = sb.create(); - boolean restrictPublicTemplatesToDomain = QueryService.RestrictPublicTemplateAccessToDomain.valueIn(caller.getDomainId()); + boolean restrictPublicTemplatesToDomain = QueryService.RestrictPublicTemplatesOfOtherDomains.valueIn(caller.getDomainId()); // verify templateId parameter and specially handle it if (templateId != null) { @@ -4407,6 +4407,6 @@ public String getConfigComponentName() { @Override public ConfigKey[] getConfigKeys() { return new ConfigKey[] {AllowUserViewDestroyedVM, UserVMDeniedDetails, UserVMReadOnlyDetails, SortKeyAscending, - AllowUserViewAllDomainAccounts, RestrictPublicTemplateAccessToDomain}; + AllowUserViewAllDomainAccounts, RestrictPublicTemplatesOfOtherDomains}; } } From a9489bfce5de640f3a312e640b52619ff7635fdd Mon Sep 17 00:00:00 2001 From: Sina Kashipazha Date: Tue, 5 Oct 2021 13:25:18 +0200 Subject: [PATCH 05/16] Justified the variable names and removed white spaces. --- .../org/apache/cloudstack/query/QueryService.java | 2 +- .../java/com/cloud/template/TemplateManager.java | 2 -- .../java/com/cloud/api/query/QueryManagerImpl.java | 12 ++++++------ 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/query/QueryService.java b/api/src/main/java/org/apache/cloudstack/query/QueryService.java index a3d3efd79418..2445ddd618bb 100644 --- a/api/src/main/java/org/apache/cloudstack/query/QueryService.java +++ b/api/src/main/java/org/apache/cloudstack/query/QueryService.java @@ -112,7 +112,7 @@ public interface QueryService { "Determines whether users can view all user accounts within the same domain", true, ConfigKey.Scope.Domain); static final ConfigKey RestrictPublicTemplatesOfOtherDomains = new ConfigKey<>("Advanced", Boolean.class, "restrict.public.templates.of.other.domains", "false", - "If true, other domains' public templates will not be visible in this domain.", true, ConfigKey.Scope.Domain); + "If true, the public templates of other domains will not be visible in this domain.", true, ConfigKey.Scope.Domain); ListResponse searchForUsers(ListUsersCmd cmd) throws PermissionDeniedException; diff --git a/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java b/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java index f8deb46c7e15..a3d9c1b79f1a 100644 --- a/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java +++ b/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java @@ -48,8 +48,6 @@ public interface TemplateManager { static final ConfigKey TemplatePreloaderPoolSize = new ConfigKey("Advanced", Integer.class, TemplatePreloaderPoolSizeCK, "8", "Size of the TemplateManager threadpool", false, ConfigKey.Scope.Global); - - static final String VMWARE_TOOLS_ISO = "vmware-tools.iso"; static final String XS_TOOLS_ISO = "xs-tools.iso"; 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 23ce2d38b52c..e156b7f3d36f 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -3636,7 +3636,7 @@ private Pair, Integer> searchForTemplatesInternal(Long temp } SearchCriteria sc = sb.create(); - boolean restrictPublicTemplatesToDomain = QueryService.RestrictPublicTemplatesOfOtherDomains.valueIn(caller.getDomainId()); + boolean restrictPublicTemplatesOfOtherDomains = QueryService.RestrictPublicTemplatesOfOtherDomains.valueIn(caller.getDomainId()); // verify templateId parameter and specially handle it if (templateId != null) { @@ -3677,7 +3677,7 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) DomainVO domain = null; if (!permittedAccounts.isEmpty()) { domain = _domainDao.findById(permittedAccounts.get(0).getDomainId()); - } else if (restrictPublicTemplatesToDomain && domainId != null) { + } else if (restrictPublicTemplatesOfOtherDomains && domainId != null) { domain = _domainDao.findById(domainId); } else { domain = _domainDao.findById(Domain.ROOT_DOMAIN); @@ -3706,7 +3706,7 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) DomainVO domainTreeNode = domain; - if(! restrictPublicTemplatesToDomain) { + if(! restrictPublicTemplatesOfOtherDomains) { domainTreeNode = getDomainOf(templateFilter, account); } relatedDomainIds.add(domainTreeNode.getId()); @@ -3716,13 +3716,13 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) } boolean isAdmin = _accountMgr.isAdmin(account.getId()); - if(! restrictPublicTemplatesToDomain) { + if(! restrictPublicTemplatesOfOtherDomains) { relatedDomainIds.addAll(getChildDomainIds(isAdmin, templateFilter, domainTreeNode)); } else if (isAdmin) { relatedDomainIds.addAll(getChildDomainIds(isAdmin, templateFilter, domain)); } } - } else if (restrictPublicTemplatesToDomain && domainId != null) { + } else if (restrictPublicTemplatesOfOtherDomains && domainId != null) { // templatefilter=all or domainid is passed, called by root admin/domain admin DomainVO domainTreeNode = domain; relatedDomainIds.add(domainTreeNode.getId()); @@ -3757,7 +3757,7 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) } else if (templateFilter == TemplateFilter.executable) { SearchCriteria scc2 = _templateJoinDao.createSearchCriteria(); scc2.addAnd("publicTemplate", SearchCriteria.Op.EQ, true); - if (restrictPublicTemplatesToDomain) { + if (restrictPublicTemplatesOfOtherDomains) { SearchCriteria scc3 = _templateJoinDao.createSearchCriteria(); scc3.addOr("domainId", SearchCriteria.Op.IN, relatedDomainIds.toArray()); scc3.addOr("domainId", SearchCriteria.Op.NULL); From 287653db0607afeadc931a7f635739651bc66e23 Mon Sep 17 00:00:00 2001 From: Sina Kashipazha Date: Mon, 8 Mar 2021 16:13:12 +0100 Subject: [PATCH 06/16] Added configuration and Integration test to restrict public template access. --- .../src/main/java/com/cloud/template/TemplateManager.java | 4 ++++ server/src/main/java/com/cloud/acl/DomainChecker.java | 7 ++++--- .../main/java/com/cloud/api/query/QueryManagerImpl.java | 5 ++++- .../main/java/com/cloud/template/TemplateManagerImpl.java | 2 +- .../component/test_template_access_across_domains.py | 1 - 5 files changed, 13 insertions(+), 6 deletions(-) diff --git a/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java b/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java index a3d9c1b79f1a..0d7077364cfd 100644 --- a/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java +++ b/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java @@ -41,6 +41,7 @@ public interface TemplateManager { static final String AllowPublicUserTemplatesCK = "allow.public.user.templates"; static final String TemplatePreloaderPoolSizeCK = "template.preloader.pool.size"; + static final String RestrictPublicTemplateAccessToDomainCK = "restrict.public.template.access.to.domain"; static final ConfigKey AllowPublicUserTemplates = new ConfigKey("Advanced", Boolean.class, AllowPublicUserTemplatesCK, "true", "If false, users will not be able to create public templates.", true, ConfigKey.Scope.Account); @@ -48,6 +49,9 @@ public interface TemplateManager { static final ConfigKey TemplatePreloaderPoolSize = new ConfigKey("Advanced", Integer.class, TemplatePreloaderPoolSizeCK, "8", "Size of the TemplateManager threadpool", false, ConfigKey.Scope.Global); + static final ConfigKey RestrictPublicTemplateAccessToDomain = new ConfigKey<>("Advanced", Boolean.class, RestrictPublicTemplateAccessToDomainCK, "false", + "If true, the public template wouldn't share between domains", true, ConfigKey.Scope.Global); + static final String VMWARE_TOOLS_ISO = "vmware-tools.iso"; static final String XS_TOOLS_ISO = "xs-tools.iso"; diff --git a/server/src/main/java/com/cloud/acl/DomainChecker.java b/server/src/main/java/com/cloud/acl/DomainChecker.java index e3c152d1c7c2..a5bb87856a78 100644 --- a/server/src/main/java/com/cloud/acl/DomainChecker.java +++ b/server/src/main/java/com/cloud/acl/DomainChecker.java @@ -56,6 +56,7 @@ import com.cloud.service.dao.ServiceOfferingDetailsDao; import com.cloud.storage.LaunchPermissionVO; import com.cloud.storage.dao.LaunchPermissionDao; +import com.cloud.template.TemplateManager; import com.cloud.template.VirtualMachineTemplate; import com.cloud.user.Account; import com.cloud.user.AccountService; @@ -168,11 +169,11 @@ public boolean checkAccess(Account caller, ControlledEntity entity, AccessType a throw new PermissionDeniedException("Domain Admin and regular users can modify only their own Public templates"); } } - } else if (QueryService.RestrictPublicTemplatesOfOtherDomains.valueIn(caller.getDomainId()) && caller.getType() != Account.ACCOUNT_TYPE_ADMIN) { // public template can be used by other accounts in the same domain or in sub-domains, and domain admin of parent domains + } else if (QueryService.RestrictPublicTemplatesOfOtherDomains.valueIn(caller.getDomainId()) && caller.getType() != Account.Type.ADMIN) { // public template can be used by other accounts in the same domain or in sub-domains, and domain admin of parent domains if (caller.getDomainId() != owner.getDomainId() && !_domainDao.isChildDomain(owner.getDomainId(), caller.getDomainId())) { - if (caller.getType() == Account.ACCOUNT_TYPE_NORMAL || caller.getType() == Account.ACCOUNT_TYPE_PROJECT) { + if (caller.getType() == Account.Type.NORMAL || caller.getType() == Account.Type.PROJECT) { throw new PermissionDeniedException(caller + "is not allowed to access the template " + template); - } else if (caller.getType() == Account.ACCOUNT_TYPE_DOMAIN_ADMIN || caller.getType() == Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN) { + } else if (caller.getType() == Account.Type.DOMAIN_ADMIN || caller.getType() == Account.Type.RESOURCE_DOMAIN_ADMIN) { if (!_domainDao.isChildDomain(caller.getDomainId(), owner.getDomainId())) { throw new PermissionDeniedException(caller + "is not allowed to access the template " + template); } 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 e156b7f3d36f..871a08b1b207 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -232,6 +232,7 @@ import com.cloud.storage.dao.VolumeDao; import com.cloud.tags.ResourceTagVO; import com.cloud.tags.dao.ResourceTagDao; +import com.cloud.template.TemplateManager; import com.cloud.template.VirtualMachineTemplate.State; import com.cloud.template.VirtualMachineTemplate.TemplateFilter; import com.cloud.user.Account; @@ -3667,6 +3668,8 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) _accountMgr.checkAccess(caller, null, false, template); } else if (template.isPublicTemplate()) { _accountMgr.checkAccess(caller, null, false, template); + } else if (template.isPublicTemplate()) { + _accountMgr.checkAccess(caller, null, false, template); } // if templateId is specified, then we will just use the id to @@ -3771,7 +3774,7 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) sc.addAnd("publicTemplate", SearchCriteria.Op.SC, scc); } else if (templateFilter == TemplateFilter.all && caller.getType() != Account.Type.ADMIN) { SearchCriteria scc = _templateJoinDao.createSearchCriteria(); - scc.addOr("publicTemplate", SearchCriteria.Op.SC, scc2); + scc.addOr("publicTemplate", SearchCriteria.Op.SC, scc); if (listProjectResourcesCriteria == ListProjectResourcesCriteria.SkipProjectResources) { if (domainId != null) { diff --git a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java index 527be5d8cd09..b707a9d1098b 100755 --- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java @@ -2255,7 +2255,7 @@ public String getConfigComponentName() { @Override public ConfigKey[] getConfigKeys() { - return new ConfigKey[] {AllowPublicUserTemplates, TemplatePreloaderPoolSize}; + return new ConfigKey[] {AllowPublicUserTemplates, TemplatePreloaderPoolSize, RestrictPublicTemplateAccessToDomain}; } public List getTemplateAdapters() { diff --git a/test/integration/component/test_template_access_across_domains.py b/test/integration/component/test_template_access_across_domains.py index e564e824dc09..0caf3a86f77c 100644 --- a/test/integration/component/test_template_access_across_domains.py +++ b/test/integration/component/test_template_access_across_domains.py @@ -187,7 +187,6 @@ def test_01_check_cross_domain_template_access(self): # Step 1 self.update_restrict_template_configuration(self.domain1.id, "true") self.update_restrict_template_configuration(self.domain2.id, "true") - self.validate_uploaded_template(self.apiclient, self.template1.id) # Step 2 From b689705ff246472553b88b9a37bb09271ae1a7d0 Mon Sep 17 00:00:00 2001 From: Sina Kashipazha Date: Tue, 16 Mar 2021 15:10:12 +0100 Subject: [PATCH 07/16] Move settings to domain. --- .../main/java/org/apache/cloudstack/query/QueryService.java | 2 ++ .../src/main/java/com/cloud/template/TemplateManager.java | 4 +--- server/src/main/java/com/cloud/acl/DomainChecker.java | 1 - .../src/main/java/com/cloud/api/query/QueryManagerImpl.java | 1 - .../src/main/java/com/cloud/template/TemplateManagerImpl.java | 2 +- 5 files changed, 4 insertions(+), 6 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/query/QueryService.java b/api/src/main/java/org/apache/cloudstack/query/QueryService.java index 2445ddd618bb..bc211383d516 100644 --- a/api/src/main/java/org/apache/cloudstack/query/QueryService.java +++ b/api/src/main/java/org/apache/cloudstack/query/QueryService.java @@ -90,6 +90,8 @@ */ public interface QueryService { + static final String RestrictPublicTemplateAccessToDomainCK = "restrict.public.template.access.to.domain"; + // Config keys ConfigKey AllowUserViewDestroyedVM = new ConfigKey<>("Advanced", Boolean.class, "allow.user.view.destroyed.vm", "false", "Determines whether users can view their destroyed or expunging vm ", true, ConfigKey.Scope.Account); diff --git a/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java b/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java index 0d7077364cfd..f8deb46c7e15 100644 --- a/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java +++ b/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java @@ -41,7 +41,6 @@ public interface TemplateManager { static final String AllowPublicUserTemplatesCK = "allow.public.user.templates"; static final String TemplatePreloaderPoolSizeCK = "template.preloader.pool.size"; - static final String RestrictPublicTemplateAccessToDomainCK = "restrict.public.template.access.to.domain"; static final ConfigKey AllowPublicUserTemplates = new ConfigKey("Advanced", Boolean.class, AllowPublicUserTemplatesCK, "true", "If false, users will not be able to create public templates.", true, ConfigKey.Scope.Account); @@ -49,8 +48,7 @@ public interface TemplateManager { static final ConfigKey TemplatePreloaderPoolSize = new ConfigKey("Advanced", Integer.class, TemplatePreloaderPoolSizeCK, "8", "Size of the TemplateManager threadpool", false, ConfigKey.Scope.Global); - static final ConfigKey RestrictPublicTemplateAccessToDomain = new ConfigKey<>("Advanced", Boolean.class, RestrictPublicTemplateAccessToDomainCK, "false", - "If true, the public template wouldn't share between domains", true, ConfigKey.Scope.Global); + static final String VMWARE_TOOLS_ISO = "vmware-tools.iso"; static final String XS_TOOLS_ISO = "xs-tools.iso"; diff --git a/server/src/main/java/com/cloud/acl/DomainChecker.java b/server/src/main/java/com/cloud/acl/DomainChecker.java index a5bb87856a78..069366ea0f30 100644 --- a/server/src/main/java/com/cloud/acl/DomainChecker.java +++ b/server/src/main/java/com/cloud/acl/DomainChecker.java @@ -56,7 +56,6 @@ import com.cloud.service.dao.ServiceOfferingDetailsDao; import com.cloud.storage.LaunchPermissionVO; import com.cloud.storage.dao.LaunchPermissionDao; -import com.cloud.template.TemplateManager; import com.cloud.template.VirtualMachineTemplate; import com.cloud.user.Account; import com.cloud.user.AccountService; 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 871a08b1b207..8de023d3a197 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -232,7 +232,6 @@ import com.cloud.storage.dao.VolumeDao; import com.cloud.tags.ResourceTagVO; import com.cloud.tags.dao.ResourceTagDao; -import com.cloud.template.TemplateManager; import com.cloud.template.VirtualMachineTemplate.State; import com.cloud.template.VirtualMachineTemplate.TemplateFilter; import com.cloud.user.Account; diff --git a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java index b707a9d1098b..527be5d8cd09 100755 --- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java @@ -2255,7 +2255,7 @@ public String getConfigComponentName() { @Override public ConfigKey[] getConfigKeys() { - return new ConfigKey[] {AllowPublicUserTemplates, TemplatePreloaderPoolSize, RestrictPublicTemplateAccessToDomain}; + return new ConfigKey[] {AllowPublicUserTemplates, TemplatePreloaderPoolSize}; } public List getTemplateAdapters() { From 309c40cb6941ff1fb23298b65c7a542e868a70bb Mon Sep 17 00:00:00 2001 From: Sina Kashipazha Date: Tue, 5 Oct 2021 12:57:24 +0200 Subject: [PATCH 08/16] Changed Config key's name and description. --- api/src/main/java/org/apache/cloudstack/query/QueryService.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/query/QueryService.java b/api/src/main/java/org/apache/cloudstack/query/QueryService.java index bc211383d516..2445ddd618bb 100644 --- a/api/src/main/java/org/apache/cloudstack/query/QueryService.java +++ b/api/src/main/java/org/apache/cloudstack/query/QueryService.java @@ -90,8 +90,6 @@ */ public interface QueryService { - static final String RestrictPublicTemplateAccessToDomainCK = "restrict.public.template.access.to.domain"; - // Config keys ConfigKey AllowUserViewDestroyedVM = new ConfigKey<>("Advanced", Boolean.class, "allow.user.view.destroyed.vm", "false", "Determines whether users can view their destroyed or expunging vm ", true, ConfigKey.Scope.Account); From eb9ba64a969e1daed7e6d7fcbc41205077639d38 Mon Sep 17 00:00:00 2001 From: Sina Kashipazha Date: Tue, 5 Oct 2021 13:25:18 +0200 Subject: [PATCH 09/16] Justified the variable names and removed white spaces. --- .../src/main/java/com/cloud/template/TemplateManager.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java b/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java index f8deb46c7e15..a3d9c1b79f1a 100644 --- a/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java +++ b/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java @@ -48,8 +48,6 @@ public interface TemplateManager { static final ConfigKey TemplatePreloaderPoolSize = new ConfigKey("Advanced", Integer.class, TemplatePreloaderPoolSizeCK, "8", "Size of the TemplateManager threadpool", false, ConfigKey.Scope.Global); - - static final String VMWARE_TOOLS_ISO = "vmware-tools.iso"; static final String XS_TOOLS_ISO = "xs-tools.iso"; From 1af0697a3203493f087b36237e073c144f892ed1 Mon Sep 17 00:00:00 2001 From: Sina Kashipazha Date: Fri, 18 Feb 2022 14:39:20 +0100 Subject: [PATCH 10/16] Moved configuration to domain scope. --- .../apache/cloudstack/query/QueryService.java | 4 +- .../com/cloud/api/query/QueryManagerImpl.java | 122 +++++++----------- 2 files changed, 49 insertions(+), 77 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/query/QueryService.java b/api/src/main/java/org/apache/cloudstack/query/QueryService.java index 2445ddd618bb..3ab597374dd5 100644 --- a/api/src/main/java/org/apache/cloudstack/query/QueryService.java +++ b/api/src/main/java/org/apache/cloudstack/query/QueryService.java @@ -111,8 +111,8 @@ public interface QueryService { "allow.user.view.all.domain.accounts", "false", "Determines whether users can view all user accounts within the same domain", true, ConfigKey.Scope.Domain); - static final ConfigKey RestrictPublicTemplatesOfOtherDomains = new ConfigKey<>("Advanced", Boolean.class, "restrict.public.templates.of.other.domains", "false", - "If true, the public templates of other domains will not be visible in this domain.", true, ConfigKey.Scope.Domain); + static final ConfigKey SharePublicTemplates = new ConfigKey<>("Advanced", Boolean.class, "share.public.templates", "true", + "If false, templates of this domain with not show up in the list templates.", true, ConfigKey.Scope.Domain); ListResponse searchForUsers(ListUsersCmd cmd) throws PermissionDeniedException; 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 8de023d3a197..4101f5f1c29f 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -26,6 +26,7 @@ import java.util.ListIterator; import java.util.Map; import java.util.Set; +import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -3567,48 +3568,21 @@ private Pair, Integer> searchForTemplatesInternal(ListTempl List permittedAccountIds = new ArrayList(); Ternary domainIdRecursiveListProject = new Ternary(cmd.getDomainId(), cmd.isRecursive(), null); _accountMgr.buildACLSearchParameters(caller, id, cmd.getAccountName(), cmd.getProjectId(), permittedAccountIds, domainIdRecursiveListProject, listAll, false); - Long domainId = domainIdRecursiveListProject.first(); ListProjectResourcesCriteria listProjectResourcesCriteria = domainIdRecursiveListProject.third(); List permittedAccounts = new ArrayList(); for (Long accountId : permittedAccountIds) { permittedAccounts.add(_accountMgr.getAccount(accountId)); } - boolean showDomr = ((templateFilter != TemplateFilter.selfexecutable) && (templateFilter != TemplateFilter.featured) && _accountMgr.isRootAdmin(caller.getId())); + boolean showDomr = ((templateFilter != TemplateFilter.selfexecutable) && (templateFilter != TemplateFilter.featured)); HypervisorType hypervisorType = HypervisorType.getType(cmd.getHypervisor()); return searchForTemplatesInternal(id, cmd.getTemplateName(), cmd.getKeyword(), templateFilter, false, null, cmd.getPageSizeVal(), cmd.getStartIndex(), cmd.getZoneId(), hypervisorType, - showDomr, cmd.listInReadyState(), permittedAccounts, caller, domainId, listProjectResourcesCriteria, tags, showRemovedTmpl, cmd.getIds(), parentTemplateId, cmd.getShowUnique()); - } - - private DomainVO getDomainOf(TemplateFilter templateFilter, Account account ) { - boolean publicTemplates = (templateFilter == TemplateFilter.featured || templateFilter == TemplateFilter.community || templateFilter == TemplateFilter.executable); - - // get all parent domain ID's all the way till root domain - //if template filter is featured, or community, all child domains should be included in search - if (publicTemplates) - return _domainDao.findById(Domain.ROOT_DOMAIN); - else - return _domainDao.findById(account.getDomainId()); - } - - private ArrayList getChildDomainIds(boolean isAdmin, TemplateFilter templateFilter, DomainVO domainTreeNode ){ - boolean publicTemplates = (templateFilter == TemplateFilter.featured || templateFilter == TemplateFilter.community || templateFilter == TemplateFilter.executable); - ArrayList domainIds = new ArrayList<>(); - - // get all child domain ID's - if (isAdmin || publicTemplates) { - List allChildDomains = _domainDao.findAllChildren(domainTreeNode.getPath(), domainTreeNode.getId()); - for (DomainVO childDomain : allChildDomains) { - domainIds.add(childDomain.getId()); - } - } - - return domainIds; + showDomr, cmd.listInReadyState(), permittedAccounts, caller, listProjectResourcesCriteria, tags, showRemovedTmpl, cmd.getIds(), parentTemplateId, cmd.getShowUnique()); } private Pair, Integer> searchForTemplatesInternal(Long templateId, String name, String keyword, TemplateFilter templateFilter, boolean isIso, Boolean bootable, Long pageSize, - Long startIndex, Long zoneId, HypervisorType hyperType, boolean showDomr, boolean onlyReady, List permittedAccounts, Account caller, Long domainId, + Long startIndex, Long zoneId, HypervisorType hyperType, boolean showDomr, boolean onlyReady, List permittedAccounts, Account caller, ListProjectResourcesCriteria listProjectResourcesCriteria, Map tags, boolean showRemovedTmpl, List ids, Long parentTemplateId, Boolean showUnique) { // check if zone is configured, if not, just return empty list @@ -3636,8 +3610,6 @@ private Pair, Integer> searchForTemplatesInternal(Long temp } SearchCriteria sc = sb.create(); - boolean restrictPublicTemplatesOfOtherDomains = QueryService.RestrictPublicTemplatesOfOtherDomains.valueIn(caller.getDomainId()); - // verify templateId parameter and specially handle it if (templateId != null) { template = _templateDao.findByIdIncludingRemoved(templateId); // Done for backward compatibility - Bug-5221 @@ -3667,8 +3639,6 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) _accountMgr.checkAccess(caller, null, false, template); } else if (template.isPublicTemplate()) { _accountMgr.checkAccess(caller, null, false, template); - } else if (template.isPublicTemplate()) { - _accountMgr.checkAccess(caller, null, false, template); } // if templateId is specified, then we will just use the id to @@ -3679,8 +3649,6 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) DomainVO domain = null; if (!permittedAccounts.isEmpty()) { domain = _domainDao.findById(permittedAccounts.get(0).getDomainId()); - } else if (restrictPublicTemplatesOfOtherDomains && domainId != null) { - domain = _domainDao.findById(domainId); } else { domain = _domainDao.findById(Domain.ROOT_DOMAIN); } @@ -3705,11 +3673,16 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) if (!permittedAccounts.isEmpty()) { for (Account account : permittedAccounts) { permittedAccountIds.add(account.getId()); + boolean publicTemplates = (templateFilter == TemplateFilter.featured || templateFilter == TemplateFilter.community); - DomainVO domainTreeNode = domain; + // get all parent domain ID's all the way till root domain + DomainVO domainTreeNode = null; + //if template filter is featured, or community, all child domains should be included in search + if (publicTemplates) { + domainTreeNode = _domainDao.findById(Domain.ROOT_DOMAIN); - if(! restrictPublicTemplatesOfOtherDomains) { - domainTreeNode = getDomainOf(templateFilter, account); + } else { + domainTreeNode = _domainDao.findById(account.getDomainId()); } relatedDomainIds.add(domainTreeNode.getId()); while (domainTreeNode.getParent() != null) { @@ -3717,22 +3690,14 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) relatedDomainIds.add(domainTreeNode.getId()); } - boolean isAdmin = _accountMgr.isAdmin(account.getId()); - if(! restrictPublicTemplatesOfOtherDomains) { - relatedDomainIds.addAll(getChildDomainIds(isAdmin, templateFilter, domainTreeNode)); - } else if (isAdmin) { - relatedDomainIds.addAll(getChildDomainIds(isAdmin, templateFilter, domain)); + // get all child domain ID's + if (_accountMgr.isAdmin(account.getId()) || publicTemplates) { + List allChildDomains = _domainDao.findAllChildren(domainTreeNode.getPath(), domainTreeNode.getId()); + for (DomainVO childDomain : allChildDomains) { + relatedDomainIds.add(childDomain.getId()); + } } } - } else if (restrictPublicTemplatesOfOtherDomains && domainId != null) { - // templatefilter=all or domainid is passed, called by root admin/domain admin - DomainVO domainTreeNode = domain; - relatedDomainIds.add(domainTreeNode.getId()); - while (domainTreeNode.getParent() != null) { - domainTreeNode = _domainDao.findById(domainTreeNode.getParent()); - relatedDomainIds.add(domainTreeNode.getId()); - } - relatedDomainIds.addAll(getChildDomainIds(true, templateFilter, domain)); } // control different template filters @@ -3757,16 +3722,8 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) // only show templates shared by others sc.addAnd("sharedAccountId", SearchCriteria.Op.IN, permittedAccountIds.toArray()); } else if (templateFilter == TemplateFilter.executable) { - SearchCriteria scc2 = _templateJoinDao.createSearchCriteria(); - scc2.addAnd("publicTemplate", SearchCriteria.Op.EQ, true); - if (restrictPublicTemplatesOfOtherDomains) { - SearchCriteria scc3 = _templateJoinDao.createSearchCriteria(); - scc3.addOr("domainId", SearchCriteria.Op.IN, relatedDomainIds.toArray()); - scc3.addOr("domainId", SearchCriteria.Op.NULL); - scc2.addAnd("domainId", SearchCriteria.Op.SC, scc3); - } SearchCriteria scc = _templateJoinDao.createSearchCriteria(); - scc.addOr("publicTemplate", SearchCriteria.Op.SC, scc2); + scc.addOr("publicTemplate", SearchCriteria.Op.EQ, true); if (!permittedAccounts.isEmpty()) { scc.addOr("accountId", SearchCriteria.Op.IN, permittedAccountIds.toArray()); } @@ -3776,11 +3733,7 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) scc.addOr("publicTemplate", SearchCriteria.Op.SC, scc); if (listProjectResourcesCriteria == ListProjectResourcesCriteria.SkipProjectResources) { - if (domainId != null) { - scc.addOr("domainPath", SearchCriteria.Op.LIKE, _domainDao.findById(domainId).getPath() + "%"); - } else { - scc.addOr("domainPath", SearchCriteria.Op.LIKE, _domainDao.findById(caller.getDomainId()).getPath() + "%"); - } + scc.addOr("domainPath", SearchCriteria.Op.LIKE, _domainDao.findById(caller.getDomainId()).getPath() + "%"); } else { if (!permittedAccounts.isEmpty()) { scc.addOr("accountId", SearchCriteria.Op.IN, permittedAccountIds.toArray()); @@ -3791,13 +3744,13 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) } } - return templateChecks(isIso, hypers, tags, name, keyword, hyperType, onlyReady, bootable, zoneId, showDomr, domainId, + return templateChecks(isIso, hypers, tags, name, keyword, hyperType, onlyReady, bootable, zoneId, showDomr, caller, showRemovedTmpl, parentTemplateId, showUnique, searchFilter, sc); } private Pair, Integer> templateChecks(boolean isIso, List hypers, Map tags, String name, String keyword, - HypervisorType hyperType, boolean onlyReady, Boolean bootable, Long zoneId, boolean showDomr, Long domainId, + HypervisorType hyperType, boolean onlyReady, Boolean bootable, Long zoneId, boolean showDomr, Account caller, boolean showRemovedTmpl, Long parentTemplateId, Boolean showUnique, Filter searchFilter, SearchCriteria sc) { if (!isIso) { @@ -3858,7 +3811,7 @@ private Pair, Integer> templateChecks(boolean isIso, List, Integer> templateChecks(boolean isIso, List, Integer> templateChecks(boolean isIso, List, Integer> findTemplatesByIdOrTempZonePair(Pair, Integer> templateDataPair, boolean showRemoved, boolean showUnique) { + private Pair, Integer> findTemplatesByIdOrTempZonePair(Pair, Integer> templateDataPair, + boolean showRemoved, boolean showUnique, Account caller) { Integer count = templateDataPair.second(); if (count.intValue() == 0) { // empty result @@ -3923,9 +3877,28 @@ private Pair, Integer> findTemplatesByIdOrTempZonePair(Pair String[] templateZonePairs = templateData.stream().map(template -> template.getTempZonePair()).toArray(String[]::new); templates = _templateJoinDao.searchByTemplateZonePair(showRemoved, templateZonePairs); } + + if(caller.getType() != Account.ACCOUNT_TYPE_ADMIN) { + templates = applyPublicTemplateRestriction(templates, caller); + count = templates.size(); + } + return new Pair, Integer>(templates, count); } + private List applyPublicTemplateRestriction(List templates, Account caller){ + List unsharableDomainIds = templates.stream() + .map(TemplateJoinVO::getDomainId) + .distinct() + .filter(domainId -> domainId != caller.getDomainId()) + .filter(Predicate.not(QueryService.SharePublicTemplates::valueIn)) + .collect(Collectors.toList()); + + return templates.stream() + .filter(Predicate.not(t -> unsharableDomainIds.contains(t.getDomainId()))) + .collect(Collectors.toList()); + } + @Override public ListResponse listIsos(ListIsosCmd cmd) { Pair, Integer> result = searchForIsosInternal(cmd); @@ -3959,7 +3932,6 @@ private Pair, Integer> searchForIsosInternal(ListIsosCmd cm List permittedAccountIds = new ArrayList(); Ternary domainIdRecursiveListProject = new Ternary(cmd.getDomainId(), cmd.isRecursive(), null); _accountMgr.buildACLSearchParameters(caller, id, cmd.getAccountName(), cmd.getProjectId(), permittedAccountIds, domainIdRecursiveListProject, listAll, false); - Long domainId = domainIdRecursiveListProject.first(); ListProjectResourcesCriteria listProjectResourcesCriteria = domainIdRecursiveListProject.third(); List permittedAccounts = new ArrayList(); for (Long accountId : permittedAccountIds) { @@ -3969,7 +3941,7 @@ private Pair, Integer> searchForIsosInternal(ListIsosCmd cm HypervisorType hypervisorType = HypervisorType.getType(cmd.getHypervisor()); return searchForTemplatesInternal(cmd.getId(), cmd.getIsoName(), cmd.getKeyword(), isoFilter, true, cmd.isBootable(), cmd.getPageSizeVal(), cmd.getStartIndex(), cmd.getZoneId(), - hypervisorType, true, cmd.listInReadyState(), permittedAccounts, caller, domainId, listProjectResourcesCriteria, tags, showRemovedISO, null, null, cmd.getShowUnique()); + hypervisorType, true, cmd.listInReadyState(), permittedAccounts, caller, listProjectResourcesCriteria, tags, showRemovedISO, null, null, cmd.getShowUnique()); } @Override @@ -4409,6 +4381,6 @@ public String getConfigComponentName() { @Override public ConfigKey[] getConfigKeys() { return new ConfigKey[] {AllowUserViewDestroyedVM, UserVMDeniedDetails, UserVMReadOnlyDetails, SortKeyAscending, - AllowUserViewAllDomainAccounts, RestrictPublicTemplatesOfOtherDomains}; + AllowUserViewAllDomainAccounts, SharePublicTemplates}; } } From a5116cbff13497db76784bb91a58d7859ea9d446 Mon Sep 17 00:00:00 2001 From: Sina Kashipazha Date: Fri, 18 Feb 2022 15:19:58 +0100 Subject: [PATCH 11/16] Added integration test to travis. --- .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index 2d2450142c35..b112953e40eb 100644 --- a/.travis.yml +++ b/.travis.yml @@ -174,6 +174,7 @@ env: component/test_stopped_vm component/test_tags component/test_templates + component/test_template_access_across_domains component/test_updateResourceCount component/test_update_vm" From 08bb47bfb58dbfa300025a9ac3f2c57aa1055c58 Mon Sep 17 00:00:00 2001 From: Sina Kashipazha Date: Tue, 22 Feb 2022 10:43:02 +0100 Subject: [PATCH 12/16] Updated the configuration's name and description. --- .../main/java/org/apache/cloudstack/query/QueryService.java | 4 ++-- .../src/main/java/com/cloud/api/query/QueryManagerImpl.java | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/query/QueryService.java b/api/src/main/java/org/apache/cloudstack/query/QueryService.java index 3ab597374dd5..231e0f2cbd18 100644 --- a/api/src/main/java/org/apache/cloudstack/query/QueryService.java +++ b/api/src/main/java/org/apache/cloudstack/query/QueryService.java @@ -111,8 +111,8 @@ public interface QueryService { "allow.user.view.all.domain.accounts", "false", "Determines whether users can view all user accounts within the same domain", true, ConfigKey.Scope.Domain); - static final ConfigKey SharePublicTemplates = new ConfigKey<>("Advanced", Boolean.class, "share.public.templates", "true", - "If false, templates of this domain with not show up in the list templates.", true, ConfigKey.Scope.Domain); + static final ConfigKey SharePublicTemplatesWithOtherDomains = new ConfigKey<>("Advanced", Boolean.class, "share.public.templates.with.other.domains", "true", + "If false, templates of this domain will not show up in the list templates of other domains.", true, ConfigKey.Scope.Domain); ListResponse searchForUsers(ListUsersCmd cmd) throws PermissionDeniedException; 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 4101f5f1c29f..0fa88d7ef8d0 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -3891,7 +3891,7 @@ private List applyPublicTemplateRestriction(List .map(TemplateJoinVO::getDomainId) .distinct() .filter(domainId -> domainId != caller.getDomainId()) - .filter(Predicate.not(QueryService.SharePublicTemplates::valueIn)) + .filter(Predicate.not(QueryService.SharePublicTemplatesWithOtherDomains::valueIn)) .collect(Collectors.toList()); return templates.stream() @@ -4381,6 +4381,6 @@ public String getConfigComponentName() { @Override public ConfigKey[] getConfigKeys() { return new ConfigKey[] {AllowUserViewDestroyedVM, UserVMDeniedDetails, UserVMReadOnlyDetails, SortKeyAscending, - AllowUserViewAllDomainAccounts, SharePublicTemplates}; + AllowUserViewAllDomainAccounts, SharePublicTemplatesWithOtherDomains}; } } From 8ccaa2b58e6a1ebf60857852ea7ffa9514ca64bc Mon Sep 17 00:00:00 2001 From: Sina Kashipazha Date: Mon, 14 Mar 2022 17:00:37 +0100 Subject: [PATCH 13/16] Extracted public template check to a separate method. --- .../java/com/cloud/acl/DomainChecker.java | 44 ++++++++++++++----- .../com/cloud/api/query/QueryManagerImpl.java | 2 +- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/com/cloud/acl/DomainChecker.java b/server/src/main/java/com/cloud/acl/DomainChecker.java index 069366ea0f30..9ab67ea10354 100644 --- a/server/src/main/java/com/cloud/acl/DomainChecker.java +++ b/server/src/main/java/com/cloud/acl/DomainChecker.java @@ -103,6 +103,38 @@ protected DomainChecker() { super(); } + /** + * + * public template can be used by other accounts in: + * + * 1. the same domain + * 2. in sub-domains + * 3. domain admin of parent domains + * + * In addition to those, everyone can access the public templates in domains that set "share.public.templates.with.other.domains" config to true. + * + * @param template template object + * @param owner owner of the template + * @param caller who wants to access to the template + */ + + private void checkPublicTemplateAccess(VirtualMachineTemplate template, Account owner, Account caller){ + if (!QueryService.SharePublicTemplatesWithOtherDomains.valueIn(owner.getDomainId()) || + caller.getDomainId() == owner.getDomainId() || + _domainDao.isChildDomain(owner.getDomainId(), caller.getDomainId())) { + return; + } + + if (caller.getType() == Account.Type.NORMAL || caller.getType() == Account.Type.PROJECT) { + throw new PermissionDeniedException(caller + "is not allowed to access the template " + template); + } else if (caller.getType() == Account.Type.DOMAIN_ADMIN || caller.getType() == Account.Type.RESOURCE_DOMAIN_ADMIN) { + if (!_domainDao.isChildDomain(caller.getDomainId(), owner.getDomainId())) { + throw new PermissionDeniedException(caller + "is not allowed to access the template " + template); + } + } + } + + @Override public boolean checkAccess(Account caller, Domain domain) throws PermissionDeniedException { if (caller.getState() != Account.State.ENABLED) { @@ -168,16 +200,8 @@ public boolean checkAccess(Account caller, ControlledEntity entity, AccessType a throw new PermissionDeniedException("Domain Admin and regular users can modify only their own Public templates"); } } - } else if (QueryService.RestrictPublicTemplatesOfOtherDomains.valueIn(caller.getDomainId()) && caller.getType() != Account.Type.ADMIN) { // public template can be used by other accounts in the same domain or in sub-domains, and domain admin of parent domains - if (caller.getDomainId() != owner.getDomainId() && !_domainDao.isChildDomain(owner.getDomainId(), caller.getDomainId())) { - if (caller.getType() == Account.Type.NORMAL || caller.getType() == Account.Type.PROJECT) { - throw new PermissionDeniedException(caller + "is not allowed to access the template " + template); - } else if (caller.getType() == Account.Type.DOMAIN_ADMIN || caller.getType() == Account.Type.RESOURCE_DOMAIN_ADMIN) { - if (!_domainDao.isChildDomain(caller.getDomainId(), owner.getDomainId())) { - throw new PermissionDeniedException(caller + "is not allowed to access the template " + template); - } - } - } + } else if (caller.getType() != Account.Type.ADMIN) { + checkPublicTemplateAccess(template, owner, caller); } } 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 0fa88d7ef8d0..cde8f581de0d 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -3878,7 +3878,7 @@ private Pair, Integer> findTemplatesByIdOrTempZonePair(Pair templates = _templateJoinDao.searchByTemplateZonePair(showRemoved, templateZonePairs); } - if(caller.getType() != Account.ACCOUNT_TYPE_ADMIN) { + if(caller.getType() != Account.Type.ADMIN) { templates = applyPublicTemplateRestriction(templates, caller); count = templates.size(); } From f37683998102870d36127b9a6e1a337ae1d8b2d0 Mon Sep 17 00:00:00 2001 From: Sina Kashipazha Date: Thu, 24 Mar 2022 13:36:14 +0100 Subject: [PATCH 14/16] Fixed rebase issue. --- server/src/main/java/com/cloud/api/query/QueryManagerImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 cde8f581de0d..0717201e2fd0 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -3730,7 +3730,7 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) sc.addAnd("publicTemplate", SearchCriteria.Op.SC, scc); } else if (templateFilter == TemplateFilter.all && caller.getType() != Account.Type.ADMIN) { SearchCriteria scc = _templateJoinDao.createSearchCriteria(); - scc.addOr("publicTemplate", SearchCriteria.Op.SC, scc); + scc.addOr("publicTemplate", SearchCriteria.Op.EQ, true); if (listProjectResourcesCriteria == ListProjectResourcesCriteria.SkipProjectResources) { scc.addOr("domainPath", SearchCriteria.Op.LIKE, _domainDao.findById(caller.getDomainId()).getPath() + "%"); From 2cb997c737445d20822aba12aba0871def3fc953 Mon Sep 17 00:00:00 2001 From: Sina Kashipazha Date: Tue, 5 Apr 2022 10:27:11 +0200 Subject: [PATCH 15/16] Apply tear down changes. --- .../component/test_template_access_across_domains.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/component/test_template_access_across_domains.py b/test/integration/component/test_template_access_across_domains.py index 0caf3a86f77c..a737e41de4e9 100644 --- a/test/integration/component/test_template_access_across_domains.py +++ b/test/integration/component/test_template_access_across_domains.py @@ -165,7 +165,7 @@ def tearDown(self): self.update_restrict_template_configuration(self.domain1.id, self.domain1_config) self.update_restrict_template_configuration(self.domain2.id, self.domain2_config) self.update_restrict_template_configuration(self.sub_domain.id, self.sub_domain_config) - cleanup_resources(self.apiclient, self.cleanup) + super(TestTemplateAccessAcrossDomains, self).tearDown() except Exception as e: raise Exception("Warning: Exception during cleanup : %s" % e) return From 4711e5715b8ce804e3400ccc229bf51069be1148 Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Thu, 21 Apr 2022 15:06:23 +0200 Subject: [PATCH 16/16] Update .travis.yml to remove the component test The test needs to be updated to use the new configuration name --- .travis.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index b112953e40eb..2d2450142c35 100644 --- a/.travis.yml +++ b/.travis.yml @@ -174,7 +174,6 @@ env: component/test_stopped_vm component/test_tags component/test_templates - component/test_template_access_across_domains component/test_updateResourceCount component/test_update_vm"