codeant-ai-for-open-source[bot] commented on code in PR #43090: URL: https://github.com/apache/superset/pull/43090#discussion_r3766991948
########## tests/unit_tests/security/test_fallback_rls_virtual_dataset.py: ########## @@ -0,0 +1,314 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +FR-12 fallback audit: a virtual dataset (a ``SqlaTable`` whose ``sql`` is an +ad-hoc query) built over a governed physical source must inherit that source's +Row Level Security when queried -- including the leak-prone *global-guest* case. + +These tests exercise the REAL predicate-resolution path: + +* real in-memory metadata DB (``session`` fixture) holding the physical + ``SqlaTable`` and the virtual ``SqlaTable``; +* real ``superset.utils.rls.apply_rls`` / ``get_predicates_for_table`` parsing + and rewriting the virtual dataset's inner SQL (the exact call + ``SqlaTable.get_from_clause`` makes at query time); +* real ``BaseDatasource.get_sqla_row_level_filters`` for the outer WHERE, and + the real ``SupersetSecurityManager.get_guest_rls_filters`` global-vs-scoped + matching logic (only the identity *resolver* + ``get_current_guest_user_if_guest`` is stubbed, so the security-critical + global/scoped decision runs for real). + +WARNING (env constraint): the task specifies integration tests under +``tests/integration_tests/security/`` when the live integration harness (app +factory + metadata DB + HTTP fixtures) runs. That full harness is not runnable +here, so this is provided as the strongest unit-level proof against the real +``utils/rls`` path with RLS rules stubbed at the security-manager seam. The +end-to-end HTTP parity assertion remains owed by F4-T3 / F3-T2 (see impl-notes). + +Audit verdict: NO-GAP-PROVEN-SAFE. Global-guest RLS is applied exactly once, on +the virtual dataset's outer WHERE (``include_global_guest_rls=True``), and is +correctly excluded from the inner-table path (``include_global_guest_rls=False``) +to avoid double application -- never dropped. No fix to +``connectors/sqla/models.py`` was required; these tests stand as the regression +guard that gates F2-T2 (fail-closed). +""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import MagicMock, patch + +from sqlalchemy.orm.session import Session + +# Seam used for patching the security manager as imported into models.py. +_SM = "superset.connectors.sqla.models.security_manager" +_FEATURE = "superset.connectors.sqla.models.is_feature_enabled" + + +def _setup_metadata(session: Session) -> None: + """Create the Superset metadata tables in the in-memory DB.""" + from superset.connectors.sqla.models import SqlaTable + + SqlaTable.metadata.create_all(session.get_bind()) + + +def _governed_virtual(session: Session) -> tuple[Any, Any, Any]: + """A governed physical table + a virtual dataset selecting from it. + + Returns ``(database, physical_table, virtual_dataset)`` already flushed so + both datasets carry real ids (the id is what distinguishes the virtual + dataset from its underlying physical source during RLS resolution). + """ + from superset import db + from superset.connectors.sqla.models import SqlaTable + from superset.models.core import Database + + _setup_metadata(session) + database = Database(database_name="d", sqlalchemy_uri="sqlite://") + physical = SqlaTable( + table_name="governed_orders", + schema="main", + database=database, + ) + virtual = SqlaTable( + table_name="my_saved_chart", + schema="main", + database=database, + sql="SELECT * FROM governed_orders", + ) + db.session.add_all([database, physical, virtual]) + db.session.flush() Review Comment: **Suggestion:** The helper accepts an in-memory `session` but persists through the global `db.session` instead. Consequently, the objects are created in a different database/session than the one whose metadata was initialized, so these tests can fail with missing tables or exercise the application database rather than the supplied isolated session. Use the passed `session` consistently for both `add_all` and `flush`. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Virtual-dataset RLS regression tests can fail during fixture setup. - ⚠️ Tests may exercise application metadata instead of isolated SQLite state. - ⚠️ Five fallback-RLS test cases depend on this helper. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fae596ebf9724efbbf6adae494244549&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=fae596ebf9724efbbf6adae494244549&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/security/test_fallback_rls_virtual_dataset.py **Line:** 76:94 **Comment:** *Api Mismatch: The helper accepts an in-memory `session` but persists through the global `db.session` instead. Consequently, the objects are created in a different database/session than the one whose metadata was initialized, so these tests can fail with missing tables or exercise the application database rather than the supplied isolated session. Use the passed `session` consistently for both `add_all` and `flush`. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43090&comment_hash=8937a47117023e23eecec16df00ecb84b771b1e389d932d371c3a6d90e340d77&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43090&comment_hash=8937a47117023e23eecec16df00ecb84b771b1e389d932d371c3a6d90e340d77&reaction=dislike'>👎</a> ########## tests/unit_tests/security/test_rls_evidence_sink.py: ########## @@ -0,0 +1,362 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Tests for the row-level-security enforcement evidence sink. + +The sink persists one ``rls_enforcement_evidence`` row per governed decision, +DECOUPLED from the decision itself: a sink failure is logged and swallowed and +must never change or block the enforcement outcome. An optional hash chain makes +the append-only trail tamper-evident when enabled by config; when disabled the +chain columns stay null and behavior is unchanged. + +Persistence is exercised against a real in-memory SQLite database created from +the model's own metadata; only the app config and identity/datasource seams are +mocked. +""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import MagicMock + +import pytest +from sqlalchemy import create_engine +from sqlalchemy.orm import Session, sessionmaker + +from superset.models.rls_evidence import RlsEnforcementEvidence +from superset.security import rls_enforcement as sink_mod +from superset.security.rls_enforcement import ( + EnforcementDecision, + EnforcementOutcome, + record_evidence, +) + +COMPILED_SQL = "SELECT col FROM governed_table" + + +def _sqlite_session() -> Session: + """Real SQLite session backed by the evidence model's own metadata.""" + engine = create_engine("sqlite://", future=True) + RlsEnforcementEvidence.__table__.create(bind=engine) + factory = sessionmaker(bind=engine, future=True) + return factory() Review Comment: **Suggestion:** The SQLite fixture creates the evidence table with the model's `BigInteger` primary key and relies on autoincrement. SQLite only auto-generates identifiers for an `INTEGER PRIMARY KEY`; this column is emitted as `BIGINT`, so inserts omit a required `id`, causing `record_evidence` to fail and swallow the error, leaving no rows for the evidence and hash-chain assertions. Use a SQLite-compatible integer primary key for this test table or explicitly provide identifiers. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Evidence persistence tests fail to observe inserted rows. - ❌ Hash-chain tests cannot create predecessor-linked records. - ⚠️ Sink failures are swallowed, masking the SQLite fixture defect. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b4e35153dfbe443db7eb06398e51adb1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b4e35153dfbe443db7eb06398e51adb1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/security/test_rls_evidence_sink.py **Line:** 50:55 **Comment:** *Type Error: The SQLite fixture creates the evidence table with the model's `BigInteger` primary key and relies on autoincrement. SQLite only auto-generates identifiers for an `INTEGER PRIMARY KEY`; this column is emitted as `BIGINT`, so inserts omit a required `id`, causing `record_evidence` to fail and swallow the error, leaving no rows for the evidence and hash-chain assertions. Use a SQLite-compatible integer primary key for this test table or explicitly provide identifiers. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43090&comment_hash=9972f35fc6a8a8245ea56366e95596df8cd66c7249d139b800935029423d2f73&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43090&comment_hash=9972f35fc6a8a8245ea56366e95596df8cd66c7249d139b800935029423d2f73&reaction=dislike'>👎</a> -- 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]
