Entra Directory Sync PR Review - edmozley/freeitsm GitHub Wiki

Entra directory sync - PR review

PR #235 by vsc55 · Design #234 · Verdict: merge and fix

The design was sound and it reused the existing import rules properly, so the code went in under its author's name and the fixes followed in their own commit. The review ran the code against a real Entra directory, which is what turned one suspicion into a proven bug and cleared two others.

What was found

Finding	How it was shown	Fix
A new account with a leaver's sign-in name took over the leaver's record, with no conflict	Real directory: an account was deleted, a new one made with the same name, and the same record was renamed and relinked	A record linked to a different object id is never matched
The conflict setting did not apply to that match	Test install	The refusal applies whatever the setting says
A sign-in could replace an existing identity link, on every OpenID Connect provider. Before the PR the database refused it	Test install: `main` refused the statement, the PR replaced the link	`oidcLinkRequester()` replaces only an import's placeholder
The `oid` lookup at sign-in could overwrite a real subject	Reading	It now requires the placeholder
Two imports of one source could run together	Reading	`dsyncLock()` around a live run
Test and group search could send a non-Entra provider's secret to Microsoft	Reading	Both refuse a provider that is not Entra
`source_ref` had no index	Reading	`idx_user_sso_source_ref`
A chosen-groups run had no ceiling on users read	Reading	`ESYNC_MAX_SCAN`
Preview said "will be added" for a disabled account, and called new people out of scope	Real directory	Both reworded
An unchanged reporting line was reported as set on every run, for every source	Real directory	Unchanged lines are skipped

A correction made during the review

The first version of the takeover finding said two live accounts could share an email. On a real directory, Entra refused to create the second one, because proxyAddresses must be unique. The bug was real but the likely route was wrong: it is reuse over time. Reading the code had produced a true bug and a false story about how it happens.

What the real directory cleared

Microsoft's documentation says advanced queries do not support $expand, and the code sends ConsistencyLevel: eventual on every request, so the manager query looked likely to fail. It worked, as did reading nested group members without $count. People, a disabled account, a manager and a nested group all landed correctly.

What was left

A reused address has no resolution on screen. The conflict is safe, but the new starter stays out until there is one.
There is no way to undo an import, and a first import has no brake. The safety brake only catches a drop in numbers. A compulsory first Preview and a tool to remove imported people are planned for all three sources.
#236 to #240 were reviewed separately. Each depends on this one.

Files

	File	Change
🟦	`includes/entra_sync.php`	`entraImportPerson()`, the `'taken'` match, the lock, the ceiling, wording
🟦	`includes/directory_sync.php`	`dsyncLock()`, `dsyncUnlock()`, unchanged managers skipped
🟧	`includes/oidc.php`, `api/auth/oidc_callback.php`	`oidcLinkRequester()`
🟨	`api/system/test_entra_connection.php`, `api/system/search_entra_groups.php`	Entra only
🟩	`database/freeitsm.sql`, `includes/db_verify_indexes.php`	The index
🟪	`tests/entra-sync-identity.php`	New, 17 checks