From 95438bb7e354beb4f3be435c98654703f4396a8e Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Tue, 23 Aug 2022 14:45:38 +0200 Subject: [PATCH 1/6] Remove canary requirement for GitHub database download --- extensions/ql-vscode/package.json | 12 ++---------- extensions/ql-vscode/src/extension.ts | 19 ++++++++----------- 2 files changed, 10 insertions(+), 21 deletions(-) diff --git a/extensions/ql-vscode/package.json b/extensions/ql-vscode/package.json index 233d7eeb306..2af45ba192c 100644 --- a/extensions/ql-vscode/package.json +++ b/extensions/ql-vscode/package.json @@ -664,7 +664,7 @@ }, { "command": "codeQLDatabases.chooseDatabaseGithub", - "when": "config.codeQL.canary && view == codeQLDatabases", + "when": "view == codeQLDatabases", "group": "navigation" }, { @@ -878,10 +878,6 @@ } ], "commandPalette": [ - { - "command": "codeQL.authenticateToGitHub", - "when": "config.codeQL.canary" - }, { "command": "codeQL.runQuery", "when": "resourceLangId == ql && resourceExtname == .ql" @@ -926,10 +922,6 @@ "command": "codeQL.viewCfg", "when": "resourceScheme == codeql-zip-archive && config.codeQL.canary" }, - { - "command": "codeQL.chooseDatabaseGithub", - "when": "config.codeQL.canary" - }, { "command": "codeQLDatabases.setCurrentDatabase", "when": "false" @@ -1175,7 +1167,7 @@ }, { "view": "codeQLDatabases", - "contents": "Add a CodeQL database:\n[From a folder](command:codeQLDatabases.chooseDatabaseFolder)\n[From an archive](command:codeQLDatabases.chooseDatabaseArchive)\n[From a URL (as a zip file)](command:codeQLDatabases.chooseDatabaseInternet)\n[From LGTM](command:codeQLDatabases.chooseDatabaseLgtm)" + "contents": "Add a CodeQL database:\n[From a folder](command:codeQLDatabases.chooseDatabaseFolder)\n[From an archive](command:codeQLDatabases.chooseDatabaseArchive)\n[From a URL (as a zip file)](command:codeQLDatabases.chooseDatabaseInternet)\n[From GitHub](command:codeQLDatabases.chooseDatabaseGithub)\n[From LGTM](command:codeQLDatabases.chooseDatabaseLgtm)" }, { "view": "codeQLEvalLogViewer", diff --git a/extensions/ql-vscode/src/extension.ts b/extensions/ql-vscode/src/extension.ts index 4da5e8ab9e0..43109b93be2 100644 --- a/extensions/ql-vscode/src/extension.ts +++ b/extensions/ql-vscode/src/extension.ts @@ -1018,19 +1018,16 @@ async function activateWithInstalledDistribution( } }; - // The "authenticateToGitHub" command is internal-only. ctx.subscriptions.push( commandRunner('codeQL.authenticateToGitHub', async () => { - if (isCanary()) { - /** - * 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( From fe5f1c417dcbc368c3589aa398d7bb22d67b275b Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Tue, 30 Aug 2022 15:05:15 +0200 Subject: [PATCH 2/6] Remove authentication requirement for download GitHub databases 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. --- extensions/ql-vscode/src/authentication.ts | 21 ++++++++++++++++----- extensions/ql-vscode/src/databaseFetcher.ts | 18 ++++++++---------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/extensions/ql-vscode/src/authentication.ts b/extensions/ql-vscode/src/authentication.ts index aae23690453..89dc8c36558 100644 --- a/extensions/ql-vscode/src/authentication.ts +++ b/extensions/ql-vscode/src/authentication.ts @@ -76,16 +76,27 @@ export class Credentials { })); } - async getOctokit(): Promise { + /** + * Creates or returns an instance of Octokit. + * + * @param requireAuthentication Whether the Octokit instance needs to be authentication as user. + * @returns An instance of Octokit. + */ + async getOctokit(requireAuthentication = true): Promise { 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 }); } return this.octokit; } diff --git a/extensions/ql-vscode/src/databaseFetcher.ts b/extensions/ql-vscode/src/databaseFetcher.ts index 3ad49521c8d..a78ba34cde0 100644 --- a/extensions/ql-vscode/src/databaseFetcher.ts +++ b/extensions/ql-vscode/src/databaseFetcher.ts @@ -10,6 +10,7 @@ import { import { CodeQLCliServer } from './cli'; import * as fs from 'fs-extra'; import * as path from 'path'; +import * as Octokit from '@octokit/rest'; import { DatabaseManager, DatabaseItem } from './databases'; import { @@ -23,6 +24,7 @@ import { logger } from './logging'; import { tmpDir } from './helpers'; import { Credentials } from './authentication'; import { REPO_REGEX, getErrorMessage } from './pure/helpers-pure'; +import { isCanary } from './config'; /** * Prompts a user to fetch a database from a remote location. Database is assumed to be an archive file. @@ -99,14 +101,16 @@ export async function promptImportGithubDatabase( throw new Error(`Invalid GitHub repository: ${githubRepo}`); } - const result = await convertGithubNwoToDatabaseUrl(githubRepo, credentials, progress); + // Only require authentication if we are running with the canary flag enabled + const octokit = await credentials.getOctokit(isCanary()); + + 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 +122,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 +522,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 +532,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); From 8d5067f62277deabb8e5b2a86988cb4055754fe2 Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Tue, 30 Aug 2022 15:09:16 +0200 Subject: [PATCH 3/6] Update CHANGELOG --- extensions/ql-vscode/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/extensions/ql-vscode/CHANGELOG.md b/extensions/ql-vscode/CHANGELOG.md index 42d9d72cc35..3c2937356c6 100644 --- a/extensions/ql-vscode/CHANGELOG.md +++ b/extensions/ql-vscode/CHANGELOG.md @@ -2,6 +2,8 @@ ## [UNRELEASED] +- Add ability for users to download databases directly from GitHub. [#1485](https://github.com/github/vscode-codeql/pull/1485) + ## 1.6.11 - 25 August 2022 No user facing changes. From ac1a97efa0cc34e1143cf4e7db0e3948eb1b7504 Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Tue, 30 Aug 2022 15:32:08 +0200 Subject: [PATCH 4/6] Refactor databaseFetcher tests to not use proxyquire --- .../no-workspace/databaseFetcher.test.ts | 40 ++++++++----------- 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/extensions/ql-vscode/src/vscode-tests/no-workspace/databaseFetcher.test.ts b/extensions/ql-vscode/src/vscode-tests/no-workspace/databaseFetcher.test.ts index 0256e29b7d9..362df65f5a4 100644 --- a/extensions/ql-vscode/src/vscode-tests/no-workspace/databaseFetcher.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/no-workspace/databaseFetcher.test.ts @@ -6,15 +6,14 @@ import { expect } from 'chai'; import { window } from 'vscode'; import { + convertGithubNwoToDatabaseUrl, convertLgtmUrlToDatabaseUrl, looksLikeLgtmUrl, findDirWithFile, looksLikeGithubRepo, } from '../../databaseFetcher'; import { ProgressCallback } from '../../commandRunner'; -import * as pq from 'proxyquire'; - -const proxyquire = pq.noPreserveCache(); +import * as Octokit from '@octokit/rest'; describe('databaseFetcher', function() { // These tests make API calls and may need extra time to complete. @@ -25,20 +24,16 @@ describe('databaseFetcher', function() { let quickPickSpy: sinon.SinonStub; let progressSpy: ProgressCallback; let mockRequest: sinon.SinonStub; - let mod: any; - - const credentials = getMockCredentials(0); + let octokit: Octokit.Octokit; beforeEach(() => { sandbox = sinon.createSandbox(); quickPickSpy = sandbox.stub(window, 'showQuickPick'); progressSpy = sandbox.spy(); mockRequest = sandbox.stub(); - mod = proxyquire('../../databaseFetcher', { - './authentication': { - Credentials: credentials, - }, - }); + octokit = ({ + request: mockRequest, + }) as unknown as Octokit.Octokit; }); afterEach(() => { @@ -90,11 +85,17 @@ describe('databaseFetcher', function() { mockRequest.resolves(mockApiResponse); quickPickSpy.resolves('javascript'); const githubRepo = 'github/codeql'; - const { databaseUrl, name, owner } = await mod.convertGithubNwoToDatabaseUrl( + const result = await convertGithubNwoToDatabaseUrl( githubRepo, - credentials, + octokit, progressSpy ); + expect(result).not.to.be.undefined; + if (result === undefined) { + return; + } + + const { databaseUrl, name, owner } = result; expect(databaseUrl).to.equal( 'https://api.github.com/repos/github/codeql/code-scanning/codeql/databases/javascript' @@ -119,7 +120,7 @@ describe('databaseFetcher', function() { mockRequest.resolves(mockApiResponse); const githubRepo = 'foo/bar-not-real'; await expect( - mod.convertGithubNwoToDatabaseUrl(githubRepo, credentials, progressSpy) + convertGithubNwoToDatabaseUrl(githubRepo, octokit, progressSpy) ).to.be.rejectedWith(/Unable to get database/); expect(progressSpy).to.have.callCount(0); }); @@ -133,19 +134,10 @@ describe('databaseFetcher', function() { mockRequest.resolves(mockApiResponse); const githubRepo = 'foo/bar-with-no-dbs'; await expect( - mod.convertGithubNwoToDatabaseUrl(githubRepo, credentials, progressSpy) + convertGithubNwoToDatabaseUrl(githubRepo, octokit, progressSpy) ).to.be.rejectedWith(/Unable to get database/); expect(progressSpy).to.have.been.calledOnce; }); - - function getMockCredentials(response: any) { - mockRequest = sinon.stub().resolves(response); - return { - getOctokit: () => ({ - request: mockRequest, - }), - }; - } }); describe('convertLgtmUrlToDatabaseUrl', () => { From 21c5ed01ad884fa64779b82b5adabce5987f2a0a Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Wed, 31 Aug 2022 11:48:27 +0200 Subject: [PATCH 5/6] Fix typo in getOctokit JSDoc Co-authored-by: Andrew Eisenberg --- extensions/ql-vscode/src/authentication.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/ql-vscode/src/authentication.ts b/extensions/ql-vscode/src/authentication.ts index 89dc8c36558..d8fc6927a0f 100644 --- a/extensions/ql-vscode/src/authentication.ts +++ b/extensions/ql-vscode/src/authentication.ts @@ -79,7 +79,7 @@ export class Credentials { /** * Creates or returns an instance of Octokit. * - * @param requireAuthentication Whether the Octokit instance needs to be authentication as user. + * @param requireAuthentication Whether the Octokit instance needs to be authenticated as user. * @returns An instance of Octokit. */ async getOctokit(requireAuthentication = true): Promise { From 23a0e03cef87611ac35737ee62a1549e36f10c55 Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Wed, 31 Aug 2022 14:22:17 +0200 Subject: [PATCH 6/6] Completely remove using credentials in non-canary mode 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. --- extensions/ql-vscode/package.json | 4 ++++ extensions/ql-vscode/src/databaseFetcher.ts | 7 +++---- extensions/ql-vscode/src/databases-ui.ts | 5 +++-- extensions/ql-vscode/src/extension.ts | 2 +- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/extensions/ql-vscode/package.json b/extensions/ql-vscode/package.json index 2af45ba192c..650f1978066 100644 --- a/extensions/ql-vscode/package.json +++ b/extensions/ql-vscode/package.json @@ -878,6 +878,10 @@ } ], "commandPalette": [ + { + "command": "codeQL.authenticateToGitHub", + "when": "config.codeQL.canary" + }, { "command": "codeQL.runQuery", "when": "resourceLangId == ql && resourceExtname == .ql" diff --git a/extensions/ql-vscode/src/databaseFetcher.ts b/extensions/ql-vscode/src/databaseFetcher.ts index a78ba34cde0..322d09b3a7e 100644 --- a/extensions/ql-vscode/src/databaseFetcher.ts +++ b/extensions/ql-vscode/src/databaseFetcher.ts @@ -11,6 +11,7 @@ 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 { @@ -24,7 +25,6 @@ import { logger } from './logging'; import { tmpDir } from './helpers'; import { Credentials } from './authentication'; import { REPO_REGEX, getErrorMessage } from './pure/helpers-pure'; -import { isCanary } from './config'; /** * Prompts a user to fetch a database from a remote location. Database is assumed to be an archive file. @@ -78,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 @@ -101,8 +101,7 @@ export async function promptImportGithubDatabase( throw new Error(`Invalid GitHub repository: ${githubRepo}`); } - // Only require authentication if we are running with the canary flag enabled - const octokit = await credentials.getOctokit(isCanary()); + const octokit = credentials ? await credentials.getOctokit(true) : new Octokit.Octokit({ retry }); const result = await convertGithubNwoToDatabaseUrl(githubRepo, octokit, progress); if (!result) { diff --git a/extensions/ql-vscode/src/databases-ui.ts b/extensions/ql-vscode/src/databases-ui.ts index cf474bfc5c6..bea2bc425a2 100644 --- a/extensions/ql-vscode/src/databases-ui.ts +++ b/extensions/ql-vscode/src/databases-ui.ts @@ -40,6 +40,7 @@ import { import { CancellationToken } from 'vscode'; import { asyncFilter, getErrorMessage } from './pure/helpers-pure'; import { Credentials } from './authentication'; +import { isCanary } from './config'; type ThemableIconPath = { light: string; dark: string } | string; @@ -301,7 +302,7 @@ export class DatabaseUI extends DisposableObject { progress: ProgressCallback, token: CancellationToken ) => { - const credentials = await this.getCredentials(); + const credentials = isCanary() ? await this.getCredentials() : undefined; await this.handleChooseDatabaseGithub(credentials, progress, token); }, { @@ -480,7 +481,7 @@ export class DatabaseUI extends DisposableObject { }; handleChooseDatabaseGithub = async ( - credentials: Credentials, + credentials: Credentials | undefined, progress: ProgressCallback, token: CancellationToken ): Promise => { diff --git a/extensions/ql-vscode/src/extension.ts b/extensions/ql-vscode/src/extension.ts index 43109b93be2..80c04dbbc4b 100644 --- a/extensions/ql-vscode/src/extension.ts +++ b/extensions/ql-vscode/src/extension.ts @@ -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); }, {