Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Remove unnecessary information from response #24056

Merged
merged 8 commits into from
May 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -650,7 +650,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 @@ -190,10 +190,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