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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

use_github does not push with https protocol and GITHUB_PAT #320

Closed
cderv opened this Issue Apr 2, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@cderv
Contributor

cderv commented Apr 2, 2018

I tried to create a 馃摝 with usethis calling at some point use_github.

First my setup is:

  • Windows
  • GITHUB_PAT environment variable is written in .Renviron
  • I get correct results from gh::gh_whoami.

Howerver, when using https protocol, use_github does not manage to push to the created repo because of a mishandling of credential with git2r.

Here how to reproduce the issue

testdir <- tempfile("testdir")
dir.create(testdir)
setwd(testdir)
library(usethis)
#> Warning: le package 'usethis' a 茅t茅 compil茅 avec la version R 3.4.4
create_package("testpkg", open = FALSE)
#> Changing active project to testpkg
#> <U+2714> Creating 'R/'
#> <U+2714> Creating 'man/'
#> <U+2714> Writing 'DESCRIPTION'
#> <U+2714> Writing 'NAMESPACE'
setwd(file.path(testdir, "testpkg/"))
use_git()
#> <U+2714> Initialising Git repo
#> <U+2714> Adding '.Rhistory', '.RData', '.Rproj.user' to './.gitignore'
#> <U+2714> Adding files and committing
use_github(protocol = "https")
#> <U+2714> Setting title and description
#>   Name:        testpkg
#>   Description: What the Package Does (One Line, Title Case)
#> <U+2714> Creating GitHub repository
#> <U+2714> Adding GitHub remote
#> <U+2714> Adding GitHub links to DESCRIPTION
#> <U+2714> Setting URL field in DESCRIPTION to 'https://github.com/cderv/testpkg'
#> <U+2714> Setting BugReports field in DESCRIPTION to 'https://github.com/cderv/testpkg/issues'
#> <U+2714> Pushing to GitHub and setting remote tracking branch
#> Error in (function (classes, fdef, mtable) : unable to find an inherited method for function 'cred_user_pass' for signature '"character", "NULL"'

the last step failed. The repository was created but use_github was not able to push to the repository. It seems it can鈥檛 deal with current git2r credential.

usethis/R/github.R

Lines 142 to 147 in 7d1c255

} else { ## protocol == "https"
## in https case, when GITHUB_PAT is passed as password,
## the username is immaterial, but git2r doesn't know that
cred <- git2r::cred_user_pass("EMAIL", auth_token)
git2r::push(r, "origin", "refs/heads/master", credentials = cred)
}

The credentials used are built with cred <- git2r::cred_user_pass("EMAIL", auth_token).
However, here auth_token = NULL by default and git2r does not know how to handle that.
auth_token = NULL by default indicates in the documentation that it should use the github PAT.
I am really not sure to understand why the use of cred_user_pass with "EMAIL" as username and an auth_token as password. I will try to investigate further the reason of this choice, but if you have an explanation, please tell me.

As it seems strange to me I tried another way.
First, git2r as specific credential object for token authentification, that by default get the GITHUB_PAT environment variable. Using this works.

r <- git2r::repository(file.path(testdir, "testpkg/"))
cred <- git2r::cred_token()
git2r::push(r, "origin", "refs/heads/master", credentials = cred)

There is also a way to use user/pass auth with the token using the username (or organization) and token as password.

cred2 <- git2r::cred_user_pass("cderv", Sys.getenv("GITHUB_PAT"))
# nothing to push but no error
git2r::push(r, "origin", "refs/heads/master", credentials = cred)

# cleaning this reprex
unlink(testdir, recursive = TRUE)

# delete manually the repo on github

A the end, do you think the choice in use_github is correct ? Should it be change to another credential object ? What are you thinking about all this ?

I am wiling to help with a PR when ready. Thanks.

@batpigandme

This comment has been minimized.

Show comment
Hide comment
@batpigandme

batpigandme Apr 2, 2018

Member

Is this different from the push problem with git2r from #300?
(i.e. does the git2r push work directly contrary to Jenny's expectation here: #300 (comment)?)

Member

batpigandme commented Apr 2, 2018

Is this different from the push problem with git2r from #300?
(i.e. does the git2r push work directly contrary to Jenny's expectation here: #300 (comment)?)

@jennybc

This comment has been minimized.

Show comment
Hide comment
@jennybc

jennybc Apr 2, 2018

Member

When this function was initially created, back inside devtools, the auth_token was initialized to the PAT, not NULL.

https://github.com/r-lib/usethis/blob/d5d20b12602be96979fa4dd15bc604066b09efdb/R/infrastructure-git.R

Then here, its default was changed to NULL:

0a20bba

But I don't see any arrangement made to fetch the GitHub PAT for git2r's use. gh will retrieve it automatically, yes, but that isn't going to help us in cred <- git2r::cred_user_pass("EMAIL", auth_token). So I think this is the mistake. If we are going to push with https, we need to explicitly retrieve the GitHub PAT if auth_token is still NULL.

A PR to fix this would be welcome @cderv.

I am really not sure to understand why the use of cred_user_pass with "EMAIL" as username and an auth_token as password. I will try to investigate further the reason of this choice, but if you have an explanation, please tell me.

This is how people with 2FA turned on can still interact with GitHub over https. This is a deliberate choice. I push this way at this point because I know I have a PAT, since we needed to create a repo.

Member

jennybc commented Apr 2, 2018

When this function was initially created, back inside devtools, the auth_token was initialized to the PAT, not NULL.

https://github.com/r-lib/usethis/blob/d5d20b12602be96979fa4dd15bc604066b09efdb/R/infrastructure-git.R

Then here, its default was changed to NULL:

0a20bba

But I don't see any arrangement made to fetch the GitHub PAT for git2r's use. gh will retrieve it automatically, yes, but that isn't going to help us in cred <- git2r::cred_user_pass("EMAIL", auth_token). So I think this is the mistake. If we are going to push with https, we need to explicitly retrieve the GitHub PAT if auth_token is still NULL.

A PR to fix this would be welcome @cderv.

I am really not sure to understand why the use of cred_user_pass with "EMAIL" as username and an auth_token as password. I will try to investigate further the reason of this choice, but if you have an explanation, please tell me.

This is how people with 2FA turned on can still interact with GitHub over https. This is a deliberate choice. I push this way at this point because I know I have a PAT, since we needed to create a repo.

@cderv

This comment has been minimized.

Show comment
Hide comment
@cderv

cderv Apr 2, 2018

Contributor

Thanks for the clarification. I better understand the choices and what is going on.
The links to previous code were useful. Thanks.

I understand the "EMAIL" username thanks to this comment

} else { ## protocol == "https"
## in https case, when GITHUB_PAT is passed as password,
## the username is immaterial, but git2r doesn't know that
## switch to git2r::cred_token() when CRAN version > v0.11.0
cred <- git2r::cred_user_pass("EMAIL", auth_token)

I see two solutions using GITHUB_PAT :

  • Using cred_token but in that case, a token can't be pass by an argument, only by an environment variable - that is not compatible with current choice of the auth_token argument.
  • Using cred_user_pass(<dummy_username>, auth_token) to use basic auth.

I will PR something based on the second one, unless we could consider relying only on environment variable and you find the first one better.

I'll do that quickly, don't worry.

@batpigandme Yes this is a bit different. I mange to push when using https and the correct way to pass credential. Here, this is just a small issue of checking auth_token == NULL and getting Sys.getenv("GITHUB_PAT") in that case as stated by the documentation. I did not check ssh behaviour - different issue.

Contributor

cderv commented Apr 2, 2018

Thanks for the clarification. I better understand the choices and what is going on.
The links to previous code were useful. Thanks.

I understand the "EMAIL" username thanks to this comment

} else { ## protocol == "https"
## in https case, when GITHUB_PAT is passed as password,
## the username is immaterial, but git2r doesn't know that
## switch to git2r::cred_token() when CRAN version > v0.11.0
cred <- git2r::cred_user_pass("EMAIL", auth_token)

I see two solutions using GITHUB_PAT :

  • Using cred_token but in that case, a token can't be pass by an argument, only by an environment variable - that is not compatible with current choice of the auth_token argument.
  • Using cred_user_pass(<dummy_username>, auth_token) to use basic auth.

I will PR something based on the second one, unless we could consider relying only on environment variable and you find the first one better.

I'll do that quickly, don't worry.

@batpigandme Yes this is a bit different. I mange to push when using https and the correct way to pass credential. Here, this is just a small issue of checking auth_token == NULL and getting Sys.getenv("GITHUB_PAT") in that case as stated by the documentation. I did not check ssh behaviour - different issue.

@jennybc

This comment has been minimized.

Show comment
Hide comment
@jennybc

jennybc Apr 2, 2018

Member

We obviously never circled back to this, even though git2r is now well past v0.11.0!

In theory, we could use git2r::cred_token() now. But git2r only considers GITHUB_PAT, whereas gh will also consider GITHUB_TOKEN. So that introduces yet another way for the gh operations to succeed and the git2r operations to mysteriously fail.

I have already brought a copy of gh::gh_token() into usethis (gh::gh_token() is exported in dev version, but not the CRAN version). I think you should use the usethis version of gh_token() to fetch auth_token inside this else{...} if auth_token is NULL. I.e. it should look like this:

cred <- git2r::cred_user_pass("EMAIL", auth_token %||% gh_token())

Here's the reference for what we're doing with the PAT and HTTPS:

https://help.github.com/articles/creating-a-personal-access-token-for-the-command-line/

If you make a PR, please include any manual tests you run. That would be a great start on #322, even if you don't finish it off, i.e. even if you just look at HTTPS scenarios.

Member

jennybc commented Apr 2, 2018

We obviously never circled back to this, even though git2r is now well past v0.11.0!

In theory, we could use git2r::cred_token() now. But git2r only considers GITHUB_PAT, whereas gh will also consider GITHUB_TOKEN. So that introduces yet another way for the gh operations to succeed and the git2r operations to mysteriously fail.

I have already brought a copy of gh::gh_token() into usethis (gh::gh_token() is exported in dev version, but not the CRAN version). I think you should use the usethis version of gh_token() to fetch auth_token inside this else{...} if auth_token is NULL. I.e. it should look like this:

cred <- git2r::cred_user_pass("EMAIL", auth_token %||% gh_token())

Here's the reference for what we're doing with the PAT and HTTPS:

https://help.github.com/articles/creating-a-personal-access-token-for-the-command-line/

If you make a PR, please include any manual tests you run. That would be a great start on #322, even if you don't finish it off, i.e. even if you just look at HTTPS scenarios.

@cderv

This comment has been minimized.

Show comment
Hide comment
@cderv

cderv Apr 3, 2018

Contributor

OK I see what to do. Thanks.

TODO:

  • I'll use %||% for getting the PAT if NULL auth_token
  • However, if env variables are not set, I'll check what git2r with results with auth_token = "" to see if this is enough.
  • I'll add a manual test file. Currently, I use the code I posted in the reprex from 1rst post above for HTTPS protocol.

I'll do that this week.

Contributor

cderv commented Apr 3, 2018

OK I see what to do. Thanks.

TODO:

  • I'll use %||% for getting the PAT if NULL auth_token
  • However, if env variables are not set, I'll check what git2r with results with auth_token = "" to see if this is enough.
  • I'll add a manual test file. Currently, I use the code I posted in the reprex from 1rst post above for HTTPS protocol.

I'll do that this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment