From f91d2e950458b2abe041e96a7f9ad67480ba9a65 Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 21 Aug 2026 18:56:23 -0700 Subject: [PATCH] test(acl): drive the wire tests from core's conformance vectors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The end-to-end tests asserted reply[7] == PERM_ACL_ADMIN, which follows the constant wherever it goes — the one thing these tests exist to stop. They now assert the literal bytes from openhop_core's acl_conformance table, so both repos are pinned to the same numbers rather than to each other's vocabulary. Checked out against 2a55dd6 with these tests copied in, all seven fail. Also cover the fail-closed import guard, which was the only untested path protecting against a mixed-version install, plus its happy path so a guard that always fires cannot pass unnoticed. --- tests/test_acl_role_numbering.py | 66 +++++++++++++++++-- tests/test_login_reply_acl_wire.py | 100 +++++++++++++++++------------ 2 files changed, 120 insertions(+), 46 deletions(-) diff --git a/tests/test_acl_role_numbering.py b/tests/test_acl_role_numbering.py index bb0a478..6fb40a9 100644 --- a/tests/test_acl_role_numbering.py +++ b/tests/test_acl_role_numbering.py @@ -13,6 +13,8 @@ from pathlib import Path import pytest import yaml +from openhop_core.protocol.acl_conformance import ROLE_MASK, ROLE_VALUES + from repeater.handler_helpers.acl import ( ACL, PERM_ACL_ADMIN, @@ -41,11 +43,12 @@ class _Id: def test_role_constants_match_firmware_clientacl_h(): - assert PERM_ACL_ROLE_MASK == 3 - assert PERM_ACL_GUEST == 0 - assert PERM_ACL_READ_ONLY == 1 - assert PERM_ACL_READ_WRITE == 2 - assert PERM_ACL_ADMIN == 3 + """Literal pin against the firmware header, via the shared vector table.""" + assert PERM_ACL_ROLE_MASK == ROLE_MASK == 0x03 + assert PERM_ACL_GUEST == ROLE_VALUES["guest"] == 0x00 + assert PERM_ACL_READ_ONLY == ROLE_VALUES["read_only"] == 0x01 + assert PERM_ACL_READ_WRITE == ROLE_VALUES["read_write"] == 0x02 + assert PERM_ACL_ADMIN == ROLE_VALUES["admin"] == 0x03 # Every role must be distinct; GUEST and READ_WRITE used to collide on 0x01. roles = {PERM_ACL_GUEST, PERM_ACL_READ_ONLY, PERM_ACL_READ_WRITE, PERM_ACL_ADMIN} assert len(roles) == 4 @@ -222,3 +225,56 @@ def test_every_role_name_is_allowed_by_the_openapi_schema(): role_name(PERM_ACL_ADMIN), } assert emitted <= set(enum), f"roles missing from the OpenAPI enum: {emitted - set(enum)}" + + +def test_acl_refuses_to_import_against_a_core_without_the_role_constants(monkeypatch): + """The fail-closed guard must actually fire, with a message that helps. + + A core lacking these symbols still builds the login reply's admin byte + from ``permissions & 0x02``, which would announce a room server's + READ_WRITE clients as admins. Refusing to import is the safe failure, and + it is worth a test because nothing else exercises that path. + """ + import importlib + import sys + import types + + import openhop_core.protocol.constants as real_constants + + stub = types.ModuleType("openhop_core.protocol.constants") + for name in dir(real_constants): + if not name.startswith("__"): + setattr(stub, name, getattr(real_constants, name)) + for missing in ( + "PERM_ACL_ROLE_MASK", + "PERM_ACL_GUEST", + "PERM_ACL_READ_ONLY", + "PERM_ACL_READ_WRITE", + "PERM_ACL_ADMIN", + "acl_is_admin", + "acl_role", + ): + delattr(stub, missing) + + monkeypatch.setitem(sys.modules, "openhop_core.protocol.constants", stub) + monkeypatch.delitem(sys.modules, "repeater.handler_helpers.acl") + + with pytest.raises(ImportError) as exc: + importlib.import_module("repeater.handler_helpers.acl") + + message = str(exc.value) + assert "openhop_core is too old" in message + assert "fix/login-perms" in message + + +def test_acl_imports_normally_against_the_installed_core(): + """Companion to the guard test: the happy path must still work. + + Guards that always fire are worse than no guard, and the test above + manipulates sys.modules. + """ + import importlib + + module = importlib.import_module("repeater.handler_helpers.acl") + assert module.PERM_ACL_ADMIN == 0x03 + assert module.is_admin_permissions(0x03) is True diff --git a/tests/test_login_reply_acl_wire.py b/tests/test_login_reply_acl_wire.py index 1ffeb09..e061475 100644 --- a/tests/test_login_reply_acl_wire.py +++ b/tests/test_login_reply_acl_wire.py @@ -23,18 +23,25 @@ from openhop_core.protocol.constants import ( ROUTE_TYPE_FLOOD, ) -from repeater.handler_helpers.acl import ( - ACL, - PERM_ACL_ADMIN, - PERM_ACL_GUEST, - PERM_ACL_READ_WRITE, -) +from openhop_core.protocol.acl_conformance import OUTBOUND + +from repeater.handler_helpers.acl import ACL ROOM_CFG = { "type": "room_server", "settings": {"admin_password": "room-admin", "guest_password": "room-guest"}, } +# How each conformance vector's (server_type, credential) is produced here. +# The ACL is built with these passwords; "" exercises allow_read_only. +CREDENTIALS = { + ("repeater", "admin_password"): ("rpt-admin", None), + ("repeater", "guest_password"): ("rpt-guest", None), + ("repeater", "blank_read_only"): ("", None), + ("room_server", "admin_password"): ("room-admin", ROOM_CFG), + ("room_server", "guest_password"): ("room-guest", ROOM_CFG), +} + def _build_login_packet( client: LocalIdentity, server: LocalIdentity, password: str, *, room: bool = False @@ -101,46 +108,57 @@ async def _login_reply(acl: ACL, password: str, *, room_config: dict = None) -> @pytest.mark.asyncio -async def test_repeater_admin_login_is_admin_on_the_wire(): - """The bug from #388: byte 7 must decode to ADMIN, not READ_WRITE.""" +@pytest.mark.parametrize( + "server_type, credential, admin_code, permissions", + OUTBOUND, + ids=[f"{srv}-{cred}" for srv, cred, _, _ in OUTBOUND], +) +async def test_outbound_conformance_vectors(server_type, credential, admin_code, permissions): + """The real ACL plus the real core handler must emit the literal bytes. + + This is the only place both halves of the fix meet, and the expectations + are literals from openhop_core.protocol.acl_conformance rather than + PERM_ACL_* — a symbolic assertion would follow the constants if they ever + drift away from the mesh again, which is exactly how #388 stayed hidden. + """ + password, room_config = CREDENTIALS[(server_type, credential)] + acl = ACL( + admin_password="rpt-admin", + guest_password="rpt-guest", + allow_read_only=True, + ) + reply = await _login_reply(acl, password, room_config=room_config) + + assert reply[6] == admin_code + assert reply[7] == permissions + + +@pytest.mark.asyncio +async def test_admin_is_role_three_not_the_0x02_bit(): + """The single assertion that would have caught #388. + + A stock client computes (perms & 3) == 3. Our admin used to be 0x02, which + that expression reads as READ_WRITE. + """ acl = ACL(admin_password="rpt-admin", guest_password="rpt-guest") reply = await _login_reply(acl, "rpt-admin") - assert reply[6] == 1 # legacy is_admin flag - assert reply[7] == PERM_ACL_ADMIN # ACL byte modern clients read - assert reply[7] & 0x03 == 3 # what a stock client computes - assert reply[7] != PERM_ACL_READ_WRITE # the value we used to send + assert reply[7] & 0x03 == 3 + assert reply[7] != 0x02 @pytest.mark.asyncio -async def test_repeater_guest_login_is_guest_on_the_wire(): - acl = ACL(admin_password="rpt-admin", guest_password="rpt-guest") - reply = await _login_reply(acl, "rpt-guest") +async def test_repeater_and_room_guests_get_different_roles(): + """The split is observable on the wire, not just in our own vocabulary. - assert reply[6] == 0 - assert reply[7] == PERM_ACL_GUEST + GUEST and READ_WRITE were both 0x01 before, so these two logins were + indistinguishable to every client. + """ + rpt = await _login_reply( + ACL(admin_password="rpt-admin", guest_password="rpt-guest"), "rpt-guest" + ) + room = await _login_reply(ACL(), "room-guest", room_config=ROOM_CFG) - -@pytest.mark.asyncio -async def test_room_server_guest_login_is_read_write_on_the_wire(): - """Room guests post messages, so they must decode as READ_WRITE, not admin.""" - reply = await _login_reply(ACL(), "room-guest", room_config=ROOM_CFG) - - assert reply[6] == 0 # read-write is not admin - assert reply[7] == PERM_ACL_READ_WRITE - - -@pytest.mark.asyncio -async def test_room_server_admin_login_is_admin_on_the_wire(): - reply = await _login_reply(ACL(), "room-admin", room_config=ROOM_CFG) - - assert reply[6] == 1 - assert reply[7] == PERM_ACL_ADMIN - - -@pytest.mark.asyncio -async def test_blank_password_read_only_login_is_guest_on_the_wire(): - reply = await _login_reply(ACL(allow_read_only=True), "") - - assert reply[6] == 0 - assert reply[7] == PERM_ACL_GUEST + assert rpt[7] == 0x00 + assert room[7] == 0x02 + assert rpt[7] != room[7]