openhab / openhab-addons

Add-ons for openHAB
https://www.openhab.org/
Eclipse Public License 2.0
1.88k stars 3.59k forks source link

[mielecloud] Handle used for OAuth2 storage should contain binding context #14783

Open jlaur opened 1 year ago

jlaur commented 1 year ago

While working with OAuth2 in context of #14745 and #14780 I noticed in my StorageHandler.For.OAuthClientService.json file that this binding uses only the e-mail address as handle:

https://github.com/openhab/openhab-addons/blob/69a09ed825b76cb7f7aff4a86181852516b78ebe/bundles/org.openhab.binding.mielecloud/src/main/java/org/openhab/binding/mielecloud/internal/handler/MieleBridgeHandler.java#L111-L113

This is currently not a problem since no other bindings use e-mail address as handle. However, it seems undesired, because if multiple bindings would do this, and users would use the same e-mail address for multiple accounts, there would be a clash.

Expected Behavior

Handle should at least be unique for the binding. Probably it should also be unique for the bridge thing, so it would be possible to have more than one bridge with different handles, so the OAuth authorization flow could be tested with one of them while the other one remains authorized.

Current Behavior

Raw e-mail address is used for creating the handle without any prefixes or postfixes.

Possible Solution

It seems that this.getThing().getUID().getAsString() is often used to create the handle.

Migration might be possible by using importAccessTokenResponse and deleteServiceAndAccessToken

Steps to Reproduce

Go through the authorization flow and check the file StorageHandler.For.OAuthClientService.json afterwards.

lolodomo commented 1 year ago

@jlaur : do you have a clear idea how to implement the change of handle id and will you be able to test it ? Note that it could then be also changed in hydrawise and myq bindings (issue #14939).

jlaur commented 1 year ago

@lolodomo - changing the handle should be straight-forward, but I didn't check closely if my proposal for backwards compatibility will hold.

I don't use this binding normally, but I do have connected Miele appliances, so I will be able to test.