From 547041646aba893f91e48a65f8262053eaf0cf49 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Wed, 17 Aug 2022 11:57:44 +0530 Subject: [PATCH] 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. --- .../cloud/tags/TaggedResourceManagerImpl.java | 28 +++++++----- .../tags/TaggedResourceManagerImplTest.java | 45 +++++++++++++++++-- 2 files changed, 59 insertions(+), 14 deletions(-) diff --git a/server/src/main/java/com/cloud/tags/TaggedResourceManagerImpl.java b/server/src/main/java/com/cloud/tags/TaggedResourceManagerImpl.java index 23e757dfe23..c855c9efa30 100644 --- a/server/src/main/java/com/cloud/tags/TaggedResourceManagerImpl.java +++ b/server/src/main/java/com/cloud/tags/TaggedResourceManagerImpl.java @@ -165,6 +165,22 @@ public class TaggedResourceManagerImpl extends ManagerBase implements TaggedReso return new Pair<>(accountId, domainId); } + protected void checkTagsDeletePermission(List 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 class TaggedResourceManagerImpl extends ManagerBase implements TaggedReso // 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 class TaggedResourceManagerImpl extends ManagerBase implements TaggedReso if (tagsToDelete.isEmpty()) { return false; } + checkTagsDeletePermission(tagsToDelete, caller); //Remove the tags Transaction.execute(new TransactionCallbackNoReturn() { diff --git a/server/src/test/java/com/cloud/tags/TaggedResourceManagerImplTest.java b/server/src/test/java/com/cloud/tags/TaggedResourceManagerImplTest.java index d3a76ff429f..6d5b314263c 100644 --- a/server/src/test/java/com/cloud/tags/TaggedResourceManagerImplTest.java +++ b/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 listResourceObjectTypes = Arrays.asList(ResourceTag.ResourceObjectType.values()); @@ -84,4 +97,30 @@ public class TaggedResourceManagerImplTest extends TestCase{ 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); + } }