Skip to content

Conversation

@chrisgavin
Copy link
Contributor

@chrisgavin chrisgavin commented Oct 22, 2020

This change allows specifying a token to be used for retrieving configuration files and queries from external repositories. This is useful if the repositories containing these files are private. It also has the side-effect of fixing the fact that external queries were not usable at all on GitHub Enterprise Server; they will now work automatically for public repositories and allow you to provide a token for private repositories.

I'm not completely convinced about the name of the configuration option so I'm very happy to hear suggestions about that (or obviously anything else about the implementation).

I've played around with the Git authentication logic a fair bit, and I think I've got something that works reasonably. I tried using the Git extraheader option to authenticate but I'm a bit concerned that by base-64 encoding the token before using it in a command we risk it being leaked by it not being redacted correctly all the time. Another option is to use a credential helper, which has the advantage of being able to pass credentials in environment variables instead of on the command line, but I'm not sure this would work cross-platform very well. I think another alternative would be to write the clone URL with credentials directly to .git/config, but that gets a bit complicated as we then need to be able to parse the gitconfig file format.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

@chrisgavin chrisgavin force-pushed the external-token-option branch from 6f63153 to 829bc20 Compare October 22, 2020 20:42
@chrisgavin chrisgavin force-pushed the external-token-option branch from 829bc20 to 625929c Compare October 23, 2020 08:20
@chrisgavin chrisgavin marked this pull request as ready for review October 23, 2020 08:39
@chrisgavin chrisgavin force-pushed the external-token-option branch 2 times, most recently from 5b963f8 to 66e8a24 Compare October 26, 2020 08:54
@chrisgavin chrisgavin force-pushed the external-token-option branch from 18378c3 to e12a8b8 Compare December 19, 2020 23:54
@chrisgavin chrisgavin force-pushed the external-token-option branch from e12a8b8 to e0c9247 Compare December 20, 2020 20:00
@chrisgavin
Copy link
Contributor Author

I've merged in main and fixed some conflicts related to the introduction of GitHubApiDetails. I'm not sure I've done a great job since combining the API token and URL into a single object seems kind of antithetical to this change. I guess the alternative is giving GitHubApiDetails two fields for tokens but I'm not sure if that's much better.

@robertbrignull
Copy link
Contributor

Hmm, I agree the introduction of GitHubApiDetails makes this PR a bit more complicated. It would be nice to just add the new token to GitHubApiDetails but that's annoying because we don't have the token available in all of the actions. It would be nice to get the type system to keep us safe, so we avoid nullable types and avoid using the same field for multiple uses.

I've had a go at doing the above as I thought it would be easier to just code it than to try to explain it in words. Have a look at #357 and let me know what you think. Short summary is we introduce a new type and use typescript's duck typing so we can just pass in a bigger type and it'll ignore the extra fields.

@chrisgavin chrisgavin deleted the external-token-option branch February 14, 2025 12:55
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.

3 participants