From 4de16ff441ea72bd985e10a45c0e3f653efed6f2 Mon Sep 17 00:00:00 2001 From: Michael Thomson Date: Fri, 29 Sep 2023 14:49:23 -0700 Subject: [PATCH 1/7] enable oauth_flow_auth_code to parse URL --- NEWS.md | 2 ++ R/oauth-flow-auth-code.R | 34 ++++++++++++++-------- tests/testthat/test-oauth-flow-auth-code.R | 16 ++++++++-- 3 files changed, 38 insertions(+), 14 deletions(-) diff --git a/NEWS.md b/NEWS.md index d046830c..68c7b1f6 100644 --- a/NEWS.md +++ b/NEWS.md @@ -38,6 +38,8 @@ * `oauth_flow_auth_code()` deprecates `host_name` and `port` arguments in favour of using `redirect_uri`. It also deprecates `host_ip` since it seems unlikely that changing this is ever useful. + +* `oauth_flow_auth_code()` allows for user to enter url (after resolving request sent to `redirect_uri`) that contains an authorization `code` and `state` to needing to manual parsing (@fh-mthomson, #TBD). * New `oauth_cache_path()` returns the path that httr2 uses for caching OAuth tokens. Additionally, you can now change the cache location by setting the diff --git a/R/oauth-flow-auth-code.R b/R/oauth-flow-auth-code.R index 65c55f63..54f31663 100644 --- a/R/oauth-flow-auth-code.R +++ b/R/oauth-flow-auth-code.R @@ -413,26 +413,36 @@ is_hosted_session <- function() { } oauth_flow_auth_code_read <- function(state) { - code <- trimws(readline("Enter authorization code: ")) - # We support two options here: + code <- trimws(readline("Enter authorization code or URL: ")) + # We support several options here: + # 1) Parsing the code and state from the resolved URL on the redirect page # - # 1) The original {gargle} style, where the user copy & pastes a + # 2) The original {gargle} style, where the user copy & pastes a # base64-encoded JSON object with both the code and state. This is used on # https://www.tidyverse.org/google-callback/; and # - # 2) The full manual approach, where the code and state are entered + # 3) The full manual approach, where the code and state are entered # independently. - result <- tryCatch( - jsonlite::fromJSON(rawToChar(openssl::base64_decode(code))), - error = function(e) { - list( - code = code, - state = trimws(readline("Enter state parameter: ")) - ) - }) + + parsed <- url_parse(code) + + if (!is.null(parsed$query)) { + result <- parsed$query + } else { + result <- tryCatch( + jsonlite::fromJSON(rawToChar(openssl::base64_decode(code))), + error = function(e) { + list( + code = code, + state = trimws(readline("Enter state parameter: ")) + ) + }) + } + if (!identical(result$state, state)) { abort("Authentication failure: state does not match") } + result$code } diff --git a/tests/testthat/test-oauth-flow-auth-code.R b/tests/testthat/test-oauth-flow-auth-code.R index 708b7c4c..1b11a96c 100644 --- a/tests/testthat/test-oauth-flow-auth-code.R +++ b/tests/testthat/test-oauth-flow-auth-code.R @@ -20,14 +20,26 @@ test_that("so-called 'hosted' sessions are detected correctly", { }) }) +test_that("URL embedding authorisation code and state can be input manually", { + state <- base64_url_rand(32) + code <- "def456" + url <- glue("https://x.com?code={code}&state={state}") + local_mocked_bindings( + readline = function(prompt = "") url + ) + expect_equal(oauth_flow_auth_code_read(state), code) + expect_error(oauth_flow_auth_code_read("invalid"), "state does not match") +}) + test_that("JSON-encoded authorisation codes can be input manually", { state <- base64_url_rand(32) - input <- list(state = state, code = "abc123") + code <- "abc123" + input <- list(state = state, code = code) encoded <- openssl::base64_encode(jsonlite::toJSON(input)) local_mocked_bindings( readline = function(prompt = "") encoded ) - expect_equal(oauth_flow_auth_code_read(state), "abc123") + expect_equal(oauth_flow_auth_code_read(state), code) expect_error(oauth_flow_auth_code_read("invalid"), "state does not match") }) From cf13cff30d6abab722e87fc11cc53595152a8a27 Mon Sep 17 00:00:00 2001 From: Michael Thomson Date: Fri, 29 Sep 2023 14:56:46 -0700 Subject: [PATCH 2/7] typo --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 68c7b1f6..751a9ff6 100644 --- a/NEWS.md +++ b/NEWS.md @@ -39,7 +39,7 @@ of using `redirect_uri`. It also deprecates `host_ip` since it seems unlikely that changing this is ever useful. -* `oauth_flow_auth_code()` allows for user to enter url (after resolving request sent to `redirect_uri`) that contains an authorization `code` and `state` to needing to manual parsing (@fh-mthomson, #TBD). +* `oauth_flow_auth_code()` allows for user to enter url (after resolving request sent to `redirect_uri`) that contains an authorization `code` and `state` to avoid manual parsing (@fh-mthomson, #TBD). * New `oauth_cache_path()` returns the path that httr2 uses for caching OAuth tokens. Additionally, you can now change the cache location by setting the From d850df289ce8d0b0313d16c4fa7042c479ecd177 Mon Sep 17 00:00:00 2001 From: Michael Thomson Date: Fri, 29 Sep 2023 14:58:10 -0700 Subject: [PATCH 3/7] more restrictive result --- R/oauth-flow-auth-code.R | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/R/oauth-flow-auth-code.R b/R/oauth-flow-auth-code.R index 54f31663..dc29c6f5 100644 --- a/R/oauth-flow-auth-code.R +++ b/R/oauth-flow-auth-code.R @@ -427,7 +427,10 @@ oauth_flow_auth_code_read <- function(state) { parsed <- url_parse(code) if (!is.null(parsed$query)) { - result <- parsed$query + result <- list( + parsed$query$code, + parsed$query$state + ) } else { result <- tryCatch( jsonlite::fromJSON(rawToChar(openssl::base64_decode(code))), From 246db1403c81c3d1c98bdf6f6229b1bd13684b25 Mon Sep 17 00:00:00 2001 From: Michael Thomson Date: Fri, 29 Sep 2023 15:03:23 -0700 Subject: [PATCH 4/7] add PR num --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 751a9ff6..7c9307fc 100644 --- a/NEWS.md +++ b/NEWS.md @@ -39,7 +39,7 @@ of using `redirect_uri`. It also deprecates `host_ip` since it seems unlikely that changing this is ever useful. -* `oauth_flow_auth_code()` allows for user to enter url (after resolving request sent to `redirect_uri`) that contains an authorization `code` and `state` to avoid manual parsing (@fh-mthomson, #TBD). +* `oauth_flow_auth_code()` allows for user to enter url (after resolving request sent to `redirect_uri`) that contains an authorization `code` and `state` to avoid manual parsing (@fh-mthomson, #326). * New `oauth_cache_path()` returns the path that httr2 uses for caching OAuth tokens. Additionally, you can now change the cache location by setting the From dc0c4c8d20f44c680795380787bca68b66084903 Mon Sep 17 00:00:00 2001 From: Michael Thomson Date: Fri, 29 Sep 2023 15:07:19 -0700 Subject: [PATCH 5/7] add names back to list --- R/oauth-flow-auth-code.R | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/R/oauth-flow-auth-code.R b/R/oauth-flow-auth-code.R index dc29c6f5..4c08a4d4 100644 --- a/R/oauth-flow-auth-code.R +++ b/R/oauth-flow-auth-code.R @@ -427,10 +427,7 @@ oauth_flow_auth_code_read <- function(state) { parsed <- url_parse(code) if (!is.null(parsed$query)) { - result <- list( - parsed$query$code, - parsed$query$state - ) + result <- parsed$query[c("code", "state")] } else { result <- tryCatch( jsonlite::fromJSON(rawToChar(openssl::base64_decode(code))), From 65c67c1e4450ed0330441395981f2043809e2950 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 11 Oct 2023 17:55:59 -0500 Subject: [PATCH 6/7] Polish news --- NEWS.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/NEWS.md b/NEWS.md index 3cbf72ea..47d544e0 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,8 @@ # httr2 (development version) +* `oauth_flow_auth_code()` allows the user to enter a URL that contains + authorization `code` and `state` parameters (@fh-mthomson, #326). + * `req_oauth_device()` now takes a `auth_url` parameter making it usable (#331, @taerwin). @@ -45,8 +48,6 @@ of using `redirect_uri`. It also deprecates `host_ip` since it seems unlikely that changing this is ever useful. -* `oauth_flow_auth_code()` allows for user to enter url (after resolving request sent to `redirect_uri`) that contains an authorization `code` and `state` to avoid manual parsing (@fh-mthomson, #326). - * New `oauth_cache_path()` returns the path that httr2 uses for caching OAuth tokens. Additionally, you can now change the cache location by setting the `HTTR2_OAUTH_CACHE` env var. From 6d29b7d794aa68263a7024f66356bb8f71920321 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 11 Oct 2023 18:08:13 -0500 Subject: [PATCH 7/7] Make logic a bit more clear --- R/oauth-flow-auth-code.R | 54 +++++++++++++--------- tests/testthat/test-oauth-flow-auth-code.R | 13 ++---- 2 files changed, 36 insertions(+), 31 deletions(-) diff --git a/R/oauth-flow-auth-code.R b/R/oauth-flow-auth-code.R index c542df9f..2fa3c7ea 100644 --- a/R/oauth-flow-auth-code.R +++ b/R/oauth-flow-auth-code.R @@ -438,38 +438,48 @@ is_hosted_session <- function() { oauth_flow_auth_code_read <- function(state) { code <- trimws(readline("Enter authorization code or URL: ")) - # We support several options here: - # 1) Parsing the code and state from the resolved URL on the redirect page - # - # 2) The original {gargle} style, where the user copy & pastes a - # base64-encoded JSON object with both the code and state. This is used on - # https://www.tidyverse.org/google-callback/; and - # - # 3) The full manual approach, where the code and state are entered - # independently. - parsed <- url_parse(code) + if (is_string_url(code)) { + # minimal setup where user copy & pastes a URL + parsed <- url_parse(code) + + code <- parsed$query$code + new_state <- parsed$query$state + } else if (is_base64_json(code)) { + # {gargle} style, where the user copy & pastes a base64-encoded JSON + # object with both the code and state. This is used on + # https://www.tidyverse.org/google-callback/ + json <- jsonlite::fromJSON(rawToChar(openssl::base64_decode(code))) - if (!is.null(parsed$query)) { - result <- parsed$query[c("code", "state")] + code <- json$code + new_state <- json$state } else { - result <- tryCatch( - jsonlite::fromJSON(rawToChar(openssl::base64_decode(code))), - error = function(e) { - list( - code = code, - state = trimws(readline("Enter state parameter: ")) - ) - }) + # Full manual approach, where the code and state are entered + # independently. + + new_state <- trimws(readline("Enter state parameter: ")) } - if (!identical(result$state, state)) { + if (!identical(state, new_state)) { abort("Authentication failure: state does not match") } - result$code + code } +is_string_url <- function(x) grepl("^https?://", x) + +is_base64_json <- function(x) { + tryCatch( + { + jsonlite::fromJSON(rawToChar(openssl::base64_decode(x))) + TRUE + }, + error = function(err) FALSE + ) +} + + # Determine whether we can fetch the OAuth authorization code from an external # source without user interaction. can_fetch_oauth_code <- function(redirect_url) { diff --git a/tests/testthat/test-oauth-flow-auth-code.R b/tests/testthat/test-oauth-flow-auth-code.R index fa67af65..0ffe0e6e 100644 --- a/tests/testthat/test-oauth-flow-auth-code.R +++ b/tests/testthat/test-oauth-flow-auth-code.R @@ -21,25 +21,20 @@ test_that("so-called 'hosted' sessions are detected correctly", { }) test_that("URL embedding authorisation code and state can be input manually", { - state <- base64_url_rand(32) - code <- "def456" - url <- glue("https://x.com?code={code}&state={state}") local_mocked_bindings( - readline = function(prompt = "") url + readline = function(prompt = "") "https://x.com?code=code&state=state" ) - expect_equal(oauth_flow_auth_code_read(state), code) + expect_equal(oauth_flow_auth_code_read("state"), "code") expect_error(oauth_flow_auth_code_read("invalid"), "state does not match") }) test_that("JSON-encoded authorisation codes can be input manually", { - state <- base64_url_rand(32) - code <- "abc123" - input <- list(state = state, code = code) + input <- list(state = "state", code = "code") encoded <- openssl::base64_encode(jsonlite::toJSON(input)) local_mocked_bindings( readline = function(prompt = "") encoded ) - expect_equal(oauth_flow_auth_code_read(state), code) + expect_equal(oauth_flow_auth_code_read("state"), "code") expect_error(oauth_flow_auth_code_read("invalid"), "state does not match") })