api, server: fix add-remove vpn user without vpn owner (apache#5850)

* api, server: fix add-remove vpn user without vpn owner

Fixes apache#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.

* change , fixes

* remove unused method

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

Author: shwstppr

api/src/main/java/com/cloud/network/vpn/RemoteAccessVpnService.java

@@ -43,6 +43,8 @@ public interface RemoteAccessVpnService {
 
     List<? extends VpnUser> listVpnUsers(long vpnOwnerId, String userName);
 
+    boolean applyVpnUsers(long vpnOwnerId, String userName, boolean forRemove) throws ResourceUnavailableException;
+
     boolean applyVpnUsers(long vpnOwnerId, String userName) throws ResourceUnavailableException;
 
     Pair<List<? extends RemoteAccessVpn>, Integer> searchForRemoteAccessVpns(ListRemoteAccessVpnsCmd cmd);

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);

server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java

@@ -16,23 +16,25 @@
 // 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;
 import org.apache.cloudstack.context.CallContext;
 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);
@@ -138,6 +137,24 @@ public class RemoteAccessVpnManagerImpl extends ManagerBase implements RemoteAcc
     int _pskLength;
     SearchBuilder<RemoteAccessVpnVO> VpnSearch;
 
+    private List<RemoteAccessVpnVO> getValidRemoteAccessVpnForAccount(long accountId) {
+        List<RemoteAccessVpnVO> vpns = _remoteAccessVpnDao.findByAccount(accountId);
+        if (CollectionUtils.isNotEmpty(vpns)) {
+            List<RemoteAccessVpnVO> validVpns = new ArrayList<>();
+            for (RemoteAccessVpnVO vpn : vpns) {
+                if (vpn.getNetworkId() != null) {
+                    Network network = _networkMgr.getNetwork(vpn.getNetworkId());
+                    if (!Network.State.Implemented.equals(network.getState())) {
+                        continue;
+                    }
+                }
+                validVpns.add(vpn);
+            }
+            vpns = validVpns;
+        }
+        return vpns;
+    }
+
     @Override
     @DB
     public RemoteAccessVpn createRemoteAccessVpn(final long publicIpId, String ipRange, boolean openFirewall, final Boolean forDisplay) throws NetworkRuleConflictException {
@@ -499,19 +516,36 @@ 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 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);
 
         s_logger.debug(String.format("Applying VPN users for %s.", owner.toString()));
-        List<RemoteAccessVpnVO> vpns = _remoteAccessVpnDao.findByAccount(vpnOwnerId);
+        List<RemoteAccessVpnVO> vpns = getValidRemoteAccessVpnForAccount(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()));
-            return false;
+            if (forRemove) {
+                return removeVpnUserWithoutRemoteAccessVpn(vpnOwnerId, userName);
+            }
+            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;
@@ -597,6 +631,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<List<? extends VpnUser>, Integer> searchForVpnUsers(ListVpnUsersCmd cmd) {
         String username = cmd.getUsername();