From e76c3638ffb72d2d70de77067b1810ac849d43e1 Mon Sep 17 00:00:00 2001 From: Jahziel Villasana-Espinoza Date: Tue, 14 Jan 2025 12:31:04 -0500 Subject: [PATCH] fix: do not remove VPP apps from team if not strictly necessary (#25411) > For #25194 # Checklist for submitter 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] 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 --- changes/25194-vpp-app-clear | 1 + server/datastore/mysql/vpp.go | 25 ++++++++-- server/datastore/mysql/vpp_test.go | 80 +++++++++++++++++++++++++++++- 3 files changed, 100 insertions(+), 6 deletions(-) create mode 100644 changes/25194-vpp-app-clear diff --git a/changes/25194-vpp-app-clear b/changes/25194-vpp-app-clear new file mode 100644 index 0000000000..b559870e66 --- /dev/null +++ b/changes/25194-vpp-app-clear @@ -0,0 +1 @@ +- Stopped VPP apps from being removed from teams whenever the VPP token team assignment is updated. \ No newline at end of file diff --git a/server/datastore/mysql/vpp.go b/server/datastore/mysql/vpp.go index 24a54cf842..2c830808b3 100644 --- a/server/datastore/mysql/vpp.go +++ b/server/datastore/mysql/vpp.go @@ -910,8 +910,14 @@ func (ds *Datastore) UpdateVPPTokenTeams(ctx context.Context, id uint, teams []u stmtRemovePolicyAutomations := `UPDATE policies p JOIN vpp_apps_teams vat ON vat.id = p.vpp_apps_teams_id AND vat.vpp_token_id = ? SET vpp_apps_teams_id = NULL` - stmtDeleteApps := `DELETE FROM vpp_apps_teams WHERE vpp_token_id = ?` - deleteArgs := []any{id} + stmtDeleteApps := `DELETE FROM vpp_apps_teams WHERE vpp_token_id = ? %s` + + var teamsFilter string + if len(teams) > 0 { + teamsFilter = "AND global_or_team_id NOT IN (?)" + } + + stmtDeleteApps = fmt.Sprintf(stmtDeleteApps, teamsFilter) var values string var args []any @@ -956,11 +962,22 @@ func (ds *Datastore) UpdateVPPTokenTeams(ctx context.Context, id uint, teams []u return ctxerr.Wrap(ctx, err, "vpp token null team check") } - if _, err := tx.ExecContext(ctx, stmtRemovePolicyAutomations, deleteArgs...); err != nil { + if _, err := tx.ExecContext(ctx, stmtRemovePolicyAutomations, id); err != nil { return ctxerr.Wrap(ctx, err, "deleting old vpp team apps policy automations") } - if _, err := tx.ExecContext(ctx, stmtDeleteApps, deleteArgs...); err != nil { + delArgs := []any{id} + if len(teams) > 0 { + inStmt, inArgs, err := sqlx.In(stmtDeleteApps, id, teams) + if err != nil { + return ctxerr.Wrap(ctx, err, "building IN statement for deleting old vpp apps teams associations") + } + + stmtDeleteApps = inStmt + delArgs = inArgs + } + + if _, err := tx.ExecContext(ctx, stmtDeleteApps, delArgs...); err != nil { return ctxerr.Wrap(ctx, err, "deleting old vpp team apps associations") } diff --git a/server/datastore/mysql/vpp_test.go b/server/datastore/mysql/vpp_test.go index 30ef377d35..89c41df222 100644 --- a/server/datastore/mysql/vpp_test.go +++ b/server/datastore/mysql/vpp_test.go @@ -33,6 +33,7 @@ func TestVPP(t *testing.T) { {"VPPTokenAppTeamAssociations", testVPPTokenAppTeamAssociations}, {"GetOrInsertSoftwareTitleForVPPApp", testGetOrInsertSoftwareTitleForVPPApp}, {"DeleteVPPAssignedToPolicy", testDeleteVPPAssignedToPolicy}, + {"TestVPPTokenTeamAssignment", testVPPTokenTeamAssignment}, } for _, c := range cases { @@ -802,7 +803,7 @@ func testVPPTokensCRUD(t *testing.T, ds *Datastore) { assert.Equal(t, dataToken.Token, upTok.Token) assert.Equal(t, orgName, upTok.OrgName) assert.Equal(t, location, upTok.Location) - assert.NotNil(t, upTok.Teams) // "All Teams" teamm array is non-nil but empty + assert.NotNil(t, upTok.Teams) // "All Teams" team array is non-nil but empty assert.Len(t, upTok.Teams, 0) tok, err = ds.GetVPPToken(ctx, tok.ID) @@ -1217,7 +1218,6 @@ func testVPPTokensCRUD(t *testing.T, ds *Datastore) { //// err = ds.DeleteVPPToken(ctx, tokNone.ID) assert.NoError(t, err) - } func testVPPTokenAppTeamAssociations(t *testing.T, ds *Datastore) { @@ -1468,3 +1468,79 @@ func testDeleteVPPAssignedToPolicy(t *testing.T, ds *Datastore) { err = ds.DeleteVPPAppFromTeam(ctx, ptr.Uint(0), va1.VPPAppID) require.NoError(t, err) } + +func testVPPTokenTeamAssignment(t *testing.T, ds *Datastore) { + ctx := context.Background() + + // Create a couple of teams + team1, err := ds.NewTeam(ctx, &fleet.Team{Name: "team1" + t.Name()}) + require.NoError(t, err) + + team2, err := ds.NewTeam(ctx, &fleet.Team{Name: "team2" + t.Name()}) + require.NoError(t, err) + + dataToken, err := test.CreateVPPTokenData(time.Now().Add(24*time.Hour), "vpp_org"+t.Name(), "vpp location"+t.Name()) + require.NoError(t, err) + tok1, err := ds.InsertVPPToken(ctx, dataToken) + assert.NoError(t, err) + _, err = ds.UpdateVPPTokenTeams(ctx, tok1.ID, []uint{}) + assert.NoError(t, err) + + // Insert some VPP apps for no team + app1 := &fleet.VPPApp{Name: "vpp_app_1", VPPAppTeam: fleet.VPPAppTeam{VPPAppID: fleet.VPPAppID{AdamID: "1", Platform: fleet.MacOSPlatform}}, BundleIdentifier: "b1"} + _, err = ds.InsertVPPAppWithTeam(ctx, app1, nil) + require.NoError(t, err) + app2 := &fleet.VPPApp{Name: "vpp_app_2", VPPAppTeam: fleet.VPPAppTeam{VPPAppID: fleet.VPPAppID{AdamID: "2", Platform: fleet.MacOSPlatform}}, BundleIdentifier: "b2"} + _, err = ds.InsertVPPAppWithTeam(ctx, app2, nil) + require.NoError(t, err) + app3 := &fleet.VPPApp{Name: "vpp_app_3", VPPAppTeam: fleet.VPPAppTeam{VPPAppID: fleet.VPPAppID{AdamID: "3", Platform: fleet.MacOSPlatform}}, BundleIdentifier: "b3"} + _, err = ds.InsertVPPAppWithTeam(ctx, app3, nil) + require.NoError(t, err) + app4 := &fleet.VPPApp{Name: "vpp_app_4", VPPAppTeam: fleet.VPPAppTeam{VPPAppID: fleet.VPPAppID{AdamID: "4", Platform: fleet.MacOSPlatform}}, BundleIdentifier: "b4"} + _, err = ds.InsertVPPAppWithTeam(ctx, app4, nil) + require.NoError(t, err) + + assigned, err := ds.GetAssignedVPPApps(ctx, &team1.ID) + require.NoError(t, err) + require.Empty(t, assigned) + + assigned, err = ds.GetAssignedVPPApps(ctx, &team2.ID) + require.NoError(t, err) + require.Empty(t, assigned) + + // assign app1 and app2 to team1 + err = ds.SetTeamVPPApps(ctx, &team1.ID, []fleet.VPPAppTeam{ + {VPPAppID: app1.VPPAppID, InstallDuringSetup: ptr.Bool(true)}, + {VPPAppID: app2.VPPAppID, SelfService: true}, + }) + require.NoError(t, err) + + assigned, err = ds.GetAssignedVPPApps(ctx, &team1.ID) + require.NoError(t, err) + require.Len(t, assigned, 2) + assert.Contains(t, assigned, app1.VPPAppID) + assert.Contains(t, assigned, app2.VPPAppID) + assert.True(t, assigned[app2.VPPAppID].SelfService) + assert.True(t, *assigned[app1.VPPAppID].InstallDuringSetup) + + // assign token to team1 and team 2 explicitly + _, err = ds.UpdateVPPTokenTeams(ctx, tok1.ID, []uint{team1.ID, team2.ID}) + require.NoError(t, err) + + // team1 should still have its apps + assigned, err = ds.GetAssignedVPPApps(ctx, &team1.ID) + require.NoError(t, err) + require.Len(t, assigned, 2) + assert.Contains(t, assigned, app1.VPPAppID) + assert.Contains(t, assigned, app2.VPPAppID) + assert.True(t, assigned[app2.VPPAppID].SelfService) + assert.True(t, *assigned[app1.VPPAppID].InstallDuringSetup) + + // assign token to just team2 + _, err = ds.UpdateVPPTokenTeams(ctx, tok1.ID, []uint{team2.ID}) + require.NoError(t, err) + // team1 should no longer have its apps, since the token was removed + assigned, err = ds.GetAssignedVPPApps(ctx, &team1.ID) + require.NoError(t, err) + require.Empty(t, assigned) +}