Core: scene.turn_on with wrong service data tries to enable all scenes

[dkagedal]

Home Assistant release with the issue:

0.99.3

Last working Home Assistant release (if known):

Operating environment (Hass.io/Docker/Windows/etc.):

Integration:

https://github.com/home-assistant/home-assistant/blob/41242110957cbab8b27fb1bfeefa3a37f5cd92dd/homeassistant/components/scene/__init__.py#L74

Description of problem:
I was creating an automation that called scene.turn_on and someone failed to format my service data properly. This led to HA enabling every scene it knew about, in turn. This caused weird flashing and potentially waking up sleeping kids etc.

I think that scene.turn_on should not default to turning on every scene if the entity id is missing, and should require all to be provided for that case.

I think the underlying reason is that scene.py uses component.async_extract_from_service(service) which defaults to returning all entities if no entity id was found.

I think this internal API is problematic since it can easily fail in unintended(?) ways like this. Best to be explicit in the intent, and for example have a separate method async_extract_from_service_or_all. Or a default=None parameter to the current method that callers can set to 'all' explicitly.

Problem-relevant configuration.yaml entries and (fill out even if it seems unimportant):

Traceback (if applicable):

Additional information:

[dkagedal]

Kind of related: https://www.monitis.com/blog/apis-and-the-principle-of-least-surprise/

[probot-home-assistant[bot]]

Hey there @home-assistant/core, mind taking a look at this issue as its been labeled with a integration (scene) you are listed as a codeowner for? Thanks!

[balloob]

That's why this behavior is deprecated and should be removed https://github.com/home-assistant/home-assistant/blob/dabdf8b577516bd04168234671b6ada335c7c763/homeassistant/helpers/entity_component.py#L173-L192

A PR to remove it is welcome 👍

[krissen]

Would this also solve the problem of script.turn_on being called with erroneous or no service data, running every script?

Came to Github to file a bug report for the above, but found this ticket, and if I understand your comment correctly, @balloob, what you're pointing at would apply to that as well?

Sidenote, on a Halloweenish tone: having a bunch of scripts (for lights, sounds, music, heating etc.), testing stuff, and suddenly finding _each and every script being executed_ is a good way to... make it seem like the house is haunted. _(Who you gonna call?)_ Everyone in the house, at 10pm, wasn't laughing though.

[dkagedal]

Sounds good, I'll send a simple removal PR when I get some time over, unless someone beats me to it.

[balloob]

@krissen correct.

@dkagedal thanks

[MatthewFlamm]

@dkagedal did you ever get around to this?
I have accidentally set off my house alarm multiple times because of this behavior. If not, what is required? Should an exception be raised instead of logging a deprecation warning?

[balloob]

The schema needs to be updated to consider this invalid data. Then if it is not passed in, we should raise an exception indeed.

[balloob]

https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/helpers/config_validation.py#L720 change vol.Optional to vol.Required.

[MatthewFlamm]

In my case, I encountered this behavior when calling a turn_on service from the dev tools service menu. The traceback to the warning was here:

https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/helpers/service.py#L266-L275

[balloob]

Right, you should raise there too. But I doubt you'll get into that code if you change the Optional to Required :)

[dkagedal]

@dkagedal did you ever get around to this?

Making the code change was easy enough, but I've been struggling to set up my dev environment and figure out how to run tests etc with my limited time. So, no, not yet I'm afraid.

[mdegat01]

@balloob this should be closed right? Was looking through help wanted issues and it looks like you fixed this issue with https://github.com/home-assistant/core/pull/29178

[MartinHjelmare]

Fixed in #29178