UN-2776 [FIX] Fixed GDrive connector showing always authenticated in workflow settings#1508
Conversation
Summary by CodeRabbit
WalkthroughAdds per-connector OAuth status and cache scoping on the frontend, passes an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant ConfigureDs
participant OAuthDs
participant GoogleBtn as GoogleOAuthButton
participant Browser as localStorage/sessionStorage
participant Google as OAuth Provider
participant Backend as Backend API
participant Cache as OAuth Cache
User->>ConfigureDs: Open connector (selectedSourceId)
ConfigureDs->>OAuthDs: Render with isExistingConnector, selectedSourceId
OAuthDs->>Browser: Read oauth-status-${id}
Note over OAuthDs,Browser: Subscribes to storage events for oauth-status-${id}
User->>GoogleBtn: Click OAuth
GoogleBtn->>OAuthDs: handleOAuth()
OAuthDs->>Browser: sessionStorage set oauth-current-connector = ${id}
OAuthDs->>Google: Open OAuth authorize window
Google-->>Backend: OAuth callback with code
Backend->>Cache: Store creds under oauth-cachekey-${id}
Backend-->>Browser: Update localStorage oauth-status-${id} = success
Note over Browser,OAuthDs: storage event updates status in component and parent
User->>ConfigureDs: Submit connector (create/update)
ConfigureDs->>Backend: POST/PUT with oauth_key
Backend->>Cache: Read creds (delete_key=False)
Backend-->>ConfigureDs: 2xx response
Backend->>Cache: Cleanup creds (delete_key=True)
ConfigureDs->>Browser: Remove oauth-cachekey-${id}, oauth-status-${id}
opt Unmount
ConfigureDs->>Browser: Cleanup oauth keys for ${id}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to Reviews > Disable Cache setting Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
kirtimanmishrazipstack
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/oauth-ds/oauth-status/OAuthStatus.jsx (1)
14-22: Use localStorage (not sessionStorage) for cross-window context; popup cannot read parent sessionStoragesessionStorage is scoped per-tab/window and isn’t available to the OAuth popup. As a result, OAuthStatus will often find null currentConnector and never persist the status. Switch to localStorage for the "oauth-current-connector" handshake so the popup can read and clear it. Also fix the comment to avoid confusion.
- // Set status for the workflow-connector that initiated OAuth (stored in sessionStorage for callback) - const currentConnector = sessionStorage.getItem("oauth-current-connector"); + // Set status for the workflow-connector that initiated OAuth (stored in localStorage for cross-window callback) + const currentConnector = localStorage.getItem("oauth-current-connector"); if (currentConnector) { - // currentConnector now contains workflowId-connectorType-sourceId format + // currentConnector contains workflowId-connectorType-sourceId format const statusKey = `oauth-status-${currentConnector}`; localStorage.setItem(statusKey, status); - // Clear the session storage after use - sessionStorage.removeItem("oauth-current-connector"); + // Clear the local storage after use + localStorage.removeItem("oauth-current-connector"); }
🧹 Nitpick comments (2)
frontend/src/components/oauth-ds/oauth-ds/OAuthDs.jsx (2)
31-39: Filter storage events by key to avoid unnecessary readsListen only to oauthStatusKey changes; use the event payload instead of rereading localStorage.
- useEffect(() => { - const handleStorageChange = () => { - // Listen for changes to our specific workflow-connector combination only - const updatedOAuthStatus = localStorage.getItem(oauthStatusKey); + useEffect(() => { + const handleStorageChange = (e) => { + // Listen only for our specific workflow-connector status key + if (e.key !== oauthStatusKey) return; + const updatedOAuthStatus = e.newValue; if (updatedOAuthStatus) { setOAuthStatus(updatedOAuthStatus); setStatus(updatedOAuthStatus); } };
115-122: Constrain connectorType PropType to known valuesThis tightens the contract and prevents silent key drift.
selectedSourceId: PropTypes.string.isRequired, - workflowId: PropTypes.string.isRequired, - connectorType: PropTypes.string.isRequired, + workflowId: PropTypes.string.isRequired, + connectorType: PropTypes.oneOf(["source", "destination"]).isRequired,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (3)
frontend/src/components/input-output/configure-ds/ConfigureDs.jsx(4 hunks)frontend/src/components/oauth-ds/oauth-ds/OAuthDs.jsx(3 hunks)frontend/src/components/oauth-ds/oauth-status/OAuthStatus.jsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
frontend/src/components/oauth-ds/oauth-ds/OAuthDs.jsx (2)
22-25: Good scoping of OAuth keys to workflow + connector type + sourceIdThis should resolve cross-connector contamination in a workflow.
49-54: LGTM on initial status bootstrappingReading the workflow/connector-scoped status on mount matches the new key strategy.
frontend/src/components/input-output/configure-ds/ConfigureDs.jsx (3)
53-57: Per-workflow/per-connector keys: goodKey structure matches OAuthDs and should prevent cross-connector contamination.
361-371: Passing workflowId and connectorType down is correctThis aligns ConfigureDs with the new OAuthDs interface.
41-41: Confirm whether projectId should be aliased to workflowId
The workflow store only exposes projectId; ensure this aligns with your intended workflow scoping or introduce a dedicated workflowId to avoid state collisions.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/input-output/configure-ds/ConfigureDs.jsx (1)
193-199: Bug: oauth-key attached only to connector_metadata.Adapter OAuth tests will miss the key. Use the correct metadata bucket based on isConnector.
- if (oAuthProvider?.length > 0) { - body["connector_metadata"] = { - ...body["connector_metadata"], - ...{ "oauth-key": cacheKey }, - }; - } + if (oAuthProvider?.length > 0) { + const metaKey = isConnector ? "connector_metadata" : "adapter_metadata"; + body[metaKey] = { + ...body[metaKey], + "oauth-key": cacheKey, + }; + }
♻️ Duplicate comments (1)
frontend/src/components/input-output/configure-ds/ConfigureDs.jsx (1)
54-58: Key scope outside workflows: verify selectedSourceId is always present.If selectedSourceId is empty when adding from outside a workflow, localStorage keys become shared (e.g., oauth-cachekey-undefined). Provide a fallback scope.
- const isExistingConnector = Boolean(editItemId || hasOAuthCredentials); + const isExistingConnector = Boolean(editItemId || hasOAuthCredentials); + // Ensure per-connector key scoping even when selectedSourceId is absent + const keyScope = + selectedSourceId || + `${(type || 'unknown').toLowerCase()}-${oAuthProvider || 'oauth'}`; + const oauthCacheKey = `oauth-cachekey-${keyScope}`; + const oauthStatusKey = `oauth-status-${keyScope}`;Run to check call sites pass a non-empty selectedSourceId in non-workflow flows:
#!/bin/bash # Inspect ConfigureDs usages and props passed for selectedSourceId rg -nP -C2 '<ConfigureDs\b' --type=js --type=jsx --type=ts --type=tsx
🧹 Nitpick comments (5)
frontend/src/components/oauth-ds/google/GoogleOAuthButton.jsx (3)
8-8: Fix label typo: "Sign in" (with space) for consistency.Default should read "Sign in" per UX copy elsewhere and PR description.
-const GoogleOAuthButton = ({ handleOAuth, status, buttonText = "Signin" }) => { +const GoogleOAuthButton = ({ handleOAuth, status, buttonText = "Sign in" }) => {
10-16: Remove derived state; compute label inline.Avoiding useState/useEffect here simplifies and prevents stale text.
- const [text, setText] = useState(""); - useEffect(() => { - if (status === "success") { - setText("Authenticated"); - return; - } - setText(buttonText); - }, [status, buttonText]); + const text = status === "success" ? "Authenticated" : buttonText;Additionally, if desired, disable the button when authenticated:
- <GoogleLoginButton onClick={handleOAuth}> + <GoogleLoginButton onClick={handleOAuth} disabled={status === "success"}>
30-30: Optional: tighten PropTypes for status.If statuses are finite, consider oneOf([...]) to catch regressions.
-GoogleOAuthButton.propTypes = { - handleOAuth: PropTypes.func.isRequired, - status: PropTypes.string, - buttonText: PropTypes.string, -}; +GoogleOAuthButton.propTypes = { + handleOAuth: PropTypes.func.isRequired, + status: PropTypes.string, // e.g., one of: '', 'success', 'error', 'pending' + buttonText: PropTypes.string, +};frontend/src/components/input-output/configure-ds/ConfigureDs.jsx (1)
352-358: Avoid mutating session adapters array.Mutating store-derived arrays can cause subtle state bugs; create a new array.
- const adaptersList = sessionDetails?.adapters; - if (adaptersList && !adaptersList.includes(adapterType)) { - adaptersList.push(adapterType); - const adaptersListInSession = { adapters: adaptersList }; - updateSessionDetails(adaptersListInSession); - } + const current = Array.isArray(sessionDetails?.adapters) + ? sessionDetails.adapters + : []; + if (!current.includes(adapterType)) { + updateSessionDetails({ adapters: [...current, adapterType] }); + }backend/connector_v2/views.py (1)
89-89: Reduce log duplication.“Fetching oauth data …” (Line 83) and “Using OAuth cache key …” communicate similar info. Keep one (ideally the latter) or lower the first to debug.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (4)
backend/connector_v2/views.py(3 hunks)frontend/src/components/input-output/configure-ds/ConfigureDs.jsx(5 hunks)frontend/src/components/oauth-ds/google/GoogleOAuthButton.jsx(2 hunks)frontend/src/components/oauth-ds/oauth-ds/OAuthDs.jsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/oauth-ds/oauth-ds/OAuthDs.jsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
frontend/src/components/input-output/configure-ds/ConfigureDs.jsx (3)
7-9: LGTM: imports added for analytics and URL util.
336-339: LGTM: clearing OAuth keys after successful submit.Matches the PR objective; prevents leakage across connectors.
369-370: Confirm downstream usage of isExistingConnector for button label.Ensure OAuthDs maps isExistingConnector → buttonText ("Reauthenticate" vs "Sign in") and propagates it to GoogleOAuthButton.
backend/connector_v2/views.py (3)
136-138: LGTM: cleanup runs only after a successful update.Assuming
_cleanup_oauth_cacheis hardened as above, this won’t impact the happy path even if the cache is already cleared.
155-157: LGTM: cleanup runs only after a successful create.Matches the front-end flow; ensures single-use cache keys.
85-89: Use provider-agnostic error text in MissingParamExceptionApply this diff in backend/connector_v2/views.py (lines 85–89):
- if not oauth_key: - raise MissingParamException( - "OAuth authentication required. Please sign in with Google first." - ) + if not oauth_key: + raise MissingParamException( + "OAuth authentication required. Please complete OAuth sign-in first." + )Confirm that clients aren’t relying on a separate param field in the error payload; if they do, continue using the
param=kwarg (e.g.MissingParamException(param="oauth_key")) or adjust the exception’s signature accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/oauth-ds/oauth-ds/OAuthDs.jsx (1)
74-81: Guard missing cache_key before proceedingIf the API doesn’t return a valid
cache_key, the flow will open a broken URL and set bad state.-const cacheKey = response?.data?.cache_key; -const encodedCacheKey = encodeURIComponent(cacheKey); -setCacheKey(cacheKey); +const cacheKey = response?.data?.cache_key; +if (!cacheKey || typeof cacheKey !== "string") { + throw new Error("OAuth cache key missing from response"); +} +const encodedCacheKey = encodeURIComponent(cacheKey); +setCacheKey(cacheKey); // Persist cache key to localStorage localStorage.setItem(oauthCacheKey, cacheKey);
♻️ Duplicate comments (3)
backend/connector_v2/views.py (2)
85-89: Make error message provider-agnostic.Remove "Google" to keep copy generic and future-proof.
- if not oauth_key: - raise MissingParamException( - "OAuth authentication required. Please sign in with Google first." - ) + if not oauth_key: + raise MissingParamException( + "OAuth authentication required. Please sign in first." + )
89-95: Handle cache miss explicitly; avoid relying on None-check only.Wrap the cache fetch to surface a friendly, consistent error and keep a defensive None guard.
- logger.info(f"Using OAuth cache key for {connector_id}") - connector_metadata = ConnectorAuthHelper.get_oauth_creds_from_cache( - cache_key=oauth_key, - delete_key=False, # Don't delete yet - wait for successful operation - ) - if connector_metadata is None: - raise MissingParamException(param=ConnectorAuthKey.OAUTH_KEY) + logger.info(f"Using OAuth cache key for {connector_id}") + try: + connector_metadata = ConnectorAuthHelper.get_oauth_creds_from_cache( + cache_key=oauth_key, + delete_key=False, # Don't delete yet - wait for successful operation + ) + except CacheMissException: + raise CacheMissException("OAuth session expired. Please sign in again.") + if connector_metadata is None: # Defensive in case the helper returns None + raise CacheMissException("OAuth session expired. Please sign in again.")frontend/src/components/oauth-ds/oauth-ds/OAuthDs.jsx (1)
66-68: Use localStorage for cross-window OAuth context (popup cannot read sessionStorage)The OAuth popup cannot access
sessionStorageof the opener tab; uselocalStorageso the popup can read/clear the context.-// Store connector context in sessionStorage for OAuth callback (survives window.open) -sessionStorage.setItem("oauth-current-connector", selectedSourceId); +// Store connector context in localStorage so the popup can read it +localStorage.setItem("oauth-current-connector", selectedSourceId);Optional: include provider (and workflow/type if available) for stronger disambiguation:
-localStorage.setItem("oauth-current-connector", selectedSourceId); +localStorage.setItem("oauth-current-connector", `${oAuthProvider}-${selectedSourceId}`);
🧹 Nitpick comments (5)
frontend/src/components/oauth-ds/oauth-ds/OAuthDs.jsx (5)
21-24: Key names: confirm uniqueness; consider scoping by providerRight now keys are scoped by
selectedSourceIdonly. If IDs aren’t globally unique across workflows/types, collisions can still occur. Consider includingoAuthProvider(and, if available, workflow/connector type) in the key.Example change:
-const oauthCacheKey = `oauth-cachekey-${selectedSourceId}`; -const oauthStatusKey = `oauth-status-${selectedSourceId}`; +const oauthCacheKey = `oauth-cachekey-${oAuthProvider}-${selectedSourceId}`; +const oauthStatusKey = `oauth-status-${oAuthProvider}-${selectedSourceId}`;
25-26: Copy: align with PR wordingPR says: new connector should show “Sign in”. Suggest:
-const buttonText = isExistingConnector ? "Reauthenticate" : "Authenticate with Google"; +const buttonText = isExistingConnector ? "Reauthenticate" : "Sign in with Google";
51-56: Initialize status unconditionally to reflect cleared stateEnsure cleared/absent status resets the UI.
-const connectorStatus = localStorage.getItem(oauthStatusKey); -if (connectorStatus) { - setStatus(connectorStatus); - setOAuthStatus(connectorStatus); -} +const connectorStatus = localStorage.getItem(oauthStatusKey); +setStatus(connectorStatus); +setOAuthStatus(connectorStatus);
86-91: Optional: add noopener for the popupMitigates reverse tabnabbing. Use if the popup doesn’t rely on
window.opener.-window.open( +window.open( url, "_blank", - "toolbar=yes,scrollbars=yes,resizable=yes,top=200,left=500,width=500,height=600" + "toolbar=yes,scrollbars=yes,resizable=yes,top=200,left=500,width=500,height=600,noopener" );
112-118: Tighten PropTypes (required where used)
oAuthProvider,setCacheKey, andsetStatusappear mandatory for correct behavior.-OAuthDs.propTypes = { - oAuthProvider: PropTypes.string, - setCacheKey: PropTypes.func, - setStatus: PropTypes.func, - selectedSourceId: PropTypes.string.isRequired, - isExistingConnector: PropTypes.bool, -}; +OAuthDs.propTypes = { + oAuthProvider: PropTypes.string.isRequired, + setCacheKey: PropTypes.func.isRequired, + setStatus: PropTypes.func.isRequired, + selectedSourceId: PropTypes.string.isRequired, + isExistingConnector: PropTypes.bool, +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (3)
backend/connector_v2/views.py(3 hunks)frontend/src/components/oauth-ds/google/GoogleOAuthButton.jsx(2 hunks)frontend/src/components/oauth-ds/oauth-ds/OAuthDs.jsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/oauth-ds/google/GoogleOAuthButton.jsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
backend/connector_v2/views.py (3)
100-120: Good: resilient, idempotent OAuth cache cleanup.Catches cache-miss and swallows failures after logging; aligns with post-success cleanup semantics.
142-144: Good: cleanup after update.Ensures the cache key is cleared only after successful update.
161-163: Good: cleanup after create.Clears the cache post-success to prevent phantom “authenticated” states.
frontend/src/components/oauth-ds/oauth-ds/OAuthDs.jsx (2)
7-7: Good: centralized exception handlingImporting and using
useExceptionHandlerfor OAuth errors is a solid improvement.
100-104: GoogleOAuthButton wiring looks correctPassing
statusandbuttonTextis consistent and clear.
|
|
kirtimanmishrazipstack
deleted the
UN-2776-gdrive-always-authenticated-inside-workflow
branch
3 participants
What
Why
oauth-cachekeycookie to local storageoauth-cachekeybased on workflow-id and connector typeHow
Frontend change:
For new connector:
For existing connector:
For both connectors:
Backend change:
Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.