Fix default "assignment added" approval processing

The default approval processing of assignment being added was flawed, because it considered an assignment whose validity state changed (from invalid to valid) as being added. This is obviously wrong: such assignment is being modified, not added. This resolves MID-7317.
Evolveum · Oct 27, 2021 · 1e43c89 · 1e43c89
1 parent e9cfa54
commit 1e43c89
Show file tree

Hide file tree

Showing 10 changed files with 232 additions and 9 deletions.
diff --git a/.../model-api/src/main/java/com/evolveum/midpoint/model/api/context/EvaluatedAssignment.java b/.../model-api/src/main/java/com/evolveum/midpoint/model/api/context/EvaluatedAssignment.java
@@ -121,4 +121,19 @@ default AssignmentType getAssignmentType(boolean old) {
      */
     Collection<? extends Mapping<?,?>> getFocusMappings();
 
+    /**
+     * Assignment is either being added in the current wave or was added in some of the previous waves.
+     */
+    boolean isBeingAdded();
+
+    /**
+     * Assignment is either being deleted in the current wave or was deleted in some of the previous waves.
+     */
+    boolean isBeingDeleted();
+
+    /**
+     * Assignment was present at the beginning and is not being deleted.
+     */
+    boolean isBeingKept();
+
 }
diff --git a/.../main/java/com/evolveum/midpoint/model/impl/lens/assignments/EvaluatedAssignmentImpl.java b/.../main/java/com/evolveum/midpoint/model/impl/lens/assignments/EvaluatedAssignmentImpl.java
@@ -615,4 +615,16 @@ public PlusMinusZero getMode() {
     public PlusMinusZero getAbsoluteMode() {
         return origin.getAbsoluteMode();
     }
+
+    public boolean isBeingAdded() {
+        return origin.isBeingAdded();
+    }
+
+    public boolean isBeingDeleted() {
+        return origin.isBeingDeleted();
+    }
+
+    public boolean isBeingKept() {
+        return origin.isBeingKept();
+    }
 }
diff --git a/...a/com/evolveum/midpoint/wf/impl/processors/primary/policy/AssignmentPolicyAspectPart.java b/...a/com/evolveum/midpoint/wf/impl/processors/primary/policy/AssignmentPolicyAspectPart.java
@@ -97,7 +97,7 @@ void extractAssignmentBasedInstructions(ObjectTreeDeltas<?> objectTreeDeltas, Pr
             trace = null;
         }
         try {
-            DeltaSetTriple<? extends EvaluatedAssignment> evaluatedAssignmentTriple = ((LensContext<?>) ctx.modelContext)
+            DeltaSetTriple<? extends EvaluatedAssignment<?>> evaluatedAssignmentTriple = ((LensContext<?>) ctx.modelContext)
                     .getEvaluatedAssignmentTriple();
             LOGGER.trace("Processing evaluatedAssignmentTriple:\n{}", DebugUtil.debugDumpLazily(evaluatedAssignmentTriple));
             if (evaluatedAssignmentTriple == null) {
@@ -176,7 +176,8 @@ private PcpStartInstruction createInstructionFromAssignment(
         }
 
         // Let's construct the approval schema plus supporting triggered approval policy rule information
-        ApprovalSchemaBuilder.Result approvalSchemaResult = createSchemaWithRules(triggeredApprovalActionRules, assignmentMode,
+        // Here we also treat default "rules" when no policy rules match.
+        ApprovalSchemaBuilder.Result approvalSchemaResult = createSchemaWithRules(triggeredApprovalActionRules,
                 evaluatedAssignment, ctx, result);
         if (approvalSchemaHelper.shouldBeSkipped(approvalSchemaResult.schemaType)) {
             return null;
@@ -271,23 +272,23 @@ private void logApprovalActions(EvaluatedAssignment<?> newAssignment,
                     verb, newAssignment, newAssignment.getThisTargetPolicyRules().size(), triggeredApprovalActionRules.size());
             for (EvaluatedPolicyRule t : triggeredApprovalActionRules) {
                 LOGGER.debug(" - Approval actions: {}", t.getEnabledActions(ApprovalPolicyActionType.class));
-                for (EvaluatedPolicyRuleTrigger trigger : t.getTriggers()) {
+                for (EvaluatedPolicyRuleTrigger<?> trigger : t.getTriggers()) {
                     LOGGER.debug("   - {}", trigger);
                 }
             }
         }
     }
 
     private ApprovalSchemaBuilder.Result createSchemaWithRules(List<EvaluatedPolicyRule> triggeredApprovalRules,
-            PlusMinusZero assignmentMode, @NotNull EvaluatedAssignment<?> evaluatedAssignment, ModelInvocationContext ctx,
+            @NotNull EvaluatedAssignment<?> evaluatedAssignment, ModelInvocationContext<?> ctx,
             OperationResult result) throws SchemaException {
 
         PrismObject<?> targetObject = evaluatedAssignment.getTarget();
         ApprovalSchemaBuilder builder = new ApprovalSchemaBuilder(main, approvalSchemaHelper);
 
         // default policy action (only if adding)
-        if (triggeredApprovalRules.isEmpty() && assignmentMode == PLUS
-                && configurationHelper.getUseDefaultApprovalPolicyRules(ctx.wfConfiguration) != DefaultApprovalPolicyRulesUsageType.NEVER) {
+        if (triggeredApprovalRules.isEmpty() && evaluatedAssignment.isBeingAdded() &&
+                configurationHelper.getUseDefaultApprovalPolicyRules(ctx.wfConfiguration) != DefaultApprovalPolicyRulesUsageType.NEVER) {
             if (builder.addPredefined(targetObject, RelationKindType.APPROVER, result)) {
                 LOGGER.trace("Added default approval action, as no explicit one was found for {}", evaluatedAssignment);
             }
@@ -353,10 +354,9 @@ private LocalizableMessage createDefaultProcessName(ModelInvocationContext<?> ct
     }
 
     // creates an ObjectDelta that will be executed after successful approval of the given assignment
-    @SuppressWarnings("unchecked")
     private ObjectDelta<? extends FocusType> assignmentToDelta(Class<? extends Objectable> focusClass,
             AssignmentType assignmentType, boolean assignmentRemoved, String objectOid) throws SchemaException {
-        PrismContainerValue value = assignmentType.clone().asPrismContainerValue();
+        PrismContainerValue<?> value = assignmentType.clone().asPrismContainerValue();
         S_ValuesEntry item = prismContext.deltaFor(focusClass)
                 .item(FocusType.F_ASSIGNMENT);
         S_ItemEntry op = assignmentRemoved ? item.delete(value) : item.add(value);

diff --git a/...impl/src/test/java/com/evolveum/midpoint/wf/impl/assignments/TestAssignmentsAdvanced.java b/...impl/src/test/java/com/evolveum/midpoint/wf/impl/assignments/TestAssignmentsAdvanced.java
@@ -19,6 +19,8 @@
 import java.io.File;
 import java.util.*;
 
+import com.evolveum.midpoint.prism.xml.XmlTypeConverter;
+
 import org.springframework.test.annotation.DirtiesContext;
 import org.springframework.test.annotation.DirtiesContext.ClassMode;
 import org.springframework.test.context.ContextConfiguration;
@@ -46,6 +48,8 @@
 import com.evolveum.midpoint.wf.impl.ExpectedWorkItem;
 import com.evolveum.midpoint.xml.ns._public.common.common_3.*;
 
+import javax.xml.datatype.XMLGregorianCalendar;
+
 /**
  * A special test dealing with assigning roles that have different metarole-induced approval policies.
  * <p>
@@ -84,6 +88,14 @@ public class TestAssignmentsAdvanced extends AbstractWfTestPolicy {
 
     private static final File ORG_LEADS2122_FILE = new File(TEST_RESOURCE_DIR, "org-leads2122.xml");
 
+    private static final TestResource<RoleType> ROLE_BEING_ENABLED = new TestResource<>(TEST_RESOURCE_DIR, "role-being-enabled.xml", "4fcf187a-09b7-4d32-b998-9cd978195a82");
+    private static final TestResource<UserType> USER_HOLDER_OF_ROLE_BEING_ENABLED = new TestResource<>(TEST_RESOURCE_DIR, "user-holder-of-role-being-enabled.xml", "c6f7ddbe-a596-4a53-8acf-e03282f3da33");
+    private static final TestResource<UserType> USER_APPROVER_OF_ROLE_BEING_ENABLED = new TestResource<>(TEST_RESOURCE_DIR, "user-approver-of-role-being-enabled.xml", "39764050-fbd8-4746-a8a6-cb9141ae31ae");
+
+    private static final TestResource<RoleType> ROLE_BEING_DISABLED = new TestResource<>(TEST_RESOURCE_DIR, "role-being-disabled.xml", "4835efdc-6fee-438b-bb09-fd428a200dbb");
+    private static final TestResource<UserType> USER_HOLDER_OF_ROLE_BEING_DISABLED = new TestResource<>(TEST_RESOURCE_DIR, "user-holder-of-role-being-disabled.xml", "586fc857-71d8-4906-a61d-46ba7941be00");
+    private static final TestResource<UserType> USER_APPROVER_OF_ROLE_BEING_DISABLED = new TestResource<>(TEST_RESOURCE_DIR, "user-approver-of-role-being-disabled.xml", "d4b1fa25-4436-4f0c-a2a1-727a536b60e3");
+
     private static final File USER_LEAD21_FILE = new File(TEST_RESOURCE_DIR, "user-lead21.xml");
     private static final File USER_LEAD22_FILE = new File(TEST_RESOURCE_DIR, "user-lead22.xml");
     private static final File USER_LEAD23_FILE = new File(TEST_RESOURCE_DIR, "user-lead23.xml");
@@ -147,6 +159,13 @@ public void initSystem(Task initTask, OperationResult initResult) throws Excepti
         userSecurityApproverDeputyOid = addAndRecomputeUser(USER_SECURITY_APPROVER_DEPUTY_FILE, initTask, initResult);
         userSecurityApproverDeputyLimitedOid = addAndRecomputeUser(USER_SECURITY_APPROVER_DEPUTY_LIMITED_FILE, initTask, initResult);
 
+        repoAdd(ROLE_BEING_ENABLED, initResult);
+        repoAdd(ROLE_BEING_DISABLED, initResult);
+        addObject(USER_HOLDER_OF_ROLE_BEING_ENABLED, initTask, initResult); // import this one first to avoid approving
+        addObject(USER_APPROVER_OF_ROLE_BEING_ENABLED, initTask, initResult);
+        addObject(USER_HOLDER_OF_ROLE_BEING_DISABLED, initTask, initResult); // import this one first to avoid approving
+        addObject(USER_APPROVER_OF_ROLE_BEING_DISABLED, initTask, initResult);
+
         DebugUtil.setPrettyPrintBeansAs(PrismContext.LANG_JSON);
     }
 
@@ -993,7 +1012,88 @@ public void test820UnassignRole27() throws Exception {
         assertNotAssignedRole(jack, roleRole27Oid);
     }
 
-    @Test      // MID-5827
+    /**
+     * An assignment is enabled by changing validTo property from 1900 to null.
+     * Such a change should *not* trigger any approvals (under the default rules).
+     *
+     * MID-7317
+     */
+    @Test
+    public void test830EnableAssignment() throws Exception {
+        login(userAdministrator);
+        Task task = getTestTask();
+        OperationResult result = getTestOperationResult();
+
+        UserType userBefore = getUserFromRepo(USER_HOLDER_OF_ROLE_BEING_ENABLED.oid).asObjectable();
+
+        when();
+        ObjectDelta<UserType> delta = deltaFor(UserType.class)
+                .item(UserType.F_ASSIGNMENT,
+                        userBefore.getAssignment().get(0).getId(),
+                        AssignmentType.F_ACTIVATION,
+                        ActivationType.F_VALID_TO)
+                .replace()
+                .asObjectDelta(USER_HOLDER_OF_ROLE_BEING_ENABLED.oid);
+        executeChanges(delta, null, task, result);
+
+        then();
+        assertSuccess(result);
+
+        List<PrismObject<CaseType>> cases =
+                getCasesForObject(USER_HOLDER_OF_ROLE_BEING_ENABLED.oid, UserType.COMPLEX_TYPE, null, task, result);
+        assertEquals("Wrong # of cases", 0, cases.size());
+
+        assertUser(USER_HOLDER_OF_ROLE_BEING_ENABLED.oid, "after")
+                .assignments()
+                    .single()
+                        .activation()
+                            .assertValidTo((XMLGregorianCalendar) null);
+    }
+
+    /**
+     * An assignment is disabled by setting validTo property to an old value.
+     * Such a change should *not* trigger any approvals under "assignment deletion" policy rule.
+     *
+     * MID-7317
+     */
+    @Test
+    public void test840DisableAssignment() throws Exception {
+        login(userAdministrator);
+        Task task = getTestTask();
+        OperationResult result = getTestOperationResult();
+
+        UserType userBefore = getUserFromRepo(USER_HOLDER_OF_ROLE_BEING_DISABLED.oid).asObjectable();
+
+        when();
+        XMLGregorianCalendar newValidTo = XmlTypeConverter.fromNow("-P1D");
+        ObjectDelta<UserType> delta = deltaFor(UserType.class)
+                .item(UserType.F_ASSIGNMENT,
+                        userBefore.getAssignment().get(0).getId(),
+                        AssignmentType.F_ACTIVATION,
+                        ActivationType.F_VALID_TO)
+                .replace(newValidTo)
+                .asObjectDelta(USER_HOLDER_OF_ROLE_BEING_DISABLED.oid);
+        executeChanges(delta, null, task, result);
+
+        then();
+        assertSuccess(result);
+
+        List<PrismObject<CaseType>> cases =
+                getCasesForObject(USER_HOLDER_OF_ROLE_BEING_DISABLED.oid, UserType.COMPLEX_TYPE, null, task, result);
+        assertEquals("Wrong # of cases", 0, cases.size());
+
+        assertUser(USER_HOLDER_OF_ROLE_BEING_DISABLED.oid, "after")
+                .assignments()
+                    .single()
+                        .activation()
+                            .assertValidTo(newValidTo)
+                            .assertEffectiveStatus(ActivationStatusType.DISABLED);
+    }
+
+    /**
+     * MID-5827
+     */
+    @Test
     public void test900AssignIdempotentRole() throws Exception {
         login(userAdministrator);
         Task task = getTestTask();

diff --git a/model/workflow-impl/src/test/resources/assignments-advanced/role-being-disabled.xml b/model/workflow-impl/src/test/resources/assignments-advanced/role-being-disabled.xml
@@ -0,0 +1,25 @@
+<!--
+  ~ Copyright (C) 2010-2021 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="4835efdc-6fee-438b-bb09-fd428a200dbb">
+    <name>role-being-disabled</name>
+    <assignment>
+        <policyRule>
+            <policyConstraints>
+                <assignment>
+                    <operation>delete</operation>
+                    <relation>default</relation>
+                </assignment>
+            </policyConstraints>
+            <policyActions>
+                <approval>
+                    <approverRelation>approver</approverRelation>
+                </approval>
+            </policyActions>
+        </policyRule>
+    </assignment>
+</role>
diff --git a/model/workflow-impl/src/test/resources/assignments-advanced/role-being-enabled.xml b/model/workflow-impl/src/test/resources/assignments-advanced/role-being-enabled.xml
@@ -0,0 +1,10 @@
+<!--
+  ~ Copyright (C) 2010-2021 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="4fcf187a-09b7-4d32-b998-9cd978195a82">
+    <name>role-being-enabled</name>
+</role>
diff --git a/...low-impl/src/test/resources/assignments-advanced/user-approver-of-role-being-disabled.xml b/...low-impl/src/test/resources/assignments-advanced/user-approver-of-role-being-disabled.xml
@@ -0,0 +1,15 @@
+<!--
+  ~ Copyright (C) 2010-2021 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 xmlns="http://midpoint.evolveum.com/xml/ns/public/common/common-3"
+      xmlns:org="http://midpoint.evolveum.com/xml/ns/public/common/org-3"
+      oid="d4b1fa25-4436-4f0c-a2a1-727a536b60e3">
+    <name>approver-of-role-being-disabled</name>
+    <assignment>
+        <targetRef oid="4835efdc-6fee-438b-bb09-fd428a200dbb" type="RoleType" relation="org:approver"/>
+    </assignment>
+</user>
diff --git a/...flow-impl/src/test/resources/assignments-advanced/user-approver-of-role-being-enabled.xml b/...flow-impl/src/test/resources/assignments-advanced/user-approver-of-role-being-enabled.xml
@@ -0,0 +1,15 @@
+<!--
+  ~ Copyright (C) 2010-2021 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 xmlns="http://midpoint.evolveum.com/xml/ns/public/common/common-3"
+      xmlns:org="http://midpoint.evolveum.com/xml/ns/public/common/org-3"
+      oid="39764050-fbd8-4746-a8a6-cb9141ae31ae">
+    <name>approver-of-role-being-enabled</name>
+    <assignment>
+        <targetRef oid="4fcf187a-09b7-4d32-b998-9cd978195a82" type="RoleType" relation="org:approver"/>
+    </assignment>
+</user>
diff --git a/...kflow-impl/src/test/resources/assignments-advanced/user-holder-of-role-being-disabled.xml b/...kflow-impl/src/test/resources/assignments-advanced/user-holder-of-role-being-disabled.xml
@@ -0,0 +1,14 @@
+<!--
+  ~ Copyright (C) 2010-2021 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 xmlns="http://midpoint.evolveum.com/xml/ns/public/common/common-3"
+      oid="586fc857-71d8-4906-a61d-46ba7941be00">
+    <name>holder-of-role-being-disabled</name>
+    <assignment>
+        <targetRef oid="4835efdc-6fee-438b-bb09-fd428a200dbb" type="RoleType"/>
+    </assignment>
+</user>
diff --git a/...rkflow-impl/src/test/resources/assignments-advanced/user-holder-of-role-being-enabled.xml b/...rkflow-impl/src/test/resources/assignments-advanced/user-holder-of-role-being-enabled.xml
@@ -0,0 +1,17 @@
+<!--
+  ~ Copyright (C) 2010-2021 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 xmlns="http://midpoint.evolveum.com/xml/ns/public/common/common-3"
+      oid="c6f7ddbe-a596-4a53-8acf-e03282f3da33">
+    <name>holder-of-role-being-enabled</name>
+    <assignment>
+        <targetRef oid="4fcf187a-09b7-4d32-b998-9cd978195a82" type="RoleType"/>
+        <activation>
+            <validTo>1900-01-01T00:00:00+00:00</validTo>
+        </activation>
+    </assignment>
+</user>