fix(spp_api_v2): address review comments on OAuth endpoint

- Move import base64 to top-level imports
- Add debug logging to Basic Auth decode errors
- Add debug logging to broad except in JSON body parsing
- Remove duplicate token_lifetime_hours fetch from _generate_jwt_token by passing it as a parameter
- Add tests for HTTP Basic Auth and form-encoded body authentication

Author: jeremi

spp_api_v2/routers/oauth.py

@@ -1,6 +1,7 @@
 # Part of OpenSPP. See LICENSE file for full copyright and licensing details.
 """OAuth 2.0 endpoints for API V2"""
 
+import base64
 import logging
 import os
 from datetime import datetime, timedelta
@@ -53,14 +54,12 @@ async def _parse_token_request(http_request: Request) -> TokenRequest:
     basic_client_secret = ""
     auth_header = http_request.headers.get("authorization", "")
     if auth_header.startswith("Basic "):
-        import base64
-
         try:
             decoded = base64.b64decode(auth_header[6:]).decode("utf-8")
             if ":" in decoded:
                 basic_client_id, basic_client_secret = decoded.split(":", 1)
-        except (ValueError, UnicodeDecodeError):
-            pass
+        except (ValueError, UnicodeDecodeError) as e:
+            _logger.debug("Failed to decode Basic Auth header: %s", e)
 
     content_type = http_request.headers.get("content-type", "")
     if "application/x-www-form-urlencoded" in content_type:
@@ -75,8 +74,8 @@ async def _parse_token_request(http_request: Request) -> TokenRequest:
     try:
         body = await http_request.json()
         return TokenRequest(**body)
-    except Exception:
-        pass
+    except Exception as e:
+        _logger.debug("Could not parse token request from JSON body, falling back: %s", e)
 
     # Fall back to Basic Auth only (grant_type defaults to client_credentials)
     if basic_client_id:
@@ -126,9 +125,14 @@ async def get_token(
             detail="Invalid client credentials",
         )
 
+    # Read configurable token lifetime (default: 24 hours for long-lived sessions)
+    # nosemgrep: odoo-sudo-without-context
+    token_lifetime_hours = int(env["ir.config_parameter"].sudo().get_param("spp_api_v2.token_lifetime_hours", "24"))
+    expires_in = token_lifetime_hours * 3600
+
     # Generate JWT token
     try:
-        token = _generate_jwt_token(env, api_client)
+        token = _generate_jwt_token(env, api_client, token_lifetime_hours)
     except Exception as e:
         _logger.exception("Error generating JWT token")
         raise HTTPException(
@@ -139,11 +143,6 @@ async def get_token(
     # Build scope string from client scopes
     scope_str = " ".join(f"{s.resource}:{s.action}" for s in api_client.scope_ids)
 
-    # Read configurable token lifetime (default: 24 hours for long-lived sessions)
-    # nosemgrep: odoo-sudo-without-context
-    token_lifetime_hours = int(env["ir.config_parameter"].sudo().get_param("spp_api_v2.token_lifetime_hours", "24"))
-    expires_in = token_lifetime_hours * 3600
-
     return TokenResponse(
         access_token=token,
         token_type="Bearer",
@@ -189,14 +188,14 @@ def _get_jwt_secret(env: Environment) -> str:
     return secret
 
 
-def _generate_jwt_token(env: Environment, api_client) -> str:
+def _generate_jwt_token(env: Environment, api_client, token_lifetime_hours: int) -> str:
     """
     Generate JWT access token for API client.
 
     Token contains:
     - client_id (external identifier, NOT database ID)
     - scopes
-    - expiration (1 hour)
+    - expiration time determined by token_lifetime_hours
 
     SECURITY: Never include database IDs in JWT.
     The auth middleware loads the full api_client record from DB using client_id.
@@ -215,10 +214,6 @@ def _generate_jwt_token(env: Environment, api_client) -> str:
     # Build payload
     # SECURITY: Never include database IDs in JWT - use client_id only
     # The auth middleware looks up the full api_client record using client_id
-    # Read configurable token lifetime (default: 24 hours)
-    # nosemgrep: odoo-sudo-without-context
-    token_lifetime_hours = int(env["ir.config_parameter"].sudo().get_param("spp_api_v2.token_lifetime_hours", "24"))
-
     now = datetime.utcnow()
     payload = {
         "iss": "openspp-api-v2",  # Issuer

spp_api_v2/tests/test_oauth.py

@@ -1,7 +1,9 @@
 # Part of OpenSPP. See LICENSE file for full copyright and licensing details.
 """Tests for OAuth endpoints"""
 
+import base64
 import json
+from urllib.parse import urlencode
 
 from ..middleware.rate_limit import get_rate_limiter
 from .common import ApiV2HttpTestCase
@@ -218,6 +220,53 @@ def test_client_last_used_date_updated(self):
         self.client.invalidate_recordset()
         self.assertTrue(self.client.last_used_date)
 
+    def test_token_generation_basic_auth(self):
+        """HTTP Basic Auth header returns access token"""
+        credentials = base64.b64encode(f"{self.client.client_id}:{self.client.client_secret}".encode()).decode("utf-8")
+
+        response = self.url_open(
+            self.url,
+            data="",
+            headers={
+                "Content-Type": "application/x-www-form-urlencoded",
+                "Authorization": f"Basic {credentials}",
+            },
+        )
+
+        self.assertEqual(response.status_code, 200)
+
+        data = json.loads(response.content)
+        self.assertIn("access_token", data)
+        self.assertEqual(data["token_type"], "Bearer")
+        self.assertIn("expires_in", data)
+        self.assertIn("scope", data)
+
+    def test_token_generation_form_encoded(self):
+        """Form-encoded body (application/x-www-form-urlencoded) returns access token"""
+        body = urlencode(
+            {
+                "grant_type": "client_credentials",
+                "client_id": self.client.client_id,
+                "client_secret": self.client.client_secret,
+            }
+        )
+
+        response = self.url_open(
+            self.url,
+            data=body,
+            headers={"Content-Type": "application/x-www-form-urlencoded"},
+        )
+
+        self.assertEqual(response.status_code, 200)
+
+        data = json.loads(response.content)
+        self.assertIn("access_token", data)
+        self.assertEqual(data["token_type"], "Bearer")
+        self.assertIn("expires_in", data)
+        self.assertIn("scope", data)
+        self.assertIn("individual:read", data["scope"])
+        self.assertIn("group:search", data["scope"])
+
     def test_token_no_scopes(self):
         """Client with no scopes still gets token but empty scope string"""
         # Create client without scopes