From 3015e5921f022b591571cce3cb639b4294c65097 Mon Sep 17 00:00:00 2001 From: Gabriel MABILLE Date: Tue, 24 Oct 2023 11:01:04 +0200 Subject: [PATCH] Chore: Move `extsvcaccounts` package to `serviceaccounts` (#76977) * Chore: Move extsvcaccounts package to serviceaccounts * Fix proxy * Fix tests * Fix linting --- pkg/server/wire.go | 4 +-- pkg/services/extsvcauth/errors.go | 3 +- pkg/services/extsvcauth/models.go | 26 ---------------- .../extsvcauth/oauthserver/oasimpl/service.go | 7 +++-- .../oauthserver/oasimpl/service_test.go | 16 +++++----- .../oauthserver/oasimpl/token_test.go | 4 +-- pkg/services/extsvcauth/registry/service.go | 2 +- .../extsvcaccounts/models.go | 5 ++-- .../extsvcaccounts/service.go | 30 +++++++++---------- .../extsvcaccounts/service_test.go | 30 +++++++++---------- pkg/services/serviceaccounts/models.go | 19 ++++++++++++ pkg/services/serviceaccounts/proxy/service.go | 6 ++-- .../serviceaccounts/proxy/service_test.go | 2 +- .../serviceaccounts/serviceaccounts.go | 8 +++++ .../tests}/extsvcaccmock.go | 22 +++++++------- 15 files changed, 93 insertions(+), 91 deletions(-) rename pkg/services/{extsvcauth => serviceaccounts}/extsvcaccounts/models.go (92%) rename pkg/services/{extsvcauth => serviceaccounts}/extsvcaccounts/service.go (91%) rename pkg/services/{extsvcauth => serviceaccounts}/extsvcaccounts/service_test.go (93%) rename pkg/services/{extsvcauth/extsvcmocks => serviceaccounts/tests}/extsvcaccmock.go (67%) diff --git a/pkg/server/wire.go b/pkg/server/wire.go index 36bc7e0e4f7..24231f93fbe 100644 --- a/pkg/server/wire.go +++ b/pkg/server/wire.go @@ -64,7 +64,6 @@ import ( "github.com/grafana/grafana/pkg/services/encryption" encryptionservice "github.com/grafana/grafana/pkg/services/encryption/service" "github.com/grafana/grafana/pkg/services/extsvcauth" - "github.com/grafana/grafana/pkg/services/extsvcauth/extsvcaccounts" "github.com/grafana/grafana/pkg/services/extsvcauth/oauthserver" "github.com/grafana/grafana/pkg/services/extsvcauth/oauthserver/oasimpl" extsvcreg "github.com/grafana/grafana/pkg/services/extsvcauth/registry" @@ -122,6 +121,7 @@ import ( secretsMigrations "github.com/grafana/grafana/pkg/services/secrets/kvstore/migrations" secretsManager "github.com/grafana/grafana/pkg/services/secrets/manager" "github.com/grafana/grafana/pkg/services/serviceaccounts" + "github.com/grafana/grafana/pkg/services/serviceaccounts/extsvcaccounts" serviceaccountsmanager "github.com/grafana/grafana/pkg/services/serviceaccounts/manager" serviceaccountsretriever "github.com/grafana/grafana/pkg/services/serviceaccounts/retriever" "github.com/grafana/grafana/pkg/services/shorturls" @@ -367,7 +367,7 @@ var wireBasicSet = wire.NewSet( authnimpl.ProvideAuthnService, supportbundlesimpl.ProvideService, extsvcaccounts.ProvideExtSvcAccountsService, - wire.Bind(new(extsvcauth.ExtSvcAccountsService), new(*extsvcaccounts.ExtSvcAccountsService)), + wire.Bind(new(serviceaccounts.ExtSvcAccountsService), new(*extsvcaccounts.ExtSvcAccountsService)), oasimpl.ProvideService, wire.Bind(new(oauthserver.OAuth2Server), new(*oasimpl.OAuth2ServiceImpl)), extsvcreg.ProvideExtSvcRegistry, diff --git a/pkg/services/extsvcauth/errors.go b/pkg/services/extsvcauth/errors.go index c605dd3f7ab..f1af61f4f0c 100644 --- a/pkg/services/extsvcauth/errors.go +++ b/pkg/services/extsvcauth/errors.go @@ -3,6 +3,5 @@ package extsvcauth import "github.com/grafana/grafana/pkg/util/errutil" var ( - ErrUnknownProvider = errutil.BadRequest("extsvcauth.unknown-provider") - ErrCredentialsNotFound = errutil.NotFound("extsvcauth.credentials-not-found") + ErrUnknownProvider = errutil.BadRequest("extsvcauth.unknown-provider") ) diff --git a/pkg/services/extsvcauth/models.go b/pkg/services/extsvcauth/models.go index e316aa6b7b8..fd69ec4efe5 100644 --- a/pkg/services/extsvcauth/models.go +++ b/pkg/services/extsvcauth/models.go @@ -3,7 +3,6 @@ package extsvcauth import ( "context" - "github.com/grafana/grafana/pkg/models/roletype" "github.com/grafana/grafana/pkg/services/accesscontrol" ) @@ -24,31 +23,6 @@ type ExternalServiceRegistry interface { SaveExternalService(ctx context.Context, cmd *ExternalServiceRegistration) (*ExternalService, error) } -//go:generate mockery --name ExtSvcAccountsService --structname MockExtSvcAccountsService --output extsvcmocks --outpkg extsvcmocks --filename extsvcaccmock.go -type ExtSvcAccountsService interface { - // ManageExtSvcAccount creates, updates or deletes the service account associated with an external service - ManageExtSvcAccount(ctx context.Context, cmd *ManageExtSvcAccountCmd) (int64, error) - // RetrieveExtSvcAccount fetches an external service account by ID - RetrieveExtSvcAccount(ctx context.Context, orgID, saID int64) (*ExtSvcAccount, error) -} - -// ExtSvcAccount represents the service account associated to an external service -type ExtSvcAccount struct { - ID int64 - Login string - Name string - OrgID int64 - IsDisabled bool - Role roletype.RoleType -} - -type ManageExtSvcAccountCmd struct { - ExtSvcSlug string - Enabled bool // disabled: the service account and its permissions will be deleted - OrgID int64 - Permissions []accesscontrol.Permission -} - type SelfCfg struct { // Enabled allows the service to request access tokens for itself Enabled bool diff --git a/pkg/services/extsvcauth/oauthserver/oasimpl/service.go b/pkg/services/extsvcauth/oauthserver/oasimpl/service.go index a0d4312057c..9aae5e8feae 100644 --- a/pkg/services/extsvcauth/oauthserver/oasimpl/service.go +++ b/pkg/services/extsvcauth/oauthserver/oasimpl/service.go @@ -33,6 +33,7 @@ import ( "github.com/grafana/grafana/pkg/services/extsvcauth/oauthserver/store" "github.com/grafana/grafana/pkg/services/extsvcauth/oauthserver/utils" "github.com/grafana/grafana/pkg/services/featuremgmt" + "github.com/grafana/grafana/pkg/services/serviceaccounts" "github.com/grafana/grafana/pkg/services/signingkeys" "github.com/grafana/grafana/pkg/services/team" "github.com/grafana/grafana/pkg/services/user" @@ -54,14 +55,14 @@ type OAuth2ServiceImpl struct { logger log.Logger accessControl ac.AccessControl acService ac.Service - saService extsvcauth.ExtSvcAccountsService + saService serviceaccounts.ExtSvcAccountsService userService user.Service teamService team.Service publicKey any } func ProvideService(router routing.RouteRegister, db db.DB, cfg *setting.Cfg, - extSvcAccSvc extsvcauth.ExtSvcAccountsService, accessControl ac.AccessControl, acSvc ac.Service, userSvc user.Service, + extSvcAccSvc serviceaccounts.ExtSvcAccountsService, accessControl ac.AccessControl, acSvc ac.Service, userSvc user.Service, teamSvc team.Service, keySvc signingkeys.Service, fmgmt *featuremgmt.FeatureManager) (*OAuth2ServiceImpl, error) { if !fmgmt.IsEnabled(featuremgmt.FlagExternalServiceAuth) { return nil, nil @@ -245,7 +246,7 @@ func (s *OAuth2ServiceImpl) SaveExternalService(ctx context.Context, registratio client.Secret = string(hashedSecret) s.logger.Debug("Save service account") - saID, errSaveServiceAccount := s.saService.ManageExtSvcAccount(ctx, &extsvcauth.ManageExtSvcAccountCmd{ + saID, errSaveServiceAccount := s.saService.ManageExtSvcAccount(ctx, &serviceaccounts.ManageExtSvcAccountCmd{ ExtSvcSlug: slugify.Slugify(client.Name), Enabled: registration.Self.Enabled, OrgID: oauthserver.TmpOrgID, diff --git a/pkg/services/extsvcauth/oauthserver/oasimpl/service_test.go b/pkg/services/extsvcauth/oauthserver/oasimpl/service_test.go index c97c5cc2c78..afcedd06cc2 100644 --- a/pkg/services/extsvcauth/oauthserver/oasimpl/service_test.go +++ b/pkg/services/extsvcauth/oauthserver/oasimpl/service_test.go @@ -21,11 +21,11 @@ import ( "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" "github.com/grafana/grafana/pkg/services/accesscontrol/actest" "github.com/grafana/grafana/pkg/services/extsvcauth" - "github.com/grafana/grafana/pkg/services/extsvcauth/extsvcmocks" "github.com/grafana/grafana/pkg/services/extsvcauth/oauthserver" "github.com/grafana/grafana/pkg/services/extsvcauth/oauthserver/oastest" "github.com/grafana/grafana/pkg/services/featuremgmt" sa "github.com/grafana/grafana/pkg/services/serviceaccounts" + saTests "github.com/grafana/grafana/pkg/services/serviceaccounts/tests" "github.com/grafana/grafana/pkg/services/signingkeys/signingkeystest" "github.com/grafana/grafana/pkg/services/team/teamtest" "github.com/grafana/grafana/pkg/services/user" @@ -50,7 +50,7 @@ type TestEnv struct { OAuthStore *oastest.MockStore UserService *usertest.FakeUserService TeamService *teamtest.FakeService - SAService *extsvcmocks.MockExtSvcAccountsService + SAService *saTests.MockExtSvcAccountsService } func setupTestEnv(t *testing.T) *TestEnv { @@ -75,7 +75,7 @@ func setupTestEnv(t *testing.T) *TestEnv { OAuthStore: &oastest.MockStore{}, UserService: usertest.NewUserServiceFake(), TeamService: teamtest.NewFakeService(), - SAService: extsvcmocks.NewMockExtSvcAccountsService(t), + SAService: saTests.NewMockExtSvcAccountsService(t), } env.S = &OAuth2ServiceImpl{ cache: localcache.New(cacheExpirationTime, cacheCleanupInterval), @@ -166,7 +166,7 @@ func TestOAuth2ServiceImpl_SaveExternalService(t *testing.T) { })) // Check that despite no credential_grants the service account still has a permission to impersonate users env.SAService.AssertCalled(t, "ManageExtSvcAccount", mock.Anything, - mock.MatchedBy(func(cmd *extsvcauth.ManageExtSvcAccountCmd) bool { + mock.MatchedBy(func(cmd *sa.ManageExtSvcAccountCmd) bool { return len(cmd.Permissions) == 1 && cmd.Permissions[0] == ac.Permission{Action: ac.ActionUsersRead, Scope: ac.ScopeUsersAll} })) }, @@ -200,7 +200,7 @@ func TestOAuth2ServiceImpl_SaveExternalService(t *testing.T) { })) // Check that despite no credential_grants the service account still has a permission to impersonate users env.SAService.AssertCalled(t, "ManageExtSvcAccount", mock.Anything, - mock.MatchedBy(func(cmd *extsvcauth.ManageExtSvcAccountCmd) bool { + mock.MatchedBy(func(cmd *sa.ManageExtSvcAccountCmd) bool { return len(cmd.Permissions) == 1 && cmd.Permissions[0] == ac.Permission{Action: ac.ActionUsersImpersonate, Scope: ac.ScopeUsersAll} })) }, @@ -318,7 +318,7 @@ func TestOAuth2ServiceImpl_GetExternalService(t *testing.T) { name: "should return error when the service account was not found", init: func(env *TestEnv) { env.OAuthStore.On("GetExternalService", mock.Anything, mock.Anything).Return(dummyClient(), nil) - env.SAService.On("RetrieveExtSvcAccount", mock.Anything, int64(1), int64(1)).Return(&extsvcauth.ExtSvcAccount{}, sa.ErrServiceAccountNotFound) + env.SAService.On("RetrieveExtSvcAccount", mock.Anything, int64(1), int64(1)).Return(&sa.ExtSvcAccount{}, sa.ErrServiceAccountNotFound) }, mockChecks: func(t *testing.T, env *TestEnv) { env.OAuthStore.AssertCalled(t, "GetExternalService", mock.Anything, mock.Anything) @@ -330,7 +330,7 @@ func TestOAuth2ServiceImpl_GetExternalService(t *testing.T) { name: "should return error when the service account has no permissions", init: func(env *TestEnv) { env.OAuthStore.On("GetExternalService", mock.Anything, mock.Anything).Return(dummyClient(), nil) - env.SAService.On("RetrieveExtSvcAccount", mock.Anything, int64(1), int64(1)).Return(&extsvcauth.ExtSvcAccount{}, nil) + env.SAService.On("RetrieveExtSvcAccount", mock.Anything, int64(1), int64(1)).Return(&sa.ExtSvcAccount{}, nil) env.AcStore.On("GetUserPermissions", mock.Anything, mock.Anything).Return(nil, fmt.Errorf("some error")) }, mockChecks: func(t *testing.T, env *TestEnv) { @@ -343,7 +343,7 @@ func TestOAuth2ServiceImpl_GetExternalService(t *testing.T) { name: "should return correctly", init: func(env *TestEnv) { env.OAuthStore.On("GetExternalService", mock.Anything, mock.Anything).Return(dummyClient(), nil) - env.SAService.On("RetrieveExtSvcAccount", mock.Anything, int64(1), int64(1)).Return(&extsvcauth.ExtSvcAccount{ID: 1}, nil) + env.SAService.On("RetrieveExtSvcAccount", mock.Anything, int64(1), int64(1)).Return(&sa.ExtSvcAccount{ID: 1}, nil) env.AcStore.On("GetUserPermissions", mock.Anything, mock.Anything).Return([]ac.Permission{{Action: ac.ActionUsersImpersonate, Scope: ac.ScopeUsersAll}}, nil) }, mockChecks: func(t *testing.T, env *TestEnv) { diff --git a/pkg/services/extsvcauth/oauthserver/oasimpl/token_test.go b/pkg/services/extsvcauth/oauthserver/oasimpl/token_test.go index 5d0b68d4944..ac49dfddb5d 100644 --- a/pkg/services/extsvcauth/oauthserver/oasimpl/token_test.go +++ b/pkg/services/extsvcauth/oauthserver/oasimpl/token_test.go @@ -22,8 +22,8 @@ import ( "github.com/grafana/grafana/pkg/models/roletype" ac "github.com/grafana/grafana/pkg/services/accesscontrol" - "github.com/grafana/grafana/pkg/services/extsvcauth" "github.com/grafana/grafana/pkg/services/extsvcauth/oauthserver" + sa "github.com/grafana/grafana/pkg/services/serviceaccounts" "github.com/grafana/grafana/pkg/services/team" "github.com/grafana/grafana/pkg/services/user" ) @@ -703,7 +703,7 @@ func setupHandleTokenRequestEnv(t *testing.T, env *TestEnv, opt func(*oauthserve opt(client1) } - sa1 := &extsvcauth.ExtSvcAccount{ + sa1 := &sa.ExtSvcAccount{ ID: client1.ServiceAccountID, Name: client1.Name, Login: client1.Name, diff --git a/pkg/services/extsvcauth/registry/service.go b/pkg/services/extsvcauth/registry/service.go index 0cb3b9db911..efeb7c1771d 100644 --- a/pkg/services/extsvcauth/registry/service.go +++ b/pkg/services/extsvcauth/registry/service.go @@ -5,9 +5,9 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/extsvcauth" - "github.com/grafana/grafana/pkg/services/extsvcauth/extsvcaccounts" "github.com/grafana/grafana/pkg/services/extsvcauth/oauthserver" "github.com/grafana/grafana/pkg/services/featuremgmt" + "github.com/grafana/grafana/pkg/services/serviceaccounts/extsvcaccounts" ) var _ extsvcauth.ExternalServiceRegistry = &Registry{} diff --git a/pkg/services/extsvcauth/extsvcaccounts/models.go b/pkg/services/serviceaccounts/extsvcaccounts/models.go similarity index 92% rename from pkg/services/extsvcauth/extsvcaccounts/models.go rename to pkg/services/serviceaccounts/extsvcaccounts/models.go index 9211ae6cc2a..8ea0cb77d36 100644 --- a/pkg/services/extsvcauth/extsvcaccounts/models.go +++ b/pkg/services/serviceaccounts/extsvcaccounts/models.go @@ -7,8 +7,7 @@ import ( ) const ( - ExtSvcPrefix = "extsvc-" - kvStoreType = "extsvc-token" + kvStoreType = "extsvc-token" // #nosec G101 - this is not a hardcoded secret tokenNamePrefix = "extsvc-token" ) @@ -18,6 +17,8 @@ var ( ErrInvalidName = errutil.BadRequest("extsvcaccounts.ErrInvalidName", errutil.WithPublicMessage("only external service account names can be prefixed with 'extsvc-'")) ErrCannotBeUpdated = errutil.BadRequest("extsvcaccounts.ErrCannotBeUpdated", errutil.WithPublicMessage("external service account cannot be updated")) ErrCannotCreateToken = errutil.BadRequest("extsvcaccounts.ErrCannotCreateToken", errutil.WithPublicMessage("cannot add external service account token")) + + ErrCredentialsNotFound = errutil.NotFound("extsvcaccounts.credentials-not-found") ) // Credentials represents the credentials associated to an external service diff --git a/pkg/services/extsvcauth/extsvcaccounts/service.go b/pkg/services/serviceaccounts/extsvcaccounts/service.go similarity index 91% rename from pkg/services/extsvcauth/extsvcaccounts/service.go rename to pkg/services/serviceaccounts/extsvcaccounts/service.go index e8bb45cb535..6eed6b6e978 100644 --- a/pkg/services/extsvcauth/extsvcaccounts/service.go +++ b/pkg/services/serviceaccounts/extsvcaccounts/service.go @@ -35,18 +35,18 @@ func ProvideExtSvcAccountsService(acSvc ac.Service, saSvc *manager.ServiceAccoun } // RetrieveExtSvcAccount fetches an external service account by ID -func (esa *ExtSvcAccountsService) RetrieveExtSvcAccount(ctx context.Context, orgID, saID int64) (*extsvcauth.ExtSvcAccount, error) { - sa, err := esa.saSvc.RetrieveServiceAccount(ctx, orgID, saID) +func (esa *ExtSvcAccountsService) RetrieveExtSvcAccount(ctx context.Context, orgID, saID int64) (*sa.ExtSvcAccount, error) { + svcAcc, err := esa.saSvc.RetrieveServiceAccount(ctx, orgID, saID) if err != nil { return nil, err } - return &extsvcauth.ExtSvcAccount{ - ID: sa.Id, - Login: sa.Login, - Name: sa.Name, - OrgID: sa.OrgId, - IsDisabled: sa.IsDisabled, - Role: roletype.RoleType(sa.Role), + return &sa.ExtSvcAccount{ + ID: svcAcc.Id, + Login: svcAcc.Login, + Name: svcAcc.Name, + OrgID: svcAcc.OrgId, + IsDisabled: svcAcc.IsDisabled, + Role: roletype.RoleType(svcAcc.Role), }, nil } @@ -63,7 +63,7 @@ func (esa *ExtSvcAccountsService) SaveExternalService(ctx context.Context, cmd * esa.logger.Warn("Impersonation setup skipped. It is not possible to impersonate with a service account token.", "service", slug) } - saID, err := esa.ManageExtSvcAccount(ctx, &extsvcauth.ManageExtSvcAccountCmd{ + saID, err := esa.ManageExtSvcAccount(ctx, &sa.ManageExtSvcAccountCmd{ ExtSvcSlug: slug, Enabled: cmd.Self.Enabled, OrgID: extsvcauth.TmpOrgID, @@ -91,13 +91,13 @@ func (esa *ExtSvcAccountsService) SaveExternalService(ctx context.Context, cmd * } // ManageExtSvcAccount creates, updates or deletes the service account associated with an external service -func (esa *ExtSvcAccountsService) ManageExtSvcAccount(ctx context.Context, cmd *extsvcauth.ManageExtSvcAccountCmd) (int64, error) { +func (esa *ExtSvcAccountsService) ManageExtSvcAccount(ctx context.Context, cmd *sa.ManageExtSvcAccountCmd) (int64, error) { if cmd == nil { esa.logger.Warn("Received no input") return 0, nil } - saID, errRetrieve := esa.saSvc.RetrieveServiceAccountIdByName(ctx, cmd.OrgID, ExtSvcPrefix+cmd.ExtSvcSlug) + saID, errRetrieve := esa.saSvc.RetrieveServiceAccountIdByName(ctx, cmd.OrgID, sa.ExtSvcPrefix+cmd.ExtSvcSlug) if errRetrieve != nil && !errors.Is(errRetrieve, sa.ErrServiceAccountNotFound) { return 0, errRetrieve } @@ -140,7 +140,7 @@ func (esa *ExtSvcAccountsService) saveExtSvcAccount(ctx context.Context, cmd *sa // Create a service account esa.logger.Debug("Create service account", "service", cmd.ExtSvcSlug, "orgID", cmd.OrgID) sa, err := esa.saSvc.CreateServiceAccount(ctx, cmd.OrgID, &sa.CreateServiceAccountForm{ - Name: ExtSvcPrefix + cmd.ExtSvcSlug, + Name: sa.ExtSvcPrefix + cmd.ExtSvcSlug, Role: newRole(roletype.RoleNone), IsDisabled: newBool(false), }) @@ -181,7 +181,7 @@ func (esa *ExtSvcAccountsService) deleteExtSvcAccount(ctx context.Context, orgID func (esa *ExtSvcAccountsService) getExtSvcAccountToken(ctx context.Context, orgID, saID int64, extSvcSlug string) (string, error) { // Get credentials from store credentials, err := esa.GetExtSvcCredentials(ctx, orgID, extSvcSlug) - if err != nil && !errors.Is(err, extsvcauth.ErrCredentialsNotFound) { + if err != nil && !errors.Is(err, ErrCredentialsNotFound) { return "", err } if credentials != nil { @@ -223,7 +223,7 @@ func (esa *ExtSvcAccountsService) GetExtSvcCredentials(ctx context.Context, orgI return nil, err } if !ok { - return nil, extsvcauth.ErrCredentialsNotFound.Errorf("No credential found for in store %v", extSvcSlug) + return nil, ErrCredentialsNotFound.Errorf("No credential found for in store %v", extSvcSlug) } return &Credentials{Secret: token}, nil } diff --git a/pkg/services/extsvcauth/extsvcaccounts/service_test.go b/pkg/services/serviceaccounts/extsvcaccounts/service_test.go similarity index 93% rename from pkg/services/extsvcauth/extsvcaccounts/service_test.go rename to pkg/services/serviceaccounts/extsvcaccounts/service_test.go index 3a5abbfe879..dc2034786bc 100644 --- a/pkg/services/extsvcauth/extsvcaccounts/service_test.go +++ b/pkg/services/serviceaccounts/extsvcaccounts/service_test.go @@ -65,7 +65,7 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) { tests := []struct { name string init func(env *TestEnv) - cmd extsvcauth.ManageExtSvcAccountCmd + cmd sa.ManageExtSvcAccountCmd checks func(t *testing.T, env *TestEnv) want int64 wantErr bool @@ -78,7 +78,7 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) { env.SaSvc.On("DeleteServiceAccount", mock.Anything, mock.Anything, mock.Anything).Return(nil) env.AcStore.On("DeleteExternalServiceRole", mock.Anything, mock.Anything).Return(nil) }, - cmd: extsvcauth.ManageExtSvcAccountCmd{ + cmd: sa.ManageExtSvcAccountCmd{ ExtSvcSlug: extSvcSlug, Enabled: false, OrgID: extSvcOrgID, @@ -87,7 +87,7 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) { checks: func(t *testing.T, env *TestEnv) { env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything, mock.MatchedBy(func(orgID int64) bool { return orgID == extSvcOrgID }), - mock.MatchedBy(func(slug string) bool { return slug == ExtSvcPrefix+extSvcSlug })) + mock.MatchedBy(func(slug string) bool { return slug == sa.ExtSvcPrefix+extSvcSlug })) env.SaSvc.AssertCalled(t, "DeleteServiceAccount", mock.Anything, mock.MatchedBy(func(orgID int64) bool { return orgID == extSvcOrgID }), mock.MatchedBy(func(saID int64) bool { return saID == extSvcAccID })) @@ -105,7 +105,7 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) { env.SaSvc.On("DeleteServiceAccount", mock.Anything, mock.Anything, mock.Anything).Return(nil) env.AcStore.On("DeleteExternalServiceRole", mock.Anything, mock.Anything).Return(nil) }, - cmd: extsvcauth.ManageExtSvcAccountCmd{ + cmd: sa.ManageExtSvcAccountCmd{ ExtSvcSlug: extSvcSlug, Enabled: true, OrgID: extSvcOrgID, @@ -114,7 +114,7 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) { checks: func(t *testing.T, env *TestEnv) { env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything, mock.MatchedBy(func(orgID int64) bool { return orgID == extSvcOrgID }), - mock.MatchedBy(func(slug string) bool { return slug == ExtSvcPrefix+extSvcSlug })) + mock.MatchedBy(func(slug string) bool { return slug == sa.ExtSvcPrefix+extSvcSlug })) env.SaSvc.AssertCalled(t, "DeleteServiceAccount", mock.Anything, mock.MatchedBy(func(orgID int64) bool { return orgID == extSvcOrgID }), mock.MatchedBy(func(saID int64) bool { return saID == extSvcAccID })) @@ -134,7 +134,7 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) { Return(extSvcAccount, nil) env.AcStore.On("SaveExternalServiceRole", mock.Anything, mock.Anything).Return(nil) }, - cmd: extsvcauth.ManageExtSvcAccountCmd{ + cmd: sa.ManageExtSvcAccountCmd{ ExtSvcSlug: extSvcSlug, Enabled: true, OrgID: extSvcOrgID, @@ -143,11 +143,11 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) { checks: func(t *testing.T, env *TestEnv) { env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything, mock.MatchedBy(func(orgID int64) bool { return orgID == extSvcOrgID }), - mock.MatchedBy(func(slug string) bool { return slug == ExtSvcPrefix+extSvcSlug })) + mock.MatchedBy(func(slug string) bool { return slug == sa.ExtSvcPrefix+extSvcSlug })) env.SaSvc.AssertCalled(t, "CreateServiceAccount", mock.Anything, mock.MatchedBy(func(orgID int64) bool { return orgID == extSvcOrgID }), mock.MatchedBy(func(cmd *sa.CreateServiceAccountForm) bool { - return cmd.Name == ExtSvcPrefix+extSvcSlug && *cmd.Role == roletype.RoleNone + return cmd.Name == sa.ExtSvcPrefix+extSvcSlug && *cmd.Role == roletype.RoleNone }), ) env.AcStore.AssertCalled(t, "SaveExternalServiceRole", mock.Anything, @@ -168,7 +168,7 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) { Return(int64(11), nil) env.AcStore.On("SaveExternalServiceRole", mock.Anything, mock.Anything).Return(nil) }, - cmd: extsvcauth.ManageExtSvcAccountCmd{ + cmd: sa.ManageExtSvcAccountCmd{ ExtSvcSlug: extSvcSlug, Enabled: true, OrgID: extSvcOrgID, @@ -177,7 +177,7 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) { checks: func(t *testing.T, env *TestEnv) { env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything, mock.MatchedBy(func(orgID int64) bool { return orgID == extSvcOrgID }), - mock.MatchedBy(func(slug string) bool { return slug == ExtSvcPrefix+extSvcSlug })) + mock.MatchedBy(func(slug string) bool { return slug == sa.ExtSvcPrefix+extSvcSlug })) env.AcStore.AssertCalled(t, "SaveExternalServiceRole", mock.Anything, mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool { return cmd.ServiceAccountID == int64(11) && cmd.ExternalServiceID == extSvcSlug && @@ -257,7 +257,7 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) { checks: func(t *testing.T, env *TestEnv) { env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything, mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }), - mock.MatchedBy(func(slug string) bool { return slug == ExtSvcPrefix+extSvcSlug })) + mock.MatchedBy(func(slug string) bool { return slug == sa.ExtSvcPrefix+extSvcSlug })) env.SaSvc.AssertCalled(t, "DeleteServiceAccount", mock.Anything, mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }), mock.MatchedBy(func(saID int64) bool { return saID == extSvcAccID })) @@ -287,7 +287,7 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) { checks: func(t *testing.T, env *TestEnv) { env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything, mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }), - mock.MatchedBy(func(slug string) bool { return slug == ExtSvcPrefix+extSvcSlug })) + mock.MatchedBy(func(slug string) bool { return slug == sa.ExtSvcPrefix+extSvcSlug })) env.SaSvc.AssertCalled(t, "DeleteServiceAccount", mock.Anything, mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }), mock.MatchedBy(func(saID int64) bool { return saID == extSvcAccID })) @@ -319,11 +319,11 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) { checks: func(t *testing.T, env *TestEnv) { env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything, mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }), - mock.MatchedBy(func(slug string) bool { return slug == ExtSvcPrefix+extSvcSlug })) + mock.MatchedBy(func(slug string) bool { return slug == sa.ExtSvcPrefix+extSvcSlug })) env.SaSvc.AssertCalled(t, "CreateServiceAccount", mock.Anything, mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }), mock.MatchedBy(func(cmd *sa.CreateServiceAccountForm) bool { - return cmd.Name == ExtSvcPrefix+extSvcSlug && *cmd.Role == roletype.RoleNone + return cmd.Name == sa.ExtSvcPrefix+extSvcSlug && *cmd.Role == roletype.RoleNone }), ) env.AcStore.AssertCalled(t, "SaveExternalServiceRole", mock.Anything, @@ -360,7 +360,7 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) { checks: func(t *testing.T, env *TestEnv) { env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything, mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }), - mock.MatchedBy(func(slug string) bool { return slug == ExtSvcPrefix+extSvcSlug })) + mock.MatchedBy(func(slug string) bool { return slug == sa.ExtSvcPrefix+extSvcSlug })) env.AcStore.AssertCalled(t, "SaveExternalServiceRole", mock.Anything, mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool { return cmd.ServiceAccountID == int64(11) && cmd.ExternalServiceID == extSvcSlug && diff --git a/pkg/services/serviceaccounts/models.go b/pkg/services/serviceaccounts/models.go index a115628a148..6346cd6cd7c 100644 --- a/pkg/services/serviceaccounts/models.go +++ b/pkg/services/serviceaccounts/models.go @@ -3,6 +3,7 @@ package serviceaccounts import ( "time" + "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/org" @@ -16,6 +17,7 @@ var ( const ( ServiceAccountPrefix = "sa-" + ExtSvcPrefix = "extsvc-" ) const ( @@ -173,6 +175,23 @@ type Stats struct { ForcedExpiryEnabled bool `xorm:"-"` } +// ExtSvcAccount represents the service account associated to an external service +type ExtSvcAccount struct { + ID int64 + Login string + Name string + OrgID int64 + IsDisabled bool + Role roletype.RoleType +} + +type ManageExtSvcAccountCmd struct { + ExtSvcSlug string + Enabled bool // disabled: the service account and its permissions will be deleted + OrgID int64 + Permissions []accesscontrol.Permission +} + // AccessEvaluator is used to protect the "Configuration > Service accounts" page access var AccessEvaluator = accesscontrol.EvalAny( accesscontrol.EvalPermission(ActionRead), diff --git a/pkg/services/serviceaccounts/proxy/service.go b/pkg/services/serviceaccounts/proxy/service.go index 46785252a98..95e79cc1a28 100644 --- a/pkg/services/serviceaccounts/proxy/service.go +++ b/pkg/services/serviceaccounts/proxy/service.go @@ -6,8 +6,8 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/apikey" - "github.com/grafana/grafana/pkg/services/extsvcauth/extsvcaccounts" "github.com/grafana/grafana/pkg/services/serviceaccounts" + "github.com/grafana/grafana/pkg/services/serviceaccounts/extsvcaccounts" "github.com/grafana/grafana/pkg/services/serviceaccounts/manager" ) @@ -101,9 +101,9 @@ func (s *ServiceAccountsProxy) UpdateServiceAccount(ctx context.Context, orgID, } func isNameValid(name string) bool { - return !strings.HasPrefix(name, extsvcaccounts.ExtSvcPrefix) + return !strings.HasPrefix(name, serviceaccounts.ExtSvcPrefix) } func isExternalServiceAccount(login string) bool { - return strings.HasPrefix(login, serviceaccounts.ServiceAccountPrefix+extsvcaccounts.ExtSvcPrefix) + return strings.HasPrefix(login, serviceaccounts.ServiceAccountPrefix+serviceaccounts.ExtSvcPrefix) } diff --git a/pkg/services/serviceaccounts/proxy/service_test.go b/pkg/services/serviceaccounts/proxy/service_test.go index 78893503a5a..a1258b80369 100644 --- a/pkg/services/serviceaccounts/proxy/service_test.go +++ b/pkg/services/serviceaccounts/proxy/service_test.go @@ -6,8 +6,8 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/apikey" - "github.com/grafana/grafana/pkg/services/extsvcauth/extsvcaccounts" "github.com/grafana/grafana/pkg/services/serviceaccounts" + "github.com/grafana/grafana/pkg/services/serviceaccounts/extsvcaccounts" "github.com/stretchr/testify/assert" ) diff --git a/pkg/services/serviceaccounts/serviceaccounts.go b/pkg/services/serviceaccounts/serviceaccounts.go index 5ef93b0fc1e..f7ab7e25b3f 100644 --- a/pkg/services/serviceaccounts/serviceaccounts.go +++ b/pkg/services/serviceaccounts/serviceaccounts.go @@ -22,3 +22,11 @@ type Service interface { AddServiceAccountToken(ctx context.Context, serviceAccountID int64, cmd *AddServiceAccountTokenCommand) (*apikey.APIKey, error) } + +//go:generate mockery --name ExtSvcAccountsService --structname MockExtSvcAccountsService --output tests --outpkg tests --filename extsvcaccmock.go +type ExtSvcAccountsService interface { + // ManageExtSvcAccount creates, updates or deletes the service account associated with an external service + ManageExtSvcAccount(ctx context.Context, cmd *ManageExtSvcAccountCmd) (int64, error) + // RetrieveExtSvcAccount fetches an external service account by ID + RetrieveExtSvcAccount(ctx context.Context, orgID, saID int64) (*ExtSvcAccount, error) +} diff --git a/pkg/services/extsvcauth/extsvcmocks/extsvcaccmock.go b/pkg/services/serviceaccounts/tests/extsvcaccmock.go similarity index 67% rename from pkg/services/extsvcauth/extsvcmocks/extsvcaccmock.go rename to pkg/services/serviceaccounts/tests/extsvcaccmock.go index b16fff68577..62dd0ef4abf 100644 --- a/pkg/services/extsvcauth/extsvcmocks/extsvcaccmock.go +++ b/pkg/services/serviceaccounts/tests/extsvcaccmock.go @@ -1,11 +1,11 @@ // Code generated by mockery v2.35.2. DO NOT EDIT. -package extsvcmocks +package tests import ( context "context" - extsvcauth "github.com/grafana/grafana/pkg/services/extsvcauth" + serviceaccounts "github.com/grafana/grafana/pkg/services/serviceaccounts" mock "github.com/stretchr/testify/mock" ) @@ -15,21 +15,21 @@ type MockExtSvcAccountsService struct { } // ManageExtSvcAccount provides a mock function with given fields: ctx, cmd -func (_m *MockExtSvcAccountsService) ManageExtSvcAccount(ctx context.Context, cmd *extsvcauth.ManageExtSvcAccountCmd) (int64, error) { +func (_m *MockExtSvcAccountsService) ManageExtSvcAccount(ctx context.Context, cmd *serviceaccounts.ManageExtSvcAccountCmd) (int64, error) { ret := _m.Called(ctx, cmd) var r0 int64 var r1 error - if rf, ok := ret.Get(0).(func(context.Context, *extsvcauth.ManageExtSvcAccountCmd) (int64, error)); ok { + if rf, ok := ret.Get(0).(func(context.Context, *serviceaccounts.ManageExtSvcAccountCmd) (int64, error)); ok { return rf(ctx, cmd) } - if rf, ok := ret.Get(0).(func(context.Context, *extsvcauth.ManageExtSvcAccountCmd) int64); ok { + if rf, ok := ret.Get(0).(func(context.Context, *serviceaccounts.ManageExtSvcAccountCmd) int64); ok { r0 = rf(ctx, cmd) } else { r0 = ret.Get(0).(int64) } - if rf, ok := ret.Get(1).(func(context.Context, *extsvcauth.ManageExtSvcAccountCmd) error); ok { + if rf, ok := ret.Get(1).(func(context.Context, *serviceaccounts.ManageExtSvcAccountCmd) error); ok { r1 = rf(ctx, cmd) } else { r1 = ret.Error(1) @@ -39,19 +39,19 @@ func (_m *MockExtSvcAccountsService) ManageExtSvcAccount(ctx context.Context, cm } // RetrieveExtSvcAccount provides a mock function with given fields: ctx, orgID, saID -func (_m *MockExtSvcAccountsService) RetrieveExtSvcAccount(ctx context.Context, orgID int64, saID int64) (*extsvcauth.ExtSvcAccount, error) { +func (_m *MockExtSvcAccountsService) RetrieveExtSvcAccount(ctx context.Context, orgID int64, saID int64) (*serviceaccounts.ExtSvcAccount, error) { ret := _m.Called(ctx, orgID, saID) - var r0 *extsvcauth.ExtSvcAccount + var r0 *serviceaccounts.ExtSvcAccount var r1 error - if rf, ok := ret.Get(0).(func(context.Context, int64, int64) (*extsvcauth.ExtSvcAccount, error)); ok { + if rf, ok := ret.Get(0).(func(context.Context, int64, int64) (*serviceaccounts.ExtSvcAccount, error)); ok { return rf(ctx, orgID, saID) } - if rf, ok := ret.Get(0).(func(context.Context, int64, int64) *extsvcauth.ExtSvcAccount); ok { + if rf, ok := ret.Get(0).(func(context.Context, int64, int64) *serviceaccounts.ExtSvcAccount); ok { r0 = rf(ctx, orgID, saID) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(*extsvcauth.ExtSvcAccount) + r0 = ret.Get(0).(*serviceaccounts.ExtSvcAccount) } }