-
Notifications
You must be signed in to change notification settings - Fork 4.5k
✨ airbyte-cdk - Adds JwtAuthenticator
to low-code
#37005
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.
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
Changes from 14 commits
ce29fb5
bb0789d
e12254b
dca7b06
58df22d
c8160c8
16f5165
1b7b006
d41e009
a1b2965
b25df50
86809bd
63db2a6
40717f5
0e374b3
70c2deb
7388a46
b67ec03
0aa43a6
342cfad
a927323
46ff236
111180a
5bc18dd
e351f3e
00db62e
8db26a5
0c14620
790e7ea
ae0cef5
dd6377b
252f863
0284d30
be03266
7515a54
2a419e7
47c4604
4b8512d
a7559cb
2772d66
0e4b8e2
aeca944
9a32e55
c7bd90b
0723fee
bb839c9
6075a42
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,9 @@ | |
# | ||
|
||
from airbyte_cdk.sources.declarative.auth.oauth import DeclarativeOauth2Authenticator | ||
from airbyte_cdk.sources.declarative.auth.jwt import JwtAuthenticator | ||
|
||
__all__ = [ | ||
"DeclarativeOauth2Authenticator", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder why SelectiveAuthenticator isn't here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have a good answer for you. Oauth is re-exported via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably not intentional. It shouldn't matter as these classes should only be used through the YAML interface |
||
"JwtAuthenticator" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For consistency, should this also be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems the oauth2 auth would be the outlier: |
||
] |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,102 @@ | ||||||
# | ||||||
# Copyright (c) 2023 Airbyte, Inc., all rights reserved. | ||||||
# | ||||||
|
||||||
from dataclasses import InitVar, dataclass | ||||||
from datetime import datetime | ||||||
from typing import Any, Mapping, Optional, Union | ||||||
|
||||||
import jwt | ||||||
from airbyte_cdk.sources.declarative.auth.declarative_authenticator import DeclarativeAuthenticator | ||||||
from airbyte_cdk.sources.declarative.interpolation.interpolated_mapping import InterpolatedMapping | ||||||
from airbyte_cdk.sources.declarative.interpolation.interpolated_string import InterpolatedString | ||||||
|
||||||
|
||||||
@dataclass | ||||||
class JwtAuthenticator(DeclarativeAuthenticator): | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have no idea what our naming convention is — do we want acronyms to be all caps? I.e. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not attached to it but I find There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I followed |
||||||
|
||||||
config: Mapping[str, Any] | ||||||
parameters: InitVar[Mapping[str, Any]] | ||||||
secret_key: Union[InterpolatedString, str] | ||||||
algorithm: Union[InterpolatedString, str] | ||||||
token_duration: Union[InterpolatedString, str] = 1200 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be a union[interpolated string, str, int]? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated this to make token duration only an integer. |
||||||
kid: Union[InterpolatedString, str] = None | ||||||
typ: Union[InterpolatedString, str] = "JWT" | ||||||
iss: Union[InterpolatedString, str] = None | ||||||
sub: Union[InterpolatedString, str] = None | ||||||
aud: Union[InterpolatedString, str] = None | ||||||
cty: Union[InterpolatedString, str] = None | ||||||
additional_jwt_headers: Mapping[str, Any] = None | ||||||
additional_jwt_payload: Mapping[str, Any] = None | ||||||
|
||||||
def __post_init__(self, parameters: Mapping[str, Any]) -> None: | ||||||
super().__init__() | ||||||
self._algorithm = InterpolatedString.create(self.algorithm, parameters=parameters) | ||||||
self._secret_key = InterpolatedString.create(self.secret_key, parameters=parameters) | ||||||
self._kid = InterpolatedString.create(self.kid, parameters=parameters) | ||||||
self._typ = InterpolatedString.create(self.typ, parameters=parameters) | ||||||
self._iss = InterpolatedString.create(self.iss, parameters=parameters) | ||||||
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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably also need to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated this to make token duration only an integer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this means the value can't be interpolated. Is that ok? |
||||||
self._additional_jwt_headers = InterpolatedMapping(self.additional_jwt_headers or {}, parameters=parameters) | ||||||
self._additional_jwt_payload = InterpolatedMapping(self.additional_jwt_payload or {}, parameters=parameters) | ||||||
|
||||||
def _get_jwt_headers(self) -> Mapping[str, Any]: | ||||||
headers = {} | ||||||
headers.update(self._additional_jwt_headers.eval(self.config)) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: these two lines could just be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incorporated. |
||||||
if self._kid: | ||||||
headers["kid"] = f"{self._kid.eval(self.config)}" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we fail if "kid" or other required params was also defined in the additional jwt headers? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Added a check for both |
||||||
if self._algorithm: | ||||||
headers["alg"] = self._get_algorithm() | ||||||
if self._typ: | ||||||
headers["typ"] = f"{self._typ.eval(self.config)}" | ||||||
if self._cty: | ||||||
headers["cty"] = f"{self._cty.eval(self.config)}" | ||||||
return headers | ||||||
|
||||||
def _get_jwt_payload(self) -> Mapping[str, Any]: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a comment explaining how this works? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @girarda Sorry, can you clarify what you're requesting? An comment within the code explaining how the _get_jwt_payload works? Or for each method? |
||||||
payload = {} | ||||||
now = int(datetime.now().timestamp()) | ||||||
exp = now + self._token_duration | ||||||
nbf = now | ||||||
payload.update(self._additional_jwt_payload.eval(self.config)) | ||||||
if self._iss: | ||||||
payload["iss"] = f"{self._iss.eval(self.config)}" | ||||||
if self._sub: | ||||||
payload["sub"] = f"{self._sub.eval(self.config)}" | ||||||
if self._aud: | ||||||
payload["aud"] = f"{self._aud.eval(self.config)}" | ||||||
payload["iat"] = now | ||||||
payload["exp"] = exp | ||||||
payload["nbf"] = nbf | ||||||
return payload | ||||||
|
||||||
def _get_algorithm(self) -> str: | ||||||
algorithm: str = self._algorithm.eval(self.config) | ||||||
if not algorithm: | ||||||
raise ValueError("Algorithm is required") | ||||||
return f"{algorithm}" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. |
||||||
|
||||||
def _get_secret_key(self) -> str: | ||||||
secret_key: str = self._secret_key.eval(self.config) | ||||||
if not secret_key: | ||||||
raise ValueError("secret_key is required") | ||||||
return f"{secret_key}" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. |
||||||
|
||||||
def _get_signed_token(self) -> str: | ||||||
return jwt.encode( | ||||||
payload=self._get_jwt_payload(), | ||||||
key=self._get_secret_key(), | ||||||
algorithm=self._get_algorithm(), | ||||||
headers=self._get_jwt_headers(), | ||||||
) | ||||||
|
||||||
@property | ||||||
def auth_header(self) -> str: | ||||||
return "Authorization" | ||||||
|
||||||
@property | ||||||
def token(self) -> str: | ||||||
return f"Bearer {self._get_signed_token()}" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is the prefix guaranteed to always be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. It appears |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -257,13 +257,15 @@ definitions: | |
- "$ref": "#/definitions/BearerAuthenticator" | ||
- "$ref": "#/definitions/CustomAuthenticator" | ||
- "$ref": "#/definitions/OAuthAuthenticator" | ||
- "$ref": "#/definitions/JwtAuthenticator" | ||
- "$ref": "#/definitions/NoAuth" | ||
- "$ref": "#/definitions/SessionTokenAuthenticator" | ||
- "$ref": "#/definitions/LegacySessionTokenAuthenticator" | ||
examples: | ||
- authenticators: | ||
token: "#/definitions/ApiKeyAuthenticator" | ||
oauth: "#/definitions/OAuthAuthenticator" | ||
jwt: "#/definitions/JwtAuthenticator" | ||
$parameters: | ||
type: object | ||
additionalProperties: true | ||
|
@@ -833,6 +835,95 @@ definitions: | |
$parameters: | ||
type: object | ||
additionalProperties: true | ||
JwtAuthenticator: | ||
title: JWT Authenticator | ||
description: Authenticator for requests using JWT authentication flow. | ||
type: object | ||
required: | ||
- type | ||
- secret_key | ||
- algorithm | ||
- jwt_headers | ||
- jwt_payload | ||
properties: | ||
type: | ||
type: string | ||
enum: [JwtAuthenticator] | ||
secret_key: | ||
type: string | ||
description: Secret used to sign the JSON web token. | ||
examples: | ||
- "{{ config['secret_key'] }}" | ||
algorithm: | ||
type: string | ||
description: Algorithm used to sign the JSON web token. | ||
examples: | ||
- "ES256" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be the default? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't believe so... I haven't found any consistency in the algorithm that APIs use: |
||
token_duration: | ||
type: integer | ||
title: Token Duration | ||
description: The amount of time in seconds a JWT token can be valid after being issued. | ||
examples: | ||
- 1200 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a common default value we can use here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good question! can we refresh the token at runtime? this is how we do it for oauth authenticators There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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). |
||
jwt_headers: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are any of those properties required? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, but they are the most common properties. |
||
type: object | ||
title: JWT Headers | ||
description: JWT headers used when signing JSON web token. | ||
additionalProperties: false | ||
properties: | ||
kid: | ||
type: string | ||
title: Key Identifier | ||
description: Private key ID for user account. | ||
examples: | ||
- "{{ config['kid'] }}" | ||
typ: | ||
type: string | ||
title: Type | ||
description: The media type of the complete JWT. | ||
examples: | ||
- JWT | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's set JWT as a default value There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. Incorporated. |
||
cty: | ||
type: string | ||
title: Content Type | ||
description: Content type of JWT header. | ||
examples: | ||
- JWT | ||
additional_jwt_headers: | ||
type: object | ||
title: Additional JWT Headers | ||
description: Additional headers to be included with the JWT headers object. | ||
additionalProperties: true | ||
jwt_payload: | ||
type: object | ||
title: JWT Payload | ||
description: JWT Payload used when signing JSON web token. | ||
additionalProperties: false | ||
properties: | ||
iss: | ||
type: string | ||
title: Issuer | ||
description: The user/principal that issued the JWT. Commonly a value unique to the user. | ||
examples: | ||
- "{{ config['iss'] }}" | ||
sub: | ||
type: string | ||
title: Subject | ||
description: The subject of the JWT. Commonly defined by the API. | ||
aud: | ||
type: string | ||
title: Audience | ||
description: The recipient that the JWT is intended for. Commonly defined by the API. | ||
examples: | ||
- "appstoreconnect-v1" | ||
additional_jwt_payload: | ||
type: object | ||
title: Additional JWT Payload Properties | ||
description: Additional properties to be added to the JWT payload. | ||
additionalProperties: true | ||
$parameters: | ||
type: object | ||
additionalProperties: true | ||
OAuthAuthenticator: | ||
title: OAuth2 | ||
description: Authenticator for requests using OAuth 2.0 authorization flow. | ||
|
@@ -1311,6 +1402,7 @@ definitions: | |
- "$ref": "#/definitions/BearerAuthenticator" | ||
- "$ref": "#/definitions/CustomAuthenticator" | ||
- "$ref": "#/definitions/OAuthAuthenticator" | ||
- "$ref": "#/definitions/JwtAuthenticator" | ||
- "$ref": "#/definitions/NoAuth" | ||
- "$ref": "#/definitions/SessionTokenAuthenticator" | ||
- "$ref": "#/definitions/LegacySessionTokenAuthenticator" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
were changes to this file intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok please only revert changes to this file. It'll be updated as part of the publish action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.