Skip to content

feat: current_user_rls_rules Jinja macro#33614

Merged
Vitor-Avila merged 3 commits into
masterfrom
feat/current_user_rls_rules-jinja-macro
May 29, 2025
Merged

feat: current_user_rls_rules Jinja macro#33614
Vitor-Avila merged 3 commits into
masterfrom
feat/current_user_rls_rules-jinja-macro

Conversation

@Vitor-Avila
Copy link
Copy Markdown
Contributor

SUMMARY

This PR adds a new Jinja macro: {{ current_user_rls_rules() }}. The macro returns a list of all RLS rules applied for this user in the current dataset. It works with both embedded and non-embedded users.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2025-05-29.at.1.13.16.AM.mov

TESTING INSTRUCTIONS

Unit tests added. For manual testing:
1.. Create a virtual dataset using below SQL:

{% set rls = current_user_rls_rules() %}
select
{% if rls %}
  {% for rule in rls %}
    'RLS Clause: {{ rule|replace("'", '`')|tojson }}'
    {% if not loop.last %}
    || '</br>' ||
    {% endif %}
  {% endfor %}
{% else %}
  ''
{% endif %}
as test
  1. Create an RLS mapped to this dataset and the admin role (make sure your test account is added to this role).
  2. Display this dataset in a table chart.

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 doc Namespace | Anything related to documentation label May 29, 2025
@dosubot dosubot Bot added authentication:row-level-security Related to Row Level Security global:jinja Related to Jinja templating labels May 29, 2025
assert cache.current_user_rls_rules() is None


def test_user_macros_without_cache_key_inclusion(mocker: MockerFixture):
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is now cover in the previous test with parametrize

Copy link
Copy Markdown

@korbit-ai korbit-ai Bot left a comment

Choose a reason for hiding this comment

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

I've completed my review and didn't find any issues.

Files scanned
File Path Reviewed
superset/jinja_context.py

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

cache key by adding the following parameter to your Jinja code:

```python
{{ current_user_rls_rules(add_to_cache_keys=False) }}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why would we want to do this? Would it allow users to see cached RLS roles that don't belong to them? — that could be a security issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's a fair point. To be honest, I just copied the same content from other macros in this doc (like current_user_roles) and updated them to my new macro.

I think in practice this is irrelevant, because even if you set add_to_cache_keys=False, the macro returns RLS rules and cache would be isolated by the final SQL query, so different RLS configs should not overlap.

Anyways, I'll be removing this section -- do you think I should also be removing the parameter from the macro so it always add the result to cache keys?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the macro returns RLS rules and cache would be isolated by the final SQL query, so different RLS configs should not overlap.

But the cache is based on the pre-rendered query, no? At least that's what I understood from @john-bodley's original PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

right, but it should still consider the RLS filters at the end, so you would only get the same query if you effectively have the same RLS config as the previous user.

Anyways, let's err on the side of cautious here, so I'll:

  1. Remove this from the docs.
  2. Remove support to the add_to_cache_keys param.
  3. Always do self.cache_key_wrapper(json.dumps(rls_rules)) in case rls_rules has a value.

Copy link
Copy Markdown
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

Awesome!

@Vitor-Avila Vitor-Avila merged commit fdea4e2 into master May 29, 2025
48 checks passed
@Vitor-Avila Vitor-Avila deleted the feat/current_user_rls_rules-jinja-macro branch May 29, 2025 14:58
LevisNgigi pushed a commit to LevisNgigi/superset that referenced this pull request Jun 18, 2025
alexandrusoare pushed a commit to alexandrusoare/superset that referenced this pull request Jun 19, 2025
@github-actions github-actions Bot added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 6.0.0 First shipped in 6.0.0 labels Dec 18, 2025
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

authentication:row-level-security Related to Row Level Security 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels doc Namespace | Anything related to documentation global:jinja Related to Jinja templating size/L 🚢 6.0.0 First shipped in 6.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants