search

sebo-b / warp

Workspace Autonomous Reservation Program - a system to help you efficiently manage hybrid (assigned, hot-desks, etc) office space.
MIT License
139 stars 60 forks source link

Support to LDAP or Active directory authentication #8

Closed n-rodrig closed 1 year ago

n-rodrig commented 2 years ago

Added support to authenticate via LDAP server like Active Directory, Allow your LDAP directory users login on your WARP instalation easily.

Supported configurations to connect LDAP server are:

LDAP auth is set WARP_AUTH_LDAP env variable to True. When enabled WARP will check user login and password via LDAP bind action and the list of authorithed groups (see WARP_LDAP_GROUP_MAP env variable). If Bind acction succed and user belongs to authorized groups login is allowed.

sebo-b commented 2 years ago

Thanks for collaborating - I will try to review it within a week and integrate it.

n-rodrig commented 2 years ago

As I make my enhancements, I'll submit a new pull request with new LDAP connectivity capability. Maybe you'd rather hold off on making any revisions.

n-rodrig commented 2 years ago

Ready for review.

echu2013 commented 2 years ago

Hello @n-rodrig , first thanks for your work! It was extremelly useful for my situation. I've added some suggestions which I've came to in my setup ! Hope it's OK for you!

n-rodrig commented 2 years ago

Hello @n-rodrig , first thanks for your work! It was extremelly useful for my situation. I've added some suggestions which I've came to in my setup ! Hope it's OK for you!

I´m happy to known that my solution is usefull for you. Feedback is well come. But, where have you added the suggestions,? I can' t see them.

echu2013 commented 2 years ago

Hello @n-rodrig , first thanks for your work! It was extremelly useful for my situation. I've added some suggestions which I've came to in my setup ! Hope it's OK for you!

I´m happy to known that my solution is usefull for you. Feedback is well come. But, where have you added the suggestions,? I can' t see them.

Just above my comment you'll see my suggestions!

Gjacquenot commented 1 year ago

👋 Any update on this merge request?

sebo-b commented 1 year ago

hi - it is still on my todo list - sorry for the delay.

sebo-b commented 1 year ago

@n-rodrig I cleaned up the code, and I'm ready to merge this PR. However I did only limited tests of this functionality (I don't have AD running), could you please test if everything works before I click merge?

n-rodrig commented 1 year ago

yes, of course!

sebo-b commented 1 year ago

Thanks @n-rodrig, however pls hold on for a day or two - I'm finishing further refactoring.

n-rodrig commented 1 year ago

@sebo-b I checked and it works,

I just did some corrections on name of LDAP values and type for LDAP configuration.

I update Readme as well to include changes,

Ready to merge on master.

cheer!

sebo-b commented 1 year ago

Hi @n-rodrig,

Thanks for checking and fixes, it is really appreciated. However, as said, I was in the middle of refactoring it further. I have just finished and checked in the changes. Many configuration variables are changed, so it is not backward compatible - please check README.md for details.

I have tested it rather extensively with OpenLDAP over plan/SSL/StartTLS, and everything seems to work. However, I don't have access to AD and I don't have any experience with it. It would be great if you can check it again (sorry for that), especially:

Please check README.md for my understanding of how the above should work.

sebo-b commented 1 year ago

ok - I'm merging it to the main branch. If there are bugs with AD support, we can always fix them in the main.

Gjacquenot commented 1 year ago

Thanks for the merge!

@sebo-b , would you be interested in using isort and then black for code formatting?

If so, I can propose the associated pull request. Or @sebo-b , you can do it yourself, as it changes mainly line ownership.

I use these commands

isort --profile black -l 100 my_code
black -l 100 my_code
n-rodrig commented 1 year ago

Hi, great job on group mapping and refactor.

I have checked, but unfortunately, it does not work for me as DN specification schema is too strict. I will make a commit and a proposal based on DN search instead of fixed schema for DN. It may be ready this week.

sebo-b commented 1 year ago

Thanks @n-rodrig - I'm also trying to run samba in AD compatibility mode in a docker to test it out. Will keep you posted if I mange.

sebo-b commented 1 year ago

hi @n-rodrig - I played with Samba and AD and I think that I have a pretty good understanding now of what the problem is. Let me update the code and then I will ask you for verification.

I've just opened this issue to track it: https://github.com/sebo-b/warp/issues/19

n-rodrig commented 1 year ago

Hi! I have found just minimal problems. I have already solved the problems and, as well, changed the way the user is queried by not using a fixed user DN scheme. This way it is allowed to have any user organization in the LDAP directory.

Pls take a look at code changes on my fork https://github.com/sebo-b/warp/compare/main...n-rodrig:warp:main

sebo-b commented 1 year ago

Thanks - this is almost exactly what I wanted to add, I just want to generalize it a bit.

Re using ast for parsing variables - I decided to use json.loads as it is safer. The end result is the same, but you just need to pass variables in Javascript format instead of Python format.

sebo-b commented 1 year ago

hi @n-rodrig - I have added changes in commit 7ef4a5f. It is successfully tested against Samba AD implementation, so I hope it will also work in your setup.

PS:

n-rodrig commented 1 year ago

Hi.

Checked and auth works but goups mapping fails with:

_File "/usr/local/lib/python3.10/site-packages/warp/authldap.py", line 112, in ldapGetUserMetadata for ldapGroup,warpGroup in ldapGroupMap: ValueError: not enough values to unpack (expected 2, got 1)

I change nothing on array definition. Seems not beeing processed as an array.

¿Could you check?

sebo-b commented 1 year ago

Hi,

That's weird - it works in my test instance. Can you please search in your project for a definition of LDAP_GROUP_MAP? In addition, can you check what's in this variable - attaching a patch to print it out:

diff --git a/warp/auth_ldap.py b/warp/auth_ldap.py
index 2f643f8..da34f80 100644
--- a/warp/auth_ldap.py
+++ b/warp/auth_ldap.py
@@ -109,6 +109,7 @@ def ldapGetUserMetadata(login,ldapConnection):
     loginAllowed = False
     searchFilterTemplate = flask.current_app.config.get('LDAP_GROUP_SEARCH_FILTER_TEMPLATE')
     ldapGroupMap = flask.current_app.config.get('LDAP_GROUP_MAP')
+    print(ldapGroupMap)
     for ldapGroup,warpGroup in ldapGroupMap:
n-rodrig commented 1 year ago

Hi: I have checked,and t works if I remove " in arrays definition

Isntead of _WARP_LDAP_GROUP_MAP = "[ ['CN=warpallowed,CN=Users,DC=samdom,DC=example,DC=com',"AD users"], [null,'Everyone'] ]"

I use:

_WARP_LDAP_GROUP_MAP=[ ['CN=warpallowed,CN=Users,DC=samdom,DC=example,DC=com',"AD users"], [null,'Everyone'] ]

sebo-b commented 1 year ago

There is a simple mistake in the first one - you have double quotation marks " in AD users - so it closes the string. I assume that if you replace them with a single ones ' it will work. image

sebo-b commented 1 year ago

I have just realized it was my mistake - sorry. I've already fixed it in the README. Thanks for bringing it up (and for your patience in re-testing this module)!