Send X-Authorization header after JWT login

[irynamatveieva]

No description provided.

[ViacheslavKlimov]

Review summary

Reviewed 5 changed files in Send X-Authorization header after JWT login. Left 3 comment(s) inline.

No correctness or security issues found — the change correctly seeds configuration.api_key["ApiKeyForm"] after JWT login so the per-request refresh hook starts firing, all four client editions (common, ce, pe, paas) carry an identical change, and the new tests genuinely pin the behavior (they exercise the real Configuration.auth_settings()/hook path, not a frozen value). The inline comments are maintainability nits: the seed logic and its explanatory comment duplicate the auth hook across four editions, and the new tests repeat login boilerplate that could be extracted into a helper.

---

This review was auto-generated. Findings may contain errors — please verify before applying changes.

[ViacheslavKlimov]

These two seed lines are a verbatim copy of the body of _AuthManager.hook (_auth.py lines 164-165: configuration.api_key["ApiKeyForm"] = token / configuration.api_key_prefix["ApiKeyForm"] = "Bearer"), and also duplicate the token= branch just below (lines 143-144). The ApiKeyForm/Bearer mapping now lives in four spots (this branch, the token= branch, the api_key branch with its ApiKey prefix, and the hook). Since the hook already encapsulates exactly this JWT-token-to-config mapping and is the single source of truth, could you seed by calling it once after on_login — e.g. auth_manager.hook(configuration) — or factor a small _seed_auth_header(configuration, token, prefix) helper the api_key/jwt/token branches share? That keeps the header format defined in one place so a future rename of the scheme name or prefix doesn't have to be chased across four call sites. Note this same block is replicated into the ce/pe/paas generated copies, so the fix belongs in common/ and regenerates outward.

[irynamatveieva]

Done in c33d2d7 — the JWT branch now seeds by calling the hook once:

auth_manager.on_login(username, password, response.token, response.refresh_token)
auth_manager.hook(configuration)

The hook stays the single source of truth for the ApiKeyForm/Bearer mapping, so a scheme or prefix rename is one edit. Checked it is safe to call at that point: auth_type is jwt in this branch, and _refresh_if_needed() returns immediately for a token that was just issued (estimated_server_time = iat + AVG_REQUEST_TIMEOUT_MS, far below exp), so no extra auth round-trip in __init__.

I did not convert the pre-existing token= branch, so the mapping is down to three sites rather than two. That branch is outside this PR's diff and routing it through the hook would change its behavior: an already-expired external token would trigger a refresh POST during construction instead of on the first request. Happy to do it as a separate change if you want it at two.

Applied in common/ and re-copied to the ce/pe/paas packages; all four client.py files remain byte-identical.

[ViacheslavKlimov]

The rationale here is genuinely useful (the chicken-and-egg with get_api_key_with_prefix is non-obvious), but eight comment lines for two lines of code is heavy, and it restates internals that could go stale if the generated Configuration changes. The block is copy-pasted verbatim into all four editions (ce/pe/paas are generated from common, so a fix in common/ regenerates outward), and its core explanation is echoed closely in the docstring of test_jwt_login_seeds_configuration_api_key and, more loosely, in the other two new tests. Would it read better condensed to a one- or two-line pointer with the long-form explanation living in a single place?

[irynamatveieva]

Condensed to four lines in c33d2d7, keeping the non-obvious part (the chicken-and-egg: the slot must already exist for the hook that fills it to run) and dropping the restated Configuration internals that could go stale:

# Seed the header slot by running the hook once: auth_settings() only
# emits X-Authorization when 'ApiKeyForm' is already in api_key, and
# the hook that fills it runs inside that same check — so unseeded, it
# never fires. After this the per-request hook keeps the header current.
auth_manager.hook(configuration)

The duplicate explanations in the new test docstrings are gone too — test_jwt_login_seeds_configuration_api_key now points here instead of repeating it, so the long-form rationale lives in one place.

[ViacheslavKlimov]

Each of the three new tests repeats the same four-line login boilerplate (_mock_login_response() + the with patch("tb_ce_client.api.login_endpoint_api.LoginEndpointApi.login", ...) block + construct client), and that same long patch-target string is now hardcoded in eight tests across this file. Consider a small helper such as _login_client(**kwargs) (or a setUp) that returns a logged-in client, so the tests keep only their distinct assertions. That also means the patch path lives in one spot if the login endpoint module ever moves.

[irynamatveieva]

Extracted in c33d2d7:

LOGIN_TARGET = "tb_ce_client.api.login_endpoint_api.LoginEndpointApi.login"

def _login_client(token=TOKEN, refresh_token=REFRESH_TOKEN, **kwargs):
    """Construct a JWT client with login() mocked; return (client, mock_login)."""
    with patch(LOGIN_TARGET, return_value=_mock_login_response(token, refresh_token)) as mock_login:
        client = ThingsboardClient(URL, "user@tb.io", "pass123", **kwargs)
    return client, mock_login

The helper returns the mock too, so the two tests that assert on call count keep working; the api_key=/token= tests that need assert_not_called use the LOGIN_TARGET constant directly. The patch path is now in one spot instead of eight, and each test is down to its distinct assertions. Net −52 lines in this file.

Verified rather than assumed: full suite 101 passed, unchanged from baseline. Mutation check — replacing auth_manager.hook(configuration) with pass fails exactly the three new tests, so the slimmed tests still pin the behavior through the real auth_settings() path. ruff check and ruff format --check clean on both edited files.

(tests/test_docs.py::test_ce_wheel_excludes_docs fails in my sandbox for lack of a poetry binary, both before and after the change.)

[ViacheslavKlimov]

Re-review summary

Re-reviewed Send X-Authorization header after JWT login — verified 3 findings from the previous review.

Status	Count
✅ Resolved	3

All three previous findings have been addressed. Also spotted 2 new observations while reviewing the fix commits — one inline, one under Additional findings below.

Finding details

• ✅ common/client.py:133 — JWT-login branch duplicated the hook's ApiKeyForm/Bearer seed logic — Fixed: the two seed lines were replaced with a single auth_manager.hook(configuration) call, so the hook stays the sole source of the mapping.
• ✅ common/client.py:129 — Eight-line comment restated Configuration internals and was duplicated across editions and test docstrings — Fixed: condensed to four lines keeping only the chicken-and-egg rationale.
• ✅ tests/test_client.py:51 — Three new tests repeated the login boilerplate and hardcoded the patch-target string — Fixed: extracted a _login_client() helper and LOGIN_TARGET constant.

Additional findings

This observation is about existing code just outside the fix's scope — spotted while reading surrounding context.

• common/client.py:138 — This follow-up nicely de-duplicated the JWT-login branch by delegating header seeding to auth_manager.hook, but the adjacent pre-existing-token branch still hand-sets the very same slots (api_key["ApiKeyForm"] = token / api_key_prefix["ApiKeyForm"] = "Bearer"). Since that branch also runs with auth_type == "jwt", set_external_token(...) followed by auth_manager.hook(configuration) would seed it identically. The two JWT paths now seed the header two different ways — exactly the divergence the refactor set out to remove — and the "Bearer" string plus the "ApiKeyForm" magic key still live in client.py here. Worth considering folding this branch into the same hook call for consistency (applies to all four ce/pe/paas/common copies).

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[ViacheslavKlimov]

Reusing the existing hook to seed the header is a clean move, but invoking a method named hook imperatively reads a little oddly at a call site — earlier in the same constructor the identical callable is registered as configuration.refresh_api_key_hook, so a future reader sees the callback both wired up and called by hand. Would a more intention-revealing name (e.g. a thin seed_auth_header(configuration) wrapper, or renaming hook to apply_auth_header) let the code say what the comment currently has to?