RBAC: Add a function to delete external service roles (#68317)

* RBAC: Add function to delete external service roles * Adding a test to the service * Update pkg/services/accesscontrol/acimpl/service_test.go Co-authored-by: Ieva <ieva.vasiljeva@grafana.com> --------- Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>
3 years ago · d7eea0d207
parent 0612f1f87a
commit d7eea0d207
7 changed files with 251 additions and 3 deletions
--- a/pkg/services/accesscontrol/accesscontrol.go
+++ b/pkg/services/accesscontrol/accesscontrol.go
@ -40,6 +40,8 @@ type Service interface {
 	DeclareFixedRoles(registrations ...RoleRegistration) error
 	// SaveExternalServiceRole creates or updates an external service's role and assigns it to a given service account id.
 	SaveExternalServiceRole(ctx context.Context, cmd SaveExternalServiceRoleCommand) error
+	// DeleteExternalServiceRole removes an external service's role and its assignment.
+	DeleteExternalServiceRole(ctx context.Context, externalServiceID string) error
 	//IsDisabled returns if access control is enabled or not
 	IsDisabled() bool
 }
--- a/pkg/services/accesscontrol/acimpl/service.go
+++ b/pkg/services/accesscontrol/acimpl/service.go
@ -14,6 +14,7 @@ import (
 	"github.com/grafana/grafana/pkg/infra/localcache"
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/infra/metrics"
+	"github.com/grafana/grafana/pkg/infra/slugify"
 	"github.com/grafana/grafana/pkg/plugins"
 	"github.com/grafana/grafana/pkg/services/accesscontrol"
 	"github.com/grafana/grafana/pkg/services/accesscontrol/api"
@ -63,6 +64,7 @@ type store interface {
 	GetUsersBasicRoles(ctx context.Context, userFilter []int64, orgID int64) (map[int64][]string, error)
 	DeleteUserPermissions(ctx context.Context, orgID, userID int64) error
 	SaveExternalServiceRole(ctx context.Context, cmd accesscontrol.SaveExternalServiceRoleCommand) error
+	DeleteExternalServiceRole(ctx context.Context, externalServiceID string) error
 }

 // Service is the service implementing role based access control.
@ -416,7 +418,7 @@ func (s *Service) SaveExternalServiceRole(ctx context.Context, cmd accesscontrol
 	}

 	if !s.features.IsEnabled(featuremgmt.FlagExternalServiceAuth) {
-		s.log.Debug("registering external service role is behind a feature flag, enable it to use this feature.")
+		s.log.Debug("registering an external service role is behind a feature flag, enable it to use this feature.")
 		return nil
 	}

@ -426,3 +428,19 @@ func (s *Service) SaveExternalServiceRole(ctx context.Context, cmd accesscontrol

 	return s.store.SaveExternalServiceRole(ctx, cmd)
 }
+
+func (s *Service) DeleteExternalServiceRole(ctx context.Context, externalServiceID string) error {
+	// If accesscontrol is disabled no need to delete the external service role
+	if accesscontrol.IsDisabled(s.cfg) {
+		return nil
+	}
+
+	if !s.features.IsEnabled(featuremgmt.FlagExternalServiceAuth) {
+		s.log.Debug("deleting an external service role is behind a feature flag, enable it to use this feature.")
+		return nil
+	}
+
+	slug := slugify.Slugify(externalServiceID)
+
+	return s.store.DeleteExternalServiceRole(ctx, slug)
+}
--- a/pkg/services/accesscontrol/acimpl/service_test.go
+++ b/pkg/services/accesscontrol/acimpl/service_test.go
@ -852,3 +852,55 @@ func TestService_SaveExternalServiceRole(t *testing.T) {
 		})
 	}
 }
+
+func TestService_DeleteExternalServiceRole(t *testing.T) {
+	tests := []struct {
+		name              string
+		initCmd           *accesscontrol.SaveExternalServiceRoleCommand
+		externalServiceID string
+		wantErr           bool
+	}{
+		{
+			name:              "handles deleting role that doesn't exist",
+			externalServiceID: "App 1",
+			wantErr:           false,
+		},
+		{
+			name: "handles deleting role that exists",
+			initCmd: &accesscontrol.SaveExternalServiceRoleCommand{
+				Global:            true,
+				ServiceAccountID:  2,
+				ExternalServiceID: "App 1",
+				Permissions:       []accesscontrol.Permission{{Action: "users:read", Scope: "users:id:1"}},
+			},
+			externalServiceID: "App 1",
+			wantErr:           false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := context.Background()
+			ac := setupTestEnv(t)
+			ac.features = featuremgmt.WithFeatures(featuremgmt.FlagExternalServiceAuth)
+
+			if tt.initCmd != nil {
+				err := ac.SaveExternalServiceRole(ctx, *tt.initCmd)
+				require.NoError(t, err)
+			}
+
+			err := ac.DeleteExternalServiceRole(ctx, tt.externalServiceID)
+			if tt.wantErr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+
+			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{})
+				require.NoError(t, errGetPerms)
+				assert.Empty(t, perms)
+			}
+		})
+	}
+}
--- a/pkg/services/accesscontrol/actest/fake.go
+++ b/pkg/services/accesscontrol/actest/fake.go
@ -57,6 +57,10 @@ func (f FakeService) SaveExternalServiceRole(ctx context.Context, cmd accesscont
 	return f.ExpectedErr
 }

+func (f FakeService) DeleteExternalServiceRole(ctx context.Context, externalServiceID string) error {
+	return f.ExpectedErr
+}
+
 var _ accesscontrol.AccessControl = new(FakeAccessControl)

 type FakeAccessControl struct {
@ -103,6 +107,10 @@ func (f FakeStore) SaveExternalServiceRole(ctx context.Context, cmd accesscontro
 	return f.ExpectedErr
 }

+func (f FakeStore) DeleteExternalServiceRole(ctx context.Context, externalServiceID string) error {
+	return f.ExpectedErr
+}
+
 var _ accesscontrol.PermissionsService = new(FakePermissionsService)

 type FakePermissionsService struct {
--- a/pkg/services/accesscontrol/database/externalservices.go
+++ b/pkg/services/accesscontrol/database/externalservices.go
@ -10,6 +10,52 @@ import (
 	"github.com/grafana/grafana/pkg/services/accesscontrol"
 )

+func extServiceRoleUID(externalServiceID string) string {
+	uid := fmt.Sprintf("%s%s_permissions", accesscontrol.ExternalServiceRoleUIDPrefix, externalServiceID)
+	return uid
+}
+
+func extServiceRoleName(externalServiceID string) string {
+	name := fmt.Sprintf("%s%s:permissions", accesscontrol.ExternalServiceRolePrefix, externalServiceID)
+	return name
+}
+
+func (s *AccessControlStore) DeleteExternalServiceRole(ctx context.Context, externalServiceID string) error {
+	uid := extServiceRoleUID(externalServiceID)
+
+	return s.sql.WithDbSession(ctx, func(sess *db.Session) error {
+		stored, errGet := getRoleByUID(ctx, sess, uid)
+		if errGet != nil {
+			// Role not found, nothing to do
+			if errors.Is(errGet, accesscontrol.ErrRoleNotFound) {
+				return nil
+			}
+			return errGet
+		}
+
+		// Delete the assignments
+		_, errDel := sess.Exec("DELETE FROM user_role WHERE role_id = ?", stored.ID)
+		if errDel != nil {
+			return errDel
+		}
+		// Shouldn't happen but just in case delete any team assignments
+		_, errDel = sess.Exec("DELETE FROM team_role WHERE role_id = ?", stored.ID)
+		if errDel != nil {
+			return errDel
+		}
+
+		// Delete the permissions
+		_, errDel = sess.Exec("DELETE FROM permission WHERE role_id = ?", stored.ID)
+		if errDel != nil {
+			return errDel
+		}
+
+		// Delete the role
+		_, errDel = sess.Exec("DELETE FROM role WHERE id = ?", stored.ID)
+		return errDel
+	})
+}
+
 func (s *AccessControlStore) SaveExternalServiceRole(ctx context.Context, cmd accesscontrol.SaveExternalServiceRoleCommand) error {
 	role := genExternalServiceRole(cmd)
 	assignment := genExternalServiceAssignment(cmd)
@ -39,8 +85,8 @@ func genExternalServiceRole(cmd accesscontrol.SaveExternalServiceRoleCommand) ac
 	role := accesscontrol.Role{
 		OrgID:       cmd.OrgID,
 		Version:     1,
-		Name:        fmt.Sprintf("%s%s:permissions", accesscontrol.ExternalServiceRolePrefix, cmd.ExternalServiceID),
-		UID:         fmt.Sprintf("%s%s_permissions", accesscontrol.ExternalServiceRoleUIDPrefix, cmd.ExternalServiceID),
+		Name:        extServiceRoleName(cmd.ExternalServiceID),
+		UID:         extServiceRoleUID(cmd.ExternalServiceID),
 		DisplayName: fmt.Sprintf("External Service %s Permissions", cmd.ExternalServiceID),
 		Description: fmt.Sprintf("External Service %s permissions", cmd.ExternalServiceID),
 		Group:       "External Service",
--- a/pkg/services/accesscontrol/database/externalservices_test.go
+++ b/pkg/services/accesscontrol/database/externalservices_test.go
@ -2,6 +2,7 @@ package database

 import (
 	"context"
+	"errors"
 	"fmt"
 	"testing"

@ -189,3 +190,113 @@ func TestAccessControlStore_SaveExternalServiceRole(t *testing.T) {
 		})
 	}
 }
+
+func TestAccessControlStore_DeleteExternalServiceRole(t *testing.T) {
+	extID := "app1"
+	tests := []struct {
+		name    string
+		init    func(t *testing.T, ctx context.Context, s *AccessControlStore)
+		id      string
+		wantErr bool
+	}{
+		{
+			name:    "delete no role",
+			id:      extID,
+			wantErr: false,
+		},
+		{
+			name: "delete local 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,
+					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,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := context.Background()
+			s := &AccessControlStore{
+				sql: db.InitTestDB(t),
+			}
+			if tt.init != nil {
+				tt.init(t, ctx, s)
+			}
+			roleID := int64(-1)
+			err := s.sql.WithDbSession(ctx, func(sess *db.Session) error {
+				role, err := getRoleByUID(ctx, sess, extServiceRoleUID(tt.id))
+				if err != nil && !errors.Is(err, accesscontrol.ErrRoleNotFound) {
+					return err
+				}
+				if role != nil {
+					roleID = role.ID
+				}
+				return nil
+			})
+			require.NoError(t, err)
+			err = s.DeleteExternalServiceRole(ctx, tt.id)
+			if tt.wantErr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+
+			// Only check removal if the role existed before
+			if roleID == -1 {
+				return
+			}
+
+			// Assignments should be deleted
+			_ = s.sql.WithDbSession(ctx, func(sess *db.Session) error {
+				var assignment accesscontrol.UserRole
+				count, err := sess.Where("role_id = ?", roleID).Count(&assignment)
+				require.NoError(t, err)
+				require.Zero(t, count)
+				return nil
+			})
+
+			// Permissions should be deleted
+			_ = s.sql.WithDbSession(ctx, func(sess *db.Session) error {
+				var permission accesscontrol.Permission
+				count, err := sess.Where("role_id = ?", roleID).Count(&permission)
+				require.NoError(t, err)
+				require.Zero(t, count)
+				return nil
+			})
+
+			// Role should be deleted
+			_ = s.sql.WithDbSession(ctx, func(sess *db.Session) error {
+				storedRole, err := getRoleByUID(ctx, sess, extServiceRoleUID(tt.id))
+				require.ErrorIs(t, err, accesscontrol.ErrRoleNotFound)
+				require.Nil(t, storedRole)
+				return nil
+			})
+		})
+	}
+}
--- a/pkg/services/accesscontrol/mock/mock.go
+++ b/pkg/services/accesscontrol/mock/mock.go
@ -31,6 +31,7 @@ type Calls struct {
 	SearchUsersPermissions         []interface{}
 	SearchUserPermissions          []interface{}
 	SaveExternalServiceRole        []interface{}
+	DeleteExternalServiceRole      []interface{}
 }

 type Mock struct {
@ -58,6 +59,7 @@ type Mock struct {
 	SearchUsersPermissionsFunc         func(context.Context, *user.SignedInUser, int64, accesscontrol.SearchOptions) (map[int64][]accesscontrol.Permission, error)
 	SearchUserPermissionsFunc          func(ctx context.Context, orgID int64, searchOptions accesscontrol.SearchOptions) ([]accesscontrol.Permission, error)
 	SaveExternalServiceRoleFunc        func(ctx context.Context, cmd accesscontrol.SaveExternalServiceRoleCommand) error
+	DeleteExternalServiceRoleFunc      func(ctx context.Context, externalServiceID string) error

 	scopeResolvers accesscontrol.Resolvers
 }
@ -246,3 +248,12 @@ func (m *Mock) SaveExternalServiceRole(ctx context.Context, cmd accesscontrol.Sa
 	}
 	return nil
 }
+
+func (m *Mock) DeleteExternalServiceRole(ctx context.Context, externalServiceID string) error {
+	m.Calls.DeleteExternalServiceRole = append(m.Calls.DeleteExternalServiceRole, []interface{}{ctx, externalServiceID})
+	// Use override if provided
+	if m.DeleteExternalServiceRoleFunc != nil {
+		return m.DeleteExternalServiceRoleFunc(ctx, externalServiceID)
+	}
+	return nil
+}