From 59dbbb41f8c6e3ef91be8915901c015a0d24d33b Mon Sep 17 00:00:00 2001
From: idafurjes <36131195+idafurjes@users.noreply.github.com>
Date: Thu, 7 Jul 2022 15:45:32 +0200
Subject: [PATCH] Do not check permissions while deleting external snapshot
 (#51897) (#51904)

(cherry picked from commit ee88b44458f525914a39aacb24a08acac513184d)
---
 pkg/api/dashboard_snapshot.go      | 30 +++++++++++++++++-------------
 pkg/api/dashboard_snapshot_test.go |  8 ++------
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/pkg/api/dashboard_snapshot.go b/pkg/api/dashboard_snapshot.go
index c2ebc7ef2a2..53c3cebed62 100644
--- a/pkg/api/dashboard_snapshot.go
+++ b/pkg/api/dashboard_snapshot.go
@@ -266,24 +266,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, models.ErrDashboardNotFound) {
-		return response.Error(500, "Error while checking permissions for snapshot", err)
-	}
-
-	if !canEdit && query.Result.UserId != c.SignedInUser.UserId && !errors.Is(err, models.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, models.ErrDashboardNotFound) {
+			return response.Error(500, "Error while checking permissions for snapshot", err)
+		}
+
+		if !canEdit && query.Result.UserId != c.SignedInUser.UserId && !errors.Is(err, models.ErrDashboardNotFound) {
+			return response.Error(403, "Access denied to this snapshot", nil)
+		}
 	}
 
 	cmd := &models.DeleteDashboardSnapshotCommand{DeleteKey: query.Result.DeleteKey}
diff --git a/pkg/api/dashboard_snapshot_test.go b/pkg/api/dashboard_snapshot_test.go
index d34d285c46d..cdab0462bb5 100644
--- a/pkg/api/dashboard_snapshot_test.go
+++ b/pkg/api/dashboard_snapshot_test.go
@@ -56,18 +56,14 @@ 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
 				mockSnapshotResult := setUpSnapshotTest(t)
-				ts := setupRemoteServer(func(rw http.ResponseWriter, req *http.Request) {
-					externalRequest = req
-				})
-				mockSnapshotResult.ExternalDeleteUrl = ts.URL
+				mockSnapshotResult.ExternalDeleteUrl = ""
+				mockSnapshotResult.External = false
 				sc.handlerFunc = hs.DeleteDashboardSnapshot
 				guardian.InitLegacyGuardian(sc.sqlStore)
 				sc.fakeReqWithParams("DELETE", sc.url, map[string]string{"key": "12345"}).exec()
 
 				assert.Equal(t, 403, sc.resp.Code)
-				require.Nil(t, externalRequest)
 			}, sqlmock)
 	})