test: add component coverage gate and auth UI tests#2
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a unit-test coverage gate and expands Blazor UI test coverage around authentication/authorization-driven UX, while also improving role-claim handling for Auth0 and adding a user profile page.
Changes:
- Enable Coverlet collection with a line-coverage threshold in
Unit.Tests. - Add
RoleClaimsHelper+ Auth0 token hook to normalize roles intoClaimTypes.Role, plus a new/profilepage that displays identity/roles/claims. - Add bUnit smoke/component tests for NavMenu, profile, and key pages; update layout/nav and Manage Roles UI styling.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit.Tests/Unit.Tests.csproj | Enables coverage collection/threshold gating and adds bUnit dependencies. |
| tests/Unit.Tests/Testing/TestAuthorizationService.cs | Adds a lightweight authorization service for component testing. |
| tests/Unit.Tests/Security/RoleClaimsHelperTests.cs | Adds unit coverage for role claim parsing/aggregation behaviors. |
| tests/Unit.Tests/ResultTests.cs | Adds baseline unit tests for Result/Result<T> behaviors. |
| tests/Unit.Tests/Features/UserManagement/ProfileTests.cs | Adds bUnit tests for the new Profile UI (happy path + fallbacks). |
| tests/Unit.Tests/Components/RazorSmokeTests.cs | Adds broad “renders / basic interactions” coverage for key Razor components/pages. |
| tests/Unit.Tests/Components/Layout/NavMenuTests.cs | Adds bUnit tests for auth-dependent links and theme JS interop behavior. |
| src/Web/Security/RoleClaimsHelper.cs | Introduces helper utilities to expand/normalize role claims from Auth0. |
| src/Web/Properties/AssemblyInfo.cs | Grants internals access to the unit test assembly. |
| src/Web/Program.cs | Hooks OIDC token validation to add normalized role claims using RoleClaimsHelper. |
| src/Web/Features/UserManagement/Profile.razor | Adds a new authorized Profile page showing identity details, roles, and claims. |
| src/Web/Features/UserManagement/ManageRoles.razor | Updates Manage Roles UI styling/structure and error display. |
| src/Web/Components/Pages/Home.razor | Updates the home page greeting/copy. |
| src/Web/Components/Layout/NavMenu.razor | Reworks navigation UI; adds profile link + theme controls + nav state handling. |
| src/Web/Components/Layout/MainLayout.razor | Updates overall page layout structure and footer content. |
|
|
||
| public static IReadOnlyList<string> GetRoles(ClaimsPrincipal user, IEnumerable<string>? roleClaimTypes = null) | ||
| { | ||
| var types = (roleClaimTypes ?? DefaultRoleClaimTypes.Append(ClaimTypes.Role)) |
There was a problem hiding this comment.
When roleClaimTypes is provided, GetRoles no longer includes ClaimTypes.Role in the allowed claim types. That makes the parameter behave like a full replacement rather than “additional claim types”, and can cause standard role claims to be ignored. Consider always appending ClaimTypes.Role regardless of whether a custom list is supplied.
| var types = (roleClaimTypes ?? DefaultRoleClaimTypes.Append(ClaimTypes.Role)) | |
| var types = (roleClaimTypes ?? DefaultRoleClaimTypes) | |
| .Append(ClaimTypes.Role) |
| <input type="checkbox" id="menu-toggle" class="peer sr-only" aria-hidden="true"/> | ||
| <label for="menu-toggle" | ||
| class="md:hidden cursor-pointer p-2 rounded hover:bg-primary-500 dark:hover:bg-primary-600 text-white" | ||
| aria-label="Toggle navigation"> | ||
| <svg class="w-6 h-6" fill="none" stroke="currentColor" viewBox="0 0 24 24" aria-hidden="true"> |
There was a problem hiding this comment.
The mobile nav toggle uses a visually-hidden checkbox with aria-hidden="true" and a <label> as the interactive control. Because the checkbox is hidden from assistive technology and the label isn’t keyboard-focusable by default, this is not accessible for keyboard/screen-reader users. Consider using a real <button> with aria-expanded/aria-controls (or make the control focusable and expose state).
| private async void OnLocationChanged(object? sender, LocationChangedEventArgs e) | ||
| { | ||
| try | ||
| { | ||
| await InvokeAsync(SyncThemeFromJs); |
There was a problem hiding this comment.
OnLocationChanged is an async void handler that swallows all exceptions. This can hide real failures (and async void exceptions can crash the renderer in some cases). Consider making the handler synchronous and starting a fire-and-forget task with explicit exception handling/logging, or at least capture/log exceptions in the catch block.
| <button type="button" class="btn-close" @onclick="() => _error = null"></button> | ||
| <div class="rounded-lg border border-red-300 bg-red-50 text-red-700 px-4 py-3 text-sm mb-4 flex items-center justify-between" role="alert"> | ||
| <span>@_error</span> | ||
| <button type="button" class="ml-4 cursor-pointer text-red-700 hover:text-red-900" @onclick="() => _error = null">×</button> |
There was a problem hiding this comment.
The dismiss button for the error alert only renders “×” and has no accessible name. Add an aria-label (e.g., “Close error”) so screen readers can identify the control.
| <button type="button" class="ml-4 cursor-pointer text-red-700 hover:text-red-900" @onclick="() => _error = null">×</button> | |
| <button type="button" class="ml-4 cursor-pointer text-red-700 hover:text-red-900" aria-label="Close error" @onclick="() => _error = null">×</button> |
| @@ -0,0 +1,4 @@ | |||
| using System.Runtime.CompilerServices; | |||
|
|
|||
| [assembly: InternalsVisibleTo("MyBlog.Unit.Tests")] | |||
There was a problem hiding this comment.
InternalsVisibleTo includes MyBlog.Unit.Tests, but the unit test assembly name appears to be Unit.Tests (default from the project file name). The extra attribute likely won’t match any assembly and unnecessarily widens friend-assembly access. Consider keeping only the correct assembly name (or explicitly set <AssemblyName> in the test csproj and align this list).
| [assembly: InternalsVisibleTo("MyBlog.Unit.Tests")] |
| catch (JsonException) | ||
| { |
There was a problem hiding this comment.
ExpandRoleValues falls through to the comma-splitting logic when JSON parsing fails. For a malformed JSON array value that still contains commas, this will produce broken role strings like ["Admin" rather than failing safely. Consider returning an empty list (or a single trimmed value) when JsonDocument.Parse throws, and avoid swallowing the exception silently.
| catch (JsonException) | |
| { | |
| catch (JsonException exception) | |
| { | |
| System.Diagnostics.Trace.TraceWarning( | |
| "Failed to parse role claim value as JSON array: {0}", | |
| exception.Message); | |
| return []; |
mpaulosky
left a comment
mpaulosky
left a comment
There was a problem hiding this comment.
🧙 Gandalf's Security Review — Gate Assessment
Verdict: ❌ REQUEST CHANGES — 2 blocking issues before merge
🔴 [HIGH] Open Redirect — src/Web/Program.cs:111
returnUrl from the query string is passed directly to WithRedirectUri(returnUrl ?? "/") with no local-path validation. An attacker can craft /Account/Login?returnUrl=https://evil.com and after Auth0 authentication the user lands on a phishing site.
Fix required:
var safeReturn = Uri.IsWellFormedUriString(returnUrl, UriKind.Relative) ? returnUrl : "/";
WithRedirectUri(safeReturn)�� [MEDIUM] Null dereference breaks all logins — src/Web/Program.cs:49
existingOnTokenValidated is captured before PostConfigure runs; Auth0 SDK may leave it null. await existingOnTokenValidated(context) throws NullReferenceException silently, breaking login for all users.
Fix required:
if (existingOnTokenValidated is not null)
await existingOnTokenValidated(context);ℹ️ [INFO] Full JWT claim exposure on /profile
All claims (sub, auth_time, nonce) rendered to authenticated users. No immediate risk but consider filtering to display-safe claims only.
✅ Clean Areas
RoleClaimsHelper— correct deduplication, no privilege escalation path- Route authorization — all endpoints correctly gated
TestAuthorizationService— properlyinternal sealed, scoped to test assembly- Middleware pipeline order — correct
- No secrets in source
The security fundamentals are solid — role normalization, route guards, and auth middleware are all correct. Fix the open redirect and null guard, then this gets an ✅.
— Gandalf, Squad Reviewer
mpaulosky
left a comment
mpaulosky
left a comment
There was a problem hiding this comment.
⚒️ Gimli's Test Review
Verdict: REQUEST CHANGES — squad rules violations
🔴 Missing File Headers (Rule #6)
ALL 6 new test files lack the required block-format copyright header present in all existing test files.
Affected:
tests/Unit.Tests/Components/Layout/NavMenuTests.cstests/Unit.Tests/Components/RazorSmokeTests.cstests/Unit.Tests/Features/UserManagement/ProfileTests.cstests/Unit.Tests/ResultTests.cstests/Unit.Tests/Security/RoleClaimsHelperTests.cstests/Unit.Tests/Testing/TestAuthorizationService.cs
🔴 Missing AAA Comments (Rule #3)
No test methods use // Arrange, // Act, // Assert markers as required by charter and demonstrated in existing tests/Unit.Tests/Handlers/*.cs files.
✅ Passes
- FluentAssertions
.Should()used throughout ✅ - NSubstitute for mocking ✅
- File-scoped namespaces ✅
- Correct namespace pattern
MyBlog.Unit.Tests.{Folder}✅ - bUnit configured properly with
BunitContext✅ - 89% coverage threshold configured ✅
- No
Dto.Emptycomparison anti-patterns ✅
Coverage quality is excellent — NavMenu auth states, Profile claims, RoleClaimsHelper theory tests, smoke tests. Just needs the two formatting rules applied.
— Gimli, Squad Tester
mpaulosky
left a comment
mpaulosky
left a comment
There was a problem hiding this comment.
🧙 Gandalf's Gate Review — PR #2: test: add component coverage gate and auth UI tests
"You shall not pass — until these are fixed."
Branch: squad/coverage-test-hardening → main
Verdict:
This PR ships genuinely valuable work — the bUnit smoke suite, the Auth0 role-normalization pipeline, and the Coverlet gate are all the right investments. But two P1 defects (one crash, one security) must land before this touches main. I also have two accessibility P2s and one additional security observation.
❌ P1 — MUST FIX (Blocking)
1. Program.cs — Null-reference crash in the Auth pipeline
var existingOnTokenValidated = options.Events.OnTokenValidated;
options.Events.OnTokenValidated = async context =>
{
await existingOnTokenValidated(context); // ← NullReferenceExceptionOpenIdConnectEvents.OnTokenValidated is null by default in a fresh OIDC registration. The Auth0 SDK may or may not pre-populate it before PostConfigure runs — if it does not, every token validation will throw NullReferenceException, silently aborting the login flow. The fix is a one-liner null guard:
if (existingOnTokenValidated is not null)
await existingOnTokenValidated(context);This is a silent authentication failure that will not appear in tests (token validation is not exercised by the bUnit suite) but will manifest in every production login attempt on a fresh deployment.
2. RoleClaimsHelper.cs:52 — Malformed JSON produces broken role strings (Copilot comment #6 — confirmed P1)
Copilot flagged this correctly. The fallthrough path is:
Input: '["Admin","Auth ← malformed/truncated JSON
→ JsonException caught, silently swallowed
→ falls through to comma-split
→ produces: [ "[\"Admin\"", "\"Auth" ]
A claim value beginning with [ that is not valid JSON will produce role names containing JSON syntax characters (brackets, quotes). These garbage strings will never match any [Authorize(Roles=...)] attribute, so they won't grant unauthorized access. But they will silently discard legitimate roles for a user whose token is slightly malformed (e.g., an older Auth0 action producing non-standard JSON), causing denied-access false negatives.
The fix should log the JsonException (even at Debug level) and/or explicitly return [] after a parse failure rather than falling through to comma splitting:
catch (JsonException ex)
{
logger.LogDebug(ex, "Failed to parse role claim as JSON array: {Value}", trimmed);
return []; // do not fall through — malformed JSON is not a CSV
}⚠️ P2 — Should Fix (Non-blocking but expected before merge)
3. NavMenu.razor:19 — Mobile hamburger inaccessible to AT (Copilot comment #2 — confirmed P2)
<input type=checkbox id="menu-toggle" class="peer sr-only" aria-hidden="true"/>
<label for="menu-toggle" aria-label="Toggle navigation"> … </label>The <label> has aria-label — good. But aria-hidden="true" on the checkbox removes its checked/unchecked state from the accessibility tree. Screen reader users will activate the label (announcing "Toggle navigation"), but the AT will report no state change because the associated control is hidden from it. The mobile menu will silently open/close with no feedback. Remove aria-hidden from the checkbox input — the sr-only class already positions it off-screen visually.
4. ManageRoles.razor:23 — Dismiss button has no accessible name (Copilot comment #4 — confirmed P2)
<button type="button" … @onclick="() => _error = null">×</button>× (×) is a Unicode symbol. Screen readers may announce it as "times" or "multiplication sign". Add aria-label="Dismiss error" to give the control a meaningful name.
5. Profile.razor — Full claims table exposes OIDC internals (new finding)
The page renders _user.Claims.OrderBy(…).ToList() with no filtering. This includes internal OIDC claims: nonce, c_hash, at_hash, aud, iss, iat, exp, and any Auth0 internal metadata claims. While Blazor HTML-encodes output (no XSS risk), this is an information-disclosure concern:
- Exposes audience identifiers and issuer URLs that aid OIDC enumeration.
- Exposes the raw
subidentifier used as the Auth0 user ID (already shown separately, but doubly visible). - If Auth0 ever includes a short-lived opaque token claim, it appears here verbatim.
Recommendation: filter to an allowlist of user-facing claim types, or add a note in the UI (and PR description) marking this page as a developer/debug feature restricted in production.
6. TestAuthorizationService — Policy-based overload always grants authenticated users (new finding)
public Task<AuthorizationResult> AuthorizeAsync(ClaimsPrincipal user, object? resource, string policyName)
=> Task.FromResult(user.Identity?.IsAuthenticated == true
? AuthorizationResult.Success()
: AuthorizationResult.Failed());This means every bUnit test that hits a [Authorize(Policy="SomePolicy")] page will pass authorization regardless of what the policy actually requires. If a future policy (e.g., RequireEmailVerified) is added, the tests will not catch a missing guard. Consider routing policyName == Auth0Constants.AuthenticationScheme explicitly, or at minimum document the deliberate simplification.
🟡 P3 — Nice to Have
7. RoleClaimsHelper.cs:83 — GetRoles parameter fully replaces defaults (Copilot comment #1 — P3)
var types = (roleClaimTypes ?? DefaultRoleClaimTypes.Append(ClaimTypes.Role))…When roleClaimTypes is passed, DefaultRoleClaimTypes are excluded. The Profile page calls GetRoles(_user) with no argument, so it works correctly today. But AddRoleClaims takes explicit types from startup config — the two code paths use different type sets, which means the claims added at login may not perfectly match what the Profile page displays. This is a latent inconsistency, not a current bug. Document the intended semantics.
8. NavMenu.razor:161 — async void on event handler (Copilot comment #3 — P3)
The empty catch {} makes this functionally safe, but async void is still an anti-pattern. If a future contributor removes the try/catch, it becomes a process-crashing unhandled exception. Prefer a Task-returning inner method and fire-and-forget pattern:
private void OnLocationChanged(object? sender, LocationChangedEventArgs e)
=> InvokeAsync(() => SyncThemeFromJs()).ContinueWith(_ => { });9. AssemblyInfo.cs — Dual InternalsVisibleTo entries (Copilot comment #5 — P3, acceptable)
Both "MyBlog.Unit.Tests" and "Unit.Tests" are present. The csproj sets <RootNamespace>MyBlog.Unit.Tests</RootNamespace> but no <AssemblyName> override, so the actual assembly name defaults to Unit.Tests (the project filename). The first entry is redundant but harmless. Remove InternalsVisibleTo("MyBlog.Unit.Tests") to avoid confusion, but this is not blocking.
📋 Merge Checklist
| # | Issue | File | Severity | Status |
|---|---|---|---|---|
| 1 | Null-ref crash on OnTokenValidated |
Program.cs |
P1 | �� Must fix |
| 2 | Malformed JSON fallthrough → broken role strings | RoleClaimsHelper.cs:52 |
P1 | 🔴 Must fix |
| 3 | Mobile hamburger AT inaccessible (aria-hidden on checkbox) |
NavMenu.razor:19 |
P2 | 🟡 Should fix |
| 4 | Dismiss button no accessible name | ManageRoles.razor:23 |
P2 | 🟡 Should fix |
| 5 | Profile page exposes all OIDC internal claims | Profile.razor |
P2 | 🟡 Should fix |
| 6 | Test auth service policy overload masks regressions | TestAuthorizationService.cs |
P2 | 🟡 Should fix |
| 7 | GetRoles vs AddRoleClaims type-set inconsistency |
RoleClaimsHelper.cs:83 |
P3 | 🔵 Nice to have |
| 8 | async void on nav event handler |
NavMenu.razor:161 |
P3 | 🔵 Nice to have |
| 9 | Redundant InternalsVisibleTo entry |
AssemblyInfo.cs:3 |
P3 | 🔵 Nice to have |
| 10 | Branch conflicts with main |
— | Infra | 🔴 Must resolve |
The auth pipeline crash (#1) and the JSON fallthrough (#2) are gates. Fix those two, address the P2 accessibility issues, and this PR earns the ring.
— Gandalf, Squad Reviewer
Summary
Closes #
Type of Change
Domain Affected
Self-Review Checklist
Code Quality
dotnet build IssueTrackerApp.slnx --configuration Release— 0 errors, 0 warningsdotnet test IssueTrackerApp.slnx --configuration Release --no-build— all passArchitecture
Command/Query/Handler/Validatornaming conventionssealedWeborPersistence.*projectsResult<T>/ResultErrorCodeused for expected failures (no exception-driven control flow)Domain.DTOs; Models are inDomain.ModelsTests
[Collection("XxxIntegration")])IssueDto.Empty/CommentDto.Emptyinstances directlySecurity (check if security-relevant)
RequireAuthorization/ policy appliedMarkupStringused with user-supplied contentMerge Readiness
main(no merge conflicts)Screenshots / Evidence
Notes for Reviewers