From 8221ee797ee5819c959ebe1d49db997dbecdc891 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 10 Jul 2026 05:52:45 -0400 Subject: [PATCH] fix(management): canonicalize role-mapping casing at write path, closing UI/actor case split (arch-review C5) Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj --- docs/requirements/Component-Security.md | 1 + .../ManagementActor.cs | 35 ++++++++-------- .../RoleMappingValidationTests.cs | 42 ++++++++++++++++++- 3 files changed, 58 insertions(+), 20 deletions(-) diff --git a/docs/requirements/Component-Security.md b/docs/requirements/Component-Security.md index 63c2baf3..3fc88241 100644 --- a/docs/requirements/Component-Security.md +++ b/docs/requirements/Component-Security.md @@ -177,6 +177,7 @@ Role checks are expressed as named ASP.NET Core authorization policies (in `Auth - `SCADA-Verifiers` → Verifier role (approves secured writes) - A user can be a member of multiple groups, granting multiple independent roles. - Group mappings are stored in the configuration database and managed via the Central UI (Administrator role). +- **Stored role casing is canonicalized at the write path** (arch-review C5): the mapping's `Role` arrives as a free string over the CLI/Management API, so `ManagementActor.HandleCreateRoleMapping`/`HandleUpdateRoleMapping` canonicalize it to the exact `Roles.All` casing (e.g. `deployer` → `Deployer`) before persisting, and reject any value not in `Roles.All`. This closes a CLI-works/UI-403s split-brain: the ASP.NET `RequireClaim` policies compare the role claim ordinal case-sensitively while the ManagementActor gates compare case-insensitively, so a row stored with off-canonical casing would pass every actor gate but fail every UI policy. Canonicalizing at this single server-side write path guarantees the two comparisons always agree. ## Permission Enforcement diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index 9aee5c93..e8ddf3d5 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -2103,28 +2103,27 @@ public class ManagementActor : ReceiveActor return await repo.GetAllMappingsAsync(); } - // An LDAP-group mapping's Role is a free string on the - // wire (CLI/API), so reject anything outside the canonical Roles.All set at the single - // server-side write path. A non-canonical role never functioned (no policy or authz - // check matches it), so rejecting it removes a silent-misconfiguration footgun rather - // than changing behaviour. Membership is checked case-insensitively to match the rest - // of the actor's role comparisons; the existing case-sensitivity asymmetry between the - // UI RequireClaim policies and the ManagementActor check is a separately-deferred change - // and is deliberately NOT altered here — the stored value's verbatim casing is preserved. - private static void ValidateMappingRole(string role) + // An LDAP-group mapping's Role is a free string on the wire (CLI/API), so both + // validate membership AND canonicalize casing at the single server-side write path. + // The stored Role must be CANONICAL casing (Roles.All): ASP.NET RequireClaim policies + // compare ordinal case-sensitively while the actor compares OrdinalIgnoreCase, so a + // row stored as "deployer" passes every actor gate but fails every UI policy — a + // CLI-works/UI-403s split-brain. Canonicalizing here closes the asymmetry (arch-review + // C5; closes the deferral formerly documented here). A role outside Roles.All never + // functioned (no policy or authz check matches it), so rejecting it removes a + // silent-misconfiguration footgun rather than changing behaviour. + private static string CanonicalizeMappingRole(string role) { - if (!Roles.All.Contains(role, StringComparer.OrdinalIgnoreCase)) - { - throw new ManagementCommandException( - $"Role '{role}' is not a recognized role. Valid roles are: {string.Join(", ", Roles.All)}."); - } + var canonical = Roles.All.FirstOrDefault(r => string.Equals(r, role, StringComparison.OrdinalIgnoreCase)); + return canonical ?? throw new ManagementCommandException( + $"Role '{role}' is not a recognized role. Valid roles are: {string.Join(", ", Roles.All)}."); } private static async Task HandleCreateRoleMapping(IServiceProvider sp, CreateRoleMappingCommand cmd, string user) { - ValidateMappingRole(cmd.Role); + var role = CanonicalizeMappingRole(cmd.Role); var repo = sp.GetRequiredService(); - var mapping = new LdapGroupMapping(cmd.LdapGroupName, cmd.Role); + var mapping = new LdapGroupMapping(cmd.LdapGroupName, role); await repo.AddMappingAsync(mapping); await repo.SaveChangesAsync(); await AuditAsync(sp, user, "Create", "RoleMapping", mapping.Id.ToString(), $"{mapping.LdapGroupName}->{mapping.Role}", mapping); @@ -2133,12 +2132,12 @@ public class ManagementActor : ReceiveActor private static async Task HandleUpdateRoleMapping(IServiceProvider sp, UpdateRoleMappingCommand cmd, string user) { - ValidateMappingRole(cmd.Role); + var role = CanonicalizeMappingRole(cmd.Role); var repo = sp.GetRequiredService(); var mapping = await repo.GetMappingByIdAsync(cmd.MappingId) ?? throw new ManagementCommandException($"RoleMapping with ID {cmd.MappingId} not found."); mapping.LdapGroupName = cmd.LdapGroupName; - mapping.Role = cmd.Role; + mapping.Role = role; await repo.UpdateMappingAsync(mapping); await repo.SaveChangesAsync(); await AuditAsync(sp, user, "Update", "RoleMapping", mapping.Id.ToString(), $"{mapping.LdapGroupName}->{mapping.Role}", mapping); diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/RoleMappingValidationTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/RoleMappingValidationTests.cs index 50a8c7e2..7b8514ad 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/RoleMappingValidationTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/RoleMappingValidationTests.cs @@ -109,9 +109,47 @@ public class RoleMappingValidationTests : TestKit, IDisposable ExpectMsg(TimeSpan.FromSeconds(5)); Assert.NotNull(inserted); - // Membership check does not alter the stored value's casing (case-sensitivity - // asymmetry is a separately-deferred change). + // A correctly-cased canonical role round-trips unchanged. Assert.Equal(Roles.Deployer, inserted!.Role); Assert.Equal("SCADA-Deploy", inserted.LdapGroupName); } + + [Fact] + public void CreateRoleMapping_LowercaseRole_StoresCanonicalCasing() + { + // arch-review C5: ASP.NET RequireClaim policies compare Role ordinal + // case-sensitively while the actor compares OrdinalIgnoreCase. A row stored + // as "deployer" would pass every actor gate but fail every UI policy (a + // CLI-works/UI-403s split-brain). The write path now canonicalizes to the + // Roles.All casing, so the stored value is always "Deployer". + LdapGroupMapping? inserted = null; + _securityRepo + .When(r => r.AddMappingAsync(Arg.Any(), Arg.Any())) + .Do(ci => inserted = ci.Arg()); + + var actor = CreateActor(); + actor.Tell(Envelope(new CreateRoleMappingCommand("SCADA-Deploy", "deployer"))); + + ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.NotNull(inserted); + Assert.Equal("Deployer", inserted!.Role); // canonical, not the verbatim "deployer" + } + + [Fact] + public void UpdateRoleMapping_LowercaseRole_StoresCanonicalCasing() + { + var existing = new LdapGroupMapping("SCADA-Deploy", Roles.Viewer) { Id = 7 }; + _securityRepo.GetMappingByIdAsync(7, Arg.Any()).Returns(existing); + LdapGroupMapping? updated = null; + _securityRepo + .When(r => r.UpdateMappingAsync(Arg.Any(), Arg.Any())) + .Do(ci => updated = ci.Arg()); + + var actor = CreateActor(); + actor.Tell(Envelope(new UpdateRoleMappingCommand(7, "SCADA-Deploy", "deployer"))); + + ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.NotNull(updated); + Assert.Equal("Deployer", updated!.Role); + } }