-
Notifications
You must be signed in to change notification settings - Fork 747
Add lost mode behaviour for iOS/iPadOS #33805
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #33805 +/- ##
==========================================
+ Coverage 63.81% 64.23% +0.42%
==========================================
Files 1876 2058 +182
Lines 185549 207072 +21523
Branches 6810 6810
==========================================
+ Hits 118399 133012 +14613
- Misses 57484 63617 +6133
- Partials 9666 10443 +777
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:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughImplements Lost Mode-based lock/unlock for iOS/iPadOS alongside existing macOS, updates datastore and interfaces, adds command issuer methods, adjusts status handling, and expands tests. Renames cleanup method to Apple-generic form, introduces device unlock enqueueing, and integrates per-platform lock/unlock control flow in services. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant API as Service
participant DS as Datastore
participant Cmd as MDMAppleCommander
participant Push as APNs Push
participant Host as iOS/iPadOS Device
rect rgb(244,248,253)
note over API,Host: Lock (iOS/iPadOS) via Lost Mode
API->>DS: Create lock activity / status
API->>Cmd: EnableLostMode(host, cmdUUID, orgName)
Cmd->>Cmd: Build EnableLostMode plist
Cmd->>DS: EnqueueDeviceLockCommand(cmd, pin="")
Cmd->>Push: Send push
Push-->>Host: MDM notify
Host-->>DS: Command result (EnableLostMode)
DS-->>API: Update lock_ref/status
end
sequenceDiagram
autonumber
participant API as Service
participant DS as Datastore
participant Cmd as MDMAppleCommander
participant Push as APNs Push
participant Host as iOS/iPadOS Device
rect rgb(244,248,253)
note over API,Host: Unlock (iOS/iPadOS) via Disable Lost Mode
API->>DS: Create unlock activity / status
API->>Cmd: DisableLostMode(host, cmdUUID)
Cmd->>Cmd: Build DisableLostMode plist
Cmd->>DS: EnqueueDeviceUnlockCommand(cmd)
Cmd->>Push: Send push
Push-->>Host: MDM notify
Host-->>DS: Command result (DisableLostMode)
DS-->>API: Update unlock_ref/status
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (1)
server/datastore/mysql/nanomdm_storage_test.go (1)
94-145: Consider expanding test coverage for unlock scenarios.The test successfully validates the happy path for unlock command enqueueing. However, it lacks coverage for important edge cases:
- Conflict handling: What happens if unlock is called twice on the same device?
- State prerequisites: Can unlock be called when no lock exists?
- Lock-unlock interaction: Does unlock properly clear the lock state created by
EnqueueDeviceLockCommand?Similar comprehensive testing exists for lock operations (see
testGetPendingLockCommandlines 147-235 andtestEnqueueDeviceLockCommandRaceConditionlines 237-370).Consider adding test cases that verify:
// Example additional test structure func testEnqueueDeviceUnlockCommandEdgeCases(t *testing.T, ds *Datastore) { // Test 1: Unlock without prior lock // Test 2: Multiple unlock attempts (conflict handling) // Test 3: Lock followed by unlock clears state // Test 4: Race condition handling for concurrent unlocks }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
changes/30889-enable-lost-mode-ios-ipados(1 hunks)ee/server/service/hosts.go(3 hunks)server/datastore/mysql/apple_mdm.go(1 hunks)server/datastore/mysql/apple_mdm_test.go(3 hunks)server/datastore/mysql/nanomdm_storage.go(2 hunks)server/datastore/mysql/nanomdm_storage_test.go(2 hunks)server/datastore/mysql/scripts.go(3 hunks)server/fleet/apple_mdm.go(1 hunks)server/fleet/datastore.go(2 hunks)server/fleet/scripts.go(2 hunks)server/mdm/apple/commander.go(1 hunks)server/mdm/apple/commander_test.go(2 hunks)server/mock/datastore_mock.go(9 hunks)server/mock/mdm/datastore_mdm_mock.go(3 hunks)server/service/apple_mdm.go(2 hunks)server/service/integration_mdm_commands_test.go(1 hunks)server/service/integration_mdm_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
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 (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/mock/mdm/datastore_mdm_mock.goserver/fleet/datastore.goserver/fleet/apple_mdm.goserver/fleet/scripts.goserver/datastore/mysql/scripts.goserver/datastore/mysql/nanomdm_storage_test.goserver/datastore/mysql/apple_mdm_test.goserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/nanomdm_storage.goserver/service/apple_mdm.goserver/service/integration_mdm_test.goserver/mdm/apple/commander_test.goserver/service/integration_mdm_commands_test.goserver/mock/datastore_mock.goserver/mdm/apple/commander.goee/server/service/hosts.go
🔇 Additional comments (6)
server/datastore/mysql/nanomdm_storage.go (1)
245-264: Good parity for lost-mode unlock bookkeepingMirrors the existing lock/wipe flows nicely—enqueuing under retry and clearing
unlock_pinwhile updatingunlock_refkeepshost_mdm_actionsin a consistent state for the new lost-mode path.server/fleet/scripts.go (2)
463-465: LGTM! Fields added to track iOS/iPadOS unlock.The new
UnlockMDMCommandandUnlockMDMCommandResultfields enable tracking of Lost Mode disable operations for iOS/iPadOS devices, aligning with the broader PR objective to support these platforms.
538-551: iOS/iPadOS unlock logic is consistent. Datastore populatesUnlockMDMCommand/UnlockMDMCommandResultthe same way as lock, andIsPendingUnlock()mirrorsIsPendingLock(). The service’sDisableLostModeenqueues the MDM command correctly. No changes required.server/mdm/apple/commander_test.go (2)
94-95: Formatting adjustment for updated signature.The
GetAllMDMConfigAssetsByNameFuncmock now includes an additionalsqlx.QueryerContextparameter, aligning with the updated method signature in the datastore interface. This is a necessary change to support the new unlock command enqueueing logic.
168-200: LGTM! Test coverage for Lost Mode enable/disable.The new test blocks validate:
- EnableLostMode: Confirms the command RequestType is "EnableLostMode", the orgName is included in the payload, and the pin is empty.
- DisableLostMode: Confirms the command RequestType is "DisableLostMode" via
EnqueueDeviceUnlockCommandFunc.Both tests properly assert invocation flags and reset them after validation, ensuring correct behavior.
Consider adding edge-case tests for:
- EnableLostMode with empty orgName: Verify handling of missing or empty organization names.
- Concurrent EnableLostMode/DisableLostMode calls: Similar to
TestMDMAppleCommanderConcurrentDeviceLock, validate behavior under concurrent requests.- Push notification failures during Lost Mode operations: Extend
TestMDMAppleCommanderDeviceLockPushNotificationFailurepattern to cover EnableLostMode/DisableLostMode.Would you like me to generate these additional test cases or open a new issue to track them?
server/fleet/apple_mdm.go (1)
25-26: LGTM. The new EnableLostMode and DisableLostMode methods are implemented in MDMAppleCommander, covered by commander tests, and invoked correctly in the hosts service.
b7dc255 to
140b52b
Compare
JordanMontgomery
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Glad we can ship this this sprint!
Related issue: Resolves #33416
It's been decided to ship the feature and in the guide mention the apple bug, that we are currently tracking.
Slack 🧵
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Added/updated automated tests
QA'd all new/changed functionality manually
Summary by CodeRabbit