✨ airbyte-cdk - Adds `JwtAuthenticator` to low-code #37005

pnilan · 2024-04-11T20:50:14Z

What

Adds new JSON Web Token (JWT) authenticator for declarative sources
Resolves https://github.com/airbytehq/airbyte-internal-issues/issues/6836

Notes

Most commonly used properties are included in the JWT Headers and JWT Payload objects in the schema.
Additional fields can be included within the additional_jwt_headers and additional_jwt_payload objects.
I did not include the jti property within the JWT Payload as it requires a unique value for every requests and as a result is not commonly used when accessing public APIs.
Developed based on the following resources:

vercel · 2024-04-11T20:50:20Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Apr 18, 2024 11:38pm

natikgadzhi

I haven't spent enough time, so just dumping comments that I've had so I don't forget about them.

I think it's a good idea to start the frontend piece as well and see how far we can get!

natikgadzhi · 2024-04-11T23:04:36Z

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

@@ -3,7 +3,9 @@
 #

 from airbyte_cdk.sources.declarative.auth.oauth import DeclarativeOauth2Authenticator
+from airbyte_cdk.sources.declarative.auth.jwt import JwtAuthenticator

 __all__ = [
    "DeclarativeOauth2Authenticator",


I wonder why SelectiveAuthenticator isn't here.

I don't have a good answer for you. Oauth is re-exported via __init__ but all other authenticators are accessed directly from their files.

Probably not intentional. It shouldn't matter as these classes should only be used through the YAML interface

natikgadzhi · 2024-04-11T23:04:50Z

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


 __all__ = [
    "DeclarativeOauth2Authenticator",
+    "JwtAuthenticator"


For consistency, should this also be DeclarativeJWTAuthenticator?

It seems the oauth2 auth would be the outlier: ApiKeyAuthenticator, BearerAuthenticator, BasicHttpAuthenticator, etc don't explicitly note declarative.

natikgadzhi · 2024-04-11T23:05:37Z

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

+
+
+@dataclass
+class JwtAuthenticator(DeclarativeAuthenticator):


I have no idea what our naming convention is — do we want acronyms to be all caps? I.e. JWTAuthenticator or JwtAuthenticator?

I'm not attached to it but I find JwtAuthenticator a bit easier to read

I followed ApiKeyAuthenticator's lead.

girarda · 2024-04-12T15:39:50Z

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

+            title: Type
+            description: The media type of the complete JWT.
+            examples:
+              - JWT


let's set JWT as a default value

Agreed. Incorporated.

girarda · 2024-04-12T15:40:54Z

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

+        description: The amount of time in seconds a JWT token can be valid after being issued.
+        examples:
+          - 1200
+      jwt_headers:


are any of those properties required?

No, but they are the most common properties.

girarda · 2024-04-12T15:41:46Z

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

+        type: string
+        description: Algorithm used to sign the JSON web token.
+        examples:
+          - "ES256"


should this be the default?

I don't believe so... I haven't found any consistency in the algorithm that APIs use:

Apple App Store Connect: ES256

Alibaba Cloud: HS256

Athena Health: RS256

Hashicorp Vault: RS256

girarda · 2024-04-12T15:41:59Z

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

+        title: Token Duration
+        description: The amount of time in seconds a JWT token can be valid after being issued.
+        examples:
+          - 1200


is there a common default value we can use here?

I haven't seen any consistency, although 1200s (20 min) is the smallest duration I've seen -- maybe a good default.

That being said, does this mean if a sync takes longer than 20 minutes it would fail? Should I include some sort of refresh mechanism? Or, is the authenticator re-instantiated per read?

good question! can we refresh the token at runtime? this is how we do it for oauth authenticators

@girarda Actually I believe we're good -- each time the the request is prepared it will invoke _get_jwt_headers which will "refresh" the expiration time (and therefore refresh the token, as the token is the headers, payload, secret_key all encoded into a single string).

girarda · 2024-04-12T15:44:48Z

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

@@ -807,6 +809,24 @@ def create_json_decoder(model: JsonDecoderModel, config: Config, **kwargs: Any)
    def create_json_file_schema_loader(model: JsonFileSchemaLoaderModel, config: Config, **kwargs: Any) -> JsonFileSchemaLoader:
        return JsonFileSchemaLoader(file_path=model.file_path or "", config=config, parameters=model.parameters or {})

+    @staticmethod
+    def create_jwt_authenticator(model: JwtAuthenticatorModel, config: Config, **kwargs: Any) -> JwtAuthenticator:


let's add unit tests here. be sure to cover to cover cases where optional value are set and omitted

Agreed. Next step is to add unit tests.

girarda · 2024-04-12T16:12:26Z

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

+    parameters: InitVar[Mapping[str, Any]]
+    secret_key: Union[InterpolatedString, str]
+    algorithm: Union[InterpolatedString, str]
+    token_duration: Union[InterpolatedString, str] = 1200


should this be a union[interpolated string, str, int]?

I've updated this to make token duration only an integer.

girarda · 2024-04-12T16:13:22Z

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

+        algorithm: str = self._algorithm.eval(self.config)
+        if not algorithm:
+            raise ValueError("Algorithm is required")
+        return f"{algorithm}"


Suggested change

return f"{algorithm}"

return algorithm

girarda · 2024-04-12T16:13:30Z

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

+        secret_key: str = self._secret_key.eval(self.config)
+        if not secret_key:
+            raise ValueError("secret_key is required")
+        return f"{secret_key}"


Suggested change

return f"{secret_key}"

return secret_key

girarda · 2024-04-12T16:15:34Z

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

+        headers = {}
+        headers.update(self._additional_jwt_headers.eval(self.config))
+        if self._kid:
+            headers["kid"] = f"{self._kid.eval(self.config)}"


should we fail if "kid" or other required params was also defined in the additional jwt headers?

Good point. Added a check for both jwt_headers and jwt_payload.

girarda · 2024-04-12T16:17:32Z

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

+
+
+@dataclass
+class JwtAuthenticator(DeclarativeAuthenticator):


I'm not attached to it but I find JwtAuthenticator a bit easier to read

… values for header properties

…lve mypy issues

…e-cdk version

girarda · 2024-04-18T16:31:46Z

airbyte-cdk/python/CHANGELOG.md

@@ -40,7 +40,7 @@ Fix CDK version mismatch introduced in 0.78.8
 Update error messaging/type for missing streams. Note: version mismatch, please use 0.78.9 instead

 ## 0.78.6
-low-code: add backward compatibility for old close slice behavior 
+low-code: add backward compatibility for old close slice behavior


were changes to this file intentional?

These were unintentional -- I didn't realize there was a Publish CDK action so I had previously manually updated the changelog/pyproject, then reverted the changes.

ok please only revert changes to this file. It'll be updated as part of the publish action

girarda · 2024-04-18T16:34:57Z

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

+            "HS384",
+            "HS512",
+            "ES256",
+            "ES256K",


this enum value isn't in jwt.py

Good catch. Updated.

girarda · 2024-04-18T16:37:20Z

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

+        self._sub = InterpolatedString.create(self.sub, parameters=parameters)
+        self._aud = InterpolatedString.create(self.aud, parameters=parameters)
+        self._cty = InterpolatedString.create(self.cty, parameters=parameters)
+        self._token_duration = self.token_duration


this means the value can't be interpolated. Is that ok?

girarda · 2024-04-18T16:38:31Z

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

+        Builds and returns the payload used when signing the JWT.
+        """
+        now = int(datetime.now().timestamp())
+        exp = now + self._token_duration if isinstance(self._token_duration, int) else now


self._token_duration can only be a int as far as I can tell

So this was a result of being flagged by mypy --> the issue is I set token_duration as optional in the schema, and therefore it has type Optional[int] even though I have a default value to use. So it'll never be None but because of the auto-generated Optional[int] in the model, I can't avoid it from being flagged by mypy. Any workarounds here you're aware of?

Also re "this means the [token_duration] value can't be interpolated. Is that ok?"

I can't think of a scenario where a connector dev would want to use a string to define the token duration. It's in the spec that the iat (issued at), exp (expires at), and nbf (not before) claims be seconds since epoch. Instead of directly defining the exp claim, I have the dev set the token duration in seconds (typically a max value allowed by the API) to dynamically set the exp. This seems to me to be a better approach then expecting a connector dev to define iat and exp direction via jinja expressions (i.e. exp: {{ int(datetime.now().timestamp() + 1200 }}.

girarda · 2024-04-18T16:39:46Z

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

+    @staticmethod
+    def create_jwt_authenticator(model: JwtAuthenticatorModel, config: Config, **kwargs: Any) -> JwtAuthenticator:
+        model.jwt_headers = (
+            JwtHeadersModel(kid=None, typ="JWT", cty=None) if not isinstance(model.jwt_headers, JwtHeadersModel) else model.jwt_headers


what are other valid types for model.jwt_headers?

I made both jwt_headers and jwt_payload optional in the schema, because all their properties are also optional there are scenarios where a dev connector may not need to set kid, typ, cty within the manifest). I covered this scenario in test_model_to_component_factory.py.

Additionally, I woud've preferred just instantiating the model without arguments since all three parameters have defaults, by mypy flagged it, so I added in the arguments.

This makes sense. Can you change the check to is not None? isinstance implies the type might be something else

girarda · 2024-04-18T16:39:56Z

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

+            JwtHeadersModel(kid=None, typ="JWT", cty=None) if not isinstance(model.jwt_headers, JwtHeadersModel) else model.jwt_headers
+        )
+        model.jwt_payload = (
+            JwtPayloadModel(iss=None, sub=None, aud=None) if not isinstance(model.jwt_payload, JwtPayloadModel) else model.jwt_payload


what are other valid types for model.jwt_payload?

See comment regarding model.jwt_headers.

Updated to not None

…ional, and updates `JwtAlgorithm` in `jwt.py`

pnilan added 14 commits April 9, 2024 08:37

Initial jwt authenticator build

ce29fb5

Update to include config eval

bb0789d

Update typo

e12254b

Updates JWT auth, schema, and factory

dca7b06

Update property retrieval in component factory

58df22d

add token duration check

c8160c8

Fix error with exp header setting

16f5165

Update _get_jwt_payload

1b7b006

Updates jwt.auth to interpolate inputs correctly

d41e009

Clean up redundancy by removing "alg" from jwt headers

a1b2965

Updates expiration time setting in jwt payload method

b25df50

Update jwt schema to remove HS256

86809bd

Merge branch 'master' into pnilan/airbyte-cdk-jwt-auth

63db2a6

chore: format code

40717f5

octavia-squidington-iii added the CDK Connector Development Kit label Apr 11, 2024

natikgadzhi reviewed Apr 11, 2024

View reviewed changes

pnilan mentioned this pull request Apr 12, 2024

Source Apple App Store Connect: Test Source for JWT Authenticator` #37027

Closed

girarda reviewed Apr 12, 2024

View reviewed changes

pnilan added 11 commits April 15, 2024 11:07

Adds base64 secret key encoding, updates jwt.py returns, adds default…

0e374b3

… values for header properties

Ran poetry run poe build

70c2deb

Imports InterpolateBoolean for base64 encoding

7388a46

Updates jwt.py to convert values to strings for serialization

b67ec03

Updates jwt.py to convert secret key and algorithm to string

0aa43a6

Update how boolean is handled for base64 encoding

342cfad

Adds base64_encode_secret_key to component factory

a927323

Add dev defined header prefix option

46ff236

Updates header prefix

111180a

Updates get header prefix for brevity

5bc18dd

Cleanup

e351f3e

pnilan added 11 commits April 16, 2024 16:45

chore: format code

ae0cef5

Updates Algorithm to be enumeration based on PyJWT supported algorithms

dd6377b

Updates unit tests for updated algorithm enumeration

252f863

chore: format code

0284d30

Merge branch 'master' into pnilan/airbyte-cdk-jwt-auth

be03266

Remove unused import

7515a54

Updates jwt.py, model_to_componet_factory, and relevant tests to reso…

2a419e7

…lve mypy issues

Update jwt.py for Any type when throwing exception

47c4604

Adds explanations of JWT authenticator methods

4b8512d

chore: format code

a7559cb

Updated for linting

2772d66

pnilan requested review from girarda and natikgadzhi April 17, 2024 21:26

pnilan marked this pull request as ready for review April 17, 2024 21:26

pnilan requested a review from a team April 17, 2024 21:26

reverts changelog, pyproject.toml, and poetry.lock to previous airbyt…

0e4b8e2

…e-cdk version

girarda reviewed Apr 18, 2024

View reviewed changes

pnilan added 4 commits April 18, 2024 12:35

Updates JwtAlgorithm to include all supported algos

aeca944

Merge branch 'master' into pnilan/airbyte-cdk-jwt-auth

9a32e55

Reverst CHANGELOG, updates model_to_component_factory method condit…

c7bd90b

…ional, and updates `JwtAlgorithm` in `jwt.py`

Update low code documentation to include JwtAuthenticator

0723fee

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Apr 18, 2024

pnilan requested a review from girarda April 18, 2024 23:01

pnilan added 2 commits April 18, 2024 16:17

Fix conditional check error for create_jwt_authenticator

bb839c9

Update create_jwt_authenticator jwt_headers and jwt_payload setting

6075a42

girarda approved these changes Apr 19, 2024

View reviewed changes

pnilan merged commit b20cd1b into master Apr 19, 2024
32 checks passed

pnilan deleted the pnilan/airbyte-cdk-jwt-auth branch April 19, 2024 16:17

strosek pushed a commit that referenced this pull request Apr 24, 2024

✨ airbyte-cdk - Adds JwtAuthenticator to low-code (#37005)

269bda0

pnilan mentioned this pull request Apr 26, 2024

Low-code CDK: Support JWT bearer token oauth flow #25728

Closed



		@dataclass
		class JwtAuthenticator(DeclarativeAuthenticator):



		@dataclass
		class JwtAuthenticator(DeclarativeAuthenticator):

✨ airbyte-cdk - Adds JwtAuthenticator to low-code #37005

✨ airbyte-cdk - Adds JwtAuthenticator to low-code #37005

Conversation

pnilan commented Apr 11, 2024 • edited Loading

What

Notes

vercel bot commented Apr 11, 2024 • edited Loading

natikgadzhi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnilan Apr 17, 2024 • edited Loading

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

pnilan Apr 15, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnilan Apr 18, 2024 • edited Loading

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

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

pnilan Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

✨ airbyte-cdk - Adds `JwtAuthenticator` to low-code #37005

✨ airbyte-cdk - Adds `JwtAuthenticator` to low-code #37005

pnilan commented Apr 11, 2024 •

edited

Loading

vercel bot commented Apr 11, 2024 •

edited

Loading

pnilan Apr 17, 2024 •

edited

Loading

pnilan Apr 15, 2024 •

edited

Loading

pnilan Apr 18, 2024 •

edited

Loading

pnilan Apr 18, 2024 •

edited

Loading