From ab3770667c0b11043b177838f8c2eddd717fcfcc Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian <1858430+suddjian@users.noreply.github.com> Date: Thu, 31 Mar 2022 14:05:17 -0700 Subject: [PATCH] chore!: remove `ROW_LEVEL_SECURITY` feature flag (permanently enable) (#19230) * permanently turn on rls feature flag * unused imports * docs * docs * unused import --- RELEASING/release-notes-1-0/README.md | 1 - RESOURCES/FEATURE_FLAGS.md | 1 - UPDATING.md | 1 + superset/config.py | 8 ----- superset/connectors/sqla/models.py | 5 ++- superset/connectors/sqla/views.py | 13 +------ superset/initialization/__init__.py | 3 -- superset/security/manager.py | 5 +-- tests/integration_tests/sqla_views_tests.py | 40 --------------------- 9 files changed, 5 insertions(+), 72 deletions(-) delete mode 100644 tests/integration_tests/sqla_views_tests.py diff --git a/RELEASING/release-notes-1-0/README.md b/RELEASING/release-notes-1-0/README.md index d476cb6e1..ed1eeea0d 100644 --- a/RELEASING/release-notes-1-0/README.md +++ b/RELEASING/release-notes-1-0/README.md @@ -96,7 +96,6 @@ Some of the new features in this release are disabled by default. Each has a fea | Dashboard Native Filters | `DASHBOARD_NATIVE_FILTERS: True` | | | Alerts & Reporting | `ALERT_REPORTS: True` | [Celery workers configured & celery beat process](https://superset.apache.org/docs/installation/async-queries-celery) | | Homescreen Thumbnails | `THUMBNAILS: TRUE, THUMBNAIL_CACHE_CONFIG: CacheConfig = { "CACHE_TYPE": "null", "CACHE_NO_NULL_WARNING": True}`| selenium, pillow 7, celery | -| Row Level Security | `ROW_LEVEL_SECURITY` | | [Extra Documentation](https://superset.apache.org/docs/security#row-level-security) | Dynamic Viz Plugin Import | `DYNAMIC_PLUGINS: True` | | # Stability and Bugfixes diff --git a/RESOURCES/FEATURE_FLAGS.md b/RESOURCES/FEATURE_FLAGS.md index 77e381c72..26ac6bfde 100644 --- a/RESOURCES/FEATURE_FLAGS.md +++ b/RESOURCES/FEATURE_FLAGS.md @@ -52,7 +52,6 @@ These features flags are **safe for production** and have been tested. - ESCAPE_MARKDOWN_HTML - ENABLE_TEMPLATE_PROCESSING - LISTVIEWS_DEFAULT_CARD_VIEW -- ROW_LEVEL_SECURITY - SCHEDULED_QUERIES [(docs)](https://superset.apache.org/docs/installation/alerts-reports) - SQL_VALIDATORS_BY_ENGINE [(docs)](https://superset.apache.org/docs/installation/sql-templating) - SQLLAB_BACKEND_PERSISTENCE diff --git a/UPDATING.md b/UPDATING.md index a911b4b54..865b3907d 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -29,6 +29,7 @@ assists people when migrating to a new version. ### Breaking Changes +- [19230](https://github.com/apache/superset/pull/19230): The `ROW_LEVEL_SECURITY` feature flag has been removed (permanently enabled). Any deployments which had set this flag to false will need to verify that the presence of the Row Level Security feature does not interfere with their use case. - [19168](https://github.com/apache/superset/pull/19168): Celery upgrade to 5.X has breaking changes on it's command line invocation. Please follow: https://docs.celeryq.dev/en/stable/whatsnew-5.2.html#step-1-adjust-your-command-line-invocation Consider migrating you celery config if you haven't already: https://docs.celeryq.dev/en/stable/userguide/configuration.html#conf-old-settings-map diff --git a/superset/config.py b/superset/config.py index 4c68aa255..62198c484 100644 --- a/superset/config.py +++ b/superset/config.py @@ -397,14 +397,6 @@ DEFAULT_FEATURE_FLAGS: Dict[str, bool] = { "DASHBOARD_FILTERS_EXPERIMENTAL": False, "GLOBAL_ASYNC_QUERIES": False, "VERSIONED_EXPORT": True, - # 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. - "ROW_LEVEL_SECURITY": True, "EMBEDDED_SUPERSET": False, # Enables Alerts and reports new implementation "ALERT_REPORTS": False, diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 291ddb376..0600f3b10 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -1393,8 +1393,7 @@ class SqlaTable(Model, BaseDatasource): # pylint: disable=too-many-public-metho raise QueryObjectValidationError( _("Invalid filter operation type: %(op)s", op=op) ) - if is_feature_enabled("ROW_LEVEL_SECURITY"): - where_clause_and += self.get_sqla_row_level_filters(template_processor) + where_clause_and += self.get_sqla_row_level_filters(template_processor) if extras: where = extras.get("where") if where: @@ -1810,7 +1809,7 @@ class SqlaTable(Model, BaseDatasource): # pylint: disable=too-many-public-metho templatable_statements.append(extras["where"]) if "having" in extras: templatable_statements.append(extras["having"]) - if is_feature_enabled("ROW_LEVEL_SECURITY") and self.is_rls_supported: + if self.is_rls_supported: templatable_statements += [ f.clause for f in security_manager.get_rls_filters(self) ] diff --git a/superset/connectors/sqla/views.py b/superset/connectors/sqla/views.py index 116b18869..aa176ba2b 100644 --- a/superset/connectors/sqla/views.py +++ b/superset/connectors/sqla/views.py @@ -22,15 +22,13 @@ from typing import Any, cast from flask import current_app, flash, Markup, redirect from flask_appbuilder import CompactCRUDMixin, expose from flask_appbuilder.fieldwidgets import Select2Widget -from flask_appbuilder.hooks import before_request from flask_appbuilder.models.sqla.interface import SQLAInterface from flask_appbuilder.security.decorators import has_access from flask_babel import lazy_gettext as _ -from werkzeug.exceptions import NotFound from wtforms.ext.sqlalchemy.fields import QuerySelectField from wtforms.validators import Regexp -from superset import app, db, is_feature_enabled +from superset import app, db from superset.connectors.base.views import DatasourceModelView from superset.connectors.sqla import models from superset.constants import MODEL_VIEW_RW_METHOD_PERMISSION_MAP, RouteMethod @@ -328,15 +326,6 @@ class RowLevelSecurityFiltersModelView(SupersetModelView, DeleteMixin): add_form_query_rel_fields = app.config["RLS_FORM_QUERY_REL_FIELDS"] edit_form_query_rel_fields = add_form_query_rel_fields - @staticmethod - def is_enabled() -> bool: - return is_feature_enabled("ROW_LEVEL_SECURITY") - - @before_request - def ensure_enabled(self) -> None: - if not self.is_enabled(): - raise NotFound() - class TableModelView( # pylint: disable=too-many-ancestors DatasourceModelView, DeleteMixin, YamlExportMixin diff --git a/superset/initialization/__init__.py b/superset/initialization/__init__.py index 5b11f0930..e600b0f2f 100644 --- a/superset/initialization/__init__.py +++ b/superset/initialization/__init__.py @@ -277,9 +277,6 @@ class SupersetAppInitializer: # pylint: disable=too-many-public-methods category="Security", category_label=__("Security"), icon="fa-lock", - menu_cond=lambda: feature_flag_manager.is_feature_enabled( - "ROW_LEVEL_SECURITY" - ), ) # diff --git a/superset/security/manager.py b/superset/security/manager.py index ab21dbd54..f57f1166c 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1222,11 +1222,8 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods return [f.get("clause", "") for f in self.get_guest_rls_filters(table)] def get_rls_cache_key(self, datasource: "BaseDatasource") -> List[str]: - # pylint: disable=import-outside-toplevel - from superset import is_feature_enabled - rls_ids = [] - if is_feature_enabled("ROW_LEVEL_SECURITY") and datasource.is_rls_supported: + if datasource.is_rls_supported: rls_ids = self.get_rls_ids(datasource) rls_str = [str(rls_id) for rls_id in rls_ids] guest_rls = self.get_guest_rls_filters_str(datasource) diff --git a/tests/integration_tests/sqla_views_tests.py b/tests/integration_tests/sqla_views_tests.py deleted file mode 100644 index 92c350c87..000000000 --- a/tests/integration_tests/sqla_views_tests.py +++ /dev/null @@ -1,40 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. -from tests.integration_tests.base_tests import SupersetTestCase -from tests.integration_tests.conftest import with_feature_flags - - -class TestRowLevelSecurityFiltersModelView(SupersetTestCase): - @with_feature_flags(ROW_LEVEL_SECURITY=False) - def test_rls_disabled(self): - """ - RLS Filters Model View: Responds not found when disabled - """ - self.login(username="admin") - uri = "/rowlevelsecurityfiltersmodelview/api" - rv = self.client.get(uri) - self.assertEqual(rv.status_code, 404) - - @with_feature_flags(ROW_LEVEL_SECURITY=True) - def test_rls_enabled(self): - """ - RLS Filters Model View: Responds successfully when enabled - """ - self.login(username="admin") - uri = "/rowlevelsecurityfiltersmodelview/api" - rv = self.client.get(uri) - self.assertEqual(rv.status_code, 200)