PMM-15293 Mint the SEP bearer from the PMM session - #5739
Merged
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>
nachodd
requested review from
Copilot,
fabio-silva and
mattiasimonato
and removed request for
a team
August 6, 2026 12:12
nachodd
marked this pull request as draft
August 6, 2026 12:19
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the embedded SEP integration in PMM UI to authenticate SEP requests as the active PMM user by exchanging the PMM session cookie for a short-lived SEP bearer token, replacing the interim dev-proxy server-side token injection approach. It does this by introducing a pluggable “token minter” seam in @sep/api, adding 401 replay recovery to the typed openapi-fetch client, and wiring PMM’s SEP routes through an in-memory token store + auth gate.
Changes:
- Add a pluggable token-minter API (
setTokenMinter) and broaden “mint endpoint” detection to avoid retry self-deadlocks. - Add one-shot 401 recovery + request replay (including body replay) for the typed
openapi-fetchclient. - Implement PMM’s in-memory SEP bearer store + route gate, and adjust the dev proxy to avoid overriding OAuth/session exchange requests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ui/packages/sep/api/tests/typed-client.test.ts | Adds typed-client 401 recovery/replay tests (including request body replay and mint-endpoint guard). |
| ui/packages/sep/api/tests/client.test.ts | Adds axios client tests for the token-minter seam and self-await regression guard. |
| ui/packages/sep/api/src/typed-client.ts | Implements typed-client 401 recovery via token mint + raw fetch replay using a pre-dispatch Request clone. |
| ui/packages/sep/api/src/index.ts | Exports setTokenMinter and MintedToken from the package surface. |
| ui/packages/sep/api/src/client.ts | Introduces MintedToken, setTokenMinter, broadens mint-endpoint detection, and routes refresh through the minter. |
| ui/packages/sep/api/README.md | Documents the token-minter seam and PMM embedded-session wiring. |
| ui/apps/pmm/vite.config.ts | Prevents dev-proxy internal-token injection from overriding /api/oauth/* routes (needed for session exchange). |
| ui/apps/pmm/src/sep/sepTokenStore.ts | New in-memory SEP bearer store with renewal scheduling and sticky signed-out behavior on rejected session. |
| ui/apps/pmm/src/sep/sepTokenStore.test.ts | Unit tests for token acquisition, renewal, storage guarantees, concurrency coalescing, and sticky signed-out handling. |
| ui/apps/pmm/src/sep/SepPage.tsx | Wraps SEP routes in SepAuthGate so the initial exchange happens on SEP route mount. |
| ui/apps/pmm/src/sep/SepAuthGate.tsx | New gate that blocks SEP content until the bearer is minted; provides retry UI for failures. |
| ui/apps/pmm/src/sep/SepAuthGate.test.tsx | Tests gating behavior (withhold children, sticky signed-out, retry, transient error messaging). |
| ui/apps/pmm/src/sep/SepAuthGate.messages.ts | New copy for the SEP auth gate’s loading/error/signed-out states. |
| ui/apps/pmm/src/sep/bootstrap.ts | Wires @sep/api to the PMM token store (provider + minter + refreshed + unauthorized callbacks). |
`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>
nachodd
added a commit
to percona/SEP
that referenced
this pull request
Aug 6, 2026
Upstreams the change PMM needed for the embedded UI (percona/pmm#5739), so the next sync of this tree into PMM does not clobber it. `refreshAccessToken()` hardcoded `POST /oauth/refresh` as the only way to obtain a token. That is right for this SPA, but PMM embeds SEP with no refresh cookie at all — it trades its own session cookie for a bearer via `POST /oauth/session/exchange` (SEP-1692) — so there the default 401s on every recovery attempt, and the whole retry machinery is dead weight. `setTokenMinter()` replaces just that call and defaults to the existing one, so nothing changes here. Everything downstream was already minter-agnostic: the single-flight coalescer, the axios 401 retry, and the `setOnRefreshed` notification all work the same whichever endpoint produced the token. `isTokenMintRequest` composes the existing `isRefreshRequest` and `isSessionRequest` guards, which the axios retry condition already listed separately, and is exported so the typed client shares one definition. Keeping them together matters: minting is single-flighted, so letting a mint's own 401 into the retry path would hand the interceptor the very promise it is running inside. The `openapi-fetch` transport gained the 401 retry the axios one already had. It previously only reported unauthorized, so every typed hook — `useCurrentUser` and all the generated-path ones — surfaced an expired token as a failure instead of recovering from it. This is a fix for both deployments, not just the embedded one. `fetch` consumes a Request's body, so the middleware stashes a clone taken before dispatch and replays that; only replay-eligible requests are cloned, and the replay goes through raw `fetch` so it cannot re-enter the middleware and loop.
8 tasks
yyyyyyyan
pushed a commit
to percona/SEP
that referenced
this pull request
Aug 7, 2026
) The CodeRabbit review of the SEP-UI-into-PMM migration PR ([percona/pmm#5653](percona/pmm#5653), PMM-15216) raised these findings against the ported copy of `@sep/api` / `@sep/framework`. Every one of them is present in the original, so they are fixed here too — otherwise the next sync in either direction re-introduces them. PMM-only wiring (route constants, the route-level admin gate, the dev-proxy env variables) is out of scope. ## Token-minter seam (upstreamed, not a review fix) The last commit is a different kind of change from the rest of this PR, so it is called out separately: it upstreams a capability PMM needed ([percona/pmm#5739](percona/pmm#5739), PMM-15293) rather than fixing a review finding. It lands here for the same reason as everything else — otherwise the next sync clobbers it. `refreshAccessToken()` hardcoded `POST /oauth/refresh` as the only way to obtain a token. That is correct for this SPA. It is not workable for PMM, which embeds SEP with no refresh cookie at all: it trades its own session cookie for a bearer through `POST /oauth/session/exchange` (SEP-1692), so the default 401s on every recovery attempt and the entire retry path is dead weight there. - **`setTokenMinter()`** replaces just that one call, defaulting to the existing behaviour — **nothing changes for a standalone SEP deployment**. Everything downstream was already minter-agnostic: the single-flight coalescer, the axios 401 retry, and the `setOnRefreshed` notification do not care which endpoint produced the token. - **`isTokenMintRequest`** composes the existing `isRefreshRequest` and `isSessionRequest` guards — which the axios retry condition already listed separately — and is exported so the typed client shares one definition rather than growing a second copy. Keeping them together is load-bearing: minting is single-flighted, so letting a mint's own 401 into the retry path hands the interceptor the very promise it is running inside, an await on itself that never settles. There is a regression test that times out rather than fails if that guard is lost. - **The `openapi-fetch` transport gained the 401 retry the axios one already had.** This one is a fix for *both* deployments: it previously only reported unauthorized, so every typed hook — `useCurrentUser` and all the generated-path ones — surfaced an expired token as a failure instead of recovering. `fetch` consumes a Request's body, so the middleware stashes a clone taken before dispatch and replays that; only replay-eligible requests are cloned, and the replay goes through raw `fetch` so it cannot re-enter the middleware and loop. `@sep/api` grows 10 tests for this: minting through a registered minter, coalescing a burst of 401s into one exchange, the self-await regression guard, a null-resolving minter, restoring the default, and on the typed side mint-and-replay, replaying a request body, replaying at most once, one mint across concurrent 401s, and no recovery attempt on a minting endpoint's own 401. **Correctness** - **`refreshAccessToken`** called the injected `_onRefreshed` handler inside the async executor, so a synchronous throw from it rejected the shared `refreshInFlight` promise — every awaiting caller saw a failed refresh, and a force-logout, for a cookie rotation that had already succeeded on the backend. The function's own comment says this must not happen. - **`SchemaFormRenderer`** returned from `handleFormSubmit` on a section violation, which react-hook-form reads as a *successful* submit (`isSubmitSuccessful = true`). `useUnsavedChangesGuard` is `isDirty && !isSubmitSuccessful` and only re-arms when `submitError` is truthy — never on this path — so the guard stayed disarmed: no `beforeunload` prompt, no `UnsavedChangesBlocker`, and the user could navigate away from a dirty form and lose it. The gate now runs in the submit event handler, ahead of `handleSubmit`. - **`normalizeChoiceDefaults`** read and wrote flat keys, but `flattenSectionFields` also returns `one_of` branch fields, whose names are dotted paths stored nested. A case-mismatched nested choice value was never canonicalised and rendered as an empty selection — the exact failure that function exists to prevent. - **`AppListPage` / `AppDetailPage`** dereferenced the optional `list_view` (`schema.list_view!.columns`), reachable through an unresolved entity route. The list page now renders `Not found`; the Overview tab falls back to an empty column set and still lists the task's own fields. - **`HostSelector`** looked up `errors[name]`, which never resolves for a dotted branch-field name, so an affected field showed no validation error. - **`extractId`** used `Number`, which turns a whitespace-only string into `0` and accepts `'1.5'` / `'0x10'`. Each result reads as a resolvable inventory id downstream: `useResolvedServiceField` enables a lookup for service `0`, and `SchemaSelector` fires `useSchemas({ serviceId: 0 })` for a service that cannot exist. - **`validationMapper`** submitted `parseInt('2.5', 10)` as `2` and `parseFloat('3.14invalid')` as `3.14`. Numeric fields now validate with `Number.isFinite` (plus `Number.isInteger` for `integer`) and coerce with `Number`. - **`useTaskLogs`** guarded on `!step`, dropping a log line with `step: ''`. `useExecutionEvents` treats `''` as the stepless bucket and the viewer labels it "General", so the two streams disagreed and stepless output was silently lost. **Stability** - **`useExecutionEvents`** returned `undefined` from `onerror` for every non-sentinel error, so `fetchEventSource` retried forever while `sseError` stayed unset and `sseLoading` stayed true — a persistently failing endpoint (500, DNS failure) left the panel spinning and reconnecting indefinitely. Consecutive failures are now counted, reset on a successful open, and the loop stops with the error surfaced. - **`StandaloneHostSelector`** disabled its Autocomplete when the hosts query failed, but `onOpen` holds the only `refetch()` trigger and a disabled Autocomplete never opens — one failure wedged the control until the page remounted. - **A scheduled-task enable/disable toggle** sent `kwargs: '{}'` in a full PUT, wiping the arguments of any task created with non-default kwargs. `kwargs` is preserved when the response carries it; `'{}'` stays the fallback until `PeriodicTaskResponse` declares the field. **Contract and consistency** - **`useCascadingField`** cleared with `undefined` while `buildFormDefaults` seeds these selector types to `''`, and counted `''` as ready — a downstream selector fetched options for an empty parent, and a bound MUI input flipped from controlled to uncontrolled. - **`SchemaDrivenApp`**'s `renderEditForm` invocation omitted `capabilities`, `submitError` and `fieldErrors`, so a consumer supplying the slot could render neither the 422 banner nor the inline field errors. `AppTaskEditPage` already passes all three to the same slot type. - **`SchemaListView`** pinned `bgcolor: 'common.white'`, which renders a white table in dark mode; it now reads the mode's own opaque surface. - **`FileField`**'s file-picker `IconButton` had no accessible name. - **`useResolvedServiceField`** discarded `useServices`' error, so callers could not tell a failed lookup from an id that matched no service. - **`packages/framework/test/setup.ts`** was an unreferenced sibling of `tests/setup.ts` registering the jest-dom matchers but no `afterEach(cleanup)`. Deleted, so a future `setupFiles` edit cannot silently drop DOM isolation for the package. **Deliberately not ported**, matching the decisions recorded on the PMM PR: production SEP routing / token exchange, flattening dotted app argument names (they denote nested backend models, which `coerceFormValues` already produces), converting `ScriptPreviewField` to TanStack Query, committing untrimmed free-solo input, relocating the `api` / `atw` test suites, `related_apps` alongside `entities`, and the hard-coded `Roboto Mono` stacks. `formatCellValue`'s `undefined` guard is already here — in a better form than the port has, which should go back to PMM.
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>
nachodd
marked this pull request as ready for review
August 10, 2026 12:51
fabio-silva
approved these changes
Aug 10, 2026
nachodd
requested review from
ademidoff and
maxkondr
and removed request for
a team
August 10, 2026 18:31
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## PMM-15216 #5739 +/- ##
=============================================
+ Coverage 45.42% 45.54% +0.11%
=============================================
Files 418 418
Lines 43336 43409 +73
=============================================
+ Hits 19685 19769 +84
+ Misses 21711 21684 -27
- Partials 1940 1956 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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>
1 task
mattiasimonato
approved these changes
Aug 11, 2026
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>
Collaborator
|
@coderabbitai full review |
✅ Action performedFull review finished. |
WalkthroughChangesSEP authentication
Go runtime reliability
Supporting project updates
Sequence Diagram(s)sequenceDiagram
participant SepPage
participant SepAuthGate
participant sepTokenStore
participant SEPAPI
SepPage->>SepAuthGate: render SEP content
SepAuthGate->>sepTokenStore: ensureSepToken()
sepTokenStore->>SEPAPI: postSessionExchange()
SEPAPI-->>sepTokenStore: short-lived bearer token
sepTokenStore-->>SepAuthGate: ready state
SepAuthGate-->>SepPage: render authenticated content
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (13)
agent/agents/postgres/pgstatmonitor/pgstatmonitor_test.go (2)
604-614: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse fatal length checks before indexing.
assert.Lenrecords a failure but continues. Lines 666-667 and Lines 674-675 can panic after a failed length check.require.NotNilat Line 610 does not reject an empty non-nil slice, and Line 638 indexes without a length check. Userequire.Lenbefore each index, or compare the complete result to a golden fixture. The test must report the defect, not hide it behind a panic. Make it so.Proposed fix
- require.NotNil(t, res) + require.Len(t, res, len(getHistogramRangesArray(vPGSM))) - assert.Len(t, res, 22) + require.Len(t, res, 22) - assert.Len(t, res, 10) + require.Len(t, res, 10)Also applies to: 630-639, 662-668, 670-675
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/agents/postgres/pgstatmonitor/pgstatmonitor_test.go` around lines 604 - 614, Update the parseHistogramFromRespCalls tests to use fatal require.Len checks before every indexed access, including the Normal case and the cases around lines 630-639 and 662-675. Replace non-fatal assert.Len checks and ensure the result length is validated before indexing, while retaining the existing value assertions.
598-657: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the histogram parser tests table-driven.
TestParseHistogramFromRespCallsrepeats setup in separate subtests and checks only selected scalar fields. Use one table ofrespCalls,prevRespCalls, expected errors, and expected histogram fixtures. Include invalid and overlong cases for both input arrays. As per coding guidelines, parser tests underagent/**/*_test.gomust use table-driven tests with golden files. Make it so.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/agents/postgres/pgstatmonitor/pgstatmonitor_test.go` around lines 598 - 657, Refactor TestParseHistogramFromRespCalls into a single table-driven test covering normal, counter-reset, invalid, negative, and overlong inputs for both respCalls and prevRespCalls. Define expected errors and complete histogram fixtures per case, compare results against golden files, and retain parallel-safe subtest execution while removing duplicated setup and scalar-only assertions.Source: Coding guidelines
agent/utils/templates/template_test.go (1)
39-39: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
t.TempDir()for this parallel test.
os.TempDir()plus a random suffix can collide during parallel execution. Manual cleanup is also skipped when a fatal assertion exits the test early. Uset.TempDir()and remove the explicitos.RemoveAllcleanup.Proposed test-isolation change
- dir := filepath.Join(os.TempDir(), fmt.Sprintf("pg_action_%05d", rand.Int63n(99999))) + dir := t.TempDir()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/utils/templates/template_test.go` at line 39, Update the test temporary-directory setup around the generated dir variable to use t.TempDir() instead of os.TempDir() with a random suffix. Remove the corresponding explicit os.RemoveAll cleanup, relying on t.TempDir() to provide isolated directories and automatic cleanup even after fatal assertions.agent/agents/mongodb/profiler/mongodb.go (1)
75-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a structured error field.
Make it so: preserve
errwithWithError(err)instead of formatting it into the message. This keeps profiler shutdown failures queryable.Proposed change
- m.l.Errorf("Can't stop profiler, reason: %v", err) + m.l.WithError(err).Error("cannot stop profiler")As per coding guidelines, use structured Logrus logging with
*logrus.Entry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/agents/mongodb/profiler/mongodb.go` around lines 75 - 78, Update the profiler shutdown error logging after prof.Stop() to use the structured Logrus entry returned by m.l.WithError(err), while keeping the existing descriptive message and error-level logging. Do not format err into the message; preserve it as the structured error field.Source: Coding guidelines
agent/agents/mongodb/profiler/internal/parser/parser_test.go (1)
94-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove test comments that repeat the code. Keep comments that explain the cancellation-ordering requirement. Remove comments that only narrate assertions, calls, or successful branches.
agent/agents/mongodb/profiler/internal/parser/parser_test.go#L94-L112: retain the explanation for delayingStop; remove assertion and success narration.agent/agents/mongodb/profiler/internal/collector/collector_test.go#L190-L223: retain the explanation for delayingStop; remove assertion and success narration.As per coding guidelines, avoid redundant comments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/agents/mongodb/profiler/internal/parser/parser_test.go` around lines 94 - 112, Remove redundant assertion and successful-branch narration comments in parser_test.go lines 94-112, retaining only the comment explaining why Stop must be delayed; apply the same cleanup to collector_test.go lines 190-223, preserving its Stop-ordering explanation and leaving test behavior unchanged.Source: Coding guidelines
ui/apps/pmm/src/sep/sepTokenStore.test.ts (1)
59-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth teardowns undo one of the four registered seams.
initSepAuth()registerssetTokenProvider,setTokenMinter,setOnRefreshed, andsetOnUnauthorizedon the shared@sep/apimodule. EachafterEachclears the minter alone, so the other three stay bound to this store for any later suite that shares the module registry. The state is reset, so nothing fails today, but a future SEP test would inherit callbacks it never installed.Make the teardown symmetric with the setup at both sites.
ui/apps/pmm/src/sep/sepTokenStore.test.ts#L59-L63: clearsetTokenProvider,setOnRefreshed, andsetOnUnauthorizedbesidesetTokenMinter(null), or call a sharedteardownSepAuth()helper.ui/apps/pmm/src/sep/SepAuthGate.test.tsx#L43-L46: apply the same teardown, and share it with the store suite rather than duplicating it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/apps/pmm/src/sep/sepTokenStore.test.ts` around lines 59 - 63, Make teardown symmetric with initSepAuth by clearing setTokenProvider, setTokenMinter, setOnRefreshed, and setOnUnauthorized. Update both ui/apps/pmm/src/sep/sepTokenStore.test.ts:59-63 and ui/apps/pmm/src/sep/SepAuthGate.test.tsx:43-46, preferably extracting and sharing a teardownSepAuth() helper rather than duplicating the cleanup.ui/apps/pmm/src/sep/SepAuthGate.tsx (1)
18-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe retry gives no feedback from the notice bar.
If the gate renders the notice bar, the phase stays
ready. A click on Retry then starts an exchange with no visible change until it resolves. The user may click again. The calls coalesce, so nothing breaks, but the control appears inert.Hold the returned promise in local state and disable the button while the exchange is in flight.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/apps/pmm/src/sep/SepAuthGate.tsx` around lines 18 - 28, Update the RetryButton component to track the promise returned by retrySepAuth in local state, setting it when the exchange starts and clearing it when it settles. Disable the Button while that promise is pending so repeated clicks are prevented and the in-flight state is visible through the control.ui/apps/pmm/src/sep/sepTokenStore.ts (2)
319-328: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
listeners.clear()detaches live subscribers.The helper is documented as test-only, but it drops every subscription. A mounted
SepAuthGatewould then never re-render on a later phase change. The unsubscribe closures stay harmless, so the fault would be silent.Number one, make the reset leave the subscriber set alone.
♻️ Proposed change
sessionRejected = false; renewalRetries = 0; snapshot = { phase, notice }; - listeners.clear(); + publish(); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/apps/pmm/src/sep/sepTokenStore.ts` around lines 319 - 328, Update resetSepAuthStore so it resets the authentication state without clearing listeners; remove the listeners.clear() call and leave the existing subscriber set intact for mounted SepAuthGate instances.
202-223: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider a recovery trigger after the backoff budget is exhausted.
After four failed renewals,
failClosed('unreachable')runs and no timer remains. The page then stays mounted with no credential until the user clicks Retry. If the network returns one second later, nothing notices.Engage a listener on
windowonline, or re-arm one long-delay attempt, so the connection heals without user action. The stickysessionRejectedguard still prevents a loop for a rejected session.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/apps/pmm/src/sep/sepTokenStore.ts` around lines 202 - 223, Update the renew function’s exhausted-retry path after failClosed('unreachable') to establish an automatic recovery trigger, preferably a window online listener that re-attempts renewal when connectivity returns. Ensure the trigger does not create duplicate listeners or retry loops, and preserve the sessionRejected guard so rejected sessions remain terminal.ui/packages/sep/api/src/typed-client.ts (1)
92-108: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the replay fetch against rejection.
lazyFetch(pristine)can reject — a network failure is the standardfetchrejection. The rejection then escapesonResponse, soemitUnauthorized()at line 149 never runs and the caller receives a network error in place of the original 401. Treat a failed replay the same as no replay.🛡️ Proposed fix
pristine.headers.set('Authorization', `Bearer ${token}`); - return lazyFetch(pristine); + try { + return await lazyFetch(pristine); + } catch { + // The replay leg failed on the wire; fall back to the original response. + return null; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/api/src/typed-client.ts` around lines 92 - 108, Update replayWithFreshToken to catch rejections from lazyFetch(pristine) and return null, treating replay failures the same as an unavailable replay so onResponse can continue to emitUnauthorized() and preserve the original 401 behavior.ui/apps/pmm/vite.config.ts (1)
86-88: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueStrip the prefix only on a true segment boundary.
Vite matches proxy keys by prefix, so a sibling path such as
/september/xalso reaches this proxy.slicethen yieldstember/x, a path with no leading slash. Anchor the strip to the boundary.♻️ Optional guard
- ? { rewrite: (path: string) => path.slice(SEP_BASE_PATH.length) || '/' } + ? { + rewrite: (path: string) => + path === SEP_BASE_PATH || path.startsWith(`${SEP_BASE_PATH}/`) + ? path.slice(SEP_BASE_PATH.length) || '/' + : path, + } : {}),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/apps/pmm/vite.config.ts` around lines 86 - 88, Update the rewrite callback in the sepStripPrefix proxy configuration to remove SEP_BASE_PATH only when the request path matches that prefix followed by a segment boundary, such as '/' or the end of the path. Preserve unrelated sibling paths unchanged and retain '/' for an exact-prefix match.ui/packages/sep/api/src/client.ts (2)
146-162: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider matching on the pathname, not on a substring.
includesmatches the literal anywhere in the URL, query string included. A request such as/api/foo?next=/oauth/sessionwould be classified as a mint request and would lose its one silent retry. Parsing the pathname removes the ambiguity for both predicates.♻️ Optional hardening
+const pathOf = (url: string | undefined) => { + if (!url) return ''; + try { + return new URL(url, 'http://x').pathname; + } catch { + return url; + } +}; + const isRefreshRequest = (url: string | undefined) => - !!url && url.includes('/oauth/refresh'); + pathOf(url).endsWith('/oauth/refresh');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/api/src/client.ts` around lines 146 - 162, Update isRefreshRequest and isTokenMintRequest to parse the URL and match only the pathname for the OAuth endpoint segments, rather than using substring checks that can match query parameters or unrelated paths. Preserve the existing token-mint classification and ensure isLoginRequest follows the same pathname-based matching behavior.
42-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport
TokenMinterfrom the public API.Export the alias from
client.tsand re-export it fromsrc/index.ts.SessionExchangeTokenResponsesatisfiesMintedTokenbecause both required fields are present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/api/src/client.ts` around lines 42 - 74, Export the TokenMinter type alias from client.ts, then re-export it through src/index.ts alongside the other public client API symbols. Keep its existing Promise<MintedToken | null> contract unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@admin/commands/summary.go`:
- Around line 192-195: Replace the unstructured cleanup error logs with
structured Logrus entries: in admin/commands/summary.go lines 192-195 and
283-286, use WithField for the archive entry name and WithError for the close
error before Error; in agent/runner/jobs/pbm_helpers.go lines 704-708, use
WithField for the temporary file path and WithError for the removal error before
Error.
In `@agent/agents/mongodb/profiler/internal/collector/collector.go`:
- Around line 186-187: Update the cursor cleanup defer in the collector flow to
create a context.WithTimeout for cursor.Close, ensuring shutdown cleanup has a
bounded deadline. Capture any Close error and log it through the collector’s
existing logger, then cancel the timeout context after cleanup; do not pass
context.Background() directly or discard the close error.
In `@api/alerting/v1/json/v1.json`:
- Line 720: Update the API tag descriptions at
api/alerting/v1/json/v1.json:720-720 to replace “lets to manage” with “allows
users to manage”; at api/inventory/v1/json/v1.json:16206-16206 to replace
“Services service” with “ServicesService”; and at
api/realtimeanalytics/v1/json/v1.json:634-634 to change “provides public API” to
“provides a public API”.
In `@qan-api2/db_test.go`:
- Line 72: Replace the shell-based execution around exec.CommandContext with
direct Docker and ClickHouse argument passing, removing cmdStr and the /bin/sh
-c wrapper. Before building the DROP DATABASE query, validate dbName against the
allowed ClickHouse identifier format and reject invalid values; ensure the
validated name is the only value interpolated into the query.
---
Nitpick comments:
In `@agent/agents/mongodb/profiler/internal/parser/parser_test.go`:
- Around line 94-112: Remove redundant assertion and successful-branch narration
comments in parser_test.go lines 94-112, retaining only the comment explaining
why Stop must be delayed; apply the same cleanup to collector_test.go lines
190-223, preserving its Stop-ordering explanation and leaving test behavior
unchanged.
In `@agent/agents/mongodb/profiler/mongodb.go`:
- Around line 75-78: Update the profiler shutdown error logging after
prof.Stop() to use the structured Logrus entry returned by m.l.WithError(err),
while keeping the existing descriptive message and error-level logging. Do not
format err into the message; preserve it as the structured error field.
In `@agent/agents/postgres/pgstatmonitor/pgstatmonitor_test.go`:
- Around line 604-614: Update the parseHistogramFromRespCalls tests to use fatal
require.Len checks before every indexed access, including the Normal case and
the cases around lines 630-639 and 662-675. Replace non-fatal assert.Len checks
and ensure the result length is validated before indexing, while retaining the
existing value assertions.
- Around line 598-657: Refactor TestParseHistogramFromRespCalls into a single
table-driven test covering normal, counter-reset, invalid, negative, and
overlong inputs for both respCalls and prevRespCalls. Define expected errors and
complete histogram fixtures per case, compare results against golden files, and
retain parallel-safe subtest execution while removing duplicated setup and
scalar-only assertions.
In `@agent/utils/templates/template_test.go`:
- Line 39: Update the test temporary-directory setup around the generated dir
variable to use t.TempDir() instead of os.TempDir() with a random suffix. Remove
the corresponding explicit os.RemoveAll cleanup, relying on t.TempDir() to
provide isolated directories and automatic cleanup even after fatal assertions.
In `@ui/apps/pmm/src/sep/SepAuthGate.tsx`:
- Around line 18-28: Update the RetryButton component to track the promise
returned by retrySepAuth in local state, setting it when the exchange starts and
clearing it when it settles. Disable the Button while that promise is pending so
repeated clicks are prevented and the in-flight state is visible through the
control.
In `@ui/apps/pmm/src/sep/sepTokenStore.test.ts`:
- Around line 59-63: Make teardown symmetric with initSepAuth by clearing
setTokenProvider, setTokenMinter, setOnRefreshed, and setOnUnauthorized. Update
both ui/apps/pmm/src/sep/sepTokenStore.test.ts:59-63 and
ui/apps/pmm/src/sep/SepAuthGate.test.tsx:43-46, preferably extracting and
sharing a teardownSepAuth() helper rather than duplicating the cleanup.
In `@ui/apps/pmm/src/sep/sepTokenStore.ts`:
- Around line 319-328: Update resetSepAuthStore so it resets the authentication
state without clearing listeners; remove the listeners.clear() call and leave
the existing subscriber set intact for mounted SepAuthGate instances.
- Around line 202-223: Update the renew function’s exhausted-retry path after
failClosed('unreachable') to establish an automatic recovery trigger, preferably
a window online listener that re-attempts renewal when connectivity returns.
Ensure the trigger does not create duplicate listeners or retry loops, and
preserve the sessionRejected guard so rejected sessions remain terminal.
In `@ui/apps/pmm/vite.config.ts`:
- Around line 86-88: Update the rewrite callback in the sepStripPrefix proxy
configuration to remove SEP_BASE_PATH only when the request path matches that
prefix followed by a segment boundary, such as '/' or the end of the path.
Preserve unrelated sibling paths unchanged and retain '/' for an exact-prefix
match.
In `@ui/packages/sep/api/src/client.ts`:
- Around line 146-162: Update isRefreshRequest and isTokenMintRequest to parse
the URL and match only the pathname for the OAuth endpoint segments, rather than
using substring checks that can match query parameters or unrelated paths.
Preserve the existing token-mint classification and ensure isLoginRequest
follows the same pathname-based matching behavior.
- Around line 42-74: Export the TokenMinter type alias from client.ts, then
re-export it through src/index.ts alongside the other public client API symbols.
Keep its existing Promise<MintedToken | null> contract unchanged.
In `@ui/packages/sep/api/src/typed-client.ts`:
- Around line 92-108: Update replayWithFreshToken to catch rejections from
lazyFetch(pristine) and return null, treating replay failures the same as an
unavailable replay so onResponse can continue to emitUnauthorized() and preserve
the original 401 behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d5f29ed2-5440-4ffb-af97-3f8e6c3107ff
⛔ Files ignored due to path filters (17)
api/accesscontrol/v1beta1/accesscontrol.pb.gw.gois excluded by!**/*.pb.gw.goapi/actions/v1/actions.pb.gw.gois excluded by!**/*.pb.gw.goapi/advisors/v1/advisors.pb.gw.gois excluded by!**/*.pb.gw.goapi/agentlocal/v1/agentlocal.pb.gw.gois excluded by!**/*.pb.gw.goapi/alerting/v1/alerting.pb.gw.gois excluded by!**/*.pb.gw.goapi/backup/v1/backup.pb.gw.gois excluded by!**/*.pb.gw.goapi/backup/v1/locations.pb.gw.gois excluded by!**/*.pb.gw.goapi/backup/v1/restores.pb.gw.gois excluded by!**/*.pb.gw.goapi/dump/v1beta1/dump.pb.gw.gois excluded by!**/*.pb.gw.goapi/inventory/v1/agents.pb.gw.gois excluded by!**/*.pb.gw.goapi/inventory/v1/nodes.pb.gw.gois excluded by!**/*.pb.gw.goapi/inventory/v1/services.pb.gw.gois excluded by!**/*.pb.gw.goapi/management/v1/service.pb.gw.gois excluded by!**/*.pb.gw.goapi/qan/v1/service.pb.gw.gois excluded by!**/*.pb.gw.goapi/realtimeanalytics/v1/realtimeanalytics.pb.gw.gois excluded by!**/*.pb.gw.goapi/server/v1/server.pb.gw.gois excluded by!**/*.pb.gw.gogo.sumis excluded by!**/*.sum
📒 Files selected for processing (84)
.github/workflows/api-docs.yml.golangci.ymladmin/commands/inventory/change_agent_rds_exporter_test.goadmin/commands/summary.goagent/agents/mongodb/mongolog/internal/mongolog.goagent/agents/mongodb/mongolog/internal/monitor_test.goagent/agents/mongodb/mongolog/mongodb.goagent/agents/mongodb/profiler/internal/collector/collector.goagent/agents/mongodb/profiler/internal/collector/collector_test.goagent/agents/mongodb/profiler/internal/parser/parser.goagent/agents/mongodb/profiler/internal/parser/parser_test.goagent/agents/mongodb/profiler/internal/profiler.goagent/agents/mongodb/profiler/internal/profiler_test.goagent/agents/mongodb/profiler/mongodb.goagent/agents/mongodb/shared/aggregator/aggregator.goagent/agents/mysql/perfschema/perfschema.goagent/agents/mysql/slowlog/parser/parser_test.goagent/agents/mysql/slowlog/slowlog.goagent/agents/mysql/slowlog/slowlog_test.goagent/agents/postgres/parser/parser_test.goagent/agents/postgres/pgstatmonitor/pgstatmonitor.goagent/agents/postgres/pgstatmonitor/pgstatmonitor_test.goagent/agents/postgres/pgstatstatements/pgstatstatements.goagent/agents/postgres/pgstatstatements/pgstatstatements_test.goagent/agents/process/process_test.goagent/agents/supervisor/supervisor_test.goagent/client/client_test.goagent/cmd/pmm-agent-entrypoint/main.goagent/config/config_test.goagent/connectionchecker/connection_checker_test.goagent/main_test.goagent/runner/actions/postgresql_show_create_table_action.goagent/runner/actions/pt_mysql_summary_action.goagent/runner/jobs/mongodb_backup_job.goagent/runner/jobs/mongodb_restore_job.goagent/runner/jobs/mysql_restore_job.goagent/runner/jobs/pbm_helpers.goagent/serviceinfobroker/service_info_broker_test.goagent/utils/templates/template_test.goagent/versioner/versioner.goapi-tests/alerting/alerting_test.goapi-tests/alerting/multiexpr_filter_test.goapi-tests/server/auth_test.goapi/accesscontrol/v1beta1/json/v1beta1.jsonapi/actions/v1/json/v1.jsonapi/advisors/v1/json/v1.jsonapi/agentlocal/v1/json/v1.jsonapi/alerting/v1/json/v1.jsonapi/backup/v1/json/v1.jsonapi/ha/v1beta1/json/v1beta1.jsonapi/inventory/v1/json/v1.jsonapi/management/v1/json/v1.jsonapi/qan/v1/json/v1.jsonapi/realtimeanalytics/v1/json/v1.jsonapi/server/v1/json/v1.jsonapi/swagger/swagger-dev.jsonapi/swagger/swagger.jsonapi/user/v1/json/v1.jsongo.modmanaged/cmd/pmm-managed-starlark/main_test.gomanaged/services/encryption/encryption_rotation_test.gomanaged/services/supervisord/devcontainer_test.gomanaged/services/supervisord/pmm_config_test.gomanaged/services/supervisord/supervisord_test.gomanaged/services/telemetry/telemetry_test.goqan-api2/db_test.goqan-api2/services/analytics/profile_test.goui/apps/pmm/src/pages/rta/overview/table/OverviewTable.constants.tsxui/apps/pmm/src/pages/rta/overview/table/OverviewTable.test.tsxui/apps/pmm/src/sep/SepAuthGate.messages.tsui/apps/pmm/src/sep/SepAuthGate.test.tsxui/apps/pmm/src/sep/SepAuthGate.tsxui/apps/pmm/src/sep/SepPage.tsxui/apps/pmm/src/sep/bootstrap.tsui/apps/pmm/src/sep/sepTokenStore.test.tsui/apps/pmm/src/sep/sepTokenStore.tsui/apps/pmm/vite.config.tsui/packages/sep/api/README.mdui/packages/sep/api/src/client.tsui/packages/sep/api/src/index.tsui/packages/sep/api/src/typed-client.tsui/packages/sep/api/tests/client.test.tsui/packages/sep/api/tests/typed-client.test.tsutils/logger/protobuf_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual)
💤 Files with no reviewable changes (2)
- admin/commands/inventory/change_agent_rds_exporter_test.go
- utils/logger/protobuf_test.go
Comment on lines
+192
to
+195
| err = rc.Close() | ||
| if err != nil { | ||
| logrus.Errorf("Failed to close zip archive %s: %v", rf.Name, err) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use structured Logrus entries for cleanup errors.
Make it so: attach the resource name and error with logrus.WithField(...).WithError(...).Error(...) instead of logrus.Errorf(...).
admin/commands/summary.go#L192-L195: log the archive entry name and close error as fields.admin/commands/summary.go#L283-L286: log the archive entry name and close error as fields.agent/runner/jobs/pbm_helpers.go#L704-L708: log the temporary file path and removal error as fields.
As per coding guidelines, use structured Logrus logging with *logrus.Entry.
📍 Affects 2 files
admin/commands/summary.go#L192-L195(this comment)admin/commands/summary.go#L283-L286agent/runner/jobs/pbm_helpers.go#L704-L708
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@admin/commands/summary.go` around lines 192 - 195, Replace the unstructured
cleanup error logs with structured Logrus entries: in admin/commands/summary.go
lines 192-195 and 283-286, use WithField for the archive entry name and
WithError for the close error before Error; in agent/runner/jobs/pbm_helpers.go
lines 704-708, use WithField for the temporary file path and WithError for the
removal error before Error.
Source: Coding guidelines
Comment on lines
+186
to
+187
| // Ensure cursor is closed even if parent context is canceled to prevent resource leaks. | ||
| defer cursor.Close(context.Background()) //nolint:errcheck,contextcheck |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- collector outline ---'
ast-grep outline agent/agents/mongodb/profiler/internal/collector/collector.go --view expanded || true
printf '%s\n' '--- collector relevant lines ---'
sed -n '130,240p' agent/agents/mongodb/profiler/internal/collector/collector.go
printf '%s\n' '--- cursor timeout references ---'
rg -n --glob '*.go' 'cursorTimeout|Cursor\.Close|cursor\.Close|func \(.*Collector.*Stop|Collector\.Stop|doneChan|docsChan' agent/agents/mongodb/profiler
printf '%s\n' '--- MongoDB driver declarations ---'
rg -n 'go.mongodb.org/mongo-driver|mongo-driver' go.mod go.sum agent/go.mod agent/go.sum 2>/dev/null || true
printf '%s\n' '--- repository guidance ---'
find .. -name AGENTS.md -printRepository: percona/pmm
Length of output: 14130
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- collector lifecycle ---'
sed -n '40,130p' agent/agents/mongodb/profiler/internal/collector/collector.go
printf '%s\n' '--- collector tail ---'
sed -n '235,285p' agent/agents/mongodb/profiler/internal/collector/collector.go
printf '%s\n' '--- profiler shutdown ---'
sed -n '80,155p' agent/agents/mongodb/profiler/internal/profiler.go
printf '%s\n' '--- applicable guidance ---'
cat AGENTS.md
printf '%s\n' '--- MongoDB driver module locations ---'
find /go/pkg/mod -path '*mongo-driver*' -maxdepth 6 -type f \( -name 'mongo.go' -o -name 'cursor.go' \) -print 2>/dev/null | head -20 || true
printf '%s\n' '--- cached Cursor.Close definitions ---'
rg -n -A45 -B10 'func \(.*Cursor.*\) Close|func \(.*Cursor.*\) close' /go/pkg/mod/go.mongodb.org /root/go/pkg/mod/go.mongodb.org 2>/dev/null || trueRepository: percona/pmm
Length of output: 35260
🌐 Web query:
MongoDB Go Driver v1.17.7 mongo.Cursor.Close context cancellation implementation
💡 Result:
In the MongoDB Go Driver (including version 1.17.7), the mongo.Cursor.Close(ctx) method does honor the provided context for cancellation [1][2][3]. When you call Close, the driver may attempt to communicate with the MongoDB server to kill the cursor remotely [4][5]. If the provided context is canceled or reaches its deadline before this network operation completes, the operation will be aborted, and the method will return an error indicating the context cancellation [6]. Because Close honors the context, it is a common best practice to use context.Background when calling Close in a defer statement [1][7][2]. This ensures that the cursor cleanup (a critical resource-freeing operation) is not blocked or aborted by the cancellation of a context that may have already expired—for example, if the primary operation (like a request handler) was canceled [1][8]. Example of recommended usage: defer cursor.Close(context.Background) Using context.Background in this context guarantees that the cleanup logic has a reasonable chance to execute, preventing potential server-side cursor leaks that could occur if the Close call itself were canceled prematurely [1][8].
Citations:
- 1: https://github.com/mongodb/mongo-go-driver/blob/v1.17.1/mongo/cursor.go
- 2: https://github.com/mongodb/mongo-go-driver/blob/v1.17.9/mongo/cursor.go
- 3: https://github.com/mongodb/mongo-go-driver/blob/60f76f5b1810553090503950fce37c876253217b/mongo/cursor.go
- 4: https://www.mongodb.com/docs/drivers/go/current/crud/query/cursor/
- 5: https://www.mongodb.com/docs/drivers/go/v1.x/fundamentals/crud/read-operations/cursor/
- 6: https://www.mongodb.com/docs/drivers/go/current/context/
- 7: https://github.com/mongodb/mongo-go-driver/blob/4f06ad2489b73cd1dbcebb5e7df09b77cb643be1/mongo/cursor.go
- 8: https://www.mongodb.com/community/forums/t/mongodb-go-driver-context-with-cursor-close-session-endsession/6748
🏁 Script executed (no clone):
#!/bin/bash
set -eu
base='https://raw.githubusercontent.com/mongodb/mongo-go-driver/v1.17.7'
printf '%s\n' '--- cursor.go Close implementation ---'
curl -fsSL "$base/mongo/cursor.go" | nl -ba | sed -n '/func (c \*Cursor) Close/,/^[[:space:]]*}/p'
printf '%s\n' '--- cursor.go surrounding Close implementation ---'
curl -fsSL "$base/mongo/cursor.go" | nl -ba | rg -n -A55 -B10 'func \(c \*Cursor\) Close'
printf '%s\n' '--- kill-cursor operation references ---'
for path in mongo/cursor.go mongo/operation/kill_cursors.go mongo/operation/operation.go mongo/client.go; do
printf '%s\n' "--- $path ---"
curl -fsSL "$base/$path" | rg -n -A35 -B10 'KillCursors|killCursors|Execute\(ctx|serverSelectionTimeout|SetContext'
done
Length of output: 435
🏁 Script executed (no clone):
#!/bin/bash
set -eu
python3 - <<'PY'
import re
import urllib.request
base = "https://raw.githubusercontent.com/mongodb/mongo-go-driver/v1.17.7/"
text = urllib.request.urlopen(base + "mongo/cursor.go").read().decode()
match = re.search(r'func \(c \*Cursor\) Close\(ctx context\.Context\) error \{.*?(?=\n\})\n\}', text, re.S)
if not match:
raise SystemExit("Cursor.Close implementation not found")
body = match.group(0)
print(body)
print("\ncontains context passed to kill operation:", "ctx" in body and "KillCursors" in body)
PY
Length of output: 2411
🏁 Script executed (no clone):
#!/bin/bash
set -eu
base='https://raw.githubusercontent.com/mongodb/mongo-go-driver/v1.17.7'
tmp=$(mktemp)
trap 'rm -f "$tmp"' EXIT
curl -kfsSL "$base/mongo/cursor.go" > "$tmp"
printf '%s\n' '--- Cursor.Close implementation ---'
sed -n '/func (c \*Cursor) Close/,/^}/p' "$tmp"
printf '%s\n' '--- Close call sites and operation setup ---'
rg -n -A35 -B12 'func \(c \*Cursor\) Close|KillCursors|killCursors' "$tmp"
printf '%s\n' '--- operation implementation ---'
for path in mongo/operation/kill_cursors.go mongo/operation/operation.go; do
printf '%s\n' "--- $path ---"
curl -kfsSL "$base/$path" | rg -n -A45 -B12 'func .*Execute|type KillCursors|KillCursors|ctx context.Context'
done
Length of output: 2681
🏁 Script executed (no clone):
#!/bin/bash
set -eu
base='https://raw.githubusercontent.com/mongodb/mongo-go-driver/v1.17.7'
for path in mongo/batch_cursor.go mongo/cursor.go mongo/operation/kill_cursors.go mongo/operation/operation.go; do
printf '%s\n' "--- $path ---"
if curl -kfsSL "$base/$path" > /tmp/mongo-driver-source; then
rg -n -A55 -B15 'func \(.*\) Close|KillCursors|killCursors|func .*Execute|ctx context.Context|serverSelectionTimeout' /tmp/mongo-driver-source || true
else
printf '%s\n' 'not found'
fi
done
Length of output: 7370
🏁 Script executed (no clone):
#!/bin/bash
set -eu
curl -kfsSL 'https://api.github.com/repos/mongodb/mongo-go-driver/git/trees/v1.17.7?recursive=1' |
python3 -c '
import json, sys
data=json.load(sys.stdin)
for item in data.get("tree", []):
p=item.get("path","")
if "cursor" in p.lower() or p.endswith("operation.go"):
print(p)
'
Length of output: 831
🏁 Script executed (no clone):
#!/bin/bash
set -eu
base='https://raw.githubusercontent.com/mongodb/mongo-go-driver/v1.17.7'
for path in x/mongo/driver/batch_cursor.go x/mongo/driver/operation/kill_cursors.go x/mongo/driver/operation/operation.go x/mongo/driver/topology/server.go; do
printf '%s\n' "--- $path ---"
if curl -kfsSL "$base/$path" > /tmp/mongo-driver-source; then
rg -n -A65 -B15 'func \(.*\) Close|KillCursors|killCursors|func .*Execute|ctx context.Context|ServerSelectionTimeout|SocketTimeout' /tmp/mongo-driver-source || true
else
printf '%s\n' 'not found'
fi
done
Length of output: 23669
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- collector shutdown tests ---'
sed -n '180,310p' agent/agents/mongodb/profiler/internal/collector/collector_test.go
printf '%s\n' '--- MongoDB timeout and shutdown patterns ---'
rg -n -A12 -B8 --glob '*.go' 'context\.WithTimeout\(context\.Background\(\)|Close\(context\.Background\(\)|Disconnect\(ctx\)|Stop\(\)' agent/agents/mongodbRepository: percona/pmm
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- MongoDB timeout constants ---'
rg -n -A4 -B4 --glob '*.go' 'mgoTimeoutSessionSocket|mgoTimeoutSessionSync|mgoTimeoutDialInfo' agent/agents/mongodb
printf '%s\n' '--- current line history ---'
git blame -L 178,192 -- agent/agents/mongodb/profiler/internal/collector/collector.go || true
printf '%s\n' '--- focused diff for collector ---'
git diff --unified=12 -- agent/agents/mongodb/profiler/internal/collector/collector.go | sed -n '1,140p'Repository: percona/pmm
Length of output: 16771
Bound cursor cleanup during shutdown.
cursor.Close sends a killCursors command, and Collector.Stop() waits for it. Use a context.WithTimeout context and log close errors instead of passing context.Background() directly. The client socket timeout limits the current wait to 5 seconds, but shutdown remains blocked during that interval.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent/agents/mongodb/profiler/internal/collector/collector.go` around lines
186 - 187, Update the cursor cleanup defer in the collector flow to create a
context.WithTimeout for cursor.Close, ensuring shutdown cleanup has a bounded
deadline. Capture any Close error and log it through the collector’s existing
logger, then cancel the timeout context after cleanup; do not pass
context.Background() directly or discard the close error.
Source: Coding guidelines
| }, | ||
| "tags": [ | ||
| { | ||
| "description": "Alerting service lets to manage alerting templates and create alerting rules from them.", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the API tag descriptions before publishing the documentation.
These descriptions contain grammar or terminology issues that reduce clarity in generated API documentation. Apply the following changes:
api/alerting/v1/json/v1.json#L720-L720: replace"lets to manage"with"allows users to manage".api/inventory/v1/json/v1.json#L16206-L16206: replace"Services service"with"ServicesService".api/realtimeanalytics/v1/json/v1.json#L634-L634: change"provides public API"to"provides a public API".
Make it so.
📍 Affects 3 files
api/alerting/v1/json/v1.json#L720-L720(this comment)api/inventory/v1/json/v1.json#L16206-L16206api/realtimeanalytics/v1/json/v1.json#L634-L634
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/alerting/v1/json/v1.json` at line 720, Update the API tag descriptions at
api/alerting/v1/json/v1.json:720-720 to replace “lets to manage” with “allows
users to manage”; at api/inventory/v1/json/v1.json:16206-16206 to replace
“Services service” with “ServicesService”; and at
api/realtimeanalytics/v1/json/v1.json:634-634 to change “provides public API” to
“provides a public API”.
|
|
||
| cmdStr := fmt.Sprintf(`docker exec pmm-clickhouse-test clickhouse client --password=clickhouse --query='DROP DATABASE IF EXISTS %s;'`, dbName) | ||
| out, err := exec.CommandContext(context.Background(), "/bin/sh", "-c", cmdStr).Output() //nolint:gosec | ||
| out, err := exec.CommandContext(context.Background(), "/bin/sh", "-c", cmdStr).Output() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove the shell wrapper before removing the suppression.
dbName is interpolated into cmdStr and passed to /bin/sh -c. A value containing shell metacharacters can execute arbitrary commands in the CI environment. Pass the Docker and ClickHouse arguments directly, and validate dbName as an allowed ClickHouse identifier before constructing the DROP DATABASE query.
As per coding guidelines, external input must be validated and bounded before it reaches a command or query.
Proposed command-construction change
- cmdStr := fmt.Sprintf(`docker exec pmm-clickhouse-test clickhouse client --password=clickhouse --query='DROP DATABASE IF EXISTS %s;'`, dbName)
- out, err := exec.CommandContext(context.Background(), "/bin/sh", "-c", cmdStr).Output()
+ if !validClickHouseDatabaseName(dbName) {
+ t.Fatalf("invalid database name %q", dbName)
+ }
+ query := "DROP DATABASE IF EXISTS " + dbName + ";"
+ out, err := exec.CommandContext(
+ context.Background(),
+ "docker", "exec", "pmm-clickhouse-test", "clickhouse", "client",
+ "--password=clickhouse", "--query="+query,
+ ).Output()🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 72-72: Dynamic command passed to exec.Command with a shell invocation. Pass arguments directly to exec.Command without a shell wrapper.
(coderabbit.command-injection.go-exec-command)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@qan-api2/db_test.go` at line 72, Replace the shell-based execution around
exec.CommandContext with direct Docker and ClickHouse argument passing, removing
cmdStr and the /bin/sh -c wrapper. Before building the DROP DATABASE query,
validate dbName against the allowed ClickHouse identifier format and reject
invalid values; ensure the validated name is the only value interpolated into
the query.
Sources: Coding guidelines, Linters/SAST tools
…ssion-exchange # Conflicts: # ui/packages/sep/api/src/typed-client.ts
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>
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>
yyyyyyyan
added a commit
that referenced
this pull request
Aug 21, 2026
#5755 (PMM-15280, Grafana service account) and #5768 (PMM-15331, the health gate that depended on it) were closed unmerged: provisioning the account by writing Grafana's rows directly was the wrong shape, and with that gone SEP provisioning is synchronous, leaving the gate nothing to report. The previous derivation still carried both, so a paired bring-up exercised code that will never ship - which is what the SEP side hit. This derivation is main (now carrying PMM-15238) plus #5762, #5759, #5653, #5739 and #5758. Recorded with -s ours so the branch moves forward without a force-push; the tree is the re-derivation. Signed-off-by: Yan Orestes <yan.orestes@percona.com>
This was referenced Aug 22, 2026
fabio-silva
pushed a commit
that referenced
this pull request
Aug 24, 2026
* 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-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> --------- Signed-off-by: Ignacio Durand <nachodurand@gmail.com> Signed-off-by: yyyyyyy <contact@yyyyyyyan.tech> Co-authored-by: yyyyyyy <contact@yyyyyyyan.tech>
fabio-silva
pushed a commit
that referenced
this pull request
Aug 24, 2026
* 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> --------- Signed-off-by: Ignacio Durand <nachodurand@gmail.com> Signed-off-by: yyyyyyy <contact@yyyyyyyan.tech> Co-authored-by: yyyyyyy <contact@yyyyyyyan.tech>
fabio-silva
added a commit
that referenced
this pull request
Aug 27, 2026
* 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-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-15358 Withhold SEP nav entries from anonymous The entries were offered to every session with a `user`, and anonymous access is one of those. It has no Grafana session cookie, so the SEP session exchange 401s and the entry opens on SepAuthGate's failure card rather than on the app. Signed-in was already the stated rule; this makes the predicate say so. Signed-off-by: Ignacio Durand <nachodurand@gmail.com> * PMM-15358 Merge consecutive canMutate checks in IncidentListPage Combine the two adjacent canMutate && blocks that gated the reopen/close icon and the rename/delete icons into a single fragment, per review feedback. Signed-off-by: Claude <noreply@anthropic.com> --------- Signed-off-by: Ignacio Durand <nachodurand@gmail.com> Signed-off-by: yyyyyyy <contact@yyyyyyyan.tech> Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: yyyyyyy <contact@yyyyyyyan.tech> Co-authored-by: Fábio Silva <ffjs1993@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
fabio-silva
added a commit
that referenced
this pull request
Aug 27, 2026
* 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>
fabio-silva
added a commit
that referenced
this pull request
Aug 27, 2026
* 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>
fabio-silva
added a commit
that referenced
this pull request
Aug 31, 2026
* PMM-15359 Report failed SEP UI actions in-tree (#5820) * 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-15384 Group the SEP apps under Management (#5840) * 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> * chore: fix vite config --------- Signed-off-by: Ignacio Durand <nachodurand@gmail.com> Signed-off-by: yyyyyyy <contact@yyyyyyyan.tech> Co-authored-by: Ignacio Durand <nachodurand@gmail.com> Co-authored-by: yyyyyyy <contact@yyyyyyyan.tech> Co-authored-by: pmm-prbot[bot] <298421014+pmm-prbot[bot]@users.noreply.github.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-15293
Feature Build: Percona-Lab/pmm-submodules#4522
What
Authenticate the embedded SEP UI as the actual PMM user by exchanging the PMM session for a short-lived SEP bearer, replacing the interim server-side token injection.
Why
ui/apps/pmm/src/sep/bootstrap.tsregisteredsetTokenProvider(() => null)and the dev proxy injectedPMM_DEV_SEP_INTERNAL_TOKENserver-side. That authenticates as SEP's internal service principal, which hardcodesis_admin = False, so every admin-gated SEP surface returns 403. Fine for verifying the UI migration; not shippable.SEP's side shipped in SEP-1692:
POST /sep/api/oauth/session/exchangeis same-origin through PMM's proxy, so the browser attachespmm_sessionautomatically. SEP validates it against Grafana, maps the org role, and returnsaccess_token+expires_in. No cookie is set and no refresh token is issued.Acceptance criteria
sepTokenStore.ts, single-flight in@sep/apisessionRejectedfailClosed()— every failure path drops the bearer firstphase: 'signedOut' | 'unreachable'ready; page never unmountsnotice; transient retried with backoffHow
@sep/api— token-minter seam (commits 1, 3)refreshAccessToken()hardcodedPOST /oauth/refreshas the only way to obtain a token. PMM's embedding 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 was already minter-agnostic: the single-flight coalescer, the axios 401 retry, thesetOnRefreshednotification.Two supporting changes:
/oauth/session*is excluded from the 401 retry branch. Minting is single-flighted, so routing a mint's own 401 back through the retry interceptor hands 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. There is a regression test that would time out if this guard were removed.useCurrentUserand every generated-path hook) could not recover at all.fetchconsumes a Request's body, so the middleware stashes a clone taken before dispatch and replays that; the replay goes through rawfetchso it cannot re-enter the middleware and loop. Only replay-eligible requests are cloned (thanks @copilot).PMM — session exchange (commit 2)
sepTokenStore.tsholds the bearer in memory only — nolocalStorage, nosessionStorage, no query cache. Renews 30s ahead of the 5-minute expiry; the transports' 401 retry is the backstop for a throttled background tab that misses the window. Concurrency is delegated torefreshAccessToken(), so a burst of parallel SEP requests triggers one exchange.SepAuthGatetriggers the first exchange when a SEP route mounts rather than at app startup. The UI has noPMM_ENABLE_SEPflag, so an eager exchange would hit SEP on every page load for every PMM user. It also closes a race the provider cannot:setTokenProvideris synchronous, so a plugin's first queries would otherwise fire before the exchange resolved. It sits insideSepPage's existing admin check, so the exchange only runs for a user allowed on the page./sep/api/oauth/*. OverwritingAuthorizationthere would authenticate the exchange as the service principal and mask whether the cookie path works at all.Fail closed, without discarding user work (commit 4)
These two rules pull in opposite directions, and reconciling them is most of this commit.
Fail closed. Every failure path runs through
failClosed(), which drops the bearer before doing anything else.getSepToken()independently returns null past the expiry, so nothing proceeds on a stale, expired, or unverified credential, and there is no cached value to fall back on. A rejected session sets a stickysessionRejectedthat refuses minting outright — the loop is cut before a request is made, not after it fails.Never destroy user work. The failure lands at one of two altitudes, chosen by whether a bearer has ever been held:
phase: 'signedOut'— takes over the pagenotice: 'signedOut'— inline, page untouchedphase: 'unreachable'— takes over the pagenotice: 'unreachable'At load there is no work in progress, so a full-page state is right. Once mounted, it is not: the previous revision moved the phase to
signedOuton a rejected renewal, which unmounted the plugin and threw away whatever was half-typed into it. The page now stays exactly as it is and the failure appears beside it, telling the user submissions will fail and offering a retry.Transient renewal failures back off at 2s, 4s, 8s, 16s and only surface if all four fail. A 401 skips the backoff entirely — the session is genuinely gone, so retrying would just repeat the rejection.
getSepAuthStatus()becamegetSepAuthState(), returning a cached{ phase, notice }snapshot souseSyncExternalStoredoes not re-render subscribers on a no-op.Testing
pnpm check-types,pnpm lint(0 errors),pnpm format:check,pnpm test— 1172 tests pass. 36 are new:packages/sep/api/tests/client.test.ts— minting through the registered minter, coalescing a burst of 401s into one exchange, the self-await regression guard, a minter resolving null, restoring the default.packages/sep/api/tests/typed-client.test.ts— mint-and-replay, replaying a request body (the clone is the crux), replaying at most once, one mint across concurrent 401s, no recovery attempt on a mint endpoint's own 401.apps/pmm/src/sep/sepTokenStore.test.ts— mocks onlypostSessionExchange, so the real@sep/apisingle-flight and unauthorized wiring are exercised. Covers acquisition, the synchronous provider, coalescing, no web-storage writes, snapshot stability, and one group per AC: failing closed (expiry, both renewal failure kinds, sticky refusal), bootstrap failure, and renewal on a mounted page (quiet backoff, surfacing only on persistence, terminal-401 immediacy, never leavingready).apps/pmm/src/sep/SepAuthGate.test.tsx— bootstrap paths, plus the non-destructive ones: a rejected session mid-session keeps the page mounted and preserves typed-in form state, and a successful retry clears the notice with the form still intact.Verified end to end (2026-08-10)
The happy path has now been exercised live, against a local SEP running the Grafana auth provider. Previously this section read "blocked by PMM-15280"; that blocker turned out to be about automatic provisioning, not about whether the mechanism works. Creating the Grafana service account by hand is enough to verify it:
POST /sep/api/oauth/session/exchangewith thepmm_sessioncookieaccess_token+expires_in: 300GET /api/users/mewith the minted bearerusername: admin,isAdmin: trueGET /api/sep/admin/settings/(admin-gated) with the same bearerSo the exchange maps the Grafana org role through to an admin-capable SEP identity, which is the whole point of this PR and was the part only tests covered.
PMM-15280 is still required — it provisions that service account automatically when
PMM_ENABLE_SEP=1, which is what makes this work in a shipped server rather than on a hand-configured dev box.Local setup needed to reproduce (SEP side)
The exchange is gated on the active auth provider supporting ambient sessions (
app/sep/deps.py:184-187), so a default dev SEP on the Casdoor provider returns 401 for every call no matter what the browser sends. To verify:AUTH.PROVIDERmust begrafana, notcasdoor— onlyGrafanaAuthProvidersetssupports_ambient_session. If a YAML profile block also declarescasdoor, set it tonullthere; the profile deep-merges overdefault.session_cookie_name: pmm_session— SEP defaults tografana_session, but PMM renames Grafana's login cookie (login_cookie_name = pmm_sessioninbuild/ansible/roles/grafana/files/grafana.ini). Wrong name, no cookie found, and the failure is indistinguishable from "not signed in".endpointmust point at Grafana under/graph(e.g.https://<pmm-host>/graph). PMM's root/api/...is pmm-managed, not Grafana.SEP__AMBIENT_SESSION_SSO_ENABLED=true— it defaults tofalse.The exchange denies with an identical 401 for every cause (no cookie, rejected session, unreachable provider) by design; the distinguishing detail only appears in SEP's log.
Known gap, by scope
The dev proxy still injects
PMM_DEV_SEP_INTERNAL_TOKENon non-OAuth paths, and having now verified the exchange live, the effect is worse than this section previously claimed.The injection is unconditional on every non-OAuth path —
proxyReq.setHeader('Authorization', ...)overwrites whatever the browser sent. It is not only a fallback when the store holds no bearer: it replaces a bearer the store does hold. Measured against a working exchange:GET /api/sep/admin/settings/pmm_sessionadmin,isAdmin: truePMM_DEV_SEP_INTERNAL_TOKENsep-service,isAdmin: falseSo with
PMM_DEV_SEP_INTERNAL_TOKENset, a dev environment sees every admin-gated SEP surface 403 even though this PR is working perfectly — the exact symptom the PR exists to remove, now caused by the leftover fallback rather than by the missing exchange. It presents as a permissions problem and sends you hunting through Grafana roles.Production is unaffected: no injection exists there. Until the follow-up lands, unset
PMM_DEV_SEP_INTERNAL_TOKENinui/apps/pmm/.env.localwhen working on any admin-gated SEP surface. This raises the priority of retiring the injection: it is no longer a fail-open subtlety but an active source of misleading 403s.Out of scope / follow-ups
SEP_INTERNAL_TOKENinjection from the proxy configuration entirely (closes the gap above).setTokenMinterseam back topercona/SEPso the next frontend sync does not clobber it.Related work
Depends on PMM-15216 Migrate the SEP UI into PMM #5653 (PMM-15216, [FE] Frontend SEP to PMM Integration — UI), which is this PR's base.
Which in turn depends on PMM-15288 Migrate UI toolchain to pnpm + oxlint/oxfmt #5728 (PMM-15288, UI toolchain migration to pnpm + oxlint/oxfmt).
Blocked for verification by PMM-15280.
SEP side: SEP-1692 (merged 2026-08-03).
API Docs updated