Skip to content

Adding in check to disable recovery lock on personal macos since it doesn't have the required permissions#48598

Merged
georgekarrv merged 1 commit into
mainfrom
georgekarrv-48594-recovery-lock-is-enforced-but-fails-on
Jul 1, 2026
Merged

Adding in check to disable recovery lock on personal macos since it doesn't have the required permissions#48598
georgekarrv merged 1 commit into
mainfrom
georgekarrv-48594-recovery-lock-is-enforced-but-fails-on

Conversation

@georgekarrv

@georgekarrv georgekarrv commented Jul 1, 2026

Copy link
Copy Markdown
Member

Related issue: Resolves #48594

Checklist for submitter

If some of the following don't apply, delete the relevant line.

  • Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
    See Changes files for more information.

  • 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.

  • Timeouts are implemented and retries are limited to avoid infinite loops

  • If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes

Testing

For unreleased bug fixes in a release candidate, one of:

  • Confirmed that the fix is not expected to adversely impact load test results
  • Alerted the release DRI if additional load testing is needed

Summary by CodeRabbit

  • Bug Fixes

    • Recovery-lock password checks now skip personally owned (BYOD) Apple devices, avoiding failures on eligible hosts.
    • Recovery-lock clear actions are no longer applied to personally owned enrollments.
  • Tests

    • Added coverage to verify BYOD devices are excluded from both recovery-lock enforcement and clear workflows.

@georgekarrv georgekarrv marked this pull request as ready for review July 1, 2026 20:51
@georgekarrv georgekarrv requested a review from a team as a code owner July 1, 2026 20:51
Copilot AI review requested due to automatic review settings July 1, 2026 20:51

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

  • Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.

Pull request overview

This PR prevents Fleet from attempting to set or clear macOS Recovery Lock on personally-owned (BYOD) MDM enrollments, where Apple strips DeviceLock/DeviceErase access rights and the command is rejected (per #48594).

Changes:

  • Exclude host_mdm.is_personal_enrollment=1 hosts from eligibility for Recovery Lock set and clear selection queries.
  • Add datastore tests to ensure personally-owned enrollments are not selected/claimed for these Recovery Lock operations.

Reviewed changes

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

File Description
server/datastore/mysql/apple_mdm.go Adds hm.is_personal_enrollment = 0 filter to Recovery Lock set/clear host-selection SQL queries and documents rationale.
server/datastore/mysql/apple_mdm_test.go Adds coverage ensuring BYOD/personal-enrollment hosts are not eligible for set, and not claimed for clear.
changes/48594-recovery-lock-byod-personal-enrollment Release-note entry (content excluded by policy; not reviewed).
Files excluded by content exclusion policy (1)
  • changes/48594-recovery-lock-byod-personal-enrollment

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

@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

This change excludes personally-owned (BYOD) macOS/iOS/iPadOS hosts from recovery-lock password enforcement. SQL queries in GetHostsForRecoveryLockAction and ClaimHostsForRecoveryLockClear now filter on hm.is_personal_enrollment = 0, preventing BYOD hosts from being selected for recovery-lock actions or clears. New tests verify BYOD hosts are excluded from both functions, and a changelog fragment documents the fix.

Changes

  • server/datastore/mysql/apple_mdm.go: Added a clarifying comment and hm.is_personal_enrollment = 0 filter to GetHostsForRecoveryLockAction and ClaimHostsForRecoveryLockClear SQL queries.
  • server/datastore/mysql/apple_mdm_test.go: Added test cases confirming BYOD-enrolled hosts are excluded from both functions' results.
  • changes/48594-recovery-lock-byod-personal-enrollment: Added changelog entry describing the fix.

Sequence Diagram(s)

Not applicable — this change is a query filtering fix without significant new control flow.

Related issues

Possibly related PRs

  • fleetdm/fleet#48534: Modifies handling of is_personal_enrollment persistence in the same file, which this PR relies on for filtering.

Suggested reviewers

  • roperzh
  • getvictor

Poem
A rabbit hopped through code so neat,
Skipping BYOD hosts, a clever feat.
No more locks where phones roam free,
Just is_personal_enrollment = 0, you see.
Tests confirm the fix holds tight — 🐇🔒

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: skipping Recovery Lock enforcement for personal Mac devices.
Linked Issues check ✅ Passed The change skips Recovery Lock for personally-owned hosts and adds tests that match the BYOD macOS bug report.
Out of Scope Changes check ✅ Passed All changes are directly related to Recovery Lock BYOD handling, plus tests and the required changes file.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed The PR description includes the required related issue and relevant checklist/testing items, and the omitted sections appear non-applicable rather than missing.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch georgekarrv-48594-recovery-lock-is-enforced-but-fails-on

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.01%. Comparing base (4076d13) to head (37819b7).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #48598   +/-   ##
=======================================
  Coverage   68.01%   68.01%           
=======================================
  Files        3678     3678           
  Lines      233758   233760    +2     
  Branches    12453    12453           
=======================================
+ Hits       158981   158988    +7     
+ Misses      60475    60474    -1     
+ Partials    14302    14298    -4     
Flag Coverage Δ
backend 69.66% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JordanMontgomery JordanMontgomery left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved conditionally - PR template still needs to be used. But code looks good

@georgekarrv

Copy link
Copy Markdown
Member Author

QA'd locally. Looks good. The failing test is unrelated

@georgekarrv georgekarrv merged commit 80b883a into main Jul 1, 2026
50 of 55 checks passed
@georgekarrv georgekarrv deleted the georgekarrv-48594-recovery-lock-is-enforced-but-fails-on branch July 1, 2026 21:20
@georgekarrv

Copy link
Copy Markdown
Member Author

#48601 cherry pick into 89

georgekarrv added a commit that referenced this pull request Jul 2, 2026
…nal macos since it doesn't have the required permissions (#48601)

Cherry-pick of #48598 into the RC branch `rc-minor-fleet-v4.89.0`.
Leanngove pushed a commit that referenced this pull request Jul 2, 2026
…oesn't have the required permissions (#48598)

<!-- Add the related story/sub-task/bug number, like Resolves #123, or
remove if NA -->
**Related issue:** Resolves #48594

# Checklist for submitter

If some of the following don't apply, delete the relevant line.

- [x] 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.

- [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.
- [x] Timeouts are implemented and retries are limited to avoid infinite
loops
- [x] If paths of existing endpoints are modified without backwards
compatibility, checked the frontend/CLI for any necessary changes

## Testing

- [x] Added/updated automated tests
- [x] Where appropriate, [automated tests simulate multiple hosts and
test for host
isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing)
(updates to one hosts's records do not affect another)

- [x] QA'd all new/changed functionality manually

For unreleased bug fixes in a release candidate, one of:

- [x] Confirmed that the fix is not expected to adversely impact load
test results
- [x] Alerted the release DRI if additional load testing is needed

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
* Recovery-lock password checks now skip personally owned (BYOD) Apple
devices, avoiding failures on eligible hosts.
* Recovery-lock clear actions are no longer applied to personally owned
enrollments.

* **Tests**
* Added coverage to verify BYOD devices are excluded from both
recovery-lock enforcement and clear workflows.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Recovery lock is enforced but fails on BYOD Manually enrolled MacOS hosts

3 participants