From 42fb42a90d54f39209f11a7e764ce31b9d405511 Mon Sep 17 00:00:00 2001 From: Khushi Jain <57278642+khushijain21@users.noreply.github.com> Date: Wed, 11 Oct 2023 17:29:13 +0530 Subject: [PATCH] PublicDashboards: Add validation deletion (#75336) --- pkg/services/publicdashboards/api/api.go | 9 ++- .../public_dashboard_service_mock.go | 10 +-- .../publicdashboards/publicdashboard.go | 2 +- .../publicdashboards/service/service.go | 15 ++++- .../publicdashboards/service/service_test.go | 63 +++++++++++++------ 5 files changed, 72 insertions(+), 27 deletions(-) diff --git a/pkg/services/publicdashboards/api/api.go b/pkg/services/publicdashboards/api/api.go index 708918f14f2..8ab2b0172e7 100644 --- a/pkg/services/publicdashboards/api/api.go +++ b/pkg/services/publicdashboards/api/api.go @@ -220,10 +220,15 @@ func (api *Api) UpdatePublicDashboard(c *contextmodel.ReqContext) response.Respo func (api *Api) DeletePublicDashboard(c *contextmodel.ReqContext) response.Response { uid := web.Params(c.Req)[":uid"] if !validation.IsValidShortUID(uid) { - return response.Err(ErrInvalidUid.Errorf("UpdatePublicDashboard: invalid Uid %s", uid)) + return response.Err(ErrInvalidUid.Errorf("DeletePublicDashboard: invalid Uid %s", uid)) } - err := api.PublicDashboardService.Delete(c.Req.Context(), uid) + dashboardUid := web.Params(c.Req)[":dashboardUid"] + if !validation.IsValidShortUID(dashboardUid) { + return response.Err(ErrInvalidUid.Errorf("DeletePublicDashboard: invalid dashboard Uid %s", dashboardUid)) + } + + err := api.PublicDashboardService.Delete(c.Req.Context(), uid, dashboardUid) if err != nil { return response.Err(err) } diff --git a/pkg/services/publicdashboards/public_dashboard_service_mock.go b/pkg/services/publicdashboards/public_dashboard_service_mock.go index 4e8e03362dd..949e0d9b3d3 100644 --- a/pkg/services/publicdashboards/public_dashboard_service_mock.go +++ b/pkg/services/publicdashboards/public_dashboard_service_mock.go @@ -49,13 +49,13 @@ func (_m *FakePublicDashboardService) Create(ctx context.Context, u *user.Signed return r0, r1 } -// Delete provides a mock function with given fields: ctx, uid -func (_m *FakePublicDashboardService) Delete(ctx context.Context, uid string) error { - ret := _m.Called(ctx, uid) +// Delete provides a mock function with given fields: ctx, uid, dashboardUid +func (_m *FakePublicDashboardService) Delete(ctx context.Context, uid string, dashboardUid string) error { + ret := _m.Called(ctx, uid, dashboardUid) var r0 error - if rf, ok := ret.Get(0).(func(context.Context, string) error); ok { - r0 = rf(ctx, uid) + if rf, ok := ret.Get(0).(func(context.Context, string, string) error); ok { + r0 = rf(ctx, uid, dashboardUid) } else { r0 = ret.Error(0) } diff --git a/pkg/services/publicdashboards/publicdashboard.go b/pkg/services/publicdashboards/publicdashboard.go index df070738b07..d6fe478c87b 100644 --- a/pkg/services/publicdashboards/publicdashboard.go +++ b/pkg/services/publicdashboards/publicdashboard.go @@ -26,7 +26,7 @@ type Service interface { Find(ctx context.Context, uid string) (*PublicDashboard, error) Create(ctx context.Context, u *user.SignedInUser, dto *SavePublicDashboardDTO) (*PublicDashboard, error) Update(ctx context.Context, u *user.SignedInUser, dto *SavePublicDashboardDTO) (*PublicDashboard, error) - Delete(ctx context.Context, uid string) error + Delete(ctx context.Context, uid string, dashboardUid string) error DeleteByDashboard(ctx context.Context, dashboard *dashboards.Dashboard) error GetMetricRequest(ctx context.Context, dashboard *dashboards.Dashboard, publicDashboard *PublicDashboard, panelId int64, reqDTO PublicDashboardQueryDTO) (dtos.MetricRequest, error) diff --git a/pkg/services/publicdashboards/service/service.go b/pkg/services/publicdashboards/service/service.go index a091e33931b..b67c9b2a2cc 100644 --- a/pkg/services/publicdashboards/service/service.go +++ b/pkg/services/publicdashboards/service/service.go @@ -332,7 +332,20 @@ func (pd *PublicDashboardServiceImpl) GetOrgIdByAccessToken(ctx context.Context, return pd.store.GetOrgIdByAccessToken(ctx, accessToken) } -func (pd *PublicDashboardServiceImpl) Delete(ctx context.Context, uid string) error { +func (pd *PublicDashboardServiceImpl) Delete(ctx context.Context, uid string, dashboardUid string) error { + // get existing public dashboard if exists + existingPubdash, err := pd.store.Find(ctx, uid) + if err != nil { + return ErrInternalServerError.Errorf("Delete: failed to find public dashboard by uid: %s: %w", uid, err) + } + if existingPubdash == nil { + return ErrPublicDashboardNotFound.Errorf("Delete: public dashboard not found by uid: %s", uid) + } + + // validate the public dashboard belongs to the dashboard + if existingPubdash.DashboardUid != dashboardUid { + return ErrInvalidUid.Errorf("Delete: the public dashboard does not belong to the dashboard") + } return pd.serviceWrapper.Delete(ctx, uid) } diff --git a/pkg/services/publicdashboards/service/service_test.go b/pkg/services/publicdashboards/service/service_test.go index dd2fa4fe6f4..4a85e60c020 100644 --- a/pkg/services/publicdashboards/service/service_test.go +++ b/pkg/services/publicdashboards/service/service_test.go @@ -1295,36 +1295,64 @@ func assertOldValueIfNull(t *testing.T, expectedValue bool, oldValue bool, nulla } func TestDeletePublicDashboard(t *testing.T) { - testCases := []struct { - Name string + pubdash := &PublicDashboard{Uid: "2", OrgId: 1, DashboardUid: "uid"} + + type mockFindResponse struct { + PublicDashboard *PublicDashboard + Err error + } + + type mockDeleteResponse struct { AffectedRowsResp int64 - ExpectedErrResp error StoreRespErr error + } + + testCases := []struct { + Name string + ExpectedErrResp error + mockFindStore *mockFindResponse + mockDeleteStore *mockDeleteResponse }{ { - Name: "Successfully deletes a public dashboards", - AffectedRowsResp: 1, - ExpectedErrResp: nil, - StoreRespErr: nil, + Name: "Successfully deletes a public dashboard", + ExpectedErrResp: nil, + mockFindStore: &mockFindResponse{pubdash, nil}, + mockDeleteStore: &mockDeleteResponse{1, nil}, }, { - Name: "Public dashboard not found", - AffectedRowsResp: 0, - ExpectedErrResp: nil, - StoreRespErr: nil, + Name: "Public dashboard not found", + ExpectedErrResp: ErrInternalServerError.Errorf("Delete: failed to find public dashboard by uid: pubdashUID: error"), + mockFindStore: &mockFindResponse{pubdash, errors.New("error")}, + mockDeleteStore: &mockDeleteResponse{0, nil}, }, { - Name: "Database error", - AffectedRowsResp: 0, - ExpectedErrResp: ErrInternalServerError.Errorf("Delete: failed to delete a public dashboard by Uid: uid db error!"), - StoreRespErr: errors.New("db error!"), + Name: "Public dashboard not found by UID", + ExpectedErrResp: ErrPublicDashboardNotFound.Errorf("Delete: public dashboard not found by uid: pubdashUID"), + mockFindStore: &mockFindResponse{nil, nil}, + mockDeleteStore: &mockDeleteResponse{0, nil}, + }, + { + Name: "Public dashboard UID does not belong to the dashboard", + ExpectedErrResp: ErrInvalidUid.Errorf("Delete: the public dashboard does not belong to the dashboard"), + mockFindStore: &mockFindResponse{&PublicDashboard{Uid: "2", OrgId: 1, DashboardUid: "wrong"}, nil}, + mockDeleteStore: &mockDeleteResponse{0, nil}, + }, + + { + Name: "Failed to delete - Database error", + ExpectedErrResp: ErrInternalServerError.Errorf("Delete: failed to delete a public dashboard by Uid: pubdashUID db error!"), + mockFindStore: &mockFindResponse{pubdash, nil}, + mockDeleteStore: &mockDeleteResponse{1, errors.New("db error!")}, }, } for _, tt := range testCases { t.Run(tt.Name, func(t *testing.T) { store := NewFakePublicDashboardStore(t) - store.On("Delete", mock.Anything, mock.Anything).Return(tt.AffectedRowsResp, tt.StoreRespErr) + store.On("Find", mock.Anything, mock.Anything).Return(tt.mockFindStore.PublicDashboard, tt.mockFindStore.Err) + if tt.ExpectedErrResp == nil || tt.mockDeleteStore.StoreRespErr != nil { + store.On("Delete", mock.Anything, mock.Anything).Return(tt.mockDeleteStore.AffectedRowsResp, tt.mockDeleteStore.StoreRespErr) + } serviceWrapper := &PublicDashboardServiceWrapperImpl{ log: log.New("test.logger"), store: store, @@ -1335,10 +1363,9 @@ func TestDeletePublicDashboard(t *testing.T) { serviceWrapper: serviceWrapper, } - err := service.Delete(context.Background(), "uid") + err := service.Delete(context.Background(), "pubdashUID", "uid") if tt.ExpectedErrResp != nil { assert.Equal(t, tt.ExpectedErrResp.Error(), err.Error()) - assert.Equal(t, tt.ExpectedErrResp.Error(), err.Error()) } else { assert.NoError(t, err) }