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

Add LDAP authentication #1626

Open
wants to merge 1 commit into
base: develop
from
Open

Add LDAP authentication #1626

wants to merge 1 commit into from

Conversation

@immae
Copy link

immae commented Feb 12, 2019

This PR implements LDAP authentication. The behavior is the expected standard behavior for LDAP authentication: a master LDAP account can search for the user that tries to log-in, then if found we try to bind as that user and if successful, the user is logged in.

I tried to add some tests, but I couldn’t find any existing tests for models and libs (the parts of the code that I touched)...

Fixes #371

@immae immae force-pushed the immae:add_ldap branch from d79d5b3 to 14e5796 Feb 12, 2019
@immae immae mentioned this pull request Feb 13, 2019
base: dc=example,dc=com
mail_entry: "mail"
user_filter: "(|(email=%username%)(uid=%username%))"

This comment has been minimized.

Copy link
@ldidry

ldidry Feb 14, 2019

Collaborator

You may want to add a setting to prevent connection failure when you use ldaps and the LDAP server has a self-signed certificate (for ex.).

I didn't use the same module than you to connect to LDAP, but on ep_mypads, I had to add this optional setting in the LDAP settings:

    "tlsOptions": {
        "rejectUnauthorized": false
    },

If you want to easily test your code against a self-signed certificated ldaps, you can use this docker image: https://github.com/rroemhild/docker-test-openldap

This comment has been minimized.

Copy link
@immae

immae Feb 14, 2019

Author

Good catch @ldidry I added that option.

I used inspiration from etherpad-lite to write this PR, so it’s definitely the same node module ldapjs that is used in both cases :)

(And thanks for pointing me to ep_mypads module, it seems great and I didn’t know about!)

@immae immae force-pushed the immae:add_ldap branch from 14e5796 to c542eca Feb 14, 2019
Copy link
Owner

Chocobozzz left a comment

I have two concerns:

  • What if the user username or email is changed in LDAP? We can say we don't support username change, but we should support email change.
  • What do we do when someone registers on the instance with a username that already exists in LDAP, but not in our local database (because the user has not yet connected for example)
config/default.yaml Outdated Show resolved Hide resolved
config/production.yaml.example Outdated Show resolved Hide resolved
config/production.yaml.example Outdated Show resolved Hide resolved
server/initializers/constants.ts Outdated Show resolved Hide resolved
server/initializers/constants.ts Outdated Show resolved Hide resolved
server/models/account/user.ts Outdated Show resolved Hide resolved
server/models/account/user.ts Outdated Show resolved Hide resolved
server/models/account/user.ts Outdated Show resolved Hide resolved
.then((userInfos) => {
const userToCreate = new UserModel({
username: username,
password: 'SomeInvalidPassword',

This comment has been minimized.

Copy link
@Chocobozzz

Chocobozzz Feb 15, 2019

Owner

null no?

This comment has been minimized.

Copy link
@immae

immae Feb 15, 2019

Author

password field cannot be null, and I didn’t want to remove this constraint as I don’t know the consequences in the rest of the code. If you think it’s safe, we can make it nullable

This comment has been minimized.

Copy link
@Chocobozzz

Chocobozzz Feb 15, 2019

Owner

Yep I think we can allow null on this column

server/models/account/user.ts Outdated Show resolved Hide resolved
@immae

This comment has been minimized.

Copy link
Author

immae commented Feb 15, 2019

What if the user username or email is changed in LDAP? We can say we don't support username change, but we should support email change.

Right, we should update the e-mail when it changes on LDAP side

What do we do when someone registers on the instance with a username that already exists in LDAP, but not in our local database (because the user has not yet connected for example)

It is created automatically

@immae immae force-pushed the immae:add_ldap branch from c542eca to c31ec83 Apr 27, 2019
@immae immae force-pushed the immae:add_ldap branch from c31ec83 to f8dd932 Apr 27, 2019
@immae

This comment has been minimized.

Copy link
Author

immae commented Apr 27, 2019

@Chocobozzz : I finally had the time to address your requests.

I didn’t manage to make the password field nullable : I changed the column in the database to make it nullable, changed in the UserModel too, and made the isUserPasswordValid validator more flexible, but it keeps failing. I may have missed something but I couldn’t find what.

I changed some of the Promise as required, but in lib/ldap.ts there are still some: the ldapjs library works with callbacks, and I don’t know how to turn that into an async/await process.

@immae

This comment has been minimized.

Copy link
Author

immae commented Apr 27, 2019

(about password field nullable: it fails in lib/user.ts createUserAccountAndChannelAndPlaylist, at line userToCreate.save(userOptions), regardless of validate value)

@Chocobozzz

This comment has been minimized.

Copy link
Owner

Chocobozzz commented May 10, 2019

I didn’t manage to make the password field nullable : I changed the column in the database to make it nullable, changed in the UserModel too, and made the isUserPasswordValid validator more flexible, but it keeps failing. I may have missed something but I couldn’t find what.

No problem, I'll merge your PR in a separate branch (next week or the week after) and will update the code myself.

Please could you add unit tests?

if (CONFIG.AUTH.LDAP.ENABLED) {
user = await UserModel.findOrCreateLDAPUser(usernameOrEmail)
}
if (!user && CONFIG.AUTH.LOCAL.ENABLED) {

This comment has been minimized.

Copy link
@Chocobozzz

Chocobozzz May 10, 2019

Owner

I think we should put the local check first. Because users that were created before the LDAP activation should be able to log in with their "old" account.

This comment has been minimized.

Copy link
@battosai30

battosai30 Sep 16, 2019

Usually it's not the good strategy to use local base first. For example in RocketChat :

  1. If LDAP is enabled, you first check if login and password are ok in LDAP server.
  2. If not, and if fallback login option is activated, you check in local app database if login/password are OK.

Usually LDAP server is the authentification security reference, so it's normal that's if activated it's checked first.

@Chocobozzz

This comment has been minimized.

Copy link
Owner

Chocobozzz commented May 10, 2019

What do we do when someone registers on the instance with a username that already exists in LDAP, but not in our local database (because the user has not yet connected for example)

It is created automatically

IMHO you should add a check in the registration controller/middleware, so users cannot register a username or email that already exists in LDAP. What do you think?

@immae

This comment has been minimized.

Copy link
Author

immae commented May 13, 2019

IRC.log

@battosai30

This comment has been minimized.

Copy link

battosai30 commented Sep 16, 2019

Hi guys,

Any update on this feature ? I'm thinking about implement PeerTube in severall structure that I managed but I really need LDAP for my users as I already use other services (like RocketChat, Nextcloud etc ...) which needs authentification and already support LDAP and allowed my users to only have a single login set on all platforms.

Regards and thanks for your great job

@immae

This comment has been minimized.

Copy link
Author

immae commented Sep 16, 2019

Hi @battosai30 I’m sorry I don’t have time at the moment to do all the changes (cf IRC log above). I also don’t know how much the code changed since May and if my PR would still work on latest version, which doesn’t help getting back to it...

@Chocobozzz

This comment has been minimized.

Copy link
Owner

Chocobozzz commented Sep 16, 2019

Hi,

I plan to improve the plugin system that would allow to register an auth method. Then, (I or @immae if he wants) will externalize this PR in a plugin.

The algo would be:

  • Every auth plugin would have a weight, defined by the administrator
  • On login, we check if the username exists in database. If yes, try to login against this user (it could be an external auth method)
  • If the user does not exist, try to login with every auth plugin ordered by weight DESC. On success with a particular auth plugin, create the user in the database with a column that specify the auth method (LDAP, local etc)

@immae What do you think?

@immae

This comment has been minimized.

Copy link
Author

immae commented Sep 16, 2019

@Chocobozzz yes that seems to be perfect :) Maybe some details would need to be adapted (for auth methods that require more than just a password), but the idea seems good

@jadjay

This comment has been minimized.

Copy link

jadjay commented Sep 30, 2019

Thanks for this !
I hope it will be soon released...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.