From d74c4e386120a8c5f69f5523927abe3a36553a4a Mon Sep 17 00:00:00 2001 From: Bernhard Windisch Date: Fri, 18 Sep 2026 18:57:12 +0200 Subject: [PATCH] fix(provisioning): a Management API consumer's first run is one file, one apply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported from the field (amZettel, 2026-09-18) with the standard first manifest of a Management API consumer: a role on the system app, a service account with a credential, and a group binding the two — handles throughout. Three things stood in the way, and none of them was the apply: 1. Service accounts applied AFTER groups, so "#mgmt-sa" in a group's Members was refused up front as Manifest.HandleAppliedTooLate. A credential only references apps and scopes, both applied earlier, so the account moves before groups — like users. The plan already listed the sections in that order; applier and order check now agree. 2. The planner canonicalized and resolved group Members against USERS only. The exporter writes a service-account member as { Key, Id } like any other, so re-planning an export announced "Member 'x' — this realm has no such entity" on an unchanged group, and the id-only spelling of the same member read as an update. Members now resolve against users, nested groups and service accounts — the three kinds the applier's member resolver has accepted since the always-staged wave. 3. The planner took its app catalog off the EXPORT, which leaves the system app out on purpose (seeded, not authored). A role on "modgud" — where oauth-authorization:* and the other Management API permissions live — was therefore announced as "the apply SKIPS this role entirely. Import the app first", while the apply created it fine: its app resolver is seeded from the live query. The plan now reads the realm's apps live, as the apply does; the same fix covers a client, API or scope pointing at the system app. Test: A_management_api_consumer_provisions_role_service_account_and_group_in_one_run — plan without a manifest error or a phantom note, apply lands role + account + group + membership in one run, export re-plans as unchanged and note-free, and the id-only member spelling is the same membership. The field report is the red run on the released build. Docs: the reference paragraph in realm-provisioning names all three member kinds and the apply order; the Members field description and the identity check's comment no longer claim service accounts apply after groups. Co-Authored-By: Claude Fable 5.1 --- docs/admin/realm-provisioning.md | 17 ++- .../ColdStart/RealmManifestApplierTests.cs | 104 ++++++++++++++++++ .../Admin/Provisioning/ManifestIdentity.cs | 11 +- .../Admin/Provisioning/RealmManifest.cs | 2 +- .../Provisioning/RealmManifestApplier.cs | 9 +- .../Provisioning/RealmManifestPlanner.cs | 49 +++++++-- 6 files changed, 169 insertions(+), 23 deletions(-) diff --git a/docs/admin/realm-provisioning.md b/docs/admin/realm-provisioning.md index 4af0d820..b165abae 100644 --- a/docs/admin/realm-provisioning.md +++ b/docs/admin/realm-provisioning.md @@ -100,12 +100,17 @@ happens to share it. Two forms of `Id`, and nothing else: **Cross-references between entities follow the same rule:** -- Groups list **`Members`** (users) and **`Roles`**; positions list **`Grants`** (users). - Each entry is a real id (`{ "Key": "acme/Author", "Id": "…" }`, where the `Key` is a - *verified hint* — never followed, and reported when it disagrees), or a `"#handle"`. - A **bare name is an error**: the "acme/Author" in the target realm need not be the one - the file was written against. Exports write the object form. Group membership is the - *only* way users get roles. +- Groups list **`Members`** (users, nested groups, service accounts) and **`Roles`**; + positions list **`Grants`** (users). Each entry is a real id (`{ "Key": "acme/Author", + "Id": "…" }`, where the `Key` is a *verified hint* — never followed, and reported when it + disagrees), or a `"#handle"`. A **bare name is an error**: the "acme/Author" in the + target realm need not be the one the file was written against. Exports write the object + form. Group membership is the *only* way users get roles. +- Sections apply in dependency order — apps, APIs, scopes, clients, login providers, + roles, users, **service accounts, then groups**, positions — so a group may name a user + *or a service account* this same file creates by `#handle`. "A machine identity, a + role and a group binding the two" is one file, one run. A role may sit on the system + app (`"App": "modgud"`) — that is where the Management API permissions live. - A real id the target realm does not have is **skipped and reported**; a `#handle` the file never declares is an **error**. A missing target is a fact about the target; a dangling handle is the file contradicting itself. diff --git a/src/dotnet/Modgud.Api.Tests/ColdStart/RealmManifestApplierTests.cs b/src/dotnet/Modgud.Api.Tests/ColdStart/RealmManifestApplierTests.cs index 207c9fd0..0c9e9428 100644 --- a/src/dotnet/Modgud.Api.Tests/ColdStart/RealmManifestApplierTests.cs +++ b/src/dotnet/Modgud.Api.Tests/ColdStart/RealmManifestApplierTests.cs @@ -1513,6 +1513,110 @@ await InTenantAsync(factory, slug, async sp => }); } + /// + /// The first run of a Management API consumer, reported from the field: a role on + /// the SYSTEM app (that is where oauth-authorization:* lives), a service + /// account, and a group binding the two — one file, handles throughout. Three things + /// used to stand in the way, none of them in the apply itself: service accounts applied + /// AFTER groups, so "#mgmt-sa" in Members was "applied too late"; the planner + /// resolved members against users only, so a service-account member — written by the + /// export in its own { Key, Id } form — read as "no such entity"; and the + /// planner took its app catalog off the export, which omits the system app, so a role + /// on it was announced as skipped while the apply created it fine. + /// + [Fact] + public async Task A_management_api_consumer_provisions_role_service_account_and_group_in_one_run() + { + await using var host = await Fixture.CreateIsolatedHostAsync(); + var factory = host.Factory; + var ct = TestContext.Current.CancellationToken; + var applier = factory.Services.GetRequiredService(); + var planner = factory.Services.GetRequiredService(); + var exporter = factory.Services.GetRequiredService(); + + const string slug = "consumer"; + Assert.False((await ProvisionRealmAsync(factory, Shell(slug), new RealmManifest(), ct)).IsError); + + var manifest = new RealmManifest + { + Roles = + [ + new RealmManifestRole + { + Id = "#mgmt-role", Name = "consumer-management", App = AppSlugs.Modgud, + Permissions = [new("oauth-authorization", "read"), new("oauth-authorization", "revoke")], + }, + ], + ServiceAccounts = + [ + new RealmManifestServiceAccount + { + Id = "#mgmt-sa", AccountName = "consumer-management", + Credentials = [new RealmManifestServiceAccountCredential { ClientId = "consumer-management" }], + }, + ], + Groups = [new RealmManifestGroup { Name = "consumer-management", Members = ["#mgmt-sa"], Roles = ["#mgmt-role"] }], + }; + + // ── Plan: no contradiction, no phantom skip — the plan sees the system app and + // the account exactly as the apply will. ───────────────────────────────────── + var plan = await planner.PlanAsync(slug, manifest, prune: false, ct: ct); + Assert.False(plan.IsError, plan.IsError ? plan.FirstError.Description : string.Empty); + Assert.DoesNotContain(plan.Value.Sections, s => s.Name == "manifest"); + var roleEntry = Assert.Single(plan.Value.Sections.Single(s => s.Name == "roles").Entries); + Assert.Equal("create", roleEntry.Action); + Assert.DoesNotContain(roleEntry.Notes, n => n.Contains("does not exist here")); + var groupEntry = Assert.Single(plan.Value.Sections.Single(s => s.Name == "groups").Entries); + Assert.Equal("create", groupEntry.Action); + Assert.DoesNotContain(groupEntry.Notes, n => n.Contains("no such entity")); + + // ── Apply: everything lands in one run. ──────────────────────────────────────── + var applied = await applier.UpdateRealmAsync(slug, manifest, ct: ct); + Assert.False(applied.IsError, applied.IsError ? applied.FirstError.Description : string.Empty); + Assert.True(applied.Value.ClientSecrets.ContainsKey("consumer-management")); + Assert.Empty(applied.Value.SkippedReferences); + Guid saId = default; + await InTenantAsync(factory, slug, async sp => + { + var session = sp.GetRequiredService(); + var sa = await session.Query().SingleAsync(s => !s.IsDeleted && s.AccountName == "consumer-management", ct); + saId = sa.Id; + var group = await session.Query().SingleAsync(g => !g.IsDeleted && g.Name == "consumer-management", ct); + Assert.Contains(sa.Id, group.MemberIds); + var role = await session.Query().SingleAsync(r => !r.IsDeleted && r.Name == "consumer-management", ct); + Assert.Contains(role.Id, group.RoleIds); + var system = await session.Query().SingleAsync(a => !a.IsDeleted && a.Slug == AppSlugs.Modgud, ct); + Assert.Equal(system.Id, role.AppId); + Assert.Equal(2, role.PermissionIds.Count); + }); + + // ── Export → plan: the account is a member in the export's own form and reads + // as unchanged, without a note; so does the role on the system app. ───────── + var exported = await exporter.ExportRealmAsync(slug, ct); + Assert.False(exported.IsError); + var exGroup = Assert.Single(exported.Value.Groups, g => g.Name == "consumer-management"); + Assert.Equal(["consumer-management"], exGroup.Members!.Select(m => m.Key)); + var again = await planner.PlanAsync(slug, exported.Value, prune: false, ct: ct); + Assert.False(again.IsError); + var againGroup = Assert.Single(again.Value.Sections.Single(s => s.Name == "groups").Entries, e => e.Key == "consumer-management"); + Assert.Equal("unchanged", againGroup.Action); + Assert.Empty(againGroup.Notes); + var againRole = Assert.Single(again.Value.Sections.Single(s => s.Name == "roles").Entries, e => e.Key == $"{AppSlugs.Modgud}/consumer-management"); + Assert.Equal("unchanged", againRole.Action); + Assert.Empty(againRole.Notes); + + // The id-only spelling of the same member is the same membership, not a change. + var idOnly = exported.Value with + { + Groups = [exGroup with { Members = [new ManifestRef { Id = new ShortGuid(saId).ToString() }] }], + }; + var idPlan = await planner.PlanAsync(slug, idOnly, prune: false, ct: ct); + Assert.False(idPlan.IsError); + var idGroup = Assert.Single(idPlan.Value.Sections.Single(s => s.Name == "groups").Entries, e => e.Key == "consumer-management"); + Assert.Equal("unchanged", idGroup.Action); + Assert.Empty(idGroup.Notes); + } + /// /// A `#handle` proves nothing about what it names — unlike a real id, which proves its /// type by loading the document. Three ways that can go wrong, all of which used to diff --git a/src/dotnet/Modgud.Api/Features/Admin/Provisioning/ManifestIdentity.cs b/src/dotnet/Modgud.Api/Features/Admin/Provisioning/ManifestIdentity.cs index 710c8eb1..92d23d1f 100644 --- a/src/dotnet/Modgud.Api/Features/Admin/Provisioning/ManifestIdentity.cs +++ b/src/dotnet/Modgud.Api/Features/Admin/Provisioning/ManifestIdentity.cs @@ -162,9 +162,10 @@ void CheckRef(ManifestRef? reference, string what, string wantedSection, string foreach (var g in manifest.Groups) { - // A member may be a user, a nested group, or a service account. Service - // accounts apply AFTER groups, so one of those has to be named by its real id - // — the order check says so up front rather than dropping it at apply time. + // A member may be a user, a nested group, or a service account. Users and + // service accounts both apply BEFORE groups, so either may be a '#handle' this + // file declares; a nested group's handle resolves only when that group comes + // earlier in the list (same section — the order check lets it through). foreach (var m in g.Members ?? []) CheckRef(m, "user, group or service account", $"{Sections.Users}|{Sections.Groups}|{Sections.ServiceAccounts}", @@ -246,8 +247,8 @@ void CheckVocabulary(string? value, string what, string ctx) Sections.LoginProviders => 5, Sections.Roles => 6, Sections.Users => 7, - Sections.Groups => 8, - Sections.ServiceAccounts => 9, + Sections.ServiceAccounts => 8, + Sections.Groups => 9, Sections.Positions => 10, _ => int.MaxValue, }; diff --git a/src/dotnet/Modgud.Api/Features/Admin/Provisioning/RealmManifest.cs b/src/dotnet/Modgud.Api/Features/Admin/Provisioning/RealmManifest.cs index 466944ad..1a6bfe19 100644 --- a/src/dotnet/Modgud.Api/Features/Admin/Provisioning/RealmManifest.cs +++ b/src/dotnet/Modgud.Api/Features/Admin/Provisioning/RealmManifest.cs @@ -482,7 +482,7 @@ public sealed record RealmManifestGroup [Description("Optional description. Absent = unchanged; explicit null clears.")] public Optional Description { get; init; } - [Description("Members for MembershipMode=Manual — a user, a NESTED GROUP, or a service account. Each entry names one by IDENTITY (ADR 0024): { \"Key\": \"alice\", \"Id\": \"\" } for one that exists here (missing = reported skip), or \"#alice\" for one this same manifest creates (undeclared = error). A bare name is an error. A service-account member must use a real id — service accounts apply after groups, so a handle would not exist yet. Absent = unchanged; [] clears the member list.")] + [Description("Members for MembershipMode=Manual — a user, a NESTED GROUP, or a service account. Each entry names one by IDENTITY (ADR 0024): { \"Key\": \"alice\", \"Id\": \"\" } for one that exists here (missing = reported skip), or \"#alice\" for one this same manifest creates (undeclared = error). A bare name is an error. Users and service accounts apply before groups, so either may be a handle; a nested group's handle resolves only when that group is listed earlier. Absent = unchanged; [] clears the member list.")] public List? Members { get; init; } [Description("Roles this group grants to its members. Each entry names a role by IDENTITY (ADR 0024): { \"Key\": \"acme/Author\", \"Id\": \"\" } for a role that exists here (missing = reported skip), or \"#author\" for a role this same manifest creates (undeclared = error). A bare name is an error. Absent = unchanged; [] clears.")] diff --git a/src/dotnet/Modgud.Api/Features/Admin/Provisioning/RealmManifestApplier.cs b/src/dotnet/Modgud.Api/Features/Admin/Provisioning/RealmManifestApplier.cs index 092c4f83..9551fd27 100644 --- a/src/dotnet/Modgud.Api/Features/Admin/Provisioning/RealmManifestApplier.cs +++ b/src/dotnet/Modgud.Api/Features/Admin/Provisioning/RealmManifestApplier.cs @@ -644,6 +644,12 @@ await SetUserTwoFactorPolicyAsync(session, existing.Id, } } + // ── Service accounts (hulls, id-pinned creates) — BEFORE groups, like users: a + // group's Members may name an account this same file creates by '#handle', and + // "M2M identity + role + group" is the standard first run of every Management + // API consumer. Credentials only reference apps and scopes, both applied above. + await ApplyServiceAccountsAsync(sp, manifest, identity, apps, secrets, skips, ct); + // ── Groups (identity = Id; the Name is mutable) ─────────────────────────── if (manifest.Groups.Count > 0) { @@ -741,9 +747,6 @@ await SetUserTwoFactorPolicyAsync(session, existing.Id, } } - // ── Service accounts (hulls, id-pinned creates) ─────────────────────────── - await ApplyServiceAccountsAsync(sp, manifest, identity, apps, secrets, skips, ct); - // ── Positions (MG-FT) — after users so grants can resolve their handles ─── await ApplyPositionsAsync(sp, manifest, identity, apps, secrets, skips, ct); diff --git a/src/dotnet/Modgud.Api/Features/Admin/Provisioning/RealmManifestPlanner.cs b/src/dotnet/Modgud.Api/Features/Admin/Provisioning/RealmManifestPlanner.cs index 10d0edff..c5eb4395 100644 --- a/src/dotnet/Modgud.Api/Features/Admin/Provisioning/RealmManifestPlanner.cs +++ b/src/dotnet/Modgud.Api/Features/Admin/Provisioning/RealmManifestPlanner.cs @@ -330,8 +330,15 @@ List CanonApps(List apps) => [.. apps.Select foreach (var r in current.Roles) roleRefs.Add(r.NaturalKey, r.Id); var userRefs = new RefCanonicalizer(); foreach (var u in current.Users) userRefs.Add(u.Key ?? u.UserName ?? u.Email, u.Id); + // A group's Members are users, nested groups AND service accounts — the exporter + // writes all three as { Key, Id }, so all three have to canonicalize, or a service + // account written id-only reads as a change the apply would never make. + var memberRefs = new RefCanonicalizer(); + foreach (var u in current.Users) memberRefs.Add(u.Key ?? u.UserName ?? u.Email, u.Id); + foreach (var g in current.Groups) memberRefs.Add(g.Name, g.Id); + foreach (var s in current.ServiceAccounts) memberRefs.Add(s.AccountName, s.Id); RealmManifestGroup CanonGroup(RealmManifestGroup g) - => g with { Members = userRefs.Canon(g.Members), Roles = roleRefs.Canon(g.Roles) }; + => g with { Members = memberRefs.Canon(g.Members), Roles = roleRefs.Canon(g.Roles) }; var positionRefs = new RefCanonicalizer(); foreach (var p in current.Positions) positionRefs.Add(p.AccountName.Trim().ToLowerInvariant(), p.Id); RealmManifestPosition CanonPosition(RealmManifestPosition p) @@ -398,7 +405,12 @@ await GroupMembershipGuards.GroupConfersRealmAdminAsync(session, perms, doc, ct) }, })); - AddSkippedReferenceNotes(manifest, current, result); + // The realm's apps are read LIVE, not off the export: the export leaves the system + // app out on purpose (it is seeded, not authored), but a role, client, API or scope + // may point at it all the same — and the applier resolves it, because its app + // resolver is seeded from the same query. The plan has to see what the apply sees. + var liveApps = await session.Query().Where(a => !a.IsDeleted).ToListAsync(ct); + AddSkippedReferenceNotes(manifest, current, liveApps, result); return result; } @@ -414,15 +426,23 @@ await GroupMembershipGuards.GroupConfersRealmAdminAsync(session, perms, doc, ct) /// reference is resolved they exist. /// private static void AddSkippedReferenceNotes( - RealmManifest manifest, RealmManifest current, RealmPlanResult result) + RealmManifest manifest, RealmManifest current, IReadOnlyList liveApps, RealmPlanResult result) { - var appSlugs = manifest.Apps.Select(a => a.Slug) - .Concat(current.Apps.Select(a => a.Slug)) + // Every app the realm HAS (system app included — the export omits it, the applier's + // resolver does not) plus every app the manifest creates. + var appSlugs = liveApps.Select(a => a.Slug) + .Concat(manifest.Apps.Select(a => a.Slug)) .ToHashSet(StringComparer.Ordinal); - // resource:action keys per app slug, from both sides. + // resource:action keys per app slug, live catalog first, manifest entries on top + // (a manifest may add entries to an app it also updates). var catalog = new Dictionary>(StringComparer.Ordinal); - foreach (var app in current.Apps.Concat(manifest.Apps)) + foreach (var app in liveApps) + { + var set = catalog[app.Slug] = new HashSet(StringComparer.Ordinal); + foreach (var perm in app.Permissions) set.Add($"{perm.Resource}:{perm.Action}"); + } + foreach (var app in manifest.Apps) { if (app.Permissions is null) continue; if (!catalog.TryGetValue(app.Slug, out var set)) @@ -439,6 +459,14 @@ private static void AddSkippedReferenceNotes( var userIds = manifest.Users.Select(u => NormalizedId(u.Id)) .Concat(current.Users.Select(u => NormalizedId(u.Id))) .OfType().ToHashSet(StringComparer.Ordinal); + // A group member is a user, a nested group or a service account — the same three + // kinds the applier's member resolver accepts. + var memberIds = userIds + .Concat(manifest.Groups.Select(g => NormalizedId(g.Id))) + .Concat(current.Groups.Select(g => NormalizedId(g.Id))) + .Concat(manifest.ServiceAccounts.Select(s => NormalizedId(s.Id))) + .Concat(current.ServiceAccounts.Select(s => NormalizedId(s.Id))) + .OfType().ToHashSet(StringComparer.Ordinal); // The readable name each live id actually carries, for the verified-hint check. var roleKeyById = current.Roles .Where(r => NormalizedId(r.Id) is not null) @@ -446,6 +474,11 @@ private static void AddSkippedReferenceNotes( var userKeyById = current.Users .Where(u => NormalizedId(u.Id) is not null) .ToDictionary(u => NormalizedId(u.Id)!, u => u.ResolveKey(), StringComparer.Ordinal); + var memberKeyById = new Dictionary(userKeyById, StringComparer.Ordinal); + foreach (var g in current.Groups) + if (NormalizedId(g.Id) is { } gid) memberKeyById.TryAdd(gid, g.Name); + foreach (var s in current.ServiceAccounts) + if (NormalizedId(s.Id) is { } sid) memberKeyById.TryAdd(sid, s.AccountName); RealmPlanEntry? Entry(string section, string key) { @@ -533,7 +566,7 @@ static string Skipped(string what) => if (ReferenceNote(reference, "Role", roleIds, roleKeyById) is { } note) Note("groups", group.Name, note); foreach (var reference in group.Members ?? []) - if (ReferenceNote(reference, "Member", userIds, userKeyById) is { } note) + if (ReferenceNote(reference, "Member", memberIds, memberKeyById) is { } note) Note("groups", group.Name, note); }