Add recovery lock password status#41322
Merged
mostlikelee merged 97 commits intomainfrom Mar 17, 2026
Merged
Conversation
For the following quick win: - #40354
The sqlJoinRecoveryLockStatus() function was missing a `deleted = 0` filter, which could cause soft-deleted recovery lock records to affect the OS settings summary counts incorrectly. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Adds TestHostDetailsRecoveryLockPasswordStatus with two subtests: - Verifies recovery lock password status populates correctly for macOS hosts - Verifies the status function is not called for non-macOS platforms Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
WalkthroughThis pull request adds recovery lock password status retrieval for MDM-managed macOS hosts. It introduces a new Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/service/hosts_test.go (1)
813-817: Tighten the test by asserting the host UUID and failing fast on unexpected non-macOS calls.Right now invocation is tracked, but asserting the UUID improves correctness, and a fail-fast callback gives clearer diagnostics if the non-macOS guard regresses.
Suggested test hardening
ds.GetHostRecoveryLockPasswordStatusFunc = func(ctx context.Context, hostUUID string) (*fleet.HostMDMRecoveryLockPassword, error) { + require.Equal(t, "test-uuid", hostUUID) return &fleet.HostMDMRecoveryLockPassword{ Status: &failedStatus, Detail: "SetRecoveryLock command failed", }, nil } @@ ds.GetHostRecoveryLockPasswordStatusFuncInvoked = false ds.GetHostRecoveryLockPasswordStatusFunc = func(ctx context.Context, hostUUID string) (*fleet.HostMDMRecoveryLockPassword, error) { - return nil, nil + t.Fatalf("GetHostRecoveryLockPasswordStatus should not be called for non-macOS hosts (uuid=%s)", hostUUID) + return nil, nil }Also applies to: 835-837
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/hosts_test.go` around lines 813 - 817, Update the test doubles for GetHostRecoveryLockPasswordStatusFunc to assert the received host UUID equals the expected test host UUID and fail fast on any unexpected invocation: inside the stub for ds.GetHostRecoveryLockPasswordStatusFunc (and the other occurrence around lines 835-837), compare the hostUUID argument to the expected UUID and call t.Fatalf (or t.Fatalf equivalent) if it differs or if the stub is called in a non-macOS/otherwise-unexpected path so the test fails immediately and gives a clear diagnostic instead of silently recording the call.
🤖 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/apple_mdm.go`:
- Around line 3656-3669: The rl_pending aggregate currently only marks rows
where status == pending, causing retryable recovery-lock rows to be excluded;
update the SELECT that computes rl_pending (from host_recovery_key_passwords) to
treat retryable statuses as pending as well (e.g., change the MAX(IF(status =
pending, 1, 0)) expression to include retryable via an IN or OR check) so
ClearRecoveryLockPendingStatus's retry window still shows rl_pending; modify the
condition that builds rl_pending in the SQL fragment referencing
host_recovery_key_passwords and rl_pending accordingly.
- Around line 7345-7363: GetHostRecoveryLockPasswordStatus reads a nullable
status so when ClearRecoveryLockPendingStatus sets status to NULL the API
returns Status: nil and hides an outstanding retry; update
GetHostRecoveryLockPasswordStatus to coalesce a nil row.Status into a concrete
"retry/pending" MDMDeliveryStatus before returning (e.g., if row.Status == nil
set the returned Status to the existing retry/pending constant on
fleet.MDMDeliveryStatus, or add such a constant if it doesn't exist) so the host
details always indicate an outstanding recovery-lock retry; refer to the
GetHostRecoveryLockPasswordStatus function and the
ClearRecoveryLockPendingStatus behavior when making the change.
---
Nitpick comments:
In `@server/service/hosts_test.go`:
- Around line 813-817: Update the test doubles for
GetHostRecoveryLockPasswordStatusFunc to assert the received host UUID equals
the expected test host UUID and fail fast on any unexpected invocation: inside
the stub for ds.GetHostRecoveryLockPasswordStatusFunc (and the other occurrence
around lines 835-837), compare the hostUUID argument to the expected UUID and
call t.Fatalf (or t.Fatalf equivalent) if it differs or if the stub is called in
a non-macOS/otherwise-unexpected path so the test fails immediately and gives a
clear diagnostic instead of silently recording the call.
🪄 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: bbb56a11-5fc3-4001-b116-c9656654bf8a
📒 Files selected for processing (11)
cmd/fleetctl/fleetctl/testing_utils/testing_utils.goserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_test.goserver/datastore/mysql/hosts.goserver/datastore/mysql/labels.goserver/fleet/datastore.goserver/fleet/hosts.goserver/mock/datastore_mock.goserver/service/apple_mdm_test.goserver/service/hosts.goserver/service/hosts_test.go
Match the pattern used by sqlJoinMDMAppleProfilesStatus() where NULL status is treated as pending. This ensures hosts in the retry state (status cleared to NULL after failed enqueue) are correctly counted as pending in the OS settings summary. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When ClearRecoveryLockPendingStatus sets status to NULL (retry state), the API now returns "pending" instead of null. This ensures the frontend displays the recovery lock row in the OS settings table for hosts in retry state, matching the behavior of the summary counts which already treat NULL as pending. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds Recovery Lock password delivery status to host details for macOS devices (MDM-enabled), and incorporates that status into the existing OS Settings/macOS Settings status aggregation used by filtering and summaries.
Changes:
- Extend
HostMDMOSSettingswith arecovery_lock_passwordstatus+detail payload and populate it ingetHostDetailsfor macOS hosts. - Add
GetHostRecoveryLockPasswordStatusto the datastore interface + MySQL implementation, and include recovery lock status in the SQL used to derive Apple OS Settings/macOS Settings status. - Update mocks and add service/datastore tests covering the new status behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/service/hosts.go | Populate recovery lock password status in host details for macOS hosts. |
| server/fleet/hosts.go | Add RecoveryLockPassword to HostMDMOSSettings and define the status payload struct. |
| server/fleet/datastore.go | Extend datastore interface with GetHostRecoveryLockPasswordStatus. |
| server/datastore/mysql/apple_mdm.go | Implement status fetch + incorporate recovery lock into Apple status SQL and joins. |
| server/datastore/mysql/hosts.go | Add recovery lock join when OS Settings/macOS Settings filters are active. |
| server/datastore/mysql/labels.go | Add recovery lock join when OS Settings/macOS Settings filters are active for label queries. |
| server/mock/datastore_mock.go | Add mock plumbing for GetHostRecoveryLockPasswordStatus. |
| server/service/hosts_test.go | Add/adjust service tests and mocks for host details recovery lock status behavior. |
| server/service/apple_mdm_test.go | Add required mock stub for new datastore method in existing host details test. |
| server/datastore/mysql/apple_mdm_test.go | Add MySQL datastore test coverage for GetHostRecoveryLockPasswordStatus. |
| cmd/fleetctl/fleetctl/testing_utils/testing_utils.go | Stub new datastore method in fleetctl server test helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+3657
to
+3670
| -- recovery lock statuses grouped by host uuid | ||
| -- NULL status is treated as pending (retry state after failed enqueue) | ||
| SELECT | ||
| host_uuid, | ||
| MAX(IF(status IS NULL OR status = ` + pending + `, 1, 0)) AS rl_pending, | ||
| MAX(IF(status = ` + failed + `, 1, 0)) AS rl_failed, | ||
| MAX(IF(status = ` + verifying + `, 1, 0)) AS rl_verifying, | ||
| MAX(IF(status = ` + verified + `, 1, 0)) AS rl_verified | ||
| FROM | ||
| host_recovery_key_passwords | ||
| WHERE | ||
| deleted = 0 | ||
| GROUP BY | ||
| host_uuid) hrlp ON h.uuid = hrlp.host_uuid |
getvictor
approved these changes
Mar 17, 2026
| host.MDM.PopulateOSSettingsAndMacOSSettings(profs, mobileconfig.FleetFileVaultPayloadIdentifier) | ||
|
|
||
| // populate recovery lock password status for macOS hosts | ||
| if host.Platform == "darwin" { |
Member
There was a problem hiding this comment.
Nit. Isn't there a constant for this?
This was referenced Mar 17, 2026
mostlikelee
added a commit
that referenced
this pull request
Mar 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #40657
Summary by CodeRabbit