You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "sha174n (via GitHub)" <gi...@apache.org> on 2026/09/08 11:46:07 UTC
[PR] fix(rls): match the RLS predicate lookup on case-normalized table identifiers [superset]
sha174n opened a new pull request, #43993:
URL: https://github.com/apache/superset/pull/43993
On databases that fold unquoted identifiers (for example PostgreSQL lowercases them), a table referenced with different letter-casing resolves to the same physical table. `get_predicates_for_table` compared the referenced table name to the registered dataset name case-sensitively, so a mismatched-case reference matched no dataset and had no row-level-security predicates injected for that reference.
This adds a case-insensitive fallback in the RLS predicate lookup, gated on whether the engine folds unquoted identifiers (determined via sqlglot dialect normalization). The fallback runs only when the exact match misses and only accepts a single unambiguous dataset, so the exact (case-sensitive) match always takes precedence and existing matches are unchanged. On case-sensitive engines the exact match is kept as-is.
Adds a unit test for the engine-folding gate and an integration test asserting that a case-mismatched table reference yields the same RLS predicates as the exact-case reference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(rls): match the RLS predicate lookup on case-normalized table identifiers [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43993:
URL: https://github.com/apache/superset/pull/43993#discussion_r3962941274
##########
superset/utils/rls.py:
##########
@@ -132,29 +159,64 @@ def get_predicates_for_table(
if exclude_dataset_id is not None:
filters.append(SqlaTable.id != exclude_dataset_id)
- dataset = (
- db.session.query(SqlaTable)
- .filter(and_(*filters, SqlaTable.schema == table.schema))
- .one_or_none()
+ def match(schema_predicate: Any) -> SqlaTable | None:
+ query = db.session.query(SqlaTable).filter(and_(*filters, schema_predicate))
+ if not fold:
+ return query.one_or_none()
+ # 0, 1 or "ambiguous" is all the folded lookup needs to tell apart
+ matches = query.limit(2).all()
+ return matches[0] if len(matches) == 1 else None
Review Comment:
When the folded lookup sees two datasets that differ only by case, this returns `None`, so `get_predicates_for_table()` silently runs the SQL without RLS. Could this fail closed instead? For example, a user who can create a virtual `SALES` dataset beside an RLS-protected `sales` dataset can make `SELECT * FROM Sales` take this ambiguous path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(rls): match the RLS predicate lookup on case-normalized table identifiers [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43993:
URL: https://github.com/apache/superset/pull/43993#issuecomment-5584843563
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43993?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `72.72727%` with `6 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 66.32%. Comparing base ([`c4e145b`](https://app.codecov.io/gh/apache/superset/commit/c4e145b071f8ec708ca16485852de3dd8bec9117?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`0f9cf60`](https://app.codecov.io/gh/apache/superset/commit/0f9cf60ebf794c81cc1843a98293f6e6303739b5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/43993?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/utils/rls.py](https://app.codecov.io/gh/apache/superset/pull/43993?src=pr&el=tree&filepath=superset%2Futils%2Frls.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvcmxzLnB5) | 72.72% | [4 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/superset/pull/43993?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (c4e145b) and HEAD (0f9cf60). Click for more details.
>
> <details><summary>HEAD has 19 uploads less than BASE</summary>
>
>| Flag | BASE (c4e145b) | HEAD (0f9cf60) |
>|------|------|------|
>|python|16|3|
>|presto|3|1|
>|hive|3|1|
>|sqlite|3|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #43993 +/- ##
===========================================
- Coverage 79.81% 66.32% -13.49%
===========================================
Files 2914 2914
Lines 170473 170494 +21
Branches 39518 39522 +4
===========================================
- Hits 136066 113085 -22981
- Misses 31885 55060 +23175
+ Partials 2522 2349 -173
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43993/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage ฮ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/43993/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `37.62% <9.09%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/43993/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/43993/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/43993/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.50% <9.09%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/43993/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.19% <72.72%> (-27.01%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/43993/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.06% <72.72%> (+<0.01%)` | :arrow_up: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/43993/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/43993?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(rls): match the RLS predicate lookup on case-normalized table identifiers [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #43993:
URL: https://github.com/apache/superset/pull/43993#issuecomment-5587337483
### <span aria-hidden="true">โ
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">๐จ</span> Latest commit | ffb3a40e5dda50de288352600c08cbb5ca24cb85 |
|<span aria-hidden="true">๐</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aa0249846be660008a6d80b |
|<span aria-hidden="true">๐</span> Deploy Preview | [https://deploy-preview-43993--superset-docs-preview.netlify.app](https://deploy-preview-43993--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">๐ฑ</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">๐ค</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctLCgIxDADQu2RtMcgQqjfwGOkkg8FOU2IFP3h3u3ub94UevvcBF4ADlOC23qY3ex2jPlIPFVt5aKru92dPJtqGbaaRmsfO1T48zNvMor36-yqzEzOelnNeqCgRImYmyVjg9wc) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-43993--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(rls): match the RLS predicate lookup on case-normalized table identifiers [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on code in PR #43993:
URL: https://github.com/apache/superset/pull/43993#discussion_r3963216477
##########
superset/utils/rls.py:
##########
@@ -132,29 +159,64 @@ def get_predicates_for_table(
if exclude_dataset_id is not None:
filters.append(SqlaTable.id != exclude_dataset_id)
- dataset = (
- db.session.query(SqlaTable)
- .filter(and_(*filters, SqlaTable.schema == table.schema))
- .one_or_none()
+ def match(schema_predicate: Any) -> SqlaTable | None:
+ query = db.session.query(SqlaTable).filter(and_(*filters, schema_predicate))
+ if not fold:
+ return query.one_or_none()
+ # 0, 1 or "ambiguous" is all the folded lookup needs to tell apart
+ matches = query.limit(2).all()
+ return matches[0] if len(matches) == 1 else None
Review Comment:
Fixed in fe89e522. On an engine that folds unquoted object names the lookup now folds up front instead of retrying after an exact miss, and returns every dataset whose name differs only in case; all of their predicates are applied, so none is dropped. That also covers the case your example hits when the reference matches the added dataset exactly (`SELECT * FROM SALES` with a `SALES` dataset registered), where an exact-first match would have returned only the unprotected one.
##########
tests/unit_tests/sql_lab_test.py:
##########
@@ -705,6 +705,77 @@ def row_level_filters(
) == ["c1 = 'public'"]
+def test_get_predicates_for_table_case_mismatched_reference(session: Session) -> None:
+ """
+ On an engine that doesn't treat unquoted identifiers as case-sensitive, a
+ reference whose catalog, schema or table casing differs from the registered
+ dataset still resolves to the same physical table, so the dataset's RLS
+ predicates must be applied. That includes a dataset stored without a schema,
+ which is scoped to the database's default schema. An engine that does treat
+ them as case-sensitive keeps the exact match, and an ambiguous
+ case-insensitive match is ignored rather than guessed at.
+ """
+ from superset.connectors.sqla.models import SqlaTable
+
+ SqlaTable.metadata.create_all(session.get_bind())
+
+ folding = Database(database_name="rls_db_ci", sqlalchemy_uri="sqlite://")
+ exact = Database(database_name="rls_db_cs", sqlalchemy_uri="mysql://localhost/db")
+ ambiguous = Database(database_name="rls_db_ambiguous", sqlalchemy_uri="sqlite://")
+ null_schema = Database(
+ database_name="rls_db_null_schema", sqlalchemy_uri="sqlite://"
+ )
+ session.add_all(
+ [
+ folding,
+ exact,
+ ambiguous,
+ null_schema,
+ SqlaTable(
+ table_name="t1", schema="public", catalog="cat", database=folding
+ ),
+ SqlaTable(table_name="t1", schema="public", catalog=None, database=exact),
+ SqlaTable(
+ table_name="t1", schema="public", catalog=None, database=ambiguous
+ ),
+ SqlaTable(
+ table_name="T1", schema="public", catalog=None, database=ambiguous
+ ),
+ SqlaTable(table_name="t1", schema=None, catalog=None, database=null_schema),
+ ]
+ )
+ session.flush()
+
+ with (
+ patch.object(
+ SqlaTable, "get_sqla_row_level_filters", return_value=[text("c1 = 1")]
+ ),
+ patch.object(Database, "get_default_schema", return_value="public"),
+ ):
+ for reference in (
+ Table("T1", "public", "cat"),
+ Table("T1", "PUBLIC", "cat"),
+ Table("t1", "public", "CAT"),
Review Comment:
Added in fe89e522: a fourth database with a `catalog=None` dataset, asserting a `CAT` reference with `default_catalog="cat"` still receives its predicate.
##########
tests/unit_tests/sql_lab_test.py:
##########
@@ -705,6 +705,77 @@ def row_level_filters(
) == ["c1 = 'public'"]
+def test_get_predicates_for_table_case_mismatched_reference(session: Session) -> None:
+ """
+ On an engine that doesn't treat unquoted identifiers as case-sensitive, a
+ reference whose catalog, schema or table casing differs from the registered
+ dataset still resolves to the same physical table, so the dataset's RLS
+ predicates must be applied. That includes a dataset stored without a schema,
+ which is scoped to the database's default schema. An engine that does treat
+ them as case-sensitive keeps the exact match, and an ambiguous
+ case-insensitive match is ignored rather than guessed at.
+ """
+ from superset.connectors.sqla.models import SqlaTable
+
+ SqlaTable.metadata.create_all(session.get_bind())
+
+ folding = Database(database_name="rls_db_ci", sqlalchemy_uri="sqlite://")
+ exact = Database(database_name="rls_db_cs", sqlalchemy_uri="mysql://localhost/db")
+ ambiguous = Database(database_name="rls_db_ambiguous", sqlalchemy_uri="sqlite://")
+ null_schema = Database(
+ database_name="rls_db_null_schema", sqlalchemy_uri="sqlite://"
+ )
+ session.add_all(
+ [
+ folding,
+ exact,
+ ambiguous,
+ null_schema,
+ SqlaTable(
+ table_name="t1", schema="public", catalog="cat", database=folding
+ ),
+ SqlaTable(table_name="t1", schema="public", catalog=None, database=exact),
+ SqlaTable(
+ table_name="t1", schema="public", catalog=None, database=ambiguous
+ ),
+ SqlaTable(
+ table_name="T1", schema="public", catalog=None, database=ambiguous
+ ),
+ SqlaTable(table_name="t1", schema=None, catalog=None, database=null_schema),
+ ]
+ )
+ session.flush()
+
+ with (
+ patch.object(
+ SqlaTable, "get_sqla_row_level_filters", return_value=[text("c1 = 1")]
+ ),
+ patch.object(Database, "get_default_schema", return_value="public"),
+ ):
+ for reference in (
+ Table("T1", "public", "cat"),
+ Table("T1", "PUBLIC", "cat"),
+ Table("t1", "public", "CAT"),
+ ):
+ assert get_predicates_for_table(reference, folding, "cat") == ["c1 = 1"], (
+ f"no predicates for {reference}"
+ )
+
+ # dataset stored without a schema, referenced via the default schema
+ assert get_predicates_for_table(
+ Table("T1", "PUBLIC", None), null_schema, None
+ ) == ["c1 = 1"]
+ assert (
+ get_predicates_for_table(Table("T1", "other", None), null_schema, None)
+ == []
+ )
+
+ assert get_predicates_for_table(Table("T1", "public", None), exact, None) == []
+ assert (
+ get_predicates_for_table(Table("t1", "PUBLIC", None), ambiguous, None) == []
+ )
Review Comment:
Covered in fe89e522, but the answer changed: exact casing no longer wins outright, because `SELECT * FROM Sales` matching an unprotected `Sales` dataset exactly would drop `sales`'s predicates. Both datasets' predicates now apply for every casing, which keeps the property you wanted guarded (the exact-case dataset's predicates are never dropped) while closing that direction. Asserted for `t1`, `T1` and `t1`/`PUBLIC` against a database where both `t1` and `T1` exist.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(rls): match the RLS predicate lookup on case-normalized table identifiers [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on PR #43993:
URL: https://github.com/apache/superset/pull/43993#issuecomment-5589503025
Addressed in b608ef9c.
On the two Bito suggestions: the missing return type hint was already present (`-> None`) by the time of that run, and the `db_engine_spec` access is no longer next to a `try/except` in `rls.py` at all, so there is nothing to widen. It is still guarded behind the dataset-miss check, and a malformed SQLAlchemy URI would already have failed the query execution that reaches this path.
The substantive change since then: the folding gate no longer trusts the sqlglot normalization strategy on its own. Engines whose dialect normalizes identifiers but whose catalog/schema/table names are case-sensitive anyway (BigQuery and datastore table ids, Druid datasources, and the shillelagh-backed `gsheets`/`shillelagh`/`superset` engines, where the table name is a URL) are excluded, so they keep exact matching. The exact and folded lookups are also collapsed into one function now, with the exact pass keeping `one_or_none()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(rls): match the RLS predicate lookup on case-normalized table identifiers [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on code in PR #43993:
URL: https://github.com/apache/superset/pull/43993#discussion_r3959348917
##########
superset/utils/rls.py:
##########
@@ -156,6 +183,34 @@ def get_predicates_for_table(
):
dataset = null_schema_dataset
+ if not dataset and _database_folds_unquoted_identifiers(database):
+ # The matches above are case-sensitive, but an engine that folds unquoted
+ # identifiers resolves a case-mismatched reference (e.g. ``BIRTH_NAMES``)
+ # to the same physical table as the registered dataset (``birth_names``).
+ # Fall back to a case-insensitive match so the dataset's RLS predicates
+ # still apply. The exact matches always take precedence, so this never
+ # changes an existing match; an ambiguous (multi-row) match is ignored
+ # rather than guessed at.
+ ci_filters = [
+ SqlaTable.database_id == database.id,
+ catalog_predicate,
+ func.lower(SqlaTable.table_name) == table.table.lower(),
+ ]
+ if exclude_dataset_id is not None:
+ ci_filters.append(SqlaTable.id != exclude_dataset_id)
+ schema_predicate = (
+ SqlaTable.schema == table.schema
+ if table.schema
+ else SqlaTable.schema.is_(None)
+ )
+ matches = (
Review Comment:
Fixed in ffb3a40: the fallback now folds schema and catalog too, and mirrors the null-schema/default-schema tier of the exact lookup, so a folded reference like `CAT.PUBLIC.T1` resolves the same dataset as `cat.public.t1`. Covered by `test_get_predicates_for_table_case_mismatched_reference`.
##########
superset/utils/rls.py:
##########
@@ -156,6 +183,34 @@ def get_predicates_for_table(
):
dataset = null_schema_dataset
+ if not dataset and _database_folds_unquoted_identifiers(database):
+ # The matches above are case-sensitive, but an engine that folds unquoted
+ # identifiers resolves a case-mismatched reference (e.g. ``BIRTH_NAMES``)
+ # to the same physical table as the registered dataset (``birth_names``).
+ # Fall back to a case-insensitive match so the dataset's RLS predicates
+ # still apply. The exact matches always take precedence, so this never
+ # changes an existing match; an ambiguous (multi-row) match is ignored
+ # rather than guessed at.
+ ci_filters = [
+ SqlaTable.database_id == database.id,
+ catalog_predicate,
+ func.lower(SqlaTable.table_name) == table.table.lower(),
Review Comment:
Correct that a parsed reference carries no quote information: `superset.sql.parse.Table` has no quoted flag, and threading one through would change the shared primitive that also feeds `extract_tables_from_statement` and the authorization path, so ffb3a40 documents the limit at the fallback instead of narrowing it. The unmatched direction applies extra predicates to a table no dataset is registered for rather than dropping a predicate that should have applied, and an ambiguous match is still ignored.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(rls): match the RLS predicate lookup on case-normalized table identifiers [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43993:
URL: https://github.com/apache/superset/pull/43993#issuecomment-5584630924
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(rls): match the RLS predicate lookup on case-normalized table identifiers [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43993:
URL: https://github.com/apache/superset/pull/43993#discussion_r3962950422
##########
tests/unit_tests/sql_lab_test.py:
##########
@@ -705,6 +705,77 @@ def row_level_filters(
) == ["c1 = 'public'"]
+def test_get_predicates_for_table_case_mismatched_reference(session: Session) -> None:
+ """
+ On an engine that doesn't treat unquoted identifiers as case-sensitive, a
+ reference whose catalog, schema or table casing differs from the registered
+ dataset still resolves to the same physical table, so the dataset's RLS
+ predicates must be applied. That includes a dataset stored without a schema,
+ which is scoped to the database's default schema. An engine that does treat
+ them as case-sensitive keeps the exact match, and an ambiguous
+ case-insensitive match is ignored rather than guessed at.
+ """
+ from superset.connectors.sqla.models import SqlaTable
+
+ SqlaTable.metadata.create_all(session.get_bind())
+
+ folding = Database(database_name="rls_db_ci", sqlalchemy_uri="sqlite://")
+ exact = Database(database_name="rls_db_cs", sqlalchemy_uri="mysql://localhost/db")
+ ambiguous = Database(database_name="rls_db_ambiguous", sqlalchemy_uri="sqlite://")
+ null_schema = Database(
+ database_name="rls_db_null_schema", sqlalchemy_uri="sqlite://"
+ )
+ session.add_all(
+ [
+ folding,
+ exact,
+ ambiguous,
+ null_schema,
+ SqlaTable(
+ table_name="t1", schema="public", catalog="cat", database=folding
+ ),
+ SqlaTable(table_name="t1", schema="public", catalog=None, database=exact),
+ SqlaTable(
+ table_name="t1", schema="public", catalog=None, database=ambiguous
+ ),
+ SqlaTable(
+ table_name="T1", schema="public", catalog=None, database=ambiguous
+ ),
+ SqlaTable(table_name="t1", schema=None, catalog=None, database=null_schema),
+ ]
+ )
+ session.flush()
+
+ with (
+ patch.object(
+ SqlaTable, "get_sqla_row_level_filters", return_value=[text("c1 = 1")]
+ ),
+ patch.object(Database, "get_default_schema", return_value="public"),
+ ):
+ for reference in (
+ Table("T1", "public", "cat"),
+ Table("T1", "PUBLIC", "cat"),
+ Table("t1", "public", "CAT"),
Review Comment:
This `CAT` case only covers a dataset stored with `catalog="cat"`; it never exercises the new null-catalog fallback gated by the case-folded default catalog. Could we add a `catalog=None` dataset and assert that a `CAT` reference with `default_catalog="cat"` still receives its RLS predicate?
##########
tests/unit_tests/sql_lab_test.py:
##########
@@ -705,6 +705,77 @@ def row_level_filters(
) == ["c1 = 'public'"]
+def test_get_predicates_for_table_case_mismatched_reference(session: Session) -> None:
+ """
+ On an engine that doesn't treat unquoted identifiers as case-sensitive, a
+ reference whose catalog, schema or table casing differs from the registered
+ dataset still resolves to the same physical table, so the dataset's RLS
+ predicates must be applied. That includes a dataset stored without a schema,
+ which is scoped to the database's default schema. An engine that does treat
+ them as case-sensitive keeps the exact match, and an ambiguous
+ case-insensitive match is ignored rather than guessed at.
+ """
+ from superset.connectors.sqla.models import SqlaTable
+
+ SqlaTable.metadata.create_all(session.get_bind())
+
+ folding = Database(database_name="rls_db_ci", sqlalchemy_uri="sqlite://")
+ exact = Database(database_name="rls_db_cs", sqlalchemy_uri="mysql://localhost/db")
+ ambiguous = Database(database_name="rls_db_ambiguous", sqlalchemy_uri="sqlite://")
+ null_schema = Database(
+ database_name="rls_db_null_schema", sqlalchemy_uri="sqlite://"
+ )
+ session.add_all(
+ [
+ folding,
+ exact,
+ ambiguous,
+ null_schema,
+ SqlaTable(
+ table_name="t1", schema="public", catalog="cat", database=folding
+ ),
+ SqlaTable(table_name="t1", schema="public", catalog=None, database=exact),
+ SqlaTable(
+ table_name="t1", schema="public", catalog=None, database=ambiguous
+ ),
+ SqlaTable(
+ table_name="T1", schema="public", catalog=None, database=ambiguous
+ ),
+ SqlaTable(table_name="t1", schema=None, catalog=None, database=null_schema),
+ ]
+ )
+ session.flush()
+
+ with (
+ patch.object(
+ SqlaTable, "get_sqla_row_level_filters", return_value=[text("c1 = 1")]
+ ),
+ patch.object(Database, "get_default_schema", return_value="public"),
+ ):
+ for reference in (
+ Table("T1", "public", "cat"),
+ Table("T1", "PUBLIC", "cat"),
+ Table("t1", "public", "CAT"),
+ ):
+ assert get_predicates_for_table(reference, folding, "cat") == ["c1 = 1"], (
+ f"no predicates for {reference}"
+ )
+
+ # dataset stored without a schema, referenced via the default schema
+ assert get_predicates_for_table(
+ Table("T1", "PUBLIC", None), null_schema, None
+ ) == ["c1 = 1"]
+ assert (
+ get_predicates_for_table(Table("T1", "other", None), null_schema, None)
+ == []
+ )
+
+ assert get_predicates_for_table(Table("T1", "public", None), exact, None) == []
+ assert (
+ get_predicates_for_table(Table("t1", "PUBLIC", None), ambiguous, None) == []
+ )
Review Comment:
The ambiguous case changes the schema casing, so the exact lookup necessarily misses. Could this also assert that an exact `t1` reference wins when both `t1` and `T1` exist? That guards the intended exact-before-folded precedence from regressing into an RLS bypass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(rls): match the RLS predicate lookup on case-normalized table identifiers [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #43993:
URL: https://github.com/apache/superset/pull/43993#discussion_r3957586114
##########
superset/utils/rls.py:
##########
@@ -156,6 +183,34 @@ def get_predicates_for_table(
):
dataset = null_schema_dataset
+ if not dataset and _database_folds_unquoted_identifiers(database):
+ # The matches above are case-sensitive, but an engine that folds unquoted
+ # identifiers resolves a case-mismatched reference (e.g. ``BIRTH_NAMES``)
+ # to the same physical table as the registered dataset (``birth_names``).
+ # Fall back to a case-insensitive match so the dataset's RLS predicates
+ # still apply. The exact matches always take precedence, so this never
+ # changes an existing match; an ambiguous (multi-row) match is ignored
+ # rather than guessed at.
+ ci_filters = [
+ SqlaTable.database_id == database.id,
+ catalog_predicate,
+ func.lower(SqlaTable.table_name) == table.table.lower(),
Review Comment:
**Suggestion:** Parsed quoted identifiers lose their quote information, so PostgreSQL reference `"BIRTH_NAMES"` can incorrectly receive predicates for the distinct unquoted dataset `birth_names`. [logic error]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a404141070fc4600a1a389fe072ed62e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a404141070fc4600a1a389fe072ed62e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/rls.py
**Line:** 197:197
**Comment:**
*Logic Error: Parsed quoted identifiers lose their quote information, so PostgreSQL reference `"BIRTH_NAMES"` can incorrectly receive predicates for the distinct unquoted dataset `birth_names`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43993&comment_hash=29328b5238a36e79587e99317c05ff3b6dfa3137ef74d42dff3dc9dd63066703&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43993&comment_hash=29328b5238a36e79587e99317c05ff3b6dfa3137ef74d42dff3dc9dd63066703&reaction=dislike'>๐</a>
##########
superset/utils/rls.py:
##########
@@ -156,6 +183,34 @@ def get_predicates_for_table(
):
dataset = null_schema_dataset
+ if not dataset and _database_folds_unquoted_identifiers(database):
+ # The matches above are case-sensitive, but an engine that folds unquoted
+ # identifiers resolves a case-mismatched reference (e.g. ``BIRTH_NAMES``)
+ # to the same physical table as the registered dataset (``birth_names``).
+ # Fall back to a case-insensitive match so the dataset's RLS predicates
+ # still apply. The exact matches always take precedence, so this never
+ # changes an existing match; an ambiguous (multi-row) match is ignored
+ # rather than guessed at.
+ ci_filters = [
+ SqlaTable.database_id == database.id,
+ catalog_predicate,
+ func.lower(SqlaTable.table_name) == table.table.lower(),
+ ]
+ if exclude_dataset_id is not None:
+ ci_filters.append(SqlaTable.id != exclude_dataset_id)
+ schema_predicate = (
+ SqlaTable.schema == table.schema
+ if table.schema
+ else SqlaTable.schema.is_(None)
+ )
+ matches = (
Review Comment:
**Suggestion:** Case-insensitive lookup still requires an exact schema match, so references using different casing for a folded schema receive no RLS predicates. [api mismatch]
**Assessment:** ๐ด `Critical` ยท ๐ `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=741052e67a9b474dbced3520299ed090&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=741052e67a9b474dbced3520299ed090&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/rls.py
**Line:** 202:206
**Comment:**
*Api Mismatch: Case-insensitive lookup still requires an exact schema match, so references using different casing for a folded schema receive no RLS predicates.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43993&comment_hash=f26d6cf4ed8041b0974ebf86c8e9960777a62f5c3988e0be2f421fe8657c85d9&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43993&comment_hash=f26d6cf4ed8041b0974ebf86c8e9960777a62f5c3988e0be2f421fe8657c85d9&reaction=dislike'>๐</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(rls): match the RLS predicate lookup on case-normalized table identifiers [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43993:
URL: https://github.com/apache/superset/pull/43993#issuecomment-5594109459
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(rls): match the RLS predicate lookup on case-normalized table identifiers [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on code in PR #43993:
URL: https://github.com/apache/superset/pull/43993#discussion_r3960748696
##########
superset/utils/rls.py:
##########
@@ -156,6 +183,34 @@ def get_predicates_for_table(
):
dataset = null_schema_dataset
+ if not dataset and _database_folds_unquoted_identifiers(database):
+ # The matches above are case-sensitive, but an engine that folds unquoted
+ # identifiers resolves a case-mismatched reference (e.g. ``BIRTH_NAMES``)
+ # to the same physical table as the registered dataset (``birth_names``).
+ # Fall back to a case-insensitive match so the dataset's RLS predicates
+ # still apply. The exact matches always take precedence, so this never
+ # changes an existing match; an ambiguous (multi-row) match is ignored
+ # rather than guessed at.
+ ci_filters = [
+ SqlaTable.database_id == database.id,
+ catalog_predicate,
+ func.lower(SqlaTable.table_name) == table.table.lower(),
Review Comment:
Narrowed rather than documented in b608ef9c: the folding gate now excludes engines whose object names are case-sensitive despite the dialect normalizing identifiers (BigQuery/datastore table ids, Druid datasources, and the shillelagh-backed gsheets/shillelagh/superset engines, whose table name is a URL). The remaining quoted-reference caveat on genuinely folding engines only ever adds predicates for a table no dataset is registered for, and an ambiguous match is still ignored.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(rls): match the RLS predicate lookup on case-normalized table identifiers [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #43993:
URL: https://github.com/apache/superset/pull/43993
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org