refactor: migrate ldap endpoints to new chained API pattern#38883
Open
smirk-dev wants to merge 1 commit intoRocketChat:developfrom
Open
refactor: migrate ldap endpoints to new chained API pattern#38883smirk-dev wants to merge 1 commit intoRocketChat:developfrom
smirk-dev wants to merge 1 commit intoRocketChat:developfrom
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 7fcf5fe The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
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 |
Contributor
WalkthroughTwo LDAP REST endpoints (ldap.testConnection and ldap.testSearch) were migrated from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/meteor/app/api/server/v1/ldap.ts (2)
76-76: Inconsistentsettings.getgeneric — add<boolean>for parity with line 42.Line 42 uses
settings.get<boolean>('LDAP_Enable')but the equivalent call inldap.testSearchomits the type parameter, weakening the type.♻️ Proposed fix
- if (settings.get('LDAP_Enable') !== true) { + if (settings.get<boolean>('LDAP_Enable') !== true) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/ldap.ts` at line 76, In ldap.testSearch, the settings.get call is missing the generic type parameter; update the call to use settings.get<boolean>('LDAP_Enable') (matching the usage on line 42) so the boolean return type is enforced and TypeScript type safety is consistent for the LDAP_Enable check within the ldap.testSearch function.
38-40: Redundant!this.userIdguard — dead code whenauthRequired: true.With
authRequired: true, the framework rejects unauthenticated requests beforeaction()is invoked, sothis.userIdis always defined at this point. The manual check is unreachable.♻️ Proposed cleanup
async function action() { - if (!this.userId) { - throw new Error('error-invalid-user'); - } - if (settings.get<boolean>('LDAP_Enable') !== true) {Apply the same change in the
ldap.testSearchaction.Also applies to: 72-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/ldap.ts` around lines 38 - 40, The explicit "if (!this.userId) { throw new Error('error-invalid-user'); }" guard is dead code because authRequired: true already ensures this.userId exists; remove that guard from the action that contains it and from the ldap.testSearch action so the methods no longer perform the redundant check, leaving the rest of the action logic intact and relying on the framework's authentication enforcement.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/refactor-ldap-api-chained-pattern.mdapps/meteor/app/api/server/v1/ldap.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/api/server/v1/ldap.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/ldap.ts (2)
packages/core-services/src/index.ts (1)
LDAP(166-166)packages/rest-typings/src/v1/ldap.ts (1)
isLdapTestSearch(18-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
.changeset/refactor-ldap-api-chained-pattern.md (1)
1-5: LGTM!Changeset accurately describes the migration; patch bump level is appropriate for a refactor with no user-visible behavior changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/api/server/v1/ldap.ts`:
- Line 32: The ajv.compile calls currently use an incorrect generic type ({
message: string }) that omits the required success field; update the type
parameter on both ajv.compile invocations (the ones compiling
messageResponseSchema) to match the schema shape (e.g., include success as true
or boolean alongside message, such as { success: true; message: string }) so the
TypeScript type reflects the schema; apply this same change to the second
compile call referenced in the review.
- Around line 71-85: The LDAP.testSearch call in the action function lacks the
try/catch used by ldap.testConnection, so failures leak raw errors and aren't
logged; wrap the await LDAP.testSearch(this.bodyParams.username) call in a
try/catch, log the caught error via SystemLogger.error (include context like
'LDAP.testSearch failed'), and re-throw a controlled, user-friendly error
(mirror the pattern used by LDAP.testConnection, e.g., throw new
Error('Search_failed') or the same conventional message) so the behavior and
logging are consistent with LDAP.testConnection.
---
Nitpick comments:
In `@apps/meteor/app/api/server/v1/ldap.ts`:
- Line 76: In ldap.testSearch, the settings.get call is missing the generic type
parameter; update the call to use settings.get<boolean>('LDAP_Enable') (matching
the usage on line 42) so the boolean return type is enforced and TypeScript type
safety is consistent for the LDAP_Enable check within the ldap.testSearch
function.
- Around line 38-40: The explicit "if (!this.userId) { throw new
Error('error-invalid-user'); }" guard is dead code because authRequired: true
already ensures this.userId exists; remove that guard from the action that
contains it and from the ldap.testSearch action so the methods no longer perform
the redundant check, leaving the rest of the action logic intact and relying on
the framework's authentication enforcement.
smirk-dev
force-pushed
the
refactor/migrate-ldap-api-chained-pattern
branch
from
3f8168f to
8d2307c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/meteor/app/api/server/v1/ldap.ts (2)
42-44: Redundantthis.userIdguard whenauthRequired: trueis already set.
authRequired: trueguarantees the route is reached only by an authenticated user, making the!this.userIdthrow unreachable. The same pattern repeats at lines 76–78. If the check is retained purely for TypeScript type-narrowing purposes, a non-throwing assertion or a type cast would be more idiomatic.♻️ Proposed simplification (if no TS narrowing is needed)
- async function action() { - if (!this.userId) { - throw new Error('error-invalid-user'); - } - - if (settings.get<boolean>('LDAP_Enable') !== true) { + async function action() { + if (settings.get<boolean>('LDAP_Enable') !== true) {Apply the same change to lines 76–78.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/ldap.ts` around lines 42 - 44, Remove the redundant runtime guard that throws on !this.userId inside handlers protected by authRequired: true (the throw new Error('error-invalid-user') check shown); either delete the throw block entirely (since authRequired guarantees an authenticated user) or replace it with a non-throwing TypeScript assertion/cast to satisfy type narrowing (e.g., assertExists(this.userId) or const userId = this.userId as string). Apply the same change to the repeated check at the later occurrence (lines 76–78) so both handlers no longer contain the unreachable throw.
21-28: Remove the localisLdapTestSearchdefinition and import it from@rocket.chat/rest-typings.The codebase already exports an identical
isLdapTestSearchvalidator from@rocket.chat/rest-typings(packages/rest-typings/src/v1/ldap.ts:18). The local definition at lines 21–28 duplicates this exact schema and should be replaced with an import to maintain consistency with the shared type definitions.Proposed fix
-import { ajv, validateUnauthorizedErrorResponse, validateForbiddenErrorResponse } from '@rocket.chat/rest-typings'; +import { ajv, isLdapTestSearch, validateUnauthorizedErrorResponse, validateForbiddenErrorResponse } from '@rocket.chat/rest-typings';-const isLdapTestSearch = ajv.compile<{ username: string }>({ - type: 'object', - properties: { - username: { type: 'string' }, - }, - required: ['username'], - additionalProperties: false, -}); -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/ldap.ts` around lines 21 - 28, Replace the local duplicate validator by removing the local const isLdapTestSearch definition and instead import the shared validator from `@rocket.chat/rest-typings`; update the top-level imports to add isLdapTestSearch and remove the AJV inline schema, and ensure any usage in this file references the imported isLdapTestSearch symbol (e.g., where the local const was used) so the module uses the centralized validator.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/refactor-ldap-api-chained-pattern.mdapps/meteor/app/api/server/v1/ldap.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/api/server/v1/ldap.ts
🧠 Learnings (2)
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/api/server/v1/ldap.ts
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/app/api/server/v1/ldap.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/ldap.ts (3)
packages/rest-typings/src/v1/ldap.ts (1)
isLdapTestSearch(18-18)apps/meteor/app/api/server/index.ts (1)
API(51-51)packages/core-services/src/index.ts (1)
LDAP(166-166)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
.changeset/refactor-ldap-api-chained-pattern.md (1)
1-5: LGTM!Changeset entry is correctly scoped (
patch) and the description accurately reflects the migration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/api/server/v1/ldap.ts`:
- Line 80: The check using settings.get on 'LDAP_Enable' is missing the boolean
generic causing an unsafe inferred type; update the call in the LDAP enabling
check to use settings.get<boolean>('LDAP_Enable') (matching the earlier call on
line 46) so the result is typed as boolean and the comparison to true remains
type-safe.
---
Duplicate comments:
In `@apps/meteor/app/api/server/v1/ldap.ts`:
- Line 36: The AJV compile calls use an incorrect generic that omits the
required success field; update the generics used with ajv.compile for
messageResponseSchema (both occurrences, e.g., the ajv.compile<{ message: string
}>(messageResponseSchema) instances) to include success constrained to true
(e.g., { message: string; success: true }) so the TypeScript type matches the
JSON schema.
- Around line 75-89: The action function calling
LDAP.testSearch(this.bodyParams.username) lacks error handling; wrap the call in
a try/catch inside the action to catch any thrown errors from LDAP.testSearch,
log the error via the same logger used elsewhere (or processLogger) and return a
user-friendly API.v1.failure (e.g. 'LDAP_test_failed' or 'LDAP_User_Not_Found')
instead of letting the raw error propagate; update the action function
surrounding LDAP.testSearch to mirror the error-handling pattern used by
ldap.testConnection so errors are logged and a sanitized failure response is
returned.
---
Nitpick comments:
In `@apps/meteor/app/api/server/v1/ldap.ts`:
- Around line 42-44: Remove the redundant runtime guard that throws on
!this.userId inside handlers protected by authRequired: true (the throw new
Error('error-invalid-user') check shown); either delete the throw block entirely
(since authRequired guarantees an authenticated user) or replace it with a
non-throwing TypeScript assertion/cast to satisfy type narrowing (e.g.,
assertExists(this.userId) or const userId = this.userId as string). Apply the
same change to the repeated check at the later occurrence (lines 76–78) so both
handlers no longer contain the unreachable throw.
- Around line 21-28: Replace the local duplicate validator by removing the local
const isLdapTestSearch definition and instead import the shared validator from
`@rocket.chat/rest-typings`; update the top-level imports to add isLdapTestSearch
and remove the AJV inline schema, and ensure any usage in this file references
the imported isLdapTestSearch symbol (e.g., where the local const was used) so
the module uses the centralized validator.
8d2307c to
20defad
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/ldap.ts (1)
42-44:userIdguard is redundant withauthRequired: true.
authRequired: trueon both endpoints means the framework rejects unauthenticated requests beforeaction()is invoked, sothis.userIdis always truthy here. The guard is unreachable dead code.♻️ Proposed fix — remove the redundant guard (applies to both endpoints)
async function action() { - if (!this.userId) { - throw new Error('error-invalid-user'); - } - if (settings.get<boolean>('LDAP_Enable') !== true) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/ldap.ts` around lines 42 - 44, Remove the redundant this.userId guard inside the action() handlers of the LDAP endpoints since both endpoints are configured with authRequired: true and the framework will reject unauthenticated requests before action() runs; locate the two occurrences of "if (!this.userId) { throw new Error('error-invalid-user'); }" inside the action methods in apps/meteor/app/api/server/v1/ldap.ts and delete those guard blocks so the handlers rely on the existing authRequired check.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/refactor-ldap-api-chained-pattern.mdapps/meteor/app/api/server/v1/ldap.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/refactor-ldap-api-chained-pattern.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/api/server/v1/ldap.ts
🧠 Learnings (4)
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/api/server/v1/ldap.ts
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/app/api/server/v1/ldap.ts
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
apps/meteor/app/api/server/v1/ldap.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/app/api/server/v1/ldap.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/ldap.ts (2)
packages/rest-typings/src/v1/ldap.ts (1)
isLdapTestSearch(18-18)packages/core-services/src/index.ts (1)
LDAP(166-166)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/api/server/v1/ldap.ts`:
- Around line 21-28: Remove the locally defined validator by deleting the
ajv.compile block that creates isLdapTestSearch and instead import the existing
validator from `@rocket.chat/rest-typings` (the exported isLdapTestSearch from
src/v1/ldap.ts); update the file's import statements to include isLdapTestSearch
and remove the local ajv.compile usage so all validation calls use the imported
isLdapTestSearch symbol.
---
Duplicate comments:
In `@apps/meteor/app/api/server/v1/ldap.ts`:
- Line 80: The call to settings.get at the LDAP enable check uses a raw type;
update the expression settings.get('LDAP_Enable') to use the boolean generic
(settings.get<boolean>('LDAP_Enable')) to match the usage on line 46 and ensure
proper typing and consistency for the LDAP_Enable condition in the LDAP-related
logic.
- Line 70: The ajv.compile usage for messageResponseSchema is using a generic of
{ message: string } but the schema includes a success field; update the generic
on ajv.compile for messageResponseSchema (the same spot as the earlier compile
at line 36) to reflect the actual shape (e.g., { success: boolean; message:
string }) so the resulting validator type matches the schema and TypeScript
type-checks correctly.
- Line 84: The call to LDAP.testSearch(this.bodyParams.username) is not wrapped
in a try/catch so errors escape without logging; wrap the invocation in the same
try/catch pattern used by ldap.testConnection: call LDAP.testSearch inside a try
block, catch any thrown error, log the error (include the error object/message
and context such as the username) using the same logger used by
ldap.testConnection, and then rethrow or return the handled error consistent
with that function's behavior.
- Line 36: The AJV compile call uses ajv.compile<{ message: string
}>(messageResponseSchema) but the schema requires both message and success:
true; update the generic to match the schema shape (e.g., ajv.compile<{ message:
string; success: true }>(messageResponseSchema)) so the compiled validator's
TypeScript type aligns with messageResponseSchema and avoids the type mismatch;
locate the ajv.compile invocation in the LDAP handler (ajv.compile and
messageResponseSchema) and adjust the generic accordingly.
---
Nitpick comments:
In `@apps/meteor/app/api/server/v1/ldap.ts`:
- Around line 42-44: Remove the redundant this.userId guard inside the action()
handlers of the LDAP endpoints since both endpoints are configured with
authRequired: true and the framework will reject unauthenticated requests before
action() runs; locate the two occurrences of "if (!this.userId) { throw new
Error('error-invalid-user'); }" inside the action methods in
apps/meteor/app/api/server/v1/ldap.ts and delete those guard blocks so the
handlers rely on the existing authRequired check.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38883 +/- ##
===========================================
+ Coverage 70.55% 70.57% +0.01%
===========================================
Files 3189 3189
Lines 112703 112703
Branches 20429 20403 -26
===========================================
+ Hits 79519 79539 +20
+ Misses 31123 31105 -18
+ Partials 2061 2059 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Migrates ldap.testConnection and ldap.testSearch POST endpoints from the legacy addRoute() pattern to the new chained .post() API pattern with typed AJV response schemas. Replaces Meteor check() with isLdapTestSearch AJV validator for request body validation on ldap.testSearch. Part of RocketChat#38876
smirk-dev
force-pushed
the
refactor/migrate-ldap-api-chained-pattern
branch
from
20defad to
7fcf5fe
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/ldap.ts (1)
41-44:if (!this.userId)guard is redundant —authRequired: truealready rejects unauthenticated requests beforeaction()runs.If the sole purpose is TS type narrowing, the check is still safe to keep, but it will never actually throw at runtime. The same applies to the identical guard at Lines 76-78.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/ldap.ts` around lines 41 - 44, The `if (!this.userId)` guards inside the action() handlers are redundant because authRequired: true already prevents unauthenticated calls; remove the throw branches in action() (and the identical guard later) to avoid unreachable runtime checks, or if you need TypeScript narrowing keep a single explicit assertion (e.g., const userId = this.userId!;) and add a short comment explaining it's only for TS narrowing — update the two occurrences referencing action() accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/refactor-ldap-api-chained-pattern.mdapps/meteor/app/api/server/v1/ldap.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/api/server/v1/ldap.ts
🧠 Learnings (5)
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/api/server/v1/ldap.ts
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/app/api/server/v1/ldap.ts
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
apps/meteor/app/api/server/v1/ldap.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/app/api/server/v1/ldap.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/app/api/server/v1/ldap.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/ldap.ts (2)
packages/rest-typings/src/v1/ldap.ts (1)
isLdapTestSearch(18-18)packages/core-services/src/index.ts (1)
LDAP(166-166)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
.changeset/refactor-ldap-api-chained-pattern.md (1)
1-5: LGTM!Changeset entry is accurate —
patchbump is appropriate for a non-breaking internal refactor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/app/api/server/v1/ldap.ts`:
- Line 36: The typed schema compile call incorrectly omits the required success
field; update the ajv.compile generic to reflect the full response shape
(include success: boolean alongside message: string) so the call to
ajv.compile<{ message: string }>(messageResponseSchema) uses the correct type
(e.g., ajv.compile<{ success: boolean; message: string }>) and matches
messageResponseSchema and any callers that expect success.
- Line 70: The JSON schema validator generics for message responses are missing
the required "success: true" property; update the ajv.compile generic used for
messageResponseSchema (the call ajv.compile<{ message: string
}>(messageResponseSchema)) to include success: true (e.g., ajv.compile<{
success: true; message: string }>(messageResponseSchema)) so the compiled
validator types correctly reflect the response shape; apply this change to all
occurrences (including the ajv.compile on Line 36 and the one currently flagged
on Line 70).
- Around line 21-28: The local ajv schema definition isLdapTestSearch duplicates
the exported schema from `@rocket.chat/rest-typings`; replace the local definition
by importing isLdapTestSearch from '@rocket.chat/rest-typings' and use that
imported symbol wherever the local one was referenced (remove the const
isLdapTestSearch = ajv.compile... block), ensuring any type parameters or ajv
instance usage match the imported schema’s expected shape.
- Line 84: Wrap the LDAP.testSearch(this.bodyParams.username) call in a
try/catch like ldap.testConnection does: catch any thrown error, call the same
logging facility (e.g., processLogger or the existing logger used in this file)
to log the error with context (include the username and a descriptive message),
and return/throw a sanitized error response consistent with the
ldap.testConnection error handling path so raw service errors are not
propagated.
- Line 80: The settings.get call for the LDAP feature flag is missing the
boolean generic; update the settings.get invocation for 'LDAP_Enable' to use a
boolean generic (matching the call at Line 46) so the expression checking !==
true is type-safe and consistent with the other usage of settings.get.
---
Nitpick comments:
In `@apps/meteor/app/api/server/v1/ldap.ts`:
- Around line 41-44: The `if (!this.userId)` guards inside the action() handlers
are redundant because authRequired: true already prevents unauthenticated calls;
remove the throw branches in action() (and the identical guard later) to avoid
unreachable runtime checks, or if you need TypeScript narrowing keep a single
explicit assertion (e.g., const userId = this.userId!;) and add a short comment
explaining it's only for TS narrowing — update the two occurrences referencing
action() accordingly.
Contributor
|
I already have a PR open for this LDAP endpoint. Please close yours and check for duplications to avoid wasting time. You should also trace my existing PR here: PR #150 before opening any new ones. so we don't have collisions between our PRs |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrates the
ldap.testConnectionandldap.testSearchPOST endpoints from the legacyAPI.v1.addRoute()pattern to the new chained.post()API pattern.Changes
addRoutecalls withAPI.v1.post()chained patterncheck()with AJV validation:ldap.testSearchnow uses the existingisLdapTestSearchAJV validator from@rocket.chat/rest-typingsviavalidateParamsinstead ofcheck(this.bodyParams, Match.ObjectIncluding({...}))messageResponseSchemafor both endpoints' response validationmeteor/checkimport dependencyMotivation
Part of the ongoing effort to migrate REST API endpoints to the new chained API pattern that enables typed response validation and future OpenAPI spec generation. This PR additionally removes a Meteor
check()dependency in favor of AJV validation.Closes part of #38876
Summary by CodeRabbit