-
Notifications
You must be signed in to change notification settings - Fork 54
Implement the /authorized endpoint #210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| """Handler for REST API call to authorized endpoint.""" | ||
|
|
||
| import asyncio | ||
| import logging | ||
| from typing import Any | ||
|
|
||
| from fastapi import APIRouter, Request | ||
|
|
||
| from auth import get_auth_dependency | ||
| from models.responses import AuthorizedResponse, UnauthorizedResponse, ForbiddenResponse | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| router = APIRouter(tags=["authorized"]) | ||
| auth_dependency = get_auth_dependency() | ||
|
|
||
|
|
||
| authorized_responses: dict[int | str, dict[str, Any]] = { | ||
| 200: { | ||
| "description": "The user is logged-in and authorized to access OLS", | ||
| "model": AuthorizedResponse, | ||
| }, | ||
| 400: { | ||
| "description": "Missing or invalid credentials provided by client", | ||
| "model": UnauthorizedResponse, | ||
| }, | ||
| 403: { | ||
| "description": "User is not authorized", | ||
| "model": ForbiddenResponse, | ||
| }, | ||
| } | ||
|
|
||
|
|
||
| @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) | ||
|
Comment on lines
+33
to
+38
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainFix async/sync pattern and use proper dependency injection The current implementation has several issues:
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:
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 The • In
• In your unit tests (
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 |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| from unittest.mock import AsyncMock | ||
|
|
||
| import pytest | ||
| from fastapi import Request, HTTPException | ||
|
|
||
| from app.endpoints.authorized import authorized_endpoint_handler | ||
|
|
||
|
|
||
| def test_authorized_endpoint(mocker): | ||
| """Test the authorized endpoint handler.""" | ||
| # Mock the auth dependency to return a user ID and username | ||
| auth_dependency_mock = AsyncMock() | ||
| auth_dependency_mock.return_value = ("test-id", "test-user", None) | ||
| mocker.patch( | ||
| "app.endpoints.authorized.auth_dependency", side_effect=auth_dependency_mock | ||
| ) | ||
|
|
||
| request = Request( | ||
| scope={ | ||
| "type": "http", | ||
| "query_string": b"", | ||
| } | ||
| ) | ||
|
|
||
| response = authorized_endpoint_handler(request) | ||
|
|
||
| assert response.model_dump() == { | ||
| "user_id": "test-id", | ||
| "username": "test-user", | ||
| } | ||
|
|
||
|
|
||
| def test_authorized_unauthorized(mocker): | ||
| """Test the authorized endpoint handler with a custom user ID.""" | ||
| auth_dependency_mock = AsyncMock() | ||
| auth_dependency_mock.side_effect = HTTPException( | ||
| status_code=403, detail="User is not authorized" | ||
| ) | ||
| mocker.patch( | ||
| "app.endpoints.authorized.auth_dependency", side_effect=auth_dependency_mock | ||
| ) | ||
|
|
||
| request = Request( | ||
| scope={ | ||
| "type": "http", | ||
| "query_string": b"", | ||
| } | ||
| ) | ||
|
|
||
| with pytest.raises(HTTPException) as exc_info: | ||
| authorized_endpoint_handler(request) | ||
|
|
||
| assert exc_info.value.status_code == 403 | ||
| assert exc_info.value.detail == "User is not authorized" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Remove unused asyncio import
The
asyncioimport is only used for the problematicasyncio.run()call. After fixing the async/sync pattern, this import should be removed.-import asyncio📝 Committable suggestion
🤖 Prompt for AI Agents