fix(auth): C4 review polish — document backward-compat JSON tolerance, shared BundleJsonOptions, PreviewAsync legacy-bundle test, doc fix (review I-2/I-3/M-1/M-2; I-1 intentionally skipped)

2026-06-02 05:15:50 -04:00
parent 731cfd3bfc
commit b13d7b3d28
5 changed files with 134 additions and 15 deletions
@@ -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;
 /// </summary>
 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;
@@ -0,0 +1,37 @@
+using System.Text.Json;
+using System.Text.Json.Serialization;
+
+namespace ZB.MOM.WW.ScadaBridge.Transport.Serialization;
+
+/// <summary>
+/// Canonical <see cref="JsonSerializerOptions"/> shared by every component that
+/// reads or writes a bundle content payload.
+/// <para>
+/// <b>Important — unknown-property tolerance:</b>
+/// <see cref="JsonUnmappedMemberHandling"/> is left at its default
+/// (<c>Skip</c>); setting it to <c>Disallow</c> would break backward-
+/// compatible deserialization of pre-C4 bundles, which may carry a top-level
+/// <c>apiKeys</c> array and/or <c>ApprovedApiKeyIds</c> fields inside
+/// <c>apiMethods[]</c> entries. That tolerance is load-bearing and must be
+/// preserved.
+/// </para>
+/// <para>
+/// <b><c>WhenWritingNull</c>:</b> new bundles never emit a top-level
+/// <c>ApiKeys</c> property (it defaults to <c>null</c> on
+/// <see cref="BundleContentDto"/>); <c>WhenWritingNull</c> ensures the field
+/// is omitted entirely from the JSON so the wire format stays clean.
+/// </para>
+/// </summary>
+internal static class BundleJsonOptions
+{
+    /// <summary>
+    /// The single shared <see cref="JsonSerializerOptions"/> instance for
+    /// all bundle content serialization and deserialization.
+    /// </summary>
+    internal static readonly JsonSerializerOptions Default = new()
+    {
+        WriteIndented = false,
+        DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
+        Converters = { new JsonStringEnumConverter() },
+    };
+}
@@ -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;

    /// <summary>
    /// Serializes the bundle content to its canonical JSON byte form. Exposed
@@ -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 <see cref="ApiKeys"/>, which is intentionally
+/// null-defaulted (see below).
 /// <para>
 /// <see cref="ApiKeys"/> is a <b>legacy, read-only</b> field retained purely for
 /// backward-compatible deserialization of bundles produced before the
@@ -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<TemplateFolderDto>(),
+            Templates: Array.Empty<TemplateDto>(),
+            SharedScripts: Array.Empty<SharedScriptDto>(),
+            ExternalSystems: Array.Empty<ExternalSystemDto>(),
+            DatabaseConnections: Array.Empty<DatabaseConnectionDto>(),
+            NotificationLists: Array.Empty<NotificationListDto>(),
+            SmtpConfigs: Array.Empty<SmtpConfigDto>(),
+            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<BundleSerializer>();
+            var manifestBuilder = scope.ServiceProvider.GetRequiredService<ManifestBuilder>();
+            var importer = scope.ServiceProvider.GetRequiredService<IBundleImporter>();
+
+            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<ManifestContentEntry>(),
+                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<IBundleImporter>();
+            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");
+    }
 }