diff --git a/NEWS.md b/NEWS.md index a32141c..f076a4a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,17 @@ # vmxr 0.1.1.9000 (development) +* OIDC access tokens now self-heal on a reused client (GEN-2344). `vmx_client()` + no longer freezes the OIDC access token at construction: the bearer token is + resolved **per request** via a provider closure, so a long-lived + `con <- vmx_client()` silently refreshes from the cached refresh token when the + short-lived access token nears expiry and keeps working across a full session + instead of throwing `vmx_auth_error` a few minutes in. Also: device-flow + failures (user-denied / code-expired / polling-exhausted) are wrapped as + `vmx_auth_error`; a cached token whose issuer/client_id do not match the + current config is no longer silently reused or clobbered (interactive login + warns first, non-interactive raises a message naming the mismatch); the + "no usable cached token" message now describes the actual cause; and + `VMX_OIDC_TOKEN_CACHE` is documented as a testing-only override. * Native OIDC device-code authentication (GEN-2332). `vmx_login()` runs the RFC 8628 device-code flow via `httr2::oauth_flow_device()` against the Authentik provider (public client, `offline_access` scope for a refresh diff --git a/R/client.R b/R/client.R index 5207df5..c99e40f 100644 --- a/R/client.R +++ b/R/client.R @@ -14,6 +14,13 @@ #' cached token. This requires `VMX_OIDC_ISSUER` + `VMX_OIDC_CLIENT_ID` to be #' set; otherwise a `vmx_auth_error` names both auth methods. #' +#' The bearer token is resolved **per request**, not frozen at construction: a +#' PAT is constant, but an OIDC access token is short-lived (~5-10 min), so a +#' long-lived `con <- vmx_client()` re-resolves each request and silently +#' refreshes from the cached refresh token when the access token is near expiry. +#' This lets a reused client keep working across a full analysis session without +#' re-login (GEN-2344). +#' #' @param base_url API base URL. Defaults to `Sys.getenv("VMX_API_BASE_URL")`. #' @param token Authentik personal access token (PAT). Defaults to #' `Sys.getenv("VMX_API_TOKEN")`, then to an OIDC access token (see @@ -32,15 +39,31 @@ vmx_client <- function(base_url = NULL, token = NULL, ...) { class = "vmx_usage_error" ) } - if (!nzchar(token)) { - # No PAT: fall back to OIDC device-code auth (cache -> refresh -> login). - token <- .vmx_client_bearer() + # Resolve the bearer token via a *provider closure* re-invoked on every request + # (see vmx_req), not a single string baked in here. A frozen OIDC access token + # expires a few minutes into a persistent `con` and every later call then 401s + # even though a valid refresh token is cached (GEN-2344); a provider re-reads + # the cache and refreshes as needed, so the client self-heals. + if (nzchar(token)) { + # Explicit PAT / VMX_API_TOKEN: constant provider. + pat <- token + token_provider <- function() pat + } else { + # No PAT: OIDC device-code auth (cache -> refresh -> login), re-resolved + # per request so an expired access token self-heals from the refresh token. + token_provider <- .vmx_client_bearer_provider() } + # Resolve once up front so auth problems (no cache, revoked refresh token, no + # OIDC config) surface at construction rather than on the first API call, and + # so the OIDC cache is primed. The return value is intentionally discarded -- + # requests re-resolve through the provider. + token_provider() + structure( list( base_url = sub("/+$", "", base_url), - token = token, + token_provider = token_provider, options = list(...) ), class = "vmx_client" diff --git a/R/http.R b/R/http.R index 561413c..0b7aee1 100644 --- a/R/http.R +++ b/R/http.R @@ -16,8 +16,11 @@ #' @keywords internal #' @noRd vmx_req <- function(client, path) { + # Resolve the bearer token *now*, per request, via the client's provider -- + # a reused client thus re-reads the OIDC cache and silently refreshes a + # near-expired access token instead of sending a stale frozen one (GEN-2344). httr2::request(paste0(client$base_url, .vmx_api_prefix, path)) |> - httr2::req_auth_bearer_token(client$token) |> + httr2::req_auth_bearer_token(client$token_provider()) |> httr2::req_user_agent("vmxr (https://github.com/generable/vmxr)") |> httr2::req_error(is_error = function(resp) FALSE) } diff --git a/R/oidc.R b/R/oidc.R index 90684e8..9f9f622 100644 --- a/R/oidc.R +++ b/R/oidc.R @@ -138,6 +138,12 @@ vmx_oidc_config <- function(issuer = NULL, client_id = NULL, scopes = NULL) { # -- on-disk cache (CLI-compatible JSON, 0600) -------------------------------- +# Resolve the on-disk cache path. The vmx CLI has *no* cache-path override -- it +# always uses the fixed `~/.config/vmx/oidc-token.json`. VMX_OIDC_TOKEN_CACHE is +# therefore a **testing-only** override, not a user-facing knob: pointing it +# elsewhere diverges vmxr's cache from the CLI's fixed path and breaks the +# "one login serves both R and the CLI" invariant. Tests set it to an isolated +# tempfile; real users should leave it unset. .vmx_oidc_cache_path <- function() { path <- trimws(Sys.getenv("VMX_OIDC_TOKEN_CACHE", unset = "")) if (!nzchar(path)) path <- .vmx_oidc_default_cache @@ -334,8 +340,12 @@ vmx_oidc_config <- function(issuer = NULL, client_id = NULL, scopes = NULL) { #' @param client_id Public OIDC client id. Defaults to `VMX_OIDC_CLIENT_ID`. #' @param scopes Space-separated scopes. Defaults to `VMX_OIDC_SCOPES`, then to #' `"openid profile email offline_access goauthentik.io/api"`. -#' @param cache_path Where to cache the token. Defaults to -#' `VMX_OIDC_TOKEN_CACHE`, then `~/.config/vmx/oidc-token.json`. +#' @param cache_path Where to cache the token. Defaults to the CLI-shared +#' `~/.config/vmx/oidc-token.json`. The `VMX_OIDC_TOKEN_CACHE` env var can +#' override this, but it is a **testing-only** override (the `vmx` CLI has no +#' such variable): pointing it elsewhere makes vmxr read/write a cache the CLI +#' never sees, breaking the "one login serves both" invariant. Leave it unset +#' in normal use. #' #' @return Invisibly, the cached token (a list; the access/refresh tokens are #' secret and never printed). @@ -346,7 +356,20 @@ vmx_login <- function(issuer = NULL, client_id = NULL, scopes = NULL, cache_path <- cache_path %||% .vmx_oidc_cache_path() endpoints <- .vmx_oidc_discover(config) oauth_client <- .vmx_oidc_client(config, endpoints$token) - raw <- .vmx_oidc_device_flow(oauth_client, endpoints$device, config$scopes) + raw <- tryCatch( + .vmx_oidc_device_flow(oauth_client, endpoints$device, config$scopes), + error = function(e) { + # Keep our own already-classed errors; wrap raw httr2 / oauth_flow_device + # failures (user denied, code expired, slow_down polling exhausted, + # timeout) into vmx_auth_error so they stay in the vmx_error hierarchy and + # callers can catch every auth failure the same way. + if (inherits(e, "vmx_error")) stop(e) + vmx_abort( + paste0("OIDC device-code login failed: ", conditionMessage(e)), + class = "vmx_auth_error", parent = e + ) + } + ) token <- .vmx_token_from_httr2(config, raw) .vmx_save_cached_token(token, cache_path) cli::cli_alert_success("Logged in to VeloMetrix; token cached at {.path {cache_path}}.") @@ -358,6 +381,11 @@ vmx_login <- function(issuer = NULL, client_id = NULL, scopes = NULL, #' Returns a usable access token, refreshing silently when the cached one is #' expired, and running [vmx_login()] when there is no usable cached token (only #' possible in an interactive session; otherwise a `vmx_auth_error` is raised). +#' +#' A cached token whose `issuer`/`client_id` do not match the current config is +#' never silently reused; interactively it is overwritten only after a warning +#' (the cache is shared with the CLI), and non-interactively it raises a +#' `vmx_auth_error` that names the mismatch. #' @keywords internal #' @noRd vmx_oidc_access_token <- function(config = vmx_oidc_config(), @@ -365,7 +393,12 @@ vmx_oidc_access_token <- function(config = vmx_oidc_config(), can_prompt = interactive()) { token <- .vmx_load_cached_token(cache_path) refresh_err <- NULL - if (!is.null(token) && .vmx_token_matches(token, config)) { + # A cached token is only usable if it was minted for the *current* issuer + + # client_id. One for a different config must not be silently reused here, nor + # silently clobbered by the login below. + cache_mismatch <- !is.null(token) && !.vmx_token_matches(token, config) + + if (!is.null(token) && !cache_mismatch) { if (!.vmx_token_expired(token)) return(token$access_token) if (!is.null(token$refresh_token)) { refreshed <- tryCatch( @@ -381,16 +414,49 @@ vmx_oidc_access_token <- function(config = vmx_oidc_config(), } } } + if (!isTRUE(can_prompt)) { + # State *why* there is no usable token accurately -- "no cache" is wrong when + # a token exists but is expired-without-refresh, or is for another config. + reason <- if (is.null(token)) { + "No cached OIDC token was found" + } else if (cache_mismatch) { + paste0( + "The cached OIDC token is for a different issuer/client_id (", + token$issuer, " / ", token$client_id, ") than the current config (", + config$issuer, " / ", config$client_id, ")" + ) + } else if (is.null(token$refresh_token)) { + "The cached OIDC token is expired and has no refresh token to renew it" + } else { + "The cached OIDC token is expired and could not be refreshed" + } msg <- paste0( - "No usable cached OIDC token and not in an interactive session. Run ", - "`vmx_login()` interactively (or set `token=` / VMX_API_TOKEN)." + reason, ", and this is not an interactive session. Run `vmx_login()` ", + "interactively (or set `token=` / VMX_API_TOKEN)." ) # Surface *why* a silent refresh failed (revoked/expired refresh token, # network) instead of collapsing every cause into the generic message. if (!is.null(refresh_err)) msg <- paste0(msg, " (token refresh failed: ", refresh_err, ")") vmx_abort(msg, class = "vmx_auth_error") } + + # Interactive: vmx_login() below runs the device flow and overwrites the cache + # at `cache_path`. If a token for a *different* provider is cached there, warn + # before clobbering it -- that file is also the CLI's login (one login serves + # both), so a silent overwrite would log the CLI out of its provider. + if (cache_mismatch) { + cli::cli_warn(c( + "Overwriting a cached OIDC token issued for a different provider.", + "i" = "Cached: issuer {.val {token$issuer}}, client_id {.val {token$client_id}}.", + "i" = "Config: issuer {.val {config$issuer}}, client_id {.val {config$client_id}}.", + "!" = paste0( + "{.path {cache_path}} is shared with the vmx CLI; continue only if you ", + "mean to switch this machine's login to the new provider." + ) + )) + } + token <- vmx_login( issuer = config$issuer, client_id = config$client_id, scopes = config$scopes, cache_path = cache_path @@ -398,10 +464,15 @@ vmx_oidc_access_token <- function(config = vmx_oidc_config(), token$access_token } -# Resolve a bearer token for vmx_client() when no PAT is configured. Attempts -# OIDC auto-auth when the OIDC env vars are set; otherwise raises a single -# vmx_auth_error naming both auth methods. -.vmx_client_bearer <- function() { +# Build a per-request bearer-token provider for vmx_client() when no PAT is +# configured. Requires OIDC to be configured (else a single vmx_auth_error names +# both auth methods). The returned closure re-resolves the OIDC access token on +# *every* call -- reading the cache and refreshing from the refresh token when +# the access token is within the expiry skew -- so a long-lived client +# self-heals instead of failing once its first access token expires (GEN-2344). +# Config and cache path are captured once so the provider is stable across the +# client's life even if the env vars later change. +.vmx_client_bearer_provider <- function() { if (!.vmx_oidc_configured()) { vmx_abort( paste0( @@ -411,5 +482,7 @@ vmx_oidc_access_token <- function(config = vmx_oidc_config(), class = "vmx_auth_error" ) } - vmx_oidc_access_token() + config <- vmx_oidc_config() + cache_path <- .vmx_oidc_cache_path() + function() vmx_oidc_access_token(config = config, cache_path = cache_path) } diff --git a/man/vmx_client.Rd b/man/vmx_client.Rd index 779ab5a..2986e16 100644 --- a/man/vmx_client.Rd +++ b/man/vmx_client.Rd @@ -31,4 +31,11 @@ When no token is supplied (neither \code{token=} nor \code{VMX_API_TOKEN}), the -- in an interactive session -- running \code{\link[=vmx_login]{vmx_login()}} if there is no usable cached token. This requires \code{VMX_OIDC_ISSUER} + \code{VMX_OIDC_CLIENT_ID} to be set; otherwise a \code{vmx_auth_error} names both auth methods. + +The bearer token is resolved \strong{per request}, not frozen at construction: a +PAT is constant, but an OIDC access token is short-lived (~5-10 min), so a +long-lived \code{con <- vmx_client()} re-resolves each request and silently +refreshes from the cached refresh token when the access token is near expiry. +This lets a reused client keep working across a full analysis session without +re-login (GEN-2344). } diff --git a/man/vmx_login.Rd b/man/vmx_login.Rd index 50e3dbc..9d3e0e5 100644 --- a/man/vmx_login.Rd +++ b/man/vmx_login.Rd @@ -14,8 +14,12 @@ vmx_login(issuer = NULL, client_id = NULL, scopes = NULL, cache_path = NULL) \item{scopes}{Space-separated scopes. Defaults to \code{VMX_OIDC_SCOPES}, then to \code{"openid profile email offline_access goauthentik.io/api"}.} -\item{cache_path}{Where to cache the token. Defaults to -\code{VMX_OIDC_TOKEN_CACHE}, then \verb{~/.config/vmx/oidc-token.json}.} +\item{cache_path}{Where to cache the token. Defaults to the CLI-shared +\verb{~/.config/vmx/oidc-token.json}. The \code{VMX_OIDC_TOKEN_CACHE} env var can +override this, but it is a \strong{testing-only} override (the \code{vmx} CLI has no +such variable): pointing it elsewhere makes vmxr read/write a cache the CLI +never sees, breaking the "one login serves both" invariant. Leave it unset +in normal use.} } \value{ Invisibly, the cached token (a list; the access/refresh tokens are diff --git a/tests/testthat/test-client.R b/tests/testthat/test-client.R index 90db7ed..ee8c3b7 100644 --- a/tests/testthat/test-client.R +++ b/tests/testthat/test-client.R @@ -7,7 +7,7 @@ test_that("vmx_client resolves explicit args", { test_that("vmx_client trims surrounding whitespace (e.g. from .Renviron)", { con <- vmx_client(base_url = " https://example.test ", token = "\tpat_abc\n") expect_equal(con$base_url, "https://example.test") - expect_equal(con$token, "pat_abc") + expect_equal(con$token_provider(), "pat_abc") }) test_that("vmx_client errors without a base_url", { diff --git a/tests/testthat/test-oidc.R b/tests/testthat/test-oidc.R index 8690834..098216b 100644 --- a/tests/testthat/test-oidc.R +++ b/tests/testthat/test-oidc.R @@ -142,14 +142,15 @@ test_that("vmx_client auto-authenticates from a valid cached OIDC token", { con <- vmx_client() expect_s3_class(con, "vmx_client") - expect_equal(con$token, "cached_acc") + # The token is resolved per request via the provider, not frozen on the object. + expect_equal(con$token_provider(), "cached_acc") }) test_that("explicit token / PAT bypasses OIDC entirely", { cache <- withr::local_tempfile(fileext = ".json") # never written local_oidc_env(cache) con <- vmx_client(token = "pat_abc") - expect_equal(con$token, "pat_abc") + expect_equal(con$token_provider(), "pat_abc") expect_false(file.exists(cache)) }) @@ -166,3 +167,154 @@ test_that("configured OIDC but no cache and non-interactive raises vmx_auth_erro local_oidc_env(cache) expect_error(vmx_oidc_access_token(can_prompt = FALSE), class = "vmx_auth_error") }) + +# --- GEN-2344: persistent-client self-heal + refresh review nits -------------- + +test_that("a long-lived vmx_client() refreshes a stale access token on a later call", { + # The structural gap the rest of the suite can't catch: it only exercises the + # fresh-client-per-call path. Here one client is reused across a token expiry. + cache <- withr::local_tempfile(fileext = ".json") + local_oidc_env(cache) + # A valid (non-expired) token at construction time. + .vmx_save_cached_token( + .vmx_token( + access_token = "acc_valid", refresh_token = "ref", + expires_at = as.numeric(Sys.time()) + 600, + token_type = "Bearer", issuer = issuer, client_id = "test-cli" + ), + cache + ) + testthat::local_mocked_bindings( + .vmx_oidc_discover = function(config) list(device = "https://auth.test/device", token = "https://auth.test/token") + ) + + # Construct once (resolves the valid token; no network needed). + con <- vmx_client() + + # Time passes and the access token expires. The provider re-reads the cache + # each request, so rewriting it as expired simulates the wall-clock elapse. + .vmx_save_cached_token( + .vmx_token( + access_token = "acc_valid", refresh_token = "ref", + expires_at = as.numeric(Sys.time()) - 10, + token_type = "Bearer", issuer = issuer, client_id = "test-cli" + ), + cache + ) + + # The next call on the *same* client refreshes (1st mock) then succeeds (2nd). + httr2::local_mocked_responses(list( + httr2::response_json(body = list(access_token = "acc_fresh", refresh_token = "ref2", expires_in = 600)), + httr2::response_json(body = list( + user_id = "usr_1", email = "a@b.co", name = "Ada", workspace_id = "ws_1", + roles = list("admin"), counts = list(treatments = 0L, data_versions = 0L, model_fits = 0L) + )) + )) + + me <- vmx_whoami(con) + expect_equal(me$email, "a@b.co") + # The stale token was silently refreshed and re-cached -- no re-login. + expect_equal(.vmx_load_cached_token(cache)$access_token, "acc_fresh") +}) + +test_that("a valid non-expired cached token is returned without refreshing (skew boundary)", { + cache <- withr::local_tempfile(fileext = ".json") + local_oidc_env(cache) + # Life well beyond the 60s skew -> must be used as-is, not refreshed. + .vmx_save_cached_token( + .vmx_token( + access_token = "acc_ok", refresh_token = "ref", + expires_at = as.numeric(Sys.time()) + 600, + token_type = "Bearer", issuer = issuer, client_id = "test-cli" + ), + cache + ) + refreshed <- FALSE + testthat::local_mocked_bindings( + .vmx_oidc_refresh_flow = function(config, token) { + refreshed <<- TRUE + stop("refresh must not run for a still-valid token") + } + ) + expect_equal(vmx_oidc_access_token(can_prompt = FALSE), "acc_ok") + expect_false(refreshed) +}) + +test_that("a cached token inside the expiry skew window is refreshed", { + cache <- withr::local_tempfile(fileext = ".json") + local_oidc_env(cache) + # 30s of life left, inside the 60s skew -> treated as expired, refreshed early. + .vmx_save_cached_token( + .vmx_token( + access_token = "acc_soon", refresh_token = "ref", + expires_at = as.numeric(Sys.time()) + 30, + token_type = "Bearer", issuer = issuer, client_id = "test-cli" + ), + cache + ) + testthat::local_mocked_bindings( + .vmx_oidc_discover = function(config) list(device = "https://auth.test/device", token = "https://auth.test/token") + ) + httr2::local_mocked_responses(list( + httr2::response_json(body = list(access_token = "acc_new", refresh_token = "ref2", expires_in = 600)) + )) + expect_equal(vmx_oidc_access_token(can_prompt = FALSE), "acc_new") +}) + +test_that("a config-mismatch cache is reported (not silently reused) when non-interactive", { + cache <- withr::local_tempfile(fileext = ".json") + local_oidc_env(cache) # config client_id = "test-cli" + .vmx_save_cached_token( + .vmx_token( + access_token = "acc_other", refresh_token = "ref", + expires_at = as.numeric(Sys.time()) + 600, + token_type = "Bearer", issuer = issuer, client_id = "other-cli" + ), + cache + ) + err <- tryCatch(vmx_oidc_access_token(can_prompt = FALSE), vmx_auth_error = function(e) e) + expect_s3_class(err, "vmx_auth_error") + expect_match(conditionMessage(err), "different issuer/client_id") + # The mismatched token was never handed back. + expect_false(grepl("acc_other", conditionMessage(err))) +}) + +test_that("a config-mismatch cache warns before overwriting on interactive login", { + cache <- withr::local_tempfile(fileext = ".json") + local_oidc_env(cache) + .vmx_save_cached_token( + .vmx_token( + access_token = "acc_other", refresh_token = "ref", + expires_at = as.numeric(Sys.time()) + 600, + token_type = "Bearer", issuer = issuer, client_id = "other-cli" + ), + cache + ) + testthat::local_mocked_bindings( + .vmx_oidc_discover = function(config) list(device = "https://auth.test/device", token = "https://auth.test/token"), + .vmx_oidc_device_flow = function(oauth_client, device_url, scopes) { + list(access_token = "acc_relogin", refresh_token = "ref_new", expires_in = 600, token_type = "Bearer") + } + ) + expect_warning( + access <- vmx_oidc_access_token(can_prompt = TRUE), + "different provider" + ) + expect_equal(access, "acc_relogin") + # The cache was overwritten with the newly-issued (matching-config) token. + expect_equal(.vmx_load_cached_token(cache)$client_id, "test-cli") +}) + +test_that("device-flow failures are wrapped as vmx_auth_error", { + cache <- withr::local_tempfile(fileext = ".json") + local_oidc_env(cache) + testthat::local_mocked_bindings( + .vmx_oidc_discover = function(config) list(device = "https://auth.test/device", token = "https://auth.test/token"), + .vmx_oidc_device_flow = function(oauth_client, device_url, scopes) { + stop("device authorization was denied by the user") + } + ) + err <- tryCatch(vmx_login(), vmx_auth_error = function(e) e) + expect_s3_class(err, "vmx_auth_error") + expect_match(conditionMessage(err), "device-code login failed") +})