Skip to content

Commit

Permalink
fix: disallow users from viewing other user's profile on config (#21302)
Browse files Browse the repository at this point in the history
  • Loading branch information
dpgaspar authored Sep 5, 2022
1 parent b71182f commit c3f8417
Show file tree
Hide file tree
Showing 6 changed files with 44 additions and 11 deletions.
22 changes: 14 additions & 8 deletions superset-frontend/src/views/CRUD/chart/ChartList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ 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 Owner from 'src/types/Owner';
import ChartCard from './ChartCard';

const FlexRowContainer = styled.div`
Expand Down Expand Up @@ -213,14 +215,19 @@ 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 handleBulkChartExport = (chartsToExport: Chart[]) => {
const ids = chartsToExport.map(({ id }) => id);
handleResourceExport('chart', ids, () => {
setPreparingExport(false);
});
setPreparingExport(true);
};
const changedByName = (lastSavedBy: Owner) =>
lastSavedBy?.first_name
? `${lastSavedBy?.first_name} ${lastSavedBy?.last_name}`
: null;

function handleBulkChartDelete(chartsToDelete: Chart[]) {
SupersetClient.delete({
Expand Down Expand Up @@ -325,13 +332,12 @@ function ChartList(props: ChartListProps) {
changed_by_url: changedByUrl,
},
},
}: any) => (
<a href={changedByUrl}>
{lastSavedBy?.first_name
? `${lastSavedBy?.first_name} ${lastSavedBy?.last_name}`
: null}
</a>
),
}: any) =>
enableBroadUserAccess ? (
<a href={changedByUrl}>{changedByName(lastSavedBy)}</a>
) : (
<>{changedByName(lastSavedBy)}</>
),
Header: t('Modified by'),
accessor: 'last_saved_by.first_name',
size: 'xl',
Expand Down
10 changes: 9 additions & 1 deletion superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -132,6 +133,8 @@ 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 openDashboardImportModal = () => {
showImportModal(true);
Expand Down Expand Up @@ -290,7 +293,12 @@ function DashboardList(props: DashboardListProps) {
changed_by_url: changedByUrl,
},
},
}: any) => <a href={changedByUrl}>{changedByName}</a>,
}: any) =>
enableBroadUserAccess ? (
<a href={changedByUrl}>{changedByName}</a>
) : (
<>{changedByName}</>
),
Header: t('Modified by'),
accessor: 'changed_by.first_name',
size: 'xl',
Expand Down
1 change: 1 addition & 0 deletions superset/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions superset/views/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
9 changes: 7 additions & 2 deletions superset/views/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -2749,8 +2749,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),
Expand Down
12 changes: 12 additions & 0 deletions tests/integration_tests/core_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit c3f8417

Please sign in to comment.