From ca7e89f6d4f317ec50dd782c2658827732063afb Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Thu, 14 Jan 2021 21:45:56 +0200 Subject: [PATCH 01/49] feat: add dashboard roles to support dashboard level access --- superset/dashboards/filters.py | 20 +++++++-- ...658_add_roles_relationship_to_dashboard.py | 45 +++++++++++++++++++ superset/models/dashboard.py | 13 +++++- 3 files changed, 73 insertions(+), 5 deletions(-) create mode 100644 superset/migrations/versions/11ccdd12658_add_roles_relationship_to_dashboard.py diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index a2421ab007ba6..fc875388db2cc 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -16,6 +16,7 @@ # under the License. from typing import Any +from flask_appbuilder.security.sqla.models import Role from flask_babel import lazy_gettext as _ from sqlalchemy import and_, or_ from sqlalchemy.orm.query import Query @@ -74,12 +75,13 @@ def apply(self, query: Query, value: Any) -> Query: datasource_perms = security_manager.user_view_menu_names("datasource_access") schema_perms = security_manager.user_view_menu_names("schema_access") - published_dash_query = ( + published_dash_datesource_perm_query = ( db.session.query(Dashboard.id) .join(Dashboard.slices) .filter( and_( - Dashboard.published == True, # pylint: disable=singleton-comparison + Dashboard.published.is_(True), + not Dashboard.roles, or_( Slice.perm.in_(datasource_perms), Slice.schema_perm.in_(schema_perms), @@ -89,6 +91,17 @@ def apply(self, query: Query, value: Any) -> Query: ) ) + published_dash_roles_based_query = ( + db.session.query(Dashboard.id) + .join(Dashboard.roles) + .filter( + and_( + Dashboard.published.is_(True), + Role.user.id == security_manager.user_model.get_user_id(), + ) + ) + ) + users_favorite_dash_query = db.session.query(FavStar.obj_id).filter( and_( FavStar.user_id == security_manager.user_model.get_user_id(), @@ -107,8 +120,9 @@ def apply(self, query: Query, value: Any) -> Query: query = query.filter( or_( Dashboard.id.in_(owner_ids_query), - Dashboard.id.in_(published_dash_query), + Dashboard.id.in_(published_dash_datesource_perm_query), Dashboard.id.in_(users_favorite_dash_query), + Dashboard.id.in_(published_dash_roles_based_query), ) ) diff --git a/superset/migrations/versions/11ccdd12658_add_roles_relationship_to_dashboard.py b/superset/migrations/versions/11ccdd12658_add_roles_relationship_to_dashboard.py new file mode 100644 index 0000000000000..94dbfaa7d1f55 --- /dev/null +++ b/superset/migrations/versions/11ccdd12658_add_roles_relationship_to_dashboard.py @@ -0,0 +1,45 @@ +# 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. +"""add roles relationship to dashboard +Revision ID: e11ccdd12658 +Revises: c878781977c6 +Create Date: 2021-01-14 19:12:43.406230 +""" +# revision identifiers, used by Alembic. +revision = "e11ccdd12658" +down_revision = "c878781977c6" +import sqlalchemy as sa +from alembic import op + + +def upgrade(): + op.create_table( + "dashboard_roles", + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("role_id", sa.Integer(), nullable=False), + sa.Column("dashboard_id", sa.Integer(), nullable=True), + sa.ForeignKeyConstraint(["dashboard_id"], ["dashboards.id"]), + sa.ForeignKeyConstraint(["role_id"], ["ab_role.id"]), + sa.PrimaryKeyConstraint("id"), + ) + + pass + + +def downgrade(): + op.drop_table("dashboard_roles") + pass diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index d7c2a6978a480..161bf2664edb4 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -22,7 +22,7 @@ import sqlalchemy as sqla from flask_appbuilder import Model from flask_appbuilder.models.decorators import renders -from flask_appbuilder.security.sqla.models import User +from flask_appbuilder.security.sqla.models import Role, User from markupsafe import escape, Markup from sqlalchemy import ( Boolean, @@ -115,6 +115,15 @@ def copy_dashboard( ) +DashboardRoles = Table( + "dashboard_roles", + metadata, + Column("id", Integer, primary_key=True), + Column("dashboard_id", Integer, ForeignKey("dashboards.id"), nullable=False), + Column("role_id", Integer, ForeignKey("ab_role.id"), nullable=False), +) + + class Dashboard( # pylint: disable=too-many-instance-attributes Model, AuditMixinNullable, ImportExportMixin ): @@ -132,7 +141,7 @@ class Dashboard( # pylint: disable=too-many-instance-attributes slices = relationship(Slice, secondary=dashboard_slices, backref="dashboards") owners = relationship(security_manager.user_model, secondary=dashboard_user) published = Column(Boolean, default=False) - + roles = relationship(security_manager.role_model, secondary=DashboardRoles) export_fields = [ "dashboard_title", "position_json", From 0f1a6db8a9310897ea14f675a81bfe94c2a4e87f Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Fri, 15 Jan 2021 13:24:05 +0200 Subject: [PATCH 02/49] fix: dashboard filter logic --- superset/dashboards/filters.py | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index fc875388db2cc..697d67252f8cd 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -18,12 +18,12 @@ from flask_appbuilder.security.sqla.models import Role from flask_babel import lazy_gettext as _ -from sqlalchemy import and_, or_ +from sqlalchemy import and_, or_, not_ from sqlalchemy.orm.query import Query from superset import db, security_manager from superset.models.core import FavStar -from superset.models.dashboard import Dashboard +from superset.models.dashboard import Dashboard, DashboardRoles from superset.models.slice import Slice from superset.views.base import BaseFilter, is_user_admin from superset.views.base_api import BaseFavoriteFilter @@ -81,7 +81,7 @@ def apply(self, query: Query, value: Any) -> Query: .filter( and_( Dashboard.published.is_(True), - not Dashboard.roles, + not_(Dashboard.roles), or_( Slice.perm.in_(datasource_perms), Slice.schema_perm.in_(schema_perms), @@ -90,16 +90,10 @@ def apply(self, query: Query, value: Any) -> Query: ) ) ) - published_dash_roles_based_query = ( db.session.query(Dashboard.id) .join(Dashboard.roles) - .filter( - and_( - Dashboard.published.is_(True), - Role.user.id == security_manager.user_model.get_user_id(), - ) - ) + .filter(and_(Dashboard.published.is_(True), Role.id.in_(user_roles)),) ) users_favorite_dash_query = db.session.query(FavStar.obj_id).filter( From 39b6b2ed72f783a49cad136859ef758eb3e86ba1 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Fri, 15 Jan 2021 22:25:09 +0200 Subject: [PATCH 03/49] fix: negate dashboard has any roles --- superset/dashboards/filters.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index 697d67252f8cd..0add33d0d349f 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -81,7 +81,7 @@ def apply(self, query: Query, value: Any) -> Query: .filter( and_( Dashboard.published.is_(True), - not_(Dashboard.roles), + ~Dashboard.roles.any(), or_( Slice.perm.in_(datasource_perms), Slice.schema_perm.in_(schema_perms), From 9884bc3c7757009be1235fb2a254d39248d22989 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Sat, 16 Jan 2021 07:18:03 +0200 Subject: [PATCH 04/49] fix: pre-commit --- superset/dashboards/filters.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index 0add33d0d349f..c7cdfe41e5d80 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -18,7 +18,7 @@ from flask_appbuilder.security.sqla.models import Role from flask_babel import lazy_gettext as _ -from sqlalchemy import and_, or_, not_ +from sqlalchemy import and_, not_, or_ from sqlalchemy.orm.query import Query from superset import db, security_manager From 3eb61b3aa9ea95dc87193d2036a5be246ae1bc98 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Sat, 16 Jan 2021 07:29:22 +0200 Subject: [PATCH 05/49] fix: unused imports --- superset/dashboards/filters.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index c7cdfe41e5d80..dbbfcb30df7be 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -18,12 +18,12 @@ from flask_appbuilder.security.sqla.models import Role from flask_babel import lazy_gettext as _ -from sqlalchemy import and_, not_, or_ +from sqlalchemy import and_, or_ from sqlalchemy.orm.query import Query from superset import db, security_manager from superset.models.core import FavStar -from superset.models.dashboard import Dashboard, DashboardRoles +from superset.models.dashboard import Dashboard from superset.models.slice import Slice from superset.views.base import BaseFilter, is_user_admin from superset.views.base_api import BaseFavoriteFilter From 47235a459d4225a130b5a1ff052f7630aa66a582 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Sat, 16 Jan 2021 07:34:58 +0200 Subject: [PATCH 06/49] fix: unused imports and invalid naming --- superset/dashboards/filters.py | 8 ++++---- superset/models/dashboard.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index dbbfcb30df7be..c5ecfb20e2ff5 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -75,7 +75,7 @@ def apply(self, query: Query, value: Any) -> Query: datasource_perms = security_manager.user_view_menu_names("datasource_access") schema_perms = security_manager.user_view_menu_names("schema_access") - published_dash_datesource_perm_query = ( + datesource_perm_query = ( db.session.query(Dashboard.id) .join(Dashboard.slices) .filter( @@ -90,7 +90,7 @@ def apply(self, query: Query, value: Any) -> Query: ) ) ) - published_dash_roles_based_query = ( + roles_based_query = ( db.session.query(Dashboard.id) .join(Dashboard.roles) .filter(and_(Dashboard.published.is_(True), Role.id.in_(user_roles)),) @@ -114,9 +114,9 @@ def apply(self, query: Query, value: Any) -> Query: query = query.filter( or_( Dashboard.id.in_(owner_ids_query), - Dashboard.id.in_(published_dash_datesource_perm_query), + Dashboard.id.in_(datesource_perm_query), Dashboard.id.in_(users_favorite_dash_query), - Dashboard.id.in_(published_dash_roles_based_query), + Dashboard.id.in_(roles_based_query), ) ) diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index 161bf2664edb4..53febd419f826 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -22,7 +22,7 @@ import sqlalchemy as sqla from flask_appbuilder import Model from flask_appbuilder.models.decorators import renders -from flask_appbuilder.security.sqla.models import Role, User +from flask_appbuilder.security.sqla.models import User from markupsafe import escape, Markup from sqlalchemy import ( Boolean, From 2f13075fb87073c6ca0f216a3fa7c682f9d8e9da Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Sat, 16 Jan 2021 08:47:06 +0200 Subject: [PATCH 07/49] fix: hopefully last lint issue --- superset/models/dashboard.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index 53febd419f826..3606da8c5cb80 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -22,7 +22,7 @@ import sqlalchemy as sqla from flask_appbuilder import Model from flask_appbuilder.models.decorators import renders -from flask_appbuilder.security.sqla.models import User +from flask_appbuilder.security.sqla.models import User from markupsafe import escape, Markup from sqlalchemy import ( Boolean, From 9d26d30cf648f950d2522b1380b04396e8c949f5 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Fri, 22 Jan 2021 10:34:05 +0200 Subject: [PATCH 08/49] fix: adapt to changes --- superset/dashboards/filters.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index c5ecfb20e2ff5..57e5c35c7081b 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -25,7 +25,7 @@ from superset.models.core import FavStar from superset.models.dashboard import Dashboard from superset.models.slice import Slice -from superset.views.base import BaseFilter, is_user_admin +from superset.views.base import BaseFilter, is_user_admin, get_user_roles from superset.views.base_api import BaseFavoriteFilter @@ -93,7 +93,7 @@ def apply(self, query: Query, value: Any) -> Query: roles_based_query = ( db.session.query(Dashboard.id) .join(Dashboard.roles) - .filter(and_(Dashboard.published.is_(True), Role.id.in_(user_roles)),) + .filter(and_(Dashboard.published.is_(True), Role.id.in_(get_user_roles())),) ) users_favorite_dash_query = db.session.query(FavStar.obj_id).filter( From 55199ed35bbff5d5d6a4cc1808fec0855c35e10d Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Fri, 22 Jan 2021 10:48:12 +0200 Subject: [PATCH 09/49] fix: fix pre-commit --- superset/dashboards/filters.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index 57e5c35c7081b..1187cbd08b86f 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -25,7 +25,7 @@ from superset.models.core import FavStar from superset.models.dashboard import Dashboard from superset.models.slice import Slice -from superset.views.base import BaseFilter, is_user_admin, get_user_roles +from superset.views.base import BaseFilter, get_user_roles, is_user_admin from superset.views.base_api import BaseFavoriteFilter From dcc8e66ace7c272571276c858f11b5a9d6be6ac3 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Fri, 22 Jan 2021 13:16:36 +0200 Subject: [PATCH 10/49] fix: extract id from role --- superset/dashboards/filters.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index 1187cbd08b86f..ab3bfca706318 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -93,7 +93,12 @@ def apply(self, query: Query, value: Any) -> Query: roles_based_query = ( db.session.query(Dashboard.id) .join(Dashboard.roles) - .filter(and_(Dashboard.published.is_(True), Role.id.in_(get_user_roles())),) + .filter( + and_( + Dashboard.published.is_(True), + Role.id.in_([x.id for x in get_user_roles()]), + ), + ) ) users_favorite_dash_query = db.session.query(FavStar.obj_id).filter( From c560de83b459311cf899744294d3a45cf3b5df18 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Fri, 22 Jan 2021 13:23:49 +0200 Subject: [PATCH 11/49] fix: extract hasNoRoleBasedAccess --- superset/dashboards/filters.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index ab3bfca706318..782be4972ea6f 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -81,7 +81,7 @@ def apply(self, query: Query, value: Any) -> Query: .filter( and_( Dashboard.published.is_(True), - ~Dashboard.roles.any(), + self.hasNoRoleBasedAccess(), or_( Slice.perm.in_(datasource_perms), Slice.schema_perm.in_(schema_perms), @@ -126,3 +126,7 @@ def apply(self, query: Query, value: Any) -> Query: ) return query + + @staticmethod + def hasNoRoleBasedAccess(): + return ~Dashboard.roles.any() From 0b869a2b89cd5e07980faaf9da48e59483fb6dbd Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Fri, 22 Jan 2021 13:49:15 +0200 Subject: [PATCH 12/49] fix: pylint bad method name casing :) --- superset/dashboards/filters.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index 782be4972ea6f..3998918890070 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -81,7 +81,7 @@ def apply(self, query: Query, value: Any) -> Query: .filter( and_( Dashboard.published.is_(True), - self.hasNoRoleBasedAccess(), + self.has_no_role_based_access(), or_( Slice.perm.in_(datasource_perms), Slice.schema_perm.in_(schema_perms), @@ -128,5 +128,5 @@ def apply(self, query: Query, value: Any) -> Query: return query @staticmethod - def hasNoRoleBasedAccess(): + def has_no_role_based_access(): return ~Dashboard.roles.any() From 27f8fb93fad7e9ea7d5c959d1331b71b49105c31 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Fri, 22 Jan 2021 14:31:55 +0200 Subject: [PATCH 13/49] fix: pre-commit non typed method --- superset/dashboards/filters.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index 3998918890070..4cd567d4ebbce 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -128,5 +128,5 @@ def apply(self, query: Query, value: Any) -> Query: return query @staticmethod - def has_no_role_based_access(): + def has_no_role_based_access() -> bool: return ~Dashboard.roles.any() From 0223844693776dea881064140c4091fc1f0b0f19 Mon Sep 17 00:00:00 2001 From: Amit Miran <47772523+amitmiran137@users.noreply.github.com> Date: Fri, 22 Jan 2021 17:20:07 +0200 Subject: [PATCH 14/49] fix: remove redundant pass --- .../11ccdd12658_add_roles_relationship_to_dashboard.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/superset/migrations/versions/11ccdd12658_add_roles_relationship_to_dashboard.py b/superset/migrations/versions/11ccdd12658_add_roles_relationship_to_dashboard.py index 94dbfaa7d1f55..a4049e39d9ca6 100644 --- a/superset/migrations/versions/11ccdd12658_add_roles_relationship_to_dashboard.py +++ b/superset/migrations/versions/11ccdd12658_add_roles_relationship_to_dashboard.py @@ -37,9 +37,6 @@ def upgrade(): sa.PrimaryKeyConstraint("id"), ) - pass - def downgrade(): op.drop_table("dashboard_roles") - pass From aa2e53eb598bb4e42a255c77ffd878c849f6dcb8 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Sat, 23 Jan 2021 13:47:07 +0200 Subject: [PATCH 15/49] feat: allow managing dashboard roles from the viewbased dashboard editor --- superset/views/dashboard/mixin.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/superset/views/dashboard/mixin.py b/superset/views/dashboard/mixin.py index 89472acf74a00..f2d5ca170369d 100644 --- a/superset/views/dashboard/mixin.py +++ b/superset/views/dashboard/mixin.py @@ -33,6 +33,7 @@ class DashboardMixin: # pylint: disable=too-few-public-methods "dashboard_title", "slug", "owners", + "roles", "position_json", "css", "json_metadata", @@ -62,6 +63,8 @@ class DashboardMixin: # pylint: disable=too-few-public-methods "want to alter specific parameters." ), "owners": _("Owners is a list of users who can alter the dashboard."), + "roles": _("Roles is a list which defines access to the dashboard. if list is " + "empty access is managed by the data access level."), "published": _( "Determines whether or not this dashboard is " "visible in the list of all dashboards" @@ -74,6 +77,7 @@ class DashboardMixin: # pylint: disable=too-few-public-methods "slug": _("Slug"), "charts": _("Charts"), "owners": _("Owners"), + "roles": _("Roles"), "published": _("Published"), "creator": _("Creator"), "modified": _("Modified"), From 58b680b335539281aa25a7726a49a7cb6665e287 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Sat, 23 Jan 2021 14:20:57 +0200 Subject: [PATCH 16/49] fix: pre-commit :) --- superset/views/dashboard/mixin.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/superset/views/dashboard/mixin.py b/superset/views/dashboard/mixin.py index f2d5ca170369d..30e0d7948d4e3 100644 --- a/superset/views/dashboard/mixin.py +++ b/superset/views/dashboard/mixin.py @@ -63,8 +63,10 @@ class DashboardMixin: # pylint: disable=too-few-public-methods "want to alter specific parameters." ), "owners": _("Owners is a list of users who can alter the dashboard."), - "roles": _("Roles is a list which defines access to the dashboard. if list is " - "empty access is managed by the data access level."), + "roles": _( + "Roles is a list which defines access to the dashboard. if list is " + "empty access is managed by the data access level." + ), "published": _( "Determines whether or not this dashboard is " "visible in the list of all dashboards" From 8714b8b580934510bc530581dbb3084a2282f212 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Tue, 26 Jan 2021 21:58:32 +0200 Subject: [PATCH 17/49] test: dashboard lists API and view endpoints --- tests/base_tests.py | 12 + tests/dashboards/consts.py | 26 ++ tests/dashboards/dashboard_test_utils.py | 97 ++++++ tests/dashboards/security/__init__.py | 0 tests/dashboards/security/base_case.py | 90 +++++ .../security/security_rbac_tests.py | 300 +++++++++++++++++ tests/dashboards/superset_factory_util.py | 318 ++++++++++++++++++ 7 files changed, 843 insertions(+) create mode 100644 tests/dashboards/consts.py create mode 100644 tests/dashboards/dashboard_test_utils.py create mode 100644 tests/dashboards/security/__init__.py create mode 100644 tests/dashboards/security/base_case.py create mode 100644 tests/dashboards/security/security_rbac_tests.py create mode 100644 tests/dashboards/superset_factory_util.py diff --git a/tests/base_tests.py b/tests/base_tests.py index e494c891757eb..7196930d8e529 100644 --- a/tests/base_tests.py +++ b/tests/base_tests.py @@ -287,6 +287,18 @@ def get_access_requests(self, username, ds_type, ds_id): def logout(self): self.client.get("/logout/", follow_redirects=True) + def grant_access_to_dashboard(self, dashboard, role_name="Public"): + role = security_manager.find_role(role_name) + dashboard.roles.append(role) + db.session.merge(dashboard) + db.session.commit() + + def revoke_access_to_dashboard(self, dashboard, role_name="Public"): + role = security_manager.find_role(role_name) + dashboard.roles.remove(role) + db.session.merge(dashboard) + db.session.commit() + def grant_public_access_to_table(self, table): public_role = security_manager.find_role("Public") perms = db.session.query(ab_models.PermissionView).all() diff --git a/tests/dashboards/consts.py b/tests/dashboards/consts.py new file mode 100644 index 0000000000000..265077db1b08b --- /dev/null +++ b/tests/dashboards/consts.py @@ -0,0 +1,26 @@ +# 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. + + +DASHBOARDS_API_URL = "api/v1/dashboard/" +GET_DASHBOARDS_LIST_VIEW = "/dashboard/list/" + +ADMIN_USERNAME = "admin" +ALPHA_USERNAME = "alpha" +GAMMA_USERNAME = "gamma" + +DEFAULT_DASHBOARD_SLUG_TO_TEST = "births" diff --git a/tests/dashboards/dashboard_test_utils.py b/tests/dashboards/dashboard_test_utils.py new file mode 100644 index 0000000000000..dace1b903e45e --- /dev/null +++ b/tests/dashboards/dashboard_test_utils.py @@ -0,0 +1,97 @@ +# 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 +import random +import string +from typing import Any, Dict, List, Optional, Tuple + +from sqlalchemy import func + +from superset import appbuilder, db +from superset.models.dashboard import Dashboard +from tests.dashboards.consts import DEFAULT_DASHBOARD_SLUG_TO_TEST + +logger = logging.getLogger(__name__) + +session = appbuilder.get_session + + +def get_mock_positions(dashboard: Dashboard) -> Dict[str, Any]: + positions = {"DASHBOARD_VERSION_KEY": "v2"} + for i, slc in enumerate(dashboard.slices): + id_ = "DASHBOARD_CHART_TYPE-{}".format(i) + position_data = { + "type": "CHART", + "id": id_, + "children": [], + "meta": {"width": 4, "height": 50, "chartId": slc.id}, + } + positions[id_] = position_data + return positions + + +def build_save_dash_parts( + dashboard_slug: Optional[str] = None, dashboard_to_edit: Optional[Dashboard] = None +) -> Tuple[Dashboard, Dict, Dict]: + if not dashboard_to_edit: + dashboard_slug = ( + dashboard_slug if dashboard_slug else DEFAULT_DASHBOARD_SLUG_TO_TEST + ) + dashboard_to_edit = get_dashboard_by_slug(dashboard_slug) + + data_before_change = { + "positions": dashboard_to_edit.position, + "dashboard_title": dashboard_to_edit.dashboard_title, + } + data_after_change = { + "css": "", + "expanded_slices": {}, + "positions": get_mock_positions(dashboard_to_edit), + "dashboard_title": dashboard_to_edit.dashboard_title, + } + return dashboard_to_edit, data_before_change, data_after_change + + +def get_all_dashboards() -> List[Dashboard]: + return db.session.query(Dashboard).all() + + +def get_dashboard_by_slug(dashboard_slug: str) -> Dashboard: + return db.session.query(Dashboard).filter_by(slug=dashboard_slug).first() + + +def count_dashboards() -> int: + return db.session.query(func.count(Dashboard.id)).first()[0] + + +def random_title(): + return f"title{random_str()}" + + +def random_slug(): + return f"slug{random_str()}" + + +def get_random_string(length): + letters = string.ascii_lowercase + result_str = "".join(random.choice(letters) for i in range(length)) + print("Random string of length", length, "is:", result_str) + return result_str + + +def random_str(): + return get_random_string(8) diff --git a/tests/dashboards/security/__init__.py b/tests/dashboards/security/__init__.py new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/tests/dashboards/security/base_case.py b/tests/dashboards/security/base_case.py new file mode 100644 index 0000000000000..854f7c5871436 --- /dev/null +++ b/tests/dashboards/security/base_case.py @@ -0,0 +1,90 @@ +# 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. +from typing import List, Optional + +from flask import Response + +from superset import app +from superset.models.dashboard import Dashboard +from tests.base_tests import SupersetTestCase +from tests.dashboards.consts import DASHBOARDS_API_URL, GET_DASHBOARDS_LIST_VIEW +from tests.dashboards.superset_factory_util import delete_all_inserted_objects + +DASHBOARD_COUNT_IN_DASHBOARDS_LIST_VIEW_FORMAT = "Record Count: {count}" + + +class BaseTestDashboardSecurity(SupersetTestCase): + def tearDown(self) -> None: + self.clean_created_objects() + + def assert_dashboards_list_view_response( + self, + response: Response, + expected_counts: int, + expected_dashboards: Optional[List[Dashboard]] = None, + not_expected_dashboards: Optional[List[Dashboard]] = None, + ) -> None: + self.assert200(response) + response_html = response.data.decode("utf-8") + if expected_counts == 0: + assert "No records found" in response_html + else: + assert ( + DASHBOARD_COUNT_IN_DASHBOARDS_LIST_VIEW_FORMAT.format( + count=str(expected_counts) + ) + in response_html + ) + expected_dashboards = expected_dashboards or [] + for dashboard in expected_dashboards: + assert dashboard.url in response_html + not_expected_dashboards = not_expected_dashboards or [] + for dashboard in not_expected_dashboards: + assert dashboard.url not in response_html + + def assert_dashboards_api_response( + self, + response: Response, + expected_counts: int, + expected_dashboards: Optional[List[Dashboard]] = None, + not_expected_dashboards: Optional[List[Dashboard]] = None, + ) -> None: + self.assert200(response) + response_data = response.json + self.assertEqual(response_data["count"], expected_counts) + response_dashboards_url = set( + map(lambda dash: dash["url"], response_data["result"]) + ) + expected_dashboards = expected_dashboards or [] + for dashboard in expected_dashboards: + self.assertIn(dashboard.url, response_dashboards_url) + not_expected_dashboards = not_expected_dashboards or [] + for dashboard in not_expected_dashboards: + self.assertNotIn(dashboard.url, response_dashboards_url) + + def get_dashboards_list_response(self) -> Response: + return self.client.get(GET_DASHBOARDS_LIST_VIEW) + + def get_dashboards_api_response(self) -> Response: + return self.client.get(DASHBOARDS_API_URL) + + def clean_created_objects(self): + with app.test_request_context(): + self.logout() + self.login("admin") + delete_all_inserted_objects() + self.logout() diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py new file mode 100644 index 0000000000000..4c4bf1d6e8f0d --- /dev/null +++ b/tests/dashboards/security/security_rbac_tests.py @@ -0,0 +1,300 @@ +# 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. +"""Unit tests for Superset""" + +from tests.dashboards.dashboard_test_utils import * +from tests.dashboards.security.base_case import BaseTestDashboardSecurity +from tests.dashboards.superset_factory_util import ( + create_dashboard_to_db, + create_database_to_db, + create_datasource_table_to_db, + create_slice_to_db, +) + +USER_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS = "gamma" +ROLE_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS = "Gamma" + + +class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity): + def test_get_dashboards_list__admin_get_all_dashboards(self): + # arrange + create_dashboard_to_db( + owners=[], slices=[create_slice_to_db()], published=False + ) + dashboard_counts = count_dashboards() + + self.login("admin") + + # act + response = self.get_dashboards_list_response() + + # assert + self.assert_dashboards_list_view_response(response, dashboard_counts) + + def test_get_dashboards_list__owner_get_all_owned_dashboards(self): + # arrange + owner = self.get_user(USER_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS) + database = create_database_to_db() + table = create_datasource_table_to_db(db_id=database.id, owners=[owner]) + first_dash = create_dashboard_to_db( + owners=[owner], slices=[create_slice_to_db(datasource_id=table.id)] + ) + second_dash = create_dashboard_to_db( + owners=[owner], slices=[create_slice_to_db(datasource_id=table.id)] + ) + owned_dashboards = [first_dash, second_dash] + not_owned_dashboards = [ + create_dashboard_to_db( + slices=[create_slice_to_db(datasource_id=table.id)], published=True + ) + ] + + self.login(USER_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS) + + # act + response = self.get_dashboards_list_response() + + # assert + self.assert_dashboards_list_view_response( + response, 2, owned_dashboards, not_owned_dashboards + ) + + def test_get_dashboards_list__user_without_any_permissions_get_empty_list(self): + create_dashboard_to_db(published=True) + self.login(USER_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS) + + # act + response = self.get_dashboards_list_response() + + # assert + self.assert_dashboards_list_view_response(response, 0) + + def test_get_dashboards_list__user_get_only_published_permitted_dashboards(self): + # arrange + published_dashboards = [ + create_dashboard_to_db(published=True), + create_dashboard_to_db(published=True), + ] + not_published_dashboards = [ + create_dashboard_to_db(published=False), + create_dashboard_to_db(published=False), + ] + + for dash in published_dashboards + not_published_dashboards: + self.grant_access_to_dashboard( + dash, ROLE_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS + ) + + self.login(USER_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS) + + # act + response = self.get_dashboards_list_response() + + # assert + self.assert_dashboards_list_view_response( + response, + len(published_dashboards), + published_dashboards, + not_published_dashboards, + ) + + # post + for dash in published_dashboards + not_published_dashboards: + self.revoke_access_to_dashboard( + dash, ROLE_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS + ) + + def test_get_dashboards_list__public_user_without_any_permissions_get_empty_list( + self, + ): + create_dashboard_to_db(published=True) + + # act + response = self.get_dashboards_list_response() + + # assert + self.assert_dashboards_list_view_response(response, 0) + + def test_get_dashboards_list__public_user_get_only_published_permitted_dashboards( + self, + ): + # arrange + published_dashboards = [ + create_dashboard_to_db(published=True), + create_dashboard_to_db(published=True), + ] + not_published_dashboards = [ + create_dashboard_to_db(published=False), + create_dashboard_to_db(published=False), + ] + + for dash in published_dashboards + not_published_dashboards: + self.grant_access_to_dashboard(dash, "Public") + + # act + response = self.get_dashboards_list_response() + + # assert + self.assert_dashboards_list_view_response( + response, + len(published_dashboards), + published_dashboards, + not_published_dashboards, + ) + + # post + for dash in published_dashboards + not_published_dashboards: + self.revoke_access_to_dashboard(dash, "Public") + + def test_get_dashboards_api__admin_get_all_dashboards(self): + # arrange + create_dashboard_to_db( + owners=[], slices=[create_slice_to_db()], published=False + ) + dashboard_counts = count_dashboards() + + self.login("admin") + + # act + response = self.get_dashboards_api_response() + + # assert + self.assert_dashboards_api_response(response, dashboard_counts) + + def test_get_dashboards_api__owner_get_all_owned_dashboards(self): + # arrange + owner = self.get_user(USER_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS) + database = create_database_to_db() + table = create_datasource_table_to_db(db_id=database.id, owners=[owner]) + first_dash = create_dashboard_to_db( + owners=[owner], slices=[create_slice_to_db(datasource_id=table.id)] + ) + second_dash = create_dashboard_to_db( + owners=[owner], slices=[create_slice_to_db(datasource_id=table.id)] + ) + owned_dashboards = [first_dash, second_dash] + not_owned_dashboards = [ + create_dashboard_to_db( + slices=[create_slice_to_db(datasource_id=table.id)], published=True + ) + ] + + self.login(USER_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS) + + # act + response = self.get_dashboards_api_response() + + # assert + self.assert_dashboards_api_response( + response, 2, owned_dashboards, not_owned_dashboards + ) + + def test_get_dashboards_api__user_without_any_permissions_get_empty_list(self): + create_dashboard_to_db(published=True) + self.login(USER_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS) + + # act + response = self.get_dashboards_api_response() + + # assert + self.assert_dashboards_api_response(response, 0) + + def test_get_dashboards_api__user_get_only_published_permitted_dashboards(self): + # arrange + published_dashboards = [ + create_dashboard_to_db(published=True), + create_dashboard_to_db(published=True), + ] + not_published_dashboards = [ + create_dashboard_to_db(published=False), + create_dashboard_to_db(published=False), + ] + + for dash in published_dashboards + not_published_dashboards: + self.grant_access_to_dashboard( + dash, ROLE_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS + ) + + self.login(USER_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS) + + # act + response = self.get_dashboards_api_response() + + # assert + self.assert_dashboards_api_response( + response, + len(published_dashboards), + published_dashboards, + not_published_dashboards, + ) + + # post + for dash in published_dashboards + not_published_dashboards: + self.revoke_access_to_dashboard( + dash, ROLE_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS + ) + + def test_get_dashboards_api__public_user_without_any_permissions_get_empty_list( + self, + ): + create_dashboard_to_db(published=True) + + # act + response = self.get_dashboards_api_response() + + # assert + self.assert_dashboards_api_response(response, 0) + + def test_get_dashboards_api__public_user_get_only_published_permitted_dashboards( + self, + ): + # arrange + published_dashboards = [ + create_dashboard_to_db(published=True), + create_dashboard_to_db(published=True), + ] + not_published_dashboards = [ + create_dashboard_to_db(published=False), + create_dashboard_to_db(published=False), + ] + + for dash in published_dashboards + not_published_dashboards: + self.grant_access_to_dashboard(dash, "Public") + + # act + response = self.get_dashboards_api_response() + + # assert + self.assert_dashboards_api_response( + response, + len(published_dashboards), + published_dashboards, + not_published_dashboards, + ) + + # post + for dash in published_dashboards + not_published_dashboards: + self.revoke_access_to_dashboard(dash, "Public") + + @staticmethod + def __add_dashboard_mode_params(dashboard_url): + full_url = dashboard_url + if dashboard_url.find("?") == -1: + full_url += "?" + else: + full_url += "&" + return full_url + "edit=true&standalone=true" diff --git a/tests/dashboards/superset_factory_util.py b/tests/dashboards/superset_factory_util.py new file mode 100644 index 0000000000000..571463f447dd5 --- /dev/null +++ b/tests/dashboards/superset_factory_util.py @@ -0,0 +1,318 @@ +# 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, Callable, List, Optional + +from flask_appbuilder import Model +from flask_appbuilder.security.sqla.models import User + +from superset import appbuilder +from superset.connectors.sqla.models import SqlaTable, sqlatable_user +from superset.models.core import Database +from superset.models.dashboard import Dashboard, dashboard_slices, dashboard_user, \ + DashboardRoles +from superset.models.slice import Slice, slice_user +from tests.dashboards.dashboard_test_utils import random_slug, random_str, random_title + +logger = logging.getLogger(__name__) + +session = appbuilder.get_session + +inserted_dashboards_ids = [] +inserted_databases_ids = [] +inserted_sqltables_ids = [] +inserted_slices_ids = [] + + +def create_dashboard_to_db( + dashboard_title: Optional[str] = None, + slug: Optional[str] = None, + published: bool = False, + owners: Optional[List[User]] = None, + slices: Optional[List[Slice]] = None, + css: str = "", + json_metadata: str = "", + position_json: str = "", +) -> Dashboard: + dashboard = create_dashboard( + dashboard_title, + slug, + published, + owners, + slices, + css, + json_metadata, + position_json, + ) + + insert_model(dashboard) + inserted_dashboards_ids.append(dashboard.id) + return dashboard + + +def create_dashboard( + dashboard_title: Optional[str] = None, + slug: Optional[str] = None, + published: bool = False, + owners: Optional[List[User]] = None, + slices: Optional[List[Slice]] = None, + css: str = "", + json_metadata: str = "", + position_json: str = "", +) -> Dashboard: + dashboard_title = dashboard_title or random_title() + slug = slug or random_slug() + owners = owners or [] + slices = slices or [] + return Dashboard( + dashboard_title=dashboard_title, + slug=slug, + published=published, + owners=owners, + css=css, + position_json=position_json, + json_metadata=json_metadata, + slices=slices, + ) + + +def insert_model(dashboard: Model) -> None: + session.add(dashboard) + session.commit() + session.refresh(dashboard) + + +def create_slice_to_db( + name: Optional[str] = None, + datasource_id: Optional[int] = None, + owners: Optional[List[User]] = None, +) -> Slice: + slice_ = create_slice(datasource_id, name, owners) + insert_model(slice_) + inserted_slices_ids.append(slice_.id) + return slice_ + + +def create_slice(datasource_id, name, owners): + name = name or random_str() + owners = owners or [] + datasource_id = ( + datasource_id or create_datasource_table_to_db(name=name + "_table").id + ) + return Slice( + slice_name=name, + datasource_id=datasource_id, + owners=owners, + datasource_type="table", + ) + + +def create_datasource_table_to_db( + name: Optional[str] = None, + db_id: Optional[int] = None, + owners: Optional[List[User]] = None, +) -> SqlaTable: + sqltable = create_datasource_table(name, db_id, owners) + insert_model(sqltable) + inserted_sqltables_ids.append(sqltable.id) + return sqltable + + +def create_datasource_table( + name: Optional[str] = None, + db_id: Optional[int] = None, + owners: Optional[List[User]] = None, +) -> SqlaTable: + name = name or random_str() + owners = owners or [] + db_id = db_id or create_database_to_db(name=name + "_db").id + return SqlaTable(table_name=name, database_id=db_id, owners=owners) + + +def create_database_to_db(name: Optional[str] = None) -> Database: + database = create_database(name) + insert_model(database) + inserted_databases_ids.append(database.id) + return database + + +def create_database(name: Optional[str] = None) -> Database: + name = name or random_str() + return Database(database_name=name, sqlalchemy_uri="sqlite:///:memory:") + + +def delete_all_inserted_objects() -> None: + delete_all_inserted_dashboards() + delete_all_inserted_slices() + delete_all_inserted_tables() + delete_all_inserted_dbs() + + +def delete_all_inserted_dashboards(): + try: + dashboards_to_delete: List[Dashboard] = session.query(Dashboard).filter( + Dashboard.id.in_(inserted_dashboards_ids) + ).all() + for dashboard in dashboards_to_delete: + try: + delete_dashboard(dashboard, False) + except Exception as ex: + logger.error(f"failed to delete {dashboard.id}", exc_info=True) + raise ex + if len(inserted_dashboards_ids) > 0: + session.commit() + inserted_dashboards_ids.clear() + except Exception as ex2: + logger.error("delete_all_inserted_dashboards failed", exc_info=True) + raise ex2 + + +def delete_dashboard(dashboard: Dashboard, do_commit: bool = False) -> None: + logger.info(f"deleting dashboard{dashboard.id}") + delete_dashboard_roles_associations(dashboard) + delete_dashboard_users_associations(dashboard) + delete_dashboard_slices_associations(dashboard) + session.delete(dashboard) + if do_commit: + session.commit() + + +def delete_dashboard_users_associations(dashboard: Dashboard) -> None: + session.execute( + dashboard_user.delete().where(dashboard_user.c.dashboard_id == dashboard.id) + ) + + +def delete_dashboard_roles_associations(dashboard: Dashboard) -> None: + session.execute( + DashboardRoles.delete().where(DashboardRoles.c.dashboard_id == dashboard.id) + ) + + +def delete_dashboard_slices_associations(dashboard: Dashboard) -> None: + session.execute( + dashboard_slices.delete().where(dashboard_slices.c.dashboard_id == dashboard.id) + ) + + +def delete_all_inserted_slices(): + try: + slices_to_delete: List[Slice] = session.query(Slice).filter( + Slice.id.in_(inserted_slices_ids) + ).all() + for slice in slices_to_delete: + try: + delete_slice(slice, False) + except Exception as ex: + logger.error(f"failed to delete {slice.id}", exc_info=True) + raise ex + if len(inserted_slices_ids) > 0: + session.commit() + inserted_slices_ids.clear() + except Exception as ex2: + logger.error("delete_all_inserted_slices failed", exc_info=True) + raise ex2 + + +def delete_slice(slice_: Slice, do_commit: bool = False) -> None: + logger.info(f"deleting slice{slice_.id}") + delete_slice_users_associations(slice_) + session.delete(slice_) + if do_commit: + session.commit() + + +def delete_slice_users_associations(slice_: Slice) -> None: + session.execute(slice_user.delete().where(slice_user.c.slice_id == slice_.id)) + + +def delete_all_inserted_tables(): + try: + tables_to_delete: List[SqlaTable] = session.query(SqlaTable).filter( + SqlaTable.id.in_(inserted_sqltables_ids) + ).all() + for table in tables_to_delete: + try: + delete_sqltable(table, False) + except Exception as ex: + logger.error(f"failed to delete {table.id}", exc_info=True) + raise ex + if len(inserted_sqltables_ids) > 0: + session.commit() + inserted_sqltables_ids.clear() + except Exception as ex2: + logger.error("delete_all_inserted_tables failed", exc_info=True) + raise ex2 + + +def delete_sqltable(table: SqlaTable, do_commit: bool = False) -> None: + logger.info(f"deleting table{table.id}") + delete_table_users_associations(table) + session.delete(table) + if do_commit: + session.commit() + + +def delete_table_users_associations(table: SqlaTable) -> None: + session.execute( + sqlatable_user.delete().where(sqlatable_user.c.table_id == table.id) + ) + + +def delete_all_inserted_dbs(): + try: + dbs_to_delete: List[Database] = session.query(Database).filter( + Database.id.in_(inserted_databases_ids) + ).all() + for db in dbs_to_delete: + try: + delete_database(db, False) + except Exception as ex: + logger.error(f"failed to delete {db.id}", exc_info=True) + raise ex + if len(inserted_databases_ids) > 0: + session.commit() + inserted_databases_ids.clear() + except Exception as ex2: + logger.error("delete_all_inserted_databases failed", exc_info=True) + raise ex2 + + +def delete_database(database: Database, do_commit: bool = False) -> None: + logger.info(f"deleting database{database.id}") + session.delete(database) + if do_commit: + session.commit() + + +def delete_all_inserted_models(model: Any, ids: List[int], delete_model: Callable): + try: + models_to_delete: List[Model] = session.query(model).filter_by( + model.id.in_(ids).all() + ) + for model in models_to_delete: + try: + delete_model(model, False) + except Exception as ex: + logger.error(f"failed to delete {model.id}", exc_info=True) + raise ex + if len(ids) > 0: + session.commit() + ids.clear() + except Exception as ex2: + logger.error(f"delete_all_inserted_models of {model} failed", exc_info=True) + raise ex2 From 60e92a3dcb28adc3acad34c90843621dc6de6f98 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Tue, 26 Jan 2021 22:09:02 +0200 Subject: [PATCH 18/49] chore: update down revision --- .../11ccdd12658_add_roles_relationship_to_dashboard.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/migrations/versions/11ccdd12658_add_roles_relationship_to_dashboard.py b/superset/migrations/versions/11ccdd12658_add_roles_relationship_to_dashboard.py index a4049e39d9ca6..b5576caa5f18b 100644 --- a/superset/migrations/versions/11ccdd12658_add_roles_relationship_to_dashboard.py +++ b/superset/migrations/versions/11ccdd12658_add_roles_relationship_to_dashboard.py @@ -16,12 +16,12 @@ # under the License. """add roles relationship to dashboard Revision ID: e11ccdd12658 -Revises: c878781977c6 +Revises: 260bf0649a77 Create Date: 2021-01-14 19:12:43.406230 """ # revision identifiers, used by Alembic. revision = "e11ccdd12658" -down_revision = "c878781977c6" +down_revision = "260bf0649a77" import sqlalchemy as sa from alembic import op From ff92508699bcd1a9199db1b86a45da09377e2a1b Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Tue, 26 Jan 2021 22:24:07 +0200 Subject: [PATCH 19/49] fix: license check and pre-commit --- tests/dashboards/security/__init__.py | 16 ++++++++++++++++ tests/dashboards/superset_factory_util.py | 8 ++++++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/tests/dashboards/security/__init__.py b/tests/dashboards/security/__init__.py index e69de29bb2d1d..13a83393a9124 100644 --- a/tests/dashboards/security/__init__.py +++ b/tests/dashboards/security/__init__.py @@ -0,0 +1,16 @@ +# 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. diff --git a/tests/dashboards/superset_factory_util.py b/tests/dashboards/superset_factory_util.py index 571463f447dd5..0738928e484c7 100644 --- a/tests/dashboards/superset_factory_util.py +++ b/tests/dashboards/superset_factory_util.py @@ -23,8 +23,12 @@ from superset import appbuilder from superset.connectors.sqla.models import SqlaTable, sqlatable_user from superset.models.core import Database -from superset.models.dashboard import Dashboard, dashboard_slices, dashboard_user, \ - DashboardRoles +from superset.models.dashboard import ( + Dashboard, + dashboard_slices, + dashboard_user, + DashboardRoles, +) from superset.models.slice import Slice, slice_user from tests.dashboards.dashboard_test_utils import random_slug, random_str, random_title From c8fadb9ed224b3cbdbf6b3c6e96af6274d558a62 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Tue, 26 Jan 2021 22:34:46 +0200 Subject: [PATCH 20/49] fix: mypy pre-commit issues --- tests/dashboards/dashboard_test_utils.py | 4 ++-- tests/dashboards/superset_factory_util.py | 19 ------------------- 2 files changed, 2 insertions(+), 21 deletions(-) diff --git a/tests/dashboards/dashboard_test_utils.py b/tests/dashboards/dashboard_test_utils.py index dace1b903e45e..8d12224bba8e2 100644 --- a/tests/dashboards/dashboard_test_utils.py +++ b/tests/dashboards/dashboard_test_utils.py @@ -34,7 +34,7 @@ def get_mock_positions(dashboard: Dashboard) -> Dict[str, Any]: positions = {"DASHBOARD_VERSION_KEY": "v2"} for i, slc in enumerate(dashboard.slices): id_ = "DASHBOARD_CHART_TYPE-{}".format(i) - position_data = { + position_data: Any = { "type": "CHART", "id": id_, "children": [], @@ -46,7 +46,7 @@ def get_mock_positions(dashboard: Dashboard) -> Dict[str, Any]: def build_save_dash_parts( dashboard_slug: Optional[str] = None, dashboard_to_edit: Optional[Dashboard] = None -) -> Tuple[Dashboard, Dict, Dict]: +) -> Tuple[Dashboard, Dict[str, Any], Dict[str, Any]]: if not dashboard_to_edit: dashboard_slug = ( dashboard_slug if dashboard_slug else DEFAULT_DASHBOARD_SLUG_TO_TEST diff --git a/tests/dashboards/superset_factory_util.py b/tests/dashboards/superset_factory_util.py index 0738928e484c7..f4459156dffab 100644 --- a/tests/dashboards/superset_factory_util.py +++ b/tests/dashboards/superset_factory_util.py @@ -301,22 +301,3 @@ def delete_database(database: Database, do_commit: bool = False) -> None: session.delete(database) if do_commit: session.commit() - - -def delete_all_inserted_models(model: Any, ids: List[int], delete_model: Callable): - try: - models_to_delete: List[Model] = session.query(model).filter_by( - model.id.in_(ids).all() - ) - for model in models_to_delete: - try: - delete_model(model, False) - except Exception as ex: - logger.error(f"failed to delete {model.id}", exc_info=True) - raise ex - if len(ids) > 0: - session.commit() - ids.clear() - except Exception as ex2: - logger.error(f"delete_all_inserted_models of {model} failed", exc_info=True) - raise ex2 From 061ce2faef4247168602a17c68cc98d465a8336a Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 27 Jan 2021 00:47:40 +0200 Subject: [PATCH 21/49] test: add security_dataset_tests.py and removed non fixture test that created coupling between tests --- tests/base_tests.py | 14 +- tests/dashboard_tests.py | 18 -- tests/dashboards/base_case.py | 114 ++++++++ tests/dashboards/consts.py | 19 ++ tests/dashboards/dashboard_test_utils.py | 10 + tests/dashboards/security/base_case.py | 36 ++- .../security/security_dataset_tests.py | 256 ++++++++++++++++++ 7 files changed, 427 insertions(+), 40 deletions(-) create mode 100644 tests/dashboards/base_case.py create mode 100644 tests/dashboards/security/security_dataset_tests.py diff --git a/tests/base_tests.py b/tests/base_tests.py index 2daf9d2608f7b..40794ea1a4f07 100644 --- a/tests/base_tests.py +++ b/tests/base_tests.py @@ -302,7 +302,11 @@ def revoke_access_to_dashboard(self, dashboard, role_name="Public"): db.session.commit() def grant_public_access_to_table(self, table): - public_role = security_manager.find_role("Public") + role_name = "Public" + self.grant_role_access_to_table(table, role_name) + + def grant_role_access_to_table(self, table, role_name): + role = security_manager.find_role(role_name) perms = db.session.query(ab_models.PermissionView).all() for perm in perms: if ( @@ -310,10 +314,14 @@ def grant_public_access_to_table(self, table): and perm.view_menu and table.perm in perm.view_menu.name ): - security_manager.add_permission_role(public_role, perm) + security_manager.add_permission_role(role, perm) def revoke_public_access_to_table(self, table): - public_role = security_manager.find_role("Public") + role_name = "Public" + self.revoke_role_access_to_table(role_name, table) + + def revoke_role_access_to_table(self, role_name, table): + public_role = security_manager.find_role(role_name) perms = db.session.query(ab_models.PermissionView).all() for perm in perms: if ( diff --git a/tests/dashboard_tests.py b/tests/dashboard_tests.py index be3e0a9be55e0..9780b1608c917 100644 --- a/tests/dashboard_tests.py +++ b/tests/dashboard_tests.py @@ -453,24 +453,6 @@ def test_only_owners_can_save(self): db.session.commit() self.test_save_dash("alpha") - def test_owners_can_view_empty_dashboard(self): - dash = db.session.query(Dashboard).filter_by(slug="empty_dashboard").first() - if not dash: - dash = Dashboard() - dash.dashboard_title = "Empty Dashboard" - dash.slug = "empty_dashboard" - else: - dash.slices = [] - dash.owners = [] - db.session.merge(dash) - db.session.commit() - - gamma_user = security_manager.find_user("gamma") - self.login(gamma_user.username) - - resp = self.get_resp("/api/v1/dashboard/") - self.assertNotIn("/superset/dashboard/empty_dashboard/", resp) - @pytest.mark.usefixtures("load_energy_table_with_slice", "load_dashboard") def test_users_can_view_published_dashboard(self): resp = self.get_resp("/api/v1/dashboard/") diff --git a/tests/dashboards/base_case.py b/tests/dashboards/base_case.py new file mode 100644 index 0000000000000..fe999a6ea75b4 --- /dev/null +++ b/tests/dashboards/base_case.py @@ -0,0 +1,114 @@ +# 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 json +from typing import Dict, Union + +import prison +from flask import Response + +from superset import app, security_manager +from tests.base_tests import SupersetTestCase +from tests.dashboards.consts import * +from tests.dashboards.dashboard_test_utils import build_save_dash_parts +from tests.dashboards.superset_factory_util import delete_all_inserted_objects + + +class DashboardTestCase(SupersetTestCase): + def get_dashboard_via_api_by_id(self, dashboard_id: int) -> Response: + uri = DASHBOARD_API_URL_FORMAT.format(dashboard_id) + return self.get_assert_metric(uri, "get") + + def get_dashboard_view_response(self, dashboard_to_access) -> Response: + return self.client.get(dashboard_to_access.url) + + def get_dashboard_api_response(self, dashboard_to_access) -> Response: + return self.client.get(DASHBOARD_API_URL_FORMAT.format(dashboard_to_access.id)) + + def get_dashboards_list_response(self) -> Response: + return self.client.get(GET_DASHBOARDS_LIST_VIEW) + + def get_dashboards_api_response(self) -> Response: + return self.client.get(DASHBOARDS_API_URL) + + def save_dashboard_via_view( + self, dashboard_id: Union[str, int], dashboard_data: Dict + ) -> Response: + save_dash_url = SAVE_DASHBOARD_URL_FORMAT.format(dashboard_id) + return self.get_resp(save_dash_url, data=dict(data=json.dumps(dashboard_data))) + + def save_dashboard( + self, dashboard_id: Union[str, int], dashboard_data: Dict + ) -> Response: + return self.save_dashboard_via_view(dashboard_id, dashboard_data) + + def delete_dashboard_via_view(self, dashboard_id: int) -> Response: + delete_dashboard_url = DELETE_DASHBOARD_VIEW_URL_FORMAT.format(dashboard_id) + return self.get_resp(delete_dashboard_url, {}) + + def delete_dashboard_via_api(self, dashboard_id): + uri = DASHBOARD_API_URL_FORMAT.format(dashboard_id) + return self.delete_assert_metric(uri, "delete") + + def bulk_delete_dashboard_via_api(self, dashboard_ids): + uri = DASHBOARDS_API_URL_WITH_QUERY_FORMAT.format(prison.dumps(dashboard_ids)) + return self.delete_assert_metric(uri, "bulk_delete") + + def delete_dashboard(self, dashboard_id: int) -> Response: + return self.delete_dashboard_via_view(dashboard_id) + + def assert_permission_was_created(self, dashboard): + view_menu = security_manager.find_view_menu(dashboard.view_name) + self.assertIsNotNone(view_menu) + self.assertEqual(len(security_manager.find_permissions_view_menu(view_menu)), 1) + + def assert_permission_kept_and_changed(self, updated_dashboard, excepted_view_id): + view_menu_after_title_changed = security_manager.find_view_menu( + updated_dashboard.view_name + ) + self.assertIsNotNone(view_menu_after_title_changed) + self.assertEqual(view_menu_after_title_changed.id, excepted_view_id) + + def assert_permissions_were_deleted(self, deleted_dashboard): + view_menu = security_manager.find_view_menu(deleted_dashboard.view_name) + self.assertIsNone(view_menu) + + def save_dash_basic_case(self, username=ADMIN_USERNAME): + # arrange + self.login(username=username) + ( + dashboard_to_save, + data_before_change, + data_after_change, + ) = build_save_dash_parts() + + # act + save_dash_response = self.save_dashboard_via_view( + dashboard_to_save.id, data_after_change + ) + + # assert + self.assertIn("SUCCESS", save_dash_response) + + # post test + self.save_dashboard(dashboard_to_save.id, data_before_change) + + def clean_created_objects(self): + with app.test_request_context(): + self.logout() + self.login("admin") + delete_all_inserted_objects() + self.logout() diff --git a/tests/dashboards/consts.py b/tests/dashboards/consts.py index 265077db1b08b..f76e4a552dbe5 100644 --- a/tests/dashboards/consts.py +++ b/tests/dashboards/consts.py @@ -15,12 +15,31 @@ # specific language governing permissions and limitations # under the License. +QUERY_FORMAT = "?q={}" DASHBOARDS_API_URL = "api/v1/dashboard/" +DASHBOARDS_API_URL_WITH_QUERY_FORMAT = DASHBOARDS_API_URL + QUERY_FORMAT +DASHBOARD_API_URL_FORMAT = DASHBOARDS_API_URL + "{}" +EXPORT_DASHBOARDS_API_URL = DASHBOARDS_API_URL + "export/" +EXPORT_DASHBOARDS_API_URL_WITH_QUERY_FORMAT = EXPORT_DASHBOARDS_API_URL + QUERY_FORMAT + +GET_DASHBOARD_VIEW_URL_FORMAT = "/superset/dashboard/{}/" +SAVE_DASHBOARD_URL_FORMAT = "/superset/save_dash/{}/" +COPY_DASHBOARD_URL_FORMAT = "/superset/copy_dash/{}/" +ADD_SLICES_URL_FORMAT = "/superset/add_slices/{}/" + +DELETE_DASHBOARD_VIEW_URL_FORMAT = "/dashboard/delete/{}" GET_DASHBOARDS_LIST_VIEW = "/dashboard/list/" +NEW_DASHBOARD_URL = "/dashboard/new/" +GET_CHARTS_API_URL = "/api/v1/chart/" + +GAMMA_ROLE_NAME = "Gamma" ADMIN_USERNAME = "admin" ALPHA_USERNAME = "alpha" GAMMA_USERNAME = "gamma" +DEFAULT_ACCESSIBLE_TABLE = "birth_names" +DASHBOARD_SLUG_OF_ACCESSIBLE_TABLE = "births" DEFAULT_DASHBOARD_SLUG_TO_TEST = "births" +WORLD_HEALTH_SLUG = "world_health" diff --git a/tests/dashboards/dashboard_test_utils.py b/tests/dashboards/dashboard_test_utils.py index 8d12224bba8e2..9fe017cc8675c 100644 --- a/tests/dashboards/dashboard_test_utils.py +++ b/tests/dashboards/dashboard_test_utils.py @@ -22,7 +22,9 @@ from sqlalchemy import func from superset import appbuilder, db +from superset.connectors.sqla.models import SqlaTable from superset.models.dashboard import Dashboard +from superset.models.slice import Slice from tests.dashboards.consts import DEFAULT_DASHBOARD_SLUG_TO_TEST logger = logging.getLogger(__name__) @@ -74,6 +76,14 @@ def get_dashboard_by_slug(dashboard_slug: str) -> Dashboard: return db.session.query(Dashboard).filter_by(slug=dashboard_slug).first() +def get_slice_by_name(slice_name: str) -> Slice: + return db.session.query(Slice).filter_by(slice_name=slice_name).first() + + +def get_sql_table_by_name(table_name: str): + return db.session.query(SqlaTable).filter_by(table_name=table_name).one() + + def count_dashboards() -> int: return db.session.query(func.count(Dashboard.id)).first()[0] diff --git a/tests/dashboards/security/base_case.py b/tests/dashboards/security/base_case.py index 854f7c5871436..89d9a8517cb49 100644 --- a/tests/dashboards/security/base_case.py +++ b/tests/dashboards/security/base_case.py @@ -16,21 +16,32 @@ # under the License. from typing import List, Optional -from flask import Response +from flask import escape, Response -from superset import app from superset.models.dashboard import Dashboard -from tests.base_tests import SupersetTestCase -from tests.dashboards.consts import DASHBOARDS_API_URL, GET_DASHBOARDS_LIST_VIEW -from tests.dashboards.superset_factory_util import delete_all_inserted_objects +from tests.dashboards.base_case import DashboardTestCase DASHBOARD_COUNT_IN_DASHBOARDS_LIST_VIEW_FORMAT = "Record Count: {count}" -class BaseTestDashboardSecurity(SupersetTestCase): +class BaseTestDashboardSecurity(DashboardTestCase): def tearDown(self) -> None: self.clean_created_objects() + def assert_dashboard_view_response( + self, response: Response, dashboard_to_access: Dashboard + ) -> None: + self.assert200(response) + assert escape(dashboard_to_access.dashboard_title) in response.data.decode( + "utf-8" + ) + + def assert_dashboard_api_response( + self, response: Response, dashboard_to_access: Dashboard + ) -> None: + self.assert200(response) + self.assertEqual(response.json["id"], dashboard_to_access.id) + def assert_dashboards_list_view_response( self, response: Response, @@ -75,16 +86,3 @@ def assert_dashboards_api_response( not_expected_dashboards = not_expected_dashboards or [] for dashboard in not_expected_dashboards: self.assertNotIn(dashboard.url, response_dashboards_url) - - def get_dashboards_list_response(self) -> Response: - return self.client.get(GET_DASHBOARDS_LIST_VIEW) - - def get_dashboards_api_response(self) -> Response: - return self.client.get(DASHBOARDS_API_URL) - - def clean_created_objects(self): - with app.test_request_context(): - self.logout() - self.login("admin") - delete_all_inserted_objects() - self.logout() diff --git a/tests/dashboards/security/security_dataset_tests.py b/tests/dashboards/security/security_dataset_tests.py new file mode 100644 index 0000000000000..149283e1ebd56 --- /dev/null +++ b/tests/dashboards/security/security_dataset_tests.py @@ -0,0 +1,256 @@ +# 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. +"""Unit tests for Superset""" +import json + +import prison +import pytest +from flask import escape + +from superset import app, security_manager +from superset.models import core as models +from tests.dashboards.base_case import DashboardTestCase +from tests.dashboards.consts import * +from tests.dashboards.dashboard_test_utils import * +from tests.dashboards.superset_factory_util import * +from tests.fixtures.energy_dashboard import load_energy_table_with_slice + + +class TestDashboardDatasetSecurity(DashboardTestCase): + @pytest.fixture + def load_dashboard(self): + with app.app_context(): + table = ( + db.session.query(SqlaTable).filter_by(table_name="energy_usage").one() + ) + # get a slice from the allowed table + slice = db.session.query(Slice).filter_by(slice_name="Energy Sankey").one() + + self.grant_public_access_to_table(table) + + pytest.hidden_dash_slug = f"hidden_dash_{random_slug()}" + pytest.published_dash_slug = f"published_dash_{random_slug()}" + + # Create a published and hidden dashboard and add them to the database + published_dash = Dashboard() + published_dash.dashboard_title = "Published Dashboard" + published_dash.slug = pytest.published_dash_slug + published_dash.slices = [slice] + published_dash.published = True + + hidden_dash = Dashboard() + hidden_dash.dashboard_title = "Hidden Dashboard" + hidden_dash.slug = pytest.hidden_dash_slug + hidden_dash.slices = [slice] + hidden_dash.published = False + + db.session.merge(published_dash) + db.session.merge(hidden_dash) + yield db.session.commit() + + self.revoke_public_access_to_table(table) + db.session.delete(published_dash) + db.session.delete(hidden_dash) + db.session.commit() + + def test_dashboard_access__admin_can_access_all(self): + # arrange + self.login(username=ADMIN_USERNAME) + dashboard_title_by_url = { + dash.url: dash.dashboard_title for dash in get_all_dashboards() + } + + # act + responses_by_url = { + url: self.client.get(url).data.decode("utf-8") + for url in dashboard_title_by_url.keys() + } + + # assert + for dashboard_url, get_dashboard_response in responses_by_url.items(): + assert ( + escape(dashboard_title_by_url[dashboard_url]) in get_dashboard_response + ) + + def test_get_dashboards__users_are_dashboards_owners(self): + # arrange + username = "gamma" + user = security_manager.find_user(username) + my_owned_dashboard = create_dashboard_to_db( + dashboard_title="My Dashboard", + slug=f"my_dash_{random_slug()}", + published=False, + owners=[user], + ) + + not_my_owned_dashboard = create_dashboard_to_db( + dashboard_title="Not My Dashboard", + slug=f"not_my_dash_{random_slug()}", + published=False, + ) + + self.login(user.username) + + # act + get_dashboards_response = self.get_resp(DASHBOARDS_API_URL) + + # assert + self.assertIn(my_owned_dashboard.url, get_dashboards_response) + self.assertNotIn(not_my_owned_dashboard.url, get_dashboards_response) + + def test_get_dashboards__owners_can_view_empty_dashboard(self): + # arrange + dash = create_dashboard_to_db("Empty Dashboard", slug="empty_dashboard") + dashboard_url = dash.url + gamma_user = security_manager.find_user("gamma") + self.login(gamma_user.username) + + # act + get_dashboards_response = self.get_resp(DASHBOARDS_API_URL) + + # assert + self.assertNotIn(dashboard_url, get_dashboards_response) + + def test_get_dashboards__users_can_view_favorites_dashboards(self): + # arrange + user = security_manager.find_user("gamma") + fav_dash_slug = f"my_favorite_dash_{random_slug()}" + regular_dash_slug = f"regular_dash_{random_slug()}" + + favorite_dash = Dashboard() + favorite_dash.dashboard_title = "My Favorite Dashboard" + favorite_dash.slug = fav_dash_slug + + regular_dash = Dashboard() + regular_dash.dashboard_title = "A Plain Ol Dashboard" + regular_dash.slug = regular_dash_slug + + db.session.merge(favorite_dash) + db.session.merge(regular_dash) + db.session.commit() + + dash = db.session.query(Dashboard).filter_by(slug=fav_dash_slug).first() + + favorites = models.FavStar() + favorites.obj_id = dash.id + favorites.class_name = "Dashboard" + favorites.user_id = user.id + + db.session.merge(favorites) + db.session.commit() + + self.login(user.username) + + # act + get_dashboards_response = self.get_resp(DASHBOARDS_API_URL) + + # assert + self.assertIn(f"/superset/dashboard/{fav_dash_slug}/", get_dashboards_response) + + def test_get_dashboards__user_can_not_view_unpublished_dash(self): + # arrange + admin_user = security_manager.find_user(ADMIN_USERNAME) + gamma_user = security_manager.find_user(GAMMA_USERNAME) + slug = f"admin_owned_unpublished_dash_{random_slug()}" + + admin_and_not_published_dashboard = create_dashboard_to_db( + "My Dashboard", slug=slug, owners=[admin_user] + ) + + self.login(gamma_user.username) + + # act - list dashboards as a gamma user + get_dashboards_response_as_gamma = self.get_resp(DASHBOARDS_API_URL) + + # assert + self.assertNotIn( + admin_and_not_published_dashboard.url, get_dashboards_response_as_gamma + ) + + @pytest.mark.usefixtures("load_energy_table_with_slice", "load_dashboard") + def test_get_dashboards__users_can_view_permitted_dashboard(self): + # arrange + accessed_table = get_sql_table_by_name("energy_usage") + self.grant_role_access_to_table(accessed_table, GAMMA_ROLE_NAME) + # get a slice from the allowed table + slice_to_add_to_dashboards = get_slice_by_name("Energy Sankey") + # Create a published and hidden dashboard and add them to the database + first_dash = create_dashboard_to_db( + dashboard_title="Published Dashboard", + slug=f"first_dash_{random_slug()}", + published=True, + slices=[slice_to_add_to_dashboards], + ) + + second_dash = create_dashboard_to_db( + dashboard_title="Hidden Dashboard", + slug=f"second_dash_{random_slug()}", + published=True, + slices=[slice_to_add_to_dashboards], + ) + + try: + self.login(GAMMA_USERNAME) + # act + get_dashboards_response = self.get_resp(DASHBOARDS_API_URL) + + # assert + self.assertIn(second_dash.url, get_dashboards_response) + self.assertIn(first_dash.url, get_dashboards_response) + finally: + self.revoke_public_access_to_table(accessed_table) + + @staticmethod + def __add_dashboard_mode_params(dashboard_url): + full_url = dashboard_url + if dashboard_url.find("?") == -1: + full_url += "?" + else: + full_url += "&" + return full_url + "edit=true&standalone=true" + + def test_get_dashboard_api_no_data_access(self): + """ + Dashboard API: Test get dashboard without data access + """ + admin = self.get_user("admin") + dashboard = create_dashboard_to_db( + random_title(), random_slug(), owners=[admin] + ) + + self.login(username="gamma") + uri = DASHBOARD_API_URL_FORMAT.format(dashboard.id) + rv = self.client.get(uri) + self.assert404(rv) + + def test_get_dashboards_api_no_data_access(self): + """ + Dashboard API: Test get dashboards no data access + """ + admin = self.get_user("admin") + title = f"title{random_str()}" + create_dashboard_to_db(title, "slug1", owners=[admin]) + + self.login(username="gamma") + arguments = { + "filters": [{"col": "dashboard_title", "opr": "sw", "value": title[0:8]}] + } + uri = DASHBOARDS_API_URL_WITH_QUERY_FORMAT.format(prison.dumps(arguments)) + rv = self.client.get(uri) + self.assert200(rv) + data = json.loads(rv.data.decode("utf-8")) + self.assertEqual(0, data["count"]) From af5d30fbca8aaa78ba8a492af295103ddbb03190 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 27 Jan 2021 08:43:56 +0200 Subject: [PATCH 22/49] fix: decouple test --- tests/dashboards/security/security_rbac_tests.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index 4c4bf1d6e8f0d..d074b9dba4380 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -177,7 +177,9 @@ def test_get_dashboards_api__admin_get_all_dashboards(self): def test_get_dashboards_api__owner_get_all_owned_dashboards(self): # arrange - owner = self.get_user(USER_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS) + username = random_str() + new_role = f"role_{random_str()}" + owner = self.create_user_with_roles(username, [new_role]) database = create_database_to_db() table = create_datasource_table_to_db(db_id=database.id, owners=[owner]) first_dash = create_dashboard_to_db( @@ -193,7 +195,7 @@ def test_get_dashboards_api__owner_get_all_owned_dashboards(self): ) ] - self.login(USER_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS) + self.login(username) # act response = self.get_dashboards_api_response() From b4d2cbbf27f94d8e027b0e6154069cf55c8f3f50 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 27 Jan 2021 09:21:00 +0200 Subject: [PATCH 23/49] fix: add_role if not exists --- tests/base_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/base_tests.py b/tests/base_tests.py index 40794ea1a4f07..dd0e3e84803bd 100644 --- a/tests/base_tests.py +++ b/tests/base_tests.py @@ -149,7 +149,7 @@ def create_user_with_roles(username: str, roles: List[str]): db.session.commit() user_to_create = security_manager.find_user(username) assert user_to_create - user_to_create.roles = [security_manager.find_role(r) for r in roles] + user_to_create.roles = [security_manager.add_role(r) for r in roles] db.session.commit() return user_to_create From 906fb5065bab121a750eda9a605a3862710861fa Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 27 Jan 2021 09:51:38 +0200 Subject: [PATCH 24/49] fix: decouple another test --- tests/dashboards/security/security_rbac_tests.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index d074b9dba4380..14922038ae5e5 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -216,6 +216,9 @@ def test_get_dashboards_api__user_without_any_permissions_get_empty_list(self): self.assert_dashboards_api_response(response, 0) def test_get_dashboards_api__user_get_only_published_permitted_dashboards(self): + username = random_str() + new_role = f"role_{random_str()}" + owner = self.create_user_with_roles(username, [new_role]) # arrange published_dashboards = [ create_dashboard_to_db(published=True), @@ -228,10 +231,10 @@ def test_get_dashboards_api__user_get_only_published_permitted_dashboards(self): for dash in published_dashboards + not_published_dashboards: self.grant_access_to_dashboard( - dash, ROLE_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS + dash, new_role ) - self.login(USER_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS) + self.login(username) # act response = self.get_dashboards_api_response() @@ -247,7 +250,7 @@ def test_get_dashboards_api__user_get_only_published_permitted_dashboards(self): # post for dash in published_dashboards + not_published_dashboards: self.revoke_access_to_dashboard( - dash, ROLE_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS + dash, new_role ) def test_get_dashboards_api__public_user_without_any_permissions_get_empty_list( From 16d40617287cfbbf4698044e0e0b91adc86668b1 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 27 Jan 2021 11:24:43 +0200 Subject: [PATCH 25/49] fix: decouple another test --- tests/dashboards/security/security_rbac_tests.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index 14922038ae5e5..22b8f71233f1c 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -206,8 +206,11 @@ def test_get_dashboards_api__owner_get_all_owned_dashboards(self): ) def test_get_dashboards_api__user_without_any_permissions_get_empty_list(self): + username = random_str() + new_role = f"role_{random_str()}" + self.create_user_with_roles(username, [new_role]) create_dashboard_to_db(published=True) - self.login(USER_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS) + self.login(username) # act response = self.get_dashboards_api_response() @@ -218,7 +221,7 @@ def test_get_dashboards_api__user_without_any_permissions_get_empty_list(self): def test_get_dashboards_api__user_get_only_published_permitted_dashboards(self): username = random_str() new_role = f"role_{random_str()}" - owner = self.create_user_with_roles(username, [new_role]) + self.create_user_with_roles(username, [new_role]) # arrange published_dashboards = [ create_dashboard_to_db(published=True), From 0c11ffcfbf7bb2e3cfb79a90c52bf3ba3c1269a6 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 27 Jan 2021 12:00:59 +0200 Subject: [PATCH 26/49] fix: pre-commit :) --- tests/dashboards/security/security_rbac_tests.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index 22b8f71233f1c..ca93399ee369f 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -233,9 +233,7 @@ def test_get_dashboards_api__user_get_only_published_permitted_dashboards(self): ] for dash in published_dashboards + not_published_dashboards: - self.grant_access_to_dashboard( - dash, new_role - ) + self.grant_access_to_dashboard(dash, new_role) self.login(username) @@ -252,9 +250,7 @@ def test_get_dashboards_api__user_get_only_published_permitted_dashboards(self): # post for dash in published_dashboards + not_published_dashboards: - self.revoke_access_to_dashboard( - dash, new_role - ) + self.revoke_access_to_dashboard(dash, new_role) def test_get_dashboards_api__public_user_without_any_permissions_get_empty_list( self, From 9c0b6a66a690cd9a563bb5cdb0d996b3391879c4 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 27 Jan 2021 15:30:19 +0200 Subject: [PATCH 27/49] fix: make user creation with roles to copy role form gamma and and to leave behind dataset permissions --- tests/base_tests.py | 28 +++++++++++-------- .../security/security_rbac_tests.py | 6 ++-- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/tests/base_tests.py b/tests/base_tests.py index dd0e3e84803bd..7936538ad8074 100644 --- a/tests/base_tests.py +++ b/tests/base_tests.py @@ -111,7 +111,6 @@ def logged_in_admin(): class SupersetTestCase(TestCase): - default_schema_backend_map = { "sqlite": "main", "mysql": "superset", @@ -130,8 +129,8 @@ def get_birth_names_dataset(): example_db = get_example_database() return ( db.session.query(SqlaTable) - .filter_by(database=example_db, table_name="birth_names") - .one() + .filter_by(database=example_db, table_name="birth_names") + .one() ) @staticmethod @@ -149,7 +148,12 @@ def create_user_with_roles(username: str, roles: List[str]): db.session.commit() user_to_create = security_manager.find_user(username) assert user_to_create - user_to_create.roles = [security_manager.add_role(r) for r in roles] + user_to_create.roles = [] + for chosen_user_role in roles: + copy_from_role = 'Gamma' + if chosen_user_role != copy_from_role: + security_manager.copy_role(copy_from_role, chosen_user_role, False) + user_to_create.roles.append(security_manager.find_role(chosen_user_role)) db.session.commit() return user_to_create @@ -171,8 +175,8 @@ def create_user( def get_user(username: str) -> ab_models.User: user = ( db.session.query(security_manager.user_model) - .filter_by(username=username) - .one_or_none() + .filter_by(username=username) + .one_or_none() ) return user @@ -278,12 +282,12 @@ def get_access_requests(self, username, ds_type, ds_id): DAR = DatasourceAccessRequest return ( db.session.query(DAR) - .filter( + .filter( DAR.created_by == security_manager.find_user(username=username), DAR.datasource_type == ds_type, DAR.datasource_id == ds_id, ) - .first() + .first() ) def logout(self): @@ -400,8 +404,8 @@ def create_fake_db(self): def delete_fake_db(self): database = ( db.session.query(Database) - .filter(Database.database_name == FAKE_DB_NAME) - .scalar() + .filter(Database.database_name == FAKE_DB_NAME) + .scalar() ) if database: db.session.delete(database) @@ -421,8 +425,8 @@ def create_fake_db_for_macros(self): def delete_fake_db_for_macros(self): database = ( db.session.query(Database) - .filter(Database.database_name == "db_for_macros_testing") - .scalar() + .filter(Database.database_name == "db_for_macros_testing") + .scalar() ) if database: db.session.delete(database) diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index ca93399ee369f..8954ae942270d 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -47,7 +47,9 @@ def test_get_dashboards_list__admin_get_all_dashboards(self): def test_get_dashboards_list__owner_get_all_owned_dashboards(self): # arrange - owner = self.get_user(USER_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS) + username = random_str() + new_role = f"role_{random_str()}" + owner = self.create_user_with_roles(username, [new_role]) database = create_database_to_db() table = create_datasource_table_to_db(db_id=database.id, owners=[owner]) first_dash = create_dashboard_to_db( @@ -63,7 +65,7 @@ def test_get_dashboards_list__owner_get_all_owned_dashboards(self): ) ] - self.login(USER_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS) + self.login(username) # act response = self.get_dashboards_list_response() From 2b8cd8e9dde920e8b12add13851c24dff89ce5cb Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 27 Jan 2021 16:38:36 +0200 Subject: [PATCH 28/49] fix: make copy_role optional and False by default --- tests/base_tests.py | 29 ++++++++++--------- .../security/security_rbac_tests.py | 8 ++--- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/tests/base_tests.py b/tests/base_tests.py index 7936538ad8074..d9d626c6beb1c 100644 --- a/tests/base_tests.py +++ b/tests/base_tests.py @@ -129,12 +129,14 @@ def get_birth_names_dataset(): example_db = get_example_database() return ( db.session.query(SqlaTable) - .filter_by(database=example_db, table_name="birth_names") - .one() + .filter_by(database=example_db, table_name="birth_names") + .one() ) @staticmethod - def create_user_with_roles(username: str, roles: List[str]): + def create_user_with_roles( + username: str, roles: List[str], copy_roles: bool = False + ): user_to_create = security_manager.find_user(username) if not user_to_create: security_manager.add_user( @@ -150,9 +152,8 @@ def create_user_with_roles(username: str, roles: List[str]): assert user_to_create user_to_create.roles = [] for chosen_user_role in roles: - copy_from_role = 'Gamma' - if chosen_user_role != copy_from_role: - security_manager.copy_role(copy_from_role, chosen_user_role, False) + if copy_roles: + security_manager.copy_role("Gamma", chosen_user_role, False) user_to_create.roles.append(security_manager.find_role(chosen_user_role)) db.session.commit() return user_to_create @@ -175,8 +176,8 @@ def create_user( def get_user(username: str) -> ab_models.User: user = ( db.session.query(security_manager.user_model) - .filter_by(username=username) - .one_or_none() + .filter_by(username=username) + .one_or_none() ) return user @@ -282,12 +283,12 @@ def get_access_requests(self, username, ds_type, ds_id): DAR = DatasourceAccessRequest return ( db.session.query(DAR) - .filter( + .filter( DAR.created_by == security_manager.find_user(username=username), DAR.datasource_type == ds_type, DAR.datasource_id == ds_id, ) - .first() + .first() ) def logout(self): @@ -404,8 +405,8 @@ def create_fake_db(self): def delete_fake_db(self): database = ( db.session.query(Database) - .filter(Database.database_name == FAKE_DB_NAME) - .scalar() + .filter(Database.database_name == FAKE_DB_NAME) + .scalar() ) if database: db.session.delete(database) @@ -425,8 +426,8 @@ def create_fake_db_for_macros(self): def delete_fake_db_for_macros(self): database = ( db.session.query(Database) - .filter(Database.database_name == "db_for_macros_testing") - .scalar() + .filter(Database.database_name == "db_for_macros_testing") + .scalar() ) if database: db.session.delete(database) diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index 8954ae942270d..80964b6450cb1 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -49,7 +49,7 @@ def test_get_dashboards_list__owner_get_all_owned_dashboards(self): # arrange username = random_str() new_role = f"role_{random_str()}" - owner = self.create_user_with_roles(username, [new_role]) + owner = self.create_user_with_roles(username, [new_role], copy_roles=True) database = create_database_to_db() table = create_datasource_table_to_db(db_id=database.id, owners=[owner]) first_dash = create_dashboard_to_db( @@ -181,7 +181,7 @@ def test_get_dashboards_api__owner_get_all_owned_dashboards(self): # arrange username = random_str() new_role = f"role_{random_str()}" - owner = self.create_user_with_roles(username, [new_role]) + owner = self.create_user_with_roles(username, [new_role], copy_roles=True) database = create_database_to_db() table = create_datasource_table_to_db(db_id=database.id, owners=[owner]) first_dash = create_dashboard_to_db( @@ -210,7 +210,7 @@ def test_get_dashboards_api__owner_get_all_owned_dashboards(self): def test_get_dashboards_api__user_without_any_permissions_get_empty_list(self): username = random_str() new_role = f"role_{random_str()}" - self.create_user_with_roles(username, [new_role]) + self.create_user_with_roles(username, [new_role], copy_roles=True) create_dashboard_to_db(published=True) self.login(username) @@ -223,7 +223,7 @@ def test_get_dashboards_api__user_without_any_permissions_get_empty_list(self): def test_get_dashboards_api__user_get_only_published_permitted_dashboards(self): username = random_str() new_role = f"role_{random_str()}" - self.create_user_with_roles(username, [new_role]) + self.create_user_with_roles(username, [new_role], copy_roles=True) # arrange published_dashboards = [ create_dashboard_to_db(published=True), From 83d76cec371f7ba7041da414514f04e1262da174 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 27 Jan 2021 17:07:10 +0200 Subject: [PATCH 29/49] fix: pre commit ;) --- tests/dashboards/base_case.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/dashboards/base_case.py b/tests/dashboards/base_case.py index fe999a6ea75b4..658909478d9c3 100644 --- a/tests/dashboards/base_case.py +++ b/tests/dashboards/base_case.py @@ -15,7 +15,7 @@ # specific language governing permissions and limitations # under the License. import json -from typing import Dict, Union +from typing import Dict, Union, Any import prison from flask import Response @@ -45,13 +45,13 @@ def get_dashboards_api_response(self) -> Response: return self.client.get(DASHBOARDS_API_URL) def save_dashboard_via_view( - self, dashboard_id: Union[str, int], dashboard_data: Dict + self, dashboard_id: Union[str, int], dashboard_data: Dict[str, Any] ) -> Response: save_dash_url = SAVE_DASHBOARD_URL_FORMAT.format(dashboard_id) return self.get_resp(save_dash_url, data=dict(data=json.dumps(dashboard_data))) def save_dashboard( - self, dashboard_id: Union[str, int], dashboard_data: Dict + self, dashboard_id: Union[str, int], dashboard_data: Dict[str, Any] ) -> Response: return self.save_dashboard_via_view(dashboard_id, dashboard_data) From 90a32fd5e87a601cf9087b21713e7515764cbf2e Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 27 Jan 2021 17:11:22 +0200 Subject: [PATCH 30/49] fix: pre commit ;) --- tests/dashboards/base_case.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/dashboards/base_case.py b/tests/dashboards/base_case.py index 658909478d9c3..42cd87bfd93e1 100644 --- a/tests/dashboards/base_case.py +++ b/tests/dashboards/base_case.py @@ -15,7 +15,7 @@ # specific language governing permissions and limitations # under the License. import json -from typing import Dict, Union, Any +from typing import Any, Dict, Union import prison from flask import Response From e7a4770c79134eed5a472bc4f3180b09249db917 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 27 Jan 2021 18:08:40 +0200 Subject: [PATCH 31/49] fix: hopfully last decoupling --- tests/dashboards/security/security_rbac_tests.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index 80964b6450cb1..65ba713693c79 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -87,6 +87,10 @@ def test_get_dashboards_list__user_without_any_permissions_get_empty_list(self): def test_get_dashboards_list__user_get_only_published_permitted_dashboards(self): # arrange + username = random_str() + new_role = f"role_{random_str()}" + self.create_user_with_roles(username, [new_role], copy_roles=True) + published_dashboards = [ create_dashboard_to_db(published=True), create_dashboard_to_db(published=True), @@ -98,10 +102,10 @@ def test_get_dashboards_list__user_get_only_published_permitted_dashboards(self) for dash in published_dashboards + not_published_dashboards: self.grant_access_to_dashboard( - dash, ROLE_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS + dash, new_role ) - self.login(USER_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS) + self.login(username) # act response = self.get_dashboards_list_response() @@ -117,7 +121,7 @@ def test_get_dashboards_list__user_get_only_published_permitted_dashboards(self) # post for dash in published_dashboards + not_published_dashboards: self.revoke_access_to_dashboard( - dash, ROLE_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS + dash, new_role ) def test_get_dashboards_list__public_user_without_any_permissions_get_empty_list( From 703621696b7504244a7b5ea9afe33d17563be740 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 27 Jan 2021 19:44:48 +0200 Subject: [PATCH 32/49] fix: remove dependency from gamma user --- tests/dashboards/security/security_rbac_tests.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index 65ba713693c79..3e8a408bb3195 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -25,9 +25,6 @@ create_slice_to_db, ) -USER_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS = "gamma" -ROLE_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS = "Gamma" - class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity): def test_get_dashboards_list__admin_get_all_dashboards(self): @@ -76,8 +73,14 @@ def test_get_dashboards_list__owner_get_all_owned_dashboards(self): ) def test_get_dashboards_list__user_without_any_permissions_get_empty_list(self): + + # arrange + username = random_str() + new_role = f"role_{random_str()}" + self.create_user_with_roles(username, [new_role], copy_roles=True) + create_dashboard_to_db(published=True) - self.login(USER_WITHOUT_DASHBOARDS_ACCESS_PERMISSIONS) + self.login(username) # act response = self.get_dashboards_list_response() From 14a7d7451eb3fba7659e004326b88e9e3689eda3 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 27 Jan 2021 21:50:05 +0200 Subject: [PATCH 33/49] fix: another try --- tests/dashboards/consts.py | 2 -- tests/dashboards/security/security_dataset_tests.py | 8 ++++++-- tests/dashboards/security/security_rbac_tests.py | 8 ++------ 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/tests/dashboards/consts.py b/tests/dashboards/consts.py index f76e4a552dbe5..a6e36839be9ed 100644 --- a/tests/dashboards/consts.py +++ b/tests/dashboards/consts.py @@ -36,10 +36,8 @@ GAMMA_ROLE_NAME = "Gamma" ADMIN_USERNAME = "admin" -ALPHA_USERNAME = "alpha" GAMMA_USERNAME = "gamma" -DEFAULT_ACCESSIBLE_TABLE = "birth_names" DASHBOARD_SLUG_OF_ACCESSIBLE_TABLE = "births" DEFAULT_DASHBOARD_SLUG_TO_TEST = "births" WORLD_HEALTH_SLUG = "world_health" diff --git a/tests/dashboards/security/security_dataset_tests.py b/tests/dashboards/security/security_dataset_tests.py index 149283e1ebd56..fa3c8f8d1c2ae 100644 --- a/tests/dashboards/security/security_dataset_tests.py +++ b/tests/dashboards/security/security_dataset_tests.py @@ -30,6 +30,7 @@ from tests.fixtures.energy_dashboard import load_energy_table_with_slice +# @mark.amit class TestDashboardDatasetSecurity(DashboardTestCase): @pytest.fixture def load_dashboard(self): @@ -184,8 +185,11 @@ def test_get_dashboards__user_can_not_view_unpublished_dash(self): @pytest.mark.usefixtures("load_energy_table_with_slice", "load_dashboard") def test_get_dashboards__users_can_view_permitted_dashboard(self): # arrange + username = random_str() + new_role = f"role_{random_str()}" + self.create_user_with_roles(username, [new_role], copy_roles=True) accessed_table = get_sql_table_by_name("energy_usage") - self.grant_role_access_to_table(accessed_table, GAMMA_ROLE_NAME) + self.grant_role_access_to_table(accessed_table, new_role) # get a slice from the allowed table slice_to_add_to_dashboards = get_slice_by_name("Energy Sankey") # Create a published and hidden dashboard and add them to the database @@ -204,7 +208,7 @@ def test_get_dashboards__users_can_view_permitted_dashboard(self): ) try: - self.login(GAMMA_USERNAME) + self.login(username) # act get_dashboards_response = self.get_resp(DASHBOARDS_API_URL) diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index 3e8a408bb3195..1a058173558d1 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -104,9 +104,7 @@ def test_get_dashboards_list__user_get_only_published_permitted_dashboards(self) ] for dash in published_dashboards + not_published_dashboards: - self.grant_access_to_dashboard( - dash, new_role - ) + self.grant_access_to_dashboard(dash, new_role) self.login(username) @@ -123,9 +121,7 @@ def test_get_dashboards_list__user_get_only_published_permitted_dashboards(self) # post for dash in published_dashboards + not_published_dashboards: - self.revoke_access_to_dashboard( - dash, new_role - ) + self.revoke_access_to_dashboard(dash, new_role) def test_get_dashboards_list__public_user_without_any_permissions_get_empty_list( self, From 2c2ade827f8f9bfb089efab1c7e3a143fa691fe5 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Thu, 28 Jan 2021 17:28:25 +0200 Subject: [PATCH 34/49] fix: after cr comments --- superset/views/dashboard/mixin.py | 5 +++-- tests/base_tests.py | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/superset/views/dashboard/mixin.py b/superset/views/dashboard/mixin.py index 30e0d7948d4e3..eab2f061ffacd 100644 --- a/superset/views/dashboard/mixin.py +++ b/superset/views/dashboard/mixin.py @@ -64,8 +64,9 @@ class DashboardMixin: # pylint: disable=too-few-public-methods ), "owners": _("Owners is a list of users who can alter the dashboard."), "roles": _( - "Roles is a list which defines access to the dashboard. if list is " - "empty access is managed by the data access level." + "Roles is a list which defines access to the dashboard. " + "these roles are always applied in addition to data access leve. " + "if no roles then dashboard is available for all roles" ), "published": _( "Determines whether or not this dashboard is " diff --git a/tests/base_tests.py b/tests/base_tests.py index d9d626c6beb1c..e64f9ffcb088c 100644 --- a/tests/base_tests.py +++ b/tests/base_tests.py @@ -294,13 +294,13 @@ def get_access_requests(self, username, ds_type, ds_id): def logout(self): self.client.get("/logout/", follow_redirects=True) - def grant_access_to_dashboard(self, dashboard, role_name="Public"): + def grant_access_to_dashboard(self, dashboard, role_name): role = security_manager.find_role(role_name) dashboard.roles.append(role) db.session.merge(dashboard) db.session.commit() - def revoke_access_to_dashboard(self, dashboard, role_name="Public"): + def revoke_access_to_dashboard(self, dashboard, role_name): role = security_manager.find_role(role_name) dashboard.roles.remove(role) db.session.merge(dashboard) From 903e3ba0a6b4b7d06982f72ec21ece2c0435cd29 Mon Sep 17 00:00:00 2001 From: Amit Miran <47772523+amitmiran137@users.noreply.github.com> Date: Fri, 29 Jan 2021 10:17:57 +0200 Subject: [PATCH 35/49] Update roles description Co-authored-by: Jesse Yang --- superset/views/dashboard/mixin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/views/dashboard/mixin.py b/superset/views/dashboard/mixin.py index eab2f061ffacd..fc44c1fcb32e0 100644 --- a/superset/views/dashboard/mixin.py +++ b/superset/views/dashboard/mixin.py @@ -65,8 +65,8 @@ class DashboardMixin: # pylint: disable=too-few-public-methods "owners": _("Owners is a list of users who can alter the dashboard."), "roles": _( "Roles is a list which defines access to the dashboard. " - "these roles are always applied in addition to data access leve. " - "if no roles then dashboard is available for all roles" + "These roles are always applied in addition to restrictions on dataset level access. " + "If no roles defined then the dashboard is available to all roles." ), "published": _( "Determines whether or not this dashboard is " From 63051e78c98c97acacc8dd0d2f22abaea90f0ef3 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Fri, 29 Jan 2021 12:54:10 +0200 Subject: [PATCH 36/49] chore(after CR) : dashboard_has_roles variable and usages in queries should_create_roles --- superset/dashboards/filters.py | 8 +++----- tests/base_tests.py | 7 ++++--- .../security/security_dataset_tests.py | 3 +-- tests/dashboards/security/security_rbac_tests.py | 16 ++++++++++------ 4 files changed, 18 insertions(+), 16 deletions(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index 4cd567d4ebbce..38202feebbeff 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -75,13 +75,14 @@ def apply(self, query: Query, value: Any) -> Query: datasource_perms = security_manager.user_view_menu_names("datasource_access") schema_perms = security_manager.user_view_menu_names("schema_access") + dashboard_has_roles = Dashboard.roles.any() datesource_perm_query = ( db.session.query(Dashboard.id) .join(Dashboard.slices) .filter( and_( Dashboard.published.is_(True), - self.has_no_role_based_access(), + ~dashboard_has_roles, or_( Slice.perm.in_(datasource_perms), Slice.schema_perm.in_(schema_perms), @@ -96,6 +97,7 @@ def apply(self, query: Query, value: Any) -> Query: .filter( and_( Dashboard.published.is_(True), + dashboard_has_roles, Role.id.in_([x.id for x in get_user_roles()]), ), ) @@ -126,7 +128,3 @@ def apply(self, query: Query, value: Any) -> Query: ) return query - - @staticmethod - def has_no_role_based_access() -> bool: - return ~Dashboard.roles.any() diff --git a/tests/base_tests.py b/tests/base_tests.py index e64f9ffcb088c..074ed2ef9e422 100644 --- a/tests/base_tests.py +++ b/tests/base_tests.py @@ -135,7 +135,7 @@ def get_birth_names_dataset(): @staticmethod def create_user_with_roles( - username: str, roles: List[str], copy_roles: bool = False + username: str, roles: List[str], should_create_roles: bool = False ): user_to_create = security_manager.find_user(username) if not user_to_create: @@ -152,8 +152,9 @@ def create_user_with_roles( assert user_to_create user_to_create.roles = [] for chosen_user_role in roles: - if copy_roles: - security_manager.copy_role("Gamma", chosen_user_role, False) + if should_create_roles: + ## copy role from gamma but without data permissions + security_manager.copy_role("Gamma", chosen_user_role, merge=False) user_to_create.roles.append(security_manager.find_role(chosen_user_role)) db.session.commit() return user_to_create diff --git a/tests/dashboards/security/security_dataset_tests.py b/tests/dashboards/security/security_dataset_tests.py index fa3c8f8d1c2ae..c6e0274ca6f14 100644 --- a/tests/dashboards/security/security_dataset_tests.py +++ b/tests/dashboards/security/security_dataset_tests.py @@ -30,7 +30,6 @@ from tests.fixtures.energy_dashboard import load_energy_table_with_slice -# @mark.amit class TestDashboardDatasetSecurity(DashboardTestCase): @pytest.fixture def load_dashboard(self): @@ -187,7 +186,7 @@ def test_get_dashboards__users_can_view_permitted_dashboard(self): # arrange username = random_str() new_role = f"role_{random_str()}" - self.create_user_with_roles(username, [new_role], copy_roles=True) + self.create_user_with_roles(username, [new_role], should_create_roles=True) accessed_table = get_sql_table_by_name("energy_usage") self.grant_role_access_to_table(accessed_table, new_role) # get a slice from the allowed table diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index 1a058173558d1..c588869ef4f16 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -46,7 +46,9 @@ def test_get_dashboards_list__owner_get_all_owned_dashboards(self): # arrange username = random_str() new_role = f"role_{random_str()}" - owner = self.create_user_with_roles(username, [new_role], copy_roles=True) + owner = self.create_user_with_roles( + username, [new_role], should_create_roles=True + ) database = create_database_to_db() table = create_datasource_table_to_db(db_id=database.id, owners=[owner]) first_dash = create_dashboard_to_db( @@ -77,7 +79,7 @@ def test_get_dashboards_list__user_without_any_permissions_get_empty_list(self): # arrange username = random_str() new_role = f"role_{random_str()}" - self.create_user_with_roles(username, [new_role], copy_roles=True) + self.create_user_with_roles(username, [new_role], should_create_roles=True) create_dashboard_to_db(published=True) self.login(username) @@ -92,7 +94,7 @@ def test_get_dashboards_list__user_get_only_published_permitted_dashboards(self) # arrange username = random_str() new_role = f"role_{random_str()}" - self.create_user_with_roles(username, [new_role], copy_roles=True) + self.create_user_with_roles(username, [new_role], should_create_roles=True) published_dashboards = [ create_dashboard_to_db(published=True), @@ -184,7 +186,9 @@ def test_get_dashboards_api__owner_get_all_owned_dashboards(self): # arrange username = random_str() new_role = f"role_{random_str()}" - owner = self.create_user_with_roles(username, [new_role], copy_roles=True) + owner = self.create_user_with_roles( + username, [new_role], should_create_roles=True + ) database = create_database_to_db() table = create_datasource_table_to_db(db_id=database.id, owners=[owner]) first_dash = create_dashboard_to_db( @@ -213,7 +217,7 @@ def test_get_dashboards_api__owner_get_all_owned_dashboards(self): def test_get_dashboards_api__user_without_any_permissions_get_empty_list(self): username = random_str() new_role = f"role_{random_str()}" - self.create_user_with_roles(username, [new_role], copy_roles=True) + self.create_user_with_roles(username, [new_role], should_create_roles=True) create_dashboard_to_db(published=True) self.login(username) @@ -226,7 +230,7 @@ def test_get_dashboards_api__user_without_any_permissions_get_empty_list(self): def test_get_dashboards_api__user_get_only_published_permitted_dashboards(self): username = random_str() new_role = f"role_{random_str()}" - self.create_user_with_roles(username, [new_role], copy_roles=True) + self.create_user_with_roles(username, [new_role], should_create_roles=True) # arrange published_dashboards = [ create_dashboard_to_db(published=True), From 7b7da449c78fbc0e4227af07c73b09d353c9fdc6 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Fri, 29 Jan 2021 13:51:40 +0200 Subject: [PATCH 37/49] chore(after CR) : move to dashboard util --- tests/base_tests.py | 12 ------------ tests/dashboards/dashboard_test_utils.py | 16 +++++++++++++++- tests/dashboards/security/security_rbac_tests.py | 16 ++++++++-------- tests/dashboards/superset_factory_util.py | 2 +- 4 files changed, 24 insertions(+), 22 deletions(-) diff --git a/tests/base_tests.py b/tests/base_tests.py index 074ed2ef9e422..a5fd9d5382da3 100644 --- a/tests/base_tests.py +++ b/tests/base_tests.py @@ -295,18 +295,6 @@ def get_access_requests(self, username, ds_type, ds_id): def logout(self): self.client.get("/logout/", follow_redirects=True) - def grant_access_to_dashboard(self, dashboard, role_name): - role = security_manager.find_role(role_name) - dashboard.roles.append(role) - db.session.merge(dashboard) - db.session.commit() - - def revoke_access_to_dashboard(self, dashboard, role_name): - role = security_manager.find_role(role_name) - dashboard.roles.remove(role) - db.session.merge(dashboard) - db.session.commit() - def grant_public_access_to_table(self, table): role_name = "Public" self.grant_role_access_to_table(table, role_name) diff --git a/tests/dashboards/dashboard_test_utils.py b/tests/dashboards/dashboard_test_utils.py index 9fe017cc8675c..c7032f87e6481 100644 --- a/tests/dashboards/dashboard_test_utils.py +++ b/tests/dashboards/dashboard_test_utils.py @@ -21,7 +21,7 @@ from sqlalchemy import func -from superset import appbuilder, db +from superset import appbuilder, db, security_manager from superset.connectors.sqla.models import SqlaTable from superset.models.dashboard import Dashboard from superset.models.slice import Slice @@ -105,3 +105,17 @@ def get_random_string(length): def random_str(): return get_random_string(8) + + +def grant_access_to_dashboard(dashboard, role_name): + role = security_manager.find_role(role_name) + dashboard.roles.append(role) + db.session.merge(dashboard) + db.session.commit() + + +def revoke_access_to_dashboard(dashboard, role_name): + role = security_manager.find_role(role_name) + dashboard.roles.remove(role) + db.session.merge(dashboard) + db.session.commit() diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index c588869ef4f16..16c2d45ef5275 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -106,7 +106,7 @@ def test_get_dashboards_list__user_get_only_published_permitted_dashboards(self) ] for dash in published_dashboards + not_published_dashboards: - self.grant_access_to_dashboard(dash, new_role) + grant_access_to_dashboard(dash, new_role) self.login(username) @@ -123,7 +123,7 @@ def test_get_dashboards_list__user_get_only_published_permitted_dashboards(self) # post for dash in published_dashboards + not_published_dashboards: - self.revoke_access_to_dashboard(dash, new_role) + revoke_access_to_dashboard(dash, new_role) def test_get_dashboards_list__public_user_without_any_permissions_get_empty_list( self, @@ -150,7 +150,7 @@ def test_get_dashboards_list__public_user_get_only_published_permitted_dashboard ] for dash in published_dashboards + not_published_dashboards: - self.grant_access_to_dashboard(dash, "Public") + grant_access_to_dashboard(dash, "Public") # act response = self.get_dashboards_list_response() @@ -165,7 +165,7 @@ def test_get_dashboards_list__public_user_get_only_published_permitted_dashboard # post for dash in published_dashboards + not_published_dashboards: - self.revoke_access_to_dashboard(dash, "Public") + revoke_access_to_dashboard(dash, "Public") def test_get_dashboards_api__admin_get_all_dashboards(self): # arrange @@ -242,7 +242,7 @@ def test_get_dashboards_api__user_get_only_published_permitted_dashboards(self): ] for dash in published_dashboards + not_published_dashboards: - self.grant_access_to_dashboard(dash, new_role) + grant_access_to_dashboard(dash, new_role) self.login(username) @@ -259,7 +259,7 @@ def test_get_dashboards_api__user_get_only_published_permitted_dashboards(self): # post for dash in published_dashboards + not_published_dashboards: - self.revoke_access_to_dashboard(dash, new_role) + revoke_access_to_dashboard(dash, new_role) def test_get_dashboards_api__public_user_without_any_permissions_get_empty_list( self, @@ -286,7 +286,7 @@ def test_get_dashboards_api__public_user_get_only_published_permitted_dashboards ] for dash in published_dashboards + not_published_dashboards: - self.grant_access_to_dashboard(dash, "Public") + grant_access_to_dashboard(dash, "Public") # act response = self.get_dashboards_api_response() @@ -301,7 +301,7 @@ def test_get_dashboards_api__public_user_get_only_published_permitted_dashboards # post for dash in published_dashboards + not_published_dashboards: - self.revoke_access_to_dashboard(dash, "Public") + revoke_access_to_dashboard(dash, "Public") @staticmethod def __add_dashboard_mode_params(dashboard_url): diff --git a/tests/dashboards/superset_factory_util.py b/tests/dashboards/superset_factory_util.py index f4459156dffab..bf44e43b930d9 100644 --- a/tests/dashboards/superset_factory_util.py +++ b/tests/dashboards/superset_factory_util.py @@ -15,7 +15,7 @@ # specific language governing permissions and limitations # under the License. import logging -from typing import Any, Callable, List, Optional +from typing import List, Optional from flask_appbuilder import Model from flask_appbuilder.security.sqla.models import User From f6c7cbeeb40a3f1400ecc8e1b6b371b008f67397 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Fri, 29 Jan 2021 14:02:57 +0200 Subject: [PATCH 38/49] chore(after CR) : remove irrelevant slugs --- .../security/security_dataset_tests.py | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/tests/dashboards/security/security_dataset_tests.py b/tests/dashboards/security/security_dataset_tests.py index c6e0274ca6f14..39493b3f26e5a 100644 --- a/tests/dashboards/security/security_dataset_tests.py +++ b/tests/dashboards/security/security_dataset_tests.py @@ -21,7 +21,7 @@ import pytest from flask import escape -from superset import app, security_manager +from superset import app from superset.models import core as models from tests.dashboards.base_case import DashboardTestCase from tests.dashboards.consts import * @@ -91,16 +91,11 @@ def test_get_dashboards__users_are_dashboards_owners(self): username = "gamma" user = security_manager.find_user(username) my_owned_dashboard = create_dashboard_to_db( - dashboard_title="My Dashboard", - slug=f"my_dash_{random_slug()}", - published=False, - owners=[user], + dashboard_title="My Dashboard", published=False, owners=[user], ) not_my_owned_dashboard = create_dashboard_to_db( - dashboard_title="Not My Dashboard", - slug=f"not_my_dash_{random_slug()}", - published=False, + dashboard_title="Not My Dashboard", published=False, ) self.login(user.username) @@ -165,10 +160,8 @@ def test_get_dashboards__user_can_not_view_unpublished_dash(self): # arrange admin_user = security_manager.find_user(ADMIN_USERNAME) gamma_user = security_manager.find_user(GAMMA_USERNAME) - slug = f"admin_owned_unpublished_dash_{random_slug()}" - admin_and_not_published_dashboard = create_dashboard_to_db( - "My Dashboard", slug=slug, owners=[admin_user] + dashboard_title="admin_owned_unpublished_dash", owners=[admin_user] ) self.login(gamma_user.username) @@ -194,14 +187,12 @@ def test_get_dashboards__users_can_view_permitted_dashboard(self): # Create a published and hidden dashboard and add them to the database first_dash = create_dashboard_to_db( dashboard_title="Published Dashboard", - slug=f"first_dash_{random_slug()}", published=True, slices=[slice_to_add_to_dashboards], ) second_dash = create_dashboard_to_db( dashboard_title="Hidden Dashboard", - slug=f"second_dash_{random_slug()}", published=True, slices=[slice_to_add_to_dashboards], ) From 225493db40574595c8e4cc682481d540356e1b25 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Fri, 29 Jan 2021 14:21:28 +0200 Subject: [PATCH 39/49] chore(after CR) : remove unused method --- tests/dashboards/security/security_dataset_tests.py | 9 --------- tests/dashboards/security/security_rbac_tests.py | 9 --------- 2 files changed, 18 deletions(-) diff --git a/tests/dashboards/security/security_dataset_tests.py b/tests/dashboards/security/security_dataset_tests.py index 39493b3f26e5a..842f5c09f3968 100644 --- a/tests/dashboards/security/security_dataset_tests.py +++ b/tests/dashboards/security/security_dataset_tests.py @@ -208,15 +208,6 @@ def test_get_dashboards__users_can_view_permitted_dashboard(self): finally: self.revoke_public_access_to_table(accessed_table) - @staticmethod - def __add_dashboard_mode_params(dashboard_url): - full_url = dashboard_url - if dashboard_url.find("?") == -1: - full_url += "?" - else: - full_url += "&" - return full_url + "edit=true&standalone=true" - def test_get_dashboard_api_no_data_access(self): """ Dashboard API: Test get dashboard without data access diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index 16c2d45ef5275..e7924f2a928e3 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -302,12 +302,3 @@ def test_get_dashboards_api__public_user_get_only_published_permitted_dashboards # post for dash in published_dashboards + not_published_dashboards: revoke_access_to_dashboard(dash, "Public") - - @staticmethod - def __add_dashboard_mode_params(dashboard_url): - full_url = dashboard_url - if dashboard_url.find("?") == -1: - full_url += "?" - else: - full_url += "&" - return full_url + "edit=true&standalone=true" From 60d2d91d10401f8e6f258c88339f2a1162a355a6 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Fri, 29 Jan 2021 14:25:59 +0200 Subject: [PATCH 40/49] chore(after CR) : remove const --- tests/dashboards/security/base_case.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/dashboards/security/base_case.py b/tests/dashboards/security/base_case.py index 89d9a8517cb49..3e85114663dbb 100644 --- a/tests/dashboards/security/base_case.py +++ b/tests/dashboards/security/base_case.py @@ -21,8 +21,6 @@ from superset.models.dashboard import Dashboard from tests.dashboards.base_case import DashboardTestCase -DASHBOARD_COUNT_IN_DASHBOARDS_LIST_VIEW_FORMAT = "Record Count: {count}" - class BaseTestDashboardSecurity(DashboardTestCase): def tearDown(self) -> None: @@ -54,8 +52,10 @@ def assert_dashboards_list_view_response( if expected_counts == 0: assert "No records found" in response_html else: + # # a way to parse number of dashboards returns + # in the list view as an html response assert ( - DASHBOARD_COUNT_IN_DASHBOARDS_LIST_VIEW_FORMAT.format( + "Record Count: {count}".format( count=str(expected_counts) ) in response_html From 84bb61ee69eb2f94930c4ae769c3c07f99dae445 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Fri, 29 Jan 2021 23:08:43 +0200 Subject: [PATCH 41/49] chore: changes after CR --- superset/dashboards/filters.py | 4 ++-- superset/views/dashboard/mixin.py | 3 ++- tests/dashboards/security/base_case.py | 4 +--- tests/dashboards/superset_factory_util.py | 4 +++- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index 38202feebbeff..c19be95568841 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -76,7 +76,7 @@ def apply(self, query: Query, value: Any) -> Query: datasource_perms = security_manager.user_view_menu_names("datasource_access") schema_perms = security_manager.user_view_menu_names("schema_access") dashboard_has_roles = Dashboard.roles.any() - datesource_perm_query = ( + datasource_perm_query = ( db.session.query(Dashboard.id) .join(Dashboard.slices) .filter( @@ -121,7 +121,7 @@ def apply(self, query: Query, value: Any) -> Query: query = query.filter( or_( Dashboard.id.in_(owner_ids_query), - Dashboard.id.in_(datesource_perm_query), + Dashboard.id.in_(datasource_perm_query), Dashboard.id.in_(users_favorite_dash_query), Dashboard.id.in_(roles_based_query), ) diff --git a/superset/views/dashboard/mixin.py b/superset/views/dashboard/mixin.py index fc44c1fcb32e0..273cfdf0b3c2f 100644 --- a/superset/views/dashboard/mixin.py +++ b/superset/views/dashboard/mixin.py @@ -65,7 +65,8 @@ class DashboardMixin: # pylint: disable=too-few-public-methods "owners": _("Owners is a list of users who can alter the dashboard."), "roles": _( "Roles is a list which defines access to the dashboard. " - "These roles are always applied in addition to restrictions on dataset level access. " + "These roles are always applied in addition to restrictions on dataset " + "level access. " "If no roles defined then the dashboard is available to all roles." ), "published": _( diff --git a/tests/dashboards/security/base_case.py b/tests/dashboards/security/base_case.py index 3e85114663dbb..6446e1add62e6 100644 --- a/tests/dashboards/security/base_case.py +++ b/tests/dashboards/security/base_case.py @@ -55,9 +55,7 @@ def assert_dashboards_list_view_response( # # a way to parse number of dashboards returns # in the list view as an html response assert ( - "Record Count: {count}".format( - count=str(expected_counts) - ) + "Record Count: {count}".format(count=str(expected_counts)) in response_html ) expected_dashboards = expected_dashboards or [] diff --git a/tests/dashboards/superset_factory_util.py b/tests/dashboards/superset_factory_util.py index bf44e43b930d9..f62f4ac2cddf7 100644 --- a/tests/dashboards/superset_factory_util.py +++ b/tests/dashboards/superset_factory_util.py @@ -111,7 +111,9 @@ def create_slice_to_db( return slice_ -def create_slice(datasource_id, name, owners): +def create_slice( + datasource_id: Optional[int], name: Optional[str], owners: Optional[List[User]] +) -> Slice: name = name or random_str() owners = owners or [] datasource_id = ( From cae15566de7f2f41783fcf7d4020d5d6ea56bb10 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Fri, 29 Jan 2021 23:23:29 +0200 Subject: [PATCH 42/49] feat: DASHBOARD_RBAC ff support --- superset/config.py | 1 + superset/dashboards/filters.py | 6 ++++-- tests/dashboards/security/security_rbac_tests.py | 5 +++++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/superset/config.py b/superset/config.py index 2febf07acd254..db54fa87cf012 100644 --- a/superset/config.py +++ b/superset/config.py @@ -343,6 +343,7 @@ def _try_json_readsha( # pylint: disable=unused-argument "ALERT_REPORTS": False, # Enable experimental feature to search for other dashboards "OMNIBAR": False, + "DASHBOARD_RBAC": False, } # Set the default view to card/grid view if thumbnail support is enabled. diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index c19be95568841..58525f393afe7 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -21,7 +21,7 @@ from sqlalchemy import and_, or_ from sqlalchemy.orm.query import Query -from superset import db, security_manager +from superset import db, security_manager, is_feature_enabled from superset.models.core import FavStar from superset.models.dashboard import Dashboard from superset.models.slice import Slice @@ -123,7 +123,9 @@ def apply(self, query: Query, value: Any) -> Query: Dashboard.id.in_(owner_ids_query), Dashboard.id.in_(datasource_perm_query), Dashboard.id.in_(users_favorite_dash_query), - Dashboard.id.in_(roles_based_query), + Dashboard.id.in_(roles_based_query) + if is_feature_enabled("DASHBOARD_RBAC") + else None, ) ) diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index e7924f2a928e3..947bccc16aa12 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -15,6 +15,7 @@ # specific language governing permissions and limitations # under the License. """Unit tests for Superset""" +from unittest import mock from tests.dashboards.dashboard_test_utils import * from tests.dashboards.security.base_case import BaseTestDashboardSecurity @@ -26,6 +27,10 @@ ) +@mock.patch.dict( + "superset.extensions.feature_flag_manager._feature_flags", + DASHBOARD_RBAC=True, +) class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity): def test_get_dashboards_list__admin_get_all_dashboards(self): # arrange From 2826b776348be204494469e6f1cdaf45bc1101ce Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Fri, 29 Jan 2021 23:29:43 +0200 Subject: [PATCH 43/49] fix: pre-commit --- superset/dashboards/filters.py | 2 +- tests/dashboards/security/security_rbac_tests.py | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index 58525f393afe7..cc0fac2dfad5a 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -21,7 +21,7 @@ from sqlalchemy import and_, or_ from sqlalchemy.orm.query import Query -from superset import db, security_manager, is_feature_enabled +from superset import db, is_feature_enabled, security_manager from superset.models.core import FavStar from superset.models.dashboard import Dashboard from superset.models.slice import Slice diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index 947bccc16aa12..efd2c990f314c 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -28,8 +28,7 @@ @mock.patch.dict( - "superset.extensions.feature_flag_manager._feature_flags", - DASHBOARD_RBAC=True, + "superset.extensions.feature_flag_manager._feature_flags", DASHBOARD_RBAC=True, ) class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity): def test_get_dashboards_list__admin_get_all_dashboards(self): From d337209cf7ea69c92e061250a8fe7c628a7f4195 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Sat, 30 Jan 2021 19:24:51 +0200 Subject: [PATCH 44/49] chore: is_feature_enabled("DASHBOARD_RBAC") on every usage --- superset/dashboards/filters.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index cc0fac2dfad5a..d7796b6bba7e3 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -75,14 +75,19 @@ def apply(self, query: Query, value: Any) -> Query: datasource_perms = security_manager.user_view_menu_names("datasource_access") schema_perms = security_manager.user_view_menu_names("schema_access") + + is_rbac_disabled_filter = [] dashboard_has_roles = Dashboard.roles.any() + if is_feature_enabled("DASHBOARD_RBAC"): + is_rbac_disabled_filter.append(~dashboard_has_roles) + datasource_perm_query = ( db.session.query(Dashboard.id) .join(Dashboard.slices) .filter( and_( Dashboard.published.is_(True), - ~dashboard_has_roles, + *is_rbac_disabled_filter, or_( Slice.perm.in_(datasource_perms), Slice.schema_perm.in_(schema_perms), @@ -91,6 +96,8 @@ def apply(self, query: Query, value: Any) -> Query: ) ) ) + + roles_based_query = ( db.session.query(Dashboard.id) .join(Dashboard.roles) @@ -118,14 +125,17 @@ def apply(self, query: Query, value: Any) -> Query: ) ) + dashboard_rbac_or_filters = [] + if is_feature_enabled("DASHBOARD_RBAC"): + dashboard_rbac_or_filters.append(Dashboard.id.in_(roles_based_query)) + query = query.filter( or_( Dashboard.id.in_(owner_ids_query), Dashboard.id.in_(datasource_perm_query), Dashboard.id.in_(users_favorite_dash_query), - Dashboard.id.in_(roles_based_query) - if is_feature_enabled("DASHBOARD_RBAC") - else None, + Dashboard.id.in_(roles_based_query), + *dashboard_rbac_or_filters, ) ) From 29a5bc54232ff579ee72596349380959e5c5f7b3 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Sat, 30 Jan 2021 19:37:20 +0200 Subject: [PATCH 45/49] fix: after CR --- superset/dashboards/filters.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index d7796b6bba7e3..42781deae1f30 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -97,7 +97,6 @@ def apply(self, query: Query, value: Any) -> Query: ) ) - roles_based_query = ( db.session.query(Dashboard.id) .join(Dashboard.roles) @@ -134,7 +133,6 @@ def apply(self, query: Query, value: Any) -> Query: Dashboard.id.in_(owner_ids_query), Dashboard.id.in_(datasource_perm_query), Dashboard.id.in_(users_favorite_dash_query), - Dashboard.id.in_(roles_based_query), *dashboard_rbac_or_filters, ) ) From ac45a58ea23f9f72898d2a669a10050e6191ffc8 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Sat, 30 Jan 2021 19:40:17 +0200 Subject: [PATCH 46/49] fix: roles_based_query inside the if is_feature_enabled("DASHBOARD_RBAC") --- superset/dashboards/filters.py | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index 42781deae1f30..1ca089e342ccf 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -97,18 +97,6 @@ def apply(self, query: Query, value: Any) -> Query: ) ) - roles_based_query = ( - db.session.query(Dashboard.id) - .join(Dashboard.roles) - .filter( - and_( - Dashboard.published.is_(True), - dashboard_has_roles, - Role.id.in_([x.id for x in get_user_roles()]), - ), - ) - ) - users_favorite_dash_query = db.session.query(FavStar.obj_id).filter( and_( FavStar.user_id == security_manager.user_model.get_user_id(), @@ -126,8 +114,21 @@ def apply(self, query: Query, value: Any) -> Query: dashboard_rbac_or_filters = [] if is_feature_enabled("DASHBOARD_RBAC"): + roles_based_query = ( + db.session.query(Dashboard.id) + .join(Dashboard.roles) + .filter( + and_( + Dashboard.published.is_(True), + dashboard_has_roles, + Role.id.in_([x.id for x in get_user_roles()]), + ), + ) + ) + dashboard_rbac_or_filters.append(Dashboard.id.in_(roles_based_query)) + query = query.filter( or_( Dashboard.id.in_(owner_ids_query), From 594e9f293a46d24a4c1cec47bf3231860aeb40d6 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Sat, 30 Jan 2021 20:29:12 +0200 Subject: [PATCH 47/49] fix: replace assert actions with the common assert pattern --- superset/dashboards/filters.py | 5 ++--- tests/dashboards/security/base_case.py | 8 ++++---- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index 1ca089e342ccf..7ca466e917186 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -116,8 +116,8 @@ def apply(self, query: Query, value: Any) -> Query: if is_feature_enabled("DASHBOARD_RBAC"): roles_based_query = ( db.session.query(Dashboard.id) - .join(Dashboard.roles) - .filter( + .join(Dashboard.roles) + .filter( and_( Dashboard.published.is_(True), dashboard_has_roles, @@ -128,7 +128,6 @@ def apply(self, query: Query, value: Any) -> Query: dashboard_rbac_or_filters.append(Dashboard.id.in_(roles_based_query)) - query = query.filter( or_( Dashboard.id.in_(owner_ids_query), diff --git a/tests/dashboards/security/base_case.py b/tests/dashboards/security/base_case.py index 6446e1add62e6..ab24734ce7d25 100644 --- a/tests/dashboards/security/base_case.py +++ b/tests/dashboards/security/base_case.py @@ -38,7 +38,7 @@ def assert_dashboard_api_response( self, response: Response, dashboard_to_access: Dashboard ) -> None: self.assert200(response) - self.assertEqual(response.json["id"], dashboard_to_access.id) + assert response.json["id"] == dashboard_to_access.id def assert_dashboards_list_view_response( self, @@ -74,13 +74,13 @@ def assert_dashboards_api_response( ) -> None: self.assert200(response) response_data = response.json - self.assertEqual(response_data["count"], expected_counts) + assert response_data["count"] == expected_counts response_dashboards_url = set( map(lambda dash: dash["url"], response_data["result"]) ) expected_dashboards = expected_dashboards or [] for dashboard in expected_dashboards: - self.assertIn(dashboard.url, response_dashboards_url) + assert dashboard.url in response_dashboards_url not_expected_dashboards = not_expected_dashboards or [] for dashboard in not_expected_dashboards: - self.assertNotIn(dashboard.url, response_dashboards_url) + assert dashboard.url not in response_dashboards_url From 80c26ae9a19e9b6b7e230fe09c253b67a1e64e95 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Sat, 30 Jan 2021 21:06:34 +0200 Subject: [PATCH 48/49] feat: FF the dashboard CRUD view with the DASHBOARD_RBAC --- superset/views/dashboard/mixin.py | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/superset/views/dashboard/mixin.py b/superset/views/dashboard/mixin.py index 273cfdf0b3c2f..1304b3bbd70d0 100644 --- a/superset/views/dashboard/mixin.py +++ b/superset/views/dashboard/mixin.py @@ -16,12 +16,27 @@ # under the License. from flask_babel import lazy_gettext as _ +from ... import is_feature_enabled from ...dashboards.filters import DashboardFilter from ..base import check_ownership +roles_edit = [] +roles_description = {} +roles_label = {} +if is_feature_enabled("DASHBOARD_RBAC"): + roles_edit = "roles" + roles_description = { + "roles": _( + "Roles is a list which defines access to the dashboard. " + "These roles are always applied in addition to restrictions on dataset " + "level access. " + "If no roles defined then the dashboard is available to all roles." + ) + } + roles_label = {"roles": _("Roles")} -class DashboardMixin: # pylint: disable=too-few-public-methods +class DashboardMixin: # pylint: disable=too-few-public-methods list_title = _("Dashboards") show_title = _("Show Dashboard") add_title = _("Add Dashboard") @@ -33,7 +48,7 @@ class DashboardMixin: # pylint: disable=too-few-public-methods "dashboard_title", "slug", "owners", - "roles", + roles_edit, "position_json", "css", "json_metadata", @@ -63,12 +78,7 @@ class DashboardMixin: # pylint: disable=too-few-public-methods "want to alter specific parameters." ), "owners": _("Owners is a list of users who can alter the dashboard."), - "roles": _( - "Roles is a list which defines access to the dashboard. " - "These roles are always applied in addition to restrictions on dataset " - "level access. " - "If no roles defined then the dashboard is available to all roles." - ), + **roles_description, "published": _( "Determines whether or not this dashboard is " "visible in the list of all dashboards" @@ -81,7 +91,7 @@ class DashboardMixin: # pylint: disable=too-few-public-methods "slug": _("Slug"), "charts": _("Charts"), "owners": _("Owners"), - "roles": _("Roles"), + **roles_label, "published": _("Published"), "creator": _("Creator"), "modified": _("Modified"), From 812bb87c53ae744ffefbfeb863b03f8ae756f503 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Sat, 30 Jan 2021 21:34:27 +0200 Subject: [PATCH 49/49] Revert "feat: FF the dashboard CRUD view with the DASHBOARD_RBAC" This reverts commit 80c26ae9 --- superset/views/dashboard/mixin.py | 28 +++++++++------------------- 1 file changed, 9 insertions(+), 19 deletions(-) diff --git a/superset/views/dashboard/mixin.py b/superset/views/dashboard/mixin.py index 1304b3bbd70d0..273cfdf0b3c2f 100644 --- a/superset/views/dashboard/mixin.py +++ b/superset/views/dashboard/mixin.py @@ -16,27 +16,12 @@ # under the License. from flask_babel import lazy_gettext as _ -from ... import is_feature_enabled from ...dashboards.filters import DashboardFilter from ..base import check_ownership -roles_edit = [] -roles_description = {} -roles_label = {} -if is_feature_enabled("DASHBOARD_RBAC"): - roles_edit = "roles" - roles_description = { - "roles": _( - "Roles is a list which defines access to the dashboard. " - "These roles are always applied in addition to restrictions on dataset " - "level access. " - "If no roles defined then the dashboard is available to all roles." - ) - } - roles_label = {"roles": _("Roles")} - class DashboardMixin: # pylint: disable=too-few-public-methods + list_title = _("Dashboards") show_title = _("Show Dashboard") add_title = _("Add Dashboard") @@ -48,7 +33,7 @@ class DashboardMixin: # pylint: disable=too-few-public-methods "dashboard_title", "slug", "owners", - roles_edit, + "roles", "position_json", "css", "json_metadata", @@ -78,7 +63,12 @@ class DashboardMixin: # pylint: disable=too-few-public-methods "want to alter specific parameters." ), "owners": _("Owners is a list of users who can alter the dashboard."), - **roles_description, + "roles": _( + "Roles is a list which defines access to the dashboard. " + "These roles are always applied in addition to restrictions on dataset " + "level access. " + "If no roles defined then the dashboard is available to all roles." + ), "published": _( "Determines whether or not this dashboard is " "visible in the list of all dashboards" @@ -91,7 +81,7 @@ class DashboardMixin: # pylint: disable=too-few-public-methods "slug": _("Slug"), "charts": _("Charts"), "owners": _("Owners"), - **roles_label, + "roles": _("Roles"), "published": _("Published"), "creator": _("Creator"), "modified": _("Modified"),