-
Notifications
You must be signed in to change notification settings - Fork 42
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
Handle different 4xx-errors #171
base: version-2
Are you sure you want to change the base?
Conversation
Trying harder to log in on a 4xx Error code.
Hi, Please bump the version (patch) and add a row to the revision history in README. let's hope this will bring us closer to understanding the issue... |
I am a bit reluctant to just release this as I don't know the code well enough to really trust what will happen if we "ccontinue" on a 4xx-error. |
As I see it the issue with this change might be that a wrong password is used once more, and thus using up allowed retries faster. The "contine" will only do one more Try on the other Endpoint, so not really a risk to start an involuntary ddos attack 😀 |
Tested it locally, and it works as expected. I added an extra debug print() here to verify that it only runs twice and with different urls:
Logging in with muy real username and password works fine. |
Can you also show that when the first url return 4** (on valid credentials) that the other url then return a 200? |
Adding a couple of print()s more:
|
I can't easily replicate the logout problem, so we just need to wait until it happens. |
I ment, in the issue, for some reason the server return 400 and breaks the try-loop, returning a LoginError as a result. With this change, the other url will be tested and we are expecting it to return a 200 result. So far I did not see proof of that. |
You can make these changes to your local vsure package without having to promote this to pypi. |
Yeah. Sorry. I just realized what you ment. I patched session.py in my HA container, adding some more debug info. Will see how it goes next time the problem appears. |
After digging through some logs here, it seems like there are two different 401-messages. If I intentionally use a bad username/password, I get this: But it seems like the "Cookie Expired" message(?) is misleadingly described as:
I am still waiting for the issue to show up again. It might be up to 48 hours or so before that happens... |
We've learned that at least... I've also made the same change in my dev environment and will report the response when I see the issue reappear. |
I got one: Lots of "Finished fetching... success: True", and then:
But then I tried something else. That did first give me another error - Session has expired - but on second try, it gave the same error as above:
I will try to make some logic that might be able to handle this. |
Tested the latest changes, and if I log in to verisure and change my password, HomeAssistant also asks me to log in again.
Now we wait to see what happens next time the timeout occurs. (I don't really like this text-matching but it is currently the best I have). |
So if the first url returns 401, so does the second. Yes the message is different, but the result is the same. That makes this pr irrelevant. I think the issue happens because we don't "trust" the device when we login. So after some time we loose trust between us and the server, requiring a new authentication to take place. |
The point is that there are different 401s and they should be treated differently. At least for now, it looks like "Session is expired" and "Invalid username/password/authentication method combination" should raise a "LoginError" (these indicate that the username/password is wrong) whereas "Username/password does not match any valid login" probably should raise a "ResponseError" as we can mos likely log in again with the same username and password (waiting for verification of that). There might be others, but those are the three different I have seen so far. |
I'll try to figure out how we can 'trust' the device when we login. If you're interested to try for yourself, install mitmproxy on you machine and install it's certificate on your phone. Set the up of your machine as proxy in your WiFi settings on your phone. Now you should be able to view the traffic. |
Trusted device was implemented in version 1 of python Verisure. Don't know if it helps. I guess it's quite a quick fix and I think you mighty be on to something here. |
That only seems to be related to mfa. I have disabled mfa for the account that I use in HA, so I am not sure trusting a device is possible then? |
Let's find out, trust is similar to |
I think now that maybe that is the way to go. If "trust" can be used also without MFA. What is needed is either the "trust" fix, OR a way to make HA retry a login with the same username/password after a certain 401. |
@Olen and everyone who reads this Can you please assist in testing #172 on home-assistant/core#98258 To do so, you have to:
Optionally, you can add some extra print statements to print(f"{response.request.url} {response.status_code} {response.text}") If the changes work as intended, you should see a request to |
But does it need to be related to mfa? |
Good question, I'll try to grab some traffic with MFA disabled. |
The change to the integration could still fix it for you. As the integration now attempts to reauthenticate on 4** responses. |
last_exception = ResponseError(response.status_code, response.text) | ||
self._base_urls.reverse() | ||
continue | ||
if response.status_code >= 400: | ||
last_exception = LoginError(response.text) | ||
break | ||
_LOGGER.warning(f'Error from {base_url}: {response.status_code} - {response.text}') |
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.
Logging here is not adding any real value, as there will be an error anyway
if response.status_code == 200: | ||
_LOGGER.debug(f'OK from {base_url}: {response.status_code}') |
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.
Logging here is not adding any real value
if "Session has expired" in response.text: | ||
raise LoginError(response.text, response.status_code) | ||
elif "Username/password does not match any valid login" in response.text: | ||
raise ResponseError(response.status_code, response.text) |
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.
4** indicates an authorization error. If we raise ResponseError here, we are not able to respond properly.
@@ -102,20 +108,32 @@ def wrapper(url, *args, **kwargs): | |||
try: | |||
response = function(base_url+url, *args, **kwargs) | |||
if response.status_code >= 500: | |||
_LOGGER.warning(f'Error from {base_url}: {response.status_code} - {response.text}') |
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.
Logging a preemptive warning will only spam the logs if the second url is successful
last_exception = RequestError(str(ex)) | ||
_LOGGER.debug(f'Switching active base_url') |
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.
Logging here is not adding any real value
Trying harder to log in on a 4xx Error code.
Also add status code to error message.
Should help fixing home-assistant/core#97885
UNTESTED!