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
Time machine server.js #1589
Time machine server.js #1589
Conversation
web/server.js
Outdated
@@ -158,9 +159,28 @@ app.get('/', (req, res) => { | |||
} | |||
const { locale } = res; | |||
const fullUrl = 'https://www.electricitymap.org' + req.originalUrl; | |||
|
|||
// basic auth and time machine cookie if we're in the back in time container |
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.
Can we change the comment to something even a contributor could understand
Something along the lines of:
// Basic auth for premium access
web/server.js
Outdated
if (process.env['BASIC_AUTH_USERNAME;']){ | ||
let user = auth(req); | ||
let authorized = false; | ||
if (user){ |
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.
Style: linter probably requires space between ) and {
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.
Some comments. Also, can you try to eslint the code? It should already be part of the devDependencies.
web/server.js
Outdated
|
||
// basic auth and time machine cookie if we're in the back in time container | ||
if (process.env['BASIC_AUTH_USERNAME;']){ | ||
let user = auth(req); |
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.
Linter will probably say it needs const
instead.
web/server.js
Outdated
@@ -158,9 +159,28 @@ app.get('/', (req, res) => { | |||
} | |||
const { locale } = res; | |||
const fullUrl = 'https://www.electricitymap.org' + req.originalUrl; | |||
|
|||
// basic auth and time machine cookie if we're in the back in time container | |||
if (process.env['BASIC_AUTH_USERNAME;']){ |
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.
why does the env variable name have a semicolon at the end?
web/server.js
Outdated
let user = auth(req); | ||
let authorized = false; | ||
if (user){ | ||
let ix = process.env['BASIC_AUTH_USERNAMES'].split(';').indexOf(user.name); |
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.
More traditional to split using a comma
web/server.js
Outdated
let user = auth(req); | ||
let authorized = false; | ||
if (user){ | ||
let ix = process.env['BASIC_AUTH_USERNAMES'].split(';').indexOf(user.name); |
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.
You can be more explicit by naming a variable hasMatchingUserName
and directly do the !== -1
comparaison here.
web/server.js
Outdated
let authorized = false; | ||
if (user){ | ||
let ix = process.env['BASIC_AUTH_USERNAMES'].split(';').indexOf(user.name); | ||
if (ix !== -1 && user.pass === process.env['BASIC_AUTH_PASSWORDS'].split(';')[ix]){ |
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.
It's a tad dangerous because this forces us to have two lists (one for users, one for passwords), and is therefore prone to errors (if you offset one item for example).
A better way would be to serialise auth using user1:pass1,user2:pass2
...
web/server.js
Outdated
} | ||
if (!authorized){ | ||
res.statusCode = 401; | ||
res.setHeader('WWW-Authenticate', 'Basic realm="example"'); |
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.
example?
for the premium subdomain, adds basic auth and replies with a
set-cookie
header so that back in time works