Check warehouse session identity and query access - #281
Draft
jat255 wants to merge 3 commits into
Draft
Conversation
jat255
added
needs-manual-review
A warehouse decides who may read what, so commons asks with a zero-row query and classifies the failure: an authorization refusal is stable and is cached per relation, a transient one retries on the next touch, and an unrecognized one is neither. The session snapshot pins the principal, roles, and namespace those answers were decided for, and a refusal names which of them moved. Every relation the listing reported is probed before anything is raised, so a name the caller got wrong and a relation they cannot read are reported in one pass rather than one round trip each. The check pairs the selection with the listing by label, which meant the two had to agree on one. They did not for an entry naming a bare table: the warehouse qualifies it from the connection's namespace, so the registry now records the relation's own label rather than the selector's.
…relation Two defects the Python implementation of the same contract exposed. The session refusal listed every field the check compares, which told a Databricks user a role had changed on a backend that has none; it now names the fields that differ, and the documentation and the governance vignette scope the role snapshot to Snowflake. And an access refusal on one named relation discarded the missing names already found, so the caller fixed a typo only to be told about the next problem on the next attempt; every relation the listing reported is now probed before anything is raised.
…dence Three behaviours both packages have to agree on, each implemented twice and asserted twice until now: how a driver failure classifies, which parts of a session identity a refusal reports, and what construction reports when several named relations fail at once. They move to tests/shared/ and both suites read them. Field names travel rather than message text, since the wording belongs to each language.
jat255
force-pushed
the
jat255/xp65-catalog-security
branch
from
September 5, 2026 04:58
f2baa36 to
e76c9ea
Compare
|
Preview deployed to Connect ( Deployed from commit e76c9ea. |
|
Preview deployed to Connect ( Deployed from commit e76c9ea. |
|
Preview deployed to Connect ( Deployed from commit e76c9ea. |
|
Preview deployed to Connect ( Deployed from commit e76c9ea. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fifth of the catalog import stack (kata xp65). Stacked on #278. Ports the access half of
catalog-security.R.A warehouse decides who may read what, so commons asks with a query that returns no rows and reads the answer off the failure. The classification is what matters: an authorization refusal is stable, so it is cached per relation; a transient one is retried on the next touch; anything unrecognized is neither. The session snapshot is there for the same reason, since access was decided for one principal, role, and namespace, and if any of those move the answers no longer apply.
Semantic-model probing is the other half of that file and stays with the semantic model work (kata gcgj).
ensure_queryable()ships with no caller: R reaches it fromsource_describe(), and Python has no first-touch surface until the retrieval work, so nothing resolves a relation label at query time yet.Shared fixtures
Three behaviours were about to be implemented and asserted twice, so they move to
tests/shared/: how a driver failure classifies (catalog-access-errors.json), which parts of a session identity a refusal reports (catalog-session-changed.json), and what construction reports when several named relations fail at once (catalog-access-precedence.json). Both suites read them. Field names travel rather than message text, since the wording belongs to each language.R changes
Two defects, both found by writing the same contract a second time.
The session refusal named every field the check compares, which told a Databricks user that a role had changed on a backend that has no role to change.
catalog_check_session_snapshot()now compares field by field and names only what differs, through a newcatalog_session_changed_fields()and a label lookup for the wording. The behaviour delta is the message text; the condition class is unchanged, and no test matched on the old wording. The roxygen fordata_source(), its.Rd, and the governance vignette carried the same over-broad claim and now scope the role snapshot to Snowflake.catalog_require_queryable_relations()raised on the first relation it could not read, which discarded any missing names it had already collected: the caller fixed a typo and only then learned about the next problem. It now skips the relations the listing never reported, probes the rest, and reports every missing name before any refusal. The delta is in the failure path only, and it can cost extra round trips there, since a refusal no longer stops the loop. A successful construction probes exactly what it did before.Blast radius: both functions are reached only from
data_source_connection(), once per warehouse branch, pluscatalog_check_session()for the session check, whichsource_query(),source_describe(),catalog_search(), and the semantic model path all go through. Neither signature changed.Why it is safe:
test-catalog-security.R(102) andtest-data-source.R(66, 10 skipped) pass. Both new fixture runners were confirmed to bite, by restoring the abort-on-first-refusal behaviour and watching five R cases fail, and by flipping an expected outcome in each suite. I did not run the full R suite, which hangs for about ten minutes until #268 lands.Worth your scrutiny: the precedence change is a behaviour change in R rather than a port artefact, and whether reporting everything at once is worth the extra probes on the failure path.
Deleted R tests: "warehouse access errors are classified conservatively" is now the fixture runner plus one R-only case for
NA_character_, which JSON cannot spell. Nothing it asserted was dropped.pkg-r/tests/testthat/fixtures/shared/is generated byscripts/sync-shared-fixtures.sh; skip it.Verification
707 Python tests, ruff and pyrefly clean.