[github] update package/manifest for private repos

[CynicalBusiness]

This edit allows for the /github/(package|manifest)-json/ badges to work with private repositories.

I've updated the raw.githubusercontent.com URL to use GitHub's own API instead, and this allows it to work for private repositories. The old tests for this badge are still passing.

EDIT: The checks are failing on an unrelated badge, I think? I only ran tests for this one badge and everything looked good there. The failure seems to be for hit-counter.

[shields-ci]

	Warnings
⚠️	This PR modified the server but none of the service tests. That's okay so long as it's refactoring existing code.

	Messages
📖	✨ Thanks for your contribution to Shields, @CynicalBusiness!

Generated by 🚫 dangerJS

[RedSparr0w]

I can't seem to get this working with a private repo, wouldn't you still need to authenticate?

[CynicalBusiness]

@RedSparr0w It still needs an access token with private repo access to be added, as I've just updated this badge to use the same system as the other GitHub API badges (specifically, I was working off of github/tag) that already work with private repositories. Without a token, it does the same thing it would if the repository doesn't exist.

[RedSparr0w]

Think this should be okay for merge, but could you give me an example of how you would include a token to access a private repo so i can test this.

[CynicalBusiness]

On my fork, I've added a GitHub user token to the gh_token field private/secret.json as stated in the docs.

That is, my secret.json is:

{
  "gh_token": "MY-GH-TOKEN-IS-HERE"
}

Without it, the badge works normally on public repos. With it, my fork has access to both private repos I own and public repos.

[RedSparr0w]

Ah, i see. Didn't realize you meant for self hosted.
Approved 👍

[paulmelnikow]

Thanks for contributing this!

I'm happy to include this behavior for self-hosting users, though let's put it behind an environment variable. Perhaps GITHUB_ASSET_AUTH=true?

That way, we're not using up our authorization making github API requests when we can make an unauthenticated request instead.

[paulmelnikow]

Hi @CynicalBusiness would you be able to address this comment? It would be great to get this merged!

[CynicalBusiness]

@paulmelnikow My apologies, I had missed that comment.
Yeah, I can add that in. Would it be best to use the environment variable or just use the auth'ed version only when the token is present?

[paulmelnikow]

Would it be best to use the environment variable or just use the auth'ed version only when the token is present?

Huh, I hadn't really thought about that. It sounds fine to just use the auth'd version when the token is present. I don't really expect self-hosting users to be concerned about preserving quota (nor expect them to use rotating tokens as we do in production). So for anyone likely to use a static token, this behavior is fine.

Probably our service tests ought to cover both paths, since they're making different requests. I wonder if there's an easy way to do that.

[paulmelnikow]

Hey, thanks for updating this. This method call is intended for generating debug information and it would be surprising to see it used here. It's also not the right condition. Production uses token rotation, so this will be true, and that's where we want to avoid sending auth information.

I think the thing to check is whether a static token is configured: whether either serverSecrets.gh_token or serverSecrets.gh_client_id is set.

[paulmelnikow]

It appears this only needs small edits in order to be merged. That is, as long as it's done soon, before #2320 reaches this badge.

Is anyone willing to pick it up?

[paulmelnikow]

Closed in favor of #2470. This turned out to be a bit more involved, as the content endpoint returns JSON with a key containing the base64-encoded file contents.