Merge pull request #1661 from jaharkes/service-call-validation

Service call validation

homeassistant/components/light/__init__.py

@@ -8,6 +8,8 @@ import logging
 import os
 import csv
 
+import voluptuous as vol
+
 from homeassistant.components import (
     group, discovery, wemo, wink, isy994,
     zwave, insteon_hub, mysensors, tellstick, vera)
@@ -18,7 +20,7 @@ from homeassistant.const import (
 from homeassistant.helpers.entity import ToggleEntity
 from homeassistant.helpers.entity_component import EntityComponent
 from homeassistant.helpers.config_validation import PLATFORM_SCHEMA  # noqa
-import homeassistant.util as util
+import homeassistant.helpers.config_validation as cv
 import homeassistant.util.color as color_util
 
 
@@ -77,6 +79,37 @@ PROP_TO_ATTR = {
     'xy_color': ATTR_XY_COLOR,
 }
 
+# Service call validation schemas
+VALID_TRANSITION = vol.All(vol.Coerce(int), vol.Clamp(min=0, max=900))
+
+LIGHT_TURN_ON_SCHEMA = vol.Schema({
+    ATTR_ENTITY_ID: cv.entity_ids,
+    ATTR_PROFILE: str,
+    ATTR_TRANSITION: VALID_TRANSITION,
+    ATTR_BRIGHTNESS: cv.byte,
+    ATTR_RGB_COLOR: vol.All(vol.ExactSequence((cv.byte, cv.byte, cv.byte)),
+                            vol.Coerce(tuple)),
+    ATTR_XY_COLOR: vol.All(vol.ExactSequence((cv.small_float, cv.small_float)),
+                           vol.Coerce(tuple)),
+    ATTR_COLOR_TEMP: vol.All(int, vol.Range(min=154, max=500)),
+    ATTR_FLASH: [FLASH_SHORT, FLASH_LONG],
+    ATTR_EFFECT: [EFFECT_COLORLOOP, EFFECT_RANDOM, EFFECT_WHITE],
+})
+
+LIGHT_TURN_OFF_SCHEMA = vol.Schema({
+    ATTR_ENTITY_ID: cv.entity_ids,
+    ATTR_TRANSITION: VALID_TRANSITION,
+})
+
+LIGHT_TOGGLE_SCHEMA = vol.Schema({
+    ATTR_ENTITY_ID: cv.entity_ids,
+    ATTR_TRANSITION: VALID_TRANSITION,
+})
+
+PROFILE_SCHEMA = vol.Schema(
+    vol.ExactSequence((str, cv.small_float, cv.small_float, cv.byte))
+)
+
 _LOGGER = logging.getLogger(__name__)
 
 
@@ -156,31 +189,22 @@ def setup(hass, config):
             next(reader, None)
 
             try:
-                for profile_id, color_x, color_y, brightness in reader:
+                for rec in reader:
-                    profiles[profile_id] = (float(color_x), float(color_y),
+                    profile, color_x, color_y, brightness = PROFILE_SCHEMA(rec)
-                                            int(brightness))
+                    profiles[profile] = (color_x, color_y, brightness)
-            except ValueError:
+            except vol.MultipleInvalid as ex:
-                # ValueError if not 4 values per row
+                _LOGGER.error("Error parsing light profile from %s: %s",
-                # ValueError if convert to float/int failed
+                              profile_path, ex)
-                _LOGGER.error(
-                    "Error parsing light profiles from %s", profile_path)
-
                 return False
 
     def handle_light_service(service):
         """Hande a turn light on or off service call."""
-        # Get and validate data
+        # Get the validated data
-        dat = service.data
+        params = service.data.copy()
 
         # Convert the entity ids to valid light ids
         target_lights = component.extract_from_service(service)
-
+        params.pop(ATTR_ENTITY_ID, None)
-        params = {}
-
-        transition = util.convert(dat.get(ATTR_TRANSITION), int)
-
-        if transition is not None:
-            params[ATTR_TRANSITION] = transition
 
         service_fun = None
         if service.service == SERVICE_TURN_OFF:
@@ -198,63 +222,11 @@ def setup(hass, config):
             return
 
         # Processing extra data for turn light on request.
-
+        profile = profiles.get(params.pop(ATTR_PROFILE, None))
-        # We process the profile first so that we get the desired
-        # behavior that extra service data attributes overwrite
-        # profile values.
-        profile = profiles.get(dat.get(ATTR_PROFILE))
 
         if profile:
-            *params[ATTR_XY_COLOR], params[ATTR_BRIGHTNESS] = profile
+            params.setdefault(ATTR_XY_COLOR, profile[:2])
-
+            params.setdefault(ATTR_BRIGHTNESS, profile[2])
-        if ATTR_BRIGHTNESS in dat:
-            # We pass in the old value as the default parameter if parsing
-            # of the new one goes wrong.
-            params[ATTR_BRIGHTNESS] = util.convert(
-                dat.get(ATTR_BRIGHTNESS), int, params.get(ATTR_BRIGHTNESS))
-
-        if ATTR_XY_COLOR in dat:
-            try:
-                # xy_color should be a list containing 2 floats.
-                xycolor = dat.get(ATTR_XY_COLOR)
-
-                # Without this check, a xycolor with value '99' would work.
-                if not isinstance(xycolor, str):
-                    params[ATTR_XY_COLOR] = [float(val) for val in xycolor]
-
-            except (TypeError, ValueError):
-                # TypeError if xy_color is not iterable
-                # ValueError if value could not be converted to float
-                pass
-
-        if ATTR_COLOR_TEMP in dat:
-            # color_temp should be an int of mireds value
-            colortemp = dat.get(ATTR_COLOR_TEMP)
-
-            # Without this check, a ctcolor with value '99' would work
-            # These values are based on Philips Hue, may need ajustment later
-            if isinstance(colortemp, int) and 154 <= colortemp <= 500:
-                params[ATTR_COLOR_TEMP] = colortemp
-
-        if ATTR_RGB_COLOR in dat:
-            try:
-                # rgb_color should be a list containing 3 ints
-                rgb_color = dat.get(ATTR_RGB_COLOR)
-
-                if len(rgb_color) == 3:
-                    params[ATTR_RGB_COLOR] = [int(val) for val in rgb_color]
-
-            except (TypeError, ValueError):
-                # TypeError if rgb_color is not iterable
-                # ValueError if not all values can be converted to int
-                pass
-
-        if dat.get(ATTR_FLASH) in (FLASH_SHORT, FLASH_LONG):
-            params[ATTR_FLASH] = dat[ATTR_FLASH]
-
-        if dat.get(ATTR_EFFECT) in (EFFECT_COLORLOOP, EFFECT_WHITE,
-                                    EFFECT_RANDOM):
-            params[ATTR_EFFECT] = dat[ATTR_EFFECT]
 
         for light in target_lights:
             light.turn_on(**params)
@@ -267,13 +239,16 @@ def setup(hass, config):
     descriptions = load_yaml_config_file(
         os.path.join(os.path.dirname(__file__), 'services.yaml'))
     hass.services.register(DOMAIN, SERVICE_TURN_ON, handle_light_service,
-                           descriptions.get(SERVICE_TURN_ON))
+                           descriptions.get(SERVICE_TURN_ON),
+                           schema=LIGHT_TURN_ON_SCHEMA)
 
     hass.services.register(DOMAIN, SERVICE_TURN_OFF, handle_light_service,
-                           descriptions.get(SERVICE_TURN_OFF))
+                           descriptions.get(SERVICE_TURN_OFF),
+                           schema=LIGHT_TURN_OFF_SCHEMA)
 
     hass.services.register(DOMAIN, SERVICE_TOGGLE, handle_light_service,
-                           descriptions.get(SERVICE_TOGGLE))
+                           descriptions.get(SERVICE_TOGGLE),
+                           schema=LIGHT_TOGGLE_SCHEMA)
 
     return True
 

homeassistant/core.py

@@ -14,6 +14,8 @@ import threading
 import time
 from types import MappingProxyType
 
+import voluptuous as vol
+
 import homeassistant.helpers.temperature as temp_helper
 import homeassistant.util as util
 import homeassistant.util.dt as dt_util
@@ -494,13 +496,14 @@ class StateMachine(object):
 class Service(object):
     """Represents a callable service."""
 
-    __slots__ = ['func', 'description', 'fields']
+    __slots__ = ['func', 'description', 'fields', 'schema']
 
-    def __init__(self, func, description, fields):
+    def __init__(self, func, description, fields, schema):
         """Initialize a service."""
         self.func = func
         self.description = description or ''
         self.fields = fields or {}
+        self.schema = schema
 
     def as_dict(self):
         """Return dictionary representation of this service."""
@@ -511,7 +514,14 @@ class Service(object):
 
     def __call__(self, call):
         """Execute the service."""
-        self.func(call)
+        try:
+            if self.schema:
+                call.data = self.schema(call.data)
+
+            self.func(call)
+        except vol.MultipleInvalid as ex:
+            _LOGGER.error('Invalid service data for %s.%s: %s',
+                          call.domain, call.service, ex)
 
 
 # pylint: disable=too-few-public-methods
@@ -560,16 +570,20 @@ class ServiceRegistry(object):
         """Test if specified service exists."""
         return service in self._services.get(domain, [])
 
-    def register(self, domain, service, service_func, description=None):
+    # pylint: disable=too-many-arguments
+    def register(self, domain, service, service_func, description=None,
+                 schema=None):
         """
         Register a service.
 
         Description is a dict containing key 'description' to describe
         the service and a key 'fields' to describe the fields.
+
+        Schema is called to coerce and validate the service data.
         """
         description = description or {}
         service_obj = Service(service_func, description.get('description'),
-                              description.get('fields', {}))
+                              description.get('fields', {}), schema)
         with self._lock:
             if domain in self._services:
                 self._services[domain][service] = service_obj

homeassistant/helpers/config_validation.py

@@ -12,6 +12,8 @@ PLATFORM_SCHEMA = vol.Schema({
     vol.Required(CONF_PLATFORM): str,
 }, extra=vol.ALLOW_EXTRA)
 
+byte = vol.All(vol.Coerce(int), vol.Range(min=0, max=255))
+small_float = vol.All(vol.Coerce(float), vol.Range(min=0, max=1))
 latitude = vol.All(vol.Coerce(float), vol.Range(min=-90, max=90))
 longitude = vol.All(vol.Coerce(float), vol.Range(min=-180, max=180))
 

tests/components/light/test_init.py

@@ -170,8 +170,8 @@ class TestLight(unittest.TestCase):
         light.turn_on(self.hass, dev1.entity_id,
                       transition=10, brightness=20)
         light.turn_on(
-            self.hass, dev2.entity_id, rgb_color=[255, 255, 255])
+            self.hass, dev2.entity_id, rgb_color=(255, 255, 255))
-        light.turn_on(self.hass, dev3.entity_id, xy_color=[.4, .6])
+        light.turn_on(self.hass, dev3.entity_id, xy_color=(.4, .6))
 
         self.hass.pool.block_till_done()
 
@@ -182,10 +182,10 @@ class TestLight(unittest.TestCase):
             data)
 
         method, data = dev2.last_call('turn_on')
-        self.assertEquals(data[light.ATTR_RGB_COLOR], [255, 255, 255])
+        self.assertEquals(data[light.ATTR_RGB_COLOR], (255, 255, 255))
 
         method, data = dev3.last_call('turn_on')
-        self.assertEqual({light.ATTR_XY_COLOR: [.4, .6]}, data)
+        self.assertEqual({light.ATTR_XY_COLOR: (.4, .6)}, data)
 
         # One of the light profiles
         prof_name, prof_x, prof_y, prof_bri = 'relax', 0.5119, 0.4147, 144
@@ -195,23 +195,24 @@ class TestLight(unittest.TestCase):
         # Specify a profile and attributes to overwrite it
         light.turn_on(
             self.hass, dev2.entity_id,
-            profile=prof_name, brightness=100, xy_color=[.4, .6])
+            profile=prof_name, brightness=100, xy_color=(.4, .6))
 
         self.hass.pool.block_till_done()
 
         method, data = dev1.last_call('turn_on')
         self.assertEqual(
             {light.ATTR_BRIGHTNESS: prof_bri,
-             light.ATTR_XY_COLOR: [prof_x, prof_y]},
+             light.ATTR_XY_COLOR: (prof_x, prof_y)},
             data)
 
         method, data = dev2.last_call('turn_on')
         self.assertEqual(
             {light.ATTR_BRIGHTNESS: 100,
-             light.ATTR_XY_COLOR: [.4, .6]},
+             light.ATTR_XY_COLOR: (.4, .6)},
             data)
 
         # Test shitty data
+        light.turn_on(self.hass)
         light.turn_on(self.hass, dev1.entity_id, profile="nonexisting")
         light.turn_on(self.hass, dev2.entity_id, xy_color=["bla-di-bla", 5])
         light.turn_on(self.hass, dev3.entity_id, rgb_color=[255, None, 2])
@@ -227,7 +228,7 @@ class TestLight(unittest.TestCase):
         method, data = dev3.last_call('turn_on')
         self.assertEqual({}, data)
 
-        # faulty attributes should not overwrite profile data
+        # faulty attributes will not trigger a service call
         light.turn_on(
             self.hass, dev1.entity_id,
             profile=prof_name, brightness='bright', rgb_color='yellowish')
@@ -235,10 +236,7 @@ class TestLight(unittest.TestCase):
         self.hass.pool.block_till_done()
 
         method, data = dev1.last_call('turn_on')
-        self.assertEqual(
+        self.assertEqual({}, data)
-            {light.ATTR_BRIGHTNESS: prof_bri,
-             light.ATTR_XY_COLOR: [prof_x, prof_y]},
-            data)
 
     def test_broken_light_profiles(self):
         """Test light profiles."""
@@ -280,5 +278,5 @@ class TestLight(unittest.TestCase):
         method, data = dev1.last_call('turn_on')
 
         self.assertEqual(
-            {light.ATTR_XY_COLOR: [.4, .6], light.ATTR_BRIGHTNESS: 100},
+            {light.ATTR_XY_COLOR: (.4, .6), light.ATTR_BRIGHTNESS: 100},
             data)

tests/components/light/test_mqtt.py

@@ -305,7 +305,7 @@ class TestLightMQTT(unittest.TestCase):
 
         state = self.hass.states.get('light.test')
         self.assertEqual(STATE_ON, state.state)
-        self.assertEqual([75, 75, 75], state.attributes['rgb_color'])
+        self.assertEqual((75, 75, 75), state.attributes['rgb_color'])
         self.assertEqual(50, state.attributes['brightness'])
 
     def test_show_brightness_if_only_command_topic(self):