[netatmo] Make OAuth2 token refresh RFC compliant

[clinique]

Resolves #14546

[jlaur]

Thanks, I will give it a test run. Has textual configuration been considered?

[clinique]

Thanks, I will give it a test run. Has textual configuration been considered?

You're correct. Textual configuration could be a real challenge because we can't override the stored configuration - so I think I should move the refreshToken to properties instead of configuration.

[jlaur]

Thanks, I will give it a test run. Has textual configuration been considered?

You're correct. Textual configuration could be a real challenge because we can't override the stored configuration - so I think I should move the refreshToken to properties instead of configuration.

Can you always reauthenticate from clientId and clientSecret to get new access and refresh token? I'm wondering if we even need a property. I guess the advantage is that we don't need to get new token when restarting openHAB/binding, but besides that we could just keep the refresh token in memory?

[lolodomo]

You're correct. Textual configuration could be a real challenge because we can't override the stored configuration - so I think I should move the refreshToken to properties instead of configuration.

That would mean we have to re-authenticate each time we restart OH !
If I correctly remember, you are not using the oAuth2 client from OH core for this binding ? Do you remember what was the reason and do you think the original reason is still valid ?
Because this would be the solution as OH is then storing the necessary data in file userdata/json/StorageHandler.For.OAuthClientService.json

[lolodomo]

The best solution would be really to use the oAuth2 client implementation from the core framework.
In case you really can't, I would suggest that you store your token(s) in your own file somewhere in userdata. The data will then be persisted and this will work whatever the way you define your thing.

[clinique]

Yes, there is a valid reason why I do not use the core oAuth mechanism. Netatmo is not complying to oAuth standard.
I opened an issue a while ago that has never been adressed here.

I tried to adress it from a core perspective here and we discussed this here

[lolodomo]

If you can't use the OH oAuth2 client, I would then like that you store your token data in a file in folder userdata/netatmo.

[clinique]

If you can't use the OH oAuth2 client, I would then like that you store your token data in a file in folder userdata/netatmo.

Ok but why not in properties ?

[lolodomo]

Because thing properties are lost when you restart OH when your thing is defined in a things file.

If you need to persist something (a token( in a file to survive and be re-used after a OH restart or upgrade, do not consider the thing properties option.

[clinique]

Because thing properties are lost when you restart OH when your thing is defined in a things file.

If you need to persist something (a token( in a file to survive and be re-used after a OH restart or upgrade, do not consider the thing properties option.

Valid. Did not think to that. Thanks, you're there !

[lolodomo]

LGTM, thank you

[lolodomo]

@clinique : there is a conflict to fix before the merge.
I will then test the PR and if test is ok, we can merge.

[clinique]

I have resolved the conflict directly online. This conflict is weird, but git may have some mysteries.

[lolodomo]

I have resolved the conflict directly online. This conflict is weird, but git may have some mysteries.

Conflict still mentioned on file ApiBridgeHandler.java.

[clinique]

I have resolved the conflict directly online. This conflict is weird, but git may have some mysteries.

Conflict still mentioned on file ApiBridgeHandler.java.

Understood why in the meantime, it's due to the merge of CO detector, I need to rebase and force push

[jlaur]

Understood why in the meantime, it's due to the merge of CO detector, I need to rebase and force push

If these two PR's conflict, we cannot cherry-pick into the 3.4.x branch, but will instead have to create a new branch tailored for that, i.e. at PR targeting 3.4.x.

[clinique]

@jlaur , @lolodomo : rebased and conflict fixed.

[lolodomo]

@clinique: tests are OK for me.

[lolodomo]

This change also simplified the process for users using config files.

I am asking myself if we should document where are stored the token files ?

[lolodomo]

And thank you @clinique for providing that change so quickly.
The delay forced by netatmo is very short !

[clinique]

You're welcome. I'll add a note in the README

[lolodomo]

Just to understand, does it mean that if you stop your OH server more than 3 hours, you will have to go through the authorization process when restarting OH ?

[clinique]

Hopefully not - I wondered the same reading the oAuth documentation. I hope they refresh with the last provided token, or else, it is the end of this binding. It's a bit hard to predict because the new refreshing refresh token is not in place currently.

[clinique]

I had this fear and you reactivated it. I opened a question on the Netatmo forum. Everything lies in the way to understand this : When refreshing tokens, Access Token and Refresh Token will be automatically renewed and former tokens invalidated.

[lolodomo]

I assume you are storing the updated refresh token in the new file.
Re-reading the sentence, I understand the old refresh token will be invalidated only when you try to refresh. So if API is not requested even during several days, there should be no invalidation.

[clinique]

You assume correctly, and I read it also the way you do.

[jlaur]

@clinique, @lolodomo - I assume we now need a separate PR targeting 3.4.x, since the merged branch seemed to be overlapping with other changes for 4.0?

[clinique]

Yes, a distinct PR has to be created for 3.4.x - I will try to find time to produce it.

[clinique]

@jlaur : done here