You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by sh...@apache.org on 2022/08/17 06:27:49 UTC
[cloudstack] branch 4.17 updated: server: fix delete resource tag permission (#6634)
This is an automated email from the ASF dual-hosted git repository.
shwstppr pushed a commit to branch 4.17
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/4.17 by this push:
new 547041646ab server: fix delete resource tag permission (#6634)
547041646ab is described below
commit 547041646aba893f91e48a65f8262053eaf0cf49
Author: Abhishek Kumar <ab...@gmail.com>
AuthorDate: Wed Aug 17 11:57:44 2022 +0530
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.
---
.../com/cloud/tags/TaggedResourceManagerImpl.java | 28 ++++++++------
.../cloud/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<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 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<ResourceTag.ResourceObjectType> 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);
+ }
}