Fix handling autz of so-called elaborate items

When an item is marked elaborate, it is considered as too complex to be fully processed by various mechanisms in midPoint, including authorizations processing. However, the original implementation needlessly skipped _any_ autz processing for these items, allowing the access even if it was obviously denied. This commit fixes this by doing at least item-level authorization checks for these elaborate items. Only the "deep dive" is disabled for them. This resolves MID-8635.
Evolveum · Apr 3, 2023 · 131cb46 · 131cb46
1 parent 9857de7
commit 131cb46
Show file tree

Hide file tree

Showing 6 changed files with 77 additions and 26 deletions.
diff --git a/...del-impl/src/main/java/com/evolveum/midpoint/model/impl/controller/SchemaTransformer.java b/...del-impl/src/main/java/com/evolveum/midpoint/model/impl/controller/SchemaTransformer.java
@@ -390,7 +390,9 @@ private <O extends ObjectType> ObjectSecurityConstraints compileSecurityConstrai
         }
     }
 
-    private void applySecurityConstraints(PrismContainerValue<?> pcv, ObjectSecurityConstraints securityConstraints, AuthorizationPhaseType phase,
+    /** The variant of `applySecurityConstraints` for a {@link PrismContainerValue}. */
+    private void applySecurityConstraints(
+            PrismContainerValue<?> pcv, ObjectSecurityConstraints securityConstraints, AuthorizationPhaseType phase,
             AuthorizationDecisionType defaultReadDecision, AuthorizationDecisionType defaultAddDecision,
             AuthorizationDecisionType defaultModifyDecision, boolean applyToDefinitions) {
         LOGGER.trace("applySecurityConstraints(items): items={}, phase={}, defaults R={}, A={}, M={}",
@@ -402,10 +404,6 @@ private void applySecurityConstraints(PrismContainerValue<?> pcv, ObjectSecurity
         for (Item<?, ?> item : pcv.getItems()) {
             ItemPath itemPath = item.getPath();
             ItemDefinition<?> itemDef = item.getDefinition();
-            if (itemDef != null && itemDef.isElaborate()) {
-                LOGGER.trace("applySecurityConstraints(item): {}: skip (elaborate)", itemPath);
-                continue;
-            }
             ItemPath nameOnlyItemPath = itemPath.namedSegmentsOnly();
             AuthorizationDecisionType itemReadDecision = computeItemDecision(securityConstraints, nameOnlyItemPath, ModelAuthorizationAction.AUTZ_ACTIONS_URLS_GET, defaultReadDecision, phase);
             AuthorizationDecisionType itemAddDecision = computeItemDecision(securityConstraints, nameOnlyItemPath, ModelAuthorizationAction.AUTZ_ACTIONS_URLS_ADD, defaultReadDecision, phase);
@@ -428,6 +426,15 @@ private void applySecurityConstraints(PrismContainerValue<?> pcv, ObjectSecurity
                 if (itemReadDecision == AuthorizationDecisionType.DENY) {
                     // Explicitly denied access to the entire container
                     itemsToRemove.add(item);
+                } else if (itemDef != null && itemDef.isElaborate()) {
+                    LOGGER.trace("applySecurityConstraints(item): {}: skipping deeper dive, applying decision '{}' (elaborate)",
+                            itemPath, itemReadDecision);
+                    if (itemReadDecision == null) {
+                        // "default" (null) means we do not analyze things further, and simply deny the access completely
+                        itemsToRemove.add(item);
+                    } else {
+                        // ALLOW means we do not analyze things further, and simply allow the access completely
+                    }
                 } else {
                     // No explicit decision (even ALLOW is not final here as something may be denied deeper inside)
                     AuthorizationDecisionType subDefaultReadDecision = defaultReadDecision;
@@ -477,8 +484,11 @@ private MutableItemDefinition<?> mutable(ItemDefinition<?> itemDef) {
         return itemDef.toMutable();
     }
 
-    private <O extends ObjectType> void applySecurityConstraints(ObjectDelta<O> objectDelta, ObjectSecurityConstraints securityConstraints, AuthorizationPhaseType phase,
-            AuthorizationDecisionType defaultReadDecision, AuthorizationDecisionType defaultAddDecision, AuthorizationDecisionType defaultModifyDecision) {
+    /** The variant of `applySecurityConstraints` for an {@link ObjectDelta}. */
+    private <O extends ObjectType> void applySecurityConstraints(
+            ObjectDelta<O> objectDelta, ObjectSecurityConstraints securityConstraints, AuthorizationPhaseType phase,
+            AuthorizationDecisionType defaultReadDecision, AuthorizationDecisionType defaultAddDecision,
+            AuthorizationDecisionType defaultModifyDecision) {
         LOGGER.trace("applySecurityConstraints(objectDelta): items={}, phase={}, defaults R={}, A={}, M={}",
                 objectDelta, phase, defaultReadDecision, defaultAddDecision, defaultModifyDecision);
         if (objectDelta == null) {
@@ -503,10 +513,6 @@ private <O extends ObjectType> void applySecurityConstraints(ObjectDelta<O> obje
         for (ItemDelta<?, ?> modification : modifications) {
             ItemPath itemPath = modification.getPath();
             ItemDefinition<?> itemDef = modification.getDefinition();
-            if (itemDef != null && itemDef.isElaborate()) {
-                LOGGER.trace("applySecurityConstraints(item): {}: skip (elaborate)", itemPath);
-                continue;
-            }
             ItemPath nameOnlyItemPath = itemPath.namedSegmentsOnly();
             AuthorizationDecisionType itemReadDecision = computeItemDecision(securityConstraints, nameOnlyItemPath, ModelAuthorizationAction.AUTZ_ACTIONS_URLS_GET, defaultReadDecision, phase);
             AuthorizationDecisionType itemAddDecision = computeItemDecision(securityConstraints, nameOnlyItemPath, ModelAuthorizationAction.AUTZ_ACTIONS_URLS_ADD, defaultReadDecision, phase);
@@ -517,6 +523,13 @@ private <O extends ObjectType> void applySecurityConstraints(ObjectDelta<O> obje
                 if (itemReadDecision == AuthorizationDecisionType.DENY) {
                     // Explicitly denied access to the entire container
                     itemsToRemove.add(modification);
+                } else if (itemDef != null && itemDef.isElaborate()) {
+                    LOGGER.trace("applySecurityConstraints(item): {}: skipping deeper dive, applying decision '{}' (elaborate)",
+                            itemPath, itemReadDecision);
+                    // See comments in the "PCV" version of this method (above)
+                    if (itemReadDecision == null) {
+                        itemsToRemove.add(modification);
+                    }
                 } else {
                     // No explicit decision (even ALLOW is not final here as something may be denied deeper inside)
                     AuthorizationDecisionType subDefaultReadDecision = defaultReadDecision;
@@ -574,8 +587,9 @@ private boolean reduceContainerValues(List<? extends PrismContainerValue<?>> val
         return removedSomething;
     }
 
-    public <D extends ItemDefinition> void applySecurityConstraints(D itemDefinition, ObjectSecurityConstraints securityConstraints,
-            AuthorizationPhaseType phase) {
+    /** The variant of `applySecurityConstraints` for a {@link ItemDefinition}. */
+    public <D extends ItemDefinition> void applySecurityConstraints(
+            D itemDefinition, ObjectSecurityConstraints securityConstraints, AuthorizationPhaseType phase) {
         if (phase == null) {
             applySecurityConstraintsPhase(itemDefinition, securityConstraints, AuthorizationPhaseType.REQUEST);
             applySecurityConstraintsPhase(itemDefinition, securityConstraints, AuthorizationPhaseType.EXECUTION);
@@ -611,7 +625,7 @@ private <D extends ItemDefinition> void applySecurityConstraintsItemDef(D itemDe
         boolean anySubElementModify = false;
         if (itemDefinition instanceof PrismContainerDefinition<?> && !thisWasSeen) {
             PrismContainerDefinition<?> containerDefinition = (PrismContainerDefinition<?>)itemDefinition;
-            List<? extends ItemDefinition> subDefinitions = ((PrismContainerDefinition<?>)containerDefinition).getDefinitions();
+            List<? extends ItemDefinition> subDefinitions = containerDefinition.getDefinitions();
             for (ItemDefinition subDef: subDefinitions) {
                 ItemPath subPath = ItemPath.create(nameOnlyItemPath, subDef.getItemName());
                 if (subDef.isElaborate()) {

diff --git a/...n/java/com/evolveum/midpoint/model/impl/schema/transform/TransformableItemDefinition.java b/...n/java/com/evolveum/midpoint/model/impl/schema/transform/TransformableItemDefinition.java
@@ -324,6 +324,7 @@ public ItemDefinition<I> deepClone(@NotNull DeepCloneOperation operation) {
 
     protected abstract TransformableItemDefinition<I,D> copy();
 
+    // FIXME we should display overridden values here (e.g. RAM access flags)
     @Override
     public String toString() {
         return "Transformable:" + delegate().toString();

diff --git a/...l-intest/src/test/java/com/evolveum/midpoint/model/intest/security/TestSecurityBasic.java b/...l-intest/src/test/java/com/evolveum/midpoint/model/intest/security/TestSecurityBasic.java
@@ -17,6 +17,8 @@
 
 import com.evolveum.midpoint.model.api.ModelInteractionService;
 import com.evolveum.midpoint.schema.processor.ResourceObjectDefinition;
+
+import org.assertj.core.api.Assertions;
 import org.springframework.test.annotation.DirtiesContext;
 import org.springframework.test.annotation.DirtiesContext.ClassMode;
 import org.springframework.test.context.ContextConfiguration;
@@ -91,8 +93,9 @@ public void test201AutzJackSuperuserRole() throws Exception {
         then();
         assertSuperuserAccess(NUMBER_OF_ALL_USERS);
 
-        Collection<SelectorOptions<GetOperationOptions>> withCases = SchemaService.get().getOperationOptionsBuilder().
-                item(AccessCertificationCampaignType.F_CASE).retrieve().build();
+        Collection<SelectorOptions<GetOperationOptions>> withCases =
+                SchemaService.get().getOperationOptionsBuilder()
+                        .item(AccessCertificationCampaignType.F_CASE).retrieve().build();
         assertSearch(AccessCertificationCampaignType.class, null, withCases, new SearchAssertion<>() {
 
             public void assertObjects(String message, List<PrismObject<AccessCertificationCampaignType>> objects) {
@@ -3166,7 +3169,11 @@ public void test380AutzJackSelfTaskOwner() throws Exception {
         assertGetDeny(UserType.class, USER_GUYBRUSH_OID);
 
         assertGetDeny(TaskType.class, TASK_USELESS_ADMINISTRATOR_OID);
-        assertGetAllow(TaskType.class, TASK_USELESS_JACK_OID);
+        var task = assertGetAllow(TaskType.class, TASK_USELESS_JACK_OID).asObjectable();
+        Assertions.assertThat(task.getActivity()).as("activity in the task").isNotNull();
+        Assertions.assertThat(task.getActivityState()).as("activity state in the task, denied by the autz").isNull();
+
+        assertOwnTaskEditSchema(task.asPrismObject());
 
         assertSearch(UserType.class, null, 0);
         assertSearch(ObjectType.class, null, 0);
@@ -3188,6 +3195,18 @@ public void test380AutzJackSelfTaskOwner() throws Exception {
         assertGlobalStateUntouched();
     }
 
+    private void assertOwnTaskEditSchema(PrismObject<TaskType> task) throws Exception {
+        PrismObjectDefinition<TaskType> def = getEditObjectDefinition(task);
+        displayDumpable("task edit def", def);
+        // All items are readable, except for `activityState` (see `read-self-task-owner` autz)
+        // All items are "addable" (see `add-self-task-owner` autz)
+        // No items are modifiable, as there's no autz to modify the task.
+        assertItemFlags(def, TaskType.F_NAME, true, true, false);
+        assertItemFlags(def, TaskType.F_DESCRIPTION, true, true, false);
+        assertItemFlags(def, TaskType.F_ACTIVITY, true, true, false);
+        assertItemFlags(def, TaskType.F_ACTIVITY_STATE, false, true, false);
+    }
+
     /**
      * Searches for users with given assignment/targetRef (both directly and using EXISTS clause). See MID-7931.
      */

diff --git a/model/model-intest/src/test/resources/security/role-self-task-owner.xml b/model/model-intest/src/test/resources/security/role-self-task-owner.xml
@@ -6,9 +6,7 @@
   -->
 <role oid="455edc40-30c6-11e7-937f-df84f38dd402"
         xmlns="http://midpoint.evolveum.com/xml/ns/public/common/common-3"
-        xmlns:t="http://prism.evolveum.com/xml/ns/public/types-3"
-        xmlns:q="http://prism.evolveum.com/xml/ns/public/query-3"
-        xmlns:org='http://midpoint.evolveum.com/xml/ns/public/common/org-3'>
+        xmlns:q="http://prism.evolveum.com/xml/ns/public/query-3">
     <name>Role Self Task Owner</name>
     <authorization>
         <name>read-self-task-owner</name>
@@ -19,6 +17,7 @@
                 <special>self</special>
             </owner>
         </object>
+        <exceptItem>activityState</exceptItem> <!-- MID-8635 -->
     </authorization>
     <authorization>
         <name>add-self-task-owner</name>

diff --git a/model/model-intest/src/test/resources/security/task-useless-administrator.xml b/model/model-intest/src/test/resources/security/task-useless-administrator.xml
@@ -7,9 +7,7 @@
   -->
 
 <task oid="daa36dba-30c7-11e7-bd7d-6311953a3ecd"
-    xmlns="http://midpoint.evolveum.com/xml/ns/public/common/common-3"
-    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
-    xmlns:xsd="http://www.w3.org/2001/XMLSchema">
+    xmlns="http://midpoint.evolveum.com/xml/ns/public/common/common-3">
 
     <name>Useless task: administrator</name>
 
@@ -21,4 +19,15 @@
     <executionState>closed</executionState>
 
     <handlerUri>http://midpoint.evolveum.com/xml/ns/public/model/synchronization/task/useless/handler-3</handlerUri>
+
+    <!-- MID-8635 -->
+    <activity>
+        <work>
+            <focusValidityScan/>
+        </work>
+    </activity>
+    <activityState>
+        <activity/>
+        <tree/>
+    </activityState>
 </task>
diff --git a/model/model-intest/src/test/resources/security/task-useless-jack.xml b/model/model-intest/src/test/resources/security/task-useless-jack.xml
@@ -7,9 +7,7 @@
   -->
 
 <task oid="642d8174-30c8-11e7-b338-c3cf3a6c548a"
-    xmlns="http://midpoint.evolveum.com/xml/ns/public/common/common-3"
-    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
-    xmlns:xsd="http://www.w3.org/2001/XMLSchema">
+    xmlns="http://midpoint.evolveum.com/xml/ns/public/common/common-3">
 
     <name>Useless task: jack</name>
 
@@ -21,4 +19,15 @@
     <executionState>closed</executionState>
 
     <handlerUri>http://midpoint.evolveum.com/xml/ns/public/model/synchronization/task/useless/handler-3</handlerUri>
+
+    <!-- MID-8635 -->
+    <activity>
+        <work>
+            <focusValidityScan/>
+        </work>
+    </activity>
+    <activityState>
+        <activity/>
+        <tree/>
+    </activityState>
 </task>