From 8d8451710365f22ee2671d692aaf3f3302a562df Mon Sep 17 00:00:00 2001 From: Gabriel MABILLE Date: Fri, 20 Sep 2024 14:43:29 +0200 Subject: [PATCH] AuthN: Introduce `DefaultOrgID` function for managed service accounts (#93432) * 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 --- .../database/externalservices.go | 14 ++- .../database/externalservices_test.go | 4 +- pkg/services/extsvcauth/models.go | 13 ++- .../serviceaccounts/database/store.go | 2 +- .../serviceaccounts/extsvcaccounts/metrics.go | 7 +- .../serviceaccounts/extsvcaccounts/models.go | 11 ++- .../serviceaccounts/extsvcaccounts/service.go | 45 +++++---- .../extsvcaccounts/service_test.go | 96 ++++++++++--------- pkg/services/serviceaccounts/models.go | 17 +++- pkg/services/serviceaccounts/models_test.go | 42 ++++++++ pkg/services/serviceaccounts/proxy/service.go | 18 ++-- .../serviceaccounts/proxy/service_test.go | 45 +++++---- 12 files changed, 192 insertions(+), 122 deletions(-) create mode 100644 pkg/services/serviceaccounts/models_test.go diff --git a/pkg/services/accesscontrol/database/externalservices.go b/pkg/services/accesscontrol/database/externalservices.go index 0ac3ec9f4f29..3c658a85e8fc 100644 --- a/pkg/services/accesscontrol/database/externalservices.go +++ b/pkg/services/accesscontrol/database/externalservices.go @@ -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 diff --git a/pkg/services/accesscontrol/database/externalservices_test.go b/pkg/services/accesscontrol/database/externalservices_test.go index 0df0860d21b3..7bde04735a54 100644 --- a/pkg/services/accesscontrol/database/externalservices_test.go +++ b/pkg/services/accesscontrol/database/externalservices_test.go @@ -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, }, }, }, diff --git a/pkg/services/extsvcauth/models.go b/pkg/services/extsvcauth/models.go index 4891ef930a9a..c86a6fa987e9 100644 --- a/pkg/services/extsvcauth/models.go +++ b/pkg/services/extsvcauth/models.go @@ -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 diff --git a/pkg/services/serviceaccounts/database/store.go b/pkg/services/serviceaccounts/database/store.go index 381470048e5b..003a41a24aa4 100644 --- a/pkg/services/serviceaccounts/database/store.go +++ b/pkg/services/serviceaccounts/database/store.go @@ -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) } diff --git a/pkg/services/serviceaccounts/extsvcaccounts/metrics.go b/pkg/services/serviceaccounts/extsvcaccounts/metrics.go index 0e2942a50fda..249d68d564a3 100644 --- a/pkg/services/serviceaccounts/extsvcaccounts/metrics.go +++ b/pkg/services/serviceaccounts/extsvcaccounts/metrics.go @@ -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) diff --git a/pkg/services/serviceaccounts/extsvcaccounts/models.go b/pkg/services/serviceaccounts/extsvcaccounts/models.go index be3845f1d2e1..0c721ebb5d79 100644 --- a/pkg/services/serviceaccounts/extsvcaccounts/models.go +++ b/pkg/services/serviceaccounts/extsvcaccounts/models.go @@ -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 { diff --git a/pkg/services/serviceaccounts/extsvcaccounts/service.go b/pkg/services/serviceaccounts/extsvcaccounts/service.go index 341328945d81..782466933911 100644 --- a/pkg/services/serviceaccounts/extsvcaccounts/service.go +++ b/pkg/services/serviceaccounts/extsvcaccounts/service.go @@ -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 { diff --git a/pkg/services/serviceaccounts/extsvcaccounts/service_test.go b/pkg/services/serviceaccounts/extsvcaccounts/service_test.go index e8f9650ca7c3..dbb1fa67c4e6 100644 --- a/pkg/services/serviceaccounts/extsvcaccounts/service_test.go +++ b/pkg/services/serviceaccounts/extsvcaccounts/service_test.go @@ -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) diff --git a/pkg/services/serviceaccounts/models.go b/pkg/services/serviceaccounts/models.go index 7756e0657f5a..775ba2457f08 100644 --- a/pkg/services/serviceaccounts/models.go +++ b/pkg/services/serviceaccounts/models.go @@ -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 +} diff --git a/pkg/services/serviceaccounts/models_test.go b/pkg/services/serviceaccounts/models_test.go new file mode 100644 index 000000000000..2e4f6fc29e0c --- /dev/null +++ b/pkg/services/serviceaccounts/models_test.go @@ -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) + }) + } +} diff --git a/pkg/services/serviceaccounts/proxy/service.go b/pkg/services/serviceaccounts/proxy/service.go index 40ab982d68b3..3cb5f4047210 100644 --- a/pkg/services/serviceaccounts/proxy/service.go +++ b/pkg/services/serviceaccounts/proxy/service.go @@ -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) -} diff --git a/pkg/services/serviceaccounts/proxy/service_test.go b/pkg/services/serviceaccounts/proxy/service_test.go index 45e1875adef5..5e3569c586a5 100644 --- a/pkg/services/serviceaccounts/proxy/service_test.go +++ b/pkg/services/serviceaccounts/proxy/service_test.go @@ -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")) }) }