You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org> on 2025/03/18 11:03:25 UTC
[PR] feat(rls): Add Groups association to RLS [superset]
Antonio-RiveroMartnez opened a new pull request, #32718:
URL: https://github.com/apache/superset/pull/32718
### SUMMARY
We recently introduced the `Groups` [with this PR](https://github.com/apache/superset/pull/32121), these changes are adding support for them on the `RLS` rules. 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
<img width="481" alt="Screenshot 2025-03-18 at 11 47 14 AM" src="https://github.com/user-attachments/assets/44245af3-62b2-419f-93f9-590c1378b003" />
##### Base rule
<img width="481" alt="Screenshot 2025-03-18 at 11 47 26 AM" src="https://github.com/user-attachments/assets/0e046584-510b-4e10-9f1f-29cda90d57b6" />
#### Rules being applied based on groups from direct user <-> group association and rule being linked to the group
<img width="1648" alt="Screenshot 2025-03-18 at 11 54 34 AM" src="https://github.com/user-attachments/assets/6139354f-66a9-4854-a6ec-4d3efd07e8a8" />
#### 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
<img width="480" alt="Screenshot 2025-03-18 at 11 58 38 AM" src="https://github.com/user-attachments/assets/6728d29a-b194-4334-89a9-775064f68fff" />
##### Group details
<img width="480" alt="Screenshot 2025-03-18 at 11 58 53 AM" src="https://github.com/user-attachments/assets/c6b2a151-5943-457e-9b90-13879084f697" />
##### Rule applied
<img width="1660" alt="Screenshot 2025-03-18 at 11 59 52 AM" src="https://github.com/user-attachments/assets/e445c110-62a4-4f1a-b27f-5527421475f6" />
### TESTING INSTRUCTIONS
1. Create group(s) using the existing functionality
2. Access the Row Level Security menu and associate the group(s) you created
3. Verify the clause gets into the generated query when for example creating a chart using a restricted table
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [X] Changes UI
- [X] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [X] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32718:
URL: https://github.com/apache/superset/pull/32718#issuecomment-2741846953
@eschutho Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/13980969259). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #32718:
URL: https://github.com/apache/superset/pull/32718#issuecomment-2733840081
@dpgaspar I agree with @villebro's comment that we should not have separate selects for each entity. We should introduce the Subject model and be able to select any subject type for RLS.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32718:
URL: https://github.com/apache/superset/pull/32718#discussion_r2000791971
##########
superset/connectors/sqla/models.py:
##########
@@ -2099,6 +2099,14 @@ def load_database(self: SqlaTable) -> None:
Column("rls_filter_id", Integer, ForeignKey("row_level_security_filters.id")),
)
+RLSFilterGroups = DBTable(
+ "rls_filter_groups",
+ metadata,
+ Column("id", Integer, primary_key=True),
+ Column("group_id", Integer, ForeignKey("ab_group.id")),
+ Column("rls_filter_id", Integer, ForeignKey("row_level_security_filters.id")),
+)
Review Comment:
### Missing unique constraint on RLS filter group assignments <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The Row Level Security (RLS) filter groups table lacks an index on the combination of group_id and rls_filter_id columns which could enable faster security policy lookups and prevent duplicate assignments.
###### Why this matters
Without a unique index on (group_id, rls_filter_id), the same RLS filter could be assigned multiple times to the same group, potentially causing performance issues during policy evaluation and inconsistent security enforcement.
###### Suggested change ∙ *Feature Preview*
```python
RLSFilterGroups = DBTable(
"rls_filter_groups",
metadata,
Column("id", Integer, primary_key=True),
Column("group_id", Integer, ForeignKey("ab_group.id")),
Column("rls_filter_id", Integer, ForeignKey("row_level_security_filters.id")),
UniqueConstraint("group_id", "rls_filter_id")
)
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4a72b5b5-8b48-4a90-97c9-d81991428aad?suggestedFixEnabled=true)
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:77b321ac-c998-4163-8915-3c9e8af6f99a -->
##########
superset/migrations/versions/2025-03-13_16-26_19631c6eb7fc_add_relationship_between_rls_filters_.py:
##########
@@ -0,0 +1,51 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Add relationship between RLS filters and groups
+
+Revision ID: 19631c6eb7fc
+Revises: 94e7a3499973
+Create Date: 2025-03-13 16:26:20.944441
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "19631c6eb7fc"
+down_revision = "94e7a3499973"
+
+import sqlalchemy as sa
+from alembic import op
+
+
+def upgrade():
+ op.create_table(
+ "rls_filter_groups",
+ sa.Column("id", sa.Integer(), primary_key=True),
+ sa.Column(
+ "rls_filter_id",
+ sa.Integer(),
+ sa.ForeignKey("row_level_security_filters.id"),
+ nullable=False,
+ ),
+ sa.Column(
+ "group_id", sa.Integer(), sa.ForeignKey("ab_group.id"), nullable=False
+ ),
+ sa.UniqueConstraint("rls_filter_id", "group_id", name="uq_rls_filter_group"),
+ )
+
+
+def downgrade():
+ op.drop_table("rls_filter_groups")
Review Comment:
### Missing FK Constraint Handling in Downgrade <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The downgrade function doesn't handle potential foreign key constraints that might prevent the table from being dropped if there are existing relationships.
###### Why this matters
If there are existing relationships in production, the migration could fail during rollback, leaving the database in an inconsistent state.
###### Suggested change ∙ *Feature Preview*
Add foreign key constraint drops before dropping the table in the downgrade function:
```python
def downgrade():
# Drop foreign key constraints first
with op.batch_alter_table('rls_filter_groups') as batch_op:
batch_op.drop_constraint('uq_rls_filter_group', type_='unique')
batch_op.drop_constraint('rls_filter_groups_rls_filter_id_fkey', type_='foreignkey')
batch_op.drop_constraint('rls_filter_groups_group_id_fkey', type_='foreignkey')
op.drop_table('rls_filter_groups')
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/87103718-941e-4494-bb3f-37a0931694ef?suggestedFixEnabled=true)
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a7c6485a-73dd-4c69-b9a0-d25593901a85 -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on PR #32718:
URL: https://github.com/apache/superset/pull/32718#issuecomment-2741130572
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on code in PR #32718:
URL: https://github.com/apache/superset/pull/32718#discussion_r2001307331
##########
superset/security/manager.py:
##########
@@ -2473,32 +2481,73 @@ def get_rls_filters(self, table: "BaseDatasource") -> list[SqlaQuery]:
return []
# pylint: disable=import-outside-toplevel
+ from flask_appbuilder.security.sqla.models import assoc_group_role
Review Comment:
nit: we could move this one to a top level import
##########
superset/connectors/sqla/models.py:
##########
@@ -2099,6 +2099,25 @@ def load_database(self: SqlaTable) -> None:
Column("rls_filter_id", Integer, ForeignKey("row_level_security_filters.id")),
)
+RLSFilterGroups = DBTable(
+ "rls_filter_groups",
+ metadata,
+ Column("id", Integer, primary_key=True),
+ Column(
+ "group_id",
+ Integer,
+ ForeignKey("ab_group.id", name="rls_filter_groups_group_id_fkey"),
+ ),
+ Column(
+ "rls_filter_id",
+ Integer,
+ ForeignKey(
+ "row_level_security_filters.id", name="rls_filter_groups_rls_filter_id_fkey"
+ ),
+ ),
+ UniqueConstraint("group_id", "rls_filter_id", name="uq_rls_filter_group"),
+)
Review 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?
##########
superset/security/manager.py:
##########
@@ -2473,32 +2481,73 @@ def get_rls_filters(self, table: "BaseDatasource") -> list[SqlaQuery]:
return []
# pylint: disable=import-outside-toplevel
+ from flask_appbuilder.security.sqla.models import assoc_group_role
+
from superset.connectors.sqla.models import (
+ RLSFilterGroups,
RLSFilterRoles,
RLSFilterTables,
RowLevelSecurityFilter,
)
+ # Roles and Groups directly linked to the user
user_roles = [role.id for role in self.get_user_roles(g.user)]
Review Comment:
`get_user_roles(g.user)` will get all roles that are assigned to a user, directly or indirectly via groups
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on PR #32718:
URL: https://github.com/apache/superset/pull/32718#issuecomment-2736247451
> @Antonio-RiveroMartnez I think it was my oversight not to add RLS as part of the Subject model overhaul (I wrote the [bullet list](https://github.com/apache/superset/issues/32116#issuecomment-2643567384) 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 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.
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. Is it a perfect solution? No. But major changes rarely follow a straight, clean path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #32718:
URL: https://github.com/apache/superset/pull/32718#discussion_r2001348227
##########
superset/connectors/sqla/models.py:
##########
@@ -2099,6 +2099,25 @@ def load_database(self: SqlaTable) -> None:
Column("rls_filter_id", Integer, ForeignKey("row_level_security_filters.id")),
)
+RLSFilterGroups = DBTable(
+ "rls_filter_groups",
+ metadata,
+ Column("id", Integer, primary_key=True),
+ Column(
+ "group_id",
+ Integer,
+ ForeignKey("ab_group.id", name="rls_filter_groups_group_id_fkey"),
+ ),
+ Column(
+ "rls_filter_id",
+ Integer,
+ ForeignKey(
+ "row_level_security_filters.id", name="rls_filter_groups_rls_filter_id_fkey"
+ ),
+ ),
+ UniqueConstraint("group_id", "rls_filter_id", name="uq_rls_filter_group"),
+)
Review 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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #32718:
URL: https://github.com/apache/superset/pull/32718#discussion_r2001348227
##########
superset/connectors/sqla/models.py:
##########
@@ -2099,6 +2099,25 @@ def load_database(self: SqlaTable) -> None:
Column("rls_filter_id", Integer, ForeignKey("row_level_security_filters.id")),
)
+RLSFilterGroups = DBTable(
+ "rls_filter_groups",
+ metadata,
+ Column("id", Integer, primary_key=True),
+ Column(
+ "group_id",
+ Integer,
+ ForeignKey("ab_group.id", name="rls_filter_groups_group_id_fkey"),
+ ),
+ Column(
+ "rls_filter_id",
+ Integer,
+ ForeignKey(
+ "row_level_security_filters.id", name="rls_filter_groups_rls_filter_id_fkey"
+ ),
+ ),
+ UniqueConstraint("group_id", "rls_filter_id", name="uq_rls_filter_group"),
+)
Review Comment:
This is the association table so we should instead make the `group_id` not nullable in the `RLSFilterGroups`, like in the migration
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #32718:
URL: https://github.com/apache/superset/pull/32718#issuecomment-2734066289
@Antonio-RiveroMartnez I think it was my oversight not to add RLS as part of the Subject model overhaul (I wrote the [bullet list](https://github.com/apache/superset/issues/32116#issuecomment-2643567384) of entities that will be changed that was later merged into the final SIP description). I totally assumed RSL 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 was 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 having 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on PR #32718:
URL: https://github.com/apache/superset/pull/32718#issuecomment-2733968038
Hey @villebro @michael-s-molina, totally agree on bringing the `Subject` model soon 🙏 but seems to me that it was considered as part of a phase 2 in [SIP-152](https://github.com/apache/superset/issues/32116)? While the `Group` entity in RLS would be still considered phase 1 since it's just a complementary element of the initial PR thus it could potentially be added now? What do you guys think? Thanks for looking into this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #32718:
URL: https://github.com/apache/superset/pull/32718#issuecomment-2741868862
> @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.
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on PR #32718:
URL: https://github.com/apache/superset/pull/32718#issuecomment-2743080229
> > @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.
>
> 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, it would probably require 2 migrations to get to our final destination (depends on the specifics of the implementation itself). But on the bright side, it would fully introduce `Groups` to Superset, that's 1 step forward to subjects, also I don't believe subjects implementation is right around the corner for multiple reasons, and I do think it's positive to get Superset to a state where `Groups` is a first class entity.
@eschutho proposal, could be a good compromise because, it would not introduce a new field to the RLS form addressing @michael-s-molina concern here: https://github.com/apache/superset/pull/32718#issuecomment-2734060623
Another alternative would be to feature flag this dropdown, not my first choice.
Our goal here is to get Phase 1 to be a stable and complete point regarding data authorization and RLS is an important feature on data authorization. Our users can't fully adopt `Groups` without RLS.
I also believe that introducing subject models and all it requires at this point is premature and would risk a refactor
I do agree and I'm fully onboard on stoping unnecessary tech debt, but I think this change is manageable and required, it's a compromise, also the next step is planed and approved on [SIP-152](https://github.com/apache/superset/issues/32116) that incorporated [SIP-126](https://github.com/apache/superset/issues/28021)
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 `Subjects` implementation isn't imminent for several reasons, making Groups a first-class entity now makes sense (and was approved).
@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](https://github.com/apache/superset/pull/32718#issuecomment-2734060623). Another alternative could be feature-flagging the dropdown, though that wouldn't be my first choice.
Our primary goal is to ensure Phase 1 is a stable and complete step forward in data authorization, with RLS playing a crucial role. Our users can't fully adopt `Groups` without RLS, making this change both necessary and timely.
Introducing now all the necessary backend implementation for Subjects is premature and could risk a refactor, since
I agree that we should avoid unnecessary tech debt, but I believe this change is small contained and justified. It strikes a balance between progress and stability, especially considering that the next step has already been planned and approved in [SIP-152](https://github.com/apache/superset/issues/32116), which builds upon [SIP-126](https://github.com/apache/superset/issues/28021).
Unless there are strong objections, I think this is a reasonable path forward.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #32718:
URL: https://github.com/apache/superset/pull/32718#issuecomment-2732769076
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/32718?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `58.82353%` with `21 lines` in your changes missing coverage. Please review.
> Project coverage is 65.75%. Comparing base [(`76d897e`)](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`3d6c8b5`)](https://app.codecov.io/gh/apache/superset/commit/3d6c8b59d0551243c5a0117651d8a0e2f1eaebfd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1631 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/32718?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/commands/utils.py](https://app.codecov.io/gh/apache/superset/pull/32718?src=pr&el=tree&filepath=superset%2Fcommands%2Futils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvdXRpbHMucHk=) | 25.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/32718?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/row\_level\_security/api.py](https://app.codecov.io/gh/apache/superset/pull/32718?src=pr&el=tree&filepath=superset%2Frow_level_security%2Fapi.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcm93X2xldmVsX3NlY3VyaXR5L2FwaS5weQ==) | 14.28% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/32718?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/commands/security/create.py](https://app.codecov.io/gh/apache/superset/pull/32718?src=pr&el=tree&filepath=superset%2Fcommands%2Fsecurity%2Fcreate.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvc2VjdXJpdHkvY3JlYXRlLnB5) | 25.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/32718?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/commands/security/update.py](https://app.codecov.io/gh/apache/superset/pull/32718?src=pr&el=tree&filepath=superset%2Fcommands%2Fsecurity%2Fupdate.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvc2VjdXJpdHkvdXBkYXRlLnB5) | 40.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/32718?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/security/manager.py](https://app.codecov.io/gh/apache/superset/pull/32718?src=pr&el=tree&filepath=superset%2Fsecurity%2Fmanager.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvc2VjdXJpdHkvbWFuYWdlci5weQ==) | 84.61% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/32718?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/commands/exceptions.py](https://app.codecov.io/gh/apache/superset/pull/32718?src=pr&el=tree&filepath=superset%2Fcommands%2Fexceptions.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvZXhjZXB0aW9ucy5weQ==) | 75.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/32718?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #32718 +/- ##
==========================================
+ Coverage 60.48% 65.75% +5.27%
==========================================
Files 1931 549 -1382
Lines 76236 39477 -36759
Branches 8568 0 -8568
==========================================
- Hits 46114 25960 -20154
+ Misses 28017 13517 -14500
+ Partials 2105 0 -2105
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/32718/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/32718/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `48.42% <39.21%> (-0.75%)` | :arrow_down: |
| [javascript](https://app.codecov.io/gh/apache/superset/pull/32718/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/32718/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.92% <58.82%> (-0.88%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/32718/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `65.75% <58.82%> (+2.27%)` | :arrow_up: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/32718/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.37% <58.82%> (+3.74%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/32718?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary>🚀 New features to boost your workflow: </summary>
- ❄ [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- 📦 [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #32718:
URL: https://github.com/apache/superset/pull/32718#issuecomment-2733810399
@Antonio-RiveroMartnez @dpgaspar as proposed in [SIP-126](https://github.com/apache/superset/issues/28021), which was integrated into [SIP-152](https://github.com/apache/superset/issues/32116), 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez closed pull request #32718: feat(rls): Add Groups association to RLS
URL: https://github.com/apache/superset/pull/32718
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32718:
URL: https://github.com/apache/superset/pull/32718#issuecomment-2741856317
@eschutho Ephemeral environment spinning up at http://35.161.55.247:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #32718:
URL: https://github.com/apache/superset/pull/32718#issuecomment-2737389563
> 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.
@dpgaspar I'm probably missing something, as I assumed RLS should work as before without this change. Is there something in 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on PR #32718:
URL: https://github.com/apache/superset/pull/32718#issuecomment-2735752497
@villebro @michael-s-molina If `Groups` are meant to be introduced in `Phase 1`, shouldn’t they also work within RLS to be fully integrated? otherwise, they are partially usable. Also, as things stand, a refactor will be needed when we introduce the `Subject` entity regardless of `Groups` being part of `RLS` or not, but I see that we would be adding extra work on top of that.
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 `Phase 1 vs Phase 2` approach so I can contribute in the best way possible.
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #32718:
URL: https://github.com/apache/superset/pull/32718#issuecomment-2734060623
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(rls): Add Groups association to RLS [superset]
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on PR #32718:
URL: https://github.com/apache/superset/pull/32718#issuecomment-2744594102
Closing this PR in favor of the `Subjects` entity coming in `Phase 2`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org