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

@@ -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/).
 

superset-frontend/src/components/TableLoader/TableLoader.test.tsx

@@ -24,14 +24,18 @@ 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' },
 ]);
 
 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();
 });

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<Array<any>>([]);
   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<any>;
           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}
     />
   );

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                    *
 # -------------------------------------------------------------------

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"

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:
         """

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/<int:user_id>/", 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/<int:user_id>/", 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/<int:user_id>/", 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/<int:user_id>/", 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/<int:user_id>/", 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/<int:user_id>/", 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(

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):