PMM-15337 Gate diagnostics on ServiceNow setup - #5770
Merged
fabio-silva merged 29 commits intoAug 24, 2026
Conversation
`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 <nachodurand@gmail.com>
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 <nachodurand@gmail.com>
`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 <nachodurand@gmail.com>
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 <nachodurand@gmail.com>
The base moved the SEP side-car behind a single `/sep` mount point (SEP_BASE_PATH, PMM-15279), so the session exchange and everything the 401 retry matches on had to move with it. Conflicts were textual, not behavioural — `client.ts` and `index.ts` auto-merged with both the minter seam and SEP_BASE_PATH intact: - `typed-client.ts` / both test files: import-block collisions, kept both sides. - `bootstrap.ts`: the base edited the old doc comment that described the token exchange as future work. This branch is that work, so its text wins, with the base's `/sep/api/...` path correction applied. - `vite.config.ts`: the base collapsed five proxied prefixes into `/sep`, forwarded unstripped. Both comment blocks kept, and `isSepAuthPath` now matches `/sep/api/oauth/` — the prefix is still on the URL when the proxy sees it, so the old `/api/oauth/` test silently stopped matching and would have let the internal token cover the exchange again. - Tests: reworked my handlers onto the base's new `API` constant, which now carries the prefix. Neither transport's mint guard needed changing: axios matches on a URL relative to `baseURL`, and the typed client's `includes('/oauth/session')` is unaffected by a prefix. Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
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 <nachodurand@gmail.com>
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 <nachodurand@gmail.com>
`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 <nachodurand@gmail.com>
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 <nachodurand@gmail.com>
…ssion-exchange # Conflicts: # ui/packages/sep/api/src/typed-client.ts
Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
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 <contact@yyyyyyyan.tech>
…into PMM-15294-sep-diagnostics-settings Signed-off-by: yyyyyyy <contact@yyyyyyyan.tech>
Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
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 <nachodurand@gmail.com>
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 <nachodurand@gmail.com>
"Collect Diagnostic Data" described the mechanism; "Support diagnostics" describes what it is for. The icon follows. Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
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 <nachodurand@gmail.com>
nachodd
requested review from
fabio-silva and
mattiasimonato
and
a lite review from Copilot
and removed request for
a team
August 13, 2026 17:01
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the SEP “ATW” diagnostics experience in the PMM UI by renaming it to Support diagnostics, improving the incident list’s error/loading behavior, and gating access to the app behind a ServiceNow connection setup prompt when delivery isn’t configured.
Changes:
- Rename “Collect Diagnostic Data” to Support diagnostics (navigation + page heading) and update the sidebar icon.
- Add a ServiceNow setup gate (with loading + fail-open semantics) in front of the Support diagnostics app, backed by a shared
useServiceNowConnectionhook. - Improve incident list UX by hiding New incident after a list load failure and disabling it while loading; fix
SepPagelayout so gated content can vertically center.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/packages/plugins/atw/tests/IncidentListPage.test.tsx | Adds plugin tests asserting create button is disabled while loading and withheld on list failure. |
| ui/packages/plugins/atw/src/IncidentListPage.tsx | Renames heading and adjusts “New incident” button rendering/disabled logic based on list loading/error state. |
| ui/apps/pmm/src/sep/ServiceNowSetupGate.tsx | Introduces the setup prompt + gate logic for ServiceNow connection state. |
| ui/apps/pmm/src/sep/ServiceNowSetupGate.test.tsx | Adds unit tests for gate behavior across configured / not-configured / drifted / loading / error scenarios. |
| ui/apps/pmm/src/sep/ServiceNowSetupGate.messages.ts | Adds user-facing strings for the setup prompt and loading label. |
| ui/apps/pmm/src/sep/SepPage.tsx | Fixes flex layout so children can occupy full page height and center vertically. |
| ui/apps/pmm/src/router.tsx | Wraps the ATW app route with ServiceNowSetupGate inside SepPage. |
| ui/apps/pmm/src/pages/settings/Settings.messages.ts | Switches support URL import to lib/constants. |
| ui/apps/pmm/src/pages/settings/Settings.constants.ts | Removes duplicated PERCONA_SUPPORT_URL, leaving only MAX_LABEL_WIDTH. |
| ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.tsx | Switches to shared useServiceNowConnection hook instead of duplicating derivation logic. |
| ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.hooks.ts | Adds shared hook that derives declared secrets, stored inputs, and connection status from one settings LIST call. |
| ui/apps/pmm/src/lib/constants.ts | Adds ServiceNow settings route constant and docs placeholder URL for Support diagnostics. |
| ui/apps/pmm/src/contexts/navigation/navigation.utils.tsx | Renames nav entry to “Support diagnostics” and swaps the icon. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+22
to
+28
| return { | ||
| declaredNames, | ||
| stored, | ||
| status: connectionStatus(declaredNames, stored), | ||
| isLoading, | ||
| error, | ||
| }; |
Collaborator
Author
There was a problem hiding this comment.
Verified and fixed in 52b4f1d. Confirmed the dead end: ServiceNowConnectionForm returns the servicenow-unavailable alert when !stored.isPresent, so the prompt was sending the operator to a tab that can only say it is unavailable. The gate now fails open on a missing key, same as it does on a failed read — both are "cannot tell, and cannot act", and the app reports its own failures.
Comment on lines
+75
to
+92
| 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(); | ||
| }); |
Collaborator
Author
There was a problem hiding this comment.
Added in 52b4f1d — "renders the app when SEP does not carry the delivery inputs key", covering a settings group that ships DIAGNOSTICS_DELIVERY without DIAGNOSTICS_DELIVERY_INPUTS. Gate suite is 7/7, app suite 445 passed.
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 <nachodurand@gmail.com>
nachodd
added a commit
to percona/SEP
that referenced
this pull request
Aug 13, 2026
A build whose settings carry no DIAGNOSTICS_DELIVERY_INPUTS key read as "not configured", so the prompt told an admin to fill in a key that is not there. Treat a missing key like a failed read and let the app render. Ported from the PMM-embedded gate (percona/pmm#5770). Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
7 tasks
fabio-silva
approved these changes
Aug 17, 2026
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 <nachodurand@gmail.com>
mattiasimonato
approved these changes
Aug 20, 2026
This was referenced Aug 22, 2026
Base automatically changed from
PMM-15294-sep-diagnostics-settings
to
PMM-15216
August 24, 2026 12:42
Picks up the base branch's merge of main (pnpm toolchain, the @vitejs/plugin-react alignment and the @percona/peak-ui rename) plus PMM-15294's ServiceNow submit path. Every conflict was this branch's refactor meeting the base branch's pre-refactor version, so all three resolve in favour of HEAD: - ServiceNowConnectionForm.tsx: this branch extracted the settings read into useServiceNowConnection() so sep/ServiceNowSetupGate.tsx can ask the same question of the same LIST response. Kept the hook and dropped the inline useSettingsList + declaredSecretNames/storedDeliveryInputs/ connectionStatus derivation, which the hook now owns. Diffing the result against PMM-15216 shows the extraction as the only delta, so none of PMM-15294's work is lost. - SepPage.tsx: kept the growing flex Box over the plain <div>; it passes Page's height down so the setup prompt can centre in the page. - Settings.messages.ts: both branches introduced PERCONA_SUPPORT_URL independently -- this one in lib/constants.ts (next to PMM_SERVICENOW_SETTINGS_PATH and SUPPORT_DIAGNOSTICS_DOCS_URL, since the gate under sep/ also needs it), the base in Settings.constants.ts. Neither had it at the merge base, so the union merge left two definitions of the same URL. Kept the lib/constants.ts one and removed the duplicate, leaving Settings.constants.ts with MAX_LABEL_WIDTH alone. Verified from ui/: pnpm install, make lint, make build, make test and make format-check all pass; tsc --noEmit clean for apps/pmm, whose suite is 461 passed / 13 skipped. Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket number: PMM-15337
Feature build: SUBMODULES-0
What
Renames the Collect Diagnostic Data app to Support diagnostics, gives it an icon that matches what it does, and puts a setup screen in front of it unless the SEP ServiceNow delivery connection is configured.
MedicalServicesOutlined. No occurrence of "Collect Diagnostic Data" remains underui/apps/pmm/srcorui/packages/plugins/atw.declaredSecretNames/storedDeliveryInputs/connectionStatusderivation moves out ofServiceNowConnectionForminto a newuseServiceNowConnectionhook. OneuseSettingsListcall backs both the settings form and the gate — TanStack Query dedupes the request. No API surface is added.ServiceNowSetupGatewraps<AtwApp/>insideSepPage→SepAuthGate(reading SEP settings needs the exchanged SEP bearer from PMM-15293, so gating outside the auth gate would fire an unauthenticated request). It shows a spinner while settings load, the app when the connection isconfigured, and a centred setup screen fornot-configured/drifted.SepPagewrapped its children in a plain<div>, breaking the flex chain fromPageso nothing below it could centre vertically. It is now a growing flex column.Why
Everything the diagnostics app can do ends in an upload to a ServiceNow case. Today it renders its incident list to every admin regardless of whether this PMM instance has a connection configured, so on an unconfigured instance a user can browse, create an incident, run a script — and only discover at the very last step that nothing can be delivered. The setup screen replaces that dead end with an explanation, a promise that nothing is collected without an explicit confirmation, and a button to the settings tab that fixes it.
Likewise, offering New incident after the list request has failed only produces a second error on top of one the user cannot act on.
Deliberate trade-off: the gate fails open
configurednot-configured(no override, or a declared secret stored empty)drifted(stored values no longer satisfy the delivery plan)A failed settings read says nothing about the connection. Showing the setup screen there would tell an operator with a perfectly good connection to go configure one, so the gate fails open and lets the app report its own errors.
Open items for review
SUPPORT_DIAGNOSTICS_DOCS_URL(ui/apps/pmm/src/lib/constants.ts). The final URL is still to be defined and must be supplied before release — it is a one-line change to that constant.MedicalServicesOutlinedfrom the MUI Material set (no custom SVG). Swapping it is a one-line import change innavigation.utils.tsx.How to Test
Setup. A PMM Server with a reachable SEP backend, signed in as a PMM admin. The SEP session exchange (PMM-15293) must be working — without it the SEP settings call is unauthenticated and the gate fails open, masking every case below.
A — Unconfigured instance shows the setup screen
SEPSettings→DIAGNOSTICS_DELIVERY_INPUTShas no override).B — Layout
max-width: 480px,padding: 16px, and 32px of separation between the button and the text above and below it.C — The button reaches the settings tab
D — Configuring flips the gate
E — Drift re-gates the app
DIAGNOSTICS_DELIVERY_INPUTSoverride) and reload.F — Fail open on a settings error
/v1/sep/**settings LIST in dev tools) and reload the page.G — Loading state
H — Incident list error state
I — Rename and icon
J — Regression
SepPage, which this PR changes) — no layout or width regression.Automated
All green. Gate unit tests cover configured / not-configured / drifted / load-error / loading and the CTA target; plugin tests cover the button's error and loading behaviour.