fix(rbac): show objects accessible by database access perm

[villebro]

SUMMARY

Currently giving a user "database access on ..." does indeed give access to all dashboards, charts and datasets on that database, but they don't show up on the list views. This fixes that. Other changes:

• Similar logic existed in multiple places across the codebase, some of which were missing recently introduced permission logic. To DRY it up, we introduce a new util that generates the necessary filter clauses and use that in all places where we need to filter by accessible datasets.
• The fairly recent PR fix: Allow dataset owners to see their datasets #20135 introduced logic to display datasets that the user owns. While it's important to show datasets that are owned, the logic introduced in that PR is no longer necessary after this change, as users will now see all owned datasets, as long as they can access them. Therefore the added logic is removed and the integration test is replaced by a test that ensures that the correct datasets are visible with the correct roles/perms.

TESTING INSTRUCTIONS

1. Add a database "X"
2. Create a role "Y" with the "database access on X" permission
3. Add a dataset from the database, create a chart from it and also create + publish a dashboard from it.
4. Create a user with roles "Gamma" and "Y"
5. Login with that user and notice that the newly added dataset + chart + dashboard is now shown on the listview.

ADDITIONAL INFORMATION

• Has associated issue: closes database access on [DATABASE] does not allow read data sources #20208
• 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 #23118 (b5698b6) into master (95eb8d7) will decrease coverage by 11.27%.
The diff coverage is 53.12%.

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

@@             Coverage Diff             @@
##           master   #23118       +/-   ##
===========================================
- Coverage   67.45%   56.19%   -11.27%     
===========================================
  Files        1898     1899        +1     
  Lines       73111    73123       +12     
  Branches     7952     7952               
===========================================
- Hits        49320    41093     -8227     
- Misses      21755    29994     +8239     
  Partials     2036     2036               

Flag	Coverage Δ
hive	?
mysql	?
postgres	?
presto	52.76% <53.12%> (+0.01%)	⬆️
python	58.96% <53.12%> (-23.37%)	⬇️
sqlite	?
unit	52.60% <53.12%> (+0.01%)	⬆️

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

Impacted Files	Coverage Δ
superset/charts/filters.py	68.33% <42.85%> (-19.60%)	⬇️
superset/utils/filters.py	45.45% <45.45%> (ø)
superset/views/base.py	61.24% <50.00%> (-15.39%)	⬇️
superset/views/chart/filters.py	70.00% <50.00%> (-5.00%)	⬇️
superset/security/manager.py	62.46% <60.00%> (-33.47%)	⬇️
superset/dashboards/filters.py	52.68% <100.00%> (-40.87%)	⬇️
superset/utils/dashboard_import_export.py	0.00% <0.00%> (-100.00%)	⬇️
superset/tags/core.py	4.54% <0.00%> (-95.46%)	⬇️
superset/key_value/commands/update.py	0.00% <0.00%> (-90.91%)	⬇️
superset/key_value/commands/delete.py	0.00% <0.00%> (-87.88%)	⬇️
... and 296 more

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

[villebro]

This test didn't clean up after itself, causing the updated test to fail, hence the added cleanup here

[dpgaspar]

This is great, could be considered a breaking change, since all users with database access to X on SQLLab, will now have access to all dashboards/charts with datasets from that database.
Would be nice to have more tests asserting this change on charts and dashboards.

[villebro]

This is great, could be considered a breaking change, since all users with database access to X on SQLLab, will now have access to all dashboards/charts with datasets from that database.
Would be nice to have more tests asserting this change on charts and dashboards.

In fact all users already had access to the underlying entities (datasets, charts and dashboards), they were just not showing up on the list views. So I rather consider this a fix, unless someone was relying on some items to be hidden but accessible.

Having said that, it does make sense to at least add a note to UPDATING.md. Do you think that would be sufficient? Or does this need to wait for 3.0 and be put behind a feature flag before then?

[dpgaspar]

This is great, could be considered a breaking change, since all users with database access to X on SQLLab, will now have access to all dashboards/charts with datasets from that database.
Would be nice to have more tests asserting this change on charts and dashboards.

In fact all users already had access to the underlying entities (datasets, charts and dashboards), they were just not showing up on the list views. So I rather consider this a fix, unless someone was relying on some items to be hidden but accessible.

Having said that, it does make sense to at least add a note to UPDATING.md. Do you think that would be sufficient? Or does this need to wait for 3.0 and be put behind a feature flag before then?

I vote for just adding a note on UPDATING, but @eschutho should have a word here

[cccs-tom]

Have we settled on the fact that this change is not breaking, @dpgaspar and @villebro? And if so could this be included in the 2.1.0 release, please @eschutho?

[eschutho]

My vote would be that this isn't a breaking change. It seems like it would have been expected behavior that the crud view shows all objects that you have access to. @cccs-tom I already cut the 2.1 release, but I could cherry this in as a fix into a future rc if there is one.

[yousoph]

/testenv up

[github-actions]

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

[cccs-tom]

Awesome, thank you @eschutho!

[villebro]

FYI, I tagged this with "v2.1", in case we end up doing 2.1.0rc2

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.