fix(auth): wire Keycloak authority from injected keys + restore sub/aud claims (closes #147)#149
Conversation
…ud claims (#147) Three-layer fix for authed endpoints returning 409 (not 401) locally: 1. AddDefaultAuthentication read 'Keycloak:Url', but the Keycloak.AuthServices Aspire hosting integration injects 'Keycloak:AuthServerUrl' + 'Keycloak:Realm' separately. Authority was therefore null → no JWT bearer scheme registered → '.RequireAuthorization()' had no DefaultChallengeScheme → InvalidOperationException → mapped to 409. Now builds authority = {AuthServerUrl}/realms/{Realm} from the keys Aspire actually injects (issuer then matches the token: localhost:8080). 2. Token had no 'sub' claim. In Keycloak 25+ 'sub' moved into the 'basic' client scope; storefront's defaultClientScopes listed a bogus 'openid' (not a real client scope) and omitted 'basic'. Fixed to [basic, profile, email, roles]. (profile/email already resolved — proof built-in scopes are seeded — so basic is too.) 3. Token had no 'aud: nextaurora-api' (ValidateAudience=true requires it). Added an oidc-audience-mapper to the storefront client. Verified: token issuer already matches; this restores the scheme + sub + aud so the buyer flow validates. Closes the auth half of the local full-stack run (#147). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 56 minutes and 50 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughAuthority resolution now builds a Keycloak realm-based authority from ChangesOIDC Authentication Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ak accepts (#147) The storefront client's redirectUris were 'http://localhost:*/*' — but Keycloak only allows wildcards at the END of a URI, so a wildcard in the port position never matches and the SPA's 'http://localhost:5173/auth/callback' was rejected with 'Invalid parameter: redirect_uri' at the login redirect. Fixed to ['http://localhost:5173/*', 'http://localhost:5173'] (Vite's stable dev port) + matching webOrigins (for the token-endpoint CORS) + post.logout.redirect.uris. Applied the same change to the running instance via the admin API for the live session. Part of the #147 local auth fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@NextAurora.ServiceDefaults/Extensions.cs`:
- Around line 217-225: The change removed the old Keycloak:Url and broke tests
that expect the JwtBearer branch to run; restore compatibility by falling back
to the legacy key or update tests: modify the authority resolution in the code
that sets authority (the block referencing
builder.Configuration["Keycloak:AuthServerUrl"] and
builder.Configuration["Keycloak:Realm"]) to also check
builder.Configuration["Keycloak:Url"] (trimmed) when AuthServerUrl+Realm are not
present, or alternatively update the unit test setup to provide both
Keycloak:AuthServerUrl and Keycloak:Realm so the JwtBearer branch is exercised;
additionally add/adjust unit tests to cover the changed authority-resolution
logic (the lines that compute authority) so the new behavior is tested.
- Around line 207-225: Extensions.cs changes the authentication contract
(authority resolution logic inside AddDefaultAuthentication) but the PR omits
the required paired documentation updates; update docs/architecture.md and the
service-request-flow artifacts (docs/service-request-flow.svg and
docs/service-request-flow.excalidraw) to reflect the new authority resolution
behavior (Authentication:Authority override vs Keycloak:AuthServerUrl +
Keycloak:Realm), or if you cannot update them in this PR create a tracked
deferred issue and reference it in the PR description; ensure the doc change
explains the new resolution priority and shows the updated middleware/auth flow
so the repo’s canonical artifacts remain in sync with AddDefaultAuthentication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 75372934-fb05-4673-b5ce-f2b20522d1f2
📒 Files selected for processing (2)
NextAurora.AppHost/realms/nextaurora-realm.jsonNextAurora.ServiceDefaults/Extensions.cs
…ange (#147) React 19 StrictMode double-invokes effects in dev, so signinRedirectCallback ran twice → the single-use authorization code was consumed by the first exchange and the second failed with invalid_code (a 400 at the token endpoint, surfaced during #147 live verification). A useRef guard ensures the exchange runs exactly once. Dev-only symptom (StrictMode), but the guard is correct regardless. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eys (#147) The ServiceDefaultsJwtTests configured the authority via 'Keycloak:Url' — the dead key this PR replaced. AddDefaultAuthentication now builds the authority from 'Keycloak:AuthServerUrl' + 'Keycloak:Realm' (what Aspire actually injects), so the test setup must use those keys or no JWT scheme registers and the option assertions fail. Resolves to the same authority value; the assertions are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…147) Pairs with the AddDefaultAuthentication change (CodeRabbit doc-discipline catch): - authority now resolves from Authentication:Authority, else built from the Aspire-injected Keycloak:AuthServerUrl + Keycloak:Realm (was the dead Keycloak:Url) - no-op fallback note now explains the 401-challenge→409 symptom (#147) - fixed stale method name AddJwtBearerAuthentication → AddDefaultAuthentication The request-flow diagram is unchanged — middleware order + authenticate→authorize are identical; only the authority's config source changed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/OrderService.Tests.Unit/Application/ServiceDefaultsJwtTests.cs (1)
18-30: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRestructure test methods with explicit AAA phase markers and narrative comments.
Per coding guidelines (CLAUDE.md "Testing"), all tests must follow the Arrange → Act → Assert structure with
// ARRANGE,// ACT,// ASSERTphase markers (all caps) and narrative comments explaining what's being set up and why. This makes the test contract immediately readable for junior developers without requiring them to read the SUT.Currently, the three test methods lack explicit phase markers; comments exist but are not tied to marked phases. Restructure each test to make the phases explicit and move narrative comments to align with each phase.
📋 Proposed refactoring for test method structure (example: Test 1)
[Fact] public void AddServiceDefaults_WhenAuthorityConfigured_SetsExplicitSigningKeyValidation() { + // ARRANGE — build a host with authority configured via Aspire Keycloak injected keys, + // triggering AddDefaultAuthentication to wire JWT Bearer options. using var host = BuildHostWithAuthority(); + + // ACT — retrieve the configured JwtBearerOptions from the DI container. var options = host.Services .GetRequiredService<IOptionsMonitor<JwtBearerOptions>>() .Get(JwtBearerDefaults.AuthenticationScheme); + + // ASSERT — ValidateIssuerSigningKey must be true. Without this explicit flag, + // JWT Bearer's implicit default still validates (via JWKS), but making it explicit + // makes the security posture auditable and prevents a future config refactor + // from accidentally disabling signature validation. options.TokenValidationParameters.ValidateIssuerSigningKey.Should().BeTrue(); }Apply the same structure to the other two test methods (SetsTightClockSkew and RetainsCoreValidations), moving existing comments into their corresponding ASSERT phases and adding narrative to each phase.
Also applies to: 32-45, 47-60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/OrderService.Tests.Unit/Application/ServiceDefaultsJwtTests.cs` around lines 18 - 30, The test methods AddServiceDefaults_WhenAuthorityConfigured_SetsExplicitSigningKeyValidation, SetsTightClockSkew, and RetainsCoreValidations should be refactored to an explicit Arrange–Act–Assert layout: add // ARRANGE, // ACT, and // ASSERT comment markers (all caps) in each test, move existing narrative comments into the phase they describe, and ensure each phase has a brief narrative explaining what is being set up, the action performed, and the assertion being checked; for example in AddServiceDefaults_WhenAuthorityConfigured_SetsExplicitSigningKeyValidation put host creation and options retrieval under // ARRANGE with a short narrative, leave retrieving the JwtBearerOptions as the // ACT, and move the existing explanatory comment and the Should().BeTrue() check under // ASSERT with a narrative explaining why explicit signing-key validation is required.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/OrderService.Tests.Unit/Application/ServiceDefaultsJwtTests.cs`:
- Around line 18-30: The test methods
AddServiceDefaults_WhenAuthorityConfigured_SetsExplicitSigningKeyValidation,
SetsTightClockSkew, and RetainsCoreValidations should be refactored to an
explicit Arrange–Act–Assert layout: add // ARRANGE, // ACT, and // ASSERT
comment markers (all caps) in each test, move existing narrative comments into
the phase they describe, and ensure each phase has a brief narrative explaining
what is being set up, the action performed, and the assertion being checked; for
example in
AddServiceDefaults_WhenAuthorityConfigured_SetsExplicitSigningKeyValidation put
host creation and options retrieval under // ARRANGE with a short narrative,
leave retrieving the JwtBearerOptions as the // ACT, and move the existing
explanatory comment and the Should().BeTrue() check under // ASSERT with a
narrative explaining why explicit signing-key validation is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a7d2ad94-785c-41b4-9bd0-54c65ff28850
📒 Files selected for processing (2)
frontend/src/features/auth/AuthCallback.tsxtests/OrderService.Tests.Unit/Application/ServiceDefaultsJwtTests.cs
1 participant
What
Authenticated endpoints returned 409 (not 401) for every request when running the full stack locally —
System.InvalidOperationException: No authenticationScheme was specified, and there was no DefaultChallengeScheme found. Three layers, all fixed:AddDefaultAuthenticationreadKeycloak:Url, but theKeycloak.AuthServices.Aspire.Hostingintegration injectsKeycloak:AuthServerUrl+Keycloak:Realmseparately. Authority resolved to null → no JWT bearer scheme registered → no default challenge scheme → 409. Now buildsauthority = {AuthServerUrl}/realms/{Realm}from the injected keys (reads the keys, doesn't hardcode the URL).sub(realm). Keycloak 25+ movedsubinto thebasicclient scope; the storefront client listed a bogusopenidscope (not a real client scope) and omittedbasic. FixeddefaultClientScopes→[basic, profile, email, roles]. (profile/emailalready resolved, proving the built-ins are seeded — sobasicis too.)aud: nextaurora-api(realm).ValidateAudience=truerequires it; added anoidc-audience-mapperto the storefront client.Verification (live, against the AppHost)
sub✓,aud: nextaurora-api✓,iss: localhost:8080(matches authority) ✓Build clean (analyzers-as-errors). Issuer matches because both the SPA (
VITE_KEYCLOAK_URL=localhost:8080) and the API resolve to the same Keycloak base.Notes
Found + fixed during the frontend epic #130 live verification. Companion local-run fixes already merged in #146. The remaining local-run item is #148 (Wolverine internal emulator queues — non-fatal, degrades saga). Closes #147.
🤖 Generated with Claude Code
Summary by CodeRabbit
Configuration Updates
Bug Fixes