validate apple payload scope conflict, and unknown variable use in dry-run - #45139
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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds a datastore API and MySQL implementation to verify Apple config profile PayloadScope conflicts, refactors the DB helper for reuse, and invokes this verification from Apple MDM and cross-platform MDM batch flows before honoring dry-run. Service validation order was adjusted so Fleet-variable and PayloadScope checks run during dry-run, and unit/integration tests and mocks were added or updated to cover conflict detection, edits/implicit-scope changes, legacy coercion, and dry-run invocation. 🚥 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 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/service/apple_mdm_test.go (1)
2756-2760: ⚡ Quick winAdd a no-write assertion for this dry-run failure path.
This test verifies the conflict is surfaced, but it doesn’t assert that persistence stays untouched when dry-run fails. Please also assert
ds.BatchSetMDMAppleProfilesFuncInvokedis false here.Suggested test hardening
err := svc.BatchSetMDMAppleProfiles(ctx, nil, nil, [][]byte{mobileconfigForTest("N1", "I1")}, true, false) require.Error(t, err) require.ErrorContains(t, err, "scope conflict") require.True(t, ds.VerifyAppleConfigProfileScopesDoNotConflictFuncInvoked) + require.False(t, ds.BatchSetMDMAppleProfilesFuncInvoked)🤖 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/service/apple_mdm_test.go` around lines 2756 - 2760, Add a no-write assertion after the dry-run failure call so the test also verifies persistence was not invoked: after calling svc.BatchSetMDMAppleProfiles and the existing require.Error/require.ErrorContains/require.True assertions, add require.False(t, ds.BatchSetMDMAppleProfilesFuncInvoked) to ensure the BatchSetMDMAppleProfiles persistence hook on the mock data store (ds.BatchSetMDMAppleProfilesFuncInvoked) was not invoked during the dry-run path that surfaced the scope conflict.
🤖 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/datastore/mysql/apple_mdm.go`:
- Around line 2426-2427: Reintroduce datastore-level scope conflict validation
inside batchSetMDMAppleProfilesDB: before performing the DB write, iterate the
incoming profiles and validate their scopes the same way the service layer does
(reject conflicting or overlapping scope assignments), returning an error if any
conflict is detected so the write is aborted; locate the validation logic used
by the service layer and replicate or call it from batchSetMDMAppleProfilesDB to
enforce the invariant at the datastore boundary.
---
Nitpick comments:
In `@server/service/apple_mdm_test.go`:
- Around line 2756-2760: Add a no-write assertion after the dry-run failure call
so the test also verifies persistence was not invoked: after calling
svc.BatchSetMDMAppleProfiles and the existing
require.Error/require.ErrorContains/require.True assertions, add
require.False(t, ds.BatchSetMDMAppleProfilesFuncInvoked) to ensure the
BatchSetMDMAppleProfiles persistence hook on the mock data store
(ds.BatchSetMDMAppleProfilesFuncInvoked) was not invoked during the dry-run path
that surfaced the scope conflict.
🪄 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: 4b35e875-e220-4759-b204-f017a8596ca2
📒 Files selected for processing (9)
changes/44456-validate-payload-scope-dry-runserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_test.goserver/fleet/datastore.goserver/mock/datastore_mock.goserver/service/apple_mdm.goserver/service/apple_mdm_test.goserver/service/mdm.goserver/service/mdm_test.go
| // We don't verify profile scopes conflict, as we have already done it at the service level | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Re-add datastore-level scope validation in batch set path
On Line 2426, removing validation from batchSetMDMAppleProfilesDB makes correctness depend on every caller doing service-layer prechecks. Keeping the datastore guard here preserves the invariant at the write boundary and avoids accidental bypasses.
Suggested patch
- // We don't verify profile scopes conflict, as we have already done it at the service level
+ // Keep datastore-level verification as a write-path safeguard (service may also pre-validate).
+ if err := ds.verifyAppleConfigProfileScopesDoNotConflictDB(ctx, tx, profiles); err != nil {
+ return false, err
+ }🤖 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 2426 - 2427, Reintroduce
datastore-level scope conflict validation inside batchSetMDMAppleProfilesDB:
before performing the DB write, iterate the incoming profiles and validate their
scopes the same way the service layer does (reject conflicting or overlapping
scope assignments), returning an error if any conflict is detected so the write
is aborted; locate the validation logic used by the service layer and replicate
or call it from batchSetMDMAppleProfilesDB to enforce the invariant at the
datastore boundary.
There was a problem hiding this comment.
This is a trade-off, and I'm unsure which direction we want to go.
Start doing more work in the:
if dryRun {
// More work here
return
}And also keep it around, which IMO makes it harder to track, when it was called and not, but as called out it requires services to ensure validation has happened.
| } | ||
|
|
||
| // Verify Apple Config profiles PaylodScope conflicts | ||
| err = svc.ds.VerifyAppleConfigProfileScopesDoNotConflict(ctx, appleProfilesSlice) |
There was a problem hiding this comment.
Would it make sense to call this only in the dry-run case? Since it will be called again during the actual insert call and it can potentially be expensive(large number of teams with duplicate profiles)?
There was a problem hiding this comment.
It should not, I removed it from the DB call (where we did this check before), so it would run once for both dry-run and actual runs. If one is left behind, that is a mistake.
There was a problem hiding this comment.
Pull request overview
This PR updates the MDM profile batch apply flow so that GitOps dry-run performs the same key validations as a real run—specifically catching Apple configuration profile PayloadScope conflicts and validating Fleet variable usage—matching the behavior reported in #44456.
Changes:
- Move the
dryRunearly-return inBatchSetMDMProfilesto occur after Fleet variable validation and ApplePayloadScopeconflict checks. - Add a new datastore-level API (
VerifyAppleConfigProfileScopesDoNotConflict) and call it from both the modern batch endpoint and the deprecated Apple-only endpoint, including dry-run paths. - Add/adjust unit tests and a new MySQL datastore test suite for scope-conflict validation logic.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/mdm.go | Runs Apple PayloadScope conflict verification (and variable validation) before returning on dry-run. |
| server/service/mdm_test.go | Adds dry-run coverage for variable validation and scope-conflict validation. |
| server/service/apple_mdm.go | Runs Apple PayloadScope conflict verification before returning on dry-run in the deprecated endpoint. |
| server/service/apple_mdm_test.go | Adds dry-run coverage ensuring scope-conflict validation triggers. |
| server/mock/datastore_mock.go | Adds mock datastore method for scope-conflict verification. |
| server/fleet/datastore.go | Extends the datastore interface with the new verification method. |
| server/datastore/mysql/apple_mdm.go | Exposes and refactors the scope-conflict verification implementation. |
| server/datastore/mysql/apple_mdm_test.go | Adds focused tests for scope-conflict detection and legacy migration behavior. |
| changes/44456-validate-payload-scope-dry-run | Adds a release note entry for the fix. |
💡 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 #45139 +/- ##
=======================================
Coverage 66.80% 66.81%
=======================================
Files 2720 2721 +1
Lines 218985 218994 +9
Branches 10747 10747
=======================================
+ Hits 146296 146319 +23
+ Misses 59520 59509 -11
+ Partials 13169 13166 -3
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:
|
Related issue: Resolves #44456
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