Support per-request OAuth auth context - #10
Conversation
|
Warning Review limit reached
More reviews will be available in 46 minutes and 34 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: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR replaces the token-validator entrypoints with a request-aware authorizer (authorize/3), threads Plug.Conn through metadata/authorization-server resolution and challenge construction, adds the Urchin.Auth.Authorizer behaviour, updates examples, and converts tests to the new authorizer/conn-aware contracts. ChangesConn-aware Authorizer Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/urchin/auth.ex (1)
211-215: 💤 Low valueRuntime validation of dynamic
resource_metadata_urlmay raise in request path.When
resource_metadata_urlis a function,validate_resource_metadata_url!/1is called at request time (line 214). If the function returns an invalid URL (non-absolute, non-http(s), or with a fragment), this will raise anArgumentErrorduring challenge generation, potentially causing an unhandled 500 instead of a graceful error response.Consider whether this should be a softer validation that logs and falls back, or if raising is the intended behavior for misconfigured resolvers.
🤖 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 `@lib/urchin/auth.ex` around lines 211 - 215, The current resource_metadata_url/2 calls validate_resource_metadata_url!/1 at request time which can raise an ArgumentError if a dynamic resolver returns an invalid URL; change this to safe validation by either calling a non-bang validator (e.g., validate_resource_metadata_url/1) or wrap the fun.(conn) + validation in a try/rescue to catch ArgumentError, log the invalid value, and return a safe fallback (nil or a configured static URL) instead of allowing the exception to bubble; update references to resource_metadata_url/1 and validate_resource_metadata_url!/1 accordingly so challenge generation never raises on malformed resolver output.
🤖 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.
Nitpick comments:
In `@lib/urchin/auth.ex`:
- Around line 211-215: The current resource_metadata_url/2 calls
validate_resource_metadata_url!/1 at request time which can raise an
ArgumentError if a dynamic resolver returns an invalid URL; change this to safe
validation by either calling a non-bang validator (e.g.,
validate_resource_metadata_url/1) or wrap the fun.(conn) + validation in a
try/rescue to catch ArgumentError, log the invalid value, and return a safe
fallback (nil or a configured static URL) instead of allowing the exception to
bubble; update references to resource_metadata_url/1 and
validate_resource_metadata_url!/1 accordingly so challenge generation never
raises on malformed resolver output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4104a4ae-5f92-4adf-b8db-9d9ddb30111a
📒 Files selected for processing (19)
CHANGELOG.mdREADME.mdSECURITY.mdexamples/authenticated_server.exsexamples/keycloak/README.mdexamples/keycloak/server.exslib/urchin/auth.exlib/urchin/auth/authorizer.exlib/urchin/auth/claims.exlib/urchin/auth/metadata.exlib/urchin/auth/plug.exlib/urchin/auth/token_validator.extest/support/auth_authorizers.extest/support/auth_validators.extest/urchin/auth/metadata_test.exstest/urchin/auth/plug_test.exstest/urchin/auth_test.exstest/urchin/endpoint_auth_test.exstest/urchin/transport/streamable_http_auth_test.exs
💤 Files with no reviewable changes (2)
- lib/urchin/auth/token_validator.ex
- test/support/auth_validators.ex
✅ Files skipped from review due to trivial changes (6)
- SECURITY.md
- examples/keycloak/README.md
- lib/urchin/auth/authorizer.ex
- CHANGELOG.md
- lib/urchin/auth/claims.ex
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/urchin/auth/metadata.ex
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
from_map/1 now reads both string- and atom-keyed maps so an authorizer that hand-builds atom-keyed claims is not silently dropped, and covers_resource?/2 returns false for a non-list audience instead of raising. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A missing or blank bearer token is now resolved by Urchin.Auth to a 401 :missing challenge before the authorizer runs, so the unauthenticated discovery bootstrap always gets the spec challenge instead of a 500 from an authorizer without a nil clause.
challenge/4 resolves the scope hint and resource_metadata URL itself; a per-request resolver that raises while the WWW-Authenticate header is built degrades to a valid challenge without the failed hint rather than escalating the 401/403 to a 500 with no header, and the scope hint is resolved only when a challenge is built. A foreign struct returned in {:ok, ...} is reported as a server error instead of crashing the pipeline.
Also dedupes the shared http(s)-URI validation and the challenge-kind type.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A :resource_metadata_url resolver preserves tenant context in the challenge (e.g. a query parameter), but Urchin serves the metadata document only at the static well-known paths derived from :resource; a resolver that points at a different path must be served by your own route or an external host. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Urchin.Auth.TokenValidatortovalidate/3so validators can resolve tenant-specific issuers, JWKS, introspection endpoints, or policy from the request.authorization_serversto be a per-request resolver for realm-aware Protected Resource Metadata.Notes
validate/3/ 3-arity callbacks.resourceremains static, so RFC 8707 audience binding continues to use the configured resource.Testing
devbox run -- mix formatdevbox run -- mix testSummary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation