48342 edit config profile endpoint#49141
Draft
andymFleet wants to merge 21 commits into
Draft
Conversation
… OS-update LocURI
Contributor
Author
|
Raise a draft for handover. |
Contributor
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 a new PATCH endpoint to edit existing MDM configuration profiles (Apple .mobileconfig, Apple declarations, Windows, Android) in place, including optional label-scope changes, with platform-specific validation and new datastore update methods.
Changes:
- Added
PATCH /api/_version_/fleet/configuration_profiles/{profile_uuid}request/response + service dispatch (UpdateMDMConfigProfile). - Implemented per-platform update flows (service + MySQL) for Apple config profiles, Apple declarations, Windows profiles, and Android profiles.
- Added extensive service-layer and datastore integration tests for update behaviors (authz, licensing, OS-update restrictions, labels, Fleet vars, and tracking).
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/handler.go | Registers the new PATCH route for configuration profile updates. |
| server/service/mdm.go | Adds request decoding, endpoint handler, and service dispatch (UpdateMDMConfigProfile). |
| server/fleet/service.go | Extends the Service interface with UpdateMDMConfigProfile. |
| server/service/apple_mdm.go | Refactors create validation and adds Apple profile/declaration update implementations + edit activity logging. |
| server/service/windows_mdm_profiles.go | Refactors create validation and adds Windows profile update implementation + edit activity logging. |
| server/datastore/mysql/apple_mdm.go | Adds UpdateMDMAppleConfigProfile datastore method (in-place update + labels + variables). |
| server/datastore/mysql/microsoft_mdm.go | Adds UpdateMDMWindowsConfigProfile datastore method (in-place update + labels + vars + OS-update tracking reconciliation). |
| server/datastore/mysql/android.go | Adds UpdateMDMAndroidConfigProfile datastore method (in-place update + labels). |
| server/fleet/datastore.go | Extends the Datastore interface with update methods for Apple/Windows/Android profiles. |
| server/fleet/activities.go | Adds ActivityTypeEditedConfigurationProfile activity type. |
| server/mock/datastore_mock.go | Mocks new datastore update methods. |
| server/mock/service/service_mock.go | Mocks new service method UpdateMDMConfigProfile. |
| server/service/mdm_test.go | Adds decode-request tests and dispatch coverage for the new endpoint/service routing. |
| server/service/apple_mdm_test.go | Adds service tests for updating Apple config profiles + declarations. |
| server/service/windows_mdm_profiles_test.go | Adds service tests for updating Windows profiles. |
| server/datastore/mysql/apple_mdm_test.go | Adds integration tests for Apple profile update semantics (content/name/labels/checksum). |
| server/datastore/mysql/microsoft_mdm_test.go | Adds integration tests for Windows profile update semantics (content/labels/vars/tracking). |
| server/datastore/mysql/android_test.go | Adds integration tests for Android profile update semantics (content/labels). |
Files excluded by content exclusion policy (1)
- changes/48342-edit-config-profile-endpoint
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+289
to
+332
| // UpdateMDMAppleConfigProfile updates an existing configuration profile's | ||
| // contents (if cp.Mobileconfig is non-empty) and/or label targeting in | ||
| // place, preserving its ProfileUUID. | ||
| func (ds *Datastore) UpdateMDMAppleConfigProfile(ctx context.Context, cp fleet.MDMAppleConfigProfile, usesFleetVars []fleet.FleetVarName) (*fleet.MDMAppleConfigProfile, error) { | ||
| err := ds.withTx(ctx, func(tx sqlx.ExtContext) error { | ||
| var existing struct { | ||
| Identifier string `db:"identifier"` | ||
| } | ||
| err := sqlx.GetContext(ctx, tx, &existing, | ||
| `SELECT identifier FROM mdm_apple_configuration_profiles WHERE profile_uuid = ?`, cp.ProfileUUID) | ||
| if err != nil { | ||
| if err == sql.ErrNoRows { | ||
| return ctxerr.Wrap(ctx, notFound("MDMAppleConfigProfile").WithName(cp.ProfileUUID)) | ||
| } | ||
| return ctxerr.Wrap(ctx, err, "get existing apple config profile") | ||
| } | ||
| if existing.Identifier != cp.Identifier { | ||
| return ctxerr.Wrap(ctx, &fleet.BadRequestError{ | ||
| Message: "The new profile's PayloadIdentifier must match the existing profile's.", | ||
| }) | ||
| } | ||
|
|
||
| if len(cp.Mobileconfig) > 0 { | ||
| // name is allowed to change along with the content, matching the | ||
| // same identifier-keyed upsert convention GitOps uses. | ||
| stmt := ` | ||
| UPDATE mdm_apple_configuration_profiles | ||
| SET mobileconfig = ?, checksum = UNHEX(MD5(?)), name = ?, uploaded_at = CURRENT_TIMESTAMP(), secrets_updated_at = ? | ||
| WHERE profile_uuid = ? AND identifier = ?` | ||
| res, err := tx.ExecContext(ctx, stmt, cp.Mobileconfig, cp.Mobileconfig, cp.Name, cp.SecretsUpdatedAt, cp.ProfileUUID, cp.Identifier) | ||
| if err != nil { | ||
| switch { | ||
| case IsDuplicate(err): | ||
| return ctxerr.Wrap(ctx, formatErrorDuplicateConfigProfile(err, &cp)) | ||
| default: | ||
| return ctxerr.Wrap(ctx, err, "updating apple mdm config profile contents") | ||
| } | ||
| } | ||
| if aff, _ := res.RowsAffected(); aff == 0 { | ||
| return ctxerr.Wrap(ctx, notFound("MDMAppleConfigProfile").WithName(cp.ProfileUUID)) | ||
| } | ||
| } | ||
|
|
||
| labels := make([]fleet.ConfigurationProfileLabel, 0, len(cp.LabelsIncludeAll)+len(cp.LabelsIncludeAny)+len(cp.LabelsExcludeAny)) |
| // that the uploaded profile's identifier/name match the existing profile's, | ||
| // then performs the atomic datastore update and logs the edit activity. | ||
| func (svc *Service) updateMDMAppleConfigProfile(ctx context.Context, profileUUID string, profile []byte, labelsInclude []string, labelsMembershipMode fleet.MDMLabelsMode, labelsExcludeAny []string) error { | ||
| // first we perform a perform basic authz check |
| // unrelated one (this mirrors how GitOps itself treats a Windows profile | ||
| // rename: as a delete-then-insert, not an in-place update). | ||
| func (svc *Service) updateMDMWindowsConfigProfile(ctx context.Context, profileUUID string, profile []byte, labelsInclude []string, labelsMembershipMode fleet.MDMLabelsMode, labelsExcludeAny []string) error { | ||
| // first we perform a perform basic authz check |
Contributor
WalkthroughAdds a PATCH endpoint for editing existing Apple Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
server/service/mdm.go (1)
2178-2207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated label-validation logic; inconsistent
defaulthandling vs.parseAndValidateAndroidConfigProfile.This labels-only branch re-implements the license check,
fleet.LabelOverlapcheck, andvalidateProfileLabelSetscall already present inparseAndValidateAndroidConfigProfile(lines ~2088-2107). The duplicatedswitch labelsMembershipModehere (2200-2205) lacks thedefaultfallback that the shared function has, so if this mode ever takes on a value other thanLabelsIncludeAll/LabelsIncludeAny, the resolvedincludeLabelswould be silently discarded here but not in the create path.Consider extracting the shared "validate + resolve label sets" logic into a single helper used by both code paths to avoid this kind of drift.
♻️ Sketch of a shared helper
// resolveLabelScoping runs the shared label-scoping validation (license // check, overlap check, and label-set resolution) used by both create and // labels-only update paths, and applies the result to cp according to mode. func (svc *Service) resolveLabelScoping(ctx context.Context, teamID *uint, labelsInclude, labelsExcludeAny []string, mode fleet.MDMLabelsMode) (includeAll, includeAny, excludeAny []fleet.ConfigurationProfileLabel, err error) { lic, _ := license.FromContext(ctx) if len(labelsInclude) > 0 || len(labelsExcludeAny) > 0 { if lic == nil || !lic.IsPremium() { return nil, nil, nil, ctxerr.Wrap(ctx, fleet.NewLicenseErrorWithCause(fleet.ConfigProfileLabelScopingPremiumCauseMsg), "checking license for profile label scoping") } } if overlap := fleet.LabelOverlap(labelsInclude, labelsExcludeAny); overlap != "" { return nil, nil, nil, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("labels", fmt.Sprintf("label %q cannot appear in both include and exclude lists", overlap))) } includeLabels, excludeLabels, err := svc.validateProfileLabelSets(ctx, teamID, labelsInclude, labelsExcludeAny) if err != nil { return nil, nil, nil, ctxerr.Wrap(ctx, err, "validating labels") } switch mode { case fleet.LabelsIncludeAny: return nil, includeLabels, excludeLabels, nil default: return includeLabels, nil, excludeLabels, nil } }🤖 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/mdm.go` around lines 2178 - 2207, Consolidate the duplicated license, overlap, and label-set validation used by parseAndValidateAndroidConfigProfile and the labels-only update branch into one shared helper, such as resolveLabelScoping. Have it resolve membership modes with LabelsIncludeAny explicitly and a default fallback to include-all, then use its returned label sets to populate MDMAndroidConfigProfile in both paths while preserving existing error wrapping.server/service/apple_mdm_test.go (1)
955-1007: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for label preservation on content-only updates.
newExistingProfile/newExistingDeclarationnever populateLabelsIncludeAll/Any/ExcludeAny, so none of these content-only-update subtests would catch labels being unintentionally cleared (see the corresponding comment onupdateMDMAppleConfigProfile/updateMDMAppleDeclarationinapple_mdm.go). Once that behavior is confirmed/fixed, add a case whereexistingalready has labels and assert they survive a content-only update.Also applies to: 1336-1430
🤖 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 955 - 1007, Add regression coverage for label preservation in the content-only update tests around UpdateMDMConfigProfile and the corresponding declaration update cases: initialize existing profiles/declarations with non-empty LabelsIncludeAll, LabelsIncludeAny, and LabelsExcludeAny, perform the update, and assert all label fields remain unchanged. If assertions fail, update updateMDMAppleConfigProfile and updateMDMAppleDeclaration to retain existing labels when applying content-only changes.
🤖 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 297-330: The final GetMDMAppleConfigProfile read after the update
may use a stale replica. Ensure that read is routed to the primary by wrapping
its context with ctxdb.RequirePrimary(ctx, true), or reuse the already updated
transaction result; update the relevant write flow in the MDM Apple
configuration profile method.
In `@server/service/apple_mdm.go`:
- Around line 1062-1107: The content-update branch of updateMDMAppleDeclaration
loses existing label assignments when a content-only PATCH omits labels. Before
calling parseAndValidateAppleDeclaration, preserve existing.LabelsIncludeAll or
existing.LabelsIncludeAny according to labelsMembershipMode and
existing.LabelsExcludeAny whenever the corresponding request values are omitted,
while still honoring explicitly supplied labels and membership-mode changes.
- Around line 1616-1658: Content-only PATCH requests clear existing label
targeting because omitted label fields remain nil and are treated as empty
updates. In updateMDMConfigProfileRequest.DecodeRequest, preserve existing
LabelsIncludeAll, LabelsIncludeAny, and LabelsExcludeAny when the corresponding
multipart label fields are omitted; only replace them when labels were
explicitly provided, so the update path around
parseAndValidateAppleConfigProfile retains associations.
In `@server/service/mdm_test.go`:
- Around line 3702-3707: Update the newExistingProfile helper to represent
global profiles with a nil TeamID when teamID is 0, while retaining a pointer
for nonzero team IDs; ensure the returned fleet.MDMAndroidConfigProfile matches
the analogous MDM model’s no-team representation so authorization and activity
tests use the correct scope.
In `@server/service/mdm.go`:
- Around line 1879-1921: Preserve whether each label field was omitted in
updateMDMConfigProfileRequest.DecodeRequest instead of converting omitted fields
to empty slices. Add tri-state/presence tracking for labels_include_all,
labels_include_any, and labels_exclude_any, then update
UpdateMDMAndroidConfigProfile (and related label-association handling such as
batchSetProfileLabelAssociationsDB) to retain existing associations when all
label fields are omitted while still applying explicit empty values as
replacements.
---
Nitpick comments:
In `@server/service/apple_mdm_test.go`:
- Around line 955-1007: Add regression coverage for label preservation in the
content-only update tests around UpdateMDMConfigProfile and the corresponding
declaration update cases: initialize existing profiles/declarations with
non-empty LabelsIncludeAll, LabelsIncludeAny, and LabelsExcludeAny, perform the
update, and assert all label fields remain unchanged. If assertions fail, update
updateMDMAppleConfigProfile and updateMDMAppleDeclaration to retain existing
labels when applying content-only changes.
In `@server/service/mdm.go`:
- Around line 2178-2207: Consolidate the duplicated license, overlap, and
label-set validation used by parseAndValidateAndroidConfigProfile and the
labels-only update branch into one shared helper, such as resolveLabelScoping.
Have it resolve membership modes with LabelsIncludeAny explicitly and a default
fallback to include-all, then use its returned label sets to populate
MDMAndroidConfigProfile in both paths while preserving existing error wrapping.
🪄 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: 1f1a026d-0130-42f0-87d5-dbce2aa042c3
📒 Files selected for processing (19)
changes/48342-edit-config-profile-endpointserver/datastore/mysql/android.goserver/datastore/mysql/android_test.goserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_test.goserver/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.goserver/fleet/activities.goserver/fleet/datastore.goserver/fleet/service.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/apple_mdm.goserver/service/apple_mdm_test.goserver/service/handler.goserver/service/mdm.goserver/service/mdm_test.goserver/service/windows_mdm_profiles.goserver/service/windows_mdm_profiles_test.go
Comment on lines
+297
to
+330
| err := sqlx.GetContext(ctx, tx, &existing, | ||
| `SELECT identifier FROM mdm_apple_configuration_profiles WHERE profile_uuid = ?`, cp.ProfileUUID) | ||
| if err != nil { | ||
| if err == sql.ErrNoRows { | ||
| return ctxerr.Wrap(ctx, notFound("MDMAppleConfigProfile").WithName(cp.ProfileUUID)) | ||
| } | ||
| return ctxerr.Wrap(ctx, err, "get existing apple config profile") | ||
| } | ||
| if existing.Identifier != cp.Identifier { | ||
| return ctxerr.Wrap(ctx, &fleet.BadRequestError{ | ||
| Message: "The new profile's PayloadIdentifier must match the existing profile's.", | ||
| }) | ||
| } | ||
|
|
||
| if len(cp.Mobileconfig) > 0 { | ||
| // name is allowed to change along with the content, matching the | ||
| // same identifier-keyed upsert convention GitOps uses. | ||
| stmt := ` | ||
| UPDATE mdm_apple_configuration_profiles | ||
| SET mobileconfig = ?, checksum = UNHEX(MD5(?)), name = ?, uploaded_at = CURRENT_TIMESTAMP(), secrets_updated_at = ? | ||
| WHERE profile_uuid = ? AND identifier = ?` | ||
| res, err := tx.ExecContext(ctx, stmt, cp.Mobileconfig, cp.Mobileconfig, cp.Name, cp.SecretsUpdatedAt, cp.ProfileUUID, cp.Identifier) | ||
| if err != nil { | ||
| switch { | ||
| case IsDuplicate(err): | ||
| return ctxerr.Wrap(ctx, formatErrorDuplicateConfigProfile(err, &cp)) | ||
| default: | ||
| return ctxerr.Wrap(ctx, err, "updating apple mdm config profile contents") | ||
| } | ||
| } | ||
| if aff, _ := res.RowsAffected(); aff == 0 { | ||
| return ctxerr.Wrap(ctx, notFound("MDMAppleConfigProfile").WithName(cp.ProfileUUID)) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Relevant symbols in server/datastore/mysql/apple_mdm.go ==\n'
ast-grep outline server/datastore/mysql/apple_mdm.go --view expanded || true
printf '\n== Find schema/migration definitions for apple MDM association tables ==\n'
rg -n --hidden --glob '!**/node_modules/**' \
'mdm_apple_configuration_profiles_labels|mdm_apple_configuration_profiles_variables|mdm_apple_configuration_profiles' \
server migrations . || true
printf '\n== Find reader(ctx) implementation and GetMDMAppleConfigProfile ==\n'
rg -n --hidden --glob '!**/node_modules/**' \
'func \(.*\) reader\(ctx context.Context\)|GetMDMAppleConfigProfile|reader\(ctx\)' \
server datastore . || true
printf '\n== Inspect likely schema files around matching lines ==\n'
for f in $(rg -l --hidden --glob '!**/node_modules/**' 'mdm_apple_configuration_profiles_labels|mdm_apple_configuration_profiles_variables'); do
echo "--- $f"
wc -l "$f"
doneRepository: fleetdm/fleet
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== reader(ctx) and GetMDMAppleConfigProfile =='
rg -n -A30 -B10 'func \(ds \*Datastore\) reader\(ctx context.Context\)|func \(ds \*Datastore\) GetMDMAppleConfigProfile\(ctx context.Context, profileUUID string\)|func \(ds \*Datastore\) GetMDMAppleConfigProfileByDeprecatedID\(ctx context.Context, profileID uint\)' server/datastore/mysql/*.go
echo
echo '== schema foreign keys for apple profile labels/variables =='
sed -n '1885,1960p' server/datastore/mysql/schema.sql
echo
echo '== any reader routing logic in datastore package =='
rg -n -A20 -B10 'reader\(|writer\(|replica|lagging|read replica|primary' server/datastore/mysql/*.go server/datastore/*.go server/*.go | head -n 250Repository: fleetdm/fleet
Length of output: 33185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== UpdateMDMAppleConfigProfile and post-commit read =='
sed -n '292,380p' server/datastore/mysql/apple_mdm.go
echo
echo '== Association updates in same function =='
sed -n '330,372p' server/datastore/mysql/apple_mdm.go
echo
echo '== Delete path for profiles =='
sed -n '605,690p' server/datastore/mysql/apple_mdm.go
echo
echo '== Any primary-required context around this flow =='
rg -n -A3 -B3 'RequirePrimary|PrimaryRequired|GetMDMAppleConfigProfile\(ctx' server/datastore/mysql/*.go server/*.goRepository: fleetdm/fleet
Length of output: 24914
Route the final read to primary. ds.reader(ctx) defaults to the replica, so the GetMDMAppleConfigProfile call here can return stale data immediately after this write. Wrap that read with ctxdb.RequirePrimary(ctx, true) or reuse the transaction result.
🤖 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 297 - 330, The final
GetMDMAppleConfigProfile read after the update may use a stale replica. Ensure
that read is routed to the primary by wrapping its context with
ctxdb.RequirePrimary(ctx, true), or reuse the already updated transaction
result; update the relevant write flow in the MDM Apple configuration profile
method.
Comment on lines
+1062
to
+1107
| var ( | ||
| decl *fleet.MDMAppleDeclaration | ||
| varNames []fleet.FleetVarName | ||
| ) | ||
| if len(profile) > 0 { | ||
| decl, varNames, _, err = svc.parseAndValidateAppleDeclaration(ctx, teamID, existing.Name, profile, labelsInclude, labelsMembershipMode, labelsExcludeAny) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if decl.Identifier != existing.Identifier { | ||
| return fleet.NewInvalidArgumentError("profile", | ||
| "The new profile's Identifier must match the existing profile's.").WithStatus(http.StatusBadRequest) | ||
| } | ||
| } else { | ||
| // no new content -- only labels are being changed. | ||
| lic, err := svc.License(ctx) | ||
| if err != nil { | ||
| return ctxerr.Wrap(ctx, err, "checking license") | ||
| } | ||
| if len(labelsInclude) > 0 || len(labelsExcludeAny) > 0 { | ||
| if lic == nil || !lic.IsPremium() { | ||
| return ctxerr.Wrap(ctx, fleet.NewLicenseErrorWithCause(fleet.ConfigProfileLabelScopingPremiumCauseMsg), "checking license for declaration profile label scoping") | ||
| } | ||
| } | ||
| if overlap := fleet.LabelOverlap(labelsInclude, labelsExcludeAny); overlap != "" { | ||
| return ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("labels", fmt.Sprintf("label %q cannot appear in both include and exclude lists", overlap))) | ||
| } | ||
| includeLabels, excludeLabels, err := svc.validateDeclarationLabelSets(ctx, teamID, labelsInclude, labelsExcludeAny) | ||
| if err != nil { | ||
| return ctxerr.Wrap(ctx, err, "validating labels") | ||
| } | ||
| decl = &fleet.MDMAppleDeclaration{ | ||
| Name: existing.Name, | ||
| Identifier: existing.Identifier, | ||
| TeamID: existing.TeamID, | ||
| RawJSON: existing.RawJSON, | ||
| SecretsUpdatedAt: existing.SecretsUpdatedAt, | ||
| } | ||
| switch labelsMembershipMode { | ||
| case fleet.LabelsIncludeAll: | ||
| decl.LabelsIncludeAll = includeLabels | ||
| case fleet.LabelsIncludeAny: | ||
| decl.LabelsIncludeAny = includeLabels | ||
| } | ||
| decl.LabelsExcludeAny = excludeLabels | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Same labels-wipe pattern applies here for declarations.
updateMDMAppleDeclaration's content branch (lines 1066-1074) calls parseAndValidateAppleDeclaration with the raw labelsInclude/labelsExcludeAny, with no fallback to existing.LabelsIncludeAll/Any/ExcludeAny when those are omitted from a content-only PATCH — mirroring the issue flagged on updateMDMAppleConfigProfile (lines 1616-1658). Same root cause, same fix needed here.
🤖 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.go` around lines 1062 - 1107, The content-update
branch of updateMDMAppleDeclaration loses existing label assignments when a
content-only PATCH omits labels. Before calling
parseAndValidateAppleDeclaration, preserve existing.LabelsIncludeAll or
existing.LabelsIncludeAny according to labelsMembershipMode and
existing.LabelsExcludeAny whenever the corresponding request values are omitted,
while still honoring explicitly supplied labels and membership-mode changes.
Comment on lines
+1616
to
+1658
| var cp *fleet.MDMAppleConfigProfile | ||
| var varNames []fleet.FleetVarName | ||
| if len(profile) > 0 { | ||
| cp, varNames, _, err = svc.parseAndValidateAppleConfigProfile(ctx, teamID, profile, labelsInclude, labelsMembershipMode, labelsExcludeAny) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if cp.Identifier != existing.Identifier { | ||
| return fleet.NewInvalidArgumentError("profile", | ||
| "The new profile's PayloadIdentifier must match the existing profile's.").WithStatus(http.StatusBadRequest) | ||
| } | ||
| } else { | ||
| // no new content -- only labels are being changed. | ||
| lic, err := svc.License(ctx) | ||
| if err != nil { | ||
| return ctxerr.Wrap(ctx, err, "checking license") | ||
| } | ||
| if len(labelsInclude) > 0 || len(labelsExcludeAny) > 0 { | ||
| if lic == nil || !lic.IsPremium() { | ||
| return ctxerr.Wrap(ctx, fleet.NewLicenseErrorWithCause(fleet.ConfigProfileLabelScopingPremiumCauseMsg), "checking license for profile label scoping") | ||
| } | ||
| } | ||
| if overlap := fleet.LabelOverlap(labelsInclude, labelsExcludeAny); overlap != "" { | ||
| return ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("labels", fmt.Sprintf("label %q cannot appear in both include and exclude lists", overlap))) | ||
| } | ||
| includeLabels, excludeLabels, err := svc.validateProfileLabelSets(ctx, &teamID, labelsInclude, labelsExcludeAny) | ||
| if err != nil { | ||
| return ctxerr.Wrap(ctx, err, "validating labels") | ||
| } | ||
| cp = &fleet.MDMAppleConfigProfile{ | ||
| Identifier: existing.Identifier, | ||
| Name: existing.Name, | ||
| TeamID: existing.TeamID, | ||
| } | ||
| switch labelsMembershipMode { | ||
| case fleet.LabelsIncludeAll: | ||
| cp.LabelsIncludeAll = includeLabels | ||
| case fleet.LabelsIncludeAny: | ||
| cp.LabelsIncludeAny = includeLabels | ||
| } | ||
| cp.LabelsExcludeAny = excludeLabels | ||
| } | ||
| cp.ProfileUUID = profileUUID |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Content-only profile PATCHes still clear label targeting
updateMDMConfigProfileRequest.DecodeRequest leaves omitted multipart label fields as nil, and the update path treats an empty label set as “clear all labels.” A content-only PATCH can therefore drop existing label associations unless the request layer preserves existing.Labels* when labels aren’t included.
🤖 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.go` around lines 1616 - 1658, Content-only PATCH
requests clear existing label targeting because omitted label fields remain nil
and are treated as empty updates. In
updateMDMConfigProfileRequest.DecodeRequest, preserve existing LabelsIncludeAll,
LabelsIncludeAny, and LabelsExcludeAny when the corresponding multipart label
fields are omitted; only replace them when labels were explicitly provided, so
the update path around parseAndValidateAppleConfigProfile retains associations.
Comment on lines
+3702
to
+3707
| newExistingProfile := func(name string, teamID uint) *fleet.MDMAndroidConfigProfile { | ||
| return &fleet.MDMAndroidConfigProfile{ | ||
| ProfileUUID: "g" + uuid.NewString(), | ||
| Name: name, | ||
| TeamID: &teamID, | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Represent global profiles with a nil TeamID.
newExistingProfile(..., 0) creates TeamID: &teamID, so supposedly global profiles are represented as team 0. Fleet’s analogous MDM model documents nil as the no-team value (server/fleet/apple_mdm.go:847-882), and this can make authorization/activity tests exercise the wrong scope.
func newExistingProfile(name string, teamID uint) *fleet.MDMAndroidConfigProfile {
+ var teamIDPtr *uint
+ if teamID != 0 {
+ teamIDPtr = &teamID
+ }
return &fleet.MDMAndroidConfigProfile{
ProfileUUID: "g" + uuid.NewString(),
Name: name,
- TeamID: &teamID,
+ TeamID: teamIDPtr,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| newExistingProfile := func(name string, teamID uint) *fleet.MDMAndroidConfigProfile { | |
| return &fleet.MDMAndroidConfigProfile{ | |
| ProfileUUID: "g" + uuid.NewString(), | |
| Name: name, | |
| TeamID: &teamID, | |
| } | |
| newExistingProfile := func(name string, teamID uint) *fleet.MDMAndroidConfigProfile { | |
| var teamIDPtr *uint | |
| if teamID != 0 { | |
| teamIDPtr = &teamID | |
| } | |
| return &fleet.MDMAndroidConfigProfile{ | |
| ProfileUUID: "g" + uuid.NewString(), | |
| Name: name, | |
| TeamID: teamIDPtr, | |
| } | |
| } |
🤖 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/mdm_test.go` around lines 3702 - 3707, Update the
newExistingProfile helper to represent global profiles with a nil TeamID when
teamID is 0, while retaining a pointer for nonzero team IDs; ensure the returned
fleet.MDMAndroidConfigProfile matches the analogous MDM model’s no-team
representation so authorization and activity tests use the correct scope.
Comment on lines
+1879
to
+1921
| func (updateMDMConfigProfileRequest) DecodeRequest(ctx context.Context, r *http.Request) (any, error) { | ||
| decoded := updateMDMConfigProfileRequest{} | ||
|
|
||
| profileUUID, ok := mux.Vars(r)["profile_uuid"] | ||
| if !ok || profileUUID == "" { | ||
| return nil, &fleet.BadRequestError{Message: "profile_uuid is required"} | ||
| } | ||
| decoded.ProfileUUID = profileUUID | ||
|
|
||
| err := parseMultipartForm(ctx, r, platform_http.MaxMultipartFormSize) | ||
| if err != nil { | ||
| return nil, &fleet.BadRequestError{ | ||
| Message: "failed to parse multipart form", | ||
| InternalErr: err, | ||
| } | ||
| } | ||
|
|
||
| // profile file is optional on update -- labels may be edited without | ||
| // replacing the profile contents | ||
| if fhs, ok := r.MultipartForm.File["profile"]; ok && len(fhs) > 0 { | ||
| decoded.Profile = fhs[0] | ||
| if decoded.Profile.Size > fleet.MaxProfileSize { | ||
| return nil, fleet.NewInvalidArgumentError("mdm", "maximum configuration profile file size is 1 MB") | ||
| } | ||
| } | ||
|
|
||
| // add labels | ||
| var existsInclAll, existsInclAny bool | ||
| decoded.LabelsIncludeAll, existsInclAll = r.MultipartForm.Value[string(fleet.LabelsIncludeAll)] | ||
| decoded.LabelsIncludeAny, existsInclAny = r.MultipartForm.Value[string(fleet.LabelsIncludeAny)] | ||
| decoded.LabelsExcludeAny = r.MultipartForm.Value[string(fleet.LabelsExcludeAny)] | ||
|
|
||
| if existsInclAll && existsInclAny { | ||
| return nil, &fleet.BadRequestError{Message: `Only one of "labels_include_all" or "labels_include_any" can be included.`} | ||
| } | ||
|
|
||
| includeLabels := append(decoded.LabelsIncludeAll, decoded.LabelsIncludeAny...) //nolint:gocritic | ||
| if overlap := fleet.LabelOverlap(includeLabels, decoded.LabelsExcludeAny); overlap != "" { | ||
| return nil, &fleet.BadRequestError{Message: fmt.Sprintf(`Label %q cannot appear in both include and exclude lists.`, overlap)} | ||
| } | ||
|
|
||
| return &decoded, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant request decoder and endpoint logic.
file="server/service/mdm.go"
echo '--- DecodeRequest and endpoint context ---'
sed -n '1870,1985p' "$file"
echo
echo '--- Android update path context ---'
sed -n '2130,2215p' "$file"
echo
echo '--- Search for related update flows and tests ---'
rg -n "LabelsIncludeAll|LabelsIncludeAny|LabelsExcludeAny|UpdateMDMAndroidConfigProfile|UpdateMDMConfigProfile|batchSetProfileLabelAssociationsDB|profile content update" server/service -SRepository: fleetdm/fleet
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="server/service/mdm.go"
tests="server/service/mdm_test.go"
echo '--- UpdateMDMConfigProfile implementation ---'
sed -n '1997,2215p' "$file"
echo
echo '--- Android parse/validate helper ---'
rg -n "func .*parseAndValidateAndroidConfigProfile|parseAndValidateAndroidConfigProfile" "$file"
sed -n '2680,2865p' "$file"
echo
echo '--- UpdateMDMAndroidConfigProfile tests ---'
sed -n '3701,3965p' "$tests"
echo
echo '--- UpdateMDMConfigProfile decode/dispatch tests ---'
sed -n '1693,1848p' "$tests"Repository: fleetdm/fleet
Length of output: 35092
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="server/service/mdm.go"
tests="server/service/mdm_test.go"
wtests="server/service/windows_mdm_profiles_test.go"
echo '--- Apple/Windows update paths ---'
sed -n '1997,2285p' "$file"
echo
echo '--- Apple/Windows update tests around content-only edits ---'
rg -n "profile content update|labels-only update|without labels|preserve|scop" "$tests" "$wtests" -S
echo
echo '--- Windows update tests section ---'
sed -n '580,780p' "$wtests"
echo
echo '--- Any explicit assertions about existing labels surviving content-only edits ---'
rg -n "LabelsIncludeAll|LabelsIncludeAny|LabelsExcludeAny" "$tests" "$wtests" -S | sed -n '1,120p'Repository: fleetdm/fleet
Length of output: 40711
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Datastore update implementations for Android config profiles ---'
rg -n "UpdateMDMAndroidConfigProfile\(|batchSetProfileLabelAssociationsDB|SetProfileLabelAssociations|MDMAndroidConfigProfile" server -S
echo
echo '--- Likely datastore implementation region ---'
file=$(rg -l "batchSetProfileLabelAssociationsDB|UpdateMDMAndroidConfigProfile" server | head -n 5 | tr '\n' ' ')
for f in $file; do
echo "### $f"
sed -n '1,260p' "$f" | sed -n '1,220p'
echo
doneRepository: fleetdm/fleet
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Android datastore update implementation ---'
sed -n '728,810p' server/datastore/mysql/android.go
echo
echo '--- Shared label association helper ---'
sed -n '1716,1835p' server/datastore/mysql/mdm.go
echo
echo '--- Android datastore update tests around labels ---'
sed -n '945,1115p' server/datastore/mysql/android_test.goRepository: fleetdm/fleet
Length of output: 15682
Preserve omitted label fields on Android profile updates. DecodeRequest drops whether labels_include_* / labels_exclude_any were absent, so UpdateMDMConfigProfile sends empty slices on file-only PATCHes. On Android, UpdateMDMAndroidConfigProfile treats an empty label set as a full replace, and batchSetProfileLabelAssociationsDB deletes the existing associations, so a content-only edit can silently un-scope the profile to all hosts. Keep a tri-state for the label fields, or reuse the existing labels when the request omits them.
🤖 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/mdm.go` around lines 1879 - 1921, Preserve whether each label
field was omitted in updateMDMConfigProfileRequest.DecodeRequest instead of
converting omitted fields to empty slices. Add tri-state/presence tracking for
labels_include_all, labels_include_any, and labels_exclude_any, then update
UpdateMDMAndroidConfigProfile (and related label-association handling such as
batchSetProfileLabelAssociationsDB) to retain existing associations when all
label fields are omitted while still applying explicit empty values as
replacements.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.Testing
Added/updated automated tests
QA'd all new/changed functionality manually
Summary by CodeRabbit