diff --git a/backend/api/v1/endpoints/calendar.py b/backend/api/v1/endpoints/calendar.py index 82e78dde..4dad31bd 100644 --- a/backend/api/v1/endpoints/calendar.py +++ b/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 ) diff --git a/backend/services/calendar_push_service.py b/backend/services/calendar_push_service.py index 1447264c..13562ee2 100644 --- a/backend/services/calendar_push_service.py +++ b/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)", diff --git a/backend/services/calendar_service/__init__.py b/backend/services/calendar_service/__init__.py index 796ded00..1735096a 100644 --- a/backend/services/calendar_service/__init__.py +++ b/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, diff --git a/backend/services/calendar_service/constants.py b/backend/services/calendar_service/constants.py index aec9324e..6a7922b2 100644 --- a/backend/services/calendar_service/constants.py +++ b/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 diff --git a/backend/services/calendar_service/provider_errors.py b/backend/services/calendar_service/provider_errors.py new file mode 100644 index 00000000..93b50aca --- /dev/null +++ b/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("
", 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) diff --git a/backend/services/calendar_service/sync.py b/backend/services/calendar_service/sync.py index 23672ba5..c3d54480 100644 --- a/backend/services/calendar_service/sync.py +++ b/backend/services/calendar_service/sync.py @@ -12,6 +12,7 @@ import logging +import httpx from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.orm import selectinload from sqlmodel import select @@ -39,7 +40,11 @@ from .constants import PROVIDER_DISPLAY_NAMES from .models_dto import ( IncrementalSyncResetRequired, + ProviderCalendarRecord, ProviderEventSyncResult, + ProviderIdentity, + ProviderRuntimeConfig, + TokenBundle, UnreadableCalendarConnectionState, ) from .oauth import ( @@ -61,6 +66,7 @@ _serialise_connection_with_push, _upsert_connection, ) +from .provider_errors import CalendarConnectFailed, _describe_failure_for_log from .providers import ( _get_microsoft_delta_cursor, _list_google_calendars, @@ -135,6 +141,34 @@ async def get_overview(db: AsyncSession, user: User) -> CalendarOverviewRead: ) +async def _read_connecting_account( + provider: str, + runtime_config: ProviderRuntimeConfig, + code: str, + code_verifier: str, +) -> tuple[TokenBundle, ProviderIdentity, list[ProviderCalendarRecord]]: + """Exchange the code, then read the account and its calendars. + + A failure at any step is raised as ``CalendarConnectFailed``, naming the step + and keeping the provider's own error code and message for the callback to + log and show. Nothing is stored until all three succeed. + """ + google = provider == CalendarProvider.GOOGLE.value + step = "token exchange" + try: + exchange_code = _exchange_google_code if google else _exchange_microsoft_code + token_bundle = await exchange_code(runtime_config, code, code_verifier) + step = "account lookup" + fetch_identity = _fetch_google_identity if google else _fetch_microsoft_identity + identity = await fetch_identity(token_bundle.access_token) + step = "calendar list" + list_calendars = _list_google_calendars if google else _list_microsoft_calendars + provider_calendars = await list_calendars(token_bundle.access_token) + except (httpx.HTTPError, KeyError, ValueError) as exc: + raise CalendarConnectFailed(step, exc) from exc + return token_bundle, identity, provider_calendars + + async def handle_callback( db: AsyncSession, provider: str, user: User, code: str, state: str ) -> CalendarConnection: @@ -157,18 +191,9 @@ async def handle_callback( status_code=400, detail="The calendar connection session is invalid" ) - if provider == CalendarProvider.GOOGLE.value: - token_bundle = await _exchange_google_code( - runtime_config, code, str(state_payload["code_verifier"]) - ) - identity = await _fetch_google_identity(token_bundle.access_token) - provider_calendars = await _list_google_calendars(token_bundle.access_token) - else: - token_bundle = await _exchange_microsoft_code( - runtime_config, code, str(state_payload["code_verifier"]) - ) - identity = await _fetch_microsoft_identity(token_bundle.access_token) - provider_calendars = await _list_microsoft_calendars(token_bundle.access_token) + token_bundle, identity, provider_calendars = await _read_connecting_account( + provider, runtime_config, code, str(state_payload["code_verifier"]) + ) connection = await _upsert_connection( db, @@ -180,11 +205,12 @@ async def handle_callback( await _refresh_connection_calendars(db, connection, provider_calendars) try: await sync_connection_in_session(db, connection.id) - except Exception: # noqa: BLE001 -- boundary: the connection is saved; the periodic sync retries + except Exception as exc: # noqa: BLE001 -- boundary: the connection is saved; the periodic sync retries logger.warning( - "Initial calendar sync failed for connection %s (%s)", + "Initial calendar sync failed for connection %s (%s): %s", connection.id, provider, + _describe_failure_for_log(exc), ) await db.refresh(connection) await _enqueue_push_channel_refresh(connection.id) @@ -540,9 +566,10 @@ async def sync_connection_in_session(db: AsyncSession, connection_id: int) -> No return except Exception as exc: # noqa: BLE001 -- boundary: provider and network errors are recorded on the connection, not raised logger.warning( - "Calendar sync failed for connection %s (%s)", + "Calendar sync failed for connection %s (%s): %s", connection.id, connection.provider, + _describe_failure_for_log(exc), ) failure_status, failure_message = _classify_sync_failure(exc) connection.sync_status = failure_status diff --git a/backend/services/calendar_service/urls.py b/backend/services/calendar_service/urls.py index 7d70b9f3..8260a30f 100644 --- a/backend/services/calendar_service/urls.py +++ b/backend/services/calendar_service/urls.py @@ -7,10 +7,13 @@ from __future__ import annotations +from urllib.parse import urlencode + from backend.models.calendar import CalendarProvider from backend.utils.config_manager import get_trusted_web_origin from .constants import ACCOUNT_REDIRECT_PATHS +from .provider_errors import _usable_reason def _build_redirect_uri(provider: str) -> str: @@ -21,8 +24,19 @@ def _build_push_notification_url(provider: str) -> str: return f"{get_trusted_web_origin()}/api/v1/calendar/webhooks/{provider}" -def _build_account_redirect(status_value: str, provider: str) -> str: +def _build_account_redirect( + status_value: str, provider: str, reason: str | None = None +) -> str: + """The settings page a calendar sign-in returns to, as a relative path. + + ``reason`` is the provider's error code for a failed connection. It is added + only to an ``error`` redirect and only when it looks like a provider code, so + the page never echoes free text from a query string or a response body. + """ provider_redirects = ACCOUNT_REDIRECT_PATHS.get(provider) if provider_redirects is None: provider_redirects = ACCOUNT_REDIRECT_PATHS[CalendarProvider.GOOGLE.value] - return provider_redirects.get(status_value, provider_redirects["error"]) + path = provider_redirects.get(status_value, provider_redirects["error"]) + if path == provider_redirects["error"] and _usable_reason(reason): + path = f"{path}&{urlencode({'reason': reason})}" + return path diff --git a/backend/tests/test_calendar_and_task_api.py b/backend/tests/test_calendar_and_task_api.py index ae122e5d..a72b8493 100644 --- a/backend/tests/test_calendar_and_task_api.py +++ b/backend/tests/test_calendar_and_task_api.py @@ -6,6 +6,7 @@ import pkgutil from datetime import UTC, datetime +import httpx import pytest from fastapi import FastAPI, HTTPException from httpx import ASGITransport, AsyncClient @@ -1229,11 +1230,11 @@ async def test_calendar_dashboard_surfaces_recordings_without_calendar_connectio [ ( HTTPException(status_code=400, detail="Provider is not configured"), - "/settings?tab=account&calendar=config-error&provider=google", + "/settings/integrations?calendar=config-error&provider=google", ), ( RuntimeError("boom"), - "/settings?tab=account&calendar=error&provider=google", + "/settings/integrations?calendar=error&provider=google", ), ], ) @@ -1267,7 +1268,7 @@ async def failing_start_authorisation(*args, **kwargs): ( "google", {"error": "access_denied"}, - "/settings?tab=account&calendar=cancelled&provider=google", + "/settings/integrations?calendar=cancelled&provider=google", ), ( "microsoft", @@ -1275,7 +1276,7 @@ async def failing_start_authorisation(*args, **kwargs): "error": "server_error", "error_description": "AADSTS50194: application is not configured as multi-tenant", }, - "/settings?tab=account&calendar=tenant-config-error&provider=microsoft", + "/settings/integrations?calendar=tenant-config-error&provider=microsoft", ), ], ) @@ -1320,7 +1321,7 @@ async def failing_handle_callback(*args, **kwargs): assert response.status_code == 303 assert ( response.headers["location"] - == "/settings?tab=account&calendar=error&provider=google" + == "/settings/integrations?calendar=error&provider=google" ) @@ -1348,8 +1349,237 @@ async def fake_handle_callback(*args, **kwargs): assert response.status_code == 303 assert ( response.headers["location"] - == "/settings?tab=account&calendar=success&provider=google" + == "/settings/integrations?calendar=success&provider=google" + ) + + +# What a provider hands back during a connection, for the tests below. None of +# these may reach the log: they are the credentials and the account's address. +CONNECT_ACCESS_TOKEN = "provider-access-token-1234" +CONNECT_REFRESH_TOKEN = "provider-refresh-token-5678" +CONNECT_CODE = "authorisation-code-9012" +CONNECT_STATE = "oauth-state-3456" +CONNECT_EMAIL = "alice@example.com" +GOOGLE_CALENDAR_LIST_FORBIDDEN = { + "error": { + "code": 403, + "message": "The caller does not have permission", + "errors": [ + { + "message": "The caller does not have permission", + "domain": "global", + "reason": "forbidden", + } + ], + "status": "PERMISSION_DENIED", + } +} +GRAPH_CALENDAR_LIST_DENIED = { + "error": { + "code": "ErrorAccessDenied", + "message": "Access is denied. Check credentials and try again.", + "innerError": {"request-id": "00000000-0000-0000-0000-000000000004"}, + } +} + + +def _backend_log(caplog: pytest.LogCaptureFixture) -> str: + """What this codebase logged, without httpx's request lines, which carry + the test client's own callback URL with its code and state.""" + return "\n".join( + record.getMessage() + for record in caplog.records + if record.name.startswith("backend") + ) + + +def _answer_provider(calendar_list_refusal: dict) -> httpx.MockTransport: + """Google or Microsoft accepting the code and the account lookup, then + refusing the calendar list with 403.""" + + def answer(request: httpx.Request) -> httpx.Response: + path = request.url.path + if request.method == "POST" and path.endswith("/token"): + return httpx.Response( + 200, + json={ + "access_token": CONNECT_ACCESS_TOKEN, + "refresh_token": CONNECT_REFRESH_TOKEN, + "expires_in": 3599, + "scope": "openid email profile calendar.readonly", + }, + ) + if path == "/v1/userinfo": + return httpx.Response( + 200, + json={"sub": "account-1", "email": CONNECT_EMAIL, "name": "Alice"}, + ) + if path == "/v1.0/me": + return httpx.Response( + 200, + json={"id": "account-1", "mail": CONNECT_EMAIL, "displayName": "Alice"}, + ) + if path in ("/calendar/v3/users/me/calendarList", "/v1.0/me/calendars"): + return httpx.Response(403, json=calendar_list_refusal) + raise AssertionError(f"unexpected provider request {request.method} {path}") + + return httpx.MockTransport(answer) + + +@pytest.fixture +def stub_provider(monkeypatch: pytest.MonkeyPatch): + """Stand in for the provider and the sign-in state a callback needs. + + The service opens its own ``httpx.AsyncClient`` per request, so the class + is swapped for one that defaults to the stub transport; the test client + passes its own transport and is unaffected. The state lookup and the rate + limit would otherwise reach for Redis. + """ + from backend.api.v1.endpoints import calendar as calendar_endpoint + from backend.services.calendar_service import sync as calendar_sync + + real_client = httpx.AsyncClient + + async def _no_rate_limit(*_args, **_kwargs) -> None: + return None + + def _stub(provider: str, calendar_list_refusal: dict) -> None: + transport = _answer_provider(calendar_list_refusal) + + class StubProviderClient(real_client): + def __init__(self, *args, **kwargs) -> None: + kwargs.setdefault("transport", transport) + super().__init__(*args, **kwargs) + + async def _pop_state(state: str) -> dict[str, object] | None: + if state != CONNECT_STATE: + return None + return {"user_id": 1, "provider": provider, "code_verifier": "verifier"} + + monkeypatch.setattr(httpx, "AsyncClient", StubProviderClient) + monkeypatch.setattr(calendar_sync, "_pop_oauth_state", _pop_state) + monkeypatch.setattr(calendar_endpoint, "enforce_rate_limit", _no_rate_limit) + + return _stub + + +@pytest.mark.anyio +@pytest.mark.parametrize( + ("provider", "refusal", "reason", "logged"), + [ + ( + "google", + GOOGLE_CALENDAR_LIST_FORBIDDEN, + "forbidden", + "HTTP 403 forbidden: The caller does not have permission", + ), + ( + "microsoft", + GRAPH_CALENDAR_LIST_DENIED, + "ErrorAccessDenied", + "HTTP 403 ErrorAccessDenied: Access is denied. Check credentials and " + "try again.", + ), + ], +) +async def test_oauth_callback_keeps_the_reason_a_provider_refused_the_calendars( + client: AsyncClient, + override_current_user, + test_session_maker: sessionmaker, + stub_provider, + caplog: pytest.LogCaptureFixture, + provider: str, + refusal: dict, + reason: str, + logged: str, +) -> None: + stub_provider(provider, refusal) + await seed_provider_config(test_session_maker, provider=provider) + override_current_user(1) + caplog.set_level(logging.DEBUG, logger="backend") + + response = await client.get( + f"/api/v1/calendar/oauth/{provider}/callback", + params={"code": CONNECT_CODE, "state": CONNECT_STATE}, + ) + + assert response.status_code == 303 + assert response.headers["location"] == ( + f"/settings/integrations?calendar=error&provider={provider}&reason={reason}" + ) + logged_text = _backend_log(caplog) + assert ( + f"Connecting a {provider} calendar failed at the calendar list step: {logged}" + in logged_text + ) + for secret in ( + CONNECT_ACCESS_TOKEN, + CONNECT_REFRESH_TOKEN, + CONNECT_CODE, + CONNECT_STATE, + CONNECT_EMAIL, + ): + assert secret not in logged_text + assert ( + await fetch_scalar( + test_session_maker, "SELECT COUNT(*) FROM calendar_connections" + ) + == 0 + ) + + +@pytest.mark.anyio +async def test_oauth_callback_logs_and_carries_a_refused_sign_in( + client: AsyncClient, + override_current_user, + caplog: pytest.LogCaptureFixture, +) -> None: + override_current_user(1) + caplog.set_level(logging.DEBUG, logger="backend") + + response = await client.get( + "/api/v1/calendar/oauth/microsoft/callback", + params={ + "error": "unauthorized_client", + "error_description": ( + f"AADSTS700016: Application not found for {CONNECT_EMAIL}." + ), + }, + ) + + assert response.status_code == 303 + assert response.headers["location"] == ( + "/settings/integrations?calendar=error&provider=microsoft" + "&reason=unauthorized_client" + ) + logged_text = _backend_log(caplog) + assert ( + "microsoft calendar sign-in came back with error unauthorized_client " + "(AADSTS700016)" + ) in logged_text + assert CONNECT_EMAIL not in logged_text + + +@pytest.mark.anyio +async def test_oauth_callback_drops_a_reason_that_is_not_a_provider_code( + client: AsyncClient, + override_current_user, + caplog: pytest.LogCaptureFixture, +) -> None: + override_current_user(1) + caplog.set_level(logging.DEBUG, logger="backend") + + response = await client.get( + "/api/v1/calendar/oauth/google/callback", + params={"error": "Call +44 20 7946 0000 to restore access"}, + ) + + assert response.headers["location"] == ( + "/settings/integrations?calendar=error&provider=google" ) + logged_text = _backend_log(caplog) + assert "google calendar sign-in came back with error (unrecognised)" in logged_text + assert "7946" not in logged_text @pytest.mark.anyio diff --git a/backend/tests/test_calendar_provider_errors.py b/backend/tests/test_calendar_provider_errors.py new file mode 100644 index 00000000..54bc3734 --- /dev/null +++ b/backend/tests/test_calendar_provider_errors.py @@ -0,0 +1,99 @@ +"""How a provider's refusal is described for the log and the settings page. + +Connecting an account (#668) and provisioning a push channel (#662) share these +helpers. Their output reaches the API log and the redirect to the settings page, +so it must keep the provider's reason and nothing of the request or account. +""" + +from __future__ import annotations + +import httpx +import pytest +from sqlalchemy.exc import IntegrityError + +from backend.services.calendar_service import ( + CalendarConnectFailed, + _build_account_redirect, + _describe_failure_for_log, + _describe_provider_failure, +) + +TOKEN_URL = "https://login.microsoftonline.com/common/oauth2/v2.0/token" + + +def _refusal(status_code: int, body: dict) -> httpx.HTTPStatusError: + request = httpx.Request("POST", TOKEN_URL, data={"code": "authorisation-code"}) + response = httpx.Response(status_code, json=body, request=request) + return httpx.HTTPStatusError("refused", request=request, response=response) + + +def test_a_token_endpoint_refusal_keeps_its_oauth_code() -> None: + failure = CalendarConnectFailed( + "token exchange", + _refusal( + 401, + { + "error": "invalid_client", + "error_description": "AADSTS7000215: Invalid client secret provided.", + }, + ), + ) + + assert failure.step == "token exchange" + assert failure.reason == "invalid_client" + assert failure.detail == ( + "HTTP 401 invalid_client: AADSTS7000215: Invalid client secret provided." + ) + assert "authorisation-code" not in failure.detail + + +def test_a_provider_message_never_carries_an_address() -> None: + detail = _describe_provider_failure( + _refusal( + 403, + { + "error": { + "code": "ErrorAccessDenied", + "message": "Access to alice.example@example.com is denied.", + } + }, + ) + ) + + assert detail == "HTTP 403 ErrorAccessDenied: Access to is denied." + + +def test_a_reason_that_is_not_a_provider_code_stays_off_the_page() -> None: + failure = CalendarConnectFailed( + "calendar list", + _refusal(403, {"error": {"code": "Call us on 0000", "message": "Denied"}}), + ) + + assert failure.reason is None + assert _build_account_redirect("error", "google", "Call us on 0000") == ( + "/settings/integrations?calendar=error&provider=google" + ) + + +@pytest.mark.parametrize("status_value", ["success", "cancelled", "config-error"]) +def test_only_a_failed_connection_carries_a_reason(status_value: str) -> None: + assert _build_account_redirect(status_value, "microsoft", "forbidden") == ( + f"/settings/integrations?calendar={status_value}&provider=microsoft" + ) + + +def test_a_database_error_is_logged_by_type_alone() -> None: + # The statement's parameters are part of the message, and can hold calendar + # names and event text. + error = IntegrityError( + "INSERT INTO calendar_sources (name) VALUES (?)", ("Alice's calendar",), None + ) + + assert _describe_failure_for_log(error) == "IntegrityError" + + +def test_a_network_error_is_logged_in_full() -> None: + assert ( + _describe_failure_for_log(httpx.ConnectError("All connection attempts failed")) + == "ConnectError: All connection attempts failed" + ) diff --git a/backend/tests/test_calendar_push_service.py b/backend/tests/test_calendar_push_service.py index 978c4f02..eb7cc113 100644 --- a/backend/tests/test_calendar_push_service.py +++ b/backend/tests/test_calendar_push_service.py @@ -16,6 +16,7 @@ from backend.core.encryption import encrypt_secret from backend.models.calendar import CalendarProvider, CalendarPushChannelStatus from backend.services import calendar_push_service as push +from backend.services.calendar_service import PROVIDER_ERROR_DETAIL_LIMIT NOW = datetime(2026, 7, 2, 12, 0, 0) @@ -167,8 +168,10 @@ def _provider_error(status_code: int, body: dict | str) -> httpx.HTTPStatusError } -def test_describe_push_failure_keeps_graph_code_and_message() -> None: - detail = push._describe_push_failure(_provider_error(400, GRAPH_VALIDATION_FAILED)) +def test_describe_provider_failure_keeps_graph_code_and_message() -> None: + detail = push._describe_provider_failure( + _provider_error(400, GRAPH_VALIDATION_FAILED) + ) assert detail == ( "HTTP 400 InvalidRequest: Subscription validation request failed. " @@ -176,8 +179,8 @@ def test_describe_push_failure_keeps_graph_code_and_message() -> None: ) -def test_describe_push_failure_keeps_google_reason_and_message() -> None: - detail = push._describe_push_failure( +def test_describe_provider_failure_keeps_google_reason_and_message() -> None: + detail = push._describe_provider_failure( _provider_error(400, GOOGLE_WEBHOOK_UNVERIFIED) ) @@ -191,29 +194,29 @@ def test_describe_push_failure_keeps_google_reason_and_message() -> None: "body", [GRAPH_VALIDATION_FAILED, GOOGLE_WEBHOOK_UNVERIFIED, "blocked", {}], ) -def test_describe_push_failure_never_carries_the_request(body) -> None: - detail = push._describe_push_failure(_provider_error(400, body)) +def test_describe_provider_failure_never_carries_the_request(body) -> None: + detail = push._describe_provider_failure(_provider_error(400, body)) assert ACCESS_TOKEN not in detail assert GOOGLE_CALENDAR_ADDRESS not in detail assert "alice" not in detail.lower() -def test_describe_push_failure_without_a_json_body_keeps_the_status() -> None: - assert push._describe_push_failure(_provider_error(502, "")) == "HTTP 502" +def test_describe_provider_failure_without_a_json_body_keeps_the_status() -> None: + assert push._describe_provider_failure(_provider_error(502, "")) == "HTTP 502" -def test_describe_push_failure_caps_the_provider_text() -> None: +def test_describe_provider_failure_caps_the_provider_text() -> None: body = {"error": {"code": "InvalidRequest", "message": "x" * 1000}} - detail = push._describe_push_failure(_provider_error(400, body)) + detail = push._describe_provider_failure(_provider_error(400, body)) assert detail.startswith("HTTP 400 InvalidRequest: xxx") - assert len(detail) == len("HTTP 400 ") + push.PUSH_ERROR_DETAIL_LIMIT + assert len(detail) == len("HTTP 400 ") + PROVIDER_ERROR_DETAIL_LIMIT -def test_describe_push_failure_names_a_network_error() -> None: - detail = push._describe_push_failure( +def test_describe_provider_failure_names_a_network_error() -> None: + detail = push._describe_provider_failure( httpx.ConnectError("All connection attempts failed") ) diff --git a/docs/CALENDAR.md b/docs/CALENDAR.md index 219813fb..d4b8ee80 100644 --- a/docs/CALENDAR.md +++ b/docs/CALENDAR.md @@ -75,6 +75,8 @@ Live sync needs no extra Microsoft setup: `Calendars.Read` covers change-notific Each connected account has **Sync now** and **Disconnect** buttons, and shows its status: **Up to date**, **Syncing**, **Needs attention** or **Reconnect required**. +If the connection fails after you approve access, the **Calendars** card says why, with the provider's own reason, and no account is added. [Connecting Fails After Sign-In](#connecting-fails-after-sign-in) lists the common causes. + To check a new setup, connect an account as a normal user, select a calendar, and confirm the dashboard shows its events. ## Dashboard and Linked Recordings @@ -125,6 +127,17 @@ Nojoin holds the **Secret ID** instead of the secret **Value**. Enter the **Valu The tenant blocks user consent for the delegated permissions. A tenant administrator must grant consent. +### Connecting Fails After Sign-In + +The **Calendars** card shows what the provider said, and the API log (`docker logs nojoin-api`) records the step that failed with the HTTP status and the provider's error code and message, for example `Connecting a google calendar failed at the calendar list step: HTTP 403 forbidden: ...`. The log never holds the account's tokens or address. Common reasons: + +- **`notACalendarUser`, or `forbidden` from Google for a Google Workspace account.** Calendar is turned off for that user, or the Workspace administrator restricts third-party apps' access to the Calendar API. In the Google Admin console, turn Calendar on for the user under **Apps > Google Workspace > Calendar**, or mark Nojoin's OAuth app as trusted under **Security > Access and data control > API controls**. +- **`admin_policy_enforced` from Google.** The Workspace administrator blocks the app. Trust it under **API controls** as above. +- **`accessNotConfigured` from Google.** The Google Calendar API is not enabled in the Google Cloud project behind the client ID. Enable it, as in [Google OAuth Setup](#google-oauth-setup). +- **`ErrorAccessDenied` from Microsoft.** The tenant blocks the app or has not consented to its permissions. See [Admin Approval Message in Microsoft](#admin-approval-message-in-microsoft). +- **`MailboxNotEnabledForRESTAPI` from Microsoft.** The account has no Exchange Online mailbox, for example because it is unlicensed or its mailbox is hosted on-premises, so it has no calendar Nojoin can read. +- **`invalid_client`.** The provider refused the app credentials. For Microsoft, see [`invalid_client` During Token Exchange](#invalid_client-during-token-exchange). + ### Google Testing Mode Blocks a User While the Google app is in testing mode, only listed test users can connect. Add the account as a test user or publish the consent screen. diff --git a/frontend/src/components/settings/CalendarConnectionsSettings.test.tsx b/frontend/src/components/settings/CalendarConnectionsSettings.test.tsx index 7ce8e8cc..efe7bfcd 100644 --- a/frontend/src/components/settings/CalendarConnectionsSettings.test.tsx +++ b/frontend/src/components/settings/CalendarConnectionsSettings.test.tsx @@ -8,10 +8,13 @@ vi.mock("./SettingsProvider", () => ({ useSettingsContext: () => context, })); +// The address the card renders at; a sign-in comes back with its outcome in the +// query string. +const navigation = { search: new URLSearchParams(), replace: vi.fn() }; vi.mock("next/navigation", () => ({ - useRouter: () => ({ replace: vi.fn(), push: vi.fn() }), + useRouter: () => ({ replace: navigation.replace, push: vi.fn() }), usePathname: () => "/settings/integrations", - useSearchParams: () => new URLSearchParams(), + useSearchParams: () => navigation.search, })); const unconfigured: CalendarOverview = { @@ -36,6 +39,8 @@ import CalendarConnectionsSettings from "./CalendarConnectionsSettings"; beforeEach(() => { context.isAdmin = false; + navigation.search = new URLSearchParams(); + navigation.replace.mockClear(); }); describe("CalendarConnectionsSettings with no providers set up", () => { @@ -96,3 +101,57 @@ describe("CalendarConnectionsSettings with a connection", () => { expect(sync).toHaveFocus(); }); }); + +describe("CalendarConnectionsSettings after a sign-in fails", () => { + it("says why Google refused the calendars, with the Workspace hint", async () => { + navigation.search = new URLSearchParams( + "calendar=error&provider=google&reason=forbidden", + ); + render({connectFailure.message}
+ {connectFailure.hint &&{connectFailure.hint}
} + {connectFailure.reasonText && ( +{connectFailure.reasonText}
+ )} + +