improve sql lab role and conditionally show saved queries on welcome

apache · Jan 4, 2023 · b91afd6 · b91afd6
1 parent fd99962
commit b91afd6
Show file tree

Hide file tree

Showing 6 changed files with 66 additions and 64 deletions.
diff --git a/superset-frontend/src/views/CRUD/welcome/ActivityTable.test.tsx b/superset-frontend/src/views/CRUD/welcome/ActivityTable.test.tsx
@@ -82,7 +82,7 @@ describe('ActivityTable', () => {
     activityData: mockData,
     setActiveChild: jest.fn(),
     user: { userId: '1' },
-    loadedCount: 3,
+    doneFetching: true,
   };
 
   let wrapper: ReactWrapper;
@@ -127,7 +127,7 @@ describe('ActivityTable', () => {
       activityData: {},
       setActiveChild: jest.fn(),
       user: { userId: '1' },
-      loadedCount: 3,
+      doneFetching: true,
     };
     const wrapper = mount(
       <Provider store={store}>

diff --git a/superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx b/superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
@@ -74,7 +74,7 @@ interface ActivityProps {
   activeChild: string;
   setActiveChild: (arg0: string) => void;
   activityData: ActivityData;
-  loadedCount: number;
+  doneFetching: boolean;
 }
 
 const Styles = styled.div`
@@ -128,7 +128,7 @@ export default function ActivityTable({
   setActiveChild,
   activityData,
   user,
-  loadedCount,
+  doneFetching,
 }: ActivityProps) {
   const [editedObjs, setEditedObjs] = useState<Array<ActivityData>>();
   const [loadingState, setLoadingState] = useState(false);
@@ -200,9 +200,7 @@ export default function ActivityTable({
       );
     });
 
-  const doneFetching = loadedCount < 3;
-
-  if ((loadingState && !editedObjs) || doneFetching) {
+  if ((loadingState && !editedObjs) || !doneFetching) {
     return <LoadingCards />;
   }
   return (

diff --git a/superset-frontend/src/views/CRUD/welcome/Welcome.tsx b/superset-frontend/src/views/CRUD/welcome/Welcome.tsx
@@ -47,6 +47,7 @@ import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags';
 import { AntdSwitch } from 'src/components';
 import getBootstrapData from 'src/utils/getBootstrapData';
 import { TableTab } from 'src/views/CRUD/types';
+import { canUserAccessSqlLab } from 'src/dashboard/util/permissionUtils';
 import { WelcomePageLastTab } from './types';
 import ActivityTable from './ActivityTable';
 import ChartTable from './ChartTable';
@@ -161,6 +162,7 @@ export const LoadingCards = ({ cover }: LoadingProps) => (
 );
 
 function Welcome({ user, addDangerToast }: WelcomeProps) {
+  const canAccessSqlLab = canUserAccessSqlLab(user);
   const userid = user.userId;
   const id = userid!.toString(); // confident that user is not a guest user
   const recent = `/superset/recent_activity/${user.userId}/?limit=6`;
@@ -179,6 +181,7 @@ function Welcome({ user, addDangerToast }: WelcomeProps) {
     null,
   );
   const [loadedCount, setLoadedCount] = useState(0);
+  const doneFetching = loadedCount === (canAccessSqlLab ? 3 : 2);
 
   const collapseState = getItem(LocalStorageKeys.homepage_collapse_state, []);
   const [activeState, setActiveState] = useState<Array<string>>(collapseState);
@@ -282,18 +285,20 @@ function Welcome({ user, addDangerToast }: WelcomeProps) {
         setLoadedCount(loadedCount => loadedCount + 1);
         addDangerToast(t('There was an issue fetching your chart: %s', err));
       });
-    getUserOwnedObjects(id, 'saved_query', ownSavedQueryFilters)
-      .then(r => {
-        setQueryData(r);
-        setLoadedCount(loadedCount => loadedCount + 1);
-      })
-      .catch((err: unknown) => {
-        setQueryData([]);
-        setLoadedCount(loadedCount => loadedCount + 1);
-        addDangerToast(
-          t('There was an issues fetching your saved queries: %s', err),
-        );
-      });
+    if (canAccessSqlLab) {
+      getUserOwnedObjects(id, 'saved_query', ownSavedQueryFilters)
+        .then(r => {
+          setQueryData(r);
+          setLoadedCount(loadedCount => loadedCount + 1);
+        })
+        .catch((err: unknown) => {
+          setQueryData([]);
+          setLoadedCount(loadedCount => loadedCount + 1);
+          addDangerToast(
+            t('There was an issue fetching your saved queries: %s', err),
+          );
+        });
+    }
   }, [otherTabFilters]);
 
   const handleToggle = () => {
@@ -323,6 +328,7 @@ function Welcome({ user, addDangerToast }: WelcomeProps) {
 
   const isRecentActivityLoading =
     !activityData?.[TableTab.Other] && !activityData?.[TableTab.Viewed];
+
   return (
     <WelcomeContainer>
       {WelcomeMessageExtension && <WelcomeMessageExtension />}
@@ -356,7 +362,7 @@ function Welcome({ user, addDangerToast }: WelcomeProps) {
                   activeChild={activeChild}
                   setActiveChild={setActiveChild}
                   activityData={activityData}
-                  loadedCount={loadedCount}
+                  doneFetching={doneFetching}
                 />
               ) : (
                 <LoadingCards />
@@ -390,18 +396,20 @@ function Welcome({ user, addDangerToast }: WelcomeProps) {
                 />
               )}
             </Collapse.Panel>
-            <Collapse.Panel header={t('Saved queries')} key="4">
-              {!queryData ? (
-                <LoadingCards cover={checked} />
-              ) : (
-                <SavedQueries
-                  showThumbnails={checked}
-                  user={user}
-                  mine={queryData}
-                  featureFlag={isFeatureEnabled(FeatureFlag.THUMBNAILS)}
-                />
-              )}
-            </Collapse.Panel>
+            {canAccessSqlLab && (
+              <Collapse.Panel header={t('Saved queries')} key="4">
+                {!queryData ? (
+                  <LoadingCards cover={checked} />
+                ) : (
+                  <SavedQueries
+                    showThumbnails={checked}
+                    user={user}
+                    mine={queryData}
+                    featureFlag={isFeatureEnabled(FeatureFlag.THUMBNAILS)}
+                  />
+                )}
+              </Collapse.Panel>
+            )}
           </Collapse>
         </>
       )}

diff --git a/superset/errors.py b/superset/errors.py
@@ -65,7 +65,6 @@ class SupersetErrorType(str, Enum):
     QUERY_SECURITY_ACCESS_ERROR = "QUERY_SECURITY_ACCESS_ERROR"
     MISSING_OWNERSHIP_ERROR = "MISSING_OWNERSHIP_ERROR"
     USER_ACTIVITY_SECURITY_ACCESS_ERROR = "USER_ACTIVITY_SECURITY_ACCESS_ERROR"
-    SQL_LAB_SECURITY_ACCESS_ERROR = "SQL_LAB_SECURITY_ACCESS_ERROR"
 
     # Other errors
     BACKEND_TIMEOUT_ERROR = "BACKEND_TIMEOUT_ERROR"

diff --git a/superset/security/manager.py b/superset/security/manager.py
@@ -189,7 +189,6 @@ class SupersetSecurityManager(  # pylint: disable=too-many-public-methods
     }
 
     ADMIN_ONLY_PERMISSIONS = {
-        "can_sql_json",  # TODO: move can_sql_json to sql_lab role
         "can_override_role_permissions",
         "can_sync_druid_source",
         "can_override_role_permissions",
@@ -223,11 +222,11 @@ class SupersetSecurityManager(  # pylint: disable=too-many-public-methods
 
     ACCESSIBLE_PERMS = {"can_userinfo", "resetmypassword"}
 
-    SQLLAB_PERMISSION_VIEWS = {
-        ("can_csv", "Superset"),
+    SQLLAB_ONLY_PERMISSIONS = {
+        ("can_my_queries", "SqlLab"),
         ("can_read", "SavedQuery"),
-        ("can_read", "Database"),
         ("can_sql_json", "Superset"),
+        ("can_sqllab_history", "Superset"),
         ("can_sqllab_viz", "Superset"),
         ("can_sqllab_table_viz", "Superset"),
         ("can_sqllab", "Superset"),
@@ -237,6 +236,12 @@ class SupersetSecurityManager(  # pylint: disable=too-many-public-methods
         ("menu_access", "Query Search"),
     }
 
+    SQLLAB_EXTRA_PERMISSION_VIEWS = {
+        ("can_csv", "Superset"),
+        ("can_read", "Superset"),
+        ("can_read", "Database"),
+    }
+
     data_access_permissions = (
         "database_access",
         "schema_access",
@@ -908,7 +913,9 @@ def _is_alpha_pvm(self, pvm: PermissionView) -> bool:
         """
 
         return not (
-            self._is_user_defined_permission(pvm) or self._is_admin_only(pvm)
+            self._is_user_defined_permission(pvm)
+            or self._is_admin_only(pvm)
+            or self._is_sql_lab_only(pvm)
         ) or self._is_accessible_to_all(pvm)
 
     def _is_gamma_pvm(self, pvm: PermissionView) -> bool:
@@ -924,8 +931,19 @@ def _is_gamma_pvm(self, pvm: PermissionView) -> bool:
             self._is_user_defined_permission(pvm)
             or self._is_admin_only(pvm)
             or self._is_alpha_only(pvm)
+            or self._is_sql_lab_only(pvm)
         ) or self._is_accessible_to_all(pvm)
 
+    def _is_sql_lab_only(self, pvm: PermissionView) -> bool:
+        """
+        Return True if the FAB permission/view is only SQL Lab related, False
+        otherwise.
+
+        :param pvm: The FAB permission/view
+        :returns: Whether the FAB object is SQL Lab related
+        """
+        return (pvm.permission.name, pvm.view_menu.name) in self.SQLLAB_ONLY_PERMISSIONS
+
     def _is_sql_lab_pvm(self, pvm: PermissionView) -> bool:
         """
         Return True if the FAB permission/view is SQL Lab related, False
@@ -934,7 +952,11 @@ def _is_sql_lab_pvm(self, pvm: PermissionView) -> bool:
         :param pvm: The FAB permission/view
         :returns: Whether the FAB object is SQL Lab related
         """
-        return (pvm.permission.name, pvm.view_menu.name) in self.SQLLAB_PERMISSION_VIEWS
+        return (
+            self._is_sql_lab_only(pvm)
+            or (pvm.permission.name, pvm.view_menu.name)
+            in self.SQLLAB_EXTRA_PERMISSION_VIEWS
+        )
 
     def _is_granter_pvm(  # pylint: disable=no-self-use
         self, pvm: PermissionView
@@ -1964,19 +1986,6 @@ def raise_for_user_activity_access(user_id: int) -> None:
                 )
             )
 
-    def raise_for_sql_lab_access(self) -> None:
-        if not (
-            self.is_admin()
-            or "sql_lab" in (role.name for role in self.get_user_roles())
-        ):
-            raise SupersetSecurityException(
-                SupersetError(
-                    error_type=SupersetErrorType.SQL_LAB_SECURITY_ACCESS_ERROR,
-                    message="You do not have access to SQL Lab",
-                    level=ErrorLevel.ERROR,
-                )
-            )
-
     def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None:
         """
         Raise an exception if the user cannot access the dashboard.

diff --git a/superset/views/core.py b/superset/views/core.py
@@ -2777,12 +2777,6 @@ def _get_sqllab_tabs(user_id: Optional[int]) -> Dict[str, Any]:
     @expose("/sqllab/", methods=["GET", "POST"])
     def sqllab(self) -> FlaskResponse:
         """SQL Editor"""
-        try:
-            security_manager.raise_for_sql_lab_access()
-        except SupersetSecurityException as ex:
-            flash(ex.error.message, "danger")
-            return redirect("/")
-
         payload = {
             "defaultDbId": config["SQLLAB_DEFAULT_DBID"],
             "common": common_bootstrap_payload(g.user),
@@ -2810,12 +2804,6 @@ def sqllab(self) -> FlaskResponse:
     @expose("/sqllab/history/", methods=["GET"])
     @event_logger.log_this
     def sqllab_history(self) -> FlaskResponse:
-        try:
-            security_manager.raise_for_sql_lab_access()
-        except SupersetSecurityException as ex:
-            flash(ex.error.message, "danger")
-            return redirect("/")
-
         return super().render_app_template()
 
     @api