Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(oauth): dont convert config values to string when building consent url #20932

Merged
merged 3 commits into from
Jan 3, 2023
Merged

fix(oauth): dont convert config values to string when building consent url #20932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
34a654b
fix: don't convert config values to strings
pedroslopez Dec 30, 2022
a2a1594
tests
pedroslopez Dec 30, 2022
de70c70
add note
pedroslopez Dec 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 20 additions & 18 deletions airbyte-server/src/main/java/io/airbyte/server/handlers/OAuthHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import static io.airbyte.metrics.lib.ApmTraceConstants.Tags.WORKSPACE_ID_KEY;

import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.node.ObjectNode;
import com.google.common.annotations.VisibleForTesting;
import io.airbyte.analytics.TrackingClient;
import io.airbyte.api.model.generated.CompleteDestinationOAuthRequest;
Expand Down Expand Up @@ -40,7 +41,6 @@
import io.airbyte.validation.json.JsonValidationException;
import java.io.IOException;
import java.net.http.HttpClient;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.UUID;
Expand Down Expand Up @@ -321,23 +321,25 @@ Map<String, String> buildJsonPathFromOAuthFlowInitParameters(final Map<String, L

@VisibleForTesting
JsonNode getOauthFromDBIfNeeded(final JsonNode oAuthInputConfigurationFromDB, final JsonNode oAuthInputConfigurationFromInput) {
final Map<String, String> result = new HashMap<>();

Jsons.deserializeToStringMap(oAuthInputConfigurationFromInput)
.forEach((k, v) -> {
if (AirbyteSecretConstants.SECRETS_MASK.equals(v)) {
if (oAuthInputConfigurationFromDB.has(k)) {
result.put(k, oAuthInputConfigurationFromDB.get(k).textValue());
} else {
LOGGER.warn("Missing the key {} in the config store in DB", k);
}

} else {
result.put(k, v);
}
});

return Jsons.jsonNode(result);
final ObjectNode result = (ObjectNode) Jsons.emptyObject();

oAuthInputConfigurationFromInput.fields().forEachRemaining(entry -> {
final String k = entry.getKey();
final JsonNode v = entry.getValue();

// Note: This does not currently handle replacing masked secrets within nested objects.
Copy link
Contributor Author

@pedroslopez pedroslopez Dec 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this, but it's not an new issue, just commented in case it comes up sometime in the future. I verified with the previous behavior and if there was a nested object in the input configuration, deserializeToStringMap actually threw an error.

if (AirbyteSecretConstants.SECRETS_MASK.equals(v.textValue())) {
if (oAuthInputConfigurationFromDB.has(k)) {
result.set(k, oAuthInputConfigurationFromDB.get(k));
} else {
LOGGER.warn("Missing the key {} in the config store in DB", k);
}
} else {
result.set(k, v);
}
});

return result;
}

@VisibleForTesting
Expand Down
9 changes: 6 additions & 3 deletions airbyte-server/src/test/java/io/airbyte/server/handlers/OAuthHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,23 +205,26 @@ void testGetOauthFromDBIfNeeded() {
"""
{
"testMask": "**********",
"testNotMask": "this"
"testNotMask": "this",
"testOtherType": true
}
""");

final JsonNode fromDb = Jsons.deserialize(
"""
{
"testMask": "mask",
"testNotMask": "notThis"
"testNotMask": "notThis",
"testOtherType": true
}
""");

final JsonNode expected = Jsons.deserialize(
"""
{
"testMask": "mask",
"testNotMask": "this"
"testNotMask": "this",
"testOtherType": true
}
""");

Expand Down