fix(sqllab): remove link to sqllab if missing perms #22566

villebro · 2023-01-02T13:15:07Z

SUMMARY

Currently the "View in SQL Lab" option is available in the Dataset menu in Explore view even when the user doesn't belong to the sql_lab role. In addition, SQL Lab is accessible (albeit doesn't work properly) when the necessary permissions are missing.

This PR does the following:

Removes the "View in SQL Lab" option from the Dataset menu
Redirects the user to the welcome page when trying to access SQL Lab without necessary permissions.
Remove "Saved Queries" from Welcome page if user is missing sql_lab role

AFTER

Now when a user that doesn't have the sql_lab role will no longer see the "View in SQL Lab" menu item:

For admin and users with sql_lab role the menu is unchanged:

When attempting to access SQL Lab without correct perms via a direct URL, the user gets redirected to the welcome page with an error toast. Also, the welcome page doesn't show the "Saved Queries" section if the user doesn't belong to the sql_lab role (we could optionally check the specific perms for saved queries, but maybe we can do that in a follow up if that's really needed):

Note that due to the way we're currently caching bootstrap data, toasts don't always appear correctly (they can appear delayed or not get cleared after displaying). I will follow up separately to fix this.

TESTING INSTRUCTIONS

For testing instructions, please see the following comment in this PR: #22566 (comment)

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2023-01-03T08:41:54Z

Codecov Report

Merging #22566 (e317744) into master (a7a4561) will increase coverage by 0.10%.
The diff coverage is 64.28%.

@@            Coverage Diff             @@
##           master   #22566      +/-   ##
==========================================
+ Coverage   66.91%   67.02%   +0.10%     
==========================================
  Files        1851     1859       +8     
  Lines       70709    71046     +337     
  Branches     7766     7771       +5     
==========================================
+ Hits        47316    47619     +303     
- Misses      21371    21404      +33     
- Partials     2022     2023       +1

Flag	Coverage Δ
hive	`52.61% <100.00%> (+0.14%)`	⬆️
javascript	`53.86% <59.45%> (+<0.01%)`	⬆️
mysql	`78.02% <100.00%> (+0.06%)`	⬆️
postgres	`78.09% <100.00%> (+0.06%)`	⬆️
presto	`52.51% <100.00%> (+0.15%)`	⬆️
python	`81.35% <100.00%> (+0.08%)`	⬆️
sqlite	`76.47% <100.00%> (-0.03%)`	⬇️
unit	`51.47% <60.00%> (+0.28%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../src/views/CRUD/data/savedquery/SavedQueryList.tsx	`63.30% <ø> (ø)`
...-frontend/src/views/CRUD/welcome/ActivityTable.tsx	`41.81% <28.57%> (-0.29%)`	⬇️
...perset-frontend/src/views/CRUD/welcome/Welcome.tsx	`71.18% <59.09%> (+0.49%)`	⬆️
...set-frontend/src/dashboard/util/permissionUtils.ts	`86.66% <80.00%> (-3.34%)`	⬇️
...re/components/controls/DatasourceControl/index.jsx	`84.90% <100.00%> (+0.29%)`	⬆️
superset/security/manager.py	`95.77% <100.00%> (+0.01%)`	⬆️
superset/dashboards/filter_state/commands/utils.py	`80.00% <0.00%> (-10.00%)`	⬇️
...uperset/dashboards/filter_state/commands/delete.py	`91.30% <0.00%> (-4.35%)`	⬇️
superset/databases/commands/update.py	`75.86% <0.00%> (-4.14%)`	⬇️
...uperset/dashboards/filter_state/commands/update.py	`96.66% <0.00%> (-3.34%)`	⬇️
... and 39 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

zhaoyongjie · 2023-01-03T09:29:02Z

/testenv up

github-actions · 2023-01-03T09:31:09Z

@zhaoyongjie Ephemeral environment spinning up at http://18.236.102.188:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

villebro · 2023-01-03T11:50:32Z

/testenv up

github-actions · 2023-01-03T11:55:22Z

@villebro Ephemeral environment spinning up at http://34.219.163.111:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

dpgaspar · 2023-01-03T12:14:58Z

superset/views/core.py

+            or "sql_lab" in (role.name for role in security_manager.get_user_roles())
+        ):
+            flash(__("You do not have access to SQL Lab"), "danger")
+            return redirect("/")


This probably makes more sense to fix on security manager, where all roles are created

Good idea, I'll move it over 👍

kgabryje

Frontend part looks great to me!

villebro · 2023-01-04T07:53:52Z

/testenv up

github-actions · 2023-01-04T07:58:13Z

@villebro Ephemeral environment spinning up at http://54.187.147.88:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

villebro · 2023-01-04T08:13:58Z

For testing I've created the following users on the eph env:

UID: gamma
PWD: gamma
Test case: directly accessing /superset/sqllab redirects to welcome page with an error toast and "View in SQL Lab" is missing in Dataset menu:

UID: sqlgamma
PWD: sqlgamma
Test case: SQL Lab works and "View in SQL Lab" is present in Dataset menu:

villebro · 2023-01-04T09:36:16Z

superset-frontend/src/dashboard/util/permissionUtils.test.ts

+import {
+  canUserAccessSqlLab,
+  canUserEditDashboard,
+  isUserAdmin,
+} from './permissionUtils';


TODO: these should probably be moved to a general util outside the dashboard section, but refactoring it seems outside the scope of this PR

villebro · 2023-01-05T18:13:43Z

tests/integration_tests/queries/saved_queries/api_tests.py

@@ -157,12 +157,12 @@ def test_get_list_saved_query_gamma(self):
        """
        Saved Query API: Test get list saved query
        """
-        gamma = self.get_user("gamma")
+        user = self.get_user("gamma_sqllab")


The gamma_sqllab test user has both Gamma and sql_lab roles.

did we had to change this user for the tests to pass?

villebro · 2023-01-06T07:56:07Z

/testenv up

github-actions · 2023-01-06T08:00:20Z

@villebro Ephemeral environment spinning up at http://35.89.82.98:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

villebro · 2023-01-06T08:22:12Z

FYI, I've recreated the ephemeral environment with the same test users as previously (same UID/PWD):

admin: the regular admin user
gamma: gamma user with access to Examples database
sqlgamma: gamma user with access to Examples database and sql_lab role

The users that have access to SQL Lab also have a saved query which one should be able to see from the Welcome page

zhaoyongjie

Test in the ephemeral env and it works as expected. Thanks for lots of refactor codes. LGTM.

dpgaspar · 2023-01-09T08:53:14Z

tests/integration_tests/queries/saved_queries/api_tests.py

@@ -157,12 +157,12 @@ def test_get_list_saved_query_gamma(self):
        """
        Saved Query API: Test get list saved query
        """
-        gamma = self.get_user("gamma")
+        user = self.get_user("gamma_sqllab")


did we had to change this user for the tests to pass?

kgabryje

Re-tested after latest changes, LGTM!

github-actions · 2023-01-09T12:03:00Z

Ephemeral environment shutdown and build artifacts deleted.

pull-request-size bot added the size/L label Jan 2, 2023

villebro force-pushed the villebro/sql-lab-links branch from 0acf6fd to f742039 Compare January 3, 2023 07:48

zhaoyongjie self-requested a review January 3, 2023 09:25

villebro added 5 commits January 3, 2023 10:56

fix(sqllab): remove link to sqllab if missing perms

e0759b9

add redirects

6bd8ca7

add test

0aba55f

replace bootstrap user with props user

d9ac91c

fix tests

03c73a2

villebro force-pushed the villebro/sql-lab-links branch from f742039 to 2a32627 Compare January 3, 2023 10:56

move redirect to backend

aaf3a71

villebro force-pushed the villebro/sql-lab-links branch from 2a32627 to aaf3a71 Compare January 3, 2023 11:02

remove redundant changes

c12ae9d

dpgaspar reviewed Jan 3, 2023

View reviewed changes

kgabryje approved these changes Jan 3, 2023

View reviewed changes

move access check to security manager

fd99962

villebro requested a review from dpgaspar January 4, 2023 07:53

villebro commented Jan 4, 2023

View reviewed changes

improve sql lab role and conditionally show saved queries on welcome

b91afd6

villebro force-pushed the villebro/sql-lab-links branch from 8a4cc4b to b91afd6 Compare January 4, 2023 13:23

villebro added 2 commits January 5, 2023 12:59

fix test

4859955

fix python test

318529e

villebro force-pushed the villebro/sql-lab-links branch from 4589668 to 318529e Compare January 5, 2023 17:39

villebro commented Jan 5, 2023

View reviewed changes

refactor welcome components

e317744

zhaoyongjie approved these changes Jan 6, 2023

View reviewed changes

dpgaspar approved these changes Jan 9, 2023

View reviewed changes

kgabryje approved these changes Jan 9, 2023

View reviewed changes

villebro merged commit 5b2ca97 into apache:master Jan 9, 2023

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sqllab): remove link to sqllab if missing perms #22566

fix(sqllab): remove link to sqllab if missing perms #22566

villebro commented Jan 2, 2023 •

edited

Loading

codecov bot commented Jan 3, 2023 •

edited

Loading

zhaoyongjie commented Jan 3, 2023

github-actions bot commented Jan 3, 2023

villebro commented Jan 3, 2023

github-actions bot commented Jan 3, 2023

dpgaspar Jan 3, 2023

villebro Jan 3, 2023

kgabryje left a comment

villebro commented Jan 4, 2023

github-actions bot commented Jan 4, 2023

villebro commented Jan 4, 2023 •

edited

Loading

villebro Jan 4, 2023

villebro Jan 5, 2023

dpgaspar Jan 9, 2023

villebro commented Jan 6, 2023

github-actions bot commented Jan 6, 2023

villebro commented Jan 6, 2023

zhaoyongjie left a comment

dpgaspar Jan 9, 2023

kgabryje left a comment

github-actions bot commented Jan 9, 2023

fix(sqllab): remove link to sqllab if missing perms #22566

fix(sqllab): remove link to sqllab if missing perms #22566

Conversation

villebro commented Jan 2, 2023 • edited Loading

SUMMARY

AFTER

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jan 3, 2023 • edited Loading

Codecov Report

zhaoyongjie commented Jan 3, 2023

github-actions bot commented Jan 3, 2023

villebro commented Jan 3, 2023

github-actions bot commented Jan 3, 2023

dpgaspar Jan 3, 2023

Choose a reason for hiding this comment

villebro Jan 3, 2023

Choose a reason for hiding this comment

kgabryje left a comment

Choose a reason for hiding this comment

villebro commented Jan 4, 2023

github-actions bot commented Jan 4, 2023

villebro commented Jan 4, 2023 • edited Loading

villebro Jan 4, 2023

Choose a reason for hiding this comment

villebro Jan 5, 2023

Choose a reason for hiding this comment

dpgaspar Jan 9, 2023

Choose a reason for hiding this comment

villebro commented Jan 6, 2023

github-actions bot commented Jan 6, 2023

villebro commented Jan 6, 2023

zhaoyongjie left a comment

Choose a reason for hiding this comment

dpgaspar Jan 9, 2023

Choose a reason for hiding this comment

kgabryje left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 9, 2023

villebro commented Jan 2, 2023 •

edited

Loading

codecov bot commented Jan 3, 2023 •

edited

Loading

villebro commented Jan 4, 2023 •

edited

Loading