Skip to content

Commit cd3acd1

Browse files
tara-elasticelasticmachine
tara-elastic
and
elasticmachine
authored
[EARS] Fix OAuth "Cancel authorization" to delete pending server-side state (#270224)
## Summary When a user clicked "Cancel authorization" during an OAuth connector flow, the UI reset correctly but the server-side `oauth_state` saved object was left behind. If the OAuth provider tab was still open and the user completed authorization there, the callback would succeed and the connector would silently become connected — even though the user had already cancelled. The orphan state would otherwise be reaped by the 10-minute TTL and the periodic cleanup task, but the window was real. This PR makes cancel actually cancel by deleting the pending state server-side: - Adds `POST /internal/actions/connector/{connectorId}/_oauth_cancel` — looks up the `oauth_state` saved object by the `state` value, verifies ownership via `profile_uid`, and deletes it. Returns 204 (idempotent: deleted and not-found both succeed), 403 if the state was created by a different user. - Adds `OAuthStateClient.deleteByState(stateParam, profileUid)` with explicit ownership handling, including the case where `createdBy` is absent (treated as forbidden). - Wires `cancelConnect()` in `useConnectorOAuthConnect` to call the new endpoint as a fire-and-forget: UI state resets synchronously; server-side cancel is best-effort cleanup. - Adds `state` to `StartOAuthFlowResponse` so the hook can pass the exact state string to the cancel endpoint. Closes elastic/search-team#14488 ### Proof https://github.com/user-attachments/assets/2dad9f8d-0911-4e6e-b4d8-9c811f0474ff ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [x] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. ### Identify risks - **Race condition (low):** A user could complete the OAuth flow in the provider tab between clicking Cancel and the server processing the cancel request. This is acceptable — cancel is best-effort and the existing 10-minute TTL provides a backstop. The UI already shows "disconnected" by the time any race could matter. - **Ownership check on legacy states:** `oauth_state` objects without a `createdBy` field (e.g. created before this change) cannot be cancelled and return 403. This is intentional — without ownership data we cannot verify the requester's right to cancel. - **No breaking changes:** The new endpoint uses `access: 'internal'`. The `state` field added to `StartOAuthFlowResponse` is additive and backward compatible. ### Release note Fixes a bug where cancelling an in-progress OAuth connector authorization flow left the pending server-side state intact, allowing the authorization to complete silently if the provider tab remained open. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
1 parent b5d7f60 commit cd3acd1

12 files changed

Lines changed: 714 additions & 14 deletions

File tree

‎x-pack/platform/packages/shared/response-ops/oauth-hooks/hooks/use_connector_oauth_connect.test.ts‎

Lines changed: 195 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -182,17 +182,189 @@ describe('useConnectorOAuthConnect', () => {
182182

183183
const onMutationSuccess = capturedMutationOptions.onSuccess as (data: {
184184
authorizationUrl: string;
185+
state: string;
185186
}) => void;
186187

187188
act(() => {
188-
onMutationSuccess({ authorizationUrl: 'https://oauth.provider/auth' });
189+
onMutationSuccess({ authorizationUrl: 'https://oauth.provider/auth', state: 'abc-state' });
189190
});
190191

191192
expect(window.open).toHaveBeenCalledWith('https://oauth.provider/auth', '_blank', 'noopener');
192193
expect(result.current.isAwaitingCallback).toBe(true);
193194
});
194195
});
195196

197+
describe('cancelConnect', () => {
198+
const triggerMutationSuccess = (state = 'pending-state') => {
199+
const onMutationSuccess = capturedMutationOptions.onSuccess as (data: {
200+
authorizationUrl: string;
201+
state: string;
202+
}) => void;
203+
act(() => {
204+
onMutationSuccess({ authorizationUrl: 'https://oauth.provider/auth', state });
205+
});
206+
};
207+
208+
it('resets isAwaitingCallback synchronously', () => {
209+
const { result } = renderHook(() =>
210+
useConnectorOAuthConnect({
211+
connectorId: 'conn-1',
212+
redirectMode: OAuthRedirectMode.NewTab,
213+
})
214+
);
215+
216+
triggerMutationSuccess();
217+
expect(result.current.isAwaitingCallback).toBe(true);
218+
219+
act(() => result.current.cancelConnect());
220+
221+
expect(result.current.isAwaitingCallback).toBe(false);
222+
});
223+
224+
it('posts to the cancel endpoint when a pending state exists', () => {
225+
mockHttpPost.mockResolvedValue(undefined);
226+
227+
const { result } = renderHook(() =>
228+
useConnectorOAuthConnect({
229+
connectorId: 'conn-1',
230+
redirectMode: OAuthRedirectMode.NewTab,
231+
})
232+
);
233+
234+
triggerMutationSuccess('my-oauth-state');
235+
act(() => result.current.cancelConnect());
236+
237+
expect(mockHttpPost).toHaveBeenCalledWith(
238+
'/internal/actions/connector/conn-1/_oauth_cancel',
239+
{ body: JSON.stringify({ state: 'my-oauth-state' }) }
240+
);
241+
});
242+
243+
it('does not post to the cancel endpoint when no state has been captured', () => {
244+
mockHttpPost.mockClear();
245+
246+
const { result } = renderHook(() =>
247+
useConnectorOAuthConnect({
248+
connectorId: 'conn-1',
249+
redirectMode: OAuthRedirectMode.NewTab,
250+
})
251+
);
252+
253+
// Cancel before any flow has started
254+
act(() => result.current.cancelConnect());
255+
256+
expect(mockHttpPost).not.toHaveBeenCalled();
257+
});
258+
259+
it('encodes connectorId in the cancel URL', () => {
260+
mockHttpPost.mockResolvedValue(undefined);
261+
262+
const { result } = renderHook(() =>
263+
useConnectorOAuthConnect({
264+
connectorId: 'id/with special&chars',
265+
redirectMode: OAuthRedirectMode.NewTab,
266+
})
267+
);
268+
269+
triggerMutationSuccess('some-state');
270+
act(() => result.current.cancelConnect());
271+
272+
expect(mockHttpPost).toHaveBeenCalledWith(
273+
expect.stringContaining(encodeURIComponent('id/with special&chars')),
274+
expect.anything()
275+
);
276+
});
277+
278+
it('clears the pending state so a second cancel does not re-post', () => {
279+
mockHttpPost.mockResolvedValue(undefined);
280+
281+
const { result } = renderHook(() =>
282+
useConnectorOAuthConnect({
283+
connectorId: 'conn-1',
284+
redirectMode: OAuthRedirectMode.NewTab,
285+
})
286+
);
287+
288+
triggerMutationSuccess('my-oauth-state');
289+
act(() => result.current.cancelConnect());
290+
mockHttpPost.mockClear();
291+
292+
act(() => result.current.cancelConnect());
293+
294+
expect(mockHttpPost).not.toHaveBeenCalled();
295+
});
296+
297+
it('clears the pending state after a successful broadcast', () => {
298+
mockHttpPost.mockResolvedValue(undefined);
299+
300+
const { result } = renderHook(() =>
301+
useConnectorOAuthConnect({
302+
connectorId: 'conn-1',
303+
redirectMode: OAuthRedirectMode.NewTab,
304+
onSuccess: jest.fn(),
305+
})
306+
);
307+
308+
triggerMutationSuccess('my-state');
309+
310+
const channel = MockBroadcastChannel.instances.find(
311+
(c) => c.name === OAUTH_BROADCAST_CHANNEL_NAME
312+
)!;
313+
act(() => {
314+
channel.onmessage!({
315+
data: { connectorId: 'conn-1', status: OAuthAuthorizationStatus.Success },
316+
} as MessageEvent);
317+
});
318+
319+
mockHttpPost.mockClear();
320+
act(() => result.current.cancelConnect());
321+
322+
expect(mockHttpPost).not.toHaveBeenCalled();
323+
});
324+
325+
it('clears the pending state after the timeout fires', () => {
326+
mockHttpPost.mockResolvedValue(undefined);
327+
328+
const { result } = renderHook(() =>
329+
useConnectorOAuthConnect({
330+
connectorId: 'conn-1',
331+
redirectMode: OAuthRedirectMode.NewTab,
332+
timeout: 5000,
333+
onError: jest.fn(),
334+
})
335+
);
336+
337+
triggerMutationSuccess('my-state');
338+
act(() => jest.advanceTimersByTime(5000));
339+
mockHttpPost.mockClear();
340+
341+
act(() => result.current.cancelConnect());
342+
343+
expect(mockHttpPost).not.toHaveBeenCalled();
344+
});
345+
346+
it('clears the pending state when connect is called for a new flow', () => {
347+
mockHttpPost.mockResolvedValue(undefined);
348+
349+
const { result } = renderHook(() =>
350+
useConnectorOAuthConnect({
351+
connectorId: 'conn-1',
352+
redirectMode: OAuthRedirectMode.NewTab,
353+
})
354+
);
355+
356+
triggerMutationSuccess('old-state');
357+
358+
// Start a new flow
359+
act(() => result.current.connect());
360+
mockHttpPost.mockClear();
361+
362+
act(() => result.current.cancelConnect());
363+
364+
expect(mockHttpPost).not.toHaveBeenCalled();
365+
});
366+
});
367+
196368
describe('NewTab mode - BroadcastChannel', () => {
197369
it('invokes onSuccess when receiving a success message for the matching connectorId', () => {
198370
const onSuccess = jest.fn();
@@ -206,8 +378,11 @@ describe('useConnectorOAuthConnect', () => {
206378

207379
const onMutationSuccess = capturedMutationOptions.onSuccess as (data: {
208380
authorizationUrl: string;
381+
state: string;
209382
}) => void;
210-
act(() => onMutationSuccess({ authorizationUrl: 'https://oauth.provider/auth' }));
383+
act(() =>
384+
onMutationSuccess({ authorizationUrl: 'https://oauth.provider/auth', state: 'test-state' })
385+
);
211386

212387
const channel = MockBroadcastChannel.instances.find(
213388
(c) => c.name === OAUTH_BROADCAST_CHANNEL_NAME
@@ -236,8 +411,11 @@ describe('useConnectorOAuthConnect', () => {
236411

237412
const onMutationSuccess = capturedMutationOptions.onSuccess as (data: {
238413
authorizationUrl: string;
414+
state: string;
239415
}) => void;
240-
act(() => onMutationSuccess({ authorizationUrl: 'https://oauth.provider/auth' }));
416+
act(() =>
417+
onMutationSuccess({ authorizationUrl: 'https://oauth.provider/auth', state: 'test-state' })
418+
);
241419

242420
const channel = MockBroadcastChannel.instances.find(
243421
(c) => c.name === OAUTH_BROADCAST_CHANNEL_NAME
@@ -265,8 +443,11 @@ describe('useConnectorOAuthConnect', () => {
265443

266444
const onMutationSuccess = capturedMutationOptions.onSuccess as (data: {
267445
authorizationUrl: string;
446+
state: string;
268447
}) => void;
269-
act(() => onMutationSuccess({ authorizationUrl: 'https://oauth.provider/auth' }));
448+
act(() =>
449+
onMutationSuccess({ authorizationUrl: 'https://oauth.provider/auth', state: 'test-state' })
450+
);
270451

271452
const channel = MockBroadcastChannel.instances.find(
272453
(c) => c.name === OAUTH_BROADCAST_CHANNEL_NAME
@@ -300,8 +481,11 @@ describe('useConnectorOAuthConnect', () => {
300481

301482
const onMutationSuccess = capturedMutationOptions.onSuccess as (data: {
302483
authorizationUrl: string;
484+
state: string;
303485
}) => void;
304-
act(() => onMutationSuccess({ authorizationUrl: 'https://oauth.provider/auth' }));
486+
act(() =>
487+
onMutationSuccess({ authorizationUrl: 'https://oauth.provider/auth', state: 'test-state' })
488+
);
305489

306490
act(() => jest.advanceTimersByTime(5000));
307491

@@ -325,8 +509,11 @@ describe('useConnectorOAuthConnect', () => {
325509

326510
const onMutationSuccess = capturedMutationOptions.onSuccess as (data: {
327511
authorizationUrl: string;
512+
state: string;
328513
}) => void;
329-
act(() => onMutationSuccess({ authorizationUrl: 'https://oauth.provider/auth' }));
514+
act(() =>
515+
onMutationSuccess({ authorizationUrl: 'https://oauth.provider/auth', state: 'test-state' })
516+
);
330517

331518
const channel = MockBroadcastChannel.instances.find(
332519
(c) => c.name === OAUTH_BROADCAST_CHANNEL_NAME
@@ -356,10 +543,11 @@ describe('useConnectorOAuthConnect', () => {
356543

357544
const onMutationSuccess = capturedMutationOptions.onSuccess as (data: {
358545
authorizationUrl: string;
546+
state: string;
359547
}) => void;
360548

361549
act(() => {
362-
onMutationSuccess({ authorizationUrl: 'https://oauth.provider/auth' });
550+
onMutationSuccess({ authorizationUrl: 'https://oauth.provider/auth', state: 'test-state' });
363551
});
364552

365553
expect(window.location.assign).toHaveBeenCalledWith('https://oauth.provider/auth');

‎x-pack/platform/packages/shared/response-ops/oauth-hooks/hooks/use_connector_oauth_connect.ts‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@ export const useConnectorOAuthConnect = ({
8989
onErrorRef.current = onError;
9090

9191
const [isAwaitingCallback, setIsAwaitingCallback] = useState(false);
92+
const pendingStateRef = useRef<string | undefined>();
9293

9394
const handleAuthRedirect = useCallback(
9495
(authorizationUrl: string) => {
@@ -110,13 +111,14 @@ export const useConnectorOAuthConnect = ({
110111
StartOAuthFlowRequestBody
111112
>({
112113
mutationFn: (request) =>
113-
http.post<{ authorizationUrl: string }>(
114+
http.post<StartOAuthFlowResponse>(
114115
`${INTERNAL_BASE_ACTION_API_PATH}/connector/${encodeURIComponent(
115116
connectorId
116117
)}/_start_oauth_flow`,
117118
{ body: JSON.stringify({ returnUrl: request.returnUrl }) }
118119
),
119-
onSuccess: ({ authorizationUrl }) => {
120+
onSuccess: ({ authorizationUrl, state }) => {
121+
pendingStateRef.current = state;
120122
setIsAwaitingCallback(true);
121123
handleAuthRedirect(authorizationUrl);
122124
},
@@ -126,6 +128,7 @@ export const useConnectorOAuthConnect = ({
126128
});
127129

128130
const connect = useCallback(() => {
131+
pendingStateRef.current = undefined;
129132
setIsAwaitingCallback(false);
130133
let resolvedReturnUrl: string | undefined;
131134
if (returnUrl) {
@@ -139,8 +142,21 @@ export const useConnectorOAuthConnect = ({
139142
}, [startOAuthFlow, redirectMode, returnUrl]);
140143

141144
const cancelConnect = useCallback(() => {
145+
const state = pendingStateRef.current;
146+
pendingStateRef.current = undefined;
142147
setIsAwaitingCallback(false);
143-
}, []);
148+
if (state) {
149+
// Fire-and-forget: best-effort server cancel; UI state is already reset above.
150+
http
151+
.post(
152+
`${INTERNAL_BASE_ACTION_API_PATH}/connector/${encodeURIComponent(
153+
connectorId
154+
)}/_oauth_cancel`,
155+
{ body: JSON.stringify({ state }) }
156+
)
157+
.catch(() => {});
158+
}
159+
}, [http, connectorId]);
144160

145161
// Handle OAuth callback timeout
146162
useEffect(() => {
@@ -149,6 +165,7 @@ export const useConnectorOAuthConnect = ({
149165
}
150166

151167
const callbackTimeout = setTimeout(() => {
168+
pendingStateRef.current = undefined;
152169
setIsAwaitingCallback(false);
153170
onErrorRef.current?.(
154171
new Error(
@@ -173,6 +190,7 @@ export const useConnectorOAuthConnect = ({
173190
if (event.data.connectorId !== connectorId) {
174191
return;
175192
}
193+
pendingStateRef.current = undefined;
176194
if (event.data.status === OAuthAuthorizationStatus.Success) {
177195
onSuccessRef.current?.();
178196
} else {

‎x-pack/platform/plugins/shared/actions/common/index.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,8 @@ export type {
9696
StartOAuthFlowPathParams,
9797
StartOAuthFlowResponse,
9898
DisconnectOAuthPathParams,
99+
CancelOAuthPathParams,
100+
CancelOAuthBody,
99101
} from './routes/connector/apis/oauth';
100102
export {
101103
OAuthAuthorizationStatus,

‎x-pack/platform/plugins/shared/actions/common/routes/connector/apis/oauth/index.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,15 @@ export {
99
startOAuthFlowRequestBodySchema,
1010
startOAuthFlowPathParamsSchema,
1111
disconnectOAuthPathParamsSchema,
12+
cancelOAuthPathParamsSchema,
13+
cancelOAuthBodySchema,
1214
} from './schemas/latest';
1315

1416
export type {
1517
StartOAuthFlowRequestBody,
1618
StartOAuthFlowPathParams,
1719
StartOAuthFlowResponse,
1820
DisconnectOAuthPathParams,
21+
CancelOAuthPathParams,
22+
CancelOAuthBody,
1923
} from './types/latest';

‎x-pack/platform/plugins/shared/actions/common/routes/connector/apis/oauth/schemas/latest.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,4 +9,6 @@ export {
99
startOAuthFlowRequestBodySchema,
1010
startOAuthFlowPathParamsSchema,
1111
disconnectOAuthPathParamsSchema,
12+
cancelOAuthPathParamsSchema,
13+
cancelOAuthBodySchema,
1214
} from './v1';

‎x-pack/platform/plugins/shared/actions/common/routes/connector/apis/oauth/schemas/v1.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,3 +18,11 @@ export const startOAuthFlowPathParamsSchema = schema.object({
1818
export const disconnectOAuthPathParamsSchema = schema.object({
1919
connectorId: schema.string(),
2020
});
21+
22+
export const cancelOAuthPathParamsSchema = schema.object({
23+
connectorId: schema.string(),
24+
});
25+
26+
export const cancelOAuthBodySchema = schema.object({
27+
state: schema.string(),
28+
});

0 commit comments

Comments
 (0)