Skip to content
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

feat: Add LDAP group validation strategy setting to channels and roles sync #32436

Merged
merged 7 commits into from
Jun 21, 2024

Conversation

matheusbsilva137
Copy link
Member

@matheusbsilva137 matheusbsilva137 commented May 14, 2024

Proposed changes (including videos or screenshots)

  • Added "Group membership validation strategy" settings to "Sync Channels" and "Sync Roles" LDAP sections -- they may enable a faster sync process when combined with a good LDAP search filter;

Available strategies:

  • Apply filter for each group: apply the LDAP user group filter for each group (key) defined in the LDAP group channel map. This is slower, but can be useful in case you need to use the #{groupName} replacement tag to define membership (e.g. when filtering by the memberOf field in groups);
  • Apply filter once to get all memberships: apply the LDAP user group filter once for each user. A given user will be considered a member of all groups returned by the LDAP search. This is a faster option that can be applied in case the #{groupName} replacement tag is not used by the filter (e.g. when filtering by the member field in groups).

Issue(s)

Steps to test or reproduce

The new "Group membership validation strategy" setting is available under both "Sync Channels" and "Sync Roles" sections in LDAP Premium settings. Both features should work just the same as in previous version when using the default "Apply filter for each group" search strategy or the new and faster "Apply filter once to get all memberships" strategy -- the only difference here is the amount of LDAP search requests triggered by RC.

Sample configuration

Sample configuration for using the "Apply filter once to get all memberships" search strategy:
Captura de tela de 2024-05-14 17-09-13

Sample configuration for using the "Apply filter for each group" search strategy:
Captura de tela de 2024-05-14 17-09-51

Caution

Switching to the new and faster "Apply filter once to get all memberships" search strategy may not work with the currently configured LDAP search filter!
When switching to the new strategy, make sure to update the user group filter so as to get all groups at once in a single query with it (be sure not to use the #{groupName} replacement tag since it is not supported by this new strategy)

Further comments

CORE-402

@matheusbsilva137 matheusbsilva137 added this to the 6.9 milestone May 14, 2024
Copy link
Contributor

dionisio-bot bot commented May 14, 2024

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

Copy link

changeset-bot bot commented May 14, 2024

🦋 Changeset detected

Latest commit: 315fd04

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 34 packages
Name Type
@rocket.chat/meteor Minor
@rocket.chat/i18n Minor
@rocket.chat/mock-providers Patch
@rocket.chat/ui-contexts Major
@rocket.chat/web-ui-registration Major
@rocket.chat/fuselage-ui-kit Major
@rocket.chat/ui-client Major
@rocket.chat/gazzodown Major
@rocket.chat/livechat Patch
@rocket.chat/ui-avatar Major
@rocket.chat/ui-video-conf Major
@rocket.chat/uikit-playground Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/core-typings Minor
@rocket.chat/rest-typings Minor
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/model-typings Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/api-client Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/ddp-client Patch
@rocket.chat/models Patch
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

codecov bot commented May 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.55%. Comparing base (1428778) to head (315fd04).
Report is 3 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #32436      +/-   ##
===========================================
- Coverage    56.56%   56.55%   -0.01%     
===========================================
  Files         2484     2484              
  Lines        54755    54755              
  Branches     11308    11308              
===========================================
- Hits         30971    30966       -5     
- Misses       21107    21109       +2     
- Partials      2677     2680       +3     
Flag Coverage Δ
e2e 56.19% <ø> (-0.06%) ⬇️
e2e-api 41.34% <ø> (-0.02%) ⬇️
unit 72.23% <ø> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@matheusbsilva137 matheusbsilva137 marked this pull request as ready for review May 15, 2024 01:07
Copy link
Contributor

@pierre-lehnen-rc pierre-lehnen-rc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR should still be flagged as a breaking change since it'll no longer be possible to use both strategies at the same time.

There should also be a migration to select the right strategy automatically if only one of them is currently being used.

Ignore this review, I see now that the PR is making a different change than the one we had discussed last time.

apps/meteor/ee/server/lib/ldap/Manager.ts Outdated Show resolved Hide resolved
Co-authored-by: Pierre Lehnen <55164754+pierre-lehnen-rc@users.noreply.github.com>
@scuciatto scuciatto modified the milestones: 6.9, 6.10 May 21, 2024
@jessicaschelly jessicaschelly added the stat: QA assured Means it has been tested and approved by a company insider label Jun 20, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jun 20, 2024
@dionisio-bot dionisio-bot bot removed the stat: ready to merge PR tested and approved waiting for merge label Jun 20, 2024
@ggazzo ggazzo merged commit 363a011 into develop Jun 21, 2024
49 checks passed
@ggazzo ggazzo deleted the feat/ldap-group-validation-strategy branch June 21, 2024 02:38
This was referenced Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA assured Means it has been tested and approved by a company insider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants