From a0c950acf6ee9b1c0e01416c2fc29aa5643dfdbd Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Wed, 6 Sep 2023 14:31:40 -0400 Subject: [PATCH] Fix auto-removal of integrations when an unrelated setting is saved (#13743) --- .../issue-13372-fix-integrations-auto-removed | 1 + server/fleet/service.go | 3 -- server/service/appconfig.go | 54 +++++++++++-------- server/service/integration_core_test.go | 43 +++++++++++++-- server/service/integration_enterprise_test.go | 50 +++++++++++++++-- 5 files changed, 120 insertions(+), 31 deletions(-) create mode 100644 changes/issue-13372-fix-integrations-auto-removed diff --git a/changes/issue-13372-fix-integrations-auto-removed b/changes/issue-13372-fix-integrations-auto-removed new file mode 100644 index 0000000000..5adb1751c3 --- /dev/null +++ b/changes/issue-13372-fix-integrations-auto-removed @@ -0,0 +1 @@ +* Fixed a bug where Jira and/or Zendesk integrations were being removed when an unrelated setting was changed. diff --git a/server/fleet/service.go b/server/fleet/service.go index e9f2b53f21..9554c78154 100644 --- a/server/fleet/service.go +++ b/server/fleet/service.go @@ -615,9 +615,6 @@ type Service interface { GetMDMAppleFileVaultSummary(ctx context.Context, teamID *uint) (*MDMAppleFileVaultSummary, error) // GetMDMAppleEnrollmentProfileByToken returns the Apple enrollment from its secret token. - // TODO(mna): this may have to be removed if we don't end up supporting - // manual enrollment via a token (currently we only support it via Fleet - // Desktop, in the My Device page). See #8701. GetMDMAppleEnrollmentProfileByToken(ctx context.Context, enrollmentToken string, enrollmentRef string) (profile []byte, err error) // GetDeviceMDMAppleEnrollmentProfile loads the raw (PList-format) enrollment diff --git a/server/service/appconfig.go b/server/service/appconfig.go index 36417d44cd..7e8f94529c 100644 --- a/server/service/appconfig.go +++ b/server/service/appconfig.go @@ -392,32 +392,42 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle } } - delJira, err := fleet.ValidateJiraIntegrations(ctx, storedJiraByProjectKey, newAppConfig.Integrations.Jira) - if err != nil { - if errors.As(err, &fleet.IntegrationTestError{}) { - return nil, ctxerr.Wrap(ctx, &fleet.BadRequestError{ - Message: err.Error(), - }) + // NOTE: the frontend will always send all integrations back when making + // changes, so as soon as Jira or Zendesk has something set, it's fair to + // assume that integrations are being modified and we have the full set of + // those integrations. When deleting, it does send empty arrays (not nulls), + // so this is fine - e.g. when deleting the last integration it sends: + // + // {"integrations":{"zendesk":[],"jira":[]}} + // + if newAppConfig.Integrations.Jira != nil || newAppConfig.Integrations.Zendesk != nil { + delJira, err := fleet.ValidateJiraIntegrations(ctx, storedJiraByProjectKey, newAppConfig.Integrations.Jira) + if err != nil { + if errors.As(err, &fleet.IntegrationTestError{}) { + return nil, ctxerr.Wrap(ctx, &fleet.BadRequestError{ + Message: err.Error(), + }) + } + return nil, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("Jira integration", err.Error())) } - return nil, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("Jira integration", err.Error())) - } - appConfig.Integrations.Jira = newAppConfig.Integrations.Jira + appConfig.Integrations.Jira = newAppConfig.Integrations.Jira - delZendesk, err := fleet.ValidateZendeskIntegrations(ctx, storedZendeskByGroupID, newAppConfig.Integrations.Zendesk) - if err != nil { - if errors.As(err, &fleet.IntegrationTestError{}) { - return nil, ctxerr.Wrap(ctx, &fleet.BadRequestError{ - Message: err.Error(), - }) + delZendesk, err := fleet.ValidateZendeskIntegrations(ctx, storedZendeskByGroupID, newAppConfig.Integrations.Zendesk) + if err != nil { + if errors.As(err, &fleet.IntegrationTestError{}) { + return nil, ctxerr.Wrap(ctx, &fleet.BadRequestError{ + Message: err.Error(), + }) + } + return nil, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("Zendesk integration", err.Error())) } - return nil, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("Zendesk integration", err.Error())) - } - appConfig.Integrations.Zendesk = newAppConfig.Integrations.Zendesk + appConfig.Integrations.Zendesk = newAppConfig.Integrations.Zendesk - // if any integration was deleted, remove it from any team that uses it - if len(delJira)+len(delZendesk) > 0 { - if err := svc.ds.DeleteIntegrationsFromTeams(ctx, fleet.Integrations{Jira: delJira, Zendesk: delZendesk}); err != nil { - return nil, ctxerr.Wrap(ctx, err, "delete integrations from teams") + // if any integration was deleted, remove it from any team that uses it + if len(delJira)+len(delZendesk) > 0 { + if err := svc.ds.DeleteIntegrationsFromTeams(ctx, fleet.Integrations{Jira: delJira, Zendesk: delZendesk}); err != nil { + return nil, ctxerr.Wrap(ctx, err, "delete integrations from teams") + } } } diff --git a/server/service/integration_core_test.go b/server/service/integration_core_test.go index 5e8363fba2..37f8a4d858 100644 --- a/server/service/integration_core_test.go +++ b/server/service/integration_core_test.go @@ -3611,6 +3611,16 @@ func (s *integrationTestSuite) TestExternalIntegrationsConfig() { require.Equal(t, "qux2", config.Integrations.Jira[1].ProjectKey) require.False(t, config.Integrations.Jira[1].EnableSoftwareVulnerabilities) + // make an unrelated appconfig change, should not remove the integrations + var appCfgResp appConfigResponse + s.DoJSON("PATCH", "/api/v1/fleet/config", json.RawMessage(`{ + "org_info": { + "org_name": "test-integrations" + } + }`), http.StatusOK, &appCfgResp) + require.Equal(t, "test-integrations", appCfgResp.OrgInfo.OrgName) + require.Len(t, appCfgResp.Integrations.Jira, 2) + // delete first Jira integration s.DoRaw("PATCH", "/api/v1/fleet/config", []byte(fmt.Sprintf(`{ "integrations": { @@ -3932,13 +3942,22 @@ func (s *integrationTestSuite) TestExternalIntegrationsConfig() { } }`, srvURL)), http.StatusOK) - // remove all integrations - s.DoRaw("PATCH", "/api/v1/fleet/config", []byte(`{ + // if no jira nor zendesk integrations are provided, does not remove integrations + appCfgResp = appConfigResponse{} + s.DoJSON("PATCH", "/api/v1/fleet/config", json.RawMessage(`{ + "integrations": {} + }`), http.StatusOK, &appCfgResp) + require.Len(t, appCfgResp.Integrations.Jira, 1) + + // if explicitly-empty arrays are provided, remove all integrations + appCfgResp = appConfigResponse{} + s.DoJSON("PATCH", "/api/v1/fleet/config", json.RawMessage(`{ "integrations": { "jira": [], "zendesk": [] } - }`), http.StatusOK) + }`), http.StatusOK, &appCfgResp) + require.Len(t, appCfgResp.Integrations.Jira, 0) // set environmental varible to use Zendesk test client t.Setenv("TEST_ZENDESK_CLIENT", "true") @@ -4004,6 +4023,16 @@ func (s *integrationTestSuite) TestExternalIntegrationsConfig() { require.Equal(t, int64(123), config.Integrations.Zendesk[1].GroupID) require.False(t, config.Integrations.Zendesk[1].EnableSoftwareVulnerabilities) + // make an unrelated appconfig change, should not remove the integrations + appCfgResp = appConfigResponse{} + s.DoJSON("PATCH", "/api/v1/fleet/config", json.RawMessage(`{ + "org_info": { + "org_name": "test-integrations-zendesk" + } + }`), http.StatusOK, &appCfgResp) + require.Equal(t, "test-integrations-zendesk", appCfgResp.OrgInfo.OrgName) + require.Len(t, appCfgResp.Integrations.Zendesk, 2) + // delete first Zendesk integration s.DoRaw("PATCH", "/api/v1/fleet/config", []byte(fmt.Sprintf(`{ "integrations": { @@ -4398,6 +4427,14 @@ func (s *integrationTestSuite) TestExternalIntegrationsConfig() { } }`, srvURL)), http.StatusUnprocessableEntity) + // if no jira nor zendesk integrations are provided, does not remove integrations + appCfgResp = appConfigResponse{} + s.DoJSON("PATCH", "/api/v1/fleet/config", json.RawMessage(`{ + "integrations": {} + }`), http.StatusOK, &appCfgResp) + require.Len(t, appCfgResp.Integrations.Jira, 1) + require.Len(t, appCfgResp.Integrations.Zendesk, 1) + // remove all integrations on exit, so that other tests can enable the // webhook as needed s.DoRaw("PATCH", "/api/v1/fleet/config", []byte(`{ diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index 51b62e2f67..329744aa68 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -1186,6 +1186,7 @@ func (s *integrationEnterpriseTestSuite) TestExternalIntegrationsTeamConfig() { require.Len(t, getResp.Team.Config.Integrations.Zendesk, 0) // disable the webhook and enable the automation + tmResp = teamResponse{} s.DoJSON("PATCH", fmt.Sprintf("/api/latest/fleet/teams/%d", team.ID), fleet.TeamPayload{ Integrations: &fleet.TeamIntegrations{ Jira: []*fleet.TeamJiraIntegration{ @@ -1206,6 +1207,24 @@ func (s *integrationEnterpriseTestSuite) TestExternalIntegrationsTeamConfig() { require.Len(t, tmResp.Team.Config.Integrations.Jira, 1) require.Equal(t, "qux", tmResp.Team.Config.Integrations.Jira[0].ProjectKey) + // update the team with an unrelated field, should not change integrations + tmResp = teamResponse{} + s.DoJSON("PATCH", fmt.Sprintf("/api/latest/fleet/teams/%d", team.ID), fleet.TeamPayload{ + Description: ptr.String("team-desc"), + }, http.StatusOK, &tmResp) + require.Len(t, tmResp.Team.Config.Integrations.Jira, 1) + require.Equal(t, "team-desc", tmResp.Team.Description) + + // make an unrelated appconfig change, should not remove the global integrations nor the teams' + var appCfgResp appConfigResponse + s.DoJSON("PATCH", "/api/v1/fleet/config", json.RawMessage(`{ + "org_info": { + "org_name": "test-integrations" + } + }`), http.StatusOK, &appCfgResp) + require.Equal(t, "test-integrations", appCfgResp.OrgInfo.OrgName) + require.Len(t, appCfgResp.Integrations.Jira, 2) + // enable the webhook without changing the integration should fail (an integration is already enabled) s.DoJSON("PATCH", fmt.Sprintf("/api/latest/fleet/teams/%d", team.ID), fleet.TeamPayload{WebhookSettings: &fleet.TeamWebhookSettings{ FailingPoliciesWebhook: fleet.FailingPoliciesWebhookSettings{ @@ -1401,6 +1420,25 @@ func (s *integrationEnterpriseTestSuite) TestExternalIntegrationsTeamConfig() { require.Equal(t, int64(122), tmResp.Team.Config.Integrations.Zendesk[0].GroupID) require.Equal(t, int64(123), tmResp.Team.Config.Integrations.Zendesk[1].GroupID) + // update the team with an unrelated field, should not change integrations + tmResp = teamResponse{} + s.DoJSON("PATCH", fmt.Sprintf("/api/latest/fleet/teams/%d", team.ID), fleet.TeamPayload{ + Description: ptr.String("team-desc-2"), + }, http.StatusOK, &tmResp) + require.Len(t, tmResp.Team.Config.Integrations.Zendesk, 2) + require.Equal(t, "team-desc-2", tmResp.Team.Description) + + // make an unrelated appconfig change, should not remove the global integrations nor the teams' + appCfgResp = appConfigResponse{} + s.DoJSON("PATCH", "/api/v1/fleet/config", json.RawMessage(`{ + "org_info": { + "org_name": "test-integrations-2" + } + }`), http.StatusOK, &appCfgResp) + require.Equal(t, "test-integrations-2", appCfgResp.OrgInfo.OrgName) + require.Len(t, appCfgResp.Integrations.Zendesk, 2) + require.Len(t, appCfgResp.Integrations.Jira, 2) + // enabling the second without disabling the first fails s.DoJSON("PATCH", fmt.Sprintf("/api/latest/fleet/teams/%d", team.ID), fleet.TeamPayload{ Integrations: &fleet.TeamIntegrations{ @@ -1629,9 +1667,15 @@ func (s *integrationEnterpriseTestSuite) TestExternalIntegrationsTeamConfig() { require.False(t, tmResp.Team.Config.WebhookSettings.FailingPoliciesWebhook.Enable) require.Empty(t, tmResp.Team.Config.WebhookSettings.FailingPoliciesWebhook.DestinationURL) - s.DoRaw("PATCH", "/api/v1/fleet/config", []byte(`{ - "integrations": {} - }`), http.StatusOK) + appCfgResp = appConfigResponse{} + s.DoJSON("PATCH", "/api/v1/fleet/config", json.RawMessage(`{ + "integrations": { + "jira": [], + "zendesk": [] + } + }`), http.StatusOK, &appCfgResp) + require.Len(t, appCfgResp.Integrations.Jira, 0) + require.Len(t, appCfgResp.Integrations.Zendesk, 0) } func (s *integrationEnterpriseTestSuite) TestMacOSUpdatesConfig() {