LDAP support #921
Jade
nex
Labels
No labels
Bug
Cherry-picking
Database
Dependencies
Dependencies/Renovate
Difficulty
Easy
Difficulty
Hard
Difficulty
Medium
Documentation
Enhancement
Good first issue
Help wanted
Inherited
Matrix/Administration
Matrix/Appservices
Matrix/Auth
Matrix/Client
Matrix/Core
Matrix/Federation
Matrix/Hydra
Matrix/MSC
Matrix/Media
Meta
Meta/CI
Meta/Packaging
Priority
Blocking
Priority
High
Priority
Low
Security
Status/Blocked
Status
Confirmed
Status
Duplicate
Status
Invalid
Status
Needs Investigation
To-Merge
Wont fix
old/ci/cd
old/rust
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
continuwuation/continuwuity!921
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "RatCornu/continuwuity:ldap"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Hi!
I implemented few monthes ago LDAP login for tuwunel and I discovered after this. I would like to add the LDAP feature here to: I tried to stick as much as possible with continuwuity code style starting from the tuwunel implementation, but I'm not sure if everything is alright: I'll change everything needed to comply to your standards.
I tested basic login on a real LDAP server with cURL and it works. As the implementation is litteraly the same as in tuwunel, I would expect other features to work as inteded.
Closes #691.
Hey, great to see you! Thanks for the PR - looks like the preflight checks ran fine, so your style was perfectly fine. I'll just give this a quick test later (hopefully tomorrow), then should be good to get it merged :)
Nice! Moreover, do not hesitate to ping for LDAP issues if needed, I'll be using continuwuity + LDAP in near future and I'm willing to help wherever I can ^^
From my late night phone review, it all looks pretty good to me. Just some minor tweaks. (I've also rambled a bit in some of the comments)
@ -1777,0 +1838,4 @@## example: "mail"##mail_attribute = "mail"I feel like this should be optional? We don't need email, and not every system will have it
Oh yeah we don't even use it in this diff afaict, probably worth removing entirely or hiding
This is a discussion in the tuwunel implementation: https://github.com/matrix-construct/tuwunel/issues/114.
I can remove it for now and add it back later if a way to store custom profile fields is added into continuwuity
I removed it for now, it's always possible to add it back later
@ -1777,0 +1840,4 @@##mail_attribute = "mail"# Attribute containing the distinguished name of the user.Distinguished name being display name? Might be better to use Matrix terminology here, if that's what you mean.
Indeed, distinguished name is the common terminology when dealing with LDAP, I change this to display name
@ -1777,0 +1854,4 @@##admin_base_dn = false# The LDAP search filter to find administrative users for conduwuit.Continuwuity
@ -52,0 +75,4 @@| Ok(hash) => (hash, user_id),| Err(_) => services.users.password_hash(lowercased_user_id)In what circumstances is this case sensitive, and when is it not? Might need some explainer comments here.
I see, this is copying logic from the original function. Ah well.
@ -52,0 +136,4 @@// password is reserved for deactivated accounts. The conduwuit password field// will never be read to login a LDAP user so it's not an issue.if !services.users.exists(lowercased_user_id).await {servicesUgh this feels hacky and like it will bite in the future, I know it's a bit out of scope but definitely a thing to improve if we get the opportunity in future. In a new database version number we could do a migration for it.
Oh also in the event of a downgrade the logic behind this being unreachable doesn't work, because the account origin won't be checked anymore. It's still probably fine because it's all or nothing and people aren't likely to downgrade off an LDAP supporting version when their server relies on it, but this wouldn't work for OIDC for example, or for partial rather than total server management.
I would say that this requires an upgrade in database version number, but tuwunel didn't do it when they added this afaik, so... I just hope no one uses LDAP, downgrades to a version without it, and expects things to work.
@ -52,0 +185,4 @@}if cfg!(feature = "ldap") && services.config.ldap.enable {ldap_login(services, &user_id, &lowercased_user_id, password).awaitThat it's 'all or nothing' is probably something to note in any documentation.
It's also possible to try normal password login if LDAP login fails if you prefer, both implementations are possible
I added an option
ldap_onlymaking things clearer@ -2044,0 +2120,4 @@#[serde(default = "default_ldap_name_attribute")]pub name_attribute: String,/// Root of the searches for admin users.All these ones without default at the last line should probably have it so the example config doesn't just say
= falsefor each item.OK so what I meant here @RatCornu is that for the example config, you can write
default: somethingand that will show up in the config asoption = something. Like in one of them you've gotdefault: "(objectClass=*)"and in teh config it shows up asfilter = "(objectClass=*)". In any case, I'm not sure why these were showing up as false when in other places that do the same they show as blank.It should be fixed!
Oh, I've just noticed that formatting ci hasn't run for whatever reason. I'm going to need to run cargo format plus cargo clippy manually lol
Isn't that part of the preflight check?
Nope, there are some formatting checks there, but in CI rustfmt has its own step
0d4c5adc5e785789afb5d39579ea471a21f4a852OK I've fixed some issues preventing it from building, etc. It should be good for testing now - it's now included in the default features.
94cde15ab2976387ab33Are you sure you meant to make those nix changes in the last commit?
976387ab334055fbe827Oops indeed sorry, I made
git add .too quickly ^^'Awesome :)
4055fbe827669fa34a0aNobody's raised any complaints after a call for testing, and the code looks good for me, so I'm happy to merge this in later today/tomorrow.
669fa34a0a57d7743037