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

support ldap auth #4751

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
191 changes: 191 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@
"jwt-decode": "~3.1.2",
"kafkajs": "^2.2.4",
"knex": "^2.4.2",
"ldapjs": "^3.0.7",
"limiter": "~2.1.0",
"liquidjs": "^10.7.0",
"mitt": "~3.0.1",
Expand Down
76 changes: 61 additions & 15 deletions server/auth.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const { log } = require("../src/util");
const { loginRateLimiter, apiRateLimiter } = require("./rate-limiter");
const { Settings } = require("./settings");
const dayjs = require("dayjs");
const ldap = require("ldapjs");

/**
* Login to web app
Expand All @@ -14,26 +15,71 @@ const dayjs = require("dayjs");
* @returns {Promise<(Bean|null)>} User or null if login failed
*/
exports.login = async function (username, password) {

if (typeof username !== "string" || typeof password !== "string") {
return null;
}

let user = await R.findOne("user", " username = ? AND active = 1 ", [
username,
]);

if (user && passwordHash.verify(password, user.password)) {
// Upgrade the hash to bcrypt
if (passwordHash.needRehash(user.password)) {
await R.exec("UPDATE `user` SET password = ? WHERE id = ? ", [
passwordHash.generate(password),
user.id,
]);
if (process.env.AUTH_METHOD === "LDAP") {
let ldapAuthSuccess = false;
await new Promise((resolve, reject) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please extract the ldap parts of this implementation into a function.
This method is too long already and adding more will not aid in readability ^^

const client = ldap.createClient({
url: [ process.env.LDAP_ADDRESS ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

On the parameters to this function (ldapjs' docs):

  • url does allow multiple LDAP servers.
    Should we support this as well?
  • there are a few options regarding timouts. I think having timeouts for LDAP operations might be a good call.
  • Should we add the reconnection parameter?

});
client.on("connectError", (err) => {
log.error("auth", "connecting ldap server failed");
Copy link
Collaborator

Choose a reason for hiding this comment

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

include the error message if it contains context aiding in debugging

resolve();
Copy link
Collaborator

Choose a reason for hiding this comment

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

please refactor this to reject/resolve with the value of ldapAuthSuccess.
This way, future PRs can't mess with the logic and introduce potential security flaws. (this logic is fine, but lets not leave a footgun lying around ^^)

});
client.bind(process.env.LDAP_UID + "=" + username + "," + process.env.LDAP_BASE_DN,
Copy link
Collaborator

Choose a reason for hiding this comment

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

please include debug logging what query is being executed

password, (err) => {
if (err) {
log.warn("auth", "ldap authentication failed for user: " + username);
} else {
log.info("auth", "ldap authentication succeeded for user: " + username);
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets use a template for better readability

Suggested change
log.warn("auth", "ldap authentication failed for user: " + username);
} else {
log.info("auth", "ldap authentication succeeded for user: " + username);
log.info("auth", `ldap failed for ${username} because ${err}`);
} else {
log.info("auth", `${username} successfully logged in via ldap`);

ldapAuthSuccess = true;
}
resolve();
});
});
if (!ldapAuthSuccess) {
return null;
}
let user = await R.findOne("user", " username = ? AND active = 1 ", [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please extract the below parts into functions.
This is hard to comprehend => easy to make mistakes. Lets not risk this in security critical code ^^

username,
]);
if (user) {
if (passwordHash.needRehash(user.password)) {
await R.exec("UPDATE `user` SET password = ? WHERE id = ? ", [
passwordHash.generate(password),
user.id,
]);
}
return user;
} else {
log.info("auth", "user: " + username + "not exists in db, create it");
let user = R.dispense("user");
user.username = username;
user.password = passwordHash.generate(password);
await R.store(user);
return await R.findOne("user", " username = ? AND active = 1 ", [ username ]);
}
} else {
let user = await R.findOne("user", " username = ? AND active = 1 ", [
username,
]);

if (user && passwordHash.verify(password, user.password)) {
// Upgrade the hash to bcrypt
if (passwordHash.needRehash(user.password)) {
await R.exec("UPDATE `user` SET password = ? WHERE id = ? ", [
passwordHash.generate(password),
user.id,
]);
}
return user;
}
return user;
}

return null;
return null;
}
};

/**
Expand Down
Loading