superset icon indicating copy to clipboard operation
superset copied to clipboard

fix(embedded): add row level security extra cache keys for guest users

Open ygelfand opened this issue 3 years ago • 4 comments

SUMMARY

Cache keys from guest token row level security clauses were not being evaluated, causing data to leak across sessions via cache.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

In one session, create/use an guest_token without a rls clause, and a session with a rls clause with an additional cache key e.g.:

   { "clause": "publisher = '{{cache_key_wrapper('Nintendo')}}'" }
 ]

The same dashboard should have independent cache for each session, and the one with the rls clause should be unable to see unfiltered data.

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

ygelfand avatar Feb 24 '22 12:02 ygelfand

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

stale[bot] avatar Apr 27 '22 18:04 stale[bot]

@michael-s-molina know anyone who would be well qualified to review this. I'm trying not to throw as much at @dpgaspar or @villebro though here I go mentioning them anyway ;)

Closing/reopening to kick-start CI.

rusackas avatar Aug 22 '24 19:08 rusackas

@michael-s-molina know anyone who would be well qualified to review this. I'm trying not to throw as much at @dpgaspar or @villebro though here I go mentioning them anyway ;)

@rusackas We don't use embedded and RLS at Airbnb so you probably pinged the wrong person 😄

michael-s-molina avatar Aug 22 '24 19:08 michael-s-molina

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 57.33%. Comparing base (0ae1ca7) to head (802f360). Report is 5863 commits behind head on master.

:exclamation: There is a different number of reports uploaded between BASE (0ae1ca7) and HEAD (802f360). Click for more details.

HEAD has 3 uploads less than BASE
Flag BASE (0ae1ca7) HEAD (802f360)
python 6 3
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #18924      +/-   ##
==========================================
- Coverage   66.21%   57.33%   -8.88%     
==========================================
  Files        1633     1746     +113     
  Lines       63210    70409    +7199     
  Branches     6409     6409              
==========================================
- Hits        41852    40372    -1480     
- Misses      19698    28377    +8679     
  Partials     1660     1660              
Flag Coverage Δ
hive ∅ <100.00%> (∅)
mysql ?
postgres ?
presto ?
python ∅ <100.00%> (∅)
sqlite ?
unit ∅ <100.00%> (?)

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Aug 22 '24 19:08 codecov[bot]

Looks like there are still a lot of failing tests here. I'll convert it to draft for now, but it may be closed due to inactivity eventually if it isn't made mergeable.

rusackas avatar Apr 22 '25 21:04 rusackas