Security fix & lock for HomeMatic by PH89 · Pull Request #11980 · home-assistant/core

PH89 · 2018-01-28T00:52:10Z

Description:

This PR intruduces a new lock component for HomeMatic door locks.
It also improves the general base lock component by adding a 3rd service open, that makes it possable to open the door by releasing the door latch.

Pull request in documentation:
home-assistant/home-assistant.io#4537

Pull request in Frontend:
https://github.com/home-assistant/home-assistant-polymer#845

Checklist:

The code change is tested and works locally.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

Locks may are capable to open the door latch. If component is support it, the SUPPORT_OPENING bitmask can be supplied in the supported_features property.

…into dev

houndci-bot · 2018-01-28T00:52:19Z

homeassistant/components/lock/homematic.py

+    def supported_features(self):
+        """Flag supported features."""
+        return SUPPORT_OPENING
+


blank line at end of file

PH89 · 2018-01-28T02:22:12Z

Checks okay from my side. Travis job timed out.

PH89 · 2018-01-28T14:20:30Z

@danielperna84 What does this breaking change label mean?
This code should not break anything with other lock components.

danielperna84 · 2018-01-28T14:35:16Z

It will break automations. Until now the component has been a switch. Having it as a lock-platform requires the automations to be changed accordingly. So when the switch.turn_on service was used before to change the state, this will not work anymore. Except when I'm missing something here. I don't own such locks, so I don't know how they are practically used. But my assumption would be, that they are currently used with the switch-services.

pvizeli · 2018-01-30T11:32:52Z

homeassistant/components/lock/homematic.py

+    @property
+    def is_locked(self):
+        """Return true if the lock is locked."""
+        return self._hmdevice.is_locked()


You can only access to self._data inside a property

@pvizeli Oh okay... Can you give a short explanation why this is the case?

@pvizeli I think this is resolved now by returning not self._hm_get_state() here.

However it is possable to access self._hmdevice, after the homematic device is linked.
Durring my testing the is_locked method is only called after the device is linked and the self._hmdevice is set up.
Please correct me if I am wrong and explain it in a little more detail.

pvizeli · 2018-01-30T11:37:03Z

@andrey-git @MartinHjelmare I have no door device. If that change legitime for all lock systems or should that be a platform only service?

MartinHjelmare

I think the new open service is valid as a general lock service. I think opening the door is a thing among smart locks.

There should also be a module level function to call the new service in the lock component.

MartinHjelmare · 2018-01-30T13:28:24Z

homeassistant/components/lock/__init__.py

 })

+# Bitfield of features supported by the lock entity
+SUPPORT_OPENING = 1


I think this should be called SUPPORT_OPEN.

andrey-git · 2018-01-30T18:35:43Z

@pvizeli I think that as long there is a single device (homematic) that supports "open" the service would be better at the platform level. However "supported_features" must be on the component level (for UI support) so I'm OK with the service being on component level.

pvizeli · 2018-02-06T11:33:27Z

homeassistant/components/lock/homematic.py

+        try:
+            return not self._hm_get_state()
+        except TypeError:
+            return False


simple: return not bool(self._hm_get_state())

@pvizeli Okay I simplified the statement.

houndci-bot · 2018-02-10T00:56:00Z

homeassistant/components/lock/demo.py

+    def supported_features(self):
+        if self._openable:
+            return SUPPORT_OPEN
+


blank line at end of file

houndci-bot · 2018-03-01T19:11:19Z

tests/components/lock/test_demo.py

+        lock.open(self.hass, OPENABLE_LOCK)
+        self.hass.block_till_done()
+
+        self.assertFalse(lock.is_locked(self.hass, OPENABLE_LOCK))


no newline at end of file

PH89 · 2018-03-01T20:03:57Z

@pvizeli Requested changes are made from my site. Can you review it?

pvizeli · 2018-03-04T10:38:20Z

Please fix the lint

pvizeli · 2018-03-04T10:42:22Z

homeassistant/components/lock/demo.py

+
+    def open(self, **kwargs):
+        """Open the door latch."""
+        self._state = STATE_OPEN


The lock component only know STATE_UNLOCKED, STATE_LOCKED

pvizeli · 2018-03-04T10:43:06Z

tests/components/lock/test_demo.py

+        lock.open(self.hass, OPENABLE_LOCK)
+        self.hass.block_till_done()
+
+        self.assertFalse(lock.is_locked(self.hass, OPENABLE_LOCK))


Mock also the service call and look if they will be called

Do you have an example for me how to do this?

https://github.com/home-assistant/home-assistant/blob/dev/tests/common.py#L193

danielperna84 · 2018-03-23T14:57:39Z

Any chance this can be included in the next version? The changes in pyhomematic that were needed to allow the Lock component caused a break for the existing implementation that is using the switch. This has been mentioned here: https://community.home-assistant.io/t/homematic-hm-sec-key-not-working-after-upgrading-to-0-64-0-65/46693
Failing automations that should lock doors are a BIG security issue. I wasn't aware of that since I myself don't own such devices.

In my opinion there are two solutions to this:

This PR gets merged (possibly without the requested test).
I revert the changes in pyhomematic to restore the previous functionality.

I personally would favour the 1. approach since completing the tests seems to be much less effort than reverting quite some code in pyhomematic.

PH89 · 2018-03-23T15:03:14Z

I will focus on completing the tests this weekend.

…into homematic_lock

PH89 · 2018-03-25T16:39:07Z

@pvizeli Tests now with service mocks.

pvizeli · 2018-03-25T20:27:10Z

I tag this for 0.66 while it is a security hole for all they use this device

* HomeMatic KeyMatic device become a real lock component * Adds supported features to lock component. Locks may are capable to open the door latch. If component is support it, the SUPPORT_OPENING bitmask can be supplied in the supported_features property. * hound improvements. * Travis improvements. * Improvements from review process * Simplifies is_locked method * Adds an openable lock in the lock demo component * removes blank line * Adds test for openable demo lock and lint and reviewer improvements. * adds new line... * Comment end with a period. * Additional blank line. * Mock service based testing, lint fixes * Update description

PH89 added 3 commits January 27, 2018 18:50

HomeMatic KeyMatic device become a real lock component

a5bcfed

Adds supported features to lock component.

502a501

Locks may are capable to open the door latch. If component is support it, the SUPPORT_OPENING bitmask can be supplied in the supported_features property.

Merge branch 'dev' of https://github.com/home-assistant/home-assistant …

02acb16

…into dev

homeassistant added integration: homematic integration: lock core new-platform platform: lock.homematic cla-signed labels Jan 28, 2018

houndci-bot reviewed Jan 28, 2018

View reviewed changes

hound improvements.

b477cc2

PH89 mentioned this pull request Jan 28, 2018

Lock component capable to open door latches home-assistant/frontend#845

Closed

Travis improvements.

175ac53

PH89 mentioned this pull request Jan 28, 2018

Documentation of new HomeMatic lock section. home-assistant/home-assistant.io#4537

Merged

2 tasks

danielperna84 added the breaking-change label Jan 28, 2018

pvizeli requested changes Jan 30, 2018

View reviewed changes

MartinHjelmare reviewed Jan 30, 2018

View reviewed changes

Improvements from review process

19c3a20

homeassistant added the cla-signed label Feb 5, 2018

pvizeli requested changes Feb 6, 2018

View reviewed changes

Simplifies is_locked method

dce9e8c

homeassistant added the cla-signed label Feb 9, 2018

Adds an openable lock in the lock demo component

460240d

homeassistant added the cla-signed label Feb 10, 2018

houndci-bot reviewed Feb 10, 2018

View reviewed changes

homeassistant added the cla-signed label Mar 1, 2018

houndci-bot reviewed Mar 1, 2018

View reviewed changes

PH89 added 3 commits March 1, 2018 20:12

adds new line...

62b7021

Comment end with a period.

06b5d7b

Additional blank line.

d168ee8

pvizeli requested changes Mar 4, 2018

View reviewed changes

PH89 added 2 commits March 25, 2018 13:24

Merge branch 'dev' of https://github.com/home-assistant/home-assistant …

cfc3c23

…into homematic_lock

Mock service based testing, lint fixes

4d6977a

homeassistant added the cla-signed label Mar 25, 2018

Update description

940e560

homeassistant added the cla-signed label Mar 25, 2018

pvizeli approved these changes Mar 25, 2018

View reviewed changes

pvizeli added this to the 0.66 milestone Mar 25, 2018

pvizeli changed the title ~~Lock component for HomeMatic KeyMatic~~ Security fix & lock for HomeMatic Mar 25, 2018

pvizeli added the cla-recheck label Mar 25, 2018

homeassistant added cla-signed and removed cla-recheck labels Mar 25, 2018

pvizeli merged commit 6d20a84 into home-assistant:dev Mar 25, 2018

balloobbot added the cherry-picked label Mar 26, 2018

balloob mentioned this pull request Mar 30, 2018

0.66.0 #13554

Merged

metbril mentioned this pull request Jul 8, 2018

Zwave lock switch is now reversed 0.73 beta #15276

Closed

home-assistant locked and limited conversation to collaborators Jul 26, 2018

ghost removed the platform: lock.homematic label Mar 21, 2019

Uh oh!

Conversation

PH89 commented Jan 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PH89 commented Jan 28, 2018

Uh oh!

PH89 commented Jan 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielperna84 commented Jan 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pvizeli commented Jan 30, 2018

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrey-git commented Jan 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PH89 commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pvizeli commented Mar 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielperna84 commented Mar 23, 2018

Uh oh!

PH89 commented Mar 23, 2018

Uh oh!

PH89 commented Mar 25, 2018

Uh oh!

pvizeli commented Mar 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

PH89 commented Jan 28, 2018 •

edited

Loading

PH89 commented Jan 28, 2018 •

edited

Loading

PH89 commented Mar 1, 2018 •

edited

Loading