New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Security fix & lock for HomeMatic #11980
Conversation
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.
def supported_features(self): | ||
"""Flag supported features.""" | ||
return SUPPORT_OPENING | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blank line at end of file
Checks okay from my side. Travis job timed out. |
@danielperna84 What does this breaking change label mean? |
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 |
@property | ||
def is_locked(self): | ||
"""Return true if the lock is locked.""" | ||
return self._hmdevice.is_locked() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can only access to self._data
inside a property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pvizeli Oh okay... Can you give a short explanation why this is the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
@andrey-git @MartinHjelmare I have no door device. If that change legitime for all lock systems or should that be a platform only service? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -39,6 +39,9 @@ | |||
vol.Optional(ATTR_CODE): cv.string, | |||
}) | |||
|
|||
# Bitfield of features supported by the lock entity | |||
SUPPORT_OPENING = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be called SUPPORT_OPEN
.
@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. |
try: | ||
return not self._hm_get_state() | ||
except TypeError: | ||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simple: return not bool(self._hm_get_state())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pvizeli Okay I simplified the statement.
def supported_features(self): | ||
if self._openable: | ||
return SUPPORT_OPEN | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blank line at end of file
tests/components/lock/test_demo.py
Outdated
lock.open(self.hass, OPENABLE_LOCK) | ||
self.hass.block_till_done() | ||
|
||
self.assertFalse(lock.is_locked(self.hass, OPENABLE_LOCK)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no newline at end of file
@pvizeli Requested changes are made from my site. Can you review it? |
Please fix the lint |
|
||
def open(self, **kwargs): | ||
"""Open the door latch.""" | ||
self._state = STATE_OPEN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lock component only know STATE_UNLOCKED, STATE_LOCKED
tests/components/lock/test_demo.py
Outdated
lock.open(self.hass, OPENABLE_LOCK) | ||
self.hass.block_till_done() | ||
|
||
self.assertFalse(lock.is_locked(self.hass, OPENABLE_LOCK)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mock also the service call and look if they will be called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have an example for me how to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 In my opinion there are two solutions to this:
I personally would favour the 1. approach since completing the tests seems to be much less effort than reverting quite some code in pyhomematic. |
I will focus on completing the tests this weekend. |
@pvizeli Tests now with service mocks. |
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
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:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
tox
run successfully. Your PR cannot be merged unless tests passREQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.