Baikal
Baikal copied to clipboard
sabre-io
Added support for LDAP.
Added support for LDAP. Supports authentication via DN, attribute, filter & group.
@ByteHamster. I've fixed the majority of problems with the linter, note that I haven't changed two things:
- Linter suggested I added a period after a url: I think it shouldn't be added, as it could cause confusion.
- Linter suggested I added curly brackets on "if" statements that only contain one line: It's a complete valid form of using if statements. If I'm not mistaken, not using curly braces on an "if" statement containing one line makes code run faster.
Linter suggested I added curly brackets on "if" statements that only contain one line: It's a complete valid form of using if statements.
Especially in projects developed by many different people, it is important to always use braces. Not using braces caused a pretty terrible bug in Apple's SSL stack a few years ago: https://en.wikipedia.org/wiki/Unreachable_code#goto_fail_bug
If I'm not mistaken, not using curly braces on an "if" statement containing one line makes code run faster.
I would be highly surprised if any code ran faster just because of a missing curly brace.
@ByteHamster, I've added curly braces. Is there anything else I should check/change?
Linter suggested I added a period after a url: I think it shouldn't be added, as it could cause confusion.
I would just put the url in quotation marks and add the period afterwards. That way, the linter should be happy.
Also, @ByteHamster, I was thinking of adding a feature where users could login using PATs instead of their password, it's not common to see PATs implemented in a LDAP database, but I think it would be beneficial for Baïkal to implement it. Should I commit it to this pull?
Hmm. One of the main goals of Baikal is to be easy to set up - even for people who bought their first php server, who have no experience whatsoever. This sounds like it will add even more settings that such users are overwhelmed by.
Well, @ByteHamster, I'd argue, that if you're setting up LDAP authentication, you're going to know what you're doing with the LDAP settings, It'll otherwise be hidden if LDAP is not selected.
@El-Virus awesome work. I've used it for a while now and it seems to be working well. Any holdups on this being merged? I'll start adding the SMTP after this.
@epsilon-0:
Awesome work. I've used it for a while now and it seems to be working well.
Great! Haven't found any issues in my installation neither.
Any holdups on this being merged?
I'm currently waiting for a member to tell me if I should add PATs to the PR, as well as merge it.
I'll start adding the SMTP after this.
Also great, I'm using your old SMTP implementation at the moment.
@ByteHamster
Sorry, I finally had time to have a look at this. Some things I noticed:
* `URI of the LDAP server` is shown even when LDAP is not selected
My bad, fixed now.
* The list of settings is huge, making the screen rather cluttered ==> I don't think there should be yet another setting for PAT
Sure. (But if you plan on adding a new tab for authentication then we could add it. (See below.))
* Please move the "tooltip"-like text (replacements, etc) below the text boxes, so they have more horizontal space - like it is already done for the invite address. This should save some vertical space.
Done
* Some settings say "default: xy". What does that mean? Does it mean that if I leave the box blank, I get that as a default?
Yes.
* Given that the number of LDAP related settings in the list is quite huge, it is rather hard to see where LDAP stops and where the remaining settings (admin password) start. I think it would be more clear if the admin password was moved up, so that all remaining settings on the page are LDAP only.
Done
* Is there a way to somehow group the LDAP settings? Eg by showing them with their own heading?
Haven't seen any way in Formal.
* All those LDAP settings get added to the main section of the config file. Is there a way to create an extra section that deals with LDAP? That way, users know that they don't need to look at these when changing the config file and not having configured LDAP. Maybe that could work together with the point above, so we could have two different settings "sections" and configuration classes?
The way it's implemented, the configuration page extends a super config class, and when it's constructed it sets the "parent" configuration section (aka. "system"). To do what you ask cleanly, the best way to fix it would be by creating a new tab such as "System Settings" or "Database Settings". Should we create a new tab for authentication? Is that what you mean?
I didn't set up an LDAP server for testing yet, will do that once the UI is completed.
Ok, I'll now proceed to fixing your subcomments and I'll commit the changes to the pr.
@ByteHamster I can't really understand why PHP 8.1 failed. I'm not familiar with the error or phpstan.
- Error comes from an unmodified file, I don't think that error was caused by this PR.
Error comes from an unmodified file, I don't think that error was caused by this PR.
Fixed in PR #1135
Please rebase this PR and it should pass.
- Could you please also add the same copyright notice to the top of the files that all other files have?
- I think it is quite unexpected having to re-enter the ldap password every time a setting is touched. When the password is kept empty, it should probably remember the old password. (otherwise users will reset the password without any visual indication that they did) At the same time, there needs to be some way to set the password to an empty string. Not 100% sure how to best do that.
- Do you have an ldap server that I could use for testing?
@ByteHamster
- Could you please also add the same copyright notice to the top of the files that all other files have?
Will commit as soon as I get epsilon-0's approval. (Done)
- I think it is quite unexpected having to re-enter the ldap password every time a setting is touched. When the password is kept empty, it should probably remember the old password. (otherwise users will reset the password without any visual indication that they did) At the same time, there needs to be some way to set the password to an empty string. Not 100% sure how to best do that.
Fixed, will commit together with copyright fix (Done). About setting it to an empty string, it's bad practice, though if you really want to do so, you could just modify the config file.
- Do you have an ldap server that I could use for testing?
I could quickly create some test users on mine. Do you have a PGP enabled e-mail address?
@epsilon-0, Is it ok for me to modify the copyright notice?
ah, totally 😺, didn't even think about this!
I could quickly create some test users on mine. Do you have a PGP enabled e-mail address?
Hmm, currently not, unfortunately.
@ByteHamster, is there any secure channel where I can reach you? Or maybe you could create a PGP key and send the public key to me?
Or maybe you could create a PGP key and send the public key to me?
Let's see if my old PGP key still works :D http://pgp.mit.edu/pks/lookup?op=vindex&search=0x027D0EEFD7DD8E87
Okay, turns out that the backslash before ldap_connect was not the problem. I simply didn't enable the ldap extension in php.ini. I guess the ldap class should check whether the ldap extension is available and then return a more helpful error message to reduce the number of support requests coming in about this feature.
The error message when entering a wrong user name is Undefined array key 0. This is a bit misleading, I think. Instead, I would check whether the result of ldap_get_entries has size 0 before accessing the array.
Instead of sometimes returning false and sometimes setting $success to false, I think it would be more clean to always use return statements. Maybe together with a try {...} finally { ldap_close() } block?
There are the LDAP-attribute for displayname and LDAP-attribute for email settings. Doesn't that get out of sync when changing something on the LDAP server? It seems like this could lead to problems with invitation emails and stuff.
There is UI allowing to change user attributes in the Baikal web interface - which creates even more inconsistencies with LDAP. Maybe Baikal could refuse to edit users when LDAP auth is enabled.
@ByteHamster
Instead of sometimes returning false and sometimes setting $success to false, I think it would be more clean to always use return statements. Maybe together with a try {...} finally { ldap_close() } block?
$return is set as to be able to create the user in the database for the first time if it doesn't exist or any other post user validation tasks.
I don't quite see how a try finally block would work. I mean, gotos are a thing, but I'd argue it's better the way it is now.
Currently there are some code paths on which ldap_close is called, and some on which it isn't. My idea with finally was something like this, where ldap_close is always called automatically, without having to keep track of the state:
protected function ldapOpen($username, $password) {
$conn = ldap_connect($this->ldap_config->ldap_uri);
if (!$conn) {
return false;
}
try {
// ...
if (authentication failed) {
return false;
}
createUserIfNecessary();
} finally {
ldap_close($conn);
}
return true;