feat(acquisition): OpenSky OAuth2 client-credentials auth - #152
Open
montge wants to merge 1 commit into
Open
Conversation
OpenSky removed HTTP Basic authentication from its REST API; the `--credentials USER:PASS` path in the acquisition layer could no longer authenticate at all. This blocks flight-data-training-pipeline finding #1: anonymous access yields ~55 bbox polls/day (nominally 400 credits), which produced the cruise-dominated capture (0.06% coord_turn labels) that task 6.8's maneuvering-scenario failure traces back to. Add `acquisition.opensky_auth` implementing the `client_credentials` grant as an `httpx.Auth` flow: mints a bearer token, caches it until 60 s before expiry, and re-mints once on a 401 before replaying the request. Token requests travel over the caller's transport, so the whole flow is testable through a single `MockTransport` with no network or real credentials. Credentials resolve from `OPENSKY_CLIENT_ID`/`OPENSKY_CLIENT_SECRET` (matching the existing `ADSBX_API_KEY` convention), then from a JSON file defaulting to `~/.config/opensky/credentials.json` — the exact format OpenSky's web UI emits, accepted unmodified. Secrets are not accepted as CLI flags: `argv` is readable by other users via `ps`. The removed `--credentials` flag is retained as a hidden argument purely to emit a migration error, since it is otherwise an unambiguous argparse prefix of `--credentials-file` and would silently bind to it. `--time` (historical replay) now fails fast when unauthenticated instead of silently returning the current snapshot. Authenticated budget is 4,000 credits/day (8,000 for an active feeder) versus ~400 anonymous. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds OAuth2 client-credentials authentication for OpenSky, including credential resolution, token caching, expiry refresh, and 401 retry handling. Acquisition APIs and the CLI now accept auth objects, require authentication for historical windows, and document the new credential workflow. ChangesOpenSky OAuth2 authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI as opensky_cli.main
participant Loader as load_credentials
participant Auth as OAuth2ClientCredentialsAuth
participant Token as OpenSky token endpoint
participant API as OpenSky API
CLI->>Loader: resolve credentials
CLI->>Auth: build authentication object
Auth->>Token: request client_credentials token
Token-->>Auth: return bearer token
Auth->>API: send authenticated acquisition request
API-->>Auth: return response or 401
Auth->>Token: mint replacement token after 401
Auth->>API: replay request once
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
python/acquisition/opensky_auth.py (1)
143-224: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider thread-safety for the cached token state.
_token/_expires_atare mutated inauth_flowwithout any synchronization.httpx.Clientis explicitly documented as safe to share across threads, and httpx's own authentication docs recommend overridingsync_auth_flow()/async_auth_flow()with a lock specifically for OAuth2-style token caching across threads (the docs' canonical example wraps token minting in athreading.RLock/asyncio.Lock). If thisOAuth2ClientCredentialsAuthinstance (or thehttpx.Clientit's attached to) is ever reused from multiple threads — plausible for a general-purpose auth helper exported fromopensky_auth.py— two threads could simultaneously detect a stale token and both fire mint requests, or interleave in ways that leave_token/_expires_atinconsistent.Current call sites (
opensky_cli.capture/main) are single-threaded, so this isn't exploitable today, but it's worth hardening given the class is a reusable public API.🔒 Sketch using httpx's documented `sync_auth_flow` + lock pattern
+import threading ... self._token: str | None = None self._expires_at: float = 0.0 + self._lock = threading.Lock() ... - def auth_flow( + def sync_auth_flow( self, request: httpx.Request ) -> Generator[httpx.Request, httpx.Response, None]: - if not self._is_fresh(): - self._store_token((yield self._build_token_request())) + with self._lock: + if not self._is_fresh(): + self._store_token((yield self._build_token_request())) request.headers["Authorization"] = f"Bearer {self._token}" response = yield request if response.status_code == 401: - self._token = None - self._store_token((yield self._build_token_request())) - request.headers["Authorization"] = f"Bearer {self._token}" - yield request + with self._lock: + self._token = None + self._store_token((yield self._build_token_request())) + request.headers["Authorization"] = f"Bearer {self._token}" + yield request🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/acquisition/opensky_auth.py` around lines 143 - 224, Make OAuth2ClientCredentialsAuth thread-safe for shared synchronous use by adding a reentrant lock and synchronizing cached-token reads, refreshes, expiry updates, and 401 re-mint/replay handling in auth_flow. Prefer the httpx sync_auth_flow pattern so token minting is serialized without holding the lock across the yielded application request, while preserving single-refresh behavior and existing request authentication.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/acquisition/opensky_auth.py`:
- Around line 186-206: Update _store_token to validate that the parsed response
payload is a dict immediately after response.json(); raise OpenSkyAuthError with
an appropriate invalid-response message for lists or scalar JSON values, and
only then access payload.get or sort its keys.
---
Nitpick comments:
In `@python/acquisition/opensky_auth.py`:
- Around line 143-224: Make OAuth2ClientCredentialsAuth thread-safe for shared
synchronous use by adding a reentrant lock and synchronizing cached-token reads,
refreshes, expiry updates, and 401 re-mint/replay handling in auth_flow. Prefer
the httpx sync_auth_flow pattern so token minting is serialized without holding
the lock across the yielded application request, while preserving single-refresh
behavior and existing request authentication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 216399ee-f83d-4495-9c60-6d3d552e66dc
📒 Files selected for processing (7)
.gitignoreTRAINING.mdpython/acquisition/opensky.pypython/acquisition/opensky_auth.pypython/acquisition/opensky_cli.pypython/tests/test_opensky_auth.pypython/tests/test_opensky_sample.py
Comment on lines
+186
to
+206
| def _store_token(self, response: httpx.Response) -> None: | ||
| if response.status_code != 200: | ||
| detail = response.text[:500] | ||
| raise OpenSkyAuthError( | ||
| f"OpenSky token request failed: HTTP {response.status_code}: {detail}" | ||
| ) | ||
| try: | ||
| payload = response.json() | ||
| except ValueError as exc: | ||
| raise OpenSkyAuthError(f"OpenSky token response was not JSON: {exc}") from exc | ||
| token = payload.get("access_token") | ||
| if not token: | ||
| raise OpenSkyAuthError( | ||
| f"OpenSky token response has no access_token (keys: {sorted(payload)})" | ||
| ) | ||
| try: | ||
| lifetime_s = float(payload.get("expires_in", DEFAULT_TOKEN_LIFETIME_S)) | ||
| except (TypeError, ValueError): | ||
| lifetime_s = DEFAULT_TOKEN_LIFETIME_S | ||
| self._token = str(token) | ||
| self._expires_at = self._clock() + max(lifetime_s - self._expiry_margin_s, 0.0) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
_store_token assumes the token JSON payload is a dict.
payload = response.json() is never type-checked before payload.get(...) is called at Lines 196/199/202. If the OpenSky token endpoint (or a misconfigured token_url) returns valid JSON that isn't an object (e.g. a list or scalar), this raises an unhandled AttributeError instead of the intended OpenSkyAuthError, unlike _credentials_from_file which does guard with isinstance(payload, dict).
🐛 Proposed fix
try:
payload = response.json()
except ValueError as exc:
raise OpenSkyAuthError(f"OpenSky token response was not JSON: {exc}") from exc
+ if not isinstance(payload, dict):
+ raise OpenSkyAuthError(
+ f"OpenSky token response must be a JSON object, got {type(payload).__name__}"
+ )
token = payload.get("access_token")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _store_token(self, response: httpx.Response) -> None: | |
| if response.status_code != 200: | |
| detail = response.text[:500] | |
| raise OpenSkyAuthError( | |
| f"OpenSky token request failed: HTTP {response.status_code}: {detail}" | |
| ) | |
| try: | |
| payload = response.json() | |
| except ValueError as exc: | |
| raise OpenSkyAuthError(f"OpenSky token response was not JSON: {exc}") from exc | |
| token = payload.get("access_token") | |
| if not token: | |
| raise OpenSkyAuthError( | |
| f"OpenSky token response has no access_token (keys: {sorted(payload)})" | |
| ) | |
| try: | |
| lifetime_s = float(payload.get("expires_in", DEFAULT_TOKEN_LIFETIME_S)) | |
| except (TypeError, ValueError): | |
| lifetime_s = DEFAULT_TOKEN_LIFETIME_S | |
| self._token = str(token) | |
| self._expires_at = self._clock() + max(lifetime_s - self._expiry_margin_s, 0.0) | |
| def _store_token(self, response: httpx.Response) -> None: | |
| if response.status_code != 200: | |
| detail = response.text[:500] | |
| raise OpenSkyAuthError( | |
| f"OpenSky token request failed: HTTP {response.status_code}: {detail}" | |
| ) | |
| try: | |
| payload = response.json() | |
| except ValueError as exc: | |
| raise OpenSkyAuthError(f"OpenSky token response was not JSON: {exc}") from exc | |
| if not isinstance(payload, dict): | |
| raise OpenSkyAuthError( | |
| f"OpenSky token response must be a JSON object, got {type(payload).__name__}" | |
| ) | |
| token = payload.get("access_token") | |
| if not token: | |
| raise OpenSkyAuthError( | |
| f"OpenSky token response has no access_token (keys: {sorted(payload)})" | |
| ) | |
| try: | |
| lifetime_s = float(payload.get("expires_in", DEFAULT_TOKEN_LIFETIME_S)) | |
| except (TypeError, ValueError): | |
| lifetime_s = DEFAULT_TOKEN_LIFETIME_S | |
| self._token = str(token) | |
| self._expires_at = self._clock() + max(lifetime_s - self._expiry_margin_s, 0.0) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/acquisition/opensky_auth.py` around lines 186 - 206, Update
_store_token to validate that the parsed response payload is a dict immediately
after response.json(); raise OpenSkyAuthError with an appropriate
invalid-response message for lists or scalar JSON values, and only then access
payload.get or sort its keys.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
OpenSky removed HTTP Basic authentication from its REST API. The acquisition layer's
--credentials USER:PASSpath could no longer authenticate at all — it was dead code that silently degraded to anonymous access.This is the prerequisite for
flight-data-training-pipelinefinding #1: anonymous access delivers ~55 bbox polls/day (nominally 400 credits), which is what produced the cruise-dominated capture — 92.9%cv, 0.06%coord_turn— that task 6.8's maneuvering-scenario failure traces back to. A turn classifier cannot be trained from data containing no turns.Authenticated accounts get 4,000 credits/day (8,000 for an active feeder ≥30% uptime).
What
acquisition/opensky_auth.py— theclient_credentialsgrant as anhttpx.Authflow:401and replays the request; a second401propagates rather than loopingMockTransport— no network, no real credentials in testsOPENSKY_CLIENT_ID/OPENSKY_CLIENT_SECRET(matching the existingADSBX_API_KEYconvention) →~/.config/opensky/credentials.json→ anonymous. The file format is exactly what OpenSky's web UI emits, accepted unmodified (clientId/clientSecretand snake_case both work).fetch_state_vectors/fetch_current_states/capturetakeauth: httpx.Auth | Nonein place of the deadcredentialstuple.--time(historical replay) now fails fast when unauthenticated instead of silently handing back the current snapshot.Security notes
argvis readable by any user on the box viaps. The old--credentialsflag is retained as a hidden argument solely to emit a migration error: it is an unambiguous argparse prefix of--credentials-file, so without this it would silently bind to the new flag and report a confusing "file not found".OpenSkyCredentials.__repr__redacts the secret so it cannot leak into logs or tracebacks..gitignorenow coverscredentials.jsonand.env*.Testing
16 new tests in
tests/test_opensky_auth.pycovering resolution precedence, both file spellings, half-set env vars, malformed files, secret redaction, token caching, expiry-margin re-mint, 401 re-mint + replay, and both token-endpoint failure modes.ruff check✅ ·pyright0 errors ✅ ·pytest136 passed, 3 skipped ✅ (the 3 skips are pre-existing —torch/onnxruntimeare not in the CI dev group)Follow-on
Unblocks the authenticated capture over terminal areas with turning traffic, which then feeds the Track B retrain (6.8). The dt-feature work that pairs with it is on
feature/imm-dt-feature.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--credentials-fileto the OpenSky command-line tool.Documentation
Bug Fixes
Security
.env.example.