houseassassin/remnawave-bedolaga-telegram-bot
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: harden account merge security and correctness

Browse Source 
- Clear ALL unique constraint fields on secondary user after merge
  (telegram_id, OAuth IDs, email, referral_code, remnawave_uuid)
- Add Literal type + runtime validation for keep_subscription_from
- Reject merge when primary user is deleted
- Validate OAuth state user_id matches authenticated user in link callback
- Replace leaked ValueError messages with generic error detail
- Fix exc_info usage for idiomatic structlog
- Fix _get_remnawave_api return type to AsyncIterator
- Remove unnecessary from __future__ import annotations
- Add 3 new tests (42 total, all passing)
This commit is contained in:
Fringg
2026-03-04 07:46:07 +03:00
parent dc7b8dc72a
commit d855e9e47f
3 changed files with 89 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/cabinet/routes/account_linking.py
+21 -15
View File
@@ -18,7 +18,7 @@ from app.database.crud.user import (
set_user_oauth_provider_id,
)
from app.database.models import User
from app.services.account_merge_service import execute_merge, get_merge_preview
from app.services.account_merge_service import _compute_auth_methods, execute_merge, get_merge_preview
from ..auth.merge_service import (
MERGE_TOKEN_TTL_SECONDS,
@@ -137,15 +137,7 @@ def _get_provider_identifier(user: User, provider: str) -> str | None:
def _count_auth_methods(user: User) -> int:
"""Count how many auth methods the user has linked."""
count = 0
if user.telegram_id:
count += 1
if user.email and user.password_hash:
count += 1
for column in _OAUTH_PROVIDER_COLUMNS.values():
if getattr(user, column, None):
count += 1
return count
return len(_compute_auth_methods(user))
# ---------------------------------------------------------------------------
@@ -237,6 +229,20 @@ async def link_provider_callback(
detail='Invalid or expired OAuth state',
)
# 1b. Validate that the user who initiated the link flow is the same user completing it
state_user_id = state_data.get('user_id')
if state_user_id and str(user.id) != state_user_id:
logger.warning(
'OAuth state user_id mismatch in link callback',
state_user_id=state_user_id,
current_user_id=user.id,
provider=provider,
)
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail='OAuth state was initiated by a different user',
)
# 2. Get provider instance
oauth_provider = get_provider(provider)
if not oauth_provider:
@@ -256,7 +262,7 @@ async def link_provider_callback(
try:
token_data = await oauth_provider.exchange_code(request.code, **exchange_kwargs)
except Exception as exc:
logger.error('OAuth code exchange failed during linking', provider=provider, exc_info=exc)
logger.error('OAuth code exchange failed during linking', provider=provider, exc_info=True)
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail='Failed to exchange authorization code',
@@ -266,7 +272,7 @@ async def link_provider_callback(
try:
user_info = await oauth_provider.get_user_info(token_data)
except Exception as exc:
logger.error('OAuth user info fetch failed during linking', provider=provider, exc_info=exc)
logger.error('OAuth user info fetch failed during linking', provider=provider, exc_info=True)
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail='Failed to fetch user information from provider',
@@ -445,11 +451,11 @@ async def execute_merge_endpoint(
logger.error('Merge execution failed (ValueError)', error=str(exc))
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail=str(exc),
detail='Account merge cannot be completed. The accounts may have already been merged or deleted.',
) from exc
except Exception as exc:
await db.rollback()
logger.error('Merge execution failed', exc_info=exc)
logger.error('Merge execution failed', exc_info=True)
raise HTTPException(
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
detail='Account merge failed due to an internal error',
@@ -468,7 +474,7 @@ async def execute_merge_endpoint(
auth_response = await _create_auth_response(merged_user, db)
await _store_refresh_token(db, merged_user.id, auth_response.refresh_token, device_info='merge')
except Exception as exc:
logger.error('Failed to create auth tokens after merge', exc_info=exc)
logger.error('Failed to create auth tokens after merge', exc_info=True)
raise HTTPException(
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
detail='Merge succeeded but failed to create new session',
app/services/account_merge_service.py
+15 -7
View File
@@ -1,8 +1,7 @@
from __future__ import annotations
from collections.abc import AsyncIterator
from contextlib import asynccontextmanager
from datetime import UTC, datetime
from typing import Any
from typing import Any, Literal
import structlog
from sqlalchemy import update
@@ -148,7 +147,7 @@ async def get_merge_preview(
@asynccontextmanager
async def _get_remnawave_api() -> RemnaWaveAPI:
async def _get_remnawave_api() -> AsyncIterator[RemnaWaveAPI]:
"""Создаёт экземпляр RemnaWave API клиента (паттерн из RemnaWaveService)."""
auth_params = settings.get_remnawave_auth_params()
base_url = (auth_params.get('base_url') or '').strip()
@@ -308,7 +307,7 @@ async def execute_merge(
db: AsyncSession,
primary_user_id: int,
secondary_user_id: int,
keep_subscription_from: str = 'primary',
keep_subscription_from: Literal['primary', 'secondary'] = 'primary',
provider: str | None = None,
provider_id: str | None = None,
) -> User:
@@ -330,6 +329,9 @@ async def execute_merge(
Raises:
ValueError: Если пользователь не найден, совпадают ID, или secondary уже удалён.
"""
if keep_subscription_from not in ('primary', 'secondary'):
raise ValueError("keep_subscription_from must be 'primary' or 'secondary'")
if primary_user_id == secondary_user_id:
raise ValueError('primary_user_id и secondary_user_id не могут совпадать')
@@ -338,6 +340,8 @@ async def execute_merge(
if not primary:
raise ValueError(f'Основной пользователь (id={primary_user_id}) не найден')
if primary.status == UserStatus.DELETED.value:
raise ValueError(f'Основной пользователь (id={primary_user_id}) удалён')
if not secondary:
raise ValueError(f'Вторичный пользователь (id={secondary_user_id}) не найден')
if secondary.status == UserStatus.DELETED.value:
@@ -461,13 +465,17 @@ async def execute_merge(
value=primary.referral_commission_percent,
)
# 14. Помечаем secondary как удалённый
# 14. Помечаем secondary как удалённый и очищаем ВСЕ unique constraint поля
secondary.status = UserStatus.DELETED.value
secondary.referral_code = None
secondary.remnawave_uuid = None
# email уже очищен выше если был перенесён, иначе очищаем для unique constraint
if secondary.email:
secondary.email = None
if secondary.telegram_id:
secondary.telegram_id = None
for field in _OAUTH_FIELDS:
if getattr(secondary, field) is not None:
setattr(secondary, field, None)
secondary.updated_at = now
logger.info(
tests/services/test_account_merge_service.py
+53
View File
@@ -273,6 +273,23 @@ class TestExecuteMergeValidation:
with pytest.raises(ValueError, match='уже удалён'):
await execute_merge(db, 1, 2)
async def test_deleted_primary_raises(self, monkeypatch):
db = _make_db()
primary = _make_user(id=1, status='deleted')
secondary = _make_user(id=2)
monkeypatch.setattr(
account_merge_service,
'get_user_by_id',
AsyncMock(side_effect=[primary, secondary]),
)
with pytest.raises(ValueError, match='удалён'):
await execute_merge(db, 1, 2)
async def test_invalid_keep_subscription_from_raises(self):
db = _make_db()
with pytest.raises(ValueError, match=r'primary.*secondary'):
await execute_merge(db, 1, 2, keep_subscription_from='invalid')
# ---------------------------------------------------------------------------
# execute_merge — data transfer
@@ -325,6 +342,8 @@ class TestExecuteMergeOAuthTransfer:
# Primary keeps its own google_id
assert result.google_id == 'g_primary'
# Secondary's conflicting google_id is cleared (unique constraint cleanup)
assert secondary.google_id is None
class TestExecuteMergeTelegramTransfer:
@@ -513,6 +532,40 @@ class TestExecuteMergeSecondaryDeleted:
assert secondary.remnawave_uuid is None
assert secondary.email is None
async def test_all_unique_fields_cleared_on_secondary(self, monkeypatch):
"""All unique constraint fields must be cleared on secondary after merge."""
db = _make_db()
# Primary has its own OAuth + telegram, so secondary's won't transfer
primary = _make_user(id=1, telegram_id=111, google_id='g1', yandex_id='y1')
secondary = _make_user(
id=2,
telegram_id=222,
google_id='g2',
yandex_id='y2',
discord_id='d2',
vk_id=999,
email='sec@e.com',
referral_code='REF',
remnawave_uuid='rw-sec',
)
monkeypatch.setattr(
account_merge_service,
'get_user_by_id',
AsyncMock(side_effect=[primary, secondary]),
)
with _patch_remnawave_delete():
await execute_merge(db, 1, 2)
# ALL unique fields cleared on secondary
assert secondary.telegram_id is None
assert secondary.google_id is None
assert secondary.yandex_id is None
assert secondary.discord_id is None
assert secondary.vk_id is None
assert secondary.email is None
assert secondary.referral_code is None
assert secondary.remnawave_uuid is None
async def test_db_flush_called(self, monkeypatch):
db = _make_db()
primary = _make_user(id=1)