From ab6ae456daecd34e676273fa4d43c52875e00e94 Mon Sep 17 00:00:00 2001 From: altef Date: Wed, 27 Nov 2019 23:31:40 +0000 Subject: [PATCH 01/39] Support and apply filters. --- superset/connectors/sqla/models.py | 10 +++- ...36a828f_add_row_level_security_table_py.py | 56 +++++++++++++++++++ superset/models/core.py | 13 +++++ superset/security.py | 10 ++++ 4 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 superset/migrations/versions/415f336a828f_add_row_level_security_table_py.py diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 93b1f441864d9..ed49b75fe65a3 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -55,7 +55,7 @@ from superset.exceptions import DatabaseNotFound from superset.jinja_context import get_template_processor from superset.models.annotations import Annotation -from superset.models.core import Database +from superset.models.core import Database, RowLevelSecurityFilter from superset.models.helpers import QueryResult from superset.utils import core as utils, import_datasource @@ -363,6 +363,7 @@ class SqlaTable(Model, BaseDatasource): sql = Column(Text) is_sqllab_view = Column(Boolean, default=False) template_params = Column(Text) + row_level_security_filters = relationship(RowLevelSecurityFilter, backref="tables", lazy=True) baselink = "tablemodelview" @@ -977,6 +978,13 @@ def _get_top_groups( return or_(*groups) def query(self, query_obj: Dict) -> QueryResult: + filters = security_manager.get_row_level_security_filters(self) # Self is the Table model + if len(filters) > 0: + where = query_obj['extras']['where'] + if len(where) > 0: + where = "({}) AND ".format(where) + query_obj['extras']['where'] = "{} {}".format(where, " AND ".join(filters)) + qry_start_dttm = datetime.now() query_str_ext = self.get_query_str_extended(query_obj) sql = query_str_ext.sql diff --git a/superset/migrations/versions/415f336a828f_add_row_level_security_table_py.py b/superset/migrations/versions/415f336a828f_add_row_level_security_table_py.py new file mode 100644 index 0000000000000..ba058ce246b80 --- /dev/null +++ b/superset/migrations/versions/415f336a828f_add_row_level_security_table_py.py @@ -0,0 +1,56 @@ +# 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_row_level_security_table.py + +Revision ID: 415f336a828f +Revises: db4b49eb0782 +Create Date: 2019-11-27 21:09:00.650139 + +""" + +# revision identifiers, used by Alembic. +revision = '415f336a828f' +down_revision = 'db4b49eb0782' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_table('row_level_security_filters', + sa.Column('created_on', sa.DateTime(), nullable=True), + sa.Column('changed_on', sa.DateTime(), nullable=True), + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('role_id', sa.Integer(), nullable=False), + sa.Column('table_id', sa.Integer(), nullable=False), + sa.Column('clause', sa.Text(), nullable=False), + sa.Column('created_by_fk', sa.Integer(), nullable=True), + sa.Column('changed_by_fk', sa.Integer(), nullable=True), + sa.ForeignKeyConstraint(['changed_by_fk'], ['ab_user.id'], ), + sa.ForeignKeyConstraint(['created_by_fk'], ['ab_user.id'], ), + sa.ForeignKeyConstraint(['role_id'], ['ab_role.id'], ), + sa.ForeignKeyConstraint(['table_id'], ['tables.id'], ), + sa.PrimaryKeyConstraint('id') + ) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_table('row_level_security_filters') + # ### end Alembic commands ### diff --git a/superset/models/core.py b/superset/models/core.py index d378248edc0f6..ab080f3aaa751 100755 --- a/superset/models/core.py +++ b/superset/models/core.py @@ -1353,6 +1353,19 @@ def user_roles(self) -> str: return "" +class RowLevelSecurityFilter(Model, AuditMixinNullable): + """ + Custom where clauses attached to Tables and Roles. + """ + + __tablename__ = "row_level_security_filters" + id = Column(Integer, primary_key=True) # pylint: disable=invalid-name + role_id = Column(Integer, ForeignKey("ab_role.id"), nullable=False) + table_id = Column(Integer, ForeignKey("tables.id"), nullable=False) + clause = Column(Text, nullable=False) + # Also look up the backref stuff to go into the table model?? + + # events for updating tags if is_feature_enabled("TAGGING_SYSTEM"): sqla.event.listen(Slice, "after_insert", ChartUpdater.after_insert) diff --git a/superset/security.py b/superset/security.py index d8dd5edcd5a50..2c05c40970520 100644 --- a/superset/security.py +++ b/superset/security.py @@ -813,3 +813,13 @@ def assert_viz_permission(self, viz: "BaseViz") -> None: """ self.assert_datasource_permission(viz.datasource) + + def get_row_level_security_filters(self, table): + """ + Retrieves the appropriate row level security filters for the current user and the passed table. + + :param table: The table to check against + :returns: A list of clause strings. + """ + roles = [role.id for role in g.user.roles] + return [filter.clause for filter in filter(lambda x: x.role_id in roles, table.row_level_security_filters)] From 024912e7d951831c62d2ee4aeae5703adb0a56a9 Mon Sep 17 00:00:00 2001 From: altef Date: Fri, 29 Nov 2019 21:00:33 +0000 Subject: [PATCH 02/39] Added the UI for row level security, and moved it all under SQLA in order to access the Table model more easily. --- superset/connectors/sqla/models.py | 23 +++++++++++-- superset/connectors/sqla/views.py | 53 +++++++++++++++++++++++++++++- superset/models/core.py | 13 -------- superset/views/core.py | 2 ++ 4 files changed, 74 insertions(+), 17 deletions(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index ed49b75fe65a3..fe8d1f87a01e6 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -55,8 +55,8 @@ from superset.exceptions import DatabaseNotFound from superset.jinja_context import get_template_processor from superset.models.annotations import Annotation -from superset.models.core import Database, RowLevelSecurityFilter -from superset.models.helpers import QueryResult +from superset.models.core import Database +from superset.models.helpers import QueryResult, AuditMixinNullable from superset.utils import core as utils, import_datasource config = app.config @@ -363,7 +363,6 @@ class SqlaTable(Model, BaseDatasource): sql = Column(Text) is_sqllab_view = Column(Boolean, default=False) template_params = Column(Text) - row_level_security_filters = relationship(RowLevelSecurityFilter, backref="tables", lazy=True) baselink = "tablemodelview" @@ -1177,3 +1176,21 @@ def get_extra_cache_keys(self, query_obj: Dict) -> List[Any]: sa.event.listen(SqlaTable, "after_insert", security_manager.set_perm) sa.event.listen(SqlaTable, "after_update", security_manager.set_perm) + + + +class RowLevelSecurityFilter(Model, AuditMixinNullable): + """ + Custom where clauses attached to Tables and Roles. + """ + + __tablename__ = "row_level_security_filters" + id = Column(Integer, primary_key=True) # pylint: disable=invalid-name + role_id = Column(Integer, ForeignKey("ab_role.id"), nullable=False) + role = relationship( + security_manager.role_model, backref="row_level_security_filters" + ) + + table_id = Column(Integer, ForeignKey("tables.id"), nullable=False) + table = relationship(SqlaTable, backref="row_level_security_filters") + clause = Column(Text, nullable=False) diff --git a/superset/connectors/sqla/views.py b/superset/connectors/sqla/views.py index c508aa9a66f03..72ef035e5e4d7 100644 --- a/superset/connectors/sqla/views.py +++ b/superset/connectors/sqla/views.py @@ -227,6 +227,57 @@ class SqlMetricInlineView(CompactCRUDMixin, SupersetModelView): appbuilder.add_view_no_menu(SqlMetricInlineView) + +class RowLevelSecurityFiltersModelView(SupersetModelView, DeleteMixin): + datamodel = SQLAInterface(models.RowLevelSecurityFilter) + + list_title = _("Row level security filter") + show_title = _("Show Row level security filter") + add_title = _("Add Row level security filter") + edit_title = _("Edit Row level security filter") + + list_columns = ["table", "role", "clause", "creator", "modified"] + order_columns = ["modified"] + edit_columns = [ + "table", + "role", + "clause", + ] + show_columns = edit_columns + search_columns = ("table", "role", "clause") + add_columns = edit_columns + base_order = ("changed_on", "desc") + description_columns = { + "table": _( + "This is the table this filter will be applied to." + ), + "role": _( + "This is the role this filter will be applied to." + ), + "clause": _( + "This is the condition that will be added to the WHERE clause. " + "For example, to only return rows for a particular client, you might put in: client_id = 9" + ) + } + label_columns = { + "table": _("Table"), + "role": _("Role"), + "clause": _("Clause"), + "creator": _("Creator"), + "modified": _("Modified") + } + + +appbuilder.add_view( + RowLevelSecurityFiltersModelView, + "Row Level Security Filters", + label=__("Row level security filters"), + category="Security", + category_label=__("Security"), + icon="fa-lock", +) + + class TableModelView(DatasourceModelView, DeleteMixin, YamlExportMixin): datamodel = SQLAInterface(models.SqlaTable) @@ -256,7 +307,7 @@ class TableModelView(DatasourceModelView, DeleteMixin, YamlExportMixin): ] base_filters = [["id", DatasourceFilter, lambda: []]] show_columns = edit_columns + ["perm", "slices"] - related_views = [TableColumnInlineView, SqlMetricInlineView] + related_views = [TableColumnInlineView, SqlMetricInlineView, RowLevelSecurityFiltersModelView] base_order = ("changed_on", "desc") search_columns = ("database", "schema", "table_name", "owners", "is_sqllab_view") description_columns = { diff --git a/superset/models/core.py b/superset/models/core.py index ab080f3aaa751..d378248edc0f6 100755 --- a/superset/models/core.py +++ b/superset/models/core.py @@ -1353,19 +1353,6 @@ def user_roles(self) -> str: return "" -class RowLevelSecurityFilter(Model, AuditMixinNullable): - """ - Custom where clauses attached to Tables and Roles. - """ - - __tablename__ = "row_level_security_filters" - id = Column(Integer, primary_key=True) # pylint: disable=invalid-name - role_id = Column(Integer, ForeignKey("ab_role.id"), nullable=False) - table_id = Column(Integer, ForeignKey("tables.id"), nullable=False) - clause = Column(Text, nullable=False) - # Also look up the backref stuff to go into the table model?? - - # events for updating tags if is_feature_enabled("TAGGING_SYSTEM"): sqla.event.listen(Slice, "after_insert", ChartUpdater.after_insert) diff --git a/superset/views/core.py b/superset/views/core.py index fd7a6f68f5a8f..9cdbb0803e3c7 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -3232,6 +3232,8 @@ class CssTemplateAsyncModelView(CssTemplateModelView): appbuilder.add_separator("Sources") + + @app.after_request def apply_http_headers(response: Response): """Applies the configuration's http headers to all responses""" From 4d6789f00ff0a2b1c2d511d3ea63bd2fa73310b8 Mon Sep 17 00:00:00 2001 From: altef Date: Sat, 30 Nov 2019 03:54:53 +0000 Subject: [PATCH 03/39] Added a row level security filter documentation entry. --- docs/security.rst | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/docs/security.rst b/docs/security.rst index 0e796e3c88c33..de2ba99c3774f 100644 --- a/docs/security.rst +++ b/docs/security.rst @@ -153,3 +153,28 @@ a set of data sources that power dashboards only made available to executives. When looking at its dashboard list, this user will only see the list of dashboards it has access to, based on the roles and permissions that were attributed. + + +Restricting access to a subset of a particular table +"""""""""""""""""""""""""""""""""""""""""""""""""""" + +Using ``Row level security filters`` (under the ``Security`` menu) you can create +filters that are assigned to a particular table, as well as a particular role. +Say people in your finance department should only have access to rows where +``department = "finance"``. You could create a ``Row level security filter`` +with that clause, and assign it to your ``Finance`` role, as well as the +applicable table. + +The ``clause`` field can contain arbitrary text which is then added to the generated +SQL statement's ``WHERE`` clause. So you could even do something like create a +filter for the last 30 days and apply it to a specific role, with a clause like +``date_field > DATE_SUB(NOW(), INTERVAL 30 DAY)``. It can also support multiple +conditions: ``client_id = 6 AND advertiser="foo"``, etc. + +You can throw whatever you want in there to define the subset of the table you want the role in question to have access to. + +All relevant ``Row level security filters`` will be ANDed together, so it's +possible to create a situation where two roles conflict in such a way as to +limit a table subset to empty. For example, the filters ``client_id=4`` and +and ``client_id=5``, applied to a role, will result in users of that role having +``client_id=4 AND client_id=5`` added to their query, which can never be true. \ No newline at end of file From 4152dc2bc4545841847e8e686e057d3dd28a8554 Mon Sep 17 00:00:00 2001 From: altef Date: Sat, 30 Nov 2019 04:24:12 +0000 Subject: [PATCH 04/39] Accidentally added two new lines to this file. --- superset/views/core.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/superset/views/core.py b/superset/views/core.py index 9cdbb0803e3c7..fd7a6f68f5a8f 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -3232,8 +3232,6 @@ class CssTemplateAsyncModelView(CssTemplateModelView): appbuilder.add_separator("Sources") - - @app.after_request def apply_http_headers(response: Response): """Applies the configuration's http headers to all responses""" From 13f1381df33356ea3513b7b4e46c97f02b45db7c Mon Sep 17 00:00:00 2001 From: altef Date: Sat, 30 Nov 2019 05:26:22 +0000 Subject: [PATCH 05/39] Blacked and iSorted, hopefully. Also, sometimes g.user may not be set. --- superset/connectors/sqla/models.py | 11 +++--- superset/connectors/sqla/views.py | 25 ++++++------- ...36a828f_add_row_level_security_table_py.py | 35 ++++++++++--------- superset/security.py | 9 +++-- 4 files changed, 41 insertions(+), 39 deletions(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index fe8d1f87a01e6..5d7f08cf373b7 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -977,13 +977,15 @@ def _get_top_groups( return or_(*groups) def query(self, query_obj: Dict) -> QueryResult: - filters = security_manager.get_row_level_security_filters(self) # Self is the Table model + filters = security_manager.get_row_level_security_filters( + self + ) # Self is the Table model if len(filters) > 0: - where = query_obj['extras']['where'] + where = query_obj["extras"]["where"] if len(where) > 0: where = "({}) AND ".format(where) - query_obj['extras']['where'] = "{} {}".format(where, " AND ".join(filters)) - + query_obj["extras"]["where"] = "{} {}".format(where, " AND ".join(filters)) + qry_start_dttm = datetime.now() query_str_ext = self.get_query_str_extended(query_obj) sql = query_str_ext.sql @@ -1178,7 +1180,6 @@ def get_extra_cache_keys(self, query_obj: Dict) -> List[Any]: sa.event.listen(SqlaTable, "after_update", security_manager.set_perm) - class RowLevelSecurityFilter(Model, AuditMixinNullable): """ Custom where clauses attached to Tables and Roles. diff --git a/superset/connectors/sqla/views.py b/superset/connectors/sqla/views.py index 72ef035e5e4d7..961d76c5fee56 100644 --- a/superset/connectors/sqla/views.py +++ b/superset/connectors/sqla/views.py @@ -227,7 +227,6 @@ class SqlMetricInlineView(CompactCRUDMixin, SupersetModelView): appbuilder.add_view_no_menu(SqlMetricInlineView) - class RowLevelSecurityFiltersModelView(SupersetModelView, DeleteMixin): datamodel = SQLAInterface(models.RowLevelSecurityFilter) @@ -238,33 +237,25 @@ class RowLevelSecurityFiltersModelView(SupersetModelView, DeleteMixin): list_columns = ["table", "role", "clause", "creator", "modified"] order_columns = ["modified"] - edit_columns = [ - "table", - "role", - "clause", - ] + edit_columns = ["table", "role", "clause"] show_columns = edit_columns search_columns = ("table", "role", "clause") add_columns = edit_columns base_order = ("changed_on", "desc") description_columns = { - "table": _( - "This is the table this filter will be applied to." - ), - "role": _( - "This is the role this filter will be applied to." - ), + "table": _("This is the table this filter will be applied to."), + "role": _("This is the role this filter will be applied to."), "clause": _( "This is the condition that will be added to the WHERE clause. " "For example, to only return rows for a particular client, you might put in: client_id = 9" - ) + ), } label_columns = { "table": _("Table"), "role": _("Role"), "clause": _("Clause"), "creator": _("Creator"), - "modified": _("Modified") + "modified": _("Modified"), } @@ -307,7 +298,11 @@ class TableModelView(DatasourceModelView, DeleteMixin, YamlExportMixin): ] base_filters = [["id", DatasourceFilter, lambda: []]] show_columns = edit_columns + ["perm", "slices"] - related_views = [TableColumnInlineView, SqlMetricInlineView, RowLevelSecurityFiltersModelView] + related_views = [ + TableColumnInlineView, + SqlMetricInlineView, + RowLevelSecurityFiltersModelView, + ] base_order = ("changed_on", "desc") search_columns = ("database", "schema", "table_name", "owners", "is_sqllab_view") description_columns = { diff --git a/superset/migrations/versions/415f336a828f_add_row_level_security_table_py.py b/superset/migrations/versions/415f336a828f_add_row_level_security_table_py.py index ba058ce246b80..6ec32d92e9f88 100644 --- a/superset/migrations/versions/415f336a828f_add_row_level_security_table_py.py +++ b/superset/migrations/versions/415f336a828f_add_row_level_security_table_py.py @@ -23,8 +23,8 @@ """ # revision identifiers, used by Alembic. -revision = '415f336a828f' -down_revision = 'db4b49eb0782' +revision = "415f336a828f" +down_revision = "db4b49eb0782" from alembic import op import sqlalchemy as sa @@ -32,25 +32,26 @@ def upgrade(): # ### commands auto generated by Alembic - please adjust! ### - op.create_table('row_level_security_filters', - sa.Column('created_on', sa.DateTime(), nullable=True), - sa.Column('changed_on', sa.DateTime(), nullable=True), - sa.Column('id', sa.Integer(), nullable=False), - sa.Column('role_id', sa.Integer(), nullable=False), - sa.Column('table_id', sa.Integer(), nullable=False), - sa.Column('clause', sa.Text(), nullable=False), - sa.Column('created_by_fk', sa.Integer(), nullable=True), - sa.Column('changed_by_fk', sa.Integer(), nullable=True), - sa.ForeignKeyConstraint(['changed_by_fk'], ['ab_user.id'], ), - sa.ForeignKeyConstraint(['created_by_fk'], ['ab_user.id'], ), - sa.ForeignKeyConstraint(['role_id'], ['ab_role.id'], ), - sa.ForeignKeyConstraint(['table_id'], ['tables.id'], ), - sa.PrimaryKeyConstraint('id') + op.create_table( + "row_level_security_filters", + sa.Column("created_on", sa.DateTime(), nullable=True), + sa.Column("changed_on", sa.DateTime(), nullable=True), + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("role_id", sa.Integer(), nullable=False), + sa.Column("table_id", sa.Integer(), nullable=False), + sa.Column("clause", sa.Text(), nullable=False), + sa.Column("created_by_fk", sa.Integer(), nullable=True), + sa.Column("changed_by_fk", sa.Integer(), nullable=True), + sa.ForeignKeyConstraint(["changed_by_fk"], ["ab_user.id"]), + sa.ForeignKeyConstraint(["created_by_fk"], ["ab_user.id"]), + sa.ForeignKeyConstraint(["role_id"], ["ab_role.id"]), + sa.ForeignKeyConstraint(["table_id"], ["tables.id"]), + sa.PrimaryKeyConstraint("id"), ) # ### end Alembic commands ### def downgrade(): # ### commands auto generated by Alembic - please adjust! ### - op.drop_table('row_level_security_filters') + op.drop_table("row_level_security_filters") # ### end Alembic commands ### diff --git a/superset/security.py b/superset/security.py index 2c05c40970520..2e8403d496062 100644 --- a/superset/security.py +++ b/superset/security.py @@ -821,5 +821,10 @@ def get_row_level_security_filters(self, table): :param table: The table to check against :returns: A list of clause strings. """ - roles = [role.id for role in g.user.roles] - return [filter.clause for filter in filter(lambda x: x.role_id in roles, table.row_level_security_filters)] + roles = [role.id for role in (g.user.roles if g.user else [])] + return [ + filter.clause + for filter in filter( + lambda x: x.role_id in roles, table.row_level_security_filters + ) + ] From 1100a601f5a7f5b68c486cfa2ba008650744f401 Mon Sep 17 00:00:00 2001 From: altef Date: Sat, 30 Nov 2019 05:38:25 +0000 Subject: [PATCH 06/39] Another isort, and handling g not having a user attribute another way. --- .../versions/415f336a828f_add_row_level_security_table_py.py | 5 ++--- superset/security.py | 5 ++++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/superset/migrations/versions/415f336a828f_add_row_level_security_table_py.py b/superset/migrations/versions/415f336a828f_add_row_level_security_table_py.py index 6ec32d92e9f88..6ebdc7b5ce974 100644 --- a/superset/migrations/versions/415f336a828f_add_row_level_security_table_py.py +++ b/superset/migrations/versions/415f336a828f_add_row_level_security_table_py.py @@ -21,14 +21,13 @@ Create Date: 2019-11-27 21:09:00.650139 """ +from alembic import op +import sqlalchemy as sa # revision identifiers, used by Alembic. revision = "415f336a828f" down_revision = "db4b49eb0782" -from alembic import op -import sqlalchemy as sa - def upgrade(): # ### commands auto generated by Alembic - please adjust! ### diff --git a/superset/security.py b/superset/security.py index 2e8403d496062..00354f0a1bfd8 100644 --- a/superset/security.py +++ b/superset/security.py @@ -821,7 +821,10 @@ def get_row_level_security_filters(self, table): :param table: The table to check against :returns: A list of clause strings. """ - roles = [role.id for role in (g.user.roles if g.user else [])] + try: + roles = [role.id for role in g.user.roles] + except AttributeError: + roles = [] return [ filter.clause for filter in filter( From 76a314a171e7fb5f2ba0699ace0731aca4652749 Mon Sep 17 00:00:00 2001 From: altef Date: Sat, 30 Nov 2019 05:48:56 +0000 Subject: [PATCH 07/39] Let's try this again #CI tests. --- ...415f336a828f_add_row_level_security_table_py.py | 2 +- superset/security.py | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/superset/migrations/versions/415f336a828f_add_row_level_security_table_py.py b/superset/migrations/versions/415f336a828f_add_row_level_security_table_py.py index 6ebdc7b5ce974..29b8895720f41 100644 --- a/superset/migrations/versions/415f336a828f_add_row_level_security_table_py.py +++ b/superset/migrations/versions/415f336a828f_add_row_level_security_table_py.py @@ -21,8 +21,8 @@ Create Date: 2019-11-27 21:09:00.650139 """ -from alembic import op import sqlalchemy as sa +from alembic import op # revision identifiers, used by Alembic. revision = "415f336a828f" diff --git a/superset/security.py b/superset/security.py index 00354f0a1bfd8..3e783a280319f 100644 --- a/superset/security.py +++ b/superset/security.py @@ -823,11 +823,11 @@ def get_row_level_security_filters(self, table): """ try: roles = [role.id for role in g.user.roles] + return [ + filter.clause + for filter in filter( + lambda x: x.role_id in roles, table.row_level_security_filters + ) + ] except AttributeError: - roles = [] - return [ - filter.clause - for filter in filter( - lambda x: x.role_id in roles, table.row_level_security_filters - ) - ] + return [] From 86234fa36009ba577882f24e63576d06b98d4ce8 Mon Sep 17 00:00:00 2001 From: altef Date: Sat, 30 Nov 2019 05:55:50 +0000 Subject: [PATCH 08/39] Adjusted import order for isort; I was sure I'd already done this.. --- superset/connectors/sqla/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 5d7f08cf373b7..c2ff2c84ad4e3 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -56,7 +56,7 @@ from superset.jinja_context import get_template_processor from superset.models.annotations import Annotation from superset.models.core import Database -from superset.models.helpers import QueryResult, AuditMixinNullable +from superset.models.helpers import AuditMixinNullable, QueryResult from superset.utils import core as utils, import_datasource config = app.config From 81c2bdbc603720f1fa0a64e54985f42e191a9798 Mon Sep 17 00:00:00 2001 From: altef Date: Sat, 30 Nov 2019 17:16:37 +0000 Subject: [PATCH 09/39] Row level filters should be wrapped in parentheses in case one contains an OR. --- superset/connectors/sqla/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index c2ff2c84ad4e3..300e8d4938960 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -984,7 +984,7 @@ def query(self, query_obj: Dict) -> QueryResult: where = query_obj["extras"]["where"] if len(where) > 0: where = "({}) AND ".format(where) - query_obj["extras"]["where"] = "{} {}".format(where, " AND ".join(filters)) + query_obj["extras"]["where"] = "{} ({})".format(where, ") AND (".join(filters)) qry_start_dttm = datetime.now() query_str_ext = self.get_query_str_extended(query_obj) From ff80addb5e22e5a6472f5bdc27cbb1b878168da6 Mon Sep 17 00:00:00 2001 From: altef Date: Sat, 30 Nov 2019 17:55:02 +0000 Subject: [PATCH 10/39] Oops, did not think that would change Black's formatting. --- superset/connectors/sqla/models.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 300e8d4938960..528ed681f9dce 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -984,7 +984,9 @@ def query(self, query_obj: Dict) -> QueryResult: where = query_obj["extras"]["where"] if len(where) > 0: where = "({}) AND ".format(where) - query_obj["extras"]["where"] = "{} ({})".format(where, ") AND (".join(filters)) + query_obj["extras"]["where"] = "{} ({})".format( + where, ") AND (".join(filters) + ) qry_start_dttm = datetime.now() query_str_ext = self.get_query_str_extended(query_obj) From aafce2fa3f5b4730f401e4765d850a5f21f278c9 Mon Sep 17 00:00:00 2001 From: altef Date: Wed, 4 Dec 2019 16:52:57 +0000 Subject: [PATCH 11/39] Changes as per @mistercrunch. --- superset/connectors/sqla/models.py | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 528ed681f9dce..07ed91880d69e 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -656,6 +656,15 @@ def adhoc_metric_to_sqla(self, metric: Dict, cols: Dict) -> Optional[Column]: return self.make_sqla_column_compatible(sqla_metric, label) + def _get_row_level_filters(self) -> List[str]: + """ + Return the appropriate row level security filters for this table and the current user. + + :returns: A list of SQL clauses to be ANDed together. + :rtype: List[str] + """ + return security_manager.get_row_level_security_filters(self) + def get_sqla_query( # sqla self, groupby, @@ -834,6 +843,10 @@ def get_sqla_query( # sqla where_clause_and.append(col_obj.get_sqla_col() == None) elif op == "IS NOT NULL": where_clause_and.append(col_obj.get_sqla_col() != None) + + where_clause_and += [ + text("({})".format(f)) for f in self._get_row_level_filters() + ] if extras: where = extras.get("where") if where: @@ -943,7 +956,6 @@ def get_sqla_query( # sqla result.df, dimensions, groupby_exprs_sans_timestamp ) qry = qry.where(top_groups) - return SqlaQuery( extra_cache_keys=extra_cache_keys, labels_expected=labels_expected, @@ -977,17 +989,6 @@ def _get_top_groups( return or_(*groups) def query(self, query_obj: Dict) -> QueryResult: - filters = security_manager.get_row_level_security_filters( - self - ) # Self is the Table model - if len(filters) > 0: - where = query_obj["extras"]["where"] - if len(where) > 0: - where = "({}) AND ".format(where) - query_obj["extras"]["where"] = "{} ({})".format( - where, ") AND (".join(filters) - ) - qry_start_dttm = datetime.now() query_str_ext = self.get_query_str_extended(query_obj) sql = query_str_ext.sql From fd0eaf61b834031375d678e5f22216c50e245d07 Mon Sep 17 00:00:00 2001 From: altef Date: Wed, 4 Dec 2019 17:55:15 +0000 Subject: [PATCH 12/39] RLS filters are now many-to-many with Roles. --- superset/connectors/sqla/models.py | 16 +++++++++--- superset/connectors/sqla/views.py | 10 ++++---- ...> 0a6f12f60c73_add_role_level_security.py} | 25 +++++++++++++------ superset/security.py | 7 +++--- 4 files changed, 38 insertions(+), 20 deletions(-) rename superset/migrations/versions/{415f336a828f_add_row_level_security_table_py.py => 0a6f12f60c73_add_role_level_security.py} (81%) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 07ed91880d69e..e6ec6b1628cc1 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -1183,6 +1183,15 @@ def get_extra_cache_keys(self, query_obj: Dict) -> List[Any]: sa.event.listen(SqlaTable, "after_update", security_manager.set_perm) +RLSFilterRoles = Table( + "rls_filter_roles", + Model.metadata, + Column("id", Integer, primary_key=True), + Column("role_id", Integer, ForeignKey("ab_role.id"), nullable=False), + Column("rlsfilter_id", Integer, ForeignKey("row_level_security_filters.id")), +) + + class RowLevelSecurityFilter(Model, AuditMixinNullable): """ Custom where clauses attached to Tables and Roles. @@ -1190,9 +1199,10 @@ class RowLevelSecurityFilter(Model, AuditMixinNullable): __tablename__ = "row_level_security_filters" id = Column(Integer, primary_key=True) # pylint: disable=invalid-name - role_id = Column(Integer, ForeignKey("ab_role.id"), nullable=False) - role = relationship( - security_manager.role_model, backref="row_level_security_filters" + roles = relationship( + security_manager.role_model, + secondary=RLSFilterRoles, + backref="row_level_security_filters", ) table_id = Column(Integer, ForeignKey("tables.id"), nullable=False) diff --git a/superset/connectors/sqla/views.py b/superset/connectors/sqla/views.py index 961d76c5fee56..a6a439947e8f9 100644 --- a/superset/connectors/sqla/views.py +++ b/superset/connectors/sqla/views.py @@ -235,16 +235,16 @@ class RowLevelSecurityFiltersModelView(SupersetModelView, DeleteMixin): add_title = _("Add Row level security filter") edit_title = _("Edit Row level security filter") - list_columns = ["table", "role", "clause", "creator", "modified"] + list_columns = ["table", "roles", "clause", "creator", "modified"] order_columns = ["modified"] - edit_columns = ["table", "role", "clause"] + edit_columns = ["table", "roles", "clause"] show_columns = edit_columns - search_columns = ("table", "role", "clause") + search_columns = ("table", "roles", "clause") add_columns = edit_columns base_order = ("changed_on", "desc") description_columns = { "table": _("This is the table this filter will be applied to."), - "role": _("This is the role this filter will be applied to."), + "roles": _("These are the roles this filter will be applied to."), "clause": _( "This is the condition that will be added to the WHERE clause. " "For example, to only return rows for a particular client, you might put in: client_id = 9" @@ -252,7 +252,7 @@ class RowLevelSecurityFiltersModelView(SupersetModelView, DeleteMixin): } label_columns = { "table": _("Table"), - "role": _("Role"), + "roles": _("Roles"), "clause": _("Clause"), "creator": _("Creator"), "modified": _("Modified"), diff --git a/superset/migrations/versions/415f336a828f_add_row_level_security_table_py.py b/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py similarity index 81% rename from superset/migrations/versions/415f336a828f_add_row_level_security_table_py.py rename to superset/migrations/versions/0a6f12f60c73_add_role_level_security.py index 29b8895720f41..d75d9437559ec 100644 --- a/superset/migrations/versions/415f336a828f_add_row_level_security_table_py.py +++ b/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py @@ -14,20 +14,21 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -"""add_row_level_security_table.py +"""add_role_level_security -Revision ID: 415f336a828f +Revision ID: 0a6f12f60c73 Revises: db4b49eb0782 -Create Date: 2019-11-27 21:09:00.650139 +Create Date: 2019-12-04 17:07:54.390805 """ -import sqlalchemy as sa -from alembic import op # revision identifiers, used by Alembic. -revision = "415f336a828f" +revision = "0a6f12f60c73" down_revision = "db4b49eb0782" +import sqlalchemy as sa +from alembic import op + def upgrade(): # ### commands auto generated by Alembic - please adjust! ### @@ -36,21 +37,29 @@ def upgrade(): sa.Column("created_on", sa.DateTime(), nullable=True), sa.Column("changed_on", sa.DateTime(), nullable=True), sa.Column("id", sa.Integer(), nullable=False), - sa.Column("role_id", sa.Integer(), nullable=False), sa.Column("table_id", sa.Integer(), nullable=False), sa.Column("clause", sa.Text(), nullable=False), sa.Column("created_by_fk", sa.Integer(), nullable=True), sa.Column("changed_by_fk", sa.Integer(), nullable=True), sa.ForeignKeyConstraint(["changed_by_fk"], ["ab_user.id"]), sa.ForeignKeyConstraint(["created_by_fk"], ["ab_user.id"]), - sa.ForeignKeyConstraint(["role_id"], ["ab_role.id"]), sa.ForeignKeyConstraint(["table_id"], ["tables.id"]), sa.PrimaryKeyConstraint("id"), ) + op.create_table( + "rls_filter_roles", + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("role_id", sa.Integer(), nullable=False), + sa.Column("rlsfilter_id", sa.Integer(), nullable=True), + sa.ForeignKeyConstraint(["rlsfilter_id"], ["row_level_security_filters.id"]), + sa.ForeignKeyConstraint(["role_id"], ["ab_role.id"]), + sa.PrimaryKeyConstraint("id"), + ) # ### end Alembic commands ### def downgrade(): # ### commands auto generated by Alembic - please adjust! ### + op.drop_table("rls_filter_roles") op.drop_table("row_level_security_filters") # ### end Alembic commands ### diff --git a/superset/security.py b/superset/security.py index 3e783a280319f..25b3b6431f0e7 100644 --- a/superset/security.py +++ b/superset/security.py @@ -824,10 +824,9 @@ def get_row_level_security_filters(self, table): try: roles = [role.id for role in g.user.roles] return [ - filter.clause - for filter in filter( - lambda x: x.role_id in roles, table.row_level_security_filters - ) + f.clause + for f in table.row_level_security_filters + if any(r.id in roles for r in f.roles) ] except AttributeError: return [] From 8b6ce7219d423a27031fea728e713a9e7659ef7b Mon Sep 17 00:00:00 2001 From: altef Date: Wed, 4 Dec 2019 18:04:16 +0000 Subject: [PATCH 13/39] Updated documentation to reflect RLS filters supporting multiple rows. --- docs/security.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/security.rst b/docs/security.rst index de2ba99c3774f..4bce99702ef64 100644 --- a/docs/security.rst +++ b/docs/security.rst @@ -159,7 +159,7 @@ Restricting access to a subset of a particular table """""""""""""""""""""""""""""""""""""""""""""""""""" Using ``Row level security filters`` (under the ``Security`` menu) you can create -filters that are assigned to a particular table, as well as a particular role. +filters that are assigned to a particular table, as well as a set of roles. Say people in your finance department should only have access to rows where ``department = "finance"``. You could create a ``Row level security filter`` with that clause, and assign it to your ``Finance`` role, as well as the @@ -171,7 +171,7 @@ filter for the last 30 days and apply it to a specific role, with a clause like ``date_field > DATE_SUB(NOW(), INTERVAL 30 DAY)``. It can also support multiple conditions: ``client_id = 6 AND advertiser="foo"``, etc. -You can throw whatever you want in there to define the subset of the table you want the role in question to have access to. +You can throw whatever you want in there to define the subset of the table you want the roles in question to have access to. All relevant ``Row level security filters`` will be ANDed together, so it's possible to create a situation where two roles conflict in such a way as to From 5f6938639d6085aff61185c96e8feb218b0f418d Mon Sep 17 00:00:00 2001 From: altef Date: Wed, 4 Dec 2019 18:12:17 +0000 Subject: [PATCH 14/39] Let's see what happens when I set it to the previous revision ID --- .../versions/0a6f12f60c73_add_role_level_security.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py b/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py index d75d9437559ec..f2e390d7ae84b 100644 --- a/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py +++ b/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py @@ -16,14 +16,14 @@ # under the License. """add_role_level_security -Revision ID: 0a6f12f60c73 +Revision ID: 415f336a828f Revises: db4b49eb0782 Create Date: 2019-12-04 17:07:54.390805 """ # revision identifiers, used by Alembic. -revision = "0a6f12f60c73" +revision = "415f336a828f" down_revision = "db4b49eb0782" import sqlalchemy as sa From 2c252d98d318a3e80842b8e423122c5011619a8a Mon Sep 17 00:00:00 2001 From: altef Date: Wed, 4 Dec 2019 18:25:15 +0000 Subject: [PATCH 15/39] Updated from upstream. --- .../versions/0a6f12f60c73_add_role_level_security.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py b/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py index f2e390d7ae84b..add47d57ee28b 100644 --- a/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py +++ b/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py @@ -16,15 +16,15 @@ # under the License. """add_role_level_security -Revision ID: 415f336a828f -Revises: db4b49eb0782 +Revision ID: 0a6f12f60c73 +Revises: 5afa9079866a Create Date: 2019-12-04 17:07:54.390805 """ # revision identifiers, used by Alembic. -revision = "415f336a828f" -down_revision = "db4b49eb0782" +revision = "0a6f12f60c73" +down_revision = "5afa9079866a" import sqlalchemy as sa from alembic import op From 7223da674a733b341ec824a189077f8fab9c2c51 Mon Sep 17 00:00:00 2001 From: altef Date: Wed, 4 Dec 2019 18:51:37 +0000 Subject: [PATCH 16/39] There was a pylint error. --- superset/connectors/sqla/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 64db04fcc61a2..8f5858946a8b1 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -1185,7 +1185,7 @@ def get_extra_cache_keys(self, query_obj: Dict) -> List[Any]: RLSFilterRoles = Table( "rls_filter_roles", - Model.metadata, + metadata, Column("id", Integer, primary_key=True), Column("role_id", Integer, ForeignKey("ab_role.id"), nullable=False), Column("rlsfilter_id", Integer, ForeignKey("row_level_security_filters.id")), From 84a50099ea09603669d868c0a0ee1f4495713cca Mon Sep 17 00:00:00 2001 From: altef Date: Mon, 9 Dec 2019 07:09:31 +0000 Subject: [PATCH 17/39] Added RLS ids to the cache keys; modified documentation; added template processing to RLS filters. --- docs/security.rst | 2 -- superset/common/query_object.py | 3 ++- superset/connectors/sqla/models.py | 12 +++++++----- superset/security.py | 22 +++++++++++++++++++++- superset/viz.py | 3 ++- 5 files changed, 32 insertions(+), 10 deletions(-) diff --git a/docs/security.rst b/docs/security.rst index 4bce99702ef64..8d409a58027a8 100644 --- a/docs/security.rst +++ b/docs/security.rst @@ -171,8 +171,6 @@ filter for the last 30 days and apply it to a specific role, with a clause like ``date_field > DATE_SUB(NOW(), INTERVAL 30 DAY)``. It can also support multiple conditions: ``client_id = 6 AND advertiser="foo"``, etc. -You can throw whatever you want in there to define the subset of the table you want the roles in question to have access to. - All relevant ``Row level security filters`` will be ANDed together, so it's possible to create a situation where two roles conflict in such a way as to limit a table subset to empty. For example, the filters ``client_id=4`` and diff --git a/superset/common/query_object.py b/superset/common/query_object.py index 21649d1d0a32c..ebc9ddb5d14e0 100644 --- a/superset/common/query_object.py +++ b/superset/common/query_object.py @@ -21,7 +21,7 @@ import simplejson as json -from superset import app +from superset import app, security_manager from superset.utils import core as utils # TODO: Type Metrics dictionary with TypedDict when it becomes a vanilla python type @@ -130,6 +130,7 @@ def cache_key(self, **extra) -> str: del cache_dict[k] if self.time_range: cache_dict["time_range"] = self.time_range + cache_dict["rls"] = security_manager.get_rls_ids(self.datasource) json_data = self.json_dumps(cache_dict, sort_keys=True) return hashlib.md5(json_data.encode("utf-8")).hexdigest() diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 8f5858946a8b1..29c87b93345df 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -656,14 +656,18 @@ def adhoc_metric_to_sqla(self, metric: Dict, cols: Dict) -> Optional[Column]: return self.make_sqla_column_compatible(sqla_metric, label) - def _get_row_level_filters(self) -> List[str]: + def _get_sqla_row_level_filters(self, template_processor) -> List[str]: """ Return the appropriate row level security filters for this table and the current user. + :param BaseTemplateProcessor template_processor: The template processor to apply to the filters. :returns: A list of SQL clauses to be ANDed together. :rtype: List[str] """ - return security_manager.get_row_level_security_filters(self) + return [ + text("({})".format(template_processor.process_template(f))) + for f in security_manager.get_rls_filters(self) + ] def get_sqla_query( # sqla self, @@ -844,9 +848,7 @@ def get_sqla_query( # sqla elif op == "IS NOT NULL": where_clause_and.append(col_obj.get_sqla_col() != None) - where_clause_and += [ - text("({})".format(f)) for f in self._get_row_level_filters() - ] + where_clause_and += self._get_sqla_row_level_filters(template_processor) if extras: where = extras.get("where") if where: diff --git a/superset/security.py b/superset/security.py index 7ed8fd95a4d7f..4e3c3126c2252 100644 --- a/superset/security.py +++ b/superset/security.py @@ -106,6 +106,7 @@ class SupersetSecurityManager(SecurityManager): "ResetPasswordView", "RoleModelView", "Security", + "RowLevelSecurityFiltersModelView", } | USER_MODEL_VIEWS ALPHA_ONLY_VIEW_MENUS = {"Upload a CSV"} @@ -878,7 +879,7 @@ def assert_viz_permission(self, viz: "BaseViz") -> None: self.assert_datasource_permission(viz.datasource) - def get_row_level_security_filters(self, table): + def get_rls_filters(self, table): """ Retrieves the appropriate row level security filters for the current user and the passed table. @@ -894,3 +895,22 @@ def get_row_level_security_filters(self, table): ] except AttributeError: return [] + + def get_rls_ids(self, table) -> List[int]: + """ + Retrieves the appropriate row level security filters IDs for the current user and the passed table. + + :param table: The table to check against + :returns: A list of IDs. + """ + try: + roles = [role.id for role in g.user.roles] + ids = [ + f.id + for f in table.row_level_security_filters + if any(r.id in roles for r in f.roles) + ] + ids.sort() + return ids + except AttributeError: + return [] diff --git a/superset/viz.py b/superset/viz.py index b123fede9303b..53e5799671de5 100644 --- a/superset/viz.py +++ b/superset/viz.py @@ -46,7 +46,7 @@ from markdown import markdown from pandas.tseries.frequencies import to_offset -from superset import app, cache, get_css_manifest_files +from superset import app, cache, get_css_manifest_files, security_manager from superset.constants import NULL_STRING from superset.exceptions import NullValueException, SpatialException from superset.utils import core as utils @@ -367,6 +367,7 @@ def cache_key(self, query_obj, **extra): cache_dict["time_range"] = self.form_data.get("time_range") cache_dict["datasource"] = self.datasource.uid cache_dict["extra_cache_keys"] = self.datasource.get_extra_cache_keys(query_obj) + cache_dict["rls"] = security_manager.get_rls_ids(self.datasource) json_data = self.json_dumps(cache_dict, sort_keys=True) return hashlib.md5(json_data.encode("utf-8")).hexdigest() From 253e992dd593b0da5eba2a9d843f0fb2efd51315 Mon Sep 17 00:00:00 2001 From: altef Date: Mon, 9 Dec 2019 07:15:23 +0000 Subject: [PATCH 18/39] A new migration was merged in. --- .../versions/0a6f12f60c73_add_role_level_security.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py b/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py index add47d57ee28b..b276e967a2cf9 100644 --- a/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py +++ b/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py @@ -17,14 +17,14 @@ """add_role_level_security Revision ID: 0a6f12f60c73 -Revises: 5afa9079866a +Revises: 89115a40e8ea Create Date: 2019-12-04 17:07:54.390805 """ # revision identifiers, used by Alembic. revision = "0a6f12f60c73" -down_revision = "5afa9079866a" +down_revision = "89115a40e8ea" import sqlalchemy as sa from alembic import op From 1b3b8f6fd2b4c5f48e97eb2cc898f9326346e294 Mon Sep 17 00:00:00 2001 From: altef Date: Mon, 9 Dec 2019 07:42:36 +0000 Subject: [PATCH 19/39] Removed RLS cache key from query_object. --- superset/common/query_object.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/superset/common/query_object.py b/superset/common/query_object.py index ebc9ddb5d14e0..21649d1d0a32c 100644 --- a/superset/common/query_object.py +++ b/superset/common/query_object.py @@ -21,7 +21,7 @@ import simplejson as json -from superset import app, security_manager +from superset import app from superset.utils import core as utils # TODO: Type Metrics dictionary with TypedDict when it becomes a vanilla python type @@ -130,7 +130,6 @@ def cache_key(self, **extra) -> str: del cache_dict[k] if self.time_range: cache_dict["time_range"] = self.time_range - cache_dict["rls"] = security_manager.get_rls_ids(self.datasource) json_data = self.json_dumps(cache_dict, sort_keys=True) return hashlib.md5(json_data.encode("utf-8")).hexdigest() From c0ac8e9ba4297a25537d8edecaf766c5dcd14d72 Mon Sep 17 00:00:00 2001 From: altef Date: Tue, 10 Dec 2019 08:48:48 +0000 Subject: [PATCH 20/39] RLS added to the cache_key from query_context. --- superset/common/query_context.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/superset/common/query_context.py b/superset/common/query_context.py index 36ba6753676dc..62aec99783e0c 100644 --- a/superset/common/query_context.py +++ b/superset/common/query_context.py @@ -23,7 +23,7 @@ import numpy as np import pandas as pd -from superset import app, cache, db +from superset import app, cache, db, security_manager from superset.connectors.base.models import BaseDatasource from superset.connectors.connector_registry import ConnectorRegistry from superset.stats_logger import BaseStatsLogger @@ -164,6 +164,7 @@ def get_df_payload(self, query_obj: QueryObject, **kwargs) -> Dict[str, Any]: query_obj.cache_key( datasource=self.datasource.uid, extra_cache_keys=extra_cache_keys, + rls=security_manager.get_rls_ids(self.datasource), **kwargs ) if query_obj From 041038e2f8b173708535a9910dd2331fa7700889 Mon Sep 17 00:00:00 2001 From: altef Date: Tue, 17 Dec 2019 07:55:20 +0000 Subject: [PATCH 21/39] Changes as per @etr2460. --- superset/connectors/sqla/models.py | 2 +- .../versions/0a6f12f60c73_add_role_level_security.py | 8 ++------ superset/security/manager.py | 2 +- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 29c87b93345df..f92db1b8741e4 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -1190,7 +1190,7 @@ def get_extra_cache_keys(self, query_obj: Dict) -> List[Any]: metadata, Column("id", Integer, primary_key=True), Column("role_id", Integer, ForeignKey("ab_role.id"), nullable=False), - Column("rlsfilter_id", Integer, ForeignKey("row_level_security_filters.id")), + Column("rls_filter_id", Integer, ForeignKey("row_level_security_filters.id")), ) diff --git a/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py b/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py index b276e967a2cf9..e9a04006d4504 100644 --- a/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py +++ b/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py @@ -31,7 +31,6 @@ def upgrade(): - # ### commands auto generated by Alembic - please adjust! ### op.create_table( "row_level_security_filters", sa.Column("created_on", sa.DateTime(), nullable=True), @@ -50,16 +49,13 @@ def upgrade(): "rls_filter_roles", sa.Column("id", sa.Integer(), nullable=False), sa.Column("role_id", sa.Integer(), nullable=False), - sa.Column("rlsfilter_id", sa.Integer(), nullable=True), - sa.ForeignKeyConstraint(["rlsfilter_id"], ["row_level_security_filters.id"]), + sa.Column("rls_filter_id", sa.Integer(), nullable=True), + sa.ForeignKeyConstraint(["rls_filter_id"], ["row_level_security_filters.id"]), sa.ForeignKeyConstraint(["role_id"], ["ab_role.id"]), sa.PrimaryKeyConstraint("id"), ) - # ### end Alembic commands ### def downgrade(): - # ### commands auto generated by Alembic - please adjust! ### op.drop_table("rls_filter_roles") op.drop_table("row_level_security_filters") - # ### end Alembic commands ### diff --git a/superset/security/manager.py b/superset/security/manager.py index 4e3c3126c2252..f8a0581b3c701 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -910,7 +910,7 @@ def get_rls_ids(self, table) -> List[int]: for f in table.row_level_security_filters if any(r.id in roles for r in f.roles) ] - ids.sort() + ids.sort() # Combinations rather than permutations return ids except AttributeError: return [] From 8f0d0d1692c258fb5034740d3cb86b71daacbf82 Mon Sep 17 00:00:00 2001 From: altef Date: Tue, 17 Dec 2019 08:08:21 +0000 Subject: [PATCH 22/39] Updating entry for RLS pull request. --- UPDATING.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/UPDATING.md b/UPDATING.md index aebe63a3e7117..542b302558e07 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -23,6 +23,8 @@ assists people when migrating to a new version. ## Next +* [8699](https://github.com/apache/incubator-superset/pull/8699): A `row_level_security_filters` table has been added, which is many-to-many with `tables` and `ab_roles`. The applicable filters are added to the sqla query, and the RLS ids are added to the query cache keys. They can be accessed through the `Security` menu, or when editting a table. + * [8732](https://github.com/apache/incubator-superset/pull/8732): Swagger user interface is now enabled by default. A new permission `show on SwaggerView` is created by `superset init` and given to the `Admin` Role. To disable the UI, set `FAB_API_SWAGGER_UI = False` on config. From e403e9b1112db04e646720ece7f3d596b2d2e2f7 Mon Sep 17 00:00:00 2001 From: altef Date: Tue, 17 Dec 2019 08:15:09 +0000 Subject: [PATCH 23/39] Another migration to skip. --- .../versions/0a6f12f60c73_add_role_level_security.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py b/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py index e9a04006d4504..4fc588bd2cf42 100644 --- a/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py +++ b/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py @@ -17,14 +17,14 @@ """add_role_level_security Revision ID: 0a6f12f60c73 -Revises: 89115a40e8ea +Revises: 817e1c9b09d0 Create Date: 2019-12-04 17:07:54.390805 """ # revision identifiers, used by Alembic. revision = "0a6f12f60c73" -down_revision = "89115a40e8ea" +down_revision = "817e1c9b09d0" import sqlalchemy as sa from alembic import op From 5a0257c7000e1b8849860b4ac12347681c8ebecd Mon Sep 17 00:00:00 2001 From: altef Date: Fri, 10 Jan 2020 22:46:37 +0000 Subject: [PATCH 24/39] Changes as per @serenajiang. --- superset/connectors/sqla/models.py | 2 +- superset/security/manager.py | 27 +++++++++------------------ 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 2423be5cc290a..bf58ecc317fff 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -654,7 +654,7 @@ def _get_sqla_row_level_filters(self, template_processor) -> List[str]: :rtype: List[str] """ return [ - text("({})".format(template_processor.process_template(f))) + text("({})".format(template_processor.process_template(f.clause))) for f in security_manager.get_rls_filters(self) ] diff --git a/superset/security/manager.py b/superset/security/manager.py index f8a0581b3c701..83820ab367b4a 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -879,38 +879,29 @@ def assert_viz_permission(self, viz: "BaseViz") -> None: self.assert_datasource_permission(viz.datasource) - def get_rls_filters(self, table): + def get_rls_filters(self, table: "BaseDatasource"): """ Retrieves the appropriate row level security filters for the current user and the passed table. :param table: The table to check against - :returns: A list of clause strings. + :returns: A list of filters strings. """ - try: + if hasattr(g, 'user'): roles = [role.id for role in g.user.roles] return [ - f.clause + f for f in table.row_level_security_filters if any(r.id in roles for r in f.roles) ] - except AttributeError: - return [] + return [] - def get_rls_ids(self, table) -> List[int]: + def get_rls_ids(self, table: "BaseDatasource") -> List[int]: """ Retrieves the appropriate row level security filters IDs for the current user and the passed table. :param table: The table to check against :returns: A list of IDs. """ - try: - roles = [role.id for role in g.user.roles] - ids = [ - f.id - for f in table.row_level_security_filters - if any(r.id in roles for r in f.roles) - ] - ids.sort() # Combinations rather than permutations - return ids - except AttributeError: - return [] + ids = [f.id for f in self.get_rls_filters(table)] + ids.sort() # Combinations rather than permutations + return ids From 9d4bcad3e6c948dddc8a0a7e8c9026f21f5b8213 Mon Sep 17 00:00:00 2001 From: altef Date: Fri, 10 Jan 2020 22:49:59 +0000 Subject: [PATCH 25/39] Blacked. --- superset/security/manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 83820ab367b4a..eb363811b21ef 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -886,7 +886,7 @@ def get_rls_filters(self, table: "BaseDatasource"): :param table: The table to check against :returns: A list of filters strings. """ - if hasattr(g, 'user'): + if hasattr(g, "user"): roles = [role.id for role in g.user.roles] return [ f From 790f3951010289ed7a2ca93807940882f50b7ae8 Mon Sep 17 00:00:00 2001 From: altef Date: Fri, 10 Jan 2020 23:02:40 +0000 Subject: [PATCH 26/39] Blacked and added some attributes to check for. --- superset/connectors/sqla/views.py | 1 - superset/security/manager.py | 6 +++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/superset/connectors/sqla/views.py b/superset/connectors/sqla/views.py index f796639a58bdd..411799cd8a0c3 100644 --- a/superset/connectors/sqla/views.py +++ b/superset/connectors/sqla/views.py @@ -221,7 +221,6 @@ class SqlMetricInlineView(CompactCRUDMixin, SupersetModelView): edit_form_extra_fields = add_form_extra_fields - appbuilder.add_view_no_menu(SqlMetricInlineView) diff --git a/superset/security/manager.py b/superset/security/manager.py index eb363811b21ef..d78aeb11cfe99 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -886,7 +886,11 @@ def get_rls_filters(self, table: "BaseDatasource"): :param table: The table to check against :returns: A list of filters strings. """ - if hasattr(g, "user"): + if ( + hasattr(g, "user") + and hasattr(g.user, "roles") + and hasattr(table, "row_level_security") + ): roles = [role.id for role in g.user.roles] return [ f From 8adfc89e73256c1c6df2ac015ee3ee91a582d07d Mon Sep 17 00:00:00 2001 From: altef Date: Tue, 14 Jan 2020 03:05:21 +0000 Subject: [PATCH 27/39] Changed to a manual query as per @mistercrunch. --- superset/connectors/sqla/views.py | 1 - superset/security/manager.py | 16 +++++++--------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/superset/connectors/sqla/views.py b/superset/connectors/sqla/views.py index 411799cd8a0c3..14b7aa1f778ff 100644 --- a/superset/connectors/sqla/views.py +++ b/superset/connectors/sqla/views.py @@ -221,7 +221,6 @@ class SqlMetricInlineView(CompactCRUDMixin, SupersetModelView): edit_form_extra_fields = add_form_extra_fields -appbuilder.add_view_no_menu(SqlMetricInlineView) class RowLevelSecurityFiltersModelView(SupersetModelView, DeleteMixin): diff --git a/superset/security/manager.py b/superset/security/manager.py index d78aeb11cfe99..2a26320d32382 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -34,7 +34,7 @@ UserModelView, ) from flask_appbuilder.widgets import ListWidget -from sqlalchemy import or_ +from sqlalchemy import or_, text from sqlalchemy.engine.base import Connection from sqlalchemy.orm.mapper import Mapper @@ -888,15 +888,13 @@ def get_rls_filters(self, table: "BaseDatasource"): """ if ( hasattr(g, "user") - and hasattr(g.user, "roles") - and hasattr(table, "row_level_security") ): - roles = [role.id for role in g.user.roles] - return [ - f - for f in table.row_level_security_filters - if any(r.id in roles for r in f.roles) - ] + from superset import db + result = db.session.execute("SELECT * FROM row_level_security_filters " + "WHERE table_id = :table AND id IN " + "(SELECT id FROM rls_filter_roles WHERE role_id IN " + "(SELECT role_id FROM ab_user_role WHERE user_id = :user))", {'table': table.id, 'user': g.user.id}) + return result; return [] def get_rls_ids(self, table: "BaseDatasource") -> List[int]: From 2b0ccfcf1041c57ec82e9e086a2f7b97dbc4fe6d Mon Sep 17 00:00:00 2001 From: altef Date: Tue, 14 Jan 2020 03:19:26 +0000 Subject: [PATCH 28/39] Blacked. --- superset/security/manager.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 2a26320d32382..8322a463ec318 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -886,15 +886,17 @@ def get_rls_filters(self, table: "BaseDatasource"): :param table: The table to check against :returns: A list of filters strings. """ - if ( - hasattr(g, "user") - ): + if hasattr(g, "user"): from superset import db - result = db.session.execute("SELECT * FROM row_level_security_filters " + + result = db.session.execute( + "SELECT * FROM row_level_security_filters " "WHERE table_id = :table AND id IN " "(SELECT id FROM rls_filter_roles WHERE role_id IN " - "(SELECT role_id FROM ab_user_role WHERE user_id = :user))", {'table': table.id, 'user': g.user.id}) - return result; + "(SELECT role_id FROM ab_user_role WHERE user_id = :user))", + {"table": table.id, "user": g.user.id}, + ) + return result return [] def get_rls_ids(self, table: "BaseDatasource") -> List[int]: From b82acf6bfa8d388428735ca18f31b5a3fd9a70f0 Mon Sep 17 00:00:00 2001 From: altef Date: Tue, 14 Jan 2020 03:25:16 +0000 Subject: [PATCH 29/39] Another migration in the meantime. --- .../versions/0a6f12f60c73_add_role_level_security.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py b/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py index 4fc588bd2cf42..f905834a2bdff 100644 --- a/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py +++ b/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py @@ -17,14 +17,14 @@ """add_role_level_security Revision ID: 0a6f12f60c73 -Revises: 817e1c9b09d0 +Revises: e96dbf2cfef0 Create Date: 2019-12-04 17:07:54.390805 """ # revision identifiers, used by Alembic. revision = "0a6f12f60c73" -down_revision = "817e1c9b09d0" +down_revision = "e96dbf2cfef0" import sqlalchemy as sa from alembic import op From d94e6aa69ad2c5d68ae72f7b39962c19832394c3 Mon Sep 17 00:00:00 2001 From: altef Date: Tue, 14 Jan 2020 03:31:41 +0000 Subject: [PATCH 30/39] Black wanted some whitespace changes. --- superset/connectors/sqla/views.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/superset/connectors/sqla/views.py b/superset/connectors/sqla/views.py index 14b7aa1f778ff..f4613be8c8f7a 100644 --- a/superset/connectors/sqla/views.py +++ b/superset/connectors/sqla/views.py @@ -221,8 +221,6 @@ class SqlMetricInlineView(CompactCRUDMixin, SupersetModelView): edit_form_extra_fields = add_form_extra_fields - - class RowLevelSecurityFiltersModelView(SupersetModelView, DeleteMixin): datamodel = SQLAInterface(models.RowLevelSecurityFilter) From f76e953f53189c9cbfd1d7de807abe333688096a Mon Sep 17 00:00:00 2001 From: altef Date: Tue, 14 Jan 2020 03:38:04 +0000 Subject: [PATCH 31/39] AttributeError: 'AnonymousUserMixin' object has no attribute 'id'. --- superset/security/manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 8322a463ec318..07167331c7f94 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -886,7 +886,7 @@ def get_rls_filters(self, table: "BaseDatasource"): :param table: The table to check against :returns: A list of filters strings. """ - if hasattr(g, "user"): + if hasattr(g, "user") and hasattr("id", g.user): from superset import db result = db.session.execute( From 3d0822d0d2d1e12a017243d9ec9cb03b3f3388fa Mon Sep 17 00:00:00 2001 From: altef Date: Tue, 14 Jan 2020 03:49:06 +0000 Subject: [PATCH 32/39] Oops, did hasattr backwards. --- superset/security/manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 07167331c7f94..b8581b37ac735 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -886,7 +886,7 @@ def get_rls_filters(self, table: "BaseDatasource"): :param table: The table to check against :returns: A list of filters strings. """ - if hasattr(g, "user") and hasattr("id", g.user): + if hasattr(g, "user") and hasattr(g.user, "id"): from superset import db result = db.session.execute( From a2e6b675f9b02ca793e1754f98b1038ae0cfe9f2 Mon Sep 17 00:00:00 2001 From: altef Date: Fri, 24 Jan 2020 08:31:04 +0000 Subject: [PATCH 33/39] Changes as per @mistercrunch. --- superset/app.py | 9 +++++++++ superset/connectors/sqla/views.py | 10 ---------- superset/security/manager.py | 30 ++++++++++++++++++++++-------- 3 files changed, 31 insertions(+), 18 deletions(-) diff --git a/superset/app.py b/superset/app.py index 4a904eea1c569..57388738ee74b 100644 --- a/superset/app.py +++ b/superset/app.py @@ -134,6 +134,7 @@ def init_views(self) -> None: TableColumnInlineView, SqlMetricInlineView, TableModelView, + RowLevelSecurityFiltersModelView, ) from superset.views.annotations import ( AnnotationLayerModelView, @@ -255,6 +256,14 @@ def init_views(self) -> None: category_label=__("Manage"), icon="fa-search", ) + appbuilder.add_view( + RowLevelSecurityFiltersModelView, + "Row Level Security Filters", + label=__("Row level security filters"), + category="Security", + category_label=__("Security"), + icon="fa-lock", + ) # # Setup views with no menu diff --git a/superset/connectors/sqla/views.py b/superset/connectors/sqla/views.py index ac5e2beb50f23..fb02f28a1b1f0 100644 --- a/superset/connectors/sqla/views.py +++ b/superset/connectors/sqla/views.py @@ -257,16 +257,6 @@ class RowLevelSecurityFiltersModelView(SupersetModelView, DeleteMixin): } -appbuilder.add_view( - RowLevelSecurityFiltersModelView, - "Row Level Security Filters", - label=__("Row level security filters"), - category="Security", - category_label=__("Security"), - icon="fa-lock", -) - - class TableModelView(DatasourceModelView, DeleteMixin, YamlExportMixin): datamodel = SQLAInterface(models.SqlaTable) include_route_methods = RouteMethod.CRUD_SET diff --git a/superset/security/manager.py b/superset/security/manager.py index acbc61961ae3d..827231828e0c9 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -894,19 +894,33 @@ def get_rls_filters(self, table: "BaseDatasource"): Retrieves the appropriate row level security filters for the current user and the passed table. :param table: The table to check against - :returns: A list of filters strings. + :returns: A list of filters. """ if hasattr(g, "user") and hasattr(g.user, "id"): from superset import db + from superset.connectors.sqla.models import ( + RLSFilterRoles, + RowLevelSecurityFilter, + ) - result = db.session.execute( - "SELECT * FROM row_level_security_filters " - "WHERE table_id = :table AND id IN " - "(SELECT id FROM rls_filter_roles WHERE role_id IN " - "(SELECT role_id FROM ab_user_role WHERE user_id = :user))", - {"table": table.id, "user": g.user.id}, + user_roles = ( + db.session.query(assoc_user_role.c.role_id) + .filter(assoc_user_role.c.user_id == g.user.id) + .subquery() + ) + filter_roles = ( + db.session.query(RLSFilterRoles.c.id) + .filter(RLSFilterRoles.c.role_id.in_(user_roles)) + .subquery() + ) + query = ( + db.session.query( + RowLevelSecurityFilter.id, RowLevelSecurityFilter.clause + ) + .filter(RowLevelSecurityFilter.table_id == table.id) + .filter(RowLevelSecurityFilter.id.in_(filter_roles)) ) - return result + return query.all() return [] def get_rls_ids(self, table: "BaseDatasource") -> List[int]: From f581cf7ac663c056ca07e5bb6b7eb75f92da34b9 Mon Sep 17 00:00:00 2001 From: altef Date: Fri, 24 Jan 2020 08:36:05 +0000 Subject: [PATCH 34/39] Doesn't look like text us required here anymore. --- superset/security/manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 827231828e0c9..a1cc75aa3c33c 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -35,7 +35,7 @@ ViewMenuModelView, ) from flask_appbuilder.widgets import ListWidget -from sqlalchemy import or_, text +from sqlalchemy import or_ from sqlalchemy.engine.base import Connection from sqlalchemy.orm.mapper import Mapper From 085a501e9efe447ac119955f97611b59cb43b3f0 Mon Sep 17 00:00:00 2001 From: altef Date: Thu, 13 Feb 2020 21:46:47 +0000 Subject: [PATCH 35/39] Changes as per @dpgaspar --- superset/connectors/sqla/views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/connectors/sqla/views.py b/superset/connectors/sqla/views.py index fb02f28a1b1f0..788e0f68a34e6 100644 --- a/superset/connectors/sqla/views.py +++ b/superset/connectors/sqla/views.py @@ -233,8 +233,8 @@ class RowLevelSecurityFiltersModelView(SupersetModelView, DeleteMixin): add_title = _("Add Row level security filter") edit_title = _("Edit Row level security filter") - list_columns = ["table", "roles", "clause", "creator", "modified"] - order_columns = ["modified"] + list_columns = ["table.table_name", "roles", "clause", "creator", "modified"] + order_columns = ["table.table_name", "clause", "modified"] edit_columns = ["table", "roles", "clause"] show_columns = edit_columns search_columns = ("table", "roles", "clause") From a62511184840552f9b1c4baf2383c2da27ad94ac Mon Sep 17 00:00:00 2001 From: altef Date: Thu, 13 Feb 2020 21:56:13 +0000 Subject: [PATCH 36/39] Two RLS tests. --- tests/security_tests.py | 71 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 1 deletion(-) diff --git a/tests/security_tests.py b/tests/security_tests.py index f16f4110317d6..bc6490121f4f2 100644 --- a/tests/security_tests.py +++ b/tests/security_tests.py @@ -19,10 +19,11 @@ from unittest.mock import Mock, patch import prison +from flask import g from superset import app, appbuilder, db, security_manager, viz from superset.connectors.druid.models import DruidCluster, DruidDatasource -from superset.connectors.sqla.models import SqlaTable +from superset.connectors.sqla.models import RowLevelSecurityFilter, SqlaTable from superset.exceptions import SupersetSecurityException from superset.models.core import Database from superset.models.slice import Slice @@ -808,3 +809,71 @@ def test_assert_viz_permission(self, mock_datasource_access): with self.assertRaises(SupersetSecurityException): security_manager.assert_viz_permission(test_viz) + + +class RowLevelSecurityTests(SupersetTestCase): + """ + Testing Row Level Security + """ + + rls_entry = None + + def setUp(self): + session = db.session + + # Create the RowLevelSecurityFilter + self.rls_entry = RowLevelSecurityFilter() + self.rls_entry.table = ( + session.query(SqlaTable).filter_by(table_name="birth_names").first() + ) + self.rls_entry.clause = "gender = 'male'" + self.rls_entry.roles.append( + security_manager.find_role("Gamma") + ) # db.session.query(Role).filter_by(name="Gamma").first()) + db.session.add(self.rls_entry) + + db.session.commit() + + def tearDown(self): + session = db.session + session.delete(self.rls_entry) + session.commit() + + # Do another test to make sure it doesn't alter another query + def test_rls_filter_alters_query(self): + g.user = self.get_user( + username="gamma" + ) # self.login() doesn't actually set the user + tbl = self.get_table_by_name("birth_names") + query_obj = dict( + groupby=[], + metrics=[], + filter=[], + is_timeseries=False, + columns=["name"], + granularity=None, + from_dttm=None, + to_dttm=None, + extras={}, + ) + sql = tbl.get_query_str(query_obj) + self.assertIn("gender = 'male'", sql) + + def test_rls_filter_doesnt_alter_query(self): + g.user = self.get_user( + username="admin" + ) # self.login() doesn't actually set the user + tbl = self.get_table_by_name("birth_names") + query_obj = dict( + groupby=[], + metrics=[], + filter=[], + is_timeseries=False, + columns=["name"], + granularity=None, + from_dttm=None, + to_dttm=None, + extras={}, + ) + sql = tbl.get_query_str(query_obj) + self.assertNotIn("gender = 'male'", sql) From a131a72f389ee28a2cf3ef7dcc547e7e6a5c9572 Mon Sep 17 00:00:00 2001 From: altef Date: Fri, 21 Feb 2020 21:37:16 +0000 Subject: [PATCH 37/39] Row level security is now disabled by default via the feature flag ENABLE_ROW_LEVEL_SECURITY. --- UPDATING.md | 2 +- superset/app.py | 17 +++++++++-------- superset/config.py | 3 +++ 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index cb1416dd2c28d..f4cf5c0670841 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -29,7 +29,7 @@ previously cached results will be invalidated when updating to the next version. * [8699](https://github.com/apache/incubator-superset/pull/8699): A `row_level_security_filters` table has been added, which is many-to-many with `tables` and `ab_roles`. The applicable filters -are added to the sqla query, and the RLS ids are added to the query cache keys. They can be +are added to the sqla query, and the RLS ids are added to the query cache keys. If RLS is enabled in config.py (`ENABLE_ROW_LEVEL_SECURITY = True`; by default, it is disabled), they can be accessed through the `Security` menu, or when editting a table. * [8732](https://github.com/apache/incubator-superset/pull/8732): Swagger user interface is now enabled by default. diff --git a/superset/app.py b/superset/app.py index 57388738ee74b..39ddc079f9ee4 100644 --- a/superset/app.py +++ b/superset/app.py @@ -256,14 +256,15 @@ def init_views(self) -> None: category_label=__("Manage"), icon="fa-search", ) - appbuilder.add_view( - RowLevelSecurityFiltersModelView, - "Row Level Security Filters", - label=__("Row level security filters"), - category="Security", - category_label=__("Security"), - icon="fa-lock", - ) + if self.config["ENABLE_ROW_LEVEL_SECURITY"]: + appbuilder.add_view( + RowLevelSecurityFiltersModelView, + "Row Level Security Filters", + label=__("Row level security filters"), + category="Security", + category_label=__("Security"), + icon="fa-lock", + ) # # Setup views with no menu diff --git a/superset/config.py b/superset/config.py index b58907d19c828..9e667591fec74 100644 --- a/superset/config.py +++ b/superset/config.py @@ -723,6 +723,9 @@ class CeleryConfig: # pylint: disable=too-few-public-methods "force_https_permanent": False, } +# Do you want to enable Row Level Security? +ENABLE_ROW_LEVEL_SECURITY = False + # # Flask session cookie options # From 1451c0526bdb37c0ca457b9100693ae4d9a08d9b Mon Sep 17 00:00:00 2001 From: altef Date: Fri, 21 Feb 2020 23:08:14 +0000 Subject: [PATCH 38/39] New head to revise. --- .../versions/0a6f12f60c73_add_role_level_security.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py b/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py index f905834a2bdff..b202b870810e3 100644 --- a/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py +++ b/superset/migrations/versions/0a6f12f60c73_add_role_level_security.py @@ -17,14 +17,14 @@ """add_role_level_security Revision ID: 0a6f12f60c73 -Revises: e96dbf2cfef0 +Revises: 3325d4caccc8 Create Date: 2019-12-04 17:07:54.390805 """ # revision identifiers, used by Alembic. revision = "0a6f12f60c73" -down_revision = "e96dbf2cfef0" +down_revision = "3325d4caccc8" import sqlalchemy as sa from alembic import op From 5eeb296223dbd25e0fbca8386faee152e4402499 Mon Sep 17 00:00:00 2001 From: altef Date: Sat, 22 Feb 2020 09:06:23 +0000 Subject: [PATCH 39/39] Changed the comment. --- superset/config.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/superset/config.py b/superset/config.py index 866541067d8ca..ef473ed27a9ec 100644 --- a/superset/config.py +++ b/superset/config.py @@ -731,7 +731,13 @@ class CeleryConfig: # pylint: disable=too-few-public-methods "force_https_permanent": False, } -# Do you want to enable Row Level Security? +# Note that: RowLevelSecurityFilter is only given by default to the Admin role +# and the Admin Role does have the all_datasources security permission. +# But, if users create a specific role with access to RowLevelSecurityFilter MVC +# and a custom datasource access, the table dropdown will not be correctly filtered +# by that custom datasource access. So we are assuming a default security config, +# a custom security config could potentially give access to setting filters on +# tables that users do not have access to. ENABLE_ROW_LEVEL_SECURITY = False #