diff --git a/src/metabase/api/search.clj b/src/metabase/api/search.clj
index b1b9c42894b4d043be4e4ebb306b086418cccf87..2f8cfe422a0ea58c7b91386508d8dca13c0b5ee1 100644
--- a/src/metabase/api/search.clj
+++ b/src/metabase/api/search.clj
@@ -16,6 +16,7 @@
[dashboard :refer [Dashboard]]
[dashboard-favorite :refer [DashboardFavorite]]
[metric :refer [Metric]]
+ [permissions :as perms]
[pulse :refer [Pulse]]
[segment :refer [Segment]]
[table :refer [Table]]]
@@ -27,13 +28,12 @@
(def ^:private SearchContext
"Map with the various allowed search parameters, used to construct the SQL query"
- {:search-string (s/maybe su/NonBlankString)
- :archived? s/Bool
- ;; either `:all` or a set of IDs and/or the string `root`
- :visible-collections coll/VisibleCollections})
+ {:search-string (s/maybe su/NonBlankString)
+ :archived? s/Bool
+ :current-user-perms #{perms/UserPath}})
(def ^:private searchable-models
- [Card Dashboard Pulse Collection Segment Metric])
+ [Card Dashboard Pulse Collection Segment Metric Table])
(def ^:private SearchableModel
(apply s/enum searchable-models))
@@ -128,6 +128,17 @@
[_]
(into default-columns table-columns))
+(defmethod columns-for-model (class Table)
+ [_]
+ [:id
+ :name
+ :description
+ [:id :table_id]
+ [:db_id :database_id]
+ [:schema :table_schema]
+ [:name :table_name]
+ [:description :table_description]])
+
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Shared Query Logic |
@@ -191,9 +202,22 @@
[model :- SearchableModel]
[[model (model->alias model)]])
+(defmulti ^:private archived-where-clause
+ {:arglists '([model archived?])}
+ (fn [model _] (class model)))
+
+(defmethod archived-where-clause :default
+ [model archived?]
+ [:= (hsql/qualify (model->alias model) :archived) archived?])
+
+;; Table has an `:active` flag, but no `:archived` flag; never return inactive Tables
+(defmethod archived-where-clause (class Table)
+ [model _]
+ [:= (hsql/qualify (model->alias model) :active) true])
+
(s/defn ^:private base-where-clause-for-model :- [(s/one (s/enum :and :=) "type") s/Any]
[model :- SearchableModel, {:keys [search-string archived?]} :- SearchContext]
- (let [archived-clause [:= (hsql/qualify (model->alias model) :archived) archived?]
+ (let [archived-clause (archived-where-clause model archived?)
search-string-clause (when (seq search-string)
[:like
(hsql/call :lower (hsql/qualify (model->alias model) :name))
@@ -213,10 +237,12 @@
(s/defn ^:private add-collection-join-and-where-clauses
"Add a `WHERE` clause to the query to only return Collections the Current User has access to; join against Collection
so we can return its `:name`."
- [honeysql-query :- su/Map, collection-id-column :- s/Keyword, {:keys [visible-collections]} :- SearchContext]
- (let [honeysql-query (h/merge-where
- honeysql-query
- (coll/visible-collection-ids->honeysql-filter-clause collection-id-column visible-collections))]
+ [honeysql-query :- su/Map, collection-id-column :- s/Keyword, {:keys [current-user-perms]} :- SearchContext]
+ (let [visible-collections (coll/permissions-set->visible-collection-ids current-user-perms)
+ collection-filter-clause (coll/visible-collection-ids->honeysql-filter-clause
+ collection-id-column
+ visible-collections)
+ honeysql-query (h/merge-where honeysql-query collection-filter-clause)]
;; add a JOIN against Collection *unless* the source table is already Collection
(cond-> honeysql-query
(not= collection-id-column :collection.id)
@@ -232,7 +258,7 @@
{:arglists '([model search-context])}
(fn [model _] (class model)))
-(s/defmethod ^:private search-query-for-model (class Card)
+(s/defmethod search-query-for-model (class Card)
[_ search-ctx :- SearchContext]
(-> (base-query-for-model Card search-ctx)
(h/left-join [CardFavorite :fave]
@@ -241,12 +267,12 @@
[:= :fave.owner_id api/*current-user-id*]])
(add-collection-join-and-where-clauses :card.collection_id search-ctx)))
-(s/defmethod ^:private search-query-for-model (class Collection)
+(s/defmethod search-query-for-model (class Collection)
[_ search-ctx :- SearchContext]
(-> (base-query-for-model Collection search-ctx)
(add-collection-join-and-where-clauses :collection.id search-ctx)))
-(s/defmethod ^:private search-query-for-model (class Dashboard)
+(s/defmethod search-query-for-model (class Dashboard)
[_ search-ctx :- SearchContext]
(-> (base-query-for-model Dashboard search-ctx)
(h/left-join [DashboardFavorite :fave]
@@ -255,7 +281,7 @@
[:= :fave.user_id api/*current-user-id*]])
(add-collection-join-and-where-clauses :dashboard.collection_id search-ctx)))
-(s/defmethod ^:private search-query-for-model (class Pulse)
+(s/defmethod search-query-for-model (class Pulse)
[_ search-ctx :- SearchContext]
;; Pulses don't currently support being archived, omit if archived is true
(-> (base-query-for-model Pulse search-ctx)
@@ -263,21 +289,40 @@
;; We don't want alerts included in pulse results
(h/merge-where [:= :alert_condition nil])))
-(s/defmethod ^:private search-query-for-model (class Metric)
+(s/defmethod search-query-for-model (class Metric)
[_ search-ctx :- SearchContext]
(-> (base-query-for-model Metric search-ctx)
(h/left-join [Table :table] [:= :metric.table_id :table.id])))
-(s/defmethod ^:private search-query-for-model (class Segment)
+(s/defmethod search-query-for-model (class Segment)
[_ search-ctx :- SearchContext]
(-> (base-query-for-model Segment search-ctx)
(h/left-join [Table :table] [:= :segment.table_id :table.id])))
+(s/defmethod search-query-for-model (class Table)
+ [_ {:keys [current-user-perms], :as search-ctx} :- SearchContext]
+ (when (seq current-user-perms)
+ (let [base-query (base-query-for-model Table search-ctx)]
+ (if (contains? current-user-perms "/")
+ base-query
+ {:select (:select base-query)
+ :from [[(merge
+ base-query
+ {:select [:id :schema :db_id :name :description
+ [(hx/concat "/db/" :db_id "/" :schema "/" :id "/") :path]]})
+ :table]]
+ :where (cons
+ :or
+ (for [path current-user-perms]
+ [:like :path (str path "%")]))}))))
+
(s/defn ^:private search
"Builds a search query that includes all of the searchable entities and runs it"
[search-ctx :- SearchContext]
- (for [row (db/query {:union-all (for [model searchable-models]
- (search-query-for-model model search-ctx))})]
+ (for [row (db/query {:union-all (for [model searchable-models
+ :let [query (search-query-for-model model search-ctx)]
+ :when (seq query)]
+ query)})]
;; MySQL returns `:favorite` as `1` or `0` so convert those to boolean as needed
(update row :favorite (fn [favorite]
(if (integer? favorite)
@@ -291,19 +336,15 @@
(s/defn ^:private make-search-context :- SearchContext
[search-string :- (s/maybe su/NonBlankString), archived-string :- (s/maybe su/BooleanString)]
- {:search-string search-string
- :archived? (Boolean/parseBoolean archived-string)
- :visible-collections (coll/permissions-set->visible-collection-ids @api/*current-user-permissions-set*)})
+ {:search-string search-string
+ :archived? (Boolean/parseBoolean archived-string)
+ :current-user-perms @api/*current-user-permissions-set*})
(api/defendpoint GET "/"
"Search Cards, Dashboards, Collections and Pulses for the substring `q`."
[q archived]
{q (s/maybe su/NonBlankString)
archived (s/maybe su/BooleanString)}
- (let [{:keys [visible-collections] :as search-ctx} (make-search-context q archived)]
- ;; Throw if the user doesn't have access to any collections
- (api/check-403 (or (= :all visible-collections)
- (seq visible-collections)))
- (search search-ctx)))
+ (search (make-search-context q archived)))
(api/define-routes)
diff --git a/src/metabase/models/permissions.clj b/src/metabase/models/permissions.clj
index 1bd0252a16559e1098579deadf3f9b41fe057905..6c0bb2d1d05a537345163543d92f741e09d8f860 100644
--- a/src/metabase/models/permissions.clj
+++ b/src/metabase/models/permissions.clj
@@ -41,27 +41,27 @@
;;; --------------------------------------------------- Validation ---------------------------------------------------
-(def ^:private ^:const valid-object-path-patterns
- [#"^/db/(\d+)/$" ; permissions for the entire DB -- native and all schemas
- #"^/db/(\d+)/native/$" ; permissions to create new native queries for the DB
- #"^/db/(\d+)/schema/$" ; permissions for all schemas in the DB
- #"^/db/(\d+)/schema/([^/]*)/$" ; permissions for a specific schema
- #"^/db/(\d+)/schema/([^/]*)/table/(\d+)/$" ; FULL permissions for a specific table
- #"^/db/(\d+)/schema/([^/]*)/table/(\d+)/read/$" ; Permissions to fetch the Metadata for a specific Table
- #"^/db/(\d+)/schema/([^/]*)/table/(\d+)/query/$" ; Permissions to run any sort of query against a Table
- #"^/db/(\d+)/schema/([^\\/]*)/table/(\d+)/query/segmented/$" ; Permissions to run a query against a Table using GTAP
- #"^/collection/(\d+)/$" ; readwrite permissions for a collection
- #"^/collection/(\d+)/read/$" ; read permissions for a collection
- #"^/collection/root/$" ; readwrite permissions for the 'Root' Collection (things with `nil` collection_id)
- #"^/collection/root/read/$"]) ; read permissions for the 'Root' Collection
+(def ^:private valid-object-path-patterns
+ [#"^/db/\d+/$" ; permissions for the entire DB -- native and all schemas
+ #"^/db/\d+/native/$" ; permissions to create new native queries for the DB
+ #"^/db/\d+/schema/$" ; permissions for all schemas in the DB
+ #"^/db/\d+/schema/[^/]*/$" ; permissions for a specific schema
+ #"^/db/\d+/schema/[^/]*/table/\d+/$" ; FULL permissions for a specific table
+ #"^/db/\d+/schema/[^/]*/table/\d+/read/$" ; Permissions to fetch the Metadata for a specific Table
+ #"^/db/\d+/schema/[^/]*/table/\d+/query/$" ; Permissions to run any sort of query against a Table
+ #"^/db/\d+/schema/[^/]*/table/\d+/query/segmented/$" ; Permissions to run a query against a Table using GTAP
+ #"^/collection/\d+/$" ; readwrite permissions for a collection
+ #"^/collection/\d+/read/$" ; read permissions for a collection
+ #"^/collection/root/$" ; readwrite permissions for the 'Root' Collection (things with `nil` collection_id)
+ #"^/collection/root/read/$"]) ; read permissions for the 'Root' Collection
(defn valid-object-path?
- "Does OBJECT-PATH follow a known, allowed format to an *object*?
- (The root path, \"/\", is not considered an object; this returns `false` for it)."
+ "Does `object-path` follow a known, allowed format to an *object*? (The root path, \"/\", is not considered an object;
+ this returns `false` for it)."
^Boolean [^String object-path]
(boolean (when (and (string? object-path)
(seq object-path))
- (some (u/rpartial re-matches object-path)
+ (some #(re-matches % object-path)
valid-object-path-patterns))))
(def ObjectPath
@@ -75,7 +75,7 @@
"Valid user permissions path."))
(defn- assert-not-admin-group
- "Check to make sure the `:group_id` for PERMISSIONS entry isn't the admin group."
+ "Check to make sure the `:group_id` for `permissions` entry isn't the admin group."
[{:keys [group_id]}]
(when (and (= group_id (:id (group/admin)))
(not *allow-admin-permissions-changes*))
@@ -83,14 +83,14 @@
{:status-code 400}))))
(defn- assert-valid-object
- "Check to make sure the value of `:object` for PERMISSIONS entry is valid."
+ "Check to make sure the value of `:object` for `permissions` entry is valid."
[{:keys [object]}]
(when (and object
(not (valid-object-path? object))
(or (not= object "/")
(not *allow-root-entries*)))
- (throw (ui18n/ex-info (tru "Invalid permissions object path: ''{0}''." object)
- {:status-code 400}))))
+ (throw (ex-info (str (tru "Invalid permissions object path: ''{0}''." object))
+ {:status-code 400, :path object}))))
(defn- assert-valid-metabot-permissions
"MetaBot permissions can only be created for Collections, since MetaBot can only interact with objects that are always
@@ -98,11 +98,11 @@
[{:keys [object group_id]}]
(when (and (= group_id (:id (group/metabot)))
(not (str/starts-with? object "/collection/")))
- (throw (ui18n/ex-info (tru "MetaBot can only have Collection permissions.")
+ (throw (ex-info (str (tru "MetaBot can only have Collection permissions."))
{:status-code 400}))))
(defn- assert-valid
- "Check to make sure this PERMISSIONS entry is something that's allowed to be saved (i.e. it has a valid `:object`
+ "Check to make sure this `permissions` entry is something that's allowed to be saved (i.e. it has a valid `:object`
path and it's not for the admin group)."
[permissions]
(doseq [f [assert-not-admin-group
@@ -156,12 +156,12 @@
;;; -------------------------------------------- Permissions Checking Fns --------------------------------------------
(defn is-permissions-for-object?
- "Does PERMISSIONS-PATH grant *full* access for OBJECT-PATH?"
+ "Does `permissions`-PATH grant *full* access for OBJECT-PATH?"
[permissions-path object-path]
(str/starts-with? object-path permissions-path))
(defn is-partial-permissions-for-object?
- "Does PERMISSIONS-PATH grant access full access for OBJECT-PATH *or* for a descendant of OBJECT-PATH?"
+ "Does `permissions`-PATH grant access full access for OBJECT-PATH *or* for a descendant of OBJECT-PATH?"
[permissions-path object-path]
(or (is-permissions-for-object? permissions-path object-path)
(str/starts-with? permissions-path object-path)))
diff --git a/src/metabase/models/table.clj b/src/metabase/models/table.clj
index df4c4f1e7d4af45f0d11d276f351e074b63defe9..b271565cae27e97f779e40c0965c4737e703bb19 100644
--- a/src/metabase/models/table.clj
+++ b/src/metabase/models/table.clj
@@ -64,7 +64,7 @@
;;; --------------------------------------------------- Hydration ----------------------------------------------------
(defn fields
- "Return the `FIELDS` belonging to a single TABLE."
+ "Return the Fields belonging to a single `table`."
[{:keys [id]}]
(db/select Field
:table_id id
@@ -73,17 +73,17 @@
{:order-by [[:position :asc] [:name :asc]]}))
(defn metrics
- "Retrieve the `Metrics` for a single TABLE."
+ "Retrieve the Metrics for a single `table`."
[{:keys [id]}]
(retrieve-metrics id :all))
(defn segments
- "Retrieve the `Segments` for a single TABLE."
+ "Retrieve the Segments for a single `table`."
[{:keys [id]}]
(retrieve-segments id :all))
(defn field-values
- "Return the `FieldValues` for all `Fields` belonging to a single TABLE."
+ "Return the FieldValues for all Fields belonging to a single `table`."
{:hydrate :field_values, :arglists '([table])}
[{:keys [id]}]
(let [field-ids (db/select-ids Field
@@ -94,7 +94,7 @@
(db/select-field->field :field_id :values FieldValues, :field_id [:in field-ids]))))
(defn pk-field-id
- "Return the ID of the primary key `Field` for TABLE."
+ "Return the ID of the primary key `Field` for `table`."
{:hydrate :pk_field, :arglists '([table])}
[{:keys [id]}]
(db/select-one-id Field
diff --git a/test/metabase/api/search_test.clj b/test/metabase/api/search_test.clj
index 7f4d4b75c4671f50681088510c800d975f2d0e7b..229441f5bf284b5cd99d0ab327bb320e8bd45f66 100644
--- a/test/metabase/api/search_test.clj
+++ b/test/metabase/api/search_test.clj
@@ -9,6 +9,7 @@
[collection :as coll :refer [Collection]]
[dashboard :refer [Dashboard]]
[dashboard-favorite :refer [DashboardFavorite]]
+ [database :refer [Database]]
[metric :refer [Metric]]
[permissions :as perms]
[permissions-group :as group :refer [PermissionsGroup]]
@@ -276,3 +277,29 @@
(and (= id (u/get-id pulse))
(= "pulse" model)))
((test-users/user->client :crowberto) :get 200 "search")))))
+
+;; You should see TABLES in the search results!
+(defn- default-table-search-row [table-name]
+ (merge
+ default-search-row
+ {:name table-name, :table_name table-name :table_id true, :archived nil, :model "table", :database_id true}))
+
+(let [table-name (tu/random-name)]
+ (expect
+ #{(default-table-search-row table-name)}
+ (tt/with-temp Table [table {:name table-name}]
+ (search-request :crowberto :q table-name))))
+
+(let [table-name (tu/random-name)]
+ (expect
+ #{(default-table-search-row table-name)}
+ (tt/with-temp Table [table {:name table-name}]
+ (search-request :rasta :q table-name))))
+
+;; you should not be able to see a Table if the current user doesn't have permissions for that Table
+(expect
+ #{}
+ (tt/with-temp* [Database [{db-id :id}]
+ Table [table {:db_id db-id}]]
+ (perms/revoke-permissions! (group/all-users) db-id)
+ (search-request :rasta :q (:name table))))
diff --git a/test/metabase/test/util.clj b/test/metabase/test/util.clj
index f9fa12d4a0a07c1668c7b1a6dd8390e5cc28c409..6d5a1319d24dea28c97ab45fa84383db09aceeda 100644
--- a/test/metabase/test/util.clj
+++ b/test/metabase/test/util.clj
@@ -66,7 +66,7 @@
~actual))
(defn- random-uppercase-letter []
- (char (+ (rand-int 26) (int \A))))
+ (char (+ (int \A) (rand-int 26))))
(defn random-name
"Generate a random string of 20 uppercase letters."