Plugin: Remove external service on plugin removal (#77712)

* Plugin: Remove external service on plugin removal

* Add feature flag check in the service registration service

* Initialize map

* Add HasExternalService as suggested

* Commit suggestion

Co-authored-by: linoman <2051016+linoman@users.noreply.github.com>

* Nit on test.

Co-authored-by: linoman <2051016+linoman@users.noreply.github.com>


---------

Co-authored-by: linoman <2051016+linoman@users.noreply.github.com>

pkg/services/serviceaccounts/extsvcaccounts/service.go

@@ -126,6 +126,37 @@ func (esa *ExtSvcAccountsService) SaveExternalService(ctx context.Context, cmd *
 	return &extsvcauth.ExternalService{Name: cmd.Name, ID: slug, Secret: token}, nil
 }
 
+func (esa *ExtSvcAccountsService) RemoveExternalService(ctx context.Context, name string) error {
+	// This is double proofing, we should never reach here anyway the flags have already been checked.
+	if !esa.features.IsEnabled(featuremgmt.FlagExternalServiceAccounts) && !esa.features.IsEnabled(featuremgmt.FlagExternalServiceAuth) {
+		esa.logger.Warn("This feature is behind a feature flag, please set it if you want to save external services")
+		return nil
+	}
+	return esa.RemoveExtSvcAccount(ctx, extsvcauth.TmpOrgID, slugify.Slugify(name))
+}
+
+func (esa *ExtSvcAccountsService) RemoveExtSvcAccount(ctx context.Context, orgID int64, extSvcSlug string) error {
+	saID, errRetrieve := esa.saSvc.RetrieveServiceAccountIdByName(ctx, orgID, sa.ExtSvcPrefix+extSvcSlug)
+	if errRetrieve != nil && !errors.Is(errRetrieve, sa.ErrServiceAccountNotFound) {
+		return errRetrieve
+	}
+
+	if saID <= 0 {
+		esa.logger.Debug("No external service account associated with this service", "service", extSvcSlug, "orgID", orgID)
+		return nil
+	}
+
+	if err := esa.deleteExtSvcAccount(ctx, orgID, extSvcSlug, saID); err != nil {
+		esa.logger.Error("Error occurred while deleting service account",
+			"service", extSvcSlug,
+			"saID", saID,
+			"error", err.Error())
+		return err
+	}
+	esa.logger.Info("Deleted external service account", "service", extSvcSlug, "orgID", orgID)
+	return nil
+}
+
 // ManageExtSvcAccount creates, updates or deletes the service account associated with an external service
 func (esa *ExtSvcAccountsService) ManageExtSvcAccount(ctx context.Context, cmd *sa.ManageExtSvcAccountCmd) (int64, error) {
 	// This is double proofing, we should never reach here anyway the flags have already been checked.
@@ -153,7 +184,6 @@ func (esa *ExtSvcAccountsService) ManageExtSvcAccount(ctx context.Context, cmd *
 					"error", err.Error())
 				return 0, err
 			}
-			esa.metrics.deletedCount.Inc()
 		}
 		esa.logger.Info("Skipping service account creation, no permission",
 			"service", cmd.ExtSvcSlug,
@@ -173,8 +203,6 @@ func (esa *ExtSvcAccountsService) ManageExtSvcAccount(ctx context.Context, cmd *
 		esa.logger.Error("Could not save service account", "service", cmd.ExtSvcSlug, "error", errSave.Error())
 		return 0, errSave
 	}
-	esa.metrics.savedCount.Inc()
-
 	return saID, nil
 }
 
@@ -212,6 +240,8 @@ func (esa *ExtSvcAccountsService) saveExtSvcAccount(ctx context.Context, cmd *sa
 		return 0, err
 	}
 
+	esa.metrics.savedCount.Inc()
+
 	return cmd.SaID, nil
 }
 
@@ -224,7 +254,11 @@ func (esa *ExtSvcAccountsService) deleteExtSvcAccount(ctx context.Context, orgID
 	if err := esa.acSvc.DeleteExternalServiceRole(ctx, slug); err != nil {
 		return err
 	}
-	return esa.DeleteExtSvcCredentials(ctx, orgID, slug)
+	if err := esa.DeleteExtSvcCredentials(ctx, orgID, slug); err != nil {
+		return err
+	}
+	esa.metrics.deletedCount.Inc()
+	return nil
 }
 
 // getExtSvcAccountToken get or create the token of an External Service

pkg/services/serviceaccounts/extsvcaccounts/service_test.go

@@ -380,3 +380,64 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) {
 		})
 	}
 }
+
+func TestExtSvcAccountsService_RemoveExtSvcAccount(t *testing.T) {
+	extSvcSlug := "grafana-test-app"
+	tmpOrgID := int64(1)
+	extSvcAccID := int64(10)
+	tests := []struct {
+		name   string
+		init   func(env *TestEnv)
+		slug   string
+		checks func(t *testing.T, env *TestEnv)
+		want   *extsvcauth.ExternalService
+	}{
+		{
+			name: "should not fail if the service account does not exist",
+			init: func(env *TestEnv) {
+				// No previous service account was attached to this slug
+				env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, tmpOrgID, sa.ExtSvcPrefix+extSvcSlug).
+					Return(int64(0), sa.ErrServiceAccountNotFound.Errorf("not found"))
+			},
+			slug: extSvcSlug,
+			want: nil,
+		},
+		{
+			name: "should remove service account",
+			init: func(env *TestEnv) {
+				// A previous service account was attached to this slug
+				env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, tmpOrgID, sa.ExtSvcPrefix+extSvcSlug).
+					Return(extSvcAccID, nil)
+				env.SaSvc.On("DeleteServiceAccount", mock.Anything, tmpOrgID, extSvcAccID).Return(nil)
+				env.AcStore.On("DeleteExternalServiceRole", mock.Anything, extSvcSlug).Return(nil)
+				// A token was previously stored in the secret store
+				_ = env.SkvStore.Set(context.Background(), tmpOrgID, extSvcSlug, kvStoreType, "ExtSvcSecretToken")
+			},
+			slug: extSvcSlug,
+			checks: func(t *testing.T, env *TestEnv) {
+				_, ok, _ := env.SkvStore.Get(context.Background(), tmpOrgID, extSvcSlug, kvStoreType)
+				require.False(t, ok, "secret should have been removed from store")
+			},
+			want: nil,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := context.Background()
+
+			env := setupTestEnv(t)
+			if tt.init != nil {
+				tt.init(env)
+			}
+
+			err := env.S.RemoveExtSvcAccount(ctx, tmpOrgID, tt.slug)
+			require.NoError(t, err)
+
+			if tt.checks != nil {
+				tt.checks(t, env)
+			}
+			env.SaSvc.AssertExpectations(t)
+			env.AcStore.AssertExpectations(t)
+		})
+	}
+}