server: fix delete resource tag permission (#6634)

Fixes #6623
This PR fixes resource tag deletion behaviour. The permission check should be done only for the tags that are passed in the API call instead of checking for all the tags for the resource.

Author: shwstppr

server/src/main/java/com/cloud/tags/TaggedResourceManagerImpl.java

@@ -165,6 +165,22 @@ private Pair<Long, Long> getAccountDomain(long resourceId, ResourceObjectType re
         return new Pair<>(accountId, domainId);
     }
 
+    protected void checkTagsDeletePermission(List<ResourceTag> tagsToDelete, Account caller) {
+        for (ResourceTag resourceTag : tagsToDelete) {
+            if(s_logger.isDebugEnabled()) {
+                s_logger.debug("Resource Tag Id: " + resourceTag.getResourceId());
+                s_logger.debug("Resource Tag AccountId: " + resourceTag.getAccountId());
+            }
+            if (caller.getAccountId() != resourceTag.getAccountId()) {
+                Account owner = _accountMgr.getAccount(resourceTag.getAccountId());
+                if(s_logger.isDebugEnabled()) {
+                    s_logger.debug("Resource Owner: " + owner);
+                }
+                _accountMgr.checkAccess(caller, null, false, owner);
+            }
+        }
+    }
+
     @Override
     @DB
     @ActionEvent(eventType = EventTypes.EVENT_TAGS_CREATE, eventDescription = "creating resource tags")
@@ -241,17 +257,6 @@ public boolean deleteTags(List<String> resourceIds, ResourceObjectType resourceT
 
         // Finalize which tags should be removed
         for (ResourceTag resourceTag : resourceTags) {
-            //1) validate the permissions
-            if(s_logger.isDebugEnabled()) {
-                s_logger.debug("Resource Tag Id: " + resourceTag.getResourceId());
-                s_logger.debug("Resource Tag AccountId: " + resourceTag.getAccountId());
-            }
-            Account owner = _accountMgr.getAccount(resourceTag.getAccountId());
-            if(s_logger.isDebugEnabled()) {
-                s_logger.debug("Resource Owner: " + owner);
-            }
-            _accountMgr.checkAccess(caller, null, false, owner);
-            //2) Only remove tag if it matches key value pairs
             if (MapUtils.isEmpty(tags)) {
                 tagsToDelete.add(resourceTag);
             } else {
@@ -278,6 +283,7 @@ public boolean deleteTags(List<String> resourceIds, ResourceObjectType resourceT
         if (tagsToDelete.isEmpty()) {
             return false;
         }
+        checkTagsDeletePermission(tagsToDelete, caller);
 
         //Remove the tags
         Transaction.execute(new TransactionCallbackNoReturn() {

server/src/test/java/com/cloud/tags/TaggedResourceManagerImplTest.java

@@ -19,24 +19,37 @@
 
 package com.cloud.tags;
 
-import com.cloud.server.ResourceTag;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import junit.framework.TestCase;
+
 import org.junit.Assert;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.Spy;
 import org.mockito.junit.MockitoJUnitRunner;
 
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.server.ResourceTag;
+import com.cloud.user.Account;
+import com.cloud.user.AccountManager;
+
+import junit.framework.TestCase;
+
 @RunWith(MockitoJUnitRunner.class)
-public class TaggedResourceManagerImplTest extends TestCase{
+public class TaggedResourceManagerImplTest extends TestCase {
+
+    @Mock
+    AccountManager accountManager;
+
 
     @Spy
+    @InjectMocks
     private final TaggedResourceManagerImpl taggedResourceManagerImplSpy = new TaggedResourceManagerImpl();
 
     private final List<ResourceTag.ResourceObjectType> listResourceObjectTypes = Arrays.asList(ResourceTag.ResourceObjectType.values());
@@ -84,4 +97,30 @@ public void validateGetTagsFromResourceMustReturnEmpty(){
             Assert.assertEquals(expectedResult, result);
         });
     }
+
+    @Test
+    public void testCheckTagsDeletePermission() {
+        long accountId = 1L;
+        Account caller = Mockito.mock(Account.class);
+        Mockito.when(caller.getAccountId()).thenReturn(accountId);
+        ResourceTag resourceTag = Mockito.mock(ResourceTag.class);
+        Mockito.when(resourceTag.getAccountId()).thenReturn(accountId);
+        taggedResourceManagerImplSpy.checkTagsDeletePermission(List.of(resourceTag), caller);
+    }
+
+    @Test(expected = PermissionDeniedException.class)
+    public void testCheckTagsDeletePermissionFail() {
+        long callerAccountId = 1L;
+        long ownerAccountId = 2L;
+        Account caller = Mockito.mock(Account.class);
+        Mockito.when(caller.getAccountId()).thenReturn(callerAccountId);
+        ResourceTag resourceTag1 = Mockito.mock(ResourceTag.class);
+        Mockito.when(resourceTag1.getAccountId()).thenReturn(callerAccountId);
+        ResourceTag resourceTag2 = Mockito.mock(ResourceTag.class);
+        Mockito.when(resourceTag2.getAccountId()).thenReturn(ownerAccountId);
+        Account owner = Mockito.mock(Account.class);
+        Mockito.when(accountManager.getAccount(ownerAccountId)).thenReturn(owner);
+        Mockito.doThrow(PermissionDeniedException.class).when(accountManager).checkAccess(caller, null, false, owner);
+        taggedResourceManagerImplSpy.checkTagsDeletePermission(List.of(resourceTag1, resourceTag2), caller);
+    }
 }