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

Send auth information in the headers #391

Merged
merged 7 commits into from
Jun 24, 2019
Merged

Conversation

gaborcsardi
Copy link
Member

Instead of sending it in query parameters.
This way it is not printed in the logs on errors.

Also refactored the download code a bit. We still respect
the user's download.file.method option.

We have tests now for the various download methods.

Closes #364.

Instead of sending it in query parameters.
This way it is not printed in the logs on errors.

Also refactored the download code a bit. We still respect
the user's `download.file.method` option.

We have tests now for the various download methods.

Closes #364.
We need to unset the `download.file.method`
while running the tests, in case the user
running the tests has a default set.
@gaborcsardi
Copy link
Member Author

@jimhester do not merge yet pls, tests on Windows are failing....

@gaborcsardi
Copy link
Member Author

OK, there is a slight problem. We cannot send headers with the wininet method on R versions older than R 3.6.0. Apparently the user agent trick does not work there.

@gaborcsardi
Copy link
Member Author

Since 3.2.x the Windows builds have libcurl, so this is not a major issue. I will add a warning if the user selected "wininet" on an older version of R and headers are needed.

Need to double quote, single quote is
not special on Windows.
@gaborcsardi
Copy link
Member Author

Actually, we always require the curl package if R is older than 3.2.0, so this hardly ever comes up. Still, I'll add a warning for this rare case.

Older R versions cannot send headers with
the wininet method.
@gaborcsardi
Copy link
Member Author

OK, I think this is ready. The download code is pretty clean now.

@jimhester
Copy link
Member

Cool, looks good, thanks for working on this!

[ci skip]
@jimhester jimhester merged commit 0c980e2 into master Jun 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send GH tokens in headers, instead of query parameters
2 participants