Fix in case of refresh token error

[tmenguy]

fixes #50

[cliviu74]

Hey @tmenguy , if you don't mind I'll split this PR in 2 as there are a lot of changes there and difficult to see which is which

• fix for requestTimeout - that's a big bug, great catch there!
• change to the authentication workflow, re-authenticate if jwtToken needs refresh

[tmenguy]

as you want no pb, you want me to do it? Lot of changes , yes a lot of commits, but see the final diff , no so much :)

[cliviu74]

@tmenguy can you take a quick peek at https://github.com/cliviu74/wallbox/pull/51/commits

This is the timeout issue fixed as you suggested.

I'll split the jwtToken in a few minutes

[tmenguy]

great, thanks a lot :)

[tmenguy]

actually if you merge your #51 I rebase and this one will be the token

[tmenguy]

so you don't have to create a new one

[tmenguy]

Can we move past this :) Will add ecosmart mode handling support right after

[tmenguy]

thx for the wrongly added files :) cleaned it with bfg, do you think we can merge this one now?

[tmenguy]

an up to merge it ?

[cliviu74]

Left a few comments on it. As it looks at the moment, I don't see the need for a retry authentication based on http code return. The way I see it, if authentication fails, it should return with an error rather than retry. There is a change of retry storms if upstream api has a temporary issue.

[tmenguy]

Hum even in case of error storm there will be only one re-auth to clean the tokens and get new ones (the from refresh Boolean is here for that) Will look at your comments.

[tmenguy]

And actually it is not a retry : simply giving back an error is not enough : you need to reset the tokens in case of this error, so I force a re-auth with retry, else the only way is to kill the whole Wallbox object. The code fixes the fact that jwtRefreshToken is expired or not valid for any reason. (by the way I don't see your comments on the PR)

[cliviu74]

You don't need to destroy the object, you can simply reauthenticate. If you want to force a token reissue, you can just wipe the token and reauthenticate (see code below)

from wallbox import Wallbox, Statuses
from dotenv import load_dotenv
import os
import hashlib

if "WALLBOX_USER" not in os.environ:
  load_dotenv()

wallboxUsername = os.getenv("WALLBOX_USER")
wallboxPassword = os.getenv("WALLBOX_PASS")

def caculateHash256(hashedString):
  hashedString = hashedString.encode()
  return hashlib.sha256(hashedString).hexdigest()

w = Wallbox(wallboxUsername, wallboxPassword)

# Authenticate with the credentials above
w.authenticate()

# Print a list of chargers in the account
print(f"Chargers under the account {wallboxUsername}: {w.getChargersList()}")

oldToken = caculateHash256(w.jwtToken)

w.jwtToken  = ""

w.authenticate()
newToken = caculateHash256(w.jwtToken)

print(f"Old hashed token: {oldToken}\nNew hashed token: {newToken}")

python3 example-token-reissue.py                                                                                         
Chargers under the account *********: [*******]
Old hashed token: 6fcba25979030f1cabb609f0ddbd9eaa13818f2335e36e0e53eb7eb6097047ba
New hashed token: cf8fbf6bacbaaa2be053c14712d41f1c8117d8b8d8a9c37a5c3000b7606754c4

Is there anything I'm missing here?

[tmenguy]

Yes, but you need to do : w.jwtToken = "" which requires a good understanding of the library intricacies, a token should be a hidden variable nobody outside the library should access: what I was proposing was doing it automatically only when needed. It is more of a design philosophy here : does your library should be fixing the issues/specificities of the system you abstract with it. As an example the Wallbox APIs needs a timeout by default as they sometimes miss-behave, this is not common knowledge, hence having a good default timeout tuned for those issues would make more sense. Here it is the same : knowing that sometimes the refresh of the refresh token need to be done is not common knowledge, hence I do think it should be abstracted by the library : rule of thumb here if every single use of a library need the exact same code wrapper, or outside fixing, it should be made as default in the library :) ... but again a design choice, I personally prefer to no put the burden of fixing what I'm abstracting to every single user of the abstraction, but again, no pb it is your library. But as an example the Homeassitant integration will need already 2 changes : one for the timeout, one for the token refresh, where a fix in the library would make the life better of all users of the Wallbox library without any hassle.

Tell me what you prefer, no worries, I'll simply maintain my fork and make it evolves (or make a radically different async version :) ).

[cliviu74]

As raised earlier, my concern is around the handling of a refresh issued on every http error. A better implementation needs to be considered to ensure the token is refreshed only when it's not valid, not on every possible error as it's proposed by this change. We are running at risk of possible locking people's wallbox account by causing a retry storm, under the current implementation when the upstream api will error out with anything else than a 401 http code.

Coming back to the token implementation, if the token is no longer valid before it's expired, it needs to be handled upstream, or at least it needs to be handled better by the authenticate method. I just don't see how your change is fixing this by issuing a continuous authentication if there is a http error returned by the upstream api. Again, this is an interface library, not an application in itself.

I would also like to address here that, while I appreciate your contributions which have been extremely valuable, I don't appreciate the suggestion that you can fork the library because you disagree with the design choice. Of course you are welcomed to maintain a fork if you feel this design fundamentally contradicts your views on how an interface library should operate, you don't need my permission to do so. I do hope though we can collaborate to maintain a useful and safe library for people to use. I must stress out that this is an UNOFFICIAL library that using an upstream commercial API and we aresponsibility not to cause any harm to the users of this library as well as ensuring we're not abusing Wallbox service (i.e. by causing a retry storm). That's why I am taking my role of vetting the changes and asking questions until I make sure we are doing this to the best of our abilities. I hope you appreciate this. I am also looking for active maintainers of this, people who can vet and merge changes, if you'd like to become one we can have a discussion.

[tmenguy]

Hi,

As for the error issue: it will throw an exception after the first call if it fails, it is not an infinite loop we have here, but yes it would do 2 call instead of one at each error, I'll commit one that wil reset the token explicitly

" I don't appreciate the suggestion that you can fork the library because you disagree with the design choice" : slight misunderstanding here, written language is messy (esp for a non native speaker like me) ... would be for my own use, no real point here to compete or anything, just having my fork for my needs.

And I appreciate the discussion we have anyway, and yes good that you take seriously your role here, its why I don't want to push too hard for changes you are not confortable with.

[tmenguy]

just pushed a change (virtually equivalent, but potentially one less API call in case a user of the library catch an auth error an retries to call it)

[tmenguy]

Hopefully this time we are ok with the latest changes :)

[tmenguy]

up

[tmenguy]

Hi probably last time I’m asking, I changed the PR to be pretty close to the the reset you were mentioning, could you just say yes or no here please so I can move on?

[tmenguy]

Up

[tmenguy]

Hi @cliviu74 , is there an issue on your side? (or a long vacation!) … after a month of silence time to ask again…

[tmenguy]

up