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

[tado] Add OAuth authentication (deadline 2025-03-15) #18354

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

andrewfg
Copy link
Contributor

@andrewfg andrewfg commented Mar 3, 2025

Resolves #18340

Depends on openhab/openhab-core#4632

Signed-off-by: Andrew Fiddian-Green [email protected]

Signed-off-by: Andrew Fiddian-Green <[email protected]>
@andrewfg andrewfg added enhancement An enhancement or new feature for an existing add-on regression Regression that happened during the development of a release. Not shown on final release notes. labels Mar 3, 2025
@andrewfg andrewfg self-assigned this Mar 3, 2025
@andrewfg andrewfg marked this pull request as draft March 3, 2025 18:08
andrewfg added 2 commits March 3, 2025 18:13
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
@jlaur
Copy link
Contributor

jlaur commented Mar 3, 2025

@andrewfg - I didn't look closely into the code, but is there anything that speaks against using the OAuth2 core classes?

@jlaur jlaur removed the regression Regression that happened during the development of a release. Not shown on final release notes. label Mar 3, 2025
@jlaur
Copy link
Contributor

jlaur commented Mar 3, 2025

is there anything that speaks against using the OAuth2 core classes?

Sorry, didn't check the issue until now. I guess ideally RFC-8628 should be implemented as well in core. But I understand a short-term solution is needed. I'm wondering though if the core classes could still be partially used, i.e. having the responsibility of refreshing the token? See for example https://community.openhab.org/t/oauth2-0-device-authorization-grant-device-code/127070. Perhaps other bindings are already doing something similar?

@andrewfg
Copy link
Contributor Author

andrewfg commented Mar 3, 2025

I looked at the core classes and I think it would be a very steep learning curve for me to implement the new workflow. I would prefer either a) someone else do that, or b) use my current solution for the time being, and eventually port it back to core.

@andrewfg
Copy link
Contributor Author

andrewfg commented Mar 3, 2025

See for example https://community.openhab.org/t/oauth2-0-device-authorization-grant-device-code/127070.

PS that does not offer any solution. Especially not to OH core. I am fairly sure that my Tado solution could eventually be taken over into core. But it needs a huge amount of study on the differences between a one binding solution and a possibly generic RFC compliant general solution.

Also this PR is a migration from a legacy to a newer authentication model, and we don’t have the freedom to start from scratch.

@jlaur
Copy link
Contributor

jlaur commented Mar 4, 2025

But it needs a huge amount of study on the differences between a one binding solution and a possibly generic RFC compliant general solution.

I fully understand. But did you consider this:

I'm wondering though if the core classes could still be partially used, i.e. having the responsibility of refreshing the token?

What I meant by this would be to simply use the store for persisting tokens through OAuthClientService. So you would implement your custom flow in the binding (which you already did), but you could then save the access token response, probably by calling importAccessTokenResponse.

The benefits would be:

  • Token refresh will be handled by core implementation which is already quite reliable.
  • Since the standard store is used, tokens will be included in backups (and there won't be issues with file-based Thing configuration).
  • When later (potentially) implementing RFC-8628, this wouldn't be a breaking change for users, since the same store is used.

What is currently missing in core is the method of doing the initial authentication, but everything after that should be supported already.

If you want to go down this path, I can offer my assistance.

Btw, I did something similar in the Bosch Indego binding. I could not use any standard grant flow, so I had to provide a hacky way for obtaining the code. After that I was then able to use the core implementation for storing and maintaining tokens.

@andrewfg andrewfg changed the title [tado] add oauth2 authentication work flow [tado] Add oauth2 authentication work flow Mar 4, 2025
@mherwege
Copy link
Contributor

mherwege commented Mar 4, 2025

Btw, I did something similar in the Bosch Indego binding. I could not use any standard grant flow, so I had to provide a hacky way for obtaining the code. After that I was then able to use the core implementation for storing and maintaining tokens.

Another binding I recently changed to take that approach is the BMW binding. It just uses the store and the token refresh mechanism, not the initial grant flow.

@andrewfg
Copy link
Contributor Author

andrewfg commented Mar 4, 2025

Ok. I will give that a try today.

@andrewfg
Copy link
Contributor Author

andrewfg commented Mar 4, 2025

Ok.

I refactored the code in order to prepare for an easier migration of the "device code grant flow" into OH core. The prior code is split into two parts -- namely the tado specific part that stays in the binding and the generic part that could eventually be migrated into OH Core.

I won't yet move it into core, since I want to test it well.

I was not able to use the oAuth store for persisting the "device code grant flow" results, so for the time being I am persisting that in the thing handler configuration instead.

Signed-off-by: Andrew Fiddian-Green <[email protected]>
@andrewfg
Copy link
Contributor Author

andrewfg commented Mar 5, 2025

Migrated the oAuth workflow into openhab/openhab-core#4632

andrewfg added 5 commits March 5, 2025 17:24
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
@andrewfg
Copy link
Contributor Author

andrewfg commented Mar 9, 2025

@mherwege / @jlaur I have implemented oAuth RFC-8628 in openhab/openhab-core#4632 and tested it on this PR. But I need your help please, concerning Insufficient configured threads: required=10 < max=10 for QueuedThreadPool errors.

Specifically I wrote a class OAuthConnectorRFC8628.java that extends the existing OH core OAuthConnector.java to implement the missing RFC-8628 functionality. I have tested the added code and it works fine. It fetches the device authentication code from the tado server, provides the user authentication url to the servlet (which redirects to that page), polls for the access token from the tado server, and returns the access token (valid 10 minutes) once approved. So the tado thing dutifully goes online for 10 minutes.

Now after the initial token, the binding is supposed to use the EXISTING refreshToken() of the OAuthClientServiceImpl.java class for refreshing it; but this existing refreshToken() code throws an Exception while starting httpClient, tokenUrl as shown below -- it crashes while creating the Jetty HttpClient() for making the refreshToken() call -- whereby the inner exception is Insufficient configured threads: required=10 < max=10 for QueuedThreadPool -- this is not part of my code, so I am wondering how to fix that? Is it a Jetty issue in OH 5.x? Or something else?

Exception while starting httpClient, tokenUrl: https://login.tado.com/oauth2/token
org.openhab.core.auth.client.oauth2.OAuthException: Exception while starting httpClient, tokenUrl: https://login.tado.com/oauth2/token
	at org.openhab.core.auth.oauth2client.internal.OAuthConnector.createHttpClient(OAuthConnector.java:381) ~[?:?]
	at org.openhab.core.auth.oauth2client.internal.OAuthConnector.grantTypeRefreshToken(OAuthConnector.java:195) ~[?:?]
	at org.openhab.core.auth.oauth2client.internal.OAuthClientServiceImpl.refreshToken(OAuthClientServiceImpl.java:319) ~[?:?]
	at org.openhab.core.auth.oauth2client.internal.OAuthClientServiceImpl.getAccessTokenResponse(OAuthClientServiceImpl.java:355) ~[?:?]
	at org.openhab.binding.tado.internal.auth.OAuthorizerV2.addAuthorization(OAuthorizerV2.java:50) ~[?:?]
	at org.openhab.binding.tado.swagger.codegen.api.client.HomeApi.showHome(HomeApi.java:238) ~[?:?]
	at org.openhab.binding.tado.internal.handler.TadoHomeHandler.initializeBridgeStatusAndPropertiesIfOffline(TadoHomeHandler.java:189) ~[?:?]
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:572) ~[?:?]
	at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:358) ~[?:?]
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:305) ~[?:?]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144) ~[?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642) ~[?:?]
	at java.lang.Thread.run(Thread.java:1583) [?:?]
Caused by: java.lang.IllegalStateException: Insufficient configured threads: required=10 < max=10 for QueuedThreadPool[OH-httpClient-OAuthConnector]@19a8dee2{STOPPED,5<=0<=10,i=0,r=-1,q=0}[NO_TRY]
	at org.eclipse.jetty.util.thread.ThreadPoolBudget.check(ThreadPoolBudget.java:165) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.util.thread.ThreadPoolBudget.leaseTo(ThreadPoolBudget.java:141) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.util.thread.ThreadPoolBudget.leaseFrom(ThreadPoolBudget.java:191) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.io.SelectorManager.doStart(SelectorManager.java:255) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:73) ~[?:?]
	at org.eclipse.jetty.util.component.ContainerLifeCycle.start(ContainerLifeCycle.java:169) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.util.component.ContainerLifeCycle.doStart(ContainerLifeCycle.java:117) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.client.AbstractConnectorHttpClientTransport.doStart(AbstractConnectorHttpClientTransport.java:64) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:73) ~[?:?]
	at org.eclipse.jetty.util.component.ContainerLifeCycle.start(ContainerLifeCycle.java:169) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.util.component.ContainerLifeCycle.doStart(ContainerLifeCycle.java:117) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.client.HttpClient.doStart(HttpClient.java:258) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:73) ~[?:?]
	at org.openhab.core.auth.oauth2client.internal.OAuthConnector.createHttpClient(OAuthConnector.java:379) ~[?:?]
	... 12 more

EDIT I think it is this https://community.openhab.org/t/fix-for-jetty-error-when-running-on-host-with-many-cores/57449 .. I am testing on an Intel machine with 20 cores :)

=> I wonder if this is something that should be auto- fixed during OH setup?

@andrewfg andrewfg marked this pull request as ready for review March 9, 2025 16:34
@mherwege
Copy link
Contributor

mherwege commented Mar 9, 2025

EDIT I think it is this https://community.openhab.org/t/fix-for-jetty-error-when-running-on-host-with-many-cores/57449 .. I am testing on an Intel machine with 20 cores :)
=> I wonder if this is something that should be auto- fixed during OH setup?

I ran into the same issue when developing the changes to the mybmw binding. Note that the only thing used from the core service is the token refresh as the way to get the token is mostly specific. I increased the defaults as suggested in the linked issue and didn’t have the problem anymore. I am not sure about this being linked entirely to the number of cores though. I am mostly developing on a pretty standard laptop. I don’t now the number of cores by heart (and I am not behind it) but I don’t think it is that much.

andrewfg added 2 commits March 9, 2025 18:59
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
@andrewfg andrewfg changed the title [tado] Add oauth2 authentication work flow [tado] Add oauth2 authentication (deadline 2025-03-15) Mar 10, 2025
@andrewfg andrewfg changed the title [tado] Add oauth2 authentication (deadline 2025-03-15) [tado] Add OAuth authentication (deadline 2025-03-15) Mar 10, 2025
Signed-off-by: Andrew Fiddian-Green <[email protected]>
@andrewfg andrewfg added the awaiting other PR Depends on another PR label Mar 10, 2025
fewer exceptions, refactoring, faster online

Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Left some comments also need to wait for the upstream PR.

Signed-off-by: Andrew Fiddian-Green <[email protected]>
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting other PR Depends on another PR enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[tado] Tado changes their OAuth2 flow from 'password' to 'device' per 15 March 2025
4 participants