Skip to content

feat: github-auth flag #4926

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

Merged
merged 3 commits into from
Mar 2, 2022
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion src/node/cli.ts
Original file line number Diff line number Diff line change
@@ -87,6 +87,7 @@ export interface UserProvidedArgs {
"locate-extension"?: string[]
"show-versions"?: boolean
category?: string
"github-auth"?: string
}

interface Option<T> {
@@ -205,6 +206,10 @@ const options: Options<Required<UserProvidedArgs>> = {
},
"uninstall-extension": { type: "string[]", description: "Uninstall a VS Code extension by id." },
"show-versions": { type: "boolean", description: "Show VS Code extension versions." },
"github-auth": {
type: "string",
description: "GitHub authentication token (can only be passed in via $GITHUB_TOKEN or the config file).",
},
"proxy-domain": { type: "string[]", description: "Domain used for proxying ports." },
"ignore-last-opened": {
type: "boolean",
@@ -336,6 +341,10 @@ export const parse = (
throw new Error("--hashed-password can only be set in the config file or passed in via $HASHED_PASSWORD")
}

if (key === "github-auth" && !opts?.configFile) {
throw new Error("--github-auth can only be set in the config file or passed in via $GITHUB_TOKEN")
}

const option = options[key]
if (option.type === "boolean") {
;(args[key] as boolean) = true
@@ -409,7 +418,12 @@ export const parse = (

logger.debug(() => [
`parsed ${opts?.configFile ? "config" : "command line"}`,
field("args", { ...args, password: undefined }),
field("args", {
...args,
password: args.password ? "<redacted>" : undefined,
"hashed-password": args["hashed-password"] ? "<redacted>" : undefined,
"github-auth": args["github-auth"] ? "<redacted>" : undefined,
}),
])

return args
@@ -530,9 +544,14 @@ export async function setDefaults(cliArgs: UserProvidedArgs, configArgs?: Config
usingEnvPassword = false
}

if (process.env.GITHUB_TOKEN) {
args["github-auth"] = process.env.GITHUB_TOKEN
}

// Ensure they're not readable by child processes.
delete process.env.PASSWORD
delete process.env.HASHED_PASSWORD
delete process.env.GITHUB_TOKEN

// Filter duplicate proxy domains and remove any leading `*.`.
const proxyDomains = new Set((args["proxy-domain"] || []).map((d) => d.replace(/^\*\./, "")))
3 changes: 2 additions & 1 deletion test/e2e/baseFixture.ts
Original file line number Diff line number Diff line change
@@ -13,11 +13,12 @@ export const describe = (
name: string,
includeCredentials: boolean,
codeServerArgs: string[],
codeServerEnv: NodeJS.ProcessEnv,
fn: (codeServer: CodeServer) => void,
) => {
test.describe(name, () => {
// This will spawn on demand so nothing is necessary on before.
const codeServer = new CodeServer(name, codeServerArgs)
const codeServer = new CodeServer(name, codeServerArgs, codeServerEnv)

// Kill code-server after the suite has ended. This may happen even without
// doing it explicitly but it seems prudent to be sure.
2 changes: 1 addition & 1 deletion test/e2e/codeServer.test.ts
Original file line number Diff line number Diff line change
@@ -2,7 +2,7 @@ import { promises as fs } from "fs"
import * as path from "path"
import { describe, test, expect } from "./baseFixture"

describe("CodeServer", true, [], () => {
describe("CodeServer", true, [], {}, () => {
test("should navigate to home page", async ({ codeServerPage }) => {
// We navigate codeServer before each test
// and we start the test with a storage state
7 changes: 5 additions & 2 deletions test/e2e/extensions.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as path from "path"
import { describe, test } from "./baseFixture"

function runTestExtensionTests() {
@@ -11,10 +12,12 @@ function runTestExtensionTests() {
})
}

describe("Extensions", true, [], () => {
const flags = ["--extensions-dir", path.join(__dirname, "./extensions")]

describe("Extensions", true, flags, {}, () => {
runTestExtensionTests()
})

describe("Extensions with --cert", true, ["--cert"], () => {
describe("Extensions with --cert", true, [...flags, "--cert"], {}, () => {
runTestExtensionTests()
})
38 changes: 38 additions & 0 deletions test/e2e/github.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { test as base } from "@playwright/test"
import { describe, expect, test } from "./baseFixture"

if (process.env.GITHUB_TOKEN) {
describe("GitHub token", true, [], {}, () => {
test("should be logged in to pull requests extension", async ({ codeServerPage }) => {
await codeServerPage.exec("git init")
await codeServerPage.exec("git remote add origin https://github.com/coder/code-server")
await codeServerPage.installExtension("GitHub.vscode-pull-request-github")
await codeServerPage.executeCommandViaMenus("View: Show Github")
await codeServerPage.page.click("text=Sign in")
await codeServerPage.page.click("text=Allow")
// It should ask to select an account, one of which will be the one we
// pre-injected.
expect(await codeServerPage.page.isVisible("text=Select an account")).toBe(false)
})
})

describe("No GitHub token", true, [], { GITHUB_TOKEN: "" }, () => {
test("should not be logged in to pull requests extension", async ({ codeServerPage }) => {
await codeServerPage.exec("git init")
await codeServerPage.exec("git remote add origin https://github.com/coder/code-server")
await codeServerPage.installExtension("GitHub.vscode-pull-request-github")
await codeServerPage.executeCommandViaMenus("View: Show Github")
await codeServerPage.page.click("text=Sign in")
await codeServerPage.page.click("text=Allow")
// Since there is no account it will ask directly for the token (because
// we are on localhost; otherwise it would initiate the oauth flow).
expect(await codeServerPage.page.isVisible("text=GitHub Personal Access Token")).toBe(false)
})
})
} else {
base.describe("GitHub token", () => {
base.skip("skipped because GITHUB_TOKEN is not set", () => {
// Playwright will not show this without a function.
})
})
}
2 changes: 1 addition & 1 deletion test/e2e/globalSetup.test.ts
Original file line number Diff line number Diff line change
@@ -2,7 +2,7 @@ import { describe, test, expect } from "./baseFixture"

// This test is to make sure the globalSetup works as expected
// meaning globalSetup ran and stored the storageState
describe("globalSetup", true, [], () => {
describe("globalSetup", true, [], {}, () => {
test("should keep us logged in using the storageState", async ({ codeServerPage }) => {
// Make sure the editor actually loaded
expect(await codeServerPage.isEditorVisible()).toBe(true)
2 changes: 1 addition & 1 deletion test/e2e/login.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { PASSWORD } from "../utils/constants"
import { describe, test, expect } from "./baseFixture"

describe("login", false, [], () => {
describe("login", false, [], {}, () => {
test("should see the login page", async ({ codeServerPage }) => {
// It should send us to the login page
expect(await codeServerPage.page.title()).toBe("code-server login")
2 changes: 1 addition & 1 deletion test/e2e/logout.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// NOTE@jsjoeio commenting out until we can figure out what's wrong
// import { describe, test, expect } from "./baseFixture"

// describe("logout", true, [], () => {
// describe("logout", true, [], {}, () => {
// test("should be able logout", async ({ codeServerPage }) => {
// // Recommended by Playwright for async navigation
// // https://github.com/microsoft/playwright/issues/1987#issuecomment-620182151
32 changes: 29 additions & 3 deletions test/e2e/models/CodeServer.ts
Original file line number Diff line number Diff line change
@@ -3,6 +3,7 @@ import * as cp from "child_process"
import { promises as fs } from "fs"
import * as path from "path"
import { Page } from "playwright"
import util from "util"
import { logError, plural } from "../../../src/common/util"
import { onLine } from "../../../src/node/util"
import { PASSWORD, workspaceDir } from "../../utils/constants"
@@ -39,7 +40,11 @@ export class CodeServer {
private closed = false
private _workspaceDir: Promise<string> | undefined

constructor(name: string, private readonly codeServerArgs: string[]) {
constructor(
name: string,
private readonly codeServerArgs: string[],
private readonly codeServerEnv: NodeJS.ProcessEnv,
) {
this.logger = logger.named(name)
}

@@ -96,6 +101,8 @@ export class CodeServer {
"node",
[
process.env.CODE_SERVER_TEST_ENTRY || ".",
"--extensions-dir",
path.join(dir, "extensions"),
...this.codeServerArgs,
// Using port zero will spawn on a random port.
"--bind-addr",
@@ -107,15 +114,14 @@ export class CodeServer {
path.join(dir, "config.yaml"),
"--user-data-dir",
dir,
"--extensions-dir",
path.join(__dirname, "../extensions"),
// The last argument is the workspace to open.
dir,
],
{
cwd: path.join(__dirname, "../../.."),
env: {
...process.env,
...this.codeServerEnv,
PASSWORD,
},
},
@@ -462,4 +468,24 @@ export class CodeServerPage {
await this.reloadUntilEditorIsReady()
}
}

/**
* Execute a command in t root of the instance's workspace directory.
*/
async exec(command: string): Promise<void> {
await util.promisify(cp.exec)(command, {
cwd: await this.workspaceDir,
})
}

/**
* Install an extension by ID to the instance's temporary extension
* directory.
*/
async installExtension(id: string): Promise<void> {
const dir = path.join(await this.workspaceDir, "extensions")
await util.promisify(cp.exec)(`node . --install-extension ${id} --extensions-dir ${dir}`, {
cwd: path.join(__dirname, "../../.."),
})
}
}
2 changes: 1 addition & 1 deletion test/e2e/openHelpAbout.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { describe, test, expect } from "./baseFixture"

describe("Open Help > About", true, [], () => {
describe("Open Help > About", true, [], {}, () => {
test("should see code-server version in about dialog", async ({ codeServerPage }) => {
// Open using the menu.
await codeServerPage.navigateMenus(["Help", "About"])
2 changes: 1 addition & 1 deletion test/e2e/terminal.test.ts
Original file line number Diff line number Diff line change
@@ -4,7 +4,7 @@ import util from "util"
import { clean, tmpdir } from "../utils/helpers"
import { describe, expect, test } from "./baseFixture"

describe("Integrated Terminal", true, [], () => {
describe("Integrated Terminal", true, [], {}, () => {
const testName = "integrated-terminal"
test.beforeAll(async () => {
await clean(testName)
24 changes: 24 additions & 0 deletions test/unit/node/cli.test.ts
Original file line number Diff line number Diff line change
@@ -313,6 +313,19 @@ describe("parser", () => {
})
})

it("should use env var github token", async () => {
process.env.GITHUB_TOKEN = "ga-foo"
const args = parse([])
expect(args).toEqual({})

const defaultArgs = await setDefaults(args)
expect(defaultArgs).toEqual({
...defaults,
"github-auth": "ga-foo",
})
expect(process.env.GITHUB_TOKEN).toBe(undefined)
})

it("should error if password passed in", () => {
expect(() => parse(["--password", "supersecret123"])).toThrowError(
"--password can only be set in the config file or passed in via $PASSWORD",
@@ -325,6 +338,12 @@ describe("parser", () => {
)
})

it("should error if github-auth passed in", () => {
expect(() => parse(["--github-auth", "fdas423fs8a"])).toThrowError(
"--github-auth can only be set in the config file or passed in via $GITHUB_TOKEN",
)
})

it("should filter proxy domains", async () => {
const args = parse(["--proxy-domain", "*.coder.com", "--proxy-domain", "coder.com", "--proxy-domain", "coder.org"])
expect(args).toEqual({
@@ -374,6 +393,11 @@ describe("parser", () => {
it("should ignore optional strings set to false", async () => {
expect(parse(["--cert=false"])).toEqual({})
})
it("should use last flag", async () => {
expect(parse(["--port", "8081", "--port", "8082"])).toEqual({
port: 8082,
})
})
})

describe("cli", () => {
2 changes: 1 addition & 1 deletion vendor/package.json
Original file line number Diff line number Diff line change
@@ -7,6 +7,6 @@
"postinstall": "./postinstall.sh"
},
"devDependencies": {
"code-oss-dev": "coder/vscode#6337ee490d16b7dfd8854d22c998f58d6cd21ef5"
"code-oss-dev": "coder/vscode#0fd21e4078ac1dddb26be024f5d4224a4b86da93"
}
}
4 changes: 2 additions & 2 deletions vendor/yarn.lock
Original file line number Diff line number Diff line change
@@ -274,9 +274,9 @@ clone-response@^1.0.2:
dependencies:
mimic-response "^1.0.0"

code-oss-dev@coder/vscode#6337ee490d16b7dfd8854d22c998f58d6cd21ef5:
code-oss-dev@coder/vscode#0fd21e4078ac1dddb26be024f5d4224a4b86da93:
version "1.63.0"
resolved "https://codeload.github.com/coder/vscode/tar.gz/6337ee490d16b7dfd8854d22c998f58d6cd21ef5"
resolved "https://codeload.github.com/coder/vscode/tar.gz/0fd21e4078ac1dddb26be024f5d4224a4b86da93"
dependencies:
"@microsoft/applicationinsights-web" "^2.6.4"
"@parcel/watcher" "2.0.3"