Add rudimentary LDAP group support #8814
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8814 +/- ##
=======================================
Coverage 43.94% 43.94%
=======================================
Files 607 607
Lines 86968 86968
=======================================
Hits 38215 38215
Misses 44051 44051
Partials 4702 4702 Continue to review full report at Codecov.
|
GiteaBot
added
the
lgtm/need 2
stertingen
force-pushed
the
ldap_groups
branch
from
3f631c0 to
60de2d5
Compare
stertingen
force-pushed
the
ldap_groups
branch
from
e013326 to
8e0ed0a
Compare
- Find user groups by - Search base, like ou=people,dc=mycompany,dc=com - Group filter, like (&(objectClass=posixGroup)(member=%s)) - User attribute used by above group filter, like cn - Additional filter for - Membership in general (allowed to log in) - Admin role
This reverts commit 155751c.
stertingen
force-pushed
the
ldap_groups
branch
from
af80210 to
e5c7d5c
Compare
stertingen
changed the title
|
All subtasks done for now. Have I missed something? |
|
I don't see point in these attributes: member-group-filter - can be already set using user filter |
No, it cannot. On some LDAP servers it's not possible to determine the user's group by a user query, the only way to know wether a user is in a group is by a group query. (EDIT: there is no memberOf attribute) In my case, I want to restrict access to members of a group. |
|
Just a heads-up from me. I am happy to see this feature being worked on. Looking forward to see this in one of the next releases. |
(as suggested by zeripath) Co-Authored-By: zeripath <[email protected]>
Co-Authored-By: 6543 <[email protected]>
There was a problem hiding this comment.
As general comments,
- I don't feel the relations between all 6 conditions fields is clearly managed and explicited, I'm taking about:
Filter/MemberGroupFilter,AdminFilter/AdminGroupFilter,RestrictedFilter/RestrictedGroupFilterand their relation in pairs (if both provided), and in between each 3 couple of conditions.
This is why I have this feeling:- All the behavior do not seem documented and maybe should as it is different for each pair.
- Is it clear that user should satisfy both Filter and MemberFilter conditions (if provided) ?
- Is it clear that admins and restricted user should satisfy user conditions ? Do we want that ?
- Is it clear that restricted user should NOT satisfy admins conditions ?
- In current code and current behavior, we issue useless LDAP queries
- no need to check
RestrictedGroupFilterif user is admin - no need to check
AdminGroupFilterifAdminFilteris already satisfied (because you chose toorboth conditions, which I'm not sure is the best).
- no need to check
- All the behavior do not seem documented and maybe should as it is different for each pair.
- There are no tests in
integrations/auth_ldap_test.gofor restricted users. I think it could be important, especially considered what behavior we want to stick in light of the previous questions.
Many thanks for your contribution. I have developed a similar working code and went through these consideration myself before actually discovering your PR. I would be happy to help to get your code included as soon as possible.
I hope my remarks are helpful, and sorry if I missed some obvious points.
Thank you @vaab those are good catches Co-Authored-By: Valentin Lab <[email protected]>
Co-Authored-By: Valentin Lab <[email protected]>
|
Let me put blocking points here about this implementation in my perspective :
I'll be happy to move forward by addressing these points. Not sure when though. |
- If both admin filter and admin group filter are set, the user must pass both tests (before, the conditions were ORed) - Same for restricted users - Log restricted group membership regardless of admin group membership (Maybe we should display a warning in that case) - Rename variables 'hasXxxGroup' to 'isInXxxGroup'
|
@vaab I made some improvements regarding the relations of the six different user/group filters.
|
|
What is the Status? |
Codecov Report
@@ Coverage Diff @@
## master #8814 +/- ##
==========================================
+ Coverage 43.79% 43.94% +0.14%
==========================================
Files 607 607
Lines 86876 86968 +92
==========================================
+ Hits 38045 38215 +170
+ Misses 44133 44051 -82
- Partials 4698 4702 +4
Continue to review full report at Codecov.
|
As our LDAP infrastructure changed (supporting the So, if there's interest in working on that feature, you may submit a new PR or merge this one. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions. |
|
Closing in favour of other PR |
Continuation of #7349, which was closed after the commits were moved to a different branch and the master branch was reset to the original master branch.
Targeting #2212.
Features:
Sub-tasks