dpgaspar commented on code in PR #22325: URL: https://github.com/apache/superset/pull/22325#discussion_r1048492703
########## superset-frontend/src/views/CRUD/rowlevelsecurity/types.ts: ########## @@ -0,0 +1,43 @@ +/** + * 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. + */ + +export enum FilterType { + REGULAR = 'Regular', + BASE = 'Base', +} + +export type RLSObject = { + id?: number; + name: string; + filter_type: FilterType; + tables?: MetaObject[]; + roles?: MetaObject[]; Review Comment: nit: can you make this more explicit, with a `RLSRoleObject[]` and `RLSTablesObject[]` for example ########## superset/row_level_security/commands/update.py: ########## @@ -0,0 +1,60 @@ +# 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. + + +import logging +from typing import Any, Dict, Optional + +from superset.commands.base import BaseCommand +from superset.connectors.sqla.models import RowLevelSecurityFilter, SqlaTable +from superset.dao.exceptions import DAOCreateFailedError +from superset.extensions import appbuilder, db, security_manager +from superset.row_level_security.commands.exceptions import RLSRuleNotFoundError +from superset.row_level_security.dao import RLSDAO + +logger = logging.getLogger(__name__) + + +class UpdateRLSRuleCommand(BaseCommand): + def __init__(self, model_id: int, data: Dict[str, Any]): + self._model_id = model_id + self._properties = data.copy() + self._model: Optional[RowLevelSecurityFilter] = None + + def run(self) -> Any: + self.validate() + try: + rule = RLSDAO.update(self._model, self._properties) + except DAOCreateFailedError as ex: Review Comment: this should be `DAOUpdateFailedError` ########## superset/row_level_security/commands/create.py: ########## @@ -0,0 +1,53 @@ +# 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. + + +import logging +from typing import Any, Dict + +from superset.commands.base import BaseCommand +from superset.connectors.sqla.models import RowLevelSecurityFilter, SqlaTable +from superset.dao.exceptions import DAOCreateFailedError +from superset.extensions import appbuilder, db, security_manager +from superset.row_level_security.dao import RLSDAO + +logger = logging.getLogger(__name__) + + +class CreateRLSRuleCommand(BaseCommand): + def __init__(self, data: Dict[str, Any]): + self._properties = data.copy() + + def run(self) -> Any: + self.validate() + try: + rule = RLSDAO.create(self._properties) + except DAOCreateFailedError as ex: + logger.exception(ex.exception) + raise DAOCreateFailedError + + return rule + + def validate(self) -> None: + roles = security_manager.find_roles_by_id(self._properties.get("roles", [])) Review Comment: better to use `populate_roles` from https://github.com/apache/superset/blob/master/superset/commands/utils.py#L66 free validation and raises if a role does not exist ########## superset/row_level_security/api.py: ########## @@ -0,0 +1,306 @@ +# 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. + +import logging +from typing import Any + +from flask import request, Response +from flask_appbuilder.api import expose, protect, rison, safe +from flask_appbuilder.models.sqla.interface import SQLAInterface +from flask_babel import ngettext +from marshmallow import ValidationError + +from superset import app +from superset.connectors.sqla.models import RowLevelSecurityFilter +from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod +from superset.dao.exceptions import DAOCreateFailedError +from superset.extensions import event_logger +from superset.row_level_security.commands.bulk_delete import BulkDeleteRLSRuleCommand +from superset.row_level_security.commands.create import CreateRLSRuleCommand +from superset.row_level_security.commands.exceptions import RLSRuleNotFoundError +from superset.row_level_security.commands.update import UpdateRLSRuleCommand +from superset.row_level_security.schemas import ( + get_delete_ids_schema, + RLSListSchema, + RLSPostSchema, + RLSPutSchema, + RLSShowSchema, +) +from superset.views.base_api import ( + BaseSupersetModelRestApi, + requires_json, + statsd_metrics, +) + +logger = logging.getLogger(__name__) + + +class RLSRestApi(BaseSupersetModelRestApi): + datamodel = SQLAInterface(RowLevelSecurityFilter) + include_route_methods = RouteMethod.REST_MODEL_VIEW_CRUD_SET | { + RouteMethod.RELATED, + "bulk_delete", + } + resource_name = "rowlevelsecurity" + class_permission_name = "Row Level Security" + openapi_spec_tag = "Row Level Security" + method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP + allow_browser_login = True + + list_columns = [ + "id", + "name", + "filter_type", + "tables.id", + "tables.table_name", + "roles.id", + "roles.name", + "clause", + "changed_on_delta_humanized", + "group_key", + ] + order_columns = ["name", "filter_type", "clause", "modified"] + add_columns = [ + "name", + "description", + "filter_type", + "tables", + "roles", + "group_key", + "clause", + ] + show_columns = [ + "name", + "description", + "filter_type", + "tables.id", + "tables.table_name", + "roles.id", + "roles.name", + "group_key", + "clause", + ] + search_columns = ( + "name", + "description", + "filter_type", + "tables", + "roles", + "group_key", + "clause", + ) + edit_columns = add_columns + + show_model_schema = RLSShowSchema() + list_model_schema = RLSListSchema() + add_model_schema = RLSPostSchema() + edit_model_schema = RLSPutSchema() + + allowed_rel_fields = {"tables", "roles"} + filter_rel_fields = app.config["RLS_FILTER_RELATED_FIELDS"] + + @expose("/", methods=["POST"]) + @protect() + @safe + @statsd_metrics + @requires_json + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.post", + log_to_statsd=False, + ) + def post(self) -> Response: + """Creates a new RLS rule + --- + post: + description: >- + Create a new RLS Rule + requestBody: + description: RLS schema + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/{{self.__class__.__name__}}.post' + responses: + 201: + description: RLS Rule added + content: + application/json: + schema: + type: object + properties: + id: + type: number + result: + $ref: '#/components/schemas/{{self.__class__.__name__}}.post' + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + try: + item = self.add_model_schema.load(request.json) + except ValidationError as error: + return self.response_400(message=error.messages) + + try: + new_model = CreateRLSRuleCommand(item).run() + return self.response(201, id=new_model.id, result=item) + except DAOCreateFailedError as ex: + logger.error( + "Error creating RLS rule %s: %s", + self.__class__.__name__, + str(ex), + exc_info=True, + ) + return self.response_422(message=str(ex)) + + @expose("/<int:pk>", methods=["PUT"]) + @protect() + @safe + @statsd_metrics + @requires_json + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.put", + log_to_statsd=False, + ) + def put(self, pk: int) -> Response: + """Updates an RLS Rule + --- + put: + description: >- + Updates an RLS Rule + parameters: + - in: path + schema: + type: integer + name: pk + description: The Rule pk + requestBody: + description: RLS schema + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/{{self.__class__.__name__}}.put' + responses: + 200: + description: Rule changed + content: + application/json: + schema: + type: object + properties: + id: + type: number + result: + $ref: '#/components/schemas/{{self.__class__.__name__}}.put' + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + + try: + item = self.edit_model_schema.load(request.json) + except ValidationError as error: + return self.response_400(message=error.messages) + + try: + new_model = UpdateRLSRuleCommand(pk, item).run() + return self.response(201, id=new_model.id, result=item) + except DAOCreateFailedError as ex: Review Comment: this should be `DAOUpdateFailedError` ########## tests/integration_tests/security/row_level_security_tests.py: ########## @@ -304,6 +306,45 @@ def test_rls_filter_doesnt_alter_admin_birth_names_query(self): assert not self.BASE_FILTER_REGEX.search(sql) +class TestRowLevelSecurityWithRelatedFilters(SupersetTestCase): + @pytest.mark.usefixtures("load_birth_names_data") + @pytest.mark.usefixtures("load_energy_table_data") + def test_rls_tables_related_api(self): + self.login("Admin") + + params = prison.dumps({"page": 0, "page_size": 100}) + + rv = self.client.get(f"/api/v1/rowlevelsecurity/related/tables?q={params}") + self.assertEqual(rv.status_code, 200) + data = json.loads(rv.data.decode("utf-8")) + result = data["result"] + + db_tables = db.session.query(SqlaTable).all() + + db_table_names = set([t.table_name for t in db_tables]) + received_tables = set([table["text"].split(".")[-1] for table in result]) + + assert data["count"] == len(db_tables) + assert len(result) == len(db_tables) + assert db_table_names == received_tables + + def test_rls_roles_related_api(self): + self.login("Admin") + params = prison.dumps({"page": 0, "page_size": 100}) + + rv = self.client.get(f"/api/v1/rowlevelsecurity/related/roles?q={params}") + self.assertEqual(rv.status_code, 200) + data = json.loads(rv.data.decode("utf-8")) + result = data["result"] + + db_role_names = set([r.name for r in security_manager.get_all_roles()]) + received_roles = set([role["text"] for role in result]) + + assert data["count"] == len(db_role_names) + assert len(result) == len(db_role_names) + assert db_role_names == received_roles Review Comment: can you add more detailed tests to the REST API, similar to dashboard, chart etc? ########## superset/initialization/__init__.py: ########## @@ -215,6 +216,7 @@ def init_views(self) -> None: appbuilder.add_api(ReportScheduleRestApi) appbuilder.add_api(ReportExecutionLogRestApi) appbuilder.add_api(SavedQueryRestApi) + appbuilder.add_api(RLSRestApi) Review Comment: nit: place it before `appbuilder.add_api(SavedQueryRestApi)` order is nice ;) ########## superset/row_level_security/commands/update.py: ########## @@ -0,0 +1,60 @@ +# 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. + + +import logging +from typing import Any, Dict, Optional + +from superset.commands.base import BaseCommand +from superset.connectors.sqla.models import RowLevelSecurityFilter, SqlaTable +from superset.dao.exceptions import DAOCreateFailedError +from superset.extensions import appbuilder, db, security_manager +from superset.row_level_security.commands.exceptions import RLSRuleNotFoundError +from superset.row_level_security.dao import RLSDAO + +logger = logging.getLogger(__name__) + + +class UpdateRLSRuleCommand(BaseCommand): + def __init__(self, model_id: int, data: Dict[str, Any]): + self._model_id = model_id + self._properties = data.copy() + self._model: Optional[RowLevelSecurityFilter] = None + + def run(self) -> Any: + self.validate() + try: + rule = RLSDAO.update(self._model, self._properties) + except DAOCreateFailedError as ex: + logger.exception(ex.exception) + raise DAOCreateFailedError + + return rule + + def validate(self) -> None: + self._model = RLSDAO.find_by_id(self._model_id) + if not self._model: + raise RLSRuleNotFoundError() + + roles = security_manager.find_roles_by_id(self._properties.get("roles", [])) Review Comment: better to use `populate_roles` from https://github.com/apache/superset/blob/master/superset/commands/utils.py#L66 free validation and raises if a role does not exist ########## superset/connectors/sqla/models.py: ########## @@ -2279,6 +2279,10 @@ class RowLevelSecurityFilter(Model, AuditMixinNullable): id = Column(Integer, primary_key=True) name = Column(String(255), unique=True, nullable=False) description = Column(Text) + # filter_type = Column( + # Enum(utils.RowLevelSecurityFilterType), + # info={"enum_class": utils.RowLevelSecurityFilterType}, + # ) Review Comment: delete? ########## superset/row_level_security/commands/update.py: ########## @@ -0,0 +1,60 @@ +# 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. + + +import logging +from typing import Any, Dict, Optional + +from superset.commands.base import BaseCommand +from superset.connectors.sqla.models import RowLevelSecurityFilter, SqlaTable +from superset.dao.exceptions import DAOCreateFailedError +from superset.extensions import appbuilder, db, security_manager +from superset.row_level_security.commands.exceptions import RLSRuleNotFoundError +from superset.row_level_security.dao import RLSDAO + +logger = logging.getLogger(__name__) + + +class UpdateRLSRuleCommand(BaseCommand): + def __init__(self, model_id: int, data: Dict[str, Any]): + self._model_id = model_id + self._properties = data.copy() + self._model: Optional[RowLevelSecurityFilter] = None + + def run(self) -> Any: + self.validate() + try: + rule = RLSDAO.update(self._model, self._properties) + except DAOCreateFailedError as ex: + logger.exception(ex.exception) + raise DAOCreateFailedError + + return rule + + def validate(self) -> None: + self._model = RLSDAO.find_by_id(self._model_id) + if not self._model: + raise RLSRuleNotFoundError() + + roles = security_manager.find_roles_by_id(self._properties.get("roles", [])) + tables = ( + db.session.query(SqlaTable) + .filter(SqlaTable.id.in_(self._properties.get("tables", []))) + .all() + ) Review Comment: better to check if a table does not exist -- 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]
