You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/03/24 12:15:01 UTC
[GitHub] [incubator-superset] axelet opened a new pull request #9365: [WIP]
fix: Row Level Security get_rls_filters func SELECT statement
axelet opened a new pull request #9365: [WIP] fix: Row Level Security get_rls_filters func SELECT statement
URL: https://github.com/apache/incubator-superset/pull/9365
### CATEGORY
Choose one
- [x] Bug Fix
- [ ] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
It appears that **get_rls_filters()** func have the following SELECT statement issue:
**filter_roles** subquery should select **rls_filter_id** (corresponding to existing **role_id**) instead of selecting the **id** of (role_id, rls_filter_id) pair. This way the proper filters can then be selected from **row_level_security_filters** table.
The query in **get_rls_filters()** should look as follows:
SELECT row_level_security_filters.id AS row_level_security_filters_id, row_level_security_filters.clause AS row_level_security_filters_clause
FROM row_level_security_filters
WHERE row_level_security_filters.table_id = %(table_id_1)s AND row_level_security_filters.id IN (SELECT rls_filter_roles.rls_filter_id
FROM rls_filter_roles
WHERE rls_filter_roles.role_id IN (SELECT ab_user_role.role_id
FROM ab_user_role
WHERE ab_user_role.user_id = %(user_id_1)s))
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9365: fix: Row
Level Security get_rls_filters func SELECT statement
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9365: fix: Row Level Security get_rls_filters func SELECT statement
URL: https://github.com/apache/incubator-superset/pull/9365#issuecomment-609738614
@axelet thanks for this, looks good. I restarted the tests (CI has been flaky lately).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #9365: [WIP]
fix: Row Level Security get_rls_filters func SELECT statement
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9365: [WIP] fix: Row Level Security get_rls_filters func SELECT statement
URL: https://github.com/apache/incubator-superset/pull/9365#issuecomment-603207943
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9365?src=pr&el=h1) Report
> Merging [#9365](https://codecov.io/gh/apache/incubator-superset/pull/9365?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/f51ab59748a72e57dc3f57084ecb8532e139f30d&el=desc) will **not change** coverage by `%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9365?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9365 +/- ##
=======================================
Coverage 59.00% 59.00%
=======================================
Files 374 374
Lines 12136 12136
Branches 2989 2989
=======================================
Hits 7161 7161
Misses 4796 4796
Partials 179 179
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9365?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Ξ = absolute <relative> (impact)`, `ΓΈ = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9365?src=pr&el=footer). Last update [f51ab59...da04d0e](https://codecov.io/gh/apache/incubator-superset/pull/9365?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] axelet commented on issue #9365: fix: Row
Level Security get_rls_filters func SELECT statement
Posted by GitBox <gi...@apache.org>.
axelet commented on issue #9365: fix: Row Level Security get_rls_filters func SELECT statement
URL: https://github.com/apache/incubator-superset/pull/9365#issuecomment-606166493
@altef @villebro So, I see we already have the **RowLevelSecurityTests** case and the **test_rls_filter_alters_query** test should catch the scenario with wrong **id** used imo. Or should we consider adding a new test?
We can make the case more general. I've altered a bit the test case in the **setUp** function, so it now adds 1 filter for 2 roles (2 role_filter relations in the **rls_filter_roles** table). And I also changed the **test_rls_filter_alters_query** test, so the user is now 'alpha' (role_id = 3), not 'gamma' (role_id = 4). This way the relation_id will not accidentally match the rls_filter_id.
The **rls_filter_roles** table at this point looksq like:
```
id | role_id | rls_filter_id
1 | 4 | 1
2 | 3 | 1
```
PS. I also changed the clause to "gender = 'boy'" for consistency with the birth_names table (it only has 'boy' and 'girl' values in this column). It's not necessary, but to avoid confusion it will be useful I guess.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] axelet commented on issue #9365: fix: Row
Level Security get_rls_filters func SELECT statement
Posted by GitBox <gi...@apache.org>.
axelet commented on issue #9365: fix: Row Level Security get_rls_filters func SELECT statement
URL: https://github.com/apache/incubator-superset/pull/9365#issuecomment-609732620
@villebro Hey, I've fixed all 'black' warns. And could you please review it one more time, so we can probably merge it then?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] amitNielsen commented on issue #9365: fix: Row
Level Security get_rls_filters func SELECT statement
Posted by GitBox <gi...@apache.org>.
amitNielsen commented on issue #9365: fix: Row Level Security get_rls_filters func SELECT statement
URL: https://github.com/apache/incubator-superset/pull/9365#issuecomment-609994860
hey @villebro
is this going to be merged as part of **0.36** ?
the row level security feature does **not** work without this fix
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9365: fix: Row
Level Security get_rls_filters func SELECT statement
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9365: fix: Row Level Security get_rls_filters func SELECT statement
URL: https://github.com/apache/incubator-superset/pull/9365#issuecomment-610325464
@axelet would you mind rebasing and force pushing? It seems GitHub is having trouble picking up that Travis is passing, and closing/opening didn't seem to help..
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] axelet opened a new pull request #9365: fix:
Row Level Security get_rls_filters func SELECT statement
Posted by GitBox <gi...@apache.org>.
axelet opened a new pull request #9365: fix: Row Level Security get_rls_filters func SELECT statement
URL: https://github.com/apache/incubator-superset/pull/9365
### CATEGORY
Choose one
- [x] Bug Fix
- [ ] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
It appears that **get_rls_filters()** func have the following SELECT statement issue:
The **filter_roles** subquery should select **rls_filter_id** (corresponding to existing **role_id**) instead of selecting the **id** of (role_id, rls_filter_id) pair. This way the proper filters can then be selected from **row_level_security_filters** table.
The query in **get_rls_filters()** should look as follows:
SELECT row_level_security_filters.id AS row_level_security_filters_id, row_level_security_filters.clause AS row_level_security_filters_clause
FROM row_level_security_filters
WHERE row_level_security_filters.table_id = %(table_id_1)s AND row_level_security_filters.id IN (SELECT rls_filter_roles.**rls_filter_id**
FROM rls_filter_roles
WHERE rls_filter_roles.role_id IN (SELECT ab_user_role.role_id
FROM ab_user_role
WHERE ab_user_role.user_id = %(user_id_1)s))
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue: no issue
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9365: fix: Row
Level Security get_rls_filters func SELECT statement
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9365: fix: Row Level Security get_rls_filters func SELECT statement
URL: https://github.com/apache/incubator-superset/pull/9365#issuecomment-610229545
@amitNielsen unfortunately this fix didn't make the `0.36.0rc3` cut, which I believe will become the official `0.36.0` release. While it would be great to get this into `0.36.0`, in the interest of getting the long overdue release out, I propose going forward with `0.36.0rc3` and addressing this fix in a later release. While I understand this may not be optimal for those intending to use RLS, the feature is still hidden behind a feature flag in `config.py`, and is not considered stable at this point.
Having said that, I don't mind working towards getting a `0.36.1` release out in the near future, in which this fix will most certainly be included.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] axelet edited a comment on issue #9365: fix:
Row Level Security get_rls_filters func SELECT statement
Posted by GitBox <gi...@apache.org>.
axelet edited a comment on issue #9365: fix: Row Level Security get_rls_filters func SELECT statement
URL: https://github.com/apache/incubator-superset/pull/9365#issuecomment-606166493
@altef @villebro So, I see we already have the **RowLevelSecurityTests** case and the **test_rls_filter_alters_query** test should catch the scenario with wrong **id** used imo. Or should we consider adding a new test?
We can make the case more general. I've altered a bit the test case in the **setUp** function, so it now adds 1 filter for 2 roles (2 role_filter relations in the **rls_filter_roles** table). And I also changed the **test_rls_filter_alters_query** test, so the user is now 'alpha' (role_id = 3), not 'gamma' (role_id = 4). This way the relation_id will not accidentally match the rls_filter_id.
The **rls_filter_roles** table at this point looks like:
```
id | role_id | rls_filter_id
1 | 4 | 1
2 | 3 | 1
```
PS. I also changed the clause to "gender = 'boy'" for consistency with the birth_names table (it only has 'boy' and 'girl' values in this column). It's not necessary, but to avoid confusion it will be useful I guess.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro closed pull request #9365: fix: Row
Level Security get_rls_filters func SELECT statement
Posted by GitBox <gi...@apache.org>.
villebro closed pull request #9365: fix: Row Level Security get_rls_filters func SELECT statement
URL: https://github.com/apache/incubator-superset/pull/9365
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9365: fix: Row
Level Security get_rls_filters func SELECT statement
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9365: fix: Row Level Security get_rls_filters func SELECT statement
URL: https://github.com/apache/incubator-superset/pull/9365#issuecomment-613616055
@axelet would you mind reopening this as a new PR? Travis/GH has had some serious trouble lately, but things seem better on freshly opened PRs.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] altef commented on issue #9365: fix: Row Level
Security get_rls_filters func SELECT statement
Posted by GitBox <gi...@apache.org>.
altef commented on issue #9365: fix: Row Level Security get_rls_filters func SELECT statement
URL: https://github.com/apache/incubator-superset/pull/9365#issuecomment-604025074
Looks good to me!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] akushniarevich-gd commented on issue #9365:
fix: Row Level Security get_rls_filters func SELECT statement
Posted by GitBox <gi...@apache.org>.
akushniarevich-gd commented on issue #9365: fix: Row Level Security get_rls_filters func SELECT statement
URL: https://github.com/apache/incubator-superset/pull/9365#issuecomment-613484266
@villebro Hey, so, do we have any known solution for this Github check issue or can it be merged without the success mark as well? Or maybe we should recreate the PR?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] axelet closed pull request #9365: fix: Row
Level Security get_rls_filters func SELECT statement
Posted by GitBox <gi...@apache.org>.
axelet closed pull request #9365: fix: Row Level Security get_rls_filters func SELECT statement
URL: https://github.com/apache/incubator-superset/pull/9365
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] axelet commented on issue #9365: fix: Row
Level Security get_rls_filters func SELECT statement
Posted by GitBox <gi...@apache.org>.
axelet commented on issue #9365: fix: Row Level Security get_rls_filters func SELECT statement
URL: https://github.com/apache/incubator-superset/pull/9365#issuecomment-609748783
@villebro Yeah, I've checked this one https://github.com/apache/incubator-superset/commit/b8f303b5c7f87211365d43915e4ad1544dd9eb60 on Friday and it passed, except the 'black' one https://travis-ci.org/github/apache/incubator-superset/builds/670666636.
So I reformatted this https://github.com/apache/incubator-superset/commit/b8f303b5c7f87211365d43915e4ad1544dd9eb60#diff-33c3a8579fb333eaeb6e1f9b36eb7d0eR840-R842 line and at least 'black' passes now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9365: fix: Row
Level Security get_rls_filters func SELECT statement
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9365: fix: Row Level Security get_rls_filters func SELECT statement
URL: https://github.com/apache/incubator-superset/pull/9365#issuecomment-613641988
@axelet I'm afraid the only solution for now is closing this and opening a new one π but do put a link to this PR in the new one so others can find the discussions here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] axelet commented on issue #9365: fix: Row
Level Security get_rls_filters func SELECT statement
Posted by GitBox <gi...@apache.org>.
axelet commented on issue #9365: fix: Row Level Security get_rls_filters func SELECT statement
URL: https://github.com/apache/incubator-superset/pull/9365#issuecomment-610477553
@villebro Done, but seems like it didn't work out too... π
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org