From f352dfdb763f5a6a0af26f3921f900614c64c28e Mon Sep 17 00:00:00 2001 From: Konstantin Sykulev Date: Tue, 11 Feb 2025 12:23:20 -0600 Subject: [PATCH] Updated BatchSetSoftwareInstallers to use bundle_identifier (#26252) The software titles unique key was changed to include bundle identifier in https://github.com/fleetdm/fleet/pull/25794. This caused an issue when running gitops - installing a software with the same bundle identifier but different names. https://github.com/fleetdm/fleet/issues/26226 - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) - [x] Added/updated automated tests - [x] A detailed QA plan exists on the associated ticket (if it isn't there, work with the product group's QA engineer to add it) - [x] Manual QA for all new/changed functionality --- server/datastore/mysql/software_installers.go | 48 +++++++++--- .../mysql/software_installers_test.go | 73 ++++++++++++++++--- 2 files changed, 98 insertions(+), 23 deletions(-) diff --git a/server/datastore/mysql/software_installers.go b/server/datastore/mysql/software_installers.go index 3e437668a0..4354785625 100644 --- a/server/datastore/mysql/software_installers.go +++ b/server/datastore/mysql/software_installers.go @@ -1071,13 +1071,14 @@ func (ds *Datastore) CleanupUnusedSoftwareInstallers(ctx context.Context, softwa func (ds *Datastore) BatchSetSoftwareInstallers(ctx context.Context, tmID *uint, installers []*fleet.UploadSoftwareInstallerPayload) error { const upsertSoftwareTitles = ` INSERT INTO software_titles - (name, source, browser) + (name, source, browser, bundle_identifier) VALUES %s ON DUPLICATE KEY UPDATE name = VALUES(name), source = VALUES(source), - browser = VALUES(browser) + browser = VALUES(browser), + bundle_identifier = VALUES(bundle_identifier) ` const loadSoftwareTitles = ` @@ -1085,7 +1086,7 @@ SELECT id FROM software_titles -WHERE (name, source, browser) IN (%s) +WHERE (unique_identifier, source, browser) IN (%s) ` const unsetAllInstallersFromPolicies = ` @@ -1190,7 +1191,7 @@ COALESCE(post_install_script_content_id != ? OR (post_install_script_content_id IS NULL AND ? IS NOT NULL) OR (? IS NULL AND post_install_script_content_id IS NOT NULL) , FALSE) is_metadata_modified FROM software_installers -WHERE global_or_team_id = ? AND title_id IN (SELECT id FROM software_titles WHERE name = ? AND source = ? AND browser = '') +WHERE global_or_team_id = ? AND title_id IN (SELECT id FROM software_titles WHERE unique_identifier = ? AND source = ? AND browser = '') ` const insertNewOrEditedInstaller = ` @@ -1216,7 +1217,7 @@ INSERT INTO software_installers ( install_during_setup ) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, - (SELECT id FROM software_titles WHERE name = ? AND source = ? AND browser = ''), + (SELECT id FROM software_titles WHERE unique_identifier = ? AND source = ? AND browser = ''), ?, (SELECT name FROM users WHERE id = ?), (SELECT email FROM users WHERE id = ?), ?, ?, COALESCE(?, false) ) ON DUPLICATE KEY UPDATE @@ -1245,7 +1246,7 @@ FROM WHERE global_or_team_id = ? AND -- this is guaranteed to select a single title_id, due to unique index - title_id IN (SELECT id FROM software_titles WHERE name = ? AND source = ? AND browser = '') + title_id IN (SELECT id FROM software_titles WHERE unique_identifier = ? AND source = ? AND browser = '') ` const deleteInstallerLabelsNotInList = ` @@ -1330,11 +1331,22 @@ WHERE var args []any for _, installer := range installers { - args = append(args, installer.Title, installer.Source, "") + args = append( + args, + installer.Title, + installer.Source, + "", + func() *string { + if strings.TrimSpace(installer.BundleIdentifier) != "" { + return &installer.BundleIdentifier + } + return nil + }(), + ) } values := strings.TrimSuffix( - strings.Repeat("(?,?,?),", len(installers)), + strings.Repeat("(?,?,?,?),", len(installers)), ",", ) if _, err := tx.ExecContext(ctx, fmt.Sprintf(upsertSoftwareTitles, values), args...); err != nil { @@ -1342,6 +1354,20 @@ WHERE } var titleIDs []uint + args = []any{} + for _, installer := range installers { + args = append( + args, + BundleIdentifierOrName(installer.BundleIdentifier, installer.Title), + installer.Source, + "", + ) + } + values = strings.TrimSuffix( + strings.Repeat("(?,?,?),", len(installers)), + ",", + ) + if err := sqlx.SelectContext(ctx, tx, &titleIDs, fmt.Sprintf(loadSoftwareTitles, values), args...); err != nil { return ctxerr.Wrap(ctx, err, "load existing titles") } @@ -1441,7 +1467,7 @@ WHERE postInstallScriptID, // WHERE clause globalOrTeamID, - installer.Title, + BundleIdentifierOrName(installer.BundleIdentifier, installer.Title), installer.Source, } @@ -1472,7 +1498,7 @@ WHERE postInstallScriptID, installer.Platform, installer.SelfService, - installer.Title, + BundleIdentifierOrName(installer.BundleIdentifier, installer.Title), installer.Source, installer.UserID, installer.UserID, @@ -1495,7 +1521,7 @@ WHERE // ID (cannot use res.LastInsertID due to the upsert statement, won't // give the id in case of update) var installerID uint - if err := sqlx.GetContext(ctx, tx, &installerID, loadSoftwareInstallerID, globalOrTeamID, installer.Title, installer.Source); err != nil { + if err := sqlx.GetContext(ctx, tx, &installerID, loadSoftwareInstallerID, globalOrTeamID, BundleIdentifierOrName(installer.BundleIdentifier, installer.Title), installer.Source); err != nil { return ctxerr.Wrapf(ctx, err, "load id of new/edited installer with name %q", installer.Filename) } diff --git a/server/datastore/mysql/software_installers_test.go b/server/datastore/mysql/software_installers_test.go index 38d7cd81ea..c87a727c9c 100644 --- a/server/datastore/mysql/software_installers_test.go +++ b/server/datastore/mysql/software_installers_test.go @@ -753,18 +753,19 @@ func testBatchSetSoftwareInstallers(t *testing.T, ds *Datastore) { tfr0, err := fleet.NewTempFileReader(ins0File, t.TempDir) require.NoError(t, err) err = ds.BatchSetSoftwareInstallers(ctx, &team.ID, []*fleet.UploadSoftwareInstallerPayload{{ - InstallScript: "install", - InstallerFile: tfr0, - StorageID: ins0, - Filename: "installer0", - Title: "ins0", - Source: "apps", - Version: "1", - PreInstallQuery: "foo", - UserID: user1.ID, - Platform: "darwin", - URL: "https://example.com", - ValidatedLabels: &fleet.LabelIdentsWithScope{}, + InstallScript: "install", + InstallerFile: tfr0, + StorageID: ins0, + Filename: "installer0", + Title: "ins0", + Source: "apps", + Version: "1", + PreInstallQuery: "foo", + UserID: user1.ID, + Platform: "darwin", + URL: "https://example.com", + ValidatedLabels: &fleet.LabelIdentsWithScope{}, + BundleIdentifier: "com.example.ins0", }}) require.NoError(t, err) softwareInstallers, err = ds.GetSoftwareInstallers(ctx, team.ID) @@ -950,6 +951,54 @@ func testBatchSetSoftwareInstallers(t *testing.T, ds *Datastore) { {Name: ins1, Source: "apps", Browser: ""}, }) + // Add software installer with same name different bundle id + err = ds.BatchSetSoftwareInstallers(ctx, &team.ID, []*fleet.UploadSoftwareInstallerPayload{{ + InstallScript: "install", + InstallerFile: tfr0, + StorageID: ins0, + Filename: "installer0", + Title: "ins0", + Source: "apps", + Version: "1", + PreInstallQuery: "foo", + UserID: user1.ID, + Platform: "darwin", + URL: "https://example.com", + ValidatedLabels: &fleet.LabelIdentsWithScope{}, + BundleIdentifier: "com.example.different.ins0", + }}) + require.NoError(t, err) + softwareInstallers, err = ds.GetSoftwareInstallers(ctx, team.ID) + require.NoError(t, err) + require.Len(t, softwareInstallers, 1) + assertSoftware([]fleet.SoftwareTitle{ + {Name: ins0, Source: "apps", Browser: "", BundleIdentifier: ptr.String("com.example.different.ins0")}, + }) + + // Add software installer with the same bundle id but different name + err = ds.BatchSetSoftwareInstallers(ctx, &team.ID, []*fleet.UploadSoftwareInstallerPayload{{ + InstallScript: "install", + InstallerFile: tfr0, + StorageID: ins0, + Filename: "installer0", + Title: "ins0-different", + Source: "apps", + Version: "1", + PreInstallQuery: "foo", + UserID: user1.ID, + Platform: "darwin", + URL: "https://example.com", + ValidatedLabels: &fleet.LabelIdentsWithScope{}, + BundleIdentifier: "com.example.ins0", + }}) + require.NoError(t, err) + softwareInstallers, err = ds.GetSoftwareInstallers(ctx, team.ID) + require.NoError(t, err) + require.Len(t, softwareInstallers, 1) + assertSoftware([]fleet.SoftwareTitle{ + {Name: "ins0-different", Source: "apps", Browser: "", BundleIdentifier: ptr.String("com.example.ins0")}, + }) + // remove everything err = ds.BatchSetSoftwareInstallers(ctx, &team.ID, []*fleet.UploadSoftwareInstallerPayload{}) require.NoError(t, err)