openhab-scripters / openhab-helper-libraries

Scripts and modules for use with openHAB
Eclipse Public License 1.0
88 stars 70 forks source link

Add PIN code support for ideAlarm #168

Closed fastlorenzo closed 5 years ago

fastlorenzo commented 5 years ago

Fixes #74

5iver commented 5 years ago

I don't have ideAlarm setup to test. @besynnerlig, can you please review and test?

We should also add something to the documentation.

fastlorenzo commented 5 years ago

Changes made. I think we should update the doc as well. The armed home and armed away items needs to be Number and not Switch types.

besynnerlig commented 5 years ago

I don't have ideAlarm setup to test. @besynnerlig, can you please review and test?

Sure, I'll do that :)

besynnerlig commented 5 years ago

Thank you for your contribution to the repo.

Fixes #47

That must be wrong. I cant see what this to do with #47

Except for the title there is not much of a pull request description. The purpose of this Pull Request is quite obvious but can you provide a brief description or summary of the new feature?

I'll have a look at the code within a few days.

fastlorenzo commented 5 years ago

Summary of the new feature: a PIN code can be used to disarm the alarm when armed home/away. The PIN code needs to be set in the configuration.py file and must be a digit. If you do not wish to use the PIN code feature, the pinCode property can be set to None.

When using the pinCode feature, the zone armAwayToggleSwitch and armHomeToggleSwitch needs to be set to a Number and not a Switch type.

fastlorenzo commented 5 years ago

Wouldn't the approach that I suggest above not only be much easier but also more flexible do you think? It won't require any breaking changes to ideAlarm at all.

This could indeed work but I took the approach of changing the toggle switches for security reasons. When you use the pinCode, you cannot disarm the alarm without knowing the pin code which is hard coded in the python script, which means even if you have access to the event bus of OH you cannot disarm the alarm. Regarding the breaking change, I agree this is one if the pinCode is set to something else than None, but if it is set to None (which is the case by default), the script works as previously (using Switch items).

You haven't added a separate alarm event that can be triggered whenever a faulty PIN code has been entered hence what will follow when that happens is hard coded (a log entry). Maybe some users would prefer to have a light to turn on, a message be sent etc.

True, I think this could be a good addition for ideAlarm to have an item which contains an error message. A specific item with invalid pin code might work as well.

fastlorenzo commented 5 years ago

I wanted to update the documentation, but don't know how to generate the html from the rst.txt files. Any help?

CrazyIvan359 commented 5 years ago

@fastlorenzo you should be adding or editing documentation in the .rst files in the /Sphinx directory. I believe Scott is pushing the changes to the output files separately so that PRs are more readable since rebuilding the pages can sometimes modify up to 100 or more files.

If you want to build the pages so you can see them for proofing purposes, there are instructions here, just don't commit any changes to files in the /docs directory.

besynnerlig commented 5 years ago

Wouldn't the approach that I suggest above not only be much easier but also more flexible do you think? It won't require any breaking changes to ideAlarm at all.

This could indeed work but I took the approach of changing the toggle switches for security reasons. When you use the pinCode, you cannot disarm the alarm without knowing the pin code which is hard coded in the python script, which means even if you have access to the event bus of OH you cannot disarm the alarm.

You could. Frankly, if an intruder can access the Event Bus, e.g use a script to send a given command to the specified Item there are numerous ways to break the functionality of ideAlarm. for example sendCommand("Z1_Arming_Mode", 0)

Regarding the breaking change, I agree this is one if the pinCode is set to something else than None, but if it is set to None (which is the case by default), the script works as previously (using Switch items).

No it won't. For example trying to get the integer value of a switch like in your statement pinCode = getItemValue(itemName, 0) will throw AttributeError: 'org.eclipse.smarthome.core.library.types.OnOffType' object has no attribute 'intValue'

Even if you adapt your code in some way you must remember that the users have custom scripts that expect the arming mode toggle switches to be of the type switch. If we shall change the item type for the arming mode toggling switches we need to gain something quite valuable to do so. We also need to rename the items to not hold the word "switch" in their names because it's confusing. Unfortunately I can't see that we gain security or anything else by doing this.

You haven't added a separate alarm event that can be triggered whenever a faulty PIN code has been entered hence what will follow when that happens is hard coded (a log entry). Maybe some users would prefer to have a light to turn on, a message be sent etc.

True, I think this could be a good addition for ideAlarm to have an item which contains an error message. A specific item with invalid pin code might work as well.

It's not what I meant. ideAlarm has Event Helpers for specific events. That's the correct place to create your own error messages. If your PR would be approved we'd need another event helper: onPinEntered(zone, pincode) . Just letting ideAlarm log an entry when a faulty PIN is entered is not enough. Having an event helper would also allow a user to define and handle multiple PIN codes. But all this and more is easier to implement using the simple script that I suggested above.

I still can't see any benefits using the suggested PR over using the simple script example that I suggested above. If you don't agree with me, please convince me by giving me more details in how the security is improved and please let me know how a user can customize what will happen when a PIN code (correct or faulty) has been entered.

fastlorenzo commented 5 years ago

I'll try your approach and see if it can work, it can indeed be easier to implement than all the breaking changes I wanted to introduce.

You could. Frankly, if an intruder can access the Event Bus, e.g use a script to send a given command to the specified Item there are numerous ways to break the functionality of ideAlarm. for example sendCommand("Z1_Arming_Mode", 0)

Good point, it might be interesting to find a solution so only the ideAlarm script can change the state of those items. I'm not sure this is something feasible in OH at the moment.

besynnerlig commented 5 years ago

I'll try your approach and see if it can work, it can indeed be easier to implement than all the breaking changes I wanted to introduce.

Please do that and please don't hesitate to ask if there's something that you can't solve by using the example script.

If you after trying think that the script is working well, we could add the example script to the documentation.

besynnerlig commented 5 years ago

I'll try your approach and see if it can work, it can indeed be easier to implement than all the breaking changes I wanted to introduce.

@fastlorenzo , have you had the time yet to check it out?

5iver commented 5 years ago

Pinging @fastlorenzo... shall we close this?

5iver commented 5 years ago

Closing due to lack of response.