Fix enrollment endpoint errors - #52611
Conversation
GitHub Actions
Inactive
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🟡 Changes recommended
Authentication accepts manual-profile tokens and relies on potentially stale replica data for current DEP assignment state.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
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
Centralizes authentication for Apple automatic enrollment and standardizes enrollment failures.
Changes:
- Validates enrollment tokens, machine info, and DEP assignments earlier.
- Treats unsupported Apple update platforms as not found.
- Updates mocks and enrollment tests.
File summaries
| File | Description |
|---|---|
changes/apple-automatic-enrollment-restructure |
Excluded change note; contents unavailable. |
server/service/apple_mdm.go |
Adds centralized DEP enrollment authentication. |
server/fleet/service.go |
Extends the service interface. |
server/mock/service/service_mock.go |
Adds generated authentication mocks. |
server/datastore/mysql/apple_mdm.go |
Returns not-found for unsupported platforms. |
server/datastore/mysql/apple_mdm_test.go |
Tests unsupported-platform behavior. |
server/service/apple_mdm_test.go |
Tests authentication ordering and outcomes. |
server/service/integration_mdm_test.go |
Adds DEP assignments to test fixtures. |
server/service/integration_mdm_dep_test.go |
Synchronizes DEP before enrollment tests. |
server/service/integration_mdm_commands_test.go |
Uses manual enrollment for command restrictions. |
Review details
Files excluded by content exclusion policy (1)
- changes/apple-automatic-enrollment-restructure
- Files reviewed: 9/10 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if _, err := svc.ds.GetMDMAppleEnrollmentProfileByToken(ctx, token); err != nil { | ||
| if fleet.IsNotFound(err) { | ||
| return fleet.NewAuthFailedError("enrollment profile not found") | ||
| } | ||
| return ctxerr.Wrap(ctx, err, "get enrollment profile") |
There was a problem hiding this comment.
there is nowhere in the current code that creates or would allow someone to create such a manual profile, but removed the possibility anyways
| } | ||
|
|
||
| // Only devices currently assigned to Fleet in ABM may enroll through this path. | ||
| assignments, err := svc.ds.GetHostDEPAssignmentsBySerial(ctx, machineInfo.Serial) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (10)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. WalkthroughThe Apple MDM enrollment endpoint now authenticates the enrollment token and verifies the device’s DEP assignment before OS checks and profile generation. The service interface and mock include the new authentication method. Unit and integration tests cover authentication errors, processing order, DEP assignment setup, and ABM scheduling. Apple OS update settings lookups now return not-found errors for unsupported host platforms. Merge Risk: ⚪ Minimal · up to Apple DEP enrollment now rejects invalid tokens, missing machine details, and unassigned serials before downstream processing, while valid devices continue through enrollment. The supplied coverage supports the intended 4xx behavior and enrollment flow. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 7 files. (3 skipped: 1 unsupported, 2 too large.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ast-grep (0.45.2)server/service/integration_mdm_test.goast-grep timed out on this file 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 |
GitHub Actions
Inactive
Related issue: Resolves #
Centralizes error handling around invalid token/invalid serial/missing machineinfo cases. Prior, depending on whether or not OS updates were enabled or the host was assigned in DEP or not these would return any number of 4xx/5xx statuses. Moved the check logic up so that those consistent checks happen first, return a 4xx if necessary, then move on to the config-specific ones
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
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Summary by CodeRabbit
Security & Enrollment
Bug Fixes