Allow discovery messages for unknown services with a warning (#4449)

* Allow discovery messages for unknown services with a warning

* Log at warning level and skip sentry report

supervisor/addons/validate.py

@@ -175,6 +175,20 @@ def _warn_addon_config(config: dict[str, Any]):
             name,
         )
 
+    invalid_services: list[str] = []
+    for service in config.get(ATTR_DISCOVERY, []):
+        try:
+            valid_discovery_service(service)
+        except vol.Invalid:
+            invalid_services.append(service)
+
+    if invalid_services:
+        _LOGGER.warning(
+            "Add-on lists the following unknown services for discovery: %s. Please report this to the maintainer of %s",
+            ", ".join(invalid_services),
+            name,
+        )
+
     return config
 
 
@@ -313,7 +327,7 @@ _SCHEMA_ADDON_CONFIG = vol.Schema(
         vol.Optional(ATTR_DOCKER_API, default=False): vol.Boolean(),
         vol.Optional(ATTR_AUTH_API, default=False): vol.Boolean(),
         vol.Optional(ATTR_SERVICES): [vol.Match(RE_SERVICE)],
-        vol.Optional(ATTR_DISCOVERY): [valid_discovery_service],
+        vol.Optional(ATTR_DISCOVERY): [str],
         vol.Optional(ATTR_BACKUP_EXCLUDE): [str],
         vol.Optional(ATTR_BACKUP_PRE): str,
         vol.Optional(ATTR_BACKUP_POST): str,

supervisor/api/discovery.py

@@ -1,6 +1,9 @@
 """Init file for Supervisor network RESTful API."""
+import logging
+
 import voluptuous as vol
 
+from ..addons.addon import Addon
 from ..const import (
     ATTR_ADDON,
     ATTR_CONFIG,
@@ -15,9 +18,11 @@ from ..discovery.validate import valid_discovery_service
 from ..exceptions import APIError, APIForbidden
 from .utils import api_process, api_validate, require_home_assistant
 
+_LOGGER: logging.Logger = logging.getLogger(__name__)
+
 SCHEMA_DISCOVERY = vol.Schema(
     {
-        vol.Required(ATTR_SERVICE): valid_discovery_service,
+        vol.Required(ATTR_SERVICE): str,
         vol.Optional(ATTR_CONFIG): vol.Maybe(dict),
     }
 )
@@ -62,11 +67,28 @@ class APIDiscovery(CoreSysAttributes):
     async def set_discovery(self, request):
         """Write data into a discovery pipeline."""
         body = await api_validate(SCHEMA_DISCOVERY, request)
-        addon = request[REQUEST_FROM]
+        addon: Addon = request[REQUEST_FROM]
+        service = body[ATTR_SERVICE]
+
+        try:
+            valid_discovery_service(service)
+        except vol.Invalid:
+            _LOGGER.warning(
+                "Received discovery message for unknown service %s from addon %s. Please report this to the maintainer of the add-on",
+                service,
+                addon.name,
+            )
 
         # Access?
         if body[ATTR_SERVICE] not in addon.discovery:
-            raise APIForbidden("Can't use discovery!")
+            _LOGGER.error(
+                "Add-on %s attempted to send discovery for service %s which is not listed in its config. Please report this to the maintainer of the add-on",
+                addon.name,
+                service,
+            )
+            raise APIForbidden(
+                "Add-ons must list services they provide via discovery in their config!"
+            )
 
         # Process discovery message
         message = self.sys_discovery.send(addon, **body)

supervisor/discovery/validate.py

@@ -33,7 +33,7 @@ SCHEMA_DISCOVERY = vol.Schema(
             {
                 vol.Required(ATTR_UUID): uuid_match,
                 vol.Required(ATTR_ADDON): str,
-                vol.Required(ATTR_SERVICE): valid_discovery_service,
+                vol.Required(ATTR_SERVICE): str,
                 vol.Required(ATTR_CONFIG): vol.Maybe(dict),
             },
             extra=vol.REMOVE_EXTRA,

supervisor/utils/sentry.py

@@ -1,6 +1,7 @@
 """Utilities for sentry."""
 
 import logging
+from typing import Any
 
 import sentry_sdk
 from sentry_sdk.integrations.aiohttp import AioHttpIntegration
@@ -16,6 +17,8 @@ from ..misc.filter import filter_data
 
 _LOGGER: logging.Logger = logging.getLogger(__name__)
 
+only_once_events: set[str] = set()
+
 
 def sentry_connected() -> bool:
     """Is sentry connected."""
@@ -44,6 +47,14 @@ def init_sentry(coresys: CoreSys) -> None:
         )
 
 
+def capture_event(event: dict[str, Any], only_once: str | None = None):
+    """Capture an event and send to sentry."""
+    if sentry_connected():
+        if only_once and only_once not in only_once_events:
+            only_once_events.add(only_once)
+            sentry_sdk.capture_event(event)
+
+
 def capture_exception(err: Exception) -> None:
     """Capture an exception and send to sentry."""
     if sentry_connected():

tests/addons/test_config.py

@@ -1,5 +1,8 @@
 """Validate Add-on configs."""
 
+import logging
+from unittest.mock import Mock
+
 import pytest
 import voluptuous as vol
 
@@ -283,4 +286,13 @@ def test_valid_slug():
     with pytest.raises(vol.Invalid):
         assert vd.SCHEMA_ADDON_CONFIG(config)
 
-    #
+
+def test_invalid_discovery(capture_event: Mock, caplog: pytest.LogCaptureFixture):
+    """Test invalid discovery."""
+    config = load_json_fixture("basic-addon-config.json")
+    config["discovery"] = ["mqtt", "junk", "junk2"]
+
+    assert vd.SCHEMA_ADDON_CONFIG(config)
+
+    with caplog.at_level(logging.WARNING):
+        assert "unknown services for discovery: junk, junk2" in caplog.text

tests/api/test_discovery.py

@@ -0,0 +1,53 @@
+"""Test discovery API."""
+
+import logging
+from unittest.mock import MagicMock, patch
+from uuid import uuid4
+
+from aiohttp.test_utils import TestClient
+import pytest
+
+from supervisor.addons.addon import Addon
+from supervisor.discovery import Discovery
+
+
+@pytest.mark.parametrize("api_client", ["local_ssh"], indirect=True)
+async def test_discovery_forbidden(
+    api_client: TestClient, caplog: pytest.LogCaptureFixture, install_addon_ssh
+):
+    """Test addon sending discovery message for an unregistered service."""
+    caplog.clear()
+
+    with caplog.at_level(logging.ERROR):
+        resp = await api_client.post("/discovery", json={"service": "mqtt"})
+
+    assert resp.status == 400
+    result = await resp.json()
+    assert result["result"] == "error"
+    assert (
+        result["message"]
+        == "Add-ons must list services they provide via discovery in their config!"
+    )
+    assert "Please report this to the maintainer of the add-on" in caplog.text
+
+
+@pytest.mark.parametrize("api_client", ["local_ssh"], indirect=True)
+async def test_discovery_unknown_service(
+    api_client: TestClient, caplog: pytest.LogCaptureFixture, install_addon_ssh: Addon
+):
+    """Test addon sending discovery message for an unkown service."""
+    caplog.clear()
+    install_addon_ssh.data["discovery"] = ["junk"]
+
+    message = MagicMock()
+    message.uuid = uuid4().hex
+
+    with caplog.at_level(logging.WARNING), patch.object(
+        Discovery, "send", return_value=message
+    ):
+        resp = await api_client.post("/discovery", json={"service": "junk"})
+
+    assert resp.status == 200
+    result = await resp.json()
+    assert result["data"]["uuid"] == message.uuid
+    assert "Please report this to the maintainer of the add-on" in caplog.text

tests/conftest.py

@@ -408,13 +408,21 @@ def sys_supervisor():
 
 
 @pytest.fixture
-async def api_client(aiohttp_client, coresys: CoreSys) -> TestClient:
+async def api_client(
+    aiohttp_client, coresys: CoreSys, request: pytest.FixtureRequest
+) -> TestClient:
     """Fixture for RestAPI client."""
 
+    request_from: str | None = getattr(request, "param", None)
+
     @web.middleware
     async def _security_middleware(request: web.Request, handler: web.RequestHandler):
-        """Make request are from Core."""
-        request[REQUEST_FROM] = coresys.homeassistant
+        """Make request are from Core or specified add-on."""
+        if request_from:
+            request[REQUEST_FROM] = coresys.addons.get(request_from, local_only=True)
+        else:
+            request[REQUEST_FROM] = coresys.homeassistant
+
         return await handler(request)
 
     api = RestAPI(coresys)
@@ -606,6 +614,15 @@ async def capture_exception() -> Mock:
         yield capture_exception
 
 
+@pytest.fixture
+async def capture_event() -> Mock:
+    """Mock capture event for testing."""
+    with patch("supervisor.utils.sentry.sentry_connected", return_value=True), patch(
+        "supervisor.utils.sentry.sentry_sdk.capture_event"
+    ) as capture_event:
+        yield capture_event
+
+
 @pytest.fixture
 async def os_available(request: pytest.FixtureRequest) -> None:
     """Mock os as available."""