fix: redirect to frontend with error params on oauth callback failure (#1254)

Author: andre-code

components/renku_data_services/connected_services/apispec_base.py

@@ -36,3 +36,8 @@ class CallbackParams(BaseAPISpec):
     model_config = ConfigDict(extra="ignore")
 
     state: str = Field(default="")
+    error: str | None = Field(default=None)
+    error_description: str | None = Field(default=None)
+    error_uri: str | None = Field(default=None)
+    code: str | None = Field(default=None)
+    iss: str | None = Field(default=None)

components/renku_data_services/connected_services/blueprints.py

@@ -2,7 +2,7 @@
 
 from dataclasses import dataclass
 from typing import Any
-from urllib.parse import unquote, urlparse, urlunparse
+from urllib.parse import parse_qsl, unquote, urlencode, urlparse, urlunparse
 
 from sanic import HTTPResponse, Request, empty, json, redirect
 from sanic.response import JSONResponse
@@ -133,6 +133,16 @@ async def _callback(request: Request) -> HTTPResponse:
 
             callback_url = self._get_callback_url(request)
 
+            if params.error:
+                next_url = await self.connected_services_repo.get_oauth2_connection_next_url_by_state(params.state)
+                if next_url:
+                    return redirect(to=self._append_query_params(next_url, params))
+                logger.info(
+                    "OAuth callback returned an error but no pending connection next_url was found "
+                    f"for state={params.state!r}"
+                )
+                raise errors.ForbiddenError(message="You do not have the required permissions for this operation.")
+
             client = await self.oauth_http_client_factory.fetch_token(
                 state=params.state, raw_url=request.url, callback_url=callback_url
             )
@@ -156,6 +166,17 @@ def _get_callback_url(self, request: Request) -> str:
             logger.warning("Forcing the callback URL to use https. Trusted proxies configuration may be incorrect.")
         return https_callback_url
 
+    @staticmethod
+    def _append_query_params(url: str, params: CallbackParams) -> str:
+        allowed_keys = {"error", "error_description", "state", "error_uri", "code", "iss"}
+        allowed_params = [(k, v) for k, v in params.model_dump(include=allowed_keys, exclude_none=True).items() if v]
+        if not allowed_params:
+            return url
+        parsed = urlparse(url)
+        existing_params = parse_qsl(parsed.query, keep_blank_values=True)
+        merged_query = urlencode([*existing_params, *allowed_params])
+        return urlunparse(parsed._replace(query=merged_query))
+
 
 @dataclass(kw_only=True)
 class OAuth2ConnectionsBP(CustomBlueprint):

components/renku_data_services/connected_services/db.py

@@ -241,6 +241,19 @@ async def get_oauth2_connection(self, connection_id: ULID, user: base_models.API
 
         return connection
 
+    async def get_oauth2_connection_next_url_by_state(self, state: str) -> str | None:
+        """Get the saved next_url for a pending OAuth2 connection using state."""
+        if not state:
+            return None
+        async with self.session_maker() as session:
+            result = await session.scalars(
+                select(schemas.OAuth2ConnectionORM)
+                .where(schemas.OAuth2ConnectionORM.state == state)
+                .where(schemas.OAuth2ConnectionORM.status == models.ConnectionStatus.pending)
+            )
+            connection = result.one_or_none()
+            return connection.next_url if connection else None
+
     async def get_provider_for_image(self, user: APIUser, image: Image) -> models.ImageProvider | None:
         """Find a provider supporting the given image."""
         registry_urls = [f"http://{image.hostname}", f"https://{image.hostname}"]

test/bases/renku_data_services/data_api/test_connected_services.py

@@ -8,6 +8,8 @@
 from sanic import Sanic
 from sanic_testing.testing import SanicASGITestClient
 
+from renku_data_services.connected_services.apispec_base import CallbackParams
+from renku_data_services.connected_services.blueprints import OAuth2ClientsBP
 from renku_data_services.data_api.app import register_all_handlers
 from renku_data_services.data_api.dependencies import DependencyManager
 from test.bases.renku_data_services.data_api.utils import create_dummy_oauth_client
@@ -365,6 +367,130 @@ async def test_callback_oauth2_authorization_flow(
     assert token_set.get("access_token") == "ACCESS_TOKEN"
 
 
+@pytest.mark.asyncio
+async def test_callback_oauth2_authorization_flow_error_redirects_to_next_url(
+    oauth2_test_client: SanicASGITestClient, user_headers, create_oauth2_provider
+):
+    provider = await create_oauth2_provider("provider_1")
+    provider_id = provider["id"]
+
+    next_url = "https://example.org/my-ui/callback"
+    qs = f"next_url={quote(next_url)}"
+
+    _, res = await oauth2_test_client.get(
+        f"/api/data/oauth2/providers/{provider_id}/authorize?{qs}", headers=user_headers
+    )
+    assert res.status_code == 302, res.text
+    location = urlparse(res.headers["location"])
+    state = parse_qs(location.query).get("state", [None])[0]
+    assert state
+
+    callback_qs = f"state={quote(state)}&error=access_denied&error_description={quote('User canceled')}"
+    _, res = await oauth2_test_client.get(f"/api/data/oauth2/callback?{callback_qs}")
+
+    assert res.status_code == 302, res.text
+    redirect_location = urlparse(res.headers["location"])
+    assert f"{redirect_location.scheme}://{redirect_location.netloc}{redirect_location.path}" == next_url
+    redirected_query = parse_qs(redirect_location.query)
+    assert redirected_query.get("error", [None])[0] == "access_denied"
+    assert redirected_query.get("error_description", [None])[0] == "User canceled"
+    assert redirected_query.get("state", [None])[0] == state
+
+
+@pytest.mark.asyncio
+async def test_callback_oauth2_authorization_flow_error_preserves_next_url_query(
+    oauth2_test_client: SanicASGITestClient, user_headers, create_oauth2_provider
+):
+    provider = await create_oauth2_provider("provider_1")
+    provider_id = provider["id"]
+
+    next_url = "https://example.org/my-ui/callback?existing=1"
+    qs = f"next_url={quote(next_url)}"
+
+    _, res = await oauth2_test_client.get(
+        f"/api/data/oauth2/providers/{provider_id}/authorize?{qs}", headers=user_headers
+    )
+    assert res.status_code == 302, res.text
+    location = urlparse(res.headers["location"])
+    state = parse_qs(location.query).get("state", [None])[0]
+    assert state
+
+    callback_qs = (
+        f"state={quote(state)}"
+        "&error=access_denied"
+        f"&error_description={quote('User canceled')}"
+        f"&error_uri={quote('https://example.org/oauth/errors/access_denied')}"
+    )
+    _, res = await oauth2_test_client.get(f"/api/data/oauth2/callback?{callback_qs}")
+
+    assert res.status_code == 302, res.text
+    redirect_location = urlparse(res.headers["location"])
+    redirected_query = parse_qs(redirect_location.query)
+    assert redirected_query.get("existing", [None])[0] == "1"
+    assert redirected_query.get("error", [None])[0] == "access_denied"
+    assert redirected_query.get("error_description", [None])[0] == "User canceled"
+    assert redirected_query.get("error_uri", [None])[0] == "https://example.org/oauth/errors/access_denied"
+    assert redirected_query.get("state", [None])[0] == state
+
+
+@pytest.mark.asyncio
+async def test_callback_oauth2_authorization_flow_error_redirects_with_all_optional_params(
+    oauth2_test_client: SanicASGITestClient, user_headers, create_oauth2_provider
+):
+    provider = await create_oauth2_provider("provider_1")
+    provider_id = provider["id"]
+
+    next_url = "https://example.org/my-ui/callback"
+    qs = f"next_url={quote(next_url)}"
+
+    _, res = await oauth2_test_client.get(
+        f"/api/data/oauth2/providers/{provider_id}/authorize?{qs}", headers=user_headers
+    )
+    assert res.status_code == 302, res.text
+    state = parse_qs(urlparse(res.headers["location"]).query).get("state", [None])[0]
+    assert state
+
+    callback_qs = (
+        f"state={quote(state)}"
+        "&error=access_denied"
+        f"&error_description={quote('User canceled')}"
+        f"&error_uri={quote('https://example.org/oauth/errors/access_denied')}"
+        "&code=auth-code-123"
+        "&iss=https%3A%2F%2Fissuer.example.org"
+    )
+    _, res = await oauth2_test_client.get(f"/api/data/oauth2/callback?{callback_qs}")
+
+    assert res.status_code == 302, res.text
+    redirected_query = parse_qs(urlparse(res.headers["location"]).query)
+    assert redirected_query.get("state", [None])[0] == state
+    assert redirected_query.get("error", [None])[0] == "access_denied"
+    assert redirected_query.get("error_description", [None])[0] == "User canceled"
+    assert redirected_query.get("error_uri", [None])[0] == "https://example.org/oauth/errors/access_denied"
+    assert redirected_query.get("code", [None])[0] == "auth-code-123"
+    assert redirected_query.get("iss", [None])[0] == "https://issuer.example.org"
+
+
+def test_append_query_params_returns_original_url_when_no_allowed_values() -> None:
+    next_url = "https://example.org/my-ui/callback?existing=1"
+    params = CallbackParams(state="")
+
+    result = OAuth2ClientsBP._append_query_params(next_url, params)
+
+    assert result == next_url
+
+
+@pytest.mark.asyncio
+async def test_callback_oauth2_authorization_flow_error_without_pending_connection_forbidden(
+    oauth2_test_client: SanicASGITestClient,
+    sanic_client: SanicASGITestClient,
+):
+    _ = sanic_client
+    callback_qs = "state=missing-state&error=access_denied&error_description=No+pending+connection"
+    _, res = await oauth2_test_client.get(f"/api/data/oauth2/callback?{callback_qs}")
+
+    assert res.status_code == 403, res.text
+
+
 @pytest.mark.asyncio
 async def test_get_account(oauth2_test_client: SanicASGITestClient, user_headers, create_oauth2_connection):
     connection = await create_oauth2_connection("provider_1")