Allow OIDC callback URL without query parameters (#729)

* Allow OIDC callback URL without query parameters

* Update apispec

Author: DavidMStraub

gramps_webapi/api/__init__.py

@@ -168,6 +168,11 @@ def register_endpt(resource: Type[Resource], url: str, name: str):
 # OIDC
 register_endpt(OIDCLoginResource, "/oidc/login/", "oidcloginresource")
 register_endpt(OIDCCallbackResource, "/oidc/callback/", "oidccallbackresource")
+register_endpt(
+    OIDCCallbackResource,
+    "/oidc/callback/<string:provider_id>",
+    "oidccallbackresource_provider",
+)
 register_endpt(OIDCConfigResource, "/oidc/config/", "oidcconfigresource")
 register_endpt(OIDCTokenExchangeResource, "/oidc/tokens/", "oidctokenexchangeresource")
 register_endpt(OIDCLogoutResource, "/oidc/logout/", "oidclogoutresource")

gramps_webapi/api/resources/oidc.py

@@ -94,11 +94,10 @@ def get(self, args):
                 500, f"OIDC client for provider '{provider_id}' not found"
             )
 
-        # Build redirect URI with provider parameter using BASE_URL
+        # Build redirect URI with provider in path (Microsoft-compatible)
+        # Using path parameter instead of query parameter for broader compatibility
         base_url = get_config("BASE_URL")
-        redirect_uri = (
-            f"{base_url.rstrip('/')}/api/oidc/callback/?provider={provider_id}"
-        )
+        redirect_uri = f"{base_url.rstrip('/')}/api/oidc/callback/{provider_id}"
 
         authorization_url = oidc_client.authorize_redirect(redirect_uri)
         return authorization_url
@@ -107,13 +106,16 @@ def get(self, args):
 class OIDCCallbackResource(Resource):
     """Resource for handling OIDC callback.
 
-    Endpoint: /api/oidc/callback/
+    Endpoint: /api/oidc/callback/ (legacy with query param)
+    Endpoint: /api/oidc/callback/<provider_id> (path param, Microsoft-compatible)
     """
 
     @limiter.limit("5/minute")
     @use_args(
         {
-            "provider": fields.Str(required=True),
+            "provider": fields.Str(
+                required=False
+            ),  # Optional for backwards compatibility
             "tree": fields.Str(required=False),
             "code": fields.Str(required=False),
             "state": fields.Str(required=False),
@@ -124,12 +126,21 @@ class OIDCCallbackResource(Resource):
         location="query",
         unknown=EXCLUDE,
     )
-    def get(self, args):
-        """Handle OIDC callback and create JWT tokens."""
+    def get(self, args, provider_id=None):
+        """Handle OIDC callback and create JWT tokens.
+
+        Args:
+            args: Query parameters
+            provider_id: Provider ID from path parameter (if using path-based route)
+        """
         if not is_oidc_enabled():
             abort_with_message(405, "OIDC authentication is not enabled")
 
-        provider_id = args.get("provider")
+        # Support both path parameter (new, Microsoft-compatible) and query parameter (legacy)
+        provider_id = provider_id or args.get("provider")
+
+        if not provider_id:
+            abort_with_message(400, "Provider ID is required")
 
         # Validate provider is available
         available_providers = get_available_oidc_providers()

gramps_webapi/data/apispec.yaml

@@ -211,7 +211,8 @@ paths:
     get:
       tags:
       - authentication
-      summary: "Handle OIDC callback and create JWT tokens."
+      summary: "Handle OIDC callback and create JWT tokens (legacy query parameter format)."
+      description: "Legacy endpoint for backwards compatibility. New deployments should use /oidc/callback/{provider_id} instead."
       operationId: oidcCallback
       parameters:
       - name: provider
@@ -255,6 +256,55 @@ paths:
         500:
           description: "Internal Server Error: OIDC client error."
 
+  /oidc/callback/{provider_id}:
+    get:
+      tags:
+      - authentication
+      summary: "Handle OIDC callback and create JWT tokens (path parameter format)."
+      description: "Path-based endpoint compatible with providers that disallow query parameters in redirect URIs. This is the preferred format for new deployments."
+      operationId: oidcCallbackProvider
+      parameters:
+      - name: provider_id
+        in: path
+        required: true
+        type: string
+        description: "The OIDC provider ID (e.g., 'google', 'microsoft', 'github')"
+        example: "microsoft"
+      - name: code
+        in: query
+        required: false
+        type: string
+        description: "Authorization code from OIDC provider"
+      - name: state
+        in: query
+        required: false
+        type: string
+        description: "State parameter from OIDC provider"
+      - name: session_state
+        in: query
+        required: false
+        type: string
+        description: "Session state parameter from OIDC provider"
+      - name: error
+        in: query
+        required: false
+        type: string
+        description: "Error parameter from OIDC provider"
+      - name: error_description
+        in: query
+        required: false
+        type: string
+        description: "Error description from OIDC provider"
+      responses:
+        302:
+          description: "Redirect: Redirects to frontend with JWT tokens."
+        400:
+          description: "Bad Request: Invalid provider or authentication error."
+        405:
+          description: "Method Not Allowed: OIDC authentication is not enabled."
+        500:
+          description: "Internal Server Error: OIDC client error."
+
   /oidc/logout/:
     get:
       tags:

tests/test_endpoints/test_oidc.py

@@ -338,6 +338,134 @@ def test_oidc_callback_missing_code(self):
             # The endpoint should handle missing code parameter gracefully
             # Implementation will determine exact behavior
 
+    @patch("gramps_webapi.api.resources.oidc.is_oidc_enabled", return_value=True)
+    @patch(
+        "gramps_webapi.api.resources.oidc.get_available_oidc_providers",
+        return_value=["microsoft"],
+    )
+    @patch("gramps_webapi.api.resources.oidc.create_or_update_oidc_user")
+    @patch("gramps_webapi.api.resources.oidc.get_name")
+    @patch("gramps_webapi.api.resources.oidc.get_tree_id")
+    @patch("gramps_webapi.api.resources.oidc.get_permissions")
+    @patch("gramps_webapi.api.resources.oidc.is_tree_disabled", return_value=False)
+    @patch("gramps_webapi.api.resources.oidc.get_tokens")
+    def test_oidc_callback_path_param_microsoft(
+        self,
+        mock_get_tokens,
+        mock_tree_disabled,
+        mock_get_permissions,
+        mock_get_tree_id,
+        mock_get_name,
+        mock_create_user,
+        mock_providers,
+        mock_oidc_enabled,
+    ):
+        """Test OIDC callback with path parameter (Microsoft-compatible URL)."""
+        # Mock OAuth client and token exchange
+        mock_oauth = MagicMock()
+        mock_oidc_client = MagicMock()
+        mock_oauth.gramps_microsoft = mock_oidc_client
+
+        # Mock token and userinfo
+        mock_token = {"access_token": "test_token"}
+        mock_userinfo = {
+            "sub": "user123",
+            "preferred_username": "testuser",
+            "email": "[email protected]",
+        }
+        mock_oidc_client.authorize_access_token.return_value = mock_token
+        mock_oidc_client.userinfo.return_value = mock_userinfo
+
+        # Mock user creation and token generation
+        mock_create_user.return_value = "user-guid-123"
+        mock_get_name.return_value = "testuser"
+        mock_get_tree_id.return_value = "test_tree"
+        mock_get_permissions.return_value = {"EditObject"}
+        mock_get_tokens.return_value = {
+            "access_token": "jwt_access_token",
+            "refresh_token": "jwt_refresh_token",
+        }
+
+        # Test path-based URL (no query param for provider)
+        with patch.dict(
+            self.client.application.extensions,
+            {"authlib.integrations.flask_client": mock_oauth},
+            clear=False,
+        ):
+            with patch.dict(self.client.application.config, {"TREE": "test_tree"}):
+                rv = self.client.get(
+                    BASE_URL + "/oidc/callback/microsoft?code=auth_code&state=abc123"
+                )
+                self.assertEqual(rv.status_code, 302)  # Redirect response
+
+                # Verify the flow worked with provider from path
+                mock_create_user.assert_called_once_with(
+                    mock_userinfo, None, "microsoft"
+                )
+
+    @patch("gramps_webapi.api.resources.oidc.is_oidc_enabled", return_value=True)
+    @patch(
+        "gramps_webapi.api.resources.oidc.get_available_oidc_providers",
+        return_value=["google"],
+    )
+    @patch("gramps_webapi.api.resources.oidc.create_or_update_oidc_user")
+    @patch("gramps_webapi.api.resources.oidc.get_name")
+    @patch("gramps_webapi.api.resources.oidc.get_tree_id")
+    @patch("gramps_webapi.api.resources.oidc.get_permissions")
+    @patch("gramps_webapi.api.resources.oidc.is_tree_disabled", return_value=False)
+    @patch("gramps_webapi.api.resources.oidc.get_tokens")
+    def test_oidc_callback_backwards_compatible_query_param(
+        self,
+        mock_get_tokens,
+        mock_tree_disabled,
+        mock_get_permissions,
+        mock_get_tree_id,
+        mock_get_name,
+        mock_create_user,
+        mock_providers,
+        mock_oidc_enabled,
+    ):
+        """Test OIDC callback still works with legacy query parameter."""
+        # Mock OAuth client and token exchange
+        mock_oauth = MagicMock()
+        mock_oidc_client = MagicMock()
+        mock_oauth.gramps_google = mock_oidc_client
+
+        # Mock token and userinfo
+        mock_token = {"access_token": "test_token"}
+        mock_userinfo = {
+            "sub": "user123",
+            "email": "[email protected]",
+        }
+        mock_oidc_client.authorize_access_token.return_value = mock_token
+        mock_oidc_client.userinfo.return_value = mock_userinfo
+
+        # Mock user creation and token generation
+        mock_create_user.return_value = "user-guid-456"
+        mock_get_name.return_value = "testuser"
+        mock_get_tree_id.return_value = "test_tree"
+        mock_get_permissions.return_value = {"ViewPrivate"}
+        mock_get_tokens.return_value = {
+            "access_token": "jwt_access_token",
+            "refresh_token": "jwt_refresh_token",
+        }
+
+        # Test legacy query-param based URL (backwards compatibility)
+        with patch.dict(
+            self.client.application.extensions,
+            {"authlib.integrations.flask_client": mock_oauth},
+            clear=False,
+        ):
+            with patch.dict(self.client.application.config, {"TREE": "test_tree"}):
+                rv = self.client.get(
+                    BASE_URL
+                    + "/oidc/callback/?provider=google&code=auth_code&state=abc123"
+                )
+                self.assertEqual(rv.status_code, 302)  # Redirect response
+
+                # Verify the flow worked with provider from query param
+                mock_create_user.assert_called_once_with(mock_userinfo, None, "google")
+
     @patch("gramps_webapi.api.resources.oidc.is_oidc_enabled", return_value=True)
     @patch(
         "gramps_webapi.api.resources.oidc.get_available_oidc_providers",