Refactoring for new Auth

[danielcherubini]

Hi,

This is a little bit of a refactor for the new Auth. I have it working locally.

I will be happy to make any changes.

Todo

• [x] Clean up auth.py
• [x] Rename auth.py to panasonic_auth.py
• [x] Rework error handling to be more explicit on authentication token expiration handling
• [x] Centralize that error handling if i can
• [x] auth.login() should only be called if the clientId isn't known. So check for that, before calling it.

[danielcherubini]

@little-quokka I like your implementation of panasonic_session.py

[heldchen]

thanks, some notes on the code:

• auth.login should only be called if the X-Client-Id is not yet known, and after that never again. currently it's called on each startup, which differs from how the app works. instead, _acc_client_id should be a stored persistent value tied to the user & login combination and /auth/v2/login only called if the credentials changed or the _acc_client_id is not yet known.

• auth.py still has a lot of debug/testing prints that probbably should be removed before it's merged

• the bearer token expiration handling seems not yet correct: on every api call the bearer token could now be expired. in that case, a token refresh must be done and the call retried. a simple api call wraper function could transparently handle that for all of the accsmart requests. in the current version of the code, only session.login() has some error handling that refreshes the token, but does not check if the error is actually due to an expired Bearer token. the api response will explicitly tell you that the token expired.

[danielcherubini]

agreed @heldchen, I'm currently working on redoing auth.py to clean it up. I also want to introduce a new error handling so that if at all there is a expiration for the token, we can handle that more directly, separating any general error handling from any unauthorized handling.

[danielcherubini]

@heldchen @little-quokka take a look at the changes to panasonic_auth.py

[heldchen]

this looks already very clean. I'd recommend to make the following changes

• if possible add a boolean parameter include_client_id to _get_new_token, then the function can also be used in _get_new_token - which makes future changes easier
• create a class constant for the X-APP-VERSION version id (f.e. PanasonicSession.APP_VERSION - this will definitely change when panasonic updates their android app
• its generally recommended to name the file the same as the class it contains

if you want to go a bit further, if this was my project I'd abstract the api logic from the rest of the tool's logic even further by doing the following changes:

• name the file "apiclient" instead
• for each api endpoint of accsmart.panasonic.com that is used by the library, create a function in the client
• this allows the separation between api functions of the library, which will make future api changes easier
• the function login doesn't just log-in the user. I would move this code to the class constructor __init__(self, username, password, tokenfFileName) and run it automatically when the class is instanciated.

[little-quokka]

Hi @heldchen, @danielcherubini. Sorry for dropping off yesterday without much warning, but I needed some sleep. Please let me know what I can help with. Unfortunately, I will be less free today than the last two days, but let's see what is possible.

[little-quokka]

I addressed a few of the issues raised above here: https://github.com/little-quokka/python-panasonic-comfort-cloud

Specifically:

• Constant for X-APP-VERSION
• Rename files (class name = file name)
• Have ComfortCloudSession be a subclass of PanasonicSession. ComfortCloudSession does all the API calls, PanasonicSession all the token handling
• Ensure that before any API call is done, the token is checked for its validity and potentially updated first
• Move --raw true message output into a single function

(The branch is based on a 1:1 copy the work from @danielcherubini, but somehow I screwed up some merging and it doesn't show as such. )

[little-quokka]

Some additional updates (https://github.com/little-quokka/python-panasonic-comfort-cloud):

• Improved error handling
• Crack open token to check expiry
• Move exceptions into separate file
• Show response.text on error (previously omitted, was in originally)
• Change ComfortCloudSession to ApiClient
• Update default token file name to allow this to be run on windows as well as linux without '-t' parameter

[little-quokka]

Some additional updates (https://github.com/little-quokka/python-panasonic-comfort-cloud):

• run pylint and fix (important) stuff reported

That is it for me now. I think most of the issues listed above were attended to. The only one I am unsure about is this one from @heldchen:

if possible add a boolean parameter include_client_id to _get_new_token, then the function can also be used in _get_new_token - which makes future changes easier

I guess the 2nd _get_new_token is another function, just unsure which one.

[heldchen]

FYI: the exact code & message received when the token expires is:

• http status code 401
• body: {"code":"4100", "message": "Token expires"}

[heldchen]

Some additional updates (https://github.com/little-quokka/python-panasonic-comfort-cloud):

• run pylint and fix (important) stuff reported

That is it for me now. I think most of the issues listed above were attended to. The only one I am unsure about is this one from @heldchen:

if possible add a boolean parameter include_client_id to _get_new_token, then the function can also be used in _get_new_token - which makes future changes easier

I guess the 2nd _get_new_token is another function, just unsure which one.

sorry, I was on my mobile and couldn't be more specific. what I meant is to update the header function so that it can also be used for this step:

        # ------------------------------------------------------------------
        # RETRIEVE ACC_CLIENT_ID
        # ------------------------------------------------------------------
        now = datetime.datetime.now()
        timestamp = now.strftime("%Y-%m-%d %H:%M:%S")
        response = requests.post(
            'https://accsmart.panasonic.com/auth/v2/login',
            headers={
                "Content-Type": "application/json;charset=utf-8",
                "User-Agent": "G-RAC",
                "X-APP-NAME": "Comfort Cloud",
                "X-APP-TIMESTAMP": timestamp,
                "X-APP-TYPE": "1",
                "X-APP-VERSION": "1.20.0",
                "X-CFC-API-KEY": generate_random_string_hex(128),
                "X-User-Authorization-V2": "Bearer " + token_response["access_token"]
            },
            json={
                "language": 0
            })
        check_response(response, 'get_acc_client_id', 200)

-->

        # ------------------------------------------------------------------
        # RETRIEVE ACC_CLIENT_ID
        # ------------------------------------------------------------------
        now = datetime.datetime.now()
        timestamp = now.strftime("%Y-%m-%d %H:%M:%S")
        response = requests.post(
            'https://accsmart.panasonic.com/auth/v2/login',
            headers= self.get_header_for_api_calls(True),
            json={
                "language": 0
            })
        check_response(response, 'get_acc_client_id', 200)

there True denotes the flag that the X-Client-Id header should not be added even if the value would be known (in case the user changed the username & password)

[little-quokka]

I understand now @heldchen. Unfortunately, there is the issue that the access_token is not in available in self._token[] at this point in time (but only in token_response[]). self._token[] is only filled after all steps were successful to avoid a half-correct state.

[little-quokka]

How to move forward from here @danielcherubini? Should I open a pull request to your repo? You already have this pull request open to the original repo. Is that the way it works?

[danielcherubini]

How to move forward from here @danielcherubini? Should I open a pull request to your repo? You already have this pull request open to the original repo. Is that the way it works?

@little-quokka yes, open a PR to my repo, and I'll get those changes in tonight, then that updates this PR.

[danielcherubini]

This is out of scope for this PR. But @lostfields I've been thinking, since the Home Assistant Integration so heavily uses this CLI app, but only the core of it... what might be smart, is if we separate the core/common functionality out of this, and put it into it's own repository, then both places can PIP install that as a package.

What do you think?

[little-quokka]

I think we can keep it in one repository @danielcherubini. I would simply move the contents of main.py into pcomfortcloud.py and remove the entry_point statement in setup.py. Then the pcomfortcloud folder is a library, and the pcomfortcloud.py outside of this folder a CLI/test program which will be useful for testing any changes to the library.

[little-quokka]

This should do it: https://github.com/little-quokka/python-panasonic-comfort-cloud/commit/c3f39d19c0808e21d7e7ee79e72fac41ffe59426

If used as a library: The contents of a loaded json token can be put into the PanasonicSession if available. If a token is provided but out of date, the token will be refreshed. If no token is provided, a new token will be created. The current token can be retrieved via get_token(). This should be done ideally after start_session() as the token will then be available and up-to-date.

If used with CLI: CLI tool is handling token file (default: token.json).

[danielcherubini]

@little-quokka Brilliant!!!!! I look forward to your PR

[little-quokka]

I think that is the PR (this is only the 2nd time I do this lately and it seems I screwed it up the first time, so better check): https://github.com/danielcherubini/python-panasonic-comfort-cloud/pull/1

[danielcherubini]

@lostfields We think this is ready for merging now. There's been a big refactoring of this.

The CLI functionality has been moved into the pycomfortcloud.py file, and now the pcomfortcloud folder is a module that can be reused by both the CLI application and also used as a module for the HA Integration

@craibo You're going to like this... So as said above, you can now treat this as a library.. I'm positive you might be able to get it working with pip install and then you won't need to copy the folder into yours every time! Cutting down the complexity.

[danielcherubini]

HUUUUUUUGE thanks to @heldchen @little-quokka @lostfields @craibo and everyone who's helped get this to this place!

[craibo]

Thanks @danielcherubini @little-quokka Just took a look and those changes will make it way easier to use in other projects👍🏻

[danielcherubini]

@heldchen sorry for not including you in the thanks. But without you none of this would have been possible

[heldchen]

no worries, glad I could be of help and thanks everyone on getting this into the integration plugin so fast! this is my first summer with a HVAC and it would have sucked to have to use the Comfort Cloud app :-)

[lawrencedudley]

Just wanted to say you're all wonderful! Thank you so, so much. My staff in the office can stop complaining about the AC once this is merged and into the HA integration.

[danielcherubini]

@heldchen I just bought a second Panasonic AC, so now it's more important than ever for me too.

If Panasonic changes something again (and I believe they will) then we now have made the changes to get things moving fast. The amount of seperation of code here, and the new people who have eyes on this will mean any of us can jump in.

I really do believe Panasonic will do something. I know what they will do, but I don't wanna put it into the universe.

[danielcherubini]

@lawrencedudley my suggestion is to switch to @craibo 's fork. I don't know what the main forks maintainer will do. But I think going forward the smartest thing to do is switch

[heldchen]

@danielcherubini I'd miss my climate mini dashboard:

[danielcherubini]

@heldchen I'm not understanding, are you saying something is wrong? or are you saying you miss having your dashboard and can't wait to get it working again?

[danielcherubini]

@little-quokka i changed your constant for app version to go get the latest version from the apple app store, the same way that @sockless-coding had it.