From 5e90ce4e98a107cdf79230ddf2c393c86915c2d4 Mon Sep 17 00:00:00 2001 From: Thorsten Date: Fri, 11 Sep 2026 12:42:33 +0200 Subject: [PATCH] Add runtime plugin toggle: "bot: plugin activate/deactivate " Worker.callback() now skips parse_body() when plugins..enabled is False in persistent_config.ini; the thread and subscription stay up and history-aware plugins keep recording, so reactivation is instant. The help plugin implements the (previously stubbed) activate/deactivate command, restricted to nicks listed in the new "admins" local config key and refusing to deactivate itself. Since the gate runs once per delivered message per worker, Config() now shares one parsed store per process, reloaded only when an ini file's mtime/size changes. get() no longer merge()s local into the runtime store - with a shared store that would have leaked credentials into persistent_config.ini on the next write. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KJp2rEnVtsgbH7FLj3jubx --- deploy/roles/urlbot/tasks/main.yml | 2 + src/distbot/bot/worker.py | 11 +- src/distbot/common/config.py | 121 +++++++++++++----- .../common/config/local_config.ini.spec | 2 + .../common/config/persistent_config.ini.spec | 9 ++ src/distbot/plugins/plugin_help.py | 33 ++++- tests/test_unit/test_config.py | 103 +++++++++++++++ tests/test_unit/test_plugin_toggle.py | 72 +++++++++++ tests/test_unit/test_worker_callback.py | 40 ++++++ 9 files changed, 355 insertions(+), 38 deletions(-) create mode 100644 tests/test_unit/test_config.py create mode 100644 tests/test_unit/test_plugin_toggle.py diff --git a/deploy/roles/urlbot/tasks/main.yml b/deploy/roles/urlbot/tasks/main.yml index 84895fe..d9f79d0 100644 --- a/deploy/roles/urlbot/tasks/main.yml +++ b/deploy/roles/urlbot/tasks/main.yml @@ -56,6 +56,8 @@ value: "{{bot_owner}}" - key: "bot_owner_email" value: "{{bot_owner_email}}" + - key: "admins" + value: "{{ admins | default([]) | join(', ') }}," - key: "amqp_uri" value: "{{ amqp_uri }}" - key: "broker_backend" diff --git a/src/distbot/bot/worker.py b/src/distbot/bot/worker.py index 813f5b3..da5674d 100644 --- a/src/distbot/bot/worker.py +++ b/src/distbot/bot/worker.py @@ -8,7 +8,7 @@ from typing import Optional from distbot.common.action import Action, send_action from distbot.common.broker import SyncBroker, get_sync_broker -from distbot.common.config import conf_get +from distbot.common.config import conf_get, is_plugin_enabled logger = logging.getLogger(__name__) @@ -76,7 +76,11 @@ class Worker(threading.Thread): self.history = self.total_history[body["to"]] action = None try: - action = self.parse_body(body) + if self.is_enabled(): + action = self.parse_body(body) + else: + logger.debug("%s is deactivated, ignoring message", self.get_subclass_name()) + # keep recording history while suspended, so the plugin isn't blind once reactivated if self.uses_history: self.total_history[body["to"]].append(body) assert (action is None) or isinstance(action, Action) @@ -95,6 +99,9 @@ class Worker(threading.Thread): def get_subclass_name(self): return self.__class__.__name__ + def is_enabled(self) -> bool: + return is_plugin_enabled(self.get_subclass_name()) + def die(self): self.broker.stop() diff --git a/src/distbot/common/config.py b/src/distbot/common/config.py index 89e1aeb..e1135e9 100644 --- a/src/distbot/common/config.py +++ b/src/distbot/common/config.py @@ -2,6 +2,7 @@ import logging import os +import threading from configobj import ConfigObj from validate import Validator @@ -17,31 +18,77 @@ def conf_set(key, value): Config().set(key, value) +def plugin_enabled_key(plugin_name: str) -> str: + return "plugins.{}.enabled".format(plugin_name.lower()) + + +def is_plugin_enabled(plugin_name: str) -> bool: + # unknown/unset means enabled; only an explicit False suspends a plugin + return conf_get(plugin_enabled_key(plugin_name)) is not False + + +def set_plugin_enabled(plugin_name: str, enabled: bool): + conf_set(plugin_enabled_key(plugin_name), bool(enabled)) + + CONFIG_SUFFIX = os.environ.get('BOTSUFFIX', '') RESOURCE_PATH = "src/distbot/common/config" +LOCAL_CONFIG_FILE = 'local_config{}.ini'.format(CONFIG_SUFFIX) +PERSISTENT_CONFIG_FILE = 'persistent_config{}.ini'.format(CONFIG_SUFFIX) + +# Parsed+validated stores, shared by all Config() instances in this process and +# reloaded only when one of the ini files changes on disk. conf_get() is called +# from hot paths (e.g. once per delivered message per worker thread), and +# parsing both files every time was the dominant cost. +_cache_lock = threading.RLock() +_cache_stamp = None +_cache_stores = None + + +def _files_stamp(): + stamp = [] + for filename in (LOCAL_CONFIG_FILE, PERSISTENT_CONFIG_FILE): + try: + st = os.stat(filename) + stamp.append((st.st_mtime_ns, st.st_size)) + except FileNotFoundError: + stamp.append(None) + return tuple(stamp) + + +def _invalidate_cache(): + global _cache_stamp, _cache_stores + with _cache_lock: + _cache_stamp = None + _cache_stores = None + + +def _load_stores(): + config_store = ConfigObj( + LOCAL_CONFIG_FILE, + configspec='{}/local_config.ini.spec'.format(RESOURCE_PATH), + encoding='utf-8' + ) + runtime_config_store = ConfigObj( + PERSISTENT_CONFIG_FILE, + configspec='{}/persistent_config.ini.spec'.format(RESOURCE_PATH), + encoding='utf-8' + ) + if not all(c.validate(Validator(), True) for c in (config_store, runtime_config_store)): + logger.error("Config Validation failed") + raise SystemError() + return config_store, runtime_config_store class Config: def __init__(self): - self.config_store = ConfigObj( - 'local_config{}.ini'.format(CONFIG_SUFFIX), - configspec='{}/local_config.ini.spec'.format(RESOURCE_PATH), - encoding='utf-8' - ) - self.runtime_config_store = ConfigObj( - 'persistent_config{}.ini'.format(CONFIG_SUFFIX), - configspec='{}/persistent_config.ini.spec'.format(RESOURCE_PATH), - encoding='utf-8' - ) - if not all(c.validate(Validator(), True) for c in (self.config_store, self.runtime_config_store)): - logger.error("Config Validation failed") - raise SystemError() - else: - # TODO: can't remember the reason for this one - # disabled for now. - # self.config_store.write() - # self.runtime_config_store.write() - pass + global _cache_stamp, _cache_stores + with _cache_lock: + stamp = _files_stamp() + if _cache_stores is None or stamp != _cache_stamp: + _cache_stores = _load_stores() + _cache_stamp = stamp + self.config_store, self.runtime_config_store = _cache_stores def _set(self, cfg, path, value): if len(path) == 1: @@ -49,26 +96,36 @@ class Config: elif not path: raise KeyError() else: + # create intermediate sections on demand (configobj turns dicts into Sections) + if path[0] not in cfg: + cfg[path[0]] = {} self._set(cfg=cfg[path[0]], path=path[1:], value=value) def set(self, key, value): - if '.' in key: - path = key.split('.') - self._set(self.runtime_config_store, path, value) - else: - self.runtime_config_store[key] = value - self.runtime_config_store.write() + with _cache_lock: + if '.' in key: + path = key.split('.') + self._set(self.runtime_config_store, path, value) + else: + self.runtime_config_store[key] = value + self.runtime_config_store.write() + # mtime granularity may not catch a write within the same tick - force a reload + _invalidate_cache() def get(self, key): - if '.' in key: - path = key.split('.') - value = self.get(path.pop(0)) - for p in path: + path = key.split('.') + with _cache_lock: + # local_config.ini takes precedence over persistent_config.ini (top-level keys only, + # the two specs don't overlap). Deliberately no merge() into the shared runtime + # store: that would leak static config (credentials!) into persistent_config.ini + # on the next write. + if path[0] in self.config_store: + value = self.config_store.get(path[0]) + else: + value = self.runtime_config_store.get(path[0]) + for p in path[1:]: value = value.get(p) # die early, if run into a dead end if value is None: break - else: - self.runtime_config_store.merge(self.config_store) - value = self.runtime_config_store.get(key) return value diff --git a/src/distbot/common/config/local_config.ini.spec b/src/distbot/common/config/local_config.ini.spec index a638ea0..d13babf 100644 --- a/src/distbot/common/config/local_config.ini.spec +++ b/src/distbot/common/config/local_config.ini.spec @@ -6,6 +6,8 @@ src-url = string bot_nickname = string bot_owner = string +# nicks allowed to activate/deactivate plugins at runtime +admins = string_list(default=list()) bot_owner_email = string detectlanguage_api_key = string diff --git a/src/distbot/common/config/persistent_config.ini.spec b/src/distbot/common/config/persistent_config.ini.spec index fb03d56..31af510 100644 --- a/src/distbot/common/config/persistent_config.ini.spec +++ b/src/distbot/common/config/persistent_config.ini.spec @@ -4,23 +4,32 @@ request_counter = integer(default=0) start_time = integer(default=0) [plugins] + # every plugin section (keyed by lowercased class name) can be suspended at + # runtime via "bot: plugin deactivate ", see Worker.callback() + [[__many__]] + enabled = boolean(default=true) [[info]] enabled = boolean(default=true) [[dsa-watcher]] + enabled = boolean(default=true) interval = integer(default=900) active = boolean(default=False) last_dsa = int() last_dsa_date = float() [[dice]] + enabled = boolean(default=true) # the "dice" feature will use more efficient random data (0) for given users enhanced-random-user = string_list(default=list()) [[moin]] + enabled = boolean(default=true) # the "moin" feature will be "disabled" for given users moin-modified-user = string_list(default=list()) moin-disabled-user = string_list(default=list()) [[teatimer]] + enabled = boolean(default=true) steep_time = integer(default=220) [[urlresolve]] + enabled = boolean(default=true) [[[blacklist]]] heise = string(default='^.*heise\.de/.*-[0-9]+\.html$') diff --git a/src/distbot/plugins/plugin_help.py b/src/distbot/plugins/plugin_help.py index 64ed45c..ec18bae 100644 --- a/src/distbot/plugins/plugin_help.py +++ b/src/distbot/plugins/plugin_help.py @@ -4,6 +4,8 @@ import logging from distbot.bot.worker import Worker from distbot.common.action import Action +from distbot.common.config import conf_get, is_plugin_enabled, set_plugin_enabled +from distbot.common.message import get_nick_from_message logger = logging.getLogger(__name__) @@ -44,6 +46,30 @@ class Plugins(Worker): if key.startswith("nick"): self.reverse_lookup[key.split('.')[1]] = plugin + def resolve_plugin_name(self, name: str) -> str | None: + """Map a class name or one of its command words to the registered plugin name (lowercased).""" + name = name.lower() + if name in self.plugin_storage: + return name + self.build_reverse_lookup() + return self.reverse_lookup.get(name) + + def set_plugin_state(self, msg, plugin_name: str, enabled: bool) -> Action: + nick = get_nick_from_message(msg) + if nick not in (conf_get("admins") or []): + return Action(msg="{}: you are not allowed to do that".format(nick)) + + resolved = self.resolve_plugin_name(plugin_name) + if not resolved: + return Action(msg="unknown plugin {}, try help".format(plugin_name)) + if resolved == self.get_subclass_name().lower() and not enabled: + return Action(msg="nice try, but then nobody could reactivate anything") + + if is_plugin_enabled(resolved) == enabled: + return Action(msg="{} is already {}".format(resolved, "active" if enabled else "deactivated")) + set_plugin_enabled(resolved, enabled) + return Action(msg="{} {}".format(resolved, "activated" if enabled else "deactivated")) + def send_help(self, plugin_name=None): if not plugin_name: msg = "Known commands/reactions:\n" + ", ".join(self.plugin_storage.keys()) @@ -72,12 +98,11 @@ class Plugins(Worker): plugin_name = bind[2] return self.send_help(plugin_name) elif bind[1] == "plugin": - - # TODO find a good way to disable plugins (completely OR just suspended) + # suspends the plugin: its thread keeps running, Worker.callback() just skips parse_body() if bind[2] == "activate": - pass + return self.set_plugin_state(msg, bind[3], enabled=True) elif bind[2] == "deactivate": - pass + return self.set_plugin_state(msg, bind[3], enabled=False) ALL = [Plugins] diff --git a/tests/test_unit/test_config.py b/tests/test_unit/test_config.py new file mode 100644 index 0000000..b756d57 --- /dev/null +++ b/tests/test_unit/test_config.py @@ -0,0 +1,103 @@ +# -*- coding: utf-8 -*- +from distbot.common.config import Config +import os + +import pytest + + +def test_config_read_write(): + path = "plugins.dsa-watcher.interval" + c = Config() + original = c.get(path) + try: + c.set(path, original + 1) + assert c.get(path) == original + 1 + finally: + c.set(path, original) + + +def test_plugin_enabled_defaults_to_true_for_unknown_plugin(): + from distbot.common.config import is_plugin_enabled + assert is_plugin_enabled("NoSuchPluginEver") is True + + +def test_plugin_enabled_round_trip(): + from distbot.common.config import is_plugin_enabled, set_plugin_enabled, plugin_enabled_key + name = "TestPluginRoundTrip" + c = Config() + try: + set_plugin_enabled(name, False) + # a fresh Config() re-reads the ini, so this is what another thread/process sees + assert is_plugin_enabled(name) is False + assert Config().get(plugin_enabled_key(name)) is False + set_plugin_enabled(name, True) + assert is_plugin_enabled(name) is True + finally: + section = c.runtime_config_store["plugins"] + section.pop(name.lower(), None) + c.runtime_config_store.write() + + +@pytest.fixture() +def tmp_config(tmp_path, monkeypatch): + """Point Config at private ini copies so cache tests can rewrite them freely.""" + import distbot.common.config as cfg_module + local = tmp_path / "local_config.ini" + persistent = tmp_path / "persistent_config.ini" + local.write_text("jid = bot@example.org\npassword = hunter2\nbot_nickname = testbot\n") + persistent.write_text("request_counter = 1\n") + monkeypatch.setattr(cfg_module, "LOCAL_CONFIG_FILE", str(local)) + monkeypatch.setattr(cfg_module, "PERSISTENT_CONFIG_FILE", str(persistent)) + cfg_module._invalidate_cache() + yield local, persistent + cfg_module._invalidate_cache() + + +def test_config_is_cached_between_instances(tmp_config): + import distbot.common.config as cfg_module + loads = [] + real_load = cfg_module._load_stores + cfg_module._load_stores = lambda: loads.append(1) or real_load() + try: + assert Config().get("bot_nickname") == "testbot" + assert Config().get("request_counter") == 1 + assert len(loads) == 1 + finally: + cfg_module._load_stores = real_load + + +def test_config_reloads_when_file_changes_on_disk(tmp_config): + local, persistent = tmp_config + assert Config().get("request_counter") == 1 + + # simulate another process writing the file, with an mtime guaranteed to differ + persistent.write_text("request_counter = 7\n") + st = os.stat(persistent) + os.utime(persistent, ns=(st.st_atime_ns, st.st_mtime_ns + 1_000_000)) + + assert Config().get("request_counter") == 7 + + +def test_config_set_is_visible_to_next_instance(tmp_config): + Config().set("request_counter", 5) + assert Config().get("request_counter") == 5 + + +def test_get_does_not_leak_local_config_into_persistent_file(tmp_config): + local, persistent = tmp_config + c = Config() + assert c.get("password") == "hunter2" + c.set("request_counter", 2) + + written = persistent.read_text() + assert "hunter2" not in written + assert "jid" not in written + assert "request_counter = 2" in written + + +def test_local_config_takes_precedence_over_persistent(tmp_config): + local, persistent = tmp_config + persistent.write_text("request_counter = 1\nbot_nickname = wrong\n") + import distbot.common.config as cfg_module + cfg_module._invalidate_cache() + assert Config().get("bot_nickname") == "testbot" diff --git a/tests/test_unit/test_plugin_toggle.py b/tests/test_unit/test_plugin_toggle.py new file mode 100644 index 0000000..ed3c544 --- /dev/null +++ b/tests/test_unit/test_plugin_toggle.py @@ -0,0 +1,72 @@ +# -*- coding: utf-8 -*- +import pytest + +import distbot.plugins.plugin_help as plugin_help +from distbot.common.action import Action +from distbot.plugins.plugin_help import Plugins + +ADMIN = "room@conf/Admin" +USER = "room@conf/Someone" + + +@pytest.fixture() +def state(monkeypatch): + """In-memory replacement for the persistent config plumbing.""" + store = {} + monkeypatch.setattr(plugin_help, "conf_get", lambda key: ["Admin"] if key == "admins" else None) + monkeypatch.setattr(plugin_help, "is_plugin_enabled", lambda name: store.get(name.lower(), True)) + monkeypatch.setattr(plugin_help, "set_plugin_enabled", lambda name, enabled: store.__setitem__(name.lower(), enabled)) + return store + + +@pytest.fixture() +def helper(state): + p = Plugins("_") + p.plugin_storage = { + "dice": {"name": "Dice", "bindings": ["nick.dice.#", "nick.würfle.#"], "description": "", "usage": ""}, + "plugins": {"name": "Plugins", "bindings": ["nick.help", "nick.plugin.activate.*"], "description": "", "usage": ""}, + } + return p + + +def _run(p, words, sender=ADMIN): + p.used_routing_key = words + return p.parse_body({"from": sender, "to": "room@conf", "body": " ".join(words)}) + + +def test_deactivate_by_class_name(helper, state): + assert _run(helper, ["nick", "plugin", "deactivate", "dice"]) == Action(msg="dice deactivated") + assert state == {"dice": False} + + +def test_deactivate_by_command_word(helper, state): + assert _run(helper, ["nick", "plugin", "deactivate", "würfle"]) == Action(msg="dice deactivated") + assert state == {"dice": False} + + +def test_activate_again(helper, state): + state["dice"] = False + assert _run(helper, ["nick", "plugin", "activate", "dice"]) == Action(msg="dice activated") + assert state == {"dice": True} + + +def test_already_in_requested_state(helper, state): + assert _run(helper, ["nick", "plugin", "activate", "dice"]) == Action(msg="dice is already active") + assert state == {} + + +def test_unknown_plugin(helper, state): + assert _run(helper, ["nick", "plugin", "deactivate", "nope"]) == Action(msg="unknown plugin nope, try help") + assert state == {} + + +def test_non_admin_is_rejected(helper, state): + assert _run(helper, ["nick", "plugin", "deactivate", "dice"], sender=USER) == \ + Action(msg="Someone: you are not allowed to do that") + assert state == {} + + +def test_cannot_deactivate_the_toggle_plugin_itself(helper, state): + result = _run(helper, ["nick", "plugin", "deactivate", "plugins"]) + assert result == Action(msg="nice try, but then nobody could reactivate anything") + assert state == {} diff --git a/tests/test_unit/test_worker_callback.py b/tests/test_unit/test_worker_callback.py index a4f743b..3b1c1ca 100644 --- a/tests/test_unit/test_worker_callback.py +++ b/tests/test_unit/test_worker_callback.py @@ -113,3 +113,43 @@ def test_callback_acks_even_if_parse_body_raises(w): w.channel.basic_publish.assert_not_called() ch.basic_ack.assert_called_once_with(delivery_tag=42) + + +@pytest.fixture() +def disabled(monkeypatch): + monkeypatch.setattr(worker_module, 'is_plugin_enabled', lambda name: False) + + +def test_callback_skips_parse_body_when_disabled(w, send_action, disabled): + w.parse_body = Mock(return_value=Action(msg='hello')) + ch, method, properties, body = _make_pika_args() + + w.callback(ch, method, properties, body) + + w.parse_body.assert_not_called() + send_action.assert_not_called() + ch.basic_ack.assert_called_once_with(delivery_tag=42) + + +def test_callback_runs_parse_body_when_enabled(w, send_action, monkeypatch): + seen = [] + monkeypatch.setattr(worker_module, 'is_plugin_enabled', lambda name: seen.append(name) or True) + w.returned_action = Action(msg='hello') + ch, method, properties, body = _make_pika_args() + + w.callback(ch, method, properties, body) + + assert seen == ['_TestWorker'] + send_action.assert_called_once() + + +def test_callback_keeps_recording_history_while_disabled(send_action, disabled): + class _HistoryWorker(_TestWorker): + uses_history = True + + w = _HistoryWorker(actionqueue='test_actions') + ch, method, properties, body = _make_pika_args(body_to='room@conf', body_text='remember me') + + w.callback(ch, method, properties, body) + + assert [m['body'] for m in w.total_history['room@conf']] == ['remember me'] -- 2.47.3