Skip to content

feat: add support for extracting cloud keys via quick login#291

Open
lzghzr wants to merge 2 commits into
Bluetooth-Devices:mainfrom
lzghzr:qrcode
Open

feat: add support for extracting cloud keys via quick login#291
lzghzr wants to merge 2 commits into
Bluetooth-Devices:mainfrom
lzghzr:qrcode

Conversation

@lzghzr

@lzghzr lzghzr commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

Password login often requires a verification code, so I added another login method.

This is a change to the core integration. lzghzr/ha-core@5b17bb2

2026-04-14.21.35.49.mov

@codecov

codecov Bot commented Apr 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 18.46154% with 106 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.93%. Comparing base (e219e76) to head (6b86f50).
⚠️ Report is 29 commits behind head on main.

Files with missing lines Patch % Lines
src/xiaomi_ble/cloud.py 18.46% 106 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #291      +/-   ##
==========================================
- Coverage   54.28%   52.93%   -1.35%     
==========================================
  Files           7        7              
  Lines        1378     1464      +86     
  Branches      268      281      +13     
==========================================
+ Hits          748      775      +27     
- Misses        512      573      +61     
+ Partials      118      116       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Ernst79

Ernst79 commented Apr 29, 2026

Copy link
Copy Markdown
Collaborator

Thanks for this PR. Can you please have a look at the failing checks first?

@bdraco

bdraco commented May 23, 2026

Copy link
Copy Markdown
Member

@bluetoothbot review

@bluetoothbot

Copy link
Copy Markdown
Contributor

@Ernst79: Thanks for this PR. Can you please have a look at the failing checks first?

To help move this forward, here's why the checks are red:

lint (pre-commit → mypy): the repo runs mypy with disallow_untyped_defs = true and no_implicit_optional = true (see pyproject.toml [tool.mypy]). QrCodeXiaomiCloudConnector.__init__ is missing the -> None return annotation, and its attributes are initialized to bare None (self._location = None, self._qr_image_url = None, …), so mypy infers their type as None and then errors when they're reassigned to strings and passed to self._session.get(self._location, …) / XiaomiCloudQrCode(self._qr_image_url, …) (whose fields are typed str). Annotating them as str | None = None like the password connector does, and adding -> None, should clear those.

test / Codecov: the new QR flow has ~18% patch coverage and there's no test module for cloud.py, so coverage drops. Adding tests with a mocked aiohttp session for get_login_qrcode(), the login() long-poll success and timeout paths, and a regression test for the existing password flow would both green the checks and catch the runtime bugs noted in the review (the _login_step_2 polling loop never actually loops, and the timeout value is in milliseconds but is passed to aiohttp as seconds). I've left inline comments with the specifics.

@bluetoothbot

Copy link
Copy Markdown
Contributor

PR Review — feat: add support for extracting cloud keys via quick login

The refactor to split XiaomiCloudConnector into an ABC with password and QR-code implementations is a reasonable design, and QR login is a genuinely useful addition for accounts that require a verification code. However the PR is not mergeable yet. Two blocking issues: (1) QrCodeXiaomiCloudConnector._login_step_2 has a dead timeout guard (start_time compared to itself) with no actual polling loop, and feeds Xiaomi's millisecond timeout directly into aiohttp's seconds-based timeout= kwarg — so the wait-for-scan logic doesn't work and can hang for hours; (2) the new connector's __init__ lacks a -> None annotation and its attributes are initialized to bare None, which fails the repo's strict mypy config — this is the failing lint check the maintainer asked about. Beyond those, _timeout is never initialized in __init__ (AttributeError if the QR body is missing), the success path of _login_step_1 doesn't actually require a qr field, session was made optional despite being dereferenced unconditionally, and the entire new flow is untested (18% patch coverage, no cloud.py test module exists). Fixing the type annotations and adding tests for both the QR and password paths will get the checks green and surface the polling/timeout bugs.


🔴 Blocking

1. QR long-poll has dead timeout guard and no polling loop (`src/xiaomi_ble/cloud.py`, L418-424)

_login_step_2 is broken in two ways:

  1. Dead guard: start_time = time.time() is captured and then compared against self._timeout on the very next line, so time.time() - start_time is always ~0 and the return False is unreachable. This is clearly leftover from an intended polling loop. QR login is a wait-for-scan flow — the long-polling endpoint returns once the user scans (or after the server's poll window). A single GET with no surrounding while time.time() - start_time < deadline: loop means that if the server returns before the scan completes (Xiaomi's longPolling can return intermediate states), login simply fails. You need a real loop that re-polls until success or until the deadline.

  2. Timeout units: timeout=self._timeout passes response_data["timeout"] straight into aiohttp's timeout= kwarg. Xiaomi's timeout field is in milliseconds (e.g. 300000), but aiohttp interprets the number as seconds. So instead of 300s this blocks for ~83 hours if no response arrives. Convert (self._timeout / 1000) or wrap in aiohttp.ClientTimeout.

Suggested shape:

async def _login_step_2(self) -> bool:
    deadline = time.time() + self._timeout / 1000
    while time.time() < deadline:
        response = await self._session.get(self._long_polling_url)
        if response.status != 200:
            return False
        data = self.to_json(await response.text())
        if "ssecurity" in data:
            self.userId = data["userId"]
            self._ssecurity = data["ssecurity"]
            ...
            return True
    return False
start_time = time.time()
if time.time() - start_time > self._timeout:
    return False
response = await self._session.get(
    self._long_polling_url, timeout=self._timeout
)
2. Untyped __init__ and attributes will fail the strict mypy lint check (`src/xiaomi_ble/cloud.py`, L386-393)

This is almost certainly the source of the failing CI lint job (pre-commit runs mypy, and pyproject.toml sets disallow_untyped_defs = true, disallow_incomplete_defs = true, no_implicit_optional = true).

  1. def __init__(self, session: aiohttp.ClientSession): is missing the -> None return annotation → disallow_untyped_defs error. Compare to PasswordXiaomiCloudConnector.__init__, which has -> None.
  2. The attributes are initialized to bare None (self._cUserId = None, self._location = None, self._qr_image_url = None, …), so mypy infers their type as None. They are later assigned str/dict values (self._location = response_data["location"], self._qr_image_url = response_data["qr"]), and self._location/self._qr_image_url are then passed where str is expected (self._session.get(self._location, …), XiaomiCloudQrCode(self._qr_image_url, self._login_url) whose fields are typed str). That produces incompatible-assignment / incompatible-argument errors.

Annotate explicitly, mirroring the password connector: self._cUserId: str | None = None, self._location: str | None = None, self._login_url: str | None = None, self._long_polling_url: str | None = None, self._pass_token: str | None = None, self._qr_image_url: str | None = None, and add -> None to __init__. The XiaomiCloudQrCode fields will also need to accept str | None, or be guarded so they're never None when constructed.

def __init__(self, session: aiohttp.ClientSession):
    super().__init__(session)
    self._cUserId = None
    self._location = None
    self._login_url = None
    self._long_polling_url = None
    self._pass_token = None
    self._qr_image_url = None

🟡 Important

1. _timeout never initialized; step_1 success path doesn't require a QR (`src/xiaomi_ble/cloud.py`, L409-416)

self._timeout is only assigned inside the if "qr" in response_data: branch of _login_step_1; it is never set in __init__. If the endpoint returns HTTP 200 but the body lacks "qr" (rate-limited, schema change, error payload), _login_step_1 still returns True, get_login_qrcode() returns XiaomiCloudQrCode(None, None) with no error, and the later login()_login_step_2 accesses self._timeoutAttributeError instead of a clean XiaomiCloudException.

Validate the body and fail loudly:

if valid:
    response_data = self.to_json(await response.text())
    if "qr" not in response_data:
        return False
    self._qr_image_url = response_data["qr"]
    ...

and initialize self._timeout in __init__ so the attribute always exists.

if valid:
    response_data = self.to_json(await response.text())
    if "qr" in response_data:
        self._qr_image_url = response_data["qr"]
        ...
        self._timeout = response_data["timeout"]
return valid
2. New auth flow is entirely untested (patch coverage 18%) (`src/xiaomi_ble/cloud.py`, L294-462)

Codecov reports 106 of the new lines uncovered and overall coverage dropping 1.49%. There is currently no test module for cloud.py at all, and none of the new QR path (QrCodeXiaomiCloudConnector, XiaomiCloudTokenFetch.get_login_qrcode, the password/QR split) is exercised. Given the subtle bugs above (polling loop, timeout units, uninitialized _timeout), tests would have caught them.

Please add tests that mock aiohttp.ClientSession and cover at minimum: (a) get_login_qrcode() returning a populated XiaomiCloudQrCode, (b) the success path through login() → step_2 → step_3, (c) the long-poll timeout/failure path, and (d) the existing password flow still working after the refactor (regression safety, since XiaomiCloudConnector was turned into an ABC). This also unblocks the maintainer's request to get the checks green.

3. session made optional but is dereferenced unconditionally (`src/xiaomi_ble/cloud.py`, L367-375)

XiaomiCloudTokenFetch.__init__ now defaults username, password, and session to None. session is still required for every code path — get_login_qrcode() passes it to QrCodeXiaomiCloudConnector(self._session) and get_device_info() passes it to the connector, both of which call self._session.get(...). A caller constructing XiaomiCloudTokenFetch() for the QR flow without a session gets an AttributeError deep inside _login_step_1 rather than a clear error at construction.

Keep session: aiohttp.ClientSession required (it's orthogonal to which auth method is chosen) and only make username/password optional, or validate session is not None in __init__.

def __init__(
    self,
    username: str | None = None,
    password: str | None = None,
    session: aiohttp.ClientSession | None = None,
) -> None:

🟢 Suggestions

1. serviceToken cookie may be absent; .value will raise (`src/xiaomi_ble/cloud.py`, L435-446)

response.cookies.get("serviceToken").value raises AttributeError if the cookie is missing (e.g. the location redirect didn't set it), because .get() returns None. The base/password connector has the same latent pattern, but this is a freshly added copy. Prefer guarding:

token = response.cookies.get("serviceToken")
if token is None:
    return False
self._serviceToken = token.value
if valid:
    self._serviceToken = response.cookies.get("serviceToken").value
2. Naming inconsistency and unused attributes between connectors (`src/xiaomi_ble/cloud.py`, L388-392)

The QR connector stores self._pass_token (snake_case) while the password connector and base use self._passToken (camelCase); pick one for consistency. Additionally, self._cUserId and self._pass_token are assigned from the long-poll response but never read anywhere in the QR flow — if they aren't needed for the downstream API calls, drop them to avoid implying they're used.

self._cUserId = None
self._pass_token = None

Checklist

  • No hardcoded secrets or credentials
  • Input validation at system boundaries — warning #3
  • Type annotations satisfy strict mypy (CI lint) — critical #2
  • Correct control flow / no dead code — critical #1
  • No resource/None-deref hazards in error paths — suggestion #6
  • New code branches are tested — warning #4
  • Consistent naming and no unused attributes — suggestion #7

Summary

The refactor to split XiaomiCloudConnector into an ABC with password and QR-code implementations is a reasonable design, and QR login is a genuinely useful addition for accounts that require a verification code. However the PR is not mergeable yet. Two blocking issues: (1) QrCodeXiaomiCloudConnector._login_step_2 has a dead timeout guard (start_time compared to itself) with no actual polling loop, and feeds Xiaomi's millisecond timeout directly into aiohttp's seconds-based timeout= kwarg — so the wait-for-scan logic doesn't work and can hang for hours; (2) the new connector's __init__ lacks a -> None annotation and its attributes are initialized to bare None, which fails the repo's strict mypy config — this is the failing lint check the maintainer asked about. Beyond those, _timeout is never initialized in __init__ (AttributeError if the QR body is missing), the success path of _login_step_1 doesn't actually require a qr field, session was made optional despite being dereferenced unconditionally, and the entire new flow is untested (18% patch coverage, no cloud.py test module exists). Fixing the type annotations and adding tests for both the QR and password paths will get the checks green and surface the polling/timeout bugs.


To rebase specific severity levels, mention me: @bluetoothbot rebase critical (fixes 🔴 only), @bluetoothbot rebase important (fixes 🔴 + 🟡), or just @bluetoothbot rebase for all.


Automated review by Kōan82c1586

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants