[netatmo] Consolidate OAuth2 handling #14755

jlaur · 2023-04-07T10:13:27Z

Recently Netatmo announced changes in their OAuth2 implementation to make it RFC compliant: #14546

On our side #14548 aligned the openHAB implementation with those changes, but with a custom OAuth2 implementation.

Since I have recently had a look at OAuth2 in context of #14745, I think it would be worth streamlining the OAuth2 implementation by using the available openHAB core classes.

According to the Netatmo documentation, the API should be fully RFC 6749 compliant, thus it should work.

In case any obstacles are found, this should be possible to solve, even it core changes would be needed. RFC violations should preferably be handled directly in the binding, but the core classes should make it possible to extend the functionality to make this possible.

I'm aware of #14548 (comment), so this issue also serves as a placeholder for documenting those potential problems, and coming up with solutions.

If I understand the problem correctly, the grant response looks like this:

{
	"access_token": "51c55d0418xxxxxxxxfa",
	"refresh_token": "51c55xxxxxxxxeb3b",
	"scope": [
		"read_station",
		"read_homecoach",
		"read_presence",
		"access_presence",
		"write_camera",
		"access_camera",
		"read_thermostat",
		"write_thermostat",
		"read_smokedetector",
		"read_camera"
	],
	"expires_in": 10800,
	"expire_in": 10800
}

which is expected to be:

{
	"access_token": "51c55d0418xxxxxxxxfa",
	"refresh_token": "51c55xxxxxxxxeb3b",
	"scope": "read_station read_homecoach read_presence access_presence write_camera access_camera read_thermostat write_thermostat read_smokedetector read_camera",
	"expires_in": 10800,
	"expire_in": 10800
}

Netatmo reply March 13rd 2023: "It's not something planned for the moment, but teams took the point and will think about it"

It seems like openhab/openhab-core#1891 should make it possible to work around.

Perhaps importAccessTokenResponse could be a second option.

@clinique, @lolodomo - FYI.

clinique · 2023-04-07T13:45:49Z

It seems like openhab/openhab-core#1891 should make it possible to work around.

It was the aim of this PR, but I failed finalizing it on Netatmo binding side because I failed to make core identify and load my deserialization class (see this comment and following), and then end up developping it in the binding itself.

I like that you opened this. Three years laters, things never get lost ! 👍

lolodomo · 2023-04-09T07:31:30Z

I like the idea too but I know that @clinique made already a lot of efforts to achieve that.
Could make sense to try again only if we are sure something has changed.

jlaur · 2023-04-09T10:51:48Z

I think the smallest improvement we could make would be using importAccessTokenResponse to inject the token information. This way we would still have a custom implementation of the OAuth2 flow, but at least we would use the standard storage for storing the tokens. WDYT?

Since I will probably hit the same wall as you did with the custom deserialization, @clinique, I guess we should revert openhab/openhab-core#1891 since it has no usages and doesn't work as intended? I'm wondering if we instead could make it possible to inject a prepared GsonBuilder already configured with a custom type adapter?

clinique · 2023-04-09T17:59:32Z

Since I will probably hit the same wall as you did with the custom deserialization, @clinique, I guess we should revert openhab/openhab-core#1891 since it has no usages and doesn't work as intended? I'm wondering if we instead could make it possible to inject a prepared GsonBuilder already configured with a custom type adapter?

Don't you think it would be better to try to make it work ? This is a field where my Java knowledge is limited.

jlaur · 2023-04-09T18:27:12Z

Since I will probably hit the same wall as you did with the custom deserialization, @clinique, I guess we should revert openhab/openhab-core#1891 since it has no usages and doesn't work as intended? I'm wondering if we instead could make it possible to inject a prepared GsonBuilder already configured with a custom type adapter?

Don't you think it would be better to try to make it work ? This is a field where my Java knowledge is limited.

I also don't know how to make it work. But is it the best approach? If a pre-configured GsonBuilder could be injected, this would give us even more control.

jlaur · 2023-04-10T13:41:57Z

I can confirm the mentioned deserialization issue.

package org.openhab.binding.netatmo.internal.deserialization;

import java.lang.reflect.Type;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.openhab.core.auth.client.oauth2.AccessTokenResponse;

import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
import com.google.gson.JsonDeserializationContext;
import com.google.gson.JsonDeserializer;
import com.google.gson.JsonElement;
import com.google.gson.JsonParseException;

@NonNullByDefault
public class AccessTokenResponseDeserializer implements JsonDeserializer<AccessTokenResponse> {

    private final Gson gson;

    public AccessTokenResponseDeserializer() {
        gson = new GsonBuilder().create();
    }

    @Override
    public @Nullable AccessTokenResponse deserialize(JsonElement element, Type arg1, JsonDeserializationContext arg2)
            throws JsonParseException {
        return gson.fromJson(element, org.openhab.binding.netatmo.internal.api.dto.AccessTokenResponse.class)
                .toStandard();
    }
}

and in AccessTokenResponse:

    public org.openhab.core.auth.client.oauth2.AccessTokenResponse toStandard() {
        var standardResponse = new org.openhab.core.auth.client.oauth2.AccessTokenResponse();

        standardResponse.setAccessToken(accessToken);
        standardResponse.setExpiresIn(expiresIn);
        standardResponse.setRefreshToken(refreshToken);

        StringJoiner stringJoiner = new StringJoiner(" ");
        scope.forEach(s -> stringJoiner.add(s.name().toLowerCase()));
        standardResponse.setScope(stringJoiner.toString());

        return standardResponse;
    }

then:

        OAuthClientService oAuthClientService = oAuthFactory
                .createOAuthClientService(this.getThing().getUID().getAsString(),
                        AuthenticationApi.TOKEN_URI.toString(), AuthenticationApi.AUTH_URI.toString(),
                        configuration.clientId, configuration.clientSecret, FeatureArea.ALL_SCOPES, false)
                .withDeserializer(AccessTokenResponseDeserializer.class);

with result:

2023-04-10 15:21:09.799 [ERROR] [oauth2client.internal.OAuthConnector] - Unable to construct custom deserializer 'org.openhab.binding.netatmo.internal.deserialization.AccessTokenResponseDeserializer'
java.lang.ClassNotFoundException: org.openhab.binding.netatmo.internal.deserialization.AccessTokenResponseDeserializer cannot be found by org.openhab.core.auth.oauth2client_4.0.0.202304080304
	at org.eclipse.osgi.internal.loader.BundleLoader.generateException(BundleLoader.java:541) ~[org.eclipse.osgi-3.18.0.jar:?]
	at org.eclipse.osgi.internal.loader.BundleLoader.findClass0(BundleLoader.java:536) ~[org.eclipse.osgi-3.18.0.jar:?]
	at org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java:416) ~[org.eclipse.osgi-3.18.0.jar:?]
	at org.eclipse.osgi.internal.loader.ModuleClassLoader.loadClass(ModuleClassLoader.java:168) ~[org.eclipse.osgi-3.18.0.jar:?]
	at java.lang.ClassLoader.loadClass(ClassLoader.java:520) ~[?:?]
	at java.lang.Class.forName0(Native Method) ~[?:?]
	at java.lang.Class.forName(Class.java:375) ~[?:?]
	at org.openhab.core.auth.oauth2client.internal.OAuthConnector.<init>(OAuthConnector.java:83) ~[?:?]
	at org.openhab.core.auth.oauth2client.internal.OAuthClientServiceImpl.refreshToken(OAuthClientServiceImpl.java:301) ~[?:?]
	at org.openhab.core.auth.oauth2client.internal.OAuthClientServiceImpl.getAccessTokenResponse(OAuthClientServiceImpl.java:336) ~[?:?]
	at org.openhab.binding.netatmo.internal.handler.ApiBridgeHandler.openConnection(ApiBridgeHandler.java:164) ~[?:?]
	at org.openhab.binding.netatmo.internal.handler.ApiBridgeHandler.lambda$0(ApiBridgeHandler.java:136) ~[?:?]
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539) ~[?:?]
	at java.util.concurrent.FutureTask.run(FutureTask.java:264) ~[?:?]
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304) ~[?:?]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) ~[?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) ~[?:?]
	at java.lang.Thread.run(Thread.java:833) ~[?:?]

jlaur · 2023-04-10T14:12:18Z

I tried a few other things to at least use the common store before jumping on the core classes:

Load and save directly through OAuthStoreHandler, but this is internal to core.
Load and save through OAuthClientService methods getAccessTokenResponse and importAccessTokenResponse. It almost works - I was able to store everything. However, getAccessTokenResponse will trigger OAuth communication etc. when access token has expired, so it will ultimately again result in deserialization issues.

For reference:

2023-04-10 14:00:25.540 [DEBUG] [mo.internal.handler.ApiBridgeHandler] - Failed to load access token: Unable to deserialize json into AccessTokenResponse/ OAuthResponseException. httpCode: 200 json: {"scope":["read_homecoach","read_station","read_thermostat","write_thermostat","read_presence","read_smokedetector","write_presence","access_presence","write_doorbell","read_camera","write_camera","read_doorbell","read_carbonmonoxidedetector","access_camera","access_doorbell"],"access_token":"xxx","refresh_token":"yyy","expires_in":10800,"expire_in":10800}

The advantage of at least using the store rather than the current custom file approach is that it would ensure forward-compatibility, so users wouldn't have to reauthorize when we'll succeed in using the core OAuth2 implementation.

jlaur · 2023-04-10T18:51:40Z

Small update: I jumped into core to try out my proposal to inject GsonBuilder here. I have been working on the related changes in the binding as well, this time fully migrating to the OAuth implementation in core.

I just now managed to authorize from a clean slate using the existing servlet. And after this I was able to restart openHAB and reauthorize automatically as well.

I'm now pretty confident that it will somehow work out, even if my current implementation will change. I'll refactor and clean up a bit, and then prepare some draft pull requests.

StorageHandler.For.OAuthClientService.json

{
  "INDEX_HANDLES": {
    "class": "java.lang.String",
    "value": "[\n  \"netatmo:account:a6fae04bab\"\n]"
  },
  "netatmo:account:a6fae04bab.AccessTokenResponse": {
    "class": "java.lang.String",
    "value": "{\n  \"accessToken\": \"xxx\",\n  \"expiresIn\": 10800,\n  \"refreshToken\": \"yyy\",\n  \"scope\": \"read_homecoach read_station read_thermostat write_thermostat read_presence access_presence read_carbonmonoxidedetector write_presence read_smokedetector access_doorbell read_camera write_doorbell write_camera read_doorbell access_camera\",\n  \"createdOn\": \"2023-04-10T18:39:15.153302900Z\"\n}"
  },
  "netatmo:account:a6fae04bab.LastUsed": {
    "class": "java.lang.String",
    "value": "\"2023-04-10T18:45:25.273590400Z\""
  },
  "netatmo:account:a6fae04bab.ServiceConfiguration": {
    "class": "java.lang.String",
    "value": "{\n  \"handle\": \"netatmo:account:a6fae04bab\",\n  \"tokenUrl\": \"https://api.netatmo.com/oauth2/token\",\n  \"authorizationUrl\": \"https://api.netatmo.com/oauth2/authorize\",\n  \"clientId\": \"zzz\",\n  \"clientSecret\": \"qqq\",\n  \"scope\": \"read_homecoach read_station write_thermostat read_thermostat access_camera write_presence read_carbonmonoxidedetector read_presence read_smokedetector write_doorbell read_camera write_camera access_presence read_doorbell access_doorbell\",\n  \"supportsBasicAuth\": false,\n  \"tokenExpiresInSeconds\": 10\n}"
  }
}

Fixes openhab#14755 Signed-off-by: Jacob Laursen <[email protected]>

clinique · 2023-04-11T08:28:03Z

Thanks for taking care of this. It will be far more clean when Netatmo binding will use core functions.

Fixes openhab#14755 Signed-off-by: Jacob Laursen <[email protected]>

…#14780) * Consolidate OAuth2 by using core implementation and storage Fixes #14755 --------- Signed-off-by: Jacob Laursen <[email protected]>

jlaur added the enhancement An enhancement or new feature for an existing add-on label Apr 7, 2023

jlaur self-assigned this Apr 7, 2023

jlaur mentioned this issue Apr 10, 2023

[oauth] Add support for custom deserialization of AccessTokenResponse openhab/openhab-core#3537

Merged

jlaur added a commit to jlaur/openhab-addons that referenced this issue Apr 10, 2023

Consolidate OAuth2 by using core implementation and storage

df6b423

Fixes openhab#14755 Signed-off-by: Jacob Laursen <[email protected]>

jlaur mentioned this issue Apr 10, 2023

[netatmo] Consolidate OAuth2 by using core implementation and storage #14780

Merged

jlaur added a commit to jlaur/openhab-addons that referenced this issue Apr 10, 2023

Consolidate OAuth2 by using core implementation and storage

be0e077

Fixes openhab#14755 Signed-off-by: Jacob Laursen <[email protected]>

jlaur added a commit to jlaur/openhab-addons that referenced this issue Apr 14, 2023

Consolidate OAuth2 by using core implementation and storage

128bb00

Fixes openhab#14755 Signed-off-by: Jacob Laursen <[email protected]>

jlaur added a commit to jlaur/openhab-addons that referenced this issue Apr 15, 2023

Consolidate OAuth2 by using core implementation and storage

8b8cbc9

Fixes openhab#14755 Signed-off-by: Jacob Laursen <[email protected]>

lolodomo closed this as completed in #14780 Apr 21, 2023

lolodomo pushed a commit that referenced this issue Apr 21, 2023

[netatmo] Consolidate OAuth2 by using core implementation and storage (…

cf3c3f1

…#14780) * Consolidate OAuth2 by using core implementation and storage Fixes #14755 --------- Signed-off-by: Jacob Laursen <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[netatmo] Consolidate OAuth2 handling #14755

[netatmo] Consolidate OAuth2 handling #14755

jlaur commented Apr 7, 2023 •

edited

Loading

clinique commented Apr 7, 2023 •

edited

Loading

lolodomo commented Apr 9, 2023

jlaur commented Apr 9, 2023

clinique commented Apr 9, 2023

jlaur commented Apr 9, 2023

jlaur commented Apr 10, 2023 •

edited

Loading

jlaur commented Apr 10, 2023 •

edited

Loading

jlaur commented Apr 10, 2023

clinique commented Apr 11, 2023

[netatmo] Consolidate OAuth2 handling #14755

[netatmo] Consolidate OAuth2 handling #14755

Comments

jlaur commented Apr 7, 2023 • edited Loading

clinique commented Apr 7, 2023 • edited Loading

lolodomo commented Apr 9, 2023

jlaur commented Apr 9, 2023

clinique commented Apr 9, 2023

jlaur commented Apr 9, 2023

jlaur commented Apr 10, 2023 • edited Loading

jlaur commented Apr 10, 2023 • edited Loading

jlaur commented Apr 10, 2023

clinique commented Apr 11, 2023

jlaur commented Apr 7, 2023 •

edited

Loading

clinique commented Apr 7, 2023 •

edited

Loading

jlaur commented Apr 10, 2023 •

edited

Loading

jlaur commented Apr 10, 2023 •

edited

Loading