feat: Authorizing guest access to embedded dashboards (#17757)

* helper methods and dashboard access

* guest token dashboard authz

* adjust csrf exempt list

* eums don't work that way

* Remove unnecessary import

* move row level security tests to their own file

* a bit of refactoring

* add guest token security tests

* refactor tests

* clean imports

* variable names can be too long apparently

* missing argument to get_user_roles

* don't redefine builtins

* remove unused imports

* fix test import

* default to global user when getting roles

* missing import

* mock it

* test get_user_roles

* infer g.user for ease of tests

* remove redundant check

* tests for guest user security manager fns

* use algo to get rid of warning messages

* tweaking access checks

* fix guest token security tests

* missing imports

* more tests

* more testing and also some small refactoring

* move validation out of parsing

* fix dashboard access check again

* add more test

Co-authored-by: Lily Kuang <lily@preset.io>

superset/common/request_contexed_based.py

@@ -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]:
-    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

superset/config.py

@@ -199,7 +199,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"
@@ -401,7 +405,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

superset/dashboards/filters.py

@@ -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_
@@ -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
 
 
@@ -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)
@@ -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(
+            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,
             )
         )
 

superset/security/api.py

@@ -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
     id = fields.String(required=True)
     rls = fields.String()
 

superset/security/guest_token.py

@@ -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
@@ -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):
@@ -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.
         """
         return True
 

superset/security/manager.py

@@ -67,7 +67,8 @@
 from superset.exceptions import SupersetSecurityException
 from superset.security.guest_token import (
     GuestToken,
-    GuestTokenResource,
+    GuestTokenResources,
+    GuestTokenResourceType,
     GuestTokenUser,
     GuestUser,
 )
@@ -1067,11 +1068,16 @@ def raise_for_access(
 
             assert datasource
 
+            should_check_dashboard_access = (
+                feature_flag_manager.is_feature_enabled("DASHBOARD_RBAC")
+                or self.is_guest_user()
+            )
+
             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")
+                    should_check_dashboard_access
                     and self.can_access_based_on_dashboard(datasource)
                 )
             ):
@@ -1097,6 +1103,14 @@ def get_user_by_username(
     def get_anonymous_user(self) -> User:  # pylint: disable=no-self-use
         return AnonymousUserMixin()
 
+    def get_user_roles(self, user: Optional[User] = None) -> List[Role]:
+        if not user:
+            user = g.user
+        if user.is_anonymous:
+            public_role = current_app.config.get("AUTH_ROLE_PUBLIC")
+            return [self.get_public_role()] if public_role else []
+        return user.roles
+
     def get_rls_filters(self, table: "BaseDatasource") -> List[SqlaQuery]:
         """
         Retrieves the appropriate row level security filters for the current user and
@@ -1195,34 +1209,39 @@ 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.
+        This does not check for the required role/permission pairs,
+        it only concerns itself with entity relationships.
 
         :param dashboard: Dashboard the user wants access to
         :raises DashboardAccessDeniedError: If the user cannot access the resource
         """
         # 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 (not is_feature_enabled("DASHBOARD_RBAC")) or any(
+                dashboard_role.id
+                in [user_role.id for user_role in self.get_user_roles()]
                 for dashboard_role in dashboard.roles
             )
 
-        can_access = (
-            is_user_admin()
-            or is_owner(dashboard, g.user)
-            or (dashboard.published and has_rbac_access)
-            or (not dashboard.published and not dashboard.roles)
-        )
+        if self.is_guest_user():
+            can_access = self.has_guest_access(
+                GuestTokenResourceType.DASHBOARD, dashboard.id
+            )
+        else:
+            can_access = (
+                is_user_admin()
+                or is_owner(dashboard, g.user)
+                or (dashboard.published and has_rbac_access())
+                or (not dashboard.published and not dashboard.roles)
+            )
 
         if not can_access:
             raise DashboardAccessDeniedError()
@@ -1255,7 +1274,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"]
@@ -1289,33 +1308,60 @@ def get_guest_user_from_request(self, req: Request) -> Optional[GuestUser]:
 
         try:
             token = self.parse_jwt_guest_token(raw_token)
+            if token.get("user") is None:
+                raise ValueError("Guest token does not contain a user claim")
+            if token.get("resources") is None:
+                raise ValueError("Guest token does not contain a resources claim")
         except Exception:  # pylint: disable=broad-except
             # The login manager will handle sending 401s.
             # We don't need to send a special error message.
             logger.warning("Invalid guest token", exc_info=True)
             return None
         else:
-            return self.guest_user_cls(
-                token=token,
-                roles=[self.find_role(current_app.config["GUEST_ROLE_NAME"])],
-            )
+            return self.get_guest_user_from_token(cast(GuestToken, token))
+
+    def get_guest_user_from_token(self, token: GuestToken) -> GuestUser:
+        return self.guest_user_cls(
+            token=token, roles=[self.find_role(current_app.config["GUEST_ROLE_NAME"])],
+        )
 
     @staticmethod
-    def parse_jwt_guest_token(raw_token: str) -> GuestToken:
+    def parse_jwt_guest_token(raw_token: str) -> Dict[str, Any]:
         """
-        Parses and validates a guest token.
-        Raises an error if the jwt is invalid:
-        if it is not signed with our secret,
-        or if required claims are not present.
+        Parses a guest token. Raises an error if the jwt fails standard claims checks.
         :param raw_token: the token gotten from the request
         :return: the same token that was passed in, tested but unchanged
         """
         secret = current_app.config["GUEST_TOKEN_JWT_SECRET"]
         algo = current_app.config["GUEST_TOKEN_JWT_ALGO"]
+        return jwt.decode(raw_token, secret, algorithms=[algo])
+
+    @staticmethod
+    def is_guest_user(user: Optional[Any] = None) -> bool:
+        # pylint: disable=import-outside-toplevel
+        from superset import is_feature_enabled
+
+        if not is_feature_enabled("EMBEDDED_SUPERSET"):
+            return False
+        if not user:
+            user = g.user
+        return hasattr(user, "is_guest_user") and user.is_guest_user
+
+    def get_current_guest_user_if_guest(self) -> Optional[GuestUser]:
+
+        if self.is_guest_user():
+            return g.user
+        return None
+
+    def has_guest_access(
+        self, resource_type: GuestTokenResourceType, resource_id: Union[str, int]
+    ) -> bool:
+        user = self.get_current_guest_user_if_guest()
+        if not user:
+            return False
 
-        token = jwt.decode(raw_token, secret, algorithms=[algo])
-        if token.get("user") is None:
-            raise ValueError("Guest token does not contain a user claim")
-        if token.get("resources") is None:
-            raise ValueError("Guest token does not contain a resources claim")
-        return cast(GuestToken, token)
+        strid = str(resource_id)
+        for resource in user.resources:
+            if resource["type"] == resource_type.value and str(resource["id"]) == strid:
+                return True
+        return False

superset/views/base.py

@@ -38,7 +38,7 @@
 from flask_appbuilder.actions import action
 from flask_appbuilder.forms import DynamicForm
 from flask_appbuilder.models.sqla.filters import BaseFilter
-from flask_appbuilder.security.sqla.models import Role, User
+from flask_appbuilder.security.sqla.models import User
 from flask_appbuilder.widgets import ListWidget
 from flask_babel import get_locale, gettext as __, lazy_gettext as _
 from flask_jwt_extended.exceptions import NoAuthorizationError
@@ -264,15 +264,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
 
 

superset/views/core.py

@@ -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,
@@ -1888,7 +1887,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(