persandstrom / python-verisure

A python module for reading and changing status of verisure devices through verisure app api.
MIT License
134 stars 42 forks source link

Raise on login error, after all attempts are exhausted #129

Closed frenck closed 3 years ago

frenck commented 3 years ago

In PR #125, automatic login with multiple login attempts was introduced. However, due to this regression, any error raised is suppressed.

This causes applications using this library not to be able to act on authentication failures.

This PR fixes this behavior by:

Originally needed for https://github.com/home-assistant/core/pull/47880, which re-factors the Verisure integration for Home Assistant.

ptz0n commented 3 years ago

Any response above HTTP 4XX should be retried on the other host, otherwise raise.

frenck commented 3 years ago

Actually @ptz0n, I would say that is incorrect.

For example, error code AUT_00008 means blacklisted and re-trying has no use at all (and can have negative side effects). Still; AUT_00008 is packed in an HTTP 401 error.

Similar effects for an invalid username/password combination.

However, this PR is not here to improve on that at all. It keeps the current logic for your concern (and I've considered it out of scope as well). This PR is merely scoped at preventing full error suppression.

ptz0n commented 3 years ago

I believe that 401 is not above 4XX. Which means that AUT_00008 should be raised, not retried. This is what the node-verisure retry on different host logic looks like:

if (error.response && error.response.status > 499 && !retrying) {
  return this.makeRequest(options, true);
}

throw error;
frenck commented 3 years ago

Ah! 👍

I interpreted it as >400, ok, so we are on the same page there 👍

persandstrom commented 3 years ago

Thanks @frenck! I'll merge this and do a release so you can get on with HA. Good point @ptz0n, I'll take a look at that.