[security] improving the security scheme #1587

mistercrunch · 2016-11-11T23:03:02Z

No description provided.

bkyryliuk · 2016-11-11T23:15:04Z

superset/utils.py

+            sm.add_permission_role(gamma, p)
+            if PUBLIC_ROLE_LIKE_GAMMA:
+                sm.add_permission_role(public, p)
+        else:


could you add a todo to remove else case once the DB state is fixed ?

I'm thinking we move to fully control these base roles, if people alter them they will be overwritten. People should create and compose new roles in their environment instead of altering the ones provided by Superset

bkyryliuk · 2016-11-11T23:16:05Z

superset/utils.py

-                sm.add_permission_role(public_role, perm)
+                p.view_menu.name in {'SQL Lab'} or
+                p.permission.name in {'can_sql_json'}
+        ):


add download csv endpoint here and maybe query search

bkyryliuk · 2016-11-11T23:22:56Z

superset/views.py

-        qry = query.filter(self.model.perm.in_(self.get_perms()))
-        return qry
+        perms = self.get_view_menus('datasource_access')
+        logging.info(perms)


will be too noisy, could you remove logging here?

bkyryliuk · 2016-11-11T23:24:14Z

tests/base_tests.py

        public_role = appbuilder.sm.find_role('Public')
        perms = db.session.query(ab_models.PermissionView).all()
        for perm in perms:
            if (perm.permission.name == 'datasource_access' and
-                    perm.view_menu and table_name in perm.view_menu.name):
+                    perm.view_menu and table.perm in perm.view_menu.name):
+                print('-==-'*20)


s/print/logging.info

bkyryliuk · 2016-11-11T23:24:30Z

tests/core_tests.py

+            .filter_by(table_name='birth_names')
+            .one()
+        )
+        print(table.perm)


ditto s/print/logging.info

bkyryliuk · 2016-11-11T23:24:46Z

superset/views.py

@@ -1841,29 +1858,6 @@ def sqllab_viz(self):
        return redirect(url)

    @has_access
-    @expose("/sql/<database_id>/")


is it intended ?

that's an old obsolete endpoint

bkyryliuk · 2016-11-11T23:24:49Z

superset/views.py

@@ -1967,38 +1961,6 @@ def cached_key(self, key):
        return "nope"

    @has_access_api
-    @expose("/results/<key>/")


is it intended ?

bkyryliuk · 2016-11-12T03:23:39Z

superset/views.py

@@ -1967,7 +1960,6 @@ def cached_key(self, key):
        return "nope"

    @has_access_api
-    @expose("/results/<key>/")


you may need keep @expose

bkyryliuk · 2016-11-12T03:24:55Z

LGTM once test is fixed and @expose is back.

bkyryliuk · 2016-11-16T22:28:58Z

superset/assets/javascripts/SqlLab/components/DatabaseSelect.jsx

@@ -23,6 +30,12 @@ class DatabaseSelect extends React.PureComponent {
      const options = data.result.map((db) => ({ value: db.id, label: db.database_name }));
      this.setState({ databaseOptions: options, databaseLoading: false });
      this.props.actions.setDatabases(data.result);
+      if (data.result.length === 0) {


it would be great to pass the server response here / check the response status

bkyryliuk · 2016-11-16T22:30:11Z

superset/cli.py

+        print("Starting server with command: " + cmd)
+        Popen(cmd, shell=True).wait()
+
+@manager.command


Expected 2 blank lines, found 1

bkyryliuk · 2016-11-16T22:30:18Z

superset/cli.py

+    """Inits the Superset application"""
+    security.sync_role_definitions()
+
+@manager.option(


Expected 2 blank lines, found 1

bkyryliuk · 2016-11-16T22:30:24Z

superset/cli.py

+    if verbose:
+        print("[DB] : " + "{}".format(db.engine))
+
+@manager.option(


Expected 2 blank lines, found 1

bkyryliuk · 2016-11-16T22:30:31Z

superset/cli.py

+        print("Loading [Unicode test data]")
+        data.load_unicode_test_data()
+
+@manager.option(


Expected 2 blank lines, found 1

bkyryliuk · 2016-11-16T22:36:12Z

superset/models.py

@@ -2178,6 +2206,9 @@ def get_having_filters(self, raw_filters):
                filters = cond
        return filters

+sqla.event.listen(DruidDatasource, 'before_insert', set_perm)


datasource_name / cluster rename will change the permission and will revoke the user access to it, just an observation.

bkyryliuk · 2016-11-16T22:37:35Z

superset/views.py

@@ -964,6 +983,19 @@ class LogModelView(SupersetModelView):
    icon="fa-list-ol")


+class QueryView(SupersetModelView):
+    datamodel = SQLAInterface(models.Query)
+    list_columns = ['user', 'database', 'status', 'start_time', 'start_time']


s/start_time/end_time
what about showing sql?

bkyryliuk · 2016-11-16T22:38:47Z

superset/views.py

@@ -2263,7 +2273,6 @@ def show_traceback(self):
            title=ascii_art.stacktrace,
            art=ascii_art.error), 500

-    @has_access


I would keep @has_access for welcome too and would add it by default to the public role

bkyryliuk · 2016-11-16T22:43:31Z

Great changes, code is more structured now ! Having more tests around basic roles would be great, but can be done later in separate PR.

bkyryliuk · 2016-11-17T19:58:03Z

LGTM

mistercrunch force-pushed the security branch from 081898f to 62d643e Compare November 11, 2016 23:03

bkyryliuk reviewed Nov 11, 2016

View reviewed changes

tests/core_tests.py

.filter_by(table_name='birth_names')

.one()

)

print(table.perm)

Copy link

Member

bkyryliuk Nov 11, 2016

Choose a reason for hiding this comment

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

ditto s/print/logging.info

bkyryliuk reviewed Nov 11, 2016

View reviewed changes

mistercrunch force-pushed the security branch from 62d643e to b1cef80 Compare November 12, 2016 00:17

bkyryliuk reviewed Nov 12, 2016

View reviewed changes

mistercrunch force-pushed the security branch 6 times, most recently from 0d05dd1 to 8af7dda Compare November 16, 2016 00:16

bkyryliuk reviewed Nov 16, 2016

View reviewed changes

superset/cli.py

if verbose:

print("[DB] : " + "{}".format(db.engine))

@manager.option(

Copy link

Member

bkyryliuk Nov 16, 2016

Choose a reason for hiding this comment

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

Expected 2 blank lines, found 1

bkyryliuk reviewed Nov 16, 2016

View reviewed changes

mistercrunch added 4 commits November 17, 2016 10:18

[security] improving the security scheme

2e494bb

Addressing comments

57b43b9

improving docs

193dad1

Creating security module to organize things

991561b

mistercrunch added 5 commits November 17, 2016 10:18

Moving CLI to its own module

c2db4a1

perms

a77ed6d

Materializung perms

212bb45

progrss

88150d2

Addressing comments, linting

53bea43

mistercrunch force-pushed the security branch from 8af7dda to 53bea43 Compare November 17, 2016 18:25

mistercrunch merged commit bce02e3 into apache:master Nov 17, 2016

mistercrunch deleted the security branch November 17, 2016 19:58

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[security] improving the security scheme #1587

[security] improving the security scheme #1587

mistercrunch commented Nov 11, 2016

bkyryliuk Nov 11, 2016

mistercrunch Nov 12, 2016

bkyryliuk Nov 12, 2016

bkyryliuk Nov 11, 2016

bkyryliuk Nov 11, 2016

bkyryliuk Nov 11, 2016

bkyryliuk Nov 11, 2016

bkyryliuk Nov 11, 2016

mistercrunch Nov 12, 2016

bkyryliuk Nov 11, 2016

mistercrunch Nov 12, 2016

bkyryliuk Nov 12, 2016

bkyryliuk commented Nov 12, 2016

bkyryliuk Nov 16, 2016

bkyryliuk Nov 16, 2016

bkyryliuk Nov 16, 2016

bkyryliuk Nov 16, 2016

bkyryliuk Nov 16, 2016

bkyryliuk Nov 16, 2016

bkyryliuk Nov 16, 2016

bkyryliuk Nov 16, 2016

bkyryliuk commented Nov 16, 2016

bkyryliuk commented Nov 17, 2016

[security] improving the security scheme #1587

[security] improving the security scheme #1587

Conversation

mistercrunch commented Nov 11, 2016

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkyryliuk commented Nov 12, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkyryliuk commented Nov 16, 2016

bkyryliuk commented Nov 17, 2016