fix: Row Level Security get_rls_filters func SELECT statement

[axelet]

CATEGORY

Choose one

• Bug Fix
• Enhancement (new features, refinement)
• Refactor
• Add tests
• Build / Development Environment
• Documentation

SUMMARY

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

TEST PLAN

ADDITIONAL INFORMATION

• 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

[codecov-io]

Codecov Report

Merging #9365 into master will not change coverage by %.
The diff coverage is n/a.

@@           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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f51ab59...da04d0e. Read the comment docs.

[altef]

Looks good to me!

[villebro]

Good catch @axelet . Given that this is a security feature, joining on the wrong keys here could potentially have serious implications. I'm wondering if we could add a unit test to catch stuff like this?

[axelet]

@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.

[axelet]

@villebro Hey, I've fixed all 'black' warns. And could you please review it one more time, so we can probably merge it then?

[villebro]

@axelet thanks for this, looks good. I restarted the tests (CI has been flaky lately).

[axelet]

@villebro Yeah, I've checked this one b8f303b on Friday and it passed, except the 'black' one https://travis-ci.org/github/apache/incubator-superset/builds/670666636.

So I reformatted this b8f303b#diff-33c3a8579fb333eaeb6e1f9b36eb7d0eR840-R842 line and at least 'black' passes now.

[amitmiran137]

hey @villebro
is this going to be merged as part of 0.36 ?
the row level security feature does not work without this fix

[villebro]

@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.

[villebro]

@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..

[axelet]

@villebro Done, but seems like it didn't work out too... 😅

[akushniarevich-gd]

@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?

[villebro]

@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.

[villebro]

@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.