diff --git a/code-reviews/Admin/findings.md b/code-reviews/Admin/findings.md index 57a8645..1833584 100644 --- a/code-reviews/Admin/findings.md +++ b/code-reviews/Admin/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 10 | +| Open findings | 7 | ## Checklist coverage @@ -63,13 +63,13 @@ | Severity | High | | Category | Security | | Location | `Program.cs:137-139`, `Hubs/FleetStatusHub.cs:11`, `Hubs/AlertHub.cs:10`, `Hubs/ScriptLogHub.cs:30` | -| Status | Open | +| Status | Resolved | **Description:** All three SignalR hubs (`/hubs/fleet`, `/hubs/alerts`, `/hubs/script-log`) are mapped with no `[Authorize]` attribute and no `.RequireAuthorization()` on the `MapHub` call. Any unauthenticated client can open a hub connection: `FleetStatusHub.SubscribeFleet()` streams every node generation/role/resilience state, `AlertHub` pushes all fleet alerts (including failure detail text), and `ScriptLogHub.TailLogAsync` streams the contents of the server `scripts-*.log` files. This is an unauthenticated information-disclosure channel that bypasses the (already broken — see Admin-001) page auth entirely. **Recommendation:** Add `[Authorize]` to each `Hub` class, or chain `.RequireAuthorization()` onto each `MapHub(...)` call in `Program.cs`. The hub `SubscribeCluster`/`TailLogAsync` methods should additionally validate that the caller claims permit the requested cluster/script scope. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `[Authorize]` added to `FleetStatusHub`, `AlertHub` and `ScriptLogHub`, and `.RequireAuthorization()` chained onto all three `MapHub(...)` calls in `Program.cs` as a belt-and-braces backstop, so an anonymous client can no longer open any hub connection. Covered by `AuthEndpointsTests.Anonymous_hub_negotiate_is_rejected`. ### Admin-004 @@ -78,13 +78,13 @@ | Severity | High | | Category | Security | | Location | `appsettings.json:3,13-14` | -| Status | Open | +| Status | Resolved | **Description:** The checked-in `appsettings.json` contains live-looking secrets in plaintext: the `ConfigDb` connection string with `User Id=sa;Password=OtOpcUaDev_2026!` and the LDAP `ServiceAccountPassword: "serviceaccount123"`. It also sets `Encrypt=False` and `AllowInsecureLdap: true`, so the SQL and LDAP credentials travel unencrypted on the wire. Committing the `sa` account password and a service-account password to source control is a credential-exposure risk; `sa` additionally grants full server control, conflicting with the `ClusterService` doc comment that production should connect with a least-privilege grant. **Recommendation:** Move all secrets out of the committed file — use user-secrets for dev and environment variables / a secret store for production; leave only non-secret placeholders in `appsettings.json`. Use a least-privilege SQL login rather than `sa`. Enable TLS for both SQL (`Encrypt=True`) and LDAP (`UseTls=true`, `AllowInsecureLdap=false`) for any non-loopback deployment, and document the dev-only exception. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — the `sa` connection string and the LDAP `ServiceAccountPassword` were replaced with empty placeholders in `appsettings.json`; a `_secrets` note documents that they are supplied via user-secrets (dev) or the `ConnectionStrings__ConfigDb` / `Authentication__Ldap__ServiceAccountPassword` environment variables (prod), and that the connection string must use `Encrypt=True` and a least-privilege SQL login. A `UserSecretsId` was added to the Admin csproj, and `Program.cs` now fails fast with a clear message when `ConfigDb` is empty/missing. Covered by `AppSettingsSecretHygieneTests`. ### Admin-005 @@ -93,13 +93,13 @@ | Severity | High | | Category | Correctness & logic bugs | | Location | `Components/Pages/Login.razor:15,107-110` | -| Status | Open | +| Status | Resolved | **Description:** `Login.razor` is an interactive component (the project default render mode is interactive server; the page declares no `@rendermode` but uses `EditForm`/`InputText` interactive binding and runs `SignInAsync` from an event handler). It calls `HttpContext.SignInAsync(...)` followed by `ctx.Response.Redirect("/")` from within a SignalR circuit callback. Writing auth cookies and HTTP redirect headers requires a live, unstarted HTTP response; in an interactive circuit the original HTTP response has long completed, so the cookie is typically not emitted and the redirect is ineffective (or throws "response has already started"). `admin-ui.md` section "Operator authentication" explicitly specifies the login as a static server-rendered HTML form POSTing to a `/auth/login` minimal-API endpoint with `data-enhance="false"` — that endpoint is not implemented and is not mapped in `Program.cs`. **Recommendation:** Implement the login as designed: a static-rendered form (`@rendermode` none, `data-enhance="false"`) posting to a `MapPost("/auth/login", ...)` minimal-API handler that does the LDAP bind, grant resolution, `SignInAsync` and redirect while the HTTP response is still owned by the endpoint. Do not perform `SignInAsync` from an interactive circuit. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `Login.razor` rewritten as a static-rendered plain HTML `
` (no `@rendermode`, no `EditForm`/`SignInAsync` in a circuit); the LDAP bind, grant resolution, cookie `SignInAsync` and redirect now run in a new `AuthEndpoints.MapAuthEndpoints()` minimal-API handler (`/auth/login`, `/auth/logout`) while the endpoint still owns the HTTP response. The handler is `AllowAnonymous`, carries an open-redirect guard on `returnUrl`, and surfaces bind errors back to the login page via a query-string. Covered by `AuthEndpointsTests` (valid login issues the cookie, invalid login redirects with error, open-redirect rejected, logout clears the cookie). ### Admin-006 diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Login.razor b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Login.razor index cec8f2a..b0e6b30 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Login.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Login.razor @@ -2,38 +2,40 @@ @* The login page must stay anonymously reachable — otherwise the fallback authorization policy (Admin-001) would lock operators out of the only way in. *@ @attribute [Microsoft.AspNetCore.Authorization.AllowAnonymous] -@using System.Security.Claims -@using Microsoft.AspNetCore.Authentication -@using Microsoft.AspNetCore.Authentication.Cookies -@using ZB.MOM.WW.OtOpcUa.Admin.Security -@inject IHttpContextAccessor Http -@inject ILdapAuthService LdapAuth -@inject IAdminRoleGrantResolver GrantResolver -@inject NavigationManager Nav + +@* Admin-005: this page is static server-rendered (no @rendermode). It is a plain HTML + form that POSTs to the /auth/login minimal-API endpoint with data-enhance="false", so + the LDAP bind, cookie SignInAsync and redirect all run while the endpoint still owns + an unstarted HTTP response. SignInAsync must NOT be called from an interactive Blazor + circuit — by then the original HTTP response has long completed. *@
OtOpcUa Admin — sign in
- + + @if (ReturnUrl is not null) + { + + }
- - + +
- - + +
- @if (_error is not null) + @if (!string.IsNullOrWhiteSpace(Error)) { -
@_error
+
@Error
} - - + +
@@ -45,73 +47,11 @@
@code { - private sealed class Input - { - public string Username { get; set; } = string.Empty; - public string Password { get; set; } = string.Empty; - } + /// Error message surfaced by the /auth/login endpoint after a failed bind. + [SupplyParameterFromQuery] + private string? Error { get; set; } - // Static-SSR form post: the model must be [SupplyParameterFromForm] or the - // submitted username/password never bind back onto _input. The property - // cannot carry an initializer (BL0008) — seed it in OnInitialized instead. - [SupplyParameterFromForm] - private Input _input { get; set; } = default!; - - private string? _error; - private bool _busy; - - protected override void OnInitialized() => _input ??= new(); - - private async Task SignInAsync() - { - _error = null; - _busy = true; - try - { - if (string.IsNullOrWhiteSpace(_input.Username) || string.IsNullOrWhiteSpace(_input.Password)) - { - _error = "Username and password are required"; - return; - } - - var result = await LdapAuth.AuthenticateAsync(_input.Username, _input.Password, CancellationToken.None); - if (!result.Success) - { - _error = result.Error ?? "Sign-in failed"; - return; - } - - // Resolve grants from the static bootstrap dictionary + DB-backed role grants. - // result.Roles (static-only) is intentionally not consulted here. - var grants = await GrantResolver.ResolveAsync(result.Groups, CancellationToken.None); - if (grants.IsEmpty) - { - _error = "Sign-in succeeded but no Admin roles mapped for your LDAP groups. Contact your administrator."; - return; - } - - var ctx = Http.HttpContext - ?? throw new InvalidOperationException("HttpContext unavailable at sign-in"); - - var claims = new List - { - new(ClaimTypes.Name, result.DisplayName ?? result.Username ?? _input.Username), - new(ClaimTypes.NameIdentifier, _input.Username), - }; - foreach (var role in grants.FleetRoles) - claims.Add(new Claim(ClaimTypes.Role, role)); - foreach (var clusterGrant in grants.ClusterRoles) - claims.Add(new Claim(ClusterRoleClaims.ClaimType, - ClusterRoleClaims.Encode(clusterGrant.ClusterId, clusterGrant.Role))); - foreach (var group in result.Groups) - claims.Add(new Claim("ldap_group", group)); - - var identity = new ClaimsIdentity(claims, CookieAuthenticationDefaults.AuthenticationScheme); - await ctx.SignInAsync(CookieAuthenticationDefaults.AuthenticationScheme, - new ClaimsPrincipal(identity)); - - ctx.Response.Redirect("/"); - } - finally { _busy = false; } - } + /// Original protected URL the operator was bounced from; round-tripped to the endpoint. + [SupplyParameterFromQuery] + private string? ReturnUrl { get; set; } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Hubs/AlertHub.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Hubs/AlertHub.cs index a108115..c90a0bf 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Hubs/AlertHub.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Hubs/AlertHub.cs @@ -1,3 +1,4 @@ +using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.SignalR; namespace ZB.MOM.WW.OtOpcUa.Admin.Hubs; @@ -7,6 +8,11 @@ namespace ZB.MOM.WW.OtOpcUa.Admin.Hubs; /// anomalies) to subscribed admin clients. Alerts don't auto-clear — the operator acks them /// from the UI via . /// +/// +/// gates the hub so failure-detail alert text is not pushed +/// to anonymous connections (Admin-003). +/// +[Authorize] public sealed class AlertHub : Hub { public const string AllAlertsGroup = "__alerts__"; diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Hubs/FleetStatusHub.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Hubs/FleetStatusHub.cs index 5af7e66..72bc96c 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Hubs/FleetStatusHub.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Hubs/FleetStatusHub.cs @@ -1,3 +1,4 @@ +using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.SignalR; namespace ZB.MOM.WW.OtOpcUa.Admin.Hubs; @@ -8,6 +9,12 @@ namespace ZB.MOM.WW.OtOpcUa.Admin.Hubs; /// scope notifications; the server sends NodeStateChanged messages whenever the poller /// observes a delta. /// +/// +/// gates the hub: an unauthenticated client cannot open the +/// connection, so the fleet generation/role/resilience stream is not an anonymous +/// information-disclosure channel (Admin-003). +/// +[Authorize] public sealed class FleetStatusHub : Hub { public Task SubscribeCluster(string clusterId) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Hubs/ScriptLogHub.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Hubs/ScriptLogHub.cs index ef79a34..28e837c 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Hubs/ScriptLogHub.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Hubs/ScriptLogHub.cs @@ -1,5 +1,6 @@ using System.Runtime.CompilerServices; using System.Text.RegularExpressions; +using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.SignalR; namespace ZB.MOM.WW.OtOpcUa.Admin.Hubs; @@ -26,7 +27,12 @@ namespace ZB.MOM.WW.OtOpcUa.Admin.Hubs; /// first, then new lines are emitted as they are appended. The stream is cancelled when /// the client disconnects. /// +/// +/// gates the hub: only an authenticated operator can +/// open the connection and tail the server scripts-*.log contents (Admin-003). +/// /// +[Authorize] public sealed class ScriptLogHub(IConfiguration configuration, ILogger logger) : Hub { /// Number of existing lines to replay from the end of the file before live-tailing. diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Program.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Program.cs index 7c98a6e..5634d96 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Program.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Program.cs @@ -1,4 +1,3 @@ -using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authentication.Cookies; using Microsoft.EntityFrameworkCore; using OpenTelemetry.Metrics; @@ -42,9 +41,20 @@ builder.Services.AddAuthorizationBuilder() builder.Services.AddCascadingAuthenticationState(); -builder.Services.AddDbContext(opt => - opt.UseSqlServer(builder.Configuration.GetConnectionString("ConfigDb") - ?? throw new InvalidOperationException("ConnectionStrings:ConfigDb not configured"))); +// Admin-004: the connection string is a secret — it is NOT committed to appsettings.json. +// Supply it via user-secrets (dev) or the ConnectionStrings__ConfigDb environment variable +// (prod). An empty/missing value is treated as "not configured" so a misconfigured deploy +// fails fast with a clear message rather than a downstream SqlClient parse error. +builder.Services.AddDbContext((sp, opt) => +{ + var connectionString = sp.GetRequiredService().GetConnectionString("ConfigDb"); + if (string.IsNullOrWhiteSpace(connectionString)) + throw new InvalidOperationException( + "ConnectionStrings:ConfigDb is not configured. Set it via user-secrets " + + "(dotnet user-secrets set \"ConnectionStrings:ConfigDb\" \"...\") or the " + + "ConnectionStrings__ConfigDb environment variable."); + opt.UseSqlServer(connectionString); +}); builder.Services.AddScoped(); builder.Services.AddScoped(); @@ -136,15 +146,17 @@ app.UseAuthentication(); app.UseAuthorization(); app.UseAntiforgery(); -app.MapPost("/auth/logout", async (HttpContext ctx) => -{ - await ctx.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme); - ctx.Response.Redirect("/"); -}); +// Admin-005: login + logout are minimal-API endpoints. The login does the LDAP bind, +// grant resolution, cookie SignInAsync and redirect while the HTTP response is still +// owned by the endpoint — a static-rendered Login.razor form posts here. +app.MapAuthEndpoints(); -app.MapHub("/hubs/fleet"); -app.MapHub("/hubs/alerts"); -app.MapHub("/hubs/script-log"); +// Admin-003: every SignalR hub requires an authenticated caller. The [Authorize] attribute +// on each Hub class is the primary gate; .RequireAuthorization() on the endpoint is the +// belt-and-braces backstop so a hub added without the attribute still cannot ship anonymous. +app.MapHub("/hubs/fleet").RequireAuthorization(); +app.MapHub("/hubs/alerts").RequireAuthorization(); +app.MapHub("/hubs/script-log").RequireAuthorization(); if (metricsEnabled) { diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Security/AuthEndpoints.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Security/AuthEndpoints.cs new file mode 100644 index 0000000..5aae189 --- /dev/null +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Security/AuthEndpoints.cs @@ -0,0 +1,101 @@ +using System.Security.Claims; +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Authentication.Cookies; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Mvc; + +namespace ZB.MOM.WW.OtOpcUa.Admin.Security; + +/// +/// Minimal-API authentication endpoints. Admin-005: the login is a static-rendered HTML +/// form (Login.razor, data-enhance="false") POSTing here, so the LDAP bind, +/// grant resolution, cookie write and the redirect all happen while the endpoint +/// still owns an unstarted HTTP response. Performing SignInAsync from an interactive +/// Blazor circuit (the previous implementation) could not emit the auth cookie because the +/// original HTTP response had already completed. +/// +public static class AuthEndpoints +{ + /// Maps POST /auth/login and POST /auth/logout. + public static IEndpointRouteBuilder MapAuthEndpoints(this IEndpointRouteBuilder endpoints) + { + // Anonymous: the login POST is the only way in, so the fallback authorization policy + // (Admin-001) must not gate it. DisableAntiforgery — the static form posts with + // data-enhance="false" and renders no token; the cookie scheme + LDAP bind are the + // gate here. (Admin-006 covers emitting a token for a hardened build.) + endpoints.MapPost("/auth/login", (Delegate)LoginAsync) + .AllowAnonymous() + .DisableAntiforgery(); + + endpoints.MapPost("/auth/logout", (Delegate)LogoutAsync) + .DisableAntiforgery(); + + return endpoints; + } + + private static async Task LoginAsync( + HttpContext ctx, + [FromForm] string? username, + [FromForm] string? password, + [FromForm] string? returnUrl, + ILdapAuthService ldapAuth, + IAdminRoleGrantResolver grantResolver, + CancellationToken ct) + { + if (string.IsNullOrWhiteSpace(username) || string.IsNullOrWhiteSpace(password)) + return RedirectToLogin("Username and password are required", returnUrl); + + var result = await ldapAuth.AuthenticateAsync(username, password, ct); + if (!result.Success) + return RedirectToLogin(result.Error ?? "Sign-in failed", returnUrl); + + // Grants come from the static bootstrap dictionary + DB-backed role grants; + // result.Roles (static-only) is intentionally not consulted here. + var grants = await grantResolver.ResolveAsync(result.Groups, ct); + if (grants.IsEmpty) + return RedirectToLogin( + "Sign-in succeeded but no Admin roles mapped for your LDAP groups. Contact your administrator.", + returnUrl); + + var claims = new List + { + new(ClaimTypes.Name, result.DisplayName ?? result.Username ?? username), + new(ClaimTypes.NameIdentifier, username), + }; + foreach (var role in grants.FleetRoles) + claims.Add(new Claim(ClaimTypes.Role, role)); + foreach (var clusterGrant in grants.ClusterRoles) + claims.Add(new Claim(ClusterRoleClaims.ClaimType, + ClusterRoleClaims.Encode(clusterGrant.ClusterId, clusterGrant.Role))); + foreach (var group in result.Groups) + claims.Add(new Claim("ldap_group", group)); + + var identity = new ClaimsIdentity(claims, CookieAuthenticationDefaults.AuthenticationScheme); + await ctx.SignInAsync(CookieAuthenticationDefaults.AuthenticationScheme, + new ClaimsPrincipal(identity)); + + return Results.Redirect(SafeReturnUrl(returnUrl)); + } + + private static async Task LogoutAsync(HttpContext ctx) + { + await ctx.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme); + return Results.Redirect("/login"); + } + + private static IResult RedirectToLogin(string error, string? returnUrl) + { + var target = $"/login?error={Uri.EscapeDataString(error)}"; + if (!string.IsNullOrWhiteSpace(returnUrl) && IsLocalUrl(returnUrl)) + target += $"&returnUrl={Uri.EscapeDataString(returnUrl)}"; + return Results.Redirect(target); + } + + /// Open-redirect guard — only same-site relative paths are honoured. + private static string SafeReturnUrl(string? returnUrl) => + !string.IsNullOrWhiteSpace(returnUrl) && IsLocalUrl(returnUrl) ? returnUrl : "/"; + + private static bool IsLocalUrl(string url) => + url.StartsWith('/') && !url.StartsWith("//") && !url.StartsWith("/\\"); +} diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/ZB.MOM.WW.OtOpcUa.Admin.csproj b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/ZB.MOM.WW.OtOpcUa.Admin.csproj index 8f29e90..ff14093 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/ZB.MOM.WW.OtOpcUa.Admin.csproj +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/ZB.MOM.WW.OtOpcUa.Admin.csproj @@ -9,6 +9,9 @@ $(NoWarn);CS1591 ZB.MOM.WW.OtOpcUa.Admin OtOpcUa.Admin + + zb-mom-ww-otopcua-admin diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/appsettings.json b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/appsettings.json index 77e7cde..c1b1e22 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/appsettings.json +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/appsettings.json @@ -1,6 +1,7 @@ { + "_secrets": "Admin-004: no secrets are committed here. Supply the ConfigDb connection string and the LDAP service-account password via user-secrets (dev) or environment variables / a secret store (prod). Env-var keys: ConnectionStrings__ConfigDb and Authentication__Ldap__ServiceAccountPassword. The connection string defaults to Encrypt=True (TLS); use a least-privilege SQL login, not 'sa'.", "ConnectionStrings": { - "ConfigDb": "Server=10.100.0.35,14330;Database=OtOpcUaConfig;User Id=sa;Password=OtOpcUaDev_2026!;TrustServerCertificate=True;Encrypt=False;" + "ConfigDb": "" }, "Authentication": { "Ldap": { @@ -11,7 +12,7 @@ "AllowInsecureLdap": true, "SearchBase": "dc=lmxopcua,dc=local", "ServiceAccountDn": "cn=serviceaccount,dc=lmxopcua,dc=local", - "ServiceAccountPassword": "serviceaccount123", + "ServiceAccountPassword": "", "DisplayNameAttribute": "cn", "GroupAttribute": "memberOf", "GroupToRole": { diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/AppSettingsSecretHygieneTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/AppSettingsSecretHygieneTests.cs new file mode 100644 index 0000000..9d56422 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/AppSettingsSecretHygieneTests.cs @@ -0,0 +1,79 @@ +using System.Text.Json; +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Admin.Tests; + +/// +/// Regression coverage for Admin-004 — the committed appsettings.json must carry no +/// plaintext secrets. The ConfigDb connection string and the LDAP +/// ServiceAccountPassword are supplied at runtime via user-secrets (dev) or +/// environment variables (prod); the checked-in file holds only empty placeholders. +/// +[Trait("Category", "Unit")] +public sealed class AppSettingsSecretHygieneTests +{ + private static JsonDocument LoadAdminAppSettings() + { + // Walk up from the test assembly to the repo root (the dir holding the .slnx) and + // read the SOURCE appsettings.json — not a bin/ copy — so the test asserts on what + // is actually committed. + var dir = AppContext.BaseDirectory; + while (dir is not null && !File.Exists(Path.Combine(dir, "ZB.MOM.WW.OtOpcUa.slnx"))) + dir = Path.GetDirectoryName(dir); + + dir.ShouldNotBeNull("could not locate the repo root (ZB.MOM.WW.OtOpcUa.slnx)"); + var path = Path.Combine(dir, "src", "Server", "ZB.MOM.WW.OtOpcUa.Admin", "appsettings.json"); + File.Exists(path).ShouldBeTrue($"Admin appsettings.json not found at {path}"); + + return JsonDocument.Parse(File.ReadAllText(path)); + } + + [Fact] + public void ConfigDb_connection_string_is_an_empty_placeholder() + { + using var doc = LoadAdminAppSettings(); + + var connectionString = doc.RootElement + .GetProperty("ConnectionStrings") + .GetProperty("ConfigDb") + .GetString(); + + connectionString.ShouldBeNullOrEmpty( + "the ConfigDb connection string must not be committed — supply it via user-secrets " + + "or the ConnectionStrings__ConfigDb environment variable (Admin-004)"); + } + + [Fact] + public void Ldap_service_account_password_is_an_empty_placeholder() + { + using var doc = LoadAdminAppSettings(); + + var password = doc.RootElement + .GetProperty("Authentication") + .GetProperty("Ldap") + .GetProperty("ServiceAccountPassword") + .GetString(); + + password.ShouldBeNullOrEmpty( + "the LDAP ServiceAccountPassword must not be committed (Admin-004)"); + } + + [Fact] + public void No_known_dev_secret_literals_appear_anywhere_in_appsettings() + { + var dir = AppContext.BaseDirectory; + while (dir is not null && !File.Exists(Path.Combine(dir, "ZB.MOM.WW.OtOpcUa.slnx"))) + dir = Path.GetDirectoryName(dir); + dir.ShouldNotBeNull(); + + var raw = File.ReadAllText(Path.Combine( + dir, "src", "Server", "ZB.MOM.WW.OtOpcUa.Admin", "appsettings.json")); + + // The exact secret literals the review (Admin-004) flagged must be gone entirely — + // not relocated to another key, not present as a comment. + raw.ShouldNotContain("OtOpcUaDev_2026!"); + raw.ShouldNotContain("serviceaccount123"); + raw.ShouldNotContain("User Id=sa"); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/AuthEndpointsTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/AuthEndpointsTests.cs new file mode 100644 index 0000000..7c324f4 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/AuthEndpointsTests.cs @@ -0,0 +1,184 @@ +using System.Net; +using Microsoft.AspNetCore.Mvc.Testing; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Admin.Security; +using ZB.MOM.WW.OtOpcUa.Admin.Services; + +namespace ZB.MOM.WW.OtOpcUa.Admin.Tests; + +/// +/// Regression coverage for Admin-003 / Admin-005. +/// +/// Admin-005 — the login is a static-rendered form posting to the /auth/login +/// minimal-API endpoint, which performs the LDAP bind, cookie SignInAsync and +/// redirect while it still owns the HTTP response (no interactive Blazor circuit). +/// +/// Admin-003 — the three SignalR hubs reject anonymous connections. +/// +/// These are HTTP-pipeline tests with a stubbed , so they +/// run without LDAP or the central SQL Server. +/// +public sealed class AuthEndpointsTests : IClassFixture +{ + private readonly StubbedAuthAppFactory _factory; + + public AuthEndpointsTests(StubbedAuthAppFactory factory) => _factory = factory; + + /// + /// Admin app host with the LDAP service stubbed (a fixed-credential pass/fail) and the + /// background poller removed so the host starts clean without DB or directory access. + /// + public sealed class StubbedAuthAppFactory : WebApplicationFactory + { + protected override IHost CreateHost(IHostBuilder builder) + { + builder.ConfigureServices(services => + { + var poller = services.SingleOrDefault(d => + d.ImplementationType?.Name == "FleetStatusPoller"); + if (poller is not null) services.Remove(poller); + + var ldap = services.SingleOrDefault(d => d.ServiceType == typeof(ILdapAuthService)); + if (ldap is not null) services.Remove(ldap); + services.AddScoped(); + + var resolver = services.SingleOrDefault(d => d.ServiceType == typeof(IAdminRoleGrantResolver)); + if (resolver is not null) services.Remove(resolver); + services.AddScoped(); + }); + return base.CreateHost(builder); + } + + public HttpClient CreateNonRedirectingClient() => + CreateClient(new WebApplicationFactoryClientOptions { AllowAutoRedirect = false }); + } + + /// Stub LDAP: good/pw binds; anything else fails. + private sealed class StubLdapAuthService : ILdapAuthService + { + public Task AuthenticateAsync(string username, string password, CancellationToken ct = default) => + Task.FromResult(username == "good" && password == "pw" + ? new LdapAuthResult(true, "Good Operator", "good", ["FleetAdmins"], ["FleetAdmin"], null) + : new LdapAuthResult(false, null, username, [], [], "Invalid username or password")); + } + + /// Stub resolver: any non-empty group set yields a FleetAdmin grant. + private sealed class StubRoleGrantResolver : IAdminRoleGrantResolver + { + public Task ResolveAsync(IReadOnlyList ldapGroups, CancellationToken cancellationToken) => + Task.FromResult(ldapGroups.Count == 0 + ? AdminRoleGrants.Empty + : new AdminRoleGrants([AdminRoles.FleetAdmin], [])); + } + + private static FormUrlEncodedContent Form(params (string Key, string Value)[] fields) => + new(fields.Select(f => new KeyValuePair(f.Key, f.Value))); + + // ── Admin-005: /auth/login endpoint ───────────────────────────────────────── + + [Fact] + public async Task Valid_login_issues_the_auth_cookie_and_redirects_home() + { + using var client = _factory.CreateNonRedirectingClient(); + + var response = await client.PostAsync("/auth/login", + Form(("username", "good"), ("password", "pw"))); + + // The endpoint owns the response, so the Set-Cookie header is actually emitted. + response.StatusCode.ShouldBeOneOf(HttpStatusCode.Redirect, HttpStatusCode.Found); + response.Headers.Location!.OriginalString.ShouldBe("/"); + response.Headers.TryGetValues("Set-Cookie", out var cookies).ShouldBeTrue( + "a successful /auth/login must emit the auth cookie"); + string.Join(';', cookies!).ShouldContain("OtOpcUa.Admin"); + } + + [Fact] + public async Task Invalid_login_redirects_back_to_login_with_an_error() + { + using var client = _factory.CreateNonRedirectingClient(); + + var response = await client.PostAsync("/auth/login", + Form(("username", "bad"), ("password", "wrong"))); + + response.StatusCode.ShouldBeOneOf(HttpStatusCode.Redirect, HttpStatusCode.Found); + response.Headers.Location!.OriginalString.ShouldContain("/login"); + response.Headers.Location!.OriginalString.ShouldContain("error"); + response.Headers.TryGetValues("Set-Cookie", out var cookies); + (cookies is null || !string.Join(';', cookies).Contains("OtOpcUa.Admin")).ShouldBeTrue( + "a failed bind must not issue the auth cookie"); + } + + [Fact] + public async Task Login_with_missing_credentials_redirects_back_to_login() + { + using var client = _factory.CreateNonRedirectingClient(); + + var response = await client.PostAsync("/auth/login", Form(("username", ""), ("password", ""))); + + response.StatusCode.ShouldBeOneOf(HttpStatusCode.Redirect, HttpStatusCode.Found); + response.Headers.Location!.OriginalString.ShouldContain("/login"); + } + + [Fact] + public async Task Login_redirect_target_is_open_redirect_safe() + { + using var client = _factory.CreateNonRedirectingClient(); + + // A returnUrl pointing off-site must be ignored — the post lands at the site root. + var response = await client.PostAsync("/auth/login", + Form(("username", "good"), ("password", "pw"), ("returnUrl", "https://evil.example.com/"))); + + response.StatusCode.ShouldBeOneOf(HttpStatusCode.Redirect, HttpStatusCode.Found); + response.Headers.Location!.OriginalString.ShouldBe("/"); + } + + [Fact] + public async Task Login_honours_a_local_return_url() + { + using var client = _factory.CreateNonRedirectingClient(); + + var response = await client.PostAsync("/auth/login", + Form(("username", "good"), ("password", "pw"), ("returnUrl", "/fleet"))); + + response.StatusCode.ShouldBeOneOf(HttpStatusCode.Redirect, HttpStatusCode.Found); + response.Headers.Location!.OriginalString.ShouldBe("/fleet"); + } + + [Fact] + public async Task Logout_endpoint_clears_the_cookie_and_redirects_to_login() + { + using var client = _factory.CreateNonRedirectingClient(); + + var response = await client.PostAsync("/auth/logout", + new FormUrlEncodedContent(Array.Empty>())); + + // No antiforgery 400 — the endpoint opts out (Admin-006 note in AuthEndpoints). + response.StatusCode.ShouldBeOneOf(HttpStatusCode.Redirect, HttpStatusCode.Found); + response.Headers.Location!.OriginalString.ShouldContain("/login"); + } + + // ── Admin-003: SignalR hubs reject anonymous connections ──────────────────── + + [Theory] + [InlineData("/hubs/fleet")] + [InlineData("/hubs/alerts")] + [InlineData("/hubs/script-log")] + public async Task Anonymous_hub_negotiate_is_rejected(string hubPath) + { + using var client = _factory.CreateNonRedirectingClient(); + + // The SignalR negotiate handshake is a POST to /negotiate. An [Authorize]'d hub + // must refuse it for an unauthenticated caller (302 to login or 401). + var response = await client.PostAsync($"{hubPath}/negotiate", + new FormUrlEncodedContent(Array.Empty>())); + + response.StatusCode.ShouldNotBe(HttpStatusCode.OK, + $"anonymous negotiate of {hubPath} must not succeed — the hub is [Authorize]-gated"); + response.StatusCode.ShouldBeOneOf( + HttpStatusCode.Unauthorized, HttpStatusCode.Forbidden, + HttpStatusCode.Redirect, HttpStatusCode.Found); + } +}