fix: shut off unneeded endpoints #8960

mistercrunch · 2020-01-13T22:16:03Z

Our list of exposed endpoints has gotten out of control, mostly due to the default FAB behavior to auto-generate and auto-expose lots of endpoints dynamically on all ModelViews, combined with the fact that we have lots of models, and sometimes multiple ModelViews per model.

This PR disables ~300 endpoints out of ~500+ we currently have.

This PR is a fairly thorough pass on all ModelViews disabling endpoints that are not used. The focus has been on ModelViews, and generally only enabling what is needed (either the CRUD and/or the REST endpoints). In many cases, I used either logic, my personal understanding of how models should be exposes, and simple grep commands to confirm assumptions.

Command to list the active endpoints

echo "print('\n'.join(sorted([str(o) for o in app.url_map.iter_rules()])))" | flask shell

Note that longer term, we want to disable all ModelView REST endpoints and favor of FAB's newer RestModelView. We also want to move away from the FAB's MVC / jinja CRUD, meaning we'll be deprecating all ModelViews.

request-info · 2020-01-13T22:16:07Z

We would appreciate it if you could provide us with more info about this issue/pr! Please do not leave the title or description empty.

mistercrunch · 2020-01-13T22:16:51Z

superset/app.py

@@ -261,14 +255,11 @@ def init_views(self) -> None:
        appbuilder.add_view_no_menu(Dashboard)
        appbuilder.add_view_no_menu(DashboardAddView)
        appbuilder.add_view_no_menu(DashboardModelViewAsync)
-        appbuilder.add_view_no_menu(DatabaseAsync)
-        appbuilder.add_view_no_menu(DatabaseTablesAsync)


These 2 are unused now...

mistercrunch · 2020-01-13T22:17:14Z

superset/app.py

        appbuilder.add_view_no_menu(Datasource)
        appbuilder.add_view_no_menu(KV)
        appbuilder.add_view_no_menu(R)
        appbuilder.add_view_no_menu(SavedQueryView)
        appbuilder.add_view_no_menu(SavedQueryViewApi)
-        appbuilder.add_view_no_menu(SliceAddView)


Merged with SliceAsync to simplify

dpgaspar · 2020-01-15T14:00:33Z

superset/views/log/views.py

@@ -24,3 +24,4 @@

 class LogModelView(LogMixin, SupersetModelView):  # pylint: disable=too-many-ancestors
    datamodel = SQLAInterface(models.Log)
+    include_route_methods = {"list"}


Consider adding "show" also, since the fields retrieved by list are not very useful, it's important to access the log detail

Unless this is 100% required for application functionality, I would recommend against exposing this information.

I think what makes sense is either {"list", "show"} or nothing here. I was thinking about leaving it for backwards compatibility. I could put it behind a config flag too, let me do that.

superset/security/manager.py

etr2460 · 2020-01-16T18:08:03Z

@graceguo-supercat do you know if we use any of these endpoints for programmatic chart/dashboard generation?

mistercrunch · 2020-01-18T16:55:39Z

superset/app.py

@@ -218,6 +211,16 @@ def init_views(self) -> None:
            category_label=__("Sources"),
            category_icon="fa-database",
        )
+        appbuilder.add_link(


side mission: fix the menu ordering

superset/app.py

superset/security/manager.py

mistercrunch · 2020-01-18T17:01:44Z

superset/views/core.py

@@ -2984,16 +2983,3 @@ def apply_http_headers(response: Response):
        if k not in response.headers:
            response.headers[k] = v
    return response
-
-
-@app.route('/<regex("panoramix\/.*"):url>')


Killing redirects for things that would have been bookmarked 4+ years ago

mistercrunch · 2020-01-18T17:02:31Z

superset/views/dashboard/views.py

@@ -117,18 +121,3 @@ class DashboardModelViewAsync(DashboardModelView):  # pylint: disable=too-many-a
        "creator": _("Creator"),
        "modified": _("Modified"),
    }
-
-
-class DashboardAddView(DashboardModelView):  # pylint: disable=too-many-ancestors


Not needed anymore now that we have ModelRestApi

codecov-io · 2020-01-18T18:05:54Z

Codecov Report

Merging #8960 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #8960   +/-   ##
=======================================
  Coverage   59.16%   59.16%           
=======================================
  Files         367      367           
  Lines       11679    11679           
  Branches     2862     2862           
=======================================
  Hits         6910     6910           
  Misses       4590     4590           
  Partials      179      179

Impacted Files	Coverage Δ
...rset/assets/src/dashboard/actions/sliceEntities.js	`9.67% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e46ff23...c868620. Read the comment docs.

dpgaspar · 2020-01-21T12:53:51Z

requirements.txt

@@ -22,7 +22,7 @@ croniter==0.3.30
 cryptography==2.7
 decorator==4.4.0          # via retry
 defusedxml==0.6.0         # via python3-openid
-flask-appbuilder==2.2.1
+flask-appbuilder==2.2.2rc1


The latest rc is rc3. When no more features are needed to support this I'll release 2.2.2

dpgaspar · 2020-01-21T12:59:18Z

tests/core_tests.py

@@ -345,7 +345,7 @@ def test_add_slice(self):
    def test_get_user_slices(self):
        self.login(username="admin")
        userid = security_manager.find_user("admin").id
-        url = "/sliceaddview/api/read?_flt_0_created_by={}".format(userid)
+        url = "/sliceasync/api/read?_flt_0_created_by={}".format(userid)


nit: Take the chance to use f strings

craig-rueda · 2020-01-21T20:47:30Z

superset/security/manager.py

+# Limiting routes on FAB model views
+UserModelView.include_route_methods = CRUD_ROUTE_METHODS | {"userinfo"}
+RoleModelView.include_route_methods = CRUD_ROUTE_METHODS
+PermissionViewModelView.include_route_methods = {"list"}


Dry these up? You added a constant above ^^

superset/app.py

We recently added a new feature to FAB allowing to whitelist the needed endpoints in ModelView and ModelRestApi. First, we set our base wrapper class to an empty set, forcing each class inheriting from it to explicitely turn on the endpoints that Superset intends to use. Second, we go ModelView by ModelView to whitelist the actual endpoints used in the app. Notes: * as a result a large set of [unneeded] permissions should be cleaned up * outside of the "private" use of endpoints in the app, people that have been using endpoints in their environment for other purposes may experience loss of functionality

mistercrunch · 2020-01-22T19:02:29Z

@etr2460 @graceguo-supercat , we'd like to move forward and merge this, let us know if you use other endpoints that we can open up for the time being.

If you share a list of endpoints you use we can take that into account as we do change management in this area.

graceguo-supercat

LGTM

john-bodley · 2020-01-28T18:31:36Z

@mistercrunch we use the /sqlmetricinlineview/api/* and /tablecolumninlineview/api/* routes syncing metadata.

superset-github-bot bot added the preset-io label Jan 13, 2020

request-info bot added the need:more-info Requires more information from author label Jan 13, 2020

pull-request-size bot added the size/L label Jan 13, 2020

mistercrunch commented Jan 13, 2020

View reviewed changes

dpgaspar reviewed Jan 15, 2020

View reviewed changes

superset/security/manager.py Outdated Show resolved Hide resolved

mistercrunch force-pushed the endpoints branch from a9b8657 to edf4408 Compare January 16, 2020 06:37

mistercrunch force-pushed the endpoints branch 3 times, most recently from 9575b43 to 425bcfa Compare January 18, 2020 16:23

mistercrunch added .important risk:hard-to-test The change will be hard to test labels Jan 18, 2020

mistercrunch force-pushed the endpoints branch 3 times, most recently from 6a89dd9 to 944e077 Compare January 18, 2020 17:27

mistercrunch commented Jan 18, 2020

View reviewed changes

mistercrunch marked this pull request as ready for review January 18, 2020 17:31

mistercrunch force-pushed the endpoints branch 2 times, most recently from 6172183 to 06c58ce Compare January 18, 2020 17:53

dpgaspar reviewed Jan 21, 2020

View reviewed changes

craig-rueda reviewed Jan 21, 2020

View reviewed changes

craig-rueda requested changes Jan 21, 2020

View reviewed changes

superset/app.py Show resolved Hide resolved

mistercrunch added 2 commits January 22, 2020 07:52

Tweaking

2b9fa58

dpgaspar and others added 11 commits January 22, 2020 07:57

Reduce the amount of endpoints using white lists

844bd58

Fix, included needed endpoints for dashboard and druid

7422860

Drying things up

e922391

fixes

84afcac

limiting more endpoints

d04a82d

Read only on some FAB model views

9cb8c6d

fixing some tests

5aec588

fixes

a062f00

Fixing more tests

fa6e236

Addressing comments

62c3697

Drying up route_methods

1e9f0ec

mistercrunch force-pushed the endpoints branch 2 times, most recently from 107e756 to 94b1642 Compare January 22, 2020 16:45

craig-rueda approved these changes Jan 22, 2020

View reviewed changes

mistercrunch force-pushed the endpoints branch from 94b1642 to ba3d68c Compare January 22, 2020 18:33

further drying

c868620

mistercrunch force-pushed the endpoints branch from ba3d68c to c868620 Compare January 22, 2020 18:51

graceguo-supercat approved these changes Jan 23, 2020

View reviewed changes

This was referenced Jan 23, 2020

[log] fix, log model view permissions #8993

Merged

Bump FAB to 2.2.2 #9004

Merged

mistercrunch merged commit 315a11d into apache:master Jan 23, 2020

mistercrunch deleted the endpoints branch January 23, 2020 16:25

etr2460 mentioned this pull request Jan 25, 2020

[dashboard] fix, enable info endpoint #9015

Merged

12 tasks

john-bodley mentioned this pull request Jan 28, 2020

[routes] Re-adding FAB API routes for SqlMetricInlineView and TableModelView #9035

Merged

12 tasks

nytai mentioned this pull request Jan 28, 2020

View Icon gone from Chart, Table, Dashboard CRUD Views #9039

Closed

3 tasks

etr2460 mentioned this pull request Apr 2, 2020

Re-enable the AnnotationLayerModelView read API #9443

Merged

12 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: shut off unneeded endpoints #8960

fix: shut off unneeded endpoints #8960

mistercrunch commented Jan 13, 2020 •

edited

Loading

request-info bot commented Jan 13, 2020

mistercrunch Jan 13, 2020

mistercrunch Jan 13, 2020

dpgaspar Jan 15, 2020

willbarrett Jan 15, 2020

mistercrunch Jan 16, 2020

etr2460 commented Jan 16, 2020

mistercrunch Jan 18, 2020

mistercrunch Jan 18, 2020

mistercrunch Jan 18, 2020

codecov-io commented Jan 18, 2020 •

edited

Loading

dpgaspar Jan 21, 2020

dpgaspar Jan 21, 2020

craig-rueda Jan 21, 2020

mistercrunch commented Jan 22, 2020 •

edited

Loading

graceguo-supercat left a comment

john-bodley commented Jan 28, 2020 •

edited

Loading

fix: shut off unneeded endpoints #8960

fix: shut off unneeded endpoints #8960

Conversation

mistercrunch commented Jan 13, 2020 • edited Loading

request-info bot commented Jan 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etr2460 commented Jan 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 18, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch commented Jan 22, 2020 • edited Loading

graceguo-supercat left a comment

Choose a reason for hiding this comment

john-bodley commented Jan 28, 2020 • edited Loading

mistercrunch commented Jan 13, 2020 •

edited

Loading

codecov-io commented Jan 18, 2020 •

edited

Loading

mistercrunch commented Jan 22, 2020 •

edited

Loading

john-bodley commented Jan 28, 2020 •

edited

Loading