search

mezz64 / pyEight

Python library to interface with the Eight Sleep API
MIT License
59 stars 15 forks source link

Allow auth data to be passed in to client constructor #29

Closed raman325 closed 2 years ago

raman325 commented 2 years ago

While working on https://github.com/home-assistant/core/pull/71095 I learned that if you request a token twice in a row in quick succession, you will get rate limited. As a result we have to sleep for five seconds so we can authenticate twice. If we allow the data we retrieve on the first request to be passed in to the client constructor though, we can avoid the second call and don't have to sleep.

mezz64 commented 2 years ago

So this is needed because the config flow has to validate auth during setup right? Then once it's actually configured it immediately runs the start routine leading to the double auth?

The only thing i'm not sure about is whether or not it makes more sense to handle this through the session, but i'm admittedly not that familiar with the hass config flow setup. Can we not carry the session over from the config flow setup and add some checks to validate an incoming session to see if we need to fetch a new token or not?

raman325 commented 2 years ago

So this is needed because the config flow has to validate auth during setup right? Then once it's actually configured it immediately runs the start routine leading to the double auth?

That's correct.

The only thing i'm not sure about is whether or not it makes more sense to handle this through the session, but i'm admittedly not that familiar with the hass config flow setup. Can we not carry the session over from the config flow setup and add some checks to validate an incoming session to see if we need to fetch a new token or not?

Hm, so we can carry the session over (in fact I think we do with the current logic in the PR), but this token is not stored in the session, it's stored in the library as of now. We could abuse the session's cookie jar to do this maybe but that doesn't seem like a good approach. And HA sets the default headers so we can't leverage that either.... I will have to think about this more

mezz64 commented 2 years ago

My original thought was that during the config flow setup the session would end up with updated headers with the token included, but thinking more that won't be the case. We currently don't update the headers as part of the token fetch action.

We could make that modification, but it does start to seem a little hacky and maybe not the best approach since it relies heavily on an external application not modifying the session to transfer the information.

mezz64 commented 2 years ago

I ultimately couldn't think of a better way to handle this and don't see any issues with your implementation.