From 9f59cf7406f138d44367f23c1bdfe1f22be57977 Mon Sep 17 00:00:00 2001 From: Emiliano Gandini Outeda Date: Wed, 9 Sep 2026 12:56:26 -0300 Subject: [PATCH 1/2] fix(security): require authentication on read endpoints Add CurrentUserDep to GET /users/{username}, tier list/lookup, and rate-limit list/get endpoints, matching their documented 401 behavior. Update the API route table accordingly. --- backend/src/modules/rate_limit/routes.py | 4 +++- backend/src/modules/tier/routes.py | 4 +++- backend/src/modules/user/routes.py | 3 ++- docs/user-guide/api/index.md | 4 ++-- 4 files changed, 10 insertions(+), 5 deletions(-) diff --git a/backend/src/modules/rate_limit/routes.py b/backend/src/modules/rate_limit/routes.py index 3556282f..4e734050 100644 --- a/backend/src/modules/rate_limit/routes.py +++ b/backend/src/modules/rate_limit/routes.py @@ -4,7 +4,7 @@ from fastcrud import PaginatedListResponse, compute_offset, paginated_response from ...infrastructure.auth.http_exceptions import DuplicateValueException, HTTPException, NotFoundException -from ...infrastructure.dependencies import AsyncSessionDep, CurrentSuperUserDep +from ...infrastructure.dependencies import AsyncSessionDep, CurrentSuperUserDep, CurrentUserDep from ..common.exceptions import ResourceExistsError, ResourceNotFoundError from ..common.utils.error_handler import handle_exception from .dependencies import RateLimitServiceDep @@ -37,6 +37,7 @@ ) async def get_rate_limits( db: AsyncSessionDep, + _: CurrentUserDep, rate_limit_service: RateLimitServiceDep, page: int = 1, items_per_page: int = 10, @@ -82,6 +83,7 @@ async def get_rate_limits( async def get_rate_limit( name: str, db: AsyncSessionDep, + _: CurrentUserDep, rate_limit_service: RateLimitServiceDep, ) -> dict[str, Any] | None: """ diff --git a/backend/src/modules/tier/routes.py b/backend/src/modules/tier/routes.py index 9613b96c..8f2a97f4 100644 --- a/backend/src/modules/tier/routes.py +++ b/backend/src/modules/tier/routes.py @@ -4,7 +4,7 @@ from fastcrud import PaginatedListResponse, compute_offset, paginated_response from ...infrastructure.auth.http_exceptions import NotFoundException -from ...infrastructure.dependencies import AsyncSessionDep +from ...infrastructure.dependencies import AsyncSessionDep, CurrentUserDep from ..common.exceptions import TierNotFoundError from ..common.utils.error_handler import handle_exception from .dependencies import TierServiceDep @@ -16,6 +16,7 @@ @router.get("/", response_model=PaginatedListResponse[TierRead], summary="List tiers") async def get_tiers( db: AsyncSessionDep, + _: CurrentUserDep, tier_service: TierServiceDep, page: int = 1, items_per_page: int = 10, @@ -39,6 +40,7 @@ async def get_tiers( async def get_tier_by_name( name: str, db: AsyncSessionDep, + _: CurrentUserDep, tier_service: TierServiceDep, ) -> dict[str, Any]: """Get a tier by name.""" diff --git a/backend/src/modules/user/routes.py b/backend/src/modules/user/routes.py index e9265cf4..acc18869 100644 --- a/backend/src/modules/user/routes.py +++ b/backend/src/modules/user/routes.py @@ -128,11 +128,12 @@ async def get_current_user_profile( Note that usernames are case-sensitive in lookup operations. """, - responses={404: {"description": "User not found"}}, + responses={401: {"description": "Not authenticated"}, 404: {"description": "User not found"}}, response_description="The requested user's profile data", ) async def get_user_by_username( username: str, + _: CurrentUserDep, db: AsyncSessionDep, user_service: UserServiceDep, ) -> dict[str, Any]: diff --git a/docs/user-guide/api/index.md b/docs/user-guide/api/index.md index c96d7575..e9357051 100644 --- a/docs/user-guide/api/index.md +++ b/docs/user-guide/api/index.md @@ -183,8 +183,8 @@ What ships out of the box (40 total routes): | Prefix | Source | Notes | |--------|--------|-------| | `POST/GET/PATCH/DELETE /api/v1/users/*` | `modules/user/routes.py` | Open create, session/superuser-gated reads/updates | -| `GET /api/v1/tiers/*` | `modules/tier/routes.py` | Public list + lookup by name | -| `GET/PATCH/DELETE /api/v1/rate-limits/*` | `modules/rate_limit/routes.py` | List/get public; PATCH/DELETE require superuser | +| `GET /api/v1/tiers/*` | `modules/tier/routes.py` | Authenticated list + lookup by name | +| `GET/PATCH/DELETE /api/v1/rate-limits/*` | `modules/rate_limit/routes.py` | List/get require authentication; PATCH/DELETE require superuser | | `POST /api/v1/auth/login`, `logout`, `refresh-csrf`, `check-auth` | `infrastructure/auth/routes.py` | Session auth | | `GET /api/v1/auth/oauth/google`, `oauth/callback/google` | `infrastructure/auth/routes.py` | Google OAuth | | `POST/GET/PATCH/DELETE /api/v1/api-keys/*` | `modules/api_keys/routes.py` | Authenticated key management | From ecedbb8bc8a4537db30b719aa251aa0bd5f30ef3 Mon Sep 17 00:00:00 2001 From: Igor Benav Date: Thu, 17 Sep 2026 21:39:53 -0300 Subject: [PATCH 2/2] keep emails out of username lookups, make rate-limit reads superuser-only, and pin the gates with tests --- backend/src/modules/rate_limit/routes.py | 17 +++-- backend/src/modules/tier/routes.py | 21 ++++-- backend/src/modules/user/routes.py | 11 +-- backend/src/modules/user/schemas.py | 18 +++++ .../api/v1/test_read_endpoint_auth.py | 68 +++++++++++++++++++ .../integration/api/v1/users/test_read.py | 24 ++++++- .../integration/api/v1/users/test_update.py | 11 +-- docs/user-guide/api/index.md | 4 +- .../authentication/user-management.md | 2 +- 9 files changed, 153 insertions(+), 23 deletions(-) create mode 100644 backend/tests/integration/api/v1/test_read_endpoint_auth.py diff --git a/backend/src/modules/rate_limit/routes.py b/backend/src/modules/rate_limit/routes.py index 4e734050..a043e241 100644 --- a/backend/src/modules/rate_limit/routes.py +++ b/backend/src/modules/rate_limit/routes.py @@ -4,7 +4,7 @@ from fastcrud import PaginatedListResponse, compute_offset, paginated_response from ...infrastructure.auth.http_exceptions import DuplicateValueException, HTTPException, NotFoundException -from ...infrastructure.dependencies import AsyncSessionDep, CurrentSuperUserDep, CurrentUserDep +from ...infrastructure.dependencies import AsyncSessionDep, CurrentSuperUserDep from ..common.exceptions import ResourceExistsError, ResourceNotFoundError from ..common.utils.error_handler import handle_exception from .dependencies import RateLimitServiceDep @@ -32,12 +32,15 @@ Results are paginated to handle systems with many rate limit configurations. """, - responses={401: {"description": "Not authenticated"}}, + responses={ + 401: {"description": "Not authenticated"}, + 403: {"description": "Not a superuser"}, + }, response_description="A paginated list of rate limits with their configuration details", ) async def get_rate_limits( db: AsyncSessionDep, - _: CurrentUserDep, + _: CurrentSuperUserDep, rate_limit_service: RateLimitServiceDep, page: int = 1, items_per_page: int = 10, @@ -77,13 +80,17 @@ async def get_rate_limits( Rate limit names are typically in the format of `path:limit:period`. """, - responses={401: {"description": "Not authenticated"}, 404: {"description": "Rate limit not found"}}, + responses={ + 401: {"description": "Not authenticated"}, + 403: {"description": "Not a superuser"}, + 404: {"description": "Rate limit not found"}, + }, response_description="Detailed configuration of the requested rate limit", ) async def get_rate_limit( name: str, db: AsyncSessionDep, - _: CurrentUserDep, + _: CurrentSuperUserDep, rate_limit_service: RateLimitServiceDep, ) -> dict[str, Any] | None: """ diff --git a/backend/src/modules/tier/routes.py b/backend/src/modules/tier/routes.py index 8f2a97f4..8bd5dbdc 100644 --- a/backend/src/modules/tier/routes.py +++ b/backend/src/modules/tier/routes.py @@ -13,7 +13,12 @@ router = APIRouter(tags=["Tiers"]) -@router.get("/", response_model=PaginatedListResponse[TierRead], summary="List tiers") +@router.get( + "/", + response_model=PaginatedListResponse[TierRead], + summary="List tiers", + responses={401: {"description": "Not authenticated"}}, +) async def get_tiers( db: AsyncSessionDep, _: CurrentUserDep, @@ -21,7 +26,7 @@ async def get_tiers( page: int = 1, items_per_page: int = 10, ) -> dict: - """Paginated list of tiers.""" + """Paginated list of tiers (authenticated).""" try: tiers_data = await tier_service.get_all( db=db, @@ -36,14 +41,22 @@ async def get_tiers( raise HTTPException(status_code=500, detail="An unexpected error occurred") -@router.get("/{name}", response_model=TierRead, summary="Get a tier by name") +@router.get( + "/{name}", + response_model=TierRead, + summary="Get a tier by name", + responses={ + 401: {"description": "Not authenticated"}, + 404: {"description": "Tier not found"}, + }, +) async def get_tier_by_name( name: str, db: AsyncSessionDep, _: CurrentUserDep, tier_service: TierServiceDep, ) -> dict[str, Any]: - """Get a tier by name.""" + """Get a tier by name (authenticated).""" try: return await tier_service.get_by_name(name, db) except TierNotFoundError: diff --git a/backend/src/modules/user/routes.py b/backend/src/modules/user/routes.py index acc18869..e890ad27 100644 --- a/backend/src/modules/user/routes.py +++ b/backend/src/modules/user/routes.py @@ -13,6 +13,7 @@ from .dependencies import UserServiceDep from .schemas import ( UserCreate, + UserProfileRead, UserRead, UserTierUpdate, UserUpdate, @@ -117,14 +118,14 @@ async def get_current_user_profile( @router.get( "/{username}", - response_model=UserRead, + response_model=UserProfileRead, summary="Get User Profile by Username", description=""" - Retrieves a user's profile information by their unique username. + Retrieves a user's public profile by their unique username. - This endpoint can be used to look up any active user in the system by their - username. It returns the same profile data structure as other user - endpoints but does not include sensitive information. + Any signed-in user can look up any other active user. The response carries + the display fields only - no email address - so the lookup can't be used to + collect addresses. Read your own full record from `/users/me`. Note that usernames are case-sensitive in lookup operations. """, diff --git a/backend/src/modules/user/schemas.py b/backend/src/modules/user/schemas.py index 4473cee6..38cdde87 100644 --- a/backend/src/modules/user/schemas.py +++ b/backend/src/modules/user/schemas.py @@ -37,6 +37,24 @@ class User(TimestampSchema, UserBase, PersistentDeletion): oauth_updated_at: datetime | None = None +class UserProfileRead(BaseModel): + """Another user's profile: the fields any signed-in user may see. + + No email address, so looking someone up by username can't be used to collect + addresses. The owner reads their own record through ``/users/me``, and a + superuser through the list and active-and-inactive endpoints. + """ + + id: int + name: Annotated[str, Field(min_length=2, max_length=30, examples=["User Userson"])] + username: Annotated[ + str, + Field(min_length=2, max_length=20, pattern=r"^[a-z0-9]+$", examples=["userson"]), + ] + profile_image_url: str + tier_id: int | None = None + + class UserRead(BaseModel): """Schema for reading user data, excludes sensitive information.""" diff --git a/backend/tests/integration/api/v1/test_read_endpoint_auth.py b/backend/tests/integration/api/v1/test_read_endpoint_auth.py new file mode 100644 index 00000000..94ac270b --- /dev/null +++ b/backend/tests/integration/api/v1/test_read_endpoint_auth.py @@ -0,0 +1,68 @@ +"""Who may read the tier and rate-limit endpoints.""" + +import pytest +from httpx import AsyncClient + +pytestmark = pytest.mark.asyncio + +ANONYMOUS_READS = [ + "/api/v1/users/", + "/api/v1/tiers/", + "/api/v1/rate-limits/", +] + + +@pytest.mark.parametrize("path", ANONYMOUS_READS) +async def test_read_endpoints_reject_anonymous_callers(client: AsyncClient, path: str): + """None of the collection reads answer without a session.""" + response = await client.get(path) + + assert response.status_code == 401 + + +async def test_named_reads_reject_anonymous_callers(client: AsyncClient, test_tier: dict): + """The by-name lookups are gated too, before the row is even looked up.""" + assert (await client.get(f"/api/v1/tiers/{test_tier['name']}")).status_code == 401 + assert (await client.get("/api/v1/rate-limits/anything")).status_code == 401 + + +async def test_tiers_are_readable_by_any_signed_in_user(auth_client: AsyncClient, test_tier: dict): + """Tiers describe what a plan offers, so any signed-in user may read them.""" + listing = await auth_client.get("/api/v1/tiers/") + named = await auth_client.get(f"/api/v1/tiers/{test_tier['name']}") + + assert listing.status_code == 200 + assert named.status_code == 200 + assert named.json()["name"] == test_tier["name"] + + +async def test_rate_limit_configuration_is_superuser_only(auth_client: AsyncClient): + """Rate-limit rows are operational configuration, not user-facing data.""" + assert (await auth_client.get("/api/v1/rate-limits/")).status_code == 403 + assert (await auth_client.get("/api/v1/rate-limits/anything")).status_code == 403 + + +async def test_rate_limits_are_readable_by_a_superuser(superuser_auth_client: AsyncClient): + """A superuser reads the same rows that PATCH and DELETE already required one for.""" + response = await superuser_auth_client.get("/api/v1/rate-limits/") + + assert response.status_code == 200 + assert "data" in response.json() + + +@pytest.mark.parametrize( + ("path", "method", "expected"), + [ + ("/api/v1/users/{username}", "get", {"401", "404"}), + ("/api/v1/tiers/", "get", {"401"}), + ("/api/v1/tiers/{name}", "get", {"401", "404"}), + ("/api/v1/rate-limits/", "get", {"401", "403"}), + ("/api/v1/rate-limits/{name}", "get", {"401", "403", "404"}), + ], +) +async def test_openapi_advertises_the_gate(client: AsyncClient, path: str, method: str, expected: set[str]): + """A client generated from the schema must know these can be refused.""" + schema = (await client.get("/openapi.json")).json() + operation = schema["paths"][path][method] + + assert expected <= set(operation["responses"]) diff --git a/backend/tests/integration/api/v1/users/test_read.py b/backend/tests/integration/api/v1/users/test_read.py index f192d573..7aa1c560 100644 --- a/backend/tests/integration/api/v1/users/test_read.py +++ b/backend/tests/integration/api/v1/users/test_read.py @@ -20,8 +20,8 @@ async def test_get_user_by_username_success(auth_client: AsyncClient, db_session data = response.json() assert data["username"] == username assert "id" in data - assert "email" in data assert "name" in data + assert "email" not in data async def test_get_user_by_username_not_found(auth_client: AsyncClient, db_session: AsyncSession): @@ -99,3 +99,25 @@ async def test_get_user_rate_limits(auth_client: AsyncClient, db_session: AsyncS assert response.status_code == 200 data = response.json() assert "rate_limits" in data + + +async def test_get_user_by_username_requires_authentication(client: AsyncClient, test_user: dict): + """An anonymous caller can't look anyone up by username.""" + response = await client.get(f"/api/v1/users/{test_user['username']}") + + assert response.status_code == 401 + + +async def test_profile_lookup_never_carries_an_email(auth_client: AsyncClient, test_user_2: dict, db_session: AsyncSession): + """Looking someone else up returns display fields, never their address. + + Registration is open, so anything this endpoint returns is readable by anyone + willing to sign up; an email address here would be a directory of addresses. + """ + response = await auth_client.get(f"/api/v1/users/{test_user_2['username']}") + + assert response.status_code == 200 + data = response.json() + assert data["username"] == test_user_2["username"] + assert "email" not in data + assert "is_superuser" not in data diff --git a/backend/tests/integration/api/v1/users/test_update.py b/backend/tests/integration/api/v1/users/test_update.py index d6ce6337..cc0d7aab 100644 --- a/backend/tests/integration/api/v1/users/test_update.py +++ b/backend/tests/integration/api/v1/users/test_update.py @@ -4,6 +4,8 @@ from httpx import AsyncClient from sqlalchemy.ext.asyncio import AsyncSession +from src.modules.user.models import User + from .test_create import generate_unique_user_data logging.basicConfig(level=logging.INFO) @@ -35,11 +37,10 @@ async def test_update_user_profile_success( assert "message" in data assert data["message"] == "User updated successfully" - get_response = await auth_client.get(f"/api/v1/users/{username}") - assert get_response.status_code == 200 - user_data = get_response.json() - assert user_data["name"] == update_data["name"] - assert user_data["email"] == update_data["email"] + stored = await db_session.get(User, test_user["id"]) + await db_session.refresh(stored) + assert stored.name == update_data["name"] + assert stored.email == update_data["email"] async def test_update_user_profile_invalid_email( diff --git a/docs/user-guide/api/index.md b/docs/user-guide/api/index.md index e9357051..afb9990e 100644 --- a/docs/user-guide/api/index.md +++ b/docs/user-guide/api/index.md @@ -182,9 +182,9 @@ What ships out of the box (40 total routes): | Prefix | Source | Notes | |--------|--------|-------| -| `POST/GET/PATCH/DELETE /api/v1/users/*` | `modules/user/routes.py` | Open create, session/superuser-gated reads/updates | +| `POST/GET/PATCH/DELETE /api/v1/users/*` | `modules/user/routes.py` | Open create; reads/updates need a session, and a lookup by username returns no email | | `GET /api/v1/tiers/*` | `modules/tier/routes.py` | Authenticated list + lookup by name | -| `GET/PATCH/DELETE /api/v1/rate-limits/*` | `modules/rate_limit/routes.py` | List/get require authentication; PATCH/DELETE require superuser | +| `GET/PATCH/DELETE /api/v1/rate-limits/*` | `modules/rate_limit/routes.py` | Superuser only | | `POST /api/v1/auth/login`, `logout`, `refresh-csrf`, `check-auth` | `infrastructure/auth/routes.py` | Session auth | | `GET /api/v1/auth/oauth/google`, `oauth/callback/google` | `infrastructure/auth/routes.py` | Google OAuth | | `POST/GET/PATCH/DELETE /api/v1/api-keys/*` | `modules/api_keys/routes.py` | Authenticated key management | diff --git a/docs/user-guide/authentication/user-management.md b/docs/user-guide/authentication/user-management.md index 6fb34400..410cb8ba 100644 --- a/docs/user-guide/authentication/user-management.md +++ b/docs/user-guide/authentication/user-management.md @@ -11,7 +11,7 @@ All under `/api/v1/users/` (defined in `modules/user/routes.py`): | `POST` | `/api/v1/users/` | Create a new user | Open | | `GET` | `/api/v1/users/` | Paginated list of users | Superuser | | `GET` | `/api/v1/users/me` | Current user's profile | Session | -| `GET` | `/api/v1/users/{username}` | Get a user by username (active only) | Open | +| `GET` | `/api/v1/users/{username}` | Public profile by username (no email) | Session | | `GET` | `/api/v1/users/active-and-inactive/{username}` | Same as above, includes soft-deleted | Superuser | | `PATCH` | `/api/v1/users/{username}` | Update profile (own or admin) | Session | | `DELETE` | `/api/v1/users/{username}` | Soft-delete a user (own or admin) | Session |