Skip to content

Commit

Permalink
chore: Remove unnecessary information from response (#24056)
Browse files Browse the repository at this point in the history
  • Loading branch information
geido authored May 17, 2023
1 parent 4ef9d25 commit 66fb486
Show file tree
Hide file tree
Showing 16 changed files with 346 additions and 25 deletions.
2 changes: 0 additions & 2 deletions superset/charts/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,6 @@ def ensure_thumbnails_enabled(self) -> Optional[Response]:
"owners.first_name",
"owners.id",
"owners.last_name",
"owners.username",
"dashboards.id",
"dashboards.dashboard_title",
"params",
Expand Down Expand Up @@ -183,7 +182,6 @@ def ensure_thumbnails_enabled(self) -> Optional[Response]:
"owners.first_name",
"owners.id",
"owners.last_name",
"owners.username",
"dashboards.id",
"dashboards.dashboard_title",
"params",
Expand Down
7 changes: 5 additions & 2 deletions superset/connectors/sqla/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
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
Expand Down Expand Up @@ -605,7 +605,10 @@ def changed_by_name(self) -> str:

@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}"

Expand Down
3 changes: 0 additions & 3 deletions superset/dashboards/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,6 @@ def ensure_thumbnails_enabled(self) -> Optional[Response]:
"certification_details",
"changed_by.first_name",
"changed_by.last_name",
"changed_by.username",
"changed_by.id",
"changed_by_name",
"changed_by_url",
Expand All @@ -184,10 +183,8 @@ def ensure_thumbnails_enabled(self) -> Optional[Response]:
"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",
Expand Down
4 changes: 2 additions & 2 deletions superset/dashboards/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
4 changes: 1 addition & 3 deletions superset/datasets/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -113,7 +113,6 @@ class DatasetRestApi(BaseSupersetModelRestApi):
"extra",
"kind",
"owners.id",
"owners.username",
"owners.first_name",
"owners.last_name",
"schema",
Expand Down Expand Up @@ -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",
Expand Down
6 changes: 5 additions & 1 deletion superset/models/dashboard.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
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
Expand Down Expand Up @@ -272,7 +273,10 @@ def changed_by_name(self) -> str:

@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}"

Expand Down
6 changes: 5 additions & 1 deletion superset/models/filter_set.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
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
Expand Down Expand Up @@ -67,7 +68,10 @@ def changed_by_name(self) -> str:

@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}"

Expand Down
8 changes: 7 additions & 1 deletion superset/models/slice.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
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
Expand Down Expand Up @@ -339,7 +340,12 @@ def created_by_url(self) -> str:

@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:
Expand Down
1 change: 0 additions & 1 deletion superset/queries/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ class QueryRestApi(BaseSupersetModelRestApi):
"user.first_name",
"user.id",
"user.last_name",
"user.username",
"start_time",
"end_time",
"tmp_table_name",
Expand Down
2 changes: 1 addition & 1 deletion superset/queries/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion superset/tags/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()


Expand Down
109 changes: 108 additions & 1 deletion tests/integration_tests/charts/api_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,114 @@ def test_update_chart(self):
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
Expand Down Expand Up @@ -823,7 +931,6 @@ def test_get_chart(self):
"owners": [
{
"id": 1,
"username": "admin",
"first_name": "admin",
"last_name": "user",
}
Expand Down
Loading

0 comments on commit 66fb486

Please sign in to comment.