Set recovery key: add remove state#41934
Merged
mostlikelee merged 5 commits intomainfrom Mar 18, 2026
Merged
Conversation
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
WalkthroughThe PR updates Apple MDM recovery lock password handling: the MySQL datastore query now returns operation_type, a derived password_available flag, and detail. NULL delivery status from the DB is initialized to Pending. HostMDMRecoveryLockPassword gains PasswordAvailable, rawStatus, and operationType fields; SetRawStatus stores raw DB values and PopulateStatus converts raw MDM delivery statuses plus operation type into a public RecoveryLockStatus (verified, pending, failed, removing_enforcement). Service and tests now invoke PopulateStatus before exposing the status. 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
server/fleet/hosts.go (1)
614-627: Consider adding anIsValid()method for consistency.The
DiskEncryptionStatustype (lines 677-690) includes anIsValid()method that validates the status value. For consistency and defensive coding, consider adding a similar method toRecoveryLockStatus.♻️ Optional: Add IsValid() method
func (s RecoveryLockStatus) addrOf() *RecoveryLockStatus { return &s } + +func (s RecoveryLockStatus) IsValid() bool { + switch s { + case RecoveryLockStatusVerified, RecoveryLockStatusVerifying, RecoveryLockStatusPending, RecoveryLockStatusFailed, RecoveryLockStatusRemovingEnforcement: + return true + default: + return false + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/fleet/hosts.go` around lines 614 - 627, Add an IsValid() method on the RecoveryLockStatus type (like DiskEncryptionStatus.IsValid()) that returns a bool by checking the receiver against the allowed constants (RecoveryLockStatusVerified, RecoveryLockStatusVerifying, RecoveryLockStatusPending, RecoveryLockStatusFailed, RecoveryLockStatusRemovingEnforcement); use it wherever you validate RecoveryLockStatus values to ensure consistency and defensive checks (refer to the RecoveryLockStatus type and its constants and the addrOf() helper when adding the method).server/datastore/mysql/apple_mdm_test.go (1)
11019-11036: Add theremove + NULLretry case too.A clear retry is represented as
operation_type='remove'withstatus=NULL, and that is the load-bearing state behind the pending-removal bug. This block only locks down the install-sideNULLtranslation, so the clear retry path can still regress unnoticed. Please add the mirror assertion that it normalizes toRecoveryLockStatusRemovingEnforcement.➕ Suggested test case
+ t.Run("returns removing_enforcement when remove status is NULL (retry state)", func(t *testing.T) { + host := test.NewHost(t, ds, "remove-null-status-host", "1.2.6.9", "removenullkey", "removenulluuid", time.Now()) + pw := apple_mdm.GenerateRecoveryLockPassword() + err := ds.SetHostsRecoveryLockPasswords(ctx, []fleet.HostRecoveryLockPasswordPayload{{HostUUID: host.UUID, Password: pw}}) + require.NoError(t, err) + err = ds.SetRecoveryLockVerified(ctx, host.UUID) + require.NoError(t, err) + _, err = ds.writer(ctx).ExecContext(ctx, + `UPDATE host_recovery_key_passwords SET operation_type = ?, status = ? WHERE host_uuid = ?`, + fleet.MDMOperationTypeRemove, fleet.MDMDeliveryPending, host.UUID, + ) + require.NoError(t, err) + err = ds.ClearRecoveryLockPendingStatus(ctx, []string{host.UUID}) + require.NoError(t, err) + + status, err := ds.GetHostRecoveryLockPasswordStatus(ctx, host.UUID) + require.NoError(t, err) + require.NotNil(t, status) + status.PopulateStatus() + require.NotNil(t, status.Status) + assert.Equal(t, fleet.RecoveryLockStatusRemovingEnforcement, *status.Status) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/apple_mdm_test.go` around lines 11019 - 11036, Add a mirrored test case that covers the "remove + NULL" retry: create a host (test.NewHost), set its recovery-lock operation to 'remove' (use the same datastore APIs that set the operation_type on the host or recovery lock record), clear the status to NULL (ds.ClearRecoveryLockPendingStatus), then call ds.GetHostRecoveryLockPasswordStatus and status.PopulateStatus and assert that status.Status equals fleet.RecoveryLockStatusRemovingEnforcement and status.Detail is empty; place this alongside the existing "returns enforcing status when status column is NULL (retry state)" test so both install and remove NULL-retry translations are validated.server/service/hosts_test.go (1)
811-831: Add a regression case for pending CLEAR → removing status mapping.This update validates
failed/installtranslation, but it doesn’t cover the core scenario from this PR’s objective: pendingREMOVEshould map toRecoveryLockStatusRemovingEnforcement. Please add a subtest for that translation path.Suggested test addition
t.Run("recovery lock password status populates for macOS", func(t *testing.T) { @@ assert.Equal(t, "SetRecoveryLock command failed", hostDetail.MDM.OSSettings.RecoveryLockPassword.Detail) }) + +t.Run("recovery lock password pending remove maps to removing enforcement", func(t *testing.T) { + pendingStatus := fleet.MDMDeliveryPending + ds.GetHostRecoveryLockPasswordStatusFunc = func(ctx context.Context, hostUUID string) (*fleet.HostMDMRecoveryLockPassword, error) { + result := &fleet.HostMDMRecoveryLockPassword{} + result.SetRawStatus(&pendingStatus, fleet.MDMOperationTypeRemove) + return result, nil + } + + ctx := license.NewContext(t.Context(), &fleet.LicenseInfo{Tier: fleet.TierPremium}) + hostDetail, err := svc.getHostDetails(test.UserContext(ctx, test.UserAdmin), &fleet.Host{ID: 42, Platform: "darwin", UUID: "test-uuid"}, fleet.HostDetailOptions{ + IncludeCVEScores: false, + IncludePolicies: false, + }) + require.NoError(t, err) + require.NotNil(t, hostDetail.MDM.OSSettings.RecoveryLockPassword.Status) + assert.Equal(t, fleet.RecoveryLockStatusRemovingEnforcement, *hostDetail.MDM.OSSettings.RecoveryLockPassword.Status) +})🤖 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 811 - 831, Add a subtest alongside the existing "recovery lock password status populates for macOS" case that exercises the pending REMOVE translation: have ds.GetHostRecoveryLockPasswordStatusFunc return a fleet.HostMDMRecoveryLockPassword with a pending status via SetRawStatus(&pendingStatus, fleet.MDMOperationTypeRemove), call svc.getHostDetails the same way, and assert that hostDetail.MDM.OSSettings.RecoveryLockPassword.Status is non-nil and equals fleet.RecoveryLockStatusRemovingEnforcement (and that Detail is preserved if set); reuse the same test setup and assertions pattern (ds.GetHostRecoveryLockPasswordStatusFuncInvoked, require.NoError, require.NotNil) as in the existing test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/datastore/mysql/apple_mdm_test.go`:
- Around line 11019-11036: Add a mirrored test case that covers the "remove +
NULL" retry: create a host (test.NewHost), set its recovery-lock operation to
'remove' (use the same datastore APIs that set the operation_type on the host or
recovery lock record), clear the status to NULL
(ds.ClearRecoveryLockPendingStatus), then call
ds.GetHostRecoveryLockPasswordStatus and status.PopulateStatus and assert that
status.Status equals fleet.RecoveryLockStatusRemovingEnforcement and
status.Detail is empty; place this alongside the existing "returns enforcing
status when status column is NULL (retry state)" test so both install and remove
NULL-retry translations are validated.
In `@server/fleet/hosts.go`:
- Around line 614-627: Add an IsValid() method on the RecoveryLockStatus type
(like DiskEncryptionStatus.IsValid()) that returns a bool by checking the
receiver against the allowed constants (RecoveryLockStatusVerified,
RecoveryLockStatusVerifying, RecoveryLockStatusPending,
RecoveryLockStatusFailed, RecoveryLockStatusRemovingEnforcement); use it
wherever you validate RecoveryLockStatus values to ensure consistency and
defensive checks (refer to the RecoveryLockStatus type and its constants and the
addrOf() helper when adding the method).
In `@server/service/hosts_test.go`:
- Around line 811-831: Add a subtest alongside the existing "recovery lock
password status populates for macOS" case that exercises the pending REMOVE
translation: have ds.GetHostRecoveryLockPasswordStatusFunc return a
fleet.HostMDMRecoveryLockPassword with a pending status via
SetRawStatus(&pendingStatus, fleet.MDMOperationTypeRemove), call
svc.getHostDetails the same way, and assert that
hostDetail.MDM.OSSettings.RecoveryLockPassword.Status is non-nil and equals
fleet.RecoveryLockStatusRemovingEnforcement (and that Detail is preserved if
set); reuse the same test setup and assertions pattern
(ds.GetHostRecoveryLockPasswordStatusFuncInvoked, require.NoError,
require.NotNil) as in the existing test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e50a1b3c-d997-4e08-a8b1-bb9872b37b89
📒 Files selected for processing (5)
server/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_test.goserver/fleet/hosts.goserver/service/hosts.goserver/service/hosts_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #41934 +/- ##
==========================================
- Coverage 66.41% 66.40% -0.01%
==========================================
Files 2509 2510 +1
Lines 200788 201184 +396
Branches 8942 8942
==========================================
+ Hits 133351 133594 +243
- Misses 55378 55518 +140
- Partials 12059 12072 +13
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:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refines macOS Recovery Lock password status reporting so the host OS Settings UI can differentiate between setting vs. clearing workflows (notably adding a “removing enforcement” state) and exposes whether a password is available.
Changes:
- Add a dedicated
RecoveryLockStatusenum and translation logic (PopulateStatus) from raw MDM delivery status + operation type. - Update host details service to populate/translate recovery lock status before returning API payloads.
- Extend datastore query/tests to include
operation_typeand apassword_availableindicator, plus new test cases for pending removal.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/fleet/hosts.go | Introduces RecoveryLockStatus and translation from raw MDM status/op type. |
| server/service/hosts.go | Calls PopulateStatus() before returning recovery lock status in host details. |
| server/service/hosts_test.go | Updates host details test to use raw status translation and assert new status values. |
| server/datastore/mysql/apple_mdm.go | Extends recovery lock status query to include operation type and password availability; returns raw status for later translation. |
| server/datastore/mysql/apple_mdm_test.go | Updates and expands datastore tests for translated states, including pending removal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/fleet/hosts.go`:
- Around line 629-651: PopulateStatus on HostMDMRecoveryLockPassword can leave a
stale r.Status when r.rawStatus is nil; before any early return or switch
mapping, reset r.Status to nil (clear previous translated value) so reused
struct instances don't leak old state. Update
HostMDMRecoveryLockPassword.PopulateStatus to set r.Status = nil at the top (or
immediately before the early return) and then proceed with the existing
operationType/rawStatus mapping logic.
- Around line 614-622: Update the RecoveryLockPasswordStatus in
frontend/interfaces/host.ts to match the backend RecoveryLockStatus and the
already-corrected frontend/interfaces/mdm.ts: remove the incorrect "verifying"
value and add "removing_enforcement", so the union type contains "verified" |
"pending" | "removing_enforcement" | "failed"; ensure
IMdmMacOsSettings.recovery_lock_password in host.ts uses this updated
RecoveryLockPasswordStatus type so it aligns with the backend's
RecoveryLockStatus and mdm.ts definition.
🪄 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: edc1c3c5-f1f3-4c5b-bfbb-2c0b4e87af0c
📒 Files selected for processing (2)
server/datastore/mysql/apple_mdm_test.goserver/fleet/hosts.go
getvictor
approved these changes
Mar 18, 2026
Member
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Looks good. Please review AI agent comment.
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 #41930
Summary by CodeRabbit
New Features
Bug Fixes