engine: Check concrete disk permissions firstly on TransferImageStatu…

…sCommand There is some situations when storageDomainId is not come with parameters (image upload cancel and pause operations). For this operations checked CREATE_DISK permission on SYSTEM_OBJECT (i.e. system-wide). Problem starts when we give permissions for user only on concrete storage domain object (not system-wide). Then permission check failed for operations without storage domain id info in parameters. Here I just add check permission for disk before other objects. Signed-off-by: Stanislav Melnichuk <melnichuk.stas@gmail.com>
oVirt · Jun 10, 2024 · 1043c13 · 1043c13
1 parent a48bad9
commit 1043c13
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 12 deletions.
diff --git a/...rc/main/java/org/ovirt/engine/core/bll/storage/disk/image/TransferImageStatusCommand.java b/...rc/main/java/org/ovirt/engine/core/bll/storage/disk/image/TransferImageStatusCommand.java
@@ -5,6 +5,7 @@
 
 import javax.inject.Inject;
 
+import org.apache.commons.lang3.ObjectUtils;
 import org.ovirt.engine.core.bll.CommandBase;
 import org.ovirt.engine.core.bll.MultiLevelAdministrationHandler;
 import org.ovirt.engine.core.bll.NonTransactiveCommandAttribute;
@@ -104,16 +105,17 @@ protected void executeCommand() {
 
     @Override
     public List<PermissionSubject> getPermissionCheckSubjects() {
-        Guid objectId = getStorageDomainId();
-        if (objectId == null) {
-            objectId = getParameters().getStorageDomainId();
-            if (objectId != null) {
-                setStorageDomainId(objectId);
-            } else {
-                objectId = MultiLevelAdministrationHandler.SYSTEM_OBJECT_ID;
-            }
+        // Set storage domain id for audit logs.
+        if (getStorageDomainId() == null && getParameters().getStorageDomainId() != null) {
+            setStorageDomainId(getParameters().getStorageDomainId());
         }
 
+        Guid objectId = ObjectUtils.firstNonNull(
+                getParameters().getDiskId(),
+                getParameters().getStorageDomainId(),
+                MultiLevelAdministrationHandler.SYSTEM_OBJECT_ID
+        );
+
         // Only check generic permissions because the command and/or ImageUpload entity may be missing
         return Collections.singletonList(new PermissionSubject(
                 objectId,

diff --git a/...est/java/org/ovirt/engine/core/bll/storage/disk/image/TransferImageStatusCommandTest.java b/...est/java/org/ovirt/engine/core/bll/storage/disk/image/TransferImageStatusCommandTest.java
@@ -7,17 +7,22 @@
 import static org.mockito.Mockito.when;
 
 import java.util.List;
+import java.util.stream.Stream;
 
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.Spy;
 import org.mockito.junit.jupiter.MockitoSettings;
 import org.mockito.quality.Strictness;
 import org.ovirt.engine.core.bll.BaseCommandTest;
+import org.ovirt.engine.core.bll.MultiLevelAdministrationHandler;
 import org.ovirt.engine.core.bll.ValidateTestUtils;
 import org.ovirt.engine.core.bll.utils.PermissionSubject;
 import org.ovirt.engine.core.common.action.TransferImageStatusParameters;
@@ -46,7 +51,8 @@ public class TransferImageStatusCommandTest extends BaseCommandTest {
     private TransferImageStatusCommand<TransferImageStatusParameters> command =
             new TransferImageStatusCommand<>(new TransferImageStatusParameters(), null);
 
-    private Guid diskId = Guid.newGuid();
+    private static Guid diskId = Guid.newGuid();
+    private static Guid storageDomainId = Guid.newGuid();
 
     @BeforeEach
     public void setUp() {
@@ -57,14 +63,33 @@ public void setUp() {
     }
 
     @Test
-    public void testCorrectPermissionCheckSubjects() {
-        Guid storageDomainId = Guid.newGuid();
+    void testBaseExecution() {
         command.getParameters().setStorageDomainId(storageDomainId);
         command.getParameters().setDiskId(diskId);
         List<PermissionSubject> subjects = command.getPermissionCheckSubjects();
         PermissionSubject subject = subjects.get(0);
-        assertEquals(storageDomainId, subject.getObjectId());
+        assertEquals(diskId, subject.getObjectId());
         ValidateTestUtils.runAndAssertValidateSuccess(command);
         command.executeCommand();
     }
+
+    @ParameterizedTest
+    @MethodSource("commandCheckPermissionTestParameters")
+    public void testCorrectPermissionCheckSubjects(Guid inputDiskId, Guid inputStorageDomainId, Guid expectedPermissionSubjectId) {
+        command.getParameters().setStorageDomainId(inputStorageDomainId);
+        command.getParameters().setDiskId(inputDiskId);
+
+        List<PermissionSubject> subjects = command.getPermissionCheckSubjects();
+        PermissionSubject subject = subjects.get(0);
+
+        assertEquals(expectedPermissionSubjectId, subject.getObjectId());
+    }
+
+    private static Stream<Arguments> commandCheckPermissionTestParameters() {
+        return Stream.of(
+                Arguments.of(diskId, storageDomainId, diskId),
+                Arguments.of(null, storageDomainId, storageDomainId),
+                Arguments.of(null, null, MultiLevelAdministrationHandler.SYSTEM_OBJECT_ID)
+        );
+    }
 }