fix(charts): View in SQL Lab with relevant perm #24903
Conversation
|
Looks like I broke the frontend test; maybe the |
|
Yeah that's the problem: https://github.com/apache/superset/pull/22566/files#diff-886aee5ca8271a9274abc1cb77a23c33481dd94edf3170a1ea57dbe717ef1ba3R71 it's just a dummy role with no permissions because the existing functionality is gated on role name. Easy to fix. |
giftig
force-pushed
the
issue-24902
branch
2 times, most recently
from
7289cec
to
824e8fe
Compare
Codecov Report
@@ Coverage Diff @@
## master #24903 +/- ##
==========================================
+ Coverage 68.96% 69.00% +0.03%
==========================================
Files 1906 1906
Lines 74122 74132 +10
Branches 8208 8209 +1
==========================================
+ Hits 51116 51152 +36
+ Misses 20883 20857 -26
Partials 2123 2123
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 9 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
rusackas
added
the
review:checkpoint
michael-s-molina
removed
the
review:checkpoint
giftig
force-pushed
the
issue-24902
branch
2 times, most recently
from
efd3864
to
51e6f4d
Compare
Do not switch this button on the basis of the name of the role, instead use the relevant permission: [SQL Lab].[menu_access], which is allocated to that role as standard. This prevents issues where the user has the correct permission but via some other role.
| test('userHasPermission always returns false for undefined user', () => { | ||
| arbitraryPermissions.forEach(permissionView => { | ||
| expect( | ||
| userHasPermission(undefinedUser, permissionView[1], permissionView[0]), |
There was a problem hiding this comment.
A minor nit, and I agree that the order of first having the view name, and then the perm makes sense, but from a usability perspective, might it be less error prone to just ask for the permissionView as the second arg to the userHasPermission function? Like so:
const userHasPermission = userHasPermission(underfinedUser, permissionView);Especially, as both viewName and permissionName are both string, making it easy to mix them up. Having a tuple [string, string] feels less error prone.
There was a problem hiding this comment.
I was considering a couple of versions of this because it irked me that it's usually expressed like View.permission when printed from the backend but the ordering is ["permission", "View"] in the frontend tuple, so whichever order you use it's inconsistent with something.
But here, surely it's just as easy to mess up a [string, string] tuple by putting arguments in the wrong order as it is messing up individual arguments? Your tuple still requires two strings and the only syntactic difference ends up being the square brackets for the tuple.
There was a problem hiding this comment.
I also toyed with a much less error prone interface like {view: "Something", permission: "can_read"} but that'd be inconsistent with the tuples found on the user, so I don't think that's great either.
There was a problem hiding this comment.
@giftig you're right, there's probably no super safe way to do this. However, I assume perms usually come back in tuple format, so for that reason [string, string] could be good. But we can do that later, this is already a really good improvement.
michael-s-molina
added
the
v3.0
(cherry picked from commit ce65a3b)
mistercrunch
added
🍒 3.0.3
🍒 3.0.4
🏷️ bot
SUMMARY
Do not switch this button on the basis of the name of the role, instead use the relevant permission:
[SQL Lab].[menu_access], which is allocated to that role as standard.This prevents issues where the user has the correct permission but via some other role.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
As a user with permissions to use SQL Lab, but not the role named
sql_lab, click the three dots next to the datasource name in the chart editor and note that the "View in SQL Lab" button now appears as expected.ADDITIONAL INFORMATION