From 4f356b214c2e65c0a4f1985e23455e264c45461a Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Wed, 5 Aug 2026 18:43:30 -0300 Subject: [PATCH 01/23] PMM-15293 Add a token-minter seam to the SEP API client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `refreshAccessToken()` hardcoded `POST /oauth/refresh` as the only way to obtain a token. An embedded host that owns the session — PMM — has no refresh cookie, so every recovery attempt would 401 there. `setTokenMinter()` replaces just that call; the default is unchanged, so the standalone SPA behaves exactly as before. Everything downstream is minter-agnostic already: the single-flight coalescer, the axios 401 retry, and the `setOnRefreshed` notification. Two supporting changes: The 401 retry now skips `/oauth/session*` as well as `/oauth/refresh`. Minting is single-flighted, so routing a mint's own 401 back through the retry interceptor would hand it the very promise it is running inside — an await on itself that never settles. The unauthorized handler still fires for those endpoints: a rejected exchange means "not signed in" and the auth layer needs to hear it. The openapi-fetch transport gained the 401 retry the axios one already had; it previously only reported unauthorized, so typed hooks could not recover at all. `fetch` consumes a Request's body, so the middleware stashes a clone taken before dispatch and replays that. The replay goes through raw `fetch` so it cannot re-enter the middleware and loop. Signed-off-by: Ignacio Durand --- ui/packages/sep/api/README.md | 32 +++- ui/packages/sep/api/src/client.ts | 92 +++++++++-- ui/packages/sep/api/src/index.ts | 2 + ui/packages/sep/api/src/typed-client.ts | 59 ++++++- ui/packages/sep/api/tests/client.test.ts | 145 +++++++++++++++++ .../sep/api/tests/typed-client.test.ts | 154 +++++++++++++++++- 6 files changed, 462 insertions(+), 22 deletions(-) diff --git a/ui/packages/sep/api/README.md b/ui/packages/sep/api/README.md index bea045d3707..1dd111546b2 100644 --- a/ui/packages/sep/api/README.md +++ b/ui/packages/sep/api/README.md @@ -30,11 +30,18 @@ packages. Anything that talks to the backend should go through here. the `openapi-fetch` `{ data, error }` tuple to throw the same shape. - **Token accessor pattern** — `setTokenProvider()` and `setOnUnauthorized()` let the auth layer plug in without the API package depending on auth state. +- **Token minter seam** — `setTokenMinter()` replaces _how_ a fresh token is + obtained. It defaults to the cookie-backed `POST /oauth/refresh`; an embedded + host that owns the session registers its own. Everything downstream — the + single-flight in `refreshAccessToken()`, the 401 retry in both transports, + the `setOnRefreshed()` notification — is minter-agnostic. - **Hooks** — `usePluginSchema`, `usePluginTasks`, `usePluginTask`, `useCreatePluginTask` (generic, predate codegen) and `useCurrentUser` (sample of the typed-hook pattern). -- **Auth functions** — `postLogin`, `postRefresh`, `fetchCurrentUser`. - Thin request wrappers consumed by the `AuthProvider` in `@sep/shell`. +- **Auth functions** — `postLogin`, `postRefresh`, `postSession`, + `postSessionExchange`, `postLogout`, `fetchCurrentUser`. Thin request wrappers + consumed by the `AuthProvider` in `@sep/shell`, and by PMM's embedded token + store for the session exchange. ## Usage @@ -60,6 +67,27 @@ setTokenProvider(() => currentAccessToken); setOnUnauthorized(() => redirectToLogin()); ``` +### Wire up auth in an embedded host that owns the session + +PMM has no SEP login flow and no refresh cookie: it trades its own session +cookie for a short-lived bearer, held in memory, and re-exchanges before expiry. +See `apps/pmm/src/sep/` for the store this wiring points at. + +```ts +import { + postSessionExchange, + setOnRefreshed, + setOnUnauthorized, + setTokenMinter, + setTokenProvider, +} from '@sep/api'; + +setTokenProvider(getSepToken); // synchronous read of the in-memory bearer +setTokenMinter(() => postSessionExchange()); // POST /oauth/session/exchange +setOnRefreshed(recordSepToken); // store it, schedule the next exchange +setOnUnauthorized(markSepSignedOut); // no login to redirect to +``` + ### Call the API ```ts diff --git a/ui/packages/sep/api/src/client.ts b/ui/packages/sep/api/src/client.ts index 30c9c6df130..442db2384e8 100644 --- a/ui/packages/sep/api/src/client.ts +++ b/ui/packages/sep/api/src/client.ts @@ -28,15 +28,50 @@ type TokenProvider = () => string | null; type OnUnauthorized = () => void; type OnRefreshed = (accessToken: string, expiresIn: number) => void; +/** + * Slim token payload every minting endpoint returns. Matches both + * `SPAOAuthTokenResponse` (`/oauth/refresh`) and `SessionExchangeTokenResponse` + * (`/oauth/session/exchange`), which mirror each other by design. + */ +export interface MintedToken { + access_token: string; + expires_in: number; +} + +/** + * Produces a fresh access token. Resolving `null` (or rejecting) means none + * could be obtained, which the caller treats as unauthorized. + */ +type TokenMinter = () => Promise; + let _getToken: TokenProvider = () => null; let _onUnauthorized: OnUnauthorized = () => {}; let _onRefreshed: OnRefreshed = () => {}; +let _mintToken: TokenMinter = mintViaRefreshCookie; /** Inject a callback that returns the current access token. */ export function setTokenProvider(provider: TokenProvider) { _getToken = provider; } +/** + * Replace how a fresh token is obtained. Defaults to the cookie-backed + * `POST /oauth/refresh` used by the standalone SPA. + * + * An embedded host that owns the session instead of SEP (PMM) registers a + * minter that exchanges its own session cookie via + * `POST /oauth/session/exchange`: no refresh cookie exists there, so the + * default would 401 on every recovery attempt. Everything downstream — + * single-flight coalescing in {@link refreshAccessToken}, the 401 retry in + * both transports, the `setOnRefreshed` notification — is transport-agnostic + * and works unchanged. + * + * Pass null to restore the default. + */ +export function setTokenMinter(minter: TokenMinter | null) { + _mintToken = minter ?? mintViaRefreshCookie; +} + /** Inject a callback invoked when the API receives an unauthorized response. */ export function setOnUnauthorized(handler: OnUnauthorized) { _onUnauthorized = handler; @@ -112,24 +147,47 @@ const isRefreshRequest = (url: string | undefined) => const isLoginRequest = (url: string | undefined) => !!url && url.includes('/oauth/login'); +/** + * Every endpoint that mints a token, whichever minter is registered. These + * must never enter the 401 retry path: `refreshAccessToken()` single-flights, + * so a 401 on the in-flight mint would hand the interceptor the very promise + * it is already running inside — an await on itself that never settles. + * + * Broader than {@link isRefreshRequest}, which still guards the unauthorized + * handler alone: a rejected mint on a session endpoint genuinely means "not + * signed in" and should reach the auth layer. + */ +export const isTokenMintRequest = (url: string | undefined) => + isRefreshRequest(url) || (!!url && url.includes('/oauth/session')); + // Internal marker so retried requests don't loop through the refresh path // again on a second 401. type RetriableConfig = InternalAxiosRequestConfig & { _retried?: boolean }; -// Single-flight refresh: concurrent callers (401 retry path + background -// timer + bootstrap) share one in-flight /oauth/refresh call. The promise -// resolves to the new access token on success and null on failure so +/** + * Default minter: rotate the `HttpOnly` refresh cookie for a new access token. + * A function declaration so it can back `_mintToken` above its own definition. + */ +async function mintViaRefreshCookie(): Promise { + const { data } = await apiClient.post('/oauth/refresh'); + return data; +} + +// Single-flight mint: concurrent callers (401 retry path + background +// timer + bootstrap) share one in-flight call to the registered minter. The +// promise resolves to the new access token on success and null on failure so // callers can decide whether to retry, surface the 401, or force logout. // -// All refresh traffic must funnel through here — the refresh token cookie -// rotates on every successful call, so parallel refreshes from different -// code paths would invalidate each other. +// All minting traffic must funnel through here — the default minter rotates +// the refresh token cookie on every successful call, so parallel refreshes +// from different code paths would invalidate each other, and a session +// exchange fanned out per request would hammer the identity provider. let refreshInFlight: Promise | null = null; /** - * Trigger (or join) the shared silent refresh. Resolves with the new - * access token, or null if the refresh failed (missing/invalid cookie, - * network error, Casdoor rejection). + * Trigger (or join) the shared silent mint. Resolves with the new access + * token, or null if minting failed (missing/invalid cookie or host session, + * network error, provider rejection). */ export function refreshAccessToken(): Promise { if (!refreshInFlight) { @@ -138,13 +196,13 @@ export function refreshAccessToken(): Promise { // from the externally-injected _onRefreshed handler must NOT be reported // as a failed refresh, otherwise the auth layer would force-logout a // user whose cookie rotation succeeded on the backend. - let data: { access_token: string; expires_in: number }; + let data: MintedToken; try { - const response = await apiClient.post<{ - access_token: string; - expires_in: number; - }>('/oauth/refresh'); - data = response.data; + const minted = await _mintToken(); + if (!minted) { + return null; + } + data = minted; } catch { return null; } finally { @@ -206,12 +264,12 @@ apiClient.interceptors.response.use( const url = config?.url; // 401 on a normal request: attempt one silent refresh, then retry. - // Skip the refresh/login endpoints themselves and already-retried requests. + // Skip the minting/login endpoints themselves and already-retried requests. if ( status === 401 && config && !config._retried && - !isRefreshRequest(url) && + !isTokenMintRequest(url) && !isLoginRequest(url) ) { const newToken = await refreshAccessToken(); diff --git a/ui/packages/sep/api/src/index.ts b/ui/packages/sep/api/src/index.ts index 7c6f340db65..cc4566e8b80 100644 --- a/ui/packages/sep/api/src/index.ts +++ b/ui/packages/sep/api/src/index.ts @@ -24,9 +24,11 @@ export { getToken, refreshAccessToken, setTokenProvider, + setTokenMinter, setOnUnauthorized, setOnRefreshed, } from './client'; +export type { MintedToken } from './client'; // Query client export { createQueryClient, defaultQueryClientConfig } from './queryClient'; diff --git a/ui/packages/sep/api/src/typed-client.ts b/ui/packages/sep/api/src/typed-client.ts index 33dbd0e1cd5..d4bc4fc5c72 100644 --- a/ui/packages/sep/api/src/typed-client.ts +++ b/ui/packages/sep/api/src/typed-client.ts @@ -36,7 +36,12 @@ * shape regardless of which client they use. */ import createClient, { type Client, type Middleware } from 'openapi-fetch'; -import { emitUnauthorized, getToken } from './client'; +import { + emitUnauthorized, + getToken, + isTokenMintRequest, + refreshAccessToken, +} from './client'; import { ApiError } from './errors'; import type { paths as MainPaths } from './generated/main'; import type { paths as SepPaths } from './generated/sep'; @@ -44,6 +49,7 @@ import type { paths as SepPaths } from './generated/sep'; const IS_DEV = import.meta.env.DEV; const isRefreshRequest = (url: string) => url.includes('/oauth/refresh'); +const isLoginRequest = (url: string) => url.includes('/oauth/login'); /** * A 200 HTML response (e.g. a follow of a login redirect) means the session @@ -55,12 +61,46 @@ function isHtmlLoginResponse(response: Response): boolean { return response.ok && ct.includes('text/html'); } +// `fetch` consumes a Request's body stream, so the instance handed to +// `onResponse` can no longer be re-sent. Stash an untouched clone taken before +// dispatch, keyed weakly so requests that never come back are not retained. +const pristineRequests = new WeakMap(); + +/** + * One silent recovery attempt for a 401: mint a fresh token — single-flighted + * with every other caller, including the axios transport — and replay the + * request with it. + * + * The replay goes through raw `fetch` rather than the typed client so it cannot + * re-enter this middleware; that bounds recovery to a single extra round-trip + * without needing a retry marker. Returns null when there is nothing to replay + * or no token could be minted. + */ +async function replayWithFreshToken( + request: Request +): Promise { + const pristine = pristineRequests.get(request); + if (!pristine) { + return null; + } + pristineRequests.delete(request); + + const token = await refreshAccessToken(); + if (!token) { + return null; + } + + pristine.headers.set('Authorization', `Bearer ${token}`); + return lazyFetch(pristine); +} + const authMiddleware: Middleware = { onRequest({ request }) { const token = getToken(); if (token) { request.headers.set('Authorization', `Bearer ${token}`); } + pristineRequests.set(request, request.clone()); if (IS_DEV) { // eslint-disable-next-line no-console console.debug( @@ -85,10 +125,27 @@ const authMiddleware: Middleware = { }); } + if ( + response.status === 401 && + !isTokenMintRequest(request.url) && + !isLoginRequest(request.url) + ) { + const replayed = await replayWithFreshToken(request); + if (replayed && replayed.status !== 401) { + return replayed; + } + // Minting failed, or the replay was rejected too — the session is gone. + emitUnauthorized(); + return replayed ?? response; + } + if ( (response.status === 401 || response.status === 303) && !isRefreshRequest(request.url) ) { + // A 401 left here is a minting endpoint rejecting the ambient session — + // "not signed in", which the auth layer must hear about. A 303 is the + // login redirect on any endpoint. emitUnauthorized(); } diff --git a/ui/packages/sep/api/tests/client.test.ts b/ui/packages/sep/api/tests/client.test.ts index 8aa43ab5f45..5891de09156 100644 --- a/ui/packages/sep/api/tests/client.test.ts +++ b/ui/packages/sep/api/tests/client.test.ts @@ -17,11 +17,13 @@ import { http, HttpResponse } from 'msw'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { postSessionExchange } from '../src/auth'; import { apiClient, refreshAccessToken, setOnRefreshed, setOnUnauthorized, + setTokenMinter, setTokenProvider, } from '../src/client'; import { ApiError } from '../src/errors'; @@ -42,6 +44,7 @@ afterEach(() => { setTokenProvider(() => null); setOnUnauthorized(() => {}); setOnRefreshed(() => {}); + setTokenMinter(null); }); describe('apiClient — Bearer token injection', () => { @@ -357,3 +360,145 @@ describe('apiClient — 401 refresh-retry', () => { expect(onUnauth).toHaveBeenCalledOnce(); }); }); + +describe('apiClient — pluggable token minter', () => { + it('recovers through the registered minter instead of /oauth/refresh', async () => { + let currentToken = 'old'; + setTokenProvider(() => currentToken); + setOnRefreshed((token) => { + currentToken = token; + }); + // The embedded PMM wiring: exchange the host session cookie, no refresh + // cookie exists. `/oauth/refresh` is deliberately left unhandled — MSW is + // configured to error on unhandled requests, so reaching it fails the test. + setTokenMinter(() => postSessionExchange()); + let exchanges = 0; + + server.use( + http.get(`${BASE}/api/protected`, ({ request }) => { + if (request.headers.get('Authorization') === 'Bearer minted') { + return HttpResponse.json({ ok: true }); + } + return HttpResponse.json({ detail: 'expired' }, { status: 401 }); + }), + http.post(`${BASE}/api/oauth/session/exchange`, () => { + exchanges += 1; + return HttpResponse.json({ access_token: 'minted', expires_in: 300 }); + }) + ); + + const res = await apiClient.get('/protected'); + + expect(res.status).toBe(200); + expect(exchanges).toBe(1); + expect(currentToken).toBe('minted'); + }); + + it('coalesces a burst of 401s into one exchange', async () => { + let currentToken = 'old'; + setTokenProvider(() => currentToken); + setOnRefreshed((token) => { + currentToken = token; + }); + setTokenMinter(() => postSessionExchange()); + let exchanges = 0; + + const protectedHandler = ({ request }: { request: Request }) => + request.headers.get('Authorization') === 'Bearer minted' + ? HttpResponse.json({ ok: true }) + : HttpResponse.json({ detail: 'expired' }, { status: 401 }); + + server.use( + http.get(`${BASE}/api/a`, protectedHandler), + http.get(`${BASE}/api/b`, protectedHandler), + http.get(`${BASE}/api/c`, protectedHandler), + http.post(`${BASE}/api/oauth/session/exchange`, async () => { + exchanges += 1; + // Hold the exchange open so all three 401s land while it is in flight. + await new Promise((resolve) => setTimeout(resolve, 20)); + return HttpResponse.json({ access_token: 'minted', expires_in: 300 }); + }) + ); + + const responses = await Promise.all([ + apiClient.get('/a'), + apiClient.get('/b'), + apiClient.get('/c'), + ]); + + expect(responses.map((r) => r.status)).toEqual([200, 200, 200]); + expect(exchanges).toBe(1); + }); + + it('does not re-enter the retry path when the exchange itself 401s', async () => { + // Regression guard: the mint is single-flighted, so routing its own 401 + // back through the retry interceptor would hand that interceptor the very + // promise it is running inside — an await on itself that never settles. + // A hang here surfaces as a test timeout, not a failed assertion. + setTokenProvider(() => 'old'); + const onUnauth = vi.fn(); + setOnUnauthorized(onUnauth); + setTokenMinter(() => postSessionExchange()); + let exchanges = 0; + + server.use( + http.get(`${BASE}/api/protected`, () => + HttpResponse.json({ detail: 'expired' }, { status: 401 }) + ), + http.post(`${BASE}/api/oauth/session/exchange`, () => { + exchanges += 1; + return HttpResponse.json({ detail: 'no session' }, { status: 401 }); + }) + ); + + await expect(apiClient.get('/protected')).rejects.toBeInstanceOf(ApiError); + expect(exchanges).toBe(1); + // Once for the rejected exchange, once for the unrecoverable request. + expect(onUnauth).toHaveBeenCalledTimes(2); + }); + + it('notifies the auth layer when the exchange endpoint rejects the session', async () => { + const onUnauth = vi.fn(); + setOnUnauthorized(onUnauth); + + server.use( + http.post(`${BASE}/api/oauth/session/exchange`, () => + HttpResponse.json({ detail: 'no session' }, { status: 401 }) + ) + ); + + await expect(postSessionExchange()).rejects.toBeInstanceOf(ApiError); + expect(onUnauth).toHaveBeenCalledOnce(); + }); + + it('treats a minter resolving null as a failed mint', async () => { + setTokenProvider(() => 'old'); + const onUnauth = vi.fn(); + setOnUnauthorized(onUnauth); + const minter = vi.fn(async () => null); + setTokenMinter(minter); + + server.use( + http.get(`${BASE}/api/protected`, () => + HttpResponse.json({ detail: 'expired' }, { status: 401 }) + ) + ); + + await expect(apiClient.get('/protected')).rejects.toBeInstanceOf(ApiError); + expect(minter).toHaveBeenCalledOnce(); + expect(onUnauth).toHaveBeenCalledOnce(); + }); + + it('restores the default /oauth/refresh minter when passed null', async () => { + setTokenMinter(async () => ({ access_token: 'custom', expires_in: 1 })); + setTokenMinter(null); + + server.use( + http.post(`${BASE}/api/oauth/refresh`, () => + HttpResponse.json({ access_token: 'from-cookie', expires_in: 300 }) + ) + ); + + await expect(refreshAccessToken()).resolves.toBe('from-cookie'); + }); +}); diff --git a/ui/packages/sep/api/tests/typed-client.test.ts b/ui/packages/sep/api/tests/typed-client.test.ts index 646bf610f24..a8988e25d5e 100644 --- a/ui/packages/sep/api/tests/typed-client.test.ts +++ b/ui/packages/sep/api/tests/typed-client.test.ts @@ -17,9 +17,14 @@ import { http, HttpResponse } from 'msw'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { setOnUnauthorized, setTokenProvider } from '../src/client'; +import { + setOnRefreshed, + setOnUnauthorized, + setTokenMinter, + setTokenProvider, +} from '../src/client'; import { ApiError } from '../src/errors'; -import { mainApi, throwOnApiError } from '../src/typed-client'; +import { mainApi, sepApi, throwOnApiError } from '../src/typed-client'; import { server } from './msw-server'; // openapi-fetch builds absolute URLs from a `baseUrl`. The generated paths @@ -48,11 +53,17 @@ beforeEach(() => { }); setTokenProvider(() => null); setOnUnauthorized(() => {}); + setOnRefreshed(() => {}); + // Default to a minter that cannot recover, so the 401 tests below observe the + // give-up path without reaching the network. The recovery suite opts in. + setTokenMinter(async () => null); }); afterEach(() => { setTokenProvider(() => null); setOnUnauthorized(() => {}); + setOnRefreshed(() => {}); + setTokenMinter(null); if (ORIGINAL_LOCATION_DESCRIPTOR) { Object.defineProperty(globalThis, 'location', ORIGINAL_LOCATION_DESCRIPTOR); } else { @@ -126,6 +137,145 @@ describe('typed-client — auth middleware', () => { }); }); +describe('typed-client — 401 recovery', () => { + const mintOnce = (token: string) => { + const minter = vi.fn(async () => ({ + access_token: token, + expires_in: 300, + })); + setTokenMinter(minter); + return minter; + }; + + it('mints a fresh token and replays the request', async () => { + let currentToken = 'stale'; + setTokenProvider(() => currentToken); + setOnRefreshed((token) => { + currentToken = token; + }); + const minter = mintOnce('fresh'); + const onUnauth = vi.fn(); + setOnUnauthorized(onUnauth); + const seenAuth: Array = []; + + server.use( + http.get('http://localhost/api/users/me', ({ request }) => { + const auth = request.headers.get('Authorization'); + seenAuth.push(auth); + if (auth === 'Bearer fresh') { + return HttpResponse.json({ id: 'abc', username: 'u' }); + } + return HttpResponse.json({ detail: 'expired' }, { status: 401 }); + }) + ); + + const user = await throwOnApiError(mainApi.GET('/api/users/me')); + + expect(user).toMatchObject({ id: 'abc' }); + expect(seenAuth).toEqual(['Bearer stale', 'Bearer fresh']); + expect(minter).toHaveBeenCalledOnce(); + expect(onUnauth).not.toHaveBeenCalled(); + }); + + it('replays a request body — `fetch` consumed the original stream', async () => { + setTokenProvider(() => 'stale'); + mintOnce('fresh'); + const seenBodies: unknown[] = []; + + server.use( + http.post( + 'http://localhost/api/apps/inventory/sync/', + async ({ request }) => { + const auth = request.headers.get('Authorization'); + seenBodies.push(await request.json()); + if (auth === 'Bearer fresh') { + return HttpResponse.json({ status: 'queued' }); + } + return HttpResponse.json({ detail: 'expired' }, { status: 401 }); + } + ) + ); + + await throwOnApiError( + sepApi.POST('/api/apps/inventory/sync/', { + body: { syncer: 'mod.Cls' }, + }) + ); + + expect(seenBodies).toEqual([{ syncer: 'mod.Cls' }, { syncer: 'mod.Cls' }]); + }); + + it('replays at most once, then reports unauthorized', async () => { + setTokenProvider(() => 'stale'); + const minter = mintOnce('fresh'); + const onUnauth = vi.fn(); + setOnUnauthorized(onUnauth); + let calls = 0; + + server.use( + http.get('http://localhost/api/users/me', () => { + calls += 1; + return HttpResponse.json({ detail: 'expired' }, { status: 401 }); + }) + ); + + await expect( + throwOnApiError(mainApi.GET('/api/users/me')) + ).rejects.toSatisfy((err) => err instanceof ApiError && err.status === 401); + expect(calls).toBe(2); + expect(minter).toHaveBeenCalledOnce(); + expect(onUnauth).toHaveBeenCalledOnce(); + }); + + it('shares one mint across concurrent 401s', async () => { + let currentToken = 'stale'; + setTokenProvider(() => currentToken); + setOnRefreshed((token) => { + currentToken = token; + }); + let mints = 0; + setTokenMinter(async () => { + mints += 1; + await new Promise((resolve) => setTimeout(resolve, 20)); + return { access_token: 'fresh', expires_in: 300 }; + }); + + server.use( + http.get('http://localhost/api/users/me', ({ request }) => + request.headers.get('Authorization') === 'Bearer fresh' + ? HttpResponse.json({ id: 'abc', username: 'u' }) + : HttpResponse.json({ detail: 'expired' }, { status: 401 }) + ) + ); + + const results = await Promise.all([ + throwOnApiError(mainApi.GET('/api/users/me')), + throwOnApiError(mainApi.GET('/api/users/me')), + ]); + + expect(results).toHaveLength(2); + expect(mints).toBe(1); + }); + + it('does not attempt recovery when the exchange endpoint itself 401s', async () => { + const minter = mintOnce('fresh'); + const onUnauth = vi.fn(); + setOnUnauthorized(onUnauth); + + server.use( + http.post('http://localhost/api/oauth/session/exchange', () => + HttpResponse.json({ detail: 'no session' }, { status: 401 }) + ) + ); + + await mainApi.POST('/api/oauth/session/exchange'); + + expect(minter).not.toHaveBeenCalled(); + // A rejected exchange is "not signed in" and must reach the auth layer. + expect(onUnauth).toHaveBeenCalledOnce(); + }); +}); + describe('throwOnApiError', () => { it('returns typed data on 2xx', async () => { server.use( From 3ef17243992821a59233f0b979237c22e8875f94 Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Wed, 5 Aug 2026 18:43:41 -0300 Subject: [PATCH 02/23] PMM-15293 Mint the SEP bearer from the PMM session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The embedded SEP UI authenticated as SEP's internal service principal: the token provider returned null and the proxy injected PMM_DEV_SEP_INTERNAL_TOKEN server-side. That principal hardcodes `is_admin = False`, so every admin-gated SEP surface answered 403. It now authenticates as the actual PMM user. `sepTokenStore` exchanges the ambient `pmm_session` cookie for a short-lived SEP bearer via `POST /api/oauth/session/exchange` (SEP-1692) and holds it in memory only — no localStorage, no sessionStorage, no query cache. It renews 30s ahead of the 5-minute expiry, and the transports' 401 retry covers the case where a throttled background tab misses that window. Concurrency is delegated to `refreshAccessToken()`, so a burst of parallel SEP requests triggers one exchange. A 401 from the exchange itself is sticky: minting is refused until the user retries, so a rejected session cannot drive an exchange loop. `SepAuthGate` triggers the first exchange when a SEP route mounts rather than at app startup — the UI has no PMM_ENABLE_SEP flag, so an eager exchange would hit SEP on every page load for every PMM user. It also closes a race the provider cannot: `setTokenProvider` is synchronous, so a plugin's first queries would otherwise fire before the exchange resolved. The dev proxy no longer injects the internal token on `/api/oauth/*`. Overwriting Authorization there would authenticate the exchange as the service principal and mask whether the cookie path works at all. Retiring the injection entirely is a follow-up. Signed-off-by: Ignacio Durand --- ui/apps/pmm/src/sep/SepAuthGate.messages.ts | 10 + ui/apps/pmm/src/sep/SepAuthGate.test.tsx | 101 ++++++++ ui/apps/pmm/src/sep/SepAuthGate.tsx | 82 +++++++ ui/apps/pmm/src/sep/SepPage.tsx | 8 +- ui/apps/pmm/src/sep/bootstrap.ts | 41 ++-- ui/apps/pmm/src/sep/sepTokenStore.test.ts | 246 ++++++++++++++++++++ ui/apps/pmm/src/sep/sepTokenStore.ts | 219 +++++++++++++++++ ui/apps/pmm/vite.config.ts | 21 +- 8 files changed, 709 insertions(+), 19 deletions(-) create mode 100644 ui/apps/pmm/src/sep/SepAuthGate.messages.ts create mode 100644 ui/apps/pmm/src/sep/SepAuthGate.test.tsx create mode 100644 ui/apps/pmm/src/sep/SepAuthGate.tsx create mode 100644 ui/apps/pmm/src/sep/sepTokenStore.test.ts create mode 100644 ui/apps/pmm/src/sep/sepTokenStore.ts diff --git a/ui/apps/pmm/src/sep/SepAuthGate.messages.ts b/ui/apps/pmm/src/sep/SepAuthGate.messages.ts new file mode 100644 index 00000000000..b054ece3b8c --- /dev/null +++ b/ui/apps/pmm/src/sep/SepAuthGate.messages.ts @@ -0,0 +1,10 @@ +export const Messages = { + loading: 'Authenticating with Smart Expert Platform…', + signedOutTitle: 'Not signed in', + signedOut: + 'Smart Expert Platform could not verify your PMM session. Sign in to PMM again, then retry.', + errorTitle: 'Could not reach Smart Expert Platform', + error: + 'Authenticating with Smart Expert Platform failed. This is usually temporary.', + retry: 'Try again', +}; diff --git a/ui/apps/pmm/src/sep/SepAuthGate.test.tsx b/ui/apps/pmm/src/sep/SepAuthGate.test.tsx new file mode 100644 index 00000000000..d3b12940370 --- /dev/null +++ b/ui/apps/pmm/src/sep/SepAuthGate.test.tsx @@ -0,0 +1,101 @@ +import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { ApiError, postSessionExchange, setTokenMinter } from '@sep/api'; +import { SepAuthGate } from './SepAuthGate'; +import { initSepAuth } from './bootstrap'; +import { resetSepAuthStore } from './sepTokenStore'; + +vi.mock('@sep/api', async (importOriginal) => ({ + ...(await importOriginal()), + postSessionExchange: vi.fn(), +})); + +const exchange = vi.mocked(postSessionExchange); + +const unauthorized = () => + new ApiError({ kind: 'http', status: 401, message: 'no session' }); + +const renderGate = () => + render( + +
plugin content
+ + ); + +beforeEach(() => { + exchange.mockReset(); + resetSepAuthStore(); + initSepAuth(); +}); + +afterEach(() => { + resetSepAuthStore(); + setTokenMinter(null); +}); + +describe('SepAuthGate', () => { + it('withholds children until the exchange resolves', async () => { + let resolveExchange: (value: { + access_token: string; + expires_in: number; + }) => void = () => {}; + exchange.mockReturnValue( + new Promise((resolve) => { + resolveExchange = resolve; + }) + ); + + renderGate(); + + expect(screen.queryByText('plugin content')).not.toBeInTheDocument(); + expect(screen.getByRole('progressbar')).toBeInTheDocument(); + + resolveExchange({ access_token: 'bearer-1', expires_in: 300 }); + + expect(await screen.findByText('plugin content')).toBeInTheDocument(); + }); + + it('renders children once a bearer is held', async () => { + exchange.mockResolvedValue({ access_token: 'bearer-1', expires_in: 300 }); + + renderGate(); + + expect(await screen.findByText('plugin content')).toBeInTheDocument(); + expect(exchange).toHaveBeenCalledOnce(); + }); + + it('reports a rejected session instead of looping on the exchange', async () => { + exchange.mockRejectedValue(unauthorized()); + + renderGate(); + + expect(await screen.findByTestId('sep-auth-error')).toHaveTextContent( + 'Not signed in' + ); + expect(screen.queryByText('plugin content')).not.toBeInTheDocument(); + // Waiting past any plausible retry delay: the failure must stay put. + await waitFor(() => expect(exchange).toHaveBeenCalledOnce()); + expect(exchange).toHaveBeenCalledOnce(); + }); + + it('exchanges again when the user retries', async () => { + exchange.mockRejectedValue(unauthorized()); + renderGate(); + await screen.findByTestId('sep-auth-error'); + + exchange.mockResolvedValue({ access_token: 'bearer-1', expires_in: 300 }); + fireEvent.click(screen.getByRole('button', { name: 'Try again' })); + + expect(await screen.findByText('plugin content')).toBeInTheDocument(); + expect(exchange).toHaveBeenCalledTimes(2); + }); + + it('distinguishes a transient failure from a rejected session', async () => { + exchange.mockRejectedValue(new Error('network down')); + + renderGate(); + + expect(await screen.findByTestId('sep-auth-error')).toHaveTextContent( + 'Could not reach Smart Expert Platform' + ); + }); +}); diff --git a/ui/apps/pmm/src/sep/SepAuthGate.tsx b/ui/apps/pmm/src/sep/SepAuthGate.tsx new file mode 100644 index 00000000000..b040f1ba60d --- /dev/null +++ b/ui/apps/pmm/src/sep/SepAuthGate.tsx @@ -0,0 +1,82 @@ +import { FC, PropsWithChildren, useEffect, useSyncExternalStore } from 'react'; +import { + Alert, + AlertTitle, + Box, + Button, + CircularProgress, +} from '@mui/material'; +import { Messages } from './SepAuthGate.messages'; +import { + ensureSepToken, + getSepAuthStatus, + retrySepAuth, + subscribeSepAuth, +} from './sepTokenStore'; + +/** + * Holds a SEP route until a SEP bearer has been minted from the PMM session. + * + * Gating here rather than exchanging at app startup keeps SEP out of the boot + * path for the PMM users who never open a SEP page — the UI has no + * `PMM_ENABLE_SEP` flag to check, so an eager exchange would hit SEP on every + * page load for everybody. + * + * It also removes a race the token provider cannot: `setTokenProvider` is + * synchronous, so a plugin's first queries would otherwise fire before the + * exchange resolves and 401 on arrival. Children do not render until a bearer + * is in hand. + */ +export const SepAuthGate: FC = ({ children }) => { + const status = useSyncExternalStore(subscribeSepAuth, getSepAuthStatus); + + useEffect(() => { + // No-ops when a bearer is already held or the session was rejected; a + // previous transient failure is retried on the next visit to a SEP route. + void ensureSepToken(); + }, []); + + if (status === 'ready') { + return <>{children}; + } + + if (status === 'signedOut' || status === 'error') { + const signedOut = status === 'signedOut'; + return ( + { + void retrySepAuth(); + }} + > + {Messages.retry} + + } + > + + {signedOut ? Messages.signedOutTitle : Messages.errorTitle} + + {signedOut ? Messages.signedOut : Messages.error} + + ); + } + + return ( + + + + ); +}; diff --git a/ui/apps/pmm/src/sep/SepPage.tsx b/ui/apps/pmm/src/sep/SepPage.tsx index 9ceda00c553..605a0e74ad6 100644 --- a/ui/apps/pmm/src/sep/SepPage.tsx +++ b/ui/apps/pmm/src/sep/SepPage.tsx @@ -3,6 +3,7 @@ import Stack from '@mui/material/Stack'; import { Page } from 'components/page'; import { useUser } from 'contexts/user'; import { OrgRole } from 'types/user.types'; +import { SepAuthGate } from './SepAuthGate'; /** * Shared container for SEP apps mounted as native PMM routes. @@ -19,6 +20,9 @@ import { OrgRole } from 'types/user.types'; * `isPMMAdmin` is `isGrafanaAdmin || orgRole === Admin`, and `roles` (org-role * only) cannot express the Grafana-admin half on its own, so it gates the * remaining case and Page renders its standard unauthorized card. + * + * `SepAuthGate` sits inside that check, so the SEP session exchange only runs + * for a user who is allowed on the page in the first place. */ export const SepPage: FC = ({ children }) => { const { user } = useUser(); @@ -29,7 +33,9 @@ export const SepPage: FC = ({ children }) => { roles={user?.isPMMAdmin ? undefined : [OrgRole.Admin]} > -
{children}
+ +
{children}
+ ); diff --git a/ui/apps/pmm/src/sep/bootstrap.ts b/ui/apps/pmm/src/sep/bootstrap.ts index 673acd4d977..e3b08b19eea 100644 --- a/ui/apps/pmm/src/sep/bootstrap.ts +++ b/ui/apps/pmm/src/sep/bootstrap.ts @@ -1,20 +1,35 @@ -import { setTokenProvider, setOnUnauthorized } from '@sep/api'; +import { + setOnRefreshed, + setOnUnauthorized, + setTokenMinter, + setTokenProvider, +} from '@sep/api'; +import { + getSepToken, + markSepSignedOut, + mintSepToken, + recordSepToken, +} from './sepTokenStore'; /** - * Interim SEP auth wiring (migration Option D). + * SEP auth wiring for the embedded UI. * - * SEP's axios client delegates the bearer token via `setTokenProvider`. During the - * migration the PMM dev proxy injects `PMM_DEV_SEP_INTERNAL_TOKEN` server-side, - * so the browser sends no token — the provider returns `null`. `setOnUnauthorized` is a - * no-op because there is no SEP login flow to redirect to (PMM owns the session). + * PMM owns the session, so SEP is authenticated as the actual PMM user by + * exchanging the `pmm_session` cookie for a short-lived SEP bearer + * (`POST /api/oauth/session/exchange`, SEP-1692) rather than by logging in. + * This replaces the interim wiring in which the dev proxy injected + * `PMM_DEV_SEP_INTERNAL_TOKEN` server-side: that authenticated as SEP's + * internal service principal, which hardcodes `is_admin = False`, so every + * admin-gated SEP surface answered 403. * - * This is replaced by the token-exchange provider (Option B), which calls - * `postSessionExchange()` (`POST /api/oauth/session/exchange`, SEP-1692) to trade - * PMM's session cookie for a short-lived SEP bearer, at which point `isAdmin` also - * comes from the token's role claim rather than the internal token's service - * principal, which hardcodes `is_admin = False`. + * Registration is side-effect free — no network call happens here. The first + * exchange is triggered by `SepAuthGate` when a SEP route mounts, so PMM users + * who never open one never talk to SEP. State and lifetime live in + * `./sepTokenStore`. */ export const initSepAuth = () => { - setTokenProvider(() => null); - setOnUnauthorized(() => {}); + setTokenProvider(getSepToken); + setTokenMinter(mintSepToken); + setOnRefreshed(recordSepToken); + setOnUnauthorized(markSepSignedOut); }; diff --git a/ui/apps/pmm/src/sep/sepTokenStore.test.ts b/ui/apps/pmm/src/sep/sepTokenStore.test.ts new file mode 100644 index 00000000000..ef61df45c09 --- /dev/null +++ b/ui/apps/pmm/src/sep/sepTokenStore.test.ts @@ -0,0 +1,246 @@ +import { + ApiError, + getToken, + postSessionExchange, + setTokenMinter, +} from '@sep/api'; +import { initSepAuth } from './bootstrap'; +import { + ensureSepToken, + getSepAuthStatus, + getSepToken, + resetSepAuthStore, + retrySepAuth, +} from './sepTokenStore'; + +// Mock only the network boundary. `refreshAccessToken`'s single-flight, the +// token-minter seam, and the unauthorized wiring stay real, so these exercise +// the store against the coordinator it actually runs against. +vi.mock('@sep/api', async (importOriginal) => ({ + ...(await importOriginal()), + postSessionExchange: vi.fn(), +})); + +const exchange = vi.mocked(postSessionExchange); + +const TTL_SECONDS = 300; +const mintedToken = (accessToken: string) => ({ + access_token: accessToken, + expires_in: TTL_SECONDS, +}); + +const unauthorized = () => + new ApiError({ kind: 'http', status: 401, message: 'no session' }); + +beforeEach(() => { + // Leave `queueMicrotask` real: `refreshAccessToken` clears its single-flight + // slot in a microtask, and faking that would deadlock the second exchange. + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout', 'Date'] }); + exchange.mockReset(); + resetSepAuthStore(); + initSepAuth(); +}); + +afterEach(() => { + resetSepAuthStore(); + setTokenMinter(null); + vi.useRealTimers(); +}); + +describe('sepTokenStore — acquiring a bearer', () => { + it('holds no token until an exchange runs', () => { + expect(getSepToken()).toBeNull(); + expect(getSepAuthStatus()).toBe('idle'); + expect(exchange).not.toHaveBeenCalled(); + }); + + it('exchanges once and exposes the bearer synchronously', async () => { + exchange.mockResolvedValue(mintedToken('bearer-1')); + + await expect(ensureSepToken()).resolves.toBe(true); + + expect(exchange).toHaveBeenCalledOnce(); + expect(getSepToken()).toBe('bearer-1'); + expect(getSepAuthStatus()).toBe('ready'); + }); + + it('serves the bearer through the token provider registered on @sep/api', async () => { + exchange.mockResolvedValue(mintedToken('bearer-1')); + + await ensureSepToken(); + + expect(getToken()).toBe('bearer-1'); + }); + + it('reuses the held bearer instead of exchanging again', async () => { + exchange.mockResolvedValue(mintedToken('bearer-1')); + + await ensureSepToken(); + await ensureSepToken(); + + expect(exchange).toHaveBeenCalledOnce(); + }); + + it('coalesces concurrent callers into one exchange', async () => { + let resolveExchange: ( + value: ReturnType + ) => void = () => {}; + exchange.mockReturnValue( + new Promise((resolve) => { + resolveExchange = resolve; + }) + ); + + const pending = Promise.all([ + ensureSepToken(), + ensureSepToken(), + ensureSepToken(), + ]); + resolveExchange(mintedToken('bearer-1')); + + await expect(pending).resolves.toEqual([true, true, true]); + expect(exchange).toHaveBeenCalledOnce(); + }); + + it('never writes the bearer to web storage', async () => { + exchange.mockResolvedValue(mintedToken('bearer-1')); + + await ensureSepToken(); + + expect(Object.keys(localStorage)).toHaveLength(0); + expect(Object.keys(sessionStorage)).toHaveLength(0); + }); +}); + +describe('sepTokenStore — lifetime', () => { + it('stops serving the bearer once it has expired', async () => { + exchange.mockResolvedValue(mintedToken('bearer-1')); + await ensureSepToken(); + + vi.setSystemTime(Date.now() + TTL_SECONDS * 1000 + 1); + + expect(getSepToken()).toBeNull(); + }); + + it('re-exchanges shortly before expiry', async () => { + exchange.mockResolvedValue(mintedToken('bearer-1')); + await ensureSepToken(); + exchange.mockResolvedValue(mintedToken('bearer-2')); + + // 30s of skew ahead of the 300s TTL. + await vi.advanceTimersByTimeAsync(270_000); + + expect(exchange).toHaveBeenCalledTimes(2); + expect(getSepToken()).toBe('bearer-2'); + expect(getSepAuthStatus()).toBe('ready'); + }); + + it('keeps renewing across successive lifetimes', async () => { + exchange.mockResolvedValue(mintedToken('bearer-1')); + await ensureSepToken(); + exchange.mockResolvedValue(mintedToken('bearer-2')); + await vi.advanceTimersByTimeAsync(270_000); + exchange.mockResolvedValue(mintedToken('bearer-3')); + await vi.advanceTimersByTimeAsync(270_000); + + expect(exchange).toHaveBeenCalledTimes(3); + expect(getSepToken()).toBe('bearer-3'); + }); + + it('drops the bearer on a failed renewal without tearing down the page', async () => { + exchange.mockResolvedValue(mintedToken('bearer-1')); + await ensureSepToken(); + exchange.mockRejectedValue(new Error('offline')); + + await vi.advanceTimersByTimeAsync(270_000); + + expect(getSepToken()).toBeNull(); + // Still `ready`, so a mounted SEP page keeps its state; the next request + // 401s and mints through the transports' retry. + expect(getSepAuthStatus()).toBe('ready'); + }); + + it('re-acquires on the next visit after a failed renewal', async () => { + exchange.mockResolvedValue(mintedToken('bearer-1')); + await ensureSepToken(); + exchange.mockRejectedValue(new Error('offline')); + await vi.advanceTimersByTimeAsync(270_000); + + exchange.mockResolvedValue(mintedToken('bearer-2')); + await expect(ensureSepToken()).resolves.toBe(true); + + expect(getSepToken()).toBe('bearer-2'); + }); + + it('stops renewing once the store is cleared', async () => { + exchange.mockResolvedValue(mintedToken('bearer-1')); + await ensureSepToken(); + + resetSepAuthStore(); + await vi.advanceTimersByTimeAsync(270_000); + + expect(exchange).toHaveBeenCalledOnce(); + }); +}); + +describe('sepTokenStore — rejected session', () => { + it('treats a 401 from the exchange as signed out', async () => { + exchange.mockRejectedValue(unauthorized()); + + await expect(ensureSepToken()).resolves.toBe(false); + + expect(getSepAuthStatus()).toBe('signedOut'); + expect(getSepToken()).toBeNull(); + }); + + it('refuses to exchange again while signed out', async () => { + exchange.mockRejectedValue(unauthorized()); + await ensureSepToken(); + + await expect(ensureSepToken()).resolves.toBe(false); + await expect(ensureSepToken()).resolves.toBe(false); + + expect(exchange).toHaveBeenCalledOnce(); + }); + + it('exchanges again only when the user explicitly retries', async () => { + exchange.mockRejectedValue(unauthorized()); + await ensureSepToken(); + exchange.mockResolvedValue(mintedToken('bearer-1')); + + await expect(retrySepAuth()).resolves.toBe(true); + + expect(exchange).toHaveBeenCalledTimes(2); + expect(getSepToken()).toBe('bearer-1'); + }); + + it('does not schedule a renewal after a rejected session', async () => { + exchange.mockRejectedValue(unauthorized()); + await ensureSepToken(); + + await vi.advanceTimersByTimeAsync(600_000); + + expect(exchange).toHaveBeenCalledOnce(); + }); +}); + +describe('sepTokenStore — transient failure', () => { + it('reports an error without going sticky', async () => { + exchange.mockRejectedValue(new Error('network down')); + + await expect(ensureSepToken()).resolves.toBe(false); + + expect(getSepAuthStatus()).toBe('error'); + }); + + it('retries on the next attempt', async () => { + exchange.mockRejectedValue(new Error('network down')); + await ensureSepToken(); + exchange.mockResolvedValue(mintedToken('bearer-1')); + + await expect(ensureSepToken()).resolves.toBe(true); + + expect(exchange).toHaveBeenCalledTimes(2); + expect(getSepToken()).toBe('bearer-1'); + }); +}); diff --git a/ui/apps/pmm/src/sep/sepTokenStore.ts b/ui/apps/pmm/src/sep/sepTokenStore.ts new file mode 100644 index 00000000000..c56fec6a268 --- /dev/null +++ b/ui/apps/pmm/src/sep/sepTokenStore.ts @@ -0,0 +1,219 @@ +import { + ApiError, + type MintedToken, + postSessionExchange, + refreshAccessToken, +} from '@sep/api'; + +/** + * In-memory holder for the SEP bearer PMM mints from its own session. + * + * `POST /api/oauth/session/exchange` (SEP-1692) trades the ambient `pmm_session` + * cookie — attached automatically, same origin through PMM's proxy — for a + * short-lived bearer. No cookie is set and no refresh token is issued, so the + * holder re-exchanges before expiry instead of refreshing. + * + * The token never leaves this module: no `localStorage`, no `sessionStorage`, no + * query cache. A page reload re-exchanges from the cookie, which is the point — + * every exchange re-reads the identity, so a role change lands within one bearer + * lifetime (5 minutes by default). + * + * Concurrency is not handled here. `refreshAccessToken()` in `@sep/api` + * single-flights every caller — the renewal timer, the initial gate, and each + * transport's 401 retry — so a burst of parallel SEP requests triggers one + * exchange. + */ + +/** + * Renew this far before the bearer actually expires, so in-flight requests + * carry a token that is still valid when SEP validates it. + */ +const EXPIRY_SKEW_MS = 30_000; + +/** Floor for the renewal delay, in case SEP ever issues a very short TTL. */ +const MIN_RENEWAL_DELAY_MS = 5_000; + +export type SepAuthStatus = + /** No exchange attempted yet. */ + | 'idle' + /** First exchange in flight; nothing to authenticate with yet. */ + | 'exchanging' + /** A usable bearer is held. */ + | 'ready' + /** + * SEP rejected the session. Sticky: minting is refused until + * {@link retrySepAuth} clears it, so a rejected session cannot drive an + * exchange loop. + */ + | 'signedOut' + /** The exchange failed for a reason that may not repeat (network, 5xx). */ + | 'error'; + +let token: string | null = null; +let expiresAtMs = 0; +let status: SepAuthStatus = 'idle'; +let renewalTimer: ReturnType | null = null; + +const listeners = new Set<() => void>(); + +const setStatus = (next: SepAuthStatus) => { + if (status === next) { + return; + } + status = next; + listeners.forEach((listener) => listener()); +}; + +const clearRenewalTimer = () => { + if (renewalTimer !== null) { + clearTimeout(renewalTimer); + renewalTimer = null; + } +}; + +/** Subscribe to status changes. Pairs with {@link getSepAuthStatus}. */ +export const subscribeSepAuth = (listener: () => void) => { + listeners.add(listener); + return () => { + listeners.delete(listener); + }; +}; + +export const getSepAuthStatus = (): SepAuthStatus => status; + +/** + * Current bearer, or null once it has expired. + * + * Synchronous because `setTokenProvider` is: the transports read it while + * building a request and cannot await. An expired token yields null rather than + * a stale bearer, and the resulting 401 routes into the transports' retry, which + * mints and replays. + */ +export const getSepToken = (): string | null => + token !== null && Date.now() < expiresAtMs ? token : null; + +/** + * Renew ahead of expiry so the bearer is replaced before any request can carry + * a dead one. + * + * A backgrounded tab has its timers throttled and may miss the window; the 401 + * retry in both transports is the backstop for that. + */ +const scheduleRenewal = (expiresIn: number) => { + clearRenewalTimer(); + const delay = Math.max( + expiresIn * 1000 - EXPIRY_SKEW_MS, + MIN_RENEWAL_DELAY_MS + ); + renewalTimer = setTimeout(() => { + renewalTimer = null; + void renew(); + }, delay); +}; + +const renew = async () => { + // Joins the shared single-flight, so a renewal racing a 401 retry is one call. + const minted = await refreshAccessToken(); + if (!minted && getSepAuthStatus() !== 'signedOut') { + // Nothing replaced a bearer that is at most EXPIRY_SKEW_MS from useless. + // Drop it, but leave the status alone: demoting a mounted SEP page to an + // error screen over a background blip would discard whatever the user was + // in the middle of. The next SEP request 401s, mints, and replays — and if + // that mint is rejected too, the unauthorized path reports it properly. + clearSepToken(); + } +}; + +const clearSepToken = () => { + token = null; + expiresAtMs = 0; + clearRenewalTimer(); +}; + +/** + * Record a freshly minted bearer. Wired to `setOnRefreshed`, so it runs whoever + * triggered the exchange — the gate, the renewal timer, or a 401 retry. + */ +export const recordSepToken = (accessToken: string, expiresIn: number) => { + token = accessToken; + expiresAtMs = Date.now() + expiresIn * 1000; + scheduleRenewal(expiresIn); + setStatus('ready'); +}; + +/** + * Drop the bearer and refuse further exchanges until {@link retrySepAuth}. + * + * Wired to `setOnUnauthorized`, which fires when a SEP call 401s and no token + * could be minted to replay it. Also reached directly when the exchange itself + * 401s, so the sticky guarantee holds even if the transports' unauthorized + * wiring changes. + */ +export const markSepSignedOut = () => { + clearSepToken(); + setStatus('signedOut'); +}; + +/** + * Mint a bearer by exchanging PMM's session cookie. Wired to `setTokenMinter`, + * replacing `@sep/api`'s default `POST /oauth/refresh` — PMM's embedding issues + * no refresh cookie, so the default would 401 on every recovery attempt. + */ +export const mintSepToken = async (): Promise => { + if (status === 'signedOut') { + return null; + } + try { + return await postSessionExchange(); + } catch (error) { + if (error instanceof ApiError && error.status === 401) { + markSepSignedOut(); + } + return null; + } +}; + +/** + * Ensure a usable bearer exists, exchanging if needed. Resolves true when SEP + * calls can be authenticated. + * + * Concurrent callers coalesce inside `refreshAccessToken()`. + */ +export const ensureSepToken = async (): Promise => { + if (getSepToken() !== null) { + setStatus('ready'); + return true; + } + if (status === 'signedOut') { + return false; + } + + setStatus('exchanging'); + const minted = await refreshAccessToken(); + if (minted !== null) { + return true; + } + // Read through the getter: the awaited mint may have flipped the status to + // `signedOut`, which TypeScript cannot see through the await. + if (getSepAuthStatus() !== 'signedOut') { + setStatus('error'); + } + return false; +}; + +/** + * Clear a terminal state and exchange again. The only way out of `signedOut`, + * so retrying stays an explicit user action rather than an automatic loop. + */ +export const retrySepAuth = (): Promise => { + clearSepToken(); + setStatus('idle'); + return ensureSepToken(); +}; + +/** Reset every module-level field. Tests only. */ +export const resetSepAuthStore = () => { + clearSepToken(); + status = 'idle'; + listeners.clear(); +}; diff --git a/ui/apps/pmm/vite.config.ts b/ui/apps/pmm/vite.config.ts index 30f19a0389d..9b350667a3e 100644 --- a/ui/apps/pmm/vite.config.ts +++ b/ui/apps/pmm/vite.config.ts @@ -28,12 +28,20 @@ const target = (hasNginxCerts ? 'https://localhost:8443' : 'https://localhost'); // SEP backend. The dev server proxies SEP's API paths to it so the migrated SEP -// plugins get real data. Interim auth (Option D): if PMM_DEV_SEP_INTERNAL_TOKEN -// is set, inject it server-side as a Bearer token so no secret reaches the -// browser. Both variables are dev-server-only, hence the PMM_DEV_ prefix. -// Replaced by the token-exchange provider (Option B) later — see src/sep/bootstrap.ts. +// plugins get real data. Residual interim auth: if PMM_DEV_SEP_INTERNAL_TOKEN is +// set, inject it server-side as a Bearer token so no secret reaches the browser. +// Both variables are dev-server-only, hence the PMM_DEV_ prefix. +// +// The browser now mints its own bearer by exchanging the PMM session (see +// src/sep/bootstrap.ts), so the injection is only a fallback for a SEP instance +// whose Grafana provider is not wired up yet. It must never cover the OAuth +// routes: overwriting Authorization there would authenticate the exchange as +// SEP's internal service principal and mask whether the cookie path works at +// all. Retiring the injection entirely is a follow-up. const sepBackendUrl = env.PMM_DEV_SEP_BACKEND_URL || 'http://localhost:8000'; const sepInternalToken = env.PMM_DEV_SEP_INTERNAL_TOKEN; +const isSepAuthPath = (url: string | undefined) => + !!url && url.startsWith('/api/oauth/'); const sepProxy = () => ({ target: sepBackendUrl, secure: false, @@ -44,7 +52,10 @@ const sepProxy = () => ({ if (!sepInternalToken) { return; } - proxy.on('proxyReq', (proxyReq: unknown) => { + proxy.on('proxyReq', (proxyReq: unknown, req: unknown) => { + if (isSepAuthPath((req as { url?: string }).url)) { + return; + } (proxyReq as { setHeader: (k: string, v: string) => void }).setHeader( 'Authorization', `Bearer ${sepInternalToken}` From a275f7a9d225fc25229138b6ffc177345cf7c013 Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Thu, 6 Aug 2026 10:14:55 -0300 Subject: [PATCH 03/23] PMM-15293 Clone only replay-eligible requests `onRequest` cloned every outbound Request so a 401 could be replayed, including the minting and login endpoints that `onResponse` explicitly excludes from the retry. Cloning buffers the body, and those clones were never going to be used. Both call sites now share one `isReplayEligible` predicate, so the clone and the retry cannot drift apart. Raised by Copilot on #5739. Signed-off-by: Ignacio Durand --- ui/packages/sep/api/src/typed-client.ts | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/ui/packages/sep/api/src/typed-client.ts b/ui/packages/sep/api/src/typed-client.ts index d4bc4fc5c72..80f0abc63fb 100644 --- a/ui/packages/sep/api/src/typed-client.ts +++ b/ui/packages/sep/api/src/typed-client.ts @@ -51,6 +51,14 @@ const IS_DEV = import.meta.env.DEV; const isRefreshRequest = (url: string) => url.includes('/oauth/refresh'); const isLoginRequest = (url: string) => url.includes('/oauth/login'); +/** + * Whether a 401 on this URL is worth one silent mint-and-replay. Minting + * endpoints are the recovery mechanism itself and login carries its own + * credentials, so a 401 from either is the answer, not a stale token. + */ +const isReplayEligible = (url: string) => + !isTokenMintRequest(url) && !isLoginRequest(url); + /** * A 200 HTML response (e.g. a follow of a login redirect) means the session * is gone. The browser can't observe the 303, so content-type is the only @@ -64,6 +72,9 @@ function isHtmlLoginResponse(response: Response): boolean { // `fetch` consumes a Request's body stream, so the instance handed to // `onResponse` can no longer be re-sent. Stash an untouched clone taken before // dispatch, keyed weakly so requests that never come back are not retained. +// +// Only replay-eligible requests are cloned: cloning buffers the body, and the +// endpoints excluded from the retry would never use theirs. const pristineRequests = new WeakMap(); /** @@ -100,7 +111,9 @@ const authMiddleware: Middleware = { if (token) { request.headers.set('Authorization', `Bearer ${token}`); } - pristineRequests.set(request, request.clone()); + if (isReplayEligible(request.url)) { + pristineRequests.set(request, request.clone()); + } if (IS_DEV) { // eslint-disable-next-line no-console console.debug( @@ -125,11 +138,7 @@ const authMiddleware: Middleware = { }); } - if ( - response.status === 401 && - !isTokenMintRequest(request.url) && - !isLoginRequest(request.url) - ) { + if (response.status === 401 && isReplayEligible(request.url)) { const replayed = await replayWithFreshToken(request); if (replayed && replayed.status !== 401) { return replayed; From a2c6df1d218f33cec39c8e7711197bcb0f20b39e Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Thu, 6 Aug 2026 10:15:09 -0300 Subject: [PATCH 04/23] PMM-15293 Fail closed without discarding user work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reworks how the store reports failure, against the updated ACs. Two rules now shape it, and they pull in opposite directions. Fail closed. Every exchange failure drops the bearer, so no request can proceed on a stale, expired, or unverified credential, and there is no cached value to fall back on. A session SEP has rejected stays sticky: minting is refused outright until the user retries, so a rejection can never drive an exchange loop. Never destroy user work. The failure now lands at one of two altitudes. Before a bearer has ever been held the page does not exist yet, so a bootstrap failure takes the page over — there is nothing to preserve. Once mounted the page stays mounted and the failure becomes an inline notice beside it. Previously a background renewal being rejected moved the phase to `signedOut`, which unmounted the plugin and threw away whatever was half-typed into it. The two are reconciled by keeping the bearer and the reporting separate: `failClosed` always drops the credential, then chooses between a phase change and a notice based on whether the page is up. A renewal that fails for a reason that may not repeat is now retried quietly with backoff — 2s, 4s, 8s, 16s — and only surfaces if all four attempts fail. A 401 skips the backoff: the session is genuinely gone and retrying would only repeat the rejection, so the user is told at once, non-destructively, that submissions from this page will fail. `getSepAuthStatus()` is replaced by `getSepAuthState()`, returning a cached `{ phase, notice }` snapshot so `useSyncExternalStore` does not re-render subscribers on a no-op. The old `error` phase is renamed `unreachable`, matching the notice of the same name. Signed-off-by: Ignacio Durand --- ui/apps/pmm/src/sep/SepAuthGate.messages.ts | 24 +- ui/apps/pmm/src/sep/SepAuthGate.test.tsx | 87 +++++-- ui/apps/pmm/src/sep/SepAuthGate.tsx | 73 ++++-- ui/apps/pmm/src/sep/sepTokenStore.test.ts | 274 ++++++++++++++------ ui/apps/pmm/src/sep/sepTokenStore.ts | 207 +++++++++++---- 5 files changed, 494 insertions(+), 171 deletions(-) diff --git a/ui/apps/pmm/src/sep/SepAuthGate.messages.ts b/ui/apps/pmm/src/sep/SepAuthGate.messages.ts index b054ece3b8c..144ca5c8ab1 100644 --- a/ui/apps/pmm/src/sep/SepAuthGate.messages.ts +++ b/ui/apps/pmm/src/sep/SepAuthGate.messages.ts @@ -1,10 +1,22 @@ export const Messages = { loading: 'Authenticating with Smart Expert Platform…', - signedOutTitle: 'Not signed in', - signedOut: - 'Smart Expert Platform could not verify your PMM session. Sign in to PMM again, then retry.', - errorTitle: 'Could not reach Smart Expert Platform', - error: - 'Authenticating with Smart Expert Platform failed. This is usually temporary.', retry: 'Try again', + // Shown instead of the page: the exchange failed at load, so there is no work + // in progress to preserve. + blocked: { + signedOutTitle: 'Not signed in', + signedOut: + 'Smart Expert Platform could not verify your PMM session. Sign in to PMM again, then retry.', + unreachableTitle: 'Could not reach Smart Expert Platform', + unreachable: + 'Authenticating with Smart Expert Platform failed. This is usually temporary.', + }, + // Shown beside a page that is already open. Never replaces it — the user may + // be part-way through a form. + notice: { + signedOut: + 'Your PMM session has ended, so Smart Expert Platform can no longer be reached. Anything you submit from this page will fail. Sign in to PMM in another tab, then retry — your work here is kept.', + unreachable: + 'Lost the connection to Smart Expert Platform. Anything you submit from this page will fail until it is back. Your work here is kept.', + }, }; diff --git a/ui/apps/pmm/src/sep/SepAuthGate.test.tsx b/ui/apps/pmm/src/sep/SepAuthGate.test.tsx index d3b12940370..3dd2f8a5fa2 100644 --- a/ui/apps/pmm/src/sep/SepAuthGate.test.tsx +++ b/ui/apps/pmm/src/sep/SepAuthGate.test.tsx @@ -1,8 +1,8 @@ -import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { act, fireEvent, render, screen } from '@testing-library/react'; import { ApiError, postSessionExchange, setTokenMinter } from '@sep/api'; import { SepAuthGate } from './SepAuthGate'; import { initSepAuth } from './bootstrap'; -import { resetSepAuthStore } from './sepTokenStore'; +import { markSepSignedOut, resetSepAuthStore } from './sepTokenStore'; vi.mock('@sep/api', async (importOriginal) => ({ ...(await importOriginal()), @@ -11,6 +11,11 @@ vi.mock('@sep/api', async (importOriginal) => ({ const exchange = vi.mocked(postSessionExchange); +const bearer = (accessToken = 'bearer-1') => ({ + access_token: accessToken, + expires_in: 300, +}); + const unauthorized = () => new ApiError({ kind: 'http', status: 401, message: 'no session' }); @@ -21,6 +26,14 @@ const renderGate = () => ); +/** A page with unsaved input, standing in for a half-filled plugin form. */ +const renderGateWithForm = () => + render( + + + + ); + beforeEach(() => { exchange.mockReset(); resetSepAuthStore(); @@ -32,12 +45,9 @@ afterEach(() => { setTokenMinter(null); }); -describe('SepAuthGate', () => { +describe('SepAuthGate — bootstrap', () => { it('withholds children until the exchange resolves', async () => { - let resolveExchange: (value: { - access_token: string; - expires_in: number; - }) => void = () => {}; + let resolveExchange: (value: ReturnType) => void = () => {}; exchange.mockReturnValue( new Promise((resolve) => { resolveExchange = resolve; @@ -49,13 +59,13 @@ describe('SepAuthGate', () => { expect(screen.queryByText('plugin content')).not.toBeInTheDocument(); expect(screen.getByRole('progressbar')).toBeInTheDocument(); - resolveExchange({ access_token: 'bearer-1', expires_in: 300 }); + resolveExchange(bearer()); expect(await screen.findByText('plugin content')).toBeInTheDocument(); }); it('renders children once a bearer is held', async () => { - exchange.mockResolvedValue({ access_token: 'bearer-1', expires_in: 300 }); + exchange.mockResolvedValue(bearer()); renderGate(); @@ -63,7 +73,7 @@ describe('SepAuthGate', () => { expect(exchange).toHaveBeenCalledOnce(); }); - it('reports a rejected session instead of looping on the exchange', async () => { + it('shows a signed-out page instead of the plugin, and does not loop', async () => { exchange.mockRejectedValue(unauthorized()); renderGate(); @@ -72,30 +82,69 @@ describe('SepAuthGate', () => { 'Not signed in' ); expect(screen.queryByText('plugin content')).not.toBeInTheDocument(); - // Waiting past any plausible retry delay: the failure must stay put. - await waitFor(() => expect(exchange).toHaveBeenCalledOnce()); expect(exchange).toHaveBeenCalledOnce(); }); + it('distinguishes an unreachable SEP from a rejected session', async () => { + exchange.mockRejectedValue(new Error('network down')); + + renderGate(); + + expect(await screen.findByTestId('sep-auth-error')).toHaveTextContent( + 'Could not reach Smart Expert Platform' + ); + }); + it('exchanges again when the user retries', async () => { exchange.mockRejectedValue(unauthorized()); renderGate(); await screen.findByTestId('sep-auth-error'); - exchange.mockResolvedValue({ access_token: 'bearer-1', expires_in: 300 }); + exchange.mockResolvedValue(bearer()); fireEvent.click(screen.getByRole('button', { name: 'Try again' })); expect(await screen.findByText('plugin content')).toBeInTheDocument(); expect(exchange).toHaveBeenCalledTimes(2); }); +}); - it('distinguishes a transient failure from a rejected session', async () => { - exchange.mockRejectedValue(new Error('network down')); - +describe('SepAuthGate — failure on a mounted page', () => { + it('reports a rejected session without unmounting the page', async () => { + exchange.mockResolvedValue(bearer()); renderGate(); + await screen.findByText('plugin content'); - expect(await screen.findByTestId('sep-auth-error')).toHaveTextContent( - 'Could not reach Smart Expert Platform' - ); + act(() => markSepSignedOut()); + + expect(screen.getByTestId('sep-auth-notice')).toBeInTheDocument(); + expect(screen.getByText('plugin content')).toBeInTheDocument(); + expect(screen.queryByTestId('sep-auth-error')).not.toBeInTheDocument(); + }); + + it('preserves in-progress form state', async () => { + exchange.mockResolvedValue(bearer()); + renderGateWithForm(); + const field = await screen.findByLabelText('target'); + fireEvent.change(field, { target: { value: 'half-written command' } }); + + act(() => markSepSignedOut()); + + expect(screen.getByTestId('sep-auth-notice')).toBeInTheDocument(); + expect(screen.getByLabelText('target')).toHaveValue('half-written command'); + }); + + it('clears the notice when the retry succeeds, keeping the page throughout', async () => { + exchange.mockResolvedValue(bearer()); + renderGateWithForm(); + const field = await screen.findByLabelText('target'); + fireEvent.change(field, { target: { value: 'half-written command' } }); + act(() => markSepSignedOut()); + + exchange.mockResolvedValue(bearer('bearer-2')); + fireEvent.click(screen.getByRole('button', { name: 'Try again' })); + + await screen.findByLabelText('target'); + expect(screen.queryByTestId('sep-auth-notice')).not.toBeInTheDocument(); + expect(screen.getByLabelText('target')).toHaveValue('half-written command'); }); }); diff --git a/ui/apps/pmm/src/sep/SepAuthGate.tsx b/ui/apps/pmm/src/sep/SepAuthGate.tsx index b040f1ba60d..868ac2fe5fa 100644 --- a/ui/apps/pmm/src/sep/SepAuthGate.tsx +++ b/ui/apps/pmm/src/sep/SepAuthGate.tsx @@ -8,12 +8,44 @@ import { } from '@mui/material'; import { Messages } from './SepAuthGate.messages'; import { + type SepAuthNotice, ensureSepToken, - getSepAuthStatus, + getSepAuthState, retrySepAuth, subscribeSepAuth, } from './sepTokenStore'; +const RetryButton: FC = () => ( + +); + +/** + * Inline report of a failure that arrived after the page was already open. + * + * Deliberately not a replacement for the page: a background renewal failing + * must not discard a half-filled form. It tells the user that submitting will + * fail and offers a retry, and leaves everything else alone. + */ +const SepAuthNoticeBar: FC<{ kind: SepAuthNotice }> = ({ kind }) => ( + } + > + {kind === 'signedOut' + ? Messages.notice.signedOut + : Messages.notice.unreachable} + +); + /** * Holds a SEP route until a SEP bearer has been minted from the PMM session. * @@ -26,9 +58,15 @@ import { * synchronous, so a plugin's first queries would otherwise fire before the * exchange resolves and 401 on arrival. Children do not render until a bearer * is in hand. + * + * Once they have rendered they stay rendered. A later failure is reported by + * `notice`, beside the page rather than instead of it. */ export const SepAuthGate: FC = ({ children }) => { - const status = useSyncExternalStore(subscribeSepAuth, getSepAuthStatus); + const { phase, notice } = useSyncExternalStore( + subscribeSepAuth, + getSepAuthState + ); useEffect(() => { // No-ops when a bearer is already held or the session was rejected; a @@ -36,32 +74,29 @@ export const SepAuthGate: FC = ({ children }) => { void ensureSepToken(); }, []); - if (status === 'ready') { - return <>{children}; + if (phase === 'ready') { + return ( + <> + {notice !== null && } + {children} + + ); } - if (status === 'signedOut' || status === 'error') { - const signedOut = status === 'signedOut'; + if (phase === 'signedOut' || phase === 'unreachable') { + const signedOut = phase === 'signedOut'; return ( { - void retrySepAuth(); - }} - > - {Messages.retry} - - } + action={} > - {signedOut ? Messages.signedOutTitle : Messages.errorTitle} + {signedOut + ? Messages.blocked.signedOutTitle + : Messages.blocked.unreachableTitle} - {signedOut ? Messages.signedOut : Messages.error} + {signedOut ? Messages.blocked.signedOut : Messages.blocked.unreachable} ); } diff --git a/ui/apps/pmm/src/sep/sepTokenStore.test.ts b/ui/apps/pmm/src/sep/sepTokenStore.test.ts index ef61df45c09..33a0d158432 100644 --- a/ui/apps/pmm/src/sep/sepTokenStore.test.ts +++ b/ui/apps/pmm/src/sep/sepTokenStore.test.ts @@ -7,7 +7,7 @@ import { import { initSepAuth } from './bootstrap'; import { ensureSepToken, - getSepAuthStatus, + getSepAuthState, getSepToken, resetSepAuthStore, retrySepAuth, @@ -24,6 +24,11 @@ vi.mock('@sep/api', async (importOriginal) => ({ const exchange = vi.mocked(postSessionExchange); const TTL_SECONDS = 300; +/** The renewal fires 30s before the 300s TTL. */ +const UNTIL_RENEWAL_MS = 270_000; +/** Backoff is 2s, 4s, 8s, 16s; this clears all four plus slack. */ +const PAST_ALL_RETRIES_MS = 60_000; + const mintedToken = (accessToken: string) => ({ access_token: accessToken, expires_in: TTL_SECONDS, @@ -32,6 +37,16 @@ const mintedToken = (accessToken: string) => ({ const unauthorized = () => new ApiError({ kind: 'http', status: 401, message: 'no session' }); +const phase = () => getSepAuthState().phase; +const notice = () => getSepAuthState().notice; + +/** Reach `ready` with a live bearer, as a mounted SEP page would be. */ +const becomeReady = async (accessToken = 'bearer-1') => { + exchange.mockResolvedValue(mintedToken(accessToken)); + await ensureSepToken(); + exchange.mockReset(); +}; + beforeEach(() => { // Leave `queueMicrotask` real: `refreshAccessToken` clears its single-flight // slot in a microtask, and faking that would deadlock the second exchange. @@ -50,7 +65,7 @@ afterEach(() => { describe('sepTokenStore — acquiring a bearer', () => { it('holds no token until an exchange runs', () => { expect(getSepToken()).toBeNull(); - expect(getSepAuthStatus()).toBe('idle'); + expect(phase()).toBe('idle'); expect(exchange).not.toHaveBeenCalled(); }); @@ -61,13 +76,11 @@ describe('sepTokenStore — acquiring a bearer', () => { expect(exchange).toHaveBeenCalledOnce(); expect(getSepToken()).toBe('bearer-1'); - expect(getSepAuthStatus()).toBe('ready'); + expect(getSepAuthState()).toEqual({ phase: 'ready', notice: null }); }); it('serves the bearer through the token provider registered on @sep/api', async () => { - exchange.mockResolvedValue(mintedToken('bearer-1')); - - await ensureSepToken(); + await becomeReady(); expect(getToken()).toBe('bearer-1'); }); @@ -103,107 +116,88 @@ describe('sepTokenStore — acquiring a bearer', () => { }); it('never writes the bearer to web storage', async () => { - exchange.mockResolvedValue(mintedToken('bearer-1')); - - await ensureSepToken(); + await becomeReady(); expect(Object.keys(localStorage)).toHaveLength(0); expect(Object.keys(sessionStorage)).toHaveLength(0); }); -}); -describe('sepTokenStore — lifetime', () => { - it('stops serving the bearer once it has expired', async () => { - exchange.mockResolvedValue(mintedToken('bearer-1')); - await ensureSepToken(); + it('hands out a stable snapshot so subscribers do not re-render on no-ops', async () => { + await becomeReady(); + const first = getSepAuthState(); - vi.setSystemTime(Date.now() + TTL_SECONDS * 1000 + 1); + await ensureSepToken(); - expect(getSepToken()).toBeNull(); + expect(getSepAuthState()).toBe(first); }); +}); - it('re-exchanges shortly before expiry', async () => { - exchange.mockResolvedValue(mintedToken('bearer-1')); - await ensureSepToken(); - exchange.mockResolvedValue(mintedToken('bearer-2')); +describe('sepTokenStore — failing closed', () => { + it('serves no token once the bearer has expired', async () => { + await becomeReady(); - // 30s of skew ahead of the 300s TTL. - await vi.advanceTimersByTimeAsync(270_000); + vi.setSystemTime(Date.now() + TTL_SECONDS * 1000 + 1); - expect(exchange).toHaveBeenCalledTimes(2); - expect(getSepToken()).toBe('bearer-2'); - expect(getSepAuthStatus()).toBe('ready'); + expect(getSepToken()).toBeNull(); + expect(getToken()).toBeNull(); }); - it('keeps renewing across successive lifetimes', async () => { - exchange.mockResolvedValue(mintedToken('bearer-1')); - await ensureSepToken(); - exchange.mockResolvedValue(mintedToken('bearer-2')); - await vi.advanceTimersByTimeAsync(270_000); - exchange.mockResolvedValue(mintedToken('bearer-3')); - await vi.advanceTimersByTimeAsync(270_000); + it('drops the bearer when a renewal is rejected', async () => { + await becomeReady(); + exchange.mockRejectedValue(unauthorized()); - expect(exchange).toHaveBeenCalledTimes(3); - expect(getSepToken()).toBe('bearer-3'); + await vi.advanceTimersByTimeAsync(UNTIL_RENEWAL_MS); + + expect(getSepToken()).toBeNull(); }); - it('drops the bearer on a failed renewal without tearing down the page', async () => { - exchange.mockResolvedValue(mintedToken('bearer-1')); - await ensureSepToken(); + it('drops the bearer when a renewal cannot complete', async () => { + await becomeReady(); exchange.mockRejectedValue(new Error('offline')); - await vi.advanceTimersByTimeAsync(270_000); + await vi.advanceTimersByTimeAsync(UNTIL_RENEWAL_MS); expect(getSepToken()).toBeNull(); - // Still `ready`, so a mounted SEP page keeps its state; the next request - // 401s and mints through the transports' retry. - expect(getSepAuthStatus()).toBe('ready'); }); - it('re-acquires on the next visit after a failed renewal', async () => { - exchange.mockResolvedValue(mintedToken('bearer-1')); + it('refuses to exchange again once the session is rejected', async () => { + exchange.mockRejectedValue(unauthorized()); await ensureSepToken(); - exchange.mockRejectedValue(new Error('offline')); - await vi.advanceTimersByTimeAsync(270_000); - exchange.mockResolvedValue(mintedToken('bearer-2')); - await expect(ensureSepToken()).resolves.toBe(true); + await expect(ensureSepToken()).resolves.toBe(false); + await expect(ensureSepToken()).resolves.toBe(false); - expect(getSepToken()).toBe('bearer-2'); + expect(exchange).toHaveBeenCalledOnce(); }); - it('stops renewing once the store is cleared', async () => { - exchange.mockResolvedValue(mintedToken('bearer-1')); + it('stops renewing after the session is rejected', async () => { + exchange.mockRejectedValue(unauthorized()); await ensureSepToken(); - resetSepAuthStore(); - await vi.advanceTimersByTimeAsync(270_000); + await vi.advanceTimersByTimeAsync(600_000); expect(exchange).toHaveBeenCalledOnce(); }); }); -describe('sepTokenStore — rejected session', () => { - it('treats a 401 from the exchange as signed out', async () => { +describe('sepTokenStore — bootstrap failure', () => { + it('shows a signed-out page when the session is rejected at load', async () => { exchange.mockRejectedValue(unauthorized()); await expect(ensureSepToken()).resolves.toBe(false); - expect(getSepAuthStatus()).toBe('signedOut'); - expect(getSepToken()).toBeNull(); + expect(getSepAuthState()).toEqual({ phase: 'signedOut', notice: null }); }); - it('refuses to exchange again while signed out', async () => { - exchange.mockRejectedValue(unauthorized()); - await ensureSepToken(); + it('shows an unreachable page when the exchange cannot complete at load', async () => { + exchange.mockRejectedValue(new Error('network down')); - await expect(ensureSepToken()).resolves.toBe(false); await expect(ensureSepToken()).resolves.toBe(false); - expect(exchange).toHaveBeenCalledOnce(); + expect(getSepAuthState()).toEqual({ phase: 'unreachable', notice: null }); }); - it('exchanges again only when the user explicitly retries', async () => { + it('recovers on an explicit retry', async () => { exchange.mockRejectedValue(unauthorized()); await ensureSepToken(); exchange.mockResolvedValue(mintedToken('bearer-1')); @@ -211,36 +205,160 @@ describe('sepTokenStore — rejected session', () => { await expect(retrySepAuth()).resolves.toBe(true); expect(exchange).toHaveBeenCalledTimes(2); - expect(getSepToken()).toBe('bearer-1'); + expect(getSepAuthState()).toEqual({ phase: 'ready', notice: null }); }); - it('does not schedule a renewal after a rejected session', async () => { - exchange.mockRejectedValue(unauthorized()); + it('retries a transient bootstrap failure on the next visit', async () => { + exchange.mockRejectedValue(new Error('network down')); await ensureSepToken(); + exchange.mockResolvedValue(mintedToken('bearer-1')); - await vi.advanceTimersByTimeAsync(600_000); + await expect(ensureSepToken()).resolves.toBe(true); - expect(exchange).toHaveBeenCalledOnce(); + expect(getSepToken()).toBe('bearer-1'); }); }); -describe('sepTokenStore — transient failure', () => { - it('reports an error without going sticky', async () => { - exchange.mockRejectedValue(new Error('network down')); +describe('sepTokenStore — renewal on a mounted page', () => { + it('renews shortly before expiry', async () => { + await becomeReady(); + exchange.mockResolvedValue(mintedToken('bearer-2')); - await expect(ensureSepToken()).resolves.toBe(false); + await vi.advanceTimersByTimeAsync(UNTIL_RENEWAL_MS); - expect(getSepAuthStatus()).toBe('error'); + expect(exchange).toHaveBeenCalledOnce(); + expect(getSepToken()).toBe('bearer-2'); + expect(getSepAuthState()).toEqual({ phase: 'ready', notice: null }); }); - it('retries on the next attempt', async () => { - exchange.mockRejectedValue(new Error('network down')); - await ensureSepToken(); - exchange.mockResolvedValue(mintedToken('bearer-1')); + it('keeps renewing across successive lifetimes', async () => { + await becomeReady(); + exchange.mockResolvedValue(mintedToken('bearer-2')); + await vi.advanceTimersByTimeAsync(UNTIL_RENEWAL_MS); + exchange.mockResolvedValue(mintedToken('bearer-3')); + await vi.advanceTimersByTimeAsync(UNTIL_RENEWAL_MS); - await expect(ensureSepToken()).resolves.toBe(true); + expect(exchange).toHaveBeenCalledTimes(2); + expect(getSepToken()).toBe('bearer-3'); + }); + it('retries a transient renewal failure quietly, without a notice', async () => { + await becomeReady(); + exchange.mockRejectedValue(new Error('offline')); + + await vi.advanceTimersByTimeAsync(UNTIL_RENEWAL_MS); + + expect(exchange).toHaveBeenCalledOnce(); + // Still `ready` with nothing on screen: a blip must not interrupt the user. + expect(getSepAuthState()).toEqual({ phase: 'ready', notice: null }); + }); + + it('backs off across several quiet attempts before giving up', async () => { + await becomeReady(); + exchange.mockRejectedValue(new Error('offline')); + await vi.advanceTimersByTimeAsync(UNTIL_RENEWAL_MS); + + await vi.advanceTimersByTimeAsync(2_000); expect(exchange).toHaveBeenCalledTimes(2); - expect(getSepToken()).toBe('bearer-1'); + expect(notice()).toBeNull(); + + await vi.advanceTimersByTimeAsync(4_000); + expect(exchange).toHaveBeenCalledTimes(3); + expect(notice()).toBeNull(); + + await vi.advanceTimersByTimeAsync(8_000); + expect(exchange).toHaveBeenCalledTimes(4); + expect(notice()).toBeNull(); + }); + + it('surfaces a transient failure only once it persists', async () => { + await becomeReady(); + exchange.mockRejectedValue(new Error('offline')); + + await vi.advanceTimersByTimeAsync(UNTIL_RENEWAL_MS + PAST_ALL_RETRIES_MS); + + expect(exchange).toHaveBeenCalledTimes(1 + 4); + // Reported beside the page, never instead of it. + expect(getSepAuthState()).toEqual({ + phase: 'ready', + notice: 'unreachable', + }); + }); + + it('recovers silently when a backoff attempt succeeds', async () => { + await becomeReady(); + exchange.mockRejectedValueOnce(new Error('offline')); + exchange.mockResolvedValue(mintedToken('bearer-2')); + + await vi.advanceTimersByTimeAsync(UNTIL_RENEWAL_MS + 2_000); + + expect(getSepToken()).toBe('bearer-2'); + expect(getSepAuthState()).toEqual({ phase: 'ready', notice: null }); + }); + + it('stops retrying once a backoff attempt succeeds', async () => { + await becomeReady(); + exchange.mockRejectedValueOnce(new Error('offline')); + exchange.mockResolvedValue(mintedToken('bearer-2')); + await vi.advanceTimersByTimeAsync(UNTIL_RENEWAL_MS + 2_000); + + // Only the next scheduled renewal should fire, not a leftover backoff. + await vi.advanceTimersByTimeAsync(1_000); + + expect(exchange).toHaveBeenCalledTimes(2); + }); + + it('reports a rejected session at once, without backing off', async () => { + await becomeReady(); + exchange.mockRejectedValue(unauthorized()); + + await vi.advanceTimersByTimeAsync(UNTIL_RENEWAL_MS); + + expect(getSepAuthState()).toEqual({ phase: 'ready', notice: 'signedOut' }); + // Terminal: retrying would only repeat the rejection. + await vi.advanceTimersByTimeAsync(PAST_ALL_RETRIES_MS); + expect(exchange).toHaveBeenCalledOnce(); + }); + + it('never leaves the ready phase, whatever the failure', async () => { + await becomeReady(); + exchange.mockRejectedValue(unauthorized()); + + await vi.advanceTimersByTimeAsync(UNTIL_RENEWAL_MS + PAST_ALL_RETRIES_MS); + + expect(phase()).toBe('ready'); + }); + + it('clears the notice when the user retries successfully', async () => { + await becomeReady(); + exchange.mockRejectedValue(unauthorized()); + await vi.advanceTimersByTimeAsync(UNTIL_RENEWAL_MS); + expect(notice()).toBe('signedOut'); + + exchange.mockResolvedValue(mintedToken('bearer-2')); + await expect(retrySepAuth()).resolves.toBe(true); + + expect(getSepAuthState()).toEqual({ phase: 'ready', notice: null }); + expect(getSepToken()).toBe('bearer-2'); + }); + + it('keeps the notice when the retry fails again', async () => { + await becomeReady(); + exchange.mockRejectedValue(unauthorized()); + await vi.advanceTimersByTimeAsync(UNTIL_RENEWAL_MS); + + await expect(retrySepAuth()).resolves.toBe(false); + + expect(getSepAuthState()).toEqual({ phase: 'ready', notice: 'signedOut' }); + }); + + it('stops renewing once the store is cleared', async () => { + await becomeReady(); + exchange.mockResolvedValue(mintedToken('bearer-2')); + + resetSepAuthStore(); + await vi.advanceTimersByTimeAsync(UNTIL_RENEWAL_MS); + + expect(exchange).not.toHaveBeenCalled(); }); }); diff --git a/ui/apps/pmm/src/sep/sepTokenStore.ts b/ui/apps/pmm/src/sep/sepTokenStore.ts index c56fec6a268..ea414a40515 100644 --- a/ui/apps/pmm/src/sep/sepTokenStore.ts +++ b/ui/apps/pmm/src/sep/sepTokenStore.ts @@ -18,6 +18,19 @@ import { * every exchange re-reads the identity, so a role change lands within one bearer * lifetime (5 minutes by default). * + * Two rules shape everything below. + * + * **Fail closed.** Any exchange failure drops the bearer immediately. Nothing + * ever proceeds on a stale, expired, or unverified credential, and there is no + * cached fallback to reach for. A session SEP has rejected is sticky: minting is + * refused until the user retries, so a rejection cannot drive an exchange loop. + * + * **Never destroy user work.** Once a bearer has been held, the page is mounted + * and may hold a half-filled form. From that point a failure is reported through + * {@link SepAuthState.notice} — an inline notice beside the still-mounted page — + * rather than by moving the phase to a full-screen state. Before that point + * there is nothing to preserve, so a bootstrap failure takes over the page. + * * Concurrency is not handled here. `refreshAccessToken()` in `@sep/api` * single-flights every caller — the renewal timer, the initial gate, and each * transport's 401 retry — so a burst of parallel SEP requests triggers one @@ -33,45 +46,81 @@ const EXPIRY_SKEW_MS = 30_000; /** Floor for the renewal delay, in case SEP ever issues a very short TTL. */ const MIN_RENEWAL_DELAY_MS = 5_000; -export type SepAuthStatus = +/** + * Backoff for a renewal that failed for a reason that may not repeat. Quiet + * while it retries; the user is only told once the attempts run out. + */ +const RENEWAL_RETRY_BASE_MS = 2_000; +const RENEWAL_RETRY_MAX_MS = 30_000; +const MAX_RENEWAL_RETRIES = 4; + +/** What the page as a whole is doing. Drives which UI the gate renders. */ +export type SepAuthPhase = /** No exchange attempted yet. */ | 'idle' /** First exchange in flight; nothing to authenticate with yet. */ | 'exchanging' - /** A usable bearer is held. */ + /** A bearer has been held. The page is mounted and stays mounted. */ | 'ready' - /** - * SEP rejected the session. Sticky: minting is refused until - * {@link retrySepAuth} clears it, so a rejected session cannot drive an - * exchange loop. - */ + /** SEP rejected the session before a bearer was ever held. */ | 'signedOut' - /** The exchange failed for a reason that may not repeat (network, 5xx). */ - | 'error'; + /** The exchange could not be completed before a bearer was ever held. */ + | 'unreachable'; + +/** + * A failure that arrived after the page was already mounted. Surfaced beside + * the page instead of replacing it, so in-progress work survives. + */ +export type SepAuthNotice = 'signedOut' | 'unreachable'; + +export interface SepAuthState { + phase: SepAuthPhase; + notice: SepAuthNotice | null; +} let token: string | null = null; let expiresAtMs = 0; -let status: SepAuthStatus = 'idle'; +let phase: SepAuthPhase = 'idle'; +let notice: SepAuthNotice | null = null; + +/** + * Sticky once SEP has rejected the session. Blocks minting outright — without + * it, every subsequent request would 401, trigger a mint, be rejected, and + * repeat. Only {@link retrySepAuth} clears it. + */ +let sessionRejected = false; + let renewalTimer: ReturnType | null = null; +let retryTimer: ReturnType | null = null; +let renewalRetries = 0; const listeners = new Set<() => void>(); -const setStatus = (next: SepAuthStatus) => { - if (status === next) { +// `useSyncExternalStore` compares snapshots by identity, so hand out a cached +// object and only replace it when something actually changed. +let snapshot: SepAuthState = { phase, notice }; + +const publish = () => { + if (snapshot.phase === phase && snapshot.notice === notice) { return; } - status = next; + snapshot = { phase, notice }; listeners.forEach((listener) => listener()); }; -const clearRenewalTimer = () => { - if (renewalTimer !== null) { - clearTimeout(renewalTimer); - renewalTimer = null; +const setPhase = (next: SepAuthPhase) => { + phase = next; + publish(); +}; + +const clearTimer = (timer: ReturnType | null) => { + if (timer !== null) { + clearTimeout(timer); } + return null; }; -/** Subscribe to status changes. Pairs with {@link getSepAuthStatus}. */ +/** Subscribe to state changes. Pairs with {@link getSepAuthState}. */ export const subscribeSepAuth = (listener: () => void) => { listeners.add(listener); return () => { @@ -79,7 +128,7 @@ export const subscribeSepAuth = (listener: () => void) => { }; }; -export const getSepAuthStatus = (): SepAuthStatus => status; +export const getSepAuthState = (): SepAuthState => snapshot; /** * Current bearer, or null once it has expired. @@ -92,6 +141,33 @@ export const getSepAuthStatus = (): SepAuthStatus => status; export const getSepToken = (): string | null => token !== null && Date.now() < expiresAtMs ? token : null; +/** Drop the bearer and stop every pending renewal. */ +const clearSepToken = () => { + token = null; + expiresAtMs = 0; + renewalTimer = clearTimer(renewalTimer); + retryTimer = clearTimer(retryTimer); +}; + +/** + * Drop the bearer and report the failure at the right altitude. + * + * Before a bearer has ever been held there is no work in progress, so the + * failure takes over the page. After that the page stays exactly as it is and + * the failure becomes an inline notice — a background renewal must never + * discard what the user was typing. + */ +const failClosed = (kind: SepAuthNotice) => { + clearSepToken(); + if (phase === 'ready') { + notice = kind; + } else { + phase = kind; + notice = null; + } + publish(); +}; + /** * Renew ahead of expiry so the bearer is replaced before any request can carry * a dead one. @@ -100,7 +176,7 @@ export const getSepToken = (): string | null => * retry in both transports is the backstop for that. */ const scheduleRenewal = (expiresIn: number) => { - clearRenewalTimer(); + renewalTimer = clearTimer(renewalTimer); const delay = Math.max( expiresIn * 1000 - EXPIRY_SKEW_MS, MIN_RENEWAL_DELAY_MS @@ -111,23 +187,39 @@ const scheduleRenewal = (expiresIn: number) => { }, delay); }; +const scheduleRenewalRetry = () => { + retryTimer = clearTimer(retryTimer); + const delay = Math.min( + RENEWAL_RETRY_BASE_MS * 2 ** (renewalRetries - 1), + RENEWAL_RETRY_MAX_MS + ); + retryTimer = setTimeout(() => { + retryTimer = null; + void renew(); + }, delay); +}; + const renew = async () => { // Joins the shared single-flight, so a renewal racing a 401 retry is one call. const minted = await refreshAccessToken(); - if (!minted && getSepAuthStatus() !== 'signedOut') { - // Nothing replaced a bearer that is at most EXPIRY_SKEW_MS from useless. - // Drop it, but leave the status alone: demoting a mounted SEP page to an - // error screen over a background blip would discard whatever the user was - // in the middle of. The next SEP request 401s, mints, and replays — and if - // that mint is rejected too, the unauthorized path reports it properly. - clearSepToken(); + if (minted !== null) { + return; + } + if (sessionRejected) { + // A rejected session is terminal and `markSepSignedOut` already reported it. + // Retrying would only repeat the rejection. + return; } -}; -const clearSepToken = () => { - token = null; - expiresAtMs = 0; - clearRenewalTimer(); + // Transient: the bearer is gone either way (fail closed), but keep quiet and + // back off — a blip should not put a notice in front of someone mid-form. + clearSepToken(); + if (renewalRetries < MAX_RENEWAL_RETRIES) { + renewalRetries += 1; + scheduleRenewalRetry(); + return; + } + failClosed('unreachable'); }; /** @@ -137,21 +229,29 @@ const clearSepToken = () => { export const recordSepToken = (accessToken: string, expiresIn: number) => { token = accessToken; expiresAtMs = Date.now() + expiresIn * 1000; + // A successful exchange proves the session is good and clears whatever the + // last failure said about it. + sessionRejected = false; + renewalRetries = 0; + retryTimer = clearTimer(retryTimer); scheduleRenewal(expiresIn); - setStatus('ready'); + phase = 'ready'; + notice = null; + publish(); }; /** - * Drop the bearer and refuse further exchanges until {@link retrySepAuth}. + * Record that SEP rejected the session, and refuse to exchange again until + * {@link retrySepAuth}. * * Wired to `setOnUnauthorized`, which fires when a SEP call 401s and no token - * could be minted to replay it. Also reached directly when the exchange itself + * could be minted to replay it. Also called directly when the exchange itself * 401s, so the sticky guarantee holds even if the transports' unauthorized * wiring changes. */ export const markSepSignedOut = () => { - clearSepToken(); - setStatus('signedOut'); + sessionRejected = true; + failClosed('signedOut'); }; /** @@ -160,7 +260,7 @@ export const markSepSignedOut = () => { * no refresh cookie, so the default would 401 on every recovery attempt. */ export const mintSepToken = async (): Promise => { - if (status === 'signedOut') { + if (sessionRejected) { return null; } try { @@ -181,39 +281,48 @@ export const mintSepToken = async (): Promise => { */ export const ensureSepToken = async (): Promise => { if (getSepToken() !== null) { - setStatus('ready'); return true; } - if (status === 'signedOut') { + if (sessionRejected) { return false; } - setStatus('exchanging'); + // Only show the spinner before the page exists. Once mounted it stays put. + if (phase !== 'ready') { + setPhase('exchanging'); + } + const minted = await refreshAccessToken(); if (minted !== null) { return true; } - // Read through the getter: the awaited mint may have flipped the status to - // `signedOut`, which TypeScript cannot see through the await. - if (getSepAuthStatus() !== 'signedOut') { - setStatus('error'); + if (!sessionRejected) { + failClosed('unreachable'); } return false; }; /** - * Clear a terminal state and exchange again. The only way out of `signedOut`, - * so retrying stays an explicit user action rather than an automatic loop. + * Clear a terminal state and exchange again. The only way out of a rejected + * session, so recovery stays an explicit user action rather than a loop. */ export const retrySepAuth = (): Promise => { + sessionRejected = false; + renewalRetries = 0; clearSepToken(); - setStatus('idle'); + if (phase !== 'ready') { + setPhase('idle'); + } return ensureSepToken(); }; /** Reset every module-level field. Tests only. */ export const resetSepAuthStore = () => { clearSepToken(); - status = 'idle'; + phase = 'idle'; + notice = null; + sessionRejected = false; + renewalRetries = 0; + snapshot = { phase, notice }; listeners.clear(); }; From ec3d246a5a06ea685a862945cdfb488c9a037d93 Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Mon, 10 Aug 2026 19:21:50 -0300 Subject: [PATCH 05/23] PMM-15293 Let the dev proxy strip the SEP prefix The proxy forwarded `/sep` unstripped on the grounds that SEP serves the prefix itself via `root_path`. It does not: SEP carries no root_path support at all - no flag, no setting, no `FastAPI(root_path=...)`, and none on the shipped side-car's `python -m app.sep.main`. So both ways of running it locally answer 404 to everything the proxy forwards. `python -m app.main` serves at `/api/...`, and `uvicorn --root-path /sep` prepends root_path to the path, so it sees `/sep/sep/...` instead. PMM_DEV_SEP_STRIP_PREFIX=1 strips the prefix on the way out, which makes the uvicorn form work while keeping `url_for()` links prefixed. It stays off by default: the right default belongs to the server-side nginx location, which does not exist in this repo yet. The internal-token guard has to match both the prefixed and stripped forms. Vite applies `rewrite` by mutating `req.url` before the proxyReq handler runs, so with the strip enabled the old prefix-only test stopped matching and would have injected the service-principal token onto the OAuth routes it must never cover - masking whether the session exchange works at all. Signed-off-by: Ignacio Durand --- ui/apps/pmm/vite.config.ts | 40 ++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/ui/apps/pmm/vite.config.ts b/ui/apps/pmm/vite.config.ts index 622871ef05c..baef9f71c70 100644 --- a/ui/apps/pmm/vite.config.ts +++ b/ui/apps/pmm/vite.config.ts @@ -30,9 +30,9 @@ const target = // SEP backend. The dev server proxies SEP's single `/sep` mount point to it so // the migrated SEP plugins get real data, mirroring the shipped topology where // pmm-server's nginx exposes the side-car under that one location (see -// SEP_BASE_PATH in @sep/api). The prefix is passed through unstripped, because -// SEP serves it itself via `root_path` — so PMM_DEV_SEP_BACKEND_URL has to point -// at a backend configured the same way. +// SEP_BASE_PATH in @sep/api). The prefix is forwarded unstripped by default, +// for a SEP that serves it itself — so PMM_DEV_SEP_BACKEND_URL has to point at a +// backend configured that way, or PMM_DEV_SEP_STRIP_PREFIX has to be set (below). // // Residual interim auth: if PMM_DEV_SEP_INTERNAL_TOKEN is set, inject it // server-side as a Bearer token so no secret reaches the browser. Both variables @@ -50,14 +50,42 @@ const target = const SEP_BASE_PATH = '/sep'; const sepBackendUrl = env.PMM_DEV_SEP_BACKEND_URL || 'http://localhost:8000'; const sepInternalToken = env.PMM_DEV_SEP_INTERNAL_TOKEN; -// Matched against the proxied request URL, which still carries the `/sep` -// prefix — it is forwarded unstripped. +// Escape hatch for a dev SEP that does not serve the `/sep` prefix itself. +// Setting PMM_DEV_SEP_STRIP_PREFIX=1 strips it on the way out. SEP carries no +// `root_path` support today — not a flag, not a setting, not on the shipped +// side-car's `python -m app.sep.main` — so in practice both ways of running it +// locally need this: +// +// - `python -m app.main` serves its routes at `/api/…` and 404s the prefix. +// - `uvicorn --root-path /sep` *prepends* root_path to the request path +// (uvicorn `h11_impl.py`: `full_path = root_path + path`) and so expects +// the proxy to forward the tail. Left unstripped, SEP sees `/sep/sep/…` +// and 404s. This is the setup that also keeps `url_for()` emitting +// correctly prefixed links, so it is the closer match to the shipped +// topology. +// +// Off by default, because the default belongs to whatever the server-side +// nginx location ends up doing — which does not exist in this repo yet. Against +// the first setup it is lossy: with no root_path, SEP's `request.url_for()` +// emits prefix-less absolute URLs inside JSON payloads, so any link it hands +// back (file downloads in particular) escapes the mount point. Fine for +// surfaces that return plain data. +const sepStripPrefix = env.PMM_DEV_SEP_STRIP_PREFIX === '1'; +// Matched against the proxied request URL. Vite applies `rewrite` to `req.url` +// before the proxy sees it, so with the strip enabled the prefix is already +// gone by the time this runs — both forms have to match, or the token would be +// injected onto the OAuth routes it must never cover. const isSepAuthPath = (url: string | undefined) => - !!url && url.startsWith(`${SEP_BASE_PATH}/api/oauth/`); + !!url && + (url.startsWith(`${SEP_BASE_PATH}/api/oauth/`) || + url.startsWith('/api/oauth/')); const sepProxy = () => ({ target: sepBackendUrl, secure: false, changeOrigin: true, + ...(sepStripPrefix + ? { rewrite: (path: string) => path.slice(SEP_BASE_PATH.length) || '/' } + : {}), configure: (proxy: { on: (e: string, cb: (...a: unknown[]) => void) => void; }) => { From 1e5aa9db92a53b6ecebce613c5236f4b07e58066 Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Mon, 10 Aug 2026 18:46:54 -0300 Subject: [PATCH 06/23] PMM-15294 Submit ServiceNow inputs to SEP settings Add a "ServiceNow connection" tab to PMM Settings so an admin can enter the receiver endpoint and the delivery plan's named secrets, and have PMM write them to SEP's settings API. The operator obtains the token out of band; PMM-15218 replaces this entry surface with a guided round trip and leaves the write path below untouched. The write is one whole-object PATCH of DIAGNOSTICS_DELIVERY_INPUTS. SEP seals the key's leaves, so a per-leaf write is not a shape the UI may improvise, and the submitted secret map must match the declared names exactly. Those names are read at runtime from the baked plan (SEPSettings -> DIAGNOSTICS_DELIVERY -> value.secrets) rather than hardcoded, so an image that renames one is followed rather than 422'd. Secrets are addressed by position, not by name: react-hook-form reads a field name as a path, and a declared name carrying a "." would register as a nested field, read back undefined, and silently overwrite a stored secret with an empty string. Stored secrets come back masked and are resubmitted verbatim so SEP restores them, except where no override exists to restore from - that case is sent empty, since a mask with nothing behind it is a 422. An empty secret is a valid save and reads as "not configured", never as an error. A rejected save leaves the previous configuration standing and reports the per-field 422 verbatim; 401, 403 and an unreachable SEP each get their own message, and a raw HTTP status is never shown. The tab sits behind SepAuthGate, so the settings calls carry the bearer minted from the PMM session (PMM-15293) rather than a cookie, which the admin-gated settings router refuses. Signed-off-by: Ignacio Durand --- .../src/pages/settings/Settings.messages.ts | 53 +++ .../pmm/src/pages/settings/Settings.test.tsx | 14 + ui/apps/pmm/src/pages/settings/Settings.tsx | 7 + .../pmm/src/pages/settings/Settings.types.ts | 6 +- .../ServiceNowConnection.constants.ts | 19 + .../servicenow/ServiceNowConnection.types.ts | 31 ++ .../ServiceNowConnection.utils.test.ts | 349 ++++++++++++++ .../servicenow/ServiceNowConnection.utils.ts | 214 +++++++++ .../ServiceNowConnectionForm.schema.ts | 33 ++ .../ServiceNowConnectionForm.test.tsx | 433 ++++++++++++++++++ .../servicenow/ServiceNowConnectionForm.tsx | 304 ++++++++++++ .../servicenow/ServiceNowConnectionTab.tsx | 17 + .../settings/components/servicenow/index.ts | 2 + 13 files changed, 1481 insertions(+), 1 deletion(-) create mode 100644 ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.constants.ts create mode 100644 ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.types.ts create mode 100644 ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.test.ts create mode 100644 ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.ts create mode 100644 ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.schema.ts create mode 100644 ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.test.tsx create mode 100644 ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.tsx create mode 100644 ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionTab.tsx create mode 100644 ui/apps/pmm/src/pages/settings/components/servicenow/index.ts diff --git a/ui/apps/pmm/src/pages/settings/Settings.messages.ts b/ui/apps/pmm/src/pages/settings/Settings.messages.ts index 85b2f143f80..0f07ca4afc0 100644 --- a/ui/apps/pmm/src/pages/settings/Settings.messages.ts +++ b/ui/apps/pmm/src/pages/settings/Settings.messages.ts @@ -4,6 +4,7 @@ export const Messages = { ssh: 'SSH key', metrics: 'Metrics resolution', advanced: 'Advanced settings', + serviceNow: 'ServiceNow connection', }, advanced: { validation: { @@ -102,6 +103,58 @@ export const Messages = { invalidFormat: 'Enter a valid SSH public key (e.g. ssh-rsa, ssh-ed25519)', }, }, + serviceNow: { + label: 'ServiceNow connection', + description: + "Connect this PMM instance to Percona's ServiceNow so features like Support diagnostics can work directly with your support cases. You'll need a Percona Support account with access to your organization's cases.", + scopeNote: + "This connection applies to the whole PMM instance. Once connected, anyone with access to Support features can send results to your organization's cases.", + subscriptionPrompt: "Don't have a Percona Support subscription?", + subscriptionLink: 'https://www.percona.com/services/support', + subscriptionLinkText: 'Learn about Percona Support', + endpointLabel: 'Receiver endpoint', + endpointPlaceholder: 'https://percona.service-now.com/', + endpointHelper: + 'Leave empty to use the receiver bundled with this PMM version.', + secretsLegend: 'Credentials', + secretHelper: (name: string) => `Sent to SEP as "${name}".`, + secretStoredHelper: (name: string) => + `Sent to SEP as "${name}". A value is stored — leave it untouched to keep it.`, + status: { + configured: + 'Connected. Support diagnostics can send results to your ServiceNow cases.', + notConfigured: + "Not configured. Support diagnostics can't send results until these details are saved.", + drifted: + "The stored details no longer match this deployment's delivery plan. Re-supply the values below.", + }, + noSecrets: + "This PMM version's delivery plan declares no credentials, so only the receiver endpoint can be set here.", + unavailable: + 'Support diagnostics delivery is not part of this PMM version, so there is nothing to connect.', + saveSuccess: 'ServiceNow connection saved', + disconnect: 'Disconnect', + disconnectTitle: 'Disconnect ServiceNow?', + disconnectBody: + 'PMM will forget the stored endpoint and credentials, and Support diagnostics will stop sending results until they are supplied again.', + disconnectConfirm: 'Disconnect', + disconnectCancel: 'Cancel', + disconnectSuccess: 'ServiceNow connection removed', + validation: { + invalidUrl: 'Enter a valid URL (e.g. https://example.service-now.com/)', + }, + errors: { + forbidden: + "Your account isn't allowed to change this connection. A PMM administrator has to save it.", + unauthenticated: + "Your session isn't valid for this action anymore. Reload the page and try again.", + unreachable: + "Couldn't reach the Support diagnostics service. The previous configuration is unchanged.", + generic: + "Couldn't save the connection. The previous configuration is unchanged.", + loadFailed: "Couldn't load the current ServiceNow connection.", + }, + }, service: { success: 'Settings updated', }, diff --git a/ui/apps/pmm/src/pages/settings/Settings.test.tsx b/ui/apps/pmm/src/pages/settings/Settings.test.tsx index 32b5a2b836c..cbd3991d0d0 100644 --- a/ui/apps/pmm/src/pages/settings/Settings.test.tsx +++ b/ui/apps/pmm/src/pages/settings/Settings.test.tsx @@ -16,6 +16,9 @@ vi.mock('./components/advanced/AdvancedSettingsForm', () => ({ vi.mock('./components/ssh-key/SshKeyForm', () => ({ SshKeyForm: () => null, })); +vi.mock('./components/servicenow', () => ({ + ServiceNowConnectionTab: () =>
, +})); const getSettingsMock = vi.mocked(settingsApi.getSettings); const mockSettings = {} as SettingsType; @@ -77,6 +80,17 @@ describe('Settings', () => { ); }); + it('activates the ServiceNow tab for /settings/servicenow-connection', async () => { + renderWithRoute('/settings/servicenow-connection'); + await waitFor(() => + expect(screen.getByTestId('settings-tab-servicenow')).toHaveAttribute( + 'aria-selected', + 'true' + ) + ); + expect(screen.getByTestId('servicenow-tab')).toBeInTheDocument(); + }); + it('defaults to metrics tab when no tab is in the URL', async () => { renderWithRoute('/settings'); await waitFor(() => diff --git a/ui/apps/pmm/src/pages/settings/Settings.tsx b/ui/apps/pmm/src/pages/settings/Settings.tsx index 9e504182ff4..78596c69748 100644 --- a/ui/apps/pmm/src/pages/settings/Settings.tsx +++ b/ui/apps/pmm/src/pages/settings/Settings.tsx @@ -9,6 +9,7 @@ import { useSettings } from 'hooks/api/useSettings'; import { SshKeyForm } from './components/ssh-key/SshKeyForm'; import { MetricsResolutionForm } from './components/metrics-resolution/MetricsResolutionForm'; import { AdvancedSettingsForm } from './components/advanced/AdvancedSettingsForm'; +import { ServiceNowConnectionTab } from './components/servicenow'; import { Messages } from './Settings.messages'; import { TabValue } from './Settings.types'; import { useNavigate, useParams } from 'react-router-dom'; @@ -65,6 +66,11 @@ export const Settings: FC = () => { value="ssh-key" label={Messages.tabs.ssh} /> + @@ -75,6 +81,7 @@ export const Settings: FC = () => { )} {tab === 'ssh-key' && } + {tab === 'servicenow-connection' && } diff --git a/ui/apps/pmm/src/pages/settings/Settings.types.ts b/ui/apps/pmm/src/pages/settings/Settings.types.ts index 0b9329e0130..5580afd5cc3 100644 --- a/ui/apps/pmm/src/pages/settings/Settings.types.ts +++ b/ui/apps/pmm/src/pages/settings/Settings.types.ts @@ -1 +1,5 @@ -export type TabValue = 'ssh-key' | 'metrics-resolution' | 'advanced-settings'; +export type TabValue = + | 'ssh-key' + | 'metrics-resolution' + | 'advanced-settings' + | 'servicenow-connection'; diff --git a/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.constants.ts b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.constants.ts new file mode 100644 index 00000000000..e0bc153233a --- /dev/null +++ b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.constants.ts @@ -0,0 +1,19 @@ +import { SettingClass } from '@sep/api'; + +/** The SEP settings class that owns the diagnostics delivery keys. */ +export const SEP_SETTINGS_CLASS: SettingClass = 'SEPSettings'; + +/** + * The single structured, writable key. SEP seals its leaves deliberately: + * `DIAGNOSTICS_DELIVERY_INPUTS__endpoint` / `__secrets` answer 422 + * (`not_overridable`), so the whole object is always written at once. + */ +export const DELIVERY_INPUTS_KEY = 'DIAGNOSTICS_DELIVERY_INPUTS'; + +/** + * The read-only delivery plan baked into the SEP image. Its `value.secrets` + * declares the secret names this deployment must supply — the form renders one + * field per declared name instead of hardcoding them, so an image that renames + * one is picked up on the next load rather than 422-ing on save. + */ +export const DELIVERY_PLAN_KEY = 'DIAGNOSTICS_DELIVERY'; diff --git a/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.types.ts b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.types.ts new file mode 100644 index 00000000000..90c90379add --- /dev/null +++ b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.types.ts @@ -0,0 +1,31 @@ +/** The shape SEP stores under `DIAGNOSTICS_DELIVERY_INPUTS`. */ +export interface DeliveryInputs { + endpoint?: string | null; + secrets?: Record; +} + +export interface StoredDeliveryInputs { + endpoint: string; + /** Secret values as SEP returns them — masked once an override exists. */ + secrets: Record; + /** Whether SEP holds a per-deployment override (masks are restorable). */ + hasOverride: boolean; + /** Whether the key came back at all; `false` means SEP does not expose it. */ + isPresent: boolean; +} + +export type ConnectionStatus = 'configured' | 'not-configured' | 'drifted'; + +export interface ServiceNowFormValues { + endpoint: string; + /** + * Secret values positionally aligned with the declared names. + * + * Deliberately not keyed by name: react-hook-form reads a field name as a + * path, so a declared name carrying a `.` or `[` would register as a nested + * field and read back as `undefined` — silently submitting an empty string + * over a stored secret. The names are runtime data from SEP, so the form + * never lets them reach a path. + */ + secrets: string[]; +} diff --git a/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.test.ts b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.test.ts new file mode 100644 index 00000000000..b382fce7e02 --- /dev/null +++ b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.test.ts @@ -0,0 +1,349 @@ +import { ApiError, REDACTED_SECRET, SettingClassGroup } from '@sep/api'; +import { Messages } from '../../Settings.messages'; +import { + DELIVERY_INPUTS_KEY, + DELIVERY_PLAN_KEY, +} from './ServiceNowConnection.constants'; +import { + buildDeliveryInputsPatch, + connectionStatus, + declaredSecretNames, + secretLabel, + sepErrorMessage, + storedDeliveryInputs, + toFormValues, +} from './ServiceNowConnection.utils'; + +const setting = (key: string, value: unknown, hasOverride = false) => + ({ + key, + value, + has_override: hasOverride, + default_value: null, + description: null, + is_advanced: false, + is_applicable: true, + is_complex: true, + is_secret: false, + reload: 'none', + setting_class: 'SEPSettings', + type: 'object', + }) as unknown as SettingClassGroup['settings'][number]; + +const groups = ( + settings: SettingClassGroup['settings'] +): SettingClassGroup[] => [ + { + setting_class: 'TasksSettings', + is_app_owned: true, + settings: [ + setting('DIAGNOSTICS_DELIVERY_INPUTS', { secrets: { nope: '' } }), + ], + }, + { setting_class: 'SEPSettings', is_app_owned: false, settings }, +]; + +const plan = (names: string[]) => + setting(DELIVERY_PLAN_KEY, { + endpoint: 'https://baked.service-now.com/', + secrets: Object.fromEntries(names.map((name) => [name, REDACTED_SECRET])), + }); + +const stored = (secrets: Record, endpoint = '') => + setting(DELIVERY_INPUTS_KEY, { endpoint, secrets }, true); + +describe('declaredSecretNames', () => { + it('reads the names off the baked plan', () => { + expect( + declaredSecretNames( + groups([plan(['sn_api_key', 'client_token']), stored({ stale: 'x' })]) + ) + ).toEqual(['sn_api_key', 'client_token']); + }); + + it('falls back to the stored inputs when the plan is not listed', () => { + expect( + declaredSecretNames(groups([stored({ sn_api_key: REDACTED_SECRET })])) + ).toEqual(['sn_api_key']); + }); + + it('ignores settings of another class', () => { + expect(declaredSecretNames(groups([]))).toEqual([]); + }); + + it('returns nothing for an absent response', () => { + expect(declaredSecretNames(undefined)).toEqual([]); + }); +}); + +describe('storedDeliveryInputs', () => { + it('reports the endpoint, the masked secrets and the override flag', () => { + expect( + storedDeliveryInputs( + groups([ + stored( + { sn_api_key: REDACTED_SECRET }, + 'https://acme.service-now.com/' + ), + ]) + ) + ).toEqual({ + endpoint: 'https://acme.service-now.com/', + secrets: { sn_api_key: REDACTED_SECRET }, + hasOverride: true, + isPresent: true, + }); + }); + + it('treats a missing key as unconfigured rather than failing', () => { + expect(storedDeliveryInputs(groups([]))).toEqual({ + endpoint: '', + secrets: {}, + hasOverride: false, + isPresent: false, + }); + }); +}); + +describe('toFormValues', () => { + it('seeds one field per declared name from the stored values', () => { + expect( + toFormValues(['sn_api_key', 'client_token'], { + endpoint: 'https://acme.service-now.com/', + secrets: { sn_api_key: REDACTED_SECRET }, + hasOverride: true, + isPresent: true, + }) + ).toEqual({ + endpoint: 'https://acme.service-now.com/', + secrets: [REDACTED_SECRET, ''], + }); + }); + + it('leaves every field empty when nothing is stored', () => { + expect( + toFormValues(['sn_api_key'], { + endpoint: '', + secrets: { sn_api_key: REDACTED_SECRET }, + hasOverride: false, + isPresent: true, + }) + ).toEqual({ endpoint: '', secrets: [''] }); + }); +}); + +describe('buildDeliveryInputsPatch', () => { + const unconfigured = { + endpoint: '', + secrets: {}, + hasOverride: false, + isPresent: true, + }; + const configured = { + endpoint: 'https://acme.service-now.com/', + secrets: { sn_api_key: REDACTED_SECRET, client_token: REDACTED_SECRET }, + hasOverride: true, + isPresent: true, + }; + + it('submits exactly the declared names, dropping anything else the form holds', () => { + const patch = buildDeliveryInputsPatch( + { + endpoint: 'https://acme.service-now.com/', + secrets: ['a', 'b', 'c'], + }, + ['sn_api_key', 'client_token'], + unconfigured + ); + + expect(patch).toEqual({ + endpoint: 'https://acme.service-now.com/', + secrets: { sn_api_key: 'a', client_token: 'b' }, + }); + }); + + it('adds a declared name the form never rendered as empty', () => { + expect( + buildDeliveryInputsPatch( + { endpoint: '', secrets: [] }, + ['sn_api_key'], + unconfigured + ) + ).toEqual({ secrets: { sn_api_key: '' } }); + }); + + it('omits a blank endpoint so SEP keeps the baked receiver', () => { + expect( + buildDeliveryInputsPatch( + { endpoint: ' ', secrets: ['a'] }, + ['sn_api_key'], + unconfigured + ) + ).toEqual({ secrets: { sn_api_key: 'a' } }); + }); + + it('trims the endpoint it does send', () => { + expect( + buildDeliveryInputsPatch( + { endpoint: ' https://acme.service-now.com/ ', secrets: [] }, + [], + unconfigured + ) + ).toEqual({ endpoint: 'https://acme.service-now.com/', secrets: {} }); + }); + + it('resubmits an untouched mask so SEP restores the stored secret', () => { + expect( + buildDeliveryInputsPatch( + { + endpoint: '', + secrets: [REDACTED_SECRET, 'new'], + }, + ['sn_api_key', 'client_token'], + configured + ) + ).toEqual({ + secrets: { sn_api_key: REDACTED_SECRET, client_token: 'new' }, + }); + }); + + it('never sends a mask there is nothing stored to restore', () => { + expect( + buildDeliveryInputsPatch( + { endpoint: '', secrets: [REDACTED_SECRET] }, + ['sn_api_key'], + unconfigured + ) + ).toEqual({ secrets: { sn_api_key: '' } }); + }); + + it('keys the payload by name even for a name that is not a valid form path', () => { + expect( + buildDeliveryInputsPatch( + { endpoint: '', secrets: ['a', 'b'] }, + ['sn.api.key', 'client[token]'], + unconfigured + ) + ).toEqual({ secrets: { 'sn.api.key': 'a', 'client[token]': 'b' } }); + }); + + it('sends empty strings, which is a valid unconfigured save', () => { + expect( + buildDeliveryInputsPatch( + { endpoint: '', secrets: ['', ''] }, + ['sn_api_key', 'client_token'], + configured + ) + ).toEqual({ secrets: { sn_api_key: '', client_token: '' } }); + }); +}); + +describe('connectionStatus', () => { + it('is configured when every declared secret has a stored value', () => { + expect( + connectionStatus(['sn_api_key'], { + endpoint: '', + secrets: { sn_api_key: REDACTED_SECRET }, + hasOverride: true, + isPresent: true, + }) + ).toBe('configured'); + }); + + it('is not configured when nothing was ever saved', () => { + expect( + connectionStatus(['sn_api_key'], { + endpoint: '', + secrets: {}, + hasOverride: false, + isPresent: true, + }) + ).toBe('not-configured'); + }); + + it('reads a saved-but-empty secret as not configured, not as a failure', () => { + expect( + connectionStatus(['sn_api_key', 'client_token'], { + endpoint: '', + secrets: { sn_api_key: REDACTED_SECRET, client_token: '' }, + hasOverride: true, + isPresent: true, + }) + ).toBe('not-configured'); + }); + + it('reports drift when the plan declares a name the stored inputs lack', () => { + expect( + connectionStatus(['sn_api_key', 'renamed_token'], { + endpoint: '', + secrets: { sn_api_key: REDACTED_SECRET, client_token: REDACTED_SECRET }, + hasOverride: true, + isPresent: true, + }) + ).toBe('drifted'); + }); +}); + +describe('sepErrorMessage', () => { + const httpError = (status: number, data?: unknown) => + new ApiError({ kind: 'http', status, message: `HTTP ${status}`, data }); + + it('surfaces the per-field 422 message SEP returns', () => { + const message = sepErrorMessage( + httpError(422, { + detail: [ + { + loc: ['body', DELIVERY_INPUTS_KEY, 'secrets'], + msg: 'undeclared secret names: extra_key', + type: 'value_error', + }, + ], + }) + ); + + expect(message).toBe('undeclared secret names: extra_key'); + }); + + it('explains a 403 instead of leaving it unaccounted for', () => { + expect(sepErrorMessage(httpError(403))).toBe( + Messages.serviceNow.errors.forbidden + ); + }); + + it('explains a 401', () => { + expect(sepErrorMessage(httpError(401))).toBe( + Messages.serviceNow.errors.unauthenticated + ); + }); + + it('reports an unreachable SEP', () => { + expect( + sepErrorMessage(new ApiError({ kind: 'network', message: 'boom' })) + ).toBe(Messages.serviceNow.errors.unreachable); + }); + + it('never leaks a raw HTTP message', () => { + expect(sepErrorMessage(httpError(500))).toBe( + Messages.serviceNow.errors.generic + ); + }); + + it('uses the caller fallback when one is given', () => { + expect(sepErrorMessage(httpError(500), 'nope')).toBe('nope'); + }); + + it('is empty without an error', () => { + expect(sepErrorMessage(null)).toBe(''); + }); +}); + +describe('secretLabel', () => { + it.each([ + ['sn_api_key', 'SN API key'], + ['client_token', 'Client token'], + ['instance_url', 'Instance URL'], + ['token', 'Token'], + ])('renders %s as %s', (name, expected) => { + expect(secretLabel(name)).toBe(expected); + }); +}); diff --git a/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.ts b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.ts new file mode 100644 index 00000000000..0803ac359e5 --- /dev/null +++ b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.ts @@ -0,0 +1,214 @@ +import { + ApiError, + REDACTED_SECRET, + SettingClassGroup, + SettingResponse, + settingErrorMessage, +} from '@sep/api'; +import { Messages } from '../../Settings.messages'; +import { + DELIVERY_INPUTS_KEY, + DELIVERY_PLAN_KEY, + SEP_SETTINGS_CLASS, +} from './ServiceNowConnection.constants'; +import { + ConnectionStatus, + DeliveryInputs, + ServiceNowFormValues, + StoredDeliveryInputs, +} from './ServiceNowConnection.types'; + +/** Locate one setting inside the `SEPSettings` group of a LIST response. */ +export const findSepSetting = ( + groups: SettingClassGroup[] | undefined, + key: string +): SettingResponse | undefined => + groups + ?.find((group) => group.setting_class === SEP_SETTINGS_CLASS) + ?.settings.find((setting) => setting.key === key); + +const asInputs = (value: unknown): DeliveryInputs => { + if (!value || typeof value !== 'object') { + return {}; + } + const { endpoint, secrets } = value as DeliveryInputs; + return { + endpoint: typeof endpoint === 'string' ? endpoint : null, + secrets: + secrets && typeof secrets === 'object' && !Array.isArray(secrets) + ? Object.fromEntries( + Object.entries(secrets).map(([name, secret]) => [ + name, + typeof secret === 'string' ? secret : '', + ]) + ) + : {}, + }; +}; + +/** + * The secret names this deployment must supply, read from the baked plan. + * + * The plan is the declaration SEP validates a write against, so it always wins. + * The stored inputs are a fallback for a SEP build that does not list the plan + * at all: their names are only stale if that build also renamed one, and the + * cost of guessing wrong there is a 422 the form shows verbatim — better than a + * form with no fields, which no operator could recover from. + */ +export const declaredSecretNames = ( + groups: SettingClassGroup[] | undefined +): string[] => { + const planNames = Object.keys( + asInputs(findSepSetting(groups, DELIVERY_PLAN_KEY)?.value).secrets ?? {} + ); + if (planNames.length > 0) { + return planNames; + } + return Object.keys( + asInputs(findSepSetting(groups, DELIVERY_INPUTS_KEY)?.value).secrets ?? {} + ); +}; + +/** + * The stored per-deployment inputs. Secrets come back masked + * ({@link REDACTED_SECRET}) once something is stored, so the values here are + * only ever displayed or resubmitted verbatim — never inspected for content. + */ +export const storedDeliveryInputs = ( + groups: SettingClassGroup[] | undefined +): StoredDeliveryInputs => { + const setting = findSepSetting(groups, DELIVERY_INPUTS_KEY); + const { endpoint, secrets } = asInputs(setting?.value); + return { + endpoint: endpoint ?? '', + secrets: secrets ?? {}, + hasOverride: setting?.has_override ?? false, + isPresent: setting !== undefined, + }; +}; + +/** + * Seed the form from what SEP stored, one field per declared secret name, in + * declaration order — the form addresses secrets by position, not by name. + */ +export const toFormValues = ( + declaredNames: string[], + stored: StoredDeliveryInputs +): ServiceNowFormValues => ({ + endpoint: stored.endpoint, + secrets: declaredNames.map((name) => + stored.hasOverride ? (stored.secrets[name] ?? '') : '' + ), +}); + +/** + * Build the PATCH value: one whole object carrying exactly the declared secret + * names. + * + * `endpoint` is dropped when blank so SEP keeps the receiver its image bakes + * in — that is also how a previously entered endpoint is reverted. A mask is + * only resubmitted when an override exists to restore it from; without one SEP + * answers 422, so it is sent as empty instead. + */ +export const buildDeliveryInputsPatch = ( + values: ServiceNowFormValues, + declaredNames: string[], + stored: StoredDeliveryInputs +): DeliveryInputs => { + const endpoint = values.endpoint.trim(); + const secrets = Object.fromEntries( + declaredNames.map((name, index) => { + const value = values.secrets[index] ?? ''; + const isUnrestorableMask = + value === REDACTED_SECRET && !stored.hasOverride; + return [name, isUnrestorableMask ? '' : value]; + }) + ); + return endpoint ? { endpoint, secrets } : { secrets }; +}; + +/** + * What the stored inputs say about delivery, without asking SEP a second time. + * + * An empty secret is a valid save that leaves delivery unavailable, so it reads + * as "not configured" rather than as a failure. A declared name with no stored + * counterpart means the image renamed one after the values were supplied — the + * value SEP still holds no longer satisfies the plan. + */ +export const connectionStatus = ( + declaredNames: string[], + stored: StoredDeliveryInputs +): ConnectionStatus => { + if (!stored.hasOverride || declaredNames.length === 0) { + return 'not-configured'; + } + if (declaredNames.some((name) => stored.secrets[name] === undefined)) { + return 'drifted'; + } + return declaredNames.every((name) => stored.secrets[name] !== '') + ? 'configured' + : 'not-configured'; +}; + +/** + * Turn a failed SEP call into something the operator can act on. + * + * SEP's 422 message is the most specific thing available — it names the + * offending secret keys — so it wins over the generic mapping. Everything else + * distinguishes "you may not do this" from "SEP did not answer", because the + * two need different responses. A raw HTTP message is never shown: anything + * unrecognised falls back to `fallback`. + */ +export const sepErrorMessage = ( + error: ApiError | null | undefined, + fallback: string = Messages.serviceNow.errors.generic +): string => { + if (!error) { + return ''; + } + const validation = settingErrorMessage(error, DELIVERY_INPUTS_KEY); + if (validation) { + return validation; + } + const { errors } = Messages.serviceNow; + if (error.status === 403) { + return errors.forbidden; + } + if (error.status === 401) { + return errors.unauthenticated; + } + if (error.kind === 'network' || error.kind === 'timeout') { + return errors.unreachable; + } + return fallback; +}; + +const ACRONYMS = new Set([ + 'api', + 'id', + 'sn', + 'url', + 'uri', + 'jwt', + 'ssl', + 'tls', +]); + +/** + * Render a SEP secret name as a field label — `sn_api_key` reads "SN API key". + * The raw name stays visible as helper text, since that is what SEP's own + * documentation and error messages call it. + */ +export const secretLabel = (name: string): string => + name + .split(/[_\-\s]+/) + .filter(Boolean) + .map((word, index) => { + if (ACRONYMS.has(word.toLowerCase())) { + return word.toUpperCase(); + } + return index === 0 + ? word.charAt(0).toUpperCase() + word.slice(1).toLowerCase() + : word.toLowerCase(); + }) + .join(' '); diff --git a/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.schema.ts b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.schema.ts new file mode 100644 index 00000000000..40cbc80b337 --- /dev/null +++ b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.schema.ts @@ -0,0 +1,33 @@ +import { z } from 'zod'; +import { Messages } from '../../Settings.messages'; + +const { invalidUrl } = Messages.serviceNow.validation; + +const isAbsoluteUrl = (value: string) => { + try { + const { protocol } = new URL(value); + return protocol === 'http:' || protocol === 'https:'; + } catch { + return false; + } +}; + +/** + * Client-side validation is deliberately thin: SEP validates the whole object + * on write and is the only authority on which secret names are acceptable, so + * the schema only catches an endpoint that could never be a URL. The secret + * values are positional and unconstrained — the names they belong to come from + * the declared plan, and a mismatch is SEP's 422 to report, which the form + * surfaces verbatim. + * + * An empty endpoint is valid: it means "keep the receiver this image bakes in". + * An empty secret is valid too, and saves as an explicitly unconfigured state. + */ +export const serviceNowSchema = z.object({ + endpoint: z + .string() + .refine((value) => value.trim() === '' || isAbsoluteUrl(value.trim()), { + message: invalidUrl, + }), + secrets: z.array(z.string()), +}); diff --git a/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.test.tsx b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.test.tsx new file mode 100644 index 00000000000..dac7935b698 --- /dev/null +++ b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.test.tsx @@ -0,0 +1,433 @@ +import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { + ApiError, + REDACTED_SECRET, + SettingClassGroup, + useResetSetting, + usePatchSetting, + useSettingsList, +} from '@sep/api'; +import { TestWrapper } from 'utils/testWrapper'; +import { wrapWithSnackbarProvider } from 'utils/testUtils'; +import { Messages } from '../../Settings.messages'; +import { ServiceNowConnectionForm } from './ServiceNowConnectionForm'; + +vi.mock('@sep/api', async (importOriginal) => ({ + ...(await importOriginal()), + useSettingsList: vi.fn(), + usePatchSetting: vi.fn(), + useResetSetting: vi.fn(), +})); + +const settingsList = vi.mocked(useSettingsList); +const patchSetting = vi.mocked(usePatchSetting); +const resetSetting = vi.mocked(useResetSetting); + +const patchMutation = vi.fn(); +const resetMutation = vi.fn(); + +const setting = (key: string, value: unknown, hasOverride = false) => + ({ + key, + value, + has_override: hasOverride, + default_value: null, + description: null, + is_advanced: false, + is_applicable: true, + is_complex: true, + is_secret: false, + reload: 'none', + setting_class: 'SEPSettings', + type: 'object', + }) as unknown as SettingClassGroup['settings'][number]; + +const sepGroups = ( + declared: string[], + storedSecrets?: Record, + endpoint = '' +): SettingClassGroup[] => [ + { + setting_class: 'SEPSettings', + is_app_owned: false, + settings: [ + setting('DIAGNOSTICS_DELIVERY', { + secrets: Object.fromEntries( + declared.map((name) => [name, REDACTED_SECRET]) + ), + }), + setting( + 'DIAGNOSTICS_DELIVERY_INPUTS', + { endpoint, secrets: storedSecrets ?? {} }, + storedSecrets !== undefined + ), + ], + }, +]; + +const mockList = ( + overrides: Partial> = {} +) => { + settingsList.mockReturnValue({ + data: sepGroups(['sn_api_key', 'client_token']), + isLoading: false, + error: null, + ...overrides, + } as ReturnType); +}; + +const mockPatch = (error: ApiError | null = null) => { + patchSetting.mockReturnValue({ + mutateAsync: patchMutation, + error, + } as unknown as ReturnType); +}; + +const renderForm = () => + render( + + {wrapWithSnackbarProvider()} + + ); + +const type = (testId: string, value: string) => + fireEvent.change(screen.getByTestId(testId), { target: { value } }); + +const submit = async () => { + const button = screen.getByTestId('servicenow-submit'); + await waitFor(() => expect(button).toBeEnabled()); + fireEvent.click(button); +}; + +beforeEach(() => { + vi.clearAllMocks(); + patchMutation.mockResolvedValue([]); + resetMutation.mockResolvedValue(undefined); + mockList(); + mockPatch(); + resetSetting.mockReturnValue({ + mutateAsync: resetMutation, + isPending: false, + } as unknown as ReturnType); +}); + +describe('ServiceNowConnectionForm — rendering', () => { + it('renders one field per declared secret name', () => { + renderForm(); + + expect(screen.getByTestId('servicenow-secret-sn_api_key')).toHaveAttribute( + 'type', + 'password' + ); + expect( + screen.getByTestId('servicenow-secret-client_token') + ).toBeInTheDocument(); + }); + + it('follows the plan when an image renames a declared secret', () => { + settingsList.mockReturnValue({ + data: sepGroups(['sn_api_key', 'renamed_token'], { + sn_api_key: REDACTED_SECRET, + client_token: REDACTED_SECRET, + }), + isLoading: false, + error: null, + } as ReturnType); + + renderForm(); + + expect( + screen.getByTestId('servicenow-secret-renamed_token') + ).toBeInTheDocument(); + expect( + screen.queryByTestId('servicenow-secret-client_token') + ).not.toBeInTheDocument(); + expect(screen.getByTestId('servicenow-status')).toHaveTextContent( + Messages.serviceNow.status.drifted + ); + }); + + it('submits a declared name that form paths cannot express, verbatim', async () => { + settingsList.mockReturnValue({ + data: sepGroups(['sn.api.key']), + isLoading: false, + error: null, + } as ReturnType); + + renderForm(); + + type('servicenow-secret-sn.api.key', 'key-1'); + await submit(); + + await waitFor(() => expect(patchMutation).toHaveBeenCalledTimes(1)); + expect(patchMutation.mock.calls[0][0].value).toEqual({ + secrets: { 'sn.api.key': 'key-1' }, + }); + }); + + it('reports an unsaved connection as not configured rather than as an error', () => { + renderForm(); + + const status = screen.getByTestId('servicenow-status'); + expect(status).toHaveTextContent(Messages.serviceNow.status.notConfigured); + expect(status).toHaveClass('MuiAlert-colorInfo'); + }); + + it('reports a saved-but-empty secret as not configured', () => { + settingsList.mockReturnValue({ + data: sepGroups(['sn_api_key', 'client_token'], { + sn_api_key: REDACTED_SECRET, + client_token: '', + }), + isLoading: false, + error: null, + } as ReturnType); + + renderForm(); + + expect(screen.getByTestId('servicenow-status')).toHaveTextContent( + Messages.serviceNow.status.notConfigured + ); + }); + + it('shows the stored values back, masked', () => { + settingsList.mockReturnValue({ + data: sepGroups( + ['sn_api_key', 'client_token'], + { sn_api_key: REDACTED_SECRET, client_token: REDACTED_SECRET }, + 'https://acme.service-now.com/' + ), + isLoading: false, + error: null, + } as ReturnType); + + renderForm(); + + expect(screen.getByTestId('servicenow-endpoint')).toHaveValue( + 'https://acme.service-now.com/' + ); + expect(screen.getByTestId('servicenow-secret-sn_api_key')).toHaveValue( + REDACTED_SECRET + ); + expect(screen.getByTestId('servicenow-status')).toHaveTextContent( + Messages.serviceNow.status.configured + ); + }); + + it('shows a spinner while the settings load', () => { + mockList({ isLoading: true, data: undefined }); + renderForm(); + + expect(screen.getByTestId('servicenow-loading')).toBeInTheDocument(); + }); + + it('explains a load that was refused rather than showing an empty form', () => { + mockList({ + data: undefined, + error: new ApiError({ kind: 'http', status: 403, message: 'HTTP 403' }), + }); + renderForm(); + + expect(screen.getByTestId('servicenow-load-error')).toHaveTextContent( + Messages.serviceNow.errors.forbidden + ); + expect(screen.queryByTestId('servicenow-submit')).not.toBeInTheDocument(); + }); + + it('still offers the endpoint when the plan declares no credentials', () => { + settingsList.mockReturnValue({ + data: sepGroups([]), + isLoading: false, + error: null, + } as ReturnType); + + renderForm(); + + expect(screen.getByTestId('servicenow-no-secrets')).toBeInTheDocument(); + expect(screen.getByTestId('servicenow-endpoint')).toBeInTheDocument(); + }); + + it('offers nothing when the deployment does not carry the key at all', () => { + settingsList.mockReturnValue({ + data: [ + { setting_class: 'SEPSettings', is_app_owned: false, settings: [] }, + ] as SettingClassGroup[], + isLoading: false, + error: null, + } as ReturnType); + + renderForm(); + + expect(screen.getByTestId('servicenow-unavailable')).toHaveTextContent( + Messages.serviceNow.unavailable + ); + expect(screen.queryByTestId('servicenow-endpoint')).not.toBeInTheDocument(); + }); +}); + +describe('ServiceNowConnectionForm — saving', () => { + it('writes the whole key in one PATCH carrying exactly the declared names', async () => { + renderForm(); + + type('servicenow-endpoint', 'https://acme.service-now.com/'); + type('servicenow-secret-sn_api_key', 'key-1'); + type('servicenow-secret-client_token', 'token-1'); + await submit(); + + await waitFor(() => expect(patchMutation).toHaveBeenCalledTimes(1)); + expect(patchMutation).toHaveBeenCalledWith({ + settingClass: 'SEPSettings', + key: 'DIAGNOSTICS_DELIVERY_INPUTS', + value: { + endpoint: 'https://acme.service-now.com/', + secrets: { sn_api_key: 'key-1', client_token: 'token-1' }, + }, + }); + }); + + it('keeps a stored secret by resubmitting the mask it was shown', async () => { + settingsList.mockReturnValue({ + data: sepGroups( + ['sn_api_key', 'client_token'], + { sn_api_key: REDACTED_SECRET, client_token: REDACTED_SECRET }, + 'https://acme.service-now.com/' + ), + isLoading: false, + error: null, + } as ReturnType); + + renderForm(); + + type('servicenow-endpoint', 'https://other.service-now.com/'); + await submit(); + + await waitFor(() => expect(patchMutation).toHaveBeenCalledTimes(1)); + expect(patchMutation.mock.calls[0][0].value).toEqual({ + endpoint: 'https://other.service-now.com/', + secrets: { + sn_api_key: REDACTED_SECRET, + client_token: REDACTED_SECRET, + }, + }); + }); + + it('accepts clearing a secret as an explicit unconfigured save', async () => { + settingsList.mockReturnValue({ + data: sepGroups(['sn_api_key', 'client_token'], { + sn_api_key: REDACTED_SECRET, + client_token: REDACTED_SECRET, + }), + isLoading: false, + error: null, + } as ReturnType); + + renderForm(); + + type('servicenow-secret-sn_api_key', ''); + await submit(); + + await waitFor(() => expect(patchMutation).toHaveBeenCalledTimes(1)); + expect(patchMutation.mock.calls[0][0].value).toEqual({ + secrets: { sn_api_key: '', client_token: REDACTED_SECRET }, + }); + }); + + it('refuses an endpoint that is not a URL before it reaches SEP', async () => { + renderForm(); + + type('servicenow-endpoint', 'not-a-url'); + type('servicenow-secret-sn_api_key', 'key-1'); + + await waitFor(() => + expect(screen.getByTestId('servicenow-submit')).toBeDisabled() + ); + expect(patchMutation).not.toHaveBeenCalled(); + }); + + it('surfaces the 422 SEP answers with instead of swallowing it', async () => { + mockPatch( + new ApiError({ + kind: 'http', + status: 422, + message: 'HTTP 422', + data: { + detail: [ + { + loc: ['body', 'DIAGNOSTICS_DELIVERY_INPUTS'], + msg: 'undeclared secret names: extra', + type: 'value_error', + }, + ], + }, + }) + ); + + renderForm(); + + expect(screen.getByTestId('servicenow-save-error')).toHaveTextContent( + 'undeclared secret names: extra' + ); + }); + + it('keeps the form on screen when the save is rejected', async () => { + patchMutation.mockRejectedValue( + new ApiError({ kind: 'network', message: 'down' }) + ); + renderForm(); + + type('servicenow-secret-sn_api_key', 'key-1'); + await submit(); + + await waitFor(() => expect(patchMutation).toHaveBeenCalled()); + expect(screen.getByTestId('servicenow-secret-sn_api_key')).toHaveValue( + 'key-1' + ); + }); +}); + +describe('ServiceNowConnectionForm — disconnecting', () => { + const renderConfigured = () => { + settingsList.mockReturnValue({ + data: sepGroups(['sn_api_key', 'client_token'], { + sn_api_key: REDACTED_SECRET, + client_token: REDACTED_SECRET, + }), + isLoading: false, + error: null, + } as ReturnType); + return renderForm(); + }; + + it('is not offered while nothing is stored', () => { + renderForm(); + + expect( + screen.queryByTestId('servicenow-disconnect') + ).not.toBeInTheDocument(); + }); + + it('confirms before clearing the stored inputs', async () => { + renderConfigured(); + + fireEvent.click(screen.getByTestId('servicenow-disconnect')); + expect(resetMutation).not.toHaveBeenCalled(); + + fireEvent.click(screen.getByTestId('servicenow-disconnect-confirm')); + + await waitFor(() => + expect(resetMutation).toHaveBeenCalledWith({ + settingClass: 'SEPSettings', + key: 'DIAGNOSTICS_DELIVERY_INPUTS', + }) + ); + }); + + it('leaves the configuration alone when the confirmation is dismissed', () => { + renderConfigured(); + + fireEvent.click(screen.getByTestId('servicenow-disconnect')); + fireEvent.click(screen.getByTestId('servicenow-disconnect-cancel')); + + expect(resetMutation).not.toHaveBeenCalled(); + }); +}); diff --git a/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.tsx b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.tsx new file mode 100644 index 00000000000..1d3cf4c999d --- /dev/null +++ b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.tsx @@ -0,0 +1,304 @@ +import { FC, useMemo, useState } from 'react'; +import Alert from '@mui/material/Alert'; +import Button from '@mui/material/Button'; +import CircularProgress from '@mui/material/CircularProgress'; +import Divider from '@mui/material/Divider'; +import Link from '@mui/material/Link'; +import Stack from '@mui/material/Stack'; +import Typography from '@mui/material/Typography'; +import { formControlClasses } from '@mui/material'; +import { TextInput } from '@percona/percona-ui'; +import { zodResolver } from '@hookform/resolvers/zod'; +import { FormProvider, useForm } from 'react-hook-form'; +import { enqueueSnackbar } from 'notistack'; +import { + type ApiError, + useResetSetting, + usePatchSetting, + useSettingsList, +} from '@sep/api'; +import { Modal } from 'components/modal'; +import { helperTextTestId } from 'utils/mui.utils'; +import { Messages } from '../../Settings.messages'; +import { MAX_LABEL_WIDTH } from '../../Settings.constants'; +import { SettingsFieldLabel } from '../settings-field-label'; +import { SettingsSubmitButton } from '../settings-submit-button'; +import { + DELIVERY_INPUTS_KEY, + SEP_SETTINGS_CLASS, +} from './ServiceNowConnection.constants'; +import { serviceNowSchema } from './ServiceNowConnectionForm.schema'; +import { ServiceNowFormValues } from './ServiceNowConnection.types'; +import { + buildDeliveryInputsPatch, + connectionStatus, + declaredSecretNames, + sepErrorMessage, + secretLabel, + storedDeliveryInputs, + toFormValues, +} from './ServiceNowConnection.utils'; + +const STATUS_SEVERITY = { + configured: 'success', + 'not-configured': 'info', + drifted: 'warning', +} as const; + +const STATUS_MESSAGE = { + configured: Messages.serviceNow.status.configured, + 'not-configured': Messages.serviceNow.status.notConfigured, + drifted: Messages.serviceNow.status.drifted, +} as const; + +/** + * Direct entry of the ServiceNow details SEP needs to deliver diagnostics. + * + * The operator obtains a ServiceNow token out of band and enters it here; + * PMM-15218 replaces this entry surface with a guided round trip and keeps the + * write path below untouched. + * + * The write is one whole-object PATCH of `DIAGNOSTICS_DELIVERY_INPUTS` carrying + * exactly the secret names the SEP image declares — SEP seals the leaves and + * rejects an unexpected name, so neither is a shape the UI may improvise. All + * validation is server-side and all-or-nothing: a rejected save leaves the + * previous configuration standing, which is why nothing here is optimistic. + */ +export const ServiceNowConnectionForm: FC = () => { + const { data: groups, isLoading, error: loadError } = useSettingsList(); + const { mutateAsync: patchSetting, error: saveError } = usePatchSetting(); + const { mutateAsync: resetSetting, isPending: isDisconnecting } = + useResetSetting(); + const [disconnectOpen, setDisconnectOpen] = useState(false); + + const declaredNames = useMemo(() => declaredSecretNames(groups), [groups]); + const stored = useMemo(() => storedDeliveryInputs(groups), [groups]); + const status = connectionStatus(declaredNames, stored); + + // `values` (not `defaultValues`) so a refetch — the invalidation after a save, + // in particular — re-seeds the fields with what SEP actually stored. React + // Hook Form only re-seeds on a deep change, so a background refetch that + // returns the same data leaves half-typed input alone. + const values = useMemo( + () => toFormValues(declaredNames, stored), + [declaredNames, stored] + ); + const methods = useForm({ + resolver: zodResolver(serviceNowSchema), + values, + }); + + if (isLoading) { + return ( + + + + ); + } + + if (loadError) { + return ( + + {sepErrorMessage(loadError, Messages.serviceNow.errors.loadFailed)} + + ); + } + + // A SEP build that does not carry the key at all would answer any write with + // a 422, so there is nothing to offer — as distinct from a deployment that + // carries it and has simply not been configured yet. + if (!stored.isPresent) { + return ( + + {Messages.serviceNow.unavailable} + + ); + } + + const onSubmit = async (values: ServiceNowFormValues) => { + try { + await patchSetting({ + settingClass: SEP_SETTINGS_CLASS, + key: DELIVERY_INPUTS_KEY, + value: buildDeliveryInputsPatch(values, declaredNames, stored), + }); + enqueueSnackbar(Messages.serviceNow.saveSuccess, { variant: 'success' }); + } catch { + // The rejected mutation is rendered inline by `saveError`; the previous + // configuration is intact because SEP writes nothing on a failed validate. + } + }; + + const onDisconnect = async () => { + try { + await resetSetting({ + settingClass: SEP_SETTINGS_CLASS, + key: DELIVERY_INPUTS_KEY, + }); + enqueueSnackbar(Messages.serviceNow.disconnectSuccess, { + variant: 'success', + }); + setDisconnectOpen(false); + } catch (error) { + enqueueSnackbar(sepErrorMessage(error as ApiError), { + variant: 'error', + }); + } + }; + + const { serviceNow } = Messages; + + return ( + + + + + {serviceNow.scopeNote} + + + + {STATUS_MESSAGE[status]} + + + + + + + + {serviceNow.secretsLegend} + + + {declaredNames.length === 0 ? ( + + {serviceNow.noSecrets} + + ) : ( + declaredNames.map((name, index) => ( + + )) + )} + + + {saveError && ( + + {sepErrorMessage(saveError)} + + )} + + + {serviceNow.subscriptionPrompt}{' '} + + {serviceNow.subscriptionLinkText} + + + + + + {stored.hasOverride && ( + + )} + + + + setDisconnectOpen(false)} + title={serviceNow.disconnectTitle} + > + + {serviceNow.disconnectBody} + + + + + + + + ); +}; diff --git a/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionTab.tsx b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionTab.tsx new file mode 100644 index 00000000000..42d0dbaf48d --- /dev/null +++ b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionTab.tsx @@ -0,0 +1,17 @@ +import { FC } from 'react'; +import { SepAuthGate } from 'sep/SepAuthGate'; +import { ServiceNowConnectionForm } from './ServiceNowConnectionForm'; + +/** + * The settings tab wrapper for the ServiceNow connection. + * + * Everything under it talks to SEP, and SEP's settings router refuses a + * cookie-only mutation before it validates anything (401), so the form is held + * behind the same session exchange the SEP routes use (PMM-15293). Gating here + * rather than in `Settings` keeps the exchange off the other tabs. + */ +export const ServiceNowConnectionTab: FC = () => ( + + + +); diff --git a/ui/apps/pmm/src/pages/settings/components/servicenow/index.ts b/ui/apps/pmm/src/pages/settings/components/servicenow/index.ts new file mode 100644 index 00000000000..c87eb9e0303 --- /dev/null +++ b/ui/apps/pmm/src/pages/settings/components/servicenow/index.ts @@ -0,0 +1,2 @@ +export { ServiceNowConnectionTab } from './ServiceNowConnectionTab'; +export { ServiceNowConnectionForm } from './ServiceNowConnectionForm'; From 0443024e4d11f14494555ff1670d7b354e91e037 Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Mon, 10 Aug 2026 19:35:55 -0300 Subject: [PATCH 07/23] PMM-15294 Judge a secretless plan on the override `connectionStatus` collapsed "no declared secrets" into `not-configured` unconditionally, so a deployment whose plan declares no credentials could save an endpoint and still be told its connection was not configured - with no way for the banner to ever say otherwise. The form offers the endpoint field in that case and accepts the save, so the status contradicted what the surface had just done. With no declared secrets there is no credential left for the deployment to supply, so a stored override is as configured as this form can make it. Absent an override it still reads as not configured. Signed-off-by: Ignacio Durand --- .../ServiceNowConnection.utils.test.ts | 22 +++++++++++++++++++ .../servicenow/ServiceNowConnection.utils.ts | 10 ++++++++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.test.ts b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.test.ts index b382fce7e02..1e75bd52e2a 100644 --- a/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.test.ts +++ b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.test.ts @@ -272,6 +272,28 @@ describe('connectionStatus', () => { ).toBe('not-configured'); }); + it('judges a plan that declares no secrets on the override alone', () => { + expect( + connectionStatus([], { + endpoint: 'https://acme.service-now.com/', + secrets: {}, + hasOverride: true, + isPresent: true, + }) + ).toBe('configured'); + }); + + it('still reads as not configured with no secrets and no override', () => { + expect( + connectionStatus([], { + endpoint: '', + secrets: {}, + hasOverride: false, + isPresent: true, + }) + ).toBe('not-configured'); + }); + it('reports drift when the plan declares a name the stored inputs lack', () => { expect( connectionStatus(['sn_api_key', 'renamed_token'], { diff --git a/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.ts b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.ts index 0803ac359e5..a39d7d72848 100644 --- a/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.ts +++ b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.ts @@ -134,14 +134,22 @@ export const buildDeliveryInputsPatch = ( * as "not configured" rather than as a failure. A declared name with no stored * counterpart means the image renamed one after the values were supplied — the * value SEP still holds no longer satisfies the plan. + * + * A plan that declares no secrets is judged on the override alone: there is no + * credential left for the deployment to supply, so a stored override is as + * configured as this form can make it, and the endpoint the operator saved + * would otherwise never stop reading as missing. */ export const connectionStatus = ( declaredNames: string[], stored: StoredDeliveryInputs ): ConnectionStatus => { - if (!stored.hasOverride || declaredNames.length === 0) { + if (!stored.hasOverride) { return 'not-configured'; } + if (declaredNames.length === 0) { + return 'configured'; + } if (declaredNames.some((name) => stored.secrets[name] === undefined)) { return 'drifted'; } From 439dfbab79d66c85c6e7c4725926c3625bccc1ae Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Tue, 11 Aug 2026 13:10:37 -0300 Subject: [PATCH 08/23] PMM-15293 Point the strip flag at SEP__ROOT_PATH The previous commit's comment claimed SEP carries no root_path support at all. That was true when it was written and stopped being true a day later: SEP-1794 (percona/SEP#1325) added a `SEP.ROOT_PATH` setting, passed to the `FastAPI(root_path=...)` constructor, so a SEP started with `SEP__ROOT_PATH=/sep` serves the prefix and the proxy forwards it untouched. Verified against a local SEP carrying the change: with ROOT_PATH set and nothing stripped, `/sep/api/oauth/session/exchange`, `/sep/api/sep/admin/settings/`, `/sep/api/apps/atw/config/` and `/sep/api/users/me` all resolve. Every one of them was a 404 before. Keep the flag: it still covers a SEP that predates the change or runs with ROOT_PATH unset. Reframe it as the fallback it now is, and warn against pairing it with uvicorn's `--root-path`, which prepends the prefix rather than declaring the mount - the two cancel out by accident rather than by design. Comment only; no behaviour change. Signed-off-by: Ignacio Durand --- ui/apps/pmm/vite.config.ts | 42 +++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/ui/apps/pmm/vite.config.ts b/ui/apps/pmm/vite.config.ts index baef9f71c70..22ec63ea232 100644 --- a/ui/apps/pmm/vite.config.ts +++ b/ui/apps/pmm/vite.config.ts @@ -30,9 +30,10 @@ const target = // SEP backend. The dev server proxies SEP's single `/sep` mount point to it so // the migrated SEP plugins get real data, mirroring the shipped topology where // pmm-server's nginx exposes the side-car under that one location (see -// SEP_BASE_PATH in @sep/api). The prefix is forwarded unstripped by default, -// for a SEP that serves it itself — so PMM_DEV_SEP_BACKEND_URL has to point at a -// backend configured that way, or PMM_DEV_SEP_STRIP_PREFIX has to be set (below). +// SEP_BASE_PATH in @sep/api). The prefix is forwarded unstripped by default, for +// a SEP serving it itself — so PMM_DEV_SEP_BACKEND_URL has to point at a backend +// started with `SEP__ROOT_PATH=/sep`, or PMM_DEV_SEP_STRIP_PREFIX has to be set +// (below). // // Residual interim auth: if PMM_DEV_SEP_INTERNAL_TOKEN is set, inject it // server-side as a Bearer token so no secret reaches the browser. Both variables @@ -50,26 +51,25 @@ const target = const SEP_BASE_PATH = '/sep'; const sepBackendUrl = env.PMM_DEV_SEP_BACKEND_URL || 'http://localhost:8000'; const sepInternalToken = env.PMM_DEV_SEP_INTERNAL_TOKEN; -// Escape hatch for a dev SEP that does not serve the `/sep` prefix itself. -// Setting PMM_DEV_SEP_STRIP_PREFIX=1 strips it on the way out. SEP carries no -// `root_path` support today — not a flag, not a setting, not on the shipped -// side-car's `python -m app.sep.main` — so in practice both ways of running it -// locally need this: +// Escape hatch for a SEP that is not serving the `/sep` prefix itself. Setting +// PMM_DEV_SEP_STRIP_PREFIX=1 strips it before forwarding. // -// - `python -m app.main` serves its routes at `/api/…` and 404s the prefix. -// - `uvicorn --root-path /sep` *prepends* root_path to the request path -// (uvicorn `h11_impl.py`: `full_path = root_path + path`) and so expects -// the proxy to forward the tail. Left unstripped, SEP sees `/sep/sep/…` -// and 404s. This is the setup that also keeps `url_for()` emitting -// correctly prefixed links, so it is the closer match to the shipped -// topology. +// The supported setup does not need this. SEP-1794 added a `SEP.ROOT_PATH` +// setting, passed to the `FastAPI(root_path=…)` constructor, so a SEP started +// with `SEP__ROOT_PATH=/sep` serves the prefix and the proxy hands it over +// untouched. Reach for the strip only against a SEP that predates that change +// or runs with `ROOT_PATH` unset. // -// Off by default, because the default belongs to whatever the server-side -// nginx location ends up doing — which does not exist in this repo yet. Against -// the first setup it is lossy: with no root_path, SEP's `request.url_for()` -// emits prefix-less absolute URLs inside JSON payloads, so any link it hands -// back (file downloads in particular) escapes the mount point. Fine for -// surfaces that return plain data. +// Do not pair it with uvicorn's `--root-path`. That flag *prepends* root_path +// to the request path (`h11_impl.py`: `full_path = root_path + path`) instead +// of declaring the mount, so SEP sees `/sep/sep/…`; the strip cancels it out, +// but `SEP__ROOT_PATH` is the setting that means what it says. +// +// Off by default: the default belongs to whatever the server-side nginx +// location ends up doing, which does not exist in this repo yet. Stripping is +// also lossy against a SEP with no `ROOT_PATH` — `request.url_for()` then emits +// prefix-less absolute URLs inside JSON payloads, so any link it hands back +// (file downloads in particular) escapes the mount point. const sepStripPrefix = env.PMM_DEV_SEP_STRIP_PREFIX === '1'; // Matched against the proxied request URL. Vite applies `rewrite` to `req.url` // before the proxy sees it, so with the strip enabled the prefix is already From 018ddab17b2d00d52e1a11fe24e70d23e240fef4 Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Wed, 12 Aug 2026 01:26:59 -0300 Subject: [PATCH 09/23] PMM-15294 Point ServiceNow form at the renamed peak-ui package Signed-off-by: Ignacio Durand --- .../settings/components/servicenow/ServiceNowConnectionForm.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.tsx b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.tsx index 1d3cf4c999d..40319d2e427 100644 --- a/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.tsx +++ b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.tsx @@ -7,7 +7,7 @@ import Link from '@mui/material/Link'; import Stack from '@mui/material/Stack'; import Typography from '@mui/material/Typography'; import { formControlClasses } from '@mui/material'; -import { TextInput } from '@percona/percona-ui'; +import { TextInput } from '@percona/peak-ui'; import { zodResolver } from '@hookform/resolvers/zod'; import { FormProvider, useForm } from 'react-hook-form'; import { enqueueSnackbar } from 'notistack'; From 44745d5443d952ae59650840fc1a6f32ab90bbf8 Mon Sep 17 00:00:00 2001 From: yyyyyyy Date: Wed, 12 Aug 2026 01:56:25 -0300 Subject: [PATCH 10/23] PMM-15293 Drop the SSR-era HTML and 303 handling again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The merge of the base branch resolved typed-client.ts in favour of this branch, which reinstated isHtmlLoginResponse and the 303 clause that PMM-15216 had deleted with the SEP-1687 port. The Jinja login route that could answer an API call with a 200 HTML body is gone, so content-type sniffing can no longer mean "session expired" — under PMM it would only fire on a proxy misconfiguration and report that as a lost session. The axios transport in client.ts already took the deletion, and the tests covering the removed behaviour are gone, so this restores parity between the two transports. The token mint-and-replay path this branch adds is untouched. Signed-off-by: yyyyyyy --- ui/packages/sep/api/src/typed-client.ts | 26 ++----------------------- 1 file changed, 2 insertions(+), 24 deletions(-) diff --git a/ui/packages/sep/api/src/typed-client.ts b/ui/packages/sep/api/src/typed-client.ts index 26b312615ff..0b7b0784dc8 100644 --- a/ui/packages/sep/api/src/typed-client.ts +++ b/ui/packages/sep/api/src/typed-client.ts @@ -61,16 +61,6 @@ const isLoginRequest = (url: string) => url.includes('/oauth/login'); const isReplayEligible = (url: string) => !isTokenMintRequest(url) && !isLoginRequest(url); -/** - * A 200 HTML response (e.g. a follow of a login redirect) means the session - * is gone. The browser can't observe the 303, so content-type is the only - * signal. Synthesise a 401 so the normal error path runs. - */ -function isHtmlLoginResponse(response: Response): boolean { - const ct = response.headers.get('content-type') ?? ''; - return response.ok && ct.includes('text/html'); -} - // `fetch` consumes a Request's body stream, so the instance handed to // `onResponse` can no longer be re-sent. Stash an untouched clone taken before // dispatch, keyed weakly so requests that never come back are not retained. @@ -132,14 +122,6 @@ const authMiddleware: Middleware = { ); } - if (isHtmlLoginResponse(response) && !isRefreshRequest(request.url)) { - emitUnauthorized(); - return new Response(null, { - status: 401, - statusText: 'Session expired (redirected to login page)', - }); - } - if (response.status === 401 && isReplayEligible(request.url)) { const replayed = await replayWithFreshToken(request); if (replayed && replayed.status !== 401) { @@ -150,13 +132,9 @@ const authMiddleware: Middleware = { return replayed ?? response; } - if ( - (response.status === 401 || response.status === 303) && - !isRefreshRequest(request.url) - ) { + if (response.status === 401 && !isRefreshRequest(request.url)) { // A 401 left here is a minting endpoint rejecting the ambient session — - // "not signed in", which the auth layer must hear about. A 303 is the - // login redirect on any endpoint. + // "not signed in", which the auth layer must hear about. emitUnauthorized(); } From 6e4f9a337e6e72a9c957b0844bf6b30f72a18472 Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Wed, 12 Aug 2026 23:34:51 -0300 Subject: [PATCH 11/23] PMM-15294 Extract Percona Support URL to a constant Signed-off-by: Ignacio Durand --- ui/apps/pmm/src/pages/settings/Settings.constants.ts | 2 ++ ui/apps/pmm/src/pages/settings/Settings.messages.ts | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/ui/apps/pmm/src/pages/settings/Settings.constants.ts b/ui/apps/pmm/src/pages/settings/Settings.constants.ts index 384203d8b4e..d0458825117 100644 --- a/ui/apps/pmm/src/pages/settings/Settings.constants.ts +++ b/ui/apps/pmm/src/pages/settings/Settings.constants.ts @@ -1 +1,3 @@ export const MAX_LABEL_WIDTH = 750; + +export const PERCONA_SUPPORT_URL = 'https://www.percona.com/services/support'; diff --git a/ui/apps/pmm/src/pages/settings/Settings.messages.ts b/ui/apps/pmm/src/pages/settings/Settings.messages.ts index 0f07ca4afc0..0213e5ab456 100644 --- a/ui/apps/pmm/src/pages/settings/Settings.messages.ts +++ b/ui/apps/pmm/src/pages/settings/Settings.messages.ts @@ -1,3 +1,5 @@ +import { PERCONA_SUPPORT_URL } from './Settings.constants'; + export const Messages = { title: 'Settings', tabs: { @@ -110,7 +112,7 @@ export const Messages = { scopeNote: "This connection applies to the whole PMM instance. Once connected, anyone with access to Support features can send results to your organization's cases.", subscriptionPrompt: "Don't have a Percona Support subscription?", - subscriptionLink: 'https://www.percona.com/services/support', + subscriptionLink: PERCONA_SUPPORT_URL, subscriptionLinkText: 'Learn about Percona Support', endpointLabel: 'Receiver endpoint', endpointPlaceholder: 'https://percona.service-now.com/', From 59ff70a184fd739d542e3e4e5c3692d7d298e2bc Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Thu, 13 Aug 2026 14:00:21 -0300 Subject: [PATCH 12/23] PMM-15337 Extract the ServiceNow connection hook The settings form and the Support diagnostics setup gate ask the same question of the same settings LIST response, so the derivation moves out of the form into useServiceNowConnection. TanStack Query dedupes the request, so both surfaces share one fetch. Signed-off-by: Ignacio Durand --- .../servicenow/ServiceNowConnection.hooks.ts | 29 +++++++++++++++++++ .../servicenow/ServiceNowConnectionForm.tsx | 23 ++++++--------- 2 files changed, 38 insertions(+), 14 deletions(-) create mode 100644 ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.hooks.ts diff --git a/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.hooks.ts b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.hooks.ts new file mode 100644 index 00000000000..f9c4809115b --- /dev/null +++ b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.hooks.ts @@ -0,0 +1,29 @@ +import { useMemo } from 'react'; +import { useSettingsList } from '@sep/api'; +import { + connectionStatus, + declaredSecretNames, + storedDeliveryInputs, +} from './ServiceNowConnection.utils'; + +/** + * What SEP currently holds for ServiceNow delivery, read once and derived. + * + * Both the settings form and the Support diagnostics setup gate ask the same + * question of the same LIST response, so the derivation lives here rather than + * in either surface — TanStack Query dedupes the request itself. + */ +export const useServiceNowConnection = () => { + const { data: groups, isLoading, error } = useSettingsList(); + + const declaredNames = useMemo(() => declaredSecretNames(groups), [groups]); + const stored = useMemo(() => storedDeliveryInputs(groups), [groups]); + + return { + declaredNames, + stored, + status: connectionStatus(declaredNames, stored), + isLoading, + error, + }; +}; diff --git a/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.tsx b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.tsx index 40319d2e427..d50a75abdc2 100644 --- a/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.tsx +++ b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.tsx @@ -11,12 +11,7 @@ import { TextInput } from '@percona/peak-ui'; import { zodResolver } from '@hookform/resolvers/zod'; import { FormProvider, useForm } from 'react-hook-form'; import { enqueueSnackbar } from 'notistack'; -import { - type ApiError, - useResetSetting, - usePatchSetting, - useSettingsList, -} from '@sep/api'; +import { type ApiError, useResetSetting, usePatchSetting } from '@sep/api'; import { Modal } from 'components/modal'; import { helperTextTestId } from 'utils/mui.utils'; import { Messages } from '../../Settings.messages'; @@ -31,13 +26,11 @@ import { serviceNowSchema } from './ServiceNowConnectionForm.schema'; import { ServiceNowFormValues } from './ServiceNowConnection.types'; import { buildDeliveryInputsPatch, - connectionStatus, - declaredSecretNames, sepErrorMessage, secretLabel, - storedDeliveryInputs, toFormValues, } from './ServiceNowConnection.utils'; +import { useServiceNowConnection } from './ServiceNowConnection.hooks'; const STATUS_SEVERITY = { configured: 'success', @@ -65,16 +58,18 @@ const STATUS_MESSAGE = { * previous configuration standing, which is why nothing here is optimistic. */ export const ServiceNowConnectionForm: FC = () => { - const { data: groups, isLoading, error: loadError } = useSettingsList(); + const { + declaredNames, + stored, + status, + isLoading, + error: loadError, + } = useServiceNowConnection(); const { mutateAsync: patchSetting, error: saveError } = usePatchSetting(); const { mutateAsync: resetSetting, isPending: isDisconnecting } = useResetSetting(); const [disconnectOpen, setDisconnectOpen] = useState(false); - const declaredNames = useMemo(() => declaredSecretNames(groups), [groups]); - const stored = useMemo(() => storedDeliveryInputs(groups), [groups]); - const status = connectionStatus(declaredNames, stored); - // `values` (not `defaultValues`) so a refetch — the invalidation after a save, // in particular — re-seeds the fields with what SEP actually stored. React // Hook Form only re-seeds on a deep change, so a background refetch that From 4077a540ae4536ec3537d06bf9e6c43e1482f5f4 Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Thu, 13 Aug 2026 14:00:29 -0300 Subject: [PATCH 13/23] PMM-15337 Gate diagnostics on ServiceNow setup Everything the app can do ends in an upload to a ServiceNow case, so on an unconfigured instance a user could browse, create an incident and run a script only to find at the last step that nothing can be delivered. A setup screen now replaces the app until delivery is configured: what the tool does, a link to the settings tab that configures it, and the promise that nothing is collected without an explicit confirmation. The gate sits inside SepAuthGate, since reading the SEP settings needs the exchanged bearer. A failed settings read says nothing about the connection, so it fails open and lets the app report its own errors. SepPage wrapped its children in a plain div, which broke the flex chain from Page and left nothing below it able to centre vertically; it is now a growing flex column. Signed-off-by: Ignacio Durand --- ui/apps/pmm/src/lib/constants.ts | 9 ++ .../src/pages/settings/Settings.constants.ts | 2 - .../src/pages/settings/Settings.messages.ts | 2 +- ui/apps/pmm/src/router.tsx | 11 +- ui/apps/pmm/src/sep/SepPage.tsx | 10 +- .../src/sep/ServiceNowSetupGate.messages.ts | 13 ++ .../pmm/src/sep/ServiceNowSetupGate.test.tsx | 127 ++++++++++++++++++ ui/apps/pmm/src/sep/ServiceNowSetupGate.tsx | 116 ++++++++++++++++ 8 files changed, 284 insertions(+), 6 deletions(-) create mode 100644 ui/apps/pmm/src/sep/ServiceNowSetupGate.messages.ts create mode 100644 ui/apps/pmm/src/sep/ServiceNowSetupGate.test.tsx create mode 100644 ui/apps/pmm/src/sep/ServiceNowSetupGate.tsx diff --git a/ui/apps/pmm/src/lib/constants.ts b/ui/apps/pmm/src/lib/constants.ts index f755d21f062..15230e94339 100644 --- a/ui/apps/pmm/src/lib/constants.ts +++ b/ui/apps/pmm/src/lib/constants.ts @@ -23,6 +23,15 @@ export const PMM_NEW_NAV_HOME_URL = `${PMM_NEW_NAV_PATH}/graph/d/pmm-home`; export const SEP_ATW_PATH = `${PMM_NEW_NAV_PATH}/sep/atw`; export const SEP_MYSQL_BACKUPS_PATH = `${PMM_NEW_NAV_PATH}/sep/mysql-backups`; +// In-app (router-relative, no basename) path of the settings tab that owns the +// ServiceNow connection — linked from the Support diagnostics setup prompt. +export const PMM_SERVICENOW_SETTINGS_PATH = `${PMM_NEW_NAV_PATH}/settings/servicenow-connection`; + +export const PERCONA_SUPPORT_URL = 'https://www.percona.com/services/support'; +// Placeholder: points at the PMM docs entry point until the Support diagnostics +// page is published. +export const SUPPORT_DIAGNOSTICS_DOCS_URL = PMM_SUPPORT_URL; + export const INTERVALS_MS = { // 5 mins SERVICE_TYPES: 300000, diff --git a/ui/apps/pmm/src/pages/settings/Settings.constants.ts b/ui/apps/pmm/src/pages/settings/Settings.constants.ts index d0458825117..384203d8b4e 100644 --- a/ui/apps/pmm/src/pages/settings/Settings.constants.ts +++ b/ui/apps/pmm/src/pages/settings/Settings.constants.ts @@ -1,3 +1 @@ export const MAX_LABEL_WIDTH = 750; - -export const PERCONA_SUPPORT_URL = 'https://www.percona.com/services/support'; diff --git a/ui/apps/pmm/src/pages/settings/Settings.messages.ts b/ui/apps/pmm/src/pages/settings/Settings.messages.ts index 0213e5ab456..c2222d00521 100644 --- a/ui/apps/pmm/src/pages/settings/Settings.messages.ts +++ b/ui/apps/pmm/src/pages/settings/Settings.messages.ts @@ -1,4 +1,4 @@ -import { PERCONA_SUPPORT_URL } from './Settings.constants'; +import { PERCONA_SUPPORT_URL } from 'lib/constants'; export const Messages = { title: 'Settings', diff --git a/ui/apps/pmm/src/router.tsx b/ui/apps/pmm/src/router.tsx index 49a556c1f2c..0cb72f7436a 100644 --- a/ui/apps/pmm/src/router.tsx +++ b/ui/apps/pmm/src/router.tsx @@ -21,6 +21,7 @@ import { AlertsPage } from 'pages/alerting/status'; import { AtwApp } from '@sep/plugins-atw'; import { SchemaDrivenPlugin } from '@sep/framework'; import { SepPage } from './sep/SepPage'; +import { ServiceNowSetupGate } from './sep/ServiceNowSetupGate'; // Route paths below are relative to the `PMM_NEW_NAV_PATH` parent, while the // shared SEP constants are absolute (the nav and each plugin's `routeBase` need @@ -90,14 +91,20 @@ const router = createBrowserRouter( // SEP apps mounted as native routes. Both plugins compose their own // , so the paths are splats. { - // ATW ("Collect Diagnostic Data") is now incident-first: AtwApp + // ATW ("Support diagnostics") is now incident-first: AtwApp // composes its own (incident list at index, workspace at // :incidentId), so this must be a splat. Backend API calls hit // /apps/atw and the incident / batch-execution endpoints. + // + // Everything the app does ends in a ServiceNow upload, so it sits + // behind the connection gate — inside SepAuthGate, since reading + // the SEP settings needs the exchanged bearer. path: `${relativeToNav(SEP_ATW_PATH)}/*`, element: ( - + + + ), }, diff --git a/ui/apps/pmm/src/sep/SepPage.tsx b/ui/apps/pmm/src/sep/SepPage.tsx index 605a0e74ad6..23f58bc6eea 100644 --- a/ui/apps/pmm/src/sep/SepPage.tsx +++ b/ui/apps/pmm/src/sep/SepPage.tsx @@ -1,4 +1,5 @@ import { FC, PropsWithChildren } from 'react'; +import Box from '@mui/material/Box'; import Stack from '@mui/material/Stack'; import { Page } from 'components/page'; import { useUser } from 'contexts/user'; @@ -34,7 +35,14 @@ export const SepPage: FC = ({ children }) => { > -
{children}
+ {/* + A flex column that grows, not a plain block: it carries the height + handed down from Page so a plugin (or the ServiceNow setup prompt) + can centre itself in the page rather than in its own content box. + */} + + {children} + diff --git a/ui/apps/pmm/src/sep/ServiceNowSetupGate.messages.ts b/ui/apps/pmm/src/sep/ServiceNowSetupGate.messages.ts new file mode 100644 index 00000000000..51089a939e4 --- /dev/null +++ b/ui/apps/pmm/src/sep/ServiceNowSetupGate.messages.ts @@ -0,0 +1,13 @@ +export const Messages = { + loading: 'Checking the ServiceNow connection…', + heading: 'Support diagnostics', + title: 'Send diagnostics straight to your support cases', + description: + "This tool runs Percona's diagnostic scripts against your monitored environment and uploads the results directly to a case in Percona's ServiceNow, skipping manual collection.", + setUp: 'Set up ServiceNow connection', + consentNote: + 'No data is collected or sent until someone selects a script and confirms.', + howItWorks: 'How Support diagnostics works', + subscriptionPrompt: "Don't have a Percona Support subscription?", + subscriptionLinkText: 'Learn about Percona Support', +}; diff --git a/ui/apps/pmm/src/sep/ServiceNowSetupGate.test.tsx b/ui/apps/pmm/src/sep/ServiceNowSetupGate.test.tsx new file mode 100644 index 00000000000..f0cdc6f7ac8 --- /dev/null +++ b/ui/apps/pmm/src/sep/ServiceNowSetupGate.test.tsx @@ -0,0 +1,127 @@ +import { render, screen } from '@testing-library/react'; +import { + ApiError, + REDACTED_SECRET, + SettingClassGroup, + useSettingsList, +} from '@sep/api'; +import { TestWrapper } from 'utils/testWrapper'; +import { PMM_SERVICENOW_SETTINGS_PATH } from 'lib/constants'; +import { ServiceNowSetupGate } from './ServiceNowSetupGate'; +import { Messages } from './ServiceNowSetupGate.messages'; + +vi.mock('@sep/api', async (importOriginal) => ({ + ...(await importOriginal()), + useSettingsList: vi.fn(), +})); + +const settingsList = vi.mocked(useSettingsList); + +const setting = (key: string, value: unknown, hasOverride = false) => + ({ + key, + value, + has_override: hasOverride, + setting_class: 'SEPSettings', + type: 'object', + }) as unknown as SettingClassGroup['settings'][number]; + +const sepGroups = ( + declared: string[], + storedSecrets?: Record +): SettingClassGroup[] => [ + { + setting_class: 'SEPSettings', + is_app_owned: false, + settings: [ + setting('DIAGNOSTICS_DELIVERY', { + secrets: Object.fromEntries( + declared.map((name) => [name, REDACTED_SECRET]) + ), + }), + setting( + 'DIAGNOSTICS_DELIVERY_INPUTS', + { endpoint: '', secrets: storedSecrets ?? {} }, + storedSecrets !== undefined + ), + ], + }, +]; + +const mockList = ( + overrides: Partial> = {} +) => { + settingsList.mockReturnValue({ + data: undefined, + isLoading: false, + error: null, + ...overrides, + } as ReturnType); +}; + +const renderGate = () => + render( + +
+ , + { wrapper: TestWrapper } + ); + +describe('ServiceNowSetupGate', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('renders the app once the connection is configured', () => { + mockList({ data: sepGroups(['sn_api_key'], { sn_api_key: 'secret' }) }); + renderGate(); + + expect(screen.getByTestId('atw-app')).toBeInTheDocument(); + expect( + screen.queryByTestId('servicenow-setup-prompt') + ).not.toBeInTheDocument(); + }); + + it('prompts for setup when nothing is stored', () => { + mockList({ data: sepGroups(['sn_api_key']) }); + renderGate(); + + expect(screen.getByTestId('servicenow-setup-prompt')).toBeInTheDocument(); + expect(screen.getByText(Messages.title)).toBeInTheDocument(); + expect(screen.queryByTestId('atw-app')).not.toBeInTheDocument(); + }); + + it('prompts for setup when the stored values no longer match the plan', () => { + mockList({ data: sepGroups(['sn_token'], { sn_api_key: 'secret' }) }); + renderGate(); + + expect(screen.getByTestId('servicenow-setup-prompt')).toBeInTheDocument(); + }); + + it('links the call to action at the ServiceNow settings tab', () => { + mockList({ data: sepGroups(['sn_api_key']) }); + renderGate(); + + expect(screen.getByTestId('servicenow-setup-cta')).toHaveAttribute( + 'href', + PMM_SERVICENOW_SETTINGS_PATH + ); + }); + + it('renders the app when the settings read failed', () => { + mockList({ + error: new ApiError({ kind: 'network', message: 'down' }), + }); + renderGate(); + + expect(screen.getByTestId('atw-app')).toBeInTheDocument(); + }); + + it('waits while the settings are loading', () => { + mockList({ isLoading: true }); + renderGate(); + + expect(screen.getByLabelText(Messages.loading)).toBeInTheDocument(); + expect(screen.queryByTestId('atw-app')).not.toBeInTheDocument(); + }); +}); diff --git a/ui/apps/pmm/src/sep/ServiceNowSetupGate.tsx b/ui/apps/pmm/src/sep/ServiceNowSetupGate.tsx new file mode 100644 index 00000000000..ddccd05f15e --- /dev/null +++ b/ui/apps/pmm/src/sep/ServiceNowSetupGate.tsx @@ -0,0 +1,116 @@ +import { FC, PropsWithChildren } from 'react'; +import Box from '@mui/material/Box'; +import Button from '@mui/material/Button'; +import CircularProgress from '@mui/material/CircularProgress'; +import Link from '@mui/material/Link'; +import Stack from '@mui/material/Stack'; +import Typography from '@mui/material/Typography'; +import { Link as RouterLink } from 'react-router-dom'; +import { + PERCONA_SUPPORT_URL, + PMM_SERVICENOW_SETTINGS_PATH, + SUPPORT_DIAGNOSTICS_DOCS_URL, +} from 'lib/constants'; +import { useServiceNowConnection } from 'pages/settings/components/servicenow/ServiceNowConnection.hooks'; +import { Messages } from './ServiceNowSetupGate.messages'; + +/** + * What an operator sees instead of the app while delivery is unconfigured: + * what the tool does, where to connect it, and what it will not do unasked. + */ +const SetupPrompt: FC = () => ( + + {Messages.heading} + + + + {Messages.title} + {Messages.description} + + + + + + + {Messages.consentNote}{' '} + + {Messages.howItWorks} + + + + {Messages.subscriptionPrompt} +
+ + {Messages.subscriptionLinkText} + + + + + + +); + +/** + * Holds the Support diagnostics app until ServiceNow delivery is configured. + * + * Anything the app can do ends in an upload to a ServiceNow case, so an + * unconfigured deployment has nothing to offer but a dead end — the prompt + * replaces the app rather than sitting beside it, and points at the settings + * tab that fixes it. + * + * A settings read that failed says nothing about the connection, so it does + * *not* produce the prompt: the app renders and reports its own failures, which + * is better than telling an operator with a working connection to go configure + * one. `drifted` does gate — SEP holds values the current delivery plan no + * longer accepts, so delivery is as broken as if nothing were stored. + */ +export const ServiceNowSetupGate: FC = ({ children }) => { + const { status, isLoading, error } = useServiceNowConnection(); + + if (isLoading) { + return ( + + + + ); + } + + if (error || status === 'configured') { + return <>{children}; + } + + return ; +}; From d603cde873930c928e64defc367b36bb7a440f78 Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Thu, 13 Aug 2026 14:00:35 -0300 Subject: [PATCH 14/23] PMM-15337 Rename nav entry and swap its icon "Collect Diagnostic Data" described the mechanism; "Support diagnostics" describes what it is for. The icon follows. Signed-off-by: Ignacio Durand --- ui/apps/pmm/src/contexts/navigation/navigation.utils.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ui/apps/pmm/src/contexts/navigation/navigation.utils.tsx b/ui/apps/pmm/src/contexts/navigation/navigation.utils.tsx index ef349fb39d3..190c60ca7a6 100644 --- a/ui/apps/pmm/src/contexts/navigation/navigation.utils.tsx +++ b/ui/apps/pmm/src/contexts/navigation/navigation.utils.tsx @@ -1,6 +1,6 @@ import DarkModeOutlined from '@mui/icons-material/DarkModeOutlined'; import LightModeOutlined from '@mui/icons-material/LightModeOutlined'; -import MonitorHeartIcon from '@mui/icons-material/MonitorHeart'; +import MedicalServicesOutlinedIcon from '@mui/icons-material/MedicalServicesOutlined'; import { MySqlIcon } from '@percona/peak-ui'; import { NavItem } from 'types/navigation.types'; import { ServiceType } from 'types/services.types'; @@ -306,8 +306,8 @@ export const addHomePage = (preferences?: UserPreferences): NavItem => { export const addSepApps = (): NavItem[] => [ { id: 'sep-atw', - text: 'Collect Diagnostic Data', - icon: MonitorHeartIcon, + text: 'Support diagnostics', + icon: MedicalServicesOutlinedIcon, url: SEP_ATW_PATH, matches: [SEP_ATW_PATH], }, From 66c93215488a41fbef3c129e74a0b8e83311bdf8 Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Thu, 13 Aug 2026 14:00:35 -0300 Subject: [PATCH 15/23] PMM-15337 Guard the New incident button MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Heading follows the rename. The create action is withheld once the list request has failed — creating would hit the backend that just failed and only produce a second error the user cannot act on — and disabled while the list is still loading. Signed-off-by: Ignacio Durand --- .../plugins/atw/src/IncidentListPage.tsx | 32 ++++++++++++------- .../atw/tests/IncidentListPage.test.tsx | 27 ++++++++++++++++ 2 files changed, 47 insertions(+), 12 deletions(-) diff --git a/ui/packages/plugins/atw/src/IncidentListPage.tsx b/ui/packages/plugins/atw/src/IncidentListPage.tsx index ed070a95496..e9cfd775f7d 100644 --- a/ui/packages/plugins/atw/src/IncidentListPage.tsx +++ b/ui/packages/plugins/atw/src/IncidentListPage.tsx @@ -124,18 +124,26 @@ export function IncidentListPage() { justifyContent="space-between" sx={{ mb: 1 }} > - Collect Diagnostic Data - + Support diagnostics + {/* + Withheld while the list is unavailable: a create would hit the same + backend that just failed, so offering it only produces a second error + on top of one the user cannot act on. + */} + {!error && ( + + )} Open an incident to run diagnostic snippets and review their results in diff --git a/ui/packages/plugins/atw/tests/IncidentListPage.test.tsx b/ui/packages/plugins/atw/tests/IncidentListPage.test.tsx index 4509c7ece90..231d5a57b49 100644 --- a/ui/packages/plugins/atw/tests/IncidentListPage.test.tsx +++ b/ui/packages/plugins/atw/tests/IncidentListPage.test.tsx @@ -88,6 +88,33 @@ describe('IncidentListPage', () => { }); }); + it('withholds the create button when the list request failed', async () => { + mockedApi.get.mockRejectedValue(new Error('Internal Server Error')); + + renderPage(); + + await waitFor(() => { + expect( + screen.getByText(/Failed to load incidents: Internal Server Error/) + ).toBeTruthy(); + }); + // Creating would hit the backend that just failed, so the action is gone + // rather than merely disabled. + expect(screen.queryByRole('button', { name: /New incident/i })).toBeNull(); + }); + + it('disables the create button until the list has loaded', async () => { + mockedApi.get.mockReturnValue(new Promise(() => {})); + + renderPage(); + + await waitFor(() => { + expect( + screen.getByRole('button', { name: /New incident/i }) + ).toBeDisabled(); + }); + }); + it('creates an incident from the dialog, sending the trimmed name', async () => { mockedApi.get.mockResolvedValue(paginated([])); mockedApi.post.mockResolvedValue({ data: incident }); From 52b4f1d3a6bfd5388a8144ac4c6c8d69b74bc4c1 Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Thu, 13 Aug 2026 16:49:16 -0300 Subject: [PATCH 16/23] PMM-15337 Fail open when SEP lacks the delivery key A SEP build whose settings carry no DIAGNOSTICS_DELIVERY_INPUTS key read as "not configured", so the gate sent the operator to a settings tab that can only answer that it is unavailable. Treat a missing key like a failed read and let the app render. Signed-off-by: Ignacio Durand --- .../pmm/src/sep/ServiceNowSetupGate.test.tsx | 18 ++++++++++++++++++ ui/apps/pmm/src/sep/ServiceNowSetupGate.tsx | 12 ++++++++---- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/ui/apps/pmm/src/sep/ServiceNowSetupGate.test.tsx b/ui/apps/pmm/src/sep/ServiceNowSetupGate.test.tsx index f0cdc6f7ac8..944698ddfd1 100644 --- a/ui/apps/pmm/src/sep/ServiceNowSetupGate.test.tsx +++ b/ui/apps/pmm/src/sep/ServiceNowSetupGate.test.tsx @@ -108,6 +108,24 @@ describe('ServiceNowSetupGate', () => { ); }); + it('renders the app when SEP does not carry the delivery inputs key', () => { + mockList({ + data: [ + { + setting_class: 'SEPSettings', + is_app_owned: false, + settings: [setting('DIAGNOSTICS_DELIVERY', { secrets: {} })], + }, + ] as SettingClassGroup[], + }); + renderGate(); + + expect(screen.getByTestId('atw-app')).toBeInTheDocument(); + expect( + screen.queryByTestId('servicenow-setup-prompt') + ).not.toBeInTheDocument(); + }); + it('renders the app when the settings read failed', () => { mockList({ error: new ApiError({ kind: 'network', message: 'down' }), diff --git a/ui/apps/pmm/src/sep/ServiceNowSetupGate.tsx b/ui/apps/pmm/src/sep/ServiceNowSetupGate.tsx index ddccd05f15e..99cea37f3a2 100644 --- a/ui/apps/pmm/src/sep/ServiceNowSetupGate.tsx +++ b/ui/apps/pmm/src/sep/ServiceNowSetupGate.tsx @@ -86,11 +86,15 @@ const SetupPrompt: FC = () => ( * A settings read that failed says nothing about the connection, so it does * *not* produce the prompt: the app renders and reports its own failures, which * is better than telling an operator with a working connection to go configure - * one. `drifted` does gate — SEP holds values the current delivery plan no - * longer accepts, so delivery is as broken as if nothing were stored. + * one. A SEP build whose settings carry no `DIAGNOSTICS_DELIVERY_INPUTS` key at + * all is the same case for a different reason — the prompt would send the + * operator to a settings tab that can only answer that it is unavailable, so + * the prompt is worse than useless there. `drifted` does gate — SEP holds + * values the current delivery plan no longer accepts, so delivery is as broken + * as if nothing were stored. */ export const ServiceNowSetupGate: FC = ({ children }) => { - const { status, isLoading, error } = useServiceNowConnection(); + const { status, stored, isLoading, error } = useServiceNowConnection(); if (isLoading) { return ( @@ -108,7 +112,7 @@ export const ServiceNowSetupGate: FC = ({ children }) => { ); } - if (error || status === 'configured') { + if (error || !stored.isPresent || status === 'configured') { return <>{children}; } From f7310ce26f410753008e964e47aa5fa5188e5120 Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Wed, 19 Aug 2026 16:46:21 -0300 Subject: [PATCH 17/23] PMM-15293 Drop the invented platform name from SEP errors The SEP auth gate named a "Smart Expert Platform" that does not exist. Rephrase the blocked and notice copy around what the user can act on - the page cannot load, their work is kept - and refer to the backend as the support platform. Also cancel the negative right margin MUI puts on an Alert's action slot, which left Try again hanging past the alert's padding. Signed-off-by: Ignacio Durand --- ui/apps/pmm/src/sep/SepAuthGate.messages.ts | 12 ++++++------ ui/apps/pmm/src/sep/SepAuthGate.test.tsx | 2 +- ui/apps/pmm/src/sep/SepAuthGate.tsx | 1 + 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/ui/apps/pmm/src/sep/SepAuthGate.messages.ts b/ui/apps/pmm/src/sep/SepAuthGate.messages.ts index 144ca5c8ab1..d5853cbff6a 100644 --- a/ui/apps/pmm/src/sep/SepAuthGate.messages.ts +++ b/ui/apps/pmm/src/sep/SepAuthGate.messages.ts @@ -1,22 +1,22 @@ export const Messages = { - loading: 'Authenticating with Smart Expert Platform…', + loading: 'Authenticating with the support platform…', retry: 'Try again', // Shown instead of the page: the exchange failed at load, so there is no work // in progress to preserve. blocked: { signedOutTitle: 'Not signed in', signedOut: - 'Smart Expert Platform could not verify your PMM session. Sign in to PMM again, then retry.', - unreachableTitle: 'Could not reach Smart Expert Platform', + "This page can't be loaded because your PMM session could not be verified. Sign in to PMM again, then retry.", + unreachableTitle: "This page can't be loaded", unreachable: - 'Authenticating with Smart Expert Platform failed. This is usually temporary.', + "This page can't be loaded because the support platform can't be reached right now. This is usually temporary.", }, // Shown beside a page that is already open. Never replaces it — the user may // be part-way through a form. notice: { signedOut: - 'Your PMM session has ended, so Smart Expert Platform can no longer be reached. Anything you submit from this page will fail. Sign in to PMM in another tab, then retry — your work here is kept.', + 'Your PMM session has ended, so the support platform can no longer be reached. Anything you submit from this page will fail. Sign in to PMM in another tab, then retry — your work here is kept.', unreachable: - 'Lost the connection to Smart Expert Platform. Anything you submit from this page will fail until it is back. Your work here is kept.', + 'Lost the connection to the support platform. Anything you submit from this page will fail until it is back. Your work here is kept.', }, }; diff --git a/ui/apps/pmm/src/sep/SepAuthGate.test.tsx b/ui/apps/pmm/src/sep/SepAuthGate.test.tsx index 3dd2f8a5fa2..3af058c2c03 100644 --- a/ui/apps/pmm/src/sep/SepAuthGate.test.tsx +++ b/ui/apps/pmm/src/sep/SepAuthGate.test.tsx @@ -91,7 +91,7 @@ describe('SepAuthGate — bootstrap', () => { renderGate(); expect(await screen.findByTestId('sep-auth-error')).toHaveTextContent( - 'Could not reach Smart Expert Platform' + "This page can't be loaded" ); }); diff --git a/ui/apps/pmm/src/sep/SepAuthGate.tsx b/ui/apps/pmm/src/sep/SepAuthGate.tsx index 868ac2fe5fa..1bb21705ea2 100644 --- a/ui/apps/pmm/src/sep/SepAuthGate.tsx +++ b/ui/apps/pmm/src/sep/SepAuthGate.tsx @@ -19,6 +19,7 @@ const RetryButton: FC = () => ( )} - {resolvedExecuteActions.map((action) => ( - - ))} + {canMutate && + resolvedExecuteActions.map((action) => ( + + ))} - {hasStoredForm ? ( + {canMutate && + (hasStoredForm ? ( + + ) : ( + + + + + + ))} + + + + {canMutate && ( - ) : ( - - - - - )} - - - - >(); const id = (detailIdParam && params[detailIdParam]) ?? params.id; const entityName = detailEntityName ?? params.entityName; @@ -1128,7 +1141,7 @@ export function PluginDetailPage({ // SchemaListView's status-cell fallback. ) : null} - {multi && !browseOnly && ( + {multi && !browseOnly && canMutate && ( <> )} - {!hideCreate && ( + {!hideCreate && canMutate && ( + ); + + async function submitAndGetOnError() { + const user = userEvent.setup(); + renderPage(); + await user.click(screen.getByRole('button', { name: 'Create Checksum' })); + await waitFor(() => expect(mockCreateTaskMutate).toHaveBeenCalledTimes(1)); + return mockCreateTaskMutate.mock.calls[0][1].onError as ( + error: unknown + ) => void; + } + + it("banners a refusal with the server's own reason, with no toast alongside it", async () => { + const onError = await submitAndGetOnError(); + + act(() => + onError( + new ApiError({ + kind: 'http', + status: 403, + message: "You don't have permission to perform this action", + }) + ) + ); + + await waitFor(() => expect(inTreeAlerts()).toHaveLength(1)); + expect(inTreeAlerts()[0]).toHaveTextContent( + "You don't have permission to perform this action" + ); + // The banner is the only signal: no error toast is raised alongside it. + expect(screen.queryAllByRole('alert')).toHaveLength(1); + }); + + it('keeps the per-field path for a 422', async () => { + const onError = await submitAndGetOnError(); + + act(() => + onError( + new ApiError({ + kind: 'http', + status: 422, + message: 'HTTP 422', + data: { + detail: [ + { + loc: ['body', 'count'], + msg: 'ensure this value is greater than 0', + }, + ], + }, + }) + ) + ); + + await waitFor(() => expect(inTreeAlerts()).toHaveLength(1)); + expect(inTreeAlerts()[0]).toHaveTextContent( + 'ensure this value is greater than 0' + ); + }); + + it('shows no banner on a successful create', async () => { + const user = userEvent.setup(); + renderPage({ renderCreateForm: submitSlot }); + + await user.click(screen.getByRole('button', { name: 'Submit slot' })); + await waitFor(() => expect(mockCreateTaskMutate).toHaveBeenCalledTimes(1)); + act(() => + mockCreateTaskMutate.mock.calls[0][1].onSuccess({ name: 'my task' }) + ); + + expect(mockNavigate).toHaveBeenCalledWith('..', { relative: 'path' }); + expect(inTreeAlerts()).toEqual([]); + }); +}); + describe('PluginCreatePage — write access', () => { it('renders the create form for a session that may mutate', () => { renderPage(); diff --git a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginCreatePage.tsx b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginCreatePage.tsx index df5913a7d62..21ca3c82ec5 100644 --- a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginCreatePage.tsx +++ b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginCreatePage.tsx @@ -114,12 +114,12 @@ export function PluginCreatePage({ navigate('..', { relative: 'path' }); }, onError: (error: unknown) => { - const message = - error instanceof Error ? error.message : 'Failed to create'; - // Transient toast is unchanged; 422s additionally map to a persistent - // banner plus inline per-field errors. - enqueueSnackbar(message, { variant: 'error' }); - setSubmitErrorState(mapSubmitError(error, sections, message)); + // Reported by the form's own persistent banner (plus inline per-field + // errors for a 422) and by nothing else: one signal per failure, and one + // that does not depend on the host mounting a snackbar provider. + setSubmitErrorState( + mapSubmitError(error, sections, 'Failed to create') + ); }, }); }; diff --git a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginDetailPage.test.tsx b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginDetailPage.test.tsx index 072cb598176..d50eca2749d 100644 --- a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginDetailPage.test.tsx +++ b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginDetailPage.test.tsx @@ -21,7 +21,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { MemoryRouter, Routes, Route } from 'react-router-dom'; import { SnackbarProvider } from 'notistack'; -import type { PluginSchema } from '@sep/api'; +import { ApiError, type PluginSchema } from '@sep/api'; import { PluginDetailPage, resolveTabFromSplat, @@ -29,6 +29,7 @@ import { } from './PluginDetailPage'; const mockDeleteMutate = vi.fn(); +const mockDeleteEntityMutate = vi.fn(); const mockExecuteMutate = vi.fn(); const mockUsePluginTask = vi.fn(); const mockUsePluginEntityDetail = vi.fn(); @@ -73,7 +74,11 @@ vi.mock('@sep/api', () => ({ mutateAsync: mockDeleteMutate, isPending: false, }), - useDeletePluginEntity: () => ({ mutateAsync: vi.fn(), isPending: false }), + useDeletePluginEntity: () => ({ + mutate: mockDeleteEntityMutate, + mutateAsync: vi.fn(), + isPending: false, + }), usePluginEntityDetail: (...args: unknown[]) => mockUsePluginEntityDetail(...args), // Needed by useTaskLogs / useExecutionEvents in the component tree @@ -85,14 +90,20 @@ vi.mock('@sep/api', () => ({ setTokenProvider: vi.fn(), ApiError: class ApiError extends Error { status?: number; - constructor(details: { status?: number; message: string }) { + data?: unknown; + constructor(details: { status?: number; message: string; data?: unknown }) { super(details.message); this.status = details.status; + this.data = details.data; } }, + // Used by the shared failure-reporting primitive to read a 422's per-field + // detail array; the flows exercised here never send one. + parseFieldErrors: () => [], })); beforeEach(() => { + mockDeleteEntityMutate.mockReset(); usePluginTasksMock.mockReset(); usePluginTasksMock.mockReturnValue(defaultPluginTasksResult()); mockCanMutate = true; @@ -113,7 +124,12 @@ vi.mock('../../hooks', () => ({ mutateAsync: mockExecuteMutate, isPending: false, }), - useStopTaskHistory: () => ({ mutate: stopMutate, isPending: false }), + useStopTaskHistory: () => ({ + mutate: stopMutate, + isPending: false, + error: null, + reset: vi.fn(), + }), })); // Execution History tab renders the real TaskHistoryTable; stub it to capture @@ -461,7 +477,7 @@ describe('PluginDetailPage execute flow', () => { ); }); - it('shows error snackbar and keeps dialog open on execute failure', async () => { + it('closes the dialog and reports the failure on the page behind it', async () => { mockExecuteMutate.mockReset(); mockExecuteMutate.mockRejectedValue(new Error('Execute failed')); mockUsePluginTask.mockReturnValue({ @@ -479,9 +495,42 @@ describe('PluginDetailPage execute flow', () => { ); await waitFor(() => - expect(screen.getByText('Execute failed')).toBeInTheDocument() + expect(screen.getByTestId('plugin-task-action-error')).toHaveTextContent( + 'Execute failed' + ) + ); + await waitFor(() => + expect(screen.queryByRole('dialog')).not.toBeInTheDocument() + ); + }); + + it("reports a refusal with the server's own reason", async () => { + mockExecuteMutate.mockReset(); + mockExecuteMutate.mockRejectedValue( + new ApiError({ + kind: 'http', + status: 403, + message: "You don't have permission to perform this action", + }) + ); + mockUsePluginTask.mockReturnValue({ + data: { id: 1, name: 'FECHK', status: 'completed' }, + isLoading: false, + }); + + renderAt('/apps/checksums/task/FECHK'); + + await userEvent.click(screen.getByTestId('plugin-task-execute')); + const dialog = await screen.findByRole('dialog'); + await userEvent.click( + within(dialog).getByTestId('plugin-task-execute-confirm') + ); + + await waitFor(() => + expect(screen.getByTestId('plugin-task-action-error')).toHaveTextContent( + "You don't have permission to perform this action" + ) ); - expect(screen.getByRole('dialog')).toBeInTheDocument(); }); it('closes dialog without calling execute when cancelled', async () => { @@ -1012,7 +1061,7 @@ describe('PluginDetailPage — execute chain composition', () => { ).not.toBeInTheDocument(); }); - it('keeps dialog open with composed chain when execute fails', async () => { + it('closes the dialog and reports the failure when a chained execute fails', async () => { const user = userEvent.setup(); mockExecuteMutate.mockRejectedValue(new Error('Chain contains a cycle')); renderWithSchema(makeSchema({ chaining: true })); @@ -1020,11 +1069,23 @@ describe('PluginDetailPage — execute chain composition', () => { const dialog = await screen.findByRole('dialog'); await addTaskToChain(dialog, 'other-task'); await user.click(within(dialog).getByTestId('plugin-task-execute-confirm')); - await screen.findByText(/Chain contains a cycle/); - expect(screen.getByRole('dialog')).toBeInTheDocument(); - expect( - within(screen.getByRole('dialog')).getByTestId('chain-sequence') - ).toHaveTextContent('other-task'); + + await waitFor(() => + expect(screen.getByTestId('plugin-task-action-error')).toHaveTextContent( + /Chain contains a cycle/ + ) + ); + await waitFor(() => + expect(screen.queryByRole('dialog')).not.toBeInTheDocument() + ); + + // Reopening the same action keeps the composed chain, so the retry does not + // start from an empty builder. + await user.click(screen.getByTestId('plugin-task-execute')); + const reopened = await screen.findByRole('dialog'); + expect(within(reopened).getByTestId('chain-sequence')).toHaveTextContent( + 'other-task' + ); }); }); @@ -1575,6 +1636,131 @@ describe('PluginDetailPage delete flow', () => { await waitFor(() => expect(screen.getByText('list page')).toBeInTheDocument() ); + expect( + screen.queryByTestId('plugin-task-action-error') + ).not.toBeInTheDocument(); + }); + + it("closes the dialog and reports a refused delete with the server's own reason", async () => { + mockDeleteMutate.mockReset(); + mockDeleteMutate.mockRejectedValue( + new ApiError({ + kind: 'http', + status: 403, + message: "You don't have permission to perform this action", + }) + ); + mockUsePluginTask.mockReturnValue({ + data: { id: 1, name: 'check1', status: 'completed' }, + isLoading: false, + }); + + renderAt('/apps/checksums/task/check1'); + + await userEvent.click(screen.getByTestId('plugin-task-delete')); + const dialog = await screen.findByRole('dialog'); + await userEvent.click( + within(dialog).getByRole('button', { name: 'Delete' }) + ); + + expect( + await screen.findByTestId('plugin-task-action-error') + ).toHaveTextContent("You don't have permission to perform this action"); + await waitFor(() => + expect(screen.queryByRole('dialog')).not.toBeInTheDocument() + ); + // The alert replaces the previous error toast rather than joining it. + expect(document.querySelector('[class*="notistack"]')).toBeNull(); + }); +}); + +describe('PluginDetailPage entity delete flow', () => { + const entitySchema = { + pluginName: 'inventory', + display_name: 'Inventory', + description: 'Test', + capabilities: {}, + entities: [ + { + name: 'nodes', + display_name: 'Nodes', + forms: [], + list_view: { columns: [{ key: 'name', label: 'Name' }] }, + }, + ], + } as unknown as PluginSchema; + + function renderEntityDetail() { + mockUsePluginEntityDetail.mockReturnValue({ + data: { id: 5, name: 'node-a' }, + isLoading: false, + }); + return render( + + + + + + } + /> + list page
} + /> + + + + + ); + } + + async function confirmEntityDelete() { + await userEvent.click(screen.getByRole('button', { name: 'Delete' })); + const dialog = await screen.findByRole('dialog'); + await userEvent.click( + within(dialog).getByRole('button', { name: 'Delete' }) + ); + } + + it("reports a refused delete on the detail page with the server's own reason", async () => { + mockDeleteEntityMutate.mockImplementation((_id, opts) => + opts.onError?.( + new ApiError({ + kind: 'http', + status: 403, + message: "You don't have permission to perform this action", + }) + ) + ); + + renderEntityDetail(); + await confirmEntityDelete(); + + expect( + await screen.findByTestId('entity-detail-action-error') + ).toHaveTextContent("You don't have permission to perform this action"); + }); + + it('reports nothing when the delete succeeds', async () => { + mockDeleteEntityMutate.mockImplementation((_id, opts) => + opts.onSuccess?.() + ); + + renderEntityDetail(); + await confirmEntityDelete(); + + await waitFor(() => + expect(mockDeleteEntityMutate).toHaveBeenCalledTimes(1) + ); + expect( + screen.queryByTestId('entity-detail-action-error') + ).not.toBeInTheDocument(); }); }); diff --git a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginDetailPage.tsx b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginDetailPage.tsx index e76d61349d1..7ec4987fcb2 100644 --- a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginDetailPage.tsx +++ b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginDetailPage.tsx @@ -70,6 +70,7 @@ import { type SepComponents, } from '@sep/api'; import { resolvePath } from '../../utils/resolvePath'; +import { ActionErrorAlert, useActionError } from '../ActionErrorAlert'; import { TaskHistoryTable, TaskHistoryStatusBadge, @@ -639,6 +640,8 @@ function LogsTab({ taskNames }: LogsTabProps) { } }} isStopping={stop.isPending} + actionError={stop.error} + onDismissActionError={stop.reset} /> )} @@ -696,10 +699,15 @@ function ActionBar({ const { enqueueSnackbar } = useSnackbar(); const deleteTask = useDeletePluginTask(pluginName); const executeTask = useExecuteTask(pluginName); + // Both actions are confirmed in a dialog that closes before the request + // settles, so the failure is held here and rendered on the page behind it. + const actionError = useActionError(); const [confirmOpen, setConfirmOpen] = useState(false); const [pendingExecute, setPendingExecute] = useState(null); const [chain, setChain] = useState(emptyChain); + /** Which execute action the current `chain` was composed for. */ + const [chainActionKey, setChainActionKey] = useState(null); const chainingEnabled = !!schema.capabilities?.chaining; const { @@ -717,8 +725,19 @@ function ActionBar({ ); useEffect(() => { - setChain(emptyChain()); - }, [pendingExecute]); + // Opening a different action starts from an empty chain; reopening the same + // one keeps what was composed, so a refused execute can be retried without + // rebuilding the chain. Closing the dialog (which now happens on failure + // too) leaves the chain alone for that retry. + if (!pendingExecute) { + return; + } + const key = `${pendingExecute.taskName}\u0000${pendingExecute.label}`; + if (key !== chainActionKey) { + setChain(emptyChain()); + setChainActionKey(key); + } + }, [pendingExecute, chainActionKey]); const resolvedExecuteActions = executeActions ?? @@ -743,6 +762,7 @@ function ActionBar({ chain_on_failure: chain.chain_on_failure, }; } + actionError.clearError(); try { const executeArgs = executeBody ? { taskName: pendingExecute.taskName, executeBody } @@ -754,18 +774,17 @@ function ActionBar({ variant: 'success', } ); - setPendingExecute(null); } catch (e) { - enqueueSnackbar( - e instanceof Error ? e.message : 'Failed to execute task', - { - variant: 'error', - } - ); + actionError.reportError(e); + } finally { + // Close on confirm whatever the outcome, like the adjacent delete: a + // dialog left open holding a failure hides the message rendered behind it. + setPendingExecute(null); } }; const handleDelete = async () => { + actionError.clearError(); try { await deleteTask.mutateAsync(taskName); enqueueSnackbar(`${schema.display_name} task deleted`, { @@ -776,12 +795,7 @@ function ActionBar({ // sub-route via nested ``), so use an absolute path. navigate(routeBase); } catch (e) { - enqueueSnackbar( - e instanceof Error ? e.message : 'Failed to delete task', - { - variant: 'error', - } - ); + actionError.reportError(e); } finally { setConfirmOpen(false); } @@ -865,6 +879,13 @@ function ActionBar({ )} + + { @@ -1026,13 +1047,18 @@ export function PluginDetailPage({ ); const [entityDeleteOpen, setEntityDeleteOpen] = useState(false); + // The confirm dialog closes on confirm, so a refusal has to land on the + // detail page the user is returned to. + const deleteEntityError = useActionError(); const confirmEntityDelete = () => { setEntityDeleteOpen(false); if (!id || !multi) { return; } + deleteEntityError.clearError(); deleteEntity.mutate(id, { + onError: (error) => deleteEntityError.reportError(error), onSuccess: () => customParentPath ? navigate(customParentPath) @@ -1100,6 +1126,13 @@ export function PluginDetailPage({ } /> + + {headingWhenChromeHidden ? ( . */ -import { render, screen } from '@testing-library/react'; +import { act, render, screen, waitFor, within } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { MemoryRouter, Route, Routes } from 'react-router-dom'; import { SnackbarProvider } from 'notistack'; import type { PluginSchema } from '@sep/api'; import { PluginListPage } from './PluginListPage'; -const { schemaListViewMock, authMock } = vi.hoisted(() => ({ +const { schemaListViewMock, deleteEntityMock, authMock } = vi.hoisted(() => ({ schemaListViewMock: vi.fn(), + deleteEntityMock: vi.fn(), /** Flipped per test to cover the read-only (non-admin) rendering. */ authMock: { canMutate: true }, })); @@ -52,14 +54,33 @@ vi.mock('@sep/api', () => ({ isLoading: false, }), useDeletePluginEntity: () => ({ - mutate: vi.fn(), + mutate: deleteEntityMock, isPending: false, variables: undefined, }), + ApiError: class ApiError extends Error { + status?: number; + data?: unknown; + constructor(details: { status?: number; message: string; data?: unknown }) { + super(details.message); + this.status = details.status; + this.data = details.data; + } + }, + parseFieldErrors: (error: { data?: { detail?: unknown } }) => + Array.isArray(error?.data?.detail) + ? (error.data.detail as { loc?: string[]; msg?: string }[]).map( + (entry) => ({ + path: (entry.loc ?? []).filter((seg) => seg !== 'body').join('.'), + message: entry.msg ?? 'Invalid value', + }) + ) + : [], })); beforeEach(() => { schemaListViewMock.mockClear(); + deleteEntityMock.mockReset(); authMock.canMutate = true; }); @@ -165,3 +186,85 @@ describe('PluginListPage — write access', () => { expect(lastListViewProps().onDeleteRow).toBeUndefined(); }); }); + +describe('PluginListPage — delete failure reporting', () => { + const deletableEntitySchema = { + name: 'inventory', + display_name: 'Inventory', + capabilities: { scheduling: false }, + entities: [ + { + name: 'nodes', + display_name: 'Nodes', + forms: [], + list_view: { + columns: [ + { key: 'name', label: 'Name' }, + { key: '_actions', label: 'Actions', format: 'actions' }, + ], + }, + }, + ], + } as unknown as PluginSchema; + + function renderDeletableEntityList() { + return render( + + + + + } + /> + + + + ); + } + + async function confirmDelete() { + const props = schemaListViewMock.mock.calls.at(-1)?.[0] as { + onDeleteRow?: (row: Record) => void; + }; + act(() => props.onDeleteRow?.({ id: 1, name: 'node-a' })); + const dialog = await screen.findByRole('dialog'); + await userEvent.click( + within(dialog).getByRole('button', { name: 'Delete' }) + ); + } + + it("reports a refused delete on the list with the server's own reason", async () => { + deleteEntityMock.mockImplementation((_id, opts) => + opts.onError?.( + new Error("You don't have permission to perform this action") + ) + ); + renderDeletableEntityList(); + + await confirmDelete(); + + expect( + await screen.findByTestId('plugin-list-action-error') + ).toHaveTextContent("You don't have permission to perform this action"); + // The alert replaces the previous error toast rather than joining it. + expect(document.querySelector('[class*="notistack"]')).toBeNull(); + }); + + it('reports nothing when the delete succeeds', async () => { + deleteEntityMock.mockImplementation((_id, opts) => opts.onSuccess?.()); + renderDeletableEntityList(); + + await confirmDelete(); + + await waitFor(() => expect(deleteEntityMock).toHaveBeenCalledTimes(1)); + expect( + screen.queryByTestId('plugin-list-action-error') + ).not.toBeInTheDocument(); + }); +}); diff --git a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginListPage.tsx b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginListPage.tsx index 84830124dd8..772af51f5a7 100644 --- a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginListPage.tsx +++ b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginListPage.tsx @@ -38,6 +38,7 @@ import { type PluginSchema, type TaskHistoryStatus, } from '@sep/api'; +import { ActionErrorAlert, useActionError } from '../ActionErrorAlert'; import { SchemaListView, type RenderListColumnOverride, @@ -174,6 +175,9 @@ export function PluginListPage({ const hasActionsColumn = listView?.columns.some((c) => c.format === 'actions') ?? false; + // The confirm dialog closes on confirm, so a refusal surfaces on the list + // page the user is left looking at. + const deleteError = useActionError(); const deleteEntity = useDeletePluginEntity( pluginName, entityName ?? '', @@ -208,13 +212,15 @@ export function PluginListPage({ if (!sid) { return; } + deleteError.clearError(); deleteEntity.mutate(sid, { onSuccess: () => { enqueueSnackbar(`${title} deleted`, { variant: 'success' }); }, - onError: (err: Error) => { - enqueueSnackbar(err.message || 'Delete failed', { variant: 'error' }); - }, + // Reported by the list's own alert rather than a toast: the confirm + // dialog is already closed, and the alert does not depend on the host + // application mounting a snackbar provider. + onError: (err: Error) => deleteError.reportError(err), }); }; @@ -228,6 +234,13 @@ export function PluginListPage({ return ( + + {multi && schema.entities && !hideEntityTabs && ( ({ isLoading: false, isError: false, }), + ApiError: class ApiError extends Error { + status?: number; + data?: unknown; + constructor(details: { status?: number; message: string; data?: unknown }) { + super(details.message); + this.status = details.status; + this.data = details.data; + } + }, + parseFieldErrors: (error: { data?: { detail?: unknown } }) => + Array.isArray(error?.data?.detail) + ? (error.data.detail as { loc?: string[]; msg?: string }[]).map( + (entry) => ({ + path: (entry.loc ?? []).filter((seg) => seg !== 'body').join('.'), + message: entry.msg ?? 'Invalid value', + }) + ) + : [], })); const schema: PluginSchema = { @@ -447,6 +465,87 @@ describe('normalizeChoiceDefaults', () => { }); }); +/** Alerts rendered by the page itself, excluding notistack's toast region. */ +function inTreeAlerts(): HTMLElement[] { + return screen + .queryAllByRole('alert') + .filter((el) => !el.className.includes('notistack')); +} + +describe('PluginTaskEditPage — failure reporting', () => { + beforeEach(() => { + mockUsePluginTask.mockReturnValue({ + data: { + name: 'check1', + data: { _form: { task_name: 'check1', title: 'Nightly' } }, + }, + isLoading: false, + }); + }); + + it("banners a refusal with the server's own reason, with no toast alongside it", async () => { + mockUpdateTaskMutate.mockImplementation((_vars, opts) => + opts.onError?.( + new ApiError({ + kind: 'http', + status: 403, + message: "You don't have permission to perform this action", + }) + ) + ); + + renderAt(); + await userEvent.click(screen.getByRole('button', { name: 'Save' })); + + await waitFor(() => expect(inTreeAlerts()).toHaveLength(1)); + expect(inTreeAlerts()[0]).toHaveTextContent( + "You don't have permission to perform this action" + ); + expect(screen.queryAllByRole('alert')).toHaveLength(1); + }); + + it('keeps the per-field path for a 422', async () => { + mockUpdateTaskMutate.mockImplementation((_vars, opts) => + opts.onError?.( + new ApiError({ + kind: 'http', + status: 422, + message: 'HTTP 422', + data: { + detail: [ + { + loc: ['body', 'count'], + msg: 'ensure this value is greater than 0', + }, + ], + }, + }) + ) + ); + + renderAt(); + await userEvent.click(screen.getByRole('button', { name: 'Save' })); + + await waitFor(() => + expect(inTreeAlerts()[0]).toHaveTextContent( + 'ensure this value is greater than 0' + ) + ); + }); + + it('shows no banner on a successful save', async () => { + mockUpdateTaskMutate.mockImplementation((_vars, opts) => + opts.onSuccess?.() + ); + + renderAt(); + await userEvent.click(screen.getByRole('button', { name: 'Save' })); + + await waitFor(() => expect(mockUpdateTaskMutate).toHaveBeenCalledTimes(1)); + expect(inTreeAlerts()).toEqual([]); + }); +}); + describe('PluginTaskEditPage — write access', () => { it('renders the edit form for a session that may mutate', () => { mockUsePluginTask.mockReturnValue({ diff --git a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginTaskEditPage.tsx b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginTaskEditPage.tsx index 086c7c11675..ec0b38c7e57 100644 --- a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginTaskEditPage.tsx +++ b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginTaskEditPage.tsx @@ -183,12 +183,12 @@ export function PluginTaskEditPage({ navigate('..', { relative: 'path' }); }, onError: (error: unknown) => { - const message = - error instanceof Error ? error.message : 'Failed to update'; - // Transient toast is unchanged; 422s additionally map to a persistent - // banner plus inline per-field errors. - enqueueSnackbar(message, { variant: 'error' }); - setSubmitErrorState(mapSubmitError(error, editableSections, message)); + // Reported by the form's own persistent banner (plus inline per-field + // errors for a 422) and by nothing else: one signal per failure, and + // one that does not depend on a host-provided snackbar. + setSubmitErrorState( + mapSubmitError(error, editableSections, 'Failed to update') + ); }, } ); diff --git a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/SchemaDrivenPlugin.test.tsx b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/SchemaDrivenPlugin.test.tsx index c7d97e633ff..5be622d287c 100644 --- a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/SchemaDrivenPlugin.test.tsx +++ b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/SchemaDrivenPlugin.test.tsx @@ -20,7 +20,7 @@ import userEvent from '@testing-library/user-event'; import { afterEach, describe, expect, it, vi } from 'vitest'; import { MemoryRouter, Route, Routes } from 'react-router-dom'; import { SnackbarProvider } from 'notistack'; -import type { PluginSchema } from '@sep/api'; +import { ApiError, type PluginSchema } from '@sep/api'; import { SchemaDrivenPlugin } from './SchemaDrivenPlugin'; import type { RenderFormSlot } from './types'; @@ -140,11 +140,30 @@ vi.mock('@sep/api', () => ({ isError: false, error: null, }), + ApiError: class ApiError extends Error { + status?: number; + data?: unknown; + constructor(details: { status?: number; message: string; data?: unknown }) { + super(details.message); + this.status = details.status; + this.data = details.data; + } + }, + parseFieldErrors: (error: { data?: { detail?: unknown } }) => + Array.isArray(error?.data?.detail) + ? (error.data.detail as { loc?: string[]; msg?: string }[]).map( + (entry) => ({ + path: (entry.loc ?? []).filter((seg) => seg !== 'body').join('.'), + message: entry.msg ?? 'Invalid value', + }) + ) + : [], })); afterEach(() => { activeSchema = schema; mockCanMutate = true; + mockUpdateMutate.mockReset(); }); function renderEdit(renderEditForm?: RenderFormSlot) { @@ -280,6 +299,48 @@ describe('SchemaDrivenPlugin — related_apps routing', () => { }); }); +/** Alerts rendered by the page itself, excluding notistack's toast region. */ +function inTreeAlerts(): HTMLElement[] { + return screen + .queryAllByRole('alert') + .filter((el) => !el.className.includes('notistack')); +} + +describe('SchemaDrivenPlugin — entity edit failure reporting', () => { + it("banners a refusal with the server's own reason, with no toast alongside it", async () => { + const user = userEvent.setup(); + mockUpdateMutate.mockImplementation((_vars, opts) => + opts.onError?.( + new ApiError({ + kind: 'http', + status: 403, + message: "You don't have permission to perform this action", + }) + ) + ); + renderEdit(); + + await user.click(screen.getByRole('button', { name: /^Save Nodes$/ })); + + await waitFor(() => expect(inTreeAlerts()).toHaveLength(1)); + expect(inTreeAlerts()[0]).toHaveTextContent( + "You don't have permission to perform this action" + ); + expect(screen.queryAllByRole('alert')).toHaveLength(1); + }); + + it('shows no banner on a successful save', async () => { + const user = userEvent.setup(); + mockUpdateMutate.mockImplementation((_vars, opts) => opts.onSuccess?.()); + renderEdit(); + + await user.click(screen.getByRole('button', { name: /^Save Nodes$/ })); + + await waitFor(() => expect(mockUpdateMutate).toHaveBeenCalledTimes(1)); + expect(inTreeAlerts()).toEqual([]); + }); +}); + describe('SchemaDrivenPlugin — write access', () => { it('renders the entity edit form for a session that may mutate', () => { renderEdit(); diff --git a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/SchemaDrivenPlugin.tsx b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/SchemaDrivenPlugin.tsx index 54d34a452a4..1ab715b267f 100644 --- a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/SchemaDrivenPlugin.tsx +++ b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/SchemaDrivenPlugin.tsx @@ -194,12 +194,12 @@ function PluginEditPage({ navigate('..', { relative: 'path' }); }, onError: (error: unknown) => { - const message = - error instanceof Error ? error.message : 'Failed to update'; - // Transient toast is unchanged; 422s additionally map to a persistent - // banner plus inline per-field errors. - enqueueSnackbar(message, { variant: 'error' }); - setSubmitErrorState(mapSubmitError(error, sections, message)); + // Reported by the form's own persistent banner (plus inline per-field + // errors for a 422) and by nothing else: one signal per failure, and + // one that does not depend on a host-provided snackbar. + setSubmitErrorState( + mapSubmitError(error, sections, 'Failed to update') + ); }, } ); diff --git a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/submitErrorMapping.test.ts b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/submitErrorMapping.test.ts index adfb24d180d..41bfc270908 100644 --- a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/submitErrorMapping.test.ts +++ b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/submitErrorMapping.test.ts @@ -18,7 +18,7 @@ import { describe, expect, it } from 'vitest'; import { ApiError } from '@sep/api'; import type { FormSection } from '../SchemaFormRenderer/types'; -import { EMPTY_SUBMIT_ERROR, mapSubmitError } from './submitErrorMapping'; +import { mapSubmitError } from './submitErrorMapping'; const SECTIONS: FormSection[] = [ { @@ -40,19 +40,67 @@ function http(status: number, detail?: unknown): ApiError { } describe('mapSubmitError', () => { - it('returns the empty state for non-422 errors (toast-only path unchanged)', () => { - expect(mapSubmitError(http(500), SECTIONS, 'Failed')).toBe( - EMPTY_SUBMIT_ERROR - ); + it("banners a non-422 failure with the server's own reason", () => { expect( mapSubmitError( - new ApiError({ kind: 'network', message: 'x' }), + new ApiError({ + kind: 'http', + status: 403, + message: "You don't have permission to perform this action", + }), SECTIONS, 'Failed' ) - ).toBe(EMPTY_SUBMIT_ERROR); - expect(mapSubmitError(new Error('boom'), SECTIONS, 'Failed')).toBe( - EMPTY_SUBMIT_ERROR + ).toEqual({ + submitError: "You don't have permission to perform this action", + fieldErrors: [], + }); + + expect(mapSubmitError(http(500), SECTIONS, 'Failed')).toEqual({ + submitError: 'HTTP 500', + fieldErrors: [], + }); + expect( + mapSubmitError( + new ApiError({ kind: 'network', message: 'Network error' }), + SECTIONS, + 'Failed' + ) + ).toEqual({ submitError: 'Network error', fieldErrors: [] }); + expect(mapSubmitError(new Error('boom'), SECTIONS, 'Failed')).toEqual({ + submitError: 'boom', + fieldErrors: [], + }); + }); + + it("keeps a 422's string detail rather than substituting the fallback", () => { + expect( + mapSubmitError( + new ApiError({ + kind: 'http', + status: 422, + message: 'Task name already in use', + data: { detail: 'Task name already in use' }, + }), + SECTIONS, + 'Failed to create' + ) + ).toEqual({ submitError: 'Task name already in use', fieldErrors: [] }); + }); + + it('falls back for a 422 that carries no reason at all', () => { + expect(mapSubmitError(http(422), SECTIONS, 'Failed to create')).toEqual({ + submitError: 'Failed to create', + fieldErrors: [], + }); + }); + + it('falls back only when the failure carries no message of its own', () => { + expect(mapSubmitError(new Error(''), SECTIONS, 'Failed to create')).toEqual( + { + submitError: 'Failed to create', + fieldErrors: [], + } ); }); diff --git a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/submitErrorMapping.ts b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/submitErrorMapping.ts index 27550fd603d..01b7dcace19 100644 --- a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/submitErrorMapping.ts +++ b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/submitErrorMapping.ts @@ -20,6 +20,7 @@ import { parseFieldErrors, type FieldValidationError, } from '@sep/api'; +import { actionErrorMessage } from '../ActionErrorAlert'; import { flattenSectionFields } from '../SchemaFormRenderer'; import type { FormSection } from '../SchemaFormRenderer/types'; @@ -40,9 +41,15 @@ export const EMPTY_SUBMIT_ERROR: SubmitErrorState = { /** * Map a mutation error into the persistent banner + per-field error state. * - * Only HTTP 422 responses produce a banner and per-field errors; every other - * failure (network, timeout, 5xx, other HTTP errors) returns the empty state so - * those paths keep their existing toast-only behavior. + * Every failure produces a banner, so a refused or broken submit is reported + * from the form's own tree rather than depending on a host-provided toast. A + * non-422 failure (403 refusal, 409, 5xx, network, timeout) carries the + * server's own reason via `ApiError.message`; only a failure with no message of + * its own falls back to `fallbackMessage`. + * + * HTTP 422 keeps its established path: `detail` is a per-field array that + * `ApiError.message` does not carry, so it is parsed into per-field errors and + * a banner listing them. * * The banner lists every parsed message — labelling each by its form field when * the path resolves to a schema field — so errors that do not map onto a @@ -56,12 +63,21 @@ export function mapSubmitError( fallbackMessage: string ): SubmitErrorState { if (!(error instanceof ApiError) || error.status !== 422) { - return EMPTY_SUBMIT_ERROR; + return { + submitError: actionErrorMessage(error, fallbackMessage), + fieldErrors: [], + }; } const fieldErrors = parseFieldErrors(error); if (fieldErrors.length === 0) { - return { submitError: fallbackMessage, fieldErrors: [] }; + // A 422 whose `detail` is a string rather than the per-field array still + // carries a server reason; `actionErrorMessage` keeps it and falls back + // only for the synthesized `HTTP 422`. + return { + submitError: actionErrorMessage(error, fallbackMessage), + fieldErrors: [], + }; } const labelByPath = new Map(); diff --git a/ui/packages/sep/framework/src/components/SnippetExecutionAccordion/SnippetExecutionAccordion.test.tsx b/ui/packages/sep/framework/src/components/SnippetExecutionAccordion/SnippetExecutionAccordion.test.tsx index 3af664f718d..b4a6c4e1c14 100644 --- a/ui/packages/sep/framework/src/components/SnippetExecutionAccordion/SnippetExecutionAccordion.test.tsx +++ b/ui/packages/sep/framework/src/components/SnippetExecutionAccordion/SnippetExecutionAccordion.test.tsx @@ -42,9 +42,11 @@ vi.mock('../TaskHistoryTable', () => ({ TaskHistoryTable: ({ data, onStopTask, + actionError, }: { data?: TaskHistoryEntry[]; onStopTask?: (entry: TaskHistoryEntry) => void; + actionError?: unknown; }) => (
{data?.[0] && onStopTask ? ( @@ -52,6 +54,13 @@ vi.mock('../TaskHistoryTable', () => ({ Stop {String(data[0].id)} ) : null} + {actionError ? ( +
+ {actionError instanceof Error + ? actionError.message + : String(actionError)} +
+ ) : null}
), })); @@ -369,6 +378,55 @@ describe('SnippetExecutionAccordion', () => { String(call[0]).includes('/snippet/history') ).length; await waitFor(() => expect(historyGets()).toBeGreaterThan(1)); + expect( + screen.queryByTestId('task-history-action-error') + ).not.toBeInTheDocument(); + }); + + it("reports a failed stop above the history with the server's own reason", async () => { + mockedApi.get.mockImplementation((url: string) => + Promise.resolve({ + data: url.includes('/snippet/history') + ? { + items: [ + { + id: 99, + status: 'running', + has_logs: false, + execution_request: { + task: 's', + target: 'h', + meta: {}, + tracking: {}, + }, + task: { id: 1, name: 's' }, + }, + ], + } + : makeSchema(), + }) + ); + mockedApi.post.mockRejectedValue( + new Error("You don't have permission to perform this action") + ); + + renderWithProviders( + + ); + + await userEvent.click( + await screen.findByRole('button', { name: 'Stop 99' }) + ); + + expect( + await screen.findByTestId('task-history-action-error') + ).toHaveTextContent("You don't have permission to perform this action"); }); it('does not render TaskHistoryTable when showHistory is false', async () => { diff --git a/ui/packages/sep/framework/src/components/SnippetExecutionAccordion/SnippetExecutionAccordion.tsx b/ui/packages/sep/framework/src/components/SnippetExecutionAccordion/SnippetExecutionAccordion.tsx index daf6ec0d577..ca2b57d60bf 100644 --- a/ui/packages/sep/framework/src/components/SnippetExecutionAccordion/SnippetExecutionAccordion.tsx +++ b/ui/packages/sep/framework/src/components/SnippetExecutionAccordion/SnippetExecutionAccordion.tsx @@ -296,6 +296,8 @@ export function SnippetExecutionAccordion({ } }} isStopping={stop.isPending} + actionError={stop.error} + onDismissActionError={stop.reset} /> )} diff --git a/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskFilesDialog.test.tsx b/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskFilesDialog.test.tsx index ebb9a7710b2..918bb5a0afd 100644 --- a/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskFilesDialog.test.tsx +++ b/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskFilesDialog.test.tsx @@ -27,6 +27,11 @@ vi.mock('@sep/api', () => ({ get: vi.fn(), }, SEP_BASE_PATH: '/sep', + // Stands in for the real recovery, which pulls axios into the graph this + // manual factory exists to keep out. Every error here is a plain `Error` with + // no blob body, which is exactly the case the real one passes through + // untouched; the parsing path is covered in useTaskFileDownload.test.tsx. + normalizeBlobError: async (error: unknown) => error, })); import { apiClient } from '@sep/api'; diff --git a/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.test.tsx b/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.test.tsx index c3405b6c0c1..077986002e9 100644 --- a/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.test.tsx +++ b/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.test.tsx @@ -48,6 +48,16 @@ vi.mock('@sep/api', () => { }, RUNNING_STATUSES, isRunningStatus: (status: string) => RUNNING_STATUSES.has(status), + // Used by the shared failure-reporting primitive; no 422 detail arrays + // reach the stop endpoint, so an empty parse is enough here. + parseFieldErrors: () => [], + ApiError: class ApiError extends Error { + status?: number; + constructor(details: { status?: number; message: string }) { + super(details.message); + this.status = details.status; + } + }, SEP_BASE_PATH: '/sep', }; }); @@ -510,6 +520,58 @@ describe('TaskHistoryTable connected stop mutation', () => { expect(screen.getByText('Stopped')).toBeInTheDocument() ); expect(mockedApiClient.get.mock.calls.length).toBeGreaterThan(1); + expect( + screen.queryByTestId('task-history-action-error') + ).not.toBeInTheDocument(); + }); + + it("reports a refused stop above the rows with the server's own reason", async () => { + mockedApiClient.get.mockResolvedValue({ + data: { + items: [makeEntry(42, 'running')], + total: 1, + offset: 0, + limit: 10, + }, + }); + mockedApiClient.post.mockRejectedValue( + new Error("You don't have permission to perform this action") + ); + + render( + + + + ); + + await waitFor(() => + expect(screen.getByText('Running')).toBeInTheDocument() + ); + await userEvent.click(screen.getByRole('button', { name: 'Stop task' })); + const dialog = await screen.findByRole('dialog'); + await userEvent.click(within(dialog).getByRole('button', { name: 'Stop' })); + + // The confirmation closes on confirm, so the alert lands above the rows the + // user is left looking at. + expect( + await screen.findByTestId('task-history-action-error') + ).toHaveTextContent("You don't have permission to perform this action"); + }); + + it('renders a caller-owned stop failure passed as actionError', () => { + render( + + {}} + actionError={new Error('Task is no longer running')} + /> + + ); + + expect(screen.getByTestId('task-history-action-error')).toHaveTextContent( + 'Task is no longer running' + ); }); }); diff --git a/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.tsx b/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.tsx index 6ab3d351f1e..4a38fe1b492 100644 --- a/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.tsx +++ b/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.tsx @@ -31,6 +31,7 @@ import Tooltip from '@mui/material/Tooltip'; import Typography from '@mui/material/Typography'; import { MaterialReactTable, type MRT_ColumnDef } from 'material-react-table'; import { useAuth } from '@sep/api'; +import { ActionErrorAlert } from '../ActionErrorAlert'; import { isRunningStatus, useStopTaskHistory, @@ -164,6 +165,10 @@ interface ViewProps { isStopping: boolean; /** True when the row's stop action will resolve to a real handler (callback or internal mutation). */ canStop: (entry: TaskHistoryEntry) => boolean; + /** Failure of the last action fired from the table, or `null`/`undefined`. */ + actionError: unknown; + /** Dismiss handler for `actionError`, when the owner supports clearing it. */ + onDismissActionError?: () => void; } function TaskHistoryTableView({ @@ -177,6 +182,8 @@ function TaskHistoryTableView({ onConfirmStop, isStopping, canStop, + actionError, + onDismissActionError, }: ViewProps) { const { canMutate } = useAuth(); const [pendingStopEntry, setPendingStopEntry] = @@ -335,6 +342,14 @@ function TaskHistoryTableView({ return ( <> + {/* The stop dialog closes on confirm, so a refusal that arrives after it + is gone has to land here, above the rows the user is left looking at. */} + ); } @@ -488,6 +512,8 @@ function PresentationalTaskHistoryTable({ onDownloadFiles, onChainItemClick, hideTaskNameColumn, + actionError, + onDismissActionError, }: PresentationalProps) { const onConfirmStop = useCallback( (entry: TaskHistoryEntry) => onStopTask?.(entry), @@ -508,6 +534,8 @@ function PresentationalTaskHistoryTable({ onConfirmStop={onConfirmStop} isStopping={!!isStopping} canStop={canStop} + actionError={actionError} + onDismissActionError={onDismissActionError} /> ); } diff --git a/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.types.ts b/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.types.ts index 1c06f3ec935..5b27be3a965 100644 --- a/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.types.ts +++ b/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.types.ts @@ -58,4 +58,18 @@ export interface TaskHistoryTableProps { ) => void; /** Hide the Task Name column (useful when scoped to a single task). */ hideTaskNameColumn?: boolean; + /** + * Failure of an action fired from this table, rendered as an alert above the + * rows. The stop confirmation closes as soon as it is confirmed, so the + * request settles with the user looking at the table — this is where the + * refusal has to appear. + * + * Required of any caller that owns the stop mutation, in either mode: pass + * its `error` here or the failure is reported nowhere. Only the connected + * variant *without* an `onStopTask` reports for itself, from its internal + * mutation, and ignores this prop. + */ + actionError?: unknown; + /** Dismiss handler for {@link actionError} (e.g. the mutation's `reset`). */ + onDismissActionError?: () => void; } diff --git a/ui/packages/sep/framework/src/hooks/useTaskFileDownload.test.tsx b/ui/packages/sep/framework/src/hooks/useTaskFileDownload.test.tsx new file mode 100644 index 00000000000..749ce53c4ec --- /dev/null +++ b/ui/packages/sep/framework/src/hooks/useTaskFileDownload.test.tsx @@ -0,0 +1,94 @@ +/** + * Copyright (C) 2026 Percona LLC + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +import { renderHook, waitFor } from '@testing-library/react'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { ApiError, apiClient } from '@sep/api'; +import { QueryWrapper } from '../../tests/queryWrapper'; +import { useTaskFileDownload } from './useTaskFileDownload'; + +vi.mock('../utils/downloadBlob', () => ({ downloadBlob: vi.fn() })); + +describe('useTaskFileDownload', () => { + afterEach(() => vi.restoreAllMocks()); + + it("surfaces a refusal's own reason, not the bare status", async () => { + // The request asks for a blob, so the 403's JSON body arrives as one and the + // reason is unreadable until it is parsed. + vi.spyOn(apiClient, 'get').mockRejectedValue( + new ApiError({ + kind: 'http', + status: 403, + message: 'HTTP 403', + data: new Blob( + [ + JSON.stringify({ + detail: "You don't have permission to perform this action", + }), + ], + { type: 'application/json' } + ), + }) + ); + + const { result } = renderHook(() => useTaskFileDownload(), { + wrapper: QueryWrapper, + }); + result.current.mutate({ + taskHistoryId: 7, + path: 'out/log.txt', + isDir: false, + }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + expect(result.current.error?.message).toBe( + "You don't have permission to perform this action" + ); + }); + + it('downloads the file on success', async () => { + const { downloadBlob } = await import('../utils/downloadBlob'); + const blob = new Blob(['hello'], { type: 'text/plain' }); + vi.spyOn(apiClient, 'get').mockResolvedValue({ data: blob }); + + const { result } = renderHook(() => useTaskFileDownload(), { + wrapper: QueryWrapper, + }); + result.current.mutate({ + taskHistoryId: 7, + path: 'out/log.txt', + isDir: false, + }); + + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + expect(downloadBlob).toHaveBeenCalledWith(blob, 'log.txt'); + }); + + it('suggests a tarball name for a directory', async () => { + const { downloadBlob } = await import('../utils/downloadBlob'); + const blob = new Blob(['tar'], { type: 'application/gzip' }); + vi.spyOn(apiClient, 'get').mockResolvedValue({ data: blob }); + + const { result } = renderHook(() => useTaskFileDownload(), { + wrapper: QueryWrapper, + }); + result.current.mutate({ taskHistoryId: 7, path: 'out/logs', isDir: true }); + + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + expect(downloadBlob).toHaveBeenCalledWith(blob, 'logs.tar.gz'); + }); +}); diff --git a/ui/packages/sep/framework/src/hooks/useTaskFileDownload.ts b/ui/packages/sep/framework/src/hooks/useTaskFileDownload.ts index 0d70401cfd8..ea89a79e748 100644 --- a/ui/packages/sep/framework/src/hooks/useTaskFileDownload.ts +++ b/ui/packages/sep/framework/src/hooks/useTaskFileDownload.ts @@ -16,7 +16,7 @@ */ import { useMutation } from '@tanstack/react-query'; -import { apiClient, SEP_BASE_PATH } from '@sep/api'; +import { apiClient, normalizeBlobError, SEP_BASE_PATH } from '@sep/api'; import { downloadBlob } from '../utils/downloadBlob'; export interface TaskFileDownloadParams { @@ -29,14 +29,22 @@ export interface TaskFileDownloadParams { export function useTaskFileDownload() { return useMutation({ mutationFn: async ({ taskHistoryId, path, isDir }) => { - const { data } = await apiClient.get( - `/files/${taskHistoryId}/download`, - { - baseURL: SEP_BASE_PATH, - params: { path }, - responseType: 'blob', - } - ); + let data: Blob; + try { + ({ data } = await apiClient.get( + `/files/${taskHistoryId}/download`, + { + baseURL: SEP_BASE_PATH, + params: { path }, + responseType: 'blob', + } + )); + } catch (error) { + // The response type applies to the error body too, so a refusal's JSON + // reason arrives as a blob. Parse it, or the caller can only report the + // bare status code. + throw await normalizeBlobError(error); + } const name = path.split('/').filter(Boolean).pop() ?? path; const suggestedName = isDir ? `${name}.tar.gz` : name; downloadBlob(data, suggestedName); diff --git a/ui/packages/sep/framework/src/index.ts b/ui/packages/sep/framework/src/index.ts index d0e9a7cea21..605fc5ca605 100644 --- a/ui/packages/sep/framework/src/index.ts +++ b/ui/packages/sep/framework/src/index.ts @@ -51,6 +51,16 @@ export { } from './components/SchemaDrivenPlugin'; export { ReadOnlyNotice } from './components/ReadOnlyNotice'; export type { ReadOnlyNoticeProps } from './components/ReadOnlyNotice'; +export { + ActionErrorAlert, + useActionError, + actionErrorMessage, + DEFAULT_ACTION_ERROR_FALLBACK, +} from './components/ActionErrorAlert'; +export type { + ActionErrorAlertProps, + ActionErrorState, +} from './components/ActionErrorAlert'; export { ServiceSelector } from './components/ServiceSelector'; export type { ServiceSelectorProps } from './components/ServiceSelector'; export { SchemaSelector } from './components/SchemaSelector'; diff --git a/ui/packages/sep/framework/tests/mutationFailureReporting.guard.test.ts b/ui/packages/sep/framework/tests/mutationFailureReporting.guard.test.ts new file mode 100644 index 00000000000..2b1ee65b969 --- /dev/null +++ b/ui/packages/sep/framework/tests/mutationFailureReporting.guard.test.ts @@ -0,0 +1,165 @@ +/** + * Copyright (C) 2026 Percona LLC + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +import { readdirSync, readFileSync, statSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'vitest'; + +/** + * Every mutation in SEP can be refused — the API restricts state-changing + * routes to admins — so a call site that fires one and reports nothing leaves + * the user with silence instead of a reason. + * + * A file that calls `.mutate` / `.mutateAsync` passes by using the shared + * primitive (`ActionErrorAlert` / `useActionError` / `actionErrorMessage`) or + * the form path (`mapSubmitError`), or by being listed in + * {@link REPORTS_ITS_OWN_WAY} with the mechanism it uses instead. + * + * A new call site therefore has exactly two ways forward: adopt the primitive, + * or state in the allowlist how the failure reaches the user. Adding nothing + * fails this test. + * + * Scoped to `ui/packages` — the SEP-derived framework and plugin packages, + * which are compiled into hosts that need not mount a snackbar provider. PMM's + * own app code under `ui/apps` keeps its toast conventions and is not scanned. + * + * Granularity is the file, not the individual call: a file that already reports + * one action's failures passes even if a second mutation is added to it + * unwired. Tightening that would mean parsing each call's surroundings, which + * trades a mechanical check for a heuristic one; the omissions this guard + * exists to catch were whole files with no failure path at all. + */ +const PACKAGES_DIR = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + '..', + '..', + '..' +); + +const PRIMITIVE_MARKERS = [ + 'ActionErrorAlert', + 'useActionError', + 'actionErrorMessage', + 'mapSubmitError', + // The JSX prop a caller owning a stop mutation passes to TaskHistoryTable, so + // its failure renders above the rows the action was fired from. Matched with + // the `={` so a same-named local `actionError` state cannot satisfy the guard + // by accident. + 'actionError={', +]; + +/** + * Call sites that already rendered a failure from SEP's own component tree + * before the primitive existed, each with the mechanism it uses. They are + * correct as they stand; migrating them onto the primitive can happen + * opportunistically. Every entry must name where the message is rendered. + */ +const REPORTS_ITS_OWN_WAY: Record = { + 'sep/framework/src/components/ScheduledTasksPanel/ScheduledTasksPanel.tsx': + 'panel-level actionError / formError state rendered as an alert above the table', + 'sep/framework/src/components/TaskHistoryTable/TaskFilesDialog.tsx': + 'in-dialog alert reading the download mutation error', + 'plugins/atw/src/hooks.ts': + 'exposes a combined `error` from the lifecycle mutations, rendered by IncidentListPage', + 'plugins/atw/src/CollectPane.tsx': 'form-level submitError banner', + 'plugins/atw/src/SendDialog.tsx': + 'in-dialog alert reading the mutation error', + 'plugins/atw/src/IncidentListPage.tsx': + 'inline alert reading the incident-action error', +}; + +const MUTATION_CALL = /\.(mutate|mutateAsync)\s*\(/; + +function sourceFiles(dir: string, acc: string[] = []): string[] { + for (const entry of readdirSync(dir)) { + if (entry === 'node_modules' || entry === 'dist' || entry === '.turbo') { + continue; + } + const full = path.join(dir, entry); + if (statSync(full).isDirectory()) { + sourceFiles(full, acc); + continue; + } + if (!/\.tsx?$/.test(entry) || /\.(test|stories)\.tsx?$/.test(entry)) { + continue; + } + acc.push(full); + } + return acc; +} + +describe('mutation failure reporting', () => { + it('every mutation call site reports its failures in-tree', () => { + const unreported: string[] = []; + const files = sourceFiles(PACKAGES_DIR); + + // A mis-resolved root would make the scan pass by finding nothing at all. + expect(files.length).toBeGreaterThan(100); + expect( + files.some((file) => + file.endsWith(path.join('sep', 'framework', 'src', 'index.ts')) + ), + `Scan rooted at ${PACKAGES_DIR} did not reach the framework package.` + ).toBe(true); + + for (const file of files) { + const source = readFileSync(file, 'utf8'); + if (!MUTATION_CALL.test(source)) { + continue; + } + const relative = path + .relative(PACKAGES_DIR, file) + .split(path.sep) + .join('/'); + if (relative in REPORTS_ITS_OWN_WAY) { + continue; + } + if (PRIMITIVE_MARKERS.some((marker) => source.includes(marker))) { + continue; + } + unreported.push(relative); + } + + expect( + unreported, + [ + 'These files fire a mutation but render no failure from SEP’s own component tree.', + 'Either report the error with ActionErrorAlert / useActionError (or mapSubmitError in a', + 'form), or add the file to REPORTS_ITS_OWN_WAY naming where its message is rendered.', + 'A toast alone does not count: a host embedding these packages is not guaranteed to', + 'mount a snackbar provider.', + ].join(' ') + ).toEqual([]); + }); + + it('the allowlist has no stale entries', () => { + const stale = Object.keys(REPORTS_ITS_OWN_WAY).filter((relative) => { + const full = path.join(PACKAGES_DIR, relative); + try { + return !MUTATION_CALL.test(readFileSync(full, 'utf8')); + } catch { + return true; + } + }); + + expect( + stale, + 'Allowlisted files that no longer fire a mutation; drop them.' + ).toEqual([]); + }); +}); From 5aed1b2cce8d2cbd72f5218b552fb079114d04c5 Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Tue, 25 Aug 2026 09:01:51 -0300 Subject: [PATCH 20/23] PMM-15358 Open SEP routes to non-admin sessions The gating added in the previous commit was unreachable: SepPage held every SEP route to PMM admins, and NavigationProvider only offered the entries to them, so no session ever rendered a control-free view. That guard predated per-control gating. SEP's API admits any authenticated session to its reads and holds every unsafe method to administrators (DEFAULT_MINIMUM_ROLE is ADMIN), so a read-only view was always something the server was willing to serve. The route now carries no role restriction and the sidebar entries are offered to every signed-in user; what a session may do is decided per control by `canMutate`. The ServiceNow setup prompt stays administrator-only. SEP holds `GET /sep/admin/settings` to administrators including its reads, so for a non-admin the settings query is skipped rather than fired to be refused, and the app renders. The prompt would be a dead end for them in any case: its only call to action is a settings tab they cannot open. The non-admin branch sits ahead of the loading branch, so a disabled query cannot leave a spinner that never resolves. Grouping the SEP entries under a "Management" section is a follow-up; this keeps the administrator's ordering unchanged. Signed-off-by: Ignacio Durand --- .../navigation/navigation.provider.tsx | 15 +++-- .../contexts/navigation/navigation.utils.tsx | 3 +- .../servicenow/ServiceNowConnection.hooks.ts | 11 +++- ui/apps/pmm/src/sep/SepPage.test.tsx | 42 +++++++++++++ ui/apps/pmm/src/sep/SepPage.tsx | 63 ++++++++----------- .../pmm/src/sep/ServiceNowSetupGate.test.tsx | 43 +++++++++++-- ui/apps/pmm/src/sep/ServiceNowSetupGate.tsx | 19 +++++- 7 files changed, 145 insertions(+), 51 deletions(-) create mode 100644 ui/apps/pmm/src/sep/SepPage.test.tsx diff --git a/ui/apps/pmm/src/contexts/navigation/navigation.provider.tsx b/ui/apps/pmm/src/contexts/navigation/navigation.provider.tsx index cab88a643cc..241ef99eaff 100644 --- a/ui/apps/pmm/src/contexts/navigation/navigation.provider.tsx +++ b/ui/apps/pmm/src/contexts/navigation/navigation.provider.tsx @@ -91,15 +91,20 @@ export const NavigationProvider: FC = ({ children }) => { items.push(addAdvisors(advisors || [])); } - if (user.isPMMAdmin) { - items.push(NAV_DIVIDERS.inventory); + items.push(NAV_DIVIDERS.inventory); + if (user.isPMMAdmin) { items.push(NAV_INVENTORY); + } - // SEP apps mounted as native routes (migration). Shown once the session - // is established; role/flag gating comes with real auth (Option B). - items.push(...addSepApps()); + // SEP apps mounted as native routes. Offered to every signed-in user, not + // only admins: SEP's API serves its reads to any authenticated session + // and holds every unsafe method to administrators, so a non-admin gets a + // read-only view with no write control rendered (PMM-15358). Grouping + // these under a "Management" section is a follow-up. + items.push(...addSepApps()); + if (user.isPMMAdmin) { if (settings?.backupManagementEnabled) { items.push(NAV_BACKUPS); } diff --git a/ui/apps/pmm/src/contexts/navigation/navigation.utils.tsx b/ui/apps/pmm/src/contexts/navigation/navigation.utils.tsx index 2a13b0d1ae1..330dad306b4 100644 --- a/ui/apps/pmm/src/contexts/navigation/navigation.utils.tsx +++ b/ui/apps/pmm/src/contexts/navigation/navigation.utils.tsx @@ -312,7 +312,8 @@ export const addHomePage = (preferences?: UserPreferences): NavItem => { // SEP apps mounted as native PMM routes (migration). Metadata (icons/labels/routes) // is lifted from SEP's appNavConfig as data only — no SEP nav component is used. -// Role/flag gating arrives with real auth (Option B). +// Deliberately unconditional: reachability is not the gate, the per-control +// mutation capability is (PMM-15358, and NavigationProvider for placement). export const addSepApps = (): NavItem[] => [ { id: 'sep-atw', diff --git a/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.hooks.ts b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.hooks.ts index f9c4809115b..3d3b8ddfcad 100644 --- a/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.hooks.ts +++ b/ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.hooks.ts @@ -12,9 +12,16 @@ import { * Both the settings form and the Support diagnostics setup gate ask the same * question of the same LIST response, so the derivation lives here rather than * in either surface — TanStack Query dedupes the request itself. + * + * `enabled: false` skips the request and reports the same shape a failed read + * does. SEP holds `GET /sep/admin/settings` to administrators including its + * reads, so a caller that may be rendered for a non-admin passes `false` rather + * than firing a request that can only answer 403. */ -export const useServiceNowConnection = () => { - const { data: groups, isLoading, error } = useSettingsList(); +export const useServiceNowConnection = ({ + enabled = true, +}: { enabled?: boolean } = {}) => { + const { data: groups, isLoading, error } = useSettingsList({ enabled }); const declaredNames = useMemo(() => declaredSecretNames(groups), [groups]); const stored = useMemo(() => storedDeliveryInputs(groups), [groups]); diff --git a/ui/apps/pmm/src/sep/SepPage.test.tsx b/ui/apps/pmm/src/sep/SepPage.test.tsx new file mode 100644 index 00000000000..e7e18088463 --- /dev/null +++ b/ui/apps/pmm/src/sep/SepPage.test.tsx @@ -0,0 +1,42 @@ +import { render, screen } from '@testing-library/react'; +import { TestWrapper } from 'utils/testWrapper'; +import { TEST_USER_ADMIN, TEST_USER_VIEWER } from 'utils/testStubs'; +import { User } from 'types/user.types'; +import { SepPage } from './SepPage'; + +// The gate mints a SEP bearer on mount; this suite is about who reaches it, so +// hold it open and let the page render its children. +vi.mock('./SepAuthGate', () => ({ + SepAuthGate: ({ children }: { children: React.ReactNode }) => <>{children}, +})); + +const renderPage = (user: User) => + render( + +
+ , + { + wrapper: ({ children }) => ( + + {children} + + ), + } + ); + +describe('SepPage', () => { + it('renders the plugin for an administrator', () => { + renderPage(TEST_USER_ADMIN); + + expect(screen.getByTestId('sep-plugin')).toBeInTheDocument(); + }); + + it('renders the plugin for a viewer rather than an unauthorized card', () => { + // SEP serves its reads to any authenticated session and holds every unsafe + // method to administrators, so the route carries no role restriction and + // the write controls are withheld per control instead (PMM-15358). + renderPage(TEST_USER_VIEWER); + + expect(screen.getByTestId('sep-plugin')).toBeInTheDocument(); + }); +}); diff --git a/ui/apps/pmm/src/sep/SepPage.tsx b/ui/apps/pmm/src/sep/SepPage.tsx index f7d15e596d0..1915ee9ca30 100644 --- a/ui/apps/pmm/src/sep/SepPage.tsx +++ b/ui/apps/pmm/src/sep/SepPage.tsx @@ -2,8 +2,6 @@ import { FC, PropsWithChildren } from 'react'; import Box from '@mui/material/Box'; import Stack from '@mui/material/Stack'; import { Page } from 'components/page'; -import { useUser } from 'contexts/user'; -import { OrgRole } from 'types/user.types'; import { SepAuthGate } from './SepAuthGate'; import { SepAuthProvider } from './SepAuthProvider'; @@ -16,44 +14,35 @@ import { SepAuthProvider } from './SepAuthProvider'; * padding, width, auth gate, and footer. No `title` is passed: the SEP * plugins already render their own headings. * - * The PMM-admin restriction is enforced here rather than left to the sidebar: - * NavigationProvider only *hides* the SEP entries for non-admins, while the - * routes still match on direct navigation. It reuses the nav's own predicate: - * `isPMMAdmin` is `isGrafanaAdmin || orgRole === Admin`, and `roles` (org-role - * only) cannot express the Grafana-admin half on its own, so it gates the - * remaining case and Page renders its standard unauthorized card. - * - * `SepAuthGate` sits inside that check, so the SEP session exchange only runs - * for a user who is allowed on the page in the first place. + * No `roles` restriction: every signed-in PMM user may open a SEP page, and + * what they can do there is decided per control rather than per route. SEP's + * API admits any authenticated session to its reads and holds every unsafe + * method to administrators, so a non-admin gets the lists, details, logs and + * history with no write control offered (PMM-15358). The admin-only route + * guard this replaced predated that per-control gating and closed the + * read-only view the API was always willing to serve. * * `SepAuthProvider` wraps the whole subtree so framework and plugin components - * can read the session's mutation capability. It sits outside the gate: a + * can read the session's mutation capability. It sits outside `SepAuthGate`: a * component rendered while the exchange is still in flight must resolve the * same capability it will hold once the bearer lands, not the non-admin * fallback. */ -export const SepPage: FC = ({ children }) => { - const { user } = useUser(); - - return ( - - - - - {/* - A flex column that grows, not a plain block: it carries the height - handed down from Page so a plugin (or the ServiceNow setup prompt) - can centre itself in the page rather than in its own content box. - */} - - {children} - - - - - - ); -}; +export const SepPage: FC = ({ children }) => ( + + + + + {/* + A flex column that grows, not a plain block: it carries the height + handed down from Page so a plugin (or the ServiceNow setup prompt) + can centre itself in the page rather than in its own content box. + */} + + {children} + + + + + +); diff --git a/ui/apps/pmm/src/sep/ServiceNowSetupGate.test.tsx b/ui/apps/pmm/src/sep/ServiceNowSetupGate.test.tsx index 944698ddfd1..301bddedce6 100644 --- a/ui/apps/pmm/src/sep/ServiceNowSetupGate.test.tsx +++ b/ui/apps/pmm/src/sep/ServiceNowSetupGate.test.tsx @@ -1,6 +1,7 @@ import { render, screen } from '@testing-library/react'; import { ApiError, + AuthContext, REDACTED_SECRET, SettingClassGroup, useSettingsList, @@ -59,11 +60,17 @@ const mockList = ( } as ReturnType); }; -const renderGate = () => +/** + * Defaults to an administrator: the prompt only ever renders for one, so every + * case below except the read-only ones is an admin case. + */ +const renderGate = (isAdmin = true) => render( - -
- , + + +
+ + , { wrapper: TestWrapper } ); @@ -143,3 +150,31 @@ describe('ServiceNowSetupGate', () => { expect(screen.queryByTestId('atw-app')).not.toBeInTheDocument(); }); }); + +describe('ServiceNowSetupGate — read-only sessions', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('renders the app for a non-admin and reads no settings', () => { + // SEP holds GET /sep/admin/settings to administrators, reads included, so + // the request is skipped rather than fired to be refused. + mockList({ data: sepGroups(['sn_api_key']) }); + renderGate(false); + + expect(screen.getByTestId('atw-app')).toBeInTheDocument(); + expect(settingsList).toHaveBeenCalledWith({ enabled: false }); + }); + + it('shows a non-admin the app rather than a prompt they cannot act on', () => { + // Same unconfigured deployment that prompts an admin: the prompt's only + // call to action is a settings tab a non-admin cannot open. + mockList({ data: sepGroups(['sn_token'], { sn_api_key: 'secret' }) }); + renderGate(false); + + expect(screen.getByTestId('atw-app')).toBeInTheDocument(); + expect( + screen.queryByTestId('servicenow-setup-prompt') + ).not.toBeInTheDocument(); + }); +}); diff --git a/ui/apps/pmm/src/sep/ServiceNowSetupGate.tsx b/ui/apps/pmm/src/sep/ServiceNowSetupGate.tsx index 99cea37f3a2..85397aedad5 100644 --- a/ui/apps/pmm/src/sep/ServiceNowSetupGate.tsx +++ b/ui/apps/pmm/src/sep/ServiceNowSetupGate.tsx @@ -6,6 +6,7 @@ import Link from '@mui/material/Link'; import Stack from '@mui/material/Stack'; import Typography from '@mui/material/Typography'; import { Link as RouterLink } from 'react-router-dom'; +import { useAuth } from '@sep/api'; import { PERCONA_SUPPORT_URL, PMM_SERVICENOW_SETTINGS_PATH, @@ -89,12 +90,26 @@ const SetupPrompt: FC = () => ( * one. A SEP build whose settings carry no `DIAGNOSTICS_DELIVERY_INPUTS` key at * all is the same case for a different reason — the prompt would send the * operator to a settings tab that can only answer that it is unavailable, so - * the prompt is worse than useless there. `drifted` does gate — SEP holds + * the prompt is worse than useless there. + * + * A non-admin never reaches any of that. SEP holds `GET /sep/admin/settings` to + * administrators, reads included, so the read is skipped rather than fired to + * be refused, and the app renders. The prompt would be a dead end for them in + * any case: its only call to action is a settings tab they cannot open. `drifted` does gate — SEP holds * values the current delivery plan no longer accepts, so delivery is as broken * as if nothing were stored. */ export const ServiceNowSetupGate: FC = ({ children }) => { - const { status, stored, isLoading, error } = useServiceNowConnection(); + const { isAdmin } = useAuth(); + const { status, stored, isLoading, error } = useServiceNowConnection({ + enabled: isAdmin, + }); + + // Ahead of the loading branch: the read is disabled for a non-admin, so there + // is nothing to wait for and a spinner would never resolve. + if (!isAdmin) { + return <>{children}; + } if (isLoading) { return ( From 6241e4c126641973f22fb869b820bf82eb984e0d Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Tue, 25 Aug 2026 09:49:37 -0300 Subject: [PATCH 21/23] PMM-15358 Address PR review comments - Skip the merged execution-schema fetch in ATW's collect pane for a read-only session. The form it feeds is already withheld, so the request bought nothing; selecting snippets still works. - Drop "Create one to get started" from the incident empty state for a session that is offered no create control. Both reported by CodeRabbit on #5819. Signed-off-by: Ignacio Durand --- ui/packages/plugins/atw/src/CollectPane.tsx | 7 ++++- .../plugins/atw/src/IncidentListPage.tsx | 4 ++- .../atw/tests/CollectPane.search.test.tsx | 25 +++++++++++++++++ .../atw/tests/IncidentListPage.test.tsx | 27 +++++++++++++++++++ 4 files changed, 61 insertions(+), 2 deletions(-) diff --git a/ui/packages/plugins/atw/src/CollectPane.tsx b/ui/packages/plugins/atw/src/CollectPane.tsx index 9c29be2671a..b2e96539358 100644 --- a/ui/packages/plugins/atw/src/CollectPane.tsx +++ b/ui/packages/plugins/atw/src/CollectPane.tsx @@ -260,7 +260,12 @@ export function CollectPane({ () => selected.map((snippet) => snippet.name), [selected] ); - const schemaQuery = useAtwMergedSchema(isClosed ? [] : selectedNames); + // A read-only session never renders the execute form, so it never needs the + // merged schema either. Selecting snippets still works — only the fetch and + // the form are withheld. + const schemaQuery = useAtwMergedSchema( + isClosed || !canMutate ? [] : selectedNames + ); const batchMutation = useAtwBatchExecute(incidentId); const searchQuery = useAtwSnippetSearch(debouncedSearch); diff --git a/ui/packages/plugins/atw/src/IncidentListPage.tsx b/ui/packages/plugins/atw/src/IncidentListPage.tsx index 283fe8e52a1..ddc5d48d7de 100644 --- a/ui/packages/plugins/atw/src/IncidentListPage.tsx +++ b/ui/packages/plugins/atw/src/IncidentListPage.tsx @@ -172,7 +172,9 @@ export function IncidentListPage() { {!isLoading && !error && (!incidents || incidents.length === 0) && ( - No incidents yet. Create one to get started. + {canMutate + ? 'No incidents yet. Create one to get started.' + : 'No incidents yet.'} )} diff --git a/ui/packages/plugins/atw/tests/CollectPane.search.test.tsx b/ui/packages/plugins/atw/tests/CollectPane.search.test.tsx index b3b247c2864..aeadd5d8a52 100644 --- a/ui/packages/plugins/atw/tests/CollectPane.search.test.tsx +++ b/ui/packages/plugins/atw/tests/CollectPane.search.test.tsx @@ -403,4 +403,29 @@ describe('CollectPane — write access', () => { ).not.toBeInTheDocument(); }); }); + + it('fetches no execution schema for a non-admin', async () => { + // The form is the only consumer of the merged schema, so a session that + // cannot execute should not pay for the request. + mockCanMutate = false; + await selectOneSnippet(); + + const schemaCalls = () => + mockedApi.get.mock.calls.filter((call) => + String(call[0]).includes('/execution-schema/') + ); + await waitFor(() => expect(schemaCalls()).toHaveLength(0)); + }); + + it('fetches the execution schema for a session that may mutate', async () => { + await selectOneSnippet(); + + await waitFor(() => + expect( + mockedApi.get.mock.calls.filter((call) => + String(call[0]).includes('/execution-schema/') + ).length + ).toBeGreaterThan(0) + ); + }); }); diff --git a/ui/packages/plugins/atw/tests/IncidentListPage.test.tsx b/ui/packages/plugins/atw/tests/IncidentListPage.test.tsx index a46902b8359..45c0b7dab89 100644 --- a/ui/packages/plugins/atw/tests/IncidentListPage.test.tsx +++ b/ui/packages/plugins/atw/tests/IncidentListPage.test.tsx @@ -345,6 +345,33 @@ describe('IncidentListPage — write access', () => { ).toBeInTheDocument(); }); + it('drops the create instruction from the empty state for a non-admin', async () => { + mockCanMutate = false; + mockedApi.get.mockResolvedValue(paginated([])); + + renderPage(); + + await waitFor(() => + expect(screen.getByText('No incidents yet.')).toBeTruthy() + ); + // The instruction points at a control this session is not offered. + expect( + screen.queryByText(/Create one to get started/i) + ).not.toBeInTheDocument(); + }); + + it('keeps the create instruction in the empty state for a session that may mutate', async () => { + mockedApi.get.mockResolvedValue(paginated([])); + + renderPage(); + + await waitFor(() => + expect( + screen.getByText(/No incidents yet\. Create one to get started\./) + ).toBeTruthy() + ); + }); + it('renders no create, close, rename or delete for a non-admin', async () => { mockCanMutate = false; mockedApi.get.mockResolvedValue(paginated([incident])); From e9407d569f24ca1daf20f45b989207b91ce0e8bf Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Tue, 25 Aug 2026 09:54:06 -0300 Subject: [PATCH 22/23] PMM-15359 Address PR review comments - Pass the failure state to a custom create-form slot. The slot bypasses SchemaFormRenderer, and this ticket removed the error toast beside it, so a caller supplying `renderCreateForm` was left with no failure signal at all. The two edit pages already threaded it. Documented the slot's obligation to render it, and corrected the type's now-stale "error snackbar" wording. - Replace the guard's file-count sanity check with a sentinel from each scanned package. A count drifts with the repo and can be satisfied by the wrong tree. Reported by CodeRabbit and Copilot on #5820. Signed-off-by: Ignacio Durand --- .../PluginCreatePage.test.tsx | 39 +++++++++++++++++++ .../SchemaDrivenPlugin/PluginCreatePage.tsx | 2 + .../components/SchemaDrivenPlugin/types.ts | 18 ++++++--- .../mutationFailureReporting.guard.test.ts | 19 +++++---- 4 files changed, 65 insertions(+), 13 deletions(-) diff --git a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginCreatePage.test.tsx b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginCreatePage.test.tsx index fdb8181ac70..9972fe41988 100644 --- a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginCreatePage.test.tsx +++ b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginCreatePage.test.tsx @@ -264,6 +264,45 @@ describe('PluginCreatePage — failure reporting', () => { ); }); + it('hands the failure to a custom form slot, which is its only signal', async () => { + // The slot bypasses SchemaFormRenderer, and the framework raises no error + // toast alongside it, so dropping this state reports the failure nowhere. + const user = userEvent.setup(); + const slotProps: { submitError?: string | null }[] = []; + const capturingSlot: RenderFormSlot = ({ onSubmit, submitError }) => { + slotProps.push({ submitError }); + return ( +
+ + {submitError ?
{submitError}
: null} +
+ ); + }; + + renderPage({ renderCreateForm: capturingSlot }); + await user.click(screen.getByRole('button', { name: 'Submit slot' })); + await waitFor(() => expect(mockCreateTaskMutate).toHaveBeenCalledTimes(1)); + + act(() => + mockCreateTaskMutate.mock.calls[0][1].onError( + new ApiError({ + kind: 'http', + status: 403, + message: "You don't have permission to perform this action", + }) + ) + ); + + await waitFor(() => + expect(slotProps.at(-1)?.submitError).toBe( + "You don't have permission to perform this action" + ) + ); + expect(inTreeAlerts()).toHaveLength(1); + }); + it('shows no banner on a successful create', async () => { const user = userEvent.setup(); renderPage({ renderCreateForm: submitSlot }); diff --git a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginCreatePage.tsx b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginCreatePage.tsx index 21ca3c82ec5..1a80b1e2f2a 100644 --- a/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginCreatePage.tsx +++ b/ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginCreatePage.tsx @@ -163,6 +163,8 @@ export function PluginCreatePage({ loading: create.isPending, capabilities, renderField, + submitError, + fieldErrors, }) ?? ( ) => void; /** Whether the underlying mutation is in flight. */ loading: boolean; @@ -46,7 +48,11 @@ export interface PluginFormSlotProps { capabilities?: PluginCapabilities; /** Per-field override threaded through, so a composed renderer can honour it. */ renderField?: RenderFieldOverride; - /** Form-level submit error banner (populated on a 422); pass to the composed renderer. */ + /** + * Form-level submit error banner; pass to the composed renderer. Populated + * for every failed submit, not only a 422 — a slot that drops it reports + * nothing at all, since the framework raises no error toast alongside it. + */ submitError?: string | null; /** Per-field validation errors (from a 422); pass to the composed renderer for inline display. */ fieldErrors?: FieldValidationError[]; @@ -54,6 +60,6 @@ export interface PluginFormSlotProps { /** * Whole-form slot override for the create / edit pages. Return custom form UI; - * the framework keeps the surrounding chrome, mutation, and snackbars. + * the framework keeps the surrounding chrome, mutation, and success snackbar. */ export type RenderFormSlot = (props: PluginFormSlotProps) => ReactNode; diff --git a/ui/packages/sep/framework/tests/mutationFailureReporting.guard.test.ts b/ui/packages/sep/framework/tests/mutationFailureReporting.guard.test.ts index 2b1ee65b969..9bda5f261ea 100644 --- a/ui/packages/sep/framework/tests/mutationFailureReporting.guard.test.ts +++ b/ui/packages/sep/framework/tests/mutationFailureReporting.guard.test.ts @@ -109,13 +109,18 @@ describe('mutation failure reporting', () => { const files = sourceFiles(PACKAGES_DIR); // A mis-resolved root would make the scan pass by finding nothing at all. - expect(files.length).toBeGreaterThan(100); - expect( - files.some((file) => - file.endsWith(path.join('sep', 'framework', 'src', 'index.ts')) - ), - `Scan rooted at ${PACKAGES_DIR} did not reach the framework package.` - ).toBe(true); + // Assert a sentinel from each scanned package rather than a file count: the + // count drifts with the repo and can be satisfied by the wrong tree. + const sentinels = [ + path.join('sep', 'framework', 'src', 'index.ts'), + path.join('plugins', 'atw', 'src', 'AtwApp.tsx'), + ]; + for (const sentinel of sentinels) { + expect( + files.some((file) => file.endsWith(sentinel)), + `Scan rooted at ${PACKAGES_DIR} did not reach ${sentinel}.` + ).toBe(true); + } for (const file of files) { const source = readFileSync(file, 'utf8'); From 77eb65ab5144fe7b1d21fb02da37a44a0568eb27 Mon Sep 17 00:00:00 2001 From: Ignacio Durand Date: Tue, 25 Aug 2026 14:05:22 -0300 Subject: [PATCH 23/23] PMM-15359 Make the stop-failure contract a type error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My earlier reply claimed the mutation guard already enforced this. It does not: the guard is file-level, so a file that contains any accepted marker passes even if a `` inside it drops `actionError`. PluginDetailPage is exactly that shape — it holds three ActionErrorAlert usages, so deleting the LogsTab wiring would go unnoticed. CodeRabbit was right to push back. `TaskHistoryTableProps` now carries a discriminated stop contract: supplying `onStopTask` requires `actionError`, and omitting it forbids both, since the connected variant reports from its own mutation and would ignore them. The internal split omits from the base interface rather than the props union — `Omit` is not distributive and would have collapsed the two branches, which was the other half of my objection and is avoidable. No production call site changed: both already passed the error. Six test call sites now say `actionError={null}` explicitly, and a `@ts-expect-error` case pins the contract so it cannot silently relax. Signed-off-by: Ignacio Durand --- .../TaskHistoryTable.test.tsx | 42 +++++++++++++- .../TaskHistoryTable/TaskHistoryTable.tsx | 20 ++++--- .../TaskHistoryTable.types.ts | 55 +++++++++++++------ .../src/components/TaskHistoryTable/index.ts | 1 + ui/packages/sep/framework/src/index.ts | 1 + 5 files changed, 92 insertions(+), 27 deletions(-) diff --git a/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.test.tsx b/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.test.tsx index 077986002e9..95f33d98f5e 100644 --- a/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.test.tsx +++ b/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.test.tsx @@ -246,7 +246,12 @@ describe('TaskHistoryTable actions', () => { const data = [makeEntry(2, 'running')]; render( - + ); await userEvent.click(screen.getByRole('button', { name: 'Stop task' })); @@ -277,6 +282,7 @@ describe('TaskHistoryTable actions', () => { disablePolling onStopTask={onStopTask} isStopping + actionError={null} /> ); @@ -288,7 +294,12 @@ describe('TaskHistoryTable actions', () => { const data = [makeEntry(2, 'running')]; render( - + ); expect(screen.getByRole('button', { name: 'Stop task' })).toBeEnabled(); @@ -299,7 +310,12 @@ describe('TaskHistoryTable actions', () => { const data = [makeEntry(2, 'running')]; render( - + ); await userEvent.click(screen.getByRole('button', { name: 'Stop task' })); @@ -558,6 +574,24 @@ describe('TaskHistoryTable connected stop mutation', () => { ).toHaveTextContent("You don't have permission to perform this action"); }); + it('makes a caller-owned stop without failure reporting a type error', () => { + // The contract that keeps a caller from wiring the stop and silently + // dropping its refusal. Runtime is unaffected; the guarantee is the + // compiler rejecting the omission, which @ts-expect-error asserts. + expect(() => + render( + + {/* @ts-expect-error `onStopTask` requires `actionError`. */} + + + ) + ).not.toThrow(); + }); + it('renders a caller-owned stop failure passed as actionError', () => { render( @@ -660,6 +694,7 @@ describe('TaskHistoryTable — write access', () => { data={[makeEntry(2, 'running')]} disablePolling onStopTask={vi.fn()} + actionError={null} /> ); @@ -677,6 +712,7 @@ describe('TaskHistoryTable — write access', () => { data={[makeEntry(2, 'running')]} disablePolling onStopTask={vi.fn()} + actionError={null} /> ); diff --git a/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.tsx b/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.tsx index 4a38fe1b492..70149f0a612 100644 --- a/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.tsx +++ b/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.tsx @@ -45,6 +45,8 @@ import { StatusBadge } from './StatusBadge'; import { TaskFilesDialog } from './TaskFilesDialog'; import type { TaskHistoryEntry, + TaskHistoryStopContract, + TaskHistoryTableBaseProps, TaskHistoryTableProps, } from './TaskHistoryTable.types'; @@ -420,7 +422,10 @@ function TaskHistoryTableView({ ); } -type ConnectedProps = Omit; +// Omit from the base rather than the props union: `Omit` is not distributive, +// so applying it to the union would collapse the stop contract's two branches. +type ConnectedProps = Omit & + TaskHistoryStopContract; function ConnectedTaskHistoryTable({ taskName, @@ -495,12 +500,13 @@ function ConnectedTaskHistoryTable({ ); } -interface PresentationalProps extends Omit< - TaskHistoryTableProps, - 'taskName' | 'statusFilter' | 'pollingIntervalMs' | 'disablePolling' -> { - data: TaskHistoryEntry[]; -} +type PresentationalProps = Omit< + TaskHistoryTableBaseProps, + 'taskName' | 'statusFilter' | 'pollingIntervalMs' | 'disablePolling' | 'data' +> & + TaskHistoryStopContract & { + data: TaskHistoryEntry[]; + }; function PresentationalTaskHistoryTable({ data, diff --git a/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.types.ts b/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.types.ts index 5b27be3a965..f946d07b368 100644 --- a/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.types.ts +++ b/ui/packages/sep/framework/src/components/TaskHistoryTable/TaskHistoryTable.types.ts @@ -23,7 +23,7 @@ import type { export type { PaginatedTaskHistory, TaskHistoryEntry, TaskHistoryStatus }; -export interface TaskHistoryTableProps { +interface TaskHistoryTableBaseProps { /** Optional task name to scope the listing to a single task. */ taskName?: string; /** Optional explicit data — bypasses the internal React Query hook (used in stories/tests). */ @@ -40,8 +40,6 @@ export interface TaskHistoryTableProps { resolveUserName?: (userId: string | null | undefined) => string; /** Action callback: view logs for a row. */ onViewLogs?: (entry: TaskHistoryEntry) => void; - /** Action callback: stop a running task. */ - onStopTask?: (entry: TaskHistoryEntry) => void; /** * Whether a stop request is currently in flight (presentational mode only). * Drives the Stop button's spinner/disabled state; defaults to `false`. The @@ -58,18 +56,41 @@ export interface TaskHistoryTableProps { ) => void; /** Hide the Task Name column (useful when scoped to a single task). */ hideTaskNameColumn?: boolean; - /** - * Failure of an action fired from this table, rendered as an alert above the - * rows. The stop confirmation closes as soon as it is confirmed, so the - * request settles with the user looking at the table — this is where the - * refusal has to appear. - * - * Required of any caller that owns the stop mutation, in either mode: pass - * its `error` here or the failure is reported nowhere. Only the connected - * variant *without* an `onStopTask` reports for itself, from its internal - * mutation, and ignores this prop. - */ - actionError?: unknown; - /** Dismiss handler for {@link actionError} (e.g. the mutation's `reset`). */ - onDismissActionError?: () => void; } + +/** + * Who owns the stop mutation, and therefore who owns reporting its failure. + * + * A caller that supplies `onStopTask` owns the mutation, so it must also supply + * `actionError`: the stop confirmation closes as soon as it is confirmed, the + * request settles with the user looking at the table, and this is the only + * place the refusal can appear. Making it a union rather than two optional + * props means dropping the error is a type error rather than a silent failure. + * + * Omitting `onStopTask` selects the connected variant's internal mutation, + * which reports for itself and would ignore these props — so the union forbids + * them there rather than accepting a value that goes nowhere. + */ +export type TaskHistoryStopContract = + | { + onStopTask?: never; + actionError?: never; + onDismissActionError?: never; + } + | { + /** Action callback: stop a running task. */ + onStopTask: (entry: TaskHistoryEntry) => void; + /** + * Failure of an action fired from this table, rendered as an alert above + * the rows. Pass the owning mutation's `error`; `null` when there is + * none. + */ + actionError: unknown; + /** Dismiss handler for `actionError` (e.g. the mutation's `reset`). */ + onDismissActionError?: () => void; + }; + +export type TaskHistoryTableProps = TaskHistoryTableBaseProps & + TaskHistoryStopContract; + +export type { TaskHistoryTableBaseProps }; diff --git a/ui/packages/sep/framework/src/components/TaskHistoryTable/index.ts b/ui/packages/sep/framework/src/components/TaskHistoryTable/index.ts index 07e4759d1a4..56c5e463ea2 100644 --- a/ui/packages/sep/framework/src/components/TaskHistoryTable/index.ts +++ b/ui/packages/sep/framework/src/components/TaskHistoryTable/index.ts @@ -25,6 +25,7 @@ export { TaskFilesDialog } from './TaskFilesDialog'; export type { TaskFilesDialogProps } from './TaskFilesDialog'; export type { TaskHistoryTableProps, + TaskHistoryStopContract, TaskHistoryEntry, TaskHistoryStatus, PaginatedTaskHistory, diff --git a/ui/packages/sep/framework/src/index.ts b/ui/packages/sep/framework/src/index.ts index 605fc5ca605..72e487df614 100644 --- a/ui/packages/sep/framework/src/index.ts +++ b/ui/packages/sep/framework/src/index.ts @@ -97,6 +97,7 @@ export { } from './components/TaskHistoryTable'; export type { TaskHistoryTableProps, + TaskHistoryStopContract, TaskHistoryEntry, TaskHistoryStatus, PaginatedTaskHistory,