Feature/add source wizard#1423
Open
himanshudube97 wants to merge 8 commits into
Open
Conversation
…byte delegation
Move the Google Sheets OAuth token exchange out of Airbyte into Dalgo. Instead of
registering client id/secret with Airbyte instance-wide and letting it exchange the
code, Dalgo now builds the Google consent URL itself, receives Google's redirect on
its own backend callback, exchanges the code for a refresh_token server-side, and
stashes it in Redis under a single-use opaque `ref`.
- consent: POST /sources/oauth/consent -> {authUrl} (builds Google URL; state nonce)
- callback: GET /sources/oauth/callback (PUBLIC, state-authenticated) -> exchanges
code, stashes refresh_token under `ref`, 302s to the frontend with only the ref
- create: POST /sources/oauth/create -> redeems `ref`, injects credentials
(client_id/secret from env + refresh_token), creates/updates the source
The auth code, client_secret, and refresh_token never reach the browser (only `ref`).
Scope is spreadsheets.readonly (least privilege). Removes the Airbyte source_oauths/*
wrappers, the SourceOAuthComplete schema/endpoint, and the instance-wide params
management command. 11 OAuth tests + full airbyte module (50) green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WalkthroughAdds Google OAuth support for Airbyte sources, including connector configuration, consent and callback endpoints, Redis-backed state and refresh-token references, credential injection during source creation, and comprehensive flow tests. ChangesGoogle OAuth Airbyte source flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant AirbyteAPI
participant OAuthService
participant Redis
participant Google
participant Airbyte
Browser->>AirbyteAPI: Start Google OAuth consent
AirbyteAPI->>OAuthService: Build consent URL and state
OAuthService->>Redis: Store state
Browser->>Google: Authorize connector
Google->>AirbyteAPI: Return callback code and state
AirbyteAPI->>OAuthService: Exchange code and complete OAuth
OAuthService->>Google: Exchange authorization code
OAuthService->>Redis: Store single-use ref
OAuthService-->>Browser: Redirect with ref
Browser->>AirbyteAPI: Create or update source with ref
AirbyteAPI->>OAuthService: Redeem ref
AirbyteAPI->>Airbyte: Create or update OAuth source
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1423 +/- ##
=======================================
Coverage ? 62.17%
=======================================
Files ? 154
Lines ? 17968
Branches ? 0
=======================================
Hits ? 11171
Misses ? 6797
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…eature/add-source-wizard
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ddpui/core/oauth/google_oauth_service.py`:
- Around line 144-159: Update exchange_google_oauth_code to catch requests.post
timeouts and response.json parsing failures, convert both into HttpError using
the existing oauth_failed callback flow, and preserve the current handling for
non-200 responses. Add regression tests covering the timeout and invalid-JSON
paths.
In `@ddpui/ddpairbyte/airbytehelpers.py`:
- Line 93: Update the flow around google_oauth_service.redeem_ref to atomically
claim the OAuth ref without permanently deleting it, then finalize its
consumption only after environment lookup and the Airbyte persistence succeeds.
On any configuration or Airbyte failure, release or restore the claim so the
refresh token remains redeemable.
- Around line 100-103: Before the payload.sourceId branch invokes update_source,
validate that the source belongs to the caller’s workspace and matches the
requested source definition, using the workspace context and existing
ownership-validation flow. Reject cross-workspace or mismatched sources before
redeeming the ref or modifying the source, and add a test covering rejection of
another workspace’s source ID.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7fb51dd-0ca3-40aa-909d-17ddc955458a
📒 Files selected for processing (8)
.env.templateddpui/api/airbyte_api.pyddpui/core/oauth/__init__.pyddpui/core/oauth/google_oauth_provider.pyddpui/core/oauth/google_oauth_service.pyddpui/ddpairbyte/airbytehelpers.pyddpui/ddpairbyte/schema.pyddpui/tests/api_tests/test_airbyte_api.py
| it into `payload.config`, and saves the source. The refresh_token never travels through | ||
| the browser. With `payload.sourceId` set it updates that source (re-authenticate); | ||
| otherwise it creates a new one.""" | ||
| refresh_token = google_oauth_service.redeem_ref(orguser, payload.ref, payload.sourceDefId) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not consume the OAuth ref before persistence succeeds.
redeem_ref() performs GETDEL before environment lookup and the Airbyte write. Any configuration or Airbyte failure permanently loses the refresh token and forces the user through consent again. Use an atomic claim/finalize flow, releasing or restoring the claim on failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ddpui/ddpairbyte/airbytehelpers.py` at line 93, Update the flow around
google_oauth_service.redeem_ref to atomically claim the OAuth ref without
permanently deleting it, then finalize its consumption only after environment
lookup and the Airbyte persistence succeeds. On any configuration or Airbyte
failure, release or restore the claim so the refresh token remains redeemable.
Comment on lines
+100
to
+103
| if payload.sourceId: | ||
| source = airbyte_service.update_source( | ||
| payload.sourceId, payload.name, config, payload.sourceDefId | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Verify that sourceId belongs to the caller's workspace before updating.
The client controls sourceId, while update_source() accepts no workspace context. A user who obtains another workspace's source ID can overwrite that source's name, configuration, and credentials. Validate workspace ownership and source definition before redeeming the ref or invoking the update; add a cross-workspace rejection test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ddpui/ddpairbyte/airbytehelpers.py` around lines 100 - 103, Before the
payload.sourceId branch invokes update_source, validate that the source belongs
to the caller’s workspace and matches the requested source definition, using the
workspace context and existing ownership-validation flow. Reject cross-workspace
or mismatched sources before redeeming the ref or modifying the source, and add
a test covering rejection of another workspace’s source ID.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ddpui/core/oauth/google_oauth_service.py`:
- Around line 175-183: Validate that the value assigned to tokens in the Google
OAuth exchange is a mapping/object before checking for "refresh_token" or
calling list(tokens). For valid JSON values such as null or a list, log the
malformed response and raise the same intended OAuth failure HttpError instead
of allowing TypeError to escape; preserve the existing missing-refresh-token
handling for valid objects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ad62120-6392-4198-8c82-e74cfa41c61e
📒 Files selected for processing (2)
ddpui/core/oauth/google_oauth_service.pyddpui/tests/api_tests/test_airbyte_api.py
Comment on lines
+175
to
+183
| try: | ||
| tokens = response.json() | ||
| except ValueError as err: | ||
| # a 200 with a non-JSON body — treat as a failed exchange, not a 500 crash | ||
| logger.error("google token exchange returned non-JSON body: %s", response.text[:200]) | ||
| raise HttpError(500, "failed to complete the oauth flow") from err | ||
| if "refresh_token" not in tokens: | ||
| logger.error("google token exchange returned no refresh_token: keys=%s", list(tokens)) | ||
| raise HttpError(400, "oauth did not return a refresh token") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate that the token response is a JSON object.
A 200 response containing valid JSON such as null or a list causes an uncaught TypeError at Line 181 instead of the intended oauth_failed redirect.
Proposed fix
try:
tokens = response.json()
except ValueError as err:
logger.error("google token exchange returned non-JSON body: %s", response.text[:200])
raise HttpError(500, "failed to complete the oauth flow") from err
+ if not isinstance(tokens, dict):
+ logger.error(
+ "google token exchange returned unexpected JSON type: %s",
+ type(tokens).__name__,
+ )
+ raise HttpError(500, "failed to complete the oauth flow")
if "refresh_token" not in tokens:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try: | |
| tokens = response.json() | |
| except ValueError as err: | |
| # a 200 with a non-JSON body — treat as a failed exchange, not a 500 crash | |
| logger.error("google token exchange returned non-JSON body: %s", response.text[:200]) | |
| raise HttpError(500, "failed to complete the oauth flow") from err | |
| if "refresh_token" not in tokens: | |
| logger.error("google token exchange returned no refresh_token: keys=%s", list(tokens)) | |
| raise HttpError(400, "oauth did not return a refresh token") | |
| try: | |
| tokens = response.json() | |
| except ValueError as err: | |
| logger.error("google token exchange returned non-JSON body: %s", response.text[:200]) | |
| raise HttpError(500, "failed to complete the oauth flow") from err | |
| if not isinstance(tokens, dict): | |
| logger.error( | |
| "google token exchange returned unexpected JSON type: %s", | |
| type(tokens).__name__, | |
| ) | |
| raise HttpError(500, "failed to complete the oauth flow") | |
| if "refresh_token" not in tokens: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ddpui/core/oauth/google_oauth_service.py` around lines 175 - 183, Validate
that the value assigned to tokens in the Google OAuth exchange is a
mapping/object before checking for "refresh_token" or calling list(tokens). For
valid JSON values such as null or a list, log the malformed response and raise
the same intended OAuth failure HttpError instead of allowing TypeError to
escape; preserve the existing missing-refresh-token handling for valid objects.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Architecture — Google OAuth for Airbyte sources
Dalgo-driven flow (Variant A: Dalgo performs the token exchange, not Airbyte). One Google Cloud OAuth app is shared across all Google connectors; only scopes + the credentials block shape vary per connector.
Layers
Request flow (3 backend hits)
Security
client_secretandrefresh_tokennever reach the browser — only the opaquerefdoes.stateandrefare single-use nonces, consumed via an atomic get+delete (MULTI/EXEC pipeline — Redis <6.2 compatible), closing the replay window.Extensibility
Add a Google source (Drive, Analytics, Ads) = one registry entry in
google_oauth_provider.py(def-id → scopes + a credentials builder). No changes to the service, API, or callback.Summary by CodeRabbit
New Features
Tests