From feecd306870c3902f8b7dcbcd4de2f8090d5c09d Mon Sep 17 00:00:00 2001 From: Mihai Doarna Date: Thu, 28 Nov 2024 09:42:58 +0200 Subject: [PATCH] API key: Fix API key migration to service account (#97100) fix api key migration to service account --- .../serviceaccounts/database/store.go | 19 +++++ .../serviceaccounts/database/store_test.go | 74 +++++++++++++++++-- 2 files changed, 86 insertions(+), 7 deletions(-) diff --git a/pkg/services/serviceaccounts/database/store.go b/pkg/services/serviceaccounts/database/store.go index 2759fa764eb..d859817e49b 100644 --- a/pkg/services/serviceaccounts/database/store.go +++ b/pkg/services/serviceaccounts/database/store.go @@ -509,11 +509,30 @@ func (s *ServiceAccountsStoreImpl) CreateServiceAccountFromApikey(ctx context.Co IsServiceAccount: true, } + // maximum number of attempts for creating a service account + attempts := 10 + return s.sqlStore.InTransaction(ctx, func(tctx context.Context) error { newSA, errCreateSA := s.userService.CreateServiceAccount(tctx, &cmd) + if errCreateSA != nil { + if errors.Is(errCreateSA, serviceaccounts.ErrServiceAccountAlreadyExists) { + // The service account we tried to create already exists with that login name. We will attempt to create + // a unique service account by adding suffixes to the initial login name (e.g. -001, -002, ... , -010). + for i := 1; errCreateSA != nil && i <= attempts; i++ { + serviceAccountName := fmt.Sprintf("%s-%03d", key.Name, i) + cmd.Login = generateLogin(prefix, key.OrgID, serviceAccountName) + newSA, errCreateSA = s.userService.CreateServiceAccount(tctx, &cmd) + if errCreateSA != nil && !errors.Is(errCreateSA, serviceaccounts.ErrServiceAccountAlreadyExists) { + break + } + } + } + } + if errCreateSA != nil { return fmt.Errorf("failed to create service account: %w", errCreateSA) } + return s.assignApiKeyToServiceAccount(tctx, key.ID, newSA.ID) }) } diff --git a/pkg/services/serviceaccounts/database/store_test.go b/pkg/services/serviceaccounts/database/store_test.go index f00cd48d4df..bc705d070b7 100644 --- a/pkg/services/serviceaccounts/database/store_test.go +++ b/pkg/services/serviceaccounts/database/store_test.go @@ -324,26 +324,85 @@ func TestIntegrationStore_MigrateApiKeys(t *testing.T) { t.Skip("skipping test in short mode") } cases := []struct { - desc string - key tests.TestApiKey - expectedErr error + desc string + serviceAccounts []user.CreateUserCommand + key tests.TestApiKey + expectedLogin string + expectedErr error }{ { - desc: "api key should be migrated to service account token", - key: tests.TestApiKey{Name: "Test1", Role: org.RoleEditor, OrgId: 1}, - expectedErr: nil, + desc: "api key should be migrated to service account token", + serviceAccounts: []user.CreateUserCommand{}, + key: tests.TestApiKey{Name: "test1", Role: org.RoleEditor, OrgId: 1}, + expectedLogin: "sa-autogen-1-test1", + expectedErr: nil, + }, + { + desc: "api key should be migrated to service account token on second attempt", + serviceAccounts: []user.CreateUserCommand{ + {Login: "sa-autogen-1-test2"}, + }, + key: tests.TestApiKey{Name: "test2", Role: org.RoleEditor, OrgId: 1}, + expectedLogin: "sa-autogen-1-test2-001", + expectedErr: nil, + }, + { + desc: "api key should be migrated to service account token on last attempt (the 10th)", + serviceAccounts: []user.CreateUserCommand{ + {Login: "sa-autogen-1-test3"}, + {Login: "sa-autogen-1-test3-001"}, + {Login: "sa-autogen-1-test3-002"}, + {Login: "sa-autogen-1-test3-003"}, + {Login: "sa-autogen-1-test3-004"}, + {Login: "sa-autogen-1-test3-005"}, + {Login: "sa-autogen-1-test3-006"}, + {Login: "sa-autogen-1-test3-007"}, + {Login: "sa-autogen-1-test3-008"}, + {Login: "sa-autogen-1-test3-009"}, + }, + key: tests.TestApiKey{Name: "test3", Role: org.RoleEditor, OrgId: 1}, + expectedLogin: "sa-autogen-1-test3-010", + expectedErr: nil, + }, + { + desc: "api key should not be migrated to service account token because all attempts failed", + serviceAccounts: []user.CreateUserCommand{ + {Login: "sa-autogen-1-test4"}, + {Login: "sa-autogen-1-test4-001"}, + {Login: "sa-autogen-1-test4-002"}, + {Login: "sa-autogen-1-test4-003"}, + {Login: "sa-autogen-1-test4-004"}, + {Login: "sa-autogen-1-test4-005"}, + {Login: "sa-autogen-1-test4-006"}, + {Login: "sa-autogen-1-test4-007"}, + {Login: "sa-autogen-1-test4-008"}, + {Login: "sa-autogen-1-test4-009"}, + {Login: "sa-autogen-1-test4-010"}, + }, + key: tests.TestApiKey{Name: "test4", Role: org.RoleEditor, OrgId: 1}, + expectedErr: serviceaccounts.ErrServiceAccountAlreadyExists, }, } for _, c := range cases { t.Run(c.desc, func(t *testing.T) { db, store := setupTestDatabase(t) + store.cfg.AutoAssignOrg = true store.cfg.AutoAssignOrgId = 1 store.cfg.AutoAssignOrgRole = "Viewer" _, err := store.orgService.CreateWithMember(context.Background(), &org.CreateOrgCommand{Name: "main"}) require.NoError(t, err) + key := tests.SetupApiKey(t, db, store.cfg, c.key) + + for _, sa := range c.serviceAccounts { + sa.IsServiceAccount = true + sa.OrgID = key.OrgID + _, err := store.userService.CreateServiceAccount(context.Background(), &sa) + require.NoError(t, err) + } + err = store.MigrateApiKey(context.Background(), key.OrgID, key.ID) if c.expectedErr != nil { require.ErrorIs(t, err, c.expectedErr) @@ -352,7 +411,7 @@ func TestIntegrationStore_MigrateApiKeys(t *testing.T) { q := serviceaccounts.SearchOrgServiceAccountsQuery{ OrgID: key.OrgID, - Query: "", + Query: c.expectedLogin, Page: 1, Limit: 50, SignedInUser: &user.SignedInUser{ @@ -370,6 +429,7 @@ func TestIntegrationStore_MigrateApiKeys(t *testing.T) { require.Equal(t, int64(1), serviceAccounts.TotalCount) saMigrated := serviceAccounts.ServiceAccounts[0] require.Equal(t, string(key.Role), saMigrated.Role) + require.Equal(t, c.expectedLogin, saMigrated.Login) tokens, err := store.ListTokens(context.Background(), &serviceaccounts.GetSATokensQuery{ OrgID: &key.OrgID,