sadpandajoe commented on code in PR #45508: URL: https://github.com/apache/superset/pull/45508#discussion_r4420122642
########## tests/unit_tests/integrations/teams/destinations_test.py: ########## @@ -0,0 +1,255 @@ +# 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. +"""Browser discovery only projects approved keys and active-installation status.""" + +from collections.abc import Iterator +from datetime import datetime, timezone +from typing import Any +from unittest.mock import MagicMock + +import pytest +from flask import current_app, Flask +from sqlalchemy import event +from sqlalchemy.engine import Engine +from sqlalchemy.orm import Session + +from superset.integrations.teams import destinations +from superset.integrations.teams.store import TeamsInstallationStore + +TENANT = "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa" +APPLICATION = "bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb" +OTHER = "cccccccc-cccc-4ccc-8ccc-cccccccccccc" + + [email protected] +def configured_app( + teams_config: dict[str, Any], teams_engine: Engine, monkeypatch: pytest.MonkeyPatch +) -> Iterator[Flask]: + app = Flask(__name__) + teams_config["destinations"].extend( + [ + { + "key": "finance", + "label": "Finance", + "team_id": "team-1", + "channel_id": "channel-2", + }, + { + "key": "support", + "label": "Support", + "team_id": "team-2", + "channel_id": "channel-3", + }, + ] + ) + app.config["ALERT_REPORTS_TEAMS"] = teams_config + monkeypatch.setattr( + destinations.feature_flag_manager, "is_feature_enabled", lambda name: True + ) + monkeypatch.setattr(destinations, "is_teams_sdk_installed", lambda: True) + engine = teams_engine + monkeypatch.setattr(destinations, "db", MagicMock(engine=engine)) + with app.app_context(): + yield app + + +def test_only_approved_public_fields_and_missing_installations( + configured_app: Flask, +) -> None: + assert destinations.list_teams_destinations() == [ + {"key": "ops", "label": "Operations", "available": False}, + {"key": "finance", "label": "Finance", "available": False}, + {"key": "support", "label": "Support", "available": False}, + ] + + +def test_removal_and_reinstallation_update_availability(configured_app: Flask) -> None: + with Session(destinations.db.engine) as session: + store = TeamsInstallationStore(session) + store.install( + TENANT, + APPLICATION, + "team-1", + "https://smba.trafficmanager.net/amer/";, + datetime.now(timezone.utc), + ) + store.install( + TENANT, + APPLICATION, + "unapproved-team", + "https://smba.trafficmanager.net/amer/";, + datetime.now(timezone.utc), + ) + session.commit() + assert [ + item["available"] for item in destinations.list_teams_destinations() + ] == [True, True, False] + store.remove( + TENANT, + APPLICATION, + "team-1", + "https://smba.trafficmanager.net/amer/";, + datetime.now(timezone.utc), + ) + session.commit() + assert not any( + item["available"] for item in destinations.list_teams_destinations() + ) + store.install( + TENANT, + APPLICATION, + "team-1", + "https://smba.trafficmanager.net/emea/";, + datetime.now(timezone.utc), + ) + session.commit() + assert [ + item["available"] for item in destinations.list_teams_destinations() + ] == [True, True, False] + assert len(destinations.list_teams_destinations()) == 3 + + [email protected]("tenant,application", [(OTHER, APPLICATION), (TENANT, OTHER)]) +def test_other_identity_cannot_make_destination_available( + configured_app: Flask, tenant: str, application: str +) -> None: + with Session(destinations.db.engine) as session: + TeamsInstallationStore(session).install( + tenant, + application, + "team-1", + "https://smba.trafficmanager.net/amer/";, + datetime.now(timezone.utc), + ) + session.commit() + assert not any(item["available"] for item in destinations.list_teams_destinations()) + + [email protected]("configuration", [{}, {"credential": "invalid-secret"}]) +def test_unconfigured_or_invalid_integration_is_hidden( + configured_app: Flask, configuration: dict[str, Any] +) -> None: + current_app.config["ALERT_REPORTS_TEAMS"] = configuration + assert destinations.configured_teams() is None + assert destinations.list_teams_destinations() == [] + + +def test_disabled_integration_does_not_query_installations( + configured_app: Flask, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr( + destinations.feature_flag_manager, "is_feature_enabled", lambda name: False + ) + monkeypatch.setattr( + destinations, "Session", MagicMock(side_effect=AssertionError("must not query")) + ) + assert destinations.list_teams_destinations() == [] + + [email protected]("team_id", ["t" * 256, " team-1", "team-1 ", "\tteam-1\n"]) +def test_invalid_team_config_is_hidden_before_installation_lookup( + configured_app: Flask, + monkeypatch: pytest.MonkeyPatch, + team_id: str, +) -> None: + """Discovery handles invalid deployment IDs instead of raising store ValueError.""" + configured_app.config["ALERT_REPORTS_TEAMS"]["destinations"][0]["team_id"] = team_id + lookup = MagicMock(side_effect=AssertionError("must not query installations")) + monkeypatch.setattr(destinations, "Session", lookup) + assert destinations.configured_teams() is None + assert destinations.list_teams_destinations() == [] + lookup.assert_not_called() + + [email protected]("installed", [False, True]) +def test_optional_sdk_controls_both_discovery_and_notification_methods( + configured_app: Flask, + monkeypatch: pytest.MonkeyPatch, + installed: bool, +) -> None: + """Bootstrap and destination discovery agree when the optional SDK is missing.""" + from superset.reports.models import ReportRecipientType + from superset.views import base + + monkeypatch.setattr(destinations, "is_teams_sdk_installed", lambda: installed) + monkeypatch.setattr(base, "app", configured_app) + configured_app.config["SLACK_API_TOKEN"] = None + if installed: + assert destinations.configured_teams() is not None + assert len(destinations.list_teams_destinations()) == 3 + assert ReportRecipientType.TEAMS in base._notification_methods() + else: + session = MagicMock(side_effect=AssertionError("must not query installations")) + monkeypatch.setattr(destinations, "Session", session) + assert destinations.configured_teams() is None + assert destinations.list_teams_destinations() == [] + assert ReportRecipientType.TEAMS not in base._notification_methods() + session.assert_not_called() + + +def test_disabled_integration_registers_and_bootstraps_without_sdk( + configured_app: Flask, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The Teams startup paths require neither SDK imports nor a dependency lookup.""" + import sys + + from superset.integrations.teams.api import register_teams_api + from superset.reports.models import ReportRecipientType + from superset.views import base + + monkeypatch.setattr( + destinations.feature_flag_manager, "is_feature_enabled", lambda _: False + ) + missing_sdk = MagicMock(side_effect=AssertionError("must not check SDK")) + monkeypatch.setattr(destinations, "is_teams_sdk_installed", missing_sdk) + monkeypatch.setitem(sys.modules, "superset.integrations.teams.adapter", None) Review Comment: `register_teams_api` and `base` are imported at the top of this test, before `sys.modules` is blocked here. If a module-level `import microsoft_teams` were added to `api.py` or `destinations.py`, those modules would already be loaded with the SDK present, so this test would still pass in CI while a deployment without the optional extra fails at startup. Could this run the disabled-Teams startup in a fresh interpreter (or import the modules after the block) and assert the callback still returns 404? ########## superset/integrations/teams/validation.py: ########## @@ -0,0 +1,95 @@ +# 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. +"""Shared Teams recipient rules for schedule saves and notification delivery.""" + +from flask import current_app + +from superset.extensions import feature_flag_manager +from superset.integrations.teams.config import ( + parse_teams_config, + TeamsConfig, + TeamsDestination, +) +from superset.reports.models import ReportDataFormat, ReportScheduleType +from superset.utils import json + + +class TeamsRecipientValidationError(ValueError): + """A field-specific error that never includes credentials or supplied values.""" + + def __init__(self, message: str, field: str = "recipients") -> None: + """Identify the schedule field that must be corrected.""" + super().__init__(message) + self.field = field + + +def validate_teams_recipient( + recipient_config: object, + schedule_type: str | None, + report_format: str | None, +) -> tuple[TeamsConfig, TeamsDestination]: + """Require an enabled, configured, attachment-free alert and an approved key. + + Accept deserialized command input or persisted recipient JSON. Installation + availability is deliberately checked only at delivery, so schedules may be + saved before a bot is installed. No SDK dependency or database access is needed. + """ + if not feature_flag_manager.is_feature_enabled("ALERT_REPORT_TEAMS"): + raise TeamsRecipientValidationError("Teams alert delivery is disabled") + try: + config = parse_teams_config(current_app.config.get("ALERT_REPORTS_TEAMS", {})) + except ValueError: + raise TeamsRecipientValidationError( + "Invalid Teams deployment configuration" + ) from None + if config is None: + raise TeamsRecipientValidationError("Teams deployment is not configured") + if schedule_type != ReportScheduleType.ALERT: + raise TeamsRecipientValidationError( + "Teams recipients require an alert schedule", "type" + ) + if report_format != ReportDataFormat.NONE: + raise TeamsRecipientValidationError( + "Teams recipients require disabled attachments (NONE format)", + "report_format", + ) + try: + recipient = ( + json.loads(recipient_config) + if isinstance(recipient_config, str) + else recipient_config + ) + except (ValueError, TypeError): + raise TeamsRecipientValidationError( + "Invalid Teams recipient configuration" + ) from None + if ( + not isinstance(recipient, dict) + or set(recipient) != {"target"} + or not isinstance(recipient["target"], str) + or not recipient["target"] + ): + raise TeamsRecipientValidationError( + "A configured Teams destination key is required" + ) + try: + destination = config.get_destination(recipient["target"]) + except ValueError: + raise TeamsRecipientValidationError( Review Comment: These Teams validation messages (here, the "A configured Teams destination key is required" and "Invalid Teams recipient configuration" ones above, and the duplicate-key message in `commands/report/base.py`) are plain English and flow straight into the 422 response and the editor's error toast. A non-English user saving an alert whose Teams destination was removed will see "Teams destination is not approved" untranslated, while the neighbouring report validators wrap theirs in `_()`. Should these go through gettext (and into `messages.pot`) like the rest of the report validation errors? ########## superset/integrations/teams/adapter.py: ########## @@ -0,0 +1,556 @@ +# 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. +"""Microsoft Teams SDK boundary for Flask callbacks and root channel delivery.""" + +from __future__ import annotations + +import asyncio +import math +from collections.abc import AsyncIterator, Callable +from contextlib import AbstractAsyncContextManager, asynccontextmanager +from dataclasses import dataclass +from datetime import datetime, timezone +from email.utils import parsedate_to_datetime +from time import monotonic +from typing import Any, Literal, Protocol +from uuid import UUID + +import httpx +import requests +from microsoft_teams.api import ( + ApiClient, + Attachment, + ClientCredentials, + InstalledActivity, + InvokeResponse, + JsonWebToken, + MessageActivityInput, + UninstalledActivity, +) +from microsoft_teams.api.auth.cloud_environment import PUBLIC +from microsoft_teams.api.clients.conversation import CreateConversationParams +from microsoft_teams.apps import HttpServer, HttpServerAdapter +from microsoft_teams.apps.events import ActivityEvent +from microsoft_teams.apps.http import HttpRequest, HttpResponse, HttpRouteHandler +from microsoft_teams.common import Client, ClientOptions +from msal import ConfidentialClientApplication +from pydantic import ValidationError + +from superset.integrations.teams.config import TeamsConfig, TeamsDestination + +EVENTS_PATH = "/api/v1/teams/events/" + + +@dataclass(frozen=True) +class TeamsInstallationEvent: + """Authenticated, deployment-scoped installation data, with no SDK objects.""" + + team_id: str + service_url: str + active: bool + event_at: datetime + + +@dataclass(frozen=True) +class TeamsEventResult: + """Callback outcome; only an authenticated event may reach persistence.""" + + status: int + event: TeamsInstallationEvent | None = None + + +class _FlaskServerAdapter(HttpServerAdapter): + """Capture the SDK route for one invocation of Flask's existing POST route. + + Flask registers its route once at startup. Each request creates a fresh SDK + authentication server; its handler runs to completion in an operation-scoped + event loop. No SDK App, outbound clients, tasks, or separate server are created. + """ + + def __init__(self) -> None: + """Hold only the handler for this request.""" + self.handler: HttpRouteHandler | None = None + + def register_route( + self, method: Literal["POST"], path: str, handler: HttpRouteHandler + ) -> None: + """Accept only the independently authenticated Teams callback.""" + if method != "POST" or path != EVENTS_PATH or self.handler is not None: + raise ValueError("Unexpected Teams SDK route registration") + self.handler = handler + + async def start(self, port: int) -> None: + """Prevent the SDK from starting its own HTTP server.""" + raise RuntimeError("Flask manages the Teams callback server") + + async def stop(self) -> None: + """Release the operation's handler; Flask owns the HTTP server.""" + self.handler = None + + +def _same_uuid(value: object, expected: str) -> bool: + """Compare concrete UUIDs without accepting tenant aliases.""" + try: + return isinstance(value, str) and UUID(value) == UUID(expected) + except ValueError: + return False + + +def _allowed_token(config: TeamsConfig, event: ActivityEvent) -> bool: + """Restrict the SDK's broader auth support to expiring bot service tokens.""" + # Only Bot Framework service tokens bind serviceurl to a signed claim. + # Agent 365/Entra activities and bot-to-bot callers are outside this integration. + token = event.token + return ( + isinstance(token, JsonWebToken) + and token.issuer == PUBLIC.token_issuer + and token.expiration is not None + and (not token.tenant_id or _same_uuid(token.tenant_id, config.tenant_id)) + ) + + +def _installation_event( + config: TeamsConfig, event: ActivityEvent, body: dict[str, Any] +) -> TeamsEventResult: + """Apply deployment restrictions only after the SDK has authenticated the token.""" + if not _allowed_token(config, event): + return TeamsEventResult(403) + token = event.token + actions = { + "add": "add", + "add-upgrade": "add", + "remove": "remove", + "remove-upgrade": "remove", + } + action = body.get("action") + if ( + body.get("type") != "installationUpdate" + or not isinstance(action, str) + or action not in actions + ): + return TeamsEventResult(400) + action = actions[action] + # Require the wire-format fields used by SDK authentication. Do not allow + # model defaults or snake_case aliases to bypass missing channel/service data. + if body.get("channelId") != "msteams" or not isinstance( + body.get("serviceUrl"), str + ): + return TeamsEventResult(400) + service_url = body["serviceUrl"] + if not service_url or token.service_url.rstrip("/") != service_url.rstrip("/"): + return TeamsEventResult(403) + try: + model = InstalledActivity if action == "add" else UninstalledActivity + activity = model.model_validate({**body, "action": action}) + except ValidationError: + return TeamsEventResult(400) + if activity.recipient.id not in { + config.application_id, + f"28:{config.application_id}", + }: + return TeamsEventResult(403) + data = activity.channel_data + if ( + data is None + or data.tenant is None + or not _same_uuid(data.tenant.id, config.tenant_id) + or activity.conversation.conversation_type != "channel" + or ( + activity.conversation.tenant_id is not None + and not _same_uuid(activity.conversation.tenant_id, config.tenant_id) + ) + or data.team is None + or not data.team.id + or data.team.team_type not in (None, "standard") + or (data.channel is not None and data.channel.type not in (None, "standard")) + ): + return TeamsEventResult(403) + approved_teams = {destination.team_id for destination in config.destinations} + if data.team.id not in approved_teams: + return TeamsEventResult(403) + event_at = activity.timestamp + if ( + body.get("attachments") + or event_at is None + or event_at.tzinfo is None + or event_at.utcoffset() is None + ): + return TeamsEventResult(400) + return TeamsEventResult( + 200, + TeamsInstallationEvent(data.team.id, service_url, action == "add", event_at), + ) + + +def process_teams_event( + config: TeamsConfig, body: dict[str, Any], headers: dict[str, str] +) -> TeamsEventResult: + """Authenticate through the SDK's server adapter without starting a server. + + The SDK's HttpServer uses PyJWKClient, whose network responses close within + its scoped fetch. It creates no persistent HTTP clients. Keep construction, + authentication, and adapter cleanup within the same event loop; asyncio.run + closes that loop on success and failure. SDK response bodies are never exposed. + """ + + async def process() -> TeamsEventResult: + """Run one SDK invocation, returning only sanitized application data.""" + adapter = _FlaskServerAdapter() + result = TeamsEventResult(500) + + async def on_request(event: ActivityEvent) -> InvokeResponse[None]: + """Receive activities only after SDK authentication succeeds.""" + nonlocal result + result = _installation_event(config, event, body) + return InvokeResponse(status=result.status) + + try: + server = HttpServer(adapter, messaging_endpoint=EVENTS_PATH) Review Comment: A new `HttpServer` is built for every callback, so (assuming the SDK's validator builds its JWKS client per instance) none of the signing-key cache or refresh cooldown carries over between requests. An unauthenticated caller who posts a well-formed JWT with any `kid` then triggers a blocking fetch of the Bot Framework key set inside the Flask worker on every request, and if the egress IP gets throttled, legitimate installation events fail key lookup with a 401 and are never recorded (Teams does not redeliver). The tests monkeypatch the signing-key lookup, so this isn't exercised. Does the SDK cache keys across `HttpServer` instances, or should the server/validator be kept per process? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
