Feature/roles and ban system#32
Conversation
…d fix date fields by adding the timezone=True property to store time zone information. Fix and add missing relationships
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…d also fix the issue of multiple bans for one user simultaneously, while preserving the ban history.
…e for consistency
* delete cloudflared.deb" * feat(models): Start implementing the role system, add a ban table, and fix date fields by adding the timezone=True property to store time zone information. Fix and add missing relationships * chore(.gitignore): remove tracking of different caches * refactor(tg_link): rename MagicTokens to MagicToken Co-authored-by: Cursor <cursoragent@cursor.com> * fix(bot): send login-link URL on start when telegram is not linked Co-authored-by: Cursor <cursoragent@cursor.com> * test(auth): add JWT unit tests Co-authored-by: Cursor <cursoragent@cursor.com> * refactor(models): Improve the readability of field checks in the bans table * feat(models): Add validation of the revoked_at field at the database level * feat(models): Add checks for the expires_at and revoked_at fields, and also fix the issue of multiple bans for one user simultaneously, while preserving the ban history. * refactor(models): Rename MagicTokens to MagicToken across the codebase for consistency * fix(modedls): Add foreigkey to the appointments table relationship * fix(tasks): Fix database issues with the background task for clearing magic tokens --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…18) * feat(ci): Add Docker image building and pushing to the ghcr registry * feat(docker-compose): Add watchtower to query the registry and download dcoker images from there. * feat(ci): Add a commit hash tag to Docker images
* fix: Remove the get_db file from infrastructure and remove logging of all SQL queries to reduce the IO load. * chore(jwt): Remove dprecated definition of the current time
Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com>
…al network (#22) Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com>
* fix(auth): Fix a vulnerability in the process of linking a Telegram bot to an account by adding X-Bot-Secret to the header * chore(auth): Improve logging of the create_telegram_magic_link function * fix(auth): Fix X-Bot-Secret authentication. Eliminates timig attack. * chore(requirements): update requrements.txt list * fix(bot): Add sending of the X-Bot-Secret header from the Telegram bot * fix(config): Remove os.environ, which causes an error in tests * Update comment src/presentation/api/v1/auth/tg_link.py Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> * docs(services): Fix logging and doc string errors --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
* feat(docker): Isolate services by separating docker networks * feat(docker): Add a system user app. Remove buffering and .pyc, __pycache__ files. * feat(docker): Remove root privileges. Add cap_drop to disable root privileges. Enable write access to the /tmp folder. * feat(logger): Remove logging to a file * feat(docker): Write a user creation command in the api container and transfer the user change to the bot container * docs: Correct syntax errors --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com>
Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com>
* feat: Add a health endpoint to check if the API is online * feat(docker): Add cleaning of outdated Docker images * fix: Change your HealthCheck internet connection to avoid being blocked by RateLimiting --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com>
#29) * feat: Bring the architecture to a clean archeticture. In the structure, repeat the project structure for easier navigation. Separate the models into different files. * fix(models): Fix circular import * fix(models): Fix circular import * fix(models): correct error texts in models --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com>
* fix: Delete alembic.ini * chore: add alembic.ini into .gitignore * chore: update alembic scheme * fix: Remove the backup key from config.py * fix: Fix the SECRET_KEY test environment variable * fix(auth): Fixing a missing SECRET_KEY error in .env * fix(tests): Fix E401 [*] Multiple imports on one line * feat(config): Add a check for the absence of DATABSE_URL in .env * Add a database engine to lifespan's fastapi * refactor: Move session factory to lifespan --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com>
Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com>
Руководство для ревьюераРеализует плановую очистку просроченных банов вместе с рефакторингом жизненного цикла DB-сессий к централизованному асинхронному SessionLocal, переносом запуска планировщика в lifespan FastAPI и ужесточением ограничений уникальности банов и проверок безопасности вокруг инициализации БД. Диаграмма последовательностей для старта FastAPI, инициализации DB-сессии и плановой очистки бановsequenceDiagram
participant FastAPIApp as FastAPI_app
participant Lifespan as lifespan
participant DBModule as database
participant Scheduler as AsyncIOScheduler
participant BanTask as remove_expired_bans
participant TokenTask as cleanup_telegram_tokens
participant GetDB as get_db
FastAPIApp->>Lifespan: lifespan(app)
activate Lifespan
Lifespan->>DBModule: SessionLocal = async_sessionmaker(engine)
Lifespan->>Scheduler: add_job(cleanup_telegram_tokens, interval 5m)
Lifespan->>Scheduler: add_job(remove_expired_bans, interval 10m)
Lifespan->>Scheduler: start()
loop every 10 minutes
Scheduler->>BanTask: remove_expired_bans()
activate BanTask
BanTask->>DBModule: SessionLocal
BanTask->>DBModule: SessionLocal()
DBModule-->>BanTask: AsyncSession
BanTask->>BanTask: update(Ban).where(expires_at < now, revoked_at is None)
BanTask->>BanTask: commit()
deactivate BanTask
end
loop request handling
FastAPIApp->>GetDB: get_db()
activate GetDB
alt [SessionLocal is None]
GetDB->>GetDB: raise RuntimeError("Database is not initialied")
else [SessionLocal is set]
GetDB->>DBModule: SessionLocal()
DBModule-->>GetDB: AsyncSession
GetDB-->>FastAPIApp: AsyncSession
end
deactivate GetDB
end
Lifespan-->>FastAPIApp: shutdown
Lifespan->>Scheduler: dispose()
Lifespan->>DBModule: engine.dispose()
deactivate Lifespan
Изменения на уровне файлов
Подсказки и командыВзаимодействие с Sourcery
Настройка работыЗайдите в свою панель управления, чтобы:
Получение помощи
Original review guide in EnglishReviewer's GuideImplements scheduled cleanup for expired bans alongside refactoring DB session lifecycle to a central async SessionLocal, moving scheduler startup into FastAPI lifespan, and tightening ban uniqueness constraints and safety checks around DB initialization. Sequence diagram for FastAPI startup, DB session init, and scheduled ban cleanupsequenceDiagram
participant FastAPIApp as FastAPI_app
participant Lifespan as lifespan
participant DBModule as database
participant Scheduler as AsyncIOScheduler
participant BanTask as remove_expired_bans
participant TokenTask as cleanup_telegram_tokens
participant GetDB as get_db
FastAPIApp->>Lifespan: lifespan(app)
activate Lifespan
Lifespan->>DBModule: SessionLocal = async_sessionmaker(engine)
Lifespan->>Scheduler: add_job(cleanup_telegram_tokens, interval 5m)
Lifespan->>Scheduler: add_job(remove_expired_bans, interval 10m)
Lifespan->>Scheduler: start()
loop every 10 minutes
Scheduler->>BanTask: remove_expired_bans()
activate BanTask
BanTask->>DBModule: SessionLocal
BanTask->>DBModule: SessionLocal()
DBModule-->>BanTask: AsyncSession
BanTask->>BanTask: update(Ban).where(expires_at < now, revoked_at is None)
BanTask->>BanTask: commit()
deactivate BanTask
end
loop request handling
FastAPIApp->>GetDB: get_db()
activate GetDB
alt [SessionLocal is None]
GetDB->>GetDB: raise RuntimeError("Database is not initialied")
else [SessionLocal is set]
GetDB->>DBModule: SessionLocal()
DBModule-->>GetDB: AsyncSession
GetDB-->>FastAPIApp: AsyncSession
end
deactivate GetDB
end
Lifespan-->>FastAPIApp: shutdown
Lifespan->>Scheduler: dispose()
Lifespan->>DBModule: engine.dispose()
deactivate Lifespan
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Coverage Report for CI Build 26831015150Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.1%) to 30.025%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions12 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Привет — я нашёл 4 проблемы и оставил несколько общих замечаний:
- В остановке планировщика в
lifespanиспользуетсяscheduler.dispose(), но уAsyncIOSchedulerесть методshutdown()для корректной остановки; вызов несуществующего метода приведёт к ошибке при завершении работы. - Фоновые задачи вроде
remove_expired_bansиcleanup_telegram_tokensиспользуютSessionLocalнапрямую без проверки наNone, поэтому, если планировщик сработает до инициализацииSessionLocalили после освобождения engine, задачи упадут; подумайте о повторном использовании паттернаget_dbили явных проверках. - В
remove_expired_bansвы вызываетеdatetime.now(timezone.utc)дважды (вwhereиvalues), что может привести к слегка несовпадающим временным меткам; вычислите значение один раз и используйте его и в фильтре, и вrevoked_at.
Prompt for AI Agents
Пожалуйста, исправьте замечания из этого code review:
## Общие комментарии
- В остановке планировщика в `lifespan` используется `scheduler.dispose()`, но у `AsyncIOScheduler` есть метод `shutdown()` для корректной остановки; вызов несуществующего метода приведёт к ошибке при завершении работы.
- Фоновые задачи вроде `remove_expired_bans` и `cleanup_telegram_tokens` используют `SessionLocal` напрямую без проверки на `None`, поэтому, если планировщик сработает до инициализации `SessionLocal` или после освобождения engine, задачи упадут; подумайте о повторном использовании паттерна `get_db` или явных проверках.
- В `remove_expired_bans` вы вызываете `datetime.now(timezone.utc)` дважды (в `where` и `values`), что может привести к слегка несовпадающим временным меткам; вычислите значение один раз и используйте его и в фильтре, и в `revoked_at`.
## Индивидуальные комментарии
### Комментарий 1
<location path="main.py" line_range="37-46" />
<code_context>
)
- app.state.SessionLocal = SessionLocal
+ @app.on_event("startup")
+ async def startup():
+ scheduler.add_job(
+ cleanup_telegram_tokens,
+ "interval",
+ minutes=5
+ )
+ scheduler.add_job(
+ remove_expired_bans,
+ "interval",
+ minutes=10
+ )
+ scheduler.start()
+
# При старте выполняется ко до yiled
</code_context>
<issue_to_address>
**issue (bug_risk):** Регистрация обработчика события startup внутри контекста lifespan может повлиять на то, когда именно задачи будут запланированы.
Поскольку декоратор теперь выполняется при входе в контекст lifespan, а не при импорте модуля, обработчик может быть зарегистрирован слишком поздно или более одного раза, если контекст будет входиться повторно. Рассмотрите варианты:
- Оставить обработчик `on_event("startup")` на этапе импорта модуля и выполнять только привязку `SessionLocal` в `lifespan`, или
- Перенести планирование задач полностью в `lifespan` (без `on_event`), чтобы явно контролировать момент добавления задач и запуска планировщика.
Также убедитесь с помощью тестов или логов, что логика startup выполняется ровно один раз в нужный момент жизненного цикла приложения.
</issue_to_address>
### Комментарий 2
<location path="main.py" line_range="55" />
<code_context>
yield
# выполняется код после yiled и остановка
+ scheduler.dispose()
await engine.dispose()
</code_context>
<issue_to_address>
**issue (bug_risk):** Использование `dispose()` для `AsyncIOScheduler`, скорее всего, некорректно; `shutdown()` — это ожидаемый API.
`AsyncIOScheduler` в APScheduler 3.x должен останавливаться через `scheduler.shutdown(...)`, а не `dispose()`. `dispose()` — это метод SQLAlchemy engine и, скорее всего, приведёт здесь к `AttributeError`. Замените его на `scheduler.shutdown()` (при необходимости настроив параметр `wait`).
</issue_to_address>
### Комментарий 3
<location path="src/infrastructure/db/database.py" line_range="13" />
<code_context>
+
+async def get_db():
+ if SessionLocal is None:
+ raise RuntimeError("Database is not initialied")
+ async with SessionLocal() as session:
yield session
</code_context>
<issue_to_address>
**nitpick (typo):** Опечатка в сообщении RuntimeError.
Замените `initialied` на `initialized` в тексте ошибки для более понятных логов.
</issue_to_address>
### Комментарий 4
<location path="src/infrastructure/tasks/remove_bans.py" line_range="7-16" />
<code_context>
+from src.domain.models.ban_model import Ban
+from src.infrastructure.db.database import SessionLocal
+
+async def remove_expired_bans():
+ async with SessionLocal() as db:
+ await db.execute(update(Ban).where(
+ Ban.expires_at.is_not(None),
+ Ban.expires_at < datetime.now(timezone.utc),
+ Ban.revoked_at.is_(None)
+ ).values(
+ revoked_at = datetime.now(timezone.utc),
+ revoked_reason="Expiration of the Term"
+ )
+ )
+
+ await db.commit()
\ No newline at end of file
</code_context>
<issue_to_address>
**suggestion:** Дважды использовать `datetime.now(timezone.utc)` в одном и том же update может привести к слегка несовпадающим временным меткам.
Здесь `datetime.now(timezone.utc)` вычисляется отдельно для условия `where` и для `revoked_at`. Под нагрузкой они могут немного отличаться, что усложняет анализ данных. Вычислите `now = datetime.now(timezone.utc)` один раз (например, в начале функции) и используйте его в обоих местах, чтобы сравнение и сохраняемая временная метка были согласованы.
</issue_to_address>Sourcery бесплатен для open source — если вам нравятся наши обзоры, пожалуйста, поделитесь ими ✨
Original comment in English
Hey - I've found 4 issues, and left some high level feedback:
- The scheduler teardown in
lifespanusesscheduler.dispose(), butAsyncIOSchedulerexposesshutdown()for a clean stop; calling a non-existent method will raise at shutdown. - Background jobs like
remove_expired_bansandcleanup_telegram_tokensuseSessionLocaldirectly without aNoneguard, so if the scheduler fires beforeSessionLocalis initialized or after the engine is disposed, they will fail; consider reusing theget_dbpattern or adding explicit checks. - In
remove_expired_bansyou calldatetime.now(timezone.utc)twice (in thewhereandvalues), which can produce slightly inconsistent timestamps; compute it once and reuse the same value for both the filter andrevoked_at.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The scheduler teardown in `lifespan` uses `scheduler.dispose()`, but `AsyncIOScheduler` exposes `shutdown()` for a clean stop; calling a non-existent method will raise at shutdown.
- Background jobs like `remove_expired_bans` and `cleanup_telegram_tokens` use `SessionLocal` directly without a `None` guard, so if the scheduler fires before `SessionLocal` is initialized or after the engine is disposed, they will fail; consider reusing the `get_db` pattern or adding explicit checks.
- In `remove_expired_bans` you call `datetime.now(timezone.utc)` twice (in the `where` and `values`), which can produce slightly inconsistent timestamps; compute it once and reuse the same value for both the filter and `revoked_at`.
## Individual Comments
### Comment 1
<location path="main.py" line_range="37-46" />
<code_context>
)
- app.state.SessionLocal = SessionLocal
+ @app.on_event("startup")
+ async def startup():
+ scheduler.add_job(
+ cleanup_telegram_tokens,
+ "interval",
+ minutes=5
+ )
+ scheduler.add_job(
+ remove_expired_bans,
+ "interval",
+ minutes=10
+ )
+ scheduler.start()
+
# При старте выполняется ко до yiled
</code_context>
<issue_to_address>
**issue (bug_risk):** Registering the startup event handler inside the lifespan context may affect when the jobs are actually scheduled.
Because the decorator now runs when the lifespan context is entered rather than at import time, the handler may be registered too late or more than once if the context is re‑entered. Consider either:
- Keeping the `on_event("startup")` handler at module import time and only wiring `SessionLocal` in `lifespan`, or
- Moving the job scheduling entirely into `lifespan` (without `on_event`) so you explicitly control when jobs are added and the scheduler is started.
Also verify via tests or logs that the startup logic runs exactly once at the intended point in the app lifecycle.
</issue_to_address>
### Comment 2
<location path="main.py" line_range="55" />
<code_context>
yield
# выполняется код после yiled и остановка
+ scheduler.dispose()
await engine.dispose()
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `dispose()` on `AsyncIOScheduler` is likely incorrect; `shutdown()` is the intended API.
`AsyncIOScheduler` in APScheduler 3.x should be stopped with `scheduler.shutdown(...)`, not `dispose()`. `dispose()` is a SQLAlchemy engine method and will likely trigger an `AttributeError` here. Replace this with `scheduler.shutdown()` (optionally configuring `wait`).
</issue_to_address>
### Comment 3
<location path="src/infrastructure/db/database.py" line_range="13" />
<code_context>
+
+async def get_db():
+ if SessionLocal is None:
+ raise RuntimeError("Database is not initialied")
+ async with SessionLocal() as session:
yield session
</code_context>
<issue_to_address>
**nitpick (typo):** Typo in the RuntimeError message.
Change `initialied` to `initialized` in the error message for clearer logs.
</issue_to_address>
### Comment 4
<location path="src/infrastructure/tasks/remove_bans.py" line_range="7-16" />
<code_context>
+from src.domain.models.ban_model import Ban
+from src.infrastructure.db.database import SessionLocal
+
+async def remove_expired_bans():
+ async with SessionLocal() as db:
+ await db.execute(update(Ban).where(
+ Ban.expires_at.is_not(None),
+ Ban.expires_at < datetime.now(timezone.utc),
+ Ban.revoked_at.is_(None)
+ ).values(
+ revoked_at = datetime.now(timezone.utc),
+ revoked_reason="Expiration of the Term"
+ )
+ )
+
+ await db.commit()
\ No newline at end of file
</code_context>
<issue_to_address>
**suggestion:** Using `datetime.now(timezone.utc)` twice inside the same update may lead to slightly inconsistent timestamps.
Here, `datetime.now(timezone.utc)` is evaluated separately for the `where` clause and `revoked_at`. Under load, these can differ slightly, making data harder to reason about. Compute `now = datetime.now(timezone.utc)` once (e.g., at the start of the function) and reuse it in both places to keep the comparison and stored timestamp aligned.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @app.on_event("startup") | ||
| async def startup(): | ||
| scheduler.add_job( | ||
| cleanup_telegram_tokens, | ||
| "interval", | ||
| minutes=5 | ||
| ) | ||
| scheduler.add_job( | ||
| remove_expired_bans, | ||
| "interval", |
There was a problem hiding this comment.
issue (bug_risk): Регистрация обработчика события startup внутри контекста lifespan может повлиять на то, когда именно задачи будут запланированы.
Поскольку декоратор теперь выполняется при входе в контекст lifespan, а не при импорте модуля, обработчик может быть зарегистрирован слишком поздно или более одного раза, если контекст будет входиться повторно. Рассмотрите варианты:
- Оставить обработчик
on_event("startup")на этапе импорта модуля и выполнять только привязкуSessionLocalвlifespan, или - Перенести планирование задач полностью в
lifespan(безon_event), чтобы явно контролировать момент добавления задач и запуска планировщика.
Также убедитесь с помощью тестов или логов, что логика startup выполняется ровно один раз в нужный момент жизненного цикла приложения.
Original comment in English
issue (bug_risk): Registering the startup event handler inside the lifespan context may affect when the jobs are actually scheduled.
Because the decorator now runs when the lifespan context is entered rather than at import time, the handler may be registered too late or more than once if the context is re‑entered. Consider either:
- Keeping the
on_event("startup")handler at module import time and only wiringSessionLocalinlifespan, or - Moving the job scheduling entirely into
lifespan(withouton_event) so you explicitly control when jobs are added and the scheduler is started.
Also verify via tests or logs that the startup logic runs exactly once at the intended point in the app lifecycle.
| yield | ||
| # выполняется код после yiled и остановка | ||
|
|
||
| scheduler.dispose() |
There was a problem hiding this comment.
issue (bug_risk): Использование dispose() для AsyncIOScheduler, скорее всего, некорректно; shutdown() — это ожидаемый API.
AsyncIOScheduler в APScheduler 3.x должен останавливаться через scheduler.shutdown(...), а не dispose(). dispose() — это метод SQLAlchemy engine и, скорее всего, приведёт здесь к AttributeError. Замените его на scheduler.shutdown() (при необходимости настроив параметр wait).
Original comment in English
issue (bug_risk): Using dispose() on AsyncIOScheduler is likely incorrect; shutdown() is the intended API.
AsyncIOScheduler in APScheduler 3.x should be stopped with scheduler.shutdown(...), not dispose(). dispose() is a SQLAlchemy engine method and will likely trigger an AttributeError here. Replace this with scheduler.shutdown() (optionally configuring wait).
|
|
||
| async def get_db(): | ||
| if SessionLocal is None: | ||
| raise RuntimeError("Database is not initialied") |
There was a problem hiding this comment.
nitpick (typo): Опечатка в сообщении RuntimeError.
Замените initialied на initialized в тексте ошибки для более понятных логов.
Original comment in English
nitpick (typo): Typo in the RuntimeError message.
Change initialied to initialized in the error message for clearer logs.
| async def remove_expired_bans(): | ||
| async with SessionLocal() as db: | ||
| await db.execute(update(Ban).where( | ||
| Ban.expires_at.is_not(None), | ||
| Ban.expires_at < datetime.now(timezone.utc), | ||
| Ban.revoked_at.is_(None) | ||
| ).values( | ||
| revoked_at = datetime.now(timezone.utc), | ||
| revoked_reason="Expiration of the Term" | ||
| ) |
There was a problem hiding this comment.
suggestion: Дважды использовать datetime.now(timezone.utc) в одном и том же update может привести к слегка несовпадающим временным меткам.
Здесь datetime.now(timezone.utc) вычисляется отдельно для условия where и для revoked_at. Под нагрузкой они могут немного отличаться, что усложняет анализ данных. Вычислите now = datetime.now(timezone.utc) один раз (например, в начале функции) и используйте его в обоих местах, чтобы сравнение и сохраняемая временная метка были согласованы.
Original comment in English
suggestion: Using datetime.now(timezone.utc) twice inside the same update may lead to slightly inconsistent timestamps.
Here, datetime.now(timezone.utc) is evaluated separately for the where clause and revoked_at. Under load, these can differ slightly, making data harder to reason about. Compute now = datetime.now(timezone.utc) once (e.g., at the start of the function) and reuse it in both places to keep the comparison and stored timestamp aligned.
Summary by Sourcery
Введение плановых задач обслуживания и корректировка работы сессий базы данных для очистки блокировок и токенов.
Новые возможности:
Улучшения:
Original summary in English
Summary by Sourcery
Introduce scheduled maintenance tasks and adjust database session handling for ban and token cleanup.
New Features:
Enhancements:
Original summary in English
Summary by Sourcery
Введение плановых задач обслуживания и корректировка работы сессий базы данных для очистки блокировок и токенов.
Новые возможности:
Улучшения:
Original summary in English
Summary by Sourcery
Introduce scheduled maintenance tasks and adjust database session handling for ban and token cleanup.
New Features:
Enhancements: