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(api): add role, permission, user APIs #21053

Closed

Conversation

jileeon
Copy link
Contributor

@jileeon jileeon commented Aug 11, 2022

SUMMARY

Integrate role, permission, user APIs on Flask-AppBuilder into Apache Superset
See #21050

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

These APIs can be used on Apache Superset Web Service. See the screenshot below.
스크린샷 2022-08-11 오후 2 59 57

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Introduces new feature or API

@jileeon jileeon changed the title feat: add role, permission, user APIs feat(api): add role, permission, user APIs Aug 12, 2022
Copy link
Member

@nytai nytai left a comment

Choose a reason for hiding this comment

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

LGTM, though would like to get @dpgaspar sign off before merging

Comment on lines 31 to 32
from flask_appbuilder.security.sqla.apis import PermissionApi, PermissionViewMenuApi, \
RoleApi, UserApi, ViewMenuApi
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
from flask_appbuilder.security.sqla.apis import PermissionApi, PermissionViewMenuApi, \
RoleApi, UserApi, ViewMenuApi
from flask_appbuilder.security.sqla.apis import (
PermissionApi,
PermissionViewMenuApi,
RoleApi,
UserApi,
ViewMenuApi,
)

@mayurnewase
Copy link
Contributor

mayurnewase commented Aug 13, 2022

I think best way to enable these is just by enabling FAB_ADD_SECURITY_API flag in config which was added in FAB 4.1.0, and FAB itself will add these.

@jileeon jileeon force-pushed the feat-add-role-permission-user-apis branch from e325b56 to ce41ac1 Compare August 16, 2022 00:00
@jileeon jileeon force-pushed the feat-add-role-permission-user-apis branch from ce41ac1 to f74945c Compare August 16, 2022 00:07
@jileeon
Copy link
Contributor Author

jileeon commented Aug 16, 2022

Hello,

I've changed the commit for @mayurnewase 's comment, and I set FAB_ADD_SECURITY_API flag to False by default.
Because I found that FAB_ADD_SECURITY_API flag isn't set to True by default in Flask-AppBuilder.

See changes and review this.

Thanks & Regards

@jileeon jileeon force-pushed the feat-add-role-permission-user-apis branch from f74945c to 9b1bf50 Compare August 16, 2022 00:10
@mayurnewase
Copy link
Contributor

mayurnewase commented Aug 16, 2022

If you just enable the flag, you won't need to add those apis in superset's init.
I think this PR should be just about adding that flag set to False in the config, and you override it locally to True as I think @dpgaspar has marked those apis as Beta.

@jileeon
Copy link
Contributor Author

jileeon commented Aug 16, 2022

@mayurnewase You are right. We can just enable it by setting FAB_ADD_SECURITY_API to True.

So I think that we no longer need this PR by the way. Do you mind if I close this PR?

P.S. I used these APIs since Superset 1.4 by importing dpgaspar/Flask-AppBuilder#1801 locally only for using them. So it happens like that. Sorry for taking your time.

Thanks.

@mayurnewase
Copy link
Contributor

mayurnewase commented Aug 16, 2022

Sure no problem, happy to help.
If this PR had helped add the flag in the config with little description on what it does would have helped anyone looking to use these apis, but its alright I will add it later.

@jileeon
Copy link
Contributor Author

jileeon commented Aug 16, 2022

@mayurnewase, That's great. I've been thinking of you said "add the flag in the config with little description". and I'm glad to add this flag later for other Superset users.

I going to close it now.

Have a nice day :)

Thanks.

@jileeon jileeon closed this Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants