Skip to content
Snippets Groups Projects
Commit 6818b678 authored by Jeff Evans's avatar Jeff Evans Committed by Cam Saul
Browse files

Enforce permissions on revision API 

Merge pull request from GHSA-pxfh-93j9-h745

Check read permission on the instance from get

Check write permission on the instance from revert

Rewrite expectations to clojure.test in revision_test.clj

Remove some ugliness (in the delay) and switch to fixtures instead

Add new test in revision_test.clj for preventing unauthorized reversions
parent 028e7160
Branches
Tags
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/api/revision.clj
+ 17
− 13
View file @ 6818b678
......@@ -5,37 +5,41 @@
[metabase.models.card :refer [Card]]
[metabase.models.dashboard :refer [Dashboard]]
[metabase.models.revision :as revision :refer [Revision]]
[schema.core :as s]
[toucan.db :as db]))
[schema.core :as s]))
(def ^:private ^:const name->entity
{"card" Card
"dashboard" Dashboard})
(def ^:private ^:const valid-entity-names
#{"card" "dashboard"})
(def ^:private Entity
"Schema for a valid revisionable entity name."
(apply s/enum (keys name->entity)))
(apply s/enum valid-entity-names))
(defn- model-and-instance [entity-name id]
(case entity-name
"card" [Card (Card id)]
"dashboard" [Dashboard (Dashboard id)]))
(api/defendpoint GET "/"
"Get revisions of an object."
[entity id]
{entity Entity, id s/Int}
(let [entity (name->entity entity)]
(api/check-404 (db/exists? entity :id id))
(revision/revisions+details entity id)))
(let [[model instance] (model-and-instance entity id)]
(when (api/read-check instance)
(revision/revisions+details model id))))
(api/defendpoint POST "/revert"
"Revert an object to a prior revision."
[:as {{:keys [entity id revision_id]} :body}]
{entity Entity, id s/Int, revision_id s/Int}
(let [entity (name->entity entity)
revision (api/check-404 (Revision :model (:name entity), :model_id id, :id revision_id))]
(let [[model instance] (model-and-instance entity id)
_ (api/write-check instance)
revision (api/check-404 (Revision :model (:name model), :model_id id, :id revision_id))]
;; if reverting a Card, make sure we have *data* permissions to run the query we're reverting to
(when (= entity Card)
(when (= model Card)
(card-api/check-data-permissions-for-query (get-in revision [:object :dataset_query])))
;; ok, we're g2g
(revision/revert!
:entity entity
:entity model
:id id
:user-id api/*current-user-id*
:revision-id revision_id)))
......
test/metabase/api/revision_test.clj
+ 129
− 102
View file @ 6818b678
(ns metabase.api.revision-test
(:require [expectations :refer :all]
(:require [clojure.test :refer :all]
[metabase.models.card :refer [Card serialize-instance]]
[metabase.models.collection :refer [Collection]]
[metabase.models.dashboard :refer [Dashboard]]
[metabase.models.dashboard-card :refer [DashboardCard]]
[metabase.models.revision :refer [push-revision! Revision revisions]]
[metabase.test.data :refer :all]
[metabase.test.data.users :refer :all]
[metabase.test :as mt]
[metabase.test.fixtures :as fixtures]
[metabase.test.data.users :as test-users]
[metabase.util :as u]
[toucan.db :as db]
[toucan.util.test :as tt]))
(use-fixtures :once (fixtures/initialize :db :test-users :web-server))
(def ^:private rasta-revision-info
(delay {:id (user->id :rasta), :common_name "Rasta Toucan", :first_name "Rasta", :last_name "Toucan"}))
{:id (test-users/user->id :rasta), :common_name "Rasta Toucan", :first_name "Rasta", :last_name "Toucan"})
(defn- get-revisions [entity object-id]
(for [revision ((user->client :rasta) :get 200 "revision", :entity entity, :id object-id)]
(for [revision (mt/user-http-request :rasta :get "revision" :entity entity, :id object-id)]
(dissoc revision :timestamp :id)))
(defn- create-card-revision [card is-creation?]
(defn- create-card-revision [card is-creation? user]
(push-revision!
:object card
:entity Card
:id (:id card)
:user-id (user->id :rasta)
:user-id (test-users/user->id user)
:is-creation? is-creation?))
(defn- create-dashboard-revision! [dash is-creation?]
(defn- create-dashboard-revision! [dash is-creation? user]
(push-revision!
:object (Dashboard (:id dash))
:entity Dashboard
:id (:id dash)
:user-id (user->id :rasta)
:user-id (test-users/user->id user)
:is-creation? is-creation?))
;;; # GET /revision
; Things we are testing for:
......@@ -41,58 +45,61 @@
; 3. :description is calculated
;; case with no revisions (maintains backwards compatibility with old installs before revisions)
(expect
[{:user {}, :diff nil, :description nil}]
(tt/with-temp Card [{:keys [id]}]
(get-revisions :card id)))
(deftest no-revisions-test
(testing "Loading revisions, where there are no revisions, should work"
(is (= [{:user {}, :diff nil, :description nil}]
(tt/with-temp Card [{:keys [id]}]
(get-revisions :card id))))))
;; case with single creation revision
(expect
[{:is_reversion false
:is_creation true
:message nil
:user @rasta-revision-info
:diff nil
:description nil}]
(tt/with-temp Card [{:keys [id] :as card}]
(create-card-revision card true)
(get-revisions :card id)))
(deftest single-revision-test
(testing "Loading a single revision works"
(is (= [{:is_reversion false
:is_creation true
:message nil
:user rasta-revision-info
:diff nil
:description nil}]
(tt/with-temp Card [{:keys [id] :as card}]
(create-card-revision card true :rasta)
(get-revisions :card id))))))
;; case with multiple revisions, including reversion
(tt/expect-with-temp [Card [{:keys [id name], :as card}]]
[{:is_reversion true
:is_creation false
:message "because i wanted to"
:user @rasta-revision-info
:diff {:before {:name "something else"}
:after {:name name}}
:description (format "renamed this Card from \"something else\" to \"%s\"." name)}
{:is_reversion false
:is_creation false
:message nil
:user @rasta-revision-info
:diff {:before {:name name}
:after {:name "something else"}}
:description (format "renamed this Card from \"%s\" to \"something else\"." name)}
{:is_reversion false
:is_creation true
:message nil
:user @rasta-revision-info
:diff nil
:description nil}]
(do
(create-card-revision card true)
(create-card-revision (assoc card :name "something else") false)
(db/insert! Revision
:model (:name Card)
:model_id id
:user_id (user->id :rasta)
:object (serialize-instance Card (:id card) card)
:message "because i wanted to"
:is_creation false
:is_reversion true)
(get-revisions :card id)))
(deftest multiple-revisions-with-reversion-test
(testing "Creating multiple revisions, with a reversion, works"
(tt/with-temp Card [{:keys [id name], :as card}]
(is (= [{:is_reversion true
:is_creation false
:message "because i wanted to"
:user rasta-revision-info
:diff {:before {:name "something else"}
:after {:name name}}
:description (format "renamed this Card from \"something else\" to \"%s\"." name)}
{:is_reversion false
:is_creation false
:message nil
:user rasta-revision-info
:diff {:before {:name name}
:after {:name "something else"}}
:description (format "renamed this Card from \"%s\" to \"something else\"." name)}
{:is_reversion false
:is_creation true
:message nil
:user rasta-revision-info
:diff nil
:description nil}]
(do
(create-card-revision card true :rasta)
(create-card-revision (assoc card :name "something else") false :rasta)
(db/insert! Revision
:model (:name Card)
:model_id id
:user_id (test-users/user->id :rasta)
:object (serialize-instance Card (:id card) card)
:message "because i wanted to"
:is_creation false
:is_reversion true)
(get-revisions :card id)))))))
;;; # POST /revision/revert
......@@ -100,47 +107,67 @@
[objects]
(mapv #(dissoc % :id) objects))
(tt/expect-with-temp [Dashboard [{:keys [id] :as dash}]
Card [{card-id :id, :as card}]]
[{:is_reversion true
:is_creation false
:message nil
:user @rasta-revision-info
:diff {:before {:cards nil}
:after {:cards [{:sizeX 2, :sizeY 2, :row 0, :col 0, :card_id card-id, :series []}]}}
:description "added a card."}
{:is_reversion false
:is_creation false
:message nil
:user @rasta-revision-info
:diff {:before {:cards [{:sizeX 2, :sizeY 2, :row 0, :col 0, :card_id card-id, :series []}]}
:after {:cards nil}}
:description "removed a card."}
{:is_reversion false
:is_creation false
:message nil
:user @rasta-revision-info
:diff {:before {:cards nil}
:after {:cards [{:sizeX 2, :sizeY 2, :row 0, :col 0, :card_id card-id, :series []}]}}
:description "added a card."}
{:is_reversion false
:is_creation true
:message nil
:user @rasta-revision-info
:diff nil
:description nil}]
(do
(create-dashboard-revision! dash true)
(let [dashcard (db/insert! DashboardCard :dashboard_id id :card_id (:id card))]
(create-dashboard-revision! dash false)
(db/simple-delete! DashboardCard, :id (:id dashcard)))
(create-dashboard-revision! dash false)
(let [[_ {previous-revision-id :id}] (revisions Dashboard id)]
;; Revert to the previous revision
((user->client :rasta) :post 200 "revision/revert", {:entity :dashboard, :id id, :revision_id previous-revision-id}))
(->> (get-revisions :dashboard id)
(mapv (fn [rev]
(if-not (:diff rev) rev
(if (get-in rev [:diff :before :cards])
(update-in rev [:diff :before :cards] strip-ids)
(update-in rev [:diff :after :cards] strip-ids))))))))
(deftest revert-test
(testing "Reverting through API works"
(tt/with-temp* [Dashboard [{:keys [id] :as dash}]
Card [{card-id :id, :as card}]]
(is (= [{:is_reversion true
:is_creation false
:message nil
:user rasta-revision-info
:diff {:before {:cards nil}
:after {:cards [{:sizeX 2, :sizeY 2, :row 0, :col 0, :card_id card-id, :series []}]}}
:description "added a card."}
{:is_reversion false
:is_creation false
:message nil
:user rasta-revision-info
:diff {:before {:cards [{:sizeX 2, :sizeY 2, :row 0, :col 0, :card_id card-id, :series []}]}
:after {:cards nil}}
:description "removed a card."}
{:is_reversion false
:is_creation false
:message nil
:user rasta-revision-info
:diff {:before {:cards nil}
:after {:cards [{:sizeX 2, :sizeY 2, :row 0, :col 0, :card_id card-id, :series []}]}}
:description "added a card."}
{:is_reversion false
:is_creation true
:message nil
:user rasta-revision-info
:diff nil
:description nil}]
(do
(create-dashboard-revision! dash true :rasta)
(let [dashcard (db/insert! DashboardCard :dashboard_id id :card_id (:id card))]
(create-dashboard-revision! dash false :rasta)
(db/simple-delete! DashboardCard, :id (:id dashcard)))
(create-dashboard-revision! dash false :rasta)
(let [[_ {previous-revision-id :id}] (revisions Dashboard id)]
;; Revert to the previous revision, allowed because rasta has permissions on parent collection
(mt/user-http-request :rasta :post "revision/revert" {:entity :dashboard
:id id
:revision_id previous-revision-id}))
(->> (get-revisions :dashboard id)
(mapv (fn [rev]
(if-not (:diff rev) rev
(if (get-in rev [:diff :before :cards])
(update-in rev [:diff :before :cards] strip-ids)
(update-in rev [:diff :after :cards] strip-ids))))))))))))
(deftest permission-check-on-revert-test
(testing "Are permissions enforced by the revert action in the revision api?"
(mt/with-non-admin-groups-no-root-collection-perms
(mt/with-temp* [Collection [collection {:name "Personal collection"}]
Dashboard [dashboard {:collection_id (u/the-id collection) :name "Personal dashboard"}]]
(create-dashboard-revision! dashboard true :crowberto)
(create-dashboard-revision! dashboard false :crowberto)
(let [dashboard-id (u/the-id dashboard)
[_ {prev-rev-id :id}] (revisions Dashboard dashboard-id)
update-req {:entity :dashboard, :id dashboard-id, :revision_id prev-rev-id}]
;; rasta should not have permissions to update the dashboard (i.e. revert), because they are not admin and do
;; not have any particular permission on the collection where it lives (because of the
;; with-non-admin-groups-no-root-collection-perms wrapper)
(is (= "You don't have permissions to do that."
(mt/user-http-request :rasta :post "revision/revert" update-req))))))))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment