Added support for LDAP.

[El-Virus]

Added support for LDAP. Supports authentication via DN, attribute, filter & group.

[El-Virus]

@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.

[ByteHamster]

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.

[El-Virus]

@ByteHamster, I'll add curly braces, then.

[El-Virus]

@ByteHamster, I've added curly braces. Is there anything else I should check/change?

[ByteHamster]

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.

[El-Virus]

Give me 2 minutes.

[El-Virus]

@ByteHamster, Done.

[El-Virus]

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?

[ByteHamster]

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.

[El-Virus]

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.

[epsilon-0]

@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.

[El-Virus]

@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.

[El-Virus]

@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.

[El-Virus]

@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.

[epsilon-0]

Its been a while, is there any reason this is still pending?

[phil-davis]

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.

[El-Virus]

@phil-davis I've merged your commit.

[phil-davis]

@ByteHamster you can review again.

[ByteHamster]

• 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?

[El-Virus]

@epsilon-0, Is it ok for me to modify the copyright notice?

[El-Virus]

@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]

@epsilon-0, Is it ok for me to modify the copyright notice?

ah, totally 😺, didn't even think about this!

[El-Virus]

@ByteHamster, Done.

[ByteHamster]

I could quickly create some test users on mine. Do you have a PGP enabled e-mail address?

Hmm, currently not, unfortunately.

[El-Virus]

@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?

[ByteHamster]

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

[El-Virus]

@ByteHamster All the issues in the last review have been fixed.

[ByteHamster]

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.

[El-Virus]

@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.

[ByteHamster]

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;

[El-Virus]

@ByteHamster but wouldn't that return false inside the try block return without calling ldap_close()? I might be mistaken, but I thought that return returned immediately, and the finally block was executed when the end of try was reached.

[ByteHamster]

The finally block is always executed, even when using a return statement. That's why I suggested this solution, so it can never be forgotten.

[seanm]

We're looking to run Baikal, and this PR looks very useful to us. Would it be helpful that we apply this PR locally and do some testing of it?

[ByteHamster]

Of course, testing is always welcome!

[solo-wanderer]

I have applied and tested this PR, and it's been solid so far. Really nice work @El-Virus and @epsilon-0 thank you!

So far, I have encountered two minors inconveniences related to each other: For simplicity, let's call shortname the username only, excluding the domain part, and longname the username+ldapdomain.com part.

1- Using the Attribute LDAP authentication mode paired with uid=%u (%u= long name) the user testuser@myldap.com can authenticate using both its shortname or its longname, which is fine since many LDAP authentications provide the same functionality. The problem is that using either of those will create two separated CalDAV accounts one using the shortname and another using the longname when in reality both are the same ldap user.

2- When a user authenticates himself using his calendar URI (https://baikal.mydomain.com/dav.php/calendars/// ) this process not only authenticates the CalDAV user but will in fact also create a different CalDAV user if the credentials entered use shortname of an LDAP user created with the longname format OR vice-versa. Which leads me to believe that this authentication process is part of the same function or calls the same function which automatically creates CalDAV accounts. IMHO, if possible, this process should only authenticate the in the calendar URI and nothing more.

[El-Virus]

@solo-wanderer, Thank you for your feedback, I'll fix the bugs in the next revision. @ByteHamster, I've been very busy lately, and I'll be even busier the next weeks, so I probably won't be able to work on LDAP for the next two months, sorry about the slowness 😅. (Besides I'll be trying to implement user federation, which will take a while.)

[TCB13]

Thank you very much for your ongoing efforts around this integration.

[El-Virus]

@ByteHamster, I'm implementing user federation, and need to access $authBackend (defined in \Baikal\Core\Server) in \BaikalAdmin\Controllers\User but I see no reference to the server object anywhere (to call a function to fetch the variable). Is there a way in Baikal to share variables between these two classes? May I use a global variable?

[mxmeeple]

May I enquire the progress of this feature? I'm quite eager to use LDAP auth, but it seems that this has stalled for quite a few months

[BobWs]

I also wonder if this is implemented or not?

[luggesexe]

Hello, we have an active LDAP-Server we could also test new changes on. As @BobWs and @mxmeeple mentioned, there is interest in this PR. Is there any update? What needs to be done where help may be needed?

[El-Virus]

@mxmeeple @BobWs @luggesexe . Yes, indeed the PR has been stale the last months. I've been quite busy with other projects and life in general, and this one is quite ambitious (Implementing User Federation, which is what needs to be done now is quite the task). But seeing that a few people depend on this, I'll be bumping this to the top of my TODO list. I'll resume development in about a month. Thank you for your support.

[HDValentin]

Thank you for your work on LDAP integration. I am also waiting for a release. What does "User Federation" mean? That I can give other ldap users the rights to look into my calendar or write into it for example?

[El-Virus]

@HDValentin: User Federation is not a LDAP feature. It will serve as a base for LDAP and other future authentication methods. Its purpose is to manage concurrent usage of multiple user sources, which will hopefully solve some of the issues of the current implementation.

[n-rodriguez]

Hi there! Any news?