From 9dc418929b43db6b4dca664dc1ce8f2a21b3fbe2 Mon Sep 17 00:00:00 2001 From: Jimmy Ma Date: Tue, 3 Jan 2023 13:36:17 -0800 Subject: [PATCH] Fix copySecrets for top level oneOfs (#20848) --- .../split_secrets/JsonSecretsProcessor.java | 80 ++++++++++--------- .../JsonSecretsProcessorTest.java | 62 ++++++++++++++ 2 files changed, 106 insertions(+), 36 deletions(-) diff --git a/airbyte-config/config-persistence/src/main/java/io/airbyte/config/persistence/split_secrets/JsonSecretsProcessor.java b/airbyte-config/config-persistence/src/main/java/io/airbyte/config/persistence/split_secrets/JsonSecretsProcessor.java index 9e2a385215305..f6af9bbe83253 100644 --- a/airbyte-config/config-persistence/src/main/java/io/airbyte/config/persistence/split_secrets/JsonSecretsProcessor.java +++ b/airbyte-config/config-persistence/src/main/java/io/airbyte/config/persistence/split_secrets/JsonSecretsProcessor.java @@ -104,59 +104,67 @@ public JsonNode copySecrets(final JsonNode src, final JsonNode dst, final JsonNo if (!isValidJsonSchema(schema)) { return dst; } + Preconditions.checkArgument(dst.isObject()); Preconditions.checkArgument(src.isObject()); final ObjectNode dstCopy = dst.deepCopy(); + return copySecretsRecursive(src, dstCopy, schema); + } + + return src; + } + // This function is modifying dstCopy in place. + private JsonNode copySecretsRecursive(final JsonNode src, JsonNode dstCopy, final JsonNode schema) { + // todo (cgardens) this is not safe. should throw. + if (!isValidJsonSchema(schema)) { + return dstCopy; + } + + Preconditions.checkArgument(dstCopy.isObject()); + Preconditions.checkArgument(src.isObject()); + + final Optional combinationKey = findJsonCombinationNode(schema); + if (combinationKey.isPresent()) { + final var arrayNode = (ArrayNode) schema.get(combinationKey.get()); + for (int i = 0; i < arrayNode.size(); i++) { + final JsonNode childSchema = arrayNode.get(i); + /* + * when traversing a oneOf or anyOf if multiple schema in the oneOf or anyOf have the SAME key, but + * a different type, then, without this test, we can try to apply the wrong schema to the object + * resulting in errors because of type mismatches. + */ + if (VALIDATOR.test(childSchema, dstCopy)) { + // Absorb field values if any of the combination option is declaring it as secrets + copySecretsRecursive(src, dstCopy, childSchema); + } + } + } else { final ObjectNode properties = (ObjectNode) schema.get(PROPERTIES_FIELD); for (final String key : Jsons.keys(properties)) { - // If the source object doesn't have this key then we have nothing to copy, so we should skip to the - // next key. - if (!src.has(key)) { + // If the source or destination doesn't have this key then we have nothing to copy, so we should + // skip to the next key. + if (!src.has(key) || !dstCopy.has(key)) { continue; } final JsonNode fieldSchema = properties.get(key); // We only copy the original secret if the destination object isn't attempting to overwrite it // I.e. if the destination object's value is set to the mask, then we can copy the original secret - if (JsonSecretsProcessor.isSecret(fieldSchema) && dst.has(key) && AirbyteSecretConstants.SECRETS_MASK.equals(dst.get(key).asText())) { - dstCopy.set(key, src.get(key)); - } else if (dstCopy.has(key)) { - // If the destination has this key, then we should consider copying it - - // Check if this schema is a combination node; if it is, find a matching sub-schema and copy based - // on that sub-schema - final var combinationKey = findJsonCombinationNode(fieldSchema); - if (combinationKey.isPresent()) { - var combinationCopy = dstCopy.get(key); - final var arrayNode = (ArrayNode) fieldSchema.get(combinationKey.get()); - for (int i = 0; i < arrayNode.size(); i++) { - final JsonNode childSchema = arrayNode.get(i); - /* - * when traversing a oneOf or anyOf if multiple schema in the oneOf or anyOf have the SAME key, but - * a different type, then, without this test, we can try to apply the wrong schema to the object - * resulting in errors because of type mismatches. - */ - if (VALIDATOR.test(childSchema, combinationCopy)) { - // Absorb field values if any of the combination option is declaring it as secrets - combinationCopy = copySecrets(src.get(key), combinationCopy, childSchema); - } - } - dstCopy.set(key, combinationCopy); - } else { - // Otherwise, this is just a plain old json node; recurse into it. If it's not actually an object, - // the recursive call will exit immediately. - final JsonNode copiedField = copySecrets(src.get(key), dstCopy.get(key), fieldSchema); - dstCopy.set(key, copiedField); - } + if (JsonSecretsProcessor.isSecret(fieldSchema) && AirbyteSecretConstants.SECRETS_MASK.equals(dstCopy.get(key).asText())) { + ((ObjectNode) dstCopy).set(key, src.get(key)); + + } else { + // Otherwise, this is just a plain old json node; recurse into it. If it's not actually an object, + // the recursive call will exit immediately. + final JsonNode copiedField = copySecretsRecursive(src.get(key), dstCopy.get(key), fieldSchema); + ((ObjectNode) dstCopy).set(key, copiedField); } } - - return dstCopy; } - return src; + return dstCopy; } static boolean isSecret(final JsonNode obj) { diff --git a/airbyte-config/config-persistence/src/test/java/io/airbyte/config/persistence/split_secrets/JsonSecretsProcessorTest.java b/airbyte-config/config-persistence/src/test/java/io/airbyte/config/persistence/split_secrets/JsonSecretsProcessorTest.java index ba84eeb71c24e..6764736cc5410 100644 --- a/airbyte-config/config-persistence/src/test/java/io/airbyte/config/persistence/split_secrets/JsonSecretsProcessorTest.java +++ b/airbyte-config/config-persistence/src/test/java/io/airbyte/config/persistence/split_secrets/JsonSecretsProcessorTest.java @@ -495,6 +495,68 @@ void doesNotCopySecretsInNestedNonCombinationNodeWhenDestinationMissing() throws assertEquals(expected, copied); } + @Test + void testCopySecretsWithTopLevelOneOf() { + final JsonNode schema = Jsons.deserialize(""" + { + "$schema": "http://json-schema.org/draft-07/schema#", + "title": "E2E Test Destination Spec", + "type": "object", + "oneOf": [ + { + "title": "Silent", + "required": ["type"], + "properties": { + "a_secret": { + "type": "string", + "airbyte_secret": true + } + } + }, + { + "title": "Throttled", + "required": ["type", "millis_per_record"], + "properties": { + "type": { + "type": "string", + "const": "THROTTLED", + "default": "THROTTLED" + }, + "millis_per_record": { + "description": "Number of milli-second to pause in between records.", + "type": "integer" + } + } + } + ] + } + """); + + final JsonNode source = Jsons.deserialize(""" + { + "type": "THROTTLED", + "a_secret": "woot" + } + """); + + final JsonNode destination = Jsons.deserialize(""" + { + "type": "THROTTLED", + "a_secret": "**********" + } + """); + + final JsonNode result = processor.copySecrets(source, destination, schema); + final JsonNode expected = Jsons.deserialize(""" + { + "type": "THROTTLED", + "a_secret": "woot" + } + """); + + assertEquals(expected, result); + } + @Nested class NoOpTest {