From 462e7db2e45db414e3894cc7c6459159e00531e3 Mon Sep 17 00:00:00 2001 From: Kasia Kucharczyk Date: Fri, 11 Dec 2020 18:47:52 +0100 Subject: [PATCH 1/9] Changed security permissions for annotations and annotation layers --- superset/annotation_layers/annotations/api.py | 6 ++++-- superset/annotation_layers/api.py | 6 ++++-- superset/views/annotations.py | 9 ++++++++- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/superset/annotation_layers/annotations/api.py b/superset/annotation_layers/annotations/api.py index d7241b650e696..1c63f699d51c6 100644 --- a/superset/annotation_layers/annotations/api.py +++ b/superset/annotation_layers/annotations/api.py @@ -52,7 +52,7 @@ openapi_spec_methods_override, ) from superset.annotation_layers.commands.exceptions import AnnotationLayerNotFoundError -from superset.constants import RouteMethod +from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod from superset.models.annotations import Annotation from superset.views.base_api import BaseSupersetModelRestApi, statsd_metrics @@ -65,7 +65,9 @@ class AnnotationRestApi(BaseSupersetModelRestApi): include_route_methods = RouteMethod.REST_MODEL_VIEW_CRUD_SET | { "bulk_delete", # not using RouteMethod since locally defined } - class_permission_name = "AnnotationLayerModelView" + class_permission_name = "Annotation" + method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP + resource_name = "annotation_layer" allow_browser_login = True diff --git a/superset/annotation_layers/api.py b/superset/annotation_layers/api.py index c608e30157d56..b47bf87ecb891 100644 --- a/superset/annotation_layers/api.py +++ b/superset/annotation_layers/api.py @@ -46,7 +46,7 @@ get_delete_ids_schema, openapi_spec_methods_override, ) -from superset.constants import RouteMethod +from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod from superset.extensions import event_logger from superset.models.annotations import AnnotationLayer from superset.views.base_api import BaseSupersetModelRestApi, statsd_metrics @@ -61,7 +61,9 @@ class AnnotationLayerRestApi(BaseSupersetModelRestApi): RouteMethod.RELATED, "bulk_delete", # not using RouteMethod since locally defined } - class_permission_name = "AnnotationLayerModelView" + class_permission_name = "Annotation" + method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP + resource_name = "annotation_layer" allow_browser_login = True diff --git a/superset/views/annotations.py b/superset/views/annotations.py index 03cc26006ff35..345fd2c15a1f0 100644 --- a/superset/views/annotations.py +++ b/superset/views/annotations.py @@ -24,7 +24,7 @@ from wtforms.validators import StopValidation from superset import is_feature_enabled -from superset.constants import RouteMethod +from superset.constants import MODEL_VIEW_RW_METHOD_PERMISSION_MAP, RouteMethod from superset.models.annotations import Annotation, AnnotationLayer from superset.typing import FlaskResponse from superset.views.base import SupersetModelView @@ -54,6 +54,9 @@ class AnnotationModelView( datamodel = SQLAInterface(Annotation) include_route_methods = RouteMethod.CRUD_SET | {"annotation"} + class_permission_name = "Annotation" + method_permission_name = MODEL_VIEW_RW_METHOD_PERMISSION_MAP + list_title = _("Annotations") show_title = _("Show Annotation") add_title = _("Add Annotation") @@ -109,6 +112,10 @@ class AnnotationLayerModelView(SupersetModelView): # pylint: disable=too-many-a datamodel = SQLAInterface(AnnotationLayer) include_route_methods = RouteMethod.CRUD_SET | {RouteMethod.API_READ} related_views = [AnnotationModelView] + + class_permission_name = "Annotation" + method_permission_name = MODEL_VIEW_RW_METHOD_PERMISSION_MAP + list_title = _("Annotation Layers") show_title = _("Show Annotation Layer") add_title = _("Add Annotation Layer") From 4d249bbd7b7c7366b615e3e6939ee36722b95640 Mon Sep 17 00:00:00 2001 From: Kasia Kucharczyk Date: Fri, 11 Dec 2020 18:49:06 +0100 Subject: [PATCH 2/9] Updated permissions in annotation layers list --- .../views/CRUD/annotationlayers/AnnotationLayersList.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/superset-frontend/src/views/CRUD/annotationlayers/AnnotationLayersList.tsx b/superset-frontend/src/views/CRUD/annotationlayers/AnnotationLayersList.tsx index d066d4dd12779..92bf9bd586e07 100644 --- a/superset-frontend/src/views/CRUD/annotationlayers/AnnotationLayersList.tsx +++ b/superset-frontend/src/views/CRUD/annotationlayers/AnnotationLayersList.tsx @@ -114,9 +114,9 @@ function AnnotationLayersList({ ); }; - const canCreate = hasPerm('can_add'); - const canEdit = hasPerm('can_edit'); - const canDelete = hasPerm('can_delete'); + const canCreate = hasPerm('can_write'); + const canEdit = hasPerm('can_write'); + const canDelete = hasPerm('can_write'); function handleAnnotationLayerEdit(layer: AnnotationLayerObject | null) { setCurrentAnnotationLayer(layer); From afd37a1a8f5798534a7d2764a820c8055721d791 Mon Sep 17 00:00:00 2001 From: Kasia Kucharczyk Date: Fri, 11 Dec 2020 18:50:51 +0100 Subject: [PATCH 3/9] Created test for retrieving premissions info. Updated uris from f-strings to strings --- tests/annotation_layers/api_tests.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/tests/annotation_layers/api_tests.py b/tests/annotation_layers/api_tests.py index 0ee361bcfbd1b..b53624394ef46 100644 --- a/tests/annotation_layers/api_tests.py +++ b/tests/annotation_layers/api_tests.py @@ -75,10 +75,24 @@ def test_info_annotation(self): Annotation API: Test info """ self.login(username="admin") - uri = f"api/v1/annotation_layer/_info" + uri = "api/v1/annotation_layer/_info" rv = self.get_assert_metric(uri, "info") assert rv.status_code == 200 + def test_info_security_query(self): + """ + Annotation API: Test info security + """ + self.login(username="admin") + params = {"keys": ["permissions"]} + uri = f"api/v1/annotation_layer/_info?q={prison.dumps(params)}" + rv = self.get_assert_metric(uri, "info") + data = json.loads(rv.data.decode("utf-8")) + assert rv.status_code == 200 + assert "can_read" in data["permissions"] + assert "can_write" in data["permissions"] + assert len(data["permissions"]) == 2 + @pytest.mark.usefixtures("create_annotation_layers") def test_get_annotation_layer_not_found(self): """ @@ -96,7 +110,7 @@ def test_get_list_annotation_layer(self): Annotation Api: Test get list annotation layers """ self.login(username="admin") - uri = f"api/v1/annotation_layer/" + uri = "api/v1/annotation_layer/" rv = self.get_assert_metric(uri, "get_list") expected_fields = [ @@ -120,7 +134,7 @@ def test_get_list_annotation_layer_sorting(self): Annotation Api: Test sorting on get list annotation layers """ self.login(username="admin") - uri = f"api/v1/annotation_layer/" + uri = "api/v1/annotation_layer/" order_columns = [ "name", From dae9b5b627bb738c5a53743a95d58b7c29e25bac Mon Sep 17 00:00:00 2001 From: Kasia Kucharczyk Date: Fri, 11 Dec 2020 18:54:35 +0100 Subject: [PATCH 4/9] Updated annotations in security_tests and added annotations to NEW_SECURITY_CONVERGE_VIEWS --- tests/security_tests.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/security_tests.py b/tests/security_tests.py index 4ef63922f7c80..2836b48501266 100644 --- a/tests/security_tests.py +++ b/tests/security_tests.py @@ -48,7 +48,7 @@ from .fixtures.energy_dashboard import load_energy_table_with_slice from .fixtures.unicode_dashboard import load_unicode_dashboard_with_slice -NEW_SECURITY_CONVERGE_VIEWS = ("CssTemplate", "SavedQuery", "Chart") +NEW_SECURITY_CONVERGE_VIEWS = ("CssTemplate", "SavedQuery", "Chart", "Annotation") def get_perm_tuples(role_name): @@ -672,7 +672,7 @@ def assert_can_gamma(self, perm_set): self.assert_can_menu("Dashboards", perm_set) def assert_can_alpha(self, perm_set): - self.assert_can_all("AnnotationLayerModelView", perm_set) + self.assert_can_all("Annotation", perm_set) self.assert_can_all("CssTemplate", perm_set) self.assert_can_all("TableModelView", perm_set) self.assert_can_read("QueryView", perm_set) From ef6aa5e271a5f5c410869888f0b05abe2d298d45 Mon Sep 17 00:00:00 2001 From: Kasia Kucharczyk Date: Fri, 11 Dec 2020 18:58:18 +0100 Subject: [PATCH 5/9] Added migration for annotations security converge --- ...cb2c78727_security_coverage_annotations.py | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 superset/migrations/versions/c25cb2c78727_security_coverage_annotations.py diff --git a/superset/migrations/versions/c25cb2c78727_security_coverage_annotations.py b/superset/migrations/versions/c25cb2c78727_security_coverage_annotations.py new file mode 100644 index 0000000000000..2f98de22db7a0 --- /dev/null +++ b/superset/migrations/versions/c25cb2c78727_security_coverage_annotations.py @@ -0,0 +1,84 @@ +# 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. +"""security_coverage_annotations + +Revision ID: c25cb2c78727 +Revises: 811494c0cc23 +Create Date: 2020-12-11 17:02:21.213046 + +""" + +from alembic import op +from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.orm import Session + +# revision identifiers, used by Alembic. +from superset.migrations.shared.security_converge import ( + add_pvms, + get_reversed_new_pvms, + get_reversed_pvm_map, + migrate_roles, + Pvm, +) + +revision = "c25cb2c78727" +down_revision = "811494c0cc23" + + +NEW_PVMS = {"Annotation": ("can_read", "can_write",)} +PVM_MAP = { + Pvm("AnnotationLayerModelView", "can_delete"): (Pvm("Annotation", "can_write"),), + Pvm("AnnotationLayerModelView", "can_list"): (Pvm("Annotation", "can_read"),), + Pvm("AnnotationLayerModelView", "can_show",): (Pvm("Annotation", "can_read"),), + Pvm("AnnotationLayerModelView", "can_add",): (Pvm("Annotation", "can_write"),), + Pvm("AnnotationLayerModelView", "can_edit",): (Pvm("Annotation", "can_write"),), + Pvm("AnnotationModelView", "can_annotation",): (Pvm("Annotation", "can_read"),), + Pvm("AnnotationModelView", "can_show",): (Pvm("Annotation", "can_read"),), + Pvm("AnnotationModelView", "can_add",): (Pvm("Annotation", "can_write"),), + Pvm("AnnotationModelView", "can_delete",): (Pvm("Annotation", "can_write"),), + Pvm("AnnotationModelView", "can_edit",): (Pvm("Annotation", "can_write"),), + Pvm("AnnotationModelView", "can_list",): (Pvm("Annotation", "can_read"),), +} + + +def upgrade(): + bind = op.get_bind() + session = Session(bind=bind) + + # Add the new permissions on the migration itself + add_pvms(session, NEW_PVMS) + migrate_roles(session, PVM_MAP) + try: + session.commit() + except SQLAlchemyError as ex: + print(f"An error occurred while upgrading annotation permissions: {ex}") + session.rollback() + + +def downgrade(): + bind = op.get_bind() + session = Session(bind=bind) + + # Add the old permissions on the migration itself + add_pvms(session, get_reversed_new_pvms(PVM_MAP)) + migrate_roles(session, get_reversed_pvm_map(PVM_MAP)) + try: + session.commit() + except SQLAlchemyError as ex: + print(f"An error occurred while downgrading annotation permissions: {ex}") + session.rollback() + pass From 2343958559ad54c98b0533f4bf3949b215396e3a Mon Sep 17 00:00:00 2001 From: Kasia Kucharczyk Date: Mon, 14 Dec 2020 11:55:20 +0100 Subject: [PATCH 6/9] Updated current revision after rebase master --- ...cb2c78727_security_converge_annotations.py | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 superset/migrations/versions/c25cb2c78727_security_converge_annotations.py diff --git a/superset/migrations/versions/c25cb2c78727_security_converge_annotations.py b/superset/migrations/versions/c25cb2c78727_security_converge_annotations.py new file mode 100644 index 0000000000000..4860b5a63dcd8 --- /dev/null +++ b/superset/migrations/versions/c25cb2c78727_security_converge_annotations.py @@ -0,0 +1,84 @@ +# 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. +"""security_coverage_annotations + +Revision ID: c25cb2c78727 +Revises: 811494c0cc23 +Create Date: 2020-12-11 17:02:21.213046 + +""" + +from alembic import op +from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.orm import Session + +# revision identifiers, used by Alembic. +from superset.migrations.shared.security_converge import ( + add_pvms, + get_reversed_new_pvms, + get_reversed_pvm_map, + migrate_roles, + Pvm, +) + +revision = "c25cb2c78727" +down_revision = "5daced1f0e76" + + +NEW_PVMS = {"Annotation": ("can_read", "can_write",)} +PVM_MAP = { + Pvm("AnnotationLayerModelView", "can_delete"): (Pvm("Annotation", "can_write"),), + Pvm("AnnotationLayerModelView", "can_list"): (Pvm("Annotation", "can_read"),), + Pvm("AnnotationLayerModelView", "can_show",): (Pvm("Annotation", "can_read"),), + Pvm("AnnotationLayerModelView", "can_add",): (Pvm("Annotation", "can_write"),), + Pvm("AnnotationLayerModelView", "can_edit",): (Pvm("Annotation", "can_write"),), + Pvm("AnnotationModelView", "can_annotation",): (Pvm("Annotation", "can_read"),), + Pvm("AnnotationModelView", "can_show",): (Pvm("Annotation", "can_read"),), + Pvm("AnnotationModelView", "can_add",): (Pvm("Annotation", "can_write"),), + Pvm("AnnotationModelView", "can_delete",): (Pvm("Annotation", "can_write"),), + Pvm("AnnotationModelView", "can_edit",): (Pvm("Annotation", "can_write"),), + Pvm("AnnotationModelView", "can_list",): (Pvm("Annotation", "can_read"),), +} + + +def upgrade(): + bind = op.get_bind() + session = Session(bind=bind) + + # Add the new permissions on the migration itself + add_pvms(session, NEW_PVMS) + migrate_roles(session, PVM_MAP) + try: + session.commit() + except SQLAlchemyError as ex: + print(f"An error occurred while upgrading annotation permissions: {ex}") + session.rollback() + + +def downgrade(): + bind = op.get_bind() + session = Session(bind=bind) + + # Add the old permissions on the migration itself + add_pvms(session, get_reversed_new_pvms(PVM_MAP)) + migrate_roles(session, get_reversed_pvm_map(PVM_MAP)) + try: + session.commit() + except SQLAlchemyError as ex: + print(f"An error occurred while downgrading annotation permissions: {ex}") + session.rollback() + pass From 0b2888bd82137011d239545bcb020cf13b2d4def Mon Sep 17 00:00:00 2001 From: Kasia Kucharczyk Date: Mon, 14 Dec 2020 11:59:07 +0100 Subject: [PATCH 7/9] Updated migration name to annotations security converge --- ...cb2c78727_security_converge_annotations.py | 2 +- ...cb2c78727_security_coverage_annotations.py | 84 ------------------- 2 files changed, 1 insertion(+), 85 deletions(-) delete mode 100644 superset/migrations/versions/c25cb2c78727_security_coverage_annotations.py diff --git a/superset/migrations/versions/c25cb2c78727_security_converge_annotations.py b/superset/migrations/versions/c25cb2c78727_security_converge_annotations.py index 4860b5a63dcd8..0a025933a193a 100644 --- a/superset/migrations/versions/c25cb2c78727_security_converge_annotations.py +++ b/superset/migrations/versions/c25cb2c78727_security_converge_annotations.py @@ -14,7 +14,7 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -"""security_coverage_annotations +"""security_converge_annotations Revision ID: c25cb2c78727 Revises: 811494c0cc23 diff --git a/superset/migrations/versions/c25cb2c78727_security_coverage_annotations.py b/superset/migrations/versions/c25cb2c78727_security_coverage_annotations.py deleted file mode 100644 index 2f98de22db7a0..0000000000000 --- a/superset/migrations/versions/c25cb2c78727_security_coverage_annotations.py +++ /dev/null @@ -1,84 +0,0 @@ -# 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. -"""security_coverage_annotations - -Revision ID: c25cb2c78727 -Revises: 811494c0cc23 -Create Date: 2020-12-11 17:02:21.213046 - -""" - -from alembic import op -from sqlalchemy.exc import SQLAlchemyError -from sqlalchemy.orm import Session - -# revision identifiers, used by Alembic. -from superset.migrations.shared.security_converge import ( - add_pvms, - get_reversed_new_pvms, - get_reversed_pvm_map, - migrate_roles, - Pvm, -) - -revision = "c25cb2c78727" -down_revision = "811494c0cc23" - - -NEW_PVMS = {"Annotation": ("can_read", "can_write",)} -PVM_MAP = { - Pvm("AnnotationLayerModelView", "can_delete"): (Pvm("Annotation", "can_write"),), - Pvm("AnnotationLayerModelView", "can_list"): (Pvm("Annotation", "can_read"),), - Pvm("AnnotationLayerModelView", "can_show",): (Pvm("Annotation", "can_read"),), - Pvm("AnnotationLayerModelView", "can_add",): (Pvm("Annotation", "can_write"),), - Pvm("AnnotationLayerModelView", "can_edit",): (Pvm("Annotation", "can_write"),), - Pvm("AnnotationModelView", "can_annotation",): (Pvm("Annotation", "can_read"),), - Pvm("AnnotationModelView", "can_show",): (Pvm("Annotation", "can_read"),), - Pvm("AnnotationModelView", "can_add",): (Pvm("Annotation", "can_write"),), - Pvm("AnnotationModelView", "can_delete",): (Pvm("Annotation", "can_write"),), - Pvm("AnnotationModelView", "can_edit",): (Pvm("Annotation", "can_write"),), - Pvm("AnnotationModelView", "can_list",): (Pvm("Annotation", "can_read"),), -} - - -def upgrade(): - bind = op.get_bind() - session = Session(bind=bind) - - # Add the new permissions on the migration itself - add_pvms(session, NEW_PVMS) - migrate_roles(session, PVM_MAP) - try: - session.commit() - except SQLAlchemyError as ex: - print(f"An error occurred while upgrading annotation permissions: {ex}") - session.rollback() - - -def downgrade(): - bind = op.get_bind() - session = Session(bind=bind) - - # Add the old permissions on the migration itself - add_pvms(session, get_reversed_new_pvms(PVM_MAP)) - migrate_roles(session, get_reversed_pvm_map(PVM_MAP)) - try: - session.commit() - except SQLAlchemyError as ex: - print(f"An error occurred while downgrading annotation permissions: {ex}") - session.rollback() - pass From e79d9929c3cd288fecc638ba4314ec181386df41 Mon Sep 17 00:00:00 2001 From: Kasia Kucharczyk Date: Tue, 15 Dec 2020 17:20:49 +0100 Subject: [PATCH 8/9] Updated annotations permissions names in AnnotationLayersList and updated test since 'can_write' has wider permissions --- .../views/CRUD/annotationlayers/AnnotationLayersList_spec.jsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset-frontend/spec/javascripts/views/CRUD/annotationlayers/AnnotationLayersList_spec.jsx b/superset-frontend/spec/javascripts/views/CRUD/annotationlayers/AnnotationLayersList_spec.jsx index 7a4847007e07e..fa6adddae69af 100644 --- a/superset-frontend/spec/javascripts/views/CRUD/annotationlayers/AnnotationLayersList_spec.jsx +++ b/superset-frontend/spec/javascripts/views/CRUD/annotationlayers/AnnotationLayersList_spec.jsx @@ -61,7 +61,7 @@ const mockUser = { }; fetchMock.get(layersInfoEndpoint, { - permissions: ['can_delete'], + permissions: ['can_write'], }); fetchMock.get(layersEndpoint, { result: mocklayers, @@ -156,7 +156,7 @@ describe('AnnotationLayersList', () => { }); it('shows/hides bulk actions when bulk actions is clicked', async () => { - const button = wrapper.find(Button).at(0); + const button = wrapper.find(Button).at(1); act(() => { button.props().onClick(); }); From 19b9306fe859168d9fd56b82bf8e64891b126c38 Mon Sep 17 00:00:00 2001 From: Kasia Kucharczyk Date: Tue, 15 Dec 2020 18:54:30 +0100 Subject: [PATCH 9/9] Updated annotations migration to current --- .../versions/c25cb2c78727_security_converge_annotations.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/superset/migrations/versions/c25cb2c78727_security_converge_annotations.py b/superset/migrations/versions/c25cb2c78727_security_converge_annotations.py index 0a025933a193a..33099dd2e74b2 100644 --- a/superset/migrations/versions/c25cb2c78727_security_converge_annotations.py +++ b/superset/migrations/versions/c25cb2c78727_security_converge_annotations.py @@ -14,10 +14,10 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -"""security_converge_annotations +"""security converge annotations Revision ID: c25cb2c78727 -Revises: 811494c0cc23 +Revises: ccb74baaa89b Create Date: 2020-12-11 17:02:21.213046 """ @@ -36,7 +36,7 @@ ) revision = "c25cb2c78727" -down_revision = "5daced1f0e76" +down_revision = "ccb74baaa89b" NEW_PVMS = {"Annotation": ("can_read", "can_write",)}