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.

[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
• The list of settings is huge, making the screen rather cluttered ==> I don't think there should be yet another setting for PAT
• 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.
• 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?
• 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.
• Is there a way to somehow group the LDAP settings? Eg by showing them with their own heading?
• 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?

I didn't set up an LDAP server for testing yet, will do that once the UI is completed.

[ByteHamster]

With an LDAP-specific settings section, you could pass that full section in a single parameter instead of extracting this huge number of settings here

[El-Virus]

I don't quite get what you mean, that parameter would still need to read the values from the config file, resulting in even larger code.

[ByteHamster]

I was thinking about something like $authBackend = new \Baikal\Core\LDAP($this->pdo, 'users', $config['ldap]);. The constructor internally needs a ton of lines to assign everything to member variables anyway.

Alternatively, how about using an LdapConfig struct? Something like this:

class LdapConfig {
    public $ldap_mode;
    public $ldap_uri;
    ...
}

$LdapConfig = new LdapConfig();
$LdapConfig->ldap_uri = $config['xy];
...
$authBackend = new \Baikal\Core\LDAP($this->pdo, 'users', $LdapConfig);

class LDAP extends \Sabre\DAV\Auth\Backend\AbstractBasic {
    protected $LdapConfig;
    public function __construct(..., $LdapConfig) {
        $this->LdapConfig = $LdapConfig;
    }
}

[El-Virus]

@ByteHamster, I see what you mean, but as LDAP config is set in "System Settings", it'll be saved under $config['system']. So either we create a new tab or we could add code in set to handle LDAP set the values independently.

[ByteHamster]

Yeah, an additional settings section sounds like a lot of work and also quite hacky. How do you feel about that "struct"? It would reduce the long list of constructor parameters, making it a bit easier to comprehend

[El-Virus]

@ByteHamster, is this ok?

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