Skip to content

feat: add OAuth support for cloud APIs - #563

Merged
zerzhang merged 8 commits into
sblibs:mainfrom
zerzhang:codex/switchbot-oauth2
Sep 15, 2026
Merged

zerzhang merged 8 commits into
sblibs:mainfrom
zerzhang:codex/switchbot-oauth2

Conversation

@zerzhang

@zerzhang zerzhang commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Human responsible: @zerzhang.

Summary

  • Add helpers for building SwitchBot OAuth authorization URLs and exchanging authorization codes.
  • Support access-token-based cloud device discovery and encryption-key retrieval.
  • Validate provider responses while keeping authorization codes, access tokens, refresh tokens, and encryption keys out of logs.
  • Preserve authentication and API error types through device and encryption-key requests.
  • Document the public-client OAuth contract and caller responsibilities.

OAuth wire contract

  • The caller supplies a SwitchBot-issued client ID and its registered redirect URI; pySwitchbot does not embed Home Assistant credentials.
  • This is a public-client flow. Open-source consumers cannot keep a client secret, so the token exchange intentionally does not send one.
  • SwitchBot's current authorization server does not support PKCE. Callers must still generate, store, and validate a single-use state; state does not replace PKCE.
  • The returned access token is sent directly in the authorization header expected by the SwitchBot internal account endpoints.
  • The current Home Assistant setup flow consumes the token transiently and does not persist it, so this change does not add an unverified refresh request helper.

Error semantics

HTTP 401 and 403 responses now surface as SwitchbotAuthenticationError, including on the pre-existing password flow. Other provider API failures surface as SwitchbotApiError; transport and service-availability failures use SwitchbotAccountConnectionError.

Testing

  • poetry run pytest --cov=switchbot tests
  • 1403 passed
  • switchbot/oauth.py and the new shared request-ID helper are fully covered

@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
switchbot/__init__.py 100.00% <100.00%> (ø)
switchbot/devices/device.py 75.51% <100.00%> (+5.60%) ⬆️
switchbot/oauth.py 100.00% <100.00%> (ø)
switchbot/utils.py 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@zerzhang zerzhang changed the title Add OAuth support for cloud APIs feat: add OAuth support for cloud APIs Sep 3, 2026
@bluetoothbot

bluetoothbot commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Previous review — superseded by a newer review below.

@bluetoothbot bluetoothbot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Warning

Important issues found.

  • Token exchange sends no client authentication and no PKCE — and the helper signature can't express either
  • Unknown-model diagnostic loses the payload that exists to add new device support

@zerzhang

zerzhang commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the detailed review. I addressed the diagnostic and error-handling findings in 2d7dbce and clarified the OAuth contract in the README and PR description.

On the OAuth wire contract: there is not currently a public SwitchBot OAuth document I can link. The implementation follows the SwitchBot-provisioned public-client flow used by the current consumer. client_id and the registered redirect_uri are supplied by the caller and are not embedded in pySwitchbot. Because the consumers are open source, a client_secret cannot be kept confidential and is intentionally not sent. SwitchBot's current authorization server also does not support PKCE, so I documented that limitation rather than exposing parameters the server cannot use. The caller must generate and validate a single-use state; the README explicitly notes that state does not replace PKCE.

Changes made from the review:

  • Restored the original full unknown-model payload and unmasked MAC diagnostic.
  • Preserved SwitchbotAuthenticationError through the later device-list and encryption-key requests.
  • Preserved SwitchbotApiError from the key endpoint and classify malformed communicationKey responses as API errors instead of connection errors.
  • Documented the 401/403 exception-type change for the existing password flow.
  • Parse only bounded error and error_description fields from OAuth failures, redact an echoed authorization code, and never log the raw response.
  • Moved the duplicated request-ID extraction into a shared helper.
  • Relaxed exact debug-message assertions while retaining the negative secret-leakage assertions.
  • Added tests for the new authentication, API, connection, malformed-response, and OAuth error paths.

The token returned by this flow is expected directly in the SwitchBot internal account endpoints' authorization header, without a Bearer prefix. The current Home Assistant setup flow uses the token transiently and does not persist it. I therefore did not add a refresh helper without a verified refresh wire contract.

Local result: poetry run pytest --cov=switchbot tests — 1403 passed; the OAuth module and shared request-ID helper are fully covered.

@bluetoothbot

Copy link
Copy Markdown
Collaborator

@zerzhang: Thanks for the detailed review. I addressed the diagnostic and error-handling findings in 2d7dbce ...

Verified 2d7dbce against head. Unknown-model payload restored, device.py:404-411. Auth passthrough now covers getdevice and key endpoint. Request-ID extraction deduped into utils.extract_request_id. Malformed communicationKey correctly classified.

PKCE point accepted as a documented server limitation, not re-raised as blocking. Residual risk stays real: an intercepted code remains redeemable by anyone. Worth documenting that redirect_uri must be an exact HTTPS endpoint, no wildcards.

Two follow-ups from this pass. README line 89 still quotes Failed to retrieve encryption key from SwitchBot Account: ...; that string no longer appears for statusCode 190 now that SwitchbotApiError propagates raw. And every non-429 4xx from the token endpoint maps to SwitchbotAuthenticationError, so a redirect_uri mismatch drives a reauth loop rather than surfacing as a config error.

@bluetoothbot

bluetoothbot commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Previous review — superseded by a newer review below.

bluetoothbot
bluetoothbot previously approved these changes Sep 3, 2026

@bluetoothbot bluetoothbot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Tip

No blocking issues found — ready to merge.

@bdraco

bdraco commented Sep 13, 2026

Copy link
Copy Markdown
Member

@zerzhang You should have access to merge this once the bot approves. There are some nits above to address though

@bluetoothbot

bluetoothbot commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

PR Review — feat: add OAuth support for cloud APIs

Merge-ready. Every earlier suggestion and silent-failure finding is resolved; only two small structural cleanups are left.

Confirmed fixed in 63e9be1:

  • The README troubleshooting entry now quotes ..., status code: 190 (SwitchbotApiError).
  • Token-endpoint errors are classified properly: 401/403 and 400 invalid_client/invalid_grant raise an auth error, other 4xx raise SwitchbotApiError, and 429/5xx raise a connection error. Tests cover each case, including a 499 with invalid_grant.
  • expires_in is returned as an integer on a copy of the token, so the provider's dict isn't changed.
  • The README example passes token["access_token"].
  • extract_request_id is case-insensitive and has direct parametrized tests, including cf-ray and empty headers.
  • A missing Items in the device response raises SwitchbotApiError.
  • An unparsable OAuth error body now logs the exception type.
  • A missing region logs a warning.

What's done well: the password and token flows share _async_get_devices and _async_retrieve_encryption_key, so there's no second copy to drift. Validation happens at the boundary: Items, communicationKey, access_token and expires_in are all type-checked and raise SwitchbotApiError instead of a raw KeyError. Negative caplog checks confirm that tokens, the authorization code, device MACs and encryption keys never appear in logs.

Remaining (non-blocking):

  • The auth/API re-raise pair is written out three times; a tuple clause would replace each pair.
  • The OAuth entry points add timing and failure logs that mostly repeat api_request's per-request logging, adding about 45 lines to a 1504-line device.py.

✅ Resolved since last review (4)

Previously-flagged issues verified fixed
  • README.md:89 README troubleshooting quotes an error message the key path no longer emits
  • switchbot/oauth.py:131 Every non-429 4xx from the token endpoint becomes an authentication error
  • switchbot/oauth.py:142 expires_in is validated and parsed, then thrown away
  • README.md:47 OAuth example leaves the access-token extraction implicit

🟢 Suggestions

1. Same two-clause passthrough appears three times; one tuple clause would do
switchbot/devices/device.py:278-281

These two back-to-back re-raise clauses show up in three places:

  • _async_get_user_info (278-281)
  • _async_get_devices (375-378)
  • _async_retrieve_encryption_key (1218-1221)

Each one contains:

except SwitchbotAuthenticationError:
    raise
except SwitchbotApiError:
    raise

The two exceptions are unrelated RuntimeError subclasses (switchbot/const/__init__.py:36,45), so the clauses behave exactly like one tuple clause. Spelling them out six times makes a simple rule look like two separate decisions, and a future call site could copy one clause and drop the other.

Fix: collapse each pair to except (SwitchbotAuthenticationError, SwitchbotApiError): raise. This is non-blocking and doesn't change behaviour.

        except SwitchbotAuthenticationError:
            raise
        except SwitchbotApiError:
            raise
        except Exception as err:
            raise SwitchbotAccountConnectionError(
2. Token entry points wrap the shared helpers only to add timing logs that `api_request` already emits
switchbot/devices/device.py:327-353

get_devices_by_token (327-353) and async_retrieve_encryption_key_by_token (1154-1186) are each about 25 lines. Their real work is a single call that builds {"authorization": access_token}. The rest is a monotonic() start time, a failure log inside except Exception: raise, and a completion log.

That logging mostly repeats what the PR already added elsewhere:

  • api_request logs duration_ms and request_id for every HTTP call.
  • _async_get_devices logs the record count and the mapped count.
  • _async_retrieve_encryption_key logs the masked device on success.

A failed OAuth fetch therefore logs its duration twice, and the password flow gets different log output from the token flow for the same shared code path. device.py is already 1504 lines, and AGENTS.md asks for a low comment and prose bar.

Fix: cut each token entry point down to the delegation call and let the shared helpers and api_request do the logging. That keeps both flows the same and removes about 45 lines. Non-blocking.

        started = time.monotonic()
        _LOGGER.debug("Retrieving SwitchBot cloud devices using an OAuth token")
        try:
            devices = await cls._async_get_devices(
                session, {"authorization": access_token}
            )
        except Exception:
            _LOGGER.debug(
                "SwitchBot OAuth cloud device retrieval failed; duration_ms=%s",
                round((time.monotonic() - started) * 1000),
            )
            raise

Checklist

  • No hardcoded secrets or credentials
  • Secrets kept out of logs (tokens, codes, keys)
  • Input validation at system boundaries
  • Error handling: no swallowed exceptions, correct taxonomy
  • No resource leaks (sessions/responses closed)
  • Documentation matches implemented behaviour
  • Diff matches the PR description (no scope creep)
  • Structural quality / no redundant layers — suggestion #1, suggestion #2

Silent Failure Analysis

🟡 **3. MEDIUM** — unvalidated response envelope / error misclassification
switchbot/devices/device.py:461-481

Risk: api_request does not check that the JSON is an object or that body is a dict. The debug log already expects body might not be a dict. A malformed but successful HTTP response then fails in one of two ways: an AttributeError/KeyError inside the callers' except Exception blocks gets reported as SwitchbotAccountConnectionError, which looks like a temporary outage and gets retried, or the non-dict body is returned and breaks later outside any handler.

response = await result.json()
body = response.get("body")
...
if response["statusCode"] != 100:
    raise SwitchbotApiError(...)
return response["body"]

Fix: Check that response is a dict with a statusCode and that body is a dict before returning. Raise SwitchbotApiError("Invalid response from SwitchBot API") when either check fails, so the error types listed in the README stay accurate.

🟡 **4. MEDIUM** — unclassified exception escaping documented contract
switchbot/devices/device.py:361-362

Risk: _extract_region(userinfo), device_info.get("Items"), and device_info.get("communicationKey") run outside the try blocks. If api_request returns a null or non-dict body, a raw AttributeError reaches the caller instead of SwitchbotApiError, so a Home Assistant config flow would show an 'unknown error' rather than a handled API failure.

userinfo = await cls._async_get_user_info(session, auth_headers)
region = _extract_region(userinfo)
...
items = device_info.get("Items")
...
communication_key = device_info.get("communicationKey")

Fix: Add isinstance(userinfo, dict) / isinstance(device_info, dict) checks next to the new Items and communicationKey checks, and raise SwitchbotApiError when they fail. Better still, validate once centrally in api_request.

🟡 **5. MEDIUM** — fallback value hiding failure
switchbot/devices/device.py:55-61

Risk: The new OAuth token path relies on the userinfo response containing botRegion, which is not proven for OAuth tokens. If it is missing or not a string, an EU or JP account is silently sent to the US endpoint. The result can be an empty device list or a status 190 error, which the README now describes as a 'not the device owner' problem, so the real cause is hidden behind a single warning.

region = userinfo.get("botRegion")
if isinstance(region, str) and region:
    return region
_LOGGER.warning("SwitchBot account region missing; defaulting to us")
return "us"

Fix: Include the region fallback in the result or error message: for example, raise or add context when a defaulted region returns zero devices or a 190 status. At minimum, log which fields userinfo actually contained so the fallback can be diagnosed.

🟡 **6. MEDIUM** — catch-all misclassification
switchbot/devices/device.py:295-309

Risk: This is pre-existing, but this PR now documents distinct exception types. In the password flow, timeouts, connection errors, 5xx responses, and the new SwitchbotApiError are all turned into SwitchbotAuthenticationError, so a network outage looks like bad credentials and can trigger needless re-auth prompts.

try:
    auth_result = await cls._get_auth_result(session, username, password)
    auth_headers = {"authorization": auth_result["access_token"]}
except Exception as err:
    raise SwitchbotAuthenticationError(f"Authentication failed: {err}") from err

Fix: Let SwitchbotApiError pass through unchanged, and map aiohttp.ClientError/TimeoutError to SwitchbotAccountConnectionError in _get_auth_result and get_devices/async_retrieve_encryption_key, the same way the new token-based paths do.


Automated review by Kōan (Claude) HEAD=63e9be1 2 min 42s

@bluetoothbot bluetoothbot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Tip

No blocking issues found — ready to merge.

@zerzhang
zerzhang merged commit 3c2b1c0 into sblibs:main Sep 15, 2026
10 checks passed
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.

3 participants