extract token load and save to token manager

[wmalgadey]

Description

Add an abstract token manager interface (TokenManagerInterface) as a base class for new TokenManagers. With this solution, it is possible to create a different mechanism to store the refresh token, without the introduction of a callback method.

This has the advantage of further abstraction and testability, while maintaining a flexible library.

To save, load or pass an external saved token, I created the FileTokenManager. This will be used as a default for token_manager and mimics the current functionality of passing a refresh token via init-parameter and/or saving and loading the refresh token to a file.

The Tado class in interface.py has been modified to pass a new class implementing the TokenManagerInterface. For instance the DeviceTokenManager. You cannot provide both a token_manager and token_file_path or saved_refresh_token.

The DeviceTokenManager is a new implementation, which (currently) cannot be configured from the outside. I did this because I wanted to make sure that, when you use it, the loading and saving will be done to a central file with proper locking for loading and saving. It didn't make sense to me, if you use the DeviceTokenManager that you would like to save to different files in the same "automation"-environment (and create different device ids!). That's what this class tries to solve!

from multiprocessing import Process, current_process
import time

from PyTado.interface.interface import Tado
from PyTado.token_manager.device_token_manager import DeviceTokenManager

def create_tado(token_manager, wait=1):
    print(f"[{current_process().pid}] Waiting for {wait} seconds")

    time.sleep(wait)

    print(f"[{current_process().pid}] Creating Tado instance")

    try:
        tado = Tado(token_manager=token_manager, debug=False)

        print(f"[{current_process().pid}] Device activation status: {tado.device_activation_status()}")
        print(f"[{current_process().pid}] Device verification URL: {tado.device_verification_url()}")

        print(f"[{current_process().pid}] {tado._http._token_manager._sync_file_path}")

        print(f"[{current_process().pid}] Starting device activation")
        tado.device_activation()

        print(f"[{current_process().pid}] Device activation status: {tado.device_activation_status()}")
    except Exception as e:
        print(f"[{current_process().pid}] Error: {e}")


def main():
    print(f"[{current_process().pid}] Main process")

    device1 = DeviceTokenManager()
    device1.save_pending_device_data({})
    device2 = DeviceTokenManager()

    processes = [
        Process(target=create_tado, args=(device1, 0,)),
        Process(target=create_tado, args=(device1, 0,)),
        Process(target=create_tado, args=(device2, 30,))
    ]

    for p in processes:
        p.start()

    for p in processes:
        p.join()

if __name__ == "__main__":
    main()

This example should start 3 Tado-instances in parallel and the first 2 should nearly in the same time present the same URL to authenticate the device. The 3rd instance should not display anything, if you activate the device within 30 seconds. Even if you create different DeviceTokenManager instances, they always use the same file to sync.

ToDos

• [ ] add functionality to use different device token manager for different tado instances in same environment

---

Related Issues

• Closes #177

---

Type of Changes

Mark the type of changes included in this pull request:

• [ ] Bugfix
• [x] New Feature
• [ ] Documentation Update
• [ ] Refactor
• [ ] Other (please specify):

---

Checklist

• [ ] I have tested the changes locally and they work as expected.
• [ ] I have added/updated necessary documentation (if applicable).
• [ ] I have followed the code style and guidelines of the project.
• [ ] I have searched for and linked any related issues.

---

Additional Notes

Add any additional comments, screenshots, or context for the reviewer(s).

---

Thank you for your contribution to PyTado! 🎉

[codecov[bot]]

Codecov Report

:x: Patch coverage is 75.47170% with 65 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 64.97%. Comparing base (c08a556) to head (875df73). :white_check_mark: All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
PyTado/token_manager/device_token_manager.py	63.21%	28 Missing and 4 partials :warning:
PyTado/http.py	77.61%	13 Missing and 2 partials :warning:
PyTado/token_manager/file_token_manager.py	74.00%	9 Missing and 4 partials :warning:
PyTado/interface/interface.py	33.33%	1 Missing and 1 partial :warning:
PyTado/token_manager/token_manager_interface.py	90.47%	2 Missing :warning:
PyTado/token_manager/dummy_device_manager.py	93.33%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #179      +/-   ##
==========================================
+ Coverage   63.41%   64.97%   +1.55%     
==========================================
  Files          14       20       +6     
  Lines        1170     1359     +189     
  Branches       98      117      +19     
==========================================
+ Hits          742      883     +141     
- Misses        388      430      +42     
- Partials       40       46       +6     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[paulvha]

first the good news : it worked "out of the box" some feedback :

I did have a look at the device_token_manager.py, but that does not seem to be used. I only see that FileTokenManager and TokenManagerInterface are used. The TokenManagerInterface.py looks more like a placeholder.

The FileTokenManager __init__() allows for a "saved_refresh_token (str | None): A refresh token which should be used, no mather what.". Not sure what the usage scenario is, but I expect it to work.

In http.py, the comment lines 155- 163 needs update to reflect the new situation.

I'll start spending time on the device_token_manager.py approach.

[paulvha]

update on device_token_manager.py. on the init() : Don't see the options for saved_refresh_token (str | None): A refresh token which should be used, no matter what." But as mentioned before I don't understand the user scenario.

On saving and loading different locks : what happens is LOCK_SH is busy loading and LOCK_EX is requested by saving ?

I was not able to find out how to pass a different token_manager. I only got it work with hard coding

load_pending_device_data() in combination with has_pending_device_data() will cause exception if NO data, but should return false or empty

change from
        if not data or "pending_device" not in data:
            raise TadoException("No pending device data found")
change TO    
        if not data or "pending_device" not in data:
            return {}

In case of a single household there is not an issue, but what it the same PyTado-instance is used for different houses. each of them have a different user/login. now there could be a race condition where a authorisation is pending for house-A, now a house-B want to login with actually a different a user/passwd, but it gets the device-ID for house-A ? So what happens if different a users/logins are used on the same PyTado instance ?

[wmalgadey]

I did have a look at the device_token_manager.py, but that does not seem to be used. I only see that FileTokenManager and TokenManagerInterface are used. The TokenManagerInterface.py looks more like a placeholder.

The TokenManagerInterface is an abstract base class, to show which methods are needed to implement a token manager. I did this with FileTokenManager and (as an example) with DeviceTokenManger.

The FileTokenManager should do the same job as bevor (save token to file, or pass an external saved token via parameter).

The DeviceTokenManger is not added as an option currently, because I did expect, that the calling application, will create TokenManagerInterface-implementation or use my DeviceTokenManager and pass it to the Tado class as an __init__-Parameter.

So one could either use our implementations ([Device|File]TokenManager) or create an own implementation and pass it to Tado(token_manager=DeviceTokenManager()). If you do pass a token_manager to Tado(..) you cannot pass a token_file_path or saved_refresh_token, because Tado() doesn't know what to do with it!

I will describe the use case in the description and create an example.

[wmalgadey]

Just so you know, I created a testcase, but I need to work on it a bit more, is is not working currently

[paulvha]

Thanks Wolfgang. I start to understand what you do. In my application I added : from PyTado.token_manager import DeviceTokenManager

changed tado_instance = Tado(token_manager = DeviceTokenManager())

EDIT : I just noticed the change you made in has_pending_device_data() to use _load_sync_file(). That worked for me as well.

[wmalgadey]

@paulvha I did fix all tests and made some modifications to device token manager. It now works in my test scenario, which I updated in the description.

@erwindouna do mind having a look at this PR?

[paulvha]

I have just tried it, but it does not work for me. First feedback

Setting OAUTH_DATA_KEY = "oauth_data" is causing all the previous stored refresh-tokens to fail first time as these are stored with "refresh_token" and as such can not be found.

Then in load_token() only the refresh_token it self is passed on, but _set_oauth_data(), expects the "refresh_token" header is part of the oauth_data that is passed on.

[paulvha]

I could not login but after sometime the login prompt showed : Why is there a time.sleep(50) in _login_device_flow()?

[paulvha]

Still I could not login. In the function _check_device_activation(), it fails the test

        if not self._token_manager.has_pending_device_data():
            self._token_manager.save_pending_device_data({})
            raise TadoException("User took too long to enter key")

In the _tokenmanager interface.py this always returns false and thus always fails

[erwindouna]

@paulvha I did fix all tests and made some modifications to device token manager. It now works in my test scenario, which I updated in the description.

@erwindouna do mind having a look at this PR?

Sure in a bit. I've been rather occupied with Home Assistant to get the needed fix in and deliver support. :)

[wmalgadey]

@paulvha

I have just tried it, but it does not work for me. First feedback

Setting OAUTH_DATA_KEY = "oauth_data" is causing all the previous stored refresh-tokens to fail first time as these are stored with "refresh_token" and as such can not be found.

Ok, missed that. I will convert old format to new style!

Then in load_token() only the refresh_token it self is passed on, but _set_oauth_data(), expects the "refresh_token" header is part of the oauth_data that is passed on.

Yes, that is the idea. We only need the refresh_token in http.py, but handle the full response inside the token_manager implementation.

I could not login but after sometime the login prompt showed : Why is there a time.sleep(50) in _login_device_flow()?

Sorry, Debug-Code. Removed it!

In the _tokenmanager interface.py this always returns false and thus always fails

I don't know why I missed that!!! I'll fix it now.

[wmalgadey]

@paulvha I did change the implementation. I did mix too much functionality and separated it now into a token_manager and device_manager functionality. I also added backward compatibility to the token_manager, so it loads a previous saved single refresh token

[paulvha]

I had the first look to the updated code this afternoon, and found a couple of issues and it failed (issue 3 below):

1. from file_token_manger.py, .init()

doing:

        if saved_refresh_token:
            self._set_oauth_data({"refresh_token": saved_refresh_token})

will not work correctly as _set_oauth_data(), in token_manager_interface.py, will also look for "expires_in", which not passed.

2. from file_token_manager, _load_oauth_data()

doing

    if FileContent.REFRESH_TOKEN in data:
        self._set_oauth_data({"refresh_token": data[FileContent.REFRESH_TOKEN]})

will not work correctly as _set_oauth_data(), in token_manager_interface.py, will also look for "expires_in", which not passed.

3. def _check_device_activation(self) -> bool:
    if self.device_activation_status == DeviceActivationStatus.COMPLETED:
        return True
   

doing :

        if not self._device_manager.has_pending_device_data():
            self._device_manager.set_pending_device_data({})
            raise TadoException("User took too long to enter key")

fails as the DummyDeviceManager.py. has_pending_device_data() returns false

I have commented the check out and then it reads the "old" refresh- and writes the new oauth-content to the file and works.

4. in has_valid_refresh_token() it calls for get_token(). The returned information is not used.. BUT it is causing the _load_oauth_data() to be called with every request() / has_valid_refresh_token() Thus a big overhead

5. I am a bit lost on all the super().xxx calls. But that is my short coming in understanding the real need.

[paulvha]

I wonder whether the correct "expires_in" is used for the access-code. This code is only valid for 10 minutes after it was provided by Tado.

When http() is called from init() it will try to load the saved information with self._token_manager.get_token(). Get_token() will call for _load_oauth_data(). When the data is loaded from the file, it will check for FileContent.REFRESH_TOKEN being in data. If not it expects that FileContent.OAUTH_DATA was loaded (not checked !)

Now the FileContent.OAUTH_DATA is send to _set_oauth_data() where "expires in" is taken from the earlier saved data and used to determine "_refresh_at". BUT how long ago was this access token and "expires in" saved in the file? That might be longer than the 600 seconds / 10 minutes and as such the "_refresh_at" will be set to an incorrect moment in the future.

At the first start there should ALWAYS be an updated triggered to obtain the new access code and to set the "_refresh_at" moment. Hence I think that saving the refresh-code only, as was done before, is enough.

[wmalgadey]

@paulvha you might also comment directly in the code, that way you can presicely point to the questionable code.

Now the FileContent.OAUTH_DATA is send to _set_oauth_data() where "expires in" is taken from the earlier saved data and used to determine "_refresh_at". BUT how long ago was this access token and "expires in" saved in the file? That might be longer than the 600 seconds / 10 minutes and as such the "_refresh_at" will be set to an incorrect moment in the future.

Ohh I missed that! expires_in is just a timespan, not a valid date :/

At the first start there should ALWAYS be an updated triggered to obtain the new access code > and to set the "_refresh_at" moment. Hence I think that saving the refresh-code only, as was done before, is enough.

Yes you are right, I tried to make it simpler by just sending the data to the "manager" and it should do whatever it has to. And for the has_valid_refresh_token-method I need the expiration. But saving an invalid information is totally false, so I will change that.

The change to the expiration should make your 1. and 2. obsolete.

I will take a look into 3. and 4. asap.

For 5. my main language is C# and I use abstraction, interfaces and polymorphism a lot 😄 , so I brought it over to python. Maybe it is a bit too much, I will check if I can simply it.

[paulvha]

@paulvha you might also comment directly in the code, that way you can presicely point to the questionable code.

Now the FileContent.OAUTH_DATA is send to _set_oauth_data() where "expires in" is taken from the earlier saved data and used to determine "_refresh_at". BUT how long ago was this access token and "expires in" saved in the file? That might be longer than the 600 seconds / 10 minutes and as such the "_refresh_at" will be set to an incorrect moment in the future.

Ohh I missed that! expires_in is just a timespan, not a valid date :/

At the first start there should ALWAYS be an updated triggered to obtain the new access code > and to set the "_refresh_at" moment. Hence I think that saving the refresh-code only, as was done before, is enough.

Yes you are right, I tried to make it simpler by just sending the data to the "manager" and it should do whatever it has to. And for the has_valid_refresh_token-method I need the expiration. But saving an invalid information is totally false, so I will change that.

The change to the expiration should make your 1. and 2. obsolete.

I will take a look into 3. and 4. asap.

For 5. my main language is C# and I use abstraction, interfaces and polymorphism a lot 😄 , so I brought it over to python. Maybe it is a bit too much, I will check if I can simply it.

@wmalgadey : you might also comment directly in the code, that way you can presicely point to the questionable code.

one person making changes in the code is enough :-).. But this one was a bit harder to explain indeed ...