PMM-15397 Hide Collect pane from read-only sessions - #5843
Open
nachodd wants to merge 5 commits into
Open
Conversation
* PMM-15293 Add a token-minter seam to the SEP API client `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> * PMM-15293 Mint the SEP bearer from the PMM session 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> * 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 <nachodurand@gmail.com> * PMM-15293 Fail closed without discarding user work 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> * 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 <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * PMM-15294 Point ServiceNow form at the renamed peak-ui package Signed-off-by: Ignacio Durand <nachodurand@gmail.com> * PMM-15293 Drop the SSR-era HTML and 303 handling again 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> * PMM-15294 Extract Percona Support URL to a constant Signed-off-by: Ignacio Durand <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * PMM-15337 Guard the New incident button 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> * 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 <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * PMM-15358 Hide SEP write controls from non-admins Port SEP-1844 to PMM's embedded SEP packages. The SEP auth context moves into @sep/api, where the framework and the plugin packages can read it without depending on the host application, and exports `canMutate` — a semantic mutation capability derived from the session rather than the administrator flag read directly at each call site. A consumer rendered outside a provider resolves to a non-admin, non-mutating session, so a stray mount hides controls rather than throwing. PMM's session is the source: SepAuthProvider fills the context from `isPMMAdmin`, which is the same mapping SEP's Grafana auth provider applies to the exchanged bearer, and PMM has it loaded before a SEP route renders. Framework create, execute, stop, retry and delete controls are hidden rather than disabled, as are the equivalents in the ATW plugin. The `actions` list column is dropped when no delete handler is supplied so a read-only list has no dead column, and the snippet execution schema query is disabled for a session that cannot execute. Reads are untouched. This is a UI-only change and never a security boundary: SEP's API is unchanged and remains the only gate. PMM already restricts SEP routes to PMM admins in SepPage, so no PMM user reaches these surfaces read-only today; the gate keeps the shared packages in step with SEP and holds if that route restriction is ever relaxed. Signed-off-by: Ignacio Durand <nachodurand@gmail.com> * PMM-15359 Report failed SEP UI actions in-tree Port SEP-1845 to PMM's embedded SEP packages. A failure that is only enqueued as a toast is invisible wherever the host mounts no snackbar provider, so `@sep/framework` gains a shared failure-reporting primitive — `ActionErrorAlert`, `useActionError` and `actionErrorMessage` — that renders the server's own reason from the failing component's own tree. Schema-driven create and edit forms get their persistent banner back for every non-422 failure, carrying the server's reason instead of returning the empty state; the 422 per-field path is unchanged. Task execute, delete, entity delete and stop-task now report through the primitive rather than a toast, and each emits exactly one failure signal. The execute confirmation closes on confirm like the adjacent delete, since a dialog left open hides the message rendered behind it; reopening the same action keeps a composed chain so a refused execute can be retried. `normalizeBlobError` recovers the reason from a `responseType: 'blob'` request, whose 403 body arrives as a Blob rather than parsed JSON — `useTaskFileDownload` now reports the refusal instead of `HTTP 403`. A mechanical guard test scans `ui/packages` for `.mutate` / `.mutateAsync` call sites and fails on any file that renders no failure and is not allowlisted with the mechanism it uses instead. PMM's own app code under `ui/apps` keeps its toast conventions and is not scanned. Signed-off-by: Ignacio Durand <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * PMM-15359 Make the stop-failure contract a type error 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 `<TaskHistoryTable onStopTask=...>` 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 <nachodurand@gmail.com> --------- Signed-off-by: Ignacio Durand <nachodurand@gmail.com> Signed-off-by: yyyyyyy <contact@yyyyyyyan.tech> Co-authored-by: yyyyyyy <contact@yyyyyyyan.tech> Co-authored-by: Fábio Silva <ffjs1993@gmail.com>
* PMM-15293 Add a token-minter seam to the SEP API client `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> * PMM-15293 Mint the SEP bearer from the PMM session 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> * 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 <nachodurand@gmail.com> * PMM-15293 Fail closed without discarding user work 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> * 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 <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * PMM-15294 Point ServiceNow form at the renamed peak-ui package Signed-off-by: Ignacio Durand <nachodurand@gmail.com> * PMM-15293 Drop the SSR-era HTML and 303 handling again 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> * PMM-15294 Extract Percona Support URL to a constant Signed-off-by: Ignacio Durand <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * PMM-15337 Guard the New incident button 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> * 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 <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * PMM-15358 Hide SEP write controls from non-admins Port SEP-1844 to PMM's embedded SEP packages. The SEP auth context moves into @sep/api, where the framework and the plugin packages can read it without depending on the host application, and exports `canMutate` — a semantic mutation capability derived from the session rather than the administrator flag read directly at each call site. A consumer rendered outside a provider resolves to a non-admin, non-mutating session, so a stray mount hides controls rather than throwing. PMM's session is the source: SepAuthProvider fills the context from `isPMMAdmin`, which is the same mapping SEP's Grafana auth provider applies to the exchanged bearer, and PMM has it loaded before a SEP route renders. Framework create, execute, stop, retry and delete controls are hidden rather than disabled, as are the equivalents in the ATW plugin. The `actions` list column is dropped when no delete handler is supplied so a read-only list has no dead column, and the snippet execution schema query is disabled for a session that cannot execute. Reads are untouched. This is a UI-only change and never a security boundary: SEP's API is unchanged and remains the only gate. PMM already restricts SEP routes to PMM admins in SepPage, so no PMM user reaches these surfaces read-only today; the gate keeps the shared packages in step with SEP and holds if that route restriction is ever relaxed. Signed-off-by: Ignacio Durand <nachodurand@gmail.com> * PMM-15359 Report failed SEP UI actions in-tree Port SEP-1845 to PMM's embedded SEP packages. A failure that is only enqueued as a toast is invisible wherever the host mounts no snackbar provider, so `@sep/framework` gains a shared failure-reporting primitive — `ActionErrorAlert`, `useActionError` and `actionErrorMessage` — that renders the server's own reason from the failing component's own tree. Schema-driven create and edit forms get their persistent banner back for every non-422 failure, carrying the server's reason instead of returning the empty state; the 422 per-field path is unchanged. Task execute, delete, entity delete and stop-task now report through the primitive rather than a toast, and each emits exactly one failure signal. The execute confirmation closes on confirm like the adjacent delete, since a dialog left open hides the message rendered behind it; reopening the same action keeps a composed chain so a refused execute can be retried. `normalizeBlobError` recovers the reason from a `responseType: 'blob'` request, whose 403 body arrives as a Blob rather than parsed JSON — `useTaskFileDownload` now reports the refusal instead of `HTTP 403`. A mechanical guard test scans `ui/packages` for `.mutate` / `.mutateAsync` call sites and fails on any file that renders no failure and is not allowlisted with the mechanism it uses instead. PMM's own app code under `ui/apps` keeps its toast conventions and is not scanned. Signed-off-by: Ignacio Durand <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * 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 <nachodurand@gmail.com> * PMM-15359 Make the stop-failure contract a type error 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 `<TaskHistoryTable onStopTask=...>` 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 <nachodurand@gmail.com> * PMM-15384 Group the SEP apps under Management The two SEP apps rendered as loose top-level entries wedged between the inventory divider and the admin-only block. They now sit under a single collapsible "Management" section placed right below Inventory, so no pre-existing entry moves and the divider still opens with Inventory. The section has no page of its own: a collapsible takes its link from its first child. addSection() keeps a section from outliving its last child, since a childless collapsible renders as a shell that opens on nothing. Adds the NavigationProvider coverage the assembled tree never had, for admin, editor and viewer, including deep links into either SEP app. Signed-off-by: Ignacio Durand <nachodurand@gmail.com> * PMM-15384 Address PR review comments Carry the anonymous guard around addSepApps() on this branch too. PR #5819 adds it on the same line, and rewriting only the comment above a bare push would make the sync conflict on prose with the guard easy to drop while reconciling. With an identical `if` on both sides the conflict is comment-only. Covers the guard with a NavigationProvider test, so the Management section stays withheld from anonymous. Signed-off-by: Ignacio Durand <nachodurand@gmail.com> --------- Signed-off-by: Ignacio Durand <nachodurand@gmail.com> Signed-off-by: yyyyyyy <contact@yyyyyyyan.tech> Co-authored-by: yyyyyyy <contact@yyyyyyyan.tech> Co-authored-by: Fábio Silva <ffjs1993@gmail.com>
On the Support diagnostics incident page, a Viewer or Editor saw the whole Collect pane -- heading, category selects and snippet picker -- while the execute form underneath was already withheld, so the pane was an interactive dead end that also fired category and snippet fetches that could never lead to an execution. Gate the pane on canMutate at the page level, leaving CollectPane's own gates untouched so it stays safe if mounted elsewhere. Results takes the full width when the pane is withheld, with an inline ReadOnlyNotice in its place, and the Results empty state no longer points a read-only session at a pane that is not on the page. UI only: SEP already requires an administrator for every unsafe ATW route, so these sessions could never execute anything. Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
nachodd
requested review from
fabio-silva and
matejkubinec
and
a lite review from Copilot
and removed request for
a team
August 27, 2026 20:21
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Support diagnostics incident workspace UI (ui/packages/plugins/atw) so read-only sessions (Viewer/Editor; canMutate === false) no longer mount or display the Collect pane, avoiding dead-end interactions and preventing category/snippet fetches that can’t lead to execution.
Changes:
- Gate the Collect pane (and its layout column) behind
canMutate, letting Results span full width for read-only sessions. - Add an inline
ReadOnlyNoticeexplaining the restriction when Collect is withheld. - Make
ResultsPane’s empty-state copy role-aware and extend tests to cover both admin and read-only behavior (including “no category GETs”).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ui/packages/plugins/atw/src/IncidentWorkspacePage.tsx | Gate Collect pane on canMutate, collapse grid to one column for read-only, and render inline ReadOnlyNotice. |
| ui/packages/plugins/atw/src/ResultsPane.tsx | Adjust empty-state message to avoid referencing Collect when canMutate is false. |
| ui/packages/plugins/atw/tests/IncidentWorkspacePage.test.tsx | Add coverage for Collect presence/absence, notice rendering, and no category fetches for read-only sessions. |
| ui/packages/plugins/atw/tests/ResultsPane.test.tsx | Add coverage ensuring read-only empty state does not mention the Collect pane. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
fabio-silva
approved these changes
Aug 31, 2026
fabio-silva
requested review from
4nte and
JiriCtvrtka
and removed request for
a team
August 31, 2026 14:02
Collaborator
Author
|
@copilot resolve the merge conflicts in this pull request |
Signed-off-by: GitHub <noreply@github.com> Co-authored-by: nachodd <4997040+nachodd@users.noreply.github.com>
Contributor
|
|
Contributor
Merged |
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.
What
On the Support diagnostics incident details page (
/pmm-ui/sep/atw/:incidentId), a user whose only role is Viewer or Editor saw the entire Collect pane — heading,Category/Subcategoryselects, and theSnippetsautocomplete — while the execute form underneath was already withheld (CollectPane.tsxgates it oncanMutate). The pane was an interactive dead end, andCategoryBrowserfired category/snippetGETs that could never lead to an execution.Now:
PaperandCollectPaneare gated oncanMutateat the page level.CollectPane's owncanMutategates are untouched, so the exported component stays safe if it is ever mounted elsewhere.mdand above when Collect is withheld, so Results spans the full width instead of leaving a dead column.ReadOnlyNoticefrom@sep/frameworksits where the pane was, so the page reads as restricted rather than broken. Same shape as the precedent inSnippetExecutionAccordion.ResultsPane's empty state no longer says "Run snippets from the Collect pane…" to a session that has no Collect pane, following the role-aware copy pattern already inIncidentListPage.Why
Reachability is not the authorization boundary here (PMM-15358): any signed-in user may open Support diagnostics, and write controls are gated one by one on
canMutate. The Collect pane was only partially gated.UI-only change. SEP resolves a minimum role of
ADMINfor every unsafe ATW route, so these sessions already got a 403 from the execution endpoints. No backend, permission, orderiveCanMutatechange.Acceptance criteria covered
mdand above.cd ui && make lint && make testpasses.Test plan
Automated —
ui/packages/plugins/atw/tests/:IncidentWorkspacePage.test.tsx— Collect present and no notice for a session that may mutate; Collect heading andSnippetscombobox absent, notice and its sentence present, Results still rendered for a non-admin; noGET /apps/atw/for a non-admin.ResultsPane.test.tsx— the empty state keeps the "Collect pane" wording for an admin and drops it for a read-only session.Each new negative assertion was confirmed to fail with the source change reverted, so they are real guards rather than vacuous ones.
Ran:
cd ui && make lint(0 errors, 0 warnings in@sep/plugins-atw),make format-check,pnpm check-types,make test(atw 101 passed; full UI suite 484 passed / 13 skipped).Manual: