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

Add check for invalid GitHub PAT to internal function #909

Open
wpetry opened this issue Jan 23, 2024 · 2 comments
Open

Add check for invalid GitHub PAT to internal function #909

wpetry opened this issue Jan 23, 2024 · 2 comments
Labels
feature New feature or request

Comments

@wpetry
Copy link

wpetry commented Jan 23, 2024

Is your feature request related to a problem? Please describe.
This feature would add more graceful and informative error handling for cases when the user's GitHub PAT in .Renviron is invalid due to expiration, inadequate permissions, or typos/copy errors.

When an invalid PAT is provided in the user's .Renviron, it is not possible to install_cmdstan(). The function fails to retrieve the version list from GitHub, and will retry several times before stopping with an error. On the user side, it appears that GitHub is not responding, but the URL provided in the message loads fine on a browser. Like many other .Renviron problems, it's difficult to diagnose that is the cause of the failure. But seems worth addressing here because cmdstanr uses a PAT stored in .Renviron to retrieve GitHub credentials.

Describe the solution you'd like
Add a check to ensure that the user's PAT does not cause a '401 Unauthorized' HTTP status when supplied to GitHub. It's not clear to me the best way to do this. Currently, try_download() throws an error when an invalid PAT is supplied, but it suppresses the warning that contains the '401 Unauthorized' HTTP status code. Some ideas:

  1. Add a new internal function that checks for different causes of failure to communicate with the GitHub API. This would also help diagnose whether the failure is due to other common causes (e.g., no internet connectivity).
  2. Modify try_download() to capture warnings when an invalid PAT is supplied. download_with_retries() could then parse these warnings to avoid needless retries with a bad PAT.
  3. Retry without supplying a PAT.

Describe alternatives you've considered
None

Additional context
None

@wpetry wpetry added the feature New feature or request label Jan 23, 2024
@jgabry
Copy link
Member

jgabry commented Jan 24, 2024

Thanks for bringing this up!

Some ideas:

  1. Add a new internal function that checks for different causes of failure to communicate with the GitHub API. This would also help diagnose whether the failure is due to other common causes (e.g., no internet connectivity).
  2. Modify try_download() to capture warnings when an invalid PAT is supplied. download_with_retries() could then parse these warnings to avoid needless retries with a bad PAT.
  3. Retry without supplying a PAT.

These all seem like reasonable ideas. As a user, what do you think would be most helpful? Any interest in working on a PR for this?

@wpetry
Copy link
Author

wpetry commented Jan 24, 2024

Maybe a better design is to modify download_with_retries(). If the first try fails, then check whether the token is valid with a simple API call that captures the HTTP status code. The idea would be similar to the implementation in the credentials package, but ideally without adding a dependency on curl. I'd welcome ideas on base R function we can use to call the GitHub API that might be more elegant than download.file() on the release list.
https://github.com/r-lib/credentials/blob/883e07af9c49ae66cc100b262e736acb5f5c2cf7/R/github-pat.R#L46C5-L53C10

Users with a valid token or no token won't see a performance difference. Only those who have some kind of problem (bad token, no internet connection, etc.) will be slowed by the check.

I don't have the bandwidth at the moment to work on a PR. I can check back again once things clear up a bit. If the issue is still open, I can take a crack at it.

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

No branches or pull requests

2 participants