diff --git a/DESCRIPTION b/DESCRIPTION index 01000870..d469c286 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: orderly2 Title: Orderly Next Generation -Version: 1.99.28 +Version: 1.99.29 Authors@R: c(person("Rich", "FitzJohn", role = c("aut", "cre"), email = "rich.fitzjohn@gmail.com"), person("Robert", "Ashton", role = "aut"), diff --git a/R/location.R b/R/location.R index 63f7d07b..737e5e79 100644 --- a/R/location.R +++ b/R/location.R @@ -41,8 +41,8 @@ ##' * `url`: The location of the server ##' ##' * `token`: The value for your your login token (currently this is -##' a GitHub token with `read:org` scope). Later we'll expand this -##' as other authentication modes are supported. +##' a GitHub token with `read:org` scope). If missing or NULL, orderly2 will +##' perform an interactive authentication against GitHub to obtain one. ##' ##' **Custom locations**: ##' @@ -110,6 +110,7 @@ orderly_location_add <- function(name, type, args, root = NULL, locate = TRUE) { assert_scalar_character(loc$args[[1]]$url, name = "args$url", call = environment()) assert_scalar_character(loc$args[[1]]$token, name = "args$token", + allow_null = TRUE, call = environment()) } @@ -764,7 +765,7 @@ new_location_entry <- function(name, type, args, call = NULL) { } else if (type == "http") { required <- "url" } else if (type == "packit") { - required <- c("url", "token") + required <- c("url") } else if (type == "custom") { required <- "driver" } diff --git a/R/location_http.R b/R/location_http.R index 3ea2766a..462e9917 100644 --- a/R/location_http.R +++ b/R/location_http.R @@ -6,8 +6,8 @@ orderly_location_http <- R6::R6Class( ), public = list( - initialize = function(url, auth = NULL) { - private$client <- outpack_http_client$new(url, auth) + initialize = function(url, authorise = NULL) { + private$client <- outpack_http_client$new(url, authorise) }, list = function() { @@ -86,26 +86,3 @@ orderly_location_http <- R6::R6Class( } ) ) - - -orderly_location_packit <- function(url, token) { - assert_scalar_character(url) - assert_scalar_character(token) - if (grepl("^\\$", token)) { - token_variable <- sub("^\\$", "", token) - token <- Sys.getenv(token_variable, NA_character_) - if (is.na(token)) { - cli::cli_abort( - "Environment variable '{token_variable}' was not set") - } - } - - if (!grepl("/$", url)) { - url <- paste0(url, "/") - } - url_login <- paste0(url, "packit/api/auth/login/api") - url_outpack <- paste0(url, "packit/api/outpack") - - auth <- list(url = url_login, data = list(token = scalar(token))) - orderly_location_http$new(url_outpack, auth) -} diff --git a/R/location_packit.R b/R/location_packit.R new file mode 100644 index 00000000..f2e54b36 --- /dev/null +++ b/R/location_packit.R @@ -0,0 +1,75 @@ +github_oauth_client <- function() { + # Surprisingly, we don't actually need the Client ID here to match the one + # used by Packit. It should be fine to hardcode a value regardless of which + # server we are talking to. + httr2::oauth_client( + id = "Ov23liUrbkR0qUtAO1zu", + token_url = "https://github.com/login/oauth/access_token", + name = "orderly2" + ) +} + +do_oauth_device_flow <- function(base_url) { + res <- httr2::oauth_token_cached( + client = github_oauth_client(), + flow = httr2::oauth_flow_device, + flow_params = list( + auth_url = "https://github.com/login/device/code", + scope = "read:org"), + cache_disk = TRUE) + res$access_token +} + +# Logging in with packit is quite slow and we'll want to cache this; but we +# won't be holding a persistent handle to the root. So for now at least we'll +# keep a pool of generated bearer token headers, stored against the hash of the +# auth details. We only store this on successful login. +# +# This does mean there's no way to flush the cache and force a login, but that +# should hopefully not be that big a problem. We'll probably want to refresh +# the tokens from the request anyway. +# +# It also means the user cannot easily use two different identities on the same +# server from within the same session. +auth_cache <- new.env(parent = emptyenv()) +packit_authorisation <- function(base_url, token) { + key <- rlang::hash(list(base_url = base_url, token = token)) + + if (is.null(auth_cache[[key]])) { + cli::cli_alert_info("Logging in to {base_url}") + if (is.null(token)) { + token <- do_oauth_device_flow(base_url) + } + + login_url <- paste0(base_url, "packit/api/auth/login/api") + res <- http_client_request( + login_url, + function(r) r %>% httr2::req_body_json(list(token = scalar(token)))) + + cli::cli_alert_success("Logged in successfully") + + auth_cache[[key]] <- list("Authorization" = paste("Bearer", res$token)) + } + auth_cache[[key]] +} + +orderly_location_packit <- function(url, token) { + assert_scalar_character(url) + assert_scalar_character(token, allow_null = TRUE) + if (!is.null(token) && grepl("^\\$", token)) { + token_variable <- sub("^\\$", "", token) + token <- Sys.getenv(token_variable, NA_character_) + if (is.na(token)) { + cli::cli_abort( + "Environment variable '{token_variable}' was not set") + } + } + + if (!grepl("/$", url)) { + url <- paste0(url, "/") + } + + orderly_location_http$new( + paste0(url, "packit/api/outpack"), + function() packit_authorisation(url, token)) +} diff --git a/R/outpack_http_client.R b/R/outpack_http_client.R index c6751ae6..b28cb615 100644 --- a/R/outpack_http_client.R +++ b/R/outpack_http_client.R @@ -3,32 +3,25 @@ outpack_http_client <- R6::R6Class( public = list( url = NULL, - auth = NULL, + authorise = NULL, - initialize = function(url, auth) { + initialize = function(url, authorise = NULL) { self$url <- sub("/$", "", url) - if (is.null(auth)) { - self$auth <- list(enabled = FALSE) + if (is.null(authorise)) { + self$authorise <- function() NULL } else { - self$auth <- list(enabled = TRUE, url = auth$url, data = auth$data) - } - }, - - authorise = function() { - needs_auth <- self$auth$enabled && is.null(self$auth$header) - if (needs_auth) { - self$auth$header <- http_client_login(self$url, self$auth) + self$authorise <- authorise } }, request = function(path, customize = identity, ...) { - self$authorise() + auth_headers <- self$authorise() http_client_request( self$url, function(r) { r |> httr2::req_url_path_append(path) |> - httr2::req_headers(!!!self$auth$header) |> + httr2::req_headers(!!!auth_headers) |> customize() }, ...) } @@ -89,28 +82,3 @@ http_client_error <- function(msg, code, errors) { class(err) <- c("outpack_http_client_error", "error", "condition") err } - - -## Logging in with packit is quite slow and we'll want to cache this; -## but we won't be holding a persistant handle to the root. So for -## now at least we'll keep a pool of generated bearer token headers, -## stored against the hash of the auth details (so the url and the -## token used to log in with). We only store this on successful -## login. -## -## This does mean there's no way to flush the cache and force a login, -## but that should hopefully not be that big a problem. We'll -## probably want to refresh the tokens from the request anyway. -auth_cache <- new.env(parent = emptyenv()) -http_client_login <- function(name, auth) { - key <- rlang::hash(auth) - if (is.null(auth_cache[[key]])) { - cli::cli_alert_info("Logging in to {name}") - - res <- http_client_request(auth$url, . %>% httr2::req_body_json(auth$data)) - - cli::cli_alert_success("Logged in successfully") - auth_cache[[key]] <- list("Authorization" = paste("Bearer", res$token)) - } - auth_cache[[key]] -} diff --git a/tests/testthat/test-location-packit.R b/tests/testthat/test-location-packit.R new file mode 100644 index 00000000..50aaf9d7 --- /dev/null +++ b/tests/testthat/test-location-packit.R @@ -0,0 +1,127 @@ +test_that("can authenticate with existing token", { + clear_auth_cache() + withr::defer(clear_auth_cache()) + + token = "my-github-token" + + mock_post <- local_mock_response( + to_json(list(token = jsonlite::unbox("my-packit-token"))), + wrap = FALSE) + + res <- evaluate_promise( + packit_authorisation("http://example.com/", token)) + + expect_length(res$messages, 2) + expect_match(res$messages[[1]], "Logging in to http://example.com") + expect_match(res$messages[[2]], "Logged in successfully") + expect_equal(res$result, + list("Authorization" = paste("Bearer", "my-packit-token"))) + + mockery::expect_called(mock_post, 1) + args <- mockery::mock_args(mock_post)[[1]] + expect_equal(args[[1]]$url, "http://example.com/packit/api/auth/login/api") + expect_equal(args[[1]]$body$data, list(token = scalar("my-github-token"))) + expect_equal(args[[1]]$body$type, "json") + + ## And a second time, does not call mock_post again: + res2 <- expect_silent( + packit_authorisation("http://example.com/", token)) + expect_equal(res2, res$result) + mockery::expect_called(mock_post, 1) +}) + + +test_that("can authenticate using device flow", { + clear_auth_cache() + withr::defer(clear_auth_cache()) + + mock_post <- local_mock_response( + to_json(list(token = jsonlite::unbox("my-packit-token"))), + wrap = FALSE) + + mockery::stub(packit_authorisation, "do_oauth_device_flow", "my-github-token") + + res <- evaluate_promise(packit_authorisation("http://example.com/", + token=NULL)) + + expect_length(res$messages, 2) + expect_match(res$messages[[1]], "Logging in to http://example.com") + expect_match(res$messages[[2]], "Logged in successfully") + expect_equal(res$result, + list("Authorization" = paste("Bearer", "my-packit-token"))) + + mockery::expect_called(mock_post, 1) + args <- mockery::mock_args(mock_post)[[1]] + expect_equal(args[[1]]$url, "http://example.com/packit/api/auth/login/api") + expect_equal(args[[1]]$body$data, list(token = scalar("my-github-token"))) + expect_equal(args[[1]]$body$type, "json") +}) + +test_that("location_packit uses authentication", { + clear_auth_cache() + withr::defer(clear_auth_cache()) + + token = "my-github-token" + id <- outpack_id() + metadata <- "packet metadata" + + mock_login <- mock_response( + to_json(list(token = jsonlite::unbox("my-packit-token"))), + wrap = FALSE) + mock_get <- mock_response(metadata) + mock <- mockery::mock(mock_login, mock_get) + httr2::local_mocked_responses(function(req) mock(req)) + + location <- orderly_location_packit("http://example.com", token) + res <- evaluate_promise(location$metadata(id)) + + expect_length(res$messages, 2) + expect_match(res$messages[[1]], "Logging in to http://example.com") + expect_match(res$messages[[2]], "Logged in successfully") + expect_equal(res$result, setNames(metadata, id)) + + mockery::expect_called(mock, 2) + + args <- mockery::mock_args(mock)[[1]] + expect_equal(args[[1]]$url, "http://example.com/packit/api/auth/login/api") + expect_equal(args[[1]]$body$data, list(token = scalar("my-github-token"))) + expect_equal(args[[1]]$body$type, "json") + + args <- mockery::mock_args(mock)[[2]] + expect_match(args[[1]]$url, + "http://example.com/packit/api/outpack/metadata/.*/text") + expect_equal(args[[1]]$headers, + list(Authorization = "Bearer my-packit-token"), + ignore_attr = TRUE) + + mock <- mockery::mock(mock_login, mock_get) +}) + +test_that("can create a packit location using an environment variable token", { + loc <- withr::with_envvar( + c("PACKIT_TOKEN" = "abc123"), + orderly_location_packit("http://example.com", "$PACKIT_TOKEN")) + + mock_login <- local_mock_response( + to_json(list(token = jsonlite::unbox("my-packit-token"))), + wrap = FALSE) + + client <- loc$.__enclos_env__$private$client + evaluate_promise(client$authorise()) + + mockery::expect_called(mock_login, 1) + + args <- mockery::mock_args(mock_login)[[1]] + expect_equal(args[[1]]$url, "http://example.com/packit/api/auth/login/api") + expect_equal(args[[1]]$body$data, list(token = scalar("abc123"))) + expect_equal(args[[1]]$body$type, "json") +}) + + +test_that("error of token variable not found", { + withr::with_envvar( + c("PACKIT_TOKEN" = NA_character_), + expect_error( + orderly_location_packit("https://example.com", "$PACKIT_TOKEN"), + "Environment variable 'PACKIT_TOKEN' was not set")) +}) diff --git a/tests/testthat/test-location.R b/tests/testthat/test-location.R index 9d20f278..ae14e1bb 100644 --- a/tests/testthat/test-location.R +++ b/tests/testthat/test-location.R @@ -810,11 +810,14 @@ test_that("validate arguments to packit locations", { expect_error( orderly_location_add("other", "packit", list(server = "example.com"), root = root), - "Fields missing from args: 'url' and 'token'") + "Field missing from args: 'url'") + expect_error( - orderly_location_add("other", "packit", list(url = "example.com"), + orderly_location_add("other", "packit", + list(url = "example.com", token = 123), root = root), - "Field missing from args: 'token'") + "'args$token' must be character") + expect_equal(orderly_location_list(root = root), "local") }) @@ -825,63 +828,49 @@ test_that("can add a packit location", { list(url = "https://example.com", token = "abc123"), root = root) expect_equal(orderly_location_list(root = root), c("local", "other")) + + mock_driver <- mockery::mock() + mockery::stub(location_driver, "orderly_location_packit", mock_driver) + dr <- location_driver("other", root) - expect_s3_class(dr, "orderly_location_http") # not actually packit - cl <- dr$.__enclos_env__$private$client - expect_equal(cl$url, "https://example.com/packit/api/outpack") + + mockery::expect_called(mock_driver, 1) expect_equal( - cl$auth, - list(enabled = TRUE, - url = "https://example.com/packit/api/auth/login/api", - data = list(token = scalar("abc123")))) + mockery::mock_args(mock_driver)[[1]], + list("https://example.com", "abc123")) }) +test_that("can add a packit location without a token", { + root <- create_temporary_root() + orderly_location_add("other", "packit", + list(url = "https://example.com"), + root = root) + expect_equal(orderly_location_list(root = root), c("local", "other")) -test_that("can create a packit location using an environment variable token", { - loc <- withr::with_envvar( - c("PACKIT_TOKEN" = "abc123"), - orderly_location_packit("https://example.com", "$PACKIT_TOKEN")) - client <- loc$.__enclos_env__$private$client - expect_equal( - client$auth, - list(enabled = TRUE, - url = "https://example.com/packit/api/auth/login/api", - data = list(token = scalar("abc123")))) + mock_driver <- mockery::mock() + mockery::stub(location_driver, "orderly_location_packit", mock_driver) + + dr <- location_driver("other", root) + + mockery::expect_called(mock_driver, 1) expect_equal( - client$url, - "https://example.com/packit/api/outpack") + mockery::mock_args(mock_driver)[[1]], + list("https://example.com", NULL)) }) - test_that("cope with trailing slash in url if needed", { loc <- orderly_location_packit("https://example.com/", "abc123") client <- loc$.__enclos_env__$private$client - expect_equal( - client$auth, - list(enabled = TRUE, - url = "https://example.com/packit/api/auth/login/api", - data = list(token = scalar("abc123")))) expect_equal( client$url, "https://example.com/packit/api/outpack") }) -test_that("error of token variable not found", { - withr::with_envvar( - c("PACKIT_TOKEN" = NA_character_), - expect_error( - orderly_location_packit("https://example.com", "$PACKIT_TOKEN"), - "Environment variable 'PACKIT_TOKEN' was not set")) -}) - - test_that("can create an outpack location, disabling auth", { loc <- orderly_location_http$new("https://example.com", NULL) client <- loc$.__enclos_env__$private$client - expect_equal( - client$auth, - list(enabled = FALSE)) + expect_equal(client$authorise(), NULL) expect_equal( client$url, "https://example.com") diff --git a/tests/testthat/test-outpack-http-client.R b/tests/testthat/test-outpack-http-client.R index 4761f3d9..b3116c92 100644 --- a/tests/testthat/test-outpack-http-client.R +++ b/tests/testthat/test-outpack-http-client.R @@ -133,111 +133,23 @@ test_that("can use the client to make requests", { test_that("can add auth details to the client", { - auth <- list(url = "http://example.com/api/login", - data = list(token = "mytoken")) - - cl <- outpack_http_client$new("http://example.com", auth) - h <- list("Authorization" = "Bearer yogi") - mock_login <- mockery::mock(h, cycle = TRUE) - mockery::stub(cl$authorise, "http_client_login", mock_login) - - cl$authorise() - - mockery::expect_called(mock_login, 1) - expect_equal( - mockery::mock_args(mock_login)[[1]], - list("http://example.com", - list(enabled = TRUE, url = auth$url, data = auth$data))) - expect_equal(cl$auth$header, h) + mock_authorise <- mockery::mock(h, cycle = TRUE) - ## Second call, does not log in - cl$authorise() - mockery::expect_called(mock_login, 1) + cl <- outpack_http_client$new("http://example.com", mock_authorise) - ## Actually perform an api call now: mock_get <- local_mock_response(json_string("[1,2,3]")) res <- cl$request("/path") expect_mapequal( res, list(status = "success", errors = NULL, data = list(1, 2, 3))) + mockery::expect_called(mock_authorise, 1) mockery::expect_called(mock_get, 1) + args <- mockery::mock_args(mock_get)[[1]] expect_equal(args[[1]]$url, "http://example.com/path") expect_equal(args[[1]]$headers, list(Authorization = "Bearer yogi"), ignore_attr = TRUE) }) - - -test_that("can send authentication request", { - clear_auth_cache() - withr::defer(clear_auth_cache()) - - mock_post <- local_mock_response( - to_json(list(token = jsonlite::unbox("mytoken"))), - wrap = FALSE) - - auth <- list( - url = "https://example.com/login", - data = list(token = "98e02a382db6a3a18e9d2e02c698478b")) - - res <- evaluate_promise( - http_client_login("foo", auth)) - - expect_length(res$messages, 2) - expect_match(res$messages[[1]], "Logging in to foo") - expect_match(res$messages[[2]], "Logged in successfully") - expect_equal(res$result, list("Authorization" = paste("Bearer", "mytoken"))) - - mockery::expect_called(mock_post, 1) - args <- mockery::mock_args(mock_post)[[1]] - expect_equal(args[[1]]$url, auth$url) - expect_equal(args[[1]]$body$data, list(token = auth$data$token)) - expect_equal(args[[1]]$body$type, "json") - - ## And a second time, does not call mock_post again: - expect_silent( - res2 <- http_client_login("foo", auth)) - expect_equal(res2, res$result) - mockery::expect_called(mock_post, 1) -}) - - -test_that("can send authenticated request", { - clear_auth_cache() - withr::defer(clear_auth_cache()) - - mock <- mockery::mock( - mock_response(to_json(list(token = jsonlite::unbox("mytoken"))), - wrap = FALSE), - mock_response(json_string("[1,2,3]"))) - httr2::local_mocked_responses(function(req) mock(req)) - - auth <- list( - url = "https://example.com/api/login", - data = list(token = "98e02a382db6a3a18e9d2e02c698478b")) - - cl <- outpack_http_client$new("http://example.com", auth) - res <- evaluate_promise(cl$request("data")) - - expect_length(res$messages, 2) - expect_match(res$messages[[1]], "Logging in to http://example.com") - expect_match(res$messages[[2]], "Logged in successfully") - expect_mapequal( - res$result, - list(status = "success", errors = NULL, data = list(1, 2, 3))) - - mockery::expect_called(mock, 2) - post_args <- mockery::mock_args(mock)[[1]] - expect_equal(post_args[[1]]$url, auth$url) - expect_equal(post_args[[1]]$body$data, list(token = auth$data$token)) - expect_equal(post_args[[1]]$body$type, "json") - - get_args <- mockery::mock_args(mock)[[2]] - expect_equal(get_args[[1]]$url, "http://example.com/data") - expect_equal(get_args[[1]]$headers, - list(Authorization = paste("Bearer mytoken")), - ignore_attr = TRUE) -})