feat(rls): Add Groups association to RLS#32718
feat(rls): Add Groups association to RLS#32718Antonio-RiveroMartnez wants to merge 7 commits intoapache:masterfrom
Conversation
- Add new table to hold many to many relation between rls and groups - Support groups as part of rls APIs - Consider groups when fetching rls filters - Update the UI to allow group management for rls
- Tests for UI changes
…valid - Move group query to util method and use it from commands - Adjust schemas for API - Consider resolving RLS clauses with roles coming from groups and viceversa - Add integration tests to our RLS APIs
Antonio-RiveroMartnez
requested review from
betodealmeida,
eschutho,
michael-s-molina and
mistercrunch
as code owners
dosubot
Bot
added
authentication:row-level-security
github-actions
Bot
added
risk:db-migration
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Fix Detected |
|---|---|---|
 |
Missing unique constraint on RLS filter group assignments ▹ view | ✅ |
 |
Missing FK Constraint Handling in Downgrade ▹ view | ✅ |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/features/rls/types.ts | ✅ |
| superset/commands/security/create.py | ✅ |
| superset/migrations/versions/2025-03-13_16-26_19631c6eb7fc_add_relationship_between_rls_filters_.py | ✅ |
| superset/commands/security/update.py | ✅ |
| superset/commands/exceptions.py | ✅ |
| superset/commands/utils.py | ✅ |
| superset/row_level_security/schemas.py | ✅ |
| superset/row_level_security/api.py | ✅ |
| superset-frontend/src/features/rls/RowLevelSecurityModal.tsx | ✅ |
| superset/connectors/sqla/models.py | ✅ |
| superset/security/manager.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Feedback and Support
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #32718 +/- ##
===========================================
+ Coverage 60.48% 83.45% +22.96%
===========================================
Files 1931 549 -1382
Lines 76236 39473 -36763
Branches 8568 0 -8568
===========================================
- Hits 46114 32941 -13173
+ Misses 28017 6532 -21485
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Fix lint on ui test
Antonio-RiveroMartnez
force-pushed
the
rls_with_groups
branch
from
8160d0b to
0b87f78
Compare
| ), | ||
| ), | ||
| UniqueConstraint("group_id", "rls_filter_id", name="uq_rls_filter_group"), | ||
| ) |
There was a problem hiding this comment.
for roles we have:
RLSFilterRoles = DBTable(
"rls_filter_roles",
metadata,
Column("id", Integer, primary_key=True),
Column("role_id", Integer, ForeignKey("ab_role.id"), nullable=False),
Column("rls_filter_id", Integer, ForeignKey("row_level_security_filters.id")),
)
do we need to remove nullable=False now? because we can have a roles or a group?
There was a problem hiding this comment.
This is the association table so we should instead make the group_id not nullable in the RLSFilterGroups, like in the migration and match the existing RLSFilterRoles
| return [] | ||
|
|
||
| # pylint: disable=import-outside-toplevel | ||
| from flask_appbuilder.security.sqla.models import assoc_group_role |
There was a problem hiding this comment.
nit: we could move this one to a top level import
| ) | ||
|
|
||
| # Roles and Groups directly linked to the user | ||
| user_roles = [role.id for role in self.get_user_roles(g.user)] |
There was a problem hiding this comment.
get_user_roles(g.user) will get all roles that are assigned to a user, directly or indirectly via groups
- Remove duplicate import - Leverage get_user_roles that already gives you all the roles of a user no matter if direct or indirect (via groups)
|
@Antonio-RiveroMartnez @dpgaspar as proposed in SIP-126, which was integrated into SIP-152, there was talk of introducing a Subject model that could be either a Role, Group or User. As we start rolling out Groups into the various entity models, it feels introducing that new model + component now would be very beneficial, as otherwise we'll have to duplicate the components (one for roles, one for groups and later one for users). This will become particularly convoluted in the Dashboard and Chart models, if we introduce editors and viewers, in which case there would be 6 dropdowns all together. Doing that work now would avoid making big design overhauls later, and would save development time by not having to migrate to the new model when it's ultimately introduced. Thoughts? |
|
Hey @villebro @michael-s-molina, totally agree on bringing the |
|
I think it will be problematic to introduce another select for group on all screens and later consolidate all selects into one. Why don't we start with a single select given that this is a new feature? In other words, why introduce technical debt when we can actually make the experience simpler by reducing the number of things users need to handle? As @villebro pointed out, with the concepts of Editor and Viewer, we would be talking about 6 selects to handle access. It's important to also consider the cost of doing in one way to later change it. We have docs and training courses that need to be updated. |
|
@Antonio-RiveroMartnez I think it was my oversight not to add RLS as part of the Subject model overhaul (I wrote the bullet list of entities that will be changed that was later merged into the final SIP description). I totally assumed RLS to be part of this work, as the current Role reference in RLS really is a subject reference. The distinction in the phasing here IMO is that Phase 1 is all about introducing Groups, and Phase 2 is all about replacing existing subject-like references with the new Subject model. The fact that we're here essentially simulating the new Subject model via double drop downs is IMO a strong indication that the Subject model + component should be introduced first, as otherwise this change would need to be completely redone in Phase 2. |
|
@villebro @michael-s-molina If I completely understand the concerns you've raised and I truly appreciate your time to review and having this discussion, I'm just trying to fully grasp the Thanks. |
@villebro @michael-s-molina I understand the concerns around aligning RLS with the Subject model. However, as @Antonio-RiveroMartnez pointed out, I also firmly believe that RLS should be introduced in Phase 1, without being tied to the Subject model yet. The core goal of Phase 1 is to introduce Groups in a backward-compatible way, and leaving them out of RLS at this stage would result in an incomplete and inconsistent implementation. Per SIP-152, Phase 1 must leave Superset in a fully functional state with respect to API and datasource authorization, ensuring a smooth transition while we prepare for the breaking changes in Phase 2. Some refactoring will be necessary in Phase 1, but that would be the case regardless of RLS. We will not include ownership or I don’t think adding a new dropdown to RLS is a strong enough reason to either prematurely introduce the Subject model or leave Phase 1 incomplete. This is the last change needed to complete Phase 1. Is it a perfect solution? No. But major changes rarely follow a straight, clean path. |
@dpgaspar I'm probably missing something, as I assumed RLS should work as before without this change. Is there something in API or datasource authorization that has changed with the introduction of groups, causing issues for RLS? If this is indeed the case, then I can understand this intermediate change. But if not, I still feel this change is best suited for Phase 2 to avoid introducing this temporary workaround. |
|
@michael-s-molina @villebro what do you think about us creating a subject dropdown that allows people to select either a group or a role? It will be an intermediary step closer to subjects, and then we wouldn't need to have two dropdowns. |
|
@eschutho Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
|
@eschutho Ephemeral environment spinning up at http://35.161.55.247:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
That would make it impossible to mix and match different subject types. I also assume it would require an extra db migration, by first adding a group reference to RLS, and later replacing that with the final subject reference (likely a new table). |
Yes, implementing this would likely require two migrations to reach our final goal, though the specifics depend on the implementation. However, on the positive side, this approach would fully introduce Groups into Superset, a step toward the broader Subjects concept. Given that a full @eschutho's proposal seems like a reasonable compromise, as it avoids introducing a new field to the RLS form—directly addressing @michael-s-molina's concern here. Another alternative could be feature-flagging the dropdown, though that wouldn't be my first choice. Our goal is to get Phase 1 to be a stable and complete point regarding data authorization and RLS is an important feature on data authorization. Superset users can't fully adopt Groups without RLS. I agree that we should avoid unnecessary tech debt, but I believe this change is small contained and justified. Especially considering that the next step has already been planned and approved in SIP-152, which builds upon SIP-126. I think this is a reasonable path forward. |
|
Closing this PR in favor of the |
6 participants
SUMMARY
We recently introduced the
Groupswith this PR, these changes are adding support for them on theRLSrules. This way it become even more friendly and usable setting rules for users.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Creating a RLS rule
Regular rule
Base rule
Rules being applied based on groups from direct user <-> group association and rule being linked to the group
Rules being applied based on groups from role association (i.e, no direct user <-> group relation, but the group linked to a role and the user linked to that same role)
RLS rule details
Group details
Rule applied
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION