Skip to content

Commit

Permalink
refactor: move Oauth helper method to helper file (#14693)
Browse files Browse the repository at this point in the history
## What
<!--
* Describe what the change is solving.
* It helps to add screenshots if it affects the frontend.
-->
Move method to helper file (rename helper file to be more general) so that the method can be used upstack. Auto-apply `final` and other auto-changes from my IDE


## Can this PR be safely reverted and rolled back?
<!--
* If you know that your be safely rolled back, check YES.*
* If that is not the case (e.g. a database migration), check NO.
* If unsure, leave it blank.*
-->
- [x] YES 💚
- [ ] NO ❌
  • Loading branch information
erohmensing committed Nov 27, 2024
1 parent c49d527 commit 7747e50
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 50 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
import io.airbyte.commons.json.JsonPaths;
import io.airbyte.commons.json.Jsons;
import io.airbyte.commons.server.errors.BadObjectSchemaKnownException;
import io.airbyte.commons.server.handlers.helpers.OAuthPathExtractor;
import io.airbyte.commons.server.handlers.helpers.OAuthHelper;
import io.airbyte.config.ActorDefinitionVersion;
import io.airbyte.config.ConfigSchema;
import io.airbyte.config.DestinationConnection;
Expand Down Expand Up @@ -325,7 +325,7 @@ public CompleteOAuthResponse completeSourceOAuth(final CompleteSourceOauthReques
} catch (final Exception e) {
LOGGER.error(ERROR_MESSAGE, e);
}
return mapToCompleteOAuthResponse(result);
return OAuthHelper.mapToCompleteOAuthResponse(result);
}

@SuppressWarnings("PMD.PreserveStackTrace")
Expand Down Expand Up @@ -387,7 +387,7 @@ public CompleteOAuthResponse completeDestinationOAuth(final CompleteDestinationO
} catch (final Exception e) {
LOGGER.error(ERROR_MESSAGE, e);
}
return mapToCompleteOAuthResponse(result);
return OAuthHelper.mapToCompleteOAuthResponse(result);
}

@SuppressWarnings("PMD.PreserveStackTrace")
Expand Down Expand Up @@ -443,37 +443,14 @@ private JsonNode getOAuthInputConfigurationForConsent(final ConnectorSpecificati
final JsonNode hydratedSourceConnectionConfiguration,
final JsonNode oAuthInputConfiguration) {
final Map<String, String> fieldsToGet =
buildJsonPathFromOAuthFlowInitParameters(OAuthPathExtractor.extractOauthConfigurationPaths(
buildJsonPathFromOAuthFlowInitParameters(OAuthHelper.extractOauthConfigurationPaths(
spec.getAdvancedAuth().getOauthConfigSpecification().getOauthUserInputFromConnectorConfigSpecification()));

final JsonNode oAuthInputConfigurationFromDB = getOAuthInputConfiguration(hydratedSourceConnectionConfiguration, fieldsToGet);

return getOauthFromDBIfNeeded(oAuthInputConfigurationFromDB, oAuthInputConfiguration);
}

CompleteOAuthResponse mapToCompleteOAuthResponse(final Map<String, Object> input) {
final CompleteOAuthResponse response = new CompleteOAuthResponse();
response.setAuthPayload(new HashMap<>());

if (input.containsKey("request_succeeded")) {
response.setRequestSucceeded("true".equals(input.get("request_succeeded")));
} else {
response.setRequestSucceeded(true);
}

if (input.containsKey("request_error")) {
response.setRequestError(input.get("request_error").toString());
}

input.forEach((k, v) -> {
if (!"request_succeeded".equals(k) && !"request_error".equals(k)) {
response.getAuthPayload().put(k, v);
}
});

return response;
}

@VisibleForTesting
Map<String, String> buildJsonPathFromOAuthFlowInitParameters(final Map<String, List<String>> oAuthFlowInitParameters) {
return oAuthFlowInitParameters.entrySet().stream()
Expand Down Expand Up @@ -558,7 +535,7 @@ public CompleteOAuthResponse writeOAuthResponseSecret(final UUID workspaceId, fi
generateOAuthSecretCoordinate(workspaceId),
payloadString, null);
}
return mapToCompleteOAuthResponse(Map.of("secretId", secretCoordinate.getFullCoordinate()));
return OAuthHelper.mapToCompleteOAuthResponse(Map.of("secretId", secretCoordinate.getFullCoordinate()));

} catch (final JsonProcessingException e) {
throw new RuntimeException("Json object could not be written to string.", e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,16 @@
package io.airbyte.commons.server.handlers.helpers;

import com.fasterxml.jackson.databind.JsonNode;
import io.airbyte.api.model.generated.CompleteOAuthResponse;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

/**
* Extract paths to oauth fields from an oauth spec.
* Static helpers for Oauth-related reading and writing.
*/
public class OAuthPathExtractor {
public class OAuthHelper {

private static final String PROPERTIES = "properties";
private static final String PATH_IN_CONNECTOR_CONFIG = "path_in_connector_config";
Expand Down Expand Up @@ -46,4 +47,33 @@ public static Map<String, List<String>> extractOauthConfigurationPaths(final Jso
}
}

/**
* Map to the result of a completeOauth request to an API response.
*
* @param input input
* @return complete oauth response
*/
public static CompleteOAuthResponse mapToCompleteOAuthResponse(final Map<String, Object> input) {
final CompleteOAuthResponse response = new CompleteOAuthResponse();
response.setAuthPayload(new HashMap<>());

if (input.containsKey("request_succeeded")) {
response.setRequestSucceeded("true".equals(input.get("request_succeeded")));
} else {
response.setRequestSucceeded(true);
}

if (input.containsKey("request_error")) {
response.setRequestError(input.get("request_error").toString());
}

input.forEach((k, v) -> {
if (!"request_succeeded".equals(k) && !"request_error".equals(k)) {
response.getAuthPayload().put(k, v);
}
});

return response;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ public static JsonNode setSecretsInConnectionConfiguration(final ConnectorSpecif
* path_in_connector_config i.e. { client_id: ['credentials', 'client_id']}
*/
@VisibleForTesting
public static Map<String, List<String>> getAdvancedAuthOAuthPaths(final ConnectorSpecification connectorSpecification, boolean includeOutputPaths)
public static Map<String, List<String>> getAdvancedAuthOAuthPaths(final ConnectorSpecification connectorSpecification,
final boolean includeOutputPaths)
throws JsonValidationException {
if (OAuthConfigSupplier.hasOAuthConfigSpecification(connectorSpecification)) {
final JsonNode completeOAuthOutputSpecification =
Expand All @@ -65,9 +66,9 @@ public static Map<String, List<String>> getAdvancedAuthOAuthPaths(final Connecto
connectorSpecification.getAdvancedAuth().getOauthConfigSpecification().getCompleteOauthServerOutputSpecification();

// Merge all the mappings into one map
Map<String, List<String>> result = new HashMap<>(OAuthPathExtractor.extractOauthConfigurationPaths(completeOAuthServerOutputSpecification));
final Map<String, List<String>> result = new HashMap<>(OAuthHelper.extractOauthConfigurationPaths(completeOAuthServerOutputSpecification));
if (includeOutputPaths) {
result.putAll(OAuthPathExtractor.extractOauthConfigurationPaths(completeOAuthOutputSpecification));
result.putAll(OAuthHelper.extractOauthConfigurationPaths(completeOAuthOutputSpecification));
}
return result;
} else {
Expand All @@ -83,7 +84,7 @@ public static Map<String, List<String>> getAdvancedAuthOAuthPaths(final Connecto
* @param spec - connector specification to get paths for
* @return Map where the key = the property and the value = the path to the property in list form.
*/
public static Map<String, List<String>> getOAuthConfigPaths(ConnectorSpecification spec) throws JsonValidationException {
public static Map<String, List<String>> getOAuthConfigPaths(final ConnectorSpecification spec) throws JsonValidationException {
if (OAuthConfigSupplier.hasOAuthConfigSpecification(spec)) {
return getAdvancedAuthOAuthPaths(spec, true);
} else {
Expand All @@ -98,7 +99,7 @@ public static Map<String, List<String>> getOAuthConfigPaths(ConnectorSpecificati
* @param spec - connector specification to get paths for
* @return Map where the key = the property and the value = the path to the property in list form.
*/
public static Map<String, List<String>> getOAuthInputPaths(ConnectorSpecification spec) throws JsonValidationException {
public static Map<String, List<String>> getOAuthInputPaths(final ConnectorSpecification spec) throws JsonValidationException {
if (OAuthConfigSupplier.hasOAuthConfigSpecification(spec)) {
return getAdvancedAuthOAuthPaths(spec, false);
} else {
Expand All @@ -121,7 +122,7 @@ public static Map<String, List<String>> getCompleteOauthServerOutputPaths(final
connectorSpecification.getAdvancedAuth().getOauthConfigSpecification().getCompleteOauthServerOutputSpecification();

// Merge all the mappings into one map
return new HashMap<>(OAuthPathExtractor.extractOauthConfigurationPaths(completeOAuthServerOutputSpecification));
return new HashMap<>(OAuthHelper.extractOauthConfigurationPaths(completeOAuthServerOutputSpecification));
} else {
throw new JsonValidationException(
String.format("Error parsing advancedAuth - see [%s]", connectorSpecification.getDocumentationUrl()));
Expand Down Expand Up @@ -150,10 +151,10 @@ public static ConnectorSpecification validateOauthParamConfigAndReturnAdvancedAu
final JsonNode oauthParamConfiguration)
throws JsonValidationException {
if (OAuthConfigSupplier.hasOAuthConfigSpecification(connectorSpecification)) {
JsonNode newConnectorSpecificationNode = Jsons.emptyObject();
Map<String, Boolean> airbyteSecret = Map.of("airbyte_secret", true);
final JsonNode newConnectorSpecificationNode = Jsons.emptyObject();
final Map<String, Boolean> airbyteSecret = Map.of("airbyte_secret", true);
final Map<String, List<String>> oauthPaths = OAuthSecretHelper.getCompleteOauthServerOutputPaths(connectorSpecification);
for (Entry<String, List<String>> entry : oauthPaths.entrySet()) {
for (final Entry<String, List<String>> entry : oauthPaths.entrySet()) {
final List<String> jsonPathList = entry.getValue();
if (Jsons.navigateTo(oauthParamConfiguration, jsonPathList) == null) {
throw new BadObjectSchemaKnownException(String.format("Missing OAuth param for key at %s", jsonPathList));
Expand All @@ -177,9 +178,9 @@ public static ConnectorSpecification validateOauthParamConfigAndReturnAdvancedAu
*/
private static List<String> alternatingList(final String property,
final List<String> list) {
List<String> result = new ArrayList<String>(list.size() * 2);
final List<String> result = new ArrayList<String>(list.size() * 2);

for (String item : list) {
for (final String item : list) {
result.add(property);
result.add(item);
}
Expand All @@ -194,7 +195,7 @@ public static void validateNoSecretsInConfiguration(final ConnectorSpecification
final JsonNode connectionConfiguration)
throws JsonValidationException {
if (OAuthConfigSupplier.hasOAuthConfigSpecification(spec)) {
Map<String, List<String>> oauthPaths = getOAuthInputPaths(spec);
final Map<String, List<String>> oauthPaths = getOAuthInputPaths(spec);
for (final Entry<String, List<String>> entry : oauthPaths.entrySet()) {
final String key = entry.getKey();
final List<String> jsonPathList = entry.getValue();
Expand All @@ -204,11 +205,11 @@ public static void validateNoSecretsInConfiguration(final ConnectorSpecification
}
}

private static void throwIfKeyExistsInConfig(JsonNode connectionConfiguration, String key, List<String> jsonPathList) {
private static void throwIfKeyExistsInConfig(final JsonNode connectionConfiguration, final String key, final List<String> jsonPathList) {
if (Jsons.navigateTo(connectionConfiguration, jsonPathList) != null) {
// The API referenced by this message is a Cloud feature and not yet available in the open source
// project but will be added.
String errorMessage = String.format(
final String errorMessage = String.format(
"Cannot set key '%s', please create an OAuth credentials override instead - https://reference.airbyte.com/reference/workspaceoauthcredentials",
key);
throw new BadObjectSchemaKnownException(errorMessage);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import io.airbyte.api.model.generated.SetInstancewideDestinationOauthParamsRequestBody;
import io.airbyte.api.model.generated.SetInstancewideSourceOauthParamsRequestBody;
import io.airbyte.commons.json.Jsons;
import io.airbyte.commons.server.handlers.helpers.OAuthHelper;
import io.airbyte.config.DestinationOAuthParameter;
import io.airbyte.config.SourceOAuthParameter;
import io.airbyte.config.persistence.ActorDefinitionVersionHelper;
Expand Down Expand Up @@ -284,9 +285,10 @@ void testCompleteSourceOAuthHandleReturnSecret()
final OAuthHandler handlerSpy = Mockito.spy(handler);

doReturn(
handler.mapToCompleteOAuthResponse(Map.of("access_token", "access", "refresh_token", "refresh"))).when(handlerSpy).completeSourceOAuth(any());
OAuthHelper.mapToCompleteOAuthResponse(Map.of("access_token", "access", "refresh_token", "refresh"))).when(handlerSpy)
.completeSourceOAuth(any());
doReturn(
handler.mapToCompleteOAuthResponse(Map.of("secret_id", "secret"))).when(handlerSpy).writeOAuthResponseSecret(any(), any());
OAuthHelper.mapToCompleteOAuthResponse(Map.of("secret_id", "secret"))).when(handlerSpy).writeOAuthResponseSecret(any(), any());

handlerSpy.completeSourceOAuthHandleReturnSecret(completeSourceOauthRequest);

Expand All @@ -313,7 +315,7 @@ void testCompleteSourceOAuthHandleReturnSecret()

@Test
void testGetSourceOAuthParamConfigNoFeatureFlag()
throws JsonValidationException, ConfigNotFoundException, IOException, io.airbyte.data.exceptions.ConfigNotFoundException {
throws JsonValidationException, IOException, io.airbyte.data.exceptions.ConfigNotFoundException {
final UUID sourceDefinitionId = UUID.randomUUID();
final UUID workspaceId = UUID.randomUUID();
final SourceOAuthParameter sourceOAuthParameter = new SourceOAuthParameter()
Expand All @@ -333,7 +335,7 @@ void testGetSourceOAuthParamConfigNoFeatureFlag()

@Test
void testGetSourceOAuthParamConfigFeatureFlagNoOverride()
throws JsonValidationException, ConfigNotFoundException, IOException, io.airbyte.data.exceptions.ConfigNotFoundException {
throws JsonValidationException, IOException, io.airbyte.data.exceptions.ConfigNotFoundException {
final UUID sourceDefinitionId = UUID.randomUUID();
final UUID workspaceId = UUID.randomUUID();
final SourceOAuthParameter sourceOAuthParameter = new SourceOAuthParameter()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Test;

class OAuthPathExtractorTest {
class OAuthHelperTest {

@Test
void testExtract() {
Expand All @@ -36,7 +36,7 @@ void testExtract() {
Map.entry("tenant_id", List.of("tenant_id")),
Map.entry("another_property", List.of("another", "property")));

Assertions.assertThat(OAuthPathExtractor.extractOauthConfigurationPaths(input))
Assertions.assertThat(OAuthHelper.extractOauthConfigurationPaths(input))
.containsExactlyInAnyOrderEntriesOf(expected);
}

Expand Down

0 comments on commit 7747e50

Please sign in to comment.