eschutho opened a new pull request, #45792:
URL: https://github.com/apache/superset/pull/45792
Shortcut: sc-129171 (related: sc-128628)
## Root cause
On Snowflake a schema-level Data Access Role (DAR) grant never matches at
runtime because of catalog casing.
- Core permission sync (`superset/commands/database/utils.py`,
`add_permissions`, ~L242-256) builds `catalog_access`/`schema_access` rows from
`database.get_all_catalog_names()`. Snowflake lists UPPERCASE names, giving
e.g. `[salesplanning_db].[SALESPLANNING_DB].[consumer_marts]`.
- Datasets / runtime use the URI default catalog
(`SnowflakeEngineSpec.get_default_catalog`,
`db_engine_specs/snowflake.py:463`), i.e. lowercase:
`[salesplanning_db].[salesplanning_db].[consumer_marts]`.
- Core compares exactly: `can_access_catalog`
(`superset/security/manager.py:2538`), `can_access_schema` (:2591),
`get_schemas_accessible_by_user` (:3111, `parts[1] == catalog` ~L3162),
`get_catalogs_accessible_by_user` (:3188), `get_datasources_accessible_by_user`
(:3282-3296), `raise_for_access` (:5434-5442).
- The DAR editor lists all data-access PVMs (`sm.get_data_access_pvms`,
`preset/security/manager.py`), so the uppercase row is selectable. The Manager
event path (`workspace_pub_sub._build_pvm_from_dar_event_permissions` ->
`find_permission_view_menu`) is exact-match and silently drops misses.
- Full `database_access` short-circuits all of this, which is why that
workaround works.
## Fix
No submodule changes. In `SupersetShellSecurityManager`
(`preset/security/manager.py`), using the pure helpers in the new
`preset/security/catalog_case.py`:
- `can_access` override: if the exact check fails for
`catalog_access`/`schema_access`, allow when the user holds a grant identical
except for the **catalog segment's case**, and the database is Snowflake. This
single seam covers every core `can_access("catalog_access"|"schema_access",
...)` caller (`can_access_catalog`, `can_access_schema`, datasource listing,
`raise_for_access`) without copying any core method.
- `user_view_menu_names` override: for Snowflake databases, adds
lower/upper-case catalog spellings of the user's catalog-scoped grants. This
covers the set-membership code that bypasses `can_access`
(`get_schemas_accessible_by_user`, `get_catalogs_accessible_by_user`,
`utils/filters.py`).
- New `find_permission_view_menus_case_insensitive`, used by
`_build_pvm_from_dar_event_permissions`: returns the exact PVM plus every other
PVM that differs only by catalog case (all variants are attached to the DAR).
`perm_urn_to_vm` is unchanged.
Gating: explicit allowlist `CASE_INSENSITIVE_CATALOG_BACKENDS =
{"snowflake"}` keyed on `Database.backend`. There is no engine-spec attribute
for identifier case sensitivity in core, so the allowlist lives here.
## Trade-offs / risks
- **Access widening**: only the catalog segment is relaxed. Database name,
schema name and permission kind still compare exactly; a different
database/schema/catalog never matches (tested). Residual risk: Snowflake quoted
identifiers can in theory create two catalogs differing only by case; they
would now be treated as one for grants. Considered acceptable for Snowflake
(very rare, catalogs are normally unquoted); it is not applied to
case-sensitive engines.
- **Schema case is not relaxed**: the evidence only shows a catalog
mismatch, and relaxing schema widens access further. If schema casing turns out
to diverge too, extend `differs_only_by_catalog_case`.
- `user_view_menu_names` expansion only lower/upper-cases the catalog; a
mixed-case catalog is still handled by the `can_access` path but not by the
set-based paths.
- Extra cost: one `Database` lookup per call when the user holds
catalog-scoped grants, and per failed exact `can_access` only when a
case-variant grant exists.
- **Silent-revocation caveat (#5650)**: DAR updates replace the permission
list wholesale, and unresolvable permissions are still dropped (now only after
the case-insensitive lookup). This PR does not change that.
- **Upstream**: the root problem (sync vs. default-catalog casing) belongs
in apache/superset (normalise catalog case at permission creation/check for
case-insensitive engines). It lives in the shell for now because core is the
submodule; an upstream follow-up is desirable and this override can then be
deleted.
## Dependency on open PRs
#5650 and #5657 are both still open and also edit
`_build_pvm_from_dar_event_permissions`. This PR replaces the
`find_permission_view_menu` lookup with
`find_permission_view_menus_case_insensitive`, so whichever lands second will
conflict textually in those few lines, and the tests in those PRs that mock
`_sm.find_permission_view_menu` will need to mock the new method instead.
`perm_urn_to_vm` (touched by #5650) is not modified here.
## Verified vs. not verified
Verified (by reading the code at the pinned submodule `bea6db6d`): all line
references above; Snowflake `get_default_catalog`; exact comparisons in core.
Verified by running: the new unit tests (below) in a **stubbed local
environment** (see test plan). They run the real core
`get_schemas_accessible_by_user`, `get_catalogs_accessible_by_user`,
`can_access_catalog` and `can_access_schema` against
`SupersetShellSecurityManager`, with the DB lookups faked.
NOT verified:
- No run against a real Snowflake database, real Superset metadata DB, or
the Docker stack / Manager (private packages / VPN unavailable here). Nothing
in a deployed environment was touched.
- The full repo test suite and pre-commit `mypy` were not run; CI is the
first real run of the project's conftest/`BaseTest` harness. These tests
deliberately avoid it.
- That the DAR editor/Manager flow end-to-end now yields schemas in SQL Lab.
- That schema names never differ in case in practice.
## Test plan
Local (stubbed env, not the repo's standard environment): a venv with
`superset-oss` installed editable; the private `clients`, `service_lib` and
`cloudevents` packages and other uninstalled third-party modules were replaced
by `MagicMock` import stubs.
```
pytest --noconftest tests/security/test_catalog_case.py
tests/multi_tenant/test_dar_permission_case_variants.py
# 37 passed
ruff check / ruff format (v0.4.0, pinned) on changed files: clean
```
Scenarios covered: user holds `schema_access`
`[salesplanning_db].[SALESPLANNING_DB].[consumer_marts]`, default catalog
`salesplanning_db`, candidates {consumer_marts, other} -> only
`consumer_marts`; explicit catalog in either case; dataset-level
`can_access_schema` with lowercase `schema_perm`; `catalog_access` variant;
event path (lowercase URN -> uppercase PVM, all variants included); negatives
(case-sensitive engine, no grant, other catalog / database / schema,
schema-case difference, anonymous user, non-Snowflake database).
CI should run: `pytest tests/security/test_catalog_case.py
tests/multi_tenant/test_dar_permission_case_variants.py`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
