fix(delmia-notifier)+chore(docker): review follow-ups — trim the env key like the file key, log which source supplied the key (never the key), hoist the not-configured message; README warns never to commit a filled-in appsettings.json; design-doc body lines updated; seed-delmia-api.sh no longer hides CLI errors and notes key names are not unique

Claude-Session: https://claude.ai/code/session_013L3XdmVii9i5sY4FChU8pB
2026-09-02 12:52:56 -04:00
parent 83f7bbfb50
commit f4363e0b4b
6 changed files with 80 additions and 18 deletions
@@ -4,7 +4,9 @@ set -euo pipefail
 # Seed the DelmiaRecipeDownload inbound method (prod's script verbatim, minus its dead `suf`
 # line — see docs/api-specs/Delmia-Recipe-Download-API.md §4) and one API key scoped to it.
 # Prints the one-time token as DELMIA_API_KEY=... — capture it; it cannot be retrieved again.
-# Not idempotent (method name must be unique): delete with `api-method delete --id`.
+# Not idempotent (method name must be unique): delete with `api-method delete --id`. Key display
+# names are NOT unique — re-running mints another key with the same name; delete the stale one with
+# `security api-key delete --key-id`.

 SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
 PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)"
@@ -12,6 +14,8 @@ CLI="$PROJECT_ROOT/src/ZB.MOM.WW.ScadaBridge.CLI/bin/Debug/net10.0/scadabridge"
 A=(--url "${SCADABRIDGE_URL:-http://localhost:9000}" --format=json --username "${SCADABRIDGE_USERNAME:-multi-role}" --password "${SCADABRIDGE_PASSWORD:-password}")
 KEY_NAME="${KEY_NAME:-delmia-notifier}"

+json_id() { python3 -c 'import json,sys; print(json.load(sys.stdin)["id"])'; }
+
 read -r -d '' METHOD_CODE <<'CS' || true
 try {
  System.Func<string, object> P = k => Parameters.ContainsKey(k) ? Parameters[k] : null;
@@ -31,13 +35,13 @@ try {
 CS

 echo "1/2 Creating inbound method DelmiaRecipeDownload (timeout 45 s)..."
-METHOD_JSON=$("$CLI" api-method create "${A[@]}" --name DelmiaRecipeDownload --timeout 45 \
+METHOD_ID=$("$CLI" api-method create "${A[@]}" --name DelmiaRecipeDownload --timeout 45 \
  --parameters '{"type":"object","properties":{"MachineCode":{"type":"string"},"DownloadPath":{"type":"string"},"WorkOrderNumber":{"type":"string"},"PartNumber":{"type":"string"},"JobStepNumber":{"type":"string"},"Username":{"type":"string"}},"required":[]}' \
  --return-def '{"type":"object","properties":{"Result":{"type":"boolean"},"ResultText":{"type":"string"}},"required":["Result","ResultText"]}' \
-  --script "$METHOD_CODE")
-echo "  $(echo "$METHOD_JSON" | python3 -c 'import json,sys; d=json.load(sys.stdin); print("id=%s name=%s" % (d["id"], d["name"]))')"
+  --script "$METHOD_CODE" | json_id)
+echo "  id=$METHOD_ID name=DelmiaRecipeDownload"

 echo "2/2 Creating API key '$KEY_NAME' scoped to DelmiaRecipeDownload..."
-KEY_JSON=$("$CLI" security api-key create "${A[@]}" --name "$KEY_NAME" --methods DelmiaRecipeDownload 2>/dev/null)
+KEY_JSON=$("$CLI" security api-key create "${A[@]}" --name "$KEY_NAME" --methods DelmiaRecipeDownload)
 TOKEN=$(echo "$KEY_JSON" | python3 -c 'import json,sys; print(json.load(sys.stdin)["token"])' 2>/dev/null || echo "$KEY_JSON" | grep -o 'sbk_[A-Za-z0-9_-]*' | head -1)
 echo "DELMIA_API_KEY=$TOKEN"
@@ -78,8 +78,9 @@ Zero third-party NuGet dependencies (BCL only) to keep the Native-AOT surface mi
  `/api/DelmiaRecipeDownload` is appended by the app; each entry is a base URL only.
 - `TimeoutSeconds` — per-attempt `HttpClient.Timeout` (default 30).
 - `LogPath` — optional diagnostic log file (relative to the exe). Omit to log to stderr only.
- **API key** comes from `SCADABRIDGE_API_KEY`. If unset/empty → `NO` + "API key not configured",
-  exit `-1`, no attempt. The key is never read from or written to a file.
+- **API key** comes from `SCADABRIDGE_API_KEY` first; since 2026-09-02 an optional `ScadaBridge:ApiKey`
+  in `appsettings.json` is the fallback when the variable is unset/blank (env wins). If neither is set →
+  `NO` + "API key not configured (SCADABRIDGE_API_KEY or ScadaBridge:ApiKey)", exit `-1`, no attempt.

 ## Request / response

@@ -170,7 +171,7 @@ cross-platform):
 - Result mapping: `Result true/false` and non-2xx → correct stdout + exit code.
 - Failover loop (via the HTTP seam / a fake handler): connect-failure advances; first responding node
  is final; `Result==false` does **not** advance; all-unreachable → `NO` + last error.
- Missing/empty `SCADABRIDGE_API_KEY` → fail-fast, no attempt.
+- Missing/empty `SCADABRIDGE_API_KEY` **and** blank `ScadaBridge:ApiKey` → fail-fast, no attempt; env-over-file precedence pinned (2026-09-02).
 - Manual live smoke against `wonder-app-vd03` (`/api/DelmiaRecipeDownload`) from the Windows build,
  mirroring the earlier `curl` verification.

@@ -34,19 +34,41 @@ internal static class ConfigLoader
        return baseUrls.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries);
    }

-    /// <summary>Resolve the API key: <c>SCADABRIDGE_API_KEY</c> wins when non-blank, otherwise <c>ScadaBridge:ApiKey</c> from <paramref name="cfg"/> (trimmed); null when neither is set. Env accessor is injected for testability.</summary>
+    /// <summary>Human-readable name of the environment source, for diagnostics (never carries key material).</summary>
+    public const string ApiKeySourceEnvironment = "environment (SCADABRIDGE_API_KEY)";
+
+    /// <summary>Human-readable name of the file source, for diagnostics (never carries key material).</summary>
+    public const string ApiKeySourceFile = "appsettings.json (ScadaBridge:ApiKey)";
+
+    /// <summary>Resolve the API key: <c>SCADABRIDGE_API_KEY</c> wins when non-blank, otherwise <c>ScadaBridge:ApiKey</c> from <paramref name="cfg"/>; both trimmed; null when neither is set. Env accessor is injected for testability.</summary>
    /// <param name="envGet">Accessor used to read an environment variable by name.</param>
    /// <param name="cfg">The loaded configuration, consulted only when the environment variable is unset/blank.</param>
    /// <returns>The resolved API key, or <c>null</c> if neither source yields a non-blank value.</returns>
-    public static string? ResolveApiKey(Func<string, string?> envGet, NotifierConfig cfg)
+    public static string? ResolveApiKey(Func<string, string?> envGet, NotifierConfig cfg) =>
+        ResolveApiKey(envGet, cfg, out _);
+
+    /// <summary>Same as <see cref="ResolveApiKey(Func{string, string?}, NotifierConfig)"/> but also reports which source supplied the key, so a stale environment variable silently shadowing an edited file is diagnosable from the log.</summary>
+    /// <param name="envGet">Accessor used to read an environment variable by name.</param>
+    /// <param name="cfg">The loaded configuration, consulted only when the environment variable is unset/blank.</param>
+    /// <param name="source"><see cref="ApiKeySourceEnvironment"/>, <see cref="ApiKeySourceFile"/>, or <c>null</c> when no source yielded a key.</param>
+    /// <returns>The resolved API key, or <c>null</c> if neither source yields a non-blank value.</returns>
+    public static string? ResolveApiKey(Func<string, string?> envGet, NotifierConfig cfg, out string? source)
    {
        var env = envGet(ApiKeyEnvVar);
        if (!string.IsNullOrWhiteSpace(env))
        {
-            return env;
+            source = ApiKeySourceEnvironment;
+            return env.Trim();
        }

        var file = cfg.ScadaBridge.ApiKey;
-        return string.IsNullOrWhiteSpace(file) ? null : file.Trim();
+        if (!string.IsNullOrWhiteSpace(file))
+        {
+            source = ApiKeySourceFile;
+            return file.Trim();
+        }
+
+        source = null;
+        return null;
    }
 }
@@ -5,6 +5,9 @@ internal static class Program
    /// <summary>Entry point: loads config, sends the recipe-download notification with failover across the configured base URLs, and reports the legacy YES/NO stdout contract.</summary>
    /// <param name="args">Command-line arguments parsed into the recipe download payload.</param>
    /// <returns>A task that resolves to the process exit code.</returns>
+    /// <summary>Stdout reason + log line when neither key source is set; one literal so the two cannot drift.</summary>
+    private const string ApiKeyNotConfigured = "API key not configured (SCADABRIDGE_API_KEY or ScadaBridge:ApiKey)";
+
    public static async Task<int> Main(string[] args)
    {
        var stdout = Console.Out;
@@ -39,13 +42,15 @@ internal static class Program
                return Reporter.Report(false, "no BaseUrls configured", stdout);
            }

-            var apiKey = ConfigLoader.ResolveApiKey(Environment.GetEnvironmentVariable, cfg);
+            var apiKey = ConfigLoader.ResolveApiKey(Environment.GetEnvironmentVariable, cfg, out var apiKeySource);
            if (apiKey is null)
            {
-                log.Write("API key not configured (SCADABRIDGE_API_KEY or ScadaBridge:ApiKey)");
-                return Reporter.Report(false, "API key not configured (SCADABRIDGE_API_KEY or ScadaBridge:ApiKey)", stdout);
+                log.Write(ApiKeyNotConfigured);
+                return Reporter.Report(false, ApiKeyNotConfigured, stdout);
            }

+            log.Write("api key source: " + apiKeySource);
+
            using var http = new HttpClient { Timeout = TimeSpan.FromSeconds(cfg.ScadaBridge.TimeoutSeconds) };
            var sender = new LoggingRecipeSender(new HttpRecipeSender(http, apiKey), log);

@@ -88,12 +88,16 @@ Placed next to the exe (copied to the output on build/publish):

 The inbound API key is sent as `X-API-Key`. It is resolved from **`SCADABRIDGE_API_KEY`** first
 (recommended for production — the key never touches disk), falling back to **`ScadaBridge:ApiKey`** in
-`appsettings.json` when the variable is unset or blank. If neither is set → `NO` +
+`appsettings.json` when the variable is unset or blank (a blank file value counts as unset too; both
+values are trimmed). The diagnostic log records which source supplied the key — never the key itself — so
+a stale environment variable silently shadowing an edited file is visible. If neither is set → `NO` +
 `API key not configured (SCADABRIDGE_API_KEY or ScadaBridge:ApiKey)`, exit `-1`, no HTTP attempt.

 Set the variable on the machine/account that DELMIA runs under (e.g. a system/user environment
 variable, or the service account's environment). If you use the file instead, restrict its ACL to the
-account DELMIA runs under — the key is stored in plain text there.
+account DELMIA runs under — the key is stored in plain text there. Fill `ApiKey` in **only on the deployed
+copy**: the repo's `appsettings.json` is tracked and copied to the build output, so a key pasted there to
+test locally is one `git add` away from being committed. It must stay `""` in source control.

 ## Building & publishing (Native AOT, `win-x64`)

@@ -106,7 +110,8 @@ dotnet publish src/ZB.MOM.WW.ScadaBridge.DelmiaNotifier -c Release -r win-x64
 # → bin/Release/net10.0/win-x64/publish/WWNotifier.exe  (+ appsettings.json)
 ```

-Copy `WWNotifier.exe` **and** `appsettings.json` to the Delmia host, set `SCADABRIDGE_API_KEY`, and
+Copy `WWNotifier.exe` **and** `appsettings.json` to the Delmia host, set `SCADABRIDGE_API_KEY` (or fill in
+`ScadaBridge:ApiKey` on that copy), and
 point `BaseUrls` at the central node(s).

 > Managed `dotnet build` / `dotnet test` run cross-platform and are the development/CI gate; only the
@@ -70,6 +70,31 @@ public class ConfigLoaderTests
        Assert.Equal("sbk_file", ConfigLoader.ResolveApiKey(_ => "   ", cfg));
    }

+    [Fact]
+    public void ResolveApiKey_trims_env_value_like_the_file_value()
+    {
+        Assert.Equal("sbk_env", ConfigLoader.ResolveApiKey(_ => "  sbk_env \t", new NotifierConfig()));
+    }
+
+    [Fact]
+    public void ResolveApiKey_reports_which_source_supplied_the_key()
+    {
+        var cfg = ConfigLoader.Load("{\"ScadaBridge\":{\"ApiKey\":\"sbk_file\"}}");
+        ConfigLoader.ResolveApiKey(_ => "sbk_env", cfg, out var fromEnv);
+        ConfigLoader.ResolveApiKey(_ => null, cfg, out var fromFile);
+        ConfigLoader.ResolveApiKey(_ => null, new NotifierConfig(), out var none);
+        Assert.Equal(ConfigLoader.ApiKeySourceEnvironment, fromEnv);
+        Assert.Equal(ConfigLoader.ApiKeySourceFile, fromFile);
+        Assert.Null(none);
+    }
+
+    [Fact]
+    public void Load_leaves_ApiKey_null_when_absent()
+    {
+        var cfg = ConfigLoader.Load("{\"ScadaBridge\":{\"BaseUrls\":\"http://x\"}}");
+        Assert.Null(cfg.ScadaBridge.ApiKey);
+    }
+
    [Fact]
    public void ResolveApiKey_returns_null_when_neither_source_set()
    {