Skip to content
Snippets Groups Projects
Unverified Commit 32403f0d authored by dpsutton's avatar dpsutton Committed by GitHub
Browse files

Don't use persisted model tables for segmented users (#25347)

* Don't use persisted model tables for segmented users

This actually isn't a bug, but due to very subtle and arbitrary reasons.

For background about why we need to ensure this never happens, we cannot
use persisted models when sandboxing is at play. A simple example is as
follows: make a model on a products table that does not select the
category. Have a sandbox on category such that someone can only see
products of category "Gizmo". the model lacks the category column but we
insert a where clause that still works. When the model is persisted,
there is no category column in the underlying table so sandboxing cannot
possibly work: the data necessary to filter is no longer associated with
the rest of the data in the model.

The fix for this is quite simple: in
`metabase.query-processor.middleware.fetch-source-query` we only splice
in the persisted query if the user is not a segmented user (product name
for sandboxing).

```clojure
(and persisted-info/*allow-persisted-substitution*
     (not (segmented-user?))  ;; <----- new check
     (:active card)
     (:definition card)
     (:query_hash card)
     (= (:query_hash card) (persisted-info/query-hash (:dataset_query card)))
     (= (:definition card) (persisted-info/metadata->definition (:result_metadata card)
                                                                (:table_name card)))
     (= (:state card) "persisted"))
```

Technical details about why this bug did not manifest

When swapping out a card__<id> to a source query, if its a model we will
see if it is persisted, and if so, we will use the native sql to select
from the persisted table. It does this by adding the native sql at a key
called `:persisted-info/native` and a middleware
`#'qp.persistence/substitute-persisted-query` walks the query replacing
the query with the native:

```clojure
;; metabase.query-processor.middleware.persistence

    (mbql.u/replace query
      (x :guard (every-pred map? :persisted-info/native))
      {:native (:persisted-info/native x)})
```

There is also a middleware that walks through the query looking for
tables with gtaps on them and replacing them. By change, the sandboxing
middleware runs immediately before the substitute-persisted middleware!

```clojure
   ;; literally the previous middleware
   (resolve 'ee.sandbox.rows/apply-sandboxing)
   #'qp.persistence/substitute-persisted-query
```

If you swap the order of these two sandboxing is broken. As is, it
"works" but not by design, just by happenstance. The sandboxing
middleware just did not know that the `:persisted-info/native` key meant
that a native query was to be substituted. In the reverse order, the
native query is already substituted and there is no change for the
sandboxing to occur.

The obvious fix is to ensure that we never even attempt to use the
persisted tables and that is what this PR does.

* Eastwood doesn't like shadowing like this

* Rearrange check order for tests

`segmented-user?` throws if there is no bound user. A test in
`fetch-source-query-test` was failing because there was no user bound,
but it wasn't attempting to swap out a persisted table, it just didn't
expect to need a user.

Moving it lower lets it short circuit on other bits that are bound to
fail (definition, query_hash, etc) requiring persistence before we check
for a bound user
parent b18d53e8
No related branches found
No related tags found
No related merge requests found
Loading
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment