-
Notifications
You must be signed in to change notification settings - Fork 14
fix: add polling to make OAuth flows more robust to popup communication failure #830
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
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 |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| "@knocklabs/react-core": patch | ||
| "@knocklabs/react": patch | ||
| --- | ||
|
|
||
| fix: poll to check if OAuth connection succeeded in case popup communication fails during Slack and Microsoft Teams auth flows |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,4 @@ | ||
| export { default as useAuthenticatedKnockClient } from "./useAuthenticatedKnockClient"; | ||
| export { default as useStableOptions } from "./useStableOptions"; | ||
| export { useAuthPostMessageListener } from "./useAuthPostMessageListener"; | ||
| export { useAuthPolling } from "./useAuthPolling"; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| import { useEffect } from "react"; | ||
|
|
||
| import { AuthCheckResult, ConnectionStatus } from "../types"; | ||
|
|
||
| export interface UseAuthPollingOptions { | ||
| popupWindowRef: React.MutableRefObject<Window | null>; | ||
| setConnectionStatus: (status: ConnectionStatus) => void; | ||
| authCheckFn: () => Promise<AuthCheckResult>; | ||
| onAuthenticationComplete?: (authenticationResp: string) => void; | ||
| } | ||
|
|
||
| /** | ||
| * Hook that polls an authentication check endpoint until success or timeout. | ||
| * | ||
| * Polls every 2 seconds for up to 3 minutes (90 iterations). Has three stop conditions: | ||
| * 1. Max timeout reached → sets error status | ||
| * 2. Popup closed + 10s grace period → stops silently | ||
| * 3. Success detected via authCheckFn → updates status and closes popup | ||
| * | ||
| * @param options - Configuration options for the polling mechanism | ||
| * | ||
| * @example | ||
| * ```tsx | ||
| * useAuthPolling({ | ||
| * popupWindowRef, | ||
| * setConnectionStatus, | ||
| * onAuthenticationComplete, | ||
| * authCheckFn: useCallback(async () => { | ||
| * return knock.slack.authCheck({ | ||
| * tenant: tenantId, | ||
| * knockChannelId: knockSlackChannelId, | ||
| * }); | ||
| * }, [knock.slack, tenantId, knockSlackChannelId]), | ||
| * }); | ||
| * ``` | ||
| */ | ||
| export function useAuthPolling(options: UseAuthPollingOptions): void { | ||
| const { | ||
| popupWindowRef, | ||
| setConnectionStatus, | ||
| onAuthenticationComplete, | ||
| authCheckFn, | ||
| } = options; | ||
|
|
||
| useEffect( | ||
| () => { | ||
| let pollCount = 0; | ||
| const maxPolls = 90; | ||
| let popupClosedAt: number | null = null; | ||
| let isActive = true; | ||
|
|
||
| const pollInterval = setInterval(async () => { | ||
| if (!isActive) { | ||
| clearInterval(pollInterval); | ||
| return; | ||
| } | ||
|
|
||
| const popupWindow = popupWindowRef.current; | ||
| if (!popupWindow) { | ||
| return; | ||
| } | ||
|
|
||
| pollCount++; | ||
|
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. Timeout counter never increments when popup ref is nullMedium Severity The |
||
|
|
||
| const isPopupClosed = popupWindow.closed; | ||
| if (isPopupClosed && !popupClosedAt) { | ||
| popupClosedAt = Date.now(); | ||
| } | ||
|
|
||
| // Stop condition 1: Max timeout reached | ||
| if (pollCount >= maxPolls) { | ||
| clearInterval(pollInterval); | ||
| setConnectionStatus("error"); | ||
| return; | ||
| } | ||
|
|
||
| // Stop condition 2: Popup closed + grace period expired | ||
| if (popupClosedAt && Date.now() - popupClosedAt > 10000) { | ||
| clearInterval(pollInterval); | ||
| popupWindowRef.current = null; | ||
| return; | ||
| } | ||
connorlindsey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| try { | ||
| const authRes = await authCheckFn(); | ||
|
|
||
| // Stop condition 3: Success detected | ||
| if (authRes.connection?.ok) { | ||
| clearInterval(pollInterval); | ||
| setConnectionStatus("connected"); | ||
| if (onAuthenticationComplete) { | ||
| onAuthenticationComplete("authComplete"); | ||
| } | ||
| if (popupWindow && !popupWindow.closed) { | ||
| popupWindow.close(); | ||
| } | ||
| popupWindowRef.current = null; | ||
| } | ||
connorlindsey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } catch (_error) { | ||
| // Continue polling on error | ||
| } | ||
| }, 2000); | ||
|
|
||
| return () => { | ||
| isActive = false; | ||
| clearInterval(pollInterval); | ||
| }; | ||
| }, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [ | ||
| // Empty deps - run once on mount and keep polling | ||
| // This is intentionally simple/brute force | ||
| ], | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| import { useEffect } from "react"; | ||
|
|
||
| import { ConnectionStatus } from "../types"; | ||
|
|
||
| export interface UseAuthPostMessageListenerOptions { | ||
| knockHost: string; | ||
| popupWindowRef: React.MutableRefObject<Window | null>; | ||
| setConnectionStatus: (status: ConnectionStatus) => void; | ||
| onAuthenticationComplete?: (authenticationResp: string) => void; | ||
| } | ||
|
|
||
| /** | ||
| * Hook that listens for postMessage events from OAuth popup windows. | ||
| * | ||
| * Handles "authComplete" and "authFailed" messages sent from the OAuth flow popup, | ||
| * validates the message origin, updates connection status, and closes the popup. | ||
| * | ||
| * @param options - Configuration options for the postMessage listener | ||
| * | ||
| * @example | ||
| * ```tsx | ||
| * useAuthPostMessageListener({ | ||
| * knockHost: knock.host, | ||
| * popupWindowRef, | ||
| * setConnectionStatus, | ||
| * onAuthenticationComplete, | ||
| * }); | ||
| * ``` | ||
| */ | ||
| export function useAuthPostMessageListener( | ||
| options: UseAuthPostMessageListenerOptions, | ||
| ): void { | ||
| const { | ||
| knockHost, | ||
| popupWindowRef, | ||
| setConnectionStatus, | ||
| onAuthenticationComplete, | ||
| } = options; | ||
|
|
||
| useEffect(() => { | ||
| const receiveMessage = (event: MessageEvent) => { | ||
| // Validate message origin for security | ||
| if (event.origin !== knockHost) { | ||
| return; | ||
| } | ||
|
|
||
| if (event.data === "authComplete") { | ||
| setConnectionStatus("connected"); | ||
| onAuthenticationComplete?.(event.data); | ||
| // Clear popup ref so polling stops and doesn't trigger callback again | ||
| if (popupWindowRef.current && !popupWindowRef.current.closed) { | ||
| popupWindowRef.current.close(); | ||
| } | ||
| popupWindowRef.current = null; | ||
| } else if (event.data === "authFailed") { | ||
| setConnectionStatus("error"); | ||
| onAuthenticationComplete?.(event.data); | ||
| popupWindowRef.current = null; | ||
| } | ||
connorlindsey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }; | ||
|
|
||
| window.addEventListener("message", receiveMessage, false); | ||
| return () => window.removeEventListener("message", receiveMessage); | ||
| }, [ | ||
| knockHost, | ||
| onAuthenticationComplete, | ||
| setConnectionStatus, | ||
| popupWindowRef, | ||
| ]); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| /** | ||
| * Represents the connection status for OAuth-based integrations (Slack, MS Teams, etc.) | ||
| */ | ||
| export type ConnectionStatus = | ||
| | "connecting" | ||
| | "connected" | ||
| | "disconnected" | ||
| | "error" | ||
| | "disconnecting"; | ||
|
|
||
| /** | ||
| * Result returned by authentication check API calls | ||
| */ | ||
| export interface AuthCheckResult { | ||
| connection?: { | ||
| ok?: boolean; | ||
| error?: string; | ||
| }; | ||
| code?: string; | ||
| response?: { | ||
| data?: { | ||
| message?: string; | ||
| }; | ||
| }; | ||
| } |
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.
🍩