Server: Added ldap authentication #9150

marcorombach · 2023-10-27T10:50:02Z

This provides the option to use up to two LDAP directories as authentication source for Joplin Server.
If activated user also will be created on the fly if LDAP authentication was successful.
The mail address is used to search the user in the directories.

Following ENV variables are available to configure the LDAP authentication:

LDAP_1_ENABLED=false
LDAP_1_USER_AUTO_CREATION=true
LDAP_1_HOST='' // ldap server in following format ldap(s)://servername:port
LDAP_1_MAIL_ATTRIBUTE='mail'
LDAP_1_FULLNAME_ATTRIBUTE='displayName'
LDAP_1_BASE_DN=''
LDAP_1_BIND_DN=''  // used for user search - leave empty if ldap server allows anonymous bind
LDAP_1_BIND_PW='' // used for user search - leave empty if ldap server allows anonymous bind

For a second LDAP server just replace 1 with 2 in the variable name.
BIND_DN and BIND_PW are optional - if not provided, a anonymous bind against the LDAP server will be tried.
MAIL_ATTRIBUTE and FULLNAME_ATTRIBUTE may vary for different LDAP servers, the preset values works for ActiveDirectory.

…ories

github-actions · 2023-10-27T10:50:21Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

marcorombach · 2023-10-27T10:52:57Z

I have read the CLA Document and I hereby sign the CLA

laurent22

Thanks @marcorombach, that looks good overall. You'll need to run yarn install and commit because CI is not passing at the moment.

Also would it be possible to add tests for this feature?

packages/server/src/config.ts

packages/server/src/env.ts

packages/server/src/models/UserModel.ts

packages/server/src/utils/ldap.ts

laurent22 · 2023-10-27T15:29:40Z

packages/server/src/utils/ldap.ts

+		try {
+			await client.bind(searchResults.searchEntries[0].dn, password);
+		} catch (ex) {
+			return null;


Shouldn't silence errors in general. Maybe wrap the error like above and rethrow it?

Changed it as suggested. But now I remembered why I did it that way.
Now the user will be confronted with an error like "LdapErr: DSID-0c090439 ..."
By returning null the user gets "invalid username or password" which is almost always the cause for throwing an error here.

What would you prefer? Maybe returning null but logging the error message?

In general we need to distinguish between user errors and server errors. If the call here is to check the user password, is it possible to look at the exception error and return null if it's an invalid login, and throw an exception if, for example, the server is unreachable? Does the error object has some code attached to it that we could look at for this?

I found the error code for invalid credentials. If an error with this code is thrown, it now will return null and otherwise it will return the error.

marcorombach · 2023-10-30T08:22:40Z

Also would it be possible to add tests for this feature?

Tests would need a mock ldap server like https://www.npmjs.com/package/ldap-server-mock to work.
This should be possible but I'd prefer to implement this at a later time if that's okay, as it will be much work to do this properly.

laurent22 · 2023-10-30T11:03:18Z

Implementing a whole mock server would be overkill, but we need at least tests for the login method since this is a very important part of the software. You can do that relatively easily in api/sessions.test.ts and by mocking the ldapLogin call. For example you can make it return a user, or no user, or make it throw an exception, and verify that everything works as expected.

packages/server/src/utils/ldapLogin.ts

marcorombach · 2023-10-31T10:46:07Z

Implementing a whole mock server would be overkill, but we need at least tests for the login method since this is a very important part of the software. You can do that relatively easily in api/sessions.test.ts and by mocking the ldapLogin call. For example you can make it return a user, or no user, or make it throw an exception, and verify that everything works as expected.

Added some tests, are they sufficient?

laurent22 · 2023-11-07T11:58:06Z

It looks good now, thanks for adding the test @marcorombach! There's now a conflict on yarn.lock - would you mind fixing it? You just need to run yarn install and commit the result

marcorombach · 2023-11-08T08:37:38Z

@laurent22 the conflict on yarn.lock is now resolved. Somehow there where changes to supportedLocales.js after running yarn install that where commited. There is a entry in .gitignore, so I don't know why that was commited. I reverted these changes and it seems like everything is ok now.

laurent22 · 2023-11-08T14:16:50Z

Ok that's strange, because that file should actually be named supportedLocales.js and it's indeed automatically built. But there's nothing in the codebase about a supportedLanguages.js file. It definitely makes sense to exclude that file in any case. Thanks for making these changes, I'm going to wait for CI to run and I'll merge

tompaah · 2023-12-20T07:17:42Z

Unable to get this to work with joplin/server:2.13.5-beta in Docker and using Authentik LDAP Outpost.
The LDAP server answers questions correctly using ldapsearch or other LDAP enabled applications.

The web interface says only is an invalid LDAP URL (protocol) in the red error field, nothing more.
Container logs

07:09:11 0|app  | 2023-12-20 07:09:11: LDAP: Starting authentication with Server 
07:09:11 0|app  | 2023-12-20 07:09:11: App: POST /login (500) (23ms)
07:09:11 0|app  | 2023-12-20 07:09:11: App: GET /css/bulma.min.css (200) (106ms)
07:09:11 0|app  | 2023-12-20 07:09:11: App: GET /css/fontawesome/css/all.min.css (200) (81ms)
07:09:11 0|app  | 2023-12-20 07:09:11: App: GET /js/jquery.min.js (200) (28ms)
07:09:11 0|app  | 2023-12-20 07:09:11: App: GET /css/main.css (200) (95ms)
07:09:11 0|app  | 2023-12-20 07:09:11: App: GET /js/main.js (200) (29ms)
07:09:11 0|app  | 2023-12-20 07:09:11: App: GET /images/server_logo.png (200) (26ms)
07:09:11 0|app  | 2023-12-20 07:09:11: App: GET /images/icons/server/manifest.webmanifest (200) (12ms)

Docker stack environment variables for LDAP are

      - LDAP_1_ENABLED=true
      - LDAP_1_CREATE_USER=false
      - LDAP_1_SERVER=ldap://192.168.xx.xx:389
      - LDAP_1_MAIL_ATTRIBUTE='mail'
      - LDAP_1_FULLNAME_ATTRIBUTE='displayName'
      - LDAP_1_BASE_DN='dc=xxxx,dc=xxxxx,dc=xxx'
      - LDAP_1_BIND_DN='cn=xxxxxx,ou=users,DC=xxxx,DC=xxxxx,DC=xxx
      - LDAP_1_BIND_PW='xxx'

Is there any way of enabling extended debug/error logging? Right now the only clue is that it appears to have problem with the LDAP_1_SERVER url but not idea as to why. Tried having the variable in single, double quotes or no quotes, this makes no difference.

marcorombach · 2023-12-20T07:51:59Z

Seems like you took the env vars from my initial PR comment. These are not the final variable names. I'll try to edit this comment.

In the meanwhile you can find the correct variables including a brief description in this file: https://github.com/laurent22/joplin/blob/dev/packages/server/src/env.ts#L111

tompaah · 2023-12-20T08:04:50Z

Seems like you took the env vars from my initial PR comment. These are not the final variable names. I'll try to edit this comment.

In the meanwhile you can find the correct variables including a brief description in this file: https://github.com/laurent22/joplin/blob/dev/packages/server/src/env.ts#L111

Thank you for the help and your contribution to the code, it's working now with the correct variable names. 😊

marcorombach · 2023-12-20T08:10:54Z

You're welcome! Glad it works for you now :)

marcorombach added 2 commits October 25, 2023 17:50

added ldap authentication and user creation for up to two ldap direct…

48279fa

…ories

changed ldap config to array

f0d88c2

github-actions bot added a commit that referenced this pull request Oct 27, 2023

@marcorombach has signed the CLA in #9150

9e65095

laurent22 reviewed Oct 27, 2023

View reviewed changes

marcorombach and others added 2 commits October 30, 2023 09:37

Merge branch 'laurent22:dev' into ldap_authentication

507dbf3

several fixes to coding style

da355ea

laurent22 reviewed Oct 30, 2023

View reviewed changes

packages/server/src/utils/ldapLogin.ts Outdated Show resolved Hide resolved

added tests and improved error handling

2028b21

marcorombach added 2 commits November 8, 2023 09:23

Merge branch 'dev' into ldap_authentication

afd3e21

reverted automatic changes to supportedLanguages.js

a3003e3

laurent22 merged commit f50019d into laurent22:dev Nov 8, 2023

rcrisanti mentioned this pull request Dec 17, 2024

Server: allow self-signed certificate for ldap auth #11531

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Server: Added ldap authentication #9150

Server: Added ldap authentication #9150

marcorombach commented Oct 27, 2023 •

edited

Loading

github-actions bot commented Oct 27, 2023 •

edited

Loading

marcorombach commented Oct 27, 2023

laurent22 left a comment

laurent22 Oct 27, 2023

marcorombach Oct 30, 2023 •

edited

Loading

laurent22 Oct 30, 2023

marcorombach Oct 31, 2023

marcorombach commented Oct 30, 2023

laurent22 commented Oct 30, 2023

marcorombach commented Oct 31, 2023

laurent22 commented Nov 7, 2023

marcorombach commented Nov 8, 2023

laurent22 commented Nov 8, 2023

tompaah commented Dec 20, 2023

marcorombach commented Dec 20, 2023

tompaah commented Dec 20, 2023

marcorombach commented Dec 20, 2023

Server: Added ldap authentication #9150

Server: Added ldap authentication #9150

Conversation

marcorombach commented Oct 27, 2023 • edited Loading

github-actions bot commented Oct 27, 2023 • edited Loading

marcorombach commented Oct 27, 2023

laurent22 left a comment

Choose a reason for hiding this comment

laurent22 Oct 27, 2023

Choose a reason for hiding this comment

marcorombach Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

laurent22 Oct 30, 2023

Choose a reason for hiding this comment

marcorombach Oct 31, 2023

Choose a reason for hiding this comment

marcorombach commented Oct 30, 2023

laurent22 commented Oct 30, 2023

marcorombach commented Oct 31, 2023

laurent22 commented Nov 7, 2023

marcorombach commented Nov 8, 2023

laurent22 commented Nov 8, 2023

tompaah commented Dec 20, 2023

marcorombach commented Dec 20, 2023

tompaah commented Dec 20, 2023

marcorombach commented Dec 20, 2023

marcorombach commented Oct 27, 2023 •

edited

Loading

github-actions bot commented Oct 27, 2023 •

edited

Loading

marcorombach Oct 30, 2023 •

edited

Loading