[Toucan 2 Prep] Replace the `IObjectPermissions` protocol with multimethods (#24917)

* [Toucan 2 prep] Don't invoke Toucan models as functions

* Some fixes

* Test fixes

* Test fix

* [Toucan 2 prep] Don't call `type` or `class` on Toucan models

* Test fixes

* More test fixes 

* Replace perms protocol with multimethods; derive models from perms policy keywords

* Test fixes 

* Appease Eastwood

* Fix errors now that App has been merged in

* Empty commit to trigger CI

.clj-kondo/config.edn

@@ -417,6 +417,7 @@
    metabase.models.action/QueryAction                                                     hooks.metabase.models.disallow-invoking-model/hook
    metabase.models.action/HTTPAction                                                      hooks.metabase.models.disallow-invoking-model/hook
    metabase.models.activity/Activity                                                      hooks.metabase.models.disallow-invoking-model/hook
+   metabase.models.app/App                                                                hooks.metabase.models.disallow-invoking-model/hook
    metabase.models.application-permissions-revision/ApplicationPermissionsRevision        hooks.metabase.models.disallow-invoking-model/hook
    metabase.models.bookmark/BookmarkOrdering                                              hooks.metabase.models.disallow-invoking-model/hook
    metabase.models.bookmark/CardBookmark                                                  hooks.metabase.models.disallow-invoking-model/hook
@@ -466,6 +467,7 @@
    metabase.models.user/User                                                              hooks.metabase.models.disallow-invoking-model/hook
    metabase.models.view-log/ViewLog                                                       hooks.metabase.models.disallow-invoking-model/hook
    metabase.models/Activity                                                               hooks.metabase.models.disallow-invoking-model/hook
+   metabase.models/App                                                                    hooks.metabase.models.disallow-invoking-model/hook
    metabase.models/ApplicationPermissionsRevision                                         hooks.metabase.models.disallow-invoking-model/hook
    metabase.models/BookmarkOrdering                                                       hooks.metabase.models.disallow-invoking-model/hook
    metabase.models/Card                                                                   hooks.metabase.models.disallow-invoking-model/hook

.clj-kondo/hooks/metabase/models/disallow_type_or_class.clj

@@ -4,6 +4,7 @@
 (def known-models
   '#{Action
      Activity
+     App
      ApplicationPermissionsRevision
      BookmarkOrdering
      Card

enterprise/backend/src/metabase_enterprise/sandbox/models/group_table_access_policy.clj

@@ -22,6 +22,10 @@
 
 (models/defmodel GroupTableAccessPolicy :group_table_access_policy)
 
+;;; only admins can work with GTAPs
+(derive GroupTableAccessPolicy ::mi/read-policy.superuser)
+(derive GroupTableAccessPolicy ::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*
@@ -138,11 +142,4 @@
    models/IModelDefaults
    {:types      (constantly {:attribute_remappings ::attribute-remappings})
     :pre-insert pre-insert
-    :pre-update pre-update})
-
-  ;; only admins can work with GTAPs
-  mi/IObjectPermissions
-  (merge
-   mi/IObjectPermissionsDefaults
-   {:can-read?  mi/superuser?
-    :can-write? mi/superuser?}))
+    :pre-update pre-update}))

shared/test/metabase/types_test.cljc

@@ -66,6 +66,7 @@
           :cljs @(#'clojure.core/get-global-hierarchy))
        :parents
        keys
+       (filter keyword?)
        (filter #(= (namespace %) (name keyword-namespace)))))
 
 (defn- test-derived-from [a-type {:keys [required disallowed]}]

src/metabase/api/app.clj

@@ -34,7 +34,7 @@
    nav_items (s/maybe [(s/maybe su/Map)])}
   (api/write-check Collection (db/select-one-field :collection_id App :id app-id))
   (db/update! App app-id (select-keys body [:dashboard_id :options :nav_items]))
-  (hydrate-details (App app-id)))
+  (hydrate-details (db/select-one App :id app-id)))
 
 ;; TODO handle personal collections, see collection/personal-collection-with-ui-details
 (api/defendpoint GET "/"

src/metabase/models/activity.clj

@@ -56,12 +56,15 @@
   models/IModel
   (merge models/IModelDefaults
          {:types      (constantly {:details :json, :topic :keyword})
-          :pre-insert pre-insert})
-  mi/IObjectPermissions
-  (merge mi/IObjectPermissionsDefaults
-         {:can-read?  (partial can-? mi/can-read?)
-          ;; TODO - when do people *write* activities?
-          :can-write? (partial can-? mi/can-write?)}))
+          :pre-insert pre-insert}))
+
+(defmethod mi/can-read? Activity
+  [& args]
+  (apply can-? mi/can-read? args))
+
+(defmethod mi/can-write? Activity
+  [& args]
+  (apply can-? mi/can-write? args))
 
 
 ;;; ------------------------------------------------------ Etc. ------------------------------------------------------

src/metabase/models/app.clj

 (ns metabase.models.app
-  (:require [metabase.models.interface :as mi]
-            [metabase.models.permissions :as perms]
+  (:require [metabase.models.permissions :as perms]
             [metabase.models.serialization.hash :as serdes.hash]
             [metabase.util :as u]
             [toucan.db :as db]
@@ -8,7 +7,10 @@
 
 (models/defmodel App :app)
 
-(u/strict-extend (class App)
+;;; You can read/write an App if you can read/write its Collection
+(derive App ::perms/use-parent-collection-perms)
+
+(u/strict-extend #_{:clj-kondo/ignore [:metabase/disallow-class-or-type-on-model]} (class App)
   models/IModel
   (merge models/IModelDefaults
          {:types (constantly {:options :json
@@ -16,10 +18,6 @@
           :properties (constantly {:timestamped? true
                                    :entity_id    true})})
 
-  ;; You can read/write an App if you can read/write its Collection
-  mi/IObjectPermissions
-  perms/IObjectPermissionsForParentCollection
-
   ;; Should not be needed as every app should have an entity_id, but currently it's
   ;; necessary to satisfy metabase-enterprise.models.entity-id-test/comprehensive-identity-hash-test.
   serdes.hash/IdentityHashable

src/metabase/models/card.clj

@@ -27,6 +27,9 @@
 
 (models/defmodel Card :report_card)
 
+;;; You can read/write a Card if you can read/write its parent Collection
+(derive Card ::perms/use-parent-collection-perms)
+
 
 ;;; -------------------------------------------------- Hydration --------------------------------------------------
 
@@ -324,10 +327,6 @@
           :pre-delete     pre-delete
           :post-select    public-settings/remove-public-uuid-if-public-sharing-is-disabled})
 
-  ;; You can read/write a Card if you can read/write its parent Collection
-  mi/IObjectPermissions
-  perms/IObjectPermissionsForParentCollection
-
   revision/IRevisioned
   (assoc revision/IRevisionedDefaults
          :serialize-instance serialize-instance)

src/metabase/models/collection.clj

@@ -39,6 +39,10 @@
 
 (models/defmodel Collection :collection)
 
+(doto Collection
+  (derive ::mi/read-policy.full-perms-for-perms-set)
+  (derive ::mi/write-policy.full-perms-for-perms-set))
+
 (def AuthorityLevel
   "Schema for valid collection authority levels"
   (s/maybe (s/enum "official")))
@@ -860,8 +864,8 @@
 
 ;;; -------------------------------------------------- IModel Impl ---------------------------------------------------
 
-(defn perms-objects-set
-  "Return the required set of permissions to `read-or-write` `collection-or-id`."
+;;; Return the required set of permissions to `read-or-write` `collection-or-id`.
+(defmethod mi/perms-objects-set Collection
   [collection-or-id read-or-write]
   (let [collection (if (integer? collection-or-id)
                      (db/select-one [Collection :id :namespace] :id (collection-or-id))
@@ -897,11 +901,6 @@
           :post-insert    post-insert
           :pre-update     pre-update
           :pre-delete     pre-delete})
-  mi/IObjectPermissions
-  (merge mi/IObjectPermissionsDefaults
-         {:can-read?         (partial mi/current-user-has-full-permissions? :read)
-          :can-write?        (partial mi/current-user-has-full-permissions? :write)
-          :perms-objects-set perms-objects-set})
 
   serdes.hash/IdentityHashable
   {:identity-hash-fields (constantly [:name :namespace parent-identity-hash])})

src/metabase/models/collection/root.clj

 (ns metabase.models.collection.root
-  (:require [metabase.models.interface :as mi]
+  (:require [metabase.models.dispatch :as models.dispatch]
+            [metabase.models.interface :as mi]
             [metabase.models.permissions :as perms]
             [metabase.public-settings.premium-features :as premium-features]
             [metabase.util :as u]
@@ -16,7 +17,17 @@
 
 (p.types/defrecord+ RootCollection [])
 
-(defn- has-perms? [collection read-or-write]
+(doto RootCollection
+  (derive ::mi/read-policy.full-perms-for-perms-set)
+  (derive ::mi/write-policy.full-perms-for-perms-set))
+
+(extend-protocol models.dispatch/Model
+  RootCollection
+  (model [_this]
+    RootCollection))
+
+(defmethod mi/perms-objects-set RootCollection
+  [collection read-or-write]
   {:pre [(map? collection)]}
   ;; HACK Collections in the "snippets" namespace have no-op permissions unless EE enhancements are enabled
   (if (and (= (u/qualified-name (:namespace collection)) "snippets")
@@ -30,14 +41,7 @@
   models/IModel
   (merge
    models/IModelDefaults
-   {:types {:type :keyword}})
-
-  mi/IObjectPermissions
-  (merge
-   mi/IObjectPermissionsDefaults
-   {:perms-objects-set has-perms?
-    :can-read?         (partial mi/current-user-has-full-permissions? :read)
-    :can-write?        (partial mi/current-user-has-full-permissions? :write)}))
+   {:types {:type :keyword}}))
 
 (def ^RootCollection root-collection
   "Special placeholder object representing the Root Collection, which isn't really a real Collection."

src/metabase/models/dashboard.clj

@@ -10,7 +10,6 @@
             [metabase.models.collection :as collection :refer [Collection]]
             [metabase.models.dashboard-card :as dashboard-card :refer [DashboardCard]]
             [metabase.models.field-values :as field-values]
-            [metabase.models.interface :as mi]
             [metabase.models.params :as params]
             [metabase.models.permissions :as perms]
             [metabase.models.pulse :as pulse :refer [Pulse]]
@@ -65,6 +64,9 @@
 (comment moderation/keep-me)
 
 (models/defmodel Dashboard :report_dashboard)
+
+(derive Dashboard ::perms/use-parent-collection-perms)
+
 ;;; ----------------------------------------------- Entity & Lifecycle -----------------------------------------------
 
 (defn- pre-delete [dashboard]
@@ -141,10 +143,6 @@
           :post-update post-update
           :post-select public-settings/remove-public-uuid-if-public-sharing-is-disabled})
 
-  ;; You can read/write a Dashboard if you can read/write its parent Collection
-  mi/IObjectPermissions
-  perms/IObjectPermissionsForParentCollection
-
   serdes.hash/IdentityHashable
   {:identity-hash-fields (constantly [:name (serdes.hash/hydrated-hash :collection)])})
 

src/metabase/models/dashboard_card.clj

@@ -19,11 +19,15 @@
 
 (models/defmodel DashboardCard :report_dashboardcard)
 
+(doto DashboardCard
+  (derive ::mi/read-policy.full-perms-for-perms-set)
+  (derive ::mi/write-policy.full-perms-for-perms-set))
+
 (declare series)
 
-(defn- perms-objects-set
-  "Return the set of permissions required to `read-or-write` this DashboardCard. If `:card` and `:series` are already
-  hydrated this method doesn't need to make any DB calls."
+;;; Return the set of permissions required to `read-or-write` this DashboardCard. If `:card` and `:series` are already
+;;; hydrated this method doesn't need to make any DB calls.
+(defmethod mi/perms-objects-set DashboardCard
   [dashcard read-or-write]
   (let [card   (or (:card dashcard)
                    (db/select-one [Card :dataset_query] :id (u/the-id (:card_id dashcard))))
@@ -48,11 +52,6 @@
                                     :visualization_settings :visualization-settings})
           :pre-insert  pre-insert
           :post-select #(set/rename-keys % {:sizex :sizeX, :sizey :sizeY})})
-  mi/IObjectPermissions
-  (merge mi/IObjectPermissionsDefaults
-         {:perms-objects-set perms-objects-set
-          :can-read?         (partial mi/current-user-has-full-permissions? :read)
-          :can-write?        (partial mi/current-user-has-full-permissions? :write)})
 
   serdes.hash/IdentityHashable
   {:identity-hash-fields (constantly [(serdes.hash/hydrated-hash :card)

src/metabase/models/database.clj

@@ -22,6 +22,10 @@
 
 (models/defmodel Database :metabase_database)
 
+(doto Database
+  (derive ::mi/read-policy.partial-perms-for-perms-set)
+  (derive ::mi/write-policy.full-perms-for-perms-set))
+
 (defn- schedule-tasks!
   "(Re)schedule sync operation tasks for `database`. (Existing scheduled tasks will be deleted first.)"
   [database]
@@ -184,7 +188,8 @@
       handle-secrets-changes
       (assoc :initial_sync_status "incomplete")))
 
-(defn- perms-objects-set [{db-id :id} read-or-write]
+(defmethod mi/perms-objects-set Database
+  [{db-id :id} read-or-write]
   #{(case read-or-write
       :read  (perms/data-perms-path db-id)
       :write (perms/db-details-write-perms-path db-id))})
@@ -206,11 +211,6 @@
           :pre-insert     pre-insert
           :pre-update     pre-update
           :pre-delete     pre-delete})
-  mi/IObjectPermissions
-  (merge mi/IObjectPermissionsDefaults
-         {:perms-objects-set perms-objects-set
-          :can-read?         (partial mi/current-user-has-partial-permissions? :read)
-          :can-write?        (partial mi/current-user-has-full-permissions? :write)})
 
   serdes.hash/IdentityHashable
   {:identity-hash-fields (constantly [:name :engine])})

src/metabase/models/dispatch.clj

@@ -2,6 +2,7 @@
   "Helpers to assist in the transition to Toucan 2. Once we switch to Toucan 2 this stuff shouldn't be needed, but we
   can update this namespace instead of having to update code all over the place."
   (:require
+   [potemkin :as p]
    [schema.core :as s]
    [toucan.db :as db]
    [toucan.models :as models]))
@@ -33,19 +34,26 @@
             (instance-of? model x))
           (format "instance of a %s" (name model))))
 
-(defn model
-  "Given either a Toucan model or a Toucan instance, return the Toucan model. Otherwise return `nil`."
-  [x]
-  (cond
-    (models/model? x)
-    x
+(p/defprotocol+ Model
+  (model [this]
+    "Given either a Toucan model or a Toucan instance, return the Toucan model. Otherwise return `nil`."))
+
+(extend-protocol Model
+  Object
+  (model [this]
+    (cond
+      (models/model? this)
+      this
+
+      (toucan-instance? this)
+      (let [model-symb (symbol (name this))]
+        (db/resolve-model model-symb))
 
-    (toucan-instance? x)
-    (let [model-symb (symbol (name x))]
-      (db/resolve-model model-symb))
+      :else
+      nil))
 
-    :else
-    nil))
+  nil
+  (model [_this] nil))
 
 (defn instance
   "Create a new instance of Toucan `model` with a map `m`.

src/metabase/models/field.clj

@@ -67,6 +67,10 @@
 
 (models/defmodel Field :metabase_field)
 
+(doto Field
+  (derive ::mi/read-policy.partial-perms-for-perms-set)
+  (derive ::mi/write-policy.full-perms-for-perms-set))
+
 (defn- hierarchy-keyword-in [column-name & {:keys [ancestor-types]}]
   (fn [k]
     (when-let [k (keyword k)]
@@ -148,11 +152,10 @@
        (perms-objects-set* db-id schema table-id read-or-write)))
    :ttl/threshold 5000))
 
-(defn- perms-objects-set
-  "Calculate set of permissions required to access a Field. For the time being permissions to access a Field are the
-   same as permissions to access its parent Table."
+;;; Calculate set of permissions required to access a Field. For the time being permissions to access a Field are the
+;;; same as permissions to access its parent Table.
+(defmethod mi/perms-objects-set Field
   [{table-id :table_id, {db-id :db_id, schema :schema} :table} read-or-write]
-  {:arglists '([field read-or-write])}
   (if db-id
     ;; if Field already has a hydrated `:table`, then just use that to generate perms set (no DB calls required)
     (perms-objects-set* db-id schema table-id read-or-write)
@@ -192,12 +195,6 @@
           :properties     (constantly {:timestamped? true})
           :pre-insert     pre-insert})
 
-  mi/IObjectPermissions
-  (merge mi/IObjectPermissionsDefaults
-         {:perms-objects-set perms-objects-set
-          :can-read?         (partial mi/current-user-has-partial-permissions? :read)
-          :can-write?        (partial mi/current-user-has-full-permissions? :write)})
-
   serdes.hash/IdentityHashable
   {:identity-hash-fields (constantly [:name (serdes.hash/hydrated-hash :table)])})
 

src/metabase/models/interface.clj

@@ -14,16 +14,14 @@
             [metabase.util.encryption :as encryption]
             [metabase.util.i18n :refer [trs tru]]
             [potemkin :as p]
-            [potemkin.types :as p.types]
             [schema.core :as s]
             [taoensso.nippy :as nippy]
+            [toucan.db :as db]
             [toucan.models :as models])
   (:import [java.io BufferedInputStream ByteArrayInputStream DataInputStream]
            java.sql.Blob
            java.util.zip.GZIPInputStream))
 
-(comment models.dispatch/keep-me)
-
 (p/import-vars
  [models.dispatch
   toucan-instance?
@@ -260,76 +258,87 @@
 ;;; |                                             New Permissions Stuff                                              |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
-(p.types/defprotocol+ IObjectPermissions
-  "Methods for determining whether the current user has read/write permissions for a given object. See documentation
-  for [[metabase.models.permissions]] for a high-level overview of the Metabase permissions system."
+(defn- dispatch-on-model [x & _args]
+  (models.dispatch/model x))
+
+(defmulti perms-objects-set
+  "Return a set of permissions object paths that a user must have access to in order to access this object. This should be
+  something like
+
+    #{\"/db/1/schema/public/table/20/\"}
+
+  `read-or-write` will be either `:read` or `:write`, depending on which permissions set we're fetching (these will be
+  the same sets for most models; they can ignore this param)."
+  {:arglists '([instance read-or-write])}
+  dispatch-on-model)
 
-  (perms-objects-set [instance ^clojure.lang.Keyword read-or-write]
-    "Return a set of permissions object paths that a user must have access to in order to access this object. This
-    should be something like #{\"/db/1/schema/public/table/20/\"}. `read-or-write` will be either `:read` or `:write`,
-    depending on which permissions set we're fetching (these will be the same sets for most models; they can ignore
-    this param).")
+(defmethod perms-objects-set :default
+  [_instance _read-or-write]
+  nil)
 
-  (can-read? [instance] [entity ^Integer id]
-    "Return whether `*current-user*` has *read* permissions for an object. You should typically use one of these
-    implementations:
+(defmulti can-read?
+  "Return whether [[metabase.api.common/*current-user*]] has *read* permissions for an object. You should typically use
+  one of these implementations:
 
   *  `(constantly true)`
   *  `superuser?`
-  *  `(partial current-user-has-full-permissions? :read)` (you must also implement `perms-objects-set` to use this)
-  *  `(partial current-user-has-partial-permissions? :read)` (you must also implement `perms-objects-set` to use
-     this)")
+  *  `(partial current-user-has-full-permissions? :read)` (you must also implement [[perms-objects-set]] to use this)
+  *  `(partial current-user-has-partial-permissions? :read)` (you must also implement [[perms-objects-set]] to use
+     this)"
+  {:arglists '([instance] [model pk])}
+  dispatch-on-model)
 
-  (^{:hydrate :can_write} can-write? [instance] [entity ^Integer id]
-   "Return whether `*current-user*` has *write* permissions for an object. You should typically use one of these
-   implmentations:
+(defmulti can-write?
+  "Return whether [[metabase.api.common/*current-user*]] has *write* permissions for an object. You should typically use
+  one of these implementations:
 
   *  `(constantly true)`
   *  `superuser?`
-  *  `(partial current-user-has-full-permissions? :write)` (you must also implement `perms-objects-set` to use this)
-  *  `(partial current-user-has-partial-permissions? :write)` (you must also implement `perms-objects-set` to use
-      this)")
-
-  (^{:added "0.32.0"} can-create? [entity m]
-    "NEW! Check whether or not current user is allowed to CREATE a new instance of `entity` with properties in map
+  *  `(partial current-user-has-full-permissions? :write)` (you must also implement [[perms-objects-set]] to use this)
+  *  `(partial current-user-has-partial-permissions? :write)` (you must also implement [[perms-objects-set]] to use
+      this)"
+  {:arglists '([instance] [model pk]), :hydrate :can_write}
+  dispatch-on-model)
+
+(defmulti can-create?
+  "NEW! Check whether or not current user is allowed to CREATE a new instance of `model` with properties in map
     `m`.
 
-  Because this method was added YEARS after `can-read?` and `can-write?`, most models do not have an implementation
+  Because this method was added YEARS after [[can-read?]] and [[can-write?]], most models do not have an implementation
   for this method, and instead `POST` API endpoints themselves contain the appropriate permissions logic (ick).
-  Implement this method as you come across models that are missing it.")
-
-  (^{:added "0.36.0"} can-update? [instance changes]
-   "NEW! Check whether or not the current user is allowed to update an object and by updating properties to values in
-   the `changes` map. This is equivalent to checking whether you're allowed to perform `(toucan.db/update! entity id
-   changes)`.
-
-  This method is appropriate for powering `PUT` API endpoints. Like `can-create?` this method was added YEARS after
-  most of the current API endpoints were written, so it is used in very few places, and this logic is determined
-  ad-hoc in the API endpoints themselves. Use this method going forward!"))
-
-(def IObjectPermissionsDefaults
-  "Default implementations for `IObjectPermissions`."
-  {:perms-objects-set
-   (constantly nil)
-
-   :can-create?
-   (fn [entity _]
-     (throw
-      (NoSuchMethodException.
-       (str (format "%s does not yet have an implementation for `can-create?`. " (name entity))
-            "Please consider adding one. See dox for `can-create?` for more details."))))
-
-   :can-update?
-   (fn
-     [instance _]
-     (throw
-      (NoSuchMethodException.
-       (str (format "%s does not yet have an implementation for `can-update?`. " (name instance))
-            "Please consider adding one. See dox for `can-update?` for more details."))))})
+  Implement this method as you come across models that are missing it."
+  {:added "0.32.0", :arglists '([model m])}
+  dispatch-on-model)
+
+(defmethod can-create? :default
+  [model _m]
+  (throw
+   (NoSuchMethodException.
+    (str (format "%s does not yet have an implementation for [[can-create?]]. " (name model))
+         "Please consider adding one. See dox for [[can-create?]] for more details."))))
+
+(defmulti can-update?
+  "NEW! Check whether or not the current user is allowed to update an object and by updating properties to values in
+   the `changes` map. This is equivalent to checking whether you're allowed to perform
+
+    (toucan.db/update! model id changes)
+
+  This method is appropriate for powering `PUT` API endpoints. Like [[can-create?]] this method was added YEARS after
+  most of the current API endpoints were written, so it is used in very few places, and this logic is determined ad-hoc
+  in the API endpoints themselves. Use this method going forward!"
+  {:added "0.36.0", :arglists '([instance changes])}
+  dispatch-on-model)
+
+(defmethod can-update? :default
+  [instance _changes]
+  (throw
+   (NoSuchMethodException.
+    (str (format "%s does not yet have an implementation for `can-update?`. " (name (models.dispatch/model instance)))
+         "Please consider adding one. See dox for `can-update?` for more details."))))
 
 (defn superuser?
-  "Is `*current-user*` is a superuser? Ignores args.
-   Intended for use as an implementation of `can-read?` and/or `can-write?`."
+  "Is [[metabase.api.common/*current-user*]] is a superuser? Ignores args. Intended for use as an implementation
+  of [[can-read?]] and/or [[can-write?]]."
   [& _]
   @(requiring-resolve 'metabase.api.common/*is-superuser?*))
 
@@ -340,9 +349,9 @@
   (contains? (current-user-permissions-set) "/"))
 
 (defn- check-perms-with-fn
-  ([fn-symb read-or-write entity object-id]
+  ([fn-symb read-or-write a-model object-id]
    (or (current-user-has-root-permissions?)
-       (check-perms-with-fn fn-symb read-or-write (entity object-id))))
+       (check-perms-with-fn fn-symb read-or-write (db/select-one a-model (models/primary-key a-model) object-id))))
 
   ([fn-symb read-or-write object]
    (and object
@@ -354,20 +363,73 @@
      (u/prog1 (f (current-user-permissions-set) perms-set)
        (log/tracef "Perms check: %s -> %s" (pr-str (list fn-symb (current-user-permissions-set) perms-set)) <>)))))
 
-(def ^{:arglists '([read-or-write entity object-id] [read-or-write object] [perms-set])}
+(def ^{:arglists '([read-or-write model object-id] [read-or-write object] [perms-set])}
   current-user-has-full-permissions?
-  "Implementation of `can-read?`/`can-write?` for the old permissions system. `true` if the current user has *full*
-  permissions for the paths returned by its implementation of `perms-objects-set`. (`read-or-write` is either `:read` or
-  `:write` and passed to `perms-objects-set`; you'll usually want to partially bind it in the implementation map)."
+  "Implementation of [[can-read?]]/[[can-write?]] for the old permissions system. `true` if the current user has *full*
+  permissions for the paths returned by its implementation of [[perms-objects-set]]. (`read-or-write` is either `:read` or
+  `:write` and passed to [[perms-objects-set]]; you'll usually want to partially bind it in the implementation map)."
   (partial check-perms-with-fn 'metabase.models.permissions/set-has-full-permissions-for-set?))
 
-(def ^{:arglists '([read-or-write entity object-id] [read-or-write object] [perms-set])}
+(def ^{:arglists '([read-or-write model object-id] [read-or-write object] [perms-set])}
   current-user-has-partial-permissions?
-  "Implementation of `can-read?`/`can-write?` for the old permissions system. `true` if the current user has *partial*
-  permissions for the paths returned by its implementation of `perms-objects-set`. (`read-or-write` is either `:read` or
-  `:write` and passed to `perms-objects-set`; you'll usually want to partially bind it in the implementation map)."
+  "Implementation of [[can-read?]]/[[can-write?]] for the old permissions system. `true` if the current user has *partial*
+  permissions for the paths returned by its implementation of [[perms-objects-set]]. (`read-or-write` is either `:read` or
+  `:write` and passed to [[perms-objects-set]]; you'll usually want to partially bind it in the implementation map)."
   (partial check-perms-with-fn 'metabase.models.permissions/set-has-partial-permissions-for-set?))
 
+(defmethod can-read? ::read-policy.always-allow
+  ([_instance]
+   true)
+  ([_model _pk]
+   true))
+
+(defmethod can-write? ::write-policy.always-allow
+  ([_instance]
+   true)
+  ([_model _pk]
+   true))
+
+(defmethod can-read? ::read-policy.partial-perms-for-perms-set
+  ([instance]
+   (current-user-has-partial-permissions? :read instance))
+  ([model pk]
+   (current-user-has-partial-permissions? :read model pk)))
+
+(defmethod can-read? ::read-policy.full-perms-for-perms-set
+  ([instance]
+   (current-user-has-full-permissions? :read instance))
+  ([model pk]
+   (current-user-has-full-permissions? :read model pk)))
+
+(defmethod can-write? ::write-policy.partial-perms-for-perms-set
+  ([instance]
+   (current-user-has-partial-permissions? :write instance))
+  ([model pk]
+   (current-user-has-partial-permissions? :write model pk)))
+
+(defmethod can-write? ::write-policy.full-perms-for-perms-set
+  ([instance]
+   (current-user-has-full-permissions? :write instance))
+  ([model pk]
+   (current-user-has-full-permissions? :write model pk)))
+
+(defmethod can-read? ::read-policy.superuser
+  ([_instance]
+   (superuser?))
+  ([_model _pk]
+   (superuser?)))
+
+(defmethod can-write? ::write-policy.superuser
+  ([_instance]
+   (superuser?))
+  ([_model _pk]
+   (superuser?)))
+
+(defmethod can-create? ::create-policy.superuser
+  [_model _m]
+  (superuser?))
+
+
 ;;;; redefs
 
 ;;; swap out [[models/defmodel]] with a special magical version that avoids redefining stuff if the definition has not

src/metabase/models/metric.clj

@@ -19,6 +19,11 @@
 
 (models/defmodel Metric :metric)
 
+(doto Metric
+  (derive ::mi/read-policy.full-perms-for-perms-set)
+  (derive ::mi/write-policy.superuser)
+  (derive ::mi/create-policy.superuser))
+
 (defn- pre-update [{:keys [creator_id id], :as updates}]
   (u/prog1 updates
     ;; throw an Exception if someone tries to update creator_id
@@ -26,7 +31,8 @@
       (when (not= creator_id (db/select-one-field :creator_id Metric :id id))
         (throw (UnsupportedOperationException. (tru "You cannot update the creator_id of a Metric.")))))))
 
-(defn- perms-objects-set [metric read-or-write]
+(defmethod mi/perms-objects-set Metric
+  [metric read-or-write]
   (let [table (or (:table metric)
                   (db/select-one ['Table :db_id :schema :id] :id (u/the-id (:table_id metric))))]
     (mi/perms-objects-set table read-or-write)))
@@ -39,15 +45,6 @@
     :properties (constantly {:timestamped? true
                              :entity_id    true})
     :pre-update pre-update})
-  mi/IObjectPermissions
-  (merge
-   mi/IObjectPermissionsDefaults
-   {:perms-objects-set perms-objects-set
-    :can-read?         (partial mi/current-user-has-full-permissions? :read)
-    ;; for the time being you need to be a superuser in order to create or update Metrics because the UI for doing so
-    ;; is only exposed in the admin panel
-    :can-write?        mi/superuser?
-    :can-create?       mi/superuser?})
 
   serdes.hash/IdentityHashable
   {:identity-hash-fields (constantly [:name (serdes.hash/hydrated-hash :table)])})

src/metabase/models/metric_important_field.clj

@@ -6,11 +6,11 @@
 
 (models/defmodel MetricImportantField :metric_important_field)
 
+(doto MetricImportantField
+  (derive ::mi/read-policy.always-allow)
+  (derive ::mi/write-policy.superuser))
+
 (u/strict-extend #_{:clj-kondo/ignore [:metabase/disallow-class-or-type-on-model]} (class MetricImportantField)
   models/IModel
   (merge models/IModelDefaults
-         {:types (constantly {:definition :json})})
-  mi/IObjectPermissions
-  (merge mi/IObjectPermissionsDefaults
-         {:can-read?  (constantly true)
-          :can-write? mi/superuser?}))
+         {:types (constantly {:definition :json})}))

src/metabase/models/moderation_review.clj

 (ns metabase.models.moderation-review
   "TODO -- this should be moved to `metabase-enterprise.content-management.models.moderation-review` since it's a
   premium-only model."
-  (:require [metabase.models.interface :as mi]
-            [metabase.models.permissions :as perms]
+  (:require [metabase.models.permissions :as perms]
             [metabase.moderation :as moderation]
             [metabase.util :as u]
             [metabase.util.schema :as su]
@@ -29,15 +28,14 @@
 
 (models/defmodel ModerationReview :moderation_review)
 
+;;; TODO: this is wrong, but what should it be?
+(derive ModerationReview ::perms/use-parent-collection-perms)
+
 (u/strict-extend #_{:clj-kondo/ignore [:metabase/disallow-class-or-type-on-model]} (class ModerationReview)
   models/IModel
   (merge models/IModelDefaults
          {:properties (constantly {:timestamped? true})
-          :types      (constantly {:moderated_item_type :keyword})})
-
-  ;; Todo: this is wrong, but what should it be?
-  mi/IObjectPermissions
-  perms/IObjectPermissionsForParentCollection)
+          :types      (constantly {:moderated_item_type :keyword})}))
 
 (def max-moderation-reviews
   "The amount of moderation reviews we will keep on hand."

src/metabase/models/native_query_snippet.clj

@@ -41,28 +41,36 @@
     :pre-insert pre-insert
     :pre-update pre-update})
 
-  mi/IObjectPermissions
-  (merge
-   mi/IObjectPermissionsDefaults
-   {:can-read?   snippet.perms/can-read?
-    :can-write?  snippet.perms/can-write?
-    :can-create? snippet.perms/can-create?
-    :can-update? snippet.perms/can-update?})
-
   serdes.hash/IdentityHashable
   {:identity-hash-fields (constantly [:name (serdes.hash/hydrated-hash :collection)])})
 
+(defmethod mi/can-read? NativeQuerySnippet
+  [& args]
+  (apply snippet.perms/can-read? args))
+
+(defmethod mi/can-write? NativeQuerySnippet
+  [& args]
+  (apply snippet.perms/can-write? args))
+
+(defmethod mi/can-create? NativeQuerySnippet
+  [& args]
+  (apply snippet.perms/can-create? args))
+
+(defmethod mi/can-update? NativeQuerySnippet
+  [& args]
+  (apply snippet.perms/can-update? args))
+
 
 ;;; ---------------------------------------------------- Schemas -----------------------------------------------------
 
 (def NativeQuerySnippetName
   "Schema checking that snippet names do not include \"}\" or start with spaces."
   (su/with-api-error-message
-   (s/pred (every-pred
-            string?
-            (complement #(boolean (re-find #"^\s+" %)))
-            (complement #(boolean (re-find #"}" %)))))
-   (deferred-tru "snippet names cannot include '}' or start with spaces")))
+    (s/pred (every-pred
+             string?
+             (complement #(boolean (re-find #"^\s+" %)))
+             (complement #(boolean (re-find #"}" %)))))
+    (deferred-tru "snippet names cannot include '}' or start with spaces")))
 
 ;;; ------------------------------------------------- Serialization --------------------------------------------------