Removed reliance on pelicanq fork of fastapi-users#525
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.
| # 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.
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: