[mybmw] Improve authentication #18235

mherwege · 2025-02-06T15:19:37Z

The current version of the binding requires a Captcha Token parameter to be set in the account bridge thing on initial configuration and for every restart of the binding or openHAB. The Captcha Token is created from a bimmerconnect webpage.

This PR makes the following changes:

Use the core OAuth2 client to retrieve and store the token and refresh token. This core service then automatically takes care of the token refresh. The token is persisted and will also survive restarts.
At initial bridge start, if no token is available yet, a page will be created for the Captcha (link in the bridge status details), and the respons will set the configuration parameter and reinitialize. Therefore, if working in the UI, it is no longer necessary to retrieve the Captcha Token from an external website.
Give clearer messages in the thing status when quota have been reached, and block further requests until quota are available again.
Adapt the tests to these changes and make sure they all pass. I have removed some integration tests that were effectively double tests with different code now.

I have tested this for the ROW region. Code is also adapted for the NA region, as it only has a few differences.

Support for China has been removed as it is not functional anyway at the moment. There also is no way to test without a Chinese user supporting it.

@martingrassl I would appreciate your testing and feedback.

jlaur · 2025-02-06T19:49:34Z

@kaikreuzer - can you add @martingrassl to the contributors team since he is one of the maintainers of this binding?

martingrassl · 2025-02-06T22:28:44Z

@mherwege thanks for your PR I'll review it at the weekend

Signed-off-by: Mark Herwege <[email protected]>

martingrassl

@mherwege thanks a lot for this PR. I could not test it, but it would be a big step forward in the stability of the system. Just added some minor comments

...nhab.binding.mybmw/src/main/java/org/openhab/binding/mybmw/internal/MyBMWHandlerFactory.java

...ng.mybmw/src/main/java/org/openhab/binding/mybmw/internal/console/MyBMWCommandExtension.java

martingrassl · 2025-02-09T20:24:04Z

...nding.mybmw/src/main/java/org/openhab/binding/mybmw/internal/handler/MyBMWBridgeHandler.java

            }
+        }
+        super.updateConfiguration(config);
+        localBridgeConfiguration = getConfigAs(MyBMWBridgeConfiguration.class);


maybe you can add a comment here - does this mean you retrieve the configuration in line 114, update it in OH central config and then get an updated version here?

Yes indeed.

...b.binding.mybmw/src/main/java/org/openhab/binding/mybmw/internal/handler/VehicleHandler.java

martingrassl · 2025-02-09T20:34:23Z

...g.openhab.binding.mybmw/src/main/java/org/openhab/binding/mybmw/internal/MyBMWConstants.java

@@ -60,6 +61,22 @@ public interface MyBMWConstants {

    static final int DEFAULT_REFRESH_INTERVAL_MINUTES = 60;

+    static final String BASE_PATH = "/" + BINDING_ID + "/";
+
+    static final String HTML_SOURCE = "captcha/";


here maybe CAPTCHA_URL_ROOT could be more self explaining

martingrassl · 2025-02-09T20:37:36Z

...ybmw/src/main/java/org/openhab/binding/mybmw/internal/handler/auth/MyBMWTokenController.java

                logger.warn(
-                        "initial Authentication failed, maybe request a new captcha token, see https://bimmer-connected.readthedocs.io/en/latest/captcha/rest_of_world.html!");
+                        "initial Authentication failed, maybe request a new captcha token, see https://bimmer-connected.readthedocs.io/en/stable/captcha.html!");


so your servlet will not replace the initial bimmer-connected hcaptcha?

Yes and no. If working through the UI, it will have the link to the servlet. But if you do the setup in thing files, you could still get the captcha from the original bimmer-connected link and set it in the thing files. This will still work. And that's why the log refers to that instead of the servlet. The servlet will only be available while waiting for a captcha.
Note that as soon as there is a token, the captcha from a configuration file will be ignored as long as the token can be refreshed.

ok yes I'm always forgetting about the manual configuration since the UI configuration improved so much over the last releases

martingrassl · 2025-02-09T20:39:08Z

...ybmw/src/main/java/org/openhab/binding/mybmw/internal/handler/auth/MyBMWTokenController.java

-                logger.warn("Authorization failed!");
+            bridgeHandler.setHCaptchaToken(Constants.EMPTY);
+        } else if (!waitingForInitialToken && tokenResponse.isExpired(Instant.now(), 5)) {
+            if (REGION_CHINA.equals(bridgeConfiguration.getRegion())) {


imho we can remove China completely - the binding can't work right now in China and nobody complained so far :)

Will do so.

martingrassl · 2025-02-09T20:40:42Z

...ybmw/src/main/java/org/openhab/binding/mybmw/internal/handler/auth/MyBMWTokenController.java

+
+                    if (bridgeConfiguration.getHCaptchaToken().isBlank()) {
+                        logger.warn(
+                                "initial Authentication failed, request a new captcha token, see https://bimmer-connected.readthedocs.io/en/stable/captcha.html!");


same here there should be a reference to the local servlet, right?

...mw/src/test/java/org/openhab/binding/mybmw/internal/handler/backend/MyBMWProxyBackendIT.java

Signed-off-by: Mark Herwege <[email protected]>

martingrassl

LGTM

lsiepel

Thanks, LGTM

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

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

mherwege self-assigned this Feb 6, 2025

mherwege requested a review from ntruchsess as a code owner February 6, 2025 15:19

jlaur added the enhancement An enhancement or new feature for an existing add-on label Feb 6, 2025

mherwege force-pushed the bmw branch from abeba63 to bf5f08b Compare February 7, 2025 10:27

authentication

b14e30f

Signed-off-by: Mark Herwege <[email protected]>

mherwege force-pushed the bmw branch from c992441 to b14e30f Compare February 7, 2025 12:21

martingrassl suggested changes Feb 9, 2025

View reviewed changes

mherwege added 3 commits February 10, 2025 11:51

review feedback

a575c5b

Signed-off-by: Mark Herwege <[email protected]>

remove support for China

b0453e8

Signed-off-by: Mark Herwege <[email protected]>

constants improvements

cffea68

Signed-off-by: Mark Herwege <[email protected]>

martingrassl approved these changes Feb 10, 2025

View reviewed changes

lsiepel approved these changes Feb 12, 2025

View reviewed changes

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

Update bundles/org.openhab.binding.mybmw/README.md

9228edb

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

lsiepel merged commit 3b820ed into openhab:main Feb 12, 2025
2 checks passed

lsiepel added this to the 5.0 milestone Feb 12, 2025

lsiepel changed the title ~~[mybmw] improve authentication~~ [mybmw] Improve authentication Feb 12, 2025

mherwege deleted the bmw branch February 12, 2025 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mybmw] Improve authentication #18235

[mybmw] Improve authentication #18235

mherwege commented Feb 6, 2025 •

edited

Loading

jlaur commented Feb 6, 2025

martingrassl commented Feb 6, 2025

martingrassl left a comment

martingrassl Feb 9, 2025

mherwege Feb 10, 2025

martingrassl Feb 9, 2025

martingrassl Feb 9, 2025

mherwege Feb 10, 2025

martingrassl Feb 10, 2025

martingrassl Feb 9, 2025

mherwege Feb 10, 2025

martingrassl Feb 9, 2025

mherwege Feb 10, 2025

martingrassl left a comment

lsiepel left a comment

[mybmw] Improve authentication #18235

[mybmw] Improve authentication #18235

Conversation

mherwege commented Feb 6, 2025 • edited Loading

jlaur commented Feb 6, 2025

martingrassl commented Feb 6, 2025

martingrassl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martingrassl left a comment

Choose a reason for hiding this comment

lsiepel left a comment

Choose a reason for hiding this comment

mherwege commented Feb 6, 2025 •

edited

Loading