feat(RLS): RESTful apis and react view for RLS #22325

mayurnewase · 2022-12-03T03:51:00Z

SUMMARY

Update RLS view to use React and REST apis.

this PR only migrates old view to new without any additional features like,

ownership
import export support.
toggle like in alerts/reports to enable/disable RLS rules.
can add this in a separate PR.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Empty state

Add rule modal

Populated list

Bulk select to delete

TESTING INSTRUCTIONS

Before merging this PR, create few RLS rules.
Then merge PR and check they are correctly shown on the new UI.
Delete all, create new and check correctly shown.
test filters in the list view.
Create chart on the table rule is set with the role and check its correctly getting applied.
Set RLS_FILTER_RELATED_FIELDS in the config.py as below

{
    "tables": [["table_name", filters.FilterStartsWith, "birth"]],
    "roles": [["name", filters.FilterContains, "Admin"]]"
}

When adding a new rule, in the table dropdown table names starting with birth should be shown and roles dropdown should only show roles containing text Admin.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- 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

…-rls

superset/row_level_security/schemas.py

superset/row_level_security/api.py

rusackas · 2022-12-06T16:51:56Z

All new files need the Apache license boilerplate at the top. I added it (via a code suggestion) to a couple so you can copy/paste from there.

…e and update rules

codecov · 2022-12-10T10:38:19Z

Codecov Report

Merging #22325 (3bfcb81) into master (b6d39d1) will increase coverage by 0.01%.
The diff coverage is 87.02%.

@@            Coverage Diff             @@
##           master   #22325      +/-   ##
==========================================
+ Coverage   66.91%   66.93%   +0.01%     
==========================================
  Files        1851     1861      +10     
  Lines       70715    71131     +416     
  Branches     7766     7797      +31     
==========================================
+ Hits        47320    47610     +290     
- Misses      21373    21485     +112     
- Partials     2022     2036      +14

Flag	Coverage Δ
hive	`?`
javascript	`53.97% <73.00%> (+0.12%)`	⬆️
mysql	`78.09% <95.07%> (+0.13%)`	⬆️
postgres	`78.16% <95.07%> (+0.13%)`	⬆️
presto	`52.43% <65.49%> (+0.07%)`	⬆️
python	`81.12% <95.07%> (-0.15%)`	⬇️
sqlite	`?`
unit	`51.27% <65.49%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset-frontend/src/views/routes.tsx	`55.00% <50.00%> (-0.27%)`	⬇️
superset/dao/base.py	`92.78% <70.00%> (-2.62%)`	⬇️
...ews/CRUD/rowlevelsecurity/RowLevelSecurityList.tsx	`72.54% <72.54%> (ø)`
...ws/CRUD/rowlevelsecurity/RowLevelSecurityModal.tsx	`73.39% <73.39%> (ø)`
...uperset/row_level_security/commands/bulk_delete.py	`88.00% <88.00%> (ø)`
superset/connectors/sqla/views.py	`87.96% <90.90%> (+0.66%)`	⬆️
superset/row_level_security/commands/update.py	`91.66% <91.66%> (ø)`
superset/row_level_security/api.py	`96.03% <96.03%> (ø)`
...ntend/src/views/CRUD/rowlevelsecurity/constants.ts	`100.00% <100.00%> (ø)`
superset/config.py	`91.51% <100.00%> (+0.02%)`	⬆️
... and 17 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

mayurnewase · 2022-12-10T11:04:12Z

superset/dao/base.py

@@ -188,3 +188,14 @@ def delete(cls, model: Model, commit: bool = True) -> Model:
            db.session.rollback()
            raise DAODeleteFailedError(exception=ex) from ex
        return model
+
+    @classmethod
+    def bulk_delete(cls, models: List[Model], commit: bool = True) -> None:


added on base class, so other apis can reuse it.

mayurnewase · 2023-01-10T02:09:24Z

Hi @mayurnewase! Thanks for this PR! I see you created a new UI page and we have a #22330 discussion about the new structure of UI pages. What do you think about it?

I wanted to know if this discussion/SIP is approved by members?
also the PR you created @EugeneTorap to start this migration isn't merged, so was expecting some formal approval in that discussion.

EugeneTorap · 2023-01-10T07:59:30Z

Hi @mayurnewase! This discussion is in progress and your opinion and feedback is important there!
I guess we'll start migrating the pages in month.

EugeneTorap · 2023-01-12T21:23:35Z

Hey @mayurnewase! If you see my #22230 PR is merged. Can you prepare a PR for migrating RLS page to src/pages? I'll be glad to review it 🙌

EugeneTorap · 2023-01-18T13:32:04Z

Hi @mayurnewase. Can you review #22775 (Move RLS to src/pages folder)?

eschutho · 2023-01-18T22:21:47Z

Heads up that changing RLS_FORM_QUERY_REL_FIELDS to RLS_FILTER_RELATED_FIELDS may affect deployments that customize this value in various ways - are we OK with this change or would this constitute a breaking change (even if it's easy to correct)? CC @eschutho @john-bodley @dpgaspar

yeah, I would consider this a breaking change, because it changes the name of a config. I noticed the comment in "Breaking changes" in UPDATING.MD as well, which we should reserve only for major versions. I think we're going to run into issues in the 2.1.0 release for our users. Can we try adding a new config, and reading either flag for now in order to make this backward compatible, and then remove the old config in 3.0?

Changing permission names is also considered a breaking change. I don't have any suggestions as to how to fix that in this PR other than going back to the old permission names.

eschutho · 2023-01-18T22:33:08Z

We didn't talk about removing views in the semantic versioning SIP but in the future, we may want to consider that as an additional override/extension point to the application and only remove views in major version bumps. Actually, on further consideration, removing a view removes an api endpoint, so it would be covered there.

This reverts commit 159dcd7.

eschutho · 2023-01-19T17:36:24Z

@mayurnewase Because of these breaking changes to semantic versioning, I'd like to propose that we revert this PR and bring it back into master for 3.0. I expect that it will break a few Superset users' workflows when they try to upgrade to the next minor release, which we're trying to avoid.

EugeneTorap · 2023-01-19T19:19:12Z

@eschutho When are we planning the 3.0 release? in 2024?

eschutho · 2023-01-20T00:40:52Z

@michael-s-molina has a better idea of that timeline than I do.

michael-s-molina · 2023-01-20T13:57:00Z

Hi @EugeneTorap. We're currently discussing the 3.0 date and our LTS support guidelines. We'll announce the official date to the community pretty soon but the current thinking (which may change) is April/2023.

mayurnewase · 2023-01-21T01:38:34Z

so no major features can be pushed to master if a major release isn't planned?
thats a bummer :(

eschutho · 2023-01-21T02:07:56Z

Oh no, you can push major features any time, just not with any of the breaking changes as outlined in the semantic versioning SIP to follow convention. And if there are any, most breaking changes in new features can be made to be backward compatible with just a few adjustments. Those suggestions are also in the sip.

EugeneTorap · 2023-01-21T08:50:34Z

@eschutho Maybe it's better to add a feature flag for the new breaking changes of RLS instead of reverting the PR?

This reverts commit 159dcd7.

mayurnewase · 2023-01-22T02:22:42Z

created PR to revert, although if it breaks any action in CI I will need some time to get back on it, so feel free to push in my branch if its urgent.
thank you all for your time on this.

eschutho · 2023-01-23T17:28:13Z

Thank you @mayurnewase. If you want to put up a new pr, I would suggest a few things to make it backward compatible:

deprecate, don't remove old views. Change the client side to use react but still keep the old views for backward compatibility. You can even change the client side route if you need to.
Instead of changing the naming on configs, you can add new ones and deprecate the old ones, but make sure that wither will work for now.
Same with permission names, add new ones and deprecate old ones, making sure that both still work.

@eschutho Maybe it's better to add a feature flag for the new breaking changes of RLS instead of reverting the PR?

I don't believe a feature flag specifically would work in this case, but I've outlined some other options.

This reverts commit 159dcd7.

…"" This reverts commit 6306803.

michael-s-molina · 2023-04-11T17:39:32Z

@mayurnewase Now that the window for breaking changes is open (until May 15th), would you like to submit these changes again and update its status on the Apache Superset 3.0 board?

initial

f5ea5ab

pull-request-size bot added the size/L label Dec 3, 2022

mayurnewase changed the title ~~feat: Restful apis and react view for RLS~~ [WIP] feat: Restful apis and react view for RLS Dec 3, 2022

mayurnewase changed the title ~~[WIP] feat: Restful apis and react view for RLS~~ [WIP] feat: RESTful apis and react view for RLS Dec 3, 2022

mayurnewase changed the title ~~[WIP] feat: RESTful apis and react view for RLS~~ [WIP] feat(RLS): RESTful apis and react view for RLS Dec 3, 2022

Merge branch 'master' of github.com:apache/superset into feat/restful…

3c194bb

…-rls

rusackas requested review from dpgaspar, villebro and nytai December 6, 2022 16:45

rusackas reviewed Dec 6, 2022

View reviewed changes

superset/row_level_security/schemas.py Outdated Show resolved Hide resolved

rusackas reviewed Dec 6, 2022

View reviewed changes

superset/row_level_security/api.py Show resolved Hide resolved

added related fields for table and roles on api, added modal to creat…

b359956

…e and update rules

pull-request-size bot added size/XXL and removed size/L labels Dec 7, 2022

all features complete

9bcd0f1

mayurnewase marked this pull request as ready for review December 10, 2022 09:54

add license

a3b352c

mayurnewase changed the title ~~[WIP] feat(RLS): RESTful apis and react view for RLS~~ feat(RLS): RESTful apis and react view for RLS Dec 10, 2022

add more license, remove todo comments

6bfe1f2

mayurnewase marked this pull request as draft December 10, 2022 10:55

mayurnewase commented Dec 10, 2022

View reviewed changes

mayurnewase added 5 commits December 10, 2022 16:36

raise correct exception on bulk_delete failure

d92bc9e

remove commented code

5e74273

optimize create and update command by batching table read queries

58388f2

add tooltip, add list and show schema

b931886

lint

dc66f89

mayurnewase marked this pull request as ready for review December 12, 2022 01:37

add tests

afdb088

mayurnewase deleted the feat/restful-rls branch January 10, 2023 02:35

sadpandajoe added a commit to preset-io/superset that referenced this pull request Jan 18, 2023

Revert "feat(RLS): RESTful apis and react view for RLS (apache#22325)"

1c2fda5

This reverts commit 159dcd7.

mayurnewase added a commit that referenced this pull request Jan 22, 2023

Revert "feat(RLS): RESTful apis and react view for RLS (#22325)"

53a2801

This reverts commit 159dcd7.

mayurnewase mentioned this pull request Jan 22, 2023

fix: revert RESTful apis and react view for RLS #22818

Merged

lyndsiWilliams added a commit that referenced this pull request Jan 25, 2023

Revert "feat(RLS): RESTful apis and react view for RLS (#22325)"

6306803

This reverts commit 159dcd7.

lyndsiWilliams added a commit that referenced this pull request Jan 25, 2023

Revert "Revert "feat(RLS): RESTful apis and react view for RLS (#22325)…

f88f49d

…"" This reverts commit 6306803.

michael-s-molina mentioned this pull request Apr 21, 2023

feat(revert): Re-introduces the RLS page #23777

Merged

9 tasks

Vitor-Avila mentioned this pull request Jul 3, 2023

The Export RLS command is not working properly preset-io/backend-sdk#223

Closed

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(RLS): RESTful apis and react view for RLS #22325

feat(RLS): RESTful apis and react view for RLS #22325

mayurnewase commented Dec 3, 2022 •

edited

rusackas commented Dec 6, 2022

codecov bot commented Dec 10, 2022 •

edited

mayurnewase Dec 10, 2022 •

edited

mayurnewase commented Jan 10, 2023 •

edited

EugeneTorap commented Jan 10, 2023

EugeneTorap commented Jan 12, 2023

EugeneTorap commented Jan 18, 2023

eschutho commented Jan 18, 2023 •

edited

eschutho commented Jan 18, 2023 •

edited

eschutho commented Jan 19, 2023

EugeneTorap commented Jan 19, 2023

eschutho commented Jan 20, 2023 •

edited

michael-s-molina commented Jan 20, 2023

mayurnewase commented Jan 21, 2023

eschutho commented Jan 21, 2023 •

edited

EugeneTorap commented Jan 21, 2023

mayurnewase commented Jan 22, 2023 •

edited

eschutho commented Jan 23, 2023 •

edited

michael-s-molina commented Apr 11, 2023

feat(RLS): RESTful apis and react view for RLS #22325

feat(RLS): RESTful apis and react view for RLS #22325

Conversation

mayurnewase commented Dec 3, 2022 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

rusackas commented Dec 6, 2022

codecov bot commented Dec 10, 2022 • edited

Codecov Report

mayurnewase Dec 10, 2022 • edited

Choose a reason for hiding this comment

mayurnewase commented Jan 10, 2023 • edited

EugeneTorap commented Jan 10, 2023

EugeneTorap commented Jan 12, 2023

EugeneTorap commented Jan 18, 2023

eschutho commented Jan 18, 2023 • edited

eschutho commented Jan 18, 2023 • edited

eschutho commented Jan 19, 2023

EugeneTorap commented Jan 19, 2023

eschutho commented Jan 20, 2023 • edited

michael-s-molina commented Jan 20, 2023

mayurnewase commented Jan 21, 2023

eschutho commented Jan 21, 2023 • edited

EugeneTorap commented Jan 21, 2023

mayurnewase commented Jan 22, 2023 • edited

eschutho commented Jan 23, 2023 • edited

michael-s-molina commented Apr 11, 2023

mayurnewase commented Dec 3, 2022 •

edited

codecov bot commented Dec 10, 2022 •

edited

mayurnewase Dec 10, 2022 •

edited

mayurnewase commented Jan 10, 2023 •

edited

eschutho commented Jan 18, 2023 •

edited

eschutho commented Jan 18, 2023 •

edited

eschutho commented Jan 20, 2023 •

edited

eschutho commented Jan 21, 2023 •

edited

mayurnewase commented Jan 22, 2023 •

edited

eschutho commented Jan 23, 2023 •

edited