feat(install): Allow downloading from private repositories

[tmsmith]

Closes #4243.

Github Private repositories return a 404 when trying to use the releases download even with a provided API Token. To solve this I used the releases API which returns an asset URL that will allow for the download request. Also with this PR, a section can be defined in the config file to inject any custom headers needed for other buckets.

{
    "lastupdate":  "2021-02-09T20:48:57.2632262-05:00",
    "hosts":  [
                  {
                      "match":  "https://localhost/repos.*",
                      "headers":  "@{Authorization=token ABC}"
                  },
                  {
                      "match":  "https://somewhere.com/.*",
                      "headers":  "@{Authorization=token XXX}"
                  }
              ],
    "checkver_token":  "<GH Token>"
}

[timharris777]

Waiting for this feature!

[JustinLoyed]

Any update if this can be reviewed and included?

[rashil2000]

Will be reviewed in the next release cycle.

[ghost]

This is great feature - waiting for it to start using scoop as "homebrew for Windows" at my company!

[rashil2000]

What scopes are required in the GH token used here? Is it a plain token?

We already use a GH API token for making authenticated requests, we can use it here too:

Scoop/libexec/scoop-config.ps1

Lines 79 to 82 in 7d5a47c

	# checkver_token:
	# GitHub API token used to make authenticated requests.
	# This is essential for checkver and similar functions
	# to run without incurring rate limits.

[ghost]

@rashil2000 I think it is repo scope, probably same as required for checkver_token token, so we definitely can merge this two - just applied patch locally, added checkver_token and run over bucket with private repos artifacts - worked fine.
However name checkver_token will be confusing, should we rename it to github_token and use for both cases?

[rashil2000]

checkver_token didn't require any scopes.

But it is fair to assume that users would already have repo scope if they plan to use private stuff.

Renaming to github_token or gh_api_token should do

[rashil2000]

@tmsmith can you update this PR with latest develop?

[ghost]

It looks like URL syntax <URL>#/filename is broken for the private repos, investigating right now.

[ghost]

I have a fix for that. If nobody objects, I will create a new PR with github private thing only (custom headers can be added later - I don't have setup to test them).
@rashil2000 should I rename checkver_token to gh_api_token in this PR?

[rashil2000]

Sure, go ahead!

[ghost]

@tmsmith I have a validated fix for issue described above. How do you want me to share it? Should I make a PR to https://github.com/tmsmith/scoop/tree/private-github-support or just share a code snippet with you?

Here it is, changes are in line with regex and line where url is set.

    # Github.com
    if ($url -match "github.com\/(?<owner>[^\/]+)\/(?<repo>[^\/]+)\/releases\/download\/(?<tag>[^\/]+)\/(?<file>[^\/#]+)(?<filename>.*)" -and (get_config 'gh_api_token')) {
        $headers = @{
            "Authorization" = "token $(get_config 'gh_api_token')"
        }
        $privateUrl = "https://api.github.com/repos/$($matches['owner'])/$($matches['repo'])"
        $assetUrl="https://api.github.com/repos/$($matches['owner'])/$($matches['repo'])/releases/tags/$($matches['tag'])"

        $isPrivate = (Invoke-RestMethod -Uri $privateUrl -Headers $headers).private
        if ($isPrivate) {
            $url = ((Invoke-RestMethod -Uri $assetUrl -Headers $headers).assets | Where-Object { $_.name -eq $matches['file'] }).url + $($matches['filename'])
        }
    }

[tmsmith]

PR is fine.

[ghost]

@tmsmith please check my comment above and fix the issue with using #/ in private repositories URLs

[tmsmith]

I updated the fork with the code snippet you posted above.

[ghost]

@tmsmith Thanks! I re-checked the current PR code and it works fine for me with gh_api_token set.

@rashil2000 I think it is good to go now. There is definitely a need to combine/rework SCOOP_CHECKVER_TOKEN, checkver-token and gh_api_token, so we use single environment variable and single configuration property across the code base, however this can be done in separate PR (I plan to open one once this one is merged). Do you have any concerns about this approach?

[rashil2000]

Actually, let's keep using checkver_token in this PR too. We can consolidate the name changes properly in a separate PR.

@tmsmith Please inline the get_headers and get-members functions, as they're not being used anywhere else. And update the changelog.

[ghost]

@tmsmith will you be able to update the PR as requested? I really want this feature to be in place before I start rolling out scoop as internal tool for the company...

[tmsmith]

I have update the files as requested. I currently do not have a windows machine that functions so I was unable to fully test the changes.

[ghost]

@tmsmith I can do the test. However can you please use checkver_token instead of gh_api_token? This is what @rashil2000 asked, so we reuse existing tokens instead of adding new. Thanks!

[ghost]

@tmsmith I did some testing for current PR state and while github stuff works fine, the proposed solution of custom headers doesn't work. I spent some time fixing it and here is what I got:

This is how hosts-related section of dl function is install.ps1 looks like (not sure why so complex approach was selected originally - everything can be achieved in single instruction:

        $hosts = get_config "hosts"
        if ($hosts) {
            $hosts | Where-Object { $url -match $_.match } | ForEach-Object {
                (ConvertFrom-StringData -StringData $_.headers).GetEnumerator() | ForEach-Object {
                    $wreq.headers[$_.Key] = $_.Value
                }
            }
        }

And here is example of configuration file hosts section definition:

    "hosts": [
        {
            "match": ".*api.github.com/repos.*",
            "headers": "Authorization=token SECRET_TOKEN\nCustomHeader=Custom Header Value"
        }
    ]

You can see: headers are defined using KEY=VALUE syntax, multiple values can be specified using \n (the JSON newline character). This setup is validated and works - header(s) get populated as expected.

Hopefully with this change/testing done, we can finally get this merged! Thanks @tmsmith and @rashil2000!

[niheaven]

Some static review, and please move Features to top of Unrelease in CHANGELOG.

Suggest using 'Allow downloading from private repositories` as CHANGELOG entry.

Since I'm not using private repo, please check if the review comments work.

PS: Also suggest using private_hosts in config file.

[ghost]

Thanks @niheaven - I checked you changes with both github and private host repo and they work fine!

@tmsmith can you please incorporate proposed changes and rename hosts to private_hosts in code. This should be last change needed before merge. Thanks in advance for helping getting this merged - appreciate it!

[niheaven]

I've done some tweaks and renamed config entry to private_hosts, and please check for the commits @tmsmith @astelmachonak-nydig @rashil2000.

If no bugs, it could be merged, and if there're some that introduced by my commits, please revert them or fixed.

[ghost]

I will re-test this tomorrow.

[ghost]

Just tested - everything works as expected from current branch. Let's merge it!

[ghost]

Thanks @tmsmith @rashil2000 @niheaven
This is great feature to have. When do you think it will be in master as part of next release?

[ghost]

@rashil2000 looks like your last commit broke it when private_hosts variable is not set :( - I am getting error:
Cannot bind argument to parameter 'StringData' because it is null.

Will need to open the PR to fix it

[rashil2000]

@rashil2000 looks like your last commit broke it when private_hosts variable is not set :( - I am getting error: Cannot bind argument to parameter 'StringData' because it is null.

Oh, sorry, my bad. I thought Where-Object and ForEach-Object functions would automatically take care of null input.

Will need to open the PR to fix it

Are you opening it?

[ghost]

Are you opening it?

Yes, shortly.

[ghost]

@rashil2000 #4832

[niheaven]

I thought Where-Object and ForEach-Object functions would automatically take care of null input.

Actually, they cannot, and that's why I rarely use them in pipeline.

[CEbbinghaus]

Is there documentation on this feature?

[rashil2000]

Run scoop help config