mirror of
https://github.com/pocket-id/pocket-id.git
synced 2025-12-06 09:13:19 +03:00
🐛 Bug Report: Expand LDAP group membership logic to fully support Active Directory group member DNs with non-username value. #211
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @abjoseph on GitHub.
Originally assigned to: @kmendell on GitHub.
Reproduction steps
I have an Active Directory instance on-prem that I currently use to centrally manage users and groups within my environment.
Expected behavior
I expect that Pocket ID, once configured for LDAP Sync. is able to determine user group membership and correctly reflect that in the "User Group" tab in the Admin UI.
Actual Behavior
Pocket ID successfully creates the desired groups but is not able to correctly associate groups with the corresponding queried users.
Version and Environment
Expand the LDAP group membership logic originally implemented as part of https://github.com/pocket-id/pocket-id/issues/234 to ALSO support Active Directory's Distinguished Name (DN) Format for the group member value. e.g.
CN=John Doe,OU=Users,OU=Security,DC=Example,DC=comwhere theCNis usually the user's "Full Name" if populated and likely would not match the value retrieved from the username attribute, which in my case issAMAccountName.The currently implemented assumption, which is not valid for the AD scenario I described above, can be found in file
backend/internal/service/ldap_service.go at line 110Code Snippet of the line referenced above show below:
Log Output
No relevant logs, as an FYI - Troubleshooting surrounding LDAP sync would benefit greatly from improved/more verbose logging.
@kmendell commented on GitHub:
@abjoseph Can you try this image
ghcr.io/pocket-id/pocket-id:ad-fixand see if its fixed? It will use the ldap user attribute to compare for groups now instead of hardcoding the uid attribute. So make sure you have the sAMAccountName set for that.@abjoseph commented on GitHub:
@kmendell Thanks for working on this, unfortunately I was not able to test the change as I encountered issues with the image.
The listening port seems to have changed from 80 to 1411.
Also, I don't think even that port is working correctly as the "Login" page no longer works and no help logs are shown in the terminal.
@kmendell commented on GitHub:
Ahh okay understood, Ill look into this.
@kmendell commented on GitHub:
If you use one of the search filters to base it off of the sAMAccountName does it still not work? The where lines your referencing are for the pocket-id database nothing to do with LDAP specificially
@abjoseph commented on GitHub:
Hello @kmendell, I'm already using a search filter for both users and groups, I know they're working since both users and groups are correctly populated in the Admin UI. The issue here is not the search filter, it's with the comparison logic on line 110 and the assumption it's based on.
E.g.
Given a user with a sAMAccountName of
jdoeanda a DN value ofCN=John Doe,OU=Users,OU=Security,DC=Example,DC=comthe logic on line 107singleMember := strings.Split(strings.Split(member, "=")[1], ",")[0]would yield a result ofJohn Doe. However, following the user search and creation bit,John Doewould have been inserted into the database with a username value ofjdoe.Returning to line 110, that bit of code would result in a comparison of
singleMember == usernamewhere singleMbember isJohn Doeand username isjdoe. The comparison fails and the group is never updated with the user that should have been marked as a member.@kmendell commented on GitHub:
hmm i dont have an AD Setup to test with but in theory this should work... Can you repull the image and try again? I made somemore changes taht i hope will fix it..
@abjoseph commented on GitHub:
@stonith404 @kmendell Ok, so I successfully migrated to v1.0.0 and then switched to the "ad-fix" image.
I performed the following steps:
Keep disabled users from LDAP.and then made some changes to the queries such that no users or groups would be found.Sync nowbutton.Sync now.An aside: I'm not sure if it is security related or not but the logs do not show any errors or output related to the LDAP sync function.
Below is my LDAP configuration for context:
@abjoseph commented on GitHub:
Those last changes fixed it!!, see screenshot below. Thank you again for the time and effort. FYI - I quite like this project and OSS in general; I also have a pretty extensive setup so if you need someone to be an extra pair of hands to test things, let me know.
@abjoseph commented on GitHub:
@stonith404 Completely missed that, I'll do the migration steps first and report back.
@stonith404 commented on GitHub:
@abjoseph Oh, you have to upgrade to
v1.0.0first. This release contains breaking changes so make sure to follow the migration docs.