From 762c88a5f176b740e942e0ecd64034c28561f8f3 Mon Sep 17 00:00:00 2001 From: Paul Lietar Date: Thu, 19 Sep 2024 14:20:28 +0100 Subject: [PATCH 1/2] Add support for specifying Packit tokens directly. As part of experimenting with new authentication methods for Packit, it is useful to be able to provide Packit tokens directly to orderly2. Until now, orderly2 would only accept GitHub tokens, and would perform a request to exchange it for a Packit token. Thankfully, GitHub tokens have a [well defined and documented prefix][ghblog], of the form `ghp_`, `gho_`, ... The underscore makes sure it so that it never could never be confused with a JWT. Thanks to this, the distinction between Packit and GitHub tokens is unambiguous. Eventually we may want to implement some of these authentication methods in orderly2 itself, but it the meantime this makes experimentation easier. [ghblog]: https://github.blog/engineering/platform-security/behind-githubs-new-authentication-token-formats/ --- DESCRIPTION | 2 +- R/location_packit.R | 6 +++++ tests/testthat/test-location-packit.R | 38 ++++++++++++++++++--------- 3 files changed, 32 insertions(+), 14 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index e1415fbf..0d82aa1f 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: orderly2 Title: Orderly Next Generation -Version: 1.99.39 +Version: 1.99.40 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_packit.R b/R/location_packit.R index f3cceb5e..f7fe346e 100644 --- a/R/location_packit.R +++ b/R/location_packit.R @@ -43,6 +43,12 @@ do_oauth_device_flow <- function(base_url, cache_disk) { # server from within the same session. auth_cache <- new.env(parent = emptyenv()) packit_authorisation <- function(base_url, token, save_token) { + # If a non-Github token is provided, we assume it is a native Packit token + # and use that directly. + if (!is.null(token) && !grepl('^gh._', token)) { + return(list("Authorization" = paste("Bearer", token))) + } + key <- rlang::hash(list(base_url = base_url, token = token)) if (is.null(auth_cache[[key]])) { diff --git a/tests/testthat/test-location-packit.R b/tests/testthat/test-location-packit.R index 664c082d..15f2cb23 100644 --- a/tests/testthat/test-location-packit.R +++ b/tests/testthat/test-location-packit.R @@ -1,15 +1,14 @@ -test_that("can authenticate with existing token", { +test_that("can authenticate with existing GitHub token", { clear_auth_cache() withr::defer(clear_auth_cache()) - token <- "my-github-token" + token <- "ghp_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)) + 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") @@ -20,7 +19,7 @@ test_that("can authenticate with existing 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$data, list(token = scalar("ghp_github-token"))) expect_equal(args[[1]]$body$type, "json") ## And a second time, does not call mock_post again: @@ -31,6 +30,18 @@ test_that("can authenticate with existing token", { }) +test_that("can authenticate with existing Packit token", { + clear_auth_cache() + withr::defer(clear_auth_cache()) + + token <- "my-packit-token" + + result <- packit_authorisation("http://example.com/", token) + expect_equal(result, + list("Authorization" = paste("Bearer", "my-packit-token"))) +}) + + test_that("can authenticate using device flow", { clear_auth_cache() withr::defer(clear_auth_cache()) @@ -39,7 +50,8 @@ test_that("can authenticate using device flow", { to_json(list(token = jsonlite::unbox("my-packit-token"))), wrap = FALSE) - mockery::stub(packit_authorisation, "do_oauth_device_flow", "my-github-token") + mockery::stub(packit_authorisation, "do_oauth_device_flow", + "ghp_github-token") res <- evaluate_promise(packit_authorisation("http://example.com/", token = NULL, @@ -54,7 +66,7 @@ test_that("can authenticate using device flow", { 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$data, list(token = scalar("ghp_github-token"))) expect_equal(args[[1]]$body$type, "json") }) @@ -62,7 +74,7 @@ test_that("location_packit uses authentication", { clear_auth_cache() withr::defer(clear_auth_cache()) - token <- "my-github-token" + token <- "ghp_github-token" id <- outpack_id() metadata <- "packet metadata" @@ -85,7 +97,7 @@ test_that("location_packit uses authentication", { 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$data, list(token = scalar("ghp_github-token"))) expect_equal(args[[1]]$body$type, "json") args <- mockery::mock_args(mock)[[2]] @@ -126,8 +138,8 @@ test_that("Can configure oauth caching behaviour", { 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")) + c("GITHUB_TOKEN" = "ghp_abc123"), + orderly_location_packit("http://example.com", "$GITHUB_TOKEN")) mock_login <- local_mock_response( to_json(list(token = jsonlite::unbox("my-packit-token"))), @@ -139,12 +151,12 @@ test_that("can create a packit location using an environment variable token", { 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$data, list(token = scalar("ghp_abc123"))) expect_equal(args[[1]]$body$type, "json") }) -test_that("error of token variable not found", { +test_that("error if token variable not found", { withr::with_envvar( c("PACKIT_TOKEN" = NA_character_), expect_error( From 59475068aad13a2b9de3f6fc168046006749972f Mon Sep 17 00:00:00 2001 From: Paul Lietar Date: Fri, 20 Sep 2024 14:59:43 +0100 Subject: [PATCH 2/2] CF --- R/location_packit.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/location_packit.R b/R/location_packit.R index f7fe346e..5f32dcd0 100644 --- a/R/location_packit.R +++ b/R/location_packit.R @@ -45,7 +45,7 @@ auth_cache <- new.env(parent = emptyenv()) packit_authorisation <- function(base_url, token, save_token) { # If a non-Github token is provided, we assume it is a native Packit token # and use that directly. - if (!is.null(token) && !grepl('^gh._', token)) { + if (!is.null(token) && !grepl("^gh._", token)) { return(list("Authorization" = paste("Bearer", token))) }