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

Fetch commits from github private repos using Authorization header #3675

Merged
merged 2 commits into from Sep 29, 2020

Conversation

@imalsogreg
Copy link
Contributor

@imalsogreg imalsogreg commented Jun 9, 2020

nix flake info calls a github API that requires authorization when the repository in question is private. Currently this request fails with a 404.

This commit adds a headers parameter to the FileTransferRequest, and the github fetchers now call it with Authorization: token <github-access-token>.

This differs from the current convention in nix of passing the access token as a query parameter, because query parameter tokens are deprecated and will be disallowed in November 2020. Using them today triggers a warning email.

This PR grew to include the work done in #3688. I've now tested it with private github and gitlab repositories.

[greghale@cedric:~/code/nix]$ result/bin/nix --option gitlab-access-token $GITLAB_TOKEN flake info gitlab:secret_org/secret_repo/tmp
warning: flake 'gitlab:secret_org/secret_repo/622bee9dc5f8723fa3e8aaaf4359946277e441e8' has deprecated attribute 'edition'
Resolved URL:  gitlab:secret_org/secret_repo/tmp
Locked URL:    gitlab:secret_org/secret_repo/622bee9dc5f8723fa3e8aaaf4359946277e441e8
Description:   Secret repo's description
Path:          /nix/store/11z2mfkba3kfkcb18l6aab4k1gk9b6h5-source
Revision:      622bee9dc5f8723fa3e8aaaf4359946277e441e8
Last modified: 2020-06-10 22:20:05

[greghale@cedric:~/code/nix]$ result/bin/nix --option github-access-token $GITHUB_TOKEN flake info github:secret_org/secret_repo
warning: flake 'github:secret_ord/secret_repo/622bee9dc5f8723fa3e8aaaf4359946277e441e8' has deprecated attribute 'edition'
Resolved URL:  github:secret_org/secret_repo
Locked URL:    github:secret_org/secret_repo/622bee9dc5f8723fa3e8aaaf4359946277e441e8
Description:   Secret repo's description
Path:          /nix/store/aaa111kba4kfkcb18l6i234k1gk9b6h5-source
Revision:      622bee9dc5f8723fa3e8aaaf4359946277e441e8
Last modified: 2020-06-10 22:20:05
@imalsogreg imalsogreg force-pushed the imalsogreg:github-api-token branch from abf999c to 83f4948 Jun 9, 2020
@imalsogreg
Copy link
Contributor Author

@imalsogreg imalsogreg commented Jun 10, 2020

@Ericson2314 I only changed the one API endpoint that didn't yet use an API token (thus resulting in a 404). Think I should update the other github fetchers with the same logic, or keep the scope of this PR small (fixing the 404)?

src/libfetchers/fetchers.hh Outdated Show resolved Hide resolved
src/libfetchers/github.cc Outdated Show resolved Hide resolved
src/libfetchers/tarball.cc Outdated Show resolved Hide resolved
src/libstore/filetransfer.cc Outdated Show resolved Hide resolved
@imalsogreg imalsogreg force-pushed the imalsogreg:github-api-token branch 3 times, most recently from 83d1b6c to a24c725 Jun 10, 2020
src/libfetchers/github.cc Outdated Show resolved Hide resolved
@imalsogreg imalsogreg force-pushed the imalsogreg:github-api-token branch from 52affed to 6173acd Jun 11, 2020
@Kloenk
Copy link
Member

@Kloenk Kloenk commented Jun 13, 2020

Just saw this. I did a similar approach for gitlab. but used a map instead of a vector.
See #3688

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jun 13, 2020

"Keys" can be repeated in headers, so a vector is more appropriate. But good to see this is being worked on for gitlab and github :).

@Kloenk
Copy link
Member

@Kloenk Kloenk commented Jun 13, 2020

@imalsogreg

"Keys" can be repeated in headers, so a vector is more appropriate. But good to see this is being worked on for gitlab and github :).

That's a good argument. Will you implement my gitlab headers? Then I would close my pr.

@imalsogreg
Copy link
Contributor Author

@imalsogreg imalsogreg commented Jun 13, 2020

@Kloenk Sure! I'll take a stab at it now.

@Kloenk Kloenk mentioned this pull request Jun 13, 2020
@imalsogreg imalsogreg force-pushed the imalsogreg:github-api-token branch from 33af97c to ed25d83 Jun 13, 2020
@Kloenk
Copy link
Member

@Kloenk Kloenk commented Jun 15, 2020

Looks got to meet so far.

Maybe squash the commits? I think 8 is kinda a lot for this small change.

@imalsogreg imalsogreg force-pushed the imalsogreg:github-api-token branch from e6f6fa9 to ff01e40 Jun 16, 2020
@imalsogreg
Copy link
Contributor Author

@imalsogreg imalsogreg commented Jun 16, 2020

@Kloenk squashed down to 1, good point :) I'm ready for a merge when it's ready, and am happy to address any other feedback.

@Kloenk
Kloenk approved these changes Jun 16, 2020
Copy link
Member

@Kloenk Kloenk left a comment

LGTM

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jun 17, 2020

I wouldn't bother squashing things until it passes CI, unless one is find rebasing and force pushing every single time.

@Kloenk
Copy link
Member

@Kloenk Kloenk commented Jun 17, 2020

@Ericson2314 the checks are failing, because flake-combat is not updated yet, and the new flake.lock uses version 6 (which is not supported yet).

So ci is not working on the flake branch at the current moment.

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jun 17, 2020

Ah right, forgot this was to that branch.

@cole-h
Copy link
Member

@cole-h cole-h commented Jun 17, 2020

FYI, flake-compat just updated to support lockfile versions 6 and 7 -- if you re-trigger CI, it should turn green (hopefully)!

@imalsogreg imalsogreg force-pushed the imalsogreg:github-api-token branch from ff01e40 to dd3a1af Jun 17, 2020
@imalsogreg imalsogreg changed the base branch from flakes to master Aug 11, 2020
@imalsogreg imalsogreg force-pushed the imalsogreg:github-api-token branch from dd3a1af to e4856e2 Aug 11, 2020
@imalsogreg
Copy link
Contributor Author

@imalsogreg imalsogreg commented Aug 11, 2020

@edolstra or @Ericson2314 , would either of you mind giving this another look and/or a merge? I've brought it up to date with master.

Thanks for the flakes blog posts! They're super helpful.

`nix flake info` calls the github 'commits' API, which requires
authorization when the repository is private. Currently this request
fails with a 404.

This commit adds an authorization header when calling the 'commits' API.
It also changes the way that the 'tarball' API authenticates, moving the
user's token from a query parameter into the Authorization header.

The query parameter method is recently deprecated and will be disallowed
in November 2020. Using them today triggers a warning email.
@imalsogreg imalsogreg force-pushed the imalsogreg:github-api-token branch from e4856e2 to a303c0b Sep 16, 2020
@edolstra edolstra self-assigned this Sep 25, 2020
@edolstra
Copy link
Member

@edolstra edolstra commented Sep 25, 2020

Thanks, will have a look. I'm getting deprecation notices from GitHub every few weeks :-)

@imalsogreg
Copy link
Contributor Author

@imalsogreg imalsogreg commented Sep 25, 2020

@edolstra @Ericson2314 @Kloenk Thanks! Also @kquick has opened a PR against my PR adding the ability to authenticate with on-premisis github/gitlab instances (imalsogreg#1). It involves changing the format of a nix config string for tokens, to accommodate the different types of tokens that github and gitlab accept, and to accommodate the fact that users have different tokens on multiple instances of github.

Should we merge that in to my PR and evaluate both at once? imalsogreg#1

@Kloenk
Copy link
Member

@Kloenk Kloenk commented Sep 25, 2020

It's a rather big pr for that functionality. So I would keep it as 2 separate prs

@edolstra edolstra merged commit cebd2fc into NixOS:master Sep 29, 2020
2 checks passed
2 checks passed
@github-actions
tests (ubuntu-latest)
Details
@github-actions
tests (macos-latest)
Details
@kquick kquick mentioned this pull request Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants