Fix concurrency problem when moving ACL rules with drag&drop

There was a concurrency problem with the “moveNetworkAclItem” API method. If two users were changing the ACL rules order at the same time, this could lead to inconsistent actions.
To solve the problem we added a “consistency check ” parameter, which is used to hold the consistency hash. This hash is created using an MD5 hash function on a String that is created with all ACL rules UUIDs concatenated in their order, which is defined via the ‘number’ field.
We also lock the editing of the ACL while executing the upgrade. This allows us to handle race conditions nicely, and present a good feedback for the user.

Author: rafaelweingartner

api/src/main/java/org/apache/cloudstack/api/ApiConstants.java

@@ -155,6 +155,7 @@ public class ApiConstants {
     public static final String IDS = "ids";
     public static final String PREVIOUS_ACL_RULE_ID = "previousaclruleid";
     public static final String NEXT_ACL_RULE_ID = "nextaclruleid";
+    public static final String MOVE_ACL_CONSISTENCY_HASH = "aclconsistencyhash";
     public static final String INTERNAL_DNS1 = "internaldns1";
     public static final String INTERNAL_DNS2 = "internaldns2";
     public static final String INTERVAL_TYPE = "intervaltype";

api/src/main/java/org/apache/cloudstack/api/command/user/network/MoveNetworkAclItemCmd.java

@@ -43,6 +43,9 @@ public class MoveNetworkAclItemCmd extends BaseAsyncCustomIdCmd {
     @Parameter(name = ApiConstants.NEXT_ACL_RULE_ID, type = CommandType.STRING, description = "The ID of the rule that is right after the new position where the rule being moved is going to be placed. This value can be 'NULL' if the rule is being moved to the last position of the network ACL list.")
     private String nextAclRuleUuid;
 
+    @Parameter(name = ApiConstants.MOVE_ACL_CONSISTENCY_HASH, type = CommandType.STRING, description = "Md5 hash used to check the consistency of the ACL rule list before applying the ACL rule move. This check is useful to manage concurrency problems that may happen when multiple users are editing the same ACL rule listing. The parameter is not required. Therefore, if the user does not send it, he/she is assuming the risk of moving ACL rules without checking the consistency of the access control list before executing the move. We use MD5 hash function on a String that is composed of all UUIDs of the ACL rules in concatenated in their respective order (order defined via 'number' field).")
+    private String aclConsistencyHash;
+
     @Override
     public void execute() {
         CallContext.current().setEventDetails(getEventDescription());
@@ -93,4 +96,8 @@ public long getEntityOwnerId() {
         Account caller = CallContext.current().getCallingAccount();
         return caller.getAccountId();
     }
+
+    public String getAclConsistencyHash() {
+        return aclConsistencyHash;
+    }
 }

server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java

@@ -33,6 +33,7 @@
 import org.apache.cloudstack.api.command.user.network.UpdateNetworkACLItemCmd;
 import org.apache.cloudstack.api.command.user.network.UpdateNetworkACLListCmd;
 import org.apache.cloudstack.context.CallContext;
+import org.apache.commons.codec.digest.DigestUtils;
 import org.apache.commons.collections.CollectionUtils;
 import org.apache.commons.lang.StringUtils;
 import org.apache.log4j.Logger;
@@ -58,6 +59,7 @@
 import com.cloud.tags.dao.ResourceTagDao;
 import com.cloud.user.Account;
 import com.cloud.user.AccountManager;
+import com.cloud.user.User;
 import com.cloud.utils.Pair;
 import com.cloud.utils.Ternary;
 import com.cloud.utils.component.ManagerBase;
@@ -958,15 +960,56 @@ public NetworkACLItem moveNetworkAclRuleToNewPosition(MoveNetworkAclItemCmd move
 
         validateMoveAclRulesData(ruleBeingMoved, previousRule, nextRule);
 
-        List<NetworkACLItemVO> allAclRules = getAllAclRulesSortedByNumber(ruleBeingMoved.getAclId());
-        if (previousRule == null) {
-            return moveRuleToTheTop(ruleBeingMoved, allAclRules);
+        try {
+            NetworkACLVO lockedAcl = _networkACLDao.acquireInLockTable(ruleBeingMoved.getAclId());
+            List<NetworkACLItemVO> allAclRules = getAllAclRulesSortedByNumber(lockedAcl.getId());
+            validateAclConsistency(moveNetworkAclItemCmd, lockedAcl, allAclRules);
+
+            if (previousRule == null) {
+                return moveRuleToTheTop(ruleBeingMoved, allAclRules);
+            }
+            if (nextRule == null) {
+                return moveRuleToTheBottom(ruleBeingMoved, allAclRules);
+            }
+            return moveRuleBetweenAclRules(ruleBeingMoved, allAclRules, previousRule, nextRule);
+        } finally {
+            _networkACLDao.releaseFromLockTable(ruleBeingMoved.getAclId());
         }
-        if (nextRule == null) {
-            return moveRuleToTheBottom(ruleBeingMoved, allAclRules);
+    }
+
+    /**
+     * Validates the consistency of the ACL; the validation process is the following.
+     * <ul>
+     *  <li> If the ACL does not have rules yet, we do not have any validation to perform;
+     *  <li> we will check first if the user provided a consistency hash; if not, we will log a warning message informing administrators that the user is performing the call is assuming the risks of applying ACL replacement without a consistency check;
+     *  <li> if the ACL consistency hash is entered by the user, we check if it is the same as we currently have in the database. If it is different we throw an exception.
+     * </ul>
+     *
+     * If the consistency hash sent by the user is the same as the one we get with the database data we should be safe to proceed.
+     */
+    protected void validateAclConsistency(MoveNetworkAclItemCmd moveNetworkAclItemCmd, NetworkACLVO lockedAcl, List<NetworkACLItemVO> allAclRules) {
+        if (CollectionUtils.isEmpty(allAclRules)) {
+            s_logger.debug(String.format("No ACL rules for [id=%s, name=%s]. Therefore, there is no need for consistency validation.", lockedAcl.getUuid(), lockedAcl.getName()));
+            return;
         }
+        String aclConsistencyHash = moveNetworkAclItemCmd.getAclConsistencyHash();
+        if (StringUtils.isBlank(aclConsistencyHash)) {
+            User callingUser = CallContext.current().getCallingUser();
+            Account callingAccount = CallContext.current().getCallingAccount();
 
-        return moveRuleBetweenAclRules(ruleBeingMoved, allAclRules, previousRule, nextRule);
+            s_logger.warn(String.format(
+                    "User [id=%s, name=%s] from Account [id=%s, name=%s] has not entered an ACL consistency hash to execute the replacement of an ACL rule. Therefore, she/he is assuming all of the risks of procedding without this validation.",
+                    callingUser.getUuid(), callingUser.getUsername(), callingAccount.getUuid(), callingAccount.getAccountName()));
+            return;
+        }
+        String aclRulesUuids = StringUtils.EMPTY;
+        for (NetworkACLItemVO rule : allAclRules) {
+            aclRulesUuids += rule.getUuid();
+        }
+        String md5UuidsSortedByNumber = DigestUtils.md5Hex(aclRulesUuids);
+        if (!md5UuidsSortedByNumber.equals(aclConsistencyHash)) {
+            throw new InvalidParameterValueException("It seems that the access control list in the database is not in the state that you used to apply the changed. Could you try it again?");
+        }
     }
 
     /**

server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java

@@ -56,6 +56,7 @@
 import com.cloud.network.vpc.dao.NetworkACLDao;
 import com.cloud.user.Account;
 import com.cloud.user.AccountManager;
+import com.cloud.user.User;
 import com.cloud.utils.db.EntityManager;
 import com.cloud.utils.exception.CloudRuntimeException;
 
@@ -113,10 +114,15 @@ public class NetworkACLServiceImplTest {
     private NetworkACLItemVO nextAclRuleMock;
     private String nextAclRuleUuid = "uuidNextAclRule";
 
+    @Mock
+    private CallContext callContextMock;
+
     @Before
     public void befoteTest() {
         PowerMockito.mockStatic(CallContext.class);
-        PowerMockito.when(CallContext.current()).thenReturn(Mockito.mock(CallContext.class));
+        PowerMockito.when(CallContext.current()).thenReturn(callContextMock);
+        Mockito.doReturn(Mockito.mock(User.class)).when(callContextMock).getCallingUser();
+        Mockito.doReturn(Mockito.mock(Account.class)).when(callContextMock).getCallingAccount();
 
         Mockito.when(networkAclDaoMock.findById(networkAclListId)).thenReturn(networkACLVOMock);
         Mockito.when(createNetworkAclCmdMock.getNetworkId()).thenReturn(1L);
@@ -936,6 +942,8 @@ public void moveNetworkAclRuleToNewPositionTestMoveRuleToTop() {
         Mockito.verify(networkAclServiceImpl, Mockito.times(0)).moveRuleToTheBottom(Mockito.eq(aclRuleBeingMovedMock), Mockito.anyListOf(NetworkACLItemVO.class));
         Mockito.verify(networkAclServiceImpl, Mockito.times(0)).moveRuleBetweenAclRules(Mockito.eq(aclRuleBeingMovedMock), Mockito.anyListOf(NetworkACLItemVO.class), Mockito.eq(previousAclRuleMock),
                 Mockito.eq(nextAclRuleMock));
+        Mockito.verify(networkAclServiceImpl, Mockito.times(1)).validateAclConsistency(Mockito.any(MoveNetworkAclItemCmd.class), Mockito.any(NetworkACLVO.class),
+                Mockito.anyListOf(NetworkACLItemVO.class));
     }
 
     @Test
@@ -957,6 +965,8 @@ public void moveNetworkAclRuleToNewPositionTestMoveRuleToBottom() {
         Mockito.verify(networkAclServiceImpl, Mockito.times(1)).moveRuleToTheBottom(Mockito.eq(aclRuleBeingMovedMock), Mockito.anyListOf(NetworkACLItemVO.class));
         Mockito.verify(networkAclServiceImpl, Mockito.times(0)).moveRuleBetweenAclRules(Mockito.eq(aclRuleBeingMovedMock), Mockito.anyListOf(NetworkACLItemVO.class), Mockito.eq(previousAclRuleMock),
                 Mockito.eq(nextAclRuleMock));
+        Mockito.verify(networkAclServiceImpl, Mockito.times(1)).validateAclConsistency(Mockito.any(MoveNetworkAclItemCmd.class), Mockito.any(NetworkACLVO.class),
+                Mockito.anyListOf(NetworkACLItemVO.class));
     }
 
     @Test
@@ -978,11 +988,17 @@ public void moveNetworkAclRuleToNewPositionTestMoveBetweenAclRules() {
         Mockito.verify(networkAclServiceImpl, Mockito.times(0)).moveRuleToTheBottom(Mockito.eq(aclRuleBeingMovedMock), Mockito.anyListOf(NetworkACLItemVO.class));
         Mockito.verify(networkAclServiceImpl, Mockito.times(1)).moveRuleBetweenAclRules(Mockito.eq(aclRuleBeingMovedMock), Mockito.anyListOf(NetworkACLItemVO.class), Mockito.eq(previousAclRuleMock),
                 Mockito.eq(nextAclRuleMock));
+        Mockito.verify(networkAclServiceImpl, Mockito.times(1)).validateAclConsistency(Mockito.any(MoveNetworkAclItemCmd.class), Mockito.any(NetworkACLVO.class),
+                Mockito.anyListOf(NetworkACLItemVO.class));
     }
 
     private void configureMoveMethodsToDoNothing() {
-        Mockito.doReturn(new ArrayList<>()).when(networkAclServiceImpl).getAllAclRulesSortedByNumber(networkAclMockId);
+        Mockito.doReturn(networkACLVOMock).when(networkAclDaoMock).acquireInLockTable(Mockito.anyLong());
+        Mockito.doReturn(true).when(networkAclDaoMock).releaseFromLockTable(Mockito.anyLong());
+
+        Mockito.doNothing().when(networkAclServiceImpl).validateAclConsistency(Mockito.any(MoveNetworkAclItemCmd.class), Mockito.any(NetworkACLVO.class), Mockito.anyListOf(NetworkACLItemVO.class));
 
+        Mockito.doReturn(new ArrayList<>()).when(networkAclServiceImpl).getAllAclRulesSortedByNumber(networkAclMockId);
         Mockito.doReturn(aclRuleBeingMovedMock).when(networkAclServiceImpl).moveRuleToTheTop(Mockito.eq(aclRuleBeingMovedMock), Mockito.anyListOf(NetworkACLItemVO.class));
         Mockito.doReturn(aclRuleBeingMovedMock).when(networkAclServiceImpl).moveRuleToTheBottom(Mockito.eq(aclRuleBeingMovedMock), Mockito.anyListOf(NetworkACLItemVO.class));
         Mockito.doReturn(aclRuleBeingMovedMock).when(networkAclServiceImpl).moveRuleBetweenAclRules(Mockito.eq(aclRuleBeingMovedMock), Mockito.anyListOf(NetworkACLItemVO.class),
@@ -1281,4 +1297,73 @@ public void updateAclRuleToNewPositionAndExecuteShiftIfNecessaryTest() {
         Assert.assertEquals(14, networkACLItemVO13.getNumber());
         Assert.assertEquals(15, networkACLItemVO14.getNumber());
     }
+
+    @Test
+    public void validateAclConsistencyTestRuleListEmpty() {
+        networkAclServiceImpl.validateAclConsistency(moveNetworkAclItemCmdMock, networkACLVOMock, new ArrayList<>());
+
+        Mockito.verify(moveNetworkAclItemCmdMock, Mockito.times(0)).getAclConsistencyHash();
+    }
+
+    @Test
+    public void validateAclConsistencyTestRuleListNull() {
+        networkAclServiceImpl.validateAclConsistency(moveNetworkAclItemCmdMock, networkACLVOMock, null);
+
+        Mockito.verify(moveNetworkAclItemCmdMock, Mockito.times(0)).getAclConsistencyHash();
+    }
+
+    @Test
+    public void validateAclConsistencyTestAclConsistencyHashIsNull() {
+        Mockito.doReturn(null).when(moveNetworkAclItemCmdMock).getAclConsistencyHash();
+
+        validateAclConsistencyTestAclConsistencyHashBlank();
+    }
+
+    @Test
+    public void validateAclConsistencyTestAclConsistencyHashIsEmpty() {
+        Mockito.doReturn("").when(moveNetworkAclItemCmdMock).getAclConsistencyHash();
+
+        validateAclConsistencyTestAclConsistencyHashBlank();
+    }
+
+    @Test
+    public void validateAclConsistencyTestAclConsistencyHashIsBlank() {
+        Mockito.doReturn("            ").when(moveNetworkAclItemCmdMock).getAclConsistencyHash();
+
+        validateAclConsistencyTestAclConsistencyHashBlank();
+    }
+
+    private void validateAclConsistencyTestAclConsistencyHashBlank() {
+        ArrayList<NetworkACLItemVO> allAclRules = new ArrayList<>();
+        allAclRules.add(networkAclItemVoMock);
+
+        networkAclServiceImpl.validateAclConsistency(moveNetworkAclItemCmdMock, networkACLVOMock, allAclRules);
+
+        Mockito.verify(moveNetworkAclItemCmdMock, Mockito.times(1)).getAclConsistencyHash();
+        Mockito.verify(callContextMock, Mockito.times(1)).getCallingAccount();
+        Mockito.verify(callContextMock, Mockito.times(1)).getCallingUser();
+    }
+
+    @Test(expected = InvalidParameterValueException.class)
+    public void validateAclConsistencyTestAclConsistencyHashIsNotEqualsToDatabaseHash() {
+        Mockito.doReturn("differentHash").when(moveNetworkAclItemCmdMock).getAclConsistencyHash();
+
+        ArrayList<NetworkACLItemVO> allAclRules = new ArrayList<>();
+        allAclRules.add(networkAclItemVoMock);
+
+        networkAclServiceImpl.validateAclConsistency(moveNetworkAclItemCmdMock, networkACLVOMock, allAclRules);
+    }
+
+    @Test
+    public void validateAclConsistencyTest() {
+        Mockito.doReturn("eac527fe45c77232ef06d9c7eb8abd94").when(moveNetworkAclItemCmdMock).getAclConsistencyHash();
+
+        ArrayList<NetworkACLItemVO> allAclRules = new ArrayList<>();
+        allAclRules.add(networkAclItemVoMock);
+
+        Mockito.doReturn("someUuid").when(networkAclItemVoMock).getUuid();
+        networkAclServiceImpl.validateAclConsistency(moveNetworkAclItemCmdMock, networkACLVOMock, allAclRules);
+
+        Mockito.verify(moveNetworkAclItemCmdMock, Mockito.times(1)).getAclConsistencyHash();
+    }
 }

ui/scripts/vpc.js

@@ -15,6 +15,10 @@
 // specific language governing permissions and limitations
 // under the License.
 (function($, cloudStack) {
+    //The drag and drop function to order ACL rules does not have access to the whole ACL.
+    //Therefore, we store the "state-hash" of the list being displayed for use in the drag and drop function.
+    var accessControlListConsistentyHashForDragAndDropFunction = "";
+
     var isNumeric = function (n) {
         return !isNaN(parseFloat(n));
     };
@@ -544,7 +548,8 @@
                         data: {
                             id: rule.id,
                             previousaclruleid: previousRuleId, 
-                            nextaclruleid: nextRuleId
+                            nextaclruleid: nextRuleId,
+                            aclconsistencyhash: accessControlListConsistentyHashForDragAndDropFunction
                         },
                         success: function(json) {
                             var pollTimer = setInterval(function() {
@@ -1415,6 +1420,11 @@
 
                                                             return acl;
                                                         });
+                                                        var allUuids = '';
+                                                        items.forEach(function(aclRule){
+                                                                            allUuids += aclRule.id;
+                                                                      });
+                                                        accessControlListConsistentyHashForDragAndDropFunction = $.md5(allUuids);
                                                     }
 
                                                     args.response.success({