From bbcc0cc4277ce3d74830643a1aec0b854e1079d7 Mon Sep 17 00:00:00 2001 From: HgO Date: Thu, 28 Jul 2022 14:37:23 +0200 Subject: [PATCH] webhook accept room_id param; config define list of allowed rooms ; check if duration is positive in command --- matrix_alertbot/alertmanager.py | 5 +- matrix_alertbot/callback.py | 12 ++-- matrix_alertbot/command.py | 19 +++-- matrix_alertbot/config.py | 4 +- matrix_alertbot/errors.py | 6 -- matrix_alertbot/webhook.py | 32 +++++++-- tests/resources/config/config.full.yml | 3 +- tests/resources/config/config.minimal.yml | 3 +- tests/test_alertmanager.py | 12 ---- tests/test_callback.py | 4 +- tests/test_command.py | 45 +++++++++++- tests/test_config.py | 8 +-- tests/test_webhook.py | 86 ++++++++++++++++++----- 13 files changed, 169 insertions(+), 70 deletions(-) diff --git a/matrix_alertbot/alertmanager.py b/matrix_alertbot/alertmanager.py index 11b22c0..304861a 100644 --- a/matrix_alertbot/alertmanager.py +++ b/matrix_alertbot/alertmanager.py @@ -11,7 +11,6 @@ from diskcache import Cache from matrix_alertbot.errors import ( AlertmanagerServerError, AlertNotFoundError, - InvalidDurationError, SilenceExpiredError, SilenceNotFoundError, ) @@ -74,11 +73,9 @@ class AlertmanagerClient: max_duration = timedelta(days=MAX_DURATION_DAYS) if duration_seconds is None or duration_seconds > max_duration.total_seconds(): end_time = start_time + max_duration - elif duration_seconds > 0: + else: duration_delta = timedelta(seconds=duration_seconds) end_time = start_time + duration_delta - else: - raise InvalidDurationError(f"Duration must be positive: {duration_seconds}") silence = { "id": silence_id, diff --git a/matrix_alertbot/callback.py b/matrix_alertbot/callback.py index e3be031..20dbfc7 100644 --- a/matrix_alertbot/callback.py +++ b/matrix_alertbot/callback.py @@ -61,7 +61,7 @@ class Callbacks: return # Ignore messages from unauthorized room - if room.room_id != self.config.room_id: + if room.room_id not in self.config.allowed_rooms: return # Extract the message text @@ -118,7 +118,7 @@ class Callbacks: event: The invite event. """ # Ignore invites from unauthorized room - if room.room_id != self.config.room_id: + if room.room_id not in self.config.allowed_rooms: return logger.debug(f"Got invite to {room.room_id} from {event.sender}.") @@ -166,7 +166,7 @@ class Callbacks: reacted_to_id: The event ID that the reaction points to. """ # Ignore reactions from unauthorized room - if room.room_id != self.config.room_id: + if room.room_id not in self.config.allowed_rooms: return # Ignore reactions from ourselves @@ -209,7 +209,7 @@ class Callbacks: async def redaction(self, room: MatrixRoom, event: RedactionEvent) -> None: # Ignore events from unauthorized room - if room.room_id != self.config.room_id: + if room.room_id not in self.config.allowed_rooms: return # Ignore redactions from ourselves @@ -239,7 +239,7 @@ class Callbacks: event: The encrypted event that we were unable to decrypt. """ # Ignore events from unauthorized room - if room.room_id != self.config.room_id: + if room.room_id not in self.config.allowed_rooms: return logger.error( @@ -263,7 +263,7 @@ class Callbacks: event: The event itself. """ # Ignore events from unauthorized room - if room.room_id != self.config.room_id: + if room.room_id not in self.config.allowed_rooms: return if event.type == "m.reaction": diff --git a/matrix_alertbot/command.py b/matrix_alertbot/command.py index 2fd3f98..183d573 100644 --- a/matrix_alertbot/command.py +++ b/matrix_alertbot/command.py @@ -11,7 +11,6 @@ from matrix_alertbot.config import Config from matrix_alertbot.errors import ( AlertmanagerError, AlertNotFoundError, - InvalidDurationError, SilenceExpiredError, SilenceNotFoundError, ) @@ -104,8 +103,19 @@ class AckAlertCommand(BaseAlertCommand): f"I tried really hard, but I can't convert the duration '{duration}' to a number of seconds.", ) return + elif duration_seconds < 0: + logger.error(f"Unable to create silence: Duration must be positive, got '{duration}'") + await send_text_to_room( + self.client, + self.room.room_id, + "I can't create a silence with a negative duration!", + ) + return + + cache_expire_time = duration_seconds else: duration_seconds = None + cache_expire_time = self.config.cache_expire_time logger.debug( "Receiving a command to create a silence for an indefinite period" ) @@ -138,7 +148,7 @@ class AckAlertCommand(BaseAlertCommand): duration_seconds, cached_silence_id, ) - except (AlertNotFoundError, InvalidDurationError) as e: + except AlertNotFoundError as e: logger.warning(f"Unable to create silence: {e}") await send_text_to_room( self.client, @@ -146,7 +156,6 @@ class AckAlertCommand(BaseAlertCommand): f"Sorry, I couldn't create silence for alert with fingerprint {alert_fingerprint}: {e}", ) return - return except AlertmanagerError as e: logger.exception(f"Unable to create silence: {e}", exc_info=e) await send_text_to_room( @@ -157,7 +166,7 @@ class AckAlertCommand(BaseAlertCommand): ) return - self.cache.set(self.event_id, alert_fingerprint, expire=duration_seconds) + self.cache.set(self.event_id, alert_fingerprint, expire=cache_expire_time) self.cache.set(alert_fingerprint, silence_id, expire=duration_seconds) await send_text_to_room( @@ -220,6 +229,8 @@ class UnackAlertCommand(BaseAlertCommand): ) return + self.cache.delete(alert_fingerprint) + await send_text_to_room( self.client, self.room.room_id, diff --git a/matrix_alertbot/config.py b/matrix_alertbot/config.py index 9cb170d..9b46062 100644 --- a/matrix_alertbot/config.py +++ b/matrix_alertbot/config.py @@ -102,7 +102,9 @@ class Config: ["matrix", "device_name"], default="matrix-alertbot" ) self.homeserver_url: str = self._get_cfg(["matrix", "url"], required=True) - self.room_id: str = self._get_cfg(["matrix", "room"], required=True) + self.allowed_rooms: list = self._get_cfg( + ["matrix", "allowed_rooms"], required=True + ) self.address: str = self._get_cfg(["webhook", "address"], required=False) self.port: int = self._get_cfg(["webhook", "port"], required=False) diff --git a/matrix_alertbot/errors.py b/matrix_alertbot/errors.py index fc65fdb..2715b18 100644 --- a/matrix_alertbot/errors.py +++ b/matrix_alertbot/errors.py @@ -49,12 +49,6 @@ class SilenceExpiredError(AlertmanagerError): pass -class InvalidDurationError(AlertmanagerError): - """An error encountered when an alert has an invalid duration.""" - - pass - - class AlertmanagerServerError(AlertmanagerError): """An error encountered with Alertmanager server.""" diff --git a/matrix_alertbot/webhook.py b/matrix_alertbot/webhook.py index bc2c99a..4fe11b2 100644 --- a/matrix_alertbot/webhook.py +++ b/matrix_alertbot/webhook.py @@ -23,27 +23,42 @@ async def get_health(request: web_request.Request) -> web.Response: return web.Response(status=200) -@routes.post("/alerts") +@routes.post("/alerts/{room_id}") async def create_alerts(request: web_request.Request) -> web.Response: data = await request.json() - logger.info(f"Received alerts: {data}") + room_id = request.match_info["room_id"] + client: AsyncClient = request.app["client"] config: Config = request.app["config"] cache: Cache = request.app["cache"] + if room_id not in config.allowed_rooms: + logger.error("Cannot send alerts to room ID {room_id}.") + return web.Response(status=401, body=f"Cannot send alerts to room ID {room_id}.") + if "alerts" not in data: + logger.error("Received data without 'alerts' key") return web.Response(status=400, body="Data must contain 'alerts' key.") + alerts = data["alerts"] + if not isinstance(data["alerts"], list): - return web.Response(status=400, body="Alerts must be a list.") + alerts_type = type(alerts).__name__ + logger.error(f"Received data with invalid alerts type '{alerts_type}'.") + return web.Response( + status=400, body=f"Alerts must be a list, got '{alerts_type}'." + ) + + logger.info(f"Received {len(alerts)} alerts for room ID {room_id}: {data}") if len(data["alerts"]) == 0: return web.Response(status=400, body="Alerts cannot be empty.") - for alert in data["alerts"]: + for alert in alerts: try: alert = Alert.from_dict(alert) - except KeyError: + except KeyError as e: + logger.error(f"Cannot parse alert dict: {e}") return web.Response(status=400, body=f"Invalid alert: {alert}.") plaintext = alert.plaintext() @@ -51,7 +66,7 @@ async def create_alerts(request: web_request.Request) -> web.Response: try: event = await send_text_to_room( - client, config.room_id, plaintext, html, notice=False + client, room_id, plaintext, html, notice=False ) except (LocalProtocolError, ClientError) as e: logger.error( @@ -62,7 +77,10 @@ async def create_alerts(request: web_request.Request) -> web.Response: body=f"An error occured when sending alert with fingerprint '{alert.fingerprint}' to Matrix room.", ) - cache.set(event.event_id, alert.fingerprint, expire=config.cache_expire_time) + if alert.firing: + cache.set( + event.event_id, alert.fingerprint, expire=config.cache_expire_time + ) return web.Response(status=200) diff --git a/tests/resources/config/config.full.yml b/tests/resources/config/config.full.yml index eb9eb62..a6e1522 100644 --- a/tests/resources/config/config.full.yml +++ b/tests/resources/config/config.full.yml @@ -20,7 +20,8 @@ matrix: device_id: ABCDEFGHIJ # What to name the logged in device device_name: fake_device_name - room: "!abcdefgh:matrix.example.com" + allowed_rooms: + - "!abcdefgh:matrix.example.com" webhook: socket: matrix-alertbot.socket diff --git a/tests/resources/config/config.minimal.yml b/tests/resources/config/config.minimal.yml index 48f0b50..75ca5d2 100644 --- a/tests/resources/config/config.minimal.yml +++ b/tests/resources/config/config.minimal.yml @@ -15,7 +15,8 @@ matrix: # The device ID that is **non pre-existing** device # If this device ID already exists, messages will be dropped silently in encrypted rooms device_id: ABCDEFGHIJ - room: "!abcdefgh:matrix.example.com" + allowed_rooms: + - "!abcdefgh:matrix.example.com" webhook: address: 0.0.0.0 diff --git a/tests/test_alertmanager.py b/tests/test_alertmanager.py index 90ca767..97427b0 100644 --- a/tests/test_alertmanager.py +++ b/tests/test_alertmanager.py @@ -17,7 +17,6 @@ from matrix_alertbot.alertmanager import AlertmanagerClient from matrix_alertbot.errors import ( AlertmanagerServerError, AlertNotFoundError, - InvalidDurationError, SilenceExpiredError, SilenceNotFoundError, ) @@ -467,17 +466,6 @@ class AlertmanagerClientTestCase(unittest.IsolatedAsyncioTestCase): silence, ) - @freeze_time(datetime.utcfromtimestamp(0)) - async def test_create_silence_raise_duration_error(self) -> None: - async with FakeAlertmanagerServerWithoutSilence() as fake_alertmanager_server: - port = fake_alertmanager_server.port - alertmanager = AlertmanagerClient( - f"http://localhost:{port}", self.fake_cache - ) - async with aiotools.closing_async(alertmanager): - with self.assertRaises(InvalidDurationError): - await alertmanager.create_silence("fingerprint1", "user", -1) - async def test_create_silence_raise_alert_not_found(self) -> None: async with FakeAlertmanagerServerWithoutAlert() as fake_alertmanager_server: port = fake_alertmanager_server.port diff --git a/tests/test_callback.py b/tests/test_callback.py index ae29250..df5544f 100644 --- a/tests/test_callback.py +++ b/tests/test_callback.py @@ -30,7 +30,7 @@ class CallbacksTestCase(unittest.IsolatedAsyncioTestCase): # We don't spec config, as it doesn't currently have well defined attributes self.fake_config = Mock() - self.fake_config.room_id = self.fake_room.room_id + self.fake_config.allowed_rooms = [self.fake_room.room_id] self.fake_config.command_prefix = "!alert " self.callbacks = Callbacks( @@ -151,8 +151,6 @@ class CallbacksTestCase(unittest.IsolatedAsyncioTestCase): fake_message_event = Mock(spec=nio.RoomMessageText) fake_message_event.sender = "@some_other_fake_user:example.com" - self.assertNotEqual(self.fake_config.room_id, self.fake_room.room_id) - # Pretend that we received a text message event await self.callbacks.message(self.fake_room, fake_message_event) diff --git a/tests/test_command.py b/tests/test_command.py index 003bd83..daad2f6 100644 --- a/tests/test_command.py +++ b/tests/test_command.py @@ -103,7 +103,7 @@ class CommandTestCase(unittest.IsolatedAsyncioTestCase): # We don't spec config, as it doesn't currently have well defined attributes self.fake_config = Mock() - self.fake_config.room_id = self.fake_room.room_id + self.fake_config.allowed_rooms = [self.fake_room.room_id] self.fake_config.command_prefix = "!alert " @patch.object(matrix_alertbot.command.AckAlertCommand, "process") @@ -246,7 +246,11 @@ class CommandTestCase(unittest.IsolatedAsyncioTestCase): self.fake_cache.get.assert_called_once_with("fingerprint1") self.fake_cache.set.assert_has_calls( [ - call("some event id", "fingerprint1", expire=None), + call( + "some event id", + "fingerprint1", + expire=self.fake_config.cache_expire_time, + ), call("fingerprint1", "silence1", expire=None), ] ) @@ -408,6 +412,37 @@ class CommandTestCase(unittest.IsolatedAsyncioTestCase): self.fake_cache.get.assert_not_called() self.fake_cache.set.assert_not_called() + @patch.object(matrix_alertbot.command, "send_text_to_room") + async def test_ack_with_negative_duration( + self, fake_send_text_to_room: Mock + ) -> None: + """Tests the callback for InviteMemberEvents""" + # Tests that the bot attempts to join a room after being invited to it + command = AckAlertCommand( + self.fake_client, + self.fake_cache, + self.fake_alertmanager, + self.fake_config, + self.fake_room, + self.fake_sender, + self.fake_event_id, + self.fake_alert_event_id, + ("-1d",), + ) + + await command.process() + + # Check that we attempted to create silences + self.fake_alertmanager.create_silence.assert_not_called() + fake_send_text_to_room.assert_called_once_with( + self.fake_client, + self.fake_room.room_id, + "I can't create a silence with a negative duration!", + ) + self.fake_cache.__getitem__.assert_not_called() + self.fake_cache.get.assert_not_called() + self.fake_cache.set.assert_not_called() + @patch.object(matrix_alertbot.command, "send_text_to_room") async def test_ack_with_alert_event_not_found_in_cache( self, fake_send_text_to_room: Mock @@ -479,7 +514,11 @@ class CommandTestCase(unittest.IsolatedAsyncioTestCase): self.fake_cache.get.assert_called_once_with("fingerprint1") self.fake_cache.set.assert_has_calls( [ - call(self.fake_event_id, "fingerprint1", expire=None), + call( + self.fake_event_id, + "fingerprint1", + expire=self.fake_config.cache_expire_time, + ), call("fingerprint1", "silence1", expire=None), ] ) diff --git a/tests/test_config.py b/tests/test_config.py index cbfc862..08219d0 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -57,7 +57,7 @@ class ConfigTestCase(unittest.TestCase): self.assertEqual("ABCDEFGHIJ", config.device_id) self.assertEqual("matrix-alertbot", config.device_name) self.assertEqual("https://matrix.example.com", config.homeserver_url) - self.assertEqual("!abcdefgh:matrix.example.com", config.room_id) + self.assertEqual(["!abcdefgh:matrix.example.com"], config.allowed_rooms) self.assertEqual("0.0.0.0", config.address) self.assertEqual(8080, config.port) @@ -95,7 +95,7 @@ class ConfigTestCase(unittest.TestCase): self.assertEqual("ABCDEFGHIJ", config.device_id) self.assertEqual("fake_device_name", config.device_name) self.assertEqual("https://matrix.example.com", config.homeserver_url) - self.assertEqual("!abcdefgh:matrix.example.com", config.room_id) + self.assertEqual(["!abcdefgh:matrix.example.com"], config.allowed_rooms) self.assertIsNone(config.address) self.assertIsNone(config.port) @@ -199,7 +199,7 @@ class ConfigTestCase(unittest.TestCase): @patch("os.path.isdir") @patch("os.path.exists") @patch("os.mkdir") - def test_parse_config_with_missing_matrix_room( + def test_parse_config_with_missing_matrix_allowed_rooms( self, fake_mkdir: Mock, fake_path_exists: Mock, fake_path_isdir: Mock ) -> None: fake_path_isdir.return_value = False @@ -207,7 +207,7 @@ class ConfigTestCase(unittest.TestCase): config_path = os.path.join(CONFIG_RESOURCES_DIR, "config.minimal.yml") config = DummyConfig(config_path) - del config.config_dict["matrix"]["room"] + del config.config_dict["matrix"]["allowed_rooms"] with self.assertRaises(RequiredConfigKeyError): config._parse_config_values() diff --git a/tests/test_webhook.py b/tests/test_webhook.py index fc17d3a..5ae75f3 100644 --- a/tests/test_webhook.py +++ b/tests/test_webhook.py @@ -24,11 +24,13 @@ class WebhookApplicationTestCase(aiohttp.test_utils.AioHTTPTestCase): self.fake_client = Mock(spec=nio.AsyncClient) self.fake_cache = Mock(spec=Cache) + self.fake_room_id = "!abcdefg:example.com" + self.fake_config = Mock(spec=Config) self.fake_config.port = aiohttp.test_utils.unused_port() self.fake_config.address = "localhost" self.fake_config.socket = "webhook.sock" - self.fake_config.room_id = "!abcdefg:example.com" + self.fake_config.allowed_rooms = [self.fake_room_id] self.fake_config.cache_expire_time = 0 self.fake_alerts = { @@ -64,13 +66,16 @@ class WebhookApplicationTestCase(aiohttp.test_utils.AioHTTPTestCase): @patch.object(matrix_alertbot.webhook, "send_text_to_room") async def test_post_alerts(self, fake_send_text_to_room: Mock) -> None: data = self.fake_alerts - async with self.client.request("POST", "/alerts", json=data) as response: + async with self.client.request( + "POST", f"/alerts/{self.fake_room_id}", json=data + ) as response: self.assertEqual(200, response.status) + fake_send_text_to_room.assert_has_calls( [ call( self.fake_client, - self.fake_config.room_id, + self.fake_room_id, "[🔥 CRITICAL] alert1: some description1", "[🔥 CRITICAL] " "alert1 (job1)
" @@ -79,7 +84,7 @@ class WebhookApplicationTestCase(aiohttp.test_utils.AioHTTPTestCase): ), call( self.fake_client, - self.fake_config.room_id, + self.fake_room_id, "[🥦 RESOLVED] alert2: some description2", "[🥦 RESOLVED] " "alert2 (job2)
" @@ -88,45 +93,83 @@ class WebhookApplicationTestCase(aiohttp.test_utils.AioHTTPTestCase): ), ] ) + self.fake_cache.set.assert_called_once_with( + fake_send_text_to_room.return_value.event_id, + "fingerprint1", + expire=self.fake_config.cache_expire_time, + ) + + @patch.object(matrix_alertbot.webhook, "send_text_to_room") + async def test_post_alerts_in_unauthorized_room( + self, fake_send_text_to_room: Mock + ) -> None: + room_id = "!unauthorized_room@example.com" + async with self.client.request( + "POST", f"/alerts/{room_id}", json=self.fake_alerts + ) as response: + self.assertEqual(401, response.status) + error_msg = await response.text() + + self.assertEqual( + "Cannot send alerts to room ID !unauthorized_room@example.com.", error_msg + ) + fake_send_text_to_room.assert_not_called() + self.fake_cache.set.assert_not_called() @patch.object(matrix_alertbot.webhook, "send_text_to_room") async def test_post_alerts_with_empty_data( self, fake_send_text_to_room: Mock ) -> None: - async with self.client.request("POST", "/alerts", json={}) as response: + async with self.client.request( + "POST", f"/alerts/{self.fake_room_id}", json={} + ) as response: self.assertEqual(400, response.status) error_msg = await response.text() - self.assertEqual("Data must contain 'alerts' key.", error_msg) + + self.assertEqual("Data must contain 'alerts' key.", error_msg) fake_send_text_to_room.assert_not_called() + self.fake_cache.set.assert_not_called() @patch.object(matrix_alertbot.webhook, "send_text_to_room") async def test_post_empty_alerts(self, fake_send_text_to_room: Mock) -> None: data: Dict = {"alerts": []} - async with self.client.request("POST", "/alerts", json=data) as response: + async with self.client.request( + "POST", f"/alerts/{self.fake_room_id}", json=data + ) as response: self.assertEqual(400, response.status) error_msg = await response.text() - self.assertEqual("Alerts cannot be empty.", error_msg) + + self.assertEqual("Alerts cannot be empty.", error_msg) fake_send_text_to_room.assert_not_called() + self.fake_cache.set.assert_not_called() @patch.object(matrix_alertbot.webhook, "send_text_to_room") async def test_post_invalid_alerts(self, fake_send_text_to_room: Mock) -> None: data = {"alerts": "invalid"} - async with self.client.request("POST", "/alerts", json=data) as response: + async with self.client.request( + "POST", f"/alerts/{self.fake_room_id}", json=data + ) as response: self.assertEqual(400, response.status) error_msg = await response.text() - self.assertEqual("Alerts must be a list.", error_msg) + + self.assertEqual("Alerts must be a list, got 'str'.", error_msg) fake_send_text_to_room.assert_not_called() + self.fake_cache.set.assert_not_called() @patch.object(matrix_alertbot.webhook, "send_text_to_room") async def test_post_alerts_with_empty_items( self, fake_send_text_to_room: Mock ) -> None: data: Dict = {"alerts": [{}]} - async with self.client.request("POST", "/alerts", json=data) as response: + async with self.client.request( + "POST", f"/alerts/{self.fake_room_id}", json=data + ) as response: self.assertEqual(400, response.status) error_msg = await response.text() - self.assertEqual("Invalid alert: {}.", error_msg) + + self.assertEqual("Invalid alert: {}.", error_msg) fake_send_text_to_room.assert_not_called() + self.fake_cache.set.assert_not_called() @patch.object( matrix_alertbot.webhook, @@ -137,19 +180,27 @@ class WebhookApplicationTestCase(aiohttp.test_utils.AioHTTPTestCase): self, fake_send_text_to_room: Mock ) -> None: data = self.fake_alerts - async with self.client.request("POST", "/alerts", json=data) as response: + async with self.client.request( + "POST", f"/alerts/{self.fake_room_id}", json=data + ) as response: self.assertEqual(500, response.status) error_msg = await response.text() - self.assertEqual( - "An error occured when sending alert with fingerprint 'fingerprint1' to Matrix room.", - error_msg, - ) + + self.assertEqual( + "An error occured when sending alert with fingerprint 'fingerprint1' to Matrix room.", + error_msg, + ) fake_send_text_to_room.assert_called_once() + self.fake_cache.set.assert_not_called() async def test_health(self) -> None: async with self.client.request("GET", "/health") as response: self.assertEqual(200, response.status) + async def test_metrics(self) -> None: + async with self.client.request("GET", "/metrics") as response: + self.assertEqual(200, response.status) + class WebhookServerTestCase(unittest.IsolatedAsyncioTestCase): async def asyncSetUp(self) -> None: @@ -160,7 +211,6 @@ class WebhookServerTestCase(unittest.IsolatedAsyncioTestCase): self.fake_config.port = aiohttp.test_utils.unused_port() self.fake_config.address = "localhost" self.fake_config.socket = "webhook.sock" - self.fake_config.room_id = "!abcdefg:example.com" self.fake_config.cache_expire_time = 0 @patch.object(matrix_alertbot.webhook.web, "TCPSite", autospec=True)