JWT authentication is alive!

[nathancday]

New function to handle authentication box_auth_jwt() via a Box Developer JWT app. Proof of concept work in internal boxGet() to show box_auth_jwt() can coexist with existing box_auth() syntax/structure. Addresses the discussion in #92 and should work to fix #23, but have not tested yet.

IJL edit: Will also fix #88

A little template to follow on your local machine:

devtools::load_all(".")
# stored in package for automated testing purposes
config_file <- system.file("tools", "boxr_config.json", package = "boxr")

# auth-yourself as me
box_auth_jwt(config_file, user_id = "6513355836")

# only modified boxGet() for proof of concept
boxr:::boxGet(file_id = 410086485840,
              local_file = "~/Desktop/test.json",
              download = TRUE, pb = TRUE)

Concerns:

• I know we have a discussion about writing to System files and I am using still using the old pattern of key pieces with Sys.env and options() here.

• No automatic re-authentication behavior yet, but I don't know if we will ever need that.

• Needs to be test in a server situation.

• Needs a good walk through to set up a JWT app set up on the Box Dev portal, similar the O-Auth2 doc.

[codecov-io]

Codecov Report

Merging #105 into master will increase coverage by 0.08%.
The diff coverage is 1.55%.

@@            Coverage Diff            @@
##           master    #105      +/-   ##
=========================================
+ Coverage    1.24%   1.32%   +0.08%     
=========================================
  Files          22      24       +2     
  Lines        1367    1506     +139     
=========================================
+ Hits           17      20       +3     
- Misses       1350    1486     +136

Impacted Files	Coverage Δ
R/boxr__internal_misc.R	13.39% <0%> (-0.37%)	⬇️
R/zzz.R	14.28% <0%> (-1.1%)	⬇️
R/boxr_delete_restore.R	0% <0%> (ø)	⬆️
R/utils-packages.R	0% <0%> (ø)
R/boxr_file_versions.R	0% <0%> (ø)	⬆️
R/boxr_add_description.R	0% <0%> (ø)	⬆️
R/boxr_misc.R	0% <0%> (ø)	⬆️
R/boxr__internal_get.R	0% <0%> (ø)	⬆️
R/boxr_search.R	0% <0%> (ø)	⬆️
R/boxr_invite.R	0% <0%> (ø)
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3963b41...53b3276. Read the comment docs.

[ijlyttle]

Hi Nate,

Just checking in to give evidence of proof-of-life :)

Looks good so far!

I have a picky suggestion and also a question about the tools/box_config.json file.

[ijlyttle]

Hi Nate - I know I'm being picky here: == can be vectorized, would you consider:

if (identical(config_file, "")) {

As this can return only a single TRUE or FALSE.

[nathancday]

Absolutely, I was mirroring the existing patterns in box_auth, but I think your suggestion is better. I'll update the existing places where == is used to check arguments in the auth functions. If left to my own devices I usually would use NULL over the empty string, do you have insight about one pattern being superior to the other?

[ijlyttle]

I think we are on the same page here - I like the NULL pattern paired with documenting the default, then using rlang::%||% (which we should consider importing internally, if we do not already).

It's a philosophical question about how/when to change the API, but I would have no problem starting now with new functionality.

[ijlyttle]

Here, too... there are others, but you get the idea...

[ijlyttle]

Is there an issue with this file being here? It seems potentially private...

[nathancday]

It is "mildly" private in the sense somebody could use it to modify that accounts Box workspace. But that account is for testing purposes so it doesn't have anything valuable in it and I can always revoke tokens if one gets compromised.

I'm sure there is a better place to store sensitive things like this, but I wanted to share bc I was thinking this would be part of the Travis build/testing. I have limited experience with Travis build args/configuration but that was my next step after JWT.

[ijlyttle]

Glad to hear - on my list of things to do is to figure out a simpler way to encrypt tokens for testing and to remain CRAN-friendly. The current reference is this: https://cran.r-project.org/web/packages/googlesheets/vignettes/managing-auth-tokens.html, but there has to be a simpler way.

Maybe along the lines being used for deploying pkgdown? https://pkgdown.r-lib.org/reference/deploy_site_github.html

[ijlyttle]

Just to note, the tidyverse team have come up with a cleaner way to manage credentials for testing: https://gargle.r-lib.org/articles/articles/managing-tokens-securely.html

I think that implementing this will be a separate issue, but just to note it here.

[nathancday]

@ijlyttle I think things are ready to test in the wild. All of the functions seem to be working, and all of the tests are passing with JWT.

I've updated the vignette to cover setting up a JWT application, and wanted to check with you about updating the downstream docs on pkgdown.

The indentical(/NULL) fix is in place, but the code could still use a solid review.

My plan is to share this with co-workers next week and collect feedback. What are your thoughts?

[ijlyttle]

Hi @nathancday - with lots of apologies, let me get back on track with this PR. Just to be clear, are you saying this is ready to go? Either way, I'll start taking a look.

[nathancday]

Zero worries. In my limited use it's seems to work for all existing tests. I do still need to figure out how to pass the token to Travis. And I think there still needs to be a discussion/adjustments about best practices for storing relevant credentials. I.e. Renviron editing

…

On Sat, Jul 20, 2019 at 3:28 PM Ian Lyttle ***@***.***> wrote: Hi @nathancday </~https://github.com/nathancday> - with lots of apologies, let me get back on track with this PR. Just to be clear, are you saying this is ready to go? Either way, I'll start taking a look. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#105?email_source=notifications&email_token=AAGFKIIGIRI264HAT4U3BBTQANROHA5CNFSM4HHIT5FKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2NURHI#issuecomment-513493149>, or mute the thread </~https://github.com/notifications/unsubscribe-auth/AAGFKIIBS34NGD4RNXG2SJDQANROHANCNFSM4HHIT5FA> .

[ijlyttle]

Let me see how far I can get by Oct. 31 - despite our present lack of complete clarity on JWT, I think a lot of other things around authentication are also coming into focus and I think (hope) we are making progress on the documentation.

Give or take a helper-function or two, I think we have all the functions we need for CRAN .

[nathancday]

I like the idea of the using October 31st as a deadline. And thanks for continuing to refine the docs. Let me know when you'd like a detailed review.

Update about adding service accounts as collaborators, I haven't heard anything direct back, but I did find this post about how to retrieve the email-account that gets assigned to the service account https://community.box.com/t5/Platform-and-Development-Forum/How-to-add-service-account-as-collaborator-on-user-folder/td-p/61445 and I will try to figure out a way to get this information/ability into R code.

[ijlyttle]

Thanks for "pushing" me towards the deadline - it was the right thing to do.

This is only suspicion on my end, but I wonder if this is going to be a process and documentation issue (aren't they all...). If we push the idea of service apps each being limited to one folder, perhaps the challenge is how to manage all of these tokens.

Please feel free either to use or ignore this idea, but what if we designated a folder in the home directory, ~/.boxr-jwt, for example. That folder could look something like:

.boxr-jwt/
  foo.json
  bar.json
  ...

We could provide a helper function, such that jwt_token("foo") would return the path to ~.boxr-jwt/foo.json. We still need to figure out issues around the user_id, i.e. an easy way to get for the service account, and how to manage for interactive testing. I'll have a think on that.

Update - it's not clear to me if, for a Box instance, exactly one service account or if a service account is created for each service-app. If it is just one, then we could imagine a bit of code that could retrieve the user_id of the service account, then perhaps use that as the default...

Update 2 - now my head is spinning... to get the user_id of the service account, we would need to be authenticated, which is the entire point, so it would be a bad idea to use that function call as the default...

[nathancday]

I don't think we want to have a service app per folder, because (like you pointed out) managing all of those high-value apps and config files becomes a secondary problem.

To me the workflow of inviting the service account as a collaborator is most promising, because it would mitigate the existing JWT risks in two ways:

1. No more ability of assuming any user's identity (service account interacts with docs as itself)
2. Effectively quarantine the app to access only certain existing documents

I think this idea requires more time to flesh out, and definitely another function to wrap the "adding of the service account as a new collaborator" API code. This was my primary motivation for de-coupling full service app support/new-workflow from next CRAN push.

My understanding is that each service-app does get its own service account.

[ijlyttle]

Making sense to me now...

[ijlyttle]

Hi @nathancday - from my perspective, this is ready to be merged. I'd like to polish the documentation, but I plan to do that in a separate PR (to master).

[nathancday]

Just got done putting the new functions box_auth_service and box_dir_invite` through the paces and it seems like this new restrictive app + collaboration workflow will be the golden goose! Only thing that I'm minutely hung up on is the referencing of the key/config file as a token file, but that is secondary to this PR.

This has been by far the longest PR, I've ever been involved with. Thanks for again for all of your consideration and alternate strategy design I think will help a lot of people. With much glee...'Squashed and merged'!!!