diff --git a/agent/ShopDbEndpointAgent.Core/UserContextProbe.cs b/agent/ShopDbEndpointAgent.Core/UserContextProbe.cs index c25d178c..01cbe4a8 100644 --- a/agent/ShopDbEndpointAgent.Core/UserContextProbe.cs +++ b/agent/ShopDbEndpointAgent.Core/UserContextProbe.cs @@ -2,8 +2,9 @@ using Microsoft.Extensions.Logging; namespace ShopDbEndpointAgent.Core; -// Runs a short PowerShell probe as the interactive user and returns what it -// wrote to the spool. +// Collects the session-scoped data that does not exist in the agent's own +// context, by running a short PowerShell probe IN THE CALLING PROCESS'S +// SESSION and writing the result to the user's spool file. // // Collects only data that genuinely does not exist in the agent's own context: // netuse - mapped drives are per logon session; "net use" from @@ -15,15 +16,18 @@ namespace ShopDbEndpointAgent.Core; // it. Sampled at probe time only, so treat it as a coarse // "was anyone at the machine" signal, not live presence. // +// This runs from `shopdbagent.exe --collect-usercontext`, which the tray - and +// only the tray - invokes, because the tray is already running as the signed-in +// operator. The service does NOT call this: it previously tried to descend into +// the session with CreateProcessAsUser, which is the direction that has to +// duplicate a token, is what endpoint protection watches for, and produced +// nothing on a live endpoint. Nothing here crosses a privilege boundary now; +// the probe simply runs as whoever the tray is. +// // The script is embedded rather than shipped as a .ps1 so the probe cannot drift // from the agent binary or be tampered with on disk between cycles. public sealed class UserContextProbe { - // The probe is bounded well below the telemetry cadence. If it cannot finish - // in this time the cycle simply reports no session data; it never delays the - // heartbeat. - private static readonly TimeSpan ProbeTimeout = TimeSpan.FromSeconds(45); - private const string ProbeScript = """ $ErrorActionPreference = 'Stop' $result = [ordered]@{} @@ -51,6 +55,30 @@ public sealed class UserContextProbe } $result['netuse'] = $netuse + # Second, independent view of the same drives. Win32_NetworkConnection + # is the "net use" table and Get-SmbMapping is the SMB client's own, + # and they disagree: a share can be Unavailable in one and absent or + # OK in the other. Neither is authoritative, so both are reported as + # collected and the server decides what to show. Normalising them + # here would discard exactly the disagreement that is diagnostic. + $smbmapping = @() + try { + Get-SmbMapping -ErrorAction Stop | + ForEach-Object { + $smbmapping += [ordered]@{ + localpath = $_.LocalPath + remotepath = $_.RemotePath + status = [string]$_.Status + } + } + } catch { + # Absent on older builds and in some session types. A missing + # source is not a drive fault, so it is recorded and left out + # rather than reported as a failed mapping. + $result['smbmappingerror'] = $_.Exception.Message + } + $result['smbmapping'] = $smbmapping + $listeners = @() try { $ownprocesses = @{} @@ -113,74 +141,72 @@ public sealed class UserContextProbe exit 0 """; - private readonly UserContextSpool spool; + // How long the probe is allowed to take. Generous: this no longer runs on + // the telemetry path, so an unresponsive WMI call delays nothing but itself. + private static readonly TimeSpan ProbeTimeout = TimeSpan.FromSeconds(60); + private readonly ILogger logger; - public UserContextProbe(UserContextSpool spool, ILogger logger) + public UserContextProbe(ILogger logger) { - this.spool = spool; this.logger = logger; } - // Returns the probe payload, or null when there is no interactive session or - // the probe produced nothing usable. Never throws: session-scoped data is a - // nice-to-have and must not be able to fail a telemetry cycle. - public Dictionary? TryCollect() + // Run the probe in THIS process's session and write the spool file. + // Returns true when the probe wrote a result. + // + // Called from the --collect-usercontext entry point, which the tray runs as + // the signed-in operator. Never throws: this is background collection and a + // failure must not take the tray or the agent process down with it. + public bool Collect() { string? scriptPath = null; try { - var resultPath = spool.TryGetResultPath(out var reason); - if (resultPath is null) - { - logger.LogDebug( - "Skipping user-context collection: {Reason}", reason); - return null; - } - - spool.EnsureSpoolDirectory(resultPath); - spool.ClearStaleResult(resultPath); - scriptPath = Path.Combine( Path.GetTempPath(), $"shopdb-usercontext-{Guid.NewGuid():N}.ps1"); File.WriteAllText(scriptPath, ProbeScript); - var arguments = - "-NoProfile -NonInteractive -ExecutionPolicy Bypass -WindowStyle Hidden -File " - + "\"" + scriptPath + "\""; - - var outcome = InteractiveProcessLauncher.LaunchInActiveSession( - "powershell.exe", arguments, Path.GetTempPath(), waitForExit: true, - waitTimeout: ProbeTimeout); - - if (!outcome.Launched) + var startInfo = new System.Diagnostics.ProcessStartInfo { - logger.LogDebug( - "The user-context probe did not launch: {Error}", outcome.Error); - return null; + FileName = "powershell.exe", + Arguments = + "-NoProfile -NonInteractive -ExecutionPolicy Bypass -WindowStyle Hidden -File " + + "\"" + scriptPath + "\"", + WorkingDirectory = Path.GetTempPath(), + UseShellExecute = false, + CreateNoWindow = true, + }; + + using var process = System.Diagnostics.Process.Start(startInfo); + if (process is null) + { + logger.LogWarning("The user-context probe could not be started."); + return false; } - if (outcome.TimedOut) + if (!process.WaitForExit((int)ProbeTimeout.TotalMilliseconds)) { logger.LogWarning( "The user-context probe did not finish within {Seconds} seconds.", ProbeTimeout.TotalSeconds); - return null; + TryKill(process); + return false; } - if (outcome.ExitCode is int exit && exit != 0) + if (process.ExitCode != 0) { logger.LogWarning( - "The user-context probe exited with code {ExitCode}.", exit); - return null; + "The user-context probe exited with code {ExitCode}.", process.ExitCode); + return false; } - return spool.TryConsumeResult(resultPath); + return true; } catch (Exception exception) { logger.LogWarning(exception, "The user-context probe failed."); - return null; + return false; } finally { @@ -193,10 +219,22 @@ public sealed class UserContextProbe } catch { - // A leftover temp script is harmless; it is overwritten by - // name on the next cycle and cleaned by the OS. + // A leftover temp script is harmless; it is uniquely named + // and cleaned by the OS. } } } } + + private void TryKill(System.Diagnostics.Process process) + { + try + { + process.Kill(entireProcessTree: true); + } + catch (Exception exception) + { + logger.LogDebug(exception, "Could not stop the timed-out user-context probe."); + } + } } diff --git a/docs/notes/endpoint-telemetry-and-ui-phases.md b/docs/notes/endpoint-telemetry-and-ui-phases.md new file mode 100644 index 00000000..754a9d9e --- /dev/null +++ b/docs/notes/endpoint-telemetry-and-ui-phases.md @@ -0,0 +1,482 @@ +# Endpoint telemetry and PC Details UI - phased working notes + +Scratch/reference notes for a multi-phase effort. Not a contract document. When +a phase lands, its details belong in AGENT-CONTRACT.md, the csproj changelog, or +the panel declarations - not here. Delete this file when all phases are done. + +Branch: wilm. Current versions at time of writing: agent 1.8.0, tray 1.1.0. + +--- + +## Ground truth established before scoping + +Verified in code, not assumed. These constrain everything below. + +**Panel groups are a hardcoded frontend list.** +`frontend/src/components/PluginAssetPanels.vue:423`: + + const RENDERED_GROUPS = ['details', 'monitors', 'software', 'activity'] + +Any panel whose `group` is not in that list silently falls back to `details`. +Nothing is logged. The comment directly above it records that this has already +caused two disappearing-card incidents (`actions` -> `monitors`, and the +`metrics` -> `monitors` rename). Tab display labels are separately hardcoded in +`plugins/computers/frontend/views/PCDetail.vue:441-449`. + +Consequence: introducing a new tab is a THREE-file change that must land in one +commit - panel `group` in `plugins/endpoints/plugin.py`, `RENDERED_GROUPS`, and +the PCDetail label map. Split it and cards vanish with no error. + +**There is no pinned/locked card concept today.** +Layout is a CSS grid ordered purely by the integer `position` on each panel. +There is no "top row", no span control except one special case: the Network +interfaces table sets full-width span because at half width it wrapped every +cell to three lines (`PluginAssetPanels.vue:693`). + +Consequence: "lock this card top-left/top-right" does not exist yet and is +itself a feature, not a config value. See Phase 5. + +**Two of the requested cards are not plugin panels at all.** +Both the Entra/Intune card (`PCDetail.vue:202-243`) and the Network/IP card +(`PCDetail.vue:249-265`) are hand-written Vue inside PCDetail's details grid, +not declarations in `plugin.py`. Moving them is a component refactor, not a +`position`/`group` edit. + +**There is no LAPS retrieval feature in this codebase.** +Searched: no LAPS button, endpoint, or credential path exists. The only hit is +a historical incident note in `plugins/tools/frontend/views/SecureSendCreate.vue:254` +about LAPS passwords once being sent over a chat link. The request assumes a +click-to-retrieve LAPS button already exists on the Entra card; it does not. +This needs its own scoping conversation - it is a credential-disclosure path +with real audit/permission implications and must not be smuggled in as part of +a layout change. See Open Questions. + +**Existing panel positions in `monitors`** (so new cards do not collide): +10 status, 12 tracked services, 13 tracked processes, 14 hardware, 15 osinfo, +16 interfaces, 17 storage, 18 operator reports, 19 listeners, 40 user context. +`activity` uses 20. `software` uses 30. + +--- + +## Phase 1 - Mapped drive reachability (agent + tray) + +STATUS: code complete, NOT fully validated. See "Where we left off" at the end +of this file before resuming. + +Highest value, and the reason the user-context work exists. Extends the probe +already added in agent 1.8.0 / tray 1.1.0. + +**No active reachability testing.** An earlier draft proposed touching each +share to see if it responded, with a per-mapping timeout. Rejected: Windows +already reports the status, so probing the share is wasted work and reintroduces +the hang risk the bounded probe exists to avoid. Read the status, do not test it. + +**The two status sources disagree, and that disagreement is the signal.** +Captured live on the same box at the same moment, same two mappings: + + Get-SmbMapping net use + OK Q: ...specdata Disconnected Q: ...specdata + OK S: ...shared Reconnecting S: ...shared + +`Get-SmbMapping` reported OK for both while `net use` reported Disconnected and +Reconnecting. So neither source alone is trustworthy: collecting only +Get-SmbMapping (the nicer, more structured one) would have reported a healthy +Q: on a box whose Q: was actually down - exactly the failure this card exists +to catch. + +Collect BOTH and keep them as separate fields per mapping. Do not silently +reconcile them in the agent; the server/UI decides presentation, and preserving +the disagreement is diagnostically useful in its own right. The card should +surface the WORSE of the two rather than the prettier one. + +Status vocabulary must include `Reconnecting` - it appears in the real capture +above and an earlier draft's `Ok`/`Disconnected`/`Unknown` set would have +forced it into the wrong bucket. Keep `Unknown` for probe failure so a failed +check never renders as green Ok. + +**Elevation matters and confirms the tray approach.** The capture was taken +from an elevated shell (`start powershell -verb runas`). Because of UAC token +separation, an elevated context sees a DIFFERENT mapped-drive table than the +normal user - often empty. The tray runs unelevated as the operator, which is +the correct context to enumerate from. Do not "improve" this later by elevating +the probe; that would silently empty the card. + +Mechanism: the probe already writes JSON to the spool file, so this rides the +existing handoff - run both commands in-session, write the combined result, +service reads it. No new transport. + +Server: persist as part of the existing `usercontext` payload key (no new +column, no migration needed - it rides `usercontextjson`). + +Note in code, commented out, per request: a future one-click remediation +command will re-establish a Disconnected mapping. Do not implement now, but +leave the command-catalog seam obvious. It will be an allowlisted action in +`plugins/endpoints/commands.py` and must run in the USER session (a SYSTEM +remap creates a mapping the operator cannot see, and per the elevation note +above an elevated remap has the same problem). + +## Phase 2 - Cheap system-side telemetry (agent service) + +All service-side, no session needed. Group these because they share the +inventory-snapshot plumbing and one migration. + +- **Time sync offset and source** - collect; NOT surfaced in the UI. Wire for + alerting only. Kerberos fails past skew, so this is a high-value silent + predictor. +- **Disk errors / SMART predictive failure** - collect and surface on the + existing Storage card (Phase 4). +- **NIC link speed and duplex** - collect, pair into the existing Network + interfaces collection, NOT surfaced yet. +- **Boot type** - fast startup vs true cold boot. Explains why uptime lies and + why a "reboot" did not clear a fault. Collect now; UI placement undecided, + candidate for the Operating system or Hardware card. +- **Windows Update history, last 5** - surfaced in Monitors (Phase 4) for + pinpointing update-induced breakage. Updates are corporate-managed; this is + diagnostic only, not control. +- **Defender/AV health** - collect and persist, do NOT surface. Known to + produce false positives in this environment; stored for future wiring. + +Explicitly dropped after discussion, do not collect: +certificates (corporate-managed), BitLocker (network join already requires it), +display topology (single monitor on the floor, trained users), session +type/RDP (RDP-in not permitted on the shop floor), logon server (local users). + +**Pending file-rename operations** - CONFIRMED, collect. A stronger +reboot-required signal than the current `pendingreboot` key, and cheap. + +Schema: these add columns to `endpointstates`. One migration, `0014`, with +idempotent guards and a real downgrade, shipped in the SAME change as the model +edit (a model referencing an unapplied column 500s every query on the table). + +## Phase 3 - Service/Process Monitors tab split + +Tab churn is exactly what has silently eaten cards twice before, so this phase +is deliberately isolated from any collection work. + +- Rename the existing `Monitors` tab to `System Monitors`. +- Add a new `Service/Process Monitors` tab. +- Move `Tracked Services` (position 12) and `Tracked Processes` (13) into it. + +Must land atomically across all three places: +1. `group` on those panels in `plugins/endpoints/plugin.py` +2. `RENDERED_GROUPS` in `PluginAssetPanels.vue` +3. the tab label map in `PCDetail.vue` + +Also update the hardcoded panel-count assertion in +`tests/test_plugins/test_endpoints_plugin.py` +(`test_endpoints_asset_panels_are_computer_scoped`) - it has already needed +bumping once (7 -> 12) and will fail again here. + +Printers: a location-based printer auto-detect collection is in progress +elsewhere. Reserve space and a position band for a printer card in the monitors +family so this work does not have to be redone when it lands. Do not build the +card now. + +## Phase 4 - Card content additions + +Depends on Phase 2 data existing. + +- **Mapped drives card** - `Get-SmbMapping`-shaped. `Disconnected` in red, + `Ok` in green. Reuse existing badge/severity styling rather than inventing + new CSS. Room reserved for the future one-click fix action. +- **Storage card** - add disk error / SMART predictive failure to the existing + card rather than a new one. +- **Windows Update card** - last 5 updates, in the System Monitors tab. +- Boot type placement to be decided when the data is visible. + +## Phase 5 - Header right-hand block (DEFERRED - revisit before building) + +Status: on hold at user request. The original ask assumed pinned cards were a +config value; they are not, and building a pinning system was not what the user +wanted to sign up for. Not scheduled. Nothing below is committed. + +The cascade worry that motivated pinning is explicitly parked - do not build +anything for it unless reflow problems actually surface. + +**Revised direction: use the empty right-hand half of the existing header.** +The PC Details header is a full-width card whose right side is currently dead +space - the hostname, badges, PC type, OS and location all sit left, and the +Status / Remote access / Refresh / Restart row runs along the bottom. The block +to the right of the identity lines is empty at every width. + +This is a better target than either pinned grid cells or extending the header +downward: +- It consumes space that already exists, so it costs ZERO additional vertical + height and cannot push the tab strip toward the fold. That was the hard + constraint, and this approach satisfies it structurally rather than by + budgeting. +- It sidesteps the Network interfaces width problem entirely (that table is + full-width by necessity and would fight a half-width pinned slot). +- Header content lives outside the panel grid, so it cannot be reflowed by + cards appearing or disappearing below it. + +Candidates for the block, unresolved: +- Entra/Intune deep links (currently a Details-tab card). Strongest candidate: + they are navigation, not data, and the user has called out their outbound + usability value. +- IP / network summary (currently the hand-written Network card). + +Density rules if this proceeds: inline label/value pairs, not nested cards. Do +not let the right block grow taller than the left identity block, or it starts +adding height after all and the zero-cost property is lost. + +Still true and still unbuilt if this proceeds: +- `Collectors` is not a current tab or group. If IP History moves to a new tab, + that is the same three-file atomic change as Phase 3. +- Entra empty state, requested: show `Please provide DeviceID` / + `Please provide ObjectID` when the backend has no value, rather than an + empty or broken card. + +LAPS is NOT a feature request - clarified by the user as an illustration of why +these outbound portal links matter to usability. No LAPS work is scoped. + +## Phase 6 - Intune/Entra link bug (independent, can be done anytime) + +Reported: the INTUNE and ENTRA buttons sometimes do not appear until the page +is refreshed. Diagnosed, root cause found, NOT yet fixed. + +**Cause: a cold-cache race, not a render problem.** + +`frontend/src/utils/siteSettings.js:9` caches only the RESOLVED settings object: + + let settingsCache = null + async function loadSettings() { + if (settingsCache) return settingsCache + ... + +The in-flight promise is not cached, so every caller during the first load +fires its own `settingsApi.list()`. + +In `plugins/computers/frontend/views/PCDetail.vue:513`, `getEntraDeviceUrls()` +is deliberately fire-and-forget (`.then(...)`, not awaited) so a settings +failure cannot block the page. Meanwhile `computersApi.get()` on line 519 IS +awaited. On a cold cache the settings response can therefore land AFTER the +computer data has rendered. Until it does, `entraUrlTemplate` / +`intuneUrlTemplate` are still `''`, so `fillTemplate()` returns null +(line 426: `if (!template || !value) return null`), and the `v-if` on the +anchors hides both links. A later refresh hits the warm cache and they appear. + +That also explains the intermittency: it depends on which response wins, so it +varies by network timing and by whether another view already warmed the cache. + +**Fix: cache the in-flight promise, not just the result.** Assign the promise +to `settingsCache` immediately so concurrent callers await the same request. +This is a one-line-shaped change in `siteSettings.js` and fixes every consumer +of site settings, not just this card - the same race exists for any view that +reads settings during mount. + +Watch for: the existing `catch` sets `settingsCache = {}` on failure, which +permanently caches the empty result for the page session. Confirm whether a +failed settings load should stay cached (currently it does) or be retryable. +Do not change that behavior silently while fixing the race. + +Verify after fixing that the links appear on a hard reload with a cold cache +and throttled network, which is the condition that reproduces it. + +DONE. Fixed in `siteSettings.js`: the in-flight promise is now cached alongside +the result, and a failed load is cached for 30s rather than the page session. +Covered by `frontend/src/utils/siteSettings.spec.js` (6 tests). Open question 5 +resolved in favour of the short TTL. + +Measured side benefit: the old code assigned `settingsCache` only AFTER the +response resolved, so each sequential `await` re-entered `loadSettings()` before +the cache existed. `applyBranding()` awaits five settings in a row at bootstrap, +`getServicenowUrls()` four, `getEntraDeviceUrls()` two - all of which were +separate GETs on a cold load. Now one. + +## Phase 7 - PCForm "Failed to load data" (NOT the same bug) + +Separate from Phase 6 and NOT caused by it - `PCForm.vue` does not import +`siteSettings.js`. Surfaced on the F3KKSGH4 edit page. + +Partially addressed, root cause still UNKNOWN. + +Evidence: the form renders with reference data present (printer list populated, +dropdowns built) but record fields empty. That means the eight-way +`Promise.all` of reference endpoints SUCCEEDED and the failure is in the +edit-only block, `PCForm.vue` lines 589-635. The only unprotected call there is +`computersApi.get(route.params.id)`; `ensureModelLoaded()` already swallows its +own errors, so it is not the source. + +Two candidates remain, with different fixes and different layers: +- the `computersApi.get` request fails (backend/route problem), or +- a TypeError while mapping the response into `form.value` (frontend shape + mismatch, plausibly from recent schema work). + +Both land in the same catch and both printed the same generic string, which is +why this could not be diagnosed from the UI. + +Done so far: the catch now uses `apiError(err, ...)` so the real server message +surfaces instead of a generic one, and the stale comment claiming a call +"handles its own failure" was removed - no such call remains in that list. + +NEXT: reload the edit page and read the now-specific error. An axios status +code points at the backend; a TypeError names the property and points at the +mapping block. Do not guess between them. + +Also worth doing regardless: a single failed reference list currently blanks +the whole form via `Promise.all` rejection. Degrading one dropdown instead +would be the behaviour the removed comment falsely claimed. + +--- + +## Open questions - answer before the relevant phase + +1. ~~LAPS~~ - RESOLVED: not a feature request. The user was illustrating why the + outbound Intune/Entra links matter to usability. No LAPS work scoped. The + security concerns noted earlier stand if it is ever revisited. +2. ~~Pending file-rename operations~~ - RESOLVED: collect. Folded into Phase 2. +3. ~~Phase 5 pinning / cascade symptom~~ - RESOLVED: not worrying about the + cascade unless it surfaces. Pinned cards dropped in favour of the header's + empty right-hand block; Phase 5 deferred. +4. **`Collectors` tab** - only relevant if Phase 5 proceeds. New tab for one + moved card, or does IP History belong in an existing tab? +5. **Failed settings load caching** - Phase 6. Should a failed + `settingsApi.list()` stay cached for the page session (current behavior) or + be retryable? Decide while fixing the race, do not change it silently. + +## Sequencing + +1 -> 2 -> 3 -> 4. Phase 6 is independent and can be slotted in anytime; it is +small, self-contained, and fixes a user-visible bug, so it is a good candidate +to do early. Phase 5 is deferred and unscheduled. + +Phases 1 and 2 are additive and independently deployable. Phase 3 is a +contained tab move. Phase 4 needs 2. + +Agent/tray version bumps: Phase 1 touches the probe (tray behavior) and Phase 2 +touches the service collector. Both need csproj changelog entries. Note the +existing coupling hazard - `Program.cs` has no unknown-arg guard, so an older +agent receiving a newer tray's verb starts a full second host rather than +no-oping. Fix that guard before any deployment where tray and agent could skew. + +## Phase 8 - fetchAllPages returns a bare array (LAST, and it affects the above) + +Deliberately last: it is a cross-cutting API-surface change, and every earlier +phase should land against the current shape rather than a moving one. + +`frontend/src/api/index.js:131` - `fetchAllPages()` returns the ROWS: + + return items + +Every other method on every API object returns the axios RESPONSE. So callers +must remember, per call site, which of two shapes they are holding: + + const rows = await vendorsApi.listAll() // array + const response = await vendorsApi.list() // { data: { data: [...] } } + +Reading `.data.data` off the array yields `Cannot read properties of undefined +(reading 'data')`. Inside a `Promise.all` that rejects the whole batch, so one +wrong unwrap blanks an entire form - and if it throws before the record fetch, +NO request is made at all, which makes it look like a backend or schema fault. +That is exactly how the PC edit page presented, and why it was misdiagnosed +twice before the log showed `/api/computers/729` was never requested. + +Known history - this class of bug has now hit at least four times: +- `MachineForm.vue` `pcsRes` (fixed previously, comment left in place) +- `PCForm.vue` `vendorRes` (fixed now) +- `MachineForm.vue` `vendorRes` and `buRes` (fixed now - the warning comment was + already sitting three lines away and the adjacent defects were still missed) +- `MeasuringToolForm.vue` `buResponse` (fixed now) + +`ConfigurationPoliciesList.vue:187` defends with +`Array.isArray(value) ? value : ...`, which is evidence the shape is not +trustworthy at the call site. + +Why this matters to the phases above: Phase 1/2 UI work adds call sites that +consume paged reference data, and Phase 5 (if revived) moves cards between +components. Any new `listAll()` caller is another chance to hit this. Landing +the phases first and this last means one migration pass, not a moving target. + +Options, in order of preference: +1. Make `fetchAllPages` return `{ data: { data: items } }` so every API method + has ONE shape. Mechanical but touches every existing `listAll()` caller in + the same commit, or they break in the opposite direction. Highest value, + highest blast radius - needs the full grep and a single atomic change. +2. Rename to something the shape is obvious from (`fetchAllRows`, + `listAllRows`). Cheap, no behavior change, keeps the trap but makes it + visible at the call site. Good interim step if 1 is too large right now. +3. Leave it and rely on review. Rejected - four occurrences say review does not + catch it, and one of them happened directly beneath a comment warning about + it. + +Whichever is chosen, do the full-codebase grep for `listAll(` first; there are +callers in core views, print views, and at least seven plugin frontends. + +--- + +# Where we left off + +Written mid-session when the test tooling stopped working. Read this first. + +## Phase 1 is code complete but NOT fully validated + +What landed, all uncommitted in the working tree: + +**`agent/ShopDbEndpointAgent.Core/UserContextProbe.cs`** - added `Get-SmbMapping` +alongside the existing `Win32_NetworkConnection` capture, emitted as a separate +`smbmapping` key with its own `smbmappingerror`. The two sources are NOT +reconciled in the probe, deliberately - see the Phase 1 notes above. Still +unelevated, still in the operator session. + +**`plugins/endpoints/api/routes.py`** - added `_drive_letter`, +`_DRIVE_STATUS_RANK`, `_worse_drive_status`, `_mapped_drives`, and pointed +`panel_usercontext()` at `_mapped_drives()`. Merges the two views by drive +letter and shows the WORSE status. Carries a long WARNING comment naming the +three load-bearing behaviours; that comment is doing real work, leave it. + +**`plugins/endpoints/commands.py`** - commented-out `remap_drive` seam with the +user-session/elevation reasoning. Comment only, no behavior. + +## Validation state - the important part + +Last KNOWN-GOOD full run: `8 failed, 2489 passed, 11 skipped`. All 8 failures +are pre-existing and unrelated: +- 3x `test_endpoints_plugin.py` timestamp tests - these fail ONLY on Windows. + `_in_zone()` calls `time.tzset()`, which does not exist on Windows (Unix-only + in CPython), so it raises AttributeError before the assertions ever run. + `_parse_dt` itself is correct. These pass on CI. Consider a + `skipif(not hasattr(time, 'tzset'))` so the suite is honest on Windows. +- 5x docs / security-headers / plugin-guard, all pre-existing. + +TWO CHANGES LANDED AFTER THAT RUN AND ARE UNVERIFIED: +1. `_mapped_drives()` now renders `(Unknown)` for a drive neither source gave a + status for. Previously it rendered blank, which reads as healthy - the exact + silent-green failure the warning comment warns about. Small but real, + behavioral, and untested. +2. The `commands.py` seam. Comment only, zero risk. + +`get_errors` on both changed files is clean, but that only proves they parse. + +## Tooling failure that stopped validation + +Every `run_command_in_terminal` call began failing with "A task was canceled", +including a two-file pytest run that should finish in under a second, and a +background launch that never wrote its `Tee-Object` output file. Two full-suite +runs had succeeded earlier in the same session, so this is not slowness - the +execution path went down partway through. + +Test Explorer discovers all 2508 tests but returns zero results for every +filter tried (`TypeName`, `MethodName`, `FullyQualifiedName`). Likely needs a VS +restart. + +## Next actions, in order + +1. Restart VS, then run `python -m pytest tests/test_plugins/test_endpoints_plugin.py -q`. + Expect 3 failed (the tzset ones) / 23 passed. Anything else is ours. +2. Consider adding tests for `_mapped_drives()` - currently zero coverage on + logic that decides whether a broken drive renders as broken. The cases that + matter: sources disagree (worse wins), drive in only one source (kept), no + status at all (Unknown, not blank), `Get-SmbMapping` absent entirely + (degrades to netuse alone). +3. Only then start Phase 2. Do not begin new collection work on an unverified + base - the untested `(Unknown)` delta would get buried under it. + +## Deferred, agreed explicitly + +Agent and tray republish happens at the END of all phases, not per-phase. The +server change is safe to deploy ahead of the agent: until the new probe ships, +`smbmapping` is simply absent from the payload and `_mapped_drives()` falls +through to `netuse` alone, rendering exactly as it does today. No ordering +constraint between them. + diff --git a/plugins/endpoints/api/routes.py b/plugins/endpoints/api/routes.py index 11171aeb..11b6f6c3 100644 --- a/plugins/endpoints/api/routes.py +++ b/plugins/endpoints/api/routes.py @@ -672,6 +672,125 @@ def panel_listeners(asset_id: int): return success_response(rows) +def _drive_letter(value): + """Normalise a drive to its bare letter so the two sources can be paired. + + net use reports 'Z:' and Get-SmbMapping reports 'Z:' or occasionally a + trailing separator, so compare on the letter alone. + """ + text = (value or '').strip().rstrip('\\/') + return text.upper() or None + + +# Worst-first. The panel shows the worse of the two sources for a drive, so a +# share that one source calls OK and the other calls Unavailable is reported as +# Unavailable: a drive that is broken in either view is broken for the operator. +_DRIVE_STATUS_RANK = { + 'unavailable': 0, + 'disconnected': 0, + 'paused': 1, + 'reconnecting': 2, + 'ok': 3, + 'connected': 3, +} + + +def _worse_drive_status(left, right): + """Return whichever status is worse, preferring a known one over None. + + An unrecognised status ranks as Reconnecting, deliberately below OK: a + status we cannot interpret must not be assumed healthy. + """ + candidates = [s for s in (left, right) if s] + if not candidates: + return None + unknown_rank = _DRIVE_STATUS_RANK['reconnecting'] + return min( + candidates, + key=lambda s: _DRIVE_STATUS_RANK.get(str(s).strip().lower(), unknown_rank) + ) + + +def _mapped_drives(context): + """Merge the two mapped-drive views into one row per drive letter. + + Windows exposes mapped drives through Win32_NetworkConnection (the 'net use' + table) and through Get-SmbMapping, and they do not always agree - a share can + be Unavailable in one and OK or absent in the other. The probe collects both + without reconciling them, so the disagreement survives to here. + + A drive present in only one source is still a drive: it is reported with the + status that source gave it, rather than dropped for lack of a second opinion. + + WARNING - this function is not covered by tests. Three behaviours here are + load-bearing and easy to break while "simplifying": + + 1. Worse-status-wins. Never report the healthier of the two sources. A + drive that either source calls Unavailable IS unavailable to the + operator, and rendering it OK sends someone to a share that is down. + 2. Single-source drives are kept, not dropped. Get-SmbMapping is absent on + some builds and session types; requiring agreement would silently hide + every mapped drive on those machines. + 3. Unknown statuses rank as Reconnecting, not as OK. An unrecognised + string must never be optimistically treated as healthy. + + Changing any of these makes a broken drive look fine, which is a silent + failure - the panel stays green and nobody is told. If you touch this, + add the tests first. + """ + drives = {} + + netuse = context.get('netuse') + if isinstance(netuse, list): + for entry in netuse: + if not isinstance(entry, dict): + continue + letter = _drive_letter(entry.get('localname') or entry.get('local')) + if not letter: + continue + drives[letter] = { + 'remote': entry.get('remotename') or entry.get('remote') or '', + 'status': entry.get('status'), + } + + smbmapping = context.get('smbmapping') + if isinstance(smbmapping, list): + for entry in smbmapping: + if not isinstance(entry, dict): + continue + letter = _drive_letter(entry.get('localpath')) + if not letter: + continue + existing = drives.get(letter) + if existing: + existing['status'] = _worse_drive_status( + existing.get('status'), entry.get('status')) + existing['remote'] = existing['remote'] or entry.get('remotepath') or '' + else: + drives[letter] = { + 'remote': entry.get('remotepath') or '', + 'status': entry.get('status'), + } + + rows = [] + for letter in sorted(drives): + drive = drives[letter] + status = (drive.get('status') or '').strip() + remote = (drive.get('remote') or '').strip() + label = '{} {}'.format(letter, remote).strip() + # A drive neither source gave a status for is Unknown, never blank. An + # unannotated row reads as healthy, so a failed check would render as a + # working drive - the one outcome this card must never produce. + if not status: + status = 'Unknown' + # Only the problem states are annotated. Tagging every healthy drive + # with "(OK)" would bury the one that is not. + if status.lower() not in ('ok', 'connected'): + label = '{} ({})'.format(label, status) + rows.append(label) + return rows + + @endpoints_bp.route('/asset//usercontext', methods=['GET']) @require_permission('endpoints.view') def panel_usercontext(asset_id: int): @@ -681,6 +800,9 @@ def panel_usercontext(asset_id: int): session 0, so they are collected by a probe run in the logged-in user's session. Nothing is reported when no one is signed in, which is a valid state and not a failure. + + Mapped drives arrive from two sources that can disagree; the worse of the + two statuses is shown so a half-broken share is never rendered as healthy. """ state = EndpointState.query.filter_by(assetid=asset_id).first() if not state: @@ -690,21 +812,14 @@ def panel_usercontext(asset_id: int): if not isinstance(context, dict) or not context: return success_response({'fields': []}) - netuse = context.get('netuse') - drives = [] - if isinstance(netuse, list): - for entry in netuse: - if isinstance(entry, dict): - local = entry.get('localname') or entry.get('local') or '' - remote = entry.get('remotename') or entry.get('remote') or '' - drives.append('{} {}'.format(local, remote).strip()) + drives = _mapped_drives(context) listeners = context.get('userlisteners') listenercount = len(listeners) if isinstance(listeners, list) else None fields = [ {'label': 'Probed user', 'value': context.get('username')}, - {'label': 'Collected', 'value': context.get('timestamp')}, + {'label': 'Collected', 'value': context.get('collectedat')}, {'label': 'Idle', 'value': _format_uptime(context.get('idleseconds'))}, {'label': 'Mapped drives', 'value': ', '.join(drives) if drives else None, 'mono': True}, diff --git a/plugins/endpoints/commands.py b/plugins/endpoints/commands.py index 2e2746e2..74fd0d9b 100644 --- a/plugins/endpoints/commands.py +++ b/plugins/endpoints/commands.py @@ -102,6 +102,27 @@ COMMAND_ALLOWLIST = { } +# Reserved seam - one-click mapped-drive remediation. NOT implemented; this is a +# placement note so the next person adds it here rather than inventing a new +# transport. When it lands it will re-establish a Disconnected mapping surfaced +# by the user-context card: +# +# 'remap_drive': { +# 'label': 'Reconnect mapped drive', +# 'description': 'Re-establish a disconnected network drive mapping.', +# 'targetkind': 'drive', +# }, +# +# It MUST execute in the interactive USER session. A SYSTEM-context remap creates +# a mapping in SYSTEM's own drive table that the operator cannot see, so the card +# would keep reporting the drive as broken while the "fix" silently succeeded +# somewhere nobody is looking. An ELEVATED remap fails the same way, because UAC +# token separation gives the elevated context a different drive table - the same +# reason the probe itself runs unelevated from the tray. +# +# CommandExecutor.RunScriptInUserSession is the existing path for this. + + # Actions that demand a permission stricter than the base endpoints.command. # The enqueue route requires the caller to ALSO hold the mapped permission, so a # destructive action (a remote reboot) is a heavier grant than a routine toggle.