fix(admin): enforce authentication on all Admin UI routes (Admin-001/002)
Admin-001: Routes.razor used a plain RouteView, so the page-level [Authorize] attributes on 11 pages were inert — every page, including mutating ones, was reachable fully unauthenticated. Admin-002: several pages (e.g. NewCluster, which writes config rows) carried no auth attribute at all. - Routes.razor: RouteView → AuthorizeRouteView with NotAuthorized / Authorizing slots; add RedirectToLogin component. - Program.cs: SetFallbackPolicy(RequireAuthenticatedUser) — secure by default for new pages/endpoints. - Login.razor: [AllowAnonymous] so login stays reachable; login page, /auth/* endpoints and static assets remain anonymous. - Add [Authorize] to the previously un-gated pages; NewCluster gated to the CanPublish (FleetAdmin) policy. Regression tests in PageAuthorizationTests pin that anonymous requests to protected/mutating routes are rejected and that login + static assets stay anonymously reachable. Admin test suite: 210/210 pass. Resolves code-review findings Admin-001 and Admin-002 (Critical). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -7,7 +7,7 @@
|
||||
| Review date | 2026-05-22 |
|
||||
| Commit reviewed | `76d35d1` |
|
||||
| Status | Reviewed |
|
||||
| Open findings | 12 |
|
||||
| Open findings | 10 |
|
||||
|
||||
## Checklist coverage
|
||||
|
||||
@@ -33,13 +33,13 @@
|
||||
| Severity | Critical |
|
||||
| Category | Security |
|
||||
| Location | `Components/Routes.razor:4-11`, `Program.cs:150` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** The router uses a plain `RouteView` (not `AuthorizeRouteView`), and `MapRazorComponents<App>()` is registered without `.RequireAuthorization()`. A page-level `[Authorize]` attribute on a routable Razor component is only enforced when the router is `AuthorizeRouteView` — with `RouteView` the attribute is inert. Consequently every page in the app, including those that carry `@attribute [Authorize]` (`ClusterDetail`, `DraftEditor`, `Reservations`, `RoleGrants`, `Certificates`, `VirtualTags`, `ScriptedAlarms`, `ScriptLog`, `DiffViewer`, `ImportEquipment`, `Account`), is reachable by a fully unauthenticated user. There is no authentication gate anywhere in the pipeline. An anonymous browser can read the full fleet configuration, audit log, certificates and ACLs, and exercise mutating pages (see Admin-002).
|
||||
|
||||
**Recommendation:** Replace `RouteView` with `AuthorizeRouteView` in `Routes.razor` (with a `<NotAuthorized>` slot that redirects to `/login`), or call `.RequireAuthorization()` on the `MapRazorComponents` endpoint with `/login` and `/auth/*` explicitly allowed anonymous. Add a fallback policy (`AddAuthorizationBuilder().SetFallbackPolicy(...)`) so new pages are secure-by-default. Re-verify every page after the gate is in place.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-22 — `Routes.razor` switched to `AuthorizeRouteView` with a `NotAuthorized` slot routing unauthenticated callers to `/login` via a new `RedirectToLogin` component; `AddAuthorizationBuilder().SetFallbackPolicy(RequireAuthenticatedUser())` makes pages secure-by-default; `Login.razor` opts out with `[AllowAnonymous]` so the login page and static assets stay anonymous. Covered by `PageAuthorizationTests` (verified failing pre-fix, passing post-fix).
|
||||
|
||||
### Admin-002
|
||||
|
||||
@@ -48,13 +48,13 @@
|
||||
| Severity | Critical |
|
||||
| Category | Security |
|
||||
| Location | `Components/Pages/Clusters/NewCluster.razor:1-7`, `Home.razor`, `Fleet.razor`, `Hosts.razor`, `AlarmsHistorian.razor`, `Clusters/ClustersList.razor`, `Clusters/Generations.razor`, `Drivers/FocasDetail.razor` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** Several routable pages carry no authorization attribute at all. Most critically `NewCluster` (`/clusters/new`) is a mutating page — its `CreateAsync` writes a new `ServerCluster` row and a draft generation. Combined with Admin-001 (the router does not enforce `[Authorize]` either), an unauthenticated user can create clusters and seed config-DB rows. `Home`, `Fleet`, `Hosts`, `AlarmsHistorian`, `ClustersList`, `Generations` and `FocasDetail` likewise expose fleet topology, host status, historian diagnostics and generation history to anonymous callers.
|
||||
|
||||
**Recommendation:** Add `@attribute [Authorize(...)]` to every routable page with the role/policy appropriate to its function (`NewCluster` and other write surfaces -> `CanPublish`/`CanEdit`; read pages -> an authenticated-user policy). A solution-wide fallback policy (see Admin-001) is the durable fix; per-page attributes remain the explicit declaration of intent.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-22 — `@attribute [Authorize]` added to every unprotected routable page (`Home`, `Fleet`, `Hosts`, `AlarmsHistorian`, `ClustersList`, `FocasDetail`, `ModbusAddressPreview`, `ModbusDiagnostics`); `NewCluster` gated with `[Authorize(Policy = "CanPublish")]` per the admin-ui.md FleetAdmin cluster-create flow. Re-triage note: `Clusters/Generations.razor` carries no `@page` directive — it is a child component of `ClusterDetail`, not a routable page, so it needs no attribute (it inherits the parent route's gate). The Admin-001 fallback policy is the durable secure-by-default backstop; the per-page attributes are the explicit declaration of intent. Covered by `PageAuthorizationTests`.
|
||||
|
||||
### Admin-003
|
||||
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
@page "/alarms/historian"
|
||||
@attribute [Microsoft.AspNetCore.Authorization.Authorize]
|
||||
@using Microsoft.AspNetCore.Components.Web
|
||||
@using ZB.MOM.WW.OtOpcUa.Admin.Services
|
||||
@using ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
@page "/clusters"
|
||||
@attribute [Microsoft.AspNetCore.Authorization.Authorize]
|
||||
@using ZB.MOM.WW.OtOpcUa.Admin.Services
|
||||
@using ZB.MOM.WW.OtOpcUa.Configuration.Entities
|
||||
@inject ClusterService ClusterSvc
|
||||
|
||||
@@ -1,4 +1,8 @@
|
||||
@page "/clusters/new"
|
||||
@* Cluster creation is a FleetAdmin operation per admin-ui.md "Add a new cluster" —
|
||||
CanPublish gates it (Admin-002). Without this attribute the page was reachable
|
||||
and its CreateAsync write path exploitable by any caller. *@
|
||||
@attribute [Microsoft.AspNetCore.Authorization.Authorize(Policy = "CanPublish")]
|
||||
@using System.ComponentModel.DataAnnotations
|
||||
@using Microsoft.AspNetCore.Components.Web
|
||||
@using ZB.MOM.WW.OtOpcUa.Admin.Services
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
@page "/drivers/focas/{InstanceId}"
|
||||
@attribute [Microsoft.AspNetCore.Authorization.Authorize]
|
||||
@using ZB.MOM.WW.OtOpcUa.Admin.Services
|
||||
@inject FocasDriverDetailService DetailSvc
|
||||
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
@page "/fleet"
|
||||
@attribute [Microsoft.AspNetCore.Authorization.Authorize]
|
||||
@using Microsoft.AspNetCore.Components.Web
|
||||
@using Microsoft.EntityFrameworkCore
|
||||
@using ZB.MOM.WW.OtOpcUa.Configuration
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
@page "/"
|
||||
@attribute [Microsoft.AspNetCore.Authorization.Authorize]
|
||||
@using Microsoft.AspNetCore.Components.Web
|
||||
@using ZB.MOM.WW.OtOpcUa.Admin.Services
|
||||
@using ZB.MOM.WW.OtOpcUa.Configuration.Entities
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
@page "/hosts"
|
||||
@attribute [Microsoft.AspNetCore.Authorization.Authorize]
|
||||
@using Microsoft.AspNetCore.Components.Web
|
||||
@using Microsoft.AspNetCore.SignalR.Client
|
||||
@using Microsoft.EntityFrameworkCore
|
||||
|
||||
@@ -1,4 +1,7 @@
|
||||
@page "/login"
|
||||
@* 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
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
@page "/modbus/address-preview"
|
||||
@attribute [Microsoft.AspNetCore.Authorization.Authorize]
|
||||
@using Microsoft.AspNetCore.Components.Web
|
||||
@using ZB.MOM.WW.OtOpcUa.Driver.Modbus
|
||||
@rendermode RenderMode.InteractiveServer
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
@page "/modbus/diagnostics/{DriverInstanceId}"
|
||||
@attribute [Microsoft.AspNetCore.Authorization.Authorize]
|
||||
@using Microsoft.AspNetCore.Components.Web
|
||||
@using ZB.MOM.WW.OtOpcUa.Admin.Services
|
||||
@rendermode RenderMode.InteractiveServer
|
||||
|
||||
@@ -0,0 +1,16 @@
|
||||
@* Server-side redirect to the login page for an unauthenticated request.
|
||||
Used by AuthorizeRouteView's NotAuthorized slot (Admin-001). The current
|
||||
path is carried through as returnUrl so the operator lands back where
|
||||
they aimed after signing in. *@
|
||||
@inject NavigationManager Nav
|
||||
|
||||
@code {
|
||||
protected override void OnInitialized()
|
||||
{
|
||||
var returnUrl = Nav.ToBaseRelativePath(Nav.Uri);
|
||||
var target = string.IsNullOrEmpty(returnUrl) || returnUrl == "login"
|
||||
? "login"
|
||||
: $"login?returnUrl={Uri.EscapeDataString("/" + returnUrl)}";
|
||||
Nav.NavigateTo(target, forceLoad: true);
|
||||
}
|
||||
}
|
||||
@@ -1,9 +1,30 @@
|
||||
@using Microsoft.AspNetCore.Components.Authorization
|
||||
@using Microsoft.AspNetCore.Components.Routing
|
||||
@using ZB.MOM.WW.OtOpcUa.Admin.Components.Layout
|
||||
|
||||
<Router AppAssembly="@typeof(Program).Assembly">
|
||||
<Found Context="routeData">
|
||||
<RouteView RouteData="@routeData" DefaultLayout="@typeof(MainLayout)"/>
|
||||
@* AuthorizeRouteView (not a plain RouteView) is what makes a page-level
|
||||
[Authorize] attribute actually enforced — with RouteView the attribute
|
||||
is inert (Admin-001). Unauthenticated users hit the NotAuthorized slot
|
||||
and are bounced to /login; the route is preserved as returnUrl. *@
|
||||
<AuthorizeRouteView RouteData="@routeData" DefaultLayout="@typeof(MainLayout)">
|
||||
<NotAuthorized>
|
||||
@if (context.User.Identity?.IsAuthenticated != true)
|
||||
{
|
||||
<RedirectToLogin/>
|
||||
}
|
||||
else
|
||||
{
|
||||
<LayoutView Layout="@typeof(MainLayout)">
|
||||
<p class="text-danger">You do not have permission to view this page.</p>
|
||||
</LayoutView>
|
||||
}
|
||||
</NotAuthorized>
|
||||
<Authorizing>
|
||||
<LayoutView Layout="@typeof(MainLayout)"><p>Authorizing…</p></LayoutView>
|
||||
</Authorizing>
|
||||
</AuthorizeRouteView>
|
||||
</Found>
|
||||
<NotFound>
|
||||
<LayoutView Layout="@typeof(MainLayout)"><p>Not found.</p></LayoutView>
|
||||
|
||||
@@ -28,9 +28,17 @@ builder.Services.AddAuthentication(CookieAuthenticationDefaults.AuthenticationSc
|
||||
o.ExpireTimeSpan = TimeSpan.FromHours(8);
|
||||
});
|
||||
|
||||
// Secure-by-default: the fallback policy requires an authenticated user for any
|
||||
// endpoint (and any routable page) that carries no explicit authorization metadata,
|
||||
// so a newly added page cannot accidentally ship anonymously reachable (Admin-001/002).
|
||||
// Pages/endpoints that must stay anonymous opt out with [AllowAnonymous] — the login
|
||||
// page, the /auth/* endpoints and static files all do.
|
||||
builder.Services.AddAuthorizationBuilder()
|
||||
.AddPolicy("CanEdit", p => p.RequireRole(AdminRoles.ConfigEditor, AdminRoles.FleetAdmin))
|
||||
.AddPolicy("CanPublish", p => p.RequireRole(AdminRoles.FleetAdmin));
|
||||
.AddPolicy("CanPublish", p => p.RequireRole(AdminRoles.FleetAdmin))
|
||||
.SetFallbackPolicy(new Microsoft.AspNetCore.Authorization.AuthorizationPolicyBuilder()
|
||||
.RequireAuthenticatedUser()
|
||||
.Build());
|
||||
|
||||
builder.Services.AddCascadingAuthenticationState();
|
||||
|
||||
|
||||
@@ -0,0 +1,140 @@
|
||||
using System.Net;
|
||||
using Microsoft.AspNetCore.Mvc.Testing;
|
||||
using Microsoft.Extensions.DependencyInjection;
|
||||
using Microsoft.Extensions.Hosting;
|
||||
using Shouldly;
|
||||
using Xunit;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.Admin.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Regression coverage for Admin-001 / Admin-002 — the router must enforce page-level
|
||||
/// <c>[Authorize]</c> attributes and the fallback authorization policy must keep every
|
||||
/// routable page (and mutating route) secure-by-default, while the login page, the
|
||||
/// <c>/auth/*</c> endpoints and static assets stay anonymously reachable.
|
||||
///
|
||||
/// These are HTTP-pipeline tests: they do not touch the config DB (the
|
||||
/// <see cref="OtOpcUaConfigDbContext"/> registration is lazy), so they run without the
|
||||
/// central SQL Server. The <see cref="FleetStatusPoller"/> hosted service is stripped out
|
||||
/// so the test host does not spin up a background DB poll loop.
|
||||
/// </summary>
|
||||
public sealed class PageAuthorizationTests : IClassFixture<PageAuthorizationTests.AdminAppFactory>
|
||||
{
|
||||
private readonly AdminAppFactory _factory;
|
||||
|
||||
public PageAuthorizationTests(AdminAppFactory factory) => _factory = factory;
|
||||
|
||||
/// <summary>
|
||||
/// A <see cref="WebApplicationFactory{TEntryPoint}"/> over the Admin app's
|
||||
/// <c>Program</c>. Removes the background poller so the host starts clean without DB
|
||||
/// access and never follows redirects so the auth gate is observable as a 302.
|
||||
/// </summary>
|
||||
public sealed class AdminAppFactory : WebApplicationFactory<Program>
|
||||
{
|
||||
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);
|
||||
});
|
||||
return base.CreateHost(builder);
|
||||
}
|
||||
|
||||
public HttpClient CreateNonRedirectingClient() =>
|
||||
CreateClient(new WebApplicationFactoryClientOptions { AllowAutoRedirect = false });
|
||||
}
|
||||
|
||||
public static readonly TheoryData<string> ProtectedRoutes = new()
|
||||
{
|
||||
"/", // Home — fleet overview
|
||||
"/fleet", // Fleet topology
|
||||
"/hosts", // Host status
|
||||
"/clusters", // Cluster list
|
||||
"/alarms/historian", // Historian diagnostics
|
||||
"/clusters/new", // NewCluster — MUTATING write surface (Admin-002)
|
||||
"/reservations", // CanPublish-gated page
|
||||
"/certificates", // FleetAdmin-gated page
|
||||
"/role-grants", // CanPublish-gated page
|
||||
"/account", // Authenticated-user page
|
||||
};
|
||||
|
||||
[Theory]
|
||||
[MemberData(nameof(ProtectedRoutes))]
|
||||
public async Task Anonymous_request_to_a_protected_page_is_rejected(string route)
|
||||
{
|
||||
using var client = _factory.CreateNonRedirectingClient();
|
||||
|
||||
var response = await client.GetAsync(route);
|
||||
|
||||
// The cookie auth handler challenges an unauthenticated request with a 302 to
|
||||
// the configured LoginPath; a 401/403 is also an acceptable "not allowed in".
|
||||
if (response.StatusCode == HttpStatusCode.Redirect ||
|
||||
response.StatusCode == HttpStatusCode.Found)
|
||||
{
|
||||
response.Headers.Location!.OriginalString.ShouldContain("/login",
|
||||
Case.Insensitive, $"anonymous GET {route} must bounce to the login page");
|
||||
}
|
||||
else
|
||||
{
|
||||
response.StatusCode.ShouldBeOneOf(HttpStatusCode.Unauthorized, HttpStatusCode.Forbidden);
|
||||
}
|
||||
|
||||
response.StatusCode.ShouldNotBe(HttpStatusCode.OK,
|
||||
$"anonymous GET {route} must not be served — page-level [Authorize] / fallback policy must gate it");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Anonymous_post_to_a_mutating_route_does_not_reach_the_handler()
|
||||
{
|
||||
using var client = _factory.CreateNonRedirectingClient();
|
||||
|
||||
// /clusters/new is the cluster-creation page; an anonymous POST to it must be
|
||||
// gated before any CreateAsync write path runs.
|
||||
var response = await client.PostAsync("/clusters/new",
|
||||
new FormUrlEncodedContent(Array.Empty<KeyValuePair<string, string>>()));
|
||||
|
||||
response.StatusCode.ShouldNotBe(HttpStatusCode.OK,
|
||||
"anonymous POST to /clusters/new must not be served");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Login_page_is_anonymously_reachable()
|
||||
{
|
||||
using var client = _factory.CreateNonRedirectingClient();
|
||||
|
||||
var response = await client.GetAsync("/login");
|
||||
|
||||
response.StatusCode.ShouldBe(HttpStatusCode.OK,
|
||||
"the login page must stay anonymous or operators can never sign in");
|
||||
var body = await response.Content.ReadAsStringAsync();
|
||||
body.ShouldContain("sign in", Case.Insensitive);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Static_assets_remain_anonymously_reachable()
|
||||
{
|
||||
using var client = _factory.CreateNonRedirectingClient();
|
||||
|
||||
// Vendored CSS served by the static-files middleware (not an endpoint) must not
|
||||
// be caught by the fallback authorization policy.
|
||||
foreach (var asset in new[] { "/app.css", "/theme.css" })
|
||||
{
|
||||
var response = await client.GetAsync(asset);
|
||||
response.StatusCode.ShouldBeOneOf(HttpStatusCode.OK, HttpStatusCode.NotModified);
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Blazor_framework_script_remains_anonymously_reachable()
|
||||
{
|
||||
using var client = _factory.CreateNonRedirectingClient();
|
||||
|
||||
// The Blazor runtime must load before any auth interaction can happen.
|
||||
var response = await client.GetAsync("/_framework/blazor.web.js");
|
||||
|
||||
response.StatusCode.ShouldBeOneOf(HttpStatusCode.OK, HttpStatusCode.NotModified);
|
||||
}
|
||||
}
|
||||
@@ -13,6 +13,7 @@
|
||||
<PackageReference Include="xunit.v3" Version="1.1.0"/>
|
||||
<PackageReference Include="Shouldly" Version="4.3.0"/>
|
||||
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.12.0"/>
|
||||
<PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="10.0.0"/>
|
||||
<PackageReference Include="xunit.runner.visualstudio" Version="3.0.2">
|
||||
<PrivateAssets>all</PrivateAssets>
|
||||
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
|
||||
|
||||
Reference in New Issue
Block a user