fix(heidenhain): refuse literal credentials and writable/mistyped tags at the save path (#803 review)

- Review 1: RawTreeService Create/UpdateDriver and Create/UpdateDevice refuse a Heidenhain config whose password fields are not blank / secret:heidenhain/... / env:OTOPCUA_HEIDENHAIN_... (DriverConfigSaveGate over HeidenhainOptionsParser.DescribeCredentialProblems: top level, Access, Devices[], case-insensitive keys; the value is never echoed), so a literal password never reaches the draft DB. The driver and device modals show the reason and disable Save. - Review 8: CreateTag/UpdateTag/ImportTags (tag modal, manual entry, CSV import, browse commit) refuse a ReadWrite Heidenhain tag or a Tag.DataType that disagrees with what the driver publishes (HeidenhainTagRules: catalog type, Float64 => Double, or the binding's dataType). The tag modal and manual-entry grid fix the access level at Read and derive the data type from the address (RawTagRowDefaults). New deploy gate HeidenhainTagDataTypeMismatch. - docs/drivers/Heidenhain.md: credential namespace convention + why, the save-path and deploy enforcement, optional-role credential behaviour, BadConfigurationError, backoff/ordering notes. Tests: HeidenhainSavePathTests (service save path; red with the gates disabled) and new DraftValidatorHeidenhainTests cases.
2026-11-10 23:54:32 -05:00
parent f7a3588b24
commit 54f58c583c
15 changed files with 427 additions and 33 deletions
@@ -27,15 +27,15 @@
        <div class="row g-3">
            <div class="col-md-4">
                <label class="form-label" for="hhInspect">INSPECT password ref</label>
-                <InputText id="hhInspect" @bind-Value="_model.InspectPassword" @bind-Value:after="EmitAsync" class="form-control form-control-sm mono" placeholder="secret:lsv2/inspect" />
+                <InputText id="hhInspect" @bind-Value="_model.InspectPassword" @bind-Value:after="EmitAsync" class="form-control form-control-sm mono" placeholder="secret:heidenhain/inspect" />
            </div>
            <div class="col-md-4">
                <label class="form-label" for="hhDnc">DNC password ref</label>
-                <InputText id="hhDnc" @bind-Value="_model.DncPassword" @bind-Value:after="EmitAsync" class="form-control form-control-sm mono" placeholder="secret:lsv2/dnc" />
+                <InputText id="hhDnc" @bind-Value="_model.DncPassword" @bind-Value:after="EmitAsync" class="form-control form-control-sm mono" placeholder="secret:heidenhain/dnc" />
            </div>
            <div class="col-md-4">
                <label class="form-label" for="hhData">DATA password ref</label>
-                <InputText id="hhData" @bind-Value="_model.DataPassword" @bind-Value:after="EmitAsync" class="form-control form-control-sm mono" placeholder="env:LSV2_DATA_PASSWORD" />
+                <InputText id="hhData" @bind-Value="_model.DataPassword" @bind-Value:after="EmitAsync" class="form-control form-control-sm mono" placeholder="env:OTOPCUA_HEIDENHAIN_DATA_PASSWORD" />
            </div>
        </div>
        <div class="form-text">Blank = use the driver-level reference. <span class="mono">secret:&lt;name&gt;</span> resolves from the node's secret store, <span class="mono">env:&lt;VAR&gt;</span> from the environment, at each login. A literal password is never stored.</div>
@@ -62,6 +62,10 @@
                            <DriverTestConnectButton DriverType="@_driverType" GetConfigJson="@BuildMergedProbeConfig" />
                        </div>

+                        @if (DriverConfigSaveGate.Validate(_driverType, _deviceConfigJson) is { } gateError)
+                        {
+                            <div class="panel notice notice-alert mt-3">@gateError</div>
+                        }
                        @if (_saveError is not null)
                        {
                            <div class="panel notice notice-alert mt-3">@_saveError</div>
@@ -73,7 +77,8 @@
                        <button type="button" class="btn btn-outline-secondary" @onclick="CloseAsync" disabled="@_busy">Cancel</button>
                        @if (_loadError is null)
                        {
-                            <button type="button" class="btn btn-primary" @onclick="SaveAsync" disabled="@_busy">
+                            <button type="button" class="btn btn-primary" @onclick="SaveAsync"
+                                    disabled="@(_busy || DriverConfigSaveGate.Validate(_driverType, _deviceConfigJson) is not null)">
                                @if (_busy) { <span class="spinner-border spinner-border-sm me-1"></span> }
                                @(_isNew ? "Create device" : "Save changes")
                            </button>
@@ -195,7 +200,9 @@

    private async Task SaveAsync()
    {
-        _busy = true; _saveError = null;
+        _saveError = DriverConfigSaveGate.Validate(_driverType, _deviceConfigJson);
+        if (_saveError is not null) return;
+        _busy = true;
        try
        {
            UnsMutationResult result;
@@ -63,6 +63,10 @@
                            </p>
                        }

+                        @if (DriverConfigSaveGate.Validate(_driverType, _driverConfigJson) is { } gateError)
+                        {
+                            <div class="panel notice notice-alert mt-3">@gateError</div>
+                        }
                        @if (_saveError is not null)
                        {
                            <div class="panel notice notice-alert mt-3">@_saveError</div>
@@ -74,7 +78,8 @@
                        <button type="button" class="btn btn-outline-secondary" @onclick="CloseAsync" disabled="@_busy">Cancel</button>
                        @if (_loadError is null)
                        {
-                            <button type="button" class="btn btn-primary" @onclick="SaveAsync" disabled="@_busy">
+                            <button type="button" class="btn btn-primary" @onclick="SaveAsync"
+                                    disabled="@(_busy || DriverConfigSaveGate.Validate(_driverType, _driverConfigJson) is not null)">
                                @if (_busy) { <span class="spinner-border spinner-border-sm me-1"></span> }
                                @(_isNew ? "Create driver" : "Save changes")
                            </button>
@@ -185,7 +190,9 @@

    private async Task SaveAsync()
    {
-        _busy = true; _saveError = null;
+        _saveError = DriverConfigSaveGate.Validate(_driverType, _driverConfigJson);
+        if (_saveError is not null) return;
+        _busy = true;
        try
        {
            UnsMutationResult result;
@@ -59,15 +59,15 @@
        <div class="row">
            <div class="col-md-4 mb-3">
                <label class="form-label" for="hhInspectPw">INSPECT password ref</label>
-                <InputText id="hhInspectPw" @bind-Value="_m.InspectPassword" @bind-Value:after="EmitAsync" class="form-control form-control-sm mono" placeholder="secret:lsv2/inspect" />
+                <InputText id="hhInspectPw" @bind-Value="_m.InspectPassword" @bind-Value:after="EmitAsync" class="form-control form-control-sm mono" placeholder="secret:heidenhain/inspect" />
            </div>
            <div class="col-md-4 mb-3">
                <label class="form-label" for="hhDncPw">DNC password ref</label>
-                <InputText id="hhDncPw" @bind-Value="_m.DncPassword" @bind-Value:after="EmitAsync" class="form-control form-control-sm mono" placeholder="secret:lsv2/dnc" />
+                <InputText id="hhDncPw" @bind-Value="_m.DncPassword" @bind-Value:after="EmitAsync" class="form-control form-control-sm mono" placeholder="secret:heidenhain/dnc" />
            </div>
            <div class="col-md-4 mb-3">
                <label class="form-label" for="hhDataPw">DATA password ref</label>
-                <InputText id="hhDataPw" @bind-Value="_m.DataPassword" @bind-Value:after="EmitAsync" class="form-control form-control-sm mono" placeholder="env:LSV2_DATA_PASSWORD" />
+                <InputText id="hhDataPw" @bind-Value="_m.DataPassword" @bind-Value:after="EmitAsync" class="form-control form-control-sm mono" placeholder="env:OTOPCUA_HEIDENHAIN_DATA_PASSWORD" />
            </div>
        </div>
        <div class="form-text">References only: blank, <span class="mono">secret:&lt;name&gt;</span> (node secret store) or <span class="mono">env:&lt;VAR&gt;</span>. Resolved at each login; a literal password is refused.</div>
@@ -59,6 +59,7 @@
                                        </td>
                                        <td>
                                            <select class="form-select form-select-sm" value="@row.AccessLevel"
+                                                    disabled="@RawTagRowDefaults.IsReadOnlyDriver(DriverType)"
                                                    @onchange="@(e => row.AccessLevel = ParseAccess(e.Value))">
                                                <option value="@TagAccessLevel.Read">Read</option>
                                                <option value="@TagAccessLevel.ReadWrite">ReadWrite</option>
@@ -209,7 +210,11 @@
    private IDictionary<string, object> BuildEditorParameters(RowModel row) => new Dictionary<string, object>
    {
        ["ConfigJson"] = row.TagConfig,
-        ["ConfigJsonChanged"] = EventCallback.Factory.Create<string>(this, v => row.TagConfig = v),
+        ["ConfigJsonChanged"] = EventCallback.Factory.Create<string>(this, v =>
+        {
+            row.TagConfig = v;
+            RawTagRowDefaults.Apply(DriverType, row.TagConfig, a => row.AccessLevel = a, d => row.DataType = d);
+        }),
        ["DriverType"] = DriverType,
        ["GetDriverConfigJson"] = (Func<string>)(() => _mergedConfigJson ?? "{}"),
    };
@@ -255,6 +260,7 @@
            var failed = 0;
            foreach (var row in _rows)
            {
+                RawTagRowDefaults.Apply(DriverType, row.TagConfig, a => row.AccessLevel = a, d => row.DataType = d);
                var input = new RawTagInput(
                    row.Name,
                    row.DataType,
@@ -57,10 +57,18 @@
                            <div class="row">
                                <div class="col-md-6 mb-3">
                                    <label class="form-label" for="raw-tag-access">Access level</label>
-                                    <InputSelect id="raw-tag-access" @bind-Value="_form.AccessLevel" class="form-select form-select-sm">
-                                        <option value="@TagAccessLevel.Read">Read</option>
-                                        <option value="@TagAccessLevel.ReadWrite">ReadWrite</option>
-                                    </InputSelect>
+                                    @if (IsReadOnlyDriver)
+                                    {
+                                        <input id="raw-tag-access" class="form-control form-control-sm" value="Read" disabled readonly />
+                                        <div class="form-text">This driver is read-only.</div>
+                                    }
+                                    else
+                                    {
+                                        <InputSelect id="raw-tag-access" @bind-Value="_form.AccessLevel" class="form-select form-select-sm">
+                                            <option value="@TagAccessLevel.Read">Read</option>
+                                            <option value="@TagAccessLevel.ReadWrite">ReadWrite</option>
+                                        </InputSelect>
+                                    }
                                </div>
                                <div class="col-md-6 mb-3">
                                    <label class="form-label">WriteIdempotent</label>
@@ -226,7 +234,11 @@
        {
            ["ConfigJson"] = _form.TagConfig,
            ["ConfigJsonChanged"] = _configJsonChanged ??=
-                EventCallback.Factory.Create<string>(this, v => _form.TagConfig = v),
+                EventCallback.Factory.Create<string>(this, v =>
+                {
+                    _form.TagConfig = v;
+                    RawTagRowDefaults.Apply(DriverType, _form.TagConfig, a => _form.AccessLevel = a, d => _form.DataType = d);
+                }),
            ["DriverType"] = DriverType ?? "",
            // Live accessor, not a snapshot: the picker reads it when the operator clicks Browse.
            ["GetDriverConfigJson"] = _getDriverConfigJson ??= () => _mergedConfigJson ?? "{}",
@@ -258,6 +270,7 @@
                return;
            }

+            RawTagRowDefaults.Apply(DriverType, _form.TagConfig, a => _form.AccessLevel = a, d => _form.DataType = d);
            var input = new RawTagInput(
                _form.Name,
                _form.DataType,
@@ -283,6 +296,8 @@
        }
    }

+    private bool IsReadOnlyDriver => RawTagRowDefaults.IsReadOnlyDriver(DriverType);
+
    private async Task CloseAsync()
    {
        Visible = false;
@@ -0,0 +1,29 @@
+using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
+using ZB.MOM.WW.OtOpcUa.Driver.Heidenhain;
+
+namespace ZB.MOM.WW.OtOpcUa.AdminUI.Uns;
+
+/// <summary>
+///     Server-side refusal of a DriverConfig / DeviceConfig that must never reach the config database. Called by
+///     <see cref="RawTreeService"/> on every driver/device create + update (the authority — a modal that skips
+///     its own check, the API or a hand-crafted circuit message still hits it) and by the driver/device modals to
+///     block Save while the form is invalid.
+/// </summary>
+/// <remarks>
+///     Today it carries one rule: a Heidenhain password field must be blank or a reference in the Heidenhain
+///     namespace (<see cref="HeidenhainOptionsParser.DescribeCredentialProblems"/>) — otherwise a literal password
+///     would be persisted in a draft (and replicated) even though the deploy gate would later refuse it.
+/// </remarks>
+public static class DriverConfigSaveGate
+{
+    /// <summary>The reason a config may not be saved for a driver type, or null when it may.</summary>
+    /// <param name="driverType">The owning driver's type.</param>
+    /// <param name="configJson">The DriverConfig or DeviceConfig JSON about to be stored.</param>
+    /// <returns>The refusal (never containing a credential value), or null.</returns>
+    public static string? Validate(string? driverType, string? configJson)
+    {
+        if (!string.Equals(driverType, DriverTypeNames.Heidenhain, StringComparison.OrdinalIgnoreCase)) return null;
+        var problems = HeidenhainOptionsParser.DescribeCredentialProblems(configJson);
+        return problems.Count == 0 ? null : string.Join(" ", problems);
+    }
+}
@@ -0,0 +1,32 @@
+using ZB.MOM.WW.OtOpcUa.Configuration.Enums;
+using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
+using ZB.MOM.WW.OtOpcUa.Driver.Heidenhain;
+
+namespace ZB.MOM.WW.OtOpcUa.AdminUI.Uns;
+
+/// <summary>
+///     Authoring-time defaults for a raw tag ROW that a driver dictates (lmxopcua#803): for a Heidenhain tag the
+///     access level is always Read and <c>Tag.DataType</c> follows the address (catalog type, or the binding's
+///     declared type). Used by the tag modal and the manual-entry grid so the operator never has to pick values the
+///     server (<c>RawTreeService</c>) and the deploy gate would refuse.
+/// </summary>
+public static class RawTagRowDefaults
+{
+    /// <summary>True for a driver whose tags can never be writable.</summary>
+    /// <param name="driverType">The driver type.</param>
+    /// <returns>Whether the access level is fixed at Read.</returns>
+    public static bool IsReadOnlyDriver(string? driverType) =>
+        string.Equals(driverType, DriverTypeNames.Heidenhain, StringComparison.OrdinalIgnoreCase);
+
+    /// <summary>Applies the driver-dictated access level and data type for a row, when the driver dictates them.</summary>
+    /// <param name="driverType">The driver type.</param>
+    /// <param name="tagConfig">The row's TagConfig.</param>
+    /// <param name="setAccess">Sets the row's access level.</param>
+    /// <param name="setDataType">Sets the row's <c>Tag.DataType</c>.</param>
+    public static void Apply(string? driverType, string? tagConfig, Action<TagAccessLevel> setAccess, Action<string> setDataType)
+    {
+        if (!IsReadOnlyDriver(driverType)) return;
+        setAccess(TagAccessLevel.Read);
+        if (HeidenhainTagRules.ExpectedTagDataType(tagConfig) is { } dataType) setDataType(dataType);
+    }
+}
@@ -483,6 +483,8 @@ public sealed class RawTreeService(IDbContextFactory<OtOpcUaConfigDbContext> dbF
        if (nameError is not null) return new UnsMutationResult(false, nameError);
        if (string.IsNullOrWhiteSpace(driverType))
            return new UnsMutationResult(false, "Pick a driver type.");
+        if (DriverConfigSaveGate.Validate(driverType, driverConfigJson) is { } configRefusal)
+            return new UnsMutationResult(false, configRefusal);

        await using var db = await dbFactory.CreateDbContextAsync(ct);

@@ -619,6 +621,8 @@ public sealed class RawTreeService(IDbContextFactory<OtOpcUaConfigDbContext> dbF
        var owner = await db.DriverInstances.FirstOrDefaultAsync(d => d.DriverInstanceId == driverInstanceId, ct);
        if (owner is null)
            return new UnsMutationResult(false, $"Driver '{driverInstanceId}' not found.");
+        if (DriverConfigSaveGate.Validate(owner.DriverType, deviceConfigJson) is { } configRefusal)
+            return new UnsMutationResult(false, configRefusal);

        // MTConnect owns one agent client per driver — mirror the deploy gate
        // (DraftValidator.ValidateMTConnectSingleDevice) so the second Device is refused where it is authored.
@@ -855,6 +859,8 @@ public sealed class RawTreeService(IDbContextFactory<OtOpcUaConfigDbContext> dbF

        if (await ValidateTagConfigAsync(db, deviceId, tagConfig, ct) is { } configError)
            return new UnsMutationResult(false, configError);
+        if (TagRowProblem(await ResolveDriverTypeAsync(db, deviceId, ct), input, tagConfig) is { } rowError)
+            return new UnsMutationResult(false, rowError);

        var id = NewId("TAG");
        db.Tags.Add(new Tag
@@ -896,6 +902,8 @@ public sealed class RawTreeService(IDbContextFactory<OtOpcUaConfigDbContext> dbF

        if (await ValidateTagConfigAsync(db, entity.DeviceId, tagConfig, ct) is { } configError)
            return new UnsMutationResult(false, configError);
+        if (TagRowProblem(await ResolveDriverTypeAsync(db, entity.DeviceId, ct), input, tagConfig) is { } rowError)
+            return new UnsMutationResult(false, rowError);

        db.Entry(entity).Property(e => e.RowVersion).OriginalValue = rowVersion;
        entity.Name = input.Name;
@@ -989,6 +997,12 @@ public sealed class RawTreeService(IDbContextFactory<OtOpcUaConfigDbContext> dbF
                continue;
            }

+            if (TagRowProblem(driverType, row.Tag, tagConfig) is { } rowError)
+            {
+                errors.Add($"{rowLabel}: {rowError}");
+                continue;
+            }
+
            if (!takenNames.Add((groupId, row.Tag.Name)))
            {
                errors.Add($"{rowLabel}: a tag named '{row.Tag.Name}' already exists at that path.");
@@ -1102,6 +1116,8 @@ public sealed class RawTreeService(IDbContextFactory<OtOpcUaConfigDbContext> dbF

        var entity = await db.DriverInstances.FirstOrDefaultAsync(d => d.DriverInstanceId == driverInstanceId, ct);
        if (entity is null) return new UnsMutationResult(false, "Row no longer exists.");
+        if (DriverConfigSaveGate.Validate(entity.DriverType, driverConfigJson) is { } configRefusal)
+            return new UnsMutationResult(false, configRefusal);

        // Sibling uniqueness in the same scope RenameDriverAsync uses (cluster + folder).
        if (await db.DriverInstances.AnyAsync(
@@ -1162,6 +1178,8 @@ public sealed class RawTreeService(IDbContextFactory<OtOpcUaConfigDbContext> dbF

        var entity = await db.Devices.FirstOrDefaultAsync(dev => dev.DeviceId == deviceId, ct);
        if (entity is null) return new UnsMutationResult(false, "Row no longer exists.");
+        if (DriverConfigSaveGate.Validate(await ResolveDriverTypeAsync(db, deviceId, ct), deviceConfigJson) is { } configRefusal)
+            return new UnsMutationResult(false, configRefusal);

        if (await db.Devices.AnyAsync(
                dev => dev.DriverInstanceId == entity.DriverInstanceId && dev.Name == name && dev.DeviceId != deviceId, ct))
@@ -1275,6 +1293,16 @@ public sealed class RawTreeService(IDbContextFactory<OtOpcUaConfigDbContext> dbF
    /// <param name="tagConfig">The normalised TagConfig JSON about to be persisted.</param>
    /// <param name="ct">Cancellation token.</param>
    /// <returns>An operator-facing error, or <see langword="null"/> when the config is acceptable.</returns>
+    /// <summary>
+    ///     Driver-specific Tag-ROW rules beyond the TagConfig (lmxopcua#803): a Heidenhain tag must be Read-only
+    ///     and carry the data type the driver publishes — the same rule the deploy gate applies.
+    /// </summary>
+    private static string? TagRowProblem(string? driverType, RawTagInput input, string tagConfig) =>
+        string.Equals(driverType, DriverTypeNames.Heidenhain, StringComparison.OrdinalIgnoreCase)
+            ? Driver.Heidenhain.HeidenhainTagRules.DescribeRowProblem(
+                input.AccessLevel == Configuration.Enums.TagAccessLevel.ReadWrite, input.DataType, tagConfig)
+            : null;
+
    private static async Task<string?> ValidateTagConfigAsync(
        OtOpcUaConfigDbContext db, string deviceId, string tagConfig, CancellationToken ct)
    {