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 | 32 +++++++++- .../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, 141 insertions(+), 41 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 56dd8aa89ef2a..2533c1ffd0d44 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 d8de53c2e6b5a..b91a6c490aaad 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 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('glob:*/api/v1/mock', 500, { + 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 d354a32d6bab7..9d89991f6b7a0 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 aabda42d36bad..61b7266a453a1 100644 --- a/superset/config.py +++ b/superset/config.py @@ -1293,6 +1293,9 @@ def SQL_QUERY_MUTATOR( # pylint: disable=invalid-name,unused-argument 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 f186819dc4970..8dc4f90308f71 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 7ecb2e4522e2e..9e45e88afcda6 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1160,6 +1160,21 @@ def get_rls_ids(self, table: "BaseDatasource") -> List[int]: 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 e674146e7e595..9557c30e83d98 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -1391,14 +1391,26 @@ def testconn(self) -> FlaskResponse: # pylint: disable=no-self-use _("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 @@ def available_domains(self) -> FlaskResponse: # pylint: disable=no-self-use 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 @@ def fave_dashboards( # pylint: disable=no-self-use @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 @@ def created_dashboards( # pylint: disable=no-self-use @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 @@ def user_slices( # pylint: disable=no-self-use @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 @@ def created_slices( # pylint: disable=no-self-use @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 dd84f976c6df8..87dca2d7b5aa5 100644 --- a/tests/integration_tests/core_tests.py +++ b/tests/integration_tests/core_tests.py @@ -790,6 +790,19 @@ def test_fetch_datasource_metadata(self): 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 @@ def test_user_profile(self, username="admin"): 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):