iOS/iPadOS device vitals: storage + DeviceInformation command (1/2) - #50046
Conversation
…e vitals Adds host_mdm_apple_device_vitals and host_mdm_apple_service_subscriptions tables plus the datastore method to persist them, and expands the DeviceInformation MDM command to request the additional Apple query keys needed to populate them. Part of #49984.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
WalkthroughAdds Apple MDM device vitals and service subscription models, datastore APIs, MySQL tables, migrations, and persistence with update/insert semantics. Service subscriptions are refreshed by removing stale slots and upserting current slots. Device Information commands now generate their query list from an exported key slice. Host deletion cleanup includes the new tables, with datastore, migration, command, null-handling, replacement, and cleanup tests. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 3
🧹 Nitpick comments (2)
server/mdm/apple/commander.go (1)
463-501: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid exposing a mutable global query slice.
DeviceInformationQueryKeyscan be reassigned, reordered, appended to, or have individual entries mutated by callers, changing every subsequent command. Keep the canonical list private and expose a copy/accessor if external consumers need it. Confirm whether this export is intended as public API.🤖 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/mdm/apple/commander.go` around lines 463 - 501, Make the canonical DeviceInformation query list private so callers cannot reassign or mutate shared state, and update internal references to use it. If external consumers require access, expose an accessor that returns a fresh copy in the declared request order; otherwise remove the export and confirm no public API depends on it.server/mdm/apple/commander_test.go (1)
1136-1136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not discard push-factory setup errors.
Capture the error from
newMockAPNSPushProviderFactoryand fail the test before constructing the push service, so setup failures are reported at their source.🤖 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/mdm/apple/commander_test.go` at line 1136, Update the test setup around newMockAPNSPushProviderFactory to capture and assert its returned error. Fail the test immediately when factory creation fails, before constructing the push service, while preserving the existing pushFactory usage for successful setup.
🤖 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_device_vitals.go`:
- Around line 183-186: Replace the update-then-insert fallbacks in the
device-vitals write flow at
server/datastore/mysql/apple_mdm_device_vitals.go:183-186 and subscription-slot
write flow at server/datastore/mysql/apple_mdm_device_vitals.go:266-269 with
atomic MySQL upserts that handle unchanged updates without attempting a
duplicate insert. Add coverage verifying that resubmitting an identical payload
succeeds.
In `@server/fleet/mdm_apple_device_vitals.go`:
- Around line 114-120: Update the persistence mapping for
DevicePropertiesAttestation in the Apple MDM device vitals datastore model so
the [][]byte certificate chain is serialized to and restored from its JSON
column alongside AccessibilitySettings, OrganizationInfo, and MDMOptions. Add
the corresponding database row/schema mapping and a round-trip test covering
non-empty attestation data; otherwise remove the field’s persisted JSON
contract.
In `@server/mdm/apple/commander_test.go`:
- Around line 1177-1178: Update the assertions in the DeviceInformation command
test to compare gotCommand.Command.Queries against an explicit, fixed list
containing all 29 required query names in the expected order, rather than
reusing DeviceInformationQueryKeys. Keep the existing RequestType assertion
unchanged.
---
Nitpick comments:
In `@server/mdm/apple/commander_test.go`:
- Line 1136: Update the test setup around newMockAPNSPushProviderFactory to
capture and assert its returned error. Fail the test immediately when factory
creation fails, before constructing the push service, while preserving the
existing pushFactory usage for successful setup.
In `@server/mdm/apple/commander.go`:
- Around line 463-501: Make the canonical DeviceInformation query list private
so callers cannot reassign or mutate shared state, and update internal
references to use it. If external consumers require access, expose an accessor
that returns a fresh copy in the declared request order; otherwise remove the
export and confirm no public API depends on it.
🪄 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 Plus
Run ID: 24e0a927-82fc-4d8f-8146-01d061fdefd4
📒 Files selected for processing (12)
server/datastore/mysql/apple_mdm_device_vitals.goserver/datastore/mysql/apple_mdm_device_vitals_test.goserver/datastore/mysql/hosts.goserver/datastore/mysql/hosts_test.goserver/datastore/mysql/migrations/tables/20260727191311_CreateHostMDMAppleDeviceVitals.goserver/datastore/mysql/migrations/tables/20260727191311_CreateHostMDMAppleDeviceVitals_test.goserver/datastore/mysql/schema.sqlserver/fleet/datastore.goserver/fleet/mdm_apple_device_vitals.goserver/mdm/apple/commander.goserver/mdm/apple/commander_test.goserver/mock/datastore_mock.go
| if affected, _ := result.RowsAffected(); affected == 0 { | ||
| if _, err := sqlx.NamedExecContext(ctx, tx, deviceVitalsInsertStmt, row); err != nil { | ||
| return ctxerr.Wrap(ctx, err, "insert host mdm apple device vitals") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect whether the MySQL connection enables matched-row reporting and find
# other affected-row-based insert fallbacks.
rg -n --glob '*.go' 'clientFoundRows|mysql\.Config|RowsAffected\(' server
rg -n 'go-sql-driver/mysql' go.mod go.sum 2>/dev/null || trueRepository: fleetdm/fleet
Length of output: 14334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Common MySQL config =="
sed -n '130,170p' server/platform/mysql/common.go
echo
echo "== Device vitals lines 140-205 =="
sed -n '140,205p' server/datastore/mysql/apple_mdm_device_vitals.go
echo
echo "== Subscription lines 240-275 =="
sed-140,275p server/datastore/mysql/apple_mdm_device_vitals.go
echo
echo "== Relevant migration/table definitions =="
rg -n "mdm_apple_device_vitals|host_service_subscription|apples_service_subscription|apples_service_subscription_slots|apples_service_subscription_slots" server/datastore/mysql server/datastore -g '*.go' | head -120Repository: fleetdm/fleet
Length of output: 5434
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Subscription upsert lines 240-275 =="
sed -n '240,275p' server/datastore/mysql/apple_mdm_device_vitals.go
echo
echo "== Vitals/update table/model references =="
rg -n "host_mdm_apple_device_vitals|host_mdm_apple_service_subscriptions|SetOrUpdateHostMDMAppleDeviceVitals" server/datastore mysql server -g '*.go' | head -200
echo
echo "== Nearby service subscription implementations =="
rg -n "host_mdm_apple_service_subscriptions" server/datastore/mysql -g '*.go' -A 10 -B 10 | head -220Repository: fleetdm/fleet
Length of output: 15765
Make both writes atomic upserts. Fleet configures the MySQL client with clientFoundRows=true, so unchanged UPDATEs report zero affected rows and fall through to INSERT, causing duplicate-key failures on repeated identical acks. Use atomic device-vitals and subscription slot upserts instead of update-then-insert fallbacks, and add coverage for resubmitting the same payload.
server/datastore/mysql/apple_mdm_device_vitals.go#L183-L186server/datastore/mysql/apple мdm_device_vitals.go#L266-L269
📍 Affects 1 file
server/datastore/mysql/apple_mdm_device_vitals.go#L183-L186(this comment)server/datastore/mysql/apple_mdm_device_vitals.go#L266-L269
🤖 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_device_vitals.go` around lines 183 - 186,
Replace the update-then-insert fallbacks in the device-vitals write flow at
server/datastore/mysql/apple_mdm_device_vitals.go:183-186 and subscription-slot
write flow at server/datastore/mysql/apple_mdm_device_vitals.go:266-269 with
atomic MySQL upserts that handle unchanged updates without attempting a
duplicate insert. Add coverage verifying that resubmitting an identical payload
succeeds.
There was a problem hiding this comment.
Verified against current code — this doesn't hold. Fleet sets clientFoundRows=true in the DSN (server/platform/mysql/common.go:154), and per MySQL's CLIENT_FOUND_ROWS semantics that makes RowsAffected() report rows matched, not rows changed — the opposite direction from what's described. So resubmitting an identical payload still reports affected=1 and never falls through to the INSERT. Added TestHostMDMAppleDeviceVitals/ResubmitIdenticalPayload (covers both the main row and a subscription slot) in 5aaa3c9 — passes cleanly, confirming there's no duplicate-key failure. Keeping update-then-insert as-is (also matches the pattern used by every other SetOrUpdate* method in this codebase); the residual near-simultaneous-write race is already handled by logging rather than failing the check-in.
| // DevicePropertiesAttestation is the raw DER certificate chain Apple | ||
| // returns (rooted at the Apple Enterprise Attestation Root CA; per manual | ||
| // testing against a physical iPhone, typically 2 certificates — the leaf | ||
| // and the "Apple Enterprise Attestation Sub CA" intermediate). Fleet does | ||
| // not currently parse the chain's custom OIDs into a derived signal. See | ||
| // https://github.com/fleetdm/fleet/issues/49984. | ||
| DevicePropertiesAttestation [][]byte `db:"-"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist DevicePropertiesAttestation consistently with the model contract.
This field is documented as a persisted JSON column, but server/datastore/mysql/apple_mdm_device_vitals.go:65-112 only marshals AccessibilitySettings, OrganizationInfo, and MDMOptions. The attestation chain is therefore dropped on every write. Add the row/schema mapping and a round-trip test, or remove the persistence claim until implemented.
🤖 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/fleet/mdm_apple_device_vitals.go` around lines 114 - 120, Update the
persistence mapping for DevicePropertiesAttestation in the Apple MDM device
vitals datastore model so the [][]byte certificate chain is serialized to and
restored from its JSON column alongside AccessibilitySettings, OrganizationInfo,
and MDMOptions. Add the corresponding database row/schema mapping and a
round-trip test covering non-empty attestation data; otherwise remove the
field’s persisted JSON contract.
There was a problem hiding this comment.
Not accurate — verified against current code. newDeviceVitalsRow (server/datastore/mysql/apple_mdm_device_vitals.go:108) does marshal DevicePropertiesAttestation via the same jsonColumn helper as the other three JSON fields, and the migration's device_properties_attestation json column exists. This was also confirmed end-to-end against a real enrolled iPhone (a 2-cert JSON array round-tripped correctly). Added TestHostMDMAppleDeviceVitalsInsertThenUpdate's new round-trip assertion in 5aaa3c9 to close the coverage gap you correctly flagged, even though the persistence itself was already working.
| require.Equal(t, "DeviceInformation", gotCommand.Command.RequestType) | ||
| require.Equal(t, DeviceInformationQueryKeys, gotCommand.Command.Queries) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the expected query list independent of the implementation.
Comparing the payload to DeviceInformationQueryKeys only proves that the slice was serialized. The test still passes if a required query is removed, reordered, or misspelled. Assert against a fixed expected list to protect the 29 newly added queries.
🤖 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/mdm/apple/commander_test.go` around lines 1177 - 1178, Update the
assertions in the DeviceInformation command test to compare
gotCommand.Command.Queries against an explicit, fixed list containing all 29
required query names in the expected order, rather than reusing
DeviceInformationQueryKeys. Keep the existing RequestType assertion unchanged.
There was a problem hiding this comment.
Valid, fixed in 5aaa3c9 — assertion now compares against an explicit, independent literal list of all 35 keys instead of the package's own var.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 49984-ios-ipados-vitals #50046 +/- ##
===========================================================
- Coverage 68.08% 68.03% -0.06%
===========================================================
Files 3936 3928 -8
Lines 250690 250700 +10
Branches 13277 13361 +84
===========================================================
- Hits 170687 170557 -130
- Misses 64691 64821 +130
- Partials 15312 15322 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- deviceInformationQueryKeys is now unexported (nothing outside the package used it) to prevent external mutation of shared slice state. - Queries assertion now compares against an explicit, independent list instead of the var it's meant to validate. - Added coverage for resubmitting an identical payload (main row and a subscription slot) and a round-trip test for non-empty DevicePropertiesAttestation.
|
Addressing the two nitpicks from the review (no dedicated inline threads to reply to):
|
| } | ||
|
|
||
| _, err = tx.Exec(` | ||
| CREATE TABLE host_mdm_apple_service_subscriptions ( |
There was a problem hiding this comment.
ServiceSubscriptions is another new vital which is returned as an array by Apple, so I modeled it as a separate table (e.g. dual-SIM devices report multiple subscriptions)
…command # Conflicts: # server/datastore/mysql/schema.sql
Move deviceVitalsUpdateStmt/deviceVitalsInsertStmt from package-level consts into SetOrUpdateHostMDMAppleDeviceVitals, matching the pattern already used by replaceHostMDMAppleServiceSubscriptions in this file.
lucasmrod
left a comment
lucasmrod
left a comment
There was a problem hiding this comment.
Looks great. Left some questions.
PS: We should update osquery-perf to push the new fields in DeviceInformation. (If this is on a subsequent PR then just ignore me this.)
| func replaceHostMDMAppleServiceSubscriptions(ctx context.Context, tx sqlx.ExtContext, hostUUID string, subscriptions []fleet.MDMAppleServiceSubscription) error { | ||
| var existingSlots []string | ||
| if err := sqlx.SelectContext(ctx, tx, &existingSlots, | ||
| `SELECT slot FROM host_mdm_apple_service_subscriptions WHERE host_uuid = ?`, hostUUID); err != nil { |
There was a problem hiding this comment.
Nit: We can probably optimize later if really need be. We could get the data of slots and then compare to determine if an UPDATE is really needed at all (maybe data doesn't change that often, but we'll know after we ship and we can iterate if need be.)
There was a problem hiding this comment.
Makes sense to iterate on this 👍 (I'll make a change for the osquery-perf changes in a separate PR, and use that to load test to see how this update, and the other update on the other table affect.)
| accessibility_settings json DEFAULT NULL, | ||
| organization_info json DEFAULT NULL, | ||
| mdm_options json DEFAULT NULL, | ||
| device_properties_attestation json DEFAULT NULL, |
There was a problem hiding this comment.
Are any of these big enough to warrant a load test?
There was a problem hiding this comment.
Probably device_properties_attestation at a big scale, which is the longest/biggest value on the table.
|
|
||
| // deviceInformationQueryKeys are the Apple query keys requested in a | ||
| // DeviceInformation command's <Queries> array, in request order. | ||
| var deviceInformationQueryKeys = []string{ |
There was a problem hiding this comment.
Do we want to ingest these fields for personal BYOD devices?
There was a problem hiding this comment.
Not sure, I'll ask product/apple-at-work group and re-iterate if needed 👍
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Relates to #49984 This is PR 2 of 2 for #49984, stacked on top of PR 1 (#50046) — diff here is scoped to just the ack-parsing/wiring work on top of that PR's storage and command changes. This PR parses the additional `DeviceInformation` ack fields requested by PR 1's command expansion and persists them via `SetOrUpdateHostMDMAppleDeviceVitals`. A persistence failure is logged rather than aborting the MDM check-in, since it's a non-critical write the next refetch will redo. # Checklist for submitter - [ ] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. It will be included in the feature branch. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually Enrolled my iPhone to Fleet (dual-SIM, physical + eSIM): <img width="1458" height="830" alt="Screenshot 2026-07-28 at 10 57 00 AM" src="https://github.com/user-attachments/assets/1735b5e6-ac2d-4dc1-a66b-c8d8063b7c76" /> `host_mdm_apple_device_vitals` — populated as expected: - `battery_level: 0.87`, `cellular_technology: 1`, all `is_*` booleans, `last_cloud_backup_date` — parsed correctly - `device_properties_attestation`: JSON array of **2** base64 DER certs (leaf + "Apple Enterprise Attestation Sub CA" intermediate) — confirms it's a cert chain, not a boolean, per the deviation noted in the parent issue - `mdm_options: {}` — key present, no sub-options applicable → empty object, not `NULL` (expected `omitempty` behavior) - `push_token: NULL` — expected; Apple only returns this for user-channel enrollments, not device-channel - `organization_info` / `accessibility_settings: NULL` — expected, nothing configured/toggled - `model_number`, `modem_firmware_version`, `supplemental_build_version`, `bluetooth_mac`, `wifi_mac`, `eas_device_identifier`, `itunes_store_account_hash` — all populated `host_mdm_apple_service_subscriptions` — **2 rows** for one dual-SIM device, confirming the multi-slot replace logic works on real hardware: - `CTSubscriptionSlotOne` — fully populated (carrier, MCC/MNC, ICCID, phone number, etc.) - `CTSubscriptionSlotTwo` — only `eid`/`imei` populated, everything else `NULL` — an inactive/unprovisioned eSIM slot reporting a sparse row, exactly the shape the tests were built to mirror <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Apple device management now captures and stores additional iPhone and iPad device vitals during device information refreshes. * Supports details such as Wi‑Fi MAC address, lost mode status, operating system information, accessibility settings, organization details, and service subscriptions. * **Bug Fixes** * Missing or unexpected device information no longer interrupts refresh processing. * Failures saving supplementary device vitals no longer prevent other device management actions from completing. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…#50514) <!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Relates to #49984 Restricts device vitals fields added by #49984 (battery level, service subscriptions/phone numbers, accessibility settings, etc.) to non-BYOD iOS/iPadOS enrollments only. Personal (BYOD) hosts continue to get the 9 fields Fleet requested before #49984 (device name, capacity, OS version, product name, lost-mode status, etc.), but not the newer ones, since those can carry PII a personal device's owner may not want their employer to see. Note: Raised during review of #50046, see [this comment](#50046 (review)). ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved support for personally enrolled iOS and iPadOS devices. * Personally enrolled devices now receive a streamlined device-information request. * Vitals for personally enrolled devices display the full list without a “View all” limit or button. * **Bug Fixes** * Corrected Vitals card behavior for BYOD devices while preserving existing behavior for other hosts. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Related issue: Relates to #49984
This is PR 1 of 2 for #49984, split for ease of review and stacked onto a feature branch (
49984-ios-ipados-vitals). This PR adds the MySQL storage (host_mdm_apple_device_vitals,host_mdm_apple_service_subscriptions) for the 29 additional iOS/iPadOS vitals, and expands theDeviceInformationMDM command'sQueriesarray to request the corresponding Apple keys. It does not yet parse or persist any new ack data — that's PR 2 (#50047), stacked on top of this branch.Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
It will be included in the feature branch.
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.Testing
Summary by CodeRabbit
New Features
Bug Fixes
Tests