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

fix: revert back to use security manager authz for dashboard when get by uuid #23330

Merged
merged 15 commits into from
Mar 16, 2023
36 changes: 22 additions & 14 deletions superset/dashboards/dao.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,10 @@
from flask_appbuilder.models.sqla.interface import SQLAInterface
from sqlalchemy.exc import SQLAlchemyError

from superset import security_manager
from superset.dao.base import BaseDAO
from superset.dashboards.commands.exceptions import DashboardNotFoundError
from superset.dashboards.filters import DashboardAccessFilter
from superset.dashboards.filters import DashboardAccessFilter, is_uuid
from superset.extensions import db
from superset.models.core import FavStar, FavStarClassName
from superset.models.dashboard import Dashboard, id_or_slug_filter
Expand All @@ -41,21 +42,28 @@ class DashboardDAO(BaseDAO):

@classmethod
def get_by_id_or_slug(cls, id_or_slug: Union[int, str]) -> Dashboard:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we change this method name to get_by_id_slug_or_uuid?

Copy link
Author

@zephyring zephyring Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a better naming could just be get_by_identifier but it could touches too many files if we want to change every id_or_slug to identifier. Note that uuid is also id in general though. So I think this naming should be fine.

query = (
db.session.query(Dashboard)
.filter(id_or_slug_filter(id_or_slug))
.outerjoin(Slice, Dashboard.slices)
.outerjoin(Slice.table)
.outerjoin(Dashboard.owners)
.outerjoin(Dashboard.roles)
)
# Apply dashboard base filters
query = cls.base_filter("id", SQLAInterface(Dashboard, db.session)).apply(
query, None
)
dashboard = query.one_or_none()
if is_uuid(id_or_slug):
# just get dashboard if it's uuid
dashboard = Dashboard.get(id_or_slug)
else:
query = (
db.session.query(Dashboard)
.filter(id_or_slug_filter(id_or_slug))
.outerjoin(Slice, Dashboard.slices)
.outerjoin(Slice.table)
.outerjoin(Dashboard.owners)
.outerjoin(Dashboard.roles)
)
# Apply dashboard base filters
query = cls.base_filter("id", SQLAInterface(Dashboard, db.session)).apply(
query, None
)
dashboard = query.one_or_none()
if not dashboard:
raise DashboardNotFoundError()

# make sure we still have basic access check from security manager
security_manager.raise_for_dashboard_access(dashboard)
return dashboard

@staticmethod
Expand Down
13 changes: 2 additions & 11 deletions superset/dashboards/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
import uuid
from typing import Any, Optional, Union
from typing import Any, Optional

from flask import g
from flask_appbuilder.security.sqla.models import Role
Expand All @@ -26,7 +25,7 @@
from superset import db, is_feature_enabled, security_manager
from superset.connectors.sqla.models import SqlaTable
from superset.models.core import Database, FavStar
from superset.models.dashboard import Dashboard
from superset.models.dashboard import Dashboard, is_uuid
from superset.models.embedded_dashboard import EmbeddedDashboard
from superset.models.slice import Slice
from superset.security.guest_token import GuestTokenResourceType, GuestUser
Expand Down Expand Up @@ -89,14 +88,6 @@ class DashboardTagFilter(BaseTagFilter): # pylint: disable=too-few-public-metho
model = Dashboard


def is_uuid(value: Union[str, int]) -> bool:
try:
uuid.UUID(str(value))
return True
except ValueError:
return False


class DashboardAccessFilter(BaseFilter): # pylint: disable=too-few-public-methods
"""
List dashboards with the following criteria:
Expand Down
4 changes: 2 additions & 2 deletions superset/dashboards/permalink/commands/create.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ def __init__(
def run(self) -> str:
self.validate()
try:
DashboardDAO.get_by_id_or_slug(self.dashboard_id)
dashboard = DashboardDAO.get_by_id_or_slug(self.dashboard_id)
value = {
"dashboardId": self.dashboard_id,
"dashboardId": str(dashboard.uuid),
"state": self.state,
}
user_id = get_user_id()
Expand Down
23 changes: 20 additions & 3 deletions superset/models/dashboard.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import json
import logging
import uuid
from collections import defaultdict
from functools import partial
from typing import Any, Callable, Dict, List, Optional, Set, Tuple, Type, Union
Expand Down Expand Up @@ -450,11 +451,27 @@ def get(cls, id_or_slug: Union[str, int]) -> Dashboard:
return qry.one_or_none()


def is_uuid(value: Union[str, int]) -> bool:
try:
uuid.UUID(str(value))
return True
except ValueError:
return False


def is_int(value: Union[str, int]) -> bool:
try:
int(value)
return True
except ValueError:
return False


def id_or_slug_filter(id_or_slug: Union[int, str]) -> BinaryExpression:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we change this method name also?

if isinstance(id_or_slug, int):
return Dashboard.id == id_or_slug
if id_or_slug.isdigit():
if is_int(id_or_slug):
return Dashboard.id == int(id_or_slug)
if is_uuid(id_or_slug):
return Dashboard.uuid == uuid.UUID(str(id_or_slug))
return Dashboard.slug == id_or_slug


Expand Down
8 changes: 6 additions & 2 deletions superset/reports/commands/execute.py
Original file line number Diff line number Diff line change
Expand Up @@ -179,15 +179,19 @@ def _get_url(
dashboard_state = self._report_schedule.extra.get("dashboard")
if dashboard_state:
permalink_key = CreateDashboardPermalinkCommand(
dashboard_id=str(self._report_schedule.dashboard_id),
dashboard_id=str(self._report_schedule.dashboard.uuid),
state=dashboard_state,
).run()
return get_url_path("Superset.dashboard_permalink", key=permalink_key)

dashboard = self._report_schedule.dashboard
dashboard_id_or_slug = (
dashboard.uuid if dashboard and dashboard.uuid else dashboard.id
)
return get_url_path(
"Superset.dashboard",
user_friendly=user_friendly,
dashboard_id_or_slug=self._report_schedule.dashboard_id,
dashboard_id_or_slug=dashboard_id_or_slug,
force=force,
**kwargs,
)
Expand Down
3 changes: 2 additions & 1 deletion tests/integration_tests/dashboards/permalink/api_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@ def test_get(test_client, login_as_admin, dashboard_id: int, permalink_salt: str
resp = test_client.get(f"api/v1/dashboard/permalink/{key}")
assert resp.status_code == 200
result = resp.json
assert result["dashboardId"] == str(dashboard_id)
dashboard_uuid = result["dashboardId"]
assert Dashboard.get(dashboard_uuid).id == dashboard_id
assert result["state"] == STATE
id_ = decode_permalink_id(key, permalink_salt)
db.session.query(KeyValueEntry).filter_by(id=id_).delete()
Expand Down