[PM-11981] Support LDAP membership with UID#841
Conversation
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #841 +/- ##
=========================================
+ Coverage 7.72% 13.15% +5.42%
=========================================
Files 68 68
Lines 2757 2775 +18
Branches 477 482 +5
=========================================
+ Hits 213 365 +152
+ Misses 2529 2381 -148
- Partials 15 29 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks for the PR Sugi!
This adds uid support, but #177 also mentions supporting nested groups specified by cn. Do you need support for nested groups by cn as well, or is not blocking at this stage? Either way I'm OK with doing that in another PR.
The main thing missing here is test coverage. Our current integration tests are in ldap-directory.service.integration.spec.ts. More info in the contributing docs, but basically it spins up a local ldap instance and performs a sync to make sure the test directory data matches the expected sync requests sent to the Bitwarden server.
You would need to add some members and groups using uids and then assert that they are correctly parsed. Existing tests should provide examples.
EDIT: it would also be ideal to add a nested group in our test data, as that is untested at the moment but this logic is touched on by your changes.
| @@ -193,13 +193,20 @@ export class LdapDirectoryService implements IDirectoryService { | |||
| ); | |||
| const userPath = this.makeSearchPath(this.syncConfig.userPath); | |||
| const userIdMap = new Map<string, string>(); | |||
There was a problem hiding this comment.
To match my suggestion above: userIdMap -> userDnMap
eliykat
changed the title
|
New Issues (1)Checkmarx found the following issues in this Pull Request
|
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
A few minor tweaks, then we can pass it to QA 🙌
Please re-request my review once you've made the changes.
sso-bitwarden
force-pushed
the
fixgroupMember
branch
from
f3fa321 to
4af8e5f
Compare
eliykat
force-pushed
the
fixgroupMember
branch
from
b1be9bc to
6081cae
Compare
This reverts commit 88ebe32.
|
4 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-11981
📔 Objective
LDAP could have many structures. For group membership, it can be listed by DN and by UID. The current Directory Connector could handle DN, but failed to cater to UID. This PR is to improve and cater for both DN and UID
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes