Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: deprecate rls base related filters #24128

Merged
merged 4 commits into from
May 19, 2023

Conversation

villebro
Copy link
Member

@villebro villebro commented May 19, 2023

SUMMARY

This is a follow-up to the work done in #23777, more specifically this discussion: #23777 (comment)

Currently Row Level Security has a dedicated set of related base filters that makes it possible to customize the filters that are applied on the Table and Role models. These are defined on the RLS_BASE_RELATED_FIELD_FILTERS config parameter.

Having these undefined by default is counter intuitive, as it means that a user that is given RLS access will be able to see ALL datasets under the RLS table dropdown, irrespective of which tables they see under the Datasets menu. To streamline this we should just use the same base filter we use for the dataset list view (DatasourceFilter).

For roles, the PR #22526 introduced the possibility to define a base filter for Roles. To avoid having duplicated logic and to give the users a unified UX, we can reuse the Role filter that users can define via the EXTRA_RELATED_QUERY_FILTERS config flag.

SCREENSHOTS

This is the datasets view for a user that is only able to access two datasets on the instance (the Gamma user only has database access to the BurgerKing database, despite there being many other databases/datasets on the instance).
image

With the DASHBOARD_RBAC feature enabled and a custom Roles filter, the user was only able to see the Team-BurgerKing role:
image

Previously, the same user would see all tables and roles on the instance if they had RLS perms. After this change the RLS dropdowns are aligned with what's available elsewhere:

RLS tables:
image

RLS roles:
image

As a bycatch, the "Roles" title is also changed to "Excluded roles" if the filter type is set to "Base" (this is similar to how the list view):
image

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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
Copy link

codecov bot commented May 19, 2023

Codecov Report

Merging #24128 (21e658e) into master (8b4222f) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 21e658e differs from pull request most recent head 2f2ace3. Consider uploading reports for the commit 2f2ace3 to get more accurate results

@@           Coverage Diff           @@
##           master   #24128   +/-   ##
=======================================
  Coverage   68.25%   68.25%           
=======================================
  Files        1952     1952           
  Lines       75349    75349           
  Branches     8204     8205    +1     
=======================================
  Hits        51433    51433           
  Misses      21810    21810           
  Partials     2106     2106           
Flag Coverage Δ
hive 53.30% <100.00%> (-0.01%) ⬇️
javascript 54.68% <100.00%> (+<0.01%) ⬆️
mysql 78.94% <100.00%> (-0.01%) ⬇️
postgres 79.02% <100.00%> (-0.01%) ⬇️
presto 53.22% <100.00%> (-0.01%) ⬇️
python 82.81% <100.00%> (-0.01%) ⬇️
sqlite 77.55% <100.00%> (-0.01%) ⬇️
unit 53.24% <100.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
superset/config.py 91.97% <ø> (-0.05%) ⬇️
...rontend/src/features/rls/RowLevelSecurityModal.tsx 73.63% <100.00%> (+0.24%) ⬆️
superset/row_level_security/api.py 96.07% <100.00%> (+0.03%) ⬆️

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

@pull-request-size pull-request-size bot added size/M and removed size/S labels May 19, 2023
@villebro villebro added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label May 19, 2023
UPDATING.md Outdated
@@ -24,6 +24,7 @@ assists people when migrating to a new version.

## Next

- [24128](https://github.com/apache/superset/pull/24128) The `RLS_BASE_RELATED_FIELD_FILTERS` config parameter has been removed. Now the Tables dropdown will feature the same tables that the user is able to see elsewhere in the application using the standard `DatasourceFilter`, and the Roles dropdown will be filtered using the filter defined in `EXTRA_RELATED_QUERY_FILTERS["role"]`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this to the Breaking Changes section?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof, good catch!

@villebro villebro merged commit 5424b95 into apache:master May 19, 2023
@michael-s-molina michael-s-molina removed the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Jun 26, 2023
@eschutho eschutho added the risk:breaking-change Issues or PRs that will introduce breaking changes label Jul 6, 2023
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels risk:breaking-change Issues or PRs that will introduce breaking changes size/M 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants