fix: Gamma users shouldn't be able to create roles #29687

hughhhh · 2024-07-24T19:37:29Z

SUMMARY

Currently users with gamma roles are still able to create roles, This PR is made to fix this as well as add test. Reason why this is happening is due to the instantiation of the ModelRestAPI classes happening here https://github.com/dpgaspar/Flask-AppBuilder/blob/2d527aae09eed50fd52c6bf886a2970adb225428/flask_appbuilder/security/sqla/manager.py#L97

To fix it we'll just be adding the new security views names to the ADMIN_ONLY_VIEW_MENUS List in the manager class

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

# logged in as GAMMA
$ curl "http://127.0.0.1:9000/api/v1/security/roles/" \
     -H 'Content-Type: application/json; charset=utf-8' \
     -d $'{
  "name": "new_role"
}'
>> 201

$ curl "http://127.0.0.1:9000/api/v1/security/roles/" \
     -H 'Content-Type: application/json; charset=utf-8' \
     -d $'{
  "name": "new_role"
}'
>> 403

TESTING INSTRUCTIONS

Configure SuperSet to use DB authentication and set FAB_ADD_SECURITY_API = True
Initialize SuperSet; be sure to add an administrator
Launch SuperSet
Browse to SuperSet
Log in as the administrator
Add a new user and grant them only the Gamma role
Log out
Log in as the new user
Browse to the swagger endpoint (/swagger/v1)
Find Security Roles
Expand POST /api/v1/security/roles
Click Try It Out
In the Model schema area provide a name for a new role
Click Execute

ADDITIONAL INFORMATION

Has associated issue: SEC-70
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

hughhhh · 2024-07-26T22:44:50Z

@dpgaspar let me know if this pattern make sense or do you have another suggestion of either overriding the view registration, or adding security checks on the indy endpoints

dpgaspar

instead of overriding register_views can we leverage this instead https://github.com/apache/superset/blob/master/superset/security/manager.py#L226
add the FAB security views

eschutho · 2024-07-31T01:15:27Z

tests/integration_tests/security/api_tests.py

+    #         data=json.dumps({"name": "new_role"}),
+    #         content_type="application/json",
+    #     )
+    #     self.assert403(response)


Yey for all the new tests! What should we do with these last two?

Going to finishes these up today, mainly wanted to get the adjustment @dpgaspar in and will close these up soon

hughhhh · 2024-08-07T16:40:41Z

superset/config.py

@@ -320,6 +320,8 @@ def _try_json_readsha(filepath: str, length: int) -> str | None:
 # ex: http://localhost:8080/swagger/v1
 FAB_API_SWAGGER_UI = True

+FAB_ADD_SECURITY_API = True


@dpgaspar did we make and update in flask app builder to make this default True? When I recently pulled these were getting KeyError: FAB_ADD_SECURITY_API not found Also I saw that flask app builder has this default True so to allow these test to run I just made this value consistent with what I saw upstream

Can you verify that this is what's happening?

Default is false on FAB: https://github.com/dpgaspar/Flask-AppBuilder/blob/2d527aae09eed50fd52c6bf886a2970adb225428/flask_appbuilder/security/sqla/manager.py#L97

The link your mentioning is an example app

dpgaspar

Looks good, just a couple of non blocking comments

dpgaspar · 2024-08-19T08:01:59Z

superset/config.py

@@ -320,6 +320,8 @@ def _try_json_readsha(filepath: str, length: int) -> str | None:
 # ex: http://localhost:8080/swagger/v1
 FAB_API_SWAGGER_UI = True

+FAB_ADD_SECURITY_API = True


Default is false on FAB: https://github.com/dpgaspar/Flask-AppBuilder/blob/2d527aae09eed50fd52c6bf886a2970adb225428/flask_appbuilder/security/sqla/manager.py#L97

The link your mentioning is an example app

dpgaspar · 2024-08-19T08:04:13Z

tests/integration_tests/security/api_tests.py

+    @with_config({"FAB_ADD_SECURITY_API": True})
+    def test_get_security_roles_gamma(self):
+        """
+        Security API: Admin should be able to create roles


nit: comment is not correct

dpgaspar · 2024-08-19T08:04:54Z

tests/integration_tests/security/api_tests.py

+    @with_config({"FAB_ADD_SECURITY_API": True})
+    def test_get_security_roles_admin(self):
+        """
+        Security API: Admin should be able to create roles


nit: Security API: Admin should be able to get roles

(cherry picked from commit 7650c47)

pull-request-size bot added the size/S label Jul 24, 2024

michael-s-molina added the review:draft label Jul 25, 2024

hughhhh force-pushed the SEC-70 branch from 3677cc9 to 02eedab Compare July 25, 2024 22:00

pull-request-size bot added size/M and removed size/S labels Jul 25, 2024

github-actions bot added the api Related to the REST API label Jul 25, 2024

hughhhh force-pushed the SEC-70 branch from 02eedab to 23f9ee9 Compare July 26, 2024 22:25

pull-request-size bot added size/L and removed size/M labels Jul 26, 2024

hughhhh requested a review from dpgaspar July 26, 2024 22:43

dpgaspar reviewed Jul 30, 2024

View reviewed changes

hughhhh changed the title ~~[WIP] SEC-70 - Gamma users shouldn't be able to create roles~~ [WIP] Gamma users shouldn't be able to create roles Jul 30, 2024

github-actions bot removed the api Related to the REST API label Jul 31, 2024

hughhhh changed the title ~~[WIP] Gamma users shouldn't be able to create roles~~ fix: Gamma users shouldn't be able to create roles Jul 31, 2024

eschutho reviewed Jul 31, 2024

View reviewed changes

hughhhh force-pushed the SEC-70 branch from c75c7f3 to 9f320f9 Compare August 6, 2024 22:30

hughhhh marked this pull request as ready for review August 6, 2024 23:10

dosubot bot added api Related to the REST API authentication:access-control Rlated to access control labels Aug 6, 2024

hughhhh force-pushed the SEC-70 branch from f66536a to 6865adb Compare August 7, 2024 03:19

github-actions bot removed the api Related to the REST API label Aug 7, 2024

hughhhh force-pushed the SEC-70 branch 2 times, most recently from f35fd4c to cc096fe Compare August 7, 2024 05:37

pull-request-size bot added size/M and removed size/L labels Aug 7, 2024

hughhhh commented Aug 7, 2024

View reviewed changes

hughhhh force-pushed the SEC-70 branch from 00a9db0 to 787fd0f Compare August 7, 2024 16:41

hughhhh requested review from dpgaspar and eschutho August 8, 2024 19:16

dpgaspar approved these changes Aug 19, 2024

View reviewed changes

hughhhh added 7 commits August 20, 2024 20:26

started test

61da3cc

test validating that 201 is coming back

0b2bd38

using config instead overriding

7da8b6f

fix

c6dbc38

remove test

69f21c7

fix last test

a7b21af

address concerns

bda5829

hughhhh force-pushed the SEC-70 branch from c554314 to bda5829 Compare August 20, 2024 20:27

hughhhh added 2 commits August 20, 2024 20:32

fix config

382d4c5

last one

fc3abf6

hughhhh merged commit 7650c47 into master Aug 21, 2024
33 checks passed

sadpandajoe pushed a commit that referenced this pull request Aug 27, 2024

fix: Gamma users shouldn't be able to create roles (#29687)

fe33689

(cherry picked from commit 7650c47)

sadpandajoe added the v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch label Sep 4, 2024

rusackas deleted the SEC-70 branch September 27, 2024 20:54

github-actions bot added 🍒 4.1.0 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels labels Nov 14, 2024

mistercrunch added the 🍒 4.1.1 label Nov 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Gamma users shouldn't be able to create roles #29687

fix: Gamma users shouldn't be able to create roles #29687

hughhhh commented Jul 24, 2024 •

edited

Loading

hughhhh commented Jul 26, 2024

dpgaspar left a comment

eschutho Jul 31, 2024

hughhhh Jul 31, 2024 •

edited

Loading

hughhhh Aug 7, 2024 •

edited

Loading

dpgaspar Aug 19, 2024

dpgaspar left a comment

dpgaspar Aug 19, 2024

dpgaspar Aug 19, 2024

dpgaspar Aug 19, 2024

fix: Gamma users shouldn't be able to create roles #29687

fix: Gamma users shouldn't be able to create roles #29687

Conversation

hughhhh commented Jul 24, 2024 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

hughhhh commented Jul 26, 2024

dpgaspar left a comment

Choose a reason for hiding this comment

eschutho Jul 31, 2024

Choose a reason for hiding this comment

hughhhh Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

hughhhh Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

dpgaspar Aug 19, 2024

Choose a reason for hiding this comment

dpgaspar left a comment

Choose a reason for hiding this comment

dpgaspar Aug 19, 2024

Choose a reason for hiding this comment

dpgaspar Aug 19, 2024

Choose a reason for hiding this comment

dpgaspar Aug 19, 2024

Choose a reason for hiding this comment

hughhhh commented Jul 24, 2024 •

edited

Loading

hughhhh Jul 31, 2024 •

edited

Loading

hughhhh Aug 7, 2024 •

edited

Loading