betodealmeida opened a new pull request, #20323: URL: https://github.com/apache/superset/pull/20323
<!--- Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/ Example: fix(dashboard): load charts correctly --> ### SUMMARY <!--- Describe the change below, including rationale and design decisions --> We currently have a feature flag `RLS_IN_SQLLAB` that, when enabled, applies any relevant RLS rules to the SQL executed in SQL Lab. The current approach parses the SQL and extracts all tables being queried. If any of those tables have RLS rules associated with them the parse tree is modified inplace, appending the RLS predicate to the associated `WHERE` clause or creating a new one if needed. @mistercrunch suggested an approach that is more bulletproof: replacing the table reference (eg, `my_table`) with a subquery that contains the RLS (`SELECT * FROM my_table WHERE rls`). This can be done conditionally on databases that support subqueries. I implemented that solution in this PR. Note that this method is probably still not bulletproof. We're still using `sqlparse` to find table references, and this has been error-prone in the past. For example, we had problems where the identifier "RAW" was not detected properly because `sqlparse` considers it a reserved keyword, since it doesn't have separate lists of keywords for each dialect. This PR and the original method are both conservative in cases like these, trying to figure out if a keyword is actually and identifier, and the unit tests cover such cases. But we should consider this feature experimental, and warn users that there are risks associated with using it. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF <!--- Skip this if not applicable --> N/A ### TESTING INSTRUCTIONS <!--- Required! What steps can be taken to manually verify the changes? --> Added unit tests. ### 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
