From 4570131fe5bf054fadcd602c3038ab19a3048e3f Mon Sep 17 00:00:00 2001 From: Ieva Date: Thu, 9 Feb 2023 12:19:08 +0000 Subject: [PATCH] Folders: simplify guardian permissions checks (#63183) simplify code --- pkg/services/folder/folderimpl/folder.go | 113 +++++------------- pkg/services/folder/folderimpl/folder_test.go | 6 +- 2 files changed, 31 insertions(+), 88 deletions(-) diff --git a/pkg/services/folder/folderimpl/folder.go b/pkg/services/folder/folderimpl/folder.go index fe267bfafca..ee6bd68c705 100644 --- a/pkg/services/folder/folderimpl/folder.go +++ b/pkg/services/folder/folderimpl/folder.go @@ -95,17 +95,17 @@ func (s *Service) Get(ctx context.Context, cmd *folder.GetFolderQuery) (*folder. var err error switch { case cmd.UID != nil: - dashFolder, err = s.getFolderByUID(ctx, cmd.SignedInUser, cmd.OrgID, *cmd.UID) + dashFolder, err = s.getFolderByUID(ctx, cmd.OrgID, *cmd.UID) if err != nil { return nil, err } case cmd.ID != nil: - dashFolder, err = s.getFolderByID(ctx, cmd.SignedInUser, *cmd.ID, cmd.OrgID) + dashFolder, err = s.getFolderByID(ctx, *cmd.ID, cmd.OrgID) if err != nil { return nil, err } case cmd.Title != nil: - dashFolder, err = s.getFolderByTitle(ctx, cmd.SignedInUser, cmd.OrgID, *cmd.Title) + dashFolder, err = s.getFolderByTitle(ctx, cmd.OrgID, *cmd.Title) if err != nil { return nil, err } @@ -113,6 +113,25 @@ func (s *Service) Get(ctx context.Context, cmd *folder.GetFolderQuery) (*folder. return nil, folder.ErrBadRequest.Errorf("either on of UID, ID, Title fields must be present") } + if dashFolder.IsGeneral() { + return dashFolder, nil + } + + // do not get guardian by the folder ID because it differs from the nested folder ID + // and the legacy folder ID has been associated with the permissions: + // use the folde UID instead that is the same for both + g, err := guardian.NewByUID(ctx, dashFolder.UID, dashFolder.OrgID, cmd.SignedInUser) + if err != nil { + return nil, err + } + + if canView, err := g.CanView(); err != nil || !canView { + if err != nil { + return nil, toFolderError(err) + } + return nil, dashboards.ErrFolderAccessDenied + } + if !s.features.IsEnabled(featuremgmt.FlagNestedFolders) { return dashFolder, nil } @@ -122,30 +141,11 @@ func (s *Service) Get(ctx context.Context, cmd *folder.GetFolderQuery) (*folder. cmd.UID = &dashFolder.UID } - if dashFolder.IsGeneral() { - return dashFolder, nil - } - f, err := s.store.Get(ctx, *cmd) if err != nil { return nil, err } - // do not get guardian by the folder ID because it differs from the nested folder ID - // and the legacy folder ID has been associated with the permissions: - // use the folde UID instead that is the same for both - g, err := guardian.NewByUID(ctx, f.UID, f.OrgID, cmd.SignedInUser) - if err != nil { - return nil, err - } - - if canView, err := g.CanView(); err != nil || !canView { - if err != nil { - return nil, toFolderError(err) - } - return nil, dashboards.ErrFolderAccessDenied - } - // always expose the dashboard store sequential ID f.ID = dashFolder.ID @@ -193,77 +193,20 @@ func (s *Service) GetParents(ctx context.Context, q folder.GetParentsQuery) ([]* return s.store.GetParents(ctx, q) } -func (s *Service) getFolderByID(ctx context.Context, user *user.SignedInUser, id int64, orgID int64) (*folder.Folder, error) { +func (s *Service) getFolderByID(ctx context.Context, id int64, orgID int64) (*folder.Folder, error) { if id == 0 { return &folder.GeneralFolder, nil } - dashFolder, err := s.dashboardFolderStore.GetFolderByID(ctx, orgID, id) - if err != nil { - return nil, err - } - - // do not get guardian by the folder ID because it differs from the nested folder ID - // and the legacy folder ID has been associated with the permissions: - // use the folde UID instead that is the same for both - g, err := guardian.NewByUID(ctx, dashFolder.UID, orgID, user) - if err != nil { - return nil, err - } - - if canView, err := g.CanView(); err != nil || !canView { - if err != nil { - return nil, toFolderError(err) - } - return nil, dashboards.ErrFolderAccessDenied - } - - return dashFolder, nil + return s.dashboardFolderStore.GetFolderByID(ctx, orgID, id) } -func (s *Service) getFolderByUID(ctx context.Context, user *user.SignedInUser, orgID int64, uid string) (*folder.Folder, error) { - dashFolder, err := s.dashboardFolderStore.GetFolderByUID(ctx, orgID, uid) - if err != nil { - return nil, err - } - - // do not get guardian by the folder ID because it differs from the nested folder ID - // and the legacy folder ID has been associated with the permissions: - // use the folde UID instead that is the same for both - g, err := guardian.NewByUID(ctx, dashFolder.UID, orgID, user) - if err != nil { - return nil, err - } - - if canView, err := g.CanView(); err != nil || !canView { - if err != nil { - return nil, toFolderError(err) - } - return nil, dashboards.ErrFolderAccessDenied - } - - return dashFolder, nil +func (s *Service) getFolderByUID(ctx context.Context, orgID int64, uid string) (*folder.Folder, error) { + return s.dashboardFolderStore.GetFolderByUID(ctx, orgID, uid) } -func (s *Service) getFolderByTitle(ctx context.Context, user *user.SignedInUser, orgID int64, title string) (*folder.Folder, error) { - dashFolder, err := s.dashboardFolderStore.GetFolderByTitle(ctx, orgID, title) - if err != nil { - return nil, err - } - - g, err := guardian.NewByUID(ctx, dashFolder.UID, orgID, user) - if err != nil { - return nil, err - } - - if canView, err := g.CanView(); err != nil || !canView { - if err != nil { - return nil, toFolderError(err) - } - return nil, dashboards.ErrFolderAccessDenied - } - - return dashFolder, nil +func (s *Service) getFolderByTitle(ctx context.Context, orgID int64, title string) (*folder.Folder, error) { + return s.dashboardFolderStore.GetFolderByTitle(ctx, orgID, title) } func (s *Service) Create(ctx context.Context, cmd *folder.CreateFolderCommand) (*folder.Folder, error) { diff --git a/pkg/services/folder/folderimpl/folder_test.go b/pkg/services/folder/folderimpl/folder_test.go index 71147b529c4..fc9b12f84a0 100644 --- a/pkg/services/folder/folderimpl/folder_test.go +++ b/pkg/services/folder/folderimpl/folder_test.go @@ -258,7 +258,7 @@ func TestIntegrationFolderService(t *testing.T) { folderStore.On("GetFolderByID", mock.Anything, orgID, expected.ID).Return(expected, nil) - actual, err := service.getFolderByID(context.Background(), usr, expected.ID, orgID) + actual, err := service.getFolderByID(context.Background(), expected.ID, orgID) require.Equal(t, expected, actual) require.NoError(t, err) }) @@ -269,7 +269,7 @@ func TestIntegrationFolderService(t *testing.T) { folderStore.On("GetFolderByUID", mock.Anything, orgID, expected.UID).Return(expected, nil) - actual, err := service.getFolderByUID(context.Background(), usr, orgID, expected.UID) + actual, err := service.getFolderByUID(context.Background(), orgID, expected.UID) require.Equal(t, expected, actual) require.NoError(t, err) }) @@ -279,7 +279,7 @@ func TestIntegrationFolderService(t *testing.T) { folderStore.On("GetFolderByTitle", mock.Anything, orgID, expected.Title).Return(expected, nil) - actual, err := service.getFolderByTitle(context.Background(), usr, orgID, expected.Title) + actual, err := service.getFolderByTitle(context.Background(), orgID, expected.Title) require.Equal(t, expected, actual) require.NoError(t, err) })