implement login via LDAP #826

Manually merged
trinity-1686a merged 6 commits from ldap into main 4 years ago
Owner

fix #312

fix #312
igalic commented 4 years ago
Owner
-            // Making fake password verification to avoid different
-            // response times that would make it possible to know
-            // if a username is registered or not.
-            User::get(conn, 1)?.auth(&query.password);

why would you get rid of this?

``` - // Making fake password verification to avoid different - // response times that would make it possible to know - // if a username is registered or not. - User::get(conn, 1)?.auth(&query.password); ``` why would you get rid of this?
Poster
Owner

It has been added back in the model, inside login()

It has been added back in the model, inside login()
trinity-1686a changed title from WIP: implement login via LDAP to implement login via LDAP 4 years ago
Poster
Owner

I've tested the implementation against Bottin, however I'm not sure how to add tests that can be run automatically.

Once this is merged, I'll add the new environment variables to the docs.

I've tested the implementation against [Bottin](https://git.deuxfleurs.fr/Deuxfleurs/bottin), however I'm not sure how to add tests that can be run automatically. Once this is merged, I'll add the new environment variables to the docs.
igalic requested changes 4 years ago
igalic left a comment
Owner

i've looked at this now and have left a few comments.
I like the general look and feel!

i've looked at this now and have left a few comments. I like the general look and feel!
@ -172,0 +176,4 @@
dependencies = [
"proc-macro2 1.0.18 (registry+https://github.com/rust-lang/crates.io-index)",
"quote 1.0.7 (registry+https://github.com/rust-lang/crates.io-index)",
"syn 1.0.34 (registry+https://github.com/rust-lang/crates.io-index)",
igalic commented 4 years ago
Owner

we should upgrade our own dependencies of these packages to 1.x

we should upgrade our own dependencies of these packages to 1.x
igalic marked this conversation as resolved
@ -243,0 +268,4 @@
user_name_attr,
mail_attr,
})
} else if addr.is_some() && base_dn.is_some() {
igalic commented 4 years ago
Owner

if addr and base_dn is_some()

if addr and base_dn `is_some()`…
trinity-1686a marked this conversation as resolved
@ -243,0 +269,4 @@
mail_attr,
})
} else if addr.is_some() && base_dn.is_some() {
panic!("Invalid LDAP configuration : both LDAP_ADDR and LDAP_BASE_DN must be set")
igalic commented 4 years ago
Owner

we panic, because both of those must be set??!

we panic, because both of those must be set??!
Poster
Owner

Hum there is a mistake making this code unreachable. If both are set, it will take the first if, if only one is set, it should take the else if (which is not the case, I'll change that), I consider that invalid as both an addr and a base_dn are required, and there is no good default value (contrary to other vars), and finaly the else is to disable ldap when none is configured

Hum there is a mistake making this code unreachable. If both are set, it will take the first `if`, if only one is set, it should take the `else if` (which is not the case, I'll change that), I consider that invalid as both an addr and a base_dn are required, and there is no good default value (contrary to other vars), and finaly the else is to disable ldap when none is configured
trinity-1686a marked this conversation as resolved
@ -300,0 +300,4 @@
let bind = ldap_conn
.simple_bind(&ldap_name, password)
.map_err(|_| Error::NotFound)?;
if bind.success().is_ok() {
igalic commented 4 years ago
Owner

if we use ldap as user-database, we could do the ldap bind once, and then pass around a connection…

…well… an Arc<Mutex<LdapConn>>

if we use ldap as user-database, we could do the ldap bind once, and then pass around a connection… …well… an `Arc<Mutex<LdapConn>>`
Poster
Owner

We could, that's more or less what I was doing on a draft a few months ago, but I lost it, and I'm not sure how much it would be helpful. Sure we use it as a user database, but we only consult it on login/account creation, so not exactly a bottleneck

We could, that's more or less what I was doing on a draft a few months ago, but I lost it, and I'm not sure how much it would be helpful. Sure we use it as a user database, but we only consult it on login/account creation, so not exactly a bottleneck
@ -300,0 +333,4 @@
Err(Error::NotFound)
}
} else {
Err(Error::NotFound)
igalic commented 4 years ago
Owner

would it be possible to do these as if not tests, so we don't nest four levels deep until we get to the actual code.

would it be possible to do these as if not tests, so we don't nest four levels deep until we get to the actual code.
trinity-1686a marked this conversation as resolved
@ -300,0 +358,4 @@
}
}
pub fn login(conn: &Connection, ident: &str, password: &str) -> Result<User> {
igalic commented 4 years ago
Owner

can you please document where in this code we now fake the login check to prevent timing attacks?
i can't seem to see it in this code.

can you please document where in this code we now fake the login check to prevent timing attacks? i can't seem to see it in this code.
trinity-1686a marked this conversation as resolved
@ -300,0 +361,4 @@
pub fn login(conn: &Connection, ident: &str, password: &str) -> Result<User> {
let local_id = Instance::get_local()?.id;
let user = User::find_by_email(conn, ident)
.or_else(|_| User::find_by_name(conn, ident, local_id))
igalic commented 4 years ago
Owner

these…

these…
igalic marked this conversation as resolved
@ -300,0 +362,4 @@
let local_id = Instance::get_local()?.id;
let user = User::find_by_email(conn, ident)
.or_else(|_| User::find_by_name(conn, ident, local_id))
.and_then(|u| {
igalic commented 4 years ago
Owner

…are going to be pain with async code

can we preemptively turn this into a match?

…are going to be pain with async code can we preemptively turn this into a `match`?
Poster
Owner

I wasn't 100% sure what you meant, if the result doesn't fit your expectations, please tell

I wasn't 100% sure what you meant, if the result doesn't fit your expectations, please tell
igalic approved these changes 4 years ago
igalic left a comment
Owner

👍

👍
Poster
Owner

I'm not sure why gitea sais " Some required checks are missing". May I still merge it?

I'm not sure why gitea sais "❌ Some required checks are missing". May I still merge it?
igalic commented 4 years ago
Owner

@trinity-1686a

I'm not sure why gitea sais " Some required checks are missing". May I still merge it?

yes, please do

@trinity-1686a >I'm not sure why gitea sais "❌ Some required checks are missing". May I still merge it? yes, please do
trinity-1686a merged commit 9ec2d93f50 into main manually 4 years ago
trinity-1686a deleted branch ldap 4 years ago

Reviewers

igalic approved these changes 4 years ago
The pull request has been manually merged as 9ec2d93f50.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Plume/Plume#826
Loading…
There is no content yet.