Ujjwaljain16 opened a new pull request, #43974: URL: https://github.com/apache/superset/pull/43974
### SUMMARY DB engine specs without catalog support (e.g. `MssqlEngineSpec`, which has `supports_catalog = False` and implements no catalog methods) never create a catalog-qualified `catalog_access`/`schema_access` permission, and register datasets with `catalog=None` -- the only form the permission and dataset-creation flows ever produce for such engines. But a dialect like T-SQL still allows (and sqlglot still parses) a fully qualified `database.schema.table` reference. A query that merely restates the connection's own database -- e.g. `abcm.dbo.temp` on a connection already pointed at `abcm` -- yields a non-empty parsed `catalog`. Checking that redundant value as a real catalog produces a `[db].[db].[schema]` permission string (or a dataset lookup filtered by that same catalog) that can never match the permission or dataset actually granted, denying access that should be authorized. This fixes #31406: a user with `schema access on [abcm].[dbo]` (or a registered dataset + `datasource_access`) was denied when running `SELECT * FROM abcm.dbo.temp`, and had to be granted much broader database-wide access to work around it. `raise_for_access` now normalizes a self-referential catalog (parsed catalog equal to the connection's own database, on an engine that doesn't support catalogs) to `None` once, up front, so both the `catalog_perm`/`schema_perm` check and the `SqlaTable.query_datasources_by_name` dataset lookup agree. A genuinely different database name (e.g. `another_db.dbo.temp`) is left untouched and still denied -- there is no permission format to authorize it for these engines, and this PR does not add one. This PR intentionally does **not**: - Change `force_dataset_match=True` (SQL Lab's stricter dataset-match requirement, #40409). That's a separate, later, deliberate hardening; schema access alone remains insufficient to execute a raw SQL Lab query without a matching registered dataset. This fix only ensures the dataset *lookup itself* isn't defeated by the same catalog-parsing artifact. - Fix Row-Level Security's identical blind spot (`get_predicates_for_table` in `superset/utils/rls.py`, which also compares against `database.get_default_catalog()`, which is `None` for these engines -- meaning RLS rules can silently fail to apply to a catalog-qualified query). Same root cause, different code path and tests. Filing a separate issue for this, since it's a data-exposure concern in its own right and deserves independent review. - Fix `DatasetDAO`'s dataset-uniqueness check (`daos/dataset.py`), which has the same blind spot at lower severity. Also a separate follow-up. Keeping this PR scoped to the permission/dataset-lookup mismatch only. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF N/A -- backend permission-check fix, no UI changes. ### TESTING INSTRUCTIONS Added regression tests to `tests/unit_tests/security/manager_test.py` covering: - catalog-qualified self-reference authorized by schema access - unqualified form still works (no regression) - a genuinely different database is still denied - a cross-catalog join denies only the unauthorized side - catalog-*supporting* engines (Postgres, etc.) are unaffected -- the legacy 2-part permission still doesn't satisfy a catalog-qualified query there, preserving SIP-95 semantics - SQL Lab's `force_dataset_match=True` strictness is unchanged - a registered dataset + `datasource_access` now correctly authorizes the self-referential-catalog case under `force_dataset_match=True` (the dataset-lookup half of the fix) ``` pytest tests/unit_tests/security/manager_test.py -v ``` ### ADDITIONAL INFORMATION - [x] Has associated issue: Fixes #31406 - [ ] 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]
