Skip to content

fix: LDAP users not merging by email during sync - #41279

Merged
dionisio-bot[bot] merged 1 commit into
developfrom
fix/ldap-merge-existing-user-by-email
Jul 11, 2026
Merged

fix: LDAP users not merging by email during sync#41279
dionisio-bot[bot] merged 1 commit into
developfrom
fix/ldap-merge-existing-user-by-email

Conversation

@abhinavkrin

@abhinavkrin abhinavkrin commented Jul 9, 2026

Copy link
Copy Markdown
Member

Proposed changes (including videos or screenshots)

During LDAP sync, when an existing (non-LDAP) Rocket.Chat user has the same email as a directory entry but a different username, the sync should merge them. It wasn't: the email fallback used during sync never matched, so the sync treated the entry as new and attempted an insert, colliding on the unique username and failing with Username already exists. [403].

This corrects the email lookup so the existing user is found and merged instead of duplicated.

Issue(s)

Closes: CORE-1285

Steps to test or reproduce

  1. Configure LDAP against a directory; create a local Rocket.Chat user whose email matches a directory user but with a different username.
  2. Enable LDAP_Merge_Existing_Users and run Sync Now.
  3. Expected: the existing user is merged and linked to LDAP (no Username already exists error, no duplicate account).

Further comments

CORE-1285

Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
@abhinavkrin
abhinavkrin requested a review from a team as a code owner July 9, 2026 14:43
@dionisio-bot

dionisio-bot Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

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

@changeset-bot

changeset-bot Bot commented Jul 9, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: a54d252

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

This PR includes changesets to release 4 packages
Name Type
@rocket.chat/models Patch
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings 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

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The MongoDB query field used in findOneWithoutLDAPByEmailAddress was corrected from email.address to emails.address to properly match user emails stored in the emails array. A changeset was added documenting this fix as a patch release for two packages.

Changes

LDAP email lookup fix

Layer / File(s) Summary
Query field correction
packages/models/src/models/Users.ts
Fixes the email match key in findOneWithoutLDAPByEmailAddress from email.address to emails.address so the query correctly searches the emails array.
Changeset documentation
.changeset/ldap-merge-email-lookup.md
Adds a patch changeset for @rocket.chat/models and @rocket.chat/meteor describing the LDAP sync fix that prevents a Username already exists error when merging users matched by email.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Suggested labels: type: bug, area: authentication

Suggested reviewers: ricardogarim, jessicaschelly

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: fixing LDAP user merging by email during sync.

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)
  • CORE-1285: Request failed with status code 401

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues found across 2 files

Re-trigger cubic

@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.44%. Comparing base (f633d7e) to head (a54d252).
⚠️ Report is 7 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #41279      +/-   ##
===========================================
- Coverage    69.52%   69.44%   -0.08%     
===========================================
  Files         3663     3663              
  Lines       145125   145125              
  Branches     25726    25746      +20     
===========================================
- Hits        100896   100786     -110     
- Misses       39945    40046     +101     
- Partials      4284     4293       +9     
Flag Coverage Δ
e2e 59.35% <ø> (-0.09%) ⬇️
e2e-api 50.30% <ø> (-0.49%) ⬇️
unit 70.47% <ø> (-0.05%) ⬇️

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@abhinavkrin abhinavkrin added this to the 8.7.0 milestone Jul 9, 2026
@abhinavkrin abhinavkrin added the stat: QA assured Means it has been tested and approved by a company insider label Jul 9, 2026
@dionisio-bot dionisio-bot Bot added the stat: ready to merge PR tested and approved waiting for merge label Jul 9, 2026
@abhinavkrin abhinavkrin removed the stat: QA assured Means it has been tested and approved by a company insider label Jul 10, 2026
@dionisio-bot dionisio-bot Bot removed the stat: ready to merge PR tested and approved waiting for merge label Jul 10, 2026
@abhinavkrin abhinavkrin added stat: ready to merge PR tested and approved waiting for merge stat: QA assured Means it has been tested and approved by a company insider labels Jul 10, 2026
@dionisio-bot
dionisio-bot Bot added this pull request to the merge queue Jul 10, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Jul 10, 2026
@dionisio-bot
dionisio-bot Bot added this pull request to the merge queue Jul 10, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 11, 2026
@dionisio-bot
dionisio-bot Bot added this pull request to the merge queue Jul 11, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 11, 2026
@dionisio-bot
dionisio-bot Bot added this pull request to the merge queue Jul 11, 2026
Merged via the queue into develop with commit 719e3db Jul 11, 2026
84 of 86 checks passed
@dionisio-bot
dionisio-bot Bot deleted the fix/ldap-merge-existing-user-by-email branch July 11, 2026 06:32
This was referenced Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: authentication stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge type: bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants