Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds OAuth (Bosch SingleKey ID support) #173

Merged
merged 16 commits into from
Apr 26, 2023
Merged

Conversation

sander1988
Copy link
Owner

  • Adds OAuth (Bosch SingleKey ID support) New Single-Key Authentification #171
  • Moved configuration move YAML to GUI.
  • Multiple mower are supported ; but the integration should be added multiple times (it checks for a unique mower serial).
  • Expose a mower a device in HA linked to it's entities (instead of only separate entities).
  • Several other fixes.
  • Updated README.

Implemented init using HA config entries.
Added config flow + account credential validation.
Added support for integration reload.
Some restructuring to allow multiple instances of the Indego integration within HA (using different Bosch Indego account).
Fixed reload of the integration.
Added support for multiple mowers in HA indego service calls.
WIP: Config flow.
…elaying the next update.

Improved code style and comments.
Updated readme.
Fixed error in sensor.
@JeedHome44
Copy link

Hello,
i'm french! sorry for my bad english ! i add your files in my HA with the Chrome extension and it's work perfectly :)
Thank you so much !

@LJvHeck
Copy link

LJvHeck commented Mar 26, 2023

It works! Great work.

@jm-73
Copy link
Collaborator

jm-73 commented Apr 1, 2023

I have created a new branch for the Bosch Single Key authentication code. I really appreciate that you have made a new pull request to the new branch. In that way I can commit the code there and test the code. Then merge the (hopefully) working code with the main branch.
Makes sense?

@jm-73
Copy link
Collaborator

jm-73 commented Apr 1, 2023

Another thing: before merging into master, someone have to clean the code and figure out the dependencies. As you have written the code now you have added another dependency to your own github repo. In my opinion the dependencies that this integration should have is pyIndego where we have coded all the logic to set up a session and comunicate with the bosch servers.

@sander1988
Copy link
Owner Author

sander1988 commented Apr 1, 2023

I have created a new branch for the Bosch Single Key authentication code. I really appreciate that you have made a new pull request to the new branch. In that way I can commit the code there and test the code. Then merge the (hopefully) working code with the main branch.
Makes sense?

Yes that's fine. Note that your message crossed a commit I just did. I did some small changes in the pyIndego library and this HA component to detect certain scenarios where the Bosch servers are down. In that case we delay the next update a bit to prevent loops. Let first test this version 1 or 2 weeks as I just made this change.

@sander1988
Copy link
Owner Author

Another thing: before merging into master, someone have to clean the code and figure out the dependencies. As you have written the code now you have added another dependency to your own github repo. In my opinion the dependencies that this integration should have is pyIndego where we have coded all the logic to set up a session and comunicate with the bosch servers.

Yes I will change the library repo URL back as soon as pyIndego has been merged. Anything else?

@jm-73
Copy link
Collaborator

jm-73 commented Apr 1, 2023

No, just keep up the good work!!! :-)

@sander1988
Copy link
Owner Author

sander1988 commented Apr 7, 2023

Let first test this version 1 or 2 weeks as I just made this change.

This is gonna take a bit longer unfortunately. My own mower is broken atm and back to Bosch for repairs :-(. I will continue testing when he's back from repairs.

@intovision
Copy link

Any chance we can get access to the branch via HACS? Just makes it easier to try out and switch versions. Thanks for the great work!

@Pr0mises
Copy link

Pr0mises commented Apr 13, 2023

I tested it for a while now, everything is working as expected.

After upgrading to HASS 2023.4.3 the intregration stopped working with this error log:

Error setting up entry Indego (Indego ID) for indego
Traceback (most recent call last):
File "/usr/src/homeassistant/homeassistant/config_entries.py", line 383, in async_setup
result = await component.async_setup_entry(hass, self)
File "/config/custom_components/indego/init.py", line 223, in async_setup_entry
await indego_hub.update_generic_data_and_load_platforms(load_platforms)
File "/config/custom_components/indego/init.py", line 369, in update_generic_data_and_load_platforms
generic_data = await self._update_generic_data()
File "/config/custom_components/indego/init.py", line 616, in _update_generic_data
await self._indego_client.update_generic_data()
File "/usr/local/lib/python3.10/site-packages/pyIndego/indego_async_client.py", line 262, in update_generic_data
self._update_generic_data(await self.get(f"alms/{self.serial}"))
File "/usr/local/lib/python3.10/site-packages/pyIndego/indego_async_client.py", line 543, in get
return await self._request(method=Methods.GET, path=path, timeout=timeout)
File "/usr/local/lib/python3.10/site-packages/pyIndego/indego_async_client.py", line 476, in _request
self._token = await self._token_refresh_method()
File "/config/custom_components/indego/init.py", line 328, in async_token_refresh
await session.async_ensure_token_valid()
File "/usr/src/homeassistant/homeassistant/helpers/config_entry_oauth2_flow.py", line 499, in async_ensure_token_valid
new_token = await self.implementation.async_refresh_token(self.token)
File "/usr/src/homeassistant/homeassistant/helpers/config_entry_oauth2_flow.py", line 92, in async_refresh_token
new_token = await self._async_refresh_token(token)
File "/usr/src/homeassistant/homeassistant/helpers/config_entry_oauth2_flow.py", line 182, in _async_refresh_token
new_token = await self._token_request(
File "/usr/src/homeassistant/homeassistant/helpers/config_entry_oauth2_flow.py", line 209, in _token_request
resp.raise_for_status()
File "/usr/local/lib/python3.10/site-packages/aiohttp/client_reqrep.py", line 1005, in raise_for_status
raise ClientResponseError(
aiohttp.client_exceptions.ClientResponseError: 400, message='Bad Request', url=URL('https://prodindego.b2clogin.com/prodindego.onmicrosoft.com/b2c_1a_signup_signin/oauth2/v2.0/token')

Upgrading to 2023.4.4 same result.
After readding the integration (keep credentials) and using the addon it's working again.
Not sure what happened, I couldn't get gather any other logs.
Looks like HASS deleted the token on my end, therefore no problem with this integration just an information.

I'll check if the next release also deletes the token or not.

@sander1988
Copy link
Owner Author

I tested it for a while now, everything is working as expected.

After upgrading to HASS 2023.4.3 the intregration stopped working with this error log:

@Pr0mises - Are you really sure it started after the upgrade? Did you restart HA before just before upgrade without getting this error?

I see the same error on some of my HA instances. But I have not upgraded. I think it's an issue with the token refresh based on the stack and error.

@Pr0mises
Copy link

@sander1988 no I didn't restart hass just before the upgrade, it has nothing to do with this push but how hass refreshes the token.

I think it's an issue with the token refresh based on the stack and error.

That's what I'm thinking as well. It's no issue at all just wanted to inform you. As you had the same error it's worth mentioning in the readme as an hint, so no new Issue are opened for this behavior as it's known.

@sander1988
Copy link
Owner Author

@sander1988 no I didn't restart hass just before the upgrade, it has nothing to do with this push but how hass refreshes the token.

I think it's an issue with the token refresh based on the stack and error.

That's what I'm thinking as well. It's no issue at all just wanted to inform you. As you had the same error it's worth mentioning in the readme as an hint, so no new Issue are opened for this behavior as it's known.

Ok. I see the 400 Bad Request. I will add some more debug output to the logs to see if the refresh is really happening and monitor it for the next few days.

FYI: I'm referring to this error:

Token request failed with status=400, body={"error":"invalid_grant","error_description":"AADB2C90080: The provided grant has expired. Please re-authenticate and try again. ...

@sander1988
Copy link
Owner Author

I think I see the why... I have not seen this before (to me it looks like a misconfiguration of the OAuth server): Both the refresh and access token use the same expiry of 1 day (expires_in).

HA refreshes around 20 seconds (https://github.com/home-assistant/core/blob/dev/homeassistant/helpers/config_entry_oauth2_flow.py#L42) before expiring. So there is a small window for the (daily) refresh to succeed otherwise you will run into this 400 Bad Request error.

I will try to figure out a way to mark the access token as expired a few hours earlier as a workaround for this issue.

@Pr0mises
Copy link

That would explain why I'm stuck at the start screen of indego connect every now and then. After restart the app I've to login again.

@sander1988
Copy link
Owner Author

That would explain why I'm stuck at the start screen of indego connect every now and then. After restart the app I've to login again.

Yes exactly, I have the same issue with their iOS app.

I think I have workaround for this HA integration... a way to force refresh it every 12 hours. That works fine in this case as long as HomeAssistant isn't offline for more than 12 hours. If you are offline for longer than 12 hours then the user has to readd the integration. I expect that won't happen for most the users. This is the best I can do here as it's a workaround, the real fix has to be implemented by Bosch on their server side. I hope they do, as it also affects their apps.

Have running this workaround in test now. I expect to update the repo within a few days.

@Pr0mises
Copy link

That's awesome, if you need more testing you could upload your fix into another branch so I can check it out.

@jm-73
Copy link
Collaborator

jm-73 commented Apr 20, 2023

Will you please commit to the dev branch instead? I have asked you this before.

@sander1988 sander1988 changed the base branch from master to develop April 21, 2023 17:08
@sander1988
Copy link
Owner Author

Will you please commit to the dev branch instead? I have asked you this before.

I have changed it. Note that I was under the assumption you can change this as I left allow edits from maintainers enabled. Maybe I'm wrong here, I use GitLab for 99% of my projects.

@sander1988
Copy link
Owner Author

sander1988 commented Apr 21, 2023

That's awesome, if you need more testing you could upload your fix into another branch so I can check it out.

Please give me a few more days to fix 2 issues:

  1. I have logs confirming what I expected regarding the refresh misconfiguration by Bosch (which results in the 400 Bad Request after a couple of days). I can implement a workaround now.
  2. I also found another issue where the state update fails and stops updating from that point (you have restart HA or reload the integration to get it working again). I will also fix that. This is caused by:
2023-04-20 03:31:11.384 ERROR (MainThread) [homeassistant] Error doing job: Future exception was never retrieved
Traceback (most recent call last):
  File "/usr/local/lib/python3.10/concurrent/futures/thread.py", line 58, in run
    result = self.fn(*self.args, **self.kwargs)
TypeError: IndegoHub._create_refresh_state_task() takes 1 positional argument but 2 were given

@Pr0mises
Copy link

Please give me a few more days to fix 2 issues:

I already pulled your latest commits and encountered the same issues. If you need any help let me know.

@sander1988
Copy link
Owner Author

I already pulled your latest commits and encountered the same issues.

That's not possible ;-) ... my latest commit is from 3 weeks ago so it doesn't contain the 400 Bad request workaround. But I expect to push something today.

Fix for error on delayed state update.
@sander1988
Copy link
Owner Author

I have pushed version 5.0.2 ; this update should fix both issues. All known issues are fixed now.

In case you are testing... Please temporary add these logging configuration lines to your HA config:

...
logger:
  logs:
    homeassistant.helpers.config_entry_oauth2_flow: debug
    custom_components.indego: debug
    pyIndego.indego_async_client: debug
...

It helps debugging in case we run into another API related error.

Let's monitor the cloud connection for another week.

@LarsLautrup
Copy link

I got everything up and running - and the integration works again... Thanks!!!
Is there a way to get more updates from the Indego? As it is right now, it looks like the states of the enitites is about 10/15 minutes delayed.

@warpbe
Copy link

warpbe commented Apr 25, 2023

I installed this last version, and it works flawlessly

@jm-73 jm-73 merged commit 253a2cb into sander1988:develop Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants