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

feat(row-level-security): add base filter type and filter grouping #10946

Merged
merged 4 commits into from
Sep 22, 2020

Conversation

villebro
Copy link
Member

@villebro villebro commented Sep 17, 2020

SUMMARY

This PR adds two new fields to Row Level Security Filters (RLS):

  • Filter type: Currently RLS filters only apply additional where clauses if a user belongs to a role referenced by a RLS filter. This can cause leakage of sensitive data, as a person that doesn't satisfy any RLS filters will see all data (=no RLS filters applied). To fix this, a base filter type is added that can be used to apply a minimum filter that applies to all users. This is especially useful for cases where by default we don't want to show any rows if a user doesn't belong to any roles that have RLS filters attached to them. In this case we can set a base filter of 1 = 0, which will always be false.
  • Group key: currently all RLS filters have to evaluate to true (=intersection). This can be problematic in cases where a user belongs to many roles that are exclusive: If, for instance, a person belongs to departments A and B and there is a RLS filter for both departments, the user won't see any rows, as the where clause will be department = 'A' AND department = 'B'. For these cases a group key department can be assigned to both RLS filters, after which they are ORed together (=union).

Example:

  • We want all data in a table to be visible to the Admin role
  • For regular users, we should only show 1 year of data
  • For researchers, 10 years of data will be shown
  • Data for domestic region is available to all users
  • Data for foreign region is restricted to management only

RLS filters:

  • Filter 1: type: Base, excluded roles: Admin, group key: "duration", clause: "date > now - 1 year"
  • Filter 2: Type: Regular, roles: Research, group key: "duration", clause: "date > now - 10 year"
  • Filter 3: Type: Base, excluded roles: Admin, group key: "region", clause: "region = 'domestic'"
  • Filter 4: Type: Regular, roles: Management, group key: "region", clause: "region = 'foreign'"

This would render the following extra clauses:

  • Admin user: None, i.e. all data would be visible
  • No roles: (date > now - 1 year) AND (region = 'domestic')
  • Research: (date > now - 1 year OR date > now - 10 year) AND (region = 'domestic')
  • Management: (date > now - 1 year) AND (region = 'domestic' OR region = 'foreign')
  • Management + Research: (date > now - 1 year OR date > now - 10 year) AND (region = 'domestic' OR region = 'foreign')

This change is backwards compatible, meaning that current filters will continue to function as before.

SCREENSHOTS

Add/Edit:
image
The custom list view. Note that Roles display "Not [Admin]" for base filters that apply to all except Admin and "All" when no roles have been excluded:
image

TEST PLAN

CI + new tests

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

tests/security_tests.py Outdated Show resolved Hide resolved
superset/security/manager.py Outdated Show resolved Hide resolved
tests/security_tests.py Show resolved Hide resolved
tests/security_tests.py Outdated Show resolved Hide resolved
tests/security_tests.py Outdated Show resolved Hide resolved
tests/security_tests.py Show resolved Hide resolved
@villebro
Copy link
Member Author

Thanks @bkyryliuk for the review, great comments!

@villebro villebro changed the title [WIP] feat(row-level-security): add filter grouping an [WIP] feat(row-level-security): add base filter type and filter grouping Sep 21, 2020
@villebro villebro changed the title [WIP] feat(row-level-security): add base filter type and filter grouping feat(row-level-security): add base filter type and filter grouping Sep 21, 2020
@villebro
Copy link
Member Author

@bkyryliuk this is ready for another round of review

class RowLevelSecurityFiltersModelView( # pylint: disable=too-many-ancestors
SupersetModelView, DeleteMixin
):
datamodel = SQLAInterface(models.RowLevelSecurityFilter)

list_widget = cast(SupersetListWidget, RowLevelSecurityListWidget)
Copy link
Member Author

@villebro villebro Sep 21, 2020

Choose a reason for hiding this comment

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

For some reason mypy flags Type[RowLevelSecurityListWidget] as being incompatible with Type[SupersetListWidget] which is required for list_widget, despite RowLevelSecurityListWidget extending from SupersetListWidget. Either I'm missing something fundamental here, or mypy is just flagging a false positive.

@altef
Copy link
Contributor

altef commented Sep 21, 2020

This looks like it adds power/flexibility to RLS without adding unnecessary complexity if you don't want that flexibility.
I can definitely see a use-case for it - seems really cool and I greatly appreciate that it doesn't break backwards compatibility

@villebro villebro merged commit 448a41a into apache:master Sep 22, 2020
@villebro villebro deleted the villebro/rls2 branch September 22, 2020 05:54
amitmiran137 pushed a commit to ofekisr/incubator-superset that referenced this pull request Sep 22, 2020
…l_access/dashboard_by_id_endpoints

* upstream/master: (29 commits)
  fix(presto): default unknown types to string type (apache#10753)
  feat(row-level-security): add base filter type and filter grouping (apache#10946)
  docs: add gallery screenshot & link in README (apache#10988)
  docs: add a "Gallery" page (apache#10968)
  build: add PR lint action (apache#10990)
  adding filters back that caused issues (apache#10989)
  chore: selectors refactor in SQLLab test suite (Cypress) (apache#10944)
  ESLint: Remove ts-ignore comments (apache#10933)
  style: fix checkbox color (apache#10970)
  fix: changed disabled rules in datasets module (apache#10979)
  fix: update the time filter for 'Last Year' option in explore (apache#10829)
  fix: use nullpool even for user lookup in the celery (apache#10938)
  Allow empty observations in alerting (apache#10939)
  fix: re-enabling several globally disabled lint rules (apache#10957)
  fix: setting specific exceptions common/query_context.py (apache#10942)
  Pylint disabled rule `pointless-string-statement` is not raising warining anymore - removing (apache#10975)
  fix: pylint disabled rules in dashboard/api.py (apache#10976)
  fix: removed disabled lint rule `too-many-locals` in connectors/base/models.py (apache#10958)
  ESLint: Re-enable rule no-access-state-in-setstate (apache#10870)
  fix: simply is_adhoc_metric (apache#10964)
  ...
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
…pache#10946)

* feat(row-level-security): add filter type and group key

* simplify tests and add custom list widget

* address comments

* use enum value to ensure case sensitive value is used
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 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 size/L 🚢 0.38.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants