Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changes/40322-make-ddm-name-check-case-insensitive
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Fixed an issue where sending a different cased display name for a DDM profile via the batch endpoint, would result in recreating the DDM profile triggering a resend.
35 changes: 30 additions & 5 deletions server/datastore/mysql/apple_mdm.go
Original file line number Diff line number Diff line change
Expand Up @@ -5108,8 +5108,9 @@ func (ds *Datastore) batchSetMDMAppleDeclarations(ctx context.Context, tx sqlx.E
incomingNames := make([]string, len(incomingDeclarations))
incomingDeclarationsMap := make(map[string]*fleet.MDMAppleDeclaration, len(incomingDeclarations))
for i, p := range incomingDeclarations {
incomingNames[i] = p.Name
incomingDeclarationsMap[p.Name] = p
name := strings.ToLower(p.Name)
incomingNames[i] = name
incomingDeclarationsMap[name] = p
Comment on lines +5111 to +5113
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Reject duplicate declaration names after normalization.

Lowercasing the key here makes two incoming declarations like Foo and foo collapse into one incomingDeclarationsMap entry. The rest of the flow still iterates both original declarations, so one upsert can overwrite the other while only one declaration's labels/UUID bookkeeping survives. Please validate and return an error when two incoming names normalize to the same value instead of letting the last one win.

Proposed fix
 	incomingNames := make([]string, len(incomingDeclarations))
 	incomingDeclarationsMap := make(map[string]*fleet.MDMAppleDeclaration, len(incomingDeclarations))
 	for i, p := range incomingDeclarations {
-		name := strings.ToLower(p.Name)
-		incomingNames[i] = name
-		incomingDeclarationsMap[name] = p
+		name := strings.ToLower(p.Name)
+		if existing, ok := incomingDeclarationsMap[name]; ok {
+			return false, &fleet.BadRequestError{
+				Message: fmt.Sprintf(
+					"declaration names %q and %q differ only by letter case; declaration names must be unique",
+					existing.Name, p.Name,
+				),
+			}
+		}
+		incomingNames[i] = name
+		incomingDeclarationsMap[name] = p
 	}
📝 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
name := strings.ToLower(p.Name)
incomingNames[i] = name
incomingDeclarationsMap[name] = p
name := strings.ToLower(p.Name)
if existing, ok := incomingDeclarationsMap[name]; ok {
return false, &fleet.BadRequestError{
Message: fmt.Sprintf(
"declaration names %q and %q differ only by letter case; declaration names must be unique",
existing.Name, p.Name,
),
}
}
incomingNames[i] = name
incomingDeclarationsMap[name] = p
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server/datastore/mysql/apple_mdm.go` around lines 5111 - 5113, When building
incomingDeclarationsMap using normalized keys (strings.ToLower(p.Name)), detect
if a normalized name already exists and reject the request instead of
overwriting: inside the loop where incomingNames[i] and
incomingDeclarationsMap[name] are assigned, check if incomingDeclarationsMap
already has an entry for that normalized name and return an error (including
both conflicting original names/p.Name values) so duplicate declarations like
"Foo" and "foo" are rejected; update the function that contains
incomingNames/incomingDeclarationsMap to perform this validation before
continuing with upserts.

}

teamIDOrZero := ds.teamIDPtrToUint(tmID)
Expand All @@ -5121,8 +5122,9 @@ func (ds *Datastore) batchSetMDMAppleDeclarations(ctx context.Context, tx sqlx.E
// figure out which declarations we should not delete, and put those into keepNames list
keepNames := make([]string, 0, len(existingDecls)+len(fleetmdm.ListFleetReservedMacOSDeclarationNames()))
for _, p := range existingDecls {
if newP := incomingDeclarationsMap[p.Name]; newP != nil {
if newP := incomingDeclarationsMap[strings.ToLower(p.Name)]; newP != nil {
keepNames = append(keepNames, p.Name)
newP.DeclarationUUID = p.DeclarationUUID
}
}
keepNames = append(keepNames, fleetmdm.ListFleetReservedMacOSDeclarationNames()...)
Expand Down Expand Up @@ -5175,9 +5177,9 @@ func (ds *Datastore) updateDeclarationsLabelAssociations(ctx context.Context, tx
}

for _, newlyInsertedDecl := range newlyInsertedDecls {
incomingDecl, ok := incomingDeclarationsMap[newlyInsertedDecl.Name]
incomingDecl, ok := incomingDeclarationsMap[strings.ToLower(newlyInsertedDecl.Name)]
if !ok {
return false, ctxerr.Wrapf(ctx, err, "declaration %q is in the database but was not incoming",
return false, ctxerr.Errorf(ctx, "declaration %q is in the database but was not incoming",
newlyInsertedDecl.Name)
}

Expand Down Expand Up @@ -5240,6 +5242,7 @@ ON DUPLICATE KEY UPDATE
raw_json = VALUES(raw_json)
`

updatedDeclarationUUIDs := make([]string, 0, len(incomingDeclarations))
for _, d := range incomingDeclarations {
declUUID := fleet.MDMAppleDeclarationUUIDPrefix + uuid.NewString()
var result sql.Result
Expand All @@ -5253,7 +5256,29 @@ ON DUPLICATE KEY UPDATE
return false, ctxerr.Wrapf(ctx, err, "insert new/edited declaration with identifier %q", d.Identifier)
}
updatedDB = updatedDB || insertOnDuplicateDidInsertOrUpdate(result)

if insertOnDuplicateDidInsertOrUpdate(result) {
updatedDeclarationUUIDs = append(updatedDeclarationUUIDs, d.DeclarationUUID)
}
}

// If we updated any declarations, make sure to update the associated host entries with the new name
// since other changes should not trigger an update but rather a full replace, name does not.
if len(updatedDeclarationUUIDs) > 0 {
const updateHostDeclarationsName = `
UPDATE host_mdm_apple_declarations hmad
JOIN mdm_apple_declarations mad ON hmad.declaration_uuid = mad.declaration_uuid
SET hmad.declaration_name = mad.name
WHERE hmad.declaration_uuid IN (?)`
updateHostDeclarationsNameStmt, args, err := sqlx.In(updateHostDeclarationsName, updatedDeclarationUUIDs)
if err != nil {
return false, ctxerr.Wrap(ctx, err, "build query to update host declaration names")
}
if _, err := tx.ExecContext(ctx, updateHostDeclarationsNameStmt, args...); err != nil {
return false, ctxerr.Wrap(ctx, err, "update host declaration names")
}
}

return updatedDB, nil
}

Expand Down
91 changes: 91 additions & 0 deletions server/datastore/mysql/apple_mdm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ func TestMDMApple(t *testing.T) {
{"DeviceLocation", testDeviceLocation},
{"TestGetDEPAssignProfileExpiredCooldowns", testGetDEPAssignProfileExpiredCooldowns},
{"DeleteMDMAppleDeclarationByNameCancelsInstalls", testDeleteMDMAppleDeclarationByNameCancelsInstalls},
{"BatchSetMDMAppleDeclarationsCaseChange", testBatchSetMDMAppleDeclarationsCaseChange},
{"RecoveryLockPasswordSetAndGet", testRecoveryLockPasswordSetAndGet},
{"RecoveryLockPasswordBulkSet", testRecoveryLockPasswordBulkSet},
{"RecoveryLockPasswordGetNotFound", testRecoveryLockPasswordGetNotFound},
Expand Down Expand Up @@ -10127,6 +10128,96 @@ func testDeleteMDMAppleDeclarationByNameCancelsInstalls(t *testing.T, ds *Datast
})
}

func testBatchSetMDMAppleDeclarationsCaseChange(t *testing.T, ds *Datastore) {
ctx := t.Context()

SetTestABMAssets(t, ds, "fleet")

runTest := func(t *testing.T, teamID *uint) {
// Create a declaration with mixed-case name.
appleDecls := []*fleet.MDMAppleDeclaration{
declForTest("Software Update Settings", "SUS", "{}"),
}
_, err := ds.BatchSetMDMProfiles(ctx, teamID, nil, nil, appleDecls, nil, nil)
require.NoError(t, err)

// Read back and capture the declaration UUID.
profs, _, err := ds.ListMDMConfigProfiles(ctx, teamID, fleet.ListOptions{})
require.NoError(t, err)
require.Len(t, profs, 1)
origUUID := profs[0].ProfileUUID
require.Equal(t, "Software Update Settings", profs[0].Name)
origIdentifier := profs[0].Identifier

// Create two hosts and enroll them.
var opts []test.NewHostOption
if teamID != nil {
opts = append(opts, test.WithTeamID(*teamID))
}
host1 := test.NewHost(t, ds, "host1", "1"+t.Name(), "h1key"+t.Name(), "host1uuid"+t.Name(), time.Now(), opts...)
host2 := test.NewHost(t, ds, "host2", "2"+t.Name(), "h2key"+t.Name(), "host2uuid"+t.Name(), time.Now(), opts...)
nanoEnroll(t, ds, host1, false)
nanoEnroll(t, ds, host2, false)
for _, h := range []*fleet.Host{host1, host2} {
err = ds.SetOrUpdateMDMData(ctx, h.ID, false, true, "https://fleetdm.com", false, fleet.WellKnownMDMFleet, "", false)
require.NoError(t, err)
}

// Force host1 to have a verified install, host2 to have a pending install.
forceSetAppleHostDeclarationStatus(t, ds, host1.UUID, test.ToMDMAppleDecl(profs[0]), fleet.MDMOperationTypeInstall, fleet.MDMDeliveryVerified)
forceSetAppleHostDeclarationStatus(t, ds, host2.UUID, test.ToMDMAppleDecl(profs[0]), fleet.MDMOperationTypeInstall, "")

// Batch-set again with only the name case changed (lowercase).
appleDecls = []*fleet.MDMAppleDeclaration{
declForTest("software update settings", "SUS", "{}"),
}
_, err = ds.BatchSetMDMProfiles(ctx, teamID, nil, nil, appleDecls, nil, nil)
require.NoError(t, err)

// Sub-test 1: case change preserves declaration UUID and updates name.
t.Run("preserves declaration UUID", func(t *testing.T) {
profs, _, err := ds.ListMDMConfigProfiles(ctx, teamID, fleet.ListOptions{})
require.NoError(t, err)
require.Len(t, profs, 1, "expected exactly 1 declaration, got %d (duplicate created)", len(profs))
require.Equal(t, origUUID, profs[0].ProfileUUID, "declaration UUID should not change on name case change")
require.Equal(t, "software update settings", profs[0].Name, "name should be updated to new case")
require.Equal(t, origIdentifier, profs[0].Identifier, "identifier should be unchanged")
})

// Sub-test 2: verified install is NOT turned into a remove+reinstall.
t.Run("verified install not disrupted", func(t *testing.T) {
assertHostProfileOpStatus(t, ds, host1.UUID,
hostProfileOpStatus{origUUID, fleet.MDMDeliveryVerified, fleet.MDMOperationTypeInstall})
})

// Sub-test 3: pending install is NOT turned into a remove.
t.Run("pending install not disrupted", func(t *testing.T) {
assertHostProfileOpStatus(t, ds, host2.UUID,
hostProfileOpStatus{origUUID, fleet.MDMDeliveryPending, fleet.MDMOperationTypeInstall})
})

// Sub-test 4: host profile has updated name for the declaration.
t.Run("host profile reflects name change", func(t *testing.T) {
profs, err := ds.GetHostMDMAppleProfiles(ctx, host1.UUID)
require.NoError(t, err)
require.Len(t, profs, 1)
require.Equal(t, "software update settings", profs[0].Name, "host profile should reflect declaration name change")
})
}

t.Run("No team", func(t *testing.T) {
runTest(t, nil)
})

t.Run("Team", func(t *testing.T) {
team, err := ds.NewTeam(t.Context(), &fleet.Team{
Name: t.Name(),
})
require.NoError(t, err)
runTest(t, &team.ID)
})
}

func testRecoveryLockPasswordSetAndGet(t *testing.T, ds *Datastore) {
ctx := t.Context()
host := test.NewHost(t, ds, "test-host-1", "1.2.3.4", "h1key", "h1uuid", time.Now())
Expand Down
Loading