New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#75 - LDAP user groups sync to Bookstack on Login #911

Merged
merged 2 commits into from Jul 15, 2018

Conversation

3 participants
@brennanmurphy
Contributor

brennanmurphy commented Jul 2, 2018

Closes issue #75

I set it up so that when users login using an LDAP connection, Bookstack will get all names of all groups that user is a part of on the LDAP server, and will try and match them to the names of roles created in Bookstack. For any matches it finds, it will add the user to that role on Bookstack. The groups to roles sync function only runs on login, so if a user's groups change on the LDAP server, they need to log out of Bookstack, and log back in for any changes to roles to take affect.

You do need to create roles on Bookstack with names that match groups on LDAP exactly for this to work. I did contemplate having Bookstack create roles on a LDAP login for all groups it found for a user, but talking to a few network admins, most applications have the app admins create the roles for matching.

I've added a few settings to the .env file to make this work. I've tried to explain them inline, and I can update the documentation on the website to reflect the changes if you would like.

Happy for any feedback, and willing to make changes as needed. At the place I work, we have been using this patch in production for ~2 months, with 40ish regular users, and have not ran into any issues. I did test it with a few OpenLDAP servers, and one Windows Active Directory server, and it worked in both instances.

This is my first PR here, so apologies if I have made any mistakes.

@ssddanbrown

This comment has been minimized.

Member

ssddanbrown commented Jul 2, 2018

Thank you very much for your efforts here @brennanmurphy, This looks great. Will review in-depth later this week.

@ssddanbrown ssddanbrown merged commit 37aa8b0 into BookStackApp:master Jul 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ssddanbrown

This comment has been minimized.

Member

ssddanbrown commented Jul 15, 2018

Thanks again for this @brennanmurphy, Took me longer than expected to review since I needed to learn how to properly set up my own Ldap groups in a way where memberOf works properly 😅 .

All worked great and worked as you described. It did make a few changes/updates afterwards but nothing major. These can be seen in the following commits:

As an overview, Here are the changes I made:

  1. Added tests to cover this new functionality.
  2. Refactored so the LdapRepo was not needed and moved the methods into the LdapService.
  3. By default, Mapped based on role display_name instead of name (But formatted lower-case and hypenated) since the name is not exposed within the interface and can be different to the display name.
  4. Added External Authentication IDs field in role edit view to allow a comma separated list of names to match against which will be used instead of the role name. This allows group/role mapping without needing to match the names and also opens up many-to-many ldap-group-to-role mappings.
  5. Removed LDAP_ADMIN_GROUP option since this would be somewhat redundant after the custom mapping in (4).

Hopefully my changes won't effect your patched, in-production BookStack instance upon upgrade of the next release but be cautious just incase my mapping changes affect your setup.

I'll tag this so I remember to document all this functionality before release.

@yoyokko

This comment has been minimized.

yoyokko commented Aug 10, 2018

jietu20180810-190426

Login error if set LDAP_USER_TO_GROUPS=true with Active Directory LDAP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment