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 and eschutho committed Jun 6, 2023
1 parent 0a48ffb commit 12bcc5b
Show file tree
Hide file tree
Showing 16 changed files with 404 additions and 24 deletions.
2 changes: 0 additions & 2 deletions superset/charts/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,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 @@ -171,7 +170,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 @@ -44,7 +44,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 @@ -655,7 +655,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 @@ -167,7 +167,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 @@ -179,10 +178,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 @@ -163,10 +163,10 @@ class DashboardGetResponseSchema(Schema):
certification_details = fields.String(description=certification_details_description)
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(description=charts_description))
owners = fields.List(fields.Nested(UserSchema))
owners = fields.List(fields.Nested(UserSchema(exclude=(["username"]))))
roles = fields.List(fields.Nested(RolesSchema))
changed_on_humanized = fields.String(data_key="changed_on_delta_humanized")
is_managed_externally = fields.Boolean(allow_none=True, default=False)
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 @@ -23,6 +23,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 @@ -264,7 +265,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 @@ -326,7 +327,12 @@ def slice_link(self) -> Markup:

@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
59 changes: 59 additions & 0 deletions superset/tags/schemas.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
from marshmallow import fields, Schema

from superset.dashboards.schemas import UserSchema

delete_tags_schema = {"type": "array", "items": {"type": "string"}}

object_type_description = "A title for the tag."

openapi_spec_methods_override = {
"get": {"get": {"description": "Get a tag detail information."}},
"get_list": {
"get": {
"description": "Get a list of tags, use Rison or JSON query "
"parameters for filtering, sorting, pagination and "
" for selecting specific columns and metadata.",
}
},
"info": {
"get": {
"description": "Several metadata information about tag API " "endpoints.",
}
},
}


class TaggedObjectEntityResponseSchema(Schema):
id = fields.Int()
type = fields.String()
name = fields.String()
url = fields.String()
changed_on = fields.DateTime()
created_by = fields.Nested(UserSchema(exclude=["username"]))
creator = fields.String()


class TagGetResponseSchema(Schema):
id = fields.Int()
name = fields.String()
type = fields.String()


class TagPostSchema(Schema):
tags = fields.List(fields.String())
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 12bcc5b

Please sign in to comment.