fix(auth): OtOpcUa Task 1.5 review — pin JWT role-claim test + document issued-only JWT role key
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.
This commit is contained in:
@@ -23,9 +23,27 @@ public sealed class JwtTokenService
|
||||
public const string UsernameClaimType = ZbClaimTypes.Username;
|
||||
|
||||
/// <summary>
|
||||
/// Role claim type used in the JWT payload. Kept as the short "Role" key for the
|
||||
/// bearer token payload; the cookie-principal uses <see cref="ZbClaimTypes.Role"/>
|
||||
/// (= <see cref="ClaimTypes.Role"/>) for framework role resolution.
|
||||
/// Role claim type used in the JWT payload.
|
||||
/// <para>
|
||||
/// <b>Issued-only / no internal JwtBearer scheme:</b> OtOpcUa uses a single Cookie
|
||||
/// authentication scheme; the JWT is minted by the <c>/auth/token</c> endpoint and
|
||||
/// consumed externally (e.g. by OPC-UA clients or automation scripts). There is no
|
||||
/// <c>AddJwtBearer</c> pipeline in OtOpcUa — the cookie stores the
|
||||
/// <see cref="System.Security.Claims.ClaimsPrincipal"/> directly. Because no internal
|
||||
/// bearer validation path exists, the short "Role" key is intentionally used here rather
|
||||
/// than the long <see cref="ClaimTypes.Role"/> URI; external consumers receive exactly the
|
||||
/// key they expect.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>If a JwtBearer scheme is ever added:</b> the
|
||||
/// <see cref="Microsoft.IdentityModel.Tokens.TokenValidationParameters"/> passed to
|
||||
/// <c>AddJwtBearer</c> MUST set <c>RoleClaimType = JwtTokenService.RoleClaimType</c> (and
|
||||
/// <c>NameClaimType = JwtTokenService.UsernameClaimType</c>) so that
|
||||
/// <c>[Authorize(Roles=...)]</c> and <c>ClaimsPrincipal.IsInRole</c> resolve correctly.
|
||||
/// <see cref="BuildValidationParameters"/> is already wired to do this and MUST be used
|
||||
/// rather than constructing <see cref="Microsoft.IdentityModel.Tokens.TokenValidationParameters"/>
|
||||
/// ad hoc.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
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 <see cref="RoleClaimType"/>
|
||||
// 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
|
||||
/// <summary>
|
||||
/// Returns the validation parameters that the JwtBearer middleware should use. Centralised
|
||||
/// so the bearer pipeline can't drift from <see cref="TryValidate"/>.
|
||||
/// <para>
|
||||
/// <b>Note:</b> <see cref="TokenValidationParameters.RoleClaimType"/> is set to
|
||||
/// <see cref="RoleClaimType"/> and <see cref="TokenValidationParameters.NameClaimType"/> is
|
||||
/// set to <see cref="UsernameClaimType"/> so that <c>[Authorize(Roles=...)]</c> and
|
||||
/// <c>ClaimsPrincipal.IsInRole</c> resolve against the short role key ("Role") that
|
||||
/// <see cref="Issue"/> mints — not the JWT-default "role" or "name" keys. This is the
|
||||
/// required pairing whenever a JwtBearer scheme is wired.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
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,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -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
|
||||
/// <summary>
|
||||
/// 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
|
||||
/// <see cref="JwtTokenService.RoleClaimType"/> (currently the short "Role" key — intentionally
|
||||
/// NOT the long ClaimTypes.Role URI, because OtOpcUa is JWT-issued-only; see
|
||||
/// <see cref="JwtTokenService.RoleClaimType"/> docs for the rationale and the caveat that
|
||||
/// applies if a JwtBearer scheme is ever added).
|
||||
/// </summary>
|
||||
[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");
|
||||
|
||||
Reference in New Issue
Block a user