From 102e78f1f1f60948095c061bb8761a6c14f292e2 Mon Sep 17 00:00:00 2001 From: Ian Littman Date: Thu, 26 Sep 2024 13:23:50 -0500 Subject: [PATCH] Fix software-with-bundle-ID add when the same title with different/no bundle ID, add missing request timeout special case for edit package endpoint (#22413) Same as #22412, for #21370, but against `main` rather than 4.57.0. If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/Committing-Changes.md#changes-files) for more information. - [x] Added/updated tests - [x] Manual QA for all new/changed functionality --------- Co-authored-by: Roberto Dip --- changes/21370-bundle-id-quickfix | 1 + changes/software-edit-request-deadline | 1 + cmd/fleet/serve.go | 3 +- server/datastore/mysql/software_installers.go | 5 +- .../mysql/software_installers_test.go | 84 ++++++++++++++++ server/datastore/mysql/vpp.go | 25 +++-- server/datastore/mysql/vpp_test.go | 96 +++++++++++++++++++ server/service/integration_enterprise_test.go | 38 ++++++++ 8 files changed, 242 insertions(+), 11 deletions(-) create mode 100644 changes/21370-bundle-id-quickfix create mode 100644 changes/software-edit-request-deadline diff --git a/changes/21370-bundle-id-quickfix b/changes/21370-bundle-id-quickfix new file mode 100644 index 000000000000..1f27bedc40af --- /dev/null +++ b/changes/21370-bundle-id-quickfix @@ -0,0 +1 @@ +- Fix "no rows" error when adding a software installer that matches an existing title's name and source but not its bundle ID diff --git a/changes/software-edit-request-deadline b/changes/software-edit-request-deadline new file mode 100644 index 000000000000..f3576256f5ed --- /dev/null +++ b/changes/software-edit-request-deadline @@ -0,0 +1 @@ +* Ensure request timeouts for software installer edits are just as high as for initial software installer uploads diff --git a/cmd/fleet/serve.go b/cmd/fleet/serve.go index e330cedcb34b..fff5cf13536d 100644 --- a/cmd/fleet/serve.go +++ b/cmd/fleet/serve.go @@ -1145,7 +1145,8 @@ the way that the Fleet server works. } } - if req.Method == http.MethodPost && strings.HasSuffix(req.URL.Path, "/fleet/software/package") { + if (req.Method == http.MethodPost && strings.HasSuffix(req.URL.Path, "/fleet/software/package")) || + (req.Method == http.MethodPatch && strings.HasSuffix(req.URL.Path, "/package") && strings.Contains(req.URL.Path, "/fleet/software/titles/")) { // when uploading a software installer, the file might be large so // the read timeout (to read the full request body) must be extended. rc := http.NewResponseController(rw) diff --git a/server/datastore/mysql/software_installers.go b/server/datastore/mysql/software_installers.go index 7d7f0169e3ad..da9f5f7ff395 100644 --- a/server/datastore/mysql/software_installers.go +++ b/server/datastore/mysql/software_installers.go @@ -174,8 +174,9 @@ func (ds *Datastore) getOrGenerateSoftwareInstallerTitleID(ctx context.Context, insertArgs := []any{payload.Title, payload.Source} if payload.BundleIdentifier != "" { - selectStmt = `SELECT id FROM software_titles WHERE bundle_identifier = ?` - selectArgs = []any{payload.BundleIdentifier} + // match by bundle identifier first, or standard matching if we don't have a bundle identifier match + selectStmt = `SELECT id FROM software_titles WHERE bundle_identifier = ? OR (name = ? AND source = ? AND browser = '') ORDER BY bundle_identifier = ? DESC LIMIT 1` + selectArgs = []any{payload.BundleIdentifier, payload.Title, payload.Source, payload.BundleIdentifier} insertStmt = `INSERT INTO software_titles (name, source, bundle_identifier, browser) VALUES (?, ?, ?, '')` insertArgs = append(insertArgs, payload.BundleIdentifier) } diff --git a/server/datastore/mysql/software_installers_test.go b/server/datastore/mysql/software_installers_test.go index 178b85807148..49be5c29c220 100644 --- a/server/datastore/mysql/software_installers_test.go +++ b/server/datastore/mysql/software_installers_test.go @@ -34,6 +34,7 @@ func TestSoftwareInstallers(t *testing.T) { {"HasSelfServiceSoftwareInstallers", testHasSelfServiceSoftwareInstallers}, {"DeleteSoftwareInstallersAssignedToPolicy", testDeleteSoftwareInstallersAssignedToPolicy}, {"GetHostLastInstallData", testGetHostLastInstallData}, + {"GetOrGenerateSoftwareInstallerTitleID", testGetOrGenerateSoftwareInstallerTitleID}, } for _, c := range cases { @@ -1098,3 +1099,86 @@ func testGetHostLastInstallData(t *testing.T, ds *Datastore) { require.NoError(t, err) require.Nil(t, host2LastInstall) } + +func testGetOrGenerateSoftwareInstallerTitleID(t *testing.T, ds *Datastore) { + ctx := context.Background() + + host1 := test.NewHost(t, ds, "host1", "", "host1key", "host1uuid", time.Now()) + host2 := test.NewHost(t, ds, "host2", "", "host2key", "host2uuid", time.Now()) + + software1 := []fleet.Software{ + {Name: "Existing Title", Version: "0.0.1", Source: "apps", BundleIdentifier: "existing.title"}, + } + software2 := []fleet.Software{ + {Name: "Existing Title", Version: "v0.0.2", Source: "apps", BundleIdentifier: "existing.title"}, + {Name: "Existing Title", Version: "0.0.3", Source: "apps", BundleIdentifier: "existing.title"}, + {Name: "Existing Title Without Bundle", Version: "0.0.3", Source: "apps"}, + } + + _, err := ds.UpdateHostSoftware(ctx, host1.ID, software1) + require.NoError(t, err) + _, err = ds.UpdateHostSoftware(ctx, host2.ID, software2) + require.NoError(t, err) + require.NoError(t, ds.SyncHostsSoftware(ctx, time.Now())) + require.NoError(t, ds.ReconcileSoftwareTitles(ctx)) + require.NoError(t, ds.SyncHostsSoftwareTitles(ctx, time.Now())) + + tests := []struct { + name string + payload *fleet.UploadSoftwareInstallerPayload + }{ + { + name: "title that already exists, no bundle identifier in payload", + payload: &fleet.UploadSoftwareInstallerPayload{ + Title: "Existing Title", + Source: "apps", + }, + }, + { + name: "title that already exists, mismatched bundle identifier in payload", + payload: &fleet.UploadSoftwareInstallerPayload{ + Title: "Existing Title", + Source: "apps", + BundleIdentifier: "com.existing.bundle", + }, + }, + { + name: "title that already exists but doesn't have a bundle identifier", + payload: &fleet.UploadSoftwareInstallerPayload{ + Title: "Existing Title Without Bundle", + Source: "apps", + }, + }, + { + name: "title that already exists, no bundle identifier in DB, bundle identifier in payload", + payload: &fleet.UploadSoftwareInstallerPayload{ + Title: "Existing Title Without Bundle", + Source: "apps", + BundleIdentifier: "com.new.bundleid", + }, + }, + { + name: "title that doesn't exist, no bundle identifier in payload", + payload: &fleet.UploadSoftwareInstallerPayload{ + Title: "New Title", + Source: "some_source", + }, + }, + { + name: "title that doesn't exist, with bundle identifier in payload", + payload: &fleet.UploadSoftwareInstallerPayload{ + Title: "New Title With Bundle", + Source: "some_source", + BundleIdentifier: "com.new.bundle", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + id, err := ds.getOrGenerateSoftwareInstallerTitleID(ctx, tt.payload) + require.NoError(t, err) + require.NotEmpty(t, id) + }) + } +} diff --git a/server/datastore/mysql/vpp.go b/server/datastore/mysql/vpp.go index 1127497f888e..cecb68ab3820 100644 --- a/server/datastore/mysql/vpp.go +++ b/server/datastore/mysql/vpp.go @@ -371,17 +371,26 @@ func (ds *Datastore) getOrInsertSoftwareTitleForVPPApp(ctx context.Context, tx s insertArgs := []any{app.Name, source} if app.BundleIdentifier != "" { - // NOTE: The index `idx_sw_titles` doesn't include the bundle - // identifier. It's possible for the select to return nothing - // but for the insert to fail if an app with the same name but - // no bundle identifier exists in the DB. + // match by bundle identifier first, or standard matching if we + // don't have a bundle identifier match switch source { case "ios_apps", "ipados_apps": - selectStmt = `SELECT id FROM software_titles WHERE bundle_identifier = ? AND source = ?` - selectArgs = []any{app.BundleIdentifier, source} + selectStmt = ` + SELECT id + FROM software_titles + WHERE (bundle_identifier = ? AND source = ?) OR (name = ? AND source = ? AND browser = '') + ORDER BY bundle_identifier = ? DESC + LIMIT 1` + selectArgs = []any{app.BundleIdentifier, source, app.Name, source, app.BundleIdentifier} default: - selectStmt = `SELECT id FROM software_titles WHERE bundle_identifier = ? AND source NOT IN ('ios_apps', 'ipados_apps')` - selectArgs = []any{app.BundleIdentifier} + selectStmt = ` + SELECT id + FROM software_titles + WHERE (bundle_identifier = ? OR (name = ? AND browser = '')) + AND source NOT IN ('ios_apps', 'ipados_apps') + ORDER BY bundle_identifier = ? DESC + LIMIT 1` + selectArgs = []any{app.BundleIdentifier, app.Name, app.BundleIdentifier} } insertStmt = `INSERT INTO software_titles (name, source, bundle_identifier, browser) VALUES (?, ?, ?, '')` insertArgs = append(insertArgs, app.BundleIdentifier) diff --git a/server/datastore/mysql/vpp_test.go b/server/datastore/mysql/vpp_test.go index 9a6c6796ceee..78cab8acba22 100644 --- a/server/datastore/mysql/vpp_test.go +++ b/server/datastore/mysql/vpp_test.go @@ -2,6 +2,7 @@ package mysql import ( "context" + "fmt" "testing" "time" @@ -30,6 +31,7 @@ func TestVPP(t *testing.T) { {"GetVPPAppByTeamAndTitleID", testGetVPPAppByTeamAndTitleID}, {"VPPTokensCRUD", testVPPTokensCRUD}, {"VPPTokenAppTeamAssociations", testVPPTokenAppTeamAssociations}, + {"GetOrInsertSoftwareTitleForVPPApp", testGetOrInsertSoftwareTitleForVPPApp}, } for _, c := range cases { @@ -1201,3 +1203,97 @@ func testVPPTokenAppTeamAssociations(t *testing.T, ds *Datastore) { _, err = ds.InsertVPPAppWithTeam(ctx, app1, &team2.ID) assert.Error(t, err) } + +func testGetOrInsertSoftwareTitleForVPPApp(t *testing.T, ds *Datastore) { + ctx := context.Background() + + host1 := test.NewHost(t, ds, "host1", "", "host1key", "host1uuid", time.Now()) + host2 := test.NewHost(t, ds, "host2", "", "host2key", "host2uuid", time.Now()) + + software1 := []fleet.Software{ + {Name: "Existing Title", Version: "0.0.1", Source: "apps", BundleIdentifier: "existing.title"}, + } + software2 := []fleet.Software{ + {Name: "Existing Title", Version: "v0.0.2", Source: "apps", BundleIdentifier: "existing.title"}, + {Name: "Existing Title", Version: "0.0.3", Source: "apps", BundleIdentifier: "existing.title"}, + {Name: "Existing Title Without Bundle", Version: "0.0.3", Source: "apps"}, + } + + _, err := ds.UpdateHostSoftware(ctx, host1.ID, software1) + require.NoError(t, err) + _, err = ds.UpdateHostSoftware(ctx, host2.ID, software2) + require.NoError(t, err) + require.NoError(t, ds.SyncHostsSoftware(ctx, time.Now())) + require.NoError(t, ds.ReconcileSoftwareTitles(ctx)) + require.NoError(t, ds.SyncHostsSoftwareTitles(ctx, time.Now())) + + tests := []struct { + name string + app *fleet.VPPApp + }{ + { + name: "title that already exists, no bundle identifier in payload", + app: &fleet.VPPApp{ + Name: "Existing Title", + LatestVersion: "0.0.1", + BundleIdentifier: "", + }, + }, + { + name: "title that already exists, bundle identifier in payload", + app: &fleet.VPPApp{ + Name: "Existing Title", + LatestVersion: "0.0.2", + BundleIdentifier: "existing.title", + }, + }, + { + name: "title that already exists but doesn't have a bundle identifier", + app: &fleet.VPPApp{ + Name: "Existing Title Without Bundle", + LatestVersion: "0.0.3", + BundleIdentifier: "", + }, + }, + { + name: "title that already exists, no bundle identifier in DB, bundle identifier in payload", + app: &fleet.VPPApp{ + Name: "Existing Title Without Bundle", + LatestVersion: "0.0.3", + BundleIdentifier: "new.bundle.id", + }, + }, + { + name: "title that doesn't exist, no bundle identifier in payload", + app: &fleet.VPPApp{ + Name: "New Title", + LatestVersion: "0.1.0", + BundleIdentifier: "", + }, + }, + { + name: "title that doesn't exist, with bundle identifier in payload", + app: &fleet.VPPApp{ + Name: "New Title", + LatestVersion: "0.1.0", + BundleIdentifier: "new.title.bundle", + }, + }, + } + + for _, platform := range fleet.VPPAppsPlatforms { + for _, tt := range tests { + t.Run(fmt.Sprintf("%s_%v", tt.name, platform), func(t *testing.T) { + tt.app.Platform = platform + var id uint + err := ds.withTx(ctx, func(tx sqlx.ExtContext) error { + var err error + id, err = ds.getOrInsertSoftwareTitleForVPPApp(ctx, tx, tt.app) + return err + }) + require.NoError(t, err) + require.NotEmpty(t, id) + }) + } + } +} diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index cb2a97966846..3d6673afd3b6 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -14366,3 +14366,41 @@ func (s *integrationEnterpriseTestSuite) TestPolicyAutomationsSoftwareInstallers require.NoError(t, err) require.Nil(t, hostVanillaOsquery5Team1LastInstall) } + +func (s *integrationEnterpriseTestSuite) TestSoftwareInstallersWithoutBundleIdentifier() { + t := s.T() + ctx := context.Background() + + // Create a host without a team + host, err := s.ds.NewHost(context.Background(), &fleet.Host{ + DetailUpdatedAt: time.Now(), + LabelUpdatedAt: time.Now(), + PolicyUpdatedAt: time.Now(), + SeenTime: time.Now().Add(-1 * time.Minute), + OsqueryHostID: ptr.String(t.Name()), + NodeKey: ptr.String(t.Name()), + UUID: uuid.New().String(), + Hostname: fmt.Sprintf("%sfoo.local", t.Name()), + Platform: "darwin", + }) + require.NoError(t, err) + + software := []fleet.Software{ + {Name: "DummyApp.app", Version: "0.0.2", Source: "apps"}, + } + // we must ingest the title with an empty bundle identifier for this + // test to be valid + require.Empty(t, software[0].BundleIdentifier) + _, err = s.ds.UpdateHostSoftware(ctx, host.ID, software) + require.NoError(t, err) + require.NoError(t, s.ds.SyncHostsSoftware(ctx, time.Now())) + require.NoError(t, s.ds.ReconcileSoftwareTitles(ctx)) + require.NoError(t, s.ds.SyncHostsSoftwareTitles(ctx, time.Now())) + + payload := &fleet.UploadSoftwareInstallerPayload{ + InstallScript: "install", + Filename: "dummy_installer.pkg", + Version: "0.0.2", + } + s.uploadSoftwareInstaller(payload, http.StatusOK, "") +}