feat: [OCISDEV-1031] implement PasswordModify and ModifyWithResult on LDAP clients

[LukasHirt]

ConnWithReconnect and ConnPool (pkg/utils/ldap) stubbed out PasswordModify and ModifyWithResult with a "not implemented" error, copied over from the existing set of unimplemented ldap.Client methods. This implements both on each client via their existing retry helpers (retry on ConnWithReconnect, do on ConnPool), mirroring the retry-on-network-error behaviour already used by Search/Add/Modify/etc.

This is a prerequisite for unifying oCIS's graph service onto this package's LDAP client: graph's own hand-rolled reconnect implementation fully implements both methods (and actually calls PasswordModify behind GRAPH_LDAP_SERVER_USE_PASSWORD_MODIFY_EXOP), so swapping it onto ConnWithReconnect/ConnPool as-is would silently break that path.

Follow-up to #658 (OCISDEV-1031).

[kw-security]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[Copilot]

Pull request overview

Implements PasswordModify and ModifyWithResult for the two LDAP client implementations in pkg/utils/ldap (ConnWithReconnect and ConnPool) by wiring them through the existing retry helpers, preventing callers (e.g., graph service) from hitting the previous “not implemented” behavior.

Changes:

• Add retry-enabled implementations of ModifyWithResult and PasswordModify to ConnWithReconnect.
• Add pooled implementations of ModifyWithResult and PasswordModify to ConnPool via do.
• Extend ConnPool tests to cover both methods and add an unreleased changelog entry.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
pkg/utils/ldap/reconnect.go	Implements ModifyWithResult/PasswordModify using the existing reconnect retry helper.
pkg/utils/ldap/pool.go	Implements ModifyWithResult/PasswordModify using the existing pool do (checkout + retry-on-network-error).
pkg/utils/ldap/pool_test.go	Adds forwarding tests for both methods via fakeConn overrides.
changelog/unreleased/enhancement-ldap-pool-password-modify.md	Documents the enhancement and links the PR.

Comments suppressed due to low confidence (1)

pkg/utils/ldap/pool_test.go:54

• fakeConn.ModifyWithResult unconditionally calls modifyWithResultFunc; if fakeConn is created without that override, calling ModifyWithResult will panic. Return a descriptive error when the override is unset to prevent nil-function panics and make test failures clearer.

func (f *fakeConn) ModifyWithResult(req *ldap.ModifyRequest) (*ldap.ModifyResult, error) {
	return f.modifyWithResultFunc(req)
}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[2403905]

Points worth noting (non-blocking)

• Retry safety of PasswordModify (minor, pre-existing pattern). On an ErrorNetwork after the server processed the change but before the response arrived, do/retry will transparently
  retry. A password-modify replaying old→new credentials would then fail (old password no longer matches) and surface a confusing error instead of the real success. This is inherent to
  the uniform retry-all-ops design already applied to Modify/Add/ModifyDN, so it's consistent — but PasswordModify is less naturally idempotent than a Modify. Not something to fix in
  this PR; just a known edge worth being aware of when graph switches over.
• Test coverage gap (minor). Both new tests cover only the happy path. The whole point of these methods is retry-on-network-error, and that branch is not exercised for the new methods
  (e.g., first call returns ldap.NewError(ldap.ErrorNetwork, …), second succeeds; assert one reconnect/re-dial). The pool's generic retry is likely tested elsewhere via Search, so this
  is optional — but a single network-retry test on one of the two new methods would lock in the behavior the changelog advertises.
• fakeConn nil-func panic (cosmetic). PasswordModify/ModifyWithResult on fakeConn call the func fields unconditionally, so a future test that triggers one without setting the
  corresponding func will panic rather than fail cleanly. Fine for current tests; a guarded fallback would be more robust if this double gets reused.

[LukasHirt]

Thanks for the review, replying to each point:

• Retry safety of PasswordModify: agreed this is real, but it's an existing tradeoff rather than something new — the uniform retry-all-ops behavior was already shipped in feat: [OCISDEV-1031] add opt-in LDAP connection pool #658 for Search/Add/Modify/ModifyDN, and proper retry/backoff design is explicitly deferred to a later story in OCISDEV-1031 ("retry/backoff (Story 2)" is out of scope here). Leaving as-is per your suggestion, but noted for whenever graph switches over onto this client.
• Test coverage gap: fixed in 9f0a48b — added a network-error-retry test for each of PasswordModify/ModifyWithResult on ConnPool, following the same pattern as TestConnPoolDoRetriesOnceOnNetworkError.
• fakeConn nil-func panic: fixed in the same commit — both methods now return a descriptive error instead of panicking when their override isn't set.

[LukasHirt]

@2403905 could you please take one more look. THX