CSAH: clear state on ABM re-enrollment#44722
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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.
| if err != nil { | ||
| return ctxerr.Wrap(ctx, err, "associating host with idp account") | ||
| } | ||
| } else if idpUUID == "" && len(hosts) > 0 { |
There was a problem hiding this comment.
While this handles OTA enrollment, I found it made sense to keep the parity up between OTA and ADE "in-sync".
This is called via the Install profile from the device on the /ota_enrollment.
There was a problem hiding this comment.
Pull request overview
This PR adds ABM/ADE re-enrollment reset logic to Apple MDM TokenUpdate, plus datastore support and tests, so Fleet can clear more host-scoped state when a device re-enrolls during Setup Assistant.
Changes:
- Gate a new Apple re-enrollment reset in
TokenUpdateonAwaitingConfiguration,TokenUpdateTally == 1, and the existing macOS migration skip. - Add
MDMAppleResetOnReenrollmentin the MySQL datastore, including transactional upcoming-activity cancellation and host-state cleanup. - Add service/datastore tests and a changelog entry for the new behavior.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/apple_mdm.go | Calls the new datastore reset during qualifying Apple TokenUpdate flows. |
| server/service/apple_mdm_test.go | Adds coverage for the new TokenUpdate gating behavior and preserve flag forwarding. |
| server/mock/datastore_mock.go | Extends the datastore mock with the new re-enrollment reset method. |
| server/fleet/datastore.go | Adds the new datastore interface method and its comment. |
| server/datastore/mysql/apple_mdm.go | Implements the new re-enrollment reset and adds IdP-association cleanup in related enrollment paths. |
| server/datastore/mysql/apple_mdm_test.go | Adds datastore tests for host-scoped reset behavior and preserve flag behavior. |
| server/datastore/mysql/activities.go | Extracts a tx-aware batch upcoming-activity cancellation helper for reuse. |
| changes/37682-clear-state-on-abm-re-enrollment | Adds the user-visible changelog entry for the feature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds Datastore.MDMAppleResetOnReenrollment(ctx, hostUUID, preserveHostActivities) which cancels upcoming host activities, deletes host-scoped Apple MDM rows (label membership, host_mdm_commands, host_mdm_idp_accounts, and a set of host vitals/reference tables), and optionally deletes past activity history and deactivates nano enrollment queue entries. Refactors batch upcoming-activity cancellation into a transaction-aware helper, clears host_mdm_idp_accounts on certain OTA/enroll-ref conditions, and invokes the reset from TokenUpdate when gated by nano-enrollment, TokenUpdateTally, and migration flags. Tests and mock updates for the new method and TokenUpdate behavior were added. 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 3
🧹 Nitpick comments (1)
server/datastore/mysql/apple_mdm_test.go (1)
5559-5563: ⚡ Quick winAdd an explicit idempotency assertion for repeated resets.
The method contract is “safe to call multiple times,” but each branch currently validates only a single call. A second call assertion would lock that down and prevent regressions.
Suggested test additions
require.NoError(t, ds.MDMAppleResetOnReenrollment(ctx, hostA.UUID, true)) // host A: everything cleared assert.Equal(t, counts{label: 0, upcoming: 0}, countRows(t, hostA)) + // idempotent on repeated execution + require.NoError(t, ds.MDMAppleResetOnReenrollment(ctx, hostA.UUID, true)) + assert.Equal(t, counts{label: 0, upcoming: 0}, countRows(t, hostA)) // host B: untouched (control - proves the reset is host-scoped) assert.Equal(t, seeded, countRows(t, hostB))require.NoError(t, ds.MDMAppleResetOnReenrollment(ctx, hostB.UUID, false)) assert.Equal(t, activityCounts{pastActivity: 0, activeQueue: 0, inactiveQueue: 2, ackedCommand: 1}, countActivityRows(t, hostB)) + // idempotent on repeated execution + require.NoError(t, ds.MDMAppleResetOnReenrollment(ctx, hostB.UUID, false)) + assert.Equal(t, + activityCounts{pastActivity: 0, activeQueue: 0, inactiveQueue: 2, ackedCommand: 1}, + countActivityRows(t, hostB)) })Also applies to: 5667-5670
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/apple_mdm_test.go` around lines 5559 - 5563, Add an explicit idempotency assertion by calling ds.MDMAppleResetOnReenrollment(ctx, hostA.UUID, true) a second time and asserting no error and that counts remain unchanged via countRows(t, hostA); update both occurrences (the block around MDMAppleResetOnReenrollment/assert.Equal at the shown location and the similar block at the other location ~5667-5670) so the test verifies the method is safe to call repeatedly and the hostA row counts stay the same after the repeated reset.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changes/37682-clear-state-on-abm-re-enrollment`:
- Line 1: Fix the typo in the changelog entry by replacing the misspelled word
"activites" with the correct spelling "activities" in the changelog string (the
text line "Implemented clearing host vitals on AB host re-enrollment, with a
config option to preserve past host activites."); update that exact string to
use "activities".
In `@server/datastore/mysql/apple_mdm.go`:
- Around line 8434-8435: The UPDATE is too broad: in apple_mdm.go the
tx.ExecContext call that deactivates nano_enrollment_queue using "WHERE id IN
(SELECT id FROM nano_enrollments WHERE device_id = ?)" (hostUUID) will cancel
unrelated commands; narrow the WHERE to only the queue rows associated with the
specific execution/activity IDs you are deleting. Modify the query in the same
function to join/filter by the execution IDs (e.g., executionIDs or activityIDs
used in the surrounding cleanup logic) — for example filter
nano_enrollment_queue by execution_id IN (...) or join to
nano_enrollment_commands/nano_activities to match the exact activity-linked
executions rather than all rows for the device UUID.
- Around line 8384-8426: The reset loop in MDMAppleResetOnReenrollment uses
appleHostRefsForMDMReset but omits host_mdm_idp_accounts and
host_maintenance_windows (and contains a duplicate "host_disks"), so add
"host_mdm_idp_accounts" and "host_maintenance_windows" to the
appleHostRefsForMDMReset slice and remove the duplicate "host_disks" entry (or
explicitly execute DELETEs for those two tables before/after the existing loop)
so IdP associations and maintenance windows are removed during reenrollment
reset; verify clearHostActivitiesForAppleMDMReset remains unchanged for
activity-specific cleanup.
---
Nitpick comments:
In `@server/datastore/mysql/apple_mdm_test.go`:
- Around line 5559-5563: Add an explicit idempotency assertion by calling
ds.MDMAppleResetOnReenrollment(ctx, hostA.UUID, true) a second time and
asserting no error and that counts remain unchanged via countRows(t, hostA);
update both occurrences (the block around
MDMAppleResetOnReenrollment/assert.Equal at the shown location and the similar
block at the other location ~5667-5670) so the test verifies the method is safe
to call repeatedly and the hostA row counts stay the same after the repeated
reset.
🪄 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: f6de5ac0-8551-4f1b-82dd-d0c55d9f13ca
📒 Files selected for processing (8)
changes/37682-clear-state-on-abm-re-enrollmentserver/datastore/mysql/activities.goserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_test.goserver/fleet/datastore.goserver/mock/datastore_mock.goserver/service/apple_mdm.goserver/service/apple_mdm_test.go
| if _, err := tx.ExecContext(ctx, "UPDATE nano_enrollment_queue SET active = 0 WHERE active = 1 AND id IN (SELECT id FROM nano_enrollments WHERE device_id = ?)", hostUUID); err != nil { | ||
| return ctxerr.Wrap(ctx, err, "deactivate nano enrollment queue for mdm reset", "host_uuid", hostUUID) |
There was a problem hiding this comment.
The nano queue cleanup is broader than the activities being removed.
This update deactivates every active nano_enrollment_queue row for any enrollment attached to the device ID. When preserveHostActivities is false, that can also cancel unrelated fresh commands from the reenrollment flow, not just commands backed by the host activities you're deleting here. Please scope this to the specific execution IDs / activity-linked commands being cleared.
As per coding guidelines, added SQL queries should apply precise filtering for the specific entity being updated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/datastore/mysql/apple_mdm.go` around lines 8434 - 8435, The UPDATE is
too broad: in apple_mdm.go the tx.ExecContext call that deactivates
nano_enrollment_queue using "WHERE id IN (SELECT id FROM nano_enrollments WHERE
device_id = ?)" (hostUUID) will cancel unrelated commands; narrow the WHERE to
only the queue rows associated with the specific execution/activity IDs you are
deleting. Modify the query in the same function to join/filter by the execution
IDs (e.g., executionIDs or activityIDs used in the surrounding cleanup logic) —
for example filter nano_enrollment_queue by execution_id IN (...) or join to
nano_enrollment_commands/nano_activities to match the exact activity-linked
executions rather than all rows for the device UUID.
There was a problem hiding this comment.
This seems like it's technically true, however with the current setup we don't queue any commands before this point (our apple_mdm worker runs after this).
Not sure how we would be able to narrow it down further, but also from my testing, this did not cause any problems.
| return "", ctxerr.New(ctx, "machine info is nil") | ||
| } | ||
|
|
||
| if enrollRef == "" { |
There was a problem hiding this comment.
This here handles clearing out the host_mdm_idp_accounts if we enroll without an IDP reference.
Before we upserted with an empty row, which might have mostly behaved the same, but I found it a bit weird.
There was a problem hiding this comment.
It will then later in the Authenticate step, reconciled into the host_emails and host_scim_user tables, or removed if we remove it here.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #44722 +/- ##
==========================================
+ Coverage 66.68% 66.70% +0.01%
==========================================
Files 2651 2651
Lines 213440 213568 +128
Branches 9638 9638
==========================================
+ Hits 142327 142454 +127
+ Misses 58149 58148 -1
- Partials 12964 12966 +2
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.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/datastore/mysql/apple_mdm.go (1)
8411-8421:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
host_mdm_idp_accountsstill survives the reenrollment reset.This cleanup only deletes
host_id-keyed tables, so the stale IdP association keyed byhost_uuidis left behind after the firstTokenUpdate. That can carry the previous user's identity into the new enrollment.Suggested fix
for _, table := range appleHostRefsForMDMReset { if _, err := tx.ExecContext(ctx, fmt.Sprintf("DELETE FROM %s WHERE host_id = ?", table), hostID); err != nil { return ctxerr.Wrap(ctx, err, fmt.Sprintf("clear %s for mdm reset", table), "host_uuid", hostUUID) } } + + if _, err := tx.ExecContext(ctx, `DELETE FROM host_mdm_idp_accounts WHERE host_uuid = ?`, hostUUID); err != nil { + return ctxerr.Wrap(ctx, err, "clear host_mdm_idp_accounts for mdm reset", "host_uuid", hostUUID) + } if !preserveHostActivities { if err := ds.clearHostActivitiesForAppleMDMReset(ctx, tx, hostUUID, hostID); err != nil { return ctxerr.Wrap(ctx, err, "clear host activities for mdm reset") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/apple_mdm.go` around lines 8411 - 8421, The code currently deletes only host_id-keyed tables in the apple MDM reset loop (appleHostRefsForMDMReset) but leaves host_mdm_idp_accounts (which is keyed by host_uuid) intact, so add an explicit deletion of host_mdm_idp_accounts by host_uuid before/after the existing loop; call tx.ExecContext(ctx, "DELETE FROM host_mdm_idp_accounts WHERE host_uuid = ?", hostUUID) and wrap errors with ctxerr.Wrap (matching the style used elsewhere), or alternatively include host_mdm_idp_accounts in the cleanup using hostUUID and ensure the error context includes "clear host_mdm_idp_accounts for mdm reset" and the host_uuid value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/datastore/mysql/apple_mdm.go`:
- Around line 8399-8405: The host lookup here uses a non-deterministic SELECT by
uuid; replace it with the same deterministic host-resolution used by
MDMResetEnrollment so both flows pick the same host record. Locate the SQL/logic
in MDMResetEnrollment that resolves a host UUID to a single host id (or extract
it into a helper like resolveHostIDByUUID), and use that exact query/path in
MDMAppleResetOnReenrollment (instead of the current simple `SELECT id FROM hosts
WHERE uuid = ?`), preserving its ordering/joins/limits and error handling so
duplicate-UUID rows are resolved identically.
---
Duplicate comments:
In `@server/datastore/mysql/apple_mdm.go`:
- Around line 8411-8421: The code currently deletes only host_id-keyed tables in
the apple MDM reset loop (appleHostRefsForMDMReset) but leaves
host_mdm_idp_accounts (which is keyed by host_uuid) intact, so add an explicit
deletion of host_mdm_idp_accounts by host_uuid before/after the existing loop;
call tx.ExecContext(ctx, "DELETE FROM host_mdm_idp_accounts WHERE host_uuid =
?", hostUUID) and wrap errors with ctxerr.Wrap (matching the style used
elsewhere), or alternatively include host_mdm_idp_accounts in the cleanup using
hostUUID and ensure the error context includes "clear host_mdm_idp_accounts for
mdm reset" and the host_uuid value.
🪄 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: 8fb63b0f-95b5-41af-92dc-9530eed4a25c
📒 Files selected for processing (5)
changes/37682-clear-state-on-abm-re-enrollmentserver/datastore/mysql/apple_mdm.goserver/fleet/datastore.goserver/service/apple_mdm.goserver/service/apple_mdm_test.go
✅ Files skipped from review due to trivial changes (1)
- changes/37682-clear-state-on-abm-re-enrollment
🚧 Files skipped from review as they are similar to previous changes (2)
- server/fleet/datastore.go
- server/service/apple_mdm_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/datastore/mysql/apple_mdm.go`:
- Around line 8382-8395: The appleHostRefsForMDMReset slice omits the
"host_device_auth" table causing MDMAppleResetOnReenrollment to leave stale
device-auth state; update the appleHostRefsForMDMReset variable to include
"host_device_auth" so MDMAppleResetOnReenrollment will clear device auth rows
for a host during re-enrollment.
🪄 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: 0632e2bb-37c4-46ae-b4ac-cdc3e953598a
📒 Files selected for processing (1)
server/datastore/mysql/apple_mdm.go
| // appleHostRefsForMDMReset are the tables referenced by host ids. | ||
| // These tables are cleared when the host is re-enrolled. | ||
| var appleHostRefsForMDMReset = []string{ | ||
| "label_membership", | ||
| "host_disks", | ||
| "host_mdm_commands", | ||
| "host_batteries", | ||
| "host_operating_system", | ||
| "host_certificates", | ||
| "host_issues", | ||
| "host_last_known_locations", | ||
| "host_munki_info", | ||
| "host_munki_issues", | ||
| } |
There was a problem hiding this comment.
Reset still leaves host_device_auth behind.
MDMAppleResetOnReenrollment doesn't include host_device_auth in the host-scoped tables it clears, so a wiped/re-enrolled device can retain stale device-auth state after the first TokenUpdate.
🧹 Minimal fix
var appleHostRefsForMDMReset = []string{
"label_membership",
"host_disks",
"host_mdm_commands",
"host_batteries",
"host_operating_system",
"host_certificates",
"host_issues",
+ "host_device_auth",
"host_last_known_locations",
"host_munki_info",
"host_munki_issues",
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // appleHostRefsForMDMReset are the tables referenced by host ids. | |
| // These tables are cleared when the host is re-enrolled. | |
| var appleHostRefsForMDMReset = []string{ | |
| "label_membership", | |
| "host_disks", | |
| "host_mdm_commands", | |
| "host_batteries", | |
| "host_operating_system", | |
| "host_certificates", | |
| "host_issues", | |
| "host_last_known_locations", | |
| "host_munki_info", | |
| "host_munki_issues", | |
| } | |
| // appleHostRefsForMDMReset are the tables referenced by host ids. | |
| // These tables are cleared when the host is re-enrolled. | |
| var appleHostRefsForMDMReset = []string{ | |
| "label_membership", | |
| "host_disks", | |
| "host_mdm_commands", | |
| "host_batteries", | |
| "host_operating_system", | |
| "host_certificates", | |
| "host_issues", | |
| "host_device_auth", | |
| "host_last_known_locations", | |
| "host_munki_info", | |
| "host_munki_issues", | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/datastore/mysql/apple_mdm.go` around lines 8382 - 8395, The
appleHostRefsForMDMReset slice omits the "host_device_auth" table causing
MDMAppleResetOnReenrollment to leave stale device-auth state; update the
appleHostRefsForMDMReset variable to include "host_device_auth" so
MDMAppleResetOnReenrollment will clear device auth rows for a host during
re-enrollment.
There was a problem hiding this comment.
That should be fine IMO.
Related issue: Resolves #43945
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary 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
Bug Fixes
Tests