diff --git a/superset/commands/security/create.py b/superset/commands/security/create.py index 7b11071fa46e..5345753c5782 100644 --- a/superset/commands/security/create.py +++ b/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 diff --git a/superset/commands/security/update.py b/superset/commands/security/update.py index 2295460fd44e..8673b6711dd7 100644 --- a/superset/commands/security/update.py +++ b/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 diff --git a/superset/daos/security.py b/superset/daos/security.py index 392d741e3d50..fc4b17db0d0e 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 7d903f92d880..39eda4f9dc5c 100644 --- a/superset/row_level_security/api.py +++ b/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", diff --git a/tests/integration_tests/security/row_level_security_tests.py b/tests/integration_tests/security/row_level_security_tests.py index a3f2c394152d..5eee03b13a56 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 f4ef07c3b378..3e1e285702ae 100644 --- a/tests/unit_tests/commands/security/rls_test.py +++ b/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",