Provisioning: unblock user deletion, drop realm identity from manifests, skip unresolvable references, one error body - #232
Merged
Conversation
…whole apply A user in the recycle bin keeps IsDeleted=false — that is what reserves their email for a restore — so the exporter still lists them and every manifest written afterwards carries them along. UpdateUserHandler refuses to edit a pending-deletion user, and because one failed op rolls the whole apply transaction back, a single binned user blocked EVERY later apply with User.DeletionPending. Staged deletes go through that same path, so it also blocked deleting any OTHER user — the error named a lifecycle state the admin had never touched for the user they were actually trying to delete. The upsert now skips such an entry instead of failing: the lifecycle state is left alone, the way back is a restore. The plan carries a matching note on update entries so an edit that will be skipped is visible beforehand, added after the unchanged→update promotion so an untouched binned user does not read as a pending change for the whole retention window. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…entity
RealmManifest.Realm was dead weight that actively got in the way. The applier
never touched the realm shell — the planner even emitted a warning saying so
("the realm shell is not modified by apply") — yet the field forced every export
to name a realm, and forced whoever exported to decide, up front, where the file
would eventually land. Slug, routing domains and primary domain are deployment
identity; carrying them between environments is exactly what you do not want.
The shell is gone from the manifest. The target realm now comes from the route
alone, so one file applies unchanged to any realm, and the data-plane boundary
stops being a guard and becomes structural: a realm admin cannot aim a manifest
elsewhere because a manifest has no field to aim with. Manifest.SlugMismatch and
the draft service's PinSlug are gone with it. Older files that still carry a
"Realm" object stay loadable — the property is simply ignored.
POST /api/admin/realms/import is retired. It only ever wrapped two steps that
both already existed (create the realm, then apply), and it wrapped them badly:
its "all-or-nothing" was not a transaction but a compensating hard-delete, so a
typo in the manifest cost you the whole tenant database. Split, the apply runs
in its own transaction and rolls back cleanly, leaving the realm intact to retry
a corrected file against — a better failure model, not a worse one.
Modgud.Provisioning.TestKit keeps its one-call convenience: ImportRealmAsync now
takes the RealmSpec alongside the manifest and performs both calls, tearing the
realm down itself if the manifest step fails (tests want isolation; the API no
longer imposes it). Its error parser also learned the second error shape it now
meets — POST /api/admin/realms answers { error: <description> } while the
manifest endpoints answer { Error: <code>, Message: <description> }, and reading
the first as a code passed a description off as a machine-readable error.
BREAKING: RealmManifest.Realm removed; POST /api/admin/realms/import removed;
RealmManifestApplier.UpdateRealmAsync and RealmManifestPlanner.PlanAsync take the
slug as their first argument; TestKit ImportRealmAsync takes a RealmSpec. The
duplicate-slug error code on the create path is Realm.DuplicateSlug (the canonical
one) rather than the import wrapper's Realm.AlreadyExists.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…reported, not fatal
A manifest is content, and content travels. Applied to a realm that does not
have every app, role or user it names — a partial export, a hand-written file,
a realm whose apps moved on — the apply used to throw on the first unknown
reference and roll the whole thing back. That made the export UI compensate: it
locked dependencies in as "Required", though most of them are not. A client
without its app is an ordinary client, and the applier itself seeds its resolver
with every app already in the realm, so a file omitting an app the target has
resolved fine anyway. The strictness rested on an assumption — that a manifest
is always a complete closure — that the format never guaranteed.
References now resolve one by one. What resolves is applied, what does not is
skipped and recorded. Ten role references where eight exist means eight roles.
Two rules keep "skipped" from turning into "cleared":
- A reference list stays a REPLACE. Applying [A, B, C] to a group holding
[A, B, D] leaves [A, B]: C is skipped, D goes because the manifest did not
ask for it. Anything else would make removal via manifest impossible.
- If a non-empty list resolves to NOTHING, the field is left unchanged rather
than written empty. An empty list is an instruction ("clear this"); a failed
lookup is not one. Without this, exporting a role without its app would
silently strip an existing role of every permission on the target.
A role's app is not an ordinary reference and gets its own handling: the app
owns the catalog the role grants from, and the domain refuses a role that is
neither app-linked nor realm-admin. An existing role (matched by its exported
id) keeps the app and permissions it has; a NEW role naming an absent app cannot
exist at all, so the whole role is skipped rather than half-created.
Skips are reported at both ends, because the result alone cannot show them — a
group left with two of three roles looks exactly like a group that asked for
two. The apply returns RealmImportResult.SkippedReferences and logs a warning;
the plan predicts the same set as per-entry notes, so the review step in front
of every draft apply shows what will not land.
With nothing genuinely required any more, the selective-export modal stops
claiming otherwise: closure entries are pre-checked and labelled "referenced"
instead of locked and labelled "required". Keeping the lock while the server no
longer needs it would have been a lie about the server; keeping the label while
allowing it to be unchecked would have been a lie about the word.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ly does Driving the UI turned up a stale note: the plan told a create entry its role would be "created without permissions" when a missing app makes the applier skip the role outright. The two branches now say what happens — an existing role keeps its app and permissions, a new one is skipped whole. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
62aad38 removed Manifest.SlugMismatch, but this test still asserted it — it lives under Authorization/ and the verification run for that commit was filtered to ColdStart/, so it was never executed. The full suite caught it. It now pins the invariant that replaced the guard: a delegated management bearer cannot write into another realm because a manifest names no realm at all. A body still carrying the retired "Realm" object is accepted and IGNORED, and the export afterwards shows the write landed in the caller's own realm. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two error shapes were in play, both using a field named "error" and meaning
different things by it. The canonical renderer sent { "error": "<description>" };
the manifest endpoints sent { "Error": "<code>", "Message": "<description>" }.
Reading the first as if it were the second passes prose off as a machine-readable
code, which is exactly what Modgud.Provisioning.TestKit started doing once it
spoke to both.
Both halves of the canonical shape were wrong. It dropped Error.Code entirely, so
a client could only branch on prose — poor for any API, worse for an IdP. And it
put a description under `error`, the key OAuth reserves for a CODE, colliding with
the convention its own token endpoints follow. Everything now answers
{ "Error": "<code>", "Message": "<description>" }.
The frontend already expected this. useAssets, usePagesApi, usePageCompositionsApi
and the user grid all read body?.Message, so every error from a ToResult endpoint
has been silently degrading to "HTTP 400" in the UI. No frontend change was needed
to fix it.
Forbidden and Unauthorized are rendered here too instead of delegating to
Results.Forbid() / Results.Unauthorized(). Under this app's cookie auth, Forbid()
turns an /api/* response into an empty-body 403 and throws away the code — which
is why THREE endpoint files had each grown a private copy of this renderer
(RolesEndpoints, RealmsEndpoints, RealmConfigEndpoints, the first of which
documents the reason). With the shared one fixed, all three are gone. Status codes
are unchanged; only the body appears.
Deliberately untouched: OAuth/OIDC/DCR keep their RFC-mandated
{ error, error_description }, and the rate-limit filter keeps its documented
{ error: "rate_limited", … } — both use lowercase `error` for a CODE, which is the
convention our own endpoints were breaking, not following.
The unit tests for the mapper asserted result TYPES only, which is how the shape
drifted unnoticed; they now read the payload.
BREAKING: any consumer reading `error` from a non-OAuth endpoint gets `Error` +
`Message` instead. 401 and 403 now carry a body where they carried none.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| { | ||
| logger.LogWarning( | ||
| "Apply of realm {Slug} skipped {Count} unresolvable manifest reference(s): {Skips}", | ||
| slug, skips.Skips.Count, string.Join(" | ", skips.Skips)); |
| { | ||
| logger.LogInformation( | ||
| "Manifest apply skipped {Context}: the user has a pending deletion and is read-only. " | ||
| + "Restore the user (or let the retention purge finish) to make it writable again.", ctx); |
windischb
force-pushed
the
claude/user-deletion-pending-prod-fff596
branch
from
September 9, 2026 19:45
c41f07e to
23b5a22
Compare
The audit gate failed on GHSA-23fw-v26w-5fgq — Microsoft.Build.Tasks.Git 10.0.301, pulled in transitively by Microsoft.SourceLink.GitHub in the two packable projects. It has been latent on develop and only surfaced now: this is the first backend-touching PR since the advisory appeared, and the backend job is skipped for frontend-only changes. There is no fix to take — every version above 10.0.301 is an 11.0.100 preview or rc. And the exposure is build-time only: SourceLink is referenced with PrivateAssets=all and runs as an MSBuild task at pack time to embed git metadata, reaching neither the produced package, nor the running product, nor any consumer's dependency graph. So the exception is worth recording. Recording one was impossible with the old gate. The `dotnet list package` vulnerability report queries the advisory database directly and ignores <NuGetAuditSuppress>, so the only ways past it were to disable the step or to take a pre-release dependency. The gate is now NuGet's restore-time audit (NU1901-NU1904), which honours suppressions: an exception has to be written down in Directory.Build.props next to its justification, and everything else still fails the build. Audit settings are pinned there rather than inherited from SDK defaults, at mode 'all' (transitive included) and level 'low' (nothing filtered out by severity). Restore and audit are deliberately ONE step: a second `dotnet restore` is incremental and emits no warnings, so a separate audit step would have passed by doing nothing. The step pipes through tee rather than capturing into a variable. Under `bash -e`, `out=$(dotnet restore ...)` aborts the step before anything is echoed, so a restore that fails outright prints nothing at all and the log shows only an exit code — which is exactly how a malformed props file cost a diagnosis round here. Verified by removing the suppression and checking the exit code, not just grepping for a pattern: without it restore reports 2 NU1902 warnings naming GHSA-23fw-v26w-5fgq and the gate fails; with it, zero. The audit has teeth and the suppression is what silences it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
windischb
force-pushed
the
claude/user-deletion-pending-prod-fff596
branch
from
September 9, 2026 19:52
23b5a22 to
c96737a
Compare
windischb
added a commit
that referenced
this pull request
Sep 14, 2026
… save stages, prune asks first (#234) * docs(decisions): ADR 0024 — a manifest identifies by id, never by name The applier matches an entity by id and then falls back to its natural key — a group by Name, a client by ClientId, a role by app/name, a user by email. Applied to a realm holding an unrelated entity of the same name, that does not create: it updates the stranger, silently, and because reference lists are replace-semantics the victim can inherit another group's members and roles. The result is indistinguishable from an intended update. It never bites the ordinary path — ids are pinned at create, so a realm first filled from an export matches by id forever after. It bites exactly where two realms grew independently, which is what a partial export is for. No better string fixes it: a name, a key, a slug can each already stand for something else in the target, because the realms never shared an identity. Cross-realm identity either exists as a stable id or not at all. Decision: identity is the id, for entity matching and reference resolution alike, and no name is ever resolved against the realm. The authoring gap that the fallback covered — a hand-written file whose new entities reference each other — is closed with a document-local handle rather than invented ids, which people would copy from an example until they collided again. An Id beginning with a hash is not an id but a name for something inside this file; a hash can never be a ShortGuid, so no new property is needed and the two meanings cannot be confused. A real id resolves against the realm and is SKIPPED when absent; a handle resolves against the manifest and is an ERROR when absent; a bare name is an error, being a form that no longer exists. That split is the one already drawn elsewhere: a missing target is a fact about the target, a dangling handle is the file contradicting itself. Recorded as not yet implemented. Removing the fallbacks was measured, not estimated: 17 of 94 provisioning tests fail, every one with an "already exists" message — the intended loud error, no product defect among them. Display-name uniqueness is deliberately left undecided here; it asks what two entities may be CALLED, not which entity a manifest means. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(provisioning)!: a manifest identifies by id, never by name (ADR 0024) The applier matched an entity by id and then fell back to its natural key. Against a realm holding an unrelated entity of the same name that did not create — it updated the stranger, silently, and because reference lists are replace-semantics the victim could inherit another group's members and roles. Indistinguishable from an intended update. This implements ADR 0024. Entity matching is id-only, in all ten sections. An entry whose Id names a live entity IS that entity (and renames it where the key is mutable); an entry without one CREATES, and a taken name now fails loudly with that type's own error instead of overwriting. The authoring gap the fallback covered — a hand-written file whose new entities reference each other — is closed with a document-local handle rather than invented ids, which people would copy from an example until they collided again. An Id starting with '#' is a name for something inside this file: never stored, assigned a real id at create, resolved in the same run. A '#' can never begin a ShortGuid, so no new property was needed. References resolve three ways and no others: a real id against the REALM (absent = reported skip), a handle against the MANIFEST (absent = error), a bare name against nothing (error). A Key beside a real id becomes a verified hint — never followed, reported when it disagrees, so a stale name cannot mislead the next reader. Handle declarations, dangling handles and name-only references are all checked BEFORE the transaction opens: a document that contradicts itself should never reach one. The plan shows the same contradictions as error entries in a new "manifest" section, which is what gates the draft apply. App slugs, scope names, API audiences and resource:action keys deliberately stay names. They are the permission VOCABULARY — what tokens carry and what clients send on the wire — not entity identity, and an entity is required to have one, so the name already exists by the time a reference needs it. A '#handle' there is now refused (Manifest.HandleNotAllowed) rather than silently resolving to nothing, which would have been exactly the failure this decision exists to prevent. Prune now keeps by IDENTITY — the ids the upsert just touched — instead of by name. That also closes a hole: an entry that RENAMED an entity left the live one, still carrying its old name, looking prunable. The apply returns AssignedIds (handle -> id) so a hand-written file can be made idempotent after one run without an export. TestKit: authoring stays name-based — a test writes Members = ["alice"] — and the kit translates to handles on the wire. So a provisioned realm can never adopt an entity that merely shares a name, and AssignedIds is how a test learns an id it never chose. Frontend: the draft UI already staged { Key, Id } and the live entity's Id in all nine sections, but it DROPPED any reference it could not resolve when saving — opening and saving a group from an uploaded manifest would have deleted half its membership. Unresolved references now ride through untouched. Docs: five places still documented POST /realms/import and a required "Realm" object, both removed in #232 and never followed up — anyone following the published docs wrote a manifest section the server ignores and never created a realm. Verified: 94/94 provisioning tests, 812/813 full suite (the one failure, RegistrationPipelineTests.Reaper_erases_only_legacy_ghosts, is unrelated and reproduces standalone). BREAKING CHANGE: a manifest entry without a real Id creates instead of updating, so a partial export can add to a foreign realm but no longer update it. Group members, group roles and position grants must name an id or a '#handle' — a bare name is refused. '#' becomes a reserved prefix for entity ids and reference strings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(provisioning)!: settings that name entities by raw id stay in their realm A manifest carries a realm's ENTITIES. Four settings fields carried something else: a raw id pointing at an entity — self-registration DefaultGroupIds (realm and per-App), an App's LoginExperience.LoginProviderIds, and the branding LogoAssetId/FaviconAssetId. An id means nothing in another realm, so exporting one breaks a transfer in one of two ways. Asset ids and provider ids ARE validated against the realm, so the whole cross-realm apply fails on a reference the author never chose. Default group ids are not, so they would be stored dangling and in silence. Neither is worth transporting. They could not point at anything the same manifest creates either: settings apply first and per-App settings second, while groups and login providers come much later — so not even a handle would resolve there. The export now leaves all four behind. Omitted is UNCHANGED under v2 merge-patch, so re-applying an export into its own realm keeps the stored wiring exactly as it was — "does not travel" never means "gets cleared". A settings edit staged in the admin UI still applies to its own realm; it is the transport that drops them, not the contract. Deliberately not building the alternative — making these real references with handle support and moving the settings section after the entities it names — until the entity-level contract has proven itself in practice. Verified: 95/95 provisioning tests, with a new one pinning both halves (the ids are gone from the export, and a re-apply leaves the live wiring intact). BREAKING CHANGE: an export no longer carries SelfRegistration.DefaultGroupIds, LoginExperience.LoginProviderIds, or the branding asset ids. Set them in the realm that owns them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(provisioning): close the holes two independent reviews found in ADR 0024 Two fresh reviewers read the branch without its commit messages. Both independently found the same two silent-misresolution holes, and one found a data-loss bug in the settings change. Everything below is theirs; the tests are the parts that would have caught them. **Per-App settings were WIPED by re-applying an export.** A per-App settings section is REPLACE, not merge-patch — StageNonOriginAsync rebuilds a whole section from the DTO whenever the section is present. Stripping the realm-local ids out of a section that still carried other values therefore did not leave them alone, it cleared them. Export -> apply unchanged wiped an App's logo, its default groups and its login-provider allow-list; LoginProviderIds = null means "every enabled provider", so it silently WIDENED the App's authentication surface, with a plan that read "unchanged". The applier now re-attaches those three from the stored override before applying: the manifest cannot carry them, therefore the manifest never changes them. **A handle proved nothing about what it named.** A real id proves its type by loading the document; the handle path loaded nothing, so `"Members": ["#platform"]` naming a GROUP resolved to that group's id and was stored as a member. Validation now records which section declared each handle and refuses a kind mismatch. **A handle could resolve to nothing, in silence.** Validation checked declaration but never apply ORDER, so a group referencing a position handle (positions apply later) dropped the reference and returned 200 with an empty SkippedReferences. Order is now checked up front. The remaining case validation cannot see — the declaring entry was itself skipped, e.g. a role whose app this realm lacks — is now REPORTED instead of dropped. Also: - TestKit: pinning a real Id on a role or user broke every reference to it. The kit could only emit a bare string, which reads back as a name and is refused. Group references are a ManifestRef now, so a real id goes out as { Key, Id }. Authoring is unchanged — `Members = ["alice"]` still compiles. - The SERVED JSON schema was never updated. Its worked example used bare names and so failed 100% of the time, and its reference description still taught the deleted id-then-name fallback. That is the file the docs tell agents to author from. - Planner compared the natural key Ordinal where the applier compares OrdinalIgnoreCase, so a client `MyApp` vs `myapp` was reported as an apply error the applier would have accepted. - Admin UI: references the pickers cannot show are preserved on save (already), but invisibly — so clearing the picker looked like it did nothing. They are now named in a notice. - Three stale [Description] texts, `Manifest.SlugMismatch` (documented, never existed), a one-arg ImportRealmAsync snippet, and the ADR's "not yet implemented" status. Verified: 97/97 provisioning tests. The per-App settings test was counter-tested — disabling the fix turns it red — because a green test that passes with the bug present is worth nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(provisioning)!: every admin save goes through the draft again ADR-0005/0017 promise that every admin-UI save stages onto a draft and is reviewed as a plan before it lands. Nine views carved out exceptions from that. Audited, only ONE had a technical reason — terminal enrollment, where a device has to participate in the moment. The rest were modelling gaps, each locally reasoned, and nobody had looked at the sum. This closes the gaps. Group members may be any principal. The domain has always allowed nested groups and service accounts as members and really expands them (ApplicationScopeResolver for permissions, Group.GetEmailsAsync for mail). The manifest modelled members as users only, so the exporter filtered the rest out — and Members being a replace-list, that made export -> apply SILENTLY DELETE them. It is also why the admin UI forced such a group onto a live save: staging it would have stripped its members. Both are gone. User IsActive is declarative. It was live because "deactivation is an action with a revocation cascade" — but service accounts and positions already carry IsActive in the manifest and run the very same cascade, deferred until after the commit (DeferringUserAccessRevoker). The argument was contradicted twice over in the same file; users were simply missing the field. A permission catalog entry has an Id. Roles and resource servers hold it as a foreign key, but the manifest identified an entry by its resource:action string, so renaming one read as "the old is gone, a new appeared" and tripped the catalog-delete guard. That is the ADR 0024 problem one level down, and it is why a catalog rename fell back to an immediate live save. Entries carry their Id now; a rename is a rename and the grants follow. The planner fills the live id into a hand-written entry before diffing, so a catalog without ids does not read as a change the apply would not make. Found while testing the above, and fixed because it blocked it: THE GROUP CYCLE GUARD WAS INVERTED. It asked "is this member already one of my descendants?", which refuses a member the group already has — no group holding a nested group could be saved twice — and waves through the edge that actually closes a loop, since a not-yet-member is by definition not a descendant. It now asks whether the member can already reach the group. Both directions are pinned by a test. Not done, deliberately: service-account CREDENTIALS. The manifest models SA hulls only, by an explicit earlier decision, and modelling credentials is a new capability (a file that can mint M2M secrets) rather than a gap to close. The three infrastructure cases that remain — system app, standard scopes, built-in Internal provider — should become read-only in the UI rather than silently writing live; that is a UI change, not a contract one. justCreated is not an exception in its own right: it is only set after a live create that already bypassed staging for another reason, and keeping the follow-up edit live is then the consistent thing to do. Verified: 101/101 provisioning tests, with new ones for the nested member surviving a round trip, the active-state toggle (including idempotence), the catalog rename keeping its grant, and the cycle guard in both directions. BREAKING CHANGE: an export now carries every group member, not only the users, and every permission catalog entry carries its Id. RealmManifestUser gains IsActive — absent stays "unchanged", so existing manifests are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(provisioning)!: a pruning apply shows what it would delete, then asks The admin UI has always shown a plan before an apply, deletions in red, and refuses while the plan carries errors. The raw API had none of it: one POST …/apply?prune=true deleted everything the file did not mention, immediately, with nobody having looked at the list. A script could empty a realm on a typo. Now the first such call answers 409 with the plan, a confirmation token, and a review link. Repeating it with ?confirm=<token> goes ahead. Both surfaces — control plane and per-realm self-service. Only ?prune=true is gated, and that is not caution but arithmetic: without it the applier never reaches PruneAsync, so an ordinary apply can add and change but never remove an entity. Gating every apply would cost every caller a round trip against a danger that does not exist. A prune whose plan deletes nothing is likewise not gated. The token is stateless — a DataProtection payload, not a stored row — so it needs no cleanup and works across nodes (ADR 0022) without a shared table. It binds the realm, the manifest, the prune flag and THE DELETION SET THE CALLER WAS SHOWN, re-computed at confirm time: if the realm moved in between and something else would now be deleted, the token is stale and the caller has to look again. Plus a 15-minute expiry. So nobody can plan a harmless file and confirm a different one, or replay yesterday's token. The manifest is ALSO parked as a shared draft in the target realm, and the response carries its review URL. That is the other half of the idea: the script that posted it need not be the thing that decides. It can mail the link on — "please review this import" — and a human opens the change in the ordinary draft workspace, reads the plan with its deletions in red, and applies or discards. A parked draft carries PruneOnApply, so applying it prunes however it is applied; a reviewer who applied it without prune would silently do half of what was asked. Parking is best-effort: if it fails the caller still has the plan and the token, because losing the review link is worth less than losing the answer. This does not stop anyone confirming blindly, and nothing could — no more than the UI can stop someone clicking through without reading. It makes the information unavoidable, which is the part we control. Verified: 101/101 provisioning tests. The prune endpoint test now walks the whole flow — refused first call, the deletion visible in the plan it hands back, a token from a DIFFERENT manifest refused, then confirmed and pruned. BREAKING CHANGE: POST /api/admin/realms/{slug}/apply?prune=true and POST /api/admin/realm-config/apply?prune=true answer 409 Manifest.ConfirmationRequired on the first call. Repeat with ?confirm=<ConfirmationToken> from the response, or apply the parked draft the response links to. Applies without prune are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(provisioning)!: service-account credentials travel, their secrets do not A service account used to transfer as a HULL: name, id, purpose, active flag. Its credentials stayed behind, so an account applied to another realm arrived unable to authenticate — and nothing said so. The machine pointed at it failed at whatever hour it next ran. Credentials are now ordinary manifest entries, nested under the account that owns them rather than sitting in the Clients list. Nesting is not cosmetic: a credential has no meaning apart from its account, and the account has to exist before one can be bound to it — under the account, that ordering is true by construction instead of a rule to remember. Secrets still do not travel, and cannot. A credential created by an apply is minted a fresh secret and handed back once in ClientSecrets, exactly as an ordinary confidential client's is. There is deliberately no secret FIELD: a manifest gets committed, copied and mailed around, and the draft workspace — which strips the secrets it knows about — does not look inside this list. So the shape travels and the secret is issued per environment, which is what "credentials are per-environment" always meant in substance. Prune reaches them, and that is the point of the earlier confirmation gate: the plan shows a credential deletion in red and a pruning apply asks before it runs, so the admin decides instead of the code refusing on their behalf. One guard remains, because the plan cannot show what a file never mentions: a credential is only prunable when the manifest actually speaks for its ACCOUNT. Leave an account out and it keeps everything — otherwise forgetting to mention an account would quietly cut off whatever authenticates as it. The SA-scoped canonical ops do the writing (Issue/Update/DeleteServiceAccountCredential), not the ordinary client ones. That is not tidiness: /admin/oauth/clients REFUSES to mutate an SA-owned client, because a credential's lifecycle belongs to its account. The first cut of this went around that guard and the tests caught it twice — on update and again on prune. Also: the plan now says what a diff structurally cannot. A diff compares what both sides have; "this account arrives with no credentials" is not a difference but an absence, so it is stated outright on the entry, along with a note that each new credential's secret is returned once and never again. Verified: 102/102 provisioning tests, with a new one covering the whole life: created with a secret returned, exported under its account and without the secret, then a pruning apply that drops one credential and leaves an unmentioned account's alone. BREAKING CHANGE: an export now carries ServiceAccounts[].Credentials, and a pruning apply deletes credentials the file drops from an account it declares. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(admin): the user modal stages its active flag and direct groups too The commit that closed the draft carve-outs said "User IsActive is declarative" and put the field into the manifest and the applier — and left the user modal writing the checkbox live. So a realm admin with a draft open deactivated a user on the spot while everything else on the same form was staged; the backend half of the claim was true and the visible half was not. The modal now stages IsActive with the profile, reads it back from the staged entity on reopen, and shows the checkbox on staged creates and draft rows as well, since the field exists for them now. The Direct-groups tab was the other live write. A membership is a fact about the GROUP — Members is the group's list — so the diff is committed onto each affected group's draft entity: the one the draft already carries, else a minimal merge-patch of Id, Name and Members built from the live group. Everything else about the group stays absent, i.e. unchanged. On reopen the picker reads membership from the staged groups, so a change made here does not look undone and the next save diffs against the draft rather than the live roster. What still writes live from this modal, and says so in a comment: the per-user 2FA policy (grace override, exemption), which has no manifest field. Also: the draft review card printed "[object Object]" for a group's roles once role references became { Key, Id } objects — it shows the readable key now. Two doc comments still claimed SA-linked clients are never pruned; they are, when the manifest declares the account. Verified in the browser against the dev realm: toggling the flag and adding a group from the user modal produced exactly two draft writes (users, groups), no live PUT, a plan with IsActive true -> false and a Members change, and both reopened as staged. pnpm type-check exit 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(provisioning): the confirmation handshake, credentials in manifests, what still writes live Three shipped commits had no documentation: a script hitting apply?prune=true saw a 409 with no page explaining the token; ServiceAccounts[].Credentials appeared in exports that the docs still called hulls; and the drafts page listed the user's active flag and group membership as immediate operations after they had become staged saves. Provisioning gets a section on the two-step confirmation — the 409 body, what the token binds, its 15-minute life, the refusal codes, and the parked shared draft with its review link — and one on service accounts: credentials travel as shape, secrets are minted per environment and returned once, the account is never pruned and its credentials only when the manifest declares it. The prune protection list is corrected in every place it is repeated. The drafts page's "what stays immediate" table is reduced to what is actually immediate, each row with its reason, and names the per-user 2FA policy as the one remaining live write in the user modal instead of hiding it. The realm API reference points at the handshake; the service-account page gains a short "in manifests" section. Angle brackets in prose ("<time>, by <caller>") broke the VitePress build — Vue reads them as tags. Replaced with braces. Docs build exit 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(provisioning)!: the per-user 2FA policy and EmailConfirmed go through the draft The last two user fields the admin modal still wrote live with a draft open were the per-user 2FA policy — grace-period override and the exempt flag — and, less visibly, EmailConfirmed on an existing user: the modal staged it, and the apply ignored it with a plan note, so the checkbox was a silent no-op. Neither is an action. Both are configuration on the user, and they now travel in RealmManifestUser like the rest. GracePeriodDaysOverride is an Optional<int?>: absent = unchanged, an explicit null clears the override back to the realm default — the same clear the admin endpoint spells as -1, written the way every other manifest field spells it. TwoFactorExempt and EmailConfirmed are plain bool?. On create they feed the same CreateUserCommand fields the endpoint uses; on update they are written the way Admin_SetGracePolicy and V2_User_Update write them — plain document updates, no events, nothing stored when the value already matches, so a re-applied manifest does not churn the documents. The exporter reads the two policy fields off UserSecurityData and nothing else from that document: hashes, stamps and authenticator keys still never travel. The modal stages the policy inside the user entity and reads it back from the staged entity on reopen; the Security tab is shown on staged creates and draft rows, where only the policy section applies (there is no clock to reset yet). The two grace-clock buttons stay live — they act on a running clock, which is exactly the "real action" the draft rule exempts. Also closed while here: a user created in a draft could not be put into a group until the draft was applied, because nothing could refer to it. It now carries a document-local '#handle' as its Id (pinned once, so a rename in the draft keeps the references valid), and the Direct-groups tab stages memberships against that handle; the apply assigns the real id and resolves the groups. The Groups tab therefore no longer hides on staged creates. Verified: a new applier test walks create-with-policy, absent = unchanged, set, and explicit-null-clears, plus the export round trip; the provisioning, draft and manifest test classes pass. In the browser against the dev realm: setting a 30-day override and the exempt flag on an existing user produced one draft write and a plan with exactly those two field changes, no call to the grace endpoint; creating a user in the draft and adding it to a group produced Users[].Id "#draftie", "#draftie" in the group's Members, and a plan with no error — and the draft row reopened with the group shown. BREAKING CHANGE: an export carries Users[].TwoFactorExempt and, when set, Users[].GracePeriodDaysOverride; a manifest's EmailConfirmed on an existing user is applied instead of ignored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(provisioning)!: settings ids travel, credentials are a desired set, and the last live saves stage Two independent audits (one over every admin view, one over every admin endpoint against the manifest) found what still wrote live with a draft open, and one bug this very branch had introduced. This closes all of it except the surfaces that have no manifest section yet (scheduled jobs, inbox retention, terminal-slot config) and the PageBuilder, which gets its own treatment later. THE BUG. KeepRealmLocalSettingsAsync, added earlier on this branch to stop an import from wiping per-App settings, restored the stored LoginProviderIds, DefaultGroupIds and branding asset ids UNCONDITIONALLY on update. The App modal stages those fields in the same realm, so narrowing an App's login-provider allow-list through a draft showed in the plan and was thrown away by the apply — a security control that read as changed and was not. Gone, and replaced by the rule the owner chose: the ids travel like every other id (ADR 0024) — one the target has is applied, one it does not have is SKIPPED and reported, never fatal, never stored dangling. For the realm patch a skipped reference is dropped (= unchanged); for a per-App section, which is a replace, "unchanged" is spelled out as the stored value, and a list that resolves to nothing keeps the stored list rather than widening to null (= every provider) — on a new App it fails closed with []. The exporter carries the ids again, so a staged clear or replace of a logo or default group is visible in the plan and conflict-checked, which it was not while the export stripped them. CREDENTIALS ARE A DESIRED SET. A service account's Credentials list now behaves like Members on a group: a credential the account has but a present list does not is deleted at apply, no prune needed; an absent list leaves them alone. The plan shows each removal as a red delete entry under clients (which also makes a pruning apply ask first) instead of a field change the apply ignored without ?prune=true. WHAT NOW STAGES. Realm branding and email branding (they ARE Settings.Branding / EmailBranding; the view never knew the draft). The whole service-account modal — account, credential add/edit/remove — with a staged credential minted its secret at apply and returned once; only Rotate stays live, and the list gets the draft overlay it lacked. Position grant issue/revoke, on creates, draft rows and edits alike (suspend/resume flip a grant's own state and stay live); the grants and terminals tabs no longer disappear on draft rows; a create with terminal slots no longer pulls the whole position out of the draft — slots are enrolled after the apply, and the modal says so. The API modal's "create implicit scope" stages the scope and the API. An unlinked client_credentials client is an ordinary exported client and stages like one (the exclusion was broader than the exporter's). The login-provider grid toggle says "(immediate)" like the client's. PLAN HONESTY. A stricter position-security floor planned clean and rolled the whole apply back at the settings service's confirmation gate; the plan now states the consequences (positions, slots, ended staffing sessions) and the apply confirms on the plan's behalf, as the positions section already did. UpdateServiceAccountCredentialDto DisplayName is Optional (absent = unchanged, null clears) — the manifest promised it, the endpoint collapsed both to null. Five code comments that contradicted the code (v1 merge semantics, "credentials deliberately not modelled" eighty lines above the method that models them, "the applier never sends Settings", users matched by email) are corrected. Verified: the settings-reference tests rewritten for the new rule (same-realm change applied, foreign id skipped and reported, nothing-resolved keeps the stored list, [] still clears); the credential test extended (plan shows the removal as a clients delete, apply deletes without prune, an entry without a list leaves credentials alone); export, planner and draft classes green. In the browser: branding staged into the settings entity with no PATCH to realm-settings; a service account created in the draft with a generated client id, plan note about the fresh secret, no live POST, and the draft row in the list. Positions could not be exercised in the dev realm (feature off) — covered by type-check and the existing applier tests only. Docs updated (provisioning, drafts, service accounts); docs build green. BREAKING CHANGE: exports carry Settings.Branding.LogoAssetId/FaviconAssetId, Settings.SelfRegistration.DefaultGroupIds and the per-App id references again, and an apply skips unresolvable ones instead of failing; a present ServiceAccounts[].Credentials list deletes credentials it omits; PUT /api/service-account/{id}/credentials/{cid} treats DisplayName as merge-patch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(provisioning): scheduled jobs and the inbox retention policy are manifest sections The audits left two realm-configuration surfaces with no manifest section at all, so their admin views could only write live: a scheduled job's schedule, enabled flag and parameters, and the inbox retention policy. Both are now sections — exported, planned, applied, and staged from their views. Jobs[] configures by the job's compiled Key. That key is vocabulary, not an entity id: a manifest can only CONFIGURE a job the deployment has, never create or prune one, so a key this build does not know (or a deployment-wide system job, which is no realm's configuration) is skipped and reported, and the plan shows it as an error entry before the apply. CronOverride is an Optional — absent keeps the override, an explicit null clears it back to the default cron, exactly the endpoint's contract — and Parameters replace wholesale when present. The exporter emits only the realm's own jobs. InboxSettings is one singleton. Inside a section null is the VALUE "never", not "unchanged", so the section is the unit of replacement: a present section replaces the stored one, an absent section stays. The draft registry and the frontend store learned a second singleton for that (they had "Settings" hard-wired as the only one). The generic planner matched by id only (ADR 0024) and read every job entry as a create that collides with an existing key — an error on every row. A section whose natural key IS its identity now opts into key matching (MatchByKey); the plan then shows one update for the edited job and unchanged for the rest. The ScheduledJob modal stages a realm job's save (a system job on the control plane stays live, Run now is an action); the Inbox Settings view stages its save. Both read the staged state back on open. Verified: a new applier test configures a realm job through the manifest (unknown key skipped and reported, export carries the override, explicit null clears it) and applies the inbox policy section by section (absent section keeps its default); planner and draft classes green. In the browser: the inbox view staged a value with the plan reading 30 -> 9 and no PUT; the job modal staged Enabled false with no PUT to /api/admin/jobs, and after the MatchByKey fix the plan carries exactly that one update. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(provisioning)!: terminal slots travel as configuration, never as enrollment A position's terminal slots are now Positions[].Terminals in the manifest, the way a service account carries its credentials: display name, location, WebAuthn RP ID, device binding, the further positions the slot serves (by identity, owner always included) and the slot client's access profile (scopes, app slugs). A slot the position lacks is created with a fresh terminal-managed client through the same checks the terminal admin API runs (policy enabled, binding allowed and above the realm floor, served positions compatible) — a client-secret slot's secret comes back once in ClientSecrets; one whose Id names a live slot is updated (details, served set with the re-enrollment guard and the staffing-session cascade, access profile). What never travels is the enrollment: a slot from an apply is Pending until a device enrolls. RP ID and binding are immutable, a revoked slot's id is not revived, and a manifest never removes a slot — revoke is terminal and stays an action; the plan notes both a slot it will create and one it leaves alone. Export emits non-revoked slots under their owning position; served positions resolve as { Key, Id } or #handle (a slot may serve a position the same file declares further down — slots apply after every position). ManifestIdentity checks the refs and the scope/app vocabulary. Fixed on the way: the exporter and the prune guard recognised a terminal-managed client by its POSITION link, which only the legacy single-position form carries. A V2 slot client is linked to its enrollment, so it was exported under Clients (re-applying that export hit the terminal-managed guard) and a pruning apply tried to delete it through the generic client delete, which refuses. Both now test the enrollment link. Also: the access update passed null as the client display name, which the service reads as "clear it" — it passes the current name. Admin UI: the position modal stages new slots and served-position changes onto the draft on creates, draft rows and edits alike (the staged policy decides whether a slot may be added; the apply updates the position before creating its slots); disable / reactivate / revoke stay live. The realm-config card counts a position's staged slots. Docs: realm-provisioning (Terminals example + semantics), configuration-drafts (immediate table, staged surfaces, limits). Test: Terminal_slots_travel_as_ configuration_and_never_as_enrollment covers create with a forward handle, export round-trip as a no-op, update by id, the keep-unlisted rule, RP ID immutability and the policy gate. BREAKING (pre-1.0): Positions[] export shape gains Terminals; a realm with V2 terminal slots exports its slot clients under Positions instead of Clients. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(provisioning): an App's Origin route is a post-commit consequence of the apply The host->App routing map lives in the global database, so it can never join the tenant transaction. AppAdminService wrote it right after its own SaveChangesAsync, which inside a manifest apply is only a flush into the shared transaction, not a commit: a later section failing rolled the realm back while the new host stayed routed (or, for a pruned App, the old host was already unrouted). Inside a TenantApplyTransaction both the route write and the removal are now recorded via Defer and run after the whole apply committed, like token revocations and staffing-session endings; on rollback they are discarded. Outside an apply the behaviour is unchanged (validate up-front, route right after the atomic save). Test: App_settings_override_applies_routes_and_exports now moves the subdomain in a manifest that fails further down (taken slug), asserts the old route survives and the new one never appears, then moves it for real and prunes the App. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.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
2 participants
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.
Started as a production incident — deleting a user failed with
User.DeletionPending— and the fix uncovered three further problems in the same area.The incident
A user in the recycle bin keeps
IsDeleted=false(that is what reserves their email for a restore), so the exporter lists them and every manifest written afterwards carries them along.UpdateUserHandlerrefuses to edit a pending-deletion user, and because one failed op rolls the whole apply transaction back, a single binned user blocked every later apply — including the staged deletion of any other user. The error named a lifecycle state the admin had never touched for the user they were actually trying to delete.The upsert now skips such an entry instead of failing, and the plan says so beforehand.
A manifest carries content, never a realm identity
RealmManifest.Realmwas dead weight that got in the way: the applier never touched the realm shell — the planner even warned that it doesn't — yet the field forced every export to name a realm, and forced whoever exported to decide up front where the file would land. Slug, routing domains and primary domain are deployment identity.The target now comes from the route alone, so one file applies unchanged to any realm, and the data-plane boundary becomes structural: a realm admin cannot aim a manifest elsewhere because a manifest has no field to aim with.
POST /api/admin/realms/importis retired — it only wrapped two steps that already existed, and wrapped them badly (its "all-or-nothing" was a compensating hard-delete, so a typo cost the whole tenant database).Unresolvable references are skipped and reported, not fatal
A manifest travels, so its references routinely name things the target does not have. The apply now resolves what it can and skips the rest, reporting via
RealmImportResult.SkippedReferencesand matching plan notes. Two rules keep "skipped" from becoming "cleared": a reference list stays a replace, and a non-empty list that resolves to nothing leaves the stored value unchanged rather than writing[].With nothing genuinely required any more, the selective-export modal stops claiming otherwise — closure entries are pre-checked and labelled "referenced" instead of locked and labelled "required".
One error body
Two shapes were in play, both using a field named
errorand meaning different things by it. The canonical renderer droppedError.Codeentirely and put a description undererror— the key OAuth reserves for a code. Everything now answers{ "Error": "<code>", "Message": "<description>" }.The frontend already expected this:
useAssets,usePagesApi,usePageCompositionsApiand the user grid all readbody?.Message, so every error from aToResultendpoint has been silently degrading to "HTTP 400" in the UI. Fixed with no frontend change.Three endpoint files had each grown a private copy of the renderer to work around
Results.Forbid()emptying the body on/api/*; with the shared one fixed, all three are gone.OAuth/OIDC/DCR and the rate-limit filter are untouched — they use lowercase
errorfor a code, which is the convention our own endpoints were breaking.Breaking
RealmManifest.Realmremoved (older files stay loadable — the property is ignored);POST /api/admin/realms/importremoved;UpdateRealmAsync/PlanAsynctake the slug first; TestKitImportRealmAsynctakes aRealmSpec.Realm.DuplicateSlug(the canonical code) rather than the import wrapper'sRealm.AlreadyExists.Error+Messageinstead oferror; 401 and 403 now carry a body.Verification
813 API integration tests and 1667 unit tests green. Frontend type-check clean. The selective-export modal was driven in a browser against a live instance, and the apply/plan endpoints were exercised live — an apply with an unresolvable group role returns 200 with the skip reported, where it previously returned 400 and rolled everything back.
🤖 Generated with Claude Code