Skip to content

Networking foundation: OpenAPI codegen, HTTP clients, encrypted token storage#3

Open
koinsaari wants to merge 3 commits into
mainfrom
feat/openapi-codegen
Open

Networking foundation: OpenAPI codegen, HTTP clients, encrypted token storage#3
koinsaari wants to merge 3 commits into
mainfrom
feat/openapi-codegen

Conversation

@koinsaari
Copy link
Copy Markdown
Contributor

Summary

Three foundational pieces for the networking + auth layer. None depend on each other; they're grouped because they're all plumbing the app needs before any feature talks to api-proxy.

1. OpenAPI codegen — feat: wire openapi-generator with vendored api-proxy spec

  • Vendor api-proxy/api/openapi.yaml to openapi/openapi.yaml; first three lines record the source commit SHA so refreshes are auditable.
  • openApiGenerate runs on every preBuild, emitting Retrofit interfaces + kotlinx.serialization models into app/build/generated/openapi/ (gitignored).
  • Pinned to generator 7.21.0 — 7.22.0 has a Gradle plugin regression that emits broken Room model references.
  • A doLast step deletes ApiClient.kt and the empty auth/ dir from generated output. ApiClient.kt pulls in retrofit's converter-scalars / converter-moshi which we don't depend on, and we construct Retrofit ourselves in HttpClients.

2. HTTP clients — feat: add HttpClients for OkHttp + Retrofit + kotlinx.serialization

HttpClients hands out two flavours of OkHttpClient (raw and authed) plus a Retrofit builder pre-wired with the kotlinx.serialization converter. Base URL is https://api.stoganet.com/, timeouts are 10s connect / 15s I/O, body logging is debug-only.

3. Encrypted token storage — feat: encrypted TokenStore via Proto DataStore + Tink AEAD

  • app/src/main/proto/tokens.proto — schema for {access, refresh, user_id, display_name}; protobuf gradle plugin compiles it via javalite only (no Kotlin DSL — we use the Java builder API directly).
  • TokensSerializer adapts DataStore's Serializer<T> contract: bytes in → Tink AES-256-GCM decrypt → Tokens.parseFrom; and the reverse on write. The Tink keyset is wrapped by a hardware-backed Android Keystore master key, so the on-disk stoganet-tokens.pb is unreadable without the device.
  • TokenStore is the public surface — Flow<Tokens>, current(), save(...), clear().

Cross-cutting

  • Tightened detekt: maxLineLength 140 → 120, enabled ExpressionBodySyntax (with includeLineWrapping=true) so single-return functions stay in = form.

Test plan

  • ./gradlew :app:openApiGenerate — emits 9 models + DefaultApi (auth + healthz)
  • ./gradlew :app:assembleDebug — clean build with generated code
  • ./gradlew detekt — clean

koinsaari and others added 3 commits May 31, 2026 20:19
Vendor api-proxy/api/openapi.yaml under openapi/ with the source commit SHA
recorded at the top. openApiGenerate emits Retrofit interfaces +
kotlinx.serialization models into build/generated/openapi/ on every preBuild;
the generated dir is gitignored.

Pin generator to 7.21.0 — 7.22.0 has a regression that emits broken Room model
references (#23658). Skip the supporting ApiClient.kt and auth/HttpBearerAuth.kt
since HttpClients constructs Retrofit itself in the next commit; ApiClient
otherwise pulls in retrofit-converter-scalars that we do not use.
Tokens schema (access, refresh, user_id, display_name) lives in tokens.proto,
compiled via the protobuf gradle plugin into javalite bindings only — we use the
Java builder API directly, no Kotlin DSL. The DataStore serializer wraps Tink's
AES-256-GCM Aead primitive whose keyset is unwrapped from the Android Keystore at
first use, so the on-disk blob is unreadable without the device's hardware-backed
master key.

Also tighten detekt: maxLineLength 140 -> 120, enable ExpressionBodySyntax so
single-return functions stay in expression-body form, and correct the HttpClients
signature flagged by ktlint.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented May 31, 2026

Claude finished @koinsaari's task in 4m 38s —— View job


Code Review

  • Gather context (REVIEW.md, CLAUDE.md)
  • Read changed files
  • Analyze for security, correctness, and architecture issues
  • Post review findings
  • Set commit status

No blocking issues. The crypto + networking foundation is solid and the architecture invariants are all respected.

Security checklist

Check Result
AuthInterceptor recursion (separate refreshClient) N/A — not implemented yet; rawOkHttp() correctly provides the interceptor-free client for that path
refreshMutex serialisation N/A — not implemented yet
All token access through TokenStore ✅ — proto DataStore + Tink AEAD; no SharedPreferences or raw file I/O for tokens
No raw Authorization header construction ✅ — none in this PR
OkHttp interceptor body logging ✅ — Level.BASIC logs only method+URL+status; no headers, no body; debug-only
New API call sites through a Repository N/A — no call sites yet

TokensSerializer.kt

The AEAD setup is correct: AES256_GCM keyset wrapped by android-keystore://stoganet_master_key, so the on-disk .pb is bound to the device. ASSOCIATED_DATA = "stoganet-tokens-v1" is proper authenticated encryption hygiene — the version suffix means a future format change is detectable rather than silently producing garbage.

One behavioral note for follow-up layers: if aead.decrypt() throws GeneralSecurityException (corrupted file, key lost after factory reset), DataStore's data flow permanently fails. At that point clear() itself throws because updateData reads first. The recovery path is deleting the file. This is inherent to encrypted DataStore and not a bug here, but whichever layer catches auth errors will need to know that TokenStore.clear() is not always sufficient.

HttpClients.kt

Clean design. rawOkHttp() / authedOkHttp() separation sets the right contract for the future AuthInterceptor — the refresh path must use the raw client, and the plumbing is already in place.

🟡 Nit (HttpClients.kt:25,27) — Both authedOkHttp() and rawOkHttp() call baseBuilder().build(), allocating a fresh OkHttpClient (thread pool + connection pool) on every invocation. OkHttp docs say the client should be a shared singleton. ServiceLocator must call each factory method exactly once and cache the result; there's no enforcement here. Fix this →

TokenStore.kt

DataStoreFactory.create throws IllegalStateException if two instances are created for the same file path. The create() companion factory doesn't guard against double-creation (and can't easily do so without a static guard). As with HttpClients, ServiceLocator must treat this as a one-shot factory. The two constraints together suggest they belong in the same "wiring" note when ServiceLocator is written.

app/build.gradle.kts

The codegen wiring is well-considered — pinned generator version with an issue citation, preBuild dependency, and manual deletion of the unused ApiClient.kt and empty auth/ dir.

🟡 Nit (build.gradle.kts:85-93) — unusedSupportingFiles paths are hardcoded strings. If the generator output layout changes in a future version (e.g., when the 7.22 regression is fixed and the pin is bumped), deleteRecursively() silently no-ops on a missing path, leaving ApiClient.kt in the output tree and breaking the build at compile time rather than at the deletion step. A check or logger.warn when the target doesn't exist would make the failure visible at the right point. Fix this →

openapi/openapi.yaml

Source SHA on line 1 is a nice touch for refresh auditability. Spec coverage looks correct for the current surface.

config/detekt/detekt.yml + gradle/libs.versions.toml

Line-length tightening (140 → 120) and ExpressionBodySyntax look right. No issues.


Summary: No blocking issues. Two nits around missing singleton enforcement in the factory methods and silent deletion failures in the codegen cleanup step. The crypto plumbing is correct, the OkHttp logging is safe at BASIC level, and the architecture invariants are all properly respected.

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.

1 participant