feat: Convert ENABLE_BROAD_ACTIVITY_ACCESS and MENU_HIDE_USER_INFO in…

…to feature flags (#24345)

UPDATING.md

@@ -34,6 +34,7 @@ assists people when migrating to a new version.
 
 ### Breaking Changes
 
+- [24345](https://github.com/apache/superset/pull/24345) Converts `ENABLE_BROAD_ACTIVITY_ACCESS` and `MENU_HIDE_USER_INFO` into feature flags and changes the value of `ENABLE_BROAD_ACTIVITY_ACCESS` to `False` as it's more secure.
 - [24342](https://github.com/apache/superset/pull/24342): Removed deprecated API `/superset/tables/<int:db_id>/<schema>/...`
 - [24335](https://github.com/apache/superset/pull/24335): Removed deprecated API `/superset/filter/<datasource_type>/<int:datasource_id>/<column>/`
 - [24333](https://github.com/apache/superset/pull/24333): Removed deprecated API `/superset/datasources`

superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts

@@ -40,6 +40,7 @@ export enum FeatureFlag {
   EMBEDDABLE_CHARTS = 'EMBEDDABLE_CHARTS',
   EMBEDDED_SUPERSET = 'EMBEDDED_SUPERSET',
   ENABLE_ADVANCED_DATA_TYPES = 'ENABLE_ADVANCED_DATA_TYPES',
+  ENABLE_BROAD_ACTIVITY_ACCESS = 'ENABLE_BROAD_ACTIVITY_ACCESS',
   ENABLE_EXPLORE_DRAG_AND_DROP = 'ENABLE_EXPLORE_DRAG_AND_DROP',
   ENABLE_JAVASCRIPT_CONTROLS = 'ENABLE_JAVASCRIPT_CONTROLS',
   ENABLE_TEMPLATE_PROCESSING = 'ENABLE_TEMPLATE_PROCESSING',

superset-frontend/src/pages/ChartList/index.tsx

@@ -68,7 +68,6 @@ import setupPlugins from 'src/setup/setupPlugins';
 import InfoTooltip from 'src/components/InfoTooltip';
 import CertifiedBadge from 'src/components/CertifiedBadge';
 import { GenericLink } from 'src/components/GenericLink/GenericLink';
-import getBootstrapData from 'src/utils/getBootstrapData';
 import Owner from 'src/types/Owner';
 import { loadTags } from 'src/components/Tags/utils';
 import ChartCard from 'src/features/charts/ChartCard';
@@ -156,8 +155,6 @@ const StyledActions = styled.div`
   color: ${({ theme }) => theme.colors.grayscale.base};
 `;
 
-const bootstrapData = getBootstrapData();
-
 function ChartList(props: ChartListProps) {
   const {
     addDangerToast,
@@ -234,8 +231,9 @@ function ChartList(props: ChartListProps) {
   const canExport =
     hasPerm('can_export') && isFeatureEnabled(FeatureFlag.VERSIONED_EXPORT);
   const initialSort = [{ id: 'changed_on_delta_humanized', desc: true }];
-  const enableBroadUserAccess =
-    bootstrapData.common.conf.ENABLE_BROAD_ACTIVITY_ACCESS;
+  const enableBroadUserAccess = isFeatureEnabled(
+    FeatureFlag.ENABLE_BROAD_ACTIVITY_ACCESS,
+  );
   const handleBulkChartExport = (chartsToExport: Chart[]) => {
     const ids = chartsToExport.map(({ id }) => id);
     handleResourceExport('chart', ids, () => {

superset-frontend/src/pages/DashboardList/index.tsx

@@ -54,7 +54,6 @@ import Dashboard from 'src/dashboard/containers/Dashboard';
 import { Dashboard as CRUDDashboard } from 'src/views/CRUD/types';
 import CertifiedBadge from 'src/components/CertifiedBadge';
 import { loadTags } from 'src/components/Tags/utils';
-import getBootstrapData from 'src/utils/getBootstrapData';
 import DashboardCard from 'src/features/dashboards/DashboardCard';
 import { DashboardStatus } from 'src/features/dashboards/types';
 
@@ -101,8 +100,6 @@ const Actions = styled.div`
   color: ${({ theme }) => theme.colors.grayscale.base};
 `;
 
-const bootstrapData = getBootstrapData();
-
 function DashboardList(props: DashboardListProps) {
   const {
     addDangerToast,
@@ -143,8 +140,9 @@ function DashboardList(props: DashboardListProps) {
   const [importingDashboard, showImportModal] = useState<boolean>(false);
   const [passwordFields, setPasswordFields] = useState<string[]>([]);
   const [preparingExport, setPreparingExport] = useState<boolean>(false);
-  const enableBroadUserAccess =
-    bootstrapData?.common?.conf?.ENABLE_BROAD_ACTIVITY_ACCESS;
+  const enableBroadUserAccess = isFeatureEnabled(
+    FeatureFlag.ENABLE_BROAD_ACTIVITY_ACCESS,
+  );
   const [sshTunnelPasswordFields, setSSHTunnelPasswordFields] = useState<
     string[]
   >([]);

superset/config.py

@@ -472,6 +472,11 @@ class D3Format(TypedDict, total=False):
     # otherwise enabling this flag won't have any effect on the DB.
     "SSH_TUNNELING": False,
     "AVOID_COLORS_COLLISION": True,
+    # Set to False to only allow viewing own recent activity
+    # or to disallow users from viewing other users profile page
+    "ENABLE_BROAD_ACTIVITY_ACCESS": False,
+    # Do not show user info or profile in the menu
+    "MENU_HIDE_USER_INFO": False,
 }
 
 # ------------------------------
@@ -1477,13 +1482,6 @@ def EMAIL_HEADER_MUTATOR(  # pylint: disable=invalid-name,unused-argument
 #
 DATASET_HEALTH_CHECK: Callable[[SqlaTable], str] | None = None
 
-# Do not show user info or profile in the menu
-MENU_HIDE_USER_INFO = False
-
-# Set to False to only allow viewing own recent activity
-# or to disallow users from viewing other users profile page
-ENABLE_BROAD_ACTIVITY_ACCESS = True
-
 # the advanced data type key should correspond to that set in the column metadata
 ADVANCED_DATA_TYPES: dict[str, AdvancedDataType] = {
     "internet_address": internet_address,

superset/connectors/sqla/models.py

@@ -32,7 +32,7 @@
 import pandas as pd
 import sqlalchemy as sa
 import sqlparse
-from flask import current_app, escape, Markup
+from flask import escape, Markup
 from flask_appbuilder import Model
 from flask_babel import lazy_gettext as _
 from jinja2.exceptions import TemplateError
@@ -594,9 +594,8 @@ def changed_by_name(self) -> str:
 
     @property
     def changed_by_url(self) -> str:
-        if (
-            not self.changed_by
-            or not current_app.config["ENABLE_BROAD_ACTIVITY_ACCESS"]
+        if not self.changed_by or not is_feature_enabled(
+            "ENABLE_BROAD_ACTIVITY_ACCESS"
         ):
             return ""
         return f"/superset/profile/{self.changed_by.username}"

superset/models/dashboard.py

@@ -24,7 +24,6 @@
 from typing import Any, Callable
 
 import sqlalchemy as sqla
-from flask import current_app
 from flask_appbuilder import Model
 from flask_appbuilder.models.decorators import renders
 from flask_appbuilder.security.sqla.models import User
@@ -271,9 +270,8 @@ def changed_by_name(self) -> str:
 
     @property
     def changed_by_url(self) -> str:
-        if (
-            not self.changed_by
-            or not current_app.config["ENABLE_BROAD_ACTIVITY_ACCESS"]
+        if not self.changed_by or not is_feature_enabled(
+            "ENABLE_BROAD_ACTIVITY_ACCESS"
         ):
             return ""
         return f"/superset/profile/{self.changed_by.username}"

superset/models/filter_set.py

@@ -20,13 +20,12 @@
 import logging
 from typing import Any
 
-from flask import current_app
 from flask_appbuilder import Model
 from sqlalchemy import Column, ForeignKey, Integer, MetaData, String, Text
 from sqlalchemy.orm import relationship
 from sqlalchemy_utils import generic_relationship
 
-from superset import app, db
+from superset import app, db, is_feature_enabled
 from superset.models.helpers import AuditMixinNullable
 
 metadata = Model.metadata  # pylint: disable=no-member
@@ -68,9 +67,8 @@ def changed_by_name(self) -> str:
 
     @property
     def changed_by_url(self) -> str:
-        if (
-            not self.changed_by
-            or not current_app.config["ENABLE_BROAD_ACTIVITY_ACCESS"]
+        if not self.changed_by or not is_feature_enabled(
+            "ENABLE_BROAD_ACTIVITY_ACCESS"
         ):
             return ""
         return f"/superset/profile/{self.changed_by.username}"

superset/models/slice.py

@@ -22,7 +22,6 @@
 from urllib import parse
 
 import sqlalchemy as sqla
-from flask import current_app
 from flask_appbuilder import Model
 from flask_appbuilder.models.decorators import renders
 from markupsafe import escape, Markup
@@ -340,9 +339,8 @@ def created_by_url(self) -> str:
 
     @property
     def changed_by_url(self) -> str:
-        if (
-            not self.changed_by
-            or not current_app.config["ENABLE_BROAD_ACTIVITY_ACCESS"]
+        if not self.changed_by or not is_feature_enabled(
+            "ENABLE_BROAD_ACTIVITY_ACCESS"
         ):
             return ""
         return f"/superset/profile/{self.changed_by.username}"

superset/security/manager.py

@@ -1980,8 +1980,11 @@ def get_rls_cache_key(self, datasource: "BaseDatasource") -> list[str]:
 
     @staticmethod
     def raise_for_user_activity_access(user_id: int) -> None:
+        # pylint: disable=import-outside-toplevel
+        from superset.extensions import feature_flag_manager
+
         if not get_user_id() or (
-            not current_app.config["ENABLE_BROAD_ACTIVITY_ACCESS"]
+            not feature_flag_manager.is_feature_enabled("ENABLE_BROAD_ACTIVITY_ACCESS")
             and user_id != get_user_id()
         ):
             raise SupersetSecurityException(

superset/views/base.py

@@ -58,6 +58,7 @@
     conf,
     db,
     get_feature_flags,
+    is_feature_enabled,
     security_manager,
 )
 from superset.commands.exceptions import CommandException, CommandInvalidError
@@ -383,12 +384,12 @@ def menu_data(user: User) -> dict[str, Any]:
             "show_language_picker": len(languages.keys()) > 1,
             "user_is_anonymous": user.is_anonymous,
             "user_info_url": None
-            if appbuilder.app.config["MENU_HIDE_USER_INFO"]
+            if is_feature_enabled("MENU_HIDE_USER_INFO")
             else appbuilder.get_url_for_userinfo,
             "user_logout_url": appbuilder.get_url_for_logout,
             "user_login_url": appbuilder.get_url_for_login,
             "user_profile_url": None
-            if user.is_anonymous or appbuilder.app.config["MENU_HIDE_USER_INFO"]
+            if user.is_anonymous or is_feature_enabled("MENU_HIDE_USER_INFO")
             else f"/superset/profile/{user.username}",
             "locale": session.get("locale", "en"),
         },

superset/views/core.py

@@ -2306,7 +2306,7 @@ def profile(self, username: str) -> FlaskResponse:
         # Prevent returning 404 when user is not found to prevent username scanning
         user_id = -1 if not user else user.id
         # Prevent unauthorized access to other user's profiles,
-        # unless configured to do so on with ENABLE_BROAD_ACTIVITY_ACCESS
+        # unless configured to do so with ENABLE_BROAD_ACTIVITY_ACCESS
         if error_obj := self.get_user_activity_access_error(user_id):
             return error_obj
 

tests/integration_tests/charts/api_tests.py

@@ -34,6 +34,7 @@
 from superset.models.slice import Slice
 from superset.utils.core import get_example_default_schema
 
+from tests.integration_tests.conftest import with_feature_flags
 from tests.integration_tests.base_api_tests import ApiOwnersTestCaseMixin
 from tests.integration_tests.base_tests import SupersetTestCase
 from tests.integration_tests.fixtures.birth_names_dashboard import (
@@ -605,12 +606,11 @@ def test_update_chart(self):
         db.session.commit()
 
     @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+    @with_feature_flags(ENABLE_BROAD_ACTIVITY_ACCESS=False)
     def test_chart_activity_access_disabled(self):
         """
         Chart API: Test ENABLE_BROAD_ACTIVITY_ACCESS = False
         """
-        access_flag = app.config["ENABLE_BROAD_ACTIVITY_ACCESS"]
-        app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = False
         admin = self.get_user("admin")
         birth_names_table_id = SupersetTestCase.get_table(name="birth_names").id
         chart_id = self.insert_chart("title", [admin.id], birth_names_table_id).id
@@ -626,17 +626,15 @@ def test_chart_activity_access_disabled(self):
         self.assertEqual(model.slice_name, new_name)
         self.assertEqual(model.changed_by_url, "")
 
-        app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = access_flag
         db.session.delete(model)
         db.session.commit()
 
     @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+    @with_feature_flags(ENABLE_BROAD_ACTIVITY_ACCESS=True)
     def test_chart_activity_access_enabled(self):
         """
         Chart API: Test ENABLE_BROAD_ACTIVITY_ACCESS = True
         """
-        access_flag = app.config["ENABLE_BROAD_ACTIVITY_ACCESS"]
-        app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = True
         admin = self.get_user("admin")
         birth_names_table_id = SupersetTestCase.get_table(name="birth_names").id
         chart_id = self.insert_chart("title", [admin.id], birth_names_table_id).id
@@ -652,7 +650,6 @@ def test_chart_activity_access_enabled(self):
         self.assertEqual(model.slice_name, new_name)
         self.assertEqual(model.changed_by_url, "/superset/profile/admin")
 
-        app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = access_flag
         db.session.delete(model)
         db.session.commit()
 

tests/integration_tests/core_tests.py

@@ -727,39 +727,36 @@ def test_user_profile(self, username="admin"):
             data = self.get_json_resp(endpoint)
             self.assertNotIn("message", data)
 
-    def test_user_profile_optional_access(self):
+    def test_user_profile_default_access(self):
         self.login(username="gamma")
         resp = self.client.get(f"/superset/profile/admin/")
-        self.assertEqual(resp.status_code, 200)
-
-        app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = False
-        resp = self.client.get(f"/superset/profile/admin/")
         self.assertEqual(resp.status_code, 403)
 
-        # Restore config
-        app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = True
+    @with_feature_flags(ENABLE_BROAD_ACTIVITY_ACCESS=True)
+    def test_user_profile_broad_access(self):
+        self.login(username="gamma")
+        resp = self.client.get(f"/superset/profile/admin/")
+        self.assertEqual(resp.status_code, 200)
 
     @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
-    def test_user_activity_access(self, username="gamma"):
+    def test_user_activity_default_access(self, username="gamma"):
         self.login(username=username)
 
-        # accessing own and other users' activity is allowed by default
         for user in ("admin", "gamma"):
             for endpoint in self._get_user_activity_endpoints(user):
                 resp = self.client.get(endpoint)
-                assert resp.status_code == 200
+                expected_status_code = 200 if user == username else 403
+                assert resp.status_code == expected_status_code
+
+    @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+    @with_feature_flags(ENABLE_BROAD_ACTIVITY_ACCESS=True)
+    def test_user_activity_broad_access(self, username="gamma"):
+        self.login(username=username)
 
-        # disabling flag will block access to other users' activity data
-        access_flag = app.config["ENABLE_BROAD_ACTIVITY_ACCESS"]
-        app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = False
         for user in ("admin", "gamma"):
             for endpoint in self._get_user_activity_endpoints(user):
                 resp = self.client.get(endpoint)
-                expected_status_code = 200 if user == username else 403
-                assert resp.status_code == expected_status_code
-
-        # restore flag
-        app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = access_flag
+                assert resp.status_code == 200
 
     @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
     def test_slice_id_is_always_logged_correctly_on_web_request(self):

tests/integration_tests/dashboards/api_tests.py

@@ -39,6 +39,7 @@
 from superset.utils.core import backend, override_user
 from superset.views.base import generate_download_headers
 
+from tests.integration_tests.conftest import with_feature_flags
 from tests.integration_tests.base_api_tests import ApiOwnersTestCaseMixin
 from tests.integration_tests.base_tests import SupersetTestCase
 from tests.integration_tests.fixtures.importexport import (
@@ -1405,12 +1406,11 @@ def test_update_dashboard(self):
         db.session.delete(model)
         db.session.commit()
 
+    @with_feature_flags(ENABLE_BROAD_ACTIVITY_ACCESS=False)
     def test_dashboard_activity_access_disabled(self):
         """
         Dashboard API: Test ENABLE_BROAD_ACTIVITY_ACCESS = False
         """
-        access_flag = app.config["ENABLE_BROAD_ACTIVITY_ACCESS"]
-        app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = False
         admin = self.get_user("admin")
         admin_role = self.get_role("Admin")
         dashboard_id = self.insert_dashboard(
@@ -1426,16 +1426,14 @@ def test_dashboard_activity_access_disabled(self):
         self.assertEqual(model.dashboard_title, "title2")
         self.assertEqual(model.changed_by_url, "")
 
-        app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = access_flag
         db.session.delete(model)
         db.session.commit()
 
+    @with_feature_flags(ENABLE_BROAD_ACTIVITY_ACCESS=True)
     def test_dashboard_activity_access_enabled(self):
         """
         Dashboard API: Test ENABLE_BROAD_ACTIVITY_ACCESS = True
         """
-        access_flag = app.config["ENABLE_BROAD_ACTIVITY_ACCESS"]
-        app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = True
         admin = self.get_user("admin")
         admin_role = self.get_role("Admin")
         dashboard_id = self.insert_dashboard(
@@ -1451,7 +1449,6 @@ def test_dashboard_activity_access_enabled(self):
         self.assertEqual(model.dashboard_title, "title2")
         self.assertEqual(model.changed_by_url, "/superset/profile/admin")
 
-        app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = access_flag
         db.session.delete(model)
         db.session.commit()