fix(auth): return 401 for API paths in require_login to prevent wrong post-login redirect
The common.js fetch('/api/auth/whoami') probe on every page load was
overwriting the redirect_after_login session key with the API endpoint URL.
After login, users were sent to the JSON endpoint instead of the original page.
Fix: require_login now returns HTTP 401 for any /api/* path, consistent
with REST conventions, and never stores API URLs as the post-login redirect.
Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com>
This commit is contained in:
+13
@@ -3,9 +3,11 @@ import inspect
|
||||
import logging
|
||||
import pathlib
|
||||
from functools import wraps
|
||||
from urllib.parse import urlparse
|
||||
|
||||
from authlib.integrations.starlette_client import OAuth
|
||||
from fastapi import APIRouter, Depends, Request, status
|
||||
from fastapi.responses import JSONResponse
|
||||
from fastapi.templating import Jinja2Templates
|
||||
from sqlalchemy.orm import Session
|
||||
from starlette.responses import RedirectResponse
|
||||
@@ -81,6 +83,17 @@ def require_login(func):
|
||||
@wraps(func)
|
||||
async def wrapper(request: Request, *args, **kwargs):
|
||||
if not request.session.get("user"):
|
||||
# For API endpoints return 401 instead of storing the URL in the session
|
||||
# and redirecting to /login. Without this guard, the /api/auth/whoami
|
||||
# probe issued by common.js on every page load would overwrite
|
||||
# redirect_after_login with the API URL, causing the post-login redirect
|
||||
# to land on a JSON endpoint rather than the original page.
|
||||
url_path = urlparse(str(request.url)).path
|
||||
if url_path.startswith("/api/"):
|
||||
return JSONResponse(
|
||||
status_code=status.HTTP_401_UNAUTHORIZED,
|
||||
content={"error": "Not authenticated"},
|
||||
)
|
||||
request.session["redirect_after_login"] = str(request.url)
|
||||
return RedirectResponse(url="/login", status_code=status.HTTP_302_FOUND)
|
||||
# Check if the wrapped function is a coroutine function
|
||||
|
||||
@@ -175,6 +175,61 @@ class TestRequireLogin:
|
||||
assert result["message"] == "sync"
|
||||
assert result["param"] == "test_value"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_returns_401_for_api_paths_when_not_authenticated(self):
|
||||
"""Test that require_login returns 401 (not redirect) for /api/* paths.
|
||||
|
||||
This prevents the /api/auth/whoami JS probe from overwriting
|
||||
redirect_after_login with an API URL, which would send the user to a
|
||||
JSON endpoint after login instead of the page they actually wanted.
|
||||
"""
|
||||
from fastapi.responses import JSONResponse
|
||||
|
||||
with patch("app.auth.AUTH_ENABLED", True):
|
||||
from app.auth import require_login
|
||||
|
||||
@require_login
|
||||
async def api_endpoint(request: Request):
|
||||
return {"message": "success"}
|
||||
|
||||
mock_request = MagicMock(spec=Request)
|
||||
mock_request.session = {}
|
||||
mock_request.url = MagicMock()
|
||||
mock_request.url.__str__ = MagicMock(return_value="http://test.com/api/auth/whoami")
|
||||
|
||||
result = await api_endpoint(mock_request)
|
||||
|
||||
assert isinstance(result, JSONResponse)
|
||||
assert result.status_code == status.HTTP_401_UNAUTHORIZED
|
||||
# Redirect URL must NOT be stored for API paths
|
||||
assert "redirect_after_login" not in mock_request.session
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_does_not_save_redirect_for_api_paths(self):
|
||||
"""Test that redirect_after_login is never set for any /api/* request."""
|
||||
from fastapi.responses import JSONResponse
|
||||
|
||||
with patch("app.auth.AUTH_ENABLED", True):
|
||||
from app.auth import require_login
|
||||
|
||||
@require_login
|
||||
async def api_endpoint(request: Request):
|
||||
return {"data": "ok"}
|
||||
|
||||
for api_path in ["/api/documents/upload", "/api/v1/resource", "/api/users/me"]:
|
||||
mock_request = MagicMock(spec=Request)
|
||||
mock_request.session = {}
|
||||
mock_request.url = MagicMock()
|
||||
mock_request.url.__str__ = MagicMock(return_value=f"http://test.com{api_path}")
|
||||
|
||||
result = await api_endpoint(mock_request)
|
||||
|
||||
assert isinstance(result, JSONResponse), f"Expected JSONResponse for {api_path}"
|
||||
assert result.status_code == status.HTTP_401_UNAUTHORIZED
|
||||
assert "redirect_after_login" not in mock_request.session, (
|
||||
f"redirect_after_login must not be set for {api_path}"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.integration
|
||||
class TestWhoamiEndpoint:
|
||||
|
||||
@@ -185,6 +185,33 @@ class TestRequireLogin:
|
||||
assert isinstance(result, RedirectResponse)
|
||||
assert result.status_code == status.HTTP_302_FOUND
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_returns_401_for_api_path_when_not_authenticated(self):
|
||||
"""Test returns 401 for /api/* paths instead of redirect-to-login.
|
||||
|
||||
Prevents the common.js /api/auth/whoami probe from overwriting
|
||||
redirect_after_login, which would send the user to a JSON endpoint
|
||||
after login instead of the page they originally requested.
|
||||
"""
|
||||
from fastapi.responses import JSONResponse
|
||||
|
||||
with patch("app.auth.AUTH_ENABLED", True):
|
||||
|
||||
@require_login
|
||||
async def test_api_endpoint(request: Request):
|
||||
return {"data": "ok"}
|
||||
|
||||
mock_request = MagicMock(spec=Request)
|
||||
mock_request.session = {}
|
||||
mock_request.url = MagicMock()
|
||||
mock_request.url.__str__ = MagicMock(return_value="http://localhost/api/auth/whoami")
|
||||
|
||||
result = await test_api_endpoint(mock_request)
|
||||
|
||||
assert isinstance(result, JSONResponse)
|
||||
assert result.status_code == status.HTTP_401_UNAUTHORIZED
|
||||
assert "redirect_after_login" not in mock_request.session
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestOAuthConfiguration:
|
||||
|
||||
Reference in New Issue
Block a user