Merge #848 #849 #850 credential name claim/pin, env: per-instance rule, merged preflight

2026-11-11 06:36:06 -05:00
parent 8755cf84bb ea156265a2
commit 26349320df
17 changed files with 958 additions and 85 deletions
@@ -72,6 +72,41 @@ public sealed class BrowserSessionCredentialScopeTests
        universal.Opens.ShouldBe(0);
    }

+    /// <summary>lmxopcua#849: an env: reference named after ANOTHER instance never opens a browser either.</summary>
+    [Theory]
+    [InlineData("env:OTOPCUA_GALAXY_VICTIM_KEY")]
+    [InlineData("env:GALAXY_MXGW_DRV_VICTIM_KEY")]
+    public async Task An_env_reference_naming_another_instance_never_opens_a_browser(string reference)
+    {
+        var bespoke = new RecordingBrowser(DriverTypeNames.Galaxy);
+        var svc = new BrowserSessionService(
+            [bespoke], new BrowseSessionRegistry(), NullLogger<BrowserSessionService>.Instance, new RecordingUniversal(), null!, null!,
+            SeededFactory());
+
+        var result = await svc.OpenAsync(DriverTypeNames.Galaxy,
+            """{"Gateway":{"Endpoint":"http://evil:5001","ApiKeySecretRef":"REF"}}""".Replace("REF", reference), "DRV-evil",
+            TestContext.Current.CancellationToken);
+
+        result.Ok.ShouldBeFalse();
+        result.Message.ShouldNotBeNull().ShouldContain("lmxopcua#849");
+        bespoke.Opens.ShouldBe(0);
+    }
+
+    /// <summary>Positive control: a shared env variable that names no instance reaches the browser.</summary>
+    [Fact]
+    public async Task A_shared_env_variable_reaches_the_browser()
+    {
+        var bespoke = new RecordingBrowser(DriverTypeNames.Galaxy);
+        var svc = new BrowserSessionService(
+            [bespoke], new BrowseSessionRegistry(), NullLogger<BrowserSessionService>.Instance, new RecordingUniversal(), null!, null!,
+            SeededFactory());
+
+        await svc.OpenAsync(DriverTypeNames.Galaxy, """{"Gateway":{"ApiKeySecretRef":"env:GALAXY_MXGW_API_KEY"}}""",
+            TestContext.Current.CancellationToken);
+
+        bespoke.Opens.ShouldBe(1);
+    }
+
    /// <summary>Positive control: the owning driver's own id-scoped secret reaches the browser.</summary>
    [Fact]
    public async Task The_owners_own_secret_reaches_the_browser()
@@ -52,4 +52,46 @@ public sealed class CredentialReferencePreflightInstanceAndPathTests
        f.Problem.ShouldContain("lmxopcua#836");
        f.Problem.ShouldNotContain("plant.pfx");
    }
+
+    /// <summary>
+    ///     lmxopcua#850: the AuthType switch in one config and the path in the other — each config passes alone, the
+    ///     runtime-merged config does not. The deploy gate catches it; the banner must show it before a deploy does.
+    /// </summary>
+    [Fact]
+    public void A_path_split_from_its_certificate_switch_is_listed_from_the_merged_config()
+    {
+        var findings = CredentialReferencePreflight.Scan(
+            [Drv("o", "Upstream", DriverTypeNames.OpcUaClient, """{"AuthType":"Anonymous","UserCertificatePath":"C:\\stolen\\host.pfx"}""")],
+            [new Device { DeviceId = "dev-o", DriverInstanceId = "o", Name = "d", DeviceConfig = """{"AuthType":"Certificate"}""" }],
+            []);
+
+        var f = findings.ShouldHaveSingleItem();
+        f.Source.ShouldBe(CredentialFindingSource.Draft);
+        f.DriverInstanceId.ShouldBe("o");
+        f.DeviceName.ShouldBeNull();
+        f.Problem.ShouldContain("merged with its devices");
+        f.Problem.ShouldContain("lmxopcua#836");
+        f.Problem.ShouldNotContain("host.pfx");
+    }
+
+    /// <summary>Control: a problem already reported on one config is not listed a second time from the merge.</summary>
+    [Fact]
+    public void A_path_already_reported_on_its_own_config_is_not_duplicated_by_the_merge() =>
+        CredentialReferencePreflight.Scan(
+                [Drv("o", "Upstream", DriverTypeNames.OpcUaClient, """{"AuthType":"Certificate","UserCertificatePath":"C:\\certs\\plant.pfx"}""")],
+                [new Device { DeviceId = "dev-o", DriverInstanceId = "o", Name = "d", DeviceConfig = "{}" }],
+                [])
+            .ShouldHaveSingleItem().Problem.ShouldNotContain("merged");
+
+    /// <summary>lmxopcua#849: an env: reference naming another instance is listed like a secret: one.</summary>
+    [Fact]
+    public void A_cross_instance_env_reference_is_listed() =>
+        CredentialReferencePreflight.Scan(
+                [
+                    Drv("DRV-a", "A", DriverTypeNames.Galaxy, "{}"),
+                    Drv("DRV-b", "B", DriverTypeNames.Galaxy, """{"Gateway":{"ApiKeySecretRef":"env:OTOPCUA_GALAXY_DRV_A_KEY"}}"""),
+                ],
+                [],
+                [])
+            .ShouldHaveSingleItem().Problem.ShouldContain("lmxopcua#849");
 }
@@ -0,0 +1,167 @@
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.AdminUI.Uns;
+using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
+
+namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns;
+
+/// <summary>
+///     lmxopcua#848: a driver's NAME is checked against the credential references stored on other drivers. Naming a driver
+///     after another same-type driver's (shared) secret segment or env part would make that reference name "another
+///     instance" and fail its next deploy; renaming a driver away from a name its own references are named after would
+///     free that name for every other driver's Test Connect / browse / save. Both are refused at create, at the /raw
+///     rename and at the config modal's update.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class CredentialNameClaimSavePathTests
+{
+    private static CancellationToken Ct => TestContext.Current.CancellationToken;
+
+    private static string Galaxy(string reference) =>
+        """{"Gateway":{"Endpoint":"http://gw:5001","ApiKeySecretRef":"REF"}}""".Replace("REF", reference);
+
+    private static async Task<(RawTreeService Svc, string Db, string SharerId)> WithSharerAsync(string reference)
+    {
+        var name = RawTreeTestDb.SeedFresh();
+        var svc = new RawTreeService(RawTreeTestDb.Factory(name));
+        var sharer = await svc.CreateDriverAsync(RawTreeTestDb.ClusterId, null, "Sharer", DriverTypeNames.Galaxy, Galaxy(reference), Ct);
+        sharer.Ok.ShouldBeTrue(sharer.Error);
+        return (svc, name, sharer.CreatedId!);
+    }
+
+    private static byte[] RowVersion(string db, string driverId)
+    {
+        using var ctx = RawTreeTestDb.CreateNamed(db);
+        return ctx.DriverInstances.Single(d => d.DriverInstanceId == driverId).RowVersion;
+    }
+
+    [Theory]
+    [InlineData("secret:galaxy/gw/very-private-name", "gw")]
+    [InlineData("secret:galaxy/gw/very-private-name", "GW")]          // case-insensitive, as the rule matches
+    [InlineData("env:GALAXY_MXGW_API_KEY_VERY_PRIVATE", "api")]       // #849 env form: API is a '_'-bounded prefix
+    [InlineData("env:OTOPCUA_GALAXY_LINE_1_VERY_PRIVATE", "line-1")]  // normalised LINE_1
+    public async Task Creating_a_driver_named_after_another_drivers_reference_is_refused(string reference, string newName)
+    {
+        var (svc, db, sharerId) = await WithSharerAsync(reference);
+
+        var result = await svc.CreateDriverAsync(RawTreeTestDb.ClusterId, null, newName, DriverTypeNames.Galaxy, "{}", Ct);
+
+        result.Ok.ShouldBeFalse();
+        result.Error!.ShouldContain("lmxopcua#848");
+        result.Error!.ShouldContain("Sharer");
+        result.Error!.ShouldContain(sharerId);
+        result.Error!.ShouldContain("Gateway.ApiKeySecretRef");
+        result.Error!.ShouldNotContain("PRIVATE", Case.Insensitive); // never the rest of the reference
+        using var check = RawTreeTestDb.CreateNamed(db);
+        check.DriverInstances.ShouldNotContain(d => d.Name == newName);
+    }
+
+    [Fact]
+    public async Task A_device_level_reference_claims_the_name_too()
+    {
+        var (svc, db, sharerId) = await WithSharerAsync("secret:galaxy/sharer-own/k");
+        using (var ctx = RawTreeTestDb.CreateNamed(db))
+        {
+            var device = ctx.Devices.Single(d => d.DriverInstanceId == sharerId);
+            device.DeviceConfig = Galaxy("secret:galaxy/plant7/k");
+            ctx.SaveChanges();
+        }
+
+        var result = await svc.CreateDriverAsync(RawTreeTestDb.ClusterId, null, "Plant7", DriverTypeNames.Galaxy, "{}", Ct);
+
+        result.Ok.ShouldBeFalse();
+        result.Error!.ShouldContain("device 'Device1'");
+    }
+
+    [Fact]
+    public async Task Another_driver_type_and_unrelated_names_are_not_claimed()
+    {
+        var (svc, _, _) = await WithSharerAsync("secret:galaxy/gw/k");
+
+        (await svc.CreateDriverAsync(RawTreeTestDb.ClusterId, null, "gw", DriverTypeNames.OpcUaClient, "{}", Ct)).Ok.ShouldBeTrue();
+        (await svc.CreateDriverAsync(RawTreeTestDb.ClusterId, null, "gw2", DriverTypeNames.Galaxy, "{}", Ct)).Ok.ShouldBeTrue();
+    }
+
+    [Fact]
+    public async Task Renaming_onto_a_claimed_name_is_refused_on_both_rename_paths()
+    {
+        var (svc, db, _) = await WithSharerAsync("secret:galaxy/gw/k");
+        var other = await svc.CreateDriverAsync(RawTreeTestDb.ClusterId, null, "Other", DriverTypeNames.Galaxy, "{}", Ct);
+        other.Ok.ShouldBeTrue(other.Error);
+
+        var renamed = await svc.RenameDriverAsync(other.CreatedId!, "gw", RowVersion(db, other.CreatedId!), Ct);
+        renamed.Ok.ShouldBeFalse();
+        renamed.Error!.ShouldContain("lmxopcua#848");
+
+        var updated = await svc.UpdateDriverAsync(other.CreatedId!, "gw", "{}", null, RowVersion(db, other.CreatedId!), Ct);
+        updated.Ok.ShouldBeFalse();
+        updated.Error!.ShouldContain("lmxopcua#848");
+
+        using var check = RawTreeTestDb.CreateNamed(db);
+        check.DriverInstances.Single(d => d.DriverInstanceId == other.CreatedId).Name.ShouldBe("Other");
+    }
+
+    /// <summary>Issue (b): renaming the victim away from the name its own secret lives under would free that name.</summary>
+    [Theory]
+    [InlineData("secret:galaxy/Sharer/apikey")]
+    [InlineData("env:OTOPCUA_GALAXY_SHARER_KEY")]
+    public async Task A_driver_whose_references_are_named_after_it_cannot_be_renamed_away(string reference)
+    {
+        var (svc, db, sharerId) = await WithSharerAsync(reference);
+
+        var renamed = await svc.RenameDriverAsync(sharerId, "Sharer-old", RowVersion(db, sharerId), Ct);
+        renamed.Ok.ShouldBeFalse();
+        renamed.Error!.ShouldContain("lmxopcua#848");
+        renamed.Error!.ShouldContain(sharerId); // points at the id-scoped form
+
+        (await svc.UpdateDriverAsync(sharerId, "Sharer-old", Galaxy(reference), null, RowVersion(db, sharerId), Ct)).Ok.ShouldBeFalse();
+    }
+
+    [Fact]
+    public async Task Re_pointing_to_the_id_in_the_same_save_allows_the_rename()
+    {
+        var (svc, db, sharerId) = await WithSharerAsync("secret:galaxy/Sharer/apikey");
+
+        var updated = await svc.UpdateDriverAsync(
+            sharerId, "Sharer-new", Galaxy($"secret:galaxy/{sharerId}/apikey"), null, RowVersion(db, sharerId), Ct);
+
+        updated.Ok.ShouldBeTrue(updated.Error);
+    }
+
+    [Theory]
+    [InlineData("sharer")] // case-only: the rule matches case-insensitively, so nothing is freed or claimed
+    [InlineData("Renamed")]
+    public async Task Renames_that_free_and_claim_nothing_pass(string newName)
+    {
+        var reference = newName == "sharer" ? "secret:galaxy/Sharer/apikey" : "secret:galaxy/shared-gw/apikey";
+        var (svc, db, sharerId) = await WithSharerAsync(reference);
+
+        var renamed = await svc.RenameDriverAsync(sharerId, newName, RowVersion(db, sharerId), Ct);
+
+        renamed.Ok.ShouldBeTrue(renamed.Error);
+    }
+
+    /// <summary>
+    ///     #849 review: a name in the system-generated id form ('drv', 'DRV-&lt;12 hex&gt;…') would shadow every driver's
+    ///     id-form references — refused at create, the /raw rename and the config modal, even with no reference stored.
+    /// </summary>
+    [Theory]
+    [InlineData("drv")]
+    [InlineData("DRV")]
+    [InlineData("DRV-0123456789ab")]
+    [InlineData("drv-0123456789AB-line")]
+    public async Task An_id_shaped_name_is_refused_on_every_name_path(string newName)
+    {
+        var (svc, db, sharerId) = await WithSharerAsync("env:OTOPCUA_GALAXY_DRV_0123456789AB_KEY");
+
+        var create = await svc.CreateDriverAsync(RawTreeTestDb.ClusterId, null, newName, DriverTypeNames.Galaxy, "{}", Ct);
+        create.Ok.ShouldBeFalse();
+        create.Error!.ShouldContain("system-generated driver id");
+
+        (await svc.RenameDriverAsync(sharerId, newName, RowVersion(db, sharerId), Ct)).Ok.ShouldBeFalse();
+        (await svc.UpdateDriverAsync(sharerId, newName, "{}", null, RowVersion(db, sharerId), Ct)).Ok.ShouldBeFalse();
+
+        // A type without credential rules is not affected.
+        (await svc.CreateDriverAsync(RawTreeTestDb.ClusterId, null, newName, DriverTypeNames.Modbus, "{}", Ct)).Ok.ShouldBeTrue();
+    }
+}
@@ -0,0 +1,67 @@
+using Microsoft.EntityFrameworkCore;
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.AdminUI.Uns;
+using ZB.MOM.WW.OtOpcUa.Configuration.Entities;
+using ZB.MOM.WW.OtOpcUa.Configuration.Enums;
+using ZB.MOM.WW.OtOpcUa.Configuration.Validation;
+using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
+
+namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns;
+
+/// <summary>
+///     The SQL-Server-backed half of <see cref="CredentialNameClaimSavePathTests"/> (lmxopcua#848): the driver + device
+///     reads of <see cref="DriverInstanceIdentities.DescribeNameChangeProblemsAsync"/> must translate on the real provider,
+///     not only run as LINQ-to-Objects under EF InMemory (the #815 live gate's crash-looping boot came from exactly that gap).
+/// </summary>
+[Collection(nameof(AdminUiSqlServerCollection))]
+public sealed class CredentialNameClaimSqlServerTests(AdminUiSqlServerFixture sql)
+{
+    private static CancellationToken Ct => TestContext.Current.CancellationToken;
+
+    [SqlServerFact]
+    public async Task Create_and_rename_are_checked_against_stored_driver_and_device_references()
+    {
+        // 8 hex chars: unique per run in the shared database, short enough for the id columns.
+        var token = Guid.NewGuid().ToString("N")[..8];
+        var clusterId = $"C-{token}";
+        var sharerId = $"DRV-s{token}";
+        await using (var db = await sql.Factory.CreateDbContextAsync(Ct))
+        {
+            db.ServerClusters.Add(new ServerCluster
+            {
+                ClusterId = clusterId, Name = $"Cluster {token}", Enterprise = "zb", Site = "warsaw-west",
+                NodeCount = 1, RedundancyMode = RedundancyMode.None, CreatedBy = "test",
+            });
+            db.DriverInstances.Add(new DriverInstance
+            {
+                DriverInstanceId = sharerId, ClusterId = clusterId, Name = $"Sharer{token}", DriverType = DriverTypeNames.Galaxy,
+                DriverConfig = "{\"Gateway\":{\"ApiKeySecretRef\":\"secret:galaxy/gw" + token + "/k\"}}",
+            });
+            db.Devices.Add(new Device
+            {
+                DeviceId = $"DEV-s{token}", DriverInstanceId = sharerId, Name = "Device1",
+                DeviceConfig = "{\"AlarmAck\":{\"FallbackPasswordSecretRef\":\"env:OTOPCUA_GALAXY_SHARER" + token.ToUpperInvariant() + "_ACK\"}}",
+            });
+            await db.SaveChangesAsync(Ct);
+        }
+
+        var svc = new RawTreeService(sql.Factory);
+
+        // Claim (driver-level secret: reference).
+        var create = await svc.CreateDriverAsync(clusterId, null, $"gw{token}", DriverTypeNames.Galaxy, "{}", Ct);
+        create.Ok.ShouldBeFalse();
+        create.Error!.ShouldContain("lmxopcua#848");
+
+        // Pin (device-level env: reference named after the sharer's own name).
+        byte[] rv;
+        await using (var db = await sql.Factory.CreateDbContextAsync(Ct))
+            rv = await db.DriverInstances.AsNoTracking().Where(d => d.DriverInstanceId == sharerId).Select(d => d.RowVersion).SingleAsync(Ct);
+        var rename = await svc.RenameDriverAsync(sharerId, $"Moved{token}", rv, Ct);
+        rename.Ok.ShouldBeFalse();
+        rename.Error!.ShouldContain("device 'Device1'");
+
+        // Positive control: an unclaimed name creates.
+        (await svc.CreateDriverAsync(clusterId, null, $"free{token}", DriverTypeNames.Galaxy, "{}", Ct)).Ok.ShouldBeTrue();
+    }
+}
@@ -76,6 +76,44 @@ public sealed class AdminOperationsActorCredentialScopeTests : ControlPlaneActor
        probe.Calls.ShouldBe(0);
    }

+    /// <summary>lmxopcua#849: an env: reference named after ANOTHER instance never reaches the probe either.</summary>
+    [Theory]
+    [InlineData("env:OTOPCUA_GALAXY_VICTIM_API_KEY")]
+    [InlineData("env:GALAXY_MXGW_DRV_VICTIM_KEY")]
+    public void An_env_reference_naming_another_instance_never_reaches_the_probe(string reference)
+    {
+        var factory = NewInMemoryDbFactory();
+        SeedGalaxyDrivers(factory);
+        var probe = new RecordingProbe(DriverTypeNames.Galaxy);
+        var actor = Sys.ActorOf(AdminOperationsActor.Props(factory, CreateTestProbe("coord").Ref, new IDriverProbe[] { probe }));
+
+        actor.Tell(new TestDriverConnect(
+            DriverTypeNames.Galaxy, """{"Gateway":{"Endpoint":"http://attacker:5001","ApiKeySecretRef":"REF"}}""".Replace("REF", reference),
+            5, Guid.NewGuid(), "DRV-evil"));
+
+        var reply = ExpectMsg<TestDriverConnectResult>(PresenceBudget, cancellationToken: TestContext.Current.CancellationToken);
+        reply.Ok.ShouldBeFalse();
+        reply.Message.ShouldNotBeNull().ShouldContain("lmxopcua#849");
+        probe.Calls.ShouldBe(0);
+    }
+
+    /// <summary>Positive control: a shared env variable naming no instance reaches the probe.</summary>
+    [Fact]
+    public void A_shared_env_variable_reaches_the_probe()
+    {
+        var factory = NewInMemoryDbFactory();
+        SeedGalaxyDrivers(factory);
+        var probe = new RecordingProbe(DriverTypeNames.Galaxy);
+        var actor = Sys.ActorOf(AdminOperationsActor.Props(factory, CreateTestProbe("coord").Ref, new IDriverProbe[] { probe }));
+
+        actor.Tell(new TestDriverConnect(
+            DriverTypeNames.Galaxy, """{"Gateway":{"Endpoint":"http://attacker:5001","ApiKeySecretRef":"env:GALAXY_MXGW_API_KEY"}}""",
+            5, Guid.NewGuid()));
+
+        ExpectMsg<TestDriverConnectResult>(PresenceBudget, cancellationToken: TestContext.Current.CancellationToken).Ok.ShouldBeTrue();
+        probe.Calls.ShouldBe(1);
+    }
+
    /// <summary>Positive control: the owner's own secret (and a shared one) does reach the probe.</summary>
    [Theory]
    [InlineData("secret:galaxy/DRV-victim/apikey")]