iframe-proxy

MateenAkram1 · 2026-10-21T12:07:52Z

Proposed changes (including videos or screenshots)

Removes the block in LDAPManager.syncUserForLogin that copied the existing user's roles and type into the LDAP data before conversion, along with its TODO.

The block was added in #23378 because the import converter used to reset roles and type to their defaults on every update. A month later, #23608 changed UserConverter.updateUser to apply those defaults only when both the incoming data and the existing user have no value. Since then the block has had no effect on the stored user:

The background LDAP sync (LDAPEEManager.updateExistingUsers) already passes existing users through the same LDAPUserConverter without this block.

The converter keeps its defaults, because newly inserted users still need them.

Two commits:

test: three unit tests for UserConverter.updateUser:
- existing roles/type are left alone when the data has none
- the defaults are applied when neither side has a value
- incoming values replace existing ones
No test covered this before. The tests pass on the current code. With the pre-[FIX] Advanced LDAP Sync Features #23608 unconditional defaults put back, the first test fails.
refactor: remove the block from Manager.ts.

No changeset: there is no user-visible change.

Issue(s)

Closes #42981

Steps to test or reproduce

cd apps/meteor && yarn .testunit:server: 3129 passing (3126 before, plus the 3 new tests), 0 failing. The run includes ee/server/lib/ldap/**/*.spec.ts.
yarn .testunit:jest, yarn .testunit:definition and tsc --noEmit --skipLibCheck pass. ESLint reports no new warnings on the touched files.
A fresh yarn dev boots without errors.

I did not test a live LDAP login, because I don't have a Premium license and LDAP server locally. The table above shows the stored result is the same in every case.

Further comments

Things I noticed nearby and left out to keep this to one concern. Happy to open separate PRs for them:

UserConverter.updateUser calls Users.updateOne({ _id }, updateData) with an inline filter instead of a named model method (see docs/database-access.md).
LDAPManager.onMapUserData fires callbacks.run('mapLDAPUserData', …) with void instead of awaiting it.
mapUserData always sets type: 'user', so an existing bot account that is synced from LDAP is changed to user. This is existing behavior and this PR does not change it.

Summary by CodeRabbit

Bug Fixes
- Improved LDAP user synchronization so imported role and account-type data is handled consistently.
- Preserved existing roles and account types when incoming user data does not specify replacements.
- Applied standard user defaults when neither existing nor imported values are available.
- Correctly replaced existing roles and account types when explicitly provided by imported data.

dionisio-bot · 2026-10-21T12:07:57Z

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

This PR is missing the 'stat: QA assured' label
This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

changeset-bot · 2026-10-21T12:07:57Z

⚠️ No Changeset found

Latest commit: b81500d

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

CLAassistant · 2026-10-21T12:08:48Z

All committers have signed the CLA.

coderabbitai · 2026-10-21T12:09:40Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: e88c7a2d-95e3-417f-bb17-a1edc4b9dbb3

📥 Commits

Reviewing files that changed from the base of the PR and between 9209eae and b81500d.

📒 Files selected for processing: 2

apps/meteor/ee/server/lib/ldap/Manager.ts
apps/meteor/tests/unit/server/lib/import/userConverter.spec.ts

💤 Files with no reviewable changes: 1

apps/meteor/ee/server/lib/ldap/Manager.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📜 Recent review details

🔇 Additional comments: 2

apps/meteor/tests/unit/server/lib/import/userConverter.spec.ts: 2

11-12: LGTM!

Also applies to: 71-72

495-531: LGTM!

Walkthrough

The LDAP login path no longer copies an existing user’s roles and type into mapped data before conversion. Tests cover how user updates handle omitted roles and type, default values, and imported values.

Changes

LDAP user update

Layer / File(s)	Summary
Roles and type update behavior `apps/meteor/ee/server/lib/ldap/Manager.ts`, `apps/meteor/tests/unit/server/lib/import/userConverter.spec.ts`	The LDAP manager no longer copies existing roles and type into mapped data before conversion. Tests cover preserving existing values when omitted, applying defaults when neither source provides values, and replacing values with imported roles and type.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor

Suggested labels: type: chore, area: authentication

Suggested reviewers: sampaiodiego

Merge Risk: ⚪ Minimal · up to b8150

Existing LDAP-linked users retain their roles on login. No change requiring correction before merge was established.

Pre-merge checks: 5

Full details: Linked Issues check

Explanation

Issue #42981 identifies the LDAP sync copy as a workaround for converter defaults. The PR removes that workaround from LDAPManager.syncUserForLogin. The current UserConverter.updateUser applies defaults only when both the imported data and existing user lack a value. The added tests cover preservation, default application, and replacement. The converter defaults remain available for new users.

Full details: Out of Scope Changes check

Explanation

The changes are limited to removing the redundant LDAP sync block and adding unit coverage for the converter behavior that makes the removal safe. The tests directly support issue #42981. No unrelated production behavior is changed.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Errors were encountered while retrieving linked issues.

Errors: 1

JIRA integration encountered authorization issues. Please disconnect and reconnect the integration in the CodeRabbit UI.

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

No issues found across 2 files

_{View guided diff | Re-trigger cubic}

MateenAkram1 added 2 commits October 21, 2026 16:39

test: cover UserConverter.updateUser keeping existing roles and type

c3b4693

refactor: drop LDAP login sync's copy of existing roles and type

b81500d

MateenAkram1 requested a review from a team as a code owner October 21, 2026 12:07

dionisio-bot Bot added Invalid PR Title community labels Oct 21, 2026

MateenAkram1 changed the title ~~Refactor/ldap redundant roles type sync~~ refactor: remove redundant roles/type copy from LDAP login sync Oct 21, 2026

cubic-dev-ai Bot reviewed Oct 21, 2026

View reviewed changes

dionisio-bot Bot removed the Invalid PR Title label Oct 21, 2026

coderabbitai Bot added area: authentication type: chore labels Oct 21, 2026

Case	With the block	Without it
New user (`existingUser` undefined)	block skipped	same
Existing user with roles	`$set` writes back the same roles	`roles` is not written; the stored value is kept
Existing user without roles	converter sets `['user']`	converter sets `['user']`
`type`	`mapUserData` always sets `type: 'user'`, so this branch never ran	same

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Repository navigation

refactor: remove redundant roles/type copy from LDAP login sync - #42993

refactor: remove redundant roles/type copy from LDAP login sync#42993
MateenAkram1 wants to merge 2 commits into
RocketChat:developfrom
MateenAkram1:refactor/ldap-redundant-roles-type-sync

MateenAkram1 commented Oct 21, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

dionisio-bot Bot commented Oct 21, 2026 •

edited

Loading

Uh oh!

changeset-bot Bot commented Oct 21, 2026

Uh oh!

CLAassistant commented Oct 21, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 21, 2026 •

edited

Loading

Uh oh!

cubic-dev-ai Bot left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

MateenAkram1 commented Oct 21, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

Summary by CodeRabbit

Uh oh!

dionisio-bot Bot commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot Bot commented Oct 21, 2026

⚠️ No Changeset found

Uh oh!

CLAassistant commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MateenAkram1 commented Oct 21, 2026 •

edited by coderabbitai Bot

Loading

dionisio-bot Bot commented Oct 21, 2026 •

edited

Loading

CLAassistant commented Oct 21, 2026 •

edited

Loading

coderabbitai Bot commented Oct 21, 2026 •

edited

Loading