Clear Recovery Lock Password

[mostlikelee]

Related issue: Resolves #41282

Summary by CodeRabbit

• New Features

  • Added capability to clear recovery locks on Apple hosts and a coordinated workflow to claim, enqueue, and process hosts needing clearance.
  • Recovery lock operations now distinguish set vs. clear flows and handle success/failure paths distinctly (including removing stored credentials on successful clear).

• Tests

  • Added comprehensive tests covering claim/clear flows, retries, soft-delete semantics, and end-to-end handling.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

server/mdm/apple/apple_mdm.go (1)

1716-1728: ⚠️ Potential issue | 🟠 Major

Don't swallow failed pending-status rollbacks.

If ClearRecoveryLockPendingStatus fails here, the host can stay stuck in pending after an enqueue failure, so later cron runs may never retry it. Logging clearErr is not enough; return it alongside the enqueue error so this state is surfaced and alerted on.

💡 Suggested change

-		if clearErr := ds.ClearRecoveryLockPendingStatus(ctx, hostUUIDs); clearErr != nil {
-			logger.ErrorContext(ctx, "failed to clear recovery lock pending status after enqueue failure",
-				"host_count", len(hostUUIDs),
-				"error", clearErr,
-			)
-		}
-		return ctxerr.Wrap(ctx, err, "enqueue SetRecoveryLock commands")
+		retErr := ctxerr.Wrap(ctx, err, "enqueue SetRecoveryLock commands")
+		if clearErr := ds.ClearRecoveryLockPendingStatus(ctx, hostUUIDs); clearErr != nil {
+			logger.ErrorContext(ctx, "failed to clear recovery lock pending status after enqueue failure",
+				"host_count", len(hostUUIDs),
+				"error", clearErr,
+			)
+			return multierror.Append(
+				retErr,
+				ctxerr.Wrap(ctx, clearErr, "clear recovery lock pending status after enqueue failure"),
+			).ErrorOrNil()
+		}
+		return retErr

Apply the same pattern in the CLEAR branch.

Also applies to: 1772-1783

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/mdm/apple/apple_mdm.go` around lines 1716 - 1728, The rollback error
from ClearRecoveryLockPendingStatus is only being logged and not returned, which
can leave hosts stuck in pending; update the failure path after the enqueue
SetRecoveryLock commands so that if ClearRecoveryLockPendingStatus returns an
error you return it (wrapped) together with the original enqueue error instead
of swallowing it — e.g., after calling ds.ClearRecoveryLockPendingStatus(ctx,
hostUUIDs) check clearErr and return ctxerr.Wrap(ctx, multiErr/combinedErr,
"clear recovery lock pending status after enqueue failure") or wrap clearErr
with the original enqueue error using ctxerr.Wrap/merge so both errors are
surfaced; apply the same change to the CLEAR branch (the equivalent code around
the ClearRecoveryLockPendingStatus call at the other location).

🤖 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 7480-7502: GetRecoveryLockOperationType and
DeleteHostRecoveryLockPassword operate on the mutable host row
(host_recovery_key_passwords WHERE host_uuid = ?), allowing races where a later
SetHostsRecoveryLockPasswords call can overwrite the row; persist and use an
immutable operation identifier and scope queries/updates to it. Add an
operation_uuid (or operation_token) column to host_recovery_key_passwords (with
a migration), have SetHostsRecoveryLockPasswords insert the new operation_uuid
when enqueuing a command, and change GetRecoveryLockOperationType and
DeleteHostRecoveryLockPassword signatures to accept that operation UUID (or look
up the current operation UUID at enqueue time) and use WHERE host_uuid = ? AND
operation_uuid = ? AND deleted = 0 for SELECT/UPDATE to ensure you're operating
on the exact enqueued recovery-lock operation rather than the mutable host row.
Ensure callers that trigger ClearRecoveryLock pass the matching operation UUID
and update tests to cover the race case.

---

Outside diff comments:
In `@server/mdm/apple/apple_mdm.go`:
- Around line 1716-1728: The rollback error from ClearRecoveryLockPendingStatus
is only being logged and not returned, which can leave hosts stuck in pending;
update the failure path after the enqueue SetRecoveryLock commands so that if
ClearRecoveryLockPendingStatus returns an error you return it (wrapped) together
with the original enqueue error instead of swallowing it — e.g., after calling
ds.ClearRecoveryLockPendingStatus(ctx, hostUUIDs) check clearErr and return
ctxerr.Wrap(ctx, multiErr/combinedErr, "clear recovery lock pending status after
enqueue failure") or wrap clearErr with the original enqueue error using
ctxerr.Wrap/merge so both errors are surfaced; apply the same change to the
CLEAR branch (the equivalent code around the ClearRecoveryLockPendingStatus call
at the other location).

🪄 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: 7c1032dc-d3a1-45ee-9d07-cf4e290665af

📥 Commits

Reviewing files that changed from the base of the PR and between 45897fd and 6ebd9ff.

📒 Files selected for processing (3)

• server/datastore/mysql/apple_mdm.go
• server/datastore/mysql/apple_mdm_test.go
• server/mdm/apple/apple_mdm.go

[mostlikelee]

@getvictor let me know if you have bandwidth to review this one

[getvictor]

Did a quick review. Looks good overall. Added some comments/questions that can be addressed later.

[getvictor]

This is confusing. These states suggest success when they're meant to mean failure. Why doesn't remove with status=NULL work here?

[mostlikelee]

this is a misleading method name, i'll rename it. We're restoring the row back to the install/verified state because the clear password command failed. there are no retries involved here.

[getvictor]

Why are we setting status to verified here?

[mostlikelee]

this isn't customer facing, more for troubleshooting. Similar to how we mark rows as verified when the set command is ack'd, we're setting this row as verified when the clear command is ack'd.

[getvictor]

There is no limit how long we can keep retrying. Probably fine, but maybe add a comment.

[Copilot]

Pull request overview

This PR adds support for clearing Recovery Lock passwords on Apple hosts, including datastore workflows to claim hosts needing a clear, enqueue ClearRecoveryLock commands, and handle result processing distinct from the set/verify flow.

Changes:

• Added a ClearRecoveryLock command pathway (claim → enqueue → result handling) alongside existing SetRecoveryLock behavior.
• Introduced datastore APIs and MySQL implementations to support host claiming, retries, restore-on-reenable, and password record deletion.
• Expanded unit/integration tests to cover clear flows, error classification, retries, and soft-delete semantics.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
server/service/apple_mdm_cmd_results_test.go	Adds result-handler tests for CLEAR ack, terminal mismatch errors, and transient retry behavior.
server/service/apple_mdm.go	Updates SetRecoveryLock results handler to branch on operation type (SET vs CLEAR) and adds retry/reset logic for clear failures.
server/mock/datastore_mock.go	Extends datastore mock to support new recovery-lock clear APIs used by services/tests.
server/mdm/apple/util.go	Adds helper to detect password-mismatch error signatures for clear flow.
server/mdm/apple/util_test.go	Adds unit tests for password-mismatch error detection.
server/mdm/apple/commander.go	Adds ClearRecoveryLock command payload generation/enqueueing.
server/mdm/apple/apple_mdm.go	Extends cron command sender to restore-on-reenable, send SET commands, and send CLEAR commands.
server/fleet/datastore.go	Adds datastore interface methods for clear workflow, restore, op-type lookup, and retry reset.
server/datastore/mysql/apple_mdm.go	Implements MySQL functions for restore, claim-for-clear, soft delete, op-type lookup, and retry reset.
server/datastore/mysql/apple_mdm_test.go	Adds MySQL integration tests for new clear/restore/retry behaviors and op-type lookup.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[getvictor]

Changes look good since my last reivew, but I did not go through all the outstanding AI agent comments.