Merge pull request #72 from metabase/better_perms_checking

Better perms checking

src/metabase/api/card.clj

@@ -30,19 +30,18 @@
 
 (defendpoint GET "/:id" [id]
   (->404 (sel :one Card :id id)
+         read-check
          (hydrate :can_read :can_write :organization)))
 
 (defendpoint PUT "/:id" [id :as {:keys [body]}]
-  (let-404 [{:keys [can_write] :as card} (sel :one Card :id id)]
-    (check-403 @can_write)
-    (check-500 (->> (util/select-non-nil-keys body :dataset_query :description :display :name :public_perms :visualization_settings)
-                    (mapply upd Card id)))
-    (sel :one Card :id id)))
+  (write-check Card id)
+  (check-500 (->> (util/select-non-nil-keys body :dataset_query :description :display :name :public_perms :visualization_settings)
+                  (mapply upd Card id)))
+  (sel :one Card :id id))
 
 (defendpoint DELETE "/:id" [id]
-  (let-404 [{:keys [can_write]} (sel :one Card :id id)]
-    (check-403 @can_write)
-    (del Card :id id)))
+  (write-check Card id)
+  (del Card :id id))
 
 (defendpoint GET "/:id/favorite" [id]
   {:favorite (boolean (some->> *current-user-id*

src/metabase/api/common.clj

@@ -181,3 +181,36 @@
                         (filter-vals #(:is-endpoint? (meta %)))
                         (map first))]
     `(defroutes ~'routes ~@api-routes ~@additional-routes)))
+
+
+;; ## NEW PERMISSIONS CHECKING MACROS
+;; Since checking `@can_read` `@can_write` is such a common pattern, these
+;; macros eliminate a bit of the redundancy around doing so.
+;; They support two forms:
+;;
+;;    (read-check my-table) ; checks @(:can_read my-table)
+;;    (read-check Table 1)  ; checks @(:can_read (sel :one Table :id 1))
+;;
+;; *  The first form is useful when you've already fetched an object (especially in threading forms such as `->404`).
+;; *  The second form takes care of fetching the object for you and is useful in cases where you won't need the object afterward
+;;    or want to combine the `sel` and permissions check statements into a single form.
+;;
+;; Both forms will throw a 404 if the object doesn't exist (saving you one more check!) and return the selected object.
+
+(defmacro read-check
+  "Checks that @can_read is true for this object."
+  ([obj]
+   `(let-404 [{:keys [~'can_read] :as obj#} ~obj]
+      (check-403 @~'can_read)
+      obj#))
+  ([entity id]
+   `(read-check (sel :one ~entity :id ~id))))
+
+(defmacro write-check
+  "Checks that @can_write is true for this object."
+  ([obj]
+   `(let-404 [{:keys [~'can_write] :as obj#} ~obj]
+      (check-403 @~'can_write)
+      obj#))
+  ([entity id]
+   `(write-check (sel :one ~entity :id ~id))))

src/metabase/api/dash.clj

@@ -33,29 +33,24 @@
     {:dashboard db})) ; why is this returned with this {:dashboard} wrapper?
 
 (defendpoint PUT "/:id" [id :as {{:keys [description name public_perms]} :body}]
-  (let-404 [{:keys [can_write]} (sel :one Dashboard :id id)]
-    (check-403 @can_write))
+  (write-check Dashboard id)
   (upd Dashboard id :description description :name name :public_perms public_perms))
 
 (defendpoint DELETE "/:id" [id]
-  (let-404 [{:keys [can_write]} (sel :one Dashboard :id id)]
-    (check-403 @can_write))
+  (write-check Dashboard id)
   (del Dashboard :id id))
 
 (defendpoint POST "/:id/cards" [id :as {{:keys [cardId]} :body}]
-  (let-404 [{:keys [can_write]} (sel :one Dashboard :id id)]
-    (check-403 @can_write))
-  (check (exists? Card :id cardId) 400 (format "Card %d doesn't exist." cardId))
+  (write-check Dashboard id)
+  (check-400 (exists? Card :id cardId))
   (ins DashboardCard :card_id cardId :dashboard_id id))
 
 (defendpoint DELETE "/:id/cards" [id dashcardId]
-  (let-404 [{:keys [can_write]} (sel :one Dashboard :id id)]
-    (check-403 @can_write))
+  (write-check Dashboard id)
   (del DashboardCard :id dashcardId :dashboard_id id))
 
 (defendpoint POST "/:id/reposition" [id :as {{:keys [cards]} :body}]
-  (let-404 [{:keys [can_write]} (sel :one Dashboard :id id)]
-    (check-403 @can_write))
+  (write-check Dashboard id)
   (dorun (map (fn [{:keys [card_id sizeX sizeY row col]}]
                 (let [{dashcard-id :id} (sel :one [DashboardCard :id] :card_id card_id :dashboard_id id)]
                   (upd DashboardCard dashcard-id :sizeX sizeX :sizeY sizeY :row row :col col)))

src/metabase/api/emailreport.clj

@@ -56,16 +56,15 @@
 
 
 (defendpoint GET "/:id" [id]
-  ; user must have READ permissions on the report
-  (let-404 [{:keys [can_read] :as report} (sel :one EmailReport :id id)]
-    (check-403 @can_read)
-    (hydrate report :creator :organization :can_read :can_write)))
+  (->404 (sel :one EmailReport :id id)
+         read-check
+         (hydrate :creator :organization :can_read :can_write)))
 
 
 (defendpoint PUT "/:id" [id :as {body :body}]
   ; user must have WRITE permissions on the report
-  (let-404 [{:keys [can_write] :as report} (sel :one EmailReport :id id)]
-    (check-403 @can_write)
+  (let-404 [report (sel :one EmailReport :id id)]
+    (write-check report)
     ;; TODO - validate that for public_perms, mode, etc are within their expected set of possible values
     ;; TODO - deal with recipients
     (check-500 (->> (-> (merge report (util/select-non-nil-keys body :name :description :public_perms :mode :dataset_query :email_addresses :schedule))
@@ -77,9 +76,8 @@
 
 
 (defendpoint DELETE "/:id" [id]
-  (let-404 [{:keys [can_write] :as report} (sel :one EmailReport :id id)]
-    (check-403 @can_write)
-    (del EmailReport :id id)))
+  (write-check EmailReport id)
+  (del EmailReport :id id))
 
 
 (defendpoint POST "/:id" [id]
@@ -89,10 +87,9 @@
 
 (defendpoint GET "/:id/executions" [id]
   ;; TODO - implementation (list recent results of a query)
-  (let-404 [{:keys [can_read] :as report} (sel :one EmailReport :id id)]
-    (check-403 @can_read)
-    (-> (sel :many EmailReportExecutions :report_id id (order :created_at :DESC) (limit 25))
-      (hydrate :organization))))
+  (read-check EmailReport id)
+  (-> (sel :many EmailReportExecutions :report_id id (order :created_at :DESC) (limit 25))
+      (hydrate :organization)))
 
 
 (define-routes)

src/metabase/api/meta/db.clj

@@ -35,13 +35,11 @@
          (hydrate :organization)))
 
 (defendpoint DELETE "/:id" [id]
-  (let-404 [{:keys [can_write]} (sel :one Database :id id)]
-    (check-403 @can_write))
+  (write-check Database id)
   (del Database :id id))
 
 (defendpoint GET "/:id/autocomplete_suggestions" [id prefix]
-  (let-404 [{:keys [can_read]} (sel :one Database :id id)]
-    (check-403 @can_read))
+  (read-check Database id)
   (let [prefix-len (count prefix)
         table-id->name (->> (sel :many [Table :id :name] :db_id id)                                             ; fetch all name + ID of all Tables for this DB
                             (map (fn [{:keys [id name]}]                                                         ; make a map of Table ID -> Table Name

src/metabase/api/meta/field.clj

@@ -6,18 +6,17 @@
                              [field :refer [Field]])))
 
 (defendpoint GET "/:id" [id]
-  (let-404 [{:keys [can_read] :as field} (sel :one Field :id id)]
-    (check-403 @can_read)
-    (hydrate field [:table :db])))
+  (->404 (sel :one Field :id id)
+         read-check
+         (hydrate [:table :db])))
 
 (defendpoint PUT "/:id" [id :as {{:keys [special_type preview_display description]} :body}]
-  (let-404 [{:keys [can_write]} (sel :one Field :id id)]
-    (check-403 @can_write))
+  (write-check Field id)
   (upd Field id :special_type special_type :preview_display preview_display :description description))
 
 (defendpoint GET "/:id/summary" [id]
-  (let-404 [{:keys [can_read count distinct-count]} (sel :one Field :id id)]
-    (check-403 @can_read)
+  (let-404 [{:keys [count distinct-count] :as field} (sel :one Field :id id)]
+    (read-check field)
     [[:count @count]
      [:distincts @distinct-count]]))
 

src/metabase/api/org.clj

@@ -20,26 +20,19 @@
   {:status 200
    :body {}})
 
-
 (defendpoint GET "/:id" [id]
-  (let-404 [{:keys [can_read] :as org} (sel :one Org :id id)]
-    (check-403 @can_read)
-    org))
-
+  (->404 (sel :one Org :id id)
+         read-check))
 
 (defendpoint GET "/slug/:slug" [slug]
-  (let-404 [{:keys [can_read] :as org} (sel :one Org :slug slug)]
-    (check-403 @can_read)
-    org))
-
+  (->404 (sel :one Org :slug slug)
+         read-check))
 
 (defendpoint PUT "/:id" [id :as {body :body}]
-  (let-404 [{:keys [can_write] :as org} (sel :one Org :id id)]
-    (check-403 @can_write)
-    (check-500 (->> (util/select-non-nil-keys body :name :description :logo_url)
-                 (mapply upd Org id)))
-    (sel :one Org :id id)))
-
+  (write-check Org id)
+  (check-500 (->> (util/select-non-nil-keys body :name :description :logo_url)
+                  (mapply upd Org id)))
+  (sel :one Org :id id))
 
 (defn grant-org-perm
   "Grants permission for given User on Org.  Creates record if needed, otherwise updates existing record."
@@ -54,48 +47,37 @@
       (upd OrgPerm (:id perm)
         :admin is-admin))))
 
-
 (defendpoint GET "/:id/members" [id]
-  (let-404 [{:keys [can_read] :as org} (sel :one Org :id id)]
-    (check-403 @can_read)
-    (-> (sel :many OrgPerm :organization_id id)
-        (hydrate :user :organization))))
-
+  (read-check Org id)
+  (-> (sel :many OrgPerm :organization_id id)
+      (hydrate :user :organization)))
 
 (defendpoint POST "/:id/members" [id :as {{:keys [first_name last_name email admin]} :body}]
   ; we require 4 attributes in the body
   (check-400 (and first_name last_name email admin))
   ; user must have admin perms on org to proceed
-  (let-404 [{:keys [can_write] :as org} (sel :one Org :id id)]
-    (check-403 @can_write)
-    (let [user-id (:id (or (sel :one [User :id] :email email)                        ; find user with existing email - if exists then grant perm
-                           (ins User
-                             :email email
-                             :first_name first_name
-                             :last_name last_name
-                             :password (str (java.util.UUID/randomUUID)))))]         ; TODO - send welcome email
-      (grant-org-perm id user-id admin)
-      (-> (sel :one OrgPerm :user_id user-id :organization_id id)
-          (hydrate :user :organization)))))
-
-
-(defendpoint POST "/:id/members/:user-id" [id user-id :as {body :body}]
-  ; user must have admin perms on org to proceed
-  (let-404 [{:keys [can_write] :as org} (sel :one Org :id id)]
-    (check-403 @can_write)
-    (let-404 [user (sel :one User :id user-id)]
-      (grant-org-perm id user-id (or (:admin body) false))
-      {:success true})))
-
-
-(defendpoint PUT "/:id/members/:user-id" [id user-id :as {body :body}]
-  ; user must have admin perms on org to proceed
-  (let-404 [{:keys [can_write] :as org} (sel :one Org :id id)]
-    (check-403 @can_write)
-    (let-404 [user (sel :one User :id user-id)]
-      (grant-org-perm id user-id (or (:admin body) false))
-      {:success true})))
+  (write-check Org id)
+  (let [user-id (:id (or (sel :one [User :id] :email email)                ; find user with existing email - if exists then grant perm
+                         (ins User
+                           :email email
+                           :first_name first_name
+                           :last_name last_name
+                           :password (str (java.util.UUID/randomUUID)))))] ; TODO - send welcome email
+    (grant-org-perm id user-id admin)
+    (-> (sel :one OrgPerm :user_id user-id :organization_id id)
+        (hydrate :user :organization))))
 
+(defendpoint POST "/:id/members/:user-id" [id user-id :as {{:keys [admin]} :body}]
+  (write-check Org id)
+  (check-404 (exists? User :id user-id))
+  (grant-org-perm id user-id (boolean admin))
+  {:success true})
+
+(defendpoint PUT "/:id/members/:user-id" [id user-id :as {{:keys [admin]} :body}]
+  (write-check Org id)
+  (check-404 (exists? User :id user-id))
+  (grant-org-perm id user-id (boolean admin))
+  {:success true})
 
 (defendpoint DELETE "/:id/members/:user-id" [id user-id :as {body :body}]
   ; user must have admin perms on org to proceed

src/metabase/api/query.clj

@@ -34,8 +34,8 @@
 (defn query-clone
   "Create a new query by cloning an existing query.  Returns a 403 if user doesn't have acces to read query."
   [query-id]
-  (let-400 [{:keys [can_read name] :as query} (sel :one Query :id query-id)]
-    (check-403 @can_read)
+  (let-400 [{:keys [name] :as query} (sel :one Query :id query-id)]
+    (read-check query)
     (->> (-> query
              (select-keys [:type :details :database_id])
              (assoc :name (str name " CLONED")
@@ -63,27 +63,25 @@
     (query-clone clone)
     (query-create body)))
 
-
 (defendpoint GET "/:id" [id]
-  (let-404 [{:keys [can_read] :as query} (sel :one Query :id id)]
-    (check-403 @can_read)
-    (hydrate query :creator :database :can_read :can_write)))
-
+  (->404 (sel :one Query :id id)
+         read-check
+         (hydrate :creator :database :can_read :can_write)))
 
 (defendpoint PUT "/:id" [id :as {{:keys [sql timezone version] :as body} :body}]
   ;; TODO - check that database exists and user has permission (if specified)
-  (let-404 [{:keys [can_write] :as query} (sel :one Query :id id)]
-    (check-403 @can_write)
-    (check-500 (-> (merge query body)
-                   (#(mapply upd Query id %))))
+  (let-404 [query ]
+    (check-500 (->404 (sel :one Query :id id)
+                      write-check
+                      (merge body)
+                      (#(mapply upd Query id %))))
     (-> (sel :one Query :id id)
         (hydrate :creator :database))))
 
 
 (defendpoint DELETE "/:id" [id]
-  (let-404 [{:keys [can_write] :as query} (sel :one [Query :id :creator_id :public_perms] :id id)]
-    (check-403 @can_write)
-    (del Query :id id)))
+  (write-check Query id)
+  (del Query :id id))
 
 
 (defendpoint POST "/:id" [id]
@@ -93,9 +91,8 @@
 
 (defendpoint GET "/:id/results" [id]
   ;; TODO - implementation (list recent results of a query)
-  (let-404 [{:keys [can_read] :as query} (sel :one Query :id id)]
-    (check-403 @can_read)
-    (sel :many QueryExecution :query_id id (order :finished_at :DESC) (limit 10))))
+  (read-check Query id)
+  (sel :many QueryExecution :query_id id (order :finished_at :DESC) (limit 10)))
 
 
 (def query-csv

src/metabase/api/user.clj

@@ -42,8 +42,8 @@
   (check-404 (exists? User :id id))
     ;; TODO - match old password against current one
     ;; TODO - password encryption
-    (upd User id :password password)
-    (sel :one User :id id))
+  (upd User id :password password)
+  (sel :one User :id id))
 
 
 (define-routes)

src/metabase/models/emailreport.clj

@@ -48,8 +48,8 @@
                   :created_at (util/new-sql-date)
                   :updated_at (util/new-sql-date)}]
     (-> (merge defaults report)
-      (assoc :dataset_query (json/write-str dataset_query)
-             :schedule (json/write-str schedule)))))
+        (assoc :dataset_query (json/write-str dataset_query)
+               :schedule (json/write-str schedule)))))
 
 (defmethod pre-update EmailReport [_ {:keys [version dataset_query schedule] :as report}]
   (assoc report
@@ -61,8 +61,7 @@
 
 (defmethod post-select EmailReport [_ {:keys [id creator_id organization_id] :as report}]
   (-> report
-    (realize-json :dataset_query)
-    (realize-json :schedule)
+    (realize-json :dataset_query :schedule)
     (util/assoc*
       :creator (delay
                  (check creator_id 500 "Can't get creator: Query doesn't have a :creator_id.")
@@ -73,4 +72,4 @@
       :recipients (delay
                     (sel :many User
                       (where {:id [in (subselect EmailReportRecipients (fields :user_id) (where {:emailreport_id id}))]}))))
-    assoc-permissions-sets))
\ No newline at end of file
+    assoc-permissions-sets))