aminghadersohi opened a new pull request, #45815: URL: https://github.com/apache/superset/pull/45815
### SUMMARY **What.** `ExploreMixin.get_from_clause` skips the datasource's own RLS when it injects RLS into the inner SQL, because a virtual dataset applies its own rules on the outer `WHERE`. The id to skip came from `getattr(self, "id", None)`, on the assumption that only a dataset has an `id`. A SQL Lab `Query` has one too: the query's own id. This PR excludes the id only when the datasource is a `SqlaTable`, and passes `None` for any other datasource. **Why.** For a chart built on a `Query` datasource, the RLS rules of whichever dataset happened to share the query's numeric id were left out of the inner SQL at execution. Rules on the table the query reads were dropped whenever that table's dataset id matched the query id. The fail-closed check that runs after a failed injection (`get_predicates_for_table(..., exclude_dataset_id=...)`) used the same id, so it also ignored those rules and let the query run without them. **How.** One change in `get_from_clause`: `self_id = self.id if isinstance(self, SqlaTable) else None`. The import is local to avoid a circular import with the connectors package. The apply path and the fail-closed retry both use this value. Other callers that pass `exclude_dataset_id`: - `superset/models/helpers.py` `get_from_clause`, the `apply_rls` call and the fail-closed `get_predicates_for_table` retry: fixed. - `superset/connectors/sqla/models.py` `SqlaTable.get_extra_cache_keys`, `collect_rls_predicates_for_sql(..., exclude_dataset_id=self.id)`: not affected, since `self` is always a dataset there. - `superset/models/sql_lab.py` `Query.get_extra_cache_keys`: passes no exclusion, so it was already correct and now matches what execution applies. - `superset/utils/rls.py` (`apply_rls`, `get_predicates_for_table`, `collect_rls_predicates_for_sql`): these only pass the parameter through. - The only `ExploreMixin` subclasses are `SqlaTable` and `Query`. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF N/A ### TESTING INSTRUCTIONS ``` pytest tests/unit_tests/models/query_datasource_rls_test.py tests/unit_tests/models/test_virtual_dataset_format.py ``` New tests in `tests/unit_tests/models/query_datasource_rls_test.py`: - `test_query_datasource_inner_sql_gets_rls_of_dataset_sharing_its_id[7]`: dataset id 7 has an RLS rule on table `t`, and a `Query` with id 7 reads `t`. The predicate must be in the `FROM` clause. `[8]` is the control, where the ids differ. - `test_query_datasource_fail_closed_counts_rls_of_dataset_sharing_its_id`: when injection fails, the same query must be refused (`QueryObjectValidationError`). - `test_virtual_dataset_inner_sql_skips_its_own_rls`: a virtual dataset still skips its own rule in the inner SQL. Before the fix, on the master `helpers.py`: ``` E AssertionError: assert 'tenant_id = 1' in 'SELECT * FROM t\n' FAILED ...::test_query_datasource_inner_sql_gets_rls_of_dataset_sharing_its_id[7] E Failed: DID NOT RAISE <class 'superset.exceptions.QueryObjectValidationError'> FAILED ...::test_query_datasource_fail_closed_counts_rls_of_dataset_sharing_its_id 2 failed, 23 passed ``` After the fix: `25 passed`. `test_virtual_dataset_format.py` builds its mock datasource with `spec=SqlaTable` rather than `spec=ExploreMixin`, since only a dataset skips its own RLS. Also run: `tests/unit_tests/models`, `tests/unit_tests/connectors` and `tests/unit_tests/utils/rls_test.py` (2118 passed), and the `tests/unit_tests/sql` gate with `--cov=superset/sql --cov-fail-under=100` (3112 passed, 3 skipped, 100%). ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
