fix: support async authenticate() and propagate auth to SubRouters#1331
Open
fix: support async authenticate() and propagate auth to SubRouters#1331
Conversation
…1296) The authentication middleware wrapper is now async-aware: if `AuthenticationHandler.authenticate()` is overridden as an `async def`, the framework awaits it automatically, enabling async ORMs, HTTP clients, and other async I/O in authentication logic. Additionally, `configure_authentication()` and `include_router()` now propagate the parent app's authentication handler to SubRouters that have not configured their own, so `auth_required=True` routes on SubRouters work without a separate `configure_authentication()` call. Closes #1296 Made-with: Cursor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds async-capable authentication (authenticate may be awaited), propagates parent authentication_handler to included SubRouters when they lack one, adds integration tests for async and inherited auth subrouters, and adds a Discord release-notify job to the release CI workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router as "MiddlewareRouter / SubRouter"
participant Auth as "AuthenticationHandler"
participant Endpoint as "Protected Endpoint"
Client->>Router: HTTP Request (with/without Bearer token)
Router->>Auth: call authenticate(token)
Note right of Auth: authenticate may return value or coroutine
Auth-->>Router: Identity or None (awaited if coroutine)
alt Identity returned
Router->>Endpoint: call handler with identity
Endpoint-->>Client: 200 OK + body
else None returned
Router-->>Client: 401 Unauthorized + WWW-Authenticate
end
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. ✨ 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 |
![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
🧹 Nitpick comments (1)
.github/workflows/release-CI.yml (1)
223-239: Guard against Discord message-length failures.The composed payload can exceed Discord webhook content limits on large releases, causing
curl -fto fail the job. Consider truncating changelog before payload creation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release-CI.yml around lines 223 - 239, The Discord payload can exceed webhook content limits when CHANGELOG is large, causing the curl call using PAYLOAD to fail; before constructing MESSAGE/PAYLOAD, truncate CHANGELOG to a safe length (e.g., 1900–2000 chars) and append an ellipsis or “(truncated)” marker, ensure the truncated string is valid UTF-8, then use that truncated variable when forming MESSAGE and PAYLOAD that are sent to DISCORD_WEBHOOK_URL via curl; modify the block that builds MESSAGE, PAYLOAD, and calls curl to perform this truncation and marking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release-CI.yml:
- Around line 220-221: PREV_TAG may be empty on first release and the grep in
CHANGELOG can return non-zero (exit 1) when there are no matching lines, which
breaks the job; update the release script so PREV_TAG is set to a sensible
fallback (e.g., the repository root commit via git rev-list --max-parents=0 HEAD
or treat it as empty range) when the tag lookup (PREV_TAG=$(git tag ... | sed -n
'2p')) yields nothing, and make the CHANGELOG assignment resilient by preventing
grep from failing the script (e.g., append an unconditional success fallback
like "|| true" or use a conditional/alternate git log invocation) so CHANGELOG
becomes an empty string instead of causing an exit; adjust the lines that set
PREV_TAG and CHANGELOG accordingly to avoid failing under set -e.
- Line 212: The workflow step currently references actions/checkout@v3 which
uses Node.js 16; update the checkout action to a supported major (e.g.,
actions/checkout@v4 or later) by replacing the uses: entry for the checkout step
in the release-CI workflow; after updating the version (actions/checkout@v4 or
`@v5/`@v6) run the workflow or a local lint/CI dry run to ensure there are no
breaking input/API changes.
---
Nitpick comments:
In @.github/workflows/release-CI.yml:
- Around line 223-239: The Discord payload can exceed webhook content limits
when CHANGELOG is large, causing the curl call using PAYLOAD to fail; before
constructing MESSAGE/PAYLOAD, truncate CHANGELOG to a safe length (e.g.,
1900–2000 chars) and append an ellipsis or “(truncated)” marker, ensure the
truncated string is valid UTF-8, then use that truncated variable when forming
MESSAGE and PAYLOAD that are sent to DISCORD_WEBHOOK_URL via curl; modify the
block that builds MESSAGE, PAYLOAD, and calls curl to perform this truncation
and marking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e05fa1b-b198-45a8-a050-7e81a351414d
📒 Files selected for processing (8)
.github/workflows/release-CI.ymlintegration_tests/base_routes.pyintegration_tests/subroutes/__init__.pyintegration_tests/subroutes/auth_subrouter.pyintegration_tests/test_async_authentication.pyrobyn/__init__.pyrobyn/authentication.pyrobyn/router.py
Merging this PR will not alter performance
Performance Changes
Comparing 
|
actions/checkout@v3 uses Node.js 16 which reached EOL in 2024. Also guards the discord notification step against non-tag triggers and prevents grep from failing the job under set -e when PREV_TAG is empty (first release) or when filtered log output is empty. Made-with: Cursor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1296
authenticate()support: The auth middleware wrapper inadd_auth_middlewareis nowasync defand usesinspect.isawaitable()to detect andawaitasyncauthenticate()implementations. This enables users to use async ORMs, HTTP clients, and other async I/O directly in theirAuthenticationHandler.authenticate()method.configure_authentication()andinclude_router()now propagate the parent app's authentication handler to SubRouters that haven't configured their own. This works regardless of call order — whetherinclude_routerorconfigure_authenticationis called first.AuthenticationHandler.authenticate()docstring to document async override support.Changes
robyn/router.pyinner_handlerinadd_auth_middlewareis nowasync def; awaits result ifauthenticate()is asyncrobyn/__init__.pyinclude_routerandconfigure_authenticationpropagate auth handler to SubRoutersrobyn/authentication.pyintegration_tests/subroutes/auth_subrouter.pyintegration_tests/test_async_authentication.pyTest plan
Made with Cursor
Summary by CodeRabbit
New Features
Tests
Documentation
Chores