From 0881998e5bac2fd754b35468bbefd2b1400bddf5 Mon Sep 17 00:00:00 2001 From: Jack Kingsman Date: Sat, 14 Mar 2026 22:58:14 -0700 Subject: [PATCH] Overhaul repeater interaction to better deal with login failure clearly --- app/models.py | 5 + app/routers/repeaters.py | 110 ++++++++++++++---- frontend/src/hooks/useRepeaterDashboard.ts | 22 +++- .../src/test/useRepeaterDashboard.test.ts | 48 +++++++- frontend/src/types.ts | 2 + tests/test_repeater_routes.py | 90 +++++++++++++- 6 files changed, 244 insertions(+), 33 deletions(-) diff --git a/app/models.py b/app/models.py index 9a062cb..98595f9 100644 --- a/app/models.py +++ b/app/models.py @@ -410,6 +410,11 @@ class RepeaterLoginResponse(BaseModel): """Response from repeater login.""" status: str = Field(description="Login result status") + authenticated: bool = Field(description="Whether repeater authentication was confirmed") + message: str | None = Field( + default=None, + description="Optional warning or error message when authentication was not confirmed", + ) class RepeaterStatusResponse(BaseModel): diff --git a/app/routers/repeaters.py b/app/routers/repeaters.py index 2cbd506..22057a1 100644 --- a/app/routers/repeaters.py +++ b/app/routers/repeaters.py @@ -44,8 +44,21 @@ ACL_PERMISSION_NAMES = { } router = APIRouter(prefix="/contacts", tags=["repeaters"]) -# Delay between repeater radio operations to allow key exchange and path establishment -REPEATER_OP_DELAY_SECONDS = 2.0 +REPEATER_LOGIN_RESPONSE_TIMEOUT_SECONDS = 5.0 +REPEATER_LOGIN_REJECTED_MESSAGE = ( + "The repeater replied but did not confirm this login. " + "Existing access may still allow some repeater operations, but admin actions may fail." +) +REPEATER_LOGIN_SEND_FAILED_MESSAGE = ( + "The login request could not be sent to the repeater. " + "The dashboard is still available, but repeater operations may fail until a login succeeds." +) +REPEATER_LOGIN_TIMEOUT_MESSAGE = ( + "No login confirmation was heard from the repeater. " + "On current repeater firmware, that can mean the password was wrong, " + "blank-password login was not allowed by the ACL, or the reply was missed in transit. " + "The dashboard is still available; try logging in again if admin actions fail." +) def _monotonic() -> float: @@ -136,31 +149,88 @@ async def _fetch_repeater_response( return None -async def prepare_repeater_connection(mc, contact: Contact, password: str) -> None: - """Prepare connection to a repeater by adding to radio and logging in. +async def prepare_repeater_connection(mc, contact: Contact, password: str) -> RepeaterLoginResponse: + """Prepare connection to a repeater by adding to radio and attempting login. Args: mc: MeshCore instance contact: The repeater contact password: Password for login (empty string for no password) - - Raises: - HTTPException: If login fails """ + pubkey_prefix = contact.public_key[:12].lower() + loop = asyncio.get_running_loop() + login_future = loop.create_future() + + def _resolve_login(event_type: EventType, message: str | None = None) -> None: + if login_future.done(): + return + login_future.set_result( + RepeaterLoginResponse( + status="ok" if event_type == EventType.LOGIN_SUCCESS else "error", + authenticated=event_type == EventType.LOGIN_SUCCESS, + message=message, + ) + ) + + success_subscription = mc.subscribe( + EventType.LOGIN_SUCCESS, + lambda _event: _resolve_login(EventType.LOGIN_SUCCESS), + attribute_filters={"pubkey_prefix": pubkey_prefix}, + ) + failed_subscription = mc.subscribe( + EventType.LOGIN_FAILED, + lambda _event: _resolve_login( + EventType.LOGIN_FAILED, + REPEATER_LOGIN_REJECTED_MESSAGE, + ), + attribute_filters={"pubkey_prefix": pubkey_prefix}, + ) + # Add contact to radio with path from DB (non-fatal — contact may already be loaded) - logger.info("Adding repeater %s to radio", contact.public_key[:12]) - await _ensure_on_radio(mc, contact) + try: + logger.info("Adding repeater %s to radio", contact.public_key[:12]) + await _ensure_on_radio(mc, contact) - # Send login with password - logger.info("Sending login to repeater %s", contact.public_key[:12]) - login_result = await mc.commands.send_login(contact.public_key, password) + logger.info("Sending login to repeater %s", contact.public_key[:12]) + login_result = await mc.commands.send_login(contact.public_key, password) - if login_result.type == EventType.ERROR: - raise HTTPException(status_code=401, detail=f"Login failed: {login_result.payload}") + if login_result.type == EventType.ERROR: + return RepeaterLoginResponse( + status="error", + authenticated=False, + message=f"{REPEATER_LOGIN_SEND_FAILED_MESSAGE} ({login_result.payload})", + ) - # Wait for key exchange to complete before sending requests - logger.debug("Waiting %.1fs for key exchange to complete", REPEATER_OP_DELAY_SECONDS) - await asyncio.sleep(REPEATER_OP_DELAY_SECONDS) + try: + return await asyncio.wait_for( + login_future, + timeout=REPEATER_LOGIN_RESPONSE_TIMEOUT_SECONDS, + ) + except asyncio.TimeoutError: + logger.warning( + "No login response from repeater %s within %.1fs", + contact.public_key[:12], + REPEATER_LOGIN_RESPONSE_TIMEOUT_SECONDS, + ) + return RepeaterLoginResponse( + status="timeout", + authenticated=False, + message=REPEATER_LOGIN_TIMEOUT_MESSAGE, + ) + except HTTPException as exc: + logger.warning( + "Repeater login setup failed for %s: %s", + contact.public_key[:12], + exc.detail, + ) + return RepeaterLoginResponse( + status="error", + authenticated=False, + message=f"{REPEATER_LOGIN_SEND_FAILED_MESSAGE} ({exc.detail})", + ) + finally: + success_subscription.unsubscribe() + failed_subscription.unsubscribe() def _require_repeater(contact: Contact) -> None: @@ -180,7 +250,7 @@ def _require_repeater(contact: Contact) -> None: @router.post("/{public_key}/repeater/login", response_model=RepeaterLoginResponse) async def repeater_login(public_key: str, request: RepeaterLoginRequest) -> RepeaterLoginResponse: - """Log in to a repeater. Adds contact to radio, sends login, waits for key exchange.""" + """Attempt repeater login and report whether auth was confirmed.""" require_connected() contact = await _resolve_contact_or_404(public_key) _require_repeater(contact) @@ -190,9 +260,7 @@ async def repeater_login(public_key: str, request: RepeaterLoginRequest) -> Repe pause_polling=True, suspend_auto_fetch=True, ) as mc: - await prepare_repeater_connection(mc, contact, request.password) - - return RepeaterLoginResponse(status="ok") + return await prepare_repeater_connection(mc, contact, request.password) @router.post("/{public_key}/repeater/status", response_model=RepeaterStatusResponse) diff --git a/frontend/src/hooks/useRepeaterDashboard.ts b/frontend/src/hooks/useRepeaterDashboard.ts index d9df745..d8aee58 100644 --- a/frontend/src/hooks/useRepeaterDashboard.ts +++ b/frontend/src/hooks/useRepeaterDashboard.ts @@ -76,6 +76,17 @@ function createInitialPaneData(): PaneData { const repeaterDashboardCache = new Map(); +function getLoginToastTitle(status: string): string { + switch (status) { + case 'timeout': + return 'Login confirmation not heard'; + case 'error': + return 'Login not confirmed'; + default: + return 'Repeater login not confirmed'; + } +} + function clonePaneData(data: PaneData): PaneData { return { ...data }; } @@ -255,13 +266,22 @@ export function useRepeaterDashboard( setLoginLoading(true); setLoginError(null); try { - await api.repeaterLogin(publicKey, password); + const result = await api.repeaterLogin(publicKey, password); if (activeIdRef.current !== conversationId) return; setLoggedIn(true); + if (!result.authenticated) { + const msg = result.message ?? 'Repeater login was not confirmed'; + setLoginError(msg); + toast.error(getLoginToastTitle(result.status), { description: msg }); + } } catch (err) { if (activeIdRef.current !== conversationId) return; const msg = err instanceof Error ? err.message : 'Login failed'; + setLoggedIn(true); setLoginError(msg); + toast.error('Login request failed', { + description: `${msg}. The dashboard is still available, but repeater operations may fail until a login succeeds.`, + }); } finally { if (activeIdRef.current === conversationId) { setLoginLoading(false); diff --git a/frontend/src/test/useRepeaterDashboard.test.ts b/frontend/src/test/useRepeaterDashboard.test.ts index d703cbd..63bfd7b 100644 --- a/frontend/src/test/useRepeaterDashboard.test.ts +++ b/frontend/src/test/useRepeaterDashboard.test.ts @@ -35,6 +35,8 @@ vi.mock('../components/ui/sonner', () => ({ // Get mock reference — cast to Record for type-safe mock method access const { api: _rawApi } = await import('../api'); const mockApi = _rawApi as unknown as Record; +const { toast } = await import('../components/ui/sonner'); +const mockToast = toast as unknown as Record; const REPEATER_KEY = 'aa'.repeat(32); @@ -58,7 +60,11 @@ describe('useRepeaterDashboard', () => { }); it('login sets loggedIn on success', async () => { - mockApi.repeaterLogin.mockResolvedValueOnce({ status: 'ok' }); + mockApi.repeaterLogin.mockResolvedValueOnce({ + status: 'ok', + authenticated: true, + message: null, + }); const { result } = renderHook(() => useRepeaterDashboard(repeaterConversation)); @@ -72,7 +78,11 @@ describe('useRepeaterDashboard', () => { }); it('login sets error on failure', async () => { - mockApi.repeaterLogin.mockRejectedValueOnce(new Error('Auth failed')); + mockApi.repeaterLogin.mockResolvedValueOnce({ + status: 'error', + authenticated: false, + message: 'Auth failed', + }); const { result } = renderHook(() => useRepeaterDashboard(repeaterConversation)); @@ -80,12 +90,19 @@ describe('useRepeaterDashboard', () => { await result.current.login('bad'); }); - expect(result.current.loggedIn).toBe(false); + expect(result.current.loggedIn).toBe(true); expect(result.current.loginError).toBe('Auth failed'); + expect(mockToast.error).toHaveBeenCalledWith('Login not confirmed', { + description: 'Auth failed', + }); }); it('loginAsGuest calls login with empty password', async () => { - mockApi.repeaterLogin.mockResolvedValueOnce({ status: 'ok' }); + mockApi.repeaterLogin.mockResolvedValueOnce({ + status: 'ok', + authenticated: true, + message: null, + }); const { result } = renderHook(() => useRepeaterDashboard(repeaterConversation)); @@ -97,6 +114,23 @@ describe('useRepeaterDashboard', () => { expect(result.current.loggedIn).toBe(true); }); + it('login still opens dashboard when request rejects', async () => { + mockApi.repeaterLogin.mockRejectedValueOnce(new Error('Network error')); + + const { result } = renderHook(() => useRepeaterDashboard(repeaterConversation)); + + await act(async () => { + await result.current.login('secret'); + }); + + expect(result.current.loggedIn).toBe(true); + expect(result.current.loginError).toBe('Network error'); + expect(mockToast.error).toHaveBeenCalledWith('Login request failed', { + description: + 'Network error. The dashboard is still available, but repeater operations may fail until a login succeeds.', + }); + }); + it('refreshPane stores data on success', async () => { const statusData = { battery_volts: 4.2, @@ -376,7 +410,11 @@ describe('useRepeaterDashboard', () => { it('restores dashboard state when navigating away and back to the same repeater', async () => { const statusData = { battery_volts: 4.2 }; - mockApi.repeaterLogin.mockResolvedValueOnce({ status: 'ok' }); + mockApi.repeaterLogin.mockResolvedValueOnce({ + status: 'ok', + authenticated: true, + message: null, + }); mockApi.repeaterStatus.mockResolvedValueOnce(statusData); mockApi.sendRepeaterCommand.mockResolvedValueOnce({ command: 'ver', diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 20fd895..180643f 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -349,6 +349,8 @@ export interface CommandResponse { export interface RepeaterLoginResponse { status: string; + authenticated: boolean; + message: string | null; } export interface RepeaterStatusResponse { diff --git a/tests/test_repeater_routes.py b/tests/test_repeater_routes.py index b33e5c3..571baf6 100644 --- a/tests/test_repeater_routes.py +++ b/tests/test_repeater_routes.py @@ -6,13 +6,14 @@ import pytest from fastapi import HTTPException from meshcore import EventType -from app.models import CommandRequest, Contact, RepeaterLoginRequest +from app.models import CommandRequest, Contact, RepeaterLoginRequest, RepeaterLoginResponse from app.radio import radio_manager from app.repository import ContactRepository from app.routers.contacts import request_trace from app.routers.repeaters import ( _batch_cli_fetch, _fetch_repeater_response, + prepare_repeater_connection, repeater_acl, repeater_advert_intervals, repeater_login, @@ -73,6 +74,7 @@ async def _insert_contact(public_key: str, name: str = "Node", contact_type: int def _mock_mc(): mc = MagicMock() mc.commands = MagicMock() + mc.commands.send_login = AsyncMock(return_value=_radio_result(EventType.MSG_SENT)) mc.commands.req_status_sync = AsyncMock() mc.commands.fetch_all_neighbours = AsyncMock() mc.commands.req_acl_sync = AsyncMock() @@ -82,6 +84,7 @@ def _mock_mc(): mc.commands.add_contact = AsyncMock(return_value=_radio_result(EventType.OK)) mc.commands.send_trace = AsyncMock(return_value=_radio_result(EventType.OK)) mc.wait_for_event = AsyncMock() + mc.subscribe = MagicMock(return_value=MagicMock(unsubscribe=MagicMock())) mc.stop_auto_message_fetching = AsyncMock() mc.start_auto_message_fetching = AsyncMock() return mc @@ -537,9 +540,16 @@ class TestRepeaterLogin: new_callable=AsyncMock, ) as mock_prepare, ): + mock_prepare.return_value = RepeaterLoginResponse( + status="ok", + authenticated=True, + message=None, + ) response = await repeater_login(KEY_A, RepeaterLoginRequest(password="secret")) assert response.status == "ok" + assert response.authenticated is True + assert response.message is None mock_prepare.assert_awaited_once() @pytest.mark.asyncio @@ -567,21 +577,89 @@ class TestRepeaterLogin: assert "not a repeater" in exc.value.detail.lower() @pytest.mark.asyncio - async def test_login_error_raises(self, test_db): + async def test_login_error_returns_warning_response(self, test_db): mc = _mock_mc() await _insert_contact(KEY_A, name="Repeater", contact_type=2) async def _prepare_fail(*args, **kwargs): - raise HTTPException(status_code=401, detail="Login failed") + return RepeaterLoginResponse( + status="error", + authenticated=False, + message="Login failed", + ) with ( patch("app.routers.repeaters.require_connected", return_value=mc), patch.object(radio_manager, "_meshcore", mc), patch("app.routers.repeaters.prepare_repeater_connection", side_effect=_prepare_fail), ): - with pytest.raises(HTTPException) as exc: - await repeater_login(KEY_A, RepeaterLoginRequest(password="bad")) - assert exc.value.status_code == 401 + response = await repeater_login(KEY_A, RepeaterLoginRequest(password="bad")) + assert response.status == "error" + assert response.authenticated is False + assert response.message == "Login failed" + + +class TestPrepareRepeaterConnection: + @pytest.mark.asyncio + async def test_returns_success_when_login_confirmed(self): + mc = _mock_mc() + contact = _make_contact() + subscriptions: dict[EventType, tuple[object, object]] = {} + + def _subscribe(event_type, callback, attribute_filters=None): + subscriptions[event_type] = (callback, attribute_filters) + return MagicMock(unsubscribe=MagicMock()) + + async def _send_login(*args, **kwargs): + callback, filters = subscriptions[EventType.LOGIN_SUCCESS] + assert filters == {"pubkey_prefix": KEY_A[:12]} + callback(_radio_result(EventType.LOGIN_SUCCESS, {"pubkey_prefix": KEY_A[:12]})) + return _radio_result(EventType.MSG_SENT) + + mc.subscribe = MagicMock(side_effect=_subscribe) + mc.commands.send_login = AsyncMock(side_effect=_send_login) + + response = await prepare_repeater_connection(mc, contact, "secret") + + assert response.status == "ok" + assert response.authenticated is True + assert response.message is None + + @pytest.mark.asyncio + async def test_returns_error_when_login_rejected(self): + mc = _mock_mc() + contact = _make_contact() + subscriptions: dict[EventType, tuple[object, object]] = {} + + def _subscribe(event_type, callback, attribute_filters=None): + subscriptions[event_type] = (callback, attribute_filters) + return MagicMock(unsubscribe=MagicMock()) + + async def _send_login(*args, **kwargs): + callback, _filters = subscriptions[EventType.LOGIN_FAILED] + callback(_radio_result(EventType.LOGIN_FAILED, {"pubkey_prefix": KEY_A[:12]})) + return _radio_result(EventType.MSG_SENT) + + mc.subscribe = MagicMock(side_effect=_subscribe) + mc.commands.send_login = AsyncMock(side_effect=_send_login) + + response = await prepare_repeater_connection(mc, contact, "bad") + + assert response.status == "error" + assert response.authenticated is False + assert "did not confirm this login" in (response.message or "") + + @pytest.mark.asyncio + async def test_returns_timeout_when_no_login_response(self): + mc = _mock_mc() + contact = _make_contact() + + with patch("app.routers.repeaters.REPEATER_LOGIN_RESPONSE_TIMEOUT_SECONDS", 0): + response = await prepare_repeater_connection(mc, contact, "secret") + + assert response.status == "timeout" + assert response.authenticated is False + assert "No login confirmation was heard from the repeater" in (response.message or "") class TestRepeaterStatus: