diff --git a/NEWS.md b/NEWS.md index f076a4a..1232288 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,15 @@ # vmxr 0.1.1.9000 (development) +* `vmx_login()` device-code prompt now matches the actual flow (GEN-2378). + Because the browser is opened at `verification_uri_complete` (the URL that + already embeds the `user_code`), the consent page shows the code **pre-filled** + and never asks the user to paste it. `vmx_login()` now prints its own guidance + before the flow — "check the code shown in the browser matches, then approve", + plus a note that you may not be prompted to paste it — reframing the misleading + "copy … and paste when requested" line that upstream `httr2` prints. The + guidance shows only in the interactive/browser branch; the non-interactive + fallback (plain `verification_uri`, where you *do* enter the code) is left to + httr2's accurate "Visit … and enter code" message. * 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 diff --git a/R/oidc.R b/R/oidc.R index 9f9f622..fd6f188 100644 --- a/R/oidc.R +++ b/R/oidc.R @@ -276,6 +276,39 @@ vmx_oidc_config <- function(issuer = NULL, client_id = NULL, scopes = NULL) { httr2::oauth_client(id = config$client_id, token_url = token_endpoint, auth = "body") } +# Pre-frame httr2's device-code prompt so it matches what actually happens here +# (GEN-2378). httr2::oauth_flow_device() hard-codes, in its *interactive* branch, +# "Copy and paste when requested by the browser" +# (r-lib/httr2 R/oauth-flow-device.R) -- but it opens `verification_uri_complete`, +# the URL that already embeds the `user_code`, so Authentik's consent page shows +# the code pre-filled and never asks the user to paste it. That "copy ... and +# paste" line therefore describes a step that doesn't happen. We can't reword or +# suppress httr2's line (no override argument; it wraps a bare readline), so we +# print our own guidance immediately before it, reframing the step as "verify the +# code matches, then approve" and warning that no paste prompt may appear. +# +# Only the interactive/browser branch is misleading. When httr2 runs +# non-interactively it instead prints "Visit and enter code " against +# the plain `verification_uri` (no embedded code), where entering the code IS the +# real step -- so we stay silent there and let httr2's accurate instruction stand +# (the paste/enter guidance belongs only on that fallback branch). We gate on the +# same predicate httr2 uses for `open_browser` (rlang::is_interactive()) so our +# framing tracks its branch exactly. +.vmx_oidc_device_prompt <- function(interactive = rlang::is_interactive()) { + if (!isTRUE(interactive)) { + return(invisible()) + } + cli::cli_bullets(c( + "i" = "A browser window will open to confirm your VeloMetrix sign-in.", + "*" = "Check that the security code shown in the browser matches the code printed below, then approve the request.", + "i" = paste0( + "You may not be prompted to enter or paste the code \u2014 it's ", + "already included in the sign-in URL, so the page may show it pre-filled." + ) + )) + invisible() +} + # Wrap httr2's RFC 8628 device-code flow in a mockable binding. Kept minimal so # tests can stub it (the real flow opens a browser and blocks on polling). .vmx_oidc_device_flow <- function(oauth_client, device_url, scopes) { @@ -356,6 +389,10 @@ 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) + # Reframe httr2's misleading "copy & paste the code" prompt before it prints + # (GEN-2378): the browser opens the pre-filled URL, so the user verifies and + # approves rather than pasting. + .vmx_oidc_device_prompt() raw <- tryCatch( .vmx_oidc_device_flow(oauth_client, endpoints$device, config$scopes), error = function(e) { diff --git a/tests/testthat/test-oidc.R b/tests/testthat/test-oidc.R index 098216b..3caaf90 100644 --- a/tests/testthat/test-oidc.R +++ b/tests/testthat/test-oidc.R @@ -318,3 +318,51 @@ test_that("device-flow failures are wrapped as vmx_auth_error", { expect_s3_class(err, "vmx_auth_error") expect_match(conditionMessage(err), "device-code login failed") }) + +# -- GEN-2378: device-code prompt wording ------------------------------------ +# httr2::oauth_flow_device() hard-codes a "Copy and paste when requested" +# line, but it opens the pre-filled verification_uri_complete, so no paste ever +# happens. .vmx_oidc_device_prompt() pre-frames that as verify-and-approve in the +# interactive/browser branch, and stays silent in the non-interactive branch +# (where httr2's own "Visit and enter code" instruction is accurate). + +test_that("device prompt reframes the browser flow as verify-and-approve (GEN-2378)", { + # Widen cli + collapse whitespace so console line-wrapping can't split a phrase + # mid-word and break the substring matches below. + withr::local_options(cli.width = 10000) + out <- gsub("\\s+", " ", paste(cli::cli_fmt(.vmx_oidc_device_prompt(interactive = TRUE)), collapse = " ")) + # Tells the user to verify the code matches and approve -- not to paste it. + expect_match(out, "matches", ignore.case = TRUE) + expect_match(out, "approve", ignore.case = TRUE) + # The modified note: warn that no paste/enter prompt may appear (code pre-filled). + expect_match(out, "pre-filled", ignore.case = TRUE) + expect_match(out, "may not be prompted", ignore.case = TRUE) + # It must NOT instruct copy-and-paste (the misleading step this issue removes). + expect_false(grepl("paste when requested", out, ignore.case = TRUE)) +}) + +test_that("device prompt stays silent in the non-interactive fallback branch (GEN-2378)", { + # httr2 prints "Visit and enter code " there, which is accurate, so + # vmxr adds nothing (the paste/enter instruction belongs only on that branch). + expect_identical(cli::cli_fmt(.vmx_oidc_device_prompt(interactive = FALSE)), character(0)) +}) + +test_that("vmx_login shows the reframed device guidance before running the flow (GEN-2378)", { + cache <- withr::local_tempfile(fileext = ".json") + local_oidc_env(cache) + seen <- character(0) + testthat::local_mocked_bindings( + .vmx_oidc_discover = function(config) list(device = "https://auth.test/device", token = "https://auth.test/token"), + # Record ordering: the guidance must precede the (browser-opening) flow. + .vmx_oidc_device_prompt = function(interactive = rlang::is_interactive()) { + seen <<- c(seen, "prompt") + invisible() + }, + .vmx_oidc_device_flow = function(oauth_client, device_url, scopes) { + seen <<- c(seen, "flow") + list(access_token = "acc", refresh_token = "ref", expires_in = 600, token_type = "Bearer") + } + ) + suppressMessages(vmx_login()) + expect_identical(seen, c("prompt", "flow")) +})