Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 25 commits into from
Jan 9, 2023

Conversation

mayurnewase
Copy link
Contributor

@mayurnewase mayurnewase commented Dec 3, 2022

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

  1. Empty state

Screenshot from 2022-12-10 15-19-13

  1. Add rule modal

Screenshot from 2022-12-12 06-55-45

  1. Populated list

Screenshot from 2022-12-10 15-21-18

  1. Bulk select to delete

Screenshot from 2022-12-10 15-21-44

TESTING INSTRUCTIONS

  1. Before merging this PR, create few RLS rules.
  2. Then merge PR and check they are correctly shown on the new UI.
  3. Delete all, create new and check correctly shown.
  4. test filters in the list view.
  5. Create chart on the table rule is set with the role and check its correctly getting applied.
  6. Set RLS_FILTER_RELATED_FIELDS in the config.py as below
{
    "tables": [["table_name", filters.FilterStartsWith, "birth"]],
    "roles": [["name", filters.FilterContains, "Admin"]]"
}
  1. 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

@mayurnewase 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 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 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
@rusackas
Copy link
Member

rusackas commented Dec 6, 2022

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.

@mayurnewase mayurnewase marked this pull request as ready for review December 10, 2022 09:54
@mayurnewase 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
@codecov
Copy link

codecov bot commented Dec 10, 2022

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 mayurnewase marked this pull request as draft December 10, 2022 10:55
@@ -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:
Copy link
Contributor Author

@mayurnewase mayurnewase Dec 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mayurnewase mayurnewase marked this pull request as ready for review December 12, 2022 01:37
@mayurnewase mayurnewase deleted the feat/restful-rls branch January 10, 2023 02:35
@EugeneTorap
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@eschutho
Copy link
Member

eschutho commented Jan 18, 2023

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
Copy link
Member

eschutho commented Jan 18, 2023

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.

sadpandajoe added a commit to preset-io/superset that referenced this pull request Jan 18, 2023
@eschutho
Copy link
Member

@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
Copy link
Contributor

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

@eschutho
Copy link
Member

eschutho commented Jan 20, 2023

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

@michael-s-molina
Copy link
Member

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
Copy link
Contributor Author

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

@eschutho
Copy link
Member

eschutho commented Jan 21, 2023

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
Copy link
Contributor

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

@mayurnewase
Copy link
Contributor Author

mayurnewase commented Jan 22, 2023

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
Copy link
Member

eschutho commented Jan 23, 2023

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

  1. 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.
  2. 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.
  3. 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.

lyndsiWilliams added a commit that referenced this pull request Jan 25, 2023
lyndsiWilliams added a commit that referenced this pull request Jan 25, 2023
@michael-s-molina
Copy link
Member

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

@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XXL 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants