Remove canary requirement for GitHub database download#1485
Conversation
This makes authentication for download GitHub CodeQL databases optional. If you are already authenticated, your token will be used. If you are not authenticated, an anonymous request will be made. If the canary flag is enabled, you will be prompted for credentials when downloading a database and you are not yet logged in.
aeisenberg
left a comment
aeisenberg
left a comment
There was a problem hiding this comment.
A couple of minor suggestions, but this can go in without them.
|
|
||
| // We don't want to set this in this.octokit because that would prevent | ||
| // authenticating when requireCredentials is true. | ||
| return new Octokit.Octokit({ retry }); |
There was a problem hiding this comment.
It might be safe to store this octokit instance in this.octokit. Above, a listener is being registered that will recreate the octokit instance when credentials change.
There was a problem hiding this comment.
Though, what you have now is also fine.
There was a problem hiding this comment.
I think it's not safe to store it in the instance variable. That would fail in the following scenario:
- You first download a database from GitHub (
this.octokitis set to an unauthenticated instance) - You now try to run a variant analysis, which requires authentication. However,
this.octokitis set so an unauthenticated instance, so an unauthenticated instance is returned whilerequireAuthenticationistrue.
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
This does not remove the previously added mechanism of not requesting credentials, but using them when they are available. I expect this to be used in the future.
shati-patel
left a comment
shati-patel
left a comment
There was a problem hiding this comment.
Thanks for the updates! This looks good to me 😺 ✨
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
I have a couple of questions about points in the code. I have tried it out though and it seems to be working as intended.
| } | ||
|
|
||
| const result = await convertGithubNwoToDatabaseUrl(githubRepo, credentials, progress); | ||
| const octokit = credentials ? await credentials.getOctokit(true) : new Octokit.Octokit({ retry }); |
There was a problem hiding this comment.
Would it be possible to move this new Octokit.Octokit({ retry }) somewhere so it's co-located with how the credentials creates its octokit? This might mean making a new util file just for creating octokits.
My reasoning here is that we want to construction to be the same in all cases, and that's difficult if we're constructing them and passing settings (like retry) from various places.
Do you think pulling that out to a shared location makes sense?
| // The "authenticateToGitHub" command is internal-only. | ||
| ctx.subscriptions.push( | ||
| commandRunner('codeQL.authenticateToGitHub', async () => { | ||
| if (isCanary()) { |
There was a problem hiding this comment.
Should this change still be here? I thought we would still only authenticate when in canary mode.
There was a problem hiding this comment.
This command should never be called in non-canary mode, so this shouldn't really matter
| * @param requireAuthentication Whether the Octokit instance needs to be authenticated as user. | ||
| * @returns An instance of Octokit. | ||
| */ | ||
| async getOctokit(requireAuthentication = true): Promise<Octokit.Octokit> { |
There was a problem hiding this comment.
Is there a case where we pass false to this method?
There was a problem hiding this comment.
No, not currently, but see 23a0e03 where I give a reasoning for why I left it like this
4 participants
This adds the ability for users to download CodeQL databases from GitHub without the canary flag being set.
If the canary flag is set, we will always require authentication. If it is not set, we do not require authentication, but will use an authentication token if it is available. The "Authenticate to GitHub" command is also available, so users which are not using the canary flag can still download private CodeQL databases using that command, although it's only available using the command palette.
See #1466 for the initial implementation of this.
Checklist
ready-for-doc-reviewlabel there.