guerrerotook / securitas-direct-new-api

This repository contains the new securitas direct API that can be integrated in Home Assistant
Apache License 2.0
78 stars 30 forks source link

adding a bit of error handling, cleaning up code, and updating docs #213

Closed cantupaz closed 7 months ago

cantupaz commented 7 months ago

Hi. I added a little bit of error handling when arming/disarming. I got rid of the ENABLE_CODE flag that was probably confusing users in the config window (I found it confusing that I could leave the code empty and I also had to uncheck the box to get rid of the PIN).

I added a few missing type hints and updated the readme file.

cantupaz commented 7 months ago

I can try to get to this tomorrow, but at least in Spain the 2FA doesn’t do anything. Of course, in the official app, it won’t let you proceed without 2FA, but the API returns the authorization token just after login. You get the exact same token after the 2FA.

From: Luis Guerrero @.> Sent: Tuesday, February 27, 2024 11:18 AM To: guerrerotook/securitas-direct-new-api @.> Cc: Erick Cantú Paz @.>; Mention @.> Subject: Re: [guerrerotook/securitas-direct-new-api] adding a bit of error handling, cleaning up code, and updating docs (PR #213)

@guerrerotook requested changes on this pull request.


In README.md https://github.com/guerrerotook/securitas-direct-new-api/pull/213#discussion_r1503984631 :

@@ -14,13 +14,27 @@ securitas:

Features

-- List all your installations and add a panel into Home Assistant. -- Support Sentinel and add two sensor for each Sentinel in each installation you have. The sensor are temperature and humidity. -- If the option is set to False, the check_alarm will only check the last status that securitas have in their server instead of checking in the alarm itself. This will decrease the number of request that show in your account. In this is set to true and you arm or disarm the alarm not throught Home Assistant, this will likely show a different state. The default value is True. -- Added a new configuration panel to change some of the options that you set during the setup. Here is an screenshot. +- Lists all your installations and add a panel into Home Assistant. +- Supports Sentinel and adds temperature and humidity sensors for each Sentinel in each installation you have. +- Supports installations with Perimetral (external) alarms. + +## Setup +Options + +- Enter the username and password for your Securitas account. +- Use 2FA (default: no). It is not necessary to use two-factor authentication to use this integration, but we're leaving that option in case Securitas changes that in the future.

Hey @cantupaz https://github.com/cantupaz I think that the default should be yes, enabled by default. I think all countries are actually enabling 2FA right now. That how it was in the past, and the API stopped working. I don't see Securitas not enabling the 2FA or even disabling this. I don't know where you are getting this information from, but this should be enabled by default.


In custom_components/securitas/init.py https://github.com/guerrerotook/securitas-direct-new-api/pull/213#discussion_r1503987133 :

@@ -50,41 +50,19 @@ CONF_DEVICE_INDIGITALL = "idDeviceIndigitall" CONF_ENTRY_ID = "entry_id" CONF_INSTALLATION_KEY = "instalation" -CONF_ENABLE_CODE = "enable_code" CONF_DELAY_CHECK_OPERATION = "delay_check_operation"

DEFAULT_USE_2FA = False

This should be true, look at my previous comment.

— Reply to this email directly, view it on GitHub https://github.com/guerrerotook/securitas-direct-new-api/pull/213#pullrequestreview-1903096665 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AT44HH2D5W7Q74TRHO7B6VTYVWXG5AVCNFSM6AAAAABD3YA5MSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMBTGA4TMNRWGU . You are receiving this because you were mentioned. https://github.com/notifications/beacon/AT44HH3H4DSNH7C5PFXHNTLYVWXG5A5CNFSM6AAAAABD3YA5MSWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTTRN3ZVS.gif Message ID: @. @.> >

guerrerotook commented 7 months ago

That is not true @cantupaz, there are a lot of issues people complaining that it logged but then after some time the integration stop working, https://github.com/guerrerotook/securitas-direct-new-api/issues/52 here in Spain, France and other countries as well.

Again, I don't know exactly where you got that it doesn't do anything. My recommendation, for everyone should be to enable this and as you said, it this fails for you have the option to disable, but not the other way around. This works for you without the 2FA fine, but we have had a lot of issues in the past with this and I believe that not enabling this will likely result in more people opening issues because weird errors that all down to not be authenticated.

guerrerotook commented 7 months ago

Just to be clear, I only want you to change the default value to true, that's it. The rest of the PR is great!

cantupaz commented 7 months ago

Easy. I said I’ll change it, but probably tomorrow because I got something to do today. Btw, I changed that months ago 😊 I agree it’s safer to switch the default in case one day Securitas stops sending the token after login.

From: Luis Guerrero @.> Sent: Tuesday, February 27, 2024 12:05 PM To: guerrerotook/securitas-direct-new-api @.> Cc: Erick Cantú Paz @.>; Mention @.> Subject: Re: [guerrerotook/securitas-direct-new-api] adding a bit of error handling, cleaning up code, and updating docs (PR #213)

That is not true @cantupaz https://github.com/cantupaz , there are a lot of issues people complaining that it logged but then after some time the integration stop working, #52 https://github.com/guerrerotook/securitas-direct-new-api/issues/52 here in Spain, France and other countries as well.

Again, I don't know exactly where you got that it doesn't do anything. My recommendation, for everyone should be to enable this and as you said, it this fails for you have the option to disable, but not the other way around. This works for you without the 2FA fine, but we have had a lot of issues in the past with this and I believe that not enabling this will likely result in more people opening issues because weird errors that all down to not be authenticated.

— Reply to this email directly, https://github.com/guerrerotook/securitas-direct-new-api/pull/213#issuecomment-1966305726 view it on GitHub, or https://github.com/notifications/unsubscribe-auth/AT44HHY3SX4QXZMMFOWWF2LYVW4XLAVCNFSM6AAAAABD3YA5MSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRWGMYDKNZSGY unsubscribe. You are receiving this because you were mentioned. https://github.com/notifications/beacon/AT44HH7PM7BACTXSVSICYY3YVW4XLA5CNFSM6AAAAABD3YA5MSWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTTVGNY34.gif Message ID: < @.> @.>

guerrerotook commented 7 months ago

Thanks very much!, don't worry about when, take your time.

cantupaz commented 7 months ago

I made the changes, please take a look.