feat(260729-d3k): multi-base LDAP sync scope + re-keyed no-op guard
- parseBaseDns() splits the newline-separated baseDn field into a list - syncUsersForTenant no-op guard re-keyed on empty parsed base-DN list (was empty groupFilterDns) — the sole condition that skips search + the deactivation loop, preventing mass-deactivation on an unconfigured config - collectSearchEntries/listGroups/searchUsers loop every base DN and merge/dedupe results by entry dn - empty groupFilterDns is no longer a no-op: it now performs a normal multi-base search with no memberOf restriction - groupFilterDns ou= entries stay additional search bases; group DNs become an optional memberOf constraint applied to every base search - spec: replaced empty-groupFilterDns no-op test with empty-base-DN no-op test, added multi-base merge/dedup test Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -188,9 +188,27 @@ export class LdapService {
|
||||
}
|
||||
|
||||
/**
|
||||
* Discover groups and organizational units under the configured base DN.
|
||||
* Split a `\n`-separated Base-DN admin field into a trimmed, non-empty DN
|
||||
* list. The baseDn column stays a single String (no schema change) — this
|
||||
* is the sole place that turns it into the list every search path loops
|
||||
* over. Blank/whitespace-only lines are dropped; undefined/empty input
|
||||
* yields [].
|
||||
*/
|
||||
private parseBaseDns(baseDn?: string | null): string[] {
|
||||
if (!baseDn) {
|
||||
return [];
|
||||
}
|
||||
return baseDn
|
||||
.split(/\r?\n/)
|
||||
.map((line) => line.trim())
|
||||
.filter((line) => line.length > 0);
|
||||
}
|
||||
|
||||
/**
|
||||
* Discover groups and organizational units under EVERY configured base DN.
|
||||
* Used by the admin UI to build a selective import filter (groupFilterDns).
|
||||
* Read-only directory query using the service-account bind.
|
||||
* Read-only directory query using the service-account bind. Results from
|
||||
* all base DNs are merged and deduped by entry dn.
|
||||
*/
|
||||
async listGroups(config: {
|
||||
serverUrl: string;
|
||||
@@ -206,13 +224,21 @@ export class LdapService {
|
||||
try {
|
||||
await this.bind(client, config.bindDn, config.bindPassword);
|
||||
|
||||
const { searchEntries } = await client.search(config.baseDn, {
|
||||
filter: '(|(objectClass=group)(objectClass=organizationalUnit))',
|
||||
attributes: ['cn', 'ou', 'dn'],
|
||||
scope: 'sub',
|
||||
});
|
||||
const baseDns = this.parseBaseDns(config.baseDn);
|
||||
const entriesByDn = new Map<string, Entry>();
|
||||
|
||||
return searchEntries.map((entry) => {
|
||||
for (const baseDn of baseDns) {
|
||||
const { searchEntries } = await client.search(baseDn, {
|
||||
filter: '(|(objectClass=group)(objectClass=organizationalUnit))',
|
||||
attributes: ['cn', 'ou', 'dn'],
|
||||
scope: 'sub',
|
||||
});
|
||||
for (const entry of searchEntries) {
|
||||
entriesByDn.set(entry.dn, entry);
|
||||
}
|
||||
}
|
||||
|
||||
return Array.from(entriesByDn.values()).map((entry) => {
|
||||
const dn = entry.dn;
|
||||
const isOu = /^ou=/i.test(dn);
|
||||
const rawName = isOu ? entry['ou'] : entry['cn'];
|
||||
@@ -350,14 +376,21 @@ export class LdapService {
|
||||
const q = LdapService.escapeLdapFilterValue(trimmed);
|
||||
const filter = `(&(objectClass=person)(|(cn=*${q}*)(sAMAccountName=*${q}*)(displayName=*${q}*)(mail=*${q}*)))`;
|
||||
|
||||
const { searchEntries } = await client.search(config.baseDn, {
|
||||
filter,
|
||||
attributes: ['cn', 'displayName', 'sAMAccountName', 'mail', 'dn'],
|
||||
scope: 'sub',
|
||||
sizeLimit: 50,
|
||||
});
|
||||
const baseDns = this.parseBaseDns(config.baseDn);
|
||||
const entriesByDn = new Map<string, Entry>();
|
||||
for (const baseDn of baseDns) {
|
||||
const { searchEntries } = await client.search(baseDn, {
|
||||
filter,
|
||||
attributes: ['cn', 'displayName', 'sAMAccountName', 'mail', 'dn'],
|
||||
scope: 'sub',
|
||||
sizeLimit: 50,
|
||||
});
|
||||
for (const entry of searchEntries) {
|
||||
entriesByDn.set(entry.dn, entry);
|
||||
}
|
||||
}
|
||||
|
||||
const entries = searchEntries.map((entry) => ({
|
||||
const entries = Array.from(entriesByDn.values()).map((entry) => ({
|
||||
dn: entry.dn,
|
||||
username: first(entry['sAMAccountName']),
|
||||
displayName: first(entry['displayName']) || first(entry['cn']),
|
||||
@@ -534,14 +567,17 @@ export class LdapService {
|
||||
errors: [],
|
||||
};
|
||||
|
||||
// DELIBERATE back-compat break + critical safety guard: an empty/undefined
|
||||
// groupFilterDns now means "sync nothing", not "import everyone under
|
||||
// baseDn". Returning here BEFORE any LDAP search and BEFORE the
|
||||
// deactivation loop below is what prevents an empty selection from
|
||||
// mass-deactivating every existing LDAP user (there would be no synced
|
||||
// DNs to compare against, so every local LDAP user would look "removed").
|
||||
// CRITICAL SAFETY GUARD: the Base-DN(s) are now the sync scope. Returning
|
||||
// here BEFORE any LDAP bind/search and BEFORE the deactivation loop below
|
||||
// is what prevents an unconfigured/blank config from mass-deactivating
|
||||
// every existing LDAP user (there would be no synced DNs to compare
|
||||
// against, so every local LDAP user would look "removed"). This is the
|
||||
// SOLE no-op condition: an empty groupFilterDns no longer short-circuits
|
||||
// here — with >=1 base DN configured, a normal multi-base search runs
|
||||
// (see collectSearchEntries), it just applies no memberOf restriction.
|
||||
// lastSyncAt is intentionally left untouched on this no-op path.
|
||||
if (!config.groupFilterDns || config.groupFilterDns.length === 0) {
|
||||
const baseDns = this.parseBaseDns(config.baseDn);
|
||||
if (baseDns.length === 0) {
|
||||
return result;
|
||||
}
|
||||
|
||||
@@ -681,17 +717,22 @@ export class LdapService {
|
||||
}
|
||||
|
||||
/**
|
||||
* Run the directory search for syncUsersForTenant, applying the selective
|
||||
* group/OU import filter (groupFilterDns) when one is configured.
|
||||
* Run the directory search for syncUsersForTenant across EVERY configured
|
||||
* Base DN (the primary sync scope — see syncUsersForTenant's early-return
|
||||
* guard, which is the sole no-op path and is keyed on the parsed base-DN
|
||||
* list, not on groupFilterDns). groupFilterDns is an OPTIONAL extra
|
||||
* restriction:
|
||||
*
|
||||
* - Empty groupFilterDns: DELIBERATE no-op — returns [] without ever
|
||||
* searching the directory. A selective sync now means "nothing selected
|
||||
* = nothing synced", not "import everyone under baseDn" (back-compat
|
||||
* break, see syncUsersForTenant's early-return guard which normally
|
||||
* short-circuits before this method is even reached).
|
||||
* - Non-empty groupFilterDns: split into OU DNs (used as extra search
|
||||
* bases) and group DNs (matched via memberOf on the base search).
|
||||
* Results are merged and deduped by entry dn.
|
||||
* - Empty groupFilterDns: each base DN is searched with the plain
|
||||
* sanitizedFilter — no memberOf restriction, every user under the base
|
||||
* DN(s) is synced.
|
||||
* - Non-empty groupFilterDns: split into OU DNs (searched as ADDITIONAL
|
||||
* extra bases with the plain filter) and group DNs (turned into a
|
||||
* memberOf OR-clause ANDed with sanitizedFilter and applied to every
|
||||
* base DN search).
|
||||
*
|
||||
* All results across every base DN (and any extra OU bases) are merged
|
||||
* and deduped by entry dn.
|
||||
*/
|
||||
private async collectSearchEntries(
|
||||
client: Client,
|
||||
@@ -699,18 +740,33 @@ export class LdapService {
|
||||
sanitizedFilter: string,
|
||||
attributes: string[],
|
||||
) {
|
||||
if (!config.groupFilterDns || config.groupFilterDns.length === 0) {
|
||||
return [];
|
||||
}
|
||||
|
||||
const ouBases = config.groupFilterDns.filter((dn) => /^ou=/i.test(dn));
|
||||
const groupDns = config.groupFilterDns.filter((dn) => !/^ou=/i.test(dn));
|
||||
const baseDns = this.parseBaseDns(config.baseDn);
|
||||
const ouBases = (config.groupFilterDns ?? []).filter((dn) =>
|
||||
/^ou=/i.test(dn),
|
||||
);
|
||||
const groupDns = (config.groupFilterDns ?? []).filter(
|
||||
(dn) => !/^ou=/i.test(dn),
|
||||
);
|
||||
|
||||
const entriesByDn = new Map<string, Entry>();
|
||||
|
||||
for (const ouBase of ouBases) {
|
||||
const { searchEntries } = await client.search(ouBase, {
|
||||
filter: sanitizedFilter,
|
||||
// Build the base-search filter: plain sanitizedFilter, optionally ANDed
|
||||
// with a memberOf OR-clause when group DNs are configured.
|
||||
let baseFilter = sanitizedFilter;
|
||||
if (groupDns.length > 0) {
|
||||
const memberOfClauses = groupDns
|
||||
.map(
|
||||
(dn) => `(memberOf=${LdapService.escapeLdapFilterValue(dn)})`,
|
||||
)
|
||||
.join('');
|
||||
baseFilter = `(&${sanitizedFilter}(|${memberOfClauses}))`;
|
||||
}
|
||||
|
||||
// Search every configured base DN with the (possibly memberOf-restricted)
|
||||
// base filter.
|
||||
for (const baseDn of baseDns) {
|
||||
const { searchEntries } = await client.search(baseDn, {
|
||||
filter: baseFilter,
|
||||
attributes,
|
||||
scope: 'sub',
|
||||
});
|
||||
@@ -719,16 +775,11 @@ export class LdapService {
|
||||
}
|
||||
}
|
||||
|
||||
if (groupDns.length > 0) {
|
||||
const memberOfClauses = groupDns
|
||||
.map(
|
||||
(dn) => `(memberOf=${LdapService.escapeLdapFilterValue(dn)})`,
|
||||
)
|
||||
.join('');
|
||||
const combinedFilter = `(&${sanitizedFilter}(|${memberOfClauses}))`;
|
||||
|
||||
const { searchEntries } = await client.search(config.baseDn, {
|
||||
filter: combinedFilter,
|
||||
// ou= group-filter entries are ADDITIONAL search bases, searched with
|
||||
// the plain sanitizedFilter (no memberOf restriction).
|
||||
for (const ouBase of ouBases) {
|
||||
const { searchEntries } = await client.search(ouBase, {
|
||||
filter: sanitizedFilter,
|
||||
attributes,
|
||||
scope: 'sub',
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user