[netatmo] Switch to Code Granting process #12726

clinique · 2022-05-12T16:14:22Z

Based on the work done in the spotify binding.
This exposes a second servlet, so I refactor servlet handling in the binding.

Closing issue #12677

Signed-off-by: clinique [email protected]

Signed-off-by: clinique <[email protected]>

clinique · 2022-05-12T16:17:07Z

Current status : authentication flow works.
Reviewing servlet handling between authentication servlet and webhook.
Documentation to be updated (based on the great spotify readme file).

Breaking changes :

ApiBridgeHandler configuration parameters have changed.
webhook url will change

lolodomo · 2022-05-13T07:10:11Z

@clinique : why saving technical data in thing configuration? The user will not be able to set them as they are provided by the API.
Why not keeping them only in memory? Of course, that will mean a new refresh token requested at each binding restart, but is it a problem?

clinique · 2022-05-13T07:12:56Z

@clinique : why saving technical data in thing configuration? The user will not be able to set them as they are provided by the API. Why not keeping them only in memory? Of course, that will mean a new refresh token requested at each binding restart, but is it a problem?

If you do not save them, then you've got to redo the approval process - not doable.
I'll double check, I think we can get rid of the code and maybe the redirectUri, but the token_refresh has to be stored.
Sure, the user can not set them the first time, but once he's got them, they can be used in things file directly.

jlaur · 2022-05-13T14:00:14Z

@clinique : why saving technical data in thing configuration? The user will not be able to set them as they are provided by the API. Why not keeping them only in memory? Of course, that will mean a new refresh token requested at each binding restart, but is it a problem?

If you do not save them, then you've got to redo the approval process - not doable. I'll double check, I think we can get rid of the code and maybe the redirectUri, but the token_refresh has to be stored. Sure, the user can not set them the first time, but once he's got them, they can be used in things file directly.

Can you link to some documentation describing the authorization flow/API? I would like to understand this. Is this the new approach: https://dev.netatmo.com/apidocumentation/oauth?

lolodomo · 2022-05-13T18:14:21Z

And did you think about using OAuth2 provided by a package of the core framework?
Tokens are then stored in a dedicated json file. The homeconnect binding is using it. Probably also others bindings.

lolodomo · 2022-05-13T18:21:57Z

https://github.com/openhab/openhab-core/tree/main/bundles/org.openhab.core.auth.oauth2client/src/main/java/org/openhab/core/auth/oauth2client/internal

clinique · 2022-05-14T09:58:50Z

@clinique : why saving technical data in thing configuration? The user will not be able to set them as they are provided by the API. Why not keeping them only in memory? Of course, that will mean a new refresh token requested at each binding restart, but is it a problem?

If you do not save them, then you've got to redo the approval process - not doable. I'll double check, I think we can get rid of the code and maybe the redirectUri, but the token_refresh has to be stored. Sure, the user can not set them the first time, but once he's got them, they can be used in things file directly.

Can you link to some documentation describing the authorization flow/API? I would like to understand this. Is this the new approach: https://dev.netatmo.com/apidocumentation/oauth?

Keeping the refresh token is enough.
The workflow is presented here, but it does not highlight how we should handle the process from one start to the other.

clinique · 2022-05-14T10:06:07Z

And did you think about using OAuth2 provided by a package of the core framework? Tokens are then stored in a dedicated json file. The homeconnect binding is using it.

It was my original plan but I had an issue with the answer provided by the API (see here). So we arrived here and there . But it still does not work. IIRW the core oauth is not able to reach the deserializer I provide.

lolodomo · 2022-05-15T11:07:51Z

It was my original plan but I had an issue with the answer provided by the API (see here). So we arrived here and there . But it still does not work. IIRW the core oauth is not able to reach the deserializer I provide.

Maybe we can debug what would be wrong in core?
Do you have a branch with the code relying on OH core OAuth2 client?

J-N-K · 2022-05-15T13:46:33Z

I believe that the core bundle must be able to create imports dynamically. This can be achieved by adding DynamicImport-Package: * to the manifest.

clinique · 2022-05-16T07:12:00Z

It was my original plan but I had an issue with the answer provided by the API (see here). So we arrived here and there . But it still does not work. IIRW the core oauth is not able to reach the deserializer I provide.

Maybe we can debug what would be wrong in core? Do you have a branch with the code relying on OH core OAuth2 client?

unfortunately not, It was 2 years ago...

lolodomo · 2022-05-16T18:48:47Z

We must keep in mind that it should be merged before 3.3 is released (end of June) to avoid a second breaking change for users and to have a working 3.3 binding in September when Netatmo will stop the old authentication method.

@clinique :please submit your last code if ready

clinique · 2022-05-17T07:09:50Z

Not much time available this week, I'll do my best as soon as possible - or else, I leave the webhooks untouched, just to take care of code granting, without optimizing servlets.

Signed-off-by: clinique <[email protected]>

lolodomo

Review part 1 of 2

bundles/org.openhab.binding.netatmo/README.md

...ng.netatmo/src/main/java/org/openhab/binding/netatmo/internal/config/ConfigurationLevel.java

lolodomo · 2022-05-18T15:10:50Z

bundles/org.openhab.binding.netatmo/src/main/resources/OH-INF/i18n/netatmo.properties

@@ -330,8 +330,7 @@ thing-type.netatmo.wind.description = Wind sensor reporting wind angle and stren

 conf-error-no-client-id = Cannot connect to Netatmo bridge as no client id is available in the configuration
 conf-error-no-client-secret = Cannot connect to Netatmo bridge as no client secret is available in the configuration
-conf-error-no-username = Cannot connect to Netatmo bridge as no username is available in the configuration
-conf-error-no-password = Cannot connect to Netatmo bridge as no password is available in the configuration
+conf-error-grant-needed = Configuration incomplete, please grant the binding to Netatmo Connect.


Please run the i18n tool as there should be other changes (parameter refreshToken, username, password).

I did it - that's weird.

Ok, I think I understand, this is not something new in this PR but something we missed when reviewing your PR for the new binding.
As you have no static things declared (in XML) linked to these configs, the i18n tool does produce nothing.
So you have no label/description for these configurations available in the translation file.
The solution would be to use @text in the config.xml file and add the corresponding entries in the default translation properties file.

This can be fixed in a next PR if we want to merge the current PR very soon.

...or by extending the translation tool to take this case in account ?

The tool can do nothing as it will know what key to produce. That is typically the case where you need to use @text/myparameterlabel and in this case, the tool will add the entry myparameterlabel in the properties file.

I am going to create a PR for that.

Signed-off-by: clinique <[email protected]>

robnielsen

Don't forget to update the things/netatmo.things example.

lolodomo

Review part 2 o f2

...ing.netatmo/src/main/java/org/openhab/binding/netatmo/internal/handler/ApiBridgeHandler.java

...ng.netatmo/src/main/java/org/openhab/binding/netatmo/internal/config/ConfigurationLevel.java

...nding.netatmo/src/main/java/org/openhab/binding/netatmo/internal/servlet/WebhookServlet.java

lolodomo · 2022-05-19T09:29:07Z

As soon as you push an update with the resolved comments, I will test it.

Signed-off-by: clinique <[email protected]>

bundles/org.openhab.binding.netatmo/README.md

robnielsen · 2022-05-19T23:14:01Z

@clinique, I built you latest code and was able to get it up and running. The only problem I ran into is the refresh token isn't optional for file based configurations.

Signed-off-by: clinique <[email protected]>

bundles/org.openhab.binding.netatmo/README.md

lolodomo · 2022-05-20T09:35:24Z

@clinique, I built you latest code and was able to get it up and running.

Same for me, it is working.

Will see tomorrow if renewing is working.

lolodomo · 2022-05-20T09:53:21Z

Can we elaborate how much it will be a breaking change for users that already moved to the new binding ?
For users using configuration files to setup things, as usual, this is not really a difficulty.
For users using UI to setup things, what does it mean ? They have to delete everything ? Only the account thing ? Maybe nothing ?

clinique · 2022-05-20T09:56:57Z

For users using UI, deleting and recreating the Account thing should be enough.

lolodomo · 2022-05-20T09:59:19Z

For users using UI, deleting and recreating the Account thing should be enough.

This can be done without removing all child things ?
All child things should then be reattached to the new bridge.
This would be a good news.

lolodomo · 2022-05-20T10:05:26Z

@clinique: please answer to the last 2 open comments and then I will commit.
Regarding the missing translations, it could then be fixed in a separate PR.

Signed-off-by: clinique <[email protected]>

lolodomo

LGTM, thank you

lolodomo · 2022-05-20T11:00:13Z

The breaking change is already mentioned in 3.3 so there is nothing more to add for users that will upgrade from 3.2 to 3.3 in July.

For users upgrading from 3.3M5 to next 3.3M6 at end of month, the account thing will certainly be offline. I think we should mention in the forum topic that the account bridge thing has to be removed and created again, child things have to be attached again to the new bridge.

jlaur · 2022-05-20T12:35:52Z

@clinique - thank you! Authorization process went smooth (file-based configuration).

openhab-bot · 2022-06-06T07:44:39Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/netatmo-recreate-things/136451/2

Signed-off-by: clinique <[email protected]>

Signed-off-by: clinique <[email protected]> Signed-off-by: Andras Uhrin <[email protected]>

Signed-off-by: clinique <[email protected]>

Starting switch to Code Granting process

22b115f

Signed-off-by: clinique <[email protected]>

clinique added enhancement An enhancement or new feature for an existing add-on (potentially) not backward compatible labels May 12, 2022

clinique self-assigned this May 12, 2022

clinique requested review from cweitkamp and lolodomo as code owners May 12, 2022 16:14

lolodomo added the work in progress A PR that is not yet ready to be merged label May 12, 2022

Finalizing granting process

7318414

Signed-off-by: clinique <[email protected]>

clinique changed the title ~~[netatmo][wip] Switch to Code Granting process~~ [netatmo] Switch to Code Granting process May 18, 2022

Some leftovers forgotten

1e7a1b9

Signed-off-by: clinique <[email protected]>

clinique removed the work in progress A PR that is not yet ready to be merged label May 18, 2022

clinique added this to the 3.3 milestone May 18, 2022

jlaur removed this from the 3.3 milestone May 18, 2022

lolodomo requested changes May 18, 2022

View reviewed changes

Code review par 1

dbc0c10

Signed-off-by: clinique <[email protected]>

robnielsen requested changes May 19, 2022

View reviewed changes

lolodomo requested changes May 19, 2022

View reviewed changes

Lolodomo code review 2

15ae03b

Signed-off-by: clinique <[email protected]>

robnielsen requested changes May 19, 2022

View reviewed changes

bundles/org.openhab.binding.netatmo/README.md Outdated Show resolved Hide resolved

bundles/org.openhab.binding.netatmo/README.md Show resolved Hide resolved

Adding mentions about the mandatory aspect of refreshToken.

7b1aae6

Signed-off-by: clinique <[email protected]>

lolodomo requested changes May 20, 2022

View reviewed changes

bundles/org.openhab.binding.netatmo/README.md Show resolved Hide resolved

bundles/org.openhab.binding.netatmo/README.md Outdated Show resolved Hide resolved

bundles/org.openhab.binding.netatmo/README.md Show resolved Hide resolved

Last modifications ?

98052f1

Signed-off-by: clinique <[email protected]>

lolodomo approved these changes May 20, 2022

View reviewed changes

lolodomo merged commit 9632a0a into openhab:main May 20, 2022

lolodomo added this to the 3.3 milestone May 20, 2022

clinique deleted the netatmo_12677 branch May 20, 2022 11:38

clinique mentioned this pull request May 20, 2022

[netatmo] Deprecated Authentication Methods #12677

Closed

jlaur mentioned this pull request May 20, 2022

[netatmo] Add translations for config parameters #12768

Merged

andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022

[netatmo] Switch to Code Granting process (openhab#12726)

376ffc5

Signed-off-by: clinique <[email protected]>

andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 12, 2022

[netatmo] Switch to Code Granting process (openhab#12726)

30b83c1

Signed-off-by: clinique <[email protected]> Signed-off-by: Andras Uhrin <[email protected]>

psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023

[netatmo] Switch to Code Granting process (openhab#12726)

08eba8b

Signed-off-by: clinique <[email protected]>

clinique mentioned this pull request Mar 8, 2023

[netatmo] Make OAuth2 token refresh RFC compliant #14548

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[netatmo] Switch to Code Granting process #12726

[netatmo] Switch to Code Granting process #12726

clinique commented May 12, 2022

clinique commented May 12, 2022

lolodomo commented May 13, 2022

clinique commented May 13, 2022 •

edited

Loading

jlaur commented May 13, 2022 •

edited

Loading

lolodomo commented May 13, 2022 •

edited

Loading

lolodomo commented May 13, 2022

clinique commented May 14, 2022

clinique commented May 14, 2022 •

edited by lolodomo

Loading

lolodomo commented May 15, 2022 •

edited

Loading

J-N-K commented May 15, 2022

clinique commented May 16, 2022

lolodomo commented May 16, 2022

clinique commented May 17, 2022

lolodomo left a comment

lolodomo May 18, 2022 •

edited

Loading

clinique May 18, 2022 •

edited

Loading

lolodomo May 20, 2022 •

edited

Loading

lolodomo May 20, 2022

clinique May 20, 2022

lolodomo May 20, 2022

lolodomo May 20, 2022

robnielsen left a comment

lolodomo left a comment

lolodomo commented May 19, 2022

robnielsen commented May 19, 2022

lolodomo commented May 20, 2022 •

edited

Loading

lolodomo commented May 20, 2022

clinique commented May 20, 2022

lolodomo commented May 20, 2022

lolodomo commented May 20, 2022

lolodomo left a comment

lolodomo commented May 20, 2022

jlaur commented May 20, 2022

openhab-bot commented Jun 6, 2022

[netatmo] Switch to Code Granting process #12726

[netatmo] Switch to Code Granting process #12726

Conversation

clinique commented May 12, 2022

clinique commented May 12, 2022

lolodomo commented May 13, 2022

clinique commented May 13, 2022 • edited Loading

jlaur commented May 13, 2022 • edited Loading

lolodomo commented May 13, 2022 • edited Loading

lolodomo commented May 13, 2022

clinique commented May 14, 2022

clinique commented May 14, 2022 • edited by lolodomo Loading

lolodomo commented May 15, 2022 • edited Loading

J-N-K commented May 15, 2022

clinique commented May 16, 2022

lolodomo commented May 16, 2022

clinique commented May 17, 2022

lolodomo left a comment

Choose a reason for hiding this comment

lolodomo May 18, 2022 • edited Loading

Choose a reason for hiding this comment

clinique May 18, 2022 • edited Loading

Choose a reason for hiding this comment

lolodomo May 20, 2022 • edited Loading

Choose a reason for hiding this comment

lolodomo May 20, 2022

Choose a reason for hiding this comment

clinique May 20, 2022

Choose a reason for hiding this comment

lolodomo May 20, 2022

Choose a reason for hiding this comment

lolodomo May 20, 2022

Choose a reason for hiding this comment

robnielsen left a comment

Choose a reason for hiding this comment

lolodomo left a comment

Choose a reason for hiding this comment

lolodomo commented May 19, 2022

robnielsen commented May 19, 2022

lolodomo commented May 20, 2022 • edited Loading

lolodomo commented May 20, 2022

clinique commented May 20, 2022

lolodomo commented May 20, 2022

lolodomo commented May 20, 2022

lolodomo left a comment

Choose a reason for hiding this comment

lolodomo commented May 20, 2022

jlaur commented May 20, 2022

openhab-bot commented Jun 6, 2022

clinique commented May 13, 2022 •

edited

Loading

jlaur commented May 13, 2022 •

edited

Loading

lolodomo commented May 13, 2022 •

edited

Loading

clinique commented May 14, 2022 •

edited by lolodomo

Loading

lolodomo commented May 15, 2022 •

edited

Loading

lolodomo May 18, 2022 •

edited

Loading

clinique May 18, 2022 •

edited

Loading

lolodomo May 20, 2022 •

edited

Loading

lolodomo commented May 20, 2022 •

edited

Loading