diff --git a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json index b1fb0428..86cb2627 100644 --- a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json +++ b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json @@ -59,7 +59,7 @@ { "id": "T8", "subject": "RoleAuthorityGuardTests: source scan proving no production file outside the authority reads OTOPCUA_ROLES, Cluster:Roles, or a bare role literal", - "status": "pending", + "status": "completed", "blockedBy": [ "T3", "T4", diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/ServiceCollectionExtensions.cs b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/ServiceCollectionExtensions.cs index 897e6869..21245437 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/ServiceCollectionExtensions.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/ServiceCollectionExtensions.cs @@ -21,7 +21,9 @@ namespace ZB.MOM.WW.OtOpcUa.ControlPlane; public static class ServiceCollectionExtensions { - public const string AdminRole = "admin"; + // R3-04 (theme 4 guard): alias RoleParser.Admin rather than duplicating the literal — the same + // pattern Runtime/ServiceCollectionExtensions.DriverRole already follows for RoleParser.Driver. + public const string AdminRole = RoleParser.Admin; public const string ConfigPublishSingletonName = "config-publish"; public const string AdminOperationsSingletonName = "admin-operations"; diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs index 7c8f5b3d..3100aa9e 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs @@ -1,5 +1,6 @@ using Akka.Actor; using Akka.Event; +using ZB.MOM.WW.OtOpcUa.Cluster; using ZB.MOM.WW.OtOpcUa.Commons.Observability; using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; using ZB.MOM.WW.OtOpcUa.Commons.Types; @@ -310,7 +311,9 @@ public sealed class DriverInstanceActor : ReceiveActor, IWithTimers { var isWindowsOnly = driverType is "Galaxy"; if (!OperatingSystem.IsWindows() && isWindowsOnly) return true; - if (roles.Contains("dev") && isWindowsOnly) return true; + // R3-04 (theme 4 guard): GossipedRolesHave, not a bare "dev" literal Contains — the same + // case-insensitivity the rest of the role authority gives every other consumer. + if (RoleParser.GossipedRolesHave(roles, RoleParser.Dev) && isWindowsOnly) return true; return false; } diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/RoleAuthorityGuardTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/RoleAuthorityGuardTests.cs new file mode 100644 index 00000000..3bb85907 --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/RoleAuthorityGuardTests.cs @@ -0,0 +1,250 @@ +using System.Text.RegularExpressions; +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Cluster.Tests; + +/// +/// R3-04 (theme 4, "one rule stated twice diverges"): the role-reading rule may live in exactly +/// one place. Three source-scan rules, modelled on +/// CommandDisposalConventionsTests's root-walk idiom, over every src/**/*.cs + +/// src/**/*.razor file: +/// +/// No file except EffectiveRoles.cs/RoleParser.cs reads +/// OTOPCUA_ROLES. +/// No file except EffectiveRoles.cs reads the Cluster:Roles +/// key as code (an operator-facing message string that merely NAMES the key is fine — it +/// reads as a sentence, i.e. its string literal also contains a space). +/// No file except RoleParser.cs declares a bare +/// "admin"/"driver"/"dev" role literal. +/// +/// A textual scan, not reflection, because the thing being guarded is a textual read +/// (Environment.GetEnvironmentVariable("OTOPCUA_ROLES"), GetSection("Cluster:Roles"), +/// a bare "driver") that reflection cannot see. +/// +/// +/// Falsification control: +/// feeds the scan the EXACT pre-fix Program.cs/Fleet.razor snippets (captured +/// verbatim before T3/T5 touched them) and asserts it finds them — proof this guard can fail, not +/// just that the current tree happens to pass. Run against the real pre-T3 tree at commit +/// 7dfd6dcc (Program.cs) / pre-T5 at bcec88ef (Fleet.razor), the live scan reported: +/// Host/Program.cs:61, :62, :63 (OTOPCUA_ROLES + two bare role literals) and +/// AdminUI/Components/Pages/Fleet.razor:149, :159 (two bare "driver" literals) +/// — recorded here per the plan's "paste the list into the commit body" instruction. +/// +public sealed class RoleAuthorityGuardTests +{ + private static readonly string[] OtopcuaRolesAllowedFiles = ["EffectiveRoles.cs", "RoleParser.cs"]; + private static readonly string[] ClusterRolesKeyAllowedFiles = ["EffectiveRoles.cs"]; + private static readonly string[] RoleLiteralAllowedFiles = ["RoleParser.cs"]; + + /// + /// Files whose bare "admin"/"driver"/"dev" literal names something OTHER + /// than an Akka Cluster:Roles role — each entry carries the reason, per the plan's + /// "add the file to the small, named allow-list WITH A REASON" instruction. + /// + private static readonly IReadOnlyDictionary RoleLiteralAllowListWithReasons = + new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["RawTreeService.cs"] = + "raw-tree node-KIND discriminator (Folder/Driver/Device/TagGroup/Tag), not an Akka role", + ["AclScopeService.cs"] = + "human-readable entity-type name passed to ResolveAsync for an error message, not a role", + ["SqlTagDefinitionFactory.cs"] = + "the Sql driver's own TagConfig JSON blob field ({\"driver\":\"Sql\",...}), not a role", + ["SqlEquipmentTagParser.cs"] = + "the Sql driver's own TagConfig JSON blob field ({\"driver\":\"Sql\",...}), not a role", + ["SqlAddressPickerBody.razor"] = + "the Sql driver's own TagConfig JSON blob field ({\"driver\":\"Sql\",...}), not a role", + ["OtOpcUaLdapAuthService.cs"] = + "an LDAP-derived ADMINUI RBAC role (the DevStubMode bypass group), not Cluster:Roles", + }; + + private static string SolutionRoot() + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir is not null && !File.Exists(Path.Combine(dir.FullName, "ZB.MOM.WW.OtOpcUa.slnx"))) + dir = dir.Parent; + dir.ShouldNotBeNull("Could not find solution root (ZB.MOM.WW.OtOpcUa.slnx)."); + return dir!.FullName; + } + + private static IEnumerable SourceFiles() + { + var srcDir = Path.Combine(SolutionRoot(), "src"); + var sep = Path.DirectorySeparatorChar; + return Directory.EnumerateFiles(srcDir, "*.cs", SearchOption.AllDirectories) + .Concat(Directory.EnumerateFiles(srcDir, "*.razor", SearchOption.AllDirectories)) + .Where(f => !f.Contains($"{sep}obj{sep}", StringComparison.Ordinal) + && !f.Contains($"{sep}bin{sep}", StringComparison.Ordinal)); + } + + /// Every line of that is not a full-line ////// + /// comment, 1-based line numbers. + private static IEnumerable<(int Line, string Text)> NonCommentLines(string text) + { + var lines = text.Split('\n'); + for (var i = 0; i < lines.Length; i++) + { + if (lines[i].TrimStart().StartsWith("//", StringComparison.Ordinal)) continue; + yield return (i + 1, lines[i]); + } + } + + private static readonly Regex BareRoleLiteral = new("\"(admin|driver|dev)\"", RegexOptions.Compiled); + + /// True when the occurrence of a -character key at + /// sits inside a string literal that ALSO contains a space — the + /// shape an operator-facing message reads as (naming the key for guidance is not a CODE READ of + /// it), per the plan's carve-out for Cluster:Roles — applied identically to + /// OTOPCUA_ROLES for the same reason (GrpcApplyAckSender's warning names BOTH keys + /// on one line; treating the two inconsistently would flag one and not the other on that exact + /// sentence). + private static bool IsAllowedMessageOccurrence(string line, int matchIndex, int matchLength) + { + var openQuote = line.LastIndexOf('"', matchIndex); + if (openQuote < 0) return false; // not inside any string literal at all -> a bare code read + + var searchFrom = matchIndex + matchLength; + if (searchFrom >= line.Length) return false; + var closeQuote = line.IndexOf('"', searchFrom); + if (closeQuote < 0) return false; + + return line[openQuote..(closeQuote + 1)].Contains(' '); + } + + private static List FindOtopcuaRolesOffenders(string text) => + NonCommentLines(text) + .Where(l => + { + var idx = l.Text.IndexOf("OTOPCUA_ROLES", StringComparison.Ordinal); + return idx >= 0 && !IsAllowedMessageOccurrence(l.Text, idx, "OTOPCUA_ROLES".Length); + }) + .Select(l => l.Line.ToString()) + .ToList(); + + private static List FindClusterRolesKeyOffenders(string text) => + NonCommentLines(text) + .Where(l => + { + var idx = l.Text.IndexOf("Cluster:Roles", StringComparison.Ordinal); + return idx >= 0 && !IsAllowedMessageOccurrence(l.Text, idx, "Cluster:Roles".Length); + }) + .Select(l => l.Line.ToString()) + .ToList(); + + private static List FindBareRoleLiteralOffenders(string text) => + NonCommentLines(text) + .Where(l => BareRoleLiteral.IsMatch(l.Text)) + .Select(l => l.Line.ToString()) + .ToList(); + + [Fact] + public void No_file_outside_the_authority_reads_OTOPCUA_ROLES() + { + var offenders = new List(); + foreach (var file in SourceFiles()) + { + if (OtopcuaRolesAllowedFiles.Contains(Path.GetFileName(file))) continue; + + foreach (var line in FindOtopcuaRolesOffenders(File.ReadAllText(file))) + offenders.Add($"{RelativePath(file)}:{line}"); + } + + offenders.ShouldBeEmpty( + "OTOPCUA_ROLES must be read only by EffectiveRoles.cs/RoleParser.cs. Offenders:\n" + + string.Join("\n", offenders)); + } + + [Fact] + public void No_file_outside_EffectiveRoles_reads_the_ClusterRoles_key_as_code() + { + var offenders = new List(); + foreach (var file in SourceFiles()) + { + if (ClusterRolesKeyAllowedFiles.Contains(Path.GetFileName(file))) continue; + + foreach (var line in FindClusterRolesKeyOffenders(File.ReadAllText(file))) + offenders.Add($"{RelativePath(file)}:{line}"); + } + + offenders.ShouldBeEmpty( + "Cluster:Roles must be read as code only by EffectiveRoles.cs (naming the key inside an " + + "operator-facing message string is fine). Offenders:\n" + string.Join("\n", offenders)); + } + + [Fact] + public void No_file_outside_RoleParser_declares_a_bare_role_literal() + { + var offenders = new List(); + foreach (var file in SourceFiles()) + { + var fileName = Path.GetFileName(file); + if (RoleLiteralAllowedFiles.Contains(fileName)) continue; + if (RoleLiteralAllowListWithReasons.ContainsKey(fileName)) continue; + + foreach (var line in FindBareRoleLiteralOffenders(File.ReadAllText(file))) + offenders.Add($"{RelativePath(file)}:{line}"); + } + + offenders.ShouldBeEmpty( + "A bare \"admin\"/\"driver\"/\"dev\" role literal must live only in RoleParser.cs (use " + + "RoleParser.Admin/.Driver/.Dev, or add the file to RoleLiteralAllowListWithReasons WITH " + + "A REASON if it genuinely names something else). Offenders:\n" + string.Join("\n", offenders)); + } + + /// The allow-list itself must not silently grow stale — every reason must be non-blank, + /// and the file it names must actually exist and actually contain the literal it is excused for + /// (an allow-list entry that no longer applies is dead and hides nothing). + [Fact] + public void Every_allow_list_entry_has_a_reason_and_still_applies() + { + foreach (var (fileName, reason) in RoleLiteralAllowListWithReasons) + { + reason.ShouldNotBeNullOrWhiteSpace($"{fileName}'s allow-list entry has no reason."); + + var matches = SourceFiles().Where(f => Path.GetFileName(f) == fileName).ToList(); + matches.ShouldNotBeEmpty($"{fileName} is allow-listed but no such file exists under src/."); + + var stillOffends = matches.Any(f => FindBareRoleLiteralOffenders(File.ReadAllText(f)).Count > 0); + stillOffends.ShouldBeTrue( + $"{fileName} is allow-listed for a bare role literal but no longer contains one — " + + "remove the stale entry."); + } + } + + /// + /// Falsification control (per the plan: "Run on the pre-T3 tree → FAIL... paste that list + /// into the commit body"). These are the EXACT pre-fix lines — copied verbatim, not + /// paraphrased — from Host/Program.cs (before T3, commit 7dfd6dcc) and + /// AdminUI/Components/Pages/Fleet.razor (before T5, commit bcec88ef). If this + /// test ever goes green for the wrong reason (the scan silently stops matching), it is no + /// longer proof the guard can fail. + /// + [Fact] + public void The_scan_actually_detects_the_pre_R3_04_offenders() + { + const string preT3ProgramCs = """ + // Roles drive the entire conditional wiring below — see ZB.MOM.WW.OtOpcUa.Cluster.RoleParser. + var roles = RoleParser.Parse(Environment.GetEnvironmentVariable("OTOPCUA_ROLES")); + var hasAdmin = roles.Contains("admin"); + var hasDriver = roles.Contains("driver"); + """; + + const string preT5FleetRazor = """ + var akkaDrivers = Cluster.MembersWithRole("driver") + .Select(n => n.Value).ToHashSet(StringComparer.OrdinalIgnoreCase); + """; + + FindOtopcuaRolesOffenders(preT3ProgramCs).ShouldNotBeEmpty( + "the falsification control failed to reproduce the pre-T3 OTOPCUA_ROLES violation"); + FindBareRoleLiteralOffenders(preT3ProgramCs).Count.ShouldBeGreaterThanOrEqualTo(2, + "the falsification control failed to reproduce BOTH pre-T3 bare-literal violations " + + "('admin' and 'driver')"); + FindBareRoleLiteralOffenders(preT5FleetRazor).ShouldNotBeEmpty( + "the falsification control failed to reproduce the pre-T5 Fleet.razor bare-literal violation"); + } + + private static string RelativePath(string fullPath) => + Path.GetRelativePath(SolutionRoot(), fullPath).Replace('\\', '/'); +}