Fix serialization of raw deltas (MID-6086)

1. CryptoUtil methods no longer fail on tunneled SchemaExceptions from the depths of prism-impl. 2. parseRealValue now correctly parses ObjectReferenceType objects (no longed using BeanUnmarshaller for their parsing) 3. Prism visitor now visits also objects embedded in reference values. 4. JaxbVisitor in RawType visits the value after being parsed. Fix #2 resolves MID-6086. Fixes #3 and #4 are necessary to correctly encrypt passwords in ShadowType objects embedded in linkRef references.
Evolveum · Mar 24, 2020 · 60328c4 · 60328c4
1 parent f57e359
commit 60328c4
Show file tree

Hide file tree

Showing 14 changed files with 233 additions and 58 deletions.
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
@@ -15,12 +15,11 @@
 import com.evolveum.midpoint.schema.result.OperationResult;
 import com.evolveum.midpoint.schema.result.OperationResultStatus;
 import com.evolveum.midpoint.util.Holder;
-import com.evolveum.midpoint.util.exception.TunnelException;
+import com.evolveum.midpoint.util.MiscUtil;
 import com.evolveum.midpoint.util.logging.Trace;
 import com.evolveum.midpoint.util.logging.TraceManager;
 import com.evolveum.midpoint.xml.ns._public.common.common_3.ObjectType;
 import com.evolveum.prism.xml.ns._public.types_3.ProtectedStringType;
-import com.evolveum.prism.xml.ns._public.types_3.RawType;
 import org.jetbrains.annotations.NotNull;
 
 import javax.crypto.Cipher;
@@ -45,24 +44,24 @@ public class CryptoUtil {
 
     /**
      * Encrypts all encryptable values in the object.
+     *
+     * Note: We could use TunnelException here (it would be cleaner) but the tunneled exception could be
+     * other than EncryptionException! For example, it could come from RawType, carrying a SchemaException.
+     * See MID-6086. So we use throwExceptionAsUnchecked hack instead.
      */
+    @SuppressWarnings("RedundantThrows")
     public static <T extends ObjectType> void encryptValues(Protector protector, PrismObject<T> object) throws EncryptionException {
-        try {
-            object.accept(createEncryptingVisitor(protector));
-        } catch (TunnelException e) {
-            throw (EncryptionException) e.getCause();
-        }
+        //noinspection unchecked
+        object.accept(createEncryptingVisitor(protector));
     }
 
     /**
      * Encrypts all encryptable values in delta.
      */
+    @SuppressWarnings("RedundantThrows")
     public static <T extends ObjectType> void encryptValues(Protector protector, ObjectDelta<T> delta) throws EncryptionException {
-        try {
-            delta.accept(createEncryptingVisitor(protector));
-        } catch (TunnelException e) {
-            throw (EncryptionException) e.getCause();
-        }
+        //noinspection unchecked
+        delta.accept(createEncryptingVisitor(protector));
     }
 
     @NotNull
@@ -76,7 +75,8 @@ private static ProtectedStringProcessor createEncryptingProcessor(Protector prot
                 try {
                     protector.encrypt(protectedString);
                 } catch (EncryptionException e) {
-                    throw new EncryptionException("Failed to encrypt value for field " + propertyName + ": " + e.getMessage(), e);
+                    MiscUtil.throwExceptionAsUnchecked(
+                            new EncryptionException("Failed to encrypt value for field " + propertyName + ": " + e.getMessage(), e));
                 }
             }
         });
@@ -85,6 +85,7 @@ private static ProtectedStringProcessor createEncryptingProcessor(Protector prot
     // Checks that everything is encrypted
     public static <T extends ObjectType> void checkEncrypted(final PrismObject<T> object) {
         try {
+            //noinspection unchecked
             object.accept(createCheckingVisitor());
         } catch (IllegalStateException e) {
             throw new IllegalStateException(e.getMessage() + " in " + object, e);
@@ -95,6 +96,7 @@ public static <T extends ObjectType> void checkEncrypted(final PrismObject<T> ob
     // Checks that everything is encrypted
     public static <T extends ObjectType> void checkEncrypted(final ObjectDelta<T> delta) {
         try {
+            //noinspection unchecked
             delta.accept(createCheckingVisitor());
         } catch (IllegalStateException e) {
             throw new IllegalStateException(e.getMessage() + " in delta " + delta, e);
@@ -184,19 +186,18 @@ public static void securitySelfTest(OperationResult parentTestResult) {
     /**
      * Re-encrypts all encryptable values in the object.
      */
+    @SuppressWarnings("RedundantThrows")
     public static <T extends ObjectType> int reencryptValues(Protector protector, PrismObject<T> object) throws EncryptionException {
-        try {
-            Holder<Integer> modCountHolder = new Holder<>(0);
-            object.accept(createVisitor((ps, propName) -> reencryptProtectedStringType(ps, propName, modCountHolder, protector)));
-            return modCountHolder.getValue();
-        } catch (TunnelException e) {
-            throw (EncryptionException) e.getCause();
-        }
+        Holder<Integer> modCountHolder = new Holder<>(0);
+        //noinspection unchecked
+        object.accept(createVisitor((ps, propName) -> reencryptProtectedStringType(ps, propName, modCountHolder, protector)));
+        return modCountHolder.getValue();
     }
 
     @NotNull
     public static <T extends ObjectType> Collection<String> getEncryptionKeyNames(PrismObject<T> object) {
         Set<String> keys = new HashSet<>();
+        //noinspection unchecked
         object.accept(createVisitor((ps, propName) -> {
             if (ps.getEncryptedDataType() != null && ps.getEncryptedDataType().getKeyInfo() != null) {
                 keys.add(ps.getEncryptedDataType().getKeyInfo().getKeyName());
@@ -208,6 +209,7 @@ public static <T extends ObjectType> Collection<String> getEncryptionKeyNames(Pr
     @SuppressWarnings("unused") // used externally
     public static <T extends ObjectType> boolean containsCleartext(PrismObject<T> object) {
         Holder<Boolean> result = new Holder<>(false);
+        //noinspection unchecked
         object.accept(createVisitor((ps, propName) -> {
             if (ps.getClearValue() != null) {
                 result.setValue(true);
@@ -219,6 +221,7 @@ public static <T extends ObjectType> boolean containsCleartext(PrismObject<T> ob
     @SuppressWarnings("unused") // used externally
     public static <T extends ObjectType> boolean containsHashedData(PrismObject<T> object) {
         Holder<Boolean> result = new Holder<>(false);
+        //noinspection unchecked
         object.accept(createVisitor((ps, propName) -> {
             if (ps.getHashedDataType() != null) {
                 result.setValue(true);
@@ -237,7 +240,7 @@ private static class CombinedVisitor implements Visitor, JaxbVisitor {
         private ProtectedStringProcessor processor;
         private String lastPropName = "?";
 
-        CombinedVisitor(ProtectedStringProcessor processor) {
+        private CombinedVisitor(ProtectedStringProcessor processor) {
             this.processor = processor;
         }
 
@@ -247,7 +250,7 @@ public void visit(JaxbVisitable visitable) {
                 try {
                     processor.apply(((ProtectedStringType) visitable), lastPropName);
                 } catch (EncryptionException e) {
-                    throw new TunnelException(e);
+                    MiscUtil.throwExceptionAsUnchecked(e);
                 }
             } else {
                 JaxbVisitable.visitPrismStructure(visitable, this);
@@ -257,7 +260,7 @@ public void visit(JaxbVisitable visitable) {
         @Override
         public void visit(Visitable visitable) {
             if (visitable instanceof PrismPropertyValue) {
-                PrismPropertyValue<?> pval = ((PrismPropertyValue) visitable);
+                PrismPropertyValue<?> pval = (PrismPropertyValue<?>) visitable;
                 Object realValue = pval.getRealValue();
                 if (realValue instanceof JaxbVisitable) {
                     String oldLastPropName = lastPropName;
@@ -290,7 +293,7 @@ private static void reencryptProtectedStringType(ProtectedStringType ps, String
                 protector.encrypt(ps);
                 increment(modCountHolder);
             } catch (EncryptionException e) {
-                throw new TunnelException(new EncryptionException("Failed to encrypt value for field " + propName + ": " + e.getMessage(), e));
+                MiscUtil.throwExceptionAsUnchecked(new EncryptionException("Failed to encrypt value for field " + propName + ": " + e.getMessage(), e));
             }
         } else if (ps.getEncryptedDataType() != null) {
             try {
@@ -300,7 +303,7 @@ private static void reencryptProtectedStringType(ProtectedStringType ps, String
                     increment(modCountHolder);
                 }
             } catch (EncryptionException e) {
-                throw new TunnelException(new EncryptionException("Failed to check/reencrypt value for field " + propName + ": " + e.getMessage(), e));
+                MiscUtil.throwExceptionAsUnchecked(new EncryptionException("Failed to check/reencrypt value for field " + propName + ": " + e.getMessage(), e));
             }
         } else {
             // no clear nor encrypted value
@@ -311,6 +314,7 @@ private static void increment(Holder<Integer> countHolder) {
         countHolder.setValue(countHolder.getValue() + 1);
     }
 
+    @SuppressWarnings("SameParameterValue")
     private static void securitySelfTestAlgorithm(String algorithmName, String transformationName,
             Integer keySize, boolean critical, OperationResult parentResult) {
         OperationResult subresult = parentResult.createSubresult(CryptoUtil.class.getName()+".securitySelfTest.algorithm."+algorithmName);

diff --git a/infra/common/src/test/java/com/evolveum/midpoint/common/TestCryptoUtil.java b/infra/common/src/test/java/com/evolveum/midpoint/common/TestCryptoUtil.java
@@ -20,14 +20,22 @@
 import java.util.HashSet;
 import java.util.Set;
 
+import com.evolveum.midpoint.prism.*;
+import com.evolveum.midpoint.prism.delta.ObjectDelta;
+import com.evolveum.midpoint.prism.path.ItemPath;
+
+import com.evolveum.midpoint.schema.DeltaConvertor;
+import com.evolveum.midpoint.schema.constants.SchemaConstants;
+
+import com.evolveum.midpoint.schema.util.ObjectTypeUtil;
+import com.evolveum.prism.xml.ns._public.types_3.ObjectDeltaType;
+
 import org.testng.annotations.BeforeSuite;
 import org.testng.annotations.Listeners;
 import org.testng.annotations.Test;
 import org.xml.sax.SAXException;
 
 import com.evolveum.midpoint.common.crypto.CryptoUtil;
-import com.evolveum.midpoint.prism.PrismContext;
-import com.evolveum.midpoint.prism.PrismObject;
 import com.evolveum.midpoint.prism.crypto.EncryptionException;
 import com.evolveum.midpoint.prism.crypto.KeyStoreBasedProtectorBuilder;
 import com.evolveum.midpoint.prism.crypto.Protector;
@@ -43,13 +51,16 @@
 import com.evolveum.midpoint.xml.ns._public.common.common_3.*;
 import com.evolveum.prism.xml.ns._public.types_3.ProtectedStringType;
 
+import javax.xml.namespace.QName;
+
 @Listeners({ com.evolveum.midpoint.tools.testng.AlphabeticalMethodInterceptor.class })
 public class TestCryptoUtil extends AbstractUnitTest {
 
     private static final File TEST_DIR = new File(TEST_RESOURCES_DIR, "crypto");
     private static final File FILE_USER_JACK = new File(TEST_DIR, "user-jack.xml");
     private static final File FILE_TASK_MODIFY_JACK_PASSWORD = new File(TEST_DIR, "task-modify-jack-password.xml");
     private static final File FILE_TASK_ADD_JACK = new File(TEST_DIR, "task-add-jack.xml");
+    private static final File FILE_TASK_ADD_ACCOUNT = new File(TEST_DIR, "task-add-account.xml");
     private static final File FILE_SYSTEM_CONFIGURATION = new File(TEST_DIR, "system-configuration.xml");
 
     private static final String KEYSTORE_PATH = TEST_RESOURCES_PATH + "/keystore.jceks";
@@ -107,6 +118,76 @@ public void test120EncryptBulkActionTask() throws Exception {
         CryptoUtil.checkEncrypted(task);
     }
 
+    /**
+     * MID-6086
+     */
+    @Test
+    public void test125EncryptAddAccountTask() throws Exception {
+        given();
+        PrismContext prismContext = getPrismContext();
+        PrismObject<UserType> task = prismContext.parserFor(FILE_TASK_ADD_ACCOUNT).xml().parse();
+
+        when();
+        CryptoUtil.encryptValues(protector, task);
+
+        then();
+        String serialized = prismContext.xmlSerializer().serialize(task);
+        System.out.println("After encryption:\n" + serialized);
+        assertFalse("Serialized object contains the password!", serialized.contains(PASSWORD_PLAINTEXT));
+
+        CryptoUtil.checkEncrypted(task);
+    }
+
+    /**
+     * MID-6086
+     */
+    @Test
+    public void test127EncryptAddAccountTaskManuallyConstructed() throws Exception {
+        given();
+        PrismContext prismContext = getPrismContext();
+        PrismObject<TaskType> task = new TaskType(prismContext)
+                .name("test127")
+                .asPrismObject();
+        PrismPropertyDefinition<ObjectDeltaType> deltasDefinition = task.getDefinition()
+                .findPropertyDefinition(ItemPath.create(TaskType.F_EXTENSION, SchemaConstants.MODEL_EXTENSION_OBJECT_DELTAS));
+        PrismProperty<ObjectDeltaType> deltas = deltasDefinition.instantiate();
+
+        ShadowType shadow = new ShadowType(prismContext)
+                .name("some-shadow");
+        PrismContainerDefinition<Containerable> attributesDef = shadow.asPrismObject().getDefinition()
+                .findContainerDefinition(ShadowType.F_ATTRIBUTES);
+        PrismContainer<?> attributes = attributesDef.instantiate();
+        shadow.asPrismObject().add(attributes);
+
+        MutablePrismPropertyDefinition<ProtectedStringType> passwordDef = prismContext.definitionFactory()
+                .createPropertyDefinition(
+                        new QName(SchemaConstants.NS_ICF_SCHEMA, "password"), ProtectedStringType.COMPLEX_TYPE);
+        PrismProperty<ProtectedStringType> password = passwordDef.instantiate();
+        ProtectedStringType passwordRealValue = new ProtectedStringType();
+        passwordRealValue.setClearValue(PASSWORD_PLAINTEXT);
+        password.setRealValue(passwordRealValue);
+        attributes.add(password);
+
+        PrismReferenceValue linkToAdd = ObjectTypeUtil.createObjectRefWithFullObject(shadow, prismContext).asReferenceValue();
+        ObjectDelta<UserType> userDelta = prismContext.deltaFor(UserType.class)
+                .item(UserType.F_LINK_REF)
+                .add(linkToAdd)
+                .asObjectDelta("some-oid");
+
+        deltas.addRealValue(DeltaConvertor.toObjectDeltaType(userDelta, null));
+        task.addExtensionItem(deltas);
+
+        when();
+        CryptoUtil.encryptValues(protector, task);
+
+        then();
+        String serialized = prismContext.xmlSerializer().serialize(task);
+        System.out.println("After encryption:\n" + serialized);
+        assertFalse("Serialized object contains the password!", serialized.contains(PASSWORD_PLAINTEXT));
+
+        CryptoUtil.checkEncrypted(task);
+    }
+
     @Test
     public void test130EncryptUserInDelta() throws Exception {
         // GIVEN

diff --git a/infra/common/src/test/resources/crypto/task-add-account.xml b/infra/common/src/test/resources/crypto/task-add-account.xml
@@ -0,0 +1,53 @@
+<?xml version="1.0"?>
+<!--
+  ~ 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.
+  -->
+
+<task xmlns="http://midpoint.evolveum.com/xml/ns/public/common/common-3"
+      xmlns:c="http://midpoint.evolveum.com/xml/ns/public/common/common-3"
+      xmlns:org="http://midpoint.evolveum.com/xml/ns/public/common/org-3"
+      xmlns:ri="http://midpoint.evolveum.com/xml/ns/public/resource/instance-3"
+      xmlns:icfs="http://midpoint.evolveum.com/xml/ns/public/connector/icf-1/resource-schema-3"
+      xmlns:t="http://prism.evolveum.com/xml/ns/public/types-3" oid="a66d6763-eced-47b7-b57d-7bf6be27dcdb" version="5">
+    <name>Execute changes</name>
+    <extension xmlns:mext="http://midpoint.evolveum.com/xml/ns/public/model/extension-3">
+        <mext:objectDeltas>
+            <t:changeType>modify</t:changeType>
+            <t:objectType>c:UserType</t:objectType>
+            <t:oid>07cc8c14-f94a-4da9-86ab-0246fc63bb6b</t:oid>
+            <t:itemDelta>
+                <t:modificationType>add</t:modificationType>
+                <t:path xmlns:c="http://midpoint.evolveum.com/xml/ns/public/common/common-3">c:linkRef</t:path>
+                <t:value xmlns="" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" type="c:ShadowType" xmlns:c="http://midpoint.evolveum.com/xml/ns/public/common/common-3" xsi:type="c:ObjectReferenceType">
+                    <object>
+                        <c:resourceRef oid="ef2bc95b-76e0-48e2-86d6-3d4f02d3fafe" type="c:ResourceType"/>
+                        <c:objectClass>ri:AccountObjectClass</c:objectClass>
+                        <c:kind>account</c:kind>
+                        <c:intent>default</c:intent>
+                        <c:attributes>
+                            <icfs:password xsi:type="t:ProtectedStringType">pass1234word</icfs:password>
+                        </c:attributes>
+                    </object>
+                </t:value>
+            </t:itemDelta>
+        </mext:objectDeltas>
+        <mext:executeOptions>
+            <force>false</force>
+            <reconcile>false</reconcile>
+            <executeImmediatelyAfterApproval>false</executeImmediatelyAfterApproval>
+        </mext:executeOptions>
+    </extension>
+    <taskIdentifier>1585065525794-0-1</taskIdentifier>
+    <ownerRef oid="00000000-0000-0000-0000-000000000002" relation="org:default" type="c:UserType">
+        <!-- administrator -->
+    </ownerRef>
+    <channel>http://midpoint.evolveum.com/xml/ns/public/gui/channels-3#user</channel>
+    <executionStatus>closed</executionStatus>
+    <category>Utility</category>
+    <handlerUri>http://midpoint.evolveum.com/xml/ns/public/model/execute-deltas/handler-3</handlerUri>
+    <recurrence>single</recurrence>
+    <binding>tight</binding>
+</task>
diff --git a/infra/prism-api/src/main/java/com/evolveum/prism/xml/ns/_public/types_3/RawType.java b/infra/prism-api/src/main/java/com/evolveum/prism/xml/ns/_public/types_3/RawType.java
@@ -469,21 +469,20 @@ public String guessFormattedValue() throws SchemaException {
 
     @Override
     public void accept(JaxbVisitor visitor) {
-        visitor.visit(this);
-        if (isParsed()) {
-            Object realValue = parsed.getRealValue();
-            if (realValue instanceof JaxbVisitable) {
-                ((JaxbVisitable) realValue).accept(visitor);
-            }
-        } else if (explicitTypeName != null) {
+        Object value;
+        if (isParsed() || explicitTypeName != null) {
+            // (Potentially) parsing the value before visiting it.
             try {
-                Object value = getValue(true);
-                if (value instanceof JaxbVisitable) {
-                    ((JaxbVisitable) value).accept(visitor);
-                }
+                value = getValue(true);
             } catch (SchemaException e) {
                 throw new TunnelException(e);
             }
+        } else {
+            value = null;
+        }
+        visitor.visit(this);
+        if (value instanceof JaxbVisitable) {
+            ((JaxbVisitable) value).accept(visitor);
         }
     }
 }
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
@@ -710,4 +710,13 @@ public void shortDump(StringBuilder sb) {
             }
         }
     }
+
+    @Override
+    public void accept(Visitor visitor) {
+        super.accept(visitor);
+        if (object != null) {
+            //noinspection unchecked
+            object.accept(visitor);
+        }
+    }
 }
diff --git a/...rism-impl/src/main/java/com/evolveum/midpoint/prism/impl/marshaller/BeanUnmarshaller.java b/...rism-impl/src/main/java/com/evolveum/midpoint/prism/impl/marshaller/BeanUnmarshaller.java
@@ -801,8 +801,8 @@ private void computeParamTypeFromSetter(String propName, Class<?> setterParamTyp
 
         private void computeParamTypeFromGetter(String propName, Class<?> getterReturnType) throws SchemaException {
             if (!Collection.class.isAssignableFrom(getterReturnType)) {
-                throw new SchemaException("Cannot find getter for field " + actualPropertyName + " in " + beanClass
-                        + " does not return collection, cannot use it to set value");
+                throw new SchemaException("Cannot find setter for field " + actualPropertyName + " in " + beanClass
+                        + ". The getter was found, but it does not return collection - so it cannot be used to set the value.");
             }
             // getter.genericReturnType = Collection<...>
             Type typeArgument = inspector.getTypeArgument(getter.getGenericReturnType(),