Add user-scoped declaration support#48796
Conversation
JordanMontgomery
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
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
Adds support for Apple DDM “user-scoped” declarations by introducing a Fleet-only PayloadScope key that controls whether declarations are delivered on the device (System) or user (User) channel, and then updating reconcile + serving logic to keep the two channels isolated end-to-end.
Changes:
- Parse/validate
PayloadScopeon upload, persist it asscope, and default missing scope toSystemfor backward compatibility. - Scope DDM serving and status transitions by channel (tokens, declaration-items, declaration payloads, status reports, and DeclarativeManagement acks).
- Update batched reconciliation to separately poke device vs user channels, including user-channel enrollment handling, grace-window holding, and cleanup for undeliverable user-scoped removals.
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/mdm.go | Validate and persist declaration scope when building declarations via GitOps/batch paths. |
| server/service/integration_mdm_ddm_test.go | Adds integration coverage that batch upload persists scope correctly (default + explicit User). |
| server/service/apple_mdm.go | Scopes DDM serving/ack handling by channel; strips Fleet-only PayloadScope at delivery. |
| server/service/apple_mdm_declarations_batched.go | Reconcile now partitions device/user changes and handles user-channel delivery decisions + cleanup. |
| server/service/apple_mdm_declarations_batched_test.go | Unit tests for user-channel delivery decision logic (deliver/hold/fail/remove-delete). |
| server/service/apple_mdm_ddm_test.go | Service-level tests for channel isolation (tokens/items) and delivery-time stripping of PayloadScope. |
| server/mock/datastore_mock.go | Updates datastore mock for new scope-aware DDM methods and new bulk delete method. |
| server/mdm/apple/reconcile.go | ComputeDeclarationDeltas now tracks changes per channel and handles scope flips. |
| server/mdm/apple/reconcile_test.go | Adds/updates tests for per-channel delta computation and scope flip behavior. |
| server/fleet/datastore.go | Datastore interface updated for scope-aware DDM queries and per-channel resync + bulk delete. |
| server/fleet/apple_mdm.go | Adds scope fields + PayloadScope parsing/validation helpers on raw declarations and host decl rows. |
| server/fleet/apple_mdm_test.go | Unit tests for PayloadScope parsing/defaulting/validation and host-decl equality update. |
| server/datastore/mysql/apple_mdm.go | Persists scope for declarations, scopes DDM queries by channel, partitions resync by scope. |
| server/datastore/mysql/apple_mdm_test.go | Updates tests for new scope-aware DDM token method + pending declarations transition signature. |
| server/datastore/mysql/apple_mdm_ddm_test.go | Adds datastore-level test asserting device/user channel isolation across DDM queries and status reports. |
| server/datastore/mysql/apple_mdm_batched.go | Includes scope in batched host declaration reads/writes and adds bulk delete helper. |
| changes/48567-user-scoped-declarations | User-visible change note (content excluded from review). |
| docs/Contributing/architecture/mdm/apple-declarative-device-management.md | Architecture docs update (content excluded from review). |
Files excluded by content exclusion policy (2)
- changes/48567-user-scoped-declarations
- docs/Contributing/architecture/mdm/apple-declarative-device-management.md
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // PayloadScope is a Fleet extension, not part of Apple's DDM schema. It's | ||
| // normally stripped at upload, but a declaration provided entirely as a | ||
| // $FLEET_SECRET_ token only resolves to JSON here, so strip it again after | ||
| // expansion to guarantee it never reaches the device. |
|
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 ignored due to path filters (1)
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughThis PR adds user-channel scoping for Apple MDM declarative management. It introduces Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/datastore/mysql/apple_mdm.go (1)
4694-4734: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate duplicated scope-defaulting logic.
The
if scope == "" { scope = fleet.PayloadScopeSystem }pattern (with the identical "Scope defaults to System" comment) is duplicated ininsertOrUpdateDeclarations(Line 4724-4727) andinsertOrUpsertMDMAppleDeclaration(Line 4917-4920), and again inBulkUpsertMDMAppleHostDeclarationsinapple_mdm_batched.go. Extracting a small helper (e.g.scopeOrDefault(scope fleet.PayloadScope) fleet.PayloadScope) would reduce triplication and the risk of the default drifting out of sync across call sites.♻️ Proposed helper
func scopeOrDefault(scope fleet.PayloadScope) fleet.PayloadScope { if scope == "" { return fleet.PayloadScopeSystem } return scope }Also applies to: 4907-4927
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/apple_mdm.go` around lines 4694 - 4734, The scope-defaulting logic is duplicated across insertOrUpdateDeclarations, insertOrUpsertMDMAppleDeclaration, and BulkUpsertMDMAppleHostDeclarations, so extract it into a small helper like scopeOrDefault and use that everywhere. Replace each inline `if scope == "" { scope = fleet.PayloadScopeSystem }` block with the shared helper to keep the default consistent and avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/service/apple_mdm_declarations_batched.go`:
- Around line 111-113: The empty-declaration early return in
MDMAppleHostDeclarationsBatchWrite is bypassing resync handling, so resync-only
hosts never reach MDMAppleHostDeclarationsGetAndClearResync and keep their
resync flag set. Move the len(declRowsToWrite) == 0 return to after the
resync-processing block, ensuring the resync lookup/clear logic still runs even
when there are no declaration rows to write.
---
Nitpick comments:
In `@server/datastore/mysql/apple_mdm.go`:
- Around line 4694-4734: The scope-defaulting logic is duplicated across
insertOrUpdateDeclarations, insertOrUpsertMDMAppleDeclaration, and
BulkUpsertMDMAppleHostDeclarations, so extract it into a small helper like
scopeOrDefault and use that everywhere. Replace each inline `if scope == "" {
scope = fleet.PayloadScopeSystem }` block with the shared helper to keep the
default consistent and avoid drift.
🪄 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: d0f290b0-515e-4e34-9fdb-f08c787b4c73
⛔ Files ignored due to path filters (1)
docs/Contributing/architecture/mdm/apple-declarative-device-management.mdis excluded by!**/*.md
📒 Files selected for processing (17)
changes/48567-user-scoped-declarationsserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_batched.goserver/datastore/mysql/apple_mdm_ddm_test.goserver/datastore/mysql/apple_mdm_test.goserver/fleet/apple_mdm.goserver/fleet/apple_mdm_test.goserver/fleet/datastore.goserver/mdm/apple/reconcile.goserver/mdm/apple/reconcile_test.goserver/mock/datastore_mock.goserver/service/apple_mdm.goserver/service/apple_mdm_ddm_test.goserver/service/apple_mdm_declarations_batched.goserver/service/apple_mdm_declarations_batched_test.goserver/service/integration_mdm_ddm_test.goserver/service/mdm.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #48796 +/- ##
==========================================
+ Coverage 68.02% 68.04% +0.01%
==========================================
Files 3681 3684 +3
Lines 233961 234281 +320
Branches 12453 12453
==========================================
+ Hits 159146 159410 +264
- Misses 60497 60536 +39
- Partials 14318 14335 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
JordanMontgomery
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
JordanMontgomery
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
|
@claude review once |
![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.
Related issue: Resolves #
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