diff --git a/homeassistant/components/color_extractor/services.py b/homeassistant/components/color_extractor/services.py index 273a54107f4d..1ca654336680 100644 --- a/homeassistant/components/color_extractor/services.py +++ b/homeassistant/components/color_extractor/services.py @@ -1,6 +1,7 @@ """Module for color_extractor (RGB extraction from images) component.""" import asyncio +from http import HTTPStatus import io import logging from typing import Any @@ -17,7 +18,7 @@ from homeassistant.components.light import ( ) from homeassistant.const import SERVICE_TURN_ON from homeassistant.core import HomeAssistant, ServiceCall, SupportsResponse, callback -from homeassistant.exceptions import ServiceValidationError +from homeassistant.exceptions import HomeAssistantError, ServiceValidationError from homeassistant.helpers import aiohttp_client, config_validation as cv from .const import ATTR_PATH, ATTR_URL, DOMAIN, SERVICE_GET_COLOR @@ -67,17 +68,14 @@ def _get_color(file_handler: io.BytesIO | str) -> tuple[int, int, int]: async def _async_extract_color_from_url( hass: HomeAssistant, url: str -) -> tuple[int, int, int] | None: +) -> tuple[int, int, int]: """Handle call for URL based image.""" if not hass.config.is_allowed_external_url(url): - _LOGGER.error( - ( - "External URL '%s' is not allowed, please add to" - " 'allowlist_external_urls'" - ), - url, + raise ServiceValidationError( + translation_domain=DOMAIN, + translation_key="url_not_allowed", + translation_placeholders={"url": url}, ) - return None _LOGGER.debug("Getting predominant RGB from image URL '%s'", url) @@ -85,14 +83,30 @@ async def _async_extract_color_from_url( try: session = aiohttp_client.async_get_clientsession(hass) - async with asyncio.timeout(10): - response = await session.get(url) + async with asyncio.timeout(10), session.get(url) as response: + if response.status != HTTPStatus.OK: + raise HomeAssistantError( + translation_domain=DOMAIN, + translation_key="http_error", + translation_placeholders={ + "url": url, + "status": str(response.status), + }, + ) + content = await response.read() - except (TimeoutError, aiohttp.ClientError) as err: - _LOGGER.error("Failed to get ColorThief image due to HTTPError: %s", err) - return None - - content = await response.content.read() + except TimeoutError as err: + raise HomeAssistantError( + translation_domain=DOMAIN, + translation_key="timeout", + translation_placeholders={"url": url}, + ) from err + except aiohttp.ClientError as err: + raise HomeAssistantError( + translation_domain=DOMAIN, + translation_key="fetch_failed", + translation_placeholders={"url": url, "error": str(err)}, + ) from err with io.BytesIO(content) as _file: _file.name = "color_extractor.jpg" @@ -103,14 +117,14 @@ async def _async_extract_color_from_url( def _extract_color_from_path( hass: HomeAssistant, file_path: str -) -> tuple[int, int, int] | None: +) -> tuple[int, int, int]: """Handle call for local file based image.""" if not hass.config.is_allowed_path(file_path): - _LOGGER.error( - "File path '%s' is not allowed, please add to 'allowlist_external_dirs'", - file_path, + raise ServiceValidationError( + translation_domain=DOMAIN, + translation_key="path_not_allowed", + translation_placeholders={"file_path": file_path}, ) - return None _LOGGER.debug("Getting predominant RGB from file path '%s'", file_path) @@ -137,22 +151,21 @@ async def async_handle_service(service_call: ServiceCall) -> None: _extract_color_from_path, service_call.hass, image_reference ) - # pylint: disable-next=home-assistant-action-swallowed-exception except UnidentifiedImageError as ex: - _LOGGER.error( - "Bad image from %s '%s' provided, are you sure it's an image? %s", - image_type, - image_reference, - ex, - ) - return + raise ServiceValidationError( + translation_domain=DOMAIN, + translation_key="invalid_image", + translation_placeholders={ + "image_type": image_type, + "image_reference": image_reference, + }, + ) from ex - if color: - service_data[ATTR_RGB_COLOR] = color + service_data[ATTR_RGB_COLOR] = color - await service_call.hass.services.async_call( - LIGHT_DOMAIN, SERVICE_TURN_ON, service_data, blocking=True - ) + await service_call.hass.services.async_call( + LIGHT_DOMAIN, SERVICE_TURN_ON, service_data, blocking=True + ) async def async_handle_get_color( @@ -186,16 +199,6 @@ async def async_handle_get_color( }, ) from ex - if color is None: - raise ServiceValidationError( - translation_domain=DOMAIN, - translation_key="invalid_image", - translation_placeholders={ - "image_type": image_type, - "image_reference": image_reference, - }, - ) - return {"color": color} diff --git a/homeassistant/components/color_extractor/strings.json b/homeassistant/components/color_extractor/strings.json index 80dbd07b77b0..8cb136cae1d2 100644 --- a/homeassistant/components/color_extractor/strings.json +++ b/homeassistant/components/color_extractor/strings.json @@ -7,8 +7,23 @@ } }, "exceptions": { + "fetch_failed": { + "message": "Failed to fetch the image from {url}: {error}" + }, + "http_error": { + "message": "Failed to fetch the image from {url}: the server responded with HTTP status {status}." + }, "invalid_image": { "message": "Bad image {image_reference} from {image_type} provided, are you sure it's an image?" + }, + "path_not_allowed": { + "message": "Path {file_path} is not allowed, add it to allowlist_external_dirs." + }, + "timeout": { + "message": "Timed out fetching the image from {url}." + }, + "url_not_allowed": { + "message": "URL {url} is not allowed, add it to allowlist_external_urls." } }, "services": { diff --git a/tests/components/color_extractor/test_services.py b/tests/components/color_extractor/test_services.py index eaf92b2bc7d9..2df6d71cab90 100644 --- a/tests/components/color_extractor/test_services.py +++ b/tests/components/color_extractor/test_services.py @@ -26,7 +26,7 @@ from homeassistant.components.light import ( ) from homeassistant.const import ATTR_ENTITY_ID, STATE_OFF, STATE_ON from homeassistant.core import HomeAssistant -from homeassistant.exceptions import ServiceValidationError +from homeassistant.exceptions import HomeAssistantError, ServiceValidationError from homeassistant.setup import async_setup_component from homeassistant.util import color as color_util @@ -175,13 +175,22 @@ async def test_url_success( async def test_url_not_allowed( hass: HomeAssistant, aioclient_mock: AiohttpClientMocker, setup_integration ) -> None: - """Test that a not allowed external URL fails to turn light on.""" + """Test that a not allowed external URL raises and doesn't turn light on.""" service_data = { ATTR_URL: "http://denied.com/images/logo.png", ATTR_ENTITY_ID: LIGHT_ENTITY, } - await _async_execute_service(hass, service_data) + with pytest.raises(ServiceValidationError) as exc_info: + await hass.services.async_call( + DOMAIN, SERVICE_TURN_ON, service_data, blocking=True + ) + + assert exc_info.value.translation_domain == DOMAIN + assert exc_info.value.translation_key == "url_not_allowed" + assert exc_info.value.translation_placeholders == { + "url": "http://denied.com/images/logo.png" + } # Light has not been modified due to failure state = hass.states.get(LIGHT_ENTITY) @@ -189,10 +198,24 @@ async def test_url_not_allowed( assert state.state == STATE_OFF +@pytest.mark.parametrize( + ("exc", "translation_key", "placeholder_keys"), + [ + pytest.param( + aiohttp.ClientError, "fetch_failed", {"url", "error"}, id="client_error" + ), + pytest.param(TimeoutError, "timeout", {"url"}, id="timeout"), + ], +) +@pytest.mark.usefixtures("setup_integration") async def test_url_exception( - hass: HomeAssistant, aioclient_mock: AiohttpClientMocker, setup_integration + hass: HomeAssistant, + aioclient_mock: AiohttpClientMocker, + exc: type[Exception], + translation_key: str, + placeholder_keys: set[str], ) -> None: - """Test that a HTTPError fails to turn light on.""" + """Test that a failed image download raises and doesn't turn light on.""" service_data = { ATTR_URL: "http://example.com/images/logo.png", ATTR_ENTITY_ID: LIGHT_ENTITY, @@ -201,10 +224,19 @@ async def test_url_exception( # Don't let the URL not being allowed sway our exception test hass.config.allowlist_external_urls.add("http://example.com/images/") - # Mock the HTTP Response with an HTTPError - aioclient_mock.get(url=service_data[ATTR_URL], exc=aiohttp.ClientError) + aioclient_mock.get(url=service_data[ATTR_URL], exc=exc) - await _async_execute_service(hass, service_data) + with pytest.raises(HomeAssistantError) as exc_info: + await hass.services.async_call( + DOMAIN, SERVICE_TURN_ON, service_data, blocking=True + ) + + assert exc_info.value.translation_domain == DOMAIN + assert exc_info.value.translation_key == translation_key + placeholders = exc_info.value.translation_placeholders + assert placeholders is not None + assert set(placeholders) == placeholder_keys + assert placeholders["url"] == service_data[ATTR_URL] # Light has not been modified due to failure state = hass.states.get(LIGHT_ENTITY) @@ -212,10 +244,18 @@ async def test_url_exception( assert state.state == STATE_OFF +@pytest.mark.parametrize( + "status", + [ + pytest.param(400, id="bad_request"), + pytest.param(304, id="not_modified"), + ], +) +@pytest.mark.usefixtures("setup_integration") async def test_url_error( - hass: HomeAssistant, aioclient_mock: AiohttpClientMocker, setup_integration + hass: HomeAssistant, aioclient_mock: AiohttpClientMocker, status: int ) -> None: - """Test that a HTTP Error (non 200) doesn't turn light on.""" + """Test that a non-OK HTTP status raises and doesn't turn light on.""" service_data = { ATTR_URL: "http://example.com/images/logo.png", ATTR_ENTITY_ID: LIGHT_ENTITY, @@ -224,10 +264,26 @@ async def test_url_error( # Don't let the URL not being allowed sway our exception test hass.config.allowlist_external_urls.add("http://example.com/images/") - # Mock the HTTP Response with a 400 Bad Request error - aioclient_mock.get(url=service_data[ATTR_URL], status=400) + aioclient_mock.get(url=service_data[ATTR_URL], status=status) - await _async_execute_service(hass, service_data) + # The body of a non-OK response must not be downloaded at all + with ( + patch( + "tests.test_util.aiohttp.AiohttpClientMockResponse.read", + side_effect=AssertionError("body read for a non-OK response"), + ), + pytest.raises(HomeAssistantError) as exc_info, + ): + await hass.services.async_call( + DOMAIN, SERVICE_TURN_ON, service_data, blocking=True + ) + + assert exc_info.value.translation_domain == DOMAIN + assert exc_info.value.translation_key == "http_error" + assert exc_info.value.translation_placeholders == { + "url": service_data[ATTR_URL], + "status": str(status), + } # Light has not been modified due to failure state = hass.states.get(LIGHT_ENTITY) @@ -298,7 +354,7 @@ async def test_file(hass: HomeAssistant, setup_integration) -> None: @patch("os.path.isfile", Mock(return_value=True)) @patch("os.access", Mock(return_value=True)) async def test_file_denied_dir(hass: HomeAssistant, setup_integration) -> None: - """Test file service fails for images in disallowed dirs.""" + """Test file service raises for images in disallowed dirs.""" service_data = { ATTR_PATH: "/path/to/a/dir/not/allowed/image.png", ATTR_ENTITY_ID: LIGHT_ENTITY, @@ -311,12 +367,16 @@ async def test_file_denied_dir(hass: HomeAssistant, setup_integration) -> None: assert state assert state.state == STATE_OFF - # Mock the file handler read with our 1x1 base64 encoded fixture image - with patch( - "homeassistant.components.color_extractor.services._get_file", _get_file_mock - ): - await hass.services.async_call(DOMAIN, SERVICE_TURN_ON, service_data) - await hass.async_block_till_done() + with pytest.raises(ServiceValidationError) as exc_info: + await hass.services.async_call( + DOMAIN, SERVICE_TURN_ON, service_data, blocking=True + ) + + assert exc_info.value.translation_domain == DOMAIN + assert exc_info.value.translation_key == "path_not_allowed" + assert exc_info.value.translation_placeholders == { + "file_path": "/path/to/a/dir/not/allowed/image.png" + } state = hass.states.get(LIGHT_ENTITY) @@ -326,6 +386,61 @@ async def test_file_denied_dir(hass: HomeAssistant, setup_integration) -> None: assert state.state == STATE_OFF +@pytest.mark.parametrize( + ("image_attr", "image_reference", "image_type"), + [ + pytest.param(ATTR_PATH, "/opt/not_an_image.txt", "file path", id="file_path"), + pytest.param( + ATTR_URL, "http://example.com/images/not_an_image.txt", "URL", id="url" + ), + ], +) +@pytest.mark.usefixtures("setup_integration") +@patch("os.path.isfile", Mock(return_value=True)) +@patch("os.access", Mock(return_value=True)) +async def test_turn_on_invalid_image( + hass: HomeAssistant, + aioclient_mock: AiohttpClientMocker, + image_attr: str, + image_reference: str, + image_type: str, +) -> None: + """Test that the turn_on service raises a ServiceValidationError when given an invalid image.""" + service_data = { + image_attr: image_reference, + ATTR_ENTITY_ID: LIGHT_ENTITY, + } + + hass.config.allowlist_external_dirs.add("/opt/") + hass.config.allowlist_external_urls.add("http://example.com/images/") + aioclient_mock.get( + url="http://example.com/images/not_an_image.txt", content=b"not an image" + ) + + with ( + patch( + "homeassistant.components.color_extractor.services._get_file", + Mock(return_value=io.BytesIO(b"not an image")), + ), + pytest.raises(ServiceValidationError) as exc_info, + ): + await hass.services.async_call( + DOMAIN, SERVICE_TURN_ON, service_data, blocking=True + ) + + assert exc_info.value.translation_domain == DOMAIN + assert exc_info.value.translation_key == "invalid_image" + assert exc_info.value.translation_placeholders == { + "image_type": image_type, + "image_reference": image_reference, + } + + # The light must stay untouched when the image cannot be read + state = hass.states.get(LIGHT_ENTITY) + assert state + assert state.state == STATE_OFF + + @patch("os.path.isfile", Mock(return_value=True)) @patch("os.access", Mock(return_value=True)) async def test_get_color_service(hass: HomeAssistant, setup_integration) -> None: @@ -390,19 +505,13 @@ async def test_get_color_service_not_allowed_path( ATTR_PATH: "/opt/not_an_image.txt", } - with ( - patch( - "homeassistant.components.color_extractor.services._get_file", - Mock(side_effect=UnidentifiedImageError("Cannot identify image file")), - ), - pytest.raises(ServiceValidationError) as exc_info, - ): + with pytest.raises(ServiceValidationError) as exc_info: await hass.services.async_call( DOMAIN, SERVICE_GET_COLOR, service_data, blocking=True, return_response=True ) - assert exc_info.value.translation_key == "invalid_image" + assert exc_info.value.translation_domain == DOMAIN + assert exc_info.value.translation_key == "path_not_allowed" assert exc_info.value.translation_placeholders == { - "image_type": "file path", - "image_reference": "/opt/not_an_image.txt", + "file_path": "/opt/not_an_image.txt", }