Skip to content

fix(semantic-layers): access-filter the /connections database branch - #43887

Draft
mikebridge wants to merge 1 commit into
apache:masterfrom
mikebridge:sc-119878-connections-databasefilter
Draft

fix(semantic-layers): access-filter the /connections database branch#43887
mikebridge wants to merge 1 commit into
apache:masterfrom
mikebridge:sc-119878-connections-databasefilter

Conversation

@mikebridge

Copy link
Copy Markdown
Contributor

SUMMARY

The semantic-layer /connections/ endpoint (SemanticLayerRestApi, mapped to can_read) lists databases and semantic layers together. _fetch_connection_items access-filters the semantic-layer branch (SemanticLayer.perm.in_(perms) when not can_access_all_datasources()), but issued a bare db.session.query(Database) for the database branch — so any caller reaching the endpoint received every database's database_name, backend, allow_dml, allow_file_upload, expose_in_sqllab and last editor, regardless of database access.

This applies DatabaseFilter — the same scoping DatabaseRestApi uses via base_filters = [["id", DatabaseFilter, lambda: []]] — to the database branch, so the database inventory is scoped exactly the way every other database path scopes it. The raw sqlalchemy_uri is not exposed by the serializer (it is loaded only so backend can be derived).

Security framing (per SECURITY.md): this is an in-scope authorization-scoping fix, not a regression introduced by any recent change. The gap is pre-existing on master. Built-in Gamma reached this endpoint before (via can_connections) and after (via can_read), so built-in-role reach is unchanged; what changed upstream is that the endpoint is now gated on the coarse, commonly-granted can_read and advertised as broadly accessible, so an operator can no longer grant "browse semantic layers" without also handing over the database inventory. Database/datasource inventory is object-scoped via DatabaseFilter on every other path.

  • Role/capability matrix row: database (connection) inventory visibility, object-scoped by DatabaseFilter base_filters on the Database API.
  • Principal assumed: Gamma, or any custom role granted can_read on SemanticLayer.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — backend authorization scoping, no UI change.

TESTING INSTRUCTIONS

Unit tests in tests/unit_tests/semantic_layers/api_test.py:

  • test_connections_all_access_user_sees_all_databases — a full-access caller has the database branch left unfiltered.
  • test_connections_limited_user_access_filters_databases — a caller without all-database access has DatabaseFilter's access predicate applied (asserts user_view_menu_names("database_access") was consulted). Reverting the fix (bare db.session.query(Database)) fails this test.

Manual: as a Gamma user (or a custom role with only can_read on SemanticLayer) that lacks access to some databases, GET /api/v1/semantic_layer/connections/ returns only the databases the user can access; an all-access user still sees all.

ADDITIONAL INFORMATION

  • Has associated issue: sc-119878 (follow-up from the fix(semantic-layers): classify SemanticLayer/SemanticView in FAB role sets #43783 / sc-119467 review)
  • Required feature flags: SEMANTIC_LAYERS (the endpoint is gated behind it; default off)
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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

🤖 Generated with Claude Code

https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC

sc-119878 (follow-up from Amin's apache#43783 review). `_fetch_connection_items`
access-filters the semantic-layer branch (`SemanticLayer.perm.in_(perms)` when
not `can_access_all_datasources()`) but issued a bare `db.session.query(Database)`
for the database branch, so any caller reaching the `can_read`-gated
`/connections/` endpoint got every database's name, backend, allow_dml,
allow_file_upload, expose_in_sqllab and last editor regardless of database access.

Apply `DatabaseFilter` (DatabaseRestApi's `base_filters` scoping) to the database
branch, before the name filter per its dynamic-filter ordering contract, so the
DB inventory is scoped the same way every other database path is.

Per SECURITY.md this is an authorization-scoping fix, not a regression by apache#43783:
the gap is pre-existing on master; built-in Gamma reached the endpoint before
(via `can_connections`) and after (via `can_read`), so built-in-role reach is
unchanged. What changed is the endpoint is now gated on the coarse, commonly
granted `can_read` and advertised in UPDATING.md as broadly accessible, so an
operator can no longer grant "browse semantic layers" without also handing over
the database inventory. Principal: Gamma / any role with `can_read` on
SemanticLayer; the exposed row is DB inventory, access-filtered on every other
path.

Tests: an all-access user leaves the DB branch unfiltered (sees all); a user
without all-database access has the access predicate applied. Reverting the fix
fails the limited-user test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
@mikebridge

Copy link
Copy Markdown
Contributor Author

@aminghadersohi — this is the DatabaseFilter follow-up you flagged on #43783 (the database branch of _fetch_connection_items being a bare db.session.query(Database) while the semantic-layer branch was access-filtered). Filed as sc-119878 and now up as this draft.

It applies DatabaseFilter("id", SQLAInterface(Database)).apply(...) to the database branch — the same base_filters scoping DatabaseRestApi uses — so the DB inventory is scoped the same way on every path. I kept your SECURITY.md framing in the PR body (pre-existing, not a regression; principal Gamma / any can_read role; in-scope authorization fix).

A 4-lens pass (preset-review for the security angle + clean-code/tidy-first/python) reviewed it: preset-review confirmed the scoping is correct and complete, matches DatabaseRestApi exactly, and — checked specifically — the retained sqlalchemy_uri in load_only is NOT serialized out, so no residual URI leak. Report filed in the spec repo. Left as a draft; would appreciate your eyes when you have a moment, and I'll undraft on your word.

@github-actions github-actions Bot added the api Related to the REST API label Sep 4, 2026
@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.45%. Comparing base (d9b201d) to head (f705e9e).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #43887   +/-   ##
=======================================
  Coverage   79.45%   79.45%           
=======================================
  Files        2895     2895           
  Lines      168167   168169    +2     
  Branches    38995    38995           
=======================================
+ Hits       133624   133626    +2     
  Misses      32044    32044           
  Partials     2499     2499           
Flag Coverage Δ
hive 37.77% <0.00%> (-0.01%) ⬇️
mysql 57.48% <0.00%> (-0.01%) ⬇️
postgres 57.51% <0.00%> (-0.01%) ⬇️
presto 39.66% <0.00%> (-0.01%) ⬇️
python 83.87% <100.00%> (+<0.01%) ⬆️
sqlite 57.21% <0.00%> (-0.01%) ⬇️
unit 74.40% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api Related to the REST API size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant