Fix handling of cross-team stale Windows MDM profiles after host movement#43990
Fix handling of cross-team stale Windows MDM profiles after host movement#43990
Conversation
…ment - Added team scoping to LocURI protection for profile deletion. - Addressed stale MDM profile rows when hosts transfer between teams. - Improved test coverage for Windows MDM profile deletion scenarios.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/agentic_review |
Code Review by Qodo
1.
|
There was a problem hiding this comment.
Pull request overview
Fixes Windows MDM configuration profile deletion behavior when hosts move between teams, ensuring LocURI protection and stale host-profile rows are handled without cross-team interference (Resolves #33418).
Changes:
- Pass the profile’s team ID through the Windows profile deletion path to correctly scope LocURI protection.
- Update team deletion flow to pass the team ID into the shared “cancel/enqueue deletes” helper.
- Add a regression test covering deletion after a host moves teams (stale cross-team
host_mdm_windows_profilesrows).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
server/datastore/mysql/teams.go |
Passes tid into the Windows delete-command enqueue helper to maintain correct team scoping. |
server/datastore/mysql/microsoft_mdm.go |
Loads team_id with SyncML for deletion and scopes LocURI protection to the profile’s team via a new parameter. |
server/datastore/mysql/microsoft_mdm_test.go |
Adds a regression test for deleting a no-team profile after some affected hosts have moved to another team. |
Comments suppressed due to low confidence (1)
server/datastore/mysql/microsoft_mdm.go:1379
- The active-profiles query used to build the LocURI protection set silently ignores errors (both sqlx.In errors and SelectContext errors). If this query fails,
activeLocURIsstays empty and the deletion path may enqueue commands for LocURIs that are still enforced by other profiles, which can unintentionally revert settings. Please return the error (or at least log and fail the operation) instead of proceeding with an empty protection set.
SELECT profile_uuid, syncml FROM mdm_windows_configuration_profiles
WHERE team_id = ? AND profile_uuid NOT IN (?)`
apStmt, apArgs, apErr := sqlx.In(activeProfilesStmt, profTeamID, profileUUIDs)
if apErr == nil {
var activeProfiles []struct {
ProfileUUID string `db:"profile_uuid"`
SyncML []byte `db:"syncml"`
}
if err := sqlx.SelectContext(ctx, tx, &activeProfiles, apStmt, apArgs...); err == nil {
for _, ap := range activeProfiles {
// Substitute SCEP variable so LocURIs are compared on
// resolved paths, consistent with the deleted profile side.
resolved := fleet.FleetVarSCEPWindowsCertificateIDRegexp.ReplaceAll(ap.SyncML, []byte(ap.ProfileUUID))
for _, uri := range fleet.ExtractLocURIsFromProfileBytes(resolved) {
activeLocURIs[uri] = struct{}{}
locURIToProtectingProfiles[uri] = append(locURIToProtectingProfiles[uri], ap.ProfileUUID)
}
}
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThe patch threads a team ID through Windows MDM profile-deletion paths: Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/datastore/mysql/microsoft_mdm.go (1)
1359-1404:⚠️ Potential issue | 🟠 MajorDon’t fail open when the protection lookup errors.
If
sqlx.InorSelectContextfails,activeLocURIsstays empty and the later partition treats every LocURI as safe, which can enqueue<Delete>for settings still enforced by other profiles inprofTeamID. Please fail closed for command generation: warn and treat all deleted-profile LocURIs as protected rather than generating unprotected deletes.🛡️ Proposed fail-closed handling
activeLocURIs := make(map[string]struct{}) // Map each protected LocURI to the profile UUIDs that protect it, // so pass 2 can check per-host applicability. locURIToProtectingProfiles := make(map[string][]string) + activeProfileProtectionLookupFailed := false if len(profileUUIDs) > 0 { // Query profile UUIDs and SyncML from profiles in the same team that // are NOT being deleted. const activeProfilesStmt = ` SELECT profile_uuid, syncml FROM mdm_windows_configuration_profiles WHERE team_id = ? AND profile_uuid NOT IN (?)` apStmt, apArgs, apErr := sqlx.In(activeProfilesStmt, profTeamID, profileUUIDs) - if apErr == nil { + if apErr != nil { + activeProfileProtectionLookupFailed = true + ds.logger.WarnContext(ctx, "skipping Windows MDM profile delete command generation: building active profile protection query failed", + "team_id", profTeamID, "err", apErr) + } else { var activeProfiles []struct { ProfileUUID string `db:"profile_uuid"` SyncML []byte `db:"syncml"` } - if err := sqlx.SelectContext(ctx, tx, &activeProfiles, apStmt, apArgs...); err == nil { + if err := sqlx.SelectContext(ctx, tx, &activeProfiles, apStmt, apArgs...); err != nil { + activeProfileProtectionLookupFailed = true + ds.logger.WarnContext(ctx, "skipping Windows MDM profile delete command generation: loading active profiles for LocURI protection failed", + "team_id", profTeamID, "err", err) + } else { for _, ap := range activeProfiles { // Substitute SCEP variable so LocURIs are compared on // resolved paths, consistent with the deleted profile side. resolved := fleet.FleetVarSCEPWindowsCertificateIDRegexp.ReplaceAll(ap.SyncML, []byte(ap.ProfileUUID)) for _, uri := range fleet.ExtractLocURIsFromProfileBytes(resolved) { @@ // Partition into safe (not protected) and protected (in activeLocURIs). var safeURIs, protectedURIs []string - for _, uri := range allURIs { - if _, isProtected := activeLocURIs[uri]; isProtected { - protectedURIs = append(protectedURIs, uri) - } else { - safeURIs = append(safeURIs, uri) + if activeProfileProtectionLookupFailed { + protectedURIs = append(protectedURIs, allURIs...) + } else { + for _, uri := range allURIs { + if _, isProtected := activeLocURIs[uri]; isProtected { + protectedURIs = append(protectedURIs, uri) + } else { + safeURIs = append(safeURIs, uri) + } } }Based on learnings, Windows MDM profile deletion is intentionally best-effort and delete-command generation/enqueue failures should be logged as warnings rather than returned as errors.
🤖 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 1359 - 1404, The protection lookup can fail silently (sqlx.In / sqlx.SelectContext) leaving activeLocURIs empty and causing dangerous open-fail deletes; update the block around activeProfilesStmt / sqlx.In / sqlx.SelectContext (and the variables activeLocURIs, locURIToProtectingProfiles) to detect any error path, log a warning via ds.logger.WarnContext including the error, set a boolean (e.g., protectionLookupFailed) and on that flag, when partitioning in the loop over targets (the code using allURIs, safeURIs, protectedURIs), treat all extracted URIs as protected (append them to protectedURIs and populate locURIToProtectingProfiles appropriately) instead of treating them as safe; ensure warnings are emitted but do not return an error (fail-closed for command generation).
🧹 Nitpick comments (1)
server/datastore/mysql/microsoft_mdm_test.go (1)
4407-4414: Add a cross-team LocURI collision to prove protection is team-scoped.This test catches the mixed-team stale-row failure, but it would still pass if LocURI protection accidentally considered active profiles from the moved host’s destination team. Add a destination-team profile with the same LocURI as
profToDelete; the existing moved-host assertion should still require./Device/Delete/Me.🧪 Suggested coverage hardening
err = ds.withTx(ctx, func(tx sqlx.ExtContext) error { _, err := ds.batchSetMDMWindowsProfilesDB(ctx, tx, nil, []*fleet.MDMWindowsConfigProfile{profToDelete, profToKeep}, nil) return err }) require.NoError(t, err) + + // Add a destination-team profile with the same LocURI. Deleting the no-team + // profile should not treat this team-scoped profile as protection. + profTeamSameLocURI := &fleet.MDMWindowsConfigProfile{ + Name: "teamProfSameLocURI", + SyncML: []byte(`<Replace><Item><Target><LocURI>./Device/Delete/Me</LocURI></Target><Data>2</Data></Item></Replace>`), + } + err = ds.withTx(ctx, func(tx sqlx.ExtContext) error { + _, err := ds.batchSetMDMWindowsProfilesDB(ctx, tx, &team.ID, []*fleet.MDMWindowsConfigProfile{profTeamSameLocURI}, nil) + return err + }) + require.NoError(t, err)Also applies to: 4470-4484
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/microsoft_mdm_test.go` around lines 4407 - 4414, Add a profile in the destination team that has the same LocURI as profToDelete to ensure LocURI collision protection is team-scoped: modify the test around the profToDelete/profToKeep insertion (the withTx call that calls ds.batchSetMDMWindowsProfilesDB) to also insert a fleet.MDMWindowsConfigProfile assigned to the destination team with identical SyncML LocURI "./Device/Delete/Me" and assert the moved-host logic still targets the profToDelete LocURI; apply the same addition for the similar block at the other location (lines referenced by the reviewer) so cross-team collisions are covered.
🤖 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_test.go`:
- Around line 4417-4422: The SQL lookups that populate profToDeleteUUID and
profToKeepUUID (inside the ExecAdhocSQL calls using sqlx.GetContext and querying
mdm_windows_configuration_profiles WHERE name = 'profDelete' / 'profKeep') are
not scoped to the no-team row; update those SELECT statements to add "AND
team_id = 0" so they only match the global (no-team) profile, and apply the same
change to the other similar profile lookup(s) later in the file that use the
same pattern.
---
Outside diff comments:
In `@server/datastore/mysql/microsoft_mdm.go`:
- Around line 1359-1404: The protection lookup can fail silently (sqlx.In /
sqlx.SelectContext) leaving activeLocURIs empty and causing dangerous open-fail
deletes; update the block around activeProfilesStmt / sqlx.In /
sqlx.SelectContext (and the variables activeLocURIs, locURIToProtectingProfiles)
to detect any error path, log a warning via ds.logger.WarnContext including the
error, set a boolean (e.g., protectionLookupFailed) and on that flag, when
partitioning in the loop over targets (the code using allURIs, safeURIs,
protectedURIs), treat all extracted URIs as protected (append them to
protectedURIs and populate locURIToProtectingProfiles appropriately) instead of
treating them as safe; ensure warnings are emitted but do not return an error
(fail-closed for command generation).
---
Nitpick comments:
In `@server/datastore/mysql/microsoft_mdm_test.go`:
- Around line 4407-4414: Add a profile in the destination team that has the same
LocURI as profToDelete to ensure LocURI collision protection is team-scoped:
modify the test around the profToDelete/profToKeep insertion (the withTx call
that calls ds.batchSetMDMWindowsProfilesDB) to also insert a
fleet.MDMWindowsConfigProfile assigned to the destination team with identical
SyncML LocURI "./Device/Delete/Me" and assert the moved-host logic still targets
the profToDelete LocURI; apply the same addition for the similar block at the
other location (lines referenced by the reviewer) so cross-team collisions are
covered.
🪄 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: 94ec5625-e44e-490f-9097-7245e91db104
📒 Files selected for processing (3)
server/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.goserver/datastore/mysql/teams.go
|
@claude review once |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix looks correct to me — team ID is now a property of the profile being deleted rather than derived from potentially stale host joins, and all four call sites pass it correctly. Deferring to a human since this touches production Windows MDM deletion logic and the LocURI protection semantics are non-trivial.
Extended reasoning...
Overview
This PR fixes an unreleased bug where cancelWindowsHostInstallsForDeletedMDMProfiles could fail with "expected profiles from 1 team, got 2" when hosts had been moved between teams while still carrying host_mdm_windows_profiles rows referencing the previous teams profile UUID. The fix threads profTeamID uint through the function signature so team scoping is driven by the profile rather than derived from host joins, updates all four call sites, and adds a reproducing test.
Security risks
This touches Windows MDM profile deletion — security-adjacent because wrong behavior could either fail to remove an uninstalled setting or spuriously delete a setting still enforced by another profile. The LocURI protection set is now scoped by team_id = ? so cross-team protection leaks are prevented, and the previously swallowed sqlx.In / SelectContext errors are now wrapped via ctxerr.Wrap (addressing Qodos feedback). No new injection, auth, or data-exposure surface is introduced.
Level of scrutiny
Moderate-to-high. The diff is focused (~80 lines of logic + a solid reproducing test) and the signature change is mechanical, but the domain — MDM profile deletion with LocURI protection semantics across team moves — is subtle. Production correctness depends on every caller passing the profiles team rather than an arbitrary team scope. I verified this is the case for all four callers.
Other factors
- Good test: the new subtest reproduces the regression (stale rows from a moved host) and also guards against cross-team LocURI protection leakage by inserting a destination-team profile with the same
./Device/ZLocURI. - CodeRabbits suggestion to scope the test SELECTs by
team_id = 0is a reasonable nit but marked resolved. - Comments in the diff clearly explain why the team must be passed rather than derived.
- No changes file present in the diff — the PR description notes this is an unreleased bug, but the submitter checklist is unticked.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #43990 +/- ##
=======================================
Coverage 65.10% 65.10%
=======================================
Files 2604 2605 +1
Lines 253885 253941 +56
Branches 9367 9367
=======================================
+ Hits 165288 165333 +45
- Misses 75798 75806 +8
- Partials 12799 12802 +3
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:
|
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/datastore/mysql/microsoft_mdm.go (1)
1424-1455:⚠️ Potential issue | 🟠 MajorKeep rows when pass 2 can still enqueue a delete.
When all LocURIs are protected in pass 1,
deleteCmd == nil, so lines 1428-1436 delete the host-profile rows before pass 2 runs. If pass 2 then enqueues a supplemental<Delete>, that command has nohost_mdm_windows_profilesrow/command_uuidto track, and response handling cannot resolve it asoperation_type='remove'for remove-specific status handling. Defer this cleanup until after pass 2, or have pass 2 return the enqueued host/command mapping and update those rows toremove/pending.🤖 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 1424 - 1455, The current code deletes host/profile rows (the delSkipStmt block that removes rows from host_mdm_windows_profiles when deleteCmd == nil) before running pass 2, which can remove the host_mdm_windows_profiles -> command_uuid linkage needed if checkAndEnqueueLabelScopedDeletes later enqueues supplemental <Delete> commands; instead, defer that deletion until after pass 2 and only remove rows for hosts that did not receive an enqueued delete. Concretely: move the deletion logic out of the deleteCmd == nil branch and run it after ds.checkAndEnqueueLabelScopedDeletes returns (or change checkAndEnqueueLabelScopedDeletes to return the set of (host_uuid, command_uuid/operation_type) it enqueued); then update host_mdm_windows_profiles rows to set operation_type='remove' and command_uuid for any enqueued supplemental deletes and only delete rows for hosts that remain without an enqueued delete. Reference symbols: deleteCmd, pass2Params, protectedURIs, host_mdm_windows_profiles, command_uuid, operation_type, and ds.checkAndEnqueueLabelScopedDeletes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/datastore/mysql/microsoft_mdm.go`:
- Around line 1424-1455: The current code deletes host/profile rows (the
delSkipStmt block that removes rows from host_mdm_windows_profiles when
deleteCmd == nil) before running pass 2, which can remove the
host_mdm_windows_profiles -> command_uuid linkage needed if
checkAndEnqueueLabelScopedDeletes later enqueues supplemental <Delete> commands;
instead, defer that deletion until after pass 2 and only remove rows for hosts
that did not receive an enqueued delete. Concretely: move the deletion logic out
of the deleteCmd == nil branch and run it after
ds.checkAndEnqueueLabelScopedDeletes returns (or change
checkAndEnqueueLabelScopedDeletes to return the set of (host_uuid,
command_uuid/operation_type) it enqueued); then update host_mdm_windows_profiles
rows to set operation_type='remove' and command_uuid for any enqueued
supplemental deletes and only delete rows for hosts that remain without an
enqueued delete. Reference symbols: deleteCmd, pass2Params, protectedURIs,
host_mdm_windows_profiles, command_uuid, operation_type, and
ds.checkAndEnqueueLabelScopedDeletes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c095777f-886c-4fe9-abc6-a594f899bef5
📒 Files selected for processing (2)
server/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.go
Unreleased bug
Related issue: Resolves #33418
Checklist for submitter
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
For unreleased bug fixes in a release candidate, one of:
Summary by CodeRabbit