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!: Merge LDAP Teams Sync and Channels Sync into a new Rooms Sync section #32390

Draft
wants to merge 4 commits into
base: release-7.0.0
Choose a base branch
from

Conversation

matheusbsilva137
Copy link
Contributor

@matheusbsilva137 matheusbsilva137 commented May 8, 2024

Proposed changes (including videos or screenshots)

  • Merge "Teams Sync" and "Channels Sync" LDAP settings into a new "Room Sync" section that is capable of syncing channels, groups and teams;
  • Update settings' names and descriptions to reflect the new organization;
  • Improve rooms sync performance by having a single LDAP search request per user (instead of one per membership);
  • Stop supporting the #{groupName} replacement in the LDAP Sync Rooms "Search Filter" (since the filter is now applied once per user);
  • Fixed issue with Channel Sync only working in case the filter was applied to the memberOf field (RC didn't bind to the admin before applying the LDAP search request to get user groups, so only the logged in user itself would be visible).

More details about settings transition to the new organization
Old setting New Setting
LDAP_Sync_User_Data_Channels LDAP_Sync_User_Data_Rooms
LDAP_Sync_User_Data_Channels_Admin LDAP_Sync_User_Data_Rooms_Admin
LDAP_Sync_User_Data_Channels_Filter LDAP_Sync_User_Data_Rooms_Filter
LDAP_Sync_User_Data_Channels_BaseDN LDAP_Sync_User_Data_Rooms_BaseDN
LDAP_Sync_User_Data_ChannelsMap LDAP_Sync_User_Data_RoomsMap
LDAP_Sync_User_Data_Channels_Enforce_AutoChannels LDAP_Sync_User_Data_Rooms_Auto_Leave
LDAP_Teams_Name_Field LDAP_Group_Name_Field
LDAP_Enable_LDAP_Groups_To_RC_Teams Removed (use LDAP_Sync_User_Data_Rooms instead)
LDAP_Groups_To_Rocket_Chat_Teams Removed (use LDAP_Sync_User_Data_RoomsMap instead)
LDAP_Validate_Teams_For_Each_Login Removed (use LDAP_Validate_Rooms_For_Each_Login instead)
LDAP_Teams_BaseDN Removed (use LDAP_Sync_User_Data_Rooms_BaseDN instead)
LDAP_Query_To_Get_User_Teams Removed (use LDAP_Sync_User_Data_Rooms_Filter instead)

Settings comparison (before and after):

ldap-settings-replacement


Caution

Teams sync must be reconfigured using the "Rooms Sync" setting
Channels sync must also be reconfigured in case the #{groupName} replacement (which is not supported anymore) was being used -- prefer filtering by the member attribute in groups instead of using the memberOf field to avoid this.

Issue(s)

Steps to test or reproduce

Demo:

ldap-rooms-sync-test.webm

Further comments

CORE-402

@matheusbsilva137 matheusbsilva137 added this to the 7.0 milestone May 8, 2024
Copy link
Contributor

dionisio-bot bot commented May 8, 2024

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

Copy link

changeset-bot bot commented May 8, 2024

⚠️ No Changeset found

Latest commit: 62b6189

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@matheusbsilva137 matheusbsilva137 changed the title feat: Merge LDAP Teams Sync and Channels Sync into a new Rooms Sync section feat!: Merge LDAP Teams Sync and Channels Sync into a new Rooms Sync section May 8, 2024
Copy link

codecov bot commented May 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.55%. Comparing base (d2530d3) to head (62b6189).

Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                @@
##           release-7.0.0   #32390      +/-   ##
=================================================
- Coverage          55.56%   55.55%   -0.02%     
=================================================
  Files               2408     2408              
  Lines              53019    53019              
  Branches           10902    10902              
=================================================
- Hits               29460    29453       -7     
- Misses             20941    20951      +10     
+ Partials            2618     2615       -3     
Flag Coverage Δ
e2e 55.02% <ø> (-0.02%) ⬇️
unit 72.86% <ø> (-0.03%) ⬇️

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

invalidValue: 'rocket.cat',
});

await this.add('LDAP_Sync_User_Data_Channels_Filter', '(&(cn=#{groupName})(memberUid=#{username}))', {
await this.add('LDAP_Sync_User_Data_Rooms_Filter', '(&(cn=#{groupName})(memberUid=#{username}))', {
Copy link
Contributor

Choose a reason for hiding this comment

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

The #{groupName} variable no longer exists for this search, so this default value is guaranteed to be invalid.

Also: change this into a dynamic setting with one default value for AD and another for non-AD, in the same way as some other settings here already work.

@ggazzo ggazzo force-pushed the release-7.0.0 branch 2 times, most recently from 3d2eba3 to d2530d3 Compare May 15, 2024 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants