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

AuthN: Introduce DefaultOrgID function for managed service accounts (#93432)

Browse Source 
* Managed Service Accounts: Use AutoAssignOrgID

* Fix the IsExternalServiceAccount function

* Reassign service account role

* Account for AutoAssignOrg

* Update pkg/services/serviceaccounts/models.go

* Simplify IsExternalServiceAccount function

* Add tests

* Easier to understand test

* Revert small change
This commit is contained in:
Gabriel MABILLE 2024-09-20 14:43:29 +02:00 committed by GitHub
parent bb69afed7c
commit 8d84517103
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
12 changed files with 192 additions and 122 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
pkg/services/accesscontrol/database/externalservices.go
View File

@ -240,6 +240,15 @@ func (*AccessControlStore) saveUserAssignment(ctx context.Context, sess *db.Sess
return errGetAssigns
}
// Revoke assignment if it's assigned to another user or service account
if len(assignments) > 0 && assignments[0].UserID != assignment.UserID {
if _, errDel := sess.Where("role_id = ?", assignment.RoleID).Delete(&accesscontrol.UserRole{}); errDel != nil {
return errDel
}
assignments = nil
}
// If no assignment exists, insert a new one.
if len(assignments) == 0 {
if _, errInsert := sess.Insert(&assignment); errInsert != nil {
return errInsert
@ -247,11 +256,6 @@ func (*AccessControlStore) saveUserAssignment(ctx context.Context, sess *db.Sess
return nil
}
// Ensure the role was assigned only to this service account
if len(assignments) > 1 || assignments[0].UserID != assignment.UserID {
return errors.New("external service role assigned to another user or service account")
}
// Ensure the assignment is in the correct organization
_, errUpdate := sess.Where("role_id = ? AND user_id = ?", assignment.RoleID, assignment.UserID).MustCols("org_id").Update(&assignment)
return errUpdate

4
pkg/services/accesscontrol/database/externalservices_test.go
View File

@ -103,10 +103,10 @@ func TestAccessControlStore_SaveExternalServiceRole(t *testing.T) {
{
cmd: accesscontrol.SaveExternalServiceRoleCommand{
ExternalServiceID: "app1",
AssignmentOrgID: 1,
AssignmentOrgID: 2,
ServiceAccountID: 2,
},
wantErr: true,
wantErr: false,
},
},
},

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

@ -4,16 +4,21 @@ import (
"context"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/setting"
)
const (
ServiceAccounts AuthProvider = "ServiceAccounts"
// TmpOrgID is the orgID we use while global service accounts are not supported.
TmpOrgIDStr string = "1"
TmpOrgID int64 = 1
)
func DefaultOrgID(cfg *setting.Cfg) int64 {
orgID := int64(1)
if cfg.AutoAssignOrg && cfg.AutoAssignOrgId > 0 {
orgID = int64(cfg.AutoAssignOrgId)
}
return orgID
}
type AuthProvider string
//go:generate mockery --name ExternalServiceRegistry --structname ExternalServiceRegistryMock --output tests --outpkg tests --filename extsvcregmock.go

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

@ -334,7 +334,7 @@ func (s *ServiceAccountsStoreImpl) SearchOrgServiceAccounts(ctx context.Context,
whereConditions = append(
whereConditions,
"login "+s.sqlStore.GetDialect().LikeStr()+" ?")
whereParams = append(whereParams, serviceaccounts.ExtSvcLoginPrefix+"%")
whereParams = append(whereParams, serviceaccounts.ExtSvcLoginPrefix(query.OrgID)+"%")
default:
s.log.Warn("Invalid filter user for service account filtering", "service account search filtering", query.Filter)
}

7
pkg/services/serviceaccounts/extsvcaccounts/metrics.go
View File

@ -5,7 +5,6 @@ import (
"time"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/services/extsvcauth"
"github.com/grafana/grafana/pkg/services/serviceaccounts"
"github.com/prometheus/client_golang/prometheus"
)
@ -16,7 +15,7 @@ type metrics struct {
deletedCount prometheus.Counter
}
func newMetrics(reg prometheus.Registerer, saSvc serviceaccounts.Service, logger log.Logger) *metrics {
func newMetrics(reg prometheus.Registerer, defaultOrgID int64, saSvc serviceaccounts.Service, logger log.Logger) *metrics {
var m metrics
m.storedCount = prometheus.NewGaugeFunc(
@ -29,10 +28,10 @@ func newMetrics(reg prometheus.Registerer, saSvc serviceaccounts.Service, logger
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
res, err := saSvc.SearchOrgServiceAccounts(ctx, &serviceaccounts.SearchOrgServiceAccountsQuery{
OrgID: extsvcauth.TmpOrgID,
OrgID: defaultOrgID,
Filter: serviceaccounts.FilterOnlyExternal,
CountOnly: true,
SignedInUser: extsvcuser,
SignedInUser: extsvcuser(defaultOrgID),
})
if err != nil {
logger.Error("Could not compute extsvc_total metric", "error", err)

11
pkg/services/serviceaccounts/extsvcaccounts/models.go
View File

@ -4,7 +4,6 @@ import (
"github.com/grafana/grafana/pkg/apimachinery/errutil"
"github.com/grafana/grafana/pkg/apimachinery/identity"
ac "github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/extsvcauth"
"github.com/grafana/grafana/pkg/services/serviceaccounts"
"github.com/grafana/grafana/pkg/services/user"
)
@ -28,14 +27,16 @@ var (
ErrCredentialsGenFailed = errutil.Internal("extsvcaccounts.ErrCredentialsGenFailed")
ErrCredentialsNotFound = errutil.NotFound("extsvcaccounts.ErrCredentialsNotFound")
ErrInvalidName = errutil.BadRequest("extsvcaccounts.ErrInvalidName", errutil.WithPublicMessage("only external service account names can be prefixed with 'extsvc-'"))
)
extsvcuser = &user.SignedInUser{
OrgID: extsvcauth.TmpOrgID,
func extsvcuser(orgID int64) *user.SignedInUser {
return &user.SignedInUser{
OrgID: orgID,
Permissions: map[int64]map[string][]string{
extsvcauth.TmpOrgID: {serviceaccounts.ActionRead: {"serviceaccounts:id:*"}},
orgID: {serviceaccounts.ActionRead: {"serviceaccounts:id:*"}},
},
}
)
}
// Credentials represents the credentials associated to an external service
type Credentials struct {

45
pkg/services/serviceaccounts/extsvcaccounts/service.go
View File

@ -22,32 +22,35 @@ import (
"github.com/grafana/grafana/pkg/services/secrets/kvstore"
sa "github.com/grafana/grafana/pkg/services/serviceaccounts"
"github.com/grafana/grafana/pkg/services/serviceaccounts/manager"
"github.com/grafana/grafana/pkg/setting"
)
type ExtSvcAccountsService struct {
acSvc ac.Service
features featuremgmt.FeatureToggles
logger log.Logger
metrics *metrics
saSvc sa.Service
skvStore kvstore.SecretsKVStore
tracer tracing.Tracer
acSvc ac.Service
defaultOrgID int64
features featuremgmt.FeatureToggles
logger log.Logger
metrics *metrics
saSvc sa.Service
skvStore kvstore.SecretsKVStore
tracer tracing.Tracer
}
func ProvideExtSvcAccountsService(acSvc ac.Service, bus bus.Bus, db db.DB, features featuremgmt.FeatureToggles, reg prometheus.Registerer, saSvc *manager.ServiceAccountsService, secretsSvc secrets.Service, tracer tracing.Tracer) *ExtSvcAccountsService {
func ProvideExtSvcAccountsService(acSvc ac.Service, cfg *setting.Cfg, bus bus.Bus, db db.DB, features featuremgmt.FeatureToggles, reg prometheus.Registerer, saSvc *manager.ServiceAccountsService, secretsSvc secrets.Service, tracer tracing.Tracer) *ExtSvcAccountsService {
logger := log.New("serviceauth.extsvcaccounts")
esa := &ExtSvcAccountsService{
acSvc: acSvc,
logger: logger,
saSvc: saSvc,
features: features,
skvStore: kvstore.NewSQLSecretsKVStore(db, secretsSvc, logger), // Using SQL store to avoid a cyclic dependency
tracer: tracer,
acSvc: acSvc,
defaultOrgID: extsvcauth.DefaultOrgID(cfg),
logger: logger,
saSvc: saSvc,
features: features,
skvStore: kvstore.NewSQLSecretsKVStore(db, secretsSvc, logger), // Using SQL store to avoid a cyclic dependency
tracer: tracer,
}
if features.IsEnabledGlobally(featuremgmt.FlagExternalServiceAccounts) {
// Register the metrics
esa.metrics = newMetrics(reg, saSvc, logger)
esa.metrics = newMetrics(reg, esa.defaultOrgID, saSvc, logger)
// Register a listener to enable/disable service accounts
bus.AddEventListener(esa.handlePluginStateChanged)
@ -78,7 +81,7 @@ func (esa *ExtSvcAccountsService) HasExternalService(ctx context.Context, name s
saName := sa.ExtSvcPrefix + slugify.Slugify(name)
saID, errRetrieve := esa.saSvc.RetrieveServiceAccountIdByName(ctx, extsvcauth.TmpOrgID, saName)
saID, errRetrieve := esa.saSvc.RetrieveServiceAccountIdByName(ctx, esa.defaultOrgID, saName)
if errRetrieve != nil && !errors.Is(errRetrieve, sa.ErrServiceAccountNotFound) {
return false, errRetrieve
}
@ -114,9 +117,9 @@ func (esa *ExtSvcAccountsService) GetExternalServiceNames(ctx context.Context) (
ctxLogger.Debug("Get external service names from store")
sas, err := esa.saSvc.SearchOrgServiceAccounts(ctx, &sa.SearchOrgServiceAccountsQuery{
OrgID: extsvcauth.TmpOrgID,
OrgID: esa.defaultOrgID,
Filter: sa.FilterOnlyExternal,
SignedInUser: extsvcuser,
SignedInUser: extsvcuser(esa.defaultOrgID),
})
if err != nil {
ctxLogger.Error("Could not fetch external service accounts from store", "error", err.Error())
@ -155,7 +158,7 @@ func (esa *ExtSvcAccountsService) SaveExternalService(ctx context.Context, cmd *
saID, err := esa.ManageExtSvcAccount(ctx, &sa.ManageExtSvcAccountCmd{
ExtSvcSlug: slug,
Enabled: cmd.Self.Enabled,
OrgID: extsvcauth.TmpOrgID,
OrgID: esa.defaultOrgID,
Permissions: cmd.Self.Permissions,
})
if err != nil {
@ -168,7 +171,7 @@ func (esa *ExtSvcAccountsService) SaveExternalService(ctx context.Context, cmd *
return nil, nil
}
token, err := esa.getExtSvcAccountToken(ctx, extsvcauth.TmpOrgID, saID, slug)
token, err := esa.getExtSvcAccountToken(ctx, esa.defaultOrgID, saID, slug)
if err != nil {
ctxLogger.Error("Could not get the external svc token",
"service", slug,
@ -189,7 +192,7 @@ func (esa *ExtSvcAccountsService) RemoveExternalService(ctx context.Context, nam
ctx, span := esa.tracer.Start(ctx, "ExtSvcAccountsService.RemoveExternalService")
defer span.End()
return esa.RemoveExtSvcAccount(ctx, extsvcauth.TmpOrgID, slugify.Slugify(name))
return esa.RemoveExtSvcAccount(ctx, esa.defaultOrgID, slugify.Slugify(name))
}
func (esa *ExtSvcAccountsService) RemoveExtSvcAccount(ctx context.Context, orgID int64, extSvcSlug string) error {

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

@ -26,6 +26,10 @@ import (
"github.com/grafana/grafana/pkg/setting"
)
var (
autoAssignOrgID = int64(2)
)
type TestEnv struct {
S *ExtSvcAccountsService
AcStore *actest.MockStore
@ -51,12 +55,13 @@ func setupTestEnv(t *testing.T) *TestEnv {
localcache.New(0, 0), fmgt, tracing.InitializeTracerForTest(), nil, nil,
permreg.ProvidePermissionRegistry(),
),
features: fmgt,
logger: logger,
metrics: newMetrics(nil, env.SaSvc, logger),
saSvc: env.SaSvc,
skvStore: env.SkvStore,
tracer: tracing.InitializeTracerForTest(),
defaultOrgID: autoAssignOrgID,
features: fmgt,
logger: logger,
metrics: newMetrics(nil, autoAssignOrgID, env.SaSvc, logger),
saSvc: env.SaSvc,
skvStore: env.SkvStore,
tracer: tracing.InitializeTracerForTest(),
}
return env
}
@ -207,14 +212,13 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) {
extSvcSlug := "grafana-test-app"
validToken, err := satokengen.New(extSvcSlug)
require.NoError(t, err, "failed to generate a valid token for the tests")
tmpOrgID := int64(1)
extSvcAccID := int64(10)
extSvcPerms := []ac.Permission{{Action: ac.ActionUsersRead, Scope: ac.ScopeUsersAll}}
extSvcAccount := &sa.ServiceAccountDTO{
Id: extSvcAccID,
Name: extSvcSlug,
Login: extSvcSlug,
OrgId: tmpOrgID,
OrgId: autoAssignOrgID,
IsDisabled: false,
Role: string(identity.RoleNone),
}
@ -231,19 +235,19 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) {
name: "should disable service account",
init: func(env *TestEnv) {
// A previous service account was attached to this slug
env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, tmpOrgID, sa.ExtSvcPrefix+extSvcSlug).
env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, autoAssignOrgID, sa.ExtSvcPrefix+extSvcSlug).
Return(extSvcAccID, nil)
env.SaSvc.On("EnableServiceAccount", mock.Anything, tmpOrgID, extSvcAccID, false).Return(nil)
env.SaSvc.On("EnableServiceAccount", mock.Anything, autoAssignOrgID, extSvcAccID, false).Return(nil)
env.AcStore.On("SaveExternalServiceRole",
mock.Anything,
mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool {
return cmd.ServiceAccountID == extSvcAccID && cmd.ExternalServiceID == extSvcSlug &&
cmd.AssignmentOrgID == tmpOrgID && len(cmd.Permissions) == 1 &&
cmd.AssignmentOrgID == autoAssignOrgID && len(cmd.Permissions) == 1 &&
cmd.Permissions[0] == extSvcPerms[0]
})).
Return(nil)
// A token was previously stored in the secret store
_ = env.SkvStore.Set(context.Background(), tmpOrgID, extSvcSlug, kvStoreType, validToken.ClientSecret)
_ = env.SkvStore.Set(context.Background(), autoAssignOrgID, extSvcSlug, kvStoreType, validToken.ClientSecret)
},
cmd: extsvcauth.ExternalServiceRegistration{
Name: extSvcSlug,
@ -253,7 +257,7 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) {
},
},
checks: func(t *testing.T, env *TestEnv) {
_, ok, _ := env.SkvStore.Get(context.Background(), tmpOrgID, extSvcSlug, kvStoreType)
_, ok, _ := env.SkvStore.Get(context.Background(), autoAssignOrgID, extSvcSlug, kvStoreType)
require.True(t, ok, "secret should have been kept in store")
},
want: &extsvcauth.ExternalService{
@ -267,12 +271,12 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) {
name: "should remove service account when no permission",
init: func(env *TestEnv) {
// A previous service account was attached to this slug
env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, tmpOrgID, sa.ExtSvcPrefix+extSvcSlug).
env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, autoAssignOrgID, sa.ExtSvcPrefix+extSvcSlug).
Return(extSvcAccID, nil)
env.SaSvc.On("DeleteServiceAccount", mock.Anything, tmpOrgID, extSvcAccID).Return(nil)
env.SaSvc.On("DeleteServiceAccount", mock.Anything, autoAssignOrgID, extSvcAccID).Return(nil)
env.AcStore.On("DeleteExternalServiceRole", mock.Anything, extSvcSlug).Return(nil)
// A token was previously stored in the secret store
_ = env.SkvStore.Set(context.Background(), tmpOrgID, extSvcSlug, kvStoreType, validToken.ClientSecret)
_ = env.SkvStore.Set(context.Background(), autoAssignOrgID, extSvcSlug, kvStoreType, validToken.ClientSecret)
},
cmd: extsvcauth.ExternalServiceRegistration{
Name: extSvcSlug,
@ -282,7 +286,7 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) {
},
},
checks: func(t *testing.T, env *TestEnv) {
_, ok, _ := env.SkvStore.Get(context.Background(), tmpOrgID, extSvcSlug, kvStoreType)
_, ok, _ := env.SkvStore.Get(context.Background(), autoAssignOrgID, extSvcSlug, kvStoreType)
require.False(t, ok, "secret should have been removed from store")
},
want: nil,
@ -292,23 +296,23 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) {
name: "should create new service account",
init: func(env *TestEnv) {
// No previous service account was attached to this slug
env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, tmpOrgID, sa.ExtSvcPrefix+extSvcSlug).
env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, autoAssignOrgID, sa.ExtSvcPrefix+extSvcSlug).
Return(int64(0), sa.ErrServiceAccountNotFound.Errorf("mock"))
env.SaSvc.On("CreateServiceAccount",
mock.Anything,
tmpOrgID,
autoAssignOrgID,
mock.MatchedBy(func(cmd *sa.CreateServiceAccountForm) bool {
return cmd.Name == sa.ExtSvcPrefix+extSvcSlug && *cmd.Role == identity.RoleNone
})).
Return(extSvcAccount, nil)
env.SaSvc.On("EnableServiceAccount", mock.Anything, tmpOrgID, extSvcAccID, true).Return(nil)
env.SaSvc.On("EnableServiceAccount", mock.Anything, autoAssignOrgID, extSvcAccID, true).Return(nil)
// Api Key was added without problem
env.SaSvc.On("AddServiceAccountToken", mock.Anything, mock.Anything, mock.Anything).Return(&apikey.APIKey{}, nil)
env.AcStore.On("SaveExternalServiceRole",
mock.Anything,
mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool {
return cmd.ServiceAccountID == extSvcAccount.Id && cmd.ExternalServiceID == extSvcSlug &&
cmd.AssignmentOrgID == tmpOrgID && len(cmd.Permissions) == 1 &&
cmd.AssignmentOrgID == autoAssignOrgID && len(cmd.Permissions) == 1 &&
cmd.Permissions[0] == extSvcPerms[0]
})).
Return(nil)
@ -331,19 +335,19 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) {
name: "should update service account",
init: func(env *TestEnv) {
// A previous service account was attached to this slug
env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, tmpOrgID, sa.ExtSvcPrefix+extSvcSlug).
env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, autoAssignOrgID, sa.ExtSvcPrefix+extSvcSlug).
Return(int64(11), nil)
env.AcStore.On("SaveExternalServiceRole",
mock.Anything,
mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool {
return cmd.ServiceAccountID == int64(11) && cmd.ExternalServiceID == extSvcSlug &&
cmd.AssignmentOrgID == tmpOrgID && len(cmd.Permissions) == 1 &&
cmd.AssignmentOrgID == autoAssignOrgID && len(cmd.Permissions) == 1 &&
cmd.Permissions[0] == extSvcPerms[0]
})).
Return(nil)
env.SaSvc.On("EnableServiceAccount", mock.Anything, extsvcauth.TmpOrgID, int64(11), true).Return(nil)
env.SaSvc.On("EnableServiceAccount", mock.Anything, autoAssignOrgID, int64(11), true).Return(nil)
// This time we don't add a token but rely on the secret store
_ = env.SkvStore.Set(context.Background(), tmpOrgID, extSvcSlug, kvStoreType, validToken.ClientSecret)
_ = env.SkvStore.Set(context.Background(), autoAssignOrgID, extSvcSlug, kvStoreType, validToken.ClientSecret)
},
cmd: extsvcauth.ExternalServiceRegistration{
Name: extSvcSlug,
@ -363,20 +367,20 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) {
name: "should recover from a failed token encryption",
init: func(env *TestEnv) {
// A previous service account was attached to this slug
env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, tmpOrgID, sa.ExtSvcPrefix+extSvcSlug).
env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, autoAssignOrgID, sa.ExtSvcPrefix+extSvcSlug).
Return(int64(11), nil)
env.AcStore.On("SaveExternalServiceRole",
mock.Anything,
mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool {
return cmd.ServiceAccountID == int64(11) && cmd.ExternalServiceID == extSvcSlug &&
cmd.AssignmentOrgID == tmpOrgID && len(cmd.Permissions) == 1 &&
cmd.AssignmentOrgID == autoAssignOrgID && len(cmd.Permissions) == 1 &&
cmd.Permissions[0] == extSvcPerms[0]
})).
Return(nil)
env.SaSvc.On("EnableServiceAccount", mock.Anything, extsvcauth.TmpOrgID, int64(11), true).Return(nil)
env.SaSvc.On("EnableServiceAccount", mock.Anything, autoAssignOrgID, int64(11), true).Return(nil)
// This time the token in the secret store is invalid
_ = env.SkvStore.Set(context.Background(), tmpOrgID, extSvcSlug, kvStoreType, "failedTokenEncryption")
_ = env.SkvStore.Set(context.Background(), autoAssignOrgID, extSvcSlug, kvStoreType, "failedTokenEncryption")
// Delete the API key and entry in the skv store
env.SaSvc.On("ListTokens", mock.Anything, mock.Anything).
Return([]apikey.APIKey{{ID: 3, Name: tokenNamePrefix + "-" + extSvcSlug}}, nil)
@ -386,7 +390,7 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) {
},
checks: func(t *testing.T, env *TestEnv) {
// Make sure the secret was updated
v, ok, _ := env.SkvStore.Get(context.Background(), tmpOrgID, extSvcSlug, kvStoreType)
v, ok, _ := env.SkvStore.Get(context.Background(), autoAssignOrgID, extSvcSlug, kvStoreType)
require.True(t, ok, "secret should have been removed from store")
require.NotEqual(t, "failedTokenEncryption", v)
},
@ -439,7 +443,7 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) {
func TestExtSvcAccountsService_RemoveExtSvcAccount(t *testing.T) {
extSvcSlug := "grafana-test-app"
tmpOrgID := int64(1)
autoAssignOrgID := int64(1)
extSvcAccID := int64(10)
tests := []struct {
name string
@ -452,7 +456,7 @@ func TestExtSvcAccountsService_RemoveExtSvcAccount(t *testing.T) {
name: "should not fail if the service account does not exist",
init: func(env *TestEnv) {
// No previous service account was attached to this slug
env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, tmpOrgID, sa.ExtSvcPrefix+extSvcSlug).
env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, autoAssignOrgID, sa.ExtSvcPrefix+extSvcSlug).
Return(int64(0), sa.ErrServiceAccountNotFound.Errorf("not found"))
},
slug: extSvcSlug,
@ -462,16 +466,16 @@ func TestExtSvcAccountsService_RemoveExtSvcAccount(t *testing.T) {
name: "should remove service account",
init: func(env *TestEnv) {
// A previous service account was attached to this slug
env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, tmpOrgID, sa.ExtSvcPrefix+extSvcSlug).
env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, autoAssignOrgID, sa.ExtSvcPrefix+extSvcSlug).
Return(extSvcAccID, nil)
env.SaSvc.On("DeleteServiceAccount", mock.Anything, tmpOrgID, extSvcAccID).Return(nil)
env.SaSvc.On("DeleteServiceAccount", mock.Anything, autoAssignOrgID, extSvcAccID).Return(nil)
env.AcStore.On("DeleteExternalServiceRole", mock.Anything, extSvcSlug).Return(nil)
// A token was previously stored in the secret store
_ = env.SkvStore.Set(context.Background(), tmpOrgID, extSvcSlug, kvStoreType, "ExtSvcSecretToken")
_ = env.SkvStore.Set(context.Background(), autoAssignOrgID, extSvcSlug, kvStoreType, "ExtSvcSecretToken")
},
slug: extSvcSlug,
checks: func(t *testing.T, env *TestEnv) {
_, ok, _ := env.SkvStore.Get(context.Background(), tmpOrgID, extSvcSlug, kvStoreType)
_, ok, _ := env.SkvStore.Get(context.Background(), autoAssignOrgID, extSvcSlug, kvStoreType)
require.False(t, ok, "secret should have been removed from store")
},
want: nil,
@ -486,7 +490,7 @@ func TestExtSvcAccountsService_RemoveExtSvcAccount(t *testing.T) {
tt.init(env)
}
err := env.S.RemoveExtSvcAccount(ctx, tmpOrgID, tt.slug)
err := env.S.RemoveExtSvcAccount(ctx, autoAssignOrgID, tt.slug)
require.NoError(t, err)
if tt.checks != nil {
@ -501,15 +505,15 @@ func TestExtSvcAccountsService_RemoveExtSvcAccount(t *testing.T) {
func TestExtSvcAccountsService_GetExternalServiceNames(t *testing.T) {
sa1 := sa.ServiceAccountDTO{
Id: 1,
Name: sa.ExtSvcPrefix + "sa-svc-1",
Login: sa.ExtSvcLoginPrefix + "sa-svc-1",
OrgId: extsvcauth.TmpOrgID,
Name: sa.ExtSvcPrefix + "svc-1",
Login: sa.ExtSvcLoginPrefix(autoAssignOrgID) + "svc-1",
OrgId: autoAssignOrgID,
}
sa2 := sa.ServiceAccountDTO{
Id: 2,
Name: sa.ExtSvcPrefix + "sa-svc-2",
Login: sa.ExtSvcLoginPrefix + "sa-svc-2",
OrgId: extsvcauth.TmpOrgID,
Name: sa.ExtSvcPrefix + "svc-2",
Login: sa.ExtSvcLoginPrefix(autoAssignOrgID) + "svc-2",
OrgId: autoAssignOrgID,
}
tests := []struct {
name string
@ -520,7 +524,7 @@ func TestExtSvcAccountsService_GetExternalServiceNames(t *testing.T) {
name: "should return names",
init: func(env *TestEnv) {
env.SaSvc.On("SearchOrgServiceAccounts", mock.Anything, mock.MatchedBy(func(cmd *sa.SearchOrgServiceAccountsQuery) bool {
return cmd.OrgID == extsvcauth.TmpOrgID &&
return cmd.OrgID == autoAssignOrgID &&
cmd.Filter == sa.FilterOnlyExternal &&
len(cmd.SignedInUser.GetPermissions()[sa.ActionRead]) > 0
})).Return(&sa.SearchOrgServiceAccountsResult{
@ -530,13 +534,13 @@ func TestExtSvcAccountsService_GetExternalServiceNames(t *testing.T) {
PerPage: 2,
}, nil)
},
want: []string{"sa-svc-1", "sa-svc-2"},
want: []string{"svc-1", "svc-2"},
},
{
name: "should handle nil search",
init: func(env *TestEnv) {
env.SaSvc.On("SearchOrgServiceAccounts", mock.Anything, mock.MatchedBy(func(cmd *sa.SearchOrgServiceAccountsQuery) bool {
return cmd.OrgID == extsvcauth.TmpOrgID &&
return cmd.OrgID == autoAssignOrgID &&
cmd.Filter == sa.FilterOnlyExternal &&
len(cmd.SignedInUser.GetPermissions()[sa.ActionRead]) > 0
})).Return(nil, nil)

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

@ -1,12 +1,13 @@
package serviceaccounts
import (
"fmt"
"strings"
"time"
"github.com/grafana/grafana/pkg/apimachinery/errutil"
"github.com/grafana/grafana/pkg/apimachinery/identity"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/extsvcauth"
"github.com/grafana/grafana/pkg/services/org"
)
@ -18,7 +19,6 @@ var (
const (
ServiceAccountPrefix = "sa-"
ExtSvcPrefix = "extsvc-"
ExtSvcLoginPrefix = ServiceAccountPrefix + extsvcauth.TmpOrgIDStr + "-" + ExtSvcPrefix
)
const (
@ -206,3 +206,16 @@ var AccessEvaluator = accesscontrol.EvalAny(
accesscontrol.EvalPermission(ActionRead),
accesscontrol.EvalPermission(ActionCreate),
)
func ExtSvcLoginPrefix(orgID int64) string {
return fmt.Sprintf("%s%d-%s", ServiceAccountPrefix, orgID, ExtSvcPrefix)
}
func IsExternalServiceAccount(login string) bool {
parts := strings.SplitAfter(login, "-")
if len(parts) < 4 {
return false
}
return parts[0] == ServiceAccountPrefix && parts[2] == ExtSvcPrefix
}

42
pkg/services/serviceaccounts/models_test.go Normal file
View File

@ -0,0 +1,42 @@
package serviceaccounts
import (
"testing"
"github.com/stretchr/testify/require"
)
func TestIsExternalServiceAccount(t *testing.T) {
tests := []struct {
name string
login string
want bool
}{
{
name: "external service account",
login: "sa-1-extsvc-test",
want: true,
},
{
name: "not external service account (too short)",
login: "sa-1-test",
want: false,
},
{
name: "not external service account (wrong sa prefix)",
login: "saN-1-extsvc-test",
want: false,
},
{
name: "not external service account (wrong extsvc prefix)",
login: "sa-1-extsvcN-test",
want: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := IsExternalServiceAccount(tt.login)
require.Equal(t, tt.want, got)
})
}
}

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

@ -56,7 +56,7 @@ func (s *ServiceAccountsProxy) AddServiceAccountToken(ctx context.Context, servi
return nil, err
}
if isExternalServiceAccount(sa.Login) {
if serviceaccounts.IsExternalServiceAccount(sa.Login) {
s.log.Error("unable to create tokens for external service accounts", "serviceAccountID", serviceAccountID)
return nil, extsvcaccounts.ErrCannotCreateToken
}
@ -82,7 +82,7 @@ func (s *ServiceAccountsProxy) DeleteServiceAccount(ctx context.Context, orgID,
return err
}
if isExternalServiceAccount(sa.Login) {
if serviceaccounts.IsExternalServiceAccount(sa.Login) {
s.log.Error("unable to delete external service accounts", "serviceAccountID", serviceAccountID)
return extsvcaccounts.ErrCannotBeDeleted
}
@ -97,7 +97,7 @@ func (s *ServiceAccountsProxy) DeleteServiceAccountToken(ctx context.Context, or
return err
}
if isExternalServiceAccount(sa.Login) {
if serviceaccounts.IsExternalServiceAccount(sa.Login) {
s.log.Error("unable to delete tokens for external service accounts", "serviceAccountID", serviceAccountID)
return extsvcaccounts.ErrCannotDeleteToken
}
@ -111,7 +111,7 @@ func (s *ServiceAccountsProxy) EnableServiceAccount(ctx context.Context, orgID i
if err != nil {
return err
}
if isExternalServiceAccount(sa.Login) {
if serviceaccounts.IsExternalServiceAccount(sa.Login) {
s.log.Error("unable to enable/disable external service accounts", "serviceAccountID", serviceAccountID)
return extsvcaccounts.ErrCannotBeUpdated
}
@ -138,7 +138,7 @@ func (s *ServiceAccountsProxy) RetrieveServiceAccount(ctx context.Context, orgID
}
if s.isProxyEnabled {
sa.IsExternal = isExternalServiceAccount(sa.Login)
sa.IsExternal = serviceaccounts.IsExternalServiceAccount(sa.Login)
sa.RequiredBy = strings.ReplaceAll(sa.Name, serviceaccounts.ExtSvcPrefix, "")
}
@ -159,7 +159,7 @@ func (s *ServiceAccountsProxy) UpdateServiceAccount(ctx context.Context, orgID,
if err != nil {
return nil, err
}
if isExternalServiceAccount(sa.Login) {
if serviceaccounts.IsExternalServiceAccount(sa.Login) {
s.log.Error("unable to update external service accounts", "serviceAccountID", serviceAccountID)
return nil, extsvcaccounts.ErrCannotBeUpdated
}
@ -176,7 +176,7 @@ func (s *ServiceAccountsProxy) SearchOrgServiceAccounts(ctx context.Context, que
if s.isProxyEnabled {
for i := range sa.ServiceAccounts {
sa.ServiceAccounts[i].IsExternal = isExternalServiceAccount(sa.ServiceAccounts[i].Login)
sa.ServiceAccounts[i].IsExternal = serviceaccounts.IsExternalServiceAccount(sa.ServiceAccounts[i].Login)
}
}
return sa, nil
@ -185,7 +185,3 @@ func (s *ServiceAccountsProxy) SearchOrgServiceAccounts(ctx context.Context, que
func isNameValid(name string) bool {
return !strings.HasPrefix(name, serviceaccounts.ExtSvcPrefix)
}
func isExternalServiceAccount(login string) bool {
return strings.HasPrefix(login, serviceaccounts.ExtSvcLoginPrefix)
}

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

@ -13,10 +13,13 @@ import (
"github.com/grafana/grafana/pkg/services/serviceaccounts/tests"
)
var _ sa.Service = (*tests.FakeServiceAccountService)(nil)
var (
_ sa.Service = (*tests.FakeServiceAccountService)(nil)
autoAssignOrgID = int64(2)
)
func TestProvideServiceAccount_crudServiceAccount(t *testing.T) {
testOrgId := int64(1)
testServiceAccountId := int64(1)
testServiceAccountTokenId := int64(1)
serviceMock := &tests.FakeServiceAccountService{}
@ -51,7 +54,7 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
tc := tc
_, err := svc.CreateServiceAccount(context.Background(), testOrgId, &tc.form)
_, err := svc.CreateServiceAccount(context.Background(), autoAssignOrgID, &tc.form)
assert.Equal(t, err, tc.expectedError, tc.description)
})
}
@ -71,10 +74,10 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) {
},
},
{
description: "should not allow to delete a service account with " + sa.ExtSvcLoginPrefix + " prefix",
description: "should not allow to delete a service account with " + sa.ExtSvcLoginPrefix(autoAssignOrgID) + " prefix",
expectedError: extsvcaccounts.ErrCannotBeDeleted,
expectedServiceAccount: &sa.ServiceAccountProfileDTO{
Login: sa.ExtSvcLoginPrefix + "my-service-account",
Login: sa.ExtSvcLoginPrefix(autoAssignOrgID) + "my-service-account",
},
},
}
@ -82,7 +85,7 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
serviceMock.ExpectedServiceAccountProfile = tc.expectedServiceAccount
err := svc.DeleteServiceAccount(context.Background(), testOrgId, testServiceAccountId)
err := svc.DeleteServiceAccount(context.Background(), autoAssignOrgID, testServiceAccountId)
assert.Equal(t, err, tc.expectedError, tc.description)
})
}
@ -105,7 +108,7 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) {
description: "should not allow to delete a external service account token",
expectedError: extsvcaccounts.ErrCannotDeleteToken,
expectedServiceAccount: &sa.ServiceAccountProfileDTO{
Login: sa.ExtSvcLoginPrefix + "my-service-account",
Login: sa.ExtSvcLoginPrefix(autoAssignOrgID) + "my-service-account",
},
},
}
@ -113,7 +116,7 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
serviceMock.ExpectedServiceAccountProfile = tc.expectedServiceAccount
err := svc.DeleteServiceAccountToken(context.Background(), testOrgId, testServiceAccountId, testServiceAccountTokenId)
err := svc.DeleteServiceAccountToken(context.Background(), autoAssignOrgID, testServiceAccountId, testServiceAccountTokenId)
assert.Equal(t, err, tc.expectedError, tc.description)
})
}
@ -135,7 +138,7 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) {
{
description: "should mark as external",
expectedServiceAccount: &sa.ServiceAccountProfileDTO{
Login: sa.ExtSvcLoginPrefix + "my-service-account",
Login: sa.ExtSvcLoginPrefix(autoAssignOrgID) + "my-service-account",
},
expectedIsExternal: true,
},
@ -144,7 +147,7 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
serviceMock.ExpectedServiceAccountProfile = tc.expectedServiceAccount
sa, err := svc.RetrieveServiceAccount(context.Background(), testOrgId, testServiceAccountId)
sa, err := svc.RetrieveServiceAccount(context.Background(), autoAssignOrgID, testServiceAccountId)
assert.NoError(t, err, tc.description)
assert.Equal(t, tc.expectedIsExternal, sa.IsExternal, tc.description)
})
@ -156,7 +159,7 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) {
TotalCount: 2,
ServiceAccounts: []*sa.ServiceAccountDTO{
{Login: "test"},
{Login: sa.ExtSvcLoginPrefix + "test"},
{Login: sa.ExtSvcLoginPrefix(autoAssignOrgID) + "test"},
},
Page: 1,
PerPage: 2,
@ -203,7 +206,7 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) {
Name: &nameWithoutProtectedPrefix,
},
expectedServiceAccount: &sa.ServiceAccountProfileDTO{
Login: sa.ExtSvcLoginPrefix + "my-service-account",
Login: sa.ExtSvcLoginPrefix(autoAssignOrgID) + "my-service-account",
},
expectedError: extsvcaccounts.ErrCannotBeUpdated,
},
@ -213,7 +216,7 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) {
Name: &nameWithProtectedPrefix,
},
expectedServiceAccount: &sa.ServiceAccountProfileDTO{
Login: sa.ExtSvcLoginPrefix + "my-service-account",
Login: sa.ExtSvcLoginPrefix(autoAssignOrgID) + "my-service-account",
},
expectedError: extsvcaccounts.ErrInvalidName,
},
@ -223,7 +226,7 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) {
t.Run(tc.description, func(t *testing.T) {
tc := tc
serviceMock.ExpectedServiceAccountProfile = tc.expectedServiceAccount
_, err := svc.UpdateServiceAccount(context.Background(), testOrgId, testServiceAccountId, &tc.form)
_, err := svc.UpdateServiceAccount(context.Background(), autoAssignOrgID, testServiceAccountId, &tc.form)
assert.Equal(t, tc.expectedError, err, tc.description)
})
}
@ -239,7 +242,7 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) {
{
description: "should allow to create a service account token",
cmd: sa.AddServiceAccountTokenCommand{
OrgId: testOrgId,
OrgId: autoAssignOrgID,
},
expectedServiceAccount: &sa.ServiceAccountProfileDTO{
Login: "my-service-account",
@ -249,10 +252,10 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) {
{
description: "should not allow to create a service account token",
cmd: sa.AddServiceAccountTokenCommand{
OrgId: testOrgId,
OrgId: autoAssignOrgID,
},
expectedServiceAccount: &sa.ServiceAccountProfileDTO{
Login: sa.ExtSvcLoginPrefix + "my-service-account",
Login: sa.ExtSvcLoginPrefix(autoAssignOrgID) + "my-service-account",
},
expectedError: extsvcaccounts.ErrCannotCreateToken,
},
@ -269,9 +272,9 @@ 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(sa.ExtSvcPrefix+"my-service-account")) // It's not a external service account login
assert.True(t, isExternalServiceAccount(sa.ExtSvcLoginPrefix+"my-service-account"))
assert.False(t, sa.IsExternalServiceAccount("my-service-account"))
assert.False(t, sa.IsExternalServiceAccount("sa-my-service-account"))
assert.False(t, sa.IsExternalServiceAccount(sa.ExtSvcPrefix+"my-service-account")) // It's not a external service account login
assert.True(t, sa.IsExternalServiceAccount(sa.ExtSvcLoginPrefix(autoAssignOrgID)+"my-service-account"))
})
}