diff --git a/pkg/services/serviceaccounts/database/store.go b/pkg/services/serviceaccounts/database/store.go index 581626a3d2a..ebb66ef4842 100644 --- a/pkg/services/serviceaccounts/database/store.go +++ b/pkg/services/serviceaccounts/database/store.go @@ -42,10 +42,19 @@ func ProvideServiceAccountsStore(cfg *setting.Cfg, store db.DB, apiKeyService ap } } +// generateLogin makes a generated string to have a ID for the service account across orgs and it's name +// this causes you to create a service account with the same name in different orgs +// not the same name in the same org +func generateLogin(prefix string, orgId int64, name string) string { + generatedLogin := fmt.Sprintf("%v-%v-%v", prefix, orgId, strings.ToLower(name)) + // in case the name has multiple spaces or dashes in the prefix or otherwise, replace them with a single dash + generatedLogin = strings.Replace(generatedLogin, "--", "-", 1) + return strings.Replace(generatedLogin, " ", "-", -1) +} + // CreateServiceAccount creates service account func (s *ServiceAccountsStoreImpl) CreateServiceAccount(ctx context.Context, orgId int64, saForm *serviceaccounts.CreateServiceAccountForm) (*serviceaccounts.ServiceAccountDTO, error) { - generatedLogin := serviceaccounts.ServiceAccountPrefix + strings.ToLower(saForm.Name) - generatedLogin = strings.ReplaceAll(generatedLogin, " ", "-") + login := generateLogin(serviceaccounts.ServiceAccountPrefix, orgId, saForm.Name) isDisabled := false role := org.RoleViewer if saForm.IsDisabled != nil { @@ -56,7 +65,7 @@ func (s *ServiceAccountsStoreImpl) CreateServiceAccount(ctx context.Context, org } newSA, err := s.userService.CreateServiceAccount(ctx, &user.CreateUserCommand{ - Login: generatedLogin, + Login: login, OrgID: orgId, Name: saForm.Name, IsDisabled: isDisabled, @@ -435,7 +444,7 @@ func (s *ServiceAccountsStoreImpl) MigrateApiKey(ctx context.Context, orgId int6 func (s *ServiceAccountsStoreImpl) CreateServiceAccountFromApikey(ctx context.Context, key *apikey.APIKey) error { prefix := "sa-autogen" cmd := user.CreateUserCommand{ - Login: fmt.Sprintf("%v-%v-%v", prefix, key.OrgID, key.Name), + Login: generateLogin(prefix, key.OrgID, key.Name), Name: fmt.Sprintf("%v-%v", prefix, key.Name), OrgID: key.OrgID, DefaultOrgRole: string(key.Role), diff --git a/pkg/services/serviceaccounts/database/store_test.go b/pkg/services/serviceaccounts/database/store_test.go index 57bbd8e4d46..42e8da77717 100644 --- a/pkg/services/serviceaccounts/database/store_test.go +++ b/pkg/services/serviceaccounts/database/store_test.go @@ -30,8 +30,8 @@ func TestMain(m *testing.M) { // Service Account should not create an org on its own func TestStore_CreateServiceAccountOrgNonExistant(t *testing.T) { _, store := setupTestDatabase(t) + serviceAccountName := "new Service Account" t.Run("create service account", func(t *testing.T) { - serviceAccountName := "new Service Account" serviceAccountOrgId := int64(1) serviceAccountRole := org.RoleAdmin isDisabled := true @@ -47,13 +47,12 @@ func TestStore_CreateServiceAccountOrgNonExistant(t *testing.T) { } func TestStore_CreateServiceAccount(t *testing.T) { - _, store := setupTestDatabase(t) - orgQuery := &org.CreateOrgCommand{Name: orgimpl.MainOrgName} - orgResult, err := store.orgService.CreateWithMember(context.Background(), orgQuery) - require.NoError(t, err) - + serviceAccountName := "new Service Account" t.Run("create service account", func(t *testing.T) { - serviceAccountName := "new Service Account" + _, store := setupTestDatabase(t) + orgQuery := &org.CreateOrgCommand{Name: orgimpl.MainOrgName} + orgResult, err := store.orgService.CreateWithMember(context.Background(), orgQuery) + require.NoError(t, err) serviceAccountOrgId := orgResult.ID serviceAccountRole := org.RoleAdmin isDisabled := true @@ -65,13 +64,11 @@ func TestStore_CreateServiceAccount(t *testing.T) { saDTO, err := store.CreateServiceAccount(context.Background(), serviceAccountOrgId, &saForm) require.NoError(t, err) - assert.Equal(t, "sa-new-service-account", saDTO.Login) assert.Equal(t, serviceAccountName, saDTO.Name) assert.Equal(t, 0, int(saDTO.Tokens)) retrieved, err := store.RetrieveServiceAccount(context.Background(), serviceAccountOrgId, saDTO.Id) require.NoError(t, err) - assert.Equal(t, "sa-new-service-account", retrieved.Login) assert.Equal(t, serviceAccountName, retrieved.Name) assert.Equal(t, serviceAccountOrgId, retrieved.OrgId) assert.Equal(t, string(serviceAccountRole), retrieved.Role) @@ -81,6 +78,82 @@ func TestStore_CreateServiceAccount(t *testing.T) { require.NoError(t, err) assert.Equal(t, saDTO.Id, retrievedId) }) + + t.Run("create service account twice same org, error", func(t *testing.T) { + _, store := setupTestDatabase(t) + orgQuery := &org.CreateOrgCommand{Name: orgimpl.MainOrgName} + orgResult, err := store.orgService.CreateWithMember(context.Background(), orgQuery) + require.NoError(t, err) + serviceAccountOrgId := orgResult.ID + serviceAccountRole := org.RoleAdmin + isDisabled := true + saForm := serviceaccounts.CreateServiceAccountForm{ + Name: serviceAccountName, + Role: &serviceAccountRole, + IsDisabled: &isDisabled, + } + + saDTO, err := store.CreateServiceAccount(context.Background(), serviceAccountOrgId, &saForm) + require.NoError(t, err) + assert.Equal(t, serviceAccountName, saDTO.Name) + assert.Equal(t, 0, int(saDTO.Tokens)) + + retrieved, err := store.RetrieveServiceAccount(context.Background(), serviceAccountOrgId, saDTO.Id) + require.NoError(t, err) + assert.Equal(t, serviceAccountName, retrieved.Name) + assert.Equal(t, serviceAccountOrgId, retrieved.OrgId) + assert.Equal(t, string(serviceAccountRole), retrieved.Role) + assert.True(t, retrieved.IsDisabled) + + retrievedId, err := store.RetrieveServiceAccountIdByName(context.Background(), serviceAccountOrgId, serviceAccountName) + require.NoError(t, err) + assert.Equal(t, saDTO.Id, retrievedId) + + // should not b able to create the same service account twice in the same org + _, err = store.CreateServiceAccount(context.Background(), serviceAccountOrgId, &saForm) + require.Error(t, err) + }) + + t.Run("create service account twice different orgs should work", func(t *testing.T) { + _, store := setupTestDatabase(t) + orgQuery := &org.CreateOrgCommand{Name: orgimpl.MainOrgName} + orgResult, err := store.orgService.CreateWithMember(context.Background(), orgQuery) + require.NoError(t, err) + serviceAccountOrgId := orgResult.ID + serviceAccountRole := org.RoleAdmin + isDisabled := true + saForm := serviceaccounts.CreateServiceAccountForm{ + Name: serviceAccountName, + Role: &serviceAccountRole, + IsDisabled: &isDisabled, + } + + saDTO, err := store.CreateServiceAccount(context.Background(), serviceAccountOrgId, &saForm) + require.NoError(t, err) + assert.Equal(t, serviceAccountName, saDTO.Name) + assert.Equal(t, 0, int(saDTO.Tokens)) + + retrieved, err := store.RetrieveServiceAccount(context.Background(), serviceAccountOrgId, saDTO.Id) + require.NoError(t, err) + assert.Equal(t, serviceAccountName, retrieved.Name) + assert.Equal(t, serviceAccountOrgId, retrieved.OrgId) + assert.Equal(t, string(serviceAccountRole), retrieved.Role) + assert.True(t, retrieved.IsDisabled) + + retrievedId, err := store.RetrieveServiceAccountIdByName(context.Background(), serviceAccountOrgId, serviceAccountName) + require.NoError(t, err) + assert.Equal(t, saDTO.Id, retrievedId) + + orgQuerySecond := &org.CreateOrgCommand{Name: "Second Org name"} + orgResultSecond, err := store.orgService.CreateWithMember(context.Background(), orgQuerySecond) + require.NoError(t, err) + serviceAccountOrgIdSecond := orgResultSecond.ID + // should not b able to create the same service account twice in the same org + saDTOSecond, err := store.CreateServiceAccount(context.Background(), serviceAccountOrgIdSecond, &saForm) + require.NoError(t, err) + assert.Equal(t, serviceAccountName, saDTOSecond.Name) + assert.Equal(t, 0, int(saDTOSecond.Tokens)) + }) } func TestStore_CreateServiceAccountRoleNone(t *testing.T) { @@ -100,13 +173,11 @@ func TestStore_CreateServiceAccountRoleNone(t *testing.T) { saDTO, err := store.CreateServiceAccount(context.Background(), serviceAccountOrgId, &saForm) require.NoError(t, err) - assert.Equal(t, "sa-new-service-account", saDTO.Login) assert.Equal(t, serviceAccountName, saDTO.Name) assert.Equal(t, 0, int(saDTO.Tokens)) retrieved, err := store.RetrieveServiceAccount(context.Background(), serviceAccountOrgId, saDTO.Id) require.NoError(t, err) - assert.Equal(t, "sa-new-service-account", retrieved.Login) assert.Equal(t, serviceAccountName, retrieved.Name) assert.Equal(t, serviceAccountOrgId, retrieved.OrgId) assert.Equal(t, string(serviceAccountRole), retrieved.Role) diff --git a/pkg/services/sqlstore/migrations/user_mig.go b/pkg/services/sqlstore/migrations/user_mig.go index 4f56666fcdb..bac9f99bf11 100644 --- a/pkg/services/sqlstore/migrations/user_mig.go +++ b/pkg/services/sqlstore/migrations/user_mig.go @@ -153,6 +153,13 @@ func addUserMigrations(mg *Migrator) { mg.AddMigration("Add unique index user_uid", NewAddIndexMigration(userV2, &Index{ Cols: []string{"uid"}, Type: UniqueIndex, })) + + // Service accounts login were not unique per org. this migration is part of making it unique per org + // to be able to create service accounts that are unique per org + mg.AddMigration("Update login field for service accounts", NewRawSQLMigration(""). + SQLite("UPDATE user SET login = 'sa-' || CAST(org_id AS TEXT) || '-' || REPLACE(login, 'sa-', '') WHERE login IS NOT NULL AND is_service_account = 1;"). + Postgres("UPDATE \"user\" SET login = 'sa-' || org_id::text || '-' || REPLACE(login, 'sa-', '') WHERE login IS NOT NULL AND is_service_account = true;"). + Mysql("UPDATE user SET login = CONCAT('sa-', CAST(org_id AS CHAR), '-', REPLACE(login, 'sa-', '')) WHERE login IS NOT NULL AND is_service_account = 1;")) } const migSQLITEisServiceAccountNullable = `ALTER TABLE user ADD COLUMN tmp_service_account BOOLEAN DEFAULT 0;