Merge pull request #1892 from github/robertbrignull/undefined_credentials

Simplify the credentials class, and clear up impossible error cases
This commit is contained in:
Robert
2022-12-22 11:20:09 +00:00
committed by GitHub
14 changed files with 520 additions and 694 deletions

View File

@@ -13,102 +13,59 @@ const SCOPES = ["repo", "gist"];
* Handles authentication to GitHub, using the VS Code [authentication API](https://code.visualstudio.com/api/references/vscode-api#authentication).
*/
export class Credentials {
/**
* A specific octokit to return, otherwise a new authenticated octokit will be created when needed.
*/
private octokit: Octokit.Octokit | undefined;
// Explicitly make the constructor private, so that we can't accidentally call the constructor from outside the class
// without also initializing the class.
// eslint-disable-next-line @typescript-eslint/no-empty-function
private constructor() {}
private constructor(octokit?: Octokit.Octokit) {
this.octokit = octokit;
}
/**
* Initializes an instance of credentials with an octokit instance.
* Initializes a Credentials instance. This will generate octokit instances
* authenticated as the user. If there is not already an authenticated GitHub
* session available then the user will be prompted to log in.
*
* Do not call this method until you know you actually need an instance of credentials.
* since calling this method will require the user to log in.
*
* @param context The extension context.
* @returns An instance of credentials.
*/
static async initialize(
context: vscode.ExtensionContext,
): Promise<Credentials> {
const c = new Credentials();
c.registerListeners(context);
c.octokit = await c.createOctokit(false);
return c;
static async initialize(): Promise<Credentials> {
return new Credentials();
}
/**
* Initializes an instance of credentials with an octokit instance using
* a token from the user's GitHub account. This method is meant to be
* used non-interactive environments such as tests.
* a specific known token. This method is meant to be used in
* non-interactive environments such as tests.
*
* @param overrideToken The GitHub token to use for authentication.
* @returns An instance of credentials.
*/
static async initializeWithToken(overrideToken: string) {
const c = new Credentials();
c.octokit = await c.createOctokit(false, overrideToken);
return c;
}
private async createOctokit(
createIfNone: boolean,
overrideToken?: string,
): Promise<Octokit.Octokit | undefined> {
if (overrideToken) {
return new Octokit.Octokit({ auth: overrideToken, retry });
}
const session = await vscode.authentication.getSession(
GITHUB_AUTH_PROVIDER_ID,
SCOPES,
{ createIfNone },
);
if (session) {
return new Octokit.Octokit({
auth: session.accessToken,
retry,
});
} else {
return undefined;
}
}
registerListeners(context: vscode.ExtensionContext): void {
// Sessions are changed when a user logs in or logs out.
context.subscriptions.push(
vscode.authentication.onDidChangeSessions(async (e) => {
if (e.provider.id === GITHUB_AUTH_PROVIDER_ID) {
this.octokit = await this.createOctokit(false);
}
}),
);
return new Credentials(new Octokit.Octokit({ auth: overrideToken, retry }));
}
/**
* 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> {
async getOctokit(): Promise<Octokit.Octokit> {
if (this.octokit) {
return this.octokit;
}
this.octokit = await this.createOctokit(requireAuthentication);
const session = await vscode.authentication.getSession(
GITHUB_AUTH_PROVIDER_ID,
SCOPES,
{ createIfNone: true },
);
if (!this.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;
return new Octokit.Octokit({
auth: session.accessToken,
retry,
});
}
}

View File

@@ -106,7 +106,7 @@ export async function promptImportGithubDatabase(
}
const octokit = credentials
? await credentials.getOctokit(true)
? await credentials.getOctokit()
: new Octokit.Octokit({ retry });
const result = await convertGithubNwoToDatabaseUrl(nwo, octokit, progress);

View File

@@ -591,7 +591,7 @@ async function activateWithInstalledDistribution(
qs,
getContextStoragePath(ctx),
ctx.extensionPath,
() => Credentials.initialize(ctx),
() => Credentials.initialize(),
);
databaseUI.init();
ctx.subscriptions.push(databaseUI);
@@ -1236,7 +1236,7 @@ async function activateWithInstalledDistribution(
commandRunner(
"codeQL.exportRemoteQueryResults",
async (queryId: string) => {
await exportRemoteQueryResults(qhm, rqm, ctx, queryId);
await exportRemoteQueryResults(qhm, rqm, queryId);
},
),
);
@@ -1251,7 +1251,6 @@ async function activateWithInstalledDistribution(
filterSort?: RepositoriesFilterSortStateWithIds,
) => {
await exportVariantAnalysisResults(
ctx,
variantAnalysisManager,
variantAnalysisId,
filterSort,
@@ -1356,7 +1355,7 @@ async function activateWithInstalledDistribution(
"codeQL.chooseDatabaseGithub",
async (progress: ProgressCallback, token: CancellationToken) => {
const credentials = isCanary()
? await Credentials.initialize(ctx)
? await Credentials.initialize()
: undefined;
await databaseUI.handleChooseDatabaseGithub(
credentials,
@@ -1411,7 +1410,7 @@ async function activateWithInstalledDistribution(
* Credentials for authenticating to GitHub.
* These are used when making API calls.
*/
const credentials = await Credentials.initialize(ctx);
const credentials = await Credentials.initialize();
const octokit = await credentials.getOctokit();
const userInfo = await octokit.users.getAuthenticated();
void showAndLogInformationMessage(

View File

@@ -397,7 +397,7 @@ export class QueryHistoryManager extends DisposableObject {
private readonly variantAnalysisManager: VariantAnalysisManager,
private readonly evalLogViewer: EvalLogViewer,
private readonly queryStorageDir: string,
private readonly ctx: ExtensionContext,
ctx: ExtensionContext,
private readonly queryHistoryConfigListener: QueryHistoryConfig,
private readonly labelProvider: HistoryItemLabelProvider,
private readonly doCompareCallback: (
@@ -633,7 +633,7 @@ export class QueryHistoryManager extends DisposableObject {
}
private getCredentials() {
return Credentials.initialize(this.ctx);
return Credentials.initialize();
}
/**

View File

@@ -1,7 +1,7 @@
import { pathExists } from "fs-extra";
import { EOL } from "os";
import { extname } from "path";
import { CancellationToken, ExtensionContext } from "vscode";
import { CancellationToken } from "vscode";
import { Credentials } from "../authentication";
import { Logger } from "../common";
@@ -26,7 +26,6 @@ export class AnalysesResultsManager {
private readonly analysesResults: Map<string, AnalysisResults[]>;
constructor(
private readonly ctx: ExtensionContext,
private readonly cliServer: CodeQLCliServer,
readonly storagePath: string,
private readonly logger: Logger,
@@ -43,7 +42,7 @@ export class AnalysesResultsManager {
return;
}
const credentials = await Credentials.initialize(this.ctx);
const credentials = await Credentials.initialize();
void this.logger.log(
`Downloading and processing results for ${analysisSummary.nwo}`,
@@ -77,7 +76,7 @@ export class AnalysesResultsManager {
(x) => !this.isAnalysisInMemory(x),
);
const credentials = await Credentials.initialize(this.ctx);
const credentials = await Credentials.initialize();
void this.logger.log("Downloading and processing analyses results");

View File

@@ -4,7 +4,6 @@ import { ensureDir, writeFile } from "fs-extra";
import {
commands,
CancellationToken,
ExtensionContext,
Uri,
ViewColumn,
window,
@@ -74,7 +73,6 @@ export async function exportSelectedRemoteQueryResults(
export async function exportRemoteQueryResults(
queryHistoryManager: QueryHistoryManager,
remoteQueriesManager: RemoteQueriesManager,
ctx: ExtensionContext,
queryId: string,
): Promise<void> {
const queryHistoryItem = queryHistoryManager.getRemoteQueryById(queryId);
@@ -107,7 +105,6 @@ export async function exportRemoteQueryResults(
const exportedResultsDirectory = join(exportDirectory, "exported-results");
await exportRemoteQueryAnalysisResults(
ctx,
exportedResultsDirectory,
query,
analysesResults,
@@ -116,7 +113,6 @@ export async function exportRemoteQueryResults(
}
export async function exportRemoteQueryAnalysisResults(
ctx: ExtensionContext,
exportedResultsPath: string,
query: RemoteQuery,
analysesResults: AnalysisResults[],
@@ -126,7 +122,6 @@ export async function exportRemoteQueryAnalysisResults(
const markdownFiles = generateMarkdown(query, analysesResults, exportFormat);
await exportResults(
ctx,
exportedResultsPath,
description,
markdownFiles,
@@ -141,7 +136,6 @@ const MAX_VARIANT_ANALYSIS_EXPORT_PROGRESS_STEPS = 2;
* The user is prompted to select the export format.
*/
export async function exportVariantAnalysisResults(
ctx: ExtensionContext,
variantAnalysisManager: VariantAnalysisManager,
variantAnalysisId: number,
filterSort: RepositoriesFilterSortStateWithIds | undefined,
@@ -239,7 +233,6 @@ export async function exportVariantAnalysisResults(
);
await exportVariantAnalysisAnalysisResults(
ctx,
exportedResultsDirectory,
variantAnalysis,
getAnalysesResults(),
@@ -251,7 +244,6 @@ export async function exportVariantAnalysisResults(
}
export async function exportVariantAnalysisAnalysisResults(
ctx: ExtensionContext,
exportedResultsPath: string,
variantAnalysis: VariantAnalysis,
analysesResults: AsyncIterable<
@@ -284,7 +276,6 @@ export async function exportVariantAnalysisAnalysisResults(
);
await exportResults(
ctx,
exportedResultsPath,
description,
markdownFiles,
@@ -328,7 +319,6 @@ async function determineExportFormat(): Promise<"gist" | "local" | undefined> {
}
export async function exportResults(
ctx: ExtensionContext,
exportedResultsPath: string,
description: string,
markdownFiles: MarkdownFile[],
@@ -341,7 +331,7 @@ export async function exportResults(
}
if (exportFormat === "gist") {
await exportToGist(ctx, description, markdownFiles, progress, token);
await exportToGist(description, markdownFiles, progress, token);
} else if (exportFormat === "local") {
await exportToLocalMarkdown(
exportedResultsPath,
@@ -353,7 +343,6 @@ export async function exportResults(
}
export async function exportToGist(
ctx: ExtensionContext,
description: string,
markdownFiles: MarkdownFile[],
progress?: ProgressCallback,
@@ -365,7 +354,7 @@ export async function exportToGist(
message: "Creating Gist",
});
const credentials = await Credentials.initialize(ctx);
const credentials = await Credentials.initialize();
if (token?.isCancellationRequested) {
throw new UserCancellationException("Cancelled");

View File

@@ -81,20 +81,19 @@ export class RemoteQueriesManager extends DisposableObject {
private readonly view: RemoteQueriesView;
constructor(
private readonly ctx: ExtensionContext,
ctx: ExtensionContext,
private readonly cliServer: CodeQLCliServer,
private readonly storagePath: string,
logger: Logger,
) {
super();
this.analysesResultsManager = new AnalysesResultsManager(
ctx,
cliServer,
storagePath,
logger,
);
this.view = new RemoteQueriesView(ctx, logger, this.analysesResultsManager);
this.remoteQueriesMonitor = new RemoteQueriesMonitor(ctx, logger);
this.remoteQueriesMonitor = new RemoteQueriesMonitor(logger);
this.remoteQueryAddedEventEmitter = this.push(
new EventEmitter<NewQueryEvent>(),
@@ -160,7 +159,7 @@ export class RemoteQueriesManager extends DisposableObject {
progress: ProgressCallback,
token: CancellationToken,
): Promise<void> {
const credentials = await Credentials.initialize(this.ctx);
const credentials = await Credentials.initialize();
const {
actionBranch,
@@ -218,7 +217,7 @@ export class RemoteQueriesManager extends DisposableObject {
remoteQuery: RemoteQuery,
cancellationToken: CancellationToken,
): Promise<void> {
const credentials = await Credentials.initialize(this.ctx);
const credentials = await Credentials.initialize();
const queryWorkflowResult = await this.remoteQueriesMonitor.monitorQuery(
remoteQuery,

View File

@@ -16,20 +16,13 @@ export class RemoteQueriesMonitor {
private static readonly maxAttemptCount = 17280;
private static readonly sleepTime = 5000;
constructor(
private readonly extensionContext: vscode.ExtensionContext,
private readonly logger: Logger,
) {}
constructor(private readonly logger: Logger) {}
public async monitorQuery(
remoteQuery: RemoteQuery,
cancellationToken: vscode.CancellationToken,
): Promise<RemoteQueryWorkflowResult> {
const credentials = await Credentials.initialize(this.extensionContext);
if (!credentials) {
throw Error("Error authenticating with GitHub");
}
const credentials = await Credentials.initialize();
let attemptCount = 0;

View File

@@ -106,7 +106,6 @@ export class VariantAnalysisManager
super();
this.variantAnalysisMonitor = this.push(
new VariantAnalysisMonitor(
ctx,
this.shouldCancelMonitorVariantAnalysis.bind(this),
),
);
@@ -125,7 +124,7 @@ export class VariantAnalysisManager
progress: ProgressCallback,
token: CancellationToken,
): Promise<void> {
const credentials = await Credentials.initialize(this.ctx);
const credentials = await Credentials.initialize();
const {
actionBranch,
@@ -479,10 +478,7 @@ export class VariantAnalysisManager
await this.onRepoStateUpdated(variantAnalysis.id, repoState);
const credentials = await Credentials.initialize(this.ctx);
if (!credentials) {
throw Error("Error authenticating with GitHub");
}
const credentials = await Credentials.initialize();
if (cancellationToken && cancellationToken.isCancellationRequested) {
repoState.downloadStatus =
@@ -580,10 +576,7 @@ export class VariantAnalysisManager
);
}
const credentials = await Credentials.initialize(this.ctx);
if (!credentials) {
throw Error("Error authenticating with GitHub");
}
const credentials = await Credentials.initialize();
void showAndLogInformationMessage(
"Cancelling variant analysis. This may take a while.",

View File

@@ -1,9 +1,4 @@
import {
CancellationToken,
commands,
EventEmitter,
ExtensionContext,
} from "vscode";
import { CancellationToken, commands, EventEmitter } from "vscode";
import { Credentials } from "../authentication";
import { getVariantAnalysis } from "./gh-api/gh-api-client";
@@ -32,7 +27,6 @@ export class VariantAnalysisMonitor extends DisposableObject {
readonly onVariantAnalysisChange = this._onVariantAnalysisChange.event;
constructor(
private readonly extensionContext: ExtensionContext,
private readonly shouldCancelMonitor: (
variantAnalysisId: number,
) => Promise<boolean>,
@@ -44,10 +38,7 @@ export class VariantAnalysisMonitor extends DisposableObject {
variantAnalysis: VariantAnalysis,
cancellationToken: CancellationToken,
): Promise<void> {
const credentials = await Credentials.initialize(this.extensionContext);
if (!credentials) {
throw Error("Error authenticating with GitHub");
}
const credentials = await Credentials.initialize();
let attemptCount = 0;
const scannedReposDownloaded: number[] = [];

View File

@@ -142,6 +142,14 @@ describe("Variant Analysis Manager", () => {
beforeEach(async () => {
writeFileStub.mockRestore();
const mockCredentials = {
getOctokit: () =>
Promise.resolve({
request: jest.fn(),
}),
} as unknown as Credentials;
jest.spyOn(Credentials, "initialize").mockResolvedValue(mockCredentials);
// Should not have asked for a language
showQuickPickSpy = jest
.spyOn(window, "showQuickPick")
@@ -367,27 +375,6 @@ describe("Variant Analysis Manager", () => {
});
describe("autoDownloadVariantAnalysisResult", () => {
describe("when credentials are invalid", () => {
beforeEach(async () => {
jest
.spyOn(Credentials, "initialize")
.mockResolvedValue(undefined as unknown as Credentials);
});
it("should return early if credentials are wrong", async () => {
try {
await variantAnalysisManager.autoDownloadVariantAnalysisResult(
scannedRepos[0],
variantAnalysis,
cancellationTokenSource.token,
);
} catch (error: any) {
expect(error.message).toBe("Error authenticating with GitHub");
}
});
});
describe("when credentials are valid", () => {
let arrayBuffer: ArrayBuffer;
let getVariantAnalysisRepoStub: jest.SpiedFunction<
@@ -404,9 +391,7 @@ describe("Variant Analysis Manager", () => {
request: jest.fn(),
}),
} as unknown as Credentials;
jest
.spyOn(Credentials, "initialize")
.mockResolvedValue(mockCredentials);
jest.spyOn(Credentials, "initialize").mockResolvedValue(mockCredentials);
const sourceFilePath = join(
__dirname,
@@ -513,11 +498,7 @@ describe("Variant Analysis Manager", () => {
);
expect(outputJsonStub).toHaveBeenCalledWith(
join(
storagePath,
variantAnalysis.id.toString(),
"repo_states.json",
),
join(storagePath, variantAnalysis.id.toString(), "repo_states.json"),
{
[scannedRepos[0].repository.id]: {
repositoryId: scannedRepos[0].repository.id,
@@ -566,11 +547,7 @@ describe("Variant Analysis Manager", () => {
);
expect(outputJsonStub).toHaveBeenCalledWith(
join(
storagePath,
variantAnalysis.id.toString(),
"repo_states.json",
),
join(storagePath, variantAnalysis.id.toString(), "repo_states.json"),
{
[scannedRepos[0].repository.id]: {
repositoryId: scannedRepos[0].repository.id,
@@ -608,11 +585,7 @@ describe("Variant Analysis Manager", () => {
);
expect(outputJsonStub).toHaveBeenCalledWith(
join(
storagePath,
variantAnalysis.id.toString(),
"repo_states.json",
),
join(storagePath, variantAnalysis.id.toString(), "repo_states.json"),
{
[scannedRepos[0].repository.id]: {
repositoryId: scannedRepos[0].repository.id,
@@ -642,20 +615,14 @@ describe("Variant Analysis Manager", () => {
},
});
await variantAnalysisManager.rehydrateVariantAnalysis(
variantAnalysis,
);
await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis);
expect(pathExistsStub).toBeCalledWith(
join(storagePath, variantAnalysis.id.toString()),
);
expect(readJsonStub).toHaveBeenCalledTimes(1);
expect(readJsonStub).toHaveBeenCalledWith(
join(
storagePath,
variantAnalysis.id.toString(),
"repo_states.json",
),
join(storagePath, variantAnalysis.id.toString(), "repo_states.json"),
);
pathExistsStub.mockRestore();
@@ -667,11 +634,7 @@ describe("Variant Analysis Manager", () => {
);
expect(outputJsonStub).toHaveBeenCalledWith(
join(
storagePath,
variantAnalysis.id.toString(),
"repo_states.json",
),
join(storagePath, variantAnalysis.id.toString(), "repo_states.json"),
{
[scannedRepos[1].repository.id]: {
repositoryId: scannedRepos[1].repository.id,
@@ -701,7 +664,6 @@ describe("Variant Analysis Manager", () => {
}
});
});
});
describe("enqueueDownload", () => {
beforeEach(async () => {
@@ -876,6 +838,8 @@ describe("Variant Analysis Manager", () => {
let variantAnalysisStorageLocation: string;
let mockCredentials: Credentials;
beforeEach(async () => {
variantAnalysis = createMockVariantAnalysis({});
@@ -889,43 +853,18 @@ describe("Variant Analysis Manager", () => {
);
await createTimestampFile(variantAnalysisStorageLocation);
await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis);
});
afterEach(() => {
fs.rmSync(variantAnalysisStorageLocation, { recursive: true });
});
describe("when the credentials are invalid", () => {
beforeEach(async () => {
jest
.spyOn(Credentials, "initialize")
.mockResolvedValue(undefined as unknown as Credentials);
});
it("should return early", async () => {
try {
await variantAnalysisManager.cancelVariantAnalysis(
variantAnalysis.id,
);
} catch (error: any) {
expect(error.message).toBe("Error authenticating with GitHub");
}
});
});
describe("when the credentials are valid", () => {
let mockCredentials: Credentials;
beforeEach(async () => {
mockCredentials = {
getOctokit: () =>
Promise.resolve({
request: jest.fn(),
}),
} as unknown as Credentials;
jest
.spyOn(Credentials, "initialize")
.mockResolvedValue(mockCredentials);
jest.spyOn(Credentials, "initialize").mockResolvedValue(mockCredentials);
});
afterEach(() => {
fs.rmSync(variantAnalysisStorageLocation, { recursive: true });
});
it("should return early if the variant analysis is not found", async () => {
@@ -947,9 +886,7 @@ describe("Variant Analysis Manager", () => {
});
try {
await variantAnalysisManager.cancelVariantAnalysis(
variantAnalysis.id,
);
await variantAnalysisManager.cancelVariantAnalysis(variantAnalysis.id);
} catch (error: any) {
expect(error.message).toBe(
`No workflow run id for variant analysis with id: ${variantAnalysis.id}`,
@@ -966,7 +903,6 @@ describe("Variant Analysis Manager", () => {
);
});
});
});
describe("copyRepoListToClipboard", () => {
let variantAnalysis: VariantAnalysis;

View File

@@ -61,10 +61,7 @@ describe("Variant Analysis Monitor", () => {
"GitHub.vscode-codeql",
)!
.activate();
variantAnalysisMonitor = new VariantAnalysisMonitor(
extension.ctx,
shouldCancelMonitor,
);
variantAnalysisMonitor = new VariantAnalysisMonitor(shouldCancelMonitor);
variantAnalysisMonitor.onVariantAnalysisChange(onVariantAnalysisChangeSpy);
variantAnalysisManager = extension.variantAnalysisManager;
@@ -77,29 +74,7 @@ describe("Variant Analysis Monitor", () => {
.mockRejectedValue(new Error("Not mocked"));
limitNumberOfAttemptsToMonitor();
});
describe("when credentials are invalid", () => {
beforeEach(async () => {
jest
.spyOn(Credentials, "initialize")
.mockResolvedValue(undefined as unknown as Credentials);
});
it("should return early if credentials are wrong", async () => {
try {
await variantAnalysisMonitor.monitorVariantAnalysis(
variantAnalysis,
cancellationTokenSource.token,
);
} catch (error: any) {
expect(error.message).toBe("Error authenticating with GitHub");
}
});
});
describe("when credentials are valid", () => {
beforeEach(async () => {
const mockCredentials = {
getOctokit: () =>
Promise.resolve({
@@ -333,7 +308,6 @@ describe("Variant Analysis Monitor", () => {
});
});
});
});
function limitNumberOfAttemptsToMonitor() {
VariantAnalysisMonitor.maxAttemptCount = 3;

View File

@@ -1,6 +1,5 @@
import { join } from "path";
import { readFile } from "fs-extra";
import { createMockExtensionContext } from "../index";
import { Credentials } from "../../../authentication";
import * as markdownGenerator from "../../../remote-queries/remote-queries-markdown-generation";
import * as ghApiClient from "../../../remote-queries/gh-api/gh-api-client";
@@ -20,7 +19,6 @@ describe("export results", () => {
.spyOn(ghApiClient, "createGist")
.mockResolvedValue(undefined);
const ctx = createMockExtensionContext();
const query = JSON.parse(
await readFile(
join(
@@ -41,7 +39,6 @@ describe("export results", () => {
);
await exportRemoteQueryAnalysisResults(
ctx,
"",
query,
analysesResults,

View File

@@ -279,7 +279,6 @@ describe("Remote queries and query history manager", () => {
jest.spyOn(Credentials, "initialize").mockResolvedValue(mockCredentials);
arm = new AnalysesResultsManager(
{} as ExtensionContext,
mockCliServer,
join(STORAGE_DIR, "queries"),
mockLogger,