From c44c6bb29a5541453051522f3a2615e55584670b Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 15 May 2026 08:25:59 +0000 Subject: [PATCH 1/2] refactor(users): split into admin/auth_local/oauth feature folders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reorganize the users module intra-package from a type-folder layout (endpoints/, service.py, oauth.py at top level) to a feature-folder layout where each sub-feature owns its routes, service, views, and components. modules/users/users/ admin/ # service, REST API, Inertia views, React components auth_local/ # login/register/reset/verify/profile + accept-invite oauth/ # provider builders + per-provider routes (__init__.py # re-exports the prior `users.oauth.{X}` surface) Shared infrastructure stays at top level (deps, manager, backend, models, contracts, mailer, middleware, bootstrap, pages, locales). UsersModule.register_routes is now the single point of cross-feature wiring; no feature imports from a sibling feature in either the API or views layer. The login page's OAuth-provider list is cached once at boot on app.state.users.oauth_providers so auth_local/views.py doesn't reach into the oauth feature. Framework filesystem conventions are preserved unchanged: pages/ stays a single tree at the package root (required by simple_module_hosting.manifest.compute_module_pages) and locales/ stays in place. Scope: the users module only. The scaffold templates, framework-conventions docs, and other modules are deliberately not touched — this is a POC to validate the layout before propagating it. Verified locally: make lint, make doctor, full pytest (1010 passed), JS tests (8 passed), and a live uvicorn+Vite run exercising the refactored auth_local/api login wrapper, auth_local/views login page, admin/api list, admin/views Inertia index, and the moved admin/components/*.tsx imports. --- docs/testing/fixtures.md | 2 +- modules/users/tests/test_api_auth.py | 2 +- modules/users/tests/test_rate_limit.py | 2 +- modules/users/tests/test_service_admin.py | 2 +- modules/users/tests/test_user_service.py | 4 +- modules/users/tests/test_views.py | 2 +- modules/users/users/admin/__init__.py | 1 + .../{endpoints/api_admin.py => admin/api.py} | 8 +- .../{ => admin}/components/IndexFilters.tsx | 0 .../users/{ => admin}/components/RolesTab.tsx | 0 .../users/{ => admin}/components/UserRow.tsx | 0 modules/users/users/{ => admin}/service.py | 0 modules/users/users/admin/views.py | 126 ++++++++++ modules/users/users/auth_local/__init__.py | 4 + .../users/{endpoints => auth_local}/api.py | 68 +----- .../users/{ => auth_local}/rate_limit.py | 0 modules/users/users/auth_local/views.py | 103 ++++++++ modules/users/users/deps.py | 4 +- modules/users/users/endpoints/__init__.py | 1 - modules/users/users/endpoints/views.py | 228 ------------------ modules/users/users/module.py | 48 +++- modules/users/users/oauth/__init__.py | 5 + .../{endpoints/api_oauth.py => oauth/api.py} | 2 +- .../users/{oauth.py => oauth/providers.py} | 0 modules/users/users/pages/Users/Index.tsx | 6 +- modules/users/users/state.py | 3 +- 26 files changed, 307 insertions(+), 314 deletions(-) create mode 100644 modules/users/users/admin/__init__.py rename modules/users/users/{endpoints/api_admin.py => admin/api.py} (95%) rename modules/users/users/{ => admin}/components/IndexFilters.tsx (100%) rename modules/users/users/{ => admin}/components/RolesTab.tsx (100%) rename modules/users/users/{ => admin}/components/UserRow.tsx (100%) rename modules/users/users/{ => admin}/service.py (100%) create mode 100644 modules/users/users/admin/views.py create mode 100644 modules/users/users/auth_local/__init__.py rename modules/users/users/{endpoints => auth_local}/api.py (73%) rename modules/users/users/{ => auth_local}/rate_limit.py (100%) create mode 100644 modules/users/users/auth_local/views.py delete mode 100644 modules/users/users/endpoints/__init__.py delete mode 100644 modules/users/users/endpoints/views.py create mode 100644 modules/users/users/oauth/__init__.py rename modules/users/users/{endpoints/api_oauth.py => oauth/api.py} (98%) rename modules/users/users/{oauth.py => oauth/providers.py} (100%) diff --git a/docs/testing/fixtures.md b/docs/testing/fixtures.md index 7356ada0..560cbcb3 100644 --- a/docs/testing/fixtures.md +++ b/docs/testing/fixtures.md @@ -105,7 +105,7 @@ The admin has `*` permission (via `DEFAULT_ROLE_PERMISSIONS["admin"]`), so it by ```python @pytest.mark.asyncio async def test_non_admin_denied(client, db_session): - from users.service import UserService + from users.admin.service import UserService svc = UserService(db_session) await svc.create(email="u@e.com", password="x", roles=["viewer"]) await db_session.commit() diff --git a/modules/users/tests/test_api_auth.py b/modules/users/tests/test_api_auth.py index 9f60ca0e..e6b34700 100644 --- a/modules/users/tests/test_api_auth.py +++ b/modules/users/tests/test_api_auth.py @@ -198,7 +198,7 @@ async def test_forgot_password_rate_limited_after_threshold( self, anon_client, users_app, users_db ): """After the configured attempt budget, /forgot-password returns 429.""" - from users.rate_limit import ThroughputLimiter + from users.auth_local.rate_limit import ThroughputLimiter # Tighten the limit for the test so we don't need to hit 10 real endpoints users_app.state.users.auth_throughput_limiter = ThroughputLimiter( diff --git a/modules/users/tests/test_rate_limit.py b/modules/users/tests/test_rate_limit.py index 46caee5a..6698f23d 100644 --- a/modules/users/tests/test_rate_limit.py +++ b/modules/users/tests/test_rate_limit.py @@ -3,7 +3,7 @@ from __future__ import annotations import pytest -from users.rate_limit import LoginRateLimiter, ThroughputLimiter +from users.auth_local.rate_limit import LoginRateLimiter, ThroughputLimiter @pytest.fixture diff --git a/modules/users/tests/test_service_admin.py b/modules/users/tests/test_service_admin.py index dfa3b564..d240588f 100644 --- a/modules/users/tests/test_service_admin.py +++ b/modules/users/tests/test_service_admin.py @@ -10,10 +10,10 @@ def _build_service(session, users_app): """Build a UserService directly (bypass FastAPI Depends).""" + from users.admin.service import UserService from users.db_adapter import UserDatabaseWithRoles from users.manager import UserManager from users.models import User - from users.service import UserService user_db = UserDatabaseWithRoles(session, User) manager = UserManager( diff --git a/modules/users/tests/test_user_service.py b/modules/users/tests/test_user_service.py index ddc118a0..bd2e5dfe 100644 --- a/modules/users/tests/test_user_service.py +++ b/modules/users/tests/test_user_service.py @@ -9,10 +9,10 @@ def _build_service(session, users_app): """Build a UserService directly (bypass FastAPI Depends).""" + from users.admin.service import UserService from users.db_adapter import UserDatabaseWithRoles from users.manager import UserManager from users.models import User - from users.service import UserService user_db = UserDatabaseWithRoles(session, User) manager = UserManager( @@ -77,10 +77,10 @@ async def test_get_list_item_unknown_user_raises_user_not_found(users_app): async def test_to_list_item_includes_created_at(users_app): """`UserListItem` carries `created_at` sourced from AuditMixin.""" from fastapi_users.password import PasswordHelper + from users.admin.service import UserService from users.db_adapter import UserDatabaseWithRoles from users.manager import UserManager from users.models import User - from users.service import UserService async with users_app.state.sm.db.session_factory() as session: user = User( diff --git a/modules/users/tests/test_views.py b/modules/users/tests/test_views.py index bf6da524..58d0f296 100644 --- a/modules/users/tests/test_views.py +++ b/modules/users/tests/test_views.py @@ -187,7 +187,7 @@ async def test_admin_edit_page_unknown_user_returns_404(admin_client): @pytest.mark.anyio async def test_roles_payload_returns_id_name_dicts(users_app): """Helper reads the roles cache and returns id/name dicts in cache order.""" - from users.endpoints.views import _roles_payload + from users.admin.views import _roles_payload payload = await _roles_payload(users_app) diff --git a/modules/users/users/admin/__init__.py b/modules/users/users/admin/__init__.py new file mode 100644 index 00000000..0045fd60 --- /dev/null +++ b/modules/users/users/admin/__init__.py @@ -0,0 +1 @@ +"""Admin user management. Intentionally empty — import via fully-qualified paths.""" diff --git a/modules/users/users/endpoints/api_admin.py b/modules/users/users/admin/api.py similarity index 95% rename from modules/users/users/endpoints/api_admin.py rename to modules/users/users/admin/api.py index 567192ee..71f1ff39 100644 --- a/modules/users/users/endpoints/api_admin.py +++ b/modules/users/users/admin/api.py @@ -1,8 +1,4 @@ -"""Admin REST endpoints for the users module. - -Split out of :mod:`.api` to keep per-file complexity manageable. Mounted -into the main ``router`` via ``include_router`` at the bottom of ``api.py``. -""" +"""Admin REST endpoints for the users module.""" from __future__ import annotations @@ -13,6 +9,7 @@ from simple_module_core.events import EventBus from simple_module_hosting.permissions import RequiresPermission +from users.admin.service import UserService from users.constants import PERM_USERS_MANAGE, sanitize_list_filters from users.contracts.events import RoleAssigned, UserDisabled, UserInvited from users.contracts.schemas import ( @@ -23,7 +20,6 @@ ) from users.deps import get_event_bus, get_mailer, get_user_service from users.exceptions import UserNotFoundError -from users.service import UserService admin_router = APIRouter( prefix="/admin", diff --git a/modules/users/users/components/IndexFilters.tsx b/modules/users/users/admin/components/IndexFilters.tsx similarity index 100% rename from modules/users/users/components/IndexFilters.tsx rename to modules/users/users/admin/components/IndexFilters.tsx diff --git a/modules/users/users/components/RolesTab.tsx b/modules/users/users/admin/components/RolesTab.tsx similarity index 100% rename from modules/users/users/components/RolesTab.tsx rename to modules/users/users/admin/components/RolesTab.tsx diff --git a/modules/users/users/components/UserRow.tsx b/modules/users/users/admin/components/UserRow.tsx similarity index 100% rename from modules/users/users/components/UserRow.tsx rename to modules/users/users/admin/components/UserRow.tsx diff --git a/modules/users/users/service.py b/modules/users/users/admin/service.py similarity index 100% rename from modules/users/users/service.py rename to modules/users/users/admin/service.py diff --git a/modules/users/users/admin/views.py b/modules/users/users/admin/views.py new file mode 100644 index 00000000..af9b38a7 --- /dev/null +++ b/modules/users/users/admin/views.py @@ -0,0 +1,126 @@ +"""Inertia view routes for admin user management.""" + +from __future__ import annotations + +import uuid + +from fastapi import APIRouter, Depends, HTTPException, Request +from inertia import InertiaResponse +from simple_module_hosting.inertia_deps import InertiaDep +from simple_module_hosting.permissions import RequiresPermission + +from users.admin.service import UserService +from users.constants import PERM_USERS_MANAGE, sanitize_list_filters +from users.deps import get_user_service +from users.exceptions import UserNotFoundError +from users.roles_cache import get_roles_cache + +router = APIRouter() + +_PAGE_ADMIN_INDEX = "Users/Users/Index" +_PAGE_ADMIN_INVITE = "Users/Users/Invite" +_PAGE_ADMIN_EDIT = "Users/Users/Edit" + + +async def _roles_payload(app) -> list[dict[str, str]]: + """Shape roles-cache entries for Inertia props.""" + return [{"id": r.id, "name": r.name} for r in await get_roles_cache(app)] + + +@router.get( + "/admin", + response_model=None, + dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], +) +async def admin_index( + request: Request, + inertia: InertiaDep, + service: UserService = Depends(get_user_service), + page: int = 1, + per_page: int = 20, + q: str | None = None, + status: str | None = None, + role: str | None = None, + verified: str | None = None, + sort: str = "email", + order: str = "asc", +) -> InertiaResponse: + clean_status, clean_verified, clean_sort, clean_order = sanitize_list_filters( + status, verified, sort, order + ) + users, total = await service.list_users( + page=page, + per_page=per_page, + search=q, + status=clean_status, + role_name=role or None, + verified=clean_verified, + sort=clean_sort, + order=clean_order, + ) + roles = await service.list_roles() + aggregates = await service.count_user_states() + return await inertia.render( + _PAGE_ADMIN_INDEX, + { + "users": [u.model_dump(mode="json") for u in users], + "pagination": {"page": page, "per_page": per_page, "total": total}, + "aggregates": aggregates, + "query": q or "", + "roles": [r.model_dump(mode="json") for r in roles], + "filters": { + "status": clean_status or "all", + "role": role or "", + "verified": clean_verified or "all", + "sort": clean_sort, + "order": clean_order, + }, + }, + ) + + +@router.get( + "/admin/invite", + response_model=None, + dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], +) +async def admin_invite_page( + request: Request, + inertia: InertiaDep, +) -> InertiaResponse: + return await inertia.render( + _PAGE_ADMIN_INVITE, + { + "roles": await _roles_payload(request.app), + }, + ) + + +@router.get( + "/admin/{user_id}", + response_model=None, + dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], +) +async def admin_edit_page( + user_id: str, + request: Request, + inertia: InertiaDep, + service: UserService = Depends(get_user_service), +) -> InertiaResponse: + try: + uid = uuid.UUID(user_id) + except ValueError as exc: + raise HTTPException(status_code=404) from exc + try: + user_item = await service.get_list_item(uid) + except UserNotFoundError: + raise HTTPException(status_code=404) from None + has_permissions = any(m.meta.name == "Permissions" for m in request.app.state.sm.modules) + return await inertia.render( + _PAGE_ADMIN_EDIT, + { + "user": user_item.model_dump(mode="json"), + "roles": await _roles_payload(request.app), + "has_permissions_module": has_permissions, + }, + ) diff --git a/modules/users/users/auth_local/__init__.py b/modules/users/users/auth_local/__init__.py new file mode 100644 index 00000000..e537ab65 --- /dev/null +++ b/modules/users/users/auth_local/__init__.py @@ -0,0 +1,4 @@ +"""Local-credential auth: login/register/reset/verify/profile. + +Intentionally empty — import via fully-qualified paths. +""" diff --git a/modules/users/users/endpoints/api.py b/modules/users/users/auth_local/api.py similarity index 73% rename from modules/users/users/endpoints/api.py rename to modules/users/users/auth_local/api.py index 352afa0d..77ba93eb 100644 --- a/modules/users/users/endpoints/api.py +++ b/modules/users/users/auth_local/api.py @@ -1,11 +1,11 @@ -"""REST API endpoints for the users module. - -Structure: - /api/users/auth/login — wrapper with rate limit - /api/users/auth/* — fastapi-users routers (register/reset/verify/logout) - /api/users/auth/accept-invite — custom (verify + set password + login) - /api/users/me — self profile - /api/users/admin/* — admin REST (RequiresPermission('users.manage')) +"""REST endpoints for local-credential auth + self profile. + +Routes owned here (all mounted by :meth:`UsersModule.register_routes`): + POST /api/users/auth/login — rate-limited wrapper around fastapi-users + POST /api/users/auth/accept-invite — verify invite + set password + login + GET /api/users/me — current user + PATCH /api/users/me — update current user + /api/users/auth-inner/* — fastapi-users stock auth router """ from __future__ import annotations @@ -16,11 +16,11 @@ from fastapi.security import OAuth2PasswordRequestForm from fastapi_users import exceptions as fu_exceptions +from users.auth_local.rate_limit import LoginRateLimiter, ThroughputLimiter from users.constants import SESSION_USER_ID_KEY from users.contracts.schemas import ( AcceptInviteRequest, SelfProfileUpdate, - UserCreate, UserRead, UserUpdate, ) @@ -29,11 +29,7 @@ fastapi_users, get_user_manager, ) -from users.endpoints.api_admin import admin_router -from users.endpoints.api_oauth import register_oauth_routes from users.manager import UserManager -from users.rate_limit import LoginRateLimiter, ThroughputLimiter -from users.settings import UsersSettings logger = logging.getLogger(__name__) router = APIRouter() @@ -127,47 +123,6 @@ async def login( router.include_router(auth_inner, prefix="/auth-inner") -def register_auth_routes(api_router: APIRouter, settings: UsersSettings) -> None: - """Mount all auth routes. - - The stock fastapi-users routers (reset/verify/register) ship POST endpoints - that trigger email side-effects or account creation. We wrap them with the - throughput limiter so an attacker can't spam password-reset emails or mint - accounts indefinitely. ``router`` itself is left unwrapped because its - rate-limited endpoints apply the dep themselves (login via LoginRateLimiter, - accept-invite via ``enforce_auth_throughput_limit``). - - The register router is always mounted; ``require_signup_enabled`` gates - it at request time so ``allow_signup`` is hot-reloadable. - - OAuth providers configured in ``settings`` are mounted under - ``/auth//{login,callback}`` — see :mod:`users.endpoints.api_oauth`. - """ - api_router.include_router(router) - api_router.include_router( - fastapi_users.get_reset_password_router(), - prefix="/auth", - tags=["users-auth"], - dependencies=[Depends(enforce_auth_throughput_limit)], - ) - api_router.include_router( - fastapi_users.get_verify_router(UserRead), - prefix="/auth", - tags=["users-auth"], - dependencies=[Depends(enforce_auth_throughput_limit)], - ) - api_router.include_router( - fastapi_users.get_register_router(UserRead, UserCreate), - prefix="/auth", - tags=["users-auth"], - dependencies=[ - Depends(require_signup_enabled), - Depends(enforce_auth_throughput_limit), - ], - ) - register_oauth_routes(api_router, settings) - - # ── Accept-invite (verify + set password + login, one shot) ───────────────── @@ -226,8 +181,3 @@ async def update_me( user, request=request, ) - - -# ── Admin REST — defined in api_admin.py, mounted here ────────────────────── - -router.include_router(admin_router) diff --git a/modules/users/users/rate_limit.py b/modules/users/users/auth_local/rate_limit.py similarity index 100% rename from modules/users/users/rate_limit.py rename to modules/users/users/auth_local/rate_limit.py diff --git a/modules/users/users/auth_local/views.py b/modules/users/users/auth_local/views.py new file mode 100644 index 00000000..42aad883 --- /dev/null +++ b/modules/users/users/auth_local/views.py @@ -0,0 +1,103 @@ +"""Inertia view routes for local-credential auth (login/register/reset/verify/profile).""" + +from __future__ import annotations + +import os + +from fastapi import APIRouter, HTTPException, Request +from inertia import InertiaResponse +from simple_module_hosting.inertia_deps import InertiaDep +from starlette.responses import RedirectResponse + +router = APIRouter() + +_PAGE_LOGIN = "Users/Login" +_PAGE_REGISTER = "Users/Register" +_PAGE_FORGOT_PASSWORD = "Users/ForgotPassword" +_PAGE_RESET_PASSWORD = "Users/ResetPassword" +_PAGE_VERIFY_EMAIL = "Users/VerifyEmail" +_PAGE_ACCEPT_INVITE = "Users/AcceptInvite" +_PAGE_PROFILE = "Users/Profile" + + +@router.get("/login", response_model=None) +async def login_page(request: Request, inertia: InertiaDep) -> InertiaResponse: + users_state = request.app.state.users + users_settings = users_state.settings + # In development only, surface the bootstrap credentials as click-to-fill + # buttons so manual QA doesn't need to retype them. Never exposed in + # production, regardless of whether the vars are set. + dev_accounts: list[dict[str, str]] = [] + if request.app.state.sm.settings.is_development: + admin_email = users_settings.bootstrap_email or os.environ.get( + "SM_USERS_BOOTSTRAP_EMAIL", "" + ) + admin_password = users_settings.bootstrap_password or os.environ.get( + "SM_USERS_BOOTSTRAP_PASSWORD", "" + ) + if admin_email and admin_password: + dev_accounts.append( + {"label": "Admin", "email": admin_email, "password": admin_password} + ) + user_email = users_settings.bootstrap_user_email or os.environ.get( + "SM_USERS_BOOTSTRAP_USER_EMAIL", "" + ) + user_password = users_settings.bootstrap_user_password or os.environ.get( + "SM_USERS_BOOTSTRAP_USER_PASSWORD", "" + ) + if user_email and user_password: + dev_accounts.append({"label": "User", "email": user_email, "password": user_password}) + return await inertia.render( + _PAGE_LOGIN, + { + "allow_signup": users_settings.allow_signup, + "dev_accounts": dev_accounts, + "login_redirect_url": users_settings.login_redirect_url, + "oauth_providers": users_state.oauth_providers, + }, + ) + + +@router.post("/logout", response_model=None) +async def logout(request: Request) -> RedirectResponse: + """POST-only to resist cross-site `` logout attacks — the menu's + logout link submits this as an Inertia form.""" + request.session.clear() + cookie_name = request.app.state.users.settings.cookie_name + # 303 forces the follow-up to GET — Inertia treats the redirect as a full + # navigation rather than replaying the POST. + response = RedirectResponse("/", status_code=303) + response.delete_cookie(cookie_name, path="/") + return response + + +@router.get("/register", response_model=None) +async def register_page(request: Request, inertia: InertiaDep) -> InertiaResponse: + if not request.app.state.users.settings.allow_signup: + raise HTTPException(status_code=404) + return await inertia.render(_PAGE_REGISTER, {}) + + +@router.get("/forgot-password", response_model=None) +async def forgot_password_page(inertia: InertiaDep) -> InertiaResponse: + return await inertia.render(_PAGE_FORGOT_PASSWORD, {}) + + +@router.get("/reset-password", response_model=None) +async def reset_password_page(inertia: InertiaDep, token: str = "") -> InertiaResponse: + return await inertia.render(_PAGE_RESET_PASSWORD, {"token": token}) + + +@router.get("/verify", response_model=None) +async def verify_page(inertia: InertiaDep, token: str = "") -> InertiaResponse: + return await inertia.render(_PAGE_VERIFY_EMAIL, {"token": token}) + + +@router.get("/invite/accept", response_model=None) +async def accept_invite_page(inertia: InertiaDep, token: str = "") -> InertiaResponse: + return await inertia.render(_PAGE_ACCEPT_INVITE, {"token": token}) + + +@router.get("/me", response_model=None) +async def profile_page(inertia: InertiaDep) -> InertiaResponse: + return await inertia.render(_PAGE_PROFILE, {}) diff --git a/modules/users/users/deps.py b/modules/users/users/deps.py index 3fdbcfee..c021afbb 100644 --- a/modules/users/users/deps.py +++ b/modules/users/users/deps.py @@ -31,7 +31,7 @@ from users.models import User if TYPE_CHECKING: - from users.service import UserService + from users.admin.service import UserService # Dev-safe singleton — UsersModule patches cookie params at startup. _cookie_transport = build_cookie_transport( @@ -62,7 +62,7 @@ async def get_user_service( db: AsyncSession = Depends(get_db), user_manager: UserManager = Depends(get_user_manager), ) -> UserService: - from users.service import UserService + from users.admin.service import UserService return UserService(db, user_manager) diff --git a/modules/users/users/endpoints/__init__.py b/modules/users/users/endpoints/__init__.py deleted file mode 100644 index e8907f25..00000000 --- a/modules/users/users/endpoints/__init__.py +++ /dev/null @@ -1 +0,0 @@ -"""REST and view endpoints for the users module.""" diff --git a/modules/users/users/endpoints/views.py b/modules/users/users/endpoints/views.py deleted file mode 100644 index 500a252c..00000000 --- a/modules/users/users/endpoints/views.py +++ /dev/null @@ -1,228 +0,0 @@ -"""Inertia view routes for the users module.""" - -from __future__ import annotations - -import os -import uuid - -from fastapi import APIRouter, Depends, HTTPException, Request -from inertia import InertiaResponse -from simple_module_hosting.inertia_deps import InertiaDep -from simple_module_hosting.permissions import RequiresPermission -from starlette.responses import RedirectResponse - -from users.constants import PERM_USERS_MANAGE, sanitize_list_filters -from users.deps import get_user_service -from users.exceptions import UserNotFoundError -from users.oauth import enabled_provider_names -from users.roles_cache import get_roles_cache -from users.service import UserService - -router = APIRouter() - -# Inertia page identifiers -_PAGE_LOGIN = "Users/Login" -_PAGE_REGISTER = "Users/Register" -_PAGE_FORGOT_PASSWORD = "Users/ForgotPassword" -_PAGE_RESET_PASSWORD = "Users/ResetPassword" -_PAGE_VERIFY_EMAIL = "Users/VerifyEmail" -_PAGE_ACCEPT_INVITE = "Users/AcceptInvite" -_PAGE_PROFILE = "Users/Profile" -_PAGE_ADMIN_INDEX = "Users/Users/Index" -_PAGE_ADMIN_INVITE = "Users/Users/Invite" -_PAGE_ADMIN_EDIT = "Users/Users/Edit" - - -async def _roles_payload(app) -> list[dict[str, str]]: - """Shape roles-cache entries for Inertia props.""" - return [{"id": r.id, "name": r.name} for r in await get_roles_cache(app)] - - -# ── Public auth pages ─────────────────────────────────────────── - - -@router.get("/login", response_model=None) -async def login_page(request: Request, inertia: InertiaDep) -> InertiaResponse: - users_settings = request.app.state.users.settings - # In development only, surface the bootstrap credentials as click-to-fill - # buttons so manual QA doesn't need to retype them. Never exposed in - # production, regardless of whether the vars are set. - dev_accounts: list[dict[str, str]] = [] - if request.app.state.sm.settings.is_development: - admin_email = users_settings.bootstrap_email or os.environ.get( - "SM_USERS_BOOTSTRAP_EMAIL", "" - ) - admin_password = users_settings.bootstrap_password or os.environ.get( - "SM_USERS_BOOTSTRAP_PASSWORD", "" - ) - if admin_email and admin_password: - dev_accounts.append( - {"label": "Admin", "email": admin_email, "password": admin_password} - ) - user_email = users_settings.bootstrap_user_email or os.environ.get( - "SM_USERS_BOOTSTRAP_USER_EMAIL", "" - ) - user_password = users_settings.bootstrap_user_password or os.environ.get( - "SM_USERS_BOOTSTRAP_USER_PASSWORD", "" - ) - if user_email and user_password: - dev_accounts.append({"label": "User", "email": user_email, "password": user_password}) - return await inertia.render( - _PAGE_LOGIN, - { - "allow_signup": users_settings.allow_signup, - "dev_accounts": dev_accounts, - "login_redirect_url": users_settings.login_redirect_url, - "oauth_providers": enabled_provider_names(users_settings), - }, - ) - - -@router.post("/logout", response_model=None) -async def logout(request: Request) -> RedirectResponse: - """Clear the session + auth cookie. POST-only to resist cross-site `` - logout attacks — the menu's logout link submits this as an Inertia form.""" - request.session.clear() - cookie_name = request.app.state.users.settings.cookie_name - # 303 forces the follow-up to GET — Inertia treats the redirect as a full - # navigation rather than replaying the POST. - response = RedirectResponse("/", status_code=303) - response.delete_cookie(cookie_name, path="/") - return response - - -@router.get("/register", response_model=None) -async def register_page(request: Request, inertia: InertiaDep) -> InertiaResponse: - if not request.app.state.users.settings.allow_signup: - raise HTTPException(status_code=404) - return await inertia.render(_PAGE_REGISTER, {}) - - -@router.get("/forgot-password", response_model=None) -async def forgot_password_page(inertia: InertiaDep) -> InertiaResponse: - return await inertia.render(_PAGE_FORGOT_PASSWORD, {}) - - -@router.get("/reset-password", response_model=None) -async def reset_password_page(inertia: InertiaDep, token: str = "") -> InertiaResponse: - return await inertia.render(_PAGE_RESET_PASSWORD, {"token": token}) - - -@router.get("/verify", response_model=None) -async def verify_page(inertia: InertiaDep, token: str = "") -> InertiaResponse: - return await inertia.render(_PAGE_VERIFY_EMAIL, {"token": token}) - - -@router.get("/invite/accept", response_model=None) -async def accept_invite_page(inertia: InertiaDep, token: str = "") -> InertiaResponse: - return await inertia.render(_PAGE_ACCEPT_INVITE, {"token": token}) - - -# ── Authenticated pages ───────────────────────────────────────── - - -@router.get("/me", response_model=None) -async def profile_page(inertia: InertiaDep) -> InertiaResponse: - return await inertia.render(_PAGE_PROFILE, {}) - - -# ── Admin pages ───────────────────────────────────────────────── - - -@router.get( - "/admin", - response_model=None, - dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], -) -async def admin_index( - request: Request, - inertia: InertiaDep, - service: UserService = Depends(get_user_service), - page: int = 1, - per_page: int = 20, - q: str | None = None, - status: str | None = None, - role: str | None = None, - verified: str | None = None, - sort: str = "email", - order: str = "asc", -) -> InertiaResponse: - clean_status, clean_verified, clean_sort, clean_order = sanitize_list_filters( - status, verified, sort, order - ) - users, total = await service.list_users( - page=page, - per_page=per_page, - search=q, - status=clean_status, - role_name=role or None, - verified=clean_verified, - sort=clean_sort, - order=clean_order, - ) - roles = await service.list_roles() - aggregates = await service.count_user_states() - return await inertia.render( - _PAGE_ADMIN_INDEX, - { - "users": [u.model_dump(mode="json") for u in users], - "pagination": {"page": page, "per_page": per_page, "total": total}, - "aggregates": aggregates, - "query": q or "", - "roles": [r.model_dump(mode="json") for r in roles], - "filters": { - "status": clean_status or "all", - "role": role or "", - "verified": clean_verified or "all", - "sort": clean_sort, - "order": clean_order, - }, - }, - ) - - -@router.get( - "/admin/invite", - response_model=None, - dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], -) -async def admin_invite_page( - request: Request, - inertia: InertiaDep, -) -> InertiaResponse: - return await inertia.render( - _PAGE_ADMIN_INVITE, - { - "roles": await _roles_payload(request.app), - }, - ) - - -@router.get( - "/admin/{user_id}", - response_model=None, - dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], -) -async def admin_edit_page( - user_id: str, - request: Request, - inertia: InertiaDep, - service: UserService = Depends(get_user_service), -) -> InertiaResponse: - try: - uid = uuid.UUID(user_id) - except ValueError as exc: - raise HTTPException(status_code=404) from exc - try: - user_item = await service.get_list_item(uid) - except UserNotFoundError: - raise HTTPException(status_code=404) from None - has_permissions = any(m.meta.name == "Permissions" for m in request.app.state.sm.modules) - return await inertia.render( - _PAGE_ADMIN_EDIT, - { - "user": user_item.model_dump(mode="json"), - "roles": await _roles_payload(request.app), - "has_permissions_module": has_permissions, - }, - ) diff --git a/modules/users/users/module.py b/modules/users/users/module.py index 0684da2c..4c0f91e8 100644 --- a/modules/users/users/module.py +++ b/modules/users/users/module.py @@ -4,7 +4,7 @@ from typing import TYPE_CHECKING -from fastapi import APIRouter +from fastapi import APIRouter, Depends from simple_module_core.menu import MenuItem, MenuRegistry, MenuSection from simple_module_core.module import ModuleBase, ModuleMeta from simple_module_core.permissions import PermissionRegistry @@ -110,15 +110,49 @@ def register_menu_items(self, registry: MenuRegistry) -> None: ) def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None: - from users.endpoints.api import register_auth_routes - from users.endpoints.views import router as views + from users.admin.api import admin_router + from users.admin.views import router as admin_views + from users.auth_local import api as auth_local_api + from users.auth_local.views import router as auth_views + from users.contracts.schemas import UserCreate, UserRead + from users.deps import fastapi_users + from users.oauth.api import register_oauth_routes from users.settings import UsersSettings # Construct settings here (re-reads env_str-bound fields like OAuth # client ids/secrets). Validators have already passed by this point — # ``register_settings`` ran first and would have raised on placeholders. - register_auth_routes(api_router, UsersSettings()) - view_router.include_router(views) + settings = UsersSettings() + + api_router.include_router(auth_local_api.router) + api_router.include_router(admin_router) + # Throughput-wrap the stock fastapi-users routers; ``require_signup_enabled`` + # gates /register at request time so ``allow_signup`` is hot-reloadable. + api_router.include_router( + fastapi_users.get_reset_password_router(), + prefix="/auth", + tags=["users-auth"], + dependencies=[Depends(auth_local_api.enforce_auth_throughput_limit)], + ) + api_router.include_router( + fastapi_users.get_verify_router(UserRead), + prefix="/auth", + tags=["users-auth"], + dependencies=[Depends(auth_local_api.enforce_auth_throughput_limit)], + ) + api_router.include_router( + fastapi_users.get_register_router(UserRead, UserCreate), + prefix="/auth", + tags=["users-auth"], + dependencies=[ + Depends(auth_local_api.require_signup_enabled), + Depends(auth_local_api.enforce_auth_throughput_limit), + ], + ) + register_oauth_routes(api_router, settings) + + view_router.include_router(auth_views) + view_router.include_router(admin_views) def register_middleware(self, app: FastAPI) -> None: from users.middleware import AuthMiddleware @@ -129,11 +163,12 @@ async def on_startup(self, app: FastAPI) -> None: """Build the mailer, rate limiter, and apply production cookie params.""" import asyncio + from users.auth_local.rate_limit import LoginRateLimiter, ThroughputLimiter from users.backend import reconfigure_cookie_transport from users.bootstrap import bootstrap_admin_from_env from users.deps import auth_backend from users.mailer import build_mailer - from users.rate_limit import LoginRateLimiter, ThroughputLimiter + from users.oauth.providers import enabled_provider_names from users.roles_cache import refresh_roles_cache state = app.state.users @@ -148,6 +183,7 @@ async def on_startup(self, app: FastAPI) -> None: max_attempts=s.auth_rate_limit_attempts, window_seconds=s.auth_rate_limit_window_seconds, ) + state.oauth_providers = enabled_provider_names(s) # Auto-fall-back from the default ``/dashboard/`` to ``/`` when the # Dashboard module isn't installed, so ``--preset minimal`` doesn't diff --git a/modules/users/users/oauth/__init__.py b/modules/users/users/oauth/__init__.py new file mode 100644 index 00000000..65115482 --- /dev/null +++ b/modules/users/users/oauth/__init__.py @@ -0,0 +1,5 @@ +"""OAuth feature — public surface re-exported for backward compatibility.""" + +from users.oauth.providers import OAuthProvider, build_clients, enabled_provider_names + +__all__ = ["OAuthProvider", "build_clients", "enabled_provider_names"] diff --git a/modules/users/users/endpoints/api_oauth.py b/modules/users/users/oauth/api.py similarity index 98% rename from modules/users/users/endpoints/api_oauth.py rename to modules/users/users/oauth/api.py index 0d0c80fb..ef364a5a 100644 --- a/modules/users/users/endpoints/api_oauth.py +++ b/modules/users/users/oauth/api.py @@ -24,7 +24,7 @@ from starlette.responses import RedirectResponse from users.deps import auth_backend, get_user_manager -from users.oauth import OAuthProvider, build_clients +from users.oauth.providers import OAuthProvider, build_clients if TYPE_CHECKING: from users.manager import UserManager diff --git a/modules/users/users/oauth.py b/modules/users/users/oauth/providers.py similarity index 100% rename from modules/users/users/oauth.py rename to modules/users/users/oauth/providers.py diff --git a/modules/users/users/pages/Users/Index.tsx b/modules/users/users/pages/Users/Index.tsx index 922091c5..be07709c 100644 --- a/modules/users/users/pages/Users/Index.tsx +++ b/modules/users/users/pages/Users/Index.tsx @@ -26,9 +26,9 @@ import { Users, } from 'lucide-react'; import { useCallback, useEffect, useMemo, useState } from 'react'; -import { type Filters, IndexFilters } from '../../components/IndexFilters'; -import { type RoleItem, RolesTab } from '../../components/RolesTab'; -import { type UserListItem, UserRow } from '../../components/UserRow'; +import { type Filters, IndexFilters } from '../../admin/components/IndexFilters'; +import { type RoleItem, RolesTab } from '../../admin/components/RolesTab'; +import { type UserListItem, UserRow } from '../../admin/components/UserRow'; interface Pagination { page: number; diff --git a/modules/users/users/state.py b/modules/users/users/state.py index f9060fd7..28f6d45a 100644 --- a/modules/users/users/state.py +++ b/modules/users/users/state.py @@ -16,8 +16,8 @@ from typing import TYPE_CHECKING if TYPE_CHECKING: + from users.auth_local.rate_limit import LoginRateLimiter, ThroughputLimiter from users.mailer import Mailer - from users.rate_limit import LoginRateLimiter, ThroughputLimiter from users.roles_cache import RoleSummary from users.settings import UsersSettings @@ -31,3 +31,4 @@ class UsersState: rate_limiter: LoginRateLimiter | None = None auth_throughput_limiter: ThroughputLimiter | None = None roles_cache: list[RoleSummary] = field(default_factory=list) + oauth_providers: list[dict[str, str]] = field(default_factory=list) From 1a175ddd85758c68a44d44a57b1205bef328c8d6 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 15 May 2026 18:27:36 +0000 Subject: [PATCH 2/2] fix(users): read login_redirect_url lazily on OAuth callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address two issues raised in PR review: 1. `oauth/api.py` captured `settings.login_redirect_url` as a string at route-registration time, then closed over it. `UsersModule.on_startup` mutates `app.state.users.settings.login_redirect_url` (dashboard fallback), and admins can change it at runtime via the settings UI, so OAuth callbacks could redirect to a stale URL. Now the callback reads it from `request.app.state.users.settings` at request time. `_build_provider_router` no longer takes a `login_redirect_url` parameter. 2. The `register_routes` comment claimed constructing `UsersSettings()` "re-reads env_str-bound fields like OAuth client ids/secrets" — this is wrong: `env_str()` is evaluated when `UsersSettings` is defined (class-attribute defaults), so re-instantiating doesn't re-read env. Updated to accurately describe why a fresh instance is acceptable here (only `build_clients` reads it, and only static fields) and to point readers at `app.state.users.settings` for mutable fields. Both issues were pre-existing latent behaviour surfaced by the review of this PR. Tests still pass (1010); doctor clean. --- modules/users/users/module.py | 8 +++++--- modules/users/users/oauth/api.py | 10 +++++++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/modules/users/users/module.py b/modules/users/users/module.py index 4c0f91e8..fb32b9f2 100644 --- a/modules/users/users/module.py +++ b/modules/users/users/module.py @@ -119,9 +119,11 @@ def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None from users.oauth.api import register_oauth_routes from users.settings import UsersSettings - # Construct settings here (re-reads env_str-bound fields like OAuth - # client ids/secrets). Validators have already passed by this point — - # ``register_settings`` ran first and would have raised on placeholders. + # Consumed only by ``register_oauth_routes`` → ``build_clients`` at + # registration time, which reads class-attribute defaults captured by + # ``env_str()`` at import. Request-time readers of mutable fields + # (e.g. ``login_redirect_url``) must go through + # ``request.app.state.users.settings``, not this instance. settings = UsersSettings() api_router.include_router(auth_local_api.router) diff --git a/modules/users/users/oauth/api.py b/modules/users/users/oauth/api.py index ef364a5a..06131a25 100644 --- a/modules/users/users/oauth/api.py +++ b/modules/users/users/oauth/api.py @@ -35,7 +35,7 @@ _SESSION_STATE_KEY_FMT = "oauth_state:{provider}" -def _build_provider_router(provider: OAuthProvider, login_redirect_url: str) -> APIRouter: +def _build_provider_router(provider: OAuthProvider) -> APIRouter: """Mount /login + /callback for one provider.""" router = APIRouter() state_key = _SESSION_STATE_KEY_FMT.format(provider=provider.name) @@ -105,7 +105,11 @@ async def callback( login_response = await auth_backend.login(strategy, user) await user_manager.on_after_login(user, request, login_response) - redirect = RedirectResponse(login_redirect_url, status_code=303) + # Read login_redirect_url lazily: ``UsersModule.on_startup`` may + # mutate it (e.g. dashboard-fallback) after this router is mounted, + # and admins can change it at runtime via the settings UI. + redirect_url = request.app.state.users.settings.login_redirect_url + redirect = RedirectResponse(redirect_url, status_code=303) for key, value in login_response.headers.items(): if key.lower() == "set-cookie": redirect.raw_headers.append((b"set-cookie", value.encode("latin-1"))) @@ -119,7 +123,7 @@ def register_oauth_routes(api_router: APIRouter, settings: UsersSettings) -> Non providers = build_clients(settings) for provider in providers: api_router.include_router( - _build_provider_router(provider, settings.login_redirect_url), + _build_provider_router(provider), prefix=f"/auth/{provider.name}", tags=["users-auth"], )