From 5194da7ab463ba92430560e4042fa4229cd05efa Mon Sep 17 00:00:00 2001 From: Bryan Lima Date: Mon, 30 Jan 2023 12:18:41 -0300 Subject: [PATCH 01/14] Add global ACLs feature --- .../user/network/CreateNetworkACLListCmd.java | 38 +++++- .../com/cloud/network/vpc/NetworkACLVO.java | 7 + .../com/cloud/network/NetworkServiceImpl.java | 17 ++- .../network/vpc/NetworkACLServiceImpl.java | 125 ++++++++++++------ .../vpc/NetworkACLServiceImplTest.java | 62 ++++++--- 5 files changed, 174 insertions(+), 75 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkACLListCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkACLListCmd.java index 14dbfcafd7b2..c5314b86acdd 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkACLListCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkACLListCmd.java @@ -16,6 +16,7 @@ // under the License. package org.apache.cloudstack.api.command.user.network; +import com.cloud.exception.PermissionDeniedException; import org.apache.cloudstack.acl.RoleType; import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiCommandResourceType; @@ -26,6 +27,7 @@ import org.apache.cloudstack.api.ServerApiException; import org.apache.cloudstack.api.response.NetworkACLResponse; import org.apache.cloudstack.api.response.VpcResponse; +import org.apache.cloudstack.context.CallContext; import org.apache.log4j.Logger; import com.cloud.event.EventTypes; @@ -35,7 +37,8 @@ import com.cloud.network.vpc.Vpc; import com.cloud.user.Account; -@APICommand(name = "createNetworkACLList", description = "Creates a network ACL for the given VPC", responseObject = NetworkACLResponse.class, +@APICommand(name = "createNetworkACLList", description = "Creates a network ACL. If no VPC is given, then it creates a global ACL that can be used by everyone.", + responseObject = NetworkACLResponse.class, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) public class CreateNetworkACLListCmd extends BaseAsyncCreateCmd { public static final Logger s_logger = Logger.getLogger(CreateNetworkACLListCmd.class.getName()); @@ -53,7 +56,6 @@ public class CreateNetworkACLListCmd extends BaseAsyncCreateCmd { @Parameter(name = ApiConstants.VPC_ID, type = CommandType.UUID, - required = true, entityType = VpcResponse.class, description = "ID of the VPC associated with this network ACL list") private Long vpcId; @@ -77,6 +79,10 @@ public Long getVpcId() { return vpcId; } + public void setVpcId(Long vpcId) { + this.vpcId = vpcId; + } + @Override public boolean isDisplay() { if (display != null) { @@ -92,6 +98,9 @@ public boolean isDisplay() { @Override public void create() { + if (getVpcId() == null) { + setVpcId(0L); + } NetworkACL result = _networkACLService.createNetworkACL(getName(), getDescription(), getVpcId(), isDisplay()); setEntityId(result.getId()); setEntityUuid(result.getUuid()); @@ -111,12 +120,23 @@ public void execute() throws ResourceUnavailableException { @Override public long getEntityOwnerId() { - Vpc vpc = _entityMgr.findById(Vpc.class, getVpcId()); - if (vpc == null) { - throw new InvalidParameterValueException("Invalid vpcId is given"); - } + Account account; + Long vpcId = getVpcId(); + + if (isAclAttachedToVpc(vpcId)) { + Vpc vpc = _entityMgr.findById(Vpc.class, vpcId); + if (vpc == null) { + throw new InvalidParameterValueException(String.format("Invalid VPC ID [%s] provided.", vpcId)); + } + account = _accountService.getAccount(vpc.getAccountId()); + } else { + account = CallContext.current().getCallingAccount(); + if (!Account.Type.ADMIN.equals(account.getType())) { + s_logger.warn(String.format("Only Root Admin can create global ACLs. Account [%s] cannot create any global ACL.", account)); + throw new PermissionDeniedException("Only Root Admin can create global ACLs."); + } - Account account = _accountService.getAccount(vpc.getAccountId()); + } return account.getId(); } @@ -139,4 +159,8 @@ public Long getApiResourceId() { public ApiCommandResourceType getApiResourceType() { return ApiCommandResourceType.NetworkAcl; } + + public boolean isAclAttachedToVpc(Long aclVpcId) { + return aclVpcId != null && aclVpcId != 0; + } } diff --git a/engine/schema/src/main/java/com/cloud/network/vpc/NetworkACLVO.java b/engine/schema/src/main/java/com/cloud/network/vpc/NetworkACLVO.java index 4eaa2b575e0d..280d5dfaf4b2 100644 --- a/engine/schema/src/main/java/com/cloud/network/vpc/NetworkACLVO.java +++ b/engine/schema/src/main/java/com/cloud/network/vpc/NetworkACLVO.java @@ -17,6 +17,8 @@ package com.cloud.network.vpc; +import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils; + import java.util.UUID; import javax.persistence.Column; @@ -85,6 +87,11 @@ public String getName() { return name; } + @Override + public String toString() { + return ReflectionToStringBuilderUtils.reflectOnlySelectedFields(this, "uuid", "name", "vpcId"); + } + public void setUuid(String uuid) { this.uuid = uuid; } diff --git a/server/src/main/java/com/cloud/network/NetworkServiceImpl.java b/server/src/main/java/com/cloud/network/NetworkServiceImpl.java index ac698f696894..c9e2e8abbac8 100644 --- a/server/src/main/java/com/cloud/network/NetworkServiceImpl.java +++ b/server/src/main/java/com/cloud/network/NetworkServiceImpl.java @@ -2098,12 +2098,9 @@ public Network doInTransaction(TransactionStatus status) throws InsufficientCapa throw new InvalidParameterValueException("Unable to find specified NetworkACL"); } - if (aclId != NetworkACL.DEFAULT_DENY && aclId != NetworkACL.DEFAULT_ALLOW) { - // ACL is not default DENY/ALLOW - // ACL should be associated with a VPC - if (!vpcId.equals(acl.getVpcId())) { - throw new InvalidParameterValueException("ACL: " + aclId + " do not belong to the VPC"); - } + Long aclVpcId = acl.getVpcId(); + if (!isDefaultAcl(aclId) && isAclAttachedToVpc(aclVpcId, vpcId)) { + throw new InvalidParameterValueException(String.format("ACL [%s] does not belong to the VPC [%s].", aclId, aclVpcId)); } } network = _vpcMgr.createVpcGuestNetwork(networkOfferingId, name, displayText, gateway, cidr, vlanId, networkDomain, owner, sharedDomainId, pNtwk, zoneId, aclType, @@ -5920,4 +5917,12 @@ public String getConfigComponentName() { public ConfigKey[] getConfigKeys() { return new ConfigKey[] {AllowDuplicateNetworkName, AllowEmptyStartEndIpAddress, VRPrivateInterfaceMtu, VRPublicInterfaceMtu, AllowUsersToSpecifyVRMtu}; } + + public boolean isDefaultAcl(Long aclId) { + return aclId == NetworkACL.DEFAULT_DENY || aclId == NetworkACL.DEFAULT_ALLOW; + } + + public boolean isAclAttachedToVpc(Long aclVpcId, Long vpcId) { + return aclVpcId != 0 && !vpcId.equals(aclVpcId); + } } diff --git a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java index ed37eb5b3751..2c6736a4264c 100644 --- a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java +++ b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java @@ -24,6 +24,7 @@ import javax.inject.Inject; +import com.cloud.exception.PermissionDeniedException; import org.apache.cloudstack.api.ApiErrorCode; import org.apache.cloudstack.api.ServerApiException; import org.apache.cloudstack.api.command.user.network.CreateNetworkACLCmd; @@ -103,12 +104,15 @@ public class NetworkACLServiceImpl extends ManagerBase implements NetworkACLServ @Override public NetworkACL createNetworkACL(final String name, final String description, final long vpcId, final Boolean forDisplay) { - final Account caller = CallContext.current().getCallingAccount(); - final Vpc vpc = _entityMgr.findById(Vpc.class, vpcId); - if (vpc == null) { - throw new InvalidParameterValueException("Unable to find VPC"); + if (vpcId != 0) { + final Account caller = CallContext.current().getCallingAccount(); + final Vpc vpc = _vpcDao.findById(vpcId); + if (vpc == null) { + throw new InvalidParameterValueException(String.format("Unable to find VPC with ID [%s].", vpcId)); + } + _accountMgr.checkAccess(caller, null, true, vpc); + } - _accountMgr.checkAccess(caller, null, true, vpc); return _networkAclMgr.createNetworkACL(name, description, vpcId, forDisplay); } @@ -212,8 +216,8 @@ public Pair, Integer> listNetworkACLs(final ListNetwo @Override @ActionEvent(eventType = EventTypes.EVENT_NETWORK_ACL_DELETE, eventDescription = "Deleting Network ACL List", async = true) public boolean deleteNetworkACL(final long id) { - final Account caller = CallContext.current().getCallingAccount(); final NetworkACL acl = _networkACLDao.findById(id); + Account account = CallContext.current().getCallingAccount(); if (acl == null) { throw new InvalidParameterValueException("Unable to find specified ACL"); } @@ -223,11 +227,7 @@ public boolean deleteNetworkACL(final long id) { throw new InvalidParameterValueException("Default ACL cannot be removed"); } - final Vpc vpc = _entityMgr.findById(Vpc.class, acl.getVpcId()); - if (vpc == null) { - throw new InvalidParameterValueException("Unable to find specified VPC associated with the ACL"); - } - _accountMgr.checkAccess(caller, null, true, vpc); + validateGlobalAclPermissionAndAclAssociatedToVpc(acl, account, "Only Root Admin can delete global ACLs."); return _networkAclMgr.deleteNetworkACL(acl); } @@ -293,15 +293,9 @@ public boolean replaceNetworkACL(final long aclId, final long networkId) throws throw new InvalidParameterValueException("Network ACL can be created just for networks of type " + Networks.TrafficType.Guest); } - if (aclId != NetworkACL.DEFAULT_DENY && aclId != NetworkACL.DEFAULT_ALLOW) { - //ACL is not default DENY/ALLOW - // ACL should be associated with a VPC - final Vpc vpc = _entityMgr.findById(Vpc.class, acl.getVpcId()); - if (vpc == null) { - throw new InvalidParameterValueException("Unable to find Vpc associated with the NetworkACL"); - } + if (aclId != NetworkACL.DEFAULT_DENY && aclId != NetworkACL.DEFAULT_ALLOW && !isGlobalAcl(acl.getVpcId())) { + validateAclAssociatedToVpc(acl.getVpcId(), caller, acl.getUuid()); - _accountMgr.checkAccess(caller, null, true, vpc); if (!network.getVpcId().equals(acl.getVpcId())) { throw new InvalidParameterValueException("Network: " + networkId + " and ACL: " + aclId + " do not belong to the same VPC"); } @@ -340,6 +334,11 @@ public NetworkACLItem createNetworkACLItem(CreateNetworkACLCmd createNetworkACLC NetworkACL acl = _networkAclMgr.getNetworkACL(aclId); validateNetworkAcl(acl); + Account caller = CallContext.current().getCallingAccount(); + + if (isGlobalAcl(acl.getVpcId()) && !Account.Type.ADMIN.equals(caller.getType())) { + throw new PermissionDeniedException("Only Root Admins can create rules for a global ACL."); + } validateAclRuleNumber(createNetworkACLCmd, acl); NetworkACLItem.Action ruleAction = validateAndCreateNetworkAclRuleAction(action); @@ -409,7 +408,8 @@ protected void validateAclRuleNumber(CreateNetworkACLCmd createNetworkAclCmd, Ne * * * After all validations, we check if the user has access to the given network ACL using {@link AccountManager#checkAccess(Account, org.apache.cloudstack.acl.SecurityChecker.AccessType, boolean, org.apache.cloudstack.acl.ControlledEntity...)}. @@ -423,12 +423,10 @@ protected void validateNetworkAcl(NetworkACL acl) { throw new InvalidParameterValueException("Default ACL cannot be modified"); } - Vpc vpc = _entityMgr.findById(Vpc.class, acl.getVpcId()); - if (vpc == null) { - throw new InvalidParameterValueException(String.format("Unable to find Vpc associated with the NetworkACL [%s]", acl.getUuid())); + Long aclVpcId = acl.getVpcId(); + if (!isGlobalAcl(aclVpcId)) { + validateAclAssociatedToVpc(aclVpcId, CallContext.current().getCallingAccount(), acl.getUuid()); } - Account caller = CallContext.current().getCallingAccount(); - _accountMgr.checkAccess(caller, null, true, vpc); } /** @@ -792,17 +790,12 @@ public boolean revokeNetworkACLItem(final long ruleId) { final NetworkACLItemVO aclItem = _networkACLItemDao.findById(ruleId); if (aclItem != null) { final NetworkACL acl = _networkAclMgr.getNetworkACL(aclItem.getAclId()); - - final Vpc vpc = _entityMgr.findById(Vpc.class, acl.getVpcId()); + final Account account = CallContext.current().getCallingAccount(); if (aclItem.getAclId() == NetworkACL.DEFAULT_ALLOW || aclItem.getAclId() == NetworkACL.DEFAULT_DENY) { throw new InvalidParameterValueException("ACL Items in default ACL cannot be deleted"); } - - final Account caller = CallContext.current().getCallingAccount(); - - _accountMgr.checkAccess(caller, null, true, vpc); - + validateGlobalAclPermissionAndAclAssociatedToVpc(acl, account, "Only Root Admin can delete global ACL rules."); } return _networkAclMgr.revokeNetworkACLItem(ruleId); } @@ -826,6 +819,9 @@ public NetworkACLItem updateNetworkACLItem(UpdateNetworkACLItemCmd updateNetwork NetworkACL acl = _networkAclMgr.getNetworkACL(networkACLItemVo.getAclId()); validateNetworkAcl(acl); + Account account = CallContext.current().getCallingAccount(); + validateGlobalAclPermissionAndAclAssociatedToVpc(acl, account, "Only Root Admins can update global ACLs."); + transferDataToNetworkAclRulePojo(updateNetworkACLItemCmd, networkACLItemVo, acl); validateNetworkACLItem(networkACLItemVo); return _networkAclMgr.updateNetworkACLItem(networkACLItemVo); @@ -912,14 +908,13 @@ protected NetworkACLItemVO validateNetworkAclRuleIdAndRetrieveIt(UpdateNetworkAC } @Override - @ActionEvent(eventType = EventTypes.EVENT_NETWORK_ACL_UPDATE, eventDescription = "updating network acl", async = true) + @ActionEvent(eventType = EventTypes.EVENT_NETWORK_ACL_UPDATE, eventDescription = "Updating Network ACL", async = true) public NetworkACL updateNetworkACL(UpdateNetworkACLListCmd updateNetworkACLListCmd) { Long id = updateNetworkACLListCmd.getId(); NetworkACLVO acl = _networkACLDao.findById(id); - Vpc vpc = _entityMgr.findById(Vpc.class, acl.getVpcId()); + Account account = CallContext.current().getCallingAccount(); - Account caller = CallContext.current().getCallingAccount(); - _accountMgr.checkAccess(caller, null, true, vpc); + validateGlobalAclPermissionAndAclAssociatedToVpc(acl, account, "Must be Root Admin to update a global ACL."); String name = updateNetworkACLListCmd.getName(); if (StringUtils.isNotBlank(name)) { @@ -1146,17 +1141,59 @@ protected void validateMoveAclRulesData(NetworkACLItemVO ruleBeingMoved, Network if (nextRule == null && previousRule == null) { throw new InvalidParameterValueException("Both previous and next ACL rule IDs cannot be invalid."); } - long aclId = ruleBeingMoved.getAclId(); + long ruleAclId = ruleBeingMoved.getAclId(); - if ((nextRule != null && nextRule.getAclId() != aclId) || (previousRule != null && previousRule.getAclId() != aclId)) { - throw new InvalidParameterValueException("Cannot use ACL rules from differenting ACLs. Rule being moved."); + if ((nextRule != null && nextRule.getAclId() != ruleAclId) || (previousRule != null && previousRule.getAclId() != ruleAclId)) { + throw new InvalidParameterValueException("Cannot use ACL rules from differentiating ACLs. Rule being moved."); } - NetworkACLVO acl = _networkACLDao.findById(aclId); - Vpc vpc = _entityMgr.findById(Vpc.class, acl.getVpcId()); + NetworkACLVO acl = _networkACLDao.findById(ruleAclId); + Account account = CallContext.current().getCallingAccount(); + long aclId = acl.getId(); + + if (aclId == NetworkACL.DEFAULT_ALLOW || aclId == NetworkACL.DEFAULT_DENY) { + throw new InvalidParameterValueException("Default ACL rules cannot be moved."); + } + + validateGlobalAclPermissionAndAclAssociatedToVpc(acl, account,"Must be Root Admin to move global ACL rules."); + } + + /** + * Checks if the given ACL is a global ACL. If it is, then checks if the account is Root Admin, and throws an exception according to {@code exceptionMessage} param if it + * does not have permission. + */ + protected void checkGlobalAclPermission(Long aclVpcId, Account account, String exceptionMessage) { + if (isGlobalAcl(aclVpcId) && !Account.Type.ADMIN.equals(account.getType())) { + throw new PermissionDeniedException(exceptionMessage); + } + } + + protected void validateAclAssociatedToVpc(Long aclVpcId, Account account, String aclUuid) { + final Vpc vpc = _vpcDao.findById(aclVpcId); if (vpc == null) { - throw new InvalidParameterValueException("Re-ordering rules for a default ACL is prohibited"); + throw new InvalidParameterValueException(String.format("Unable to find specified VPC [%s] associated with the ACL [%s].", aclVpcId, aclUuid)); } - Account caller = CallContext.current().getCallingAccount(); - _accountMgr.checkAccess(caller, null, true, vpc); + _accountMgr.checkAccess(account, null, true, vpc); + } + + /** + * It performs two different verifications depending on if the ACL is global or not. + * + */ + protected void validateGlobalAclPermissionAndAclAssociatedToVpc(NetworkACL acl, Account account, String exception){ + if (isGlobalAcl(acl.getVpcId())) { + s_logger.info(String.format("Checking if account [%s] has permission to manipulate global ACL [%s].", account, acl)); + checkGlobalAclPermission(acl.getVpcId(), account, exception); + } else { + s_logger.info(String.format("Validating ACL [%s] associated to VPC [%s] with account [%s].", acl, acl.getVpcId(), account)); + validateAclAssociatedToVpc(acl.getVpcId(), account, acl.getUuid()); + } + } + + protected boolean isGlobalAcl(Long aclVpcId) { + return aclVpcId != null && aclVpcId == 0; } } diff --git a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java b/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java index d7333a9da111..dee84c103fb6 100644 --- a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java +++ b/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java @@ -30,6 +30,7 @@ import java.util.List; import java.util.Map; +import com.cloud.network.vpc.dao.VpcDao; import org.apache.cloudstack.acl.SecurityChecker.AccessType; import org.apache.cloudstack.api.ServerApiException; import org.apache.cloudstack.api.command.user.network.CreateNetworkACLCmd; @@ -83,6 +84,8 @@ public class NetworkACLServiceImplTest { @Mock private EntityManager entityManagerMock; @Mock + private VpcDao vpcDaoMock; + @Mock private AccountManager accountManagerMock; @Mock private NetworkACLDao networkAclDaoMock; @@ -106,6 +109,7 @@ public class NetworkACLServiceImplTest { private Long networkOfferingMockId = 2L; private Long networkMockVpcMockId = 3L; private long networkAclListId = 1l; + private static final String SOME_UUID = "someUuid"; @Mock private MoveNetworkAclItemCmd moveNetworkAclItemCmdMock; @@ -124,8 +128,14 @@ public class NetworkACLServiceImplTest { @Mock private CallContext callContextMock; + @Mock + private VpcVO vpcVOMock; + + @Mock + private Account accountMock; + @Before - public void befoteTest() { + public void beforeTest() { PowerMockito.mockStatic(CallContext.class); PowerMockito.when(CallContext.current()).thenReturn(callContextMock); Mockito.doReturn(Mockito.mock(User.class)).when(callContextMock).getCallingUser(); @@ -179,9 +189,9 @@ public NetworkACLItemVO answer(InvocationOnMock invocation) throws Throwable { } }); - NetworkACLItem netowkrAclRuleCreated = networkAclServiceImpl.createNetworkACLItem(createNetworkAclCmdMock); + NetworkACLItem networkAclRuleCreated = networkAclServiceImpl.createNetworkACLItem(createNetworkAclCmdMock); - Assert.assertEquals(number == null ? 6 : number, netowkrAclRuleCreated.getNumber()); + Assert.assertEquals(number == null ? 6 : number, networkAclRuleCreated.getNumber()); InOrder inOrder = Mockito.inOrder( networkAclServiceImpl, networkAclManagerMock, networkAclItemDaoMock); inOrder.verify(networkAclServiceImpl).createAclListIfNeeded(createNetworkAclCmdMock); @@ -396,15 +406,14 @@ public void validateNetworkAclTestAclDefaulDeny() { @Test(expected = InvalidParameterValueException.class) public void validateNetworkAclTestAclNotDefaulWithoutVpc() { Mockito.when(networkAclMock.getId()).thenReturn(3L); - Mockito.doReturn(null).when(entityManagerMock).findById(Vpc.class, networkMockVpcMockId); - ; + Mockito.doReturn(null).when(vpcDaoMock).findById(networkMockVpcMockId); ; networkAclServiceImpl.validateNetworkAcl(networkAclMock); } @Test @PrepareForTest(CallContext.class) - public void validateNetworkAclTestAclNotDefaulWithVpc() { + public void validateNetworkAclTestAclNotDefaultWithVpc() { CallContext callContextMock = Mockito.mock(CallContext.class); Mockito.doReturn(Mockito.mock(Account.class)).when(callContextMock).getCallingAccount(); @@ -414,12 +423,12 @@ public void validateNetworkAclTestAclNotDefaulWithVpc() { Mockito.when(networkAclMock.getId()).thenReturn(3L); Mockito.when(networkAclMock.getVpcId()).thenReturn(networkMockVpcMockId); - Mockito.doReturn(Mockito.mock(Vpc.class)).when(entityManagerMock).findById(Vpc.class, networkMockVpcMockId); + Mockito.doReturn(vpcVOMock).when(vpcDaoMock).findById(networkMockVpcMockId); Mockito.doNothing().when(accountManagerMock).checkAccess(Mockito.any(Account.class), Mockito.isNull(AccessType.class), Mockito.eq(true), Mockito.any(Vpc.class)); networkAclServiceImpl.validateNetworkAcl(networkAclMock); - Mockito.verify(entityManagerMock).findById(Vpc.class, networkMockVpcMockId); + Mockito.doReturn(vpcVOMock).when(vpcDaoMock).findById(networkMockVpcMockId); Mockito.verify(accountManagerMock).checkAccess(Mockito.any(Account.class), Mockito.isNull(AccessType.class), Mockito.eq(true), Mockito.any(Vpc.class)); PowerMockito.verifyStatic(CallContext.class); @@ -711,16 +720,24 @@ public void updateNetworkACLItemTest() throws ResourceUnavailableException { Mockito.doReturn(networkAclItemVoMock).when(networkAclServiceImpl).validateNetworkAclRuleIdAndRetrieveIt(updateNetworkACLItemCmdMock); Mockito.doReturn(networkAclMock).when(networkAclManagerMock).getNetworkACL(networkAclMockId); Mockito.doNothing().when(networkAclServiceImpl).validateNetworkAcl(Mockito.eq(networkAclMock)); + Mockito.doNothing().when(networkAclServiceImpl).validateGlobalAclPermissionAndAclAssociatedToVpc(Mockito.any(NetworkACL.class), Mockito.any(Account.class), Mockito.anyString()); Mockito.doNothing().when(networkAclServiceImpl).transferDataToNetworkAclRulePojo(Mockito.eq(updateNetworkACLItemCmdMock), Mockito.eq(networkAclItemVoMock), Mockito.eq(networkAclMock)); Mockito.doNothing().when(networkAclServiceImpl).validateNetworkACLItem(networkAclItemVoMock); Mockito.doReturn(networkAclItemVoMock).when(networkAclManagerMock).updateNetworkACLItem(networkAclItemVoMock); + CallContext callContextMock = Mockito.mock(CallContext.class); + Mockito.doReturn(accountMock).when(callContextMock).getCallingAccount(); + + PowerMockito.mockStatic(CallContext.class); + PowerMockito.when(CallContext.current()).thenReturn(callContextMock); + networkAclServiceImpl.updateNetworkACLItem(updateNetworkACLItemCmdMock); InOrder inOrder = Mockito.inOrder(networkAclServiceImpl, networkAclManagerMock); inOrder.verify(networkAclServiceImpl).validateNetworkAclRuleIdAndRetrieveIt(updateNetworkACLItemCmdMock); inOrder.verify(networkAclManagerMock).getNetworkACL(networkAclMockId); inOrder.verify(networkAclServiceImpl).validateNetworkAcl(networkAclMock); + inOrder.verify(networkAclServiceImpl).validateGlobalAclPermissionAndAclAssociatedToVpc(networkAclMock, accountMock, "Only Root Admins can update global ACLs."); inOrder.verify(networkAclServiceImpl).transferDataToNetworkAclRulePojo(Mockito.eq(updateNetworkACLItemCmdMock), Mockito.eq(networkAclItemVoMock), Mockito.eq(networkAclMock)); inOrder.verify(networkAclServiceImpl).validateNetworkACLItem(networkAclItemVoMock); inOrder.verify(networkAclManagerMock).updateNetworkACLItem(networkAclItemVoMock); @@ -877,13 +894,18 @@ public void updateNetworkACLTestParametersNotNull() { Mockito.when(updateNetworkACLListCmdMock.getCustomId()).thenReturn(customId); Mockito.when(updateNetworkACLListCmdMock.getId()).thenReturn(networkAclListId); Mockito.when(updateNetworkACLListCmdMock.getDisplay()).thenReturn(false); + Mockito.when(networkACLVOMock.getVpcId()).thenReturn(networkMockVpcMockId); + Mockito.doReturn(vpcVOMock).when(vpcDaoMock).findById(networkMockVpcMockId); + Mockito.doNothing().when(accountManagerMock).checkAccess(Mockito.any(Account.class), + Mockito.isNull(AccessType.class), Mockito.eq(true), Mockito.any(Vpc.class)); + networkAclServiceImpl.updateNetworkACL(updateNetworkACLListCmdMock); - InOrder inOrder = Mockito.inOrder(networkAclDaoMock, entityManagerMock, entityManagerMock, accountManagerMock, networkACLVOMock); + InOrder inOrder = Mockito.inOrder(networkAclDaoMock, vpcDaoMock, accountManagerMock, networkACLVOMock); inOrder.verify(networkAclDaoMock).findById(networkAclListId); - inOrder.verify(entityManagerMock).findById(Mockito.eq(Vpc.class), Mockito.anyLong()); + inOrder.verify(vpcDaoMock).findById(Mockito.anyLong()); inOrder.verify(accountManagerMock).checkAccess(Mockito.any(Account.class), Mockito.isNull(AccessType.class), Mockito.eq(true), nullable(Vpc.class)); @@ -1077,13 +1099,17 @@ public void updateNetworkACLTestParametersWithNullValues() { Mockito.when(updateNetworkACLListCmdMock.getCustomId()).thenReturn(null); Mockito.when(updateNetworkACLListCmdMock.getId()).thenReturn(networkAclListId); Mockito.when(updateNetworkACLListCmdMock.getDisplay()).thenReturn(null); + Mockito.when(networkACLVOMock.getVpcId()).thenReturn(networkMockVpcMockId); + Mockito.doReturn(vpcVOMock).when(vpcDaoMock).findById(networkMockVpcMockId); + Mockito.doNothing().when(accountManagerMock).checkAccess(Mockito.any(Account.class), + Mockito.isNull(AccessType.class), Mockito.eq(true), Mockito.any(Vpc.class)); networkAclServiceImpl.updateNetworkACL(updateNetworkACLListCmdMock); - InOrder inOrder = Mockito.inOrder(networkAclDaoMock, entityManagerMock, accountManagerMock, networkACLVOMock); + InOrder inOrder = Mockito.inOrder(networkAclDaoMock, vpcDaoMock, accountManagerMock, networkACLVOMock); inOrder.verify(networkAclDaoMock).findById(networkAclListId); - inOrder.verify(entityManagerMock).findById(eq(Vpc.class), Mockito.anyLong()); + inOrder.verify(vpcDaoMock).findById(Mockito.anyLong()); inOrder.verify(accountManagerMock).checkAccess(any(Account.class), isNull(), eq(true), nullable(Vpc.class)); Mockito.verify(networkACLVOMock, Mockito.times(0)).setName(null); @@ -1100,22 +1126,22 @@ public void validateMoveAclRulesDataTestSuccesfullExecution() { Mockito.when(nextAclRuleMock.getAclId()).thenReturn(networkAclMockId); Mockito.when(previousAclRuleMock.getAclId()).thenReturn(networkAclMockId); - Mockito.doReturn(networkAclMock).when(networkAclDaoMock).findById(networkAclMockId); - Mockito.doReturn(Mockito.mock(Vpc.class)).when(entityManagerMock).findById(Vpc.class, networkMockVpcMockId); + Mockito.when(networkAclMock.getUuid()).thenReturn(SOME_UUID); CallContext callContextMock = Mockito.mock(CallContext.class); Mockito.doReturn(Mockito.mock(Account.class)).when(callContextMock).getCallingAccount(); + Mockito.doReturn(networkAclMock).when(networkAclDaoMock).findById(networkAclMockId); + PowerMockito.mockStatic(CallContext.class); PowerMockito.when(CallContext.current()).thenReturn(callContextMock); - Mockito.doNothing().when(accountManagerMock).checkAccess(Mockito.any(Account.class), Mockito.isNull(AccessType.class), Mockito.eq(true), Mockito.any(Vpc.class)); + Mockito.doNothing().when(networkAclServiceImpl).validateGlobalAclPermissionAndAclAssociatedToVpc(Mockito.any(NetworkACL.class), Mockito.any(Account.class), Mockito.anyString()); networkAclServiceImpl.validateMoveAclRulesData(aclRuleBeingMovedMock, previousAclRuleMock, nextAclRuleMock); Mockito.verify(networkAclDaoMock).findById(networkAclMockId); - Mockito.verify(entityManagerMock).findById(Vpc.class, networkMockVpcMockId); - Mockito.verify(accountManagerMock).checkAccess(Mockito.any(Account.class), Mockito.isNull(AccessType.class), Mockito.eq(true), Mockito.any(Vpc.class)); + Mockito.verify(networkAclServiceImpl).validateGlobalAclPermissionAndAclAssociatedToVpc(Mockito.any(NetworkACL.class), Mockito.any(Account.class), Mockito.anyString()); } @Test @@ -1386,7 +1412,7 @@ public void validateAclConsistencyTest() { ArrayList allAclRules = new ArrayList<>(); allAclRules.add(networkAclItemVoMock); - Mockito.doReturn("someUuid").when(networkAclItemVoMock).getUuid(); + Mockito.doReturn(SOME_UUID).when(networkAclItemVoMock).getUuid(); networkAclServiceImpl.validateAclConsistency(moveNetworkAclItemCmdMock, networkACLVOMock, allAclRules); Mockito.verify(moveNetworkAclItemCmdMock, Mockito.times(1)).getAclConsistencyHash(); From c8e1efb5286855df36d18d3f1a290e765f8d6c8c Mon Sep 17 00:00:00 2001 From: Bryan Lima Date: Mon, 30 Jan 2023 17:56:43 -0300 Subject: [PATCH 02/14] Add unit tests --- .../vpc/NetworkACLServiceImplTest.java | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java b/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java index dee84c103fb6..5142f14a3133 100644 --- a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java +++ b/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java @@ -30,6 +30,7 @@ import java.util.List; import java.util.Map; +import com.cloud.exception.PermissionDeniedException; import com.cloud.network.vpc.dao.VpcDao; import org.apache.cloudstack.acl.SecurityChecker.AccessType; import org.apache.cloudstack.api.ServerApiException; @@ -1417,4 +1418,36 @@ public void validateAclConsistencyTest() { Mockito.verify(moveNetworkAclItemCmdMock, Mockito.times(1)).getAclConsistencyHash(); } + + @Test + public void checkGlobalAclPermissionTestGlobalAclWithRootAccountShouldWork() { + Mockito.doReturn(Account.Type.ADMIN).when(accountMock).getType(); + Mockito.doReturn(true).when(networkAclServiceImpl).isGlobalAcl(Mockito.anyLong()); + + networkAclServiceImpl.checkGlobalAclPermission(networkMockVpcMockId, accountMock, "exception"); + } + + @Test(expected = PermissionDeniedException.class) + public void checkGlobalAclPermissionTestGlobalAclWithNonRootAccountShouldThrow() { + Mockito.doReturn(Account.Type.NORMAL).when(accountMock).getType(); + Mockito.doReturn(true).when(networkAclServiceImpl).isGlobalAcl(Mockito.anyLong()); + + networkAclServiceImpl.checkGlobalAclPermission(networkMockVpcMockId, accountMock, "exception"); + } + + @Test + public void validateAclAssociatedToVpcTestNonNullVpcShouldCheckAccess() { + Mockito.doReturn(vpcVOMock).when(vpcDaoMock).findById(Mockito.anyLong()); + + networkAclServiceImpl.validateAclAssociatedToVpc(networkMockVpcMockId, accountMock, SOME_UUID); + + Mockito.verify(accountManagerMock, Mockito.times(1)).checkAccess(Mockito.any(Account.class), isNull(), Mockito.anyBoolean(), Mockito.any(Vpc.class)); + } + + @Test(expected = InvalidParameterValueException.class) + public void validateAclAssociatedToVpcTestNullVpcShouldThrowInvalidParameterValueException() { + Mockito.doReturn(null).when(vpcDaoMock).findById(Mockito.anyLong()); + + networkAclServiceImpl.validateAclAssociatedToVpc(networkMockVpcMockId, accountMock, SOME_UUID); + } } From 0b6270d641b49d5cb1570fd61830d8aec160c4c7 Mon Sep 17 00:00:00 2001 From: Bryan Lima Date: Mon, 13 Feb 2023 13:49:28 +0000 Subject: [PATCH 03/14] Add smoke tests for global ACL --- test/integration/smoke/test_global_acls.py | 212 +++++++++++++++++++++ 1 file changed, 212 insertions(+) create mode 100644 test/integration/smoke/test_global_acls.py diff --git a/test/integration/smoke/test_global_acls.py b/test/integration/smoke/test_global_acls.py new file mode 100644 index 000000000000..30a5b8a0b6b1 --- /dev/null +++ b/test/integration/smoke/test_global_acls.py @@ -0,0 +1,212 @@ +# 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. +from marvin.cloudstackTestCase import cloudstackTestCase +from marvin.lib.utils import cleanup_resources +from marvin.lib.base import (Network, NetworkACLList, NetworkOffering, VpcOffering, VPC, NetworkACL) +from marvin.lib.common import (get_domain, get_zone) +from nose.plugins.attrib import attr +from marvin.cloudstackException import CloudstackAPIException + + +class Services: + """Test Global ACLs + """ + + def __init__(self): + self.services = { + "root_domain": { + "name": "ROOT", + }, + "domain": { + "name": "Domain", + }, + "user": { + "username": "user", + "roletype": 0, + }, + "domain_admin": { + "username": "Domain admin", + "roletype": 2, + }, + "root_admin": { + "username": "Root admin", + "roletype": 1, + }, + "vpc": { + "name": "vpc-networkacl", + "displaytext": "vpc-networkacl", + "cidr": "10.1.1.0/24", + }, + "vpcnetwork": { + "name": "vpcnetwork", + "displaytext": "vpcnetwork", + }, + "rule": { + "protocol": "all", + "traffictype": "ingress", + } + } + + +class TestGlobalACLs(cloudstackTestCase): + + @classmethod + def setUpClass(cls): + cls.testClient = super(TestGlobalACLs, cls).getClsTestClient() + cls.api_client = cls.testClient.getApiClient() + + cls.services = Services().services + cls.domain = get_domain(cls.api_client) + cls.zone = get_zone(cls.api_client, cls.testClient.getZoneForTests()) + return + + @classmethod + def tearDownClass(cls): + super(TestGlobalACLs, cls).tearDownClass() + + def setUp(self): + self.apiclient = self.testClient.getApiClient() + self.user_apiclient = self.testClient.getUserApiClient(self.services["user"]["username"], + self.services["domain"]["name"], + self.services["user"]["roletype"]) + + self.domain_admin_apiclient = self.testClient.getUserApiClient(self.services["domain_admin"]["username"], + self.services["domain"]["name"], + self.services["domain_admin"]["roletype"]) + + self.admin_apiclient = self.testClient.getUserApiClient(self.services["root_admin"]["username"], + self.services["root_domain"]["name"], + self.services["root_admin"]["roletype"]) + + self.cleanup = [] + return + + def tearDown(self): + try: + cleanup_resources(self.apiclient, self.cleanup) + cleanup_resources(self.user_apiclient, self.cleanup) + except Exception as e: + self.debug("Warning! Exception in tearDown: %s" % e) + + @attr(tags=["advanced", "basic"], required_hardware="false") + def test_create_global_acl(self): + """ Test create global ACL as a normal user, domain admin and root admin users. + """ + + self.debug("Creating ACL list as a normal user, should raise exception.") + self.assertRaisesRegex(CloudstackAPIException, "Only Root Admin can create global ACLs.", + NetworkACLList.create, apiclient=self.user_apiclient, services={}, + name="acl", description="acl") + + self.debug("Creating ACL list as a domain admin, should raise exception.") + self.assertRaisesRegex(CloudstackAPIException, "Only Root Admin can create global ACLs.", + NetworkACLList.create, apiclient=self.domain_admin_apiclient, services={}, + name="acl", description="acl") + + self.debug("Creating ACL list as a root admin, should work.") + acl = NetworkACLList.create(apiclient=self.admin_apiclient, services={}, name="acl", description="acl") + self.assertIsNotNone(acl, "A root admin user should be able to create a global ACL.") + + return + + @attr(tags=["advanced", "basic"], required_hardware="false") + def test_replace_acl_of_network(self): + """ Test to replace ACL of a VPC as a normal user, domain admin and root admin users. + """ + # Create network offering + networkOffering = NetworkOffering.list(self.apiclient, name="DefaultIsolatedNetworkOfferingForVpcNetworks") + self.assertTrue(networkOffering is not None and len(networkOffering) > 0, "No VPC based network offering") + + # Getting VPC offering + vpcOffering = VpcOffering.list(self.apiclient, name="Default VPC offering") + self.assertTrue(vpcOffering is not None and len(vpcOffering) > 0, "No VPC offerings found") + + # Creating VPC + vpc = VPC.create( + apiclient=self.apiclient, + services=self.services["vpc"], + networkDomain="vpc.networkacl", + vpcofferingid=vpcOffering[0].id, + zoneid=self.zone.id, + domainid=self.domain.id + ) + self.assertTrue(vpc is not None, "VPC creation failed") + + # Creating ACL list + acl = NetworkACLList.create(apiclient=self.apiclient, services={}, name="acl", description="acl") + + # Creating tier on VPC with ACL list + network = Network.create( + apiclient=self.apiclient, + services=self.services["vpcnetwork"], + accountid="Admin", + domainid=self.domain.id, + networkofferingid=networkOffering[0].id, + zoneid=self.zone.id, + vpcid=vpc.id, + aclid=acl.id, + gateway="10.1.1.1", + netmask="255.255.255.192" + ) + + # User should be able to replace ACL + network.replaceACLList(apiclient=self.user_apiclient, aclid=acl.id) + # Domain Admin should be able to replace ACL + network.replaceACLList(apiclient=self.domain_admin_apiclient, aclid=acl.id) + # Admin should be able to replace ACL + network.replaceACLList(apiclient=self.admin_apiclient, aclid=acl.id) + + return + + @attr(tags=["advanced", "basic"], required_hardware="false") + def test_create_acl_rule(self): + """ Test to create ACL rule as a normal user, domain admin and root admin users. + """ + # Creating ACL list + acl = NetworkACLList.create(apiclient=self.admin_apiclient, services={}, name="acl", description="acl") + + self.debug("Creating ACL rule as a user, should raise exception.") + self.assertRaisesRegex(CloudstackAPIException, "Only Root Admins can create rules for a global ACL.", + NetworkACL.create, self.user_apiclient, services=self.services["rule"], aclid=acl.id) + self.debug("Creating ACL rule as a domain admin, should raise exception.") + self.assertRaisesRegex(CloudstackAPIException, "Only Root Admins can create rules for a global ACL.", + NetworkACL.create, self.domain_admin_apiclient, services=self.services["rule"], aclid=acl.id) + self.debug("Creating ACL rule as a root admin, should work.") + NetworkACL.create(self.admin_apiclient, services=self.services["rule"], aclid=acl.id) + + return + + @attr(tags=["advanced", "basic"], required_hardware="false") + def test_delete_acl_rule(self): + """ Test to delete ACL rule as a normal user, domain admin and root admin users. + """ + # Creating ACL list + acl = NetworkACLList.create(apiclient=self.api_client, services={}, name="acl", description="acl") + + # Creating ACL rule + acl_rule = NetworkACL.create(self.apiclient, services=self.services["rule"], aclid=acl.id) + + self.debug("Deleting ACL rule as a user, should raise exception.") + self.assertRaisesRegex(Exception, "Only Root Admin can delete global ACL rules.", + NetworkACL.delete, acl_rule, self.user_apiclient) + self.debug("Deleting ACL rule as a domain admin, should raise exception.") + self.assertRaisesRegex(Exception, "Only Root Admin can delete global ACL rules.", + NetworkACL.delete, acl_rule, self.domain_admin_apiclient) + self.debug("Deleting ACL rule as a root admin, should work.") + NetworkACL.delete(acl_rule, self.admin_apiclient) + + return From f6236d706891a4abdcccf639d8147a8e9eb39ea8 Mon Sep 17 00:00:00 2001 From: Bryan Lima Date: Mon, 13 Feb 2023 11:38:47 -0300 Subject: [PATCH 04/14] Address review --- .../command/user/network/CreateNetworkACLListCmd.java | 8 +++----- .../com/cloud/network/vpc/NetworkACLServiceImpl.java | 9 ++++----- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkACLListCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkACLListCmd.java index c5314b86acdd..e5dbcc7b6d1b 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkACLListCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkACLListCmd.java @@ -121,12 +121,10 @@ public void execute() throws ResourceUnavailableException { @Override public long getEntityOwnerId() { Account account; - Long vpcId = getVpcId(); - - if (isAclAttachedToVpc(vpcId)) { - Vpc vpc = _entityMgr.findById(Vpc.class, vpcId); + if (isAclAttachedToVpc(this.vpcId)) { + Vpc vpc = _entityMgr.findById(Vpc.class, this.vpcId); if (vpc == null) { - throw new InvalidParameterValueException(String.format("Invalid VPC ID [%s] provided.", vpcId)); + throw new InvalidParameterValueException(String.format("Invalid VPC ID [%s] provided.", this.vpcId)); } account = _accountService.getAccount(vpc.getAccountId()); } else { diff --git a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java index 2c6736a4264c..a54a7a73ea07 100644 --- a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java +++ b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java @@ -106,7 +106,7 @@ public class NetworkACLServiceImpl extends ManagerBase implements NetworkACLServ public NetworkACL createNetworkACL(final String name, final String description, final long vpcId, final Boolean forDisplay) { if (vpcId != 0) { final Account caller = CallContext.current().getCallingAccount(); - final Vpc vpc = _vpcDao.findById(vpcId); + final Vpc vpc = _entityMgr.findById(Vpc.class, vpcId); if (vpc == null) { throw new InvalidParameterValueException(String.format("Unable to find VPC with ID [%s].", vpcId)); } @@ -1141,14 +1141,13 @@ protected void validateMoveAclRulesData(NetworkACLItemVO ruleBeingMoved, Network if (nextRule == null && previousRule == null) { throw new InvalidParameterValueException("Both previous and next ACL rule IDs cannot be invalid."); } - long ruleAclId = ruleBeingMoved.getAclId(); + long aclId = ruleBeingMoved.getAclId(); - if ((nextRule != null && nextRule.getAclId() != ruleAclId) || (previousRule != null && previousRule.getAclId() != ruleAclId)) { + if ((nextRule != null && nextRule.getAclId() != aclId) || (previousRule != null && previousRule.getAclId() != aclId)) { throw new InvalidParameterValueException("Cannot use ACL rules from differentiating ACLs. Rule being moved."); } - NetworkACLVO acl = _networkACLDao.findById(ruleAclId); + NetworkACLVO acl = _networkACLDao.findById(aclId); Account account = CallContext.current().getCallingAccount(); - long aclId = acl.getId(); if (aclId == NetworkACL.DEFAULT_ALLOW || aclId == NetworkACL.DEFAULT_DENY) { throw new InvalidParameterValueException("Default ACL rules cannot be moved."); From b839acdcabd0e4391487ecfa98866c178a2d4a05 Mon Sep 17 00:00:00 2001 From: Bryan Lima Date: Mon, 13 Feb 2023 12:47:00 -0300 Subject: [PATCH 05/14] Revert change --- .../main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java index a54a7a73ea07..da6db9edfb01 100644 --- a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java +++ b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java @@ -106,7 +106,7 @@ public class NetworkACLServiceImpl extends ManagerBase implements NetworkACLServ public NetworkACL createNetworkACL(final String name, final String description, final long vpcId, final Boolean forDisplay) { if (vpcId != 0) { final Account caller = CallContext.current().getCallingAccount(); - final Vpc vpc = _entityMgr.findById(Vpc.class, vpcId); + final Vpc vpc = _vpcDao.findById(vpcId); if (vpc == null) { throw new InvalidParameterValueException(String.format("Unable to find VPC with ID [%s].", vpcId)); } From 8ba0f5968a00dcb6099175eea3b2e07167901af6 Mon Sep 17 00:00:00 2001 From: Bryan Lima Date: Mon, 13 Feb 2023 13:41:51 -0300 Subject: [PATCH 06/14] Fix unit tests --- .../java/com/cloud/network/vpc/NetworkACLServiceImplTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java b/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java index 5142f14a3133..e5fa11b15cee 100644 --- a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java +++ b/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java @@ -106,7 +106,7 @@ public class NetworkACLServiceImplTest { @Mock private UpdateNetworkACLListCmd updateNetworkACLListCmdMock; - private Long networkAclMockId = 1L; + private Long networkAclMockId = 5L; private Long networkOfferingMockId = 2L; private Long networkMockVpcMockId = 3L; private long networkAclListId = 1l; @@ -1133,6 +1133,7 @@ public void validateMoveAclRulesDataTestSuccesfullExecution() { Mockito.doReturn(Mockito.mock(Account.class)).when(callContextMock).getCallingAccount(); Mockito.doReturn(networkAclMock).when(networkAclDaoMock).findById(networkAclMockId); + Mockito.when(networkAclMock.getId()).thenReturn(networkAclMockId); PowerMockito.mockStatic(CallContext.class); PowerMockito.when(CallContext.current()).thenReturn(callContextMock); From c25cbaf57afa70a46de739e99eb178850fd3f8a7 Mon Sep 17 00:00:00 2001 From: Bryan Lima Date: Wed, 15 Feb 2023 16:28:28 -0300 Subject: [PATCH 07/14] Address Daan's reviews --- test/integration/smoke/test_global_acls.py | 59 +++++++++++++++++----- 1 file changed, 46 insertions(+), 13 deletions(-) diff --git a/test/integration/smoke/test_global_acls.py b/test/integration/smoke/test_global_acls.py index 30a5b8a0b6b1..fe7d0fc329d3 100644 --- a/test/integration/smoke/test_global_acls.py +++ b/test/integration/smoke/test_global_acls.py @@ -67,11 +67,11 @@ class TestGlobalACLs(cloudstackTestCase): @classmethod def setUpClass(cls): cls.testClient = super(TestGlobalACLs, cls).getClsTestClient() - cls.api_client = cls.testClient.getApiClient() + cls.apiclient = cls.testClient.getApiClient() cls.services = Services().services - cls.domain = get_domain(cls.api_client) - cls.zone = get_zone(cls.api_client, cls.testClient.getZoneForTests()) + cls.domain = get_domain(cls.apiclient) + cls.zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests()) return @classmethod @@ -79,7 +79,6 @@ def tearDownClass(cls): super(TestGlobalACLs, cls).tearDownClass() def setUp(self): - self.apiclient = self.testClient.getApiClient() self.user_apiclient = self.testClient.getUserApiClient(self.services["user"]["username"], self.services["domain"]["name"], self.services["user"]["roletype"]) @@ -96,11 +95,7 @@ def setUp(self): return def tearDown(self): - try: - cleanup_resources(self.apiclient, self.cleanup) - cleanup_resources(self.user_apiclient, self.cleanup) - except Exception as e: - self.debug("Warning! Exception in tearDown: %s" % e) + super(TestGlobalACLs, cls).tearDown() @attr(tags=["advanced", "basic"], required_hardware="false") def test_create_global_acl(self): @@ -119,6 +114,7 @@ def test_create_global_acl(self): self.debug("Creating ACL list as a root admin, should work.") acl = NetworkACLList.create(apiclient=self.admin_apiclient, services={}, name="acl", description="acl") + self.cleanup.append(acl) self.assertIsNotNone(acl, "A root admin user should be able to create a global ACL.") return @@ -127,9 +123,9 @@ def test_create_global_acl(self): def test_replace_acl_of_network(self): """ Test to replace ACL of a VPC as a normal user, domain admin and root admin users. """ - # Create network offering + # Get network offering networkOffering = NetworkOffering.list(self.apiclient, name="DefaultIsolatedNetworkOfferingForVpcNetworks") - self.assertTrue(networkOffering is not None and len(networkOffering) > 0, "No VPC based network offering") + self.assertTrue(networkOffering is not None and len(networkOffering) > 0, "No VPC network offering") # Getting VPC offering vpcOffering = VpcOffering.list(self.apiclient, name="Default VPC offering") @@ -144,6 +140,7 @@ def test_replace_acl_of_network(self): zoneid=self.zone.id, domainid=self.domain.id ) + self.cleanup.append(vpc) self.assertTrue(vpc is not None, "VPC creation failed") # Creating ACL list @@ -162,6 +159,7 @@ def test_replace_acl_of_network(self): gateway="10.1.1.1", netmask="255.255.255.192" ) + self.cleanup.append(network) # User should be able to replace ACL network.replaceACLList(apiclient=self.user_apiclient, aclid=acl.id) @@ -178,6 +176,7 @@ def test_create_acl_rule(self): """ # Creating ACL list acl = NetworkACLList.create(apiclient=self.admin_apiclient, services={}, name="acl", description="acl") + self.cleanup.append(acl) self.debug("Creating ACL rule as a user, should raise exception.") self.assertRaisesRegex(CloudstackAPIException, "Only Root Admins can create rules for a global ACL.", @@ -186,7 +185,8 @@ def test_create_acl_rule(self): self.assertRaisesRegex(CloudstackAPIException, "Only Root Admins can create rules for a global ACL.", NetworkACL.create, self.domain_admin_apiclient, services=self.services["rule"], aclid=acl.id) self.debug("Creating ACL rule as a root admin, should work.") - NetworkACL.create(self.admin_apiclient, services=self.services["rule"], aclid=acl.id) + acl_rule = NetworkACL.create(self.admin_apiclient, services=self.services["rule"], aclid=acl.id) + self.cleanup.append(acl_rule) return @@ -195,7 +195,8 @@ def test_delete_acl_rule(self): """ Test to delete ACL rule as a normal user, domain admin and root admin users. """ # Creating ACL list - acl = NetworkACLList.create(apiclient=self.api_client, services={}, name="acl", description="acl") + acl = NetworkACLList.create(apiclient=self.apiclient, services={}, name="acl", description="acl") + self.cleanup.append(acl) # Creating ACL rule acl_rule = NetworkACL.create(self.apiclient, services=self.services["rule"], aclid=acl.id) @@ -206,7 +207,39 @@ def test_delete_acl_rule(self): self.debug("Deleting ACL rule as a domain admin, should raise exception.") self.assertRaisesRegex(Exception, "Only Root Admin can delete global ACL rules.", NetworkACL.delete, acl_rule, self.domain_admin_apiclient) + self.debug("Deleting ACL rule as a root admin, should work.") NetworkACL.delete(acl_rule, self.admin_apiclient) + # Verify if the number of ACL rules is equal to four, i.e. the number of rules + # for the default ACLs `default_allow` (2 rules) and `default_deny` (2 rules) ACLs + number_of_acl_rules = acl_rule.list(apiclient=self.admin_apiclient) + self.assertEqual(len(number_of_acl_rules), 4) + return + + + @attr(tags=["advanced", "basic"], required_hardware="false") + def test_delete_global_acl(self): + """ Test delete global ACL as a normal user, domain admin and root admin users. + """ + + # Creating ACL list. Not adding to cleanup as it will be deleted in this method + acl = NetworkACLList.create(apiclient=self.apiclient, services={}, name="acl", description="acl") + + self.debug("Deleting ACL list as a normal user, should raise exception.") + self.assertRaisesRegex(Exception, "Only Root Admin can delete global ACLs.", + NetworkACLList.delete, acl, apiclient=self.user_apiclient) + + self.debug("Deleting ACL list as a domain admin, should raise exception.") + self.assertRaisesRegex(Exception, "Only Root Admin can delete global ACLs.", + NetworkACLList.delete, acl, apiclient=self.domain_admin_apiclient) + + self.debug("Deleting ACL list as a root admin, should work.") + acl.delete(apiclient=self.admin_apiclient) + + # Verify if number of ACLs is equal to two, i.e. the number of default ACLs `default_allow` and `default_deny` + number_of_acls = NetworkACLList.list(apiclient=self.admin_apiclient) + self.assertEqual(len(number_of_acls), 2) + + return \ No newline at end of file From a033d55d2ea811f64b71a6b9f640d5bc8b8fd672 Mon Sep 17 00:00:00 2001 From: Bryan Lima Date: Wed, 15 Feb 2023 16:35:03 -0300 Subject: [PATCH 08/14] Fix no newline for python test --- test/integration/smoke/test_global_acls.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/smoke/test_global_acls.py b/test/integration/smoke/test_global_acls.py index fe7d0fc329d3..6baa5d865b21 100644 --- a/test/integration/smoke/test_global_acls.py +++ b/test/integration/smoke/test_global_acls.py @@ -242,4 +242,4 @@ def test_delete_global_acl(self): number_of_acls = NetworkACLList.list(apiclient=self.admin_apiclient) self.assertEqual(len(number_of_acls), 2) - return \ No newline at end of file + return From a1f4d79141c1425fba4f3021d52e9132731941a2 Mon Sep 17 00:00:00 2001 From: Bryan Lima <42067040+BryanMLima@users.noreply.github.com> Date: Thu, 16 Feb 2023 10:00:06 -0300 Subject: [PATCH 09/14] Apply suggestions from Daan review Co-authored-by: dahn --- test/integration/smoke/test_global_acls.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/integration/smoke/test_global_acls.py b/test/integration/smoke/test_global_acls.py index 6baa5d865b21..b3e33a7e16b3 100644 --- a/test/integration/smoke/test_global_acls.py +++ b/test/integration/smoke/test_global_acls.py @@ -200,6 +200,7 @@ def test_delete_acl_rule(self): # Creating ACL rule acl_rule = NetworkACL.create(self.apiclient, services=self.services["rule"], aclid=acl.id) + self.cleanup.append(acl_rule) self.debug("Deleting ACL rule as a user, should raise exception.") self.assertRaisesRegex(Exception, "Only Root Admin can delete global ACL rules.", @@ -210,6 +211,7 @@ def test_delete_acl_rule(self): self.debug("Deleting ACL rule as a root admin, should work.") NetworkACL.delete(acl_rule, self.admin_apiclient) + self.cleanup.remove(acl_rule) # Verify if the number of ACL rules is equal to four, i.e. the number of rules # for the default ACLs `default_allow` (2 rules) and `default_deny` (2 rules) ACLs @@ -226,6 +228,7 @@ def test_delete_global_acl(self): # Creating ACL list. Not adding to cleanup as it will be deleted in this method acl = NetworkACLList.create(apiclient=self.apiclient, services={}, name="acl", description="acl") + self.cleanup.append(acl) self.debug("Deleting ACL list as a normal user, should raise exception.") self.assertRaisesRegex(Exception, "Only Root Admin can delete global ACLs.", @@ -237,6 +240,7 @@ def test_delete_global_acl(self): self.debug("Deleting ACL list as a root admin, should work.") acl.delete(apiclient=self.admin_apiclient) + self.cleanup.remove(acl) # Verify if number of ACLs is equal to two, i.e. the number of default ACLs `default_allow` and `default_deny` number_of_acls = NetworkACLList.list(apiclient=self.admin_apiclient) From b7d5c243d2d0eb32d70eec577def4b282fd8efbc Mon Sep 17 00:00:00 2001 From: Bryan Lima Date: Mon, 20 Feb 2023 14:55:32 +0000 Subject: [PATCH 10/14] Fix marvin tests --- test/integration/smoke/test_global_acls.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/smoke/test_global_acls.py b/test/integration/smoke/test_global_acls.py index b3e33a7e16b3..53ae9905f33d 100644 --- a/test/integration/smoke/test_global_acls.py +++ b/test/integration/smoke/test_global_acls.py @@ -95,7 +95,7 @@ def setUp(self): return def tearDown(self): - super(TestGlobalACLs, cls).tearDown() + super(TestGlobalACLs, self).tearDown() @attr(tags=["advanced", "basic"], required_hardware="false") def test_create_global_acl(self): From 2cb979671242fa9fe1abeeecaaf37060f93197f5 Mon Sep 17 00:00:00 2001 From: Bryan Lima Date: Fri, 17 Mar 2023 17:22:17 -0300 Subject: [PATCH 11/14] Address reviews --- .../java/com/cloud/network/vpc/NetworkACLServiceImpl.java | 2 +- test/integration/smoke/test_global_acls.py | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java index da6db9edfb01..e0887098d91f 100644 --- a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java +++ b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java @@ -408,7 +408,7 @@ protected void validateAclRuleNumber(CreateNetworkACLCmd createNetworkAclCmd, Ne *
    *
  • If the parameter is null, we return an {@link InvalidParameterValueException}; *
  • Default ACLs {@link NetworkACL#DEFAULT_ALLOW} and {@link NetworkACL#DEFAULT_DENY} cannot be modified. Therefore, if any of them is provided we throw a {@link InvalidParameterValueException}; - *
  • If no VPC is given, then it is a global ACL and there is no need to check any VPC ID. However, if a VPC is given and it does not exist, we will throw an + *
  • If no VPC is given, then it is a global ACL and there is no need to check any VPC ID. However, if a VPC is given and it does not exist, throws an * {@link InvalidParameterValueException}. *
* diff --git a/test/integration/smoke/test_global_acls.py b/test/integration/smoke/test_global_acls.py index 53ae9905f33d..47db858c1214 100644 --- a/test/integration/smoke/test_global_acls.py +++ b/test/integration/smoke/test_global_acls.py @@ -74,10 +74,6 @@ def setUpClass(cls): cls.zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests()) return - @classmethod - def tearDownClass(cls): - super(TestGlobalACLs, cls).tearDownClass() - def setUp(self): self.user_apiclient = self.testClient.getUserApiClient(self.services["user"]["username"], self.services["domain"]["name"], From 77fc59d355fe1c339ca27e260e45afe1548e32fb Mon Sep 17 00:00:00 2001 From: Bryan Lima Date: Mon, 24 Apr 2023 11:08:54 -0300 Subject: [PATCH 12/14] Prevent global ACL removal upon VPC deletion --- .../src/main/java/com/cloud/network/vpc/VpcManagerImpl.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java b/server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java index 4456f10b5463..7e5206ac5086 100644 --- a/server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java +++ b/server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java @@ -1972,16 +1972,14 @@ public boolean cleanupVpcResources(final long vpcId, final Account caller, final searchBuilder.and("vpcId", searchBuilder.entity().getVpcId(), Op.IN); final SearchCriteria searchCriteria = searchBuilder.create(); - searchCriteria.setParameters("vpcId", vpcId, 0); + searchCriteria.setParameters("vpcId", vpcId); final Filter filter = new Filter(NetworkACLVO.class, "id", false, null, null); final Pair, Integer> aclsCountPair = _networkAclDao.searchAndCount(searchCriteria, filter); final List acls = aclsCountPair.first(); for (final NetworkACLVO networkAcl : acls) { - if (networkAcl.getId() != NetworkACL.DEFAULT_ALLOW && networkAcl.getId() != NetworkACL.DEFAULT_DENY) { - _networkAclMgr.deleteNetworkACL(networkAcl); - } + _networkAclMgr.deleteNetworkACL(networkAcl); } VpcVO vpc = vpcDao.findById(vpcId); From a04e1e0ad9314ea62abf9b641595f0956572a7ea Mon Sep 17 00:00:00 2001 From: Bryan Lima Date: Mon, 13 Nov 2023 16:32:37 -0300 Subject: [PATCH 13/14] Allow creating private gateway with global ACL --- .../network/vpc/NetworkACLServiceImpl.java | 17 ++++++++++------- .../com/cloud/network/vpc/VpcManagerImpl.java | 10 +++++++++- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java index e0887098d91f..8139ac1c49ec 100644 --- a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java +++ b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java @@ -222,8 +222,7 @@ public boolean deleteNetworkACL(final long id) { throw new InvalidParameterValueException("Unable to find specified ACL"); } - //Do not allow deletion of default ACLs - if (acl.getId() == NetworkACL.DEFAULT_ALLOW || acl.getId() == NetworkACL.DEFAULT_DENY) { + if (isDefaultAcl(acl.getId())) { throw new InvalidParameterValueException("Default ACL cannot be removed"); } @@ -253,7 +252,7 @@ public boolean replaceNetworkACLonPrivateGw(final long aclId, final long private throw new InvalidParameterValueException("Unable to find specified vpc id"); } - if (aclId != NetworkACL.DEFAULT_DENY && aclId != NetworkACL.DEFAULT_ALLOW) { + if (!isDefaultAcl(aclId)) { final Vpc vpc = _entityMgr.findById(Vpc.class, acl.getVpcId()); if (vpc == null) { throw new InvalidParameterValueException("Unable to find Vpc associated with the NetworkACL"); @@ -293,7 +292,7 @@ public boolean replaceNetworkACL(final long aclId, final long networkId) throws throw new InvalidParameterValueException("Network ACL can be created just for networks of type " + Networks.TrafficType.Guest); } - if (aclId != NetworkACL.DEFAULT_DENY && aclId != NetworkACL.DEFAULT_ALLOW && !isGlobalAcl(acl.getVpcId())) { + if (!isDefaultAcl(aclId) && !isGlobalAcl(acl.getVpcId())) { validateAclAssociatedToVpc(acl.getVpcId(), caller, acl.getUuid()); if (!network.getVpcId().equals(acl.getVpcId())) { @@ -419,7 +418,7 @@ protected void validateNetworkAcl(NetworkACL acl) { throw new InvalidParameterValueException("Unable to find specified ACL."); } - if (acl.getId() == NetworkACL.DEFAULT_DENY || acl.getId() == NetworkACL.DEFAULT_ALLOW) { + if (isDefaultAcl(acl.getId())) { throw new InvalidParameterValueException("Default ACL cannot be modified"); } @@ -792,7 +791,7 @@ public boolean revokeNetworkACLItem(final long ruleId) { final NetworkACL acl = _networkAclMgr.getNetworkACL(aclItem.getAclId()); final Account account = CallContext.current().getCallingAccount(); - if (aclItem.getAclId() == NetworkACL.DEFAULT_ALLOW || aclItem.getAclId() == NetworkACL.DEFAULT_DENY) { + if (isDefaultAcl(aclItem.getAclId())) { throw new InvalidParameterValueException("ACL Items in default ACL cannot be deleted"); } validateGlobalAclPermissionAndAclAssociatedToVpc(acl, account, "Only Root Admin can delete global ACL rules."); @@ -1149,7 +1148,7 @@ protected void validateMoveAclRulesData(NetworkACLItemVO ruleBeingMoved, Network NetworkACLVO acl = _networkACLDao.findById(aclId); Account account = CallContext.current().getCallingAccount(); - if (aclId == NetworkACL.DEFAULT_ALLOW || aclId == NetworkACL.DEFAULT_DENY) { + if (isDefaultAcl(aclId)) { throw new InvalidParameterValueException("Default ACL rules cannot be moved."); } @@ -1195,4 +1194,8 @@ protected void validateGlobalAclPermissionAndAclAssociatedToVpc(NetworkACL acl, protected boolean isGlobalAcl(Long aclVpcId) { return aclVpcId != null && aclVpcId == 0; } + + protected boolean isDefaultAcl(long aclId) { + return aclId == NetworkACL.DEFAULT_ALLOW || aclId == NetworkACL.DEFAULT_DENY; + } } diff --git a/server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java b/server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java index 7e5206ac5086..7985205f4c7e 100644 --- a/server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java +++ b/server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java @@ -2200,7 +2200,7 @@ private PrivateGateway createVpcPrivateGateway(final long vpcId, Long physicalNe if (aclVO == null) { throw new InvalidParameterValueException("Invalid network acl id passed "); } - if (aclVO.getVpcId() != vpcId && !(aclId == NetworkACL.DEFAULT_DENY || aclId == NetworkACL.DEFAULT_ALLOW)) { + if (aclVO.getVpcId() != vpcId && !isDefaultAcl(aclId) && !isGlobalAcl(aclVO.getVpcId())) { throw new InvalidParameterValueException("Private gateway and network acl are not in the same vpc"); } @@ -3154,4 +3154,12 @@ private List filterChildSubDomains(final List domainIds) { } return filteredDomainIds; } + + protected boolean isGlobalAcl(Long aclVpcId) { + return aclVpcId != null && aclVpcId == 0; + } + + protected boolean isDefaultAcl(long aclId) { + return aclId == NetworkACL.DEFAULT_ALLOW || aclId == NetworkACL.DEFAULT_DENY; + } } From f99fff6a2b09a36c9cc0815ae1be388cfbec7081 Mon Sep 17 00:00:00 2001 From: Bryan Lima Date: Fri, 17 Nov 2023 10:44:09 -0300 Subject: [PATCH 14/14] Add smoke test to github ci --- .github/workflows/ci.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c0422cd4bd0e..9df42f52a88a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -108,7 +108,8 @@ jobs: smoke/test_reset_configuration_settings smoke/test_reset_vm_on_reboot smoke/test_resource_accounting - smoke/test_resource_detail", + smoke/test_resource_detail + smoke/test_global_acls", "smoke/test_router_dhcphosts smoke/test_router_dns smoke/test_router_dnsservice