diff --git a/repeater/handler_helpers/login.py b/repeater/handler_helpers/login.py index cecc557..feea53d 100644 --- a/repeater/handler_helpers/login.py +++ b/repeater/handler_helpers/login.py @@ -159,7 +159,7 @@ class LoginHelper: self.acls_by_name.get(name), self._live_acl_for_store_key(identity_acl.store_key or ""), }: - if superseded is not None: + if superseded is not None and superseded is not identity_acl: superseded.detach_store() self._drop_acl(superseded) identity_acl.load() @@ -426,6 +426,10 @@ class LoginHelper: from repeater.handler_helpers.acl import acl_identity_label, move_identity_acl label = acl_identity_label(new_name, "room_server") + if self.sqlite_handler is None and old_pubkey_hex.lower() != new_pubkey_hex.lower(): + # A key change with nowhere to move the entries would save a key + # whose access list is gone. + raise RuntimeError("no ACL store is available to move the room's access list") live = self.acls_by_name.get(old_name) if live is None or live.store_key != old_pubkey_hex.lower(): # The name index can lag the config (a hot reload refused), but @@ -434,10 +438,6 @@ class LoginHelper: if live is not None: live.move_store(new_pubkey_hex, label, commit) return - if self.sqlite_handler is None and old_pubkey_hex.lower() != new_pubkey_hex.lower(): - # A key change with nowhere to move the entries would save a key - # whose access list is gone. - raise RuntimeError("no ACL store is available to move the room's access list") move_identity_acl(self.sqlite_handler, old_pubkey_hex, new_pubkey_hex, label, commit) def forget_identity_acl(self, name: str, pubkey_hex: str) -> int: diff --git a/repeater/web/api_endpoints.py b/repeater/web/api_endpoints.py index f9fede9..334aacf 100644 --- a/repeater/web/api_endpoints.py +++ b/repeater/web/api_endpoints.py @@ -6870,8 +6870,15 @@ class APIEndpoints: registration_success = False # Only reload if identity_key was actually provided and not empty, or if name changed needs_reload = data.get("identity_key") or "new_name" in data + # A room with no password is not registered for logins, so a live + # reload would leave its login ACL under the old name; it waits + # for a restart instead. + room_settings = identity.get("settings") or {} + can_log_in = bool( + room_settings.get("admin_password") or room_settings.get("guest_password") + ) - if needs_reload and self.daemon_instance: + if needs_reload and can_log_in and self.daemon_instance: try: from openhop_core import LocalIdentity @@ -6913,16 +6920,20 @@ class APIEndpoints: and hasattr(identity_manager, "unregister_identity") else None ) - registration_success = self.daemon_instance._register_identity_everywhere( - name=final_name, - identity=room_identity, - config=identity, - identity_type="room_server", - ) - if not registration_success and previous is not None: - identity_manager.register_identity( - old_name, previous[0], previous[1], previous[2] + try: + registration_success = ( + self.daemon_instance._register_identity_everywhere( + name=final_name, + identity=room_identity, + config=identity, + identity_type="room_server", + ) ) + finally: + if not registration_success and previous is not None: + identity_manager.register_identity( + old_name, previous[0], previous[1], previous[2] + ) if registration_success: logger.info( f"Hot reload: Re-registered identity '{final_name}' with all systems" @@ -7066,17 +7077,12 @@ class APIEndpoints: if hasattr(self.daemon_instance, "identity_manager"): identity_manager = self.daemon_instance.identity_manager - # Remove from named_identities dict - if name_s in identity_manager.named_identities: - del identity_manager.named_identities[name_s] - logger.info(f"Removed identity {name_s} from named_identities") + # Release its name and hash slot, so a room re-created with + # this name or key registers live rather than after a restart. + if identity_manager.unregister_identity(name_s) is not None: + logger.info(f"Unregistered identity {name_s}") unregister_success = True - # Note: We don't remove from identities dict (keyed by hash) - # because we'd need to look up the hash first, and there could - # be multiple identities with the same hash - # Full cleanup happens on restart - except Exception as unreg_error: logger.error( f"Failed to unregister identity {name_s}: {unreg_error}", exc_info=True @@ -7336,6 +7342,8 @@ class APIEndpoints: login_helper = getattr(daemon, "login_helper", None) if login_helper is None: return [] + # LoginHelper.get_acl_by_name is required: the hash lookup it replaced + # attributes one identity's ACL to another that shares its hash byte. owners = [] if getattr(daemon, "local_identity", None): diff --git a/tests/test_acl_api.py b/tests/test_acl_api.py index 4041244..668ac29 100644 --- a/tests/test_acl_api.py +++ b/tests/test_acl_api.py @@ -439,7 +439,9 @@ class _LiveDaemon: ) self.config = {"identities": {"room_servers": [cfg]}} - def _register_identity_everywhere(self, name, identity, config, identity_type): + def _register_identity_everywhere( + self, name, identity, config, identity_type, previous_name=None + ): if not self.identity_manager.register_identity( name=name, identity=identity, config=config, identity_type=identity_type ): @@ -510,3 +512,45 @@ def test_a_refused_hot_reload_keeps_the_room_registered(db, request_ctx): assert [n for n, *_ in daemon.identity_manager.get_identities_by_type("room_server")] == [ "room-a" ] + + +def test_a_hot_reload_that_raises_keeps_the_room_registered(db, request_ctx): + daemon = _LiveDaemon(db, "room-a", "11" * 32) + api = _api(daemon, daemon.config) + daemon._register_identity_everywhere = MagicMock(side_effect=RuntimeError("boom")) + + request_ctx.method = "PUT" + request_ctx.json = {"name": "room-a", "new_name": "room-b"} + assert "Restart required" in api.update_identity()["message"] + assert [n for n, *_ in daemon.identity_manager.get_identities_by_type("room_server")] == [ + "room-a" + ] + + +def test_a_rename_that_clears_the_passwords_waits_for_a_restart(db, request_ctx): + daemon = _LiveDaemon(db, "room-a", "11" * 32) + api = _api(daemon, daemon.config) + + request_ctx.method = "PUT" + request_ctx.json = { + "name": "room-a", + "new_name": "room-b", + "settings": {"admin_password": "", "guest_password": ""}, + } + assert "Restart required" in api.update_identity()["message"] + # Nothing half-applied: the running room keeps its registration and ACL. + assert [n for n, *_ in daemon.identity_manager.get_identities_by_type("room_server")] == [ + "room-a" + ] + assert daemon.login_helper.get_acl_by_name("room-a") is not None + + +def test_delete_identity_frees_the_name_and_hash(db, request_ctx): + daemon = _LiveDaemon(db, "room-a", "11" * 32) + api = _api(daemon, daemon.config) + + request_ctx.method = "DELETE" + assert api.delete_identity(name="room-a", type="room_server")["success"] is True + identity = LocalIdentity(seed=bytes.fromhex("11" * 32)) + # Re-creating the room registers at once instead of conflicting until a restart. + assert daemon.identity_manager.registration_error("room-a", identity, "room_server") is None