From 648f9fa54b90ce4cca84737a21214fd92c0e9fcc Mon Sep 17 00:00:00 2001 From: Maxim Sukharev Date: Tue, 23 Jul 2019 07:30:28 +0200 Subject: [PATCH] Set owner to dashboards and charts on import (#7894) * Allow to pass user for dashboard import cli Dashboard import assign current user from flask context during import. But in case of cli import there is no flask user and imported charts don't have an owner which prevents ability to edit them. * Reset ownership on dashboard import For overriding existing charts it requires `owners` property to be set. * Add tests for reset ownership * Use ORM to decode dashboards json Creating instances using ORM allows to normally work with relations * Fix test_import_dashboard_1_slice test Previously tests used side-effect of slices import which kept id from json on insert into db. --- superset/cli.py | 11 ++++- superset/models/core.py | 13 +++-- superset/models/helpers.py | 15 +++++- superset/utils/core.py | 20 ++------ tests/import_export_tests.py | 94 +++++++++++++++++++++++++++++++++++- 5 files changed, 128 insertions(+), 25 deletions(-) diff --git a/superset/cli.py b/superset/cli.py index c4f83097f..cc66f5884 100755 --- a/superset/cli.py +++ b/superset/cli.py @@ -23,6 +23,7 @@ from sys import stdout import click from colorama import Fore, Style +from flask import g from pathlib2 import Path import yaml @@ -184,7 +185,13 @@ def refresh_druid(datasource, merge): default=False, help="recursively search the path for json files", ) -def import_dashboards(path, recursive): +@click.option( + "--username", + "-u", + default=None, + help="Specify the user name to assign dashboards to", +) +def import_dashboards(path, recursive, username): """Import dashboards from JSON""" p = Path(path) files = [] @@ -194,6 +201,8 @@ def import_dashboards(path, recursive): files.extend(p.glob("*.json")) elif p.exists() and recursive: files.extend(p.rglob("*.json")) + if username is not None: + g.user = security_manager.find_user(username=username) for f in files: logging.info("Importing dashboard from file %s", f) try: diff --git a/superset/models/core.py b/superset/models/core.py index 59d38eb45..bfc080bf0 100755 --- a/superset/models/core.py +++ b/superset/models/core.py @@ -361,6 +361,7 @@ class Slice(Model, AuditMixinNullable, ImportMixin): slc_to_import.alter_params(remote_id=slc_to_import.id, import_time=import_time) slc_to_import = slc_to_import.copy() + slc_to_import.reset_ownership() params = slc_to_import.params_dict slc_to_import.datasource_id = ConnectorRegistry.get_datasource_by_name( session, @@ -617,7 +618,9 @@ class Dashboard(Model, AuditMixinNullable, ImportMixin): ): existing_dashboard = dash + dashboard_to_import = dashboard_to_import.copy() dashboard_to_import.id = None + dashboard_to_import.reset_ownership() # position_json can be empty for dashboards # with charts added from chart-edit page and without re-arranging if dashboard_to_import.position_json: @@ -646,14 +649,10 @@ class Dashboard(Model, AuditMixinNullable, ImportMixin): session.flush() return existing_dashboard.id else: - # session.add(dashboard_to_import) causes sqlachemy failures - # related to the attached users / slices. Creating new object - # allows to avoid conflicts in the sql alchemy state. - copied_dash = dashboard_to_import.copy() - copied_dash.slices = new_slices - session.add(copied_dash) + dashboard_to_import.slices = new_slices + session.add(dashboard_to_import) session.flush() - return copied_dash.id + return dashboard_to_import.id @classmethod def export_dashboards(cls, dashboard_ids): diff --git a/superset/models/helpers.py b/superset/models/helpers.py index 7c39c1c0e..472ad749d 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -21,7 +21,7 @@ import json import logging import re -from flask import escape, Markup +from flask import escape, g, Markup from flask_appbuilder.models.decorators import renders from flask_appbuilder.models.mixins import AuditMixin import humanize @@ -265,6 +265,19 @@ class ImportMixin(object): d.update(kwargs) self.params = json.dumps(d) + def reset_ownership(self): + """ object will belong to the user the current user """ + # make sure the object doesn't have relations to a user + # it will be filled by appbuilder on save + self.created_by = None + self.changed_by = None + # flask global context might not exist (in cli or tests for example) + try: + if g.user: + self.owners = [g.user] + except Exception: + self.owners = [] + @property def params_dict(self): return json_to_dict(self.params) diff --git a/superset/utils/core.py b/superset/utils/core.py index 8f0de13fe..b7d3370cc 100644 --- a/superset/utils/core.py +++ b/superset/utils/core.py @@ -254,25 +254,15 @@ def decode_dashboards(o): from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn if "__Dashboard__" in o: - d = models.Dashboard() - d.__dict__.update(o["__Dashboard__"]) - return d + return models.Dashboard(**o["__Dashboard__"]) elif "__Slice__" in o: - d = models.Slice() - d.__dict__.update(o["__Slice__"]) - return d + return models.Slice(**o["__Slice__"]) elif "__TableColumn__" in o: - d = TableColumn() - d.__dict__.update(o["__TableColumn__"]) - return d + return TableColumn(**o["__TableColumn__"]) elif "__SqlaTable__" in o: - d = SqlaTable() - d.__dict__.update(o["__SqlaTable__"]) - return d + return SqlaTable(**o["__SqlaTable__"]) elif "__SqlMetric__" in o: - d = SqlMetric() - d.__dict__.update(o["__SqlMetric__"]) - return d + return SqlMetric(**o["__SqlMetric__"]) elif "__datetime__" in o: return datetime.strptime(o["__datetime__"], "%Y-%m-%dT%H:%M:%S") else: diff --git a/tests/import_export_tests.py b/tests/import_export_tests.py index 03068a4ed..30a55ce48 100644 --- a/tests/import_export_tests.py +++ b/tests/import_export_tests.py @@ -18,9 +18,10 @@ import json import unittest +from flask import Flask, g from sqlalchemy.orm.session import make_transient -from superset import db +from superset import db, security_manager from superset.connectors.druid.models import DruidColumn, DruidDatasource, DruidMetric from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn from superset.models import core as models @@ -375,6 +376,10 @@ class ImportExportTests(SupersetTestCase): ) expected_position = dash_with_1_slice.position + # new slice id (auto-incremental) assigned on insert + # id from json is used only for updating position with new id + meta = expected_position["DASHBOARD_CHART_TYPE-10006"]["meta"] + meta["chartId"] = imported_dash.slices[0].id self.assertEquals(expected_position, imported_dash.position) def test_import_dashboard_2_slices(self): @@ -453,6 +458,93 @@ class ImportExportTests(SupersetTestCase): json.loads(imported_dash.json_metadata), ) + def test_import_new_dashboard_slice_reset_ownership(self): + app = Flask("test_import_dashboard_slice_set_user") + with app.app_context(): + admin_user = security_manager.find_user(username="admin") + self.assertTrue(admin_user) + gamma_user = security_manager.find_user(username="gamma") + self.assertTrue(gamma_user) + g.user = gamma_user + + dash_with_1_slice = self._create_dashboard_for_import(id_=10200) + # set another user as an owner of importing dashboard + dash_with_1_slice.created_by = admin_user + dash_with_1_slice.changed_by = admin_user + dash_with_1_slice.owners = [admin_user] + + imported_dash_id = models.Dashboard.import_obj(dash_with_1_slice) + imported_dash = self.get_dash(imported_dash_id) + self.assertEqual(imported_dash.created_by, gamma_user) + self.assertEqual(imported_dash.changed_by, gamma_user) + self.assertEqual(imported_dash.owners, [gamma_user]) + + imported_slc = imported_dash.slices[0] + self.assertEqual(imported_slc.created_by, gamma_user) + self.assertEqual(imported_slc.changed_by, gamma_user) + self.assertEqual(imported_slc.owners, [gamma_user]) + + def test_import_override_dashboard_slice_reset_ownership(self): + app = Flask("test_import_dashboard_slice_set_user") + with app.app_context(): + admin_user = security_manager.find_user(username="admin") + self.assertTrue(admin_user) + gamma_user = security_manager.find_user(username="gamma") + self.assertTrue(gamma_user) + g.user = gamma_user + + dash_with_1_slice = self._create_dashboard_for_import(id_=10300) + + imported_dash_id = models.Dashboard.import_obj(dash_with_1_slice) + imported_dash = self.get_dash(imported_dash_id) + self.assertEqual(imported_dash.created_by, gamma_user) + self.assertEqual(imported_dash.changed_by, gamma_user) + self.assertEqual(imported_dash.owners, [gamma_user]) + + imported_slc = imported_dash.slices[0] + self.assertEqual(imported_slc.created_by, gamma_user) + self.assertEqual(imported_slc.changed_by, gamma_user) + self.assertEqual(imported_slc.owners, [gamma_user]) + + # re-import with another user shouldn't change the permissions + g.user = admin_user + + dash_with_1_slice = self._create_dashboard_for_import(id_=10300) + + imported_dash_id = models.Dashboard.import_obj(dash_with_1_slice) + imported_dash = self.get_dash(imported_dash_id) + self.assertEqual(imported_dash.created_by, gamma_user) + self.assertEqual(imported_dash.changed_by, gamma_user) + self.assertEqual(imported_dash.owners, [gamma_user]) + + imported_slc = imported_dash.slices[0] + self.assertEqual(imported_slc.created_by, gamma_user) + self.assertEqual(imported_slc.changed_by, gamma_user) + self.assertEqual(imported_slc.owners, [gamma_user]) + + def _create_dashboard_for_import(self, id_=10100): + slc = self.create_slice("health_slc" + str(id_), id=id_ + 1) + dash_with_1_slice = self.create_dashboard( + "dash_with_1_slice" + str(id_), slcs=[slc], id=id_ + 2 + ) + dash_with_1_slice.position_json = """ + {{"DASHBOARD_VERSION_KEY": "v2", + "DASHBOARD_CHART_TYPE-{0}": {{ + "type": "DASHBOARD_CHART_TYPE", + "id": {0}, + "children": [], + "meta": {{ + "width": 4, + "height": 50, + "chartId": {0} + }} + }} + }} + """.format( + slc.id + ) + return dash_with_1_slice + def test_import_table_no_metadata(self): table = self.create_table("pure_table", id=10001) imported_id = SqlaTable.import_obj(table, import_time=1989)