diff --git a/superset/charts/api.py b/superset/charts/api.py index 3235d3b0003..6a4bf04aa1a 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -134,7 +134,6 @@ class ChartRestApi(BaseSupersetModelRestApi): "owners.first_name", "owners.id", "owners.last_name", - "owners.username", "dashboards.id", "dashboards.dashboard_title", "params", @@ -183,7 +182,6 @@ class ChartRestApi(BaseSupersetModelRestApi): "owners.first_name", "owners.id", "owners.last_name", - "owners.username", "dashboards.id", "dashboards.dashboard_title", "params", diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 2b64951578a..e339f7b1f4c 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -43,7 +43,7 @@ import numpy as np import pandas as pd import sqlalchemy as sa import sqlparse -from flask import escape, Markup +from flask import current_app, escape, Markup from flask_appbuilder import Model from flask_babel import lazy_gettext as _ from jinja2.exceptions import TemplateError @@ -605,7 +605,10 @@ class SqlaTable( @property def changed_by_url(self) -> str: - if not self.changed_by: + if ( + not self.changed_by + or not current_app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] + ): return "" return f"/superset/profile/{self.changed_by.username}" diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index cd481c24112..be248f7877f 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -172,7 +172,6 @@ class DashboardRestApi(BaseSupersetModelRestApi): "certification_details", "changed_by.first_name", "changed_by.last_name", - "changed_by.username", "changed_by.id", "changed_by_name", "changed_by_url", @@ -184,10 +183,8 @@ class DashboardRestApi(BaseSupersetModelRestApi): "created_by.last_name", "dashboard_title", "owners.id", - "owners.username", "owners.first_name", "owners.last_name", - "owners.email", "roles.id", "roles.name", "is_managed_externally", diff --git a/superset/dashboards/schemas.py b/superset/dashboards/schemas.py index 169d80613d3..32558de4ab0 100644 --- a/superset/dashboards/schemas.py +++ b/superset/dashboards/schemas.py @@ -193,10 +193,10 @@ class DashboardGetResponseSchema(Schema): ) changed_by_name = fields.String() changed_by_url = fields.String() - changed_by = fields.Nested(UserSchema) + changed_by = fields.Nested(UserSchema(exclude=(["username"]))) changed_on = fields.DateTime() charts = fields.List(fields.String(metadata={"description": charts_description})) - owners = fields.List(fields.Nested(UserSchema)) + owners = fields.List(fields.Nested(UserSchema(exclude=(["username"])))) roles = fields.List(fields.Nested(RolesSchema)) tags = fields.Nested(TagSchema, many=True) changed_on_humanized = fields.String(data_key="changed_on_delta_humanized") diff --git a/superset/datasets/api.py b/superset/datasets/api.py index c51be66f8f0..d52e6227932 100644 --- a/superset/datasets/api.py +++ b/superset/datasets/api.py @@ -103,7 +103,7 @@ class DatasetRestApi(BaseSupersetModelRestApi): "changed_by_name", "changed_by_url", "changed_by.first_name", - "changed_by.username", + "changed_by.last_name", "changed_on_utc", "changed_on_delta_humanized", "default_endpoint", @@ -113,7 +113,6 @@ class DatasetRestApi(BaseSupersetModelRestApi): "extra", "kind", "owners.id", - "owners.username", "owners.first_name", "owners.last_name", "schema", @@ -146,7 +145,6 @@ class DatasetRestApi(BaseSupersetModelRestApi): "template_params", "select_star", "owners.id", - "owners.username", "owners.first_name", "owners.last_name", "columns.advanced_data_type", diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index 9292e1acd1b..9afd74f5e38 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -24,6 +24,7 @@ from functools import partial from typing import Any, Callable, Dict, List, Optional, Set, Tuple, Type, Union import sqlalchemy as sqla +from flask import current_app from flask_appbuilder import Model from flask_appbuilder.models.decorators import renders from flask_appbuilder.security.sqla.models import User @@ -272,7 +273,10 @@ class Dashboard(Model, AuditMixinNullable, ImportExportMixin): @property def changed_by_url(self) -> str: - if not self.changed_by: + if ( + not self.changed_by + or not current_app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] + ): return "" return f"/superset/profile/{self.changed_by.username}" diff --git a/superset/models/filter_set.py b/superset/models/filter_set.py index 4bbef264900..1ace5bca32d 100644 --- a/superset/models/filter_set.py +++ b/superset/models/filter_set.py @@ -20,6 +20,7 @@ import json import logging from typing import Any, Dict +from flask import current_app from flask_appbuilder import Model from sqlalchemy import Column, ForeignKey, Integer, MetaData, String, Text from sqlalchemy.orm import relationship @@ -67,7 +68,10 @@ class FilterSet(Model, AuditMixinNullable): @property def changed_by_url(self) -> str: - if not self.changed_by: + if ( + not self.changed_by + or not current_app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] + ): return "" return f"/superset/profile/{self.changed_by.username}" diff --git a/superset/models/slice.py b/superset/models/slice.py index cc975d26350..d08e345d824 100644 --- a/superset/models/slice.py +++ b/superset/models/slice.py @@ -22,6 +22,7 @@ from typing import Any, Dict, Optional, Type, TYPE_CHECKING from urllib import parse import sqlalchemy as sqla +from flask import current_app from flask_appbuilder import Model from flask_appbuilder.models.decorators import renders from markupsafe import escape, Markup @@ -339,7 +340,12 @@ class Slice( # pylint: disable=too-many-public-methods @property def changed_by_url(self) -> str: - return f"/superset/profile/{self.changed_by.username}" # type: ignore + if ( + not self.changed_by + or not current_app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] + ): + return "" + return f"/superset/profile/{self.changed_by.username}" @property def icons(self) -> str: diff --git a/superset/queries/api.py b/superset/queries/api.py index a88b4b5ce81..bc607420242 100644 --- a/superset/queries/api.py +++ b/superset/queries/api.py @@ -82,7 +82,6 @@ class QueryRestApi(BaseSupersetModelRestApi): "user.first_name", "user.id", "user.last_name", - "user.username", "start_time", "end_time", "tmp_table_name", diff --git a/superset/queries/schemas.py b/superset/queries/schemas.py index c29c1c03b6b..b139784c5b5 100644 --- a/superset/queries/schemas.py +++ b/superset/queries/schemas.py @@ -65,7 +65,7 @@ class QuerySchema(Schema): tab_name = fields.String() tmp_table_name = fields.String() tracking_url = fields.String() - user = fields.Nested(UserSchema) + user = fields.Nested(UserSchema(exclude=["username"])) class Meta: # pylint: disable=too-few-public-methods model = Query diff --git a/superset/tags/schemas.py b/superset/tags/schemas.py index 2081adf69fa..71ab005bbc3 100644 --- a/superset/tags/schemas.py +++ b/superset/tags/schemas.py @@ -45,7 +45,7 @@ class TaggedObjectEntityResponseSchema(Schema): name = fields.String() url = fields.String() changed_on = fields.DateTime() - created_by = fields.Nested(UserSchema) + created_by = fields.Nested(UserSchema(exclude=["username"])) creator = fields.String() diff --git a/tests/integration_tests/charts/api_tests.py b/tests/integration_tests/charts/api_tests.py index 5a261fb5520..99b32752814 100644 --- a/tests/integration_tests/charts/api_tests.py +++ b/tests/integration_tests/charts/api_tests.py @@ -605,6 +605,114 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixin): db.session.delete(model) db.session.commit() + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_chart_activity_access_disabled(self): + """ + Chart API: Test ENABLE_BROAD_ACTIVITY_ACCESS = False + """ + access_flag = app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] + app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = False + admin = self.get_user("admin") + birth_names_table_id = SupersetTestCase.get_table(name="birth_names").id + chart_id = self.insert_chart("title", [admin.id], birth_names_table_id).id + chart_data = { + "slice_name": (new_name := "title1_changed"), + } + self.login(username="admin") + uri = f"api/v1/chart/{chart_id}" + rv = self.put_assert_metric(uri, chart_data, "put") + self.assertEqual(rv.status_code, 200) + model = db.session.query(Slice).get(chart_id) + + self.assertEqual(model.slice_name, new_name) + self.assertEqual(model.changed_by_url, "") + + app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = access_flag + db.session.delete(model) + db.session.commit() + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_chart_activity_access_enabled(self): + """ + Chart API: Test ENABLE_BROAD_ACTIVITY_ACCESS = True + """ + access_flag = app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] + app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = True + admin = self.get_user("admin") + birth_names_table_id = SupersetTestCase.get_table(name="birth_names").id + chart_id = self.insert_chart("title", [admin.id], birth_names_table_id).id + chart_data = { + "slice_name": (new_name := "title1_changed"), + } + self.login(username="admin") + uri = f"api/v1/chart/{chart_id}" + rv = self.put_assert_metric(uri, chart_data, "put") + self.assertEqual(rv.status_code, 200) + model = db.session.query(Slice).get(chart_id) + + self.assertEqual(model.slice_name, new_name) + self.assertEqual(model.changed_by_url, "/superset/profile/admin") + + app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = access_flag + db.session.delete(model) + db.session.commit() + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_chart_get_list_no_username(self): + """ + Chart API: Tests that no username is returned + """ + admin = self.get_user("admin") + birth_names_table_id = SupersetTestCase.get_table(name="birth_names").id + chart_id = self.insert_chart("title", [admin.id], birth_names_table_id).id + chart_data = { + "slice_name": (new_name := "title1_changed"), + "owners": [admin.id], + } + self.login(username="admin") + uri = f"api/v1/chart/{chart_id}" + rv = self.put_assert_metric(uri, chart_data, "put") + self.assertEqual(rv.status_code, 200) + model = db.session.query(Slice).get(chart_id) + + response = self.get_assert_metric("api/v1/chart/", "get_list") + res = json.loads(response.data.decode("utf-8"))["result"] + + current_chart = [d for d in res if d["id"] == chart_id][0] + self.assertEqual(current_chart["slice_name"], new_name) + self.assertNotIn("username", current_chart["changed_by"].keys()) + self.assertNotIn("username", current_chart["owners"][0].keys()) + + db.session.delete(model) + db.session.commit() + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_chart_get_no_username(self): + """ + Chart API: Tests that no username is returned + """ + admin = self.get_user("admin") + birth_names_table_id = SupersetTestCase.get_table(name="birth_names").id + chart_id = self.insert_chart("title", [admin.id], birth_names_table_id).id + chart_data = { + "slice_name": (new_name := "title1_changed"), + "owners": [admin.id], + } + self.login(username="admin") + uri = f"api/v1/chart/{chart_id}" + rv = self.put_assert_metric(uri, chart_data, "put") + self.assertEqual(rv.status_code, 200) + model = db.session.query(Slice).get(chart_id) + + response = self.get_assert_metric(uri, "get") + res = json.loads(response.data.decode("utf-8"))["result"] + + self.assertEqual(res["slice_name"], new_name) + self.assertNotIn("username", res["owners"][0].keys()) + + db.session.delete(model) + db.session.commit() + def test_update_chart_new_owner_not_admin(self): """ Chart API: Test update set new owner implicitly adds logged in owner @@ -823,7 +931,6 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixin): "owners": [ { "id": 1, - "username": "admin", "first_name": "admin", "last_name": "user", } diff --git a/tests/integration_tests/dashboards/api_tests.py b/tests/integration_tests/dashboards/api_tests.py index 2ba00506d76..9c5ad217d1e 100644 --- a/tests/integration_tests/dashboards/api_tests.py +++ b/tests/integration_tests/dashboards/api_tests.py @@ -31,7 +31,7 @@ import yaml from freezegun import freeze_time from sqlalchemy import and_ -from superset import db, security_manager +from superset import app, db, security_manager from superset.models.dashboard import Dashboard from superset.models.core import FavStar, FavStarClassName from superset.reports.models import ReportSchedule, ReportScheduleType @@ -424,7 +424,6 @@ class TestDashboardApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixi "owners": [ { "id": 1, - "username": "admin", "first_name": "admin", "last_name": "user", } @@ -1405,6 +1404,109 @@ class TestDashboardApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixi db.session.delete(model) db.session.commit() + def test_dashboard_activity_access_disabled(self): + """ + Dashboard API: Test ENABLE_BROAD_ACTIVITY_ACCESS = False + """ + access_flag = app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] + app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = False + admin = self.get_user("admin") + admin_role = self.get_role("Admin") + dashboard_id = self.insert_dashboard( + "title1", "slug1", [admin.id], roles=[admin_role.id] + ).id + self.login(username="admin") + uri = f"api/v1/dashboard/{dashboard_id}" + dashboard_data = {"dashboard_title": "title2"} + rv = self.client.put(uri, json=dashboard_data) + self.assertEqual(rv.status_code, 200) + model = db.session.query(Dashboard).get(dashboard_id) + + self.assertEqual(model.dashboard_title, "title2") + self.assertEqual(model.changed_by_url, "") + + app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = access_flag + db.session.delete(model) + db.session.commit() + + def test_dashboard_activity_access_enabled(self): + """ + Dashboard API: Test ENABLE_BROAD_ACTIVITY_ACCESS = True + """ + access_flag = app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] + app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = True + admin = self.get_user("admin") + admin_role = self.get_role("Admin") + dashboard_id = self.insert_dashboard( + "title1", "slug1", [admin.id], roles=[admin_role.id] + ).id + self.login(username="admin") + uri = f"api/v1/dashboard/{dashboard_id}" + dashboard_data = {"dashboard_title": "title2"} + rv = self.client.put(uri, json=dashboard_data) + self.assertEqual(rv.status_code, 200) + model = db.session.query(Dashboard).get(dashboard_id) + + self.assertEqual(model.dashboard_title, "title2") + self.assertEqual(model.changed_by_url, "/superset/profile/admin") + + app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = access_flag + db.session.delete(model) + db.session.commit() + + def test_dashboard_get_list_no_username(self): + """ + Dashboard API: Tests that no username is returned + """ + admin = self.get_user("admin") + admin_role = self.get_role("Admin") + dashboard_id = self.insert_dashboard( + "title1", "slug1", [admin.id], roles=[admin_role.id] + ).id + model = db.session.query(Dashboard).get(dashboard_id) + self.login(username="admin") + uri = f"api/v1/dashboard/{dashboard_id}" + dashboard_data = {"dashboard_title": "title2"} + rv = self.client.put(uri, json=dashboard_data) + self.assertEqual(rv.status_code, 200) + + response = self.get_assert_metric("api/v1/dashboard/", "get_list") + res = json.loads(response.data.decode("utf-8"))["result"] + + current_dash = [d for d in res if d["id"] == dashboard_id][0] + self.assertEqual(current_dash["dashboard_title"], "title2") + self.assertNotIn("username", current_dash["changed_by"].keys()) + self.assertNotIn("username", current_dash["owners"][0].keys()) + + db.session.delete(model) + db.session.commit() + + def test_dashboard_get_no_username(self): + """ + Dashboard API: Tests that no username is returned + """ + admin = self.get_user("admin") + admin_role = self.get_role("Admin") + dashboard_id = self.insert_dashboard( + "title1", "slug1", [admin.id], roles=[admin_role.id] + ).id + model = db.session.query(Dashboard).get(dashboard_id) + self.login(username="admin") + uri = f"api/v1/dashboard/{dashboard_id}" + dashboard_data = {"dashboard_title": "title2"} + rv = self.client.put(uri, json=dashboard_data) + self.assertEqual(rv.status_code, 200) + + response = self.get_assert_metric(uri, "get") + res = json.loads(response.data.decode("utf-8"))["result"] + + self.assertEqual(res["dashboard_title"], "title2") + self.assertNotIn("username", res["changed_by"].keys()) + self.assertNotIn("username", res["owners"][0].keys()) + + db.session.delete(model) + db.session.commit() + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_update_dashboard_chart_owners(self): """ diff --git a/tests/integration_tests/datasets/api_tests.py b/tests/integration_tests/datasets/api_tests.py index 60a8ee58bdf..055cf4779eb 100644 --- a/tests/integration_tests/datasets/api_tests.py +++ b/tests/integration_tests/datasets/api_tests.py @@ -28,6 +28,7 @@ import yaml from sqlalchemy.orm import joinedload from sqlalchemy.sql import func +from superset import app from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn from superset.dao.exceptions import ( DAOCreateFailedError, @@ -1315,6 +1316,107 @@ class TestDatasetApi(SupersetTestCase): db.session.delete(dataset) db.session.commit() + def test_dataset_get_list_no_username(self): + """ + Dataset API: Tests that no username is returned + """ + if backend() == "sqlite": + return + + dataset = self.insert_default_dataset() + self.login(username="admin") + table_data = {"description": "changed_description"} + uri = f"api/v1/dataset/{dataset.id}" + rv = self.client.put(uri, json=table_data) + self.assertEqual(rv.status_code, 200) + + response = self.get_assert_metric("api/v1/dataset/", "get_list") + res = json.loads(response.data.decode("utf-8"))["result"] + + current_dataset = [d for d in res if d["id"] == dataset.id][0] + self.assertEqual(current_dataset["description"], "changed_description") + self.assertNotIn("username", current_dataset["changed_by"].keys()) + + db.session.delete(dataset) + db.session.commit() + + def test_dataset_get_no_username(self): + """ + Dataset API: Tests that no username is returned + """ + if backend() == "sqlite": + return + + dataset = self.insert_default_dataset() + self.login(username="admin") + table_data = {"description": "changed_description"} + uri = f"api/v1/dataset/{dataset.id}" + rv = self.client.put(uri, json=table_data) + self.assertEqual(rv.status_code, 200) + + response = self.get_assert_metric(uri, "get") + res = json.loads(response.data.decode("utf-8"))["result"] + + self.assertEqual(res["description"], "changed_description") + self.assertNotIn("username", res["changed_by"].keys()) + + db.session.delete(dataset) + db.session.commit() + + def test_dataset_activity_access_enabled(self): + """ + Dataset API: Test ENABLE_BROAD_ACTIVITY_ACCESS = True + """ + if backend() == "sqlite": + return + + access_flag = app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] + app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = True + dataset = self.insert_default_dataset() + self.login(username="admin") + table_data = {"description": "changed_description"} + uri = f"api/v1/dataset/{dataset.id}" + rv = self.client.put(uri, json=table_data) + self.assertEqual(rv.status_code, 200) + + response = self.get_assert_metric("api/v1/dataset/", "get_list") + res = json.loads(response.data.decode("utf-8"))["result"] + + current_dataset = [d for d in res if d["id"] == dataset.id][0] + self.assertEqual(current_dataset["description"], "changed_description") + self.assertEqual(current_dataset["changed_by_url"], "/superset/profile/admin") + + app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = access_flag + db.session.delete(dataset) + db.session.commit() + + def test_dataset_activity_access_disabled(self): + """ + Dataset API: Test ENABLE_BROAD_ACTIVITY_ACCESS = Fase + """ + if backend() == "sqlite": + return + + access_flag = app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] + app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = False + dataset = self.insert_default_dataset() + self.login(username="admin") + table_data = {"description": "changed_description"} + uri = f"api/v1/dataset/{dataset.id}" + rv = self.put_assert_metric(uri, table_data, "put") + self.assertEqual(rv.status_code, 200) + + response = self.get_assert_metric("api/v1/dataset/", "get_list") + res = json.loads(response.data.decode("utf-8"))["result"] + + current_dataset = [d for d in res if d["id"] == dataset.id][0] + self.assertEqual(current_dataset["description"], "changed_description") + self.assertEqual(current_dataset["changed_by_url"], "") + + app.config["ENABLE_BROAD_ACTIVITY_ACCESS"] = access_flag + db.session.delete(dataset) + db.session.commit() + def test_update_dataset_item_not_owned(self): """ Dataset API: Test update dataset item not owned diff --git a/tests/integration_tests/queries/api_tests.py b/tests/integration_tests/queries/api_tests.py index 7abcb31df1e..b3b291cf966 100644 --- a/tests/integration_tests/queries/api_tests.py +++ b/tests/integration_tests/queries/api_tests.py @@ -285,7 +285,6 @@ class TestQueryApi(SupersetTestCase): "first_name", "id", "last_name", - "username", ] assert list(data["result"][0]["database"].keys()) == [ "database_name", diff --git a/tests/integration_tests/sqllab_tests.py b/tests/integration_tests/sqllab_tests.py index 27ccdde96be..16cc16d2645 100644 --- a/tests/integration_tests/sqllab_tests.py +++ b/tests/integration_tests/sqllab_tests.py @@ -647,9 +647,11 @@ class TestSqlLab(SupersetTestCase): admin = security_manager.find_user("admin") gamma_sqllab = security_manager.find_user("gamma_sqllab") self.assertEqual(3, len(data["result"])) - user_queries = [result.get("user").get("username") for result in data["result"]] - assert admin.username in user_queries - assert gamma_sqllab.username in user_queries + user_queries = [ + result.get("user").get("first_name") for result in data["result"] + ] + assert admin.first_name in user_queries + assert gamma_sqllab.first_name in user_queries def test_query_api_can_access_all_queries(self) -> None: """