Don't return secrets in the API & Only update credentials when requested #1022
Conversation
sherifnada
requested review from
cgardens,
jrhizor,
michel-tricot and
ChristopheDuong
|
Publishing this for review to get feedback since the approach is slightly different than what was laid out in #986 in that we do not introspect into the value of secrets to figure out if they should be updated or not. |
sherifnada
changed the title
| @@ -1135,6 +1135,8 @@ components: | |||
| $ref: "#/components/schemas/SourceId" | |||
| connectionConfiguration: | |||
| $ref: "#/components/schemas/SourceConfiguration" | |||
| updateConfigurationSecrets: | |||
There was a problem hiding this comment.
this strategy is different from the originally proposed one in the issue #986. Why the change?
How does this implementation handle the case where a configuration has 2 secrets and only one is updated?
| Preconditions.checkArgument(schema.isObject()); | ||
|
|
||
| ObjectNode properties = (ObjectNode) schema.get(PROPERTIES_FIELD); | ||
| JsonNode copy = obj.deepCopy(); |
There was a problem hiding this comment.
Jsons.clone is an option too. I don't know what guarantees deepCopy gives but if you do, then good to go!
There was a problem hiding this comment.
deepCopy can return a different type e.g: ObjectNode. Just a small thing. I didn't realize jsons.clone was a thing
| return obj.isObject() && obj.has(AIRBYTE_SECRET_FIELD) && obj.get(AIRBYTE_SECRET_FIELD).asBoolean(); | ||
| } | ||
|
|
||
| private static void assertValidSchema(JsonNode node) { |
There was a problem hiding this comment.
why do we want to throw is a schema doesn't match our expectations? don't we just want to do nothing? e.g. if there is no top level properties and it is just a oneOf we don't want to fail do we?
There was a problem hiding this comment.
fair enough. will fix
|
Addressing comments, un-requested review for now |
sherifnada
requested review from
cgardens,
michel-tricot,
ChristopheDuong and
jrhizor
|
Ready for another round of review |
What
JsonSecretsProcessorwhich exposes methods for expunging or absorbing secrets to and from configuration objects.JsonSecretsProcessor, change Source/Destination endpoints to never return any sensitive configs.shouldUpdateCredentialsflag toSourceUpdate/DestinationUpdateto indicate whether the values of sensitive fields should be updated or not. If not, ignores the value of secrets even if they are set in the input to the update method.Reading Order
JsonSecretsProcessor.java+JsonSecretsProcessorTest.javaconfig.yamlSourceHandler.javaDestinationHandler.javaTODO