PublicDashboards: Add validation deletion (#75336)

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)
 	}

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)
 	}

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)

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)
 }
 

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)
 			}