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

If not in an interactive session, can gargle_oauth_email() default to TRUE? #92

Closed
MarkEdmondson1234 opened this issue Jul 7, 2019 · 14 comments

Comments

@MarkEdmondson1234
Copy link
Contributor

@MarkEdmondson1234 MarkEdmondson1234 commented Jul 7, 2019

To help with non-interactive sessions, in the context of the advice on the website here:

Could this be made even smoother by detecting if in an interactive session or not (e.g. if(interactive()), and if FALSE defaulting the gargle_oauth_email = TRUE option?

It will cover 95% of the use cases I've seen (where a user has one cache token on a server for scheduled tasks) and prevent a potential hangup when the option is not set and the interactive menu comes up. I favour the JSON service email option instead, but that usually involves adding the service email to accounts that may not be possible.

@jennybc

This comment has been minimized.

Copy link
Member

@jennybc jennybc commented Jul 8, 2019

Hmm, I'll have to think about it. I totally agree it would remove a lot of failure. The question is: should we? The original plan was to be exceedingly careful -- maybe even, pedantic -- about getting the user's consent to use a token that we find.

@MarkEdmondson1234

This comment has been minimized.

Copy link
Contributor Author

@MarkEdmondson1234 MarkEdmondson1234 commented Jul 8, 2019

If not, perhaps the error returned could give a hint on what has happened. "Do you need to set option(gargle_oauth_email) ?" I'll try it on a non-interactive script to see what happens now, for httr it was a cryptic error about not connecting to a server IIRC.

@MarkEdmondson1234

This comment has been minimized.

Copy link
Contributor Author

@MarkEdmondson1234 MarkEdmondson1234 commented Jul 13, 2019

The most pertinent example of this is when attempting auth in an RMarkdown doc - if not set correctly then the rendering just hangs, with no error, which is mega-confusing for folks not intimate with the auth process. For googleAuthR I've put in the below thats tested before the call to token_fetch() but I reckon something similar would be a good addition in gargle too:

  # to aid non-interactive scripts
  if(is.null(email) 
     && !interactive() 
     && is.null(getOption("gargle_oauth_email"))){
    stop("Non-interactive session and no authentication email selected.
         \nSetup JSON service email auth or specify email in gar_auth(email='me@preauthenticated.com'", call.=FALSE)
  }
@EricGoldsmith

This comment has been minimized.

Copy link

@EricGoldsmith EricGoldsmith commented Aug 22, 2019

+1 for this request. The current behavior is a breaking change from httr OAuth cache handling.

@jennybc

This comment has been minimized.

Copy link
Member

@jennybc jennybc commented Aug 23, 2019

Yeah, maybe we could check if interactive or for an env var that suggests we're knitting and act as if email = TRUE, but warn that the user needs to specify this clearly in code to suppress the warning.

@jennybc

This comment has been minimized.

Copy link
Member

@jennybc jennybc commented Aug 23, 2019

Also, just to spread the knowledge, this situation is addressed specifically in the Non-interactive auth vignette: https://gargle.r-lib.org/articles/non-interactive-auth.html#i-just-want-my--rmd-to-render

@EricGoldsmith

This comment has been minimized.

Copy link

@EricGoldsmith EricGoldsmith commented Aug 23, 2019

Yeah, that vingette was how I found the workaround/solution.

In addition to the use-case that Mark mentioned, I have several anomaly detection and reporting scripts that run from cron/launchd that rely on cached credentials. Since upgrading recently to the packages that use gargle, I've had to make code changes (adding options(gargle_oauth_email = TRUE)) to restore the prior (transparent) credential caching behavior.

So, I'm very much in favor of making email = TRUE the default for non-interactive sessions. But it's also a convenience for interactive sessions, to mimic the prior behavior.

To net this out, I'd suggest that if there's only one credential in the cache, use it by default. Only prompt the user if there's more than one to choose from (but still allow this to be overridden via options(gargle_oauth_email = "jenny@example.org")).

Perhaps I'm being too prescriptive here, based on my use-cases. With all the work you've done on various packages, I'm sure you have a more informed view of the landscape. So, I'll just stick with relaying my desire for backward compatibility with httr's cached credential use behavior.

@jennybc

This comment has been minimized.

Copy link
Member

@jennybc jennybc commented Aug 25, 2019

Our increased commitment to getting permission to use user creds is due to an overall awareness of the need to be really transparent when accessing and using sensitive data. I think jobs running on remote servers should probably be using service account tokens, for example. OAuth2 user tokens are really designed for use by actual users, hence our design choices. I think the level of magic in httr is partially responsible for lots of people pushing OAuth2 creds to GitHub, because they have so little awareness of them. I will not link to this 😱 search, but let's just say there are several hundreds of these.

So I agree we could switch to a "use and warn" strategy. But I'd really like these remote-running scripts to contain code that indicates clear permission to use auto-discovered user credentials.

@MarkEdmondson1234

This comment has been minimized.

Copy link
Contributor Author

@MarkEdmondson1234 MarkEdmondson1234 commented Aug 25, 2019

For me, the warning or even a stop() would solve the problem of sitting there wondering why the script isn't working. It is a small change once you know what it is, the main issue I think is diagnosing what the problem is.

That way individual packages can decide how much they want to support OAuth2 tokens vs nudging to JSON creds. For the reasons mentioned above I removed OAuth2 support for compute engine/cloud storage as it wasn't worth it, but for the more public APIs there is less critical stuff at risk (e.g.not stuff you can be charged for)

@jennybc jennybc added this to the v0.4.0 milestone Oct 2, 2019
@jennybc

This comment has been minimized.

Copy link
Member

@jennybc jennybc commented Oct 2, 2019

Quick note to self: succeed but warn

Update: I can't literally warn, because of tryCatch() in token_fetch(), but I can emit some messaging.

@jennybc jennybc closed this in 44936f8 Oct 3, 2019
@jennybc

This comment has been minimized.

Copy link
Member

@jennybc jennybc commented Oct 3, 2019

@MarkEdmondson1234 @EricGoldsmith Please try this out and let me know if you have any feedback.

@EricGoldsmith

This comment has been minimized.

Copy link

@EricGoldsmith EricGoldsmith commented Oct 23, 2019

I appreciate the compromise. Thanks.

@EricGoldsmith

This comment has been minimized.

Copy link

@EricGoldsmith EricGoldsmith commented Nov 4, 2019

Follow-up question ...

With gargle v0.4.0, I get the following warning:

Using an auto-discovered, cached token.
To suppress this message, modify your code or options to clearly consent to the use of a cached token.
See gargle's "Non-interactive auth" vignette for more details:
https://gargle.r-lib.org/articles/non-interactive-auth.html
The gmailr package is using a cached token for <my email address>.

But, the first line of my code is:

options(gargle_oauth_email = TRUE)

which is cited as a solution to the warning in the vignette referenced in the message.

I want to make sure I'm not misunderstanding the warning or the docs before opening a new issue for this.

@jennybc

This comment has been minimized.

Copy link
Member

@jennybc jennybc commented Nov 4, 2019

Yeah, at the moment, I only suppress the warning message when the user gives a specific email address, continuing to be very conservative. I could be persuaded that the generic TRUE is sufficient. Do you have any specific reason to use TRUE instead of an actual email here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.