feat(dashboard_rbac): dashboard extra jwt

[amitmiran137]

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

• Has associated issue:
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

[villebro]

A few quick comments

[codecov]

Codecov Report

Merging #13773 (b9bd404) into master (f6f412b) will increase coverage by 0.00%.
The diff coverage is 86.20%.

❗ Current head b9bd404 differs from pull request most recent head 308c659. Consider uploading reports for the commit 308c659 to get more accurate results

@@           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           

Flag	Coverage Δ
cypress	56.04% <100.00%> (+<0.01%)	⬆️
hive	80.28% <85.18%> (+0.01%)	⬆️
javascript	66.28% <25.00%> (-0.01%)	⬇️
mysql	80.57% <85.18%> (+<0.01%)	⬆️
postgres	80.60% <85.18%> (+<0.01%)	⬆️
presto	80.29% <85.18%> (+0.01%)	⬆️
python	81.15% <85.18%> (+<0.01%)	⬆️
sqlite	80.17% <85.18%> (+0.01%)	⬆️

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

Impacted Files	Coverage Δ
superset/security/manager.py	90.47% <70.00%> (-0.62%)	⬇️
superset/app.py	81.09% <83.33%> (-0.09%)	⬇️
superset/utils/dashboard_jwt_manager.py	84.00% <84.00%> (ø)
superset-frontend/src/chart/chartAction.js	78.71% <100.00%> (+0.10%)	⬆️
...perset-frontend/src/dashboard/containers/Chart.jsx	100.00% <100.00%> (ø)
...frontend/src/dashboard/reducers/getInitialState.js	84.26% <100.00%> (ø)
superset/charts/schemas.py	100.00% <100.00%> (ø)
superset/common/query_context.py	82.35% <100.00%> (+0.19%)	⬆️
superset/config.py	90.78% <100.00%> (+0.03%)	⬆️
superset/extensions.py	93.33% <100.00%> (+0.18%)	⬆️
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6f412b...308c659. Read the comment docs.

[suddjian]

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)

[suddjian]

I'm also not really clear on why the jwt is needed. Some more background info would be helpful.

[amitmiran137]

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

[amitmiran137]

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

[suddjian]

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?

[ktmud]

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.

[amitmiran137]

@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

[ktmud]

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?

[amitmiran137]

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

[suddjian]

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.

[amitmiran137]

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

[suddjian]

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)

[ktmud]

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.

[amitmiran137]

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

[ktmud]

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.

[suddjian]

Maybe we need a "nested dashboard" viz plugin 😁

[amitmiran137]

Adding another important statement here about the final JWT lifecycle:

1. The jwt will be consumed from the dashboard frontend component by calling dashboard API and one of the properties inside will be the extra_jwt ( similar to fetching it from the bootstrap data RIP 😜)

2. the jwt is then passed on every chart data request within the form data back to the backend

3. upon raise_for_access existing validation in addition to permission of dataset/schema
   The jwt is being parsed and questioned either a dataset permission exist within

P.S
in any other chart data API call no jwt will exist therfore only dataset/schema permission mechanism applies

[amitmiran137]

closing this one and opened a simpler solution here: #13992