Merge pull request #221 from metabase/use_annotations

rewrite API to use the new annotations

.dir-locals.el

@@ -5,6 +5,7 @@
                               (api-let 2)
                               (auto-parse 1)
                               (catch-api-exceptions 0)
+                              (check 1)
                               (context 2)
                               (expect 1)
                               (expect-eval-actual-first 1)
@@ -12,6 +13,7 @@
                               (ins 1)
                               (let-400 1)
                               (let-404 1)
+                              (let-500 1)
                               (match 1)
                               (match-$ 1)
                               (macrolet 1)

README.md

@@ -43,7 +43,7 @@ Run the linter with
 
 Run unit tests with
 
-    lein expectations
+    lein test
 
 
 ## Documentation

src/metabase/api/annotation.clj

 (ns metabase.api.annotation
+  "`/api/annotation` endpoints."
   (:require [korma.core :as korma]
             [compojure.core :refer [defroutes GET PUT POST DELETE]]
             [medley.core :refer [mapply]]
             [metabase.api.common :refer :all]
             [metabase.db :refer :all]
             [metabase.models.hydrate :refer :all]
-            (metabase.models [annotation :refer [Annotation annotation-general]]
+            (metabase.models [annotation :refer [Annotation annotation-general annotation-description]]
                              [org :refer [Org]])
             [metabase.util :as util]))
 
@@ -14,34 +15,50 @@
 (def object-models {:table 33
                     :field 34})
 
+(defannotation AnnotationObjectModel->ID
+  "Check that param is a valid `object-models` key string (e.g., `\"table\"`), and return corresponding value (e.g., `33`)."
+  [symb value :nillable]
+  (-> (checkp-contains? (set (keys object-models)) symb (keyword value))
+      object-models))
+
+(defannotation AnnotationType [symb value :nillable]
+  (annotation:Integer symb value)
+  (checkp-contains? (set [annotation-description annotation-general]) symb value))
+
 
 (defendpoint GET "/" [org object_model object_id]
-  (require-params org)
+  {org Required, object_model AnnotationObjectModel->ID}
   (read-check Org org)
-  (if (and object_model object_id)
-    ;; caller wants annotations about a specific entity
-    (-> (sel :many Annotation :organization_id org :object_type_id ((keyword object_model) object-models) :object_id object_id (korma/order :start :DESC))
-      (hydrate :author))
-    ;; default is to return all annotations
-    (-> (sel :many Annotation :organization_id org (korma/order :start :DESC))
-      (hydrate :author))))
+  (-> (if (and object_model object_id)
+        ;; caller wants annotations about a specific entity
+        (sel :many Annotation :organization_id org :object_type_id object_model :object_id object_id (korma/order :start :DESC))
+        ;; default is to return all annotations
+        (sel :many Annotation :organization_id org (korma/order :start :DESC)))
+      (hydrate :author)))
 
 
 (defendpoint POST "/" [:as {{:keys [organization start end title body annotation_type object_model object_id]
                              :or {annotation_type annotation-general}
                              :as request-body} :body}]
-  (require-params organization start end body annotation_type object_model object_id)
+  {organization    [Required Integer]
+   start           [Required Date]
+   end             [Required Date]
+   body            [Required NonEmptyString]
+   title           NonEmptyString
+   annotation_type [Required AnnotationType]
+   object_model    [Required AnnotationObjectModel->ID]
+   object_id       [Required Integer]}
   ;; user only needs to be member of an organization (read perms) to be able to post annotations
   (read-check Org organization)
   (-> (ins Annotation
         :organization_id organization
         :author_id *current-user-id*
-        :start (util/parse-iso8601 start)
-        :end (util/parse-iso8601 end)
+        :start start
+        :end end
         :title title
         :body body
         :annotation_type annotation_type
-        :object_type_id ((keyword object_model) object-models)
+        :object_type_id object_model
         :object_id object_id
         :edit_count 1)
     (hydrate :author)))
@@ -53,24 +70,26 @@
     (hydrate annotation :author)))
 
 
-(defendpoint PUT "/:id" [id :as {body :body}]
-  (check-400 (util/contains-many? body :start :end :title :body))
-  (let-404 [annotation (sel :one Annotation :id id)]
+(defendpoint PUT "/:id" [id :as {{:keys [start end title body]} :body}]
+  {start [Required Date]
+   end   [Required Date]
+   title [Required NonEmptyString]
+   body  [Required NonEmptyString]}
+  (let-404 [{:keys [edit_count] :as annotation} (sel :one Annotation :id id)]
     (read-check Org (:organization_id annotation))
-    (check-500 (->> (-> body
-                      (util/select-non-nil-keys :start :end :title :body)
-                      (assoc :start (util/parse-iso8601 (:start body)))
-                      (assoc :end (util/parse-iso8601 (:end body)))
-                      (assoc :edit_count (inc (get annotation :edit_count 0))))
-                 (mapply upd Annotation id)))
+    (check-500 (upd Annotation :id id
+                    :start start
+                    :end end
+                    :title title
+                    :body body
+                    :edit_count (inc (or edit_count 0))))
     (sel :one Annotation :id id)))
 
 
 (defendpoint DELETE "/:id" [id]
   (let-404 [annotation (sel :one Annotation :id id)]
     (read-check Org (:organization_id annotation))
-    (del Annotation :id id)
-    {:success true}))
+    (del Annotation :id id)))
 
 
 (define-routes)

src/metabase/api/card.clj

@@ -8,35 +8,55 @@
             (metabase.models [hydrate :refer [hydrate simple-batched-hydrate]]
                              [card :refer [Card]]
                              [card-favorite :refer [CardFavorite]]
+                             [org :refer [Org]]
                              [user :refer [User]])
             [metabase.util :as util]))
 
+(defannotation CardFilterOption [symb value :nillable]
+  (checkp-contains? #{:all :mine :fave} symb (keyword value)))
+
 (defendpoint GET "/" [org f]
-  (-> (case (or (keyword f) :all) ; default value for `f` is `:all`
-                :all (sel :many Card :organization_id org (order :name :ASC))
-                :mine (sel :many Card :organization_id org :creator_id *current-user-id* (order :name :ASC))
-                :fav (->> (-> (sel :many [CardFavorite :card_id] :owner_id *current-user-id*)
-                              (hydrate :card))
-                          (map :card)
-                          (sort-by :name)))
+  {org Required, f CardFilterOption}
+  (read-check Org org)
+  (-> (case (or f :all) ; default value for `f` is `:all`
+        :all  (sel :many Card :organization_id org (order :name :ASC))
+        :mine (sel :many Card :organization_id org :creator_id *current-user-id* (order :name :ASC))
+        :fav  (->> (-> (sel :many [CardFavorite :card_id] :owner_id *current-user-id*)
+                       (hydrate :card))
+                   (map :card)
+                   (sort-by :name)))
       (simple-batched-hydrate User :creator_id :creator)))
 
-(defendpoint POST "/" [:as {:keys [body]}]
-  (->> (-> body
-           (select-keys [:dataset_query :description :display :name :organization :public_perms :visualization_settings])
-           (clojure.set/rename-keys {:organization :organization_id} )
-           (assoc :creator_id *current-user-id*))
-       (mapply ins Card)))
+(defendpoint POST "/" [:as {{:keys [dataset_query description display name organization public_perms visualization_settings]} :body}]
+  {name         [Required NonEmptyString]
+   public_perms [Required PublicPerms]}
+  ;; TODO - which other params are required?
+  (read-check Org organization)
+  (ins Card
+    :creator_id *current-user-id*
+    :dataset_query dataset_query
+    :description description
+    :display display
+    :name name
+    :organization_id organization
+    :public_perms public_perms
+    :visualization_settings visualization_settings))
 
 (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]}]
+(defendpoint PUT "/:id" [id :as {{:keys [dataset_query description display name public_perms visualization_settings]} :body}]
+  {name         NonEmptyString, public_perms PublicPerms}
   (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)))
+  (check-500 (upd-non-nil-keys Card id
+                               :dataset_query dataset_query
+                               :description description
+                               :display display
+                               :name name
+                               :public_perms public_perms
+                               :visualization_settings visualization_settings))
   (sel :one Card :id id))
 
 (defendpoint DELETE "/:id" [id]

src/metabase/api/common.clj

@@ -6,10 +6,12 @@
             [metabase.api.common.internal :refer :all]
             [metabase.db :refer :all]
             [metabase.db.internal :refer [entity->korma]]
-            [metabase.util :as u])
+            [metabase.util :as u]
+            [metabase.util.password :as password])
   (:import com.metabase.corvus.api.ApiException))
 
-(declare check-404)
+(declare check-403
+         check-404)
 
 ;;; ## DYNAMIC VARIABLES
 ;; These get bound by middleware for each HTTP request.
@@ -91,13 +93,59 @@
      (if (empty? rest-args) test
          (recur (first rest-args) (second rest-args) (drop 2 rest-args))))))
 
-(defmacro require-params
-  "Checks that a list of params are non-nil or throws a 400."
-  [& params]
-  `(do
-     ~@(map (fn [param]
-              `(check ~param [400 ~(str "'" (name param) "' is a required param.")]))
-            params)))
+(defn check-exists?
+  "Check that object with ID exists in the DB, or throw a 404."
+  [entity id]
+  (check-404 (exists? entity :id id)))
+
+(defn check-superuser
+  "Check that `*current-user*` is a superuser or throw a 403."
+  []
+  (check-403 (:is_superuser @*current-user*)))
+
+
+;;; #### checkp- functions: as in "check param". These functions expect that you pass a symbol so they can throw ApiExceptions w/ relevant error messages.
+
+(defmacro checkp-with
+  "Check (TEST-FN VALUE), or throw an exception with STATUS-CODE (default is 400).
+   SYMB is passed in order to give the user a relevant error message about which parameter was bad.
+
+   Returns VALUE upon success.
+
+    (checkp-with (partial? contains? {:all :mine}) f :all)
+      -> :all
+    (checkp-with (partial? contains {:all :mine}) f :bad)
+      -> ApiException: Invalid value ':bad' for 'f': test failed: (partial? contains? {:all :mine}
+
+   You may optionally pass a MESSAGE to append to the ApiException upon failure;
+   this will be used in place of the \"test failed: ...\" message.
+
+   MESSAGE may be either a string or a pair like `[status-code message]`."
+  ([test-fn symb value message-or-status+message-pair]
+   {:pre [(symbol? symb)]}
+   (let [[status-code message] (if (string? message-or-status+message-pair) [400 message-or-status+message-pair]
+                                   message-or-status+message-pair)]
+     `(let [value# ~value]
+        (check (~test-fn value#)
+          [~status-code (format "Invalid value '%s' for '%s': %s" (str value#) ~symb ~message)])
+        value#)))
+  ([test-fn symb value]
+   `(checkp-with ~test-fn ~symb ~value ~(str "test failed: " test-fn))))
+
+(defn checkp-contains?
+  "Check that the VALUE of parameter SYMB is in VALID-VALUES, or throw a 400.
+   Returns VALUE upon success.
+
+    (checkp-contains? #{:fav :all :mine} 'f f)
+    -> (check (contains? #{:fav :all :mine} f)
+         [400 (str \"Invalid value '\" f \"' for 'f': must be one of: #{:fav :all :mine}\")])"
+  [valid-values-set symb value]
+  {:pre [(set? valid-values-set)
+         (symbol? symb)]}
+  (checkp-with (partial contains? valid-values-set) symb value (str "must be one of: " valid-values-set)))
+
+
+;;; #### api-let, api->, etc.
 
 ;; The following all work exactly like the corresponding Clojure versions
 ;; but take an additional arg at the beginning called RESPONSE-PAIR.
@@ -174,50 +222,131 @@
 
 ;;; ### Arg annotation fns
 
-(defmulti arg-annotation-fn
-  "Multimethod that should return a form suitable for use in a let binding.
+(defmulti -arg-annotation-fn
+  "*Internal* - don't use this directly.
 
-   Dispatches on the arg annotation as a keyword, and is also passed the symbol
-   of the argument that should be checked.
+   Multimethod used internally to dispatch arg annotation functions.
+   Dispatches on the arg annotation as a keyword.
 
-    (defendpoint GET ... [id] {id Required})
-
-     -> (let [id ~(arg-annotation-fn :Required id)]
-           ...)
-
-     -> (let [id (do (require-params id) id)]
-          ...)"
-  (fn [annotation-kw arg-symb]
-    {:pre [(keyword? annotation-kw)
-           (symbol? arg-symb)]}
+    {id Required}
+    -> ((-arg-annotation-fn :Required) 'id id)
+    -> (annotation:Required 'id id)"
+  (fn [annotation-kw]
+    {:pre [(keyword? annotation-kw)]}
     annotation-kw))
 
 ;; By default, throw an exception if we see an arg annotation we don't understand
-(defmethod arg-annotation-fn :default [annotation-kw arg-symbol]
-  (throw (Exception. (format "Don't know what to do with arg annotation '%s' on arg '%s'!" (name annotation-kw) (name arg-symbol)))))
+(defmethod -arg-annotation-fn :default [annotation-kw]
+  (throw (Exception. (format "Don't know what to do with arg annotation '%s'!" (name annotation-kw)))))
 
 ;; ### defannotation
 
 (defmacro defannotation
   "Convenience for defining a new `defendpoint` arg annotation.
 
-   BINDING is the actual symbol name of the arg being checked; `defannotation` returns form(s)
-   that will be included in the let binding for the annotated arg.
-
-    (defannotation Required [param]
-      `(require-params ~param)       ; quasiquoting needed to keep require-params from being evaluated at macroexpansion time
-      param)"
-  [annotation-name [binding] & body]
-  `(defmethod arg-annotation-fn ~(keyword annotation-name) [~'_ ~binding]
-     `(do ~~@body)))
+    (defannotation Required [symb value]
+      (when-not value
+        (throw (ApiException. 400 (format \"'%s' is a required param.\" symb))))
+      value)
+
+   SYMBOL-BINDING is bound to the *symbol* of the annotated API arg (e.g., `'org`).
+   This is useful for returning relevant error messages to the user (see example above).
+
+   VALUE-BINDING is bound to the *value* of the annotated API arg (e.g., `1`).
+
+   You may optionally specify that the param is `:nillable`.
+   This means BODY will only be evaluated if VALUE is non-nil.
+
+    (defannotation CardFilterOption [symb value :nillable]
+      (checkp-contains? #{:all :mine :fav} symb (keyword value)))
+
+   Internally, `defannotation` creates a function with the name of the annotation prefixed by `annotation:`.
+   This can be used to test the annotation:
+
+    (annotation:Required org 100) -> 100
+    (annotation:Required org nil) -> ApiException: 'org' is a required param.
+
+   You can also use it inside the body of another annotation:
+
+    (defannotation PublicPerm [symb value :nillable]
+      (annotation:Integer symb value]
+      (checkp-contains? #{0 1 2} symb value))"
+  {:arglists '([annotation-name docstr? [symbol-binding value-binding nillable?] & body])}
+  [annotation-name & args]
+  {:pre [(symbol? annotation-name)]}
+  (let [[docstr [[symbol-binding value-binding & [nillable?]] & body]] (u/optional string? args)]
+    (assert (symbol? symbol-binding))
+    (assert (symbol? value-binding))
+    (assert (or (nil? nillable?)
+                (= nillable? :nillable)))
+    (let [fn-name (symbol (str "annotation:" annotation-name))]
+      `(do
+         (defn ~fn-name ~@(when docstr [docstr]) [~symbol-binding ~value-binding]
+           {:pre [(symbol? ~symbol-binding)]}
+           ~(if nillable?
+              `(when ~value-binding
+                 ~@body)
+              `(do
+                 ~@body)))
+         (defmethod -arg-annotation-fn ~(keyword annotation-name) [~'_]
+           ~fn-name)))))
 
 ;; ### common annotation definitions
 
-;; `required` just calls require-params
-(defannotation Required [param]
-  `(require-params ~param)
-  param)
-
+(defannotation Required
+  "Throw a 400 if param is `nil`."
+  [symb value]
+  (when-not value
+    (throw (ApiException. (int 400) (format "'%s' is a required param." symb))))
+  value)
+
+(defannotation Date
+  "try to parse 'date' string as an ISO-8601 date"
+  [symb value :nillable]
+  (try (u/parse-iso8601 value)
+          (catch Throwable _
+            (throw (ApiException. (int 400) (format "'%s' is not a valid date." symb))))))
+
+(defannotation String->Integer [symb value :nillable]
+  (try (Integer/parseInt value)
+       (catch java.lang.NumberFormatException _
+         (format "Invalid value '%s' for '%s': cannot parse as an integer." value symb))))
+
+(defannotation Integer
+  "Check that a param is an integer (this does *not* cast the param!)"
+  [symb value :nillable]
+  (checkp-with integer? symb value "value must be an integer."))
+
+(defannotation Boolean
+  "Check that param is a boolean (this does *not* cast the param!)"
+  [symb value :nillable]
+  (checkp-with boolean? symb value "value must be a boolean."))
+
+(defannotation Dict
+  "Check that param is a dictionary (this does *not* cast the param!)"
+  [symb value :nillable]
+  (checkp-with map? symb value "value must be a dictionary."))
+
+(defannotation NonEmptyString
+  "Check that param is a non-empty string (strings that only contain whitespace are considered empty)."
+  [symb value :nillable]
+  (checkp-with (complement clojure.string/blank?) symb value "value must be a non-empty string."))
+
+(defannotation PublicPerms
+  "check that perms is `int` in `#{0 1 2}`"
+  [symb value :nillable]
+  (annotation:Integer symb value)
+  (checkp-contains? #{0 1 2} symb value))
+
+(defannotation Email
+  "Check that param is a valid email address."
+  [symb value :nillable]
+  (checkp-with u/is-email? symb value "Not a valid email address."))
+
+(defannotation ComplexPassword
+  "Check that a password is complex enough."
+  [symb value]
+  (checkp-with password/is-complex? symb value "Insufficient password strength"))
 
 ;;; ### defendpoint
 
@@ -242,11 +371,11 @@
         [arg-annotations body] (u/optional #(and (map? %) (every? symbol? (keys %))) more)]
     `(do (def ~name
            (~method ~route ~args
-                    (auto-parse ~args
-                      (catch-api-exceptions
+                    (catch-api-exceptions
+                      (auto-parse ~args
                         (let-annotated-args ~arg-annotations
-                          (-> (do ~@body)
-                              wrap-response-if-needed))))))
+                                            (-> (do ~@body)
+                                                wrap-response-if-needed))))))
          (alter-meta! #'~name assoc :is-endpoint? true))))
 
 (defmacro define-routes

src/metabase/api/common/internal.clj

@@ -21,22 +21,28 @@
 
 ;;; ## ROUTE TYPING / AUTO-PARSE SHARED FNS
 
+(defn parse-int [value]
+  (try (Integer/parseInt value)
+       (catch java.lang.NumberFormatException _
+         (throw (ApiException. (int 400) (format "Not a valid integer: '%s'" value))))))
+
 (def ^:dynamic *auto-parse-types*
   "Map of `param-type` -> map with the following keys:
 
      :route-param-regex Regex pattern that should be used for params in Compojure route forms
      :parser            Function that should be used to parse args"
   {:int {:route-param-regex #"[0-9]+"
-         :parser 'Integer/parseInt}
+         :parser 'metabase.api.common.internal/parse-int}
    :uuid {:route-param-regex #"[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}"
           :parser nil}})
 
 (def ^:dynamic *auto-parse-arg-name-patterns*
   "Sequence of `[param-pattern parse-type]` pairs.
    A param with name matching PARAM-PATTERN should be considered to be of AUTO-PARSE-TYPE."
-  [[#"^uuid$" :uuid]
-   [#"^[\w-_]*id$" :int]
-   [#"^org$" :int]])
+  [[#"^uuid$"       :uuid]
+   [#"^session_id$" :uuid]
+   [#"^[\w-_]*id$"  :int]
+   [#"^org$"        :int]])
 
 (defn arg-type
   "Return a key into `*auto-parse-types*` if ARG has a matching pattern in `*auto-parse-arg-name-patterns*`.
@@ -155,7 +161,7 @@
   [[annotation-kw arg-symb]] ; dispatch-fn passed as a param to avoid circular dependencies
   {:pre [(keyword? annotation-kw)
          (symbol? arg-symb)]}
-  `[~arg-symb ~((eval 'metabase.api.common/arg-annotation-fn) annotation-kw arg-symb)])
+  `[~arg-symb (~((eval 'metabase.api.common/-arg-annotation-fn) annotation-kw) '~arg-symb ~arg-symb)])
 
 (defn process-arg-annotations [annotations-map]
   {:pre [(or (nil? annotations-map)

src/metabase/api/dash.clj

@@ -7,36 +7,45 @@
             (metabase.models [hydrate :refer [hydrate]]
                              [card :refer [Card]]
                              [dashboard :refer [Dashboard]]
-                             [dashboard-card :refer [DashboardCard]])
+                             [dashboard-card :refer [DashboardCard]]
+                             [org :refer [Org]])
             [metabase.util :as u]))
 
+(defannotation DashFilterOption [symb value :nillable]
+  (checkp-contains? #{:all :mine} symb (keyword value)))
+
 (defendpoint GET "/" [org f]
-  (-> (case (or (keyword f) :all) ; default value for f is `all`
-        :all (sel :many Dashboard :organization_id org)
+  {org Required, f DashFilterOption}
+  (read-check Org org)
+  (-> (case (or f :all) ; default value for f is `all`
+        :all  (sel :many Dashboard :organization_id org)
         :mine (sel :many Dashboard :organization_id org :creator_id *current-user-id*))
       (hydrate :creator :organization)))
 
-(defendpoint POST "/" [:as {:keys [body]}]
-  (let [{:keys [organization]} body]
-    (check-403 (org-perms-case organization ; check that current-user can make a Dashboard for this org
-                 :admin true
-                 :default true
-                 nil false))
-    (->> (-> body
-             (select-keys [:organization :name :public_perms])
-             (clojure.set/rename-keys {:organization :organization_id})
-             (assoc :creator_id *current-user-id*))
-         (medley/mapply ins Dashboard))))
+(defendpoint POST "/" [:as {{:keys [organization name public_perms] :as body} :body}]
+  {name         Required
+   organization Required
+   public_perms Required}
+  (read-check Org organization) ; any user who has permissions for this Org can make a dashboard
+  (ins Dashboard
+    :organization_id organization
+    :name name
+    :public_perms public_perms
+    :creator_id *current-user-id*))
 
 (defendpoint GET "/:id" [id]
   (let-404 [db (-> (sel :one Dashboard :id id)
                    (hydrate :creator :organization [:ordered_cards [:card :creator]] :can_read :can_write))]
     {:dashboard db})) ; why is this returned with this {:dashboard} wrapper?
 
-(defendpoint PUT "/:id" [id :as {body :body}]
+(defendpoint PUT "/:id" [id :as {{:keys [description name public_perms]} :body}]
+  {name         NonEmptyString
+   public_perms PublicPerms}
   (write-check Dashboard id)
-  (check-500 (->> (u/select-non-nil-keys body :description :name :public_perms)
-                  (medley/mapply upd Dashboard id)))
+  (check-500 (upd-non-nil-keys Dashboard id
+                               :description description
+                               :name name
+                               :public_perms public_perms))
   (sel :one Dashboard :id id))
 
 (defendpoint DELETE "/:id" [id]
@@ -44,11 +53,13 @@
   (del Dashboard :id id))
 
 (defendpoint POST "/:id/cards" [id :as {{:keys [cardId]} :body}]
+  {cardId [Required Integer]}
   (write-check Dashboard id)
   (check-400 (exists? Card :id cardId))
   (ins DashboardCard :card_id cardId :dashboard_id id))
 
 (defendpoint DELETE "/:id/cards" [id dashcardId]
+  {dashcardId [Required String->Integer]}
   (write-check Dashboard id)
   (del DashboardCard :id dashcardId :dashboard_id id))
 

src/metabase/api/emailreport.clj

@@ -8,16 +8,24 @@
             (metabase.models [common :as common]
                              [hydrate :refer :all]
                              [database :refer [databases-for-org]]
-                             [emailreport :refer [EmailReport modes-input days-of-week times-of-day]]
+                             [emailreport :refer [EmailReport modes-input days-of-week times-of-day] :as model]
                              [emailreport-executions :refer [EmailReportExecutions]]
+                             [org :refer [Org]]
                              [user :refer [users-for-org]])
             [metabase.util :as util]))
 
+(defannotation EmailReportFilterOption [symb value :nillable]
+  (checkp-contains? #{:all :mine} symb (keyword value)))
+
+(defannotation EmailReportMode
+  "Check that param is a value int ID for an email report mode."
+  [symb value :nillable]
+  (annotation:Integer symb value)
+  (checkp-contains? (set (map :id (vals model/modes))) symb value))
 
 (defendpoint GET "/form_input" [org]
-  (require-params org)
-  ; we require admin/default perms on the org to do this operation
-  (check-403 ((:perms-for-org @*current-user*) org))
+  {org Required}
+  (read-check Org org)
   (let [dbs (databases-for-org org)
         users (users-for-org org)]
     {:permissions common/permissions
@@ -32,28 +40,33 @@
 (defendpoint GET "/" [org f]
   ;; TODO - filter by f == "mine"
   ;; TODO - filter by creator == self OR public_perms > 0
-  (require-params org)
-  ; we require admin/default perms on the org to do this operation
-  (check-403 ((:perms-for-org @*current-user*) org))
+  {org Required, f EmailReportFilterOption}
+  (read-check Org org)
   (-> (sel :many EmailReport
         (where {:organization_id org})
         (where {:public_perms [> common/perms-none]})
         (order :name :ASC))
-    (hydrate :creator :organization :can_read :can_write)))
-
-
-(defendpoint POST "/" [:as {{:keys [organization] :as body} :body}]
-  ; enforce a few required attributes
-  (check-400 (util/contains-many? body :organization :name :dataset_query :schedule))
-  ; we require admin/default perms on the org to do this operation
-  (check-403 ((:perms-for-org @*current-user*) organization))
-  ;; TODO - validate that for public_perms, mode, etc are within their expected set of possible values
-  ;; TODO - deal with recipients
-  (check-500 (->> (-> body
-                    (select-keys [:organization :name :description :public_perms :mode :dataset_query :email_addresses :schedule])
-                    (clojure.set/rename-keys {:organization :organization_id})
-                    (assoc :creator_id *current-user-id*))
-               (mapply ins EmailReport))))
+    (hydrate :creator :organization :can_read "can_write")))
+
+
+(defendpoint POST "/" [:as {{:keys [dataset_query description email_addresses mode name organization public_perms schedule] :as body} :body}]
+  {dataset_query  Required
+   name           Required
+   organization   Required
+   schedule       Required
+   mode           EmailReportMode
+   public_perms   PublicPerms}
+  (read-check Org organization)
+  (check-500 (ins EmailReport
+               :creator_id *current-user-id*
+               :dataset_query dataset_query
+               :description description
+               :email_addresses email_addresses
+               :mode mode
+               :name name
+               :organization_id organization
+               :public_perms public_perms
+               :schedule schedule))) ; TODO - deal with recipients
 
 
 (defendpoint GET "/:id" [id]
@@ -62,18 +75,24 @@
          (hydrate :creator :organization :can_read :can_write)))
 
 
-(defendpoint PUT "/:id" [id :as {body :body}]
-  ; user must have WRITE permissions on the report
+(defendpoint PUT "/:id" [id :as {{:keys [dataset_query description email_addresses mode name public_perms schedule] :as body} :body}]
+  {name         NonEmptyString
+   mode         EmailReportMode
+   public_perms PublicPerms}
   (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
+    (check-500 (upd-non-nil-keys EmailReport id
+                                 :dataset_query   dataset_query
+                                 :description     description
+                                 :email_addresses email_addresses
+                                 :mode            mode
+                                 :name            name
+                                 :public_perms    public_perms
+                                 :schedule        schedule
+                                 :version         (inc (:version report)))))
+  (-> (sel :one EmailReport :id id)
+      (hydrate :creator :database :can_read :can_write)))
     ;; TODO - deal with recipients
-    (check-500 (->> (-> (merge report (util/select-non-nil-keys body :name :description :public_perms :mode :dataset_query :email_addresses :schedule))
-                      ;; filter keys again AFTER merging with the existing report data.  this is needed to add version into the mix
-                      (util/select-non-nil-keys :name :description :public_perms :mode :version :dataset_query :email_addresses :schedule))
-                 (mapply upd EmailReport id)))
-    (-> (sel :one EmailReport :id id)
-      (hydrate :creator :database :can_read :can_write))))
 
 
 (defendpoint DELETE "/:id" [id]

src/metabase/api/meta/db.clj

@@ -15,16 +15,21 @@
                              [table :refer [Table]])
             [metabase.util :as u]))
 
+(defannotation DBEngine [symb value :nillable]
+  (checkp-contains? (set (map first driver/available-drivers)) symb value))
+
 (defendpoint GET "/" [org]
-  (require-params org)
-  (check-403 (org-can-read org))
+  {org Required}
+  (read-check Org org)
   (-> (sel :many Database :organization_id org (order :name))
       (simple-batched-hydrate Org :organization_id :organization)))
 
 (defendpoint POST "/" [:as {{:keys [org name engine details] :as body} :body}]
-  (require-params org name engine details)
-  (check (contains? (set (map first driver/available-drivers)) engine) [400 "Invalid engine type specified."])
-  (check-403 (org-can-write org))
+  {org     Required
+   name    [Required NonEmptyString]
+   engine  [Required DBEngine]
+   details [Required Dict]}
+  (write-check Org org)
   (let-500 [new-db (ins Database :organization_id org :name name :engine engine :details details)]
     ;; kick off background job to gather schema metadata about our new db
     (future (driver/sync-tables new-db))
@@ -35,9 +40,10 @@
   {:timezones metabase.models.common/timezones
    :engines driver/available-drivers})
 
-;Stub function that will eventually validate a connection string
+;; Stub function that will eventually validate a connection string
 (defendpoint POST "/validate" [:as {{:keys [host port]} :body}]
-  (require-params host port)
+  {host Required
+   port Required}
   (let [response-invalid (fn [m] {:status 400 :body {:valid false :message m}})]
     (cond
       (not (u/host-up? host)) (response-invalid "Host not reachable")
@@ -48,10 +54,13 @@
   (->404 (sel :one Database :id id)
          (hydrate :organization)))
 
-(defendpoint PUT "/:id" [id :as {body :body}]
+(defendpoint PUT "/:id" [id :as {{:keys [name engine details]} :body}]
+  {name NonEmptyString, details Dict} ; TODO - check that engine is a valid choice
   (write-check Database id)
-  (check-500 (->> (u/select-non-nil-keys body :name :engine :details)
-                  (medley/mapply upd Database id)))
+  (check-500 (upd-non-nil-keys Database id
+                               :name name
+                               :engine engine
+                               :details details))
   (sel :one Database :id id))
 
 (defendpoint DELETE "/:id" [id]
@@ -88,8 +97,8 @@
         (simple-batched-hydrate Table :table_id :table))))
 
 (defendpoint POST "/:id/sync" [id]
-  (let-404 [db (sel :one Database :id id)]   ; TODO - run sync-tables asynchronously
-           (driver/sync-tables db))
+  (let-404 [db (sel :one Database :id id)]
+    (future (driver/sync-tables db))) ; run sync-tables asynchronously
   {:status :ok})
 
 

src/metabase/api/meta/field.clj

@@ -5,19 +5,33 @@
             [metabase.db :refer :all]
             [metabase.driver :as driver]
             (metabase.models [hydrate :refer [hydrate]]
-                             [field :refer [Field]]
-                             [foreign-key :refer [ForeignKey]])
+                             [field :refer [Field] :as field]
+                             [foreign-key :refer [ForeignKey] :as fk])
             [metabase.util :as u]))
 
+(defannotation FieldSpecialType [symb value :nillable]
+  (checkp-contains? field/special-types symb (keyword value)))
+
+(defannotation FieldType [symb value :nillable]
+  (checkp-contains? field/field-types symb (keyword value)))
+
+(defannotation ForeignKeyRelationship [symb value :nillable]
+  (checkp-contains? fk/relationships symb (keyword value)))
+
 (defendpoint GET "/:id" [id]
   (->404 (sel :one Field :id id)
          read-check
          (hydrate [:table :db])))
 
-(defendpoint PUT "/:id" [id :as {body :body}]
+(defendpoint PUT "/:id" [id :as {{:keys [special_type preview_display description]} :body}]
+  {special_type FieldSpecialType
+   ;; TODO - base_type ??
+   }
   (write-check Field id)
-  (check-500 (->> (u/select-non-nil-keys body :special_type :preview_display :description)
-                  (medley/mapply upd Field id)))
+  (check-500 (upd-non-nil-keys Field id
+               :special_type    special_type
+               :preview_display preview_display
+               :description     description))
   (sel :one Field :id id))
 
 (defendpoint GET "/:id/summary" [id]
@@ -30,18 +44,19 @@
 (defendpoint GET "/:id/foreignkeys" [id]
   (read-check Field id)
   (-> (sel :many ForeignKey :origin_id id)
-    (hydrate [:origin [:table]] [:destination [:table]])))
+      (hydrate [:origin :table] [:destination :table])))
 
 
 (defendpoint POST "/:id/foreignkeys" [id :as {{:keys [target_field relationship]} :body}]
-  (require-params target_field relationship)
+  {target_field Required
+   relationship [Required ForeignKeyRelationship]}
   (write-check Field id)
   (write-check Field target_field)
   (-> (ins ForeignKey
         :origin_id id
         :destination_id target_field
         :relationship relationship)
-    (hydrate [:origin [:table]] [:destination [:table]])))
+    (hydrate [:origin :table] [:destination :table])))
 
 
 ;; ## TODO - Endpoints not yet implemented

src/metabase/api/meta/table.clj

@@ -9,26 +9,31 @@
                              [database :refer [Database]]
                              [field :refer [Field]]
                              [foreign-key :refer [ForeignKey]]
-                             [table :refer [Table]])
+                             [table :refer [Table] :as table])
             [metabase.util :as u]))
 
+(defannotation TableEntityType [symb value :nillable]
+  (checkp-contains? table/entity-types symb (keyword value)))
 
 (defendpoint GET "/" [org]
-  (require-params org)
-  (let [db-ids (->> (sel :many [Database :id] :organization_id org)
-                 (map :id))]
+  {org Required}
+  (let [db-ids (sel :many :id Database :organization_id org)]
     (-> (sel :many Table :db_id [in db-ids] (order :name :ASC))
-      (simple-batched-hydrate Database :db_id :db))))
+        (simple-batched-hydrate Database :db_id :db))))
 
 
 (defendpoint GET "/:id" [id]
   (->404 (sel :one Table :id id)
          (hydrate :db :pk_field)))
 
-(defendpoint PUT "/:id" [id :as {body :body}]
+(defendpoint PUT "/:id" [id :as {{:keys [entity_name entity_type description]} :body}]
+  {entity_name NonEmptyString
+   entity_type TableEntityType}
   (write-check Table id)
-  (check-500 (->> (u/select-non-nil-keys body :entity_name :entity_type :description)
-                  (m/mapply upd Table id)))
+  (check-500 (upd-non-nil-keys Table id
+                               :entity_name entity_name
+                               :entity_type entity_type
+                               :description description))
   (sel :one Table :id id))
 
 (defendpoint GET "/:id/fields" [id]
@@ -40,11 +45,10 @@
 
 (defendpoint GET "/:id/fks" [id]
   (read-check Table id)
-  (let-404 [field-ids (->> (sel :many :fields [Field :id] :table_id id)
-                        (map :id))]
+  (let-404 [field-ids (sel :many :id Field :table_id id)]
     (-> (sel :many ForeignKey :destination_id [in field-ids])
-      ;; TODO - it's a little silly to hydrate both of these table objects
-      (hydrate [:origin [:table]] [:destination [:table]]))))
+        ;; TODO - it's a little silly to hydrate both of these table objects
+        (hydrate [:origin :table] [:destination :table]))))
 
 
 ;; TODO - GET /:id/segments

src/metabase/api/org.clj

@@ -10,7 +10,6 @@
                              [org-perm :refer [OrgPerm grant-org-perm]])
             [metabase.util :as util]))
 
-
 (defendpoint GET "/" []
   (if (:is_superuser @*current-user*)
     ;; superusers get all organizations
@@ -20,17 +19,13 @@
 
 
 (defendpoint POST "/" [:as {{:keys [name slug] :as body} :body}]
-  (require-params name slug)
-  ;; user must be a superuser to proceed
-  (check-403 (:is_superuser @*current-user*))
-  ;; create the new org
+  {name [Required NonEmptyString]
+   slug [Required NonEmptyString]} ; TODO - check logo_url ?
+  (check-superuser)
   (let-500 [{:keys [id] :as new-org} (->> (util/select-non-nil-keys body :slug :name :description :logo_url)
-                                       (mapply ins Org))]
-    ;; now that the Org exists, add the creator as the first admin member
-    (grant-org-perm id *current-user-id* true)
-    ;; make sure the api response is still the newly created org
-    new-org))
-
+                                          (mapply ins Org))]
+    (grant-org-perm id *current-user-id* true) ; now that the Org exists, add the creator as the first admin member
+    new-org))                                  ; make sure the api response is still the newly created org
 
 (defendpoint GET "/:id" [id]
   (->404 (sel :one Org :id id)
@@ -42,10 +37,13 @@
          read-check))
 
 
-(defendpoint PUT "/:id" [id :as {body :body}]
+(defendpoint PUT "/:id" [id :as {{:keys [name description logo_url]} :body}]
+  {name NonEmptyString}
   (write-check Org id)
-  (check-500 (->> (util/select-non-nil-keys body :name :description :logo_url)
-                  (mapply upd Org id)))
+  (check-500 (upd-non-nil-keys Org id
+                               :description description
+                               :logo_url logo_url
+                               :name name))
   (sel :one Org :id id))
 
 
@@ -57,9 +55,10 @@
 
 (defendpoint POST "/:id/members" [id :as {{:keys [first_name last_name email admin]
                                            :or {admin false}} :body}]
-  ; check our input
-  (check-400 (and first_name last_name email (util/is-email? email)))
-  ; user must have admin perms on org to proceed
+  {admin      Boolean
+   first_name [Required NonEmptyString]
+   last_name  [Required NonEmptyString]
+   email      [Required Email]}
   (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
@@ -73,25 +72,26 @@
 
 
 (defendpoint POST "/:id/members/:user-id" [id user-id :as {{:keys [admin]} :body}]
+  {admin Boolean}
   (write-check Org id)
-  (check-404 (exists? User :id user-id))
-  (grant-org-perm id user-id (boolean admin))
+  (check-exists? User user-id)
+  (grant-org-perm id user-id admin)
   {:success true})
 
+;; TODO `POST` and `PUT` endpoints are exactly the same. Do we need both?
 
 (defendpoint PUT "/:id/members/:user-id" [id user-id :as {{:keys [admin]} :body}]
+  {admin Boolean}
   (write-check Org id)
-  (check-404 (exists? User :id user-id))
-  (grant-org-perm id user-id (boolean admin))
+  (check-exists? User user-id)
+  (grant-org-perm id user-id admin)
   {:success true})
 
 
 (defendpoint DELETE "/: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)]
-      (del OrgPerm :user_id user-id :organization_id id))))
+  (write-check Org id)
+  (check-exists? User user-id)
+  (del OrgPerm :user_id user-id :organization_id id))
 
 
 (define-routes)

src/metabase/api/qs.clj

@@ -12,9 +12,9 @@
 
 (declare execute-query)
 
-
 (defendpoint POST "/" [:as {{:keys [timezone database sql] :as body} :body}]
-  (require-params database sql)
+  {database [Required Integer]
+   sql      [Required NonEmptyString]} ; TODO - check timezone
   (read-check Database database)
   (let [dataset-query {:type "native"
                        :database database

src/metabase/api/query.clj

@@ -14,22 +14,23 @@
                              [query-execution :refer [QueryExecution all-fields]])
             [metabase.util :as util]))
 
+(defannotation QueryFilterOption [symb value :nillable]
+  (checkp-contains? #{:all :mine} symb (keyword value)))
 
 (defendpoint GET "/form_input" [org]
-  (require-params org)
-  (check-403 ((:perms-for-org @*current-user*) org))
-  (let [dbs (databases-for-org org)]
-    {:permissions common/permissions
-     :timezones common/timezones
-     :databases dbs}))
+  {org Required}
+  (read-check Org org)
+  {:permissions common/permissions
+   :timezones   common/timezones
+   :databases   (databases-for-org org)})
 
 
 (defendpoint GET "/" [org f]
-  (require-params org)
-  (check-403 ((:perms-for-org @*current-user*) org))
-  (-> (case (or (keyword f) :all) ; default value for `f` is `:all`
+  {org Required, f QueryFilterOption}
+  (read-check Org org)
+  (-> (case (or f :all) ; default value for `f` is `:all`
         :all (sel :many Query
-               (where (or (= :creator_id *current-user-id*) (> :public_perms common/perms-none)))
+                  (where (or (= :creator_id *current-user-id*) (> :public_perms common/perms-none)))
                (where {:database_id [in (subselect Database (fields :id) (where {:organization_id org}))]})
                (order :name :ASC))
         :mine (sel :many Query :creator_id *current-user-id*
@@ -57,7 +58,8 @@
   [{:keys [name sql timezone public_perms database]
     :or {name (str "New Query: " (java.util.Date.))
          public_perms common/perms-none}}]
-  (require-params database sql)
+  (check database [400 "'database' is a required param."]
+         sql      [400 "'sql' is a required param."])
   (read-check Database database)
   (ins Query
     :type "rawsql"
@@ -70,6 +72,7 @@
 
 
 (defendpoint POST "/" [:as {{:keys [clone] :as body} :body}]
+  {clone Integer}
   (if clone
     (query-clone clone)
     (query-create body)))
@@ -81,15 +84,17 @@
          (hydrate :creator :database :can_read :can_write)))
 
 
-(defendpoint PUT "/:id" [id :as {{:keys [timezone database] :as body} :body}]
-  (require-params database)
+(defendpoint PUT "/:id" [id :as {{:keys [name public_perms details timezone database] :as body} :body}]
+  {database     [Required Dict]
+   name         NonEmptyString
+   public_perms PublicPerms}
   (read-check Database (:id database))
   (let-404 [query (sel :one Query :id id)]
     (write-check query)
     (-> (util/select-non-nil-keys body :name :public_perms :details)
-      (assoc :version (:version query)    ; don't increment this here. That happens on pre-update
-             :database_id (:id database)) ; you can change the DB of a Query why??
-      (#(mapply upd Query id %)))
+        (assoc :version (:version query)    ; don't increment this here. That happens on pre-update
+               :database_id (:id database)) ; you can change the DB of a Query why??
+        (#(mapply upd Query id %)))
     (-> (sel :one Query :id id)
         (hydrate :creator :database))))
 

src/metabase/api/result.clj

@@ -24,7 +24,7 @@
 
 ;; Returns the actual data response for a given query result (as if the query was just executed)
 (defendpoint GET "/:id/response" [id]
-  (let-404 [{:keys [query_id] :as query-execution} (eval `(sel :one ~all-fields :id ~id))]
+  (let-404 [{:keys [query_id] :as query-execution} (sel :one all-fields :id id)]
     ;; NOTE - this endpoint requires there to be a saved query associated with this execution
     (check-404 query_id)
     (let-404 [{{can_read :can_read} :query} (hydrate query-execution :query)]
@@ -33,16 +33,16 @@
 
 
 ;; Returns the data response for a given query result as a CSV file
-(def query-result-csv
-  (GET "/:id/csv" [id]
-    (let-404 [{:keys [result_data query_id] :as query-execution} (sel :one all-fields :id id)]
-      ;; NOTE - this endpoint requires there to be a saved query associated with this execution
-      (check-404 query_id)
-      (let-404 [{{can_read :can_read name :name} :query} (hydrate query-execution :query)]
-        (check-403 @can_read)
-        {:status 200
-         :body (with-out-str (csv/write-csv *out* (into [(:columns result_data)] (:rows result_data))))
-         :headers {"Content-Type" "text/csv", "Content-Disposition" (str "attachment; filename=\"" name ".csv\"")}}))))
-
-
-(define-routes query-result-csv)
+(defendpoint GET "/:id/csv" [id]
+  (let-404 [{:keys [result_data query_id] :as query-execution} (sel :one all-fields :id id)]
+    ;; NOTE - this endpoint requires there to be a saved query associated with this execution
+    (check-404 query_id)
+    (let-404 [{{can_read :can_read name :name} :query} (hydrate query-execution :query)]
+      (check-403 @can_read)
+      {:status 200
+       :body (with-out-str (csv/write-csv *out* (into [(:columns result_data)] (:rows result_data))))
+       :headers {"Content-Type" "text/csv"
+                 "Content-Disposition" (str "attachment; filename=\"" name ".csv\"")}})))
+
+
+(define-routes)

src/metabase/api/search.clj

@@ -56,9 +56,12 @@
           models))
 
 ;; primary search endpoint
-(defendpoint POST "/" [:as {{:keys [org page results_per_page load_all q models]
+(defendpoint POST "/" [:as {{:keys [page results_per_page q models]
                              :or {page 1
                                   results_per_page 10}} :body}]
+  {page             Integer
+   results_per_page Integer
+   q                [Required NonEmptyString]}
   (let [models (if (empty? models) (vals search-choices)         ; if `models` is unspecified default to all search choices
                    (->> models                                   ; otherwise get the corresponding search choice maps
                         (map keyword)
@@ -81,7 +84,7 @@
             :end_index (+ offset (count page-results))}}))
 
 ;; return map of available search choices -> plural name like `{:table "Tables"}`
-(defendpoint GET "/model_choices" [org]
+(defendpoint GET "/model_choices" []
   {:choices {:metabase (->> search-choices
                             (map (fn [[choice {:keys [plural-name]}]]
                                    {choice plural-name}))

src/metabase/api/session.clj

@@ -15,9 +15,11 @@
 
 ;; login
 (defendpoint POST "/" [:as {{:keys [email password] :as body} :body}]
-  (require-params email password)
+  {email    [Required Email]
+   password [Required NonEmptyString]}
   (let-400 [user (sel :one :fields [User :id :password_salt :password] :email email (korma/where {:is_active true}))]
-    (check (creds/bcrypt-verify (str (:password_salt user) password) (:password user)) [400 "password mismatch"])
+    (check (creds/bcrypt-verify (str (:password_salt user) password) (:password user))
+      [400 "password mismatch"])
     (let [session-id (str (java.util.UUID/randomUUID))]
       (ins Session
         :id session-id
@@ -26,9 +28,9 @@
 
 
 ;; logout
-(defendpoint DELETE "/" [:as {{:keys [session_id]} :params}]
-  (check-400 session_id)
-  (check-400 (exists? Session :id session_id))
+(defendpoint DELETE "/" [session_id]
+  {session_id [Required NonEmptyString]}
+  (check-exists? Session session_id)
   (del Session :id session_id))
 
 
@@ -40,7 +42,7 @@
   ;; This is a bit sketchy. Someone malicious could send a bad origin header and hit this endpoint to send
   ;; a forgotten password email to another User, and take them to some sort of phishing site. Although not sure
   ;; what you could phish from them since they already forgot their password.
-  (require-params email)
+  {email [Required Email]}
   (let [{user-id :id user-name :common_name} (sel :one User :email email)
         reset-token (java.util.UUID/randomUUID)
         password-reset-url (str origin "/auth/reset_password/" reset-token)]
@@ -55,8 +57,8 @@
 
 ;; set password from reset token
 (defendpoint POST "/reset_password" [:as {{:keys [token password] :as body} :body}]
-  (require-params token password)
-  (check (password/is-complex? password) [400 "Insufficient password strength"])
+  {token    Required
+   password [Required ComplexPassword]}
   (let-404 [user (sel :one :fields [User :id :reset_triggered] :reset_token token)]
     ;; check that the reset was triggered within the last 1 HOUR, after that the token is considered expired
     (check-404 (> (* 60 60 1000) (- (System/currentTimeMillis) (get user :reset_triggered 0))))

src/metabase/api/setting.clj

@@ -6,25 +6,25 @@
 
 ;; ## Get all settings + values
 (defendpoint GET "/" []
-  (check-403 (:is_superuser @*current-user*))
+  (check-superuser)
   (setting/all-with-descriptions))
 
 ;; ## Get a single setting
 (defendpoint GET "/:key" [key]
-  (require-params key)
-  (check-403 (:is_superuser @*current-user*))
+  {key Required}
+  (check-superuser)
   (setting/get (keyword key)))
 
 ;; ## Create/update a setting
 (defendpoint PUT "/:key" [key  :as {{:keys [value]} :body}]
-  (require-params key value)
-  (check-403 (:is_superuser @*current-user*))
+  {key Required, value Required}
+  (check-superuser)
   (setting/set (keyword key) value))
 
 ;; ## Delete a setting
 (defendpoint DELETE "/:key" [key]
-  (require-params key)
-  (check-403 (:is_superuser @*current-user*))
+  {key Required}
+  (check-superuser)
   (setting/delete (keyword key)))
 
 (define-routes)

src/metabase/api/setup.clj

@@ -5,18 +5,21 @@
             (metabase.models [session :refer [Session]]
                              [user :refer [User set-user-password]])
             [metabase.setup :as setup]
-            [metabase.util :as util]
-            [metabase.util.password :as password]))
+            [metabase.util :as util]))
 
+(defannotation SetupToken
+  "Check that param matches setup token or throw a 403."
+  [symb value]
+  (checkp-with setup/token-match? symb value [403 "Token does not match the setup token."]))
 
 ;; special endpoint for creating the first user during setup
 ;; this endpoint both creates the user AND logs them in and returns a session id
 (defendpoint POST "/user" [:as {{:keys [token first_name last_name email password] :as body} :body}]
-  ;; check our input
-  (require-params token first_name last_name email password)
-  (check-400 (and (util/is-email? email) (password/is-complex? password)))
-  ;; the submitted token must match our setup token
-  (check-403 (setup/token-match? token))
+  {first_name [Required NonEmptyString]
+   last_name  [Required NonEmptyString]
+   email      [Required Email]
+   password   [Required ComplexPassword]
+   token      [Required SetupToken]}
   ;; extra check.  don't continue if there is already a user in the db.
   (let [session-id (str (java.util.UUID/randomUUID))
         new-user (ins User

src/metabase/api/user.clj

@@ -3,24 +3,20 @@
             [compojure.core :refer [defroutes GET PUT]]
             [medley.core :refer [mapply]]
             [metabase.api.common :refer :all]
-            [metabase.db :refer [sel upd exists?]]
+            [metabase.db :refer [sel upd upd-non-nil-keys exists?]]
             (metabase.models [hydrate :refer [hydrate]]
                              [user :refer [User set-user-password]])
-            [metabase.util :refer [is-email? select-non-nil-keys]]
             [metabase.util.password :as password]))
 
-(defannotation Email [email]
-  `(check (is-email? ~email) [400 (format ~(str (name email) " '%s' is not a valid email.") ~email)])
-  email)
-
-(defannotation ComplexPassword [password]
-  `(check (password/is-complex? ~password) [400 "Insufficient password strength"])
-  password)
-
+(defn ^:private check-self-or-superuser
+  "Check that USER-ID is `*current-user-id*` or that `*current-user*` is a superuser, or throw a 403."
+  [user-id]
+  {:pre [(integer? user-id)]}
+  (check-403 (or (= user-id *current-user-id*)
+                 (:is_superuser @*current-user*))))
 
 (defendpoint GET "/" []
-  ;; user must be a superuser to proceed
-  (check-403 (:is_superuser @*current-user*))
+  (check-superuser) ; user must be a superuser to proceed
   (sel :many User))
 
 
@@ -30,30 +26,30 @@
 
 
 (defendpoint GET "/:id" [id]
-  ;; user must be getting their own details OR they must be a superuser to proceed
-  (check-403 (or (= id *current-user-id*) (:is_superuser @*current-user*)))
+  (check-self-or-superuser id)        ; user must be getting their own details OR they must be a superuser to proceed
   (check-404 (sel :one User :id id)))
 
 
-(defendpoint PUT "/:id" [id :as {{:keys [email] :as body} :body}]
-  {email [Required Email]}
-  ;; user must be getting their own details OR they must be a superuser to proceed
-  (check-403 (or (= id *current-user-id*) (:is_superuser @*current-user*)))
-  ;; can't change email if it's already taken BY ANOTHER ACCOUNT
-  (when id
-    (check-400 (not (exists? User :email email :id [not= id]))))
-  (check-500 (->> (select-non-nil-keys body :email :first_name :last_name)
-                  (mapply upd User id)))
+(defendpoint PUT "/:id" [id :as {{:keys [email first_name last_name] :as body} :body}]
+  {email      [Required Email]
+   first_name NonEmptyString
+   last_name  NonEmptyString}
+  (check-self-or-superuser id)
+  (check-400 (not (exists? User :email email :id [not= id]))) ; can't change email if it's already taken BY ANOTHER ACCOUNT
+  (check-500 (upd-non-nil-keys User id
+                               :email email
+                               :first_name first_name
+                               :last_name last_name))
   (sel :one User :id id))
 
 
 (defendpoint PUT "/:id/password" [id :as {{:keys [password old_password]} :body}]
-  {password [Required ComplexPassword]
+  {password     [Required ComplexPassword]
    old_password Required}
-  (check-403 (or (= id *current-user-id*)
-                 (:is_superuser @*current-user*)))
+  (check-self-or-superuser id)
   (let-404 [user (sel :one [User :password_salt :password] :id id)]
-    (check (creds/bcrypt-verify (str (:password_salt user) old_password) (:password user)) [400 "password mismatch"]))
+    (check (creds/bcrypt-verify (str (:password_salt user) old_password) (:password user))
+      [400 "password mismatch"]))
   (set-user-password id password)
   (sel :one User :id id))