Repository: bimmerconnected/bimmer_connected
Pull request #650 (2024-08-24 15:01:38)

Always raise MyBMWAuthError during login
## Proposed change
<!--
  Describe the big picture of your changes here to communicate to the
  maintainers why we should accept this pull request. If it fixes a bug
  or resolves a feature request, be sure to link to that issue in the
  additional information section.
-->
Always raise `MyBMWAuthError` when a HTTP error occurs during authentication.

This should solve retry loops due to quota issues like [here](https://github.com/home-assistant/core/issues/124464#issuecomment-2306719550) and trigger the HA reauthentication flow (stopping retries).

## Type of change
- [x] Bugfix (non-breaking change which fixes an issue)
- [ ] New feature (which adds functionality to this library)
- [ ] Breaking change (fix/feature causing existing functionality to break)
- [ ] Code quality improvements to existing code or addition of tests

## Additional information
<!--
  Details are important, and help maintainers processing your PR.
  Please be sure to fill out additional details, if applicable.
-->

- This PR fixes or closes issue: fixes #https://github.com/home-assistant/core/issues/124464
- This PR is related to issue: 

## Checklist
<!--
  Put an `x` in the boxes that apply. You can also fill these out after
  creating the PR. If you're unsure about any of them, don't hesitate to ask.
  We're here to help! This is simply a reminder of what we are going to look
  for before merging your code.
-->

- [x] The code change is tested and works locally.
- [x] Tests have been added to verify that the new code works.


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/pull/650?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 100.00%. Comparing base [(`ef3db68`)](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/commit/ef3db68a9dbd8a29b459d57e743b77d6242a4a76?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected) to head [(`51b0641`)](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/commit/51b06417a783c3fa6986685f16b6681b0712255d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected).
> Report is 19 commits behind head on master.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff            @@
##            master      #650   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           17        17           
  Lines         1428      1418   -10     
=========================================
- Hits          1428      1418   -10     
```

| [Flag](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/pull/650/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected) | Coverage Δ | |
|---|---|---|
| [3.10](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/pull/650/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected) | `100.00% <100.00%> (ø)` | |
| [3.11](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/pull/650/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected) | `100.00% <100.00%> (ø)` | |
| [3.12](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/pull/650/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected) | `100.00% <100.00%> (ø)` | |
| [3.8](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/pull/650/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected) | `100.00% <100.00%> (ø)` | |
| [3.9](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/pull/650/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected) | `100.00% <100.00%> (ø)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected#carryforward-flags-in-the-pull-request-comment) to find out more.

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/bimmerconnected/bimmer_connected/pull/650?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bimmerconnected).


Patch:
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 855bcdb..c200782 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -1,6 +1,6 @@
 repos:
   - repo: https://github.com/charliermarsh/ruff-pre-commit
-    rev: v0.5.6
+    rev: v0.6.2
     hooks:
       - id: ruff
         args:
diff --git a/bimmer_connected/api/authentication.py b/bimmer_connected/api/authentication.py
index c2203ce..bf5198c 100644
--- a/bimmer_connected/api/authentication.py
+++ b/bimmer_connected/api/authentication.py
@@ -117,7 +117,7 @@ class MyBMWAuthentication(httpx.Auth):
         try:
             response.raise_for_status()
         except httpx.HTTPStatusError as ex:
-            await handle_httpstatuserror(ex, log_handler=_LOGGER)
+            await handle_httpstatuserror(ex, module="API", log_handler=_LOGGER)
 
     async def login(self) -> None:
         """Get a valid OAuth token."""
@@ -390,7 +390,7 @@ class MyBMWLoginClient(httpx.AsyncClient):
                 try:
                     response.raise_for_status()
                 except httpx.HTTPStatusError as ex:
-                    await handle_httpstatuserror(ex, log_handler=_LOGGER)
+                    await handle_httpstatuserror(ex, module="AUTH", log_handler=_LOGGER)
 
         kwargs["event_hooks"]["response"].append(raise_for_status_event_handler)
 
@@ -420,7 +420,7 @@ class MyBMWLoginRetry(httpx.Auth):
             try:
                 response.raise_for_status()
             except httpx.HTTPStatusError as ex:
-                await handle_httpstatuserror(ex, log_handler=_LOGGER)
+                await handle_httpstatuserror(ex, module="AUTH", log_handler=_LOGGER)
 
 
 def get_retry_wait_time(response: httpx.Response) -> int:
diff --git a/bimmer_connected/api/utils.py b/bimmer_connected/api/utils.py
index d40135c..b362179 100644
--- a/bimmer_connected/api/utils.py
+++ b/bimmer_connected/api/utils.py
@@ -64,14 +64,17 @@ async def handle_httpstatuserror(
     # By default we will raise a MyBMWAPIError
     _ex_to_raise = MyBMWAPIError
 
-    # HTTP status code is 401 or 403, raise MyBMWAuthError instead
-    if ex.response.status_code in [401, 403]:
-        _ex_to_raise = MyBMWAuthError
-
     # Quota errors can either be 429 Too Many Requests or 403 Quota Exceeded (instead of 401 Forbidden)
-    if ex.response.status_code == 429 or (ex.response.status_code == 403 and "quota" in ex.response.text.lower()):
+    if (
+        ex.response.status_code == 429 or (ex.response.status_code == 403 and "quota" in ex.response.text.lower())
+    ) and module != "AUTH":
         _ex_to_raise = MyBMWQuotaError
 
+    # HTTP status code is 401 or 403, raise MyBMWAuthError instead
+    # Always raise MyBMWAuthError as final when logging in (e.g. HTTP 429 should still be AuthError)
+    elif ex.response.status_code in [401, 403] or module == "AUTH":
+        _ex_to_raise = MyBMWAuthError
+
     try:
         # Try parsing the known BMW API error JSON
         _err = ex.response.json()


Test patch:
diff --git a/bimmer_connected/tests/test_account.py b/bimmer_connected/tests/test_account.py
index 7ebd7fa..347c97e 100644
--- a/bimmer_connected/tests/test_account.py
+++ b/bimmer_connected/tests/test_account.py
@@ -27,6 +27,7 @@ from . import (
     get_fingerprint_count,
     load_response,
 )
+from .common import MyBMWMockRouter
 from .conftest import prepare_account_with_vehicles
 
 
@@ -389,7 +390,7 @@ async def test_refresh_token_getset(bmw_fixture: respx.Router):
 
 
 @pytest.mark.asyncio
-async def test_429_retry_ok_login(caplog, bmw_fixture: respx.Router):
+async def test_429_retry_ok_oauth_config(caplog, bmw_fixture: respx.Router):
     """Test the login flow using refresh_token."""
     account = MyBMWAccount(TEST_USERNAME, TEST_PASSWORD, TEST_REGION)
 
@@ -416,7 +417,7 @@ async def test_429_retry_ok_login(caplog, bmw_fixture: respx.Router):
 
 
 @pytest.mark.asyncio
-async def test_429_retry_raise_login(caplog, bmw_fixture: respx.Router):
+async def test_429_retry_raise_oauth_config(caplog, bmw_fixture: respx.Router):
     """Test the login flow using refresh_token."""
     account = MyBMWAccount(TEST_USERNAME, TEST_PASSWORD, TEST_REGION)
 
@@ -436,6 +437,55 @@ async def test_429_retry_raise_login(caplog, bmw_fixture: respx.Router):
     assert len(log_429) == 3
 
 
+@pytest.mark.asyncio
+async def test_429_retry_ok_authenticate(caplog, bmw_fixture: respx.Router):
+    """Test the login flow using refresh_token."""
+    account = MyBMWAccount(TEST_USERNAME, TEST_PASSWORD, TEST_REGION)
+
+    json_429 = {"statusCode": 429, "message": "Rate limit is exceeded. Try again in 2 seconds."}
+
+    bmw_fixture.post("/gcdm/oauth/authenticate").mock(
+        side_effect=[
+            httpx.Response(429, json=json_429),
+            httpx.Response(429, json=json_429),
+            MyBMWMockRouter.authenticate_sideeffect,  # type: ignore[list-item]
+            MyBMWMockRouter.authenticate_sideeffect,  # type: ignore[list-item]
+        ]
+    )
+    caplog.set_level(logging.DEBUG)
+
+    with mock.patch("asyncio.sleep", new_callable=mock.AsyncMock):
+        await account.get_vehicles()
+
+    log_429 = [
+        r
+        for r in caplog.records
+        if r.module == "authentication" and "seconds due to 429 Too Many Requests" in r.message
+    ]
+    assert len(log_429) == 2
+
+
+@pytest.mark.asyncio
+async def test_429_retry_raise_authenticate(caplog, bmw_fixture: respx.Router):
+    """Test the login flow using refresh_token."""
+    account = MyBMWAccount(TEST_USERNAME, TEST_PASSWORD, TEST_REGION)
+
+    json_429 = {"statusCode": 429, "message": "Rate limit is exceeded. Try again in 2 seconds."}
+
+    bmw_fixture.post("/gcdm/oauth/authenticate").mock(return_value=httpx.Response(429, json=json_429))
+    caplog.set_level(logging.DEBUG)
+
+    with mock.patch("asyncio.sleep", new_callable=mock.AsyncMock), pytest.raises(MyBMWAuthError):
+        await account.get_vehicles()
+
+    log_429 = [
+        r
+        for r in caplog.records
+        if r.module == "authentication" and "seconds due to 429 Too Many Requests" in r.message
+    ]
+    assert len(log_429) == 3
+
+
 @pytest.mark.asyncio
 async def test_429_retry_ok_vehicles(caplog, bmw_fixture: respx.Router):
     """Test waiting on 429 for vehicles."""