Removed reliance on pelicanq fork of fastapi-users#525
Removed reliance on pelicanq fork of fastapi-users#525georgelgeback wants to merge 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the authentication/user-management integration off the fastapi-users-pelicanq fork onto upstream fastapi-users (and its companion SQLAlchemy adapter), and adjusts the permission system to close a privilege-escalation gap while adding regression tests.
Changes:
- Replaced
fastapi-users-pelicanqimports with upstreamfastapi-usersand updated related package pins. - Reworked permission evaluation to rely on DB-backed permissions (and tightened “manage” vs “super” behavior).
- Added new permission-focused tests and expanded fixtures to support them.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| user/user_stuff.py | Swaps to upstream fastapi-users and introduces a sync→async session proxy for the user DB adapter. |
| user/user_manager.py | Updates fastapi-users imports for the user manager. |
| user/token_strategy.py | Removes custom JWT permissions strategy; uses upstream JWTStrategy. |
| user/refresh_auth_backend.py | Updates fastapi-users import paths for refresh backend types. |
| user/permission.py | Updates permission checks to validate against DB permissions and adjusts “manage/super” logic. |
| user/custom_auth_router.py | Updates fastapi-users import paths for custom auth routes. |
| routes/user_router.py | Updates BaseUserManager import path. |
| routes/permission_router.py | Adds /permissions/me endpoint returning the current member’s permissions. |
| routes/auth_router.py | Updates schema import path. |
| tests/test_permissions.py | Adds regression tests for permission hierarchy and /permissions/me. |
| tests/basic_fixtures.py | Refactors admin fixtures and adds view/manage/super post fixtures. |
| db_models/user_model.py | Updates fastapi-users SQLAlchemy base table import path. |
| api_schemas/user_schemas.py | Updates fastapi-users schemas import path. |
| requirements.txt | Replaces pelicanq fork packages with upstream packages and bumps related dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from fastapi_users_pelicanq.db import SQLAlchemyUserDatabase | ||
| from typing import Any, cast | ||
| from fastapi_users.authentication import AuthenticationBackend, BearerTransport, CookieTransport | ||
| from fastapi_users.db import SQLAlchemyUserDatabase |
There was a problem hiding this comment.
SQLAlchemyUserDatabase is being imported from fastapi_users.db, but requirements.txt adds fastapi-users-db-sqlalchemy (the package that typically provides the SQLAlchemy integration). If SQLAlchemyUserDatabase isn’t re-exported by fastapi_users, this will fail at import time. Consider importing it from fastapi_users_db_sqlalchemy to match the dependency you pinned.
| from fastapi_users.db import SQLAlchemyUserDatabase | |
| from fastapi_users_db_sqlalchemy import SQLAlchemyUserDatabase |
| # This is such a horrible hack, but the alternative is essentially a full refactor of the code | ||
| class _AsyncSessionProxy: | ||
| """Expose an async-session-like interface on top of a sync Session.""" | ||
|
|
||
| def __init__(self, session: Session): | ||
| self._session = session | ||
|
|
||
| def add(self, instance: Any) -> None: | ||
| self._session.add(instance) | ||
|
|
||
| async def execute(self, statement: Any, *args: Any, **kwargs: Any) -> Any: | ||
| return self._session.execute(statement, *args, **kwargs) | ||
|
|
||
| async def commit(self) -> None: | ||
| self._session.commit() | ||
|
|
||
| async def refresh(self, instance: Any) -> None: | ||
| self._session.refresh(instance) | ||
|
|
||
| async def delete(self, instance: Any) -> None: | ||
| self._session.delete(instance) | ||
|
|
There was a problem hiding this comment.
_AsyncSessionProxy only implements a small subset of the AsyncSession interface, but it’s being passed to SQLAlchemyUserDatabase and type-cast to AsyncSession. If the fastapi-users SQLAlchemy adapter calls other session APIs (e.g. rollback/close/get/scalars), this will raise runtime attribute errors. A safer approach is to either delegate unknown attributes via __getattr__ (and implement commonly-used async methods like rollback()/close()), or switch to a real AsyncSession / run sync DB calls via run_in_threadpool.
| @@ -1,5 +1,5 @@ | |||
| from typing import TYPE_CHECKING, Callable, Optional | |||
| from fastapi_users_pelicanq.db import SQLAlchemyBaseUserTable | |||
| from fastapi_users.db import SQLAlchemyBaseUserTable | |||
There was a problem hiding this comment.
SQLAlchemyBaseUserTable is imported from fastapi_users.db, but with the dependency split (fastapi-users-db-sqlalchemy is pinned in requirements) the SQLAlchemy base table class is usually provided by fastapi_users_db_sqlalchemy. If fastapi_users doesn’t re-export this symbol, this will break at import time; consider importing from fastapi_users_db_sqlalchemy for compatibility with the pinned packages.
| from fastapi_users.db import SQLAlchemyBaseUserTable | |
| from fastapi_users_db_sqlalchemy import SQLAlchemyBaseUserTable |
|
|
||
|
|
||
| @permission_router.get("/me", response_model=list[tuple[str, str]] | None) | ||
| def get_my_permissions(member: Annotated[User_DB | None, Permission.check_member()]): |
There was a problem hiding this comment.
Permission.check_member() appears to return False when the requester is anonymous or not a member, but this parameter is annotated as User_DB | None. This works with if not member, but the type hint is misleading (and can confuse editors/mypy). Consider returning None from the dependency instead of False, or widening the annotation here to include bool.
| def get_my_permissions(member: Annotated[User_DB | None, Permission.check_member()]): | |
| def get_my_permissions(member: Annotated[User_DB | bool | None, Permission.check_member()]): |
| if claim_target != required_target and claim_target != "all": | ||
| return False |
There was a problem hiding this comment.
The comment immediately above this block (re: “recieve a token with pemissions…”) is now misleading because permission checks no longer decode JWT permissions and instead iterate DB permissions (user.posts -> post.permissions). Please update/remove that comment (and fix the typos) so it matches the current behavior.
This PR uses a pretty unsatisfying method of wrapping the poor async fastapi-users so that we can interact with it in a non-async manner. That wrapper is pretty vibe coded, not my area of expertise. Allegedly you could also choose to have two connections to the database, one sync and one async, and then rewrite user management, permission management and auth using async. You could also rewrite the entire backend to be async, but both of these would need quite a lot of work (no more direct references to db objects, update all the tests, querying in a different way etc.) so I thought this was the best alternative.
I think there could also be room for forking our own version of fastapi-users and only removing the async parts, if we really don't want the async wrapper.
This PR needs to be pushed to prod at the same time as the corresponding frontend one: