Skip to content

Add dynamic user search to LDAP synchronization - #5362

Merged
nilsteampassnet merged 4 commits into
nilsteampassnet:developfrom
guerricv:add_search_field_ldap_sync_page
Sep 11, 2026
Merged

nilsteampassnet merged 4 commits into
nilsteampassnet:developfrom
guerricv:add_search_field_ldap_sync_page

Conversation

@guerricv

@guerricv guerricv commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Description

Adds a dynamic client-side search to the LDAP synchronization user list.

  • Places a compact search field before the Refresh and Add role actions in a responsive toolbar.
  • Filters the already-loaded rows by login, display name, first name, last name, and email address.
  • Provides one explicit, accessible clear button that behaves consistently across browsers and is disabled while the field is empty.
  • Displays a translated empty-result state when no LDAP user matches.
  • Preserves and reapplies the active filter after each AJAX refresh.
  • Escapes directory-provided search values with htmlEncode() before inserting them into the DOM.
  • Adds a focused PHPUnit regression test for the controls, toolbar ordering, accessibility, and JavaScript wiring.

No LDAP query, database schema, translation, or AJAX endpoint is changed.

Related issue

None.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (existing behaviour changes)
  • Documentation
  • Translation
  • Refactor / maintenance

How has this been tested?

  • GitHub Actions passes on PHP 8.2 and PHP 8.3.
  • PHPStan level 4 passes.
  • CodeQL reports no new alerts in code changed by this pull request.
  • JavaScript/TypeScript analysis, table-prefix validation, and the production-autoloader check pass.
  • Scrutinizer reports no new issues and passing tests.
  • git diff --check passes locally.
  • Targeted static checks confirm the compact toolbar ordering, accessible controls, escaped search index, clear action, empty-result state, and filter reapplication after AJAX rendering.
  • Added tests/Unit/LdapSynchronizationSearchTest.php for regression coverage.
  • Manual LDAP and user-role testing was not available in this environment.

Checklist

  • PHPStan level 4 passes (php app/vendor/bin/phpstan analyse --memory-limit=2G)
  • The test suite passes (php _tools/phpunit.phar — see CONTRIBUTING.md for the one-time setup)
  • Every new public function has a docblock
  • Variable names and comments are in English
  • No var_dump() or console.log() was added by this change
  • New app/sources/*.queries.php files have a matching public/sources/ proxy shim (not applicable: no handler was added)
  • Changes to teampassclasses were applied to both copies (app/includes/libraries/ and app/vendor/) (not applicable: no class was changed)
  • app/vendor/composer/ is in its production form (git checkout -- app/vendor/composer/)

Impact on install / upgrade

  • No schema change
  • Schema change — an install/upgrade_run_X.X.X.php script is included and the fresh install path was tested

Screenshots

Not available in this environment because there is no local PHP/LDAP runtime.

@nilsteampassnet

Copy link
Copy Markdown
Owner

@guerricv, thanks for this, it's a genuinely useful addition to a page that gets unwieldy on large directories, and the implementation is careful.
I checked it out locally: PHPStan level 4 is clean on both changed pages and the full suite passes (1716 tests, 59406 assertions), including your four new ones.

A few things I'd like adjusted before merging:

1. Please revert the button label change.
$lang->get('list_users')$lang->get('refresh') isn't part of the search feature, and that button is also the initial load action, "Refresh" reads oddly before anything has ever been listed. Happy to discuss it separately if you feel strongly.

2. Please add the roles to the search index.
searchText currently covers login/displayname/givenname/sn/mail, but not entry.ldap_user_groups. "Find every user in a given AD group" is probably the most common reason someone reaches for a filter on this page, and the data is already in the payload:

const searchText = [
    userLogin,
    entry.displayname !== undefined ? entry.displayname[0] : '',
    entry.givenname !== undefined ? entry.givenname[0] : '',
    entry.sn !== undefined ? entry.sn[0] : '',
    entry.mail !== undefined ? entry.mail[0] : '',
    (entry.ldap_user_groups || []).join(' ')
].join(' ')
  1. LdapSynchronizationSearchTest::testToolbarOrdersSearchBeforeRefreshAndRoleActions() uses $toolbarStart before asserting it.
    substr($view, $toolbarStart, $tableStart - $toolbarStart) runs ahead of assertIsInt(). With declare(strict_types=1), a missing marker raises a TypeError instead of failing the assertion cleanly. Please move both assertIsInt() calls above the substr()/arithmetic.

  2. Drop the inline style from the test assertion.
    assertStringContainsString('style="width: 260px; max-width: 100%;" id="ldap-users-search-wrapper"', ...) pins a regression test to a cosmetic detail — any width tweak breaks it. Asserting on id="ldap-users-search-wrapper" alone is enough. Ideally the width itself moves to public/assets/css/teampass.css rather than staying inline.

Minor, take them or leave them:

  • Icon convention: users.php uses fa-solid fa-… (36 occurrences vs 6 fas). fas fa-search / fas fa-times / fas fa-info would be more consistent as fa-solid fa-magnifying-glass / fa-xmark / fa-info (fa-times is the FA5 alias).
  • #ldap-users-search-no-results sits inside #ldap-users-table, which carries p-0, so the message is flush against the card edge and misaligned with the table. Moving it just after that container (or adding px-3) fixes the alignment.
  • $(document).on('input keyup', …) filters twice per keystroke; input alone already covers typing, pasting and clearing on every supported browser.

What I specifically checked and found correct, so you don't have to re-verify it: htmlEncode() escapes both quote characters, so data-search can't be broken out of by a directory-supplied value, and jQuery.attr() hands back the decoded string so matching still works on the raw text. The wrapper visibility is handled at all four #ldap-users-table toggle sites, the filter is correctly reapplied after the AJAX re-render, .hidden is the TeamPass class (not the one Bootstrap 4 dropped), and all four language keys exist in both english.php and french.php. That's the part that usually goes wrong on this page, and it's right here.

@guerricv

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review. I addressed the feedback in commit 65469f055:

  • restored the list_users label;
  • added ldap_user_groups to the client-side search index;
  • reordered the marker assertions so offsets are only used after assertIsInt() (and applied the same hardening to the AJAX re-render test);
  • moved the search width to public/assets/css/teampass.css and removed the cosmetic width assertion;
  • updated the icons to the current Font Awesome convention;
  • padded the empty-result message;
  • reduced the search listener to the input event only.

The existing htmlEncode() protection, wrapper visibility handling, and post-AJAX filter reapplication remain unchanged. PHP is not available in my local environment, so the pushed commit is ready for the GitHub CI validation.

@nilsteampassnet

Copy link
Copy Markdown
Owner

Reviewed 65469f055 — all seven points are addressed, and you also hardened testFilterIsReappliedAfterAjaxRendering(), which I hadn't asked for. Nice.

Verified locally against develop:

  • full suite: OK (1716 tests, 59407 assertions)
  • PHPStan level 4 on both changed pages: no errors
  • merges cleanly onto develop
  • fa-magnifying-glass and fa-xmark both resolve in the bundled Font Awesome 7.2.0
  • public/assets/css/teampass.css has a single copy, so no dual-location issue

Merging. Thanks for the clean iteration and for keeping the escaping and the post-AJAX filter reapplication untouched, that was the delicate part.

@nilsteampassnet
nilsteampassnet merged commit cb29835 into nilsteampassnet:develop Sep 11, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants