From d844405cec7174c48d1a05607938bb6cfd57463f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 02:19:35 -0400 Subject: [PATCH] fix(templates): hoist (DataType,ElementDataType,Value) attribute validation into TemplateService (#92) --- .../TemplateService.cs | 68 +++++++ .../TemplateServiceTests.cs | 171 ++++++++++++++++++ 2 files changed, 239 insertions(+) diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/TemplateService.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/TemplateService.cs index c71deae9..9b97fe20 100644 --- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/TemplateService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/TemplateService.cs @@ -2,6 +2,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Entities.Templates; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; using ZB.MOM.WW.ScadaBridge.Commons.Types; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; namespace ZB.MOM.WW.ScadaBridge.TemplateEngine; @@ -266,6 +267,16 @@ public class TemplateService string user, CancellationToken cancellationToken = default) { + // Defense-in-depth (#92): hoist the (DataType, ElementDataType, Value) + // guard — previously only enforced on the CLI/API path in + // ManagementActor.ValidateAttributeTypes — into the service so the + // Central UI's direct TemplateService write path shares one server-side + // check. Mirrors ValidateAttributeTypes exactly, reusing AttributeValueCodec. + var typeError = ValidateAttributeTypes( + attribute.Name, attribute.DataType, attribute.ElementDataType, attribute.Value); + if (typeError != null) + return Result.Failure(typeError); + var template = await _repository.GetTemplateByIdAsync(templateId, cancellationToken); if (template == null) return Result.Failure($"Template with ID {templateId} not found."); @@ -312,6 +323,17 @@ public class TemplateService if (existing == null) return Result.Failure($"Attribute with ID {attributeId} not found."); + // Defense-in-depth (#92): hoist the (DataType, ElementDataType, Value) + // guard out of ManagementActor.ValidateAttributeTypes so the Central UI's + // direct TemplateService write path shares one server-side check. DataType + // and ElementDataType are fixed by the defining level (not copied from the + // proposed attribute below), so the effective persisted triple is the + // existing type pairing with the proposed value. Reuses AttributeValueCodec. + var typeError = ValidateAttributeTypes( + existing.Name, existing.DataType, existing.ElementDataType, proposed.Value); + if (typeError != null) + return Result.Failure(typeError); + // Validate override rules if this is an override (parent has same-named attribute) var template = await _repository.GetTemplateByIdAsync(existing.TemplateId, cancellationToken); if (template?.ParentTemplateId != null) @@ -412,6 +434,52 @@ public class TemplateService return Result.Success(true); } + /// + /// Validates the (DataType, ElementDataType, Value) triple for an attribute. + /// Returns a human-readable failure message (for Result.Failure), or + /// null when the triple is valid. Mirrors the CLI/API-path guard in + /// ManagementActor.ValidateAttributeTypes so both server write paths + /// share one server-side check (#92), reusing + /// for the element-type and value-decode logic: + /// + /// DataType must be a defined enum value. + /// List attributes require a valid scalar element type, and a present + /// default value must decode against it. + /// Scalar attributes may not carry an element type. + /// + /// + private static string? ValidateAttributeTypes( + string name, DataType dataType, DataType? elementType, string? value) + { + if (!Enum.IsDefined(dataType)) + return $"Attribute '{name}' has an unrecognised data type."; + + if (dataType == DataType.List) + { + if (elementType is null || !AttributeValueCodec.IsValidElementType(elementType.Value)) + return $"List attribute '{name}' requires a valid element type " + + "(String, Int32, Float, Double, Boolean, DateTime)."; + + if (!string.IsNullOrWhiteSpace(value)) + { + try + { + AttributeValueCodec.Decode(value, DataType.List, elementType); + } + catch (FormatException ex) + { + return $"List attribute '{name}' has an invalid list value: {ex.Message}"; + } + } + } + else if (elementType is not null) + { + return $"Attribute '{name}': ElementDataType is only valid on List attributes."; + } + + return null; + } + // ======================================================================== // WP-3: Alarm Definitions // ======================================================================== diff --git a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/TemplateServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/TemplateServiceTests.cs index dbb09738..d8211da8 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/TemplateServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/TemplateServiceTests.cs @@ -234,6 +234,177 @@ public class TemplateServiceTests Assert.Contains("already exists", result.Error); } + // ── #92: (DataType, ElementDataType, Value) validation hoisted into the + // service so the Central UI's direct write path shares the same + // server-side guard as ManagementActor.ValidateAttributeTypes. ── + + [Fact] + public async Task AddAttribute_InvalidDataType_Fails() + { + var template = new Template("Pump") { Id = 1 }; + _repoMock.Setup(r => r.GetTemplateByIdAsync(1, It.IsAny())).ReturnsAsync(template); + + // An out-of-range enum value is the in-service equivalent of an + // unrecognised data-type token (the CLI/API path rejects the token before + // the enum is parsed). + var attr = new TemplateAttribute("Temperature") { DataType = (DataType)999, Value = "0" }; + var result = await _service.AddAttributeAsync(1, attr, "admin"); + + Assert.True(result.IsFailure); + Assert.Contains("data type", result.Error, StringComparison.OrdinalIgnoreCase); + // The attribute must never reach the repository. + _repoMock.Verify(r => r.AddTemplateAttributeAsync(It.IsAny(), It.IsAny()), Times.Never); + } + + [Fact] + public async Task AddAttribute_ListMissingElementType_Fails() + { + var template = new Template("Pump") { Id = 1 }; + _repoMock.Setup(r => r.GetTemplateByIdAsync(1, It.IsAny())).ReturnsAsync(template); + + var attr = new TemplateAttribute("SetPoints") { DataType = DataType.List, ElementDataType = null }; + var result = await _service.AddAttributeAsync(1, attr, "admin"); + + Assert.True(result.IsFailure); + Assert.Contains("element type", result.Error, StringComparison.OrdinalIgnoreCase); + _repoMock.Verify(r => r.AddTemplateAttributeAsync(It.IsAny(), It.IsAny()), Times.Never); + } + + [Fact] + public async Task AddAttribute_ListInvalidElementType_Fails() + { + var template = new Template("Pump") { Id = 1 }; + _repoMock.Setup(r => r.GetTemplateByIdAsync(1, It.IsAny())).ReturnsAsync(template); + + // Binary is a real DataType but not a valid List element scalar. + var attr = new TemplateAttribute("SetPoints") { DataType = DataType.List, ElementDataType = DataType.Binary }; + var result = await _service.AddAttributeAsync(1, attr, "admin"); + + Assert.True(result.IsFailure); + Assert.Contains("element type", result.Error, StringComparison.OrdinalIgnoreCase); + _repoMock.Verify(r => r.AddTemplateAttributeAsync(It.IsAny(), It.IsAny()), Times.Never); + } + + [Fact] + public async Task AddAttribute_ScalarWithElementType_Fails() + { + var template = new Template("Pump") { Id = 1 }; + _repoMock.Setup(r => r.GetTemplateByIdAsync(1, It.IsAny())).ReturnsAsync(template); + + var attr = new TemplateAttribute("Temperature") { DataType = DataType.Float, ElementDataType = DataType.Int32, Value = "0" }; + var result = await _service.AddAttributeAsync(1, attr, "admin"); + + Assert.True(result.IsFailure); + Assert.Contains("ElementDataType", result.Error); + _repoMock.Verify(r => r.AddTemplateAttributeAsync(It.IsAny(), It.IsAny()), Times.Never); + } + + [Fact] + public async Task AddAttribute_ListValueDoesNotDecode_Fails() + { + var template = new Template("Pump") { Id = 1 }; + _repoMock.Setup(r => r.GetTemplateByIdAsync(1, It.IsAny())).ReturnsAsync(template); + + // A valid typed List, but the value carries an element that is not an Int32. + var attr = new TemplateAttribute("SetPoints") + { + DataType = DataType.List, ElementDataType = DataType.Int32, Value = """["abc"]""" + }; + var result = await _service.AddAttributeAsync(1, attr, "admin"); + + Assert.True(result.IsFailure); + Assert.Contains("invalid list value", result.Error, StringComparison.OrdinalIgnoreCase); + _repoMock.Verify(r => r.AddTemplateAttributeAsync(It.IsAny(), It.IsAny()), Times.Never); + } + + [Fact] + public async Task AddAttribute_ScalarValueDoesNotParse_Fails() + { + // Mirrors what AttributeValueCodec.Decode would accept for the declared + // (DataType, ElementDataType). Scalars decode as-is in the codec, so a + // free-form scalar value is accepted by both the CLI/API guard and the + // service guard — there is no scalar-string parse here. Documenting the + // accepted shape: a non-numeric scalar Value is allowed (validated at deploy). + var template = new Template("Pump") { Id = 1 }; + _repoMock.Setup(r => r.GetTemplateByIdAsync(1, It.IsAny())).ReturnsAsync(template); + _repoMock.Setup(r => r.GetAllTemplatesAsync(It.IsAny())) + .ReturnsAsync(new List