BookStackApp / BookStack

A platform to create documentation/wiki content built with PHP & Laravel
https://www.bookstackapp.com/
MIT License
15.43k stars 1.94k forks source link

RFC2307 (openLDAP/Posix) style LDAP group memberships #5287

Open bennet0496 opened 4 weeks ago

bennet0496 commented 4 weeks ago

Describe the feature you'd like

There are two major ways groups can be resolved in LDAP Directory Servers. Either via the an attribute on the user (usually memberOf, RFC2307bis). Or via the group object just listing its members (usually with a memberUid), with a second query required to to find a user's groups (RFC2307). Bookstack currently only support the former to sync user groups. I'd like Bookstack to support both modes of group membership to also support Posix conform directory servers.

Describe the benefits this would bring to existing BookStack users

OpenLDAP servers (or maybe other Non-AD Servers as well), predominately used in Unix/Linux environments, by default use the RFC2307 mode of managing group memberships, requiring extra setup to use the other via the memberOf-overlay. While it is certainly not uncommon to the have this overlay configured in the openLDAP world. Both modes are mutually exclusive, and can not trivially migrated from one to the other. This leaves users that in the past setup their OpenLDAP server, the default posix way, either without group sync capabilities in Bookstack or with the burden of migrating their LDAP directory and connected services to use RFC2307bis instead.

Can the goal of this request already be achieved via other means?

No

Have you searched for an existing open/closed issue?

How long have you been using BookStack?

1 to 5 years

Additional context

I provided an exemplary implementation in #5281. However, it is currently unclear how large the user base of this feature would be. And as @ssddanbrown pointed out, it might not be worth adding this additional complexity for only a handful of users. Therefore this feature request is primarily meant to gauge interest of the broader user base for this feature.

@ssddanbrown also noted that it might be an option to add custom group sync capabilities via logical themes. However, while this might be a reasonable middle ground, I'm not sure if there are that may other (useful) ways to sync groups (even with other login methods) and whether the complexity is better spend adding the feature directly or indirectly by letting user hook into the sync process.

bennet0496 commented 4 weeks ago

After testing around a bit, the following logical theme seams to to the trick

Theme::listen(ThemeEvents::AUTH_LOGIN, function (string $authSystem, User $user) {
    $groupSyncService = App::get(GroupSyncService::class);
    $ldapService = App::get(LdapService::class);

    $getUserGroups = Closure::bind(function (string $userName) {
        $memberAttr = "memberUid";
        $groupBase = "ou=groups,ou=members,dc=example,dc=com";

        $ldapConnection = $this->getConnection();
        $this->bindSystemUser($ldapConnection);

        $followReferrals = $this->config['follow_referrals'] ? 1 : 0;
        $this->ldap->setOption($ldapConnection, LDAP_OPT_REFERRALS, $followReferrals);

        $filter = sprintf("(&(%s=%s)(objectClass=posixGroup))", $memberAttr, $userName);

        $groups = $this->ldap->searchAndGetEntries($ldapConnection, $groupBase, $filter, ["CN"]);

        if ($groups['count'] === 0) {
            return [];
        }

        return array_filter(array_map(function ($elm) {
            if (is_array($elm) && array_key_exists("cn", $elm)) {
                return $elm['cn'][0];
            } else {
                return null;
            }
        }, $groups));
    }, $ldapService, $ldapService);

    $groups = $getUserGroups->call($ldapService, $user->external_auth_id);
    $groupSyncService->syncUserWithFoundGroups($user, $groups, false);
});

Which is basically the code we have had running for 3 years now, but now attached with reflection to the LdapService class via the logical theme. Because it seams that the "normal" group sync is also only executed once at login.

ssddanbrown commented 4 weeks ago

@bennet0496 Good to see you were able to come up with a logical theme solution! Feel free to let me know if anything is particularly problematic in maintaining that on updates/changes, but it's not an area we touch too much so should be fairly stable.

The only thing I noticed is that, unless I'm missing something, the $userName is not escaped for the LDAP filter. Probably won't matter in most cases, but could be a trick thing to debug if a user ever has a ldap filter var (eg. a ()&|=) in their name.

Also, might want a check at the start of the event, with a check against the $authSystem to ensure it's ldap before running the logic, so you are not fighting it if/when the LDAP system is down and you're trying to regain access via standard authentication.

bennet0496 commented 4 weeks ago

Ok thanks good to know, that it should be stable for the foreseeable future. As for the username I'm not too sure whether these chars are even valid for an uid and I also think no sane admin would allow them in their system :) but also I guess never say never so I will check how to best escape them. And checking the $authSystem is a good point, it will add that, but I guess if the only concern is getting in when LDAP is broken, then one could also deactivate the theme for that occasion.

And I think will then just implement this solution in our instance and call it a day - should probably why more stable and easier to maintain that applying a patch with every update. So if you want to close this feature request feel free. But it is maybe also worth keeping it up a while to see whether there is any additional interest getting this into the core.