support ldap auth #4751

zhangweiqaz · 2024-05-10T04:56:27Z

Tick the checkbox if you understand [x]:

I have read and understand the pull request rules.

Resolves #5359

Description

add ldap auth support. It will works if below enviroment variables exist

AUTH_METHOD: LDAP
LDAP_ADDRESS: 'ldap://ip:port'
LDAP_BASE_DN: 'cn=users,dc=xxxx,dc=com'
LDAP_UID=uid
The auth DN will be 'uid=username,cn=users,dc=xxxx,dc=com'.
If AUTH_METHOD not equal 'LDAP', it auth with local database user
Fixes #(issue)

Type of change

Please delete any options that are not relevant.

New feature (non-breaking change which adds functionality)

Checklist

My code follows the style guidelines of this project
I ran ESLint and other linters for modified files
I have performed a self-review of my own code and tested it
I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
My changes generates no new warnings
My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.

CommanderStorm

Tip

Please see the part of the contribution guide where we talk about discussing plans beforehand for future PRs.
"Duping work" is not maintainable both for contributors or maintainers ^^

To the PR:

Please rebase this PR on top of Add basic multiple admin users support #3571.
How can this be tested?
=> Please include a short paragraph how to set up a docker/.. environment to test that LDAP is working.
Could you write a first draft how this can be documented? (I have only a basic clue what half of the variables do)

Be aware that our current focus is on getting v2.0 out of the door (see #4500 for further details)
=> reviewing #3571 (as a prerequisite for this PR) might take longer.

CommanderStorm · 2024-05-10T18:00:16Z

server/auth.js

-            ]);
+    if (process.env.AUTH_METHOD === "LDAP") {
+        let ldapAuthSuccess = false;
+        await new Promise((resolve, reject) => {


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 ^^

CommanderStorm · 2024-05-10T18:01:00Z

server/auth.js

+                log.error("auth", "connecting ldap server failed");
+                resolve();
+            });
+            client.bind(process.env.LDAP_UID + "=" + username + "," + process.env.LDAP_BASE_DN,


please include debug logging what query is being executed

CommanderStorm · 2024-05-10T18:04:32Z

server/auth.js

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


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 ^^)

CommanderStorm · 2024-05-10T18:05:42Z

server/auth.js

+                url: [ process.env.LDAP_ADDRESS ]
+            });
+            client.on("connectError", (err) => {
+                log.error("auth", "connecting ldap server failed");


include the error message if it contains context aiding in debugging

CommanderStorm · 2024-05-10T18:12:35Z

server/auth.js

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


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

CommanderStorm · 2024-05-10T18:13:19Z

server/auth.js

+                        log.warn("auth", "ldap authentication failed for user: " + username);
+                    } else {
+                        log.info("auth", "ldap authentication succeeded for user: " + username);


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`);

CommanderStorm · 2024-05-10T18:16:00Z

server/auth.js

+        let ldapAuthSuccess = false;
+        await new Promise((resolve, reject) => {
+            const client = ldap.createClient({
+                url: [ process.env.LDAP_ADDRESS ]


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?

…into ldap-auth

zhangweiqaz · 2024-05-11T06:55:45Z

Thanks for your advises, that help me a lot. Due to #3571 is big change, this PR can be delayed or restructured.
Ldap auth can be test below step:
1、create a temp ldap server
docker run --detach --rm --name openldap
-p 1389:1389
--env LDAP_ADMIN_USERNAME=admin
--env LDAP_ADMIN_PASSWORD=adminpassword
--env LDAP_USERS=customuser
--env LDAP_PASSWORDS=custompassword
--env LDAP_ROOT=dc=example,dc=org
--env LDAP_ADMIN_DN=cn=admin,dc=example,dc=org
bitnami/openldap:latest
2、set the enviroment variables
export AUTH_METHOD="LDAP"
export LDAP_ADDRESS="ldap://ldap_server_ip:1389"
export LDAP_UID="cn"
export LDAP_BASE_DN="ou=users,dc=example,dc=org"
3、run uptime-kuma and use "customuser" and "custompassword" to login

Zaid-maker · 2024-05-11T14:43:13Z

Thanks for your advises, that help me a lot. Due to #3571 is big change, this PR can be delayed or restructured. Ldap auth can be test below step: 1、create a temp ldap server docker run --detach --rm --name openldap -p 1389:1389 --env LDAP_ADMIN_USERNAME=admin --env LDAP_ADMIN_PASSWORD=adminpassword --env LDAP_USERS=customuser --env LDAP_PASSWORDS=custompassword --env LDAP_ROOT=dc=example,dc=org --env LDAP_ADMIN_DN=cn=admin,dc=example,dc=org bitnami/openldap:latest 2、set the enviroment variables export AUTH_METHOD="LDAP" export LDAP_ADDRESS="ldap://ldap_server_ip:1389" export LDAP_UID="cn" export LDAP_BASE_DN="ou=users,dc=example,dc=org" 3、run uptime-kuma and use "customuser" and "custompassword" to login

You can use handy tool to quickly test docker containers (sort of) Dockge

support ldap auth

31bc6e8

CommanderStorm requested changes May 10, 2024

View reviewed changes

zhangwei0181 and others added 4 commits May 11, 2024 06:35

move ldap auth to a function

a2140d4

Merge branch 'louislam:master' into ldap-auth

dec7ef5

remove wrong line

90130c9

Merge branch 'ldap-auth' of https://github.com/zhangweiqaz/uptime-kuma …

da72713

…into ldap-auth

CommanderStorm mentioned this pull request May 14, 2024

Exec monitor #1117

Open

1 task

CommanderStorm added blocked cannot progress because of external reasons area:user-management pr:needs review this PR needs a review by maintainers or other community members pr:please address review comments this PR needs a bit more work to be mergable labels May 19, 2024

CommanderStorm mentioned this pull request May 19, 2024

[feat] Remote header auth #1731

Draft

6 tasks

github-actions bot added pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again and removed pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again labels May 20, 2024

CommanderStorm added the pr:needs other pr merged pending other PR to be merged first label May 23, 2024

github-actions bot added pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again and removed pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again labels May 23, 2024

CommanderStorm removed the pr:needs review this PR needs a review by maintainers or other community members label Jun 3, 2024

github-actions bot removed the pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again label Jun 24, 2024

github-actions bot added the pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again label Jul 15, 2024

rvpool mentioned this pull request Nov 18, 2024

Feature Request: Add LDAP Authentication #5359

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support ldap auth #4751

support ldap auth #4751

zhangweiqaz commented May 10, 2024 •

edited by CommanderStorm

Loading

CommanderStorm left a comment •

edited

Loading

CommanderStorm May 10, 2024

CommanderStorm May 10, 2024

CommanderStorm May 10, 2024

CommanderStorm May 10, 2024

CommanderStorm May 10, 2024

CommanderStorm May 10, 2024

CommanderStorm May 10, 2024

zhangweiqaz commented May 11, 2024

Zaid-maker commented May 11, 2024

support ldap auth #4751

Are you sure you want to change the base?

support ldap auth #4751

Conversation

zhangweiqaz commented May 10, 2024 • edited by CommanderStorm Loading

Description

Type of change

Checklist

Screenshots (if any)

CommanderStorm left a comment • edited Loading

Choose a reason for hiding this comment

CommanderStorm May 10, 2024

Choose a reason for hiding this comment

CommanderStorm May 10, 2024

Choose a reason for hiding this comment

CommanderStorm May 10, 2024

Choose a reason for hiding this comment

CommanderStorm May 10, 2024

Choose a reason for hiding this comment

CommanderStorm May 10, 2024

Choose a reason for hiding this comment

CommanderStorm May 10, 2024

Choose a reason for hiding this comment

CommanderStorm May 10, 2024

Choose a reason for hiding this comment

zhangweiqaz commented May 11, 2024

Zaid-maker commented May 11, 2024

zhangweiqaz commented May 10, 2024 •

edited by CommanderStorm

Loading

CommanderStorm left a comment •

edited

Loading