SyncML <Delete> Windows profiles - #42206
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
Adds Windows profile “removal” support by generating/enqueuing SyncML <Delete> commands when profiles are removed from a host or deleted, bringing Windows behavior closer to macOS profile removal.
Changes:
- Extend Windows profile reconciliation to enqueue
<Delete>commands for profiles that should be removed from hosts. - Add SyncML delete-command generation + remove-specific delivery status mapping (treat 404/405 as success for remove).
- Update datastore profile-deletion flows to read stored SyncML before deletion and attempt best-effort removal.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/microsoft_mdm.go | Reconciler now builds “remove” targets and enqueues delete commands. |
| server/fleet/microsoft_mdm.go | Implements delete command generation and remove-specific status mapping. |
| server/fleet/microsoft_mdm_test.go | Adds unit tests for delete command generation and remove-status handling. |
| server/datastore/mysql/microsoft_mdm.go | Two-phase deletion logic + operation-type aware response handling + mark-for-remove updates. |
| changes/33418-windows-mdm-profile-deletion | Release note for Windows profile deletion behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughAdds Windows MDM profile removal: when a Windows configuration profile is deleted or a host changes teams, Fleet reads stored profile SyncML, generates corresponding SyncML Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/datastore/mysql/microsoft_mdm.go`:
- Around line 1023-1062: The update loop unconditionally flips
host_mdm_windows_profiles to remove/pending and sets a new command_uuid even
when no delete command was enqueued; fix this by tracking which profiles
actually had a delete command inserted and only update those. In the first loop
(where you read profileContents, call fleet.BuildDeleteCommandFromProfileBytes
and ds.mdmWindowsInsertCommandForHostsDB) record successful enqueues (e.g., add
profUUID to a set or mark target with a flag) and skip adding to that set when
syncML is missing or BuildDeleteCommandFromProfileBytes or
mdmWindowsInsertCommandForHostsDB returns an error. In the second loop (the
sqlx.In update block) iterate only over the profUUIDs that were successfully
enqueued (the tracked set or flagged targets) so you only set operation_type =
remove, status = pending and command_uuid for profiles that actually have a
corresponding delete command.
- Around line 986-1002: The SELECT used in selectSentStmt currently only returns
rows with status IS NOT NULL and operation_type = fleet.MDMOperationTypeInstall,
which misses hosts that already have a pending remove (operation_type='remove')
with NULL status; change the WHERE clause to include either sent installs OR any
remove rows by using a compound condition such as "WHERE profile_uuid IN (?) AND
((status IS NOT NULL AND operation_type = ?) OR (operation_type = ?))", adjust
the sqlx.In call to pass profileUUIDs, fleet.MDMOperationTypeInstall,
fleet.MDMOperationTypeRemove as selArgs, and keep the rest of the code using
selStmt/selArgs and sentRows unchanged so removed-but-not-yet-statused hosts are
included.
In `@server/fleet/microsoft_mdm.go`:
- Around line 1719-1726: BuildDeleteCommandFromProfileBytes currently parses the
raw stored SyncML which can produce multiple top-level <Delete> commands for
SCEP cert profiles; mirror the install-side behavior by normalizing SCEP
payloads to ensure they are wrapped in an <Atomic> element before parsing.
Update BuildDeleteCommandFromProfileBytes to call a new helper (e.g.,
NormalizeSCEPPayloadAtomic) on profileBytes prior to
UnmarshallMultiTopLevelXMLProfile so non-atomic SCEP payloads are wrapped into a
single atomic envelope; keep references to BuildDeleteCommandFromProfileBytes
and UnmarshallMultiTopLevelXMLProfile so reviewers can locate the change. Ensure
the normalization only alters SCEP payloads and preserves other profiles' XML
unchanged.
- Around line 1601-1614: The builder that processes
UnmarshallMultiTopLevelXMLProfile(cmdWithSecret.RawCommand) sets statusMapper
based on isRemoveOperation but the per-target failed-atomic detail logic only
inspects ReplaceCommands and AddCommands, so failed Delete (atomic remove)
responses produce empty Detail; update the per-target detail construction to
also inspect cmds.DeleteCommands (in the same place ReplaceCommands and
AddCommands are checked) and include DeleteCommands entries when
isRemoveOperation so failed removes populate the Detail field correctly (ensure
any indexing/ID lookups and merging logic mirror how ReplaceCommands/AddCommands
are handled).
In `@server/service/microsoft_mdm.go`:
- Around line 2598-2623: The loop that builds remove targets (iterating over
toRemove and using removeTargets, cmdTarget, and hostProfilesToUpdate) currently
marks host profiles as pending (appends hp with Status = MDMDeliveryPending and
sets CommandUUID) before the delete command is actually generated/queued;
instead, only create and append the hostProfilesToUpdate entry after
BuildDeleteCommandFromProfileBytes succeeds and the command has been
enqueued/assigned a real command UUID. Concretely: defer creating the hp and
setting Status/CommandUUID inside the remove loop until after successful call to
BuildDeleteCommandFromProfileBytes (and after the code path that queues the
command and confirms the command UUID), update cmdTarget.cmdUUID only when the
command is guaranteed queued, and apply the same change to the equivalent block
referenced by the other occurrence that mirrors lines using
toRemove/removeTargets (the block around the later 2741-2757 section).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e9fe774-b2ff-4aee-8847-3767c7777f77
📒 Files selected for processing (5)
changes/33418-windows-mdm-profile-deletionserver/datastore/mysql/microsoft_mdm.goserver/fleet/microsoft_mdm.goserver/fleet/microsoft_mdm_test.goserver/service/microsoft_mdm.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #42206 +/- ##
==========================================
+ Coverage 66.63% 66.70% +0.06%
==========================================
Files 2532 2524 -8
Lines 202579 202661 +82
Branches 9027 8966 -61
==========================================
+ Hits 134979 135175 +196
+ Misses 55397 55248 -149
- Partials 12203 12238 +35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Address code review: SCEP atomic normalization for delete, failed atomic detail includes DeleteCommands, only update rows when delete command was actually enqueued, batch UPDATE for mark-for-removal, if-else→switch lint fix, remove unused const. - Fix install query to re-install profiles currently marked for removal but back in desired state. - Fix test assertions to account for two-phase Windows profile removal.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/service/integration_mdm_profiles_test.go (1)
4026-4050:⚠️ Potential issue | 🟠 MajorDon’t infer “delete” from the absence of
ReplaceCommands.Add-only Windows installs are valid, and this file already exercises them elsewhere. Classifying every
AtomicwithoutReplaceCommandsas a remove will undercount installs, skip their DB-status assertions, and make this helper return the wrong result as soon as an Add-only profile flows through it.🛠️ Suggested fix
for _, c := range cmds { cmdID := c.Cmd.CmdID status := syncml.CmdStatusOK if c.Verb == "Atomic" { - if len(c.Cmd.ReplaceCommands) > 0 { - // Install command (Atomic with Replace sub-commands) + if len(c.Cmd.DeleteCommands) > 0 { + // Delete command (Atomic with Delete sub-commands) + status = syncml.CmdStatusOK + } else if len(c.Cmd.ReplaceCommands) > 0 || len(c.Cmd.AddCommands) > 0 { + // Install command (Atomic with Replace/Add sub-commands) atomicInstallCmds = append(atomicInstallCmds, c) status = mdmResponseStatus for _, rc := range c.Cmd.ReplaceCommands { require.NotEmpty(t, rc.CmdID) } - } else { - // Delete command (Atomic with Delete sub-commands) - status = syncml.CmdStatusOK + for _, ac := range c.Cmd.AddCommands { + require.NotEmpty(t, ac.CmdID) + } + } else { + require.Fail(t, "unexpected Atomic command without install or delete subcommands") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_mdm_profiles_test.go` around lines 4026 - 4050, The code currently treats any Atomic command without ReplaceCommands as a delete; instead check for explicit delete sub-commands and treat absence of ReplaceCommands as an add-only install. Change the branch in the loop that inspects c.Verb == "Atomic" to: if len(c.Cmd.ReplaceCommands) > 0 -> handle install (append to atomicInstallCmds and set status = mdmResponseStatus); else if len(c.Cmd.DeleteCommands) > 0 -> handle delete (set status = syncml.CmdStatusOK); else -> treat as add-only install (append to atomicInstallCmds and set status = mdmResponseStatus). Update references to c.Cmd.DeleteCommands, atomicInstallCmds, status, mdmResponseStatus, and syncml.CmdStatusOK so add-only Atomics are counted and get DB-status assertions.server/fleet/microsoft_mdm.go (1)
1619-1625:⚠️ Potential issue | 🟠 MajorBest-effort remove doesn't apply inside
<Atomic>.This branch still derives the profile status from the wrapper status only, so nested
<Delete>404/405responses never participate in the overall result. Atomic/SCEP removals can therefore stay failed even when each child delete should count as success for best-effort removal. For remove operations, the atomic path should aggregate the nested command statuses too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/fleet/microsoft_mdm.go` around lines 1619 - 1625, The atomic branch currently sets commandStatus from only the wrapper status (using statuses[cmdWithSecret.CommandUUID]); change it so remove operations inside an <Atomic> aggregate child command statuses instead: iterate the nested child commands present in cmds (the Atomic wrapper's children), look up each child's status in the statuses map, map each child's status via statusMapper(*status.Data), and combine them with best-effort semantics (treat child Delete 404/405 as success) to produce the overall commandStatus; ensure you still fall back to the wrapper status only if child statuses are missing. Use the existing symbols cmds, cmdWithSecret.CommandUUID, statuses, statusMapper, and commandStatus to locate and implement the aggregation.
♻️ Duplicate comments (1)
server/datastore/mysql/microsoft_mdm.go (1)
992-1006:⚠️ Potential issue | 🟠 MajorDon't drop already-pending removes during profile deletion.
Phase 0 deletes every existing
operation_type='remove'row, then Phase 2 reloads only sent installs. If a host was already markedremovewithstatus=NULLbecause it left scope earlier, this path erases the only row for that host and the later reconcile pass has no stored SyncML left to build the<Delete>from. Keep pending remove rows in the Phase 2 source set, or restrict Phase 0 cleanup to terminal remove rows only.As per coding guidelines, when reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity.
Also applies to: 1021-1027
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/microsoft_mdm.go` around lines 992 - 1006, The Phase 0 cleanup currently deletes all rows with operation_type = fleet.MDMOperationTypeRemove via delExistingRemoveStmt which removes pending removes (status IS NULL); change the SQL to only delete terminal remove rows so pending removes are preserved for Phase 2. Modify delExistingRemoveStmt to add a filter (e.g., "AND status IS NOT NULL" or specific terminal status values) so that delRemStmt/delRemArgs and the tx.ExecContext call only remove terminal remove rows, not rows with status NULL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/datastore/mysql/mdm_test.go`:
- Around line 1338-1380: The assertHostProfiles helper is becoming destructive
because it calls cleanupStaleWindowsRemoveRows; remove that call so
assertHostProfiles remains read-only and only verifies DB state. Instead,
advance Windows remove-phase state explicitly from tests by calling the existing
simulateWindowsRemoveReconciliation (or a new explicit cleanup helper) at the
specific phase boundaries that are meant to reconcile/remove Windows remove
rows; keep cleanupStaleWindowsRemoveRows as an explicit utility invoked only by
tests that intend to advance the Windows remove lifecycle. Ensure references to
cleanupStaleWindowsRemoveRows and simulateWindowsRemoveReconciliation are used
from the test phases, not inside assertHostProfiles.
In `@server/datastore/mysql/microsoft_mdm.go`:
- Around line 953-959: The transaction currently deletes the MDM profile row
before attempting to build or persist the SyncML/delete payload, so failures in
SyncML generation (or in cancelWindowsHostInstallsForDeletedMDMProfiles) leave
the DB row gone but no persisted payload to retry; change the flow so you build
and/or persist the delete payload(s) first and call
cancelWindowsHostInstallsForDeletedMDMProfiles (or return its error) before
calling deleteMDMWindowsConfigProfile, or alternatively make
deleteMDMWindowsConfigProfile persist the delete payload atomically with the row
removal; ensure any SyncML/build errors cause the transaction to return an error
(abort) rather than log a warning and continue — apply the same fix to the
similar block around lines 1063-1077.
- Around line 948-951: The preload SELECT using sqlx.GetContext currently
returns raw sql.ErrNoRows which prevents deleteMDMWindowsConfigProfile from
hitting its notFound("MDMWindowsProfile") path; change the preload to detect
sql.ErrNoRows (the error returned by sqlx.GetContext) and, when that occurs,
return the same notFound("MDMWindowsProfile") error that
deleteMDMWindowsConfigProfile expects so missing profiles still result in a
404—locate the sqlx.GetContext(..., &syncML, `SELECT syncml ...`, profileUUID)
call and branch on sql.ErrNoRows to return notFound("MDMWindowsProfile") instead
of wrapping/returning the raw DB error.
---
Outside diff comments:
In `@server/fleet/microsoft_mdm.go`:
- Around line 1619-1625: The atomic branch currently sets commandStatus from
only the wrapper status (using statuses[cmdWithSecret.CommandUUID]); change it
so remove operations inside an <Atomic> aggregate child command statuses
instead: iterate the nested child commands present in cmds (the Atomic wrapper's
children), look up each child's status in the statuses map, map each child's
status via statusMapper(*status.Data), and combine them with best-effort
semantics (treat child Delete 404/405 as success) to produce the overall
commandStatus; ensure you still fall back to the wrapper status only if child
statuses are missing. Use the existing symbols cmds, cmdWithSecret.CommandUUID,
statuses, statusMapper, and commandStatus to locate and implement the
aggregation.
In `@server/service/integration_mdm_profiles_test.go`:
- Around line 4026-4050: The code currently treats any Atomic command without
ReplaceCommands as a delete; instead check for explicit delete sub-commands and
treat absence of ReplaceCommands as an add-only install. Change the branch in
the loop that inspects c.Verb == "Atomic" to: if len(c.Cmd.ReplaceCommands) > 0
-> handle install (append to atomicInstallCmds and set status =
mdmResponseStatus); else if len(c.Cmd.DeleteCommands) > 0 -> handle delete (set
status = syncml.CmdStatusOK); else -> treat as add-only install (append to
atomicInstallCmds and set status = mdmResponseStatus). Update references to
c.Cmd.DeleteCommands, atomicInstallCmds, status, mdmResponseStatus, and
syncml.CmdStatusOK so add-only Atomics are counted and get DB-status assertions.
---
Duplicate comments:
In `@server/datastore/mysql/microsoft_mdm.go`:
- Around line 992-1006: The Phase 0 cleanup currently deletes all rows with
operation_type = fleet.MDMOperationTypeRemove via delExistingRemoveStmt which
removes pending removes (status IS NULL); change the SQL to only delete terminal
remove rows so pending removes are preserved for Phase 2. Modify
delExistingRemoveStmt to add a filter (e.g., "AND status IS NOT NULL" or
specific terminal status values) so that delRemStmt/delRemArgs and the
tx.ExecContext call only remove terminal remove rows, not rows with status NULL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 567675cd-3fe4-4f5e-a6d4-8afd31d77f85
📒 Files selected for processing (7)
server/datastore/mysql/mdm_test.goserver/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.goserver/datastore/mysql/testing_utils.goserver/fleet/microsoft_mdm.goserver/service/integration_mdm_profiles_test.goserver/service/microsoft_mdm.go
💤 Files with no reviewable changes (1)
- server/datastore/mysql/microsoft_mdm_test.go
Lint fixes:
- Remove unused simulateWindowsRemoveReconciliation function
- Replace interface{} with any (modernize lint)
Integration test fixes:
- TestDeleteMDMProfileCancelsInstalls: both Windows hosts have
non-NULL status (pending) after reconciler runs, so both get
remove+pending (not just host4)
- TestWindowsProfileResend: account for <Delete> commands when
profile content changes; clean up queued commands in subtest
cleanup to prevent cross-contamination
- TestHostMDMProfilesExcludeLabels: add profile_name to
windowsProfilesToRemoveQuery so remove rows have correct names
- Preserve NotFound error contract in DeleteMDMWindowsConfigProfile when profile doesn't exist (Copilot, CodeRabbit) - Restore IsNotFound assertion in TestMDMWindowsConfigProfiles - Only delete remove+verified rows in response handler cleanup, not verifying (Copilot: verifying is in-flight, not terminal) - Restrict install query for remove rows to exclude verifying/verified to prevent install churn while Delete is in-flight (Copilot) - Use 16-byte zero checksum instead of empty slice for BINARY(16) column compatibility (Copilot) - Add deprecation comment to UpdateOrDeleteHostMDMWindowsProfile explaining it is superseded by response handler cleanup - Remove SQL comment containing ? inside query template (was treated as bind variable by sqlx.In, causing runtime errors)
cleanupStaleWindowsRemoveRows now only queries and deletes remove rows for hosts present in the current want map, instead of scanning all remove rows in the table. This prevents implicitly hiding issues for hosts the current test phase doesn't check.
- WindowsResponseToDeliveryStatusForRemove now treats 500 (Command Failed) as success. Windows returns 500 (not 405) for CSP nodes that do not support <Delete>, such as DeviceLock/AccountLockoutPolicy and some SystemServices nodes. Since removal is best-effort, this prevents permanent remove+failed rows in host details. - TestDeleteMDMProfileCancelsInstalls: fix second Windows assertion at line 6927 to expect persistent remove+pending rows (no simulated device check-in processes the <Delete> command in this test). - TestHostMDMProfilesExcludeLabels: fix assertion to expect install (not remove) after label exclusion is removed and profile becomes desired again -- the install query correctly flips remove rows back to install when the profile re-enters the desired state. - TestWindowsProfileResend: content change updates profile in place (same name, different checksum) so only a re-install is sent, not a delete+install pair.
@JordanMontgomery |
JordanMontgomery
left a comment
JordanMontgomery
left a comment
There was a problem hiding this comment.
@getvictor since there is a followup PR coming I think this is probably OK but I would try to test the following scenario:
- Admin registers system for MDM
- Admin uploads profile
- Profile synced to system
- System shutdown
- Admin deletes profile enqueuing deletes
- Admin uploads profile again enqueuing adds
- Device checks in - Do the DELETEs get added before or after the ADDs in the commands sent to the device?
It seems like we'd maybe want the commands sent to the device to be ordered. Because we send them all as one big blob today we don't do that but it didn't necessarily matter as much. Now it matters a lot more. I know you filed a bug around this but I think this new functionality perhaps makes this worse
…delete # Conflicts: # server/datastore/mysql/microsoft_mdm.go
@JordanMontgomery Yes, this is an issue. I will fix it by ordering by created_at in the follow up. Also, this will not completely solve determinism since profiles created in a batch can have the same created_at. |
Related issue: Resolves #33418
Demo video: https://www.youtube.com/watch?v=gtsIYxmIOSo
Docs: https://github.com/fleetdm/fleet/pull/42269/changes
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Summary by CodeRabbit
New Features
Tests