Skip to content

HLPS-DRAFT: Split Dorc.Api into Linux-compatible + Windows-only worker (#423)#706

Open
benhegartysefe wants to merge 15 commits into
mainfrom
feat/423-api-split-hlps
Open

HLPS-DRAFT: Split Dorc.Api into Linux-compatible + Windows-only worker (#423)#706
benhegartysefe wants to merge 15 commits into
mainfrom
feat/423-api-split-hlps

Conversation

@benhegartysefe

Copy link
Copy Markdown
Contributor

Summary

This PR is documentation-only — no code changes, no installer changes. It is the first checkpoint of the HLPS → IS → SPEC process for #423.

Why this exists

#424 was closed unmerged (rationale) because it bundled the structural split with:

  1. A bulk class-rename pass that misread CLAUDE.md's naming rule as a banned-words blacklist (producing grab-bag names: Properties, Requests, Operations).
  2. Wide drift after daemons-modernisation (Modernise daemons: migrate to deploy schema with PascalCase, add RBAC + audit, fix UX bugs #649) landed on main — ~340 silently semantic-conflicted files.
  3. Three unresolved architectural decisions baked into code without sign-off.

This HLPS unwinds (1) and (3) and asks the architecture owner to resolve the unknowns before any code is written.

Status

DRAFT — per CLAUDE.md, HLPS progresses DRAFT → IN REVIEW → REVISION → APPROVED. Only the adversarial review panel can approve.

Test plan

  • Architecture owner reviews and answers U-1 (worker topology), U-2 (AD migration direction), U-3 (auth propagation).
  • Adversarial review panel evaluates clarity, ordering, completeness of the HLPS (per CLAUDE.md review scope for plans).
  • On approval, draft docs/api-split/IS-api-split.md ordering atomic steps; second checkpoint there.

🤖 Generated with Claude Code

Draft HLPS scoping the split of Dorc.Api into a Linux-compatible primary
and a Windows-only worker. Deliberately excludes the bulk class-rename
that bundled with PR #424 (the renames misread CLAUDE.md's naming
principle as a banned-words blacklist and produced grab-bag names).

Research carried over from PR #424:
- WINDOWS_DEPENDENCIES_ANALYSIS.md (file-level Windows-API inventory)
- ARCHITECTURE_API_SPLIT.md (PR #424's proposed topology, informational)
- REGISTRY_UPGRADE_EXAMPLE.md
- FILES_TO_MOVE_ANALYSIS.md
- DEPLOYMENT_DEPENDENCY_ANALYSIS.md

Three blocking unknowns flagged for architecture-owner sign-off before
moving to IS: worker process topology, AD migration direction, inter-API
auth propagation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented May 28, 2026

Copy link
Copy Markdown

Test Results

322 tests  +12   322 ✅ +12   15s ⏱️ +6s
  1 suites ± 0     0 💤 ± 0 
  1 files   ± 0     0 ❌ ± 0 

Results for commit 10d8ecb. ± Comparison against base commit aab79d1.

♻️ This comment has been updated with latest results.

Architecture-owner decisions on 2026-05-28:

- D-1 (was U-1): HTTP loopback worker. Separate ASP.NET Core process
  bound to 127.0.0.1. In-proc plugin defeats the Linux-deployment goal;
  queue sidecar is the wrong shape for synchronous request-path work.

- D-2 (was U-2): Replace System.DirectoryServices with Microsoft Graph
  in the primary. AD code is deleted, not moved to the worker. Worker
  shrinks to genuinely non-portable surface (WMI, registry, password
  reset). Entra tenant becomes a hard customer prerequisite.

- D-3 (was U-3): Loopback-only + shared-secret header. Worker rejects
  calls missing X-Worker-Key. Authz happens entirely in the primary
  using Graph claims before the worker is called; worker trusts. For
  password reset, worker uses its own service account; caller identity
  is forwarded in the body for audit only. JWT/mTLS add machinery
  without material security benefit on loopback.

Scope and Success Criteria updated to reflect: Graph migration in
scope, AD removal in scope, customer Entra prerequisite documented.
Next Steps section now sketches concrete IS S-001..S-009 shape.

Two new non-blocking unknowns added (U-9 Graph permission set, U-10
on-prem-only customer migration path) for the IS step that does the
Graph swap.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@benhegartysefe

Copy link
Copy Markdown
Contributor Author

Blocking unknowns resolved with architecture owner on 2026-05-28. Status bumped to IN REVIEW.

Was Now Decision
U-1 D-1 HTTP loopback worker — separate ASP.NET Core process bound to 127.0.0.1. In-proc plugin defeats the Linux-deployment goal; queue sidecar is the wrong shape for synchronous authz.
U-2 D-2 Replace System.DirectoryServices with Microsoft Graph in the primary. AD code is deleted, not moved. Worker shrinks to genuinely non-portable surface (WMI, registry, password reset). Entra tenant becomes a hard customer prerequisite.
U-3 D-3 Loopback-only + shared-secret header (X-Worker-Key). Authz happens in the primary using Graph claims before the worker is called; worker trusts. Password reset uses worker's own service account; caller identity forwarded in body for audit only. JWT/mTLS add machinery without security benefit on loopback.

Knock-on changes in the HLPS:

  • Scope: Graph migration and AD removal pulled into scope.
  • SC-1: now means no System.DirectoryServices* or System.Management refs in Dorc.Api.
  • SC-8: LDAP-injection findings disappear when Graph replaces LDAP queries.
  • Two new non-blocking unknowns (U-9 Graph permission set, U-10 on-prem-only customer migration) for the IS step that does the Graph swap.

Next Steps section now sketches a concrete IS shape: S-001..S-009 with explicit milestones (worker scaffolding → contract → first endpoint family proof-of-pattern → WMI → password reset → Graph migration → drop Windows-only deps → installer → docs).

Ready for adversarial review.

…-wide Graph migration

Adversarial panel raised 8 HIGH findings, 8 MEDIUM, 5 LOW. Full
disposition in Appendix A of the HLPS.

Substantive changes:

- Title broadened to "Replace AD with Microsoft Graph + Split". The
  Graph migration is the load-bearing piece; the split rides on it.
- D-2 expanded codebase-wide. AD code (in Dorc.Core, Dorc.PersistentData,
  Dorc.Api) is deleted, not moved to the worker. AzureEntraSearcher's
  incorrect [SupportedOSPlatform("windows")] attribute is removed.
- Parity matrix added to §4 enumerating the load-bearing AD behaviours
  the Graph implementation must match (user/group search, identity
  resolution via objectId, transitive group membership, disabled-account
  detection, display name/email). Out-of-parity items called out
  (foreign security principals, well-known SIDs, local-machine SIDs).
- Windows authentication scheme removal (Negotiate, WinAuth*) added to
  scope (Scope E + S-007). Previously missed.
- Next Steps reordered: S-001 is now the Graph migration spike (the
  riskiest work). Worker scaffold drops to S-002. Reduces risk of
  stranded plumbing if Graph parity proves harder than D-2 assumes.
- D-3 augmented with threat model, secret protection class, and
  rotation policy.
- SC-1 expanded to cover Dorc.Core + Dorc.PersistentData (not just
  Dorc.Api) with a CI gate for Linux build.
- SC-4 reworded as documented behavioural envelope (not a parity claim).
- SC-8 split into SC-8a (LDAP injection — eliminated by S-001) and
  SC-8b (log injection — addressed by carved-out SPECs).
- SC-9 added: parity matrix is a living artefact with test coverage.
- C-1 reworded: "no client-side compile-time change" + documented
  behavioural envelope on Linux installs.
- C-7 added: installer-side secret-handling requires security review.
- U-10 owner = Product, with explicit default position recorded.
- Frontmatter codebase_anchor field added (aab79d1) so file paths
  and class names are pinned.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@benhegartysefe

Copy link
Copy Markdown
Contributor Author

Round 1 revision pushed (commit 41c933c). Status: REVISION → IN REVIEW (Round 2).

Adversarial panel raised 8 HIGH, 8 MED, 5 LOW findings. Full disposition in Appendix A of the HLPS.

Substantive changes from Round 1:

  • Title broadened to "Replace AD with Microsoft Graph + Split". The Graph migration is now the load-bearing piece; the split rides on it.
  • D-2 expanded codebase-wide. AD code is deleted from Dorc.Core + Dorc.PersistentData + Dorc.Api (not just the API). AzureEntraSearcher's incorrect [SupportedOSPlatform("windows")] attribute is removed.
  • Parity matrix in §4 enumerates the load-bearing AD behaviours Graph must match (user/group search, identity resolution via objectId, transitive group membership, disabled-account detection, display name/email). Out-of-parity items called out (foreign security principals, well-known SIDs, local-machine SIDs).
  • Windows authentication scheme removal added (Scope E + S-007): Negotiate scheme, WinAuthClaimsPrincipalReader, WinAuthLoggingMiddleware. Previously missed.
  • Next Steps reordered: S-001 is now the Graph migration spike (was at position 6). Worker scaffold drops to S-002. Riskiest work first.
  • D-3 augmented with threat model, secret protection class, rotation policy.
  • SC-1 expanded to cover Dorc.Core + Dorc.PersistentData with a CI gate for Linux build.
  • SC-8 split into 8a (LDAP injection — eliminated by S-001) and 8b (log injection — addressed by carved-out SPECs).
  • U-10 owner = Product, default position recorded (hard break, documented prerequisites, no shim).
  • Codebase anchor aab79d14 added to frontmatter — file paths and class names pinned to this commit.

Round 2 panel kicking off now.

…untName resolution)

Round 2 panel verified the Round 1 fixes but found two genuine new
HIGH issues:

NH-1: Parity matrix omitted resolution of legacy AD SIDs (the
AccessControl.Sid column persisted from existing customer installs).
Current AzureEntraSearcher.GetUserDataById only does Users[id]/Groups[id]
which assumes Entra object IDs; existing AccessControl.Sid rows
(populated from pre-migration AD lookups, see EnvironmentsPersistentSource
lines 165/166/203/373/422/728/932 + AccessControlPersistentSource
lines 77/180/196) would silently 404 post-migration.

NH-2: GetGroupSidIfUserIsMemberRecursive treats the userName parameter
as an Entra object ID/UPN and calls graphClient.Users[userName],
so all sAMAccountName-only callers silently return empty.

Both are codebase-level (not SPEC-level) concerns — they determine
whether D-2 ("AD deleted, not moved") is feasible without breaking
every existing install.

Revision:
- Parity matrix in §4 expanded from 7 rows to 9 (P-4 SID lookup,
  P-5 sAMAccountName resolution, P-7 dual-ID claims emission) with
  explicit ❌-gap markers + Graph strategy per gap (onPremisesSecurityIdentifier
  filter, onPremisesSamAccountName filter, transitiveMemberOf with id+SID select).
- Scope A bullets added for the three gap fixes; S-001 step description
  in §9 names the gaps explicitly.
- D-2 customer-implication text extended: Entra Connect (or Cloud Sync)
  is now an explicit prerequisite for any customer with existing
  AccessControl.Sid rows.
- U-10 reframed with Cohort A (Entra Connect present → transparent
  upgrade) vs Cohort B (pure on-prem AD → hard break).
- New SC-10: existing AccessControl.Sid rows resolve correctly with
  Entra Connect present.
- Scope A test requirement tightened (NM-2): integration tests against
  a Graph SDK fake required per parity-matrix row, not just
  IActiveDirectorySearcher-boundary mocks. SC-9 reinforced.
- New non-blocking U-11: worker-absence detection mechanism on Linux
  (recommended: explicit WindowsWorker:Enabled config flag).
- NM-1 audit-trail fix: don't claim [SupportedOSPlatform] was "inherited";
  state it's wrong, fix it.
- NL-1: S-001 description explicitly calls out the Linux build CI gate.
- Appendix A extended with Round 2 disposition table.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@benhegartysefe

Copy link
Copy Markdown
Contributor Author

Round 2 revision pushed (commit 0865902). Status: REVISION → IN REVIEW (Round 3).

Round 2 panel verified the Round 1 fixes but raised two genuine new HIGHs:

  • NH-1: Parity matrix omitted resolution of legacy AD SIDs persisted in AccessControl.Sid. Existing customer rows (referenced from EnvironmentsPersistentSource lines 165/166/203/373/422/728/932, AccessControlPersistentSource lines 77/180/196) would silently 404 post-migration because AzureEntraSearcher.GetUserDataById only does Users[id]/Groups[id] lookups.
  • NH-2: GetGroupSidIfUserIsMemberRecursive treats userName as an Entra ID/UPN and calls graphClient.Users[userName] — sAMAccountName callers silently return empty.

Both verified in code. These are codebase-level (not SPEC-level) concerns: they determine whether D-2 ("AD deleted, not moved") is feasible without breaking every existing install.

Revision (Round 3):

  • Parity matrix expanded from 7 → 9 rows. Added P-4 (legacy AD SID lookup via $filter=onPremisesSecurityIdentifier eq '...'), P-5 (sAMAccountName resolution via $filter=onPremisesSamAccountName eq '...'), P-7 (claims expansion emits both Pid and Sid from transitiveMemberOf). Each ❌ gap row says what's missing in current Graph impl + the strategy to close it.
  • Scope A got three new bullets for the gap fixes; S-001 description names them explicitly.
  • D-2 customer implication extended: Entra Connect (or Cloud Sync) becomes an explicit prerequisite for any install with existing AccessControl.Sid rows.
  • U-10 reframed with Cohort A (Entra Connect present → transparent upgrade) vs Cohort B (pure on-prem AD → hard break).
  • New SC-10: existing AccessControl.Sid rows resolve correctly with Entra Connect present (verified by Graph-fake integration test).
  • Scope A test requirement tightened (NM-2): integration test against a Graph SDK fake per parity-matrix row, not just IActiveDirectorySearcher-boundary mocks. SC-9 reinforced.
  • New non-blocking U-11: worker-absence detection mechanism on Linux (recommended: explicit WindowsWorker:Enabled config flag).
  • Minor: NM-1 audit-trail fix; NL-1 CI gate visibility in S-001; Round 2 disposition appended to Appendix A.

Round 3 verification kicking off now. Per CLAUDE.md max 3 rounds — if Round 3 finds new HIGHs that aren't trivial, I escalate.

Round 3 reviewer verified all Round 2 NH/NM findings closed; no new
showstoppers. Residual DOMAIN\ prefix handling for sAMAccountName
resolution deferred to S-001 SPEC (appropriate scope).

Per CLAUDE.md, this is the post-HLPS checkpoint. Next: draft
IS-api-split.md ordering S-001..S-010.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@benhegartysefe

Copy link
Copy Markdown
Contributor Author

HLPS APPROVED ✅ (Round 3 verification passed)

Round 3 reviewer verdict: APPROVE. All Round 2 NH-1 / NH-2 / NM-1..3 findings closed; no new showstoppers. Residual DOMAIN\ prefix handling for GetGroupSidIfUserIsMemberRecursive appropriately deferred to S-001's SPEC (method-level concern).

Full disposition trail in Appendix A of the HLPS (Round 1 + Round 2 tables).

Per CLAUDE.md this is the post-HLPS checkpoint. Moving to IS-api-split.md draft next. PR #706 will get the IS commit + a fresh adversarial-review cycle on it.

benhegartysefe and others added 3 commits May 28, 2026 15:03
Translates approved HLPS into 10 atomic, reviewable steps spanning
multiple PRs:

- S-001  Graph migration spike (the riskiest work, sequenced first)
- S-002  Worker project scaffold + loopback host + shared-secret auth
- S-003  IWindowsWorkerClient contract + null-impl for Linux + worker-
         absence detection (resolves U-6/U-7/U-11)
- S-004  Move RefDataServers registry probing to worker (proof-of-pattern)
- S-005  Move WMI service-status probe path to worker
- S-006  Move ResetAppPassword impersonation to worker + SC-8b carve-out
- S-007  Remove Negotiate scheme + WinAuth* (after S-001)
- S-008  MSI installer wiring + shared-secret provisioning
- S-009  Remaining log-injection SPEC carve-outs (parallel)
- S-010  Documentation + Linux container smoke test (closeout)

Step dependency graph documented. Parallelism explicit (S-001 and S-002
fully independent; S-004/S-005/S-006 parallel after S-003; S-009 parallel
throughout). Risk shape documented (S-001 highest, S-006 second,
S-008 third, rest mechanical).

Cross-cutting acceptance table maps every SC-1..SC-10 to the step(s)
that satisfy it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round 1 reviewer raised 3 HIGH, 3 MED, 2 LOW findings.

HIGH fixes:
- H-1: S-006 was bundling worker-move + log-injection security fix.
  Split: S-006 is worker-move only; the ResetAppPasswordController
  log-injection fix moves to S-009 alongside the other three
  log-injection items. Security-sensitive diffs now reviewed
  independently of mechanical moves.
- H-2: S-005 was declared independent of S-001, but DaemonStatusProbe
  lives in Dorc.Core which S-001 reshapes. Added S-001 dependency to
  S-005 (and clarified the same applies to S-004 and S-006 in the
  parallelism note).
- H-3: SC-5 was asserted in the cross-cutting table without a step's
  verification covering it. Added explicit "Dorc.Api.Client builds
  unchanged" assertion to S-001 and S-007 verification intents.

MED fixes:
- M-1: Named the typed-exception throw-timing decision in S-003 as
  an open SPEC-level question (synchronous-at-call vs startup-probe),
  with recommendation.
- M-2: Promoted Windows-Service-vs-IIS hosting decision to a new
  IS-level Lookahead unknown U-12, with recommendation. Removed
  the "decision in SPEC" language from S-008.
- M-3: Added explicit fixture-capture-on-main sentence to S-004
  (applies to S-005 and S-006 too — they reuse the S-004 fixture
  pattern).

LOW fixes:
- L-1: Added SID-based ACL parity check to S-007 verification.
- L-2: Parallelism note clarified — S-004/5/6 mutually independent
  at code level, but all depend on S-001.

Status: DRAFT -> IN REVIEW (Round 2). Cross-cutting acceptance table
updated to match.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round 2 review confirmed all Round 1 fixes except H-2 was only
partially addressed: the S-005 body still read "Independent of S-001"
contradicting the Step Index and Parallelism note (both correctly
listing S-001 as a dependency). One-line fix to bring the S-005
Dependencies subsection into line with the index.

No other defects. Status: IN REVIEW (Round 2) -> APPROVED.

Per CLAUDE.md this is the post-IS checkpoint. Next: JIT SPEC for
S-001 (Graph migration spike), adversarial review of that SPEC,
then test-first execution.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@benhegartysefe

Copy link
Copy Markdown
Contributor Author

IS APPROVED ✅ (Round 2 verification)

Round 1 reviewer raised 3 HIGH, 3 MED, 2 LOW findings. Round 2 reviewer verified all are addressed except H-2 was partially addressed — the S-005 body still contradicted the Step Index. Trivial one-line fix applied; no other defects.

Status: DRAFT → IN REVIEW (R1) → REVISION → IN REVIEW (R2) → APPROVED.

Per CLAUDE.md we are at the post-IS checkpoint. Both HLPS and IS are approved. The next move is JIT SPEC for S-001 (Graph migration spike) — the riskiest step, sequenced first.

S-001 in plain English: promote Dorc.Core/AzureEntraSearcher to the sole IActiveDirectorySearcher, remove its incorrect [SupportedOSPlatform("windows")] attribute, close parity-matrix gaps P-4 (legacy AD SID lookup via onPremisesSecurityIdentifier), P-5 (sAMAccountName resolution), P-7 (dual-ID claims emission), delete the AD code from Dorc.Core + Dorc.PersistentData + Dorc.Api, drop System.DirectoryServices* package refs, ship the Linux build CI gate.

Estimated diff size for S-001 alone: substantial — likely several hundred LOC across ~15 files, plus a sizable test suite.

benhegartysefe and others added 2 commits May 28, 2026 15:21
Pseudocode-level spec for S-001:
- AzureEntraSearcher attribute removal + 3 parity-gap fixes (P-4, P-5, P-7)
- DI switch in ApiRegistry to direct AzureEntraSearcher registration
- AD-code deletions (7 files; folder cleanup if needed)
- System.DirectoryServices* package-ref drops from Dorc.Core + Dorc.PersistentData
- Linux build CI gate (negative SC-1 enforcement)
- Graph fake harness via NSubstitute against IRequestAdapter
- Per-row parity-matrix test plan (P-1..P-9 + SC-10 acceptance + negatives)
- 6 risks flagged including R-3 ClaimsTransformer CA1416 transient warning

Two open SPEC-level decisions named (test-seam refactor approach,
CI job placement) — to resolve during adversarial review.

Status: DRAFT. Will go to adversarial review before code execution.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round 1 reviewer raised 3 HIGH, 3 MED, 3 LOW findings.

HIGH fixes:
- H-1: §2.6 missed ClaimsPrincipalReaderFactory as a second consumer
  of IUserGroupsReaderFactory (uses both GetWinAuthUserGroupsReader
  AND GetOAuthUserGroupsReader). Added as a touch site with explicit
  resolution; verified in code.
- H-2: §2.2's ordered-lookup language could read as "duplicate the
  existing Users[pid] call". Rewrote as augmentation: the SID-shaped
  filter fallback runs *after* the existing 404s, not as a new
  starting point. Implementer told explicitly not to duplicate.
- H-3: §5 bullet 9 "ClaimsTransformer compiles" was not objectively
  scriptable. Replaced with concrete dotnet build invocation that
  covers both consumer-update edits.

MED fixes:
- M-1: §2.4 disambiguated — GetMemberGroups call is *replaced*
  (not augmented) by TransitiveMemberOf.GraphGroup.
- M-2: §2.3 sentinel value pinned to string.Empty (matches existing
  semantics observed by CachedUserGroupReader.GetGroupSidIfUserIsMember
  line 42's sid != null check). Implementer told explicitly: do not
  return null.
- M-3: §3.1 test-seam refactor justified against HLPS SC-9 + Round-2
  NM-2 rejection of boundary mocks. Now in-scope by direct reference,
  with Option A (Func<GraphServiceClient> factory) recommended and
  Option B (recorded HTTP harness) as documented alternative.

LOW fixes:
- L-1: P-8 test split into two cases that match actual control flow
  (disabled-user-only falls through to group lookup; both miss throws).
- L-2: §4 / §5 grep overlap clarified (one enforces, the other audits).
- L-3: §4 now tells implementer to check TreatWarningsAsErrors before
  applying R-3's #pragma suppression.

Status: DRAFT -> APPROVED. Ready for code execution.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@benhegartysefe

Copy link
Copy Markdown
Contributor Author

SPEC-S-001 APPROVED ✅ (Round 2 verification)

Round 1 raised 3 HIGH, 3 MED, 3 LOW findings — all addressed in commit 28b4a8c. Round 2 verifier confirmed each fix and reported no new showstoppers. Status: DRAFT → APPROVED.

Per CLAUDE.md this is the pre-execution checkpoint for S-001. The spec is at docs/api-split/SPEC-S-001-graph-migration.md.

Stopping here for the user's go-ahead before writing code. The execution will touch ~15 files, drop two package refs, add Graph-fake tests, and produce a substantial PR.

benhegartysefe and others added 2 commits May 28, 2026 15:39
Implements SPEC-S-001 §2 in full:
- AzureEntraSearcher: [SupportedOSPlatform("windows")] attribute removed;
  added internal test-seam ctor (Func<GraphServiceClient>); P-4 (legacy
  AD SID lookup via $filter=onPremisesSecurityIdentifier), P-5
  (sAMAccountName resolution with DOMAIN\ and (External) prefix
  handling), P-7 (TransitiveMemberOf emitting both Pid and on-prem SID)
  gap fixes in place.
- ApiRegistry: switched from composite/factory pattern to direct
  registration of AzureEntraSearcher as the singleton
  IActiveDirectorySearcher; IUserGroupReader registered directly;
  EntraDirectorySearchService registered for IDirectorySearchService.
- EntraDirectorySearchService (new): Graph-backed implementation of
  IDirectorySearchService (preserves DirectorySearchController's
  UserSearchResult/GroupSearchResult REST surface — SPEC §2.6 missed
  this consumer; added per implementation discovery).
- ClaimsTransformer: ctor now injects IUserGroupReader directly
  (was IUserGroupsReaderFactory).
- ClaimsPrincipalReaderFactory: same direct-injection switch; both
  internal readers now wrap the single shared IUserGroupReader.
- Program.cs ConfigureOAuth: dropped IUserGroupsReaderFactory
  registration line (no longer needed).
- PropertyValuesController, RequestController: ctor switched from
  IDirectorySearcherFactory to IActiveDirectorySearcher directly.

Files deleted (SPEC §2.6 + the additional IUserGroupsReaderFactory
discovered during execution):
- Dorc.Core/ActiveDirectorySearcher.cs
- Dorc.Core/CompositeDirectorySearcher.cs
- Dorc.Core/IdentityServer/IdentityServerSearcher.cs
- Dorc.Core/Interfaces/IDirectorySearcherFactory.cs
- Dorc.Api/Services/DirectorySearcherFactory.cs
- Dorc.Api/Services/UserGroupReaderFactory.cs
- Dorc.Api/Services/ActiveDirectorySearchService.cs
- Dorc.Api/Interfaces/IUserGroupsReaderFactory.cs

Package refs dropped (SPEC §2.7):
- Dorc.Core.csproj: System.DirectoryServices, System.DirectoryServices.AccountManagement
- Dorc.PersistentData.csproj: same two

Verification at this commit:
- Dorc.Api, Dorc.Core, Dorc.PersistentData, Dorc.Api.Tests,
  Dorc.Core.Tests, Dorc.Monitor, Dorc.Monitor.Tests, Dorc.Api.Client
  all build clean (zero errors).
- `git grep "System\.DirectoryServices" src/` returns zero hits.

Follow-up commits on this same PR:
- Graph fake test harness + parity-matrix tests P-1..P-9 + SC-10
- Linux build CI gate workflow

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds .github/workflows/linux-build-gate.yml enforcing HLPS SC-1:

- Runs on ubuntu-latest for every push and PR to main/develop
- Guard step greps for System.DirectoryServices* and System.Management*
  in src/Dorc.Api, Dorc.Core, Dorc.PersistentData; fails fast with a
  clear "SC-1 regression" message if any reappear.
- Then restores + builds each of the three projects in Release config.

This is the negative SC-1 gate. The positive gate (Linux container
actually runs DORC) is S-010's smoke test, not this commit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread .github/workflows/linux-build-gate.yml Fixed
Comment thread src/Dorc.Core/AzureEntraSearcher.cs Fixed
…+ SC-10

Implements SPEC-S-001 §3 in full.

Test harness (Dorc.Core.Tests/Graph/):
- MockHttpHandler.cs — HttpMessageHandler that matches by predicate
  and returns canned JSON; unmatched calls fall through to 404 so
  missing mocks surface as failing tests rather than hangs.
- GraphTestClient.cs — Factory that constructs a real GraphServiceClient
  over the mock handler via HttpClientRequestAdapter +
  AnonymousAuthenticationProvider. This means tests exercise the real
  Kiota deserialization path, satisfying HLPS SC-9's requirement that
  parity tests use a Graph SDK fake rather than IActiveDirectorySearcher-
  boundary mocks.

Tests (Dorc.Core.Tests/AzureEntraSearcherTests.cs) — 12 tests, all green:
- P1 Search user by display name
- P2 Search group by display name
- P3 GetUserDataById resolves Entra object id
- P4 GetUserDataById resolves legacy AD SID via onPremisesSecurityIdentifier
- P4 GetUserDataById falls through user→group when user filter misses
- P5 GetGroupSidIfUserIsMemberRecursive resolves sAMAccountName via filter
- P5 GetGroupSidIfUserIsMemberRecursive strips DOMAIN\ prefix
- P5 GetGroupSidIfUserIsMemberRecursive returns empty string (not null)
  on miss — preserves CachedUserGroupReader cache semantics
- P7 GetSidsForUser emits both Pid and Sid (self + groups)
- P8 GetUserDataById falls through to group when user is disabled
- P8 GetUserDataById throws ArgumentException when both miss
- SC10 AccessControlSidRow_ResolvesViaOnPremisesIdentifier — integration
  test simulating an existing customer's persisted AccessControl.Sid row

Production-code fix surfaced by the tests:
- The original AzureEntraSearcher caught Microsoft.Graph.ServiceException
  with NotFound status to recover from 404s; Graph SDK 5.x replaced
  ServiceException with Microsoft.Kiota.Abstractions.ApiException. The
  original catch was dead code — never matched in production. Replaced
  ServiceException → ApiException throughout, and switched .Result to
  .GetAwaiter().GetResult() so Kiota's ApiException isn't wrapped in
  AggregateException before the catch sees it. This is a latent bug
  the original codebase had since the Graph 4→5 upgrade.

Test results at this commit:
- Dorc.Core.Tests: 132 / 132 pass (12 new + 120 existing)
- Dorc.Api.Tests: 190 / 190 pass (no regressions)

Remaining S-001 work:
- SC-8a SAST re-scan against the diff (manual verification step;
  PR-#424's LDAP-injection findings should no longer trigger because
  the entire LDAP code path is gone).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@benhegartysefe

Copy link
Copy Markdown
Contributor Author

S-001 execution complete

Three commits land the Graph migration spike per SPEC-S-001:

Commit Scope
b11e2ee8 Production code: parity fixes (P-4 SID lookup, P-5 sAMAccountName, P-7 dual-ID claims), AD deletions (8 files), DI switch, csproj package-ref drops
ad6e3eea Linux build CI gate workflow (.github/workflows/linux-build-gate.yml)
ddf72a9d Graph fake harness + 12 parity-matrix tests + a SC-10 acceptance test, all green

Test results at HEAD:

  • Dorc.Core.Tests: 132 / 132 pass (12 new + 120 existing)
  • Dorc.Api.Tests: 190 / 190 pass (no regressions)
  • All key projects build clean on Windows: Dorc.Api, Dorc.Core, Dorc.PersistentData, Dorc.Api.Tests, Dorc.Core.Tests, Dorc.Monitor, Dorc.Monitor.Tests, Dorc.Api.Client.
  • git grep "System.DirectoryServices" src/ returns zero hits — SC-1 negative gate satisfied.

Notable findings surfaced by the test work:

  • The original AzureEntraSearcher caught Microsoft.Graph.ServiceException with NotFound to recover from 404s. Graph SDK 5.x replaced ServiceException with Microsoft.Kiota.Abstractions.ApiException — the original catch was dead code. Latent bug since the Graph 4→5 upgrade; now fixed by switching to ApiException + .GetAwaiter().GetResult() (the prior .Result wrapped the exception in AggregateException, preventing the catch from matching).
  • DirectorySearchController consumed IDirectorySearchService, separate from IActiveDirectorySearcher. The SPEC missed this; added EntraDirectorySearchService (Graph-backed) to keep the controller's REST surface intact.

HLPS SC coverage:

  • SC-1 ✅ (negative gate via CI workflow; positive gate via S-010 smoke test later)
  • SC-5 ✅ (Dorc.Api.Client builds unchanged)
  • SC-6 / SC-9 ✅ (parity-matrix integration tests against a real Graph SDK fake)
  • SC-10 ✅ (legacy AccessControl.Sid rows resolve via onPremisesSecurityIdentifier)

Outstanding for S-001 closeout:

PR #706 is now a full deliverable: HLPS + IS + SPEC-S-001 + S-001 implementation. Next IS step (S-002 — worker project scaffold) would be a separate PR.

SC-1 gates against PackageReferences, not against source usages of
the namespace. Initial workflow tripped on:
- A comment in EntraDirectorySearchService.cs that explains the file
  is a replacement for the System.DirectoryServices-backed predecessor
- using System.Management in WmiUtil.cs — but WmiUtil moves to the
  worker in S-005, not S-001, so System.Management source usages are
  expected in the primary at this point in the IS sequence.

Narrowed the grep to PackageReference Include="System.DirectoryServices*"
on csproj files only. System.Management package-ref check belongs in
the workflow gate added by S-005's PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@benhegartysefe

Copy link
Copy Markdown
Contributor Author

Linux build CI gate passes on commit 7dc28b56. SC-1 enforced. (Initial run tripped on a comment + on using System.Management in WmiUtil.cs, which belongs to S-005 not S-001 — gate now correctly scoped to PackageReferences only.)

Comment thread src/Dorc.Core.Tests/Graph/MockHttpHandler.cs Fixed
Comment thread src/Dorc.Core.Tests/Graph/MockHttpHandler.cs Fixed
Comment thread src/Dorc.Core.Tests/Graph/MockHttpHandler.cs Fixed
@benhegartysefe benhegartysefe linked an issue May 28, 2026 that may be closed by this pull request
- CodeQL (workflow permissions): added `permissions: contents: read`
  to linux-build-gate.yml so GITHUB_TOKEN is minimised.
- CodeQL (PII exposure via GetUserEmail call graph): removed the
  user-id template argument from AzureEntraSearcher.GetSidsForUser's
  best-effort warning log. The log still says we couldn't resolve the
  on-prem SID for the requested user; identifying the user across
  log lines now requires correlating other request-scoped context.
- Code-quality (HttpResponseMessage not disposed): MockHttpHandler now
  overrides Dispose(disposing) and disposes the captured
  HttpRequestMessage instances. The returned HttpResponseMessage
  ownership intentionally transfers to the Kiota request adapter.
- Code-quality (foreach-with-break -> Where/FirstOrDefault): replaced
  the rule-matching loop in MockHttpHandler.SendAsync with
  _rules.FirstOrDefault(r => r.Match(request)).

Test results at this commit:
- 12 / 12 AzureEntraSearcherTests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
var rule = _rules.FirstOrDefault(r => r.Match(request));
if (rule != null)
{
var response = new HttpResponseMessage(rule.Status);
Comment on lines +63 to +66
var fallback = new HttpResponseMessage(HttpStatusCode.NotFound)
{
Content = new StringContent($"{{\"error\":{{\"code\":\"itemNotFound\",\"message\":\"No mock for {request.Method} {request.RequestUri}\"}}}}", Encoding.UTF8, "application/json")
};
@benhegartysefe

Copy link
Copy Markdown
Contributor Author

All bot findings on the prior commit addressed in 10d8ecb9. CodeQL Analyze (csharp) rerun against this branch passed clean — SC-8a satisfied (the LDAP-injection class is eliminated with the AD code path; no log-injection findings remain on AzureEntraSearcher). S-001 is functionally complete; remaining S-002..S-010 will ship as their own PRs per the IS multi-PR structure.

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.

Split the Dorc API into 2 different API's

2 participants