From f9debb27f82368f6c9ad0b2112cfdce03c56c619 Mon Sep 17 00:00:00 2001 From: Pavol Mederly Date: Mon, 6 Apr 2020 14:32:21 +0200 Subject: [PATCH] Stop visiting embedded objects by default Visitor in prism module did NOT visit objects embedded in references for a very long time. Recently we changed this behavior and started visiting embedded objects - in order to assure encryption of protected values in such objects (see 60328c40b2b99c6cf41ab6ce90145fae941d07bd). But this change broke other things, like resolution of references in ApprovalSchemaExecutionInformationType. Analysis of other uses indicate that visiting embedded objects can be wrong in other contexts as well. So now we are reverting to the old behavior and providing ConfigurableVisitor to allow controlling this feature. This resolves MID-6171. --- .../midpoint/common/crypto/CryptoUtil.java | 7 +++- .../midpoint/prism/ConfigurableVisitor.java | 34 +++++++++++++++++++ .../midpoint/prism/util/PrismAsserts.java | 2 -- .../prism/impl/PrismReferenceValueImpl.java | 2 +- .../midpoint/prism/PrismInternalTestUtil.java | 2 +- .../midpoint/prism/TestPrismParsing.java | 2 +- .../model/impl/ModelObjectResolver.java | 26 +++++++++++--- .../impl/controller/ModelController.java | 4 +-- .../wf/impl/AbstractWfTestPolicy.java | 6 ++-- .../midpoint/repo/common/ObjectResolver.java | 2 +- 10 files changed, 70 insertions(+), 17 deletions(-) create mode 100644 infra/prism-api/src/main/java/com/evolveum/midpoint/prism/ConfigurableVisitor.java diff --git a/infra/common/src/main/java/com/evolveum/midpoint/common/crypto/CryptoUtil.java b/infra/common/src/main/java/com/evolveum/midpoint/common/crypto/CryptoUtil.java index 7e82a460b6c..55c624526e7 100644 --- a/infra/common/src/main/java/com/evolveum/midpoint/common/crypto/CryptoUtil.java +++ b/infra/common/src/main/java/com/evolveum/midpoint/common/crypto/CryptoUtil.java @@ -235,7 +235,7 @@ interface ProtectedStringProcessor { void apply(ProtectedStringType protectedString, String propertyName) throws EncryptionException; } - private static class CombinedVisitor implements Visitor, JaxbVisitor { + private static class CombinedVisitor implements ConfigurableVisitor, JaxbVisitor { private ProtectedStringProcessor processor; private String lastPropName = "?"; @@ -270,6 +270,11 @@ public void visit(Visitable visitable) { } } } + + @Override + public boolean shouldVisitEmbeddedObjects() { + return true; // Needed to encrypt secrets in embedded objects. See also 60328c40b2b99c6cf41ab6ce90145fae941d07bd. + } } @NotNull diff --git a/infra/prism-api/src/main/java/com/evolveum/midpoint/prism/ConfigurableVisitor.java b/infra/prism-api/src/main/java/com/evolveum/midpoint/prism/ConfigurableVisitor.java new file mode 100644 index 00000000000..edfb67e1377 --- /dev/null +++ b/infra/prism-api/src/main/java/com/evolveum/midpoint/prism/ConfigurableVisitor.java @@ -0,0 +1,34 @@ +/* + * Copyright (c) 2020 Evolveum and contributors + * + * This work is dual-licensed under the Apache License 2.0 + * and European Union Public License. See LICENSE file for details. + */ + +package com.evolveum.midpoint.prism; + +import com.evolveum.midpoint.util.annotation.Experimental; + +/** + * Visitor with configurable behavior. + */ +@Experimental +public interface ConfigurableVisitor> extends Visitor { + + /** + * Should we visit also objects that are embedded in references? + */ + boolean shouldVisitEmbeddedObjects(); + + /** + * Helper method. + */ + static boolean shouldVisitEmbeddedObjects(Visitor visitor) { + //noinspection SimplifiableIfStatement + if (visitor instanceof ConfigurableVisitor) { + return ((ConfigurableVisitor) visitor).shouldVisitEmbeddedObjects(); + } else { + return false; // This is the behavior before 60328c40b2b99c6cf41ab6ce90145fae941d07bd (March 24th, 2020) + } + } +} diff --git a/infra/prism-api/src/main/java/com/evolveum/midpoint/prism/util/PrismAsserts.java b/infra/prism-api/src/main/java/com/evolveum/midpoint/prism/util/PrismAsserts.java index c53ab69d6ab..daf8d1b517d 100644 --- a/infra/prism-api/src/main/java/com/evolveum/midpoint/prism/util/PrismAsserts.java +++ b/infra/prism-api/src/main/java/com/evolveum/midpoint/prism/util/PrismAsserts.java @@ -1257,7 +1257,6 @@ public static void assertEquivalent(String message, ItemP public static void assertImmutable(Definition definition) { definition.accept(def -> { - System.out.println("Checking " + def); if (!def.isImmutable()) { fail("Definition is not immutable: " + def); } @@ -1266,7 +1265,6 @@ public static void assertImmutable(Definition definition) { public static void assertMutable(Definition definition) { definition.accept(def -> { - System.out.println("Checking " + def); if (def.isImmutable()) { fail("Definition is not mutable: " + def); } diff --git a/infra/prism-impl/src/main/java/com/evolveum/midpoint/prism/impl/PrismReferenceValueImpl.java b/infra/prism-impl/src/main/java/com/evolveum/midpoint/prism/impl/PrismReferenceValueImpl.java index d1646392eff..92d04fd1e5f 100644 --- a/infra/prism-impl/src/main/java/com/evolveum/midpoint/prism/impl/PrismReferenceValueImpl.java +++ b/infra/prism-impl/src/main/java/com/evolveum/midpoint/prism/impl/PrismReferenceValueImpl.java @@ -714,7 +714,7 @@ public void shortDump(StringBuilder sb) { @Override public void accept(Visitor visitor) { super.accept(visitor); - if (object != null) { + if (object != null && ConfigurableVisitor.shouldVisitEmbeddedObjects(visitor)) { //noinspection unchecked object.accept(visitor); } diff --git a/infra/prism-impl/src/test/java/com/evolveum/midpoint/prism/PrismInternalTestUtil.java b/infra/prism-impl/src/test/java/com/evolveum/midpoint/prism/PrismInternalTestUtil.java index 8f737de9b6c..f9a049ec8ed 100644 --- a/infra/prism-impl/src/test/java/com/evolveum/midpoint/prism/PrismInternalTestUtil.java +++ b/infra/prism-impl/src/test/java/com/evolveum/midpoint/prism/PrismInternalTestUtil.java @@ -296,7 +296,7 @@ public static void assertUserJack(PrismObject user, boolean expectRawI user.assertDefinitions("test"); assertUserJackContent(user, expectRawInConstructions, expectFullPolyName, withIncomplete); assertUserJackExtension(user, withIncomplete); - assertVisitor(user, 75); + assertVisitor(user, 71); assertPathVisitor(user, UserType.F_ASSIGNMENT, true, 9); assertPathVisitor(user, ItemPath.create(UserType.F_ASSIGNMENT, USER_ASSIGNMENT_1_ID), true, 3); diff --git a/infra/prism-impl/src/test/java/com/evolveum/midpoint/prism/TestPrismParsing.java b/infra/prism-impl/src/test/java/com/evolveum/midpoint/prism/TestPrismParsing.java index 697e08d489f..3171fe04331 100644 --- a/infra/prism-impl/src/test/java/com/evolveum/midpoint/prism/TestPrismParsing.java +++ b/infra/prism-impl/src/test/java/com/evolveum/midpoint/prism/TestPrismParsing.java @@ -390,7 +390,7 @@ protected void assertUserAdhoc(PrismObject user, boolean expectRawInCo user.checkConsistence(); assertUserJackContent(user, expectRawInConstructions, true, withIncomplete); assertUserExtensionAdhoc(user); - assertVisitor(user, 62); + assertVisitor(user, 58); } private void assertUserExtensionAdhoc(PrismObject user) { diff --git a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/ModelObjectResolver.java b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/ModelObjectResolver.java index 4efe6b7cb25..eb8efc7bbe3 100644 --- a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/ModelObjectResolver.java +++ b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/ModelObjectResolver.java @@ -17,6 +17,7 @@ import com.evolveum.midpoint.schema.SearchResultList; import com.evolveum.midpoint.task.api.TaskManager; +import com.evolveum.midpoint.util.annotation.Experimental; import com.evolveum.midpoint.wf.api.WorkflowManager; import com.evolveum.midpoint.xml.ns._public.common.common_3.*; @@ -325,13 +326,30 @@ public R searchOrgTreeWidthFirst(PrismObject object, return null; } + @Experimental @Override - public void resolveAllReferences(Collection pcvs, Object taskObject, OperationResult result) { + public void resolveAllReferences(Collection> pcvs, Object taskObject, OperationResult result) { Session session = openResolutionSession(null); Task task = (Task) taskObject; - Visitor visitor = (o) -> { - if (o instanceof PrismReferenceValue) { - resolveReference((PrismReferenceValue) o, "resolving object reference", session, task, result); + ConfigurableVisitor visitor = new ConfigurableVisitor() { + @Override + public boolean shouldVisitEmbeddedObjects() { + // This is to avoid endless recursion when resolving cases: A parent case has references to its children + // whereas child cases have references to their parent. We could deal with this using SmartVisitable + // but that would be overkill for the basic use - it would resolve much more than needed. So we simply stop + // visiting embedded objects because in the basic use case we are simply not interested in them. + // See also MID-6171. + // + // Should we need this feature we will add an option or create a separate method + // e.g. "resolveAllReferencesDeeply". + return false; + } + + @Override + public void visit(Visitable visitable) { + if (visitable instanceof PrismReferenceValue) { + resolveReference((PrismReferenceValue) visitable, "resolving object reference", session, task, result); + } } }; pcvs.forEach(pcv -> pcv.accept(visitor)); diff --git a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/controller/ModelController.java b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/controller/ModelController.java index 3a1052aceb4..d3e76428772 100644 --- a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/controller/ModelController.java +++ b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/controller/ModelController.java @@ -2297,10 +2297,10 @@ private void computePolyStringVisit(Visitable visitable) { PolyString polyString = pval.getValue(); if (polyString.getOrig() == null) { String orig = localizationService.translate(polyString); - LOGGER.info("PPPP1: Filling out orig value of polyString {}: {}", polyString, orig); + LOGGER.trace("PPPP1: Filling out orig value of polyString {}: {}", polyString, orig); polyString.setComputedOrig(orig); polyString.recompute(prismContext.getDefaultPolyStringNormalizer()); - LOGGER.info("PPPP2: Resulting polyString: {}", polyString); + LOGGER.trace("PPPP2: Resulting polyString: {}", polyString); } } } diff --git a/model/workflow-impl/src/test/java/com/evolveum/midpoint/wf/impl/AbstractWfTestPolicy.java b/model/workflow-impl/src/test/java/com/evolveum/midpoint/wf/impl/AbstractWfTestPolicy.java index a420267eb2c..104a8b84c49 100644 --- a/model/workflow-impl/src/test/java/com/evolveum/midpoint/wf/impl/AbstractWfTestPolicy.java +++ b/model/workflow-impl/src/test/java/com/evolveum/midpoint/wf/impl/AbstractWfTestPolicy.java @@ -17,10 +17,7 @@ import static com.evolveum.midpoint.xml.ns._public.common.common_3.CaseWorkItemType.F_ORIGINAL_ASSIGNEE_REF; import java.io.File; -import java.util.ArrayList; -import java.util.Collection; -import java.util.Comparator; -import java.util.List; +import java.util.*; import org.apache.commons.collections4.CollectionUtils; import org.springframework.beans.factory.annotation.Autowired; @@ -466,6 +463,7 @@ protected void afterFirstClockworkRun(CaseType rootCase, if (subcase.getApprovalContext() != null) { OperationResult opResult = new OperationResult("dummy"); ApprovalSchemaExecutionInformationType info = workflowManager.getApprovalSchemaExecutionInformation(subcase.getOid(), opTask, opResult); + modelObjectResolver.resolveAllReferences(Collections.singleton(info.asPrismContainerValue()), opTask, result); // MID-6171 display("Execution info for " + subcase, info); opResult.computeStatus(); assertSuccess("Unexpected problem when looking at getApprovalSchemaExecutionInformation result", opResult); diff --git a/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/ObjectResolver.java b/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/ObjectResolver.java index d644a4ebc7f..e776d70d485 100644 --- a/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/ObjectResolver.java +++ b/repo/repo-common/src/main/java/com/evolveum/midpoint/repo/common/ObjectResolver.java @@ -73,7 +73,7 @@ SearchResultList> searchObjects(Class t // EXPERIMENTAL (implemented only for ModelObjectResolver) // TODO clean up this mess - default void resolveAllReferences(Collection pcvs, Object taskObject, OperationResult result) { + default void resolveAllReferences(Collection> pcvs, Object taskObject, OperationResult result) { throw new UnsupportedOperationException(); }