feat(dashboard_rbac): dashboard extra jwt #13773
Conversation
…ting permission mechanisem
… feat/dashboard_extra_jwt * upstream/feat/dashboard_extra_jwt: start of dashboard jwt manager # Conflicts: # superset/utils/dashboard_jwt_manager.py
amitmiran137
changed the title
Codecov Report
@@ Coverage Diff @@
## master #13773 +/- ##
=======================================
Coverage 78.34% 78.34%
=======================================
Files 934 935 +1
Lines 47395 47401 +6
Branches 5964 5948 -16
=======================================
+ Hits 37130 37137 +7
+ Misses 10121 10120 -1
Partials 144 144
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
junlincc
added
the
dashboard:security:access
* master: (56 commits) test: Adds tests and storybook to CertifiedIcon component (#13457) chore: Moves CheckboxIcons to Checkbox folder (#13459) chore: Removes Popover duplication (#13462) build(deps): bump elliptic from 6.5.3 to 6.5.4 in /docs (#13527) fix: allow spaces in DB names (#13800) chore: Update PR template for SIP-59 DB migrations process (#13855) Add CODEOWNERS (#13759) feat(alerts & reports): Easier to read execution logs (#13752) fix: Disallows negative options remaining (#13749) Fix broken link (#13861) fix(native-filters): add global async query support to native filters (#13837) Displays row limit warning with Alert component (#13854) fix(errors): Downgrade error on stop query to a warning (#13826) fix(alerts and reports): Unify timestamp format on execution log view (#13718) fix(sqllab): warning message when rows limited (#13841) chore: add success log whenever a connection is working (#13811) fix(native-filters): improve loading styles for filter component (#13794) chore: update change log with cherry-picks for release 1.1 (#13824) feat: added support to configure the default explorer viz (#13610) fix(#13734): Properly escape special characters in CSV output (#13735) ...
| @@ -1869,6 +1875,19 @@ def dashboard( # pylint: disable=too-many-locals | |||
| if key not in [param.value for param in utils.ReservedUrlParameters] | |||
| } | |||
|
|
|||
| extra_jwt = {} | |||
| if feature_flag_manager.is_feature_enabled("DASHBOARD_RBAC"): | |||
There was a problem hiding this comment.
There is a project underway to make Dashboards part of a single page app. This code will only work if dashboards are accessed from this route, but in a SPA that will not necessarily be the case. If you click a link to a dashboard from the dashboards list or the homepage, the routing will be done on the frontend. Maybe this code should be moved to a separate endpoint that the frontend calls to get the jwt.
(see also #13306)
There was a problem hiding this comment.
I'm also not really clear on why the jwt is needed. Some more background info would be helpful.
There was a problem hiding this comment.
Ok so the jwt is being used to manage read data access of charts that exists on a dashboard implicitly just by having a access to the dashboard
This really upgrade the dashboard access RBAC mechanism by also giving Temporary access to datasets that are being used within the dash lard without the need to explicitly giving permission to those datasets
There was a problem hiding this comment.
I'm totally aware of SPA project and I plan to also support and implement the dashboard jwt feature under that as well
But for now until SPA maturity I want to to be supported in the old style
There was a problem hiding this comment.
I'm expecting to have a PR up that removes dashboard as a standalone app this week, so I think compatibility is an important consideration now.
What about adding logic to the security of the chart data and dataset endpoints, to check whether access should be granted at the time of request? Is that an option? Sorry if I'm missing something, I'd just like to fully understand the need for this JWT pattern. Is the time limit for access a feature requirement or an implementation detail?
There was a problem hiding this comment.
I'm a little bit wary of the security implication of allowing dashboard readers to have access to charts and datasets they explicitly don't have access to.
What if someone creates a dashboard with charts that use datasets they don't have access to, but add themselves to the allowed list of roles? What is the access control flow look like? Would you restrict who can change dashboard roles only to those who have admin access to the underlying datasets? Because if someone has view access to a dataset or edit access to a dashboard, it doesn't mean they can just publish this dataset or the content of this dashboard to any audience.
| if not ( | ||
| self.can_access_schema(datasource) | ||
| ds_allowed_in_dashboard |
There was a problem hiding this comment.
@ktmud @suddjian upon each chart data query request (legacy or v1 API) this part is being called of validating from extracting the signed jwt if that dataset is being called from a queryContext /Viz
Only dashboard owner/admin an give dashboard access (see the video)
Let's assume that daahboard access is being granted to a read only persona
And to that persona explore chart permission is not granted as well
So that type of a user can only see data under the dashboard
He cannot edit the dataset or play around with it in the explore
In theory he could use that jwt to query the dataset freely in SQLlab(if he permission to sqllab to begin with)
From a dashboard point of view is up to the dashboard creator to wrap data with the relevant datasets(either physical or virtual) that will restrict the readonly personas to what is relevant for them
If there is a huge physical dataset that might expose sensitive data then you can always have a thiner portion of it by creating a virtual one
How would you suggest make that flow secure?
@suddjian I would be happy to do a follow up PR just to support SPA , no worries there
There was a problem hiding this comment.
Could you elaborate more on why you need users without direct access to certain datasets to have access to that dataset through dashboards? Can you just create virtual datasets and grant them read access to the dataset instead?
There was a problem hiding this comment.
in our org every dashboard might serve different set of users which basically means a separated role per dashboard.
so managing access for every dataset on every dashboard is a cumbersome work.
it will require managing all datasets permissions for every role.
this problem scale when having hundreds of dashboards.
additionally, dataset permission mechanism has a defect , when changing a virtual dataset name then permission name changes as well but if it is already associated to a role you'll need to remove it and then re-associate bc permission is based on the name of the dataset and not by the id
There was a problem hiding this comment.
I would be happy to do a follow up PR just to support SPA , no worries there
In the SPA project we have already done the work to remove dashboard-specific bootstrap data so that the apps can be cleanly merged. I think rather than adding new bootstrap properties at this stage, it would be better to use an endpoint. That said, if you're okay with this feature breaking in the very near future, I won't stand in your way. The compatibility discussion is a relatively minor issue.
I think the security concerns have already been discussed somewhat in SIP-51. Anyone using the feature flag should understand that dashboard owners have the ability to grant data access to others arbitrarily. These effects should be well documented. It's an entirely different security profile for the application.
I am of the opinion that the security manager should make a database query to find the information it needs, rather than using this new JWT pattern. I haven't seen anything that indicates that a JWT is actually required here, though I could be mistaken.
Introducing new authorization patterns specific to a given feature flag will make it more difficult to onboard into the Superset codebase, as well as making it more complicated to change system behavior. The JWT pattern also makes it complicated to revoke or change a user's access, and introduces a new responsibility for the client to correctly manage the JWT.
There was a problem hiding this comment.
The jwt and it's data indicates that the request indeed coming from the Dashboard and with the nessecery dataset permission.
Otherwise upon chart data request it would be impossible to know if the request does originates from a dashboard or not
There was a problem hiding this comment.
But you could write a query selecting dashboards the user can access which use the dataset that's being requested, no?
Something akin to:
query = query(BaseDatasource).join(Slice).join(Dashboard).filter(BaseDatasource.id == datasource.id)
query = DashboardFilter("id", SQLAInterface(Dashboard, db.session)).apply(query)
There was a problem hiding this comment.
What if you want to publish a dashboard to some users, but a subset of these users cannot have access to a couple of more sensitive charts in the dashboard? If seems by turning on the DASHBOARD_RBAC flag, there is no way to accommodate this case, unless you add some kind of dashboard level metadata to indicate this dashboard should not issue a JWT.
There was a problem hiding this comment.
In this use case we would probably just have 2 separated Dashboards
Just bc there no sense of providing dashboard to a client and show him only have of charts, that is a very bad UX don't you think ? 💆
I know Air BnB have this portion of a dashboard use case
But for us it just doesnt makes sense 🙅
Our Dashboards are for external clients , try to explain to them why they see only half
There was a problem hiding this comment.
I'd say even your external clients may have people with different access levels. Maybe not now, but it's possible.
Sometimes you want a single dashboard for all audience because it's easier to navigate and easier to communicate---you can share the same short link to all users and executives don't have to jump between two dashboards just to see the more sensitive data. You may also have two duplicate dashboards with 80% of the content the same, but it'd gradually become difficult to maintain.
There was a problem hiding this comment.
Maybe we need a "nested dashboard" viz plugin 😁
|
Adding another important statement here about the final JWT lifecycle:
P.S |
* master: (26 commits) chore: bump to new superset-ui version (#13932) fix: do not run containers as root by default in Helm chart (#13917) feat(explore): adhoc column formatting for Table chart (#13758) fix(sqla-query): order by aggregations in Presto and Hive (#13739) feat(alert/report): add ALERTS_ATTACH_REPORTS feature flags + feature (#13894) test: Fixes PropertiesModal_spec (#13548) fix: Pin Prophet dependency after breaking changes (#13852) test: Adds tests to dnd controls (#13650) test: Adds tests to the AnnotationLayer component (#13748) test: Refactor and enhance tests for the Explore DatasourcePanel Component (#13799) Add tests (#13778) test: DisplayQueryButton (#13750) Fixing condition around left margin for dashboard layout. Fixes #13863 (#13905) Revert "fix: select table overlay (#13694)" (#13901) test: Adds tests to the OptionControls component (#13729) test: DatasourceControl (#13605) tests for function handleScroll (#13896) test: Adds tests to the CustomFrame component (#13675) test: Adds tests to the AdvancedFrame component (#13664) test: DataTableControl (#13668) ...
|
closing this one and opened a simpler solution here: #13992 |
SUMMARY
This is the second milestone in dashboard_rbac support where having a dashboard role entitles the user for a temporary data access to all of the datasets within a dashboard by passing a backend generated jwt from dashboard to charts
this was tested with legacy and v1 API data calls
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
dashboard_extra_jwt_effect.mov
TEST PLAN
ADDITIONAL INFORMATION