The app captured Gitea's OAuth refresh token on login but never used it -- an expired access token forced a full new browser-based login (and consent screen) every time. Now silently exchanges the stored refresh token for a new access token in the background, proactively before it goes stale, shared via one AuthSession singleton across all five features' API clients instead of each independently reading a possibly-stale token. Verified Gitea's refresh_token grant shape against its own source (routers/web/auth/oauth2_provider.go) before implementing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
172 lines
12 KiB
Markdown
172 lines
12 KiB
Markdown
# TASK-029: OAuth refresh-token support (stop re-prompting for consent every session)
|
|
|
|
Status: implemented; all required checks pass; live device verification pending
|
|
Owner: Claude
|
|
Reviewer: Codex
|
|
|
|
## Outcome
|
|
|
|
The app previously captured Gitea's OAuth refresh token on login but never actually used it: once
|
|
the access token's stated expiry passed, `RepositoryListViewModel.restoreSession()` cleared the
|
|
session outright and forced the user through a brand-new `authorization_code` flow — browser open,
|
|
Gitea login, and (per the human product owner's direct complaint, "애플리케이션 승인 안하고 로그인
|
|
쭉 이어서 되어있게 해주면 안되나?") the "Authorize this application?" consent screen — every time.
|
|
The app now silently exchanges the stored refresh token for a new access token in the background
|
|
before it's needed, never opening a browser or showing any Gitea UI, which is what the standard
|
|
OAuth `refresh_token` grant is specifically designed to do without ever re-prompting for consent.
|
|
|
|
## Scope
|
|
|
|
- Included: `data/auth/AuthApi.kt` (`AuthenticationGateway.refreshToken`, its `AuthApi`
|
|
implementation, shared token-response parsing extracted from `exchangeCode`), new
|
|
`core/auth/android/RefreshingTokenProvider.kt` (the actual refresh-before-stale logic, shared
|
|
Mutex for single-flight refresh), new `core/auth/android/AuthSession.kt` (one shared
|
|
`TokenVault`/`RefreshingTokenProvider` per process, replacing five independent
|
|
`AndroidKeystoreTokenVault`/`BearerTokenProvider` constructions), `feature/repositories/RepositoryListViewModel.kt`
|
|
(`tokenProvider` param, `restoreSession()`/`loadRepositories()` no longer short-circuit on a
|
|
stale clock check), and the `Factory` classes of `CodeBrowserViewModel`, `HistoryViewModel`,
|
|
`WorkItemsViewModel`, `NotificationsViewModel`, `RepositoryListViewModel` (all five now share
|
|
`AuthSession`'s vault/provider instead of each building its own disconnected one).
|
|
- Excluded: No change to the initial `authorization_code` login flow itself (TASK-026's Custom
|
|
Tabs change is untouched), no change to what's stored or how (still the same
|
|
`AndroidKeystoreTokenVault` encryption, just one shared instance instead of five), no reactive
|
|
"retry the request once after a 401" fallback — this task is proactive-refresh only (see Known
|
|
limitations).
|
|
|
|
## Design notes
|
|
|
|
- **Verified against Gitea's own source before implementing**, per this project's established
|
|
convention for anything not directly provable from the client: `routers/web/auth/oauth2_provider.go`'s
|
|
`handleRefreshToken` confirms the `refresh_token` grant reads `grant_type`, `refresh_token`, and
|
|
`client_id` (no `client_secret` needed for a public/PKCE client — only confidential clients are
|
|
validated against one), and reuses the same `NewAccessTokenResponse` response shape as the
|
|
existing, already-working `authorization_code` exchange. Gitea's own refresh-token reuse
|
|
detection (`grant.Counter`-based) confirms it rotates refresh tokens on each use, which is why
|
|
the response's `refresh_token` (when present) always replaces the one just used, not the other
|
|
way around.
|
|
- **Proactive, not reactive**: `RefreshingTokenProvider` refreshes a token that's within 60 seconds
|
|
of its stated expiry *before* handing it out, rather than waiting for an API call to actually
|
|
fail with a 401 first. This matters specifically because the refresh-aware provider is shared
|
|
(see below) across five independent feature API clients — proactively refreshing means whichever
|
|
feature happens to make the next call already sees a fresh token, instead of each one separately
|
|
racing a token that's already gone stale and only some of them noticing.
|
|
- **One shared `AuthSession` singleton, not five independent instances**: previously every
|
|
`ViewModel.Factory` built its own `AndroidKeystoreTokenVault` and `BearerTokenProvider` from
|
|
scratch, meaning a token refreshed by (say) the repository list would never be visible to the
|
|
code browser's independent copy. `AuthSession` holds exactly one `TokenVault` and one
|
|
`RefreshingTokenProvider` per process; every Factory now pulls from it instead. This also makes
|
|
the provider's internal `Mutex` actually meaningful — it stops concurrent callers *across
|
|
different features* from independently trying to consume the same single-use, rotating refresh
|
|
token, which would make every refresh but the first one fail.
|
|
- **`RepositoryListViewModel.restoreSession()`'s old "expiresAtEpochMillis already past -> clear
|
|
and fail immediately" check was removed outright**, not adjusted, since it actively worked
|
|
against refresh support: it fired *before* the (now refresh-capable) API call ever had a chance
|
|
to run. The exact same fallback behavior it existed for — clearing the session and surfacing
|
|
`AUTHENTICATION_EXPIRED` — still happens today, just now driven by a genuine failed API call
|
|
(refresh attempted and failed, or no refresh token available) rather than a client-side clock
|
|
guess made before ever trying.
|
|
- **`RepositoryListViewModel`'s constructor gained a `tokenProvider` parameter defaulted to the old
|
|
no-refresh behavior** (`BearerTokenProvider { tokenVault.read()?.accessToken.orEmpty() }`)
|
|
specifically so no existing test needed to change — only production wiring (the `Factory`)
|
|
explicitly passes the shared refresh-aware one.
|
|
|
|
## Acceptance criteria
|
|
|
|
- [x] `AuthApi.refreshToken` sends `grant_type=refresh_token`, `client_id`, `refresh_token`, and no
|
|
`client_secret`, to the same `/login/oauth/access_token` endpoint as the existing code
|
|
exchange.
|
|
- [x] A successful refresh response's `access_token`/`refresh_token`/`expires_in` are parsed and
|
|
persisted; a response omitting `refresh_token` falls back to reusing the one just sent rather
|
|
than losing refresh capability entirely.
|
|
- [x] `RefreshingTokenProvider` returns the stored token unchanged when it isn't close to expiry,
|
|
refreshes (and persists the result) once within 60 seconds of expiry, falls back to the stale
|
|
token — without attempting a network call — when no refresh token is stored, and falls back
|
|
to the stale token (leaving the vault untouched) when the refresh attempt itself fails.
|
|
- [x] A token with no `expiresAtEpochMillis` at all is never treated as due for refresh.
|
|
- [x] `RepositoryListViewModel.restoreSession()` authenticates through the injected token provider
|
|
(not a stale value read once at the top of the function) — proven directly by a test using a
|
|
deliberately-wrong stored token alongside a provider that returns a different one.
|
|
- [x] Every existing test continues to pass unmodified except the two direct `AuthenticationGateway`
|
|
fakes, which now also implement the new interface method.
|
|
- [x] Build, unit tests, and lint pass.
|
|
- [ ] Live device confirmation that a session now survives past what used to force a full
|
|
re-login/re-approval — not yet observed over a long enough idle period this session; see
|
|
Known limitations.
|
|
|
|
## Security and privacy
|
|
|
|
- No new data is stored beyond what `TokenSet` already held (`refreshToken` was already part of
|
|
the persisted, Android-Keystore-encrypted `TokenSet`; this task is the first code to actually use
|
|
the field, not the first to store it).
|
|
- Refresh failures fail closed: any error refreshing falls back to the *existing* stale token
|
|
(which then draws a normal 401 from Gitea, handled exactly as before) — never to a broadened
|
|
scope, an unencrypted fallback, or a silently-extended session past what the server itself allows.
|
|
- `client_secret` is deliberately never sent for this public/PKCE client's refresh calls, matching
|
|
the existing `authorization_code` exchange and Gitea's own documented requirement that only
|
|
confidential clients need one.
|
|
|
|
## Required checks
|
|
|
|
```text
|
|
./gradlew assembleDebug
|
|
./gradlew testDebugUnitTest
|
|
./gradlew lintDebug
|
|
```
|
|
|
|
## Evidence
|
|
|
|
- `data/auth/AuthApi.kt`: `AuthenticationGateway.refreshToken(refreshToken, nowEpochMillis)`;
|
|
`AuthApi`'s implementation and `exchangeCode` now share one `parseTokenResponse(...)` helper
|
|
(`fallbackRefreshToken` parameter distinguishes "nothing to fall back to" for a fresh code
|
|
exchange from "reuse the token just sent" for a refresh).
|
|
- `core/auth/android/RefreshingTokenProvider.kt` (new): the refresh-before-stale `BearerTokenProvider`
|
|
implementation described above.
|
|
- `core/auth/android/AuthSession.kt` (new): the shared singleton wiring.
|
|
- `feature/repositories/RepositoryListViewModel.kt`: `tokenProvider` constructor param (defaulted);
|
|
`restoreSession()`'s early-expiry short-circuit removed; `loadRepositories()` takes no arguments
|
|
and reads `tokenProvider.accessToken()` instead of a captured `TokenSet.accessToken`; `Factory`
|
|
wired to `AuthSession`.
|
|
- `CodeBrowserViewModel.kt`, `HistoryViewModel.kt`, `WorkItemsViewModel.kt`,
|
|
`NotificationsViewModel.kt`: each `Factory` now calls `AuthSession.tokenProvider(application)`
|
|
instead of constructing its own `AndroidKeystoreTokenVault`/`BearerTokenProvider` pair.
|
|
- New tests: `AuthApiTest.kt` (refresh sends the right grant/fields and no client secret, response
|
|
without a `refresh_token` falls back to the one sent, blank refresh token rejected before any
|
|
request); `RefreshingTokenProviderTest.kt` (not-yet-expired passthrough, refresh-and-persist
|
|
within the skew window, proactive-before-actual-expiry, no-refresh-token fallback, failed-refresh
|
|
fallback leaving the vault untouched, empty-string when nothing stored, no-expiry-never-refreshes);
|
|
`RepositoryListViewModelTest.kt` (new test proving `restoreSession` authenticates through the
|
|
injected provider rather than a raw stored token; `FakeAuthGateway` updated to implement the new
|
|
interface method and capture the token it was called with).
|
|
- `./gradlew compileDebugKotlin compileDebugUnitTestKotlin`: BUILD SUCCESSFUL (one intermediate
|
|
failure — a second, missed `loadRepositories(tokens)` call site in `authenticate()` after the
|
|
signature change — found and fixed before this).
|
|
- `./gradlew testDebugUnitTest`: BUILD SUCCESSFUL, all 231+ unit tests passing (two intermediate
|
|
failures were bugs in the new tests' own fixture data, not the implementation — fixed before
|
|
this: one test's `expiresAtEpochMillis` was accidentally always within the skew window regardless
|
|
of `now`, and one asserted the vault was `null` after a failed refresh when "untouched" was the
|
|
actually-intended and correct behavior).
|
|
- `./gradlew assembleDebug lintDebug`: BUILD SUCCESSFUL, 0 lint errors.
|
|
- Live device: build installed over wireless `adb`.
|
|
|
|
## Known limitations
|
|
|
|
- **This task is proactive-refresh only.** There is no "catch a live 401, refresh, and retry the
|
|
same request once" fallback at the HTTP-transport layer. In practice this should rarely matter —
|
|
the provider refreshes 60 seconds ahead of the token's own stated expiry before any request that
|
|
goes through it — but a token invalidated *server-side* for a reason other than ordinary expiry
|
|
(e.g. an admin revoking the OAuth grant) would still surface as a live 401 exactly as before this
|
|
task, correctly falling through to `AUTHENTICATION_EXPIRED` rather than being silently retried.
|
|
- **Not yet confirmed against a real long-idle session on the device.** The fix targets a problem
|
|
(repeated forced re-login/re-approval) that only manifests after enough real time passes for
|
|
Gitea's access token to actually expire — this session's live testing so far has been continuous
|
|
active use, not a multi-hour idle gap, so the *felt* improvement (no more surprise "승인" prompts)
|
|
hasn't been directly observed yet, only proven correct at the unit level.
|
|
- Gitea's server-side `expires_in` / refresh-token TTL/rotation settings are instance-configurable
|
|
(`OAuth2.InvalidateRefreshTokens` and friends) and unknown for this specific Gitea instance;
|
|
nothing in this implementation assumes a particular value, but the practical frequency of silent
|
|
refreshes on this instance hasn't been observed yet either.
|
|
|
|
## Review findings
|
|
|
|
None yet — pending Codex/human review.
|