From 54cfc53ed61f574d2b17a7ee7f8199757deaaf2d Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Thu, 1 Sep 2022 20:55:05 +0100 Subject: [PATCH 1/5] fix: disallow users from viewing other user's profile on ENABLE_BROAD_ACTIVITY_ACCESS --- .../src/views/CRUD/chart/ChartList.tsx | 25 +++++++++++++------ .../views/CRUD/dashboard/DashboardList.tsx | 10 +++++++- superset/config.py | 1 + superset/views/base.py | 1 + superset/views/core.py | 9 +++++-- 5 files changed, 35 insertions(+), 11 deletions(-) diff --git a/superset-frontend/src/views/CRUD/chart/ChartList.tsx b/superset-frontend/src/views/CRUD/chart/ChartList.tsx index ec58b28bb1e5c..d160c79374646 100644 --- a/superset-frontend/src/views/CRUD/chart/ChartList.tsx +++ b/superset-frontend/src/views/CRUD/chart/ChartList.tsx @@ -62,6 +62,7 @@ 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 { bootstrapData } from 'src/preamble'; import ChartCard from './ChartCard'; const FlexRowContainer = styled.div` @@ -213,7 +214,8 @@ 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 || false; const handleBulkChartExport = (chartsToExport: Chart[]) => { const ids = chartsToExport.map(({ id }) => id); handleResourceExport('chart', ids, () => { @@ -325,13 +327,20 @@ function ChartList(props: ChartListProps) { changed_by_url: changedByUrl, }, }, - }: any) => ( - - {lastSavedBy?.first_name - ? `${lastSavedBy?.first_name} ${lastSavedBy?.last_name}` - : null} - - ), + }: any) => + enableBroadUserAccess ? ( + + {lastSavedBy?.first_name + ? `${lastSavedBy?.first_name} ${lastSavedBy?.last_name}` + : null} + + ) : ( + <> + {lastSavedBy?.first_name + ? `${lastSavedBy?.first_name} ${lastSavedBy?.last_name}` + : null} + + ), Header: t('Modified by'), accessor: 'last_saved_by.first_name', size: 'xl', diff --git a/superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx b/superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx index 7dbb30159d91e..bbc6d4328792f 100644 --- a/superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx +++ b/superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx @@ -49,6 +49,7 @@ import ImportModelsModal from 'src/components/ImportModal/index'; import Dashboard from 'src/dashboard/containers/Dashboard'; import CertifiedBadge from 'src/components/CertifiedBadge'; +import { bootstrapData } from 'src/preamble'; import DashboardCard from './DashboardCard'; import { DashboardStatus } from './types'; @@ -132,6 +133,8 @@ function DashboardList(props: DashboardListProps) { const [importingDashboard, showImportModal] = useState(false); const [passwordFields, setPasswordFields] = useState([]); const [preparingExport, setPreparingExport] = useState(false); + const enableBroadUserAccess = + bootstrapData?.common.conf.ENABLE_BROAD_ACTIVITY_ACCESS || false; const openDashboardImportModal = () => { showImportModal(true); @@ -290,7 +293,12 @@ function DashboardList(props: DashboardListProps) { changed_by_url: changedByUrl, }, }, - }: any) => {changedByName}, + }: any) => + enableBroadUserAccess ? ( + {changedByName} + ) : ( + <>{changedByName} + ), Header: t('Modified by'), accessor: 'changed_by.first_name', size: 'xl', diff --git a/superset/config.py b/superset/config.py index e5e9f506ccba1..29b644d27f711 100644 --- a/superset/config.py +++ b/superset/config.py @@ -1335,6 +1335,7 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument 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 diff --git a/superset/views/base.py b/superset/views/base.py index 4bc491fdeb311..9fbd70291d393 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -88,6 +88,7 @@ "SUPERSET_DASHBOARD_PERIODICAL_REFRESH_WARNING_MESSAGE", "DISABLE_DATASET_SOURCE_EDIT", "ENABLE_JAVASCRIPT_CONTROLS", + "ENABLE_BROAD_ACTIVITY_ACCESS", "DEFAULT_SQLLAB_LIMIT", "DEFAULT_VIZ_TYPE", "SQL_MAX_ROW", diff --git a/superset/views/core.py b/superset/views/core.py index 4f392337902b8..290cd302ee409 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -2744,8 +2744,13 @@ def profile(self, username: str) -> FlaskResponse: user = ( db.session.query(ab_models.User).filter_by(username=username).one_or_none() ) - if not user: - abort(404, description=f"User: {username} does not exist.") + # 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 + error_obj = self.get_user_activity_access_error(user_id) + if error_obj: + return error_obj payload = { "user": bootstrap_user_data(user, include_perms=True), From 37c65be21dcd362008df853015a6752a332512ee Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Fri, 2 Sep 2022 11:46:47 +0100 Subject: [PATCH 2/5] fix FE test --- superset-frontend/src/views/CRUD/chart/ChartList.tsx | 2 +- superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/superset-frontend/src/views/CRUD/chart/ChartList.tsx b/superset-frontend/src/views/CRUD/chart/ChartList.tsx index d160c79374646..d66cb4fe0ac69 100644 --- a/superset-frontend/src/views/CRUD/chart/ChartList.tsx +++ b/superset-frontend/src/views/CRUD/chart/ChartList.tsx @@ -215,7 +215,7 @@ function ChartList(props: ChartListProps) { 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 || false; + bootstrapData?.common?.conf?.ENABLE_BROAD_ACTIVITY_ACCESS || false; const handleBulkChartExport = (chartsToExport: Chart[]) => { const ids = chartsToExport.map(({ id }) => id); handleResourceExport('chart', ids, () => { diff --git a/superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx b/superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx index bbc6d4328792f..f38f733fb1572 100644 --- a/superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx +++ b/superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx @@ -134,7 +134,7 @@ function DashboardList(props: DashboardListProps) { const [passwordFields, setPasswordFields] = useState([]); const [preparingExport, setPreparingExport] = useState(false); const enableBroadUserAccess = - bootstrapData?.common.conf.ENABLE_BROAD_ACTIVITY_ACCESS || false; + bootstrapData?.common?.conf?.ENABLE_BROAD_ACTIVITY_ACCESS || false; const openDashboardImportModal = () => { showImportModal(true); From 35f792b3fb9ce2e16225e15a845d57caeb712f1f Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Fri, 2 Sep 2022 14:47:19 +0100 Subject: [PATCH 3/5] add test --- .../src/views/CRUD/chart/ChartList.tsx | 19 +++++++++---------- tests/integration_tests/core_tests.py | 12 ++++++++++++ 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/superset-frontend/src/views/CRUD/chart/ChartList.tsx b/superset-frontend/src/views/CRUD/chart/ChartList.tsx index d66cb4fe0ac69..cb56243294983 100644 --- a/superset-frontend/src/views/CRUD/chart/ChartList.tsx +++ b/superset-frontend/src/views/CRUD/chart/ChartList.tsx @@ -63,6 +63,7 @@ import InfoTooltip from 'src/components/InfoTooltip'; import CertifiedBadge from 'src/components/CertifiedBadge'; import { GenericLink } from 'src/components/GenericLink/GenericLink'; import { bootstrapData } from 'src/preamble'; +import Owner from 'src/types/Owner'; import ChartCard from './ChartCard'; const FlexRowContainer = styled.div` @@ -223,6 +224,12 @@ function ChartList(props: ChartListProps) { }); setPreparingExport(true); }; + const changedByName = (lastSavedBy: Owner) => { + // eslint-disable-next-line no-unused-expressions + return lastSavedBy?.first_name + ? `${lastSavedBy?.first_name} ${lastSavedBy?.last_name}` + : null; + }; function handleBulkChartDelete(chartsToDelete: Chart[]) { SupersetClient.delete({ @@ -329,17 +336,9 @@ function ChartList(props: ChartListProps) { }, }: any) => enableBroadUserAccess ? ( - - {lastSavedBy?.first_name - ? `${lastSavedBy?.first_name} ${lastSavedBy?.last_name}` - : null} - + {changedByName(lastSavedBy)} ) : ( - <> - {lastSavedBy?.first_name - ? `${lastSavedBy?.first_name} ${lastSavedBy?.last_name}` - : null} - + <>{changedByName(lastSavedBy)} ), Header: t('Modified by'), accessor: 'last_saved_by.first_name', diff --git a/tests/integration_tests/core_tests.py b/tests/integration_tests/core_tests.py index 3d92d7c0e05e2..8a58b7f1591d8 100644 --- a/tests/integration_tests/core_tests.py +++ b/tests/integration_tests/core_tests.py @@ -894,6 +894,18 @@ def test_user_profile(self, username="admin"): data = self.get_json_resp(endpoint) self.assertNotIn("message", data) + def test_user_profile_optional_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 + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_user_activity_access(self, username="gamma"): self.login(username=username) From 0648ae13d513fa30a37894d491ecd0740189a2b2 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Fri, 2 Sep 2022 14:59:24 +0100 Subject: [PATCH 4/5] fix lint --- superset-frontend/src/views/CRUD/chart/ChartList.tsx | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/superset-frontend/src/views/CRUD/chart/ChartList.tsx b/superset-frontend/src/views/CRUD/chart/ChartList.tsx index cb56243294983..7440d11ac60da 100644 --- a/superset-frontend/src/views/CRUD/chart/ChartList.tsx +++ b/superset-frontend/src/views/CRUD/chart/ChartList.tsx @@ -224,12 +224,10 @@ function ChartList(props: ChartListProps) { }); setPreparingExport(true); }; - const changedByName = (lastSavedBy: Owner) => { - // eslint-disable-next-line no-unused-expressions - return lastSavedBy?.first_name + const changedByName = (lastSavedBy: Owner) => + lastSavedBy?.first_name ? `${lastSavedBy?.first_name} ${lastSavedBy?.last_name}` : null; - }; function handleBulkChartDelete(chartsToDelete: Chart[]) { SupersetClient.delete({ From 9c8d88a9e7aac88ff48ae1ccc8cdd4930610353c Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 5 Sep 2022 11:54:52 +0100 Subject: [PATCH 5/5] address comment --- superset-frontend/src/views/CRUD/chart/ChartList.tsx | 2 +- superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/superset-frontend/src/views/CRUD/chart/ChartList.tsx b/superset-frontend/src/views/CRUD/chart/ChartList.tsx index 7440d11ac60da..8fbf37392f870 100644 --- a/superset-frontend/src/views/CRUD/chart/ChartList.tsx +++ b/superset-frontend/src/views/CRUD/chart/ChartList.tsx @@ -216,7 +216,7 @@ function ChartList(props: ChartListProps) { 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 || false; + bootstrapData?.common?.conf?.ENABLE_BROAD_ACTIVITY_ACCESS; const handleBulkChartExport = (chartsToExport: Chart[]) => { const ids = chartsToExport.map(({ id }) => id); handleResourceExport('chart', ids, () => { diff --git a/superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx b/superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx index f38f733fb1572..8569f840d3688 100644 --- a/superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx +++ b/superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx @@ -134,7 +134,7 @@ function DashboardList(props: DashboardListProps) { const [passwordFields, setPasswordFields] = useState([]); const [preparingExport, setPreparingExport] = useState(false); const enableBroadUserAccess = - bootstrapData?.common?.conf?.ENABLE_BROAD_ACTIVITY_ACCESS || false; + bootstrapData?.common?.conf?.ENABLE_BROAD_ACTIVITY_ACCESS; const openDashboardImportModal = () => { showImportModal(true);