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.

Sign up for GitHub

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

Jump to bottom

feat(rls): enable row level security by default #13772

Merged
merged 1 commit into from
Mar 26, 2021
Merged

feat(rls): enable row level security by default #13772

merged 1 commit into from
Mar 26, 2021

Conversation

villebro
Copy link
Member

SUMMARY

This sets the RLS feature flag to be enabled by default, making RLS generally available.
TODO: add note in UPDATING.md

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

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

@codecov
Copy link

codecov bot commented Mar 24, 2021 •
edited
Loading

Codecov Report

Merging #13772 (0cd5633) into master (9773aba) will increase coverage by 1.40%.
The diff coverage is n/a.

❗ Current head 0cd5633 differs from pull request most recent head 3419020. Consider uploading reports for the commit 3419020 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13772      +/-   ##
==========================================
+ Coverage   75.91%   77.32%   +1.40%     
==========================================
  Files         933      933              
  Lines       47185    47186       +1     
  Branches     5872     5872              
==========================================
+ Hits        35821    36487     +666     
+ Misses      11191    10557     -634     
+ Partials      173      142      -31
Flag Coverage Δ
cypress 56.24% <ø> (+10.23%) ⬆️
hive ?
mysql 80.42% <ø> (-0.14%) ⬇️
postgres 80.59% <ø> (ø)
presto 80.25% <ø> (-0.01%) ⬇️
python 80.70% <ø> (-0.41%) ⬇️
sqlite 80.15% <ø> (ø)

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

Impacted Files Coverage Δ
superset/config.py 90.71% <ø> (ø)
superset/db_engines/hive.py 0.00% <0.00%> (-85.72%) ⬇️
superset/db_engine_specs/hive.py 74.23% <0.00%> (-16.54%) ⬇️
superset/db_engine_specs/presto.py 88.28% <0.00%> (-0.42%) ⬇️
...perset-frontend/src/views/CRUD/chart/ChartList.tsx 84.49% <0.00%> (+0.77%) ⬆️
superset-frontend/src/components/ListView/utils.ts 85.95% <0.00%> (+0.82%) ⬆️
.../src/explore/components/ControlPanelsContainer.tsx 97.27% <0.00%> (+0.90%) ⬆️
superset-frontend/src/components/Select/styles.tsx 86.30% <0.00%> (+1.36%) ⬆️
superset-frontend/src/chart/Chart.jsx 64.81% <0.00%> (+1.85%) ⬆️
...-frontend/src/dashboard/actions/dashboardLayout.js 98.03% <0.00%> (+1.96%) ⬆️
... and 94 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 9773aba...3419020. Read the comment docs.

junlin-qa
junlin-qa approved these changes Mar 24, 2021 •
edited
Loading
View reviewed changes
dpgaspar
dpgaspar approved these changes Mar 26, 2021
View reviewed changes
Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

LGTM, evaluate if it makes sense to completely remove the switch

superset/config.py
@@ -349,7 +349,7 @@ def _try_json_readsha( # pylint: disable=unused-argument
# by that custom datasource access. So we are assuming a default security config,
# a custom security config could potentially give access to setting filters on
# tables that users do not have access to.
"ROW_LEVEL_SECURITY": False,
"ROW_LEVEL_SECURITY": True,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could just remove this flag from the config, one less config key, and a bunch of if's on the code

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll leave it for a short period just to make it easy for orgs running master to disable the feature in case we run into a nasty bug.

@villebro villebro merged commit 5ae91e2 into apache:master Mar 26, 2021
@villebro villebro deleted the villebro/enable-rls branch March 26, 2021 15:40
allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021
@villebro
feat(rls): enable row level security by default (apache#13772)
1269cb3
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpgaspar dpgaspar dpgaspar approved these changes

@amitmiran137 amitmiran137 amitmiran137 approved these changes

@junlin-qa junlin-qa junlin-qa approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/XS 🚢 1.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@villebro @dpgaspar @amitmiran137 @junlin-qa @mistercrunch