-
Notifications
You must be signed in to change notification settings - Fork 226
Remove canary requirement for GitHub database download #1485
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
Changes from all commits
95438bb
fe5f1c4
8d5067f
ac1a97e
21c5ed0
23a0e03
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,16 +76,27 @@ export class Credentials { | |
| })); | ||
| } | ||
|
|
||
| async getOctokit(): Promise<Octokit.Octokit> { | ||
| /** | ||
| * Creates or returns an instance of Octokit. | ||
| * | ||
| * @param requireAuthentication Whether the Octokit instance needs to be authenticated as user. | ||
| * @returns An instance of Octokit. | ||
| */ | ||
| async getOctokit(requireAuthentication = true): Promise<Octokit.Octokit> { | ||
| if (this.octokit) { | ||
| return this.octokit; | ||
| } | ||
|
|
||
| this.octokit = await this.createOctokit(true); | ||
| // octokit shouldn't be undefined, since we've set "createIfNone: true". | ||
| // The following block is mainly here to prevent a compiler error. | ||
| this.octokit = await this.createOctokit(requireAuthentication); | ||
|
|
||
| if (!this.octokit) { | ||
| throw new Error('Did not initialize Octokit.'); | ||
| if (requireAuthentication) { | ||
| throw new Error('Did not initialize Octokit.'); | ||
| } | ||
|
|
||
| // We don't want to set this in this.octokit because that would prevent | ||
| // authenticating when requireCredentials is true. | ||
| return new Octokit.Octokit({ retry }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be safe to store this octokit instance in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though, what you have now is also fine.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's not safe to store it in the instance variable. That would fail in the following scenario:
|
||
| } | ||
| return this.octokit; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,8 @@ import { | |
| import { CodeQLCliServer } from './cli'; | ||
| import * as fs from 'fs-extra'; | ||
| import * as path from 'path'; | ||
| import * as Octokit from '@octokit/rest'; | ||
| import { retry } from '@octokit/plugin-retry'; | ||
|
|
||
| import { DatabaseManager, DatabaseItem } from './databases'; | ||
| import { | ||
|
|
@@ -76,7 +78,7 @@ export async function promptImportInternetDatabase( | |
| export async function promptImportGithubDatabase( | ||
| databaseManager: DatabaseManager, | ||
| storagePath: string, | ||
| credentials: Credentials, | ||
| credentials: Credentials | undefined, | ||
| progress: ProgressCallback, | ||
| token: CancellationToken, | ||
| cli?: CodeQLCliServer | ||
|
|
@@ -99,14 +101,15 @@ export async function promptImportGithubDatabase( | |
| throw new Error(`Invalid GitHub repository: ${githubRepo}`); | ||
| } | ||
|
|
||
| const result = await convertGithubNwoToDatabaseUrl(githubRepo, credentials, progress); | ||
| const octokit = credentials ? await credentials.getOctokit(true) : new Octokit.Octokit({ retry }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be possible to move this 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 Do you think pulling that out to a shared location makes sense? |
||
|
|
||
| const result = await convertGithubNwoToDatabaseUrl(githubRepo, octokit, progress); | ||
| if (!result) { | ||
| return; | ||
| } | ||
|
|
||
| const { databaseUrl, name, owner } = result; | ||
|
|
||
| const octokit = await credentials.getOctokit(); | ||
| /** | ||
| * The 'token' property of the token object returned by `octokit.auth()`. | ||
| * The object is undocumented, but looks something like this: | ||
|
|
@@ -118,14 +121,9 @@ export async function promptImportGithubDatabase( | |
| * We only need the actual token string. | ||
| */ | ||
| const octokitToken = (await octokit.auth() as { token: string })?.token; | ||
| if (!octokitToken) { | ||
| // Just print a generic error message for now. Ideally we could show more debugging info, like the | ||
| // octokit object, but that would expose a user token. | ||
| throw new Error('Unable to get GitHub token.'); | ||
| } | ||
| const item = await databaseArchiveFetcher( | ||
| databaseUrl, | ||
| { 'Accept': 'application/zip', 'Authorization': `Bearer ${octokitToken}` }, | ||
| { 'Accept': 'application/zip', 'Authorization': octokitToken ? `Bearer ${octokitToken}` : '' }, | ||
| databaseManager, | ||
| storagePath, | ||
| `${owner}/${name}`, | ||
|
|
@@ -523,7 +521,7 @@ function convertGitHubUrlToNwo(githubUrl: string): string | undefined { | |
|
|
||
| export async function convertGithubNwoToDatabaseUrl( | ||
| githubRepo: string, | ||
| credentials: Credentials, | ||
| octokit: Octokit.Octokit, | ||
| progress: ProgressCallback): Promise<{ | ||
| databaseUrl: string, | ||
| owner: string, | ||
|
|
@@ -533,7 +531,6 @@ export async function convertGithubNwoToDatabaseUrl( | |
| const nwo = convertGitHubUrlToNwo(githubRepo) || githubRepo; | ||
| const [owner, repo] = nwo.split('/'); | ||
|
|
||
| const octokit = await credentials.getOctokit(); | ||
| const response = await octokit.request('GET /repos/:owner/:repo/code-scanning/codeql/databases', { owner, repo }); | ||
|
|
||
| const languages = response.data.map((db: any) => db.language); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -970,7 +970,7 @@ async function activateWithInstalledDistribution( | |
| progress: ProgressCallback, | ||
| token: CancellationToken | ||
| ) => { | ||
| const credentials = await Credentials.initialize(ctx); | ||
| const credentials = isCanary() ? await Credentials.initialize(ctx) : undefined; | ||
| await databaseUI.handleChooseDatabaseGithub(credentials, progress, token); | ||
| }, | ||
| { | ||
|
|
@@ -1018,19 +1018,16 @@ async function activateWithInstalledDistribution( | |
| } | ||
| }; | ||
|
|
||
| // The "authenticateToGitHub" command is internal-only. | ||
| ctx.subscriptions.push( | ||
| commandRunner('codeQL.authenticateToGitHub', async () => { | ||
| if (isCanary()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this change still be here? I thought we would still only authenticate when in canary mode.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This command should never be called in non-canary mode, so this shouldn't really matter |
||
| /** | ||
| * Credentials for authenticating to GitHub. | ||
| * These are used when making API calls. | ||
| */ | ||
| const credentials = await Credentials.initialize(ctx); | ||
| const octokit = await credentials.getOctokit(); | ||
| const userInfo = await octokit.users.getAuthenticated(); | ||
| void showAndLogInformationMessage(`Authenticated to GitHub as user: ${userInfo.data.login}`); | ||
| } | ||
| /** | ||
| * Credentials for authenticating to GitHub. | ||
| * These are used when making API calls. | ||
| */ | ||
| const credentials = await Credentials.initialize(ctx); | ||
| const octokit = await credentials.getOctokit(); | ||
| const userInfo = await octokit.users.getAuthenticated(); | ||
| void showAndLogInformationMessage(`Authenticated to GitHub as user: ${userInfo.data.login}`); | ||
| })); | ||
|
|
||
| ctx.subscriptions.push( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a case where we pass
falseto this method?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not currently, but see 23a0e03 where I give a reasoning for why I left it like this