Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: disallow users from viewing other user's profile on config #21302

Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 || false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably better safe than sorry, but in a similar context on the backend we always do config["ENABLE_BROAD_ACTIVITY_ACCESS"] instead of ``config.get("ENABLE_BROAD_ACTIVITY_ACCESS")`, since we assume the variable should always be defined. So maybe we could get by with

Suggested change
bootstrapData?.common?.conf?.ENABLE_BROAD_ACTIVITY_ACCESS || false;
bootstrapData?.common?.conf?.ENABLE_BROAD_ACTIVITY_ACCESS;

(arguably that whole bootstrapData object with all nested fields should also be fully available)

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 || false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here


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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code we are just removing the links to the profiles. Is there anything else that is actually blocking the user to visit a profile?

Copy link
Member Author

@dpgaspar dpgaspar Sep 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, on the backend change, the call to get_user_activity_access_error will assert if ENABLE_BROAD_ACTIVITY_ACCESS is True or not, if it's False and the user_id is not equal to the current user, the response will be 403 with an error msg. Following same pattern already in-place on other endpoints on /superset.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks for adding that

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 @@ -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),
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