-
Notifications
You must be signed in to change notification settings - Fork 30
feat(oauth): Support refresh_token_error on the OAuthAuthenticatorModel level and … #829
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| Mapping, | ||
| MutableMapping, | ||
| Optional, | ||
| Tuple, | ||
| Type, | ||
| Union, | ||
| cast, | ||
|
|
@@ -400,6 +401,9 @@ | |
| from airbyte_cdk.sources.declarative.models.declarative_component_schema import ( | ||
| RecordSelector as RecordSelectorModel, | ||
| ) | ||
| from airbyte_cdk.sources.declarative.models.declarative_component_schema import ( | ||
| RefreshTokenUpdater as RefreshTokenUpdaterModel, | ||
| ) | ||
| from airbyte_cdk.sources.declarative.models.declarative_component_schema import ( | ||
| RemoveFields as RemoveFieldsModel, | ||
| ) | ||
|
|
@@ -2789,6 +2793,9 @@ def create_oauth_authenticator( | |
| else None | ||
| ) | ||
|
|
||
| refresh_token_error_status_codes, refresh_token_error_key, refresh_token_error_values = ( | ||
| self._get_refresh_token_error_information(model) | ||
| ) | ||
| if model.refresh_token_updater: | ||
| # ignore type error because fixing it would have a lot of dependencies, revisit later | ||
| return DeclarativeSingleUseRefreshTokenOauth2Authenticator( # type: ignore | ||
|
|
@@ -2839,9 +2846,9 @@ def create_oauth_authenticator( | |
| token_expiry_date_format=model.token_expiry_date_format, | ||
| token_expiry_is_time_of_expiration=bool(model.token_expiry_date_format), | ||
| message_repository=self._message_repository, | ||
| refresh_token_error_status_codes=model.refresh_token_updater.refresh_token_error_status_codes, | ||
| refresh_token_error_key=model.refresh_token_updater.refresh_token_error_key, | ||
| refresh_token_error_values=model.refresh_token_updater.refresh_token_error_values, | ||
| refresh_token_error_status_codes=refresh_token_error_status_codes, | ||
| refresh_token_error_key=refresh_token_error_key, | ||
| refresh_token_error_values=refresh_token_error_values, | ||
| ) | ||
| # ignore type error because fixing it would have a lot of dependencies, revisit later | ||
| return DeclarativeOauth2Authenticator( # type: ignore | ||
|
|
@@ -2868,8 +2875,59 @@ def create_oauth_authenticator( | |
| message_repository=self._message_repository, | ||
| profile_assertion=profile_assertion, | ||
| use_profile_assertion=model.use_profile_assertion, | ||
| refresh_token_error_status_codes=refresh_token_error_status_codes, | ||
| refresh_token_error_key=refresh_token_error_key, | ||
| refresh_token_error_values=refresh_token_error_values, | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def _get_refresh_token_error_information( | ||
| model: OAuthAuthenticatorModel, | ||
| ) -> Tuple[Tuple[int, ...], str, Tuple[str, ...]]: | ||
| """ | ||
| In a previous version of the CDK, the auth error as config_error was only done if a refresh token updater was | ||
| defined. As a transition, we added those fields on the OAuthAuthenticatorModel. This method ensures that the | ||
| information is defined only once and return the right fields. | ||
| """ | ||
| refresh_token_updater = model.refresh_token_updater | ||
| is_defined_on_refresh_token_updated = refresh_token_updater and ( | ||
| refresh_token_updater.refresh_token_error_status_codes | ||
| or refresh_token_updater.refresh_token_error_key | ||
| or refresh_token_updater.refresh_token_error_values | ||
| ) | ||
| is_defined_on_oauth_authenticator = ( | ||
| model.refresh_token_error_status_codes | ||
| or model.refresh_token_error_key | ||
| or model.refresh_token_error_values | ||
| ) | ||
| if is_defined_on_refresh_token_updated and is_defined_on_oauth_authenticator: | ||
| raise ValueError( | ||
maxi297 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| "refresh_token_error should either be defined on the OAuthAuthenticatorModel or the RefreshTokenUpdaterModel, not both" | ||
| ) | ||
|
|
||
| if is_defined_on_refresh_token_updated: | ||
| not_optional_refresh_token_updater: RefreshTokenUpdaterModel = refresh_token_updater # type: ignore # we know from the condition that this is not None | ||
| return ( | ||
| tuple(not_optional_refresh_token_updater.refresh_token_error_status_codes) | ||
|
Contributor
Author
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. Note that this will have the same behavior as the previous code but is a bit different. The previous code would pass |
||
| if not_optional_refresh_token_updater.refresh_token_error_status_codes | ||
| else (), | ||
| not_optional_refresh_token_updater.refresh_token_error_key or "", | ||
| tuple(not_optional_refresh_token_updater.refresh_token_error_values) | ||
| if not_optional_refresh_token_updater.refresh_token_error_values | ||
| else (), | ||
| ) | ||
| elif is_defined_on_oauth_authenticator: | ||
| return ( | ||
| tuple(model.refresh_token_error_status_codes) | ||
| if model.refresh_token_error_status_codes | ||
| else (), | ||
| model.refresh_token_error_key or "", | ||
| tuple(model.refresh_token_error_values) if model.refresh_token_error_values else (), | ||
| ) | ||
|
|
||
| # returning default values we think cover most cases | ||
| return (400,), "error", ("invalid_grant", "invalid_permissions") | ||
|
|
||
| def create_offset_increment( | ||
| self, | ||
| model: OffsetIncrementModel, | ||
|
|
||
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.
I struggle a bit with
is_defined_on_refresh_token_updatedandis_defined_on_oauth_authenticatorbecause if someone wanted to disable the error handling to flag these as system_errors, they would have to set impossible values instead of defining them with none values. That being said, it seems like a weird case to handle and there is an escape path so I'm not too worried