iOS/iPadOS device vitals: ack parsing + wiring (2/2) - #50047
Conversation
|
@coderabbitai full review |
✅ Action performedFull review finished. |
WalkthroughApple MDM refetch handling now parses device-vitals data from Possibly related issues
🚥 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/service/apple_mdm_test.go (1)
7477-7493: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
require.False(t, ptr.ValOrZero(...))passes when the field isnil.For the keys the payload sets to
<false/>(AwaitingConfiguration,DiagnosticSubmissionEnabled,IsDoNotDisturbInEffect,IsMDMLostModeEnabled,IsNetworkTethered,PersonalHotspotEnabled,AccessibilitySettings.ZoomEnabled), a regression that drops the value entirely would still satisfy these assertions. Assert non-nil first so "parsed false" is distinguished from "not parsed" — that's precisely the failure mode this test exists to catch.💚 Example for one field (apply to the other false-valued bools)
- require.False(t, ptr.ValOrZero(gotVitals.AwaitingConfiguration)) + require.NotNil(t, gotVitals.AwaitingConfiguration) + require.False(t, *gotVitals.AwaitingConfiguration)🤖 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/service/apple_mdm_test.go` around lines 7477 - 7493, Strengthen the false-valued boolean assertions in the vitals parsing test by first requiring each payload-set field to be non-nil, then asserting its value is false. Apply this to AwaitingConfiguration, DiagnosticSubmissionEnabled, IsDoNotDisturbInEffect, IsMDMLostModeEnabled, IsNetworkTethered, PersonalHotspotEnabled, and AccessibilitySettings.ZoomEnabled, preserving the existing true-valued assertions.
🤖 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.
Nitpick comments:
In `@server/service/apple_mdm_test.go`:
- Around line 7477-7493: Strengthen the false-valued boolean assertions in the
vitals parsing test by first requiring each payload-set field to be non-nil,
then asserting its value is false. Apply this to AwaitingConfiguration,
DiagnosticSubmissionEnabled, IsDoNotDisturbInEffect, IsMDMLostModeEnabled,
IsNetworkTethered, PersonalHotspotEnabled, and
AccessibilitySettings.ZoomEnabled, preserving the existing true-valued
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ade435f-e180-4804-891c-d2db64001c4c
📒 Files selected for processing (3)
server/service/apple_mdm.goserver/service/apple_mdm_device_vitals.goserver/service/apple_mdm_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 49984-ios-ipados-vitals #50047 +/- ##
==========================================================
Coverage ? 68.11%
==========================================================
Files ? 3939
Lines ? 251064
Branches ? 13277
==========================================================
Hits ? 171009
Misses ? 64721
Partials ? 15334
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:
|
…50046) <!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **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 the `DeviceInformation` MDM command's `Queries` array 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 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. - [x] 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. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually (see results on PR2) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for collecting and storing Apple MDM device vitals and cellular service subscription details. * Expanded Apple device information requests to include the full set of supported query fields. * Added database support for retaining device vitals and subscription data. * **Bug Fixes** * Host deletion now also removes associated Apple MDM vitals and subscription records. * **Tests** * Added coverage for vitals updates, nullable values, subscription synchronization, database constraints, and device information requests. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Parses the additional DeviceInformation ack fields requested by the command expansion added in the prior PR and persists them via SetOrUpdateHostMDMAppleDeviceVitals. A persistence failure is logged rather than aborting the check-in, since it's a non-critical write the next refetch will redo. Part of #49984.
140876b to
2d16ab0
Compare
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
DeviceInformationack fields requested by PR 1's command expansion and persists them viaSetOrUpdateHostMDMAppleDeviceVitals. 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/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
It will be included in the feature branch.
Testing
Added/updated automated tests
QA'd all new/changed functionality manually
Enrolled my iPhone to Fleet (dual-SIM, physical + eSIM):
host_mdm_apple_device_vitals— populated as expected:battery_level: 0.87,cellular_technology: 1, allis_*booleans,last_cloud_backup_date— parsed correctlydevice_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 issuemdm_options: {}— key present, no sub-options applicable → empty object, notNULL(expectedomitemptybehavior)push_token: NULL— expected; Apple only returns this for user-channel enrollments, not device-channelorganization_info/accessibility_settings: NULL— expected, nothing configured/toggledmodel_number,modem_firmware_version,supplemental_build_version,bluetooth_mac,wifi_mac,eas_device_identifier,itunes_store_account_hash— all populatedhost_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— onlyeid/imeipopulated, everything elseNULL— an inactive/unprovisioned eSIM slot reporting a sparse row, exactly the shape the tests were built to mirrorSummary by CodeRabbit
New Features
Bug Fixes