You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "villebro (via GitHub)" <gi...@apache.org> on 2023/05/19 07:15:45 UTC
[GitHub] [superset] villebro opened a new pull request, #24128: chore: deprecate rls base related filters
villebro opened a new pull request, #24128:
URL: https://github.com/apache/superset/pull/24128
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Currently Row Level Security has a dedicated set of related base filters that makes it possible to customize the filters that are applied on the Table and Role models. Having these undefined by default is counter intuitive, as it means, that a user that is given RLS access will be able to see ALL datasets under the RLS table dropdown, irrespective of which tables they see under the Datasets menu. To streamline this we should just use the same base filter we use for the dataset list view (`DatasourceFilter`).
For roles, the PR #22526 introduced the possibility to define a base filter for Roles. To avoid having duplicated logic and to give the users a unified UX, we can reuse the Role filter that users can define via the `EXTRA_RELATED_QUERY_FILTERS` config flag.
### SCREENSHOTS
This is the datasets view for a user that is only able to access two datasets on the instance.
With the `DASHBOARD_RBAC` feature enabled and a custom Roles filter, the user was only able to see the `Team-BurgerKing` role:
Previously, the same user would see all tables and roles on the instance if they had RLS perms. After this change the RLS dropdowns are aligned with what's available elsewhere:
RLS tables:
RLS roles:
### 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:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
[GitHub] [superset] villebro commented on a diff in pull request #24128: chore: deprecate rls base related filters
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #24128:
URL: https://github.com/apache/superset/pull/24128#discussion_r1199003202
##########
UPDATING.md:
##########
@@ -24,6 +24,7 @@ assists people when migrating to a new version.
## Next
+- [24128](https://github.com/apache/superset/pull/24128) The `RLS_BASE_RELATED_FIELD_FILTERS` config parameter has been removed. Now the Tables dropdown will feature the same tables that the user is able to see elsewhere in the application using the standard `DatasourceFilter`, and the Roles dropdown will be filtered using the filter defined in `EXTRA_RELATED_QUERY_FILTERS["role"]`.
Review Comment:
Oof, good catch!
--
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
[GitHub] [superset] villebro merged pull request #24128: chore: deprecate rls base related filters
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro merged PR #24128:
URL: https://github.com/apache/superset/pull/24128
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #24128: chore: deprecate rls base related filters
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #24128:
URL: https://github.com/apache/superset/pull/24128#discussion_r1198952338
##########
UPDATING.md:
##########
@@ -24,6 +24,7 @@ assists people when migrating to a new version.
## Next
+- [24128](https://github.com/apache/superset/pull/24128) The `RLS_BASE_RELATED_FIELD_FILTERS` config parameter has been removed. Now the Tables dropdown will feature the same tables that the user is able to see elsewhere in the application using the standard `DatasourceFilter`, and the Roles dropdown will be filtered using the filter defined in `EXTRA_RELATED_QUERY_FILTERS["role"]`.
Review Comment:
Could you move this to the **Breaking Changes** section?
--
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
[GitHub] [superset] codecov[bot] commented on pull request #24128: chore: deprecate rls base related filters
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24128:
URL: https://github.com/apache/superset/pull/24128#issuecomment-1554153244
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24128?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#24128](https://app.codecov.io/gh/apache/superset/pull/24128?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (79f4b1d) into [master](https://app.codecov.io/gh/apache/superset/commit/6b5459121f9542c9f0976aec1ebdfe7b1a078472?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (6b54591) will **decrease** coverage by `11.35%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head 79f4b1d differs from pull request most recent head 9942789. Consider uploading reports for the commit 9942789 to get more accurate results
```diff
@@ Coverage Diff @@
## master #24128 +/- ##
===========================================
- Coverage 68.27% 56.93% -11.35%
===========================================
Files 1952 1952
Lines 75468 75469 +1
Branches 8202 8202
===========================================
- Hits 51529 42966 -8563
- Misses 21832 30396 +8564
Partials 2107 2107
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `53.10% <100.00%> (+<0.01%)` | :arrow_up: |
| python | `59.34% <100.00%> (-23.47%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `53.09% <100.00%> (+<0.01%)` | :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.
| [Impacted Files](https://app.codecov.io/gh/apache/superset/pull/24128?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [superset/config.py](https://app.codecov.io/gh/apache/superset/pull/24128?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `91.42% <ø> (-0.60%)` | :arrow_down: |
| [superset/row\_level\_security/api.py](https://app.codecov.io/gh/apache/superset/pull/24128?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcm93X2xldmVsX3NlY3VyaXR5L2FwaS5weQ==) | `61.16% <100.00%> (-34.88%)` | :arrow_down: |
... and [305 files with indirect coverage changes](https://app.codecov.io/gh/apache/superset/pull/24128/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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