From c4b04952d0e446b2347d2e6928478e2207102567 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt <33317356+villebro@users.noreply.github.com> Date: Wed, 8 Dec 2021 11:30:23 +0200 Subject: [PATCH] feat: customize recent activity access (#17589) * feat: customize recent activity access * address comments * fix and add tests * add alert assertion and UPDATING.md entry * replace .get_id() with .id * fix updating comment * update config name --- UPDATING.md | 1 + .../TableLoader/TableLoader.test.tsx | 34 ++++++++++- .../src/components/TableLoader/index.tsx | 17 +++++- superset/config.py | 3 + superset/errors.py | 1 + superset/security/manager.py | 15 +++++ superset/views/core.py | 59 ++++++++++++------- tests/integration_tests/core_tests.py | 54 ++++++++++++----- 8 files changed, 142 insertions(+), 42 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 56dd8aa89..2533c1ffd 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -43,6 +43,7 @@ assists people when migrating to a new version. ### Other +- [17589](https://github.com/apache/incubator-superset/pull/17589): It is now possible to limit access to users' recent activity data by setting the `ENABLE_BROAD_ACTIVITY_ACCESS` config flag to false, or customizing the `raise_for_user_activity_access` method in the security manager. - [16809](https://github.com/apache/incubator-superset/pull/16809): When building the superset frontend assets manually, you should now use Node 16 (previously Node 14 was required/recommended). Node 14 will most likely still work for at least some time, but is no longer actively tested for on CI. - [17536](https://github.com/apache/superset/pull/17536): introduced a key-value endpoint to store dashboard filter state. This endpoint is backed by Flask-Caching and the default configuration assumes that the values will be stored in the file system. If you are already using another cache backend like Redis or Memchached, you'll probably want to change this setting in `superset_config.py`. The key is `FILTER_STATE_CACHE_CONFIG` and the available settings can be found in Flask-Caching [docs](https://flask-caching.readthedocs.io/en/latest/). diff --git a/superset-frontend/src/components/TableLoader/TableLoader.test.tsx b/superset-frontend/src/components/TableLoader/TableLoader.test.tsx index d8de53c2e..b91a6c490 100644 --- a/superset-frontend/src/components/TableLoader/TableLoader.test.tsx +++ b/superset-frontend/src/components/TableLoader/TableLoader.test.tsx @@ -24,7 +24,10 @@ import { storeWithState } from 'spec/fixtures/mockStore'; import ToastContainer from 'src/components/MessageToasts/ToastContainer'; import TableLoader, { TableLoaderProps } from '.'; -fetchMock.get('glob:*/api/v1/mock', [ +const NO_DATA_TEXT = 'No data available'; +const MOCK_GLOB = 'glob:*/api/v1/mock'; + +fetchMock.get(MOCK_GLOB, [ { id: 1, name: 'John Doe' }, { id: 2, name: 'Jane Doe' }, ]); @@ -32,6 +35,7 @@ fetchMock.get('glob:*/api/v1/mock', [ const defaultProps: TableLoaderProps = { dataEndpoint: '/api/v1/mock', addDangerToast: jest.fn(), + noDataText: NO_DATA_TEXT, }; function renderWithProps(props: TableLoaderProps = defaultProps) { @@ -83,12 +87,36 @@ test('renders with mutator', async () => { expect(await screen.findAllByRole('heading', { level: 4 })).toHaveLength(2); }); -test('renders error message', async () => { - fetchMock.mock('glob:*/api/v1/mock', 500, { +test('renders empty message', async () => { + fetchMock.mock(MOCK_GLOB, [], { overwriteRoutes: true, }); renderWithProps(); + expect(await screen.findByText('No data available')).toBeInTheDocument(); +}); + +test('renders blocked message', async () => { + fetchMock.mock(MOCK_GLOB, 403, { + overwriteRoutes: true, + }); + + renderWithProps(); + + expect( + await screen.findByText('Access to user activity data is restricted'), + ).toBeInTheDocument(); + expect(screen.queryByRole('alert')).not.toBeInTheDocument(); +}); + +test('renders error message', async () => { + fetchMock.mock(MOCK_GLOB, 500, { + overwriteRoutes: true, + }); + + renderWithProps(); + + expect(await screen.findByText(NO_DATA_TEXT)).toBeInTheDocument(); expect(await screen.findByRole('alert')).toBeInTheDocument(); }); diff --git a/superset-frontend/src/components/TableLoader/index.tsx b/superset-frontend/src/components/TableLoader/index.tsx index d354a32d6..9d89991f6 100644 --- a/superset-frontend/src/components/TableLoader/index.tsx +++ b/superset-frontend/src/components/TableLoader/index.tsx @@ -27,12 +27,14 @@ export interface TableLoaderProps { dataEndpoint?: string; mutator?: (data: JsonObject) => any[]; columns?: string[]; + noDataText?: string; addDangerToast(text: string): any; } const TableLoader = (props: TableLoaderProps) => { const [data, setData] = useState>([]); const [isLoading, setIsLoading] = useState(true); + const [isBlocked, setIsBlocked] = useState(false); useEffect(() => { const { dataEndpoint, mutator } = props; @@ -41,16 +43,22 @@ const TableLoader = (props: TableLoaderProps) => { .then(({ json }) => { const data = (mutator ? mutator(json) : json) as Array; setData(data); + setIsBlocked(false); setIsLoading(false); }) - .catch(() => { + .catch(response => { setIsLoading(false); - props.addDangerToast(t('An error occurred')); + if (response.status === 403) { + setIsBlocked(true); + } else { + setIsBlocked(false); + props.addDangerToast(t('An error occurred')); + } }); } }, [props]); - const { columns, ...tableProps } = props; + const { columns, noDataText, ...tableProps } = props; const memoizedColumns = useMemo(() => { let tableColumns = columns; @@ -79,6 +87,9 @@ const TableLoader = (props: TableLoaderProps) => { pageSize={50} loading={isLoading} emptyWrapperType={EmptyWrapperType.Small} + noDataText={ + isBlocked ? t('Access to user activity data is restricted') : noDataText + } {...tableProps} /> ); diff --git a/superset/config.py b/superset/config.py index aabda42d3..61b7266a4 100644 --- a/superset/config.py +++ b/superset/config.py @@ -1293,6 +1293,9 @@ MENU_HIDE_USER_INFO = False SQLALCHEMY_DOCS_URL = "https://docs.sqlalchemy.org/en/13/core/engines.html" SQLALCHEMY_DISPLAY_TEXT = "SQLAlchemy docs" +# Set to False to only allow viewing own recent activity +ENABLE_BROAD_ACTIVITY_ACCESS = True + # ------------------------------------------------------------------- # * WARNING: STOP EDITING HERE * # ------------------------------------------------------------------- diff --git a/superset/errors.py b/superset/errors.py index f186819dc..8dc4f9030 100644 --- a/superset/errors.py +++ b/superset/errors.py @@ -63,6 +63,7 @@ class SupersetErrorType(str, Enum): DATABASE_SECURITY_ACCESS_ERROR = "DATABASE_SECURITY_ACCESS_ERROR" QUERY_SECURITY_ACCESS_ERROR = "QUERY_SECURITY_ACCESS_ERROR" MISSING_OWNERSHIP_ERROR = "MISSING_OWNERSHIP_ERROR" + USER_ACTIVITY_SECURITY_ACCESS_ERROR = "USER_ACTIVITY_SECURITY_ACCESS_ERROR" # Other errors BACKEND_TIMEOUT_ERROR = "BACKEND_TIMEOUT_ERROR" diff --git a/superset/security/manager.py b/superset/security/manager.py index 7ecb2e452..9e45e88af 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1160,6 +1160,21 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods ids.sort() # Combinations rather than permutations return ids + @staticmethod + def raise_for_user_activity_access(user_id: int) -> None: + user = g.user if g.user and g.user.get_id() else None + if not user or ( + not current_app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] + and user_id != user.id + ): + raise SupersetSecurityException( + SupersetError( + error_type=SupersetErrorType.USER_ACTIVITY_SECURITY_ACCESS_ERROR, + message="Access to user's activity data is restricted", + level=ErrorLevel.ERROR, + ) + ) + @staticmethod def raise_for_dashboard_access(dashboard: "Dashboard") -> None: """ diff --git a/superset/views/core.py b/superset/views/core.py index e674146e7..9557c30e8 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -1391,14 +1391,26 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods _("Unexpected error occurred, please check your logs for details"), 400 ) + @staticmethod + def get_user_activity_access_error(user_id: int) -> Optional[FlaskResponse]: + try: + security_manager.raise_for_user_activity_access(user_id) + except SupersetSecurityException as ex: + return json_error_response(ex.message, status=403,) + return None + @api @has_access_api @event_logger.log_this @expose("/recent_activity//", methods=["GET"]) - def recent_activity( # pylint: disable=no-self-use + def recent_activity( # pylint: disable=too-many-locals self, user_id: int ) -> FlaskResponse: """Recent activity (actions) for a given user""" + error_obj = self.get_user_activity_access_error(user_id) + if error_obj: + return error_obj + limit = request.args.get("limit") limit = int(limit) if limit and limit.isdigit() else 100 actions = request.args.get("actions", "explore,dashboard").split(",") @@ -1526,15 +1538,16 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods def fave_dashboards_by_username(self, username: str) -> FlaskResponse: """This lets us use a user's username to pull favourite dashboards""" user = security_manager.find_user(username=username) - return self.fave_dashboards(user.get_id()) + return self.fave_dashboards(user.id) @api @has_access_api @event_logger.log_this @expose("/fave_dashboards//", methods=["GET"]) - def fave_dashboards( # pylint: disable=no-self-use - self, user_id: int - ) -> FlaskResponse: + def fave_dashboards(self, user_id: int) -> FlaskResponse: + error_obj = self.get_user_activity_access_error(user_id) + if error_obj: + return error_obj qry = ( db.session.query(Dashboard, FavStar.dttm) .join( @@ -1567,9 +1580,10 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods @has_access_api @event_logger.log_this @expose("/created_dashboards//", methods=["GET"]) - def created_dashboards( # pylint: disable=no-self-use - self, user_id: int - ) -> FlaskResponse: + def created_dashboards(self, user_id: int) -> FlaskResponse: + error_obj = self.get_user_activity_access_error(user_id) + if error_obj: + return error_obj Dash = Dashboard qry = ( db.session.query(Dash) @@ -1595,12 +1609,13 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods @event_logger.log_this @expose("/user_slices", methods=["GET"]) @expose("/user_slices//", methods=["GET"]) - def user_slices( # pylint: disable=no-self-use - self, user_id: Optional[int] = None - ) -> FlaskResponse: + def user_slices(self, user_id: Optional[int] = None) -> FlaskResponse: """List of slices a user owns, created, modified or faved""" if not user_id: - user_id = g.user.get_id() + user_id = cast(int, g.user.id) + error_obj = self.get_user_activity_access_error(user_id) + if error_obj: + return error_obj owner_ids_query = ( db.session.query(Slice.id) @@ -1647,12 +1662,13 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods @event_logger.log_this @expose("/created_slices", methods=["GET"]) @expose("/created_slices//", methods=["GET"]) - def created_slices( # pylint: disable=no-self-use - self, user_id: Optional[int] = None - ) -> FlaskResponse: + def created_slices(self, user_id: Optional[int] = None) -> FlaskResponse: """List of slices created by this user""" if not user_id: - user_id = g.user.get_id() + user_id = cast(int, g.user.id) + error_obj = self.get_user_activity_access_error(user_id) + if error_obj: + return error_obj qry = ( db.session.query(Slice) .filter( # pylint: disable=comparison-with-callable @@ -1677,12 +1693,13 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods @event_logger.log_this @expose("/fave_slices", methods=["GET"]) @expose("/fave_slices//", methods=["GET"]) - def fave_slices( # pylint: disable=no-self-use - self, user_id: Optional[int] = None - ) -> FlaskResponse: + def fave_slices(self, user_id: Optional[int] = None) -> FlaskResponse: """Favorite slices for a user""" - if not user_id: - user_id = g.user.get_id() + if user_id is None: + user_id = g.user.id + error_obj = self.get_user_activity_access_error(user_id) + if error_obj: + return error_obj qry = ( db.session.query(Slice, FavStar.dttm) .join( diff --git a/tests/integration_tests/core_tests.py b/tests/integration_tests/core_tests.py index dd84f976c..87dca2d7b 100644 --- a/tests/integration_tests/core_tests.py +++ b/tests/integration_tests/core_tests.py @@ -790,6 +790,19 @@ class TestCore(SupersetTestCase): for k in keys: self.assertIn(k, resp.keys()) + @staticmethod + def _get_user_activity_endpoints(user: str): + userid = security_manager.find_user(user).id + return ( + f"/superset/recent_activity/{userid}/", + f"/superset/created_slices/{userid}/", + f"/superset/created_dashboards/{userid}/", + f"/superset/fave_slices/{userid}/", + f"/superset/fave_dashboards/{userid}/", + f"/superset/user_slices/{userid}/", + f"/superset/fave_dashboards_by_username/{user}/", + ) + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_user_profile(self, username="admin"): self.login(username=username) @@ -805,23 +818,34 @@ class TestCore(SupersetTestCase): resp = self.get_json_resp(url) self.assertEqual(resp["count"], 1) - userid = security_manager.find_user("admin").id resp = self.get_resp(f"/superset/profile/{username}/") self.assertIn('"app"', resp) - data = self.get_json_resp(f"/superset/recent_activity/{userid}/") - self.assertNotIn("message", data) - data = self.get_json_resp(f"/superset/created_slices/{userid}/") - self.assertNotIn("message", data) - data = self.get_json_resp(f"/superset/created_dashboards/{userid}/") - self.assertNotIn("message", data) - data = self.get_json_resp(f"/superset/fave_slices/{userid}/") - self.assertNotIn("message", data) - data = self.get_json_resp(f"/superset/fave_dashboards/{userid}/") - self.assertNotIn("message", data) - data = self.get_json_resp(f"/superset/user_slices/{userid}/") - self.assertNotIn("message", data) - data = self.get_json_resp(f"/superset/fave_dashboards_by_username/{username}/") - self.assertNotIn("message", data) + + for endpoint in self._get_user_activity_endpoints(username): + data = self.get_json_resp(endpoint) + self.assertNotIn("message", data) + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_user_activity_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 + + # 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 @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_slice_id_is_always_logged_correctly_on_web_request(self):