codeant-ai-for-open-source[bot] commented on code in PR #43090: URL: https://github.com/apache/superset/pull/43090#discussion_r3766994625
########## tests/unit_tests/models/test_rls_evidence.py: ########## @@ -0,0 +1,228 @@ +# 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 append-only ``rls_enforcement_evidence`` table and model. + +The table is exercised against a real SQLite database (same engine family +Superset supports for its metadata store) via the model's own metadata: +the table is created, a row is inserted and read back, and the schema shape +(columns, types, nullability, indices) is asserted. A second test runs the +Alembic migration's ``upgrade``/``downgrade`` against a throwaway on-disk +SQLite database to prove the table is created and cleanly dropped. +""" + +from __future__ import annotations + +from datetime import datetime, timezone + +import sqlalchemy as sa +from sqlalchemy import create_engine, inspect +from sqlalchemy.orm import Session, sessionmaker + + +def _create_table(engine: sa.engine.Engine) -> None: + from superset.models.rls_evidence import RlsEnforcementEvidence + + RlsEnforcementEvidence.__table__.create(bind=engine) + + +def test_evidence_table_columns_and_nullability() -> None: + """@AC-FR10-01: the evidence table has the required columns with the + correct types and nullability.""" + from superset.models.rls_evidence import RlsEnforcementEvidence + + engine = create_engine("sqlite://", future=True) + _create_table(engine) + + inspector = inspect(engine) + columns = {c["name"]: c for c in inspector.get_columns("rls_enforcement_evidence")} + + expected_nullability = { + "id": False, + "ts": False, + "path": False, + "identity_handle": False, + "datasource_kind": False, + "datasource_id": True, + "outcome": False, + "applied_filter_count": False, + "denial_class": True, + "integrity_prev_hash": True, + "integrity_hash": True, + } + assert set(columns) == set(expected_nullability) + for name, nullable in expected_nullability.items(): + assert columns[name]["nullable"] is nullable, name + + # Primary key is id. + pk = inspector.get_pk_constraint("rls_enforcement_evidence") + assert pk["constrained_columns"] == ["id"] + + # No foreign keys — evidence must outlive rule deletion. + assert inspector.get_foreign_keys("rls_enforcement_evidence") == [] + + # Tablename constant on the model. + assert RlsEnforcementEvidence.__tablename__ == "rls_enforcement_evidence" + + +def test_evidence_table_indices() -> None: + """@AC-FR10-02: the three query indices are present.""" + engine = create_engine("sqlite://", future=True) + _create_table(engine) + + inspector = inspect(engine) + index_names = { + ix["name"] for ix in inspector.get_indexes("rls_enforcement_evidence") + } + assert { + "ix_rls_evidence_ts", + "ix_rls_evidence_identity_ts", + "ix_rls_evidence_outcome_ts", + } <= index_names + + +def test_evidence_row_round_trips() -> None: + """@AC-FR10-03: an evidence row inserts and reads back against a real DB.""" + from superset.models.rls_evidence import RlsEnforcementEvidence + + engine = create_engine("sqlite://", future=True) + _create_table(engine) + factory = sessionmaker(bind=engine, future=True) + session: Session = factory() + + ts = datetime(2026, 8, 7, 12, 0, 0, tzinfo=timezone.utc).replace(tzinfo=None) + row = RlsEnforcementEvidence( + ts=ts, + path="embedded_guest", + identity_handle="opaque-handle", + datasource_kind="query", + datasource_id=42, + outcome="applied", + applied_filter_count=2, + ) + session.add(row) + session.commit() Review Comment: **Suggestion:** The test creates the model on SQLite even though its auto-incrementing primary key is declared as `BigInteger`; SQLite only auto-generates row IDs for an exact `INTEGER PRIMARY KEY`, so the subsequent insert can fail with a NOT NULL constraint error instead of round-tripping. Use a SQLite-compatible integer variant for this test or run the model insert against a database that supports the declared type. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ RLS evidence model tests fail during SQLite insertion. - ⚠️ CI cannot validate evidence row persistence. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3d01b3ac533e4c3f9481bb17998cc266&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=3d01b3ac533e4c3f9481bb17998cc266&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/models/test_rls_evidence.py **Line:** 101:117 **Comment:** *Type Error: The test creates the model on SQLite even though its auto-incrementing primary key is declared as `BigInteger`; SQLite only auto-generates row IDs for an exact `INTEGER PRIMARY KEY`, so the subsequent insert can fail with a NOT NULL constraint error instead of round-tripping. Use a SQLite-compatible integer variant for this test or run the model insert against a database that supports the declared type. 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=cb9f5b6426bf2952eba990a09b905c7e719fca5f0ee06ff85549aae911eba850&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43090&comment_hash=cb9f5b6426bf2952eba990a09b905c7e719fca5f0ee06ff85549aae911eba850&reaction=dislike'>👎</a> ########## tests/unit_tests/security/test_rls_evidence_api.py: ########## @@ -0,0 +1,242 @@ +# 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 read-only RLS enforcement evidence query API. + +The endpoint exposes the append-only ``rls_enforcement_evidence`` audit trail to +an auditor/admin scope, gated by a dedicated FAB permission. It is queryable by +outcome, identity handle, and time range, and returns audit fields only — never +governed rows, rule/clause text, or SQL. + +These are app-factory HTTP tests: the request travels through the real +Flask-AppBuilder router and the ``@protect`` authorization decorator against a +metadata database provisioned on demand from the model's own metadata. +""" + +from __future__ import annotations + +from datetime import datetime +from typing import Any + +from pytest_mock import MockerFixture +from sqlalchemy.orm.session import Session + +from superset.models.rls_evidence import RlsEnforcementEvidence + +EVIDENCE_URL = "/api/v1/security/rls/enforcement-evidence/" + + +def _seed_evidence(session: Session) -> None: + """Provision the evidence table and a couple of audit rows.""" + from superset.security.rls_evidence_api import RlsEnforcementEvidenceRestApi + + RlsEnforcementEvidenceRestApi.datamodel._session = session + RlsEnforcementEvidence.metadata.create_all(session.get_bind()) + + session.add( + RlsEnforcementEvidence( + ts=datetime(2026, 8, 1, 10, 0, 0), + path="explore", + identity_handle="handle-applied-abc", + datasource_kind="query", + datasource_id=7, + outcome="applied", + applied_filter_count=2, + denial_class=None, + integrity_prev_hash=None, + integrity_hash="hash-1", + ) + ) + session.add( + RlsEnforcementEvidence( + ts=datetime(2026, 8, 2, 11, 0, 0), + path="embedded_guest", + identity_handle="handle-denied-xyz", + datasource_kind="query", + datasource_id=9, + outcome="denied", + applied_filter_count=0, + denial_class="cross_db_ref", + integrity_prev_hash="hash-1", + integrity_hash="hash-2", + ) + ) + session.commit() + + +# --------------------------------------------------------------------------- +# Authorized read +# --------------------------------------------------------------------------- + + +def test_auditor_can_list_evidence( + session: Session, + client: Any, + full_api_access: None, +) -> None: + """@AC-FR10-19: an authorized auditor lists enforcement evidence (200).""" + _seed_evidence(session) + + response = client.get(EVIDENCE_URL) + + assert response.status_code == 200 + payload = response.json + assert payload["count"] == 2 + outcomes = {row["outcome"] for row in payload["result"]} + assert outcomes == {"applied", "denied"} + + +def test_auditor_can_get_single_evidence_row( + session: Session, + client: Any, + full_api_access: None, +) -> None: + """@AC-FR10-20: an authorized auditor fetches a single evidence row (200).""" + _seed_evidence(session) + + listed = client.get(EVIDENCE_URL).json["result"] + pk = listed[0]["id"] + + response = client.get(f"{EVIDENCE_URL}{pk}") + + assert response.status_code == 200 + assert response.json["result"]["outcome"] in {"applied", "denied"} + + +def test_evidence_filterable_by_outcome( + session: Session, + client: Any, + full_api_access: None, +) -> None: + """@AC-FR10-21: results can be filtered to a single outcome class.""" + _seed_evidence(session) + + response = client.get( + f"{EVIDENCE_URL}?q=(filters:!((col:outcome,opr:eq,value:denied)))" + ) + + assert response.status_code == 200 + payload = response.json + assert payload["count"] == 1 + assert payload["result"][0]["outcome"] == "denied" + assert payload["result"][0]["denial_class"] == "cross_db_ref" + + +# --------------------------------------------------------------------------- +# Authorization (adversarial — the negative is the point) +# --------------------------------------------------------------------------- + + +def test_unauthorized_user_is_denied( + session: Session, + client: Any, + mocker: MockerFixture, +) -> None: + """@AC-FR10-22: a caller WITHOUT the auditor permission is denied (401/403). + + The permission is enforced, not decorative: authentication passes but the + evidence permission check fails, so no audit rows are disclosed. + """ + from superset import security_manager + + _seed_evidence(session) + + # Authentication succeeds; the resource is not public; the permission check + # for reading evidence fails. + mocker.patch( + "flask_appbuilder.security.decorators.verify_jwt_in_request", + return_value=True, + ) + mocker.patch.object(security_manager, "is_item_public", return_value=False) + mocker.patch.object(security_manager, "has_access", return_value=False) + + response = client.get(EVIDENCE_URL) + + assert response.status_code in (401, 403) Review Comment: **Suggestion:** This negative test never establishes an authenticated user: patching `verify_jwt_in_request` to return `True` does not populate the request's authentication context, and the plain test client remains anonymous. The endpoint can therefore return 401 before evaluating `has_access`, allowing the test to pass even if the dedicated permission check is broken. Authenticate a user with valid access to the application but without the evidence permission, then assert the permission denial. [security] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Authorization test can pass without checking permissions. - ❌ Dedicated evidence-permission regressions may go undetected. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=133c6420143f4b029c2aa194881d2077&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=133c6420143f4b029c2aa194881d2077&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_api.py **Line:** 159:168 **Comment:** *Security: This negative test never establishes an authenticated user: patching `verify_jwt_in_request` to return `True` does not populate the request's authentication context, and the plain test client remains anonymous. The endpoint can therefore return 401 before evaluating `has_access`, allowing the test to pass even if the dedicated permission check is broken. Authenticate a user with valid access to the application but without the evidence permission, then assert the permission denial. 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=692159ee637ea86fc028f8d7229187fcd9df102c5a35873a93bc3ba317bce533&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43090&comment_hash=692159ee637ea86fc028f8d7229187fcd9df102c5a35873a93bc3ba317bce533&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]
