From d0777eee29784c5aecb9499c84858ac9926af9c5 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 2 Jun 2026 06:30:10 -0400 Subject: [PATCH] =?UTF-8?q?fix(auth):=20OtOpcUa=20Task=201.5=20review=20?= =?UTF-8?q?=E2=80=94=20pin=20JWT=20role-claim=20test=20+=20document=20issu?= =?UTF-8?q?ed-only=20JWT=20role=20key?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix 1 (test): Token_payload_uses_canonical_zb_claim_keys now asserts that the JWT payload carries at least one role under JwtTokenService.RoleClaimType ("Role"), pinning the role-key contract so a future rename is caught immediately. Adds a comment explaining why alice has roles (appsettings "ReadOnly"→"ConfigViewer" baseline). Adds missing `using ZB.MOM.WW.OtOpcUa.Security.Jwt` to the test file. Fix 2 (no-validation path — no AddJwtBearer in production pipeline): grep of src/ confirms no AddJwtBearer / JwtBearer scheme in ServiceCollectionExtensions or Host; the ServiceCollectionExtensions doc comment explicitly states "no JwtBearer parallel scheme". RoleClaimType intentionally stays the short "Role" key. Three changes: - RoleClaimType doc comment documents issued-only nature, the caveat that a JwtBearer scheme MUST use BuildValidationParameters(), and that BuildValidationParameters is already wired to set RoleClaimType+NameClaimType correctly. - Issue() inline comment at the role-mint site references RoleClaimType docs. - BuildValidationParameters() now sets RoleClaimType=RoleClaimType and NameClaimType=UsernameClaimType so that if it is ever passed to AddJwtBearer, role/name resolution is correct without any extra wiring. TryValidate() is refactored to delegate to BuildValidationParameters() so the two can never drift. All 35 security tests green. --- .../Jwt/JwtTokenService.cs | 53 +++++++++++++------ .../AuthEndpointsIntegrationTests.cs | 23 +++++++- 2 files changed, 60 insertions(+), 16 deletions(-) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Security/Jwt/JwtTokenService.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Security/Jwt/JwtTokenService.cs index a240f4b9..89981d9e 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Security/Jwt/JwtTokenService.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Security/Jwt/JwtTokenService.cs @@ -23,9 +23,27 @@ public sealed class JwtTokenService public const string UsernameClaimType = ZbClaimTypes.Username; /// - /// Role claim type used in the JWT payload. Kept as the short "Role" key for the - /// bearer token payload; the cookie-principal uses - /// (= ) for framework role resolution. + /// Role claim type used in the JWT payload. + /// + /// Issued-only / no internal JwtBearer scheme: OtOpcUa uses a single Cookie + /// authentication scheme; the JWT is minted by the /auth/token endpoint and + /// consumed externally (e.g. by OPC-UA clients or automation scripts). There is no + /// AddJwtBearer pipeline in OtOpcUa — the cookie stores the + /// directly. Because no internal + /// bearer validation path exists, the short "Role" key is intentionally used here rather + /// than the long URI; external consumers receive exactly the + /// key they expect. + /// + /// + /// If a JwtBearer scheme is ever added: the + /// passed to + /// AddJwtBearer MUST set RoleClaimType = JwtTokenService.RoleClaimType (and + /// NameClaimType = JwtTokenService.UsernameClaimType) so that + /// [Authorize(Roles=...)] and ClaimsPrincipal.IsInRole resolve correctly. + /// is already wired to do this and MUST be used + /// rather than constructing + /// ad hoc. + /// /// public const string RoleClaimType = "Role"; @@ -66,6 +84,8 @@ public sealed class JwtTokenService new(DisplayNameClaimType, displayName), new(UsernameClaimType, username), }; + // Role claims use the short RoleClaimType key ("Role") — see the + // doc comment for the issued-only rationale and the JwtBearer caveat. foreach (var role in roles) claims.Add(new Claim(RoleClaimType, role)); @@ -86,18 +106,9 @@ public sealed class JwtTokenService public bool TryValidate(string token, out ClaimsPrincipal? principal) { principal = null; - var key = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(_options.SigningKey)); - var parameters = new TokenValidationParameters - { - ValidateIssuer = true, - ValidIssuer = _options.Issuer, - ValidateAudience = true, - ValidAudience = _options.Audience, - ValidateLifetime = true, - ValidateIssuerSigningKey = true, - IssuerSigningKey = key, - ClockSkew = TimeSpan.Zero, - }; + // Delegate to BuildValidationParameters so RoleClaimType/NameClaimType are always in + // sync with the mint constants — no risk of this method diverging from the bearer path. + var parameters = BuildValidationParameters(); try { @@ -115,6 +126,14 @@ public sealed class JwtTokenService /// /// Returns the validation parameters that the JwtBearer middleware should use. Centralised /// so the bearer pipeline can't drift from . + /// + /// Note: is set to + /// and is + /// set to so that [Authorize(Roles=...)] and + /// ClaimsPrincipal.IsInRole resolve against the short role key ("Role") that + /// mints — not the JWT-default "role" or "name" keys. This is the + /// required pairing whenever a JwtBearer scheme is wired. + /// /// public TokenValidationParameters BuildValidationParameters() => new() { @@ -126,5 +145,9 @@ public sealed class JwtTokenService ValidateIssuerSigningKey = true, IssuerSigningKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(_options.SigningKey)), ClockSkew = TimeSpan.Zero, + // Pair these with the constants used at mint time so role/name resolution is correct + // if this is ever passed to AddJwtBearer. See RoleClaimType doc comment for rationale. + RoleClaimType = RoleClaimType, + NameClaimType = UsernameClaimType, }; } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Security.Tests/AuthEndpointsIntegrationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Security.Tests/AuthEndpointsIntegrationTests.cs index bbd92169..4bcb3452 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Security.Tests/AuthEndpointsIntegrationTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Security.Tests/AuthEndpointsIntegrationTests.cs @@ -19,6 +19,7 @@ using ZB.MOM.WW.OtOpcUa.Configuration.Entities; using ZB.MOM.WW.OtOpcUa.Configuration.Enums; using ZB.MOM.WW.OtOpcUa.Configuration.Services; using ZB.MOM.WW.OtOpcUa.Security.Endpoints; +using ZB.MOM.WW.OtOpcUa.Security.Jwt; using ZB.MOM.WW.OtOpcUa.Security.Ldap; namespace ZB.MOM.WW.OtOpcUa.Security.Tests; @@ -360,11 +361,18 @@ public sealed class AuthEndpointsIntegrationTests : IAsyncLifetime /// /// Task 1.5 — JWT payload uses canonical claim keys: after login and token issue the JWT /// payload segment MUST contain "zb:username" and "zb:displayname" keys (not the old short - /// "Username"/"DisplayName" strings). + /// "Username"/"DisplayName" strings), AND the role claim(s) MUST be carried under the key + /// (currently the short "Role" key — intentionally + /// NOT the long ClaimTypes.Role URI, because OtOpcUa is JWT-issued-only; see + /// docs for the rationale and the caveat that + /// applies if a JwtBearer scheme is ever added). /// [Fact] public async Task Token_payload_uses_canonical_zb_claim_keys() { + // Arrange — the appsettings baseline maps group "ReadOnly" → role "ConfigViewer", so alice + // (whose groups are ["ReadOnly"]) will carry at least one role in the issued JWT. + // No extra DB rows needed — the appsettings GroupToRole entry is always active. var client = NewClient(); var loginResp = await client.PostAsJsonAsync("/auth/login", @@ -391,6 +399,19 @@ public sealed class AuthEndpointsIntegrationTests : IAsyncLifetime "JWT payload must carry 'zb:displayname' claim (canonical ZbClaimTypes.DisplayName)"); displayNameEl.GetString().ShouldBe("Alice User"); + // Role claim(s) must be carried under JwtTokenService.RoleClaimType (= "Role"). + // This pins the role-key contract: any future rename of RoleClaimType will be caught here. + // The appsettings "ReadOnly" → "ConfigViewer" mapping guarantees alice has ≥1 role. + payloadJson.TryGetProperty(JwtTokenService.RoleClaimType, out var roleEl).ShouldBeTrue( + $"JWT payload must carry at least one role under JwtTokenService.RoleClaimType " + + $"(\"{JwtTokenService.RoleClaimType}\")"); + // The role value may be a string (single) or array (multiple); either way it must be non-empty. + if (roleEl.ValueKind == JsonValueKind.Array) + roleEl.EnumerateArray().Select(e => e.GetString()).ShouldNotBeEmpty( + "JWT role array must contain at least one role value"); + else + roleEl.GetString().ShouldNotBeNullOrEmpty("JWT role value must not be empty"); + // Old short-name literals must NOT be present. payloadJson.TryGetProperty("Username", out _).ShouldBeFalse( "JWT payload must not carry old 'Username' key after Task 1.5 migration");