Skip to content
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

feat: add customization options for the login page #5633

Merged
merged 8 commits into from
Oct 13, 2022
4 changes: 2 additions & 2 deletions src/browser/pages/login.html
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
http-equiv="Content-Security-Policy"
content="style-src 'self'; script-src 'self' 'unsafe-inline'; manifest-src 'self'; img-src 'self' data:; font-src 'self' data:;"
/>
<title>code-server login</title>
<title>{{APP_NAME}} login</title>
<link rel="icon" href="{{CS_STATIC_BASE}}/src/browser/media/favicon-dark-support.svg" />
<link rel="alternate icon" href="{{CS_STATIC_BASE}}/src/browser/media/favicon.ico" />
<link rel="manifest" href="{{BASE}}/manifest.json" crossorigin="use-credentials" />
Expand All @@ -24,7 +24,7 @@
<div class="center-container">
<div class="card-box">
<div class="header">
<h1 class="main">Welcome to code-server</h1>
<h1 class="main">{{WELCOME_TEXT}}</h1>
<div class="sub">Please log in below. {{PASSWORD_MSG}}</div>
</div>
<div class="content">
Expand Down
13 changes: 12 additions & 1 deletion src/node/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ export interface UserProvidedArgs extends UserProvidedCodeArgs {
"ignore-last-opened"?: boolean
link?: OptionalString
verbose?: boolean
"app-name"?: string
"welcome-text"?: string
/* Positional arguments. */
_?: string[]
}
Expand Down Expand Up @@ -238,7 +240,16 @@ export const options: Options<Required<UserProvidedArgs>> = {

log: { type: LogLevel },
verbose: { type: "boolean", short: "vvv", description: "Enable verbose logging." },

"app-name": {
type: "string",
short: "an",
description: "The name to use in branding. Will be shown in titlebar and welcome message",
},
"welcome-text": {
type: "string",
short: "w",
description: "Text to show on login page",
},
link: {
type: OptionalString,
description: `
Expand Down
4 changes: 4 additions & 0 deletions src/node/routes/login.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ export class RateLimiter {

const getRoot = async (req: Request, error?: Error): Promise<string> => {
const content = await fs.readFile(path.join(rootPath, "src/browser/pages/login.html"), "utf8")
const appName = req.args["app-name"] || "code-server"
const welcomeText = req.args["welcome-text"] || `Welcome to ${appName}`
let passwordMsg = `Check the config file at ${humanPath(os.homedir(), req.args.config)} for the password.`
if (req.args.usingEnvPassword) {
passwordMsg = "Password was set from $PASSWORD."
Expand All @@ -38,6 +40,8 @@ const getRoot = async (req: Request, error?: Error): Promise<string> => {
return replaceTemplates(
req,
content
.replace(/{{APP_NAME}}/g, appName)
.replace(/{{WELCOME_TEXT}}/g, welcomeText)
.replace(/{{PASSWORD_MSG}}/g, passwordMsg)
.replace(/{{ERROR}}/, error ? `<div class="error">${escapeHtml(error.message)}</div>` : ""),
)
Expand Down
4 changes: 4 additions & 0 deletions test/unit/node/cli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ describe("parser", () => {

"1",
"--verbose",
["--app-name", "custom instance name"],
["--welcome-text", "welcome to code"],
"2",

["--locale", "ja"],
Expand Down Expand Up @@ -123,6 +125,8 @@ describe("parser", () => {
socket: path.resolve("mumble"),
"socket-mode": "777",
verbose: true,
"app-name": "custom instance name",
"welcome-text": "welcome to code",
version: true,
"bind-addr": "192.169.0.1:8080",
})
Expand Down
46 changes: 46 additions & 0 deletions test/unit/node/routes/login.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,5 +92,51 @@ describe("login", () => {

expect(htmlContent).toContain("Incorrect password")
})

it("should return correct app-name", async () => {
process.env.PASSWORD = previousEnvPassword
const appName = "testnäme"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: same as here

Suggested change
const appName = "testnäme"
const appName = "testname"

const codeServer = await integration.setup([`--app-name=${appName}`], "")
const resp = await codeServer.fetch("/login", { method: "GET" })

const htmlContent = await resp.text()
expect(resp.status).toBe(200)
expect(htmlContent).toContain(`${appName}</h1>`)
expect(htmlContent).toContain(`<title>${appName} login</title>`)
})

it("should return correct app-name when unset", async () => {
process.env.PASSWORD = previousEnvPassword
const appName = "code-server"
const codeServer = await integration.setup([], "")
const resp = await codeServer.fetch("/login", { method: "GET" })

const htmlContent = await resp.text()
expect(resp.status).toBe(200)
expect(htmlContent).toContain(`${appName}</h1>`)
expect(htmlContent).toContain(`<title>${appName} login</title>`)
})

it("should return correct welcome text", async () => {
process.env.PASSWORD = previousEnvPassword
const welcomeText = "Welcome to your code workspace! öäü🔐"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should we use characters without accent marks in case they're rendered differently on someone's machine?

Suggested change
const welcomeText = "Welcome to your code workspace! öäü🔐"
const welcomeText = "Welcome to your code workspace!"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my idea was to test if there are any implications when using unicode, but I don't know if that should be there

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that could be tested separately if needed but I personally don't think we need to. thoughts @code-asher ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can also merge now and i can fix later if needed. don't want to hold you back :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I don't think it will do any harm, should be ready to merge. I would be glad if you could add the hacktoberfest-accepted label so it counts for https://hacktoberfest.com :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems chill to me 👍

const codeServer = await integration.setup([`--welcome-text=${welcomeText}`], "")
const resp = await codeServer.fetch("/login", { method: "GET" })

const htmlContent = await resp.text()
expect(resp.status).toBe(200)
expect(htmlContent).toContain(welcomeText)
})

it("should return correct welcome text when none is set but app-name is", async () => {
process.env.PASSWORD = previousEnvPassword
const appName = "testnäme"
const codeServer = await integration.setup([`--app-name=${appName}`], "")
const resp = await codeServer.fetch("/login", { method: "GET" })

const htmlContent = await resp.text()
expect(resp.status).toBe(200)
expect(htmlContent).toContain(`Welcome to ${appName}`)
})
})
})