don't send AccountConfiguration for non macs - #44085
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
GitHub Actions
Inactive
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe post-DEP enrollment flow was changed to only enqueue the 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/worker/apple_mdm_test.go (1)
827-839: Consider tightening the assertion.Unlike the sibling macOS test (line 792) which uses
ElementsMatchagainst the full expected command list, this test only checksNotContains "AccountConfiguration". Given the comment at lines 834–836 states no MDM commands should be enqueued for iOS here, anElementsMatch(t, []string{}, cmdTypes)(orrequire.Empty(t, cmdTypes)) would more precisely pin the behavior and catch regressions that send other unintended commands to iOS.Also, the queued release-device follow-up job mentioned in the comment isn't actually asserted — consider adding a
ds.GetQueuedJobscheck if that behavior is worth pinning, or remove the aside from the comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/worker/apple_mdm_test.go` around lines 827 - 839, Tighten the test assertion so it verifies no MDM commands are enqueued for iOS rather than only asserting absence of "AccountConfiguration": replace the current require.NotContains check on cmdTypes with require.Empty(t, cmdTypes) or require.ElementsMatch(t, []string{}, cmdTypes) to assert an empty command list; update the test around QueueAppleMDMJob / w.ProcessJobs and getEnqueuedCommandTypes usage accordingly, and optionally either add a ds.GetQueuedJobs assertion to pin the expected queued release-device follow-up job or remove that aside from the comment.changes/43228-dont-send-account-configuration-for-non-mac (1)
1-1: Minor: changelog phrasing is a bit awkward.Consider tightening wording, e.g.: "Fixed an issue where an
AccountConfigurationcommand (macOS-only) was incorrectly sent to iOS/iPadOS devices when end-user authentication was enabled."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@changes/43228-dont-send-account-configuration-for-non-mac` at line 1, Update the changelog sentence to the tighter phrasing: replace the current line describing the fix with: "Fixed an issue where an `AccountConfiguration` command (macOS-only) was incorrectly sent to iOS/iPadOS devices when end-user authentication was enabled." Locate the existing entry mentioning AccountConfiguration and macOS only and swap the text exactly as suggested, preserving the command name formatting and parentheses around "macOS-only."
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@changes/43228-dont-send-account-configuration-for-non-mac`:
- Line 1: Update the changelog sentence to the tighter phrasing: replace the
current line describing the fix with: "Fixed an issue where an
`AccountConfiguration` command (macOS-only) was incorrectly sent to iOS/iPadOS
devices when end-user authentication was enabled." Locate the existing entry
mentioning AccountConfiguration and macOS only and swap the text exactly as
suggested, preserving the command name formatting and parentheses around
"macOS-only."
In `@server/worker/apple_mdm_test.go`:
- Around line 827-839: Tighten the test assertion so it verifies no MDM commands
are enqueued for iOS rather than only asserting absence of
"AccountConfiguration": replace the current require.NotContains check on
cmdTypes with require.Empty(t, cmdTypes) or require.ElementsMatch(t, []string{},
cmdTypes) to assert an empty command list; update the test around
QueueAppleMDMJob / w.ProcessJobs and getEnqueuedCommandTypes usage accordingly,
and optionally either add a ds.GetQueuedJobs assertion to pin the expected
queued release-device follow-up job or remove that aside from the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9865ff96-c0cf-4f86-831b-86cbfabcc63c
📒 Files selected for processing (3)
changes/43228-dont-send-account-configuration-for-non-macserver/worker/apple_mdm.goserver/worker/apple_mdm_test.go
There was a problem hiding this comment.
Pull request overview
This PR fixes an Apple MDM enrollment issue where Fleet would enqueue the macOS-only AccountConfiguration command for iOS/iPadOS devices during ADE enrollment when end-user authentication (SSO) is enabled, leading to rejected-command errors.
Changes:
- Gate
AccountConfigurationenqueueing to macOS platforms only. - Add a regression test ensuring iOS enrollment with SSO enabled does not enqueue
AccountConfiguration. - Add a changelog entry documenting the fix.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/worker/apple_mdm.go | Adds a macOS platform guard around AccountConfiguration enqueueing logic. |
| server/worker/apple_mdm_test.go | Adds a test covering iOS ADE enrollment with SSO enabled to ensure AccountConfiguration is not enqueued. |
| changes/43228-dont-send-account-configuration-for-non-mac | Documents the user-visible behavior fix in release notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #44085 +/- ##
==========================================
+ Coverage 63.40% 66.76% +3.35%
==========================================
Files 2628 2623 -5
Lines 211127 211139 +12
Branches 9499 9270 -229
==========================================
+ Hits 133856 140957 +7101
+ Misses 64922 57355 -7567
- Partials 12349 12827 +478
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:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
Related issue: Resolves #43228
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/oree/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
Summary by CodeRabbit
Bug Fixes
Tests