Improve authorization for filter items

In order to evaluate a filter, one has to be authorized to access items (and their values) used for filter evaluation. The support for this feature was present but a bit incomplete. "Deny" authorizations were not taken into account, and authorizations for unrelated types (required e.g. by the referencedBy filter) were ignored. This commit partially fixes that: "deny" authorizations are now supported in the same way as "allow" ones, and some filter items are checked, at least at a rudimentary level. To be improved later. (Also adding forgotten TestExpressionProfiles to test suite.) Related to MID-9638 and MID-9670.
Evolveum · Apr 30, 2024 · 4cd16d5 · 4cd16d5
1 parent e253373
commit 4cd16d5
Show file tree

Hide file tree

Showing 15 changed files with 394 additions and 137 deletions.
diff --git a/infra/schema/src/main/java/com/evolveum/midpoint/schema/selector/eval/FilteringContext.java b/infra/schema/src/main/java/com/evolveum/midpoint/schema/selector/eval/FilteringContext.java
@@ -131,8 +131,8 @@ public boolean maySkipOnSearch() {
     /**
      * Creates a sub-context when evaluating embedded selector (e.g. `parent`).
      *
-     * @see MatchingContext#next(String, String)
-     * @see MatchingContext#next(DelegatorSelection, String, String)
+     * @see MatchingContext#next(String, String, Boolean)
+     * @see MatchingContext#next(DelegatorSelection, String, String, boolean)
      */
     public @NotNull FilteringContext next(
             @NotNull Class<?> filterType,

diff --git a/...model-intest/src/test/java/com/evolveum/midpoint/model/intest/TestExpressionProfiles.java b/...model-intest/src/test/java/com/evolveum/midpoint/model/intest/TestExpressionProfiles.java
@@ -265,7 +265,8 @@ public void test120RestrictedRoleAutoBadMappingExpression() throws Exception {
                 fail("unexpected success");
             } catch (SecurityViolationException e) {
                 assertExpectedException(e)
-                        .hasMessageContaining("Denied access to functionality of script in expression in mapping in autoassign mapping")
+                        .hasMessageContaining("Denied access to functionality of script in")
+                        .hasMessageContaining("expression in mapping in autoassign mapping")
                         .hasMessageContaining(DETAIL_REASON_MESSAGE_BOOM_RESTRICTED);
                 assertLocation(
                         e,
@@ -306,7 +307,8 @@ public void test130RestrictedRoleAutoBadMappingCondition() throws Exception {
                 fail("unexpected success");
             } catch (SecurityViolationException e) {
                 assertExpectedException(e)
-                        .hasMessageContaining("Denied access to functionality of script in condition in mapping in autoassign mapping")
+                        .hasMessageContaining("Denied access to functionality of script in")
+                        .hasMessageContaining("condition in mapping in autoassign mapping")
                         .hasMessageContaining(DETAIL_REASON_MESSAGE_BOOM_RESTRICTED);
                 assertLocation(
                         e,

diff --git a/...est/java/com/evolveum/midpoint/model/intest/security/AbstractInitializedSecurityTest.java b/...est/java/com/evolveum/midpoint/model/intest/security/AbstractInitializedSecurityTest.java
@@ -80,6 +80,10 @@ public abstract class AbstractInitializedSecurityTest extends AbstractInitialize
     protected static final TestObject<UserType> USER_ESTEVAN = TestObject.file(TEST_DIR, "user-estevan.xml", "00000000-0000-0000-0000-110000000012");
     protected static final TestObject<UserType> USER_CAPSIZE = TestObject.file(TEST_DIR, "user-capsize.xml", "bab2c6a8-5f2a-11e8-97d2-4fc12ba39043");
 
+    // loaded at the beginning, not modified by any test
+    protected static final TestObject<UserType> USER_ALEX = TestObject.file(TEST_DIR, "user-alex.xml", "90b46002-15df-4de5-b73b-2103860fb2b1");
+    protected static final TestObject<UserType> USER_BETTY = TestObject.file(TEST_DIR, "user-betty.xml", "64b3462b-a221-4a6f-9ad3-4bc4b622ccc5");
+
     // loaded ad-hoc (not in init)
     protected static final TestObject<UserType> USER_DEPUTY_1 = TestObject.file(TEST_DIR, "user-deputy-1.xml", "af69e388-88bd-43f9-9259-73676124c196");
     protected static final TestObject<UserType> USER_DEPUTY_2 = TestObject.file(TEST_DIR, "user-deputy-2.xml", "0223b993-b8bd-4599-8873-80d04b88a1ce");
@@ -166,6 +170,7 @@ public abstract class AbstractInitializedSecurityTest extends AbstractInitialize
     protected static final TestObject<RoleType> ROLE_END_USER_WITH_PRIVACY = TestObject.file(TEST_DIR, "role-end-user-with-privacy.xml", "2abaef72-af5b-11e8-ae9a-b33bc5b8cb74");
     protected static final TestObject<RoleType> ROLE_APPROVER_UNASSIGN_ROLES = TestObject.file(TEST_DIR, "role-approver-unassign-roles.xml", "5d9cead8-3a2e-11e7-8609-f762a755b58e");
     protected static final TestObject<RoleType> ROLE_USE_TASK_TEMPLATES = TestObject.file(TEST_DIR, "role-use-task-templates.xml", "ac97ca9d-7bb6-44b3-8d10-3b309c97f866");
+    protected static final TestObject<RoleType> ROLE_DENY_READ_ASSIGNMENT_AND_ROLE_MEMBERSHIP_REF = TestObject.file(TEST_DIR, "role-deny-read-assignment-and-roleMembershipRef.xml", "2c328dbc-a40d-43a8-a9e1-266c96cad22d");
     protected static final TestObject<OrgType> ORG_REQUESTABLE = TestObject.file(TEST_DIR, "org-requestable.xml", "8f2bd344-a46c-4c0b-aa34-db08b7d7f7f2");
     protected static final TestObject<OrgType> ORG_INDIRECT_PIRATE = TestObject.file(TEST_DIR, "org-indirect-pirate.xml", "59024142-5830-11e7-80e6-ffbee06efb45");
     protected static final TestObject<OrgType> ORG_CHEATERS = TestObject.file(TEST_DIR, "org-cheaters.xml", "944cef84-6570-11e7-8262-079921253d05");
@@ -196,8 +201,8 @@ public abstract class AbstractInitializedSecurityTest extends AbstractInitialize
     protected static final XMLGregorianCalendar JACK_VALID_FROM_LONG_AGO = XmlTypeConverter.createXMLGregorianCalendar(10000L);
     protected static final XMLGregorianCalendar JACK_VALID_TO_LONG_AHEAD = XmlTypeConverter.createXMLGregorianCalendar(10000000000000L);
 
-    protected static final int NUMBER_OF_ALL_USERS = 11;
-    protected static final int NUMBER_OF_IMPORTED_ROLES = 76;
+    protected static final int NUMBER_OF_ALL_USERS = 13;
+    protected static final int NUMBER_OF_IMPORTED_ROLES = 77;
     protected static final int NUMBER_OF_ALL_ORGS = 11;
 
     protected String userRumRogersOid;
@@ -303,6 +308,7 @@ public void initSystem(Task initTask, OperationResult initResult) throws Excepti
         repoAdd(ROLE_UNASSIGN_SELF_REQUESTABLE, initResult);
         repoAdd(ROLE_APPROVER_UNASSIGN_ROLES, initResult);
         repoAdd(ROLE_USE_TASK_TEMPLATES, initResult);
+        repoAdd(ROLE_DENY_READ_ASSIGNMENT_AND_ROLE_MEMBERSHIP_REF, initResult);
 
         repoAdd(ORG_REQUESTABLE, initResult);
         repoAdd(ORG_INDIRECT_PIRATE, initResult);
@@ -318,6 +324,10 @@ public void initSystem(Task initTask, OperationResult initResult) throws Excepti
 
         repoAdd(USER_CHARLES, initResult);
 
+        // Imported via model in order to have also roleMembershipRef etc
+        initTestObjects(initTask, initResult,
+                USER_ALEX, USER_BETTY);
+
         PrismObject<UserType> userRum = createUser(USER_RUM_ROGERS_NAME, "Rum Rogers");
         addObject(userRum, initTask, initResult);
         userRumRogersOid = userRum.getOid();

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
@@ -3128,6 +3128,37 @@ public void test313AutzAnonymousPrivilegedRestore() throws Exception {
         assertGlobalStateUntouched();
     }
 
+    /** Searching for "roles of teammate" with denied access to `assignment` item. MID-9638. */
+    @Test
+    public void test320AutzDenyReadAssignmentAndRoleMembershipRef() throws Exception {
+        given();
+        cleanupAutzTest(USER_JACK_OID);
+        assignRole(USER_JACK_OID, ROLE_READONLY.oid);
+        assignRole(USER_JACK_OID, ROLE_DENY_READ_ASSIGNMENT_AND_ROLE_MEMBERSHIP_REF.oid);
+        login(USER_JACK_USERNAME);
+
+        when("searching for users based on assignment/targetRef (forbidden)");
+        assertSearch(
+                UserType.class,
+                queryFor(UserType.class)
+                        .item(UserType.F_ASSIGNMENT, AssignmentType.F_TARGET_REF)
+                        .ref(ROLE_BASIC.oid) // at least alex has this role
+                        .build(),
+                0); // access to assignment/targetRef is explicitly denied
+
+        when("searching for roles of teammate alex (forbidden)");
+        var alexRolesQuery = createRolesOfTeammateQuery(USER_ALEX.oid);
+        assertSearch(RoleType.class, alexRolesQuery, 0);
+    }
+
+    private ObjectQuery createRolesOfTeammateQuery(String userOid) {
+        return queryFor(RoleType.class)
+                .referencedBy(UserType.class, UserType.F_ASSIGNMENT.append(AssignmentType.F_TARGET_REF))
+                .id(userOid)
+                .and().not().type(ArchetypeType.class)
+                .build();
+    }
+
     @Test
     public void test360AutzJackAuditorRole() throws Exception {
         given();

diff --git a/model/model-intest/src/test/resources/logback-test.xml b/model/model-intest/src/test/resources/logback-test.xml
@@ -109,9 +109,13 @@
     <logger name="com.evolveum.midpoint.model.common.expression" level="DEBUG" />
     <logger name="com.evolveum.midpoint.model.impl.controller.ObjectMerger" level="DEBUG" />
     <logger name="com.evolveum.midpoint.notifications" level="DEBUG" />
+
     <logger name="com.evolveum.midpoint.security" level="DEBUG" />
-    <logger name="com.evolveum.midpoint.security.enforcer.impl.SecurityEnforcerImpl" level="DEBUG" />
     <logger name="com.evolveum.midpoint.model.impl.security" level="DEBUG" />
+    <!-- for troubleshooting authorization tests -->
+    <!--<logger name="com.evolveum.midpoint.security" level="TRACE" />-->
+    <!--<logger name="com.evolveum.midpoint.model.impl.security" level="TRACE" />-->
+
     <!-- TODO set DEBUG for specific sync task handlers -->
     <logger name="com.evolveum.midpoint.model.impl.sync.SynchronizationServiceImpl" level="TRACE" />
     <!--<logger name="com.evolveum.midpoint.model.impl.tasks.RecomputeTaskHandler" level="DEBUG" />--><!-- FIXME -->

diff --git a/...el-intest/src/test/resources/security/role-deny-read-assignment-and-roleMembershipRef.xml b/...el-intest/src/test/resources/security/role-deny-read-assignment-and-roleMembershipRef.xml
@@ -0,0 +1,21 @@
+<!--
+  ~ Copyright (C) 2010-2024 Evolveum and contributors
+  ~
+  ~ This work is dual-licensed under the Apache License 2.0
+  ~ and European Union Public License. See LICENSE file for details.
+  -->
+
+<role xmlns="http://midpoint.evolveum.com/xml/ns/public/common/common-3"
+    oid="2c328dbc-a40d-43a8-a9e1-266c96cad22d">
+    <name>role-deny-read-assignment-and-roleMembershipRef</name>
+    <authorization>
+        <name>deny-read-assignment-and-roleMembershipRef</name>
+        <decision>deny</decision>
+        <action>http://midpoint.evolveum.com/xml/ns/public/security/authorization-model-3#read</action>
+        <object>
+            <type>UserType</type>
+        </object>
+        <item>roleMembershipRef</item>
+        <item>assignment</item>
+    </authorization>
+</role>
diff --git a/model/model-intest/src/test/resources/security/user-alex.xml b/model/model-intest/src/test/resources/security/user-alex.xml
@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+  ~ Copyright (c) 2010-2018 Evolveum and contributors
+  ~
+  ~ This work is dual-licensed under the Apache License 2.0
+  ~ and European Union Public License. See LICENSE file for details.
+  -->
+<user oid="90b46002-15df-4de5-b73b-2103860fb2b1"
+   xmlns='http://midpoint.evolveum.com/xml/ns/public/common/common-3'>
+    <name>alex</name>
+    <assignment>
+        <targetRef oid="00000000-0000-0000-0000-00000000aad1" type="RoleType"/>
+    </assignment>
+</user>
diff --git a/model/model-intest/src/test/resources/security/user-betty.xml b/model/model-intest/src/test/resources/security/user-betty.xml
@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+  ~ Copyright (c) 2010-2018 Evolveum and contributors
+  ~
+  ~ This work is dual-licensed under the Apache License 2.0
+  ~ and European Union Public License. See LICENSE file for details.
+  -->
+<user oid="64b3462b-a221-4a6f-9ad3-4bc4b622ccc5"
+   xmlns='http://midpoint.evolveum.com/xml/ns/public/common/common-3'>
+    <name>betty</name>
+    <assignment>
+        <targetRef oid="3a17b131-82c2-4669-a491-791081be9c04" type="RoleType"/>
+    </assignment>
+</user>
diff --git a/model/model-intest/testng-integration-full.xml b/model/model-intest/testng-integration-full.xml
@@ -19,6 +19,7 @@
             <class name="com.evolveum.midpoint.model.intest.security.TestSecurityMultitenant"/>
             <class name="com.evolveum.midpoint.model.intest.security.TestSecurityItemValues"/>
             <class name="com.evolveum.midpoint.model.intest.security.TestSecurityGovernance"/>
+            <class name="com.evolveum.midpoint.model.intest.TestExpressionProfiles"/>
             <class name="com.evolveum.midpoint.model.intest.TestRunAs"/>
         </classes>
     </test>

diff --git a/.../src/main/java/com/evolveum/midpoint/security/enforcer/api/PositiveNegativeItemPaths.java b/.../src/main/java/com/evolveum/midpoint/security/enforcer/api/PositiveNegativeItemPaths.java
@@ -206,7 +206,8 @@ public void shortDump(StringBuilder sb) {
         }
     }
 
-    protected void dumpItems(StringBuilder sb, List<? extends ItemPath> items) {
+    // TODO move to a better place
+    public static void dumpItems(StringBuilder sb, List<? extends ItemPath> items) {
         if (items.isEmpty()) {
             sb.append("[none]");
         } else {

diff --git a/...l/src/main/java/com/evolveum/midpoint/security/enforcer/impl/EnforcerFilterOperation.java b/...l/src/main/java/com/evolveum/midpoint/security/enforcer/impl/EnforcerFilterOperation.java
@@ -19,7 +19,6 @@
 import org.jetbrains.annotations.Nullable;
 
 import com.evolveum.midpoint.prism.PrismObject;
-import com.evolveum.midpoint.prism.path.ItemPath;
 import com.evolveum.midpoint.prism.query.ObjectFilter;
 import com.evolveum.midpoint.repo.common.query.SelectorToFilterTranslator;
 import com.evolveum.midpoint.schema.result.OperationResult;
@@ -91,7 +90,7 @@ F computeSecurityFilter(@Nullable AuthorizationPhaseType phase, OperationResult
             securityFilter = new PartialOp(nonStrict(phase)).computeFilter(result);
         } else {
             F filterBoth = new PartialOp(both()).computeFilter(result);
-            F filterRequest = new PartialOp( strict(REQUEST)).computeFilter(result);
+            F filterRequest = new PartialOp(strict(REQUEST)).computeFilter(result);
             F filterExecution = new PartialOp(strict(EXECUTION)).computeFilter(result);
             securityFilter =
                     gizmo.or(
@@ -118,13 +117,13 @@ class PartialOp {
 
         private final @NotNull PhaseSelector phaseSelector;
 
-        /** TODO */
-        @NotNull private final QueryAutzItemPaths queryItemsSpec = new QueryAutzItemPaths();
+        /** What objects/items do we need the access to (to evaluate the query), and do we have it? */
+        @NotNull private final QueryObjectsAutzCoverage queryObjectsAutzCoverage = new QueryObjectsAutzCoverage();
 
-        /** TODO */
+        /** Filter returning the set of visible (allowed) objects. */
         private F securityFilterAllow = null;
 
-        /** TODO */
+        /** Filter returning the set of invisible (denied) objects. */
         private F securityFilterDeny = null;
 
         PartialOp(@NotNull PhaseSelector phaseSelector) {
@@ -145,7 +144,7 @@ private F computeFilter(OperationResult result)
                 throws SchemaException, ObjectNotFoundException, ExpressionEvaluationException, CommunicationException,
                 ConfigurationException, SecurityViolationException {
 
-            queryItemsSpec.addRequiredItems(origFilter); // MID-3916
+            queryObjectsAutzCoverage.addRequiredItems(filterType, origFilter); // MID-3916, MID-9670
             tracePartialOperationStarted();
 
             int i = 0;
@@ -173,6 +172,8 @@ private F computeFilter(OperationResult result)
                     continue;
                 }
 
+                queryObjectsAutzCoverage.processAuthorizationForOtherTypes(filterType, authorization);
+
                 var applicable = autzEvaluation.computeFilter();
 
                 // the end of processing of given authorization was logged as part of the method call above
@@ -182,13 +183,15 @@ private F computeFilter(OperationResult result)
                             gizmo.adopt(
                                     ObjectQueryUtil.simplify(autzEvaluation.getAutzFilter()),
                                     authorization);
+
+                    if (!gizmo.isNone(autzSecurityFilter)) {
+                        // If we are sure we match no objects, we ignore items allowed/denied by this authorization.
+                        queryObjectsAutzCoverage.processAuthorizationForFilterType(filterType, authorization);
+                    }
+
                     // The authorization is applicable to this situation. Now we can process the decision.
                     if (authorization.isAllow()) {
                         securityFilterAllow = gizmo.or(securityFilterAllow, autzSecurityFilter);
-                        if (!gizmo.isNone(autzSecurityFilter)) {
-                            // TODO resolve for shifted authorizations!
-                            queryItemsSpec.collectItems(authorization);
-                        }
                     } else { // "deny" type authorization
                         if (authorization.hasItemSpecification()) {
                             // TODO resolve for shifted authorizations!
@@ -208,11 +211,11 @@ private F computeFilter(OperationResult result)
                 }
             }
 
-            List<ItemPath> unsatisfiedItems = queryItemsSpec.evaluateUnsatisfiedItems();
-            if (!unsatisfiedItems.isEmpty()) {
+            String unsatisfiedItemsDescription = queryObjectsAutzCoverage.getUnsatisfiedItemsDescription();
+            if (unsatisfiedItemsDescription != null) {
                 F secFilter = gizmo.createDenyAll();
-                // TODO lazy string concatenation?
-                tracePartialOperationFinished(secFilter, "deny because items " + unsatisfiedItems + " are not allowed");
+                tracePartialOperationFinished(
+                        secFilter, "deny because items " + unsatisfiedItemsDescription + " are not allowed");
                 return secFilter;
             }
             securityFilterAllow = gizmo.simplify(securityFilterAllow);
@@ -239,7 +242,7 @@ private void tracePartialOperationStarted() {
                         new PartialFilterOperationStarted<>(
                                 this,
                                 phaseSelector,
-                                queryItemsSpec.shortDump()));
+                                queryObjectsAutzCoverage.shortDump()));
             }
         }
 
@@ -255,8 +258,8 @@ private void tracePartialOperationFinished(F secFilter, String comment) {
         }
 
         /** For diagnostics purposes. */
-        @NotNull QueryAutzItemPaths getQueryItemsSpec() {
-            return queryItemsSpec;
+        @NotNull QueryObjectsAutzCoverage getQueryObjectsAutzCoverage() {
+            return queryObjectsAutzCoverage;
         }
 
         /** For diagnostics purposes. */