Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use rstudioapi instead of readline if calling oauth_flow_auth_code() from RStudio IDE #410

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

fh-mthomson
Copy link
Contributor

@fh-mthomson fh-mthomson commented Dec 12, 2023

Closes #406

Test plan

Running oauth_flow_auth_code_read("state") successfully prompts twice (initial prompt, then state) in either case described in parent issue:

  1. sequential R chunks run in .Rmd
  2. knitting .Rmd

Implementation considerations

  • Initially implemented a version of using only askpass, given broader generalizability outside of RStudio IDE; however, this doesn't scale as well off the shelf to the non-interactive use case (e.g., knitting in RStudio) - per ?askpass::askpass:
By default askpass() returns NULL in non-interactive sessions. (These include knitr runs and testthat tests.) If you want to force a password prompt in non-interactive sessions, set the rlib_interactive option to TRUE:
options(rlib_interactive = TRUE)
  • Opted instead for an initial implementation of rstudioapi > askpass, but this may be too layered - open to any feedback!
  • Also considered getPass but it seems less mature / robust.

Note: leaving this instance of readline() since it's not really used to solicit a character string from the user and is likely better handled by another prompt function, anyway.

@fh-mthomson fh-mthomson changed the title use askpass instead of readline for oauth_flow_auth_code use rstudioapi or askpass instead of readline for oauth_flow_auth_code Dec 12, 2023
@jennybc
Copy link
Member

jennybc commented Dec 12, 2023

In case it's relevant, gargle uses readline() for very specific reasons, which you can read about here:

r-lib/gargle#242

The short version is to support Jupyter notebooks and, in particular, Google Colab.

@fh-mthomson fh-mthomson changed the title use rstudioapi or askpass instead of readline for oauth_flow_auth_code use rstudioapi instead of readline if calling oauth_flow_auth_code() from RStudio IDE Dec 21, 2023
@fh-mthomson
Copy link
Contributor Author

In case it's relevant, gargle uses readline() for very specific reasons, which you can read about here:

r-lib/gargle#242

The short version is to support Jupyter notebooks and, in particular, Google Colab.

Super relevant! I simplified a bit to only use rstudioapi if running in RStudio IDE; otherways, stick with readline(). Lmk if this strikes a better balance.

I do think this PR is a meaningful improvement; it would prevent folks on our end from missing an Okta re-auth prompt several times a day.

R/oauth-flow-password.R Outdated Show resolved Hide resolved
R/oauth-flow-password.R Outdated Show resolved Hide resolved
R/oauth-flow-auth-code.R Outdated Show resolved Hide resolved
R/oauth-flow-auth-code.R Outdated Show resolved Hide resolved
if (is_rstudio_session()) {
check_installed("rstudioapi")
result <- rstudioapi::askForPassword(prompt)
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this have an !is_interactive() branch with an informative error?

This comment was marked as duplicate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two scenarios running outside of RStudio that I'm worried about:

  • You're running R interactively and call knit(): does readline() cause execution to pause? Do you see the correct prompt?
  • You're running R in batch mode and call knit(): won't readline() will return "" causing an authentication problem that's hard to diagnose?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any more thoughts on this comment? I think it's the main blocker for merging this PR.

Copy link
Contributor Author

@fh-mthomson fh-mthomson Mar 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay while juggling some other things. Of note, this PR is a specific workaround for Snowflake auth in Workbench and was before I chatted with @atheriel about his efforts to integrate OAuth more directly via Workbench (rather than our current wrappers around httr2 which have been holding up reasonably well).

Regardless, I had a chance to think a bit more about this, motivated by Quarto's execution approach not supporting interactive user prompts.

I've pushed a change, per your suggestion and very helpful test cases! I think it's closer to a reasonable approach.

Current behavior:

  1. In RStudio:
    a. .rmd -> knitting: pauses to receive prompt (new, yay!)
    b. .qmd -> render: fails with an expected, or at least, well-documented error:
Error:
! RStudio not running
  1. Outside of RStudio: both of your test cases:
## Error in `prompt_user()` at script.R:18:3:
## ! Unable to obtain user input in a non-interactive session.

Open to any thoughts!

@hadley
Copy link
Member

hadley commented Dec 21, 2023

Thanks for working on this!

R/oauth-flow-auth-code.R Outdated Show resolved Hide resolved
R/oauth-flow-auth-code.R Show resolved Hide resolved
if (is_rstudio_session()) {
check_installed("rstudioapi")
result <- rstudioapi::askForPassword(prompt)
} else {

This comment was marked as duplicate.

vignettes/articles/wrapping-apis.Rmd Outdated Show resolved Hide resolved
if (is_rstudio_session()) {
check_installed("rstudioapi")
result <- rstudioapi::askForPassword(prompt)
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two scenarios running outside of RStudio that I'm worried about:

  • You're running R interactively and call knit(): does readline() cause execution to pause? Do you see the correct prompt?
  • You're running R in batch mode and call knit(): won't readline() will return "" causing an authentication problem that's hard to diagnose?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable user response when oauth_flow_auth_code() is called indirectly
3 participants