From b13d7b3d28803d82046ffe4abb3a245e1e0bfb5c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 2 Jun 2026 05:15:50 -0400 Subject: [PATCH] =?UTF-8?q?fix(auth):=20C4=20review=20polish=20=E2=80=94?= =?UTF-8?q?=20document=20backward-compat=20JSON=20tolerance,=20shared=20Bu?= =?UTF-8?q?ndleJsonOptions,=20PreviewAsync=20legacy-bundle=20test,=20doc?= =?UTF-8?q?=20fix=20(review=20I-2/I-3/M-1/M-2;=20I-1=20intentionally=20ski?= =?UTF-8?q?pped)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Import/BundleImporter.cs | 15 ++-- .../Serialization/BundleJsonOptions.cs | 37 ++++++++ .../Serialization/BundleSerializer.cs | 10 +-- .../Serialization/EntityDtos.cs | 3 +- .../Import/BundleImporterApplyTests.cs | 84 +++++++++++++++++++ 5 files changed, 134 insertions(+), 15 deletions(-) create mode 100644 src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/BundleJsonOptions.cs diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs index 9a5dab59..17069bd5 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs @@ -1,7 +1,6 @@ using System.IO.Compression; using System.Security.Cryptography; using System.Text.Json; -using System.Text.Json.Serialization; using Microsoft.Extensions.Options; using ZB.MOM.WW.ScadaBridge.Commons.Entities.ExternalSystems; using ZB.MOM.WW.ScadaBridge.Commons.Entities.InboundApi; @@ -45,12 +44,14 @@ namespace ZB.MOM.WW.ScadaBridge.Transport.Import; /// public sealed class BundleImporter : IBundleImporter { - private static readonly JsonSerializerOptions ContentJsonOptions = new() - { - WriteIndented = false, - DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, - Converters = { new JsonStringEnumConverter() }, - }; + // All bundle content deserialization goes through BundleJsonOptions.Default. + // IMPORTANT — unknown JSON properties must remain ALLOWED (the default + // JsonUnmappedMemberHandling.Skip). Pre-C4 bundles may carry a top-level + // "apiKeys" array and/or "ApprovedApiKeyIds" inside "apiMethods[]" entries. + // Setting JsonUnmappedMemberHandling.Disallow here would cause those bundles + // to fail deserialization, breaking backward-compat. This tolerance is + // load-bearing and must not be changed. (Fix I-2) + private static readonly JsonSerializerOptions ContentJsonOptions = BundleJsonOptions.Default; private readonly BundleSerializer _bundleSerializer; private readonly ManifestValidator _manifestValidator; diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/BundleJsonOptions.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/BundleJsonOptions.cs new file mode 100644 index 00000000..77da293e --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/BundleJsonOptions.cs @@ -0,0 +1,37 @@ +using System.Text.Json; +using System.Text.Json.Serialization; + +namespace ZB.MOM.WW.ScadaBridge.Transport.Serialization; + +/// +/// Canonical shared by every component that +/// reads or writes a bundle content payload. +/// +/// Important — unknown-property tolerance: +/// is left at its default +/// (Skip); setting it to Disallow would break backward- +/// compatible deserialization of pre-C4 bundles, which may carry a top-level +/// apiKeys array and/or ApprovedApiKeyIds fields inside +/// apiMethods[] entries. That tolerance is load-bearing and must be +/// preserved. +/// +/// +/// WhenWritingNull: new bundles never emit a top-level +/// ApiKeys property (it defaults to null on +/// ); WhenWritingNull ensures the field +/// is omitted entirely from the JSON so the wire format stays clean. +/// +/// +internal static class BundleJsonOptions +{ + /// + /// The single shared instance for + /// all bundle content serialization and deserialization. + /// + internal static readonly JsonSerializerOptions Default = new() + { + WriteIndented = false, + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, + Converters = { new JsonStringEnumConverter() }, + }; +} diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/BundleSerializer.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/BundleSerializer.cs index 3dd9f7cb..dd74c3ba 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/BundleSerializer.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/BundleSerializer.cs @@ -1,6 +1,5 @@ using System.IO.Compression; using System.Text.Json; -using System.Text.Json.Serialization; using ZB.MOM.WW.ScadaBridge.Commons.Types.Transport; using ZB.MOM.WW.ScadaBridge.Transport.Encryption; @@ -26,12 +25,9 @@ public sealed class BundleSerializer private const string ContentJsonEntryName = "content.json"; private const string ContentEncEntryName = "content.enc"; - private static readonly JsonSerializerOptions JsonOptions = new() - { - WriteIndented = false, - DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, - Converters = { new JsonStringEnumConverter() }, - }; + // All bundle content serialization goes through BundleJsonOptions.Default — + // see that class for the rationale (WhenWritingNull + unknown-member tolerance). + private static readonly JsonSerializerOptions JsonOptions = BundleJsonOptions.Default; /// /// Serializes the bundle content to its canonical JSON byte form. Exposed diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntityDtos.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntityDtos.cs index 7fe8a253..be8a881d 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntityDtos.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntityDtos.cs @@ -30,7 +30,8 @@ public sealed record EntityAggregate( /// Top-level serializable bundle payload. Lists are sequenced in dependency /// order so importers can apply them inline. Lists are never null on the wire /// — empty arrays are preferred over nulls so JSON consumers can rely on each -/// property being present. +/// property being present — except , which is intentionally +/// null-defaulted (see below). /// /// is a legacy, read-only field retained purely for /// backward-compatible deserialization of bundles produced before the diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs index 93a3bf84..7beca08a 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs @@ -839,4 +839,88 @@ public sealed class BundleImporterApplyTests : IDisposable Assert.Equal(1, result.Added); // the API method Assert.NotEqual(Guid.Empty, result.BundleImportId); } + + // ───────────────────────────────────────────────────────────────────── + // Fix I-3: PreviewAsync must also tolerate a legacy bundle that carries + // an ApiKeys section without surfacing those keys as importable preview + // rows. Mirror the ApplyAsync_ignores_legacy_api_keys_in_bundle_without_failing + // setup: hand-pack a legacy bundle with a populated ApiKeys section plus + // one ApiMethod, then assert that PreviewAsync completes without fault, + // surfaces no ApiKey/key preview items, and DOES surface the ApiMethod + // preview item. + // ───────────────────────────────────────────────────────────────────── + [Fact] + public async Task PreviewAsync_on_legacy_bundle_does_not_surface_key_items() + { + // Arrange: hand-pack a legacy bundle whose content JSON contains an + // ApiKeys array plus one API method. New exports never emit ApiKeys, + // so we build the BundleContentDto directly with a populated (non-null) + // legacy ApiKeys list to faithfully simulate a pre-C4 file. + var legacyContent = new BundleContentDto( + TemplateFolders: Array.Empty(), + Templates: Array.Empty(), + SharedScripts: Array.Empty(), + ExternalSystems: Array.Empty(), + DatabaseConnections: Array.Empty(), + NotificationLists: Array.Empty(), + SmtpConfigs: Array.Empty(), + ApiMethods: new[] + { + new ApiMethodDto("GetStatus", "return 0;", + ParameterDefinitions: null, ReturnDefinition: null, TimeoutSeconds: 30), + }, + ApiKeys: new[] + { + new ApiKeyDto("legacy-key-x", "hash-x", IsEnabled: true, Secrets: null), + }); + + Guid sessionId; + await using (var scope = _provider.CreateAsyncScope()) + { + var serializer = scope.ServiceProvider.GetRequiredService(); + var manifestBuilder = scope.ServiceProvider.GetRequiredService(); + var importer = scope.ServiceProvider.GetRequiredService(); + + var contentBytes = serializer.SerializeContentBytes(legacyContent); + + // Sanity: the packed content really does carry an ApiKeys section, + // so the test is exercising the legacy-ignore path rather than a no-op. + var contentJson = System.Text.Encoding.UTF8.GetString(contentBytes); + Assert.Contains("legacy-key-x", contentJson); + + var manifest = manifestBuilder.Build( + sourceEnvironment: "legacy-env", + exportedBy: "alice", + scadaBridgeVersion: "0.9.0", + encryption: null, + summary: new BundleSummary(0, 0, 0, 0, 0, 0, 0, 1), + contents: Array.Empty(), + contentBytes: contentBytes); + await using var packed = serializer.Pack(legacyContent, manifest, passphrase: null, encryptor: null); + + using var ms = new MemoryStream(); + await packed.CopyToAsync(ms); + ms.Position = 0; + var session = await importer.LoadAsync(ms, passphrase: null); + sessionId = session.SessionId; + } + + // Act — call PreviewAsync; must not throw. + ImportPreview preview; + await using (var scope = _provider.CreateAsyncScope()) + { + var importer = scope.ServiceProvider.GetRequiredService(); + preview = await importer.PreviewAsync(sessionId); + } + + // Assert — no ApiKey preview rows surfaced (keys are not transported). + Assert.DoesNotContain(preview.Items, item => + item.EntityType.Contains("Key", StringComparison.OrdinalIgnoreCase) || + item.Name.Contains("legacy-key", StringComparison.OrdinalIgnoreCase)); + + // The one ApiMethod IS surfaced as a New item (method does not exist in + // the target DB, so ConflictKind.New is expected). + Assert.Contains(preview.Items, item => + item.EntityType == "ApiMethod" && item.Name == "GetStatus"); + } }