diff --git a/pkg/api/dashboard_snapshot.go b/pkg/api/dashboard_snapshot.go index 05e57abbdec..2c2ae051586 100644 --- a/pkg/api/dashboard_snapshot.go +++ b/pkg/api/dashboard_snapshot.go @@ -268,24 +268,28 @@ func (hs *HTTPServer) DeleteDashboardSnapshot(c *models.ReqContext) response.Res return response.Error(404, "Failed to get dashboard snapshot", nil) } - dashboardID := query.Result.Dashboard.Get("id").MustInt64() - - guardian := guardian.New(c.Req.Context(), dashboardID, c.OrgId, c.SignedInUser) - canEdit, err := guardian.CanEdit() - // check for permissions only if the dahboard is found - if err != nil && !errors.Is(err, dashboards.ErrDashboardNotFound) { - return response.Error(500, "Error while checking permissions for snapshot", err) - } - - if !canEdit && query.Result.UserId != c.SignedInUser.UserId && !errors.Is(err, dashboards.ErrDashboardNotFound) { - return response.Error(403, "Access denied to this snapshot", nil) - } - if query.Result.External { err := deleteExternalDashboardSnapshot(query.Result.ExternalDeleteUrl) if err != nil { return response.Error(500, "Failed to delete external dashboard", err) } + } else { + // When creating an external snapshot, its dashboard content is empty. This means that the mustInt here returns a 0, + // which before RBAC would result in a dashboard which has no ACL. A dashboard without an ACL would fallback + // to the user’s org role, which for editors and admins would essentially always be allowed here. With RBAC, + // all permissions must be explicit, so the lack of a rule for dashboard 0 means the guardian will reject. + dashboardID := query.Result.Dashboard.Get("id").MustInt64() + + guardian := guardian.New(c.Req.Context(), dashboardID, c.OrgId, c.SignedInUser) + canEdit, err := guardian.CanEdit() + // check for permissions only if the dahboard is found + if err != nil && !errors.Is(err, dashboards.ErrDashboardNotFound) { + return response.Error(500, "Error while checking permissions for snapshot", err) + } + + if !canEdit && query.Result.UserId != c.SignedInUser.UserId && !errors.Is(err, dashboards.ErrDashboardNotFound) { + return response.Error(403, "Access denied to this snapshot", nil) + } } cmd := &dashboardsnapshots.DeleteDashboardSnapshotCommand{DeleteKey: query.Result.DeleteKey} diff --git a/pkg/api/dashboard_snapshot_test.go b/pkg/api/dashboard_snapshot_test.go index 5c09202cb09..43251646977 100644 --- a/pkg/api/dashboard_snapshot_test.go +++ b/pkg/api/dashboard_snapshot_test.go @@ -65,11 +65,7 @@ func TestDashboardSnapshotAPIEndpoint_singleSnapshot(t *testing.T) { t.Run("When user has editor role and is not in the ACL", func(t *testing.T) { loggedInUserScenarioWithRole(t, "Should not be able to delete snapshot when calling DELETE on", "DELETE", "/api/snapshots/12345", "/api/snapshots/:key", models.ROLE_EDITOR, func(sc *scenarioContext) { - var externalRequest *http.Request - ts := setupRemoteServer(func(rw http.ResponseWriter, req *http.Request) { - externalRequest = req - }) - hs := &HTTPServer{dashboardsnapshotsService: setUpSnapshotTest(t, 0, ts.URL)} + hs := &HTTPServer{dashboardsnapshotsService: setUpSnapshotTest(t, 0, "")} sc.handlerFunc = hs.DeleteDashboardSnapshot dashSvc := dashboards.NewFakeDashboardService(t) @@ -79,7 +75,6 @@ func TestDashboardSnapshotAPIEndpoint_singleSnapshot(t *testing.T) { sc.fakeReqWithParams("DELETE", sc.url, map[string]string{"key": "12345"}).exec() assert.Equal(t, 403, sc.resp.Code) - require.Nil(t, externalRequest) }, sqlmock) })