From 72d32eed27c058467aba8e02077b5b2e97c61c8d Mon Sep 17 00:00:00 2001 From: Gabriel MABILLE Date: Wed, 29 Nov 2023 12:12:30 +0100 Subject: [PATCH] ExtSvcAuth: Assign roles locally (#78669) * ExtSvcAuth: Assign roles locally * Fix test * HandlePluginStateChanged in the OrgID * Remove Global from command * Use AssignmentOrgID instead of OrgID * Remove unecessary test case --- .../accesscontrol/acimpl/service_test.go | 14 ++-- .../database/externalservices.go | 10 +-- .../database/externalservices_test.go | 79 +++---------------- pkg/services/accesscontrol/models.go | 7 +- pkg/services/accesscontrol/models_test.go | 23 ++---- .../extsvcauth/oauthserver/oasimpl/service.go | 7 +- .../serviceaccounts/extsvcaccounts/service.go | 9 +-- .../extsvcaccounts/service_test.go | 14 ++-- 8 files changed, 44 insertions(+), 119 deletions(-) diff --git a/pkg/services/accesscontrol/acimpl/service_test.go b/pkg/services/accesscontrol/acimpl/service_test.go index 14c61d6015d..eed3f3bcbc8 100644 --- a/pkg/services/accesscontrol/acimpl/service_test.go +++ b/pkg/services/accesscontrol/acimpl/service_test.go @@ -762,7 +762,7 @@ func TestService_SaveExternalServiceRole(t *testing.T) { runs: []run{ { cmd: accesscontrol.SaveExternalServiceRoleCommand{ - OrgID: 2, + AssignmentOrgID: 2, ServiceAccountID: 2, ExternalServiceID: "App 1", Permissions: []accesscontrol.Permission{{Action: "users:read", Scope: "users:id:1"}}, @@ -776,7 +776,7 @@ func TestService_SaveExternalServiceRole(t *testing.T) { runs: []run{ { cmd: accesscontrol.SaveExternalServiceRoleCommand{ - Global: true, + AssignmentOrgID: 1, ServiceAccountID: 2, ExternalServiceID: "App 1", Permissions: []accesscontrol.Permission{{Action: "users:read", Scope: "users:id:1"}}, @@ -785,7 +785,7 @@ func TestService_SaveExternalServiceRole(t *testing.T) { }, { cmd: accesscontrol.SaveExternalServiceRoleCommand{ - Global: true, + AssignmentOrgID: 1, ServiceAccountID: 2, ExternalServiceID: "App 1", Permissions: []accesscontrol.Permission{ @@ -802,7 +802,7 @@ func TestService_SaveExternalServiceRole(t *testing.T) { runs: []run{ { cmd: accesscontrol.SaveExternalServiceRoleCommand{ - OrgID: 2, + AssignmentOrgID: 2, ExternalServiceID: "App 1", Permissions: []accesscontrol.Permission{{Action: "users:read", Scope: "users:id:1"}}, }, @@ -825,7 +825,7 @@ func TestService_SaveExternalServiceRole(t *testing.T) { require.NoError(t, err) // Check that the permissions and assignment are stored correctly - perms, errGetPerms := ac.getUserPermissions(ctx, &user.SignedInUser{OrgID: r.cmd.OrgID, UserID: 2}, accesscontrol.Options{}) + perms, errGetPerms := ac.getUserPermissions(ctx, &user.SignedInUser{OrgID: r.cmd.AssignmentOrgID, UserID: 2}, accesscontrol.Options{}) require.NoError(t, errGetPerms) assert.ElementsMatch(t, r.cmd.Permissions, perms) } @@ -848,7 +848,7 @@ func TestService_DeleteExternalServiceRole(t *testing.T) { { name: "handles deleting role that exists", initCmd: &accesscontrol.SaveExternalServiceRoleCommand{ - Global: true, + AssignmentOrgID: 1, ServiceAccountID: 2, ExternalServiceID: "App 1", Permissions: []accesscontrol.Permission{{Action: "users:read", Scope: "users:id:1"}}, @@ -877,7 +877,7 @@ func TestService_DeleteExternalServiceRole(t *testing.T) { if tt.initCmd != nil { // Check that the permissions and assignment are removed correctly - perms, errGetPerms := ac.getUserPermissions(ctx, &user.SignedInUser{OrgID: tt.initCmd.OrgID, UserID: 2}, accesscontrol.Options{}) + perms, errGetPerms := ac.getUserPermissions(ctx, &user.SignedInUser{OrgID: tt.initCmd.AssignmentOrgID, UserID: 2}, accesscontrol.Options{}) require.NoError(t, errGetPerms) assert.Empty(t, perms) } diff --git a/pkg/services/accesscontrol/database/externalservices.go b/pkg/services/accesscontrol/database/externalservices.go index 4335b573348..ea69ff79fc5 100644 --- a/pkg/services/accesscontrol/database/externalservices.go +++ b/pkg/services/accesscontrol/database/externalservices.go @@ -79,7 +79,7 @@ func (s *AccessControlStore) SaveExternalServiceRole(ctx context.Context, cmd ac func genExternalServiceRole(cmd accesscontrol.SaveExternalServiceRoleCommand) accesscontrol.Role { name := extServiceRoleName(cmd.ExternalServiceID) role := accesscontrol.Role{ - OrgID: cmd.OrgID, + OrgID: accesscontrol.GlobalOrgID, // External Service Roles are global Version: 1, Name: name, UID: accesscontrol.PrefixedRoleUID(name), @@ -90,21 +90,15 @@ func genExternalServiceRole(cmd accesscontrol.SaveExternalServiceRoleCommand) ac Created: time.Now(), Updated: time.Now(), } - if cmd.Global { - role.OrgID = accesscontrol.GlobalOrgID - } return role } func genExternalServiceAssignment(cmd accesscontrol.SaveExternalServiceRoleCommand) accesscontrol.UserRole { assignment := accesscontrol.UserRole{ - OrgID: cmd.OrgID, + OrgID: cmd.AssignmentOrgID, UserID: cmd.ServiceAccountID, Created: time.Now(), } - if cmd.Global { - assignment.OrgID = accesscontrol.GlobalOrgID - } return assignment } diff --git a/pkg/services/accesscontrol/database/externalservices_test.go b/pkg/services/accesscontrol/database/externalservices_test.go index b782e791ff8..21bbbda171a 100644 --- a/pkg/services/accesscontrol/database/externalservices_test.go +++ b/pkg/services/accesscontrol/database/externalservices_test.go @@ -27,7 +27,7 @@ func TestAccessControlStore_SaveExternalServiceRole(t *testing.T) { { cmd: accesscontrol.SaveExternalServiceRoleCommand{ ExternalServiceID: "app1", - Global: true, + AssignmentOrgID: 1, ServiceAccountID: 1, Permissions: []accesscontrol.Permission{ {Action: "users:read", Scope: "users:id:1"}, @@ -44,7 +44,7 @@ func TestAccessControlStore_SaveExternalServiceRole(t *testing.T) { { cmd: accesscontrol.SaveExternalServiceRoleCommand{ ExternalServiceID: "app1", - Global: true, + AssignmentOrgID: 1, ServiceAccountID: 1, Permissions: []accesscontrol.Permission{ {Action: "users:read", Scope: "users:id:1"}, @@ -55,7 +55,7 @@ func TestAccessControlStore_SaveExternalServiceRole(t *testing.T) { { cmd: accesscontrol.SaveExternalServiceRoleCommand{ ExternalServiceID: "app1", - Global: true, + AssignmentOrgID: 1, ServiceAccountID: 1, Permissions: []accesscontrol.Permission{ {Action: "users:write", Scope: "users:id:1"}, @@ -65,51 +65,13 @@ func TestAccessControlStore_SaveExternalServiceRole(t *testing.T) { }, }, }, - { - name: "allow switching role from local to global and back", - runs: []run{ - { - cmd: accesscontrol.SaveExternalServiceRoleCommand{ - ExternalServiceID: "app1", - OrgID: 1, - ServiceAccountID: 1, - Permissions: []accesscontrol.Permission{ - {Action: "users:read", Scope: "users:id:1"}, - {Action: "users:read", Scope: "users:id:2"}, - }, - }, - }, - { - cmd: accesscontrol.SaveExternalServiceRoleCommand{ - ExternalServiceID: "app1", - Global: true, - ServiceAccountID: 1, - Permissions: []accesscontrol.Permission{ - {Action: "users:read", Scope: "users:id:1"}, - {Action: "users:read", Scope: "users:id:2"}, - }, - }, - }, - { - cmd: accesscontrol.SaveExternalServiceRoleCommand{ - ExternalServiceID: "app1", - OrgID: 1, - ServiceAccountID: 1, - Permissions: []accesscontrol.Permission{ - {Action: "users:read", Scope: "users:id:1"}, - {Action: "users:read", Scope: "users:id:2"}, - }, - }, - }, - }, - }, { name: "edge case - remove all permissions", runs: []run{ { cmd: accesscontrol.SaveExternalServiceRoleCommand{ ExternalServiceID: "app1", - Global: true, + AssignmentOrgID: 1, ServiceAccountID: 1, Permissions: []accesscontrol.Permission{ {Action: "users:read", Scope: "users:id:1"}, @@ -120,7 +82,7 @@ func TestAccessControlStore_SaveExternalServiceRole(t *testing.T) { { cmd: accesscontrol.SaveExternalServiceRoleCommand{ ExternalServiceID: "app1", - Global: true, + AssignmentOrgID: 1, ServiceAccountID: 1, Permissions: []accesscontrol.Permission{}, }, @@ -133,14 +95,14 @@ func TestAccessControlStore_SaveExternalServiceRole(t *testing.T) { { cmd: accesscontrol.SaveExternalServiceRoleCommand{ ExternalServiceID: "app1", - Global: true, + AssignmentOrgID: 1, ServiceAccountID: 1, }, }, { cmd: accesscontrol.SaveExternalServiceRoleCommand{ ExternalServiceID: "app1", - Global: true, + AssignmentOrgID: 1, ServiceAccountID: 2, }, wantErr: true, @@ -167,8 +129,7 @@ func TestAccessControlStore_SaveExternalServiceRole(t *testing.T) { storedRole, err := getRoleByUID(ctx, sess, accesscontrol.PrefixedRoleUID(extServiceRoleName(tt.runs[i].cmd.ExternalServiceID))) require.NoError(t, err) require.NotNil(t, storedRole) - require.Equal(t, tt.runs[i].cmd.Global, storedRole.Global(), "Incorrect global state of the role") - require.Equal(t, tt.runs[i].cmd.OrgID, storedRole.OrgID, "Incorrect OrgID of the role") + require.True(t, storedRole.Global(), "Incorrect global state of the role") storedPerm, err := getRolePermissions(ctx, sess, storedRole.ID) require.NoError(t, err) @@ -181,8 +142,7 @@ func TestAccessControlStore_SaveExternalServiceRole(t *testing.T) { has, err := sess.Where("role_id = ? AND user_id = ?", storedRole.ID, tt.runs[i].cmd.ServiceAccountID).Get(&assignment) require.NoError(t, err) require.True(t, has) - require.Equal(t, tt.runs[i].cmd.Global, assignment.OrgID == accesscontrol.GlobalOrgID, "Incorrect global state of the assignment") - require.Equal(t, tt.runs[i].cmd.OrgID, assignment.OrgID, "Incorrect OrgID for the role assignment") + require.Equal(t, tt.runs[i].cmd.AssignmentOrgID, assignment.OrgID, "Incorrect OrgID for the role assignment") return nil }) @@ -206,27 +166,10 @@ func TestAccessControlStore_DeleteExternalServiceRole(t *testing.T) { wantErr: false, }, { - name: "delete local role", + name: "delete role", init: func(t *testing.T, ctx context.Context, s *AccessControlStore) { errSave := s.SaveExternalServiceRole(ctx, accesscontrol.SaveExternalServiceRoleCommand{ - OrgID: 2, - ExternalServiceID: extID, - ServiceAccountID: 3, - Permissions: []accesscontrol.Permission{ - {Action: "users:read", Scope: "users:id:1"}, - {Action: "users:write", Scope: "users:id:1"}, - }, - }) - require.NoError(t, errSave) - }, - id: extID, - wantErr: false, - }, - { - name: "delete global role", - init: func(t *testing.T, ctx context.Context, s *AccessControlStore) { - errSave := s.SaveExternalServiceRole(ctx, accesscontrol.SaveExternalServiceRoleCommand{ - Global: true, + AssignmentOrgID: 2, ExternalServiceID: extID, ServiceAccountID: 3, Permissions: []accesscontrol.Permission{ diff --git a/pkg/services/accesscontrol/models.go b/pkg/services/accesscontrol/models.go index 81b7ec3be61..14795734b73 100644 --- a/pkg/services/accesscontrol/models.go +++ b/pkg/services/accesscontrol/models.go @@ -274,8 +274,7 @@ type SetResourcePermissionCommand struct { } type SaveExternalServiceRoleCommand struct { - OrgID int64 - Global bool + AssignmentOrgID int64 ExternalServiceID string ServiceAccountID int64 Permissions []Permission @@ -289,10 +288,6 @@ func (cmd *SaveExternalServiceRoleCommand) Validate() error { // slugify the external service id ID for the role to have correct name and uid cmd.ExternalServiceID = slugify.Slugify(cmd.ExternalServiceID) - if (cmd.OrgID == GlobalOrgID) != cmd.Global { - return fmt.Errorf("invalid org id %d for global role %t", cmd.OrgID, cmd.Global) - } - // Check and deduplicate permissions if cmd.Permissions == nil || len(cmd.Permissions) == 0 { return errors.New("no permissions provided") diff --git a/pkg/services/accesscontrol/models_test.go b/pkg/services/accesscontrol/models_test.go index 64595f0ffb8..c46dd45500b 100644 --- a/pkg/services/accesscontrol/models_test.go +++ b/pkg/services/accesscontrol/models_test.go @@ -15,21 +15,10 @@ func TestSaveExternalServiceRoleCommand_Validate(t *testing.T) { wantPermissions []Permission wantErr bool }{ - { - name: "invalid global statement", - cmd: SaveExternalServiceRoleCommand{ - OrgID: 1, - Global: true, - ExternalServiceID: "app 1", - ServiceAccountID: 2, - Permissions: []Permission{{Action: "users:read", Scope: "users:id:1"}}, - }, - wantErr: true, - }, { name: "invalid no permissions", cmd: SaveExternalServiceRoleCommand{ - OrgID: 1, + AssignmentOrgID: 1, ExternalServiceID: "app 1", ServiceAccountID: 2, Permissions: []Permission{}, @@ -39,7 +28,7 @@ func TestSaveExternalServiceRoleCommand_Validate(t *testing.T) { { name: "invalid service account id", cmd: SaveExternalServiceRoleCommand{ - OrgID: 1, + AssignmentOrgID: 1, ExternalServiceID: "app 1", ServiceAccountID: -1, Permissions: []Permission{{Action: "users:read", Scope: "users:id:1"}}, @@ -49,7 +38,7 @@ func TestSaveExternalServiceRoleCommand_Validate(t *testing.T) { { name: "invalid no Ext Service ID", cmd: SaveExternalServiceRoleCommand{ - OrgID: 1, + AssignmentOrgID: 1, ServiceAccountID: 2, Permissions: []Permission{{Action: "users:read", Scope: "users:id:1"}}, }, @@ -59,7 +48,7 @@ func TestSaveExternalServiceRoleCommand_Validate(t *testing.T) { name: "slugify the external service ID correctly", cmd: SaveExternalServiceRoleCommand{ ExternalServiceID: "ThisIs a Very Strange ___ App Name?", - Global: true, + AssignmentOrgID: 1, ServiceAccountID: 2, Permissions: []Permission{{Action: "users:read", Scope: "users:id:1"}}, }, @@ -69,7 +58,7 @@ func TestSaveExternalServiceRoleCommand_Validate(t *testing.T) { { name: "invalid empty Action", cmd: SaveExternalServiceRoleCommand{ - OrgID: 1, + AssignmentOrgID: 1, ExternalServiceID: "app 1", ServiceAccountID: 2, Permissions: []Permission{{Action: "", Scope: "users:id:1"}}, @@ -80,7 +69,7 @@ func TestSaveExternalServiceRoleCommand_Validate(t *testing.T) { { name: "permission deduplication", cmd: SaveExternalServiceRoleCommand{ - OrgID: 1, + AssignmentOrgID: 1, ExternalServiceID: "app 1", ServiceAccountID: 2, Permissions: []Permission{ diff --git a/pkg/services/extsvcauth/oauthserver/oasimpl/service.go b/pkg/services/extsvcauth/oauthserver/oasimpl/service.go index a6f99863a27..8c5a90248d2 100644 --- a/pkg/services/extsvcauth/oauthserver/oasimpl/service.go +++ b/pkg/services/extsvcauth/oauthserver/oasimpl/service.go @@ -458,7 +458,12 @@ func (*OAuth2ServiceImpl) handleRegistrationPermissions(registration *extsvcauth // handlePluginStateChanged reset the client authorized grant_types according to the plugin state func (s *OAuth2ServiceImpl) handlePluginStateChanged(ctx context.Context, event *pluginsettings.PluginStateChangedEvent) error { - s.logger.Info("Plugin state changed", "pluginId", event.PluginId, "enabled", event.Enabled) + s.logger.Debug("Plugin state changed", "pluginId", event.PluginId, "enabled", event.Enabled) + + if event.OrgId != extsvcauth.TmpOrgID { + s.logger.Debug("External Service not tied to this organization", "OrgId", event.OrgId) + return nil + } // Retrieve client associated to the plugin client, err := s.sqlstore.GetExternalServiceByName(ctx, event.PluginId) diff --git a/pkg/services/serviceaccounts/extsvcaccounts/service.go b/pkg/services/serviceaccounts/extsvcaccounts/service.go index 1306a9d66db..84c984a0ac3 100644 --- a/pkg/services/serviceaccounts/extsvcaccounts/service.go +++ b/pkg/services/serviceaccounts/extsvcaccounts/service.go @@ -297,8 +297,7 @@ func (esa *ExtSvcAccountsService) saveExtSvcAccount(ctx context.Context, cmd *sa // update the service account's permissions esa.logger.Debug("Update role permissions", "service", cmd.ExtSvcSlug, "saID", cmd.SaID) if err := esa.acSvc.SaveExternalServiceRole(ctx, ac.SaveExternalServiceRoleCommand{ - OrgID: ac.GlobalOrgID, - Global: true, + AssignmentOrgID: cmd.OrgID, ExternalServiceID: cmd.ExtSvcSlug, ServiceAccountID: cmd.SaID, Permissions: cmd.Permissions, @@ -397,17 +396,17 @@ func (esa *ExtSvcAccountsService) DeleteExtSvcCredentials(ctx context.Context, o } func (esa *ExtSvcAccountsService) handlePluginStateChanged(ctx context.Context, event *pluginsettings.PluginStateChangedEvent) error { - esa.logger.Info("Plugin state changed", "pluginId", event.PluginId, "enabled", event.Enabled) + esa.logger.Debug("Plugin state changed", "pluginId", event.PluginId, "enabled", event.Enabled) errEnable := esa.EnableExtSvcAccount(ctx, &sa.EnableExtSvcAccountCmd{ ExtSvcSlug: event.PluginId, Enabled: event.Enabled, - OrgID: extsvcauth.TmpOrgID, + OrgID: event.OrgId, }) // Ignore service account not found error if errors.Is(errEnable, sa.ErrServiceAccountNotFound) { - esa.logger.Debug("No ext svc account with this plugin", "pluginId", event.PluginId) + esa.logger.Debug("No ext svc account with this plugin", "pluginId", event.PluginId, "orgId", event.OrgId) return nil } return errEnable diff --git a/pkg/services/serviceaccounts/extsvcaccounts/service_test.go b/pkg/services/serviceaccounts/extsvcaccounts/service_test.go index 8c0af676a43..ab0c14c0aef 100644 --- a/pkg/services/serviceaccounts/extsvcaccounts/service_test.go +++ b/pkg/services/serviceaccounts/extsvcaccounts/service_test.go @@ -84,7 +84,7 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) { mock.Anything, mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool { return cmd.ServiceAccountID == extSvcAccID && cmd.ExternalServiceID == extSvcSlug && - cmd.OrgID == int64(ac.GlobalOrgID) && len(cmd.Permissions) == 1 && + cmd.AssignmentOrgID == extSvcOrgID && len(cmd.Permissions) == 1 && cmd.Permissions[0] == extSvcPerms[0] })). Return(nil) @@ -133,7 +133,7 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) { mock.Anything, mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool { return cmd.ServiceAccountID == extSvcAccount.Id && cmd.ExternalServiceID == extSvcSlug && - cmd.OrgID == int64(ac.GlobalOrgID) && len(cmd.Permissions) == 1 && + cmd.AssignmentOrgID == extSvcOrgID && len(cmd.Permissions) == 1 && cmd.Permissions[0] == extSvcPerms[0] })). Return(nil) @@ -158,7 +158,7 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) { mock.Anything, mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool { return cmd.ServiceAccountID == int64(11) && cmd.ExternalServiceID == extSvcSlug && - cmd.OrgID == int64(ac.GlobalOrgID) && len(cmd.Permissions) == 1 && + cmd.AssignmentOrgID == extSvcOrgID && len(cmd.Permissions) == 1 && cmd.Permissions[0] == extSvcPerms[0] })). Return(nil) @@ -228,7 +228,7 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) { mock.Anything, mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool { return cmd.ServiceAccountID == extSvcAccID && cmd.ExternalServiceID == extSvcSlug && - cmd.OrgID == int64(ac.GlobalOrgID) && len(cmd.Permissions) == 1 && + cmd.AssignmentOrgID == tmpOrgID && len(cmd.Permissions) == 1 && cmd.Permissions[0] == extSvcPerms[0] })). Return(nil) @@ -291,14 +291,14 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) { return cmd.Name == sa.ExtSvcPrefix+extSvcSlug && *cmd.Role == roletype.RoleNone })). Return(extSvcAccount, nil) - env.SaSvc.On("EnableServiceAccount", mock.Anything, extsvcauth.TmpOrgID, extSvcAccID, true).Return(nil) + env.SaSvc.On("EnableServiceAccount", mock.Anything, tmpOrgID, 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.OrgID == int64(ac.GlobalOrgID) && len(cmd.Permissions) == 1 && + cmd.AssignmentOrgID == tmpOrgID && len(cmd.Permissions) == 1 && cmd.Permissions[0] == extSvcPerms[0] })). Return(nil) @@ -327,7 +327,7 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) { mock.Anything, mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool { return cmd.ServiceAccountID == int64(11) && cmd.ExternalServiceID == extSvcSlug && - cmd.OrgID == int64(ac.GlobalOrgID) && len(cmd.Permissions) == 1 && + cmd.AssignmentOrgID == tmpOrgID && len(cmd.Permissions) == 1 && cmd.Permissions[0] == extSvcPerms[0] })). Return(nil)