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]