estuary-cdk: refactor the placement of client creds in OAuth requests#3503
estuary-cdk: refactor the placement of client creds in OAuth requests#3503nicolaslazo merged 5 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors OAuth client credentials placement in the estuary-cdk to support flexible credential placement strategies (headers vs. form body) and simplifies the OAuth specification models.
- Introduces a builder pattern via
with_client_credentials_placement()method to override default credential placement - Merges
OAuth2RotatingTokenSpecintoOAuth2Specby addingadditionalTokenExchangeBodyfield with a default empty dict - Simplifies token exchange logic by using
grant_typeandclient_credentials_placementClassVars to eliminate redundant match/case logic
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| estuary-cdk/estuary_cdk/flow.py | Adds RequestDataPlacement type, adds grant_type and client_credentials_placement ClassVars to OAuth credential classes, adds with_client_credentials_placement() method to credential classes, merges OAuth2RotatingTokenSpec into OAuth2Spec, and refactors for_provider() methods to use cls parameter |
| estuary-cdk/estuary_cdk/http.py | Refactors _fetch_oauth2_token() to use credential ClassVars (grant_type, client_credentials_placement) instead of match/case on credential types, updates to use merged OAuth2Spec, and includes formatting improvements |
| estuary-cdk/estuary_cdk/capture/common.py | Removes OAuth2RotatingTokenSpec import |
| estuary-cdk/estuary_cdk/capture/base_capture_connector.py | Formatting and import organization improvements |
| source-zendesk-support-native/source_zendesk_support_native/models.py | Updates OAuth2RotatingTokenSpec to OAuth2Spec |
| source-outreach/source_outreach/models.py | Updates OAuth2RotatingTokenSpec to OAuth2Spec and removes redundant additionalTokenExchangeBody=None parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c08f6dd to
84f3e25
Compare
Alex-Bair
left a comment
There was a problem hiding this comment.
Had a few small comments.
Additionally whenever I make changes that affect multiple connectors, I make sure that the associated tests are passing in the CI checks. That helps put me a little at ease & makes it easier to claim that my CDK level changes didn't break individual connectors. It looks like the source-zendesk-support-native tests are failing, so we should figure out why they're failing & get them passing before merging this PR.
84f3e25 to
73a8389
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0f5e012 to
4fb85cc
Compare
Alex-Bair
left a comment
There was a problem hiding this comment.
Looking good! I had a few more comments, but this is getting really close to being ready to be merged.
4fb85cc to
382070d
Compare
A new `with_client_credentials_location` class method has been implemented to specify whether client ids and secrets will go into request headers or forms. To make it chainable with `for_provider`, the latter has been turned into a class method too -- a later commit will replace the redundant definitions with a single one. In the process of adapting `for_provider` to this new scheme it was also determined that it was preferrable to have dynamically created classes report their original OAuth flavour instead of a generic `_OAuthCredentials`. This change required spec updates for six different connectors.
382070d to
7ea8818
Compare
Alex-Bair
left a comment
There was a problem hiding this comment.
LGTM % a couple comments.
If you're alright with it, let's wait and merge this next week when more folks are around to support after Thanksgiving.
7ea8818 to
4b5042f
Compare
4b5042f to
1096227
Compare
1096227 to
b02e4a5
Compare
Description:
So far, each OAuth flavour in the CDK had a hardcoded place where OAuth client credentials were supposed to go -- either the headers, or the request body. However, in the process of developing the QuickBooks connector (#3468) we realised its rotating tokens system broke the assumed standard: client ids and secrets were to be concatenated and base64-encoded before being placed in the headers.
This PR adds a new class method to our OAuth models:
with_client_credentials_placement, which allows for a builder pattern-like interface to override what we'd used as the standard so far. This means no pre-existing connector code needed to be updated.Two smaller changes were effected too
OAuth2RotatingTokenSpecgot merged intoOAuth2Specand now the latter supports theadditionalTokenExchangeBodyfieldheadersandformdictionaries has been simplifiedWorkflow steps:
(How does one use this feature, and how has it changed)
Documentation links affected:
(list any documentation links that you created, or existing ones that you've identified as needing updates, along with a brief description)
Notes for reviewers:
Changes have been tested on the following connectors: