Skip to content

Commit

Permalink
Add support for specifying Packit tokens directly.
Browse files Browse the repository at this point in the history
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/
  • Loading branch information
plietar committed Sep 20, 2024
1 parent 3ccdada commit 762c88a
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 14 deletions.
2 changes: 1 addition & 1 deletion DESCRIPTION
Original file line number Diff line number Diff line change
@@ -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 = "[email protected]"),
person("Robert", "Ashton", role = "aut"),
Expand Down
6 changes: 6 additions & 0 deletions R/location_packit.R
Original file line number Diff line number Diff line change
Expand Up @@ -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]])) {
Expand Down
38 changes: 25 additions & 13 deletions tests/testthat/test-location-packit.R
Original file line number Diff line number Diff line change
@@ -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")
Expand All @@ -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:
Expand All @@ -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())
Expand All @@ -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,
Expand All @@ -54,15 +66,15 @@ 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")
})

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"

Expand All @@ -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]]
Expand Down Expand Up @@ -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"))),
Expand All @@ -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(
Expand Down

0 comments on commit 762c88a

Please sign in to comment.