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

Jump to bottom

Fetch commits from github private repos using Authorization header #3675

Merged
merged 2 commits into from Sep 29, 2020
Merged

Fetch commits from github private repos using Authorization header #3675

merged 2 commits into from Sep 29, 2020

Conversation

imalsogreg
Copy link
Contributor

@imalsogreg imalsogreg commented Jun 9, 2020 •
edited

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

@Ericson2314
Copy link
Member

Link to deprecation notice: https://developer.github.com/changes/2019-11-05-deprecated-passwords-and-authorizations-api/#authenticating-using-query-parameters

@imalsogreg
Copy link
Contributor Author

@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)?

edolstra
edolstra reviewed Jun 10, 2020
View reviewed changes
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 github-api-token branch 3 times, most recently from 83d1b6c to a24c725 Compare June 10, 2020 15:28
@Kloenk
Copy link
Member

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

"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 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

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

@Kloenk Kloenk mentioned this pull request Jun 13, 2020
Closed
@Kloenk
Copy link
Member

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
Copy link
Contributor Author

@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
View reviewed changes
Copy link
Member

@Kloenk Kloenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Ericson2314
Copy link
Member

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 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

Ah right, forgot this was to that branch.

@cole-h
Copy link
Member

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 changed the base branch from flakes to master August 11, 2020 02:18
@imalsogreg
Copy link
Contributor Author

@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.

@imalsogreg
Fetch commits from github/gitlab using Auth header
a303c0b 
`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.
@edolstra edolstra self-assigned this Sep 25, 2020
@edolstra
Copy link
Member

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

@imalsogreg
Copy link
Contributor Author

@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 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
@kquick kquick mentioned this pull request Sep 29, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ericson2314 Ericson2314 Ericson2314 approved these changes

@edolstra edolstra edolstra left review comments

@Kloenk Kloenk Kloenk approved these changes

Assignees

@edolstra edolstra

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@imalsogreg @Ericson2314 @Kloenk @cole-h @edolstra