diff --git a/R/location_http.R b/R/location_http.R index 564d15e6..4d153396 100644 --- a/R/location_http.R +++ b/R/location_http.R @@ -92,10 +92,13 @@ orderly_location_packit <- function(url, token) { token <- Sys.getenv(token_variable, NA_character_) if (is.na(token)) { cli::cli_abort( - "Environment value '{token_variable}' was not set") + "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") diff --git a/R/outpack_http_client.R b/R/outpack_http_client.R index 87f9457b..d5fe4630 100644 --- a/R/outpack_http_client.R +++ b/R/outpack_http_client.R @@ -1,40 +1,36 @@ outpack_http_client <- R6::R6Class( "outpack_http_client", - private = list( - auth = NULL - ), - public = list( url = NULL, - token = NULL, + auth = NULL, initialize = function(url, auth) { - self$url <- url + self$url <- sub("/$", "", url) if (is.null(auth)) { - private$auth <- list(enabled = FALSE) + self$auth <- list(enabled = FALSE) } else { - private$auth <- list(enabled = TRUE, url = auth$url, data = auth$data) + self$auth <- list(enabled = TRUE, url = auth$url, data = auth$data) } }, authorise = function() { - needs_auth <- private$auth$enabled && is.null(private$auth$header) + needs_auth <- self$auth$enabled && is.null(self$auth$header) if (needs_auth) { - private$auth$header <- http_client_login(self$url, private$auth) + self$auth$header <- http_client_login(self$url, self$auth) } }, get = function(path, ...) { self$authorise() http_client_request(httr::GET, paste0(self$url, path), ..., - private$auth$header) + self$auth$header) }, post = function(path, body, ...) { self$authorise() http_client_request(httr::POST, paste0(self$url, path), body = body, ..., - private$auth$header) + self$auth$header) } )) diff --git a/tests/testthat/helper-outpack-http.R b/tests/testthat/helper-outpack-http.R index e0b6ecd2..918b50e5 100644 --- a/tests/testthat/helper-outpack-http.R +++ b/tests/testthat/helper-outpack-http.R @@ -29,3 +29,8 @@ json_string <- function(s) { class(s) <- "json" s } + + +clear_auth_cache <- function() { + rm(list = ls(auth_cache), envir = auth_cache) +} diff --git a/tests/testthat/test-location.R b/tests/testthat/test-location.R index 2496f624..6036dd6a 100644 --- a/tests/testthat/test-location.R +++ b/tests/testthat/test-location.R @@ -734,6 +734,12 @@ test_that("validate arguments to path locations", { orderly_location_add("other", "path", list(root = "mypath"), root = root), "Field missing from args: 'path'") + expect_equal(orderly_location_list(root = root), "local") +}) + + +test_that("validate arguments to http locations", { + root <- create_temporary_root() expect_error( orderly_location_add("other", "http", list(server = "example.com"), root = root), @@ -742,6 +748,88 @@ test_that("validate arguments to path locations", { }) +test_that("validate arguments to packit locations", { + root <- create_temporary_root() + expect_error( + orderly_location_add("other", "packit", list(server = "example.com"), + root = root), + "Fields missing from args: 'url' and 'token'") + expect_error( + orderly_location_add("other", "packit", list(url = "example.com"), + root = root), + "Field missing from args: 'token'") + expect_equal(orderly_location_list(root = root), "local") +}) + + +test_that("can add a packit location", { + root <- create_temporary_root() + orderly_location_add("other", "packit", + list(url = "example.com", token = "abc123"), + 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")))) + expect_equal( + client$url, + "https://example.com/packit/api/outpack") +}) + + +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$url, + "https://example.com") +}) + + +test_that("strip trailing slash from outpack url", { + loc <- orderly_location_http$new("https://example.com/", NULL) + expect_equal( + loc$.__enclos_env__$private$client$url, + "https://example.com") +}) + + test_that("can load a custom location driver", { skip_if_not_installed("mockery") mock_driver <- mockery::mock("value") diff --git a/tests/testthat/test-outpack-http-client.R b/tests/testthat/test-outpack-http-client.R index 75112203..200ca9fe 100644 --- a/tests/testthat/test-outpack-http-client.R +++ b/tests/testthat/test-outpack-http-client.R @@ -100,3 +100,73 @@ test_that("can use the client to make requests", { expect_equal(mockery::mock_args(mock_get)[[1]], list("http://example.com/path", NULL)) }) + + +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 <- httr::add_headers("Authorization" = paste("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) + + ## Second call, does not log in + cl$authorise() + mockery::expect_called(mock_login, 1) + + ## Actually perform an api call now: + mock_get <- mockery::mock(mock_response(json_string("[1,2,3]"))) + mockery::stub(cl$get, "httr::GET", mock_get) + res <- cl$get("/path") + expect_mapequal( + res, + list(status = "success", errors = NULL, data = list(1, 2, 3))) + mockery::expect_called(mock_get, 1) + expect_equal(mockery::mock_args(mock_get)[[1]], + list("http://example.com/path", h)) +}) + + +test_that("can send authentication request", { + clear_auth_cache() + withr::defer(clear_auth_cache()) + + auth <- list( + url = "https://example.com/login", + data = list(token = ids::random_id())) + mock_post <- mockery::mock( + mock_response( + to_json(list(token = jsonlite::unbox("mytoken"))), + wrap = FALSE)) + + mockery::stub(http_client_login, "httr::POST", mock_post) + + 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, + httr::add_headers("Authorization" = paste("Bearer", "mytoken"))) + + mockery::expect_called(mock_post, 1) + expect_equal(mockery::mock_args(mock_post)[[1]], + list(auth$url, + body = list(token = auth$data$token), + encode = "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) +})