From 2795f9827a8a6e41e6fb6484e999e654fbaa4ea6 Mon Sep 17 00:00:00 2001 From: Gabriel MABILLE Date: Thu, 14 Mar 2024 19:11:02 +0100 Subject: [PATCH] ExtSvcAccounts: FIX prevent service account deletion (#84502) * ExtSvcAccounts: Fix External Service Accounts Login check Co-authored-by: Karl Persson * Remove service accounts assignments and permissions on delete * Fix first set of tests * Fix second batch of tests * Fix third batch of tests --------- Co-authored-by: Karl Persson --- pkg/services/extsvcauth/models.go | 3 +- .../serviceaccounts/database/store.go | 5 +- .../serviceaccounts/database/store_test.go | 18 ++-- .../extsvcaccounts/service_test.go | 4 +- .../serviceaccounts/manager/service.go | 7 +- .../serviceaccounts/manager/service_test.go | 4 +- .../serviceaccounts/manager/stats_test.go | 4 +- pkg/services/serviceaccounts/models.go | 2 + pkg/services/serviceaccounts/proxy/service.go | 2 +- .../serviceaccounts/proxy/service_test.go | 90 +++++++++---------- 10 files changed, 77 insertions(+), 62 deletions(-) diff --git a/pkg/services/extsvcauth/models.go b/pkg/services/extsvcauth/models.go index c2a71dde453..4891ef930a9 100644 --- a/pkg/services/extsvcauth/models.go +++ b/pkg/services/extsvcauth/models.go @@ -10,7 +10,8 @@ const ( ServiceAccounts AuthProvider = "ServiceAccounts" // TmpOrgID is the orgID we use while global service accounts are not supported. - TmpOrgID int64 = 1 + TmpOrgIDStr string = "1" + TmpOrgID int64 = 1 ) type AuthProvider string diff --git a/pkg/services/serviceaccounts/database/store.go b/pkg/services/serviceaccounts/database/store.go index ebb66ef4842..b99925c9978 100644 --- a/pkg/services/serviceaccounts/database/store.go +++ b/pkg/services/serviceaccounts/database/store.go @@ -45,6 +45,9 @@ 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 +// -- WARNING: +// -- if you change this function you need to change the ExtSvcLoginPrefix as well +// -- to make sure they are not considered as regular service accounts 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 @@ -331,7 +334,7 @@ func (s *ServiceAccountsStoreImpl) SearchOrgServiceAccounts(ctx context.Context, whereConditions = append( whereConditions, "login "+s.sqlStore.GetDialect().LikeStr()+" ?") - whereParams = append(whereParams, serviceaccounts.ServiceAccountPrefix+serviceaccounts.ExtSvcPrefix+"%") + whereParams = append(whereParams, serviceaccounts.ExtSvcLoginPrefix+"%") default: s.log.Warn("Invalid filter user for service account filtering", "service account search filtering", query.Filter) } diff --git a/pkg/services/serviceaccounts/database/store_test.go b/pkg/services/serviceaccounts/database/store_test.go index 42e8da77717..038971d33d0 100644 --- a/pkg/services/serviceaccounts/database/store_test.go +++ b/pkg/services/serviceaccounts/database/store_test.go @@ -447,14 +447,14 @@ func TestStore_MigrateAllApiKeys(t *testing.T) { } func TestServiceAccountsStoreImpl_SearchOrgServiceAccounts(t *testing.T) { initUsers := []tests.TestUser{ - {Name: "satest-1", Role: string(org.RoleViewer), Login: "sa-satest-1", IsServiceAccount: true}, + {Name: "satest-1", Role: string(org.RoleViewer), Login: "sa-1-satest-1", IsServiceAccount: true}, {Name: "usertest-2", Role: string(org.RoleEditor), Login: "usertest-2", IsServiceAccount: false}, - {Name: "satest-3", Role: string(org.RoleEditor), Login: "sa-satest-3", IsServiceAccount: true}, - {Name: "satest-4", Role: string(org.RoleAdmin), Login: "sa-satest-4", IsServiceAccount: true}, - {Name: "extsvc-test-5", Role: string(org.RoleNone), Login: "sa-extsvc-test-5", IsServiceAccount: true}, - {Name: "extsvc-test-6", Role: string(org.RoleNone), Login: "sa-extsvc-test-6", IsServiceAccount: true}, - {Name: "extsvc-test-7", Role: string(org.RoleNone), Login: "sa-extsvc-test-7", IsServiceAccount: true}, - {Name: "extsvc-test-8", Role: string(org.RoleNone), Login: "sa-extsvc-test-8", IsServiceAccount: true}, + {Name: "satest-3", Role: string(org.RoleEditor), Login: "sa-1-satest-3", IsServiceAccount: true}, + {Name: "satest-4", Role: string(org.RoleAdmin), Login: "sa-1-satest-4", IsServiceAccount: true}, + {Name: "extsvc-test-5", Role: string(org.RoleNone), Login: "sa-1-extsvc-test-5", IsServiceAccount: true}, + {Name: "extsvc-test-6", Role: string(org.RoleNone), Login: "sa-1-extsvc-test-6", IsServiceAccount: true}, + {Name: "extsvc-test-7", Role: string(org.RoleNone), Login: "sa-1-extsvc-test-7", IsServiceAccount: true}, + {Name: "extsvc-test-8", Role: string(org.RoleNone), Login: "sa-1-extsvc-test-8", IsServiceAccount: true}, } db, store := setupTestDatabase(t) @@ -522,10 +522,10 @@ func TestServiceAccountsStoreImpl_SearchOrgServiceAccounts(t *testing.T) { expectedCount: 4, }, { - desc: "should return service accounts with sa-satest login", + desc: "should return service accounts with sa-1-satest login", query: &serviceaccounts.SearchOrgServiceAccountsQuery{ OrgID: orgID, - Query: "sa-satest", + Query: "sa-1-satest", SignedInUser: userWithPerm, Filter: serviceaccounts.FilterIncludeAll, }, diff --git a/pkg/services/serviceaccounts/extsvcaccounts/service_test.go b/pkg/services/serviceaccounts/extsvcaccounts/service_test.go index 1624c71ed82..e715dc49a20 100644 --- a/pkg/services/serviceaccounts/extsvcaccounts/service_test.go +++ b/pkg/services/serviceaccounts/extsvcaccounts/service_test.go @@ -448,13 +448,13 @@ func TestExtSvcAccountsService_GetExternalServiceNames(t *testing.T) { sa1 := sa.ServiceAccountDTO{ Id: 1, Name: sa.ExtSvcPrefix + "sa-svc-1", - Login: sa.ServiceAccountPrefix + sa.ExtSvcPrefix + "sa-svc-1", + Login: sa.ExtSvcLoginPrefix + "sa-svc-1", OrgId: extsvcauth.TmpOrgID, } sa2 := sa.ServiceAccountDTO{ Id: 2, Name: sa.ExtSvcPrefix + "sa-svc-2", - Login: sa.ServiceAccountPrefix + sa.ExtSvcPrefix + "sa-svc-2", + Login: sa.ExtSvcLoginPrefix + "sa-svc-2", OrgId: extsvcauth.TmpOrgID, } tests := []struct { diff --git a/pkg/services/serviceaccounts/manager/service.go b/pkg/services/serviceaccounts/manager/service.go index 141e79e1a94..0f9f7a8fc68 100644 --- a/pkg/services/serviceaccounts/manager/service.go +++ b/pkg/services/serviceaccounts/manager/service.go @@ -26,6 +26,7 @@ const ( ) type ServiceAccountsService struct { + acService accesscontrol.Service store store log log.Logger backgroundLog log.Logger @@ -54,6 +55,7 @@ func ProvideServiceAccountsService( orgService, ) s := &ServiceAccountsService{ + acService: accesscontrolService, store: serviceAccountsStore, log: log.New("serviceaccounts"), backgroundLog: log.New("serviceaccounts.background"), @@ -174,7 +176,10 @@ func (sa *ServiceAccountsService) DeleteServiceAccount(ctx context.Context, orgI if err := validServiceAccountID(serviceAccountID); err != nil { return err } - return sa.store.DeleteServiceAccount(ctx, orgID, serviceAccountID) + if err := sa.store.DeleteServiceAccount(ctx, orgID, serviceAccountID); err != nil { + return err + } + return sa.acService.DeleteUserPermissions(ctx, orgID, serviceAccountID) } func (sa *ServiceAccountsService) EnableServiceAccount(ctx context.Context, orgID, serviceAccountID int64, enable bool) error { diff --git a/pkg/services/serviceaccounts/manager/service_test.go b/pkg/services/serviceaccounts/manager/service_test.go index bf0633bf1eb..39744123bfa 100644 --- a/pkg/services/serviceaccounts/manager/service_test.go +++ b/pkg/services/serviceaccounts/manager/service_test.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/extensions/accesscontrol/actest" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/apikey" "github.com/grafana/grafana/pkg/services/org" @@ -116,8 +117,9 @@ func (f *SecretsCheckerFake) CheckTokens(ctx context.Context) error { } func TestProvideServiceAccount_DeleteServiceAccount(t *testing.T) { + acSvc := &actest.FakeService{} storeMock := newServiceAccountStoreFake() - svc := ServiceAccountsService{storeMock, log.New("test"), log.New("background.test"), &SecretsCheckerFake{}, false, 0} + svc := ServiceAccountsService{acSvc, storeMock, log.New("test"), log.New("background.test"), &SecretsCheckerFake{}, false, 0} testOrgId := 1 t.Run("should create service account", func(t *testing.T) { diff --git a/pkg/services/serviceaccounts/manager/stats_test.go b/pkg/services/serviceaccounts/manager/stats_test.go index 3f9099a6f46..7dff6d52447 100644 --- a/pkg/services/serviceaccounts/manager/stats_test.go +++ b/pkg/services/serviceaccounts/manager/stats_test.go @@ -7,13 +7,15 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/extensions/accesscontrol/actest" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/serviceaccounts" ) func Test_UsageStats(t *testing.T) { + acSvc := &actest.FakeService{} storeMock := newServiceAccountStoreFake() - svc := ServiceAccountsService{storeMock, log.New("test"), log.New("background-test"), &SecretsCheckerFake{}, true, 5} + svc := ServiceAccountsService{acSvc, storeMock, log.New("test"), log.New("background-test"), &SecretsCheckerFake{}, true, 5} err := svc.DeleteServiceAccount(context.Background(), 1, 1) require.NoError(t, err) diff --git a/pkg/services/serviceaccounts/models.go b/pkg/services/serviceaccounts/models.go index 71b6aec6ac9..d83d298ea88 100644 --- a/pkg/services/serviceaccounts/models.go +++ b/pkg/services/serviceaccounts/models.go @@ -6,6 +6,7 @@ import ( "github.com/grafana/grafana/pkg/models/roletype" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/auth/identity" + "github.com/grafana/grafana/pkg/services/extsvcauth" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/util/errutil" ) @@ -18,6 +19,7 @@ var ( const ( ServiceAccountPrefix = "sa-" ExtSvcPrefix = "extsvc-" + ExtSvcLoginPrefix = ServiceAccountPrefix + extsvcauth.TmpOrgIDStr + "-" + ExtSvcPrefix ) const ( diff --git a/pkg/services/serviceaccounts/proxy/service.go b/pkg/services/serviceaccounts/proxy/service.go index 66d18da9452..10f2a601dcd 100644 --- a/pkg/services/serviceaccounts/proxy/service.go +++ b/pkg/services/serviceaccounts/proxy/service.go @@ -187,5 +187,5 @@ func isNameValid(name string) bool { } func isExternalServiceAccount(login string) bool { - return strings.HasPrefix(login, serviceaccounts.ServiceAccountPrefix+serviceaccounts.ExtSvcPrefix) + return strings.HasPrefix(login, serviceaccounts.ExtSvcLoginPrefix) } diff --git a/pkg/services/serviceaccounts/proxy/service_test.go b/pkg/services/serviceaccounts/proxy/service_test.go index db0325a2172..45e1875adef 100644 --- a/pkg/services/serviceaccounts/proxy/service_test.go +++ b/pkg/services/serviceaccounts/proxy/service_test.go @@ -8,12 +8,12 @@ import ( "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/infra/log" - "github.com/grafana/grafana/pkg/services/serviceaccounts" + sa "github.com/grafana/grafana/pkg/services/serviceaccounts" "github.com/grafana/grafana/pkg/services/serviceaccounts/extsvcaccounts" "github.com/grafana/grafana/pkg/services/serviceaccounts/tests" ) -var _ serviceaccounts.Service = (*tests.FakeServiceAccountService)(nil) +var _ sa.Service = (*tests.FakeServiceAccountService)(nil) func TestProvideServiceAccount_crudServiceAccount(t *testing.T) { testOrgId := int64(1) @@ -29,19 +29,19 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) { t.Run("should create service account", func(t *testing.T) { testCases := []struct { description string - form serviceaccounts.CreateServiceAccountForm + form sa.CreateServiceAccountForm expectedError error }{ { description: "should create service account and not return error", - form: serviceaccounts.CreateServiceAccountForm{ + form: sa.CreateServiceAccountForm{ Name: "my-service-account", }, expectedError: nil, }, { description: "should not allow to create a service account with extsvc prefix", - form: serviceaccounts.CreateServiceAccountForm{ + form: sa.CreateServiceAccountForm{ Name: "extsvc-my-service-account", }, expectedError: extsvcaccounts.ErrInvalidName, @@ -61,20 +61,20 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) { testCases := []struct { description string expectedError error - expectedServiceAccount *serviceaccounts.ServiceAccountProfileDTO + expectedServiceAccount *sa.ServiceAccountProfileDTO }{ { description: "should allow to delete a service account", expectedError: nil, - expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ + expectedServiceAccount: &sa.ServiceAccountProfileDTO{ Login: "my-service-account", }, }, { - description: "should not allow to delete a service account with sa-extsvc prefix", + description: "should not allow to delete a service account with " + sa.ExtSvcLoginPrefix + " prefix", expectedError: extsvcaccounts.ErrCannotBeDeleted, - expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ - Login: "sa-extsvc-my-service-account", + expectedServiceAccount: &sa.ServiceAccountProfileDTO{ + Login: sa.ExtSvcLoginPrefix + "my-service-account", }, }, } @@ -88,24 +88,24 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) { } }) - t.Run("should delete service account", func(t *testing.T) { + t.Run("should delete service account token", func(t *testing.T) { testCases := []struct { description string expectedError error - expectedServiceAccount *serviceaccounts.ServiceAccountProfileDTO + expectedServiceAccount *sa.ServiceAccountProfileDTO }{ { description: "should allow to delete a service account token", expectedError: nil, - expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ + expectedServiceAccount: &sa.ServiceAccountProfileDTO{ Login: "my-service-account", }, }, { description: "should not allow to delete a external service account token", expectedError: extsvcaccounts.ErrCannotDeleteToken, - expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ - Login: "sa-extsvc-my-service-account", + expectedServiceAccount: &sa.ServiceAccountProfileDTO{ + Login: sa.ExtSvcLoginPrefix + "my-service-account", }, }, } @@ -122,20 +122,20 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) { t.Run("should retrieve service account with IsExternal field", func(t *testing.T) { testCases := []struct { description string - expectedServiceAccount *serviceaccounts.ServiceAccountProfileDTO + expectedServiceAccount *sa.ServiceAccountProfileDTO expectedIsExternal bool }{ { description: "should not mark as external", - expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ + expectedServiceAccount: &sa.ServiceAccountProfileDTO{ Login: "my-service-account", }, expectedIsExternal: false, }, { description: "should mark as external", - expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ - Login: "sa-extsvc-my-service-account", + expectedServiceAccount: &sa.ServiceAccountProfileDTO{ + Login: sa.ExtSvcLoginPrefix + "my-service-account", }, expectedIsExternal: true, }, @@ -151,17 +151,17 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) { } }) - t.Run("should mark external service accounts correctly", func(t *testing.T) { - serviceMock.ExpectedSearchOrgServiceAccountsResult = &serviceaccounts.SearchOrgServiceAccountsResult{ + t.Run("should flag external service accounts correctly", func(t *testing.T) { + serviceMock.ExpectedSearchOrgServiceAccountsResult = &sa.SearchOrgServiceAccountsResult{ TotalCount: 2, - ServiceAccounts: []*serviceaccounts.ServiceAccountDTO{ + ServiceAccounts: []*sa.ServiceAccountDTO{ {Login: "test"}, - {Login: serviceaccounts.ServiceAccountPrefix + serviceaccounts.ExtSvcPrefix + "test"}, + {Login: sa.ExtSvcLoginPrefix + "test"}, }, Page: 1, PerPage: 2, } - res, err := svc.SearchOrgServiceAccounts(context.Background(), &serviceaccounts.SearchOrgServiceAccountsQuery{OrgID: 1}) + res, err := svc.SearchOrgServiceAccounts(context.Background(), &sa.SearchOrgServiceAccountsQuery{OrgID: 1}) require.Len(t, res.ServiceAccounts, 2) require.NoError(t, err) require.False(t, res.ServiceAccounts[0].IsExternal) @@ -173,47 +173,47 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) { nameWithProtectedPrefix := "extsvc-my-updated-service-account" testCases := []struct { description string - form serviceaccounts.UpdateServiceAccountForm - expectedServiceAccount *serviceaccounts.ServiceAccountProfileDTO + form sa.UpdateServiceAccountForm + expectedServiceAccount *sa.ServiceAccountProfileDTO expectedError error }{ { description: "should update a non-external service account with a valid name", - form: serviceaccounts.UpdateServiceAccountForm{ + form: sa.UpdateServiceAccountForm{ Name: &nameWithoutProtectedPrefix, }, - expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ + expectedServiceAccount: &sa.ServiceAccountProfileDTO{ Login: "my-service-account", }, expectedError: nil, }, { description: "should not allow to update a non-external service account with extsvc prefix", - form: serviceaccounts.UpdateServiceAccountForm{ + form: sa.UpdateServiceAccountForm{ Name: &nameWithProtectedPrefix, }, - expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ + expectedServiceAccount: &sa.ServiceAccountProfileDTO{ Login: "my-service-account", }, expectedError: extsvcaccounts.ErrInvalidName, }, { description: "should not allow to update an external service account with a valid name", - form: serviceaccounts.UpdateServiceAccountForm{ + form: sa.UpdateServiceAccountForm{ Name: &nameWithoutProtectedPrefix, }, - expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ - Login: "sa-extsvc-my-service-account", + expectedServiceAccount: &sa.ServiceAccountProfileDTO{ + Login: sa.ExtSvcLoginPrefix + "my-service-account", }, expectedError: extsvcaccounts.ErrCannotBeUpdated, }, { description: "should not allow to update an external service account with a extsvc prefix", - form: serviceaccounts.UpdateServiceAccountForm{ + form: sa.UpdateServiceAccountForm{ Name: &nameWithProtectedPrefix, }, - expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ - Login: "sa-extsvc-my-service-account", + expectedServiceAccount: &sa.ServiceAccountProfileDTO{ + Login: sa.ExtSvcLoginPrefix + "my-service-account", }, expectedError: extsvcaccounts.ErrInvalidName, }, @@ -232,27 +232,27 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) { t.Run("should add service account tokens", func(t *testing.T) { testCases := []struct { description string - cmd serviceaccounts.AddServiceAccountTokenCommand - expectedServiceAccount *serviceaccounts.ServiceAccountProfileDTO + cmd sa.AddServiceAccountTokenCommand + expectedServiceAccount *sa.ServiceAccountProfileDTO expectedError error }{ { description: "should allow to create a service account token", - cmd: serviceaccounts.AddServiceAccountTokenCommand{ + cmd: sa.AddServiceAccountTokenCommand{ OrgId: testOrgId, }, - expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ + expectedServiceAccount: &sa.ServiceAccountProfileDTO{ Login: "my-service-account", }, expectedError: nil, }, { description: "should not allow to create a service account token", - cmd: serviceaccounts.AddServiceAccountTokenCommand{ + cmd: sa.AddServiceAccountTokenCommand{ OrgId: testOrgId, }, - expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ - Login: "sa-extsvc-my-service-account", + expectedServiceAccount: &sa.ServiceAccountProfileDTO{ + Login: sa.ExtSvcLoginPrefix + "my-service-account", }, expectedError: extsvcaccounts.ErrCannotCreateToken, }, @@ -271,7 +271,7 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) { t.Run("should identify service account logins for being external or not", func(t *testing.T) { assert.False(t, isExternalServiceAccount("my-service-account")) assert.False(t, isExternalServiceAccount("sa-my-service-account")) - assert.False(t, isExternalServiceAccount("extsvc-my-service-account")) - assert.True(t, isExternalServiceAccount("sa-extsvc-my-service-account")) + assert.False(t, isExternalServiceAccount(sa.ExtSvcPrefix+"my-service-account")) // It's not a external service account login + assert.True(t, isExternalServiceAccount(sa.ExtSvcLoginPrefix+"my-service-account")) }) }