Add a whoami function (user, token, scopes); closes #39 #51

jennybc · 2016-12-08T10:16:56Z

Are you willing to have such a function here? I think it would be handy for new users and troubleshooting. But I could put it in githug instead, if you'd prefer not.

If you are receptive, I will flesh out the "HERE'S HOW TO GET A TOKEN AND WHERE TO STICK IT" message and add a test. And do anything else you suggest.

codecov-io · 2016-12-08T10:21:18Z

Current coverage is 67.87% (diff: 96.00%)

Merging #51 into master will increase coverage by 2.95%

@@             master        #51   diff @@
==========================================
  Files             7          8     +1   
  Lines           228        249    +21   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            148        169    +21   
  Misses           80         80          
  Partials          0          0

Powered by Codecov. Last update 23308b6...3b00a4b

gaborcsardi · 2016-12-08T10:40:59Z

Hmmm. I can see that this is very useful, so it is tempting.

I am also not sure if gh is the right place for it. Why do you think it is better placed here than in githug?

TBH, I still haven't given up the hope of having a proper higher level GH API package, and I think gh_whoami could go there. But we don't have that package right now, and I won't write it anytime soon, so if you think it is better placed here than in githug, I'll be happy to merge it. :)

jennybc · 2016-12-08T16:34:47Z

Why do you think it is better placed here than in githug?

Short-term, this is much closer to going to CRAN! Even long-term, gh should be the least common denominator for GitHub API work from R. I'd recommend anyone doing this in a script or package use gh. So it seems like the right place for this sort of diagnostic.

I will finish off the PR today and you can render your final judgment 🙏.

gaborcsardi · 2016-12-08T16:46:18Z

I see gh as the basic machinery of a GH API, and gh_whoami does not qualify as basic to me. :)

But it will be fine here, for now. (Which, as we know, likely means forever. :)

jennybc · 2016-12-08T16:59:27Z

I also just realized this ~~sort of gets at~~ addresses #39:

I'm mostly imagining this would be a place to hang the documentation for how to set up your GITHUB_PAT env var, and to give users a way to check that they're correctly configured.

jennybc · 2016-12-08T19:45:50Z

My roxygen seems different from your roxygen.

Testing: shall I assume nothing, i.e. no token necessarily available? Or take advantage of whatever you've rigged up with httrmock (which I have not yet digested)?

jennybc · 2016-12-08T19:47:42Z

BTW I have used successfully with GHE.

jennybc · 2016-12-09T06:44:20Z

From another conversation:

I can create a token for the gh-testing user and share that with you using hadley/secure, then you can actually run the tests. I'll try to get to it today evening.

I basically see how this works. But yes I need that token. I played around with my own to get this far. I had some very puzzling times until I discovered httrmock::mocking_status()! I clearly don't understand the httrmock workflow.

I obviously won't leave the test like this, with my own info in there and skipping on travis and appveyor.

gaborcsardi · 2016-12-09T09:12:48Z

Yes, sorry, I realized that hadley/secure is not going to be a long term solution, because it is not going to CRAN, so I started to look for other options. But for now I could just share the token with you. Based on this: https://gist.github.com/jeroenooms/1ffbfbdad40f4aad6657c337a4924f0e I used this code:

github_pubkey <- function(user){
  url <- sprintf("https://api.github.com/users/%s/keys", user)
  keys <- jsonlite::fromJSON(url)
  lapply(keys$key, openssl::read_pubkey)
}

jenny <- github_pubkey('jennybc')
pubkey <- jenny[[1]] # in case there are multiple keys

token <- readLines("~/works/r-pkgs/gh/tests/testthat/github-token.txt")
buf <- openssl::rsa_encrypt(charToRaw(token), pubkey)
cat(openssl::base64_encode(buf))

And here is the encrypted token:

MLDsFiOAVtdbbORHOCvfbTDMW6k1eQGPu7PfIXqzDe4SagO49eMAsv0f1g03eGORS+QU53MllUDPmYzlYczUVIdnNPjK1yygqS4NJDJNsWsbsDU1I19+KIFDqQ2lkOqq1/3fQeyvtmr7B4FHAbPMAtFLz/OFdbCPbcHurNCtIs9lck3YXp8IZBO9IaKzEEA1M+i+bnWBXxTbU8E0E1PDGPFXTAY34/GLsm7Th2Nz9OZ+EkZC7tLMHFtxuUYd8qi7lOZ6IxMYPs7b6t6QQ0l0QprHOUQWEVe7Ra/2m9Q5OWpvzoHNDkXFBwdSiNVQb0UhUpOfHL3M4yTjaZ/tYIZ3ZgE4nhKsogGLqhdPhuTXsFxbIqH1AxS+1ugOKcZ+f9VDdN2m/tdmjooxUITJI5+bNBSV9iawSaikD6nLPydqLIdH3a3UJ6akcMbBcjw10jvC67Q8JJhHiJVXjcIHKqwPfnVkwpPlAp6Lnn1Y3NNuzQsnIQJeRjQStd8kTxKNBake3X077d4PF7VloBghu396Waudjqmjy5D/VIIs+tvRlMkGDnf45r/MwjFPENIz9vXesNxSvmm7QdBULRJKcbrM62/BgVntEDUy2sF7nTFCaLnzbpRmz2H+msAAkugSyem3GGrkSScWz1iAGIQRrXFvM5AXQJGirVsM2XZdAu+c6ig=

You can just decrypt it with your private key.

gaborcsardi · 2016-12-09T09:26:00Z

CC-ing @jimhester, who might be interested in doing this kind of mocking for devtools test cases, for GH queries and even downloads.

As for the httrmock workflow, I am also just working it out, so it is quite experimental. Right now we have this:

if (file.exists("github-token.txt")) {
  Sys.setenv(GITHUB_TOKEN = readLines("github-token.txt", n = 1))
  Sys.setenv(DEBUGME = "httrmock")
  httrmock::start_recording()
  httrmock::start_replaying()

} else {
  httrmock::stop_recording()
  httrmock::start_replaying()
}

which basically means, reading from the back, that if you don't have a token, then you just want to replay the recorded responses. This is how the tests run on CRAN. Everything that was recorded before is just replayed (start_replaying) and requests that were not recorded before are performed, but not recorded (stop_recording).

If you do have a token, then, 1) everything that was recorded before is replayed (start_replaying, so that a test run does not perform all GH requests), and 2) everything that was not recorded before is performed and then recorded (start_recording).

The idea is that most of the time you don't want to perform all GH requests while developing gh, only the new ones. While developing, you might want to change the first block, actually. E.g. to re-record everything, you would do start_recording and stop_replaying. Or, while adding some features or new API points, or tests for them, you might want stop_recording and start_replaying, so that all new test requests are performed, but the old ones are not.

Does it make sense? I realize that it is a bit messy, and it would be great to improve it. E.g. we could have some interactive mode chooser, that you could use to select the desired behavior for the current session. I would even show it in my R prompt.

jimhester · 2016-12-09T14:42:40Z

I think one additional thing we need to make this nice is the ability to run a teardown file(s) in testthat / devtools. Because ideally you want recording and replaying on when you are running the tests, but off when you are developing. Also r-lib/devtools#1202, r-lib/devtools#1169 would be useful for the same reasons.

Also in lookup I had planned on writing a function with instructions on setting up a Github Token, as that package won't work well without one. I briefly looked into using https://developer.github.com/v3/oauth_authorizations/#create-a-new-authorization to request a token from the user automatically based on basic authentication, but I am not quite clear how the flow works with two factor authentication. Something like that might be useful for GitHug or here as well.

gaborcsardi · 2016-12-09T14:46:53Z

I think one additional thing we need to make this nice is the ability to run a teardown file(s) in testthat / devtools.

Exactly. The code I cited is in "setup", i.e. helper.R, but there is currently no teardown in testthat.....

Something like that might be useful for GitHug or here as well.

Maybe in githug. :) I think gh should only contain the basic infrastructure, ideally.

jennybc · 2016-12-09T16:49:51Z

Success with the token! Thanks.

ideally you want recording and replaying on when you are running the tests, but off when you are developing

Right now helper.R is run every time you load_all(), so it's easy to get into a recording/replaying state without really thinking about it. This is how I puzzled myself because I inadvertently recorded some >400s, tinkering with gh_whoami() and nonexistent/bad tokens. Then of course they got replayed until I figured out what I had done. I am wondering if the condition for changing the recording/replaying state should be more than just "github-token.txt exists".

Also in lookup I had planned on writing a function with instructions on setting up a Github Token, as that package won't work well without one. I briefly looked into using https://developer.github.com/v3/oauth_authorizations/#create-a-new-authorization to request a token from the user automatically based on basic authentication, but I am not quite clear how the flow works with two factor authentication. Something like that might be useful for GitHug or here as well.

Yes I would also like to help people store a PAT somehow, from one of these packages. However, given the way the Authorizations API works, it almost feels like we could create as much friction as we remove by using it. In Happy Git, I have instructions to obtain PAT in the browser and offer this code snippet to help store it:

cat("GITHUB_PAT=8c70...adf2\n",
    file = file.path(normalizePath("~/"), ".Renviron"), append = TRUE)

I know that's very low tech 😔.

gaborcsardi · 2016-12-09T16:53:05Z

Right now helper.R is run every time you load_all(), so it's easy to get into a recording/replaying state without really thinking about it. This is how I puzzled myself because I inadvertently recorded some >400s, tinkering with gh_whoami() and nonexistent/bad tokens. Then of course they got replayed until I figured out what I had done. I am wondering if the condition for changing the recording/replaying state should be more than just "github-token.txt exists".

Agreed. How about controlling the behavior via environment variables? Then we could have a default behavior, for the user, and developers could change it via a simple command that would just get/set an environment variable?

jennybc · 2016-12-09T19:35:59Z

tests/testthat/test-whoami.R

+})
+
+test_that("whoami works in absence of PAT", {
+  expect_message(res <- gh_whoami(.token = ""),


@jimhester Re: #50. I know it's regarded as good practice to include a specific message inside expect_error() and friends. Do you think the HTTP version of this mindset is that one should write expectations in this context for a specific HTTP status? I'm trying to understand if your motivation for #50 is something you want to do in lookup or for testing or ....

I think would be great if expect_error retained the error object, and we could test the error class. See r-lib/testthat#530

Well I was using it in lookup https://github.com/jimhester/lookup/blob/e841d72819e39242e0987aa6f23b240c9d47d60c/R/rcpp.R#L3.

But I think if you are expecting a response to have a specific error class you should catch just that specific class and let any other error be signaled normally.

I am now catching and expecting a specific error class and HTTP error.

jennybc · 2016-12-09T19:36:46Z

tests/testthat/test-whoami.R

+})
+
+test_that("whoami errors with bad PAT", {
+  expect_error(res <- gh_whoami(.token = NA), "Requires authentication")


@jimhester More examples of "should I expect a certain status?" instead of doing this.

Yes here I would definitely check the error class rather than the message, the message could potentially be changed by GitHub any time, the HTTP error code should be more stable.

I am now catching and expecting a specific error class and HTTP error.

jennybc · 2016-12-09T19:59:07Z

How about controlling the behavior via environment variables?

That sounds great! Does the environment variable R_TESTS help us at all? It will be non-NULL when the tests are running via devtools::test().

gaborcsardi · 2016-12-09T20:02:48Z

Yes, R_TESTS could indeed help, but I am a bit reluctant to use it, as it causes trouble for many packages, and some people (including myself) often unset it.

jimhester · 2016-12-09T20:12:33Z

Re: environment variables @lionel- has a PR that should help some r-lib/devtools#1391, but it is not yet merged.

This allows you to catch a github_error or the exact status code directly.

Note: there's only one new recording because the second test matches against first recording. It just happens to pass in a happy coincidence.

gaborcsardi · 2016-12-09T23:45:37Z

Btw. if you skip() the tests, then I can merge this, and you don't have to wait for the httrmock updates.

jennybc · 2016-12-09T23:53:35Z

I have skipped the one that I know must fail for now. The first one should not. Seeing if setting GITHUB_TOKEN to something other than NULL fixes that.

jennybc · 2016-12-10T00:00:13Z

VICTORY. I had to set GITHUB_TOKEN to some value on Travis and Appveyor. I hope that's OK. It should never be actually used and, if it is, I guess you'd want to know and fail anyway?

gaborcsardi · 2016-12-10T00:04:19Z

Yeah, I think that is fine.

gaborcsardi · 2016-12-10T00:06:57Z

R/gh_request.R

-  if (token != "") auth <- c("Authorization" = paste("token", token))
+  if (isTRUE(token != "")) {
+    auth <- c("Authorization" = paste("token", token))
+  }


Sorry, can you write this with if () { ... } else { ... }? I think it shows the intent better.

gaborcsardi · 2016-12-10T00:08:59Z

R/gh_response.R

@@ -21,7 +21,7 @@ gh_process_response <- function(response) {
      headers = heads,
      message = paste0("GitHub API error (", status_code(response), "): ",
                       heads$`status`, "\n  ", res$message, "\n")
-    ), class = c("condition", "error"))
+    ), class = c("github_error", paste0("http_error_", status_code(response)), "error", "condition"))


This was already added by Jim's PR, no? Can you please rebase, if it is easy? If it is not easy, then don't worry about it, it is not worth getting into git trouble for this.....

I merged master into my branch in the middle of this adventure (my branch started before I pulled the commit from master with that PR). This will work out when you squash, yes?

Yes, right, should be fine I think.

gaborcsardi · 2016-12-10T00:12:19Z

R/gh_whoami.R

+#' your planned tasks. The \code{repo} scope, for example, is one many are
+#' likely to need. The token itself is a string of 40 letters and digits. You
+#' can store it any way you like and provide explicitly via the \code{.token}
+#' argument to \code{\link{gh}()}.


markdown roxygen parsing is on for this project, so feel free to write markdown if you want to. You would need the dev version of roxygen2, though, so if you don't want to deal with that, that's fine, too.

I haven't done this here. Maybe after the merge I could do it for all exported functions at once?

You don't have to do it, just mentioned it for next time. :) I don't think rewriting it it worth the effort.

gaborcsardi · 2016-12-10T00:16:43Z

R/gh_whoami.R

+#'
+#' Reports wallet name, GitHub login, and GitHub URL for the current
+#' authenticated user, the first few and last characters of the token, and the
+#' associated scopes.


Hmmm. I am not a security expert, but I would not print out anything from the token. I know that the user has access to it, but I just don't want it to appear in printouts, Rmds, etc.

Let me think about this a bit. I see that it can be useful for beginners, but I am still concerned.

I thought showing first 4 and last 4 characters was OK. I do the same over in googlesheets. It is helpful when you have more than one token in your life and there's no obvious way to give them names. But we can kill it or reveal even less of it.

I don't know, TBH. Showing 8 characters makes the token 2^(8*4) times less secure. We would show 32 bits of the 160.

On the other hand, this is not sg you can "crack", an attacker would need to try each remaining valid key in an API request....

How about we only show the first two letters now? Is that still useful for the user?

I'll also ask my security consultant, @jeroenooms. :)

OK 2 characters it is.

gaborcsardi · 2016-12-10T00:16:49Z

R/gh_whoami.R

+#' Put a line break at the end! If you’re using an editor that shows line
+#' numbers, there should be (at least) two lines, where the second one is empty.
+#' Restart R for this to take effect. Call \code{gh_whoami()} to confirm
+#' success.


Wow! You really have a lot of empathy for beginners. :)

I may have answered this question a few dozen times.

gaborcsardi · 2016-12-10T00:17:34Z

R/gh_whoami.R

+            "For more on what to do with the PAT, see ?gh_whoami.")
+    return(invisible(NULL))
+  }
+  req <- gh_build_request(endpoint = "/user", token = .token,


I guess if we do not show the token, then you don't need to build a gh_request manually, right?

You're right. I can avoid all the manual stuff and simply use gh(). Have simplified that.

gaborcsardi · 2016-12-10T00:19:05Z

Thanks much, looks awesome! I added some small comments.

Most important is about showing part of the token on the screen. I am not sure if that's a good idea to be honest, but I might be paranoid.

gaborcsardi · 2016-12-10T01:23:05Z

OK, I will merge this now, and maybe I'll change the token printout slightly.

gaborcsardi · 2016-12-10T01:23:25Z

Thanks much!

gaborcsardi · 2016-12-10T16:09:42Z

@jennybc FYI, I added a simple version of httrmock contexts.

jennybc · 2016-12-10T16:40:31Z

Great! I will pursue that. I might be able to resume the routes.json activity this week for automating tests and I'm sure this will come up.

gaborcsardi · 2016-12-10T16:46:50Z

Great! I'll still add better request matching. E.g. for POST we should at least use the data....

jennybc · 2016-12-10T16:55:36Z

I will start with GETs anyway, as I'm sure that will be highly educational.

jennybc · 2016-12-10T16:56:14Z

This PR was a good warm-up because I understand the basic mojo of httrmock now but did not before.

Add a whoami function (user, token, scopes)

45114fa

jennybc added 5 commits December 8, 2016 11:32

documentation

decdec5

unrelated typos

02c7998

roxygenize

3d337d5

better pointer to help

029b20f

link to gh_whoami() help from gh help

d1535a9

jennybc changed the title ~~Add a whoami function (user, token, scopes)~~ Add a whoami function (user, token, scopes); closes #39 Dec 8, 2016

test gh_whoami()

c801f3d

jennybc commented Dec 9, 2016

View reviewed changes

jennybc and others added 3 commits December 9, 2016 13:36

Announce mocking status when recording

6d74510

Add additional classes to returned error objects (#50)

acf9cdf

This allows you to catch a github_error or the exact status code directly.

Record tests (sort of, see below) in test-error.R

b50c502

Note: there's only one new recording because the second test matches against first recording. It just happens to pass in a happy coincidence.

Improve and record tests of gh_whoami()

dcc7513

Make GITHUB_TOKEN be non-NULL on travis and appveyor

27733c4

Don't run any example re: gh_whoami()

4656545

gaborcsardi reviewed Dec 10, 2016

View reviewed changes

jennybc added 2 commits December 9, 2016 16:51

Use if() {} else {} when forming Authorization header

c9a6bf3

Use gh() instead of low-level fxns in gh_whoami()

3b00a4b

gaborcsardi merged commit 951031e into r-lib:master Dec 10, 2016

jennybc mentioned this pull request Dec 12, 2016

Development workflow re: recording and replaying r-lib/httrmock#5

Closed

Add a whoami function (user, token, scopes); closes #39 #51

Add a whoami function (user, token, scopes); closes #39 #51

Conversation

jennybc commented Dec 8, 2016

codecov-io commented Dec 8, 2016 • edited Loading

Current coverage is 67.87% (diff: 96.00%)

gaborcsardi commented Dec 8, 2016

jennybc commented Dec 8, 2016

gaborcsardi commented Dec 8, 2016

jennybc commented Dec 8, 2016 • edited Loading

jennybc commented Dec 8, 2016

jennybc commented Dec 8, 2016

jennybc commented Dec 9, 2016

gaborcsardi commented Dec 9, 2016

gaborcsardi commented Dec 9, 2016

jimhester commented Dec 9, 2016

gaborcsardi commented Dec 9, 2016

jennybc commented Dec 9, 2016

gaborcsardi commented Dec 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybc commented Dec 9, 2016

gaborcsardi commented Dec 9, 2016

jimhester commented Dec 9, 2016

gaborcsardi commented Dec 9, 2016

jennybc commented Dec 9, 2016

jennybc commented Dec 10, 2016

gaborcsardi commented Dec 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborcsardi Dec 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborcsardi commented Dec 10, 2016

gaborcsardi commented Dec 10, 2016

gaborcsardi commented Dec 10, 2016

gaborcsardi commented Dec 10, 2016

jennybc commented Dec 10, 2016

gaborcsardi commented Dec 10, 2016

jennybc commented Dec 10, 2016

jennybc commented Dec 10, 2016

codecov-io commented Dec 8, 2016 •

edited

Loading

jennybc commented Dec 8, 2016 •

edited

Loading

gaborcsardi Dec 10, 2016 •

edited

Loading