IntenseWebs/grafana
3
0
Fork 0
mirror of https://github.com/grafana/grafana.git synced 2025-02-25 18:55:37 -06:00
Code Issues Packages Projects Releases Wiki Activity

ExtSvcAccounts: FIX prevent service account deletion (#84502)

Browse Source 
* ExtSvcAccounts: Fix External Service Accounts Login check

Co-authored-by: Karl Persson <kalle.persson@grafana.com>

* 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 <kalle.persson@grafana.com>
This commit is contained in:
Gabriel MABILLE 2024-03-14 19:11:02 +01:00 committed by GitHub
parent 827860d459
commit 2795f9827a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 77 additions and 62 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
pkg/services/extsvcauth/models.go
View File

@ -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

5
pkg/services/serviceaccounts/database/store.go
View File

@ -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)
}

18
pkg/services/serviceaccounts/database/store_test.go
View File

@ -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,
},

4
pkg/services/serviceaccounts/extsvcaccounts/service_test.go
View File

@ -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 {

7
pkg/services/serviceaccounts/manager/service.go
View File

@ -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 {

4
pkg/services/serviceaccounts/manager/service_test.go
View File

@ -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) {

4
pkg/services/serviceaccounts/manager/stats_test.go
View File

@ -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)

2
pkg/services/serviceaccounts/models.go
View File

@ -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 (

2
pkg/services/serviceaccounts/proxy/service.go
View File

@ -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)
}

90
pkg/services/serviceaccounts/proxy/service_test.go
View File

@ -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"))
})
}