fix(dashboard-rbac): use normal rbac when no roles chosen

[villebro]

SUMMARY

Currently it's impossible to use the normal RBAC model when the Dashboard RBAC feature flag is enabled. This is contrary to the description, which on one hand states that when the FF is enabled and no roles are chosen, ALL datasets are accessible, and on the other hand states that regular dataset perms apply.

With these changes, the logic is as follows:

• If no roles are defined for a dashboard, regular RBAC always applies.
• If a role/roles are defined for a dashboard, those take precedent, i.e. regular RBAC no longer applies. This is unchanged
• Datasource permission checks now check if the user has Dashboard RBAC access to a dashboard that references the dataset - if it does, the datasource is assumed to be accessible. If not, regular RBAC applies.

In addition, tests are added, and the documentation is aligned with how the feature actually works.

In addition, when trying to access a dashboard the user didn't have access to, they would either be greeted by one of the following:

Regular RBAC:

Dashboard RBAC:

Now they get redirected to the main page with the following toast:

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

• Has associated issue: Closes RBAC not Working #18634
• 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]

Codecov Report

Merging #23586 (41447ac) into master (117360c) will increase coverage by 0.01%.
The diff coverage is 69.60%.

❗ Current head 41447ac differs from pull request most recent head cacc2c2. Consider uploading reports for the commit cacc2c2 to get more accurate results

@@            Coverage Diff             @@
##           master   #23586      +/-   ##
==========================================
+ Coverage   67.72%   67.73%   +0.01%     
==========================================
  Files        1916     1918       +2     
  Lines       74029    74067      +38     
  Branches     8040     8040              
==========================================
+ Hits        50135    50172      +37     
- Misses      21846    21847       +1     
  Partials     2048     2048              

Flag	Coverage Δ
hive	52.74% <25.00%> (-0.01%)	⬇️
javascript	53.96% <60.97%> (-0.01%)	⬇️
mysql	78.51% <66.66%> (+0.01%)	⬆️
postgres	78.58% <66.66%> (+0.01%)	⬆️
presto	52.66% <25.00%> (-0.01%)	⬇️
python	82.43% <73.80%> (+0.01%)	⬆️
sqlite	77.09% <66.66%> (+0.01%)	⬆️
unit	52.63% <32.14%> (+<0.01%)	⬆️

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

Impacted Files	Coverage Δ
.../plugin-chart-echarts/src/Funnel/transformProps.ts	72.34% <ø> (ø)
...s/plugin-chart-echarts/src/Graph/transformProps.ts	72.36% <ø> (ø)
...hart-echarts/src/MixedTimeseries/transformProps.ts	0.00% <ø> (ø)
...ins/plugin-chart-echarts/src/Pie/transformProps.ts	55.71% <ø> (ø)
...s/plugin-chart-echarts/src/Radar/transformProps.ts	0.00% <ø> (ø)
...gin-chart-echarts/src/Timeseries/transformProps.ts	58.33% <ø> (ø)
...d/plugins/plugin-chart-echarts/src/utils/series.ts	88.48% <ø> (ø)
...rc/components/Datasource/ChangeDatasourceModal.tsx	78.68% <ø> (ø)
...ponents/ReportModal/HeaderReportDropdown/index.tsx	67.50% <ø> (ø)
...src/dashboard/components/PropertiesModal/index.tsx	64.03% <ø> (ø)
... and 90 more

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

[john-bodley]

@villebro can all this logic reside in the security manager? The issue is having security logic outside of the security manager adds complexity if you have custom logic in your security manager which grants everyone access to view the dashboard but block at the chart level if you don't have access to the underlying dataset.

[villebro]

I agree, it would probably make sense to have this in the security manager. However, in the scope of this PR I decided not to change the existing logic.

[john-bodley]

Why do we break after confirming that they have access to at least one as opposed to all datasets?

[villebro]

Answering both this and the previous question here:

• The logic for reloading the datasets is to check if the user has access to at least one dataset on the dashboard. AFAIK, this is how regular dashboard RBAC (not the feature flag!) works - if you have access to at least one dataset on the dashboard, you are permitted to view the dashboard. Therefore, we break after confirming that the user is able to access at least one dataset. If we want to change this logic we need to also change the base filter that chooses which dashboards to display on the list view.
• The datasets are reloaded in the block above to check individually if the user has access to them or not. This check was previously only done if ENABLE_ACCESS_REQUEST was enabled - if not, the user would get an obscure React error (when linking from the dashboard page) or a while page with some unformatted text indicating that the user doesn't have access to the dashboard (direct link). Now the user is redirected to the list view with a toast, like we do for other similar access denied errors.

[john-bodley]

Why are the datasets reloaded?