Add combined include/exclude label targeting for MDM profiles (API and GitOps)#46437
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.
There was a problem hiding this comment.
Pull request overview
This PR updates MDM configuration profile label targeting so profiles/declarations can combine an include label mode with labels_exclude_any, while preventing overlapping include/exclude labels and blocking deletion of labels referenced by MDM profiles.
Changes:
- Relaxed profile label validation across API, app config, team specs, and GitOps label usage to allow include + exclude combinations.
- Added overlap detection for labels appearing in both include and exclude lists.
- Added datastore-level deletion guard and tests for labels referenced by MDM profiles/declarations.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
server/service/windows_mdm_profiles.go |
Passes and validates excluded labels separately for Windows profiles. |
server/service/testing_client_test.go |
Updates teardown to null MDM profile/declaration label references before deleting labels. |
server/service/mdm.go |
Updates multipart/batch validation and profile creation dispatch for include + exclude targeting. |
server/service/mdm_test.go |
Updates service test callers for the new profile creation signatures. |
server/service/integration_mdm_profiles_test.go |
Updates integration coverage for valid include + exclude targeting and overlap errors. |
server/service/apple_mdm.go |
Adds separate excluded-label handling for Apple profiles and declarations. |
server/service/apple_mdm_test.go |
Updates Apple service tests for the new method signatures. |
server/service/appconfig.go |
Updates app config validation for include modes and include/exclude overlap. |
server/mock/service/service_mock.go |
Updates mock service signatures and forwarding. |
server/fleet/service.go |
Updates service interface signatures. |
server/fleet/mdm.go |
Adds helper for detecting include/exclude label overlap. |
server/datastore/mysql/labels.go |
Blocks label deletion when referenced by MDM profile/declaration label rows. |
server/datastore/mysql/labels_test.go |
Adds deletion-blocking coverage for profile/declaration label references. |
ee/server/service/teams.go |
Updates team custom settings validation for include + exclude targeting. |
cmd/fleetctl/fleetctl/gitops.go |
Updates GitOps label usage collection for MDM custom settings. |
cmd/fleetctl/fleetctl/gitops_label_usage_test.go |
Adds GitOps label usage tests for include + exclude combinations. |
changes/32073-cpie-integration-test2 |
Adds changelog entry for targeting and label deletion behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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:
WalkthroughThis PR allows combining labels_exclude_any with a single include mode for MDM profiles, enforces that only one include mode is specified, rejects any label present in both include and exclude lists, propagates exclude lists through profile creation APIs (Apple/Windows/Android), updates Service signatures and mocks, updates Fleetctl/GitOps label usage, prevents deleting labels referenced by MDM profiles/declarations, and updates tests and fixtures to reflect these behaviors. Possibly related PRs
🚥 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/service/mdm.go (1)
1843-1895:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce the include/exclude overlap rule in the service method.
NewMDMAndroidConfigProfilevalidates both label sets separately, but it never rejects a label that appears in bothlabelsandlabelsExcludeAny. That means a direct service caller can still persist the contradictory targeting this PR is supposed to forbid, even though the request decoders and batch validator already block it.Suggested fix
func (svc *Service) NewMDMAndroidConfigProfile(ctx context.Context, teamID uint, profileName string, data []byte, labels []string, labelsMembershipMode fleet.MDMLabelsMode, labelsExcludeAny []string) (*fleet.MDMAndroidConfigProfile, error) { if err := svc.authz.Authorize(ctx, &fleet.MDMConfigProfileAuthz{TeamID: &teamID}, fleet.ActionWrite); err != nil { return nil, ctxerr.Wrap(ctx, err) } @@ if err := cp.ValidateUserProvided(license.IsPremium(ctx)); err != nil { err := &fleet.BadRequestError{Message: "Couldn't add. " + err.Error()} return nil, ctxerr.Wrap(ctx, err, "validate profile") } + + if overlap := fleet.ProfileLabelOverlap(labels, labelsExcludeAny); overlap != "" { + return nil, ctxerr.Wrap(ctx, + fleet.NewInvalidArgumentError("labels_exclude_any", + fmt.Sprintf(`Label %q cannot appear in both include and exclude lists.`, overlap)), + "validate profile labels", + ) + } labelMap, err := svc.validateProfileLabels(ctx, &teamID, labels) if err != nil { return nil, ctxerr.Wrap(ctx, err, "validating labels") }
🧹 Nitpick comments (3)
server/service/integration_mdm_profiles_test.go (1)
3833-3837: ⚡ Quick winNarrow the broken-label simulation update scope.
At Line 3836,
WHERE label_id = ?updates all associations for that label in the test DB. Scope it to the three profiles created in this test to keep isolation robust.💡 Suggested diff
mysqltest.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { - _, err := q.ExecContext(ctx, `UPDATE mdm_configuration_profile_labels SET label_id = NULL WHERE label_id = ?`, lblFoo.ID) + _, err := q.ExecContext(ctx, ` + UPDATE mdm_configuration_profile_labels + SET label_id = NULL + WHERE label_id = ? + AND apple_profile_uuid IN (?, ?, ?)`, + lblFoo.ID, tm2ProfF.ProfileUUID, tm2ProfG.ProfileUUID, tm2ProfH.ProfileUUID) return err })As per coding guidelines: “When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied… check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results.”
🤖 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/integration_mdm_profiles_test.go` around lines 3833 - 3837, The SQL update currently nullifies all rows for lblFoo via mysqltest.ExecAdhocSQL on mdm_configuration_profile_labels WHERE label_id = ?, which affects every association for that label; restrict the update to only the three profiles created in this test by adding an explicit filter on profile IDs (e.g., WHERE label_id = ? AND profile_id IN (...)) using the test's profile ID variables (the slice/vars holding the three created profile IDs) so only those three associations are nullified.server/service/testing_client_test.go (1)
183-191: 💤 Low valueConsider deleting label association records instead of nulling them.
The current approach sets
label_id = NULLin the MDM profile/declaration label tables to work around the new RESTRICT FK constraints. While this allows label deletion to proceed, it leaves orphaned records inmdm_configuration_profile_labelsandmdm_declaration_labels.For cleaner test state, consider deleting these records entirely:
DELETE FROM mdm_configuration_profile_labels WHERE label_id IS NOT NULL DELETE FROM mdm_declaration_labels WHERE label_id IS NOT NULLThis would ensure no leftover association records accumulate across test runs.
🤖 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/testing_client_test.go` around lines 183 - 191, Replace the UPDATE-null workaround with deletes so we remove association rows instead of leaving orphans: inside the mysqltest.ExecAdhocSQL block that currently executes UPDATE mdm_configuration_profile_labels and UPDATE mdm_declaration_labels, execute DELETE FROM mdm_configuration_profile_labels WHERE label_id IS NOT NULL and DELETE FROM mdm_declaration_labels WHERE label_id IS NOT NULL instead (i.e., update the SQL executed by mysqltest.ExecAdhocSQL in testing_client_test.go to perform deletes rather than setting label_id = NULL).cmd/fleetctl/fleetctl/gitops_label_usage_test.go (1)
73-98: ⚡ Quick winAdd the overlap rejection case here too.
These cases only prove that include+exclude is allowed. They don't cover the required failure mode where the same label appears in both lists, so the current regression in
getLabelUsagewould still pass the test suite.🤖 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 `@cmd/fleetctl/fleetctl/gitops_label_usage_test.go` around lines 73 - 98, Update TestGetLabelUsageIncludeAndExcludeAllowed to also assert the overlap rejection: add a case where the same label appears in both an include list and an exclude list (e.g., LabelsIncludeAny/LabelsIncludeAll and LabelsExcludeAny using the same string) for a fleet.MDMProfileSpec, call getLabelUsage(config) and require that it returns a non-nil error (rather than succeeding), and ensure the test checks that this overlapping-label scenario is treated as invalid by getLabelUsage.
🤖 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 `@cmd/fleetctl/fleetctl/gitops.go`:
- Around line 853-857: The code currently only rejects using both
LabelsIncludeAll and LabelsIncludeAny together but does not prevent the same
label appearing in an include list and in LabelsExcludeAny; add a check before
calling updateLabelUsage to detect any overlap between setting.LabelsExcludeAny
and the union of setting.LabelsIncludeAll and setting.LabelsIncludeAny (compute
intersection of those slices), and if any conflicting labels are found return an
error (include filepath.Base(setting.Path) and the conflicting label names) so
the profile is rejected early; keep using the same symbols
(setting.LabelsIncludeAll, setting.LabelsIncludeAny, setting.LabelsExcludeAny,
updateLabelUsage) to locate where to insert the check.
In `@server/service/windows_mdm_profiles.go`:
- Around line 80-86: When validating labelsExcludeAny in windows_mdm_profiles.go
(the block that calls svc.validateProfileLabels and assigns
cp.LabelsExcludeAny), add a check that rejects any label that appears in both
the include set (cp.Labels or labels from the earlier validateProfileLabels
call) and the exclude set: compute the intersection between the validated
include labels (cp.Labels or the map returned for labels) and the excludeMap
returned for labelsExcludeAny, and if any overlap exists return an error (use
ctxerr.Wrap for consistency) indicating conflicting include/exclude labels so
contradictory profiles cannot be created.
---
Nitpick comments:
In `@cmd/fleetctl/fleetctl/gitops_label_usage_test.go`:
- Around line 73-98: Update TestGetLabelUsageIncludeAndExcludeAllowed to also
assert the overlap rejection: add a case where the same label appears in both an
include list and an exclude list (e.g., LabelsIncludeAny/LabelsIncludeAll and
LabelsExcludeAny using the same string) for a fleet.MDMProfileSpec, call
getLabelUsage(config) and require that it returns a non-nil error (rather than
succeeding), and ensure the test checks that this overlapping-label scenario is
treated as invalid by getLabelUsage.
In `@server/service/integration_mdm_profiles_test.go`:
- Around line 3833-3837: The SQL update currently nullifies all rows for lblFoo
via mysqltest.ExecAdhocSQL on mdm_configuration_profile_labels WHERE label_id =
?, which affects every association for that label; restrict the update to only
the three profiles created in this test by adding an explicit filter on profile
IDs (e.g., WHERE label_id = ? AND profile_id IN (...)) using the test's profile
ID variables (the slice/vars holding the three created profile IDs) so only
those three associations are nullified.
In `@server/service/testing_client_test.go`:
- Around line 183-191: Replace the UPDATE-null workaround with deletes so we
remove association rows instead of leaving orphans: inside the
mysqltest.ExecAdhocSQL block that currently executes UPDATE
mdm_configuration_profile_labels and UPDATE mdm_declaration_labels, execute
DELETE FROM mdm_configuration_profile_labels WHERE label_id IS NOT NULL and
DELETE FROM mdm_declaration_labels WHERE label_id IS NOT NULL instead (i.e.,
update the SQL executed by mysqltest.ExecAdhocSQL in testing_client_test.go to
perform deletes rather than setting label_id = NULL).
🪄 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: 3f8c8a7e-6021-4208-bff6-a3bd1709c02e
📒 Files selected for processing (17)
changes/32073-cpie-integration-test2cmd/fleetctl/fleetctl/gitops.gocmd/fleetctl/fleetctl/gitops_label_usage_test.goee/server/service/teams.goserver/datastore/mysql/labels.goserver/datastore/mysql/labels_test.goserver/fleet/mdm.goserver/fleet/service.goserver/mock/service/service_mock.goserver/service/appconfig.goserver/service/apple_mdm.goserver/service/apple_mdm_test.goserver/service/integration_mdm_profiles_test.goserver/service/mdm.goserver/service/mdm_test.goserver/service/testing_client_test.goserver/service/windows_mdm_profiles.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #46437 +/- ##
=======================================
Coverage 66.90% 66.90%
=======================================
Files 2831 2831
Lines 224929 224943 +14
Branches 11524 11487 -37
=======================================
+ Hits 150481 150494 +13
Misses 60796 60796
- Partials 13652 13653 +1
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:
|
andymFleet
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_test.go`:
- Around line 310-312: The UPDATE in ExecAdhocSQL currently nulls label_id for
all rows matching lbl.ID in mdm_configuration_profile_labels; narrow the WHERE
clause to the specific profile under test by adding the profile identifier (e.g.
apple_profile_uuid or the profile id) to the predicate so the ExecContext call
only updates rows where label_id = ? AND apple_profile_uuid = <profileUUID> (or
the corresponding profile id variable) to avoid cross-row mutations.
🪄 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: 18e2ac4e-89f4-47ac-b747-28cafba289d1
📒 Files selected for processing (7)
cmd/fleetctl/fleetctl/gitops_test.gocmd/fleetctl/fleetctl/testdata/gitops/global_windows_custom_settings_invalid_label_mix.ymlserver/datastore/mysql/apple_mdm_test.goserver/datastore/mysql/mdm_test.goserver/datastore/mysql/microsoft_mdm_test.goserver/service/integration_mdm_profiles_test.goserver/service/mdm.go
✅ Files skipped from review due to trivial changes (1)
- cmd/fleetctl/fleetctl/testdata/gitops/global_windows_custom_settings_invalid_label_mix.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- server/service/integration_mdm_profiles_test.go
- server/service/mdm.go
JordanMontgomery
left a comment
JordanMontgomery
left a comment
There was a problem hiding this comment.
One other comment I'm wondering about is if the label validation function can be updated to only do a single validation, rather than once for include labels and once for exclude labels. The reason I ask is it looks like the existing Batch endpoint already does that which makes me think the function probably supports it just fine, though not sure if it makes the code any more or less readable it would reduce the number of hits to the DB for a profile upload
…en labels via NULL update
…e_any can now combine with include modes
…on across appconfig and teams
andymFleet
force-pushed
the
45180-cpie-api-gitops-include-exclude-targeting
branch
from
d02215d to
2a305c6
Compare
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Related issue: Resolves #45180
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.Testing
Added/updated automated tests
QA'd all new/changed functionality manually
For unreleased bug fixes in a release candidate, one of:
Summary by CodeRabbit
New Features
Bug Fixes