Bmoric/refacto secret processor #11362
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,105 @@ | ||||||
| /* | ||||||
| * Copyright (c) 2021 Airbyte, Inc., all rights reserved. | ||||||
| */ | ||||||
|
|
||||||
| package io.airbyte.config.persistence.split_secrets; | ||||||
|
|
||||||
| import com.fasterxml.jackson.databind.JsonNode; | ||||||
| import com.fasterxml.jackson.databind.node.ArrayNode; | ||||||
| import com.fasterxml.jackson.databind.node.ObjectNode; | ||||||
| import com.google.common.base.Preconditions; | ||||||
| import io.airbyte.commons.json.Jsons; | ||||||
| import lombok.Builder; | ||||||
|
|
||||||
| @Builder | ||||||
| public class JsonSecretsProcessorFactory { | ||||||
|
|
||||||
| @Builder.Default | ||||||
| private boolean maskSecrets = true; | ||||||
|
|
||||||
| @Builder.Default | ||||||
| private boolean copySecrets = true; | ||||||
|
|
||||||
| public JsonSecretsProcessor createJsonSecretsProcessor() { | ||||||
| return new JsonSecretsProcessor() { | ||||||
|
|
||||||
| @Override | ||||||
|
There was a problem hiding this comment. not sure I understand why these methods need to be overridden here. I.e. why not just move There was a problem hiding this comment. It comes from a previous PR: #11296. In order to still be able to use the export functionality, we want to sometime not mask the secret. That's why we have this behavior of having a flag to mask or not mask the secrets. There was a problem hiding this comment. I think my question is more about why // note the non-abstract class
public class JsonSecretsProcessor {
private final boolean maskSecrets;
private final boolean copySecrets;
public JsonNode maskSecrets(final JsonNode obj, final JsonNode schema) {
// do stuff...
}
public JsonNode copySecrets(final JsonNode src, final JsonNode dst, final JsonNode schema) {
// do other stuff...
}
}and then |
||||||
| public JsonNode maskSecrets(final JsonNode obj, final JsonNode schema) { | ||||||
|
There was a problem hiding this comment. nitpick: it seems a bit weird that There was a problem hiding this comment. That's true, I have updated it. |
||||||
| if (maskSecrets) { | ||||||
| // if schema is an object and has a properties field | ||||||
| if (!canBeProcessed(schema)) { | ||||||
| return obj; | ||||||
| } | ||||||
| Preconditions.checkArgument(schema.isObject()); | ||||||
|
There was a problem hiding this comment. nitpick: |
||||||
|
|
||||||
| final SecretKeys secretKeys = getAllSecretKeys(schema); | ||||||
edgao marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| return maskAllSecrets(obj, secretKeys); | ||||||
| } | ||||||
|
|
||||||
| return obj; | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| public JsonNode copySecrets(final JsonNode src, final JsonNode dst, final JsonNode schema) { | ||||||
| if (copySecrets) { | ||||||
| if (!canBeProcessed(schema)) { | ||||||
| return dst; | ||||||
| } | ||||||
| Preconditions.checkArgument(dst.isObject()); | ||||||
| Preconditions.checkArgument(src.isObject()); | ||||||
|
|
||||||
| final ObjectNode dstCopy = dst.deepCopy(); | ||||||
edgao marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
||||||
| 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)) { | ||||||
| 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 value of the secret isn't set to the mask | ||||||
|
There was a problem hiding this comment.
Suggested change
nitpick: match the comment ("copy if equal to mask") with what the branch is actually doing ( |
||||||
| if (JsonSecretsProcessor.isSecret(fieldSchema) && dst.has(key) && dst.get(key).asText().equals(SECRETS_MASK)) { | ||||||
| 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); | ||||||
edgao marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| } | ||||||
| } | ||||||
| dstCopy.set(key, combinationCopy); | ||||||
| } else { | ||||||
| // Otherwise, this is just a plain old json object; recurse into it. | ||||||
|
There was a problem hiding this comment. I think I wrote this else-case originally so this is a bit awkward, but this would be a more helpful comment (right?):
Suggested change
|
||||||
| final JsonNode copiedField = copySecrets(src.get(key), dstCopy.get(key), fieldSchema); | ||||||
| dstCopy.set(key, copiedField); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return dstCopy; | ||||||
| } | ||||||
|
|
||||||
| return src; | ||||||
| } | ||||||
|
|
||||||
| }; | ||||||
| } | ||||||
|
|
||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this ever set to false in production? (and why is it necessary for tests?)