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

[boschindego] Refactor OAuth2 implementation #14950

Merged
merged 4 commits into from
May 11, 2023

Conversation

jlaur
Copy link
Contributor

@jlaur jlaur commented May 6, 2023

This refactoring addresses some issues:

  • When a bridge thing is removed, the corresponding OAuth2 token will now be deleted.
  • When authorization fails (for example when renewing token), the bridge will now change status to OFFLINE with proper authorization failure description, and as a consequence child things will have status detail BRIDGE_OFFLINE rather than the authorization failure description.
  • Reinitialization of bridge things would cause authorization to fail for child things. This is now fixed.

Follow-up to #14745

@jlaur jlaur requested a review from jofleck as a code owner May 6, 2023 21:19
@jlaur jlaur requested a review from lolodomo May 6, 2023 21:19
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur marked this pull request as draft May 7, 2023 07:28
jlaur added 2 commits May 7, 2023 13:29
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur changed the title [boschindego] Delete OAuth2 token when thing is removed [boschindego] Refactor OAuth2 implementation May 7, 2023
@jlaur jlaur marked this pull request as ready for review May 7, 2023 21:15
@lolodomo
Copy link
Contributor

lolodomo commented May 8, 2023

You just afraid me. Why is so complex for this binding ? I just hope I have not missed something for all the other bindings I updated...

@jlaur
Copy link
Contributor Author

jlaur commented May 8, 2023

You just afraid me. Why is so complex for this binding ? I just hope I have not missed something for all the other bindings I updated...

Yeah, I was struggling a bit with the design. Mostly I'm addressing technical debt created by #14745 when I introduced the bridge being responsible for authorization, but still letting each child thing have their own API controller object.

This PR decouples OAuthClientService from the controllers, so instead they will get a shared AuthorizationProvider instance managed by the bridge. This enables the bridge to freely replace the OAuthClientService instance without affecting the child things.

Next, I didn't want the bridge to renew tokens by itself without any requests from the child things. So I introduced a listener so that the bridge will be notified when authorization state changes, in which case the bridge should carry the proper description and the child things should track the status and become BRIDGE_OFFLINE (for example). On the other hand, when completing the OAuth2 flow, the bridge should notify the child things that this has occurred so they can request state updates and come online as fast as possible.

It may look a bit complex, but at least now responsibilities are clearly divided and bridge/things will be notified on important state updates both ways.

I guess the alternative would be to let the bridge be responsible for everything, i.e. all API calls should go through the bridge. I'm not sure it's cleaner, but I may not have given it enough consideration.

Any suggestions are welcome.

Examples:

  • Initially all things are offline because OAuth flow hasn't been completed yet. When completed, BoschAccountHandler will change status to ONLINE and notify all child things that the authorization flow is completed. Subsequently, they will change status to UNKNOWN, request initial state and ultimately become ONLINE as well.
  • BoschIndegoHandler (child) calls IndegoDeviceController to get current state. Controller calls AuthorizationProvider to get authorization header. Token has expired and a renewal attempt is made, but fails with IOException, for example, because of Azure. IndegoAuthenticationException is thrown and caught by BoschIndegoHandler but also BoschAccountHandler is notified about this. As a consequence, the bridge will change status to OFFLINE with COMMUNICATION_ERROR and proper description. Subsequently, all child handlers will change status to OFFLINE with BRIDGE_OFFLINE.
  • While offline, child things will still periodically request state. When first child succeeds after getting valid token through AuthorizationProvider, again BoschAccountHandler will be notified and go ONLINE and indirectly mark all children as UNKNOWN. The child triggering the successful authorization will then go immediately ONLINE after that according to logic in BoschIndegoHandler. Other children will follow after their next scheduled state update.

@jlaur jlaur 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 May 8, 2023
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@lolodomo lolodomo merged commit 1fafec5 into openhab:main May 11, 2023
@lolodomo lolodomo added this to the 4.0 milestone May 11, 2023
@jlaur jlaur deleted the 14536-boschindego-oauth2 branch May 24, 2023 20:03
tb4jc pushed a commit to tb4jc/openhab-addons that referenced this pull request Jun 19, 2023
* Delete OAuth2 token when thing is removed
* Fix reinitialization
* Introduce abstraction for OAuthClientService
* Improve thing status synchronization

---------

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Thomas Burri <thomas.burri@alstomgroup.com>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Aug 9, 2023
* Delete OAuth2 token when thing is removed
* Fix reinitialization
* Introduce abstraction for OAuthClientService
* Improve thing status synchronization

---------

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Matt Myers <mmyers75@icloud.com>
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
* Delete OAuth2 token when thing is removed
* Fix reinitialization
* Introduce abstraction for OAuthClientService
* Improve thing status synchronization

---------

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants