You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jl...@apache.org on 2026/08/11 19:41:31 UTC
(superset) branch master updated: fix(rls): return a descriptive error for duplicate rule names (#42819)
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 998c4086867 fix(rls): return a descriptive error for duplicate rule names (#42819)
998c4086867 is described below
commit 998c4086867ef0110d104df9a3e3fae763a2be1a
Author: Joe Li <jo...@preset.io>
AuthorDate: Tue Aug 11 12:41:24 2026 -0700
fix(rls): return a descriptive error for duplicate rule names (#42819)
Co-authored-by: Claude Opus 4.8 <no...@anthropic.com>
Co-authored-by: rusackas <ev...@rusackas.com>
---
superset/commands/security/create.py | 43 ++++--
superset/commands/security/update.py | 57 ++++---
superset/daos/security.py | 20 ++-
superset/row_level_security/api.py | 4 +
.../security/row_level_security_tests.py | 53 +++++++
tests/unit_tests/commands/security/rls_test.py | 170 +++++++++++++++++++--
6 files changed, 307 insertions(+), 40 deletions(-)
diff --git a/superset/commands/security/create.py b/superset/commands/security/create.py
index 7b11071fa46..75096b70b1b 100644
--- a/superset/commands/security/create.py
+++ b/superset/commands/security/create.py
@@ -19,7 +19,9 @@
import logging
from typing import Any
+from flask_babel import gettext as _
from marshmallow import ValidationError
+from sqlalchemy.exc import IntegrityError
from superset.commands.base import BaseCommand
from superset.commands.exceptions import DatasourceNotFoundValidationError
@@ -43,9 +45,38 @@ class CreateRLSRuleCommand(BaseCommand):
@transaction()
def run(self) -> Any:
self.validate()
- return RLSDAO.create(attributes=self._properties)
+ try:
+ new_model = RLSDAO.create(attributes=self._properties)
+ db.session.flush()
+ except IntegrityError as ex:
+ # The preflight uniqueness check in ``validate`` isn't atomic with
+ # this insert, so fall back to the database's unique constraint
+ # and translate it into the same descriptive validation error.
+ raise ValidationError(
+ {"name": [_("A rule with this name already exists.")]}
+ ) from ex
+ return new_model
def validate(self) -> None:
+ # Datasource existence/access is validated before revealing whether
+ # the requested name is already in use, so an unauthorized caller
+ # can't use the duplicate-name response to enumerate rule names.
+ tables = (
+ db.session.query(SqlaTable)
+ .filter(SqlaTable.id.in_(self._tables)) # type: ignore[attr-defined]
+ .all()
+ )
+ if len(tables) != len(self._tables):
+ raise DatasourceNotFoundValidationError()
+ raise_for_datasource_access(tables)
+ self._properties["tables"] = tables
+
+ name = self._properties.get("name")
+ if name and not RLSDAO.validate_uniqueness(name):
+ raise ValidationError(
+ {"name": [_("A rule with this name already exists.")]}
+ )
+
if (
self._properties.get("filter_type")
== RowLevelSecurityFilterType.REGULAR.value
@@ -61,13 +92,3 @@ class CreateRLSRuleCommand(BaseCommand):
default_to_user=False,
)
self._properties["subjects"] = subjects
-
- tables = (
- db.session.query(SqlaTable)
- .filter(SqlaTable.id.in_(self._tables)) # type: ignore[attr-defined]
- .all()
- )
- if len(tables) != len(self._tables):
- raise DatasourceNotFoundValidationError()
- raise_for_datasource_access(tables)
- self._properties["tables"] = tables
diff --git a/superset/commands/security/update.py b/superset/commands/security/update.py
index 2295460fd44..68f59ffd22b 100644
--- a/superset/commands/security/update.py
+++ b/superset/commands/security/update.py
@@ -19,7 +19,9 @@
import logging
from typing import Any, Optional
+from flask_babel import gettext as _
from marshmallow import ValidationError
+from sqlalchemy.exc import IntegrityError
from superset.commands.base import BaseCommand
from superset.commands.exceptions import DatasourceNotFoundValidationError
@@ -47,13 +49,49 @@ class UpdateRLSRuleCommand(BaseCommand):
def run(self) -> Any:
self.validate()
assert self._model
- return RLSDAO.update(self._model, self._properties)
+ try:
+ updated_model = RLSDAO.update(self._model, self._properties)
+ db.session.flush()
+ except IntegrityError as ex:
+ # The preflight uniqueness check in ``validate`` isn't atomic with
+ # this update, so fall back to the database's unique constraint
+ # and translate it into the same descriptive validation error.
+ raise ValidationError(
+ {"name": [_("A rule with this name already exists.")]}
+ ) from ex
+ return updated_model
def validate(self) -> None:
self._model = RLSDAO.find_by_id(int(self._model_id))
if not self._model:
raise RLSRuleNotFoundError()
+ # Datasource access is validated before revealing whether the
+ # requested name is already in use, so an unauthorized caller can't
+ # use the duplicate-name response to enumerate rule names.
+ if "tables" in self._properties:
+ tables = (
+ db.session.query(SqlaTable)
+ .filter(SqlaTable.id.in_(self._tables)) # type: ignore[attr-defined]
+ .all()
+ )
+ if len(tables) != len(self._tables):
+ raise DatasourceNotFoundValidationError()
+ raise_for_datasource_access(tables)
+ self._properties["tables"] = tables
+ else:
+ # A partial update that omits ``tables`` still mutates the rule, so
+ # enforce datasource access against the rule's existing tables to
+ # avoid letting a caller edit a rule bound to datasources they
+ # cannot access.
+ raise_for_datasource_access(self._model.tables)
+
+ name = self._properties.get("name")
+ if name and not RLSDAO.validate_uniqueness(name, self._model.id):
+ raise ValidationError(
+ {"name": [_("A rule with this name already exists.")]}
+ )
+
# Only resolve and overwrite the relationships that are actually present
# in the request body. A partial update (e.g. changing only the name)
# must leave the rule's existing tables/subjects bindings untouched
@@ -76,20 +114,3 @@ class UpdateRLSRuleCommand(BaseCommand):
raise ValidationError(
{"subjects": ["Regular RLS filters require at least one subject."]}
)
-
- if "tables" in self._properties:
- tables = (
- db.session.query(SqlaTable)
- .filter(SqlaTable.id.in_(self._tables)) # type: ignore[attr-defined]
- .all()
- )
- if len(tables) != len(self._tables):
- raise DatasourceNotFoundValidationError()
- raise_for_datasource_access(tables)
- self._properties["tables"] = tables
- else:
- # A partial update that omits ``tables`` still mutates the rule, so
- # enforce datasource access against the rule's existing tables to
- # avoid letting a caller edit a rule bound to datasources they
- # cannot access.
- raise_for_datasource_access(self._model.tables)
diff --git a/superset/daos/security.py b/superset/daos/security.py
index 392d741e3d5..fc4b17db0d0 100644
--- a/superset/daos/security.py
+++ b/superset/daos/security.py
@@ -15,9 +15,27 @@
# specific language governing permissions and limitations
# under the License.
+from typing import Optional
+
from superset.connectors.sqla.models import RowLevelSecurityFilter
from superset.daos.base import BaseDAO
+from superset.extensions import db
class RLSDAO(BaseDAO[RowLevelSecurityFilter]):
- pass
+ @classmethod
+ def validate_uniqueness(cls, name: str, rule_id: Optional[int] = None) -> bool:
+ """
+ Validate that the RLS rule name is unique.
+
+ :param name: RLS rule name
+ :param rule_id: id of the rule being updated, excluded from the check so
+ that saving a rule without renaming it is not treated as a collision
+ :return: True if the name is unique, False otherwise
+ """
+ query = db.session.query(RowLevelSecurityFilter).filter(
+ RowLevelSecurityFilter.name == name
+ )
+ if rule_id is not None:
+ query = query.filter(RowLevelSecurityFilter.id != rule_id)
+ return not db.session.query(query.exists()).scalar()
diff --git a/superset/row_level_security/api.py b/superset/row_level_security/api.py
index 7d903f92d88..39eda4f9dc5 100644
--- a/superset/row_level_security/api.py
+++ b/superset/row_level_security/api.py
@@ -237,6 +237,8 @@ class RLSRestApi(BaseSupersetModelRestApi):
exc_info=True,
)
return self.response_422(message=str(ex))
+ except ValidationError as error:
+ return self.response_422(message=error.messages)
except RLSDatasourceForbiddenError as ex:
logger.warning(
"Forbidden datasource while creating RLS rule %s: %s",
@@ -330,6 +332,8 @@ class RLSRestApi(BaseSupersetModelRestApi):
exc_info=True,
)
return self.response_422(message=str(ex))
+ except ValidationError as error:
+ return self.response_422(message=error.messages)
except RLSDatasourceForbiddenError as ex:
logger.warning(
"Forbidden datasource while updating RLS rule %s: %s",
diff --git a/tests/integration_tests/security/row_level_security_tests.py b/tests/integration_tests/security/row_level_security_tests.py
index a3f2c394152..5eee03b13a5 100644
--- a/tests/integration_tests/security/row_level_security_tests.py
+++ b/tests/integration_tests/security/row_level_security_tests.py
@@ -715,6 +715,59 @@ def test_model_view_rls_add_name_unique(admin_client):
assert rv.status_code == 422
+@pytest.mark.usefixtures("create_dataset", "rls_filters")
+def test_model_view_rls_add_duplicate_name_error_is_descriptive(admin_client):
+ """Creating a rule with an existing name returns a descriptive message.
+
+ The duplicate is rejected before the DB write so the client receives the
+ reason instead of an empty/opaque ``IntegrityError`` string.
+ """
+ test_dataset = _get_test_dataset()
+ rv = admin_client.post(
+ "/api/v1/rowlevelsecurity/",
+ json={
+ "name": "rls_entry1",
+ "description": "Some description",
+ "filter_type": "Regular",
+ "tables": [test_dataset.id],
+ "subjects": [_subject_for_role(security_manager.find_role("Alpha")).id],
+ "group_key": "group_key_1",
+ "clause": "client_id=1",
+ },
+ )
+ assert rv.status_code == 422
+ data = json.loads(rv.data.decode("utf-8"))
+ assert data["message"] == {"name": ["A rule with this name already exists."]}
+
+
+@pytest.mark.usefixtures("create_dataset", "rls_filters")
+def test_model_view_rls_update_duplicate_name_error_is_descriptive(admin_client):
+ """Renaming a rule to another rule's name returns a descriptive message."""
+ rls_entry2 = (
+ db.session.query(RowLevelSecurityFilter).filter_by(name="rls_entry2")
+ ).one()
+ rv = admin_client.put(
+ f"/api/v1/rowlevelsecurity/{rls_entry2.id}",
+ json={"name": "rls_entry1"},
+ )
+ assert rv.status_code == 422
+ data = json.loads(rv.data.decode("utf-8"))
+ assert data["message"] == {"name": ["A rule with this name already exists."]}
+
+
+@pytest.mark.usefixtures("create_dataset", "rls_filters")
+def test_model_view_rls_update_same_name_succeeds(admin_client):
+ """Saving a rule without changing its name is not treated as a collision."""
+ rls_entry1 = (
+ db.session.query(RowLevelSecurityFilter).filter_by(name="rls_entry1")
+ ).one()
+ rv = admin_client.put(
+ f"/api/v1/rowlevelsecurity/{rls_entry1.id}",
+ json={"name": "rls_entry1"},
+ )
+ assert rv.status_code == 200
+
+
@pytest.mark.usefixtures("create_dataset", "rls_filters")
def test_model_view_rls_add_tables_required(admin_client):
rv = admin_client.post(
diff --git a/tests/unit_tests/commands/security/rls_test.py b/tests/unit_tests/commands/security/rls_test.py
index f4ef07c3b37..906ce4c1784 100644
--- a/tests/unit_tests/commands/security/rls_test.py
+++ b/tests/unit_tests/commands/security/rls_test.py
@@ -18,6 +18,7 @@ from unittest.mock import MagicMock, patch
import pytest
from marshmallow import ValidationError
+from sqlalchemy.exc import IntegrityError
from superset.commands.security.create import CreateRLSRuleCommand
from superset.commands.security.delete import DeleteRLSRuleCommand
@@ -93,20 +94,161 @@ def test_create_rls_rule_forbidden_if_any_datasource_denied() -> None:
def test_create_regular_rls_rule_requires_subjects() -> None:
- command = CreateRLSRuleCommand(
- {
- "filter_type": RowLevelSecurityFilterType.REGULAR.value,
- "tables": [1],
- "subjects": [],
- }
- )
-
- with pytest.raises(ValidationError) as exc:
- command.validate()
+ tables = _mock_tables(1)
+
+ with (
+ _patch_query("superset.commands.security.create", tables),
+ patch(
+ "superset.commands.security.utils.security_manager.can_access_datasource",
+ return_value=True,
+ ),
+ ):
+ command = CreateRLSRuleCommand(
+ {
+ "filter_type": RowLevelSecurityFilterType.REGULAR.value,
+ "tables": [1],
+ "subjects": [],
+ }
+ )
+
+ with pytest.raises(ValidationError) as exc:
+ command.validate()
assert "subjects" in exc.value.messages
+def test_create_rls_rule_rejects_duplicate_name() -> None:
+ tables = _mock_tables(1)
+
+ with (
+ _patch_query("superset.commands.security.create", tables),
+ patch(
+ "superset.commands.security.utils.security_manager.can_access_datasource",
+ return_value=True,
+ ),
+ patch(
+ "superset.commands.security.create.RLSDAO.validate_uniqueness",
+ return_value=False,
+ ) as validate_uniqueness,
+ ):
+ command = CreateRLSRuleCommand({"name": "dup", "tables": [1], "subjects": []})
+ with pytest.raises(ValidationError) as exc:
+ command.validate()
+
+ validate_uniqueness.assert_called_once_with("dup")
+ assert "name" in exc.value.messages
+
+
+def test_create_rls_rule_allows_unique_name() -> None:
+ tables = _mock_tables(1)
+
+ with (
+ _patch_query("superset.commands.security.create", tables),
+ patch(
+ "superset.commands.security.create.RLSDAO.validate_uniqueness",
+ return_value=True,
+ ),
+ patch(
+ "superset.commands.security.utils.security_manager.can_access_datasource",
+ return_value=True,
+ ),
+ ):
+ command = CreateRLSRuleCommand(
+ {"name": "unique", "tables": [1], "subjects": []}
+ )
+ command.validate()
+
+
+def test_create_rls_rule_translates_concurrent_duplicate_name() -> None:
+ """A unique-constraint violation surfaced at flush time (e.g. from a
+ concurrent request that slipped past the preflight check) must be
+ translated into the same descriptive validation error, not left as an
+ opaque database error.
+ """
+ command = CreateRLSRuleCommand({"name": "dup", "tables": [1], "subjects": []})
+
+ with (
+ patch.object(command, "validate"),
+ patch("superset.commands.security.create.RLSDAO.create"),
+ patch(
+ "superset.commands.security.create.db.session.flush",
+ side_effect=IntegrityError("stmt", {}, Exception("duplicate name")),
+ ),
+ ):
+ with pytest.raises(ValidationError) as exc:
+ command.run.__wrapped__(command)
+
+ assert "name" in exc.value.messages
+
+
+def test_update_rls_rule_translates_concurrent_duplicate_name() -> None:
+ """Same as the create-command race, but for the update path."""
+ command = UpdateRLSRuleCommand(1, {"name": "dup"})
+ command._model = MagicMock()
+
+ with (
+ patch.object(command, "validate"),
+ patch("superset.commands.security.update.RLSDAO.update"),
+ patch(
+ "superset.commands.security.update.db.session.flush",
+ side_effect=IntegrityError("stmt", {}, Exception("duplicate name")),
+ ),
+ ):
+ with pytest.raises(ValidationError) as exc:
+ command.run.__wrapped__(command)
+
+ assert "name" in exc.value.messages
+
+
+def test_update_rls_rule_rejects_duplicate_name() -> None:
+ rule = MagicMock()
+ rule.id = 1
+
+ with (
+ patch(
+ "superset.commands.security.update.RLSDAO.find_by_id",
+ return_value=rule,
+ ),
+ patch(
+ "superset.commands.security.update.RLSDAO.validate_uniqueness",
+ return_value=False,
+ ) as validate_uniqueness,
+ ):
+ command = UpdateRLSRuleCommand(1, {"name": "dup"})
+ with pytest.raises(ValidationError) as exc:
+ command.validate()
+
+ # The rule being updated is excluded from the uniqueness check.
+ validate_uniqueness.assert_called_once_with("dup", 1)
+ assert "name" in exc.value.messages
+
+
+def test_update_rls_rule_allows_unchanged_name() -> None:
+ """Saving a rule without renaming it must not be rejected as a duplicate."""
+ rule = MagicMock()
+ rule.id = 1
+ rule.tables = _mock_tables(1)
+
+ with (
+ patch(
+ "superset.commands.security.update.RLSDAO.find_by_id",
+ return_value=rule,
+ ),
+ patch(
+ "superset.commands.security.update.RLSDAO.validate_uniqueness",
+ return_value=True,
+ ) as validate_uniqueness,
+ patch(
+ "superset.commands.security.utils.security_manager.can_access_datasource",
+ return_value=True,
+ ),
+ ):
+ command = UpdateRLSRuleCommand(1, {"name": "same"})
+ command.validate()
+
+ validate_uniqueness.assert_called_once_with("same", 1)
+
+
def test_update_rls_rule_forbidden_when_no_datasource_access() -> None:
tables = _mock_tables(1)
@@ -182,6 +324,10 @@ def test_update_rls_rule_partial_update_preserves_tables_and_subjects() -> None:
patch(
"superset.commands.security.update.populate_subject_list",
) as populate_subject_list,
+ patch(
+ "superset.commands.security.update.RLSDAO.validate_uniqueness",
+ return_value=True,
+ ),
patch("superset.commands.security.update.db.session.query") as query,
patch(
"superset.commands.security.utils.security_manager.can_access_datasource",
@@ -241,6 +387,10 @@ def test_update_rls_rule_partial_update_enforces_access_on_existing_tables() ->
"superset.commands.security.update.RLSDAO.find_by_id",
return_value=rule,
),
+ patch(
+ "superset.commands.security.update.RLSDAO.validate_uniqueness",
+ return_value=True,
+ ),
patch("superset.commands.security.update.db.session.query") as query,
patch(
"superset.commands.security.utils.security_manager.can_access_datasource",