fix VPP installers and add tests

fleetdm · Sep 26, 2024 · aafb59c · aafb59c
1 parent a7589f1
commit aafb59c
Show file tree

Hide file tree

Showing 4 changed files with 219 additions and 8 deletions.
diff --git 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,78 @@ 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, 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 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
@@ -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
@@ -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,89 @@ 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 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
@@ -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, "")
+}