Skip to content

Conversation

@gtg472b
Copy link
Contributor

@gtg472b gtg472b commented Sep 6, 2020

This fix makes sure that RLS filters are searched for templatable jinja content, ensuring cached visualizations aren't shown to the wrong user.

SUMMARY

I added a check to see if there are any RLS filters that are templated

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

I haven't tried it with multiple layers of RLS (eg, a user has multiple roles, and each role has different RLS filters), so that may need to be tested.

ADDITIONAL INFORMATION

  • Has associated issue: Fixes rls cache keys not being used #10804
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

This fix makes sure that RLS filters are searched for templatable jinja content, ensuring cached visualizations aren't shown to the wrong user.
@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2020

Codecov Report

Merging #10805 into master will decrease coverage by 4.27%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10805      +/-   ##
==========================================
- Coverage   65.51%   61.24%   -4.28%     
==========================================
  Files         802      802              
  Lines       37815    37815              
  Branches     3557     3557              
==========================================
- Hits        24775    23158    -1617     
- Misses      12936    14471    +1535     
- Partials      104      186      +82     
Flag Coverage Δ
#cypress ?
#javascript 61.62% <ø> (ø)
#python 61.01% <100.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
superset/connectors/sqla/models.py 89.75% <100.00%> (+0.02%) ⬆️
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupColors.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/chart/ChartContainer.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupFormatters.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/reducers/index.js 0.00% <0.00%> (-100.00%) ⬇️
... and 158 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 92f2353...7c8a50a. Read the comment docs.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Good catch @gtg472b 👍

This fix makes sure that RLS filters are searched for templatable jinja content, ensuring cached visualizations aren't shown to the wrong user.

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
@gtg472b
Copy link
Contributor Author

gtg472b commented Sep 7, 2020

Thanks for fixing it @villebro . I don't have a dev system set up for Superset, and I almost never do anything in Python. Never use GitHub either (this is my first PR!)

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

In that case you've done a stellar job @gtg472b! LGTM, going to merge as soon as CI finishes 🎉

@villebro villebro merged commit 702cfe9 into apache:master Sep 7, 2020
amitmiran137 pushed a commit to ofekisr/incubator-superset that referenced this pull request Sep 7, 2020
…boards_permissions

* upstream/master: (32 commits)
  docs: Add a note to contributing.md on reporting security vulnerabilities (apache#10796)
  Fix: Include RLS filters for cache keys (apache#10805)
  feat: filters for database list view (apache#10772)
  fix: MVC show saved query (apache#10781)
  added creator column and adjusted order columns (apache#10789)
  security: disallow uuid package on jinja2 (apache#10794)
  feat: CRUD REST API for saved queries (apache#10777)
  fix: disable domain sharding on explore view (apache#10787)
  fix: can not type `0.05` in `TextControl` (apache#10778)
  fix: pivot table timestamp grouping (apache#10774)
  fix: add validator information to email/slack alerts (apache#10762)
  More Label touchups (margins) (apache#10722)
  fix: dashboard extra filters (apache#10692)
  fix: re-installing local superset in cache image (apache#10766)
  feat: SIP-34 table list view for databases (apache#10705)
  refactor: convert DatasetList schema filter to use new distinct api (apache#10746)
  chore: removing fsevents dependency (apache#10751)
  Fix precommit hook for docs/installation.rst (apache#10759)
  feat(database): POST, PUT, DELETE API endpoints (apache#10741)
  docs: Update OAuth configuration in installation.rst (apache#10748)
  ...
@dpgaspar dpgaspar added the v0.38 label Sep 10, 2020
dpgaspar pushed a commit to preset-io/superset that referenced this pull request Sep 10, 2020
* Fix: Include RLS filters for cache keys

This fix makes sure that RLS filters are searched for templatable jinja content, ensuring cached visualizations aren't shown to the wrong user.

* Fix: Include RLS filters for cache keys

This fix makes sure that RLS filters are searched for templatable jinja content, ensuring cached visualizations aren't shown to the wrong user.

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
villebro added a commit to preset-io/superset that referenced this pull request Sep 11, 2020
* Fix: Include RLS filters for cache keys

This fix makes sure that RLS filters are searched for templatable jinja content, ensuring cached visualizations aren't shown to the wrong user.

* Fix: Include RLS filters for cache keys

This fix makes sure that RLS filters are searched for templatable jinja content, ensuring cached visualizations aren't shown to the wrong user.

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
villebro added a commit that referenced this pull request Sep 11, 2020
* Fix: Include RLS filters for cache keys

This fix makes sure that RLS filters are searched for templatable jinja content, ensuring cached visualizations aren't shown to the wrong user.

* Fix: Include RLS filters for cache keys

This fix makes sure that RLS filters are searched for templatable jinja content, ensuring cached visualizations aren't shown to the wrong user.

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
villebro added a commit that referenced this pull request Sep 16, 2020
* Fix: Include RLS filters for cache keys

This fix makes sure that RLS filters are searched for templatable jinja content, ensuring cached visualizations aren't shown to the wrong user.

* Fix: Include RLS filters for cache keys

This fix makes sure that RLS filters are searched for templatable jinja content, ensuring cached visualizations aren't shown to the wrong user.

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
* Fix: Include RLS filters for cache keys

This fix makes sure that RLS filters are searched for templatable jinja content, ensuring cached visualizations aren't shown to the wrong user.

* Fix: Include RLS filters for cache keys

This fix makes sure that RLS filters are searched for templatable jinja content, ensuring cached visualizations aren't shown to the wrong user.

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
* Fix: Include RLS filters for cache keys

This fix makes sure that RLS filters are searched for templatable jinja content, ensuring cached visualizations aren't shown to the wrong user.

* Fix: Include RLS filters for cache keys

This fix makes sure that RLS filters are searched for templatable jinja content, ensuring cached visualizations aren't shown to the wrong user.

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
@mistercrunch mistercrunch added 🍒 0.37.2 Cherry-picked to 0.37.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 First shipped in 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/XS v0.37 v0.37.2 v0.38 🍒 0.37.2 Cherry-picked to 0.37.2 🚢 0.38.0 First shipped in 0.38.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rls cache keys not being used

6 participants