From 54f58c583ca39626b321a48119d4391ec860c6ff Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 10 Nov 2026 23:54:32 -0500 Subject: [PATCH] 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. --- docs/drivers/Heidenhain.md | 55 +++++-- .../Validation/HeidenhainDeployChecks.cs | 13 ++ .../HeidenhainTagRules.cs | 54 +++++++ .../DeviceForms/HeidenhainDeviceForm.razor | 6 +- .../Shared/Drivers/DeviceModal.razor | 11 +- .../Shared/Drivers/DriverConfigModal.razor | 11 +- .../Drivers/Forms/HeidenhainDriverForm.razor | 6 +- .../Shared/Raw/RawManualTagEntryModal.razor | 8 +- .../Components/Shared/Raw/RawTagModal.razor | 25 ++- .../Uns/DriverConfigSaveGate.cs | 29 ++++ .../Uns/RawTagRowDefaults.cs | 32 ++++ .../Uns/RawTreeService.cs | 28 ++++ .../DraftValidatorHeidenhainTests.cs | 23 ++- .../Uns/HeidenhainAuthoringTests.cs | 8 +- .../Uns/HeidenhainSavePathTests.cs | 151 ++++++++++++++++++ 15 files changed, 427 insertions(+), 33 deletions(-) create mode 100644 src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Heidenhain.Contracts/HeidenhainTagRules.cs create mode 100644 src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/DriverConfigSaveGate.cs create mode 100644 src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTagRowDefaults.cs create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/HeidenhainSavePathTests.cs diff --git a/docs/drivers/Heidenhain.md b/docs/drivers/Heidenhain.md index a30864d84..978088a90 100644 --- a/docs/drivers/Heidenhain.md +++ b/docs/drivers/Heidenhain.md @@ -16,9 +16,14 @@ Three independent layers, each sufficient on its own for its concern: 1. **No write capability.** `HeidenhainDriver` does not implement `IWritable`; `DriverInstanceActor` refuses every OPC write before it reaches the driver (`HeidenhainDriverEndToEndTests`). -2. **No writable nodes.** The deploy gate `HeidenhainTagWritable` refuses a `ReadWrite` access level on any tag - under a Heidenhain driver, so no node is ever advertised writable. A `"writable": true` TagConfig key is - rejected by the tag reader and dropped by the typed editor. +2. **No writable nodes.** A `ReadWrite` access level is refused at **authoring** — `RawTreeService` + create/update/import (so the tag modal, manual entry, CSV import and browse commit), where the modal and the + manual-entry grid also fix the access level at Read — and again by the deploy gate `HeidenhainTagWritable`, so no + node is ever advertised writable. A `"writable": true` TagConfig key is rejected by the tag reader and dropped by + the typed editor. The same paths tie `Tag.DataType` to what the driver publishes (the catalog type for a fixed + address — `Float64` ⇒ `Double` — or the binding's declared type): the modals default it from the address, the + save path refuses a mismatch, and the deploy gate `HeidenhainTagDataTypeMismatch` catches anything older + (one rule, `HeidenhainTagRules`). 3. **Payload-aware outbound allowlist** (`Lsv2ReadOnlyPolicy`). `Lsv2Codec.Frame` — the only frame builder — checks command **and** payload immediately before any byte is written. There is no raw-command API (`Lsv2Client`'s public surface is pinned by a reflection test) and no configuration knob that widens it. @@ -70,12 +75,31 @@ Device config (one `/raw` Device per control, AdminUI **HeidenhainDeviceForm**): `Port` (19000), optional per-device `InspectPassword`/`DncPassword`/`DataPassword` overrides. A device is validated only once it carries tags, so an unconfigured default `Device1` does not fault the driver. -**Credentials.** A password field is blank, `secret:` (the node's encrypted secret store, via -`ISecretResolver`) or `env:`. It is resolved **at each login** and never held in options or session -state. A literal is refused by the parser, both AdminUI forms and the deploy gate `HeidenhainCredentialPersisted` -(the message never echoes the value). Fail-closed: an absent secret / unset variable fails the login rather than -sending the reference text. Plain LSV2 is unencrypted — reach the control over an externally managed secure -network; there is no SSH subsystem. +**Credentials — namespaced references only.** A password field is one of: + +| Form | Resolved from | +|---|---| +| blank | no password | +| `secret:heidenhain/` (e.g. `secret:heidenhain/line3/inspect`; `[A-Za-z0-9._-]` segments, no `..`) | the node's encrypted secret store (`ISecretResolver`), same slash-scoped naming as `galaxy//apikey` | +| `env:OTOPCUA_HEIDENHAIN_` (exact upper-case prefix, `NAME` = `[A-Z0-9_]+`) | the process environment | + +**Why namespaced:** the reference is resolved on the node (for Test Connect: on the admin node) and sent in +**cleartext** in `A_LG` to whatever Host the author configured. An unscoped reference would let anyone who can edit +a driver exfiltrate `env:ConnectionStrings__ConfigDb`, `env:ServerHistorian__ApiKey` or any other secret by pointing +Host at their own machine. Anything outside the namespace is refused by **every** layer: the parser +(`HeidenhainOptionsParser`, so the factory and the probe), the resolver **again at resolve time** +(`HeidenhainCredentials`, fail-closed independently of earlier validation), the AdminUI **save path** +(`RawTreeService` create/update driver + device refuse the config server-side via `DriverConfigSaveGate`, and the +driver/device modals disable Save while it is invalid — so a literal never reaches the config database), and the +deploy gate `HeidenhainCredentialPersisted`. Messages never echo the value. + +Resolved **at each login**, never held in options or session state. Fail-closed: an absent secret or unset +variable fails the login rather than sending the reference text. An unusable **INSPECT** credential fails the +connect (INSPECT is required). An unusable **optional** DNC/DATA credential — unresolvable, or a password the LSV2 +login payload cannot carry (NUL, non-Latin-1, > 247 bytes) — never fails the session: the login is not sent, the +reads that need that role read `BadConfigurationError` (capability `ConfigurationError`), and INSPECT telemetry +continues. Plain LSV2 is unencrypted — reach the control over an externally managed secure network; there is no +SSH subsystem. ## Addresses (TagConfig) @@ -134,6 +158,7 @@ At most 256 PLC/DATA bindings per control (excess are skipped and reported on `/ | Binding type mismatch | `BadTypeMismatch` | | Out-of-region binding, alarm overflow | `BadOutOfRange` / `InvalidData` (never a truncated list) | | Disabled feature (telemetry off, timers off, DATA off) | `BadNotSupported` / `Disabled` | +| Unusable optional DNC/DATA credential, or a request the read-only policy refuses | `BadConfigurationError` / `ConfigurationError` — that role's reads only; never sent; the session stays up | | Not yet acquired | `BadWaitingForInitialData` | | Transport lost | **every** remote value of that control `BadNoCommunication` at once; capabilities `CommunicationLost` | @@ -150,8 +175,14 @@ overrides, axes, tool, the whole alarm walk) and aliased custom addresses are re **generation**; a batch returning after its session was retired can never overwrite the outage invalidation. - **Reads are cache-only** (`ReadAsync` never touches the wire); subscriptions push on (value, **status**) change, so a control returning with identical readings still re-publishes Good. -- Comms loss → all-value invalidation → bounded exponential reconnect (`Reconnect.*`). Unlike the Java base, an - initial connect failure also retries (no manual restart). +- Comms loss → all-value invalidation → bounded exponential reconnect (`Reconnect.*`, clamped to 100 ms–10 min). + The backoff resets only after a session **published a poll batch** — a control that accepts login + version and + then drops is backed off, not re-dialled every `InitialDelayMs`. Unlike the Java base, an initial connect + failure also retries (no manual restart). The loop survives any unexpected fault (logged, retired, backed off); + JSON-`null` config sections are normalised to defaults by the parser. +- Subscriptions: the initial push and later publications are ordered under one lock, so a subscriber never + receives an older value after a newer one. A re-init disposes every control loop first; a late start of a + disposed loop is a no-op. - **Health:** `Degraded` only while a control's session is down (connect failing / lost), with `LastSuccessfulRead` frozen — the conjunction the #527 comms-loss watchdog needs. Per-tag refusals, denials and decode errors are quality, not health (else the watchdog would reconnect-loop a control that merely denies one @@ -174,7 +205,7 @@ session to the first device: INSPECT login + `R_VR`. A bare TCP listener does no - `Configuration.Tests/DraftValidatorHeidenhainTests`, `AdminUI.Tests/Uns/HeidenhainAuthoringTests` + the fleet parity suites. - **LiveIntegration** (`--filter Category=LiveIntegration`, skipped unless `HEIDENHAIN_LSV2_HOST` is set; optional - `HEIDENHAIN_LSV2_PORT`, `HEIDENHAIN_LSV2_INSPECT_PASSWORD`): read-only INSPECT identity/selector/layout survey and a + `HEIDENHAIN_LSV2_PORT`, `OTOPCUA_HEIDENHAIN_INSPECT_PASSWORD`): read-only INSPECT identity/selector/layout survey and a driver end-to-end status poll. Record its output as .NET commissioning evidence. ## Hardware-gated — not yet proven diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/HeidenhainDeployChecks.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/HeidenhainDeployChecks.cs index 0518155e8..4b134c8ba 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/HeidenhainDeployChecks.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/HeidenhainDeployChecks.cs @@ -30,6 +30,9 @@ public static class HeidenhainDeployChecks /// A literal password in a Heidenhain DriverConfig/DeviceConfig. public const string CredentialCode = "HeidenhainCredentialPersisted"; + /// A Heidenhain tag whose Tag.DataType disagrees with the type the driver publishes. + public const string DataTypeCode = "HeidenhainTagDataTypeMismatch"; + /// Invalid Heidenhain settings or endpoint. public const string ConfigCode = "HeidenhainConfigInvalid"; @@ -56,6 +59,16 @@ public static class HeidenhainDeployChecks tag.TagId)); } + foreach (var tag in tags) + { + var expected = HeidenhainTagRules.ExpectedTagDataType(tag.TagConfig); + if (expected is null || string.Equals(expected, tag.DataType, StringComparison.OrdinalIgnoreCase)) continue; + errors.Add(new(DataTypeCode, + $"Tag '{tag.Name}' on Heidenhain driver '{driver.Name}' has data type {tag.DataType}, but the driver " + + $"publishes {expected} for its address — the node would advertise a type its values never carry.", + tag.TagId)); + } + HeidenhainDriverOptions options; try { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Heidenhain.Contracts/HeidenhainTagRules.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Heidenhain.Contracts/HeidenhainTagRules.cs new file mode 100644 index 000000000..8e454d000 --- /dev/null +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Heidenhain.Contracts/HeidenhainTagRules.cs @@ -0,0 +1,54 @@ +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Heidenhain; + +/// +/// Rules about the Tag ROW (not its TagConfig) for a Heidenhain tag: the one authority shared by every AdminUI +/// authoring path (RawTagModal, manual entry, CSV import, browse commit — all through RawTreeService) +/// and the deploy gate, so authoring and deploy can never disagree. +/// +public static class HeidenhainTagRules +{ + /// + /// The Tag.DataType (OPC UA built-in type name) a Heidenhain tag must carry for the value the driver + /// publishes, or null when the TagConfig does not resolve (the TagConfig gate reports that). + /// + /// The TagConfig JSON. + /// e.g. Int32, Double, String; null when unresolvable. + public static string? ExpectedTagDataType(string? tagConfig) => + HeidenhainTagDefinitionFactory.TryFromTagConfig("rule", "", tagConfig, out var def, out _) + ? OpcTypeName(def.DataType) + : null; + + /// The OPC UA built-in type name (the Tag.DataType vocabulary) for a published data type. + /// The driver data type. + /// The type name. + public static string OpcTypeName(DriverDataType type) => type switch + { + DriverDataType.Boolean => "Boolean", + DriverDataType.Int16 => "Int16", + DriverDataType.Int32 => "Int32", + DriverDataType.Int64 => "Int64", + DriverDataType.Float64 => "Double", + DriverDataType.String => "String", + _ => type.ToString(), + }; + + /// + /// Why a Heidenhain tag row may not be saved / deployed: a writable access level (the driver is read-only), + /// or a Tag.DataType that disagrees with what the driver publishes for its address. + /// + /// Whether the row's access level is ReadWrite. + /// The row's Tag.DataType. + /// The row's TagConfig. + /// The reason, or null when the row is acceptable. + public static string? DescribeRowProblem(bool readWrite, string? tagDataType, string? tagConfig) + { + if (readWrite) + return "Heidenhain tags are read-only — set the access level to Read (the driver never writes to the control)."; + var expected = ExpectedTagDataType(tagConfig); + if (expected is not null && !string.Equals(expected, tagDataType, StringComparison.OrdinalIgnoreCase)) + return $"Data type {tagDataType} does not match what the driver publishes for this address ({expected})."; + return null; + } +} diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DeviceForms/HeidenhainDeviceForm.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DeviceForms/HeidenhainDeviceForm.razor index ecaeade4b..ec8829186 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DeviceForms/HeidenhainDeviceForm.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DeviceForms/HeidenhainDeviceForm.razor @@ -27,15 +27,15 @@
- +
- +
- +
Blank = use the driver-level reference. secret:<name> resolves from the node's secret store, env:<VAR> from the environment, at each login. A literal password is never stored.
diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DeviceModal.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DeviceModal.razor index eaebf2068..fb380aaf8 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DeviceModal.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DeviceModal.razor @@ -62,6 +62,10 @@ + @if (DriverConfigSaveGate.Validate(_driverType, _deviceConfigJson) is { } gateError) + { +
@gateError
+ } @if (_saveError is not null) {
@_saveError
@@ -73,7 +77,8 @@ @if (_loadError is null) { - @@ -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; diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverConfigModal.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverConfigModal.razor index 8c3e7047a..3a415b596 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverConfigModal.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverConfigModal.razor @@ -63,6 +63,10 @@

} + @if (DriverConfigSaveGate.Validate(_driverType, _driverConfigJson) is { } gateError) + { +
@gateError
+ } @if (_saveError is not null) {
@_saveError
@@ -74,7 +78,8 @@ @if (_loadError is null) { - @@ -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; diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/HeidenhainDriverForm.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/HeidenhainDriverForm.razor index 6c90e3753..ee0e6b005 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/HeidenhainDriverForm.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/HeidenhainDriverForm.razor @@ -59,15 +59,15 @@
- +
- +
- +
References only: blank, secret:<name> (node secret store) or env:<VAR>. Resolved at each login; a literal password is refused.
diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Raw/RawManualTagEntryModal.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Raw/RawManualTagEntryModal.razor index 05ef3dbb7..abfe80c7c 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Raw/RawManualTagEntryModal.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Raw/RawManualTagEntryModal.razor @@ -59,6 +59,7 @@ +
This driver is read-only.
+ } + else + { + + + + + }
@@ -226,7 +234,11 @@ { ["ConfigJson"] = _form.TagConfig, ["ConfigJsonChanged"] = _configJsonChanged ??= - EventCallback.Factory.Create(this, v => _form.TagConfig = v), + EventCallback.Factory.Create(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; diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/DriverConfigSaveGate.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/DriverConfigSaveGate.cs new file mode 100644 index 000000000..9b46518e7 --- /dev/null +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/DriverConfigSaveGate.cs @@ -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; + +/// +/// Server-side refusal of a DriverConfig / DeviceConfig that must never reach the config database. Called by +/// 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. +/// +/// +/// Today it carries one rule: a Heidenhain password field must be blank or a reference in the Heidenhain +/// namespace () — otherwise a literal password +/// would be persisted in a draft (and replicated) even though the deploy gate would later refuse it. +/// +public static class DriverConfigSaveGate +{ + /// The reason a config may not be saved for a driver type, or null when it may. + /// The owning driver's type. + /// The DriverConfig or DeviceConfig JSON about to be stored. + /// The refusal (never containing a credential value), or null. + 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); + } +} diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTagRowDefaults.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTagRowDefaults.cs new file mode 100644 index 000000000..5718c756c --- /dev/null +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTagRowDefaults.cs @@ -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; + +/// +/// 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 Tag.DataType 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 (RawTreeService) and the deploy gate would refuse. +/// +public static class RawTagRowDefaults +{ + /// True for a driver whose tags can never be writable. + /// The driver type. + /// Whether the access level is fixed at Read. + public static bool IsReadOnlyDriver(string? driverType) => + string.Equals(driverType, DriverTypeNames.Heidenhain, StringComparison.OrdinalIgnoreCase); + + /// Applies the driver-dictated access level and data type for a row, when the driver dictates them. + /// The driver type. + /// The row's TagConfig. + /// Sets the row's access level. + /// Sets the row's Tag.DataType. + public static void Apply(string? driverType, string? tagConfig, Action setAccess, Action setDataType) + { + if (!IsReadOnlyDriver(driverType)) return; + setAccess(TagAccessLevel.Read); + if (HeidenhainTagRules.ExpectedTagDataType(tagConfig) is { } dataType) setDataType(dataType); + } +} diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs index 0a8a904fd..74172dfd7 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs @@ -483,6 +483,8 @@ public sealed class RawTreeService(IDbContextFactory 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 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 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 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 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 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 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 dbF /// The normalised TagConfig JSON about to be persisted. /// Cancellation token. /// An operator-facing error, or when the config is acceptable. + /// + /// 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. + /// + 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 ValidateTagConfigAsync( OtOpcUaConfigDbContext db, string deviceId, string tagConfig, CancellationToken ct) { diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorHeidenhainTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorHeidenhainTests.cs index a71303447..52de5a589 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorHeidenhainTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorHeidenhainTests.cs @@ -56,7 +56,7 @@ public sealed class DraftValidatorHeidenhainTests [Fact] public void Secret_and_env_references_are_accepted() - => Validate(Driver("""{"Access":{"InspectPassword":"secret:lsv2/inspect","DncPassword":"env:LSV2_DNC"}}"""), Dev(), T(TagAccessLevel.Read)) + => Validate(Driver("""{"Access":{"InspectPassword":"secret:heidenhain/inspect","DncPassword":"env:OTOPCUA_HEIDENHAIN_DNC"}}"""), Dev(), T(TagAccessLevel.Read)) .ShouldNotContain(e => e.Code.StartsWith("Heidenhain", StringComparison.Ordinal)); [Fact] @@ -66,6 +66,27 @@ public sealed class DraftValidatorHeidenhainTests Validate(Driver(), Dev("{}")).ShouldNotContain(e => e.Code.StartsWith("Heidenhain", StringComparison.Ordinal)); } + [Fact] + public void A_tag_data_type_that_disagrees_with_the_published_type_is_refused() + { + var tag = T(TagAccessLevel.Read); + tag.DataType = "Float"; + var e = Validate(Driver(), Dev(), tag).Single(x => x.Code == HeidenhainDeployChecks.DataTypeCode); + e.Context.ShouldBe("t1"); + e.Message.ShouldContain("Int32"); + var binding = T(TagAccessLevel.Read); + binding.TagConfig = """{"address":"PLC:DWORD:0","dataType":"Int64"}"""; + binding.DataType = "Int64"; + Validate(Driver(), Dev(), binding).ShouldNotContain(x => x.Code == HeidenhainDeployChecks.DataTypeCode); + } + + [Theory] + [InlineData("env:ConnectionStrings__ConfigDb")] + [InlineData("secret:galaxy/inst1/apikey")] + public void An_out_of_namespace_reference_is_refused(string reference) + => Validate(Driver("{\"Access\":{\"InspectPassword\":\"" + reference + "\"}}"), Dev(), T(TagAccessLevel.Read)) + .ShouldContain(e => e.Code == HeidenhainDeployChecks.CredentialCode); + [Fact] public void Out_of_range_settings_are_refused() => Validate(Driver("""{"Polling":{"MaxAlarms":999}}"""), Dev(), T(TagAccessLevel.Read)) diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/HeidenhainAuthoringTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/HeidenhainAuthoringTests.cs index 9d872e286..d7b59a66d 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/HeidenhainAuthoringTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/HeidenhainAuthoringTests.cs @@ -35,13 +35,13 @@ public sealed class HeidenhainAuthoringTests var authored = """{"Devices":[{"Host":"10.0.0.5","DeviceName":"m1"}],"Custom":42}"""; var m = HeidenhainDriverFormModel.FromJson(authored); m.Validate().ShouldBeNull(); - m.InspectPassword = "secret:lsv2/inspect"; + m.InspectPassword = "secret:heidenhain/inspect"; m.TimersEnabled = true; var json = m.ToJson(); var o = HeidenhainOptionsParser.Parse(json); HeidenhainOptionsParser.Validate(o).ShouldBeEmpty(); // the factory's ParseOptions gate o.Devices.Single().Host.ShouldBe("10.0.0.5"); - o.Access.InspectPassword.ShouldBe("secret:lsv2/inspect"); + o.Access.InspectPassword.ShouldBe("secret:heidenhain/inspect"); o.Polling.TimersEnabled.ShouldBeTrue(); JsonNode.Parse(json)!["Custom"]!.GetValue().ShouldBe(42); // no field is ever written as a number where the parser expects a name — there are no enums at all @@ -67,11 +67,11 @@ public sealed class HeidenhainAuthoringTests d.Validate().ShouldBeNull(); d.DataPassword = "plain"; d.Validate().ShouldNotBeNull(); - d.DataPassword = "env:LSV2_DATA"; + d.DataPassword = "env:OTOPCUA_HEIDENHAIN_DATA"; var json = d.ToJson(); JsonNode.Parse(json)!["Note"]!.GetValue().ShouldBe("keep"); var opts = HeidenhainOptionsParser.Parse($$"""{"Devices":[{{json}}]}"""); - opts.Devices.Single().DataPassword.ShouldBe("env:LSV2_DATA"); + opts.Devices.Single().DataPassword.ShouldBe("env:OTOPCUA_HEIDENHAIN_DATA"); HeidenhainDeviceModel.FromJson("{}").Validate().ShouldNotBeNull(); // host required } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/HeidenhainSavePathTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/HeidenhainSavePathTests.cs new file mode 100644 index 000000000..28d1f0a8c --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/HeidenhainSavePathTests.cs @@ -0,0 +1,151 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.AdminUI.Uns; +using ZB.MOM.WW.OtOpcUa.Configuration.Enums; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns; + +/// +/// #803 review findings 1 + 8 at the SAVE path (RawTreeService), not just the editor models: a literal or +/// out-of-namespace Heidenhain password never reaches the config DB, and a Heidenhain tag row is refused when it +/// is ReadWrite or carries a data type the driver does not publish — through every authoring entry point +/// (modal create/update, manual entry = CreateTagAsync, CSV/browse = ImportTagsAsync). +/// +[Trait("Category", "Unit")] +public sealed class HeidenhainSavePathTests +{ + private static CancellationToken Ct => TestContext.Current.CancellationToken; + + private static async Task<(RawTreeService Svc, string Db, string DriverId, string DeviceId)> HeidenhainAsync() + { + var name = RawTreeTestDb.SeedFresh(); + var svc = new RawTreeService(RawTreeTestDb.Factory(name)); + var created = await svc.CreateDriverAsync(RawTreeTestDb.ClusterId, null, "hh1", DriverTypeNames.Heidenhain, "{}", Ct); + created.Ok.ShouldBeTrue(created.Error); + using var db = RawTreeTestDb.CreateNamed(name); + var device = db.Devices.Single(d => d.DriverInstanceId == created.CreatedId); + return (svc, name, created.CreatedId!, device.DeviceId); + } + + // ------------------------------------------------------------------ finding 1: credentials never persisted + + [Theory] + [InlineData("""{"Access":{"InspectPassword":"hunter2"}}""")] + [InlineData("""{"access":{"dncpassword":"hunter2"}}""")] + [InlineData("""{"Access":{"DataPassword":"env:ConnectionStrings__ConfigDb"}}""")] + [InlineData("""{"Devices":[{"Host":"h","InspectPassword":"secret:galaxy/inst1/apikey"}]}""")] + public async Task CreateDriver_refuses_a_literal_or_out_of_namespace_password_and_persists_nothing(string config) + { + var name = RawTreeTestDb.SeedFresh(); + var svc = new RawTreeService(RawTreeTestDb.Factory(name)); + var result = await svc.CreateDriverAsync(RawTreeTestDb.ClusterId, null, "hh1", DriverTypeNames.Heidenhain, config, Ct); + result.Ok.ShouldBeFalse(); + result.Error!.ShouldNotContain("hunter2"); + using var db = RawTreeTestDb.CreateNamed(name); + db.DriverInstances.ShouldNotContain(d => d.Name == "hh1"); + } + + [Fact] + public async Task UpdateDriver_refuses_a_literal_password_and_keeps_the_stored_config() + { + var (svc, dbName, driverId, _) = await HeidenhainAsync(); + byte[] rv; + using (var db = RawTreeTestDb.CreateNamed(dbName)) rv = db.DriverInstances.Single(d => d.DriverInstanceId == driverId).RowVersion; + var result = await svc.UpdateDriverAsync(driverId, "hh1", """{"Access":{"InspectPassword":"hunter2"}}""", null, rv, Ct); + result.Ok.ShouldBeFalse(); + using var check = RawTreeTestDb.CreateNamed(dbName); + check.DriverInstances.Single(d => d.DriverInstanceId == driverId).DriverConfig.ShouldNotContain("hunter2"); + } + + [Fact] + public async Task CreateDevice_and_UpdateDevice_refuse_a_literal_password() + { + var (svc, dbName, driverId, deviceId) = await HeidenhainAsync(); + (await svc.CreateDeviceAsync(driverId, "cnc2", """{"Host":"10.0.0.5","DncPassword":"hunter2"}""", Ct)).Ok.ShouldBeFalse(); + byte[] rv; + using (var db = RawTreeTestDb.CreateNamed(dbName)) rv = db.Devices.Single(d => d.DeviceId == deviceId).RowVersion; + (await svc.UpdateDeviceAsync(deviceId, "Device1x", """{"Host":"10.0.0.5","DataPassword":"hunter2"}""", true, rv, Ct)).Ok.ShouldBeFalse(); + using var check = RawTreeTestDb.CreateNamed(dbName); + check.Devices.Where(d => d.DriverInstanceId == driverId).ShouldAllBe(d => !d.DeviceConfig.Contains("hunter2")); + } + + [Fact] + public async Task Namespaced_references_save_and_other_drivers_are_unaffected() + { + var (svc, _, driverId, _) = await HeidenhainAsync(); + (await svc.CreateDeviceAsync(driverId, "cnc2", + """{"Host":"10.0.0.5","InspectPassword":"secret:heidenhain/cnc2/inspect","DncPassword":"env:OTOPCUA_HEIDENHAIN_DNC"}""", Ct)).Ok.ShouldBeTrue(); + // A Modbus driver's config is none of this gate's business. + (await svc.CreateDriverAsync(RawTreeTestDb.ClusterId, null, "mb9", DriverTypeNames.Modbus, """{"InspectPassword":"x"}""", Ct)).Ok.ShouldBeTrue(); + } + + [Fact] + public void The_modal_save_gate_is_the_same_rule() + { + DriverConfigSaveGate.Validate(DriverTypeNames.Heidenhain, """{"Access":{"InspectPassword":"hunter2"}}""").ShouldNotBeNull(); + DriverConfigSaveGate.Validate(DriverTypeNames.Heidenhain, """{"Access":{"InspectPassword":"secret:heidenhain/a"}}""").ShouldBeNull(); + DriverConfigSaveGate.Validate(DriverTypeNames.Modbus, """{"Access":{"InspectPassword":"hunter2"}}""").ShouldBeNull(); + } + + // ------------------------------------------------------------------ finding 8: tag rows + + private static RawTagInput Row(string name, string dataType, TagAccessLevel access, string config) => + new(name, dataType, access, WriteIdempotent: false, PollGroupId: null, config); + + private const string Exec = """{"address":"Status/ExecutionState"}"""; + + [Fact] + public async Task CreateTag_refuses_ReadWrite_and_a_mismatched_data_type() + { + var (svc, _, _, deviceId) = await HeidenhainAsync(); + (await svc.CreateTagAsync(deviceId, null, Row("a", "Int32", TagAccessLevel.ReadWrite, Exec), Ct)).Ok.ShouldBeFalse(); + (await svc.CreateTagAsync(deviceId, null, Row("b", "Float", TagAccessLevel.Read, Exec), Ct)).Ok.ShouldBeFalse(); + (await svc.CreateTagAsync(deviceId, null, Row("c", "Int32", TagAccessLevel.Read, Exec), Ct)).Ok.ShouldBeTrue(); + (await svc.CreateTagAsync(deviceId, null, + Row("d", "Double", TagAccessLevel.Read, """{"address":"Axes/Positions","isArray":true}"""), Ct)).Ok.ShouldBeTrue(); + (await svc.CreateTagAsync(deviceId, null, + Row("e", "Int64", TagAccessLevel.Read, """{"address":"PLC:DWORD:0","dataType":"Int64"}"""), Ct)).Ok.ShouldBeTrue(); + (await svc.CreateTagAsync(deviceId, null, + Row("f", "Int32", TagAccessLevel.Read, """{"address":"PLC:DWORD:0","dataType":"Int64"}"""), Ct)).Ok.ShouldBeFalse(); + } + + [Fact] + public async Task UpdateTag_refuses_ReadWrite() + { + var (svc, dbName, _, deviceId) = await HeidenhainAsync(); + var created = await svc.CreateTagAsync(deviceId, null, Row("a", "Int32", TagAccessLevel.Read, Exec), Ct); + byte[] rv; + using (var db = RawTreeTestDb.CreateNamed(dbName)) rv = db.Tags.Single(t => t.TagId == created.CreatedId).RowVersion; + (await svc.UpdateTagAsync(created.CreatedId!, Row("a", "Int32", TagAccessLevel.ReadWrite, Exec), rv, Ct)).Ok.ShouldBeFalse(); + using var check = RawTreeTestDb.CreateNamed(dbName); + check.Tags.Single(t => t.TagId == created.CreatedId).AccessLevel.ShouldBe(TagAccessLevel.Read); + } + + [Fact] + public async Task ImportTags_names_the_ReadWrite_and_mismatched_rows_and_inserts_nothing() + { + var (svc, dbName, _, deviceId) = await HeidenhainAsync(); + var outcome = await svc.ImportTagsAsync(deviceId, + [ + new RawTagImportRow(null, Row("ok", "Int32", TagAccessLevel.Read, Exec)), + new RawTagImportRow(null, Row("rw", "Int32", TagAccessLevel.ReadWrite, Exec)), + new RawTagImportRow(null, Row("dt", "String", TagAccessLevel.Read, Exec)), + ], Ct); + outcome.Errors.Count.ShouldBe(2); + using var db = RawTreeTestDb.CreateNamed(dbName); + db.Tags.ShouldNotContain(t => t.DeviceId == deviceId); + } + + [Fact] + public void Row_defaults_force_Read_and_derive_the_data_type() + { + var access = TagAccessLevel.ReadWrite; + var dataType = "Float"; + RawTagRowDefaults.Apply(DriverTypeNames.Heidenhain, """{"address":"Overrides/Feed"}""", a => access = a, d => dataType = d); + access.ShouldBe(TagAccessLevel.Read); + dataType.ShouldBe("Double"); + RawTagRowDefaults.Apply(DriverTypeNames.Modbus, Exec, a => access = TagAccessLevel.ReadWrite, d => dataType = "x"); + dataType.ShouldBe("Double"); // untouched for other drivers + } +}