From ca4968682f6f0eaeea98cf78d381a3cef46b3b2e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cam=20Sau=CC=88l?= <cammsaul@gmail.com>
Date: Tue, 7 Jul 2015 05:23:53 -0700
Subject: [PATCH] can_read and can_write are now hydrated via methods
---
src/metabase/models/card.clj | 17 +++++---
src/metabase/models/common.clj | 46 ---------------------
src/metabase/models/dashboard.clj | 12 +++++-
src/metabase/models/database.clj | 3 +-
src/metabase/models/hydrate.clj | 13 +++++-
src/metabase/models/interface.clj | 61 +++++++++++++++++++++-------
test/metabase/models/common_test.clj | 26 ------------
7 files changed, 81 insertions(+), 97 deletions(-)
delete mode 100644 test/metabase/models/common_test.clj
diff --git a/src/metabase/models/card.clj b/src/metabase/models/card.clj
index 86e91a60aa3..9c2ebd5cdea 100644
--- a/src/metabase/models/card.clj
+++ b/src/metabase/models/card.clj
@@ -2,8 +2,7 @@
(:require [korma.core :refer :all, :exclude [defentity]]
[metabase.api.common :refer [*current-user-id*]]
[metabase.db :refer :all]
- (metabase.models [common :refer :all]
- [interface :refer :all]
+ (metabase.models [interface :refer :all]
[user :refer [User]])))
(def ^:const display-types
@@ -19,6 +18,14 @@
:table
:timeseries})
+(defrecord CardInstance []
+ clojure.lang.IFn
+ (invoke [this k]
+ (get this k)))
+
+(extend-ICanReadWrite CardInstance :read :public-perms, :write :public-perms)
+
+
(defentity Card
[(table :report_card)
(hydration-keys card)
@@ -26,10 +33,10 @@
timestamped]
(post-select [_ {:keys [creator_id] :as card}]
- (-> (assoc card
- :creator (delay (sel :one User :id creator_id)))
- assoc-permissions-sets))
+ (map->CardInstance (assoc card :creator (delay (sel :one User :id creator_id)))))
(pre-cascade-delete [_ {:keys [id]}]
(cascade-delete 'metabase.models.dashboard-card/DashboardCard :card_id id)
(cascade-delete 'metabase.models.card-favorite/CardFavorite :card_id id)))
+
+(extend-ICanReadWrite CardEntity :read :public-perms, :write :public-perms)
diff --git a/src/metabase/models/common.clj b/src/metabase/models/common.clj
index adb7a0a0980..b9fcc6005ab 100644
--- a/src/metabase/models/common.clj
+++ b/src/metabase/models/common.clj
@@ -15,8 +15,6 @@
"US/Pacific"
"America/Costa_Rica"])
-;;; ALLEN'S PERMISSIONS IMPLEMENTATION
-
(def ^:const perms-none 0)
(def ^:const perms-read 1)
(def ^:const perms-readwrite 2)
@@ -27,50 +25,6 @@
{:id perms-readwrite :name "Read & Write"}])
-;;; CAM'S PERMISSIONS IMPL
-;; (TODO - need to use one or the other)
-
-(defn public-permissions
- "Return the set of public permissions for some object with key `:public_perms`. Possible permissions are `:read` and `:write`."
- [{:keys [public_perms]}]
- (check public_perms 500 "Can't check public permissions: object doesn't have :public_perms.")
- ({0 #{}
- 1 #{:read}
- 2 #{:read :write}} public_perms))
-
-(defn user-permissions
- "Return the set of current user's permissions for some object with keys `:creator_id` and `:public_perms`."
- [{:keys [creator_id public_perms] :as obj}]
- (check creator_id 500 "Can't check user permissions: object doesn't have :creator_id."
- public_perms 500 "Can't check user permissions: object doesn't have :public_perms.")
- (cond (:is_superuser *current-user*) #{:read :write} ; superusers have full access to everything
- (= creator_id *current-user-id*) #{:read :write} ; if user created OBJ they have all permissions
- (<= perms-read public_perms) #{:read} ; if the object is public then everyone gets :read
- :else #{})) ; default is user has no permissions a.k.a private
-
-(defn user-can?
- "Check if *current-user* has a given PERMISSION for OBJ.
- PERMISSION should be either `:read` or `:write`."
- [permission obj]
- (contains? @(:user-permissions-set obj) permission))
-
-(defn assoc-permissions-sets
- "Associates the following delays with OBJ:
-
- * `:public-permissions-set`
- * `:user-permissions-set`
- * `:can_read`
- * `:can_write`
-
- Note that these delays depend upon the presence of `creator_id`, and `public_perms` fields in OBJ."
- [obj]
- (u/assoc* obj
- :public-permissions-set (delay (public-permissions <>))
- :user-permissions-set (delay (user-permissions <>))
- :can_read (delay (user-can? :read <>))
- :can_write (delay (user-can? :write <>))))
-
-
(defn name->human-readable-name
"Convert a string NAME of some object like a `Table` or `Field` to one more friendly to humans.
diff --git a/src/metabase/models/dashboard.clj b/src/metabase/models/dashboard.clj
index caa588809ba..6c0ae81d38d 100644
--- a/src/metabase/models/dashboard.clj
+++ b/src/metabase/models/dashboard.clj
@@ -7,6 +7,14 @@
[user :refer [User]])
[metabase.util :as u]))
+(defrecord DashboardInstance []
+ clojure.lang.IFn
+ (invoke [this k]
+ (get this k)))
+
+(extend-ICanReadWrite DashboardInstance :read :public-perms, :write :public-perms)
+
+
(defentity Dashboard
[(table :report_dashboard)
timestamped]
@@ -16,7 +24,9 @@
(assoc :creator (delay (sel :one User :id creator_id))
:description (u/jdbc-clob->str description)
:ordered_cards (delay (sel :many DashboardCard :dashboard_id id (order :created_at :asc))))
- assoc-permissions-sets))
+ map->DashboardInstance))
(pre-cascade-delete [_ {:keys [id]}]
(cascade-delete DashboardCard :dashboard_id id)))
+
+(extend-ICanReadWrite DashboardEntity :read :public-perms, :write :public-perms)
diff --git a/src/metabase/models/database.clj b/src/metabase/models/database.clj
index 338dbaf9f3d..1683ae7ed60 100644
--- a/src/metabase/models/database.clj
+++ b/src/metabase/models/database.clj
@@ -2,8 +2,7 @@
(:require [korma.core :refer :all, :exclude [defentity]]
[metabase.api.common :refer [*current-user*]]
[metabase.db :refer :all]
- (metabase.models [common :refer [assoc-permissions-sets]]
- [interface :refer :all])))
+ [metabase.models.interface :refer :all]))
(defrecord DatabaseInstance []
;; preserve normal IFn behavior so things like ((sel :one Database) :id) work correctly
diff --git a/src/metabase/models/hydrate.clj b/src/metabase/models/hydrate.clj
index 5a9037aedfd..0234b7e094f 100644
--- a/src/metabase/models/hydrate.clj
+++ b/src/metabase/models/hydrate.clj
@@ -1,6 +1,7 @@
(ns metabase.models.hydrate
"Functions for deserializing and hydrating fields in objects fetched from the DB."
(:require [metabase.db :refer [sel]]
+ [metabase.models.interface :as models]
[metabase.util :as u]))
(declare batched-hydrate
@@ -131,14 +132,22 @@
(if (can-batched-hydrate? results k) (batched-hydrate results k)
(simple-hydrate results k)))
+(def ^:private hydration-k->method
+ "Methods that can be used to hydrate corresponding keys."
+ {:can_read #(models/can-read? %)
+ :can_write #(models/can-write? %)})
+
(defn- simple-hydrate
"Hydrate keyword K in results by dereferencing corresponding delays when applicable."
[results k]
{:pre [(keyword? k)]}
(map (fn [result]
(let [v (k result)]
- (if-not (delay? v) result ; if v isn't a delay it's either already hydrated or nil.
- (assoc result k @v)))) ; don't barf on nil; just no-op
+ (cond
+ (delay? v) (assoc result k @v) ; hydrate delay if possible
+ (and (not v)
+ (hydration-k->method k)) (assoc result k ((hydration-k->method k) result)) ; otherwise if no value exists look for a method we can use for hydration
+ :else result))) ; otherwise don't barf, v may already be hydrated
results))
(defn- already-hydrated?
diff --git a/src/metabase/models/interface.clj b/src/metabase/models/interface.clj
index 7cb1af3c617..996ce6b3339 100644
--- a/src/metabase/models/interface.clj
+++ b/src/metabase/models/interface.clj
@@ -8,20 +8,50 @@
metabase.db.internal
[metabase.util :as u]))
+;;; ## ---------------------------------------- PERMISSIONS CHECKING ----------------------------------------
+
(defprotocol ICanReadWrite
(can-read? [obj] [entity ^Integer id])
(can-write? [obj] [entity ^Integer id]))
+(defn- superuser? [& _]
+ (:is_superuser @@(resolve 'metabase.api.common/*current-user*)))
+
+(defn- owner?
+ ([{:keys [creator_id], :as obj}]
+ (assert creator_id "Can't check user permissions: object doesn't have :creator_id.")
+ (or (superuser?)
+ (= creator_id @(resolve 'metabase.api.common/*current-user-id*))))
+ ([entity id]
+ (or (superuser?)
+ (owner? (entity id)))))
+
+(defn- has-public-perms? [rw]
+ (let [perms (delay (require 'metabase.models.common)
+ (case rw
+ :r @(resolve 'metabase.models.common/perms-read)
+ :w @(resolve 'metabase.models.common/perms-readwrite)))]
+ (fn -has-public-perms?
+ ([{:keys [public_perms], :as obj}]
+ (assert public_perms "Can't check user permissions: object doesn't have :public_perms.")
+ (or (owner? obj)
+ (>= public_perms @perms)))
+ ([entity id]
+ (or (owner? entity id)
+ (-has-public-perms? (entity id)))))))
+
(defn extend-ICanReadWrite
"Add standard implementations of `can-read?` and `can-write?` to KLASS."
[klass & {:keys [read write]}]
- (let [key->method #(case %
- :always (constantly true)
- :superuser (fn [& args]
- (:is_superuser @@(resolve 'metabase.api.common/*current-user*))))]
+ (let [key->method (fn [rw v]
+ (case v
+ :always (constantly true)
+ :public-perms (has-public-perms? rw)
+ :owner #'owner?
+ :superuser #'superuser?))]
(extend klass
- ICanReadWrite {:can-read? (key->method read)
- :can-write? (key->method write)})))
+ ICanReadWrite {:can-read? (key->method :r read)
+ :can-write? (key->method :w write)})))
;;; ## ---------------------------------------- ENTITIES ----------------------------------------
@@ -102,15 +132,16 @@
(defn -invoke-entity
"Basically the same as `(sel :one Entity :id id)`." ; TODO - deduplicate with sel
[entity id]
- (when (metabase.config/config-bool :mb-db-logging)
- (clojure.tools.logging/debug
- "DB CALL: " (:name entity) id))
- (let [[obj] (k/select (assoc entity :fields (::default-fields entity))
- (k/where {:id id})
- (k/limit 1))]
- (some->> obj
- (internal-post-select entity)
- (post-select entity))))
+ (when id
+ (when (metabase.config/config-bool :mb-db-logging)
+ (clojure.tools.logging/debug
+ "DB CALL: " (:name entity) id))
+ (let [[obj] (k/select (assoc entity :fields (::default-fields entity))
+ (k/where {:id id})
+ (k/limit 1))]
+ (some->> obj
+ (internal-post-select entity)
+ (post-select entity)))))
(defn- update-updated-at [obj]
(assoc obj :updated_at (u/new-sql-timestamp)))
diff --git a/test/metabase/models/common_test.clj b/test/metabase/models/common_test.clj
deleted file mode 100644
index 2ac473653d0..00000000000
--- a/test/metabase/models/common_test.clj
+++ /dev/null
@@ -1,26 +0,0 @@
-(ns metabase.models.common-test
- (:require [expectations :refer :all]
- [metabase.api.common :refer [*current-user-id*]]
- [metabase.models.common :refer :all]))
-
-;;; tests for PUBLIC-PERMISSIONS
-
-(expect #{}
- (public-permissions {:public_perms 0}))
-
-(expect #{:read}
- (public-permissions {:public_perms 1}))
-
-(expect #{:read :write}
- (public-permissions {:public_perms 2}))
-
-
-;;; tests for USER-PERMISSIONS
-
-;; creator can read + write
-(expect #{:read :write}
- (binding [*current-user-id* 100]
- (user-permissions {:creator_id 100
- :public_perms 0})))
-
-;; TODO - write tests for the rest of the `user-permissions`
--
GitLab