diff --git a/NEWS.md b/NEWS.md index 89dd605f..7694ce96 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,6 +2,7 @@ * Fixed OAuth token cache pruning so that it actually matches the encrypted `.rds.enc` files written to disk; previously the pruning pattern only matched an unencrypted `.rds` file that was never created, so cached tokens were never automatically deleted regardless of age. * httr2 now requires rlang >= 1.3.0, which changes the hash used to name files cached by `req_cache()` and on-disk OAuth token caches (e.g. from `req_oauth_auth_code(cache_disk = TRUE)`). Existing cached files won't match the new hash, so they'll be silently ignored (triggering a normal cache miss/re-authentication) and cleaned up over time by the usual pruning rules; you can also delete them manually. +* `req_oauth_*()` gains an `expiry_margin` argument to control how early cached OAuth tokens are treated as expired; the default margin increases from 5 to 30 seconds (@zacdav-db, #860). # httr2 1.2.3 diff --git a/R/oauth-flow-auth-code.R b/R/oauth-flow-auth-code.R index 43d9e7c6..6e5aceee 100644 --- a/R/oauth-flow-auth-code.R +++ b/R/oauth-flow-auth-code.R @@ -83,6 +83,9 @@ #' Learn more in . #' @param cache_key If you want to cache multiple tokens per app, use this #' key to disambiguate them. +#' @param expiry_margin Number of seconds before a token's stated expiry that +#' it should be treated as expired. Increase this for servers that reject +#' tokens shortly before they expire. Defaults to 30 seconds. #' @returns `req_oauth_auth_code()` returns a modified HTTP [request] that will #' use OAuth; `oauth_flow_auth_code()` returns an [oauth_token]. #' @examples @@ -106,7 +109,8 @@ req_oauth_auth_code <- function( token_params = list(), redirect_uri = oauth_redirect_uri(), cache_disk = FALSE, - cache_key = NULL + cache_key = NULL, + expiry_margin = 30 ) { auth_url <- oauth_flow_url(auth_url, client, "auth_url") redirect <- normalize_redirect_uri(redirect_uri = redirect_uri) @@ -122,7 +126,13 @@ req_oauth_auth_code <- function( ) cache <- cache_choose(client, cache_disk, cache_key) - req_oauth(req, "oauth_flow_auth_code", params, cache = cache) + req_oauth( + req, + "oauth_flow_auth_code", + params, + cache = cache, + expiry_margin = expiry_margin + ) } #' @export diff --git a/R/oauth-flow-client-credentials.R b/R/oauth-flow-client-credentials.R index 9a6bfe74..ed9a5766 100644 --- a/R/oauth-flow-client-credentials.R +++ b/R/oauth-flow-client-credentials.R @@ -28,7 +28,8 @@ req_oauth_client_credentials <- function( req, client, scope = NULL, - token_params = list() + token_params = list(), + expiry_margin = 30 ) { params <- list( client = client, @@ -37,7 +38,13 @@ req_oauth_client_credentials <- function( ) cache <- cache_mem(client, NULL) - req_oauth(req, "oauth_flow_client_credentials", params, cache = cache) + req_oauth( + req, + "oauth_flow_client_credentials", + params, + cache = cache, + expiry_margin = expiry_margin + ) } #' @export diff --git a/R/oauth-flow-device.R b/R/oauth-flow-device.R index 8977c386..289a63cc 100644 --- a/R/oauth-flow-device.R +++ b/R/oauth-flow-device.R @@ -41,7 +41,8 @@ req_oauth_device <- function( auth_params = list(), token_params = list(), cache_disk = FALSE, - cache_key = NULL + cache_key = NULL, + expiry_margin = 30 ) { auth_url <- oauth_flow_url(auth_url, client, "device_auth_url") params <- list( @@ -54,7 +55,13 @@ req_oauth_device <- function( token_params = token_params ) cache <- cache_choose(client, cache_disk, cache_key) - req_oauth(req, "oauth_flow_device", params, cache = cache) + req_oauth( + req, + "oauth_flow_device", + params, + cache = cache, + expiry_margin = expiry_margin + ) } #' @export diff --git a/R/oauth-flow-jwt.R b/R/oauth-flow-jwt.R index af9002b0..ea3f063a 100644 --- a/R/oauth-flow-jwt.R +++ b/R/oauth-flow-jwt.R @@ -46,7 +46,8 @@ req_oauth_bearer_jwt <- function( signature = "jwt_encode_sig", signature_params = list(), scope = NULL, - token_params = list() + token_params = list(), + expiry_margin = 30 ) { params <- list( client = client, @@ -58,7 +59,13 @@ req_oauth_bearer_jwt <- function( ) cache <- cache_mem(client, claim) - req_oauth(req, "oauth_flow_bearer_jwt", params, cache = cache) + req_oauth( + req, + "oauth_flow_bearer_jwt", + params, + cache = cache, + expiry_margin = expiry_margin + ) } #' @export diff --git a/R/oauth-flow-password.R b/R/oauth-flow-password.R index 1d28b143..9afdc60d 100644 --- a/R/oauth-flow-password.R +++ b/R/oauth-flow-password.R @@ -33,7 +33,8 @@ req_oauth_password <- function( scope = NULL, token_params = list(), cache_disk = FALSE, - cache_key = username + cache_key = username, + expiry_margin = 30 ) { params <- list( client = client, @@ -43,7 +44,13 @@ req_oauth_password <- function( token_params = token_params ) cache <- cache_choose(client, cache_disk = cache_disk, cache_key = cache_key) - req_oauth(req, "oauth_flow_password", params, cache = cache) + req_oauth( + req, + "oauth_flow_password", + params, + cache = cache, + expiry_margin = expiry_margin + ) } #' @export diff --git a/R/oauth-flow-refresh.R b/R/oauth-flow-refresh.R index c3d7d8ff..73e8f968 100644 --- a/R/oauth-flow-refresh.R +++ b/R/oauth-flow-refresh.R @@ -36,7 +36,8 @@ req_oauth_refresh <- function( client, refresh_token = Sys.getenv("HTTR2_REFRESH_TOKEN"), scope = NULL, - token_params = list() + token_params = list(), + expiry_margin = 30 ) { params <- list( client = client, @@ -46,7 +47,13 @@ req_oauth_refresh <- function( ) cache <- cache_mem(client, refresh_token) - req_oauth(req, "oauth_flow_refresh", params, cache = cache) + req_oauth( + req, + "oauth_flow_refresh", + params, + cache = cache, + expiry_margin = expiry_margin + ) } #' @export diff --git a/R/oauth-flow-token-exchange.R b/R/oauth-flow-token-exchange.R index 7858ec13..e646431c 100644 --- a/R/oauth-flow-token-exchange.R +++ b/R/oauth-flow-token-exchange.R @@ -65,7 +65,8 @@ req_oauth_token_exchange <- function( requested_token_type = NULL, actor_token = NULL, actor_token_type = NULL, - token_params = list() + token_params = list(), + expiry_margin = 30 ) { params <- list( client = client, @@ -80,7 +81,13 @@ req_oauth_token_exchange <- function( token_params = token_params ) cache <- cache_mem(client, NULL) - req_oauth(req, "oauth_flow_token_exchange", params, cache = cache) + req_oauth( + req, + "oauth_flow_token_exchange", + params, + cache = cache, + expiry_margin = expiry_margin + ) } #' @export diff --git a/R/oauth-token.R b/R/oauth-token.R index 05e9d07f..e54ae37b 100644 --- a/R/oauth-token.R +++ b/R/oauth-token.R @@ -66,7 +66,7 @@ print.httr2_token <- function(x, ...) { invisible(x) } -token_has_expired <- function(token, delay = 5) { +token_has_expired <- function(token, delay = 30) { if (is.null(token$expires_at)) { FALSE } else { diff --git a/R/oauth.R b/R/oauth.R index 10146a24..11d47bc0 100644 --- a/R/oauth.R +++ b/R/oauth.R @@ -15,36 +15,57 @@ #' @param flow An `oauth_flow_` function used to generate the access token. #' @param flow_params Parameters for the flow. This should be a named list #' whose names match the argument names of `flow`. +#' @param expiry_margin Number of seconds before a token's stated expiry that +#' it should be treated as expired. Increase this for servers that reject +#' tokens shortly before they expire. Defaults to 30 seconds. #' @returns An [oauth_token]. #' @keywords internal #' @export -req_oauth <- function(req, flow, flow_params, cache) { +req_oauth <- function(req, flow, flow_params, cache, expiry_margin = 30) { + check_number_whole(expiry_margin, min = 0) + # Want req object to contain meaningful objects, not just a closure req <- req_auth_sign( req, fun = auth_oauth_sign, - params = list(flow = flow, flow_params = flow_params), + params = list( + flow = flow, + flow_params = flow_params, + expiry_margin = expiry_margin + ), cache = cache ) req <- req_policies(req, auth_oauth = TRUE) req } -auth_oauth_sign <- function(req, cache, flow, flow_params) { +auth_oauth_sign <- function( + req, + cache, + flow, + flow_params, + expiry_margin = 30 +) { token <- auth_oauth_token_get( cache = cache, flow = flow, - flow_params = flow_params + flow_params = flow_params, + expiry_margin = expiry_margin ) req_auth_bearer_token(req, token$access_token) } -auth_oauth_token_get <- function(cache, flow, flow_params = list()) { +auth_oauth_token_get <- function( + cache, + flow, + flow_params = list(), + expiry_margin = 30 +) { token <- cache$get() if (is.null(token)) { token <- exec(flow, !!!flow_params) cache$set(token) - } else if (token_has_expired(token)) { + } else if (token_has_expired(token, delay = expiry_margin)) { cache$clear() if (is.null(token$refresh_token)) { token <- exec(flow, !!!flow_params) diff --git a/man/req_oauth.Rd b/man/req_oauth.Rd index cc8872ea..19d3be32 100644 --- a/man/req_oauth.Rd +++ b/man/req_oauth.Rd @@ -4,7 +4,7 @@ \alias{req_oauth} \title{OAuth authentication} \usage{ -req_oauth(req, flow, flow_params, cache) +req_oauth(req, flow, flow_params, cache, expiry_margin = 30) } \arguments{ \item{req}{A httr2 \link{request} object.} @@ -22,6 +22,10 @@ cached yet. \item \code{set()} saves the token to the cache. \item \code{clear()} removes the token from the cache }} + +\item{expiry_margin}{Number of seconds before a token's stated expiry that +it should be treated as expired. Increase this for servers that reject +tokens shortly before they expire. Defaults to 30 seconds.} } \value{ An \link{oauth_token}. diff --git a/man/req_oauth_auth_code.Rd b/man/req_oauth_auth_code.Rd index 98f65cec..76509fee 100644 --- a/man/req_oauth_auth_code.Rd +++ b/man/req_oauth_auth_code.Rd @@ -15,7 +15,8 @@ req_oauth_auth_code( token_params = list(), redirect_uri = oauth_redirect_uri(), cache_disk = FALSE, - cache_key = NULL + cache_key = NULL, + expiry_margin = 30 ) oauth_flow_auth_code( @@ -81,6 +82,10 @@ Learn more in \url{https://httr2.r-lib.org/articles/oauth.html}.} \item{cache_key}{If you want to cache multiple tokens per app, use this key to disambiguate them.} + +\item{expiry_margin}{Number of seconds before a token's stated expiry that +it should be treated as expired. Increase this for servers that reject +tokens shortly before they expire. Defaults to 30 seconds.} } \value{ \code{req_oauth_auth_code()} returns a modified HTTP \link{request} that will diff --git a/man/req_oauth_bearer_jwt.Rd b/man/req_oauth_bearer_jwt.Rd index 0d6b6ee7..66fac3b9 100644 --- a/man/req_oauth_bearer_jwt.Rd +++ b/man/req_oauth_bearer_jwt.Rd @@ -12,7 +12,8 @@ req_oauth_bearer_jwt( signature = "jwt_encode_sig", signature_params = list(), scope = NULL, - token_params = list() + token_params = list(), + expiry_margin = 30 ) oauth_flow_bearer_jwt( @@ -49,6 +50,10 @@ supplies its own \code{claim} in \code{auth_params}. The client assertion claim \item{token_params}{List containing additional parameters passed to the \code{token_url}.} + +\item{expiry_margin}{Number of seconds before a token's stated expiry that +it should be treated as expired. Increase this for servers that reject +tokens shortly before they expire. Defaults to 30 seconds.} } \value{ \code{req_oauth_bearer_jwt()} returns a modified HTTP \link{request} that will diff --git a/man/req_oauth_client_credentials.Rd b/man/req_oauth_client_credentials.Rd index c246bd5c..3a71c4dc 100644 --- a/man/req_oauth_client_credentials.Rd +++ b/man/req_oauth_client_credentials.Rd @@ -5,7 +5,13 @@ \alias{oauth_flow_client_credentials} \title{OAuth with client credentials} \usage{ -req_oauth_client_credentials(req, client, scope = NULL, token_params = list()) +req_oauth_client_credentials( + req, + client, + scope = NULL, + token_params = list(), + expiry_margin = 30 +) oauth_flow_client_credentials(client, scope = NULL, token_params = list()) } @@ -18,6 +24,10 @@ oauth_flow_client_credentials(client, scope = NULL, token_params = list()) \item{token_params}{List containing additional parameters passed to the \code{token_url}.} + +\item{expiry_margin}{Number of seconds before a token's stated expiry that +it should be treated as expired. Increase this for servers that reject +tokens shortly before they expire. Defaults to 30 seconds.} } \value{ \code{req_oauth_client_credentials()} returns a modified HTTP \link{request} that will diff --git a/man/req_oauth_device.Rd b/man/req_oauth_device.Rd index 6d847492..28458b16 100644 --- a/man/req_oauth_device.Rd +++ b/man/req_oauth_device.Rd @@ -15,7 +15,8 @@ req_oauth_device( auth_params = list(), token_params = list(), cache_disk = FALSE, - cache_key = NULL + cache_key = NULL, + expiry_margin = 30 ) oauth_flow_device( @@ -60,6 +61,10 @@ Learn more in \url{https://httr2.r-lib.org/articles/oauth.html}.} \item{cache_key}{If you want to cache multiple tokens per app, use this key to disambiguate them.} + +\item{expiry_margin}{Number of seconds before a token's stated expiry that +it should be treated as expired. Increase this for servers that reject +tokens shortly before they expire. Defaults to 30 seconds.} } \value{ \code{req_oauth_device()} returns a modified HTTP \link{request} that will diff --git a/man/req_oauth_password.Rd b/man/req_oauth_password.Rd index 13dc5814..47ab1a31 100644 --- a/man/req_oauth_password.Rd +++ b/man/req_oauth_password.Rd @@ -13,7 +13,8 @@ req_oauth_password( scope = NULL, token_params = list(), cache_disk = FALSE, - cache_key = username + cache_key = username, + expiry_margin = 30 ) oauth_flow_password( @@ -49,6 +50,10 @@ Learn more in \url{https://httr2.r-lib.org/articles/oauth.html}.} \item{cache_key}{If you want to cache multiple tokens per app, use this key to disambiguate them.} + +\item{expiry_margin}{Number of seconds before a token's stated expiry that +it should be treated as expired. Increase this for servers that reject +tokens shortly before they expire. Defaults to 30 seconds.} } \value{ \code{req_oauth_password()} returns a modified HTTP \link{request} that will diff --git a/man/req_oauth_refresh.Rd b/man/req_oauth_refresh.Rd index bab7307a..6e208c4a 100644 --- a/man/req_oauth_refresh.Rd +++ b/man/req_oauth_refresh.Rd @@ -10,7 +10,8 @@ req_oauth_refresh( client, refresh_token = Sys.getenv("HTTR2_REFRESH_TOKEN"), scope = NULL, - token_params = list() + token_params = list(), + expiry_margin = 30 ) oauth_flow_refresh( @@ -34,6 +35,10 @@ is to look in \code{HTTR2_REFRESH_TOKEN}.} \item{token_params}{List containing additional parameters passed to the \code{token_url}.} + +\item{expiry_margin}{Number of seconds before a token's stated expiry that +it should be treated as expired. Increase this for servers that reject +tokens shortly before they expire. Defaults to 30 seconds.} } \value{ \code{req_oauth_refresh()} returns a modified HTTP \link{request} that will diff --git a/man/req_oauth_token_exchange.Rd b/man/req_oauth_token_exchange.Rd index 0cad1df7..9c9f9a05 100644 --- a/man/req_oauth_token_exchange.Rd +++ b/man/req_oauth_token_exchange.Rd @@ -16,7 +16,8 @@ req_oauth_token_exchange( requested_token_type = NULL, actor_token = NULL, actor_token_type = NULL, - token_params = list() + token_params = list(), + expiry_margin = 30 ) oauth_flow_token_exchange( @@ -65,6 +66,10 @@ one of the options in \href{https://datatracker.ietf.org/doc/html/rfc8693#sectio \item{token_params}{List containing additional parameters passed to the \code{token_url}.} + +\item{expiry_margin}{Number of seconds before a token's stated expiry that +it should be treated as expired. Increase this for servers that reject +tokens shortly before they expire. Defaults to 30 seconds.} } \value{ \code{req_oauth_token_exchange()} returns a modified HTTP \link{request} that diff --git a/tests/testthat/_snaps/oauth.md b/tests/testthat/_snaps/oauth.md index bf7dbbaf..5661d90d 100644 --- a/tests/testthat/_snaps/oauth.md +++ b/tests/testthat/_snaps/oauth.md @@ -1,3 +1,11 @@ +# req_oauth validates and stores expiry margin + + Code + req_oauth(req, "", list(), NULL, expiry_margin = -1) + Condition + Error in `req_oauth()`: + ! `expiry_margin` must be a whole number larger than or equal to 0, not the number -1. + # can store on disk Code diff --git a/tests/testthat/test-oauth-flow-client-credentials.R b/tests/testthat/test-oauth-flow-client-credentials.R new file mode 100644 index 00000000..825b0724 --- /dev/null +++ b/tests/testthat/test-oauth-flow-client-credentials.R @@ -0,0 +1,10 @@ +test_that("can configure token expiry margin", { + client <- oauth_client("test", "https://example.com/token", secret = "secret") + default_req <- request("https://example.com") |> + req_oauth_client_credentials(client) + req <- request("https://example.com") |> + req_oauth_client_credentials(client, expiry_margin = 40) + + expect_equal(default_req$policies$auth_sign$params$expiry_margin, 30) + expect_equal(req$policies$auth_sign$params$expiry_margin, 40) +}) diff --git a/tests/testthat/test-oauth-token.R b/tests/testthat/test-oauth-token.R index 7d7fa939..e883c8ea 100644 --- a/tests/testthat/test-oauth-token.R +++ b/tests/testthat/test-oauth-token.R @@ -22,10 +22,10 @@ test_that("can compute token expiry", { token <- oauth_token("xyz") expect_equal(token_has_expired(token), FALSE) - # Respects delay - token <- oauth_token("xyz", expires_in = 8, .date = Sys.time() - 10) + token <- oauth_token("xyz", expires_in = 20, .date = Sys.time()) expect_equal(token_has_expired(token), TRUE) + expect_equal(token_has_expired(token, delay = 0), FALSE) - token <- oauth_token("xyz", expires_in = 10, .date = Sys.time()) + token <- oauth_token("xyz", expires_in = 60, .date = Sys.time()) expect_equal(token_has_expired(token), FALSE) }) diff --git a/tests/testthat/test-oauth.R b/tests/testthat/test-oauth.R index 7bca4868..4211ad67 100644 --- a/tests/testthat/test-oauth.R +++ b/tests/testthat/test-oauth.R @@ -101,6 +101,58 @@ test_that("can retrieve non-expired token from cache", { expect_equal(auth_oauth_token_get(cache, oauth_flow_refresh), token) }) +test_that("expiry margin controls when cached tokens are refreshed", { + client <- oauth_client("test", "http://example.org/test") + cache <- cache_mem(client) + cached <- oauth_token("cached", expires_in = 60) + refreshed <- oauth_token("refreshed") + cache$set(cached) + + expect_equal( + auth_oauth_token_get(cache, function(...) refreshed, expiry_margin = 30), + cached + ) + expect_equal( + auth_oauth_token_get(cache, function(...) refreshed, expiry_margin = 90), + refreshed + ) + expect_equal(cache$get(), refreshed) +}) + +test_that("expiry margin preserves refresh token behavior", { + client <- oauth_client("test", "http://example.org/test") + cache <- cache_mem(client) + cache$set( + oauth_token("cached", refresh_token = "refresh", expires_in = 60) + ) + + local_mocked_bindings( + token_refresh = function(client, refresh_token, token_params = list()) { + oauth_token("refreshed", used_refresh_token = refresh_token) + } + ) + + token <- auth_oauth_token_get( + cache, + function(...) NULL, + flow_params = list(client = client), + expiry_margin = 90 + ) + expect_equal(token$access_token, "refreshed") + expect_equal(token$used_refresh_token, "refresh") +}) + +test_that("req_oauth validates and stores expiry margin", { + req <- request("https://example.com") + req <- req_oauth(req, "", list(), NULL, expiry_margin = 40) + + expect_equal(req$policies$auth_sign$params$expiry_margin, 40) + expect_snapshot( + req_oauth(req, "", list(), NULL, expiry_margin = -1), + error = TRUE + ) +}) + # Cache -------------------------------------------------------------------