fix(calendar): log and show why connecting a calendar failed

backend/api/v1/endpoints/calendar.py

@@ -1,5 +1,6 @@
 import json
 import logging
+import re
 
 from fastapi import APIRouter, Depends, HTTPException, Query, Request, Response
 from fastapi.responses import PlainTextResponse, RedirectResponse
@@ -24,7 +25,10 @@
     handle_microsoft_notification,
 )
 from backend.services.calendar_service import (
+    PROVIDER_REASON_PATTERN,
+    CalendarConnectFailed,
     _build_account_redirect,
+    _describe_failure_for_log,
     disconnect_connection,
     get_dashboard_summary,
     get_overview,
@@ -46,6 +50,8 @@
 # notification batches are small, so anything larger is rejected before parsing
 # to stop an attacker exhausting worker memory with a huge JSON body.
 MICROSOFT_WEBHOOK_MAX_BODY_BYTES = 1024 * 1024
+# An Entra sign-in error number, such as AADSTS50194, inside an error description.
+AADSTS_CODE_PATTERN = re.compile(r"AADSTS\d{1,8}")
 
 
 def _validate_provider(provider: str) -> str:
@@ -69,6 +75,27 @@ def _map_callback_status(
     return "error"
 
 
+def _log_refused_sign_in(
+    provider: str, error: str | None, error_description: str | None
+) -> None:
+    """Log why the provider sent the user back without a code.
+
+    Keeps the OAuth error code and any Entra ``AADSTS`` number, never the
+    description itself, which can name the account. Both come from the query
+    string, so only values shaped like a provider code are written.
+    """
+    if not error:
+        logger.warning("%s calendar sign-in came back without a code", provider)
+        return
+    aadsts = AADSTS_CODE_PATTERN.search(error_description or "")
+    logger.warning(
+        "%s calendar sign-in came back with error %s%s",
+        provider,
+        error if PROVIDER_REASON_PATTERN.fullmatch(error) else "(unrecognised)",
+        f" ({aadsts.group(0)})" if aadsts else "",
+    )
+
+
 @router.get("", response_model=CalendarOverviewRead)
 @router.get("/", response_model=CalendarOverviewRead)
 async def get_calendar_overview(
@@ -132,7 +159,12 @@ async def start_calendar_authorisation_redirect(
         return RedirectResponse(
             _build_account_redirect(status_value, provider), status_code=303
         )
-    except Exception:  # noqa: BLE001 -- boundary: OAuth start failures are shown on the account page, not as a 500
+    except Exception as exc:  # noqa: BLE001 -- boundary: OAuth start failures are logged and shown on the settings page, not as a 500
+        logger.warning(
+            "Starting a %s calendar sign-in failed: %s",
+            provider,
+            _describe_failure_for_log(exc),
+        )
         return RedirectResponse(
             _build_account_redirect("error", provider), status_code=303
         )
@@ -163,13 +195,37 @@ async def calendar_authorisation_callback(
 
     if error or not code or not state:
         status_value = _map_callback_status(provider, error, error_description)
+        if status_value != "cancelled":
+            _log_refused_sign_in(provider, error, error_description)
         return RedirectResponse(
-            _build_account_redirect(status_value, provider), status_code=303
+            _build_account_redirect(status_value, provider, error), status_code=303
         )
 
     try:
         await handle_callback(db, provider, current_user, code, state)
-    except Exception:  # noqa: BLE001 -- boundary: callback failures are shown on the account page, not as a 500
+    except CalendarConnectFailed as exc:
+        logger.warning(
+            "Connecting a %s calendar failed at the %s step: %s",
+            provider,
+            exc.step,
+            exc.detail,
+        )
+        return RedirectResponse(
+            _build_account_redirect("error", provider, exc.reason), status_code=303
+        )
+    except HTTPException as exc:
+        # The integration is not configured, or the sign-in session expired or
+        # belongs to someone else: fixed messages with nothing of the account.
+        logger.warning("Connecting a %s calendar failed: %s", provider, exc.detail)
+        return RedirectResponse(
+            _build_account_redirect("error", provider), status_code=303
+        )
+    except Exception as exc:  # noqa: BLE001 -- boundary: callback failures are logged and shown on the settings page, not as a 500
+        logger.warning(
+            "Connecting a %s calendar failed: %s",
+            provider,
+            _describe_failure_for_log(exc),
+        )
         return RedirectResponse(
             _build_account_redirect("error", provider), status_code=303
         )

backend/services/calendar_push_service.py

@@ -44,8 +44,10 @@
 from backend.services.calendar_service import (
     MICROSOFT_GRAPH_URL,
     _build_push_notification_url,
+    _describe_provider_failure,
     _get_access_token_for_connection,
     _parse_iso_datetime,
+    _truncate_error,
     get_provider_runtime_config,
 )
 from backend.utils.time import utc_now
@@ -68,9 +70,7 @@
 # blocked, domain unverified, permission missing) and needs a change here first.
 PUSH_FAILURE_BACKOFF = timedelta(minutes=30)
 PUSH_REJECTED_BACKOFF = timedelta(hours=6)
-# Cap on the provider's error code and message kept in logs and ``last_error``.
-PUSH_ERROR_DETAIL_LIMIT = 200
-# ``_describe_push_failure`` starts ``last_error`` with the HTTP status, and
+# ``_describe_provider_failure`` starts ``last_error`` with the HTTP status, and
 # ``_push_retry_backoff`` reads it back from there.
 RECORDED_HTTP_STATUS = re.compile(r"^HTTP (\d{3})\b")
 HTTP_CLIENT_ERRORS = range(400, 500)
@@ -120,69 +120,6 @@ def _format_graph_datetime(value: datetime) -> str:
     return value.replace(microsecond=0).isoformat() + "Z"
 
 
-def _truncate_error(value: str | None, limit: int = 500) -> str | None:
-    if not value:
-        return None
-    cleaned = " ".join(str(value).split())
-    return cleaned[:limit] if cleaned else None
-
-
-def _google_error_reason(error: dict[str, Any]) -> str | None:
-    """Google's specific reason (``pushWebhookBadDomain``), else its status."""
-    for item in error.get("errors") or []:
-        if isinstance(item, dict) and isinstance(item.get("reason"), str):
-            return item["reason"]
-    status = error.get("status")
-    return status if isinstance(status, str) else None
-
-
-def _provider_error_fields(payload: Any) -> tuple[str | None, str | None]:
-    """Pick the error code and message out of a provider's JSON error body.
-
-    Graph sends ``{"error": {"code": "InvalidRequest", "message": ...}}``, Google
-    ``{"error": {"code": 400, "message": ..., "errors": [{"reason": ...}]}}``,
-    and a token endpoint ``{"error": "invalid_grant", "error_description": ...}``.
-    """
-    if not isinstance(payload, dict):
-        return None, None
-    error = payload.get("error")
-    if isinstance(error, str):
-        description = payload.get("error_description")
-        return error, description if isinstance(description, str) else None
-    if not isinstance(error, dict):
-        return None, None
-    code = error.get("code")
-    if not isinstance(code, str):
-        code = _google_error_reason(error)
-    message = error.get("message")
-    return code, message if isinstance(message, str) else None
-
-
-def _describe_push_failure(exc: Exception) -> str:
-    """Summarise a failed provisioning call for the log and ``last_error``.
-
-    Keeps the HTTP status and the provider's own error code and message, which
-    say why a subscription was refused where httpx's status line does not. It
-    reads only the error fields of the response body, never the request, so no
-    token or calendar address reaches it, and it caps the provider's text.
-    """
-    if isinstance(exc, httpx.HTTPStatusError):
-        try:
-            payload = exc.response.json()
-        except ValueError:
-            payload = None
-        code, message = _provider_error_fields(payload)
-        detail = _truncate_error(
-            ": ".join(part for part in (code, message) if part),
-            PUSH_ERROR_DETAIL_LIMIT,
-        )
-        status_line = f"HTTP {exc.response.status_code}"
-        return f"{status_line} {detail}" if detail else status_line
-    detail = _truncate_error(str(exc), PUSH_ERROR_DETAIL_LIMIT)
-    name = type(exc).__name__
-    return f"{name}: {detail}" if detail else name
-
-
 def _push_retry_backoff(last_error: str | None) -> timedelta:
     """How long a failed channel waits before its next provisioning attempt.
 
@@ -626,7 +563,7 @@ async def _provision_selected_calendars(
         try:
             await _ensure_channel(ctx, calendar, channel)
         except Exception as exc:  # noqa: BLE001 -- boundary: one calendar's channel failing must not stop the others
-            detail = _describe_push_failure(exc)
+            detail = _describe_provider_failure(exc)
             logger.warning(
                 "Failed to provision %s push channel for calendar %s: %s "
                 "(next attempt in %d minutes)",

backend/services/calendar_service/__init__.py

@@ -93,6 +93,18 @@
     _serialise_source,
     _upsert_connection,
 )
+from .provider_errors import (
+    PROVIDER_ERROR_DETAIL_LIMIT,
+    PROVIDER_REASON_PATTERN,
+    CalendarConnectFailed,
+    _describe_failure_for_log,
+    _describe_provider_failure,
+    _google_error_reason,
+    _provider_error_fields,
+    _provider_error_reason,
+    _truncate_error,
+    _usable_reason,
+)
 from .providers import (
     _build_google_events_query_params,
     _build_microsoft_calendar_view_url,

backend/services/calendar_service/constants.py

@@ -63,9 +63,12 @@
     "cancelled",
     "tenant-config-error",
 )
+# The callback lands on the page that holds the Calendars card. The settings
+# index turns a legacy ``?tab=`` link into a category route and drops every other
+# query parameter, so pointing at ``/settings?tab=...`` loses the result.
 ACCOUNT_REDIRECT_PATHS = {
     provider: {
-        status_value: f"/settings?{urlencode({'tab': 'account', 'calendar': status_value, 'provider': provider})}"
+        status_value: f"/settings/integrations?{urlencode({'calendar': status_value, 'provider': provider})}"
         for status_value in ACCOUNT_REDIRECT_STATUSES
     }
     for provider in PROVIDER_DISPLAY_NAMES

backend/services/calendar_service/provider_errors.py

@@ -0,0 +1,140 @@
+"""Reading a provider's error response for the log and the user.
+
+Shared by connecting an account (the OAuth callback) and by push-channel
+provisioning. Each helper reads only the error fields of a response body, never
+the request, so no token, authorisation code, state value or calendar address
+reaches the output, and any email address a provider echoes back is masked.
+
+It imports only the ``models_dto`` leaf from the calendar package.
+"""
+
+from __future__ import annotations
+
+import re
+from typing import Any
+
+import httpx
+
+from .models_dto import CalendarReauthorisationRequired
+
+# Cap on the provider's error code and message kept in logs and ``last_error``.
+PROVIDER_ERROR_DETAIL_LIMIT = 200
+# A provider error code as it may travel in the settings redirect: Google's
+# ``notACalendarUser``, Graph's ``MailboxNotEnabledForRESTAPI``, OAuth's
+# ``invalid_client``. Anything else is dropped rather than echoed to the page.
+PROVIDER_REASON_PATTERN = re.compile(r"[A-Za-z][A-Za-z0-9_.-]{0,63}")
+EMAIL_ADDRESS_PATTERN = re.compile(r"[\w.+-]+@[\w-]+(?:\.[\w-]+)+")
+
+
+class CalendarConnectFailed(Exception):
+    """A provider refused or failed a step of connecting an account.
+
+    ``step`` names where it happened, ``reason`` is the provider's own error code
+    when it gave a usable one, and ``detail`` is the described failure the
+    callback logs.
+    """
+
+    def __init__(self, step: str, exc: Exception) -> None:
+        self.step = step
+        self.reason = _provider_error_reason(exc)
+        self.detail = _describe_provider_failure(exc)
+        super().__init__(f"{step}: {self.detail}")
+
+
+def _truncate_error(value: str | None, limit: int = 500) -> str | None:
+    if not value:
+        return None
+    cleaned = " ".join(str(value).split())
+    return cleaned[:limit] if cleaned else None
+
+
+def _google_error_reason(error: dict[str, Any]) -> str | None:
+    """Google's specific reason (``pushWebhookBadDomain``), else its status."""
+    for item in error.get("errors") or []:
+        if isinstance(item, dict) and isinstance(item.get("reason"), str):
+            return item["reason"]
+    status = error.get("status")
+    return status if isinstance(status, str) else None
+
+
+def _provider_error_fields(payload: Any) -> tuple[str | None, str | None]:
+    """Pick the error code and message out of a provider's JSON error body.
+
+    Graph sends ``{"error": {"code": "InvalidRequest", "message": ...}}``, Google
+    ``{"error": {"code": 400, "message": ..., "errors": [{"reason": ...}]}}``,
+    and a token endpoint ``{"error": "invalid_grant", "error_description": ...}``.
+    Google's numeric ``code`` repeats the HTTP status, so its reason stands in.
+    """
+    if not isinstance(payload, dict):
+        return None, None
+    error = payload.get("error")
+    if isinstance(error, str):
+        description = payload.get("error_description")
+        return error, description if isinstance(description, str) else None
+    if not isinstance(error, dict):
+        return None, None
+    code = error.get("code")
+    if not isinstance(code, str):
+        code = _google_error_reason(error)
+    message = error.get("message")
+    return code, message if isinstance(message, str) else None
+
+
+def _response_error_fields(
+    exc: httpx.HTTPStatusError,
+) -> tuple[str | None, str | None]:
+    try:
+        payload = exc.response.json()
+    except ValueError:
+        payload = None
+    return _provider_error_fields(payload)
+
+
+def _mask_addresses(value: str) -> str:
+    return EMAIL_ADDRESS_PATTERN.sub("<address>", value)
+
+
+def _describe_provider_failure(exc: Exception) -> str:
+    """Summarise a failed provider call for the log and ``last_error``.
+
+    Keeps the HTTP status and the provider's own error code and message, which
+    say why a request was refused where httpx's status line does not, and caps
+    the provider's text. Any other exception keeps its type and message.
+    """
+    if isinstance(exc, httpx.HTTPStatusError):
+        code, message = _response_error_fields(exc)
+        detail = _truncate_error(
+            _mask_addresses(": ".join(part for part in (code, message) if part)),
+            PROVIDER_ERROR_DETAIL_LIMIT,
+        )
+        status_line = f"HTTP {exc.response.status_code}"
+        return f"{status_line} {detail}" if detail else status_line
+    detail = _truncate_error(_mask_addresses(str(exc)), PROVIDER_ERROR_DETAIL_LIMIT)
+    name = type(exc).__name__
+    return f"{name}: {detail}" if detail else name
+
+
+def _describe_failure_for_log(exc: Exception) -> str:
+    """Describe a failure at a boundary that catches everything.
+
+    A provider's answer, a network error and a refused grant are described in
+    full. Anything else keeps only its type: a database error's message carries
+    the statement's parameters, which can hold calendar names and event text.
+    """
+    if isinstance(exc, (httpx.HTTPError, CalendarReauthorisationRequired)):
+        return _describe_provider_failure(exc)
+    return type(exc).__name__
+
+
+def _usable_reason(value: str | None) -> str | None:
+    if value and PROVIDER_REASON_PATTERN.fullmatch(value):
+        return value
+    return None
+
+
+def _provider_error_reason(exc: Exception) -> str | None:
+    """The provider's error code, when it is safe to carry to the settings page."""
+    if not isinstance(exc, httpx.HTTPStatusError):
+        return None
+    code, _message = _response_error_fields(exc)
+    return _usable_reason(code)