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

feat: Authorizing guest access to embedded dashboards #17757

Merged
merged 33 commits into from
Jan 21, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
a21a417
helper methods and dashboard access
suddjian Dec 7, 2021
ad3572e
guest token dashboard authz
suddjian Dec 11, 2021
4fd8715
adjust csrf exempt list
suddjian Dec 11, 2021
7353826
eums don't work that way
suddjian Dec 11, 2021
5c0a7f5
Remove unnecessary import
suddjian Dec 20, 2021
2141f2d
move row level security tests to their own file
suddjian Dec 22, 2021
bfdbad5
a bit of refactoring
suddjian Dec 22, 2021
36a1224
add guest token security tests
suddjian Dec 22, 2021
7181333
refactor tests
suddjian Jan 6, 2022
27a2ec3
clean imports
suddjian Jan 6, 2022
09aed96
variable names can be too long apparently
suddjian Jan 13, 2022
c7bfa96
missing argument to get_user_roles
suddjian Jan 13, 2022
aa672b9
don't redefine builtins
suddjian Jan 13, 2022
091786d
remove unused imports
suddjian Jan 13, 2022
5e23ce9
fix test import
suddjian Jan 13, 2022
a62b2f5
default to global user when getting roles
suddjian Jan 13, 2022
e9d50c2
Merge branch 'embedded' into guest-token-authz
suddjian Jan 13, 2022
67affe4
Merge branch 'embedded' into guest-token-authz
suddjian Jan 13, 2022
4d5a691
missing import
suddjian Jan 13, 2022
13a2038
mock it
suddjian Jan 13, 2022
0be28da
test get_user_roles
suddjian Jan 13, 2022
c5bded9
infer g.user for ease of tests
suddjian Jan 14, 2022
ebf9400
remove redundant check
suddjian Jan 14, 2022
deceb33
tests for guest user security manager fns
suddjian Jan 14, 2022
128f3a0
use algo to get rid of warning messages
suddjian Jan 14, 2022
ede1367
tweaking access checks
suddjian Jan 18, 2022
38a89ae
fix guest token security tests
suddjian Jan 18, 2022
1927a1c
missing imports
suddjian Jan 18, 2022
128b90c
more tests
suddjian Jan 18, 2022
b73a81f
more testing and also some small refactoring
suddjian Jan 20, 2022
eccec4d
move validation out of parsing
suddjian Jan 20, 2022
e833ef5
fix dashboard access check again
suddjian Jan 20, 2022
5c3a5fb
add more test
lilykuang Jan 21, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 1 addition & 15 deletions superset/common/request_contexed_based.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,10 @@
# under the License.
from __future__ import annotations

from typing import List, TYPE_CHECKING

from flask import g

from superset import conf, security_manager

if TYPE_CHECKING:
from flask_appbuilder.security.sqla.models import Role


def get_user_roles() -> List[Role]:
suddjian marked this conversation as resolved.
Show resolved Hide resolved
if g.user.is_anonymous:
public_role = conf.get("AUTH_ROLE_PUBLIC")
return [security_manager.get_public_role()] if public_role else []
return g.user.roles


def is_user_admin() -> bool:
user_roles = [role.name.lower() for role in get_user_roles()]
user_roles = [role.name.lower() for role in security_manager.get_user_roles()]
admin_role = conf.get("AUTH_ROLE_ADMIN").lower()
return admin_role in user_roles
8 changes: 6 additions & 2 deletions superset/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,11 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]:
WTF_CSRF_ENABLED = True

# Add endpoints that need to be exempt from CSRF protection
WTF_CSRF_EXEMPT_LIST = ["superset.views.core.log", "superset.charts.data.api.data"]
WTF_CSRF_EXEMPT_LIST = [
"superset.views.core.log",
"superset.views.core.explore_json",
"superset.charts.data.api.data",
]

# Whether to run the web server in debug mode or not
DEBUG = os.environ.get("FLASK_ENV") == "development"
Expand Down Expand Up @@ -389,7 +393,7 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]:
# a custom security config could potentially give access to setting filters on
# tables that users do not have access to.
"ROW_LEVEL_SECURITY": True,
"EMBEDDED_SUPERSET": False,
"EMBEDDED_SUPERSET": False, # This requires that the public role be available
# Enables Alerts and reports new implementation
"ALERT_REPORTS": False,
# Enable experimental feature to search for other dashboards
Expand Down
24 changes: 19 additions & 5 deletions superset/dashboards/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
# under the License.
from typing import Any, Optional

from flask import g
from flask_appbuilder.security.sqla.models import Role
from flask_babel import lazy_gettext as _
from sqlalchemy import and_, or_
Expand All @@ -25,7 +26,8 @@
from superset.models.core import FavStar
from superset.models.dashboard import Dashboard
from superset.models.slice import Slice
from superset.views.base import BaseFilter, get_user_roles, is_user_admin
from superset.security.guest_token import GuestTokenResourceType, GuestUser
from superset.views.base import BaseFilter, is_user_admin
from superset.views.base_api import BaseFavoriteFilter


Expand Down Expand Up @@ -112,7 +114,7 @@ def apply(self, query: Query, value: Any) -> Query:
)
)

dashboard_rbac_or_filters = []
feature_flagged_filters = []
if is_feature_enabled("DASHBOARD_RBAC"):
roles_based_query = (
db.session.query(Dashboard.id)
Expand All @@ -121,19 +123,31 @@ def apply(self, query: Query, value: Any) -> Query:
and_(
Dashboard.published.is_(True),
dashboard_has_roles,
Role.id.in_([x.id for x in get_user_roles()]),
Role.id.in_([x.id for x in security_manager.get_user_roles()]),
),
)
)

dashboard_rbac_or_filters.append(Dashboard.id.in_(roles_based_query))
feature_flagged_filters.append(Dashboard.id.in_(roles_based_query))

if is_feature_enabled("EMBEDDED_SUPERSET") and security_manager.is_guest_user(
suddjian marked this conversation as resolved.
Show resolved Hide resolved
g.user
):
guest_user: GuestUser = g.user
embedded_dashboard_ids = [
r["id"]
for r in guest_user.resources
if r["type"] == GuestTokenResourceType.DASHBOARD.value
]
if len(embedded_dashboard_ids) != 0:
feature_flagged_filters.append(Dashboard.id.in_(embedded_dashboard_ids))

query = query.filter(
or_(
Dashboard.id.in_(owner_ids_query),
Dashboard.id.in_(datasource_perm_query),
Dashboard.id.in_(users_favorite_dash_query),
*dashboard_rbac_or_filters,
*feature_flagged_filters,
)
)

Expand Down
2 changes: 1 addition & 1 deletion superset/security/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class UserSchema(Schema):


class ResourceSchema(Schema):
type = fields.String(required=True)
type = fields.String(required=True) # todo figure out how to make this an enum
suddjian marked this conversation as resolved.
Show resolved Hide resolved
id = fields.String(required=True)
rls = fields.String()

Expand Down
14 changes: 11 additions & 3 deletions superset/security/guest_token.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
from enum import Enum
from typing import List, Optional, TypedDict, Union

from flask_appbuilder.security.sqla.models import Role
Expand All @@ -26,17 +27,24 @@ class GuestTokenUser(TypedDict, total=False):
last_name: str


class GuestTokenResourceType(Enum):
DASHBOARD = "dashboard"


class GuestTokenResource(TypedDict):
type: str
type: GuestTokenResourceType
id: Union[str, int]
rls: Optional[str]


GuestTokenResources = List[GuestTokenResource]


class GuestToken(TypedDict):
iat: float
exp: float
user: GuestTokenUser
resources: List[GuestTokenResource]
resources: GuestTokenResources


class GuestUser(AnonymousUserMixin):
Expand All @@ -50,7 +58,7 @@ class GuestUser(AnonymousUserMixin):
def is_authenticated(self) -> bool:
"""
This is set to true because guest users should be considered authenticated,
at least in most places. The treatment of this flag is pretty inconsistent.
at least in most places. The treatment of this flag is kind of inconsistent.
Copy link
Member Author

Choose a reason for hiding this comment

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

lol apparently this got downgraded to "kind of" somewhere along the way

"""
return True

Expand Down
74 changes: 62 additions & 12 deletions superset/security/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@
from superset.security.guest_token import (
GuestToken,
GuestTokenResource,
GuestTokenResources,
GuestTokenResourceType,
GuestTokenUser,
GuestUser,
)
Expand Down Expand Up @@ -278,7 +280,7 @@ def can_access(self, permission_name: str, view_name: str) -> bool:
"""

user = g.user
if user.is_anonymous:
if user.is_anonymous and not self.is_guest_user(user):
return self.is_item_public(permission_name, view_name)
return self._has_view_access(user, permission_name, view_name)

Expand Down Expand Up @@ -1067,11 +1069,17 @@ def raise_for_access(

assert datasource

is_dashboard_access_check_applicable = feature_flag_manager.is_feature_enabled(
"DASHBOARD_RBAC"
) or feature_flag_manager.is_feature_enabled(
"EMBEDDED_SUPERSET"
suddjian marked this conversation as resolved.
Show resolved Hide resolved
)

if not (
self.can_access_schema(datasource)
or self.can_access("datasource_access", datasource.perm or "")
or (
feature_flag_manager.is_feature_enabled("DASHBOARD_RBAC")
is_dashboard_access_check_applicable
and self.can_access_based_on_dashboard(datasource)
)
):
Expand All @@ -1097,6 +1105,12 @@ def get_user_by_username(
def get_anonymous_user(self) -> User: # pylint: disable=no-self-use
return AnonymousUserMixin()

def get_user_roles(self) -> List[Role]:
if g.user.is_anonymous:
suddjian marked this conversation as resolved.
Show resolved Hide resolved
public_role = current_app.config.get("AUTH_ROLE_PUBLIC")
return [self.get_public_role()] if public_role else []
return g.user.roles
suddjian marked this conversation as resolved.
Show resolved Hide resolved

def get_rls_filters(self, table: "BaseDatasource") -> List[SqlaQuery]:
"""
Retrieves the appropriate row level security filters for the current user and
Expand Down Expand Up @@ -1195,8 +1209,7 @@ def raise_for_user_activity_access(user_id: int) -> None:
)
)

@staticmethod
def raise_for_dashboard_access(dashboard: "Dashboard") -> None:
def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None:
"""
Raise an exception if the user cannot access the dashboard.

Expand All @@ -1206,21 +1219,28 @@ def raise_for_dashboard_access(dashboard: "Dashboard") -> None:
# pylint: disable=import-outside-toplevel
from superset import is_feature_enabled
from superset.dashboards.commands.exceptions import DashboardAccessDeniedError
from superset.views.base import get_user_roles, is_user_admin
from superset.views.base import is_user_admin
from superset.views.utils import is_owner

has_rbac_access = True

if is_feature_enabled("DASHBOARD_RBAC"):
has_rbac_access = any(
dashboard_role.id in [user_role.id for user_role in get_user_roles()]
def has_rbac_access() -> bool:
return is_feature_enabled("DASHBOARD_RBAC") and any(
dashboard_role.id
in [user_role.id for user_role in self.get_user_roles()]
for dashboard_role in dashboard.roles
)

has_published_access = (
not is_feature_enabled("DASHBOARD_RBAC")
and not self.is_guest_user(g.user)
suddjian marked this conversation as resolved.
Show resolved Hide resolved
and dashboard.published
)

can_access = (
is_user_admin()
or is_owner(dashboard, g.user)
or (dashboard.published and has_rbac_access)
or has_published_access
or has_rbac_access()
or self.has_guest_access(GuestTokenResourceType.DASHBOARD, dashboard.id)
suddjian marked this conversation as resolved.
Show resolved Hide resolved
or (not dashboard.published and not dashboard.roles)
)

Expand Down Expand Up @@ -1255,7 +1275,7 @@ def _get_current_epoch_time() -> float:
return time.time()

def create_guest_access_token(
self, user: GuestTokenUser, resources: List[GuestTokenResource]
self, user: GuestTokenUser, resources: GuestTokenResources
) -> bytes:
secret = current_app.config["GUEST_TOKEN_JWT_SECRET"]
algo = current_app.config["GUEST_TOKEN_JWT_ALGO"]
Expand Down Expand Up @@ -1319,3 +1339,33 @@ def parse_jwt_guest_token(raw_token: str) -> GuestToken:
if token.get("resources") is None:
raise ValueError("Guest token does not contain a resources claim")
return cast(GuestToken, token)

@staticmethod
def is_guest_user(user: Any) -> bool:
# pylint: disable=import-outside-toplevel
from superset import is_feature_enabled

if not is_feature_enabled("EMBEDDED_SUPERSET"):
return False
return hasattr(user, "is_guest_user") and user.is_guest_user
suddjian marked this conversation as resolved.
Show resolved Hide resolved

def get_current_guest_user_if_guest(self) -> Optional[GuestUser]:
# pylint: disable=import-outside-toplevel
from superset.extensions import feature_flag_manager
suddjian marked this conversation as resolved.
Show resolved Hide resolved

if self.is_guest_user(g.user):
return g.user
return None
suddjian marked this conversation as resolved.
Show resolved Hide resolved

def has_guest_access(
self, resource_type: GuestTokenResourceType, id: Union[str, int]
) -> bool:
user = self.get_current_guest_user_if_guest()
if not user:
suddjian marked this conversation as resolved.
Show resolved Hide resolved
return False

strid = str(id)
for resource in user.resources:
suddjian marked this conversation as resolved.
Show resolved Hide resolved
if resource["type"] == resource_type.value and str(resource["id"]) == strid:
return True
return False
suddjian marked this conversation as resolved.
Show resolved Hide resolved
9 changes: 1 addition & 8 deletions superset/views/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -263,15 +263,8 @@ def create_table_permissions(table: models.SqlaTable) -> None:
security_manager.add_permission_view_menu("schema_access", table.schema_perm)


def get_user_roles() -> List[Role]:
if g.user.is_anonymous:
public_role = conf.get("AUTH_ROLE_PUBLIC")
return [security_manager.find_role(public_role)] if public_role else []
return g.user.roles


def is_user_admin() -> bool:
user_roles = [role.name.lower() for role in list(get_user_roles())]
user_roles = [role.name.lower() for role in list(security_manager.get_user_roles())]
return "admin" in user_roles


Expand Down
5 changes: 3 additions & 2 deletions superset/views/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@
data_payload_response,
generate_download_headers,
get_error_msg,
get_user_roles,
handle_api_exception,
json_error_response,
json_errors_response,
Expand Down Expand Up @@ -1886,7 +1885,9 @@ def publish( # pylint: disable=no-self-use
f"ERROR: cannot find dashboard {dashboard_id}", status=404
)

edit_perm = is_owner(dash, g.user) or admin_role in get_user_roles()
edit_perm = (
is_owner(dash, g.user) or admin_role in security_manager.get_user_roles()
)
if not edit_perm:
username = g.user.username if hasattr(g.user, "username") else "user"
return json_error_response(
Expand Down