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