fix(rls): return a descriptive error for duplicate rule names

superset/commands/security/create.py

@@ -19,6 +19,7 @@
 import logging
 from typing import Any
 
+from flask_babel import gettext as _
 from marshmallow import ValidationError
 
 from superset.commands.base import BaseCommand
@@ -46,6 +47,12 @@ def run(self) -> Any:
         return RLSDAO.create(attributes=self._properties)
 
     def validate(self) -> None:
+        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

superset/commands/security/update.py

@@ -19,6 +19,7 @@
 import logging
 from typing import Any, Optional
 
+from flask_babel import gettext as _
 from marshmallow import ValidationError
 
 from superset.commands.base import BaseCommand
@@ -54,6 +55,12 @@ def validate(self) -> None:
         if not self._model:
             raise RLSRuleNotFoundError()
 
+        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

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()

superset/row_level_security/api.py

@@ -237,6 +237,8 @@ def post(self) -> Response:
                 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 @@ def put(self, pk: int) -> Response:
                 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",

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(

tests/unit_tests/commands/security/rls_test.py

@@ -107,6 +107,88 @@ def test_create_regular_rls_rule_requires_subjects() -> None:
     assert "subjects" in exc.value.messages
 
 
+def test_create_rls_rule_rejects_duplicate_name() -> None:
+    with 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_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 +264,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 +327,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",