Skip to content

Commit

Permalink
Migrate collection and permissions namespaces to Malli (metabase#32647)
Browse files Browse the repository at this point in the history 
* Migrate collection and permissions namespaces to Malli

* Fix docstring

* Fix Kondo warnings
  • Loading branch information
@camsaul
camsaul authored Aug 3, 2023
1 parent f538747 commit 096f6cf
Show file tree
Hide file tree
Showing 123 changed files with 1,885 additions and 1,842 deletions.
64 changes: 34 additions & 30 deletions .clj-kondo/config.edn
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,39 @@
{:config-paths ["macros"]
:linters
{:unresolved-symbol
{:aliased-namespace-symbol {:level :warning}
:invalid-arity {:skip-args [metabase.mbql.util.match/match]} ; TODO (cam): can we fix these?
:keyword-binding {:level :warning}
:main-without-gen-class {:level :warning}
:misplaced-docstring {:level :warning}
:missing-body-in-when {:level :warning}
:missing-docstring {:level :warning}
:missing-else-branch {:level :warning}
:namespace-name-mismatch {:level :warning}
:non-arg-vec-return-type-hint {:level :warning}
:reduce-without-init {:level :warning}
:redundant-fn-wrapper {:level :warning}
:refer-all {:level :warning, :exclude [clojure.test]}
:single-key-in {:level :warning}
:unused-referred-var {:exclude {compojure.core [GET DELETE POST PUT]}}
:use {:level :warning}
:warn-on-reflection {:level :warning}

;;
;; disabled linters
;;

:unexpected-recur {:level :off} ; TODO (cam): I think we just need to tell it how to handle MBQL match and we can enable this?
:used-underscored-binding {:level :off} ; false positives: see https://github.com/clj-kondo/clj-kondo/issues/2152

;;
;; TODO (cam): here are some more linters we should experiment with enabling -- some might be useful.
;;

;; :docstring-leading-trailing-whitespace {:level :warning}
;; :docstring-no-summary {:level :warning}
;; :shadowed-var {:level :warning}

:unresolved-symbol
{:exclude
[instaparse.core/transform
(cljs.test/is [=? malli=])
Expand All @@ -25,33 +58,6 @@
(taoensso.nippy/extend-freeze)
(taoensso.nippy/extend-thaw)]}

:refer-all {:level :warning
:exclude [clojure.test]}
;; TODO (cam): I think we just need to tell it how to handle MBQL match and we can enable this?
:unexpected-recur {:level :off}
;; TODO (cam): can we fix these?
:unused-referred-var {:exclude {compojure.core [GET DELETE POST PUT]}}
:missing-else-branch {:level :warning}
:misplaced-docstring {:level :warning}
:non-arg-vec-return-type-hint {:level :warning}
:missing-body-in-when {:level :warning}
:missing-docstring {:level :warning}
;; TODO (braden): This is useful, but it doesn't grok eg. enterprise/backend/src
:namespace-name-mismatch {:level :off}
:use {:level :warning}
:redundant-fn-wrapper {:level :warning}
:invalid-arity {:skip-args [metabase.mbql.util.match/match]}
:warn-on-reflection {:level :warning}
;; TODO (cam): here are some more linters we should experiment with enabling -- some might be useful.
;; :docstring-no-summary {:level :warning}
;; :docstring-leading-trailing-whitespace {:level :warning}
;; :reduce-without-init {:level :warning}
;; :used-underscored-binding {:level :warning}
;; :single-key-in {:level :warning}
;; :keyword-binding {:level :warning}
;; :main-without-gen-class {:level :warning}
;; :shadowed-var {:level :warning}

:deprecated-var
{:exclude
{metabase.cmd/dump {:namespaces ["metabase\\.cmd-test" "metabase-enterprise\\.serialization\\.cmd-test"]}
Expand Down Expand Up @@ -738,9 +744,7 @@
metabase-lib
{:linters
{:docstring-leading-trailing-whitespace {:level :warning}
:reduce-without-init {:level :warning}
:used-underscored-binding {:level :warning}
:single-key-in {:level :warning}
:keyword-binding {:level :warning}
:shadowed-var {:level :warning}
:metabase/deftest-not-marked-parallel-or-synchronized {:level :warning}
Expand Down
1 change: 0 additions & 1 deletion deps.edn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,6 @@
:exec-fn dev.h2-shell/shell
:java-opts ["-Dfile.encoding=UTF-8"]}

;; clojure -M:generate-automagic-dashboards-pot
:generate-automagic-dashboards-pot
{:main-opts ["-m" "metabase.automagic-dashboards.rules"]}

Expand Down
21 changes: 10 additions & 11 deletions enterprise/backend/src/metabase_enterprise/advanced_permissions/models/permissions.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@
(:require
[metabase.models.permissions :as perms]
[metabase.public-settings.premium-features :as premium-features]
#_{:clj-kondo/ignore [:deprecated-namespace]}
[metabase.util.schema :as su]
[schema.core :as s]))
[metabase.util.malli :as mu]
[metabase.util.malli.schema :as ms]))

;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Shared Util Functions |
Expand Down Expand Up @@ -84,15 +83,15 @@
(doseq [[table-id table-perms] new-schema-perms]
(update-table-download-permissions! group-id db-id schema table-id table-perms)))))

(s/defn update-db-download-permissions!
(mu/defn update-db-download-permissions!
"Update the download permissions graph for a database.
This mostly works similar to [[metabase.models.permission/update-db-data-access-permissions!]], with a few key
differences:
- Permissions have three levels: full, limited, and none.
- Native query download permissions are fully inferred from the non-native download permissions. For more details,
see the docstring for [[metabase.models.permissions/update-native-download-permissions!]]."
[group-id :- su/IntGreaterThanZero db-id :- su/IntGreaterThanZero new-download-perms :- perms/DownloadPermissionsGraph]
[group-id :- ms/PositiveInt db-id :- ms/PositiveInt new-download-perms :- perms/DownloadPermissionsGraph]
(when-not (premium-features/enable-advanced-permissions?)
(throw (perms/ee-permissions-exception :download)))
(when-let [schemas (:schemas new-download-perms)]
Expand Down Expand Up @@ -156,9 +155,9 @@
(doseq [[table-id table-perms] new-schema-perms]
(update-table-data-model-permissions! group-id db-id schema table-id table-perms)))))

(s/defn update-db-data-model-permissions!
(mu/defn update-db-data-model-permissions!
"Update the data model permissions graph for a database."
[group-id :- su/IntGreaterThanZero db-id :- su/IntGreaterThanZero new-data-model-perms :- perms/DataModelPermissionsGraph]
[group-id :- ms/PositiveInt db-id :- ms/PositiveInt new-data-model-perms :- perms/DataModelPermissionsGraph]
(when-not (premium-features/enable-advanced-permissions?)
(throw (perms/ee-permissions-exception :data-model)))
(when-let [schemas (:schemas new-data-model-perms)]
Expand Down Expand Up @@ -187,9 +186,9 @@
[db-id]
(perms/feature-perms-path :details :yes db-id))

(s/defn update-db-details-permissions!
(mu/defn update-db-details-permissions!
"Update the DB details permissions for a database."
[group-id :- su/IntGreaterThanZero db-id :- su/IntGreaterThanZero new-perms :- perms/DetailsPermissions]
[group-id :- ms/PositiveInt db-id :- ms/PositiveInt new-perms :- perms/DetailsPermissions]
(when-not (premium-features/enable-advanced-permissions?)
(throw (perms/ee-permissions-exception :details)))
(case new-perms
Expand All @@ -201,9 +200,9 @@
:no
(revoke-permissions! :details :yes group-id db-id)))

(s/defn update-db-execute-permissions!
(mu/defn update-db-execute-permissions!
"Update the DB details permissions for a database."
[group-id :- su/IntGreaterThanZero db-id :- su/IntGreaterThanZero new-perms :- perms/ExecutePermissions]
[group-id :- ms/PositiveInt db-id :- ms/PositiveInt new-perms :- perms/ExecutePermissions]
(when-not (premium-features/enable-advanced-permissions?)
(throw (perms/ee-permissions-exception :execute)))
(revoke-permissions! :execute :all group-id db-id)
Expand Down
4 changes: 3 additions & 1 deletion enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@
(ldap-group-membership-filter))]
(assoc user-info :attributes (syncable-user-attributes result)))))

(defenterprise-schema fetch-or-create-user! :- (mi/InstanceOf User)
;;; for some reason the `:clj-kondo/ignore` doesn't work inside of [[defenterprise-schema]]
#_{:clj-kondo/ignore [:deprecated-var]}
(defenterprise-schema fetch-or-create-user! :- (mi/InstanceOf:Schema User)
"Using the `user-info` (from `find-user`) get the corresponding Metabase user, creating it if necessary."
:feature :sso-ldap
[{:keys [first-name last-name email groups attributes], :as user-info} :- EEUserInfo
Expand Down
60 changes: 28 additions & 32 deletions enterprise/backend/src/metabase_enterprise/sandbox/api/table.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,68 +4,64 @@
[compojure.core :refer [GET]]
[metabase.api.common :as api]
[metabase.api.table :as api.table]
[metabase.db.query :as mdb.query]
[metabase.mbql.util :as mbql.u]
[metabase.models.card :refer [Card]]
[metabase.models.interface :as mi]
[metabase.models.permissions :as perms]
[metabase.models.table :as table :refer [Table]]
[metabase.util :as u]
#_{:clj-kondo/ignore [:deprecated-namespace]}
[metabase.util.schema :as su]
[schema.core :as s]
[metabase.util.malli :as mu]
[metabase.util.malli.schema :as ms]
[toucan2.core :as t2]))

(s/defn ^:private find-gtap-question :- (s/maybe (mi/InstanceOf Card))
(mu/defn ^:private find-gtap-question :- [:maybe (mi/InstanceOf Card)]
"Find the associated GTAP question (if there is one) for the given `table-or-table-id` and
`user-or-user-id`. Returns nil if no question was found."
[table-or-table-id user-or-user-id]
(some->> (mdb.query/query
{:select [:c.id :c.dataset_query]
:from [[:sandboxes]]
:join [[:permissions_group_membership :pgm] [:= :sandboxes.group_id :pgm.group_id]
[:report_card :c] [:= :c.id :sandboxes.card_id]]
:where [:and
[:= :sandboxes.table_id (u/the-id table-or-table-id)]
[:= :pgm.user_id (u/the-id user-or-user-id)]]})
first
(mi/do-after-select Card)))
(t2/select-one Card
{:select [:c.id :c.dataset_query]
:from [[:sandboxes]]
:join [[:permissions_group_membership :pgm] [:= :sandboxes.group_id :pgm.group_id]
[:report_card :c] [:= :c.id :sandboxes.card_id]]
:where [:and
[:= :sandboxes.table_id (u/the-id table-or-table-id)]
[:= :pgm.user_id (u/the-id user-or-user-id)]]}))

(s/defn only-segmented-perms? :- s/Bool
(mu/defn only-sandboxed-perms? :- :boolean
"Returns true if the user has only segemented and not full table permissions. If the user has full table permissions
we wouldn't want to apply this segment filtering."
[table :- (mi/InstanceOf Table)]
(and
(not (perms/set-has-full-permissions? @api/*current-user-permissions-set*
(perms/table-query-path table)))
(perms/table-query-path table)))
(perms/set-has-full-permissions? @api/*current-user-permissions-set*
(perms/table-segmented-query-path table))))
(perms/table-sandboxed-query-path table))))

(s/defn ^:private query->fields-ids :- (s/maybe [s/Int])
[{{{:keys [fields]} :query} :dataset_query}]
(mu/defn ^:private query->fields-ids :- [:maybe [:sequential :int]]
[{{{:keys [fields]} :query} :dataset_query} :- [:maybe :map]]
(mbql.u/match fields [:field (id :guard integer?) _] id))

(defn- maybe-filter-fields [table query-metadata-response]
;; If we have segmented permissions and the associated GTAP limits the fields returned, we need make sure the
;; If we have sandboxed permissions and the associated GTAP limits the fields returned, we need make sure the
;; query_metadata endpoint also excludes any fields the GTAP query would exclude
(if-let [gtap-field-ids (and (only-segmented-perms? table)
(if-let [gtap-field-ids (and (only-sandboxed-perms? table)
(seq (query->fields-ids (find-gtap-question table api/*current-user-id*))))]
(update query-metadata-response :fields #(filter (comp (set gtap-field-ids) u/the-id) %))
query-metadata-response))

#_{:clj-kondo/ignore [:deprecated-var]}
(api/defendpoint-schema GET "/:id/query_metadata"
"This endpoint essentially acts as a wrapper for the OSS version of this route. When a user has segmented permissions
(api/defendpoint GET "/:id/query_metadata"
"This endpoint essentially acts as a wrapper for the OSS version of this route. When a user has sandboxed permissions
that only gives them access to a subset of columns for a given table, those inaccessable columns should also be
excluded from what is show in the query builder. When the user has full permissions (or no permissions) this route
doesn't add/change anything from the OSS version. See the docs on the OSS version of the endpoint for more
information."
[id include_sensitive_fields include_hidden_fields include_editable_data_model]
{include_sensitive_fields (s/maybe su/BooleanString)
include_hidden_fields (s/maybe su/BooleanString)
include_editable_data_model (s/maybe su/BooleanString)}
{id ms/PositiveInt
include_sensitive_fields [:maybe ms/BooleanString]
include_hidden_fields [:maybe ms/BooleanString]
include_editable_data_model [:maybe ms/BooleanString]}
(let [table (api/check-404 (t2/select-one Table :id id))
segmented-perms? (only-segmented-perms? table)
sandboxed-perms? (only-sandboxed-perms? table)
thunk (fn []
(maybe-filter-fields
table
Expand All @@ -74,9 +70,9 @@
{:include-sensitive-fields? include_sensitive_fields
:include-hidden-fields? include_hidden_fields
:include-editable-data-model? include_editable_data_model})))]
;; if the user has segmented perms, temporarily upgrade their perms to read perms for the Table so they can see
;; the metadata
(if segmented-perms?
;; if the user has sandboxed perms, temporarily upgrade their perms to read perms for the Table so they can see the
;; metadata
(if sandboxed-perms?
(binding [api/*current-user-permissions-set* (atom
(set/union
@api/*current-user-permissions-set*
Expand Down
2 changes: 1 addition & 1 deletion enterprise/backend/src/metabase_enterprise/sandbox/api/user.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,6 @@
:when (seq login-attributes)]
(set (keys login-attributes)))
;; combine all the sets of attribute keys into a single set
(reduce set/union)))
(reduce set/union #{})))

(api/define-routes api/+check-superuser)
29 changes: 7 additions & 22 deletions enterprise/backend/src/metabase_enterprise/sandbox/models/group_table_access_policy.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,16 @@
[metabase.server.middleware.session :as mw.session]
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
[metabase.util.log :as log]
#_{:clj-kondo/ignore [:deprecated-namespace]}
[metabase.util.schema :as su]
[metabase.util.malli :as mu]
[metabase.util.malli.schema :as ms]
[methodical.core :as methodical]
[schema.core :as s]
[toucan2.core :as t2]))

(set! *warn-on-reflection* true)

(def GroupTableAccessPolicy
"Used to be the toucan1 model name defined using [[toucan.models/defmodel]], now it's a reference to the toucan2 model name.
We'll keep this till we replace all the symbols in our codebase."
"Used to be the toucan1 model name defined using [[toucan.models/defmodel]], now it's a reference to the toucan2 model
name. We'll keep this till we replace all the symbols in our codebase."
:model/GroupTableAccessPolicy)

(methodical/defmethod t2/table-name :model/GroupTableAccessPolicy [_model] :sandboxes)
Expand All @@ -39,19 +37,6 @@
(derive ::mi/read-policy.superuser)
(derive ::mi/write-policy.superuser))

;; This guard is to make sure this file doesn't get compiled twice when building the uberjar -- that will totally
;; screw things up because Toucan models use Potemkin `defrecord+` under the hood.
(when *compile-files*
(defonce previous-compilation-trace (atom nil))
(when @previous-compilation-trace
(log/info "THIS FILE HAS ALREADY BEEN COMPILED!!!!!")
(log/info "This compilation trace:")
((requiring-resolve 'clojure.pprint/pprint) (vec (.getStackTrace (Thread/currentThread))))
(log/info "Previous compilation trace:")
((requiring-resolve 'clojure.pprint/pprint) @previous-compilation-trace)
(throw (ex-info "THIS FILE HAS ALREADY BEEN COMPILED!!!!!" {})))
(reset! previous-compilation-trace (vec (.getStackTrace (Thread/currentThread)))))

(defn- normalize-attribute-remapping-targets [attribute-remappings]
(m/map-vals
mbql.normalize/normalize
Expand Down Expand Up @@ -92,7 +77,7 @@
:expected table-col-base-type
:actual (:base_type col)}))))))

(s/defn check-columns-match-table
(mu/defn check-columns-match-table
"Make sure the result metadata data columns for the Card associated with a GTAP match up with the columns in the Table
that's getting GTAPped. It's ok to remove columns, but you cannot add new columns. The base types of the Card
columns can derive from the respective base types of the columns in the Table itself, but you cannot return an
Expand All @@ -104,7 +89,7 @@
(when-let [result-metadata (t2/select-one-fn :result_metadata Card :id card-id)]
(check-columns-match-table table-id result-metadata))))

([table-id :- su/IntGreaterThanZero result-metadata-columns]
([table-id :- ms/PositiveInt result-metadata-columns]
;; prevent circular refs
(classloader/require 'metabase.query-processor)
(let [table-cols (table-field-names->cols table-id)]
Expand Down Expand Up @@ -147,7 +132,7 @@
id
(u/select-keys-when sandbox :present #{:card_id :attribute_remappings})))
(t2/select-one GroupTableAccessPolicy :id id))
(let [expected-permission-path (perms/table-segmented-query-path (:table_id sandbox))]
(let [expected-permission-path (perms/table-sandboxed-query-path (:table_id sandbox))]
(when-let [permission-path-id (t2/select-one-fn :id Permissions :object expected-permission-path)]
(first (t2/insert-returning-instances! GroupTableAccessPolicy (assoc sandbox :permission_id permission-path-id))))))))

Expand Down
2 changes: 1 addition & 1 deletion enterprise/backend/src/metabase_enterprise/sandbox/models/params/field_values.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
;; slight optimization: for the `field-id->field-values` version we can batched hydrate `:table` to avoid having to
;; make a bunch of calls to fetch Table. For `get-or-create-field-values` we don't hydrate `:table` so we can fall
;; back to fetching it manually with `field/table`
(table/only-segmented-perms? (or table (field/table field))))
(table/only-sandboxed-perms? (or table (field/table field))))

(defn- table-id->gtap
"Find the GTAP for current user that apply to table `table-id`."
Expand Down
Loading

0 comments on commit 096f6cf

Please sign in to comment.