PMM-15294 Submit ServiceNow inputs to SEP settings - #5758
Conversation
`refreshAccessToken()` hardcoded `POST /oauth/refresh` as the only way to obtain a token. An embedded host that owns the session — PMM — has no refresh cookie, so every recovery attempt would 401 there. `setTokenMinter()` replaces just that call; the default is unchanged, so the standalone SPA behaves exactly as before. Everything downstream is minter-agnostic already: the single-flight coalescer, the axios 401 retry, and the `setOnRefreshed` notification. Two supporting changes: The 401 retry now skips `/oauth/session*` as well as `/oauth/refresh`. Minting is single-flighted, so routing a mint's own 401 back through the retry interceptor would hand it the very promise it is running inside — an await on itself that never settles. The unauthorized handler still fires for those endpoints: a rejected exchange means "not signed in" and the auth layer needs to hear it. The openapi-fetch transport gained the 401 retry the axios one already had; it previously only reported unauthorized, so typed hooks could not recover at all. `fetch` consumes a Request's body, so the middleware stashes a clone taken before dispatch and replays that. The replay goes through raw `fetch` so it cannot re-enter the middleware and loop. Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
The embedded SEP UI authenticated as SEP's internal service principal: the token provider returned null and the proxy injected PMM_DEV_SEP_INTERNAL_TOKEN server-side. That principal hardcodes `is_admin = False`, so every admin-gated SEP surface answered 403. It now authenticates as the actual PMM user. `sepTokenStore` exchanges the ambient `pmm_session` cookie for a short-lived SEP bearer via `POST /api/oauth/session/exchange` (SEP-1692) and holds it in memory only — no localStorage, no sessionStorage, no query cache. It renews 30s ahead of the 5-minute expiry, and the transports' 401 retry covers the case where a throttled background tab misses that window. Concurrency is delegated to `refreshAccessToken()`, so a burst of parallel SEP requests triggers one exchange. A 401 from the exchange itself is sticky: minting is refused until the user retries, so a rejected session cannot drive an exchange loop. `SepAuthGate` triggers the first exchange when a SEP route mounts rather than at app startup — the UI has no PMM_ENABLE_SEP flag, so an eager exchange would hit SEP on every page load for every PMM user. It also closes a race the provider cannot: `setTokenProvider` is synchronous, so a plugin's first queries would otherwise fire before the exchange resolved. The dev proxy no longer injects the internal token on `/api/oauth/*`. Overwriting Authorization there would authenticate the exchange as the service principal and mask whether the cookie path works at all. Retiring the injection entirely is a follow-up. Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
`onRequest` cloned every outbound Request so a 401 could be replayed, including the minting and login endpoints that `onResponse` explicitly excludes from the retry. Cloning buffers the body, and those clones were never going to be used. Both call sites now share one `isReplayEligible` predicate, so the clone and the retry cannot drift apart. Raised by Copilot on #5739. Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
Reworks how the store reports failure, against the updated ACs. Two
rules now shape it, and they pull in opposite directions.
Fail closed. Every exchange failure drops the bearer, so no request can
proceed on a stale, expired, or unverified credential, and there is no
cached value to fall back on. A session SEP has rejected stays sticky:
minting is refused outright until the user retries, so a rejection can
never drive an exchange loop.
Never destroy user work. The failure now lands at one of two altitudes.
Before a bearer has ever been held the page does not exist yet, so a
bootstrap failure takes the page over — there is nothing to preserve.
Once mounted the page stays mounted and the failure becomes an inline
notice beside it. Previously a background renewal being rejected moved
the phase to `signedOut`, which unmounted the plugin and threw away
whatever was half-typed into it.
The two are reconciled by keeping the bearer and the reporting separate:
`failClosed` always drops the credential, then chooses between a phase
change and a notice based on whether the page is up.
A renewal that fails for a reason that may not repeat is now retried
quietly with backoff — 2s, 4s, 8s, 16s — and only surfaces if all four
attempts fail. A 401 skips the backoff: the session is genuinely gone
and retrying would only repeat the rejection, so the user is told at
once, non-destructively, that submissions from this page will fail.
`getSepAuthStatus()` is replaced by `getSepAuthState()`, returning a
cached `{ phase, notice }` snapshot so `useSyncExternalStore` does not
re-render subscribers on a no-op. The old `error` phase is renamed
`unreachable`, matching the notice of the same name.
Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
The base moved the SEP side-car behind a single `/sep` mount point (SEP_BASE_PATH, PMM-15279), so the session exchange and everything the 401 retry matches on had to move with it. Conflicts were textual, not behavioural — `client.ts` and `index.ts` auto-merged with both the minter seam and SEP_BASE_PATH intact: - `typed-client.ts` / both test files: import-block collisions, kept both sides. - `bootstrap.ts`: the base edited the old doc comment that described the token exchange as future work. This branch is that work, so its text wins, with the base's `/sep/api/...` path correction applied. - `vite.config.ts`: the base collapsed five proxied prefixes into `/sep`, forwarded unstripped. Both comment blocks kept, and `isSepAuthPath` now matches `/sep/api/oauth/` — the prefix is still on the URL when the proxy sees it, so the old `/api/oauth/` test silently stopped matching and would have let the internal token cover the exchange again. - Tests: reworked my handlers onto the base's new `API` constant, which now carries the prefix. Neither transport's mint guard needed changing: axios matches on a URL relative to `baseURL`, and the typed client's `includes('/oauth/session')` is unaffected by a prefix. Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
The proxy forwarded `/sep` unstripped on the grounds that SEP serves the prefix itself via `root_path`. It does not: SEP carries no root_path support at all - no flag, no setting, no `FastAPI(root_path=...)`, and none on the shipped side-car's `python -m app.sep.main`. So both ways of running it locally answer 404 to everything the proxy forwards. `python -m app.main` serves at `/api/...`, and `uvicorn --root-path /sep` prepends root_path to the path, so it sees `/sep/sep/...` instead. PMM_DEV_SEP_STRIP_PREFIX=1 strips the prefix on the way out, which makes the uvicorn form work while keeping `url_for()` links prefixed. It stays off by default: the right default belongs to the server-side nginx location, which does not exist in this repo yet. The internal-token guard has to match both the prefixed and stripped forms. Vite applies `rewrite` by mutating `req.url` before the proxyReq handler runs, so with the strip enabled the old prefix-only test stopped matching and would have injected the service-principal token onto the OAuth routes it must never cover - masking whether the session exchange works at all. Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
Add a "ServiceNow connection" tab to PMM Settings so an admin can enter the receiver endpoint and the delivery plan's named secrets, and have PMM write them to SEP's settings API. The operator obtains the token out of band; PMM-15218 replaces this entry surface with a guided round trip and leaves the write path below untouched. The write is one whole-object PATCH of DIAGNOSTICS_DELIVERY_INPUTS. SEP seals the key's leaves, so a per-leaf write is not a shape the UI may improvise, and the submitted secret map must match the declared names exactly. Those names are read at runtime from the baked plan (SEPSettings -> DIAGNOSTICS_DELIVERY -> value.secrets) rather than hardcoded, so an image that renames one is followed rather than 422'd. Secrets are addressed by position, not by name: react-hook-form reads a field name as a path, and a declared name carrying a "." would register as a nested field, read back undefined, and silently overwrite a stored secret with an empty string. Stored secrets come back masked and are resubmitted verbatim so SEP restores them, except where no override exists to restore from - that case is sent empty, since a mask with nothing behind it is a 422. An empty secret is a valid save and reads as "not configured", never as an error. A rejected save leaves the previous configuration standing and reports the per-field 422 verbatim; 401, 403 and an unreachable SEP each get their own message, and a raw HTTP status is never shown. The tab sits behind SepAuthGate, so the settings calls carry the bearer minted from the PMM session (PMM-15293) rather than a cookie, which the admin-gated settings router refuses. Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds a new ServiceNow connection tab under PMM Settings that lets admins enter the SEP diagnostics delivery receiver endpoint and any delivery-plan-declared secret values, then persists them via SEP’s settings API using a single atomic PATCH of DIAGNOSTICS_DELIVERY_INPUTS.
Changes:
- Adds a new Settings tab/route (
/settings/servicenow-connection) and label text for “ServiceNow connection”. - Introduces a SEP-auth-gated ServiceNow connection form that loads declared secret names from SEP’s baked delivery plan, supports masked secret round-trips, and provides disconnect (reset) behavior.
- Adds unit and component tests for rendering, submission payload shape, validation, and error surfacing.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ui/apps/pmm/src/pages/settings/Settings.types.ts | Extends TabValue union to include the new ServiceNow tab route value. |
| ui/apps/pmm/src/pages/settings/Settings.tsx | Adds a new MUI Tab and renders ServiceNowConnectionTab when selected. |
| ui/apps/pmm/src/pages/settings/Settings.test.tsx | Verifies the ServiceNow tab activates for /settings/servicenow-connection. |
| ui/apps/pmm/src/pages/settings/Settings.messages.ts | Adds UI copy for the ServiceNow connection tab, status text, validation, and error messages. |
| ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionTab.tsx | Wraps the ServiceNow form in SepAuthGate so requests use the SEP bearer flow. |
| ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.tsx | Implements the direct-entry form, status banner, submit (PATCH), and disconnect (reset) flows. |
| ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.test.tsx | Component tests for field rendering, payload correctness, masks, validation, and disconnect confirmation. |
| ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.schema.ts | Zod schema for minimal client-side validation (absolute URL / empty allowed). |
| ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.ts | Utilities for extracting declared names/stored values, building PATCH payload, status derivation, and error mapping. |
| ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.test.ts | Unit tests covering secret-name resolution, patch builder rules, status transitions, and error mapping. |
| ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.types.ts | Types for SEP delivery inputs, stored inputs, and form values (positional secrets). |
| ui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.constants.ts | Constants for SEP settings class and key names (DIAGNOSTICS_DELIVERY, DIAGNOSTICS_DELIVERY_INPUTS). |
| ui/apps/pmm/src/pages/settings/components/servicenow/index.ts | Re-exports the ServiceNow tab/form components for Settings integration. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`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>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
WalkthroughThis change adds a ServiceNow settings tab and authenticated connection form. It defines SEP data handling, validation, status and error mapping, save and disconnect actions, localized messages, and comprehensive tests. ChangesServiceNow settings integration
Sequence Diagram(s)sequenceDiagram
participant Settings
participant ServiceNowConnectionTab
participant SepAuthGate
participant ServiceNowConnectionForm
participant SEPSettingsAPI
Settings->>ServiceNowConnectionTab: select servicenow-connection route
ServiceNowConnectionTab->>SepAuthGate: render authenticated tab
SepAuthGate->>ServiceNowConnectionForm: render form
ServiceNowConnectionForm->>SEPSettingsAPI: list SEP settings
SEPSettingsAPI-->>ServiceNowConnectionForm: return delivery inputs and plan
ServiceNowConnectionForm->>SEPSettingsAPI: patch delivery inputs on save
SEPSettingsAPI-->>ServiceNowConnectionForm: return save result
ServiceNowConnectionForm->>SEPSettingsAPI: reset delivery inputs on disconnect
SEPSettingsAPI-->>ServiceNowConnectionForm: return reset result
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@ui/apps/pmm/src/pages/settings/Settings.messages.ts`:
- Line 113: Move the support URL from the subscriptionLink definition into a
shared constant in constants.ts, then update subscriptionLink to reference that
constant instead of embedding the URL string. Export and import the constant
using the existing project conventions.
🪄 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: 8dd2b122-6d92-4f6a-990c-705a8e555448
📒 Files selected for processing (13)
ui/apps/pmm/src/pages/settings/Settings.messages.tsui/apps/pmm/src/pages/settings/Settings.test.tsxui/apps/pmm/src/pages/settings/Settings.tsxui/apps/pmm/src/pages/settings/Settings.types.tsui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.constants.tsui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.types.tsui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.test.tsui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnection.utils.tsui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.schema.tsui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.test.tsxui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionForm.tsxui/apps/pmm/src/pages/settings/components/servicenow/ServiceNowConnectionTab.tsxui/apps/pmm/src/pages/settings/components/servicenow/index.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual)
The previous commit's comment claimed SEP carries no root_path support at all. That was true when it was written and stopped being true a day later: SEP-1794 (percona/SEP#1325) added a `SEP.ROOT_PATH` setting, passed to the `FastAPI(root_path=...)` constructor, so a SEP started with `SEP__ROOT_PATH=/sep` serves the prefix and the proxy forwards it untouched. Verified against a local SEP carrying the change: with ROOT_PATH set and nothing stripped, `/sep/api/oauth/session/exchange`, `/sep/api/sep/admin/settings/`, `/sep/api/apps/atw/config/` and `/sep/api/users/me` all resolve. Every one of them was a 404 before. Keep the flag: it still covers a SEP that predates the change or runs with ROOT_PATH unset. Reframe it as the fallback it now is, and warn against pairing it with uvicorn's `--root-path`, which prepends the prefix rather than declaring the mount - the two cancel out by accident rather than by design. Comment only; no behaviour change. Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
…ssion-exchange # Conflicts: # ui/packages/sep/api/src/typed-client.ts
Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
The merge of the base branch resolved typed-client.ts in favour of this branch, which reinstated isHtmlLoginResponse and the 303 clause that PMM-15216 had deleted with the SEP-1687 port. The Jinja login route that could answer an API call with a 200 HTML body is gone, so content-type sniffing can no longer mean "session expired" — under PMM it would only fire on a proxy misconfiguration and report that as a lost session. The axios transport in client.ts already took the deletion, and the tests covering the removed behaviour are gone, so this restores parity between the two transports. The token mint-and-replay path this branch adds is untouched. Signed-off-by: yyyyyyy <contact@yyyyyyyan.tech>
…into PMM-15294-sep-diagnostics-settings Signed-off-by: yyyyyyy <contact@yyyyyyyan.tech>
Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
The 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>
#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>
| @@ -1 +1,3 @@ | |||
| export const MAX_LABEL_WIDTH = 750; | |||
|
|
|||
| export const PERCONA_SUPPORT_URL = 'https://www.percona.com/services/support'; | |||
There was a problem hiding this comment.
@catalinaadam can we provide a shortened url for this?
Ticket number: PMM-15294
Feature Build: Percona-Lab/pmm-submodules#4523
What
A ServiceNow connection tab in PMM Settings where an admin enters the receiver endpoint and the delivery plan's named secrets, and PMM writes them to SEP's settings API.
This is the direct-entry path: the operator obtains a ServiceNow token out of band and types it in. PMM-15218 replaces this entry surface with a guided ServiceNow round trip (currently blocked on the ServiceNow side) — it replaces the surface, not the write path below, which is why the two are separated the way they are.
Why
SEP bakes the delivery-plan skeleton into its embedded profile and exposes only the per-deployment inputs — the receiver endpoint and the plan's named secrets — as one structured, overridable settings key. Without a UI for it, diagnostics delivery can only be configured by hand-editing SEP's configuration. See SEP-1698.
The contract this codes against
Shipped and verified against SEP
main; not a proposal. The whole design of the diff follows from four properties of it:not_overridableDIAGNOSTICS_DELIVERY_INPUTS. There is no partial-write path anywhere.SEPSettings→DIAGNOSTICS_DELIVERY→value.secrets), never hardcoded. One field is rendered per declared name, so an image that renames one is followed rather than 422'd.""before sending, because a mask with nothing behind it is a 422.Requests go through
usePatchSetting()/useSettingsList()/useResetSetting()inui/packages/sep/api/src/hooks/useSettings.ts, which already ship the single-key atomic body.How
Where it lives
Settingsgains a fourth tab, matching the design at Figma (although, the functionality is not implemented the same way as in Figma, since the integration is blocked by ServiceNow at the moment). PMM-15218 can then swap the tab body from this form to the OAuth button without moving anything.The tab wraps its content in
SepAuthGate, so the calls carry the bearer minted from the PMM session (#5739) rather than a cookie — SEP's settings router is admin-gated on reads as well as writes, and refuses a cookie-only mutation with a 401 before it validates anything.Secrets are addressed by position, not by name
ServiceNowFormValues.secretsis astring[]aligned with the declared names, and the names are zipped back on only when building the PATCH.react-hook-form reads a field name as a path. A declared name carrying a
.— and these names are runtime data from SEP — would register as a nested field, read backundefined, and silently submit""over a stored secret. Positional addressing removes that class entirely. Covered by a unit test (sn.api.key,client[token]) and a rendering test that types into a dotted field and asserts the payload key survives.Failure reporting
Validation is all-or-nothing server-side, so nothing here is optimistic: a rejected save refetches nothing and the previous configuration stands, with the typed-in values still on screen.
The per-field 422 message is surfaced verbatim — it names the offending secret keys, which is more useful than anything the UI could synthesise. Below that, 401, 403 and an unreachable SEP each get their own message, and a raw HTTP status is never shown. The 403 case matters: the ACs call out never leaving the user with an unexplained 403.
Status
Derived from the stored inputs rather than a second round trip:
configured,not-configured(nothing saved, or a declared secret saved empty), ordrifted(the plan declares a name the stored inputs lack — an image renamed one after the values were supplied).Two states short-circuit the form: a deployment whose settings list carries no
DIAGNOSTICS_DELIVERY_INPUTSat all says so instead of offering a form whose every write would 422, and a plan declaring no secrets still offers the endpoint, since the two are independent.Testing
tsc --noEmit,oxlint(0 errors),oxfmt --check,vite build, andvitest run— 434 tests pass, 48 of them new.ServiceNowConnection.utils.test.ts— declared-name resolution and its fallback, the mask/empty/omitted-endpoint rules in the patch builder, exact-name submission, every status transition, and the error mapping.ServiceNowConnectionForm.test.tsx— one field per declared name, a rename followed across a reload, the single atomic PATCH, masks preserved on an untouched resubmit, clearing a secret as an explicit unconfigured save, the 422 surfaced, the form intact after a rejected save, and disconnect behind its confirmation.Verified against a live SEP
Not only unit tests: run against a local SEP on the Grafana auth provider, with the bearer minted from the PMM session by #5739. The exchange returns an admin-capable identity (
isAdmin: true) and the admin-gated settings list returns 200, so the tab loads and writes as a real PMM admin.Out of scope
Related work
Depends on PMM-15293 Mint the SEP bearer from the PMM session #5739 (PMM-15293, SEP session exchange), which is this PR's base — the admin-gated write is unreachable without it.
Which in turn depends on PMM-15216 Migrate the SEP UI into PMM #5653 (PMM-15216) and PMM-15288 Migrate UI toolchain to pnpm + oxlint/oxfmt #5728 (PMM-15288).
SEP side: SEP-1698.
API Docs updated