Implement the /authorized endpoint #210
Conversation
Follow-up patch for the Authentication work, this patch implements the /authorized endpoint to test the user authentication. Same as previously implemented in road-core/service. Signed-off-by: Lucas Alvares Gomes <lucasagomes@gmail.com>
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI_App
participant Auth_Dependency
participant AuthorizedEndpoint
Client->>FastAPI_App: POST /authorized
FastAPI_App->>Auth_Dependency: Verify credentials
Auth_Dependency-->>FastAPI_App: user_id, username (or raise exception)
FastAPI_App->>AuthorizedEndpoint: Call handler with user info
AuthorizedEndpoint-->>Client: 200 AuthorizedResponse (or 400/403 error)
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (3)
tests/unit/app/endpoints/test_authorized.py (3)
33-34: Fix misleading docstring.The docstring mentions "custom user ID" but the test is actually testing unauthorized access.
- """Test the authorized endpoint handler with a custom user ID.""" + """Test the authorized endpoint handler when user is not authorized."""
18-23: Consider using a more robust Request object creation.The minimal Request object creation works for these tests, but consider using FastAPI's TestClient or creating a helper function for more robust request creation if this pattern is used elsewhere.
Example helper function:
def create_test_request() -> Request: """Create a minimal test request object.""" return Request( scope={ "type": "http", "method": "POST", "path": "/authorized", "query_string": b"", "headers": [], } )Also applies to: 43-48
1-55: Consider adding edge case tests for better coverage.The current tests cover the basic success and failure scenarios. Consider adding tests for:
- Different types of authentication errors (e.g., invalid tokens, expired tokens)
- Malformed requests
- Testing the actual response model validation
Would you like me to generate additional test cases for better coverage?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/app/endpoints/authorized.py(1 hunks)src/app/endpoints/feedback.py(2 hunks)src/app/endpoints/query.py(2 hunks)src/app/routers.py(2 hunks)src/models/responses.py(1 hunks)tests/unit/app/endpoints/test_authorized.py(1 hunks)tests/unit/app/test_routers.py(3 hunks)tests/unit/models/test_responses.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
tests/unit/models/test_responses.py (1)
src/models/responses.py (4)
QueryResponse(25-46)StatusResponse(215-244)AuthorizedResponse(247-268)UnauthorizedResponse(271-285)
src/app/endpoints/feedback.py (1)
src/models/responses.py (2)
UnauthorizedResponse(271-285)ForbiddenResponse(288-300)
src/app/endpoints/authorized.py (2)
src/auth/__init__.py (1)
get_auth_dependency(14-38)src/models/responses.py (3)
AuthorizedResponse(247-268)UnauthorizedResponse(271-285)ForbiddenResponse(288-300)
src/app/endpoints/query.py (1)
src/models/responses.py (3)
QueryResponse(25-46)UnauthorizedResponse(271-285)ForbiddenResponse(288-300)
tests/unit/app/endpoints/test_authorized.py (1)
src/app/endpoints/authorized.py (1)
authorized_endpoint_handler(34-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-pr
🔇 Additional comments (17)
src/models/responses.py (3)
247-268: LGTM: Well-structured authorization response modelThe
AuthorizedResponsemodel follows the established patterns in the codebase with proper documentation, type hints, and examples for API documentation.
271-285: LGTM: Consistent unauthorized response modelThe
UnauthorizedResponsemodel is properly structured with appropriate documentation and examples for the 400 status code scenarios.
288-300: LGTM: Appropriate inheritance patternThe
ForbiddenResponseinheriting fromUnauthorizedResponseis a good design choice since both represent authorization failures with similar structure, differing only in the example message.src/app/routers.py (2)
14-14: LGTM: Proper router importThe import of the
authorizedrouter follows the established pattern and is consistent with other endpoint imports.
32-32: LGTM: Consistent router inclusionThe inclusion of the
authorizedrouter with the/v1prefix follows the established pattern used by other endpoints in the application.tests/unit/app/test_routers.py (3)
16-16: LGTM: Proper test importThe import of the
authorizedrouter in the test follows the established pattern and ensures the test can verify the new router's inclusion.
38-38: LGTM: Correct router count updateThe expected router count is correctly updated from 8 to 9 to account for the new authorized router.
47-47: LGTM: Comprehensive router verificationThe assertion properly verifies that the authorized router is included in the list of routers, maintaining complete test coverage.
tests/unit/models/test_responses.py (3)
1-6: LGTM: Proper test importsThe import statement is correctly updated to include the new response models that need to be tested.
38-48: LGTM: Comprehensive AuthorizedResponse testThe test properly verifies the constructor behavior and attribute assignment for the
AuthorizedResponsemodel, following the established testing pattern.
51-59: LGTM: Comprehensive UnauthorizedResponse testThe test properly verifies the constructor behavior and attribute assignment for the
UnauthorizedResponsemodel, following the established testing pattern.src/app/endpoints/authorized.py (1)
17-30: LGTM: Well-structured response definitionsThe response definitions are comprehensive and properly document the different HTTP status codes with appropriate models and descriptions.
src/app/endpoints/feedback.py (2)
13-18: LGTM: Imports are correctly added for authorization response models.The imports for
UnauthorizedResponseandForbiddenResponseare properly added and align with the models defined insrc/models/responses.py.
30-37: LGTM: Response documentation properly added for authorization errors.The response documentation for 400 and 403 status codes is correctly structured and uses the appropriate models. The descriptions clearly indicate the purpose of each status code.
src/app/endpoints/query.py (2)
26-26: LGTM: Import statement correctly updated.The import statement properly includes the new
UnauthorizedResponseandForbiddenResponsemodels alongside the existingQueryResponse.
47-54: LGTM: Response documentation properly added for authorization errors.The response documentation for 400 and 403 status codes is correctly structured and maintains consistency with the feedback endpoint. The descriptions and model references are appropriate.
tests/unit/app/endpoints/test_authorized.py (1)
9-31: LGTM: Test correctly validates successful authorization.The test properly mocks the auth dependency and validates the response structure. The assertions are appropriate for testing the successful authorization scenario.
| @@ -0,0 +1,38 @@ | |||
| """Handler for REST API call to authorized endpoint.""" | |||
|
|
|||
| import asyncio | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused asyncio import
The asyncio import is only used for the problematic asyncio.run() call. After fixing the async/sync pattern, this import should be removed.
-import asyncio📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import asyncio |
🤖 Prompt for AI Agents
In src/app/endpoints/authorized.py at line 3, remove the unused import of
asyncio since it is only needed for the asyncio.run() call, which will be
removed when fixing the async/sync pattern. Delete the line importing asyncio to
clean up unused imports.
| @router.post("/authorized", responses=authorized_responses) | ||
| def authorized_endpoint_handler(_request: Request) -> AuthorizedResponse: | ||
| """Handle request to the /authorized endpoint.""" | ||
| # Ignore the user token, we should not return it in the response | ||
| user_id, user_name, _ = asyncio.run(auth_dependency(_request)) | ||
| return AuthorizedResponse(user_id=user_id, username=user_name) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix async/sync pattern and use proper dependency injection
The current implementation has several issues:
- Async/sync mismatch: Using
asyncio.run()in a sync handler can cause problems in FastAPI's async context - Bypassing dependency injection: The auth dependency should be injected using FastAPI's
Depends()mechanism - Error handling: No explicit error handling for authentication failures
Apply this diff to fix the implementation:
-@router.post("/authorized", responses=authorized_responses)
-def authorized_endpoint_handler(_request: Request) -> AuthorizedResponse:
- """Handle request to the /authorized endpoint."""
- # Ignore the user token, we should not return it in the response
- user_id, user_name, _ = asyncio.run(auth_dependency(_request))
- return AuthorizedResponse(user_id=user_id, username=user_name)
+@router.post("/authorized", responses=authorized_responses)
+async def authorized_endpoint_handler(
+ auth_result: tuple[str, str, str] = Depends(auth_dependency)
+) -> AuthorizedResponse:
+ """Handle request to the /authorized endpoint."""
+ # Ignore the user token, we should not return it in the response
+ user_id, user_name, _ = auth_result
+ return AuthorizedResponse(user_id=user_id, username=user_name)This approach:
- Makes the handler async to properly work with FastAPI
- Uses FastAPI's dependency injection system
- Allows FastAPI to handle authentication errors automatically
- Eliminates the problematic
asyncio.run()call
Let me verify how the auth dependency is typically used in other endpoints:
🏁 Script executed:
#!/bin/bash
# Search for other usages of auth_dependency in the codebase
rg -A 5 -B 5 "auth_dependency" --type pyLength of output: 19722
🏁 Script executed:
#!/bin/bash
rg -A 20 -B 5 "def get_auth_dependency" src/auth/__init__.pyLength of output: 901
🏁 Script executed:
#!/bin/bash
rg -A 20 -B 0 "def test_authorized_endpoint" -n tests/unit/app/endpoints/test_authorized.pyLength of output: 796
🏁 Script executed:
#!/bin/bash
rg -A 5 -B 2 "class AuthInterface" --type pyLength of output: 531
Use FastAPI’s dependency injection instead of asyncio.run and update tests
The /authorized handler should let FastAPI call the async auth dependency rather than spinning up a new event loop. Please:
• In src/app/endpoints/authorized.py:
- Remove the
Requestparameter andasyncio.run(...)call - Inject the dependency with
Depends(auth_dependency) - Mark the handler
async def(so FastAPI can await the auth) - Import
Dependsfromfastapi
• In your unit tests (tests/unit/app/endpoints/test_authorized.py):
- Stop calling the handler directly with a raw
Request - Use FastAPI’s
TestClient(orAsyncClient) andapp.dependency_overridesto stub outauth_dependency - Assert on the full HTTP response instead of the handler return value
Example diff for the endpoint:
--- a/src/app/endpoints/authorized.py
+++ b/src/app/endpoints/authorized.py
@@
-from fastapi import APIRouter, Request
+from fastapi import APIRouter, Depends
@@
-@router.post("/authorized", responses=authorized_responses)
-def authorized_endpoint_handler(_request: Request) -> AuthorizedResponse:
- """Handle request to the /authorized endpoint."""
- # Ignore the user token, we should not return it in the response
- user_id, user_name, _ = asyncio.run(auth_dependency(_request))
- return AuthorizedResponse(user_id=user_id, username=user_name)
+@router.post("/authorized", responses=authorized_responses)
+async def authorized_endpoint_handler(
+ auth_result: tuple[str, str, str] = Depends(auth_dependency)
+) -> AuthorizedResponse:
+ """Handle request to the /authorized endpoint."""
+ user_id, user_name, _ = auth_result
+ return AuthorizedResponse(user_id=user_id, username=user_name)And a sketch for updating the test:
from fastapi.testclient import TestClient
from app.main import app
from app.endpoints.authorized import auth_dependency
def test_authorized_endpoint():
app.dependency_overrides[auth_dependency] = lambda req: ("test-id", "test-user", None)
client = TestClient(app)
response = client.post("/authorized")
assert response.status_code == 200
assert response.json() == {"user_id": "test-id", "username": "test-user"}🤖 Prompt for AI Agents
In src/app/endpoints/authorized.py lines 33-38, replace the synchronous handler
using asyncio.run with an async def function that injects auth_dependency via
FastAPI's Depends. Remove the Request parameter and import Depends from fastapi.
Then, in tests/unit/app/endpoints/test_authorized.py, stop calling the handler
directly with a raw Request; instead, use FastAPI's TestClient and override
auth_dependency with app.dependency_overrides to provide a stub. Finally, make
assertions on the full HTTP response from the client.post call rather than the
handler's return value.
|
@tisnik should I change the code to best match what the coderabbitai pointed out ? Or leave it as-is ? Honestly, I think the code is fine but, if u think it needs to change I'm ok modifying it as well |
I think it is ok as is (if we change this one endpoint, we'd need to change other too, probably not on plate today ;) |
|
@umago I can merge if you don't need any other review. You choose ;) |
|
@tisnik go ahead then, this endpoint is super simple. It's just a way for folks to test if they are authorized or not so I think we can go ahead and merge this. Thank you! |
2 participants
Description
Follow-up patch for the Authentication work, this patch implements the /authorized endpoint to test the user authentication. Same as previously implemented in road-core/service.
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Documentation
Tests