Low code CDK: Allow query param / body injection for api key authenticator #26953

flash1293 · 2023-06-02T13:41:28Z

What

Part of #20790

This PR deprecates the header param of the api key authenticator and introduces a request_option param that allows to also inject the auth key into a query param or the body

How

To allow this I had to change the class hierarchy a bit - the important part is that the simple retriever can spread in authenticator-defined query params and body params into outgoing requests. To do so the interface of the authenticator known to the retriever has to include methods for getting query params and body params (not for headers as this is already handled by the current setup using the Session class from the requests package). Currently this is set to AuthBase. As every authenticator can theoretically be able to do this, DeclarativeAuthenticator is extended with the required methods and the authenticator type in the requester is changed to DeclarativeAuthenticator.

This removes a bunch of double inheritance for other declarative connectors which is a nice side effect.

As an image:

Note that the python class ApiKeyAuthenticator is changed to only support the request option, the deprecated header property is handled in the component factory to isolate this behavior to the yaml layer.

Theoretically, custom authenticators used to work fine as long as they implemented AuthBase, now they have to implement DeclarativeAuthenticator. I checked existing custom authenticators and they all do this already, I guess it was implied that a custom authenticator needs to do this in the first place (it just didn't bear any weight so far). I clarified this in the description.

🚨 User Impact 🚨

Low code CDK users can now inject the token from the api key authenticator in request params or the body:

type: ApiKeyAuthenticator
token: "{{ config.token }}"
inject_into:
   type: "RequestOption"
   field_name: "authKey"
   inject_into: "request_parameter"

The header property is still supported, but should not be used anymore.

The ApiKeyAuthenticator class from airbyte_cdk.sources.declarative.auth.token does not support the header parameter anymore.

airbyte-cdk/python/airbyte_cdk/sources/declarative/auth/declarative_authenticator.py

airbyte-cdk/python/airbyte_cdk/sources/declarative/retrievers/simple_retriever.py

…ameter-authenticator

girarda · 2023-06-05T13:23:23Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/retrievers/simple_retriever.py

@@ -191,6 +197,8 @@ def request_headers(
            self.requester.get_request_headers,
            self.paginator.get_request_headers,
            self.stream_slicer.get_request_headers,
+            # auth headers are handled separately by passing the authenticator to the HttpStream constructor


open question: would it be preferable to pass None to the HttpStream constructor and handle request headers like the query params and body for consistency?

I would like to split this out from the main change as it would require changes to the other authenticators as well. Let's get the changes in to unblock downstream changes and come back to it

airbyte-cdk/python/airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py

girarda · 2023-06-05T13:44:17Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py

+            if model.inject_into
+            else RequestOption(
+                inject_into=RequestOptionType.header,
+                field_name=InterpolatedString.create(model.header, parameters=model.parameters).eval(config),


why is the field_name evaluated here instead of in the authenticator?

Good point, that's not necessary

girarda · 2023-06-05T13:48:10Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/declarative_component_schema.yaml

@@ -110,14 +109,23 @@ definitions:
          - "Token token={{ config['api_key'] }}"
      header:
        title: Header Name
-        description: The name of the HTTP header that will be set to the API key.
+        description: The name of the HTTP header that will be set to the API key. This setting is deprecated, use inject_into instead. If header and inject_into is defined at the same time, header will be ignored.


can we fail if both are set to avoid suprises?

girarda · 2023-06-05T14:16:14Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/auth/declarative_authenticator.py

    """
    Interface used to associate which authenticators can be used as part of the declarative framework
    """

+    def get_request_params(self) -> MutableMapping[str, Any]:


why is this method returning a MutableMapping?

girarda · 2023-06-05T14:16:39Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/auth/declarative_authenticator.py

+        """HTTP request parameter to add to the requests"""
+        return {}
+
+    def get_request_body_data(self) -> Union[Mapping, str]:


nit: the type is incomplete. It should be Union[Mapping[str, Any], str]

girarda · 2023-06-05T14:16:50Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/auth/declarative_authenticator.py

+        """Form-encoded body data to set on the requests"""
+        return {}
+
+    def get_request_body_json(self) -> Mapping:


nit: the type is incomplete

girarda · 2023-06-05T14:17:50Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/auth/token.py

+    def get_request_body_data(self) -> Union[Mapping, str, None]:
+        return self._get_request_options(RequestOptionType.body_data)
+
+    def get_request_body_json(self) -> Union[Mapping, None]:


nit: Optional[Mapping[str, Any]]

girarda · 2023-06-05T14:18:08Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/auth/token.py

+    def get_request_params(self) -> Union[MutableMapping[str, Any], None]:
+        return self._get_request_options(RequestOptionType.request_parameter)
+
+    def get_request_body_data(self) -> Union[Mapping, str, None]:


nit: Optional[Union[Mapping[str, Any], str]

girarda · 2023-06-05T14:18:24Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/auth/token.py

+            options[self._field_name.eval(self.config)] = self.token
+        return options
+
+    def get_request_params(self) -> Union[MutableMapping[str, Any], None]:


nit: Optional[Mapping[str, Any]]

flash1293 · 2023-06-06T09:41:57Z

@girarda Adjusted the PR, could you have another look?

Fails if both inject_into and header are defined
Cleaned up the types

flash1293 · 2023-06-07T09:44:10Z

/test connector=connectors/source-zenloop local_cdk=1

🕑 connectors/source-zenloop https://github.com/airbytehq/airbyte/actions/runs/5198472485
✅ connectors/source-zenloop https://github.com/airbytehq/airbyte/actions/runs/5198472485
No Python unittests run

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:598: The previous and actual discovered catalogs are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:695: This tests currently leads to too much failures. We need to fix the connectors at scale first.
============ 38 passed, 3 skipped, 41 warnings in 81.23s (0:01:21) =============

flash1293 · 2023-06-07T09:54:10Z

/test connector=connectors/source-lokalise local_cdk=1

🕑 connectors/source-lokalise https://github.com/airbytehq/airbyte/actions/runs/5198558454
✅ connectors/source-lokalise https://github.com/airbytehq/airbyte/actions/runs/5198558454
No Python unittests run

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: This connector does not implement incremental sync
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:598: The previous and actual discovered catalogs are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:695: This tests currently leads to too much failures. We need to fix the connectors at scale first.
======================== 35 passed, 4 skipped in 51.16s ========================

make authenticator more flexible

12d979a

octavia-squidington-iii added the CDK Connector Development Kit label Jun 2, 2023

Joe Reuter added 3 commits June 2, 2023 15:42

format

23e1a8e

format

610b48f

format

fd084b8

girarda reviewed Jun 2, 2023

View reviewed changes

airbyte-cdk/python/airbyte_cdk/sources/declarative/auth/declarative_authenticator.py Outdated Show resolved Hide resolved

airbyte-cdk/python/airbyte_cdk/sources/declarative/retrievers/simple_retriever.py Outdated Show resolved Hide resolved

Joe Reuter added 2 commits June 5, 2023 11:30

Merge remote-tracking branch 'origin/master' into flash1293/query-par…

3f81b44

…ameter-authenticator

format

6f95b68

flash1293 marked this pull request as ready for review June 5, 2023 10:06

flash1293 requested a review from a team as a code owner June 5, 2023 10:06

flash1293 requested a review from girarda June 5, 2023 10:06

Joe Reuter added 2 commits June 5, 2023 12:14

format

b03dee7

fix problem

98b3240

girarda reviewed Jun 5, 2023

View reviewed changes

code review

53cc9a1

girarda approved these changes Jun 6, 2023

View reviewed changes

Merge branch 'master' into flash1293/query-parameter-authenticator

e1e885e

flash1293 merged commit c4981a7 into master Jun 7, 2023
17 checks passed

flash1293 deleted the flash1293/query-parameter-authenticator branch June 7, 2023 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Low code CDK: Allow query param / body injection for api key authenticator #26953

Low code CDK: Allow query param / body injection for api key authenticator #26953

flash1293 commented Jun 2, 2023 •

edited

girarda Jun 5, 2023

flash1293 Jun 6, 2023 •

edited

girarda Jun 5, 2023

flash1293 Jun 6, 2023

girarda Jun 5, 2023

girarda Jun 5, 2023

girarda Jun 5, 2023

girarda Jun 5, 2023

girarda Jun 5, 2023

girarda Jun 5, 2023

girarda Jun 5, 2023

flash1293 commented Jun 6, 2023

flash1293 commented Jun 7, 2023 •

edited by github-actions bot

flash1293 commented Jun 7, 2023 •

edited by github-actions bot

Low code CDK: Allow query param / body injection for api key authenticator #26953

Low code CDK: Allow query param / body injection for api key authenticator #26953

Conversation

flash1293 commented Jun 2, 2023 • edited

What

How

🚨 User Impact 🚨

Choose a reason for hiding this comment

flash1293 Jun 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 commented Jun 6, 2023

flash1293 commented Jun 7, 2023 • edited by github-actions bot

Build Passed

flash1293 commented Jun 7, 2023 • edited by github-actions bot

Build Passed

flash1293 commented Jun 2, 2023 •

edited

flash1293 Jun 6, 2023 •

edited

flash1293 commented Jun 7, 2023 •

edited by github-actions bot

flash1293 commented Jun 7, 2023 •

edited by github-actions bot