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(Digest): Add RLS at digest generation for Charts and Dashboards #30336

Merged
merged 8 commits into from
Sep 24, 2024

Conversation

geido
Copy link
Member

@geido geido commented Sep 19, 2024

SUMMARY

Digest generation for charts and dashboards wasn't considering RLS for users and guest users (via Embedded). This PR includes the RLS in the hash generation for the digest. The digest is included in cache keys and will be updated according to the RLS rules. Previously you could generate a screenshot and any RLS applied later won't be applied to follow-up screenshots as the same cache key was being reused.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N.A.

TESTING INSTRUCTIONS

  1. Generate a screenshot of a Dashboard
  2. Add RLS rules for the current user
  3. Generate the screenshot again
  4. The screenshot should present the Dashboard with RLS rules applied

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

@github-actions github-actions bot added the api Related to the REST API label Sep 19, 2024
@dosubot dosubot bot added the authentication:row-level-security Related to Row Level Security label Sep 19, 2024
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.

First pass comments

superset/thumbnails/digest.py Outdated Show resolved Hide resolved
superset/thumbnails/digest.py Outdated Show resolved Hide resolved
@apache apache deleted a comment from github-actions bot Sep 20, 2024
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.

I forgot to say great work, thanks for taking this on, important improvement! Second pass comments.

superset/dashboards/api.py Outdated Show resolved Hide resolved
superset/dashboards/api.py Show resolved Hide resolved
superset/security/manager.py Outdated Show resolved Hide resolved
superset/thumbnails/digest.py Outdated Show resolved Hide resolved
tests/unit_tests/thumbnails/test_digest.py Outdated Show resolved Hide resolved
superset/thumbnails/digest.py Outdated Show resolved Hide resolved
@geido geido merged commit de3af85 into master Sep 24, 2024
33 checks passed
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@rusackas rusackas deleted the geido/feat/rls-digest branch September 27, 2024 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to the REST API authentication:row-level-security Related to Row Level Security size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants