1004 OAuth2 client credentials flow #1159
Conversation
| authentication.strategy, authentication.configuration # type: ignore | ||
| ) | ||
| auth_strategy.get_access_token(db, code, connection_config) | ||
| connection_config.secrets = {**connection_config.secrets, "code": code} # type: ignore |
There was a problem hiding this comment.
Adding the code outside of the get_access_token to make the function reusable between the different OAuth2 flows.
| @@ -216,7 +174,7 @@ def _validate_and_store_response( | |||
| # This alternate way of specifying the expiration is handled | |||
| # by the optional expires_in field of the OAuth2AuthenticationConfiguration | |||
|
|
|||
| expires_in = response.get("expires_in") or self.expires_in | |||
| expires_in = self.expires_in or response.get("expires_in") | |||
There was a problem hiding this comment.
Switching order to be able to override expiration even if one is provided in the token response.
galvana
added
run unsafe ci checks
galvana
added
run unsafe ci checks
| @@ -259,7 +262,7 @@ def authorize_connection( | |||
| authentication = connection_config.get_saas_config().client_config.authentication # type: ignore | |||
|
|
|||
| try: | |||
| auth_strategy: OAuth2AuthenticationStrategy = get_strategy( | |||
| auth_strategy: OAuth2AuthorizationCodeAuthenticationStrategy = get_strategy( | |||
There was a problem hiding this comment.
i smell a merge conflict with #1163 😅
There was a problem hiding this comment.
this looks really good to me - nice job abstracting out the common components. i have only some very minor comments - please let me know what you think, but i don't consider them blocking.
also, just noting that we'll need to resolve merge conflicts in #1163 once this is merged.
| self.token_request = configuration.token_request | ||
| self.refresh_request = configuration.refresh_request | ||
|
|
||
| @abstractmethod |
There was a problem hiding this comment.
it's probably not a huge deal if things still work fine, but seems a bit strange to me that you're re-defining this abstract method here when it's already been defined in the AuthenticationStrategy base class. perhaps i'm missing the reasoning for doing so, though.
There was a problem hiding this comment.
Nice catch, I deleted the re-definition
| oauth2_client_credentials_connection_config, | ||
| oauth2_client_credentials_configuration, | ||
| ): | ||
| # cast some time magic |
There was a problem hiding this comment.
who said time travel wasn't possible
| }, | ||
| ) | ||
|
|
||
| # make sure we can use the expires_in value in the config if no expires_in is provided |
There was a problem hiding this comment.
nit: should this comment be moved to the docstring for the test method?
There was a problem hiding this comment.
sorry, i spoke a bit too soon on my previous review -- i shouldn't have approved yet because you're missing a CHANGELOG.md update. i actually think this change should be logged as under Breaking Changes because technically it would break any existing configs that have the old oauth2 auth strategy name.
|
Updated the changelog with the breaking change and made the requested changes. |
Purpose
Adding the client credential OAuth2 flow. We previously only supported the authentication code flow but we need to be able to support additional flows for different SaaS connectors.
Changes
oauth2strategy tooauth2_authentication_codeand addedoauth2_client_credentialsChecklist
CHANGELOG.mdfileCHANGELOG.mdfile is being appended toUnreleasedsection in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.Run Unsafe PR Checkslabel has been applied, and checks have passed, if this PR touches any external servicesTicket
Fixes #1004