Service accounts: UI migration results (#68789)

* ui migration WIP

* merge

* migration tests for api

* revert chagnes to align with main

* revert chagnes to align with main

* revert chagnes to align with main

* remove unused code and comments

* revert gen files

* retry logic inplace

* fix a any

* fixed types

* migraiton results now show only result if no failures

* review comments

* wording to make it more actionable

* add migraiton summary text onyl for failed apikeys

* fixed wording and added a close button to the modal

* made the button close the modal

* moved state into component

* fix based on review, naming and removed unused code

* service account migration state optional

* making migration result undefined

* showing total and migrated numbers for a successful migration

* fix payload const to take the payload

pkg/services/serviceaccounts/api/api.go

@@ -38,7 +38,7 @@ type service interface {
 	SearchOrgServiceAccounts(ctx context.Context, query *serviceaccounts.SearchOrgServiceAccountsQuery) (*serviceaccounts.SearchOrgServiceAccountsResult, error)
 	ListTokens(ctx context.Context, query *serviceaccounts.GetSATokensQuery) ([]apikey.APIKey, error)
 	DeleteServiceAccount(ctx context.Context, orgID, serviceAccountID int64) error
-	MigrateApiKeysToServiceAccounts(ctx context.Context, orgID int64) error
+	MigrateApiKeysToServiceAccounts(ctx context.Context, orgID int64) (*serviceaccounts.MigrationResult, error)
 	MigrateApiKey(ctx context.Context, orgID int64, keyId int64) error
 	// Service account tokens
 	AddServiceAccountToken(ctx context.Context, serviceAccountID int64, cmd *serviceaccounts.AddServiceAccountTokenCommand) (*apikey.APIKey, error)
@@ -315,11 +315,12 @@ func (api *ServiceAccountsAPI) SearchOrgServiceAccountsWithPaging(c *contextmode
 
 // POST /api/serviceaccounts/migrate
 func (api *ServiceAccountsAPI) MigrateApiKeysToServiceAccounts(ctx *contextmodel.ReqContext) response.Response {
-	if err := api.service.MigrateApiKeysToServiceAccounts(ctx.Req.Context(), ctx.OrgID); err != nil {
-		return response.Error(http.StatusInternalServerError, "Internal server error", err)
+	results, err := api.service.MigrateApiKeysToServiceAccounts(ctx.Req.Context(), ctx.OrgID)
+	if err != nil {
+		return response.JSON(http.StatusInternalServerError, results)
 	}
 
-	return response.Success("API keys migrated to service accounts")
+	return response.JSON(http.StatusOK, results)
 }
 
 // POST /api/serviceaccounts/migrate/:keyId

pkg/services/serviceaccounts/api/api_test.go

@@ -2,6 +2,7 @@ package api
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"net/http"
 	"strings"
@@ -235,6 +236,66 @@ func TestServiceAccountsAPI_UpdateServiceAccount(t *testing.T) {
 	}
 }
 
+func TestServiceAccountsAPI_MigrateApiKeysToServiceAccounts(t *testing.T) {
+	type TestCase struct {
+		desc                    string
+		orgId                   int64
+		basicRole               org.RoleType
+		permissions             []accesscontrol.Permission
+		expectedMigrationResult *serviceaccounts.MigrationResult
+		expectedCode            int
+	}
+
+	tests := []TestCase{
+		{
+			desc:      "should be able to migrate API keys to service accounts with correct permissions",
+			orgId:     1,
+			basicRole: org.RoleAdmin,
+			permissions: []accesscontrol.Permission{
+				{Action: serviceaccounts.ActionCreate, Scope: serviceaccounts.ScopeAll},
+			},
+			expectedMigrationResult: &serviceaccounts.MigrationResult{
+				Total:         5,
+				Migrated:      4,
+				Failed:        1,
+				FailedDetails: []string{"API key name: failedKey - Error: migration error"},
+			},
+			expectedCode: http.StatusOK,
+		},
+		{
+			desc:      "should not be able to migrate API keys to service accounts with wrong permissions",
+			orgId:     2,
+			basicRole: org.RoleAdmin,
+			permissions: []accesscontrol.Permission{
+				{Action: serviceaccounts.ActionCreate, Scope: serviceaccounts.ScopeAll},
+			},
+			expectedCode: http.StatusForbidden,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			server := setupTests(t, func(a *ServiceAccountsAPI) {
+				a.service = &fakeServiceAccountService{ExpectedMigrationResult: tt.expectedMigrationResult}
+			})
+
+			req := server.NewRequest(http.MethodPost, "/api/serviceaccounts/migrate", nil)
+			webtest.RequestWithSignedInUser(req, &user.SignedInUser{OrgRole: tt.basicRole, OrgID: tt.orgId, Permissions: map[int64]map[string][]string{1: accesscontrol.GroupScopesByAction(tt.permissions)}})
+			res, err := server.SendJSON(req)
+			require.NoError(t, err)
+
+			assert.Equal(t, tt.expectedCode, res.StatusCode)
+			if tt.expectedCode == http.StatusOK {
+				var result serviceaccounts.MigrationResult
+				err := json.NewDecoder(res.Body).Decode(&result)
+				require.NoError(t, err)
+				assert.Equal(t, tt.expectedMigrationResult, &result)
+			}
+			require.NoError(t, res.Body.Close())
+		})
+	}
+}
+
 func setupTests(t *testing.T, opts ...func(a *ServiceAccountsAPI)) *webtest.Server {
 	t.Helper()
 	cfg := setting.NewCfg()
@@ -264,6 +325,7 @@ type fakeServiceAccountService struct {
 	ExpectedServiceAccountTokens  []apikey.APIKey
 	ExpectedServiceAccount        *serviceaccounts.ServiceAccountDTO
 	ExpectedServiceAccountProfile *serviceaccounts.ServiceAccountProfileDTO
+	ExpectedMigrationResult       *serviceaccounts.MigrationResult
 }
 
 func (f *fakeServiceAccountService) CreateServiceAccount(ctx context.Context, orgID int64, saForm *serviceaccounts.CreateServiceAccountForm) (*serviceaccounts.ServiceAccountDTO, error) {
@@ -293,3 +355,8 @@ func (f *fakeServiceAccountService) AddServiceAccountToken(ctx context.Context,
 func (f *fakeServiceAccountService) DeleteServiceAccountToken(ctx context.Context, orgID, id, tokenID int64) error {
 	return f.ExpectedErr
 }
+
+func (f *fakeServiceAccountService) MigrateApiKeysToServiceAccounts(ctx context.Context, orgID int64) (*serviceaccounts.MigrationResult, error) {
+	fmt.Printf("fake migration result: %v", f.ExpectedMigrationResult)
+	return f.ExpectedMigrationResult, f.ExpectedErr
+}

pkg/services/serviceaccounts/database/store.go

@@ -364,25 +364,35 @@ func (s *ServiceAccountsStoreImpl) SearchOrgServiceAccounts(ctx context.Context,
 	return searchResult, nil
 }
 
-func (s *ServiceAccountsStoreImpl) MigrateApiKeysToServiceAccounts(ctx context.Context, orgId int64) error {
+func (s *ServiceAccountsStoreImpl) MigrateApiKeysToServiceAccounts(ctx context.Context, orgId int64) (*serviceaccounts.MigrationResult, error) {
 	basicKeys, err := s.apiKeyService.GetAllAPIKeys(ctx, orgId)
 	if err != nil {
-		return err
+		return nil, err
 	}
+
+	migrationResult := &serviceaccounts.MigrationResult{
+		Total:           len(basicKeys),
+		Migrated:        0,
+		Failed:          0,
+		FailedApikeyIDs: []int64{},
+		FailedDetails:   []string{},
+	}
+
 	if len(basicKeys) > 0 {
 		for _, key := range basicKeys {
 			err := s.CreateServiceAccountFromApikey(ctx, key)
 			if err != nil {
-				s.log.Error("migating to service accounts failed with error", err)
-				return err
+				s.log.Error("migating to service accounts failed with error", err.Error())
+				migrationResult.Failed++
+				migrationResult.FailedDetails = append(migrationResult.FailedDetails, fmt.Sprintf("API key name: %s - Error: %s", key.Name, err.Error()))
+				migrationResult.FailedApikeyIDs = append(migrationResult.FailedApikeyIDs, key.ID)
+			} else {
+				migrationResult.Migrated++
+				s.log.Debug("API key converted to service account token", "keyId", key.ID)
 			}
-			s.log.Debug("API key converted to service account token", "keyId", key.ID)
 		}
 	}
-	if err := s.kvStore.Set(ctx, orgId, "serviceaccounts", "migrationStatus", "1"); err != nil {
-		s.log.Error("Failed to write API keys migration status", err)
-	}
-	return nil
+	return migrationResult, nil
 }
 
 func (s *ServiceAccountsStoreImpl) MigrateApiKey(ctx context.Context, orgId int64, keyId int64) error {
@@ -415,17 +425,12 @@ func (s *ServiceAccountsStoreImpl) CreateServiceAccountFromApikey(ctx context.Co
 		IsServiceAccount: true,
 	}
 
-	return s.sqlStore.WithTransactionalDbSession(ctx, func(sess *db.Session) error {
-		newSA, errCreateSA := s.userService.CreateServiceAccount(ctx, &cmd)
+	return s.sqlStore.InTransaction(ctx, func(tctx context.Context) error {
+		newSA, errCreateSA := s.userService.CreateServiceAccount(tctx, &cmd)
 		if errCreateSA != nil {
 			return fmt.Errorf("failed to create service account: %w", errCreateSA)
 		}
-
-		if err := s.assignApiKeyToServiceAccount(sess, key.ID, newSA.ID); err != nil {
-			return fmt.Errorf("failed to migrate API key to service account token: %w", err)
-		}
-
-		return nil
+		return s.assignApiKeyToServiceAccount(tctx, key.ID, newSA.ID)
 	})
 }
 

pkg/services/serviceaccounts/database/store_test.go

@@ -219,11 +219,13 @@ func TestStore_MigrateApiKeys(t *testing.T) {
 
 func TestStore_MigrateAllApiKeys(t *testing.T) {
 	cases := []struct {
-		desc                   string
-		keys                   []tests.TestApiKey
-		orgId                  int64
-		expectedServiceAccouts int64
-		expectedErr            error
+		desc                    string
+		keys                    []tests.TestApiKey
+		orgId                   int64
+		expectedServiceAccounts int64
+		expectedErr             error
+		expectedMigratedResults *serviceaccounts.MigrationResult
+		ctxWithFastCancel       bool
 	}{
 		{
 			desc: "api keys should be migrated to service account tokens within provided org",
@@ -232,9 +234,16 @@ func TestStore_MigrateAllApiKeys(t *testing.T) {
 				{Name: "test2", Role: org.RoleEditor, Key: "secret2", OrgId: 1},
 				{Name: "test3", Role: org.RoleEditor, Key: "secret3", OrgId: 2},
 			},
-			orgId:                  1,
-			expectedServiceAccouts: 2,
-			expectedErr:            nil,
+			orgId:                   1,
+			expectedServiceAccounts: 2,
+			expectedErr:             nil,
+			expectedMigratedResults: &serviceaccounts.MigrationResult{
+				Total:           2,
+				Migrated:        2,
+				Failed:          0,
+				FailedApikeyIDs: []int64{},
+				FailedDetails:   []string{},
+			},
 		},
 		{
 			desc: "api keys from another orgs shouldn't be migrated",
@@ -242,9 +251,16 @@ func TestStore_MigrateAllApiKeys(t *testing.T) {
 				{Name: "test1", Role: org.RoleEditor, Key: "secret1", OrgId: 2},
 				{Name: "test2", Role: org.RoleEditor, Key: "secret2", OrgId: 2},
 			},
-			orgId:                  1,
-			expectedServiceAccouts: 0,
-			expectedErr:            nil,
+			orgId:                   1,
+			expectedServiceAccounts: 0,
+			expectedErr:             nil,
+			expectedMigratedResults: &serviceaccounts.MigrationResult{
+				Total:           0,
+				Migrated:        0,
+				Failed:          0,
+				FailedApikeyIDs: []int64{},
+				FailedDetails:   []string{},
+			},
 		},
 		{
 			desc: "expired api keys should be migrated",
@@ -252,9 +268,16 @@ func TestStore_MigrateAllApiKeys(t *testing.T) {
 				{Name: "test1", Role: org.RoleEditor, Key: "secret1", OrgId: 1},
 				{Name: "test2", Role: org.RoleEditor, Key: "secret2", OrgId: 1, IsExpired: true},
 			},
-			orgId:                  1,
-			expectedServiceAccouts: 2,
-			expectedErr:            nil,
+			orgId:                   1,
+			expectedServiceAccounts: 2,
+			expectedErr:             nil,
+			expectedMigratedResults: &serviceaccounts.MigrationResult{
+				Total:           2,
+				Migrated:        2,
+				Failed:          0,
+				FailedApikeyIDs: []int64{},
+				FailedDetails:   []string{},
+			},
 		},
 	}
 
@@ -271,7 +294,7 @@ func TestStore_MigrateAllApiKeys(t *testing.T) {
 				tests.SetupApiKey(t, db, key)
 			}
 
-			err = store.MigrateApiKeysToServiceAccounts(context.Background(), c.orgId)
+			results, err := store.MigrateApiKeysToServiceAccounts(context.Background(), c.orgId)
 			if c.expectedErr != nil {
 				require.ErrorIs(t, err, c.expectedErr)
 			} else {
@@ -294,8 +317,8 @@ func TestStore_MigrateAllApiKeys(t *testing.T) {
 				}
 				serviceAccounts, err := store.SearchOrgServiceAccounts(context.Background(), &q)
 				require.NoError(t, err)
-				require.Equal(t, c.expectedServiceAccouts, serviceAccounts.TotalCount)
-				if c.expectedServiceAccouts > 0 {
+				require.Equal(t, c.expectedServiceAccounts, serviceAccounts.TotalCount)
+				if c.expectedServiceAccounts > 0 {
 					saMigrated := serviceAccounts.ServiceAccounts[0]
 					require.Equal(t, string(c.keys[0].Role), saMigrated.Role)
 
@@ -306,6 +329,7 @@ func TestStore_MigrateAllApiKeys(t *testing.T) {
 					require.NoError(t, err)
 					require.Len(t, tokens, 1)
 				}
+				require.Equal(t, c.expectedMigratedResults, results)
 			}
 		})
 	}

pkg/services/serviceaccounts/database/token_store.go

@@ -110,23 +110,24 @@ func (s *ServiceAccountsStoreImpl) RevokeServiceAccountToken(ctx context.Context
 }
 
 // assignApiKeyToServiceAccount sets the API key service account ID
-func (s *ServiceAccountsStoreImpl) assignApiKeyToServiceAccount(sess *db.Session, apiKeyId int64, serviceAccountId int64) error {
-	key := apikey.APIKey{ID: apiKeyId}
-	exists, err := sess.Get(&key)
-	if err != nil {
-		s.log.Warn("API key not loaded", "err", err)
-		return err
-	}
-	if !exists {
-		s.log.Warn("API key not found", "err", err)
-		return apikey.ErrNotFound
-	}
-	key.ServiceAccountId = &serviceAccountId
+func (s *ServiceAccountsStoreImpl) assignApiKeyToServiceAccount(ctx context.Context, apiKeyId int64, serviceAccountId int64) error {
+	return s.sqlStore.WithDbSession(ctx, func(sess *db.Session) error {
+		key := apikey.APIKey{ID: apiKeyId}
+		exists, err := sess.Get(&key)
+		if err != nil {
+			s.log.Warn("API key not loaded", "err", err)
+			return err
+		}
+		if !exists {
+			s.log.Warn("API key not found", "err", err)
+			return apikey.ErrNotFound
+		}
+		key.ServiceAccountId = &serviceAccountId
 
-	if _, err := sess.ID(key.ID).Update(&key); err != nil {
-		s.log.Warn("Could not update api key", "err", err)
-		return err
-	}
-
-	return nil
+		if _, err := sess.ID(key.ID).Update(&key); err != nil {
+			s.log.Warn("Could not update api key", "err", err)
+			return err
+		}
+		return nil
+	})
 }

pkg/services/serviceaccounts/manager/service.go

@@ -234,9 +234,9 @@ func (sa *ServiceAccountsService) MigrateApiKey(ctx context.Context, orgID, keyI
 	}
 	return sa.store.MigrateApiKey(ctx, orgID, keyID)
 }
-func (sa *ServiceAccountsService) MigrateApiKeysToServiceAccounts(ctx context.Context, orgID int64) error {
+func (sa *ServiceAccountsService) MigrateApiKeysToServiceAccounts(ctx context.Context, orgID int64) (*serviceaccounts.MigrationResult, error) {
 	if err := validOrgID(orgID); err != nil {
-		return err
+		return nil, err
 	}
 	return sa.store.MigrateApiKeysToServiceAccounts(ctx, orgID)
 }

pkg/services/serviceaccounts/manager/service_test.go

@@ -18,6 +18,7 @@ type FakeServiceAccountStore struct {
 	ExpectedServiceAccountProfileDTO        *serviceaccounts.ServiceAccountProfileDTO
 	ExpectedSearchServiceAccountQueryResult *serviceaccounts.SearchOrgServiceAccountsResult
 	ExpectedStats                           *serviceaccounts.Stats
+	expectedMigratedResults                 *serviceaccounts.MigrationResult
 	ExpectedAPIKeys                         []apikey.APIKey
 	ExpectedAPIKey                          *apikey.APIKey
 	ExpectedBoolean                         bool
@@ -60,8 +61,8 @@ func (f *FakeServiceAccountStore) DeleteServiceAccount(ctx context.Context, orgI
 }
 
 // MigrateApiKeysToServiceAccounts is a fake migrating api keys to service accounts.
-func (f *FakeServiceAccountStore) MigrateApiKeysToServiceAccounts(ctx context.Context, orgID int64) error {
-	return f.ExpectedError
+func (f *FakeServiceAccountStore) MigrateApiKeysToServiceAccounts(ctx context.Context, orgID int64) (*serviceaccounts.MigrationResult, error) {
+	return f.expectedMigratedResults, f.ExpectedError
 }
 
 // MigrateApiKey is a fake migrating an api key to a service account.

pkg/services/serviceaccounts/manager/store.go

@@ -15,7 +15,7 @@ type store interface {
 	RetrieveServiceAccount(ctx context.Context, orgID, serviceAccountID int64) (*serviceaccounts.ServiceAccountProfileDTO, error)
 	RetrieveServiceAccountIdByName(ctx context.Context, orgID int64, name string) (int64, error)
 	DeleteServiceAccount(ctx context.Context, orgID, serviceAccountID int64) error
-	MigrateApiKeysToServiceAccounts(ctx context.Context, orgID int64) error
+	MigrateApiKeysToServiceAccounts(ctx context.Context, orgID int64) (*serviceaccounts.MigrationResult, error)
 	MigrateApiKey(ctx context.Context, orgID int64, keyId int64) error
 	ListTokens(ctx context.Context, query *serviceaccounts.GetSATokensQuery) ([]apikey.APIKey, error)
 	RevokeServiceAccountToken(ctx context.Context, orgId, serviceAccountId, tokenId int64) error

pkg/services/serviceaccounts/models.go

@@ -37,6 +37,14 @@ var (
 	ErrDuplicateToken                    = errutil.NewBase(errutil.StatusBadRequest, "serviceaccounts.ErrTokenAlreadyExists", errutil.WithPublicMessage("service account token with given name already exists in the organization"))
 )
 
+type MigrationResult struct {
+	Total           int
+	Migrated        int
+	Failed          int
+	FailedApikeyIDs []int64
+	FailedDetails   []string
+}
+
 type ServiceAccount struct {
 	Id int64
 }