From 02627ee100e004c168dd1a9575972f2d6d7c8a22 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Tue, 11 Jan 2022 15:39:06 +0530 Subject: [PATCH 1/4] api, server: fix add-remove vpn user without vpn owner Fixes #5711 ACS should not add a new user in Add state when the owner account does not have VPN access. While removing VPN user ACS should not fail completely when owner account ahs no VPN. Signed-off-by: Abhishek Kumar --- .../network/vpn/RemoteAccessVpnService.java | 4 ++ .../api/command/user/vpn/AddVpnUserCmd.java | 1 + .../command/user/vpn/RemoveVpnUserCmd.java | 3 +- .../vpn/RemoteAccessVpnManagerImpl.java | 42 +++++++++++++++---- 4 files changed, 41 insertions(+), 9 deletions(-) diff --git a/api/src/main/java/com/cloud/network/vpn/RemoteAccessVpnService.java b/api/src/main/java/com/cloud/network/vpn/RemoteAccessVpnService.java index 5426d181e70f..7622638dd31e 100644 --- a/api/src/main/java/com/cloud/network/vpn/RemoteAccessVpnService.java +++ b/api/src/main/java/com/cloud/network/vpn/RemoteAccessVpnService.java @@ -43,6 +43,10 @@ public interface RemoteAccessVpnService { List listVpnUsers(long vpnOwnerId, String userName); + void removeVpnUserWithoutValidVpnOwner(long id); + + boolean applyVpnUsers(long vpnOwnerId, String userName, boolean forRemove) throws ResourceUnavailableException; + boolean applyVpnUsers(long vpnOwnerId, String userName) throws ResourceUnavailableException; Pair, Integer> searchForRemoteAccessVpns(ListRemoteAccessVpnsCmd cmd); diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/vpn/AddVpnUserCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/vpn/AddVpnUserCmd.java index 0dba83b61b00..38df72361e32 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/vpn/AddVpnUserCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/vpn/AddVpnUserCmd.java @@ -121,6 +121,7 @@ public void execute() { Account account = _entityMgr.findById(Account.class, vpnUser.getAccountId()); try { if (!_ravService.applyVpnUsers(vpnUser.getAccountId(), userName)) { + _ravService.removeVpnUserWithoutValidVpnOwner(vpnUser.getId()); throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to add vpn user"); } } catch (Exception ex) { diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/vpn/RemoveVpnUserCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/vpn/RemoveVpnUserCmd.java index 33cbb46485c0..caeb0608b6a8 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/vpn/RemoveVpnUserCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/vpn/RemoveVpnUserCmd.java @@ -120,9 +120,8 @@ public void execute() { } boolean appliedVpnUsers = false; - try { - appliedVpnUsers = _ravService.applyVpnUsers(ownerId, userName); + appliedVpnUsers = _ravService.applyVpnUsers(ownerId, userName, true); } catch (ResourceUnavailableException ex) { String errorMessage = String.format("Failed to refresh VPN user=[%s] due to resource unavailable. VPN owner id=[%s].", userName, ownerId); s_logger.error(errorMessage, ex); diff --git a/server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java b/server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java index 74b53f14f076..406c2104985c 100644 --- a/server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java +++ b/server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java @@ -16,16 +16,16 @@ // under the License. package com.cloud.network.vpn; +import java.lang.reflect.InvocationTargetException; import java.util.ArrayList; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import javax.inject.Inject; import javax.naming.ConfigurationException; -import org.apache.log4j.Logger; - import org.apache.cloudstack.acl.SecurityChecker.AccessType; import org.apache.cloudstack.api.command.user.vpn.ListRemoteAccessVpnsCmd; import org.apache.cloudstack.api.command.user.vpn.ListVpnUsersCmd; @@ -33,6 +33,8 @@ import org.apache.cloudstack.framework.config.ConfigKey; import org.apache.cloudstack.framework.config.Configurable; import org.apache.cloudstack.framework.config.dao.ConfigurationDao; +import org.apache.commons.collections.CollectionUtils; +import org.apache.log4j.Logger; import com.cloud.configuration.Config; import com.cloud.domain.DomainVO; @@ -91,9 +93,6 @@ import com.cloud.utils.db.TransactionStatus; import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.net.NetUtils; -import java.lang.reflect.InvocationTargetException; -import java.util.stream.Collectors; -import org.apache.commons.collections.CollectionUtils; public class RemoteAccessVpnManagerImpl extends ManagerBase implements RemoteAccessVpnService, Configurable { private final static Logger s_logger = Logger.getLogger(RemoteAccessVpnManagerImpl.class); @@ -499,9 +498,29 @@ public void doInTransactionWithoutResult(TransactionStatus status) { } } + @DB + private boolean removeVpnUserWithoutRemoteAccessVpn(long vpnOwnerId, String userName) { + VpnUserVO vpnUser = _vpnUsersDao.findByAccountAndUsername(vpnOwnerId, userName); + if (vpnUser == null) { + s_logger.error(String.format("VPN user not found with ownerId: %d and username: %s", vpnOwnerId, userName)); + return false; + } + if (!State.Revoke.equals(vpnUser.getState())) { + s_logger.error(String.format("VPN user with ownerId: %d and username: %s is not in revoked state, current state: %s", vpnOwnerId, userName, vpnUser.getState())); + return false; + } + return _vpnUsersDao.remove(vpnUser.getId()); + } + @DB @Override - public boolean applyVpnUsers(long vpnOwnerId, String userName) throws ResourceUnavailableException { + public void removeVpnUserWithoutValidVpnOwner(long id) { + _vpnUsersDao.remove(id); + } + + @DB + @Override + public boolean applyVpnUsers(long vpnOwnerId, String userName, boolean forRemove) throws ResourceUnavailableException { Account caller = CallContext.current().getCallingAccount(); Account owner = _accountDao.findById(vpnOwnerId); _accountMgr.checkAccess(caller, null, true, owner); @@ -510,7 +529,10 @@ public boolean applyVpnUsers(long vpnOwnerId, String userName) throws ResourceU List vpns = _remoteAccessVpnDao.findByAccount(vpnOwnerId); if (CollectionUtils.isEmpty(vpns)) { - s_logger.debug(String.format("Unable to add VPN user due to there are no remote access VPNs configured on %s to apply VPN user.", owner.toString())); + if (forRemove) { + return removeVpnUserWithoutRemoteAccessVpn(vpnOwnerId, userName); + } + s_logger.error(String.format("Unable to add VPN user due to there are no remote access VPNs configured on %s to apply VPN user.", owner.toString())); return false; } @@ -597,6 +619,12 @@ public void doInTransactionWithoutResult(TransactionStatus status) { return success; } + @DB + @Override + public boolean applyVpnUsers(long vpnOwnerId, String userName) throws ResourceUnavailableException { + return applyVpnUsers(vpnOwnerId, userName, false); + } + @Override public Pair, Integer> searchForVpnUsers(ListVpnUsersCmd cmd) { String username = cmd.getUsername(); From 0dfcf8d8ce7721b1ebf3470538caaa3c3496586a Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Fri, 4 Feb 2022 17:57:22 +0530 Subject: [PATCH 2/4] change Signed-off-by: Abhishek Kumar --- .../api/command/user/vpn/AddVpnUserCmd.java | 1 - .../vpn/RemoteAccessVpnManagerImpl.java | 21 ++++++++++++++++--- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/vpn/AddVpnUserCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/vpn/AddVpnUserCmd.java index 38df72361e32..0dba83b61b00 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/vpn/AddVpnUserCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/vpn/AddVpnUserCmd.java @@ -121,7 +121,6 @@ public void execute() { Account account = _entityMgr.findById(Account.class, vpnUser.getAccountId()); try { if (!_ravService.applyVpnUsers(vpnUser.getAccountId(), userName)) { - _ravService.removeVpnUserWithoutValidVpnOwner(vpnUser.getId()); throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to add vpn user"); } } catch (Exception ex) { diff --git a/server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java b/server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java index 406c2104985c..9110838051c2 100644 --- a/server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java +++ b/server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java @@ -137,6 +137,21 @@ public class RemoteAccessVpnManagerImpl extends ManagerBase implements RemoteAcc int _pskLength; SearchBuilder VpnSearch; + private List getValidRemoteAccessVpnForAccount(long accountId) { + List vpns = _remoteAccessVpnDao.findByAccount(accountId); + if (CollectionUtils.isNotEmpty(vpns)) { + List validVpns = new ArrayList<>(); + for (RemoteAccessVpnVO vpn : vpns) { + Network network = _networkMgr.getNetwork(vpn.getNetworkId()); + if (Network.State.Implemented.equals(network.getState())) { + validVpns.add(vpn); + } + } + vpns = validVpns; + } + return vpns; + } + @Override @DB public RemoteAccessVpn createRemoteAccessVpn(final long publicIpId, String ipRange, boolean openFirewall, final Boolean forDisplay) throws NetworkRuleConflictException { @@ -526,14 +541,14 @@ public boolean applyVpnUsers(long vpnOwnerId, String userName, boolean forRemove _accountMgr.checkAccess(caller, null, true, owner); s_logger.debug(String.format("Applying VPN users for %s.", owner.toString())); - List vpns = _remoteAccessVpnDao.findByAccount(vpnOwnerId); + List vpns = getValidRemoteAccessVpnForAccount(vpnOwnerId); if (CollectionUtils.isEmpty(vpns)) { if (forRemove) { return removeVpnUserWithoutRemoteAccessVpn(vpnOwnerId, userName); } - s_logger.error(String.format("Unable to add VPN user due to there are no remote access VPNs configured on %s to apply VPN user.", owner.toString())); - return false; + s_logger.warn(String.format("Unable to apply VPN user due to there are no remote access VPNs configured on %s to apply VPN user.", owner.toString())); + return true; } RemoteAccessVpnVO vpnTemp = null; From c3420dd6b8a9f6b529c31ee35943a2db458cbab7 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Mon, 7 Feb 2022 11:53:17 +0530 Subject: [PATCH 3/4] fix Signed-off-by: Abhishek Kumar --- .../cloud/network/vpn/RemoteAccessVpnManagerImpl.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java b/server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java index 9110838051c2..ad397e3ca92d 100644 --- a/server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java +++ b/server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java @@ -142,10 +142,13 @@ private List getValidRemoteAccessVpnForAccount(long accountId if (CollectionUtils.isNotEmpty(vpns)) { List validVpns = new ArrayList<>(); for (RemoteAccessVpnVO vpn : vpns) { - Network network = _networkMgr.getNetwork(vpn.getNetworkId()); - if (Network.State.Implemented.equals(network.getState())) { - validVpns.add(vpn); + if (vpn.getNetworkId() != null) { + Network network = _networkMgr.getNetwork(vpn.getNetworkId()); + if (!Network.State.Implemented.equals(network.getState())) { + continue; + } } + validVpns.add(vpn); } vpns = validVpns; } From f272fba028089b615aecbcb5f094102500d5e6df Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Wed, 9 Feb 2022 16:27:15 +0530 Subject: [PATCH 4/4] remove unused method Signed-off-by: Abhishek Kumar --- .../java/com/cloud/network/vpn/RemoteAccessVpnService.java | 2 -- .../com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java | 6 ------ 2 files changed, 8 deletions(-) diff --git a/api/src/main/java/com/cloud/network/vpn/RemoteAccessVpnService.java b/api/src/main/java/com/cloud/network/vpn/RemoteAccessVpnService.java index 7622638dd31e..bbb9771d27aa 100644 --- a/api/src/main/java/com/cloud/network/vpn/RemoteAccessVpnService.java +++ b/api/src/main/java/com/cloud/network/vpn/RemoteAccessVpnService.java @@ -43,8 +43,6 @@ public interface RemoteAccessVpnService { List listVpnUsers(long vpnOwnerId, String userName); - void removeVpnUserWithoutValidVpnOwner(long id); - boolean applyVpnUsers(long vpnOwnerId, String userName, boolean forRemove) throws ResourceUnavailableException; boolean applyVpnUsers(long vpnOwnerId, String userName) throws ResourceUnavailableException; diff --git a/server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java b/server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java index ad397e3ca92d..61d247d7b8a5 100644 --- a/server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java +++ b/server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java @@ -530,12 +530,6 @@ private boolean removeVpnUserWithoutRemoteAccessVpn(long vpnOwnerId, String user return _vpnUsersDao.remove(vpnUser.getId()); } - @DB - @Override - public void removeVpnUserWithoutValidVpnOwner(long id) { - _vpnUsersDao.remove(id); - } - @DB @Override public boolean applyVpnUsers(long vpnOwnerId, String userName, boolean forRemove) throws ResourceUnavailableException {