fix(auth): correct AD user search special chars, format toggle, and root domain [R8S-1106] (#3063)

2026-08-07 10:04:49 +00:00 · 2026-07-23 08:59:09 +12:00
parent a1ef4ba0f9
commit 5af9070991
11 changed files with 330 additions and 85 deletions
@@ -29,7 +29,17 @@ export default class AdSettingsController {
  parseDomainName(account) {
    this.domainName = '';

-    if (!account || !account.includes('@')) {
+    if (!account) {
+      return;
+    }
+
+    // Service account entered as a distinguished name (e.g. cn=reader,dc=portainer,dc=io)
+    if (!account.includes('@')) {
+      this.domainSuffix = account
+        .split(',')
+        .map((part) => part.trim())
+        .filter((part) => part.toLowerCase().startsWith('dc='))
+        .join(',');
      return;
    }

@@ -0,0 +1,41 @@
+import { describe, it, expect } from 'vitest';
+
+import AdSettingsController from './ad-settings.controller';
+
+function createController() {
+  return new AdSettingsController(null, null);
+}
+
+describe('parseDomainName', () => {
+  it('derives the domain suffix from a UPN-format service account', () => {
+    const ctrl = createController();
+
+    ctrl.parseDomainName('reader@portainer.io');
+
+    expect(ctrl.domainSuffix).toBe('dc=portainer,dc=io');
+  });
+
+  it('derives the domain suffix from a DN-format service account', () => {
+    const ctrl = createController();
+
+    ctrl.parseDomainName('cn=reader,dc=portainer,dc=io');
+
+    expect(ctrl.domainSuffix).toBe('dc=portainer,dc=io');
+  });
+
+  it('trims whitespace around DN components', () => {
+    const ctrl = createController();
+
+    ctrl.parseDomainName('cn=reader, dc=portainer, dc=io');
+
+    expect(ctrl.domainSuffix).toBe('dc=portainer,dc=io');
+  });
+
+  it('clears the domain suffix when a DN has no domain components', () => {
+    const ctrl = createController();
+
+    ctrl.parseDomainName('cn=reader');
+
+    expect(ctrl.domainSuffix).toBe('');
+  });
+});
@@ -28,8 +28,12 @@ export default class LdapUserSearchItemController {
  }

  removeGroup(index) {
-    this.groups.splice(index, 1);
+    // Invoked from the React GroupDnBuilder, i.e. outside Angular's digest, so
+    // wrap the mutation in $evalAsync to trigger a re-render of the ng-repeat.
+    this.$scope.$evalAsync(() => {
+      this.groups = this.groups.toSpliced(index, 1);
      this.onGroupsChange(this.groups);
+    });
  }

  addGroup() {
@@ -21,7 +21,7 @@
      <div class="input-group">
        <div class="input-group-btn">
          <button
-            class="btn btn-primary"
+            class="btn btn-light"
            ng-model="$ctrl.config.UserNameAttribute"
            uib-btn-radio="'sAMAccountName'"
            style="margin-left: 0px"
@@ -30,7 +30,7 @@
            >username</button
          >
          <button
-            class="btn btn-primary"
+            class="btn btn-light"
            ng-model="$ctrl.config.UserNameAttribute"
            uib-btn-radio="'userPrincipalName'"
            limited-feature-dir="{{::$ctrl.limitedFeatureId}}"
@@ -1,6 +1,7 @@
 import { render, screen } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 import { describe, it, expect, vi } from 'vitest';
+import { useState } from 'react';

 import { DnBuilder } from './DnBuilder';

@@ -57,4 +58,24 @@ describe('DnBuilder', () => {
    expect(onChange).not.toHaveBeenCalled();
    expect(screen.getByRole('textbox')).toHaveValue('admins');
  });
+
+  it('keeps the entry row visible after clearing all characters', async () => {
+    const user = userEvent.setup();
+    const suffix = 'dc=example,dc=com';
+
+    function Wrapper() {
+      const [value, setValue] = useState('ou=Users,dc=example,dc=com');
+      return <DnBuilder value={value} suffix={suffix} onChange={setValue} />;
+    }
+
+    render(<Wrapper />);
+
+    const input = screen.getByRole('textbox');
+    await user.clear(input);
+
+    // The row must remain (an empty value is a valid in-progress state), even
+    // though the DN string it round-trips through cannot represent it.
+    expect(screen.getByRole('textbox')).toBeInTheDocument();
+    expect(screen.getByRole('textbox')).toHaveValue('');
+  });
 });
@@ -1,4 +1,4 @@
-import { useCallback, useEffect, useState } from 'react';
+import { useEffect, useRef, useState } from 'react';

 import { FeatureId } from '@/react/portainer/feature-flags/enums';

@@ -20,27 +20,37 @@ export function DnBuilder({
  label,
  limitedFeatureId,
 }: Props) {
-  const [entries, setEntries] = useState<DnEntry[]>([]);
-
-  const handleEntriesChange = useCallback(
-    (newEntries: DnEntry[]) => {
-      setEntries(newEntries);
-      const dn = buildDN(newEntries, suffix);
-      if (dn !== value) {
-        onChange(dn);
-      }
-    },
-    [suffix, value, onChange]
+  const [entries, setEntries] = useState<DnEntry[]>(() =>
+    parseDN(value, suffix)
  );

+  // The DN string can't represent an empty (in-progress) row, so re-parsing our
+  // own emitted value would drop such a row — making the box vanish the moment
+  // you clear its text. Track what we emitted so we re-sync entries only when
+  // `value` changes from an external source (initial load, reset).
+  const emittedRef = useRef(buildDN(entries, suffix));
+
  useEffect(() => {
-    handleEntriesChange(parseDN(value, suffix));
-  }, [value, suffix, handleEntriesChange]);
+    if ((value || '') !== emittedRef.current) {
+      const parsed = parseDN(value, suffix);
+      emittedRef.current = buildDN(parsed, suffix);
+      setEntries(parsed);
+    }
+  }, [value, suffix]);
+
+  // Keep the emitted DN in sync with the entries and suffix.
+  useEffect(() => {
+    const dn = buildDN(entries, suffix);
+    if (dn !== emittedRef.current) {
+      emittedRef.current = dn;
+      onChange(dn);
+    }
+  }, [entries, suffix, onChange]);

  return (
    <DnEntriesField
      value={entries}
-      onChange={handleEntriesChange}
+      onChange={setEntries}
      label={label}
      limitedFeatureId={limitedFeatureId}
    />
@@ -1,4 +1,5 @@
 import { Plus } from 'lucide-react';
+import { useState } from 'react';

 import { isLimitedToBE } from '@/react/portainer/feature-flags/feature-flags.service';
 import { FeatureId } from '@/react/portainer/feature-flags/enums';
@@ -6,10 +7,11 @@ import { FeatureId } from '@/react/portainer/feature-flags/enums';
 import { Select, Input } from '@@/form-components/Input';
 import { Widget, WidgetBody } from '@@/Widget';
 import { Button } from '@@/buttons';
+import { FormError } from '@@/form-components/FormError';
 import { useInputList } from '@@/form-components/InputList/useInputList';
 import { InputListActionButtons } from '@@/form-components/InputList/ActionButtons';

-import { DnEntry } from './ldap-dn-utils';
+import { DnEntry, validateDnEntryValue } from './ldap-dn-utils';

 const typeOptions = [
  { label: 'OU Name', value: 'ou' },
@@ -98,7 +100,30 @@ interface DnEntryItemProps {
 }

 function DnEntryItem({ item, onChange, disabled, readOnly }: DnEntryItemProps) {
+  // Keep the raw text the user typed locally so an invalid value (e.g. one
+  // containing a comma) stays visible alongside the warning instead of being
+  // dropped by the DN parse/build round-trip. Only valid values propagate up.
+  const [value, setValue] = useState(item.value);
+
+  // Re-sync when the entry value changes externally (e.g. reorder, suffix
+  // change) using React's adjust-state-during-render pattern.
+  const [lastItemValue, setLastItemValue] = useState(item.value);
+  if (item.value !== lastItemValue) {
+    setLastItemValue(item.value);
+    setValue(item.value);
+  }
+
+  const error = validateDnEntryValue(value);
+
+  function handleValueChange(newValue: string) {
+    setValue(newValue);
+    if (!validateDnEntryValue(newValue)) {
+      onChange({ ...item, value: newValue });
+    }
+  }
+
  return (
+    <div className="flex w-full flex-col gap-1">
      <div className="flex w-full gap-2">
        <div className="w-1/3">
          <Select
@@ -113,13 +138,15 @@ function DnEntryItem({ item, onChange, disabled, readOnly }: DnEntryItemProps) {
        </div>
        <div className="w-5/12">
          <Input
-          value={item.value}
-          onChange={(e) => onChange({ ...item, value: e.target.value })}
+            value={value}
+            onChange={(e) => handleValueChange(e.target.value)}
            disabled={disabled}
            readOnly={readOnly}
            data-cy="ldap-dn-builder-input"
          />
        </div>
      </div>
+      {error && <FormError>{error}</FormError>}
+    </div>
  );
 }
@@ -1,4 +1,4 @@
-import { useEffect, useState } from 'react';
+import { useEffect, useRef, useState } from 'react';

 import { FeatureId } from '@/react/portainer/feature-flags/enums';
 import { isLimitedToBE } from '@/react/portainer/feature-flags/feature-flags.service';
@@ -32,37 +32,43 @@ export function GroupDnBuilder({
    parseDN(parsePath(value, suffix), suffix)
  );

+  // The DN string can't represent an empty (in-progress) group name or path
+  // row, so re-parsing our own emitted value would drop it — making the field
+  // vanish as you clear its text. Track what we emitted so we re-sync only when
+  // `value` changes from an external source (initial load, reset).
+  const emittedRef = useRef(buildGroupDN(groupName, entries, suffix));
+
+  useEffect(() => {
+    if (value !== emittedRef.current) {
+      const parsedGroupName = parseGroupName(value, suffix);
+      const parsedEntries = parseDN(parsePath(value, suffix), suffix);
+      emittedRef.current = buildGroupDN(parsedGroupName, parsedEntries, suffix);
+      setGroupName(parsedGroupName);
+      setEntries(parsedEntries);
+    }
+  }, [value, suffix]);
+
+  // Keep the emitted DN in sync with the group name, path entries and suffix.
  useEffect(() => {
-    const groupName = parseGroupName(value, suffix);
-    const entries = parseDN(parsePath(value, suffix), suffix);
-    setGroupName(groupName);
-    setEntries(entries);
    const dn = buildGroupDN(groupName, entries, suffix);
-    if (dn !== value) {
+    if (dn !== emittedRef.current) {
+      emittedRef.current = dn;
      onChange(index, dn);
    }
-  }, [index, onChange, suffix, value]);
+  }, [groupName, entries, suffix, index, onChange]);

  return (
    <>
      <GroupNameField
        id={`group-name-input-${index}`}
        value={groupName}
-        onChange={(newGroupName) => {
-          setGroupName(newGroupName);
-          onChange(index, buildGroupDN(newGroupName, entries, suffix));
-        }}
+        onChange={setGroupName}
        disabled={isLimited}
        onRemoveClick={onRemoveClick ? () => onRemoveClick(index) : undefined}
      />
      <DnEntriesField
        value={entries}
-        onChange={(entries: DnEntry[]) => {
-          setEntries(entries);
-          if (groupName) {
-            onChange(index, buildGroupDN(groupName, entries, suffix));
-          }
-        }}
+        onChange={setEntries}
        label="Path to group"
        limitedFeatureId={limitedFeatureId}
      />
@@ -23,23 +23,22 @@ export function GroupNameField({
      <label htmlFor={id} className="col-sm-4 control-label text-left">
        Group Name
      </label>
-      <div className="col-sm-7 pl-0">
+      <div className="col-sm-8">
+        <div className="flex items-center gap-2">
+          <div className="flex-1">
            <Input
              id={id}
              data-cy="group-name-input"
              value={value}
              onChange={(e) => onChange(e.target.value)}
              disabled={disabled}
-          readOnly={disabled}
            />
          </div>
          {onRemoveClick && (
-        <div className="col-sm-1">
            <Button
              type="button"
              color="danger"
              size="medium"
-            className="vertical-center"
              onClick={onRemoveClick}
              disabled={disabled}
              icon={Trash2}
@@ -47,8 +46,9 @@ export function GroupNameField({
              title="Remove Group"
              aria-label="Remove Group"
            />
-        </div>
          )}
        </div>
+      </div>
+    </div>
  );
 }
@@ -1,6 +1,11 @@
 import { describe, it, expect } from 'vitest';

-import { parseDN, buildDN, DnEntry } from './ldap-dn-utils';
+import {
+  parseDN,
+  buildDN,
+  validateDnEntryValue,
+  DnEntry,
+} from './ldap-dn-utils';

 describe('parseDN', () => {
  it('should parse DN with OU entries', () => {
@@ -79,6 +84,25 @@ describe('parseDN', () => {
      { type: 'ou', value: 'dept_2' },
    ]);
  });
+
+  it('should preserve special characters in values', () => {
+    const result = parseDN(
+      'ou=R&D (Eng.),ou=Sales+Mktg,dc=example,dc=com',
+      'dc=example,dc=com'
+    );
+
+    expect(result).toEqual([
+      { type: 'ou', value: 'R&D (Eng.)' },
+      { type: 'ou', value: 'Sales+Mktg' },
+    ]);
+  });
+
+  it('should round-trip values containing special characters', () => {
+    const dn = 'ou=R&D (Eng.),dc=example,dc=com';
+    const suffix = 'dc=example,dc=com';
+
+    expect(buildDN(parseDN(dn, suffix), suffix)).toBe(dn);
+  });
 });

 describe('buildDN', () => {
@@ -152,3 +176,63 @@ describe('buildDN', () => {
    expect(result).toBe('');
  });
 });
+
+describe('validateDnEntryValue', () => {
+  it('should accept an empty value', () => {
+    expect(validateDnEntryValue('')).toBeUndefined();
+  });
+
+  it('should accept values with allowed characters', () => {
+    expect(validateDnEntryValue('R&D (Eng.)')).toBeUndefined();
+    expect(validateDnEntryValue('Sales_Team 01')).toBeUndefined();
+  });
+
+  it('should accept an equals sign (escaped only in the type, not the value)', () => {
+    expect(validateDnEntryValue('a=b')).toBeUndefined();
+  });
+
+  it('should accept non-ASCII characters', () => {
+    expect(validateDnEntryValue('Café Müller')).toBeUndefined();
+  });
+
+  it('should accept an interior number sign', () => {
+    expect(validateDnEntryValue('Team#1')).toBeUndefined();
+  });
+
+  it('should reject each reserved character', () => {
+    expect(validateDnEntryValue('a"b')).toContain('"');
+    expect(validateDnEntryValue('a+b')).toContain('+');
+    expect(validateDnEntryValue('a,b')).toContain(',');
+    expect(validateDnEntryValue('a;b')).toContain(';');
+    expect(validateDnEntryValue('a<b')).toContain('<');
+    expect(validateDnEntryValue('a>b')).toContain('>');
+    expect(validateDnEntryValue('a\\b')).toContain('\\');
+  });
+
+  it('should list every reserved character it finds', () => {
+    expect(validateDnEntryValue('a,b+c')).toBe(
+      'These characters are not allowed in a DN entry value: + ,'
+    );
+  });
+
+  it('should reject a leading space or number sign', () => {
+    expect(validateDnEntryValue(' Users')).toBe(
+      'A DN entry value cannot start with a space or "#".'
+    );
+    expect(validateDnEntryValue('#Users')).toBe(
+      'A DN entry value cannot start with a space or "#".'
+    );
+  });
+
+  it('should reject a trailing space', () => {
+    expect(validateDnEntryValue('Users ')).toBe(
+      'A DN entry value cannot end with a space.'
+    );
+  });
+
+  it('should reject control characters', () => {
+    expect(validateDnEntryValue('a\tb')).toBe(
+      'A DN entry value cannot contain control characters.'
+    );
+  });
+});
@@ -3,11 +3,53 @@ export interface DnEntry {
  value: string;
 }

+// Characters that RFC 4514 requires to be escaped anywhere inside a
+// distinguished-name attribute value, mirroring go-ldap's EscapeDN (the
+// library the backend authenticates with). Portainer's DN builder concatenates
+// values into the DN string without escaping, so an unescaped occurrence would
+// corrupt the DN — we reject them up front with a warning instead.
+// Note: '=' is intentionally absent (it only needs escaping in the type, not
+// the value) and non-ASCII characters are allowed (valid UTF-8 in a DN).
+const RESERVED_DN_VALUE_CHARS = ['"', '+', ',', ';', '<', '>', '\\'];
+
+const CONTROL_CHAR_MAX = 0x1f;
+const DELETE_CHAR = 0x7f;
+
+export function validateDnEntryValue(value: string): string | undefined {
+  const reserved = RESERVED_DN_VALUE_CHARS.filter((char) =>
+    value.includes(char)
+  );
+  if (reserved.length > 0) {
+    return `These characters are not allowed in a DN entry value: ${reserved.join(
+      ' '
+    )}`;
+  }
+
+  // A leading '#' is read as a hex-encoded value, and leading/trailing spaces
+  // are stripped, so both positions must be escaped (RFC 4514 §2.4).
+  if (value.startsWith(' ') || value.startsWith('#')) {
+    return 'A DN entry value cannot start with a space or "#".';
+  }
+  if (value.endsWith(' ')) {
+    return 'A DN entry value cannot end with a space.';
+  }
+
+  const hasControlChar = [...value].some((char) => {
+    const code = char.charCodeAt(0);
+    return code <= CONTROL_CHAR_MAX || code === DELETE_CHAR;
+  });
+  if (hasControlChar) {
+    return 'A DN entry value cannot contain control characters.';
+  }
+
+  return undefined;
+}
+
 export function parseDN(
  dn: string | undefined,
  domainSuffix: string
 ): DnEntry[] {
-  const regex = /(\w+)=([a-zA-Z0-9_ -]*),?/;
+  const regex = /(\w+)=([^,]*),?/;
  const ouValues: DnEntry[] = [];
  let left = dn || '';
  let match = left.match(regex);